feature/react-native-roomlist-subscriptions-improvement #280
Conversation
|
WalkthroughA new native implementation of the Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Component
participant Hook as useRoomListSubscription (Native)
participant Relay as Relay Environment
participant AppState as App State Listener
App->>Hook: Mounts component
Hook->>Relay: Subscribes to room list
AppState-->>Hook: App goes inactive
Hook->>Relay: Unsubscribes from room list
AppState-->>Hook: App returns to foreground
Hook->>Relay: Re-subscribes to room list
App->>Hook: Unmounts component
Hook->>Relay: Cleans up subscription
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
gabrieleandro
force-pushed
the
feature/react-native-roomlist-subscriptions-improvement
branch
from
2174444 to
7847c78
Compare
gabrieleandro
force-pushed
the
feature/react-native-roomlist-subscriptions-improvement
branch
from
588a2bc to
ed09a8e
Compare
|
gabrieleandro
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/components/modules/messages/common/graphql/subscriptions/useRoomListSubscription.tsx (2)
55-55: Consider using a proper error handler instead of console.error.Using
console.errordirectly may not be appropriate for production environments. Consider accepting an error handler as a parameter or using a centralized error reporting system.- onError: console.error, + onError: onError || console.error,And update the function signature to include an optional error handler parameter.
57-82: Consider adding inline documentation for complex Relay operations.The updater function contains sophisticated Relay store manipulation logic that handles both user removal and room updates. While the implementation appears correct, adding inline comments would improve maintainability.
Consider adding comments like:
updater: ( store: RecordSourceSelectorProxy<unknown>, data: useRoomListSubscription$data | null | undefined, ) => { const roomId = data?.chatRoomOnRoomUpdate?.room?.node?.id if (!roomId) return + + // If user was removed from the room, delete it from all connections if (wasRemovedFromChatRoom(data, profileId)) { getChatRoomConnections(store, profileId).forEach((connectionRecord) => ConnectionHandler.deleteNode(connectionRecord, roomId), ) } else { + // Otherwise, update the room by re-inserting it to maintain proper order const isArchived = data?.chatRoomOnRoomUpdate?.room?.node?.isArchived getChatRoomConnections( store, profileId, + // Only update connections that match the room's archived status ({ q, archived }) => q === '' && archived === isArchived, ).forEach((connectionRecord) => { + // Remove old edge and insert updated one at the beginning ConnectionHandler.deleteNode(connectionRecord, roomId) const serverEdge = store.getRootField('chatRoomOnRoomUpdate')?.getLinkedRecord('room') const edge = ConnectionHandler.buildConnectionEdge(store, connectionRecord, serverEdge) if (edge) { ConnectionHandler.insertEdgeBefore(connectionRecord, edge) } }) } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/messages/common/graphql/subscriptions/useRoomListSubscription.tsx(2 hunks)packages/components/modules/messages/native/graphql/subscriptions/useRoomListSubscription.tsx(1 hunks)packages/components/modules/messages/native/index.ts(1 hunks)packages/components/package.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/components/modules/messages/common/graphql/subscriptions/useRoomListSubscription.tsx (1)
packages/components/modules/messages/common/utils.ts (1)
getChatRoomConnections(60-70)
🔇 Additional comments (9)
packages/components/modules/messages/common/graphql/subscriptions/useRoomListSubscription.tsx (2)
41-45: LGTM! Clean utility function extraction.The
wasRemovedFromChatRoomfunction is well-implemented with proper optional chaining and clear logic for checking if the current user was removed from the chat room.
105-109: Excellent refactoring for modularity.The extraction of subscription configuration into
getRoomListSubscriptionConfigsuccessfully modularizes the logic while maintaining the same behavior. TheuseMemousage is correct with proper dependencies.packages/components/package.json (1)
4-4: Appropriate version bump for new functionality.The minor version increment from 1.2.9 to 1.3.0 correctly reflects the addition of the new native
useRoomListSubscriptionhook implementation.packages/components/CHANGELOG.md (1)
3-7: Well-documented changelog entry.The changelog entry clearly describes the new native
useRoomListSubscriptionhook functionality and its lifecycle-aware behavior. The description effectively communicates the purpose and benefits of the implementation.packages/components/modules/messages/native/index.ts (1)
3-3: Correct export addition for the new native hook.The export follows the established pattern and properly exposes the new
useRoomListSubscriptionhook to the module's public API.packages/components/modules/messages/native/graphql/subscriptions/useRoomListSubscription.tsx (4)
15-34: Well-structured hook implementation.The hook signature is appropriate with necessary parameters, and the setup correctly uses the refactored
getRoomListSubscriptionConfigfunction. The disposable ref pattern and memoization are properly implemented.
36-45: Proper subscription lifecycle management.The subscribe/unsubscribe functions correctly handle Relay subscription disposal to prevent memory leaks. The profileId guard and useCallback usage are appropriate.
47-54: Correct focus-based subscription management.The
useFocusEffectimplementation properly subscribes when the screen gains focus and unsubscribes on blur, following React Navigation best practices for lifecycle-aware subscriptions.
56-58: Addresses iOS WebSocket suspension issues effectively.The
useAppStateSubscriptionusage correctly handles the case where WebSocket connections are suspended when the app goes to background, re-establishing the subscription when the app returns to the foreground. This directly addresses the PR objectives.
gabrieleandro
deleted the
feature/react-native-roomlist-subscriptions-improvement
branch
Implements native
useRoomListSubscriptionHookEnhanced the subscription lifecycle by subscribing and unsubscribing based on the app's active/inactive state.
Ensures real-time updates are re-established automatically after app resumes from the background or device unlock.
Prevents missed events or stale subscriptions due to suspended WebSocket connections on iOS.