fix: useMessagesListSubscription hook #276
Conversation
vitorguima
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughThis change removes the React Native-specific Changes
Sequence Diagram(s)sequenceDiagram
participant Component (Native)
participant useMessagesListSubscription (Native)
participant Relay Environment
Component (Native) ->> useMessagesListSubscription (Native): Mount (with roomId, profileId, environment)
useMessagesListSubscription (Native) ->> useFocusEffect: Register focus callback
useFocusEffect -->> useMessagesListSubscription (Native): On focus
useMessagesListSubscription (Native) ->> Relay Environment: requestSubscription (start)
useFocusEffect -->> useMessagesListSubscription (Native): On unfocus/cleanup
useMessagesListSubscription (Native) ->> Relay Environment: Dispose subscription
Estimated code review effort2 (~15 minutes) Possibly related PRs
Suggested labels
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/subscriptions/useMessagesListSubscription.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/messages/native/index.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/messages/native/graphql/subscriptions/useMessagesListSubscription.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
✨ Finishing Touches
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 (
|
|
![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
🔭 Outside diff range comments (1)
packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx (1)
32-32: Dependency array may be incomplete.The
profileIdis used in thevariablesobject (line 27) but is missing from the dependency array. This could cause the config to not update whenprofileIdchanges.- }, [roomId]) + }, [roomId, profileId])
🧹 Nitpick comments (1)
packages/components/modules/messages/native/graphql/subscriptions/useMessagesListSubscription.tsx (1)
27-27: Consider enhanced error handling.While
console.errorprovides basic error logging, consider if additional error handling is needed for production environments (e.g., error reporting service, user notification).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx(1 hunks)packages/components/modules/messages/native/graphql/subscriptions/useMessagesListSubscription.tsx(1 hunks)packages/components/modules/messages/native/index.ts(1 hunks)packages/components/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
- GitHub Check: Component Test Packages
🔇 Additional comments (5)
packages/components/package.json (1)
4-4: Version bump looks appropriate.The minor version increment from 1.2.6 to 1.2.7 correctly reflects the abstraction of the React Native-specific hook, which is a backward-compatible change.
packages/components/CHANGELOG.md (1)
3-7: Changelog entry accurately documents the change.The entry clearly explains the abstraction of the React Native-specific hook and the reasoning behind it (preventing web import issues).
packages/components/modules/messages/native/index.ts (1)
1-2: Clean native module entry point.The barrel export pattern correctly establishes the entry point for native message components and follows standard conventions.
packages/components/modules/messages/native/graphql/subscriptions/useMessagesListSubscription.tsx (1)
8-46: Well-implemented native subscription hook.The implementation correctly:
- Uses
useFocusEffectfor React Native lifecycle management- Properly manages subscription cleanup with disposable refs
- Guards against missing required parameters
- Uses appropriate memoization for performance
packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx (1)
1-35: Clean abstraction of web-specific subscription logic.The removal of React Native-specific code and simplification to use
useSubscriptioncorrectly separates concerns between web and native implementations.
useMessagesListSubscriptionOnRnhook to native interface so it doesn't break web version due to import problems. In this case: expo-router won't be part of web applications so it will break when importing through the previous path.Summary by CodeRabbit