Use server time for object ID per spec #98
Conversation
WalkthroughAdds a server-time fetch API to CoreSDK and switches map/counter creation to use server-provided timestamps; adjusts synchronization points accordingly; updates package pins and related tests/mocks to assert server-time-based IDs. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,235,255,0.6)
participant Caller as createMap/createCounter (caller)
participant InternalQ1 as InternalDefaultRealtimeObjects\n(sync block 1)
end
participant CoreSDK as CoreSDK (async)
participant Plugin as AblyPlugin (internal)
rect rgba(220,235,255,0.6)
participant InternalQ2 as InternalDefaultRealtimeObjects\n(sync block 2)
participant Ops as Operation store
end
Caller->>InternalQ1: Enter sync — validate channel state
InternalQ1-->>Caller: Exit sync
Caller->>CoreSDK: await fetchServerTime()
CoreSDK->>Plugin: nosync_fetchServerTime(for:) via continuation
Plugin-->>CoreSDK: return server time / error
CoreSDK-->>Caller: resume with Date or throw ARTErrorInfo
Caller->>InternalQ2: Enter sync — build creationOperation(timestamp: serverTime)
InternalQ2->>Ops: store creation operation
InternalQ2-->>Caller: Complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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/98/AblyLiveObjects
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (1)
11-22: Consider whether theclockparameter is still needed in the test helper.The
createDefaultRealtimeObjectshelper still accepts aclockparameter (defaulting toMockSimpleClock()), but the tests validating timestamp behavior now useserverTimefromMockCoreSDKinstead. If theclockparameter is no longer used for timestamp generation in object creation, consider whether it's still needed here or should be documented for its remaining purpose (e.g., garbage collection timing).Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (1)
183-189: The secondwithSyncblock forcreationOperationmay be unnecessary.The
creationOperationcomputation at lines 185-188 doesn't appear to read or write any mutable state frommutableStateMutex— it only uses theentriesandtimestampparameters. The sync block seems to be present only for consistency with the previous structure.Consider whether this could be simplified to compute
creationOperationoutside the mutex:- let creationOperation = try mutableStateMutex.withSync { _ throws(ARTErrorInfo) in - // RTO11f - ObjectCreationHelpers.nosync_creationOperationForLiveMap( - entries: entries, - timestamp: timestamp, - ) - } + // RTO11f + let creationOperation = ObjectCreationHelpers.nosync_creationOperationForLiveMap( + entries: entries, + timestamp: timestamp, + )The
trykeyword also appears unnecessary since the closure doesn't actually throw.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved(1 hunks)Package.resolved(1 hunks)Package.swift(1 hunks)Sources/AblyLiveObjects/Internal/CoreSDK.swift(2 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift(2 hunks)Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift(4 hunks)Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/swift.mdc)
**/*.swift: Specify an explicit access control level (SwiftLint explicit_acl) for all declarations in Swift code (tests are exempt)
When extending a type, put the access level on the extension declaration rather than on each member (tests are exempt)
Prefer implicit .init(...) when the type can be inferred in initializer expressions
Prefer enum case shorthand (.caseName) when the type can be inferred
For JSONValue or WireValue, prefer using literal syntax via ExpressibleBy*Literal where possible
Prefer Swift raw string literals for JSON strings instead of escaping double quotes
When an array literal begins with an initializer expression, place the initializer on the line after the opening bracket
Files:
Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swiftSources/AblyLiveObjects/Internal/CoreSDK.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swiftPackage.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift
Tests/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
Tests/**/*.swift: Use the Swift Testing framework (import Testing), not XCTest, in test files
Do not usefatalErrorfor expectation failures; prefer Swift Testing’s#require
Only add labels to test cases or suites when the label differs from the suite struct or test method name
Tag tests per CONTRIBUTING.md’s "Attributing tests to a spec point" with exact comment format; distinguish@specvs@specPartial; do not repeat@specfor the same spec point
Add comments in tests to clarify when certain test data is irrelevant to the scenario
In tests, import Ably usingimport Ably
In tests, import AblyLiveObjects using@testable import AblyLiveObjects
In tests, import_AblyPluginSupportPrivateusingimport _AblyPluginSupportPrivate(do not useinternal import)
When passing a logger to internal components in tests, useTestLogger()
When unwrapping optionals in tests, prefer#requireoverguard let
Files:
Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift
Sources/AblyLiveObjects/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/swift.mdc)
In AblyLiveObjects library (non-test) code, import modules as: Ably with
import Ably, and _AblyPluginSupportPrivate withinternal import _AblyPluginSupportPrivate
Files:
Sources/AblyLiveObjects/Internal/CoreSDK.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift
Sources/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
For
testsOnly_property declarations, do not add generic explanatory comments (their meaning is understood)
Files:
Sources/AblyLiveObjects/Internal/CoreSDK.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift
🧬 Code graph analysis (3)
Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
fetchServerTime(87-107)
Sources/AblyLiveObjects/Internal/CoreSDK.swift (2)
Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
fetchServerTime(41-43)Sources/AblyLiveObjects/Utility/MarkerProtocolHelpers.swift (1)
castPluginPublicErrorInfo(51-53)
Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (1)
Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (1)
createInternalQueue(688-691)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.3) - GitHub Check: SPM (Xcode 16.3)
- GitHub Check: check-documentation
- GitHub Check: Generate code coverage
🔇 Additional comments (10)
Package.resolved (1)
1-94: Lock file updates are consistent with Package.swift changes.The resolved revisions match the explicit revision pins added in Package.swift for ably-cocoa and ably-cocoa-plugin-support. The delta-codec-cocoa version bump to 1.3.5 appears to be a transitive dependency update.
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
1-94: Workspace lock file is consistent with root Package.resolved.Package.swift (1)
23-24: Revision pins with TODO comments are appropriate for pre-release development.The explicit revision pins for
ably-cocoaandably-cocoa-plugin-supportallow depending on the newfetchServerTime()API before it's released. The TODO comments appropriately flag these for unpinning before the next release.Consider creating a tracking issue to ensure these pins are reverted before release, or ensure this is captured in your release checklist.
Also applies to: 29-30
Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (2)
1205-1247: Test correctly validates server time usage in map creation.The test now uses
MockCoreSDKwith a fixedserverTimeand verifies the object ID contains the expected timestamp in milliseconds. This properly tests RTO11f7.
1338-1373: Test correctly validates server time usage in counter creation.Consistent with the map creation test, this validates RTO12f5 by checking the counter object ID contains the server timestamp in milliseconds.
Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
11-16: Mock implementation correctly providesfetchServerTime()for testing.The
serverTimeproperty with a default value and the synchronous return infetchServerTime()is appropriate for mock testing. This aligns with theCoreSDKprotocol extension shown in the relevant code snippets.Also applies to: 40-43
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (2)
174-204: Good refactoring to support async server time fetch.The approach of validating channel state first, then fetching server time outside the mutex (since it's async), then computing the creation operation is correct. The mutex is properly released during the async
fetchServerTime()call.Note: There's a potential time-of-check-to-time-of-use (TOCTOU) gap where the channel state could change between the initial validation (line 177) and the subsequent operations. However, this is acceptable because:
- The
publishcall will fail with an appropriate error if the channel becomes invalid- This is a standard optimistic validation pattern for async operations
224-230: Server time integration for counter creation looks correct.The pattern matches the map creation flow: fetch server time, then use it for the creation operation. This correctly implements RTO12f5.
Sources/AblyLiveObjects/Internal/CoreSDK.swift (2)
11-12: LGTM!The protocol method declaration is clean, follows the established typed throws pattern, and the documentation appropriately references the RTO16 spec.
87-107: LGTM!The implementation correctly bridges the callback-based
nosync_fetchServerTimeto async/await usingwithCheckedContinuationwith aResulttype, then uses.get()to convert to typed throws. Good practices observed:
- Dispatches to
internalQueuebefore calling thenosync_method, maintaining the queue contract- Defensive
dispatchPreconditionto verify callback execution context- Clean capture list
[client, pluginAPI]avoiding unnecessaryselfcapture- Proper error conversion using
ARTErrorInfo.castPluginPublicErrorInfo- Clear precondition message for the invariant that both serverTime and error cannot be nil
lawrence-forooghian
force-pushed
the
50-use-server-time-for-object-ID
branch
from
81057cb to
1d08044
Compare
github-actions
bot
temporarily deployed
to
staging/pull/98/AblyLiveObjects
lawrence-forooghian
force-pushed
the
50-use-server-time-for-object-ID
branch
from
1d08044 to
e3940fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (1)
183-189: Consider removing the unnecessary synchronization block.The
nosync_creationOperationForLiveMaphelper is a static function that doesn't access shared mutable state (confirmed by thenosync_prefix and the fact that the lambda ignores themutableStateparameter with_). Wrapping this call inwithSyncadds unnecessary lock overhead.Apply this diff to simplify:
- let creationOperation = mutableStateMutex.withSync { _ in - // RTO11f - ObjectCreationHelpers.nosync_creationOperationForLiveMap( - entries: entries, - timestamp: timestamp, - ) - } + // RTO11f + let creationOperation = ObjectCreationHelpers.nosync_creationOperationForLiveMap( + entries: entries, + timestamp: timestamp, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved(1 hunks)Package.resolved(1 hunks)Package.swift(1 hunks)Sources/AblyLiveObjects/Internal/CoreSDK.swift(2 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift(2 hunks)Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift(4 hunks)Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift
- AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved
- Package.swift
- Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift
🧰 Additional context used
📓 Path-based instructions (3)
**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/swift.mdc)
**/*.swift: Specify an explicit access control level (SwiftLint explicit_acl) for all declarations in Swift code (tests are exempt)
When extending a type, put the access level on the extension declaration rather than on each member (tests are exempt)
Prefer implicit .init(...) when the type can be inferred in initializer expressions
Prefer enum case shorthand (.caseName) when the type can be inferred
For JSONValue or WireValue, prefer using literal syntax via ExpressibleBy*Literal where possible
Prefer Swift raw string literals for JSON strings instead of escaping double quotes
When an array literal begins with an initializer expression, place the initializer on the line after the opening bracket
Files:
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Internal/CoreSDK.swift
Sources/AblyLiveObjects/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/swift.mdc)
In AblyLiveObjects library (non-test) code, import modules as: Ably with
import Ably, and _AblyPluginSupportPrivate withinternal import _AblyPluginSupportPrivate
Files:
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Internal/CoreSDK.swift
Sources/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
For
testsOnly_property declarations, do not add generic explanatory comments (their meaning is understood)
Files:
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Internal/CoreSDK.swift
🧬 Code graph analysis (1)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (4)
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (1)
withSync(25-35)Sources/AblyLiveObjects/Internal/CoreSDK.swift (2)
nosync_validateChannelState(124-136)fetchServerTime(87-107)Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
fetchServerTime(41-43)Sources/AblyLiveObjects/Internal/ObjectCreationHelpers.swift (1)
nosync_creationOperationForLiveMap(103-160)
🔇 Additional comments (6)
Package.resolved (2)
2-2: Origin hash change looks consistent with dependency updatesThe updated
"originHash"is expected whenever the dependency graph (pins) changes; nothing to fix here.
5-28: Confirm new dependency pins (SHAs and version) are intentional and aligned with server‑time workThe new pins for:
ably-cocoa→ revision23954b55bea7fa24beaa6e43beb580d501e6b6e1ably-cocoa-plugin-support→ revision37ad19df2cc6063c74ec88ecefc2ddf491db5ebfare correctly matched in
Package.swiftand in sync withPackage.resolved. Both include TODO comments confirming these are intentional temporary pins to be unpinned before the next release.Note:
delta-codec-cocoa1.3.5 inPackage.resolvedis a transitive dependency (pulled throughably-cocoa) and does not require an entry inPackage.swift—this is standard Swift Package Manager behavior.Sources/AblyLiveObjects/Internal/CoreSDK.swift (2)
11-12: LGTM!The protocol method declaration is well-documented and properly typed. The addition of server time retrieval capability aligns with the PR objectives to implement RTO16.
87-107: LGTM!The implementation correctly bridges the callback-based
nosync_fetchServerTimeAPI to Swift's async/await. The defensive precondition checks on line 94 and lines 99-101 ensure the contract withpluginAPIis upheld, and the error mapping toARTErrorInfois consistent with the existing codebase.Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (2)
224-230: LGTM!The shift to server time for counter creation correctly implements RTO12f5. The implementation is clean and doesn't introduce unnecessary synchronization overhead.
Note: The same channel state validation timing concern mentioned for
createMapapplies here as well.
175-178: Channel state could change between validation and publish.The validation at line 177 occurs while the lock is held, but the lock is released before
fetchServerTime()at line 181. During this async operation, the channel could transition toDETACHED,FAILED, orSUSPENDED. Thepublish()call at line 192 does not validate channel state before sending, so a message could be published even after the channel has entered an invalid state.Consider re-validating the channel state immediately before
publish(), or holding the lock through the entire sequence if feasible per the RTO11 specification requirements.
This was skipped in dcdb350 due to time constraints. Resolves #50.
The ably-cocoa changes are in ably/ably-cocoa#2149.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.