Get rid of `ChatError` by lawrence-forooghian · Pull Request #431 · ably/ably-chat-swift

lawrence-forooghian · 2025-10-15T20:24:51Z

This is a refactor that combines the ChatError and InternalError types (having three different error types — ARTErrorInfo, InternalError, and ChatError — at play in the codebase was getting rather confusing!)

This is groundwork for some further error-related changes.

Summary by CodeRabbit

Refactor
- Unified internal error handling and standardized how errors are wrapped and converted across messaging, presence, rooms, channels, realtime, and reactions; public APIs and external behavior remain unchanged.
Tests
- Updated tests and helpers to expect the new internal error-wrapping so diagnostics and failure checks remain accurate.

coderabbitai · 2025-10-15T20:25:15Z

Walkthrough

Reworks internal error handling by routing Ably/ART errors through InternalError.fromAblyCocoa(ARTErrorInfo) and internal states through InternalError.internallyThrown(...); replaces many error.toInternalError() and direct ARTErrorInfo(...) constructions across adapters, room lifecycle, messages, mocks, and tests. Public signatures remain unchanged. (50 words)

Changes

Cohort / File(s)	Summary
Internal error model & conversions `Sources/AblyChat/Errors.swift`, `Sources/AblyChat/InternalError.swift`	Introduce unified internal error abstraction using `InternalError.fromAblyCocoa(ARTErrorInfo)` and `InternalError.internallyThrown(InternallyThrown)`, plus `toARTErrorInfo()`; consolidate internal cases under `InternallyThrown` and add helpers/conversions. Note: InternalError.swift content was removed/reworked per diff.
Ably Cocoa adapters & paginated results `Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift`, `Sources/AblyChat/PaginatedResult.swift`	Replace `error.toInternalError()` with `InternalError.fromAblyCocoa(error)` in catch blocks and resume/fail continuations with `.fromAblyCocoa(...)`.
Realtime / Channel / Presence adapters `Sources/AblyChat/...` (various adapter files referenced in diff)	Map thrown/returned errors from `error.toInternalError()` to `InternalError.fromAblyCocoa(error)` across realtime/channel/presence adapter paths (attach/detach/publish/presence operations).
Room lifecycle & rooms `Sources/AblyChat/RoomLifecycleManager.swift`, `Sources/AblyChat/Rooms.swift`	Replace direct `ARTErrorInfo(...)` / `.toInternalError()` with `InternalError.internallyThrown(...).toARTErrorInfo()` or throw `InternalError.internallyThrown(...)` in lifecycle, presence and room-option checks.
Messages & reactions `Sources/AblyChat/DefaultMessages.swift`, `Sources/AblyChat/DefaultMessageReactions.swift`, `Sources/AblyChat/Message.swift`	Route error creation through `InternalError.internallyThrown(...)` and convert to `ARTErrorInfo()` where previously constructed directly.
Mocks and realtime test helpers `Tests/AblyChatTests/Mocks/MockRealtime.swift`, `Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift`, `Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift`	Update catch blocks to throw or return `InternalError.fromAblyCocoa(error)` instead of `error.toInternalError()`.
Tests & test helpers `Tests/AblyChatTests/ChatAPITests.swift`, `Tests/AblyChatTests/DefaultMessageReactionsTests.swift`, `Tests/AblyChatTests/DefaultPresenceTests.swift`, `Tests/AblyChatTests/Helpers/Helpers.swift`, `Tests/AblyChatTests/InternalErrorTests.swift`	Update assertions and helpers to expect `.fromAblyCocoa` / `.internallyThrown(...).toARTErrorInfo()` wrapping and adjusted enum-case mappings; helper utilities adapted to new InternalError shape.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client
  participant SDK as AblyChat SDK
  participant Internal as InternalError
  participant ART as ARTErrorInfo

  Client->>SDK: perform operation (realtime/room/channel/message)
  SDK->>SDK: run checks / call Ably Cocoa
  alt Ably/ART error
    SDK->>Internal: InternalError.fromAblyCocoa(ARTErrorInfo) %%#{"color":"#F6C352"}
    Internal->>ART: .toARTErrorInfo() when reporting externally
    SDK-->>Client: propagate failure (ARTErrorInfo / InternalError)
  else Internal condition
    SDK->>Internal: InternalError.internallyThrown(case) %%#{"color":"#9FD3C7"}
    Internal->>ART: .toARTErrorInfo()
    SDK-->>Client: propagate failure (ARTErrorInfo / InternalError)
  else success
    SDK-->>Client: return success
  end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

[ECO-5602] Make Room's status API consistent with Connection and JS #408 — Overlaps on RoomLifecycleManager changes and lifecycle error handling; may conflict or need coordination.

Suggested reviewers

umair-ably
maratal

Poem

In burrows of code I hop and weave,
I wrap each error in tidy sleeves.
fromAblyCocoa hums, internallyThrown sings,
Tests nod, mocks catch—tidy tidy things.
A carrot of fixes for all my springs! 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 35.56% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The PR title "Get rid of `ChatError`" is directly aligned with the stated objective and the primary change in the changeset. The refactor consolidates the `ChatError` and `InternalError` types by merging `ChatError` into a reorganized `InternalError` structure with an `InternallyThrown` enum that contains cases previously belonging to `ChatError` (such as `.unableDeleteReactionWithoutName`, `.attachSerialIsNotDefined`, and others). The systematic replacement of `ChatError` cases with `InternalError.internallyThrown(...)` patterns across multiple files confirms that eliminating `ChatError` as a separate type is the primary change. The title is concise, specific, and accurately captures the main objective without unnecessary detail.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch 32-get-rid-of-ChatError

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (4)

Sources/AblyChat/DefaultMessages.swift (1)
182-183: Prefer logging the error’s message

Use InternalError.message for clearer logs instead of "(error)".

Apply:
-logger.log(message: "\(error)", level: .error)
+logger.log(message: error.message, level: .error)
As per coding guidelines

Also applies to: 186-188
Sources/AblyChat/Rooms.swift (1)
171-176: Good migration to InternalError.internallyThrown

Error cases and public boundary conversion preserved. One suggestion: mark DefaultRooms as @mainactor to enforce isolation of mutable state (roomStates, etc.) at type level, not just via protocol requirements.

Outside this hunk, change the class declaration to:
@MainActor
internal class DefaultRooms<RoomFactory: AblyChat.RoomFactory>: Rooms {
    ...
}
As per coding guidelines

Also applies to: 348-348
Sources/AblyChat/RoomLifecycleManager.swift (1)
213-216: Migration looks correct; consider @mainactor on the class

Correctly uses InternalError internally and ARTErrorInfo only for public-facing emissions. 👍

Recommend marking DefaultRoomLifecycleManager as @mainactor to ensure all state mutations (roomStatus, currentOperationID, continuations) are actor-isolated even for non-protocol helpers like didReceiveChannelStateEvent.

Outside this hunk, change the class declaration to:
@MainActor
internal class DefaultRoomLifecycleManager: RoomLifecycleManager {
    ...
}
As per coding guidelines

Also applies to: 396-399, 458-465, 612-620
Sources/AblyChat/Errors.swift (1)
171-177: Avoid hardcoding the status code userInfo key

Centralize "ARTErrorInfoStatusCode" to a single constant to prevent typos and ease future changes.

For example:
private let artErrorInfoStatusCodeKey = "ARTErrorInfoStatusCode"
...
userInfo[artErrorInfoStatusCodeKey] = internallyThrownError.codeAndStatusCode.statusCode

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 26cdd82 and 3dda7d8.

📒 Files selected for processing (17)

Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (9 hunks)
Sources/AblyChat/DefaultMessageReactions.swift (1 hunks)
Sources/AblyChat/DefaultMessages.swift (1 hunks)
Sources/AblyChat/Errors.swift (3 hunks)
Sources/AblyChat/InternalError.swift (0 hunks)
Sources/AblyChat/Message.swift (2 hunks)
Sources/AblyChat/PaginatedResult.swift (1 hunks)
Sources/AblyChat/RoomLifecycleManager.swift (4 hunks)
Sources/AblyChat/Rooms.swift (2 hunks)
Tests/AblyChatTests/ChatAPITests.swift (1 hunks)
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (2 hunks)
Tests/AblyChatTests/DefaultPresenceTests.swift (6 hunks)
Tests/AblyChatTests/Helpers/Helpers.swift (3 hunks)
Tests/AblyChatTests/InternalErrorTests.swift (1 hunks)
Tests/AblyChatTests/Mocks/MockRealtime.swift (1 hunks)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1 hunks)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (3 hunks)

💤 Files with no reviewable changes (1)

Sources/AblyChat/InternalError.swift

🧰 Additional context used

📓 Path-based instructions (3)

Tests/AblyChatTests/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Tests/AblyChatTests/**/*.swift: Place all tests under Tests/AblyChatTests
Use test attribution tags: @SPEC, @specOneOf(m/n), @specPartial, @specUntested, @specNotApplicable with appropriate spec IDs and explanations
For Swift Testing #expect(throws:) with typed errors, move typed-throw code into a separate non-typed-throw function (until Xcode 16.3+)

Files:

Tests/AblyChatTests/ChatAPITests.swift
Tests/AblyChatTests/DefaultMessageReactionsTests.swift
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
Tests/AblyChatTests/DefaultPresenceTests.swift
Tests/AblyChatTests/Helpers/Helpers.swift
Tests/AblyChatTests/Mocks/MockRealtime.swift
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
Tests/AblyChatTests/InternalErrorTests.swift

Sources/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Sources/**/*.swift: Prefer protocol-based design; expose SDK functionality via protocols and prefer opaque return types (some Protocol) over existential any Protocol
Isolate all mutable state to the main actor; mark stateful objects with @mainactor
Public API should use typed throws with ARTErrorInfo
Use InternalError internally and convert to ARTErrorInfo at public API boundaries
Test-only APIs in the library must be prefixed testsOnly_ and wrapped in #if DEBUG
Reference Chat SDK spec items in implementation comments (e.g., // @SPEC CHA-RL3g)
For public structs exposed by the API, provide an explicit public memberwise initializer
When using AsyncSequence operators in @mainactor contexts, mark operator closures as @sendable
For Task, CheckedContinuation, and AsyncThrowingStream (which don’t support typed errors), use Result and call .get() to bridge typed errors
Avoid Dictionary.mapValues for typed-throwing transformations; use ablyChat_mapValuesWithTypedThrow
When the compiler struggles with typed throws, explicitly annotate do throws(InternalError) blocks
Specify error types in closures, e.g., try items.map { jsonValue throws(InternalError) in … }

Files:

Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift
Sources/AblyChat/DefaultMessages.swift
Sources/AblyChat/PaginatedResult.swift
Sources/AblyChat/Rooms.swift
Sources/AblyChat/DefaultMessageReactions.swift
Sources/AblyChat/RoomLifecycleManager.swift
Sources/AblyChat/Message.swift
Sources/AblyChat/Errors.swift

Tests/AblyChatTests/Mocks/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Place mock implementations under Tests/AblyChatTests/Mocks

Files:

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
Tests/AblyChatTests/Mocks/MockRealtime.swift
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift

🧬 Code graph analysis (8)

Tests/AblyChatTests/DefaultMessageReactionsTests.swift (1)

Sources/AblyChat/Errors.swift (1)

toARTErrorInfo (164-185)

Sources/AblyChat/DefaultMessages.swift (1)

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)

failure (71-73)

Tests/AblyChatTests/DefaultPresenceTests.swift (1)

Sources/AblyChat/Errors.swift (1)

toARTErrorInfo (164-185)

Sources/AblyChat/DefaultMessageReactions.swift (1)

Sources/AblyChat/Errors.swift (1)

toARTErrorInfo (164-185)

Sources/AblyChat/RoomLifecycleManager.swift (1)

Sources/AblyChat/Errors.swift (1)

toARTErrorInfo (164-185)

Sources/AblyChat/Message.swift (1)

Sources/AblyChat/Errors.swift (1)

toARTErrorInfo (164-185)

Tests/AblyChatTests/InternalErrorTests.swift (1)

Sources/AblyChat/Errors.swift (1)

toARTErrorInfo (164-185)

Sources/AblyChat/Errors.swift (3)

Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (2)

attach (349-363)

detach (365-379)

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (2)

attach (94-102)

detach (108-116)

Sources/AblyChat/Room.swift (2)

attach (377-383)

detach (386-392)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)

GitHub Check: Example app, macOS (Xcode 26.0)
GitHub Check: Example app, tvOS (Xcode 26.0)
GitHub Check: Example app, iOS (Xcode 26.0)
GitHub Check: Xcode, release configuration, macOS (Xcode 26.0)
GitHub Check: SPM (Xcode 26.0)
GitHub Check: Xcode, release configuration, iOS (Xcode 26.0)
GitHub Check: Xcode, macOS (Xcode 26.0)
GitHub Check: Xcode, release configuration, tvOS (Xcode 26.0)
GitHub Check: Xcode, iOS (Xcode 26.0)
GitHub Check: Xcode, tvOS (Xcode 26.0)
GitHub Check: SPM, release configuration (Xcode 26.0)
GitHub Check: check-documentation

🔇 Additional comments (14)

Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (1)

167-167: LGTM! Consistent error wrapping pattern.

The refactoring consistently wraps ably-cocoa errors with InternalError.ablyCocoa(error) across all adapter methods. This aligns with the internal error handling strategy and maintains typed throws throughout the internal API.

Also applies to: 237-237, 255-255, 271-271, 287-287, 303-303, 361-361, 377-377, 393-393

Tests/AblyChatTests/Mocks/MockRealtime.swift (1)

47-47: LGTM! Mock behavior aligned with production code.

The mock correctly wraps the error with InternalError.ablyCocoa(error), maintaining consistency with the production adapter implementations.

Sources/AblyChat/PaginatedResult.swift (1)

38-38: LGTM! Consistent error wrapping.

The change correctly wraps the ably-cocoa error with InternalError.ablyCocoa(error), aligning with the internal error handling refactor.

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)

85-85: LGTM! Consistent mock implementation.

The mock correctly throws InternalError.ablyCocoa(error), matching the behavior of production adapters.

Sources/AblyChat/DefaultMessageReactions.swift (1)

44-44: LGTM! Correct boundary conversion.

The change properly converts the internal error to ARTErrorInfo at the public API boundary using toARTErrorInfo(), following the coding guidelines.

Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)

33-33: LGTM! Consistent mock implementation.

All catch blocks correctly wrap errors with InternalError.ablyCocoa(error), maintaining consistency with the production code pattern.

Also applies to: 45-45, 79-79

Tests/AblyChatTests/DefaultMessageReactionsTests.swift (1)

78-78: LGTM! Test expectations correctly updated.

The test expectations now verify that the thrown ARTErrorInfo matches the expected error generated via InternalError.internallyThrown(...).toARTErrorInfo(), correctly testing the public API boundary conversion.

Also applies to: 98-98

Tests/AblyChatTests/ChatAPITests.swift (1)

24-29: Matching on InternalError is correct: sendMessage is internal and throws InternalError.

Likely an incorrect or invalid review comment.

Sources/AblyChat/Message.swift (1)

227-227: Good switch to internal error + conversion

Public API still throws ARTErrorInfo; internal source uses InternalError.internallyThrown with correct badRequest mapping. LGTM.

Also applies to: 248-248, 253-253

Tests/AblyChatTests/DefaultPresenceTests.swift (1)

87-88: Tests align with new InternalError → ARTErrorInfo flow

Updated construction/expectations look correct and consistent with toARTErrorInfo() behavior. 👍

Also applies to: 118-119, 194-195, 225-226, 330-331, 379-380

Tests/AblyChatTests/InternalErrorTests.swift (1)

9-9: LGTM

Covers both ablyCocoa passthrough and internallyThrown → ErrorInfo mapping to badRequest.

Also applies to: 17-17

Tests/AblyChatTests/Helpers/Helpers.swift (1)

13-15: Helpers updated correctly for InternalError.ablyCocoa and new cases

Pattern matches and enumCase mapping look consistent with the refactor. 👍

Also applies to: 36-41, 46-48, 52-56

Sources/AblyChat/Errors.swift (2)

163-184: toARTErrorInfo implementation looks solid

Correctly sets domain, code, status (via userInfo), description, and underlying cause. Matches the new error model. 👍

222-259: Mapping of codes/status to spec looks correct

presenceOperationRequiresRoomAttach → 400

roomTransitionedToInvalidStateForPresenceOperation → 500

discontinuity → 500

moderation/beforePublish rejections → 422

others → 400

LGTM.

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (5)

Sources/AblyChat/Errors.swift (2)

162-218: Internal mapping reads well; add lightweight tests for code/status pairs

The code/status mapping and spec notes are clear. Consider unit tests exercising each case in codeAndStatusCode to prevent regressions when adding new cases.

Also applies to: 220-258

350-378: Uniform conversion helpers: consider naming/placement tweak

The toInternalError() helpers are fine. Optionally rename to asInternalError() for symmetry with other “asX” conversions and group them under a single extension file to reduce scattering.

Tests/AblyChatTests/DefaultPresenceTests.swift (1)

87-88: Deduplicate error construction in tests

Extract a small helper (e.g., makePresenceInvalidStateError(status: cause:)) to build these ARTErrorInfo values and reduce repetition.

Also applies to: 118-119, 194-195, 225-226, 330-331, 379-380

Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (1)

167-168: Prefer typed catch for clarity; optional helper to DRY

Use catch let error as ARTErrorInfo { throw InternalError.ablyCocoa(error) } for explicitness. Optionally extract a small helper to wrap Result<T, ARTErrorInfo>.get() into throws(InternalError) to avoid repeating the catch/wrap pattern.

Also applies to: 237-238, 255-256, 271-272, 287-288, 303-304, 361-362, 377-378, 393-394

Sources/AblyChat/RoomLifecycleManager.swift (1)

19-22: Update docs: replace ChatError references

Docs still mention ChatError-derived ARTErrorInfo. Update to reflect InternalError.internallyThrown(...).toARTErrorInfo() or reference ErrorCode where appropriate.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 7635627 and 4751614.

📒 Files selected for processing (17)

Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (9 hunks)
Sources/AblyChat/DefaultMessageReactions.swift (1 hunks)
Sources/AblyChat/DefaultMessages.swift (1 hunks)
Sources/AblyChat/Errors.swift (2 hunks)
Sources/AblyChat/InternalError.swift (0 hunks)
Sources/AblyChat/Message.swift (2 hunks)
Sources/AblyChat/PaginatedResult.swift (1 hunks)
Sources/AblyChat/RoomLifecycleManager.swift (4 hunks)
Sources/AblyChat/Rooms.swift (2 hunks)
Tests/AblyChatTests/ChatAPITests.swift (1 hunks)
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (2 hunks)
Tests/AblyChatTests/DefaultPresenceTests.swift (6 hunks)
Tests/AblyChatTests/Helpers/Helpers.swift (2 hunks)
Tests/AblyChatTests/InternalErrorTests.swift (1 hunks)
Tests/AblyChatTests/Mocks/MockRealtime.swift (1 hunks)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1 hunks)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (3 hunks)

💤 Files with no reviewable changes (1)

Sources/AblyChat/InternalError.swift

🚧 Files skipped from review as they are similar to previous changes (8)

Sources/AblyChat/Message.swift
Sources/AblyChat/DefaultMessageReactions.swift
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
Tests/AblyChatTests/DefaultMessageReactionsTests.swift
Tests/AblyChatTests/ChatAPITests.swift
Sources/AblyChat/DefaultMessages.swift
Sources/AblyChat/PaginatedResult.swift
Tests/AblyChatTests/Mocks/MockRealtime.swift

🧰 Additional context used

📓 Path-based instructions (3)

Tests/AblyChatTests/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Tests/AblyChatTests/**/*.swift: Place all tests under Tests/AblyChatTests
Use test attribution tags: @SPEC, @specOneOf(m/n), @specPartial, @specUntested, @specNotApplicable with appropriate spec IDs and explanations
For Swift Testing #expect(throws:) with typed errors, move typed-throw code into a separate non-typed-throw function (until Xcode 16.3+)

Files:

Tests/AblyChatTests/InternalErrorTests.swift
Tests/AblyChatTests/Helpers/Helpers.swift
Tests/AblyChatTests/DefaultPresenceTests.swift
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift

Sources/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Sources/**/*.swift: Prefer protocol-based design; expose SDK functionality via protocols and prefer opaque return types (some Protocol) over existential any Protocol
Isolate all mutable state to the main actor; mark stateful objects with @mainactor
Public API should use typed throws with ARTErrorInfo
Use InternalError internally and convert to ARTErrorInfo at public API boundaries
Test-only APIs in the library must be prefixed testsOnly_ and wrapped in #if DEBUG
Reference Chat SDK spec items in implementation comments (e.g., // @SPEC CHA-RL3g)
For public structs exposed by the API, provide an explicit public memberwise initializer
When using AsyncSequence operators in @mainactor contexts, mark operator closures as @sendable
For Task, CheckedContinuation, and AsyncThrowingStream (which don’t support typed errors), use Result and call .get() to bridge typed errors
Avoid Dictionary.mapValues for typed-throwing transformations; use ablyChat_mapValuesWithTypedThrow
When the compiler struggles with typed throws, explicitly annotate do throws(InternalError) blocks
Specify error types in closures, e.g., try items.map { jsonValue throws(InternalError) in … }

Files:

Sources/AblyChat/RoomLifecycleManager.swift
Sources/AblyChat/Errors.swift
Sources/AblyChat/Rooms.swift
Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift

Tests/AblyChatTests/Mocks/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Place mock implementations under Tests/AblyChatTests/Mocks

Files:

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift

🧬 Code graph analysis (4)

Tests/AblyChatTests/InternalErrorTests.swift (1)

Sources/AblyChat/Errors.swift (1)

toARTErrorInfo (134-155)

Sources/AblyChat/RoomLifecycleManager.swift (1)

Sources/AblyChat/Errors.swift (1)

toARTErrorInfo (134-155)

Sources/AblyChat/Errors.swift (2)

Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (2)

attach (349-363)

detach (365-379)

Sources/AblyChat/Room.swift (2)

attach (377-383)

detach (386-392)

Tests/AblyChatTests/DefaultPresenceTests.swift (1)

Sources/AblyChat/Errors.swift (1)

toARTErrorInfo (134-155)

⏰ 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). (11)

GitHub Check: Example app, macOS (Xcode 26.0)
GitHub Check: Example app, iOS (Xcode 26.0)
GitHub Check: Xcode, release configuration, macOS (Xcode 26.0)
GitHub Check: Example app, tvOS (Xcode 26.0)
GitHub Check: Xcode, release configuration, tvOS (Xcode 26.0)
GitHub Check: Xcode, release configuration, iOS (Xcode 26.0)
GitHub Check: Xcode, macOS (Xcode 26.0)
GitHub Check: Xcode, tvOS (Xcode 26.0)
GitHub Check: Xcode, iOS (Xcode 26.0)
GitHub Check: SPM, release configuration (Xcode 26.0)
GitHub Check: SPM (Xcode 26.0)

🔇 Additional comments (16)

Sources/AblyChat/Errors.swift (3)

157-161: message passthrough looks good

Simple and useful for logs; no change requested.

281-318: Descriptions are user‑friendly and specific

Good UX text; no change requested.

320-344: Cause propagation is sensible

Optional causes are surfaced only when meaningful. LGTM.

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)

85-85: Aligns with InternalError model

Wrapping ARTErrorInfo via .ablyCocoa is correct for typed throws.

Tests/AblyChatTests/InternalErrorTests.swift (2)

9-9: LGTM: ablyCocoa passthrough verified

Keeps identity of ARTErrorInfo instance as expected.

17-17: LGTM: non‑ARTErrorInfo conversion validated

Asserts code mapping and message content. Solid coverage.

Sources/AblyChat/RoomLifecycleManager.swift (4)

213-214: Discontinuity emission aligned with new model

Wrapping cause and producing ARTErrorInfo via InternalError is correct.

395-399: Correct error surfaces for ATTACH preconditions

Throwing InternalError.internallyThrown(.roomIsReleasing/.roomIsReleased) matches spec and typed throws.

458-465: Correct error surfaces for DETACH preconditions

Matches spec mapping; keep as is.

612-613: Presence wait: correct invalid-state errors

Good use of internallyThrown variants to signal invalid states with appropriate status codes.

Also applies to: 619-620

Sources/AblyChat/Rooms.swift (2)

171-176: LGTM!

The error construction correctly uses InternalError.internallyThrown within the typed-throw block and will be converted to ARTErrorInfo at the public API boundary (line 272), aligning with the coding guidelines.

348-348: LGTM!

The error construction correctly passes InternalError to failCreation, which flows through the Result<..., InternalError> type and is properly bridged via .get() (line 265) before conversion to ARTErrorInfo at the public API boundary (line 272).

Tests/AblyChatTests/Helpers/Helpers.swift (4)

5-39: LGTM!

The dual handling of both ARTErrorInfo and InternalError.internallyThrown is appropriate for a test helper that needs to verify errors at different layers of abstraction. The logic correctly checks for equality of codes, status codes, causes, and optional messages in both branches.

41-47: LGTM!

The change from .errorInfo to .ablyCocoa correctly reflects the new internal error handling model introduced by this PR.

49-64: LGTM!

The simplification to jsonValueDecodingError and other cases appropriately reduces complexity while maintaining the necessary distinctions for test verification.

66-72: LGTM!

The new helper function provides a clean way to verify error cases in tests. Implementation is straightforward and correct.

coderabbitai

Actionable comments posted: 0

♻️ Duplicate comments (1)

Sources/AblyChat/Errors.swift (1)

140-141: Refactor: Extract hardcoded userInfo key to a constant.

The string literal "ARTErrorInfoStatusCode" should be extracted to a private static constant to avoid typos and improve maintainability, as noted in the past review comment.

Apply this diff to extract the constant:

+    private static let artErrorInfoStatusCodeKey = "ARTErrorInfoStatusCode"
+
     /// Returns the error that this should be converted to when exposed via the SDK's public API.
     internal func toARTErrorInfo() -> ARTErrorInfo {
         switch self {
         case let .ablyCocoa(errorInfo):
             return errorInfo
         case let .internallyThrown(internallyThrownError):
             var userInfo: [String: Any] = [:]
             // TODO: copied and pasted from implementation of -[ARTErrorInfo createWithCode:status:message:requestId:] because there's no way to pass domain; revisit in https://github.com/ably-labs/ably-chat-swift/issues/32. Also the ARTErrorInfoStatusCode variable in ably-cocoa is not public.
-            userInfo["ARTErrorInfoStatusCode"] = internallyThrownError.codeAndStatusCode.statusCode
+            userInfo[Self.artErrorInfoStatusCodeKey] = internallyThrownError.codeAndStatusCode.statusCode
             userInfo[NSLocalizedDescriptionKey] = internallyThrownError.localizedDescription

🧹 Nitpick comments (5)

Sources/AblyChat/DefaultMessages.swift (1)

3-3: Consider marking DefaultMessages as @mainactor

This type holds mutable state (currentSubscriptionPoint, subscriptionPoints) and interacts with @MainActor-only protocols. Marking the class @mainactor would align with “isolate mutable state to the main actor.”

As per coding guidelines

Tests/AblyChatTests/DefaultMessageReactionsTests.swift (1)

78-79: Assertions updated to new InternalError → ARTErrorInfo mapping

Looks correct and aligned with the refactor.

To make tests less brittle to userInfo/message changes, consider asserting domain and code (and optionally cause) rather than full equality. Example:
#expect(thrownError?.domain == expected.domain && thrownError?.code == expected.code)

Also applies to: 98-99

Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (1)

167-169: Consistent wrapping of ARTErrorInfo via .ablyCocoa after Result.get()

The mapping is uniform and matches the new error model. LGTM.

Consider extracting a small helper to reduce duplication, e.g., awaitFromCallback { (resume: (Result<T, ARTErrorInfo>) -> Void) in ... } that returns T or throws InternalError.ablyCocoa.

Optionally replace _ in CheckedContinuation<..., _> with Never for clarity and tool support.

Also applies to: 237-239, 255-257, 271-273, 287-289, 303-305, 361-363, 377-379, 393-395
Tests/AblyChatTests/Mocks/MockRealtime.swift (1)
47-48: Use typed catch for ARTErrorInfo when wrapping with .ablyCocoa

Prefer catching ARTErrorInfo to ensure only Ably Cocoa errors are wrapped:
-        } catch {
-            throw InternalError.ablyCocoa(error)
+        } catch let error as ARTErrorInfo {
+            throw InternalError.ablyCocoa(error)
+        } catch {
+            // Unexpected non-ARTErrorInfo; keep context
+            throw InternalError.internallyThrown(.other(.unexpected(cause: error)))
         }
Sources/AblyChat/Errors.swift (1)
140-141: Update misleading TODO comment.

The TODO comment states "there's no way to pass domain" but the code on line 150 explicitly passes the domain parameter to ARTErrorInfo. This comment appears to be outdated and should be revised or removed.

Consider updating the TODO to:
-            // TODO: copied and pasted from implementation of -[ARTErrorInfo createWithCode:status:message:requestId:] because there's no way to pass domain; revisit in https://github.com/ably-labs/ably-chat-swift/issues/32. Also the ARTErrorInfoStatusCode variable in ably-cocoa is not public.
+            // TODO: copied and pasted from implementation of -[ARTErrorInfo createWithCode:status:message:requestId:]; revisit in https://github.com/ably-labs/ably-chat-swift/issues/32. Also the ARTErrorInfoStatusCode variable in ably-cocoa is not public.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 4751614 and ac30055.

📒 Files selected for processing (17)

Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (9 hunks)
Sources/AblyChat/DefaultMessageReactions.swift (1 hunks)
Sources/AblyChat/DefaultMessages.swift (1 hunks)
Sources/AblyChat/Errors.swift (2 hunks)
Sources/AblyChat/InternalError.swift (0 hunks)
Sources/AblyChat/Message.swift (2 hunks)
Sources/AblyChat/PaginatedResult.swift (1 hunks)
Sources/AblyChat/RoomLifecycleManager.swift (5 hunks)
Sources/AblyChat/Rooms.swift (2 hunks)
Tests/AblyChatTests/ChatAPITests.swift (1 hunks)
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (2 hunks)
Tests/AblyChatTests/DefaultPresenceTests.swift (6 hunks)
Tests/AblyChatTests/Helpers/Helpers.swift (2 hunks)
Tests/AblyChatTests/InternalErrorTests.swift (1 hunks)
Tests/AblyChatTests/Mocks/MockRealtime.swift (1 hunks)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1 hunks)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (3 hunks)

💤 Files with no reviewable changes (1)

Sources/AblyChat/InternalError.swift

🚧 Files skipped from review as they are similar to previous changes (8)

Sources/AblyChat/PaginatedResult.swift
Tests/AblyChatTests/DefaultPresenceTests.swift
Sources/AblyChat/DefaultMessageReactions.swift
Tests/AblyChatTests/InternalErrorTests.swift
Sources/AblyChat/Message.swift
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
Tests/AblyChatTests/ChatAPITests.swift
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift

🧰 Additional context used

📓 Path-based instructions (3)

Tests/AblyChatTests/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Tests/AblyChatTests/**/*.swift: Place all tests under Tests/AblyChatTests
Use test attribution tags: @SPEC, @specOneOf(m/n), @specPartial, @specUntested, @specNotApplicable with appropriate spec IDs and explanations
For Swift Testing #expect(throws:) with typed errors, move typed-throw code into a separate non-typed-throw function (until Xcode 16.3+)

Files:

Tests/AblyChatTests/Helpers/Helpers.swift
Tests/AblyChatTests/DefaultMessageReactionsTests.swift
Tests/AblyChatTests/Mocks/MockRealtime.swift

Sources/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Sources/**/*.swift: Prefer protocol-based design; expose SDK functionality via protocols and prefer opaque return types (some Protocol) over existential any Protocol
Isolate all mutable state to the main actor; mark stateful objects with @mainactor
Public API should use typed throws with ARTErrorInfo
Use InternalError internally and convert to ARTErrorInfo at public API boundaries
Test-only APIs in the library must be prefixed testsOnly_ and wrapped in #if DEBUG
Reference Chat SDK spec items in implementation comments (e.g., // @SPEC CHA-RL3g)
For public structs exposed by the API, provide an explicit public memberwise initializer
When using AsyncSequence operators in @mainactor contexts, mark operator closures as @sendable
For Task, CheckedContinuation, and AsyncThrowingStream (which don’t support typed errors), use Result and call .get() to bridge typed errors
Avoid Dictionary.mapValues for typed-throwing transformations; use ablyChat_mapValuesWithTypedThrow
When the compiler struggles with typed throws, explicitly annotate do throws(InternalError) blocks
Specify error types in closures, e.g., try items.map { jsonValue throws(InternalError) in … }

Files:

Sources/AblyChat/DefaultMessages.swift
Sources/AblyChat/RoomLifecycleManager.swift
Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift
Sources/AblyChat/Errors.swift
Sources/AblyChat/Rooms.swift

Tests/AblyChatTests/Mocks/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Place mock implementations under Tests/AblyChatTests/Mocks

Files:

Tests/AblyChatTests/Mocks/MockRealtime.swift

🧬 Code graph analysis (4)

Tests/AblyChatTests/DefaultMessageReactionsTests.swift (1)

Sources/AblyChat/Errors.swift (1)

toARTErrorInfo (134-155)

Sources/AblyChat/DefaultMessages.swift (1)

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)

failure (71-73)

Sources/AblyChat/RoomLifecycleManager.swift (1)

Sources/AblyChat/Errors.swift (1)

toARTErrorInfo (134-155)

Sources/AblyChat/Errors.swift (3)

Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (2)

attach (349-363)

detach (365-379)

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (2)

attach (94-102)

detach (108-116)

Sources/AblyChat/Room.swift (2)

attach (377-383)

detach (386-392)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)

GitHub Check: Xcode, release configuration, macOS (Xcode 26.0)
GitHub Check: Example app, tvOS (Xcode 26.0)
GitHub Check: Xcode, release configuration, tvOS (Xcode 26.0)
GitHub Check: Example app, iOS (Xcode 26.0)
GitHub Check: Xcode, tvOS (Xcode 26.0)
GitHub Check: Example app, macOS (Xcode 26.0)
GitHub Check: Xcode, release configuration, iOS (Xcode 26.0)
GitHub Check: SPM (Xcode 26.0)
GitHub Check: Xcode, macOS (Xcode 26.0)
GitHub Check: Xcode, iOS (Xcode 26.0)
GitHub Check: SPM, release configuration (Xcode 26.0)
GitHub Check: check-documentation

🔇 Additional comments (12)

Sources/AblyChat/DefaultMessages.swift (1)

182-183: Good migration to InternalError.internallyThrown in continuation failures

The new error construction is consistent with the refactor and preserves control flow. LGTM.

Also applies to: 186-189

Sources/AblyChat/RoomLifecycleManager.swift (2)

19-22: Doc updates reflect new InternalError usage for presence checks

Accurately documents the new error shapes. LGTM.

213-216: Room lifecycle now throws InternalError.internallyThrown in all branches

Error construction is consistent, and discontinuity emission converts to ARTErrorInfo at the boundary. LGTM.

Also applies to: 396-399, 462-465, 610-613, 618-620

Sources/AblyChat/Rooms.swift (2)

171-176: LGTM! Error construction aligns with new internal model.

The change correctly replaces the old error construction pattern with InternalError.internallyThrown(.inconsistentRoomOptions(...)). This will be converted to ARTErrorInfo at the public API boundary (line 272), maintaining the expected public API contract.

348-348: LGTM! Consistent error construction pattern.

The change correctly uses InternalError.internallyThrown(.roomReleasedBeforeOperationCompleted) instead of constructing an ARTErrorInfo and converting it. This aligns with the new internal error handling model.

Tests/AblyChatTests/Helpers/Helpers.swift (4)

5-39: LGTM! Test helper correctly handles both error paths.

The updated isChatError function properly validates both ARTErrorInfo (in the chat domain) and InternalError.internallyThrown cases, checking code, statusCode, cause, and optional message for both paths. This aligns with the new internal error model while maintaining test coverage.

41-47: LGTM! Updated to match new InternalError case.

Correctly updated to match against InternalError.ablyCocoa instead of the previous InternalError.errorInfo case.

49-64: LGTM! Simplified enum aligns with refactored error model.

The InternalError.Case enum is now simpler, with only jsonValueDecodingError and other cases. The enumCase computed property correctly maps InternalError.internallyThrown(.other(.jsonValueDecodingError)) to the specific case and everything else to .other.

66-72: LGTM! Useful helper for case-based error checking.

The new isInternalErrorWithCase helper provides a clean way to check if an error is an InternalError with a specific case. Implementation is straightforward and correct.

Sources/AblyChat/Errors.swift (3)

123-161: LGTM! New internal error model is well-structured.

The InternalError enum with ablyCocoa and internallyThrown cases provides a clean separation between ably-cocoa errors and Chat SDK-specific errors. The toARTErrorInfo() conversion method correctly handles both paths:

ablyCocoa: Returns the ARTErrorInfo directly

internallyThrown: Constructs an ARTErrorInfo with proper domain, code, userInfo (including statusCode and localizedDescription), and optional cause

The computed message property is a useful addition for logging.

165-346: LGTM! Comprehensive internal error model with proper metadata.

The InternallyThrown enum provides a well-structured set of internal error cases with associated metadata. Key strengths:

Each case has meaningful associated values where needed (e.g., requested and existing options for inconsistentRoomOptions)

The ErrorCodeAndStatusCode enum properly distinguishes between fixed and variable status codes

The codeAndStatusCode, localizedDescription, and cause computed properties provide consistent error information

Helper methods like descriptionOfFeature and the AttachOrDetach enum improve code clarity

Error messages are descriptive and actionable

The Other nested enum provides a clean way to wrap various external error types.

348-378: LGTM! Convenient conversion extensions for internal error types.

The extensions on ChatAPI.ChatError, HeadersValue.JSONDecodingError, JSONValueDecodingError, PaginatedResultError, and DefaultMessages.MessagesError provide a clean and consistent way to convert these types to InternalError. The pattern of wrapping them in InternalError.internallyThrown(.other(...)) maintains uniformity across the error handling model.

coderabbitai

Actionable comments posted: 1

♻️ Duplicate comments (1)

Sources/AblyChat/Errors.swift (1)

139-152: Centralize the status-code key and update the outdated TODO

We’re still hard-coding "ARTErrorInfoStatusCode" and the adjacent TODO claims we can’t pass a domain, even though we now do. Please extract that key into a single private constant (or nested enum) and refresh/remove the TODO so it reflects the current reality. This was called out in the earlier review and is still outstanding.

🧹 Nitpick comments (1)

Sources/AblyChat/PaginatedResult.swift (1)
42-44: Consider updating for consistency with the new error pattern.

Line 43 still uses the older .toInternalError() method for PaginatedResultError. For consistency with the new pattern introduced on line 38, consider refactoring to use .internallyThrown():
-            continuation.resume(returning: .failure(PaginatedResultError.noErrorWithInvalidResponse.toInternalError()))
+            continuation.resume(returning: .failure(.internallyThrown(PaginatedResultError.noErrorWithInvalidResponse)))

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between ac30055 and 8d67745.

📒 Files selected for processing (17)

Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (9 hunks)
Sources/AblyChat/DefaultMessageReactions.swift (1 hunks)
Sources/AblyChat/DefaultMessages.swift (1 hunks)
Sources/AblyChat/Errors.swift (2 hunks)
Sources/AblyChat/InternalError.swift (0 hunks)
Sources/AblyChat/Message.swift (2 hunks)
Sources/AblyChat/PaginatedResult.swift (1 hunks)
Sources/AblyChat/RoomLifecycleManager.swift (5 hunks)
Sources/AblyChat/Rooms.swift (2 hunks)
Tests/AblyChatTests/ChatAPITests.swift (1 hunks)
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (2 hunks)
Tests/AblyChatTests/DefaultPresenceTests.swift (6 hunks)
Tests/AblyChatTests/Helpers/Helpers.swift (2 hunks)
Tests/AblyChatTests/InternalErrorTests.swift (1 hunks)
Tests/AblyChatTests/Mocks/MockRealtime.swift (1 hunks)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1 hunks)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (3 hunks)

💤 Files with no reviewable changes (1)

Sources/AblyChat/InternalError.swift

🚧 Files skipped from review as they are similar to previous changes (10)

Tests/AblyChatTests/DefaultPresenceTests.swift
Sources/AblyChat/Message.swift
Sources/AblyChat/DefaultMessageReactions.swift
Sources/AblyChat/RoomLifecycleManager.swift
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
Tests/AblyChatTests/Mocks/MockRealtime.swift
Sources/AblyChat/DefaultMessages.swift
Tests/AblyChatTests/InternalErrorTests.swift
Tests/AblyChatTests/ChatAPITests.swift
Tests/AblyChatTests/DefaultMessageReactionsTests.swift

🧰 Additional context used

📓 Path-based instructions (3)

Tests/AblyChatTests/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Tests/AblyChatTests/**/*.swift: Place all tests under Tests/AblyChatTests
Use test attribution tags: @SPEC, @specOneOf(m/n), @specPartial, @specUntested, @specNotApplicable with appropriate spec IDs and explanations
For Swift Testing #expect(throws:) with typed errors, move typed-throw code into a separate non-typed-throw function (until Xcode 16.3+)

Files:

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
Tests/AblyChatTests/Helpers/Helpers.swift

Tests/AblyChatTests/Mocks/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Place mock implementations under Tests/AblyChatTests/Mocks

Files:

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift

Sources/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Sources/**/*.swift: Prefer protocol-based design; expose SDK functionality via protocols and prefer opaque return types (some Protocol) over existential any Protocol
Isolate all mutable state to the main actor; mark stateful objects with @mainactor
Public API should use typed throws with ARTErrorInfo
Use InternalError internally and convert to ARTErrorInfo at public API boundaries
Test-only APIs in the library must be prefixed testsOnly_ and wrapped in #if DEBUG
Reference Chat SDK spec items in implementation comments (e.g., // @SPEC CHA-RL3g)
For public structs exposed by the API, provide an explicit public memberwise initializer
When using AsyncSequence operators in @mainactor contexts, mark operator closures as @sendable
For Task, CheckedContinuation, and AsyncThrowingStream (which don’t support typed errors), use Result and call .get() to bridge typed errors
Avoid Dictionary.mapValues for typed-throwing transformations; use ablyChat_mapValuesWithTypedThrow
When the compiler struggles with typed throws, explicitly annotate do throws(InternalError) blocks
Specify error types in closures, e.g., try items.map { jsonValue throws(InternalError) in … }

Files:

Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift
Sources/AblyChat/Rooms.swift
Sources/AblyChat/PaginatedResult.swift
Sources/AblyChat/Errors.swift

🧬 Code graph analysis (2)

Sources/AblyChat/PaginatedResult.swift (1)

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)

failure (71-73)

Sources/AblyChat/Errors.swift (5)

Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (2)

attach (349-363)

detach (365-379)

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (2)

attach (94-102)

detach (108-116)

Example/AblyChatExample/Mocks/MockClients.swift (2)

attach (85-87)

detach (89-91)

Tests/AblyChatTests/Mocks/MockRoom.swift (2)

attach (47-49)

detach (51-53)

Sources/AblyChat/Room.swift (2)

attach (377-383)

detach (386-392)

⏰ 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). (11)

GitHub Check: Xcode, release configuration, iOS (Xcode 26.0)
GitHub Check: Example app, macOS (Xcode 26.0)
GitHub Check: Example app, tvOS (Xcode 26.0)
GitHub Check: Xcode, release configuration, tvOS (Xcode 26.0)
GitHub Check: Example app, iOS (Xcode 26.0)
GitHub Check: Xcode, tvOS (Xcode 26.0)
GitHub Check: Xcode, release configuration, macOS (Xcode 26.0)
GitHub Check: Xcode, iOS (Xcode 26.0)
GitHub Check: Xcode, macOS (Xcode 26.0)
GitHub Check: SPM, release configuration (Xcode 26.0)
GitHub Check: SPM (Xcode 26.0)

🔇 Additional comments (1)

Sources/AblyChat/PaginatedResult.swift (1)

38-38: LGTM! Correctly adopts the new internal error pattern.

The change from error.toInternalError() to .ablyCocoa(error) properly wraps the ARTErrorInfo using the new standardized pattern, aligning with the PR's goal of consolidating internal error handling.

maratal

Some questions

This is a refactor that combines the ChatError and InternalError types (having three different error types — ARTErrorInfo, InternalError, and ChatError — at play in the codebase was getting rather confusing!) This is groundwork for some further error-related changes.

coderabbitai

Actionable comments posted: 1

♻️ Duplicate comments (2)

Sources/AblyChat/Errors.swift (1)

139-153: Centralize statusCode userInfo key and update stale TODO.

Replace the hard-coded "ARTErrorInfoStatusCode" with a private static constant to avoid typos.
The TODO about “no way to pass domain” is outdated; you are passing domain:. Reword to reflect only the statusCode-key workaround.

Example:

 internal func toARTErrorInfo() -> ARTErrorInfo {
     switch self {
     case let .fromAblyCocoa(errorInfo):
         return errorInfo
     case let .internallyThrown(internallyThrownError):
-        var userInfo: [String: Any] = [:]
-        // TODO: copied and pasted from implementation of -[ARTErrorInfo createWithCode:status:message:requestId:] because there’s no way to pass domain; revisit in https://github.com/ably-labs/ably-chat-swift/issues/32. Also the ARTErrorInfoStatusCode variable in ably-cocoa is not public.
-        userInfo["ARTErrorInfoStatusCode"] = internallyThrownError.codeAndStatusCode.statusCode
+        var userInfo: [String: Any] = [:]
+        // TODO: ably-cocoa does not expose a public constant for the statusCode userInfo key; track in https://github.com/ably/ably-cocoa/issues/XXXX.
+        userInfo[Self.artErrorInfoStatusCodeKey] = internallyThrownError.codeAndStatusCode.statusCode
         userInfo[NSLocalizedDescriptionKey] = internallyThrownError.localizedDescription
         ...
         return ARTErrorInfo(
             domain: errorDomain,
             code: internallyThrownError.codeAndStatusCode.code.rawValue,
             userInfo: userInfo,
         )
     }
 }
+
+// Near InternalError
+private extension InternalError {
+    static let artErrorInfoStatusCodeKey = "ARTErrorInfoStatusCode"
+}

Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (1)

167-168: Downcast caught errors to ARTErrorInfo before wrapping (compile-time fix).

In these catch blocks, error is any Error, but InternalError.fromAblyCocoa expects ARTErrorInfo. This won’t compile without a downcast. Since .get() throws ARTErrorInfo, use a typed catch.

Apply this at each site:
-        } catch {
-            throw InternalError.fromAblyCocoa(error)
+        } catch let ablyError as ARTErrorInfo {
+            throw InternalError.fromAblyCocoa(ablyError)
         }
Run to locate any remaining instances:
#!/bin/bash
rg -n -C2 -P 'catch\s*\{\s*throw\s+InternalError\.fromAblyCocoa\(\s*error\s*\)\s*\}' --glob '!**/DerivedData/**'
Also applies to: 237-238, 255-256, 271-272, 287-288, 303-304, 361-362, 377-378, 393-394

🧹 Nitpick comments (2)

Tests/AblyChatTests/InternalErrorTests.swift (1)

17-17: LGTM: internallyThrown path validated; consider attribution tags.

Assertion covers code/status and message content. Please add test attribution tags per guidelines (e.g., @specNotApplicable with a brief note).
Tests/AblyChatTests/Helpers/Helpers.swift (1)
14-20: Consider simplifying the message check.

The closure for checking the optional message works correctly but could be more concise.

Apply this diff to simplify:
-            && {
-                guard let message else {
-                    return true
-                }
-
-                return ablyError.message == message
-            }()
+            && (message == nil || ablyError.message == message)

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 8d67745 and 3891cb9.

📒 Files selected for processing (17)

Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (9 hunks)
Sources/AblyChat/DefaultMessageReactions.swift (1 hunks)
Sources/AblyChat/DefaultMessages.swift (1 hunks)
Sources/AblyChat/Errors.swift (2 hunks)
Sources/AblyChat/InternalError.swift (0 hunks)
Sources/AblyChat/Message.swift (2 hunks)
Sources/AblyChat/PaginatedResult.swift (1 hunks)
Sources/AblyChat/RoomLifecycleManager.swift (5 hunks)
Sources/AblyChat/Rooms.swift (2 hunks)
Tests/AblyChatTests/ChatAPITests.swift (1 hunks)
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (2 hunks)
Tests/AblyChatTests/DefaultPresenceTests.swift (6 hunks)
Tests/AblyChatTests/Helpers/Helpers.swift (2 hunks)
Tests/AblyChatTests/InternalErrorTests.swift (1 hunks)
Tests/AblyChatTests/Mocks/MockRealtime.swift (1 hunks)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1 hunks)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (3 hunks)

💤 Files with no reviewable changes (1)

Sources/AblyChat/InternalError.swift

🚧 Files skipped from review as they are similar to previous changes (5)

Sources/AblyChat/PaginatedResult.swift
Tests/AblyChatTests/DefaultPresenceTests.swift
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
Sources/AblyChat/Rooms.swift
Tests/AblyChatTests/DefaultMessageReactionsTests.swift

🧰 Additional context used

📓 Path-based instructions (3)

Sources/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Sources/**/*.swift: Prefer protocol-based design; expose SDK functionality via protocols and prefer opaque return types (some Protocol) over existential any Protocol
Isolate all mutable state to the main actor; mark stateful objects with @mainactor
Public API should use typed throws with ARTErrorInfo
Use InternalError internally and convert to ARTErrorInfo at public API boundaries
Test-only APIs in the library must be prefixed testsOnly_ and wrapped in #if DEBUG
Reference Chat SDK spec items in implementation comments (e.g., // @SPEC CHA-RL3g)
For public structs exposed by the API, provide an explicit public memberwise initializer
When using AsyncSequence operators in @mainactor contexts, mark operator closures as @sendable
For Task, CheckedContinuation, and AsyncThrowingStream (which don’t support typed errors), use Result and call .get() to bridge typed errors
Avoid Dictionary.mapValues for typed-throwing transformations; use ablyChat_mapValuesWithTypedThrow
When the compiler struggles with typed throws, explicitly annotate do throws(InternalError) blocks
Specify error types in closures, e.g., try items.map { jsonValue throws(InternalError) in … }

Files:

Sources/AblyChat/Message.swift
Sources/AblyChat/RoomLifecycleManager.swift
Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift
Sources/AblyChat/DefaultMessageReactions.swift
Sources/AblyChat/DefaultMessages.swift
Sources/AblyChat/Errors.swift

Tests/AblyChatTests/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Tests/AblyChatTests/**/*.swift: Place all tests under Tests/AblyChatTests
Use test attribution tags: @SPEC, @specOneOf(m/n), @specPartial, @specUntested, @specNotApplicable with appropriate spec IDs and explanations
For Swift Testing #expect(throws:) with typed errors, move typed-throw code into a separate non-typed-throw function (until Xcode 16.3+)

Files:

Tests/AblyChatTests/ChatAPITests.swift
Tests/AblyChatTests/Mocks/MockRealtime.swift
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
Tests/AblyChatTests/Helpers/Helpers.swift
Tests/AblyChatTests/InternalErrorTests.swift

Tests/AblyChatTests/Mocks/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Place mock implementations under Tests/AblyChatTests/Mocks

Files:

Tests/AblyChatTests/Mocks/MockRealtime.swift
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift

🧬 Code graph analysis (6)

Sources/AblyChat/Message.swift (1)

Sources/AblyChat/Errors.swift (1)

toARTErrorInfo (134-155)

Sources/AblyChat/RoomLifecycleManager.swift (1)

Sources/AblyChat/Errors.swift (1)

toARTErrorInfo (134-155)

Sources/AblyChat/DefaultMessageReactions.swift (1)

Sources/AblyChat/Errors.swift (1)

toARTErrorInfo (134-155)

Sources/AblyChat/DefaultMessages.swift (1)

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)

failure (71-73)

Tests/AblyChatTests/InternalErrorTests.swift (1)

Sources/AblyChat/Errors.swift (1)

toARTErrorInfo (134-155)

Sources/AblyChat/Errors.swift (2)

Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (2)

attach (349-363)

detach (365-379)

Sources/AblyChat/Room.swift (2)

attach (377-383)

detach (386-392)

⏰ 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). (11)

GitHub Check: Example app, iOS (Xcode 26.0)
GitHub Check: Xcode, iOS (Xcode 26.0)
GitHub Check: Xcode, release configuration, macOS (Xcode 26.0)
GitHub Check: Xcode, release configuration, iOS (Xcode 26.0)
GitHub Check: Xcode, release configuration, tvOS (Xcode 26.0)
GitHub Check: Example app, macOS (Xcode 26.0)
GitHub Check: Example app, tvOS (Xcode 26.0)
GitHub Check: Xcode, macOS (Xcode 26.0)
GitHub Check: Xcode, tvOS (Xcode 26.0)
GitHub Check: SPM (Xcode 26.0)
GitHub Check: SPM, release configuration (Xcode 26.0)

🔇 Additional comments (20)

Tests/AblyChatTests/ChatAPITests.swift (1)

24-25: LGTM: assertion matches the new InternalError surface.

Clear pattern-match against the expected case.

Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)

84-86: LGTM: failure wraps ARTErrorInfo into InternalError correctly.

No issues; mapping is precise and typed.

Sources/AblyChat/Errors.swift (1)

123-131: Review comment is incorrect; no changes needed.

The review expects a global extension Error { func toARTErrorInfo() } to exist, but the actual implementation is more type-safe: performAttachOperation() declares async throws(InternalError), so the caught error is already narrowed to InternalError at the catch site. The toARTErrorInfo() method exists only on the InternalError enum (Errors.swift:134), which is the correct approach. No global Error extension is needed or present, and this is not a deficiency—typed throws eliminate the need for it.

Likely an incorrect or invalid review comment.

Sources/AblyChat/DefaultMessages.swift (2)

181-183: Correct: use InternalError with Result bridging.

Resuming the continuation with .failure(InternalError.internallyThrown(.attachSerialIsNotDefined)) and later .get() adheres to the typed-throw guidance.

186-187: Good: standardized InternalError for attach failure cause.

Centralizes channel failure under .channelFailedToAttach(cause:). Logging the error object is fine.

Sources/AblyChat/RoomLifecycleManager.swift (6)

19-22: Docs updated to reflect InternalError usage — looks right.

The comments now describe InternalError throws per CHA‑RL9/PR3/PR10/PR6. No action required.

213-216: Discontinuity now emitted as ARTErrorInfo via InternalError — good.

Keeps subscriber surface consistent while preserving original cause in userInfo.

395-401: Attach precondition errors correctly standardized.

Throwing .roomIsReleasing / .roomIsReleased as InternalError matches RL1b/RL1c.

458-465: Detach precondition errors correctly standardized.

.roomIsReleasing, .roomIsReleased, and .roomInFailedState now unified under InternalError for RL2b/RL2c/RL2d.

612-613: Presence wait: invalid transition throws InternalError — good.

Includes original ARTErrorInfo as cause, satisfying RL9c.

619-620: Presence wait: require attach standardized.

.presenceOperationRequiresRoomAttach(feature:) is the right InternalError here (PR3h/PR10h/PR6h).

Tests/AblyChatTests/InternalErrorTests.swift (1)

9-9: LGTM: use InternalError.fromAblyCocoa for passthrough.

Confirms identity round‑trip to ARTErrorInfo.

Sources/AblyChat/DefaultMessageReactions.swift (1)

44-44: Verified: error mapping is correct.

The unableDeleteReactionWithoutName case correctly maps to .fixedStatusCode(.badRequest), which resolves to code 40000 and HTTP status 400, as required by CHA-MR11. The boundary conversion at line 44 properly throws the converted ARTErrorInfo, following the guideline to use InternalError internally and convert at public API boundaries.

Sources/AblyChat/Message.swift (3)

226-228: Verification confirmed: code 40000 and status 400 are correctly mapped.

The error chain is properly implemented:

ErrorCode.badRequest = 40000

cannotApplyEventForDifferentMessage → .fixedStatusCode(.badRequest)

InternalError→ARTErrorInfo conversion maintains code and status through the public API boundary

The implementation satisfies CHA-M11e (code 40000, status 400) as documented in Message.swift line 225.

252-254: LGTM: mismatched-serial now standardized via InternalError.

Both code paths consistently use InternalError.internallyThrown(.cannotApplyEventForDifferentMessage).toARTErrorInfo(), aligned with the spec: error code 40000 maps to "bad request" with HTTP status 400 (.badRequest). Line 227 (CHA-M11e, summary-event) and line 253 (CHA-M11b, message-event) are standardized correctly.

246-249: LGTM: created-event guard returns correct standardized ARTErrorInfo with error code 40000 and status code 400.

Verification confirms that cannotApplyCreatedMessageEvent correctly maps to ErrorCode.badRequest = 40000 with HTTP status code 400, satisfying spec requirement CHA-M11a.

Tests/AblyChatTests/Helpers/Helpers.swift (4)

23-36: LGTM!

The internally-thrown InternalError validation logic is clear and correct, with appropriate early returns for each validation check.

41-47: LGTM!

The update from .errorInfo to .fromAblyCocoa correctly aligns with the new error wrapping structure introduced in this PR.

56-62: Note the specific nested pattern matching.

The pattern .internallyThrown(.other(.jsonValueDecodingError)) at line 58 is quite specific with three levels of nesting. While this is appropriate for the current error structure, be aware that changes to InternalError's internal structure will require updating this pattern. Since this is in test helpers, the maintenance burden is acceptable.

66-72: LGTM!

The new isInternalErrorWithCase helper is a clean and useful addition for test assertions against specific error cases.

lawrence-forooghian · 2025-10-17T12:57:33Z

Gonna merge this one — please let me know if you have any further comments @maratal

github-actions Bot temporarily deployed to staging/pull/431/AblyChat October 15, 2025 20:26 Inactive

lawrence-forooghian force-pushed the 32-get-rid-of-ChatError branch from 3dda7d8 to 7635627 Compare October 15, 2025 20:30

github-actions Bot temporarily deployed to staging/pull/431/AblyChat October 15, 2025 20:31 Inactive

coderabbitai Bot reviewed Oct 15, 2025

View reviewed changes

lawrence-forooghian force-pushed the 32-get-rid-of-ChatError branch 2 times, most recently from 7105ce6 to 4751614 Compare October 16, 2025 13:36

github-actions Bot temporarily deployed to staging/pull/431/AblyChat October 16, 2025 13:38 Inactive

coderabbitai Bot reviewed Oct 16, 2025

View reviewed changes

Comment thread Sources/AblyChat/Errors.swift

lawrence-forooghian requested review from maratal and umair-ably October 16, 2025 13:55

lawrence-forooghian force-pushed the 32-get-rid-of-ChatError branch from 4751614 to ac30055 Compare October 16, 2025 14:44

github-actions Bot temporarily deployed to staging/pull/431/AblyChat October 16, 2025 14:45 Inactive

coderabbitai Bot reviewed Oct 16, 2025

View reviewed changes

lawrence-forooghian force-pushed the 32-get-rid-of-ChatError branch from ac30055 to 8d67745 Compare October 16, 2025 19:29

github-actions Bot temporarily deployed to staging/pull/431/AblyChat October 16, 2025 19:30 Inactive

coderabbitai Bot reviewed Oct 16, 2025

View reviewed changes

Comment thread Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift

lawrence-forooghian mentioned this pull request Oct 16, 2025

Throw our own ErrorInfo type instead of ARTErrorInfo #432

Merged

maratal reviewed Oct 16, 2025

View reviewed changes

Comment thread Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift Outdated

Comment thread Sources/AblyChat/Errors.swift

lawrence-forooghian force-pushed the 32-get-rid-of-ChatError branch from 8d67745 to 3891cb9 Compare October 17, 2025 11:56

github-actions Bot deployed to staging/pull/431/AblyChat October 17, 2025 11:58 View deployment

coderabbitai Bot reviewed Oct 17, 2025

View reviewed changes

Comment thread Tests/AblyChatTests/Mocks/MockRealtime.swift

lawrence-forooghian requested a review from maratal October 17, 2025 12:06

umair-ably approved these changes Oct 17, 2025

View reviewed changes

lawrence-forooghian merged commit 39ab907 into main Oct 17, 2025
17 checks passed

lawrence-forooghian deleted the 32-get-rid-of-ChatError branch October 17, 2025 13:00

This was referenced Oct 17, 2025

Remove InternalError's conformance to Error #434

Merged

[ECO-5608, ECO-5609] Align error codes and messages with spec #437

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid of `ChatError`#431

Get rid of `ChatError`#431
lawrence-forooghian merged 1 commit intomainfrom
32-get-rid-of-ChatError

lawrence-forooghian commented Oct 15, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 15, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

maratal left a comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

lawrence-forooghian commented Oct 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Conversation

lawrence-forooghian commented Oct 15, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maratal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lawrence-forooghian commented Oct 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

lawrence-forooghian commented Oct 15, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 15, 2025 •

edited

Loading