[ECO-4944] feat: add room level reaction implementation #36
Conversation
WalkthroughThe changes in this pull request involve significant modifications to the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
chat-android/src/main/java/com/ably/chat/Utils.kt (1)
38-51: LGTM! Well-structured coroutine implementation.The implementation follows good practices:
- Consistent with existing coroutine patterns in the codebase
- Proper error handling and propagation
- Clean integration with Ably's completion-based API
Consider adding KDoc documentation to match the style of other utility functions:
+/** + * Publishes a message to the channel using coroutines. + * + * @param message The message to publish + * @throws AblyException if the publish operation fails + */ suspend fun Channel.publishCoroutine(message: PubSubMessage) = suspendCoroutine { continuation ->chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt (3)
41-56: Enhance channel name test coverageWhile the current test verifies the basic channel name format, consider adding test cases for:
- Edge cases: empty roomId, special characters in roomId
- Negative cases: null roomId
Example additional test:
@Test fun `channel name handles special characters in roomId`() = runTest { val roomReactions = DefaultRoomReactions( roomId = "foo:bar", clientId = "client1", realtimeChannels = realtimeChannels, ) assertEquals( "foo:bar::$chat::$reactions", roomReactions.channel.name, ) }
58-108: Enhance subscription test coverageThe subscription test is thorough but could be improved in several areas:
- Verify subscription cleanup/unsubscribe
- Test non-empty metadata mapping
- Add explicit test for isSelf=true case
Consider adding these test cases:
@Test fun `should handle subscription cleanup`() = runTest { val subscription = roomReactions.subscribe { } subscription.unsubscribe() verify { realtimeChannel.unsubscribe("roomReaction", any()) } } @Test fun `should map self reactions correctly`() = runTest { // Test with clientId matching the room's clientId // to verify isSelf is true }Also, consider testing the metadata mapping with non-empty data:
data = JsonObject().apply { addProperty("type", "like") + add("metadata", JsonObject().apply { + addProperty("count", 1) + }) }
18-18: Add class-level documentationConsider adding KDoc documentation to describe the test class purpose and its relationship to the specifications being tested (CHA-ER1, CHA-ER3a).
Example:
/** * Tests for [DefaultRoomReactions] implementation verifying: * - Channel name formatting (CHA-ER1) * - Reaction subscription handling (CHA-ER3a) */ class RoomReactionsTest {chat-android/src/main/java/com/ably/chat/Room.kt (1)
Line range hint
133-143: Consider adding error handling to attach/detach methods.While the implementation is correct, consider adding error handling to gracefully handle failures during attach/detach operations. This is particularly important for maintaining room state consistency.
Consider wrapping the operations in a try-catch block:
override suspend fun attach() { + try { messages.channel.attachCoroutine() typing.channel.attachCoroutine() reactions.channel.attachCoroutine() + } catch (e: Exception) { + // Handle or propagate the error appropriately + throw RoomException("Failed to attach to room channels", e) + } } override suspend fun detach() { + try { messages.channel.detachCoroutine() typing.channel.detachCoroutine() reactions.channel.detachCoroutine() + } catch (e: Exception) { + // Handle or propagate the error appropriately + throw RoomException("Failed to detach from room channels", e) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- chat-android/src/main/java/com/ably/chat/Room.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/RoomReactions.kt (2 hunks)
- chat-android/src/main/java/com/ably/chat/Utils.kt (1 hunks)
- chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt (1 hunks)
🔇 Additional comments (2)
chat-android/src/main/java/com/ably/chat/Room.kt (1)
108-109: Improved dependency injection pattern!The change from passing
realtimeClientto passing specific components (clientIdandrealtimeChannels) is a good architectural improvement that:
- Reduces coupling by only exposing required dependencies
- Makes the code more testable
- Follows interface segregation principle
Let's verify the consistency of this pattern across the codebase:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Verified: Consistent dependency injection pattern across codebase
The new pattern of passing specific components (
clientIdandrealtimeChannels) instead of the entirerealtimeClientis consistently implemented:
- All
DefaultRoomReactionsinstantiations in both production and test code follow this pattern- The only other usage of
realtimeClient.channelsinTyping.ktis for a different component and doesn't affect this change🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if realtimeClient is still directly passed to DefaultRoomReactions # or if other components are using the new pattern consistently # Search for DefaultRoomReactions instantiation ast-grep --pattern 'DefaultRoomReactions($$$)' # Search for other usages of realtimeClient.auth.clientId and realtimeClient.channels rg 'realtimeClient\.(auth\.clientId|channels)' -A 2Length of output: 2416
chat-android/src/main/java/com/ably/chat/RoomReactions.kt (1)
108-109:⚠️ Potential issueUpdate
DefaultRoomReactionsconstructor usage throughout the codebaseThe constructor of
DefaultRoomReactionshas changed to acceptclientIdandrealtimeChannelsinstead ofrealtimeClient. Ensure that all instantiations ofDefaultRoomReactionsare updated to match the new constructor signature to prevent any instantiation errors.Run the following script to find all usages of
DefaultRoomReactions:
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
chat-android/src/main/java/com/ably/chat/RoomReactions.kt (2)
107-114: LGTM! Consider documenting the channel name format.The constructor changes improve dependency injection by accepting only the required dependencies. However, the channel name format (
$roomId::$chat::$reactions) could benefit from documentation explaining its structure and any constraints.Consider adding a comment explaining:
- The purpose of each segment in the channel name
- Any restrictions on the
roomIdformat- Reference to the specification that defines this format
Line range hint
157-159: Implement onDiscontinuity to handle connection state changes.The
onDiscontinuitymethod is crucial for handling connection state changes and ensuring reliable reaction delivery.Would you like me to help implement this method? I can provide a solution that:
- Monitors channel state changes
- Notifies listeners of connectivity issues
- Handles reconnection scenarios
example/src/main/java/com/ably/chat/example/MainActivity.kt (2)
88-97: Add error handling for room operationsWhile the room attachment logic is correctly implemented, consider adding error handling for the attach/detach operations to gracefully handle potential failures.
DisposableEffect(Unit) { coroutineScope.launch { - room.attach() + try { + room.attach() + } catch (e: Exception) { + // Handle attachment failure + Log.e("MainActivity", "Failed to attach to room", e) + } } onDispose { coroutineScope.launch { - room.detach() + try { + room.detach() + } catch (e: Exception) { + Log.e("MainActivity", "Failed to detach from room", e) + } } } }
163-164: Enhance reaction display accessibilityThe current text display of reactions should include content description for screen readers and consider using proper emoji semantic markup.
-Text("Received reactions: ${receivedReactions.joinToString()}", modifier = Modifier.padding(16.dp)) +Text( + text = "Received reactions: ${receivedReactions.joinToString()}", + modifier = Modifier + .padding(16.dp) + .semantics { + contentDescription = "Received reactions: ${receivedReactions.size} reactions" + } +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- chat-android/src/main/java/com/ably/chat/RoomReactions.kt (2 hunks)
- example/build.gradle.kts (1 hunks)
- example/src/main/java/com/ably/chat/example/MainActivity.kt (7 hunks)
- gradle/libs.versions.toml (2 hunks)
🔇 Additional comments (4)
example/build.gradle.kts (1)
70-70: LGTM! Verify version catalog entry.The addition of the konfetti-compose library aligns well with implementing visual feedback for room-level reactions.
Let's verify the version catalog entry exists:
✅ Verification successful
✓ Version catalog entry for konfetti-compose is properly configured
The version catalog entry for konfetti-compose is correctly set up with both the version declaration (
2.0.4) and the library declaration referencing the version.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify konfetti-compose entry in version catalog # Expected: Entry for konfetti-compose version and library declaration # Test: Search for konfetti-compose in version catalog rg "konfetti-compose" gradle/libs.versions.tomlLength of output: 173
chat-android/src/main/java/com/ably/chat/RoomReactions.kt (1)
154-155: LGTM! Proper subscription cleanup implemented.The implementation correctly handles subscription cleanup by returning a Subscription object that unsubscribes the listener when disposed.
example/src/main/java/com/ably/chat/example/MainActivity.kt (2)
40-40: LGTM: Proper setup for reactions functionalityThe new import and state management for reactions are correctly implemented using Compose state management best practices.
Also applies to: 83-83
Line range hint
83-164: Verify reaction implementation completenessWhile the basic room-level reaction functionality is implemented, please verify:
- Reaction persistence across app restarts
- Proper synchronization across different clients
- Rate limiting for reaction sending
- Handling of network disconnections
ttypic
force-pushed
the
ECO-4944/room-level-reactions
branch
from
f2da984
to
7d14c1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
chat-android/src/main/java/com/ably/chat/RoomReactions.kt (2)
111-112: Improve documentation for the channel name format.The channel name format
$roomId::$chat::$reactionsappears to be an implementation detail referenced by "(CHA-ER1)". Consider adding a documentation comment explaining the format's significance and any constraints on the roomId parameter.
Line range hint
157-159: Implement onDiscontinuity method.The
onDiscontinuitymethod is currently unimplemented. This method is required by theEmitsDiscontinuitiesinterface and should handle connection state changes or other discontinuities in the reaction stream.Would you like me to help implement this method or create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- chat-android/src/main/java/com/ably/chat/RoomReactions.kt (2 hunks)
- example/build.gradle.kts (1 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 (2)
- example/build.gradle.kts
- gradle/libs.versions.toml
🔇 Additional comments (4)
chat-android/src/main/java/com/ably/chat/RoomReactions.kt (1)
150-150: Ensure thread-safe access to clientId.The comparison
pubSubMessage.clientId == clientIdassumes thatclientIdwon't change during the lifetime of the subscription. Consider makingclientIdimmutable by declaring it asprivate valto prevent potential race conditions.example/src/main/java/com/ably/chat/example/MainActivity.kt (3)
40-40: LGTM: Import added for reaction functionalityThe import is correctly placed and necessary for the new reaction feature.
118-126: Reference existing review commentThe previous review comment about enhancing reaction handling robustness is still applicable to this code segment.
158-162: Reference existing review commentThe previous review comment about adding error handling and loading state for reaction sending is still applicable to this code segment.
|
@ttypic The overall PR looks promising. While we have some unit tests to verify room reaction behavior, there's still some ambiguity regarding their end-to-end functionality. Referring to the ably test pyramid guidelines, we're missing the crucial middle layer of integration tests. With the upcoming beta release and the limited time available, integrating chat-UTS into the Kotlin ecosystem seems unlikely at this point. In the meantime, we should ensure a way to test the chat feature using the Ably sandbox. As discussed and agreed earlier, we should start incorporating integration tests within this PR itself. Setting up the sandbox should only take 2-3 hours max, but will provide significant value, enhancing our confidence in both the short and long term. The goal is to have a feature-specific integration test that assures us the feature will work in a production environment. While we will eventually integrate UTS, having an in-house testing framework as a backup is always better. You might like to assign this issue to yourself -> #26, wdyt |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Room level reactions implementation
Summary by CodeRabbit
New Features
RoomReactionsfunctionality to allow sending and subscribing to reactions in real-time.Channelclass.Bug Fixes
subscribemethod for incoming messages.Tests
DefaultRoomReactionsclass to verify channel naming and subscription behavior.Chores
konfettilibrary for use with Jetpack Compose.konfetti-composeto version2.0.4.