-
Notifications
You must be signed in to change notification settings - Fork 2
[AIT-255] Fix events emitted upon sync #111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe PR changes replaceData flows in LiveCounter and LiveMap to capture previous data, update state, and return calculated diffs (counter or map). It adds ObjectDiffHelpers for diff computation and updates/extends unit and integration tests to validate diff outputs (RTLC6h / RTLM6h). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (4)**/*.swift📄 CodeRabbit inference engine (.cursor/rules/swift.mdc)
Files:
Tests/**/*.swift📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
Files:
Sources/AblyLiveObjects/**/*.swift📄 CodeRabbit inference engine (.cursor/rules/swift.mdc)
Files:
Sources/**/*.swift📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
Files:
🧬 Code graph analysis (3)Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift (4)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (2)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (2)
⏰ 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). (12)
🔇 Additional comments (6)
✏️ Tip: You can disable this entire section by setting 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Sources/AblyLiveObjects/Internal/ObjectDiffHelpers.swift`:
- Around line 1-4: Replace the unused Foundation import at the top of the file
with the required module imports: remove "import Foundation" and add "internal
import _AblyPluginSupportPrivate" and "import Ably" so the file follows the
AblyLiveObjects coding guidelines; update the header above the internal enum
ObjectDiffHelpers accordingly.
🧹 Nitpick comments (1)
Sources/AblyLiveObjects/Internal/ObjectDiffHelpers.swift (1)
96-109: Consider simplifying using Optional's built-in equality.Since
JSONObjectOrArrayconforms toEquatable, the entire function can be simplified to a single expression, as Swift'sOptionalautomatically provides==when the wrapped type isEquatable.♻️ Simplified implementation
private static func areJSONEqual( _ json1: JSONObjectOrArray?, _ json2: JSONObjectOrArray? ) -> Bool { - switch (json1, json2) { - case (nil, nil): - return true - case (nil, .some), (.some, nil): - return false - case let (.some(j1), .some(j2)): - // JSONObjectOrArray conforms to Equatable, so we can use == directly - return j1 == j2 - } + json1 == json2 }
📜 Review details
Configuration used: Repository 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)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swiftSources/AblyLiveObjects/Internal/ObjectDiffHelpers.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftTests/AblyLiveObjectsTests/ObjectDiffHelpersTests.swiftTests/AblyLiveObjectsTests/ObjectsPoolTests.swift
🧰 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/ObjectDiffHelpersTests.swiftTests/AblyLiveObjectsTests/ObjectsPoolTests.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swiftSources/AblyLiveObjects/Internal/ObjectDiffHelpers.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.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/ObjectDiffHelpersTests.swiftTests/AblyLiveObjectsTests/ObjectsPoolTests.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.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/InternalDefaultLiveCounter.swiftSources/AblyLiveObjects/Internal/ObjectDiffHelpers.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.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/InternalDefaultLiveCounter.swiftSources/AblyLiveObjects/Internal/ObjectDiffHelpers.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift
🧬 Code graph analysis (6)
Tests/AblyLiveObjectsTests/ObjectDiffHelpersTests.swift (2)
Sources/AblyLiveObjects/Internal/ObjectDiffHelpers.swift (2)
calculateCounterDiff(11-17)calculateMapDiff(25-62)Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (1)
internalMapEntry(408-418)
Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift (4)
Tests/AblyLiveObjectsTests/Helpers/Subscriber.swift (1)
getInvocations(23-33)Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (1)
map(71-92)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (1)
entries(143-150)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (1)
value(100-104)
Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (1)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (3)
createZeroValued(66-81)nosync_replaceData(203-216)value(100-104)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (1)
Sources/AblyLiveObjects/Internal/ObjectDiffHelpers.swift (1)
calculateCounterDiff(11-17)
Sources/AblyLiveObjects/Internal/ObjectDiffHelpers.swift (2)
Sources/AblyLiveObjects/Internal/InternalLiveObject.swift (1)
tombstone(17-41)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (1)
keys(152-155)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (2)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (1)
mergeInitialValue(354-371)Sources/AblyLiveObjects/Internal/ObjectDiffHelpers.swift (1)
calculateMapDiff(25-62)
⏰ 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). (12)
- GitHub Check: Example app, tvOS (Xcode 16.4)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.4) - GitHub Check: Xcode, tvOS (Xcode 16.4)
- GitHub Check: Xcode, macOS (Xcode 16.4)
- GitHub Check: Xcode, iOS (Xcode 16.4)
- GitHub Check: Example app, macOS (Xcode 16.4)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.4) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.4) - GitHub Check: SPM (Xcode 16.4)
- GitHub Check: Example app, iOS (Xcode 16.4)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.4) - GitHub Check: Generate code coverage
🔇 Additional comments (11)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (1)
334-351: Diff-based update path looks correct.Storing
previousDatabefore mutation and returning the calculated diff ensures a single full delta for replaceData.Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (1)
296-486: DiffCalculationTests provide solid RTLM6h coverage.The scenarios exercise expected diffs for createOp merges, tombstones, and unchanged entries.
Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift (3)
121-127: Map diff emission assertions align with RTLM6h.These checks correctly validate the full-diff update for existing maps.
163-166: Counter diff emission expectation matches RTLC6h.The assertion correctly validates the single diff update.
374-388: Complex sync scenario diff assertions look correct.The emitted updates now reflect the full before/after deltas as intended.
Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (1)
153-233: RTLC6h diff tests are well covered.Positive, negative, and createOp-merged deltas are exercised cleanly.
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (1)
471-512: LGTM! Clean implementation of RTLM6g/RTLM6h spec changes.The implementation correctly:
- Captures
previousDatabefore any data mutations (RTLM6g)- Discards the
LiveMapUpdatefrommergeInitialValuewhencreateOpis present (RTLM6d)- Returns a unified diff computed via
ObjectDiffHelpers.calculateMapDiff(RTLM6h)This ensures a single event representing the full before-and-after state change during sync, matching the behavioral change described in the PR objectives.
Sources/AblyLiveObjects/Internal/ObjectDiffHelpers.swift (4)
11-17: LGTM!Simple and correct implementation of RTLC14b - the diff amount is computed as
newData - previousData.
66-72: LGTM!Correctly compares only the
datafield for diff purposes, ignoring metadata fields (timeserial,tombstonedAt) as documented.
75-93: LGTM!Comprehensive nil handling and field-by-field comparison. The comparison covers all
ObjectDatafields appropriately.
25-62: The current implementation is correct. The tests explicitly expectcalculateMapDiffto return.update(DefaultLiveMapUpdate(update: [:]))even when there are no changes—not.noop. The test assertion#expect(update.update?.update.isEmpty == true)confirms this: it expects the.updatecase with an empty dictionary, which would fail if.noopwere returned instead. This behavior is intentional and consistent withcalculateCounterDiff, which also always returns.updateregardless of whether the diff amount is zero.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
f1d5b97 to
8fc5d09
Compare
175e957 to
b1b0427
Compare
2cea0d1 to
31bf8db
Compare
31bf8db to
97e805d
Compare
This implements the spec changes from [1] at 3f86319. That is, the update events emitted by a sync are now calculated from the before/after diff of the object state, as opposed to just being calculated from the createOp. Written by Claude based on the spec. [1] ably/specification#414
97e805d to
1185b08
Compare
maratal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This implements the spec changes from ably/specification#414 at
3f86319. That is, the update events emitted by a sync are now calculated from the before/after diff of the object state, as opposed to just being calculated from the createOp.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.