Ba 2114 add members to existing group #206
Conversation
Ronan-Fernandes
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughThis pull request introduces extensive updates across messaging and design modules. It reintroduces and updates imports and constants, enhances GraphQL fragments with additional fields (e.g., role), and refines utility functions such as Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (3)
packages/components/modules/messages/__shared__/LeaveGroupDialog/index.tsx (1)
51-76: Add error handling for mutation variables.The mutation could fail silently if
roomIdorprofileIdare undefined. Consider adding error handling and user feedback.const onRemoveConfirmed = () => { - if (!roomId || !profileId) return + if (!roomId || !profileId) { + sendToast('Unable to process request: Missing required information', { type: 'error' }) + onClose() + return + } commit({ // ... rest of the code }) }packages/components/modules/messages/utils.ts (1)
14-25: Add error handling for fragment data.The
useGroupNameAndAvatarfunction doesn't handle the case where the fragment data is invalid or missing.export const useGroupNameAndAvatar = ( headerRef: GroupTitleFragment$key | RoomTitleFragment$key | null | undefined, ) => { + if (!headerRef) { + return { + title: undefined, + avatar: undefined, + } + } const header = useFragment<GroupTitleFragment$key>( GroupTitleFragment, headerRef as GroupTitleFragment$key, ) return { title: header?.title, avatar: header?.image?.url, } }packages/components/modules/messages/GroupDetails/index.tsx (1)
40-42: Add null check for connections array.The connections array could be undefined if
group?.idis falsy, which might cause issues in the subscription.Apply this diff to add a null check:
- const connections = group?.id - ? [ConnectionHandler.getConnectionID(group.id, 'ChatRoom_participants')] - : [] + const connections = group?.id + ? [ConnectionHandler.getConnectionID(group.id, 'ChatRoom_participants')] + : undefined
🧹 Nitpick comments (32)
packages/components/modules/messages/EditGroup/types.ts (1)
12-26: LGTM! Well-structured interface with good type safety.The interface follows good practices with proper typing and component composition pattern.
Consider these improvements:
- Add JSDoc comments to document the default behavior when optional UI components are not provided
- Consider making props required when their corresponding component is provided. For example:
- Searchbar?: FC<SearchbarProps> - SearchbarProps?: Partial<SearchbarProps> + Searchbar?: FC<SearchbarProps> + SearchbarProps?: Partial<SearchbarProps> & (Searchbar extends undefined ? object : Required<{}>)This pattern ensures that if
Searchbaris provided,SearchbarPropsmust also be provided.packages/design-system/components/icons/FilledCloseIcon/index.tsx (1)
6-15: Consider making the icon size configurable.While the implementation is solid, the hardcoded fontSize might limit reusability. Consider making it configurable through props with a default value.
- <SvgIcon sx={{ fontSize: 24, color: 'grey.600', ...sx }} {...props}> + <SvgIcon sx={{ color: 'grey.600', ...sx }} {...props}>This way, the icon size can be controlled through the fontSize prop or sx prop, making the component more flexible while maintaining the same default behavior through the theme.
packages/design-system/components/dialogs/ConfirmDialog/index.tsx (1)
23-25: Consider prop order to handle overrides correctly.The spread props might conflict with built-in props. To ensure proper overriding:
- Move spread props before built-in props to allow built-in props to take precedence
- Or document the override behavior for consumers
Example for DialogTitle:
- <DialogTitle variant="h6" {...DialogTitleProps}> + <DialogTitle {...DialogTitleProps} variant="h6">Similar changes apply to DialogContent and DialogActions components.
Also applies to: 27-35, 37-44
packages/components/modules/messages/__shared__/LeaveGroupDialog/constants.ts (1)
16-36: Consider these improvements to the text copy structure.
- The
IS_REMOVINGaction only includes text for theADMINrole. Consider if theMEMBERrole should also have text defined for consistency.- The long text strings could benefit from being broken into multiple lines for better readability.
- Consider extracting these strings to a localization system for future internationalization support.
Example of improved string formatting:
[LEAVE_GROUP_DIALOG_TEXT_COPY_TYPE_KEYS.CONTENT]: - 'You can choose a new admin from the people listed under members. If you leave the group without choosing a new admin, the most senior group member will become admin.', + 'You can choose a new admin from the people listed under members. ' + + 'If you leave the group without choosing a new admin, ' + + 'the most senior group member will become admin.',packages/components/modules/messages/EditGroup/index.tsx (4)
41-55: Consider improving type safety by replacinganytypes.The component props use
anytypes for several optional components and their props. Consider creating proper type definitions to improve type safety and maintainability.- allProfilesRef: any, + allProfilesRef: PreloadedQuery<AllProfilesQuery>, - ProfileCardProps?: any, + ProfileCardProps?: Partial<ProfileCardComponentProps>,
73-79: Consider improving type safety in the useMemo hook.The type assertion in the useMemo hook could be made more type-safe by properly typing the edge parameter and avoiding the
anytype.- (edge: any) => edge?.node?.profile && edge.node.profile, + (edge: { node: { profile: ProfileNode | null } } | null) => + edge?.node?.profile ?? null,
114-114: Consider using undefined assignment instead of delete operator.The static analysis tool flagged the use of the delete operator. While the performance impact is minimal in this case, you can improve it by using undefined assignment.
- delete dirtyValues.participants + dirtyValues.participants = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 114-114: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
156-162: Consider renaming onClose to better reflect its purpose.The
onCloseprop passed to AddMembersDialog is actually handling successful member addition. Consider renaming it to better reflect its purpose.<AddMembersDialog open={open} allProfilesRef={allProfilesRef} - onClose={handleAddMemberSuccess} + onSuccess={handleAddMemberSuccess} profileId={profileId} roomId={roomId} />packages/components/modules/messages/EditGroup/constants.ts (1)
6-6: Consider adding validation or default fallbacks.Returning an object literal for
CreateOrEditGroupis clear and concise. However, missing or empty strings (fortitle) or undefined images might have downstream effects. Consider adding input validation or using default placeholders fortitle/image. This ensures the function can gracefully handle all edge cases.packages/design-system/components/buttons/AvatarButton/types.ts (1)
1-6: Add optional alt-text usage guidelines for accessibility.The interface is straightforward. For improved accessibility, consider making
altmandatory or adding guidance on providing descriptive text when an image is available. Also, verify whetheronClickshould receive mouse event arguments for more complex interaction scenarios.packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts (1)
11-11: Consider adding specific types for the refetch function.The
anytype inRefetchFnDynamic<any, any>could be replaced with more specific types to improve type safety.- refetch?: RefetchFnDynamic<any, any> + refetch?: RefetchFnDynamic<ChatRoomsQuery$data, Record<string, unknown>>packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/styled.tsx (1)
12-12: Remove commented code.Clean up the commented-out padding line if it's no longer needed.
packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/styled.tsx (1)
4-16: Consider improving responsive design implementation.A few suggestions to enhance the component:
- The fixed
48pxwidth ingridTemplateColumnsmight not scale well on very small screens- The padding in the media query appears redundant as it's the same as the default
Consider this implementation:
export const MainContainer = styled(Box)(({ theme }) => ({ alignItems: 'center', display: 'grid', width: '100%', height: '100%', - gridTemplateColumns: '48px auto min-content', + gridTemplateColumns: 'minmax(32px, 48px) auto min-content', gap: theme.spacing(1.5), padding: theme.spacing(1.5), [theme.breakpoints.down('sm')]: { maxWidth: '600px', - padding: `${theme.spacing(1.5)} ${theme.spacing(1.5)}`, }, }))packages/design-system/components/buttons/AvatarButton/__storybook__/stories.tsx (1)
17-18: Enhance stories with proper action handlers and accessibility testing.The stories use
console.logfor click handlers which isn't ideal for production code. Consider these improvements:
- Use Storybook's action handlers
- Add more variants (disabled state, loading state)
- Include accessibility testing
Example implementation:
- onClick: () => console.log('clicked'), + onClick: action('button-clicked'),Also consider adding:
export const Disabled: Story = { args: { onClick: action('button-clicked'), disabled: true, }, } export const Loading: Story = { args: { onClick: action('button-clicked'), loading: true, }, }Also applies to: 23-24
packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/types.ts (2)
13-14: Consider using a more specific type forloadNext.Instead of using
LoadMoreFn<any>, consider using a more specific type that matches your pagination query type to ensure type safety.- loadNext?: LoadMoreFn<any> + loadNext?: LoadMoreFn<ProfilesListPaginationQuery>
15-15: Consider using more specific types forVirtuosoProps.Instead of using
anyfor both type parameters ofVirtuosoProps, consider using more specific types that match your data structure.- VirtuosoProps?: Partial<VirtuosoProps<any, any>> + VirtuosoProps?: Partial<VirtuosoProps<ProfileNode, unknown>>packages/components/modules/messages/CreateGroup/types.ts (1)
15-16: Consider renaming ConnectionsList props for consistency.Since the codebase is moving towards using
ProfilesList, consider renaming these props to maintain consistency:- ConnectionsList?: FC<ProfilesListProps> - ConnectionsListProps?: Partial<ProfilesListProps> + ProfilesList?: FC<ProfilesListProps> + ProfilesListProps?: Partial<ProfilesListProps>packages/components/modules/messages/__shared__/types.ts (1)
9-13: Consider using more specific types for participants arrays.Using
any[]reduces type safety. Consider usingProfileNode[]or a more specific type that matches your participant data structure.export interface AddRemoveParticipants { - addParticipants?: any[] - participants?: any[] - removeParticipants?: any[] + addParticipants?: ProfileNode[] + participants?: ProfileNode[] + removeParticipants?: ProfileNode[] }packages/design-system/components/buttons/AvatarButton/index.tsx (1)
38-38: Consider specifying loading behavior for the Image component.The Next.js Image component should have a loading behavior specified for better user experience.
- {imageString && <Image src={imageString} alt={alt} width={24} height={24} />} + {imageString && <Image src={imageString} alt={alt} width={24} height={24} loading="lazy" />}packages/components/modules/messages/__shared__/constants.ts (2)
17-27: Consider using a type-safe key mapping.The current implementation uses string literals which could lead to typos.
Consider using this type-safe approach:
-export const CREATE_OR_EDIT_GROUP_FORM_VALUE: Record< - keyof CreateOrEditGroup, - keyof CreateOrEditGroup -> & - Record<keyof TitleAndImage, keyof TitleAndImage> = { - title: 'title', - participants: 'participants', - addParticipants: 'addParticipants', - removeParticipants: 'removeParticipants', - image: 'image', -} +export const CREATE_OR_EDIT_GROUP_FORM_VALUE = { + title: 'title', + participants: 'participants', + addParticipants: 'addParticipants', + removeParticipants: 'removeParticipants', + image: 'image', +} as const satisfies Record<keyof (CreateOrEditGroup & TitleAndImage), string>
37-45: Consider strengthening form validation.The current validation schema could be enhanced for better data integrity.
Consider these improvements:
export const DEFAULT_CREATE_OR_EDIT_GROUP_FORM_VALIDATION = z.object({ - [CREATE_OR_EDIT_GROUP_FORM_VALUE.title]: z.string().min(1, { message: 'Please enter a title' }), + [CREATE_OR_EDIT_GROUP_FORM_VALUE.title]: z.string().min(1, { message: 'Please enter a title' }).max(100, { message: 'Title is too long' }).trim(), [CREATE_OR_EDIT_GROUP_FORM_VALUE.addParticipants]: z.array(z.any()), [CREATE_OR_EDIT_GROUP_FORM_VALUE.participants]: z .array(z.any()) .min(1, { message: 'Please select at least one member' }), [CREATE_OR_EDIT_GROUP_FORM_VALUE.removeParticipants]: z.array(z.any()), - [CREATE_OR_EDIT_GROUP_FORM_VALUE.image]: z.any(), + [CREATE_OR_EDIT_GROUP_FORM_VALUE.image]: z.string().optional(), })packages/components/modules/messages/__shared__/GroupChatMembersList/types.ts (1)
14-36: Consider improving type safety and documentation.The interface could benefit from stronger typing and JSDoc documentation.
Consider these improvements:
+/** + * Props for the GroupChatMembersList component + * @template T - The type of the form values + */ -export interface GroupChatMembersListProps extends PropsWithChildren { +export interface GroupChatMembersListProps<T extends CreateOrEditGroup = CreateOrEditGroup> extends PropsWithChildren { FORM_VALUE: Record<keyof CreateOrEditGroup, keyof CreateOrEditGroup> - setValue: UseFormSetValue<any> - watch: UseFormWatch<CreateOrEditGroup> + setValue: UseFormSetValue<T> + watch: UseFormWatch<T> currentParticipants: ProfileNode[] connections?: ProfileNode[] refetch?: RefetchFnDynamic<any, any>packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/index.tsx (1)
20-41: Consider extracting styles to a styled component.The inline styles could be moved to a styled component for better maintainability.
Consider creating a styled component for the IconButton:
+const StyledIconButton = styled(IconButton)({ + position: 'absolute', + top: 0, + right: 0, + zIndex: 1, +}) return ( <MainContainer key={`chat-room-item-${id}`}> <Box sx={{ position: 'relative' }}> - <IconButton - size="small" - sx={{ - position: 'absolute', - top: 0, - right: 0, - zIndex: 1, - }} - onClick={() => handleRemoveMember(profileRef)} - > + <StyledIconButton size="small" onClick={() => handleRemoveMember(profileRef)}>packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/index.tsx (2)
22-29: Consider memoizing the event handler.The event handler is recreated on every render.
Consider using
useCallback:+import { FC, useCallback } from 'react' const AddMembersCard: FC<AddMembersCardProps> = ({ profile: profileRef, handleAddMember, handleRemoveMember, isMember = false, }) => { const { id, image, name, urlPath } = useFragment(ProfileItemFragment, profileRef) - const handleCheckboxChange = (event: React.ChangeEvent<HTMLInputElement>) => { + const handleCheckboxChange = useCallback((event: React.ChangeEvent<HTMLInputElement>) => { const { target } = event if (target.checked) { handleAddMember(profileRef) } else { handleRemoveMember(profileRef) } - } + }, [handleAddMember, handleRemoveMember, profileRef])
39-44: Consider handling missing urlPath gracefully.The current implementation might render an empty
@whenurlPath.pathis undefined.Consider this improvement:
<Typography variant="caption" color="text.secondary"> - {urlPath?.path && `@${urlPath.path}`} + {urlPath?.path ? `@${urlPath.path}` : null} </Typography>packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/index.tsx (2)
78-79: Consider memoizing the itemContent callback.The
itemContentcallback passed to Virtuoso is recreated on every render. This could impact performance for large lists.+import { useCallback } from 'react' const ProfilesList: FC<ProfilesListProps> = ({ // ...other props }) => { + const itemContent = useCallback( + (_index: number, item: ProfileNode) => renderItem(item), + [renderItem] + ) return ( <Virtuoso data={profiles} - itemContent={(_index, item) => renderItem(item)} + itemContent={itemContent} // ...other props /> ) }
91-93: Consider using Virtuoso for non-paginated lists as well.The non-paginated implementation doesn't use virtualization, which could lead to performance issues with large lists.
-<Box maxHeight={250} overflow="auto" sx={{ scrollbarWidth: 'none' }} {...NormalListProps}> - {profiles.map((member: ProfileNode) => renderItem(member, true))} -</Box> +<Virtuoso + data={profiles} + itemContent={(_index, item) => renderItem(item, true)} + style={{ scrollbarWidth: 'none', maxHeight: '250px' }} + {...NormalListProps} +/>packages/components/modules/messages/__shared__/LeaveGroupDialog/index.tsx (1)
33-49: Consider extracting text copy logic to a separate utility.The
getLeaveGroupDialogTextCopyfunction contains complex nested conditionals. Moving this to a separate utility would improve maintainability.+const getTextCopyKey = ( + isLeaving: boolean, + isSoleAdmin: boolean, +): { action: string; role: string } => ({ + action: isLeaving ? LEAVE_GROUP_DIALOG_TEXT_COPY_ACTION_KEYS.IS_LEAVING : LEAVE_GROUP_DIALOG_TEXT_COPY_ACTION_KEYS.IS_REMOVING, + role: isLeaving && isSoleAdmin ? LEAVE_GROUP_DIALOG_TEXT_COPY_ROLE_KEYS.ADMIN : LEAVE_GROUP_DIALOG_TEXT_COPY_ROLE_KEYS.MEMBER, +}) +const getLeaveGroupDialogTextCopy = ( + type: ValueOf<typeof LEAVE_GROUP_DIALOG_TEXT_COPY_TYPE_KEYS>, + isLeaving: boolean, + isSoleAdmin: boolean, +) => { + const { action, role } = getTextCopyKey(isLeaving, isSoleAdmin) + return LEAVE_GROUP_DIALOG_TEXT_COPY[action][role][type] +}packages/components/modules/messages/__shared__/GroupChatMembersList/index.tsx (1)
79-90: Memoize the renderItem function.The
renderItemfunction is recreated on every render, which could impact performance.-const renderItem = (profile: ProfileNode, isMember = false) => { +const renderItem = useCallback((profile: ProfileNode, isMember = false) => { if (!profile) return null return ( <ProfileCard profile={profile} handleAddMember={handleAddMember} handleRemoveMember={handleRemoveMember} isMember={isMember} {...ProfileCardProps} /> ) -} +}, [handleAddMember, handleRemoveMember, ProfileCardProps])packages/components/modules/messages/CreateGroup/index.tsx (2)
3-3: LGTM! Consider optimizing type imports.The reorganization of imports and use of shared components improves code maintainability.
Consider using type-only imports for types to optimize bundle size:
-import { CreateOrEditGroup, ProfileNode } from '../__shared__/types' +import type { CreateOrEditGroup, ProfileNode } from '../__shared__/types'Also applies to: 14-30
79-116: Consider enhancing error handling.While the mutation handling is generally good, consider adding more specific error handling:
onCompleted: (response) => { const errors = response?.chatRoomCreate?.errors if (errors) { sendToast('Something went wrong', { type: 'error' }) + // Display specific error message if available + const errorMessage = errors[0]?.messages?.[0] || 'Something went wrong' + sendToast(errorMessage, { type: 'error' }) setFormRelayErrors(formReturn, errors) } else { + sendToast('Group created successfully', { type: 'success' }) setChatRoom({ id: response?.chatRoomCreate?.room?.node?.id }) onValidSubmission() } },packages/design-system/components/buttons/AvatarButton/__storybook__/IconButton.mdx (1)
1-63: Enhance documentation with accessibility guidelines.The documentation is well-structured but could benefit from additional information.
Consider adding:
- Accessibility (a11y) guidelines
- Keyboard interaction patterns
- Visual states documentation (hover, focus, active)
- Size variants if applicable
- Code examples for different use cases
Example addition:
+## Accessibility + +- Role: The IconButton uses the native `button` role +- Keyboard navigation: Focusable and clickable with Enter/Space +- ARIA: Requires `aria-label` when no visible text is present + +## Keyboard Interactions + +- `Enter` or `Space`: Triggers the button's action +- `Tab`: Moves focus to the button + +## Visual States + +- Default: Normal button state +- Hover: Visual feedback on mouse hover +- Focus: Visible focus ring for keyboard navigation +- Active: Visual feedback when clicked +- Disabled: Greyed out, non-interactive
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
packages/components/__generated__/ChatRoomQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ChatRoomsQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/CreateChatRoomMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/RoomTitleFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/UpdateChatRoomMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/chatRoomsPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/useRoomListSubscription.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (51)
packages/components/modules/messages/ChatRoom/ChatRoomHeader/index.tsx(3 hunks)packages/components/modules/messages/CreateChatRoomList/index.tsx(2 hunks)packages/components/modules/messages/CreateGroup/ConnectionsList/index.tsx(0 hunks)packages/components/modules/messages/CreateGroup/constants.ts(0 hunks)packages/components/modules/messages/CreateGroup/index.tsx(5 hunks)packages/components/modules/messages/CreateGroup/styled.tsx(0 hunks)packages/components/modules/messages/CreateGroup/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/styled.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/styled.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/constants.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/styled.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts(1 hunks)packages/components/modules/messages/EditGroup/constants.ts(1 hunks)packages/components/modules/messages/EditGroup/index.tsx(4 hunks)packages/components/modules/messages/EditGroup/types.ts(1 hunks)packages/components/modules/messages/GroupDetails/ProfileCard/constants.ts(0 hunks)packages/components/modules/messages/GroupDetails/ProfileCard/index.tsx(1 hunks)packages/components/modules/messages/GroupDetails/index.tsx(3 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/types.ts(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/types.ts(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/ProfileCard/index.tsx(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/ProfileCard/types.ts(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/index.tsx(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/types.ts(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/index.tsx(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/styled.tsx(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/types.ts(1 hunks)packages/components/modules/messages/__shared__/LeaveGroupDialog/constants.ts(1 hunks)packages/components/modules/messages/__shared__/LeaveGroupDialog/index.tsx(3 hunks)packages/components/modules/messages/__shared__/LeaveGroupDialog/types.ts(1 hunks)packages/components/modules/messages/__shared__/constants.ts(1 hunks)packages/components/modules/messages/__shared__/types.ts(1 hunks)packages/components/modules/messages/graphql/fragments/RoomTitle.ts(1 hunks)packages/components/modules/messages/types.ts(0 hunks)packages/components/modules/messages/utils.ts(3 hunks)packages/components/schema.graphql(2 hunks)packages/design-system/components/buttons/AvatarButton/__storybook__/IconButton.mdx(1 hunks)packages/design-system/components/buttons/AvatarButton/__storybook__/stories.tsx(1 hunks)packages/design-system/components/buttons/AvatarButton/index.tsx(1 hunks)packages/design-system/components/buttons/AvatarButton/styled.tsx(1 hunks)packages/design-system/components/buttons/AvatarButton/types.ts(1 hunks)packages/design-system/components/buttons/index.ts(1 hunks)packages/design-system/components/dialogs/ConfirmDialog/index.tsx(2 hunks)packages/design-system/components/dialogs/ConfirmDialog/types.ts(2 hunks)packages/design-system/components/icons/FilledCloseIcon/index.tsx(1 hunks)packages/design-system/components/icons/index.ts(1 hunks)
💤 Files with no reviewable changes (5)
- packages/components/modules/messages/GroupDetails/ProfileCard/constants.ts
- packages/components/modules/messages/CreateGroup/constants.ts
- packages/components/modules/messages/CreateGroup/styled.tsx
- packages/components/modules/messages/CreateGroup/ConnectionsList/index.tsx
- packages/components/modules/messages/types.ts
✅ Files skipped from review due to trivial changes (6)
- packages/design-system/components/icons/index.ts
- packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/types.ts
- packages/components/modules/messages/shared/GroupChatMembersList/ProfileCard/index.tsx
- packages/components/modules/messages/MessagesList/MessagesGroup/types.ts
- packages/components/modules/messages/shared/GroupChatMembersList/ProfileCard/types.ts
- packages/components/modules/messages/GroupDetails/ProfileCard/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/messages/EditGroup/index.tsx
[error] 114-114: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (33)
packages/components/modules/messages/EditGroup/types.ts (1)
1-10: LGTM! Well-organized imports.The import statements are properly organized with clear separation between external dependencies and internal modules. Good use of generated types from GraphQL queries.
packages/design-system/components/icons/FilledCloseIcon/index.tsx (3)
1-3: LGTM! Clean and focused imports.The imports are minimal and appropriate for the component's needs.
5-5: LGTM! Well-structured component definition.The component is properly typed with FC and uses clean props destructuring pattern.
18-18: LGTM! Clean export.The default export is appropriate for a single component file.
packages/design-system/components/dialogs/ConfirmDialog/types.ts (2)
3-3: LGTM! Clean import of MUI Dialog props types.The import is correctly organized and provides the necessary types for the new props.
13-15: LGTM! Well-structured prop types for dialog customization.The new optional props are correctly typed using
Partial<T>, which:
- Allows flexible customization of MUI Dialog components
- Maintains backward compatibility
- Follows MUI's patterns for prop types
packages/design-system/components/dialogs/ConfirmDialog/index.tsx (1)
17-19: LGTM! Props initialized with safe defaults.Empty objects as default values prevent undefined errors while maintaining backward compatibility.
packages/components/modules/messages/__shared__/LeaveGroupDialog/constants.ts (3)
1-4: LGTM! Well-structured action keys.The constant follows TypeScript best practices with 'as const' assertion and has clear, descriptive keys.
6-9: LGTM! Clear role definitions.The roles are well-defined and follow the same pattern of using 'as const' for type safety.
11-14: LGTM! Appropriate text type keys.The text type keys are well-structured and maintain consistency with the pattern.
packages/components/modules/messages/EditGroup/index.tsx (1)
46-52: LGTM! Well-structured component customization.The implementation provides good flexibility through default components and customizable props, with clean state management for the dialog.
Also applies to: 57-57
packages/components/modules/messages/EditGroup/constants.ts (1)
1-1: No concerns with the updated import.The switch from
EditGroupUploadtoCreateOrEditGroupappears consistent with the refactoring goals.packages/components/modules/messages/EditGroup/AddMembersDialog/styled.tsx (1)
1-6: Styling and layout look fine.Using
theme.spacingmaintains consistency with the Material-UI design system. No issues found here.packages/components/modules/messages/__shared__/LeaveGroupDialog/types.ts (1)
1-10: LGTM! Interface changes improve type safety and customization.The changes to
LeaveGroupDialogPropsinterface are well-structured:
- Making
profileIdandremovingParticipantIdrequired improves type safety- Adding
customContentandcustomTitleprovides better customization- Adding
isSoleAdminenables role-based UI/UX decisionspackages/design-system/components/buttons/index.ts (1)
1-2: LGTM! Export follows consistent pattern.The export of
AvatarButtonand its types follows the same pattern as other button exports in the file.packages/components/modules/messages/EditGroup/AddMembersDialog/constants.ts (1)
1-11: LGTM! Clean and well-structured form validation.The validation schema is correctly defined, properly extending the base validation and adding the title field.
packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/styled.tsx (1)
8-8: Review the maxWidth constraint.A maxWidth of 64px seems very restrictive for a container that includes an avatar and potentially other content. Consider if this constraint is intentional or needs adjustment.
packages/design-system/components/buttons/AvatarButton/styled.tsx (1)
4-13: LGTM! Well-structured responsive layout.The component is well-implemented with:
- Clear grid structure
- Proper responsive design using breakpoints
- Consistent theme spacing usage
packages/components/modules/messages/graphql/fragments/RoomTitle.ts (1)
17-17: Verify the type of the role field.The
rolefield has been added but its type isn't apparent from the fragment. This information is crucial for type safety and documentation.Let's check the GraphQL schema for the role field definition:
packages/components/modules/messages/__shared__/constants.ts (1)
8-15: LGTM! Well-structured role definitions.The role constants and types are well-defined using TypeScript's const assertions and the
ValueOfutility type.packages/components/modules/messages/CreateChatRoomList/index.tsx (2)
7-7: LGTM! Component simplification improves maintainability.The replacement of multiple components with a single
AvatarButtoncomponent simplifies the code while maintaining functionality.Also applies to: 132-137
19-19: LGTM! Better organization of shared types.Moving types to a shared directory improves code organization and reusability.
packages/components/modules/messages/ChatRoom/ChatRoomHeader/index.tsx (2)
41-42: LGTM! Enhanced role-based access control.The addition of
useCheckIsAdminhook centralizes admin role checking logic, improving maintainability.
114-114: Address TODO comment about sole admin removal.The TODO comment indicates that sole admin removal functionality needs to be implemented.
Would you like me to help implement the sole admin removal functionality or create an issue to track this task?
packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx (3)
43-47: LGTM! Well-structured form management.The form setup with Zod validation and proper mode configuration follows best practices.
143-146: Review the fixed height style.Using a fixed
height: '100vh'withmaxHeight: '500px'might cause layout issues on different screen sizes.Consider using responsive values or container-relative heights instead.
74-82: LGTM! Proper error handling.The mutation error handling with user feedback and form error setting is well implemented.
packages/components/modules/messages/GroupDetails/index.tsx (2)
55-55: LGTM! Improved admin role management.Using the
useCheckIsAdminhook centralizes the admin role checking logic and makes it more maintainable.
114-114: Address TODO comment about sole admin removal.The TODO comment about sole admin removal should be addressed to ensure proper handling of admin roles.
Would you like me to help implement the sole admin removal functionality or create an issue to track this task?
packages/components/modules/messages/CreateGroup/index.tsx (3)
35-72: LGTM! Well-structured form setup.The form configuration is well-organized with:
- Proper default values and validation
- Clear prop definitions
- Consistent error handling
118-131: LGTM! Well-implemented participant handling.Good use of:
- Proper type safety with ProfileNode
- Efficient memoization with correct dependencies
- Clear filtering logic
175-193: LGTM! Clean UI implementation.The GroupChatMembersList integration:
- Reduces code duplication
- Improves maintainability
- Properly handles all required functionality
packages/components/schema.graphql (1)
191-191: LGTM! Well-defined schema changes.The addition of
addedParticipantsfield:
- Maintains symmetry with existing
removedParticipantsfield- Uses correct type ([ChatRoomParticipant])
- Supports proper participant tracking
Also applies to: 315-315
packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/types.ts
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/types.ts
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/__shared__/GroupChatMembersList/styled.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/__shared__/GroupChatMembersList/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/__shared__/GroupChatMembersList/index.tsx
Outdated
Show resolved
Hide resolved
Ronan-Fernandes
force-pushed
the
BA-2114-add-members-to-existing-group
branch
from
65e4af2 to
a54ad85
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/types.ts (1)
13-13: Consider using a more specific type instead of 'any'.The use of
LoadMoreFn<any>reduces type safety. Consider creating a generic type parameter for the interface to maintain type safety:-export interface ProfilesListProps { +export interface ProfilesListProps<T = any> { profiles: ProfileNode[] isLoadingNext?: boolean hasNext?: boolean - loadNext?: LoadMoreFn<any> + loadNext?: LoadMoreFn<T>packages/components/modules/messages/EditGroup/constants.ts (1)
19-22: Consider using a more specific validation type for participants.Using
z.any()for participants validation is too permissive and could allow invalid data to pass through. Consider using a more specific validation schema.export const DEFAULT_FORM_VALIDATION = z.object({ ...DEFAULT_CREATE_OR_EDIT_GROUP_FORM_VALIDATION.shape, - [CREATE_OR_EDIT_GROUP_FORM_VALUE.participants]: z.any(), + [CREATE_OR_EDIT_GROUP_FORM_VALUE.participants]: z.array( + z.object({ + id: z.string(), + // Add other required profile fields + }) + ).optional(), })packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx (2)
76-85: Enhance error handling in onSubmit.The error handling could be improved by providing more specific error messages and handling network errors.
onCompleted: (response) => { const errors = response?.chatRoomUpdate?.errors if (errors) { - sendToast('Something went wrong', { type: 'error' }) + const errorMessage = errors.map(e => e.message).join(', ') + sendToast(errorMessage || 'Failed to update chat room', { type: 'error' }) setFormRelayErrors(formReturn, errors) } else { onClose() reset() } }, +onError: (error) => { + sendToast('Network error occurred. Please try again.', { type: 'error' }) + console.error('Update chat room error:', error) +},
146-149: Avoid using viewport height for scrollable containers.Using
100vhfor height can cause issues on mobile browsers due to dynamic viewport changes. Consider using a fixed height or dynamic calculation.VirtuosoProps: { style: { - height: '100vh', + height: '500px', maxHeight: '500px', }, },packages/components/modules/messages/EditGroup/index.tsx (3)
59-76: Consider enhancing error handling for data fetching.While the pagination and data fetching implementation is solid, consider adding error boundaries or error states to handle potential failures in data loading.
const { data: membersList, loadNext, isLoadingNext, hasNext, refetch, + hasError, } = usePaginationFragment<ChatRoomParticipantsPaginationQuery, MembersListFragment$key>( MembersListFragment, group, ) +if (hasError) { + return ( + <Box> + <Typography color="error"> + Failed to load members. Please try again. + </Typography> + </Box> + ) +}
111-111: Consider using undefined assignment instead of delete operator.While the performance impact is minimal in this case, following the static analysis suggestion could lead to more consistent code.
-delete dirtyValues.participants +dirtyValues.participants = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 111-111: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
155-162: Enhance dialog accessibility.Consider adding ARIA labels and roles to improve accessibility of the AddMembersDialog.
<AddMembersDialog open={open} allProfilesRef={allProfilesRef} onClose={handleAddMemberSuccess} profileId={profileId} roomId={roomId} isPending={isPending} + aria-label="Add members to group" + role="dialog" />packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/index.tsx (2)
15-33: Consider adding type for loadNext parameter.The
loadNextfunction is called with a numeric parameter (5) but the parameter type isn't explicitly defined in the props interface.Consider updating the type definition to explicitly define the parameter type:
interface ProfilesListProps { // ... other props loadNext?: (count: number) => void; // ... }
45-50: Consider optimizing conditional rendering logic.The conditional rendering could be more maintainable by combining the conditions.
Consider this refactor:
- if (!isPending && searchValue && emptyProfilesList && isPaginated) return <SearchNotFoundState /> - - if (!isPending && emptyProfilesList && isPaginated) return <EmptyProfilesListState /> + if (!isPending && emptyProfilesList && isPaginated) { + return searchValue ? <SearchNotFoundState /> : <EmptyProfilesListState /> + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
packages/components/modules/messages/CreateChatRoomList/index.tsx(2 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/styled.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/styled.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/constants.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/styled.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts(1 hunks)packages/components/modules/messages/EditGroup/constants.ts(1 hunks)packages/components/modules/messages/EditGroup/index.tsx(4 hunks)packages/components/modules/messages/EditGroup/types.ts(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/types.ts(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/types.ts(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/ProfileCard/types.ts(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/index.tsx(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/types.ts(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/index.tsx(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/types.ts(1 hunks)packages/components/schema.graphql(2 hunks)packages/design-system/components/buttons/AvatarButton/index.tsx(1 hunks)packages/design-system/components/buttons/AvatarButton/types.ts(1 hunks)packages/design-system/components/buttons/index.ts(1 hunks)packages/design-system/components/dialogs/ConfirmDialog/index.tsx(2 hunks)packages/design-system/components/dialogs/ConfirmDialog/types.ts(2 hunks)packages/design-system/components/icons/FilledCloseIcon/index.tsx(1 hunks)packages/design-system/components/icons/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (22)
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/types.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/styled.tsx
- packages/components/schema.graphql
- packages/components/modules/messages/EditGroup/AddMembersDialog/constants.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/styled.tsx
- packages/design-system/components/icons/FilledCloseIcon/index.tsx
- packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/types.ts
- packages/design-system/components/buttons/index.ts
- packages/design-system/components/icons/index.ts
- packages/design-system/components/dialogs/ConfirmDialog/types.ts
- packages/components/modules/messages/shared/GroupChatMembersList/ProfileCard/types.ts
- packages/design-system/components/buttons/AvatarButton/index.tsx
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/types.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/index.tsx
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/index.tsx
- packages/components/modules/messages/shared/GroupChatMembersList/types.ts
- packages/design-system/components/buttons/AvatarButton/types.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/styled.tsx
- packages/components/modules/messages/MessagesList/MessagesGroup/types.ts
- packages/components/modules/messages/CreateChatRoomList/index.tsx
- packages/design-system/components/dialogs/ConfirmDialog/index.tsx
- packages/components/modules/messages/EditGroup/types.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/messages/EditGroup/index.tsx
[error] 111-111: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (13)
packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/types.ts (3)
1-7: LGTM! Import statements are well-organized.The new BoxProps import is correctly added to support the NormalListProps property.
9-12: LGTM! Well-structured interface with clear property definitions.The interface is well-organized with a good balance of required and optional properties. The renaming from ConnectionsListProps to ProfilesListProps better reflects its broader usage.
Also applies to: 14-22
23-26: LGTM! Good UI customization options.The new optional properties provide good flexibility for customizing the component's behavior and appearance.
packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts (1)
3-10: LGTM! Well-structured TypeScript interface.The
AddMembersDialogPropsinterface is well-defined with appropriate types and clear prop names.packages/components/modules/messages/__shared__/GroupChatMembersList/index.tsx (2)
45-50: Add debouncing to search functionality.The search triggers a refetch on every keystroke, which could lead to unnecessary API calls.
59-65: Add validation for member operations.The member addition function doesn't validate the profile data or handle potential errors.
packages/components/modules/messages/EditGroup/index.tsx (2)
3-28: LGTM! Well-organized imports.The imports are logically grouped and all appear to be used within the component.
38-52: LGTM! Well-structured props with good defaults.The component follows good practices by:
- Providing default components
- Including proper TypeScript types
- Setting sensible default values for optional props
packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/index.tsx (5)
1-14: LGTM! Well-organized imports.The imports are properly organized and all seem necessary for the component's functionality.
34-44: LGTM! Good accessibility practices.The loading state implementation includes proper accessibility attributes and clean conditional rendering.
54-67: Improve semantic HTML and accessibility.The
menuelement is not being used correctly, as noted in a previous review.
75-90: LGTM! Well-implemented virtualized list.The Virtuoso implementation with pagination and dynamic loading is clean and efficient.
76-90: Consider keyboard accessibility implications.The
scrollbarWidth: 'none'style might affect keyboard users' ability to track their position in the list.Consider making the scrollbar visible when using keyboard navigation:
-style={{ scrollbarWidth: 'none', maxHeight: '250px' }} +style={{ + maxHeight: '250px', + scrollbarWidth: 'none', + '&:focus-within': { + scrollbarWidth: 'auto' + } +}}Also applies to: 91-94
packages/design-system/components/buttons/AvatarButton/__storybook__/IconButton.mdx
Outdated
Show resolved
Hide resolved
packages/design-system/components/buttons/AvatarButton/index.tsx
Outdated
Show resolved
Hide resolved
packages/design-system/components/buttons/AvatarButton/styled.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/styled.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx
Outdated
Show resolved
Hide resolved
Ronan-Fernandes
force-pushed
the
BA-2114-add-members-to-existing-group
branch
from
a54ad85 to
c651779
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
packages/components/modules/messages/CreateChatRoomList/index.tsx (2)
104-105: Improve height calculation for virtualized list.The TODO comment and hardcoded height calculation could cause layout issues. Consider using CSS Grid or Flexbox for dynamic height calculation.
- style={{ scrollbarWidth: 'none', maxHeight: 'calc(100vh - 72px - 57px - 61px - 72px)' }} + style={{ + scrollbarWidth: 'none', + height: '100%' + }}Also update the parent container:
const MainContainer = styled(Box)({ display: 'flex', flexDirection: 'column', + height: '100vh', + overflow: 'hidden', })
46-51: Add debouncing to search functionality.The search triggers a refetch on every keystroke, which could lead to unnecessary API calls.
+import { useMemo } from 'react' +import debounce from 'lodash/debounce' + const debouncedRefetch = useMemo( + () => debounce((value: string) => refetch({ q: value }), 300), + [refetch] + ) const handleSearchChange: ChangeEventHandler<HTMLInputElement> = (e) => { const value = e.target.value || '' startTransition(() => { - refetch({ q: value }) + debouncedRefetch(value) }) }
♻️ Duplicate comments (1)
packages/components/modules/messages/utils.ts (1)
71-83: 🛠️ Refactor suggestionReplace 'any' type with proper interfaces.
The useCheckIsAdmin function uses unsafe 'any' types which bypass TypeScript's type checking.
Apply the following type definitions:
+interface Profile { + id?: string +} +interface ParticipantNode { + node?: { + profile?: Profile + role?: string + } +} +interface ParticipantsConnection { + edges?: ParticipantNode[] +} -export const useCheckIsAdmin = (participants: any) => { +export const useCheckIsAdmin = (participants: ParticipantsConnection) => { const { currentProfile } = useCurrentProfile() const me = participants?.edges?.find( - (edge: any) => edge?.node?.profile?.id === currentProfile?.id, + (edge) => edge?.node?.profile?.id === currentProfile?.id, ) const isAdmin = me?.node?.role === CHAT_ROOM_PARTICIPANT_ROLES.admin const isSoleAdmin = isAdmin && participants?.edges?.filter( - (edge: any) => edge?.node?.role === CHAT_ROOM_PARTICIPANT_ROLES.admin, + (edge) => edge?.node?.role === CHAT_ROOM_PARTICIPANT_ROLES.admin, ).length === 1 return { isAdmin, isSoleAdmin } }
🧹 Nitpick comments (13)
packages/design-system/components/buttons/AvatarButton/__storybook__/IconButton.mdx (3)
5-10: Clear Component Overview with Minor Enhancement SuggestionsThe "Component Documentation" section and bullet-point description for the IconButton are clear and concise. For improved clarity, consider highlighting any unique behaviors or variants the IconButton offers beyond standard functionality (if applicable).
12-25: Comprehensive Use Cases, Consider Additional ContextThe list of use cases is thorough and provides good contextual examples for when to use an IconButton. Optionally, consider adding guidance on how this component differs from similar buttons (e.g., if there's any overlap or a specific niche compared to an AvatarButton) to aid developers in choosing the right component.
43-64: Effective Example Usage, Consider Interactive EnhancementsThe example usage code snippet is well-constructed and demonstrates practical integration of the IconButton. For future improvements, you might consider linking to an interactive demo or providing an additional example to showcase dynamic states (e.g., loading or disabled) of the component.
packages/components/modules/messages/__shared__/LeaveGroupDialog/index.tsx (2)
33-49: Consider simplifying the text copy logic.While functional, the nested conditional logic could be simplified for better readability.
Consider this more concise approach:
- const getLeaveGroupDialogTextCopy = ( - type: ValueOf<typeof LEAVE_GROUP_DIALOG_TEXT_COPY_TYPE_KEYS>, - ) => { - if (profileId === removingParticipantId) { - if (isSoleAdmin) { - return LEAVE_GROUP_DIALOG_TEXT_COPY[LEAVE_GROUP_DIALOG_TEXT_COPY_ACTION_KEYS.IS_LEAVING][ - LEAVE_GROUP_DIALOG_TEXT_COPY_ROLE_KEYS.ADMIN - ][type] - } - return LEAVE_GROUP_DIALOG_TEXT_COPY[LEAVE_GROUP_DIALOG_TEXT_COPY_ACTION_KEYS.IS_LEAVING][ - LEAVE_GROUP_DIALOG_TEXT_COPY_ROLE_KEYS.MEMBER - ][type] - } - return LEAVE_GROUP_DIALOG_TEXT_COPY[LEAVE_GROUP_DIALOG_TEXT_COPY_ACTION_KEYS.IS_REMOVING][ - LEAVE_GROUP_DIALOG_TEXT_COPY_ROLE_KEYS.ADMIN - ][type] - } + const getLeaveGroupDialogTextCopy = ( + type: ValueOf<typeof LEAVE_GROUP_DIALOG_TEXT_COPY_TYPE_KEYS>, + ) => { + const isLeaving = profileId === removingParticipantId + const action = isLeaving + ? LEAVE_GROUP_DIALOG_TEXT_COPY_ACTION_KEYS.IS_LEAVING + : LEAVE_GROUP_DIALOG_TEXT_COPY_ACTION_KEYS.IS_REMOVING + const role = isLeaving && !isSoleAdmin + ? LEAVE_GROUP_DIALOG_TEXT_COPY_ROLE_KEYS.MEMBER + : LEAVE_GROUP_DIALOG_TEXT_COPY_ROLE_KEYS.ADMIN + + return LEAVE_GROUP_DIALOG_TEXT_COPY[action][role][type] + }
51-76: Consider enhancing error handling.While the mutation logic is sound, consider providing more specific error messages to help users understand and resolve issues.
Consider adding error type handling:
onError: (error) => { - sendToast(error.message, { type: 'error' }) + const errorMessage = error.message.includes('permission') + ? 'You do not have permission to perform this action' + : error.message.includes('not found') + ? 'Member not found in the group' + : 'Failed to remove member. Please try again.' + sendToast(errorMessage, { type: 'error' }) },packages/components/modules/messages/__shared__/types.ts (2)
4-7: Consider refining the image type for better type safety.While the current implementation works, consider creating a more specific type for the image property to better handle different image scenarios.
+type ImageType = string | File | Blob | null export interface TitleAndImage { title: string - image?: string | File | Blob | null + image?: ImageType }
21-24: Consider consistent use of NonNullable.The
ProfileEdgetype doesn't useNonNullablewhileProfileNodedoes. Consider being consistent with nullability handling.export type AllProfiles = NonNullable<AllProfilesListFragment$data['allProfiles']> export type AllProfilesEdges = AllProfiles['edges'] -export type ProfileEdge = AllProfilesEdges[number] +export type ProfileEdge = NonNullable<AllProfilesEdges[number]> export type ProfileNode = NonNullable<AllProfilesEdges[number]>['node']packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/types.ts (1)
9-26: Add JSDoc comments to document the interface properties.The interface properties would benefit from documentation explaining their purpose and usage.
+/** + * Props for the ProfilesList component + */ export interface ProfilesListProps { + /** Array of profile nodes to display */ profiles: ProfileNode[] + /** Whether more profiles are being loaded */ isLoadingNext?: boolean + /** Whether there are more profiles to load */ hasNext?: boolean + /** Function to load more profiles */ loadNext?: LoadMoreFn<any> searchValue?: string | null VirtuosoProps?: Partial<VirtuosoProps<any, any>> NormalListProps?: Partial<BoxProps> isPending?: boolean label?: string title?: string renderItem: (profile: ProfileNode, isMember?: boolean) => JSX.Element | null SearchNotFoundState?: FC EmptyProfilesListState?: FC allowAddMember?: boolean onAddMemberClick?: () => void removeTitle?: boolean }packages/components/modules/messages/__shared__/GroupChatMembersList/index.tsx (1)
79-90: Memoize the renderItem function.The
renderItemfunction is recreated on every render. Consider memoizing it withuseCallback.- const renderItem = (profile: ProfileNode, isMember = false) => { + const renderItem = useCallback((profile: ProfileNode, isMember = false) => { if (!profile) return null return ( <ProfileCard profile={profile} handleAddMember={handleAddMember} handleRemoveMember={handleRemoveMember} isMember={isMember} {...ProfileCardProps} /> ) - } + }, [handleAddMember, handleRemoveMember, ProfileCardProps])packages/components/modules/messages/CreateGroup/index.tsx (2)
35-47: Consider adding proper type forallProfilesRef.The
allProfilesRefprop is implicitly typed asany. Consider defining a proper type for better type safety.- allProfilesRef, + allProfilesRef: PreloadedQuery<AllProfilesListQuery>,
79-82: Address the TODO comment about filter handling.The TODO comment indicates that filter handling needs to be implemented. Currently, it defaults
unreadMessagestofalse.Would you like me to help implement proper filter handling or create an issue to track this task?
Also applies to: 96-101
packages/components/modules/messages/EditGroup/index.tsx (2)
111-111: Consider using undefined assignment for better performance.Using the
deleteoperator can impact performance. Consider using undefined assignment instead.- delete dirtyValues.participants + dirtyValues.participants = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 111-111: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
144-151: Consider optimizing the refetch after adding members.The refetch after adding members might cause unnecessary rerenders. Consider using a more targeted update approach.
- refetch?.({}) + // Use ConnectionHandler to update the connection locally + ConnectionHandler.insertEdgeAfter( + store.get(roomId), + newEdge, + 'participants' + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
packages/components/__generated__/ChatRoomQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ChatRoomsQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/CreateChatRoomMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/RoomTitleFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/UpdateChatRoomMutation.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/chatRoomsPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/useRoomListSubscription.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (51)
packages/components/modules/messages/ChatRoom/ChatRoomHeader/index.tsx(3 hunks)packages/components/modules/messages/CreateChatRoomList/index.tsx(2 hunks)packages/components/modules/messages/CreateGroup/ConnectionsList/index.tsx(0 hunks)packages/components/modules/messages/CreateGroup/constants.ts(0 hunks)packages/components/modules/messages/CreateGroup/index.tsx(5 hunks)packages/components/modules/messages/CreateGroup/styled.tsx(0 hunks)packages/components/modules/messages/CreateGroup/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/styled.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/styled.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/constants.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/styled.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts(1 hunks)packages/components/modules/messages/EditGroup/constants.ts(1 hunks)packages/components/modules/messages/EditGroup/index.tsx(4 hunks)packages/components/modules/messages/EditGroup/types.ts(1 hunks)packages/components/modules/messages/GroupDetails/ProfileCard/constants.ts(0 hunks)packages/components/modules/messages/GroupDetails/ProfileCard/index.tsx(1 hunks)packages/components/modules/messages/GroupDetails/index.tsx(3 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/types.ts(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/types.ts(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/ProfileCard/index.tsx(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/ProfileCard/types.ts(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/index.tsx(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/types.ts(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/index.tsx(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/styled.tsx(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/types.ts(1 hunks)packages/components/modules/messages/__shared__/LeaveGroupDialog/constants.ts(1 hunks)packages/components/modules/messages/__shared__/LeaveGroupDialog/index.tsx(3 hunks)packages/components/modules/messages/__shared__/LeaveGroupDialog/types.ts(1 hunks)packages/components/modules/messages/__shared__/constants.ts(1 hunks)packages/components/modules/messages/__shared__/types.ts(1 hunks)packages/components/modules/messages/graphql/fragments/RoomTitle.ts(1 hunks)packages/components/modules/messages/types.ts(0 hunks)packages/components/modules/messages/utils.ts(3 hunks)packages/components/schema.graphql(2 hunks)packages/design-system/components/buttons/AvatarButton/__storybook__/IconButton.mdx(1 hunks)packages/design-system/components/buttons/AvatarButton/__storybook__/stories.tsx(1 hunks)packages/design-system/components/buttons/AvatarButton/index.tsx(1 hunks)packages/design-system/components/buttons/AvatarButton/styled.tsx(1 hunks)packages/design-system/components/buttons/AvatarButton/types.ts(1 hunks)packages/design-system/components/buttons/index.ts(1 hunks)packages/design-system/components/dialogs/ConfirmDialog/index.tsx(2 hunks)packages/design-system/components/dialogs/ConfirmDialog/types.ts(2 hunks)packages/design-system/components/icons/FilledCloseIcon/index.tsx(1 hunks)packages/design-system/components/icons/index.ts(1 hunks)
💤 Files with no reviewable changes (5)
- packages/components/modules/messages/CreateGroup/styled.tsx
- packages/components/modules/messages/CreateGroup/constants.ts
- packages/components/modules/messages/types.ts
- packages/components/modules/messages/GroupDetails/ProfileCard/constants.ts
- packages/components/modules/messages/CreateGroup/ConnectionsList/index.tsx
🚧 Files skipped from review as they are similar to previous changes (35)
- packages/design-system/components/icons/index.ts
- packages/components/modules/messages/MessagesList/MessagesGroup/types.ts
- packages/components/modules/messages/graphql/fragments/RoomTitle.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/styled.tsx
- packages/design-system/components/icons/FilledCloseIcon/index.tsx
- packages/components/modules/messages/shared/GroupChatMembersList/styled.tsx
- packages/components/modules/messages/EditGroup/AddMembersDialog/constants.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/types.ts
- packages/design-system/components/buttons/AvatarButton/types.ts
- packages/design-system/components/buttons/index.ts
- packages/design-system/components/dialogs/ConfirmDialog/types.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/styled.tsx
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/types.ts
- packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/types.ts
- packages/components/modules/messages/shared/GroupChatMembersList/ProfileCard/index.tsx
- packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts
- packages/components/modules/messages/GroupDetails/ProfileCard/index.tsx
- packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/styled.tsx
- packages/design-system/components/buttons/AvatarButton/storybook/stories.tsx
- packages/components/modules/messages/shared/GroupChatMembersList/ProfileCard/types.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/index.tsx
- packages/components/modules/messages/shared/LeaveGroupDialog/constants.ts
- packages/components/modules/messages/shared/GroupChatMembersList/types.ts
- packages/design-system/components/dialogs/ConfirmDialog/index.tsx
- packages/components/modules/messages/ChatRoom/ChatRoomHeader/index.tsx
- packages/components/modules/messages/shared/GroupChatMembersList/ProfilesList/index.tsx
- packages/components/modules/messages/shared/constants.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/index.tsx
- packages/design-system/components/buttons/AvatarButton/index.tsx
- packages/components/modules/messages/EditGroup/types.ts
- packages/design-system/components/buttons/AvatarButton/styled.tsx
- packages/components/modules/messages/shared/LeaveGroupDialog/types.ts
- packages/components/modules/messages/EditGroup/constants.ts
- packages/components/schema.graphql
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/messages/EditGroup/index.tsx
[error] 111-111: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (20)
packages/components/modules/messages/GroupDetails/index.tsx (3)
19-19: LGTM!The import statement is correctly organized and follows consistent formatting.
55-55: LGTM! Clean implementation of admin role checking.The
useCheckIsAdminhook effectively centralizes the admin role checking logic, providing bothisAdminandisSoleAdminstates in a clean, reusable way.
114-122: Verify if the TODO comment is still relevant.The implementation of
isSoleAdminprop suggests that sole admin removal is being handled now. Please verify if the TODO comment on line 114 is still relevant or can be removed.The prop changes look good:
- Safe handling of
removingParticipantIdwith nullish coalescing- Addition of
isSoleAdminprop for handling sole admin casespackages/design-system/components/buttons/AvatarButton/__storybook__/IconButton.mdx (3)
1-4: Potential Naming Inconsistency in Meta and File PlacementThe file is located within the
AvatarButtondirectory, yet its metadata and documentation referenceIconButton. Please confirm whether this is intentional. IfIconButtonis indeed meant to be a separate component, consider moving the file to its corresponding directory or updating the title and content to reflect the correct component name. Additionally, if this documentation was copy-pasted from another source, please update the details to be specific to this component.
26-34: Well-Documented Props SectionThe props are clearly listed with concise type annotations and descriptions. If default values exist or if there are any constraints (such as accepted values or ranges), including that information could further enhance the documentation.
35-42: Useful Notes on Related ComponentsThe "Notes" section effectively ties in related components, such as Button, Tooltip, Menu, and Icons, which provides valuable context. No changes are necessary unless additional related components or usage nuances need to be mentioned in future updates.
packages/components/modules/messages/__shared__/LeaveGroupDialog/index.tsx (3)
6-6: LGTM! Well-structured imports and constants.The addition of typed constants for dialog text copy improves maintainability and type safety.
Also applies to: 11-17
20-29: LGTM! Props provide good flexibility.The new optional props (
customTitle,customContent,isSoleAdmin) with sensible defaults enhance component customization while maintaining backward compatibility.
78-99: LGTM! Clean and flexible dialog rendering.The dialog rendering nicely handles custom content with fallbacks to generated text, following React best practices.
packages/components/modules/messages/__shared__/types.ts (3)
1-2: LGTM! Well-structured imports.The imports are properly typed using generated GraphQL fragment types, which ensures type safety.
15-15: LGTM! Clean interface composition.Good use of TypeScript's interface extension to combine related types.
17-19: LGTM! Well-structured message types.Good use of TypeScript features:
- NonNullable utility type for type safety
- Clear type hierarchy derived from GraphQL fragment
- Proper use of indexed access types
packages/components/modules/messages/utils.ts (1)
8-8: LGTM! Imports are well organized.The new imports for RoomTitleFragment type and CHAT_ROOM_PARTICIPANT_ROLES constants are properly placed and necessary for the functionality.
Also applies to: 10-10
packages/components/modules/messages/__shared__/GroupChatMembersList/index.tsx (2)
59-65: Add validation for member operations.The member addition function still lacks validation for profile data and duplicate members.
45-50: Add debouncing to search functionality.The search still triggers a refetch on every keystroke.
packages/components/modules/messages/CreateGroup/types.ts (1)
9-21: LGTM!The interface changes look good. The switch to ProfilesListProps and addition of MembersList properties is well-structured and type-safe.
packages/components/modules/messages/CreateGroup/index.tsx (2)
3-3: LGTM! Good job on improving code organization.The changes effectively consolidate shared components and types, promoting code reuse and maintainability.
Also applies to: 14-15, 21-29
175-193: LGTM! Good job on improving UI consistency.The use of
GroupChatMembersListcomponent effectively consolidates member management functionality and improves code reuse.packages/components/modules/messages/EditGroup/index.tsx (2)
3-3: LGTM! Good job on improving type safety.The addition of proper fragment types and hooks improves type safety and functionality.
Also applies to: 17-21, 23-28
155-162: LGTM! Good job on improving member management UI.The addition of
AddMembersDialogand use ofGroupChatMembersListeffectively improves the member management experience.Also applies to: 191-210
packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/types.ts
Outdated
Show resolved
Hide resolved
Ronan-Fernandes
force-pushed
the
BA-2114-add-members-to-existing-group
branch
from
c651779 to
0cf060f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/types.ts (1)
9-26: Add JSDoc documentation for the interface.Consider adding JSDoc documentation to describe the purpose and usage of the interface and its properties.
+/** + * Props for the ProfilesList component. + * @property profiles - Array of profile nodes to display + * @property isLoadingNext - Whether more profiles are being loaded + * @property hasNext - Whether there are more profiles to load + * @property loadNext - Function to load more profiles + * @property searchValue - Current search filter value + * @property VirtuosoProps - Props for the Virtuoso list component + * @property NormalListProps - Props for the Box component when not virtualized + * @property isPending - Whether the list is in a pending state + * @property label - Optional label for the list + * @property title - Optional title for the list + * @property renderItem - Function to render each profile item + * @property SearchNotFoundState - Component to show when search has no results + * @property EmptyProfilesListState - Component to show when list is empty + * @property allowAddMember - Whether to show add member option + * @property onAddMemberClick - Callback when add member is clicked + * @property removeTitle - Whether to hide the title + */ export interface ProfilesListProps {packages/components/modules/messages/CreateChatRoomList/index.tsx (1)
132-137: Consider improving image path management and accessibility.While the AvatarButton implementation is clean and follows design system patterns, consider these improvements:
- Move the image path to a constants file to maintain consistency and ease updates
- Enhance the alt text to be more descriptive, e.g., "Create new group chat"
+// In a constants file +export const AVATAR_IMAGES = { + GROUP_CHAT: '/svg/avatar-group-chat.svg' +} as const - <AvatarButton - onClick={onGroupChatCreationButtonClicked} - caption="New Group" - imageString="/svg/avatar-group-chat.svg" - alt="Avatar Group Fallback" - /> + <AvatarButton + onClick={onGroupChatCreationButtonClicked} + caption="New Group" + imageString={AVATAR_IMAGES.GROUP_CHAT} + alt="Create new group chat" + />packages/components/modules/messages/EditGroup/index.tsx (3)
38-76: Consider replacinganytypes with proper type definitions.The code uses
anytype in several places which reduces type safety:
- Line 40:
allProfilesRef: any- Line 44:
ProfileCardProps = {}- Line 73:
(edge: any)Consider defining proper types for these cases to improve type safety and maintainability. For example:
-allProfilesRef: any, +allProfilesRef: PreloadedQuery<AllProfilesQuery>, -ProfileCardProps = {}, +ProfileCardProps: ProfileCardProps = {}, -(edge: any) +(edge: { node?: { profile: ProfileNode | null } | null })
146-151: Add error handling for refetch operation.The refetch operation could potentially fail silently. Consider adding error handling to provide better user feedback.
const handleAddMemberSuccess = () => { startTransition(() => { setOpen(false) - refetch?.({}) + refetch?.({}).catch((error) => { + console.error('Failed to refetch members:', error); + sendToast('Failed to refresh member list', { type: 'error' }); + }); }) }
205-209: Consider reordering spread props to prevent accidental overrides.The spread of
MembersListPropsafter specific props could accidentally override important functionality likeallowAddMemberandonAddMemberClick.MembersListProps={{ + ...MembersListProps, allowAddMember: true, onAddMemberClick: () => setOpen(true), - ...MembersListProps, }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
packages/components/modules/messages/CreateChatRoomList/index.tsx(2 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/styled.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/styled.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/VirtuosoStyles.css(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/constants.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/styled.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts(1 hunks)packages/components/modules/messages/EditGroup/constants.ts(1 hunks)packages/components/modules/messages/EditGroup/index.tsx(4 hunks)packages/components/modules/messages/EditGroup/types.ts(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/types.ts(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/types.ts(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/ProfileCard/types.ts(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/index.tsx(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/types.ts(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/index.tsx(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/types.ts(1 hunks)packages/components/schema.graphql(2 hunks)packages/design-system/components/buttons/AvatarButton/index.tsx(1 hunks)packages/design-system/components/buttons/AvatarButton/types.ts(1 hunks)packages/design-system/components/buttons/index.ts(1 hunks)packages/design-system/components/dialogs/ConfirmDialog/index.tsx(2 hunks)packages/design-system/components/dialogs/ConfirmDialog/types.ts(2 hunks)packages/design-system/components/icons/FilledCloseIcon/index.tsx(1 hunks)packages/design-system/components/icons/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/modules/messages/EditGroup/AddMembersDialog/VirtuosoStyles.css
🚧 Files skipped from review as they are similar to previous changes (26)
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/types.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/types.ts
- packages/design-system/components/icons/index.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/styled.tsx
- packages/design-system/components/buttons/AvatarButton/types.ts
- packages/components/modules/messages/MessagesList/MessagesGroup/types.ts
- packages/design-system/components/buttons/index.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/constants.ts
- packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/types.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/styled.tsx
- packages/components/modules/messages/shared/GroupChatMembersList/ProfileCard/types.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/styled.tsx
- packages/design-system/components/dialogs/ConfirmDialog/types.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts
- packages/design-system/components/buttons/AvatarButton/index.tsx
- packages/components/modules/messages/shared/GroupChatMembersList/ProfilesList/index.tsx
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddedMembersCard/index.tsx
- packages/components/schema.graphql
- packages/components/modules/messages/shared/GroupChatMembersList/types.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx
- packages/components/modules/messages/shared/GroupChatMembersList/index.tsx
- packages/components/modules/messages/EditGroup/constants.ts
- packages/design-system/components/icons/FilledCloseIcon/index.tsx
- packages/design-system/components/dialogs/ConfirmDialog/index.tsx
- packages/components/modules/messages/EditGroup/AddMembersDialog/AddMembersCard/index.tsx
- packages/components/modules/messages/EditGroup/types.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/messages/EditGroup/index.tsx
[error] 111-111: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (3)
packages/components/modules/messages/__shared__/GroupChatMembersList/ProfilesList/types.ts (1)
13-13: Replaceanytypes with more specific types.Using
anyreduces type safety. Consider using more specific types:- loadNext?: LoadMoreFn<any> + loadNext?: LoadMoreFn<ProfileNode> - VirtuosoProps?: Partial<VirtuosoProps<any, any>> + VirtuosoProps?: Partial<VirtuosoProps<ProfileNode, unknown>>Also applies to: 15-15
packages/components/modules/messages/CreateChatRoomList/index.tsx (2)
7-7: LGTM! Import changes are well-organized.The imports are properly structured with clear separation between external and internal dependencies. The move of type definitions to a shared location improves code organization.
Also applies to: 12-12, 19-19, 21-21
104-105: Address the TODO comment regarding height calculation.The current height calculation is hardcoded and brittle. Consider implementing a more robust solution:
- Use CSS custom properties (variables) for header heights
- Implement a dynamic height calculation based on viewport and component measurements
- Consider using ResizeObserver for responsive adjustments
Would you like me to provide an example implementation using one of these approaches?
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (15)
packages/design-system/components/buttons/AvatarButton/__storybook__/stories.tsx (3)
17-17: Replace console.log with action handlers in stories.Using console.log in stories isn't recommended. Instead, use Storybook's action handlers for better debugging and documentation.
- onClick: () => console.log('clicked'), + onClick: action('clicked'),Don't forget to add the import:
import { action } from '@storybook/addon-actions'Also applies to: 23-23, 30-30, 37-37
31-31: Use valid image paths in stories.The hardcoded image path '/path/to/avatar.jpg' might not exist. Consider using a real image path from your assets or a placeholder image service.
- imageString: '/path/to/avatar.jpg', + imageString: 'https://via.placeholder.com/150',Also applies to: 38-38
15-41: Add aria-label to stories for better accessibility.Consider adding aria-label prop to the stories to improve accessibility.
Example for the WithCaption story:
args: { onClick: () => console.log('clicked'), caption: 'Add Member', + 'aria-label': 'Add member button', },packages/design-system/components/buttons/AvatarButton/__storybook__/AvatarButton.mdx (2)
25-28: Enhance props documentation with more details.Consider adding more detailed prop information including:
- Exact TypeScript types
- Required vs optional status
- Default values
- Prop validation rules
Example format:
## Props | Prop | Type | Required | Default | Description | |------|------|----------|---------|-------------| | onClick | () => void | Yes | - | Function called when button is clicked | | imageString | string | No | undefined | URL of the avatar image | | caption | string | No | undefined | Caption text displayed next to avatar | | alt | string | No | "Avatar Button" | Alt text for the avatar image |
39-52: Update example code to use TypeScript.The example code should use TypeScript to better demonstrate proper usage with types.
-```javascript +```typescript import { AvatarButton } from '@baseapp-frontend/design-system' +import { FC } from 'react' -const MyComponent = () => { +const MyComponent: FC = () => { return ( <AvatarButton onClick={() => console.log('clicked')} imageString="/path/to/avatar.jpg" caption="Add Member" + aria-label="Add member button" /> ) }packages/components/modules/messages/EditGroup/AddMemberCard/styled.tsx (1)
12-15: Remove duplicated padding in responsive styles.The padding in the responsive block is redundant as it's identical to the base padding.
[theme.breakpoints.down('sm')]: { maxWidth: '600px', - padding: `${theme.spacing(1.5)} ${theme.spacing(1.5)}`, },packages/components/modules/messages/EditGroup/AddMemberCard/index.tsx (2)
32-32: Remove unnecessary key prop.The
keyprop onMainContaineris unnecessary as it's not part of a list or array of elements.- <MainContainer key={`chat-room-item-${id}`}> + <MainContainer>
22-29: Consider memoizing the event handler.The checkbox change handler is recreated on every render. Consider using
useCallbackto optimize performance.+ import { FC, useCallback } from 'react' - const handleCheckboxChange = (event: React.ChangeEvent<HTMLInputElement>) => { + const handleCheckboxChange = useCallback((event: React.ChangeEvent<HTMLInputElement>) => { const { target } = event if (target.checked) { handleAddMember(profileRef) } else { handleRemoveMember(profileRef) } - } + }, [handleAddMember, handleRemoveMember, profileRef])packages/components/modules/messages/EditGroup/AddMembersMobile/constants.ts (1)
8-11: Consider adding specific validation rules for the title field.While the title field is correctly marked as required, consider adding additional validation rules like minimum/maximum length, character restrictions, etc.
- [CREATE_OR_EDIT_GROUP_FORM_VALUE.title]: z.string(), + [CREATE_OR_EDIT_GROUP_FORM_VALUE.title]: z.string() + .min(1, 'Title is required') + .max(50, 'Title must be less than 50 characters') + .regex(/^[a-zA-Z0-9\s-_]+$/, 'Title can only contain letters, numbers, spaces, hyphens and underscores'),packages/components/modules/messages/EditGroup/AddMembersMobile/index.tsx (2)
91-97: Optimize profile filtering with useMemo dependencies.The profiles memo could be optimized by including participants in dependencies to prevent unnecessary filtering when participants change.
const profiles = useMemo( () => allProfiles?.edges .filter((edge) => edge?.node && edge?.node.id !== profileId) .map((edge) => edge?.node) || [], - [allProfiles, profileId], + [allProfiles?.edges, profileId], )
126-128: Improve accessibility for IconButtons.The IconButtons need more descriptive aria-labels and could benefit from tooltips for better user experience.
- <IconButton onClick={onClose} aria-label="cancel adding member"> + <IconButton + onClick={onClose} + aria-label="Go back to previous screen" + title="Back" + > <Iconify icon="eva:arrow-ios-back-fill" width={24} /> </IconButton> ... <IconButton - aria-label="Add Member" + aria-label={`Add ${participants.length} members to group`} + title="Confirm" disabled={isEditButtonDisabled || isPending} isLoading={isMutationInFlight || isPending} onClick={() => { onSubmit() }} >Also applies to: 133-141
packages/components/modules/messages/EditGroup/AddedMemberCard/index.tsx (2)
32-37: Add error handling for avatar image loading.The avatar component should handle image loading errors gracefully.
<AvatarWithPlaceholder width={64} height={64} src={image?.url} + alt={`${name}'s avatar`} + onError={(e) => { + console.warn(`Failed to load avatar for ${name}:`, e); + }} sx={{ alignSelf: 'center', justifySelf: 'center' }} />
39-45: Ensure consistent width constraints.The Typography component's maxWidth should match the container's maxWidth for consistent layout.
<Typography variant="subtitle2" noWrap sx={{ overflow: 'hidden', textOverflow: 'ellipsis', - maxWidth: '64px' + maxWidth: '120px' // Match container maxWidth }} >packages/components/modules/messages/EditGroup/index.tsx (2)
147-154: Add cleanup for pending transitions.The useTransition hook should be cleaned up to prevent memory leaks.
const [isPending, startTransition] = useTransition() +const [isTransitionCancelled, setIsTransitionCancelled] = useState(false) +useEffect(() => { + return () => setIsTransitionCancelled(true) +}, []) const handleAddMemberSuccess = () => { startTransition(() => { + if (isTransitionCancelled) return setOpen(false) refetch?.({}) }) }
205-224: Add error boundary for GroupChatMembersList.The GroupChatMembersList component should be wrapped in an error boundary to handle potential rendering errors gracefully.
+<ErrorBoundary + fallback={ + <Typography color="error"> + Failed to load members list. Please try again. + </Typography> + } +> <GroupChatMembersList FORM_VALUE={FORM_VALUE} setValue={setValue} watch={watch} currentParticipants={participants} refetch={refetch} membersLoadNext={loadNext} membersHasNext={hasNext} membersIsLoadingNext={isLoadingNext} Searchbar={Searchbar} SearchbarProps={SearchbarProps} ProfileCard={ProfileCard} ProfileCardProps={ProfileCardProps} MembersList={MembersList} MembersListProps={{ allowAddMember: true, onAddMemberClick: () => setOpen(true), ...MembersListProps, }} /> +</ErrorBoundary>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ 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/messages/EditGroup/AddMemberCard/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMemberCard/styled.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMemberCard/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersMobile/constants.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersMobile/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersMobile/styled.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersMobile/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddedMemberCard/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddedMemberCard/styled.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddedMemberCard/types.ts(1 hunks)packages/components/modules/messages/EditGroup/index.tsx(4 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/styled.tsx(1 hunks)packages/design-system/components/buttons/AvatarButton/__storybook__/AvatarButton.mdx(1 hunks)packages/design-system/components/buttons/AvatarButton/__storybook__/stories.tsx(1 hunks)packages/design-system/components/buttons/AvatarButton/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/modules/messages/EditGroup/AddMemberCard/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/components/modules/messages/shared/GroupChatMembersList/styled.tsx
- packages/design-system/components/buttons/AvatarButton/index.tsx
- packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/messages/EditGroup/index.tsx
[error] 114-114: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (5)
packages/components/modules/messages/EditGroup/AddMemberCard/styled.tsx (1)
4-11: LGTM! Well-structured grid layout.The grid layout is well-designed with appropriate spacing and alignment for the avatar, text content, and checkbox.
packages/components/modules/messages/EditGroup/AddMemberCard/index.tsx (1)
14-48: LGTM! Well-structured component with clear responsibilities.The component is well-implemented with:
- Clear prop interface
- Proper data management using Relay
- Clean rendering logic with appropriate Material-UI components
packages/components/modules/messages/EditGroup/AddMembersMobile/types.ts (1)
3-9: LGTM! Props interface is well-defined.The interface provides clear typing for all required props with appropriate optionality.
packages/components/modules/messages/EditGroup/AddMembersMobile/styled.tsx (1)
4-6: LGTM! Styled component follows MUI patterns.The component correctly uses theme spacing for consistent padding.
packages/components/modules/messages/EditGroup/AddedMemberCard/styled.tsx (1)
4-13: Review container dimensions and grid layout.The current layout configuration has potential issues:
- maxWidth of 64px seems too restrictive for a container that includes both an avatar and text.
- Single column layout with 'min-content' might not provide optimal spacing.
Please verify if this layout works well with:
- Longer member names
- Different screen sizes
- Different avatar sizes
Consider these improvements:
export const MainContainer = styled(Box)(({ theme }) => ({ alignItems: 'center', display: 'grid', width: '100%', - maxWidth: '64px', + maxWidth: '120px', // Increased to accommodate content gridTemplateRows: 'auto auto', - gridTemplateColumns: 'min-content', + gridTemplateColumns: '1fr', // Use fractional unit for better content distribution gap: theme.spacing(0.5), margin: theme.spacing(1), }))
packages/components/modules/messages/EditGroup/AddMembersMobile/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/EditGroup/AddMembersMobile/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/EditGroup/AddedMemberCard/types.ts
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/EditGroup/AddedMemberCard/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/components/modules/messages/EditGroup/AddMembersMobile/types.ts (1)
1-1: Consider using absolute imports for better maintainability.Multiple parent directory traversals (
../../../../) can make the codebase harder to maintain. Consider configuring TypeScript path aliases to use absolute imports.Example configuration in
tsconfig.json:{ "compilerOptions": { "baseUrl": ".", "paths": { + "@generated/*": ["__generated__/*"] } } }Then update the import:
-import { ChatRoomsQuery$data } from '../../../../__generated__/ChatRoomsQuery.graphql' +import { ChatRoomsQuery$data } from '@generated/ChatRoomsQuery.graphql'packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts (2)
1-1: Consider using path aliases for deep imports.The deep relative import path could be simplified using TypeScript path aliases, making it more maintainable and less prone to errors during file moves.
Example configuration in
tsconfig.json:{ "compilerOptions": { "baseUrl": ".", "paths": { "@generated/*": ["__generated__/*"] } } }Then the import could be simplified to:
import { ChatRoomsQuery$data } from '@generated/ChatRoomsQuery.graphql'
3-11: Add JSDoc comments and strengthen type safety.The interface would benefit from documentation and stronger typing.
Consider applying these improvements:
+/** Props for the AddMembersDialog component */ export interface AddMembersDialogProps { + /** Reference to all available profiles data */ allProfilesRef: ChatRoomsQuery$data + /** Callback to close the dialog */ onClose: VoidFunction - handleSubmitSuccess: VoidFunction + /** Callback when members are successfully added */ + handleSubmitSuccess: (addedMemberIds: string[]) => void + /** Whether the dialog is currently open */ open: boolean + /** ID of the current user's profile */ profileId: string + /** ID of the chat room to add members to */ roomId?: string + /** Whether an operation is currently in progress */ isPending: boolean }This provides:
- Documentation for better maintainability
- Type safety for the success handler to ensure added member IDs are passed
packages/components/modules/messages/EditGroup/index.tsx (1)
148-154: Add error handling for the transition.While the transition implementation is good, consider adding error boundaries to handle potential failures during refetch operations.
const handleAddMemberSuccess = () => { setOpen(false) startTransition(() => { + try { refetch?.({}) + } catch (error) { + console.error('Failed to refetch members:', error) + sendToast('Failed to refresh member list', { type: 'error' }) + } }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersMobile/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersMobile/types.ts(1 hunks)packages/components/modules/messages/EditGroup/index.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx
- packages/components/modules/messages/EditGroup/AddMembersMobile/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/messages/EditGroup/index.tsx
[error] 114-114: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (5)
packages/components/modules/messages/EditGroup/AddMembersMobile/types.ts (1)
3-10: LGTM! Well-structured interface definition.The interface follows TypeScript best practices with clear property names, appropriate types, and proper optional property marking.
packages/components/modules/messages/EditGroup/index.tsx (4)
3-12: LGTM! Well-structured component customization.The component follows good practices by allowing customization through props while providing sensible defaults.
Also applies to: 40-51
62-79: LGTM! Efficient data management implementation.The use of
usePaginationFragmentanduseMemodemonstrates good performance optimization practices.
114-114: Replace delete operator for better performance.The delete operator can impact performance. Use undefined assignment instead.
-delete dirtyValues.participants +dirtyValues.participants = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 114-114: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
207-226: LGTM! Clean component composition.The GroupChatMembersList implementation demonstrates good component composition with clear prop passing patterns.
Ronan-Fernandes
force-pushed
the
BA-2114-add-members-to-existing-group
branch
from
6d6358a to
d1ec253
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/modules/profiles/graphql/queries/AllProfilesList.ts (1)
6-29: Consider documenting the sorting behavior.The fragment is well-structured, but it would be helpful to document the available sorting options and their impact.
export const fragmentQuery = graphql` + # Supported orderBy values: + # - "name": Sort profiles alphabetically by name (default) + # - "-created": Sort profiles by creation date, newest first fragment AllProfilesListFragment on Query
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/components/__generated__/AllProfilesListFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/AllProfilesListPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ChatRoomsQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (6)
packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersMobile/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersMobile/types.ts(1 hunks)packages/components/modules/messages/EditGroup/index.tsx(4 hunks)packages/components/modules/profiles/graphql/queries/AllProfilesList.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/components/modules/messages/EditGroup/AddMembersMobile/types.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts
- packages/components/modules/messages/EditGroup/AddMembersMobile/index.tsx
- packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/messages/EditGroup/index.tsx
[error] 114-114: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (5)
packages/components/modules/messages/EditGroup/index.tsx (4)
40-54: LGTM! Well-structured props with good customization options.The component allows for flexible customization through prop injection while maintaining sensible defaults.
147-154: Good implementation of data refetching after member addition.The implementation addresses the previous discussion about needing to refetch data after adding new members. The use of
useTransitionhelps manage loading states during the refetch operation.
114-114: Replace delete operator for better performance.The static analysis tool correctly flags the use of the delete operator as a potential performance issue.
-delete dirtyValues.participants +dirtyValues.participants = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 114-114: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
156-166: LGTM! Well-implemented responsive design and member management.The component handles both desktop and mobile views elegantly while maintaining consistent functionality. The member list implementation is flexible and handles loading states appropriately.
Also applies to: 207-226
packages/components/modules/profiles/graphql/queries/AllProfilesList.ts (1)
12-12: Verify the impact of changing the default sort order.The default sorting has been changed from creation date (descending) to alphabetical by name. This could affect the user experience in components using this fragment.
Let's verify the components that might be affected by this change:
Additionally, please confirm:
- Is this change intentional and aligned with the requirements?
- Have the affected components been tested with the new default sorting?
Ronan-Fernandes
force-pushed
the
BA-2114-add-members-to-existing-group
branch
from
d1ec253 to
b684185
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/components/modules/messages/EditGroup/index.tsx (1)
114-114: 🛠️ Refactor suggestionReplace delete operator for better performance.
Use undefined assignment instead of the delete operator to improve performance.
-delete dirtyValues.participants +dirtyValues.participants = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 114-114: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🧹 Nitpick comments (4)
packages/components/modules/messages/__shared__/GroupChatMembersList/types.ts (2)
27-31: Consider using more specific prop types for container components.The container components (
ProfilesContainer,SearchbarContainer) are typed with genericBoxProps. Consider creating specific interfaces for these containers to better document their intended usage and constraints.
14-37: Add JSDoc documentation for the interface.This interface has many props with complex types. Adding JSDoc documentation would help other developers understand the purpose and requirements of each prop.
Example documentation:
/** * Props for the GroupChatMembersList component. * @property {Record<keyof CreateOrEditGroup, keyof CreateOrEditGroup>} FORM_VALUE - Form field mappings * @property {UseFormSetValue<CreateOrEditGroup>} setValue - Function to set form values * ... */ export interface GroupChatMembersListProps extends PropsWithChildren {packages/components/modules/messages/CreateGroup/index.tsx (2)
120-131: Optimize useMemo dependencies array.The
useMemodependencies array includes the entireparticipantsarray which could cause unnecessary recalculations. Consider using a derived value likeparticipants.lengthor participant IDs.const profiles = useMemo( () => allProfiles?.edges .filter( (edge) => edge?.node && edge?.node.id !== currentProfile?.id && !participants.some((member: ProfileNode) => member?.id === edge?.node?.id), ) .map((edge) => edge?.node) || [], - [allProfiles, participants, currentProfile?.id], + [allProfiles, currentProfile?.id, participants.map(p => p.id).join(',')], )
105-115: Enhance error handling in mutation response.The mutation error handling could be more specific about what went wrong. Consider providing more detailed error messages based on the error type.
onCompleted: (response) => { const errors = response?.chatRoomCreate?.errors if (errors) { - sendToast('Something went wrong', { type: 'error' }) + const errorMessage = errors.map(e => e.message).join(', ') + sendToast(`Failed to create group: ${errorMessage}`, { type: 'error' }) setFormRelayErrors(formReturn, errors) } else { setChatRoom({ id: response?.chatRoomCreate?.room?.node?.id }) onValidSubmission() } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/components/__generated__/AllProfilesListFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/AllProfilesListPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ChatRoomsQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (10)
packages/components/modules/messages/CreateGroup/index.tsx(5 hunks)packages/components/modules/messages/CreateGroup/styled.tsx(0 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersMobile/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersMobile/types.ts(1 hunks)packages/components/modules/messages/EditGroup/index.tsx(4 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/index.tsx(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/types.ts(1 hunks)packages/components/modules/profiles/graphql/queries/AllProfilesList.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/components/modules/messages/CreateGroup/styled.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/components/modules/profiles/graphql/queries/AllProfilesList.ts
- packages/components/modules/messages/EditGroup/AddMembersMobile/types.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts
- packages/components/modules/messages/EditGroup/AddMembersMobile/index.tsx
- packages/components/modules/messages/shared/GroupChatMembersList/index.tsx
- packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/messages/EditGroup/index.tsx
[error] 114-114: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/components/modules/messages/EditGroup/index.tsx (4)
3-28: LGTM! Well-organized imports.The new imports are logically grouped and necessary for the added member management functionality.
40-58: LGTM! Well-structured props with defaults.The component follows good practices by:
- Providing default components while allowing customization
- Using proper TypeScript types
- Including appropriate default values for props
62-79: Consider optimizing participant updates.The current implementation uses refetch to update the participants list. As discussed in previous reviews, consider implementing automatic updates through ChatRoomUpdates subscription for better real-time synchronization.
156-227: LGTM! Well-implemented responsive UI.The implementation:
- Handles mobile view appropriately
- Provides good customization options through props
- Manages loading states properly with useTransition
packages/components/modules/messages/CreateGroup/index.tsx (1)
57-61: Verify minimum participants validation.Ensure that the form validation schema (
DEFAULT_FORM_VALIDATION) includes checks for minimum number of participants in a group chat.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for validation schema definition rg -A 10 "DEFAULT_CREATE_OR_EDIT_GROUP_FORM_VALIDATION" .Length of output: 5279
Minimum participants validation confirmed
The shared validation schema (DEFAULT_CREATE_OR_EDIT_GROUP_FORM_VALIDATION) includes a minimum check on the participants field via z.array(z.any()).min(1, { message: 'Please select at least one member' }). This ensures that at least one participant is required in a group chat.
packages/components/modules/messages/__shared__/GroupChatMembersList/types.ts
Outdated
Show resolved
Hide resolved
Ronan-Fernandes
force-pushed
the
BA-2114-add-members-to-existing-group
branch
from
b684185 to
b1183e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/components/modules/messages/CreateGroup/index.tsx (1)
37-42: Add error handling for loadNext operation.The loadNext operation from useAllProfilesList should handle potential errors.
const { data: { allProfiles }, loadNext, isLoadingNext, hasNext, refetch, -} = useAllProfilesList(allProfilesRef) +} = useAllProfilesList(allProfilesRef, { + onError: (error) => { + console.error('Failed to load more profiles:', error) + sendToast('Failed to load more profiles', { type: 'error' }) + }, +})
🧹 Nitpick comments (4)
packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts (1)
6-16: Consider making core components required and add documentation.The interface structure has a few areas for improvement:
GroupChatMembersListand its props appear to be core functionality but are marked optional- Complex props would benefit from documentation
- The component and its props could be better organized
Consider applying these improvements:
export interface AddMembersDialogProps { + /** Reference to all chat room profiles data */ allProfilesRef: ChatRoomsQuery$data + /** Callback when dialog is closed */ - onClose: VoidFunction + onClose: () => void + /** Callback when members are successfully added */ - handleSubmitSuccess: VoidFunction + handleSubmitSuccess: () => void open: boolean profileId: string roomId?: string isPending: boolean - GroupChatMembersList?: FC<GroupChatMembersListProps> - GroupChatMembersListProps?: Partial<GroupChatMembersListProps> + /** Component for rendering the members list */ + GroupChatMembersList: FC<GroupChatMembersListProps> + /** Props to customize the members list behavior */ + groupChatMembersListProps?: Partial<GroupChatMembersListProps> }packages/components/modules/messages/CreateGroup/types.ts (1)
8-9: Consider using a more specific type for GroupChatMembersListProps.Instead of using
Partial<GroupChatMembersListProps>, consider creating a subset type that only includes the props that are actually customizable in this context.- GroupChatMembersListProps?: Partial<GroupChatMembersListProps> + GroupChatMembersListProps?: Pick<GroupChatMembersListProps, 'ProfileCardProps' | 'SearchbarProps' | 'MembersListProps'>packages/components/modules/messages/CreateGroup/index.tsx (1)
69-69: Add type safety to participants mapping.The mapping of participants to IDs could be more type-safe.
- const participantsIds = (participants || []).map((member: ProfileNode) => member?.id) + const participantsIds = (participants || []) + .filter((member): member is ProfileNode => member !== null && member !== undefined) + .map((member) => member.id)packages/components/modules/messages/EditGroup/index.tsx (1)
66-72: Add type safety to participants mapping.The mapping of participants could be more type-safe by adding proper type guards.
const participants = useMemo( () => - membersList?.participants?.edges?.map( - (edge: any) => edge?.node?.profile && edge.node.profile, - ) as ProfileNode[], + membersList?.participants?.edges + ?.filter((edge): edge is NonNullable<typeof edge> => edge !== null) + .map((edge) => edge.node?.profile) + .filter((profile): profile is ProfileNode => profile !== null) ?? [], [membersList], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/components/__generated__/AllProfilesListFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/AllProfilesListPaginationQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ChatRoomsQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (12)
packages/components/modules/messages/CreateGroup/index.tsx(4 hunks)packages/components/modules/messages/CreateGroup/styled.tsx(0 hunks)packages/components/modules/messages/CreateGroup/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersMobile/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersMobile/types.ts(1 hunks)packages/components/modules/messages/EditGroup/index.tsx(4 hunks)packages/components/modules/messages/EditGroup/types.ts(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/index.tsx(1 hunks)packages/components/modules/messages/__shared__/GroupChatMembersList/types.ts(1 hunks)packages/components/modules/profiles/graphql/queries/AllProfilesList.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/components/modules/messages/CreateGroup/styled.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/components/modules/messages/EditGroup/AddMembersMobile/types.ts
- packages/components/modules/profiles/graphql/queries/AllProfilesList.ts
- packages/components/modules/messages/shared/GroupChatMembersList/types.ts
- packages/components/modules/messages/EditGroup/AddMembersMobile/index.tsx
- packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx
- packages/components/modules/messages/EditGroup/types.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/messages/EditGroup/index.tsx
[error] 107-107: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts (2)
1-4: LGTM! Well-organized imports.The imports are logically organized and properly typed with GraphQL generated types.
14-15: Verify the component usage pattern.Let's check how these optional components are being used to ensure the suggested changes won't cause breaking changes.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for AddMembersDialog usage patterns echo "Searching for AddMembersDialog usage patterns..." rg -A 5 "GroupChatMembersList.*=" --type typescriptLength of output: 195
Let's re-run the search using file type flags for TypeScript files (.ts and .tsx):
🏁 Script executed:
#!/bin/bash echo "Searching for GroupChatMembersList usage in .ts files..." rg -A 5 "GroupChatMembersList" -t ts echo "Searching for GroupChatMembersList usage in .tsx files..." rg -A 5 "GroupChatMembersList" -t tsxLength of output: 17978
Component usage verified.
The search results show that the optional GroupChatMembersList and its corresponding props are used consistently across multiple modules (e.g., CreateGroup, EditGroup, AddMembersDialog, AddMembersMobile). All components either receive these optional values or default to using the DefaultGroupChatMembersList as provided in the shared module. There’s no evidence of any breaks in implementation stemming from this pattern.packages/components/modules/messages/__shared__/GroupChatMembersList/index.tsx (2)
63-69: Add validation for member operations.The member addition/removal functions don't validate the profile data or handle potential errors.
49-54: Add debouncing to search functionality.The search triggers a refetch on every keystroke, which could lead to unnecessary API calls.
packages/components/modules/messages/EditGroup/index.tsx (1)
107-107: Replace delete operator for better performance.The static analysis tool correctly flags the use of the delete operator as a potential performance issue.
🧰 Tools
🪛 Biome (1.9.4)
[error] 107-107: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/components/modules/messages/__shared__/GroupChatMembersList/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/components/modules/messages/EditGroup/AddMemberCard/index.tsx (1)
65-65: Add accessibility attributes to the checkbox.The checkbox lacks proper accessibility attributes for screen readers.
- {!isExistingMember && <Checkbox checked={isBeingAdded} onChange={handleCheckboxChange} />} + {!isExistingMember && ( + <Checkbox + checked={isBeingAdded} + onChange={handleCheckboxChange} + aria-label={`${isBeingAdded ? 'Remove' : 'Add'} ${name} ${isBeingAdded ? 'from' : 'to'} group`} + /> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/components/modules/messages/EditGroup/AddMemberCard/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMemberCard/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts(1 hunks)packages/components/modules/messages/EditGroup/AddMembersMobile/index.tsx(1 hunks)packages/components/modules/messages/EditGroup/AddMembersMobile/types.ts(1 hunks)packages/components/modules/messages/EditGroup/index.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/components/modules/messages/EditGroup/AddMembersMobile/types.ts
- packages/components/modules/messages/EditGroup/AddMembersDialog/index.tsx
- packages/components/modules/messages/EditGroup/AddMembersMobile/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/messages/EditGroup/index.tsx
[error] 107-107: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (3)
packages/components/modules/messages/EditGroup/AddMembersDialog/types.ts (1)
7-18: LGTM! Well-structured interface with clear prop types.The interface is comprehensive and type-safe, covering all necessary props for the dialog component.
packages/components/modules/messages/EditGroup/index.tsx (2)
107-107: Replace delete operator for better performance.The static analysis tool correctly flags the use of the delete operator as a potential performance issue.
🧰 Tools
🪛 Biome (1.9.4)
[error] 107-107: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
144-146: Add error handling for refetch operation.The refetch operation should handle potential errors.
packages/components/modules/messages/EditGroup/AddMemberCard/types.ts
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/EditGroup/AddMemberCard/index.tsx
Outdated
Show resolved
Hide resolved
Ronan-Fernandes
force-pushed
the
BA-2114-add-members-to-existing-group
branch
from
355cf39 to
b41c90a
Compare
Ronan-Fernandes
force-pushed
the
BA-2114-add-members-to-existing-group
branch
2 times, most recently
from
1111e39 to
8a60c8d
Compare
Ronan-Fernandes
changed the base branch from
master
to
BA-2259-improve-dev-mode
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (34)
packages/components/modules/messages/web/__shared__/types.ts (1)
3-4: Remove unnecessary empty line.There's an extra empty line that affects readability.
import { MessagesListFragment$data } from "../../../../__generated__/MessagesListFragment.graphql" -packages/design-system/components/web/icons/ErrorIcon/index.tsx (1)
7-38: Consider using more semantic IDs for SVG elements.While the SVG implementation is solid, the IDs could be more descriptive to improve maintainability:
- "Group" → "ErrorIconWrapper"
- "Group_2" → "ErrorIconPaths"
- "Path" → "VerticalLine"
- "Path_2" → "Dot"
- "Path_3" → "Octagon"
- <g id="Group"> - <g id="Group_2"> + <g id="ErrorIconWrapper"> + <g id="ErrorIconPaths"> <path - id="Path" + id="VerticalLine" d="M12 12V7.5" stroke="currentColor" strokeWidth="1.5" strokeLinecap="round" strokeLinejoin="round" /> <path - id="Path_2" + id="Dot" d="M11.999 15.5C11.861 15.5 11.749 15.612 11.75 15.75C11.75 15.888 11.862 16 12 16C12.138 16 12.25 15.888 12.25 15.75C12.25 15.612 12.138 15.5 11.999 15.5" stroke="currentColor" strokeWidth="1.5" strokeLinecap="round" strokeLinejoin="round" /> <path - id="Path_3" + id="Octagon" fillRule="evenodd"packages/design-system/components/web/buttons/AvatarButton/__storybook__/AvatarButton.mdx (3)
9-10: Add accessibility information to the component description.Consider adding details about keyboard navigation, ARIA attributes, and screen reader support to help developers create accessible implementations.
- **Purpose**: The `AvatarButton` component provides a clickable button that displays an avatar and an optional caption. - - **Expected Behavior**: The `AvatarButton` renders as a button containing an avatar image and an optional caption. It responds to hover and click states with visual feedback. + - **Expected Behavior**: The `AvatarButton` renders as a button containing an avatar image and an optional caption. It responds to hover and click states with visual feedback. + - **Accessibility**: The component is keyboard navigable, uses appropriate ARIA attributes, and provides screen reader support through the alt text.
32-35: Add links to related component documentation.Consider adding links to the Storybook documentation of each related component to help developers find more information easily.
- **Related Components**: - - `Button`: Standard button component for text-based actions - - `Avatar`: Component for displaying user avatars - - `Tooltip`: Can be used to provide additional context for the `AvatarButton` + - [`Button`](/story/baseapp-frontend-designsystem-buttons-button--docs): Standard button component for text-based actions + - [`Avatar`](/story/baseapp-frontend-designsystem-avatar--docs): Component for displaying user avatars + - [`Tooltip`](/story/baseapp-frontend-designsystem-tooltip--docs): Can be used to provide additional context for the `AvatarButton`
39-51: Enhance example with TypeScript and error handling.Consider adding TypeScript types and demonstrating error handling for image loading failures.
```javascript -import { AvatarButton } from '@baseapp-frontend/design-system' +import { AvatarButton } from '@baseapp-frontend/design-system' +import { useState } from 'react' + +type Props = { + onMemberAdd: () => void +} -const MyComponent = () => { +const MyComponent: React.FC<Props> = ({ onMemberAdd }) => { + const [imageError, setImageError] = useState(false) + return ( <AvatarButton - onClick={() => console.log('clicked')} + onClick={onMemberAdd} imageString="/path/to/avatar.jpg" caption="Add Member" + onImageError={() => setImageError(true)} + fallbackIcon={imageError ? 'person' : undefined} /> ) }packages/design-system/providers/web/SnackbarProvider/ProgressBar/index.tsx (2)
6-10: Consider memoizing the component for better performance.While the implementation is clean, consider wrapping the component with React.memo since it's a presentational component that depends only on its props.
-const ProgressAnimation: FC<ProgressAnimationProps> = ({ animationTime, severity }) => ( +const ProgressAnimation: FC<ProgressAnimationProps> = React.memo(({ animationTime, severity }) => ( <ProgressContainer severity={severity}> <ProgressBarContainer {...{ severity, animationTime }} /> </ProgressContainer> -) +))
8-8: Consider explicit prop passing instead of spreading.For better code clarity and type safety, consider passing props explicitly instead of using object spread.
- <ProgressBarContainer {...{ severity, animationTime }} /> + <ProgressBarContainer severity={severity} animationTime={animationTime} />packages/design-system/providers/web/SnackbarProvider/constants.tsx (2)
4-4: Consider increasing HIDE_DURATION for better accessibility.3000ms might be too short for users to read longer messages. Consider increasing it to 5000ms or making it configurable based on message length.
-export const HIDE_DURATION = 3000 +export const HIDE_DURATION = 5000
5-10: Maintain consistent color handling across icons.The error icon uses
error.mainwhile info usestext.primary. Other icons don't specify colors. Consider standardizing color handling:export const OUTLINED_ALERT_ICONS = { error: <ErrorIcon sx={{ color: 'error.main', width: '24px', height: '24px' }} />, - info: <Iconify icon="eva:info-outline" width={24} sx={{ color: 'text.primary' }} />, - success: <Iconify icon="eva:checkmark-circle-2-outline" width={24} />, - warning: <Iconify icon="eva:alert-triangle-outline" width={24} />, + info: <Iconify icon="eva:info-outline" width={24} sx={{ color: 'info.main' }} />, + success: <Iconify icon="eva:checkmark-circle-2-outline" width={24} sx={{ color: 'success.main' }} />, + warning: <Iconify icon="eva:alert-triangle-outline" width={24} sx={{ color: 'warning.main' }} />, }packages/design-system/providers/web/SnackbarProvider/styled.tsx (1)
4-22: LGTM! Consider adding padding for better content spacing.The styled component is well-structured with responsive design and proper theme integration. The breakpoints are appropriately set for different screen sizes.
Consider adding padding for better content spacing:
export const SnackbarContentContainer = styled(Box)(({ theme }) => ({ display: 'flex', minWidth: '360px', flexDirection: 'column', justifyContent: 'center', alignItems: 'flex-start', borderRadius: '8px', border: `1px solid ${theme.palette.grey[400]}`, backgroundColor: theme.palette.common.white, boxShadow: theme.customShadows.z8, overflow: 'hidden', maxWidth: '60%', + padding: theme.spacing(2), [theme.breakpoints.down('md')]: { maxWidth: '80%', }, [theme.breakpoints.down('sm')]: { maxWidth: '100%', }, }))packages/design-system/providers/web/SnackbarProvider/ProgressBar/styled.tsx (2)
6-16: LGTM! Consider using theme spacing for height.The
ProgressContainercomponent is well-structured with proper theme integration and severity-based styling.Consider using theme spacing for height to maintain consistency:
export const ProgressContainer = styled(Box)<AlertContainerProps>(({ theme, severity }) => ({ display: 'flex', flexDirection: 'column', alignItems: 'flex-start', width: '100%', - height: '4px', + height: theme.spacing(0.5), backgroundColor: severity === 'info' ? alpha(theme.palette.grey[500], 0.24) : alpha(theme.palette[severity].main, 0.24), }))
18-34: Consider using keyframes helper for better maintainability.The animation is well-implemented, but using Material-UI's keyframes helper would improve maintainability.
Consider using the keyframes helper:
+import { keyframes } from '@mui/material' + +const increaseWidth = keyframes` + from { + width: 0; + } + to { + width: 100%; + } +` + export const ProgressBarContainer = styled(Box)<ProgressBarContainerProps>( ({ theme, animationTime, severity }) => ({ borderRadius: '50px', - animation: `increase-width ${animationTime}ms linear forwards`, - '@keyframes increase-width': { - from: { - width: 0, - }, - to: { - width: '100%', - }, - }, + animation: `${increaseWidth} ${animationTime}ms linear forwards`, height: '100%', backgroundColor: severity === 'info' ? theme.palette.text.primary : theme.palette[severity].main, }), )packages/utils/functions/form/setFormApiErrors/index.ts (1)
14-23: Improve error handling with more specific error types.The error handling is improved with the addition of JSON parsing, but it could benefit from more specific error types.
Consider using a type guard and custom error type:
+type ApiError = { + response?: { + data?: Record<string, unknown> + } + message?: string +} + +function isApiError(err: unknown): err is ApiError { + return typeof err === 'object' && err !== null +} + -export const setFormApiErrors = < +export const setFormApiErrors = < TFieldValues extends FieldValues = FieldValues, TContext = any, TTransformedValues extends FieldValues | undefined = undefined, >( form: UseFormReturn<TFieldValues, TContext, TTransformedValues>, - err: any, + err: unknown, ) => { let errorObject + if (!isApiError(err)) return + if (err.response?.data && typeof err.response.data === 'object') { errorObject = err.response?.data } else if (err.message) {packages/design-system/providers/web/SnackbarProvider/index.tsx (1)
39-55: Consider simplifying the timeout logic.The timeout management logic could be more concise by combining the cleanup logic.
useEffect(() => { - if (shouldShowProgress && open && !timeoutID.current) { + if (shouldShowProgress && open) { timeoutID.current = setTimeout(() => { timeoutID.current = null handleClose() }, HIDE_DURATION) + + return () => { + if (timeoutID.current) { + clearTimeout(timeoutID.current) + timeoutID.current = null + } + } } - return () => { - if (timeoutID.current) { - clearTimeout(timeoutID.current) - timeoutID.current = null - } - } }, [open, shouldShowProgress])packages/design-system/providers/web/SnackbarProvider/__storybook__/stories.tsx (1)
40-84: Consider reducing story boilerplate using story composition.The stories with progress bar share similar structure and could be simplified using Storybook's story composition.
+const Template: Story = { + render: (args) => <SnackbarWrapper {...args} />, +} export const InfoWithProgress: Story = { - render: (args) => <SnackbarWrapper {...args} />, + ...Template, args: { message: 'This is an info message with progress bar', shouldShowProgress: true, type: 'info', }, }packages/design-system/providers/web/SnackbarProvider/__storybook__/Snackbar.mdx (2)
10-10: Fix grammar in the documentation.Add missing comma for better readability.
-The messages disappear automatically after a fixed amount of time, but can also be dismissed before by clicking on a close icon. The component can be used with or without a bar indicating the remaining time before the message is dismissed. If used without a bar, the time until automatic dismissal is restarted after the user interacts with the message, if used with bar the message is always dismissed after the timeout, no matter whether the user interacted with it or not. +The messages disappear automatically after a fixed amount of time, but can also be dismissed before by clicking on a close icon. The component can be used with or without a bar indicating the remaining time before the message is dismissed. If used without a bar, the time until automatic dismissal is restarted after the user interacts with the message, if used with a bar, the message is always dismissed after the timeout, no matter whether the user interacted with it or not.🧰 Tools
🪛 LanguageTool
[uncategorized] ~10-~10: A comma might be missing here.
Context: ...nteracts with the message, if used with bar the message is always dismissed after t...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
15-15: Remove unnecessary comma in the documentation.Remove the comma before "if" for correct grammar.
-The default for showing the progress bar, indicating the time left before the snackbar disappears. Will be overwritten by the 'shouldShowProgress' option of sendToast, if it is set. +The default for showing the progress bar, indicating the time left before the snackbar disappears. Will be overwritten by the 'shouldShowProgress' option of sendToast if it is set.🧰 Tools
🪛 LanguageTool
[typographical] ~15-~15: Usually, there’s no comma before “if”.
Context: ...'shouldShowProgress' option of sendToast, if it is set. - ...other: All other pr...(IF_NO_COMMA)
packages/utils/CHANGELOG.md (2)
3-7: Consider clarifying the package-specific changes.The current changelog entry appears to duplicate the design-system changes. Since this is the utils package, consider focusing on any utility functions or hooks that were added/modified to support the snackbar component.
-## 3.1.5 - -### Patch Changes - -- Implement a snackbar component with a 'progress bar' indicating the remaining time before it automatically disappears +## 3.1.5 + +### Patch Changes + +- Add utility functions and hooks to support the snackbar component with progress bar functionality
9-13: Enhance the changelog entry with more details.The current entry could be more descriptive about the types of Error objects handled and any specific improvements made.
-## 3.1.4 - -### Patch Changes - -- setFormApiErrors also handles Error objects +## 3.1.4 + +### Patch Changes + +- Enhanced setFormApiErrors to handle Error objects in addition to validation messages, improving error handling capabilitiespackages/components/.scripts/build.mjs (1)
6-21: Build sequence and early steps
TherunBuildfunction logs each step clearly. Awaiting each sub-process ensures orderly execution.Consider whether certain tasks (like Relay compilation or Babel transpilation) could be made optional in more advanced configurations.
packages/components/modules/messages/web/ChatRoom/index.tsx (1)
26-26: TODO: Consider implementing query preloading.The TODO comment indicates that query preloading should be implemented instead of using lazy loading.
Would you like me to help implement query preloading to improve performance? I can provide a solution or create an issue to track this task.
packages/design-system/.scripts/build.mjs (1)
9-61: Consider enhancing error handling and process management.A few suggestions to improve the build script:
- Consider using a local copy of currentBuildProcesses to avoid direct mutation
- Add specific error types for different build stages
- Consider adding timeouts for build processes
Here's a suggested improvement for process management:
export const runBuild = async (currentBuildProcesses = []) => { + const processes = [...currentBuildProcesses] // ... existing code ... - currentBuildProcesses?.push(tsupProc) + processes.push(tsupProc) // ... existing code ... } finally { - currentBuildProcesses = [] + processes.length = 0 + currentBuildProcesses.length = 0 } }packages/design-system/.scripts/dev-command.mjs (2)
70-85: Enhance the ignored files check with optional chaining.The static analysis tool correctly suggests using optional chaining for the stats check.
- if (stats && stats.isFile()) { + if (stats?.isFile()) {🧰 Tools
🪛 Biome (1.9.4)
[error] 78-78: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
18-21: Consider extracting magic numbers and improving state management.
- Extract the timeout value as a named constant
- Consider encapsulating the state variables in a class or object
+const BUILD_DEBOUNCE_MS = 2000 + let isBuilding = false let needsRebuild = false let buildTimeout = null let currentBuildProcesses = [] // ... rest of the code ... watcher.on('all', (event, changedPath) => { const relativePath = path.relative(rootDir, changedPath).replace(/\\/g, '/') console.log(`${commandTag} Detected event "${event}" on: ${relativePath}`) if (buildTimeout) clearTimeout(buildTimeout) - buildTimeout = setTimeout(runWatchBuild, 2000) + buildTimeout = setTimeout(runWatchBuild, BUILD_DEBOUNCE_MS) })Also applies to: 88-93
.scripts/command-utils.mjs (1)
76-96: Add timeout handling towaitForReadyFile.The file watcher should have a timeout to prevent indefinite waiting.
Apply this diff to add timeout handling:
export const waitForReadyFile = async ({ readyFilePath, commandTag }) => { console.log( chalk.yellow(`${commandTag} Waiting for other packages to start...`) ) try { await fs.access(readyFilePath) return } catch (err) {} return new Promise((resolve, reject) => { const watcher = chokidar.watch(readyFilePath, { ignoreInitial: true }) + const timeout = setTimeout(() => { + watcher.close() + reject(new Error('Timeout waiting for ready file')) + }, 60000) // 1 minute timeout watcher.on("add", () => { watcher.close() + clearTimeout(timeout) resolve() }) watcher.on("error", (err) => { watcher.close() + clearTimeout(timeout) reject(err) }) }) }packages/components/.scripts/dev-command.mjs (2)
75-90: Use optional chaining for stats check.The static analysis tool correctly suggests using optional chaining for better code readability.
Apply this diff to use optional chaining:
- if (stats && stats.isFile()) { + if (stats?.isFile()) {🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
65-65: Extract magic numbers into named constants.The watch regex and build timeout delay should be extracted into named constants at the top of the file for better maintainability.
Apply this diff to extract constants:
+const WATCH_PATH_REGEX = /^modules\/(?:.*\/)?(common|web|native)\/.*$/ +const BUILD_TIMEOUT_MS = 2000 + -const watchRegex = /^modules\/(?:.*\/)?(common|web|native)\/.*$/ +const watchRegex = WATCH_PATH_REGEX // ... later in the file ... if (buildTimeout) clearTimeout(buildTimeout) - buildTimeout = setTimeout(runWatchBuild, 2000) + buildTimeout = setTimeout(runWatchBuild, BUILD_TIMEOUT_MS)Also applies to: 93-98
packages/components/modules/messages/web/__shared__/LeaveGroupDialog/index.tsx (2)
33-49: Simplify dialog text copy logic using early returns.The nested if statements can be simplified for better readability.
Apply this diff to use early returns:
const getLeaveGroupDialogTextCopy = ( type: ValueOf<typeof LEAVE_GROUP_DIALOG_TEXT_COPY_TYPE_KEYS>, ) => { - if (profileId === removingParticipantId) { - if (isSoleAdmin) { - return LEAVE_GROUP_DIALOG_TEXT_COPY[LEAVE_GROUP_DIALOG_TEXT_COPY_ACTION_KEYS.IS_LEAVING][ - LEAVE_GROUP_DIALOG_TEXT_COPY_ROLE_KEYS.ADMIN - ][type] - } - return LEAVE_GROUP_DIALOG_TEXT_COPY[LEAVE_GROUP_DIALOG_TEXT_COPY_ACTION_KEYS.IS_LEAVING][ - LEAVE_GROUP_DIALOG_TEXT_COPY_ROLE_KEYS.MEMBER - ][type] + if (profileId !== removingParticipantId) { + return LEAVE_GROUP_DIALOG_TEXT_COPY[LEAVE_GROUP_DIALOG_TEXT_COPY_ACTION_KEYS.IS_REMOVING][ + LEAVE_GROUP_DIALOG_TEXT_COPY_ROLE_KEYS.ADMIN + ][type] } - return LEAVE_GROUP_DIALOG_TEXT_COPY[LEAVE_GROUP_DIALOG_TEXT_COPY_ACTION_KEYS.IS_REMOVING][ - LEAVE_GROUP_DIALOG_TEXT_COPY_ROLE_KEYS.ADMIN + + if (isSoleAdmin) { + return LEAVE_GROUP_DIALOG_TEXT_COPY[LEAVE_GROUP_DIALOG_TEXT_COPY_ACTION_KEYS.IS_LEAVING][ + LEAVE_GROUP_DIALOG_TEXT_COPY_ROLE_KEYS.ADMIN + ][type] + } + + return LEAVE_GROUP_DIALOG_TEXT_COPY[LEAVE_GROUP_DIALOG_TEXT_COPY_ACTION_KEYS.IS_LEAVING][ + LEAVE_GROUP_DIALOG_TEXT_COPY_ROLE_KEYS.MEMBER ][type] }
68-68: Enhance success message with participant details.The success message could be more descriptive by including the participant's name or role.
Apply this diff to enhance the success message:
- sendToast('Member was successfully removed') + sendToast('Group member was successfully removed from the chat')packages/components/modules/messages/web/CreateGroup/index.tsx (1)
69-69: Improve type safety for participants mapping.The type assertion for
memberasProfileNodein the map function could be replaced with proper type definitions.- const participantsIds = (participants || []).map((member: ProfileNode) => member?.id) + const participantsIds = (participants || []).map((member) => member?.id)packages/components/modules/messages/web/GroupDetails/index.tsx (1)
119-119: Address the TODO comment regarding sole admin removal.The comment indicates that sole admin removal functionality needs to be implemented.
Would you like me to help implement the sole admin removal functionality or create an issue to track this task?
packages/components/modules/messages/web/EditGroup/index.tsx (3)
106-106: Optimize performance by avoiding the delete operator.The delete operator can impact performance. Consider using undefined assignment instead.
- delete dirtyValues.participants + dirtyValues.participants = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 106-106: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
67-69: Improve type safety for edge mapping.The type assertion and any type usage could be replaced with proper type definitions.
- membersList?.participants?.edges?.map( - (edge: any) => edge?.node?.profile && edge.node.profile, - ) as ProfileNode[] + membersList?.participants?.edges?.map( + (edge) => edge?.node?.profile + ).filter((profile): profile is ProfileNode => !!profile)
92-92: Add proper type for form data.The form submission handler uses
anytype for the data parameter.- const onSubmit = handleSubmit((data: any) => { + const onSubmit = handleSubmit((data: typeof DEFAULT_FORM_VALUES) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (107)
.scripts/command-utils.mjs(1 hunks)README.md(1 hunks)package.json(1 hunks)packages/authentication/CHANGELOG.md(1 hunks)packages/authentication/package.json(1 hunks)packages/components/.scripts/build-command.mjs(1 hunks)packages/components/.scripts/build.mjs(1 hunks)packages/components/.scripts/dev-command.mjs(1 hunks)packages/components/.storybook/main.ts(1 hunks)packages/components/CHANGELOG.md(1 hunks)packages/components/README.md(1 hunks)packages/components/modules/comments/web/CommentItem/index.tsx(1 hunks)packages/components/modules/messages/common/constants.ts(1 hunks)packages/components/modules/messages/common/graphql/fragments/RoomTitle.ts(1 hunks)packages/components/modules/messages/common/graphql/mutations/ArchiveChatRoom.ts(3 hunks)packages/components/modules/messages/common/graphql/queries/ChatRoomQuery.ts(1 hunks)packages/components/modules/messages/common/utils.ts(3 hunks)packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/ChatRoomOptions/index.tsx(1 hunks)packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/ChatRoomOptions/types.ts(1 hunks)packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsx(5 hunks)packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/types.ts(1 hunks)packages/components/modules/messages/web/ChatRoom/index.tsx(1 hunks)packages/components/modules/messages/web/ChatRoomsList/ChatRoomItem/index.tsx(1 hunks)packages/components/modules/messages/web/CreateChatRoomList/index.tsx(2 hunks)packages/components/modules/messages/web/CreateGroup/ConnectionsList/index.tsx(0 hunks)packages/components/modules/messages/web/CreateGroup/ConnectionsList/types.ts(0 hunks)packages/components/modules/messages/web/CreateGroup/constants.ts(0 hunks)packages/components/modules/messages/web/CreateGroup/index.tsx(4 hunks)packages/components/modules/messages/web/CreateGroup/styled.tsx(0 hunks)packages/components/modules/messages/web/CreateGroup/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMemberCard/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMemberCard/styled.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMemberCard/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/ScrollBarCustomStyle.css(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/constants.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/styled.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersMobile/constants.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersMobile/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersMobile/styled.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersMobile/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddedMemberCard/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddedMemberCard/styled.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddedMemberCard/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/constants.ts(1 hunks)packages/components/modules/messages/web/EditGroup/index.tsx(4 hunks)packages/components/modules/messages/web/EditGroup/types.ts(1 hunks)packages/components/modules/messages/web/GroupDetails/ProfileCard/constants.ts(0 hunks)packages/components/modules/messages/web/GroupDetails/ProfileCard/index.tsx(1 hunks)packages/components/modules/messages/web/GroupDetails/index.tsx(3 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx(3 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/types.ts(1 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfileCard/index.tsx(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfileCard/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfilesList/index.tsx(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfilesList/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/index.tsx(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/styled.tsx(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/LeaveGroupDialog/constants.ts(1 hunks)packages/components/modules/messages/web/__shared__/LeaveGroupDialog/index.tsx(3 hunks)packages/components/modules/messages/web/__shared__/LeaveGroupDialog/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/constants.ts(1 hunks)packages/components/modules/messages/web/__shared__/types.ts(1 hunks)packages/components/modules/profiles/common/graphql/queries/AllProfilesList.ts(1 hunks)packages/components/package.json(3 hunks)packages/components/schema.graphql(70 hunks)packages/components/tsup.config.ts(1 hunks)packages/design-system/.scripts/build-command.mjs(1 hunks)packages/design-system/.scripts/build.mjs(1 hunks)packages/design-system/.scripts/dev-command.mjs(1 hunks)packages/design-system/.storybook/main.ts(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/README.md(1 hunks)packages/design-system/components/web/buttons/AvatarButton/__storybook__/AvatarButton.mdx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/__storybook__/stories.tsx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/index.tsx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/styled.tsx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/types.ts(1 hunks)packages/design-system/components/web/buttons/index.ts(1 hunks)packages/design-system/components/web/dialogs/ConfirmDialog/index.tsx(2 hunks)packages/design-system/components/web/dialogs/ConfirmDialog/types.ts(2 hunks)packages/design-system/components/web/icons/ErrorIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/FilledCloseIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/index.ts(1 hunks)packages/design-system/package.json(2 hunks)packages/design-system/providers/web/SnackbarProvider/ProgressBar/index.tsx(1 hunks)packages/design-system/providers/web/SnackbarProvider/ProgressBar/styled.tsx(1 hunks)packages/design-system/providers/web/SnackbarProvider/ProgressBar/types.ts(1 hunks)packages/design-system/providers/web/SnackbarProvider/__storybook__/Snackbar.mdx(1 hunks)packages/design-system/providers/web/SnackbarProvider/__storybook__/stories.tsx(1 hunks)packages/design-system/providers/web/SnackbarProvider/constants.tsx(1 hunks)packages/design-system/providers/web/SnackbarProvider/index.tsx(2 hunks)packages/design-system/providers/web/SnackbarProvider/styled.tsx(1 hunks)packages/design-system/providers/web/SnackbarProvider/types.ts(1 hunks)packages/design-system/tsup.config.ts(1 hunks)packages/graphql/CHANGELOG.md(1 hunks)packages/graphql/package.json(1 hunks)packages/provider/CHANGELOG.md(1 hunks)packages/provider/package.json(1 hunks)packages/utils/CHANGELOG.md(1 hunks)packages/utils/functions/api/getApiErrorMessage/__tests__/getApiErrorMessage.test.ts(1 hunks)packages/utils/functions/api/getApiErrorMessage/index.ts(1 hunks)packages/utils/functions/form/setFormApiErrors/__tests__/setFormApiErrors.test.ts(2 hunks)packages/utils/functions/form/setFormApiErrors/index.ts(1 hunks)
⛔ Files not processed due to max files limit (8)
- packages/utils/hooks/useNotification/index.tsx
- packages/utils/hooks/useNotification/types.ts
- packages/utils/index.ts
- packages/utils/package.json
- packages/wagtail/CHANGELOG.md
- packages/wagtail/package.json
- pnpm-workspace.yaml
- turbo.json
💤 Files with no reviewable changes (5)
- packages/components/modules/messages/web/CreateGroup/styled.tsx
- packages/components/modules/messages/web/CreateGroup/ConnectionsList/types.ts
- packages/components/modules/messages/web/CreateGroup/constants.ts
- packages/components/modules/messages/web/CreateGroup/ConnectionsList/index.tsx
- packages/components/modules/messages/web/GroupDetails/ProfileCard/constants.ts
✅ Files skipped from review due to trivial changes (6)
- packages/provider/package.json
- packages/graphql/package.json
- packages/authentication/package.json
- packages/design-system/README.md
- packages/provider/CHANGELOG.md
- packages/authentication/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (42)
- packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/types.ts
- packages/design-system/components/web/icons/index.ts
- packages/design-system/components/web/buttons/index.ts
- packages/components/modules/messages/web/EditGroup/AddMembersMobile/styled.tsx
- packages/components/modules/messages/web/EditGroup/AddedMemberCard/types.ts
- packages/components/modules/messages/web/EditGroup/AddMemberCard/styled.tsx
- packages/components/modules/messages/web/MessagesList/MessagesGroup/types.ts
- packages/components/modules/messages/web/EditGroup/AddedMemberCard/styled.tsx
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/styled.tsx
- packages/components/modules/comments/web/CommentItem/index.tsx
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/constants.ts
- packages/components/modules/messages/web/EditGroup/AddMembersMobile/constants.ts
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/ScrollBarCustomStyle.css
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfileCard/types.ts
- packages/design-system/components/web/buttons/AvatarButton/types.ts
- packages/components/modules/messages/common/constants.ts
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfileCard/index.tsx
- packages/components/modules/messages/web/shared/GroupChatMembersList/types.ts
- packages/design-system/components/web/buttons/AvatarButton/styled.tsx
- packages/components/modules/messages/web/EditGroup/AddMemberCard/types.ts
- packages/design-system/components/web/icons/FilledCloseIcon/index.tsx
- packages/components/modules/messages/common/graphql/fragments/RoomTitle.ts
- packages/design-system/components/web/dialogs/ConfirmDialog/types.ts
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/types.ts
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfilesList/types.ts
- packages/design-system/components/web/buttons/AvatarButton/index.tsx
- packages/components/modules/messages/web/CreateChatRoomList/index.tsx
- packages/components/modules/messages/web/EditGroup/AddedMemberCard/index.tsx
- packages/components/modules/messages/web/GroupDetails/ProfileCard/index.tsx
- packages/design-system/components/web/buttons/AvatarButton/storybook/stories.tsx
- packages/design-system/components/web/dialogs/ConfirmDialog/index.tsx
- packages/components/modules/messages/web/EditGroup/AddMembersMobile/types.ts
- packages/components/modules/messages/web/EditGroup/AddMembersMobile/index.tsx
- packages/components/modules/messages/web/EditGroup/AddMemberCard/index.tsx
- packages/components/modules/messages/web/shared/constants.ts
- packages/components/modules/messages/web/shared/GroupChatMembersList/styled.tsx
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/index.tsx
- packages/components/modules/profiles/common/graphql/queries/AllProfilesList.ts
- packages/components/modules/messages/web/EditGroup/types.ts
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfilesList/index.tsx
- packages/components/schema.graphql
- packages/components/modules/messages/web/shared/LeaveGroupDialog/types.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/design-system/.scripts/dev-command.mjs
[error] 78-78: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/components/modules/messages/web/EditGroup/index.tsx
[error] 106-106: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/components/.scripts/dev-command.mjs
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 LanguageTool
packages/design-system/providers/web/SnackbarProvider/__storybook__/Snackbar.mdx
[uncategorized] ~10-~10: A comma might be missing here.
Context: ...nteracts with the message, if used with bar the message is always dismissed after t...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[typographical] ~15-~15: Usually, there’s no comma before “if”.
Context: ...'shouldShowProgress' option of sendToast, if it is set. - ...other: All other pr...
(IF_NO_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Lint Packages
🔇 Additional comments (64)
packages/components/README.md (1)
21-21: LGTM! Command update improves clarity.The change to use the full package name
@baseapp-frontend/componentsin the Storybook command is more explicit and helps prevent ambiguity.Also applies to: 26-26
README.md (1)
86-86: LGTM! Command update maintains consistency.The change to use the full package name
@baseapp-frontend/authenticationin the build command maintains consistency with:
- The package naming convention shown in the dependencies example
- The import example
- The similar change in the components package README
packages/components/CHANGELOG.md (3)
3-8: LGTM! Clear and well-documented version entry.The patch change is well-described, clearly explaining what was removed (leave option for non-group chats) and what was added (archive functionality).
9-19: LGTM! Comprehensive version entry with clear dependency updates.The patch change clearly describes the new snackbar component feature, and dependency versions are explicitly specified.
20-29: LGTM! Clear dependency updates.The version entry properly documents the dependency updates with specific versions.
packages/utils/functions/api/getApiErrorMessage/index.ts (1)
12-12: LGTM! Improved error message fallback logic.The change to use
error.messageas the fallback instead ofparsedMessageis a safer approach. It ensures we always have a meaningful error message, even when JSON parsing succeeds butparsedMessage.detailis not present.packages/utils/functions/api/getApiErrorMessage/__tests__/getApiErrorMessage.test.ts (1)
37-37: LGTM! Test description accurately reflects the behavior.The updated test description clearly communicates the expected behavior when handling invalid JSON in error messages. The test suite provides good coverage of various error scenarios.
packages/graphql/CHANGELOG.md (3)
3-8: LGTM! Version 1.2.6 entry follows the standard format.The changelog entry correctly documents the dependency update to
@baseapp-frontend/utils@3.1.5.
10-15: LGTM! Version 1.2.5 entry follows the standard format.The changelog entry correctly documents the dependency update to
@baseapp-frontend/utils@3.1.4.
3-15: Document the functional changes in the changelog.The changelog entries only document dependency updates, but the PR introduces significant functional changes related to chat room participants and admin roles. Consider adding these changes to the changelog to provide better visibility of the new features and modifications.
Please update the changelog to include:
- Addition of new GraphQL fields for chat room update types
- Changes related to chat room participant roles
- Addition of admin status functionality
packages/components/modules/messages/web/__shared__/types.ts (3)
10-14: Replaceany[]with more specific types.The
AddRemoveParticipantsinterface usesany[]which reduces type safety. Consider using more specific types based on the participant data structure.export interface AddRemoveParticipants { - addParticipants?: any[] - participants?: any[] - removeParticipants?: any[] + addParticipants?: ProfileNode[] + participants?: ProfileNode[] + removeParticipants?: ProfileNode[] }
16-16: LGTM!Clean interface extension that combines
TitleAndImageandAddRemoveParticipantsproperties.
18-20: Well-structured type definitions!The type definitions for both messages and profiles:
- Use NonNullable appropriately for type safety
- Follow a clear pattern for GraphQL fragment type extraction
- Are well-structured and reusable
Also applies to: 22-25
packages/design-system/components/web/icons/ErrorIcon/index.tsx (2)
1-3: LGTM! Clean and well-typed imports.The imports are minimal and correctly typed, following React + TypeScript best practices.
5-6: LGTM! Well-structured component definition.The component follows Material-UI patterns with:
- Proper TypeScript typing using FC
- Flexible styling through sx prop merging
- Correct props forwarding
packages/design-system/components/web/buttons/AvatarButton/__storybook__/AvatarButton.mdx (3)
1-3: LGTM!The Storybook configuration follows best practices with a clear hierarchical title structure.
14-21: LGTM!The use cases are well-documented with a clear distinction between current and potential usage scenarios.
23-29: Enhance props documentation with type information and constraints.The props documentation should be more detailed to help developers use the component correctly.
packages/design-system/providers/web/SnackbarProvider/types.ts (1)
5-6: LGTM! Clean type definition.The type definition is well-structured, properly extending MUI's SnackbarProps and including an optional boolean flag for progress indication.
packages/design-system/providers/web/SnackbarProvider/ProgressBar/types.ts (1)
3-9: LGTM! Well-structured type definitions.The types are well-organized, following composition patterns and properly extending MUI's types. Breaking down props into focused types improves maintainability and reusability.
packages/utils/functions/form/setFormApiErrors/index.ts (1)
25-33: LGTM! The undefined check is more precise.The change from truthy check to undefined check is a good improvement as it correctly handles
nulland0values.packages/components/.storybook/main.ts (1)
17-17: LGTM! Story paths follow existing pattern.The new story paths for design system providers are correctly structured and follow the existing pattern in the configuration.
Also applies to: 23-26
packages/utils/functions/form/setFormApiErrors/__tests__/setFormApiErrors.test.ts (2)
81-96: Great addition of test coverage for null and blank fields!The test case effectively verifies error handling for edge cases with null and empty string values.
98-115: Excellent test coverage for Error object handling!The test case properly verifies the function's ability to parse error messages from Error objects, which is a common error format in JavaScript/TypeScript applications.
packages/design-system/providers/web/SnackbarProvider/index.tsx (1)
89-106: LGTM! Well-structured Alert component with proper theme integration.The Alert component is properly configured with theme-aware styling and custom icons.
packages/design-system/CHANGELOG.md (1)
3-9: LGTM!The changelog entry is well-structured and follows semantic versioning. The changes are clearly documented, including both the new feature (snackbar component with progress bar) and the dependency update.
packages/components/modules/messages/web/EditGroup/constants.ts (2)
1-7: LGTM! Good use of shared constants.The refactoring to use shared constants and types from
__shared__promotes code reuse and maintainability.
9-17:❓ Verification inconclusive
Verify the impact of the return type change.
The function's return type has been updated from
EditGroupUploadtoCreateOrEditGroup. While the structure looks correct, ensure all consumers of this function are updated to handle the new type.
🏁 Script executed:
#!/bin/bash # Search for usages of getDefaultFormValues to verify type compatibility rg "getDefaultFormValues" --type typescriptLength of output: 78
Please verify the impact of the return type change manually.
While the updated function now returns a
CreateOrEditGroupas intended, our initial automated search did not yield usage results (due to the--type typescripterror). To ensure that all consumers are compatible with this type change:
- Check manually (or re-run the above search command) for any references to
getDefaultFormValuesacross your TypeScript files.- Confirm that downstream code is updated to work with the
CreateOrEditGrouptype instead of the previous type.
🏁 Script executed:
#!/bin/bash # Search for usages of getDefaultFormValues in TypeScript files rg "getDefaultFormValues" -g "*.ts"Length of output: 137
Ensure all consumers are manually verified for the type change.
The function now returns aCreateOrEditGroupobject instead of the previous type. Our automated search only revealed the function’s definition and no direct references elsewhere, so it’s essential to manually check that any indirect consumers or future invocations properly handle the updated type.
- Action Item: Manually inspect any call sites or modules using
getDefaultFormValuesto confirm they work with the newCreateOrEditGroupstructure.packages/components/modules/messages/common/graphql/mutations/ArchiveChatRoom.ts (6)
1-1: Use ofuseCurrentProfile
This import statement looks appropriate for retrieving the current user's profile information.
4-4: Relay import
These Relay imports are standard. No issues spotted with the usage ofConnectionHandler,useMutation, or related types.
7-7: Utility import
ImportinggetChatRoomConnectionsis consistent with the code’s approach for managing various chat room connection records.
14-14: Archival field
Adding theisArchivedfield to the returnedroompayload clarifies which chat rooms have been archived.
32-32: RetrievingcurrentProfile
AccessingcurrentProfilebefore using it in the mutation helps prevent null references and aligns with typical Relay patterns.
47-59: Updater archiving logic
ConnectionHandler.deleteNodeis a valid approach to remove the archived chat room from local records. However, the conditionarchived === !data?.chatRoomArchive?.room?.isArchivedcan be somewhat confusing.Please confirm that this filter correctly distinguishes archived vs. non-archived connections for all cases. A quick test or comment clarifying this logic might be beneficial.
packages/components/.scripts/build.mjs (6)
1-2: Imports for build scripts
Usingchalkfor colorized output andexecafor running commands is a common practice. Just ensure both libraries are in the appropriate dependencies.
4-4: Command tag
DefiningcommandTagfor log output provides clear source labeling during the build.
22-43: Concurrent bundling and type generation
Runningtsupand TypeScript declarations in parallel can shorten build times. The approach is well-structured.
45-50: Cleaning temporary files
Running a dedicated clean step maintains a tidy environment. This is straightforward and effective.
51-60: Error handling
Logging errors except when signaled bySIGTERMis a practical approach to handling interruption gracefully.
61-62: Resetting ongoing processes
ClearingcurrentBuildProcessesin thefinallyblock prevents lingering references after the build completes or fails.packages/design-system/.scripts/build-command.mjs (1)
1-3: Build command invocation
CallingrunBuild()provides a clean entry point for executing the build steps defined inbuild.mjs.packages/components/.scripts/build-command.mjs (1)
1-4: LGTM!The implementation is clean and follows best practices by separating the build command entry point from the build logic.
packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/ChatRoomOptions/types.ts (1)
1-8: LGTM!The new properties enhance the interface by providing necessary state information for archived chats and group-specific features. The properties are well-named and their types are appropriate.
packages/components/modules/messages/common/graphql/queries/ChatRoomQuery.ts (1)
1-14: LGTM!The addition of the
isArchivedfield is well-integrated into the query structure and aligns with the changes in other files for handling archived chats.packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/types.ts (1)
1-16: LGTM!The addition of the
isArchivedproperty is well-integrated into theChatRoomHeaderPropsinterface and aligns with the changes in other files for handling archived chats.packages/components/modules/messages/web/CreateGroup/types.ts (1)
3-3: LGTM! The changes improve component composition.The shift to using
GroupChatMembersListconsolidates member management functionality into a single component, improving maintainability and reducing prop drilling. The optional props allow for flexible component customization.Also applies to: 8-9
packages/components/tsup.config.ts (1)
7-7: LGTM! Clean builds prevent stale artifacts.Setting
clean: trueensures that each build starts fresh by cleaning the output directory, preventing potential issues with stale artifacts.packages/design-system/tsup.config.ts (1)
7-7: LGTM! Consistent build configuration across packages.Setting
clean: truealigns with the components package configuration, ensuring consistent build behavior across packages.packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/ChatRoomOptions/index.tsx (2)
8-10: LGTM! Enhanced archive functionality with proper loading states.The changes improve UX by:
- Showing appropriate text based on archive state.
- Preventing multiple archive actions during mutation.
Also applies to: 16-18
19-30: LGTM! Improved group-specific menu items.The conditional rendering of group options improves UX by only showing relevant menu items based on chat type.
packages/components/modules/messages/web/__shared__/LeaveGroupDialog/constants.ts (1)
1-36: LGTM! Well-structured constants with type safety.The implementation is clean, maintainable, and follows TypeScript best practices:
- Uses
as constfor type safety- Clear separation of concerns with distinct constant groups
- Intuitive nested structure for dialog text variations
packages/components/modules/messages/web/ChatRoom/index.tsx (1)
46-46: LGTM! Clean addition of isArchived prop.The boolean coercion using
!!ensures type safety for the isArchived prop.packages/design-system/.storybook/main.ts (1)
16-18: LGTM! Good addition of provider stories.The new story paths for providers follow Storybook's best practices and maintain consistency with the existing component stories configuration.
packages/components/modules/messages/common/utils.ts (2)
14-20: Add error handling for invalid headerRef type.The function casts
headerReftoGroupTitleFragment$keywithout validation, which could lead to runtime errors if an invalid type is provided.
71-83: Improve type safety in useCheckIsAdmin hook.The hook uses
anytype for participants which could lead to runtime errors. Consider creating proper types for the participants data structure.packages/components/modules/messages/web/__shared__/GroupChatMembersList/index.tsx (1)
49-54: Add debounce to search input.The search input directly triggers a refetch on every change, which could lead to unnecessary API calls. Consider adding debounce.
packages/components/modules/messages/web/ChatRoomsList/ChatRoomItem/index.tsx (1)
73-94:❓ Verification inconclusive
Verify store updates after removing the updater function.
The removal of the ConnectionHandler updater function might affect how the Relay store is updated after archiving/unarchiving chat rooms. Please ensure that the store is properly updated through other means (e.g., refetch, automatic store updates).
🏁 Script executed:
#!/bin/bash # Search for other store update mechanisms ast-grep --pattern 'useArchiveChatRoomMutation($$$)'Length of output: 349
Action Required: Verify Relay Store Update Mechanism
After checking the codebase, both the ChatRoomItem and ChatRoomHeader components use the
useArchiveChatRoomMutationhook without the ConnectionHandler updater function. No alternative explicit updater or refetch mechanism was found in these files. Please confirm that the Relay environment’s automatic store update behavior or another mechanism correctly handles the archived/unarchived chat room updates.
- Key Files:
packages/components/modules/messages/web/ChatRoomItem/index.tsxpackages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsxEnsure to test the UI behavior after the mutation to verify the store is updated as expected.
package.json (1)
26-27:✅ Verification successful
Verify compatibility of new dev dependencies.
Let's confirm that these versions are stable and compatible with the project's Node.js version (>=20.17.0).
Also applies to: 29-29
🌐 Web query:
What are the latest stable versions of chalk, chokidar, and execa? Are they compatible with Node.js 20?💡 Result:
The latest stable versions and their compatibility with Node.js 20 are as follows:
chalk
- Latest version: v5.0.0 (released December 2024)[1][7]
- Compatibility: Requires Node.js ≥12.20[1]
- Key changes: Pure ESM package, removed less-used coloring methods like
.keyword(), addedoverlinestyle support[1]chokidar
- Latest version: v4.0.3 (released December 2024)[9]
- Compatibility: Requires Node.js ≥14[9]
- Improvements: Reduced dependencies (from 13 to 1), dropped glob support[9]
execa
- Latest version: v9.5.2 (released December 2024)[2][8]
- Compatibility: Works with modern Node.js versions including v20[2][8]
- Recent features: IPC message passing between processes, graceful cancellation[2]
Node.js 20 Compatibility
All three packages are fully compatible with Node.js 20[1][4][9]:
- Node.js 20 requires ≥14 for chokidar (met by chokidar v4)
- Tested with updated V8 engine (v11.3) in Node.js 20[4]
- Execa v9.x specifically addresses PATH handling improvements relevant to modern environments[2]
No breaking changes or compatibility issues were found between these package versions and Node.js 20 based on current release notes[1][4][9].
Citations:
- 1: https://github.com/chalk/chalk/releases
- 2: https://github.com/sindresorhus/execa/releases
- 3: https://www.npmjs.com/package/node-sass-chokidar
- 4: https://nodejs.org/en/blog/announcements/v20-release-announce
- 5: https://github.com/ehmicky/nvexeca
- 6: https://forum.ghost.org/t/help-updating-ghost/31455
- 7: https://www.npmjs.com/package/chalk/v/2.4.1?activeTab=versions
- 8: https://security.snyk.io/package/npm/execa
- 9: https://www.npmjs.com/package/chokidar
- 10: https://deno.com/blog/v2.0
Dependency versions verified and compatible with Node.js 20
After reviewing the latest release notes and compatibility details, the new devDependencies in package.json have been confirmed as stable:
- chalk: Specified as "^5.4.1" (latest stable branch confirms compatibility with Node.js ≥12).
- chokidar: Specified as "^4.0.3" (fully compatible with Node.js ≥14).
- execa: Specified as "9.5.2" (designed for modern Node.js versions including v20).
No issues or breaking changes were found for Node.js 20, so these versions can be safely used. Also applies to lines 29-29.
packages/design-system/package.json (2)
4-4: LGTM! Version bump to 1.0.4.The version increment follows semantic versioning principles.
11-12:✅ Verification successful
Verify the new build and dev command files.
The build and dev scripts now use dedicated command files. Please ensure these files exist at:
.scripts/build-command.mjs.scripts/dev-command.mjs
🏁 Script executed:
#!/bin/bash # Check if the new command files exist ls -l packages/design-system/.scripts/build-command.mjs packages/design-system/.scripts/dev-command.mjsLength of output: 301
Good to go – file verification confirmed
I verified that both the build (
.scripts/build-command.mjs) and dev (.scripts/dev-command.mjs) command files exist in the expected location within thepackages/design-systemdirectory. The file details confirm their presence and expected sizes:
packages/design-system/.scripts/build-command.mjs(51 bytes)packages/design-system/.scripts/dev-command.mjs(2425 bytes)No further action is required.
packages/components/package.json (4)
4-4: LGTM! Version bump to 1.0.5.The version increment follows semantic versioning principles.
7-7: LGTM! Renamed script for clarity.The rename from
babel:bundletobabel:transpilebetter reflects the script's purpose, as it's transpiling the code rather than bundling it.
12-13:✅ Verification successful
Verify the new build and dev command files.
The build and dev scripts now use dedicated command files. Please ensure these files exist at:
.scripts/build-command.mjs.scripts/dev-command.mjs
🏁 Script executed:
#!/bin/bash # Check if the new command files exist ls -l packages/components/.scripts/build-command.mjs packages/components/.scripts/dev-command.mjsLength of output: 289
Verified: Build and Dev Command Files Exist
Both the build and dev command files exist in the expected folder (
packages/components/.scripts) and are valid as per the verification output.
- Confirmed files:
packages/components/.scripts/build-command.mjs(51 bytes)packages/components/.scripts/dev-command.mjs(2610 bytes)
115-115:❓ Verification inconclusive
Verify workspace dependency version change impact.
The dependency on
@baseapp-frontend/confighas been changed fromworkspace:*toworkspace:^. This change affects version resolution behavior.
🌐 Web query:
What is the difference between workspace:* and workspace:^ in pnpm workspace dependencies?💡 Result:
The
workspace:*andworkspace:^protocols in pnpm handle monorepo dependency management differently based on semantic versioning constraints:Key Differences
workspace:*
- Pins dependencies to exact versions found in your monorepo
- When published: Converts to fixed version (e.g.,
1.5.0)- Ensures consumers get identical code used during development
- Example:
"utils": "workspace:*" → "utils": "1.5.0"after publish
workspace:^
- Allows minor/patch updates post-publication using semver caret
- When published: Converts to version range (e.g.,
^1.5.0)- Permits downstream users to get compatible updates automatically
- Example:
"utils": "workspace:^" → "utils": "^1.5.0"after publishUsage Recommendations
Protocol Best For Publishing Behavior *Strict parity between dev/prod environments Exact version lock ^Library packages with semver-compatible APIs Caret range for minor updates The distinction primarily affects published packages, not local development workflows[4][7]. Both protocols ensure monorepo packages resolve locally during development via symlinks[3][6]. Use
workspace:*when you need absolute consistency between monorepo development and published artifacts[4][7].Citations:
- 1: https://adamcoster.com/blog/pnpm-config
- 2: https://pnpm.io/cli/deploy
- 3: `"workspace:*"` vs `"*"` in `package.json`'s `dependencies` vercel/turborepo#7687
- 4: https://cuyl.github.io/pnpm.github.io/workspaces/
- 5: https://pnpm.io/cli/add
- 6: https://www.shew.dev/monorepos/package-managers/pnpm
- 7: https://pnpm.io/workspaces
- 8: https://answers.netlify.com/t/using-pnpm-workspaces-in-2023/84882
- 9: https://dev.to/jonlauridsen/attempt-2-workspaces-npm-pnpm-336a
- 10: https://edrickleong.hashnode.dev/why-you-shouldnt-use-npm-workspaces
Verify Workspace Dependency Version Impact
The dependency on
@baseapp-frontend/configinpackages/components/package.jsonhas been updated from usingworkspace:*(which pins the dependency to an exact version during development and publishes a fixed version) toworkspace:^(which allows semver-compatible minor/patch updates in published artifacts). This change means that downstream users could receive updates automatically as long as they remain compatible with the current major version.Please ensure that this flexibility is intended and that consumers of the package are equipped to handle potential minor/patch updates.
packages/components/modules/messages/web/EditGroup/constants.ts
Outdated
Show resolved
Hide resolved
...components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/web/__shared__/GroupChatMembersList/index.tsx
Show resolved
Hide resolved
packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsx
Outdated
Show resolved
Hide resolved
anicioalexandre
force-pushed
the
BA-2259-improve-dev-mode
branch
from
1ea18a1 to
7afb5f5
Compare
Ronan-Fernandes
force-pushed
the
BA-2114-add-members-to-existing-group
branch
2 times, most recently
from
731c13d to
c99fba2
Compare
anicioalexandre
force-pushed
the
BA-2259-improve-dev-mode
branch
2 times, most recently
from
f25e030 to
e183314
Compare
anicioalexandre
dismissed stale reviews from tsl-ps2 and pt-tsl
The base branch was changed.
Ronan-Fernandes
force-pushed
the
BA-2114-add-members-to-existing-group
branch
2 times, most recently
from
4db8d06 to
535b500
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/components/modules/messages/web/CreateGroup/index.tsx (1)
66-103: 🛠️ Refactor suggestionAdd validation for minimum required participants.
The form submission should validate that there is at least one participant besides the creator.
const onSubmit = handleSubmit((data: CreateOrEditGroup) => { + if (!data.participants?.length) { + sendToast('Please add at least one participant to the group', { type: 'error' }) + return + } const dirtyValues = filterDirtyValues({ values: data, dirtyFields }) // ... rest of the code })
🧹 Nitpick comments (11)
packages/components/modules/messages/web/__shared__/LeaveGroupDialog/index.tsx (1)
51-76: Enhance error handling in onRemoveConfirmed.Consider adding error handling for the case when
removingParticipantIdis undefined.const onRemoveConfirmed = () => { - if (!roomId || !profileId) return + if (!roomId || !profileId || !removingParticipantId) { + sendToast('Invalid operation: Missing required data', { type: 'error' }) + return + }packages/components/modules/messages/common/utils.ts (1)
14-25: Add validation for headerRef type.The function should validate the headerRef type before casting.
export const useGroupNameAndAvatar = ( headerRef: GroupTitleFragment$key | RoomTitleFragment$key | null | undefined, ) => { + if (!headerRef) { + return { title: undefined, avatar: undefined } + } const header = useFragment<GroupTitleFragment$key>( GroupTitleFragment, headerRef as GroupTitleFragment$key, )packages/components/modules/messages/web/__shared__/GroupChatMembersList/index.tsx (2)
83-94: Add error boundary for profile rendering.The
renderItemfunction should be wrapped in an error boundary to gracefully handle rendering failures.+import { ErrorBoundary } from 'react-error-boundary' + const renderItem = (profile: ProfileNode, isMember = false) => { if (!profile) return null return ( + <ErrorBoundary fallback={<div>Error loading profile</div>}> <ProfileCard profile={profile} handleAddMember={handleAddMember} handleRemoveMember={handleRemoveMember} isMember={isMember} {...ProfileCardProps} /> + </ErrorBoundary> ) }
96-133: Consider adding loading states for member actions.The component should show loading states during add/remove operations to provide better user feedback.
+const [loadingProfileId, setLoadingProfileId] = useState<string | null>(null) + const handleAddMember = (profile: ProfileNode) => { + setLoadingProfileId(profile.id) setValue(FORM_VALUE.participants, [...currentParticipants, profile], { shouldValidate: true, shouldDirty: true, shouldTouch: true, }) + setLoadingProfileId(null) } return ( <ProfileCard profile={profile} handleAddMember={handleAddMember} handleRemoveMember={handleRemoveMember} isMember={isMember} + isLoading={loadingProfileId === profile.id} {...ProfileCardProps} /> )packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsx (1)
74-81: Add loading state for dialog operations.The dialog should show loading states during leave/remove operations.
+const [isDialogActionLoading, setIsDialogActionLoading] = useState(false) <LeaveGroupDialog open={open} onClose={() => setOpen(false)} profileId={currentProfile?.id ?? ''} roomId={roomId} removingParticipantId={currentProfile?.id ?? ''} isSoleAdmin={isSoleAdmin} + isLoading={isDialogActionLoading} + onBeforeAction={() => setIsDialogActionLoading(true)} + onAfterAction={() => setIsDialogActionLoading(false)} />packages/components/modules/messages/web/CreateGroup/index.tsx (1)
107-118: Optimize participant filtering.The filtering logic can be optimized by using a Set for better performance with large lists.
+const participantIds = useMemo( + () => new Set(participants.map((member: ProfileNode) => member?.id)), + [participants] +) const profiles = useMemo( () => allProfiles?.edges .filter( (edge) => edge?.node && edge?.node.id !== currentProfile?.id && - !participants.some((member: ProfileNode) => member?.id === edge?.node?.id), + !participantIds.has(edge?.node?.id), ) .map((edge) => edge?.node) || [], - [allProfiles, participants, currentProfile?.id], + [allProfiles, participantIds, currentProfile?.id], )packages/components/modules/messages/web/GroupDetails/index.tsx (2)
119-127: Remove TODO comment and implement sole admin removal handling.The TODO comment indicates missing functionality for sole admin removal. This should be implemented to ensure proper group management.
Would you like me to help implement the sole admin removal handling logic? This would include:
- Preventing sole admin from leaving without assigning a new admin
- Adding a dialog to assign a new admin before leaving
- Handling edge cases like no other members available
73-84: Add state persistence for participant removal.The participant removal state is lost on component re-render. Consider using a more persistent state management solution.
-const [removingParticipantId, setRemovingParticipantId] = useState<string | undefined>(undefined) -const removingParticipantName = useRef<string | null | undefined>(undefined) +const [removingParticipant, setRemovingParticipant] = useState<{ + id: string | undefined + name: string | null | undefined +}>({ id: undefined, name: undefined }) const initiateRemoval = (id: string, name: string | null | undefined) => { - setRemovingParticipantId(id) - removingParticipantName.current = name + setRemovingParticipant({ id, name }) } const handleRemoveDialogClose = () => { - setRemovingParticipantId(undefined) - removingParticipantName.current = undefined + setRemovingParticipant({ id: undefined, name: undefined }) }packages/components/modules/messages/web/EditGroup/index.tsx (2)
65-71: Consider improving type safety in participants mapping.The
edge?.node?.profilemapping usesanytype which could be made more type-safe.Consider this improvement:
- membersList?.participants?.edges?.map( - (edge: any) => edge?.node?.profile && edge.node.profile, - ) as ProfileNode[], + membersList?.participants?.edges?.map( + (edge: { node?: { profile?: ProfileNode } }) => + edge?.node?.profile ?? null, + ).filter((profile): profile is ProfileNode => profile !== null),
106-106: Consider alternative to delete operator for better performance.The use of
deleteoperator can impact performance.Consider this alternative:
- delete dirtyValues.participants + dirtyValues.participants = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 106-106: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/components/modules/messages/web/CreateChatRoomList/index.tsx (1)
128-133:❓ Verification inconclusive
LGTM with minor suggestions.
The
AvatarButtonimplementation is clean and follows the component's API correctly. Consider these improvements:
- Move the SVG path to a constants file for better maintainability
- Use a more descriptive alt text, e.g., "Create new group chat"
+// Add to constants file +const GROUP_CHAT_AVATAR_PATH = '/svg/avatar-group-chat.svg' <AvatarButton onClick={onGroupChatCreationButtonClicked} caption="New Group" - imageString="/svg/avatar-group-chat.svg" - alt="Avatar Group Fallback" + imageString={GROUP_CHAT_AVATAR_PATH} + alt="Create new group chat" />Let's verify that the SVG asset exists:
🏁 Script executed:
#!/bin/bash # Check if the SVG file exists in the public directory fd -t f "avatar-group-chat.svg" -x echo "Found SVG at: {}"Length of output: 58
LGTM with minor suggestions
The
AvatarButtoncomponent implementation is clean and adheres to the component API. However, please address the following:
- SVG Asset Management: Move the hardcoded SVG path (
"/svg/avatar-group-chat.svg") to a constants file for better maintainability.- Alt Text Improvement: Update the
altattribute to be more descriptive (e.g.,"Create new group chat").- Asset Verification: The automated search for the SVG asset did not produce any output. Please manually verify that
avatar-group-chat.svgexists in the expected location.+// Add to constants file +const GROUP_CHAT_AVATAR_PATH = '/svg/avatar-group-chat.svg'; <AvatarButton onClick={onGroupChatCreationButtonClicked} caption="New Group" - imageString="/svg/avatar-group-chat.svg" - alt="Avatar Group Fallback" + imageString={GROUP_CHAT_AVATAR_PATH} + alt="Create new group chat" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (58)
packages/components/modules/comments/web/CommentItem/index.tsx(1 hunks)packages/components/modules/messages/common/constants.ts(1 hunks)packages/components/modules/messages/common/graphql/fragments/RoomTitle.ts(1 hunks)packages/components/modules/messages/common/utils.ts(3 hunks)packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsx(3 hunks)packages/components/modules/messages/web/CreateChatRoomList/index.tsx(2 hunks)packages/components/modules/messages/web/CreateGroup/ConnectionsList/index.tsx(0 hunks)packages/components/modules/messages/web/CreateGroup/ConnectionsList/types.ts(0 hunks)packages/components/modules/messages/web/CreateGroup/constants.ts(0 hunks)packages/components/modules/messages/web/CreateGroup/index.tsx(4 hunks)packages/components/modules/messages/web/CreateGroup/styled.tsx(0 hunks)packages/components/modules/messages/web/CreateGroup/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMemberCard/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMemberCard/styled.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMemberCard/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/ScrollBarCustomStyle.css(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/constants.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/styled.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersMobile/constants.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersMobile/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersMobile/styled.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersMobile/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddedMemberCard/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddedMemberCard/styled.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddedMemberCard/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/constants.ts(1 hunks)packages/components/modules/messages/web/EditGroup/index.tsx(4 hunks)packages/components/modules/messages/web/EditGroup/types.ts(1 hunks)packages/components/modules/messages/web/GroupDetails/ProfileCard/constants.ts(0 hunks)packages/components/modules/messages/web/GroupDetails/ProfileCard/index.tsx(1 hunks)packages/components/modules/messages/web/GroupDetails/index.tsx(3 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/types.ts(1 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfileCard/index.tsx(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfileCard/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfilesList/index.tsx(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfilesList/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/index.tsx(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/styled.tsx(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/LeaveGroupDialog/constants.ts(1 hunks)packages/components/modules/messages/web/__shared__/LeaveGroupDialog/index.tsx(3 hunks)packages/components/modules/messages/web/__shared__/LeaveGroupDialog/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/constants.ts(1 hunks)packages/components/modules/messages/web/__shared__/types.ts(1 hunks)packages/components/modules/profiles/common/graphql/queries/AllProfilesList.ts(1 hunks)packages/design-system/components/web/buttons/AvatarButton/__storybook__/AvatarButton.mdx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/__storybook__/stories.tsx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/index.tsx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/styled.tsx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/types.ts(1 hunks)packages/design-system/components/web/buttons/index.ts(1 hunks)packages/design-system/components/web/dialogs/ConfirmDialog/index.tsx(2 hunks)packages/design-system/components/web/dialogs/ConfirmDialog/types.ts(2 hunks)packages/design-system/components/web/icons/FilledCloseIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/index.ts(1 hunks)
💤 Files with no reviewable changes (5)
- packages/components/modules/messages/web/CreateGroup/constants.ts
- packages/components/modules/messages/web/CreateGroup/ConnectionsList/types.ts
- packages/components/modules/messages/web/GroupDetails/ProfileCard/constants.ts
- packages/components/modules/messages/web/CreateGroup/styled.tsx
- packages/components/modules/messages/web/CreateGroup/ConnectionsList/index.tsx
🚧 Files skipped from review as they are similar to previous changes (43)
- packages/design-system/components/web/icons/index.ts
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfileCard/types.ts
- packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/types.ts
- packages/components/modules/messages/web/EditGroup/AddMemberCard/styled.tsx
- packages/components/modules/messages/web/MessagesList/MessagesGroup/types.ts
- packages/design-system/components/web/buttons/AvatarButton/styled.tsx
- packages/components/modules/messages/web/EditGroup/AddMembersMobile/styled.tsx
- packages/components/modules/messages/web/EditGroup/AddedMemberCard/styled.tsx
- packages/components/modules/comments/web/CommentItem/index.tsx
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfileCard/index.tsx
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfilesList/types.ts
- packages/design-system/components/web/icons/FilledCloseIcon/index.tsx
- packages/components/modules/messages/common/graphql/fragments/RoomTitle.ts
- packages/design-system/components/web/buttons/index.ts
- packages/design-system/components/web/buttons/AvatarButton/index.tsx
- packages/components/modules/messages/web/EditGroup/AddMembersMobile/constants.ts
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/styled.tsx
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/index.tsx
- packages/components/modules/messages/web/GroupDetails/ProfileCard/index.tsx
- packages/design-system/components/web/buttons/AvatarButton/types.ts
- packages/components/modules/messages/web/EditGroup/AddedMemberCard/types.ts
- packages/components/modules/profiles/common/graphql/queries/AllProfilesList.ts
- packages/components/modules/messages/web/shared/GroupChatMembersList/styled.tsx
- packages/components/modules/messages/web/shared/GroupChatMembersList/types.ts
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfilesList/index.tsx
- packages/components/modules/messages/common/constants.ts
- packages/components/modules/messages/web/EditGroup/AddMemberCard/types.ts
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/constants.ts
- packages/components/modules/messages/web/EditGroup/AddMembersMobile/types.ts
- packages/design-system/components/web/dialogs/ConfirmDialog/types.ts
- packages/components/modules/messages/web/shared/constants.ts
- packages/components/modules/messages/web/EditGroup/AddedMemberCard/index.tsx
- packages/design-system/components/web/buttons/AvatarButton/storybook/AvatarButton.mdx
- packages/design-system/components/web/dialogs/ConfirmDialog/index.tsx
- packages/components/modules/messages/web/EditGroup/AddMemberCard/index.tsx
- packages/components/modules/messages/web/EditGroup/AddMembersMobile/index.tsx
- packages/design-system/components/web/buttons/AvatarButton/storybook/stories.tsx
- packages/components/modules/messages/web/shared/LeaveGroupDialog/constants.ts
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/ScrollBarCustomStyle.css
- packages/components/modules/messages/web/EditGroup/types.ts
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/types.ts
- packages/components/modules/messages/web/CreateGroup/types.ts
- packages/components/modules/messages/web/shared/LeaveGroupDialog/types.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/messages/web/EditGroup/index.tsx
[error] 106-106: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (13)
packages/components/modules/messages/web/EditGroup/constants.ts (2)
19-21: Strengthen the participants validation schema.The validation schema still uses
z.any()for participants validation which is too permissive and could lead to runtime issues.
9-17: LGTM! Function signature updated correctly.The return type change from
EditGroupUploadtoCreateOrEditGroupaligns with the PR objectives for managing group members.packages/components/modules/messages/web/__shared__/types.ts (2)
10-14: Replaceany[]with more specific types.The
AddRemoveParticipantsinterface still usesany[]which reduces type safety.
18-25: LGTM! Well-structured type definitions for messages and profiles.The new type definitions for
AllMessages,MessageEdges,MessageNode,AllProfiles,AllProfilesEdges,ProfileEdge, andProfileNodeprovide strong type safety and clear data structure.packages/components/modules/messages/web/__shared__/LeaveGroupDialog/index.tsx (1)
33-49: LGTM! Well-structured dynamic text copy management.The
getLeaveGroupDialogTextCopyfunction effectively handles different scenarios based on user roles and actions.packages/components/modules/messages/common/utils.ts (1)
71-83: Improve type safety in useCheckIsAdmin hook.The hook uses
anytype for participants which could lead to runtime errors.packages/components/modules/messages/web/__shared__/GroupChatMembersList/index.tsx (2)
49-54: Add debounce to search input.The search input directly triggers a refetch on every change, which could lead to unnecessary API calls.
63-69: Add validation for duplicate members.The function should check if the profile is already in the participants list before adding it.
packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsx (1)
52-65: Add error handling and user feedback for archive operation.packages/components/modules/messages/web/EditGroup/index.tsx (3)
3-34: LGTM! Import changes align with member management feature.The new imports and type definitions are well-structured and support the member management functionality.
148-216: LGTM! Well-implemented responsive member management UI.The implementation correctly handles:
- Responsive dialog display based on screen size
- Member list management with loading states
- Proper prop drilling for customization
221-229: LGTM! Clean wrapper implementation.The wrapper component correctly handles profile ID injection with proper null checks.
packages/components/modules/messages/web/CreateChatRoomList/index.tsx (1)
6-6: LGTM! Import changes are well-organized.The imports are properly structured and align with the component's requirements.
Also applies to: 10-10, 18-18
Ronan-Fernandes
force-pushed
the
BA-2114-add-members-to-existing-group
branch
from
535b500 to
3210155
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
packages/components/modules/messages/web/CreateChatRoomList/index.tsx (1)
128-133: Enhance accessibility and internationalization.Consider the following improvements:
- Use a more descriptive alt text that explains the purpose of the button
- Extract the "New Group" caption to a translation file for i18n support
Apply this diff:
<AvatarButton onClick={onGroupChatCreationButtonClicked} - caption="New Group" + caption={t('messages.createGroup.newGroupButton')} imageString="/svg/avatar-group-chat.svg" - alt="Avatar Group Fallback" + alt="Create a new group chat" />packages/design-system/components/web/buttons/AvatarButton/__storybook__/AvatarButton.mdx (1)
39-52: Improve example with TypeScript and correct imports.The example could be enhanced with TypeScript types and the correct import path.
Apply this diff:
-```javascript +```typescript -import { AvatarButton } from '@baseapp-frontend/design-system' +import { AvatarButton } from '@baseapp-frontend/design-system/components/web/buttons' -const MyComponent = () => { +const MyComponent: React.FC = () => { return ( <AvatarButton - onClick={() => console.log('clicked')} + onClick={(): void => console.log('clicked')} imageString="/path/to/avatar.jpg" caption="Add Member" + alt="Add a new member" /> ) } export default MyComponentpackages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfilesList/index.tsx (3)
14-31: LGTM! Props are well-defined with sensible defaults.The component's interface is well-designed and aligns with the PR objectives for adding members to existing groups.
Consider extracting the magic number
5used inloadNext(line 84) to a named constant at the top of the file for better maintainability:+const LOAD_MORE_ITEMS_COUNT = 5;
32-43: Consider using theme spacing for consistency.The loading state implementation is clean and accessible, but could be improved for better maintainability.
Consider using theme spacing instead of hardcoded values:
- sx={{ paddingTop: 3, paddingBottom: 1 }} + sx={{ paddingTop: theme => theme.spacing(3), paddingBottom: theme => theme.spacing(1) }} - CircularProgressProps={{ size: 15 }} + CircularProgressProps={{ size: theme.spacing(1.875) }}
51-96: Consider improvements for cross-browser compatibility and configurability.The virtualization implementation is good for performance, but there are a few areas for improvement:
- Make the maxHeight configurable via props:
+interface ProfilesListProps { + maxHeight?: number | string; + // ... other props +} // In the component - style={{ scrollbarWidth: 'none', maxHeight: '250px' }} + style={{ scrollbarWidth: 'none', maxHeight: maxHeight ?? '250px' }} - <Box maxHeight={250} overflow="auto" + <Box maxHeight={maxHeight ?? 250} overflow="auto"
- Add cross-browser support for hiding scrollbars:
+const scrollbarHiddenStyles = { + scrollbarWidth: 'none' as const, + '&::-webkit-scrollbar': { + display: 'none', + }, + msOverflowStyle: 'none', +}; // In the component - style={{ scrollbarWidth: 'none', maxHeight: maxHeight ?? '250px' }} + style={{ ...scrollbarHiddenStyles, maxHeight: maxHeight ?? '250px' }} - <Box maxHeight={maxHeight ?? 250} overflow="auto" sx={{ scrollbarWidth: 'none' }} + <Box maxHeight={maxHeight ?? 250} overflow="auto" sx={scrollbarHiddenStyles}packages/components/modules/messages/web/__shared__/types.ts (1)
16-16: Consider separating create and edit group interfaces.The combined interface might not accurately represent the different requirements between creating and editing a group. Consider creating separate interfaces with appropriate required/optional fields for each operation.
-export interface CreateOrEditGroup extends TitleAndImage, AddRemoveParticipants {} +export interface CreateGroup extends TitleAndImage { + participants: ProfileNode[] +} + +export interface EditGroup extends Partial<TitleAndImage>, AddRemoveParticipants {}packages/components/modules/messages/web/__shared__/LeaveGroupDialog/index.tsx (1)
72-74: Enhance error message specificity.The generic error message could be enhanced to provide more context about what went wrong during the operation.
- sendToast(error.message, { type: 'error' }) + sendToast( + `Failed to ${removingParticipantId === profileId ? 'leave' : 'remove member from'} group: ${error.message}`, + { type: 'error' } + )packages/components/modules/messages/web/CreateGroup/index.tsx (1)
162-174: Consider memoizing the GroupChatMembersList props.The component props object is recreated on every render. Consider memoizing it to optimize performance.
+const memoizedProps = useMemo( + () => ({ + FORM_VALUE, + setValue, + watch, + currentParticipants: participants, + connections: profiles, + refetch, + connectionsLoadNext: loadNext, + connectionsHasNext: hasNext, + connectionsIsLoadingNext: isLoadingNext, + ProfilesContainer, + ...GroupChatMembersListProps, + }), + [ + participants, + profiles, + refetch, + loadNext, + hasNext, + isLoadingNext, + GroupChatMembersListProps, + ] +) - <GroupChatMembersList - FORM_VALUE={FORM_VALUE} - setValue={setValue} - watch={watch} - currentParticipants={participants} - connections={profiles} - refetch={refetch} - connectionsLoadNext={loadNext} - connectionsHasNext={hasNext} - connectionsIsLoadingNext={isLoadingNext} - ProfilesContainer={ProfilesContainer} - {...GroupChatMembersListProps} - /> + <GroupChatMembersList {...memoizedProps} />packages/components/modules/messages/web/EditGroup/index.tsx (2)
106-106: Consider using undefined assignment for better performance.The
deleteoperator can impact performance. Consider using undefined assignment instead.- delete dirtyValues.participants + dirtyValues.participants = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 106-106: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
201-216: Consider memoizing MembersListProps.The nested props object is recreated on every render. Consider memoizing it to optimize performance.
+const memoizedMembersListProps = useMemo( + () => ({ + allowAddMember: true, + onAddMemberClick: () => setOpen(true), + ...(GroupChatMembersListProps?.MembersListProps ?? {}), + }), + [GroupChatMembersListProps?.MembersListProps] +) +const memoizedProps = useMemo( + () => ({ + FORM_VALUE, + setValue, + watch, + currentParticipants: participants, + refetch, + membersLoadNext: loadNext, + membersHasNext: hasNext, + membersIsLoadingNext: isLoadingNext, + MembersListProps: memoizedMembersListProps, + ...GroupChatMembersListProps, + }), + [ + participants, + refetch, + loadNext, + hasNext, + isLoadingNext, + memoizedMembersListProps, + GroupChatMembersListProps, + ] +) - <GroupChatMembersList - FORM_VALUE={FORM_VALUE} - setValue={setValue} - watch={watch} - currentParticipants={participants} - refetch={refetch} - membersLoadNext={loadNext} - membersHasNext={hasNext} - membersIsLoadingNext={isLoadingNext} - MembersListProps={{ - allowAddMember: true, - onAddMemberClick: () => setOpen(true), - ...(GroupChatMembersListProps?.MembersListProps ?? {}), - }} - {...GroupChatMembersListProps} - /> + <GroupChatMembersList {...memoizedProps} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (58)
packages/components/modules/comments/web/CommentItem/index.tsx(1 hunks)packages/components/modules/messages/common/constants.ts(1 hunks)packages/components/modules/messages/common/graphql/fragments/RoomTitle.ts(1 hunks)packages/components/modules/messages/common/utils.ts(3 hunks)packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsx(3 hunks)packages/components/modules/messages/web/CreateChatRoomList/index.tsx(2 hunks)packages/components/modules/messages/web/CreateGroup/ConnectionsList/index.tsx(0 hunks)packages/components/modules/messages/web/CreateGroup/ConnectionsList/types.ts(0 hunks)packages/components/modules/messages/web/CreateGroup/constants.ts(0 hunks)packages/components/modules/messages/web/CreateGroup/index.tsx(4 hunks)packages/components/modules/messages/web/CreateGroup/styled.tsx(0 hunks)packages/components/modules/messages/web/CreateGroup/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMemberCard/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMemberCard/styled.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMemberCard/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/ScrollBarCustomStyle.css(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/constants.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/styled.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersMobile/constants.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersMobile/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersMobile/styled.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersMobile/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddedMemberCard/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddedMemberCard/styled.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddedMemberCard/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/constants.ts(1 hunks)packages/components/modules/messages/web/EditGroup/index.tsx(4 hunks)packages/components/modules/messages/web/EditGroup/types.ts(1 hunks)packages/components/modules/messages/web/GroupDetails/ProfileCard/constants.ts(0 hunks)packages/components/modules/messages/web/GroupDetails/ProfileCard/index.tsx(1 hunks)packages/components/modules/messages/web/GroupDetails/index.tsx(3 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/types.ts(1 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfileCard/index.tsx(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfileCard/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfilesList/index.tsx(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfilesList/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/index.tsx(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/styled.tsx(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/LeaveGroupDialog/constants.ts(1 hunks)packages/components/modules/messages/web/__shared__/LeaveGroupDialog/index.tsx(3 hunks)packages/components/modules/messages/web/__shared__/LeaveGroupDialog/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/constants.ts(1 hunks)packages/components/modules/messages/web/__shared__/types.ts(1 hunks)packages/components/modules/profiles/common/graphql/queries/AllProfilesList.ts(1 hunks)packages/design-system/components/web/buttons/AvatarButton/__storybook__/AvatarButton.mdx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/__storybook__/stories.tsx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/index.tsx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/styled.tsx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/types.ts(1 hunks)packages/design-system/components/web/buttons/index.ts(1 hunks)packages/design-system/components/web/dialogs/ConfirmDialog/index.tsx(2 hunks)packages/design-system/components/web/dialogs/ConfirmDialog/types.ts(2 hunks)packages/design-system/components/web/icons/FilledCloseIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/index.ts(1 hunks)
💤 Files with no reviewable changes (5)
- packages/components/modules/messages/web/CreateGroup/styled.tsx
- packages/components/modules/messages/web/CreateGroup/ConnectionsList/index.tsx
- packages/components/modules/messages/web/GroupDetails/ProfileCard/constants.ts
- packages/components/modules/messages/web/CreateGroup/ConnectionsList/types.ts
- packages/components/modules/messages/web/CreateGroup/constants.ts
🚧 Files skipped from review as they are similar to previous changes (42)
- packages/design-system/components/web/icons/index.ts
- packages/components/modules/messages/web/MessagesList/MessagesGroup/types.ts
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/styled.tsx
- packages/components/modules/comments/web/CommentItem/index.tsx
- packages/design-system/components/web/buttons/index.ts
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/ScrollBarCustomStyle.css
- packages/components/modules/messages/common/graphql/fragments/RoomTitle.ts
- packages/components/modules/messages/web/EditGroup/AddMemberCard/styled.tsx
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfileCard/types.ts
- packages/components/modules/messages/web/EditGroup/AddedMemberCard/styled.tsx
- packages/design-system/components/web/buttons/AvatarButton/types.ts
- packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/types.ts
- packages/design-system/components/web/icons/FilledCloseIcon/index.tsx
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/types.ts
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfilesList/types.ts
- packages/components/modules/messages/web/EditGroup/AddedMemberCard/types.ts
- packages/components/modules/profiles/common/graphql/queries/AllProfilesList.ts
- packages/components/modules/messages/web/EditGroup/AddMembersMobile/constants.ts
- packages/components/modules/messages/web/GroupDetails/ProfileCard/index.tsx
- packages/components/modules/messages/common/constants.ts
- packages/components/modules/messages/web/EditGroup/AddMembersMobile/types.ts
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/index.tsx
- packages/components/modules/messages/web/shared/GroupChatMembersList/styled.tsx
- packages/design-system/components/web/dialogs/ConfirmDialog/index.tsx
- packages/components/modules/messages/web/EditGroup/AddMemberCard/index.tsx
- packages/components/modules/messages/web/shared/GroupChatMembersList/index.tsx
- packages/design-system/components/web/buttons/AvatarButton/index.tsx
- packages/design-system/components/web/buttons/AvatarButton/styled.tsx
- packages/components/modules/messages/web/EditGroup/AddMembersMobile/index.tsx
- packages/components/modules/messages/web/shared/constants.ts
- packages/components/modules/messages/web/EditGroup/AddMemberCard/types.ts
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfileCard/index.tsx
- packages/components/modules/messages/web/shared/GroupChatMembersList/types.ts
- packages/components/modules/messages/web/EditGroup/AddMembersMobile/styled.tsx
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/constants.ts
- packages/components/modules/messages/web/shared/LeaveGroupDialog/constants.ts
- packages/design-system/components/web/dialogs/ConfirmDialog/types.ts
- packages/components/modules/messages/web/shared/LeaveGroupDialog/types.ts
- packages/design-system/components/web/buttons/AvatarButton/storybook/stories.tsx
- packages/components/modules/messages/web/CreateGroup/types.ts
- packages/components/modules/messages/web/EditGroup/types.ts
- packages/components/modules/messages/web/EditGroup/AddedMemberCard/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/messages/web/EditGroup/index.tsx
[error] 106-106: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (14)
packages/components/modules/messages/web/EditGroup/constants.ts (3)
1-7: LGTM! Good use of shared constants.The imports are well-structured, and using shared constants promotes code reuse and maintainability.
9-17: LGTM! Return type aligns with group member management.The function correctly initializes the form values with empty arrays for
addParticipantsandremoveParticipants, which aligns with the PR objectives for managing group members.
19-22: Strengthen the participants validation schema.Using
z.any()for participants validation is too permissive and could lead to runtime issues. Consider defining a more specific validation schema that matches the expected data structure.export const DEFAULT_FORM_VALIDATION = z.object({ ...DEFAULT_CREATE_OR_EDIT_GROUP_FORM_VALIDATION.shape, - [CREATE_OR_EDIT_GROUP_FORM_VALUE.participants]: z.any(), + [CREATE_OR_EDIT_GROUP_FORM_VALUE.participants]: z.array( + z.object({ + id: z.string(), + // Add other required participant fields + }) + ), })packages/design-system/components/web/buttons/AvatarButton/__storybook__/AvatarButton.mdx (1)
23-29: Enhance props documentation with type information and constraints.The props documentation should be more detailed to help developers use the component correctly.
packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfilesList/index.tsx (2)
1-13: LGTM! Well-organized imports and type declarations.The imports follow a logical order and make good use of the design system components.
44-50: LGTM! Clear and comprehensive conditional rendering logic.The empty states and search results are handled appropriately with clear variable names.
packages/components/modules/messages/web/__shared__/types.ts (1)
10-14: Replaceany[]with more specific types.The
AddRemoveParticipantsinterface usesany[]which reduces type safety. Consider using more specific types based on the participant data structure.packages/components/modules/messages/common/utils.ts (2)
71-83: Improve type safety in useCheckIsAdmin hook.The hook uses
anytype for participants which could lead to runtime errors. Consider creating proper types for the participants data structure.
14-20: Add error handling for invalid headerRef type.The
useGroupNameAndAvatarfunction castsheaderReftoGroupTitleFragment$keywithout validation, which could lead to runtime errors if an invalid type is provided.packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsx (1)
52-65: Add error handling and user feedback for archive operation.The archive operation should handle errors and provide feedback to the user about the operation's success or failure.
packages/components/modules/messages/web/CreateGroup/index.tsx (1)
66-69: LGTM! Type safety improvements.The changes improve type safety by:
- Using the more descriptive
CreateOrEditGrouptype.- Adding explicit type annotation for
memberin the participants mapping.packages/components/modules/messages/web/GroupDetails/index.tsx (2)
60-60: LGTM! Improved admin check logic.The new
useCheckIsAdminhook provides a cleaner way to handle admin permissions and sole admin status.
120-127: LGTM! Enhanced dialog props.The changes improve the LeaveGroupDialog by:
- Providing a default empty string for
removingParticipantId.- Adding
isSoleAdminprop for better UX.packages/components/modules/messages/web/EditGroup/index.tsx (1)
141-146: LGTM! Robust member addition handling.The success handler properly:
- Closes the dialog.
- Uses transition for smoother UX.
- Refetches member list to ensure data consistency.
packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsx
Show resolved
Hide resolved
packages/components/modules/messages/web/EditGroup/AddMembersDialog/ScrollBarCustomStyle.css
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfileCard/types.ts
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/web/EditGroup/AddMemberCard/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
packages/components/modules/messages/web/CreateChatRoomList/index.tsx (1)
102-103: 🛠️ Refactor suggestionAddress the TODO comment regarding height calculation.
The current height calculation is brittle and depends on specific pixel values. Consider:
- Using CSS Grid or Flexbox for dynamic height calculation
- Creating a constant for header heights
- Using CSS custom properties for maintainable values
- style={{ scrollbarWidth: 'none', maxHeight: 'calc(100vh - 72px - 57px - 61px - 72px)' }} + style={{ + scrollbarWidth: 'none', + height: '100%', + flex: 1, + minHeight: 0 + }}Also update the parent container:
const MainContainer = styled(Box)({ + display: 'flex', + flexDirection: 'column', + height: '100%', + overflow: 'hidden' })packages/design-system/components/web/buttons/AvatarButton/__storybook__/AvatarButton.mdx (1)
39-53: 🛠️ Refactor suggestionCorrect JSX Prop Syntax in Example Usage.
The example usage snippet contains a JSX syntax issue:
- The
Iconprop should be passed as an expression (e.g., useIcon={AddMemberIcon}instead ofIcon=AddMemberIcon).- The
IconPropsprop should use proper object literal notation. For instance, change fromIconProps={{sx={{fontSize: 20}}}}toIconProps={{ sx: { fontSize: 20 } }}.Please apply the following diff to update the example:
- Icon=AddMemberIcon - IconProps={{sx={{fontSize: 20}}}} + Icon={AddMemberIcon} + IconProps={{ sx: { fontSize: 20 } }}
🧹 Nitpick comments (4)
packages/design-system/components/web/icons/AddMemberIcon/index.tsx (1)
7-39: Add ARIA attributes for better accessibility.While the implementation is correct, consider enhancing accessibility by adding ARIA attributes to the SVG.
Apply this diff to improve accessibility:
- <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none"> + <svg + xmlns="http://www.w3.org/2000/svg" + width="24" + height="24" + viewBox="0 0 24 24" + fill="none" + role="img" + aria-label="Add member icon" + >packages/components/modules/messages/web/EditGroup/AddMemberCard/types.ts (1)
1-1: Consider using path aliases for better maintainability.The deep relative import path (
../../../../profiles/common) could be fragile if the directory structure changes. Consider using TypeScript path aliases (e.g.,@profiles/common) to make imports more maintainable.packages/design-system/components/web/buttons/AvatarButton/index.tsx (1)
18-35: Enhance accessibility by adding keyboard interaction attributes.The button should be keyboard accessible. Consider adding the following improvements:
- <AvatarButtonContainer onClick={onClick}> + <AvatarButtonContainer + onClick={onClick} + role="button" + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + onClick?.() + } + }} + >packages/design-system/components/web/buttons/AvatarButton/__storybook__/AvatarButton.mdx (1)
30-36: Related Components Section is Informative.
Listing related components such asButton,Avatar, andTooltipis helpful. For improved usability, consider adding links to their documentation if available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
packages/components/modules/messages/common/constants.ts(1 hunks)packages/components/modules/messages/common/types.ts(1 hunks)packages/components/modules/messages/common/utils.ts(3 hunks)packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsx(3 hunks)packages/components/modules/messages/web/CreateChatRoomList/index.tsx(2 hunks)packages/components/modules/messages/web/EditGroup/AddMemberCard/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMemberCard/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddedMemberCard/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddedMemberCard/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfileCard/index.tsx(2 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfileCard/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfilesList/index.tsx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/__storybook__/AvatarButton.mdx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/__storybook__/stories.tsx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/index.tsx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/types.ts(1 hunks)packages/design-system/components/web/icons/AddMemberIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/NewGroupIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/components/modules/messages/common/constants.ts
- packages/components/modules/messages/web/EditGroup/AddedMemberCard/types.ts
- packages/design-system/components/web/icons/index.ts
- packages/components/modules/messages/web/EditGroup/AddedMemberCard/index.tsx
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfilesList/index.tsx
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfileCard/index.tsx
- packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsx
- packages/components/modules/messages/web/EditGroup/AddMemberCard/index.tsx
- packages/design-system/components/web/buttons/AvatarButton/types.ts
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/index.tsx
- packages/components/modules/messages/common/utils.ts
🔇 Additional comments (14)
packages/components/modules/messages/common/types.ts (2)
4-4: LGTM! Import statement follows consistent pattern.The addition of
CHAT_ROOM_PARTICIPANT_ROLESto the existing import statement maintains code organization and supports the role-based access control requirements.
12-12: LGTM! Type definition enhances type safety for role checks.The new
ChatRoomParticipantRolestype follows the established pattern usingValueOfand provides type safety for role-based access control, which is crucial for the admin-only functionality in the group management features.packages/design-system/components/web/icons/AddMemberIcon/index.tsx (3)
1-3: LGTM! Clean and minimal imports.The imports are well-organized, importing only the necessary types and components.
5-6: LGTM! Props handling follows best practices.Good use of:
- TypeScript FC type for type safety
- Props destructuring with rest operator
- Proper merging of custom styles with default styles
42-42: LGTM! Clean export.The default export is appropriate for a single component.
packages/components/modules/messages/web/EditGroup/AddMemberCard/types.ts (1)
3-9: Well-structured interface with proper typing!The
AddMemberCardPropsinterface is well-designed with:
- Strong typing using
ProfileNodeinstead ofany- Clear handler functions for add/remove operations
- Appropriate boolean flags for UI states
This aligns perfectly with the PR objectives for managing group members.
packages/design-system/components/web/buttons/AvatarButton/__storybook__/stories.tsx (1)
18-18: Replace console.log with Storybook actions.The stories are using console.log for click handlers which should be replaced with Storybook actions for better debugging and documentation.
Also applies to: 24-24, 31-31, 38-38
packages/design-system/components/web/icons/NewGroupIcon/index.tsx (1)
5-46: Well-implemented SVG icon component!The implementation follows SVG best practices:
- Uses currentColor for theme compatibility
- Includes proper viewBox and dimensions
- Clean and optimized path definitions
packages/components/modules/messages/web/CreateChatRoomList/index.tsx (1)
129-133: LGTM! Clean implementation of the new group creation button.The AvatarButton is well integrated with appropriate props for the group creation functionality.
packages/design-system/components/web/buttons/AvatarButton/__storybook__/AvatarButton.mdx (4)
1-3: Meta Tag Usage is Correct.
The import statement and the<Meta>tag are properly included to integrate this component into Storybook.
5-11: Component Documentation Section is Well-Structured.
The documentation clearly outlines the component’s purpose and expected behavior. As the component evolves, ensure any new states or behaviors are documented here.
12-22: Use Cases Section Provides Good Context.
The current and potential use cases are clearly listed, which helps developers understand how and when to utilize theAvatarButtoncomponent.
23-29: Enhance Props Documentation with Detailed Type Information.
Consider replacing the bullet list with a table format that includes columns for Prop, Type, Required, Default, and Description. This will greatly improve clarity and consistency with our documentation standards.packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfileCard/types.ts (1)
3-8: LGTM! The interface is well-structured.The
ProfileCardPropsinterface aligns well with the PR objectives for member management functionality. The optionalisMemberflag will be useful for conditional rendering in the UI.
packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfileCard/types.ts
Show resolved
Hide resolved
Ronan-Fernandes
force-pushed
the
BA-2114-add-members-to-existing-group
branch
3 times, most recently
from
b1fd1b9 to
6648798
Compare
feat: leave chatroom as admin feat: moved group chat list display functionality from CreateGroup into shared component GroupChatMembersList feat: add members to existing group Add member PR fixes Mobile updates PR updates feat: highlight existing member Add members carousel Fix rebase PR fixes
Ronan-Fernandes
force-pushed
the
BA-2114-add-members-to-existing-group
branch
from
6648798 to
3d5ad40
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
packages/components/modules/messages/web/GroupDetails/index.tsx (1)
132-133:⚠️ Potential issueMissing "Add Member" functionality required by PR objectives.
While the edit button visibility is correctly controlled by admin status, the PR objectives require an "Add Member" option for admins. This functionality appears to be missing from the current implementation.
The PR objectives specifically mention:
- "Add Member" option should be available only to admins on the edit group page
- A modal should display a list of contacts
- Users should be able to select multiple contacts
Please implement the "Add Member" functionality as specified in the PR objectives. Would you like assistance with implementing this feature?
packages/design-system/components/web/buttons/AvatarButton/__storybook__/AvatarButton.mdx (1)
39-53: 🛠️ Refactor suggestion
⚠️ Potential issueCorrect JSX Prop Assignment in Example Usage
Within the example usage, theIconprop is passed asIcon=AddMemberIcon(line 46), which will not correctly evaluate the JavaScript variable. It should be wrapped in curly braces (i.e.Icon={AddMemberIcon}) so the JSX engine interprets it as an expression.Example diff suggestion:
- Icon=AddMemberIcon + Icon={AddMemberIcon}Additionally, for clarity and consistency, consider formatting the
IconPropsobject with proper spacing (e.g.IconProps={{ sx: { fontSize: 20 } }}) to improve readability.
♻️ Duplicate comments (4)
packages/components/modules/messages/web/EditGroup/constants.ts (1)
19-22:⚠️ Potential issueStrengthen the participants validation schema.
Using
z.any()for participants validation is too permissive and could lead to runtime issues. Consider defining a more specific validation schema that matches the expected data structure.export const DEFAULT_FORM_VALIDATION = z.object({ ...DEFAULT_CREATE_OR_EDIT_GROUP_FORM_VALIDATION.shape, - [CREATE_OR_EDIT_GROUP_FORM_VALUE.participants]: z.any(), + [CREATE_OR_EDIT_GROUP_FORM_VALUE.participants]: z.array( + z.object({ + id: z.string(), + // Add other required participant fields + }) + ), })packages/components/modules/messages/web/__shared__/types.ts (1)
10-14:⚠️ Potential issueReplace
any[]with more specific types.The
AddRemoveParticipantsinterface usesany[]which reduces type safety. Based on the available types, we should useProfileNode[].export interface AddRemoveParticipants { - addParticipants?: any[] - participants?: any[] - removeParticipants?: any[] + addParticipants?: ProfileNode[] + participants?: ProfileNode[] + removeParticipants?: ProfileNode[] }packages/components/modules/messages/common/utils.ts (1)
15-21:⚠️ Potential issueAdd type validation for headerRef.
The function still casts
headerRefwithout proper validation, which could lead to runtime errors.export const useGroupNameAndAvatar = ( headerRef: GroupTitleFragment$key | RoomTitleFragment$key | null | undefined, ) => { + if (!headerRef || !('__fragmentType' in headerRef)) { + return { title: undefined, avatar: undefined } + } const header = useFragment<GroupTitleFragment$key>( GroupTitleFragment, headerRef as GroupTitleFragment$key, )packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsx (1)
79-82:⚠️ Potential issueImprove error handling for undefined profile ID.
Using empty string as fallback for undefined profile IDs could lead to invalid API calls.
Apply this diff:
- profileId={currentProfile?.id ?? ''} - removingParticipantId={currentProfile?.id ?? ''} + profileId={currentProfile?.id} + removingParticipantId={currentProfile?.id} + disabled={!currentProfile?.id}
🧹 Nitpick comments (6)
packages/design-system/components/web/icons/AddMemberIcon/index.tsx (2)
6-6: Consider using theme tokens for fontSize.While the styling is flexible, consider using theme tokens for fontSize to maintain consistency across the design system.
- <SvgIcon sx={{ fontSize: 24, color: 'primary.contrastText', ...sx }} {...props}> + <SvgIcon sx={{ fontSize: 'medium', color: 'primary.contrastText', ...sx }} {...props}>
7-38: Add ARIA attributes for better accessibility.The SVG implementation is clean, but adding ARIA attributes would improve accessibility.
- <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none"> + <svg + xmlns="http://www.w3.org/2000/svg" + width="24" + height="24" + viewBox="0 0 24 24" + fill="none" + role="img" + aria-label="Add member icon" + >packages/components/modules/messages/common/utils.ts (1)
72-81: Add error handling for edge cases in useCheckIsAdmin.The hook should handle cases where participants or edges are undefined.
export const useCheckIsAdmin = (participants: MembersListFragment$data['participants']) => { const { currentProfile } = useCurrentProfile() + if (!participants?.edges || !currentProfile?.id) { + return { isAdmin: false, isSoleAdmin: false } + } const me = participants?.edges?.find((edge) => edge?.node?.profile?.id === currentProfile?.id) const isAdmin = me?.node?.role === CHAT_ROOM_PARTICIPANT_ROLES.admin const isSoleAdmin = isAdmin && participants?.edges?.filter((edge) => edge?.node?.role === CHAT_ROOM_PARTICIPANT_ROLES.admin) .length === 1 return { isAdmin, isSoleAdmin } }packages/components/modules/messages/web/CreateGroup/index.tsx (1)
83-88: Address TODO comment about filter handling.The TODO comment indicates that filter handling needs to be implemented properly.
Would you like me to help implement proper filter handling for the connection ID or create an issue to track this task?
packages/components/modules/messages/web/EditGroup/index.tsx (2)
106-106: Optimize object property removal.Using the delete operator can impact performance.
Apply this diff to use undefined assignment instead:
- delete dirtyValues.participants + dirtyValues.participants = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 106-106: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
143-145: Consider optimizing the refetch operation.The refetch operation inside startTransition could be optimized by using empty parameters.
Apply this diff:
- refetch?.({}) + refetch?.()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (62)
packages/components/modules/comments/web/CommentItem/index.tsx(1 hunks)packages/components/modules/messages/common/constants.ts(1 hunks)packages/components/modules/messages/common/graphql/fragments/RoomTitle.ts(1 hunks)packages/components/modules/messages/common/types.ts(1 hunks)packages/components/modules/messages/common/utils.ts(3 hunks)packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsx(3 hunks)packages/components/modules/messages/web/CreateChatRoomList/ChatRoomListItem/index.tsx(2 hunks)packages/components/modules/messages/web/CreateChatRoomList/index.tsx(2 hunks)packages/components/modules/messages/web/CreateGroup/ConnectionsList/index.tsx(0 hunks)packages/components/modules/messages/web/CreateGroup/ConnectionsList/types.ts(0 hunks)packages/components/modules/messages/web/CreateGroup/ProfileCard/types.ts(0 hunks)packages/components/modules/messages/web/CreateGroup/constants.ts(0 hunks)packages/components/modules/messages/web/CreateGroup/index.tsx(4 hunks)packages/components/modules/messages/web/CreateGroup/styled.tsx(0 hunks)packages/components/modules/messages/web/CreateGroup/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMemberCard/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMemberCard/styled.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMemberCard/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/constants.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/styled.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersDialog/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersMobile/constants.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersMobile/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersMobile/styled.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddMembersMobile/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/AddedMemberCard/index.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddedMemberCard/styled.tsx(1 hunks)packages/components/modules/messages/web/EditGroup/AddedMemberCard/types.ts(1 hunks)packages/components/modules/messages/web/EditGroup/constants.ts(1 hunks)packages/components/modules/messages/web/EditGroup/index.tsx(4 hunks)packages/components/modules/messages/web/EditGroup/types.ts(1 hunks)packages/components/modules/messages/web/GroupDetails/ProfileCard/constants.ts(0 hunks)packages/components/modules/messages/web/GroupDetails/ProfileCard/index.tsx(1 hunks)packages/components/modules/messages/web/GroupDetails/index.tsx(3 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/types.ts(1 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfileCard/index.tsx(2 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfileCard/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfilesList/index.tsx(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/ProfilesList/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/index.tsx(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/styled.tsx(1 hunks)packages/components/modules/messages/web/__shared__/GroupChatMembersList/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/LeaveGroupDialog/constants.ts(1 hunks)packages/components/modules/messages/web/__shared__/LeaveGroupDialog/index.tsx(3 hunks)packages/components/modules/messages/web/__shared__/LeaveGroupDialog/types.ts(1 hunks)packages/components/modules/messages/web/__shared__/constants.ts(1 hunks)packages/components/modules/messages/web/__shared__/types.ts(1 hunks)packages/components/modules/profiles/common/graphql/queries/AllProfilesList.ts(1 hunks)packages/design-system/components/web/buttons/AvatarButton/__storybook__/AvatarButton.mdx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/__storybook__/stories.tsx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/index.tsx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/styled.tsx(1 hunks)packages/design-system/components/web/buttons/AvatarButton/types.ts(1 hunks)packages/design-system/components/web/buttons/index.ts(1 hunks)packages/design-system/components/web/dialogs/ConfirmDialog/index.tsx(2 hunks)packages/design-system/components/web/dialogs/ConfirmDialog/types.ts(2 hunks)packages/design-system/components/web/icons/AddMemberIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/FilledCloseIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/NewGroupIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/index.ts(2 hunks)
💤 Files with no reviewable changes (6)
- packages/components/modules/messages/web/CreateGroup/ProfileCard/types.ts
- packages/components/modules/messages/web/CreateGroup/ConnectionsList/types.ts
- packages/components/modules/messages/web/GroupDetails/ProfileCard/constants.ts
- packages/components/modules/messages/web/CreateGroup/ConnectionsList/index.tsx
- packages/components/modules/messages/web/CreateGroup/styled.tsx
- packages/components/modules/messages/web/CreateGroup/constants.ts
🚧 Files skipped from review as they are similar to previous changes (44)
- packages/components/modules/messages/web/EditGroup/AddMembersMobile/styled.tsx
- packages/components/modules/comments/web/CommentItem/index.tsx
- packages/components/modules/messages/web/EditGroup/AddMembersMobile/constants.ts
- packages/design-system/components/web/icons/NewGroupIcon/index.tsx
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/constants.ts
- packages/components/modules/messages/web/EditGroup/AddMemberCard/styled.tsx
- packages/design-system/components/web/buttons/AvatarButton/styled.tsx
- packages/components/modules/messages/common/constants.ts
- packages/design-system/components/web/buttons/AvatarButton/index.tsx
- packages/design-system/components/web/buttons/AvatarButton/types.ts
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/styled.tsx
- packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/types.ts
- packages/components/modules/messages/web/EditGroup/AddedMemberCard/types.ts
- packages/components/modules/messages/common/types.ts
- packages/components/modules/messages/web/GroupDetails/ProfileCard/index.tsx
- packages/components/modules/profiles/common/graphql/queries/AllProfilesList.ts
- packages/design-system/components/web/buttons/index.ts
- packages/components/modules/messages/web/EditGroup/AddMembersMobile/index.tsx
- packages/components/modules/messages/web/MessagesList/MessagesGroup/types.ts
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfilesList/types.ts
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfileCard/types.ts
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/index.tsx
- packages/design-system/components/web/dialogs/ConfirmDialog/types.ts
- packages/design-system/components/web/dialogs/ConfirmDialog/index.tsx
- packages/components/modules/messages/web/EditGroup/AddedMemberCard/styled.tsx
- packages/components/modules/messages/web/shared/GroupChatMembersList/types.ts
- packages/design-system/components/web/icons/FilledCloseIcon/index.tsx
- packages/components/modules/messages/web/shared/GroupChatMembersList/styled.tsx
- packages/components/modules/messages/web/shared/LeaveGroupDialog/constants.ts
- packages/components/modules/messages/web/EditGroup/AddedMemberCard/index.tsx
- packages/components/modules/messages/common/graphql/fragments/RoomTitle.ts
- packages/components/modules/messages/web/shared/GroupChatMembersList/index.tsx
- packages/components/modules/messages/web/shared/LeaveGroupDialog/types.ts
- packages/components/modules/messages/web/EditGroup/AddMembersDialog/types.ts
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfileCard/index.tsx
- packages/components/modules/messages/web/shared/GroupChatMembersList/ProfilesList/index.tsx
- packages/design-system/components/web/icons/index.ts
- packages/components/modules/messages/web/EditGroup/types.ts
- packages/components/modules/messages/web/EditGroup/AddMemberCard/index.tsx
- packages/components/modules/messages/web/EditGroup/AddMembersMobile/types.ts
- packages/components/modules/messages/web/shared/LeaveGroupDialog/index.tsx
- packages/components/modules/messages/web/shared/constants.ts
- packages/design-system/components/web/buttons/AvatarButton/storybook/stories.tsx
- packages/components/modules/messages/web/EditGroup/AddMemberCard/types.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/messages/web/EditGroup/index.tsx
[error] 106-106: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Lint Packages
- GitHub Check: Component Test Packages
🔇 Additional comments (17)
packages/components/modules/messages/web/EditGroup/constants.ts (2)
1-7: LGTM! Good job on improving code reuse.Moving constants and types to a shared location promotes better code organization and reusability.
9-17: LGTM! Well-structured implementation.The function provides a clean interface for managing group participants, with separate arrays for additions and removals.
packages/design-system/components/web/icons/AddMemberIcon/index.tsx (3)
1-3: LGTM! Clean and minimal imports.The imports are well-organized, importing only the necessary types and components.
5-5: LGTM! Well-typed component definition.The component is properly typed with
FC<SvgIconProps>and uses clean props destructuring.
42-42: LGTM! Clean export.The default export follows React component conventions.
packages/components/modules/messages/web/GroupDetails/index.tsx (3)
21-21: LGTM! Clean import addition.The new
useCheckIsAdminhook import follows the existing import pattern.
61-61: LGTM! Improved admin check implementation.The
useCheckIsAdminhook simplifies the admin status check logic and provides additional functionality (isSoleAdmin) that will be useful for the member management features.
120-128: Address the TODO comment about sole admin removal.The TODO comment indicates that sole admin removal will be handled in another story. This is important for the member management feature to work correctly.
The prop changes look good:
- Using null coalescing for
removingParticipantIdensures type safety- Adding
isSoleAdminprop enables proper handling of admin-specific casesPlease confirm if there's a follow-up story or issue tracking the sole admin removal implementation. This is crucial for completing the member management feature.
packages/components/modules/messages/web/CreateGroup/types.ts (1)
3-3: LGTM! Good refactoring of types.The changes consolidate member management functionality by replacing multiple specific props with a single
GroupChatMembersListcomponent and its props. This improves code organization and maintainability.Also applies to: 8-9
packages/components/modules/messages/web/__shared__/types.ts (1)
18-25: LGTM! Well-structured type definitions.The new type definitions for messages and profiles make good use of TypeScript features like NonNullable and proper array indexing, ensuring type safety throughout the application.
packages/components/modules/messages/web/CreateChatRoomList/ChatRoomListItem/index.tsx (1)
7-7: LGTM! Good UI enhancement.Replacing Typography with TypographyWithEllipsis improves the user experience by properly handling text overflow in chat room names.
Also applies to: 33-33
packages/components/modules/messages/web/CreateChatRoomList/index.tsx (1)
129-133: LGTM! Clean refactor using AvatarButton.The replacement of the GroupChatContainer with AvatarButton consolidates the functionality into a single, reusable component, improving code maintainability.
packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsx (1)
54-67:⚠️ Potential issueAdd error handling for archive operation.
The archive operation should handle errors and provide feedback to users.
Apply this diff:
+import { useNotification } from '@baseapp-frontend/utils' + const ChatRoomHeader: FC<ChatRoomHeaderProps> = ({ ... }) => { + const { sendToast } = useNotification() const toggleArchiveChatroom = () => { popover.onClose() if (currentProfile?.id && roomId) { commit({ variables: { input: { roomId, profileId: currentProfile.id, archive: !isArchived, }, }, + onCompleted: () => { + sendToast( + `Chat ${isArchived ? 'unarchived' : 'archived'} successfully.`, + { type: 'success' } + ) + }, + onError: (error) => { + sendToast( + `Failed to ${isArchived ? 'unarchive' : 'archive'} chat: ${error.message}`, + { type: 'error' } + ) + }, }) } }packages/components/modules/messages/web/CreateGroup/index.tsx (1)
69-69: LGTM! Good type safety practice.The explicit type annotation for
memberasProfileNodeimproves code clarity and type safety.packages/design-system/components/web/buttons/AvatarButton/__storybook__/AvatarButton.mdx (3)
1-3: Storybook Meta Setup Looks Good
The import and usage of the Meta component to set the Storybook title are correctly implemented.
9-11: Clear Component Purpose & Behavior Documentation
The purpose and expected behavior of the AvatarButton are clearly documented, providing a straightforward description for consumers of the component.
23-29: Enhance Props Documentation for Clarity
While the props are documented as bullet points, consider enhancing this section using a tabular format (as previously suggested) to clearly specify each prop’s type, whether it’s required, default values, and a detailed description.Example diff suggestion:
-## Props - -- **onClick** (function): The function to be called when the button is clicked. -- **Icon** (ReactNode, optional): The icon to be displayed in the button. -- **IconProps** (SvgIconProps, optional): Additional properties for configuring the icon (e.g. size, color, etc.). -- **caption** (string, optional): The caption text to be displayed next to the avatar. +## Props + +| Prop | Type | Required | Default | Description | +|-----------|-----------------------|----------|---------------|----------------------------------------------------------------------| +| onClick | `() => void` | Yes | - | Function called when the button is clicked | +| Icon | `ReactNode` | No | - | Icon displayed inside the button | +| IconProps | `SvgIconProps` | No | - | Additional configuration for the icon (e.g. size, color, etc.) | +| caption | `string` | No | - | Optional caption text shown next to the avatar |
|
Acceptance Criteria
Context
The only instance where we add members to groups is at the creation of the group, but in this story we want to enable adding members to an existing group (after creation)
Business Rules - Add Member List
Adding a member is only enabled to admins
Given I open the edit group page, I should see an "Add Member" option in the Members List section.
Given I've clicked on the Add Member option, then the "Add Member Modal" should appear with a list of Contacts I can add to the Group
While on the Add Member List, I should be able to select multiple contacts to add at once.
Make sure that the scroll bar can be selectable in the mobile view so that users don't have to manually scroll in order to get to the bottom for example.
While on the Add Member List, I should be able to use a search bar to look for a specific contact.
Given I have selected at least 1 Contact then enable the "confirm" button.
Given I have confirmed the addition of the selected members then they should be added to the group and appear in the Members Section.
Given I am on the Add Members List, when I click the cancel button, then the Add Member Modal should close and the selections should not be saved if opened again.
Guardrails
In this story we will even show already existing members in the Add member list. QA and other developers should know that it might break if they select an existing member to be added again. We will fix this in another story.
In summary we are doing the following in this story:
**Create the Add Member Modal
Design Link: BaseApp - WEB
Summary by CodeRabbit
New Features
AddMemberIcon,FilledCloseIcon, andNewGroupIconfor better icon representation in the UI.GroupChatMembersListcomponent for managing and displaying group chat members.AddMembersDialogandAddMembersMobilecomponents for streamlined member addition.Improvements
ConfirmDialogwith customizable props for greater flexibility.AvatarButtoncomponent for improved user interaction.LeaveGroupDialogwith dynamic text and simplified logic.Documentation