Conversation
rf2tsl
requested review from
anicioalexandre and
priscilladeroode
as code owners
|
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThreaded a new callback prop Changes
Sequence DiagramsequenceDiagram
participant User as User
participant MI as MemberItem
participant GP as GroupDetailsPage
participant LGD as LeaveGroupDialog
User->>MI: Click "Remove" on member
MI->>GP: setMemberToRemoveId(memberId)
GP->>GP: set memberToRemoveId state
Note over GP,LGD: LeaveGroupDialog opens with removingParticipantId = memberToRemoveId ?? currentProfile.id
User->>LGD: Confirm removal
LGD->>GP: onClose()
GP->>GP: clear memberToRemoveId
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/components/modules/messages/native/__shared__/LeaveGroupDialog/index.tsx (1)
30-33:⚠️ Potential issue | 🔴 CriticalBug: Admin is redirected away after removing another member.
handleLeaveGroupSuccessalways callsrouter.replace('/messages'), but when an admin removes someone else, they should stay on the group details page — only self-removal ("Leave Group") warrants navigating away.Proposed fix
const handleLeaveGroupSuccess = () => { onClose() - router.replace('/messages') + if (profileId === removingParticipantId) { + router.replace('/messages') + } }
🤖 Fix all issues with AI agents
In `@packages/components/modules/messages/native/GroupDetailsPage/index.tsx`:
- Line 48: The effect in GroupDetailsPage references openConfirmLeaveGroupDialog
but only lists memberToRemoveId in its dependency array, causing a hooks
dependency lint error; fix by including openConfirmLeaveGroupDialog in the
dependency array of the useEffect that closes over memberToRemoveId (or refactor
by memoizing openConfirmLeaveGroupDialog with useCallback and then add that
callback to the dependency array), so the effect's dependencies accurately
reflect the values it reads.
- Around line 44-48: Remove the useEffect that opens the confirm dialog and
replace it with an explicit handler (e.g., handleSetMemberToRemove) that calls
setMemberToRemoveId(id) and then setOpenConfirmLeaveGroupDialog(true); update
anywhere you pass setMemberToRemoveId (notably the Members component) to pass
handleSetMemberToRemove instead; keep onClose clearing memberToRemoveId and
closing the dialog as-is so retries of the same ID will reopen correctly and
avoid the stale-same-value edge case caused by effect-driven logic.
In
`@packages/components/modules/messages/native/GroupDetailsPage/Members/MemberItem/index.tsx`:
- Around line 135-137: The remove-member handler currently only calls
setMemberToRemoveId(profile.id) and does not close the bottom sheet, causing the
sheet to remain visible behind the confirmation dialog; update the handler used
in MemberItem (handleRemoveMember) to also call bottomDrawerRef.current?.close()
before setting the member ID (mirroring handleToggleAdminClicked behavior) so
the bottom drawer is dismissed prior to opening the LeaveGroupDialog/modal.
packages/components/modules/messages/native/GroupDetailsPage/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/native/GroupDetailsPage/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/native/GroupDetailsPage/Members/MemberItem/index.tsx
Show resolved
Hide resolved
lokmanliton
left a comment
lokmanliton
left a comment
There was a problem hiding this comment.
Copy and modal title is not same as design,
Here in design it says - Remove group member? but in implementation, it is Remove from chat?
|
lokmanliton
left a comment
lokmanliton
left a comment
There was a problem hiding this comment.
Got a minor UX issue when I close remove group modal, It is not animated like other modals as per video.
https://www.loom.com/share/0cb2ef6603bb44079c9c1ea445b5d401
BTW, I am approving the pr.
3 participants
Acceptance Criteria
Removing a Member
Given I am an admin of the group, when I select a member from the group details view, then I should have the option to remove that member from the group.
We should have already implemented the Remove Member option button in the Admin Permission story, but not the functionality.
Given I click on remove member, I should see a confirmation dialog, to make sure I want to do this action.
Given I confirm the removal of the member, then the removed member should no longer see the group in their chat list or have access to its messages.
We are doing a hard delete of the group chat for that user.
It should be implemented the same behavior as the web version.
Design Link: https://www.figma.com/design/z5ZdVlKEMmGIX4GVnjwfxA/BaseApp---NATIVE?node-id=3324-255017&t=TlbSP8vPOBBG6fwE-0
Approvd
https://app.approvd.io/silverlogic/BA/stories/41475
Summary by CodeRabbit
New Features
Bug Fixes
Other