[AIT-207] Partial object sync#117
[AIT-207] Partial object sync#117lawrence-forooghian wants to merge 4 commits intoAIT-322-v6-ProtocolMessagefrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughA refactoring replaces array-based sync object storage with a dedicated SyncObjectsPool type that manages partial sync message merging and accumulation. SyncObjectsPool validates, merges map entries, and handles counter states during OBJECT_SYNC sequences. Changes
Sequence DiagramsequenceDiagram
participant Channel
participant InternalDefaultRealtimeObjects
participant SyncObjectsPool
participant ObjectsPool
Channel->>InternalDefaultRealtimeObjects: Receive OBJECT_SYNC message
InternalDefaultRealtimeObjects->>SyncObjectsPool: accumulate(objectMessage)
activate SyncObjectsPool
Note over SyncObjectsPool: Validate state & object type<br/>Merge map entries / handle tombstone<br/>Store or update entry
SyncObjectsPool-->>InternalDefaultRealtimeObjects: Pool updated
deactivate SyncObjectsPool
Channel->>InternalDefaultRealtimeObjects: Receive next OBJECT_SYNC (or end)
InternalDefaultRealtimeObjects->>SyncObjectsPool: accumulate(objectMessage)
activate SyncObjectsPool
Note over SyncObjectsPool: Continue merging partial data
SyncObjectsPool-->>InternalDefaultRealtimeObjects: Pool updated
deactivate SyncObjectsPool
InternalDefaultRealtimeObjects->>ObjectsPool: nosync_applySyncObjectsPool(pool)
activate ObjectsPool
loop For each entry in pool
ObjectsPool->>ObjectsPool: Create/update object with merged state
end
ObjectsPool-->>InternalDefaultRealtimeObjects: Objects applied
deactivate ObjectsPool
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 |
cf6edee to
a2a010e
Compare
7689879 to
dadaade
Compare
a2a010e to
67a92dd
Compare
dadaade to
ee5b2f9
Compare
67a92dd to
b900be3
Compare
ee5b2f9 to
dff72b2
Compare
b900be3 to
e1438f6
Compare
ffc512f to
02bfedb
Compare
e1438f6 to
1989994
Compare
3a18180 to
7ec81af
Compare
1989994 to
70d9862
Compare
7ec81af to
65b36dc
Compare
70d9862 to
183c690
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/SyncObjectsPool.swift`:
- Line 1: This file currently only imports Foundation but must explicitly import
Ably and the internal plugin support module; update the top of
SyncObjectsPool.swift to add `import Ably` and an internal import for
`_AblyPluginSupportPrivate` (i.e., `@_internal import
_AblyPluginSupportPrivate`) so the non-test AblyLiveObjects sources have the
required module visibility for symbols used in this file.
- Around line 52-55: The code force-unwraps merged.object and mergedObject.map
during map merging which can crash if the stored entry for that objectId isn't a
map; change the logic in the merge path to safely unwrap and type-check before
accessing entries: use optional binding on merged.object and merged.object?.map
(or detect if merged.object is a non-map entry) and handle the mismatch by
either initializing a new map entry for merged (e.g., create a new map container
with empty entries) or skipping/overwriting safely instead of force-unwrapping;
update the references around merged.object, mergedObject.map, and
mergedMap.entries to use the safe optional values and fallbacks.
In `@Tests/AblyLiveObjectsTests/JS` Integration
Tests/ObjectsIntegrationTests.swift:
- Around line 736-967: Add the required spec attribution comments above each new
test case: for the "OBJECT_SYNC sequence builds object tree across multiple sync
messages" scenario, add the appropriate `@spec` or `@specPartial` comment
referencing the exact spec point; for "partial OBJECT_SYNC merges map entries
across multiple messages for the same objectId" add the `@specPartial` (since it’s
a partial-sync scenario) with the exact spec id; and for "OBJECT_SYNC does not
break when receiving an unknown object type" add the `@spec` comment with the
exact spec id. Place each attribution as a single-line comment immediately
before the .init(...) block (do not duplicate the same `@spec` for a spec already
attributed elsewhere) and use the exact comment format mandated by
CONTRIBUTING.md so the tests are traceable.
In `@Tests/AblyLiveObjectsTests/SyncObjectsPoolTests.swift`:
- Around line 1-3: The test file SyncObjectsPoolTests.swift is missing required
module imports; update the top of the file (where you currently have "@testable
import AblyLiveObjects", "import Foundation", "import Testing") to also import
Ably and _AblyPluginSupportPrivate by adding "import Ably" and "import
_AblyPluginSupportPrivate" so the tests can reference Ably types and plugin
support; keep `@testable` import AblyLiveObjects as-is and do not use an internal
import for _AblyPluginSupportPrivate.
- Line 95: In SyncObjectsPoolTests (the test class with the duplicate `@spec` tag
RTO5f2a2), locate the two occurrences of "@spec RTO5f2a2" and remove the
duplicate by converting one of them to the split-style tag required by
CONTRIBUTING.md (e.g., use "@specOneOf RTO5f2a2" or the appropriate
`@specPartial/`@specOneOf form per the guidelines), ensuring the exact comment
formatting matches the repository's spec-tag convention and that only one plain
"@spec RTO5f2a2" remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0502d5af-8a15-4698-8d80-2d11502f56ea
📒 Files selected for processing (9)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Internal/ObjectsPool.swiftSources/AblyLiveObjects/Internal/SyncObjectsPool.swiftSources/AblyLiveObjects/Internal/SyncObjectsPoolEntry.swiftTests/AblyLiveObjectsTests/Helpers/TestFactories.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swiftTests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swiftTests/AblyLiveObjectsTests/ObjectsPoolTests.swiftTests/AblyLiveObjectsTests/SyncObjectsPoolTests.swift
💤 Files with no reviewable changes (1)
- Sources/AblyLiveObjects/Internal/SyncObjectsPoolEntry.swift
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
Show resolved
Hide resolved
3d02fe7 to
18df1c8
Compare
18df1c8 to
cbd73a9
Compare
cbd73a9 to
00d0efc
Compare
15dfe04 to
2dcf9bb
Compare
Will be useful for some upcoming tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Give the sync objects pool its own type instead of using a bare [SyncObjectsPoolEntry] array. This makes the concept explicit and is preparation for implementing partial object sync, which will expand the capabilities of this type. I wouldn't pay too much attention to the tests; they'll be replaced soon. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Port the two integration tests added in ably-js commit b20d3ed: - OBJECT_SYNC sequence builds object tree across multiple sync messages - OBJECT_SYNC does not break when receiving an unknown object type Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implements the changes from spec commits 1f22417, 9f4d7de, and 963ec30, which allow the server to split a large object across multiple OBJECT_SYNC protocol messages. The new integration test is ported from ably-js commit d0bc431. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
00d0efc to
12faf8b
Compare
Note: This PR is based on top of #114; please review that one first.
Implements the following spec commits, which allow the server to split a large object over multiple
OBJECT_SYNCmessages.See commit message for more details.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests