BA-1882: update unread mechanism #146
Conversation
|
|
Warning Rate limit exceeded@Ronan-Fernandes has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📥 CommitsReviewing files that changed from the base of the PR and between 5ceacda81d1ac38d65e9109268f3a2f2aba231c7 and 08f9118. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
WalkthroughThe changes encompass multiple components within the messaging module, primarily focusing on enhancing real-time functionality and state management. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatRoomsList
participant useMessageCountUpdate
participant MessagesList
participant ChatRoom
User->>ChatRoomsList: Open Chat Rooms
ChatRoomsList->>useMessageCountUpdate: Subscribe to Message Count
useMessageCountUpdate-->>ChatRoomsList: Real-time Message Count Update
ChatRoomsList->>MessagesList: Render Messages
MessagesList-->>ChatRoom: Display Messages
ChatRoom->>User: Show Chat Room
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 (
|
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx (1)
Line range hint
42-44: Consider enhancing error handlingThe current error handling only logs to console. Consider implementing proper error handling and recovery mechanisms.
onError: console.error, +onError: (error) => { + console.error('Message count subscription error:', error); + // Add error reporting service call + // Implement retry logic if needed +},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 913f14e and 8de11b1df0bd9b8a101e1bdb13902416175e847a.
⛔ Files ignored due to path filters (1)
packages/components/__generated__/useMessageCountUpdateSubscription.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (5)
packages/components/modules/messages/ChatRoomsList/ChatRoomItem/styled.tsx(1 hunks)packages/components/modules/messages/ChatRoomsList/index.tsx(2 hunks)packages/components/modules/messages/MessagesList/index.tsx(3 hunks)packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx(1 hunks)packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx(1 hunks)
🔇 Additional comments (7)
packages/components/modules/messages/ChatRoomsList/ChatRoomItem/styled.tsx (1)
9-9: LGTM: Improved vertical alignment
The addition of alignItems: 'center' properly centers the grid items vertically within their cells, enhancing the visual consistency of the chat card layout.
packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx (2)
68-68: LGTM: Correct dependency addition
Adding profile?.id to the dependency array is necessary as it's used in the variables object. This ensures the subscription is properly updated when the profile changes.
Line range hint 33-33: Address the TODO comment about backend subscription
The comment suggests potential issues with the backend subscription. This should be investigated and resolved.
Would you like me to help create a GitHub issue to track the backend subscription investigation?
packages/components/modules/messages/MessagesList/index.tsx (3)
99-99: LGTM: Style adjustment for better layout
The paddingBottom: 0 style addition helps ensure consistent spacing in the virtualized list.
128-128: Verify impact of followOutput change
Changing followOutput from "smooth" to false could affect the user experience when new messages arrive.
Let's check for related scroll behavior implementations:
✅ Verification successful
followOutput change is safe to proceed
The search results show that followOutput is only used once in the entire codebase, specifically in the MessagesList component. The change from "smooth" to false is isolated to this component and doesn't affect any other implementations of Virtuoso across the codebase. Other Virtuoso instances in components like ChatRoomsList, NotificationsList, and CommentsList don't use this prop at all.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Virtuoso implementations with followOutput
ast-grep --pattern 'Virtuoso {
$$$
followOutput={$_}
$$$
}'
Length of output: 67
Script:
#!/bin/bash
# Let's try a different approach to search for Virtuoso components and followOutput prop
rg -i "virtuoso.*followOutput" -A 3 -B 3
Length of output: 42
Script:
#!/bin/bash
# Let's search for Virtuoso component usage and followOutput separately
echo "=== Searching for Virtuoso component usage ==="
rg "<Virtuoso" -A 5 -B 5
echo -e "\n=== Searching for followOutput prop ==="
rg "followOutput" -A 2 -B 2
Length of output: 12078
Line range hint 109-119: Verify mutation trigger conditions and error handling
The mutation trigger for marking messages as read needs additional considerations:
- The TODO comment suggests uncertainty about the implementation approach
- Missing error handling for the mutation
- No loading state management during mutation
Let's verify the mutation implementation:
Consider enhancing the implementation:
endReached={() => {
if (room?.unreadMessagesCount && room?.unreadMessagesCount > 0) {
+ try {
commitMutation({
variables: {
input: {
roomId: room.id,
profileId: profile?.id as string,
},
},
+ onCompleted: (response, errors) => {
+ if (errors) {
+ console.error('Failed to mark messages as read:', errors);
+ }
+ },
})
+ } catch (error) {
+ console.error('Error marking messages as read:', error);
+ }
}
}}packages/components/modules/messages/ChatRoomsList/index.tsx (1)
15-15:
Review temporary subscription implementation
The TODO comment suggests this is a temporary solution until the chatRoomOnRoomUpdate subscription is fixed. This could lead to:
- Duplicate subscriptions and unnecessary network calls
- Potential race conditions between subscriptions
- Memory leaks if subscriptions aren't properly cleaned up
Let's analyze the subscription implementations:
Consider:
- Document the known issues with
chatRoomOnRoomUpdatesubscription - Create a tracking issue for fixing the original subscription
- Add cleanup logic to prevent memory leaks
- Implement proper error handling for the subscription
Also applies to: 74-74
...ages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 8de11b1df0bd9b8a101e1bdb13902416175e847a and ee01db286a1a98f9a0305f029314db6d7ccccb62.
📒 Files selected for processing (2)
packages/components/CHANGELOG.md(1 hunks)packages/components/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/package.json
Ronan-Fernandes
force-pushed
the
BA-1882-FE-packages-Unread-updates
branch
from
ee01db2 to
27e08ab
Compare
Ronan-Fernandes
force-pushed
the
BA-1882-FE-packages-Unread-updates
branch
3 times, most recently
from
306130e to
b22964c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/components/modules/messages/ChatRoom/index.tsx (2)
62-65: Remove unnecessaryuseEffectupdating refsAfter simplifying the main
useEffect, theuseEffecton lines 62-65 that updatesprevProfileIdRef.currentandprevRoomIdRef.currentis no longer needed. Removing it will simplify the component.
26-26: EliminatehasRunRefvariableWith the refactored
useEffect, thehasRunRefvariable is no longer used. Consider removingconst hasRunRef = useRef(false)to clean up the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between f1fde05f1e040a898d2003c989a2fdd53cca4a72 and b22964c9c919602fc3819f42bd7f62f9292a0dab.
⛔ Files ignored due to path filters (1)
packages/components/__generated__/useMessageCountUpdateSubscription.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (8)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/messages/ChatRoom/index.tsx(3 hunks)packages/components/modules/messages/ChatRoomsList/ChatRoomItem/styled.tsx(1 hunks)packages/components/modules/messages/ChatRoomsList/index.tsx(2 hunks)packages/components/modules/messages/MessagesList/index.tsx(0 hunks)packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx(1 hunks)packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx(1 hunks)packages/components/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/components/modules/messages/MessagesList/index.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/components/modules/messages/ChatRoomsList/ChatRoomItem/styled.tsx
- packages/components/package.json
- packages/components/CHANGELOG.md
- packages/components/modules/messages/ChatRoomsList/index.tsx
- packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx
- packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx
Ronan-Fernandes
force-pushed
the
BA-1882-FE-packages-Unread-updates
branch
from
b22964c to
c33e268
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/components/modules/messages/ChatRoom/index.tsx (1)
Line range hint
67-69: Enhance error handling for chat room.The current error handling is basic. Consider:
- Adding more specific error states (network error, access denied, etc.)
- Providing a more user-friendly error message
- Adding retry functionality
- if (!chatRoom) { - return <div>Chat room not found</div> - } + if (!chatRoom) { + return ( + <Box display="flex" justifyContent="center" alignItems="center" p={2}> + <Typography variant="body1"> + Unable to load chat room. Please try again later. + </Typography> + <Button onClick={() => window.location.reload()}> + Retry + </Button> + </Box> + ) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between b22964c9c919602fc3819f42bd7f62f9292a0dab and c33e2685ce037e2474c91a677066d871f0755f37.
⛔ Files ignored due to path filters (1)
packages/components/__generated__/useMessageCountUpdateSubscription.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (7)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/messages/ChatRoom/index.tsx(3 hunks)packages/components/modules/messages/ChatRoomsList/ChatRoomItem/styled.tsx(1 hunks)packages/components/modules/messages/ChatRoomsList/index.tsx(2 hunks)packages/components/modules/messages/MessagesList/index.tsx(0 hunks)packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx(1 hunks)packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/components/modules/messages/MessagesList/index.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/components/modules/messages/ChatRoomsList/ChatRoomItem/styled.tsx
- packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx
- packages/components/CHANGELOG.md
- packages/components/modules/messages/ChatRoomsList/index.tsx
- packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx
🔇 Additional comments (4)
packages/components/modules/messages/ChatRoom/index.tsx (4)
26-26: Remove unnecessary hasRunRef.
The hasRunRef is an anti-pattern for handling useEffect cleanup. This should be removed as part of the useEffect refactoring.
44-60: Simplify useEffect cleanup logic.
The current implementation uses a complex pattern with hasRunRef and conditional cleanup. Please refer to the previous review comment for the recommended refactoring approach.
62-65: 🛠️ Refactor suggestion
Consider removing the second useEffect.
This useEffect can be eliminated if we follow the previous review suggestion of capturing the current values in the cleanup function closure.
- const prevProfileIdRef = useRef<string | undefined>(profile?.id)
- const prevRoomIdRef = useRef<string | undefined>(chatRoom?.id)
- useEffect(() => {
- prevProfileIdRef.current = profile?.id
- prevRoomIdRef.current = chatRoom?.id
- }, [profile?.id, chatRoom?.id])Likely invalid or redundant comment.
39-39: Verify mutation usage across the codebase.
Let's ensure the read message mutation is being used consistently across the application.
Also applies to: 48-55
✅ Verification successful
Mutation usage is correctly implemented
The useReadMessageMutation is being used appropriately in the ChatRoom component:
- It's called in the cleanup phase of a useEffect to mark messages as read when leaving the chat room
- The mutation is properly defined with error handling and toast notifications
- The implementation follows the correct pattern for Relay mutations
- No inconsistencies found in its usage across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of the ReadMessages mutation
ast-grep --pattern 'useReadMessageMutation'
# Search for direct mutation calls
rg -A 5 'ReadMessages' --type typescript
Length of output: 482
Script:
#!/bin/bash
# Search for ReadMessages mutation with proper file type
rg -t ts -t tsx -A 5 'ReadMessages'
# Search for the mutation definition and its usage pattern
ast-grep --pattern 'mutation ReadMessages'
# Look for the mutation file content
cat packages/components/modules/messages/graphql/mutations/ReadMessages.ts
Length of output: 1500
Script:
#!/bin/bash
# Search for the actual usage of the mutation in the ChatRoom component
cat packages/components/modules/messages/ChatRoom/index.tsx
# Search for any other potential usages with proper file extension
rg -g '*.{ts,tsx}' -A 5 'chatRoomReadMessages'
Length of output: 6709
Ronan-Fernandes
force-pushed
the
BA-1882-FE-packages-Unread-updates
branch
from
c33e268 to
b19df67
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/components/modules/messages/ChatRoom/index.tsx (2)
28-28: Address the TODO comment regarding query preloadingThe TODO comment indicates a potential performance optimization. Consider implementing query preloading to improve the initial loading experience.
Would you like me to help create an issue to track this optimization task or provide guidance on implementing query preloading?
Line range hint
67-69: Enhance error handling for chat roomThe current error handling is basic and could be improved. Consider:
- Adding more specific error states (not found vs. error loading)
- Implementing a proper error boundary
- Adding retry functionality
Would you like me to provide an example implementation with proper error handling?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between c33e2685ce037e2474c91a677066d871f0755f37 and b19df6773e014b9c97405777b8fd65a1792dc497.
⛔ Files ignored due to path filters (1)
packages/components/__generated__/useMessageCountUpdateSubscription.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (7)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/messages/ChatRoom/index.tsx(3 hunks)packages/components/modules/messages/ChatRoomsList/ChatRoomItem/styled.tsx(1 hunks)packages/components/modules/messages/ChatRoomsList/index.tsx(2 hunks)packages/components/modules/messages/MessagesList/index.tsx(0 hunks)packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx(1 hunks)packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/components/modules/messages/MessagesList/index.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/components/modules/messages/ChatRoomsList/ChatRoomItem/styled.tsx
- packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx
- packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx
- packages/components/CHANGELOG.md
- packages/components/modules/messages/ChatRoomsList/index.tsx
🔇 Additional comments (5)
packages/components/modules/messages/ChatRoom/index.tsx (5)
3-3: LGTM: Imports are correctly structured
The new imports are properly organized and necessary for the component's enhanced functionality.
Also applies to: 11-11, 14-14
26-26: 🛠️ Refactor suggestion
Consider removing hasRunRef
The hasRunRef introduces unnecessary complexity in managing the effect's lifecycle. As noted in the previous review, this can be simplified by restructuring the effect.
41-42: 🛠️ Refactor suggestion
Simplify state tracking
The use of refs to track previous values can be eliminated by capturing current values in the effect closure, as suggested in the previous review.
44-60:
Simplify useEffect cleanup handling
This implementation matches the exact pattern that was previously identified as needing improvement. Please implement the suggested refactor from the previous review to simplify the cleanup logic and remove the need for hasRunRef.
38-39: Verify message reading behavior
Let's verify that the message reading functionality is properly integrated with the profile management.
✅ Verification successful
Message reading functionality is properly integrated
The verification shows that the message reading functionality is correctly implemented:
- The
useReadMessageMutationhook is properly integrated with profile management in the ChatRoom component - The mutation is triggered when the component unmounts, using the correct profile ID and room ID
- The implementation includes proper error handling and toast notifications
- The profile context is consistently used across the messaging components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that might be affected by the message reading changes
rg -l "useReadMessageMutation|useCurrentProfile" --type typescript
# Look for potential race conditions or duplicate message reading logic
ast-grep --pattern 'useReadMessageMutation'
Length of output: 510
Script:
#!/bin/bash
# Let's check the implementation of useReadMessageMutation and its usage
cat packages/components/modules/messages/graphql/mutations/ReadMessages.ts
# Let's also check the full context of ChatRoom component
cat packages/components/modules/messages/ChatRoom/index.tsx
# Let's check for profile-related implementations
rg "useCurrentProfile" -A 5
Length of output: 22812
Ronan-Fernandes
force-pushed
the
BA-1882-FE-packages-Unread-updates
branch
from
b19df67 to
f29aa35
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/components/modules/messages/ChatRoom/index.tsx (1)
Line range hint
67-69: Enhance error handling implementationThe current error handling is basic and needs improvement as noted by the TODO comment. Consider:
- Adding more specific error states (network error, access denied, etc.)
- Providing more informative error messages
- Adding retry functionality for failed queries
Would you like me to help implement a more robust error handling solution?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between b19df6773e014b9c97405777b8fd65a1792dc497 and f29aa3546829fa4ff1cdcadde7e12cb046184fae.
⛔ Files ignored due to path filters (1)
packages/components/__generated__/useMessageCountUpdateSubscription.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (6)
packages/components/modules/messages/ChatRoom/index.tsx(3 hunks)packages/components/modules/messages/ChatRoomsList/ChatRoomItem/styled.tsx(1 hunks)packages/components/modules/messages/ChatRoomsList/index.tsx(2 hunks)packages/components/modules/messages/MessagesList/index.tsx(0 hunks)packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx(1 hunks)packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/components/modules/messages/MessagesList/index.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/components/modules/messages/ChatRoomsList/ChatRoomItem/styled.tsx
- packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx
- packages/components/modules/messages/ChatRoomsList/index.tsx
- packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx
🔇 Additional comments (4)
packages/components/modules/messages/ChatRoom/index.tsx (4)
3-3: LGTM! Import changes are appropriate
The new imports align well with the component's enhanced functionality for managing read status and profile state.
Also applies to: 11-11, 14-14
44-65: Previous review comments remain valid
The existing review comments about simplifying the useEffect implementation and removing the unnecessary effect for ref updates are still applicable.
26-26: 🛠️ Refactor suggestion
Consider simplifying the state management approach
The current implementation uses multiple refs to track component state, which adds complexity. Consider using React's built-in effect cleanup mechanism instead of manually tracking previous values and initial render state.
The state management could be simplified by:
- Removing
hasRunRefentirely - Capturing current values in the effect closure instead of using refs
- Utilizing the effect cleanup function consistently
This aligns with the previous review comment about simplifying the useEffect implementation.
Also applies to: 38-42
38-39: Verify the read status mutation behavior
The changes affect how message read status is handled. Let's verify the implementation across the application.
✅ Verification successful
Read message mutation implementation is properly integrated
The verification shows that the ReadMessagesMutation is correctly implemented and integrated across the chat system:
- The mutation is properly defined in
packages/components/modules/messages/graphql/mutations/ReadMessages.ts - It updates the
unreadMessagesCountfield which is consistently used across components:- Chat room list (
ChatRoomsList/ChatRoomItem) - Message groups (
MessagesList/MessagesGroup) - Room queries and subscriptions
- Chat room list (
- The mutation is used in the chat room component to mark messages as read
- The changes are reflected through GraphQL subscriptions for real-time updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of ReadMessages mutation
ast-grep --pattern 'useReadMessageMutation'
# Search for related message status updates
rg -l 'message.*status|read.*message' --type=typescript --type=tsx
Length of output: 510
Script:
#!/bin/bash
# Search for message status and read message patterns with correct file extensions
rg -l "message.*status|read.*message" --type-add 'ts:*.{ts,tsx}' --type=ts
# Look for the ReadMessages mutation implementation
ast-grep --pattern 'mutation ReadMessages'
# Search for message read status related code
rg -A 5 "readMessage|messageRead|ReadMessage" --type-add 'ts:*.{ts,tsx}' --type=ts
Length of output: 48138
Ronan-Fernandes
force-pushed
the
BA-1882-FE-packages-Unread-updates
branch
from
f29aa35 to
5ceacda
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/components/modules/messages/ChatRoom/index.tsx (2)
Line range hint
67-69: Enhance error handling implementationThe current error handling is basic and could be improved to provide more context to users.
Consider enhancing the error state:
- if (!chatRoom) { - return <div>Chat room not found</div> - } + if (!chatRoom) { + return ( + <Box display="flex" justifyContent="center" alignItems="center" p={2}> + <Typography variant="body1" color="error"> + Chat room not found. Room ID: {roomId} + </Typography> + </Box> + ) + }
Line range hint
19-89: Consider separating data management logicThe component currently handles both UI rendering and complex data management logic. Consider extracting the message read status logic into a custom hook for better maintainability and reusability.
Example structure:
// useMessageReadStatus.ts export const useMessageReadStatus = (roomId: string, profileId: string | undefined) => { const [commitMutation] = useReadMessageMutation() useEffect(() => { const currentProfileId = profileId const currentRoomId = roomId return () => { if (currentProfileId && currentRoomId) { commitMutation({ variables: { input: { roomId: currentRoomId, profileId: currentProfileId }, }, }) } } }, [profileId, roomId]) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between f29aa3546829fa4ff1cdcadde7e12cb046184fae and 5ceacda81d1ac38d65e9109268f3a2f2aba231c7.
⛔ Files ignored due to path filters (1)
packages/components/__generated__/useMessageCountUpdateSubscription.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (6)
packages/components/modules/messages/ChatRoom/index.tsx(4 hunks)packages/components/modules/messages/ChatRoomsList/ChatRoomItem/styled.tsx(1 hunks)packages/components/modules/messages/ChatRoomsList/index.tsx(2 hunks)packages/components/modules/messages/MessagesList/index.tsx(0 hunks)packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx(1 hunks)packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/components/modules/messages/MessagesList/index.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/components/modules/messages/ChatRoomsList/ChatRoomItem/styled.tsx
- packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx
- packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx
- packages/components/modules/messages/ChatRoomsList/index.tsx
🔇 Additional comments (2)
packages/components/modules/messages/ChatRoom/index.tsx (2)
3-3: LGTM! Imports are properly organized
The new imports are correctly added and necessary for the implemented functionality.
Also applies to: 5-5
44-65:
Simplify effects implementation
The current implementation of both useEffect hooks introduces unnecessary complexity and potential issues with cleanup handling.
The previous review comments about simplifying these effects remain valid. The suggested refactoring would:
- Remove the need for hasRunRef
- Eliminate the second effect entirely
- Ensure more reliable cleanup behavior
| @@ -21,6 +23,7 @@ const ChatRoom: FC<ChatRoomProps> = ({ | |||
| SendMessage = DefaultSendMessage, | |||
| SendMessageProps = {}, | |||
| }) => { | |||
| const hasRunRef = useRef(false) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the state management pattern
The current implementation uses multiple refs to track state between renders, which adds complexity. This could be simplified by leveraging React's closure mechanics in useEffect.
Consider removing the refs and capturing the values directly in the effect closure:
- const hasRunRef = useRef(false)
- const prevProfileIdRef = useRef<string | undefined>(profile?.id)
- const prevRoomIdRef = useRef<string | undefined>(chatRoom?.id)Also applies to: 38-42
Ronan-Fernandes
force-pushed
the
BA-1882-FE-packages-Unread-updates
branch
from
5ceacda to
68d80a7
Compare
Ronan-Fernandes
force-pushed
the
BA-1882-FE-packages-Unread-updates
branch
from
5da7599 to
8ff1275
Compare
Ronan-Fernandes
force-pushed
the
BA-1882-FE-packages-Unread-updates
branch
from
0edd25e to
08f9118
Compare
|
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style