[ECO-4947] Spec complete for typing indicators #141
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe pull request introduces significant enhancements to the chat functionality within the AblyChat application. Key modifications include the implementation of typing indicators, improved connection status handling, and the introduction of a new Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
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 pause |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (7)
Sources/AblyChat/TimerManager.swift (2)
3-4: Add documentation comments for the actor.Consider adding documentation comments to explain the purpose and usage of this actor, especially since it's part of the public API for typing indicators.
+/// An actor that manages timer-based operations in a thread-safe manner. +/// Used primarily for handling typing indicator timeouts and other time-based events. internal final actor TimerManager { private var currentTask: Task<Void, Never>?
23-25: Consider checking task status for more accuracy.The current implementation might return true even if the task has completed but hasn't been cleared yet.
Consider this more accurate implementation:
internal func hasRunningTask() -> Bool { - currentTask != nil + if let task = currentTask { + return !task.isCancelled + } + return false }Sources/AblyChat/Room.swift (1)
Line range hint
67-228: Consider extracting feature initialization logic.The feature initialization pattern is repeated for multiple features (messages, reactions, presence, occupancy, and now typing). Consider extracting this into a factory pattern or builder to:
- Reduce code duplication
- Make feature addition more maintainable
- Centralize feature initialization logic
This would make future feature additions easier and reduce the risk of inconsistencies in initialization patterns.
Tests/AblyChatTests/IntegrationTests.swift (2)
Line range hint
93-177: Consider documenting the rationale for delay durationsThe test structure for reactions, occupancy, and presence is well-organized. The 2-second delays are necessary for eventual consistency in occupancy updates. Consider adding a brief comment explaining why these specific delay durations were chosen.
Add a comment explaining the rationale for the 2-second delay duration, for example:
- // It can take a moment for the occupancy to update from the clients connecting above, so we'll wait a 2 seconds here. + // Wait 2 seconds for occupancy updates to propagate through the Ably system. + // This duration was chosen based on empirical testing and typical network latency considerations.
178-206: Well-structured typing indicator tests, consider additional edge casesThe typing indicator test implementation is solid and covers the basic flow:
- Subscription to typing events
- Verification of typing start
- Verification of automatic timeout
Consider adding these additional test cases:
- Multiple users typing simultaneously
- Manual stop typing (vs timeout)
- Reconnection scenarios
Example test case for multiple users typing:
// Test multiple users typing simultaneously try await txRoom.typing.start() try await rxRoom.typing.start() var multipleTypingEvents: [TypingEvent] = [] for await typingEvent in rxTypingSubscription { multipleTypingEvents.append(typingEvent) if typingEvent.currentlyTyping.count == 2 { break } } #expect(multipleTypingEvents.last?.currentlyTyping.count == 2)Example/AblyChatExample/ContentView.swift (1)
232-242: Add error handling for typing subscriptionWhile the implementation correctly handles the typing events, it should include error handling for the subscription stream to maintain stability.
Task { + do { for await typing in typingSubscription { withAnimation { typingInfo = typing.currentlyTyping.isEmpty ? "" : "Typing: \(typing.currentlyTyping.joined(separator: ", "))..." } } + } catch { + print("Typing subscription error: \(error)") + // Consider implementing retry logic or user notification + } }Sources/AblyChat/DefaultTyping.swift (1)
79-101: Simplify Asynchronous Logic inget()MethodIn the
get()method, you're usingwithCheckedThrowingContinuationto bridge the callback-basedpresence.getmethod to async/await (lines 90-101). While this is acceptable, since we're refactoring other methods, consider using anasyncalternative if available for consistency and readability.Proposed Improvement: Use Async Alternative if Available
If the Ably SDK provides an
asyncversion ofpresence.get, you can simplify the method:// (CHA-T2) Users may retrieve a list of the currently typing client IDs... internal func get() async throws -> Set<String> { logger.log(message: "Getting presence", level: .debug) // CHA-T2c to CHA-T2f do { try await featureChannel.waitToBeAbleToPerformPresenceOperations(requestedByFeature: RoomFeature.presence) } catch { logger.log(message: "Error waiting to perform presence get: \(error)", level: .error) throw error } - return try await withCheckedThrowingContinuation { continuation in - channel.presence.get { [processPresenceGet] members, error in - do { - let presenceMembers = try processPresenceGet(members, error) - continuation.resume(returning: presenceMembers) - } catch { - continuation.resume(throwing: error) - // processPresenceGet will log any errors - } - } - } + let members = try await channel.presence.get() + let presenceMembers = try processPresenceGet(members, nil) + return presenceMembers }Ensure that
channel.presence.get()supportsasync/await. This change improves code readability and maintains consistency with other async methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
Example/AblyChatExample/ContentView.swift(6 hunks)Sources/AblyChat/DefaultConnection.swift(0 hunks)Sources/AblyChat/DefaultTyping.swift(1 hunks)Sources/AblyChat/Room.swift(4 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Sources/AblyChat/RoomOptions.swift(1 hunks)Sources/AblyChat/TimerManager.swift(1 hunks)Tests/AblyChatTests/IntegrationTests.swift(5 hunks)
💤 Files with no reviewable changes (1)
- Sources/AblyChat/DefaultConnection.swift
🔇 Additional comments (13)
Sources/AblyChat/TimerManager.swift (2)
18-21: LGTM!
The cancellation implementation is correct and thread-safe.
1-26: Verify timer cleanup in parent components.
Since this TimerManager is crucial for typing indicators, ensure proper cleanup is implemented in parent components to avoid memory leaks.
✅ Verification successful
Let me check how these components handle cleanup by examining their implementations.
Timer cleanup is properly implemented in parent components
The verification shows that timer cleanup is well-handled in both parent components:
-
In
DefaultTyping:- Timer is automatically cancelled when
stop()is called - Uses weak self references to prevent retain cycles
- Timer tasks are properly cleaned up when typing sessions end
- Timer is automatically cancelled when
-
In
DefaultConnection:- Implements proper timer cancellation when connection state changes
- Uses weak self references in callbacks
- Cleans up timers when superseding states occur
The implementation follows best practices for memory management and timer cleanup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TimerManager usage to verify proper cleanup
rg -A 5 "TimerManager"
Length of output: 1579
Script:
#!/bin/bash
# Check DefaultTyping.swift implementation for cleanup
cat Sources/AblyChat/DefaultTyping.swift
# Check DefaultConnection.swift implementation for cleanup
cat Sources/AblyChat/DefaultConnection.swift
Length of output: 17776
Sources/AblyChat/RoomOptions.swift (2)
30-30: LGTM! Clear and well-documented specification.
The comment clearly documents the typing timeout configuration option and its default value, following the specification format.
32-34: Implementation matches spec, but note the breaking change.
The timeout reduction from 10s to 5s matches the specification but represents a breaking change that could affect existing implementations.
Let's verify the consistent usage of this timeout value across the codebase:
✅ Verification successful
Breaking change verified: timeout reduction from 10s to 5s is consistently implemented
The codebase scan confirms that:
- The new 5-second default timeout is correctly implemented in both
RoomOptions.swiftandRoom.swift - No legacy references to the old 10-second timeout for typing were found
- The change aligns with the documented specification comment stating "The default is 5000ms"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any hardcoded timeout values that might need updating
# Look for any numeric values that might be related to typing timeouts
rg -g '!*.{md,txt,json}' '(typing.*?(10|5000|10000|5))|([^a-zA-Z]([5]|10)\s*(seconds|s\b))'
Length of output: 1051
Script:
#!/bin/bash
# Let's check for any test files that might be verifying the timeout behavior
rg -l "test.*typing.*timeout|typing.*timeout.*test" --type-add 'swift:*.swift'
# Also check for any direct references to the old 10-second timeout
rg "10\s*\)" --type swift
Length of output: 1962
Sources/AblyChat/RoomFeature.swift (1)
26-27: LGTM! Implementation aligns with the specification.
The implementation correctly follows the CHA-T1 specification by using the dedicated channel format <roomId>::$chat::$typingIndicators for typing indicators.
Let's verify that the typing channel name is used consistently across the codebase:
✅ Verification successful
Let me gather more information to ensure the typing indicators implementation is complete and consistent.
Implementation is consistent with the specification and used correctly across the codebase
The typing indicators channel name implementation is correctly used throughout the codebase:
- The channel name format
<roomId>::$chat::$typingIndicatorsis properly defined inRoomFeature.swift - The implementation is consistently used in
DefaultTyping.swiftwhich handles all typing-related operations - Integration tests verify the typing channel functionality
- The feature is properly integrated into the room lifecycle management
- Error handling specifically for typing channels is implemented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the typing channel name usage
# Expected: All typing-related channel names should follow the format <roomId>::$chat::$typingIndicators
# Search for typing channel name usage
rg -l "typingIndicators"
# Search for potential inconsistent channel name formats
rg "::.*typing"
Length of output: 382
Script:
#!/bin/bash
# Search for any typing-related code to verify implementation completeness
rg -A 5 -B 5 "typing" --type swift
# Search for channel name construction to verify consistent usage
ast-grep --pattern 'case typing = "typingIndicators"'
Length of output: 38874
Sources/AblyChat/Room.swift (3)
Line range hint 8-10: LGTM! Documentation and property declaration are consistent.
The typing property follows the established pattern for optional features, with clear documentation about the fatal error case.
67-67: LGTM! Property declaration follows established patterns.
The _typing property declaration maintains consistency with other optional feature properties in the class.
190-190: LGTM! Feature channel setup is correct.
The typing feature is properly integrated into the feature channel creation system.
Tests/AblyChatTests/IntegrationTests.swift (3)
66-89: LGTM: Well-structured message testing sequence
The message testing section has been improved with clear sequential numbering and descriptive comments. The test flow is logical and covers important scenarios:
- Pre-subscription message sending
- Subscription setup
- Post-subscription message verification
- Historical message retrieval
209-227: LGTM: Thorough cleanup and verification
The cleanup section ensures proper resource management by:
- Detaching the room and verifying status
- Releasing the room and verifying status
- Confirming new room creation post-release
39-39: LGTM: Appropriate test timeout configuration
The 2-second timeout for typing indicators is appropriate for testing purposes, while being shorter than the production default of 5 seconds to keep test execution time reasonable.
Let's verify the default timeout value in the production code:
Also applies to: 48-48
✅ Verification successful
Verified: Test timeout value is appropriately configured
The 2-second timeout in tests is a good choice since the production code (RoomOptions.swift) shows a default timeout of 5 seconds, as evidenced by:
public var timeout: TimeInterval = 5
public init(timeout: TimeInterval = 5)Using a shorter timeout in tests helps maintain quick test execution while still validating the typing indicator functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the default typing timeout in RoomOptions
rg -A 2 "TypingOptions.*init" Sources/
Length of output: 40
Script:
#!/bin/bash
# Let's try different approaches to find typing-related configurations
# Search for typing-related code in Swift files
rg -t swift "typing|Typing" Sources/
# Also search for any timeout configurations
rg -t swift "timeout.*=.*[0-9]" Sources/
Length of output: 8185
Example/AblyChatExample/ContentView.swift (1)
61-63: LGTM: Room initialization properly configured for typing feature
The typing feature is correctly initialized alongside other room features.
Sources/AblyChat/DefaultTyping.swift (1)
211-223: Efficient Use of EventTracker Actor for Concurrency Control
The EventTracker actor correctly manages concurrent presence events by ensuring thread-safe updates and checks of event IDs. This helps prevent race conditions and ensures that only the latest typing events are processed.
✅ Actions performedReviews paused. |
|
I would suggest filter out own typings. |
some users might not run it on 2 separate simulators so i think it's still nice to see in the demo app |
Spec complete for typing indicators
typing.indicators.mov
add unit tests as part of this issue
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests