[AIT-287] Implement new rules for discarding ops buffered during sync#121
Merged
lawrence-forooghian merged 2 commits intomainfrom Mar 11, 2026
Merged
Conversation
WalkthroughRefines OBJECT operation buffering and sync state transitions: preserves buffered operations when continuing an existing sync sequence, clears buffers on channel ATTACHED or when starting a new sync, and exposes an internal test accessor for buffered count. Tests and integration calls updated to use flag-based ATTACHED messages. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
github-actions
bot
temporarily deployed
to
staging/pull/121/AblyLiveObjects
lawrence-forooghian
force-pushed
the
AIT-287-new-rules-for-discarding-buffered-events
branch
from
07ffcf9 to
047959f
Compare
github-actions
bot
temporarily deployed
to
staging/pull/121/AblyLiveObjects
lawrence-forooghian
force-pushed
the
AIT-287-new-rules-for-discarding-buffered-events
branch
from
047959f to
65dda76
Compare
github-actions
bot
temporarily deployed
to
staging/pull/121/AblyLiveObjects
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (1)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (1)
87-87: Remove the generic comment on thetestsOnly_property.Line 87 adds a generic explanatory comment for a
testsOnly_declaration; repo guidance asks to omit these inSources/**/*.swift.✂️ Suggested cleanup
- /// Returns the number of buffered object operations if in the SYNCING state, or nil otherwise. internal var testsOnly_bufferedObjectOperationsCount: Int? {As per coding guidelines,
Sources/**/*.swift: “FortestsOnly_property declarations, do not add generic explanatory comments (their meaning is understood)”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift` at line 87, Remove the generic explanatory comment added for the testsOnly_ property; locate the testsOnly_ declaration in InternalDefaultRealtimeObjects (the testsOnly_ property in InternalDefaultRealtimeObjects.swift) and delete the comment above it so the property has no generic explanatory comment per Sources/**/*.swift guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Tests/AblyLiveObjectsTests/JS` Integration
Tests/ObjectsIntegrationTests.swift:
- Around line 607-622: This test case is missing the required spec-point
attribution comment; add an exact-format comment (either // `@spec`(<section
reference>) or // `@specPartial`(<section reference>)) immediately above the
.init( ... ) test declaration that references the relevant spec point governing
ATTACHED without HAS_OBJECTS behavior, choosing `@spec` vs `@specPartial` per the
guideline and avoid duplicating the same `@spec` tag elsewhere for the same spec
point; ensure the comment is placed in the same test block that contains the
action closure (the anonymous .init with description "on ATTACHED without
HAS_OBJECTS clears local state") so tools can associate it with that test.
---
Nitpick comments:
In `@Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift`:
- Line 87: Remove the generic explanatory comment added for the testsOnly_
property; locate the testsOnly_ declaration in InternalDefaultRealtimeObjects
(the testsOnly_ property in InternalDefaultRealtimeObjects.swift) and delete the
comment above it so the property has no generic explanatory comment per
Sources/**/*.swift guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6f3ee39a-112d-4dbd-b002-94cd53865bdc
📒 Files selected for processing (3)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swiftTests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
Show resolved
Hide resolved
lawrence-forooghian
force-pushed
the
AIT-324-apply-on-ACK
branch
from
cbb49fd to
1dc20e7
Compare
Port of ably-js commit b329fd2. Verifies that when an ATTACHED message is received without the HAS_OBJECTS flag, the SDK clears all local LiveObjects state (root map should have no keys). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lawrence-forooghian
force-pushed
the
AIT-287-new-rules-for-discarding-buffered-events
branch
from
65dda76 to
343e8cf
Compare
github-actions
bot
temporarily deployed
to
staging/pull/121/AblyLiveObjects
Implements spec commit 997584f. Integration tests ported from ably-js commit 9b2224d. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lawrence-forooghian
force-pushed
the
AIT-287-new-rules-for-discarding-buffered-events
branch
from
343e8cf to
f15cfac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (2)
87-88: Drop the generic doc comment on thistestsOnly_accessor.The name already carries the intent, so this extra sentence just adds noise.
✂️ Suggested cleanup
- /// Returns the number of buffered object operations if in the SYNCING state, or nil otherwise. internal var testsOnly_bufferedObjectOperationsCount: Int? {As per coding guidelines, "For
testsOnly_property declarations, do not add generic explanatory comments (their meaning is understood)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift` around lines 87 - 88, Remove the generic doc comment above the internal property testsOnly_bufferedObjectOperationsCount; the name already documents intent, so delete the comment block (the triple-slash/`///` line) immediately preceding the internal var testsOnly_bufferedObjectOperationsCount declaration in InternalDefaultRealtimeObjects so only the property remains.
802-804: Make the discard scope explicit here.After the referenced spec change,
RTO5a2aonly discards the in-flight sync sequence state; buffered OBJECT ops were moved underRTO4donATTACHED. “discard previous” reads broader than what this branch now does and makes it easy to reintroduce the old buffer reset by accident. (github.com)📝 Suggested wording
- // RTO5a2a: new sequence started, discard previous. Else we continue the existing sequence per RTO5a3 + // RTO5a2a: new sequence started, discard the previous in-flight sync sequence. + // bufferedObjectOperations are cleared on ATTACHED per RTO4d, not here. syncingData.syncSequence = nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift` around lines 802 - 804, The comment for RTO5a2a is too broad; update it to explicitly state that only the in‑flight sync sequence state (syncingData.syncSequence) is being discarded here and that buffered OBJECT ops must NOT be reset in this branch (buffer clearing is handled under RTO4d on ATTACHED). Locate the line referencing RTO5a2a and syncingData.syncSequence and amend the comment to reflect this narrower scope so future changes won't accidentally clear buffered ops.Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (1)
421-421: ExerciseRTO4don thehasObjects == falsepath too.
RTO4dnow owns buffered-operation disposal for everyATTACHED, and the oldRTO5a2bclause was explicitly replaced. This case never makesbufferedObjectOperationsnon-empty, so the false branch can regress without the test failing. (github.com)🧪 Suggested coverage addition
`#expect`(realtimeObjects.testsOnly_hasSyncSequence) `#expect`(originalPool.entries.count == 3) // root + 2 additional objects + internalQueue.ably_syncNoDeadlock { + realtimeObjects.nosync_handleObjectProtocolMessage(objectMessages: [ + TestFactories.mapCreateOperationMessage(objectId: "map:buffered@999"), + ]) + } + `#expect`(realtimeObjects.testsOnly_bufferedObjectOperationsCount == 1) + // When: onChannelAttached is called with hasObjects = false internalQueue.ably_syncNoDeadlock { realtimeObjects.nosync_onChannelAttached(hasObjects: false) } @@ // RTO4b3, RTO4b4, RTO4d: SyncObjectsPool must be cleared, sync sequence cleared, BufferedObjectOperations cleared, appliedOnAckSerials cleared `#expect`(!realtimeObjects.testsOnly_hasSyncSequence) + `#expect`(realtimeObjects.testsOnly_bufferedObjectOperationsCount == nil) `#expect`(realtimeObjects.testsOnly_appliedOnAckSerials.isEmpty)Also applies to: 497-499
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift` at line 421, Update the test to also exercise the RTO4d behavior when hasObjects == false: in InternalDefaultRealtimeObjectsTests add a run that simulates an ATTACHED state with hasObjects set to false and verify that bufferedObjectOperations remains empty (and any disposal logic invoked) so the new RTO4d buffered-operation disposal path is executed; specifically modify the test(s) around the RTO4d checks (and the similar cases at the region identified for lines 497-499) to create an ATTACHED lifecycle transition with hasObjects == false and assert the same disposal/clear expectations as the true branch (refer to symbols RTO4d, hasObjects, bufferedObjectOperations and the replaced RTO5a2b behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Tests/AblyLiveObjectsTests/JS` Integration
Tests/ObjectsIntegrationTests.swift:
- Around line 2297-2316: The test currently only checks final state after
calling objectsHelper.processObjectStateMessageOnChannel(... syncSerial:
"serial:") so add an intermediate assertion after the injected operation (and
after ATTACHED is processed) but before calling
processObjectStateMessageOnChannel to verify the post-ATTACHED operation remains
buffered: either assert the buffered-op count via the new buffered-count
accessor on the object or assert that root.get(key: "baz") is still nil; then
keep the existing final assertion that after processObjectStateMessageOnChannel
the buffered op has been applied. Reference
processObjectOperationMessageOnChannel, processObjectStateMessageOnChannel and
root.get when placing the new check.
---
Nitpick comments:
In `@Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift`:
- Around line 87-88: Remove the generic doc comment above the internal property
testsOnly_bufferedObjectOperationsCount; the name already documents intent, so
delete the comment block (the triple-slash/`///` line) immediately preceding the
internal var testsOnly_bufferedObjectOperationsCount declaration in
InternalDefaultRealtimeObjects so only the property remains.
- Around line 802-804: The comment for RTO5a2a is too broad; update it to
explicitly state that only the in‑flight sync sequence state
(syncingData.syncSequence) is being discarded here and that buffered OBJECT ops
must NOT be reset in this branch (buffer clearing is handled under RTO4d on
ATTACHED). Locate the line referencing RTO5a2a and syncingData.syncSequence and
amend the comment to reflect this narrower scope so future changes won't
accidentally clear buffered ops.
In `@Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift`:
- Line 421: Update the test to also exercise the RTO4d behavior when hasObjects
== false: in InternalDefaultRealtimeObjectsTests add a run that simulates an
ATTACHED state with hasObjects set to false and verify that
bufferedObjectOperations remains empty (and any disposal logic invoked) so the
new RTO4d buffered-operation disposal path is executed; specifically modify the
test(s) around the RTO4d checks (and the similar cases at the region identified
for lines 497-499) to create an ATTACHED lifecycle transition with hasObjects ==
false and assert the same disposal/clear expectations as the true branch (refer
to symbols RTO4d, hasObjects, bufferedObjectOperations and the replaced RTO5a2b
behavior).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a475bc14-3f64-4cdd-bddf-57a5df8d9f92
📒 Files selected for processing (3)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swiftTests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
Show resolved
Hide resolved
lawrence-forooghian
deleted the
AIT-287-new-rules-for-discarding-buffered-events
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: This PR is based on top of #118; please review that one first.
Implements ably/specification@997584f. Integration tests ported from ably/ably-js@9b2224d.
Summary by CodeRabbit
Tests
Refactor