Support RTP17g1: presence auto re-enter with a different connId #1913
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe changes involve modifications to the Changes
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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/1913/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1913/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1913/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
test/common/modules/shared_helper.js (2)
271-277: LGTM with a suggestion: Consider adding error handlingThe changes properly support both callback and promise-based patterns, improving the method's flexibility. However, consider adding error handling for potential connection state transition failures.
Consider wrapping the state transition in a try-catch block:
if (cb) { + try { realtime.connection.once('suspended', function () { cb(); }); + } catch (err) { + cb(err); + } } else { + try { return realtime.connection.once('suspended'); + } catch (err) { + return Promise.reject(err); + } }
Line range hint
260-277: Consider documenting the test helper's role in presence testingThese changes enhance the test helper's capability to simulate connection state transitions, which is crucial for testing presence auto re-entry scenarios. Consider adding documentation that explains how this helper should be used in presence-related tests, particularly for scenarios involving connection ID changes.
Add a JSDoc comment to explain the helper's role:
/** * Simulates a connection becoming suspended, which is useful for testing presence * re-entry scenarios, especially with different connection IDs. * @param {Realtime} realtime - The realtime client instance * @param {Function} [cb] - Optional callback, if not provided returns a promise * @returns {Promise<void>} - Returns a promise when no callback is provided */test/realtime/presence.test.js (3)
1714-1714: Unnecessary Transport ParameterremainPresentForThe
remainPresentFor: 5000parameter inhelper.AblyRealtime({transportParams: {remainPresentFor: 5000}});may not be necessary for this test or could affect other tests if not reset.Consider removing the parameter or adding a comment explaining its necessity.
1721-1721: Handle Potential Errors When Awaiting PromisesWhen calling
await channel.presence.get(), it's good practice to handle potential errors to prevent unhandled promise rejections.Consider wrapping the call in a try-catch block:
- await channel.presence.get() + try { + await channel.presence.get(); + } catch (err) { + // Handle error appropriately + }
1741-1741: Clarify Assertion Message for Message ID ComparisonThe assertion
assert.notEqual(enter.id, member1.id, 'Check the new enter did not have the msgId of the original');checks that the message IDs differ. Ensure that the reasoning behind this expectation is documented for future maintainability.Consider enhancing the assertion message or adding comments to explain why the message IDs should not be equal after reconnection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/common/lib/client/realtimepresence.ts (2 hunks)
- test/common/modules/shared_helper.js (2 hunks)
- test/realtime/presence.test.js (2 hunks)
🔇 Additional comments (4)
test/common/modules/shared_helper.js (1)
260-260: LGTM: Improved promise supportThe change enhances the method by properly propagating the promise from
_becomeSuspended, making it more compatible with async/await patterns while maintaining backward compatibility with callbacks.src/common/lib/client/realtimepresence.ts (1)
441-442: LGTM: Clean implementation of RTP17g1 specThe implementation correctly suppresses the member ID when the connection ID has changed, preventing stale member IDs from being used during presence re-entry.
test/realtime/presence.test.js (2)
1667-1674: Possible Logic Issue in Presence Synchronization CheckThe condition
if (channel.presence.syncComplete)checks if the presence synchronization is complete before callingchannel.sync(). This seems counterintuitive since you might want to synchronize only if it's not complete.Please verify whether the condition should be negated to ensure synchronization happens when it is incomplete:
- if (channel.presence.syncComplete) { + if (!channel.presence.syncComplete) {
1730-1737: Assumption About Connection ID Change May Not Always HoldThe test assumes that after reconnection, the connection ID will change due to suspension. However, if the suspension duration is short, the connection may resume with the same ID.
Please verify whether the test reliably reproduces the scenario where the connection ID changes. If not, consider simulating a longer suspension or adjusting the test to account for both possibilities.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
github-actions
bot
temporarily deployed
to
staging/pull/1913/features
should not have its id set
Co-authored-by: Owen Pearson <[email protected]>
SimonWoolf
force-pushed
the
auto-re-enter-diff-connid
branch
from
cf94386 to
acf45a0
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1913/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1913/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1913/features
SimonWoolf
force-pushed
the
auto-re-enter-diff-connid
branch
from
acf45a0 to
2d5323d
Compare
see ably/specification#215
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
async/awaitfor better readability and maintainability.