[ECO-5608, ECO-5609] Align error codes and messages with spec#437
[ECO-5608, ECO-5609] Align error codes and messages with spec#437lawrence-forooghian merged 5 commits intomainfrom
Conversation
WalkthroughRefactors internal error handling into a parameterized Changes
Sequence Diagram(s)sequenceDiagram
participant API as ChatAPI
participant REST as REST layer
participant Parser as JSONDecoding
rect rgb(247,250,255)
note right of API: makeRequest(path: String, ...)
API->>REST: request(method, path: path, params, body)
REST-->>API: HTTP response
API->>Parser: decode response
alt decode succeeds
Parser-->>API: Model
else decode fails
Parser-->>API: throw JSONValueDecodingError(type:, rawValue:)
end
alt missing expected item
API-->>API: throw InternalError.noItemInResponse(path: path)
end
end
sequenceDiagram
participant Caller as Feature
participant Lifecycle as RoomLifecycleManager
Caller->>Lifecycle: waitToBeAbleToPerformPresenceOperations()
alt attached or attaching
Lifecycle-->>Caller: return (ready)
else not attached
Lifecycle-->>Caller: throw InternalError.presenceOperationRequiresRoomAttach
else invalid/releasing/failed
Lifecycle-->>Caller: throw InternalError.roomTransitionedToInvalidStateForPresenceOperation(newState, cause)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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/437/AblyChat
lawrence-forooghian
force-pushed
the
426-427-align-codes-and-messages
branch
from
73feb83 to
f8a014d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/437/AblyChat
lawrence-forooghian
force-pushed
the
426-427-align-codes-and-messages
branch
from
f8a014d to
3a2ee83
Compare
github-actions
bot
temporarily deployed
to
staging/pull/437/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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/AblyChat/RoomLifecycleManager.swift (1)
591-606: Potential double-resume of continuation in waitToBeAbleToPerformPresenceOperationsIf multiple status changes arrive before
nextRoomStatusSubscription.off()runs, the closure can resume the same continuation more than once. Move.off()into the callback and guard with a local flag to ensure single resume.- var nextRoomStatusSubscription: StatusSubscription! - var nextRoomStatusChange: RoomStatusChange! + var nextRoomStatusSubscription: StatusSubscription! + var nextRoomStatusChange: RoomStatusChange! + var didResume = false 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 - nextRoomStatusChange = statusChange - self?.logger.log(message: "waitToBeAbleToPerformPresenceOperations got status change \(String(describing: nextRoomStatusChange))", level: .debug) - continuation.resume() + guard !didResume else { return } + didResume = true + nextRoomStatusChange = statusChange + self?.logger.log(message: "waitToBeAbleToPerformPresenceOperations got status change \(String(describing: nextRoomStatusChange))", level: .debug) + nextRoomStatusSubscription.off() + continuation.resume() } } - nextRoomStatusSubscription.off()Also applies to: 607-614
🧹 Nitpick comments (2)
Sources/AblyChat/ChatAPI.swift (1)
196-217: Unify naming: prefer “path” consistentlymakeRequest now uses
_ path: String, but makePaginatedRequest still uses_ url: String. For consistency and clearer semantics with realtime.request(path:), rename the param and call sites.- private func makePaginatedRequest<Response: JSONDecodable & Sendable & Equatable>( - _ url: String, + private func makePaginatedRequest<Response: JSONDecodable & Sendable & Equatable>( + _ path: String, params: [String: String]? = nil, ) async throws(ErrorInfo) -> some PaginatedResult<Response> { - let paginatedResponse = try await realtime.request("GET", path: url, params: params, body: nil, headers: [:]) + let paginatedResponse = try await realtime.request("GET", path: path, params: params, body: nil, headers: [:])Sources/AblyChat/InternalError.swift (1)
203-224: Minor consistency: use uppercased status in presence-transition messageOther cases use descriptionOfRoomStatus(…) for “ATTACHED/…”. Consider reusing it in roomTransitionedToInvalidStateForPresenceOperation to keep output uniform.
- case let .roomTransitionedToInvalidStateForPresenceOperation(newState: newState, cause: cause): + case let .roomTransitionedToInvalidStateForPresenceOperation(newState: newState, cause: cause): failedActionBareInfinitive = "perform presence operation" - reason = "room transitioned to invalid state \(newState): \(cause, default: "(nil cause)")" + reason = "room transitioned to invalid state \(Self.descriptionOfRoomStatus(newState)): \(cause, default: "(nil cause)")"Also applies to: 241-329
📜 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 (21)
Sources/AblyChat/ChatAPI.swift(4 hunks)Sources/AblyChat/DefaultMessageReactions.swift(2 hunks)Sources/AblyChat/DefaultMessages.swift(2 hunks)Sources/AblyChat/DefaultPresence.swift(6 hunks)Sources/AblyChat/ErrorInfo.swift(2 hunks)Sources/AblyChat/InternalError.swift(2 hunks)Sources/AblyChat/JSONCodable.swift(2 hunks)Sources/AblyChat/Message.swift(3 hunks)Sources/AblyChat/PaginatedResult.swift(1 hunks)Sources/AblyChat/RoomFeature.swift(0 hunks)Sources/AblyChat/RoomLifecycleManager.swift(5 hunks)Sources/AblyChat/Rooms.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(1 hunks)Tests/AblyChatTests/DefaultMessageReactionsTests.swift(3 hunks)Tests/AblyChatTests/DefaultPresenceTests.swift(13 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(10 hunks)Tests/AblyChatTests/DefaultRoomsTests.swift(3 hunks)Tests/AblyChatTests/ErrorInfoTests.swift(1 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(2 hunks)Tests/AblyChatTests/MessageTests.swift(6 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(1 hunks)
💤 Files with no reviewable changes (1)
- Sources/AblyChat/RoomFeature.swift
🧰 Additional context used
📓 Path-based instructions (4)
**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.swift: Use protocol-based design; expose SDK functionality via protocols and prefer associated types with opaque return types (some Protocol) instead of existentials (any Protocol)
Isolate all mutable state to the main actor; mark stateful objects with @mainactor
Public API must use typed throws with ErrorInfo; use InternalError internally and convert at the public API boundary
For public structs emitted by the API, provide an explicit public memberwise initializer
When using AsyncSequence operators in @mainactor contexts, mark operator closures as @sendable
Task, CheckedContinuation, and AsyncThrowingStream do not support typed errors; use Result and call .get() to surface typed errors
Do not use Dictionary.mapValues for typed throws; use ablyChat_mapValuesWithTypedThrow instead
When the compiler struggles with typed throws, explicitly declare the error type on do blocks (e.g., do throws(InternalError))
Specify error types in closures using the throws(ErrorType) syntax (e.g., try items.map { jsonValue throws(InternalError) in ... })
Mark any test-only APIs with testsOnly_ prefix and wrap them in #if DEBUG
Files:
Tests/AblyChatTests/DefaultMessageReactionsTests.swiftSources/AblyChat/DefaultMessageReactions.swiftSources/AblyChat/DefaultPresence.swiftSources/AblyChat/PaginatedResult.swiftSources/AblyChat/ErrorInfo.swiftSources/AblyChat/Rooms.swiftTests/AblyChatTests/ErrorInfoTests.swiftSources/AblyChat/Message.swiftSources/AblyChat/JSONCodable.swiftTests/AblyChatTests/MessageTests.swiftTests/AblyChatTests/Helpers/Helpers.swiftTests/AblyChatTests/Mocks/MockRoomLifecycleManager.swiftTests/AblyChatTests/DefaultRoomsTests.swiftTests/AblyChatTests/ChatAPITests.swiftSources/AblyChat/ChatAPI.swiftTests/AblyChatTests/DefaultRoomLifecycleManagerTests.swiftSources/AblyChat/RoomLifecycleManager.swiftTests/AblyChatTests/DefaultPresenceTests.swiftSources/AblyChat/DefaultMessages.swiftSources/AblyChat/InternalError.swift
Tests/AblyChatTests/**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
Tests/AblyChatTests/**/*.swift: Use test attribution tags to reference spec coverage: @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 until Xcode 16.3+
Files:
Tests/AblyChatTests/DefaultMessageReactionsTests.swiftTests/AblyChatTests/ErrorInfoTests.swiftTests/AblyChatTests/MessageTests.swiftTests/AblyChatTests/Helpers/Helpers.swiftTests/AblyChatTests/Mocks/MockRoomLifecycleManager.swiftTests/AblyChatTests/DefaultRoomsTests.swiftTests/AblyChatTests/ChatAPITests.swiftTests/AblyChatTests/DefaultRoomLifecycleManagerTests.swiftTests/AblyChatTests/DefaultPresenceTests.swift
Tests/AblyChatTests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place tests under Tests/AblyChatTests/ as the root for test targets
Files:
Tests/AblyChatTests/DefaultMessageReactionsTests.swiftTests/AblyChatTests/ErrorInfoTests.swiftTests/AblyChatTests/MessageTests.swiftTests/AblyChatTests/Helpers/Helpers.swiftTests/AblyChatTests/Mocks/MockRoomLifecycleManager.swiftTests/AblyChatTests/DefaultRoomsTests.swiftTests/AblyChatTests/ChatAPITests.swiftTests/AblyChatTests/DefaultRoomLifecycleManagerTests.swiftTests/AblyChatTests/DefaultPresenceTests.swift
Tests/AblyChatTests/Mocks/**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
Put mock implementations under Tests/AblyChatTests/Mocks/
Files:
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
🧬 Code graph analysis (14)
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (1)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)
Sources/AblyChat/DefaultPresence.swift (2)
Sources/AblyChat/RoomLifecycleManager.swift (1)
waitToBeAbleToPerformPresenceOperations(580-622)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
waitToBeAbleToPerformPresenceOperations(60-69)
Sources/AblyChat/PaginatedResult.swift (1)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)
Tests/AblyChatTests/ErrorInfoTests.swift (2)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasCode(10-26)
Sources/AblyChat/Message.swift (1)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)
Sources/AblyChat/JSONCodable.swift (1)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2)
Sources/AblyChat/RoomLifecycleManager.swift (1)
waitToBeAbleToPerformPresenceOperations(580-622)Tests/AblyChatTests/Helpers/Helpers.swift (1)
addRecord(114-118)
Tests/AblyChatTests/DefaultRoomsTests.swift (2)
Sources/AblyChat/Rooms.swift (2)
get(60-63)get(164-269)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasCode(10-26)
Sources/AblyChat/ChatAPI.swift (3)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)Tests/AblyChatTests/Mocks/MockRealtime.swift (1)
request(29-45)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (1)
request(148-169)
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (4)
Sources/AblyChat/RoomLifecycleManager.swift (6)
performAttachOperation(370-372)performAttachOperation(374-376)performDetachOperation(433-435)performDetachOperation(437-439)testsOnly_subscribeToStatusChangeWaitEvents(634-636)waitToBeAbleToPerformPresenceOperations(580-622)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (3)
performAttachOperation(25-31)performDetachOperation(33-39)waitToBeAbleToPerformPresenceOperations(60-69)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasCode(10-26)Tests/AblyChatTests/Helpers/RoomLifecycleManager+AsyncSequence.swift (1)
testsOnly_subscribeToStatusChangeWaitEvents(60-74)
Sources/AblyChat/RoomLifecycleManager.swift (2)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
waitToBeAbleToPerformPresenceOperations(60-69)Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)
Tests/AblyChatTests/DefaultPresenceTests.swift (2)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)Tests/AblyChatTests/Helpers/Helpers.swift (2)
hasCode(10-26)hasRecord(120-131)
Sources/AblyChat/DefaultMessages.swift (1)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)
Sources/AblyChat/InternalError.swift (3)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (2)
attach(94-102)detach(108-116)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (2)
attach(351-365)detach(367-381)Sources/AblyChat/Room.swift (2)
attach(377-379)detach(382-384)
⏰ 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). (10)
- GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 26.0) - GitHub Check: Example app, tvOS (Xcode 26.0)
- GitHub Check: Example app, macOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 26.0) - GitHub Check: Xcode, tvOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: Xcode, macOS (Xcode 26.0)
- GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: SPM,
releaseconfiguration (Xcode 26.0)
🔇 Additional comments (58)
Sources/AblyChat/ErrorInfo.swift (1)
26-35: LGTM! Clean simplification of internal error property access.The updated property paths (
internalError.code.rawValueandinternalError.code.statusCode) align with the new internal error model and improve code clarity while maintaining the public API surface.Also applies to: 70-79
Sources/AblyChat/DefaultMessageReactions.swift (2)
40-41: Good addition of spec reference.The CHA-MR11b1 comment improves traceability to the specification for the non-unique reaction validation.
98-100: Good addition of spec reference.The CHA-MR7c comment documents the validation requirement, though the implementation choice of
fatalErrorfor programmer errors is appropriate for Swift.Sources/AblyChat/Rooms.swift (1)
169-175: Improved error context for room options mismatch.The updated error
roomExistsWithDifferentOptions(requested:existing:)provides much better debugging information by including both the requested and existing options, compared to the previous genericinconsistentRoomOptions.Tests/AblyChatTests/ChatAPITests.swift (1)
24-29: Test correctly updated for simplified error hierarchy.The pattern match now directly checks for
.noItemInResponsewithout nested wrappers, aligning with the flattened internal error model.Tests/AblyChatTests/Helpers/Helpers.swift (2)
4-26: Test helper correctly updated for new error model.The renamed
hasCode(_:cause:message:)function (previouslyhasCodeAndStatusCode) simplifies the test API while maintaining the implicit statusCode validation mentioned in the documentation.
29-44: LGTM! Enum case helper updated appropriately.The
enumCaseproperty updated to handle the newjsonValueDecodingErrorcase pattern.Sources/AblyChat/DefaultPresence.swift (1)
23-23: Consistent removal of requestedByFeature parameter.All six presence operation call sites correctly updated to use the parameterless
waitToBeAbleToPerformPresenceOperations()API, aligning with the protocol signature change shown in the relevant code snippets.Also applies to: 44-44, 66-66, 97-97, 125-125, 153-153
Tests/AblyChatTests/MessageTests.swift (2)
9-9: Test annotations correctly updated to align with spec changes.The spec reference updates (CHA-M11a/b/e → CHA-M11h/i/j) reflect the specification restructuring mentioned in the PR objectives.
Also applies to: 63-63, 353-353, 393-393
59-59: Error codes correctly updated from generic to specific.All three test assertions consistently updated from error code
40000(generic BadRequest) to40003, aligning with the specification's more specific error categorization for invalid arguments.Also applies to: 112-112, 434-434
Tests/AblyChatTests/ErrorInfoTests.swift (1)
51-62: Test correctly updated for new internal error structure.The test now exercises the updated error conversion pathway using
headersValueJSONDecodingErrorinstead of the removedChatErrorwrapper, and validates the standardized error message format. The spec annotation (CHA-GP6) is appropriately added.Tests/AblyChatTests/DefaultMessageReactionsTests.swift (2)
61-78: LGTM: Spec reference and error assertion updated correctly.The spec reference update from CHA-MR4a1 to CHA-MR4a2 and the new specific error case
InternalError.sendMessageReactionEmptyMessageSerialimprove clarity and align with the specification changes.
81-98: LGTM: Spec reference and error assertion updated correctly.The spec reference update from CHA-MR11a1 to CHA-MR11a2 and the new specific error case
InternalError.deleteMessageReactionEmptyMessageSerialmaintain consistency with the send operation changes.Sources/AblyChat/JSONCodable.swift (2)
33-33: LGTM: Enhanced error with type information.Adding the
typeparameter to the error case improves error diagnostics by including the expected type in the error message.
296-296: LGTM: Error throw updated to include type information.The updated throw site now provides both the expected type and the raw value, which will significantly improve error messages when decoding fails.
Sources/AblyChat/PaginatedResult.swift (1)
42-49: LGTM: Improved error handling with clearer separation of concerns.The split validation (nil check with
preconditionFailure, then status code check) is clearer than the previous approach. UsingpreconditionFailurefor nil response is appropriate since it indicates a programming error in the SDK (ARTHTTPPaginatedResponse should always provide either an error or a response).The new
InternalError.paginatedResultStatusCodeerror is more specific and aligns with the standardized error model.Tests/AblyChatTests/DefaultRoomsTests.swift (3)
174-180: LGTM: Updated to use more specific error code.The change from a generic
badRequestto the specificroomExistsWithDifferentOptionserror code improves error clarity and aligns with the specification.
215-221: LGTM: Consistent error code update.This test correctly uses the same
roomExistsWithDifferentOptionserror code, maintaining consistency with the first test case.
373-373: LGTM: Simplified error assertion.The change from
hasCodeAndStatusCodetohasCodereflects the updated error checking API while maintaining the same validation logic.Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
60-68: LGTM: API simplified by removing feature parameter.Removing the
requestedByFeatureparameter simplifies the presence operation API. The mock implementation correctly records the call with no arguments.Sources/AblyChat/DefaultMessages.swift (2)
100-100: LGTM: More descriptive error for subscription point resolution.The new
InternalError.failedToResolveSubscriptionPointBecauseMessagesInstanceGoneis more descriptive than the genericMessagesError.noReferenceToSelfand follows a consistent naming pattern.
162-167: LGTM: Consistent error naming for subscription point failures.Both errors (
failedToResolveSubscriptionPointBecauseAttachSerialNotDefinedandfailedToResolveSubscriptionPointBecauseChannelFailedToAttach) follow the established naming pattern and clearly describe the failure reason.Sources/AblyChat/Message.swift (3)
181-181: LGTM: Enhanced error diagnostics with type information.Adding the
type: ChatMessageAction.selfparameter provides better error messages when decoding fails, consistent with the changes in JSONCodable.swift.
227-227: LGTM: More specific error for reaction summary events.The new
cannotApplyReactionSummaryEventForDifferentMessageerror is more specific than the previous generic error, making it easier to distinguish between different event application failures.
248-253: LGTM: Distinct errors for different message event scenarios.Using separate error cases (
cannotApplyCreatedMessageEventandcannotApplyMessageEventForDifferentMessage) improves error clarity and makes debugging easier by immediately identifying which validation failed.Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (4)
100-118: LGTM: Consolidated error code with updated spec references.The consolidation to
roomInInvalidState(from the now-obsoleteroomIsReleasing) simplifies the error model. The spec reference update to CHA-RL1l reflects the latest specification.
283-317: LGTM: Consistent error code updates for detach operations.All detach operation tests correctly use
roomInInvalidStatewith updated spec references (CHA-RL2l, CHA-RL2m), maintaining consistency across the test suite.
937-940: LGTM: Simplified discontinuity error assertion.The updated
hasCodemethod withroomDiscontinuityand cause parameter correctly validates the discontinuity error structure.
956-1069: LGTM: Presence operation tests updated to parameterless API.All four presence operation tests correctly updated to:
- Use
waitToBeAbleToPerformPresenceOperations()without therequestedByFeatureparameter- Use the simplified
roomInInvalidStateerror code- Include clear error messages explaining required room status
The changes are thorough and consistent across all test scenarios (attaching→attached, attaching→failed, attached, and other statuses).
Tests/AblyChatTests/DefaultPresenceTests.swift (15)
77-79: Updated wait helper invocation looks correctSignature and empty-args expectation align with the parameterless API.
87-88: Good: presence ATTACHING → invalid transition mapped via InternalErrorConstructing roomTransitionedToInvalidStateForPresenceOperation(...) then toErrorInfo() matches the new flow.
105-105: Asserting code and cause is spot onValidates .roomInInvalidState and preserves the underlying attachError.
109-111: Wait helper recorded as expectedMock call signature matches "waitToBeAbleToPerformPresenceOperations()".
118-118: Correct error for not-attached presence oppresenceOperationRequiresRoomAttach is the intended precondition failure.
134-134: Right code asserted.roomInInvalidState matches mapping for the precondition failure case.
184-186: Update presence while attaching: wait call verifiedMatches new API and test intent.
194-195: Invalid transition error wiring LGTMMirrors the enter() failure path.
212-212: Cause propagation asserted correctlyKeeps attachError as cause.
216-218: Wait helper invocation recordedConsistent with other tests.
225-225: Precondition error on update presenceCorrect use of presenceOperationRequiresRoomAttach.
241-241: Right code for invalid stateMatches mapping.
330-330: Precondition error for presence get()Correct error source.
346-346: Code assertion LGTM.roomInInvalidState is expected.
369-371: Wait helper check again OKSignature and args match mocks.
Sources/AblyChat/ChatAPI.swift (3)
141-145: Correct: empty serial → InvalidArgument via InternalErrorGuard + sendMessageReactionEmptyMessageSerial.toErrorInfo() aligns with spec text.
159-163: Delete reaction empty serial validation is correctMaps to deleteMessageReactionEmptyMessageSerial.toErrorInfo().
209-213: Nice: empty paginated response → InternalError.noItemInResponseClearer diagnostics than a generic decode failure.
Sources/AblyChat/RoomLifecycleManager.swift (4)
395-399: Attach invalid-state mapping looks correct.roomIsReleasing(.attach) and .roomInInvalidStateForAttach(roomStatus) align with the new error taxonomy.
458-462: Detach invalid-state mapping looks correct.roomIsReleasing(.detach) and .roomInInvalidStateForDetach(roomStatus) as per spec points.
609-614: Error construction and mapping LGTMThrows roomTransitionedToInvalidStateForPresenceOperation(...).toErrorInfo() and presenceOperationRequiresRoomAttach.toErrorInfo() per CHA-RL9c/PR3h.
Also applies to: 620-621
625-636: DEBUG test hooks are cleartestsOnly_subscribeToStatusChangeWaitEvents and event emission are consistent with other DEBUG hooks.
Also applies to: 633-636
Sources/AblyChat/InternalError.swift (7)
9-20: Attach/Detach invalid-state cases are a good splitroomInInvalidStateForAttach/Detach improves diagnostics and mapping.
36-45: Reactions: empty serial mapped to InvalidArgumentsend/delete variants separate the contexts cleanly.
Also applies to: 41-45
51-55: roomIsReleasing folded into roomInInvalidState code — OKComment explains spec change; mapping kept consistent via code property.
66-75: Presence errors and messages consistentCode maps to .roomInInvalidState; message follows “unable to …; …” format.
Also applies to: 170-176, 266-272
154-201: code mapping switch reads wellEach InternalError maps deterministically to an ErrorCode.
376-386: Convenience conversions for decoding errors LGTMheadersValueJSONDecodingError/jsonValueDecodingError mapping is straightforward.
Also applies to: 382-385
127-152: I'll search GitHub for the Ably specification repository to find the authoritative error code mappings.I need to search more specifically for the Chat specification and PR 394:
I found Chat error codes in the 102000 range in ably-common's errors.json, but the specific codes referenced (102100, 102106, 102107, 102112) and PR 394 are not appearing in search results. Let me perform one final search to verify these specific codes:
Based on my web search attempts, I've gathered the following information:
Findings:
Chat error codes are documented in ably-common's errors.json, with codes in the 102000 range including codes like 102000-102005 and 102050-102054. However, the specific error codes in your review (102100, 102106, 102107, 102112) do not appear in publicly available search results.
Additionally, I was unable to locate any public reference to "PR 394" in the Ably Chat or specification repositories.
Verification Status:
- ✅ ably-common repository exists and contains shared error codes
- ✅ ably/specification repository is the authoritative source for SDK development guidance
- ❌ The specific Chat error codes (102100, 102106, 102107, 102112) cannot be verified through public sources
- ❌ PR 394 reference cannot be located publicly
Conclusion:
The error code values and their HTTP status mappings appear to be from an internal or non-public Chat specification. Without access to the authoritative Ably Chat specification or PR 394, I cannot definitively confirm whether these mappings are correct or outdated.
Verify error code constants and HTTP status mappings against the Chat specification
The specific error codes (102100, 102106, 102107, 102112) and PR 394 referenced in this review cannot be confirmed through public Ably repositories or documentation. Please verify the mappings directly against the internal Ably Chat specification to ensure they remain current.
lawrence-forooghian
force-pushed
the
426-427-align-codes-and-messages
branch
from
3a2ee83 to
a8d06d6
Compare
github-actions
bot
temporarily deployed
to
staging/pull/437/AblyChat
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/AblyChat/RoomLifecycleManager.swift (1)
591-606: Avoid potential double-resume in withCheckedContinuation by unsubscribing inside the callback before resuming.Unsubscribe inside the handler, then remove the post‑continuation
off().- await withCheckedContinuation { (continuation: CheckedContinuation<Void, _>) in + 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 nextRoomStatusChange = statusChange self?.logger.log(message: "waitToBeAbleToPerformPresenceOperations got status change \(String(describing: nextRoomStatusChange))", level: .debug) + nextRoomStatusSubscription.off() continuation.resume() } } - nextRoomStatusSubscription.off()
♻️ Duplicate comments (1)
Sources/AblyChat/InternalError.swift (1)
271-275: Non-standard string interpolation will not compile without a custom helper. Replace with standard Optional formatting.Use
cause.map(String.init(describing:)) ?? "(nil cause)".- reason = "room transitioned to invalid state \(newState): \(cause, default: "(nil cause)")" + reason = "room transitioned to invalid state \(newState): \(cause.map(String.init(describing:)) ?? "(nil cause)")" @@ - reason = "room experienced a discontinuity: \(cause, default: "(nil cause)")" + reason = "room experienced a discontinuity: \(cause.map(String.init(describing:)) ?? "(nil cause)")" @@ - reason = "channel failed to attach: \(cause, default: "(nil cause)")" + reason = "channel failed to attach: \(cause.map(String.init(describing:)) ?? "(nil cause)")"Also applies to: 295-295
🧹 Nitpick comments (4)
Sources/AblyChat/ChatAPI.swift (2)
159-162: Delete reaction: empty-serial check OK; add pre-validation for missing name when required.Keep the empty-serial error. Additionally, enforce name presence for reaction types that require it so callers get a deterministic SDK error before the request.
internal func deleteReactionFromMessage(_ messageSerial: String, roomName: String, params: DeleteMessageReactionParams) async throws(ErrorInfo) -> MessageReactionResponse { // ... - var httpParams: [String: String] = [ + // If the reaction type requires a name, validate early. + if params.type != .unique, params.name == nil { + throw InternalError.unableDeleteReactionWithoutName(reactionType: params.type.rawValue).toErrorInfo() + } + + var httpParams: [String: String] = [ "type": params.type.rawValue, ] httpParams["name"] = params.name return try await makeRequest(endpoint, method: "DELETE", params: httpParams) }Also applies to: 166-171
219-229: Rename_ url→_ pathfor consistency withmakeRequestandrealtime.request(path:).Purely internal but avoids confusion.
- private func makePaginatedRequest<Response: JSONDecodable & Sendable & Equatable>( - _ url: String, + private func makePaginatedRequest<Response: JSONDecodable & Sendable & Equatable>( + _ path: String, params: [String: String]? = nil, ) async throws(ErrorInfo) -> some PaginatedResult<Response> { - let paginatedResponse = try await realtime.request("GET", path: url, params: params, body: nil, headers: [:]) + let paginatedResponse = try await realtime.request("GET", path: path, params: params, body: nil, headers: [:]) let jsonValues = paginatedResponse.items.map { JSONValue(ablyCocoaData: $0) } let items = try jsonValues.map { jsonValue throws(ErrorInfo) in try Response(jsonValue: jsonValue) } return paginatedResponse.toPaginatedResult(items: items) }Sources/AblyChat/InternalError.swift (1)
80-84: Optional: improve case name grammar.Consider
unableToDeleteReactionWithoutNamefor readability. Public API unaffected if internal-only.Sources/AblyChat/RoomLifecycleManager.swift (1)
76-83: Mark the stateful manager type as @mainactor to enforce isolation of mutable state.Matches the guideline “Isolate all mutable state to the main actor; mark stateful objects with @mainactor.”
-internal class DefaultRoomLifecycleManager: RoomLifecycleManager { +@MainActor +internal class DefaultRoomLifecycleManager: RoomLifecycleManager {
📜 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 (21)
Sources/AblyChat/ChatAPI.swift(4 hunks)Sources/AblyChat/DefaultMessageReactions.swift(2 hunks)Sources/AblyChat/DefaultMessages.swift(2 hunks)Sources/AblyChat/DefaultPresence.swift(6 hunks)Sources/AblyChat/ErrorInfo.swift(2 hunks)Sources/AblyChat/InternalError.swift(2 hunks)Sources/AblyChat/JSONCodable.swift(2 hunks)Sources/AblyChat/Message.swift(3 hunks)Sources/AblyChat/PaginatedResult.swift(1 hunks)Sources/AblyChat/RoomFeature.swift(0 hunks)Sources/AblyChat/RoomLifecycleManager.swift(5 hunks)Sources/AblyChat/Rooms.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(1 hunks)Tests/AblyChatTests/DefaultMessageReactionsTests.swift(3 hunks)Tests/AblyChatTests/DefaultPresenceTests.swift(13 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(10 hunks)Tests/AblyChatTests/DefaultRoomsTests.swift(3 hunks)Tests/AblyChatTests/ErrorInfoTests.swift(1 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(2 hunks)Tests/AblyChatTests/MessageTests.swift(6 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(1 hunks)
💤 Files with no reviewable changes (1)
- Sources/AblyChat/RoomFeature.swift
🚧 Files skipped from review as they are similar to previous changes (9)
- Sources/AblyChat/DefaultMessageReactions.swift
- Sources/AblyChat/Rooms.swift
- Sources/AblyChat/DefaultPresence.swift
- Tests/AblyChatTests/ErrorInfoTests.swift
- Sources/AblyChat/ErrorInfo.swift
- Tests/AblyChatTests/DefaultPresenceTests.swift
- Sources/AblyChat/DefaultMessages.swift
- Sources/AblyChat/PaginatedResult.swift
- Tests/AblyChatTests/DefaultMessageReactionsTests.swift
🧰 Additional context used
📓 Path-based instructions (4)
**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.swift: Use protocol-based design; expose SDK functionality via protocols and prefer associated types with opaque return types (some Protocol) instead of existentials (any Protocol)
Isolate all mutable state to the main actor; mark stateful objects with @mainactor
Public API must use typed throws with ErrorInfo; use InternalError internally and convert at the public API boundary
For public structs emitted by the API, provide an explicit public memberwise initializer
When using AsyncSequence operators in @mainactor contexts, mark operator closures as @sendable
Task, CheckedContinuation, and AsyncThrowingStream do not support typed errors; use Result and call .get() to surface typed errors
Do not use Dictionary.mapValues for typed throws; use ablyChat_mapValuesWithTypedThrow instead
When the compiler struggles with typed throws, explicitly declare the error type on do blocks (e.g., do throws(InternalError))
Specify error types in closures using the throws(ErrorType) syntax (e.g., try items.map { jsonValue throws(InternalError) in ... })
Mark any test-only APIs with testsOnly_ prefix and wrap them in #if DEBUG
Files:
Tests/AblyChatTests/MessageTests.swiftTests/AblyChatTests/DefaultRoomLifecycleManagerTests.swiftTests/AblyChatTests/ChatAPITests.swiftTests/AblyChatTests/DefaultRoomsTests.swiftSources/AblyChat/Message.swiftSources/AblyChat/JSONCodable.swiftSources/AblyChat/RoomLifecycleManager.swiftSources/AblyChat/InternalError.swiftTests/AblyChatTests/Helpers/Helpers.swiftTests/AblyChatTests/Mocks/MockRoomLifecycleManager.swiftSources/AblyChat/ChatAPI.swift
Tests/AblyChatTests/**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
Tests/AblyChatTests/**/*.swift: Use test attribution tags to reference spec coverage: @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 until Xcode 16.3+
Files:
Tests/AblyChatTests/MessageTests.swiftTests/AblyChatTests/DefaultRoomLifecycleManagerTests.swiftTests/AblyChatTests/ChatAPITests.swiftTests/AblyChatTests/DefaultRoomsTests.swiftTests/AblyChatTests/Helpers/Helpers.swiftTests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
Tests/AblyChatTests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place tests under Tests/AblyChatTests/ as the root for test targets
Files:
Tests/AblyChatTests/MessageTests.swiftTests/AblyChatTests/DefaultRoomLifecycleManagerTests.swiftTests/AblyChatTests/ChatAPITests.swiftTests/AblyChatTests/DefaultRoomsTests.swiftTests/AblyChatTests/Helpers/Helpers.swiftTests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
Tests/AblyChatTests/Mocks/**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
Put mock implementations under Tests/AblyChatTests/Mocks/
Files:
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
🧬 Code graph analysis (7)
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (4)
Sources/AblyChat/RoomLifecycleManager.swift (6)
performAttachOperation(370-372)performAttachOperation(374-376)performDetachOperation(433-435)performDetachOperation(437-439)testsOnly_subscribeToStatusChangeWaitEvents(634-636)waitToBeAbleToPerformPresenceOperations(580-622)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (3)
performAttachOperation(25-31)performDetachOperation(33-39)waitToBeAbleToPerformPresenceOperations(60-69)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasCode(10-26)Tests/AblyChatTests/Helpers/RoomLifecycleManager+AsyncSequence.swift (1)
testsOnly_subscribeToStatusChangeWaitEvents(60-74)
Tests/AblyChatTests/DefaultRoomsTests.swift (2)
Sources/AblyChat/Rooms.swift (2)
get(60-63)get(164-269)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasCode(10-26)
Sources/AblyChat/Message.swift (1)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)
Sources/AblyChat/JSONCodable.swift (1)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)
Sources/AblyChat/RoomLifecycleManager.swift (1)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
waitToBeAbleToPerformPresenceOperations(60-69)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2)
Sources/AblyChat/RoomLifecycleManager.swift (1)
waitToBeAbleToPerformPresenceOperations(580-622)Tests/AblyChatTests/Helpers/Helpers.swift (1)
addRecord(114-118)
Sources/AblyChat/ChatAPI.swift (4)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)Tests/AblyChatTests/Mocks/MockRealtime.swift (1)
request(29-45)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (1)
request(148-169)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (1)
first(50-52)
⏰ 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,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: Xcode, tvOS (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: SPM,
releaseconfiguration (Xcode 26.0) - GitHub Check: SPM (Xcode 26.0)
🔇 Additional comments (25)
Tests/AblyChatTests/Helpers/Helpers.swift (2)
5-26: LGTM! Helper function simplified appropriately.The refactoring from
hasCodeAndStatusCodetohasCodealigns well with the new error model. The note on line 8 is helpful for encouraging proper error validation.
36-43: LGTM! Error case handling updated correctly.The switch case properly reflects the new flat error structure for
jsonValueDecodingError.Tests/AblyChatTests/ChatAPITests.swift (1)
23-29: LGTM! Error assertion updated correctly.The error matching has been properly updated to reflect the new flattened error structure, checking for
.internalError(.noItemInResponse)directly instead of the nested variant.Tests/AblyChatTests/DefaultRoomsTests.swift (3)
158-181: LGTM! Error expectations correctly updated.The test properly validates the
roomExistsWithDifferentOptionserror using the simplifiedhasCodehelper.
183-225: LGTM! Consistent error validation.The second test for
roomExistsWithDifferentOptionsfollows the same pattern and correctly validates the error case.
329-383: LGTM! Release operation error handling validated.The test correctly checks for
roomReleasedBeforeOperationCompletederror using thehasCodehelper.Sources/AblyChat/JSONCodable.swift (2)
29-34: LGTM! Enhanced error information.Adding the
typeparameter tofailedToDecodeFromRawValueprovides better context for debugging decoding failures.
275-300: LGTM! Call site updated correctly.The function properly passes both the type information and raw value to the enhanced error case.
Tests/AblyChatTests/MessageTests.swift (4)
9-61: LGTM! Spec annotations and error code updated.The spec annotation has been correctly updated to CHA-M11h, and the error code changed to 40003, aligning with the new error model.
63-114: LGTM! Consistent updates.The spec annotation (CHA-M11i) and error code (40003) are correctly updated.
352-391: LGTM! Summary event spec annotation updated.The spec annotation has been correctly updated to CHA-M11j for the reaction summary event test.
393-436: LGTM! Consistent error code and spec annotation.The error code (40003) and spec annotation (CHA-M11j) are correctly aligned.
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
60-69: LGTM! Mock updated for parameterless presence API.The mock correctly reflects the simplified presence wait API that no longer requires a
requestedByFeatureparameter. The call recording with empty arguments is appropriate.Sources/AblyChat/Message.swift (3)
169-198: LGTM! JSON decoding error enhanced with type information.The call to
failedToDecodeFromRawValuecorrectly passes bothChatMessageAction.selfand the raw value, improving error context.
213-233: LGTM! Error case updated for reaction summary events.The error has been appropriately renamed to
cannotApplyReactionSummaryEventForDifferentMessage, providing clearer semantics. The comment correctly references the InvalidArgument error code from the spec.
235-266: LGTM! Message event error cases refined.The error cases have been properly updated:
cannotApplyCreatedMessageEventfor created eventscannotApplyMessageEventForDifferentMessagefor serial mismatchesBoth align with spec terminology referencing InvalidArgument errors.
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (4)
91-119: LGTM! Error consolidation for invalid room states.The tests correctly use
roomInInvalidStateerror for both releasing and released states. The comment on line 100 helpfully explains that the oldroomIsReleasingerror code no longer exists in the spec.
274-318: LGTM! Consistent error handling for detach operations.The detach operation tests properly validate
roomInInvalidStateerrors for releasing, released, and failed states. Spec annotations have been updated appropriately (CHA-RL2l, CHA-RL2m).
900-945: LGTM! Discontinuity error validation updated.The test correctly validates the
roomDiscontinuityerror code with its cause, using thehasCodehelper method properly.
948-1070: LGTM! Presence wait API simplified correctly.All calls to
waitToBeAbleToPerformPresenceOperations()have been updated to remove therequestedByFeatureparameter. The error expectations correctly validateroomInInvalidStatewith appropriate messages. The test at line 1069 includes a helpful message explaining the room must be in ATTACHED or ATTACHING status.Sources/AblyChat/ChatAPI.swift (2)
141-144: Spec-aligned empty-serial check — good.Throwing
InternalError.sendMessageReactionEmptyMessageSerial.toErrorInfo()matches CHA‑MR4a2; typed throws preserved.
196-207: Path propagation and typed throws — good.Switch to
pathand throwingInternalError.noItemInResponse(path:)is clean and aligns with the new error surface. Conversion toJSONValueandResponse(jsonValue:)remains correct.Also applies to: 209-217
Sources/AblyChat/RoomLifecycleManager.swift (2)
19-23: Docstring fix — good.Updated to the new
roomTransitionedToInvalidStateForPresenceOperation(newState:cause:)andpresenceOperationRequiresRoomAttach.
609-614: Presence-flow errors use new InternalError variants — good.Error propagation via
toErrorInfo()is typed and spec-aligned.Also applies to: 620-621
Sources/AblyChat/InternalError.swift (1)
131-151: Unable to verify error mappings against spec PR 394—recommend manual review.The error codes and status mappings in
InternalError.swift(lines 131–151) are consistent and well-established throughout the codebase:
- Numeric values: badRequest (40000), invalidArgument (40003), roomDiscontinuity (102_100), roomReleasedBeforeOperationCompleted (102_106), roomExistsWithDifferentOptions (102_107), roomInInvalidState (102_112)
- Status mappings: 400 for badRequest, invalidArgument, roomReleasedBeforeOperationCompleted, roomInInvalidState, roomExistsWithDifferentOptions; 500 for roomDiscontinuity
The code references "Common Error Codes used by Chat" and "Chat-specific Error Codes" sections of the chat spec (comment at line 141). These codes appear across 50+ test assertions with consistent spec point references (CHA-RL1l, CHA-RL2l, CHA-RL2m, etc.), but GitHub PR 394 is not accessible to confirm the exact mappings.
Verify these values against the merged specification to confirm alignment with PR 394.
lawrence-forooghian
force-pushed
the
426-427-align-codes-and-messages
branch
from
a8d06d6 to
2b00409
Compare
lawrence-forooghian
changed the base branch from
main
to
2025-10-21-undo-spec-pinning
github-actions
bot
temporarily deployed
to
staging/pull/437/AblyChat
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/AblyChat/InternalError.swift (1)
271-271: Verify custom string interpolation helper exists.The syntax
\(cause, default: "(nil cause)")requires a customStringInterpolationextension with anappendInterpolation(_:default:)method. A previous review verified this helper doesn't exist in the codebase, which would cause compilation errors.Verify whether the code compiles successfully:
#!/bin/bash # Description: Check if the project builds successfully and search for StringInterpolation extensions # Check for any StringInterpolation extensions that might support this syntax rg -n "appendInterpolation" --type swift -A3 -B1 # Also check if there's a custom String extension rg -n "extension.*String.*Interpolation" --type swift -A5If the interpolation helper is missing, replace with standard Swift:
-reason = "room transitioned to invalid state \(newState): \(cause, default: "(nil cause)")" +reason = "room transitioned to invalid state \(newState): \(cause.map { String(describing: $0) } ?? "(nil cause)")"Also applies to: 274-274, 295-295
🧹 Nitpick comments (3)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
60-67: Signature/recording alignment looks good; two small nits.
- Typo in fatalError message (“resultOfWaitToBeAblePerformPresenceOperations”): missing “To”.
- Consider marking this mock @mainactor to match state isolation in tests and production interfaces. This avoids accidental cross-actor mutation of counters. As per coding guidelines.
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (1)
1063-1070: Message assertion adds confidence but is brittle.
Hard-coding the message ties tests to wording tweaks. Consider asserting code+cause only, or centralize the expected message via a helper to reduce churn. Based on coding guidelines.Tests/AblyChatTests/DefaultPresenceTests.swift (1)
109-111: Tiny test ergonomics nit.
You repeat the “waitToBeAble…” record assertion across tests; consider a small helper like expectPresenceWait(roomLifecycleManager) to DRY it.
📜 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 (21)
Sources/AblyChat/ChatAPI.swift(4 hunks)Sources/AblyChat/DefaultMessageReactions.swift(2 hunks)Sources/AblyChat/DefaultMessages.swift(2 hunks)Sources/AblyChat/DefaultPresence.swift(6 hunks)Sources/AblyChat/ErrorInfo.swift(2 hunks)Sources/AblyChat/InternalError.swift(2 hunks)Sources/AblyChat/JSONCodable.swift(2 hunks)Sources/AblyChat/Message.swift(3 hunks)Sources/AblyChat/PaginatedResult.swift(1 hunks)Sources/AblyChat/RoomFeature.swift(0 hunks)Sources/AblyChat/RoomLifecycleManager.swift(5 hunks)Sources/AblyChat/Rooms.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(1 hunks)Tests/AblyChatTests/DefaultMessageReactionsTests.swift(3 hunks)Tests/AblyChatTests/DefaultPresenceTests.swift(13 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(10 hunks)Tests/AblyChatTests/DefaultRoomsTests.swift(3 hunks)Tests/AblyChatTests/ErrorInfoTests.swift(1 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(2 hunks)Tests/AblyChatTests/MessageTests.swift(6 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(1 hunks)
💤 Files with no reviewable changes (1)
- Sources/AblyChat/RoomFeature.swift
🚧 Files skipped from review as they are similar to previous changes (6)
- Sources/AblyChat/ErrorInfo.swift
- Tests/AblyChatTests/DefaultMessageReactionsTests.swift
- Sources/AblyChat/Rooms.swift
- Tests/AblyChatTests/Helpers/Helpers.swift
- Tests/AblyChatTests/ChatAPITests.swift
- Sources/AblyChat/DefaultMessageReactions.swift
🧰 Additional context used
📓 Path-based instructions (4)
**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.swift: Use protocol-based design; expose SDK functionality via protocols and prefer associated types with opaque return types (some Protocol) instead of existentials (any Protocol)
Isolate all mutable state to the main actor; mark stateful objects with @mainactor
Public API must use typed throws with ErrorInfo; use InternalError internally and convert at the public API boundary
For public structs emitted by the API, provide an explicit public memberwise initializer
When using AsyncSequence operators in @mainactor contexts, mark operator closures as @sendable
Task, CheckedContinuation, and AsyncThrowingStream do not support typed errors; use Result and call .get() to surface typed errors
Do not use Dictionary.mapValues for typed throws; use ablyChat_mapValuesWithTypedThrow instead
When the compiler struggles with typed throws, explicitly declare the error type on do blocks (e.g., do throws(InternalError))
Specify error types in closures using the throws(ErrorType) syntax (e.g., try items.map { jsonValue throws(InternalError) in ... })
Mark any test-only APIs with testsOnly_ prefix and wrap them in #if DEBUG
Files:
Sources/AblyChat/DefaultPresence.swiftTests/AblyChatTests/DefaultRoomsTests.swiftTests/AblyChatTests/ErrorInfoTests.swiftSources/AblyChat/PaginatedResult.swiftSources/AblyChat/JSONCodable.swiftTests/AblyChatTests/MessageTests.swiftSources/AblyChat/ChatAPI.swiftTests/AblyChatTests/DefaultRoomLifecycleManagerTests.swiftSources/AblyChat/DefaultMessages.swiftSources/AblyChat/InternalError.swiftTests/AblyChatTests/Mocks/MockRoomLifecycleManager.swiftSources/AblyChat/Message.swiftSources/AblyChat/RoomLifecycleManager.swiftTests/AblyChatTests/DefaultPresenceTests.swift
Tests/AblyChatTests/**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
Tests/AblyChatTests/**/*.swift: Use test attribution tags to reference spec coverage: @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 until Xcode 16.3+
Files:
Tests/AblyChatTests/DefaultRoomsTests.swiftTests/AblyChatTests/ErrorInfoTests.swiftTests/AblyChatTests/MessageTests.swiftTests/AblyChatTests/DefaultRoomLifecycleManagerTests.swiftTests/AblyChatTests/Mocks/MockRoomLifecycleManager.swiftTests/AblyChatTests/DefaultPresenceTests.swift
Tests/AblyChatTests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place tests under Tests/AblyChatTests/ as the root for test targets
Files:
Tests/AblyChatTests/DefaultRoomsTests.swiftTests/AblyChatTests/ErrorInfoTests.swiftTests/AblyChatTests/MessageTests.swiftTests/AblyChatTests/DefaultRoomLifecycleManagerTests.swiftTests/AblyChatTests/Mocks/MockRoomLifecycleManager.swiftTests/AblyChatTests/DefaultPresenceTests.swift
Tests/AblyChatTests/Mocks/**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
Put mock implementations under Tests/AblyChatTests/Mocks/
Files:
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
🧬 Code graph analysis (12)
Sources/AblyChat/DefaultPresence.swift (2)
Sources/AblyChat/RoomLifecycleManager.swift (1)
waitToBeAbleToPerformPresenceOperations(580-622)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
waitToBeAbleToPerformPresenceOperations(60-69)
Tests/AblyChatTests/DefaultRoomsTests.swift (2)
Sources/AblyChat/Rooms.swift (2)
get(60-63)get(164-269)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasCode(10-26)
Tests/AblyChatTests/ErrorInfoTests.swift (2)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasCode(10-26)
Sources/AblyChat/PaginatedResult.swift (1)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)
Sources/AblyChat/JSONCodable.swift (1)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)
Sources/AblyChat/ChatAPI.swift (4)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)Tests/AblyChatTests/Mocks/MockRealtime.swift (1)
request(29-45)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (1)
request(148-169)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (1)
first(50-52)
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (4)
Sources/AblyChat/RoomLifecycleManager.swift (6)
performAttachOperation(370-372)performAttachOperation(374-376)performDetachOperation(433-435)performDetachOperation(437-439)testsOnly_subscribeToStatusChangeWaitEvents(634-636)waitToBeAbleToPerformPresenceOperations(580-622)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (3)
performAttachOperation(25-31)performDetachOperation(33-39)waitToBeAbleToPerformPresenceOperations(60-69)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasCode(10-26)Tests/AblyChatTests/Helpers/RoomLifecycleManager+AsyncSequence.swift (1)
testsOnly_subscribeToStatusChangeWaitEvents(60-74)
Sources/AblyChat/DefaultMessages.swift (1)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2)
Sources/AblyChat/RoomLifecycleManager.swift (1)
waitToBeAbleToPerformPresenceOperations(580-622)Tests/AblyChatTests/Helpers/Helpers.swift (1)
addRecord(114-118)
Sources/AblyChat/Message.swift (1)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)
Sources/AblyChat/RoomLifecycleManager.swift (2)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
waitToBeAbleToPerformPresenceOperations(60-69)Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)
Tests/AblyChatTests/DefaultPresenceTests.swift (2)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(371-373)Tests/AblyChatTests/Helpers/Helpers.swift (2)
hasCode(10-26)hasRecord(120-131)
⏰ 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: Xcode,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 26.0) - GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: Xcode, tvOS (Xcode 26.0)
- GitHub Check: Xcode, macOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 26.0) - GitHub Check: SPM,
releaseconfiguration (Xcode 26.0) - GitHub Check: SPM (Xcode 26.0)
- GitHub Check: check-documentation
🔇 Additional comments (25)
Sources/AblyChat/DefaultMessages.swift (1)
100-100: Consider whether throwing is appropriate when instance is deallocated.A past reviewer suggested a silent exit here since "it's gone now, who cares?". The current approach throws an error, which provides diagnostic information. Consider whether:
- The error helps diagnose issues in production/testing
- Or if silent failure is more appropriate for a deallocated instance
Both approaches are valid; the choice depends on whether you want visibility into this edge case.
Based on learnings
Sources/AblyChat/RoomLifecycleManager.swift (1)
19-22: LGTM! Signature and documentation updated consistently.The removal of the
requestedByFeatureparameter simplifies the API, and the documentation correctly references the updated error variant withnewState:cause:parameters. All call sites have been updated accordingly.Sources/AblyChat/DefaultPresence.swift (1)
23-23: LGTM! All call sites updated consistently.The removal of the
requestedByFeatureparameter has been applied consistently across all presence operations.Also applies to: 44-44, 66-66, 97-97, 125-125, 153-153
Tests/AblyChatTests/ErrorInfoTests.swift (1)
51-61: LGTM! Test updated to reflect new internal error model.The test now exercises the
headersValueJSONDecodingErrorvariant and verifies the correct error code and message format.Tests/AblyChatTests/DefaultRoomsTests.swift (1)
180-180: LGTM! Test assertions updated for new error codes.The tests now correctly verify the new
roomExistsWithDifferentOptionsandroomReleasedBeforeOperationCompletederror codes.Also applies to: 221-221, 373-373
Sources/AblyChat/JSONCodable.swift (1)
33-33: LGTM! Type information enriches error diagnostics.Adding the
typeparameter tofailedToDecodeFromRawValuewill help with debugging by making it clear which type failed to decode from its raw value.Also applies to: 296-296
Sources/AblyChat/ChatAPI.swift (2)
196-196: LGTM! Parameter rename improves clarity.Renaming
urltopathis more accurate since the parameter is a path segment, not a complete URL. The change has been propagated correctly through the call chain.Also applies to: 209-209
212-212: LGTM! Error includes path for better diagnostics.Including the
pathparameter in thenoItemInResponseerror will help with debugging by showing which endpoint failed to return an item.Tests/AblyChatTests/MessageTests.swift (1)
59-59: Error code 40003 is correctly mapped but external spec verification needed.The codebase shows error code 40003 is defined as
invalidArgumentinSources/AblyChat/InternalError.swift:132and all three test assertions (lines 59, 112, 434) consistently check for this code. The test scenarios—mismatched message serial, invalid timestamp, and wrong message type—logically represent invalid arguments. The InternalError enum documentation states these error codes derive from the Ably Chat spec, but the spec document itself cannot be verified within the codebase.Sources/AblyChat/Message.swift (1)
181-182: Good alignment to spec-driven errors and richer decode failures.
- Using JSONValueDecodingError.failedToDecodeFromRawValue(type:…, rawValue:…) improves diagnostics.
- Specific InternalError variants for reaction/message mismatch and created-event are clearer and map well to InvalidArgument.
Please confirm these variants map to the intended public code (InvalidArgument) in ErrorInfo formatting. Based on coding guidelines.
Also applies to: 225-228, 246-254
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (5)
100-105: Spec-aligned error assertions (roomInInvalidState) look correct.
These updates reflect the consolidated error code and read cleanly. LGTM.Also applies to: 107-119, 283-288, 290-302, 304-318
937-941: Discontinuity cause assertion is precise and valuable.
Verifying the propagated cause strengthens failure diagnosis. LGTM.
974-985: Presence wait: parameterless API usage verified end-to-end.
The subscription/wait flow and success path assertions look solid. LGTM.
1013-1016: Presence wait: failure path assertions are correct.
Asserting roomInInvalidState with the attach error as cause matches the new InternalError shape. LGTM.Also applies to: 1024-1034
1046-1049: Attached fast-path test reads clean and minimal.
Good direct success assertion on parameterless wait. LGTM.Tests/AblyChatTests/DefaultPresenceTests.swift (3)
77-79: Updated recorder expectations for parameterless presence-wait are correct.
Matches the new mock signature and ensures the wait happens. LGTM.Also applies to: 105-105, 184-186, 212-212, 216-218, 369-371, 401-403
87-87: Failure-path construction aligns with new InternalError variants.
Using roomTransitionedToInvalidStateForPresenceOperation and mapping to roomInInvalidState with cause is spot on. LGTM.Also applies to: 194-194, 379-379
118-118: Room-in-invalid-state expectations read clearly across operations.
Consistent spec coverage across enter/update/get flows. LGTM.Also applies to: 134-135, 225-225, 241-242, 330-330, 346-347, 397-397
Sources/AblyChat/InternalError.swift (7)
9-75: LGTM! Spec-aligned error cases are well-documented.The error cases are clearly documented with spec references, and the associated values provide appropriate context for error reporting.
78-78: TODO is appropriately scoped for issue #438.The non-spec errors are marked for future review per the linked issue. This is a reasonable interim approach.
127-152: LGTM! Error code enum correctly implements spec values.The error codes and status code mappings align with the spec requirements. The previously flagged typo has been corrected.
154-201: LGTM! Error code mappings are comprehensive and correct.All error cases are properly mapped to their corresponding error codes, with appropriate spec references.
203-239: LGTM! Helper methods provide clear error context.The
descriptionOfRoomStatusandAttachOrDetachhelpers effectively support error message generation. While a past review suggested "action" instead of "bareInfinitive", the current naming is linguistically accurate and documents the property's grammatical purpose.
331-361: LGTM! Cause extraction correctly handles all error cases.The implementation properly extracts cause information from error cases that have it, and returns nil for others.
363-386: LGTM! Error conversion protocol is well-designed.The
ConvertibleToInternalErrorprotocol provides a clean abstraction for converting domain-specific errors toInternalError, with appropriate conformances for JSON decoding errors.
Based on spec PR [1] at 23ca1db. I've also added documentation for all of the InternalError cases to describe what they represent. We can also use this documentation to explain the associated data. Note that this highlights that there are some removed spec points that we're still implementing; things that we missed in e.g. the single channel migration. We still need to address those but it's not in the scope of the current task. We still have some usage of the 40000 BadRequest code for errors not specified by the spec, violating CHA-GP5; will revisit these errors separately in #438. Note that we no longer have any error codes with a variable status code; will remove this mechanism in a separate commit in case we need to restore it some time. Resolves #426. [1] ably/specification#394
Want to treat them like our other errors and give them proper messages and allow each to have its own code if necessary.
No longer needed as of abc3d51.
Got Claude to rewrite these and made some tweaks. There's still room for improvement here (especially with the decoding error ones, where there's not really enough context about the operation that was being performed). Resolves #427.
lawrence-forooghian
force-pushed
the
426-427-align-codes-and-messages
branch
from
2b00409 to
dfd423e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (2)
81-89: Test asserts against a channel that isn’t used by the managerYou create
channelbut don’t pass it tocreateManager(...), so the assertion checks the wrong instance. Inject the channel.- let channel = createChannel() - let manager = createManager(forTestingWhatHappensWhenCurrentlyIn: .attached) + let channel = createChannel() + let manager = createManager(forTestingWhatHappensWhenCurrentlyIn: .attached, channel: channel)
142-145: Fix invalidasync letusage and ensure tasks are awaitedPatterns like
async let _ = try await ...are invalid and also leave child tasks un-awaited, risking test flakiness and compilation errors. Capture the task withoutawaiton the RHS and await it later.Apply these localized fixes:
- attach_ifOtherOperationInProgress_waitsForItToComplete (Lines 142-165)
- async let _ = manager.performDetachOperation(testsOnly_forcingOperationID: detachOperationID) + async let detaching: Void = manager.performDetachOperation(testsOnly_forcingOperationID: detachOperationID) @@ - try await attachResult + try await attachResult + try await detaching
- attach_transitionsToAttaching (Lines 179-190)
- async let _ = try await manager.performAttachOperation() + async let attaching: Void = try manager.performAttachOperation() @@ channelAttachOperation.complete(behavior: .success) + try await attaching
- detach_transitionsToDetaching (Lines 378-388)
- async let _ = try await manager.performDetachOperation() + async let detaching: Void = try manager.performDetachOperation() @@ channelDetachOperation.complete(behavior: .success) + try await detaching
- release_transitionsToReleasing (Lines 559-569)
- async let _ = await manager.performReleaseOperation() + async let releasing: Void = manager.performReleaseOperation() @@ channelDetachOperation.complete(behavior: .success) + await releasing
- channelStateChange_withOperationInProgress (Lines 716-740)
- async let _ = manager.performAttachOperation() + async let attaching: Void = manager.performAttachOperation() @@ channelAttachBehavior.complete(behavior: .success /* arbitrary */ ) + try await attaching
- waitToBeAbleToPerformPresenceOperations_whenAttaching_whenTransitionsToAttached (Lines 969-986)
- async let _ = manager.performAttachOperation(testsOnly_forcingOperationID: attachOperationID) + async let attaching: Void = manager.performAttachOperation(testsOnly_forcingOperationID: attachOperationID) @@ channelAttachOperation.complete(behavior: .success) @@ - try await waitToBeAbleToPerformPresenceOperationsResult + try await waitToBeAbleToPerformPresenceOperationsResult + try await attaching
- waitToBeAbleToPerformPresenceOperations_whenAttaching_whenTransitionsToNonAttachedStatus (Lines 1008-1034)
- async let _ = manager.performAttachOperation(testsOnly_forcingOperationID: attachOperationID) + async let attaching: Void = manager.performAttachOperation(testsOnly_forcingOperationID: attachOperationID) @@ channelAttachOperation.complete(behavior: .completeAndChangeState(.failure(channelAttachError), newState: .failed)) @@ - #expect(try #require(caughtError).hasCode(.roomInInvalidState, cause: expectedCause)) + #expect(try #require(caughtError).hasCode(.roomInInvalidState, cause: expectedCause)) + _ = try? await attachingAlso applies to: 179-181, 342-343, 379-380, 560-561, 716-717, 969-973, 1008-1012
♻️ Duplicate comments (4)
Sources/AblyChat/PaginatedResult.swift (1)
42-48: ReplacepreconditionFailurewith typed error handling and accept 2xx status codes.Two issues remain unaddressed from the previous review:
PreconditionFailure crashes the app: Line 43's
preconditionFailurewill terminate the process. Replace with a typed internal error (e.g.,InternalError.unexpectedNilPaginatedResponse) and resume the continuation with failure.Status code check is overly restrictive: Line 46 checks for exact equality with 200, but Ably's REST API treats all 2xx codes as success. Change to
(200...299).contains(paginatedResponse.statusCode).Sources/AblyChat/InternalError.swift (2)
270-271: Custom string interpolation syntax will not compile.Lines 270, 273, and 294 use the syntax
\(cause, default: "(nil cause)"), which is not standard Swift and requires a customStringInterpolationextension that does not exist in the codebase (verified by searching the entire project).Replace with standard Swift syntax:
cause.map { String(describing: $0) } ?? "(nil cause)"or simply:
"\(cause?.description ?? "(nil cause)")"Also applies to: 273-274, 294-295
129-129: Fix typo: "Commmon" → "Common".Both lines contain a typo in the comment text.
Also applies to: 140-140
Sources/AblyChat/RoomLifecycleManager.swift (1)
19-23: Docstring now references the correct error symbol (newState:cause:)The documentation reflects the updated
roomTransitionedToInvalidStateForPresenceOperation(newState:cause:). Thanks for addressing the earlier note.
🧹 Nitpick comments (1)
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (1)
1068-1070: Assert error code only, not hardcoded messageThe hardcoded message string is dynamically constructed in
InternalError.swift(line 326) from separateopandreasonvalues. If either value changes, the test fails despite correct behavior.Instead of asserting the full message, assert only the error code:
#expect(caughtError.hasCode(.roomInInvalidState))
📜 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 (21)
Sources/AblyChat/ChatAPI.swift(4 hunks)Sources/AblyChat/DefaultMessageReactions.swift(2 hunks)Sources/AblyChat/DefaultMessages.swift(2 hunks)Sources/AblyChat/DefaultPresence.swift(6 hunks)Sources/AblyChat/ErrorInfo.swift(2 hunks)Sources/AblyChat/InternalError.swift(2 hunks)Sources/AblyChat/JSONCodable.swift(2 hunks)Sources/AblyChat/Message.swift(3 hunks)Sources/AblyChat/PaginatedResult.swift(1 hunks)Sources/AblyChat/RoomFeature.swift(0 hunks)Sources/AblyChat/RoomLifecycleManager.swift(5 hunks)Sources/AblyChat/Rooms.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(1 hunks)Tests/AblyChatTests/DefaultMessageReactionsTests.swift(3 hunks)Tests/AblyChatTests/DefaultPresenceTests.swift(13 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(10 hunks)Tests/AblyChatTests/DefaultRoomsTests.swift(3 hunks)Tests/AblyChatTests/ErrorInfoTests.swift(1 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(2 hunks)Tests/AblyChatTests/MessageTests.swift(6 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(1 hunks)
💤 Files with no reviewable changes (1)
- Sources/AblyChat/RoomFeature.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/AblyChat/DefaultMessageReactions.swift
🚧 Files skipped from review as they are similar to previous changes (8)
- Tests/AblyChatTests/DefaultMessageReactionsTests.swift
- Sources/AblyChat/DefaultPresence.swift
- Sources/AblyChat/DefaultMessages.swift
- Tests/AblyChatTests/Helpers/Helpers.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
- Sources/AblyChat/Message.swift
- Sources/AblyChat/ErrorInfo.swift
- Tests/AblyChatTests/ErrorInfoTests.swift
🧰 Additional context used
📓 Path-based instructions (3)
**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.swift: Use protocol-based design; expose SDK functionality via protocols and prefer associated types with opaque return types (some Protocol) instead of existentials (any Protocol)
Isolate all mutable state to the main actor; mark stateful objects with @mainactor
Public API must use typed throws with ErrorInfo; use InternalError internally and convert at the public API boundary
For public structs emitted by the API, provide an explicit public memberwise initializer
When using AsyncSequence operators in @mainactor contexts, mark operator closures as @sendable
Task, CheckedContinuation, and AsyncThrowingStream do not support typed errors; use Result and call .get() to surface typed errors
Do not use Dictionary.mapValues for typed throws; use ablyChat_mapValuesWithTypedThrow instead
When the compiler struggles with typed throws, explicitly declare the error type on do blocks (e.g., do throws(InternalError))
Specify error types in closures using the throws(ErrorType) syntax (e.g., try items.map { jsonValue throws(InternalError) in ... })
Mark any test-only APIs with testsOnly_ prefix and wrap them in #if DEBUG
Files:
Sources/AblyChat/Rooms.swiftTests/AblyChatTests/DefaultRoomLifecycleManagerTests.swiftSources/AblyChat/JSONCodable.swiftTests/AblyChatTests/DefaultRoomsTests.swiftTests/AblyChatTests/ChatAPITests.swiftTests/AblyChatTests/DefaultPresenceTests.swiftSources/AblyChat/PaginatedResult.swiftSources/AblyChat/InternalError.swiftSources/AblyChat/ChatAPI.swiftTests/AblyChatTests/MessageTests.swiftSources/AblyChat/RoomLifecycleManager.swift
Tests/AblyChatTests/**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
Tests/AblyChatTests/**/*.swift: Use test attribution tags to reference spec coverage: @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 until Xcode 16.3+
Files:
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swiftTests/AblyChatTests/DefaultRoomsTests.swiftTests/AblyChatTests/ChatAPITests.swiftTests/AblyChatTests/DefaultPresenceTests.swiftTests/AblyChatTests/MessageTests.swift
Tests/AblyChatTests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place tests under Tests/AblyChatTests/ as the root for test targets
Files:
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swiftTests/AblyChatTests/DefaultRoomsTests.swiftTests/AblyChatTests/ChatAPITests.swiftTests/AblyChatTests/DefaultPresenceTests.swiftTests/AblyChatTests/MessageTests.swift
🧬 Code graph analysis (8)
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (4)
Sources/AblyChat/RoomLifecycleManager.swift (6)
performAttachOperation(370-372)performAttachOperation(374-376)performDetachOperation(433-435)performDetachOperation(437-439)testsOnly_subscribeToStatusChangeWaitEvents(634-636)waitToBeAbleToPerformPresenceOperations(580-622)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (3)
performAttachOperation(25-31)performDetachOperation(33-39)waitToBeAbleToPerformPresenceOperations(60-69)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasCode(10-26)Tests/AblyChatTests/Helpers/RoomLifecycleManager+AsyncSequence.swift (1)
testsOnly_subscribeToStatusChangeWaitEvents(60-74)
Sources/AblyChat/JSONCodable.swift (1)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(370-372)
Tests/AblyChatTests/DefaultRoomsTests.swift (2)
Sources/AblyChat/Rooms.swift (2)
get(60-63)get(164-269)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasCode(10-26)
Tests/AblyChatTests/DefaultPresenceTests.swift (2)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(370-372)Tests/AblyChatTests/Helpers/Helpers.swift (2)
hasCode(10-26)hasRecord(120-131)
Sources/AblyChat/PaginatedResult.swift (1)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(370-372)
Sources/AblyChat/InternalError.swift (3)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (2)
attach(94-102)detach(108-116)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (2)
attach(351-365)detach(367-381)Sources/AblyChat/Room.swift (2)
attach(377-379)detach(382-384)
Sources/AblyChat/ChatAPI.swift (3)
Sources/AblyChat/InternalError.swift (2)
toErrorInfo(123-125)toErrorInfo(370-372)Tests/AblyChatTests/Mocks/MockRealtime.swift (1)
request(29-45)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (1)
request(148-169)
Sources/AblyChat/RoomLifecycleManager.swift (1)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (1)
waitToBeAbleToPerformPresenceOperations(60-69)
⏰ 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, tvOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: Xcode, macOS (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) - GitHub Check: SPM,
releaseconfiguration (Xcode 26.0) - GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: SPM (Xcode 26.0)
🔇 Additional comments (13)
Tests/AblyChatTests/ChatAPITests.swift (1)
24-29: LGTM!The error matching pattern correctly uses the new
InternalError.noItemInResponsevariant, aligning with the broader error refactoring in this PR.Sources/AblyChat/Rooms.swift (1)
170-174: LGTM!The new
roomExistsWithDifferentOptionserror variant provides better debugging information by including both the requested and existing options, and correctly converts toErrorInfoviatoErrorInfo().Tests/AblyChatTests/DefaultRoomsTests.swift (2)
180-180: LGTM!Test assertions correctly verify the new
roomExistsWithDifferentOptionserror code, aligning with the spec changes.Also applies to: 221-221
373-373: LGTM!Test assertion correctly verifies the
roomReleasedBeforeOperationCompletederror code.Sources/AblyChat/JSONCodable.swift (1)
33-33: LGTM!Adding the
typeparameter tofailedToDecodeFromRawValueprovides better error diagnostics by identifying which type failed to decode. The throw site at line 296 correctly passesT.self.Also applies to: 296-296
Tests/AblyChatTests/MessageTests.swift (2)
9-9: LGTM!Spec tag updates (CHA-M11a→CHA-M11h, CHA-M11b→CHA-M11i) and error code changes (40000→40003) correctly align with the specification changes. The new
invalidArgumentcode (40003) more accurately represents validation errors than the previousbadRequestcode (40000).Also applies to: 59-59, 63-63, 112-112
353-353: LGTM!Spec tag updates to CHA-M11j and error code change to 40003 (
invalidArgument) align with the specification refactoring.Also applies to: 393-393, 434-434
Sources/AblyChat/ChatAPI.swift (2)
141-143: LGTM!The updated error handling correctly uses the new
InternalErrorvariants (sendMessageReactionEmptyMessageSerialanddeleteMessageReactionEmptyMessageSerial) and aligns with the spec's requirement to throwInvalidArgumenterrors for empty message serials.Also applies to: 159-161
196-196: LGTM!The parameter rename from
urltopathmore accurately reflects REST path semantics. Including the path in thenoItemInResponseerror (line 212) provides better debugging context.Also applies to: 209-209, 212-212
Tests/AblyChatTests/DefaultPresenceTests.swift (1)
77-79: LGTM on presence wait and error alignmentCalls switched to the parameterless
waitToBeAbleToPerformPresenceOperations()and assertions now target.roomInInvalidStatewith appropriate causes. Matches the new InternalError variants and mock recording signature.Also applies to: 88-88, 105-111, 118-118, 134-135, 185-186, 194-194, 212-218, 225-225, 241-242, 330-331, 346-347, 369-371, 379-379, 397-403
Sources/AblyChat/RoomLifecycleManager.swift (3)
395-399: Attach invalid-state mapping aligned with specUsing
roomIsReleasing(operation: .attach)androomInInvalidStateForAttach(roomStatus)is clearer and spec-aligned.
458-462: Detach invalid-state mapping aligned with specSymmetric handling with
roomIsReleasing(operation: .detach)androomInInvalidStateForDetach(roomStatus)looks good.
609-621: Presence wait: correct error propagation on non-attached transitionThrowing
roomTransitionedToInvalidStateForPresenceOperation(newState:cause:)andpresenceOperationRequiresRoomAttachmatches the tests and spec points.
2 participants
Aligns error codes and messages with ably/specification#394. See commit messages for more details.
We still have some usage of the 40000
BadRequestcode for errors not specified by the spec, violating CHA-GP5; will revisit these errors separately in #438.Resolves #426, resolves #427.
Summary by CodeRabbit
Bug Fixes
Chores