Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRealtimeChannel now always invokes the LiveObjects objects re-sync after receiving an ATTACHED protocol message when the Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
e95e32e to
06b746a
Compare
mschristensen
left a comment
There was a problem hiding this comment.
LGTM, just a small comment
That is, do it when we get a discontinuity, not when a new sync sequence changes, per spec changes in [1]. Integration tests ported from JS in [2] at 06b746a. All written by Claude. [1] ably/specification#416 [2] ably/ably-js#2150
That is, do it when we get a discontinuity, not when a new sync sequence starts, per spec changes in [1]. Integration tests ported from JS in [2] at 06b746a. All written by Claude. [1] ably/specification#416 [2] ably/ably-js#2150
06b746a to
469a562
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/plugins/liveobjects/realtimeobject.ts (1)
205-216: Minor: buffer is cleared twice in the!hasObjectspath.Line 205 sets
_bufferedObjectOperations = [], then_endSync()(line 215) reassigns it to[]again at line 288. The second assignment is a no-op and causes no harm, but it adds slight noise to the_endSynccontract.♻️ Optional: skip redundant clear in the !hasObjects branch
this._bufferedObjectOperations = []; // RTO4a this._startNewSync(); // RTO4b if (!hasObjects) { // If no HAS_OBJECTS flag was received on attach, end the sync sequence immediately and treat it as no objects on the channel. // Reset the objects pool to its initial state and emit update events so subscribers to the root object are notified of changes. this._objectsPool.resetToInitialPool(true); // RTO4b1, RTO4b2 this._syncObjectsDataPool.clear(); // RTO4b3 this._endSync(); // RTO4b4 }The
_endSyncpath that calls_applyObjectMessages([])followed bythis._bufferedObjectOperations = []is already a no-op when the buffer is empty, so no behavioural change is needed — this is purely observational.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/liveobjects/realtimeobject.ts` around lines 205 - 216, The !_hasObjects branch redundantly clears the buffer by assigning this._bufferedObjectOperations = [] at the top of the method and again indirectly inside _endSync(); remove the first explicit clear in the !hasObjects path so that _startNewSync() and the subsequent _objectsPool.resetToInitialPool(true), _syncObjectsDataPool.clear(), and _endSync() are executed without the extra this._bufferedObjectOperations = [] assignment; keep _endSync()'s buffer reset (and any calls to _applyObjectMessages([]) there) as the single place that finalizes/clears the buffer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/liveobjects/realtimeobject.ts`:
- Around line 200-207: The onAttached handling currently clears
_bufferedObjectOperations unconditionally in realtimeobject.onAttached (calling
_startNewSync) even for resumed ATTACHEDs; change the behavior to match the PR
description by either (A) adding the same RESUMED guard used for presence in
realtimechannel before calling this._object.onAttached(hasObjects) so onAttached
is only invoked when resumed is false, or (B) extend onAttached to accept a
resumed flag (e.g., onAttached(hasObjects, resumed)) and only clear
this._bufferedObjectOperations / call _startNewSync when resumed is false;
update the callsite that invokes this._object.onAttached(...) accordingly.
---
Nitpick comments:
In `@src/plugins/liveobjects/realtimeobject.ts`:
- Around line 205-216: The !_hasObjects branch redundantly clears the buffer by
assigning this._bufferedObjectOperations = [] at the top of the method and again
indirectly inside _endSync(); remove the first explicit clear in the !hasObjects
path so that _startNewSync() and the subsequent
_objectsPool.resetToInitialPool(true), _syncObjectsDataPool.clear(), and
_endSync() are executed without the extra this._bufferedObjectOperations = []
assignment; keep _endSync()'s buffer reset (and any calls to
_applyObjectMessages([]) there) as the single place that finalizes/clears the
buffer.
469a562 to
d4d6223
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/realtime/liveobjects.test.js (1)
2603-2651: Same race as the previous scenario.Consider the same post-ATTACHED tick here to avoid flakiness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/realtime/liveobjects.test.js` around lines 2603 - 2651, The test has a race after injectAttachedMessage causing flakiness; after calling injectAttachedMessage(helper, channel, ...) ensure you wait one event loop tick before sending the next operation so the ATTACHED processing completes first. Add a short await (e.g. await helper.tick() or await Promise.resolve()) immediately after injectAttachedMessage and before the subsequent objectsHelper.processObjectOperationMessageOnChannel call (references: injectAttachedMessage, objectsHelper.processObjectOperationMessageOnChannel, helper.tick).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/realtime/liveobjects.test.js`:
- Around line 551-600: The test for "ATTACHED without HAS_OBJECTS" races because
it asserts events/state immediately after calling injectAttachedMessage; add an
explicit microtask tick before the assertions so async ATTACHED handling can
run. Specifically, after calling injectAttachedMessage(helper, channel, 0) await
the next tick (using nextTick or equivalent) before checking receivedEvents and
entryInstance.size(); this mirrors the earlier test's wait and prevents flakes
in realtimeObject and entryInstance assertions.
- Around line 2553-2599: The test races because ATTACHED processing is
asynchronous and the subsequent operation sent via
objectsHelper.processObjectOperationMessageOnChannel may run before the ATTACHED
handler has cleared buffers; after calling injectAttachedMessage(helper,
channel, 1 << 7) (the ATTACHED event), yield to the event loop (e.g., await a
microtask/tick or helper.yield if available) before sending the post-attach op
so that injectAttachedMessage completes and buffered ops are cleared; ensure the
await is placed between the injectAttachedMessage(...) call and the following
objectsHelper.processObjectOperationMessageOnChannel(...) call.
---
Duplicate comments:
In `@test/realtime/liveobjects.test.js`:
- Around line 2603-2651: The test has a race after injectAttachedMessage causing
flakiness; after calling injectAttachedMessage(helper, channel, ...) ensure you
wait one event loop tick before sending the next operation so the ATTACHED
processing completes first. Add a short await (e.g. await helper.tick() or await
Promise.resolve()) immediately after injectAttachedMessage and before the
subsequent objectsHelper.processObjectOperationMessageOnChannel call
(references: injectAttachedMessage,
objectsHelper.processObjectOperationMessageOnChannel, helper.tick).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/common/lib/client/realtimechannel.tssrc/plugins/liveobjects/realtimeobject.tstest/realtime/liveobjects.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/plugins/liveobjects/realtimeobject.ts
- src/common/lib/client/realtimechannel.ts
d4d6223 to
059be02
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/plugins/liveobjects/realtimeobject.ts (1)
210-217: Unconditional buffer clear is correct per spec; past concern addressed.The previous review flagged
_bufferedObjectOperations = []at line 215 as contradicting a "RESUMED=false-only" condition. The PR objectives explicitly state "cleared on every ATTACHED message" with no RESUMED guard — the comment at lines 210–214 now captures the rationale clearly. This is intentional behavior per spec.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/liveobjects/realtimeobject.ts` around lines 210 - 217, This review comment is a duplicate and the code intentionally clears the buffer on every ATTACHED per spec; mark or remove the duplicate review and ensure the explanatory comment above the assignments (_bufferedObjectOperations = [] and _startNewSync()) remains intact and clearly references ATTACHED/RESUMED behavior so future reviewers understand this is deliberate (no RESUMED guard).test/realtime/liveobjects.test.js (2)
2825-2834:⚠️ Potential issue | 🟡 MinorYield after ATTACHED before sending post-attach ops.
ATTACHED processing (buffer clearing) is async; sending the next op immediately can race. Add a tick after each ATTACHED.
💡 Suggested tweak
// any ATTACHED message must clear buffered operations and start a new sync sequence await injectAttachedMessage(helper, channel, 1 << 7); // HAS_OBJECTS flag is bit 7 + await new Promise((res) => nextTick(res)); // inject another operation that should be applied when sync ends await objectsHelper.processObjectOperationMessageOnChannel({// the RESUMED flag is irrelevant for LiveObjects buffering — any ATTACHED must clear // buffered operations and start a new sync sequence await injectAttachedMessage(helper, channel, (1 << 7) | (1 << 2)); // HAS_OBJECTS flag and RESUMED flag + await new Promise((res) => nextTick(res)); // inject another operation after ATTACHED await objectsHelper.processObjectOperationMessageOnChannel({Also applies to: 2875-2885
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/realtime/liveobjects.test.js` around lines 2825 - 2834, The test races because ATTACHED handling is async: after calling injectAttachedMessage(helper, channel, 1 << 7) you must yield to the event loop before sending post-attach ops so buffered-clearing finishes; fix by inserting an awaited microtask tick (e.g. await Promise.resolve() or await new Promise(r => setImmediate(r))) immediately after each injectAttachedMessage call before calling objectsHelper.processObjectOperationMessageOnChannel (same change for the other occurrence around the 2875-2885 area), referencing injectAttachedMessage and objectsHelper.processObjectOperationMessageOnChannel to locate the spots to update.
720-731:⚠️ Potential issue | 🟡 MinorAdd a microtask yield after ATTACHED without HAS_OBJECTS.
ATTACHED handling can be async; asserting immediately can race and flake. Insert a nextTick before checking events/state.
💡 Suggested tweak
// inject ATTACHED without HAS_OBJECTS flag await injectAttachedMessage(helper, channel, 0); // no HAS_OBJECTS flag + await new Promise((res) => nextTick(res)); // client should have transitioned through SYNCING to SYNCED expect(receivedEvents).to.deep.equal(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/realtime/liveobjects.test.js` around lines 720 - 731, The ATTACHED-without-HAS_OBJECTS test can race because ATTACHED handling is async; after calling injectAttachedMessage(helper, channel, 0) (the call that injects ATTACHED) insert a microtask yield before the assertions (i.e., await a next tick such as awaiting Promise.resolve() or process.nextTick wrapped in a promise) so the async handlers can run; place this yield immediately after the injectAttachedMessage call and before checking receivedEvents and entryInstance.size().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/plugins/liveobjects/realtimeobject.ts`:
- Around line 210-217: This review comment is a duplicate and the code
intentionally clears the buffer on every ATTACHED per spec; mark or remove the
duplicate review and ensure the explanatory comment above the assignments
(_bufferedObjectOperations = [] and _startNewSync()) remains intact and clearly
references ATTACHED/RESUMED behavior so future reviewers understand this is
deliberate (no RESUMED guard).
In `@test/realtime/liveobjects.test.js`:
- Around line 2825-2834: The test races because ATTACHED handling is async:
after calling injectAttachedMessage(helper, channel, 1 << 7) you must yield to
the event loop before sending post-attach ops so buffered-clearing finishes; fix
by inserting an awaited microtask tick (e.g. await Promise.resolve() or await
new Promise(r => setImmediate(r))) immediately after each injectAttachedMessage
call before calling objectsHelper.processObjectOperationMessageOnChannel (same
change for the other occurrence around the 2875-2885 area), referencing
injectAttachedMessage and objectsHelper.processObjectOperationMessageOnChannel
to locate the spots to update.
- Around line 720-731: The ATTACHED-without-HAS_OBJECTS test can race because
ATTACHED handling is async; after calling injectAttachedMessage(helper, channel,
0) (the call that injects ATTACHED) insert a microtask yield before the
assertions (i.e., await a next tick such as awaiting Promise.resolve() or
process.nextTick wrapped in a promise) so the async handlers can run; place this
yield immediately after the injectAttachedMessage call and before checking
receivedEvents and entryInstance.size().
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/common/lib/client/realtimechannel.tssrc/plugins/liveobjects/realtimeobject.tstest/realtime/liveobjects.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/common/lib/client/realtimechannel.ts
|
I think that the commit message here also needs updating a bit — it's still referring to things like Realtime server state caching; a link to spec and/or DR would now be more appropriate, I think. |
059be02 to
6e175ba
Compare
Updated 6e175ba |
…ests Protocol message processing involves multiple async steps; awaiting ensures all event handlers have settled before asserting.
Buffered object operations must be cleared on every ATTACHED message, not at the start of a new OBJECT_SYNC sequence. If HAS_OBJECTS is set on ATTACHED, the server will deliver a sync sequence following the attachment, guaranteeing that the objects in that sequence include at least all operations up to the attach point. If HAS_OBJECTS is not set, the client performs an implicit sync sequence by transitioning to SYNCING and immediately clearing local state. Read more in LODR-058 [1] and the specification change [2]. Resolves AIT-285 [1] https://ably.atlassian.net/wiki/x/NQAEHgE [2] ably/specification#416 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6e175ba to
9b2224d
Compare
Buffered object operations must be cleared on every ATTACHED message,
not at the start of a new OBJECT_SYNC sequence. If HAS_OBJECTS is set
on ATTACHED, the server will deliver a sync sequence following the
attachment, guaranteeing that the objects in that sequence include at
least all operations up to the attach point. If HAS_OBJECTS is not set,
the client performs an implicit sync sequence by transitioning to
SYNCING and immediately clearing local state.
Read more in LODR-058 [1] and the specification change [2].
Resolves AIT-285
[1] https://ably.atlassian.net/wiki/x/NQAEHgE
[2] ably/specification#416
Summary by CodeRabbit
Bug Fixes
Tests