[ECO-5082] feat: presence basic implementation #42
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several enhancements to the chat application, primarily focusing on the presence management system. Key changes include the addition of new dependencies in the Gradle build file, modifications to the Changes
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: 7
🧹 Outside diff range and nitpick comments (8)
chat-android/src/main/java/com/ably/chat/Room.kt (1)
Line range hint
131-133: Track the unimplemented status property.The
statusproperty is currently marked with TODO, which aligns with the PR objectives stating that room lifecycle state is intentionally omitted. However, we should track this for future implementation.Would you like me to create a GitHub issue to track the implementation of the room lifecycle state and status property?
example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt (2)
30-32: Consider breaking down the Composable into smaller components.Instead of suppressing the "LongMethod" warning, consider extracting the member list and action buttons into separate Composable functions for better maintainability and reusability.
+@Composable +private fun MembersList(members: List<PresenceMember>) { + members.forEach { member -> + BasicText("${member.clientId} - (${(member.data as? JsonObject)?.get("status")?.asString})") + Spacer(modifier = Modifier.height(4.dp)) + } +} +@Composable +private fun PresenceActions(presence: Presence, onDismiss: () -> Unit) { + val coroutineScope = rememberCoroutineScope() + Button(onClick = { + coroutineScope.launch { + presence.enter(JsonObject().apply { + addProperty("status", "online") + }) + } + }) { + Text("Join") + } + // ... other buttons +}
1-109: Consider implementing a PresenceState sealed class for better state management.While this implementation provides basic presence functionality, it could benefit from a more robust state management approach. This would help handle the various states (loading, error, success) more effectively and prepare for the future addition of room lifecycle state.
Consider implementing a state class:
sealed class PresenceState { object Loading : PresenceState() data class Error(val message: String) : PresenceState() data class Success( val members: List<PresenceMember>, val isUpdating: Boolean = false ) : PresenceState() }This would provide a foundation for handling room lifecycle states when they are implemented in the future.
chat-android/src/main/java/com/ably/chat/Utils.kt (1)
58-60: Add documentation for presence get operations.Consider adding KDoc documentation explaining:
- The purpose of these functions
- The expected format of
Param- The return type and potential exceptions
Also, consider adding a timeout parameter to prevent indefinite waiting on IO operations:
suspend fun PubSubPresence.getCoroutine( + timeout: Duration = Duration.seconds(30), param: Param ) = withContext(Dispatchers.IO) { + withTimeout(timeout) { get(param) + } }Also applies to: 62-65
chat-android/src/test/java/com/ably/chat/PresenceTest.kt (2)
19-31: Consider extracting the channel name and reviewing mockk configuration.A few suggestions to improve the test setup:
- Consider extracting the channel name pattern "room1::$chat::$messages" into a constant for better maintainability
- The
relaxed=trueon the PubSubPresence mock might mask missing method implementations. Consider using strict mocking unless there's a specific reason not to.+ private companion object { + const val TEST_CHANNEL = "room1::\$chat::\$messages" + } - private val pubSubChannel = spyk<Channel>(buildRealtimeChannel("room1::\$chat::\$messages")) + private val pubSubChannel = spyk<Channel>(buildRealtimeChannel(TEST_CHANNEL))
37-67: Consider reducing test duplication through helper methods.The test cases contain significant duplicate code for setup and verification. Consider extracting common patterns into helper methods:
private fun setupPresenceTest(): Pair<PresenceListener, DeferredValue<PresenceEvent>> { val presenceListenerSlot = slot<PresenceListener>() every { pubSubPresence.subscribe(capture(presenceListenerSlot)) } returns Unit val deferredValue = DeferredValue<PresenceEvent>() presence.subscribe { deferredValue.completeWith(it) } return presenceListenerSlot.captured to deferredValue }This would make the tests more concise and maintainable while keeping the test intentions clear.
Also applies to: 73-104, 110-143
chat-android/src/main/java/com/ably/chat/Presence.kt (1)
191-195: Consider always returning aJsonObjectinwrapInUserCustomDataCurrently,
wrapInUserCustomData(data)returnsnullifdataisnull. If the presence methods expect a non-null payload, returning an emptyJsonObjectmight be safer to prevent potentialNullPointerException.You could modify the function to always return a
JsonObject:-private fun wrapInUserCustomData(data: PresenceData?) = data?.let { - JsonObject().apply { - add("userCustomData", data) - } -} +private fun wrapInUserCustomData(data: PresenceData?): JsonObject { + return JsonObject().apply { + data?.let { + add("userCustomData", data) + } + } +}This ensures that a
JsonObjectis always returned, withuserCustomDataadded only whendatais notnull.example/src/main/java/com/ably/chat/example/MainActivity.kt (1)
78-104: Modularize theAppcomposable by extracting theTopAppBarandChatcomponents.The
Appcomposable is responsible for multiple concerns, such as the top app bar, the chat screen, and the presence popup. Consider breaking it down into smaller, reusable components to improve readability and maintainability.@Composable fun App(chatClient: ChatClient) { var showPopup by remember { mutableStateOf(false) } Scaffold( modifier = Modifier.fillMaxSize(), topBar = { ChatTopBar(onMembersClick = { showPopup = true }) }, ) { innerPadding -> Chat( chatClient, modifier = Modifier.padding(innerPadding), ) if (showPopup) { PresencePopup(chatClient, onDismiss = { showPopup = false }) } } } @Composable fun ChatTopBar(onMembersClick: () -> Unit) { TopAppBar( title = { Text("Chat") }, actions = { IconButton(onClick = onMembersClick) { Icon(Icons.Default.Person, contentDescription = "Show members") } }, ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
chat-android/build.gradle.kts(1 hunks)chat-android/src/main/java/com/ably/chat/Presence.kt(5 hunks)chat-android/src/main/java/com/ably/chat/Room.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Utils.kt(2 hunks)chat-android/src/test/java/com/ably/chat/PresenceTest.kt(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(5 hunks)example/src/main/java/com/ably/chat/example/Settings.kt(1 hunks)example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- example/src/main/java/com/ably/chat/example/Settings.kt
🔇 Additional comments (19)
chat-android/build.gradle.kts (1)
48-48: LGTM! Verify version catalog entry.
The addition of the Coroutine Core dependency is appropriate for implementing the async presence features. The dependency is correctly managed through the version catalog.
Let's verify the version catalog entry:
✅ Verification successful
Version catalog entry is properly defined and versioned
The coroutine.core dependency is correctly defined in the version catalog with:
- Version declaration:
coroutine = "1.8.1" - Library declaration:
coroutine-core = { group = "org.jetbrains.kotlinx", name = "kotlinx-coroutines-core", version.ref = "coroutine" }
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if coroutine.core is properly defined in the version catalog
# Expected: Find the library declaration in the version catalog (typically gradle/libs.versions.toml)
# Look for the version catalog file
fd --type f "libs.versions.toml" --exec cat {} \; 2>/dev/null
# Alternatively, check if it's defined in settings.gradle.kts
fd --type f "settings.gradle.kts" --exec cat {} \; 2>/dev/null
Length of output: 3469
chat-android/src/main/java/com/ably/chat/Room.kt (3)
94-95: LGTM! Good refactoring.
The addition of the clientId property centralizes the access to realtimeClient.auth.clientId, improving code maintainability and following the DRY principle.
112-112: LGTM! Consistent with clientId refactoring.
The change to use the centralized clientId property maintains consistency with the rest of the implementation.
105-107: LGTM! Verify presence initialization in integration tests.
The presence initialization changes improve clarity and type safety. However, since this is part of the basic presence implementation, we should verify the integration.
✅ Verification successful
Let me search for test files and presence-related test cases with a different approach.
Presence initialization is well-tested and correctly implemented
The codebase has comprehensive test coverage for presence functionality in PresenceTest.kt, including:
- Proper initialization of presence with channel and clientId
- Transformation of presence messages
- Various presence event scenarios with different data types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for presence-related tests to ensure coverage
rg -t kotlin "test.*presence" -i
Length of output: 200
Script:
#!/bin/bash
# Search for test files
fd "Test.*\.kt$"
# Search for presence-related code in test files
rg -t kotlin "presence" --glob "*Test*.kt" -B2 -A2
Length of output: 8603
chat-android/src/main/java/com/ably/chat/Utils.kt (2)
3-3: LGTM: Imports are well-organized and necessary.
The new imports support the coroutine-based presence operations and follow Kotlin conventions.
Also applies to: 9-9, 13-15
58-77: Verify error propagation in presence operations.
Let's ensure that errors from the underlying Ably SDK are properly propagated through these coroutine wrappers.
✅ Verification successful
Based on the gathered information, I can now provide a final response:
Coroutine wrappers properly handle error propagation
The coroutine wrappers are implemented using withContext(Dispatchers.IO) which preserves the exception propagation chain. Any exceptions thrown by the underlying Ably SDK methods (enterClient, updateClient, leaveClient, get) will be automatically propagated through the coroutine context without being swallowed or transformed. This is the correct approach for suspending function wrappers in Kotlin.
- The implementation in
Utils.ktfollows Kotlin's structured concurrency principles where exceptions are propagated up the call chain - The higher-level
Presenceclass uses these coroutine wrappers directly without additional try-catch blocks, allowing exceptions to flow to the caller - The IO dispatcher ensures proper threading without affecting error handling semantics
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in Ably SDK presence operations
rg -A 5 "class.*Presence.*Exception|throws.*Exception.*presence|catch.*presence"
Length of output: 82
Script:
#!/bin/bash
# Let's search for error handling in the presence-related coroutine functions
rg -A 5 "enterClient|updateClient|leaveClient|get.*presence" --type kotlin
# Also search for try-catch blocks around these presence operations
ast-grep --pattern 'try {
$$$
enterClient($$$)
$$$
} catch ($$$) {
$$$
}'
ast-grep --pattern 'try {
$$$
updateClient($$$)
$$$
} catch ($$$) {
$$$
}'
ast-grep --pattern 'try {
$$$
leaveClient($$$)
$$$
} catch ($$$) {
$$$
}'
# Let's also check how these coroutine wrappers are used
rg -A 5 "enterClientCoroutine|updateClientCoroutine|leaveClientCoroutine|getCoroutine.*presence"
Length of output: 6957
Script:
#!/bin/bash
# Let's check the PubSubPresence interface or class to understand error handling
rg -A 10 "interface PubSubPresence|class PubSubPresence"
# Let's also check for any exception handling in the implementation
rg -A 10 "implements PubSubPresence"
# Check for any error handling in the higher level Presence class
rg -A 10 "class Presence.*\{" --type kotlin
Length of output: 2009
chat-android/src/test/java/com/ably/chat/PresenceTest.kt (1)
1-18: LGTM! Well-structured imports.
The file has a clean organization of imports, properly separating testing frameworks (JUnit, MockK), Ably SDK components, and coroutines testing utilities.
chat-android/src/main/java/com/ably/chat/Presence.kt (10)
5-6: Imports for JSON handling are appropriate
The addition of JsonElement and JsonObject imports is necessary for handling presence data as JSON. This enhances type specificity and data manipulation capabilities.
14-14: Enhanced type safety with PresenceData alias
Creating a type alias PresenceData as JsonElement improves type safety over using a generic Any. This makes the code more readable and less error-prone.
34-34: Default parameters improve usability of get method
Providing a default empty list for params in suspend fun get(params: List<Param> = listOf()) allows callers to omit parameters when defaults suffice, improving the method's usability.
48-62: Optional data parameters in presence methods enhance flexibility
Allowing data to be nullable with default null in enter, update, and leave methods gives flexibility to the API users. Ensure that the implementation correctly handles null values to avoid unexpected behavior.
94-94: Nullable data in PresenceMember ensures robustness
Making the data property nullable in the PresenceMember data class accommodates cases where presence data might not be provided, enhancing the robustness of the member representation.
129-134: Nullable data and accurate timestamp in PresenceEvent
Updating the timestamp to Long improves accuracy for time representation. Making data nullable in PresenceEvent aligns with the changes in PresenceMember and ensures consistent handling of optional data.
149-149: Ensure correct handling of data extraction
When extracting data with (user.data as? JsonObject)?.get("userCustomData"), there is a possibility that user.data is null or not a JsonObject. Make sure that this scenario is handled gracefully to prevent potential issues in data processing.
Consider adding checks or logging to handle unexpected data formats. If necessary, adjust the data extraction logic to account for different user.data types.
170-178: Proper conversion in presence event subscription
In the subscribe method, presence messages are converted into PresenceEvent objects. Ensure that casting (it.data as? JsonObject)?.get("userCustomData") handles all possible data formats correctly and that any null values are appropriately managed.
Confirm that the listener implementation can handle PresenceEvent instances where data might be null.
158-166: Confirm null data handling in Ably SDK methods
The enter, update, and leave methods call Ably SDK methods with wrapInUserCustomData(data) as the payload. Since wrapInUserCustomData(data) can return null, ensure that the Ably SDK methods enterClientCoroutine, updateClientCoroutine, and leaveClientCoroutine accept null data without causing errors.
You might want to check the Ably SDK documentation or source code to confirm that passing null for data is acceptable. Here's a script that could help:
#!/bin/bash
# Description: Check if Ably SDK methods accept `null` data.
# Search for the method definitions in the SDK.
rg 'fun (enterClientCoroutine|updateClientCoroutine|leaveClientCoroutine)' -t kotlin -A 5143-152: Safely casting user.data to JsonObject
In the get method, user.data is cast using (user.data as? JsonObject)?.get("userCustomData"). If user.data is not a JsonObject, this will safely return null. Confirm that this behavior is acceptable and that any downstream code can handle null values appropriately.
Please ensure that all possible types of user.data are considered and handled. You can run the following script to verify the types of user.data in presence messages:
✅ Verification successful
Let me analyze the presence data handling patterns in the codebase to verify the safety of the JsonObject casting.
This will help us understand how presence data is structured and handled throughout the codebase.
Safe handling of presence data confirmed
The null-safe casting of user.data to JsonObject is correct and well-handled because:
PresenceDatais defined astypealias PresenceData = JsonElement- The
wrapInUserCustomDatafunction ensures data is always wrapped in a JsonObject with "userCustomData" key - The null-safe cast and property access
(user.data as? JsonObject)?.get("userCustomData")properly handles both:- Cases where data is null (returns null)
- Cases where data contains the expected JsonObject structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the types of `user.data` in presence messages.
# Search for occurrences of `user.data` usage.
rg 'user\.data' -t kotlin -A 3
Length of output: 91
Script:
#!/bin/bash
# Let's try a broader search to understand the data handling in presence messages
# Search for presence-related data handling
rg "PresenceMember|presence.*data" -t kotlin -A 3
# Also search for any JsonObject usage
rg "JsonObject.*data" -t kotlin -A 3
# Look for presence message handling patterns
ast-grep --pattern 'data = $$$'
Length of output: 5972
Script:
#!/bin/bash
# Let's examine how presence data is wrapped and handled
# Look for wrapInUserCustomData implementation
rg "wrapInUserCustomData" -t kotlin -A 5
# Look for presence data type definitions
rg "typealias.*PresenceData|type.*PresenceData" -t kotlin
# Check presence-related tests for data handling patterns
rg "class.*PresenceTest" -t kotlin -B 10 -A 20
Length of output: 4672
example/src/main/java/com/ably/chat/example/MainActivity.kt (2)
162-164: The code change looks good!
The LazyColumn modifier has been formatted for better readability without altering the functionality.
116-116: Verify that the Settings.ROOM_ID constant is defined and used consistently across the codebase.
The code now uses a constant Settings.ROOM_ID to retrieve the room. Ensure that this constant is defined in the Settings.kt file and that all occurrences of the room ID in the codebase have been replaced with this constant.
✅ Verification successful
Let me search for more specific information about the Settings class and ROOM_ID usage.
Let me search specifically for any Settings.kt file and its contents.
Settings.ROOM_ID constant is properly defined and consistently used
The constant is correctly defined in example/src/main/java/com/ably/chat/example/Settings.kt as "my-room" and is consistently used in both locations where room ID is needed:
MainActivity.kt: For getting the room instancePresencePopup.kt: For accessing room presence
No hardcoded room IDs were found elsewhere in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Test: Search for the definition of the constant. Expect: A single occurrence in `Settings.kt`.
rg -c $'ROOM_ID\s*=\s*"[^"]+"' --files-with-matches | grep -q "Settings.kt"
# Test: Search for string literals matching the room ID pattern. Expect: No occurrences.
rg $'"[a-zA-Z0-9]+"' --files-with-matches | grep -v "Settings.kt"
Length of output: 1774
Script:
#!/bin/bash
# Search for Settings class and ROOM_ID constant definition
fd -e kt -e java | xargs rg "class.*Settings" -A 5
# Search for all usages of Settings.ROOM_ID
rg "Settings\.ROOM_ID" -A 2
Length of output: 638
Script:
#!/bin/bash
# Find Settings.kt file
fd Settings.kt
# If found, show its contents
fd Settings.kt -x cat {}
Length of output: 186
ttypic
force-pushed
the
ECO-5082/add-presence
branch
from
47eb802
to
9412107
Compare
ttypic
force-pushed
the
ECO-5082/add-presence
branch
from
9412107
to
b3f1923
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
chat-android/src/main/java/com/ably/chat/Presence.kt (4)
143-152: Add error handling and logging for data conversionThe current implementation might silently fail when processing malformed data. Consider:
- Adding error handling for data conversion
- Including debug logging for troubleshooting
- Documenting the expected data format
override suspend fun get(params: List<Param>): List<PresenceMember> { val usersOnPresence = presence.getCoroutine(params) return usersOnPresence.map { user -> + val userData = when (val rawData = user.data) { + is JsonObject -> rawData.get("userCustomData") + else -> { + Log.d(TAG, "Unexpected data format: $rawData") + null + } + } PresenceMember( clientId = user.clientId, action = user.action, - data = (user.data as? JsonObject)?.get("userCustomData"), + data = userData, updatedAt = user.timestamp, ) } }
170-178: Improve presence event data handling safetySimilar to the get method, the event handling uses unsafe casting that could silently fail.
val presenceListener = PubSubPresenceListener { + val eventData = when (val rawData = it.data) { + is JsonObject -> rawData.get("userCustomData") + else -> { + Log.d(TAG, "Unexpected event data format: $rawData") + null + } + } val presenceEvent = PresenceEvent( action = it.action, clientId = it.clientId, timestamp = it.timestamp, - data = (it.data as? JsonObject)?.get("userCustomData"), + data = eventData, ) listener.onEvent(presenceEvent) }
188-189: Track unimplemented onDiscontinuity methodThe TODO comment should be tracked to ensure it's not forgotten in future implementations.
Would you like me to create a GitHub issue to track the implementation of the
onDiscontinuitymethod?
191-195: Document the expected data formatThe
wrapInUserCustomDatamethod creates a specific JSON structure, but this format isn't documented. Consider adding KDoc comments to explain the expected data format and structure.+ /** + * Wraps the presence data in a standardized JSON structure. + * @param data The raw presence data to wrap + * @return A JsonObject with the data wrapped in a "userCustomData" field + */ private fun wrapInUserCustomData(data: PresenceData?) = data?.let { JsonObject().apply { add("userCustomData", data) } }example/src/main/java/com/ably/chat/example/MainActivity.kt (2)
78-79: Document experimental API usage.The
@OptIn(ExperimentalMaterial3Api::class)annotation indicates use of experimental APIs that may change. Consider adding a comment explaining why it's safe to use in this context and the potential migration strategy if the API changes.
Line range hint
107-196: Consider breaking down the Chat composable for better maintainability.The Chat composable is quite long and handles multiple responsibilities (messages, reactions, input, state management). Consider splitting it into smaller, focused composables:
+ @Composable + private fun MessageList( + messages: List<Message>, + listState: LazyListState, + modifier: Modifier = Modifier + ) { + LazyColumn( + modifier = modifier + .padding(16.dp), + userScrollEnabled = true, + state = listState, + ) { + items(messages.size) { index -> + MessageBubble(messages[index]) + } + } + } + @Composable + private fun ReactionsList(reactions: List<String>) { + if (reactions.isNotEmpty()) { + Text( + "Received reactions: ${reactions.joinToString()}", + modifier = Modifier.padding(16.dp) + ) + } + }This would make the main Chat composable more focused on orchestration rather than implementation details.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
chat-android/build.gradle.kts(1 hunks)chat-android/src/main/java/com/ably/chat/Presence.kt(5 hunks)chat-android/src/main/java/com/ably/chat/Room.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Utils.kt(2 hunks)chat-android/src/test/java/com/ably/chat/PresenceTest.kt(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(5 hunks)example/src/main/java/com/ably/chat/example/Settings.kt(1 hunks)example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- chat-android/build.gradle.kts
- chat-android/src/main/java/com/ably/chat/Room.kt
- chat-android/src/test/java/com/ably/chat/PresenceTest.kt
- example/src/main/java/com/ably/chat/example/Settings.kt
- example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt
🔇 Additional comments (4)
chat-android/src/main/java/com/ably/chat/Presence.kt (1)
5-14: LGTM: Type safety improvement with JsonElement
The change from Any to JsonElement for PresenceData improves type safety and makes the data handling more explicit.
example/src/main/java/com/ably/chat/example/MainActivity.kt (2)
80-104: LGTM! Well-structured composable implementation.
The App composable follows Material Design guidelines and demonstrates good state management practices:
- Clean separation of top-level UI components
- Proper state handling for popup visibility
- Correct usage of Scaffold with TopAppBar
116-116: LGTM! Good use of Settings constant.
Using Settings.ROOM_ID instead of a hardcoded string improves maintainability.
chat-android/src/main/java/com/ably/chat/Utils.kt (1)
68-82: Add KDoc documentation and enhance error handling
As previously suggested, please add KDoc documentation explaining the behavior of these presence lifecycle methods. Additionally, consider adding error handling for common scenarios and implementing timeout protection for IO operations.
Also applies to: 84-98, 100-114
ttypic
force-pushed
the
ECO-5082/add-presence
branch
from
b3f1923
to
2cbf2f6
Compare
ttypic
force-pushed
the
ECO-5082/add-presence
branch
from
b1896e8
to
6e1fcc6
Compare
ttypic
force-pushed
the
ECO-5082/add-presence
branch
from
6e1fcc6
to
2297f70
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
chat-android/src/test/java/com/ably/chat/Sandbox.kt (1)
18-38: Add tests for the Sandbox utilityEven though this is a test utility, it's critical enough to warrant its own tests to ensure reliable test infrastructure.
Consider adding:
- Unit tests mocking the HTTP client
- Integration tests with actual sandbox (marked as optional)
- Tests for error scenarios
Would you like me to help create these tests?
chat-android/src/test/java/com/ably/chat/SandboxTest.kt (3)
10-10: Remove unused importThe
assertEmptyimport fromMultipleFailureExceptionis not used in the code.-import org.junit.runners.model.MultipleFailureException.assertEmpty
48-60: Improve helper methods flexibility and validationThe helper methods could be enhanced:
- Make sandbox environment configurable
- Add API key format validation
- Use consistent client ID parameter across methods
-private fun createSandboxChatClient(apiKey: String): DefaultChatClient { +private fun createSandboxChatClient( + apiKey: String, + environment: String = "sandbox", + clientId: String = "sandbox-client" +): DefaultChatClient { + require(apiKey.contains(":")) { "Invalid API key format" } - val realtime = createSandboxRealtime(apiKey) + val realtime = createSandboxRealtime(apiKey, environment, clientId) return DefaultChatClient(realtime, ClientOptions()) } -private fun createSandboxRealtime(chatApiKey: String, chatClientId: String = "sandbox-client"): AblyRealtime = +private fun createSandboxRealtime( + chatApiKey: String, + environment: String = "sandbox", + chatClientId: String = "sandbox-client" +): AblyRealtime = AblyRealtime( PubSubClientOptions().apply { key = chatApiKey - environment = "sandbox" + environment = environment clientId = chatClientId }, )
1-60: Consider future test coverage for room lifecycleWhile the current implementation intentionally omits room lifecycle state (as per PR objectives), consider planning additional test cases for:
- Presence behavior during room detachment
- Presence member updates during room state changes
- Error scenarios in presence operations
This will ensure robust coverage as the feature evolves.
chat-android/src/main/java/com/ably/chat/Presence.kt (2)
32-36: Consider trade-offs of typed parameters vs extensibilityThe change from
List<Param>to typed parameters improves usability and type safety. However, as mentioned in past reviews, this might limit extensibility if Ably SDK adds new parameters in the future.Consider keeping both methods:
- The current typed parameter version for common use cases
- A lower-level version that accepts
List<Param>for advanced use cases
159-159: Consider using string value for waitForSync parameterAs noted in past reviews, Ably's Java SDK expects "true" as a string value for the waitForSync parameter. While toString() conversion works, being explicit would better match the SDK's expectations.
- if (waitForSync) add(Param(GET_WAITFORSYNC, true)) + if (waitForSync) add(Param(GET_WAITFORSYNC, "true"))
🛑 Comments failed to post (8)
chat-android/src/test/java/com/ably/chat/Sandbox.kt (4)
35-37:
⚠️ Potential issueAdd error handling to tearDown()
The tearDown method should handle potential failures gracefully to ensure proper cleanup.
Apply this improvement:
suspend fun tearDown() { - client.delete("https://sandbox-rest.ably.io/apps/$appId") + try { + client.delete("$SANDBOX_URL/$appId") + } catch (e: Exception) { + throw IllegalStateException("Failed to tear down sandbox app: $appId", e) + } }Committable suggestion skipped: line range outside the PR's diff.
16-16: 🛠️ Refactor suggestion
Move HttpClient into Sandbox class and ensure proper cleanup
The global HttpClient instance should be moved into the Sandbox class and properly closed when no longer needed to prevent resource leaks. This also improves encapsulation and prevents potential issues with concurrent tests.
Apply this refactor:
-val client = HttpClient(CIO) class Sandbox { + private val client = HttpClient(CIO) private lateinit var appId: String lateinit var apiKey: String private set + fun close() { + client.close() + }📝 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.class Sandbox { private val client = HttpClient(CIO) private lateinit var appId: String lateinit var apiKey: String private set fun close() { client.close() }
40-45: 🛠️ Refactor suggestion
Bundle test resources locally instead of loading from GitHub
Loading test resources from GitHub is fragile and can cause test failures due to network issues or changes to the remote file.
- Copy the JSON file to
src/test/resources/test-app-setup.json- Apply this improvement:
-suspend fun loadAppCreationRequestBody(): JsonElement = - JsonParser.parseString( - client.get("https://raw.githubusercontent.com/ably/ably-common/refs/heads/main/test-resources/test-app-setup.json") { - contentType(ContentType.Application.Json) - }.bodyAsText(), - ).asJsonObject.get("post_apps") +suspend fun loadAppCreationRequestBody(): JsonElement { + return Sandbox::class.java.getResourceAsStream("/test-app-setup.json") + ?.bufferedReader() + ?.use { reader -> + JsonParser.parseString(reader.readText()).asJsonObject["post_apps"] + ?: throw IllegalStateException("Missing post_apps in test setup JSON") + } ?: throw IllegalStateException("Failed to load test setup JSON") +}📝 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 loadAppCreationRequestBody(): JsonElement { return Sandbox::class.java.getResourceAsStream("/test-app-setup.json") ?.bufferedReader() ?.use { reader -> JsonParser.parseString(reader.readText()).asJsonObject["post_apps"] ?: throw IllegalStateException("Missing post_apps in test setup JSON") } ?: throw IllegalStateException("Failed to load test setup JSON") }
24-33:
⚠️ Potential issueAdd error handling and make the implementation more robust
The current implementation has several potential issues:
- No error handling for HTTP or JSON parsing operations
- Hardcoded array index for key selection
- Hardcoded URLs
Consider applying these improvements:
+ companion object { + private const val SANDBOX_URL = "https://sandbox-rest.ably.io/apps" + private const val REQUIRED_KEY_CAPABILITIES = setOf("chat", "channel") + } suspend fun setUp() { - val response: HttpResponse = client.post("https://sandbox-rest.ably.io/apps") { - contentType(ContentType.Application.Json) - setBody(loadAppCreationRequestBody().toString()) - } - val body = JsonParser.parseString(response.bodyAsText()) - // From JS chat repo at 7985ab7 — "The key we need to use is the one at index 5, which gives enough permissions to interact with Chat and Channels" - apiKey = body.asJsonObject["keys"].asJsonArray[5].asJsonObject["keyStr"].asString - appId = body.asJsonObject["appId"].asString + try { + val response: HttpResponse = client.post(SANDBOX_URL) { + contentType(ContentType.Application.Json) + setBody(loadAppCreationRequestBody().toString()) + } + val body = JsonParser.parseString(response.bodyAsText()).asJsonObject + appId = body["appId"]?.asString ?: throw IllegalStateException("Missing appId in response") + + // Find the first key with required capabilities + apiKey = body["keys"]?.asJsonArray + ?.firstOrNull { key -> + key.asJsonObject["capability"].asJsonObject.keySet() + .containsAll(REQUIRED_KEY_CAPABILITIES) + } + ?.asJsonObject?.get("keyStr")?.asString + ?: throw IllegalStateException("No key found with required capabilities") + } catch (e: Exception) { + throw IllegalStateException("Failed to set up sandbox app", e) + } }📝 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.companion object { private const val SANDBOX_URL = "https://sandbox-rest.ably.io/apps" private const val REQUIRED_KEY_CAPABILITIES = setOf("chat", "channel") } suspend fun setUp() { try { val response: HttpResponse = client.post(SANDBOX_URL) { contentType(ContentType.Application.Json) setBody(loadAppCreationRequestBody().toString()) } val body = JsonParser.parseString(response.bodyAsText()).asJsonObject appId = body["appId"]?.asString ?: throw IllegalStateException("Missing appId in response") // Find the first key with required capabilities apiKey = body["keys"]?.asJsonArray ?.firstOrNull { key -> key.asJsonObject["capability"].asJsonObject.keySet() .containsAll(REQUIRED_KEY_CAPABILITIES) } ?.asJsonObject?.get("keyStr")?.asString ?: throw IllegalStateException("No key found with required capabilities") } catch (e: Exception) { throw IllegalStateException("Failed to set up sandbox app", e) } }chat-android/src/test/java/com/ably/chat/SandboxTest.kt (2)
27-34: 🛠️ Refactor suggestion
Add missing verifications to the presence test
While the test verifies the empty presence list, it's missing important checks:
- Verify successful room attachment before checking presence
- Add timeout handling for async operations
- Add error handling for failed attachment
@Test fun `should return empty list of presence members if nobody is entered`() = runTest { val chatClient = createSandboxChatClient(sandbox.apiKey) val room = chatClient.rooms.get("basketball") room.attach() + assertEquals(ChannelState.attached, room.state) val members = room.presence.get() assertEquals(0, members.size) }Committable suggestion skipped: line range outside the PR's diff.
36-45: 🛠️ Refactor suggestion
Enhance presence enter test robustness
The test should be more thorough in verifying the presence functionality:
- Verify successful room attachment
- Verify presence enter completion
- Avoid hardcoding the client ID in assertions
@Test fun `should return yourself as presence member after you entered`() = runTest { val chatClient = createSandboxChatClient(sandbox.apiKey) val room = chatClient.rooms.get("basketball") room.attach() + assertEquals(ChannelState.attached, room.state) room.presence.enter() + // Wait for presence enter completion val members = room.presence.get() assertEquals(1, members.size) - assertEquals("sandbox-client", members.first().clientId) + assertEquals(chatClient.options.clientId, members.first().clientId) }Committable suggestion skipped: line range outside the PR's diff.
gradle/libs.versions.toml (1)
62-62: 🛠️ Refactor suggestion
Use version catalog reference for android-kotlin plugin
The hardcoded version
2.0.21breaks consistency with version management. Consider using the existingkotlinversion reference to maintain consistency and prevent version conflicts.-android-kotlin = { id = "org.jetbrains.kotlin.android", version = "2.0.21" } +android-kotlin = { id = "org.jetbrains.kotlin.android", version.ref = "kotlin" }📝 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.android-kotlin = { id = "org.jetbrains.kotlin.android", version.ref = "kotlin" }chat-android/src/main/java/com/ably/chat/Presence.kt (1)
198-200:
⚠️ Potential issueCritical: Implement onDiscontinuity method
The
onDiscontinuitymethod is not implemented, which could lead to missed presence state synchronization issues. This is particularly important for handling connection state changes and presence set resynchronization.Would you like help implementing this method or creating a GitHub issue to track this task?
| @@ -50,6 +56,63 @@ suspend fun Channel.publishCoroutine(message: PubSubMessage) = suspendCoroutine | |||
| ) | |||
| } | |||
|
|
|||
| suspend fun PubSubPresence.getCoroutine(param: Param) = withContext(Dispatchers.IO) { | |||
There was a problem hiding this comment.
We can now safely remove this helper method right
| * @returns {Promise<PresenceMessage[]>} or upon failure, the promise will be rejected with an [[Ably.ErrorInfo]] object which explains the error. | ||
| * Method to get list of the current online users and returns the latest presence messages associated to it. | ||
| * @param {Ably.RealtimePresenceParams} params - Parameters that control how the presence set is retrieved. | ||
| * @returns {List<PresenceMessage>} or upon failure, the promise will throw [[Ably.ErrorInfo]] object which explains the error. |
There was a problem hiding this comment.
| * @returns {List<PresenceMessage>} or upon failure, the promise will throw [[Ably.ErrorInfo]] object which explains the error. | |
| * @returns {List<PresenceMessage>} or upon failure, will throw [[Ably.ErrorInfo]] object which explains the error. |
| @@ -40,21 +47,21 @@ interface Presence : EmitsDiscontinuities { | |||
| * @param {PresenceData} data - The users data, a JSON serializable object that will be sent to all subscribers. | |||
| * @returns {Promise<void>} or upon failure, the promise will be rejected with an {@link ErrorInfo} object which explains the error. | |||
There was a problem hiding this comment.
| * @returns {Promise<void>} or upon failure, the promise will be rejected with an {@link ErrorInfo} object which explains the error. | |
| * @returns when operation is a success, upon failure, will throw [[Ably.ErrorInfo]] object which explains the error. |
| @@ -40,21 +47,21 @@ interface Presence : EmitsDiscontinuities { | |||
| * @param {PresenceData} data - The users data, a JSON serializable object that will be sent to all subscribers. | |||
| * @returns {Promise<void>} or upon failure, the promise will be rejected with an {@link ErrorInfo} object which explains the error. | |||
| */ | |||
| suspend fun enter(data: PresenceData?) | |||
| suspend fun enter(data: PresenceData? = null) | |||
|
|
|||
| /** | |||
| * Method to update room presence, will emit an update event to all subscribers. If the user is not present, it will be treated as a join event. | |||
| * @param {PresenceData} data - The users data, a JSON serializable object that will be sent to all subscribers. | |||
| * @returns {Promise<void>} or upon failure, the promise will be rejected with an {@link ErrorInfo} object which explains the error. | |||
There was a problem hiding this comment.
Same goes for all other documentation where promise is mentioned as per https://github.com/ably-labs/ably-chat-kotlin/pull/42/files#r1840393426
ttypic
force-pushed
the
ECO-5082/add-presence
branch
from
2297f70
to
7eafc4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
chat-android/src/test/java/com/ably/chat/SandboxTest.kt (1)
35-42: Enhance presence member verification and test coverage.
- Verify room attachment state before proceeding.
- Verify the presence state of the member (e.g., "online").
- Consider adding test cases for:
- Error scenarios (network issues, invalid states)
- Multiple members
- Leaving presence
- Updating presence data
fun `should return yourself as presence member after you entered`() = runTest { val chatClient = createSandboxChatClient(sandbox.apiKey) val room = chatClient.rooms.get("basketball") room.attach() + assertEquals(ConnectionState.ATTACHED, room.state) room.presence.enter() val members = room.presence.get() assertEquals(1, members.size) - assertEquals("sandbox-client", members.first().clientId) + with(members.first()) { + assertEquals("sandbox-client", clientId) + assertEquals("online", state) // Verify presence state + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
chat-android/src/main/java/com/ably/chat/Presence.kt(5 hunks)chat-android/src/test/java/com/ably/chat/SandboxTest.kt(1 hunks)
🔇 Additional comments (4)
chat-android/src/main/java/com/ably/chat/Presence.kt (4)
5-16: LGTM: Improved type safety with JsonElement
The change from Any to JsonElement for PresenceData provides better type safety and clearer contract for presence data handling.
97-97: LGTM: Appropriate type adjustments
Good improvements:
- Making
PresenceDatanullable better represents optional presence data - Using
Longfor timestamp prevents potential overflow issues
Also applies to: 132-132, 137-137
169-178: LGTM: Clean implementation of presence operations
The implementations are consistent and properly handle data wrapping.
203-207: LGTM: Clean data wrapping implementation
The helper method is well-focused and properly handles null values.
| fun `should return empty list of presence members if nobody is entered`() = runTest { | ||
| val chatClient = createSandboxChatClient(sandbox.apiKey) | ||
| val room = chatClient.rooms.get("basketball") | ||
| room.attach() | ||
| assertEquals(ChannelState.attached, room.messages.channel.state) | ||
| val members = room.presence.get() | ||
| assertEquals(0, members.size) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify room attachment completion before checking presence.
The test should verify that the room is successfully attached before proceeding with presence checks. This ensures that any presence-related operations are performed on a fully initialized room.
fun `should return empty list of presence members if nobody is entered`() = runTest {
val chatClient = createSandboxChatClient(sandbox.apiKey)
val room = chatClient.rooms.get("basketball")
room.attach()
+ // Wait for room to be attached
+ assertEquals(ConnectionState.ATTACHED, room.state)
val members = room.presence.get()
assertEquals(0, members.size)
}📝 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.
| fun `should return empty list of presence members if nobody is entered`() = runTest { | |
| val chatClient = createSandboxChatClient(sandbox.apiKey) | |
| val room = chatClient.rooms.get("basketball") | |
| room.attach() | |
| assertEquals(ChannelState.attached, room.messages.channel.state) | |
| val members = room.presence.get() | |
| assertEquals(0, members.size) | |
| } | |
| fun `should return empty list of presence members if nobody is entered`() = runTest { | |
| val chatClient = createSandboxChatClient(sandbox.apiKey) | |
| val room = chatClient.rooms.get("basketball") | |
| room.attach() | |
| // Wait for room to be attached | |
| assertEquals(ConnectionState.ATTACHED, room.state) | |
| val members = room.presence.get() | |
| assertEquals(0, members.size) | |
| } |
| fun `should return empty list of presence members if nobody is entered`() = runTest { | ||
| val chatClient = createSandboxChatClient(sandbox.apiKey) | ||
| val room = chatClient.rooms.get("basketball") | ||
| room.attach() | ||
| assertEquals(ChannelState.attached, room.messages.channel.state) | ||
| val members = room.presence.get() | ||
| assertEquals(0, members.size) | ||
| } | ||
|
|
||
| @Test | ||
| fun `should return yourself as presence member after you entered`() = runTest { | ||
| val chatClient = createSandboxChatClient(sandbox.apiKey) | ||
| val room = chatClient.rooms.get("basketball") | ||
| room.attach() | ||
| room.presence.enter() | ||
| val members = room.presence.get() | ||
| assertEquals(1, members.size) | ||
| assertEquals("sandbox-client", members.first().clientId) |
There was a problem hiding this comment.
💡 Codebase verification
Additional test coverage is still needed for robust presence implementation
After reviewing the existing tests, the original review comment remains valid. Current test coverage focuses on:
- Basic presence functionality (SandboxTest.kt)
- Presence event transformation and data handling (PresenceTest.kt)
- API error cases (ChatApiTest.kt)
Key scenarios still missing:
- Concurrent presence operations with multiple clients
- Presence state synchronization between clients
- Presence updates and state transitions (enter → update → leave)
- Network disconnection/reconnection handling
- Edge cases like duplicate entries or conflicting states
These scenarios are critical for a production-ready presence implementation.
🔗 Analysis chain
Expand test coverage for robust presence implementation.
While these tests cover basic presence functionality, consider adding tests for:
- Concurrent presence operations
- Presence synchronization between multiple clients
- Presence updates and state changes
- Room lifecycle integration (mentioned as omitted in PR description)
- Network disconnection and reconnection scenarios
This will ensure the presence implementation is robust and production-ready.
Let's check for any existing presence-related tests:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for presence-related test files
rg -t kotlin -l "presence" "src/test"
# Search for test methods related to presence
rg -t kotlin "fun.*presence" "src/test"
Length of output: 173
Script:
#!/bin/bash
# Search for presence-related test files in the correct path
rg -t kotlin -l "presence" "chat-android/src/test"
# Search for test methods related to presence
rg -t kotlin "fun.*presence" "chat-android/src/test"
# Look for any presence-related test classes
fd -e kt . "chat-android/src/test" --exec grep -l "presence"
Length of output: 784
Script:
#!/bin/bash
# Let's examine the content of PresenceTest.kt and ChatApiTest.kt to understand existing test coverage
echo "=== PresenceTest.kt ==="
cat "chat-android/src/test/java/com/ably/chat/PresenceTest.kt"
echo -e "\n=== ChatApiTest.kt ==="
cat "chat-android/src/test/java/com/ably/chat/ChatApiTest.kt"
Length of output: 8401
| * Method to get list of the current online users and returns the latest presence messages associated to it. | ||
| * @param {Ably.RealtimePresenceParams} params - Parameters that control how the presence set is retrieved. | ||
| * @throws {@link io.ably.lib.types.AblyException} object which explains the error. | ||
| * @returns {List<PresenceMessage>} | ||
| */ | ||
| suspend fun get(params: List<Params>): List<PresenceMember> | ||
| suspend fun get(waitForSync: Boolean = true, clientId: String? = null, connectionId: String? = null): List<PresenceMember> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Documentation needs update to match implementation
The documentation refers to Ably.RealtimePresenceParams but the method signature uses individual parameters. Consider using a typed parameter object as suggested in previous reviews for better API clarity and future extensibility.
data class RealtimePresenceParams(
val waitForSync: Boolean = true,
val clientId: String? = null,
val connectionId: String? = null
)
suspend fun get(params: RealtimePresenceParams? = null): List<PresenceMember>| data = (user.data as? JsonObject)?.get("userCustomData"), | ||
| updatedAt = user.timestamp, |
There was a problem hiding this comment.
Add safe casting for JsonObject data extraction
The current implementation uses an unsafe cast that could fail at runtime. Consider using safe cast and proper error handling.
- data = (user.data as? JsonObject)?.get("userCustomData"),
+ data = runCatching {
+ (user.data as? JsonObject)?.get("userCustomData")
+ }.getOrNull(),📝 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.
| data = (user.data as? JsonObject)?.get("userCustomData"), | |
| updatedAt = user.timestamp, | |
| data = runCatching { | |
| (user.data as? JsonObject)?.get("userCustomData") | |
| }.getOrNull(), | |
| updatedAt = user.timestamp, |
| data = (it.data as? JsonObject)?.get("userCustomData"), | ||
| ) |
There was a problem hiding this comment.
Add safe casting in presence event creation
Similar to the get method, this uses an unsafe cast that could fail at runtime.
- data = (it.data as? JsonObject)?.get("userCustomData"),
+ data = runCatching {
+ (it.data as? JsonObject)?.get("userCustomData")
+ }.getOrNull(),📝 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.
| data = (it.data as? JsonObject)?.get("userCustomData"), | |
| ) | |
| data = runCatching { | |
| (it.data as? JsonObject)?.get("userCustomData") | |
| }.getOrNull(), |
| override fun onDiscontinuity(listener: EmitsDiscontinuities.Listener): Subscription { | ||
| TODO("Not yet implemented") | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Implementation needed across multiple components
The onDiscontinuity method is not implemented in multiple core components: Messages, Presence, Typing, RoomReactions, and Occupancy. This appears to be a systematic gap in implementing the EmitsDiscontinuities interface across the codebase. The interface is defined to handle discontinuity events with error information, but none of the implementations are complete.
chat-android/src/main/java/com/ably/chat/Messages.ktchat-android/src/main/java/com/ably/chat/Presence.ktchat-android/src/main/java/com/ably/chat/Typing.ktchat-android/src/main/java/com/ably/chat/RoomReactions.ktchat-android/src/main/java/com/ably/chat/Occupancy.kt
🔗 Analysis chain
Implement onDiscontinuity method
The onDiscontinuity method is marked as not implemented. Please clarify if this is intentional or if it should be implemented as part of this PR.
Would you like help implementing this method?
Let me check the implementation in other files to understand the expected behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if onDiscontinuity is used in tests or other files
rg -l "onDiscontinuity"
Length of output: 357
Script:
#!/bin/bash
# Check EmitsDiscontinuities interface definition
cat chat-android/src/main/java/com/ably/chat/EmitsDiscontinuities.kt
# Check implementations in other files
rg "onDiscontinuity.*{" -A 5 --multiline
Length of output: 944
Script:
#!/bin/bash
# Check implementations in other files with fixed regex
rg "override fun onDiscontinuity" -A 5
# Also check if there are any tests for this functionality
fd -e kt -e java test --exec rg -l "onDiscontinuity" {} \;
Length of output: 2233
ttypic
force-pushed
the
ECO-5014/simple-sandbox-setup
branch
from
0b019ad
to
2b17388
Compare
This implementation doesn’t fully follow the spec; it omits all points that reference the room lifecycle state
ttypic
force-pushed
the
ECO-5082/add-presence
branch
from
7eafc4f
to
492fba8
Compare
This implementation doesn’t fully follow the spec; it omits all points that reference the room lifecycle state
Summary by CodeRabbit
Release Notes
New Features
PresencePopupfor displaying chat presence information, allowing users to manage their status.Settingsobject, including a constantROOM_ID.Bug Fixes
Improvements
DefaultRoomclass by centralizingclientIdretrieval.