- 
                Notifications
    You must be signed in to change notification settings 
- Fork 2
[ECO-5337] Implement the subscriptions spec #27
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
12aa415    to
    965eb05      
    Compare
  
    965eb05    to
    3643c8b      
    Compare
  
    3643c8b    to
    c055602      
    Compare
  
    bc814fb    to
    e6dc68b      
    Compare
  
    c055602    to
    749a7d6      
    Compare
  
    e6dc68b    to
    1754935      
    Compare
  
    749a7d6    to
    e87102e      
    Compare
  
    1754935    to
    e66c130      
    Compare
  
    e87102e    to
    7d95aca      
    Compare
  
    e66c130    to
    dcbe617      
    Compare
  
    It's a bit annoying that Xcode's autocomplete didn't do this when I created these implementations.
Missed this in ce8c022.
Didn't do this in ce8c022 because I didn't have a good idea of our threading approach. But for the initial approach that we'll be taking — namely, calling the callbacks on ably-cocoa's callback queue — I think we'll need it. Haven't done it for batch stuff yet because I don't yet know whether it'll be necessary (will get a better idea when we implement it). Now that the callback can be called on any thread, we can no longer easily use the "capture the return value of `subscribe` pattern so that we can unsubscribe later" pattern. So, I've decided to pass the subscripiton as a second argument to the callback. Might be there's a better pattern we can use (e.g. pass in an object to the `subscribe` call, like JS `fetch`'s AbortController), but this'll do for now.
The correct behaviour wasn't clear from the spec when I wrote cb427d8, but new spec PR [1] makes it seem that this is the right thing to do (still needs clarifying though). [1] ably/specification#346
Mistake in 6430358.
This is preparation for implementing subscriptions. Cursor updated the tests.
Motivation as in 3f6de86; the new spec points in [1] tell us these can throw. [1] ably/specification#346
7d95aca    to
    1434bbb      
    Compare
  
    | @coderabbitai full review | 
| ✅ Actions performedFull review triggered. | 
| WalkthroughThis update introduces a comprehensive overhaul of the LiveObjects subscription API. It adds new generic update types, explicit callback queue management, and subscription handles to the listener APIs. Internal classes and tests are updated to support these changes, including new mechanisms for emitting updates, managing subscriptions, and propagating errors. Test coverage is expanded to verify subscription, emission, and unsubscription behaviors. Changes
 Sequence Diagram(s)sequenceDiagram
    participant User as User Code
    participant PublicObj as Public LiveObject
    participant InternalObj as Internal LiveObject
    participant State as LiveObjectMutableState
    participant Queue as userCallbackQueue
    User->>PublicObj: subscribe(listener)
    PublicObj->>InternalObj: subscribe(listener, coreSDK)
    InternalObj->>State: subscribe(listener, coreSDK, updateSelfLater)
    State-->>InternalObj: SubscribeResponse
    InternalObj-->>PublicObj: SubscribeResponse
    PublicObj-->>User: SubscribeResponse
    Note over State: When update occurs
    InternalObj->>State: emit(update, on: userCallbackQueue)
    State->>Queue: dispatch listener(update, subscription)
    Queue-->>User: listener(update, subscription)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~75 minutes Assessment against linked issues
 Assessment against linked issues: Out-of-scope changes
 Suggested reviewers
 Poem
 Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 ( | 
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
🔭 Outside diff range comments (1)
Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (1)
341-401: Clarify root reset behavior: clear data only, do not replace instanceThe enhancements to
handlesHasObjectsFalseare solid, but the lingering TODO about whether to replace or clear the root object can now be resolved. By convention (and as seen in common object-pool patterns), a “reset”/“clear” operation should empty the root’s data without re-instantiating the pool/root object itself.Actions:
- Remove the TODO at line 398.
- Update the comment to state clearly: “Per spec and common practice, clearing the pool should preserve the root instance and only clear its data.”
🧹 Nitpick comments (1)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (1)
417-431: Consider simplifying the update merging logic.The current implementation uses
uniqueKeysWithValueswith a comment expressing concern about this approach. Since the keys come from iterating over map entries, they should indeed be unique by definition. Consider using a simpler approach:let update = filteredPerKeyUpdates.reduce(into: [:]) { result, element in result.merge(element.update) { _, new in new } }This would make the uniqueness guarantee more explicit and avoid the need for
uniqueKeysWithValues.
Based on [1] at 2963300. Have not implemented RTL04b1's channel mode checking for same reason as mentioned in 8d881e2. Have not currently tested `replaceData`'s return value; will do once [2] clarified. [1] ably/specification#346 [2] https://github.com/ably/specification/pull/346/files#r2201363446
1434bbb    to
    392fae3      
    Compare
  
    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: 0
🧹 Nitpick comments (1)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (1)
417-431: Consider simplifying the update aggregation logic.The current implementation is overly complex for merging non-overlapping key updates. Since each key operation produces a unique key, the concern about overlapping keys shouldn't exist.
Simplify the update aggregation:
-// RTLM17c: Merge the updates, skipping no-ops -// I don't love having to use uniqueKeysWithValues, when I shouldn't have to. I should be able to reason _statically_ that there are no overlapping keys. The problem that we're trying to use LiveMapUpdate throughout instead of something more communicative. But I don't know what's to come in the spec so I don't want to mess with this internal interface. -let filteredPerKeyUpdates = perKeyUpdates.compactMap { update -> LiveMapUpdate? in - switch update { - case .noop: - nil - case let .update(update): - update - } -} -let filteredPerKeyUpdateKeyValuePairs = filteredPerKeyUpdates.reduce(into: []) { result, element in - result.append(contentsOf: Array(element.update)) -} -let update = Dictionary(uniqueKeysWithValues: filteredPerKeyUpdateKeyValuePairs) -return .update(DefaultLiveMapUpdate(update: update)) +// RTLM17c: Merge the updates, skipping no-ops +var updates: [String: DefaultLiveMapUpdate.UpdateType] = [:] +for perKeyUpdate in perKeyUpdates { + if case let .update(mapUpdate) = perKeyUpdate { + updates.merge(mapUpdate.update) { _, new in new } + } +} +return updates.isEmpty ? .noop : .update(DefaultLiveMapUpdate(update: updates))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
- CONTRIBUTING.md(1 hunks)
- Sources/AblyLiveObjects/Internal/DefaultLiveCounterUpdate.swift(1 hunks)
- Sources/AblyLiveObjects/Internal/DefaultLiveMapUpdate.swift(1 hunks)
- Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift(9 hunks)
- Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift(17 hunks)
- Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift(14 hunks)
- Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift(2 hunks)
- Sources/AblyLiveObjects/Internal/LiveObjectUpdate.swift(1 hunks)
- Sources/AblyLiveObjects/Internal/ObjectsPool.swift(8 hunks)
- Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveCounter.swift(1 hunks)
- Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveMap.swift(1 hunks)
- Sources/AblyLiveObjects/Utility/NSLock+Extensions.swift(1 hunks)
- Tests/AblyLiveObjectsTests/Helpers/Subscriber.swift(1 hunks)
- Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift(14 hunks)
- Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift(44 hunks)
- Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift(5 hunks)
- Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift(3 hunks)
- Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift(15 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift
- Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift
- Sources/AblyLiveObjects/Internal/DefaultLiveMapUpdate.swift
- Sources/AblyLiveObjects/Internal/DefaultLiveCounterUpdate.swift
- Sources/AblyLiveObjects/Utility/NSLock+Extensions.swift
- CONTRIBUTING.md
- Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveCounter.swift
- Tests/AblyLiveObjectsTests/Helpers/Subscriber.swift
- Sources/AblyLiveObjects/Internal/LiveObjectUpdate.swift
- Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveMap.swift
- Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift
🧰 Additional context used
📓 Path-based instructions (3)
**/!(*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/LiveObjectMutableState.swift
- Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift
- Sources/AblyLiveObjects/Internal/ObjectsPool.swift
- 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/LiveObjectMutableState.swift
- Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift
- Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift
- Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift
- Sources/AblyLiveObjects/Internal/ObjectsPool.swift
- Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift
- Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift
**/Tests/**/*.swift
📄 CodeRabbit Inference Engine (.cursor/rules/testing.mdc)
**/Tests/**/*.swift: Use the Swift Testing framework (import Testing), not XCTest.
Do not usefatalErrorin response to a test expectation failure. Favour the usage of Swift Testing's#requiremacro.
Only add labels to test cases or suites when the label is different to the name of the suitestructor test method.
Follow the guidelines given under 'Attributing tests to a spec point' in the fileCONTRIBUTING.mdin 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@specand@specPartialand be sure not to write@specmultiple times for the same specification point.
Add comments that explain when some piece of test data is not important for the scenario being tested.
When you need to import the following modules in the tests, do so in the following way: Ably: useimport Ably; AblyLiveObjects: use@testable import AblyLiveObjects; AblyPlugin: useimport AblyPlugin; do not dointernal import.
When you need to pass a logger to internal components in the tests, passTestLogger().
When you need to unwrap an optional value in the tests, favour using#requireinstead ofguard let.
When creatingtestsOnly_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.
Files:
- Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift
- Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift
- Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift
🧠 Learnings (8)
📓 Common learnings
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'.
Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (5)
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.
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.
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'.
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.
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 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.
Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift (14)
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.
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 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.
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.
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.
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.
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.
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.
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.
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, 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.
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.
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 you need to pass a logger to internal components in the tests, pass TestLogger().
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().
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.
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 : Do not use fatalError in response to a test expectation failure. Favour the usage of Swift Testing's #require macro.
Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (15)
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 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.
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.
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.
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.
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.
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.
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.
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.
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.
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 : Only add labels to test cases or suites when the label is different to the name of the suite struct or test method.
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 you need to pass a logger to internal components in the tests, pass TestLogger().
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().
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, 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.
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.
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 : Do not use fatalError in response to a test expectation failure. Favour the usage of Swift Testing's #require macro.
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (2)
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.
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.
Sources/AblyLiveObjects/Internal/ObjectsPool.swift (3)
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.
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.
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'.
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (4)
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.
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.
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.
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 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.
Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift (16)
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.
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 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.
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.
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.
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.
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.
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 : Only add labels to test cases or suites when the label is different to the name of the suite struct or test method.
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 : Only add labels to test cases or suites when the label is different to the name of the suite struct or test method.
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.
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, 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.
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 you need to pass a logger to internal components in the tests, pass TestLogger().
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().
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.
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 : Do not use fatalError in response to a test expectation failure. Favour the usage of Swift Testing's #require macro.
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.
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.
🧬 Code Graph Analysis (1)
Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift (4)
Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (1)
arguments(675-689)Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (5)
subscribe(88-103)
emit(114-128)
unsubscribeAll(105-107)
unsubscribe(81-85)
unsubscribe(109-112)Tests/AblyLiveObjectsTests/Helpers/Subscriber.swift (2)
createListener(35-50)
getInvocations(22-32)Sources/AblyLiveObjects/Utility/NSLock+Extensions.swift (1)
ablyLiveObjects_withLockWithTypedThrow(5-9)
⏰ 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: Xcode, releaseconfiguration, iOS (Xcode 16.3)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Xcode, releaseconfiguration, tvOS (Xcode 16.3)
- GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Xcode, releaseconfiguration, macOS (Xcode 16.3)
- GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: SPM (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: SPM, releaseconfiguration (Xcode 16.3)
- GitHub Check: check-documentation
- GitHub Check: Generate code coverage
🔇 Additional comments (29)
Tests/AblyLiveObjectsTests/ObjectsPoolTests.swift (6)
12-13: LGTM! Consistent callback queue parameter addition.The addition of
userCallbackQueue: .mainparameters aligns with the architectural changes to support explicit callback queue management in LiveObjects.
80-82: Proper specification point attribution.The
@specOneOfannotations correctly follow the format specified inCONTRIBUTING.mdfor attributing tests to specification points.
84-90: Good async test conversion with proper subscriber setup.The conversion to async function enables testing of update emissions, and the subscriber setup follows the established pattern for capturing callback invocations.
113-116: Excellent update emission verification.The test properly verifies that updates are both stored and emitted according to specification points RTO5c1a2 and RTO5c7. The async pattern for retrieving subscriber invocations is correctly implemented.
134-135: Correct counter value calculation with createOp.The test properly accounts for the updated counter behavior where the final value includes both the state count (10) and the createOp count (5), resulting in 15. This aligns with the new
InternalDefaultLiveCounterimplementation.Also applies to: 144-144
272-350: Comprehensive sync scenario test with proper update verification.The test effectively covers the complex sync scenario while properly verifying update emissions for both maps and counters. The async conversion and subscriber setup are correctly implemented to test the deferred update emission behavior.
Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (3)
6-23: Well-designed generic implementation with proper concurrency support.The conversion to a generic type with
Sendableconstraint is appropriate for the subscription system. The subscription tracking via dictionary is efficient and the initializer properly handles the new structure.
64-103: Robust subscription architecture with proper lifecycle management.The subscription system is well-designed with:
- Proper weak reference handling to prevent memory cycles
- Correct error throwing for detached/failed channel states per RTLO4b2
- Clean separation of concerns with the UpdateLiveObject typealias
105-128: Excellent emission and unsubscription implementation.The implementation correctly:
- Handles
.noopupdates per RTLO4b4c1 by returning early- Dispatches update callbacks asynchronously on the specified queue per RTLO4b4c2
- Provides clean unsubscription methods per RTLO4d
Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (3)
13-13: Consistent callback queue parameter additions.The systematic addition of
userCallbackQueue: .mainthroughout all counter creation calls properly aligns with the architectural changes for explicit callback queue management.Also applies to: 31-31, 47-47, 63-63, 90-90, 103-103, 118-118, 138-138, 160-160, 184-184, 200-200, 224-224, 265-265, 289-289, 326-326, 361-361, 400-400
322-352: Proper async test conversion with subscription verification.The test correctly:
- Converts to async to enable awaiting callback invocations
- Sets up subscribers to capture emitted updates
- Verifies the expected update emission per RTLC7d1a
395-418: Good test coverage for unsupported operations.The test properly verifies that applying unsupported operations (like MAP_CREATE to a counter) results in no emitted updates, correctly covering specification point RTLC7d3.
Sources/AblyLiveObjects/Internal/ObjectsPool.swift (3)
57-83: Elegant deferred update emission design.The
DeferredUpdateenum provides a clean abstraction for collecting updates during sync processing and emitting them afterwards. ThereplaceDatamethod correctly returns the deferred update for later emission, supporting the RTO5c7 specification requirement.
185-199: Proper deferred update emission implementation.The enhanced sync logic correctly:
- Collects deferred updates during sync processing without immediate emission
- Emits all collected updates after sync completion per RTO5c7
- Ensures consistent state before update emission
Also applies to: 243-247
251-261: Reasonable reset implementation with appropriate specification concerns.The
reset()method correctly handles the RTO4b scenario by preserving only the root object and clearing its data. The TODO comment appropriately flags the specification ambiguity that should be resolved.Tests/AblyLiveObjectsTests/LiveObjectMutableStateTests.swift (3)
16-16: Correct type parameter updates for generic implementation.The updates properly align with the conversion of
LiveObjectMutableStateto a generic type, including the correct parameter label for the test-only constructor.Also applies to: 101-103
117-319: Excellent comprehensive subscription test coverage.The
SubscriptionTestsprovide thorough coverage of:
- Subscription error handling for invalid channel states (RTLO4b2)
- Emission behavior for noop vs update cases (RTLO4b4c1, RTLO4b4c2)
- Multiple unsubscription scenarios (RTLO4b5b, RTLO4d)
The
MutableStateStorehelper class is well-designed for thread-safe testing with proper mutex usage and weak reference handling.
240-241: Appropriate handling of known limitation.Disabling the test with a clear explanation and GitHub issue reference (#28) is the correct approach for a known limitation. The test implementation is preserved for future resolution.
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (6)
48-48: LGTM! Consistent userCallbackQueue propagation.The
userCallbackQueueproperty is properly added and consistently threaded through all initializers and factory methods.Also applies to: 57-57, 64-64, 73-73, 77-77, 89-89, 96-96
193-213: Well-implemented subscription management with proper thread safety.The subscribe and unsubscribeAll methods correctly use mutex locking for thread safety and weak self references to avoid retain cycles.
225-233: Clean implementation of the emit method.The method correctly delegates to the internal liveObject's emit functionality with proper synchronization.
241-241: Excellent design improvement with LiveObjectUpdate return types.Changing methods from void to return
LiveObjectUpdate<DefaultLiveMapUpdate>enables proper distinction between actual updates and no-ops, which is crucial for efficient event emission.Also applies to: 253-253, 265-265, 303-303, 319-319, 356-356, 386-386, 510-510, 544-544, 618-618
328-333: Correct implementation of resetData per RTO4b2 specification.The method properly emits removal events for all existing keys before clearing the data, ensuring subscribers are notified of the reset operation.
Also applies to: 636-645
455-462: Proper event emission after operation application.The implementation correctly captures update results and emits them on the userCallbackQueue as specified in RTLM15d1a, RTLM15d2a, and RTLM15d3a.
Also applies to: 474-483, 490-495
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (5)
31-31: Consistent userCallbackQueue implementation.The property is properly added and threaded through all initializers, matching the pattern in LiveMap.
Also applies to: 39-39, 48-48, 52-52, 62-62, 68-68
99-119: Proper subscription management implementation.The methods follow the same thread-safe pattern as LiveMap with appropriate mutex locking and weak self handling.
130-139: Clean emit method implementation.Follows the same pattern as LiveMap for external update emission.
144-144: Well-structured update return values.The methods correctly return either
.update(DefaultLiveCounterUpdate(amount:))with the increment amount or.noop, providing clear feedback about the operation result.Also applies to: 151-151, 158-158, 165-165, 220-237, 293-303
260-265: Correct event emission implementation.Operations properly emit updates on the userCallbackQueue as specified in RTLC7d1a and RTLC7d2a.
Also applies to: 268-270
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
Note: This PR is based on top of #26; please review that one first.
Implements ably/specification#346. See commit messages for more details.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Style