BA-1964: update unread badge and divider behavior #159
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/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
WalkthroughThe pull request introduces significant changes to the messaging components in the application, particularly the Changes
Possibly related PRs
Suggested reviewers
Poem
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 and nitpick comments (2)
packages/components/modules/messages/ChatRoom/index.tsx (1)
Line range hint
26-26: Consider addressing TODOs before merging.There are two TODOs that should be addressed:
- Pre-loading the query instead of using lazy loading
- Handling errors for chatRoom
These could impact the user experience and error handling of the application.
Would you like help implementing these improvements? I can help with:
- Converting to preloaded query using QueryLoader
- Adding proper error boundaries and error states
Also applies to: 35-35
packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx (1)
Line range hint
60-78: Consider extracting styles and adding i18n supportThe new unread messages divider logic is clean and effective. Consider these improvements:
- Extract the divider styling to a styled component or theme
- Add internationalization support for the "New Messages" text
Example refactor:
+ const UnreadDivider = styled(Divider)(({ theme }) => ({ + '&.MuiDivider-root::before, &.MuiDivider-root::after': { + borderTop: `1px solid ${theme.palette.error.light}`, + }, + })); if (currentMessage?.id === firstUnreadMessageId) { return ( - <Divider - variant="fullWidth" - sx={{ - '&.MuiDivider-root::before, &.MuiDivider-root::after': { - borderTop: `1px solid ${theme.palette.error.light}`, - }, - }} - > + <UnreadDivider variant="fullWidth"> <Typography variant="caption" color="error" sx={{ textAlign: 'center' }}> - New Messages + {t('messages.newMessages')} </Typography> - </Divider> + </UnreadDivider> ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/components/modules/messages/ChatRoom/index.tsx(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx(3 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/types.ts(1 hunks)packages/components/modules/messages/MessagesList/index.tsx(6 hunks)
🔇 Additional comments (7)
packages/components/modules/messages/MessagesList/MessagesGroup/types.ts (1)
15-15: LGTM! Verify type usage across components.
The optional firstUnreadMessageId type is well-defined. The union type string | null properly handles cases where there might not be any unread messages.
Let's verify the type usage across components:
✅ Verification successful
Type usage is consistent across components
The verification shows proper type consistency:
- Type definition in
types.ts:firstUnreadMessageId?: string | null - State declaration in
MessagesList/index.tsx:useState<string | null | undefined>(null) - Prop passing and usage in both
MessagesListandMessagesGroupcomponents
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for firstUnreadMessageId usage to ensure type consistency
ast-grep --pattern 'firstUnreadMessageId?: string | null'
rg 'firstUnreadMessageId'
Length of output: 930
packages/components/modules/messages/ChatRoom/index.tsx (1)
3-3: Good job simplifying the ChatRoom component!
The removal of profile and read status management from this component improves separation of concerns. The component now focuses solely on rendering the chat room interface.
Also applies to: 16-42
packages/components/modules/messages/MessagesList/index.tsx (2)
Line range hint 144-154: Consider adding error handling for virtualization.
The TODO comment about overscan causing stack overflow should be addressed with proper error boundaries.
Let's check for similar virtualization issues:
Consider implementing:
- Error boundaries around the virtualized list
- Fallback rendering for large datasets
- Progressive loading with proper throttling
62-70: 🛠️ Refactor suggestion
Potential race condition in unread message handling.
The effect properly cleans up state, but there's a potential race condition between setting firstUnreadMessageId and the read message mutation.
Let's verify the mutation timing:
Consider combining the effects and ensuring atomic updates:
useEffect(() => {
if (room?.unreadMessagesCount !== 0) {
const firstUnreadMessage = getFirstUnreadMessage()
setFirstUnreadMessageId(firstUnreadMessage?.id)
+ if (firstUnreadMessage) {
+ commitMutation({
+ variables: {
+ input: {
+ roomId: room?.id as string,
+ profileId: currentProfile?.id as string,
+ },
+ },
+ })
+ }
}
return () => {
setFirstUnreadMessageId(null)
}
-}, [selectedRoom, currentProfile])
+}, [selectedRoom, currentProfile, room?.unreadMessagesCount])packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx (3)
26-26: LGTM: Clean prop addition
The firstUnreadMessageId prop is well-integrated and properly typed.
79-79: LGTM: Correct dependency updates
The dependencies array properly reflects the variables used in the callback, including theme colors for proper reactivity.
26-26: Verify unread message handling integration
Let's ensure the unread message handling is consistent across the messaging system.
Also applies to: 60-60
✅ Verification successful
Based on the results, I can now generate the final response as we have sufficient information about the unread message handling integration.
Unread message handling is properly integrated
The firstUnreadMessageId is well-integrated across the messaging system:
- Properly managed with useState in the parent MessagesList component
- Correctly passed down as a prop to MessagesGroup
- Used appropriately in MessagesGroup to render an unread messages divider
- Type-safe implementation with proper TypeScript interfaces
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how firstUnreadMessageId is managed in parent components
echo "Checking MessagesList component for firstUnreadMessageId handling:"
rg -A 5 "firstUnreadMessageId" "packages/components/modules/messages/MessagesList"
echo "\nChecking for potential unread message state management:"
ast-grep --pattern 'const [first_unread_message_id, set_first_unread_message_id] = useState<string | null>'
Length of output: 4409
Ronan-Fernandes
force-pushed
the
BA-1964-FE-package-mark-as-read-behavior-change
branch
from
0fab4c8 to
99922a0
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/components/modules/messages/MessagesList/index.tsx (1)
50-61:⚠️ Potential issueConsider edge cases in getFirstUnreadMessage function.
The function's array access pattern could lead to undefined access issues.
Apply this safer implementation:
const getFirstUnreadMessageId = () => { if (room?.unreadMessagesCount === 0) return null return allMessages.find((message, index) => { - const previousMessage = allMessages?.[index + 1] - const hasPreviousMessage = !!previousMessage + const previousMessage = index < allMessages.length - 1 ? allMessages[index + 1] : null + const hasPreviousMessage = Boolean(previousMessage) return ( message?.profile?.id !== currentProfile?.id && !message?.isRead && (!hasPreviousMessage || previousMessage?.isRead) ) })?.id }
🧹 Nitpick comments (1)
packages/components/modules/messages/ChatRoom/index.tsx (1)
Line range hint
16-45: LGTM: Component successfully simplified to focus on core rendering.The removal of profile management and message reading logic has effectively streamlined this component. The changes align well with the single responsibility principle, making the code more maintainable.
This refactoring improves the separation of concerns by moving the message read status management to a more appropriate location in the MessagesList component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/messages/ChatRoom/index.tsx(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx(3 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/types.ts(1 hunks)packages/components/modules/messages/MessagesList/index.tsx(5 hunks)packages/components/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/components/package.json
- packages/components/modules/messages/MessagesList/MessagesGroup/types.ts
- packages/components/CHANGELOG.md
- packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx
🔇 Additional comments (3)
packages/components/modules/messages/ChatRoom/index.tsx (1)
3-4: LGTM: Simplified imports align with component's focused responsibility.
The removal of unnecessary hooks and the reduction to core React imports better reflects the component's streamlined responsibility of rendering the chat room.
packages/components/modules/messages/MessagesList/index.tsx (2)
46-48: LGTM: Appropriate hooks added for message read status management.
The addition of these hooks aligns with moving the read status management responsibility to this component.
109-111: LGTM: Properly passing firstUnreadMessageId to MessagesGroup.
The integration of the unread message tracking with the MessagesGroup component is implemented correctly.
| useEffect(() => { | ||
| if (room?.unreadMessagesCount !== 0) { | ||
| commitMutation({ | ||
| variables: { | ||
| input: { | ||
| roomId: room?.id as string, | ||
| profileId: currentProfile?.id as string, | ||
| }, | ||
| }, | ||
| }) | ||
| } | ||
| }, [room?.id, room?.unreadMessagesCount, currentProfile]) |
There was a problem hiding this comment.
Add error handling for the read message mutation.
The mutation commitment lacks error handling which could lead to silent failures.
Consider adding error handling:
useEffect(() => {
if (room?.unreadMessagesCount !== 0) {
commitMutation({
variables: {
input: {
roomId: room?.id as string,
profileId: currentProfile?.id as string,
},
},
+ onError: (error) => {
+ console.error('Failed to mark messages as read:', error);
+ // Consider showing a user-friendly error notification
+ },
})
}
}, [room?.id, room?.unreadMessagesCount, currentProfile])📝 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.
| useEffect(() => { | |
| if (room?.unreadMessagesCount !== 0) { | |
| commitMutation({ | |
| variables: { | |
| input: { | |
| roomId: room?.id as string, | |
| profileId: currentProfile?.id as string, | |
| }, | |
| }, | |
| }) | |
| } | |
| }, [room?.id, room?.unreadMessagesCount, currentProfile]) | |
| useEffect(() => { | |
| if (room?.unreadMessagesCount !== 0) { | |
| commitMutation({ | |
| variables: { | |
| input: { | |
| roomId: room?.id as string, | |
| profileId: currentProfile?.id as string, | |
| }, | |
| }, | |
| onError: (error) => { | |
| console.error('Failed to mark messages as read:', error); | |
| // Consider showing a user-friendly error notification | |
| }, | |
| }) | |
| } | |
| }, [room?.id, room?.unreadMessagesCount, currentProfile]) |
| })?.id | ||
| } | ||
|
|
||
| const firstUnreadMessageId = useMemo(getFirstUnreadMessageId, [selectedRoom, currentProfile]) |
There was a problem hiding this comment.
💡 Codebase verification
Dependencies for firstUnreadMessageId memoization need to be updated
The getFirstUnreadMessageId function uses room, allMessages, and currentProfile internally, but the current memoization only includes selectedRoom and currentProfile. Based on the code analysis:
allMessagesis derived fromroom?.allMessages?.edges- The function checks
room?.unreadMessagesCount - It uses
message?.profile?.idandmessage?.isReadfromallMessages
The memoization should include:
room?.unreadMessagesCountallMessages
🔗 Analysis chain
Verify memoization dependencies for firstUnreadMessageId.
The current dependencies might not capture all relevant state changes.
Also applies to: 121-121
Let me gather more information about the dependencies used in the memoization.
Let me try a different approach to find the dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for state variables used in getFirstUnreadMessageId
rg -A 2 "const.*State|useState" packages/components/modules/messages/MessagesList/
# Search for variables used in the function
ast-grep --pattern 'const getFirstUnreadMessageId = () => { $$$ }'
Length of output: 1616
Script:
#!/bin/bash
# Search for allMessages and room variables
rg -B 5 "const \{.*allMessages.*\}" packages/components/modules/messages/MessagesList/
rg -B 5 "const \{.*room.*\}" packages/components/modules/messages/MessagesList/
# Search for any updates to these variables
ast-grep --pattern 'setAllMessages($$$)'
ast-grep --pattern 'setRoom($$$)'
Length of output: 249
Script:
#!/bin/bash
# Search for variable declarations and props destructuring
rg -B 2 "const.*=.*props" packages/components/modules/messages/MessagesList/
rg -B 2 "const \{.*\}.*=.*props" packages/components/modules/messages/MessagesList/
# Look for allMessages and room in the component
rg "allMessages|room" packages/components/modules/messages/MessagesList/index.tsx
# Check for any hooks that might update these values
rg "use\w+" packages/components/modules/messages/MessagesList/index.tsx
Length of output: 2482
anicioalexandre
force-pushed
the
BA-1964-FE-package-mark-as-read-behavior-change
branch
from
99922a0 to
51aec0b
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/components/modules/messages/MessagesList/index.tsx (3)
50-61:⚠️ Potential issueFix edge cases in getFirstUnreadMessageId function
The function has potential edge cases when accessing array indices.
Apply this safer implementation:
const getFirstUnreadMessageId = () => { if (room?.unreadMessages?.count === 0) return null return allMessages.find((message, index) => { - const previousMessage = allMessages?.[index + 1] - const hasPreviousMessage = !!previousMessage + const previousMessage = index < allMessages.length - 1 ? allMessages[index + 1] : null + const hasPreviousMessage = Boolean(previousMessage) return ( message?.profile?.id !== currentProfile?.id && !message?.isRead && (!hasPreviousMessage || previousMessage?.isRead) ) })?.id }
65-76:⚠️ Potential issueAdd error handling for read message mutation
The mutation lacks error handling which could lead to silent failures.
Add error handling:
useEffect(() => { if (room?.unreadMessages?.count !== 0) { commitMutation({ variables: { input: { roomId: room?.id as string, profileId: currentProfile?.id as string, }, }, + onError: (error) => { + console.error('Failed to mark messages as read:', error); + // Consider showing a user-friendly error notification + }, }) } }, [room?.id, room?.unreadMessages?.count, currentProfile])
63-63:⚠️ Potential issueUpdate dependencies for firstUnreadMessageId memoization
The memoization dependencies are incomplete and should include all values used in the calculation.
The dependencies should include:
room?.unreadMessages?.countallMessages
🧹 Nitpick comments (1)
packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx (1)
76-76: Consider memoizing theme.palette.error.lightThe theme value could be memoized to prevent unnecessary re-renders.
+ const errorLight = useMemo(() => theme.palette.error.light, [theme.palette.error.light]); const renderUnreadMessagesDivider = useCallback( (index: number) => { // ... }, - [allMessages, firstUnreadMessageId, theme.palette.error.light], + [allMessages, firstUnreadMessageId, errorLight], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/components/CHANGELOG.md(2 hunks)packages/components/modules/messages/ChatRoom/index.tsx(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx(3 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/types.ts(1 hunks)packages/components/modules/messages/MessagesList/index.tsx(5 hunks)packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx(0 hunks)packages/components/modules/messages/graphql/subscriptions/useMessagesListSubscription.tsx(0 hunks)packages/components/package.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/components/modules/messages/graphql/subscriptions/useMessagesListSubscription.tsx
- packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/components/modules/messages/MessagesList/MessagesGroup/types.ts
- packages/components/CHANGELOG.md
- packages/components/package.json
🔇 Additional comments (4)
packages/components/modules/messages/ChatRoom/index.tsx (2)
Line range hint 26-33: Consider implementing query preloading as noted in TODO comment
The lazy loading of the chat room query could impact initial render performance. Consider implementing query preloading to improve the user experience.
Would you like me to help implement query preloading or create a GitHub issue to track this improvement?
Line range hint 17-54: LGTM! Good separation of concerns
The removal of profile management and message reading logic from this component improves its focus and maintainability. The component now solely handles rendering the chat room UI, which better aligns with the single responsibility principle.
packages/components/modules/messages/MessagesList/index.tsx (1)
Line range hint 141-144: Address TODO comment regarding overscan
The TODO comment indicates a potential stack overflow issue with overscan.
Would you like me to help investigate alternative solutions for implementing virtualization that don't risk stack overflow? We could:
- Implement chunked loading
- Use a different virtualization strategy
- Add proper error boundaries
packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx (1)
Line range hint 57-75: LGTM! Clean implementation of unread messages divider
The new implementation using firstUnreadMessageId simplifies the logic and provides a clear, single source of truth for determining where to show the unread messages divider.
anicioalexandre
changed the title
anicioalexandre
deleted the
BA-1964-FE-package-mark-as-read-behavior-change
branch
Summary by CodeRabbit
Summary by CodeRabbit
New Features
firstUnreadMessageIdin theMessagesGroupcomponent to enhance unread message handling.MessagesListcomponent.Bug Fixes
ChatRoomcomponent by removing unnecessary state management, improving performance and clarity.Documentation
firstUnreadMessageIdproperty in theMessagesGroupPropsinterface.package.json.