[ECO-5184] feat: update channel hooks implementation #1947
Conversation
WalkthroughThe pull request introduces a new optional Changes
Assessment against linked issues
Poem
✨ Finishing Touches
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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/1947/features
github-actions
bot
temporarily deployed
to
staging/pull/1947/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1947/bundle-report
![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
🧹 Nitpick comments (2)
ably.d.ts (1)
896-902: LGTM! Consider enhancing the documentation.The new
attachOnSubscribeproperty is well-defined and aligns with the PR objectives to address implicitattach()hooks. The default value and scope are clearly documented.Consider adding:
- A link to the channel attachment documentation
- Examples showing the behavior with both
trueandfalsevalues/** * A boolean which determines whether calling subscribe * on a channel or presence object should trigger an implicit attach. Defaults to `true` * + * @see https://ably.com/docs/realtime/channels#attach + * @example + * ```typescript + * // Default behavior (attachOnSubscribe: true) + * const channel1 = client.channels.get('example', { attachOnSubscribe: true }); + * await channel1.subscribe('event', () => {}); // Implicitly attaches + * + * // Explicit attachment (attachOnSubscribe: false) + * const channel2 = client.channels.get('example', { attachOnSubscribe: false }); + * await channel2.subscribe('event', () => {}); // Does not attach + * await channel2.attach(); // Explicit attach required + * ``` * * Note: this option is for realtime client libraries only */src/platform/react-hooks/src/hooks/usePresence.ts (1)
Line range hint
63-71: Consider using constants for channel states in cleanup function.The cleanup function uses hardcoded channel states
['attached', 'attaching']while the rest of the code uses imported constants. Consider defining these states in the constants file for consistency.- const canLeaveFromChannelState = ['attached', 'attaching'].includes(channel.state); + const ACTIVE_CHANNEL_STATES = ['attached', 'attaching'] as const; + const canLeaveFromChannelState = ACTIVE_CHANNEL_STATES.includes(channel.state);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ably.d.ts(1 hunks)src/common/lib/client/realtimechannel.ts(1 hunks)src/common/lib/client/realtimepresence.ts(1 hunks)src/platform/react-hooks/src/AblyReactHooks.ts(1 hunks)src/platform/react-hooks/src/ChannelProvider.tsx(3 hunks)src/platform/react-hooks/src/hooks/constants.ts(1 hunks)src/platform/react-hooks/src/hooks/usePresence.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/platform/react-hooks/src/hooks/constants.ts
🧰 Additional context used
🪛 GitHub Actions: Test (react hooks)
src/platform/react-hooks/src/ChannelProvider.tsx
[error] 57-57: TypeError: channel.attach is not a function. The channel object does not have the expected attach method, causing test failures in React hooks tests.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-npm-package
- GitHub Check: test-node (16.x)
🔇 Additional comments (10)
src/platform/react-hooks/src/AblyReactHooks.ts (1)
17-25: LGTM! The changes align with the PR objectives.The modifications to
channelOptionsWithAgentcorrectly implement the requirement to make channel attachment explicit by:
- Adding explicit return type for better type safety
- Setting
attachOnSubscribe: falseto prevent implicit channel attachmentsrc/platform/react-hooks/src/ChannelProvider.tsx (3)
1-6: LGTM! Dependencies are properly imported.The new imports support the connection state management functionality.
13-13: LGTM! New skip property added.The
skipproperty enhances control over channel attachment behavior.
27-30: LGTM! Connection state management implementation.The implementation properly tracks and updates the connection state.
src/common/lib/client/realtimepresence.ts (1)
457-461: LGTM! Conditional channel attachment implementation.The implementation correctly:
- Checks
attachOnSubscribeoption before attaching- Includes traceability comment
(RTP6d)src/common/lib/client/realtimechannel.ts (1)
436-441: LGTM! Conditional channel attachment implementation.The implementation correctly:
- Checks
attachOnSubscribeoption before attaching- Includes traceability comment
(RTL7g)- Returns appropriate value based on the condition
ably.d.ts (1)
896-902: Verify the impact on existing channel subscriptions.Let's check the codebase for any potential issues with the new
attachOnSubscribebehavior.✅ Verification successful
Safe addition with backward compatibility maintained
The new
attachOnSubscribeoption is a safe addition because:
- It's optional and defaults to
true, preserving existing behavior- Existing code will continue to work without modification
- The behavior change only affects code that explicitly opts out
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing channel subscriptions that might be affected by the new attachOnSubscribe behavior # Test 1: Look for channel subscriptions that might rely on implicit attachment echo "Checking for channel subscriptions..." rg -A 2 "subscribe\(" --type ts --type js # Test 2: Look for explicit channel attachments echo "Checking for explicit channel attachments..." rg "attach\(\)" --type ts --type js # Test 3: Look for channel options usage echo "Checking for channel options usage..." ast-grep --pattern 'channels.get($_, { $$ })'Length of output: 58148
src/platform/react-hooks/src/hooks/usePresence.ts (3)
9-9: LGTM! Good refactoring of constants.Moving the constants to a separate module improves maintainability and reusability across the codebase.
Line range hint
31-34: Verify the impact of skipping presence updates.The current implementation stores
messageOrPresenceObjectin a ref to prevent infinite loops, but this means changes to the object won't trigger a presence update. This could lead to stale presence data.Consider implementing one of these solutions:
- Add deep equality comparison (as mentioned in issue Have 'messageOrPresenceObject' parameter passed to
usePresencehook as a dependency in itsuseEffect/useCallbackcalls #1688)- Document this limitation clearly in the hook's JSDoc
- Add a warning when the object changes but presence isn't updated
Would you like me to help implement any of these solutions?
Line range hint
18-24: Verify integration with new attachment behavior.Given that the PR introduces
attachOnSubscribe = falsefor channels, we should verify that this presence hook correctly handles the new explicit attachment behavior.✅ Verification successful
Hook correctly handles attachment states
The usePresence hook properly integrates with channel state management through useChannelInstance and includes appropriate state checks before presence operations. It's compatible with the new explicit attachment behavior as it:
- Respects channel states through INACTIVE_CHANNEL_STATES
- Delegates channel management to useChannelInstance
- Implements proper state monitoring
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of attachOnSubscribe in related files rg -l "attachOnSubscribe" --type ts # Check if any presence-related tests need to be updated rg -l "usePresence.*test" --type ts # Look for potential attachment-related edge cases ast-grep --pattern 'channel.attach($$$)'Length of output: 7451
Script:
#!/bin/bash # Check usePresence implementation and its relationship with attachOnSubscribe ast-grep --pattern 'function usePresence<T>($$$)' # Check AblyReactHooks configuration handling ast-grep --pattern 'attachOnSubscribe.*=.*false' # Look for presence-related attachment logic in RealtimePresence rg -A 5 "enter|update|leave.*channel\.attach" src/common/lib/client/realtimepresence.tsLength of output: 3462
Script:
#!/bin/bash # Check usePresence hook implementation and channel state handling rg -A 10 "usePresence.*=.*{" src/platform/react-hooks/src/hooks/usePresence.ts # Check if usePresence uses ChannelProvider rg -A 5 "useChannel|ChannelProvider" src/platform/react-hooks/src/hooks/usePresence.ts # Look for presence state management rg -A 5 "useState.*Presence|useEffect.*presence" src/platform/react-hooks/src/hooks/usePresence.tsLength of output: 1537
| const shouldAttachToTheChannel = !skip && !INACTIVE_CONNECTION_STATES.includes(connectionState); | ||
|
|
||
| useLayoutEffect(() => { | ||
| if (shouldAttachToTheChannel) { | ||
| channel.attach(); | ||
| } | ||
|
|
||
| return () => { | ||
| if (channel.state !== 'failed') { | ||
| channel.detach(); | ||
| } | ||
| }; | ||
| }, [shouldAttachToTheChannel, channel]); |
There was a problem hiding this comment.
Fix the channel attachment implementation.
The test failure indicates that channel.attach is not a function. This suggests that either:
- The channel object is not properly initialized, or
- The channel object doesn't have the expected interface.
Verify the channel initialization and interface:
#!/bin/bash
# Search for channel initialization and interface definition
ast-grep --pattern 'interface Channel {
$$$
attach(): $_;
$$$
}'
# Search for channel object creation
ast-grep --pattern 'channel = $_.channels.get($name)'
ttypic
force-pushed
the
ECO-5184/channel-hooks-improvements
branch
from
1758e16 to
ce8e9af
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1947/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1947/features
github-actions
bot
temporarily deployed
to
staging/pull/1947/typedoc
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/platform/react-hooks/src/hooks/useChannelAttach.ts (2)
27-29: Consider optimizing state synchronization.The manual state sync check outside
useEffectcould cause unnecessary re-renders. Consider moving this logic into theuseConnectionStateListenercallback.- if (ably.connection.state !== connectionState) { - setConnectionState(ably.connection.state); - } useConnectionStateListener((stateChange) => { + if (ably.connection.state !== stateChange.current) { setConnectionState(stateChange.current); + } }, ablyId);
35-39: Enhance error handling for better debugging.The current fire-and-forget approach with basic console logging could make debugging difficult in production.
- channel.attach().catch((reason) => { - // we use a fire-and-forget approach for attaching, but calling detach during the attaching process or while - // suspending can cause errors that will be automatically resolved - console.log(reason); - }); + channel.attach().catch((error: Ably.ErrorInfo) => { + // Log structured error information for better debugging + console.warn( + `Channel attachment failed:`, + { + code: error.code, + statusCode: error.statusCode, + message: error.message, + cause: error.cause, + channelState: channel.state, + } + ); + });src/platform/react-hooks/src/AblyReactHooks.ts (1)
17-26: Add a comment explaining theattachOnSubscribesetting.The function name change better reflects its purpose. However, it would be helpful to add a comment explaining why
attachOnSubscribeis set tofalsehere, as it gives explicit control over channel attachment to theChannelProvider.export function channelOptionsForReactHooks(options?: Ably.ChannelOptions): Ably.ChannelOptions { + // Set attachOnSubscribe to false to give explicit control over channel attachment to the ChannelProvider return { ...options, params: { ...options?.params, agent: `react-hooks/${version}`, }, attachOnSubscribe: false, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
ably.d.ts(1 hunks)src/common/lib/client/realtimechannel.ts(1 hunks)src/common/lib/client/realtimepresence.ts(1 hunks)src/platform/react-hooks/src/AblyReactHooks.ts(1 hunks)src/platform/react-hooks/src/ChannelProvider.tsx(2 hunks)src/platform/react-hooks/src/fakes/ably.ts(3 hunks)src/platform/react-hooks/src/hooks/constants.ts(1 hunks)src/platform/react-hooks/src/hooks/useChannel.ts(2 hunks)src/platform/react-hooks/src/hooks/useChannelAttach.test.tsx(1 hunks)src/platform/react-hooks/src/hooks/useChannelAttach.ts(1 hunks)src/platform/react-hooks/src/hooks/usePresence.ts(2 hunks)src/platform/react-hooks/src/hooks/usePresenceListener.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/platform/react-hooks/src/ChannelProvider.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/platform/react-hooks/src/hooks/constants.ts
- src/common/lib/client/realtimepresence.ts
- src/common/lib/client/realtimechannel.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-npm-package
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (16.x)
🔇 Additional comments (6)
src/platform/react-hooks/src/hooks/useChannelAttach.ts (1)
1-15: LGTM! Well-structured hook interface.The hook parameters and return type are well-defined, providing clear control over channel attachment behavior.
src/platform/react-hooks/src/hooks/usePresenceListener.ts (1)
68-68: LGTM! Proper integration of useChannelAttach.The hook is correctly integrated with the same parameters used throughout the component.
src/platform/react-hooks/src/hooks/useChannel.ts (1)
86-86: LGTM! Proper integration of useChannelAttach.The hook is correctly integrated with the same parameters used throughout the component.
src/platform/react-hooks/src/fakes/ably.ts (1)
166-169: LGTM! Test doubles for newattach()methods.The empty async
attach()methods added to the fake implementations align with the newattachOnSubscribebehavior. The implementation is appropriate for test doubles.Also applies to: 206-209, 359-362
src/platform/react-hooks/src/hooks/usePresence.ts (1)
8-9: LGTM! Improved code organization and attachment handling.Good improvements:
- Moving constants to a separate file reduces duplication
- Using
useChannelAttachhook aligns with the new explicit attachment behaviorAlso applies to: 49-50
ably.d.ts (1)
896-902: LGTM! Well-documented type definition forattachOnSubscribe.The new
attachOnSubscribeproperty is:
- Well-documented with clear description
- Properly typed as optional boolean
- Explicitly scoped to realtime client libraries
ttypic
force-pushed
the
ECO-5184/channel-hooks-improvements
branch
from
ce8e9af to
dd948d4
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1947/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1947/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1947/features
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/platform/react-hooks/src/hooks/useChannelAttach.ts (2)
18-25: Consider usinguseCallbackfor the state change handler.The state change handler function is recreated on every render. While this might not cause performance issues, using
useCallbackwould follow React best practices.- useConnectionStateListener((stateChange) => { - setConnectionState(stateChange.current); - }, ablyId); + useConnectionStateListener( + useCallback((stateChange) => { + setConnectionState(stateChange.current); + }, []), + ablyId + );
35-39: Enhance error logging for debugging.The current error logging is too basic. Consider adding more context about the error and the channel state.
- console.log(reason); + console.warn( + `Channel attachment failed for channel ${channel.name}:`, + reason, + `Current channel state: ${channel.state}` + );src/platform/react-hooks/src/hooks/usePresence.ts (1)
Line range hint
71-77: Consider extracting the leave condition logic.The leave condition checks are complex and could benefit from being extracted into a named function for better readability and maintainability.
+ const canLeavePresence = (connection: Ably.Connection, channel: Ably.RealtimeChannel) => { + const canLeaveFromConnectionState = !INACTIVE_CONNECTION_STATES.includes(connection.state); + const canLeaveFromChannelState = ['attached', 'attaching'].includes(channel.state); + return canLeaveFromChannelState && canLeaveFromConnectionState; + }; + - const canLeaveFromConnectionState = !INACTIVE_CONNECTION_STATES.includes(ably.connection.state); - const canLeaveFromChannelState = ['attached', 'attaching'].includes(channel.state); - if (canLeaveFromChannelState && canLeaveFromConnectionState) { + if (canLeavePresence(ably.connection, channel)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
ably.d.ts(1 hunks)src/common/lib/client/realtimechannel.ts(1 hunks)src/common/lib/client/realtimepresence.ts(1 hunks)src/platform/react-hooks/src/AblyReactHooks.ts(1 hunks)src/platform/react-hooks/src/ChannelProvider.tsx(2 hunks)src/platform/react-hooks/src/fakes/ably.ts(3 hunks)src/platform/react-hooks/src/hooks/constants.ts(1 hunks)src/platform/react-hooks/src/hooks/useChannel.ts(2 hunks)src/platform/react-hooks/src/hooks/useChannelAttach.test.tsx(1 hunks)src/platform/react-hooks/src/hooks/useChannelAttach.ts(1 hunks)src/platform/react-hooks/src/hooks/usePresence.ts(2 hunks)src/platform/react-hooks/src/hooks/usePresenceListener.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/platform/react-hooks/src/ChannelProvider.tsx
- src/platform/react-hooks/src/fakes/ably.ts
- src/common/lib/client/realtimepresence.ts
- src/platform/react-hooks/src/AblyReactHooks.ts
- src/platform/react-hooks/src/hooks/constants.ts
- src/platform/react-hooks/src/hooks/useChannelAttach.test.tsx
- src/common/lib/client/realtimechannel.ts
- ably.d.ts
- src/platform/react-hooks/src/hooks/useChannel.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (4)
src/platform/react-hooks/src/hooks/useChannelAttach.ts (1)
11-15: LGTM! Clear and well-typed hook signature.The hook parameters are well-defined and properly typed, allowing for flexible control over channel attachment.
src/platform/react-hooks/src/hooks/usePresenceListener.ts (1)
68-68: LGTM! Proper integration of channel attachment.The
useChannelAttachhook is correctly integrated, ensuring explicit channel attachment control as intended by the PR objectives.src/platform/react-hooks/src/hooks/usePresence.ts (2)
49-50: LGTM! Improved state management.The integration of
useChannelAttacheffectively replaces the previous connection state management, aligning with the PR's goal of explicit attachment control.
52-53: Verify the order of state checks.The order of checks in
shouldNotEnterPresencemight affect the behavior. Consider documenting why this specific order was chosen.✅ Verification successful
The order of state checks is correct and follows a logical hierarchy
The order (connection → channel → skip) follows the natural dependency chain where connection state is a prerequisite for channel operations, preventing unnecessary checks and potential errors. This is consistent with the cleanup logic which applies the same principles, as evidenced by the comments explaining that presence operations require specific connection and channel states.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar patterns of state checks in the codebase rg -A 2 "INACTIVE_CONNECTION_STATES.includes.*INACTIVE_CHANNEL_STATES.includes"Length of output: 368
Script:
#!/bin/bash # Find definitions of the constants ast-grep --pattern 'const INACTIVE_CONNECTION_STATES = $_' ast-grep --pattern 'const INACTIVE_CHANNEL_STATES = $_' # Get broader context of usePresence hook rg -B 5 -A 10 "shouldNotEnterPresence|INACTIVE_CONNECTION_STATES|INACTIVE_CHANNEL_STATES" src/platform/react-hooks/src/hooks/usePresence.tsLength of output: 2489
| if (shouldAttachToTheChannel) { | ||
| channel.attach().catch((reason) => { | ||
| // we use a fire-and-forget approach for attaching, but calling detach during the attaching process or while | ||
| // suspending can cause errors that will be automatically resolved |
There was a problem hiding this comment.
nice, this keeps the behavior with channel attachment same as before, but we now have a better contol over when we should attach to channel, instead of doing it any subscribe/presence.enter call
|
I suppose what was the nice thing about the |
ttypic
force-pushed
the
ECO-5184/channel-hooks-improvements
branch
from
dd948d4 to
fd6d136
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1947/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1947/features
github-actions
bot
temporarily deployed
to
staging/pull/1947/typedoc
|
Thanks a lot for the review, @VeskeR !
yes, that was the intention! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/platform/react-hooks/src/hooks/usePresence.ts (1)
52-53: Well-handled cleanup with proper state checks.The implementation properly handles presence cleanup by checking both connection and channel states before leaving presence. This prevents exceptions during disconnection or detachment scenarios.
However, consider adding debug logging to track presence leave attempts and their outcomes:
if (canLeaveFromChannelState && canLeaveFromConnectionState) { + console.debug('[Ably Hooks] Leaving presence', { channelState: channel.state, connectionState: ably.connection.state }); channel.presence.leave(); }Also applies to: 65-74
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
ably.d.ts(1 hunks)src/common/lib/client/realtimechannel.ts(1 hunks)src/common/lib/client/realtimepresence.ts(1 hunks)src/platform/react-hooks/src/AblyReactHooks.ts(1 hunks)src/platform/react-hooks/src/ChannelProvider.tsx(2 hunks)src/platform/react-hooks/src/fakes/ably.ts(3 hunks)src/platform/react-hooks/src/hooks/constants.ts(1 hunks)src/platform/react-hooks/src/hooks/useChannel.ts(2 hunks)src/platform/react-hooks/src/hooks/useChannelAttach.test.tsx(1 hunks)src/platform/react-hooks/src/hooks/useChannelAttach.ts(1 hunks)src/platform/react-hooks/src/hooks/usePresence.ts(2 hunks)src/platform/react-hooks/src/hooks/usePresenceListener.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/platform/react-hooks/src/ChannelProvider.tsx
- src/platform/react-hooks/src/fakes/ably.ts
- src/common/lib/client/realtimepresence.ts
- src/platform/react-hooks/src/hooks/constants.ts
- src/platform/react-hooks/src/AblyReactHooks.ts
- src/common/lib/client/realtimechannel.ts
- src/platform/react-hooks/src/hooks/useChannel.ts
- ably.d.ts
- src/platform/react-hooks/src/hooks/useChannelAttach.ts
- src/platform/react-hooks/src/hooks/useChannelAttach.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (2)
src/platform/react-hooks/src/hooks/usePresenceListener.ts (1)
68-68: Consider hook ordering impact.The
useChannelAttachhook is called after theuseEffectthat sets up presence subscription. While a previous review noted this might not matter, it's worth verifying if the order could impact the subscription behavior, especially during initial mount or reconnection scenarios.Let's verify the hook dependencies and potential race conditions:
✅ Verification successful
Hook ordering is correct and safe
The current implementation is safe because:
useChannelAttachmanages channel attachment independently on component mount- Effects properly react to connection state changes
- Test coverage shows correct handling of various states and transitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of useChannelAttach to understand typical ordering rg -B 3 -A 3 "useChannelAttach\(" # Look for any related issues or discussions rg -l "race condition.*presence|presence.*race condition"Length of output: 5665
src/platform/react-hooks/src/hooks/usePresence.ts (1)
49-50: LGTM! Good consolidation of connection state management.Replacing
useConnectionStateListenerwithuseChannelAttacheffectively consolidates the connection state management, aligning with the PR's goal of explicit channel attachment control.
Because of implicit `attach()` hooks can produce additional errors e.g. when updating ably client to avoid this situation, channels created inside `ChannelProvider` now use `attachOnSubscribe = false` flag and attach is happening explicitly.
ttypic
force-pushed
the
ECO-5184/channel-hooks-improvements
branch
from
fd6d136 to
fdf0fb5
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1947/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1947/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1947/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/platform/react-hooks/src/hooks/usePresence.ts (2)
49-50: Great architectural improvement!The switch to
useChannelAttachaligns perfectly with the PR objectives by providing explicit control over the attachment lifecycle. This change not only addresses the current issues with implicitattach()hooks but also sets up the foundation for future enhancements like the "detach on unmount" feature.
Line range hint
65-73: Consider consolidating the cleanup conditions.The cleanup function's conditions could be simplified to improve readability and maintainability.
Consider this refactoring:
- const canLeaveFromConnectionState = !INACTIVE_CONNECTION_STATES.includes(ably.connection.state); - const canLeaveFromChannelState = ['attached', 'attaching'].includes(channel.state); - if (canLeaveFromChannelState && canLeaveFromConnectionState) { + const isConnectionActive = !INACTIVE_CONNECTION_STATES.includes(ably.connection.state); + const isChannelActive = !INACTIVE_CHANNEL_STATES.includes(channel.state); + if (isConnectionActive && isChannelActive) { channel.presence.leave(); }This change:
- Uses consistent naming patterns
- Reuses the
INACTIVE_CHANNEL_STATESconstant for consistency- Makes the conditions more intuitive to read
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
ably.d.ts(1 hunks)src/common/lib/client/realtimechannel.ts(1 hunks)src/common/lib/client/realtimepresence.ts(1 hunks)src/platform/react-hooks/src/AblyReactHooks.ts(1 hunks)src/platform/react-hooks/src/ChannelProvider.tsx(2 hunks)src/platform/react-hooks/src/fakes/ably.ts(3 hunks)src/platform/react-hooks/src/hooks/constants.ts(1 hunks)src/platform/react-hooks/src/hooks/useChannel.ts(2 hunks)src/platform/react-hooks/src/hooks/useChannelAttach.test.tsx(1 hunks)src/platform/react-hooks/src/hooks/useChannelAttach.ts(1 hunks)src/platform/react-hooks/src/hooks/usePresence.ts(2 hunks)src/platform/react-hooks/src/hooks/usePresenceListener.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/platform/react-hooks/src/ChannelProvider.tsx
- src/platform/react-hooks/src/hooks/constants.ts
- src/platform/react-hooks/src/fakes/ably.ts
- src/platform/react-hooks/src/hooks/usePresenceListener.ts
- src/common/lib/client/realtimepresence.ts
- src/platform/react-hooks/src/AblyReactHooks.ts
- src/common/lib/client/realtimechannel.ts
- src/platform/react-hooks/src/hooks/useChannelAttach.test.tsx
- src/platform/react-hooks/src/hooks/useChannel.ts
- src/platform/react-hooks/src/hooks/useChannelAttach.ts
- ably.d.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (1)
src/platform/react-hooks/src/hooks/usePresence.ts (1)
8-9: LGTM! Good refactoring of constants.Moving the constants to a separate file improves code maintainability and reusability across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/platform/react-hooks/src/hooks/useChannelAttach.ts (1)
28-30:⚠️ Potential issueFix potential race condition in connection state synchronization.
The direct state check and update outside of useEffect could race with the state updates from the listener.
Move the state synchronization into an effect:
- if (ably.connection.state !== connectionState) { - setConnectionState(ably.connection.state); - } + useEffect(() => { + if (ably.connection.state !== connectionState) { + setConnectionState(ably.connection.state); + } + }, [ably.connection.state, connectionState]);
🧹 Nitpick comments (3)
src/platform/react-hooks/src/utils.ts (2)
4-4: Consider adding type safety for the logger access.The
anytype could be replaced with a more specific type since the Logger class is exposed on the BaseClient.-@param ablyClient ably core SDK client, it has any type because we access internal Logger class +@param ablyClient ably core SDK client with Logger accessinterface AblyWithLogger { Logger: { logAction: (logger: any, level: string, message: string) => void; LOG_ERROR: string; }; logger: any; }
9-9: Consider using LOG_WARN for automatically resolvable errors.Based on the past review discussion, since these errors are automatically resolved, consider using LOG_WARN instead of LOG_ERROR.
- ablyClient.Logger.logAction(ablyClient.logger, ablyClient.Logger.LOG_ERROR, `[react-hooks] ${message}`); + ablyClient.Logger.logAction(ablyClient.logger, ablyClient.Logger.LOG_WARN, `[react-hooks] ${message}`);src/platform/react-hooks/src/hooks/useChannelAttach.ts (1)
36-40: Consider enhancing error handling for attach failures.The current fire-and-forget approach with error logging might miss important error states that could affect the application's behavior.
Consider adding error state tracking:
const [attachError, setAttachError] = useState<Error | null>(null); // In the effect channel.attach().catch((reason) => { logError(ably, reason.toString()); setAttachError(reason); }); // Return error state return { connectionState, attachError };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/platform/react-hooks/src/hooks/useChannelAttach.ts(1 hunks)src/platform/react-hooks/src/utils.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
src/platform/react-hooks/src/hooks/useChannelAttach.ts
[warning] 42-42:
React Hook useEffect has a missing dependency: 'ably'. Either include it or remove the dependency array
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (1)
src/platform/react-hooks/src/hooks/useChannelAttach.ts (1)
32-32: LGTM: Clear condition for channel attachment.The condition effectively prevents attachment during inactive connection states, which aligns with the PR's objective of explicit attachment control.
| useEffect(() => { | ||
| if (shouldAttachToTheChannel) { | ||
| channel.attach().catch((reason) => { | ||
| // we use a fire-and-forget approach for attaching, but calling detach during the attaching process or while | ||
| // suspending can cause errors that will be automatically resolved | ||
| logError(ably, reason.toString()); | ||
| }); | ||
| } | ||
| }, [shouldAttachToTheChannel, channel]); |
There was a problem hiding this comment.
Add missing dependency to useEffect.
The effect uses ably from the outer scope but it's not included in the dependencies array.
useEffect(() => {
if (shouldAttachToTheChannel) {
channel.attach().catch((reason) => {
logError(ably, reason.toString());
});
}
- }, [shouldAttachToTheChannel, channel]);
+ }, [shouldAttachToTheChannel, channel, ably]);📝 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 (shouldAttachToTheChannel) { | |
| channel.attach().catch((reason) => { | |
| // we use a fire-and-forget approach for attaching, but calling detach during the attaching process or while | |
| // suspending can cause errors that will be automatically resolved | |
| logError(ably, reason.toString()); | |
| }); | |
| } | |
| }, [shouldAttachToTheChannel, channel]); | |
| useEffect(() => { | |
| if (shouldAttachToTheChannel) { | |
| channel.attach().catch((reason) => { | |
| // we use a fire-and-forget approach for attaching, but calling detach during the attaching process or while | |
| // suspending can cause errors that will be automatically resolved | |
| logError(ably, reason.toString()); | |
| }); | |
| } | |
| }, [shouldAttachToTheChannel, channel, ably]); |
🧰 Tools
🪛 GitHub Check: lint
[warning] 42-42:
React Hook useEffect has a missing dependency: 'ably'. Either include it or remove the dependency array
Because of implicit
attach()hooks can produce additional errors e.g. when updating ably clientto avoid this situation, channels created inside
ChannelProvidernow useattachOnSubscribe = falseflag and attach is happening explicitly.Summary by CodeRabbit
New Features
attachOnSubscribeconfiguration for more flexible channel subscription behavior.useChannelAttachhook for managing channel attachment to the Ably Realtime service.attachmethod to multiple classes for future functionality.Refactor
Tests
useChannelAttachhook to ensure correct behavior.