[ECO-5114] Provide mechanism for waiting to be able to perform presence operations #115
Conversation
WalkthroughThe changes in this pull request enhance the error handling and state management capabilities of the Ably Chat SDK. New error cases are introduced in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@lawrence-forooghian has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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: 5
🧹 Outside diff range and nitpick comments (9)
Tests/AblyChatTests/Helpers/Helpers.swift (1)
16-22: Consider simplifying the message validation logic.While the current implementation is functionally correct, it could be more readable using a direct conditional expression.
Consider this simpler alternative:
- && { - guard let message else { - return true - } - - return ablyError.message == message - }() + && (message == nil || ablyError.message == message)This achieves the same functionality with less code and improved readability.
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (2)
8-8: Fix naming inconsistency in initializer parameter.The property name doesn't match the parameter name in the initializer:
- Property:
resultOfWaitToBeAbleToPerformPresenceOperations- Parameter:
resultOfWaitToBeAblePerformPresenceOperations(missing "To")
30-36: Add documentation for test behavior.Consider adding documentation to explain how this mock method behaves in test scenarios and how to configure it through the initializer.
+ /// Simulates waiting for presence operations to be possible. + /// Configure the behavior by setting `resultOfWaitToBeAblePerformPresenceOperations` in the initializer: + /// - When nil (default): causes a fatal error when called + /// - When success: completes normally + /// - When failure: throws the provided errorSources/AblyChat/RoomFeature.swift (1)
57-57: Consider restricting the setter access level.To prevent external modification of the
roomLifecycleManager, consider marking it asprivate(set):- internal var roomLifecycleManager: RoomLifecycleManager + internal private(set) var roomLifecycleManager: RoomLifecycleManagerSources/AblyChat/Room.swift (2)
Line range hint
118-135: Add documentation for the new helper methods.Consider adding documentation comments to explain:
- The purpose of each method
- The relationship between partial dependencies and feature channels
- The expected input and output
Example documentation:
/// Creates partial dependencies for feature channels including the underlying channel and lifecycle contributor. /// - Parameters: /// - roomID: The unique identifier for the room /// - realtime: The realtime client instance /// - Returns: A dictionary mapping features to their partial dependencies private static func createFeatureChannelPartialDependencies...
Line range hint
118-135: Consider adding presence feature support.The PR objectives mention implementing a mechanism for presence operations, but the channel creation methods currently only handle messages and reactions features.
Consider:
- Adding presence to the list of features in
createFeatureChannelPartialDependencies- Implementing the presence-related methods that are currently marked with
fatalError- Adding the waiting mechanism for presence operations as described in the PR objectives
Would you like me to provide a detailed implementation suggestion for these changes?
Sources/AblyChat/Errors.swift (2)
14-15: Consider a more descriptive name for the error codeThe name
nonspecificdoesn't clearly convey its purpose for presence operations. Consider a more specific name likepresenceOperationErrorto better reflect its usage.- case nonspecific = 40000 + case presenceOperationError = 40000
115-117: Consider distinct error codes for different presence operation errorsCurrently, both presence operation errors map to the same
.nonspecificerror code. This makes it harder to distinguish between these errors at the API level. Consider creating separate error codes for each case.case .presenceOperationRequiresRoomAttach, .presenceOperationDisallowedForCurrentRoomStatus: - .nonspecific + switch self { + case .presenceOperationRequiresRoomAttach: + .presenceOperationRequiresAttach + case .presenceOperationDisallowedForCurrentRoomStatus: + .presenceOperationInvalidStatus + }This would require adding the corresponding cases to
ErrorCode:public enum ErrorCode: Int { case presenceOperationRequiresAttach = 40001 case presenceOperationInvalidStatus = 40002 // ... }Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (1)
1367-1517: Well-structured test implementation for presence operation waiting.The test suite thoroughly covers the waiting mechanism for presence operations across different room states. The implementation follows best practices with:
- Clear test organization using Given/When/Then
- Comprehensive state coverage
- Proper error handling and verification
- Good use of mock objects
However, there are a few opportunities for improvement:
- Consider extracting common setup code for the attaching state tests into a helper method to reduce duplication between
waitToBeAbleToPerformPresenceOperations_whenAttaching_whenAttachSucceedsandwaitToBeAbleToPerformPresenceOperations_whenAttaching_whenAttachFails.- Consider adding timeout handling to prevent tests from hanging if the wait operation never completes.
Example helper method:
private func setupManagerInAttachingState() async throws -> ( DefaultRoomLifecycleManager<MockRoomLifecycleContributor>, SignallableChannelOperation, UUID ) { let contributorAttachOperation = SignallableChannelOperation() let contributor = createContributor(attachBehavior: contributorAttachOperation.behavior) let manager = await createManager(contributors: [contributor]) let roomStatusSubscription = await manager.onChange(bufferingPolicy: .unbounded) let attachOperationID = UUID() async let _ = manager.performAttachOperation(testsOnly_forcingOperationID: attachOperationID) _ = await roomStatusSubscription.attachingElements().first { _ in true } return (manager, contributorAttachOperation, attachOperationID) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
Sources/AblyChat/Errors.swift(7 hunks)Sources/AblyChat/Room.swift(3 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Sources/AblyChat/RoomLifecycleManager.swift(12 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(11 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(2 hunks)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(1 hunks)
🧰 Additional context used
📓 Learnings (2)
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (2)
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631
Timestamp: 2024-11-12T15:07:39.465Z
Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631
Timestamp: 2024-11-12T15:07:31.866Z
Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (4)
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#67
File: Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift:98-102
Timestamp: 2024-11-12T15:07:31.865Z
Learning: The cleanup of old subscriptions in `MockRoomLifecycleContributorChannel` is acknowledged with a TODO comment and is tracked in issue #36.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#67
File: Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift:98-102
Timestamp: 2024-11-12T15:07:31.865Z
Learning: The cleanup of old subscriptions in `MockRoomLifecycleContributorChannel` is acknowledged with a TODO comment and is tracked in issue #36.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631
Timestamp: 2024-11-12T15:07:39.465Z
Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631
Timestamp: 2024-11-12T15:07:31.866Z
Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.
🔇 Additional comments (19)
Tests/AblyChatTests/Helpers/Helpers.swift (2)
5-5: LGTM! Clear and concise documentation.
The documentation clearly explains the new message parameter functionality.
7-7: LGTM! Well-designed signature change.
The optional message parameter with a default value of nil maintains backward compatibility while adding new functionality.
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (1)
Line range hint 8-36: Implementation aligns well with PR objectives.
The mock implementation provides the flexibility needed to test all the room status scenarios described in the PR objectives through the configurable Result parameter.
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (2)
42-42: LGTM! Good enhancement to behavior modeling.
The change from returning AttachOrDetachResult to AttachOrDetachBehavior enables more sophisticated testing scenarios by allowing behaviors to be chained together. This is particularly valuable for testing complex state transitions in room lifecycle management.
79-81: LGTM! Clean implementation of behavior chaining.
The recursive processing of behaviors is well implemented. This pattern allows test cases to simulate complex sequences of state changes, which is essential for testing the new presence operation waiting mechanism.
Sources/AblyChat/RoomFeature.swift (3)
32-36: LGTM! Clear and well-documented protocol purpose.
The documentation effectively explains the protocol's responsibilities and available functionality.
40-51: LGTM! Comprehensive implementation of presence operation requirements.
The method signature and documentation thoroughly cover all required scenarios from the PR objectives, including proper error handling for each room status.
63-65: LGTM! Clean implementation with proper delegation.
The implementation correctly delegates to the roomLifecycleManager, maintaining good separation of concerns.
Sources/AblyChat/Room.swift (3)
85-87: LGTM! Good separation of concerns.
The refactoring improves code organization by separating channel creation from contributor initialization, making the code more maintainable.
113-116: LGTM! Clean dependency grouping.
The FeatureChannelPartialDependencies struct provides a well-encapsulated way to group related channel dependencies.
94-95: Verify the safety of force unwrapping in feature initialization.
While there's a TODO comment referencing issue #105, the current implementation force unwraps channel values which could lead to runtime crashes if the channels dictionary is missing expected features.
Let's verify that the required features are always present in the channels dictionary:
✅ Verification successful
Let me gather more information about the feature channels creation to verify the safety.
Let me check one more aspect to understand how the feature channels are used and accessed.
Based on the gathered information, I can now provide a final response:
Force unwrapping of feature channels is safe in this implementation
The force unwrapping is actually safe because:
- Feature channels are created using a dictionary initialized with
uniqueKeysWithValuesfrom a fixed set of features (messages and reactions) - The
RoomFeatureenum has these as explicit cases - The dictionary creation maps over these specific features, ensuring they are always present
- The channel creation happens in a single atomic operation with no conditional logic that could skip features
The TODO comment about force unwrapping (issue #105) might be addressing a different concern or suggesting a more elegant solution, but the current implementation is not at risk of runtime crashes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any cases where messages or reactions features might be missing
# Look for conditional creation of these features or early returns in channel creation
# Search for any conditional logic around feature creation
rg -A 5 "RoomFeature\.(messages|reactions)" --type swift
# Search for any early returns or guards in channel creation methods
ast-grep --pattern 'func createFeatureChannel$$${
$$$
return $$$
}'
Length of output: 700
Script:
#!/bin/bash
# Check the implementation of createFeatureChannels and related code
ast-grep --pattern 'static func createFeatureChannels($$$) {
$$$
}'
# Check RoomFeature enum definition and usage
rg -A 10 "enum RoomFeature" --type swift
# Check if there are any guards or conditions in the Room initialization
ast-grep --pattern 'init($$$) {
$$$
featureChannels $$$
$$$
}'
Length of output: 815
Script:
#!/bin/bash
# Check how feature channels are accessed and used
rg -A 5 "featureChannels\[" --type swift
# Check the full Room.swift implementation for context
rg -A 10 "class Room" --type swift
Length of output: 742
Sources/AblyChat/Errors.swift (5)
41-42: LGTM!
The grouping of the new error code with other 400-level client errors is appropriate and follows HTTP standards.
75-76: LGTM!
The new error cases are well-named and align perfectly with the PR objectives for handling presence operations in different room states. The feature parameter provides good context for error messages.
Line range hint 121-134: LGTM!
The helper methods are well-structured and effectively reduce code duplication. The implementation follows Swift best practices for string interpolation and switch expressions.
Also applies to: 136-153
171-174: LGTM!
The error messages are clear, user-friendly, and provide specific guidance about what went wrong and what needs to be done.
188-190: LGTM!
Appropriate handling of the cause property for the new error cases, correctly returning nil as these are self-contained errors.
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (2)
8-26: LGTM! Improved naming clarity in SignallableChannelOperation.
The renaming from complete(result:) to complete(behavior:) and corresponding documentation updates better reflect the purpose of the mock operation.
1480-1481: Investigate compiler issue with #expect(..., throws:).
The comments indicate a compiler crash when using #expect(..., throws:). This should be investigated and reported as it forces the use of more verbose error handling code.
Run the following script to check if this is a known issue:
Also applies to: 1505-1506
Sources/AblyChat/RoomLifecycleManager.swift (1)
49-49:
Invalid Syntax: Do Not Specify Error Type in throws Clause
In Swift, you cannot specify an error type in the throws clause of a function. The syntax throws(ARTErrorInfo) is invalid. Instead, you should use throws without specifying the error type. This issue appears in several function declarations and needs to be corrected.
For example, update the function declarations as follows:
// At line 49
-func waitToBeAbleToPerformPresenceOperations(requestedByFeature requester: RoomFeature) async throws(ARTErrorInfo)
+func waitToBeAbleToPerformPresenceOperations(requestedByFeature requester: RoomFeature) async throws
// At lines 608-612
-private func waitForCompletionOfOperationWithID(
_ waitedOperationID: UUID,
requester: OperationWaitRequester
) async throws(ARTErrorInfo) {
+private func waitForCompletionOfOperationWithID(
_ waitedOperationID: UUID,
requester: OperationWaitRequester
) async throws {
// At lines 656-659
-private func performAnOperation(
forcingOperationID forcedOperationID: UUID?,
_ body: (UUID) async throws(ARTErrorInfo) -> Void
) async throws(ARTErrorInfo) {
+private func performAnOperation(
forcingOperationID forcedOperationID: UUID?,
_ body: (UUID) async throws -> Void
) async throws {
// At line 696
-private func bodyOfAttachOperation(operationID: UUID) async throws(ARTErrorInfo) {
+private func bodyOfAttachOperation(operationID: UUID) async throws
// At line 816
-private func bodyOfDetachOperation(operationID: UUID) async throws(ARTErrorInfo) {
+private func bodyOfDetachOperation(operationID: UUID) async throws
// At line 972
-internal func waitToBeAbleToPerformPresenceOperations(requestedByFeature requester: RoomFeature) async throws(ARTErrorInfo) {
+internal func waitToBeAbleToPerformPresenceOperations(requestedByFeature requester: RoomFeature) async throwsAlso applies to: 608-612, 656-659, 696-696, 816-816, 972-972
⛔ Skipped due to learnings
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#106
File: Sources/AblyChat/DefaultRoomLifecycleContributor.swift:26-28
Timestamp: 2024-11-12T15:07:31.865Z
Learning: In Swift 6, typed throws are allowed, so it's acceptable to specify the error type in the `throws` declaration, such as `throws(ARTErrorInfo)`.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Sources/AblyChat/RoomLifecycleManager.swift:265-266
Timestamp: 2024-11-12T15:07:31.866Z
Learning: In `RoomLifecycleManager.swift`, the use of `try!` with `await` for `clock.sleep` is intentional due to reasons outlined in the associated GitHub issue. Do not suggest changing this usage unless the issue is resolved.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Sources/AblyChat/RoomLifecycleManager.swift:265-266
Timestamp: 2024-11-12T15:07:31.866Z
Learning: In `RoomLifecycleManager.swift`, the use of `try!` with `await` for `clock.sleep` is intentional due to reasons outlined in the associated GitHub issue. Do not suggest changing this usage unless the issue is resolved.
lawrence-forooghian
force-pushed
the
112-wait-to-be-able-to-do-presence
branch
from
5313690 to
698f8b5
Compare
lawrence-forooghian
changed the base branch from
main
to
fix-intermittent-test-failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
Sources/AblyChat/RoomFeature.swift (1)
32-51: LGTM! Consider adding @throws documentation.The documentation is comprehensive and well-structured, clearly explaining the protocol's purpose and the new method's behavior. The references to spec points (CHA-PR3d, etc.) provide excellent traceability.
Consider adding
@throwsdocumentation to explicitly document the error conditions:/// - Parameters: /// - requester: The room feature that wishes to perform a presence operation. This is only used for customising the message of the thrown error. + /// - Throws: An `ARTErrorInfo` derived from either: + /// - `ChatError.presenceOperationRequiresRoomAttach` if room is DETACHED + /// - `ChatError.presenceOperationDisallowedForCurrentRoomStatus` for other invalid statesSources/AblyChat/RoomLifecycleManager.swift (1)
910-914: Consider removing force-try in release operationWhile the comment explains the current need for force-try, it would be better to handle this properly in the future.
Consider adding a TODO comment to track this technical debt:
// See note on performAnOperation for the current need for this force try +// TODO: Remove force-try once performAnOperation supports non-throwing operations // swiftlint:disable:next force_try try! await performAnOperation(forcingOperationID: forcedOperationID) { operationID inTests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (4)
Line range hint
448-456: Consistent implementation of detachImpl closuresThe detachImpl closures across different test cases follow a consistent pattern for handling attach/detach behavior simulation. However, there's an opportunity to reduce code duplication.
Consider extracting the common detachImpl pattern into a helper function:
private func createDetachImpl(successAtCallCount: Int = 3) -> @Sendable (Int) async -> MockRoomLifecycleContributorChannel.AttachOrDetachBehavior { return { callCount in if callCount < successAtCallCount { return .failure(ARTErrorInfo(domain: "SomeDomain", code: 123)) } return .success } }Also applies to: 651-657, 831-838
1369-1519: Comprehensive test coverage for presence operationsThe new test cases thoroughly cover the waitToBeAbleToPerformPresenceOperations functionality across different room states:
- Attaching state with successful/failed attach
- Attached state
- Detached state
- Other states
The tests are well-structured and include:
- Clear setup and teardown
- Proper error handling
- Comprehensive assertions
- Detailed spec references
However, there are some TODOs that need to be addressed:
Would you like me to help create GitHub issues for the remaining TODOs mentioned in the spec comments? For example:
- Implementing the feature-specific waits (CHA-PR3d, CHA-PR10d, etc.)
- Converting specPartial to specOneOf once features are implemented
1482-1483: Document the compiler crash workaroundThe comments about compiler crashes with #expect(…, throws:) should be expanded to help future maintainers.
Consider adding more context to the comment:
-// (Note: I wanted to use #expect(…, throws:) below, but for some reason it made the compiler _crash_! No idea why. So, gave up on that.) +// TODO: Investigate compiler crash with #expect(…, throws:) +// Current workaround: Using do-catch instead of #expect(…, throws:) due to a compiler crash. +// Crash occurs when using throws assertion with async/await code. +// Reference: [Add link to issue tracker if available]Also applies to: 1507-1508
987-989: Improve readability of async test setupThe async setup code for handling detach operations could be more readable.
Consider extracting the setup into a helper method:
private func waitForDetachingState(_ manager: DefaultRoomLifecycleManager<MockRoomLifecycleContributor>) async -> Task<Void, Error> { let roomStatusSubscription = await manager.onChange(bufferingPolicy: .unbounded) let task = Task { try await manager.performDetachOperation() } _ = await roomStatusSubscription.first { $0.current == .detaching } return task }Also applies to: 1012-1012
Sources/AblyChat/Room.swift (1)
Line range hint
118-125: Improve flexibility by dynamically generatingRoomFeaturesbased onRoomOptionsThe
createFeatureChannelPartialDependenciesmethod currently uses a hardcoded list[RoomFeature.messages, RoomFeature.reactions]. This approach limits scalability when adding new features like presence or typing indicators. Consider modifying this method to generate the list ofRoomFeatures dynamically based onoptionsto enhance flexibility and maintainability.Apply this diff to modify the method signature and implementation:
- private static func createFeatureChannelPartialDependencies(roomID: String, realtime: RealtimeClient) -> [RoomFeature: FeatureChannelPartialDependencies] { - .init(uniqueKeysWithValues: [RoomFeature.messages, RoomFeature.reactions].map { feature in + private static func createFeatureChannelPartialDependencies(roomID: String, realtime: RealtimeClient, enabledFeatures: [RoomFeature]) -> [RoomFeature: FeatureChannelPartialDependencies] { + .init(uniqueKeysWithValues: enabledFeatures.map { feature in let channel = realtime.getChannel(feature.channelNameForRoomID(roomID)) let contributor = DefaultRoomLifecycleContributor(channel: .init(underlyingChannel: channel), feature: feature) return (feature, .init(channel: channel, contributor: contributor)) }) }And when calling the method, determine the enabled features:
+ let enabledFeatures = [RoomFeature.messages] + (options.reactions != nil ? [RoomFeature.reactions] : []) - let featureChannelPartialDependencies = Self.createFeatureChannelPartialDependencies(roomID: roomID, realtime: realtime) + let featureChannelPartialDependencies = Self.createFeatureChannelPartialDependencies(roomID: roomID, realtime: realtime, enabledFeatures: enabledFeatures)This change allows the method to adapt based on the enabled features in
RoomOptions, making it easier to add new features in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
Sources/AblyChat/Errors.swift(7 hunks)Sources/AblyChat/Room.swift(3 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Sources/AblyChat/RoomLifecycleManager.swift(12 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(12 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(2 hunks)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Sources/AblyChat/Errors.swift
- Tests/AblyChatTests/Helpers/Helpers.swift
- Tests/AblyChatTests/Mocks/MockFeatureChannel.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
🧰 Additional context used
📓 Learnings (1)
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (4)
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631
Timestamp: 2024-11-12T15:07:39.465Z
Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631
Timestamp: 2024-11-12T15:07:31.866Z
Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:335-335
Timestamp: 2024-11-12T15:07:31.865Z
Learning: In Swift Testing, test method names do not need to start with `test`.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:335-335
Timestamp: 2024-11-12T15:07:31.865Z
Learning: In Swift Testing, test method names do not need to start with `test`.
🔇 Additional comments (8)
Sources/AblyChat/RoomFeature.swift (2)
57-65: LGTM! Clean and focused implementation.
The implementation follows good practices:
- Simple delegation to the roomLifecycleManager
- Proper error propagation
- Consistent with the protocol's visibility requirements
63-65: Verify complete implementation of room status handling.
While the delegation looks correct, we should verify that the roomLifecycleManager implements all required room status handling as specified in the PR objectives.
✅ Verification successful
Room status handling is properly implemented in RoomLifecycleManager
The verification shows comprehensive status handling in RoomLifecycleManager.swift:
- Handles
.attachedstate with proper operation checks - Handles
.attachingstate with transient disconnect timeout logic - Handles
.detachedstate with appropriate error throwing - Handles additional states like
.releasing,.released,.suspended,.failed - Includes specific error handling for presence operations based on room status (CHA-PR3e, CHA-PR11e, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify roomLifecycleManager implements all required status handling
# Search for the implementation of waitToBeAbleToPerformPresenceOperations
ast-grep --pattern 'func waitToBeAbleToPerformPresenceOperations(requestedByFeature $$$) $$$'
# Search for status handling in the implementation
rg -A 10 'case .attached:|case .attaching:|case .detached:'
Length of output: 8939
Sources/AblyChat/RoomLifecycleManager.swift (5)
543-544: LGTM: Improved error handling with Result type
The change to use Result<Void, ARTErrorInfo> with a non-throwing continuation is a good improvement. It provides more precise error handling and better type safety.
560-561: LGTM: Made waitingOperationID optional to support presence operations
The change to make waitingOperationID optional aligns well with the architectural changes for presence operations support.
577-598: LGTM: Well-structured enum for operation wait requests
The new OperationWaitRequester enum provides a clean abstraction for different types of wait requests, with good helper properties for logging and ID extraction.
972-995: LGTM: Comprehensive presence operation state handling
The implementation properly handles different room states for presence operations, following the specifications in the PR objectives.
978-979:
Critical: Replace fatalError with proper error handling
The fatalError for the attachingDueToContributorStateChange case should be replaced with proper error handling to avoid crashes in production.
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (1)
8-26: Improved error handling in SignallableChannelOperation class
The changes to the SignallableChannelOperation class enhance its behavior by:
- Updating documentation to clarify behavior expectations
- Changing from result-based to behavior-based completion
- Improving type safety with MockRoomLifecycleContributorChannel.AttachOrDetachBehavior
Will use in an upcoming commit.
This is what its documentation says it does, but I mistakenly didn’t do it in e3dcfe1. (It didn’t actually matter because nobody currently uses the error that this method throws; they will do in an upcoming commit, though.)
Will use in an upcoming commit (want to add a manager method with a typed throw).
This is needed for Umair to be able to implement the presence and typing indicators spec points that I’ve mentioned in the code. Based on spec at f7ca465. Resolves #112.
lawrence-forooghian
force-pushed
the
112-wait-to-be-able-to-do-presence
branch
from
698f8b5 to
9c97883
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
Sources/AblyChat/RoomFeature.swift (1)
34-38: Consider using more formal terminology in documentation.While the documentation clearly explains the protocol's responsibilities, consider replacing the informal term "mishmash" with more professional terminology like "composition" or "combination".
-/// This mishmash exists to give a room feature access to: +/// This composition exists to give a room feature access to:Sources/AblyChat/Errors.swift (3)
14-15: Consider a more descriptive name for the error code.While
nonspecificaligns with the spec's usage of 40000, consider a more descriptive name likepresenceOperationErrorsince it's specifically used for presence operation errors in this context.
115-117: Consider documenting the error code mapping.Both presence operation error cases map to the same
.nonspecificerror code (40000). Consider adding a comment explaining why these distinct error cases share the same error code, referencing the spec if possible.
Line range hint
121-134: Consider makingdescriptionOfFeaturepublic.This helper function could be useful for other parts of the SDK that need to convert features to strings. Consider making it public if it might be needed elsewhere.
Sources/AblyChat/Room.swift (1)
Line range hint
134-165: Consider extracting channel options configurationThe method handles channel configuration well, but the logic for configuring channel options could be extracted into separate methods for better organization and readability.
Consider this refactoring:
private static func createFeatureChannelPartialDependencies(roomID: String, roomOptions: RoomOptions, realtime: RealtimeClient) -> [RoomFeature: FeatureChannelPartialDependencies] { .init(uniqueKeysWithValues: [ RoomFeature.messages, RoomFeature.reactions, RoomFeature.presence, RoomFeature.occupancy, ].map { feature in - let channelOptions = ARTRealtimeChannelOptions() - - // channel setup for presence and occupancy - if feature == .presence { - let channelOptions = ARTRealtimeChannelOptions() - let presenceOptions = roomOptions.presence - - if presenceOptions?.enter ?? false { - channelOptions.modes.insert(.presence) - } - - if presenceOptions?.subscribe ?? false { - channelOptions.modes.insert(.presenceSubscribe) - } - } else if feature == .occupancy { - channelOptions.params = ["occupancy": "metrics"] - } + let channelOptions = configureChannelOptions(for: feature, with: roomOptions) let channel = realtime.getChannel(feature.channelNameForRoomID(roomID), opts: channelOptions) let contributor = DefaultRoomLifecycleContributor(channel: .init(underlyingChannel: channel), feature: feature) return (feature, .init(channel: channel, contributor: contributor)) }) } + +private static func configureChannelOptions(for feature: RoomFeature, with roomOptions: RoomOptions) -> ARTRealtimeChannelOptions { + let channelOptions = ARTRealtimeChannelOptions() + + switch feature { + case .presence: + if let presenceOptions = roomOptions.presence { + if presenceOptions.enter { + channelOptions.modes.insert(.presence) + } + if presenceOptions.subscribe { + channelOptions.modes.insert(.presenceSubscribe) + } + } + case .occupancy: + channelOptions.params = ["occupancy": "metrics"] + default: + break + } + + return channelOptions +}Sources/AblyChat/RoomLifecycleManager.swift (1)
651-659: Consider addressing the force-try requirement.While the changes improve type safety with
ARTErrorInfo, there's still a need to use force-try for non-throwing operations. Consider refactoring to avoid force-try, perhaps by using a generic return type that can represent both throwing and non-throwing operations.Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (2)
1371-1374: Consider updating spec references.The
@specPartialannotations indicate incomplete implementation. Consider adding TODO tracking issues for the remaining implementation parts to ensure full spec coverage.
1445-1452: Consider extracting error handling into a helper method.The error handling pattern is repeated across multiple test methods. Consider extracting it into a helper method to reduce code duplication and improve maintainability.
+ private func assertThrows<T>( + _ operation: () async throws -> T, + expectedCode: Int, + expectedMessage: String? = nil + ) async { + var caughtError: Error? + do { + _ = try await operation() + } catch { + caughtError = error + } + if let message = expectedMessage { + #expect(isChatError(caughtError, withCode: expectedCode, message: message)) + } else { + #expect(isChatError(caughtError, withCode: expectedCode)) + } + }Also applies to: 1485-1493, 1510-1518
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
Sources/AblyChat/Errors.swift(7 hunks)Sources/AblyChat/Room.swift(3 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Sources/AblyChat/RoomLifecycleManager.swift(12 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(11 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(2 hunks)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Tests/AblyChatTests/Helpers/Helpers.swift
- Tests/AblyChatTests/Mocks/MockFeatureChannel.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
🧰 Additional context used
📓 Learnings (1)
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (2)
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#67
File: Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift:98-102
Timestamp: 2024-11-12T15:07:31.865Z
Learning: The cleanup of old subscriptions in `MockRoomLifecycleContributorChannel` is acknowledged with a TODO comment and is tracked in issue #36.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631
Timestamp: 2024-11-12T15:07:39.465Z
Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.
🔇 Additional comments (24)
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (2)
42-42: LGTM! Enhanced behavior chaining capability.
The change from AttachOrDetachResult to AttachOrDetachBehavior as the return type enables more sophisticated mocking scenarios where one behavior can transition into another. This is particularly useful for testing complex state transitions in room lifecycle management.
79-81: LGTM! Verify recursive behavior handling.
The recursive handling of behaviors is well-implemented, allowing for chained state transitions while maintaining actor isolation. This change aligns well with the PR objectives for testing room status transitions.
Let's verify the recursive behavior handling implementation:
✅ Verification successful
Behavior handling implementation is safe and well-structured
The implementation shows a well-designed recursive behavior system with proper safeguards:
- The
AttachOrDetachBehaviorenum has clear termination cases (completeandcompleteAndChangeState) - The
fromFunctioncase allows for dynamic behavior chaining but must eventually resolve to a termination case - The recursive call in
performBehaviormaintains the samecallCount, preventing counter-based infinite loops - The implementation is contained within test mocks, limiting any potential issues to the test environment
The behavior system is primarily used for testing state transitions and has appropriate error handling through throws. No circular dependencies or unsafe recursive patterns were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential infinite recursion scenarios in tests
# Look for test cases that might create circular behavior chains
rg -A 10 "fromFunction.*fromFunction" Tests/
# Look for deep behavior chains that might cause stack overflow
rg -A 10 "fromFunction.*complete" Tests/
Length of output: 89
Script:
#!/bin/bash
# Let's analyze the behavior handling implementation more thoroughly
# First, check how behaviors are defined and used
rg -A 5 "enum.*Behavior" Tests/
# Look for the performBehavior implementation
ast-grep --pattern 'func performBehavior($_, $_) {
$$$
}'
# Check for recursive calls or chaining patterns
rg -A 5 "performBehavior" Tests/
# Look for test cases that exercise behavior transitions
rg -A 10 "test.*Behavior" Tests/
Length of output: 4785
Sources/AblyChat/RoomFeature.swift (2)
41-53: LGTM! Well-documented protocol addition.
The new protocol method is thoroughly documented with clear specifications for different room statuses, aligning perfectly with the PR objectives and linked issue requirements.
59-67: Verify roomLifecycleManager initialization.
The implementation looks good and follows proper separation of concerns. However, let's ensure that roomLifecycleManager is properly initialized in all DefaultFeatureChannel instances.
✅ Verification successful
Let's try a different approach to find the initialization patterns in Swift code. The previous pattern might not have matched due to Swift's syntax specifics. These commands should help us locate the class definition and initialization patterns.
Let's first identify the relevant Swift files and then examine how DefaultFeatureChannel and roomLifecycleManager are used throughout the codebase. This should help us locate the initialization patterns even if they don't match our previous AST patterns.
roomLifecycleManager is properly initialized
The verification shows that roomLifecycleManager is correctly initialized:
- In
Room.swift, thecreateFeatureChannelsmethod initializesDefaultFeatureChannelinstances with all required dependencies, includingroomLifecycleManager - The initialization is done through a proper constructor that sets all required properties
- The
lifecycleManagerparameter is passed from the parent scope ensuring proper dependency injection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for DefaultFeatureChannel initializations
ast-grep --pattern 'DefaultFeatureChannel(channel: $_, contributor: $_, roomLifecycleManager: $_)'
# Search for potential missing initializations
rg -l "DefaultFeatureChannel" | xargs rg "init.*DefaultFeatureChannel.*\)"
Length of output: 175
Script:
#!/bin/bash
# Search for DefaultFeatureChannel class definition and initialization
rg -A 10 "class DefaultFeatureChannel"
# Search for roomLifecycleManager initialization in init methods
ast-grep --pattern 'init($$$) {
$$$
self.roomLifecycleManager = $_
$$$
}'
# Search for DefaultFeatureChannel initialization with different pattern
ast-grep --pattern 'let $_ = DefaultFeatureChannel($$$)'
Length of output: 181
Script:
#!/bin/bash
# Search for files containing DefaultFeatureChannel
fd -e swift | xargs rg -l "DefaultFeatureChannel"
# Once we find the files, let's look at their content
fd -e swift | xargs rg "DefaultFeatureChannel" -B 2 -A 5
# Search for roomLifecycleManager in Swift files
fd -e swift | xargs rg "roomLifecycleManager"
Length of output: 1821
Sources/AblyChat/Errors.swift (5)
41-42: LGTM!
The status code mapping of 400 for the new error code is appropriate as these are client-side validation errors.
75-76: LGTM!
The new error cases accurately represent the requirements for handling presence operations based on room status, as specified in the PR objectives.
136-153: LGTM!
Well-structured helper types and functions that improve code organization and reuse. Good use of Swift's new switch expression syntax.
171-174: LGTM!
The error messages are clear, informative, and provide good context about why the presence operation failed.
188-190: LGTM!
Consistent handling of causes where presence operation errors, like other validation errors, don't have underlying causes.
Sources/AblyChat/Room.swift (4)
87-89: LGTM! Good separation of concerns
The refactoring improves code organization by separating channel creation from feature initialization. This makes the code more maintainable and testable.
96-97: LGTM! Good integration of lifecycle management
The feature channels creation now properly integrates with the lifecycle manager, which aligns well with the PR's objective of managing presence operations based on room status.
129-132: LGTM! Well-structured data encapsulation
The struct provides a clean way to group related channel initialization data while maintaining proper access control.
166-174: LGTM! Clean and functional implementation
The method effectively transforms partial dependencies into feature channels while properly injecting the lifecycle manager. The functional approach using mapValues makes the code concise and clear.
Sources/AblyChat/RoomLifecycleManager.swift (7)
543-543: LGTM: Improved error handling with specific error type.
The change to use Result<Void, ARTErrorInfo> instead of throwing a generic Error provides better type safety and clearer error handling.
560-560: LGTM: Made waitingOperationID optional to support presence operations.
The change allows the struct to represent wait events from both regular operations (with an ID) and presence operations (without an ID).
577-598: LGTM: Well-structured enum for operation wait requesters.
The enum provides a clean separation between different types of requesters with helpful utility methods for logging and ID extraction. The implementation aligns well with the PR objectives for supporting presence operations.
608-633: LGTM: Enhanced operation handling with improved logging and error handling.
The changes properly integrate the new OperationWaitRequester while maintaining clean error propagation and detailed logging.
638-644: LGTM: Consistent error handling in operation completion.
The changes align well with the new Result type system and provide consistent error handling throughout the operation lifecycle.
691-693: LGTM: Consistent error handling in attach/detach operations.
The changes maintain consistency with the new error handling system.
Also applies to: 811-813
972-995:
Critical: Replace fatalError with proper error handling.
The waitToBeAbleToPerformPresenceOperations implementation has a critical issue where it uses fatalError for the attachingDueToContributorStateChange case. This could cause app crashes in production.
Consider throwing a specific error instead:
- fatalError("waitToBeAbleToPerformPresenceOperations doesn't currently handle attachingDueToContributorStateChange")
+ throw ARTErrorInfo(chatError: .unhandledAttachingDueToContributorStateChange(feature: requester))Also, ensure to update the error type definition to include this new case.
Likely invalid or redundant comment.
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (4)
8-26: LGTM! Good improvement to the mock implementation.
The changes to SignallableChannelOperation class improve clarity by shifting from a result-based to a behavior-based completion model, which better aligns with the testing requirements and makes the test intentions clearer.
1369-1370: LGTM! Clear section organization.
The new section for presence operations tests is well-organized and clearly marked.
1375-1407: LGTM! Comprehensive test coverage for presence operations.
The test implementation thoroughly covers all required scenarios for presence operations:
- Waiting while attaching (success and failure cases)
- Behavior when already attached
- Error handling for detached state
- Error handling for other states
The test cases are well-structured and include appropriate assertions for each scenario.
Also applies to: 1413-1453, 1459-1469, 1475-1519
1482-1482:
Investigate compiler crash with #expect throws.
The comments indicate a compiler crash when using #expect(..., throws:). This should be investigated and reported as it might indicate a compiler bug.
Also applies to: 1507-1507
Note: This PR is based on top of #118; please review that one first.
This should unblock @umair-ably’s implementation of presence and typing indicators. See commit messages for more details.
Resolves #112.
Summary by CodeRabbit
New Features
Helpersfunction to validate error messages.MockFeatureChannelto handle presence operation readiness.Bug Fixes
Tests