Ba 1914 baseapp activity log list page #162
Conversation
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
WalkthroughThis pull request introduces a comprehensive activity log module for the frontend application. The module includes a new Changes
Sequence DiagramsequenceDiagram
participant User
participant ActivityLogComponent
participant GraphQL
participant LogGroups
User->>ActivityLogComponent: Open Activity Log
ActivityLogComponent->>GraphQL: Fetch Initial Logs
GraphQL-->>ActivityLogComponent: Return Log Data
ActivityLogComponent->>LogGroups: Render Log Groups
User->>ActivityLogComponent: Apply Filters/Search
ActivityLogComponent->>GraphQL: Refetch Filtered Logs
GraphQL-->>ActivityLogComponent: Return Filtered Logs
ActivityLogComponent->>LogGroups: Update Log Groups
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between 35671ff84488505b706546edfb1a7846d808e4ae and 58e4ced. ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (18)
🚧 Files skipped from review as they are similar to previous changes (18)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
junioresc
requested review from
nossila,
rafaelhdr,
pablogadhi,
anicioalexandre,
deboracosilveira,
mathieubouhelier,
pt-tsl and
diegoMCampos
and removed request for
diegoMCampos
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (9)
packages/components/modules/activity-log/DateRangeChip/types.ts (1)
1-4: Consider enhancing type safety and documentation.While the interface is well-structured, consider these improvements:
- Add JSDoc comments to document the purpose of the interface and its props
- Consider using a more specific type for the date range to prevent accidental prop misuse
+/** Props for the DateRangeChip component that handles date range selection */ export interface DateRangeChipProps { + /** Current date range value as [startDate, endDate]. Both dates can be null when no range is selected */ value: [Date | null, Date | null] + /** Callback fired when the date range changes */ onChange: (newValue: [Date | null, Date | null]) => void }packages/components/modules/activity-log/ActivityUserSearchBar/types.ts (1)
1-6: Consider adding search-specific props and documentation.The interface could be enhanced with additional props that are common in search components:
import { ChangeEvent } from 'react' +/** Props for the ActivityUserSearchBar component that handles user search functionality */ export interface ActivityUserSearchBarProps { + /** Current search input value */ value: string + /** Callback fired when the search input changes */ onChange: (event: ChangeEvent<HTMLInputElement>) => void + /** Optional placeholder text for the search input */ + placeholder?: string + /** Optional debounce delay in milliseconds */ + debounceMs?: number + /** Optional loading state for async search operations */ + isLoading?: boolean }packages/components/modules/activity-log/graphql/queries/ActivityLogs.ts (1)
3-7: LGTM! Consider adding documentation.The query follows Relay best practices with proper fragment spreading and pagination parameters. Consider adding JSDoc comments to document the query's purpose and parameter usage.
+/** + * GraphQL query for fetching paginated activity logs + * @param first - Number of items to fetch + * @param after - Cursor for pagination + */ export const ActivityLogsQuery = graphql` query ActivityLogsQuery($first: Int, $after: String) {packages/components/modules/activity-log/LogGroups/types.ts (1)
21-34: Add documentation for nullable fields and verb values.The
Loginterface would benefit from:
- Documentation explaining when URL and user can be null
- A union type for valid verb values to prevent invalid verbs
+/** Represents a single activity log entry */ export interface Log { id: string createdAt: string - verb: string + /** The type of activity performed. */ + verb: 'comments.add_comment' | 'comments.edit_comment' | 'comments.delete_comment' | 'comments.reply_comment' /** URL associated with the activity, null for activities without a direct link */ url: string | null /** User who performed the activity, null for system-generated activities */ user: { id: string fullName: string | null email: string | null avatar: { url: string } | null } | null }packages/components/modules/activity-log/LogItem/index.tsx (1)
11-16: Move verbMapping to a shared constants file.The
verbMappingobject could be reused across components and should be moved to a shared location. Also, consider using an enum or const assertion for type safety.-const verbMapping: { [key: string]: string } = { +// In shared/constants/activityLog.ts +export const ACTIVITY_VERBS = { + ADD_COMMENT: 'comments.add_comment', + EDIT_COMMENT: 'comments.edit_comment', + DELETE_COMMENT: 'comments.delete_comment', + REPLY_COMMENT: 'comments.reply_comment', +} as const + +export const VERB_DISPLAY_TEXT: Record<typeof ACTIVITY_VERBS[keyof typeof ACTIVITY_VERBS], string> = { 'comments.add_comment': 'Created a comment', 'comments.edit_comment': 'Edited a comment', 'comments.delete_comment': 'Deleted a comment', 'comments.reply_comment': 'Replied to a comment', -}packages/components/modules/activity-log/EventFilterChip/index.tsx (1)
7-9: Consider adding loading and error states.The component should handle loading and error states for a better user experience.
-const EventFilterChip: FC<EventFilterChipProps> = ({ options, selectedOptions, onChange }) => { +const EventFilterChip: FC<EventFilterChipProps> = ({ + options, + selectedOptions, + onChange, + isLoading = false, + error = null +}) => { const [anchorEl, setAnchorEl] = useState<null | HTMLElement>(null)packages/components/modules/activity-log/ActivityLogComponent/index.tsx (1)
24-27: Consider making the page size configurableThe hard-coded value of 10 for pagination count should be extracted to a constant or configuration value for better maintainability.
+const DEFAULT_PAGE_SIZE = 10; + const handleSearch = (e: React.ChangeEvent<HTMLInputElement>) => { setSearchTerm(e.target.value) - refetch({ count: 10, cursor: null }) + refetch({ count: DEFAULT_PAGE_SIZE, cursor: null }) }packages/components/modules/activity-log/LogGroups/index.tsx (2)
1-17: LGTM! Good use of virtualization for performance.Consider adding prop types validation for better runtime safety.
+import PropTypes from 'prop-types'; + const LogGroups: FC<LogGroupsProps> = ({ ... }) => { // ... component implementation } + +LogGroups.propTypes = { + logGroups: PropTypes.arrayOf(PropTypes.shape({ + lastActivityTimestamp: PropTypes.string.isRequired, + logs: PropTypes.arrayOf(PropTypes.object).isRequired + })).isRequired, + loadNext: PropTypes.func.isRequired, + hasNext: PropTypes.bool.isRequired, + isLoadingNext: PropTypes.bool.isRequired +};
74-78: Maintain consistency in pagination configurationThe page size (10) should use the same configuration constant as suggested in ActivityLogComponent.
endReached={() => { if (hasNext) { - loadNext(10) + loadNext(DEFAULT_PAGE_SIZE) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 3c2ea87 and 62d562f199ec2772ff8e78d96185a98f4c8ae3bf.
⛔ Files ignored due to path filters (3)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (16)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/types.ts(1 hunks)packages/components/modules/activity-log/ActivityUserSearchBar/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityUserSearchBar/types.ts(1 hunks)packages/components/modules/activity-log/DateRangeChip/index.tsx(1 hunks)packages/components/modules/activity-log/DateRangeChip/types.ts(1 hunks)packages/components/modules/activity-log/EventFilterChip/index.tsx(1 hunks)packages/components/modules/activity-log/EventFilterChip/types.ts(1 hunks)packages/components/modules/activity-log/LogGroups/index.tsx(1 hunks)packages/components/modules/activity-log/LogGroups/types.ts(1 hunks)packages/components/modules/activity-log/LogItem/index.tsx(1 hunks)packages/components/modules/activity-log/LogItem/types.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogs.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts(1 hunks)packages/components/modules/activity-log/index.ts(1 hunks)packages/components/schema.graphql(59 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/modules/activity-log/index.ts
🔇 Additional comments (10)
packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts (2)
9-36: GraphQL Fragment Definition Looks Good
The ActivityLogsFragmentQuery is well-defined with appropriate fields and argument definitions, aligning correctly with the schema.
38-82: useActivityLogs Hook Implementation is Solid
The custom hook correctly utilizes usePaginationFragment for data fetching and implements proper memoization and grouping logic for the activity logs.
packages/components/schema.graphql (1)
7-41: ActivityLog Type and Connections Defined Properly
The ActivityLog type and its associated connection and edge types are well-structured and align with GraphQL best practices.
packages/components/modules/activity-log/LogItem/types.ts (1)
1-5: LogItemProps Interface Defined Correctly
The LogItemProps interface is properly defined, and the import statement accurately references the Log type.
packages/components/modules/activity-log/EventFilterChip/types.ts (1)
1-5: EventFilterChipProps Interface Structure is Appropriate
The EventFilterChipProps interface is well-structured, with correctly typed properties that accurately represent the expected data.
packages/components/modules/activity-log/ActivityLogComponent/types.ts (1)
1-5:
Add missing prop types for required functionality.
Based on the PR objectives, the component needs to handle several features that aren't reflected in the current type definition:
import { ActivityLogsPaginationQuery$data } from '../../../__generated__/ActivityLogsPaginationQuery.graphql'
+/** Props for the ActivityLogComponent that displays paginated activity logs */
export interface ActivityLogComponentProps {
+ /** GraphQL query reference for activity log data */
queryRef: ActivityLogsPaginationQuery$data | null
+ /** Optional initial page size for pagination */
+ pageSize?: number
+ /** Optional callback when an activity log item is clicked */
+ onActivityClick?: (activityId: string) => void
+ /** Optional error handler for failed data fetching */
+ onError?: (error: Error) => void
}
+/** Configuration for activity log grouping */
+export interface ActivityLogGroupConfig {
+ /** Time window in minutes for grouping activities by user */
+ groupingWindowMinutes: number
+}The current type definition lacks props for:
- Pagination configuration (required by PR objectives)
- Error handling for failed data fetching
- Click handlers for comment links
- Grouping configuration (15-minute window mentioned in objectives)
Let's verify if these features are implemented in the component:
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (3)
1-14: LGTM! Well-structured imports and type definitions.
The component is properly marked as client-side and imports all necessary dependencies.
29-50: LGTM! Clean and well-structured render logic.
The component layout is well-organized and properly passes all necessary props to child components.
20-22: Verify type casting safety
The type casting of queryRef to ActivityLogsFragment$key could potentially cause runtime issues if the wrong type is passed.
✅ Verification successful
Type casting is safe in this context
The type casting of queryRef is actually safe and intentional. The code follows the Relay pagination pattern where:
- The component accepts
ActivityLogsPaginationQuery$datathrough props - The
useActivityLogshook expectsActivityLogsFragment$keywhich is a fragment reference type - The casting is necessary because the query data contains the fragment reference (via
$fragmentSpreads)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of ActivityLogComponent to verify correct prop passing
ast-grep --pattern 'ActivityLogComponent $props'
Length of output: 50
Script:
#!/bin/bash
# Search for usages of ActivityLogComponent with a more flexible pattern
rg "ActivityLogComponent" -A 3
# Also search for the type definition to understand the expected prop type
rg "ActivityLogsFragment\$key" -A 3
# Search for any imports or exports of ActivityLogComponent
rg "import.*ActivityLogComponent|export.*ActivityLogComponent"
Length of output: 2187
Script:
#!/bin/bash
# Let's check the type definition of useActivityLogs to understand the expected type
rg "useActivityLogs" -A 3
# Also search for ActivityLogsPaginationQuery type to understand the relationship
rg "ActivityLogsPaginationQuery" -A 3
# And check for any type definitions related to ActivityLogsFragment
ast-grep --pattern 'type ActivityLogsFragment'
Length of output: 7363
packages/components/modules/activity-log/LogGroups/index.tsx (1)
24-35: LGTM! Well-implemented loading state with good accessibility.
The loading state implementation is clean and properly handles customization through props.
| const log = edge?.node as Log | ||
| const userId = log?.user?.id as string | ||
| const timestamp = new Date(log.createdAt).getTime() |
There was a problem hiding this comment.
Handle Potential Undefined User IDs in Logs
If log.user or log.user.id is undefined, userId will be undefined, leading to logs being grouped under an undefined key, which might cause unexpected behavior. Consider adding a check to handle logs without user IDs appropriately.
Apply the following diff to handle undefined user IDs:
const log = edge?.node as Log
-const userId = log?.user?.id as string
+const userId = log?.user?.id
+if (!userId) {
+ // Handle logs with undefined user IDs appropriately
+ // For example, you might skip these logs or group them under a special key
+ return
+}Committable suggestion skipped: line range outside the PR's diff.
packages/components/schema.graphql
Outdated
| interface NodeActivityLogInterface { | ||
| nodeActivityLogs(visibility: VisibilityTypes, first: Int = 10, offset: Int, before: String, after: String, last: Int, createdFrom: Date, createdTo: Date, userPk: Decimal, profilePk: Decimal): ActivityLogConnection | ||
| } |
There was a problem hiding this comment.
Ensure Consistent Parameter Types in NodeActivityLogInterface
The userPk and profilePk parameters in the nodeActivityLogs field are typed as Decimal. To maintain consistency with other pk fields typed as Int!, consider changing these to Int.
Apply this diff to correct the parameter types:
nodeActivityLogs(
visibility: VisibilityTypes,
first: Int = 10,
offset: Int,
before: String,
after: String,
last: Int,
createdFrom: Date,
createdTo: Date,
- userPk: Decimal,
- profilePk: Decimal
+ userPk: Int,
+ profilePk: Int
): ActivityLogConnection📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface NodeActivityLogInterface { | |
| nodeActivityLogs(visibility: VisibilityTypes, first: Int = 10, offset: Int, before: String, after: String, last: Int, createdFrom: Date, createdTo: Date, userPk: Decimal, profilePk: Decimal): ActivityLogConnection | |
| } | |
| interface NodeActivityLogInterface { | |
| nodeActivityLogs(visibility: VisibilityTypes, first: Int = 10, offset: Int, before: String, after: String, last: Int, createdFrom: Date, createdTo: Date, userPk: Int, profilePk: Int): ActivityLogConnection | |
| } |
packages/components/schema.graphql
Outdated
| cursor: String! | ||
| } | ||
|
|
||
| type Query { | ||
| activityLogs(visibility: VisibilityTypes, first: Int = 10, offset: Int, before: String, after: String, last: Int, createdFrom: Date, createdTo: Date, userPk: Decimal, profilePk: Decimal): ActivityLogConnection |
There was a problem hiding this comment.
Review Data Types of userPk and profilePk Parameters
The parameters userPk and profilePk in the activityLogs query are currently typed as Decimal. Since primary keys (pk) are of type Int!, consider changing these to Int for consistency and to prevent potential type mismatches.
Apply this diff to correct the parameter types:
activityLogs(
visibility: VisibilityTypes,
first: Int = 10,
offset: Int,
before: String,
after: String,
last: Int,
createdFrom: Date,
createdTo: Date,
- userPk: Decimal,
- profilePk: Decimal
+ userPk: Int,
+ profilePk: Int
): ActivityLogConnection📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| activityLogs(visibility: VisibilityTypes, first: Int = 10, offset: Int, before: String, after: String, last: Int, createdFrom: Date, createdTo: Date, userPk: Decimal, profilePk: Decimal): ActivityLogConnection | |
| activityLogs(visibility: VisibilityTypes, first: Int = 10, offset: Int, before: String, after: String, last: Int, createdFrom: Date, createdTo: Date, userPk: Int, profilePk: Int): ActivityLogConnection |
| <TextField | ||
| fullWidth | ||
| variant="outlined" | ||
| placeholder="Search..." | ||
| value={value} | ||
| onChange={onChange} | ||
| /> | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility improvements and optimize search performance.
The search bar component needs the following improvements:
- Add proper ARIA labels for accessibility
- Implement debounce to prevent excessive re-renders during typing
- Add loading state handling for search operations
const ActivityUserSearchBar: FC<ActivityUserSearchBarProps> = ({ value, onChange }) => (
<TextField
fullWidth
variant="outlined"
placeholder="Search..."
value={value}
onChange={onChange}
+ aria-label="Search users"
+ inputProps={{
+ 'aria-description': 'Enter text to search for users in activity log'
+ }}
/>
)Consider implementing debounce:
import { useCallback } from 'react';
import debounce from 'lodash/debounce';
const debouncedOnChange = useCallback(
debounce((value: string) => {
onChange(value);
}, 300),
[onChange]
);| const DateRangeChip: FC<DateRangeChipProps> = ({ value, onChange }) => ( | ||
| <DateRangePicker | ||
| startText="Start" | ||
| endText="End" | ||
| value={value} | ||
| onChange={onChange} | ||
| renderInput={( | ||
| startProps: DateRangePickerProps['renderInput'], | ||
| endProps: DateRangePickerProps['renderInput'], | ||
| ) => <Chip label={`From: ${startProps.inputProps.value} To: ${endProps.inputProps.value}`} />} | ||
| /> | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and error handling for the DateRangePicker.
The component should handle invalid date ranges and provide feedback to users.
const DateRangeChip: FC<DateRangeChipProps> = ({ value, onChange }) => (
<DateRangePicker
startText="Start"
endText="End"
value={value}
onChange={onChange}
+ minDate={new Date(2020, 0, 1)}
+ maxDate={new Date()}
+ shouldDisableDate={(date) => {
+ return date > new Date();
+ }}
+ onError={(error) => {
+ console.error('Date range error:', error);
+ }}
renderInput={...}
/>
)Committable suggestion skipped: line range outside the PR's diff.
| renderInput={( | ||
| startProps: DateRangePickerProps['renderInput'], | ||
| endProps: DateRangePickerProps['renderInput'], | ||
| ) => <Chip label={`From: ${startProps.inputProps.value} To: ${endProps.inputProps.value}`} />} | ||
| /> |
There was a problem hiding this comment.
Fix date formatting and add error handling.
The current implementation has several issues:
- Direct string interpolation of dates without proper formatting
- Missing error handling for invalid dates
- Accessibility improvements needed
renderInput={(
startProps: DateRangePickerProps['renderInput'],
endProps: DateRangePickerProps['renderInput'],
-) => <Chip label={`From: ${startProps.inputProps.value} To: ${endProps.inputProps.value}`} />}
+) => {
+ const formatDate = (date: string) => {
+ try {
+ return new Date(date).toLocaleDateString();
+ } catch {
+ return 'Invalid date';
+ }
+ };
+
+ return (
+ <Chip
+ label={`From: ${formatDate(startProps.inputProps.value)} To: ${formatDate(endProps.inputProps.value)}`}
+ aria-label={`Date range from ${formatDate(startProps.inputProps.value)} to ${formatDate(endProps.inputProps.value)}`}
+ />
+ );
+}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| renderInput={( | |
| startProps: DateRangePickerProps['renderInput'], | |
| endProps: DateRangePickerProps['renderInput'], | |
| ) => <Chip label={`From: ${startProps.inputProps.value} To: ${endProps.inputProps.value}`} />} | |
| /> | |
| renderInput={( | |
| startProps: DateRangePickerProps['renderInput'], | |
| endProps: DateRangePickerProps['renderInput'], | |
| ) => { | |
| const formatDate = (date: string) => { | |
| try { | |
| return new Date(date).toLocaleDateString(); | |
| } catch { | |
| return 'Invalid date'; | |
| } | |
| }; | |
| return ( | |
| <Chip | |
| label={`From: ${formatDate(startProps.inputProps.value)} To: ${formatDate(endProps.inputProps.value)}`} | |
| aria-label={`Date range from ${formatDate(startProps.inputProps.value)} to ${formatDate(endProps.inputProps.value)}`} | |
| /> | |
| ); | |
| }} | |
| /> |
| LoadingStateProps?: any | ||
| VirtuosoProps?: Partial<VirtuosoProps<any, any>> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using 'any' type for LoadingStateProps.
Consider creating a specific interface for loading state props to improve type safety.
- LoadingStateProps?: any
+ LoadingStateProps?: {
+ // Add specific loading state properties here
+ isLoading?: boolean;
+ message?: string;
+ }Committable suggestion skipped: line range outside the PR's diff.
| const handleToggle = (option: string) => { | ||
| const currentIndex = selectedOptions.indexOf(option) | ||
| const newSelectedOptions = [...selectedOptions] | ||
|
|
||
| if (currentIndex === -1) { | ||
| newSelectedOptions.push(option) | ||
| } else { | ||
| newSelectedOptions.splice(currentIndex, 1) | ||
| } | ||
|
|
||
| onChange(newSelectedOptions) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize array operations and memoize handlers.
The handleToggle function creates a new array on every call and could be optimized.
+import { useCallback } from 'react'
+
const EventFilterChip: FC<EventFilterChipProps> = ({ options, selectedOptions, onChange }) => {
- const handleToggle = (option: string) => {
+ const handleToggle = useCallback((option: string) => {
const currentIndex = selectedOptions.indexOf(option)
- const newSelectedOptions = [...selectedOptions]
-
- if (currentIndex === -1) {
- newSelectedOptions.push(option)
- } else {
- newSelectedOptions.splice(currentIndex, 1)
- }
-
+ const newSelectedOptions = currentIndex === -1
+ ? [...selectedOptions, option]
+ : selectedOptions.filter((_, index) => index !== currentIndex)
+
onChange(newSelectedOptions)
- }
+ }, [selectedOptions, onChange])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleToggle = (option: string) => { | |
| const currentIndex = selectedOptions.indexOf(option) | |
| const newSelectedOptions = [...selectedOptions] | |
| if (currentIndex === -1) { | |
| newSelectedOptions.push(option) | |
| } else { | |
| newSelectedOptions.splice(currentIndex, 1) | |
| } | |
| onChange(newSelectedOptions) | |
| } | |
| import { useCallback } from 'react' | |
| const handleToggle = useCallback((option: string) => { | |
| const currentIndex = selectedOptions.indexOf(option) | |
| const newSelectedOptions = currentIndex === -1 | |
| ? [...selectedOptions, option] | |
| : selectedOptions.filter((_, index) => index !== currentIndex) | |
| onChange(newSelectedOptions) | |
| }, [selectedOptions, onChange]) |
| return ( | ||
| <> | ||
| <Chip label="Filter" onClick={handleClick} /> | ||
| <Menu anchorEl={anchorEl} open={Boolean(anchorEl)} onClose={handleClose}> | ||
| {options.map((option) => ( | ||
| <MenuItem key={option} onClick={() => handleToggle(option)}> | ||
| <Checkbox checked={selectedOptions.indexOf(option) > -1} /> | ||
| <ListItemText primary={option} /> | ||
| </MenuItem> | ||
| ))} | ||
| </Menu> | ||
| </> | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility and keyboard navigation.
The component needs proper ARIA labels and keyboard navigation support.
return (
<>
- <Chip label="Filter" onClick={handleClick} />
+ <Chip
+ label="Filter"
+ onClick={handleClick}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ handleClick(e)
+ }
+ }}
+ role="button"
+ aria-haspopup="true"
+ aria-expanded={Boolean(anchorEl)}
+ tabIndex={0}
+ />
<Menu
anchorEl={anchorEl}
open={Boolean(anchorEl)}
onClose={handleClose}
+ role="listbox"
+ aria-label="Filter options"
>
{options.map((option) => (
<MenuItem key={option} onClick={() => handleToggle(option)}>
<Checkbox checked={selectedOptions.indexOf(option) > -1} />
- <ListItemText primary={option} />
+ <ListItemText
+ primary={option}
+ aria-selected={selectedOptions.indexOf(option) > -1}
+ />
</MenuItem>
))}
</Menu>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <> | |
| <Chip label="Filter" onClick={handleClick} /> | |
| <Menu anchorEl={anchorEl} open={Boolean(anchorEl)} onClose={handleClose}> | |
| {options.map((option) => ( | |
| <MenuItem key={option} onClick={() => handleToggle(option)}> | |
| <Checkbox checked={selectedOptions.indexOf(option) > -1} /> | |
| <ListItemText primary={option} /> | |
| </MenuItem> | |
| ))} | |
| </Menu> | |
| </> | |
| ) | |
| return ( | |
| <> | |
| <Chip | |
| label="Filter" | |
| onClick={handleClick} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| handleClick(e) | |
| } | |
| }} | |
| role="button" | |
| aria-haspopup="true" | |
| aria-expanded={Boolean(anchorEl)} | |
| tabIndex={0} | |
| /> | |
| <Menu | |
| anchorEl={anchorEl} | |
| open={Boolean(anchorEl)} | |
| onClose={handleClose} | |
| role="listbox" | |
| aria-label="Filter options" | |
| > | |
| {options.map((option) => ( | |
| <MenuItem key={option} onClick={() => handleToggle(option)}> | |
| <Checkbox checked={selectedOptions.indexOf(option) > -1} /> | |
| <ListItemText | |
| primary={option} | |
| aria-selected={selectedOptions.indexOf(option) > -1} | |
| /> | |
| </MenuItem> | |
| ))} | |
| </Menu> | |
| </> | |
| ) |
| const renderLogItem = (log: any) => { | ||
| if (!log) return null | ||
|
|
||
| return <LogItem key={log.id} log={log} /> | ||
| } |
There was a problem hiding this comment.
Fix type safety issue in renderLogItem
The use of any type and lack of type checking could lead to runtime errors.
- const renderLogItem = (log: any) => {
+ const renderLogItem = (log: Log) => {
if (!log) return null
return <LogItem key={log.id} log={log} />
}Committable suggestion skipped: line range outside the PR's diff.
junioresc
force-pushed
the
BA-1914-baseapp-activity-log-list-page
branch
from
62d562f to
bae0029
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/components/modules/activity-log/LogItem/index.tsx (1)
11-18: Improve verb mapping implementationThe current implementation could be improved in several ways:
- Use TypeScript enums or constants for verbs to prevent typos
- Move mapping to a separate configuration file for better maintainability
- Add typing for the verb strings
Consider this implementation:
// types.ts export enum ActivityVerb { ADD_COMMENT = 'comments.add_comment', EDIT_COMMENT = 'comments.edit_comment', DELETE_COMMENT = 'comments.delete_comment', REPLY_COMMENT = 'comments.reply_comment', } // verbMapping.ts export const verbMapping: Record<ActivityVerb, string> = { [ActivityVerb.ADD_COMMENT]: 'Created a comment', [ActivityVerb.EDIT_COMMENT]: 'Edited a comment', [ActivityVerb.DELETE_COMMENT]: 'Deleted a comment', [ActivityVerb.REPLY_COMMENT]: 'Replied to a comment', }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 62d562f199ec2772ff8e78d96185a98f4c8ae3bf and bae0029cc0133b69982ad00308e47942c3f6dec1.
⛔ Files ignored due to path filters (3)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (16)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/types.ts(1 hunks)packages/components/modules/activity-log/ActivityUserSearchBar/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityUserSearchBar/types.ts(1 hunks)packages/components/modules/activity-log/DateRangeChip/index.tsx(1 hunks)packages/components/modules/activity-log/DateRangeChip/types.ts(1 hunks)packages/components/modules/activity-log/EventFilterChip/index.tsx(1 hunks)packages/components/modules/activity-log/EventFilterChip/types.ts(1 hunks)packages/components/modules/activity-log/LogGroups/index.tsx(1 hunks)packages/components/modules/activity-log/LogGroups/types.ts(1 hunks)packages/components/modules/activity-log/LogItem/index.tsx(1 hunks)packages/components/modules/activity-log/LogItem/types.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogs.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts(1 hunks)packages/components/modules/activity-log/index.ts(1 hunks)packages/components/schema.graphql(64 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/components/modules/activity-log/DateRangeChip/types.ts
- packages/components/modules/activity-log/graphql/queries/ActivityLogs.ts
- packages/components/modules/activity-log/EventFilterChip/types.ts
- packages/components/modules/activity-log/LogItem/types.ts
- packages/components/modules/activity-log/ActivityUserSearchBar/types.ts
- packages/components/modules/activity-log/ActivityLogComponent/types.ts
- packages/components/modules/activity-log/DateRangeChip/index.tsx
- packages/components/modules/activity-log/LogGroups/types.ts
- packages/components/modules/activity-log/ActivityLogComponent/index.tsx
- packages/components/modules/activity-log/ActivityUserSearchBar/index.tsx
- packages/components/modules/activity-log/index.ts
- packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts
- packages/components/modules/activity-log/EventFilterChip/index.tsx
- packages/components/modules/activity-log/LogGroups/index.tsx
- packages/components/schema.graphql
🔇 Additional comments (1)
packages/components/modules/activity-log/LogItem/index.tsx (1)
20-26: Add accessibility improvements and extract styles
The component needs accessibility improvements and should use theme-based styling.
| interface LogItemProps { | ||
| log: Log | ||
| } |
There was a problem hiding this comment.
Expand LogItemProps interface to match requirements
The current interface only includes the log property, but according to the requirements, we need to display user information, avatar, and timestamp.
Consider expanding the interface:
interface LogItemProps {
log: Log
+ user: {
+ name: string
+ avatarUrl?: string
+ }
+ timestamp: string
+ isLastInGroup?: boolean
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface LogItemProps { | |
| log: Log | |
| } | |
| interface LogItemProps { | |
| log: Log | |
| user: { | |
| name: string | |
| avatarUrl?: string | |
| } | |
| timestamp: string | |
| isLastInGroup?: boolean | |
| } |
| const LogItem: FC<LogItemProps> = ({ log }) => ( | ||
| <Box display="flex" alignItems="center" borderLeft="1px solid #000" marginLeft="20px"> | ||
| <Typography ml="30px" variant="body2"> | ||
| {getDisplayText(log.verb)} | ||
| </Typography> | ||
| </Box> | ||
| ) |
There was a problem hiding this comment.
Implement missing UI requirements
The current implementation is missing several key requirements:
- User avatar and name display
- Timestamp
- Hyperlinked comment text
- Proper grouping structure
Here's a suggested implementation:
-const LogItem: FC<LogItemProps> = ({ log }) => (
+const LogItem: FC<LogItemProps> = ({ log, user, timestamp, isLastInGroup }) => (
<Box
- display="flex"
- alignItems="center"
- borderLeft="1px solid #000"
- marginLeft="20px"
+ display="flex"
+ alignItems="flex-start"
+ gap={2}
+ sx={{
+ borderLeft: (theme) => `1px solid ${theme.palette.divider}`,
+ marginLeft: theme => theme.spacing(2.5),
+ paddingY: 1
+ }}
+ role="listitem"
+ aria-label={`Activity: ${getDisplayText(log.verb)}`}
>
- <Typography ml="30px" variant="body2">
- {getDisplayText(log.verb)}
- </Typography>
+ <Avatar
+ src={user.avatarUrl}
+ alt={user.name}
+ sx={{ width: 32, height: 32 }}
+ />
+ <Box>
+ <Typography variant="subtitle2" component="span">
+ {user.name}
+ </Typography>
+ <Typography variant="body2" color="text.secondary">
+ {getDisplayText(log.verb)}{' '}
+ <Link href={`/comments/${log.commentId}`}>comment</Link>
+ </Typography>
+ {isLastInGroup && (
+ <Typography variant="caption" color="text.secondary">
+ {timestamp}
+ </Typography>
+ )}
+ </Box>
</Box>
)Committable suggestion skipped: line range outside the PR's diff.
junioresc
force-pushed
the
BA-1914-baseapp-activity-log-list-page
branch
3 times, most recently
from
75f33eb to
4276259
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/components/modules/activity-log/LogGroups/index.tsx (3)
9-17: Add JSDoc documentation for the component and its props.Consider adding comprehensive documentation to improve maintainability and developer experience.
+/** + * Renders a virtualized list of activity log groups with infinite scrolling. + * @param {LogGroup[]} logGroups - Array of log groups to display + * @param {ComponentType} LoadingState - Component to render during loading + * @param {object} LoadingStateProps - Props for the loading state component + * @param {object} VirtuosoProps - Props to customize Virtuoso behavior + * @param {(limit: number) => void} loadNext - Function to load more logs + * @param {boolean} hasNext - Whether more logs are available + * @param {boolean} isLoadingNext - Whether next page is being loaded + */ const LogGroups: FC<LogGroupsProps> = ({
74-78: Make the page size configurableThe loadNext function uses a hardcoded limit of 10. Consider making this configurable through props for better flexibility.
endReached={() => { if (hasNext) { - loadNext(10) + loadNext(pageSize) } }}Add pageSize to component props:
interface LogGroupsProps { // ... existing props pageSize?: number; }
66-81: Enhance accessibilityConsider adding appropriate ARIA attributes to improve accessibility:
- <div className="overflow-x-auto hide-scrollbar"> + <div + className="overflow-x-auto hide-scrollbar" + role="feed" + aria-label="Activity log" + > <Virtuoso useWindowScroll data={logGroups} - itemContent={(_index, group) => renderItemContent(group)} + itemContent={(_index, group) => ( + <div role="article" aria-label={`Activity by ${group.logs[0]?.user?.fullName}`}> + {renderItemContent(group)} + </div> + )} components={{ Footer: renderLoadingState, }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 75f33eb9bc98139a3f130aee1f2d1369652a7c78 and 427625928bcd6fc8801e11370139b2ba653f52ab.
⛔ Files ignored due to path filters (3)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (16)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/types.ts(1 hunks)packages/components/modules/activity-log/ActivityUserSearchBar/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityUserSearchBar/types.ts(1 hunks)packages/components/modules/activity-log/DateRangeChip/index.tsx(1 hunks)packages/components/modules/activity-log/DateRangeChip/types.ts(1 hunks)packages/components/modules/activity-log/EventFilterChip/index.tsx(1 hunks)packages/components/modules/activity-log/EventFilterChip/types.ts(1 hunks)packages/components/modules/activity-log/LogGroups/index.tsx(1 hunks)packages/components/modules/activity-log/LogGroups/types.ts(1 hunks)packages/components/modules/activity-log/LogItem/index.tsx(1 hunks)packages/components/modules/activity-log/LogItem/types.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogs.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts(1 hunks)packages/components/modules/activity-log/index.ts(1 hunks)packages/components/schema.graphql(67 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/components/modules/activity-log/ActivityUserSearchBar/types.ts
- packages/components/modules/activity-log/DateRangeChip/types.ts
- packages/components/modules/activity-log/LogItem/types.ts
- packages/components/modules/activity-log/EventFilterChip/types.ts
- packages/components/modules/activity-log/graphql/queries/ActivityLogs.ts
- packages/components/modules/activity-log/ActivityLogComponent/types.ts
- packages/components/modules/activity-log/LogGroups/types.ts
- packages/components/modules/activity-log/ActivityUserSearchBar/index.tsx
- packages/components/modules/activity-log/ActivityLogComponent/index.tsx
- packages/components/modules/activity-log/LogItem/index.tsx
- packages/components/modules/activity-log/DateRangeChip/index.tsx
- packages/components/modules/activity-log/EventFilterChip/index.tsx
- packages/components/modules/activity-log/index.ts
- packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts
- packages/components/schema.graphql
🔇 Additional comments (1)
packages/components/modules/activity-log/LogGroups/index.tsx (1)
18-22: Fix type safety issue in renderLogItem
The use of any type and lack of type checking could lead to runtime errors.
| const renderItemContent = (group: LogGroup) => ( | ||
| <Box | ||
| key={group.lastActivityTimestamp} | ||
| display="flex" | ||
| flexDirection="column" | ||
| justifyContent="center" | ||
| gap="12px" | ||
| pt="6px" | ||
| pb="16px" | ||
| maxWidth="568px" | ||
| > | ||
| <Box display="flex" alignItems="center" gap="12px"> | ||
| <Avatar | ||
| sizes="small" | ||
| src={group.logs[0]?.user?.avatar?.url ?? ''} | ||
| alt={group.logs[0]?.user?.fullName ?? ''} | ||
| /> | ||
| <Typography variant="subtitle2" mb="6px"> | ||
| {group.logs[0]?.user?.fullName} | ||
| </Typography> | ||
| </Box> | ||
| {group.logs.map((log: Log) => renderLogItem(log))} | ||
| <Typography variant="caption"> | ||
| {new Date(group.lastActivityTimestamp).toLocaleString()} | ||
| </Typography> | ||
| </Box> | ||
| ) |
There was a problem hiding this comment.
Implement missing comment hyperlink requirement
According to the PR objectives, the term "Comment" in activity text should be hyperlinked to the corresponding comment detail page. This requirement is not implemented in the current code.
Would you like me to provide an implementation example for the comment hyperlink requirement?
packages/components/modules/activity-log/ActivityLogComponent/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/activity-log/ActivityLogComponent/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/activity-log/ActivityLogComponent/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/activity-log/EventFilterChip/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/activity-log/DateRangeChip/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/activity-log/ActivityLogComponent/index.tsx
Outdated
Show resolved
Hide resolved
junioresc
force-pushed
the
BA-1914-baseapp-activity-log-list-page
branch
2 times, most recently
from
d358ab8 to
de293c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
packages/components/modules/activity-log/ActivityLogComponent/LogItem.tsx (4)
7-12: Consider adding extended mappings or i18n support for verbMapping.
This object-based approach is great for providing human-readable text. If you anticipate supporting more actions or translations in the future, consider either extendingverbMappingor integrating it with an internationalization (i18n) library.
14-15: Optional performance enhancement by memoizing getDisplayText.
IfgetDisplayTextis called in a large list or loop, you may consider memoizing it to avoid repeated computations. However, this is optional if performance is not a concern.
16-20: Provide a more explicit fallback for missing verb data.
Returningnullis fine for missing or invalidlog.verb, but consider logging or providing a fallback text in case the data is incomplete. This might help you diagnose unexpected logs.
21-27: Ensure consistent styling or theming for log entries.
The left border andml="30px"may be fine for now, but consider extracting style constants or using a Material-UI theme for consistent spacing and color usage throughout the app.packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip.tsx (3)
8-16: Consider potential states for an empty or disabled filter list.
You handle the menu anchor well, but ifoptionsis empty or in a loading state, you might want to disable theChipor display a different label.
18-29: Use Set for more concise addition and removal logic.
When toggling the selected options, using aSetstructure might simplify or clarify the logic. Here’s an example approach:const handleToggle = (option: string) => { - const currentIndex = selectedOptions.indexOf(option) - const newSelectedOptions = [...selectedOptions] - if (currentIndex === -1) { - newSelectedOptions.push(option) - } else { - newSelectedOptions.splice(currentIndex, 1) - } + const newSelectionSet = new Set(selectedOptions) + if (newSelectionSet.has(option)) { + newSelectionSet.delete(option) + } else { + newSelectionSet.add(option) + } + const newSelectedOptions = Array.from(newSelectionSet) onChange(newSelectedOptions) }
31-43: Potential enhancement for user feedback on selection changes.
Currently, the label for theChipis static ("Filter"). Consider dynamically reflecting the number of selected filters or changing the label to provide instant user feedback.packages/components/modules/activity-log/ActivityLogComponent/LogGroups.tsx (2)
25-36: Ensure loading states for first fetch vs. loadNext.
Right now,renderLoadingStatedisplays an empty box unlessisLoadingNextis true. If there’s a chance the initial load might also display a spinner, ensure that scenario is properly handled (for example, with a different loading indicator outsiderenderLoadingState).
66-84: Check for “no logs” scenario handling.
Consider displaying a helpful message iflogGroupsis empty (e.g., “No activities found.”). This enhances usability and avoids rendering an empty list.packages/components/modules/activity-log/ActivityLogComponent/types.ts (2)
17-23: Rename or unify option types for clarity.
You’re currently usingEventFilterOptionas(typeof EVENT_FILTER_OPTIONS)[number]. If you foresee different or dynamic event filter options, consider defining a more explicit type or interface for future maintainability.
25-33: Add JSDoc or in-code docs for these props.
LogGroupsPropsincludes complex pagination-related fields. Documenting each prop in-code (via JSDoc or TypeScript comments) will make it clearer for future contributors.packages/components/modules/activity-log/ActivityLogComponent/index.tsx (1)
43-49: Extract filter options as constantsThe filter options are currently hardcoded in the component. For better maintainability and reusability, these should be extracted as constants.
+const ACTIVITY_FILTER_OPTIONS = ['All', 'Comments', 'Reactions', 'Posts'] as const +type ActivityFilterOption = typeof ACTIVITY_FILTER_OPTIONS[number] <EventFilterChip - options={['All', 'Comments', 'Reactions', 'Posts']} + options={ACTIVITY_FILTER_OPTIONS} selectedOptions={selectedFilters} onChange={setSelectedFilters} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d358ab8f76b6597153e1641e609a86dcc94f3165 and de293c7978cdc72ccf5db1653dd9440f55d8dec7.
⛔ Files ignored due to path filters (3)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (13)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogGroups.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogItem.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/constants.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/types.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogs.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts(1 hunks)packages/components/modules/activity-log/index.ts(1 hunks)packages/components/modules/activity-log/types.ts(1 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(0 hunks)
💤 Files with no reviewable changes (1)
- packages/components/schema.graphql
✅ Files skipped from review due to trivial changes (3)
- packages/components/modules/activity-log/ActivityLogComponent/constants.ts
- packages/components/package.json
- packages/components/modules/activity-log/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/components/modules/activity-log/graphql/queries/ActivityLogs.ts
- packages/components/modules/activity-log/index.ts
- packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts
🔇 Additional comments (8)
packages/components/modules/activity-log/ActivityLogComponent/LogItem.tsx (1)
30-30: Export looks good.
The default export is clearly named, making the file usage straightforward.
packages/components/modules/activity-log/ActivityLogComponent/LogGroups.tsx (2)
19-23: Leverage unique keys for list items.
You are already using log.id in the LogItem, which is good. Double-check that log.id is always guaranteed to be unique. If there’s a chance id might be duplicated, consider systematically generating keys or using a stable unique ID.
38-64: Verify user avatar URLs and fallback images.
You are using the first log’s user’s avatar to display. If some logs lack a user or avatar, ensure you have a fallback image or handle the scenario gracefully.
packages/components/modules/activity-log/ActivityLogComponent/types.ts (1)
39-41: Confirm log type coverage.
LogItemProps references ActivityLogNode. Ensure ActivityLogNode accounts for any potential missing fields from the backend, especially if logs can differ in structure based on the event type.
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (3)
1-20: LGTM! Clean imports and props structure
The component's setup follows best practices with:
- Proper organization of imports
- Client-side directive for Next.js
- Customizable LogGroups component with default implementation
51-57: LGTM! Proper implementation of infinite scrolling
The LogGroups component correctly implements infinite scrolling pagination as required, with:
- Proper loading state handling
- Next page detection
- Customization through prop spreading
30-59: LGTM! Clean and responsive layout
The layout implementation:
- Centers content appropriately
- Uses proper spacing and padding
- Maintains responsive design principles
packages/components/CHANGELOG.md (1)
3-8: LGTM! Proper changelog entry
The changelog entry correctly documents the addition of activity log components with appropriate version bump.
| const [selectedFilters, setSelectedFilters] = useState<string[]>(['All']) | ||
| const [isPending] = useTransition() | ||
| const { control, watch } = useForm({ defaultValues: { search: '' } }) | ||
| const searchValue = watch('search') | ||
| const { logGroups, loadNext, hasNext, isLoadingNext } = useActivityLogs(queryRef) | ||
| const handleSearchChange: ChangeEventHandler<HTMLInputElement> = () => {} | ||
| const handleSearchClear = () => {} | ||
| const emptyLogsList = logGroups.length === 0 |
There was a problem hiding this comment.
Implement search functionality
The search handlers handleSearchChange and handleSearchClear are currently empty. These need to be implemented to fulfill the search functionality mentioned in the PR objectives.
Would you like me to help implement these handlers? Here's a suggested implementation:
-const handleSearchChange: ChangeEventHandler<HTMLInputElement> = () => {}
-const handleSearchClear = () => {}
+const handleSearchChange: ChangeEventHandler<HTMLInputElement> = (event) => {
+ // Implement debounced search here
+ console.log('Search value:', event.target.value)
+}
+const handleSearchClear = () => {
+ // Reset form and filters
+ control._reset({ search: '' })
+}Committable suggestion skipped: line range outside the PR's diff.
@anicioalexandre I tried doing pnpm relay-update-schema but I get an error from baseapp chats saying isGroup is not recognized. I checked out to BA-1943 on the Baseapp-Backend package and ran the migration but it is still not being recognized by relay or graphql |
@junioresc weird is your backend up to date with master? it should be there |
junioresc
force-pushed
the
BA-1914-baseapp-activity-log-list-page
branch
from
de293c7 to
f88737e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/types.ts (1)
5-9: Add JSDoc documentation for the interface.Consider adding JSDoc comments to document the purpose of each prop and provide examples. This will improve maintainability and developer experience.
+/** + * Props for the EventFilterChip component + * @property {EventFilterOption[]} options - Available filter options to display + * @property {string[]} selectedOptions - Currently selected filter options + * @property {(selected: string[]) => void} onChange - Callback when selection changes + */ export interface EventFilterChipProps { options: EventFilterOption[] selectedOptions: string[] onChange: (selected: string[]) => void }packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/index.tsx (3)
7-9: Consider memoizing the component.Since this component might be rendered in a list of filters, consider wrapping it with
React.memoto prevent unnecessary re-renders.
18-29: Simplify the handleToggle implementation.The current implementation using array operations can be simplified using Set or filter.
- const handleToggle = (option: string) => { - const currentIndex = selectedOptions.indexOf(option) - const newSelectedOptions = [...selectedOptions] - - if (currentIndex === -1) { - newSelectedOptions.push(option) - } else { - newSelectedOptions.splice(currentIndex, 1) - } - - onChange(newSelectedOptions) - } + const handleToggle = (option: string) => { + const newSelectedOptions = selectedOptions.includes(option) + ? selectedOptions.filter(item => item !== option) + : [...selectedOptions, option] + onChange(newSelectedOptions) + }
10-16: Memoize event handlers with useCallback.Memoize the event handlers to prevent unnecessary re-renders of child components.
+import { FC, useState, useCallback } from 'react' - const handleClick = (event: React.MouseEvent<HTMLElement>) => { + const handleClick = useCallback((event: React.MouseEvent<HTMLElement>) => { setAnchorEl(event.currentTarget) - } + }, []) - const handleClose = () => { + const handleClose = useCallback(() => { setAnchorEl(null) - } + }, [])packages/components/modules/activity-log/ActivityLogComponent/LogGroups/types.ts (1)
17-18: Improve type safety by avoiding 'any' type.Consider defining proper types instead of using
any:
- For
LoadingStateProps, create an interface with the expected props- For
VirtuosoProps, use the correct generic types instead ofany- LoadingStateProps?: any + LoadingStateProps?: { + size?: number + className?: string + style?: React.CSSProperties + } - VirtuosoProps?: Partial<VirtuosoProps<any, any>> + VirtuosoProps?: Partial<VirtuosoProps<LogGroup, unknown>>packages/components/modules/activity-log/ActivityLogComponent/LogItem/index.tsx (1)
26-26: Use MUI theme for consistent styling.Instead of hardcoding colors, use MUI theme for better maintainability and consistency.
- <Typography color="#919EAB33" ml="30px" variant="body2"> + <Typography color="text.disabled" ml="30px" variant="body2">packages/components/modules/activity-log/ActivityLogComponent/LogGroups/index.tsx (3)
50-54: Improve avatar handling with fallback.The current avatar implementation doesn't handle fallbacks gracefully. Consider using MUI's built-in fallback functionality.
<Avatar sizes="small" - src={group.logs[0]?.user?.avatar?.url ?? ''} - alt={group.logs[0]?.user?.fullName ?? ''} + src={group.logs[0]?.user?.avatar?.url} + alt={group.logs[0]?.user?.fullName} > + {group.logs[0]?.user?.fullName?.[0]?.toUpperCase()} </Avatar>
60-62: Enhance date formatting for better readability.The current date formatting using
toLocaleString()might not provide the best user experience. Consider using a date formatting library or custom formatting based on the timestamp.<Typography variant="caption"> - {new Date(group.lastActivityTimestamp).toLocaleString()} + {formatRelativeTime(new Date(group.lastActivityTimestamp))} </Typography>Consider adding a utility function like:
function formatRelativeTime(date: Date): string { const now = new Date(); const diff = now.getTime() - date.getTime(); if (diff < 60000) return 'just now'; if (diff < 3600000) return `${Math.floor(diff / 60000)}m ago`; if (diff < 86400000) return `${Math.floor(diff / 3600000)}h ago`; return date.toLocaleDateString(); }
67-82: Consider adding error boundary.The virtualized list should handle potential rendering errors gracefully.
<div className="overflow-x-auto hide-scrollbar"> + <ErrorBoundary fallback={<ErrorMessage />}> <Virtuoso useWindowScroll data={logGroups} itemContent={(_index, group) => renderItemContent(group)} components={{ Footer: renderLoadingState, }} endReached={() => { if (hasNext) { loadNext(10) } }} {...VirtuosoProps} /> + </ErrorBoundary> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between de293c7978cdc72ccf5db1653dd9440f55d8dec7 and f88737e2a12abb91f28610bef6e0377be564591f.
⛔ Files ignored due to path filters (3)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (16)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/constants.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/types.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogGroups/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogGroups/types.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogItem/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogItem/types.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/types.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogs.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts(1 hunks)packages/components/modules/activity-log/index.ts(1 hunks)packages/components/modules/activity-log/types.ts(1 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(0 hunks)
💤 Files with no reviewable changes (1)
- packages/components/schema.graphql
✅ Files skipped from review due to trivial changes (1)
- packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/constants.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/components/CHANGELOG.md
- packages/components/package.json
- packages/components/modules/activity-log/types.ts
- packages/components/modules/activity-log/ActivityLogComponent/types.ts
- packages/components/modules/activity-log/graphql/queries/ActivityLogs.ts
- packages/components/modules/activity-log/ActivityLogComponent/index.tsx
- packages/components/modules/activity-log/index.ts
- packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Applications and Packages
- GitHub Check: Lint
🔇 Additional comments (3)
packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/types.ts (1)
3-3: LGTM! Good use of type extraction.Using
typeofwith indexed access type is a clean way to keep types in sync with constants.packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/index.tsx (1)
1-46: Verify integration with activity log filtering requirements.The component implements basic filtering functionality, but let's verify it meets the PR requirements for activity log filtering.
packages/components/modules/activity-log/ActivityLogComponent/LogItem/types.ts (1)
1-5: LGTM! Clean type definition.The
LogItemPropsinterface is well-defined and properly imports theActivityLogNodetype.
| return ( | ||
| <> | ||
| <Chip label="Filter" onClick={handleClick} /> | ||
| <Menu anchorEl={anchorEl} open={Boolean(anchorEl)} onClose={handleClose}> | ||
| {options.map((option) => ( | ||
| <MenuItem key={option} onClick={() => handleToggle(option)}> | ||
| <Checkbox checked={selectedOptions.indexOf(option) > -1} /> | ||
| <ListItemText primary={option} /> | ||
| </MenuItem> | ||
| ))} | ||
| </Menu> | ||
| </> | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and user experience.
The component needs accessibility improvements and keyboard navigation support.
return (
<>
- <Chip label="Filter" onClick={handleClick} />
+ <Chip
+ label="Filter"
+ onClick={handleClick}
+ onKeyDown={(e) => e.key === 'Enter' && handleClick(e)}
+ role="button"
+ tabIndex={0}
+ aria-haspopup="true"
+ aria-expanded={Boolean(anchorEl)}
+ />
<Menu
anchorEl={anchorEl}
open={Boolean(anchorEl)}
onClose={handleClose}
+ aria-label="Filter options"
>
{options.map((option) => (
<MenuItem key={option} onClick={() => handleToggle(option)}>
- <Checkbox checked={selectedOptions.indexOf(option) > -1} />
+ <Checkbox
+ checked={selectedOptions.includes(option)}
+ inputProps={{ 'aria-label': `Select ${option}` }}
+ />
<ListItemText primary={option} />
</MenuItem>
))}
</Menu>
</>
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <> | |
| <Chip label="Filter" onClick={handleClick} /> | |
| <Menu anchorEl={anchorEl} open={Boolean(anchorEl)} onClose={handleClose}> | |
| {options.map((option) => ( | |
| <MenuItem key={option} onClick={() => handleToggle(option)}> | |
| <Checkbox checked={selectedOptions.indexOf(option) > -1} /> | |
| <ListItemText primary={option} /> | |
| </MenuItem> | |
| ))} | |
| </Menu> | |
| </> | |
| ) | |
| return ( | |
| <> | |
| <Chip | |
| label="Filter" | |
| onClick={handleClick} | |
| onKeyDown={(e) => e.key === 'Enter' && handleClick(e)} | |
| role="button" | |
| tabIndex={0} | |
| aria-haspopup="true" | |
| aria-expanded={Boolean(anchorEl)} | |
| /> | |
| <Menu | |
| anchorEl={anchorEl} | |
| open={Boolean(anchorEl)} | |
| onClose={handleClose} | |
| aria-label="Filter options" | |
| > | |
| {options.map((option) => ( | |
| <MenuItem key={option} onClick={() => handleToggle(option)}> | |
| <Checkbox | |
| checked={selectedOptions.includes(option)} | |
| inputProps={{ 'aria-label': `Select ${option}` }} | |
| /> | |
| <ListItemText primary={option} /> | |
| </MenuItem> | |
| ))} | |
| </Menu> | |
| </> | |
| ) |
| return ( | ||
| <Box display="flex" alignItems="center" borderLeft="1px solid #919EAB33" marginLeft="20px"> | ||
| <Typography color="#919EAB33" ml="30px" variant="body2"> | ||
| {getDisplayText(log.verb)} | ||
| </Typography> | ||
| </Box> | ||
| ) |
There was a problem hiding this comment.
Add hyperlink to Comment text per requirements.
The PR objectives specify that "Comment" in activity text should be hyperlinked to the corresponding comment detail page.
<Box display="flex" alignItems="center" borderLeft="1px solid #919EAB33" marginLeft="20px">
<Typography color="#919EAB33" ml="30px" variant="body2">
- {getDisplayText(log.verb)}
+ {log.verb.includes('comment') ? (
+ <>
+ {getDisplayText(log.verb).replace('comment', '')}
+ <Link href={`/comments/${log.targetId}`}>comment</Link>
+ </>
+ ) : (
+ getDisplayText(log.verb)
+ )}
</Typography>
</Box>Committable suggestion skipped: line range outside the PR's diff.
| if (hasNext) { | ||
| loadNext(10) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Consider reducing the batch size to 5 to align with other components
The codebase shows a consistent pattern of using a batch size of 5 for pagination across different components (notifications, comments, messages, and members). The current value of 10 in ActivityLog is an outlier. Other components have noted potential stack size issues with larger batch sizes.
🔗 Analysis chain
Verify pagination batch size.
The hardcoded value of 10 for loadNext might need adjustment based on performance testing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other pagination batch sizes in the codebase
rg -A 2 "loadNext\(" | grep -v "loadNext(10)"
Length of output: 2567
Script:
#!/bin/bash
# Search for constant definitions related to pagination batch sizes
rg "NUMBER_OF_.*_TO_LOAD_NEXT|MESSAGES_TO_LOAD_NEXT" -B 1 -A 1
Length of output: 5550
junioresc
force-pushed
the
BA-1914-baseapp-activity-log-list-page
branch
from
f88737e to
e8f4134
Compare
junioresc
force-pushed
the
BA-1914-baseapp-activity-log-list-page
branch
3 times, most recently
from
7705efc to
418ceae
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (1)
26-27:⚠️ Potential issueImplement search handlers
The search handlers
handleSearchChangeandhandleSearchClearare currently empty. These need to be implemented to enable the search functionality required by the PR objectives.Consider implementing them like this:
-const handleSearchChange: ChangeEventHandler<HTMLInputElement> = () => {} -const handleSearchClear = () => {} +const handleSearchChange: ChangeEventHandler<HTMLInputElement> = () => { + startTransition(() => { + // Update search query in your data fetching logic + }) +} +const handleSearchClear = () => { + control._reset({ search: '' }) +}
🧹 Nitpick comments (2)
packages/components/modules/activity-log/ActivityLogComponent/LogGroups/index.tsx (2)
50-54: Add fallback for missing avatarConsider adding a fallback avatar when the user's avatar URL is not available. This would improve the UI consistency.
<Avatar sizes="small" - src={group.logs[0]?.user?.avatar?.url ?? ''} + src={group.logs[0]?.user?.avatar?.url ?? '/default-avatar.png'} alt={group.logs[0]?.user?.fullName ?? ''} />
60-62: Improve timestamp formattingThe current timestamp implementation could be enhanced by:
- Considering timezone context
- Using relative time for better UX (e.g., "2 hours ago")
- Respecting user's locale settings
<Typography variant="caption"> - {new Date(group.lastActivityTimestamp).toLocaleString()} + {new Intl.RelativeTimeFormat(undefined, { numeric: 'auto' }).format( + -Math.round((Date.now() - new Date(group.lastActivityTimestamp).getTime()) / (1000 * 60 * 60)), + 'hours' + )} </Typography>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 7705efc14342b192bb4e579e00c35236b33f0e3b and 418ceae8158d9ac8c83c6c5da0944e30b6084e7e.
⛔ Files ignored due to path filters (3)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (16)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/constants.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/types.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogGroups/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogGroups/types.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogItem/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogItem/types.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/types.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogs.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts(1 hunks)packages/components/modules/activity-log/index.ts(1 hunks)packages/components/modules/activity-log/types.ts(1 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(0 hunks)
💤 Files with no reviewable changes (1)
- packages/components/schema.graphql
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/components/package.json
- packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/constants.ts
- packages/components/modules/activity-log/graphql/queries/ActivityLogs.ts
- packages/components/modules/activity-log/ActivityLogComponent/LogItem/types.ts
- packages/components/CHANGELOG.md
- packages/components/modules/activity-log/types.ts
- packages/components/modules/activity-log/ActivityLogComponent/LogItem/index.tsx
- packages/components/modules/activity-log/ActivityLogComponent/types.ts
- packages/components/modules/activity-log/ActivityLogComponent/LogGroups/types.ts
- packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/types.ts
- packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/index.tsx
- packages/components/modules/activity-log/index.ts
- packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: Component Test Applications and Packages
🔇 Additional comments (2)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (1)
44-46: Extract filter options to constantsThe filter options should be extracted to a constant for better maintainability and type safety. This was mentioned in a previous review.
+const ACTIVITY_FILTER_OPTIONS = ['All', 'Comments', 'Reactions', 'Posts'] as const +type ActivityFilterOption = typeof ACTIVITY_FILTER_OPTIONS[number] <EventFilterChip - options={['All', 'Comments', 'Reactions', 'Posts']} + options={ACTIVITY_FILTER_OPTIONS} selectedOptions={selectedFilters} onChange={setSelectedFilters} />packages/components/modules/activity-log/ActivityLogComponent/LogGroups/index.tsx (1)
76-78: Align batch size with codebase standardsThe current batch size of 10 differs from the standard batch size of 5 used across other components in the codebase (notifications, comments, messages, etc.).
if (hasNext) { - loadNext(10) + loadNext(5) }
packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/activity-log/ActivityLogComponent/LogItem/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/activity-log/ActivityLogComponent/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/activity-log/ActivityLogComponent/index.tsx
Outdated
Show resolved
Hide resolved
junioresc
force-pushed
the
BA-1914-baseapp-activity-log-list-page
branch
from
418ceae to
6ccacd9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/components/modules/activity-log/ActivityLogComponent/LogGroups/index.tsx (3)
19-23: Remove unnecessary key prop in renderLogItem.The
keyprop inrenderLogItemis redundant since this component is used within amapfunction that should handle the key prop.- return <LogItem key={log.id} log={log} /> + return <LogItem log={log} />
25-36: Consider using theme spacing for consistency.Replace hardcoded padding values with theme spacing to maintain consistency across the application.
- sx={{ paddingTop: 3, paddingBottom: 1 }} + sx={{ pt: 3, pb: 1 }}
38-64: Several improvements for renderItemContent function.
- Consider extracting user information to reduce optional chaining.
- Use theme spacing for consistency.
- Consider using a date formatting library for i18n support.
const renderItemContent = (group: LogGroup) => { + const user = group.logs[0]?.user + const formattedDate = new Intl.DateTimeFormat(undefined, { + dateStyle: 'medium', + timeStyle: 'short' + }).format(new Date(group.lastActivityTimestamp)) return ( <Box key={group.lastActivityTimestamp} display="flex" flexDirection="column" justifyContent="center" gap={1.5} - pt="6px" - pb="16px" + pt={0.75} + pb={2} maxWidth="568px" > <Box display="flex" alignItems="center" gap={1.5}> <Avatar sizes="small" - src={group.logs[0]?.user?.avatar?.url ?? ''} - alt={group.logs[0]?.user?.fullName ?? ''} + src={user?.avatar?.url ?? ''} + alt={user?.fullName ?? ''} /> <Typography variant="subtitle2" mb={0.75}> - {group.logs[0]?.user?.fullName} + {user?.fullName} </Typography> </Box> {group.logs.map((log: ActivityLogNode) => renderLogItem(log))} <Typography variant="caption"> - {new Date(group.lastActivityTimestamp).toLocaleString()} + {formattedDate} </Typography> </Box> ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 418ceae8158d9ac8c83c6c5da0944e30b6084e7e and 6ccacd939ce1b1ae9908339e1c695e7b807fabb4.
⛔ Files ignored due to path filters (3)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (16)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/constants.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/types.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogGroups/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogGroups/types.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogItem/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogItem/types.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/types.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogs.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts(1 hunks)packages/components/modules/activity-log/index.ts(1 hunks)packages/components/modules/activity-log/types.ts(1 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(0 hunks)
💤 Files with no reviewable changes (1)
- packages/components/schema.graphql
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/components/package.json
- packages/components/CHANGELOG.md
- packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/constants.ts
- packages/components/modules/activity-log/ActivityLogComponent/LogItem/types.ts
- packages/components/modules/activity-log/ActivityLogComponent/types.ts
- packages/components/modules/activity-log/types.ts
- packages/components/modules/activity-log/graphql/queries/ActivityLogs.ts
- packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/types.ts
- packages/components/modules/activity-log/ActivityLogComponent/index.tsx
- packages/components/modules/activity-log/ActivityLogComponent/LogGroups/types.ts
- packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/index.tsx
- packages/components/modules/activity-log/ActivityLogComponent/LogItem/index.tsx
- packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts
- packages/components/modules/activity-log/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Applications and Packages
- GitHub Check: Lint
🔇 Additional comments (4)
packages/components/modules/activity-log/ActivityLogComponent/LogGroups/index.tsx (4)
1-9: LGTM! Clean and well-organized imports.The imports are properly structured and include all necessary dependencies.
10-18: LGTM! Well-structured component props.Props are well-defined with appropriate defaults and TypeScript types.
76-78: Consider reducing the batch size to 5 to align with other components.The codebase shows a consistent pattern of using a batch size of 5 for pagination across different components.
- loadNext(10) + loadNext(5)
67-67: Verify the hide-scrollbar class implementation.Ensure that the
hide-scrollbarclass is properly defined in your CSS/styling system.✅ Verification successful
The
hide-scrollbarclass is properly implementedThe class is correctly defined in the design system's Tailwind plugins with cross-browser support for hiding scrollbars.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for hide-scrollbar class definition rg "hide-scrollbar" -t css -t scss -t lessLength of output: 71
Script:
#!/bin/bash # Search for hide-scrollbar class in all files rg "hide-scrollbar" # Find tailwind config files fd "tailwind.config" --type f # Search for global style files fd -e css -e scss -e lessLength of output: 675
Script:
#!/bin/bash # Check the implementation of hide-scrollbar in the Tailwind plugin cat packages/design-system/styles/tailwind/plugins/index.tsLength of output: 1126
junioresc
force-pushed
the
BA-1914-baseapp-activity-log-list-page
branch
from
6ccacd9 to
35671ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/components/modules/activity-log/ActivityLogComponent/constants.ts (1)
1-1: LGTM! Consider documenting future filter options.The filter options align well with the PR objectives. Since the activity log is designed to be expandable for future activities, consider adding a comment documenting potential future filter options to help maintain the list.
+// Add new filter options here as more activity types are supported export const EVENT_FILTER_OPTIONS = ['All', 'Comments', 'Reactions', 'Posts'] as constpackages/components/modules/activity-log/ActivityLogComponent/types.ts (1)
7-11: LGTM! Consider adding JSDoc comments.The interface is well-structured. Consider adding JSDoc comments to document the purpose of each prop, especially for optional props like
LogGroupsandLogGroupsProps.+/** + * Props for the ActivityLogComponent + * @property queryRef - GraphQL query data for activity logs + * @property LogGroups - Optional custom component for rendering log groups + * @property LogGroupsProps - Optional props to customize LogGroups behavior + */ export interface ActivityLogComponentProps { queryRef: ActivityLogsPaginationQuery$data LogGroups?: FC<LogGroupsProps> LogGroupsProps?: Partial<LogGroupsProps> }packages/components/modules/activity-log/ActivityLogComponent/LogGroups/index.tsx (2)
50-55: Add error handling for avatar loading.Consider adding an
onErrorhandler for the Avatar component to handle cases where the avatar URL is invalid or fails to load.<Avatar style={{ marginBottom: '4px' }} sizes="small" src={group.logs[0]?.user?.avatar?.url ?? ''} alt={group.logs[0]?.user?.fullName ?? 'User activity log avatar'} + onError={(e) => { + // Prevent infinite error loop + e.currentTarget.onerror = null; + e.currentTarget.src = '/default-avatar.png'; // Add default avatar path + }} />
68-83: Consider adding error boundary for virtualized list.The Virtuoso component might fail if the data structure is unexpected. Consider wrapping it in an error boundary to gracefully handle rendering failures.
+import { ErrorBoundary } from 'react-error-boundary' + return ( <div className="overflow-x-auto hide-scrollbar"> + <ErrorBoundary fallback={<div>Error loading activity logs</div>}> <Virtuoso useWindowScroll data={logGroups} itemContent={(_index, group) => renderItemContent(group)} components={{ Footer: renderLoadingState, }} endReached={() => { if (hasNext) { loadNext(10) } }} {...VirtuosoProps} /> + </ErrorBoundary> </div> )packages/components/modules/messages/CreateGroup/ConnectionsList/index.tsx (2)
Line range hint
46-48: Consider increasing the batch size for better UX.Loading only 5 items at a time might cause too frequent API calls during scrolling. Consider increasing this to a more optimal batch size (e.g., 20-25 items) to reduce server load while maintaining smooth scrolling.
- loadNext(5) + loadNext(25)
Line range hint
41-43: Make the container height configurable.The hardcoded maxHeight of 250px might not be flexible enough for different use cases. Consider making it configurable through props.
+ containerHeight?: number | string; ... - style={{ scrollbarWidth: 'none', maxHeight: '250px' }} + style={{ scrollbarWidth: 'none', maxHeight: containerHeight ?? '250px' }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 6ccacd939ce1b1ae9908339e1c695e7b807fabb4 and 35671ff84488505b706546edfb1a7846d808e4ae.
⛔ Files ignored due to path filters (3)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ActivityLogsQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (19)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/types.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogGroups/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogGroups/types.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogItem/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/LogItem/types.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/constants.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/index.tsx(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/types.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogs.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts(1 hunks)packages/components/modules/activity-log/index.ts(1 hunks)packages/components/modules/activity-log/types.ts(1 hunks)packages/components/modules/messages/ChatRoomsList/index.tsx(1 hunks)packages/components/modules/messages/CreateChatRoomList/index.tsx(1 hunks)packages/components/modules/messages/CreateGroup/ConnectionsList/index.tsx(1 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(0 hunks)
💤 Files with no reviewable changes (1)
- packages/components/schema.graphql
✅ Files skipped from review due to trivial changes (2)
- packages/components/modules/messages/ChatRoomsList/index.tsx
- packages/components/modules/messages/CreateChatRoomList/index.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/components/package.json
- packages/components/CHANGELOG.md
- packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/types.ts
- packages/components/modules/activity-log/graphql/queries/ActivityLogs.ts
- packages/components/modules/activity-log/types.ts
- packages/components/modules/activity-log/ActivityLogComponent/LogItem/index.tsx
- packages/components/modules/activity-log/ActivityLogComponent/index.tsx
- packages/components/modules/activity-log/ActivityLogComponent/EventFilterChip/index.tsx
- packages/components/modules/activity-log/ActivityLogComponent/LogGroups/types.ts
- packages/components/modules/activity-log/index.ts
- packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Lint
🔇 Additional comments (6)
packages/components/modules/activity-log/ActivityLogComponent/LogGroups/index.tsx (2)
77-79: Align batch size with codebase standards.The current batch size of 10 differs from the codebase standard of 5 used in other components (notifications, comments, messages, and members).
- loadNext(10) + loadNext(5)
39-65: Group timestamp formatting aligns with requirements.The implementation correctly displays timestamps for activity groups and follows the PR requirement of grouping activities within 15-minute intervals.
packages/components/modules/activity-log/ActivityLogComponent/LogItem/types.ts (2)
1-8: Consider virtualization requirementsSince the PR mentions infinite scrolling pagination, verify if this component will be used within a virtualized list component. If so, consider adding height-related props or documentation about height requirements for proper virtualization.
1-3: Verify the ActivityLogNode type structureLet's ensure the ActivityLogNode type includes all necessary fields for the activity log requirements mentioned in the PR objectives (user avatar, name, timestamp, etc.).
✅ Verification successful
Import and type structure are correctly defined
The ActivityLogNode type is properly derived from the GraphQL schema through the ActivityLogsFragment, ensuring all necessary fields for the activity log requirements are available. The relative import path is correct and follows the established type chain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ActivityLogNode type definition ast-grep --pattern 'type ActivityLogNode = { $$$ }' # Alternatively, look for interface definition ast-grep --pattern 'interface ActivityLogNode { $$$ }'Length of output: 114
Script:
#!/bin/bash # Find the types.ts file in the activity-log module fd types.ts packages/components/modules/activity-log # Search for ActivityLogNode in any format rg "ActivityLogNode" --type ts -A 5 # Try more flexible ast-grep patterns ast-grep --pattern 'export $_ ActivityLogNode $_'Length of output: 4336
Script:
#!/bin/bash # Search for ActivityLogEdges and related type definitions rg "ActivityLogEdges" --type ts -A 10 # Look for any type definitions in the main types.ts file cat packages/components/modules/activity-log/types.tsLength of output: 627
packages/components/modules/messages/CreateGroup/ConnectionsList/index.tsx (2)
10-10: LGTM! Good architectural improvement.Moving SearchNotFoundState to a shared directory improves component reusability and maintainability.
Let's verify if other components are consistently using the shared SearchNotFoundState:
✅ Verification successful
Import path is consistent across the codebase
All components importing SearchNotFoundState are consistently using the shared version from the
__shared__directory:
- ActivityLogComponent
- CreateGroup/ConnectionsList
- CreateChatRoomList
- ChatRoomsList
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining direct imports of SearchNotFoundState # that haven't been updated to use the shared component rg "import.*SearchNotFoundState.*(?<!__shared__/SearchNotFoundState)" --type tsLength of output: 379
Script:
#!/bin/bash # Find all SearchNotFoundState imports and component definitions echo "=== SearchNotFoundState imports ===" rg "import.*SearchNotFoundState" --type ts echo -e "\n=== SearchNotFoundState component definitions ===" ast-grep --pattern 'export default function SearchNotFoundState'Length of output: 875
Script:
#!/bin/bash # Search for SearchNotFoundState component definition with different patterns echo "=== Arrow function or class component definition ===" ast-grep --pattern 'export $_ SearchNotFoundState' echo -e "\n=== Component file by name ===" fd SearchNotFoundState.tsxLength of output: 273
Line range hint
13-23: Missing required functionality from PR objectives.Based on the PR objectives, the following requirements appear to be missing:
- User activity grouping within 15-minute intervals
- User avatar and name display
- Timestamp display for the last activity
Consider extending the component props and implementation to support these requirements.
Let's check if these features are implemented elsewhere:
| export interface LogItemProps { | ||
| log: ActivityLogNode | ||
| sx?: SxProps<Theme> | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding props for grouping and link handling
Based on the PR requirements:
- Activities should be grouped within 15-minute intervals
- "Comment" text should be hyperlinked to the detail page
Consider extending the interface with:
isGrouped?: booleancommentUrl?: string
export interface LogItemProps {
log: ActivityLogNode
sx?: SxProps<Theme>
+ isGrouped?: boolean
+ commentUrl?: string
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface LogItemProps { | |
| log: ActivityLogNode | |
| sx?: SxProps<Theme> | |
| } | |
| export interface LogItemProps { | |
| log: ActivityLogNode | |
| sx?: SxProps<Theme> | |
| isGrouped?: boolean | |
| commentUrl?: string | |
| } |
junioresc
force-pushed
the
BA-1914-baseapp-activity-log-list-page
branch
from
35671ff to
58e4ced
Compare
|
As a user, on the Baseapp Activity Log,I would like to See a list of activiy logs on my platform, In order to monitor changes.
Acceptance Criteria
Context
The objective of this story is to create the main structure of the activity logs page based on the records of PG History. We will use only some modals as examples of the logs to show.
Business Rules - List Page
Given I navigate to the Activity logs page, it should display a list of activities saved in PG history for the following activities:
Created comment
Reacted to a comment
Pinned comment
Edited a comment
List should allow to include more activities in the future
The Activity log list should be sorted from most recent to oldest activity
The Activity log will have a infinite scroller pagination.
Include a simple loading state as part of this story that can be enhanced in the future
Business Rules - Activity Log Entries
Activity log entries should be grouped by user if the activities were done within intervals of 15 minutes.
Ex: Act1 (9:00am) - Act2 (9:10am)- Act3(9:16am). Only Act 1 and 2 will be in the same group.
The activity log entry will display the avatar with a placeholder image and the name of the user next to it per Figma
Each activity log entry will also include a timestamp for the last activity done in the group
The word "Comment" con the Activity text needs to be a hyperlink to the comment detail page
Design Link: https://www.figma.com/design/XRD6wSl1m8Kz6XUcAy5CLp/BaseApp---WEB?node-id=3738-46140&t=0SETItr5R9RznbuZ-4
Approvd
https://app.approvd.io/silverlogic/BA/stories/34835
Summary by CodeRabbit
New Features
Improvements
Version Update