[ECO-4945] Spec complete for Ephemeral Room Reactions #89
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe 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 Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 withMessageEvent.The addition of the
RoomReactionEventsenum 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
MessageEventenum, 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
Reactionstruct 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:
- Use
letinstead ofvarfor properties that shouldn't change after initialization (e.g.,createdAt,clientID).- The
isSelfproperty could be computed based on theclientID, 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 comprehensiveThis 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 enhancedThis test correctly verifies that the
subscribemethod returns a non-nil subscription, aligning with the CHA-ER4 specification.Consider enhancing this test to verify the subscription's functionality:
- Mock a reaction event.
- Trigger the event through the mock channel.
- 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 enhancementThe 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:
- Test error handling scenarios (e.g., network errors, invalid inputs).
- Test with various room IDs to ensure proper handling of different formats.
- Test the unsubscribe functionality if available.
- Add tests for any public methods not currently covered.
These additions would provide a more comprehensive test suite and increase confidence in the
DefaultRoomReactionsimplementation.Sources/AblyChat/Room.swift (1)
69-73: LGTM with suggestion: Consider more graceful error handling.The updated implementation of the
reactionsproperty is good. It correctly checks for the availability of reactions and provides a clear error message when accessed inappropriately.However, while
fatalErroraligns 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
ChatErrorenum 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
publishmethod 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
MockRealtimeChannelclass 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
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.allCasesis 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
extrasdictionary is a good improvement. It removes the unnecessarytoJSON()call and simplifies the code.For improved clarity, consider adding a type annotation to the
headersvariable:-let headers = extras["headers"] as? Headers +let headers: Headers? = extras["headers"] as? HeadersThis makes it explicit that
Headersis 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:
- Line 9: Understanding of
@MainActorusage.- Line 20: Handling unsubscribing (CHA-M4b).
- Line 47: Revisiting errors thrown.
- 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
@MainActorusage, 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.createconsistently throughout the file. While this provides a uniform way of creating errors, it might benefit from a more structured approach.Consider creating a custom
Errorenum that conforms to Swift'sErrorprotocol. 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.createcalls 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 SwiftDictionaryInstead ofNSDictionaryFor better type safety and adherence to Swift conventions, consider using Swift's native
Dictionarytype instead ofNSDictionary.Suggested change:
-let extras: NSDictionary = ["headers": params.headers ?? [:]] +let extras: [String: Any] = ["headers": params.headers ?? [:]]
3-3: Follow Up on TODO Regarding@MainActorUsageThe TODO comment mentions revisiting the
@MainActorannotation 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
📒 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 ReactionsThe addition of
RoomReactionEventsis a good start for implementing Ephemeral Room Reactions. To ensure a complete implementation:
- Verify that other parts of the codebase are updated to handle these new events.
- Consider adding documentation comments to explain the usage and purpose of
RoomReactionEvents.- Ensure that any relevant interfaces or protocols are updated to include methods for handling room reactions.
- 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
RoomReactionEventsis being used and where additional implementation or testing might be needed.✅ Verification successful
RoomReactionEvents Integration Verified Successfully
All necessary implementations and tests for
RoomReactionEventsare 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 3Length 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 3Length of output: 11331
Sources/AblyChat/Reaction.swift (3)
3-4: LGTM: Type aliases enhance code clarity.The introduction of
ReactionHeadersandReactionMetadatatype 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
Reactioninstances. It's well-aligned with the current struct definition.Note: If the
isSelfproperty 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
Reactionstruct 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:
- Clear and descriptive type aliases
- Comprehensive property set for the
Reactionstruct- Public interface allowing easy integration
Suggested enhancements:
- Use documentation comments for better tooling integration
- Consider immutability for certain properties
- Evaluate the possibility of making
isSelfa computed propertyThese 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
SendReactionParamscontributes 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-implementedThis test correctly verifies that the
DefaultRoomReactionsinitializer 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 thedisplayedTextpropertyThe
displayedTextproperty in theReactionextension has been simplified to return thetypedirectly 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:
- Is this change aligned with the specifications for Ephemeral Room Reactions?
- How will the UI handle the display of reactions now that they're returned as strings instead of emojis?
- 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
displayedTextis used throughout the codebase:✅ Verification successful
displayedTextproperty usage is consistent after the changeThe
displayedTextproperty is consistently used as aStringthroughout the codebase. TheshowReactionfunction utilizesdisplayedTextcorrectly 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
_reactionsproperty 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
_reactionsis well-implemented. It correctly uses theoptionsparameter to determine if reactions should be enabled, and the asynchronous initialization ofDefaultRoomReactionsis 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
_reactionsproperty and its conditional initialization, along with the updatedreactionsproperty implementation, provide a clear and structured approach to handling room reactions.Key points:
- The new
_reactionsproperty is well-encapsulated and correctly initialized.- The
reactionsproperty now includes proper checks for reaction availability.- 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 improvementsThe changes to the
Subscriptionstruct enhance its flexibility and align well with the PR objectives of supporting both functional and mock implementations of the Chat app. The addition of thefinish()method and the updates to theAsyncIteratorstruct 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 suggestionsThe changes to the
Subscriptionstruct 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:
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 usingthrowsinstead offatalError.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.
AsyncIteratordocumentation:
The implementation ofAsyncIteratorlooks 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 suggestionConsider refining the
finish()method implementationThe new
finish()method is a good addition for signaling the end of a subscription. However, there are a few points to consider:
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.
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).
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 ConfirmedThe
finish()method inSources/AblyChat/Subscription.swiftis only invoked within its own implementation. This confirms that modifying the method to throw a custom error instead of callingfatalErrorwill 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 3Length 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 3Length of output: 580
Example/AblyChatExample/Mocks/MockClients.swift (1)
Line range hint
1-438: Overall assessment of MockClients.swiftThe 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:
- Different channel states and transitions.
- Error scenarios in asynchronous operations.
- 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 IDsThe comparison
message.clientId == realtime.clientIdassumes thatrealtime.clientIdis notnil. Ifrealtime.clientIdcan benil, this comparison may lead to unexpected behavior.Please verify that
realtime.clientIdis always non-nil or modify the comparison to handle optional values safely.Example/AblyChatExample/ContentView.swift (1)
44-44:⚠️ Potential issueAvoid 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_KEYin 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.
umair-ably
force-pushed
the
ECO-4945
branch
2 times, most recently
from
7ed89eb
to
d661732
Compare
There was a problem hiding this comment.
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 nameThe 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 throwsThis name more accurately describes the full scope of what's being tested.
43-55: Suggestion: Enhance subscription verificationThe test correctly verifies that the
subscribemethod 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 scenariosThe 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:
- Error handling: Test how
DefaultRoomReactionsbehaves when the realtime connection fails or when sending a reaction fails.- Edge cases: Test with empty or very large reaction data.
- Multiple subscriptions: Verify behavior when multiple subscriptions are created.
- Unsubscribe functionality: Test that unsubscribing works correctly.
These additional tests would provide more comprehensive coverage of the
DefaultRoomReactionsclass.Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (2)
11-14: Consider alternatives tononisolated(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 usingnonisolated(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 propertiesThis approach ensures thread-safety without resorting to unsafe code.
207-210: LGTM with a minor suggestionThe 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
fatalErrorfor any unimplemented edge cases.You might want to add a
fatalErrorat 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
Environmentenum 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:
- Consider making the
modeconfigurable, perhaps through an environment variable or a build configuration. This would allow easier switching between mock and live modes without code changes.- 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@StateformockChatClientas well, similar toliveChatClient. This would provide consistent state management for both client types:@State private var mockChatClient: MockChatClientThen initialize it in the
init()method alongsideliveChatClient.
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()andshowReactions()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 issueLGTM! Consider mode-based initialization and API key management.
The initialization of both
liveRealtimeandliveChatClientis correctly implemented. However, there are a few points to consider:
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).
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.
|
@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 |
|
@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
force-pushed
the
ECO-4945
branch
5 times, most recently
from
3b3bc5b
to
11323e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
Sources/AblyChat/DefaultRoomReactions.swift (2)
25-73: Consider extracting message validation logicThe 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 caseThe
noReferenceToSelfcase inRoomReactionsErroris 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 scenariosWhile 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 strategyGiven 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:
- Basic integration verification (current test)
- Comprehensive reactions testing (new test)
- 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:
- Add XML documentation comments to describe the method's purpose and thread-safety guarantees
- 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:
- Reaction channel attachment failures
- Reaction channel state transitions
- 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 configurableThe 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 bufferingUsing
.unboundedbuffering policy could lead to memory issues with high message volume. Consider:
- Using a bounded buffer size
- 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 subscriptionThe 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
📒 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.
| @@ -15,7 +15,12 @@ internal enum RoomFeature { | |||
| case .messages: | |||
| // (CHA-M1) Chat messages for a Room are sent on a corresponding realtime channel <roomId>::$chat::$chatMessages. For example, if your room id is my-room then the messages channel will be my-room::$chat::$chatMessages. | |||
| "chatMessages" | |||
| case .typing, .reactions, .presence, .occupancy: | |||
| case .typing: | |||
| "typingIndicators" | |||
There was a problem hiding this comment.
.typing doesn't seem relevant to this PR; let's keep it as it was before i.e. add these as we implement the features, so we can write the spec point in?
There was a problem hiding this comment.
Have you? It looks the same to me
| // (10) Subscribe to reactions | ||
| let rxReactionSubscription = await rxRoom.reactions.subscribe(bufferingPolicy: .unbounded) | ||
|
|
||
| // (11) Now that we’re subscribed to reactions, send a message on the other client and check that we receive it on the subscription |
There was a problem hiding this comment.
| // (11) Now that we’re subscribed to reactions, send a message on the other client and check that we receive it on the subscription | |
| // (11) Now that we’re subscribed to reactions, send a reaction on the other client and check that we receive it on the subscription |
There was a problem hiding this comment.
It looks the same to me?
|
I’ve just merged #108, which implements |
|
@coderabbitai pause |
done |
`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
✅ Actions performedReviews paused. |
| let messagesDiscontinuitySubscription = await roomReactions.subscribeToDiscontinuities() | ||
| await featureChannel.emitDiscontinuity(featureChannelDiscontinuity) | ||
|
|
||
| // Then: The DefaultMessages instance emits this discontinuity through `subscribeToDiscontinuities` |
There was a problem hiding this comment.
| // Then: The DefaultMessages instance emits this discontinuity through `subscribeToDiscontinuities` | |
| // Then: The DefaultReactions instance emits this discontinuity through `subscribeToDiscontinuities` |
| @@ -15,7 +15,12 @@ internal enum RoomFeature { | |||
| case .messages: | |||
| // (CHA-M1) Chat messages for a Room are sent on a corresponding realtime channel <roomId>::$chat::$chatMessages. For example, if your room id is my-room then the messages channel will be my-room::$chat::$chatMessages. | |||
| "chatMessages" | |||
| case .typing, .reactions, .presence, .occupancy: | |||
| case .typing: | |||
| "typingIndicators" | |||
There was a problem hiding this comment.
Have you? It looks the same to me
| // (10) Subscribe to reactions | ||
| let rxReactionSubscription = await rxRoom.reactions.subscribe(bufferingPolicy: .unbounded) | ||
|
|
||
| // (11) Now that we’re subscribed to reactions, send a message on the other client and check that we receive it on the subscription |
There was a problem hiding this comment.
It looks the same to me?
Spec complete for Ephemeral Room Reactions in line with [1].
CHA-ER3b&CHA-ER3chave 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
Reactionstruct for representing reactions in chat.Bug Fixes
Tests
DefaultRoomReactionsclass to validate functionalities.Documentation