Open
Conversation
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
WalkthroughThis PR implements a publish-and-apply pattern for live objects where operations are applied locally upon ACK from the Realtime channel. It introduces operation source tracking, a PublishResult type, InternalRealtimeObjectsProtocol abstraction, and synchronization infrastructure for coordinating local applies with channel state changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant PublicAPI as Public API
participant InternalRT as InternalRealtimeObjects
participant CoreSDK as CoreSDK
participant Channel as Realtime Channel
participant LocalPool as Local Object Pool
Client->>PublicAPI: increment()/set()
PublicAPI->>InternalRT: nosync_publishAndApply()
par Publish Path
InternalRT->>CoreSDK: nosync_publish(callback:)
CoreSDK->>Channel: Send ObjectMessage
and Buffer Path
InternalRT->>InternalRT: Store PublishAndApplySyncWaiter
end
Channel-->>CoreSDK: ACK with PublishResult
CoreSDK-->>InternalRT: Callback(PublishResult)
rect rgba(100, 200, 100, 0.5)
InternalRT->>InternalRT: createSynthetic message from PublishResult
InternalRT->>LocalPool: Apply with source: .local
LocalPool-->>InternalRT: Confirm applied (Bool)
InternalRT->>InternalRT: Track in appliedOnAckSerials
end
InternalRT->>InternalRT: Drain PublishAndApplySyncWaiters(outcome: .synced)
InternalRT-->>PublicAPI: Callback with success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
lawrence-forooghian
force-pushed
the
AIT-324-apply-on-ACK
branch
from
8afca5a to
e183c0e
Compare
lawrence-forooghian
changed the base branch from
main
to
make-CoreSDK-use-callbacks
Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveMap.swift
Outdated
Show resolved
Hide resolved
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
lawrence-forooghian
added a commit
that referenced
this pull request
Mar 2, 2026
- Package.swift: restore dependency URLs (remove .git suffix), add TODO comments to unpin before release - PublishResult: remove explicit Sendable (inferred by Swift 6) - Remove @discardableResult from nosync_apply methods; update tests to capture and assert on the return value - siteCode: change from pull to push pattern — remove nosync_siteCode() from CoreSDK, push from DefaultInternalPlugin.nosync_onConnected via nosync_setSiteCode - Replace NSSelectorFromString with #selector - Introduce InternalDefaultRealtimeObjectsProtocol extending LiveMapObjectsPoolDelegate, consolidating the separate delegate and realtimeObjects fields in public proxy objects; create MockRealtimeObjects to simplify map/counter publish tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
lawrence-forooghian
force-pushed
the
AIT-324-apply-on-ACK
branch
from
779d380 to
f1510fc
Compare
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
lawrence-forooghian
force-pushed
the
AIT-324-apply-on-ACK
branch
from
f1510fc to
52ecf38
Compare
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
lawrence-forooghian
force-pushed
the
AIT-324-apply-on-ACK
branch
from
52ecf38 to
aa84a72
Compare
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
lawrence-forooghian
force-pushed
the
AIT-324-apply-on-ACK
branch
from
fa85697 to
d012aa6
Compare
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
lawrence-forooghian
force-pushed
the
AIT-324-apply-on-ACK
branch
from
d012aa6 to
18a80fa
Compare
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift
Outdated
Show resolved
Hide resolved
lawrence-forooghian
force-pushed
the
AIT-324-apply-on-ACK
branch
from
18a80fa to
63d3c6a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
Show resolved
Hide resolved
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
Tests/AblyLiveObjectsTests/JS Integration Tests/TestProxyTransport.swift (1)
407-407: Use implicit.initfor inferredARTConnectionDetailsconstruction.Type inference is available for the
connectionDetailsproperty assignment on line 407. The codebase already uses this pattern in similar contexts (see ObjectsIntegrationTests.swift lines 3913 and 3941).Suggested change
- msg.connectionDetails = ARTConnectionDetails(clientId: clientId, connectionKey: "a8c10!t-3D0O4ejwTdvLkl-b33a8c10", maxMessageSize: 16384, maxFrameSize: 262_144, maxInboundRate: 250, connectionStateTtl: 60, serverId: "testServerId", maxIdleInterval: 15000, objectsGCGracePeriod: 86_400_000, siteCode: nil) + msg.connectionDetails = .init(clientId: clientId, connectionKey: "a8c10!t-3D0O4ejwTdvLkl-b33a8c10", maxMessageSize: 16384, maxFrameSize: 262_144, maxInboundRate: 250, connectionStateTtl: 60, serverId: "testServerId", maxIdleInterval: 15000, objectsGCGracePeriod: 86_400_000, siteCode: nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/AblyLiveObjectsTests/JS` Integration Tests/TestProxyTransport.swift at line 407, The assignment to msg.connectionDetails uses an explicit ARTConnectionDetails(...) constructor; change it to use type-inferred shorthand by calling .init(...) so it matches the project's style (e.g., replace ARTConnectionDetails(clientId:..., ...) with .init(clientId:..., ...)) at the msg.connectionDetails assignment site to keep consistency with other tests using inferred initialization.Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (2)
1797-1801: Prefer value-based assertions over reference identity for thrown errors.At Line 1800,
===can become brittle if error objects are bridged/reboxed. Comparing stable fields (code,statusCode,message) is more robust.Suggested assertion update
- `#expect`(thrownError === publishError) + `#expect`(thrownError.code == publishError.code) + `#expect`(thrownError.statusCode == publishError.statusCode) + `#expect`(thrownError.message == publishError.message)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift` around lines 1797 - 1801, The test currently asserts thrownError === publishError (reference identity) which is brittle; change the assertion to compare ARTErrorInfo fields instead: extract the thrownError and publishError (both ARTErrorInfo) returned by realtimeObjects.createMap and assert their .code, .statusCode and .message (or equivalent stable properties) are equal to each other so the test uses value-based equality rather than reference identity; locate the check around thrownError, publishError and createMap to update the assertion accordingly.
1334-1338: Consider extracting repeated “transition to synced state” setup.The same prelude is repeated across multiple tests; a small helper would reduce drift and make intent clearer.
Also applies to: 1382-1386, 1417-1421, 1487-1491, 1528-1532, 1564-1568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift` around lines 1334 - 1338, Extract the repeated "transition to synced state" setup into a small helper function (e.g., transitionToSyncedState or makeRealtimeObjectsSynced) that calls internalQueue.ably_syncNoDeadlock { realtimeObjects.nosync_onChannelAttached(hasObjects: false); realtimeObjects.nosync_setSiteCode("site1") }, then replace the duplicated blocks in the tests (locations referencing internalQueue.ably_syncNoDeadlock and realtimeObjects.nosync_onChannelAttached / nosync_setSiteCode) with a single call to that helper to reduce duplication and clarify intent.Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift (1)
4507-4553: Usedeferfor interceptor restoration in these tests.These tests restore interceptors manually. If the test throws before restore, hooks can leak for the rest of the test body. Use
defer { interceptor.restore() }consistently after creation.Also applies to: 4587-4599
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/AblyLiveObjectsTests/JS` Integration Tests/ObjectsIntegrationTests.swift around lines 4507 - 4553, After creating an EchoInterceptor instance (symbol: EchoInterceptor and variable echoInterceptor), add a defer { echoInterceptor.restore() } immediately so the interceptor is always restored even if the test throws; remove or keep the later manual restore calls (restore() at the end) as redundant but ensure restore happens after any explicit releaseAll() call (method: releaseAll) so behavior is unchanged. Apply the same change to the other test where EchoInterceptor is created (the second block referenced) to avoid leaking hooks.Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (1)
11-15: Add explicit ACL on the protocol requirement.
nosync_publishAndApplyshould declare its access level explicitly to satisfy the project ACL rule.💡 Proposed fix
internal protocol InternalRealtimeObjectsProtocol: LiveMapObjectsPoolDelegate { @@ - func nosync_publishAndApply( + internal func nosync_publishAndApply( objectMessages: [OutboundObjectMessage], coreSDK: CoreSDK, callback: `@escaping` `@Sendable` (Result<Void, ARTErrorInfo>) -> Void, ) }As per coding guidelines, "Specify an explicit access control level (SwiftLint explicit_acl) for all declarations in Swift code (tests are exempt)".
🤖 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 11 - 15, The protocol requirement nosync_publishAndApply currently lacks an explicit access control level; update its declaration to include the appropriate ACL (for example `internal` or `public` depending on the protocol's intended visibility) so it satisfies SwiftLint explicit_acl. Locate the nosync_publishAndApply requirement in InternalDefaultRealtimeObjects (and the corresponding protocol declaration if separate) and prepend the chosen access modifier to the function signature, ensuring all matching implementations/signatures across the codebase use the same access level.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift`:
- Around line 749-753: The no-`HAS_OBJECTS` sync-completion path currently calls
nosync_drainPublishAndApplySyncWaiters(outcome: .synced) but does not clear the
appliedOnAckSerials set, leaving stale ACK serials that can suppress later
OBJECTs; update the RTO4b no-HAS_OBJECTS branch to reset/clear
appliedOnAckSerials before or immediately after calling
nosync_drainPublishAndApplySyncWaiters (and make the identical change in the
other occurrence around the 861-863 region) so that ACK-tracking state is
cleared on this sync-complete path.
In `@Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift`:
- Around line 410-421: The comment is contradictory: it says the operation "will
be applied" though the test asserts it is discarded (applied == false). Update
the comment near the internalQueue.ably_syncNoDeadlock call and the
counter.nosync_apply invocation to state that the operation will be discarded
(not applied) because its serial "ts1" is lexicographically less than the
existing "ts2" and thus should be ignored in this discard-path case; ensure the
comment references the discard expectation so it matches the assertion
(!applied) and unchanged state checks.
In `@Tests/AblyLiveObjectsTests/JS` Integration
Tests/ObjectsIntegrationTests.swift:
- Around line 4461-4468: The test uses a fixed Task.sleep(2_000_000_000) to wait
for the ACK/publish-and-apply sequence, which is flaky; replace the hard sleep
with a deterministic wait for the actual event/state change (e.g., register an
interceptor or callback that signals when publishAndApply completes, or await an
AsyncContinuation/AsyncStream or XCTestExpectation fulfilled by the ACK handler)
so that the code calling counter.increment(5) proceeds only after the real
ACK/publishAndApply event is observed; locate the sleep call and the surrounding
usage of counter.increment and publishAndApply and replace the sleep with
awaiting the explicit signal from that interceptor/handler.
- Around line 4234-4702: Several new `@Test` functions in the Apply-on-ACK section
(applyOnAckScenarios, echoAfterAckDoesNotDoubleApply,
ackAfterEchoDoesNotDoubleApply, applyOnAckDoesNotUpdateSiteTimeserials,
operationBufferedDuringSyncIsAppliedAfterSyncCompletes,
appliedOnAckSerialsIsClearedOnSync,
publishAndApplyRejectsOnChannelStateChangeDuringSync,
subscriptionCallbacksFireForBothLocallyAppliedAndRealtimeReceivedOperations)
lack the required spec attribution comments; add the appropriate single-line
spec comment above each test using the exact format from CONTRIBUTING.md (use
`@spec` or `@specPartial` as appropriate), ensure you do not duplicate the same
`@spec` on multiple tests for the same spec point, and place the comment
immediately above the corresponding test declaration so metadata tooling picks
it up.
---
Nitpick comments:
In `@Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift`:
- Around line 11-15: The protocol requirement nosync_publishAndApply currently
lacks an explicit access control level; update its declaration to include the
appropriate ACL (for example `internal` or `public` depending on the protocol's
intended visibility) so it satisfies SwiftLint explicit_acl. Locate the
nosync_publishAndApply requirement in InternalDefaultRealtimeObjects (and the
corresponding protocol declaration if separate) and prepend the chosen access
modifier to the function signature, ensuring all matching
implementations/signatures across the codebase use the same access level.
In `@Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift`:
- Around line 1797-1801: The test currently asserts thrownError === publishError
(reference identity) which is brittle; change the assertion to compare
ARTErrorInfo fields instead: extract the thrownError and publishError (both
ARTErrorInfo) returned by realtimeObjects.createMap and assert their .code,
.statusCode and .message (or equivalent stable properties) are equal to each
other so the test uses value-based equality rather than reference identity;
locate the check around thrownError, publishError and createMap to update the
assertion accordingly.
- Around line 1334-1338: Extract the repeated "transition to synced state" setup
into a small helper function (e.g., transitionToSyncedState or
makeRealtimeObjectsSynced) that calls internalQueue.ably_syncNoDeadlock {
realtimeObjects.nosync_onChannelAttached(hasObjects: false);
realtimeObjects.nosync_setSiteCode("site1") }, then replace the duplicated
blocks in the tests (locations referencing internalQueue.ably_syncNoDeadlock and
realtimeObjects.nosync_onChannelAttached / nosync_setSiteCode) with a single
call to that helper to reduce duplication and clarify intent.
In `@Tests/AblyLiveObjectsTests/JS` Integration
Tests/ObjectsIntegrationTests.swift:
- Around line 4507-4553: After creating an EchoInterceptor instance (symbol:
EchoInterceptor and variable echoInterceptor), add a defer {
echoInterceptor.restore() } immediately so the interceptor is always restored
even if the test throws; remove or keep the later manual restore calls
(restore() at the end) as redundant but ensure restore happens after any
explicit releaseAll() call (method: releaseAll) so behavior is unchanged. Apply
the same change to the other test where EchoInterceptor is created (the second
block referenced) to avoid leaking hooks.
In `@Tests/AblyLiveObjectsTests/JS` Integration Tests/TestProxyTransport.swift:
- Line 407: The assignment to msg.connectionDetails uses an explicit
ARTConnectionDetails(...) constructor; change it to use type-inferred shorthand
by calling .init(...) so it matches the project's style (e.g., replace
ARTConnectionDetails(clientId:..., ...) with .init(clientId:..., ...)) at the
msg.connectionDetails assignment site to keep consistency with other tests using
inferred initialization.
ℹ️ Review info
Configuration used: Repository 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 (26)
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolvedPackage.resolvedPackage.swiftSources/AblyLiveObjects/Internal/CoreSDK.swiftSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Internal/ObjectsOperationSource.swiftSources/AblyLiveObjects/Internal/ObjectsPool.swiftSources/AblyLiveObjects/Internal/PublishResult.swiftSources/AblyLiveObjects/Protocol/InboundObjectMessage+Synthetic.swiftSources/AblyLiveObjects/Protocol/ObjectMessage.swiftSources/AblyLiveObjects/Public/Public Proxy Objects/InternalLiveMapValue+ToPublic.swiftSources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveCounter.swiftSources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveMap.swiftSources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Public/Public Proxy Objects/PublicObjectsStore.swiftSources/AblyLiveObjects/Utility/Errors.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swiftTests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swiftTests/AblyLiveObjectsTests/JS Integration Tests/TestProxyTransport.swiftTests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swiftTests/AblyLiveObjectsTests/Mocks/MockRealtimeObjects.swift
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
Show resolved
Hide resolved
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
Outdated
Show resolved
Hide resolved
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
lawrence-forooghian
force-pushed
the
AIT-324-apply-on-ACK
branch
from
9d87b1f to
a77505b
Compare
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
Instead of waiting for the server to echo back an operation before applying it locally, operations are now applied immediately upon receiving the ACK from Realtime. Implements the behaviours from spec commit 56a0bba and ports the corresponding integration tests from ably-js commit 6b1c2de, plus the test fix in ably-js commit f9fbe8e (from [1]). [1] ably/ably-js#2175 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lawrence-forooghian
force-pushed
the
AIT-324-apply-on-ACK
branch
from
a77505b to
cbb49fd
Compare
3 tasks
lawrence-forooghian
changed the title
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
1 participant
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.
When you call a LiveObjects mutation method (e.g.
map.set()), the SDK now applies the effects of this operation to the local LiveObjects data as soon as it receives the server's acknowledgement of this operation. This is an improvement over earlier versions, in which the SDK did not apply such an operation until receiving the operation's echo.Supporting PRs:
Related PRs:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests