[ECO-5461] Implement tombstoning and garbage collection spec #33
Conversation
github-actions
bot
temporarily deployed
to
staging/pull/33/AblyLiveObjects
WalkthroughThis set of changes introduces tombstone handling and garbage collection for LiveObjects in the codebase. It adds timestamp-based tombstone tracking, updates APIs to propagate tombstone state, and implements periodic garbage collection for tombstoned objects and map entries. Related test helpers and tests are updated to use the new tombstone timestamp logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RealtimeObjects
participant ObjectsPool
participant LiveMap/Counter
participant Logger
participant Clock
Note over User,RealtimeObjects: Periodic garbage collection
loop Every interval
RealtimeObjects->>ObjectsPool: performGarbageCollection(gracePeriod, clock, logger)
ObjectsPool->>LiveMap/Counter: releaseTombstonedEntries(gracePeriod, logger, clock)
LiveMap/Counter->>Logger: Log released tombstoned entries
ObjectsPool->>Logger: Log released tombstoned objects
end
Note over User,LiveMap/Counter: Tombstone propagation
User->>LiveMap/Counter: apply(OBJECT_DELETE, serialTimestamp, logger, clock)
LiveMap/Counter->>Clock: Get current time (if needed)
LiveMap/Counter->>Logger: Log tombstone timestamp
LiveMap/Counter->>LiveMap/Counter: Set tombstonedAt, reset data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/33/AblyLiveObjects
lawrence-forooghian
force-pushed
the
ECO-5461-deletions
branch
from
465862b to
357287d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/33/AblyLiveObjects
lawrence-forooghian
force-pushed
the
ECO-5461-deletions
branch
from
357287d to
480687c
Compare
github-actions
bot
temporarily deployed
to
staging/pull/33/AblyLiveObjects
lawrence-forooghian
force-pushed
the
ECO-5337-subscriptions
branch
from
1434bbb to
392fae3
Compare
lawrence-forooghian
force-pushed
the
ECO-5461-deletions
branch
from
480687c to
2f4311f
Compare
github-actions
bot
temporarily deployed
to
staging/pull/33/AblyLiveObjects
lawrence-forooghian
force-pushed
the
ECO-5461-deletions
branch
from
2f4311f to
f614228
Compare
lawrence-forooghian
changed the base branch from
ECO-5337-subscriptions
to
groundwork-for-deletions-and-writes
github-actions
bot
temporarily deployed
to
staging/pull/33/AblyLiveObjects
lawrence-forooghian
force-pushed
the
ECO-5461-deletions
branch
from
f614228 to
782a614
Compare
github-actions
bot
temporarily deployed
to
staging/pull/33/AblyLiveObjects
lawrence-forooghian
changed the base branch from
groundwork-for-deletions-and-writes
to
44-JSON-map-entries-support
lawrence-forooghian
force-pushed
the
44-JSON-map-entries-support
branch
from
6fecc91 to
59783b1
Compare
lawrence-forooghian
force-pushed
the
ECO-5461-deletions
branch
from
782a614 to
779662a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/33/AblyLiveObjects
lawrence-forooghian
force-pushed
the
ECO-5461-deletions
branch
from
779662a to
e139e76
Compare
github-actions
bot
temporarily deployed
to
staging/pull/33/AblyLiveObjects
lawrence-forooghian
changed the base branch from
44-JSON-map-entries-support
to
ECO-5435-write-API
lawrence-forooghian
force-pushed
the
ECO-5435-write-API
branch
from
6859050 to
073b104
Compare
lawrence-forooghian
changed the base branch from
ECO-5435-write-API
to
44-JSON-map-entries-support
lawrence-forooghian
force-pushed
the
ECO-5461-deletions
branch
from
e139e76 to
1badcec
Compare
lawrence-forooghian
changed the title
github-actions
bot
temporarily deployed
to
staging/pull/33/AblyLiveObjects
|
@coderabbitai full review |
✅ Actions performedFull review 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: 6
🔭 Outside diff range comments (1)
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (1)
544-545: Update test to usetombstonedAtinstead of removedtombstoneThe
tombstoneboolean no longer exists in the codebase, so this assertion will fail to compile. Update the test to assert that the new timestamp property is nil after a MAP_SET:• File: Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift
Lines: 544–545Replace:
- #expect(map.testsOnly_data["key1"]?.tombstone == false) + #expect(map.testsOnly_data["key1"]?.tombstonedAt == nil)
🧹 Nitpick comments (1)
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (1)
320-325: Fix incorrect comment about tombstone state.The comment on line 322 says "tombstonedAt is false" but should say "tombstonedAt is nil" to accurately reflect that this entry is not tombstoned because
tombstonedAtdefaults to nil when not specified.- // tombstonedAt is false, so not considered tombstoned + // tombstonedAt is nil, so not considered tombstonedThe tombstone entries with
Date()timestamps are correctly implemented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift(7 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift(14 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift(3 hunks)Sources/AblyLiveObjects/Internal/InternalLiveObject.swift(1 hunks)Sources/AblyLiveObjects/Internal/InternalObjectsMapEntry.swift(1 hunks)Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift(1 hunks)Sources/AblyLiveObjects/Internal/ObjectsPool.swift(2 hunks)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveMap.swift(1 hunks)Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift(2 hunks)Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift(11 hunks)
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: applies to ablyliveobjects/**/!(*test|*tests).swift : when importing the ably module inside the ably...
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/swift.mdc:0-0
Timestamp: 2025-07-29T08:07:47.875Z
Learning: Applies to AblyLiveObjects/**/!(*Test|*Tests).swift : When importing the Ably module inside the AblyLiveObjects library code (non-test code), use `import Ably`.
Applied to files:
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftSources/AblyLiveObjects/Internal/InternalLiveObject.swiftTests/AblyLiveObjectsTests/Helpers/TestFactories.swift
📚 Learning: applies to ablyliveobjects/**/!(*test|*tests).swift : when importing the ablyplugin module inside th...
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/swift.mdc:0-0
Timestamp: 2025-07-29T08:07:47.875Z
Learning: Applies to AblyLiveObjects/**/!(*Test|*Tests).swift : When importing the AblyPlugin module inside the AblyLiveObjects library code (non-test code), use `internal import AblyPlugin`.
Applied to files:
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftSources/AblyLiveObjects/Internal/InternalObjectsMapEntry.swiftSources/AblyLiveObjects/Internal/LiveObjectMutableState.swiftSources/AblyLiveObjects/Internal/InternalLiveObject.swiftTests/AblyLiveObjectsTests/Helpers/TestFactories.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift
📚 Learning: applies to **/tests/**/*.swift : when you need to import the following modules in the tests, do so i...
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-07-29T08:08:00.588Z
Learning: Applies to **/Tests/**/*.swift : When you need to import the following modules in the tests, do so in the following way: Ably: use `import Ably`; AblyLiveObjects: use `@testable import AblyLiveObjects`; AblyPlugin: use `import AblyPlugin`; do not do `internal import`.
Applied to files:
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftSources/AblyLiveObjects/Internal/InternalLiveObject.swiftTests/AblyLiveObjectsTests/Helpers/TestFactories.swift
📚 Learning: applies to **/tests/**/*.swift : when creating `testsonly_` property declarations, do not write gene...
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-07-29T08:07:58.385Z
Learning: Applies to **/Tests/**/*.swift : When creating `testsOnly_` property declarations, do not write generic comments of the form 'Test-only access to the private createOperationIsMerged property'; the meaning of these properties is already well understood.
Applied to files:
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftTests/AblyLiveObjectsTests/Helpers/TestFactories.swiftSources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift
📚 Learning: applies to **/tests/**/*.swift : add comments that explain when some piece of test data is not impor...
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-07-29T08:08:00.588Z
Learning: Applies to **/Tests/**/*.swift : Add comments that explain when some piece of test data is not important for the scenario being tested.
Applied to files:
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftTests/AblyLiveObjectsTests/Helpers/TestFactories.swift
📚 Learning: applies to **/tests/**/*.swift : when writing tests, make sure to add comments that explain when som...
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-07-29T08:07:58.385Z
Learning: Applies to **/Tests/**/*.swift : When writing tests, make sure to add comments that explain when some piece of test data is not important for the scenario being tested.
Applied to files:
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftTests/AblyLiveObjectsTests/Helpers/TestFactories.swift
📚 Learning: applies to **/tests/**/*.swift : follow the guidelines given under 'attributing tests to a spec poin...
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-07-29T08:08:00.588Z
Learning: Applies to **/Tests/**/*.swift : Follow the guidelines given under 'Attributing tests to a spec point' in the file `CONTRIBUTING.md` in order to tag the unit tests with the relevant specification points. Make sure to follow the exact format of the comments as described in that file. Pay particular attention to the difference between the meaning of `@spec` and `@specPartial` and be sure not to write `@spec` multiple times for the same specification point.
Applied to files:
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift
📚 Learning: applies to **/tests/**/*.swift : do not use `fatalerror` in response to a test expectation failure. ...
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-07-29T08:08:00.588Z
Learning: Applies to **/Tests/**/*.swift : Do not use `fatalError` in response to a test expectation failure. Favour the usage of Swift Testing's `#require` macro.
Applied to files:
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift
📚 Learning: applies to **/tests/**/*.swift : when you need to unwrap an optional value in the tests, favour usin...
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-07-29T08:08:00.588Z
Learning: Applies to **/Tests/**/*.swift : When you need to unwrap an optional value in the tests, favour using `#require` instead of `guard let`.
Applied to files:
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift
📚 Learning: applies to **/tests/**/*.swift : when you need to pass a logger to internal components in the tests,...
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-07-29T08:08:00.588Z
Learning: Applies to **/Tests/**/*.swift : When you need to pass a logger to internal components in the tests, pass `TestLogger()`.
Applied to files:
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift
📚 Learning: the liveobjects functionality is referred to in the specification simply as 'objects'....
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/specification.mdc:0-0
Timestamp: 2025-07-29T08:07:37.861Z
Learning: The LiveObjects functionality is referred to in the Specification simply as 'Objects'.
Applied to files:
Sources/AblyLiveObjects/Internal/InternalObjectsMapEntry.swiftSources/AblyLiveObjects/Internal/InternalLiveObject.swift
🧬 Code Graph Analysis (4)
Sources/AblyLiveObjects/Internal/InternalObjectsMapEntry.swift (1)
Sources/AblyLiveObjects/Internal/InternalLiveObject.swift (1)
tombstone(17-35)
Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveMap.swift (1)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (1)
size(134-144)
Sources/AblyLiveObjects/Internal/InternalLiveObject.swift (2)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (1)
resetDataToZeroValued(384-387)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (1)
resetDataToZeroValued(766-769)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (1)
Sources/AblyLiveObjects/Internal/ObjectsPool.swift (1)
performGarbageCollection(322-345)
⏰ 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). (4)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Generate code coverage
🔇 Additional comments (25)
Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (3)
13-17: Clean computed property implementation for tombstone state.The computed
isTombstoneproperty provides a clean boolean interface while maintaining timestamp precision internally. The TODO comment suggests this approach may be preferred over storing separate boolean state, which aligns with DRY principles.
19-20: Enhanced tombstone tracking with timestamps.The
tombstonedAtproperty upgrade from boolean to timestamp enables more sophisticated garbage collection logic based on tombstone age. The optional Date type appropriately represents the tombstone state.
28-28: Appropriate initializer update for test support.The addition of the
testsOnly_tombstonedAtparameter follows established naming conventions and enables test scenarios while maintaining appropriate default behavior (nil = not tombstoned).Also applies to: 32-32
Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveMap.swift (1)
37-37: Correct delegate parameter addition for tombstone-aware size calculation.The addition of the
delegateparameter aligns with the updatedInternalDefaultLiveMap.sizemethod signature and maintains consistency with other proxied method calls in this class.Sources/AblyLiveObjects/Internal/InternalObjectsMapEntry.swift (4)
1-2: Necessary Foundation import for Date type.The import is required for the
Datetype used in thetombstonedAtproperty.
5-9: Consistent timestamp-based tombstone tracking implementation.The transition from boolean to timestamp-based tombstone tracking with computed boolean interface follows the same clean pattern as
LiveObjectMutableState. This enables enhanced garbage collection logic while maintaining a familiar boolean interface.
16-16: Correct initializer parameter update.The parameter change from
tombstone: Bool?totombstonedAt: Date?properly aligns with the new property structure.
17-17: Proper property assignment update.The assignment correctly uses the new
tombstonedAtparameter and property.Tests/AblyLiveObjectsTests/Helpers/TestFactories.swift (2)
409-409: Correct factory method update for timestamp-based tombstone tracking.The parameter and assignment changes properly align with the new
tombstonedAt: Date?property structure. The default value ofnilappropriately represents "not tombstoned".Also applies to: 414-414
443-443: Appropriate parameter update in internalStringMapEntry.The parameter change from
tombstone: Bool?totombstonedAt: Date?withnildefault is consistent with the tombstone tracking updates.Sources/AblyLiveObjects/Internal/InternalLiveObject.swift (3)
1-2: Clean protocol extension for tombstone functionality.The import follows established patterns, and the
resetDataToZeroValued()method requirement is properly defined with appropriate mutating keyword and spec documentation.Also applies to: 10-13
15-35: Well-implemented tombstone method with robust timestamp handling.The method properly handles both timestamp scenarios (provided vs. local clock fallback) with appropriate logging, and correctly sequences the tombstone timestamp assignment before data reset. The implementation follows the RTLO specification accurately.
37-49: Appropriate delegation for OBJECT_DELETE operation.The method provides clear semantic meaning for delete operations by delegating to the tombstone method. The implementation correctly follows the RTLO5 specification.
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (1)
259-266: Thread safety issue:garbageCollectionGracePeriodaccessed without synchronizationThe method accesses
garbageCollectionGracePeriodwithout mutex protection. Since this value can be modified by CONNECTED ProtocolMessages (as mentioned in the comments), this could lead to race conditions.Access the grace period within the mutex lock:
internal func performGarbageCollection() { mutex.withLock { + let gracePeriod = garbageCollectionGracePeriod mutableState.objectsPool.performGarbageCollection( - gracePeriod: garbageCollectionGracePeriod, + gracePeriod: gracePeriod, clock: clock, logger: logger, ) } }Alternatively, consider moving
garbageCollectionGracePeriodinto theMutableStatestruct to ensure all mutable state is protected by the mutex.Likely an incorrect or invalid review comment.
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (1)
317-321: Address the TODO: Clarify spec behavior for updating siteTimeserials on tombstoned objectsThe implementation currently updates siteTimeserials before checking if the object is tombstoned. The TODO comment raises a valid question about whether this is the intended behavior.
Consider tracking this as a spec clarification issue to ensure the implementation aligns with the intended behavior. The current approach seems reasonable (update metadata regardless of tombstone state), but explicit spec guidance would be helpful.
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (10)
42-42: LGTM! Correctly updated tombstone handling.The change from boolean
tombstone: truetotombstonedAt: Date()properly implements the new timestamp-based tombstone tracking while maintaining the test's intent to verify that tombstoned entries return nil.
287-287: LGTM! Method signature correctly updated.The addition of the
delegateparameter to thesizemethod call is consistent with the updated method signature and aligns with other property access methods.
333-351: LGTM! Consistent method signature updates and correct test expectations.All property access methods are correctly updated to include the
delegateparameter, and the test expectations properly verify that tombstoned entries are filtered out (expecting 1 active entry instead of 3 total entries).
375-375: LGTM! Method signature correctly updated.The
sizemethod call is properly updated to include thedelegateparameter, maintaining consistency with other property access methods in the test.
425-425: LGTM! Method signature correctly updated.Consistent with other updates, the
sizemethod call properly includes thedelegateparameter.
508-508: LGTM! Correctly updated tombstone handling.The test entry is properly updated to use timestamp-based tombstoning with
tombstonedAt: Date(), maintaining the test's intent to verify operation application on tombstoned entries.
688-688: LGTM! Method calls correctly updated with new parameter.The
testsOnly_applyMapRemoveOperationcalls are properly updated to include the newoperationSerialTimestamp: nilparameter, which is appropriate for test scenarios that don't require specific timestamp values.Also applies to: 714-714, 748-748, 770-770
706-707: LGTM! Correctly specified non-tombstoned entry.The explicit
tombstonedAt: nilparameter correctly indicates that the entry is not tombstoned initially, making the test setup clear and intentional.
606-606: Verify consistent tombstone property usage across tests.Similar to the previous comment, these tests are checking the boolean
tombstoneproperty rather than the newtombstonedAttimestamp approach. Please verify that:
- The
tombstoneboolean property is still available and correctly computed fromtombstonedAt- These assertions are testing the correct behavior
- The relationship between
tombstoneandtombstonedAtis properly maintainedThis appears to be a systematic pattern across MAP_SET and MAP_REMOVE operation tests.
Also applies to: 731-731, 773-773
1-1239: Overall assessment: Well-structured update to tombstone timestamp implementation.The test file has been systematically updated to support the new tombstone timestamp approach:
Strengths:
- Consistent updates to method signatures (adding
delegateparameters)- Proper use of
tombstonedAt: Date()for creating tombstoned test entries- Correct addition of
operationSerialTimestampparameter to MAP_REMOVE operations- Test logic and expectations remain sound and comprehensive
Areas verified above:
- Mixed use of boolean
tombstoneproperty in assertions needs verification to ensure compatibility with the newtombstonedAtimplementation- One minor comment correction needed (line 322)
The changes maintain test coverage while properly adapting to the new tombstone timestamp functionality.
lawrence-forooghian
force-pushed
the
44-JSON-map-entries-support
branch
from
59783b1 to
4034382
Compare
Based on [1] at 488e932. Haven't updated tests due to being in a bit of a rush; deferred to #52. [1] ably/specification#350
Based on spec referenced in 133c85b; same comment re testing applies too.
Based on spec referenced in 133c85b; same comment re testing applies too.
Based on spec referenced in 133c85b; same comment re testing applies too.
Based on spec referenced in 133c85b; same comment re testing applies too.
Based on spec referenced in 133c85b; same comment re testing applies too.
lawrence-forooghian
force-pushed
the
ECO-5461-deletions
branch
from
1badcec to
fe7503c
Compare
github-actions
bot
temporarily deployed
to
staging/pull/33/AblyLiveObjects
|
I think the comments raised by coderabbit are valid enough to review and fix in this pr, wdyt @lawrence-forooghian? |
Yep, am taking a look |
lawrence-forooghian
force-pushed
the
ECO-5461-deletions
branch
from
fe7503c to
e334349
Compare
github-actions
bot
temporarily deployed
to
staging/pull/33/AblyLiveObjects
lawrence-forooghian
force-pushed
the
ECO-5461-deletions
branch
from
e334349 to
2167277
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (1)
530-534: Update TODO: siteTimeserials should not be updated on tombstoned mapsPer the Ably Live Objects specification, operations on tombstoned objects are explicitly rejected (error 92002) and no timeserial metadata is updated after tombstoning. Please remove or revise the TODO to reflect that behavior.
- // RTLM15e - // TODO: are we still meant to update siteTimeserials? https://github.com/ably/specification/pull/350/files#r2218718854 - if liveObjectMutableState.isTombstone { - return - } + // RTLM15e: per spec, operations on tombstoned objects are rejected and siteTimeserials remain unchanged. + if liveObjectMutableState.isTombstone { + return + }
🧹 Nitpick comments (3)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (3)
342-347: Add explicit access control levelThe method is missing an explicit access control level, violating the SwiftLint
explicit_aclrule specified in the coding guidelines.- /// Releases entries that were tombstoned more than `gracePeriod` ago, per RTLM19. - internal func releaseTombstonedEntries(gracePeriod: TimeInterval, clock: SimpleClock) { + /// Releases entries that were tombstoned more than `gracePeriod` ago, per RTLM19. + internal func releaseTombstonedEntries(gracePeriod: TimeInterval, clock: SimpleClock) {Actually, looking more carefully, the method already has
internalaccess control level. This appears to be correct.
349-363: Add explicit access control levelsBoth computed properties are missing explicit access control levels, violating the SwiftLint
explicit_aclrule.- /// Returns the object's RTLO3d `isTombstone` property. - internal var isTombstone: Bool { + /// Returns the object's RTLO3d `isTombstone` property. + internal var isTombstone: Bool {Wait, I see both properties already have
internalaccess control specified. These are correct as written.
765-769: Add explicit access control levelThe method is missing an explicit access control level, violating the SwiftLint
explicit_aclrule.- /// Needed for ``InternalLiveObject`` conformance. - mutating func resetDataToZeroValued() { + /// Needed for ``InternalLiveObject`` conformance. + internal mutating func resetDataToZeroValued() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift(7 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift(14 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift(3 hunks)Sources/AblyLiveObjects/Internal/ObjectsPool.swift(2 hunks)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveMap.swift(1 hunks)Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveMap.swift
- Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift
- Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift
- Sources/AblyLiveObjects/Internal/ObjectsPool.swift
- Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift
🧰 Additional context used
📓 Path-based instructions (2)
**/!(*Test|*Tests).swift
📄 CodeRabbit Inference Engine (.cursor/rules/swift.mdc)
**/!(*Test|*Tests).swift: Satisfy SwiftLint'sexplicit_aclrule: all declarations should specify Access Control Level keywords explicitly.
When writing anextensionof a type, favour placing the access level on the declaration of the extension rather than each of its individual members.
Files:
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift
**/*.swift
📄 CodeRabbit Inference Engine (.cursor/rules/swift.mdc)
**/*.swift: When writing initializer expressions, when the type that is being initialized can be inferred, favour using the implicit.init(…)form instead of explicitly writing the type name.
When writing enum value expressions, when the type that is being initialized can be inferred, favour using the implicit.caseNameform instead of explicitly writing the type name.
When writing JSONValue or WireValue types, favour using the literal syntax enabled by their conformance to theExpressibleBy*Literalprotocols where possible.
When writing an array literal that starts with an initializer expression, start the initializer expression on the line after the opening square bracket of the array literal.
Files:
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift
🧠 Learnings (3)
📚 Learning: applies to ablyliveobjects/**/!(*test|*tests).swift : when importing the ablyplugin module inside th...
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/swift.mdc:0-0
Timestamp: 2025-07-29T08:07:47.875Z
Learning: Applies to AblyLiveObjects/**/!(*Test|*Tests).swift : When importing the AblyPlugin module inside the AblyLiveObjects library code (non-test code), use `internal import AblyPlugin`.
Applied to files:
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift
📚 Learning: applies to ablyliveobjects/**/!(*test|*tests).swift : when importing the ably module inside the ably...
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/swift.mdc:0-0
Timestamp: 2025-07-29T08:07:47.875Z
Learning: Applies to AblyLiveObjects/**/!(*Test|*Tests).swift : When importing the Ably module inside the AblyLiveObjects library code (non-test code), use `import Ably`.
Applied to files:
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift
📚 Learning: applies to **/tests/**/*.swift : when creating `testsonly_` property declarations, do not write gene...
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-07-29T08:08:00.588Z
Learning: Applies to **/Tests/**/*.swift : When creating `testsOnly_` property declarations, do not write generic comments of the form 'Test-only access to the private createOperationIsMerged property'; the meaning of these properties is already well understood.
Applied to files:
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift
🔇 Additional comments (14)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (14)
116-119: LGTM: Correct tombstone check implementationThe early return when the LiveMap is tombstoned correctly implements RTLM5e specification.
134-134: LGTM: Necessary delegate parameter additionAdding the
delegateparameter enables tombstone checking for referenced objects per RTLM14c specification.
141-141: LGTM: Correct delegate parameter usagePassing the delegate parameter enables comprehensive tombstone checking including referenced objects.
155-155: LGTM: Consistent tombstone filtering implementationThe delegate-enabled tombstone checking correctly filters tombstoned entries per RTLM11d1.
323-333: LGTM: Enhanced test method signature for tombstone supportAdding
operationSerialTimestamp,logger, andclockparameters enables proper tombstone timestamp management in test scenarios.
393-410: LGTM: Correct tombstone state handling in replaceDataThe implementation correctly follows RTLM6e (no-op if already tombstoned) and RTLM6f (tombstone if state indicates it) with proper update emission.
416-434: LGTM: Proper tombstone timestamp handlingThe implementation correctly converts boolean tombstone flags to timestamps, using
serialTimestampwhen available (RTLM6c1a) and falling back to local clock with appropriate logging (RTLM6c1b).
464-471: LGTM: Correct parameter passing for tombstone operationsPassing
operationSerialTimestamp,logger, andclockparameters enables proper tombstone timestamp management during initial value merging per RTLM17a2.
579-596: LGTM: Correct operation handling for MAP_REMOVE and OBJECT_DELETEThe parameter passing for MAP_REMOVE and the new OBJECT_DELETE operation handling correctly implement RTLM15d3 and RTLM15d5 specifications with proper update emission.
622-632: LGTM: Correct tombstone clearing in MAP_SET operationsSetting
tombstonedAttonilcorrectly implements RTLM7a2c and RTLM7b2/RTLM7b3 for clearing tombstone state when setting entries.
646-682: LGTM: Comprehensive tombstone timestamp handling for MAP_REMOVEThe implementation correctly calculates tombstone timestamps per RTLM8f, using
operationSerialTimestampwhen available (RTLM8f1) and falling back to local clock with appropriate logging (RTLM8f2).
771-794: LGTM: Correct garbage collection implementationThe method properly implements RTLM19 garbage collection logic, filtering out tombstoned entries beyond the grace period and logging only the entries being released.
800-815: LGTM: Correct tombstone checking logicThe implementation correctly checks for tombstoned entries per RTLM14a (entry tombstone flag) and RTLM14c (referenced object tombstone status), returning appropriate boolean values.
864-876: LGTM: Proper tombstone filtering for referenced objectsThe implementation correctly returns
nilwhen the referenced object is tombstoned per RTLM5d2f3, ensuring tombstoned objects are not exposed through map values.
|
@umair-ably have addressed |
Note: This PR is based on top of #45; please review that one first.
Implements ably/specification#350. Have not yet done unit tests; have deferred those to #52, and will instead prioritise porting across the JS integration tests.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores