BA-2226 Three dot menu cleanup #210
Conversation
tsl-ps2
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughThe pull request enhances the chat room archiving functionality by adding an Changes
Possibly related PRs
Suggested reviewers
Poem
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/common/graphql/queries/ChatRoomQuery.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/ChatRoomOptions/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/messages/common/graphql/mutations/ArchiveChatRoom.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx (1)
83-95: Maintain consistent error handling across components.Apply the same error handling pattern used in ChatRoomHeader for consistency.
- if (currentProfile?.id) { - commit({ - variables: { - input: { - roomId: roomRef.id, - profileId: currentProfile.id, - archive: !isInArchivedTab, - }, - }, - }) + if (!currentProfile?.id) { + sendToast('Unable to archive chat room. Please try again.', { type: 'error' }) + return } + commit({ + variables: { + input: { + roomId: roomRef.id, + profileId: currentProfile.id, + archive: !isInArchivedTab, + }, + }, + onCompleted: () => { + sendToast(`Chat room ${!isInArchivedTab ? 'archived' : 'unarchived'} successfully`, { + type: 'success', + }) + }, + })
🧹 Nitpick comments (2)
packages/components/modules/messages/ChatRoom/index.tsx (1)
46-46: Consider using explicit Boolean conversion.Instead of using double negation (
!!), consider usingBoolean(chatRoom.isArchived)for better readability.- isArchived={!!chatRoom.isArchived} + isArchived={Boolean(chatRoom.isArchived)}packages/components/modules/messages/graphql/mutations/ArchiveChatRoom.ts (1)
47-59: Enhance updater function robustness.The updater function could benefit from early returns and more explicit error handling.
updater: (store, data) => { - if (!data?.chatRoomArchive?.errors && currentProfile?.id) { + if (data?.chatRoomArchive?.errors || !currentProfile?.id) { + config?.updater?.(store, data) + return + } + + const roomId = data?.chatRoomArchive?.room?.id + if (!roomId) { + config?.updater?.(store, data) + return + } + getChatRoomConnections( store, currentProfile.id, ({ archived }) => archived === !data?.chatRoomArchive?.room?.isArchived, ).forEach((connectionRecord) => { - if (data?.chatRoomArchive?.room?.id) - ConnectionHandler.deleteNode(connectionRecord, data?.chatRoomArchive?.room?.id) + ConnectionHandler.deleteNode(connectionRecord, roomId) }) - } config?.updater?.(store, data) },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
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__/**packages/components/__generated__/ChatRoomQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (8)
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(5 hunks)packages/components/modules/messages/ChatRoom/ChatRoomHeader/types.ts(1 hunks)packages/components/modules/messages/ChatRoom/index.tsx(1 hunks)packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx(2 hunks)packages/components/modules/messages/graphql/mutations/ArchiveChatRoom.ts(3 hunks)packages/components/modules/messages/graphql/queries/ChatRoomQuery.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/components/modules/messages/ChatRoom/ChatRoomHeader/ChatRoomOptions/types.ts (1)
2-4: LGTM! Props align well with PR objectives.The new boolean props effectively support the required functionality:
isArchivedfor archive/unarchive stateisArchiveMutationInFlightfor mutation state handlingisGroupfor conditional rendering of group optionspackages/components/modules/messages/graphql/queries/ChatRoomQuery.ts (1)
7-7: LGTM! Query updated to support archive functionality.The
isArchivedfield is correctly added to fetch the chat room's archived state.packages/components/modules/messages/ChatRoom/ChatRoomHeader/types.ts (1)
6-6: LGTM! Header props updated for archive support.The
isArchivedboolean prop is correctly added to support archive state in the header.packages/components/modules/messages/ChatRoom/ChatRoomHeader/ChatRoomOptions/index.tsx (2)
16-18: LGTM! Archive option implementation looks good.The archive menu item correctly:
- Disables during mutation with
isArchiveMutationInFlight- Toggles text between "Archive Chat" and "Unarchive Chat"
19-30: LGTM! Group options correctly hidden for one-on-one chats.The implementation aligns with PR requirements by conditionally rendering group-specific options only when
isGroupis true.
packages/components/modules/messages/ChatRoom/ChatRoomHeader/ChatRoomOptions/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/ChatRoom/ChatRoomHeader/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/ChatRoom/ChatRoomHeader/index.tsx
Outdated
Show resolved
Hide resolved
tsl-ps2
force-pushed
the
BA-2226-three-dot-menu-cleanup
branch
from
71e141d to
438228f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/components/modules/messages/web/ChatRoomsList/ChatRoomItem/index.tsx (1)
80-98:⚠️ Potential issueAdd closeOnClick to archive action to meet PR requirements.
The PR objectives specify that the three-dot menu should disappear after selecting any option. While this is implemented for the "Mark as Unread" action, it's missing for the archive action.
{ disabled: isMutationInFlight, icon: !isInArchivedTab ? <ArchiveIcon /> : <UnarchiveIcon />, label: !isInArchivedTab ? 'Archive Chat' : 'Unarchive Chat', onClick: () => { if (currentProfile?.id) { commit({ variables: { input: { roomId: roomRef.id, profileId: currentProfile.id, archive: !isInArchivedTab, }, }, }) } }, hasPermission: true, + closeOnClick: true, },
🧹 Nitpick comments (3)
packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/ChatRoomOptions/index.tsx (1)
16-18: Consider enhancing UX during archive operation.While disabling the menu item during mutation is good, consider adding a loading indicator to provide better feedback to users.
- <MenuItem onClick={onArchiveClicked} disabled={isArchiveMutationInFlight}> - <Typography variant="body2">{isArchived ? 'Unarchive Chat' : 'Archive Chat'}</Typography> + <MenuItem onClick={onArchiveClicked} disabled={isArchiveMutationInFlight}> + <Box display="flex" alignItems="center" gap={1}> + <Typography variant="body2">{isArchived ? 'Unarchive Chat' : 'Archive Chat'}</Typography> + {isArchiveMutationInFlight && <CircularProgress size={16} />} + </Box> + </MenuItem>packages/components/modules/messages/web/ChatRoom/index.tsx (1)
46-46: Consider more explicit boolean conversion.While
!!works, consider usingBoolean()for more explicit type conversion.- isArchived={!!chatRoom.isArchived} + isArchived={Boolean(chatRoom.isArchived)}packages/components/modules/messages/web/ChatRoomsList/ChatRoomItem/index.tsx (1)
73-94: Add error handling for archive/unarchive operations.While the archive functionality is well-implemented, consider adding error handling and user feedback for failed operations.
const [commit, isMutationInFlight] = useArchiveChatRoomMutation() +const [error, setError] = useState<Error | null>(null) // ... in onClick handler commit({ variables: { input: { roomId: roomRef.id, profileId: currentProfile.id, archive: !isInArchivedTab, }, }, + onError: (error) => { + setError(error) + // Show error toast/notification + }, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
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/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(6 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
🔇 Additional comments (8)
packages/components/modules/messages/common/graphql/mutations/ArchiveChatRoom.ts (2)
1-1: Imports look correct
These new imports (useCurrentProfile, ConnectionHandler, etc.) appear properly scoped and logically placed. No issues noted.Also applies to: 4-4, 7-7
32-32: Consider a fallback for missing profile
IfcurrentProfileis ever undefined, mutation logic may unintentionally skip essential store updates. Ensure thatcurrentProfileis reliably present, or handle that fallback scenario gracefully.packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/ChatRoomOptions/types.ts (1)
2-4: New props align well with archiving & group handling
These boolean props are clear and descriptive, helping manage distinct menu behavior effectively.packages/components/modules/messages/common/graphql/queries/ChatRoomQuery.ts (1)
7-7: “isArchived” field aligns with archiving logic
AddingisArchivedensures you can accurately fetch and handle this state in the client.packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/types.ts (1)
6-6: LGTM! Clean type addition.The new
isArchivedboolean property is well-defined and supports the archive functionality requirement.packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/ChatRoomOptions/index.tsx (1)
7-14: Well-structured implementation of menu options!The changes perfectly align with the PR requirements:
- Group-specific options ("Group Details" and "Leave Group") are only shown for group chats
- Archive option adapts based on the current state
packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsx (1)
125-134: LGTM! Menu properly closes after selection.The implementation correctly handles the requirement to close the three-dot menu after any option is selected.
packages/components/modules/messages/web/ChatRoomsList/ChatRoomItem/index.tsx (1)
12-12: LGTM! Import changes align with the new architecture.The removal of connection-related imports and addition of
useFragmentaligns well with moving the connection management responsibility to the mutation hook.
packages/components/modules/messages/common/graphql/mutations/ArchiveChatRoom.ts
Show resolved
Hide resolved
packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsx
Show resolved
Hide resolved
tsl-ps2
force-pushed
the
BA-2226-three-dot-menu-cleanup
branch
from
438228f to
1530cd5
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/modules/messages/common/graphql/mutations/ArchiveChatRoom.ts (1)
24-46: Add success notification for archive action.Consider adding a success toast notification to provide feedback when the chat room is successfully archived/unarchived.
onCompleted: (response, errors) => { errors?.forEach((error) => { sendToast(error.message, { type: 'error' }) }) + if (!errors?.length && response?.chatRoomArchive?.room) { + sendToast( + `Chat room ${response.chatRoomArchive.room.isArchived ? 'archived' : 'unarchived'} successfully`, + { type: 'success' } + ) + } config?.onCompleted?.(response, errors) },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/components/CHANGELOG.md(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/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(6 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/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/package.json
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/components/modules/messages/common/graphql/queries/ChatRoomQuery.ts
- packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/types.ts
- packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/ChatRoomOptions/types.ts
- packages/components/modules/messages/web/ChatRoomsList/ChatRoomItem/index.tsx
- packages/components/modules/messages/web/ChatRoom/index.tsx
- packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/ChatRoomOptions/index.tsx
🔇 Additional comments (5)
packages/components/modules/messages/common/graphql/mutations/ArchiveChatRoom.ts (2)
1-22: LGTM!The imports and mutation structure are well-organized, and the
isArchivedfield is correctly added to the mutation response.
47-59: Clarify deletion logic for archived chat rooms.Deleting the node from all matching connections could result in unexpected behavior or UI inconsistencies if multiple views reference the same room. Consider either separating the archived node into its own connection or ensuring this deletion reflects the intended user experience. Adding a success toast may also help confirm the action.
packages/components/modules/messages/web/ChatRoom/ChatRoomHeader/index.tsx (2)
47-60: Add error handling for archive operation.The archive mutation lacks error handling. Consider adding error handling and user feedback.
90-134: LGTM!The UI changes correctly implement the PR objectives:
- Group details are only accessible in group chats
- Three-dot menu options are properly handled with close actions
packages/components/CHANGELOG.md (1)
3-8: LGTM!The changelog entry accurately describes the changes made to the three-dot menu functionality.
* BA-2226 * BA-2226 Versioning
* BA-2226 * BA-2226 Versioning
* BA-2226 * BA-2226 Versioning
* BA-2226 * BA-2226 Versioning
Acceptance Criteria
Given I am in an 1 on 1 Chat Room (which is a chat room that is not a group chat), when I open the 3 Dot menu, then I should not see the following options:
Leave Group
Group Details
Given I am on any type of Chat room (1 on 1 or Group chat), when I click an option on the3 Dot menu then the menu should disappear.
Implement the Archive behavior on the Archive Chat option in the 3 Dot Menu
Current behavior
Loom: https://www.loom.com/share/64f71a85d88f46c69e3a79eed133efe0
Approvd
https://app.approvd.io/silverlogic/BA/stories/38916
Summary by CodeRabbit
New Features
Bug Fixes