[ECO-4946] feat: typing indicators #58
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several modifications across various classes in the chat application. Notably, it enhances logging capabilities by adding a 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 (
|
| ) : Typing { | ||
| private val typingIndicatorsChannelName = "$roomId::\$chat::\$typingIndicators" | ||
|
|
||
| override val channel: Channel | ||
| get() = realtimeClient.channels.get(typingIndicatorsChannelName, ChatChannelOptions()) | ||
| @OptIn(ExperimentalCoroutinesApi::class) |
There was a problem hiding this comment.
limitedParallelism has been marked stable as per latest kotlinx version.
ttypic
force-pushed
the
ECO-4946/typing-indicators
branch
from
6b21784
to
670fa45
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
chat-android/src/test/java/com/ably/chat/SandboxTest.kt (2)
44-44: Consider extracting the timeout value as a constant.The magic number
10_000would be more maintainable as a named constant, especially if it needs to be reused in other tests.+ private companion object { + const val TYPING_TIMEOUT_MS = 10_000L + } - val roomOptions = RoomOptions(typing = TypingOptions(timeoutMs = 10_000)) + val roomOptions = RoomOptions(typing = TypingOptions(timeoutMs = TYPING_TIMEOUT_MS))
40-58: Good test implementation, consider adding timeout verification and cleanup.The test effectively verifies the basic typing indication functionality. However, consider these improvements:
- Add verification that typing indication expires after the timeout
- Add cleanup to detach clients after the test
- Consider edge cases (e.g., multiple typing starts, cancellation)
Here's a suggested enhancement:
@Test fun `should return typing indication for client`() = runTest { val chatClient1 = sandbox.createSandboxChatClient("client1") val chatClient2 = sandbox.createSandboxChatClient("client2") val roomId = UUID.randomUUID().toString() val roomOptions = RoomOptions(typing = TypingOptions(timeoutMs = TYPING_TIMEOUT_MS)) val chatClient1Room = chatClient1.rooms.get(roomId, roomOptions) chatClient1Room.attach() val chatClient2Room = chatClient2.rooms.get(roomId, roomOptions) chatClient2Room.attach() try { val deferredValue = CompletableDeferred<TypingEvent>() chatClient1Room.typing.subscribe { deferredValue.complete(it) } chatClient1Room.typing.start() val typingEvent = deferredValue.await() assertEquals(setOf("client1"), typingEvent.currentlyTyping) assertEquals(setOf("client1"), chatClient2Room.typing.get()) + // Verify timeout + delay(TYPING_TIMEOUT_MS + 100) + assertTrue(chatClient2Room.typing.get().isEmpty()) } finally { + chatClient1Room.detach() + chatClient2Room.detach() } }chat-android/src/main/java/com/ably/chat/Rooms.kt (2)
50-50: Add KDoc documentation for the logger parameterConsider adding parameter documentation to help other developers understand the purpose and requirements of the logger.
internal class DefaultRooms( private val realtimeClient: RealtimeClient, private val chatApi: ChatApi, override val clientOptions: ClientOptions, private val clientId: String, + /** + * Logger instance for tracking room lifecycle events and debugging + */ private val logger: Logger, ) : Rooms {
Line range hint
1-83: Consider adding typing-specific logging pointsSince this is part of the typing indicators feature, consider defining specific logging points for typing state changes to aid in debugging real-time typing indicator functionality. This could include:
- Room member typing state changes
- Typing indicator broadcast events
- Typing timeout events
chat-android/src/main/java/com/ably/chat/Room.kt (1)
110-114: Consider adding property documentationThe implementation of custom getters with backing properties is well-structured. Consider adding KDoc comments to document the backing property pattern usage for better maintainability.
+ /** + * Backing property pattern implementation for messages. + * Provides controlled access to the underlying DefaultMessages instance. + */ override val messages: Messages get() = _messages + /** + * Backing property pattern implementation for typing. + * Provides controlled access to the underlying DefaultTyping instance. + */ override val typing: Typing get() = _typingchat-android/src/main/java/com/ably/chat/Presence.kt (1)
142-143: Consider extracting the mapping logic to a separate function.The implementation looks good and the direct parameter approach is cleaner than the previous List. However, the mapping logic could be extracted to improve reusability and maintainability.
Consider applying this refactor:
+ private fun mapToPresenceMember(user: PresenceMessage) = PresenceMember( + clientId = user.clientId, + action = user.action, + data = (user.data as? JsonObject)?.get("userCustomData"), + updatedAt = user.timestamp, + extras = user.extras?.data?.asJsonObject?.entrySet()?.associate { it.key to it.value.asString } + ) override suspend fun get(waitForSync: Boolean, clientId: String?, connectionId: String?): List<PresenceMember> { - return presence.getCoroutine(waitForSync, clientId, connectionId).map { user -> - PresenceMember( - clientId = user.clientId, - action = user.action, - data = (user.data as? JsonObject)?.get("userCustomData"), - updatedAt = user.timestamp, - ) - } + return presence.getCoroutine(waitForSync, clientId, connectionId).map(::mapToPresenceMember) }chat-android/src/main/java/com/ably/chat/Utils.kt (1)
79-128: Consider reducing code duplication across coroutine wrappers.The three client coroutine functions share very similar implementation structure. Consider extracting the common pattern into a higher-order function:
private suspend inline fun PubSubPresence.clientCoroutineWrapper( clientId: String, data: JsonElement?, crossinline action: (String, JsonElement?, CompletionListener) -> Unit ) = suspendCancellableCoroutine { continuation -> action( clientId, data, object : CompletionListener { override fun onSuccess() { continuation.resume(Unit) } override fun onError(reason: ErrorInfo?) { continuation.resumeWithException(AblyException.fromErrorInfo(reason)) } } ) } // Usage: suspend fun PubSubPresence.enterClientCoroutine( clientId: String, data: JsonElement? = JsonNull.INSTANCE ) = clientCoroutineWrapper(clientId, data, ::enterClient)example/src/main/java/com/ably/chat/example/MainActivity.kt (2)
205-210: Throttle typing indicators to reduce unnecessary network callsCalling
room.typing.start()on every character input can lead to excessive network traffic and potential performance issues. Implementing a debounce mechanism will limit how often the typing indicator is sent.Apply this diff to implement debouncing:
var messageText by remember { mutableStateOf(TextFieldValue("")) } +var typingJob: Job? = null ChatInputField( sending = sending, messageInput = messageText, onMessageChange = { messageText = it + typingJob?.cancel() + typingJob = coroutineScope.launch { + delay(500) // Wait for 500ms after the last input + room.typing.start() + } }, onSendClick = { /* ... */ }, onReactionClick = { /* ... */ }, )
113-113: Display a user-friendly room titleUsing
Settings.ROOM_IDdirectly as the title might expose internal IDs or be less readable for users. Consider mapping the room ID to a user-friendly name.Example adjustment:
TopAppBar( title = { - Text(Settings.ROOM_ID) + Text("Chat Room") }, actions = { /* ... */ }, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
chat-android/src/main/java/com/ably/chat/ChatClient.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Presence.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Room.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Rooms.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Typing.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Utils.kt(2 hunks)chat-android/src/test/java/com/ably/chat/SandboxTest.kt(2 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(7 hunks)
🔇 Additional comments (14)
chat-android/src/main/java/com/ably/chat/ChatClient.kt (1)
65-65: LGTM! Verify logger initialization in DefaultRooms.
The addition of the logger parameter is consistent with the logging enhancement pattern. The logger is properly initialized with context and configuration options.
Let's verify the logger parameter is properly handled in DefaultRooms:
✅ Verification successful
Logger parameter is properly initialized and used in DefaultRooms
The verification confirms that:
- The
DefaultRoomsclass correctly accepts theloggerparameter in its constructor - The logger is properly passed down to
DefaultRoominstances in thegetmethod - The implementation follows a consistent pattern of logger propagation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify DefaultRooms constructor accepts and initializes the logger parameter
# Check DefaultRooms constructor definition
ast-grep --pattern 'class DefaultRooms(
$$$
logger: Logger,
$$$
) {
$$$
}'
# Look for logger initialization or usage in DefaultRooms
rg -l "logger\." "chat-android/src/main/java/com/ably/chat/Rooms.kt"
Length of output: 153
Script:
#!/bin/bash
# Let's try a different approach to find DefaultRooms and logger usage
# Search for DefaultRooms class definition with broader pattern
ast-grep --pattern 'class DefaultRooms'
# Search for logger usage with context
rg "logger" "chat-android/src/main/java/com/ably/chat/Rooms.kt" -A 2 -B 2
# Check if Rooms.kt exists and show its content
fd "Rooms.kt" --exec cat {}
Length of output: 3218
chat-android/src/main/java/com/ably/chat/Rooms.kt (1)
Line range hint 55-71: LGTM! Verify DefaultRoom constructor update
The synchronized room creation and error handling look good. The logger is correctly passed to DefaultRoom.
Let's verify the DefaultRoom constructor has been updated to accept the logger:
✅ Verification successful
DefaultRoom constructor is correctly updated with logger parameter
The verification confirms that the DefaultRoom constructor in chat-android/src/main/java/com/ably/chat/Room.kt has been properly updated to include the logger parameter:
internal class DefaultRoom(
override val roomId: String,
override val options: RoomOptions,
private val realtimeClient: RealtimeClient,
chatApi: ChatApi,
clientId: String,
private val logger: Logger,
) : Room🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify DefaultRoom constructor matches the new parameter
ast-grep --pattern 'class DefaultRoom(
$$$
logger: Logger,
$$$
)'
Length of output: 73
Script:
#!/bin/bash
# Let's try a broader search for DefaultRoom class definition
ast-grep --pattern 'class DefaultRoom'
# Also search for the constructor parameters
rg "class DefaultRoom.*\(" -A 10
Length of output: 2993
chat-android/src/main/java/com/ably/chat/Room.kt (3)
90-90: LGTM: Improved encapsulation and logging capabilities
The changes enhance the class design by:
- Making
realtimeClientprivate, which improves encapsulation - Adding structured logging support through the
loggerparameter
Also applies to: 93-93
151-151: LGTM: Proper resource cleanup
The addition of _typing.release() ensures proper cleanup of typing-related resources.
Let's verify the release implementation in DefaultTyping:
✅ Verification successful
Proper resource cleanup confirmed
The release() implementation in DefaultTyping properly cancels the typingScope coroutine scope, which ensures cleanup of all typing-related coroutines including the typing timer job and event processing. This matches the cleanup pattern used in other components and prevents potential memory leaks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for release method implementation in DefaultTyping
ast-grep --pattern 'class DefaultTyping($$$) {
$$$
fun release() {
$$$
}
$$$
}'
Length of output: 93
Script:
#!/bin/bash
# Let's try a broader search for DefaultTyping class and its implementation
rg -l "class DefaultTyping"
# Then search for any release method in the codebase related to typing
rg "fun release" -A 5
Length of output: 2250
Script:
#!/bin/bash
# Let's examine the DefaultTyping class implementation
cat chat-android/src/main/java/com/ably/chat/Typing.kt
Length of output: 7095
102-108: LGTM: Well-structured typing implementation
The DefaultTyping initialization is properly configured with all necessary parameters and follows good practices:
- Uses appropriate scoping with private visibility
- Includes contextual logging with specific tag
- Properly integrates with room configuration
Let's verify the DefaultTyping implementation:
✅ Verification successful
Typing implementation verified and properly structured
The DefaultTyping implementation correctly handles all the parameters passed in the initialization:
- Properly uses the
roomIdto construct the typing indicators channel name - Integrates with
realtimeClientfor channel operations - Maintains
clientIdfor member identification - Incorporates
optionsfor typing configuration - Utilizes the provided
loggerfor tracing and error logging
The implementation also includes proper presence handling, subscription management, and thread-safe listener collection, which aligns well with the typing indicator requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for DefaultTyping class implementation to ensure it handles all provided parameters
ast-grep --pattern 'class DefaultTyping($$$) {
$$$
}'
Length of output: 93
Script:
#!/bin/bash
# Let's try searching for DefaultTyping using ripgrep to locate the file first
rg "class DefaultTyping" -l
# Then search for its implementation with context
rg "class DefaultTyping" -A 20
Length of output: 1912
Script:
#!/bin/bash
# Let's check the complete implementation including methods
rg "class DefaultTyping" -A 50 chat-android/src/main/java/com/ably/chat/Typing.kt
# Also check the Typing interface it implements
rg "interface Typing" -A 10 chat-android/src/main/java/com/ably/chat/Typing.kt
Length of output: 2189
chat-android/src/main/java/com/ably/chat/Presence.kt (2)
153-153: LGTM!
The implementation is clean, efficient, and correctly uses the coroutine-based presence check.
Line range hint 142-153: Verify integration with typing indicators feature.
The presence API changes look well-suited for implementing typing indicators. However, let's verify how this integrates with the typing indicator feature implementation.
✅ Verification successful
Let me gather more information about the typing indicator implementation to verify the integration.
Presence API changes are well integrated with typing indicators implementation
The simplified Presence API is correctly integrated with the typing indicators feature. The implementation in Typing.kt effectively uses the presence methods:
- Uses
presence.getCoroutine()for retrieving typing status - Implements
enterClientCoroutineandleaveClientCoroutinefor managing typing state - Properly handles presence events through subscription
- Includes retry logic for presence operations
The presence API changes provide exactly what's needed for the typing indicators feature to work reliably.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for typing indicator implementations that use these presence methods
rg -l "presence\.(get|isUserPresent|enter|update|leave)" --type kotlin
# Search for typing-related presence data
ast-grep --pattern 'JsonObject().apply {
$$$
add("typing", $_)
$$$
}'
Length of output: 374
Script:
#!/bin/bash
# Check the implementation of typing indicators
cat chat-android/src/main/java/com/ably/chat/Typing.kt
# Look for presence usage in typing implementation
rg -A 5 -B 5 "presence\." chat-android/src/main/java/com/ably/chat/Typing.kt
Length of output: 8449
chat-android/src/main/java/com/ably/chat/Utils.kt (3)
4-4: LGTM: Imports are appropriate for the new functionality.
The added imports support the enhanced presence functionality with proper parameter handling.
Also applies to: 7-9
79-94: LGTM: Well-implemented coroutine wrapper with proper cancellation support.
The implementation follows Kotlin coroutines best practices:
- Uses suspendCancellableCoroutine for proper cancellation support
- Handles errors appropriately with AblyException
- Provides convenient default for data parameter
66-76: Verify default parameter values and spread operator usage.
The refactored function provides a more idiomatic Kotlin API. However:
- The default
waitForSync = trueshould be verified against Ably's best practices for presence synchronization. - Consider if the spread operator usage with
params.toTypedArray()could be replaced with a more direct approach.
✅ Verification successful
Let me gather more information about presence-related implementations and documentation.
Let me check one more aspect regarding the spread operator usage and parameter handling in similar contexts.
Implementation follows established patterns and best practices
The current implementation is well-aligned with the codebase patterns and practices:
- The
waitForSync = truedefault is appropriate as it ensures data consistency, and no contradicting usage was found in the codebase. - The spread operator with
toTypedArray()is consistently used across the codebase for parameter passing, particularly inChatApi.kt. - The
buildListpattern for parameter construction is also consistently used, as seen in similar implementations inChatApi.kt.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other waitForSync usages to verify the default value
rg -l "waitForSync.*=.*false" --type kotlin
# Search for Ably presence get calls to compare implementation
ast-grep --pattern 'presence.get($$$)'
Length of output: 302
Script:
#!/bin/bash
# Search for presence-related documentation and comments
rg -i "presence.*sync" --type kotlin -A 3
# Look for tests involving presence get operations
rg "test.*presence.*get" --type kotlin -A 5
# Search for any presence-related constants or configuration
rg "PRESENCE|presence.*default" --type kotlin
# Look for any presence-related interfaces or abstract classes
ast-grep --pattern 'interface $_ {
$$$
get($$$)
$$$
}'
Length of output: 2085
Script:
#!/bin/bash
# Look for similar parameter building patterns
ast-grep --pattern 'buildList {
$$$
}'
# Search for other spread operator usages with toTypedArray
rg "toTypedArray\(\)" --type kotlin -A 2
# Check for any Ably documentation comments about presence get
rg -i "@param.*waitForSync|@param.*clientId|@param.*connectionId" --type kotlin
Length of output: 2652
chat-android/src/main/java/com/ably/chat/Typing.kt (2)
205-205: Verify correctness of the exponential backoff calculation
The calculation for the delay duration in the exponential backoff is as follows:
val delayDuration = min(
PRESENCE_GET_RETRY_MAX_INTERVAL_MS,
PRESENCE_GET_RETRY_INTERVAL_MS * 2.0.pow(numRetries).toLong(),
)Ensure that this calculation aligns with the intended exponential backoff strategy. Specifically, verify that:
- The base retry interval is correctly doubled on each retry.
- The delay does not exceed
PRESENCE_GET_RETRY_MAX_INTERVAL_MS. - The
toLong()conversion does not cause unexpected truncation or overflow.
Run the following script to test the exponential backoff values:
This script will help confirm that the delay durations are calculated as expected.
✅ Verification successful
This will help us verify:
- The actual values of the constants being used
- The complete implementation of the delay duration calculation
- How these values are used in the retry mechanism
Exponential backoff implementation is correct
The implementation is mathematically sound and follows best practices:
- Base interval (1500ms) is correctly used as the starting point
- Exponential growth using
2.0.pow(numRetries)provides proper doubling on each retry - The
min()function correctly caps the delay at max interval (30,000ms) - The
toLong()conversion is safely applied after the power calculation, preventing any precision loss
The delay sequence will be:
- First retry: 1500ms
- Second retry: 3000ms
- Third retry: 6000ms
- Fourth retry: 12000ms
- Fifth retry: 24000ms
All subsequent retries: 30000ms (capped by max interval)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Calculate and display delay durations for retries.
# Test: Output delay durations for each retry to verify correctness.
# Expect: Delay durations should double each time until the maximum interval is reached.
PRESENCE_GET_RETRY_INTERVAL_MS=1500
PRESENCE_GET_RETRY_MAX_INTERVAL_MS=30000
PRESENCE_GET_MAX_RETRIES=5
for numRetries in $(seq 0 $PRESENCE_GET_MAX_RETRIES); do
delayDuration=$(echo "$PRESENCE_GET_RETRY_INTERVAL_MS * 2 ^ $numRetries" | bc)
delayDuration=$(echo "if ($delayDuration > $PRESENCE_GET_RETRY_MAX_INTERVAL_MS) $PRESENCE_GET_RETRY_MAX_INTERVAL_MS else $delayDuration" | bc)
echo "Attempt $numRetries: Delay Duration = $delayDuration ms"
done
Length of output: 71
Script:
#!/bin/bash
# Let's first check the actual constants defined in the code
rg "PRESENCE_GET_RETRY" -B 2 -A 2
# Then let's examine the entire retry mechanism implementation
ast-grep --pattern 'val delayDuration = $_'
Length of output: 1863
175-177:
Ensure 'release' method is invoked to prevent resource leaks
The release method cancels the typingScope to prevent potential memory leaks from lingering coroutines. It's crucial to ensure that release is called when a DefaultTyping instance is no longer needed, such as when closing a room or terminating the session.
Run the following script to search for invocations of the release method to confirm it is properly called:
✅ Verification successful
Let's search for more context about where and how DefaultTyping is used to understand the cleanup requirements.
release() is properly called in cleanup chain
The verification shows that the release() method is properly integrated into the cleanup chain:
Room.release()calls_typing.release()to cleanup typing resourcesRooms.ktinvokesroom?.release()when removing rooms- The cleanup chain ensures that when a room is closed or removed, all associated resources including the typing scope are properly released
This implementation follows a proper cleanup pattern where the parent component (Room) is responsible for releasing its child components (Typing), and the container (Rooms) handles the cleanup when removing items.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the 'release' method is invoked appropriately.
# Test: Search for calls to 'release' in the codebase.
# Expect: 'release' should be called wherever 'DefaultTyping' instances are no longer required.
rg 'typing\.release\(\)'
Length of output: 192
Script:
#!/bin/bash
# Let's try a more comprehensive search for release method calls
rg -A 2 -B 2 'release\(\)' --type kotlin
# Also search for potential cleanup patterns
rg -A 2 -B 2 'onCleared|onDestroy' --type kotlin
# Search for DefaultTyping usage
ast-grep --pattern 'class $_ : DefaultTyping'
ast-grep --pattern 'val $_ = DefaultTyping'
Length of output: 1659
Script:
#!/bin/bash
# Search for DefaultTyping instantiation and usage
rg -A 5 -B 5 'DefaultTyping'
# Look for potential lifecycle-related files
fd -e kt lifecycle
fd -e kt viewmodel
Length of output: 6263
example/src/main/java/com/ably/chat/example/MainActivity.kt (2)
295-311: Confirm accessibility of randomClientId
Ensure that randomClientId used in typingUsers is the same as the one used for the chat client. If they differ, it might lead to incorrect exclusion of the current user's typing status.
Run the following script to verify randomClientId consistency:
Ensure that randomClientId is declared once and consistently referenced throughout the file.
✅ Verification successful
Let me verify the context of randomClientId declaration to ensure it's accessible from the typingUsers function.
randomClientId is properly accessible and consistently used
The verification confirms that randomClientId is declared at file level scope in MainActivity.kt, making it accessible to all functions within the file, including typingUsers. It's consistently used across the file for:
- Chat client initialization (
clientId = randomClientId) - Message display logic
- Typing indicator exclusion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check that `randomClientId` is consistently used.
# Find all occurrences of `randomClientId`
rg 'randomClientId'
Length of output: 700
Script:
#!/bin/bash
# Get more context around the randomClientId declaration
rg -B 5 -A 5 'val randomClientId = UUID'
Length of output: 1033
98-107: 🛠️ Refactor suggestion
Use LaunchedEffect instead of DisposableEffect for coroutine side effects
Since you're performing side effects that involve coroutines when the composable enters and leaves the composition, LaunchedEffect is more appropriate. It handles coroutine scope and cancellation correctly without the need for an explicit coroutineScope.
Here's how you can refactor the code:
LaunchedEffect(Unit) {
room.attach()
awaitDispose {
room.detach()
}
}⛔ Skipped due to learnings
Learnt from: ttypic
PR: ably-labs/ably-chat-kotlin#16
File: example/src/main/java/com/ably/chat/example/MainActivity.kt:86-103
Timestamp: 2024-11-12T15:07:39.465Z
Learning: When using `rememberCoroutineScope`, the `CoroutineScope` is tied to the composable's lifecycle, so replacing `DisposableEffect` with `LaunchedEffect` is unnecessary.
| var numRetries = 0 | ||
| while (numRetries <= PRESENCE_GET_MAX_RETRIES) { | ||
| try { | ||
| val currentlyTyping = get() | ||
| emit(currentlyTyping) | ||
| return // Exit if successful | ||
| } catch (e: Exception) { | ||
| numRetries++ | ||
| val delayDuration = min( | ||
| PRESENCE_GET_RETRY_MAX_INTERVAL_MS, | ||
| PRESENCE_GET_RETRY_INTERVAL_MS * 2.0.pow(numRetries).toLong(), | ||
| ) | ||
| logger.debug("Retrying in $delayDuration ms... (Attempt $numRetries of $PRESENCE_GET_MAX_RETRIES)", e) | ||
| delay(delayDuration) | ||
| } | ||
| } | ||
| logger.error("Failed to get members after $PRESENCE_GET_MAX_RETRIES retries") |
There was a problem hiding this comment.
Address off-by-one error in the retry loop of 'processEvent'
In the processEvent function, the condition while (numRetries <= PRESENCE_GET_MAX_RETRIES) may result in one extra retry beyond the intended maximum. Since numRetries starts at 0, the loop can execute up to PRESENCE_GET_MAX_RETRIES + 1 times.
Consider adjusting the condition to ensure the maximum number of retries is not exceeded:
- Change the condition to
while (numRetries < PRESENCE_GET_MAX_RETRIES).
Apply this diff to correct the loop condition:
private suspend fun processEvent() {
var numRetries = 0
- while (numRetries <= PRESENCE_GET_MAX_RETRIES) {
+ while (numRetries < PRESENCE_GET_MAX_RETRIES) {
try {
val currentlyTyping = get()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var numRetries = 0 | |
| while (numRetries <= PRESENCE_GET_MAX_RETRIES) { | |
| try { | |
| val currentlyTyping = get() | |
| emit(currentlyTyping) | |
| return // Exit if successful | |
| } catch (e: Exception) { | |
| numRetries++ | |
| val delayDuration = min( | |
| PRESENCE_GET_RETRY_MAX_INTERVAL_MS, | |
| PRESENCE_GET_RETRY_INTERVAL_MS * 2.0.pow(numRetries).toLong(), | |
| ) | |
| logger.debug("Retrying in $delayDuration ms... (Attempt $numRetries of $PRESENCE_GET_MAX_RETRIES)", e) | |
| delay(delayDuration) | |
| } | |
| } | |
| logger.error("Failed to get members after $PRESENCE_GET_MAX_RETRIES retries") | |
| var numRetries = 0 | |
| while (numRetries < PRESENCE_GET_MAX_RETRIES) { | |
| try { | |
| val currentlyTyping = get() | |
| emit(currentlyTyping) | |
| return // Exit if successful | |
| } catch (e: Exception) { | |
| numRetries++ | |
| val delayDuration = min( | |
| PRESENCE_GET_RETRY_MAX_INTERVAL_MS, | |
| PRESENCE_GET_RETRY_INTERVAL_MS * 2.0.pow(numRetries).toLong(), | |
| ) | |
| logger.debug("Retrying in $delayDuration ms... (Attempt $numRetries of $PRESENCE_GET_MAX_RETRIES)", e) | |
| delay(delayDuration) | |
| } | |
| } | |
| logger.error("Failed to get members after $PRESENCE_GET_MAX_RETRIES retries") |
| DisposableEffect(Unit) { | ||
| coroutineScope.launch { | ||
| room.attach() | ||
| } | ||
| onDispose { | ||
| coroutineScope.launch { | ||
| room.detach() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid launching coroutines inside onDispose in DisposableEffect
Launching coroutines within the onDispose block of DisposableEffect can lead to coroutines that outlive the composable's lifecycle, potentially causing memory leaks or unexpected behavior. It's safer to perform the detach operation synchronously.
Apply this diff to fix the issue:
DisposableEffect(Unit) {
coroutineScope.launch {
room.attach()
}
onDispose {
- coroutineScope.launch {
room.detach()
- }
}
}Committable suggestion skipped: line range outside the PR's diff.
ttypic
force-pushed
the
ECO-4946/typing-indicators
branch
from
670fa45
to
2bb20af
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
chat-android/src/main/java/com/ably/chat/Presence.kt (2)
Line range hint
142-152: Add null safety checks and complete PresenceMember mappingThe implementation has a few potential improvements:
- Add null safety check for
user.clientId- Include
extrasmapping inPresenceMembercreation- Add safer type checking for data casting
Consider applying this improvement:
override suspend fun get(waitForSync: Boolean, clientId: String?, connectionId: String?): List<PresenceMember> { return presence.getCoroutine(waitForSync, clientId, connectionId).map { user -> PresenceMember( - clientId = user.clientId, + clientId = user.clientId ?: throw IllegalStateException("Presence member must have a clientId"), action = user.action, - data = (user.data as? JsonObject)?.get("userCustomData"), + data = when (user.data) { + is JsonObject -> user.data.get("userCustomData") + else -> user.data + }, updatedAt = user.timestamp, + extras = user.extras?.let { extras -> + extras.asJsonObject?.entrySet()?.associate { it.key to it.value.asString } + } ) } }
153-153: Consider optimizing presence checkThe current implementation fetches all presence members to check if a user is present. This could be inefficient for rooms with many members.
Consider optimizing by limiting the results:
-override suspend fun isUserPresent(clientId: String): Boolean = presence.getCoroutine(clientId = clientId).isNotEmpty() +override suspend fun isUserPresent(clientId: String): Boolean = presence.getCoroutine( + clientId = clientId, + limit = 1 +).isNotEmpty()chat-android/src/main/java/com/ably/chat/Utils.kt (2)
66-76: Add parameter validation and documentation.The function signature is more idiomatic now, but could benefit from:
- Parameter validation for edge cases
- KDoc documentation explaining the purpose of each parameter and their defaults
Add documentation and validation:
+/** + * Retrieves presence messages with optional filtering parameters. + * + * @param waitForSync Whether to wait for presence sync to complete before returning results + * @param clientId Filter results by specific client ID + * @param connectionId Filter results by specific connection ID + * @return List of presence messages matching the specified criteria + * @throws IllegalArgumentException if both clientId and connectionId are specified + */ suspend fun PubSubPresence.getCoroutine( waitForSync: Boolean = true, clientId: String? = null, connectionId: String? = null, ): List<PresenceMessage> = withContext(Dispatchers.IO) { + require(!(clientId != null && connectionId != null)) { + "Cannot filter by both clientId and connectionId simultaneously" + } val params = buildList { if (waitForSync) add(Param(GET_WAITFORSYNC, true)) clientId?.let { add(Param(GET_CLIENTID, it)) } connectionId?.let { add(Param(GET_CONNECTIONID, it)) } } get(*params.toTypedArray()).asList() }
66-128: Changes effectively support typing indicators feature.The enhanced presence functionality aligns well with the PR objectives:
- Improved presence message filtering supports tracking specific users' typing status
- Default JsonNull for presence data simplifies typing state updates
- Better coroutine support ensures smooth real-time typing indicators
Consider adding rate limiting for typing indicator updates to prevent excessive presence updates.
chat-android/src/main/java/com/ably/chat/Typing.kt (1)
123-129: Consider enhancing error handling in presence subscriptionWhile the null check for clientId is good, consider handling other potential exceptions that could occur during presence subscription to ensure robust error recovery.
channel.presence.subscribe { - if (it.clientId == null) { - logger.error("unable to handle typing event; no clientId", staticContext = mapOf("member" to it.toString())) - } else { - eventBus.tryEmit(Unit) - } + try { + if (it.clientId == null) { + logger.error("unable to handle typing event; no clientId", staticContext = mapOf("member" to it.toString())) + return@subscribe + } + eventBus.tryEmit(Unit) + } catch (e: Exception) { + logger.error("Error processing presence event", e, staticContext = mapOf("member" to it.toString())) + } }example/src/main/java/com/ably/chat/example/MainActivity.kt (2)
91-94: Add error handling for room initializationWhile the room initialization is correct, consider adding error handling for cases where room creation fails.
val room = chatClient.rooms.get( Settings.ROOM_ID, RoomOptions(typing = TypingOptions(), presence = PresenceOptions(), reactions = RoomReactionsOptions), - ) + ).also { room -> + room.onError = { error -> + Log.e("MainActivity", "Room error: ${error.message}") + // Handle error appropriately + } + }
127-135: Consider adding accessibility supportThe typing indicator UI looks good, but could benefit from accessibility improvements.
Text( modifier = Modifier.padding(start = 16.dp), text = "Currently typing: ${currentlyTyping.joinToString(", ")}", style = MaterialTheme.typography.bodySmall.copy( color = Color.Gray, ), + // Add accessibility support + modifier = Modifier + .padding(start = 16.dp) + .semantics { + contentDescription = "Users currently typing: ${currentlyTyping.joinToString(", ")}" + liveRegion = LiveRegion.Polite + } )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
chat-android/src/main/java/com/ably/chat/ChatClient.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Presence.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Room.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Rooms.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Typing.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Utils.kt(2 hunks)chat-android/src/test/java/com/ably/chat/SandboxTest.kt(2 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(7 hunks)gradle/libs.versions.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- chat-android/src/main/java/com/ably/chat/ChatClient.kt
- chat-android/src/main/java/com/ably/chat/Room.kt
- chat-android/src/main/java/com/ably/chat/Rooms.kt
- chat-android/src/test/java/com/ably/chat/SandboxTest.kt
🔇 Additional comments (8)
gradle/libs.versions.toml (1)
22-22:
Consider using a stable version of kotlinx.coroutines
The update to coroutines version 1.9.0 introduces potential stability risks as this is not a stable release. Since typing indicators likely rely heavily on coroutines, consider using the latest stable version (1.8.1-Beta) to ensure reliability.
Let's verify the compatibility and stability:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Coroutines usage appears safe for version 1.9.0 upgrade
The codebase uses standard coroutine APIs that are stable across versions:
- Basic coroutine builders (
launch) - Core utilities (
withContext,suspendCancellableCoroutine) - Standard flow APIs (
MutableSharedFlow) - Testing utilities (
runTest,runBlocking)
No usage of experimental or unstable APIs from 1.9.0 was found. The typing indicators implementation uses well-established coroutine patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check kotlinx.coroutines compatibility and stability
# Check if any source files use new APIs from 1.9.0
rg -l "import kotlinx.coroutines" | while read -r file; do
echo "=== Checking $file for coroutine usage ==="
rg "import kotlinx.coroutines|suspend fun|launch|async|flow" "$file"
done
# Check for potential breaking changes in the codebase
fd -e kt -e java | xargs rg "withContext|supervisorScope|coroutineScope|withTimeout"
Length of output: 8156
chat-android/src/main/java/com/ably/chat/Presence.kt (1)
Line range hint 142-153: Verify typing indicator implementation
The presence changes look good and align with the PR objectives. Let's verify the typing indicator implementation that uses this presence functionality.
✅ Verification successful
Let me gather more information about the typing indicator implementation.
Typing indicator implementation verified successfully
The implementation in Typing.kt correctly utilizes the simplified presence API. Key verifications:
- Uses
presence.enterClientCoroutine()andpresence.leaveClientCoroutine()for typing state management - Properly handles typing timeouts and state updates
- Includes retry logic for presence queries
- Has test coverage confirming the functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for typing indicator implementation that uses this presence API
rg -l "presence\.(enter|update|leave)" --type kotlin
# Look for typing-related presence data
ast-grep --pattern 'JsonObject().apply {
$$$
"typing"
$$$
}'
Length of output: 347
Script:
#!/bin/bash
# Check the Typing.kt implementation
cat chat-android/src/main/java/com/ably/chat/Typing.kt
# Look for typing-related tests
rg "typing" chat-android/src/test --type kotlin -C 2
Length of output: 9068
chat-android/src/main/java/com/ably/chat/Typing.kt (4)
6-19: LGTM: Well-structured imports and constants
The retry mechanism constants are well-documented and have reasonable values. The imports are organized logically and all are being utilized in the implementation.
Also applies to: 24-34
67-73: Reconsider removal of suspend modifier
The previous review comment about changing start() and stop() from suspend functions to regular functions remains valid. This change hides asynchronous behavior and may impact error handling capabilities of the callers.
194-211: Fix retry loop condition
The previous review comment about the off-by-one error in the retry loop remains valid. The condition numRetries <= PRESENCE_GET_MAX_RETRIES allows for one extra retry.
93-122: LGTM: Well-structured implementation with proper error handling
The implementation shows good practices:
- Comprehensive logging
- Proper coroutine scope management with
limitedParallelism - Well-structured state management with thread-safe collections
- Clear separation of concerns
example/src/main/java/com/ably/chat/example/MainActivity.kt (2)
295-310: LGTM! Well-implemented typing users tracking
The implementation is clean, properly manages subscriptions, and correctly filters out the current user from typing indicators.
98-107: Previous review comment about coroutines in onDispose is still valid
The issue about launching coroutines inside onDispose hasn't been addressed yet.
| suspend fun PubSubPresence.enterClientCoroutine(clientId: String, data: JsonElement? = JsonNull.INSTANCE) = | ||
| suspendCancellableCoroutine { continuation -> | ||
| enterClient( | ||
| clientId, | ||
| data, | ||
| object : CompletionListener { | ||
| override fun onSuccess() { | ||
| continuation.resume(Unit) | ||
| } | ||
|
|
||
| override fun onError(reason: ErrorInfo?) { | ||
| continuation.resumeWithException(AblyException.fromErrorInfo(reason)) | ||
| } | ||
| }, | ||
| ) | ||
| } | ||
| override fun onError(reason: ErrorInfo?) { | ||
| continuation.resumeWithException(AblyException.fromErrorInfo(reason)) | ||
| } | ||
| }, | ||
| ) | ||
| } | ||
|
|
||
| suspend fun PubSubPresence.updateClientCoroutine(clientId: String, data: JsonElement?) = suspendCancellableCoroutine { continuation -> | ||
| updateClient( | ||
| clientId, | ||
| data, | ||
| object : CompletionListener { | ||
| override fun onSuccess() { | ||
| continuation.resume(Unit) | ||
| } | ||
| suspend fun PubSubPresence.updateClientCoroutine(clientId: String, data: JsonElement? = JsonNull.INSTANCE) = | ||
| suspendCancellableCoroutine { continuation -> | ||
| updateClient( | ||
| clientId, | ||
| data, | ||
| object : CompletionListener { | ||
| override fun onSuccess() { | ||
| continuation.resume(Unit) | ||
| } | ||
|
|
||
| override fun onError(reason: ErrorInfo?) { | ||
| continuation.resumeWithException(AblyException.fromErrorInfo(reason)) | ||
| } | ||
| }, | ||
| ) | ||
| } | ||
| override fun onError(reason: ErrorInfo?) { | ||
| continuation.resumeWithException(AblyException.fromErrorInfo(reason)) | ||
| } | ||
| }, | ||
| ) | ||
| } | ||
|
|
||
| suspend fun PubSubPresence.leaveClientCoroutine(clientId: String, data: JsonElement?) = suspendCancellableCoroutine { continuation -> | ||
| leaveClient( | ||
| clientId, | ||
| data, | ||
| object : CompletionListener { | ||
| override fun onSuccess() { | ||
| continuation.resume(Unit) | ||
| } | ||
| suspend fun PubSubPresence.leaveClientCoroutine(clientId: String, data: JsonElement? = JsonNull.INSTANCE) = | ||
| suspendCancellableCoroutine { continuation -> | ||
| leaveClient( | ||
| clientId, | ||
| data, | ||
| object : CompletionListener { | ||
| override fun onSuccess() { | ||
| continuation.resume(Unit) | ||
| } | ||
|
|
||
| override fun onError(reason: ErrorInfo?) { | ||
| continuation.resumeWithException(AblyException.fromErrorInfo(reason)) | ||
| } | ||
| }, | ||
| ) | ||
| } | ||
| override fun onError(reason: ErrorInfo?) { | ||
| continuation.resumeWithException(AblyException.fromErrorInfo(reason)) | ||
| } | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in completion listeners.
The completion listener implementation is duplicated across enterClientCoroutine, updateClientCoroutine, and leaveClientCoroutine. Consider extracting this common pattern into a reusable function.
Extract the common completion listener logic:
+private fun createCompletionListener(continuation: CancellableContinuation<Unit>) =
+ object : CompletionListener {
+ override fun onSuccess() {
+ continuation.resume(Unit)
+ }
+
+ override fun onError(reason: ErrorInfo?) {
+ continuation.resumeWithException(AblyException.fromErrorInfo(reason))
+ }
+ }
suspend fun PubSubPresence.enterClientCoroutine(clientId: String, data: JsonElement? = JsonNull.INSTANCE) =
suspendCancellableCoroutine { continuation ->
- enterClient(
- clientId,
- data,
- object : CompletionListener {
- override fun onSuccess() {
- continuation.resume(Unit)
- }
-
- override fun onError(reason: ErrorInfo?) {
- continuation.resumeWithException(AblyException.fromErrorInfo(reason))
- }
- },
- )
+ enterClient(clientId, data, createCompletionListener(continuation))
}
suspend fun PubSubPresence.updateClientCoroutine(clientId: String, data: JsonElement? = JsonNull.INSTANCE) =
suspendCancellableCoroutine { continuation ->
- updateClient(
- clientId,
- data,
- object : CompletionListener {
- override fun onSuccess() {
- continuation.resume(Unit)
- }
-
- override fun onError(reason: ErrorInfo?) {
- continuation.resumeWithException(AblyException.fromErrorInfo(reason))
- }
- },
- )
+ updateClient(clientId, data, createCompletionListener(continuation))
}
suspend fun PubSubPresence.leaveClientCoroutine(clientId: String, data: JsonElement? = JsonNull.INSTANCE) =
suspendCancellableCoroutine { continuation ->
- leaveClient(
- clientId,
- data,
- object : CompletionListener {
- override fun onSuccess() {
- continuation.resume(Unit)
- }
-
- override fun onError(reason: ErrorInfo?) {
- continuation.resumeWithException(AblyException.fromErrorInfo(reason))
- }
- },
- )
+ leaveClient(clientId, data, createCompletionListener(continuation))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| suspend fun PubSubPresence.enterClientCoroutine(clientId: String, data: JsonElement? = JsonNull.INSTANCE) = | |
| suspendCancellableCoroutine { continuation -> | |
| enterClient( | |
| clientId, | |
| data, | |
| object : CompletionListener { | |
| override fun onSuccess() { | |
| continuation.resume(Unit) | |
| } | |
| override fun onError(reason: ErrorInfo?) { | |
| continuation.resumeWithException(AblyException.fromErrorInfo(reason)) | |
| } | |
| }, | |
| ) | |
| } | |
| override fun onError(reason: ErrorInfo?) { | |
| continuation.resumeWithException(AblyException.fromErrorInfo(reason)) | |
| } | |
| }, | |
| ) | |
| } | |
| suspend fun PubSubPresence.updateClientCoroutine(clientId: String, data: JsonElement?) = suspendCancellableCoroutine { continuation -> | |
| updateClient( | |
| clientId, | |
| data, | |
| object : CompletionListener { | |
| override fun onSuccess() { | |
| continuation.resume(Unit) | |
| } | |
| suspend fun PubSubPresence.updateClientCoroutine(clientId: String, data: JsonElement? = JsonNull.INSTANCE) = | |
| suspendCancellableCoroutine { continuation -> | |
| updateClient( | |
| clientId, | |
| data, | |
| object : CompletionListener { | |
| override fun onSuccess() { | |
| continuation.resume(Unit) | |
| } | |
| override fun onError(reason: ErrorInfo?) { | |
| continuation.resumeWithException(AblyException.fromErrorInfo(reason)) | |
| } | |
| }, | |
| ) | |
| } | |
| override fun onError(reason: ErrorInfo?) { | |
| continuation.resumeWithException(AblyException.fromErrorInfo(reason)) | |
| } | |
| }, | |
| ) | |
| } | |
| suspend fun PubSubPresence.leaveClientCoroutine(clientId: String, data: JsonElement?) = suspendCancellableCoroutine { continuation -> | |
| leaveClient( | |
| clientId, | |
| data, | |
| object : CompletionListener { | |
| override fun onSuccess() { | |
| continuation.resume(Unit) | |
| } | |
| suspend fun PubSubPresence.leaveClientCoroutine(clientId: String, data: JsonElement? = JsonNull.INSTANCE) = | |
| suspendCancellableCoroutine { continuation -> | |
| leaveClient( | |
| clientId, | |
| data, | |
| object : CompletionListener { | |
| override fun onSuccess() { | |
| continuation.resume(Unit) | |
| } | |
| override fun onError(reason: ErrorInfo?) { | |
| continuation.resumeWithException(AblyException.fromErrorInfo(reason)) | |
| } | |
| }, | |
| ) | |
| } | |
| override fun onError(reason: ErrorInfo?) { | |
| continuation.resumeWithException(AblyException.fromErrorInfo(reason)) | |
| } | |
| }, | |
| ) | |
| } | |
| private fun createCompletionListener(continuation: CancellableContinuation<Unit>) = | |
| object : CompletionListener { | |
| override fun onSuccess() { | |
| continuation.resume(Unit) | |
| } | |
| override fun onError(reason: ErrorInfo?) { | |
| continuation.resumeWithException(AblyException.fromErrorInfo(reason)) | |
| } | |
| } | |
| suspend fun PubSubPresence.enterClientCoroutine(clientId: String, data: JsonElement? = JsonNull.INSTANCE) = | |
| suspendCancellableCoroutine { continuation -> | |
| enterClient(clientId, data, createCompletionListener(continuation)) | |
| } | |
| suspend fun PubSubPresence.updateClientCoroutine(clientId: String, data: JsonElement? = JsonNull.INSTANCE) = | |
| suspendCancellableCoroutine { continuation -> | |
| updateClient(clientId, data, createCompletionListener(continuation)) | |
| } | |
| suspend fun PubSubPresence.leaveClientCoroutine(clientId: String, data: JsonElement? = JsonNull.INSTANCE) = | |
| suspendCancellableCoroutine { continuation -> | |
| leaveClient(clientId, data, createCompletionListener(continuation)) | |
| } |
There was a problem hiding this comment.
This seems like a good suggestion to avoid code duplication.
There was a problem hiding this comment.
@sacOO7, glad you find the suggestion helpful!
| onMessageChange = { | ||
| messageText = it | ||
| coroutineScope.launch { | ||
| room.typing.start() | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add debouncing and stop typing event
The current implementation could benefit from two improvements:
- Debounce typing notifications to prevent flooding
- Add stop typing event when input is cleared
onMessageChange = {
messageText = it
+ // Debounce typing notifications
coroutineScope.launch {
- room.typing.start()
+ if (it.text.isEmpty()) {
+ room.typing.stop()
+ } else {
+ withContext(Dispatchers.IO) {
+ delay(300) // Add debounce delay
+ room.typing.start()
+ }
+ }
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onMessageChange = { | |
| messageText = it | |
| coroutineScope.launch { | |
| room.typing.start() | |
| } | |
| }, | |
| onMessageChange = { | |
| messageText = it | |
| // Debounce typing notifications | |
| coroutineScope.launch { | |
| if (it.text.isEmpty()) { | |
| room.typing.stop() | |
| } else { | |
| withContext(Dispatchers.IO) { | |
| delay(300) // Add debounce delay | |
| room.typing.start() | |
| } | |
| } | |
| } | |
| }, |
ttypic
force-pushed
the
ECO-4946/typing-indicators
branch
from
2bb20af
to
3b7cf96
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
chat-android/src/main/java/com/ably/chat/Typing.kt (3)
102-105: Add documentation for the event bus configurationConsider adding documentation explaining the purpose of the event bus and its configuration choices (buffer capacity and overflow strategy).
177-190: Consider adding timeout validationThe startTypingTimer implementation has good error handling for uninitialized options, but consider adding validation for the timeout value to ensure it's positive and reasonable.
private fun startTypingTimer() { val timeout = options?.timeoutMs ?: throw AblyException.fromErrorInfo( ErrorInfo( "Typing options hasn't been initialized", ErrorCodes.BadRequest, ), ) + require(timeout > 0) { "Timeout must be positive" } + require(timeout <= 60_000) { "Timeout must not exceed 60 seconds" } logger.trace("DefaultTyping.startTypingTimer()") typingJob = typingScope.launch {
Line range hint
1-218: Consider documenting the real-time synchronization strategyThe implementation provides a robust real-time typing indicator feature using Ably's presence functionality. Consider adding documentation that explains:
- The synchronization strategy using presence events
- The retry and timeout mechanisms
- Potential edge cases (e.g., network disconnections, multiple devices)
This would help future maintainers understand the design decisions and operational characteristics of the feature.
example/src/main/java/com/ably/chat/example/MainActivity.kt (1)
127-135: Add accessibility support for typing indicatorsThe typing indicator text should be more accessible. Consider adding a contentDescription for screen readers.
if (currentlyTyping.isNotEmpty()) { Text( modifier = Modifier.padding(start = 16.dp), text = "Currently typing: ${currentlyTyping.joinToString(", ")}", + // Add semantic description for accessibility + modifier = Modifier + .padding(start = 16.dp) + .semantics { + contentDescription = "Users currently typing: ${currentlyTyping.joinToString(", ")}" + }, style = MaterialTheme.typography.bodySmall.copy( color = Color.Gray, ), ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
chat-android/src/main/java/com/ably/chat/Typing.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Utils.kt(2 hunks)chat-android/src/test/java/com/ably/chat/SandboxTest.kt(2 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- chat-android/src/test/java/com/ably/chat/SandboxTest.kt
🔇 Additional comments (9)
chat-android/src/main/java/com/ably/chat/Utils.kt (3)
66-76: Well-structured implementation with good practices!
The function effectively uses:
- Kotlin's DSL features with buildList
- Proper coroutine context for IO operations
- Sensible default parameters
- Null-safe parameter handling
79-128: Good use of suspendCancellableCoroutine and default parameters!
The implementation correctly:
- Uses suspendCancellableCoroutine for proper cancellation support
- Provides JsonNull.INSTANCE as a sensible default for the data parameter
The previous review comment about reducing code duplication in completion listeners is still valid. Consider implementing the suggested refactoring to improve maintainability.
66-128: Verify presence state handling for typing indicators
Let's verify how these presence functions are used in the typing indicator implementation.
chat-android/src/main/java/com/ably/chat/Typing.kt (3)
23-28: LGTM: Well-defined retry constants with clear documentation
The constants for retry logic are well-structured with appropriate values and clear documentation. The exponential backoff strategy with defined maximum intervals is a good practice for handling transient failures.
212-218: LGTM: Efficient duplicate event handling
The emit method efficiently prevents duplicate events by comparing with the last state before notifying listeners. The implementation is thread-safe using CopyOnWriteArrayList.
115-129: Consider handling potential race conditions in initialization
The initialization block sets up concurrent operations. Consider the following improvements:
- Document the rationale for using
tryEmitinstead ofemit - Add error handling for the presence subscription
Let's verify the presence subscription error handling:
✅ Verification successful
Let me gather more information about the tryEmit usage and error handling patterns in the codebase.
Let me check the implementation of the error handling in the Presence class and the event bus usage.
Let me try a different approach to check the implementations.
Based on the gathered information, I can now provide the final response:
The concurrent initialization is properly handled with appropriate error handling and buffering
The implementation shows:
tryEmitusage is justified as it's configured withBufferOverflow.DROP_OLDESTstrategy, making it a suitable choice for typing indicators where dropping old events is acceptable- Presence subscription has error handling for null clientId case with proper error logging
- The
typingScopeuseslimitedParallelism(1)to prevent concurrent execution issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for presence subscription error handling patterns in the codebase
rg -A 5 "presence.subscribe"
Length of output: 3890
Script:
#!/bin/bash
# Search for tryEmit usage patterns
rg -A 3 "tryEmit"
# Search for presence error handling patterns
rg -B 2 -A 3 "presence.*error|error.*presence" --ignore-case
Length of output: 1358
Script:
#!/bin/bash
# Check Presence class implementation for error handling
ast-grep --pattern 'class Presence {
$$$
}'
# Check eventBus implementation in Typing class
ast-grep --pattern 'class Typing {
$$$
}'
Length of output: 93
Script:
#!/bin/bash
# Check Presence class implementation
rg -A 20 "^class Presence" chat-android/src/main/java/com/ably/chat/Presence.kt
# Check Typing class implementation focusing on eventBus
rg -B 5 -A 10 "private val eventBus" chat-android/src/main/java/com/ably/chat/Typing.kt
Length of output: 743
example/src/main/java/com/ably/chat/example/MainActivity.kt (3)
98-107: Previous review comment is still applicable
The issue with launching coroutines inside onDispose remains. Please refer to the previous review comment about avoiding coroutine launches in onDispose to prevent potential lifecycle issues.
205-210: Previous review comment about debouncing is still applicable
The suggestion to add debouncing and stop typing event remains valid to prevent flooding the server with typing events.
91-94: Implementation successfully meets PR objectives
The typing indicators feature has been implemented successfully with all necessary components:
- Room configuration with typing options
- UI components for displaying typing status
- Event handling for typing notifications
- Proper subscription management
The suggested improvements will help make the implementation more robust, but the core functionality is working as expected.
Also applies to: 295-310
| @Composable | ||
| fun typingUsers(typing: Typing): State<Set<String>> { | ||
| val currentlyTyping = remember { mutableStateOf(emptySet<String>()) } | ||
|
|
||
| DisposableEffect(typing) { | ||
| val subscription = typing.subscribe { typingEvent -> | ||
| currentlyTyping.value = typingEvent.currentlyTyping - randomClientId | ||
| } | ||
|
|
||
| onDispose { | ||
| subscription.unsubscribe() | ||
| } | ||
| } | ||
|
|
||
| return currentlyTyping | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for typing subscription
The typing subscription should handle potential errors gracefully to prevent app crashes and provide feedback to users when needed.
@Composable
fun typingUsers(typing: Typing): State<Set<String>> {
val currentlyTyping = remember { mutableStateOf(emptySet<String>()) }
+ val scope = rememberCoroutineScope()
DisposableEffect(typing) {
- val subscription = typing.subscribe { typingEvent ->
- currentlyTyping.value = typingEvent.currentlyTyping - randomClientId
- }
+ val subscription = typing.subscribe(
+ onNext = { typingEvent ->
+ currentlyTyping.value = typingEvent.currentlyTyping - randomClientId
+ },
+ onError = { error ->
+ scope.launch {
+ // Handle error appropriately (e.g., show snackbar, retry logic)
+ Log.e("TypingIndicator", "Error in typing subscription", error)
+ }
+ }
+ )
onDispose {
subscription.unsubscribe()
}
}
return currentlyTyping
}Committable suggestion skipped: line range outside the PR's diff.
|
Overall, the implementation looks super good 👍 . I will review once again, if needed. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
chat-android/src/main/java/com/ably/chat/Typing.kt (1)
116-130: Consider adding error metrics for missing clientId eventsWhile the error logging is good, consider tracking these errors with metrics to monitor the frequency of missing clientId events in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
chat-android/src/main/java/com/ably/chat/Typing.kt(2 hunks)
🔇 Additional comments (5)
chat-android/src/main/java/com/ably/chat/Typing.kt (5)
Line range hint 24-34: LGTM: Well-defined retry mechanism constants
The retry mechanism constants are well-documented and follow good practices with reasonable values for retry intervals and maximum retries.
93-115: LGTM: Well-structured constructor and properties
The implementation shows good practices:
- Proper dependency injection through constructor parameters
- Well-configured coroutine scope with SupervisorJob for error isolation
- Efficient event bus implementation using MutableSharedFlow
146-167: LGTM: Proper implementation of start/stop methods
The implementation correctly addresses the previous review comments:
- Maintains suspension semantics with .join()
- Properly handles coroutine cancellation
- Includes appropriate logging
213-219: LGTM: Efficient event emission implementation
The emit method shows good practices:
- Prevents duplicate notifications
- Efficiently handles listener notifications
- Thread-safe listener iteration using CopyOnWriteArrayList
193-211:
Fix off-by-one error in retry loop condition
The current condition numRetries <= PRESENCE_GET_MAX_RETRIES allows for one extra retry attempt. This was previously identified in the review comments but hasn't been addressed.
Apply this fix:
- while (numRetries <= PRESENCE_GET_MAX_RETRIES) {
+ while (numRetries < PRESENCE_GET_MAX_RETRIES) {Likely invalid or redundant comment.
ttypic
force-pushed
the
ECO-4946/typing-indicators
branch
from
a7c1bb5
to
76e3d5d
Compare
Added typing indicators feature
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation