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.

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

[ECO-4945] Spec complete for Ephemeral Room Reactions #89

Merged
merged 1 commit into from
Nov 14, 2024
Merged

Conversation

umair-ably
Copy link
Collaborator

@umair-ably umair-ably commented Oct 18, 2024

Spec complete for Ephemeral Room Reactions in line with [1].

CHA-ER3b & CHA-ER3c have not been implemented despite being outlined in [1], as per the ADR at [2].

Example app has also been updated to support both a working and mock implementation of the Chat app. Video shows the example app being connected to a real ChatAPI/Realtime instance:

updated.demo.app.mov

[1] - https://sdk.ably.com/builds/ably/specification/pull/200/chat-features/
[2] - https://ably.atlassian.net/wiki/spaces/CHA/pages/3438116905/CHADR-066+Removing+Reserved+Keyspace#Solution

Unit tests are not exhaustive, will add more as part of #88

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a mode-based system for managing chat client connections (mock/live).
    • Added new asynchronous functions for handling chat room interactions and reactions.
    • Enhanced visual effects for reactions with new properties.
    • Added a new Reaction struct for representing reactions in chat.
    • Implemented a subscription system for handling reactions in integration tests.
  • Bug Fixes

    • Improved error handling for accessing reactions when not enabled.
    • Updated logic to simplify extraction of headers in message subscriptions.
  • Tests

    • Added unit tests for the DefaultRoomReactions class to validate functionalities.
    • Enhanced mock channel to track the last published message details.
    • Improved integration tests to include reaction handling and validation.
  • Documentation

    • Updated comments and TODOs for future improvements in message subscriptions.

Copy link

coderabbitai bot commented Oct 18, 2024

Note

Reviews paused

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Walkthrough

The changes in this pull request enhance the chat application by introducing a structured approach for managing chat client connections, distinguishing between mock and live environments. Key additions include a mode-based system, new asynchronous functions for handling messages and reactions, and improved error handling. The implementation of mock clients facilitates testing, while updates to the Reaction struct and related classes refine the user experience. Additionally, new tests are added to ensure the functionality of the DefaultRoomReactions class.

Changes

File Path Change Summary
Example/AblyChatExample/ContentView.swift Introduced mode-based system for chat client connections, added roomID, Environment enum, and new async functions.
Example/AblyChatExample/Mocks/Misc.swift Updated displayedText in Reaction extension to return type string instead of emoji.
Example/AblyChatExample/Mocks/MockClients.swift Added mock implementations for chat client system, modified attach method in MockRoom.
Sources/AblyChat/DefaultMessages.swift Simplified extraction of headers in subscribe method.
Sources/AblyChat/DefaultRoomReactions.swift Introduced DefaultRoomReactions class for managing room reactions, added methods for sending and subscribing to reactions.
Sources/AblyChat/Events.swift Added RoomReactionEvents enum for reaction events.
Sources/AblyChat/Reaction.swift Introduced Reaction struct with properties for chat reactions.
Sources/AblyChat/Room.swift Modified Room protocol and DefaultRoom to enhance error handling for reactions.
Sources/AblyChat/RoomReactions.swift Added asQueryItems() method to SendReactionParams struct.
Sources/AblyChat/Subscription.swift Introduced finish() method to Subscription struct for signaling completion.
Tests/AblyChatTests/DefaultRoomReactionsTests.swift Added unit tests for DefaultRoomReactions class to validate functionalities.
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift Enhanced MockRealtimeChannel to track last published message details.

Assessment against linked issues

Objective Addressed Explanation
Implement room level reactions (ECO-4945)

Possibly related issues

Possibly related PRs

Suggested reviewers

  • lawrence-forooghian

Poem

🐰 In the chat room where bunnies play,
New features hop in, brightening the day!
With reactions that twirl and messages that flow,
Mock clients are ready, let the fun grow!
So gather 'round, friends, let’s cheer and delight,
For a chat full of joy, everything feels right! 🌟


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 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.

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: 6

🧹 Outside diff range and nitpick comments (17)
Sources/AblyChat/Events.swift (1)

5-7: LGTM! Consider aligning the raw value format with MessageEvent.

The addition of the RoomReactionEvents enum is well-structured and consistent with the existing codebase. It's correctly defined as internal and uses a raw string type, which is appropriate for event naming.

For improved consistency with the MessageEvent enum, consider using a dot notation in the raw value:

 internal enum RoomReactionEvents: String {
-    case reaction = "roomReaction"
+    case reaction = "room.reaction"
 }

This change would align the format with MessageEvent.created = "message.created".

Sources/AblyChat/Reaction.swift (2)

6-7: Consider using a documentation comment format.

The comment explaining the Reaction struct is informative. However, consider converting it to a documentation comment format (e.g., /// ... or /** ... */) to enable better integration with documentation tools.

Here's a suggested format:

/// A Reaction corresponds to a single reaction in a chat room.
/// This is analogous to a single user-specified message on an Ably channel
/// (NOTE: not a ProtocolMessage).
///
/// - Note: Implements CHA-ER2
public struct Reaction: Sendable {
    // ...
}

Line range hint 8-13: Consider immutability and computed properties.

The current implementation allows all properties to be mutable. Consider the following suggestions:

  1. Use let instead of var for properties that shouldn't change after initialization (e.g., createdAt, clientID).
  2. The isSelf property could be computed based on the clientID, ensuring consistency.

Here's a potential refactor:

public struct Reaction: Sendable {
    public var type: String
    public var metadata: ReactionMetadata
    public var headers: ReactionHeaders
    public let createdAt: Date
    public let clientID: String
    public var isSelf: Bool {
        // Implement logic to determine if this reaction belongs to the current user
        // This might involve comparing with a stored current user ID
        fatalError("Implementation needed")
    }

    // Update the initializer accordingly
}
Tests/AblyChatTests/DefaultRoomReactionsTests.swift (3)

19-40: LGTM: Reaction sending test is comprehensive

This test thoroughly verifies that reactions are sent in the correct format, aligning with the CHA-ER3a specification. It checks the message name, data, and extras, which is excellent.

Consider adding a test case with empty metadata and headers to ensure the method handles edge cases correctly.


43-55: LGTM with suggestions: Subscription test can be enhanced

This test correctly verifies that the subscribe method returns a non-nil subscription, aligning with the CHA-ER4 specification.

Consider enhancing this test to verify the subscription's functionality:

  1. Mock a reaction event.
  2. Trigger the event through the mock channel.
  3. Assert that the subscription receives the correct reaction data.

This would provide a more comprehensive test of the subscription mechanism.


1-56: Good test coverage with room for enhancement

The test suite provides good coverage of the main functionalities of DefaultRoomReactions, including initialization, sending reactions, and subscribing. The use of mock objects for Realtime and RealtimeChannel is appropriate for unit testing.

Consider adding the following tests to improve coverage:

  1. Test error handling scenarios (e.g., network errors, invalid inputs).
  2. Test with various room IDs to ensure proper handling of different formats.
  3. Test the unsubscribe functionality if available.
  4. Add tests for any public methods not currently covered.

These additions would provide a more comprehensive test suite and increase confidence in the DefaultRoomReactions implementation.

Sources/AblyChat/Room.swift (1)

69-73: LGTM with suggestion: Consider more graceful error handling.

The updated implementation of the reactions property is good. It correctly checks for the availability of reactions and provides a clear error message when accessed inappropriately.

However, while fatalError aligns with the protocol documentation, it might be too severe for production code. Consider using a throwing property or returning an optional instead, which would allow for more graceful error handling at the call site.

Here's a potential alternative implementation:

public nonisolated var reactions: any RoomReactions {
    get throws {
        guard let _reactions else {
            throw ChatError.reactionsNotEnabled
        }
        return _reactions
    }
}

This approach would require defining a ChatError enum and updating the protocol, but it would provide more flexibility in error handling.

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

11-14: LGTM! Consider adding documentation for the new properties.

The addition of these properties enhances the mock object's capabilities for testing. The use of nonisolated(unsafe) is justified for testing purposes.

Consider adding documentation comments for each new property to explain their purpose and usage in tests.


207-210: LGTM! Consider adding a comment about the method's purpose.

The updated publish method correctly stores the last published message details, enhancing the mock object's testing capabilities.

Consider adding a brief comment explaining that this method is used for testing purposes to store the last published message details.


Line range hint 1-238: Overall, the changes enhance the mock object's testing capabilities.

The additions to the MockRealtimeChannel class improve its ability to track and verify message publishing in tests. These changes align well with the PR objectives of enhancing the testing infrastructure.

Consider creating a separate issue to document the testing strategy for the chat functionality, including how these mock objects are used in various test scenarios. This documentation would be valuable for maintaining and expanding the test suite in the future.

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

70-70: Approve the change with a minor suggestion.

This change improves the mock implementation by replacing the fatalError() with a print statement, which is more appropriate for a mock object. It allows for better testing and debugging.

Consider using a logging framework or a debug print function instead of a plain print statement. This would make it easier to control the visibility of these messages in different environments. For example:

Logger.debug("Mock client attached to room with roomID: \(roomID)")

or

debugPrint("Mock client attached to room with roomID: \(roomID)")

154-154: Approve the change with a suggestion for improved safety.

This change enhances the mock implementation by using actual emoji representations of reactions, which aligns well with the PR objectives for Ephemeral Room Reactions. It ensures that only valid reaction types are used in the mock.

To improve safety and avoid potential crashes, consider using optional chaining and providing a default value:

type: ReactionType.allCases.randomElement()?.emoji ?? "👍",

This ensures that even if ReactionType.allCases is empty (which is unlikely but possible), the code won't crash and will use a default emoji.

Sources/AblyChat/DefaultMessages.swift (3)

77-77: LGTM! Consider adding a type annotation for clarity.

The simplification of accessing the headers from the extras dictionary is a good improvement. It removes the unnecessary toJSON() call and simplifies the code.

For improved clarity, consider adding a type annotation to the headers variable:

-let headers = extras["headers"] as? Headers
+let headers: Headers? = extras["headers"] as? Headers

This makes it explicit that Headers is the expected type, which could be helpful for future maintainers.


Line range hint 9-9: Address TODO comments to improve code completeness and clarity.

There are several TODO comments throughout the file that indicate areas needing further work or clarification:

  1. Line 9: Understanding of @MainActor usage.
  2. Line 20: Handling unsubscribing (CHA-M4b).
  3. Line 47: Revisiting errors thrown.
  4. Line 180: Implementing discontinuity events subscription (CHA-M7).

Consider creating separate issues for each TODO item if they don't already exist. This will help track and prioritize these improvements. For the @MainActor usage, it might be beneficial to add a comment explaining its purpose until the issue is resolved.

Also applies to: 20-20, 47-47, 180-180


Line range hint 1-359: Review and potentially refactor error handling approach.

The current error handling approach uses ARTErrorInfo.create consistently throughout the file. While this provides a uniform way of creating errors, it might benefit from a more structured approach.

Consider creating a custom Error enum that conforms to Swift's Error protocol. This would allow for more specific error cases and improved type safety. For example:

enum ChatError: Error {
    case malformedMessage(String)
    case channelAttachmentFailed(String)
    case invalidSubscriptionPoint(String)
    // Add more specific error cases as needed
}

Then, replace ARTErrorInfo.create calls with these custom errors. This approach would make error handling more Swift-idiomatic and easier to manage as the codebase grows.

Sources/AblyChat/DefaultRoomReactions.swift (2)

22-22: Use Swift Dictionary Instead of NSDictionary

For better type safety and adherence to Swift conventions, consider using Swift's native Dictionary type instead of NSDictionary.

Suggested change:

-let extras: NSDictionary = ["headers": params.headers ?? [:]]
+let extras: [String: Any] = ["headers": params.headers ?? [:]]

3-3: Follow Up on TODO Regarding @MainActor Usage

The TODO comment mentions revisiting the @MainActor annotation as part of issue #83. Ensure this is tracked, and the necessary adjustments are made once the issue is addressed.

Would you like assistance in creating a plan to address this issue or help in updating the code once the decision is made?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 27354a8 and e69dd27.

📒 Files selected for processing (12)
  • Example/AblyChatExample/ContentView.swift (5 hunks)
  • Example/AblyChatExample/Mocks/Misc.swift (1 hunks)
  • Example/AblyChatExample/Mocks/MockClients.swift (2 hunks)
  • Sources/AblyChat/DefaultMessages.swift (1 hunks)
  • Sources/AblyChat/DefaultRoomReactions.swift (1 hunks)
  • Sources/AblyChat/Events.swift (1 hunks)
  • Sources/AblyChat/Reaction.swift (1 hunks)
  • Sources/AblyChat/Room.swift (2 hunks)
  • Sources/AblyChat/RoomReactions.swift (1 hunks)
  • Sources/AblyChat/Subscription.swift (1 hunks)
  • Tests/AblyChatTests/DefaultRoomReactionsTests.swift (1 hunks)
  • Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (2 hunks)
🧰 Additional context used
📓 Learnings (1)
Example/AblyChatExample/Mocks/MockClients.swift (4)
Learnt from: maratal
PR: ably-labs/ably-chat-swift#34
File: Example/AblyChatExample/Mocks/MockClients.swift:24-44
Timestamp: 2024-10-08T15:58:47.376Z
Learning: In mock implementations, it's acceptable to leave the `release` method unimplemented during early development.
Learnt from: maratal
PR: ably-labs/ably-chat-swift#34
File: Example/AblyChatExample/Mocks/MockClients.swift:24-44
Timestamp: 2024-09-22T21:36:09.485Z
Learning: In mock implementations, it's acceptable to leave the `release` method unimplemented during early development.
Learnt from: maratal
PR: ably-labs/ably-chat-swift#34
File: Example/AblyChatExample/Mocks/MockClients.swift:367-369
Timestamp: 2024-10-08T15:58:47.376Z
Learning: In mock implementations, it's acceptable to leave `subscribeToDiscontinuities` unimplemented during early development.
Learnt from: maratal
PR: ably-labs/ably-chat-swift#34
File: Example/AblyChatExample/Mocks/MockClients.swift:367-369
Timestamp: 2024-09-22T21:19:09.956Z
Learning: In mock implementations, it's acceptable to leave `subscribeToDiscontinuities` unimplemented during early development.
🔇 Additional comments (17)
Sources/AblyChat/Events.swift (1)

4-7: Consider additional implementation details for Room Reactions

The addition of RoomReactionEvents is a good start for implementing Ephemeral Room Reactions. To ensure a complete implementation:

  1. Verify that other parts of the codebase are updated to handle these new events.
  2. Consider adding documentation comments to explain the usage and purpose of RoomReactionEvents.
  3. Ensure that any relevant interfaces or protocols are updated to include methods for handling room reactions.
  4. Update unit tests to cover scenarios involving room reaction events.

To verify the integration of RoomReactionEvents, you can run the following script:

This script will help identify areas where RoomReactionEvents is being used and where additional implementation or testing might be needed.

✅ Verification successful

RoomReactionEvents Integration Verified Successfully

All necessary implementations and tests for RoomReactionEvents are present and properly integrated.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for usage of RoomReactionEvents and related reaction handling

# Test: Search for RoomReactionEvents usage
echo "Searching for RoomReactionEvents usage:"
rg --type swift "RoomReactionEvents" -C 3

# Test: Search for reaction-related method implementations
echo "\nSearching for reaction-related method implementations:"
rg --type swift "func.*reaction" -C 3

# Test: Search for reaction-related tests
echo "\nSearching for reaction-related tests:"
rg --type swift "test.*reaction" -C 3

Length of output: 4670


Script:

#!/bin/bash
# Description: Perform a comprehensive search for reaction-related tests with various naming conventions.

# Test: Search for test functions containing 'reaction' irrespective of prefix
echo "Searching for reaction-related test functions with any prefix:"
rg --type swift "func.*reaction" -C 3

# Test: Search for XCTest cases related to reactions
echo "\nSearching for XCTest cases related to reactions:"
rg --type swift "XCTestCase.*reaction" -C 3

# Test: Search for any occurrence of 'reaction' in test files
echo "\nSearching for any 'reaction' occurrences in test files:"
rg --type swift "reaction" Tests/ -C 3

Length of output: 11331

Sources/AblyChat/Reaction.swift (3)

3-4: LGTM: Type aliases enhance code clarity.

The introduction of ReactionHeaders and ReactionMetadata type aliases is a good practice. It improves code readability and provides flexibility for future changes if needed.


Line range hint 15-22: LGTM: Initializer is well-implemented.

The initializer correctly sets all properties and provides a public interface for creating Reaction instances. It's well-aligned with the current struct definition.

Note: If the isSelf property becomes computed in the future, remember to update this initializer accordingly.


Line range hint 1-22: Overall assessment: Well-implemented with room for minor enhancements.

The Reaction struct and associated type aliases are well-designed and align with the PR objectives for Ephemeral Room Reactions. The implementation provides a clear structure for representing reactions in the chat system.

Key strengths:

  1. Clear and descriptive type aliases
  2. Comprehensive property set for the Reaction struct
  3. Public interface allowing easy integration

Suggested enhancements:

  1. Use documentation comments for better tooling integration
  2. Consider immutability for certain properties
  3. Evaluate the possibility of making isSelf a computed property

These changes will further improve code quality and maintainability.

Sources/AblyChat/RoomReactions.swift (1)

Line range hint 1-29: Overall implementation aligns with PR objectives.

The addition of the internal extension to SendReactionParams contributes to the finalization of specifications for Ephemeral Room Reactions. The implementation doesn't affect the public API and provides a useful method for converting reaction parameters to query items.

However, ensure that this implementation aligns with the referenced specification document mentioned in the PR objectives. Also, consider if any additional changes are needed to fully support the Ephemeral Room Reactions feature as described in the specification.

To ensure consistency with the specification, please run the following script:

This script will help identify any discrepancies between the implementation and the specification document.

Tests/AblyChatTests/DefaultRoomReactionsTests.swift (1)

6-16: LGTM: Initialization test is well-implemented

This test correctly verifies that the DefaultRoomReactions initializer sets the channel name as expected. It aligns with the CHA-ER1 specification.

Example/AblyChatExample/Mocks/Misc.swift (1)

91-91: Clarify the rationale for changing the displayedText property

The displayedText property in the Reaction extension has been simplified to return the type directly as a string, instead of mapping it to an emoji. While this change simplifies the code and provides more flexibility, it also removes the visual representation of reactions, which could impact the user experience.

Could you please clarify:

  1. Is this change aligned with the specifications for Ephemeral Room Reactions?
  2. How will the UI handle the display of reactions now that they're returned as strings instead of emojis?
  3. Are there any plans to implement a mapping of these string types to visual representations elsewhere in the application?

To understand the impact of this change, let's check how displayedText is used throughout the codebase:

✅ Verification successful

displayedText property usage is consistent after the change

The displayedText property is consistently used as a String throughout the codebase. The showReaction function utilizes displayedText correctly by treating it as a string, and message insertions concatenate it appropriately. There are no detected issues affecting the user experience.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usages of displayedText
rg --type swift 'displayedText'

Length of output: 466


Script:

#!/bin/bash
# Extract the implementation of showReaction in ContentView.swift
ast-grep --lang swift --pattern $'func showReaction($_) { $$$ }' Example/AblyChatExample/ContentView.swift

# Extract the message insertion and rendering logic in ContentView.swift
rg --type swift 'messages\.insert|BasicListItem'

Length of output: 3501


Script:

#!/bin/bash
# Extract SwiftUI views or UIKit components that render BasicListItem.text
rg --type swift 'BasicListItem' | grep -E 'View|Cell' | uniq

# Alternatively, search for where BasicListItem.text is used in UI rendering
rg --type swift '\.text\s*='

Length of output: 949

Sources/AblyChat/Room.swift (3)

26-26: LGTM: New private property for reactions.

The addition of the _reactions property is well-implemented. It's correctly declared as private and uses an optional type, which aligns with the possibility that reactions might not be available for all rooms.


57-61: LGTM: Proper initialization of reactions.

The conditional initialization of _reactions is well-implemented. It correctly uses the options parameter to determine if reactions should be enabled, and the asynchronous initialization of DefaultRoomReactions is handled appropriately.


Line range hint 1-138: Overall assessment: Good implementation with minor improvement suggestions.

The changes in this file effectively implement the specifications for Ephemeral Room Reactions. The addition of the _reactions property and its conditional initialization, along with the updated reactions property implementation, provide a clear and structured approach to handling room reactions.

Key points:

  1. The new _reactions property is well-encapsulated and correctly initialized.
  2. The reactions property now includes proper checks for reaction availability.
  3. Error handling has been improved, though there's potential for further refinement to make it more production-friendly.

These changes align well with the PR objectives and enhance the overall structure and functionality of the chat application.

Sources/AblyChat/Subscription.swift (3)

Line range hint 1-83: Overall assessment: Good changes with room for minor improvements

The changes to the Subscription struct enhance its flexibility and align well with the PR objectives of supporting both functional and mock implementations of the Chat app. The addition of the finish() method and the updates to the AsyncIterator struct are well-implemented.

However, there are opportunities for improvement in error handling, particularly for the mock mode, and in documentation. Implementing the suggested changes will further improve the code's maintainability and usability.

Despite these minor suggestions, the overall implementation is solid and achieves the intended goals of the PR.


Line range hint 1-83: Overall structure improvements and documentation suggestions

The changes to the Subscription struct enhance its flexibility by supporting both real and mock implementations, which aligns well with the PR objectives. However, there are a few suggestions for improvement:

  1. Error handling in mock mode:
    Consider implementing a consistent error handling strategy across the struct for mock mode operations. This could involve creating custom errors and using throws instead of fatalError.

  2. TODO and future changes:
    The TODO comment suggests upcoming changes related to unsubscribing. It might be beneficial to create a GitHub issue (if not already done) to track this future work and link it in the comment.

  3. AsyncIterator documentation:
    The implementation of AsyncIterator looks correct, but it could benefit from additional comments explaining the different modes and their purposes.

Consider adding documentation to the AsyncIterator:

public struct AsyncIterator: AsyncIteratorProtocol {
    /// Represents the mode of iteration, either using the default AsyncStream
    /// or a mock AsyncSequence provided for testing purposes.
    fileprivate enum Mode {
        /// Default mode using AsyncStream for real-time data.
        case `default`(iterator: AsyncStream<Element>.AsyncIterator)
        /// Mock mode using a provided AsyncSequence for testing.
        case mockAsyncSequence(iterator: AnyNonThrowingAsyncSequence.AsyncIterator)

        // ... rest of the implementation ...
    }

    // ... rest of the struct ...
}

To ensure comprehensive documentation, please run the following script:

#!/bin/bash
# Check for public entities without documentation comments
ast-grep --lang swift --pattern $'public $_ $name($_) {
  $$$
}'

This will help identify any public entities that might need additional documentation.


74-82: 🛠️ Refactor suggestion

Consider refining the finish() method implementation

The new finish() method is a good addition for signaling the end of a subscription. However, there are a few points to consider:

  1. The fatal error in the mock mode might be too severe. Consider throwing a custom error instead, which would allow for more graceful error handling in client code.

  2. The TODO comment suggests that this implementation might need further revision. Ensure that the current implementation aligns with the requirements mentioned (CHA-M4b & CHA-ER4b).

  3. The method is public, which is good for exposing it to all Subscription types as mentioned in the TODO.

Consider refactoring the mock mode handling:

public func finish() throws {
    switch mode {
    case let .default(_, continuation):
        continuation.finish()
    case .mockAsyncSequence:
        throw SubscriptionError.cannotFinishMockSubscription
    }
}

// Add this enum to your error definitions
public enum SubscriptionError: Error {
    case cannotFinishMockSubscription
}

This approach allows for more flexible error handling in client code.

To ensure this change doesn't break existing functionality, please run the following script:

This will help identify any places where the current implementation is being used and might need updating.

✅ Verification successful

Verification Successful: finish() Method Usage Confirmed

The finish() method in Sources/AblyChat/Subscription.swift is only invoked within its own implementation. This confirms that modifying the method to throw a custom error instead of calling fatalError will not affect other parts of the codebase.

No other usages of finish() were found, ensuring that the proposed change aligns with the current implementation and improves error handling without introducing issues.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any existing uses of the `finish()` method
rg --type swift 'finish\(\)' -C 3

Length of output: 517


Script:

#!/bin/bash
# Corrected ast-grep pattern to find the public `finish` method in Swift
ast-grep --lang swift --pattern 'public func finish() {
  $$$
}'

Length of output: 672


Script:

#!/bin/bash
# Broad search for any usage of `finish` method with optional whitespace
rg --type swift '\bfinish\s*\(\)' -A 3

Length of output: 580

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

Line range hint 1-438: Overall assessment of MockClients.swift

The changes in this file improve the mock implementations for the chat application, aligning well with the PR objectives. These improvements will facilitate better testing and debugging, supporting both functional and mock implementations of the Chat app.

The modifications enhance the realism of the mocks and provide better debugging information. The suggestions provided in the previous comments will further improve safety and logging practices.

Great work on refining these mock implementations!

Sources/AblyChat/DefaultMessages.swift (1)

Line range hint 1-359: Consider adding more unit tests to cover complex asynchronous operations.

The file contains complex asynchronous operations and channel event handling. While the PR description mentions that unit tests have been added, it also notes that they are not exhaustive.

To ensure the reliability and correctness of these operations, consider adding more comprehensive unit tests. Focus on testing:

  1. Different channel states and transitions.
  2. Error scenarios in asynchronous operations.
  3. Edge cases in message handling and subscription management.

Here's a script to check the current test coverage for this file:

This script will help identify existing tests and potential gaps in test coverage.

Sources/AblyChat/DefaultRoomReactions.swift (1)

62-62: Ensure Safe Comparison of Client IDs

The comparison message.clientId == realtime.clientId assumes that realtime.clientId is not nil. If realtime.clientId can be nil, this comparison may lead to unexpected behavior.

Please verify that realtime.clientId is always non-nil or modify the comparison to handle optional values safely.

Example/AblyChatExample/ContentView.swift (1)

44-44: ⚠️ Potential issue

Avoid hardcoding API keys in the code

Hardcoding API keys directly in the source code can lead to security vulnerabilities. It's recommended to use environment variables or a secure configuration file to manage sensitive data like API keys.

Consider updating the code as follows:

- options.key = "4dypbA.0N1o5A:I0GPeR-Sh15rJwPLPURx9kE27hwPqAVZnjcVO3T2BlU"
+ options.key = ProcessInfo.processInfo.environment["ABLY_API_KEY"] ?? ""

Make sure to set the ABLY_API_KEY in your environment variables when running the application.

⛔ Skipped due to learnings
Learnt from: maratal
PR: ably-labs/ably-chat-swift#34
File: Example/AblyChatExample/ContentView.swift:35-118
Timestamp: 2024-10-08T15:58:47.376Z
Learning: Skip code review suggestions for `ContentView.swift` in the `Example/AblyChatExample` project.
Learnt from: maratal
PR: ably-labs/ably-chat-swift#34
File: Example/AblyChatExample/ContentView.swift:35-118
Timestamp: 2024-09-22T21:32:01.793Z
Learning: Skip code review suggestions for `ContentView.swift` in the `Example/AblyChatExample` project.

Sources/AblyChat/RoomReactions.swift Show resolved Hide resolved
Sources/AblyChat/DefaultRoomReactions.swift Show resolved Hide resolved
Sources/AblyChat/DefaultRoomReactions.swift Outdated Show resolved Hide resolved
Sources/AblyChat/DefaultRoomReactions.swift Outdated Show resolved Hide resolved
Sources/AblyChat/DefaultRoomReactions.swift Outdated Show resolved Hide resolved
Example/AblyChatExample/ContentView.swift Outdated Show resolved Hide resolved
@umair-ably umair-ably force-pushed the ECO-4945 branch 2 times, most recently from 7ed89eb to d661732 Compare October 18, 2024 11:08
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.

Caution

Inline review comments failed to post

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (10)
Tests/AblyChatTests/DefaultRoomReactionsTests.swift (3)

19-40: Suggestion: Consider a more descriptive test name

The test thoroughly verifies that reactions are sent in the correct format, including checks for event name, data, and extras. It aligns well with the CHA-ER3a specification.

To improve clarity, consider renaming the test to reflect all aspects being verified. For example:

func reactionsAreSentWithCorrectEventNameDataAndExtras() async throws

This name more accurately describes the full scope of what's being tested.


43-55: Suggestion: Enhance subscription verification

The test correctly verifies that the subscribe method returns a non-nil subscription, aligning with the CHA-ER4 specification. To make the test more robust, consider adding additional checks on the returned subscription. For example:

// Then
#expect(subscription != nil)
#expect(subscription?.bufferingPolicy == .unbounded)
// Add more checks as appropriate, e.g., verifying the channel, event name, etc.

These additional checks would provide more confidence in the correctness of the returned subscription.


1-56: Suggestion: Consider adding more test scenarios

The test file provides good coverage of the main functionalities of DefaultRoomReactions, aligning with the specified requirements (CHA-ER1, CHA-ER3a, CHA-ER4). The use of mock objects effectively isolates the tests from external dependencies.

To further improve the test suite, consider adding the following scenarios:

  1. Error handling: Test how DefaultRoomReactions behaves when the realtime connection fails or when sending a reaction fails.
  2. Edge cases: Test with empty or very large reaction data.
  3. Multiple subscriptions: Verify behavior when multiple subscriptions are created.
  4. Unsubscribe functionality: Test that unsubscribing works correctly.

These additional tests would provide more comprehensive coverage of the DefaultRoomReactions class.

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

11-14: Consider alternatives to nonisolated(unsafe)

While the comment suggests that nonisolated(unsafe) is acceptable in this testing context, it's generally a good practice to avoid unsafe code when possible. Consider using a thread-safe alternative or explaining the rationale for using nonisolated(unsafe) in more detail.

A safer alternative could be to use a concurrent data structure or a serial dispatch queue to manage access to these properties. For example:

private let queue = DispatchQueue(label: "com.mockRealtimeChannel.messageQueue")
private var _lastMessagePublishedName: String?
private var _lastMessagePublishedData: Any?
private var _lastMessagePublishedExtras: (any ARTJsonCompatible)?

var lastMessagePublishedName: String? {
    queue.sync { _lastMessagePublishedName }
}

// Similar getters for other properties

This approach ensures thread-safety without resorting to unsafe code.


207-210: LGTM with a minor suggestion

The implementation correctly stores the last published message details, which is useful for testing. However, for consistency with other methods in this mock class, consider adding a fatalError for any unimplemented edge cases.

You might want to add a fatalError at the end of the method for any potential future extensions:

func publish(_ name: String?, data: Any?, extras: (any ARTJsonCompatible)?) {
    lastMessagePublishedName = name
    lastMessagePublishedExtras = extras
    lastMessagePublishedData = data
    // For any future extensions
    fatalError("Any additional functionality not implemented")
}

This maintains consistency with other methods in the class and makes it clear that any additional functionality beyond storing the message details is not yet implemented.

Example/AblyChatExample/ContentView.swift (5)

15-24: LGTM! Consider making the mode configurable.

The introduction of the mode-based system with the Environment enum is a good approach, aligning well with the PR objectives. It allows for both mock and live implementations, which is great for testing and development.

A few suggestions:

  1. Consider making the mode configurable, perhaps through an environment variable or a build configuration. This would allow easier switching between mock and live modes without code changes.
  2. Can you clarify the intention behind the default roomID? Is "DemoRoomID" meant to be used in all cases, or should it be configurable as well?

25-32: LGTM! Consider consistent state management.

The separation of mock and live clients is well-implemented, aligning with the mode-based system. This approach provides flexibility for testing and live usage.

Suggestion for consistency:
Consider using @State for mockChatClient as well, similar to liveChatClient. This would provide consistent state management for both client types:

@State private var mockChatClient: MockChatClient

Then initialize it in the init() method alongside liveChatClient.


127-137: LGTM! Clear separation of mock and live functionalities.

The changes effectively implement mode-based task execution, aligning well with the PR objectives. The addition of attachRoom() ensures proper room attachment before other operations, which is a good practice.

Suggestion for improved clarity:
Consider extracting the mock-specific tasks into a separate function for better readability:

func executeMockTasks() {
    tryTask { try await showPresence() }
    tryTask { try await showTypings() }
    tryTask { try await showOccupancy() }
    tryTask { try await showRoomStatus() }
}

// In the body:
if mode == .mock {
    zStack.tryTask { try await executeMockTasks() }
}

This approach would make it easier to manage and update mock-specific functionalities in the future.


156-159: LGTM! Good addition for room attachment.

The new attachRoom() function is a valuable addition, ensuring that the room is properly attached before other operations are performed. This improves the reliability of the chat functionality.

Suggestion for improvement:
Consider adding error handling or retrying logic within this function. For example:

func attachRoom() async throws {
    let maxAttempts = 3
    var attempts = 0
    while attempts < maxAttempts {
        do {
            try await room().attach()
            return
        } catch {
            attempts += 1
            if attempts == maxAttempts {
                throw error
            }
            try await Task.sleep(nanoseconds: 1_000_000_000) // Wait 1 second before retrying
        }
    }
}

This would make the room attachment process more robust in case of temporary network issues.


Line range hint 161-183: LGTM! Improved real-time functionality for messages and reactions.

The updates to showMessages() and showReactions() functions significantly improve the real-time capabilities of the chat application. The use of subscriptions for both messages and reactions aligns well with the expected behavior of a modern chat application. The addition of handling previous messages is particularly valuable for providing chat history to users.

Suggestion for improvement:
Consider adding error handling and potentially a retry mechanism for the subscriptions. For example:

func showMessages() async throws {
    while true {
        do {
            let messagesSubscription = try await room().messages.subscribe(bufferingPolicy: .unbounded)
            // ... rest of the function ...
        } catch {
            print("Error in message subscription: \(error)")
            try await Task.sleep(nanoseconds: 5_000_000_000) // Wait 5 seconds before retrying
        }
    }
}

This would make the application more resilient to temporary network issues or service disruptions.

🛑 Comments failed to post (1)
Example/AblyChatExample/ContentView.swift (1)

41-49: ⚠️ Potential issue

LGTM! Consider mode-based initialization and API key management.

The initialization of both liveRealtime and liveChatClient is correctly implemented. However, there are a few points to consider:

  1. The API key and client ID are currently empty. This will cause issues when running in live mode. Consider adding a warning comment or, preferably, implement a secure method to inject these values (e.g., through environment variables or a secure configuration file).

  2. The initialization is currently unconditional. For better performance and resource management, consider conditionally initializing based on the mode:

init() {
    if mode == .live {
        let options = ARTClientOptions()
        options.key = "" // TODO: Set this securely
        options.clientId = "" // TODO: Set this securely
        liveRealtime = ARTRealtime(options: options)
        _liveChatClient = State(initialValue: DefaultChatClient(realtime: liveRealtime, clientOptions: .init()))
    } else {
        // Mock initialization if needed
    }
}

This approach ensures that live resources are only initialized when necessary.

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 minor questions

Sources/AblyChat/DefaultRoomReactions.swift Outdated Show resolved Hide resolved
@lawrence-forooghian
Copy link
Collaborator

@umair-ably I’ve just merged #100, which adds a basic integration test to the library; I think it would be cool if you could expand on this to do some basic testing of reactions

@lawrence-forooghian
Copy link
Collaborator

@umair-ably I’ve just merged #103, which changes the way in which the features get hold of their channels (namely, they now receive them in their initializer); this will need to be applied for reactions too, else the reactions channel won't get attached.

@umair-ably umair-ably force-pushed the ECO-4945 branch 5 times, most recently from 3b3bc5b to 11323e6 Compare November 12, 2024 14:23
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: 5

🧹 Outside diff range and nitpick comments (9)
Sources/AblyChat/DefaultRoomReactions.swift (2)

25-73: Consider extracting message validation logic

The message validation logic in the subscribe method is quite complex. Consider extracting it into a separate private method for better readability and maintainability.

Here's a suggested refactor:

+    private func validateAndCreateReaction(from message: ARTMessage, clientID: String) throws -> Reaction {
+        guard let data = message.data as? [String: Any],
+              let reactionType = data["type"] as? String
+        else {
+            throw ARTErrorInfo.create(withCode: 50000, status: 500, message: "Received incoming message without data or text")
+        }
+
+        guard let messageClientID = message.clientId else {
+            throw ARTErrorInfo.create(withCode: 50000, status: 500, message: "Received incoming message without clientId")
+        }
+
+        guard let timestamp = message.timestamp else {
+            throw ARTErrorInfo.create(withCode: 50000, status: 500, message: "Received incoming message without timestamp")
+        }
+
+        guard let extras = try message.extras?.toJSON() else {
+            throw ARTErrorInfo.create(withCode: 50000, status: 500, message: "Received incoming message without extras")
+        }
+
+        let metadata = data["metadata"] as? Metadata
+        let headers = extras["headers"] as? Headers
+
+        return Reaction(
+            type: reactionType,
+            metadata: metadata ?? .init(),
+            headers: headers ?? .init(),
+            createdAt: timestamp,
+            clientID: messageClientID,
+            isSelf: messageClientID == clientID
+        )
+    }

 internal func subscribe(bufferingPolicy: BufferingPolicy) async -> Subscription<Reaction> {
     let subscription = Subscription<Reaction>(bufferingPolicy: bufferingPolicy)

     channel.subscribe(RoomReactionEvents.reaction.rawValue) { [clientID, logger] message in
         Task {
             do {
-                // Current validation logic
+                let reaction = try validateAndCreateReaction(from: message, clientID: clientID)
                 subscription.emit(reaction)
             } catch {
                 logger.log(message: "Error processing incoming reaction message: \(error)", level: .error)
             }
         }
     }

     return subscription
 }

80-82: Remove unused error case

The noReferenceToSelf case in RoomReactionsError is not used anywhere in the code.

Consider removing it if it's not needed:

-    private enum RoomReactionsError: Error {
-        case noReferenceToSelf
-    }
Tests/AblyChatTests/IntegrationTests.swift (2)

75-84: Consider expanding reaction test scenarios

While the basic reaction test is good, consider adding these scenarios to improve coverage:

  • Multiple reaction types
  • Multiple reactions to the same message
  • Error cases (e.g., invalid reaction types)
  • Concurrent reactions

Example additional test:

// Test multiple reactions
try await txRoom.reactions.send(params: .init(type: "heart"))
try await txRoom.reactions.send(params: .init(type: "thumbsup"))
let reactions = try await rxReactionSubscription.collect(2)
#expect(reactions.map(\.type).sorted() == ["heart", "thumbsup"])

// Test invalid reaction
do {
    try await txRoom.reactions.send(params: .init(type: "invalid_reaction"))
    XCTFail("Expected error for invalid reaction")
} catch {
    // Assert error type
}

Line range hint 1-106: Consider comprehensive test coverage strategy

Given that unit tests aren't exhaustive (as mentioned in PR objectives), this integration test could be expanded to provide additional coverage. Consider creating a separate integration test method specifically for comprehensive reactions testing scenarios, keeping this basic integration test focused on the happy path.

This would help maintain a clear separation between:

  1. Basic integration verification (current test)
  2. Comprehensive reactions testing (new test)
  3. Unit tests (to be expanded in issue Add more tests for room reactions  #88)
Sources/AblyChat/Subscription.swift (1)

75-82: Consider enhancing error handling and documentation.

The implementation looks good, but consider these improvements:

  1. Add XML documentation comments to describe the method's purpose and thread-safety guarantees
  2. Make the fatal error message more descriptive about why mock sequences don't support finishing

Here's a suggested improvement:

+    /// Signals that the subscription has finished emitting elements.
+    ///
+    /// - Note: This method is thread-safe and can be called from any thread.
+    /// - Important: This method cannot be called on mock subscriptions as they manage their own lifecycle.
     public func finish() {
         switch mode {
         case let .default(_, continuation):
             continuation.finish()
         case .mockAsyncSequence:
-            fatalError("`finish` cannot be called on a Subscription that was created using init(mockAsyncSequence:)")
+            fatalError("`finish` cannot be called on a mock Subscription as it follows the lifecycle of its source AsyncSequence")
         }
     }
Tests/AblyChatTests/DefaultRoomTests.swift (1)

Line range hint 1-194: Enhance test coverage for reactions-related scenarios.

While the tests cover basic channel setup, consider adding test cases for:

  1. Reaction channel attachment failures
  2. Reaction channel state transitions
  3. Error scenarios when working with reactions

This aligns with the PR objectives mentioning that test coverage isn't exhaustive yet.

Would you like me to help generate additional test cases for these scenarios?

Example/AblyChatExample/ContentView.swift (3)

15-16: Consider making roomID configurable

The hardcoded room ID might not be suitable for production use. Consider making it configurable through environment variables or initialization parameters.

-    private let roomID = "DemoRoomID"
+    private let roomID: String
+
+    init(roomID: String = "DemoRoomID") {
+        self.roomID = roomID
+        // ... rest of init
+    }

162-163: Consider adding bounds to message buffering

Using .unbounded buffering policy could lead to memory issues with high message volume. Consider:

  1. Using a bounded buffer size
  2. Implementing pagination for previous messages
-        let messagesSubscription = try await room().messages.subscribe(bufferingPolicy: .unbounded)
+        let messagesSubscription = try await room().messages.subscribe(bufferingPolicy: .bounded(maxSize: 100))

Line range hint 179-184: Add error handling for reaction subscription

The reaction subscription lacks error handling, which could lead to silent failures. Consider adding error handling and retry logic.

     func showReactions() async throws {
-        let reactionSubscription = try await room().reactions.subscribe(bufferingPolicy: .unbounded)
-        for await reaction in reactionSubscription {
-            withAnimation {
-                showReaction(reaction.displayedText)
+        do {
+            let reactionSubscription = try await room().reactions.subscribe(bufferingPolicy: .bounded(maxSize: 50))
+            for try await reaction in reactionSubscription {
+                withAnimation {
+                    showReaction(reaction.displayedText)
+                }
             }
+        } catch {
+            print("Failed to subscribe to reactions: \(error)")
+            // Consider implementing retry logic here
         }
     }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 7ed89eb and 11323e6.

📒 Files selected for processing (15)
  • Example/AblyChatExample/ContentView.swift (5 hunks)
  • Example/AblyChatExample/Mocks/Misc.swift (1 hunks)
  • Example/AblyChatExample/Mocks/MockClients.swift (2 hunks)
  • Sources/AblyChat/DefaultMessages.swift (2 hunks)
  • Sources/AblyChat/DefaultRoomReactions.swift (1 hunks)
  • Sources/AblyChat/Events.swift (1 hunks)
  • Sources/AblyChat/Reaction.swift (1 hunks)
  • Sources/AblyChat/Room.swift (3 hunks)
  • Sources/AblyChat/RoomFeature.swift (1 hunks)
  • Sources/AblyChat/RoomReactions.swift (1 hunks)
  • Sources/AblyChat/Subscription.swift (1 hunks)
  • Tests/AblyChatTests/DefaultRoomReactionsTests.swift (1 hunks)
  • Tests/AblyChatTests/DefaultRoomTests.swift (6 hunks)
  • Tests/AblyChatTests/IntegrationTests.swift (4 hunks)
  • Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
  • Example/AblyChatExample/Mocks/Misc.swift
  • Example/AblyChatExample/Mocks/MockClients.swift
  • Sources/AblyChat/DefaultMessages.swift
  • Sources/AblyChat/Events.swift
  • Sources/AblyChat/Reaction.swift
  • Sources/AblyChat/Room.swift
  • Sources/AblyChat/RoomReactions.swift
  • Tests/AblyChatTests/DefaultRoomReactionsTests.swift
  • Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
🧰 Additional context used
📓 Learnings (1)
Sources/AblyChat/DefaultRoomReactions.swift (1)
Learnt from: umair-ably
PR: ably-labs/ably-chat-swift#89
File: Sources/AblyChat/DefaultRoomReactions.swift:73-75
Timestamp: 2024-11-10T17:12:32.395Z
Learning: In the 'ably-chat-swift' repository (Swift project), always ignore giving feedback on usage of `fatalError("Not implemented")` in the code, including for methods like `subscribeToDiscontinuities()`.
🔇 Additional comments (9)
Sources/AblyChat/RoomFeature.swift (3)

18-19: LGTM: Clear and consistent channel suffix for typing indicators

The implementation provides a clear suffix for typing indicator channels, maintaining consistency with the channel naming pattern.


20-22: LGTM: Reactions implementation aligns with specification

The implementation correctly follows the specification point CHA-ER1, using the appropriate channel suffix for reactions. The comment provides excellent documentation by referencing the spec and including an example.


23-26: Verify unimplemented features handling

The error handling for unimplemented features is appropriate, using fatalError with a descriptive message. However, since PR objectives mention that CHA-ER3b and CHA-ER3c are intentionally not included, let's verify this aligns with the ADR.

Tests/AblyChatTests/IntegrationTests.swift (2)

Line range hint 25-36: LGTM: Setup section properly configured for reactions testing

The changes appropriately initialize both transmitting and receiving rooms with reaction capabilities, maintaining consistency between both clients.


94-105: Verify reactions cleanup after room release

Consider adding verification that reactions are properly cleaned up after room release. This ensures no lingering subscriptions or data remain.

Add these verifications after release:

// Verify reactions are cleaned up
let postReleaseRxReactionSubscription = await postReleaseRxRoom.reactions.subscribe(bufferingPolicy: .unbounded)
#expect(await postReleaseRxReactionSubscription.isEmpty)

// Verify previous reactions aren't accessible
let postReleaseReactions = try await postReleaseRxRoom.reactions.getHistory()
#expect(postReleaseReactions.isEmpty)
Sources/AblyChat/Subscription.swift (2)

75-82: Verify integration with room reactions functionality.

Since this is part of the Ephemeral Room Reactions feature, we should verify how the finish() method integrates with reaction handling.

#!/bin/bash
# Search for reaction-related usage of Subscription
echo "Searching for reaction-related files..."
rg -l "Reaction" --type swift

echo "Checking usage of finish() method in reaction context..."
ast-grep --pattern 'finish()' --lang swift

echo "Checking for potential subscription lifecycle handling..."
rg -A 5 "Subscription.*finish"

74-74: Verify the timeline for implementing unsubscription functionality.

The TODO comment references issue #36 for implementing unsubscription functionality. This seems important for completing the specifications mentioned in CHA-M4b & CHA-ER4b.

Example/AblyChatExample/ContentView.swift (2)

52-53: LGTM! Clean implementation of client selection

The room selection logic is well-implemented, providing a clean switch between mock and live clients.


130-138: LGTM! Safe handling of mock-only features

The implementation properly guards mock-only features, preventing runtime crashes in live mode. This successfully addresses the previous review comment about fatal errors.

Sources/AblyChat/DefaultRoomReactions.swift Show resolved Hide resolved
Sources/AblyChat/DefaultRoomReactions.swift Outdated Show resolved Hide resolved
Tests/AblyChatTests/DefaultRoomTests.swift Show resolved Hide resolved
Tests/AblyChatTests/DefaultRoomTests.swift Show resolved Hide resolved
Example/AblyChatExample/ContentView.swift Show resolved Hide resolved
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

Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian left a comment

Choose a reason for hiding this comment

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

My review wasn't very in-depth, just a handful of comments 👍

Sources/AblyChat/RoomFeature.swift Outdated Show resolved Hide resolved
Tests/AblyChatTests/IntegrationTests.swift Outdated Show resolved Hide resolved
@lawrence-forooghian
Copy link
Collaborator

I’ve just merged #108, which implements subscribeToDiscontinuities for Messages (CHA-M7). Could you please update here to do the same for reactions (CHA-ER5)? You should be able to copy what I did there (make Reactions take a FeatureChannel, call through to its subscribeToDiscontinuities, add a test), but let me know if you have any questions 🙂

@umair-ably
Copy link
Collaborator Author

@coderabbitai pause

@umair-ably
Copy link
Collaborator Author

I’ve just merged #108, which implements subscribeToDiscontinuities for Messages (CHA-M7). Could you please update here to do the same for reactions (CHA-ER5)? You should be able to copy what I did there (make Reactions take a FeatureChannel, call through to its subscribeToDiscontinuities, add a test), but let me know if you have any questions 🙂

done

Copy link

coderabbitai bot commented Nov 14, 2024

✅ Actions performed

Reviews paused.

Tests/AblyChatTests/DefaultRoomReactionsTests.swift Outdated Show resolved Hide resolved
Sources/AblyChat/RoomFeature.swift Outdated Show resolved Hide resolved
Tests/AblyChatTests/IntegrationTests.swift Outdated Show resolved Hide resolved
`CHA-ER3b` & `CHA-ER3c` have not been implemented despite being outlined in [1], as per the ADR at [2].
Example app has also been updated to support both a working and mock implementation of the Chat app.

[1] - https://sdk.ably.com/builds/ably/specification/pull/200/chat-features/
[2] - https://ably.atlassian.net/wiki/spaces/CHA/pages/3438116905/CHADR-066+Removing+Reserved+Keyspace#Solution
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants