BA-2060: leave group chat #176
Conversation
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/components/modules/messages/ChatRoom/ChatRoomHeader/ChatRoomOptions/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
WalkthroughThis pull request introduces several enhancements to the messaging and group chat functionality across multiple components. The changes focus on improving user interactions within chat rooms, adding new options for managing group chats, and refactoring existing components. Key modifications include the introduction of a Changes
Sequence DiagramsequenceDiagram
participant User
participant ChatRoomHeader
participant ChatRoomOptions
participant LeaveGroupDialog
User->>ChatRoomHeader: Click options button
ChatRoomHeader->>ChatRoomOptions: Open popover
User->>ChatRoomOptions: Select "Leave Group"
ChatRoomOptions->>LeaveGroupDialog: Trigger leave group dialog
User->>LeaveGroupDialog: Confirm leave
LeaveGroupDialog->>ChatRoomHeader: Remove user from group
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
tsl-ps2
force-pushed
the
BA-2054-removing-a-member-frontend-packages
branch
2 times, most recently
from
8741deb to
8286379
Compare
pt-tsl
force-pushed
the
BA-2060-leave-group
branch
from
3db11c4 to
85ac599
Compare
| </Box> | ||
| <Box> | ||
| <IconButton | ||
| onClick={(e) => onChatRoomOptionsClicked(e)} |
There was a problem hiding this comment.
onClick={onChatRoomOptionsClicked} is a bit shorter
| <ChatRoomOptions | ||
| onArchiveClicked={() => {}} | ||
| onDetailsClicked={() => | ||
| roomHeader.isGroup ? onDisplayGroupDetailsClicked : undefined |
There was a problem hiding this comment.
Does this really work without invoking onDisplayGroupDetailsClicked? Or should it rather be onDetailsClicked={roomHeader.isGroup ? onDisplayGroupDetailsClicked : undefined} (or onDetailsClicked={() => roomHeader.isGroup ? onDisplayGroupDetailsClicked() : undefined})
There was a problem hiding this comment.
It works, but I can see how that can lead to confusion. I'll change it.
| @@ -0,0 +1,26 @@ | |||
| import { FC } from 'react' | |||
There was a problem hiding this comment.
Typo in filename MemeberOptionsMenu
| profileId, | ||
| removeParticipants: [removingParticipantId ?? profileId], | ||
| }, | ||
| connections: [ConnectionHandler.getConnectionID(roomId, 'ChatRoom_participants')], |
There was a problem hiding this comment.
Does this work if the connection does not exist (when you leave a group from the chat room header menu)? Then connections is probably [undefined] but the argument definition in relay is something like [ID!]! (i.e. array of non-zero ids).
If you know this works, just ignore the comment.
There was a problem hiding this comment.
Not sure if I understood what you meant by "the connection does not exist (when you leave a group from the chat room header menu)". The connection exists in the MembersList fragment, so it doesn't really matter from where you invoke the connection: relay will just update that store and, if the component that uses that fragment is mounted, that update will trigger a rerender.
|
|
||
| const LeaveGroupDialog: FC<LeaveGroupDialogProps> = ({ | ||
| title = 'Leave group chat?', | ||
| content = 'You will stop receiving messages from this conversation and people will see that you left.', |
There was a problem hiding this comment.
I see that this is the message from the designs, but probably the designs were written assuming that we do a soft delete? Given that we implemented a hard delete, I think a wording like 'You will no longer see nor receive messages from this conversion and people will see that you left.' would be more appropriate (at least the user should be warned that he can no longer access previous messages). So we should maybe double check whether this message is the intended one?
There was a problem hiding this comment.
Apparently this got approved, so I'm keeping as is
|
Is there a backend PR for this? Otherwise, I am afraid that only the admin can leave a group that way. (When I wrote the |
Tbh I overlooked that. Thanks for pointing it out, I'll check and see if any changes are required |
tsl-ps2
force-pushed
the
BA-2054-removing-a-member-frontend-packages
branch
from
e8abc6d to
85a6f18
Compare
pt-tsl
force-pushed
the
BA-2060-leave-group
branch
3 times, most recently
from
411d74c to
6569b22
Compare
pt-tsl
changed the base branch from
BA-2054-removing-a-member-frontend-packages
to
master
pt-tsl
force-pushed
the
BA-2060-leave-group
branch
from
6569b22 to
80b648a
Compare
![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 (1)
packages/components/modules/messages/GroupDetails/ProfileCard/AdminOptionsMenu/index.tsx (1)
Line range hint
19-23: Add type safety for admin-only actions.The "Remove" action should be properly typed to ensure it's only available to admins. Consider creating a discriminated union type to enforce this at compile time.
type AdminAction = { isAdmin: true; onToggleAdminClicked: VoidFunction; onRemoveClicked: VoidFunction; }; type NonAdminAction = { isAdmin: false; }; type AdminOptionsProps = { onViewProfileClicked: VoidFunction; } & (AdminAction | NonAdminAction);
🧹 Nitpick comments (9)
packages/components/modules/messages/ChatRoom/index.tsx (1)
Line range hint
37-37: Enhance error handling for chat room not found case.The current error handling shows a basic "Chat room not found" message. Consider enhancing this with a more user-friendly error state that includes:
- A clear explanation of what went wrong
- Possible actions the user can take
packages/components/modules/messages/__shared__/LeaveGroupDialog/index.tsx (1)
48-50: Enhance error message handling.The current implementation directly displays the error message from the backend. Consider mapping common error cases to user-friendly messages.
- sendToast(error.message, { type: 'error' }) + const userFriendlyMessage = mapErrorToUserMessage(error.message) + sendToast(userFriendlyMessage, { type: 'error' })Consider adding a helper function like:
const mapErrorToUserMessage = (error: string): string => { switch (error) { case 'UNAUTHORIZED': return 'You do not have permission to leave this group' case 'GROUP_NOT_FOUND': return 'This group chat no longer exists' default: return 'Unable to leave group. Please try again later.' } }packages/components/modules/messages/GroupDetails/ProfileCard/index.tsx (2)
50-50: Implement profile viewing functionality.There are multiple TODO comments indicating that the profile viewing functionality is not implemented.
Would you like me to help implement the profile viewing functionality or create an issue to track this task?
Also applies to: 60-60, 61-61
45-73: Simplify therenderMenuItemsfunction.The function has redundant logic. The last return statement is identical to the first condition but with different
isMevalue. This can be simplified.const renderMenuItems = () => { - if (isMe) { - return ( - <MemberOptionsMenu - isMe={isMe} - onViewProfileClicked={popover.onClose /* TODO: Add functionality */} - onRemoveClicked={handleRemoveClicked} - /> - ) - } - if (!isMe && hasAdminPermissions) { return ( <AdminOptionsMenu onViewProfileClicked={popover.onClose /* TODO: Add functionality */} onToggleAdminClicked={popover.onClose /* TODO: Add functionality */} onRemoveClicked={handleRemoveClicked} /> ) } - - return ( + return ( <MemberOptionsMenu isMe={isMe} onViewProfileClicked={popover.onClose /* TODO: Add functionality */} onRemoveClicked={handleRemoveClicked} /> ) }packages/components/modules/messages/ChatRoom/ChatRoomHeader/index.tsx (2)
102-102: Implement archive functionality.The
onArchiveClickedprop is passed an empty function.Would you like me to help implement the archive functionality or create an issue to track this task?
42-44: Remove redundant event propagation prevention.Event propagation is stopped in both the button click handler and the popover root props. This is redundant as stopping it in the button click handler is sufficient.
const onChatRoomOptionsClicked = (e: React.MouseEvent<HTMLButtonElement>) => { e.stopPropagation() popover.onOpen(e) } // ... <Popover open={popover.open} onClose={popover.onClose} - slotProps={{ - root: { onClick: (e: React.MouseEvent<HTMLDivElement>) => e.stopPropagation() }, - }} >Also applies to: 97-99
packages/components/modules/messages/GroupDetails/index.tsx (1)
117-117: Implement sole admin removal handling.The TODO comment indicates that sole admin removal functionality is pending implementation.
Would you like me to help implement the sole admin removal functionality or create an issue to track this task?
packages/components/schema.graphql (2)
99-113: Consider adding permission fields for group operations.The
ChatRoomtype should include fields to determine if the current user can perform group operations (like leaving). This would help frontend validation.Consider adding fields like:
type ChatRoom implements Node { # ... existing fields + canLeave(profileId: ID): Boolean }
222-227: Document role permissions for group operations.The
ChatRoomParticipantRolesenum lacks documentation about what operations each role can perform, including whether members can leave groups.Add descriptions to clarify permissions:
enum ChatRoomParticipantRoles { - """member""" + """Regular member who can send messages and leave the group""" MEMBER - """admin""" + """Administrator who can manage members and group settings""" ADMIN }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/components/modules/messages/ChatRoom/ChatRoomHeader/ChatRoomOptions/index.tsx(1 hunks)packages/components/modules/messages/ChatRoom/ChatRoomHeader/ChatRoomOptions/types.ts(1 hunks)packages/components/modules/messages/ChatRoom/ChatRoomHeader/index.tsx(1 hunks)packages/components/modules/messages/ChatRoom/ChatRoomHeader/styled.tsx(1 hunks)packages/components/modules/messages/ChatRoom/ChatRoomHeader/types.ts(1 hunks)packages/components/modules/messages/ChatRoom/index.tsx(1 hunks)packages/components/modules/messages/GroupDetails/GroupDetailsHeader/types.ts(1 hunks)packages/components/modules/messages/GroupDetails/ProfileCard/AdminOptionsMenu/index.tsx(1 hunks)packages/components/modules/messages/GroupDetails/ProfileCard/AdminOptionsMenu/types.ts(1 hunks)packages/components/modules/messages/GroupDetails/ProfileCard/MemberOptionsMenu/index.tsx(1 hunks)packages/components/modules/messages/GroupDetails/ProfileCard/MemberOptionsMenu/types.ts(1 hunks)packages/components/modules/messages/GroupDetails/ProfileCard/index.tsx(3 hunks)packages/components/modules/messages/GroupDetails/index.tsx(4 hunks)packages/components/modules/messages/__shared__/LeaveGroupDialog/index.tsx(1 hunks)packages/components/modules/messages/__shared__/LeaveGroupDialog/types.ts(1 hunks)packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx(1 hunks)packages/components/schema.graphql(9 hunks)packages/design-system/components/avatars/CircledAvatar/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/modules/messages/GroupDetails/GroupDetailsHeader/types.ts
🔇 Additional comments (12)
packages/components/modules/messages/__shared__/LeaveGroupDialog/types.ts (1)
1-10: 🛠️ Refactor suggestionSeparate leave and remove participant functionality
The interface is handling two distinct responsibilities: leaving a group and removing participants. This could lead to confusion and potential bugs. Consider splitting into two separate interfaces for better clarity and type safety.
-export interface LeaveGroupDialogProps { +export interface BaseDialogProps { title?: string content?: string - profileId?: string - roomId?: string - removingParticipantId?: string - removingParticipantName?: string | null open: boolean onClose: VoidFunction } +export interface LeaveGroupDialogProps extends BaseDialogProps { + profileId: string // Required when leaving + roomId: string // Required when leaving +} + +export interface RemoveParticipantDialogProps extends BaseDialogProps { + roomId: string // Required when removing + removingParticipantId: string // Required when removing + removingParticipantName?: string // Optional for display +}Also, consider adding validation for the backend integration:
✅ Verification successful
Remove unused participant removal properties
The interface suggests dual functionality for leaving groups and removing participants, but the codebase only implements group leaving. The participant removal properties (
removingParticipantIdandremovingParticipantName) are never used.export interface LeaveGroupDialogProps { title?: string content?: string profileId?: string roomId?: string - removingParticipantId?: string - removingParticipantName?: string | null open: boolean onClose: VoidFunction }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if backend supports member-initiated group leave rg -A 5 "mutation.*LeaveGroup|mutation.*RemoveMember" --type graphqlLength of output: 102
Script:
#!/bin/bash # Find the component implementation ast-grep --pattern 'const LeaveGroupDialog = ({ $$ }: LeaveGroupDialogProps) => { $$$ }' # Find usage patterns rg "LeaveGroupDialog" -B 2 -A 2Length of output: 3754
packages/design-system/components/avatars/CircledAvatar/types.ts (1)
8-8: Verify if this change is necessary for the leave group feature.Making
hasErroroptional seems unrelated to the PR's objective of implementing the leave group functionality. Please clarify the reason for this change or consider moving it to a separate PR.packages/components/modules/messages/GroupDetails/ProfileCard/MemberOptionsMenu/index.tsx (2)
16-22:⚠️ Potential issueAdd confirmation dialog for Leave Group action.
Per acceptance criteria, clicking "Leave Group" should show a confirmation prompt before proceeding. Currently, the action is executed immediately upon click.
17-18: Verify backend support for member leave action.Given the concern raised about backend support, verify that the
onRemoveClickedhandler integrates with an appropriate backend mutation that allows non-admin members to leave.packages/components/modules/messages/ChatRoom/ChatRoomHeader/ChatRoomOptions/index.tsx (2)
13-16: Address TODO comment about archive functionality.The TODO comment indicates that archive functionality is not implemented, yet we're exposing the menu item to users. Consider either implementing the archive functionality or removing/hiding the menu item until it's ready.
Would you like me to help implement the archive functionality or create an issue to track this task?
12-25: LGTM! Menu implementation meets acceptance criteria.The implementation satisfies the acceptance criteria by providing:
- Menu items for group actions
- "Leave Group" option with proper visual emphasis (red color)
- Clear, user-friendly labels
packages/components/modules/messages/ChatRoom/ChatRoomHeader/styled.tsx (1)
25-25: LGTM! Grid layout properly accommodates new menu button.The addition of a third min-content column provides the necessary space for the menu button while maintaining a responsive layout.
packages/components/modules/messages/ChatRoom/index.tsx (1)
Line range hint
28-28: Consider implementing the TODO for query preloading.Lazy loading the chat room query might impact initial load performance. Consider implementing the suggested preloading strategy.
Would you like me to help implement the query preloading or create an issue to track this task?
packages/components/modules/messages/__shared__/LeaveGroupDialog/index.tsx (2)
15-16: LGTM! Dialog content approved by design team.The dialog title and content were previously discussed and approved, despite concerns about hard delete implications.
31-36: Verify backend compatibility for leave group functionality.Based on the PR comments discussion about backend implementation, we need to verify that:
- The mutation allows non-admin members to leave the group
- The hard delete behavior is properly implemented on the backend
packages/components/schema.graphql (2)
188-190: Subscription type supports leave group updates.The
ChatRoomOnRoomUpdatesubscription includes the necessary fields to handle group leave events:
roomfor updated group detailsremovedParticipantsfor tracking who left
298-305:⚠️ Potential issueClarify if ChatRoomUpdate can be used for leaving groups.
The
ChatRoomUpdateInputtype allows removing participants, but it's not clear if:
- Members can remove themselves (leave)
- This mutation is restricted to admins only
This aligns with the concern raised in the PR comments about admin-only restrictions.
Consider either:
- Documenting that this mutation supports self-removal
- Adding a separate mutation for leaving groups:
input ChatRoomLeaveInput { roomId: ID! profileId: ID! clientMutationId: String } type ChatRoomLeavePayload { """May contain more than one error for same field.""" errors: [ErrorType] _debug: DjangoDebug room: ChatRoomEdge removedParticipant: ChatRoomParticipant clientMutationId: String }
| export interface MemberOptionsMenuProps { | ||
| isMe: boolean | ||
| onViewProfileClicked: VoidFunction | ||
| onRemoveClicked: VoidFunction | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding admin permission check
The interface should include an isAdmin property to properly handle permission-based menu options, especially since the PR comments mention concerns about admin-only functionality in the backend.
export interface MemberOptionsMenuProps {
isMe: boolean
+ isAdmin: boolean
onViewProfileClicked: VoidFunction
onRemoveClicked: VoidFunction
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface MemberOptionsMenuProps { | |
| isMe: boolean | |
| onViewProfileClicked: VoidFunction | |
| onRemoveClicked: VoidFunction | |
| } | |
| export interface MemberOptionsMenuProps { | |
| isMe: boolean | |
| isAdmin: boolean | |
| onViewProfileClicked: VoidFunction | |
| onRemoveClicked: VoidFunction | |
| } |
| export interface ChatRoomOptionsProps { | ||
| onArchiveClicked: () => void | ||
| onDetailsClicked: () => void | ||
| onLeaveClicked: () => void | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety for chat room types
The interface should include properties to distinguish between group and direct chats, as some options (like leaving) are only applicable to group chats. Also consider adding admin permission check.
export interface ChatRoomOptionsProps {
+ isGroupChat: boolean
+ isAdmin?: boolean
onArchiveClicked: () => void
onDetailsClicked: () => void
onLeaveClicked: () => void
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface ChatRoomOptionsProps { | |
| onArchiveClicked: () => void | |
| onDetailsClicked: () => void | |
| onLeaveClicked: () => void | |
| } | |
| export interface ChatRoomOptionsProps { | |
| isGroupChat: boolean | |
| isAdmin?: boolean | |
| onArchiveClicked: () => void | |
| onDetailsClicked: () => void | |
| onLeaveClicked: () => void | |
| } |
| export interface AdminOptionsProps { | ||
| isAdmin: boolean | ||
| isMe: boolean | ||
| onToggleAdminClicked: () => void | ||
| onRemoveClicked: () => void | ||
| onViewProfileClicked: VoidFunction | ||
| onToggleAdminClicked: VoidFunction | ||
| onRemoveClicked: VoidFunction |
There was a problem hiding this comment.
Reconsider removal of permission flags
- The removal of
isAdminandisMeflags could lead to incorrect access control. These flags are crucial for conditional rendering and permission checks. - The
onRemoveClickedfunction name is ambiguous - it could mean either removing a member or leaving the group.
export interface AdminOptionsProps {
+ isAdmin: boolean
+ isMe: boolean
onViewProfileClicked: VoidFunction
onToggleAdminClicked: VoidFunction
- onRemoveClicked: VoidFunction
+ onRemoveMemberClicked: VoidFunction
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface AdminOptionsProps { | |
| isAdmin: boolean | |
| isMe: boolean | |
| onToggleAdminClicked: () => void | |
| onRemoveClicked: () => void | |
| onViewProfileClicked: VoidFunction | |
| onToggleAdminClicked: VoidFunction | |
| onRemoveClicked: VoidFunction | |
| export interface AdminOptionsProps { | |
| isAdmin: boolean | |
| isMe: boolean | |
| onViewProfileClicked: VoidFunction | |
| onToggleAdminClicked: VoidFunction | |
| onRemoveMemberClicked: VoidFunction | |
| } |
| onDisplayGroupDetailsClicked: VoidFunction | ||
| roomId?: string |
There was a problem hiding this comment.
Make roomId required for leave group functionality.
Since the room ID is essential for the leave group feature, this property should be required rather than optional. This ensures type safety and prevents potential runtime errors when implementing the leave group functionality.
- roomId?: string
+ roomId: string📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onDisplayGroupDetailsClicked: VoidFunction | |
| roomId?: string | |
| onDisplayGroupDetailsClicked: VoidFunction | |
| roomId: string |
| <MenuItem onClick={onToggleAdminClicked}> | ||
| <Typography variant="body2">Make Admin</Typography> | ||
| </MenuItem> |
There was a problem hiding this comment.
Restore permission checks for admin actions.
The removal of isAdmin checks could lead to permission issues. The "Make Admin" option should only be available to admins.
- <MenuItem onClick={onToggleAdminClicked}>
+ {isAdmin && (
+ <MenuItem onClick={onToggleAdminClicked}>
<Typography variant="body2">Make Admin</Typography>
- </MenuItem>
+ </MenuItem>
+ )}Committable suggestion skipped: line range outside the PR's diff.
| onRemoval?: VoidFunction | ||
| isRemoval?: boolean |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused parameter isRemoval.
The isRemoval parameter is declared but never used in the code.
const useRoomListSubscription = ({
connections,
profileId,
- onRemoval?: VoidFunction
- isRemoval?: boolean
+ onRemoval?: VoidFunction
}) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onRemoval?: VoidFunction | |
| isRemoval?: boolean | |
| onRemoval?: VoidFunction |
| @@ -38,13 +36,14 @@ const GroupDetails: FC<GroupDetailsProps> = ({ | |||
| const { currentProfile } = useCurrentProfile() | |||
| const { avatar, title } = useGroupNameAndAvatar(group) | |||
| const theme = useTheme() | |||
| const profileId = currentProfile?.id ?? '' | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve handling of undefined profileId.
Using an empty string as a fallback for undefined profileId might lead to unexpected behavior. Consider handling the undefined case explicitly.
-const profileId = currentProfile?.id ?? ''
+const profileId = currentProfile?.id
const connections = group?.id
? [ConnectionHandler.getConnectionID(group.id, 'ChatRoom_participants')]
: []
-// TODO: Is there a safer way to ensure the current profile id is not undefined?
useRoomListSubscription({
- profileId,
+ profileId: profileId ?? '',
connections,
onRemoval: onBackButtonClicked,
})Also applies to: 44-48
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/components/modules/messages/__shared__/LeaveGroupDialog/constants.ts (1)
25-26: 🛠️ Refactor suggestionUpdate member leave message to reflect hard deletion.
Since a hard delete will be performed when a user leaves the group, the content should warn users that they will lose access to previous messages.
- 'You will stop receiving messages from this conversation and people will see that you left.', + 'You will lose access to all messages from this conversation and people will see that you left. This action cannot be undone.',
🧹 Nitpick comments (8)
packages/components/modules/messages/graphql/fragments/RoomTitle.ts (1)
Line range hint
1-21: Consider documenting the possible role values.To improve maintainability, consider adding a comment documenting the possible values for the
rolefield and their implications for the leave group functionality.import { graphql } from 'react-relay' +// Possible role values: +// - ADMIN: Can manage group members and settings +// - MEMBER: Can participate and leave the group // non-group chat rooms have the avatar and name of the other participant as title export const RoomTitleFragment = graphql` fragment RoomTitleFragment on ChatRoom {packages/components/modules/messages/GroupDetails/ProfileCard/index.tsx (2)
38-38: Consider using a more descriptive variable name.While
isMeworks, a more descriptive name likeisCurrentUserorisOwnProfilewould better convey its purpose.- const isMe = currentProfile?.id === id + const isCurrentUser = currentProfile?.id === id
45-73: Refactor renderMenuItems to reduce duplication and improve readability.The current implementation:
- Duplicates the MemberOptionsMenu rendering logic
- Uses nested if statements where early returns would be clearer
Consider this refactored version:
const renderMenuItems = () => { if (isMe) { return ( <MemberOptionsMenu isMe={isMe} onViewProfileClicked={popover.onClose /* TODO: Add functionality */} onRemoveClicked={handleRemoveClicked} /> ) } - if (!isMe && hasAdminPermissions) { + if (hasAdminPermissions) { return ( <AdminOptionsMenu onViewProfileClicked={popover.onClose /* TODO: Add functionality */} onToggleAdminClicked={popover.onClose /* TODO: Add functionality */} onRemoveClicked={handleRemoveClicked} /> ) } - return ( - <MemberOptionsMenu - isMe={isMe} - onViewProfileClicked={popover.onClose /* TODO: Add functionality */} - onRemoveClicked={handleRemoveClicked} - /> - ) + return <MemberOptionsMenu + isMe={false} + onViewProfileClicked={popover.onClose /* TODO: Add functionality */} + onRemoveClicked={handleRemoveClicked} + /> }packages/components/modules/messages/utils.ts (1)
14-20: Add type guard for headerRef casting.The type casting of headerRef might fail at runtime if the wrong type is passed.
export const useGroupNameAndAvatar = ( headerRef: GroupTitleFragment$key | RoomTitleFragment$key | null | undefined, ) => { + if (!headerRef || !('__fragmentType' in headerRef)) { + return { title: undefined, avatar: undefined } + } + if (headerRef.__fragmentType !== 'GroupTitleFragment') { + throw new Error('Invalid fragment type passed to useGroupNameAndAvatar') + } const header = useFragment<GroupTitleFragment$key>( GroupTitleFragment, - headerRef as GroupTitleFragment$key, + headerRef, ) return { title: header?.title, avatar: header?.image?.url, } }packages/components/modules/messages/__shared__/LeaveGroupDialog/types.ts (1)
1-10: Add JSDoc documentation and make roomId required.The interface lacks documentation and roomId is used as required in the implementation.
+/** + * Props for the LeaveGroupDialog component + * @property customContent - Optional custom content text for the dialog + * @property customTitle - Optional custom title text for the dialog + * @property isSoleAdmin - Whether the current user is the only admin + * @property onClose - Callback function when dialog is closed + * @property open - Whether the dialog is open + * @property profileId - ID of the current user's profile + * @property removingParticipantId - ID of the participant being removed + * @property roomId - ID of the chat room + */ export interface LeaveGroupDialogProps { customContent?: string customTitle?: string isSoleAdmin?: boolean onClose: VoidFunction open: boolean profileId: string removingParticipantId: string - roomId?: string + roomId: string }packages/components/modules/messages/__shared__/LeaveGroupDialog/index.tsx (1)
78-99: Add loading state to dialog content.The dialog content should reflect the loading state during mutation.
return ( <ConfirmDialog title={ customTitle ?? getLeaveGroupDialogTextCopy(LEAVE_GROUP_DIALOG_TEXT_COPY_TYPE_KEYS.TITLE) } content={ + isMutationInFlight ? ( + 'Processing your request...' + ) : ( customContent ?? getLeaveGroupDialogTextCopy(LEAVE_GROUP_DIALOG_TEXT_COPY_TYPE_KEYS.CONTENT) + ) } action={ <LoadingButton color="error" onClick={onRemoveConfirmed} disabled={isMutationInFlight} loading={isMutationInFlight} > {removingParticipantId === profileId ? 'Leave group' : 'Remove'} </LoadingButton> } onClose={onClose} open={open} /> )packages/components/modules/messages/ChatRoom/ChatRoomHeader/index.tsx (1)
52-118: Consider accessibility improvements.While the UI implementation is solid, consider these accessibility enhancements:
- Add aria-expanded to the options button
- Add role="menu" to the Popover content
- Add aria-haspopup="menu" to the options button
<IconButton onClick={onChatRoomOptionsClicked} aria-label="Show chatroom options" + aria-expanded={popover.open} + aria-haspopup="menu" > <ThreeDotsIcon sx={{ fontSize: '24px' }} /> </IconButton> <Popover open={popover.open} onClose={popover.onClose} slotProps={{ root: { onClick: (e: React.MouseEvent<HTMLDivElement>) => e.stopPropagation() }, }} + role="menu" >packages/components/modules/messages/GroupDetails/index.tsx (1)
115-122: Add error handling for leave group operation.Consider adding error handling and user feedback for failed leave group operations.
<LeaveGroupDialog profileId={profileId} roomId={group?.id} open={!!removingParticipantId} removingParticipantId={removingParticipantId ?? ''} onClose={handleRemoveDialogClose} isSoleAdmin={isSoleAdmin} + onError={(error) => { + // Show error notification + handleRemoveDialogClose() + }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 80b648a and d1959ab75c2406342c96a26bb8bf12ea73e524fe.
⛔ 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 (8)
packages/components/modules/messages/ChatRoom/ChatRoomHeader/index.tsx(1 hunks)packages/components/modules/messages/GroupDetails/ProfileCard/index.tsx(3 hunks)packages/components/modules/messages/GroupDetails/index.tsx(4 hunks)packages/components/modules/messages/__shared__/LeaveGroupDialog/constants.ts(1 hunks)packages/components/modules/messages/__shared__/LeaveGroupDialog/index.tsx(1 hunks)packages/components/modules/messages/__shared__/LeaveGroupDialog/types.ts(1 hunks)packages/components/modules/messages/graphql/fragments/RoomTitle.ts(1 hunks)packages/components/modules/messages/utils.ts(3 hunks)
🔇 Additional comments (10)
packages/components/modules/messages/graphql/fragments/RoomTitle.ts (1)
17-17: LGTM! The role field addition enables role-based permissions.The addition of the
rolefield is essential for implementing the leave group functionality with proper permission checks.Let's verify that the
rolefield exists in the GraphQL schema and document its possible values:✅ Verification successful
✅ Role field is properly defined and typed
The
rolefield is correctly implemented:
- Uses
ChatRoomParticipantRolesenum (ADMIN, MEMBER)- Properly integrated in the GraphQL schema
- Correctly typed in generated code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the role field in the GraphQL schema # Expected: The role field should be defined in the schema with enum values # Search for role field definition in schema files fd -e graphql -e gql . | xargs rg -A 5 'type.*Profile|enum.*Role|field.*role'Length of output: 2122
Script:
#!/bin/bash # Find the type definition and usage of RoomTitle fragment ast-grep --pattern 'fragment RoomTitle on $_' # Find where this fragment is used rg -l 'RoomTitle' --type tsLength of output: 840
Script:
#!/bin/bash # Check the generated type definition cat packages/components/__generated__/RoomTitleFragment.graphql.tsLength of output: 4958
packages/components/modules/messages/GroupDetails/ProfileCard/index.tsx (4)
18-18: LGTM! Clean import additions.The new imports are well-organized and necessary for the leave group functionality.
Also applies to: 20-20
49-51: Address TODO comments before merging.Multiple TODO comments indicate incomplete functionality for profile viewing. This should be implemented before merging to ensure complete feature functionality.
Please confirm if the profile viewing functionality is planned for a separate PR or should be implemented as part of this one.
Also applies to: 59-61, 69-70
118-118: LGTM! Clean menu rendering implementation.The use of renderMenuItems centralizes the menu rendering logic and makes the JSX cleaner.
48-52: Verify integration with leave group functionality.This component seems to be part of the leave group chat implementation, but we should verify:
- That MemberOptionsMenu includes the "Leave Group" option as specified in the PR objectives
- That the confirmation dialog is properly integrated
- That the backend API call is properly handled
Let's check the MemberOptionsMenu implementation:
✅ Verification successful
Implementation verified and properly integrated
The leave group functionality is correctly implemented with:
- Proper UI integration through MemberOptionsMenu
- State management and confirmation dialog in GroupDetails
- GraphQL integration for the actual removal
- Proper handling of admin roles and edge cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check MemberOptionsMenu implementation for leave group functionality rg -A 10 "Leave Group" packages/components/modules/messages/GroupDetails/ProfileCard/MemberOptionsMenu/Length of output: 944
Script:
#!/bin/bash # Get full MemberOptionsMenu implementation cat packages/components/modules/messages/GroupDetails/ProfileCard/MemberOptionsMenu/index.tsx # Check for API integration rg -g "*.ts*" -A 5 "leaveGroup|removeFromGroup" packages/Length of output: 776
Script:
#!/bin/bash # Check parent component implementation cat packages/components/modules/messages/GroupDetails/ProfileCard/index.tsx # Look for API/service layer implementation rg -g "*.ts*" "removeFromGroup|leaveGroup" packages/services/Length of output: 3993
Script:
#!/bin/bash # Look for initiateRemoval implementation in parent components rg -g "*.tsx" -B 5 "initiateRemoval=" packages/components/modules/messages/GroupDetails/Length of output: 634
Script:
#!/bin/bash # Get full GroupDetails component implementation cat packages/components/modules/messages/GroupDetails/index.tsx # Look for any mutation/API calls related to group removal rg -g "*.ts*" -A 5 "mutation.*RemoveFromGroup|mutation.*LeaveGroup" packages/Length of output: 6353
packages/components/modules/messages/ChatRoom/ChatRoomHeader/index.tsx (3)
1-23: LGTM! Clean import organization and state management.The imports and state management are well-organized and appropriate for the leave group chat feature implementation.
46-49: LGTM! Clean event handling.The event handler correctly prevents event propagation and follows React best practices.
27-42: Consider explicit handling of undefined profileId.Using an empty string as a fallback for
currentProfile?.idmight lead to unexpected behavior. Consider handling the undefined case explicitly or adding validation.-const { currentProfile } = useCurrentProfile() +const { currentProfile } = useCurrentProfile() +if (!currentProfile?.id) { + console.warn('User profile ID is undefined') +}packages/components/modules/messages/GroupDetails/index.tsx (2)
38-48: Consistent handling of profileId needed across components.Similar to ChatRoomHeader, this component uses empty string fallback for undefined profileId. Consider standardizing the approach across components.
114-122: Address TODO comment regarding sole admin removal.The TODO comment indicates that sole admin removal logic will be handled in another story. This is a critical feature that needs to be implemented to prevent groups from being left without an admin.
Would you like me to help design the sole admin removal logic or create an issue to track this requirement?
| export const useCheckIsAdmin = (participants: any) => { | ||
| const { currentProfile } = useCurrentProfile() | ||
| const me = participants?.edges?.find( | ||
| (edge: any) => 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, | ||
| ).length === 1 | ||
| return { isAdmin, isSoleAdmin } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety in useCheckIsAdmin hook.
The function uses 'any' type and lacks proper type checking for undefined values.
-export const useCheckIsAdmin = (participants: any) => {
+import { ParticipantsConnectionFragment$data } from '../../__generated__/ParticipantsConnectionFragment.graphql'
+
+export const useCheckIsAdmin = (participants: ParticipantsConnectionFragment$data | null) => {
const { currentProfile } = useCurrentProfile()
- const me = participants?.edges?.find(
- (edge: any) => edge?.node?.profile?.id === currentProfile?.id,
+ const me = participants?.edges?.find((edge) =>
+ edge?.node?.profile?.id ? edge.node.profile.id === currentProfile?.id : false,
)
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,
- ).length === 1
+ (participants?.edges?.filter((edge) =>
+ edge?.node?.role ? edge.node.role === CHAT_ROOM_PARTICIPANT_ROLES.admin : false,
+ )?.length ?? 0) === 1
return { isAdmin, isSoleAdmin }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const useCheckIsAdmin = (participants: any) => { | |
| const { currentProfile } = useCurrentProfile() | |
| const me = participants?.edges?.find( | |
| (edge: any) => 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, | |
| ).length === 1 | |
| return { isAdmin, isSoleAdmin } | |
| import { ParticipantsConnectionFragment$data } from '../../__generated__/ParticipantsConnectionFragment.graphql' | |
| export const useCheckIsAdmin = (participants: ParticipantsConnectionFragment$data | null) => { | |
| const { currentProfile } = useCurrentProfile() | |
| const me = participants?.edges?.find((edge) => | |
| edge?.node?.profile?.id ? edge.node.profile.id === currentProfile?.id : false, | |
| ) | |
| const isAdmin = me?.node?.role === CHAT_ROOM_PARTICIPANT_ROLES.admin | |
| const isSoleAdmin = | |
| isAdmin && | |
| (participants?.edges?.filter((edge) => | |
| edge?.node?.role ? edge.node.role === CHAT_ROOM_PARTICIPANT_ROLES.admin : false, | |
| )?.length ?? 0) === 1 | |
| return { isAdmin, isSoleAdmin } | |
| } |
| const onRemoveConfirmed = () => { | ||
| if (!roomId || !profileId) return | ||
| commit({ | ||
| variables: { | ||
| input: { | ||
| roomId, | ||
| profileId, | ||
| removeParticipants: [removingParticipantId], | ||
| }, | ||
| connections: [ConnectionHandler.getConnectionID(roomId, 'ChatRoom_participants')], | ||
| }, | ||
| onCompleted: (response) => { | ||
| if ( | ||
| removingParticipantId && | ||
| removingParticipantId !== profileId && | ||
| !response?.chatRoomUpdate?.errors | ||
| ) { | ||
| sendToast('Member was successfully removed') | ||
| } | ||
| onClose() | ||
| }, | ||
| onError: (error) => { | ||
| sendToast(error.message, { type: 'error' }) | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and user feedback.
The function lacks proper error handling and consistent user feedback.
const onRemoveConfirmed = () => {
- if (!roomId || !profileId) return
+ if (!roomId || !profileId) {
+ sendToast('Missing required data', { type: 'error' })
+ return
+ }
+
+ const connectionId = ConnectionHandler.getConnectionID(roomId, 'ChatRoom_participants')
+ if (!connectionId) {
+ sendToast('Failed to update chat room', { type: 'error' })
+ return
+ }
+
commit({
variables: {
input: {
roomId,
profileId,
removeParticipants: [removingParticipantId],
},
- connections: [ConnectionHandler.getConnectionID(roomId, 'ChatRoom_participants')],
+ connections: [connectionId],
},
onCompleted: (response) => {
+ const hasErrors = response?.chatRoomUpdate?.errors
if (
removingParticipantId &&
removingParticipantId !== profileId &&
- !response?.chatRoomUpdate?.errors
+ !hasErrors
) {
sendToast('Member was successfully removed')
+ } else if (!hasErrors) {
+ sendToast('You have left the group')
}
onClose()
},
onError: (error) => {
sendToast(error.message, { type: 'error' })
+ onClose()
},
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onRemoveConfirmed = () => { | |
| if (!roomId || !profileId) return | |
| commit({ | |
| variables: { | |
| input: { | |
| roomId, | |
| profileId, | |
| removeParticipants: [removingParticipantId], | |
| }, | |
| connections: [ConnectionHandler.getConnectionID(roomId, 'ChatRoom_participants')], | |
| }, | |
| onCompleted: (response) => { | |
| if ( | |
| removingParticipantId && | |
| removingParticipantId !== profileId && | |
| !response?.chatRoomUpdate?.errors | |
| ) { | |
| sendToast('Member was successfully removed') | |
| } | |
| onClose() | |
| }, | |
| onError: (error) => { | |
| sendToast(error.message, { type: 'error' }) | |
| }, | |
| }) | |
| } | |
| const onRemoveConfirmed = () => { | |
| if (!roomId || !profileId) { | |
| sendToast('Missing required data', { type: 'error' }) | |
| return | |
| } | |
| const connectionId = ConnectionHandler.getConnectionID(roomId, 'ChatRoom_participants') | |
| if (!connectionId) { | |
| sendToast('Failed to update chat room', { type: 'error' }) | |
| return | |
| } | |
| commit({ | |
| variables: { | |
| input: { | |
| roomId, | |
| profileId, | |
| removeParticipants: [removingParticipantId], | |
| }, | |
| connections: [connectionId], | |
| }, | |
| onCompleted: (response) => { | |
| const hasErrors = response?.chatRoomUpdate?.errors | |
| if ( | |
| removingParticipantId && | |
| removingParticipantId !== profileId && | |
| !hasErrors | |
| ) { | |
| sendToast('Member was successfully removed') | |
| } else if (!hasErrors) { | |
| sendToast('You have left the group') | |
| } | |
| onClose() | |
| }, | |
| onError: (error) => { | |
| sendToast(error.message, { type: 'error' }) | |
| onClose() | |
| }, | |
| }) | |
| } |
pt-tsl
force-pushed
the
BA-2060-leave-group
branch
from
d1959ab to
853a950
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/components/modules/messages/ChatRoom/ChatRoomHeader/index.tsx (1)
94-100: Simplify event propagation handling.The event handling in Popover can be simplified by using the
onClickprop directly.<Popover open={popover.open} onClose={popover.onClose} - slotProps={{ - root: { onClick: (e: React.MouseEvent<HTMLDivElement>) => e.stopPropagation() }, - }} + onClick={(e) => e.stopPropagation()} >packages/components/modules/messages/GroupDetails/index.tsx (1)
117-117: Track TODO for sole admin removal handling.The TODO comment indicates missing functionality for handling sole admin removal scenarios.
Would you like me to create an issue to track this TODO? I can help implement the sole admin removal logic in a separate PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d1959ab75c2406342c96a26bb8bf12ea73e524fe and 853a950.
📒 Files selected for processing (18)
packages/components/modules/messages/ChatRoom/ChatRoomHeader/ChatRoomOptions/index.tsx(1 hunks)packages/components/modules/messages/ChatRoom/ChatRoomHeader/ChatRoomOptions/types.ts(1 hunks)packages/components/modules/messages/ChatRoom/ChatRoomHeader/index.tsx(1 hunks)packages/components/modules/messages/ChatRoom/ChatRoomHeader/styled.tsx(1 hunks)packages/components/modules/messages/ChatRoom/ChatRoomHeader/types.ts(1 hunks)packages/components/modules/messages/ChatRoom/index.tsx(1 hunks)packages/components/modules/messages/GroupDetails/GroupDetailsHeader/types.ts(1 hunks)packages/components/modules/messages/GroupDetails/ProfileCard/AdminOptionsMenu/index.tsx(1 hunks)packages/components/modules/messages/GroupDetails/ProfileCard/AdminOptionsMenu/types.ts(1 hunks)packages/components/modules/messages/GroupDetails/ProfileCard/MemberOptionsMenu/index.tsx(1 hunks)packages/components/modules/messages/GroupDetails/ProfileCard/MemberOptionsMenu/types.ts(1 hunks)packages/components/modules/messages/GroupDetails/ProfileCard/index.tsx(3 hunks)packages/components/modules/messages/GroupDetails/index.tsx(4 hunks)packages/components/modules/messages/__shared__/LeaveGroupDialog/index.tsx(1 hunks)packages/components/modules/messages/__shared__/LeaveGroupDialog/types.ts(1 hunks)packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx(1 hunks)packages/components/schema.graphql(8 hunks)packages/design-system/components/avatars/CircledAvatar/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/components/modules/messages/ChatRoom/ChatRoomHeader/ChatRoomOptions/types.ts
- packages/components/modules/messages/GroupDetails/ProfileCard/MemberOptionsMenu/types.ts
- packages/design-system/components/avatars/CircledAvatar/types.ts
- packages/components/modules/messages/ChatRoom/ChatRoomHeader/ChatRoomOptions/index.tsx
- packages/components/modules/messages/ChatRoom/ChatRoomHeader/styled.tsx
- packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx
- packages/components/modules/messages/GroupDetails/ProfileCard/AdminOptionsMenu/index.tsx
- packages/components/modules/messages/GroupDetails/ProfileCard/MemberOptionsMenu/index.tsx
- packages/components/modules/messages/ChatRoom/ChatRoomHeader/types.ts
- packages/components/modules/messages/ChatRoom/index.tsx
- packages/components/modules/messages/GroupDetails/ProfileCard/index.tsx
- packages/components/modules/messages/GroupDetails/GroupDetailsHeader/types.ts
- packages/components/modules/messages/GroupDetails/ProfileCard/AdminOptionsMenu/types.ts
🔇 Additional comments (8)
packages/components/modules/messages/__shared__/LeaveGroupDialog/types.ts (1)
1-10: LGTM! Well-structured interface definition.The
LeaveGroupDialogPropsinterface is well-designed with:
- Clear separation between optional and required props
- Proper typing for all fields
- Support for both leaving and removing participants from a group
packages/components/modules/messages/__shared__/LeaveGroupDialog/index.tsx (3)
14-24: LGTM! Props handling and mutation setup.The component is well-structured with proper hooks usage and prop handling.
54-71: LGTM! Clean UI implementation.The dialog UI is well-implemented with:
- Proper conditional rendering
- Loading state handling
- Clear action buttons
27-52: 🛠️ Refactor suggestionEnhance error handling and user feedback.
The mutation handling needs improvements:
- Missing success toast when leaving group
- No error handling for missing connection ID
- No cleanup on error
Apply this diff to improve the implementation:
const onRemoveConfirmed = () => { - if (!roomId || !profileId) return + if (!roomId || !profileId) { + sendToast('Missing required data', { type: 'error' }) + return + } + + const connectionId = ConnectionHandler.getConnectionID(roomId, 'ChatRoom_participants') + if (!connectionId) { + sendToast('Failed to update chat room', { type: 'error' }) + return + } + commit({ variables: { input: { roomId, profileId, removeParticipants: [removingParticipantId ?? profileId], }, - connections: [ConnectionHandler.getConnectionID(roomId, 'ChatRoom_participants')], + connections: [connectionId], }, onCompleted: (response) => { + const hasErrors = response?.chatRoomUpdate?.errors if ( removingParticipantId && removingParticipantId !== profileId && - !response?.chatRoomUpdate?.errors + !hasErrors ) { sendToast(`${removingParticipantName} was successfully removed`) + } else if (!hasErrors) { + sendToast('You have left the group') } onClose() }, onError: (error) => { sendToast(error.message, { type: 'error' }) + onClose() }, }) }Likely invalid or redundant comment.
packages/components/modules/messages/ChatRoom/ChatRoomHeader/index.tsx (2)
49-54: LGTM! Clean LeaveGroupDialog integration.The LeaveGroupDialog is properly integrated with required props and state handling.
69-89: LGTM! Well-structured chat title container.The chat title container is well-implemented with:
- Proper conditional click handling
- Clear visual hierarchy
- Responsive layout
packages/components/modules/messages/GroupDetails/index.tsx (1)
118-135: LGTM! Well-implemented member removal dialog.The LeaveGroupDialog integration is well-implemented with:
- Proper conditional content
- Clear user feedback
- Clean state management
packages/components/schema.graphql (1)
12-16: Schema changes appear unrelated to leave group feature.These changes to the ActivityLog type and related queries seem unrelated to the current PR's objectives of implementing the leave group chat feature.
Also applies to: 355-355, 732-732, 1145-1161, 1194-1194, 1352-1352, 1763-1763
Acceptance Criteria
Leave group - As a Member
We are going to do a hard delete for that Group Chat for that user.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes