Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ECO-4974] Throw ARTErrorInfo from public API #234

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[ECO-4974] Throw ARTErrorInfo from public API #234

wants to merge 6 commits into from
+1,078 −686

Conversation

lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Mar 12, 2025 •
edited by coderabbitai bot
Loading

This updates our public API to use typed throws, and specifically to always throw ARTErrorInfo. This makes us consistent with ably-cocoa, and allows users to access the error's statusCode to make decisions about whether they can retry the action that caused the error.

See commit messages for more details.

Resolves #43.

Summary by CodeRabbit

  • Documentation

    • Revised guidelines for error handling with clear, typed error practices.

  • Refactor

    • Enhanced asynchronous methods to provide more precise and consistent error feedback.
    • Improved concurrency support by updating async/await patterns across the SDK.

  • Tests

    • Updated error expectations and assertions in test suites for increased reliability.

@coderabbitai coderabbitai
Copy link

coderabbitai bot commented Mar 12, 2025 •
edited
Loading

Walkthrough

This pull request refines error handling throughout the Chat SDK. It adds a new “Throwing errors” section to the documentation and updates numerous asynchronous method signatures to explicitly throw either an InternalError or ARTErrorInfo. A dedicated InternalError.swift file is introduced to unify error conversion. Additionally, helper extensions and mapping functions are enhanced, while test cases and mocks are adjusted to accommodate the new error types.

Changes

File(s) Change Summary
CONTRIBUTING.md Added a "Throwing errors" section that describes guidelines for using typed throws and handling limitations in Swift (e.g. with Task, CheckedContinuation, etc.).
…/AblyCocoaExtensions/Ably+Concurrency.swift
…/ChatAPI.swift
…/Message.swift
…/Messages.swift
…/JSONCodable.swift
…/Headers.swift
…/PresenceDataDTO.swift		 Updated asynchronous API methods to use explicit error types (InternalError and ARTErrorInfo). Introduced a new async method (requestAsync) and refined JSON decoding error conversion.
…/DefaultMessages.swift
…/DefaultOccupancy.swift
…/DefaultPresence.swift
…/DefaultTyping.swift
…/Typing.swift
…/Presence.swift		 Modified presence, messaging, and typing operations to throw specified error types with improved error conversion and structured handling in do-catch blocks and continuations.
…/DefaultRoomLifecycleContributor.swift
…/Room.swift
…/RoomFeature.swift
…/RoomLifecycleManager.swift
…/Rooms.swift		 Enhanced room lifecycle methods by updating attach/detach signatures and room creation flows to throw InternalError or ARTErrorInfo, including error conversion using helper methods.
…/DefaultRoomReactions.swift
…/RoomReactionDTO.swift
…/RoomReactions.swift
…/PaginatedResult.swift
…/Occupancy.swift		 Revised reaction and paginated result handling with enhanced error typing and conversion in closure mapping and asynchronous pagination.
…/InternalError.swift
…/Dictionary+Extensions.swift
…/Errors.swift		 Introduced a centralized InternalError enum for unified error management, added helper extensions to convert errors to ARTErrorInfo, and implemented typed mapping in dictionaries.
Tests and Mocks (all files under Tests/AblyChatTests/… and Example/AblyChatExample/Mocks/…) Adjusted test cases and mock implementations to expect the new error types. Updated assertions and error matching logic (using helper functions such as isInternalErrorWrappingErrorInfo and isInternalErrorWithCase) to reflect the changes.

Sequence Diagram(s)
Loading 
sequenceDiagram
    participant C as Client
    participant API as Chat API Method
    participant IC as InternalError Converter
    participant PA as Public API Layer
    C->>API: Call async method (e.g. sendMessage)
    API->>API: Execute with async/continuation
    alt An error occurs
        API->>IC: Convert caught error → InternalError
        IC-->>API: Return InternalError
        API->>PA: Convert InternalError to ARTErrorInfo for public API
        PA-->>C: Throw ARTErrorInfo
    else Success
        API-->>C: Return result
    end

Possibly related PRs

  • [ECO-5170] Improve API and internals for presence data #189: The changes in the main PR, which focus on enhancing error handling and documentation for the public API, are related to the retrieved PR, as both involve modifications to the handling of presence data and error types in the SDK, particularly in the context of methods that manage presence operations.
  • [ECO-4982] Integrate lifecycle manager into existing room operations #106: The changes in the main PR focus on enhancing error handling documentation and practices in the SDK, while the retrieved PR introduces a room lifecycle manager that modifies how room operations handle errors, specifically changing error types in method signatures; thus, they are related at the code level through their shared focus on error handling improvements.

Poem

I'm a little rabbit, hopping in the code,
Finding bugs and errors hidden in a node.
Now each throw is precise, each error well-defined,
With InternalError and ARTErrorInfo perfectly aligned.
Hop on, dear coder, let our error-free paths be twined! 🐇✨

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot temporarily deployed to staging/pull/234/AblyChat March 12, 2025 19:17 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/234/AblyChat March 12, 2025 19:20 Inactive
coderabbitai[bot]
coderabbitai bot reviewed Mar 12, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🔭 Outside diff range comments (1)
Sources/AblyChat/DefaultPresence.swift (1)

267-295: 🛠️ Refactor suggestion

Robust decoding of presence messages.
processPresenceGet (lines 267-295) handles multiple possible missing fields (clientID, timestamp, data). The approach is well-structured. Consider grouping the repeated guard statements under a single descriptive error method to keep code DRY and to unify the error response, especially if more presence fields are introduced.

🧹 Nitpick comments (17)
CONTRIBUTING.md (1)

43-43: Minor grammatical improvement needed

There's a small grammatical issue in this line.

-  - It is not currently possible to create a `Task`, `CheckedContinuation`, or `AsyncThrowingStream` with a specific error type. You will need to instead return a `Result` and then call its `.get()` method.
+  - It is not currently possible to create a `Task`, `CheckedContinuation`, or `AsyncThrowingStream` with a specific error type. You will need to instead return a `Result` and then call its `.get()` method.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~43-~43: “its” (belonging to it) seems less likely than “it”
Context: ...instead return a Result and then call its .get() method. - `Dictionary.mapVal...

(AI_HYDRA_LEO_CPT_ITS_IT)

Sources/AblyChat/Rooms.swift (1)

154-253: Consistent internal do throws(InternalError) enclosure
By first catching errors as InternalError and then converting to ARTErrorInfo only at the API boundary, you ensure more precise error handling logic inside the method. The stepwise approach (guarding existing room state vs. waiting for release) is well-structured, though it’s quite verbose. If the logic expands further, consider extracting subroutines to enhance readability.

Sources/AblyChat/Room.swift (1)

232-232: Suggest refined error code for clarity.
While wrapping an ARTErrorInfo into an InternalError is valid, consider using a more descriptive or unique error code than 40000 for greater clarity.

Sources/AblyChat/PaginatedResult.swift (3)

10-12: Consider converting these properties into async/throwing methods.
Defining next, first, and current as computed properties that are async throws is unusual and may reduce clarity, since properties ordinarily imply trivial access. Converting them to methods (e.g. func next() async throws -> (any PaginatedResult<T>)?) can make the asynchronous or error-prone nature more explicit.

25-30: Clarify the noErrorWithInvalidResponse case.
Using .failure(PaginatedResultError.noErrorWithInvalidResponse.toInternalError()) is fine, but the name can be confusing. Consider naming it something like invalidResponse if that is the true meaning.

45-46: Use a common error type or expand this enum.
Since PaginatedResultError currently contains a single case, you could either rename it to better reflect the scenario (e.g. InvalidResponseError) or consolidate it into an existing error enum if appropriate.

Sources/AblyChat/DefaultMessages.swift (2)

117-125: Potential repeated throw in error handling path.
Inside the catch block (lines 117-125), you rethrow the same error after logging it. This is fine, but if you plan to unify error types, consider converting to a single typed error on first throw. Right now, some errors get converted to ARTErrorInfo while others pass through as-is.

160-190: Repeated do/catch code for REST calls.
The methods get, send, update, and delete (lines 160-190) follow a repetitive pattern: a do block that calls chatAPI.* and throws error.toARTErrorInfo(). Consider factoring this pattern into a helper function to reduce code duplication.

- do {
-   return try await chatAPI.getMessages(roomId: roomID, params: options)
- } catch {
-   throw error.toARTErrorInfo()
- }
+ return try await convertError {
+   try await chatAPI.getMessages(roomId: roomID, params: options)
+ }
Sources/AblyChat/ChatAPI.swift (2)

22-26: Consider using a safer conversion for numbers.
The initializer for SendMessageResponse (lines 22-25) forcibly converts createdAt using jsonObject.numberValueForKey("createdAt"). If the server returns a floating value or an unexpected numeric type, it might cause runtime issues. Consider safe-casting or providing a fallback.

134-170: Delete message request with partial body.
deleteMessage (lines 134-170) conditionally adds description and metadata to the request body. If a future logic introduces additional optional fields, consider centralizing the body-building logic in a single function. This helps reduce code duplication across message operations.

Sources/AblyChat/RoomLifecycleManager.swift (4)

590-600: Avoid indefinite continuation in OperationResultContinuations.
While storing continuations by operation ID is straightforward, carefully handle any edge cases where an operation might complete exceptionally or never complete. If a continuation is never removed, it can lead to memory leaks or indefinite hangs.

655-656: Recommend structured concurrency for waiting logic.
waitForCompletionOfOperationWithID uses an ad-hoc continuation approach. While valid, consider adopting Swift’s new concurrency patterns (e.g., AsyncStream) for operation coordination. This can simplify code and reduce the risk of continuation misuse.

865-897: Check for stale references upon detach.
In performDetachOperation, if the manager is deallocated or the contributor references become stale mid-cycle, you might end up with a partial operation. You do handle repeated detach attempts for non-failed contributors, but ensure no concurrency drift breaks the final state.

1220-1252: Wait logic for presence operations can be canceled.
waitToBeAbleToPerformPresenceOperations (lines 1220-1252) uses subscription to wait for status changes. If the task is canceled, the next status change might never be handled. Consider adding a short-circuit or Task.isCancelled check in your loop to avoid waiting indefinitely in canceled tasks.

Sources/AblyChat/DefaultPresence.swift (3)

72-97: isUserPresent concurrency approach.
isUserPresent lines 72-97 also calls waitToBeAbleToPerformPresenceOperations before calling channel.presence.getAsync(...). This is correct, but be sure your concurrency usage doesn’t cause race conditions if presence changes while you’re waiting.

167-199: Leaving presence with typed throws.
leave(optionalData:) (lines 167-199) replicates the same approach as enter. Code duplication is minimal but might be further simplified if you wrap the logic in a shared helper since it’s nearly identical—only the presence action differs.

252-257: Add more descriptive error message.
When presenceData is nil at line 254, you throw a generic "Received incoming message without data". Improve clarity by specifying that the presence data was unexpectedly nil to help debugging.

- let error = ARTErrorInfo.create(withCode: 50000, status: 500, message: "Received incoming message without data")
+ let error = ARTErrorInfo.create(withCode: 50000, status: 500, message: "Received nil presence data in message")
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a9dbd86 and dba13ae.

📒 Files selected for processing (42)
  • CONTRIBUTING.md (1 hunks)
  • Sources/AblyChat/AblyCocoaExtensions/Ably+Concurrency.swift (1 hunks)
  • Sources/AblyChat/ChatAPI.swift (7 hunks)
  • Sources/AblyChat/DefaultMessages.swift (7 hunks)
  • Sources/AblyChat/DefaultOccupancy.swift (1 hunks)
  • Sources/AblyChat/DefaultPresence.swift (5 hunks)
  • Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1 hunks)
  • Sources/AblyChat/DefaultRoomReactions.swift (1 hunks)
  • Sources/AblyChat/DefaultTyping.swift (3 hunks)
  • Sources/AblyChat/Errors.swift (4 hunks)
  • Sources/AblyChat/Extensions/Dictionary+Extensions.swift (1 hunks)
  • Sources/AblyChat/Headers.swift (3 hunks)
  • Sources/AblyChat/InternalError.swift (1 hunks)
  • Sources/AblyChat/JSONCodable.swift (22 hunks)
  • Sources/AblyChat/Message.swift (2 hunks)
  • Sources/AblyChat/Messages.swift (6 hunks)
  • Sources/AblyChat/Occupancy.swift (2 hunks)
  • Sources/AblyChat/PaginatedResult.swift (3 hunks)
  • Sources/AblyChat/Presence.swift (7 hunks)
  • Sources/AblyChat/PresenceDataDTO.swift (1 hunks)
  • Sources/AblyChat/Room.swift (4 hunks)
  • Sources/AblyChat/RoomFeature.swift (2 hunks)
  • Sources/AblyChat/RoomLifecycleManager.swift (17 hunks)
  • Sources/AblyChat/RoomReactionDTO.swift (2 hunks)
  • Sources/AblyChat/RoomReactions.swift (1 hunks)
  • Sources/AblyChat/Rooms.swift (6 hunks)
  • Sources/AblyChat/Typing.swift (2 hunks)
  • Tests/AblyChatTests/ChatAPITests.swift (1 hunks)
  • Tests/AblyChatTests/DefaultMessagesTests.swift (3 hunks)
  • Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (1 hunks)
  • Tests/AblyChatTests/DefaultRoomPresenceTests.swift (6 hunks)
  • Tests/AblyChatTests/DefaultRoomTypingTests.swift (2 hunks)
  • Tests/AblyChatTests/DefaultRoomsTests.swift (2 hunks)
  • Tests/AblyChatTests/Helpers/Helpers.swift (2 hunks)
  • Tests/AblyChatTests/InternalErrorTests.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRoom.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomFactory.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2 hunks)
  • Tests/AblyChatTests/PresenceDataDTOTests.swift (1 hunks)
  • Tests/AblyChatTests/RoomReactionDTOTests.swift (2 hunks)
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md

[uncategorized] ~43-~43: “its” (belonging to it) seems less likely than “it”
Context: ...instead return a Result and then call its .get() method. - `Dictionary.mapVal...

(AI_HYDRA_LEO_CPT_ITS_IT)

[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...ify the type of the thrown error, like: do throws(InternalError) { … }. - The compiler will never infer the t...

(UNLIKELY_OPENING_PUNCTUATION)

[uncategorized] ~46-~46: Loose punctuation mark.
Context: ...ill need to specify this yourself; e.g. let items = try jsonValues.map { jsonValue throws(InternalError) in … }. ### Testing guidelines #### Exposin...

(UNLIKELY_OPENING_PUNCTUATION)

⏰ Context from checks skipped due to timeout of 90000ms (10)
  • GitHub Check: Example app, tvOS (Xcode 16)
  • GitHub Check: Example app, iOS (Xcode 16)
  • GitHub Check: Example app, macOS (Xcode 16)
  • GitHub Check: Xcode, tvOS (Xcode 16)
  • GitHub Check: Xcode, release configuration, tvOS (Xcode 16)
  • GitHub Check: Xcode, iOS (Xcode 16)
  • GitHub Check: Xcode, release configuration, iOS (Xcode 16)
  • GitHub Check: Xcode, macOS (Xcode 16)
  • GitHub Check: Xcode, release configuration, macOS (Xcode 16)
  • GitHub Check: SPM (Xcode 16)
🔇 Additional comments (113)
Sources/AblyChat/PresenceDataDTO.swift (1)

13-13: Appropriate removal of throws keyword

The removal of the throws keyword from this initializer simplifies the API since this simple assignment operation doesn't need error propagation. This aligns with the broader effort to standardize error handling across the codebase.

Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (1)

2194-2194: Appropriately broadened error type to accommodate new error handling

Changing the error type from ARTErrorInfo? to Error? correctly adapts the test to the updated error handling mechanism. This allows the test to catch any error type thrown by the implementation, which is necessary since the SDK is standardizing on different error types for different layers.

Tests/AblyChatTests/ChatAPITests.swift (1)

23-27: Correctly updated error checking logic

The modification to use pattern matching with InternalError correctly adapts the test to the new error handling mechanism. This update properly verifies that errors are now wrapped in the InternalError type, which is consistent with the SDK's standardized approach to error handling.

Sources/AblyChat/RoomReactions.swift (1)

17-17: Improved API clarity with typed throws

This change implements the PR objective by specifying that the method throws ARTErrorInfo rather than a generic error. This provides better API clarity for users, allowing them to make more informed decisions about error handling, particularly for determining whether operations can be retried based on the error's status code.

Tests/AblyChatTests/PresenceDataDTOTests.swift (1)

21-25: Improved error validation pattern.

The test now uses a closure pattern to capture and validate the error type using isInternalErrorWithCase, which is more flexible than directly checking for a specific error type. This aligns with the PR's objective to standardize on typed error handling across the SDK.

Sources/AblyChat/DefaultRoomReactions.swift (1)

24-24: Function signature updated to use typed throws.

The method now explicitly specifies that it throws ARTErrorInfo instead of a generic error, which aligns with the PR objective to standardize error handling in the public API. This change improves API clarity by communicating the exact error type that callers should expect to handle.

Sources/AblyChat/Headers.swift (3)

1-2: Added Ably import.

Added import for the Ably module, which is likely needed for the error handling modifications in this file.

78-78: Updated initializer to use typed throws.

The initializer now explicitly declares that it throws InternalError instead of a generic error, improving type safety and API clarity.

89-89: Modified error transformation pattern.

The error is now converted to an InternalError using the toInternalError() extension method before being thrown. This ensures consistency with the updated error handling approach across the SDK.

Sources/AblyChat/Errors.swift (4)

215-215: Added new case for handling internal errors without ARTErrorInfo.

The new nonErrorInfoInternalError case allows for representing internal errors that don't have an associated ARTErrorInfo, which enhances the error handling system's flexibility.

228-231: Added error status code handling for internal errors.

Internal errors without an ARTErrorInfo are now treated as non-recoverable user errors with a .badRequest status code. This provides a consistent approach to handling these errors in the public API.

314-317: Added localized description for internal errors.

The implementation provides a simple string representation of the internal error enum case. This ensures that all errors have descriptive messages for debugging and user feedback.

347-348: Updated error cases without a cause.

Added the new nonErrorInfoInternalError case to the list of errors that don't have an underlying cause, maintaining consistency in the error handling implementation.

Sources/AblyChat/Extensions/Dictionary+Extensions.swift (1)

1-8: Good implementation of typed error handling for dictionary transformations.

This new utility method ablyChat_mapValuesWithTypedThrow properly preserves the error type thrown by the transform function, allowing for more precise error handling compared to Dictionary.mapValues which uses rethrows.

The implementation correctly preserves key uniqueness and propagates the typed error, which aligns with the PR objective of standardizing error types throughout the API.

Tests/AblyChatTests/DefaultRoomTypingTests.swift (2)

83-84: Good use of typed throws syntax for error handling.

Changed the do block to use do throws(ARTErrorInfo) to explicitly specify the expected error type. This change removes the need for explicit casting of the error to ARTErrorInfo later in the catch block, making the code cleaner while maintaining type safety.

This aligns well with the PR objective of standardizing error types across the codebase.

108-109: Consistent application of typed throws pattern.

Similar to the previous change, this modification uses do throws(ARTErrorInfo) to specify the expected error type, ensuring consistent error handling across test methods.

This approach follows the typed throws pattern being applied throughout the codebase.

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

16-16: Updated method signature with typed throws to specify InternalError.

Modified the method signature to use throws(InternalError) instead of generic throws, which makes the error type explicit. This change supports the PR objective of standardizing error types throughout the codebase.

The implementation correctly specifies that the createRoom method can throw an InternalError, providing more precise error handling information to callers.

Tests/AblyChatTests/RoomReactionDTOTests.swift (3)

10-14: Good update to use structured error handling with isInternalErrorWithCase

The updated test now properly checks that the error is an InternalError with the specific case of .jsonValueDecodingError, aligning with the changes made to standardize error types in the library.

19-23: Consistent error verification approach

This follows the same pattern of verifying the specific InternalError case, maintaining consistency with the other test updates.

73-77: Aligned error handling pattern for Extras tests

The test for RoomReactionDTO.Extras now follows the same structured error verification pattern, creating consistency across all test cases.

Tests/AblyChatTests/InternalErrorTests.swift (1)

1-25: Well-structured tests for error conversion

These tests properly validate the two key conversion paths for InternalError to ARTErrorInfo:

  1. When the underlying error is already an ARTErrorInfo, it should be returned directly
  2. When the underlying error is not an ARTErrorInfo, it should be properly wrapped

This implementation directly supports the PR objective of standardizing public API errors as ARTErrorInfo and ensures the conversion mechanism works correctly.

Tests/AblyChatTests/DefaultRoomPresenceTests.swift (2)

106-114: Updated to use typed throws in error handling

The test now explicitly declares the expected error type with throws(ARTErrorInfo), which aligns with the changes to standardize on typed throws in the public API.

131-138: Consistent error type specification across tests

All error handling blocks have been updated to use throws(ARTErrorInfo), ensuring consistency across the test suite and properly validating that the functions throw the expected error type.

Also applies to: 224-232, 249-256, 329-336, 399-407

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

1-1: Added necessary import for ARTErrorInfo

Added the Ably import which is required for the ARTErrorInfo type used in the method signatures.

47-47: Updated method signatures to use typed throws

The attach() and detach() methods now explicitly specify that they throw ARTErrorInfo, aligning with the PR goal of standardizing on typed throws in the public API.

Also applies to: 51-51

Sources/AblyChat/DefaultOccupancy.swift (1)

56-62: Method correctly updated to throw ARTErrorInfo

The get() method has been properly updated to explicitly throw ARTErrorInfo instead of a generic error, aligning with the PR's objective of standardizing error handling. The implementation effectively wraps the original code in a do-catch block and converts any caught errors to ARTErrorInfo using the toARTErrorInfo() method.

This change ensures that the error type is consistent with the updated protocol definition and allows clients to access additional error information like statusCode.

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

67-78: Properly updated attach() method to throw InternalError

The method signature has been correctly updated to throw InternalError instead of a generic error. The implementation now properly wraps the call to performBehavior() in a do-catch block and converts any caught errors to InternalError using the toInternalError() method.

This change maintains consistency with the rest of the codebase's error handling approach.

82-93: Properly updated detach() method to throw InternalError

Similar to the attach() method, the detach() method has been correctly updated to throw InternalError and includes proper error conversion in the do-catch block.

This change is consistent with the error handling approach being implemented across the codebase.

Sources/AblyChat/Occupancy.swift (2)

30-30: Protocol method signature correctly updated

The get() method signature in the Occupancy protocol has been properly updated to explicitly throw ARTErrorInfo. This change aligns with the PR objective of standardizing public API error types and matches the implementation in DefaultOccupancy.

This change ensures that clients will have access to the error's statusCode to help determine if they can retry actions that resulted in errors.

69-69: JSONObjectDecodable initializer correctly updated

The initializer signature has been properly updated to throw InternalError instead of a generic error. This change is consistent with the pattern of having internal methods throw InternalError while public API methods throw ARTErrorInfo.

This change contributes to the standardization of error types throughout the codebase.

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

25-34: Mock implementation correctly updated to throw InternalError

The waitToBeAbleToPerformPresenceOperations method has been properly updated to throw InternalError instead of ARTErrorInfo. The implementation now includes a do-catch block that correctly converts any caught errors to InternalError using the toInternalError() method.

This change maintains consistency with the rest of the error handling approach in the codebase and properly reflects the behavior of the interface it's implementing.

CONTRIBUTING.md (2)

37-46: Great addition of error handling guidelines

The documentation on typed throws is clear and comprehensive, offering valuable guidance on when to use ARTErrorInfo versus InternalError. The guidelines align well with the PR objective of ensuring consistent error typing.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~43-~43: “its” (belonging to it) seems less likely than “it”
Context: ...instead return a Result and then call its .get() method. - `Dictionary.mapVal...

(AI_HYDRA_LEO_CPT_ITS_IT)

[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...ify the type of the thrown error, like: do throws(InternalError) { … }. - The compiler will never infer the t...

(UNLIKELY_OPENING_PUNCTUATION)

[uncategorized] ~46-~46: Loose punctuation mark.
Context: ...ill need to specify this yourself; e.g. let items = try jsonValues.map { jsonValue throws(InternalError) in … }. ### Testing guidelines #### Exposin...

(UNLIKELY_OPENING_PUNCTUATION)

45-46: Documentation clearly explains Swift typing inference limitations

The explanation of error type inference limitations in do blocks and closures is excellent. It will help developers understand why they need to explicitly specify error types in certain scenarios.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...ify the type of the thrown error, like: do throws(InternalError) { … }. - The compiler will never infer the t...

(UNLIKELY_OPENING_PUNCTUATION)

[uncategorized] ~46-~46: Loose punctuation mark.
Context: ...ill need to specify this yourself; e.g. let items = try jsonValues.map { jsonValue throws(InternalError) in … }. ### Testing guidelines #### Exposin...

(UNLIKELY_OPENING_PUNCTUATION)

Sources/AblyChat/DefaultRoomLifecycleContributor.swift (2)

37-39: Updated error type aligns with internal error handling strategy

Changing from throws(ARTErrorInfo) to throws(InternalError) correctly implements the internal error handling strategy outlined in the contributing guidelines, where InternalError is used internally and converted to ARTErrorInfo at the public API boundary.

41-43: Updated error type aligns with internal error handling strategy

Similarly, updating the detach method to throw InternalError maintains consistency with the internal error handling approach.

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

5-18: Improved error checking logic for test helpers

The updated implementation correctly checks for both direct ARTErrorInfo errors and InternalError instances that wrap ARTErrorInfo. This enhancement aligns with the new error handling strategy and improves test reliability.

35-41: Helpful utility function for internal error testing

This new helper function provides a clean way to verify when an InternalError is wrapping a specific ARTErrorInfo, which will be useful for testing the error conversion logic mentioned in the contributing guidelines.

43-66: Well-structured error case enumeration

The addition of the Case enum provides a clear categorization of internal error types, making tests more readable and maintainable. The enumCase property offers a convenient way to check error types without complex pattern matching.

68-74: Useful utility for case-based error testing

This function complements the enumeration by providing a simple way to check if an error is of a specific case type, which will streamline test assertions.

Sources/AblyChat/Typing.swift (3)

30-30: Public API now uses typed throws with ARTErrorInfo

The change to throws(ARTErrorInfo) correctly implements the requirements specified in the contributing guidelines, ensuring that the public API uses typed throws with ARTErrorInfo as the error type.

42-42: Public API now uses typed throws with ARTErrorInfo

The start() method now correctly specifies ARTErrorInfo as its thrown error type, aligning with the PR objective.

50-50: Public API now uses typed throws with ARTErrorInfo

The stop() method now correctly specifies ARTErrorInfo as its thrown error type, aligning with the PR objective.

Sources/AblyChat/RoomReactionDTO.swift (2)

43-43: Good use of typed throws

The initializer now explicitly declares that it throws InternalError, which aligns with the PR objective to make error handling more specific and consistent across the codebase.

61-64: Improved error handling with typed throws

The implementation now uses ablyChat_mapValuesWithTypedThrow to explicitly handle potential errors with the InternalError type during JSON parsing. This is a good pattern that increases type safety and makes error handling more predictable.

Tests/AblyChatTests/DefaultMessagesTests.swift (3)

61-67: Temporary workaround for compiler crash

The TODO comment explains that this is a temporary solution to work around a compiler crash. Make sure to track issue #233 so this code can be reverted once Xcode 16.3 is released.

Could you confirm if there's a plan to monitor the release of Xcode 16.3 so these workarounds can be removed promptly?

273-277: Improved error validation approach

The test now uses a more specific validation for the thrown error, checking if it's an InternalError wrapping an ARTErrorInfo. This aligns with the PR objective to ensure proper error types are thrown from the API.

318-324:

✅ Verification successful

Same compiler crash workaround

This is the same workaround pattern as seen earlier in the file. The implementation maintains the test's intent while working around the compiler limitation.

🏁 Script executed:

#!/bin/bash
# Check if there are other instances of this workaround pattern that might need to be reverted later
rg --type swift "TODO: avoids compiler crash.*revert once Xcode 16\.3 released" -B 1 -A 5

Length of output: 2579

Compiler Crash Workaround Validated Across Test Files

The workaround pattern in the tests (as seen in both DefaultMessagesTests.swift and DefaultRoomsTests.swift) consistently applies the necessary temporary fix for the Xcode compiler crash. The tests preserve their intended behavior, and the pattern is uniformly applied. Please ensure that when Xcode 16.3 is released, these workarounds are reverted as noted in the TODO comments.

Sources/AblyChat/RoomFeature.swift (2)

73-73: Implementation correctly updated

The implementation in DefaultFeatureChannel has been properly updated to match the protocol change, maintaining consistency in error handling.

61-61:

✅ Verification successful

Protocol method now throws InternalError

The method signature has been updated to throw InternalError instead of ARTErrorInfo. This aligns with the PR objective but make sure all implementers of this protocol have been updated accordingly.

🏁 Script executed:

#!/bin/bash
# Check for implementations of waitToBeAbleToPerformPresenceOperations to ensure they've been updated
rg --type swift "func waitToBeAbleToPerformPresenceOperations.*\(" -A 1

Length of output: 2928

Verified: Protocol Method Signature Update Confirmed

The change to throw InternalError instead of ARTErrorInfo has been successfully propagated. All relevant implementations (including those in Sources and Tests) are updated accordingly:

  • Sources/AblyChat/RoomFeature.swift – Signature now correctly uses InternalError.
  • Sources/AblyChat/RoomLifecycleManager.swift – Updated signature in both protocol and its internal implementation.
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift and MockFeatureChannel.swift – Mocks now reflect the updated method signature.
  • Additional tests in DefaultRoomLifecycleManagerTests.swift confirm consistent usage across scenarios.

No further changes are required.

Sources/AblyChat/Message.swift (3)

155-155: Good use of typed throws

The initializer now explicitly declares that it throws InternalError, which aligns with the PR objective to make error handling more specific.

165-167: Improved error handling with typed throws

The implementation now uses ablyChat_mapValuesWithTypedThrow to handle errors with the specific InternalError type when parsing JSON values. This improves type safety and error handling predictability.

170-170: Consistent typed throws in operation mapping

The operation mapping closure now explicitly throws InternalError, maintaining consistency with the other error handling improvements in this file.

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

19-29: Good use of typed throws in performAttachOperation
The typed throws with InternalError cleanly encapsulate internal Ably errors (ARTErrorInfo). The logic for rethrowing via error.toInternalError() aligns well with the new error handling approach.

31-41: Consistent InternalError usage in performDetachOperation
This method follows the same pattern as performAttachOperation, ensuring uniform error propagation.

Sources/AblyChat/InternalError.swift (3)

1-16: Well-structured definition of InternalError
Defining a dedicated internal error enum reduces ambiguity and provides a clear mapping from ARTErrorInfo to internal and public-facing errors.

17-31: toARTErrorInfo() and message property provide smooth error translation
These functions uniformly translate internal errors into ARTErrorInfo and facilitate consistent logging. Good clarity in bridging internal and external error layers.

33-61: Extensions streamline conversions to InternalError
Each extension method clearly wraps domain-specific error types for unified handling. This modular approach helps maintain consistency as the SDK evolves.

Sources/AblyChat/Rooms.swift (5)

28-28: Typed throws on get(...): async throws(ARTErrorInfo)
Exposing ARTErrorInfo at the public API boundary meets the PR objective to consistently throw typed Ably errors.

256-260: Clear final conversion to ARTErrorInfo
Catching InternalError and rethrowing as ARTErrorInfo satisfies the typed error requirement. Good job ensuring that bridging logic is at the end.

263-283: Helper functions for creation-failure signaling
The makeCreationFailureFunctions approach is creative, providing structured ways to inject errors into in-flight async tasks. This design is flexible, though it’s somewhat intricate. Consider adding unit tests that ensure all code paths for success/failure are covered.

Would you like a sample test snippet or script to verify coverage of these code paths?

294-299: createRoom(...) modernization
Shifting to async throws(InternalError) is consistent with the rest of the refactor. The method body is straightforward and easy to follow.

335-335: Graceful logging for release-induced failure
Transforming the “room released” scenario into an InternalError clarifies the reason for creation failure. Adequate debug logs help with diagnosis.

Sources/AblyChat/Room.swift (7)

91-91: Public API typed throws alignment looks good.
The new func attach() async throws(ARTErrorInfo) signature aligns well with the PR objective of exposing ARTErrorInfo from the public API.

98-98: Consistent typed throws for detaching.
The func detach() async throws(ARTErrorInfo) signature is consistent with the typed-throw approach used in attach().

142-142: Internal error type usage is acceptable.
Using InternalError in createRoom at an internal protocol level is fine, as these errors are not directly exposed through the public interface.

148-148: Continuation of internal error typing.
This factory method’s async throws(InternalError) signature is consistent with the rest of the internal error handling approach.

224-224: Initialization error handling.
The constructor’s async throws(InternalError) signature cleanly distinguishes internal errors during DefaultRoom creation.

397-402: Rethrowing as ARTErrorInfo is correct.
This attach() implementation properly catches lower-level errors and rethrows them as ARTErrorInfo, matching the public API contract.

405-410: Consistent detach code.
Similar to attach(), the detach() method correctly wraps and rethrows errors as ARTErrorInfo.

Sources/AblyChat/Messages.swift (7)

18-18: Typed throws for subscribe(bufferingPolicy:).
Changing the signature to async throws(ARTErrorInfo) is consistent with the new approach of exposing ARTErrorInfo at the public API level.

23-23: Additive typed throws in default subscribe().
Exposing ARTErrorInfo here as well ensures consistency with the buffering-policy overload.

33-33: Query method error type.
Enabling get(options:) to throw ARTErrorInfo clarifies error details for message history retrieval.

47-47: Enhanced error detail for sending messages.
Using ARTErrorInfo in send(params:) improves transparency around send-failure causes.

63-63: Specific typed throw for message updates.
update(newMessage:description:metadata:) now explicitly throwing ARTErrorInfo matches the broader error-handling improvements.

78-78: Explicit error type for message deletion.
delete(message:params:) adopting ARTErrorInfo consistently completes the suite of typed throws for message operations.

89-89: Unified subscription convenience.
The default subscribe() method’s async throws(ARTErrorInfo) complements the buffering-policy-based overload cleanly.

Sources/AblyChat/AblyCocoaExtensions/Ably+Concurrency.swift (9)

6-29: Bridge from callback-based requests to Swift concurrency.
The internal requestAsync method elegantly wraps the callback in withCheckedContinuation, properly rethrowing errors as InternalError.

32-46: Asynchronous attach extension.
attachAsync() correctly maps the callback-based attach flow to async/await, rethrowing ARTErrorInfo as InternalError for internal usage.

48-62: Asynchronous detach extension.
detachAsync() follows the same pattern as attachAsync(), ensuring consistent error wrapping.

65-72: Concurrent-safe presence message struct.
Defining PresenceMessage separately with only required fields is a clean, sendable-friendly approach.

74-85: Initializer bridging to PresenceMessage.
Reading fields from ARTPresenceMessage and converting them to PresenceMessage is straightforward and correct.

88-105: Fetching presence members asynchronously.
getAsync() properly wraps presence retrieval in an async/await continuation, then rethrows as InternalError.

107-123: Query-based presence fetch.
Similarly, getAsync(_ query:) is well-implemented, paralleling getAsync() to retrieve presence with typed error handling.

125-139: Asynchronous presence leave.
leaveAsync(_:) bridging from callbacks to async is consistent, ensuring typed InternalError from underlying ARTErrorInfo.

141-155: Client-specific presence entry.
enterClientAsync parallels leaveAsync, maintaining consistency when bridging the callback flow.

Sources/AblyChat/PaginatedResult.swift (3)

19-19: Good use of typed Result with continuations.
Switching to CheckedContinuation<Result<PaginatedResultWrapper<Response>, InternalError>, Never> clarifies success/failure scenarios and aligns with typed error handling.

98-100: LGTM: Extension for conversion is well-structured.
The helper method toPaginatedResult(items:) cleanly encapsulates logic for converting low-level responses into a typed wrapper.

36-36:

✅ Verification successful

Validate Swift language version for advanced closure throws syntax.
try jsonValues.map { jsonValue throws(InternalError) in … } is valid in newer Swift versions; confirm that your minimum deployment environment supports this feature. Otherwise, you may need to map errors within a standard do … catch block.

🏁 Script executed:

#!/bin/bash
# Searching for Swift language version constraints in the project files
rg -A 3 "SWIFT_VERSION|swift-tools-version"

Length of output: 1770

Swift Language Version Confirmed

The project is correctly configured to use Swift 6.0 (as seen in both Package.swift and the Xcode project settings). This version supports the advanced closure throws(InternalError) syntax, so no changes are required.

  • Confirmed in Package.swift: // swift-tools-version: 6.0
  • Confirmed in Example/AblyChatExample.xcodeproj: SWIFT_VERSION = 6.0
Sources/AblyChat/DefaultTyping.swift (3)

122-161: Typed error conversion boundary is consistent.
Wrapping InternalError within a do throws(InternalError) block and converting it to ARTErrorInfo externally ensures the public API consistently exposes ARTErrorInfo. This design is aligned well with typed error handling.

164-191: Stop method concurrency approach looks consistent.
The concurrency approach for stop() uses the same pattern as start(). No major concerns unless multiple actors call stop() simultaneously, which your concurrency strategy may already handle.

198-210: Good approach to refining processPresenceGet.
This helper ensures each member has a clientId. Throwing an InternalError as soon as one is missing is a clean fail-fast mechanism.

Sources/AblyChat/Presence.swift (1)

15-15: Consistent usage of ARTErrorInfo in public methods.
All these methods now throw ARTErrorInfo, which aligns with the newly standardized typed throws approach. This improves error specificity and consistency throughout the presence API.

Also applies to: 27-27, 39-39, 49-49, 59-59, 69-69, 99-99, 107-107, 115-115

Sources/AblyChat/DefaultMessages.swift (3)

261-280: Clarify distinction between InternalError vs. ARTErrorInfo.
In resolveSubscriptionStart(...) (line 261) and serialOnChannelAttach(...) (line 280), you switch from throwing InternalError to ARTErrorInfo in some places. This is consistent with the rest of the typed-throws approach, but ensure all code paths use the final error type the caller expects. Thoroughly test edge cases (e.g., missing channelSerial) to confirm.

295-335: Check concurrency safety of withCheckedContinuation usage.
The withCheckedContinuation call (lines 295-335) properly resumes the continuation only once. However, if multiple channel events arrive in quick succession, ensure there's no race that might resume or mutate nillableContinuation prematurely. Consider using actor-isolated or async streams to handle potential concurrency complexities.

60-69:

✅ Verification successful

Handle concurrency within Task blocks carefully.
When subscribing to realtime messages (lines 60-69), each message callback spawns a Task that performs multiple checks and can throw ARTErrorInfo. This is correct usage of Swift concurrency, but be mindful that if self is deallocated or if channel is detached mid-callback, subsequent calls (like message.extras) could fail. Consider adding defensive checks if needed.

Do you want to verify if the object remains valid throughout the entire Task lifecycle? You could run a quick script to grep for possible mid-callback references:

🏁 Script executed:

#!/bin/bash
rg -A 5 "Task {"

Length of output: 94

Ensure Task Block’s Object Validity Remains Intact
In the realtime message subscription (lines 60–69 of Sources/AblyChat/DefaultMessages.swift), the usage of Task { … } follows Swift concurrency patterns and is acceptable. However, please verify that any objects referenced within the Task’s closure—such as self or channel—are still valid for the entire Task lifecycle. This is important because if, for example, self is deallocated or the channel is detached before the Task completes, subsequent property accesses (like message.extras, if they occur) might fail. Consider adding defensive checks (or adjusting capture semantics, such as using [weak self]) if these objects’ lifetimes are uncertain.

🏁 Script executed:

#!/bin/bash
# Use fixed-string search to avoid regex errors when looking for "Task {" in the file
rg -F -A 5 "Task {" Sources/AblyChat/DefaultMessages.swift

Length of output: 1284

Concurrency Handling Validated: No Further Changes Needed

The verification shows that the Task blocks in Sources/AblyChat/DefaultMessages.swift are using Swift concurrency correctly. In the realtime message subscription block (lines 60–69), there isn’t an unnecessary capture of self—and where self is needed (such as within the MainActor block or during attachment handling), the code already leverages [weak self] or optional chaining. This defensive approach adequately prevents issues due to object deallocation or detached channels during the Task’s lifecycle.

  • Realtime message subscription Task: Does not capture self unnecessarily.
  • Subsequent Task blocks: Correctly use [weak self] and safe optional chaining when referencing instance members.
  • General recommendation: Continue monitoring if new references are added mid-callback, but no changes are required at this time.
Sources/AblyChat/ChatAPI.swift (3)

13-16: Ensure paginated request usage is correct.
getMessages (lines 13-16) now returns a typed any PaginatedResult<Message> with throws(InternalError). Verify all call sites handle InternalError properly and confirm that your unwrapped JSON decoding doesn’t miss potential edge cases.

43-78: Consistent error transformation for ‘sendMessage’.
sendMessage now throws InternalError if clientId is nil or a network error occurs. This is good for typed error consistency. Just ensure that the top-level code using sendMessage can handle InternalError uniformly. No issues found with concurrency.

172-205: makePaginatedRequest concurrency check.
makePaginatedRequest (lines 195-205) uses requestAsync internally. This approach looks sound. Verify concurrency usage, especially if other tasks cancel or alter the realtime reference. Ensure the realtime.requestAsync call doesn’t get canceled mid-pipeline.

Sources/AblyChat/RoomLifecycleManager.swift (4)

44-50: Typed errors in lifecycle operations.
performAttachOperation and performDetachOperation now throw InternalError. This ensures alignment with typed error handling. Double-check that calling code properly awaits or handles these operations, as they can fail at runtime.

746-759: Handle potential race conditions around status changes.
In performAttachOperation (lines 746-759), if the channel state changes again while you wait, ensure you don’t incorrectly override or skip states. You do partial checks with status.operationID. Just confirm that concurrency or reentrancy can’t cause you to lose an important update.

790-817: Graceful fallback for suspended or failed states.
Inside performAttachmentCycle, suspended or failed states cause immediate exit from the loop with subsequent scheduling of a retry or rundown operation (lines 805-817). This design makes sense. Validate that any ephemeral state changes in contributorState after the error are safe to ignore.

971-972: Retain original error context when rethrowing.
After the detach loop, an error is rethrown at lines 971-972 using firstDetachError.toInternalError(). That preserves typed error consistency. Confirm the original error’s message and cause remain visible to end users.

Sources/AblyChat/DefaultPresence.swift (4)

22-45: Unified typed throws for ‘get()’.
Your get() function lines 22-45 now uses throws(ARTErrorInfo) externally while converting internal errors to InternalError first. This is consistent but watch for any new internal error types that never get converted to ARTErrorInfo, e.g., PresenceDataDTO decoding errors.

107-130: Converting presence operation errors.
For enter(optionalData:) (lines 107-130), you correctly catch any errors and rethrow them via .toARTErrorInfo(). This matches the typed throw approach. No immediate concerns, but continued coverage in tests will help ensure it’s robust.

142-165: Updating presence with fallback data.
In update(optionalData:) (lines 142-165), you build a PresenceDataDTO and then call presence.updateAsync. If data is nil, you pass an empty object. Confirm with product owners whether passing no data or an empty object is the intended default.

297-321: Prevent partial data in presence subscription events.
processPresenceSubscribe (lines 297-321) again decodes presence data. If clientId or timestamp is missing, you throw an error. This is consistent. Ensure that your documentation clarifies possible behaviors to library consumers if partial presence data arrives from unexpected sources.

Sources/AblyChat/JSONCodable.swift (7)

9-9: Good update to the protocol method signatures

Changing the init(jsonValue:) method to explicitly throw InternalError instead of a generic error aligns well with the PR objective of standardizing error types. This provides more type safety and clearer error contracts.

26-26: Consistent error type specification

The update to init(jsonObject:) to throw InternalError specifically maintains consistency with other method signatures in this file. This is a good practice for API clarity.

38-41: Good error handling implementation

Converting JSONValueDecodingError to InternalError using the toInternalError() method ensures consistent error types are propagated through the public API. This aligns with the PR objectives to standardize error handling.

58-68: Appropriate method signature update

The update to the objectValueForKey method signature ensures consistent error handling. The implementation correctly uses toInternalError() to convert domain-specific errors to the standard InternalError type.

73-87: Consistent error handling in optional methods

The error handling in optionalObjectValueForKey has been appropriately updated. The method now explicitly throws InternalError and uses toInternalError() for error conversion, maintaining consistency with the non-optional variant.

275-279: Good error handling for RawRepresentable values

The RawRepresentable handling methods have been properly updated to throw InternalError with consistent error conversion. This includes both the public methods and the private implementation method, which is thorough and maintains consistency.

Also applies to: 286-292, 294-300

1-301: Overall consistent implementation of the error handling strategy

All method signatures and error handling code have been consistently updated to use InternalError instead of generic errors. This systematic approach ensures a uniform error handling experience across the API, which aligns perfectly with the PR objectives of standardizing errors for better usability.

The changes also enable users to access specific error information like statusCode (as mentioned in the PR objectives) by propagating the more specific error type. This is a significant improvement for API consumers who need to make informed decisions about error recovery.

@github-actions github-actions bot temporarily deployed to staging/pull/234/AblyChat March 12, 2025 19:28 Inactive
coderabbitai[bot]
coderabbitai bot reviewed Mar 12, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (14)
Tests/AblyChatTests/InternalErrorTests.swift (1)

16-16: Consider consistent naming pattern for test methods

The first test method is named toARTErrorInfo_whenUnderlyingErrorIsARTErrorInfo, while this one has a test prefix: testToARTErrorInfo_whenUnderlyingErrorIsNotARTErrorInfo. For consistency, consider removing the test prefix.

-    func testToARTErrorInfo_whenUnderlyingErrorIsNotARTErrorInfo() {
+    func toARTErrorInfo_whenUnderlyingErrorIsNotARTErrorInfo() {
CONTRIBUTING.md (2)

43-43: Minor grammar correction in documentation

-  - It is not currently possible to create a `Task`, `CheckedContinuation`, or `AsyncThrowingStream` with a specific error type. You will need to instead return a `Result` and then call its `.get()` method.
+  - It is not currently possible to create a `Task`, `CheckedContinuation`, or `AsyncThrowingStream` with a specific error type. You will need to instead return a `Result` and then call its `.get()` method.

The possessive "its" is correct here, referring to the Result's .get() method.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~43-~43: “its” (belonging to it) seems less likely than “it”
Context: ...instead return a Result and then call its .get() method. - `Dictionary.mapVal...

(AI_HYDRA_LEO_CPT_ITS_IT)

45-46: Consider improving punctuation in code examples

There are loose punctuation marks before the code examples. Consider standardizing the approach to introducing code examples throughout the documentation.

-  - There are times when the compiler struggles to infer the type of the error thrown within a `do` block. In these cases, you can disable type inference for a `do` block and explicitly specify the type of the thrown error, like: `do throws(InternalError) { … }`.
-  - The compiler will never infer the type of the error thrown by a closure; you will need to specify this yourself; e.g. `let items = try jsonValues.map { jsonValue throws(InternalError) in … }`.
+  - There are times when the compiler struggles to infer the type of the error thrown within a `do` block. In these cases, you can disable type inference for a `do` block and explicitly specify the type of the thrown error, like `do throws(InternalError) { … }`.
+  - The compiler will never infer the type of the error thrown by a closure; you will need to specify this yourself, e.g., `let items = try jsonValues.map { jsonValue throws(InternalError) in … }`.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...ify the type of the thrown error, like: do throws(InternalError) { … }. - The compiler will never infer the t...

(UNLIKELY_OPENING_PUNCTUATION)

[uncategorized] ~46-~46: Loose punctuation mark.
Context: ...ill need to specify this yourself; e.g. let items = try jsonValues.map { jsonValue throws(InternalError) in … }. ### Testing guidelines #### Exposin...

(UNLIKELY_OPENING_PUNCTUATION)

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

43-66: Evaluate naming clarity for the nested Case enum.
The nested enum Case clarifies high-level error categories, which is good for typed error handling. However, consider giving it a more descriptive name (e.g., InternalErrorType) if you expect to expand it significantly.

Sources/AblyChat/InternalError.swift (3)

3-31: Ensure completeness of InternalError conversions.
The layout of .errorInfo(ARTErrorInfo) and .other(Other) is straightforward. If you plan on adding further specialized cases (beyond Other), confirm you won’t lose vital metadata if you rely on toARTErrorInfo() for external usage.

45-49: Minimal overhead with HeadersValue.JSONDecodingError bridging.
This extension is straightforward. If you plan to add more detail (e.g., original JSON snippet) to the error, consider augmenting the other(...) payload for richer debugging.

51-55: Maintain consistent error detail.
JSONValueDecodingError is mapped similarly to headersValueJSONDecodingError, which is good for uniformity. Consider standardizing error messages across these two for logs if they differ significantly in internal usage.

Sources/AblyChat/Rooms.swift (4)

104-108: Check concurrency race conditions in waitForRoom().
Awaiting creationTask.value is done in an actor, which is good. Be mindful of external cancellations (e.g., a user calling release(roomID:)), ensuring the canceled states are properly handled.

166-181: Appropriate fallback to waitForRoom.
Your fallback logic clarifies waiting on the existing map entry if the entry is already present. The debug logs are helpful. Consider adding a short doc snippet explaining the concurrency rationale for future maintainers.

263-283: Effective approach to manage creation failure signals.
The usage of AsyncStream<Result<Void, InternalError>> is a neat pattern to unify a fail signal and normal release completion. Recommend adding stress tests to confirm that multiple signals or cancellations don’t cause unexpected behaviors.

335-336: Use descriptive error on forced release.
Failing creation with a .roomReleasedBeforeOperationCompleted message is helpful. If you see repeated confusion in user logs, consider adding more context about which room and operation triggered the release.

Sources/AblyChat/AblyCocoaExtensions/Ably+Concurrency.swift (1)

6-28: Ensure consistency of error handling and consider avoiding fatalError.
Although using fatalError (line 22) to handle programmer errors aligns with the current design comments, you could replace it with a custom assertion or descriptive error throw that gracefully fails without abruptly terminating. This helps maintain stability in environments where unexpected runtime termination is undesirable.

- fatalError("ably-cocoa request threw an error - this indicates a programmer error")
+ assertionFailure("ably-cocoa request encountered an unexpected error; please check usage.")
+ throw InternalError(description: "Programmer error in AblyCocoa request")
Sources/AblyChat/DefaultPresence.swift (1)

211-211: Catch thrown errors during subscription callbacks.
In both subscription callbacks (lines 211 & 230), if processPresenceSubscribe throws, the Task will silently fail. Consider wrapping with do-catch and logging or accommodating user-facing error handling for more graceful fallback.

Task {
-   let presenceEvent = try processPresenceSubscribe(...)
-   subscription.emit(presenceEvent)
+   do {
+       let presenceEvent = try processPresenceSubscribe(...)
+       subscription.emit(presenceEvent)
+   } catch {
+       logger.log(message: "Error subscribing: \(error)", level: .error)
+   }
}

Also applies to: 230-230

Sources/AblyChat/RoomLifecycleManager.swift (1)

758-759: _performAttachOperation and performAttachmentCycle: watch concurrency states.
The code checks for in-progress operations and uses status.operationID to coordinate concurrency. Ensure all transitions remain atomic if anything else tries to modify status simultaneously (e.g., from other actor tasks).

Also applies to: 764-785, 791-791

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between dba13ae and 1dfa781.

📒 Files selected for processing (42)
  • CONTRIBUTING.md (1 hunks)
  • Sources/AblyChat/AblyCocoaExtensions/Ably+Concurrency.swift (1 hunks)
  • Sources/AblyChat/ChatAPI.swift (7 hunks)
  • Sources/AblyChat/DefaultMessages.swift (7 hunks)
  • Sources/AblyChat/DefaultOccupancy.swift (1 hunks)
  • Sources/AblyChat/DefaultPresence.swift (5 hunks)
  • Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1 hunks)
  • Sources/AblyChat/DefaultRoomReactions.swift (1 hunks)
  • Sources/AblyChat/DefaultTyping.swift (3 hunks)
  • Sources/AblyChat/Errors.swift (4 hunks)
  • Sources/AblyChat/Extensions/Dictionary+Extensions.swift (1 hunks)
  • Sources/AblyChat/Headers.swift (3 hunks)
  • Sources/AblyChat/InternalError.swift (1 hunks)
  • Sources/AblyChat/JSONCodable.swift (22 hunks)
  • Sources/AblyChat/Message.swift (2 hunks)
  • Sources/AblyChat/Messages.swift (6 hunks)
  • Sources/AblyChat/Occupancy.swift (2 hunks)
  • Sources/AblyChat/PaginatedResult.swift (3 hunks)
  • Sources/AblyChat/Presence.swift (7 hunks)
  • Sources/AblyChat/PresenceDataDTO.swift (1 hunks)
  • Sources/AblyChat/Room.swift (4 hunks)
  • Sources/AblyChat/RoomFeature.swift (2 hunks)
  • Sources/AblyChat/RoomLifecycleManager.swift (17 hunks)
  • Sources/AblyChat/RoomReactionDTO.swift (2 hunks)
  • Sources/AblyChat/RoomReactions.swift (1 hunks)
  • Sources/AblyChat/Rooms.swift (6 hunks)
  • Sources/AblyChat/Typing.swift (2 hunks)
  • Tests/AblyChatTests/ChatAPITests.swift (1 hunks)
  • Tests/AblyChatTests/DefaultMessagesTests.swift (3 hunks)
  • Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (1 hunks)
  • Tests/AblyChatTests/DefaultRoomPresenceTests.swift (6 hunks)
  • Tests/AblyChatTests/DefaultRoomTypingTests.swift (2 hunks)
  • Tests/AblyChatTests/DefaultRoomsTests.swift (2 hunks)
  • Tests/AblyChatTests/Helpers/Helpers.swift (2 hunks)
  • Tests/AblyChatTests/InternalErrorTests.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRoom.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomFactory.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2 hunks)
  • Tests/AblyChatTests/PresenceDataDTOTests.swift (1 hunks)
  • Tests/AblyChatTests/RoomReactionDTOTests.swift (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (31)
  • Sources/AblyChat/DefaultRoomReactions.swift
  • Tests/AblyChatTests/ChatAPITests.swift
  • Sources/AblyChat/PresenceDataDTO.swift
  • Tests/AblyChatTests/PresenceDataDTOTests.swift
  • Tests/AblyChatTests/DefaultRoomsTests.swift
  • Tests/AblyChatTests/RoomReactionDTOTests.swift
  • Sources/AblyChat/Errors.swift
  • Tests/AblyChatTests/DefaultRoomPresenceTests.swift
  • Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift
  • Sources/AblyChat/Headers.swift
  • Sources/AblyChat/Extensions/Dictionary+Extensions.swift
  • Sources/AblyChat/DefaultOccupancy.swift
  • Sources/AblyChat/RoomReactions.swift
  • Tests/AblyChatTests/DefaultMessagesTests.swift
  • Tests/AblyChatTests/DefaultRoomTypingTests.swift
  • Sources/AblyChat/RoomReactionDTO.swift
  • Tests/AblyChatTests/Mocks/MockRoomFactory.swift
  • Sources/AblyChat/DefaultRoomLifecycleContributor.swift
  • Sources/AblyChat/RoomFeature.swift
  • Sources/AblyChat/Message.swift
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
  • Sources/AblyChat/Typing.swift
  • Tests/AblyChatTests/Mocks/MockRoom.swift
  • Sources/AblyChat/Occupancy.swift
  • Sources/AblyChat/DefaultMessages.swift
  • Sources/AblyChat/Messages.swift
  • Sources/AblyChat/Room.swift
  • Sources/AblyChat/DefaultTyping.swift
  • Sources/AblyChat/ChatAPI.swift
  • Sources/AblyChat/Presence.swift
  • Sources/AblyChat/PaginatedResult.swift
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md

[uncategorized] ~43-~43: “its” (belonging to it) seems less likely than “it”
Context: ...instead return a Result and then call its .get() method. - `Dictionary.mapVal...

(AI_HYDRA_LEO_CPT_ITS_IT)

[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...ify the type of the thrown error, like: do throws(InternalError) { … }. - The compiler will never infer the t...

(UNLIKELY_OPENING_PUNCTUATION)

[uncategorized] ~46-~46: Loose punctuation mark.
Context: ...ill need to specify this yourself; e.g. let items = try jsonValues.map { jsonValue throws(InternalError) in … }. ### Testing guidelines #### Exposin...

(UNLIKELY_OPENING_PUNCTUATION)

⏰ Context from checks skipped due to timeout of 90000ms (10)
  • GitHub Check: Example app, tvOS (Xcode 16)
  • GitHub Check: Example app, iOS (Xcode 16)
  • GitHub Check: Example app, macOS (Xcode 16)
  • GitHub Check: Xcode, release configuration, tvOS (Xcode 16)
  • GitHub Check: Xcode, tvOS (Xcode 16)
  • GitHub Check: Xcode, release configuration, iOS (Xcode 16)
  • GitHub Check: SPM (Xcode 16)
  • GitHub Check: Xcode, iOS (Xcode 16)
  • GitHub Check: Xcode, release configuration, macOS (Xcode 16)
  • GitHub Check: Xcode, macOS (Xcode 16)
🔇 Additional comments (34)
Tests/AblyChatTests/InternalErrorTests.swift (1)

1-24: Good test coverage for InternalError to ARTErrorInfo conversion

The tests effectively verify both scenarios:

  1. When the underlying error is already ARTErrorInfo
  2. When the underlying error is a different error type

These tests align perfectly with the PR objective of ensuring the public API throws ARTErrorInfo consistently.

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

67-80: Appropriate error handling in attach method

The method now throws InternalError instead of ARTErrorInfo and properly converts any caught errors using toInternalError(). This aligns with the PR objective of standardizing error types.

82-95: Appropriate error handling in detach method

Similar to the attach method, this implementation properly converts errors to InternalError and maintains consistency with the error handling approach.

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

25-35: Appropriate error handling in waitToBeAbleToPerformPresenceOperations method

The method has been updated to throw InternalError instead of ARTErrorInfo and properly converts errors using the toInternalError() method. This implementation aligns with the PR objective of standardizing error types throughout the codebase.

CONTRIBUTING.md (1)

35-47: Excellent documentation on error handling approach

The new section provides comprehensive guidance on typed throws in the SDK, clearly explaining:

  • The use of ARTErrorInfo in the public API
  • The use of InternalError internally
  • Swift's limitations with typed throws
  • Workarounds for common scenarios

This documentation will be valuable for current and future contributors to understand the error handling approach.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~43-~43: “its” (belonging to it) seems less likely than “it”
Context: ...instead return a Result and then call its .get() method. - `Dictionary.mapVal...

(AI_HYDRA_LEO_CPT_ITS_IT)

[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...ify the type of the thrown error, like: do throws(InternalError) { … }. - The compiler will never infer the t...

(UNLIKELY_OPENING_PUNCTUATION)

[uncategorized] ~46-~46: Loose punctuation mark.
Context: ...ill need to specify this yourself; e.g. let items = try jsonValues.map { jsonValue throws(InternalError) in … }. ### Testing guidelines #### Exposin...

(UNLIKELY_OPENING_PUNCTUATION)

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

5-18: Confirm consistency in error checks.
The updated logic conditionally casts maybeError to both ARTErrorInfo and InternalError to detect a chat error. Ensure that callsites handle both raw ARTErrorInfo and wrapped InternalError consistently. Also, verifying test coverage for isChatError with both error forms would be beneficial.

Do you already have tests that pass in an InternalError.errorInfo(...)? If not, here's a recommended approach to add them for completeness.

35-41: Validate isInternalErrorWrappingErrorInfo usage.
This helper is concise and appears correct. However, ensure all references where an InternalError might embed non-matching ARTErrorInfo (e.g., different code or domain) are tested. Otherwise, you risk false positives if an ARTErrorInfo is unexpectedly equal.

68-75: Additional coverage for isInternalErrorWithCase.
This function elegantly checks whether an error matches a specific internal error case. Consider augmenting test scenarios that pass in other error instances, verifying it returns false for non-matching types.

Sources/AblyChat/InternalError.swift (3)

33-37: Good extension for bridging ARTErrorInfo to InternalError.
This simple conversion accurately wraps external ARTErrorInfo. Ensure that places converting from ARTErrorInfo follow up with an appropriate typed error check if additional context is needed.

39-43: Verify coverage for ChatAPI.ChatError conversion.
It’s beneficial that ChatAPI.ChatError can now map to InternalError. Confirm that each variant of ChatAPI.ChatError has an appropriate integration test to ensure the bridging logic is exercised.

57-61: Validate PaginatedResultError path.
Mapping PaginatedResultError to an .other(.paginatedResultError) maintains type safety. If you discover any confusion around pagination or partial fetch errors, consider adding more explicit error codes or messages to facilitate debugging.

Sources/AblyChat/Rooms.swift (6)

28-28: Typed throws provides clarity.
Changing get(roomID:options:) to throw (ARTErrorInfo) explicitly conveys runtime error types, which is a good step toward safer Swift API design. Ensure your documentation highlights this typed-throw contract.

86-89: Ensure creation task lifecycle is well-defined.
The creationTask storing Result<RoomFactory.Room, InternalError> is helpful for concurrency. Confirm that the failCreation closure is consistently invoked to avoid leaving any tasks hanging.

154-164: Validate mismatch logic for room options.
The new check that throws an inconsistentRoomOptions error is clear. Confirm all code paths that might override previously stored roomOptions are tested to avoid legitimate overrides being flagged.

182-253: Complex concurrency flow – proceed with caution.
The parallel tasks waiting for release completion or creation failure are well-coordinated with withTaskGroup. This is powerful but can be tricky. Carefully maintain logs and test coverage to prevent subtle concurrency bugs (e.g., attempting multiple failCreation calls from different threads).

256-260: Re-check error translation to ARTErrorInfo.
Wrapping error.toARTErrorInfo() inside catch ensures external callers see ARTErrorInfo. Keep an eye out for internal errors that might contain additional info lost in translation.

294-294: Create room flow.
Creating the room and immediately storing it in roomStates is correct. Just ensure a robust test that forcibly cancels the task before or during createRoom to confirm safety.

Sources/AblyChat/AblyCocoaExtensions/Ably+Concurrency.swift (4)

32-46: Confirm async usage and concurrency correctness in attachAsync.
The withCheckedContinuation pattern to return Result<Void, ARTErrorInfo> looks proper, and the subsequent .get() call ensures typed error propagation. Just ensure that any concurrent modifications to shared state are wrapped or guarded appropriately in the call sites.

48-62: Confirm async usage and concurrency correctness in detachAsync.
Same as with attachAsync, the concurrency model appears correct, and typed errors are consistently converted into InternalError.

65-72: Validate PresenceMessage structure compliance with concurrency.
While marking it as a Sendable version of ARTPresenceMessage is correct, consider double-checking that all its fields conform to thread-safe usage, especially if they’re ever mutated after initialization.

88-171: Overall consistency in typed async presence APIs.
All new presence methods (getAsync, leaveAsync, enterClientAsync, updateAsync) correctly use withCheckedContinuation and convert ARTErrorInfo into InternalError. The usage of preconditionFailure for unexpected nil branches also appears purposeful. This uniform approach strengthens clarity.

Sources/AblyChat/DefaultPresence.swift (4)

22-45: Synchronized error conversion in get().
The logic properly waits for presence readiness and catches errors, translating them to ARTErrorInfo. The approach is consistent with the rest of the typed-throws design.

73-97: isUserPresent aligns well with typed-throws design.
The call to getAsync(ARTRealtimePresenceQuery(clientId: …)) logically complements the function’s purpose. No issues noted.

108-131: Validate test coverage for enter(optionalData:).
Enforcing typed errors and logging are good. Confirm that your test suite includes coverage for success/failure paths, ensuring that presence calling code properly awaits and handles potential errors.

297-297: Prompt consistency for error conversions.
processPresenceSubscribe directly throws ARTErrorInfo, while other methods rely on conversion to InternalError. If uniform typed throws are desired throughout, consider converting or rethrowing as InternalError here as well.

Sources/AblyChat/RoomLifecycleManager.swift (4)

15-16: Typing the attach/detach operations for InternalError.
Switching from ARTErrorInfo to InternalError ensures consistent usage across the codebase. The asynchronous approach remains coherent with the concurrency patterns in the rest of the SDK.

Also applies to: 44-45, 49-49

589-590: Refined operation flow with typed continuation results.
Embedding InternalError in the continuation and wait flow is sensible for orchestrating attach/detach across multiple contributors. This improves clarity on final error states.

Also applies to: 653-655, 679-679, 700-703

817-830: Graceful handling for suspended/failed states in attach/detach cycles.
This robust approach transitions the room status to suspended or failed while scheduling or executing follow-up operations. Its stepwise flow, with fallback cycles, seems thorough. A coverage check for partial failures (one channel fails while another succeeds) may help.

Also applies to: 883-897, 905-905, 925-927, 940-940, 971-972

1220-1252: Verifying presence operations readiness.
The logic around waitToBeAbleToPerformPresenceOperations checks for an attached state or fails early if unreachable. This clarifies the state machine around presence operations.

Sources/AblyChat/JSONCodable.swift (5)

8-10: Protocol method signatures properly updated for typed throws

The update to init(jsonValue:) to use typed throws with InternalError is correctly implemented. This change aligns with the PR objective to standardize error handling in the public API.

25-27: Protocol method signatures properly updated for typed throws

The update to init(jsonObject:) to use typed throws with InternalError is correctly implemented. This matches the approach used in the JSONDecodable protocol.

38-44: Error conversion implementation is consistent

The implementation correctly uses the .toInternalError() method to convert JSONValueDecodingError to InternalError. This ensures that all errors thrown from this protocol implementation are of the expected type.

58-68: Error handling standardization in dictionary extension

The method signature update and error conversion implementation for objectValueForKey is properly implemented. Both error cases now consistently throw InternalError using the .toInternalError() conversion method.

73-87: Consistent error handling across all dictionary extension methods

All dictionary extension methods have been consistently updated to use typed throws with InternalError and properly convert errors using .toInternalError(). This approach is applied uniformly across all helper methods, ensuring a consistent error handling pattern.

Also applies to: 94-104, 109-123, 130-140, 145-159, 166-176, 181-195, 202-212, 219-233, 244-248, 253-259, 275-279, 286-292, 294-300

@lawrence-forooghian lawrence-forooghian changed the base branch from main to revert-some-tests March 12, 2025 20:37
Base automatically changed from revert-some-tests to main March 12, 2025 23:22
@lawrence-forooghian
Move realtime.request usage into an async helper

Verified

This commit was signed with the committer’s verified signature.
lawrence-forooghian Lawrence Forooghian
GPG key ID: C0C4A6A46EB6AD78
Verified
Learn about vigilant mode
400617f 
Our current usage of `realtime.request` is quite messy at the call site,
and will get more so when we need to use typed throws throughout the
codebase for #43. So move it to a helper (I've changed it to use typed
throws whilst I'm at it).

This also opens the door to us using a
RoomLifecycleContributorChannel-like protocol internally when making
requests, which will simplify mocking.
@lawrence-forooghian
Move presence.get usage into an async helper

Verified

This commit was signed with the committer’s verified signature.
lawrence-forooghian Lawrence Forooghian
GPG key ID: C0C4A6A46EB6AD78
Verified
Learn about vigilant mode
4c7c5a8 
Motivation as in 400617f. It's nice that the presence data and extras
are now expressed in terms of JSONValue, which simplifies various call
sites.
@lawrence-forooghian
Move presence.leave usage into an async helper
fc70a0d 
Motivation as in 4c7c5a8.
@lawrence-forooghian
Move presence.enter usage into an async helper
94ead73 
Motivation as in 4c7c5a8.
@lawrence-forooghian
Move presence.update usage into an async helper
00b50ae 
Motivation as in 4c7c5a8.
@lawrence-forooghian lawrence-forooghian force-pushed the 43-use-typed-throws branch 2 times, most recently from 37d38ac to 11d58da Compare March 13, 2025 13:08
@github-actions github-actions bot temporarily deployed to staging/pull/234/AblyChat March 13, 2025 13:09 Inactive
coderabbitai[bot]
coderabbitai bot reviewed Mar 13, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
Sources/AblyChat/DefaultTyping.swift (1)

101-102: 🛠️ Refactor suggestion

Ensure error.message is valid for non-InternalError values.

If the thrown error isn’t an InternalError or ARTErrorInfo, calling error.message may cause runtime issues. Consider fallback to error.localizedDescription to handle all error types safely.

- logger.log(message: error.message, level: .error)
+ if let internalError = error as? InternalError {
+     logger.log(message: internalError.message, level: .error)
+ } else if let artError = error as? ARTErrorInfo {
+     logger.log(message: artError.message, level: .error)
+ } else {
+     logger.log(message: "\(error)", level: .error)
+ }
🧹 Nitpick comments (6)
CONTRIBUTING.md (1)

45-46: Consider adjusting punctuation to avoid potential style issues.

The dash (“-”) following the code snippet can cause punctuation style warnings. Replacing it with a comma, semicolon, or a full stop can improve clarity and eliminate the warning.

- like: `do throws(InternalError) { … }`. - The compiler will never infer the type...
+ like: `do throws(InternalError) { … }`. The compiler will never infer the type...
🧰 Tools
🪛 LanguageTool

[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...ify the type of the thrown error, like: do throws(InternalError) { … }. - The compiler will never infer the t...

(UNLIKELY_OPENING_PUNCTUATION)

[uncategorized] ~46-~46: Loose punctuation mark.
Context: ...ill need to specify this yourself; e.g. let items = try jsonValues.map { jsonValue throws(InternalError) in … }. ### Testing guidelines #### Exposin...

(UNLIKELY_OPENING_PUNCTUATION)

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

73-75: Unimplemented detach in mock
Currently calls fatalError("Not yet implemented"). Consider returning a no-op or a made-up error for test stability rather than halting.

-fatalError("Not yet implemented")
+throw ARTErrorInfo.createMockError(description: "Mock detach not implemented.")

324-326: isUserPresent remains unimplemented
Marking this with fatalError can disrupt test flows. Consider returning a predictable bool or throwing a mock error to facilitate testing edge cases.

-fatalError("Not yet implemented")
+return false // or
+throw ARTErrorInfo.createMockError(description: "isUserPresent not implemented.")
Sources/AblyChat/DefaultPresence.swift (2)

73-97: Add unit tests for isUserPresent(clientID:).
While the logic is straightforward, including dedicated test coverage for both present and absent client scenarios would strengthen confidence.

210-210: Possible unhandled errors within the Task.
Because errors thrown inside the Task block can be lost without a do-catch, consider wrapping try processPresenceSubscribe(...) to handle or log failures properly.

Sources/AblyChat/RoomLifecycleManager.swift (1)

764-865: Extend typed errors throughout attach cycle.
Converting attach failures into InternalError is coherent. Consider documenting partial attach scenarios if some contributors fail halfway through.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 1dfa781 and 11d58da.

📒 Files selected for processing (41)
  • CONTRIBUTING.md (1 hunks)
  • Example/AblyChatExample/Mocks/MockClients.swift (13 hunks)
  • Sources/AblyChat/AblyCocoaExtensions/Ably+Concurrency.swift (1 hunks)
  • Sources/AblyChat/ChatAPI.swift (7 hunks)
  • Sources/AblyChat/DefaultMessages.swift (6 hunks)
  • Sources/AblyChat/DefaultOccupancy.swift (1 hunks)
  • Sources/AblyChat/DefaultPresence.swift (5 hunks)
  • Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1 hunks)
  • Sources/AblyChat/DefaultRoomReactions.swift (1 hunks)
  • Sources/AblyChat/DefaultTyping.swift (3 hunks)
  • Sources/AblyChat/Errors.swift (4 hunks)
  • Sources/AblyChat/Extensions/Dictionary+Extensions.swift (1 hunks)
  • Sources/AblyChat/Headers.swift (3 hunks)
  • Sources/AblyChat/InternalError.swift (1 hunks)
  • Sources/AblyChat/JSONCodable.swift (22 hunks)
  • Sources/AblyChat/Message.swift (2 hunks)
  • Sources/AblyChat/Messages.swift (6 hunks)
  • Sources/AblyChat/Occupancy.swift (2 hunks)
  • Sources/AblyChat/PaginatedResult.swift (3 hunks)
  • Sources/AblyChat/Presence.swift (7 hunks)
  • Sources/AblyChat/PresenceDataDTO.swift (1 hunks)
  • Sources/AblyChat/Room.swift (4 hunks)
  • Sources/AblyChat/RoomFeature.swift (2 hunks)
  • Sources/AblyChat/RoomLifecycleManager.swift (17 hunks)
  • Sources/AblyChat/RoomReactionDTO.swift (2 hunks)
  • Sources/AblyChat/RoomReactions.swift (1 hunks)
  • Sources/AblyChat/Rooms.swift (6 hunks)
  • Sources/AblyChat/Typing.swift (2 hunks)
  • Tests/AblyChatTests/ChatAPITests.swift (2 hunks)
  • Tests/AblyChatTests/DefaultMessagesTests.swift (2 hunks)
  • Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (1 hunks)
  • Tests/AblyChatTests/DefaultRoomsTests.swift (2 hunks)
  • Tests/AblyChatTests/Helpers/Helpers.swift (2 hunks)
  • Tests/AblyChatTests/InternalErrorTests.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRoom.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomFactory.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2 hunks)
  • Tests/AblyChatTests/PresenceDataDTOTests.swift (1 hunks)
  • Tests/AblyChatTests/RoomReactionDTOTests.swift (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (31)
  • Tests/AblyChatTests/PresenceDataDTOTests.swift
  • Sources/AblyChat/RoomReactions.swift
  • Tests/AblyChatTests/DefaultRoomsTests.swift
  • Tests/AblyChatTests/Mocks/MockRoom.swift
  • Sources/AblyChat/Extensions/Dictionary+Extensions.swift
  • Sources/AblyChat/Headers.swift
  • Tests/AblyChatTests/DefaultMessagesTests.swift
  • Tests/AblyChatTests/RoomReactionDTOTests.swift
  • Tests/AblyChatTests/Mocks/MockRoomFactory.swift
  • Tests/AblyChatTests/ChatAPITests.swift
  • Sources/AblyChat/DefaultRoomReactions.swift
  • Sources/AblyChat/Typing.swift
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift
  • Sources/AblyChat/Message.swift
  • Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift
  • Sources/AblyChat/Occupancy.swift
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
  • Sources/AblyChat/Errors.swift
  • Tests/AblyChatTests/Mocks/MockFeatureChannel.swift
  • Sources/AblyChat/RoomFeature.swift
  • Sources/AblyChat/DefaultRoomLifecycleContributor.swift
  • Sources/AblyChat/DefaultOccupancy.swift
  • Sources/AblyChat/RoomReactionDTO.swift
  • Sources/AblyChat/PresenceDataDTO.swift
  • Tests/AblyChatTests/InternalErrorTests.swift
  • Sources/AblyChat/PaginatedResult.swift
  • Sources/AblyChat/Messages.swift
  • Sources/AblyChat/Room.swift
  • Sources/AblyChat/DefaultMessages.swift
  • Sources/AblyChat/Presence.swift
  • Sources/AblyChat/ChatAPI.swift
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md

[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...ify the type of the thrown error, like: do throws(InternalError) { … }. - The compiler will never infer the t...

(UNLIKELY_OPENING_PUNCTUATION)

[uncategorized] ~46-~46: Loose punctuation mark.
Context: ...ill need to specify this yourself; e.g. let items = try jsonValues.map { jsonValue throws(InternalError) in … }. ### Testing guidelines #### Exposin...

(UNLIKELY_OPENING_PUNCTUATION)

⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: Example app, tvOS (Xcode 16)
  • GitHub Check: Example app, iOS (Xcode 16)
  • GitHub Check: Xcode, release configuration, tvOS (Xcode 16)
  • GitHub Check: Xcode, tvOS (Xcode 16)
  • GitHub Check: SPM (Xcode 16)
  • GitHub Check: Xcode, iOS (Xcode 16)
  • GitHub Check: Xcode, release configuration, macOS (Xcode 16)
  • GitHub Check: Xcode, macOS (Xcode 16)
🔇 Additional comments (71)
Tests/AblyChatTests/Helpers/Helpers.swift (2)

8-33: Looks good!

The logic for detecting and matching ARTErrorInfo appears sound. The guard statement and subsequent checks effectively confirm appropriate error domain and codes.

35-38:

❓ Verification inconclusive

Verify that ARTErrorInfo supports equality checks.

You're using if case let .errorInfo(actualErrorInfo) = internalError, expectedErrorInfo == actualErrorInfo. Ensure ARTErrorInfo implements equality or that you’ve added a custom extension to support this comparison.

🏁 Script executed:

#!/bin/bash
# Description: Search for any extension or conformance of `ARTErrorInfo` to `Equatable`.
fd -e swift --exec rg "struct|class|extension\s+ARTErrorInfo.*Equatable" {}

Length of output: 12837

Action Required: Verify Equality Conformance for ARTErrorInfo

The code uses an equality check on ARTErrorInfo:

if let internalError = error as? InternalError, case let .errorInfo(actualErrorInfo) = internalError, expectedErrorInfo == actualErrorInfo { … }

Our initial search (via the provided shell script) did not return clear evidence that ARTErrorInfo explicitly conforms to Equatable—or that there is a custom extension providing an equality implementation. Please manually verify (or update the implementation) to ensure that ARTErrorInfo supports equality comparisons as intended.

• Location: Tests/AblyChatTests/Helpers/Helpers.swift, Lines 35–38

Sources/AblyChat/InternalError.swift (1)

1-62: Structured internal error handling looks great!

Defining InternalError with .errorInfo and .other variants neatly unifies multiple error sources. Returning a well-defined ARTErrorInfo ensures consistent public exposure of error data.

Sources/AblyChat/DefaultTyping.swift (2)

198-214: Previous concurrency concern still applies.

106-107:

✅ Verification successful

Validate consistency of error conversion to ARTErrorInfo.

The catch block rethrows using error.toARTErrorInfo(). Confirm that all possible errors in the do-block are indeed InternalError, to avoid runtime issues if another error type propagates.

Also applies to: 148-149, 173-174

🏁 Script executed:

#!/bin/bash
# Description: Search for async calls that might throw something other than `InternalError`.
rg -A 4 "throws\(InternalError\)|throws\(ARTErrorInfo\)"

Length of output: 75611

Ensure error conversion strictly handles InternalError.
In DefaultTyping.swift (lines 106–107, 148–149, and 173–174), the catch block rethrows errors via error.toARTErrorInfo(). Our verification confirms that the surrounding do blocks are declared with throws(InternalError), ensuring that only InternalError instances are expected. Please double-check that no other error types are inadvertently introduced into these blocks—for both current implementations and any future changes—so that the conversion remains safe and consistent.

Sources/AblyChat/Rooms.swift (7)

28-28: Use typed throw in get for clearer error handling
Designating throws(ARTErrorInfo) in the method signature provides explicit error semantics, making it clearer to callers what exceptions to expect.

86-89: Refined error type usage in RoomMapEntry.requestAwaitingRelease
Switching from a generic error to InternalError helps unify error handling and maintain consistency with the rest of the codebase.

105-112: Consistent approach in waitForRoom()
Using async throws(InternalError) and retrieving the Result via creationTask.value.get() cleanly bridges concurrency and typed error handling. No issues noted.

154-261: Robust concurrency logic in get(roomID:options:)
The approach to synchronize room creation and release, handle in-progress states, and convert InternalError to ARTErrorInfo ensures type-safe error propagation. The withTaskGroup usage to wait for the first completed operation is a solid concurrency pattern.

264-283: Neat creation-failure streaming mechanism
Defining makeCreationFailureFunctions() to yield a failure via AsyncStream is an elegant way to coordinate inter-task failure signals. No concerns noted.

294-299: createRoom complements get perfectly
This helper adheres to the same error-handling policy, ensuring consistency across room creation code paths.

335-335: Consistent bridging from ARTErrorInfo to InternalError
Invoking .toInternalError() on a newly created ARTErrorInfo aligns with the rest of the file's typed-error strategy.

Sources/AblyChat/AblyCocoaExtensions/Ably+Concurrency.swift (9)

6-29: requestAsync leverages Swift concurrency effectively
Using withCheckedContinuation for callback-based APIs is appropriate. The fallback to fatalError() for programmer errors underscores the design intent that these exceptions should not typically occur in production.

32-46: attachAsync with typed error bridging
Attaching the channel with a Result<Void, ARTErrorInfo> continuation and converting to InternalError ensures consistency with higher-level error handling.

48-62: detachAsync follows the same pattern
Again, a clear use of withCheckedContinuation, bridging ARTErrorInfo → InternalError. No issues noted.

65-87: New PresenceMessage struct
Mapping from ARTPresenceMessage into a Sendable struct is straightforward and safe. This avoids concurrency pitfalls when sharing presence data.

88-106: getAsync() presence retrieval
Capturing presence state with explicit typed errors fits well. The usage of .get() on the Result ensures typed error propagation.

107-123: getAsync(_ query:) variant
Same pattern as the no-parameter method, with typed InternalError. Implementation is consistent and clear.

125-139: leaveAsync(_:) method
No concerns: checks for error, bridges to InternalError, completes successfully otherwise.

141-155: enterClientAsync bridging
Using withCheckedContinuation and bridging ARTErrorInfo → InternalError is consistent across these concurrency extensions.

157-171: updateAsync clarity
Retaining the callback-based logic but returning async throws(InternalError) improves code readability and error handling uniformity.

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

26-33: Mock get uses a typed throw
Switching to throws(ARTErrorInfo) for the get method is consistent with the updated API expectations. Implementation is straightforward.

69-71: Mock attach partial implementation
Logging-only is fine for a mock. The typed throw aligns with the real method's signature.

125-127: Mock get(options:)
Returning a basic MockMessagesPaginatedResult is sufficient for testing scenarios.

129-145: Mock send for messages
Emitting the newly created Message to the subscription storage simulates sending behavior nicely.

147-163: Mock update for messages
This approach consistently updates the action to .update and emits a fresh message object. Looks good.

165-181: Mock delete for messages
Similarly sets .delete action and emits the new state. No issues found.

214-224: Mock send for reactions
Generating a new Reaction and emitting it is consistent with mock patterns.

261-263: Mock get in typing
Returning two random names from MockStrings is a valid approach for simulation.

265-267: Mock start in typing
Emitting a typing event with the current client ID is sufficient for basic testing.

269-271: Mock stop in typing
Emitting an empty TypingEvent is similarly adequate for tests.

328-330: Mock presence enter()
Delegating to enter(dataForEvent: nil) is consistent with typed-error refactoring.

332-334: Mock presence enter(data:)
Correctly funnels calls into the shared enter(dataForEvent:) helper. Looks good.

336-345: Private enter(dataForEvent:)
Emitting a .enter event to mockSubscriptions is a straightforward simulation of presence entering.

347-349: Presence update() no-arg
Delegation to update(dataForEvent: nil) preserves consistency. No concerns.

351-353: Presence update(data:)
Similarly delegates to the shared update function. Implementation is coherent.

355-364: Private update(dataForEvent:)
Emitting an .update event ensures presence status changes can be captured.

366-368: Presence leave()
Again, nicely delegates to leave(dataForEvent: nil).

370-372: Presence leave(data:)
Consistent approach. No issues.

374-383: Private leave(dataForEvent:)
Properly emits a .leave presence event. This addresses leaving logic well for mocking scenarios.

422-424: Mock get in occupancy
Provides a static occupancy event. Suitable for simple testing.

Sources/AblyChat/DefaultPresence.swift (12)

22-44: Consistent re-throwing with ARTErrorInfo.
The method correctly wraps InternalError errors and re-throws them as ARTErrorInfo. Logging is also well placed for troubleshooting.

47-69: Parameterized presence retrieval is now fully utilized.
Passing params.asARTRealtimePresenceQuery() ensures the query parameters are respected, resolving previous concerns about unused parameters.

99-104: Helper method usage is clear.
These lines simply delegate to enter(optionalData:), which improves maintainability by keeping logic centralized.

107-130: Robust async presence enter flow.
This block cleanly waits for the room to be attachable before calling enterClientAsync, then transforms and re-throws errors as ARTErrorInfo.

133-139: Consistent delegation for update(data:).
Similar to enter(), this pattern keeps exposed methods small and maintainable.

142-164: Error conversion in update(optionalData:).
You are correctly logging the error and converting to ARTErrorInfo, maintaining consistent error-handling patterns.

167-173: Delegation for leave(data:).
This matches the established style, increasing coherence across presence operations.

176-198: Presence leave operation.
Re-throwing InternalError as ARTErrorInfo maintains a uniform public interface. Logging statements are appropriate for debugging.

230-230: Same potential unhandled exception in Task.
This invocation mirrors line 210’s pattern.

252-257: Graceful handling of missing data in decodePresenceDataDTO().
Throwing an internal error when presenceData is nil makes the failure state explicit and ensures consistent error conversion.

267-295: Mapping PresenceMessage to domain model.
Neatly handles missing client IDs or timestamps and logs errors with context. This is a good, defensive approach.

297-297: Typed throws in subscription processing.
This move to throws(InternalError) ensures uniform error handling throughout.

Sources/AblyChat/RoomLifecycleManager.swift (14)

14-16: Transition to throws(InternalError) in channel protocol.
Using typed errors here unifies error handling at the protocol level and clarifies expected failure modes.

44-45: RoomLifecycleManager operations now use typed throws.
Shifting to throws(InternalError) for attach/detach/wait operations ensures internal errors are handled consistently.

Also applies to: 49-49

589-590: OperationResultContinuations now storing InternalError.
Revising continuation results to Result<Void, InternalError> centralizes error representation and prevents confusion.

653-655: Typed error throw in waitForCompletionOfOperationWithID.
This clarifies that operation completions can propagate InternalError to callers.

678-680: Operation completion result with InternalError.
Consistently capturing typed errors is helpful for reliability and debugging.

700-704: performAnOperation now throws(InternalError).
Adopting typed throws refines the contract for all room lifecycle operations.

746-759: Typed attach operation.
Calls to _performAttachOperation are now fully typed, aligning with the broader error-handling pattern.

865-871: Refined detach error signature.
Ensuring performDetachOperation throws InternalError keeps operations symmetrical.

877-899: Detachment cycle now typed.
Centralizing all detach failure paths around InternalError clarifies how partial failures propagate.

924-989: Partial contributor detachment handling.
Retry logic for non-failed contributors is robust. The approach to handling concurrency is sound here.

971-972: Detachment errors rethrown as InternalError.
This final ensures consistent error form across the entire cycle.

1075-1075: performRetryOperation typed throw.
Handling suspended channels under a uniform error type simplifies retry logic.

1178-1187: performRundownOperation error alignment.
Equally adopting InternalError for the RUNDOWN flow fosters consistency with attach/detach.

1220-1251: Presence operation readiness checks.
Throwing presenceOperationRequiresRoomAttach clarifies the precondition needed (attached room) and improves developer feedback.

Sources/AblyChat/JSONCodable.swift (4)

9-9: Protocols updated to throw InternalError.
Applying typed throws in JSONDecodable and JSONObjectDecodable clarifies decoding failure modes.

Also applies to: 26-26, 38-38

58-220: Dictionary extensions now throw InternalError.
This uniform approach ensures decoding mistakes produce consistent internal exceptions instead of generic errors.

245-253: Date extraction with typed errors.
Interpreting Ably’s epoch-based timestamps now explicitly returns an InternalError on invalid input.

275-286: Safe raw-value decoding with typed errors.
Providing a clear error if init(rawValue:) fails improves debugging and user feedback.

Also applies to: 294-294

maratal
maratal reviewed Mar 13, 2025
View reviewed changes
Copy link
Collaborator

@maratal maratal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some questions.

@lawrence-forooghian
Throw ARTErrorInfo from public API

Verified

This commit was signed with the committer’s verified signature.
lawrence-forooghian Lawrence Forooghian
GPG key ID: C0C4A6A46EB6AD78
Verified
Learn about vigilant mode
Loading
Loading status checks…
c09ac03 
This updates our public API to use typed throws, and specifically to
always throw ARTErrorInfo. This makes us consistent with ably-cocoa, and
allows users to access the error's `statusCode` to make decisions about
whether they can retry the action that caused the error.

I've gone for a light-touch approach of not changing any of the various
error types that exist inside the SDK; inside I've hidden them all
behind a common InternalError type, which can be converted to an
ARTErrorInfo. We can revisit this in #32 (i.e. should we keep this rich
internal Swift error types or just throw ARTErrorInfo internally?)

Resolves #43.
coderabbitai[bot]
coderabbitai bot reviewed Mar 13, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 11d58da and c09ac03.

📒 Files selected for processing (41)
  • CONTRIBUTING.md (1 hunks)
  • Example/AblyChatExample/Mocks/MockClients.swift (13 hunks)
  • Sources/AblyChat/AblyCocoaExtensions/Ably+Concurrency.swift (1 hunks)
  • Sources/AblyChat/ChatAPI.swift (7 hunks)
  • Sources/AblyChat/DefaultMessages.swift (6 hunks)
  • Sources/AblyChat/DefaultOccupancy.swift (1 hunks)
  • Sources/AblyChat/DefaultPresence.swift (5 hunks)
  • Sources/AblyChat/DefaultRoomLifecycleContributor.swift (1 hunks)
  • Sources/AblyChat/DefaultRoomReactions.swift (1 hunks)
  • Sources/AblyChat/DefaultTyping.swift (3 hunks)
  • Sources/AblyChat/Errors.swift (4 hunks)
  • Sources/AblyChat/Extensions/Dictionary+Extensions.swift (1 hunks)
  • Sources/AblyChat/Headers.swift (3 hunks)
  • Sources/AblyChat/InternalError.swift (1 hunks)
  • Sources/AblyChat/JSONCodable.swift (22 hunks)
  • Sources/AblyChat/Message.swift (2 hunks)
  • Sources/AblyChat/Messages.swift (6 hunks)
  • Sources/AblyChat/Occupancy.swift (2 hunks)
  • Sources/AblyChat/PaginatedResult.swift (3 hunks)
  • Sources/AblyChat/Presence.swift (7 hunks)
  • Sources/AblyChat/PresenceDataDTO.swift (1 hunks)
  • Sources/AblyChat/Room.swift (4 hunks)
  • Sources/AblyChat/RoomFeature.swift (2 hunks)
  • Sources/AblyChat/RoomLifecycleManager.swift (17 hunks)
  • Sources/AblyChat/RoomReactionDTO.swift (2 hunks)
  • Sources/AblyChat/RoomReactions.swift (1 hunks)
  • Sources/AblyChat/Rooms.swift (6 hunks)
  • Sources/AblyChat/Typing.swift (2 hunks)
  • Tests/AblyChatTests/ChatAPITests.swift (2 hunks)
  • Tests/AblyChatTests/DefaultMessagesTests.swift (2 hunks)
  • Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (1 hunks)
  • Tests/AblyChatTests/DefaultRoomsTests.swift (2 hunks)
  • Tests/AblyChatTests/Helpers/Helpers.swift (2 hunks)
  • Tests/AblyChatTests/InternalErrorTests.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRoom.swift (2 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomFactory.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2 hunks)
  • Tests/AblyChatTests/PresenceDataDTOTests.swift (1 hunks)
  • Tests/AblyChatTests/RoomReactionDTOTests.swift (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (33)
  • Sources/AblyChat/Headers.swift
  • Sources/AblyChat/PresenceDataDTO.swift
  • Sources/AblyChat/Extensions/Dictionary+Extensions.swift
  • Sources/AblyChat/Errors.swift
  • Tests/AblyChatTests/PresenceDataDTOTests.swift
  • Sources/AblyChat/RoomReactions.swift
  • Tests/AblyChatTests/RoomReactionDTOTests.swift
  • Sources/AblyChat/DefaultRoomReactions.swift
  • Tests/AblyChatTests/ChatAPITests.swift
  • Tests/AblyChatTests/Mocks/MockFeatureChannel.swift
  • Tests/AblyChatTests/DefaultRoomsTests.swift
  • Sources/AblyChat/Occupancy.swift
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift
  • Sources/AblyChat/RoomReactionDTO.swift
  • Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift
  • Tests/AblyChatTests/DefaultMessagesTests.swift
  • Tests/AblyChatTests/Mocks/MockRoom.swift
  • Sources/AblyChat/Typing.swift
  • Sources/AblyChat/DefaultRoomLifecycleContributor.swift
  • Tests/AblyChatTests/InternalErrorTests.swift
  • Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
  • Tests/AblyChatTests/Mocks/MockRoomFactory.swift
  • Sources/AblyChat/Message.swift
  • Sources/AblyChat/DefaultOccupancy.swift
  • Sources/AblyChat/Room.swift
  • Sources/AblyChat/RoomFeature.swift
  • Sources/AblyChat/DefaultMessages.swift
  • Sources/AblyChat/Presence.swift
  • Sources/AblyChat/RoomLifecycleManager.swift
  • Example/AblyChatExample/Mocks/MockClients.swift
  • Sources/AblyChat/Messages.swift
  • Sources/AblyChat/PaginatedResult.swift
  • Sources/AblyChat/JSONCodable.swift
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md

[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...ify the type of the thrown error, like: do throws(InternalError) { … }. - The compiler will never infer the t...

(UNLIKELY_OPENING_PUNCTUATION)

[uncategorized] ~46-~46: Loose punctuation mark.
Context: ...ill need to specify this yourself; e.g. let items = try jsonValues.map { jsonValue throws(InternalError) in … }. ### Testing guidelines #### Exposin...

(UNLIKELY_OPENING_PUNCTUATION)

⏰ Context from checks skipped due to timeout of 90000ms (11)
  • GitHub Check: Example app, tvOS (Xcode 16)
  • GitHub Check: Example app, iOS (Xcode 16)
  • GitHub Check: Example app, macOS (Xcode 16)
  • GitHub Check: Xcode, release configuration, tvOS (Xcode 16)
  • GitHub Check: Xcode, tvOS (Xcode 16)
  • GitHub Check: Xcode, release configuration, iOS (Xcode 16)
  • GitHub Check: Xcode, iOS (Xcode 16)
  • GitHub Check: Xcode, release configuration, macOS (Xcode 16)
  • GitHub Check: Xcode, macOS (Xcode 16)
  • GitHub Check: SPM (Xcode 16)
  • GitHub Check: SPM, release configuration (Xcode 16)
🔇 Additional comments (30)
CONTRIBUTING.md (1)

35-47: Great addition of error handling documentation!

This new "Throwing errors" section provides clear guidance on the error handling strategy - using ARTErrorInfo for public API and InternalError internally. The section about Swift's typed throws limitations and workarounds is particularly helpful for contributors.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~45-~45: Loose punctuation mark.
Context: ...ify the type of the thrown error, like: do throws(InternalError) { … }. - The compiler will never infer the t...

(UNLIKELY_OPENING_PUNCTUATION)

[uncategorized] ~46-~46: Loose punctuation mark.
Context: ...ill need to specify this yourself; e.g. let items = try jsonValues.map { jsonValue throws(InternalError) in … }. ### Testing guidelines #### Exposin...

(UNLIKELY_OPENING_PUNCTUATION)

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

5-22: Well-implemented error checking enhancements

The improved isChatError function now properly handles both direct ARTErrorInfo errors and InternalError instances that wrap an ARTErrorInfo. This makes the test helpers compatible with the new error handling approach.

35-41: Good addition of specialized error checking

The new isInternalErrorWrappingErrorInfo function provides a clean way to verify if an error is an InternalError wrapping a specific ARTErrorInfo. This will make tests more specific and readable.

43-66: Well-designed error categorization

The InternalError.Case enum provides a clean way to categorize different types of internal errors, enhancing testing capabilities. The enumCase property enables convenient access to the specific case of an error without complex pattern matching in tests.

68-74: Useful helper for error case checking

The isInternalErrorWithCase function complements the error case system, allowing for concise checks of error types in tests without boilerplate casting and pattern matching.

Sources/AblyChat/InternalError.swift (2)

3-31: Well-structured error handling implementation

The InternalError enum provides a clean, unified approach to error handling. The documentation explains the current design and future plans well. The toARTErrorInfo() method provides a clear conversion path from internal errors to public API errors.

33-61: Good extensions for error conversion

These extensions provide a consistent way to convert various error types to InternalError. This approach reduces boilerplate code throughout the codebase and ensures uniform error handling.

Sources/AblyChat/DefaultTyping.swift (5)

85-107: Improved error typing in get() method

The method now correctly uses typed throws with ARTErrorInfo for the public API and handles InternalError internally. The implementation has been simplified by using getAsync() directly.

111-149: Well-structured error handling in start() method

The method now uses a continuation pattern that returns a Result<Void, InternalError> and properly converts internal errors to ARTErrorInfo at the public API boundary. The implementation is clean and consistent with the new error handling approach.

153-174: Improved error handling in stop() method

The method now properly throws typed errors (ARTErrorInfo) and handles InternalError internally, maintaining consistency with the overall error handling approach in the SDK.

182-194: Good error handling in processPresenceGet

The method now properly throws InternalError and converts ARTErrorInfo to InternalError using the new extension method, maintaining consistency with the overall error handling approach.

198-204: Note about error propagation in Task

This code throws an error inside a Task, but since the function isn't async, the error won't propagate to the caller. However, this is existing behavior from before this PR and outside the current scope of changes.

Is this error handling pattern intended? Since the function isn't async, errors thrown within the Task won't propagate to the original caller. Consider adding a comment explaining this design decision or creating a separate issue to address this in the future.

Sources/AblyChat/Rooms.swift (5)

28-28: The protocol method signature is correctly updated to throw ARTErrorInfo.

The get method now explicitly throws ARTErrorInfo instead of a generic Error, which aligns with the PR objective to use typed throws in the public API.

86-88: Good update of the RoomMapEntry enum to use InternalError.

The change from generic Error to the more specific InternalError in both the creationTask result type and the failCreation closure parameter improves type safety and error handling clarity.

105-105: waitForRoom() method now throws InternalError as expected.

This change correctly aligns with the internal error handling strategy where internal methods throw InternalError.

154-260: Good implementation of the error handling pattern in the get method.

The implementation correctly uses do throws(InternalError) for internal error handling and converts to ARTErrorInfo at the public API boundary. The code correctly maintains the error conversion flow and preserves the detailed errors.

294-294: createRoom method now throws InternalError correctly.

This internal method has been updated to specifically throw InternalError, which aligns with the internal error handling strategy.

Sources/AblyChat/AblyCocoaExtensions/Ably+Concurrency.swift (4)

6-29: Well-implemented requestAsync method with proper error handling.

This new method extends ARTRealtimeInstanceMethodsProtocol with an asynchronous HTTP request capability that properly handles and converts errors. The implementation uses withCheckedContinuation correctly to bridge the callback-based API to Swift's async/await pattern.

32-63: Good update of attachAsync and detachAsync methods to throw InternalError.

Both methods now correctly use InternalError for error handling, which aligns with the internal error strategy in the codebase.

65-86: Well-designed PresenceMessage struct to ensure Sendable compliance.

The new PresenceMessage struct provides a Sendable version of ARTPresenceMessage, which is important for concurrency safety. The implementation correctly includes only the properties needed for the Chat SDK and provides a clean conversion method from the Ably Cocoa type.

88-171: Comprehensive set of async presence methods with proper error handling.

The extension adds several asynchronous methods to ARTRealtimePresenceProtocol that correctly handle errors and provide a modern Swift API surface. The implementation of updateAsync correctly calls the update method as expected.

Sources/AblyChat/ChatAPI.swift (4)

13-13: Public method signatures correctly updated to throw InternalError.

The method signatures for getMessages, sendMessage, updateMessage, deleteMessage, and getOccupancy have been properly updated to specify that they throw InternalError, which aligns with the internal error handling strategy.

Also applies to: 43-43, 82-82, 134-134, 172-172

45-46: Proper error conversion to InternalError in client ID validation.

The error handling for missing client ID now correctly converts ARTErrorInfo to InternalError using the toInternalError() method, maintaining the error type consistency.

Also applies to: 84-85

177-193: Good refactoring of makeRequest to use requestAsync and proper error handling.

The implementation now uses the new requestAsync method and correctly handles errors by throwing InternalError when needed. This simplifies the error handling path and maintains type consistency.

198-205: Updated makePaginatedRequest with proper error handling.

The method now correctly specifies that it throws InternalError and uses typed throw annotations in the mapping closure, ensuring error type consistency throughout the API.

Sources/AblyChat/DefaultPresence.swift (5)

22-22: Public method signatures correctly updated to throw ARTErrorInfo.

All public methods in the DefaultPresence class have been updated to specify that they throw ARTErrorInfo, which aligns with the PR objective to use typed throws in the public API.

Also applies to: 47-47, 73-73, 99-99, 103-103, 133-133, 137-137, 167-167, 171-171

23-44: Good implementation of the error handling pattern in all presence methods.

Each method correctly uses do throws(InternalError) blocks for internal error handling and converts to ARTErrorInfo at the public API boundary. The implementation also properly uses the new async methods from ARTRealtimePresenceProtocol.

Also applies to: 48-69, 74-96, 109-130, 143-164, 177-198

252-265: Proper error handling in decodePresenceDataDTO.

The method now correctly throws InternalError and converts errors appropriately, maintaining consistency with the rest of the error handling strategy.

267-295: Updated processPresenceGet to work with PresenceMessage and proper error handling.

The method now accepts PresenceMessage instead of ARTPresenceMessage and properly throws InternalError, aligning with the changes in the rest of the codebase.

297-297: Updated processPresenceSubscribe to work with PresenceMessage.

The method signature has been updated to accept PresenceMessage directly, which works well with the new PresenceMessage struct and improves the code's clarity.

maratal
maratal approved these changes Mar 13, 2025
View reviewed changes
Copy link
Collaborator

@maratal maratal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coderabbitai coderabbitai[bot]

@maratal maratal

@umair-ably umair-ably

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider using typed throws on public API
2 participants
@lawrence-forooghian @maratal