[ECO-4948] Runs room checks prior to performing presence operations #135
Conversation
WalkthroughThe pull request introduces significant updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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: 0
🧹 Outside diff range and nitpick comments (11)
Example/AblyChatExample/ContentView.swift (6)
185-189: Add error handling in backgroundTaskfor message subscriptionIn the
showMessages()function, the backgroundTaskmay encounter errors during the message subscription loop that are not currently handled. To prevent unhandled exceptions, consider wrapping the loop in ado-catchblock.Apply this diff to include error handling:
Task { + do { for await message in messagesSubscription { withAnimation { messages.insert(BasicListItem(id: message.timeserial, title: message.clientID, text: message.text), at: 0) } } + } catch { + // Handle error appropriately + print("Error receiving messages: \(error)") + } }
198-202: Add error handling in backgroundTaskfor reaction subscriptionIn the
showReactions()function, the backgroundTaskhandling reactions may throw errors that are not caught. Wrap the subscription loop in ado-catchblock to handle potential exceptions.Apply this diff:
Task { + do { for await reaction in reactionSubscription { withAnimation { showReaction(reaction.displayedText) } } + } catch { + // Handle error appropriately + print("Error receiving reactions: \(error)") + } }
211-219: Add error handling in backgroundTaskfor presence eventsThe
TaskinshowPresence()that listens for presence events may encounter errors. To enhance robustness, wrap the for-await loop in ado-catchblock.Apply this diff:
Task { + do { for await event in try await room().presence.subscribe(events: [.enter, .leave, .update]) { withAnimation { let status = event.data?.userCustomData?["status"]?.value as? String let clientPresenceChangeMessage = "\(event.clientID) \(event.action.displayedText)" let presenceMessage = status != nil ? "\(clientPresenceChangeMessage) with status: \(status!)" : clientPresenceChangeMessage messages.insert(BasicListItem(id: UUID().uuidString, title: "System", text: presenceMessage), at: 0) } } + } catch { + // Handle error appropriately + print("Error receiving presence events: \(error)") + } }
226-234: Add error handling in backgroundTaskfor typing eventsIn
showTypings(), the backgroundTasksubscribing to typing events lacks error handling. Consider adding ado-catchblock to manage potential errors.Apply this diff:
Task { + do { for await typing in try await room().typing.subscribe(bufferingPolicy: .unbounded) { withAnimation { typingInfo = "Typing: \(typing.currentlyTyping.joined(separator: ", "))..." Task { try? await Task.sleep(nanoseconds: 1 * 1_000_000_000) withAnimation { typingInfo = "" } } } } + } catch { + // Handle error appropriately + print("Error receiving typing events: \(error)") + } }
248-252: Add error handling in backgroundTaskfor occupancy updatesThe
TaskinshowOccupancy()that listens for occupancy events does not handle errors. Wrapping the loop in ado-catchblock will help manage any exceptions that occur.Apply this diff:
Task { + do { for await event in try await room().occupancy.subscribe(bufferingPolicy: .unbounded) { withAnimation { occupancyInfo = "Connections: \(event.presenceMembers) (\(event.connections))" } } + } catch { + // Handle error appropriately + print("Error receiving occupancy updates: \(error)") + } }
259-271: Add error handling in backgroundTaskfor room status changesIn
showRoomStatus(), errors within theTasksubscribing to room status changes may go unhandled. Include ado-catchblock to handle potential errors.Apply this diff:
Task { + do { for await status in try await room().onStatusChange(bufferingPolicy: .unbounded) { withAnimation { if status.current.isAttaching { statusInfo = "\(status.current)...".capitalized } else { statusInfo = "\(status.current)".capitalized if status.current == .attached { Task { try? await Task.sleep(nanoseconds: 1 * 1_000_000_000) withAnimation { statusInfo = "" } } } } } } + } catch { + // Handle error appropriately + print("Error receiving room status changes: \(error)") + } }Tests/AblyChatTests/IntegrationTests.swift (2)
115-124: Consider making the occupancy verification more robust.The current implementation uses a hardcoded 2-second sleep to wait for occupancy updates. This approach might be fragile in different environments or under varying load conditions.
Consider implementing a polling mechanism with a timeout instead:
-// It can take a moment for the occupancy to update from the clients entering presence above, so we'll wait 2 seconds here. -try await Task.sleep(nanoseconds: 2_000_000_000) +// Poll for occupancy updates with a timeout +func waitForOccupancyUpdate(room: Room, expectedPresenceMembers: Int, timeout: TimeInterval = 5) async throws { + let start = Date() + while Date().timeIntervalSince(start) < timeout { + let occupancy = try await room.occupancy.get() + if occupancy.presenceMembers == expectedPresenceMembers { + return + } + try await Task.sleep(nanoseconds: 100_000_000) // 100ms + } + throw TimeoutError("Occupancy update didn't reach expected value within timeout") +} + +try await waitForOccupancyUpdate(room: rxRoom, expectedPresenceMembers: 1)
Line range hint
125-136: Standardize the approach for timing-dependent verifications.Similar to the previous section, this segment uses a hardcoded sleep for occupancy updates after presence leave operation.
Consider extending the polling mechanism to handle both presence enter and leave operations:
+// Add a new case to handle presence leave +enum PresenceOperation { + case enter + case leave +} + +func waitForOccupancyUpdate(room: Room, operation: PresenceOperation, timeout: TimeInterval = 5) async throws { + let expectedMembers = operation == .enter ? 1 : 0 + let start = Date() + while Date().timeIntervalSince(start) < timeout { + let occupancy = try await room.occupancy.get() + if occupancy.presenceMembers == expectedMembers { + return + } + try await Task.sleep(nanoseconds: 100_000_000) // 100ms + } + throw TimeoutError("Occupancy update didn't reach expected value within timeout") +}Sources/AblyChat/DefaultPresence.swift (3)
98-105: LGTM! Consider adding retry mechanism for critical operations.The error handling is well-implemented. Given that enter() is a critical operation for user presence, consider implementing a retry mechanism with exponential backoff for transient failures.
145-152: LGTM! Consider ensuring reliable cleanup.The error handling is well-implemented. Since leave() is crucial for cleaning up presence state, consider implementing a mechanism to ensure eventual consistency of the presence set, even if the initial leave operation fails.
Line range hint
24-152: Excellent implementation of the room readiness checks.The consistent error handling pattern across all presence operations creates a robust and maintainable implementation. A few architectural considerations for future improvements:
- Consider implementing a circuit breaker pattern for presence operations to prevent overwhelming the system during issues
- Add telemetry for tracking failed presence operations to help identify patterns
- Consider implementing presence state reconciliation mechanisms for handling edge cases
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Example/AblyChatExample/ContentView.swift(2 hunks)Sources/AblyChat/DefaultPresence.swift(6 hunks)Tests/AblyChatTests/IntegrationTests.swift(2 hunks)
🧰 Additional context used
📓 Learnings (1)
Example/AblyChatExample/ContentView.swift (2)
Learnt from: maratal
PR: ably-labs/ably-chat-swift#34
File: Example/AblyChatExample/ContentView.swift:27-33
Timestamp: 2024-11-12T15:07:39.465Z
Learning: In `ContentView.swift`, the `room()` method does not use force try; it uses `try await` and handles errors appropriately.
Learnt from: maratal
PR: ably-labs/ably-chat-swift#34
File: Example/AblyChatExample/ContentView.swift:35-118
Timestamp: 2024-11-12T15:07:31.865Z
Learning: Skip code review suggestions for `ContentView.swift` in the `Example/AblyChatExample` project.
🔇 Additional comments (8)
Example/AblyChatExample/ContentView.swift (1)
137-144: Ensure intentional error handling in consolidated .tryTask block
By consolidating multiple asynchronous functions into a single .tryTask, any error thrown by one function will prevent subsequent functions from executing. Please verify that this behavior is intended and acceptable for the application's flow.
Tests/AblyChatTests/IntegrationTests.swift (3)
137-176: LGTM! Comprehensive presence event testing.
The test coverage for presence events is thorough, including:
- Verification of all presence event types (enter, update, leave)
- Validation of custom data persistence
- Testing from both client perspectives
178-196: LGTM! Thorough cleanup and verification.
The cleanup process is well-structured with proper verification of:
- Room detachment
- Status change confirmation
- Room release
- Object reference validation
112-114: LGTM! Room attachment is correctly placed before presence operations.
The addition of this step ensures that the room is properly attached before performing any presence operations, which is a critical prerequisite.
Let's verify the presence operation requirements in the codebase:
✅ Verification successful
Let me verify if these presence operations are properly guarded by room attachment by checking the surrounding context.
Let me check if there are any other presence operations in the codebase that might not be properly guarded by room attachment.
Based on the comprehensive analysis of the codebase, I can now generate the final response:
Room attachment is correctly implemented as a prerequisite for presence operations
The verification confirms that:
- The room attachment is properly placed before any presence operations in the integration tests
- The
RoomLifecycleManagerensures proper attachment handling - The presence operations are consistently performed after room attachment across the codebase
- The implementation follows the correct pattern where presence operations require an attached channel state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that presence operations are consistently guarded by room attachment checks
# Look for presence operations and their relationship with room attachment
ast-grep --pattern 'try await $_.presence.$_($$$)'
Length of output: 1186
Script:
#!/bin/bash
# Get the context around presence operations to verify attachment
rg "presence\." Tests/AblyChatTests/IntegrationTests.swift -B 5 -A 1
Length of output: 3827
Script:
#!/bin/bash
# Search for room attachment patterns and presence operations in the entire codebase
rg "attach\(\)" -A 10
Length of output: 22248
Sources/AblyChat/DefaultPresence.swift (4)
24-31: LGTM! Well-structured error handling for presence operations.
The implementation properly ensures room readiness before performing presence operations, with appropriate error handling and logging. This aligns well with the PR objective of running room checks prior to presence operations.
48-55: LGTM! Consistent error handling pattern maintained.
The implementation follows the same robust error handling pattern as the get() method, maintaining consistency across the codebase.
121-129: LGTM! Comprehensive error handling implementation.
The implementation maintains consistency with other presence operations while providing detailed error information.
73-80: LGTM! Consider verifying error handling scenarios.
The implementation maintains the consistent error handling pattern. Consider adding integration tests to verify the behavior when the room is not ready for presence operations.
✅ Verification successful
Error handling is well-tested, no additional verification needed
The error handling for waitToBeAbleToPerformPresenceOperations is thoroughly tested in DefaultRoomLifecycleManagerTests.swift with comprehensive test cases covering:
- Success scenario when channel is attaching and attach succeeds
- Failure scenario when channel attach fails
- Behavior when already attached
- Error cases for detached state
- Error cases for other invalid states
Each scenario includes proper error propagation and validation of error messages, making additional integration tests unnecessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for integration tests covering error scenarios
rg -l "isUserPresent.*throw" "Tests/"
Length of output: 37
Script:
#!/bin/bash
# Search for test files containing isUserPresent method
rg -l "isUserPresent" "Tests/"
Length of output: 28
Script:
#!/bin/bash
# Search for test files containing waitToBeAbleToPerformPresenceOperations
rg -l "waitToBeAbleToPerformPresenceOperations" "Tests/"
Length of output: 221
Script:
#!/bin/bash
# Search for presence-related test files
fd "Presence.*\.swift" "Tests/"
Length of output: 31
Script:
#!/bin/bash
# Let's check the content of DefaultRoomLifecycleManagerTests.swift to see how waitToBeAbleToPerformPresenceOperations is tested
rg "waitToBeAbleToPerformPresenceOperations" -A 10 "Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift"
Length of output: 8982
Script:
#!/bin/bash
# Let's also check the mock implementation to understand the test coverage
rg "waitToBeAbleToPerformPresenceOperations" -A 5 "Tests/AblyChatTests/Mocks/MockFeatureChannel.swift"
Length of output: 512
…Channel.waitToBeAbleToPerformPresenceOperations` which was implemented as part of [1]. It also became necessary to clean up the example app as this uncovered race conditions between concurrently running Tasks.
umair-ably
force-pushed
the
ECO-4948-v2
branch
from
27d748e to
370d444
Compare
There was a problem hiding this comment.
Left two minor suggestions. I'm not sure about changes in ContentView.swift wdyt @lawrence-forooghian
Follow up PR from ECO-4948
Runs room checks prior to performing presence operations. Also tidied up the example app.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation