[ECO-5602] Make Room's status API consistent with Connection and JS
#408
Conversation
WalkthroughRefactors RoomStatus to remove associated error values and introduces separate error propagation via new error properties on Room, RoomLifecycleManager, and RoomStatusChange. Updates lifecycle control flow to pass optional errors during status transitions. Adjusts examples, mocks, helpers, and tests to use direct enum comparisons and the new error pathways. Removes obsolete helpers and comments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as App/UI
participant Room as Room
participant LM as RoomLifecycleManager
participant Ably as Ably Channel
App->>Room: attach()
Room->>LM: attach()
LM->>Ably: channel.attach()
Ably-->>LM: state event (ATTACHING/ATTACHED/FAILED) + reason?
LM->>LM: changeStatus(to: .attaching/.attached/.failed, error: reason)
LM-->>Room: emit RoomStatusChange{current, previous, error}
Room-->>App: onStatusChange callback
Note over Room,App: App reads Room.status and Room.error separately
sequenceDiagram
autonumber
actor App as App/UI
participant LM as RoomLifecycleManager
participant Ably as Ably Channel
App->>LM: detach()
LM->>Ably: channel.detach()
Ably-->>LM: state event (DETACHING/DETACHED/SUSPENDED) + reason?
LM->>LM: changeStatus(to: .detaching/.detached/.suspended, error: reason)
LM-->>App: RoomStatusChange{current, previous, error}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
temporarily deployed
to
staging/pull/408/AblyChat
lawrence-forooghian
force-pushed
the
406-align-RoomStatus-with-ConnectionStatus
branch
from
b6ed9df to
ec44ba2
Compare
lawrence-forooghian
changed the title
lawrence-forooghian
changed the title
Room's status API consistent with Connection and JS
github-actions
bot
temporarily deployed
to
staging/pull/408/AblyChat
This undoes the public API change introduced in d51887c, in which we try to make it clear from the type system whether a given room status is guaranteed to have an error, guaranteed to have no error, or maybe has an error. I still think that the API from d51887c is the "correct" one, but I suggested making this change in JS (see our #12) and nobody seemed particularly keen to do so (particularly because the core SDKs, whose errors we are largely just reflecting, don't expose such an API and don't make these guarantees, leading us to have to guess what the correct contract is). So, change the API to match JS and our Connection type; that is, remove the RoomStatus associated types and introduce `error` properties on Room and RoomStatusChange. We can revisit this one day once we have a core SDK API that makes these guarantees (e.g. in upcoming ably-swift). Resolves #406.
lawrence-forooghian
force-pushed
the
406-align-RoomStatus-with-ConnectionStatus
branch
2 times, most recently
from
ae4acbf to
7362b76
Compare
lawrence-forooghian
force-pushed
the
406-align-RoomStatus-with-ConnectionStatus
branch
from
7362b76 to
d1b0655
Compare
lawrence-forooghian
changed the title
Room's status API consistent with Connection and JSRoom's status API consistent with Connection and JS
github-actions
bot
temporarily deployed
to
staging/pull/408/AblyChat
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Example/AblyChatExample/ContentView.swift (1)
364-368: LGTM – direct enum comparisons correctly updated.Optional: now that
RoomStatusChange.erroris exposed, consider surfacing errors in the UI:room.onStatusChange { status in withAnimation { // existing statusInfo updates… + if let error = status.error { + statusInfo += " (Error: \(error.localizedDescription))" + } } }Sources/AblyChat/Room.swift (1)
82-85: Consider enhancing the documentation for the error property.The documentation for the
errorproperty is minimal. Consider expanding it to clarify:
- When this property is set (e.g., "Set when the room enters certain statuses such as failed, suspended, or after failed attach/detach operations")
- When it's nil (e.g., "Nil when the room is in a successful state or before any error has occurred")
- Its relationship to
RoomStatusChange.errorExample enhancement:
/** - * The current error, if any, that caused the room to enter the current status. + * The current error, if any, that caused the room to enter its current status. + * + * This property is set when the room transitions to an error state (such as + * ``RoomStatus/failed`` or ``RoomStatus/suspended``) or when an attach/detach + * operation fails. It is `nil` when the room is operating normally or has not + * yet encountered an error. + * + * - Returns: The error associated with the current room status, or `nil` if no error. */ var error: ARTErrorInfo? { get }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
Example/AblyChatExample/ContentView.swift(1 hunks)Example/AblyChatExample/Mocks/MockClients.swift(2 hunks)Sources/AblyChat/Connection.swift(0 hunks)Sources/AblyChat/Room.swift(3 hunks)Sources/AblyChat/RoomLifecycleManager.swift(10 hunks)Sources/AblyChat/RoomStatus.swift(1 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(38 hunks)Tests/AblyChatTests/DefaultRoomTests.swift(2 hunks)Tests/AblyChatTests/Helpers/Equatable.swift(1 hunks)Tests/AblyChatTests/Helpers/SubscriptionAsyncSequence+RoomStatus.swift(0 hunks)Tests/AblyChatTests/IntegrationTests.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoom.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(2 hunks)
💤 Files with no reviewable changes (2)
- Sources/AblyChat/Connection.swift
- Tests/AblyChatTests/Helpers/SubscriptionAsyncSequence+RoomStatus.swift
🧰 Additional context used
📓 Path-based instructions (3)
**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.swift: Expose SDK functionality via protocols; prefer associated types with opaque return types (some Protocol) over existentials (any Protocol)
Isolate all mutable state to the main actor; mark stateful objects with @mainactor
When using AsyncSequence operators in @mainactor contexts, mark operator closures as @sendable
Task, CheckedContinuation, and AsyncThrowingStream don’t support typed errors—use Result and call .get()
Dictionary.mapValues doesn’t support typed throws—use ablyChat_mapValuesWithTypedThrow extension
When the compiler struggles with typed throws, explicitly specify the error type with do throws(InternalError)
Specify the error type in closures, e.g., try items.map { jsonValue throws(InternalError) in … }
Files:
Example/AblyChatExample/ContentView.swiftTests/AblyChatTests/Mocks/MockRoom.swiftTests/AblyChatTests/IntegrationTests.swiftExample/AblyChatExample/Mocks/MockClients.swiftTests/AblyChatTests/Mocks/MockRoomLifecycleManager.swiftTests/AblyChatTests/DefaultRoomTests.swiftSources/AblyChat/Room.swiftSources/AblyChat/RoomStatus.swiftTests/AblyChatTests/DefaultRoomLifecycleManagerTests.swiftSources/AblyChat/RoomLifecycleManager.swiftTests/AblyChatTests/Helpers/Equatable.swift
Tests/**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
Tests/**/*.swift: Use test attribution tags in tests: @SPEC, @specOneOf(m/n), @specPartial, @specUntested - , @specNotApplicable -
For Swift Testing #expect(throws:) with typed errors, move typed-throw code into a separate non-typed-throw function
Files:
Tests/AblyChatTests/Mocks/MockRoom.swiftTests/AblyChatTests/IntegrationTests.swiftTests/AblyChatTests/Mocks/MockRoomLifecycleManager.swiftTests/AblyChatTests/DefaultRoomTests.swiftTests/AblyChatTests/DefaultRoomLifecycleManagerTests.swiftTests/AblyChatTests/Helpers/Equatable.swift
Sources/**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
Sources/**/*.swift: Public API must use typed throws with ARTErrorInfo; use InternalError internally and convert at public API boundaries
Reference Chat SDK features spec items in code comments when implementing behavior (e.g., // @SPEC CHA-RL3g)
For public structs exposed by the API, provide an explicit public memberwise initializer
Test-only APIs in the library must be prefixed with testsOnly_ and wrapped in #if DEBUG
Files:
Sources/AblyChat/Room.swiftSources/AblyChat/RoomStatus.swiftSources/AblyChat/RoomLifecycleManager.swift
🧬 Code graph analysis (2)
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (3)
Sources/AblyChat/RoomLifecycleManager.swift (1)
createManager(44-53)Tests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift (1)
createManager(11-14)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)
emitEvent(191-195)
Sources/AblyChat/RoomLifecycleManager.swift (1)
Sources/AblyChat/InternalError.swift (1)
toARTErrorInfo(19-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 26.0) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 26.0) - GitHub Check: Example app, macOS (Xcode 26.0)
- GitHub Check: Example app, tvOS (Xcode 26.0)
- GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: Xcode, macOS (Xcode 26.0)
- GitHub Check: Xcode, tvOS (Xcode 26.0)
- GitHub Check: Example app, macOS (Xcode 26.0)
- GitHub Check: Xcode, macOS (Xcode 26.0)
- GitHub Check: Example app, tvOS (Xcode 26.0)
- GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: Xcode, tvOS (Xcode 26.0)
- GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 26.0) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 26.0)
🔇 Additional comments (20)
Example/AblyChatExample/Mocks/MockClients.swift (3)
75-75: LGTM! Error property aligns with new Room protocol.The addition of the
errorproperty toMockRoomis consistent with the broader API changes whereRoomnow exposes error separately from status.
79-83: LGTM! Error derivation logic is correct.The change to compute
newStatusfirst and then deriveerrorbased on the status is a clean refactor that aligns with the new API where errors are separate from status enum cases.
100-105: LGTM! Status and error are correctly propagated.The update to set both
statusanderrorproperties before invoking the callback ensures consistency between the mock's state and the emitted status change event.Tests/AblyChatTests/Mocks/MockRoom.swift (1)
43-45: LGTM! Test mock implementation is appropriate.The
fatalErrorimplementation is acceptable for a test mock, as tests using this property should provide their own implementation or avoid accessing it. The approach is consistent with other mock methods in this class.Tests/AblyChatTests/IntegrationTests.swift (2)
99-101: LGTM! Status assertions updated correctly.The test assertions now correctly compare against plain enum cases (
.attached) rather than error-bearing variants (.attached(error: nil)), aligning with the updatedRoomStatusAPI.
493-495: LGTM! Detach status assertions updated correctly.Consistent with the attached status checks above, these assertions now compare against plain enum cases, matching the refactored
RoomStatusAPI.Tests/AblyChatTests/Helpers/Equatable.swift (1)
6-8: LGTM! Error identity comparison is correctly implemented.The addition of
lhs.error === rhs.errorto the equality check is correct. Using identity comparison (===) forARTErrorInfo(a reference type) is appropriate for test assertions.Sources/AblyChat/Room.swift (2)
224-237: LGTM! RoomStatusChange error property is well-integrated.The addition of the
errorproperty toRoomStatusChangewith an optional default value maintains backward compatibility and aligns with theConnectionStatusChangepattern mentioned in the PR objectives.
412-414: LGTM! Error delegation to lifecycle manager is correct.The
errorproperty correctly delegates tolifecycleManager.error, maintaining proper separation of concerns where the lifecycle manager owns the error state.Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
12-12: LGTM! Mock lifecycle manager correctly implements error property.The addition of the
_errorstorage, initializer parameter, and computederrorproperty correctly implements the updatedRoomLifecycleManagerprotocol requirements for testing purposes.Also applies to: 17-23, 60-62
Sources/AblyChat/RoomStatus.swift (1)
15-15: LGTM! Core API refactoring successfully removes error payloads from status enum.The removal of error associated values from all
RoomStatuscases is the central change of this PR. This simplification:
- Aligns
RoomStatuswithConnectionStatusand the JavaScript SDK- Enables error to be accessed via the separate
Room.errorproperty- Simplifies pattern matching and equality checks throughout the codebase
The change is consistently applied across all error-bearing cases (attaching, attached, detaching, detached, suspended, failed).
Also applies to: 20-20, 25-25, 30-30, 35-35, 40-40
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (5)
68-73: LGTM! Initial state test correctly verifies nil error.The addition of
#expect(manager.error == nil)ensures the initial state has no error, which is correct for the.initializedstatus.
185-188: LGTM! Attaching status checks updated correctly.The test now:
- Compares against plain
.attachingenum case (no error payload)- Separately verifies
manager.error == nilThis correctly validates the refactored API.
252-257: LGTM! Error propagation is correctly validated.The test verifies that the same error object is propagated to:
- The status change event (
failedStatusChange.error)- The manager's error property (
manager.error)- The thrown error (
roomAttachError)Using identity comparison (
===) forARTErrorInfoensures the same error instance is used throughout, which is appropriate for reference types.
387-389: LGTM! Detaching status assertions updated consistently.Similar to the attaching status checks, the test correctly:
- Compares against
.detachingwithout error payload- Separately verifies
error == nilMaintains consistency with the attach operation tests.
770-773: LGTM! Channel state change error propagation is correctly tested.The test now verifies that when a channel state change occurs with an error:
- The room status changes to match the channel state
- The
manager.erroris set to the same error object using identity comparisonThis ensures proper error propagation from channel events to room state.
Tests/AblyChatTests/DefaultRoomTests.swift (1)
258-269: Test updates look goodAssertions correctly track
room.statusand shared error identity to reflect the new API surface.Sources/AblyChat/RoomLifecycleManager.swift (3)
184-192: Status change propagation looks solidCapturing the latest error alongside status and emitting it through
RoomStatusChangekeeps lifecycle state and diagnostics aligned with the new API.
409-429: Attach flow handles error state correctlyShifting the error propagation into
changeStatusensures callers get the latest failure reason without relying on status-associated payloads.
582-612: Presence wait guard now correctly respects errorsValidating that the next status change both reaches
.attachedand carries no error prevents silently continuing after an errored attach.
lawrence-forooghian
deleted the
406-align-RoomStatus-with-ConnectionStatus
branch
This undoes the public API change introduced in d51887c, in which we try to make it clear from the type system whether a given room status is guaranteed to have an error, guaranteed to have no error, or maybe has an error.
I still think that the API from d51887c is the "correct" one, but I suggested making this change in JS (see our #12) and nobody seemed particularly keen to do so (particularly because the core SDKs, whose errors we are largely just reflecting, don't expose such an API and don't make these guarantees, leading us to have to guess what the correct contract is).
So, change the API to match JS and our
Connectiontype; that is, remove theRoomStatusassociated types and introduceerrorproperties onRoomandRoomStatusChange. We can revisit this one day once we have a core SDK API that makes these guarantees (e.g. in upcoming ably-swift).Resolves #406.
Summary by CodeRabbit
New Features
Refactor
Tests