[ECO-5251] Messages tests (single channel)#249
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
## Walkthrough
This pull request adds Equatable support to the `InternalError` enum by comparing error details, restructures and renames several tests in the default messages suite to validate REST API message sending and error handling, refactors the mock paginated response by removing an obsolete property and updating its logic, and enhances realtime mocks by adding call recording and improved state change handling with thread-safe callback mechanisms. It also refactors subscription APIs across core components from async sequence-based to callback-based with explicit `SubscriptionHandle`s, introduces new subscription storage classes, updates the `Room` and `RoomLifecycleManager` protocols and implementations accordingly, and adds a new discontinuity event subscription mechanism.
## Changes
| File(s) | Change Summary |
|---------|----------------|
| Sources/AblyChat/InternalError.swift | Added an extension for `InternalError` to conform to Equatable by defining a static `==` that compares `toARTErrorInfo()` results. |
| Tests/AblyChatTests/DefaultMessagesTests.swift | Restructured tests: renamed and added methods to verify REST Chat API message sending, proper error handling, subscription point determination based on realtime channel state, and filtering of malformed messages. |
| Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift | Removed the `_isLast` property; updated the initializer to derive `isLast` from `_hasNext` and modified the static `nextPage` instance to include new fields and adjusted pagination logic. |
| Tests/AblyChatTests/Mocks/MockRealtime.swift | Added a `callRecorder` property to log `request` calls in `MockRealtime`. |
| Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift | Enhanced `MockRealtimeChannel` with new state change and message JSON properties, updated `on` methods to store callbacks and immediately invoke them if a state change is preset, and improved subscription behavior to emit constructed messages from JSON. |
| Example/AblyChatExample/ContentView.swift | Refactored event subscriptions from async sequence iteration to callback-based subscriptions with explicit unsubscription, simplifying message and event handling logic. |
| Example/AblyChatExample/Misc/Utils.swift | Added utility async functions `after` and `periodic` to schedule closures on the main actor after delays or periodically. |
| Example/AblyChatExample/Mocks/MockClients.swift | Refactored mock client classes to use callback-based subscription methods returning `SubscriptionHandle` instead of async sequences, unifying subscription patterns. |
| Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift | Added new classes `MockSubscriptionHandleStorage` and `MockMessageSubscriptionHandleStorage` to manage callback-based subscriptions with handles and periodic event emission. |
| Sources/AblyChat/Connection.swift | Changed `Connection` protocol subscription method to callback-based with `SubscriptionHandle`, providing async sequence subscription as a default extension. |
| Sources/AblyChat/DefaultConnection.swift | Updated `DefaultConnection` to use callback-based subscription with `SubscriptionHandle` for connection status changes instead of async sequence subscription. |
| Sources/AblyChat/DefaultMessages.swift | Refactored `DefaultMessages` subscription method to callback-based returning `MessageSubscriptionHandle`, removing buffering policy and async sequence subscription. |
| Sources/AblyChat/DefaultOccupancy.swift | Changed occupancy subscription to callback-based with `SubscriptionHandle`, removing async sequence subscription and buffering policy. |
| Sources/AblyChat/DefaultPresence.swift | Changed presence subscription methods to callback-based with `SubscriptionHandle`, removing buffering policy and async sequence subscription. |
| Sources/AblyChat/DefaultRoomReactions.swift | Changed room reactions subscription to callback-based with `SubscriptionHandle`, removing buffering policy and async sequence subscription. |
| Sources/AblyChat/DefaultTyping.swift | Changed typing subscription to callback-based with `SubscriptionHandle`, removing buffering policy and async sequence subscription. |
| Sources/AblyChat/Discontinuity.swift | Added new `DiscontinuityEvent` struct and `ProvidesDiscontinuity` protocol with callback-based subscription method and async sequence default implementations. |
| Sources/AblyChat/DiscontinuityEvent.swift | Deleted old `DiscontinuityEvent` struct definition (moved to `Discontinuity.swift`). |
| Sources/AblyChat/Messages.swift | Changed `Messages` protocol subscription to callback-based with `MessageSubscriptionHandle`, added default async sequence subscription implementation. |
| Sources/AblyChat/Occupancy.swift | Changed `Occupancy` protocol subscription to callback-based with `SubscriptionHandle`, added default async sequence subscription implementation. |
| Sources/AblyChat/Presence.swift | Changed `Presence` protocol subscription methods to callback-based with `SubscriptionHandle`, added default async sequence subscription implementations. |
| Sources/AblyChat/Room.swift | Extended `Room` protocol to conform to `ProvidesDiscontinuity`, changed subscription methods for status changes and discontinuity events to callback-based with `SubscriptionHandle`, removed async sequence subscription methods. |
| Sources/AblyChat/RoomLifecycleManager.swift | Extended `RoomLifecycleManager` protocol to conform to `ProvidesDiscontinuity`, changed subscription methods for room status and discontinuity events to callback-based with `SubscriptionHandle`, updated waiting logic to use continuations. |
| Sources/AblyChat/RoomReactions.swift | Added callback-based subscription method with `SubscriptionHandle` to `RoomReactions` protocol, added default async sequence subscription implementations. |
| Sources/AblyChat/Subscription.swift | Added new `SubscriptionHandle` struct encapsulating an unsubscribe closure. |
| Sources/AblyChat/SubscriptionHandleStorage.swift | Added internal `SubscriptionHandleStorage` class to manage callback-based subscriptions with handles and emit events. |
| Sources/AblyChat/Typing.swift | Changed `Typing` protocol subscription to callback-based with `SubscriptionHandle`, added default async sequence subscription implementations. |
| Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift | Added extension on `RoomLifecycleManager` providing async sequence subscription methods for room status changes with buffering policy. |
| Tests/AblyChatTests/Mocks/MockRoom.swift | Added stub callback-based subscription methods `onStatusChange` and `onDiscontinuity` returning `SubscriptionHandle`. |
| Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift | Replaced subscription storage with `SubscriptionHandleStorage`, changed subscription methods to callback-based returning `SubscriptionHandle`. Removed old async sequence subscription methods. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant RoomLifecycleManager
participant SubscriptionHandleStorage
Client->>RoomLifecycleManager: onRoomStatusChange(callback)
RoomLifecycleManager->>SubscriptionHandleStorage: create(callback)
SubscriptionHandleStorage-->>RoomLifecycleManager: SubscriptionHandle(unsubscribe)
RoomLifecycleManager-->>Client: SubscriptionHandle
Note over SubscriptionHandleStorage, Client: When event occurs
SubscriptionHandleStorage->>Client: callback(event)
Client->>SubscriptionHandleStorage: unsubscribe()
SubscriptionHandleStorage-->>Client: removes subscriptionsequenceDiagram
participant Client
participant DefaultMessages
participant RealtimeChannel
Client->>DefaultMessages: subscribe(callback)
DefaultMessages->>RealtimeChannel: on("chatMessage", listener)
RealtimeChannel-->>DefaultMessages: message event
DefaultMessages->>Client: callback(message)
Client->>DefaultMessages: unsubscribe()
DefaultMessages->>RealtimeChannel: off("chatMessage", listener)
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
|
maratal
force-pushed
the
fix/235-messages-tests
branch
from
5c4b962 to
e946995
Compare
|
With recent changes in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
13-21: Test method structure looks clear.
The "Given-When-Then" format is well-defined, and the assertion on the recorded HTTP request (#expectusage) aligns with verifying the request path, method, and payload. As an optional improvement, consider verifying aspects of the response or returned message to ensure all round-trip behavior is tested where possible.Also applies to: 23-24, 26-32
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Sources/AblyChat/InternalError.swift(1 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(5 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(4 hunks)Tests/AblyChatTests/Mocks/MockRealtime.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
Sources/AblyChat/InternalError.swift (2)
Tests/AblyChatTests/Helpers/Helpers.swift (1)
lhs(127-132)Sources/AblyChat/PaginatedResult.swift (1)
lhs(99-104)
Tests/AblyChatTests/DefaultMessagesTests.swift (8)
Sources/AblyChat/DefaultMessages.swift (10)
send(36-38)send(182-188)subscribe(28-30)subscribe(76-171)update(40-42)update(190-196)get(32-34)get(174-180)onDiscontinuity(48-50)onDiscontinuity(207-209)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (2)
subscribe(137-161)get(76-83)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (5)
subscribe(7-9)subscribe(11-13)update(53-58)get(23-29)get(31-37)Sources/AblyChat/Messages.swift (1)
getPreviousMessages(313-315)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (1)
first(50-52)Sources/AblyChat/InternalError.swift (5)
toInternalError(40-42)toInternalError(46-48)toInternalError(52-54)toInternalError(58-60)toInternalError(64-66)Sources/AblyChat/RoomFeature.swift (1)
onDiscontinuity(69-71)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (1)
onDiscontinuity(19-21)
🔇 Additional comments (25)
Sources/AblyChat/InternalError.swift (1)
33-37: Good addition of Equatable conformance.Adding Equatable support to InternalError is a good improvement that will make testing error handling more straightforward. The implementation correctly compares two errors by using their converted ARTErrorInfo representations.
Tests/AblyChatTests/Mocks/MockRealtime.swift (2)
7-7: Good addition of call recording capability.Adding a call recorder will make testing method invocations more robust, allowing tests to verify that methods are being called with expected parameters.
39-42: Properly implementing call recording.The implementation correctly records the request method calls with all relevant parameters. This will be valuable for verifying API interactions in tests.
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (4)
13-13: Simplified initializer by removing redundant parameter.Good simplification by removing the
isLastparameter since it can be derived fromhasNext.
43-43: Improved property design for isLast.The implementation now correctly defines
isLastas the logical opposite ofhasNext, which eliminates the potential for inconsistent state between these two related properties.
120-122: Updated pagination settings for test data.Setting
hasNext: truefor the messages test data correctly configures it to simulate a paginated result with more items available.
128-155: Enhanced mock data with additional fields.The updated mock data now includes important fields like clientId, action, and version, making it more representative of real-world responses. The pagination is also properly configured with
hasNext: falseto indicate it's the last page.Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (5)
14-14: Good addition of state change handling.Adding the
stateChangeToEmitForListenerproperty enables the mock to simulate channel state changes during tests.
28-31: Enhanced mock initialization options.The additional parameters provide more flexibility for configuring the mock's behavior during tests, particularly for message emission and state change handling.
134-156: Improved message subscription handling.The implementation now correctly handles creating and emitting a message from JSON data, which provides more flexibility in testing different message scenarios with varied content and metadata.
167-186: Added thread-safe callback management.The implementation now properly uses NSLock to ensure thread safety when managing callbacks. This is important to prevent race conditions during tests, especially when callbacks might be invoked from different threads.
230-242: Added helpful utility methods for callback invocation.These methods provide a clean and thread-safe way to trigger callbacks during tests, which will make it easier to simulate various channel events and state changes.
Tests/AblyChatTests/DefaultMessagesTests.swift (13)
7-11: Good addition of specification markers.
These lines neatly reference the relevant specification sections (CHA-M3, CHA-M3a, etc.), clarifying the purpose of the test suite.
34-36: Error handling test is thorough.
This test properly checks thatARTErrorInfois raised when the REST Chat API returns an error. The usage of#expectto assert the thrown error matches the intended domain and code is clear and maintainable.Also applies to: 38-40, 42-42, 49-49, 51-51, 53-55
58-58: Subscription point test for ATTACHED channel is solid.
You correctly verify that when the channel is already attached, thegetPreviousMessagescall uses the currentchannelSerial. This approach ensures that the test covers the scenario of an attached channel’s subscription point.Also applies to: 60-84
86-88: Attach-serial logic confirmation.
Verifying that theattachSerialis used when the channel is not yet attached (and transitions fromattachingtoattached) confirms correct fallback behavior. The test’s structure is consistent with the prior approach.Also applies to: 90-93, 95-98, 101-113
115-161: Reattachment handling test is comprehensive.
This test checks the scenario whereresumed == falseafter reattachment, confirming that the subscription resets toattachSerial. All transitions and state changes are clearly tested, ensuring correctness.
162-162: UPDATE event handling for unsubscribed states is solid.
Ensuring the subscription point resets to theattachSerialupon an update event withresumed == falseis crucial. The test’s explicit path transitions successfully validate channel state changes.Also applies to: 164-203
207-243: Standard history query options test is well-demonstrated.
This verifies the acceptance of normal query options and enforces “direction = backwards” by default, which is consistent with the specification. The assertion that.init(orderBy: .oldestFirst)results in a “backwards” param helps confirm correct internal logic.
247-268: Server error throw test in subscription getPreviousMessages.
Your verification of error propagation fromMockRealtimetoARTErrorInfoprovides clear coverage for negative server responses.
270-298: GET messages test aligns with the specification.
Confirming that a PaginatedResult can be iterated upon satisfies the real-world usage of retrieving messages in multiple pages. This coverage is both necessary and well structured.
302-327: Error throw on GET messages.
You ensure that the thrownARTErrorInfois tested for the correct code and message. Good negative-path coverage.
334-373: Incoming messages subscription coverage is comprehensive.
Verifying headers and metadata in the received message ensures the subscription pipeline is correctly preserving message properties. This thoroughness helps protect against regression in the future.
375-416: Malformed message handling test is valuable.
Testing that malformed JSON events are not emitted to subscribers is an essential reliability check. The test logic ensures only valid events make it through.
422-441: Discontinuity forwarding test is straightforward and clear.
By emitting aDiscontinuityEventand verifying thatDefaultMessagesrelays the event, the test confirms the correct propagation of discontinuities.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Example app, tvOS (Xcode 16.2)
- GitHub Check: Generate code coverage (Xcode 16.2)
- GitHub Check: Example app, iOS (Xcode 16.2)
- GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Example app, macOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.2) - GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.2) - GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - GitHub Check: SPM (Xcode 16.2)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.2) - GitHub Check: check-documentation
🔇 Additional comments (5)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (5)
14-14: Good addition of stateChangeToEmitForListener propertyThis provides a clean way to simulate channel state changes in tests, allowing test code to specify a state change that should be immediately emitted when a listener is registered.
Also applies to: 31-31, 43-43
28-28: Good enhancement with messageJSONToEmitOnSubscribe propertyThis addition complements the existing messageToEmitOnSubscribe property by providing more flexibility for tests to construct messages from JSON data, which is useful for testing various scenarios.
Also applies to: 38-38, 134-134
167-169: Good thread safety implementation with NSLockAdding explicit locking mechanism using NSLock is a good practice for ensuring thread-safe access to the callback properties, especially since they might be accessed from different threads in test scenarios.
171-186: Well-implemented thread-safe callback registrationThe on(_:) methods now properly use locking to ensure thread safety when registering callbacks, and the improved parameter naming enhances readability.
230-242: Good addition of thread-safe callback invocation methodsThese new methods provide a clean API for tests to trigger state change events while ensuring thread safety with proper locking.
|
I'll fix commit history before the merge. |
|
@maratal according to the spec coverage report, there are some CHA-M spec points that are not tested. Please can you address? |
maratal
force-pushed
the
fix/235-presence-tests
branch
4 times, most recently
from
410e4ae to
6cf2821
Compare
maratal
force-pushed
the
fix/235-messages-tests
branch
from
f285b4a to
b20087a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Tests/AblyChatTests/DefaultMessagesTests.swift (2)
253-255: Same missingasync throwsannotation as aboveThis
doIthelper suffers from the same compilation issue—apply the explicit signature fix here as well.
(See earlier comment for the diff.)
311-313: Repeat of theasync throwsclosure problemFor consistency and successful compilation, annotate this third
doItclosure too.
🧹 Nitpick comments (2)
Tests/AblyChatTests/DefaultMessagesTests.swift (2)
366-368: Mis-labelled spec reference can mislead future maintainersThe inline comment says “Wrong name, should be CHA-M4k” but still tags
@spec CHA-M5k.
Consider correcting the tag to avoid confusion when generating coverage reports.-// Wrong name, should be CHA-M4k -// @spec CHA-M5k +// @spec CHA-M4k // corrected
360-364: PreferXCTExpectFailureor#expectFailureover commenting-out assertionsKeeping assertions commented out hides potentially valuable tests and risks them being forgotten.
Mark the section as an expected failure until the compiler bug is fixed:XCTExpectFailure("Awaiting fix for https://github.com/ably/ably-chat-swift/issues/233") { let receivedMessage = try #require(await messagesSubscription.first { _ in true }) #expect(receivedMessage.headers == ["numberKey": .number(10), "stringKey": .string("hello")]) #expect(receivedMessage.metadata == ["numberKey": .number(10), "stringKey": .string("hello")]) }This keeps the test active while making CI pass.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
Sources/AblyChat/InternalError.swift(1 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(4 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(4 hunks)Tests/AblyChatTests/Mocks/MockRealtime.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Tests/AblyChatTests/Mocks/MockRealtime.swift
- Sources/AblyChat/InternalError.swift
- Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
- Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift
⏰ Context from checks skipped due to timeout of 90000ms (34)
- GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.1)
- GitHub Check: Example app, iOS (Xcode 16.2)
- GitHub Check: Example app, tvOS (Xcode 16.2)
- GitHub Check: Example app, macOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.1) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.2) - GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.1)
- GitHub Check: Example app, tvOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.2) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.1) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.1) - GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Xcode, tvOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Xcode, macOS (Xcode 16.1)
- GitHub Check: Xcode, iOS (Xcode 16.1)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.1) - GitHub Check: SPM,
releaseconfiguration (Xcode 16.2) - GitHub Check: SPM,
releaseconfiguration (Xcode 16.3) - GitHub Check: SPM (Xcode 16.1)
- GitHub Check: SPM (Xcode 16.3)
- GitHub Check: SPM (Xcode 16.2)
- GitHub Check: Generate code coverage
maratal
force-pushed
the
fix/235-messages-tests
branch
from
b20087a to
9537490
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (4)
Sources/AblyChat/DefaultConnection.swift (1)
20-77:⚠️ Potential issueCallback may execute off the MainActor despite
@MainActorexpectation
realtime.connection.oninvokes the closure on the queue chosen by the Ably SDK (not necessarily the main thread).
Because the body callscallback(statusChange)directly three times (lines 51, 62 & 76), the@MainActorguarantee of the callback is violated and Swift’s actor-isolation checker will not protect you at runtime. This can surface as race conditions or “MainActor-only class cannot be used on background thread” crashes in clients holding UIKit / SwiftUI state inside their listener.Suggested fix – hop to the MainActor before invoking the listener:
- callback(statusChange) + Task { @MainActor in + callback(statusChange) + }Apply the same hop at all call-sites (lines 51, 62, 76).
Doing so keeps the public API contract intact and avoids unexpected thread hops for the SDK user.
Sources/AblyChat/DefaultTyping.swift (1)
74-110:⚠️ Potential issueCross-actor access inside Ably listener – dispatch back to the MainActor
Implementationis isolated to the main actor.
The closure you pass tochannel.subscribeis executed on Ably’s internal queue, so every access toself,logger, ortypingTimerManager, and the@MainActorcallback violates actor isolation rules and can trigger Swift Concurrency runtime warnings.- let startedEventListener = channel.subscribe(TypingEventType.started.rawValue) { [weak self] message in - guard let self, let messageClientID = message.clientId else { - return - } - logger.log(message: "Received started typing message: \(message)", level: .debug) - … - callback( - TypingSetEvent( - … - ) - ) - … - callback( - TypingSetEvent( - … - ) - ) - } + let startedEventListener = channel.subscribe(TypingEventType.started.rawValue) { [weak self] message in + Task { @MainActor [weak self] in + guard let self, let messageClientID = message.clientId else { return } + + logger.log(message: "Received started typing message: \(message)", level: .debug) + … + callback( + TypingSetEvent( + … + ) + ) + } + }Apply the same pattern to the
stoppedEventListenerbelow.
This keeps all state changes and callback invocations inside the main actor boundary.Sources/AblyChat/DefaultOccupancy.swift (1)
40-71:⚠️ Potential issueActor-isolation breach & unmanaged background thread
The closure passed to
channel.subscribeexecutes off-actor.
All accesses tologger,options, and the@MainActorcallback break isolation.- let eventListener = channel.subscribe(OccupancyEvents.meta.rawValue) { [logger] message in - logger.log(message: "Received occupancy message: \(message)", level: .debug) - … - callback(occupancyEvent) - } + let eventListener = channel.subscribe(OccupancyEvents.meta.rawValue) { [weak self] message in + Task { @MainActor [weak self] in + guard let self else { return } + logger.log(message: "Received occupancy message: \(message)", level: .debug) + … + callback(occupancyEvent) + } + }Also dispatch
off(eventListener)back onto the main actor in the returnedSubscriptionHandle, mirroring the change suggested for typing.Sources/AblyChat/DefaultMessages.swift (1)
74-86: 🛠️ Refactor suggestionThrown errors inside listener are swallowed – add logging or surfacing
The inner
do / catchignores every parsing/conversion error. Silent failure makes diagnosing malformed realtime events difficult.} catch { - // note: this replaces some existing code that also didn't handle any thrown error; I suspect not intentional, will leave whoever writes the tests for this class to see what's going on + logger.log( + message: "Failed to decode incoming message: \(error)", + level: .error + ) }
♻️ Duplicate comments (1)
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
44-53:⚠️ Potential issueClosure
doItstill lacks an explicitasync throwssignatureWe raised this earlier — without the signature the compiler treats the closure as synchronous & non-throwing, so
await/tryinside it is illegal. Xcode 16.2/16.3 will fail to compile.- let doIt = { - _ = try await defaultMessages.send(params: .init(text: "hey")) - } + let doIt: @Sendable () async throws -> Void = { + _ = try await defaultMessages.send(params: .init(text: "hey")) + }Replicate the same fix for the similar
doIthelpers around lines 253 and 311.🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 52-52: Force casts should be avoided
(force_cast)
🧹 Nitpick comments (20)
Sources/AblyChat/Typing.swift (1)
15-21: Clarify thread expectations for implementersThe callback parameter is annotated
@MainActor, but the protocol does not state that implementations must dispatch onto the main actor before calling it. Consider adding a short doc string remark such as “Implementers must invoke the listener on the main actor” to avoid accidental misuse.No code change required, only documentation.
Sources/AblyChat/Occupancy.swift (1)
44-58: Termination handler needlessly spawns a new taskInside the async-sequence wrapper you already run on MainActor, but
unsubscribe()is dispatched through a brand-newTask { @MainActor in … }.
Creating another hop is unnecessary overhead and adds an extra asynchronous boundary that can make deterministic shutdown harder in tests.- subscription.addTerminationHandler { - Task { @MainActor in - subscriptionHandle.unsubscribe() - } - } + subscription.addTerminationHandler { + subscriptionHandle.unsubscribe() // Already on MainActor + }Unless
SubscriptionHandle.unsubscribe()explicitly requires suspension (it currently does not), calling it synchronously is simpler and avoids race conditions when the caller relies on immediate unsubscription.Sources/AblyChat/DefaultRoomReactions.swift (1)
14-16: Expose the same discardable-result guarantee as the inner implementationMost callers ignore the returned
SubscriptionHandle. The innerImplementation.subscribeis annotated with@discardableResult, but the façade method is not, so unused-result warnings will leak out to SDK users.-internal func subscribe(_ callback: @escaping @MainActor (Reaction) -> Void) -> SubscriptionHandle { +@discardableResult +internal func subscribe(_ callback: @escaping @MainActor (Reaction) -> Void) -> SubscriptionHandle { implementation.subscribe(callback) }This keeps the public surface irritation-free and consistent with the rest of the subscription APIs.
Sources/AblyChat/Discontinuity.swift (2)
5-9: Prefer immutable stored property
erroris never reassigned after initialisation; declaring it asletcommunicates intent and enables additional compiler optimisations.- public var error: ARTErrorInfo + public let error: ARTErrorInfo
37-46: Avoid spawning an extra Task just to unsubscribe
Subscription.addTerminationHandleralready executes its closure on the same context that terminates the async sequence.
Wrappingunsubscribe()inTask { @MainActor … }introduces unnecessary indirection and can lead to actor-hop overhead.- subscription.addTerminationHandler { - Task { @MainActor in - subscriptionHandle.unsubscribe() - } - } + subscription.addTerminationHandler { + subscriptionHandle.unsubscribe() + }Sources/AblyChat/DefaultTyping.swift (1)
11-14: Surface mismatch between protocol isolation and callback execution
subscribe(_:)exposes a@MainActorcallback, but the call-site here merely forwards the closure toimplementation.subscribewithout guaranteeing main-actor execution when the callback is invoked.
This is harmless here, yet worth documenting so callers know their closure is indeed executed on the main actor.Sources/AblyChat/DefaultOccupancy.swift (1)
10-13: Document callback execution contextAs with typing, callers are promised a
@MainActorcallback but nothing here enforces that guarantee. State this explicitly in the doc-comment or ensure dispatch inimplementation.subscribe.Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (2)
47-61: Retain and cancel the repeating task to avoid runaway timers
periodic(with:interval:)presumably returns aTask(or similar handle) that must stay alive for the closure to execute.
BecauseSubscriptiondrops the returned task, it may get de-allocated immediately, stopping emission.
Conversely, ifperiodicmanages its own lifecycle, you still need a way to cancel it when the subscription ends.- periodic(with: interval) { + let timer = periodic(with: interval) { if needNext { callback(randomElement()) } return needNext } handle = SubscriptionHandle { needNext = false + timer.cancel() // ensure the loop stops onTerminate() }Store
timeras a property (let timer: Task<(), Never>) to keep it alive.
97-118: Duplicate timer-retention fix forMockMessageSubscriptionHandleStorageApply the same
timerretention & cancellation pattern here to keep both mock storages consistent and leak-free.Sources/AblyChat/Connection.swift (2)
19-29: Minor doc-comment polish“subscription handler” should read “subscription handle”, and the callback parameter description could clarify that it runs on the main actor.
- * - Returns: A subscription handler that can be used to unsubscribe … + * - Returns: A ``SubscriptionHandle`` that can be used to unsubscribe …This keeps the public docs precise and in sync with the actual API surface.
46-60: Consider cancelling the handle without spinning a newTaskInside
addTerminationHandleryou already run on the current actor; wrappingunsubscribe()inTask { @MainActor in … }adds an extra hop and may race with object deallocation.If the goal is merely to ensure main-actor execution, you can replace the
Taskblock with@MainActorisolation on the closure:subscription.addTerminationHandler { @MainActor in subscriptionHandle.unsubscribe() }Simpler, cheaper, and deterministic.
Sources/AblyChat/Messages.swift (1)
354-361: Consider makingunsubscribeasynchronous
unsubscribemay hit Ably-Cocoa and network-bound state-changes; a synchronous closure gives no way to await completion or surface errors.
Returning@MainActor () async -> Void(and, if needed,throws) would future-proof the API and match the rest of the async surface.Sources/AblyChat/DefaultMessages.swift (1)
5-7: Name no longer reflects stored type
MessageSubscriptionWrapper.subscriptionnow stores aMessageSubscriptionHandle, not aMessageSubscription.
Consider renaming to avoid confusion, e.g.handleorsubscriptionHandle.Sources/AblyChat/Presence.swift (1)
79-85: Missing@MainActoron callback parameter
Presenceis annotated@MainActor, but thecallbackclosure insubscribe(event:_:)is not.
Marking it@MainActor(or explicitly documenting that it’s invoked on the main actor) prevents accidental cross-actor UI updates.-@discardableResult -func subscribe(event: PresenceEventType, _ callback: @escaping ((PresenceEvent) -> Void)) -> SubscriptionHandle +@discardableResult +func subscribe(event: PresenceEventType, _ callback: @escaping @MainActor (PresenceEvent) -> Void) -> SubscriptionHandleExample/AblyChatExample/Mocks/MockClients.swift (3)
69-74:randomStatusChangecan crash if the array is ever emptiedYou’re force‐unwrapping the result of
randomElement():RoomStatusChange(current: [...].randomElement()!, previous: .attaching(error: nil))If someone later “tidies up” the literals and the array becomes empty, the mock will crash at runtime.
A tiny guard keeps the mock robust:- RoomStatusChange(current: [.attached(error: nil), ... , .suspended(error: .createUnknownError())].randomElement()!, previous: .attaching(error: nil)) + let next = [ + RoomStatus.attached(error: nil), RoomStatus.attached(error: nil), + /* … */ + RoomStatus.suspended(error: .createUnknownError()) + ].randomElement() ?? .attached(error: nil) + RoomStatusChange(current: next, previous: .attaching(error: nil))
83-98: Potential retain-cycle & cancellation race inonStatusChange
needNextis captured strongly by the timer and mutated from the cancellation closure.
Ifperiodiccalls its block afterSubscriptionHandle’s cancel has setneedNext = false, you’re safe, but if it fires whileneedNextis stilltrueand the handle is being torn down on another thread, you might end up invokingcallbackone last time after cancellation.Two low-cost safeguards:
- var needNext = true + let lock = NSLock() + var needNext = true … - if needNext { + lock.lock(); defer { lock.unlock() } + if needNext { callback(randomStatusChange()) } return needNext … - needNext = false + lock.lock(); needNext = false; lock.unlock()(or mark
periodic’s block@MainActorto serialize updates).
117-140: Return value relies on implicit return – consider being explicitAlthough Swift allows single-expression implicit returns, these async/throws mocks are quite dense.
An explicitreturnimproves readability and silences “implicit return” SwiftLint rules many teams enable.- mockSubscriptions.create( + return mockSubscriptions.create(Tests/AblyChatTests/DefaultMessagesTests.swift (3)
51-53: Avoid force-casting toARTErrorInfoin tests
as! ARTErrorInfowill crash the test suite if the error type ever changes.
Safer pattern:#expect { guard let art = error as? ARTErrorInfo else { return false } return art == expected }Or use pattern-matching:
} throws: { case let art as ARTErrorInfo in art == expected }Also applies to: 259-261, 318-318
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 52-52: Force casts should be avoided
(force_cast)
262-262: Strip trailing blank line before closing braceSwiftLint’s
vertical_whitespace_closing_bracesrule flags this empty line.🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 262-262: Don't include vertical whitespace (empty line) before closing braces
(vertical_whitespace_closing_braces)
367-369: Incorrect spec tag commentThe inline note says
// Wrong name, should be CHA-M4k // @spec CHA-M5kbut the test covers malformed realtime events (CHA-M4k).
Update the@spectag to avoid confusion in coverage tooling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (30)
Example/AblyChatExample/ContentView.swift(2 hunks)Example/AblyChatExample/Misc/Utils.swift(1 hunks)Example/AblyChatExample/Mocks/MockClients.swift(9 hunks)Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift(2 hunks)Sources/AblyChat/Connection.swift(1 hunks)Sources/AblyChat/DefaultConnection.swift(4 hunks)Sources/AblyChat/DefaultMessages.swift(4 hunks)Sources/AblyChat/DefaultOccupancy.swift(3 hunks)Sources/AblyChat/DefaultPresence.swift(2 hunks)Sources/AblyChat/DefaultRoomReactions.swift(3 hunks)Sources/AblyChat/DefaultTyping.swift(6 hunks)Sources/AblyChat/Discontinuity.swift(1 hunks)Sources/AblyChat/DiscontinuityEvent.swift(0 hunks)Sources/AblyChat/InternalError.swift(1 hunks)Sources/AblyChat/Messages.swift(6 hunks)Sources/AblyChat/Occupancy.swift(2 hunks)Sources/AblyChat/Presence.swift(3 hunks)Sources/AblyChat/Room.swift(5 hunks)Sources/AblyChat/RoomLifecycleManager.swift(5 hunks)Sources/AblyChat/RoomReactions.swift(2 hunks)Sources/AblyChat/Subscription.swift(1 hunks)Sources/AblyChat/SubscriptionHandleStorage.swift(1 hunks)Sources/AblyChat/Typing.swift(2 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(3 hunks)Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift(2 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(4 hunks)Tests/AblyChatTests/Mocks/MockRealtime.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(4 hunks)Tests/AblyChatTests/Mocks/MockRoom.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(2 hunks)
💤 Files with no reviewable changes (1)
- Sources/AblyChat/DiscontinuityEvent.swift
✅ Files skipped from review due to trivial changes (1)
- Example/AblyChatExample/Misc/Utils.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- Sources/AblyChat/InternalError.swift
- Tests/AblyChatTests/Mocks/MockRealtime.swift
- Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift
- Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
🧰 Additional context used
🧠 Learnings (7)
Sources/AblyChat/Subscription.swift (4)
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/SubscriptionHandleStorage.swift:37-40
Timestamp: 2025-05-12T21:02:28.274Z
Learning: In the AblyChat Swift codebase, the `SubscriptionHandle` struct has an `unsubscribe()` method for terminating subscriptions, not a `cancel()` method.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Example/AblyChatExample/ContentView.swift:203-209
Timestamp: 2025-05-12T21:11:08.937Z
Learning: In the AblyChat Swift codebase, SubscriptionHandle doesn't need to be retained/stored to keep subscriptions active. The subscription continues to work even if the handle is not stored, as the underlying services maintain references to the callbacks until explicitly unsubscribed via the handle's unsubscribe() method.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Example/AblyChatExample/ContentView.swift:203-209
Timestamp: 2025-05-12T21:11:08.937Z
Learning: In the AblyChat Swift codebase, SubscriptionHandle doesn't need to be retained/stored to keep subscriptions active. The subscription callbacks continue to work even when the handle is not stored, as the underlying services maintain their own storage of callbacks that remain registered until explicitly unsubscribed via the handle's unsubscribe() method.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultTyping.swift:131-138
Timestamp: 2025-05-12T21:04:36.263Z
Learning: In the AblyChat Swift codebase, SubscriptionHandle closures should capture self weakly with `[weak self]` to avoid potential retain cycles, particularly when calling channel.unsubscribe() within the closure.
Sources/AblyChat/DefaultConnection.swift (3)
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Example/AblyChatExample/ContentView.swift:203-209
Timestamp: 2025-05-12T21:11:08.937Z
Learning: In the AblyChat Swift codebase, SubscriptionHandle doesn't need to be retained/stored to keep subscriptions active. The subscription continues to work even if the handle is not stored, as the underlying services maintain references to the callbacks until explicitly unsubscribed via the handle's unsubscribe() method.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Example/AblyChatExample/ContentView.swift:203-209
Timestamp: 2025-05-12T21:11:08.937Z
Learning: In the AblyChat Swift codebase, SubscriptionHandle doesn't need to be retained/stored to keep subscriptions active. The subscription callbacks continue to work even when the handle is not stored, as the underlying services maintain their own storage of callbacks that remain registered until explicitly unsubscribed via the handle's unsubscribe() method.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultTyping.swift:131-138
Timestamp: 2025-05-12T21:04:36.263Z
Learning: In the AblyChat Swift codebase, SubscriptionHandle closures should capture self weakly with `[weak self]` to avoid potential retain cycles, particularly when calling channel.unsubscribe() within the closure.
Sources/AblyChat/SubscriptionHandleStorage.swift (3)
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Example/AblyChatExample/ContentView.swift:203-209
Timestamp: 2025-05-12T21:11:08.937Z
Learning: In the AblyChat Swift codebase, SubscriptionHandle doesn't need to be retained/stored to keep subscriptions active. The subscription callbacks continue to work even when the handle is not stored, as the underlying services maintain their own storage of callbacks that remain registered until explicitly unsubscribed via the handle's unsubscribe() method.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Example/AblyChatExample/ContentView.swift:203-209
Timestamp: 2025-05-12T21:11:08.937Z
Learning: In the AblyChat Swift codebase, SubscriptionHandle doesn't need to be retained/stored to keep subscriptions active. The subscription continues to work even if the handle is not stored, as the underlying services maintain references to the callbacks until explicitly unsubscribed via the handle's unsubscribe() method.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultTyping.swift:131-138
Timestamp: 2025-05-12T21:04:36.263Z
Learning: In the AblyChat Swift codebase, SubscriptionHandle closures should capture self weakly with `[weak self]` to avoid potential retain cycles, particularly when calling channel.unsubscribe() within the closure.
Sources/AblyChat/Occupancy.swift (1)
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultOccupancy.swift:10-13
Timestamp: 2025-05-12T21:01:14.109Z
Learning: The `Occupancy` protocol in the Ably Chat Swift SDK is annotated with `@MainActor`, making all conforming types like `DefaultOccupancy` automatically main actor-isolated without requiring explicit annotations on their methods.
Sources/AblyChat/DefaultTyping.swift (2)
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultTyping.swift:11-14
Timestamp: 2025-05-12T21:03:31.914Z
Learning: The `Typing` protocol in the Ably Chat Swift SDK is annotated with `@MainActor`, making all conforming types like `DefaultTyping` automatically main actor-isolated without requiring explicit annotations on their methods.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultTyping.swift:11-14
Timestamp: 2025-05-12T21:03:31.914Z
Learning: The `Typing` protocol in the Ably Chat Swift SDK is annotated with `@MainActor`, making all conforming types like `DefaultTyping` automatically main actor-isolated without requiring explicit annotations on their methods.
Sources/AblyChat/DefaultOccupancy.swift (1)
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultOccupancy.swift:10-13
Timestamp: 2025-05-12T21:01:14.109Z
Learning: The `Occupancy` protocol in the Ably Chat Swift SDK is annotated with `@MainActor`, making all conforming types like `DefaultOccupancy` automatically main actor-isolated without requiring explicit annotations on their methods.
Example/AblyChatExample/Mocks/MockClients.swift (1)
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Example/AblyChatExample/Mocks/MockClients.swift:0-0
Timestamp: 2025-05-16T21:04:26.221Z
Learning: In the ably-chat-swift project, mock implementations (like those in MockClients.swift) are intentionally kept simple, sometimes omitting parameter-based filtering behavior for testing simplicity.
🧬 Code Graph Analysis (8)
Sources/AblyChat/Subscription.swift (4)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)
unsubscribe(178-180)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (1)
unsubscribe(15-17)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (2)
unsubscribe(256-258)unsubscribe(368-370)Example/AblyChatExample/Mocks/MockRealtime.swift (6)
unsubscribe(154-156)unsubscribe(158-160)unsubscribe(162-164)unsubscribe(328-330)unsubscribe(332-334)unsubscribe(336-338)
Tests/AblyChatTests/Mocks/MockRoom.swift (6)
Example/AblyChatExample/Mocks/MockClients.swift (3)
onStatusChange(83-98)onStatusChange(456-469)onDiscontinuity(100-103)Sources/AblyChat/Connection.swift (2)
onStatusChange(46-60)onStatusChange(65-67)Sources/AblyChat/Room.swift (4)
onStatusChange(122-136)onStatusChange(141-143)onStatusChange(330-333)onDiscontinuity(341-344)Tests/AblyChatTests/DefaultRoomTests.swift (2)
onStatusChange(257-279)onDiscontinuity(284-306)Sources/AblyChat/Discontinuity.swift (2)
onDiscontinuity(36-49)onDiscontinuity(54-56)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
onDiscontinuity(82-85)
Sources/AblyChat/SubscriptionHandleStorage.swift (2)
Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (9)
create(15-27)create(66-77)create(122-139)subscriptionDidTerminate(29-31)subscriptionDidTerminate(79-81)subscriptionDidTerminate(141-143)emit(33-37)emit(83-87)emit(145-149)Sources/AblyChat/Subscription.swift (1)
emit(78-85)
Sources/AblyChat/DefaultPresence.swift (6)
Sources/AblyChat/DefaultOccupancy.swift (2)
subscribe(10-13)subscribe(39-72)Sources/AblyChat/DefaultTyping.swift (2)
subscribe(11-14)subscribe(74-145)Sources/AblyChat/Presence.swift (5)
subscribe(137-151)subscribe(164-178)subscribe(181-183)subscribe(186-188)toARTPresenceAction(277-288)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)
unsubscribe(178-180)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (1)
unsubscribe(15-17)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (2)
unsubscribe(256-258)unsubscribe(368-370)
Sources/AblyChat/Discontinuity.swift (5)
Example/AblyChatExample/Mocks/MockClients.swift (1)
onDiscontinuity(100-103)Sources/AblyChat/Room.swift (1)
onDiscontinuity(341-344)Sources/AblyChat/RoomLifecycleManager.swift (1)
onDiscontinuity(221-224)Sources/AblyChat/SubscriptionHandleStorage.swift (1)
emit(37-41)Sources/AblyChat/Subscription.swift (2)
emit(78-85)addTerminationHandler(103-118)
Sources/AblyChat/Connection.swift (5)
Sources/AblyChat/DefaultConnection.swift (1)
onStatusChange(20-89)Sources/AblyChat/Room.swift (3)
onStatusChange(122-136)onStatusChange(141-143)onStatusChange(330-333)Sources/AblyChat/SubscriptionHandleStorage.swift (1)
emit(37-41)Sources/AblyChat/Subscription.swift (2)
emit(78-85)addTerminationHandler(103-118)Tests/AblyChatTests/SubscriptionTests.swift (1)
emit(13-23)
Example/AblyChatExample/ContentView.swift (5)
Example/AblyChatExample/Mocks/MockClients.swift (12)
subscribe(117-140)subscribe(224-240)subscribe(254-270)subscribe(408-410)subscribe(412-414)subscribe(428-438)update(164-180)update(370-372)update(374-376)update(378-387)onStatusChange(83-98)onStatusChange(456-469)Sources/AblyChat/Room.swift (3)
onStatusChange(122-136)onStatusChange(141-143)onStatusChange(330-333)Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (3)
create(15-27)create(66-77)create(122-139)Sources/AblyChat/DefaultConnection.swift (1)
onStatusChange(20-89)Example/AblyChatExample/Misc/Utils.swift (1)
after(4-9)
Tests/AblyChatTests/DefaultMessagesTests.swift (5)
Example/AblyChatExample/Mocks/MockClients.swift (18)
send(146-162)send(212-222)subscribe(117-140)subscribe(224-240)subscribe(254-270)subscribe(408-410)subscribe(412-414)subscribe(428-438)update(164-180)update(370-372)update(374-376)update(378-387)get(26-33)get(142-144)get(272-274)get(323-333)get(335-345)get(440-442)Sources/AblyChat/DefaultMessages.swift (8)
send(26-28)send(170-176)subscribe(18-20)subscribe(66-159)update(30-32)update(178-184)get(22-24)get(162-168)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasRecord(152-164)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (3)
subscribe(142-170)emitEvent(197-201)get(78-85)Example/AblyChatExample/Mocks/MockRealtime.swift (2)
update(272-274)update(276-278)
🪛 SwiftLint (0.57.0)
Tests/AblyChatTests/DefaultMessagesTests.swift
[Error] 52-52: Force casts should be avoided
(force_cast)
[Error] 260-260: Force casts should be avoided
(force_cast)
[Error] 318-318: Force casts should be avoided
(force_cast)
[Error] 262-262: Don't include vertical whitespace (empty line) before closing braces
(vertical_whitespace_closing_braces)
⏰ Context from checks skipped due to timeout of 90000ms (34)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.2) - GitHub Check: Example app, macOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.2) - GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.1) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.1) - GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.1) - GitHub Check: Xcode, macOS (Xcode 16.1)
- GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - GitHub Check: Xcode, iOS (Xcode 16.1)
- GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Example app, iOS (Xcode 16.1)
- GitHub Check: Example app, tvOS (Xcode 16.2)
- GitHub Check: Xcode, tvOS (Xcode 16.1)
- GitHub Check: SPM (Xcode 16.3)
- GitHub Check: Example app, tvOS (Xcode 16.1)
- GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.2)
- GitHub Check: Example app, macOS (Xcode 16.1)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.1) - GitHub Check: SPM (Xcode 16.1)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.2) - GitHub Check: SPM (Xcode 16.2)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.3) - GitHub Check: Generate code coverage
🔇 Additional comments (23)
Sources/AblyChat/Subscription.swift (1)
3-7: Well-structured SubscriptionHandle implementation!The
SubscriptionHandlestruct provides a clean, type-safe way to manage unsubscription from events. The@MainActorannotation on the unsubscribe closure ensures thread safety for UI operations.Tests/AblyChatTests/Mocks/MockRoom.swift (2)
71-74: Mock implementation for onStatusChange follows protocol patternThe stub implementation aligns with the callback-based subscription pattern used throughout the codebase. The
@MainActorannotation ensures consistency with the real implementation.
76-79: Mock implementation for onDiscontinuity follows protocol patternThis stub implementation aligns with the callback-based subscription pattern used throughout the codebase. The
@discardableResultannotation is appropriate for methods where the return value might not always be needed.Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift (2)
2-2: Appropriate use of @testable importUsing
@testable import AblyChatis necessary to access internal entities in tests.
53-74: Well-implemented AsyncSequence wrapper for subscription eventsThis extension provides a bridge between the new callback-based subscription pattern and the AsyncSequence-based testing approach. It correctly manages subscription lifecycle by adding a termination handler that unsubscribes from the original callback-based subscription.
The implementation follows the pattern seen in other parts of the codebase where similar extensions exist, maintaining consistency.
Sources/AblyChat/SubscriptionHandleStorage.swift (5)
3-14: Well-designed subscription storage implementationThe
SubscriptionHandleStorageclass provides a clean, type-safe way to manage callback-based subscriptions. The class structure with a privateSubscriptionElementstruct is well-organized.
16-24: Correctly implemented subscription creation with weak self captureThe
createmethod properly generates a unique ID for each subscription and correctly capturesselfweakly in the unsubscribe closure to prevent potential retain cycles, which aligns with the best practices for this codebase.
26-30: Useful debug property for testingThe
testsOnly_subscriptionCountproperty provides a convenient way to verify subscription management in tests without exposing internal implementation details in production code.
32-34: Clean subscription terminationThe
subscriptionDidTerminatemethod correctly removes the subscription from the storage dictionary using the unique ID.
36-41: Efficient event emission to all subscriptionsThe
emitmethod properly iterates through all active subscriptions and invokes their callbacks with the provided element. This implementation is straightforward and efficient.Sources/AblyChat/DefaultConnection.swift (2)
59-67: Potential retain-cycle between timer andselfInside
timerManager.setTimerthe closure capturesselfstrongly (self.error = …).
If the connection never reconnects and the user forgets to callunsubscribe(), the timer retains the closure → the closure retainsself→DefaultConnectionstays alive and the timer never invalidates.Capture
selfweakly to eliminate the cycle:- timerManager.setTimer(interval: 5.0) { [timerManager] in + timerManager.setTimer(interval: 5.0) { [weak self, timerManager] in timerManager.cancelTimer() - callback(statusChange) + guard let self else { return } + Task { @MainActor in + callback(statusChange) + }[ suggest_essential_refactor ]
82-88: Clean, minimal unsubscribe logicThe returned
SubscriptionHandlecancels the transient-disconnect timer and unregisters the Ably listener, both under aweak selfcapture. This prevents accidental retention ofDefaultConnectionand neatly encapsulates teardown logic – nice job!Sources/AblyChat/Typing.swift (1)
51-75: Async-sequence wrapper looks solidThe adapter correctly:
- Forwards events into a
Subscriptionbuffer.- Unsubscribes when the sequence completes via a termination handler executed on the main actor.
Good balance between new callback API and backward-compatibility.
Sources/AblyChat/RoomReactions.swift (2)
20-30: Same MainActor dispatch caveat as Typing / ConnectionEnsure that concrete implementations of
RoomReactions.subscribe(_:)call thecallbackon the main actor; otherwise the@MainActorcontract is broken for SDK users.Mirror the fix proposed for
DefaultConnection(wrap invocations inTask { @MainActor in … }).
32-56: Wrapper implementation mirrors Typing – looks correctThe async-sequence adapter follows the same pattern as other modules, ensuring event forwarding and clean unsubscription. Consistency across protocols is appreciated.
Sources/AblyChat/DefaultTyping.swift (1)
137-144: 🛠️ Refactor suggestionEnsure unsubscription happens on MainActor
unsubscribe/offmutate Ably channel state that is normally expected to run on the main actor.
Dispatch back similarly to avoid cross-actor violations:- return SubscriptionHandle { - if let startedEventListener { - self.channel.unsubscribe(startedEventListener) - } - if let stoppedEventListener { - self.channel.unsubscribe(stoppedEventListener) - } - } + return SubscriptionHandle { [weak self] in + Task { @MainActor [weak self] in + guard let self else { return } + if let startedEventListener { self.channel.unsubscribe(startedEventListener) } + if let stoppedEventListener { self.channel.unsubscribe(stoppedEventListener) } + } + }⛔ Skipped due to learnings
Learnt from: maratal PR: ably/ably-chat-swift#286 File: Sources/AblyChat/DefaultOccupancy.swift:66-69 Timestamp: 2025-05-12T21:02:25.928Z Learning: SubscriptionHandle.unsubscribe is already marked with @MainActor annotation, so there's no need to wrap code in MainActor.run when calling it.Learnt from: maratal PR: ably/ably-chat-swift#286 File: Sources/AblyChat/DefaultOccupancy.swift:66-69 Timestamp: 2025-05-12T21:02:25.928Z Learning: SubscriptionHandle.unsubscribe is already annotated with @MainActor, so there's no need to wrap the code in MainActor.run when using it.Learnt from: maratal PR: ably/ably-chat-swift#286 File: Sources/AblyChat/DefaultTyping.swift:131-138 Timestamp: 2025-05-12T21:04:36.263Z Learning: In the AblyChat Swift codebase, SubscriptionHandle closures should capture self weakly with `[weak self]` to avoid potential retain cycles, particularly when calling channel.unsubscribe() within the closure.Example/AblyChatExample/ContentView.swift (4)
203-209: Ordering of side-effects before anattach()is fine – just confirming intentSubscribing before the room is attached is safe because the SDK will buffer events until
attach()completes.
No change requested – just making sure this was an intentional choice.
322-330: Good practice: time-boxed connection status loggingCapturing the handle in the
afterclosure ensures it stays alive only for the required duration and unsubscribes deterministically. Nice touch.
239-265:⚠️ Potential issueMessage stream stops after
showMessagesreturns – store theSubscriptionHandle
subscriptionHandleis a local variable; whenshowMessagesreturns it goes out of scope, is de-initialised and automatically unsubscribes.
As a result you will only receive messages that arrive during the execution ofshowMessages; live updates afterwards will be silently dropped.@@ - let subscriptionHandle = try await room.messages.subscribe { message in + // Keep the handle in a view-level state property so it lives + // for as long as the view needs realtime updates. + messagesSubscriptionHandle = try await room.messages.subscribe { message in @@ } - let previousMessages = try await subscriptionHandle.getPreviousMessages(.init()) + let previousMessages = try await messagesSubscriptionHandle!.getPreviousMessages(.init())and add a property at the top of the view:
@State private var chatClient = Environment.current.createChatClient() +@State private var messagesSubscriptionHandle: SubscriptionHandle?Repeat for the other subscription helpers (reactions, presence, typing, occupancy) or collect them in an array:
@State private var subscriptionHandles: [SubscriptionHandle] = []This prevents accidental unsubscription and gives you a place to clean up in
deinitif the example grows.⛔ Skipped due to learnings
Learnt from: maratal PR: ably/ably-chat-swift#286 File: Example/AblyChatExample/ContentView.swift:203-209 Timestamp: 2025-05-12T21:11:08.937Z Learning: In the AblyChat Swift codebase, SubscriptionHandle doesn't need to be retained/stored to keep subscriptions active. The subscription continues to work even if the handle is not stored, as the underlying services maintain references to the callbacks until explicitly unsubscribed via the handle's unsubscribe() method.Learnt from: maratal PR: ably/ably-chat-swift#286 File: Example/AblyChatExample/ContentView.swift:203-209 Timestamp: 2025-05-12T21:11:08.937Z Learning: In the AblyChat Swift codebase, SubscriptionHandle doesn't need to be retained/stored to keep subscriptions active. The subscription callbacks continue to work even when the handle is not stored, as the underlying services maintain their own storage of callbacks that remain registered until explicitly unsubscribed via the handle's unsubscribe() method.Learnt from: maratal PR: ably/ably-chat-swift#286 File: Sources/AblyChat/DefaultTyping.swift:131-138 Timestamp: 2025-05-12T21:04:36.263Z Learning: In the AblyChat Swift codebase, SubscriptionHandle closures should capture self weakly with `[weak self]` to avoid potential retain cycles, particularly when calling channel.unsubscribe() within the closure.
276-297: 🛠️ Refactor suggestionSame “lost handle” issue for reactions / presence subscriptions
subscribeToReactions,subscribeToPresence,subscribeToTypingEvents, andsubscribeToOccupancyall discard the returnedSubscriptionHandle, so the callbacks are removed immediately after the method exits.A concise pattern:
func subscribeToReactions(room: Room) { - room.reactions.subscribe { reaction in + subscriptionHandles.append( + room.reactions.subscribe { reaction in withAnimation { showReaction(reaction.displayedText) } - } + } + ) }Keeping the handles solves the same real-time drop-out described above.
⛔ Skipped due to learnings
Learnt from: maratal PR: ably/ably-chat-swift#286 File: Example/AblyChatExample/ContentView.swift:203-209 Timestamp: 2025-05-12T21:11:08.937Z Learning: In the AblyChat Swift codebase, SubscriptionHandle doesn't need to be retained/stored to keep subscriptions active. The subscription callbacks continue to work even when the handle is not stored, as the underlying services maintain their own storage of callbacks that remain registered until explicitly unsubscribed via the handle's unsubscribe() method.Learnt from: maratal PR: ably/ably-chat-swift#286 File: Example/AblyChatExample/ContentView.swift:203-209 Timestamp: 2025-05-12T21:11:08.937Z Learning: In the AblyChat Swift codebase, SubscriptionHandle doesn't need to be retained/stored to keep subscriptions active. The subscription continues to work even if the handle is not stored, as the underlying services maintain references to the callbacks until explicitly unsubscribed via the handle's unsubscribe() method.Learnt from: maratal PR: ably/ably-chat-swift#286 File: Sources/AblyChat/DefaultTyping.swift:131-138 Timestamp: 2025-05-12T21:04:36.263Z Learning: In the AblyChat Swift codebase, SubscriptionHandle closures should capture self weakly with `[weak self]` to avoid potential retain cycles, particularly when calling channel.unsubscribe() within the closure.Sources/AblyChat/RoomLifecycleManager.swift (1)
221-224: Callback-based discontinuity handler looks goodThe new
onDiscontinuitymirrors the status-change API and cleanly delegates toSubscriptionHandleStorage.
Implementation is correct and thread-safe under the actor; no issues spotted.Sources/AblyChat/Room.swift (2)
112-144: Nice ergonomic wrapper for async-sequence consumersProviding an async-sequence façade (
Subscription<RoomStatusChange>) on top of the new callback API preserves source compatibility and keeps buffering behaviour explicit. The termination handler correctly unsubscribes.Minor nit: the
Task { @MainActor in … }could be replaced withTask { subscriptionHandle.unsubscribe() }because the enclosing extension is already@MainActor, but current code is perfectly safe.
330-333: Delegation to lifecycle manager is straightforward
DefaultRoomsimply forwards status-change subscriptions toRoomLifecycleManager, ensuring single source of truth for status updates. Implementation looks correct.
maratal
force-pushed
the
fix/235-messages-tests
branch
from
9537490 to
2bbc803
Compare
maratal
force-pushed
the
fix/235-messages-tests
branch
from
5fa7ce5 to
1164f2f
Compare
maratal
force-pushed
the
fix/235-messages-tests
branch
from
1164f2f to
d85e713
Compare
maratal
force-pushed
the
259-switch-to-callbacks
branch
from
c997f5a to
3e07df6
Compare
maratal
force-pushed
the
fix/235-messages-tests
branch
from
d85e713 to
912abb3
Compare
maratal
force-pushed
the
259-switch-to-callbacks
branch
4 times, most recently
from
f3d52dc to
e694b85
Compare
maratal
force-pushed
the
fix/235-messages-tests
branch
from
912abb3 to
02b8d33
Compare
maratal
force-pushed
the
fix/235-messages-tests
branch
from
02b8d33 to
7039a6a
Compare
2 participants
Part of #235
Summary by CodeRabbit
New Features
Bug Fixes
Chores