[ECO-5287] Switch to callbacks #286
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThis change refactors the chat SDK and example app to use callback-based subscription APIs instead of Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK
participant SubscriptionHandle
Client->>SDK: subscribeToEvent(callback)
SDK->>SubscriptionHandle: create with unsubscribe logic
SDK-->>Client: return SubscriptionHandle
SDK-->>Client: invoke callback(event) on event
Client->>SubscriptionHandle: unsubscribe()
SubscriptionHandle-->>SDK: remove callback
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
Poem
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🔭 Outside diff range comments (2)
Sources/AblyChat/DefaultTyping.swift (1)
74-107: 🛠️ Refactor suggestionChannel callbacks may arrive off the main actor – hop back before touching main-actor state
Implementationis@MainActor, but the Ably channel likely calls its listeners on a background queue. Accessingself,typingTimerManager, and emitting callbacks violates isolation.Wrap the body in
await MainActor.run { … }(or mark the closure@MainActorif the SDK guarantees delivery on the main thread):- let startedEventListener = channel.subscribe(TypingEvents.started.rawValue) { [weak self] message in - guard let self, let messageClientID = message.clientId else { - return - } + let startedEventListener = channel.subscribe(TypingEvents.started.rawValue) { [weak self] message in + await MainActor.run { + guard let self, let messageClientID = message.clientId else { return } … + } }Repeat the same pattern for
stoppedEventListener.Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (1)
33-37:⚠️ Potential issue💥 Possible crash: dictionary mutated while being enumerated
emit(_:)iterates directly oversubscriptions.values.
If any callback triggersunsubscribe()during that iteration (which removes the entry from the same dictionary), the collection is mutated while being enumerated and will trap at runtime with: “dictionary was mutated while being enumerated”.The same pattern appears three times (generic, handle, and message‐handle storages).
Copy the values to a temporary array before iterating, or defer removal until after emission.- for subscription in subscriptions.values { - subscription.subscription?.emit(element) - } + let currentSubscriptions = Array(subscriptions.values) // copy to avoid concurrent mutation + for subscription in currentSubscriptions { + subscription.subscription?.emit(element) + }Replicate the same change for
MockSubscriptionHandleStorage.emitandMockMessageSubscriptionHandleStorage.emit.Also applies to: 81-85, 139-143
♻️ Duplicate comments (1)
Example/AblyChatExample/ContentView.swift (1)
277-320: Same lifetime issue for all other subscription helpers
subscribeToReactions,subscribeToPresence,subscribeToTypingEvents, andsubscribeToOccupancysuffer from the same problem: the returned handle is ignored. Re-structure them to return the handle and persist it, or inject aSubscriptionHandleStoragethat retains them.
🧹 Nitpick comments (18)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)
137-139: Consider explicit @mainactor for TaskGood change to run the callback asynchronously, which better simulates real-world behavior. Consider making the main actor context explicit by using
Task { @MainActor into match the callback's annotation, although the current implementation will still work correctly because of the callback's own annotation.- Task { + Task { @MainActor in callback(messageToEmitOnSubscribe) }Tests/AblyChatTests/Mocks/MockRoom.swift (2)
71-74: Consider no-op implementation instead of fatalErrorThe stub implementation with
fatalErroris appropriate if these methods aren't expected to be called in tests. However, consider providing a simple no-op implementation that returns an emptySubscriptionHandlefor better test flexibility without requiring explicit mocking of every method.@discardableResult func onStatusChange(_ callback: @escaping ((RoomStatusChange) -> Void)) -> SubscriptionHandle { - fatalError("Not implemented") + return SubscriptionHandle { /* No-op */ } }
76-79: Consider no-op implementation instead of fatalErrorSimilar to the suggestion for
onStatusChange, consider providing a simple no-op implementation unless you need to catch unintended usage in tests.@discardableResult func onDiscontinuity(_ callback: @escaping ((DiscontinuityEvent) -> Void)) -> SubscriptionHandle { - fatalError("Not implemented") + return SubscriptionHandle { /* No-op */ } }Example/AblyChatExample/Misc/Utils.swift (1)
14-14: Handle task cancellation errorThe
try?suppresses all errors, including cancellation. Since cancellation is a normal control flow mechanism, consider handling it explicitly.- try? await Task.sleep(nanoseconds: UInt64(interval * 1_000_000_000)) + do { + try await Task.sleep(nanoseconds: UInt64(interval * 1_000_000_000)) + } catch is CancellationError { + break + } catch { + // Unexpected error, could log here + }Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift (1)
54-55: Remove superfluous blank line after opening braceSwiftLint flags
vertical_whitespace_opening_braces; the empty line immediately after the opening brace breaks the rule.-internal extension RoomLifecycleManager { - +internal extension RoomLifecycleManager {🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 55-55: Don't include vertical whitespace (empty line) after opening braces
(vertical_whitespace_opening_braces)
Sources/AblyChat/DefaultConnection.swift (1)
59-67: Timer callback retainsDefaultConnection– captureselfweakly
timerManager.setTimercapturesselfstrongly.
If the caller abandons theSubscriptionHandlebefore the timer fires, the timer retains the entireDefaultConnectioninstance for at least 5 s, which is unnecessary and may delay deinit in tests.- timerManager.setTimer(interval: 5.0) { [timerManager] in + timerManager.setTimer(interval: 5.0) { [timerManager, weak self] in + guard let self else { return }Sources/AblyChat/DefaultOccupancy.swift (1)
41-44: Copy-paste typo: references “presence” instead of “occupancy”The fatal error message mentions presence events, but this API deals with occupancy events, which will mislead users.
-fatalError("In order to be able to subscribe to presence events, please set enableEvents to true in the room's occupancy options.") +fatalError("In order to be able to subscribe to occupancy events, please set enableEvents to true in the room's occupancy options.")Sources/AblyChat/Discontinuity.swift (1)
3-9: Make the stored property immutable
errornever needs to be mutated after construction. Preferletovervarfor value-type safety.- public var error: ARTErrorInfo + public let error: ARTErrorInfoSources/AblyChat/RoomReactions.swift (1)
42-56: Consider factoring out duplicated bridging logicThe “create
Subscription, wire callback, add termination handler” pattern now exists in at least six files (Typing, Presence, Occupancy, Messages, Discontinuity, Reactions). Extracting a small helper:func bridge<Elem>( bufferingPolicy: BufferingPolicy, subscribe: (@escaping (Elem) -> Void) -> SubscriptionHandle ) -> Subscription<Elem> { … }would remove duplication and ensure consistency.
Sources/AblyChat/Typing.swift (2)
15-21: Callback execution context should be explicitSame remark as for RoomReactions: specify (or enforce) on which actor/queue
callbackis run.
This prevents subtle UI thread bugs in clients.
78-82: Minor naming nitpick
subscribe()overload without parameters could be renamedsubscribeUnbounded()to communicate buffering behaviour explicitly, matchingsubscribe(bufferingPolicy:).Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (1)
66-76: SwiftLint formatting violations flaggedSwiftLint reports
multiline_parameters_brackets&opening_braceerrors on the highlighted ranges.
Although purely stylistic, they will fail CI if linting is enforced. Re-indent or put the surrounding parentheses on their own lines to silence the linter.Also applies to: 95-104, 119-128
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 66-66: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
[Error] 68-68: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
Sources/AblyChat/Occupancy.swift (1)
44-58: Minor: retain cycle not possible but unnecessary strong capture
subscriptionHandleis captured strongly inside the termination handler, but it is only needed to callunsubscribe()once.
Capture it weakly (or move the call outside the asyncTask) to avoid prolonging its lifetime:- subscription.addTerminationHandler { - Task { @MainActor in - subscriptionHandle.unsubscribe() - } - } + subscription.addTerminationHandler { [handle = subscriptionHandle] in + Task { @MainActor in + handle.unsubscribe() + } + }This keeps semantics identical while making the ownership explicit.
Sources/AblyChat/Connection.swift (1)
23-29: Docstring wording – “handle” not “handler”The return value is a
SubscriptionHandle, so the sentence should read “A subscription handle …” to keep terminology consistent with other protocols (Messages,Occupancy, …).- * - Returns: A subscription handler that can be used to unsubscribe from ``ConnectionStatusChange`` events. + * - Returns: A subscription handle that can be used to unsubscribe from ``ConnectionStatusChange`` events.Same nit applies in
Occupancy.swift.Sources/AblyChat/Presence.swift (1)
125-178: Nice backward-compatibility wrapper – but remember to cancel theTaskThe termination handler launches an un-awaited
Tasksolely to callunsubscribe().
Although harmless, you can simply callsubscriptionHandle.unsubscribe()synchronously because the surroundingaddTerminationHandleralready executes on the main actor. This avoids an extra detached task allocation.This is purely a micro-optimisation; feel free to ignore.
Sources/AblyChat/Messages.swift (1)
308-335: Simplify error propagation ingetPreviousMessagesSince the closure already returns
ARTErrorInfo, thetry … throws(ARTErrorInfo)wrapper is no longer needed:-public func getPreviousMessages(params: QueryOptions) async throws(ARTErrorInfo) -> any PaginatedResult<Message> { - try await getPreviousMessages(params) -} +public func getPreviousMessages(params: QueryOptions) async throws(ARTErrorInfo) -> any PaginatedResult<Message> { + try await getPreviousMessages(params) +}The method can be inlined or kept as is; consider removing the wrapper to reduce indirection.
Sources/AblyChat/Room.swift (1)
112-144: AsyncSequence bridge is well-implemented but can leak ifSubscriptionoutlives the callbackThe helper properly forwards events and unsubscribes on termination, but the termination handler captures
subscriptionHandlestrongly. If client code accidentally stores theSubscriptionforever the handle is never released, causing the underlying callback to remain subscribed indefinitely.Diff suggestion to break the retain cycle:
- let subscriptionHandle = onStatusChange { statusChange in - subscription.emit(statusChange) - } + weak var weakHandle: SubscriptionHandle? + let subscriptionHandle = onStatusChange { statusChange in + subscription.emit(statusChange) + } + weakHandle = subscriptionHandle ... - subscriptionHandle.unsubscribe() + weakHandle?.unsubscribe()Example/AblyChatExample/Mocks/MockClients.swift (1)
422-428: SwiftLint multiline-arguments violations – fix formatting to keep CI greenStatic-analysis reports several
multiline_argumentserrors (e.g. here). Re-indent so each argument is on its own line or all on one line:- ConnectionStatusChange(current: [.connected, .connecting].randomElement()!, - previous: [.suspended, .disconnected].randomElement()!, - retryIn: 1) + ConnectionStatusChange( + current: [.connected, .connecting].randomElement()!, + previous: [.suspended, .disconnected].randomElement()!, + retryIn: 1 + )🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 427-427: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Error] 424-424: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 426-426: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
Example/AblyChatExample/ContentView.swift(2 hunks)Example/AblyChatExample/Misc/Utils.swift(1 hunks)Example/AblyChatExample/Mocks/MockClients.swift(10 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(5 hunks)Sources/AblyChat/Discontinuity.swift(1 hunks)Sources/AblyChat/DiscontinuityEvent.swift(0 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/Helpers/Subscription+RoomStatusChange.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(1 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
🧰 Additional context used
🧬 Code Graph Analysis (7)
Sources/AblyChat/RoomReactions.swift (6)
Sources/AblyChat/Messages.swift (4)
subscribe(86-107)subscribe(110-112)emit(324-326)addTerminationHandler(328-331)Sources/AblyChat/Presence.swift (4)
subscribe(137-151)subscribe(164-178)subscribe(181-183)subscribe(186-188)Sources/AblyChat/Occupancy.swift (2)
subscribe(44-58)subscribe(63-65)Sources/AblyChat/Typing.swift (2)
subscribe(61-75)subscribe(80-82)Sources/AblyChat/DefaultRoomReactions.swift (2)
subscribe(14-16)subscribe(53-101)Sources/AblyChat/Subscription.swift (2)
emit(78-85)addTerminationHandler(103-118)
Tests/AblyChatTests/Mocks/MockRoom.swift (4)
Example/AblyChatExample/Mocks/MockClients.swift (3)
onStatusChange(83-95)onStatusChange(421-428)onDiscontinuity(97-100)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)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
onDiscontinuity(68-71)
Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift (4)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
onRoomStatusChange(63-66)Sources/AblyChat/RoomLifecycleManager.swift (1)
onRoomStatusChange(177-180)Sources/AblyChat/SubscriptionHandleStorage.swift (1)
emit(37-41)Sources/AblyChat/Subscription.swift (2)
emit(78-85)addTerminationHandler(103-118)
Sources/AblyChat/DefaultPresence.swift (5)
Sources/AblyChat/Presence.swift (4)
subscribe(137-151)subscribe(164-178)subscribe(181-183)subscribe(186-188)Sources/AblyChat/DefaultOccupancy.swift (2)
subscribe(10-13)subscribe(39-70)Sources/AblyChat/DefaultTyping.swift (2)
subscribe(11-14)subscribe(74-139)Sources/AblyChat/DefaultRoomReactions.swift (2)
subscribe(14-16)subscribe(53-101)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (2)
unsubscribe(256-258)unsubscribe(368-370)
Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (5)
Example/AblyChatExample/Misc/Utils.swift (1)
periodic(11-20)Sources/AblyChat/SubscriptionStorage.swift (3)
create(18-30)subscriptionDidTerminate(38-40)emit(43-47)Sources/AblyChat/Messages.swift (2)
emit(324-326)getPreviousMessages(333-335)Sources/AblyChat/Subscription.swift (1)
emit(78-85)Example/AblyChatExample/Mocks/MockSubscription.swift (1)
emit(13-15)
Sources/AblyChat/SubscriptionHandleStorage.swift (1)
Sources/AblyChat/Subscription.swift (1)
emit(78-85)
Sources/AblyChat/Presence.swift (5)
Sources/AblyChat/Messages.swift (4)
subscribe(86-107)subscribe(110-112)emit(324-326)addTerminationHandler(328-331)Sources/AblyChat/Occupancy.swift (2)
subscribe(44-58)subscribe(63-65)Sources/AblyChat/RoomReactions.swift (2)
subscribe(42-56)subscribe(61-63)Sources/AblyChat/DefaultPresence.swift (4)
subscribe(46-49)subscribe(51-54)subscribe(264-285)subscribe(287-311)Sources/AblyChat/Subscription.swift (2)
emit(78-85)addTerminationHandler(103-118)
🪛 SwiftLint (0.57.0)
Sources/AblyChat/DefaultOccupancy.swift
[Error] 67-67: Conditional statements should always return on the next line
(conditional_returns_on_newline)
Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift
[Error] 55-55: Don't include vertical whitespace (empty line) after opening braces
(vertical_whitespace_opening_braces)
Sources/AblyChat/Messages.swift
[Error] 87-87: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift
[Error] 125-125: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 128-128: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 66-66: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
[Error] 68-68: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
[Error] 95-95: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
[Error] 99-99: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
[Error] 119-119: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
[Error] 122-122: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
[Error] 100-100: Opening braces should be preceded by a single space and on the same line as the declaration
(opening_brace)
[Error] 123-123: Opening braces should be preceded by a single space and on the same line as the declaration
(opening_brace)
Sources/AblyChat/DefaultMessages.swift
[Error] 138-138: Conditional statements should always return on the next line
(conditional_returns_on_newline)
Example/AblyChatExample/Mocks/MockClients.swift
[Error] 131-131: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Error] 227-227: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Error] 254-254: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Error] 289-289: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Error] 402-402: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Error] 427-427: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Error] 424-424: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 426-426: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Xcode, tvOS (Xcode 16.1)
- GitHub Check: Xcode, iOS (Xcode 16.1)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Xcode, macOS (Xcode 16.1)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: Generate code coverage
- GitHub Check: lint
🔇 Additional comments (9)
Sources/AblyChat/Subscription.swift (1)
3-7: Well-structured SubscriptionHandle implementationThe new
SubscriptionHandlestruct is well-designed with appropriateSendableconformance and@MainActorannotation on the unsubscribe closure. This provides a clear, consistent way to manage subscription lifecycles across the codebase as part of the transition to callback-based APIs.Sources/AblyChat/DefaultRoomReactions.swift (1)
58-93: Clarify / standardise the execution context ofcallback
channel.subscribemay dispatch its listener on an internal Ably queue, not the main actor.
BecauseImplementationis@MainActor, clients could (reasonably) assume the providedcallbackis also invoked on the main actor. Please either:
- Explicitly hop to the main actor:
-logger.log(message: "Emitting reaction: \(reaction)", level: .debug) -callback(reaction) +logger.log(message: "Emitting reaction: \(reaction)", level: .debug) +Task { @MainActor in + callback(reaction) +}
- Or document that the callback is not guaranteed to run on any specific actor/queue.
Leaving this implicit may lead to subtle UI-thread bugs.
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
63-71: Isolation mismatch: calling@MainActorstorage from a non-isolated context
SubscriptionHandleStorageis@MainActor.
onRoomStatusChangeandonDiscontinuityaccess it synchronously from a non-isolated method, which violates actor isolation and will fail to compile under strict concurrency checking.Consider:
- @discardableResult - func onRoomStatusChange(_ callback: @escaping ((RoomStatusChange) -> Void)) -> SubscriptionHandle { + @MainActor + @discardableResult + func onRoomStatusChange(_ callback: @escaping (RoomStatusChange) -> Void) -> SubscriptionHandle { roomStatusSubscriptions.create(callback) } - @discardableResult - func onDiscontinuity(_ callback: @escaping ((DiscontinuityEvent) -> Void)) -> SubscriptionHandle { + @MainActor + @discardableResult + func onDiscontinuity(_ callback: @escaping (DiscontinuityEvent) -> Void) -> SubscriptionHandle { discontinuitySubscriptions.create(callback) }Alternatively, wrap the calls in
await MainActor.run { … }.Sources/AblyChat/Discontinuity.swift (1)
36-49: Potential cross-actor call intoSubscription.emit
onDiscontinuity { … }may execute the callback on an Ably internal queue rather than theMainActor.
BecauseSubscription.emit(_:)is not actor-isolated, multiple threads could call it concurrently, risking data races in the underlyingAsyncStream.Wrap the emission in a
Task { @MainActor in … }, mirroring the unsubscription path:- let subscriptionHandle = onDiscontinuity { statusChange in - subscription.emit(statusChange) - } + let subscriptionHandle = onDiscontinuity { statusChange in + Task { @MainActor in + subscription.emit(statusChange) + } + }Please confirm that Ably always invokes listeners on the main queue; if not, the above guard is required.
Sources/AblyChat/RoomReactions.swift (1)
20-30: Document thread on which callbacks are executedFor consumers it is important to know whether
subscribe(_:)invokescallbackon theMainActor, the Ably callback queue, or a background thread. Add a note to the doc-comment or enforce delivery onMainActorwith@MainActoron the method signature.@discardableResult @MainActor // ← if delivery should be on the main actor func subscribe(_ callback: @escaping (Reaction) -> Void) -> SubscriptionHandle[ suggest_nitpick ]
Sources/AblyChat/DefaultMessages.swift (1)
129-136: Potentially missing@MainActordispatch for UI callers
callback(message)is invoked directly from Ably’s event listener queue.
If UI layers assume the closure runs on the main actor (e.g.ContentView), they will mutate state off-thread. Consider delivering on the main actor:callbackOnMain(message) func callbackOnMain(_ message: Message) { Task { @MainActor in callback(message) } }or document the threading contract clearly.
Sources/AblyChat/Room.swift (3)
7-8:Roomnow conforms toProvidesDiscontinuity– looks correctAdding
ProvidesDiscontinuityto theRoomprotocol makes the new callback-based discontinuity API available to all room types and keeps the public surface consistent with the rest of the refactor. No functional or naming issues spotted here.
69-75: Callback signature & return type are clear and SwiftyThe new
onStatusChange(_:) -> SubscriptionHandlemethod matches the rest of the SDK and keeps the closure non-escaping until stored bySubscriptionHandle. Good job marking it@discardableResultso callers can silently ignore the handle if they wish.
330-334: Delegation tolifecycleManageris straightforward
DefaultRoomsimply forwards both callbacks toRoomLifecycleManager; no issues observed.
Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Example/AblyChatExample/Mocks/MockClients.swift (1)
97-100:⚠️ Potential issue
onDiscontinuitycrashes at runtime – must be implementedThe
fatalError("Not yet implemented")violates theRoomcontract and will crash the example app the first time a discontinuity subscription is created.Replace with a stub implementation like:
- @discardableResult - func onDiscontinuity(_ callback: @escaping ((DiscontinuityEvent) -> Void)) -> SubscriptionHandle { - fatalError("Not yet implemented") - } + @discardableResult + func onDiscontinuity(_ callback: @escaping ((DiscontinuityEvent) -> Void)) -> SubscriptionHandle { + // Emit no events – sufficient for most UI tests + SubscriptionHandle {} // immediately unsubscribed + }
🧹 Nitpick comments (6)
Example/AblyChatExample/Mocks/MockClients.swift (6)
114-132: Fix Swift formatting issues insubscribemethodThe multiline arguments should be formatted according to the SwiftLint rules.
func subscribe(_ callback: @escaping ((Message) -> Void)) async throws(ARTErrorInfo) -> MessageSubscriptionHandle { mockSubscriptions.create( randomElement: { Message( serial: "\(Date().timeIntervalSince1970)", action: .create, clientID: MockStrings.names.randomElement()!, roomID: self.roomID, text: MockStrings.randomPhrase(), createdAt: Date(), metadata: [:], headers: [:], version: "", timestamp: Date(), operation: nil ) }, previousMessages: { _ in MockMessagesPaginatedResult(clientID: self.clientID, roomID: self.roomID) - }, interval: 3, callback: callback) + }, + interval: 3, + callback: callback + ) }🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 131-131: Arguments should be either on the same line, or one per line
(multiline_arguments)
216-228: Fix Swift formatting issues insubscribemethodThe multiline arguments should be formatted according to the SwiftLint rules.
@discardableResult func subscribe(_ callback: @escaping ((Reaction) -> Void)) -> SubscriptionHandle { mockSubscriptions.create( randomElement: { Reaction( type: ReactionType.allCases.randomElement()!.emoji, metadata: [:], headers: [:], createdAt: Date(), clientID: self.clientID, isSelf: false ) - }, interval: 0.5, callback: callback) + }, + interval: 0.5, + callback: callback + ) }🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 227-227: Arguments should be either on the same line, or one per line
(multiline_arguments)
242-254: Fix Swift formatting issues insubscribemethodThe multiline arguments should be formatted according to the SwiftLint rules.
@discardableResult func subscribe(_ callback: @escaping ((TypingSetEvent) -> Void)) -> SubscriptionHandle { mockSubscriptions.create( randomElement: { TypingSetEvent( type: .setChanged, currentlyTyping: [ MockStrings.names.randomElement()!, MockStrings.names.randomElement()!, ], change: .init(clientId: MockStrings.names.randomElement()!, type: .started) ) - }, interval: 2, callback: callback) + }, + interval: 2, + callback: callback + ) }🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 253-253: Arguments should be either on the same line, or one per line
(multiline_arguments)
292-301: Fix Swift formatting issues increateSubscriptionmethodThe multiline arguments should be formatted according to the SwiftLint rules.
private func createSubscription(callback: @escaping (PresenceEvent) -> Void) -> SubscriptionHandle { mockSubscriptions.create( randomElement: { PresenceEvent( action: [.enter, .leave].randomElement()!, clientID: MockStrings.names.randomElement()!, timestamp: Date(), data: nil ) - }, interval: 5, callback: callback) + }, + interval: 5, + callback: callback + ) }🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 300-300: Arguments should be either on the same line, or one per line
(multiline_arguments)
408-414: Fix Swift formatting issues insubscribemethodThe multiline arguments should be formatted according to the SwiftLint rules.
@discardableResult func subscribe(_ callback: @escaping ((OccupancyEvent) -> Void)) -> SubscriptionHandle { mockSubscriptions.create( randomElement: { let random = Int.random(in: 1...10) return OccupancyEvent(connections: random, presenceMembers: Int.random(in: 0...random)) - }, interval: 2, callback: callback) + }, + interval: 2, + callback: callback + ) }🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 413-413: Arguments should be either on the same line, or one per line
(multiline_arguments)
432-439: Fix Swift formatting issues inonStatusChangemethodThe multiline arguments should be formatted according to the SwiftLint rules.
@discardableResult func onStatusChange(_ callback: @escaping ((ConnectionStatusChange) -> Void)) -> SubscriptionHandle { mockSubscriptions.create( randomElement: { - ConnectionStatusChange(current: [.connected, .connecting].randomElement()!, - previous: [.suspended, .disconnected].randomElement()!, - retryIn: 1) + ConnectionStatusChange( + current: [.connected, .connecting].randomElement()!, + previous: [.suspended, .disconnected].randomElement()!, + retryIn: 1 + ) - }, interval: 5, callback: callback) + }, + interval: 5, + callback: callback + ) }🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 438-438: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Error] 435-435: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 437-437: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
Example/AblyChatExample/Mocks/MockClients.swift(11 hunks)Sources/AblyChat/DefaultTyping.swift(6 hunks)Sources/AblyChat/Typing.swift(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/AblyChat/Typing.swift
- Sources/AblyChat/DefaultTyping.swift
🧰 Additional context used
🧠 Learnings (1)
Example/AblyChatExample/Mocks/MockClients.swift (2)
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.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Example/AblyChatExample/Mocks/MockClients.swift:83-95
Timestamp: 2025-05-12T21:46:44.489Z
Learning: The `periodic` function in Example/AblyChatExample/Misc/Utils.swift is designed to execute its closure on the main actor (`@MainActor`), ensuring thread safety when used with shared mutable state that's only accessed from the main actor.
🧬 Code Graph Analysis (1)
Example/AblyChatExample/Mocks/MockClients.swift (13)
Sources/AblyChat/DefaultConnection.swift (1)
onStatusChange(20-87)Sources/AblyChat/Room.swift (4)
onStatusChange(122-136)onStatusChange(141-143)onStatusChange(330-333)onDiscontinuity(341-344)Tests/AblyChatTests/Mocks/MockRoom.swift (4)
onStatusChange(43-45)onStatusChange(71-74)onDiscontinuity(76-79)onDiscontinuity(81-83)Example/AblyChatExample/Misc/Utils.swift (1)
periodic(11-20)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
onDiscontinuity(68-71)Sources/AblyChat/RoomLifecycleManager.swift (1)
onDiscontinuity(221-224)Sources/AblyChat/DefaultMessages.swift (2)
subscribe(18-20)subscribe(66-157)Sources/AblyChat/DefaultTyping.swift (2)
subscribe(11-14)subscribe(74-145)Sources/AblyChat/Presence.swift (4)
subscribe(137-151)subscribe(164-178)subscribe(181-183)subscribe(186-188)Sources/AblyChat/DefaultOccupancy.swift (2)
subscribe(10-13)subscribe(39-70)Sources/AblyChat/DefaultPresence.swift (4)
subscribe(46-49)subscribe(51-54)subscribe(264-285)subscribe(287-311)Sources/AblyChat/DefaultRoomReactions.swift (2)
subscribe(14-16)subscribe(53-101)Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (3)
create(15-27)create(66-75)create(119-133)
🪛 SwiftLint (0.57.0)
Example/AblyChatExample/Mocks/MockClients.swift
[Error] 131-131: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Error] 227-227: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Error] 253-253: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Error] 300-300: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Error] 413-413: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Error] 438-438: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Error] 435-435: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 437-437: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
⏰ Context from checks skipped due to timeout of 90000ms (34)
- GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Example app, tvOS (Xcode 16.2)
- GitHub Check: Example app, iOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Example app, tvOS (Xcode 16.1)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.2) - GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.2) - GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - GitHub Check: Xcode, tvOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.1) - GitHub Check: SPM (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.1) - GitHub Check: SPM,
releaseconfiguration (Xcode 16.3) - GitHub Check: SPM (Xcode 16.2)
- GitHub Check: Xcode, macOS (Xcode 16.1)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.2) - GitHub Check: SPM (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.1) - GitHub Check: SPM,
releaseconfiguration (Xcode 16.1) - GitHub Check: Generate code coverage
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Sources/AblyChat/Messages.swift (1)
100-104: Consider using async closure for unsubscribe.The termination handler wraps the
unsubscribe()call in aTask { @MainActor in ... }block, which suggests theunsubscribe()method requires the main actor but isn't itself marked as@MainActor. This is a bit inconsistent with theunsubscribefunction on line 357 which is explicitly marked as@MainActor.Consider making the termination handler's Task creation more explicit about its purpose:
subscription.addTerminationHandler { - Task { @MainActor in + // Ensure unsubscribe runs on the main actor as required + Task { @MainActor [weak subscriptionHandle] in + guard let handle = subscriptionHandle else { return } subscriptionHandle.unsubscribe() } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (9)
Example/AblyChatExample/Mocks/MockClients.swift(9 hunks)Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift(2 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/Messages.swift(6 hunks)Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoom.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- Tests/AblyChatTests/Mocks/MockRoom.swift
- Sources/AblyChat/DefaultConnection.swift
- Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift
- Sources/AblyChat/DefaultOccupancy.swift
- Sources/AblyChat/DefaultPresence.swift
- Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift
- Sources/AblyChat/DefaultMessages.swift
- Example/AblyChatExample/Mocks/MockClients.swift
🧰 Additional context used
🧠 Learnings (1)
Sources/AblyChat/Messages.swift (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (34)
- GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Example app, tvOS (Xcode 16.2)
- GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.2)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Example app, tvOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Example app, iOS (Xcode 16.1)
- GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.2) - GitHub Check: Example app, macOS (Xcode 16.1)
- GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.2) - GitHub Check: Xcode, tvOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - GitHub Check: SPM (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.1)
- GitHub Check: SPM (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.1) - GitHub Check: Xcode, macOS (Xcode 16.1)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.1) - GitHub Check: SPM (Xcode 16.1)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.2) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.1) - GitHub Check: SPM,
releaseconfiguration (Xcode 16.1) - GitHub Check: Generate code coverage
🔇 Additional comments (4)
Sources/AblyChat/Messages.swift (4)
15-19: Good usage of the callback pattern for subscription.The transition to a callback-based API with an explicit subscription handle is a good architectural choice. This pattern makes subscription management clearer and more explicit compared to the previous AsyncSequence approach.
96-98: Good use of weak capture for subscription reference.The implementation correctly uses
[weak subscription]to avoid potential retain cycles, consistent with the learning from previous reviews. This prevents memory leaks where the subscription and its handlers might hold strong references to each other.
307-335: Improved error handling with consistent ARTErrorInfo.The change to use
ARTErrorInfoconsistently throughout the error handling path improves the API's consistency. The simplification of thegetPreviousMessagesimplementation in line 334 is cleaner and directly propagates the original error without unnecessary conversion.
354-361: Well-designed subscription handle.The new
MessageSubscriptionHandlestruct is a good design pattern that encapsulates the subscription management functions clearly. It separates the concerns of subscription management from the message sequence iteration, making the API more modular and easier to use.The explicit documentation of the unsubscribe and getPreviousMessages closures improves developer understanding of the API.
maratal
force-pushed
the
259-switch-to-callbacks
branch
from
84df281 to
3c2bb03
Compare
maratal
force-pushed
the
259-switch-to-callbacks
branch
from
84df281 to
eadad04
Compare
maratal
force-pushed
the
259-switch-to-callbacks
branch
from
eadad04 to
19ecedf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
Sources/AblyChat/DefaultRoomReactions.swift (1)
54-93: 🛠️ Refactor suggestionEnsure callback is executed on the Main-Actor
subscribe(_:)promises that the suppliedcallbackwill be called on the main actor, yet thechannel.subscribeclosure may be invoked from any queue Ably chooses (even if we currently configure it to the main queue – that guarantee is outside this API contract).Wrap the invocation to honour the annotation and avoid accidental data races:
- let reaction = Reaction( + let reaction = Reaction( type: dto.type, metadata: dto.metadata ?? [:], headers: dto.headers ?? [:], createdAt: timestamp, clientID: messageClientID, isSelf: messageClientID == clientID ) - logger.log(message: "Emitting reaction: \(reaction)", level: .debug) - callback(reaction) + logger.log(message: "Emitting reaction: \(reaction)", level: .debug) + Task { @MainActor in + callback(reaction) + }This keeps the public contract intact even if the Ably dispatch queue is changed in the future.
♻️ Duplicate comments (3)
Sources/AblyChat/RoomLifecycleManager.swift (1)
593-607:⚠️ Potential issueStill-unresolved crash & suspension risk – remove IUO and capture
selfstronglyThe force-unwrapped variables (
SubscriptionHandle!,RoomStatusChange!) will crash ifonRoomStatusChangeunexpectedly returnsnil.
Additionally, capturingselfweakly allows the continuation to remain un-resumed if the manager is deallocated mid-wait, leaving a suspended task forever.- var nextRoomStatusSubscription: SubscriptionHandle! - var nextRoomStatusChange: RoomStatusChange! - await withCheckedContinuation { [weak self] (continuation: CheckedContinuation<Void, _>) in + var nextRoomStatusSubscription: SubscriptionHandle? + var nextRoomStatusChange: RoomStatusChange? + await withCheckedContinuation { (continuation: CheckedContinuation<Void, _>) in self?.logger.log(message: "waitToBeAbleToPerformPresenceOperations waiting for status change", level: .debug) @@ - nextRoomStatusSubscription.unsubscribe() + nextRoomStatusSubscription?.unsubscribe() @@ - if nextRoomStatusChange.current != .attached(error: nil) { + guard let change = nextRoomStatusChange, change.current == .attached(error: nil) else {These tweaks (1) eliminate a potential crash, (2) guarantee the continuation is always completed, and (3) avoid actor-hopping by keeping everything on the main actor.
This exact issue was raised in an earlier review and remains outstanding.Sources/AblyChat/RoomReactions.swift (1)
45-53: 🛠️ Refactor suggestionEmit on Main-Actor to avoid data races
subscription.emitmutates internal state that is not main-actor-isolated.
Calling it from an arbitrary context may introduce races. The fix mirrors the pattern already applied in other bridging helpers:- let subscriptionHandle = subscribe { reaction in - subscription.emit(reaction) + let subscriptionHandle = subscribe { reaction in + Task { @MainActor in + subscription.emit(reaction) + } }Same recommendation was provided previously; still pending.
Sources/AblyChat/Messages.swift (1)
86-107:⚠️ Potential issuePotential race condition in subscription implementation.
There's a race condition where messages received between establishing the subscription (line 88) and assigning the
emitMessageclosure (line 96) will be lost becauseemitMessageis stillnilduring that window.Consider rewriting the implementation to avoid this race condition by initializing all components before starting the actual subscription:
func subscribe(bufferingPolicy: BufferingPolicy) async throws(ARTErrorInfo) -> MessageSubscription { let subscription = MessageSubscription( bufferingPolicy: bufferingPolicy, getPreviousMessages: { _ in // Temporary placeholder that will be updated once we have the subscription handle throw ARTErrorInfo(code: 0, message: "Subscription not fully initialized") } ) + // Create a wrapper to update the getPreviousMessages implementation later + var getMessagesImpl: ((QueryOptions) async throws(ARTErrorInfo) -> any PaginatedResult<Message>)? + + // Use reflection to update the getPreviousMessages implementation + subscription.addTerminationHandler { + Task { @MainActor in + // Will be set after subscribe call below + let handle = await _handle + handle.unsubscribe() + } + } + + var _handle: MessageSubscriptionHandle! + + // Subscribe with a closure that directly forwards to the subscription + _handle = try await subscribe { [weak subscription] message in + subscription?.emit(message) + } + + // Update the getPreviousMessages implementation + // This requires modifying MessageSubscription to allow updating this closure + subscription._updateGetPreviousMessages(_handle.getPreviousMessages) return subscription }
🧹 Nitpick comments (4)
Sources/AblyChat/RoomLifecycleManager.swift (1)
600-603: Unsubscribe crosses actor boundary
SubscriptionHandle.unsubscribe()is@MainActor. Because the whole manager conforms to the@MainActorprotocol, this method is indeed running on the main actor, so the call is currently safe. Please keep this in mind if you ever remove@MainActorfrom the protocol.Sources/AblyChat/DefaultRoomReactions.swift (1)
98-100: Minor: early-exit wheneventListenerisnil
channel.subscribecan legally returnnil. A quick guard avoids scheduling needless work:- return SubscriptionHandle { [weak self] in - self?.channel.unsubscribe(eventListener) + guard let eventListener else { + return SubscriptionHandle {} + } + + return SubscriptionHandle { [weak self] in + self?.channel.unsubscribe(eventListener) }Sources/AblyChat/RoomReactions.swift (1)
28-30: Document thread-safety guaranteeSince
callbackis annotated@MainActor, please add a brief doc-comment line stating that the closure is always invoked on the main actor. This helps consumers rely on the guarantee.Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (1)
47-61: Consider extracting common subscription logic.There's significant duplication between the initialization logic in
MockSubscriptionHandleStorageandMockMessageSubscriptionHandleStorage. Consider extracting the common periodic emission pattern into a shared helper method to improve maintainability.+@MainActor +private func createPeriodicEmission<T>( + randomElement: @escaping @Sendable () -> T, + interval: Double, + callback: @escaping (T) -> Void, + onTerminate: @escaping () -> Void +) -> (needNextBinding: inout Bool, handle: () -> Void) { + var needNext = true + periodic(with: interval) { + if needNext { + callback(randomElement()) + } + return needNext + } + return (needNext, { + needNext = false + onTerminate() + }) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (26)
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/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/Helpers/Subscription+RoomStatusChange.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(1 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 as they are similar to previous changes (20)
- Sources/AblyChat/Subscription.swift
- Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
- Example/AblyChatExample/Misc/Utils.swift
- Tests/AblyChatTests/Mocks/MockRoom.swift
- Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
- Sources/AblyChat/DefaultConnection.swift
- Sources/AblyChat/SubscriptionHandleStorage.swift
- Sources/AblyChat/DefaultTyping.swift
- Sources/AblyChat/Occupancy.swift
- Sources/AblyChat/Typing.swift
- Sources/AblyChat/DefaultOccupancy.swift
- Sources/AblyChat/Presence.swift
- Sources/AblyChat/DefaultMessages.swift
- Example/AblyChatExample/ContentView.swift
- Sources/AblyChat/DefaultPresence.swift
- Sources/AblyChat/Discontinuity.swift
- Sources/AblyChat/Connection.swift
- Sources/AblyChat/Room.swift
- Example/AblyChatExample/Mocks/MockClients.swift
🧰 Additional context used
🧠 Learnings (2)
Sources/AblyChat/DefaultRoomReactions.swift (4)
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.
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/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/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.
Sources/AblyChat/Messages.swift (1)
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.
🧬 Code Graph Analysis (2)
Sources/AblyChat/DefaultRoomReactions.swift (5)
Sources/AblyChat/DefaultOccupancy.swift (1)
subscribe(10-13)Sources/AblyChat/DefaultPresence.swift (4)
subscribe(46-49)subscribe(51-54)subscribe(264-285)subscribe(287-311)Sources/AblyChat/RoomReactions.swift (2)
subscribe(42-56)subscribe(61-63)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (2)
subscribe(135-144)unsubscribe(152-154)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (2)
unsubscribe(256-258)unsubscribe(368-370)
Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (5)
Example/AblyChatExample/Misc/Utils.swift (1)
periodic(11-20)Sources/AblyChat/SubscriptionHandleStorage.swift (3)
create(16-24)subscriptionDidTerminate(32-34)emit(37-41)Sources/AblyChat/SubscriptionStorage.swift (3)
create(18-30)subscriptionDidTerminate(38-40)emit(43-47)Sources/AblyChat/Messages.swift (2)
emit(324-326)getPreviousMessages(333-335)Sources/AblyChat/Subscription.swift (1)
emit(78-85)
⏰ Context from checks skipped due to timeout of 90000ms (34)
- GitHub Check: Example app, tvOS (Xcode 16.1)
- GitHub Check: Example app, iOS (Xcode 16.2)
- GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Example app, tvOS (Xcode 16.2)
- GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.2) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.1) - GitHub Check: Example app, iOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.2) - GitHub Check: Example app, macOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.1) - GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - 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, iOS (Xcode 16.1)
- GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: Xcode, macOS (Xcode 16.1)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: SPM (Xcode 16.3)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.3) - GitHub Check: SPM (Xcode 16.2)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.2) - GitHub Check: SPM (Xcode 16.1)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.1) - GitHub Check: Generate code coverage
🔇 Additional comments (5)
Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (2)
40-88: Well-implemented subscription handle storage with proper memory management.The
MockSubscriptionHandleStorageclass is well structured with weak reference handling to prevent memory leaks. The class effectively handles periodic emission of random elements with proper lifecycle management through the subscription handle.
90-150: MessageSubscriptionHandleStorage effectively models the callback-based subscription pattern.The
MockMessageSubscriptionHandleStorageclass properly implements the callback-based subscription pattern with the added functionality of previous message retrieval, aligning well with theMessageSubscriptionHandleinterface in the main code.Sources/AblyChat/Messages.swift (3)
18-19: Improved API design with explicit subscription handling.The change to a callback-based API with an explicit subscription handle improves resource management and makes unsubscription more straightforward. This aligns with modern Swift API design patterns.
307-308: Improved error handling with specific throw type.The unified error type
ARTErrorInfofor thegetPreviousMessagesclosure simplifies error handling and improves API consistency.
354-361: Well-designed subscription handle with clear responsibilities.The
MessageSubscriptionHandlestruct clearly encapsulates the two key operations needed for message subscriptions: unsubscription and retrieving previous messages. The use of@MainActorforunsubscribeensures thread safety.However, make sure that any closures stored inside
unsubscribecaptureselfweakly to prevent retain cycles:// Example of proper weak capturing in unsubscribe closures public let unsubscribe: @MainActor () -> Void = { [weak self] in self?.channel.unsubscribe() }
maratal
force-pushed
the
259-switch-to-callbacks
branch
from
19ecedf to
20b3ebe
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Sources/AblyChat/Messages.swift (2)
77-107:⚠️ Potential issuePotential race condition in subscription implementation
There's a race condition where messages could be lost between subscribing (line 88) and assigning the
emitMessagecallback (line 96). Any messages received during this window will be dropped becauseemitMessageis stillnil.According to past review comments, this issue was intended to be fixed in commit eadad04, but the issue persists in the current implementation.
Consider this improved implementation:
func subscribe(bufferingPolicy: BufferingPolicy) async throws(ARTErrorInfo) -> MessageSubscription { let subscription = MessageSubscription( bufferingPolicy: bufferingPolicy, getPreviousMessages: { _ in // Create a temporary placeholder that will be updated after we have the real handle throw ARTErrorInfo(code: 0, message: "Subscription not initialized yet") } ) let subscriptionHandle = try await subscribe { [weak subscription] message in subscription?.emit(message) } // Use a more complex mechanism to update the getPreviousMessages function // or add a method to MessageSubscription to allow updating it after creation subscription.addTerminationHandler { Task { @MainActor in subscriptionHandle.unsubscribe() } } return subscription }Alternatively, consider adding a method to
MessageSubscriptionthat allows updating thegetPreviousMessagesfunction after initialization.
77-107: Potential race condition in subscription implementationThere's a race condition in the default extension implementation where messages could be lost. Any message received by the callback between lines 88-90 and the assignment to
emitMessageat line 96 will be dropped becauseemitMessageis stillnilduring that window.#!/bin/bash # Check if this issue has been addressed in other parts of the codebase rg -A 2 -B 2 "emitMessage.*nil" --type swift
🧹 Nitpick comments (2)
Sources/AblyChat/Messages.swift (1)
100-104: Use weak self in termination handlerThe closure doesn't capture
selfdirectly, but it should still use[weak subscriptionHandle]to avoid potential retain cycles, particularly when callingunsubscribe().subscription.addTerminationHandler { - Task { @MainActor in - subscriptionHandle.unsubscribe() - } + Task { [weak subscriptionHandle] @MainActor in + subscriptionHandle?.unsubscribe() + } }Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (1)
90-150: Good implementation of message-specific subscription storageThe
MockMessageSubscriptionHandleStorageextends the subscription pattern to include message-specific functionality like previous message retrieval. It properly follows the same memory management patterns as the base class.One minor improvement could be to further unify the code between these two classes since they share significant implementation details.
Consider refactoring these two classes to reduce duplication. You could use a base class or protocol with a generic type parameter to handle the common subscription management logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (26)
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/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/Helpers/Subscription+RoomStatusChange.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(1 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 as they are similar to previous changes (23)
- Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
- Tests/AblyChatTests/Mocks/MockRoom.swift
- Sources/AblyChat/Subscription.swift
- Example/AblyChatExample/Misc/Utils.swift
- Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift
- Sources/AblyChat/DefaultConnection.swift
- Sources/AblyChat/RoomReactions.swift
- Sources/AblyChat/DefaultRoomReactions.swift
- Sources/AblyChat/DefaultOccupancy.swift
- Sources/AblyChat/DefaultTyping.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
- Sources/AblyChat/Discontinuity.swift
- Sources/AblyChat/SubscriptionHandleStorage.swift
- Sources/AblyChat/Occupancy.swift
- Sources/AblyChat/Typing.swift
- Sources/AblyChat/DefaultMessages.swift
- Sources/AblyChat/RoomLifecycleManager.swift
- Sources/AblyChat/Connection.swift
- Sources/AblyChat/Presence.swift
- Example/AblyChatExample/ContentView.swift
- Sources/AblyChat/DefaultPresence.swift
- Example/AblyChatExample/Mocks/MockClients.swift
- Sources/AblyChat/Room.swift
🧰 Additional context used
🧠 Learnings (1)
Sources/AblyChat/Messages.swift (1)
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.
⏰ 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: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Example app, tvOS (Xcode 16.2)
- GitHub Check: Example app, iOS (Xcode 16.2)
- GitHub Check: Example app, iOS (Xcode 16.1)
- GitHub Check: Example app, tvOS (Xcode 16.1)
- GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - GitHub Check: Xcode, tvOS (Xcode 16.1)
- GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: Xcode, iOS (Xcode 16.1)
- GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Xcode, macOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.1) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.2) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.2) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.1) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.1) - GitHub Check: SPM,
releaseconfiguration (Xcode 16.3) - GitHub Check: SPM,
releaseconfiguration (Xcode 16.1) - GitHub Check: SPM,
releaseconfiguration (Xcode 16.2) - GitHub Check: SPM (Xcode 16.3)
- GitHub Check: SPM (Xcode 16.2)
- GitHub Check: SPM (Xcode 16.1)
- GitHub Check: Generate code coverage
🔇 Additional comments (11)
Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (3)
41-88: Great implementation of the MockSubscriptionHandleStorage class!The implementation correctly handles subscription lifecycle management with proper memory management:
- Uses
[weak self]in closures to avoid retain cycles- Properly removes terminated subscriptions from storage
- Uses the
@MainActorannotation consistently- Implements a clean termination mechanism via the
needNextflag
90-150: LGTM: MockMessageSubscriptionHandleStorage implementationThis implementation extends the pattern from MockSubscriptionHandleStorage to include message-specific functionality. It correctly handles the additional
previousMessagesparameter and returns the appropriateMessageSubscriptionHandletype.
40-88: Well-designed mock subscription handle storage implementationThe
MockSubscriptionHandleStorageclass effectively simulates subscription behavior with proper memory management. It uses weak references correctly in line 72 to avoid retain cycles, and appropriately handles unsubscription through theSubscriptionHandle.Sources/AblyChat/Messages.swift (8)
9-20: Good API transition from AsyncSequence to callback-based subscriptionThe protocol change properly documents the new callback-based approach and clarifies the return type being a subscription handle. This aligns with the PR objective to switch to callbacks.
96-98: Properly use weak capture to avoid retain cyclesGood job using
[weak subscription]in the callback closure to avoid potential retain cycles. This aligns with the learned preference in the codebase to avoid strong reference cycles.
354-361: Well-designed MessageSubscriptionHandle structThis new struct clearly encapsulates the unsubscription logic and previous message retrieval functionality. The public API is clean and consistent with the rest of the callback-based approach.
9-20: Protocol changes align with the new callback-based modelThe updated
Messagesprotocol now uses a callback-based approach for subscribing to messages, replacing the previous AsyncSequence-based pattern. The documentation clearly explains the expected behavior.
96-98: Correctly using weak capture to avoid retain cyclesThe code properly captures
subscriptionweakly in the callback closure to prevent retain cycles, which is essential for memory management with subscription patterns.
100-104: Good subscription lifecycle managementThe termination handler correctly uses
Taskwith@MainActorto ensure the unsubscription happens on the main thread, which is important for UI-related code.
307-307: Updated error type for consistencyThe method now throws
ARTErrorInfodirectly instead of an internal error type, which improves consistency throughout the API.
354-361: Well-designed subscription handle structThe new
MessageSubscriptionHandlestruct encapsulates both unsubscription logic and previous message retrieval in a clean, well-documented way. This approach provides a clear API for managing subscriptions.
maratal
force-pushed
the
259-switch-to-callbacks
branch
from
20b3ebe to
7115f7c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Sources/AblyChat/Messages.swift (1)
77-107:⚠️ Potential issueRace condition in backward compatibility implementation.
There's a window between subscribing to message events (line 88) and setting up the emitter (line 96) where incoming messages will be dropped because
emitMessageis still nil.Consider restructuring to initialize the subscription and setup message forwarding before subscribing:
func subscribe(bufferingPolicy: BufferingPolicy) async throws(ARTErrorInfo) -> MessageSubscription { let subscription = MessageSubscription( bufferingPolicy: bufferingPolicy, getPreviousMessages: { params in // Create a temporary placeholder that will be replaced once we have the real handle throw ARTErrorInfo(code: 0, message: "Subscription not initialized") } ) // Create the message handler closure that forwards to the subscription let messageHandler: (Message) -> Void = { [weak subscription] message in subscription?.emit(message) } // Subscribe using the handler let subscriptionHandle = try await subscribe(messageHandler) // Replace the getPreviousMessages implementation in the subscription // Note: This would require modifying MessageSubscription to allow updating getPreviousMessages subscription.addTerminationHandler { Task { @MainActor in subscriptionHandle.unsubscribe() } } return subscription }Note that this approach requires either:
- Modifying
MessageSubscriptionto allow updating thegetPreviousMessagesproperty after initialization, or- Creating a special initializer that can take a promise for the
getPreviousMessagesfunction that will be fulfilled later
🧹 Nitpick comments (1)
Sources/AblyChat/Room.swift (1)
341-344: Consider using weak self in the SubscriptionHandle closure.Based on the project's patterns, consider capturing
selfweakly in the closure to avoid potential retain cycles when the subscription handle is stored.internal func onDiscontinuity(_ callback: @escaping @MainActor (DiscontinuityEvent) -> Void) -> SubscriptionHandle { - lifecycleManager.onDiscontinuity(callback) + lifecycleManager.onDiscontinuity { [weak self] event in + guard self != nil else { return } + callback(event) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (26)
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/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/Helpers/Subscription+RoomStatusChange.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(1 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 as they are similar to previous changes (22)
- Tests/AblyChatTests/Mocks/MockRoom.swift
- Sources/AblyChat/Subscription.swift
- Sources/AblyChat/DefaultConnection.swift
- Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
- Example/AblyChatExample/Misc/Utils.swift
- Sources/AblyChat/SubscriptionHandleStorage.swift
- Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift
- Sources/AblyChat/RoomReactions.swift
- Sources/AblyChat/DefaultTyping.swift
- Sources/AblyChat/Occupancy.swift
- Sources/AblyChat/Discontinuity.swift
- Sources/AblyChat/Typing.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
- Sources/AblyChat/DefaultMessages.swift
- Sources/AblyChat/Connection.swift
- Sources/AblyChat/DefaultOccupancy.swift
- Sources/AblyChat/DefaultRoomReactions.swift
- Sources/AblyChat/Presence.swift
- Example/AblyChatExample/ContentView.swift
- Sources/AblyChat/DefaultPresence.swift
- Example/AblyChatExample/Mocks/MockClients.swift
- Sources/AblyChat/RoomLifecycleManager.swift
🧰 Additional context used
🧠 Learnings (1)
Sources/AblyChat/Messages.swift (1)
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.
🧬 Code Graph Analysis (1)
Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (6)
Example/AblyChatExample/Misc/Utils.swift (1)
periodic(11-20)Sources/AblyChat/SubscriptionHandleStorage.swift (3)
create(16-24)subscriptionDidTerminate(32-34)emit(37-41)Sources/AblyChat/SubscriptionStorage.swift (3)
create(18-30)subscriptionDidTerminate(38-40)emit(43-47)Sources/AblyChat/Messages.swift (2)
emit(324-326)getPreviousMessages(333-335)Sources/AblyChat/Subscription.swift (1)
emit(78-85)Example/AblyChatExample/Mocks/MockSubscription.swift (1)
emit(13-15)
⏰ Context from checks skipped due to timeout of 90000ms (34)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.1) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.2) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.2) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Example app, macOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.1) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.1) - GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Example app, tvOS (Xcode 16.2)
- GitHub Check: Example app, iOS (Xcode 16.1)
- GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Example app, tvOS (Xcode 16.1)
- GitHub Check: Example app, macOS (Xcode 16.1)
- GitHub Check: Example app, iOS (Xcode 16.2)
- GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Xcode, macOS (Xcode 16.1)
- GitHub Check: Xcode, tvOS (Xcode 16.1)
- GitHub Check: Xcode, iOS (Xcode 16.1)
- GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.3) - GitHub Check: SPM,
releaseconfiguration (Xcode 16.1) - GitHub Check: SPM (Xcode 16.3)
- GitHub Check: SPM (Xcode 16.1)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.2) - GitHub Check: SPM (Xcode 16.2)
- GitHub Check: Generate code coverage
🔇 Additional comments (12)
Sources/AblyChat/Room.swift (5)
7-7: Protocol conformance addition looks good.The addition of
ProvidesDiscontinuityconformance to theRoomprotocol is appropriate as part of the transition to callback-based event subscription.
73-74: Well-structured callback-based subscription API replacement.The new callback-based
onStatusChangemethod is a good replacement for the previous async sequence-based approach. The use of@discardableResultis appropriate since users may not always need to store the subscription handle.
112-136: Good backward compatibility implementation.This extension provides backward compatibility for existing code that relies on the async sequence-based API. The implementation correctly:
- Creates a new subscription with the specified buffering policy
- Subscribes via the new callback-based API
- Forwards events to the subscription
- Handles termination by unsubscribing
138-143: Appropriate unbounded buffering convenience method.This unbounded buffering implementation provides a convenient way to use the default buffering policy.
330-333: Delegate pattern used correctly.The implementation correctly delegates to the lifecycle manager's corresponding method, maintaining the separation of concerns.
Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (4)
40-88: Good implementation of MockSubscriptionHandleStorage.The mock implementation effectively simulates callback-based subscriptions by:
- Using the
periodicutility to emit random elements at specified intervals- Providing a subscription handle that can stop the emission and clean up
- Managing subscription storage with proper memory management via weak references
57-60: Correct subscription termination pattern.The subscription handle correctly stops emissions by setting
needNextto false and calling the termination handler, which ensures proper cleanup of resources.
90-150: Message subscription storage implementation follows consistent patterns.The
MockMessageSubscriptionHandleStorageclass correctly extends the basic subscription model to include message-specific functionality, particularly thegetPreviousMessagescapability.
113-117: Appropriate MessageSubscriptionHandle construction.The handle correctly encapsulates both unsubscription and message retrieval functionality, matching the production implementation.
Sources/AblyChat/Messages.swift (3)
19-19: Callback-based subscription API is well-designed.The transition from async sequence-based subscription to callback-based subscription is consistent with other parts of the codebase. The return type
MessageSubscriptionHandleclearly communicates its purpose.
307-307: Simplified error handling in MessageSubscription.The change from throwing
InternalErrorto explicitly throwingARTErrorInfosimplifies error handling and makes the API more consistent.Also applies to: 312-313
354-361: Well-designed MessageSubscriptionHandle structure.The
MessageSubscriptionHandlestruct clearly encapsulates the two key operations needed after subscription:
- Unsubscription via the
unsubscribeclosure- Retrieving previous messages via the
getPreviousMessagesclosureThis is a clean design that separates concerns and makes the API intuitive.
maratal
force-pushed
the
259-switch-to-callbacks
branch
from
7115f7c to
1eb7b32
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
Sources/AblyChat/RoomLifecycleManager.swift (5)
593-607:⚠️ Potential issuePotential memory leak in continuation handling.
The closure passed to
onRoomStatusChangecapturesselfweakly with[weak self], which could lead to the continuation never being resumed ifselfis deallocated during the execution ofwaitToBeAbleToPerformPresenceOperations. Since this method is called from within the manager itself,selfcannot disappear during its own execution, so capturingselfstrongly is safer and avoids the risk of a hanging continuation.Also,
nextRoomStatusSubscriptionis declared as implicitly unwrapped optional and later force-unwrapped on line 606, which could crash if the subscription initialization fails.-var nextRoomStatusSubscription: SubscriptionHandle! +var nextRoomStatusSubscription: SubscriptionHandle? // ... -nextRoomStatusSubscription.unsubscribe() +nextRoomStatusSubscription?.unsubscribe()and
- nextRoomStatusSubscription = self.onRoomStatusChange { [weak self] statusChange in + nextRoomStatusSubscription = self.onRoomStatusChange { statusChange in
606-607: Potential null pointer dereferenceForcefully unwrapping
nextRoomStatusSubscriptionwhen callingunsubscribe()could lead to a crash if the subscription handle is nil for any reason.
593-593: 🛠️ Refactor suggestionUse optional instead of implicitly unwrapped optional
The variable
nextRoomStatusSubscriptionis declared as an implicitly unwrapped optional (!), which could lead to a crash if it's not properly initialized.-var nextRoomStatusSubscription: SubscriptionHandle! +var nextRoomStatusSubscription: SubscriptionHandle?Then update the unsubscribe call on line 606:
-nextRoomStatusSubscription.unsubscribe() +nextRoomStatusSubscription?.unsubscribe()
593-607:⚠️ Potential issueFix weak self capture in continuation
The code still uses
[weak self]in theonRoomStatusChangecallback, which could lead to a potential deadlock ifselfis deallocated before a status change arrives. Based on previous review discussions, you agreed to remove the weak capture.Additionally,
nextRoomStatusSubscriptionis an implicitly unwrapped optional, which could crash ifonRoomStatusChangereturned nil.Apply these changes:
var nextRoomStatusSubscription: SubscriptionHandle! var nextRoomStatusChange: RoomStatusChange! await withCheckedContinuation { (continuation: CheckedContinuation<Void, _>) in self.logger.log(message: "waitToBeAbleToPerformPresenceOperations waiting for status change", level: .debug) #if DEBUG self.statusChangeWaitEventSubscriptions.emit(.init()) #endif - nextRoomStatusSubscription = self.onRoomStatusChange { [weak self] statusChange in + nextRoomStatusSubscription = self.onRoomStatusChange { statusChange in nextRoomStatusChange = statusChange - self?.logger.log(message: "waitToBeAbleToPerformPresenceOperations got status change \(String(describing: nextRoomStatusChange))", level: .debug) + self.logger.log(message: "waitToBeAbleToPerformPresenceOperations got status change \(String(describing: nextRoomStatusChange))", level: .debug) continuation.resume() } }
593-607:⚠️ Potential issuePotential memory leak in continuation handling
The continuation pattern for waiting on room status changes uses a weak reference to self in the callback closure, but could still lead to a memory leak if the subscription is never unsubscribed.
When capturing
selfweakly in the subscription callback, ifselfis deallocated while the continuation is active, the continuation might never be resumed. Even though the method unsubscribes after the continuation is resumed, if the manager is deallocated before a status change arrives, the unsubscription would never occur.Since this method can't be called after
selfis deallocated (it's a method onself), and based on the past review comments, the correct approach is to:
- Remove the weak reference to self in the callback closure
- Change the implicitly unwrapped optional to a regular optional
-var nextRoomStatusSubscription: SubscriptionHandle! +var nextRoomStatusSubscription: SubscriptionHandle? // ... - nextRoomStatusSubscription = self.onRoomStatusChange { [weak self] statusChange in - nextRoomStatusChange = statusChange - self?.logger.log(message: "waitToBeAbleToPerformPresenceOperations got status change \(String(describing: nextRoomStatusChange))", level: .debug) - continuation.resume() + nextRoomStatusSubscription = self.onRoomStatusChange { statusChange in + nextRoomStatusChange = statusChange + self.logger.log(message: "waitToBeAbleToPerformPresenceOperations got status change \(String(describing: nextRoomStatusChange))", level: .debug) + continuation.resume()And then use optional chaining for unsubscription:
- nextRoomStatusSubscription.unsubscribe() + nextRoomStatusSubscription?.unsubscribe()Sources/AblyChat/Messages.swift (1)
86-107: Potential race condition in subscription implementation.There's a race condition in the default extension implementation where messages could be lost. Any message received by the callback between lines 88-90 and the assignment to
emitMessageat line 96 will be dropped becauseemitMessageis stillnilduring that window.Example/AblyChatExample/Mocks/MockClients.swift (1)
101-103:⚠️ Potential issue
onDiscontinuitycrashes at runtime – must be implemented
fatalError("Not yet implemented")violates theRoomcontract and will crash the example app the first time a discontinuity subscription is created.@discardableResult func onDiscontinuity(_: @escaping @MainActor (DiscontinuityEvent) -> Void) -> SubscriptionHandle { - fatalError("Not yet implemented") + // Return a subscription handle that does nothing - no events will be emitted + return SubscriptionHandle {} }
🧹 Nitpick comments (3)
Sources/AblyChat/Room.swift (1)
130-132: Consider weak self in termination handlerBased on the retrieved learning, subscription handle closures should capture self weakly to avoid potential retain cycles. Consider adding a weak capture in the termination handler.
subscription.addTerminationHandler { - Task { @MainActor in + Task { [weak self] @MainActor in + guard let self else { return } subscriptionHandle.unsubscribe() } }Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (1)
47-61: Consider adding memory safety comments.The initialization stores a closure reference in
callbackwhile also capturing it in theperiodicblock. This creates a strong reference cycle that's only broken whenneedNext = falseis set during unsubscription.Consider adding a comment explaining this design choice, which is fine for this mock implementation but should be carefully considered in production code.
Example/AblyChatExample/Mocks/MockClients.swift (1)
450-455: Missing weak variable annotation.The
mockSubscriptionsinstance could potentially cause a retain cycle if the subscription holds a strong reference back to theMockConnectioninstance.Based on the learnings from other files in this PR, consider using
[weak self]in subscription callbacks to prevent any potential retain cycles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (26)
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/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/Helpers/Subscription+RoomStatusChange.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(1 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 as they are similar to previous changes (19)
- Sources/AblyChat/Subscription.swift
- Example/AblyChatExample/Misc/Utils.swift
- Tests/AblyChatTests/Mocks/MockRoom.swift
- Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
- Tests/AblyChatTests/Helpers/Subscription+RoomStatusChange.swift
- Sources/AblyChat/DefaultConnection.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
- Sources/AblyChat/RoomReactions.swift
- Sources/AblyChat/SubscriptionHandleStorage.swift
- Sources/AblyChat/DefaultOccupancy.swift
- Sources/AblyChat/DefaultRoomReactions.swift
- Sources/AblyChat/Occupancy.swift
- Sources/AblyChat/DefaultTyping.swift
- Sources/AblyChat/Typing.swift
- Sources/AblyChat/DefaultMessages.swift
- Example/AblyChatExample/ContentView.swift
- Sources/AblyChat/Discontinuity.swift
- Sources/AblyChat/DefaultPresence.swift
- Sources/AblyChat/Presence.swift
🧰 Additional context used
🧠 Learnings (3)
Sources/AblyChat/RoomLifecycleManager.swift (1)
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/Messages.swift (1)
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/Mocks/MockClients.swift (3)
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.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Example/AblyChatExample/Mocks/MockClients.swift:83-95
Timestamp: 2025-05-12T21:46:44.489Z
Learning: The `periodic` function in Example/AblyChatExample/Misc/Utils.swift is designed to execute its closure on the main actor (`@MainActor`), ensuring thread safety when used with shared mutable state that's only accessed from the main actor.
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 (2)
Sources/AblyChat/Connection.swift (4)
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)
Example/AblyChatExample/Mocks/MockClients.swift (11)
Sources/AblyChat/Connection.swift (2)
onStatusChange(46-60)onStatusChange(65-67)Sources/AblyChat/DefaultConnection.swift (1)
onStatusChange(20-89)Sources/AblyChat/Room.swift (4)
onStatusChange(122-136)onStatusChange(141-143)onStatusChange(330-333)onDiscontinuity(341-344)Tests/AblyChatTests/Mocks/MockRoom.swift (4)
onStatusChange(43-45)onStatusChange(71-74)onDiscontinuity(76-79)onDiscontinuity(81-83)Example/AblyChatExample/Misc/Utils.swift (1)
periodic(11-20)Sources/AblyChat/Discontinuity.swift (2)
onDiscontinuity(36-49)onDiscontinuity(54-56)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
onDiscontinuity(68-71)Sources/AblyChat/RoomLifecycleManager.swift (1)
onDiscontinuity(221-224)Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (3)
create(15-27)create(66-77)create(122-139)Sources/AblyChat/SubscriptionHandleStorage.swift (1)
create(16-24)Example/AblyChatExample/Mocks/MockRealtime.swift (4)
enter(264-266)enter(268-270)leave(280-282)leave(284-286)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Xcode, macOS (Xcode 16.1)
- GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Xcode, tvOS (Xcode 16.1)
- GitHub Check: Xcode, iOS (Xcode 16.1)
🔇 Additional comments (51)
Sources/AblyChat/RoomLifecycleManager.swift (17)
4-4: Good addition of ProvidesDiscontinuity protocol.The change to make RoomLifecycleManager conform to the ProvidesDiscontinuity protocol aligns with the overall refactoring to standardize the subscription mechanism across the codebase.
9-10: API change conforms to new callback pattern.The addition of @discardableResult and the change to a callback-based approach returning a SubscriptionHandle is consistent with the broader refactoring strategy.
104-105: Good replacement of subscription storage type.Changing from SubscriptionStorage to SubscriptionHandleStorage properly supports the new callback-based subscription model.
177-180: Implementation correctly uses the new subscription handle pattern.The implementation properly creates a subscription handle for status change callbacks.
221-224: Standardized discontinuity subscription API.The discontinuity subscription method follows the same pattern as other subscription methods, which is good for API consistency.
608-611: The roomStatus equality check may need to consider error state.When checking if the nextRoomStatusChange is attached, the code specifically checks for
.attached(error: nil). This is good as it ensures no errors are present in the attached state.
4-4: Protocol enhancement with ProvidesDiscontinuityGood addition of the
ProvidesDiscontinuityprotocol conformance to theRoomLifecycleManagerprotocol. This standardizes the discontinuity handling across the codebase and aligns with the callback-based subscription pattern.
9-10: Signature update for callback-based modelThe change from async sequence to callback-based API with
SubscriptionHandlereturn is consistent with the broader refactoring effort. The@discardableResultattribute is appropriate since not all callers will need to manage the subscription lifecycle.
104-105: Storage type update to match new subscription modelGood replacement of
SubscriptionStoragewithSubscriptionHandleStorageto support the callback-based subscription approach.
178-180: Callback implementation for status changesThe implementation correctly uses the new
SubscriptionHandleStorage.createmethod to register callbacks for room status changes, completing the transition to the callback-based model.
221-224: Discontinuity subscription implementationProperly implements the
ProvidesDiscontinuityprotocol method using the new callback registration approach.
4-4: Protocol now conforms to ProvidesDiscontinuityThe RoomLifecycleManager protocol now conforms to ProvidesDiscontinuity, following the codebase's shift to a standardized approach for handling discontinuity events.
9-10: API change to callback-based subscription modelThe signature of
onRoomStatusChangehas been updated to use a callback-based approach returning aSubscriptionHandleinstead of an async sequence. The@discardableResultattribute allows callers to ignore the returned handle when unsubscription isn't needed.
104-105: Storage change to support new subscription modelThe implementation now uses
SubscriptionHandleStorageinstead ofSubscriptionStorage, aligning with the new callback-based subscription model.
177-180: Implementation of callback-based room status change subscriptionThe implementation of
onRoomStatusChangenow returns aSubscriptionHandleand delegates to theroomStatusChangeSubscriptionsstorage to create the subscription.
221-224: Implementation of callback-based discontinuity subscriptionThe implementation of
onDiscontinuitynow returns aSubscriptionHandleand delegates to thediscontinuitySubscriptionsstorage to create the subscription, following the same pattern asonRoomStatusChange.
608-611: Status change check is correctly implementedThe code properly checks if the new status is attached, and throws an appropriate error if not, which aligns with the room lifecycle requirements.
Sources/AblyChat/Room.swift (18)
7-7: Good conformance to ProvidesDiscontinuity protocol.Adding the ProvidesDiscontinuity protocol to Room ensures consistent handling of discontinuity events across the SDK.
73-74: API change follows consistent pattern.The change to a callback-based approach with @discardableResult makes the Room API consistent with other components in the system.
112-136: Well-designed backward compatibility.This extension elegantly preserves backward compatibility with the AsyncSequence interface while internally using the new callback-based API. The implementation correctly:
- Creates a subscription with the specified buffering policy
- Sets up a callback that forwards events to the subscription
- Adds a termination handler to unsubscribe the callback when the subscription ends
The use of
[weak self]is avoided in the callback, which is good practice when the subscription might outlive the object it's capturing. The unsubscription in the termination handler ensures proper cleanup.
138-143: Good default implementation for unbounded buffering.This method provides a convenient way to create an unbounded subscription while reusing the existing implementation.
330-333: Implementation correctly delegates to lifecycle manager.The DefaultRoom implementation properly delegates the status change subscription to the lifecycle manager.
341-344: Discontinuity subscription follows the same pattern.The onDiscontinuity implementation also correctly delegates to the lifecycle manager, maintaining consistency.
7-7: Protocol conformance additionGood addition of the
ProvidesDiscontinuityprotocol conformance to theRoomprotocol, which standardizes discontinuity handling across the codebase.
73-74: API signature update for callback-based modelThe change from async sequence to callback-based API with
SubscriptionHandlereturn type aligns with the broader refactoring effort. The@discardableResultattribute is appropriate since not all callers will need to manage the subscription lifecycle.
112-136: Well-implemented backward compatibilityExcellent implementation of backward compatibility for the async sequence API. This extension creates a new
Subscription, subscribes using the new callback API, and properly manages the subscription lifecycle by adding a termination handler to unsubscribe when the async sequence is no longer needed.
138-143: Good default implementationProviding a default implementation for the unbounded buffering policy maintains API compatibility and reduces code duplication.
330-333: Clean delegation to lifecycle managerThe implementation correctly delegates to the lifecycle manager's corresponding method, maintaining a clean separation of concerns.
341-344: Proper discontinuity event delegationThe implementation correctly delegates discontinuity event handling to the lifecycle manager, completing the implementation of the
ProvidesDiscontinuityprotocol.
7-7: Protocol now conforms to ProvidesDiscontinuityThe Room protocol now conforms to ProvidesDiscontinuity, aligning with the codebase's standardized approach for handling discontinuity events.
73-74: API change to callback-based subscription modelThe signature of
onStatusChangehas been updated to use a callback-based approach returning aSubscriptionHandleinstead of an async sequence. The@discardableResultattribute allows callers to ignore the returned handle when unsubscription isn't needed.
112-136: Excellent backward compatibility implementationThe extension provides backward compatibility by implementing the async sequence interface on top of the new callback-based API. This maintains the existing API surface while leveraging the new implementation.
The subscription is properly set up to:
- Create an async sequence subscription with the desired buffering policy
- Forward events from the callback-based subscription to the async sequence
- Unsubscribe the callback when the subscription is terminated
This approach elegantly bridges the old and new APIs.
138-143: Default unbounded subscription preservedThe convenience method for creating an unbounded subscription is correctly preserved, maintaining backward compatibility for existing code.
330-333: Implementation delegates to lifecycle managerThe implementation of
onStatusChangecorrectly delegates to the lifecycle manager's corresponding method, maintaining the existing behavior while using the new API pattern.
341-344: Implementation delegates to lifecycle managerThe implementation of
onDiscontinuitycorrectly delegates to the lifecycle manager's corresponding method, maintaining the existing behavior while using the new API pattern.Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (2)
41-88: Well-structured subscription storage implementation.The
MockSubscriptionHandleStorageclass provides a clean implementation of the callback-based subscription pattern. It's properly marked with@MainActorto ensure thread safety and leverages theperiodicutility function for simulating events.
91-150: Good extension of the subscription pattern for messages.
MockMessageSubscriptionHandleStorageproperly extends the pattern to include support for fetching previous messages via theMessageSubscriptionHandle. The implementation follows the same pattern as the base class with appropriate additions.Sources/AblyChat/Connection.swift (2)
19-28: Clean API transition to callback-based subscriptions.The updated
onStatusChangemethod now accepts a callback and returns aSubscriptionHandleinstead of an async sequence. The documentation has been appropriately updated to reflect this change.
36-60: Excellent backward-compatibility implementation.This extension provides a clean backward-compatible implementation by wrapping the new callback-based API. It properly handles the subscription lifecycle:
- Creates a
Subscriptionwith the specified buffering policy- Subscribes using the new callback-based method and forwards events
- Sets up proper cleanup via termination handlers
This pattern allows for a smooth transition to the new API while maintaining compatibility with existing code.
Sources/AblyChat/Messages.swift (2)
15-19: API updated for callback-based subscription pattern.The
subscribemethod signature has been updated to accept a callback and return aMessageSubscriptionHandleinstead of an async sequence. The documentation has been appropriately updated.
354-361: Good subscription handle encapsulation.The new
MessageSubscriptionHandlestruct provides a clean encapsulation of unsubscription and previous message retrieval functionality. This aligns well with the callback-based subscription pattern adopted throughout the codebase.Example/AblyChatExample/Mocks/MockClients.swift (10)
70-74: Efficient random status generation with static closure.Defining
randomStatusChangeas a static closure improves performance by avoiding repeated creation of the random status generator. This is a good optimization for a method that will be called repeatedly.
83-98: Proper implementation of callback-based room status subscription.The implementation correctly uses
[weak self]to avoid retain cycles and properly manages the subscription lifecycle with theperiodicutility. The status change events are generated randomly at the specified interval.
110-111: Type-specific storage usage is appropriate.Using
MockMessageSubscriptionHandleStorage<Message>is the right choice here as it provides the additional functionality needed for message subscriptions, specifically the ability to fetch previous messages.
117-140: Clean implementation of message subscription with mocks.The subscription implementation correctly uses the message-specific storage class and provides realistic mock message generation. The interval of 3 seconds for periodic emissions is reasonable for testing purposes.
225-240: Good implementation of reaction subscription.The implementation follows the same pattern as other subscription methods, using
MockSubscriptionHandleStorageto manage the subscription lifecycle and emit random reactions.
255-270: Proper typing subscription implementation.The implementation correctly sets up a subscription for typing events with periodic emission of random typing events. The 2-second interval is appropriate for simulating typing behavior.
308-321: Reusable subscription creation for presence events.Good refactoring to extract the common subscription creation logic into a private helper method that can be reused by different presence subscription methods.
408-414: Presence subscriptions don't filter by event type.The current implementation doesn't filter events based on the event type parameters. I see from past review comments that this was intentionally simplified for mock implementations.
429-438: Appropriate occupancy subscription implementation.The code follows the established pattern for subscriptions, using random generation for occupancy events. The implementation includes a nice touch by ensuring the presence member count never exceeds the connection count.
457-469: Well-implemented connection status subscription.The implementation follows the same pattern as other subscription methods, generating random connection status changes and delivering them to the callback. The 5-second interval seems appropriate for testing connection state transitions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Sources/AblyChat/RoomLifecycleManager.swift (1)
611-625:⚠️ Potential issueFix potential deadlock in continuation handling.
There are two issues with the current implementation:
The
nextRoomStatusSubscriptionvariable is an implicitly unwrapped optional (IUO) which could crash if the subscription fails to initialize.The closure in
onRoomStatusChangecapturesselfweakly with[weak self]. Since this code is running within the manager's own method,selfcannot be deallocated during execution, so a weak capture could cause the continuation to never be resumed ifselfis released.Apply this fix:
-var nextRoomStatusSubscription: SubscriptionHandle! +var nextRoomStatusSubscription: SubscriptionHandle? -nextRoomStatusSubscription = self.onRoomStatusChange { [weak self] statusChange in +nextRoomStatusSubscription = self.onRoomStatusChange { statusChange in nextRoomStatusChange = statusChange - self?.logger.log(message: "waitToBeAbleToPerformPresenceOperations got status change \(String(describing: nextRoomStatusChange))", level: .debug) + self.logger.log(message: "waitToBeAbleToPerformPresenceOperations got status change \(String(describing: nextRoomStatusChange))", level: .debug) continuation.resume() } -nextRoomStatusSubscription.unsubscribe() +nextRoomStatusSubscription?.unsubscribe()
🧹 Nitpick comments (1)
Sources/AblyChat/RoomLifecycleManager.swift (1)
626-629: Handle potential nil status change.The code assumes
nextRoomStatusChangewill always be set before this line executes, but for safety, consider using optional binding to handle potential nil values.-if nextRoomStatusChange.current != .attached(error: nil) { +if let statusChange = nextRoomStatusChange, statusChange.current != .attached(error: nil) { // CHA-RL9c - throw ARTErrorInfo(chatError: .roomTransitionedToInvalidStateForPresenceOperation(cause: nextRoomStatusChange.current.error)).toInternalError() + throw ARTErrorInfo(chatError: .roomTransitionedToInvalidStateForPresenceOperation(cause: statusChange.current.error)).toInternalError() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Sources/AblyChat/Room.swift(4 hunks)Sources/AblyChat/RoomLifecycleManager.swift(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
Sources/AblyChat/RoomLifecycleManager.swift (1)
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.
🔇 Additional comments (14)
Sources/AblyChat/Room.swift (8)
66-74: Clean callback-based subscription API design.The refactoring from async sequence to callback-based subscription with explicit subscription handle looks good. This approach provides a clear and explicit contract for managing subscription lifecycles.
76-85: Consistent subscription pattern for discontinuity events.The new callback-based API for discontinuity event subscriptions follows the same pattern as status changes, maintaining consistency across the Room protocol's event subscription APIs.
115-120: Good API extension providing access to the underlying channel.Exposing the underlying channel via a non-isolated property allows for direct access to Ably's realtime functionality when needed, which improves API flexibility.
123-147: Well-implemented backward compatibility for async sequences.The extension implementation provides a clean backward compatibility layer that:
- Creates a Subscription with the specified buffering policy
- Sets up the callback subscription and forwards events
- Properly manages lifecycle with termination handlers
- Ensures unsubscribe calls happen on the MainActor
This approach maintains API compatibility while leveraging the new callback-based implementation.
149-154: Clean implementation of convenience method.The unbounded buffering convenience method is well-implemented, delegating to the main implementation with the appropriate buffering policy.
156-184: Good parallel implementation for discontinuity events.The async sequence subscription for discontinuity events mirrors the pattern used for status changes, maintaining consistency across the API.
371-374: Clean implementation of the status change callback method.The DefaultRoom implementation correctly delegates to the lifecycle manager's corresponding method, maintaining the separation of concerns.
382-385: Consistent discontinuity callback implementation.The implementation follows the same pattern as the status change subscription, maintaining consistency throughout the codebase.
Sources/AblyChat/RoomLifecycleManager.swift (6)
9-10: Clean callback-based subscription protocol design.The refactoring to accept a callback and return a SubscriptionHandle provides a clear contract for subscription management.
24-25: Consistent subscription pattern for discontinuity events.The callback-based API for discontinuity events follows the same pattern as status changes, maintaining API consistency.
107-108: Proper subscription storage type update.The change from SubscriptionStorage to SubscriptionHandleStorage aligns with the new callback-based subscription model.
180-183: Clean implementation of the status change callback method.The implementation correctly creates and returns a subscription handle using the storage.
224-227: Consistent discontinuity callback implementation.The implementation follows the same pattern as the status change subscription, maintaining consistency.
229-242: Good backward compatibility for discontinuity subscriptions.The method provides a clean bridge between the new callback API and the previous async sequence API by:
- Creating a Subscription with the specified buffering policy
- Setting up the callback subscription and forwarding events
- Properly managing lifecycle with termination handlers
- Ensuring unsubscribe calls happen on the MainActor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Sources/AblyChat/SubscriptionAsyncSequence.swift (3)
3-9: Consider improving the documentation structure.The extensive inline comments provide valuable context, but they could be better organized. Consider moving some of the implementation details and design decisions to a separate documentation file or structured comments.
-// A non-throwing `AsyncSequence` (means that we can iterate over it without a `try`). -// -// This should respect the `BufferingPolicy` passed to the `subscribe(bufferingPolicy:)` method. -// -// At some point we should define how this thing behaves when you iterate over it from multiple loops, or when you pass it around. I'm not yet sufficiently experienced with `AsyncSequence` to know what's idiomatic. I tried the same thing out with `AsyncStream` (two tasks iterating over a single stream) and it appears that each element is delivered to precisely one consumer. But we can leave that for later. On a similar note consider whether it makes a difference whether this is a struct or a class. -// -// I wanted to implement this as a protocol (from which `MessageSubscription` would then inherit) but struggled to do so (see https://forums.swift.org/t/struggling-to-create-a-protocol-that-inherits-from-asyncsequence-with-primary-associated-type/73950 where someone suggested it's a compiler bug), hence the struct. I was also hoping that upon switching to Swift 6 we could use AsyncSequence's `Failure` associated type to simplify the way in which we show that the subscription is non-throwing, but it turns out this can only be done in macOS 15 etc. So I think that for now we're stuck with things the way they are. +/** + * A non-throwing `AsyncSequence` implementation that bridges callback-based subscriptions + * with AsyncSequence interface. This should respect the `BufferingPolicy` passed to + * subscription methods. + * + * Implementation Notes: + * - Multiple iteration behavior is undefined (documented in public API) + * - Uses fatal errors for programmer errors in mock mode + * - Designed as a class due to AsyncSequence protocol limitations + * - TODO: Define multi-consumer behavior and evaluate struct vs class trade-offs + */
119-131: Optimize the iterator's next() method to avoid unnecessary copying.The current implementation uses pattern matching and reassignment which creates unnecessary copies of the iterator state. Consider using inout parameters or a more efficient approach.
- mutating func next() async -> Element? { - switch self { - case var .default(iterator: iterator): - let next = await iterator.next() - self = .default(iterator: iterator) - return next - case var .mockAsyncSequence(iterator: iterator): - let next = await iterator.next() - self = .mockAsyncSequence(iterator: iterator) - return next - } - } + mutating func next() async -> Element? { + switch self { + case .default(iterator: let iterator): + var mutableIterator = iterator + let next = await mutableIterator.next() + self = .default(iterator: mutableIterator) + return next + case .mockAsyncSequence(iterator: let iterator): + var mutableIterator = iterator + let next = await mutableIterator.next() + self = .mockAsyncSequence(iterator: mutableIterator) + return next + } + }
77-77: Consistency in fatal error handling across mock mode violations.The fatal error messages and handling are inconsistent across the three methods. Consider standardizing the approach and potentially providing a more informative error message.
- fatalError("`emit` cannot be called on a Subscription that was created using init(mockAsyncSequence:)") + fatalError("Cannot call emit(_:) on a SubscriptionAsyncSequence created with init(mockAsyncSequence:). This method is only available for default mode instances.") - fatalError("`finish` cannot be called on a Subscription that was created using init(mockAsyncSequence:)") + fatalError("Cannot call testsOnly_finish() on a SubscriptionAsyncSequence created with init(mockAsyncSequence:). This method is only available for default mode instances.") - fatalError("`addTerminationHandler(_:)` cannot be called on a Subscription that was created using init(mockAsyncSequence:)") + fatalError("Cannot call addTerminationHandler(_:) on a SubscriptionAsyncSequence created with init(mockAsyncSequence:). This method is only available for default mode instances.")Also applies to: 92-92, 110-110
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Sources/AblyChat/SubscriptionAsyncSequence.swift(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (34)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.1) - GitHub Check: Example app, iOS (Xcode 16.2)
- GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.2)
- GitHub Check: Example app, tvOS (Xcode 16.2)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Example app, tvOS (Xcode 16.1)
- GitHub Check: Example app, macOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.2) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.1) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.2) - GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.3) - GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.1) - GitHub Check: Xcode, tvOS (Xcode 16.1)
- GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: Xcode, iOS (Xcode 16.1)
- GitHub Check: Xcode, macOS (Xcode 16.1)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.1) - GitHub Check: SPM,
releaseconfiguration (Xcode 16.2) - GitHub Check: SPM (Xcode 16.2)
- GitHub Check: SPM (Xcode 16.3)
- GitHub Check: SPM (Xcode 16.1)
- GitHub Check: Generate code coverage
🔇 Additional comments (1)
Sources/AblyChat/SubscriptionAsyncSequence.swift (1)
14-14: Let’s inspect the class definition and its stored properties to confirm that all mutable state is either inherently thread-safe or actor-isolated:#!/bin/bash # Show the SubscriptionAsyncSequence class and its surrounding context rg -n -A20 "class SubscriptionAsyncSequence" Sources/AblyChat/SubscriptionAsyncSequence.swift # List all stored property declarations in the file rg -n "let " Sources/AblyChat/SubscriptionAsyncSequence.swift rg -n "var " Sources/AblyChat/SubscriptionAsyncSequence.swift
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
maratal
force-pushed
the
259-switch-to-callbacks
branch
from
bc68da8 to
ec95f2f
Compare
maratal
force-pushed
the
259-switch-to-callbacks
branch
from
ec95f2f to
bb5d088
Compare
maratal
force-pushed
the
259-switch-to-callbacks
branch
from
bb5d088 to
f3d52dc
Compare
…syncSequence`. - Update example app to use callbacks.
…event indefinitely.
maratal
force-pushed
the
259-switch-to-callbacks
branch
from
f3d52dc to
e694b85
Compare
Closes #259
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores