[ECO-5002] Fix usePresence won't leave presence if unmount triggered during channel attaching #1884
Conversation
…nel attaching This fixes a race condition issue, where a React app might trigger a component unmount while channel is still in the `attaching` state, thus `presence.leave` was never called for a channel and member would remain there until app reload.
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/platform/react-hooks/src/hooks/usePresence.ts (1 hunks)
🔇 Additional comments (1)
src/platform/react-hooks/src/hooks/usePresence.ts (1)
Line range hint
1-95: Overall assessment: Good fix with room for further improvementsThe changes effectively address the race condition described in the PR objectives. By including the 'attaching' state in the channel state check, the hook now correctly handles the scenario where a component unmounts during channel attachment.
However, there are a few additional considerations:
Error Handling: Consider adding error handling around the
channel.presence.leave()call. Although we're checking states, unexpected errors could still occur.Testing: Ensure that unit tests are updated or added to cover this new behavior, especially the edge case of unmounting during channel attachment.
Documentation: Update the hook's documentation to reflect this change in behavior, particularly noting that it now handles unmounting during channel attachment.
Performance: While the current implementation is correct, frequently checking connection and channel states might impact performance in high-frequency update scenarios. Consider implementing a memoization strategy if this becomes an issue.
Consistency: Ensure that this change is consistently applied across other similar hooks or components that deal with presence.
To further improve the robustness of this hook, consider implementing a state machine pattern for managing the lifecycle of presence. This could provide a more structured approach to handling various states and transitions, making the code more maintainable and easier to reason about as complexity grows.
To ensure consistency across the codebase, let's check for similar patterns:
This will help identify if similar changes need to be applied elsewhere in the codebase.
✅ Verification successful
Verification Successful: No additional instances of
presence.leavefound in production code.The usage of
channel.presence.leave()inusePresence.tsis consistent, and no other instances were found in the production codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar presence leave patterns in other files rg --type typescript 'presence\.leave' -C 5Length of output: 80
Script:
#!/bin/bash # Search for similar presence leave patterns in other TypeScript files rg 'presence\.leave' -g '*.ts' -g '*.tsx' -C 5Length of output: 2584
VeskeR
deleted the
ECO-5002/fix-usePresence-channel-name-race-condition
branch
This fixes a race condition issue, where a React app might trigger a component unmount while channel is still in the
attachingstate, thuspresence.leavewas never called for a channel and member would remain there until app reload.Partially fixes https://ably.atlassian.net/browse/ECO-5002, see more context in the internal slack thread.
Summary by CodeRabbit