[ECO-4943] feat: basic sending and receiving messages in the Chat #16
Conversation
WalkthroughThe changes primarily involve refactoring and enhancing the chat functionality in the Ably library. Key updates include the introduction of a Changes
Assessment against linked issues
Possibly related PRs
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (13)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (12)
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 (
|
It's proven to be easier to manage to implement when subscription return object that you can use to unsubscribe
ttypic
force-pushed
the
feat-use-cancelation-object
branch
from
14aecaf
to
54ecb93
Compare
ttypic
force-pushed
the
ECO-4943/sending-receiving-messages
branch
from
98158ac
to
4d3c92c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- chat-android/src/main/java/com/ably/chat/ChatApi.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/ChatClient.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/JsonUtils.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/Messages.kt (4 hunks)
- chat-android/src/main/java/com/ably/chat/Room.kt (2 hunks)
- chat-android/src/main/java/com/ably/chat/RoomOptions.kt (2 hunks)
- chat-android/src/main/java/com/ably/chat/Rooms.kt (3 hunks)
- chat-android/src/main/java/com/ably/chat/Utils.kt (1 hunks)
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt (5 hunks)
- chat-android/src/test/java/com/ably/chat/MessagesTest.kt (1 hunks)
- chat-android/src/test/java/com/ably/chat/TestUtils.kt (1 hunks)
- chat-android/src/test/java/io/ably/lib/realtime/RealtimeUtils.kt (1 hunks)
- example/src/main/java/com/ably/chat/example/MainActivity.kt (5 hunks)
Files skipped from review due to trivial changes (1)
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
Additional comments not posted (46)
chat-android/src/test/java/io/ably/lib/realtime/RealtimeUtils.kt (2)
12-17: LGTM!The
buildChannelStateChangefunction provides a convenient way to createChannelStateChangeobjects for testing purposes. The function parameters cover all the necessary fields to construct aChannelStateChangeobject.Note: This function can be removed if the visibility of the
ChannelStateChangeconstructor is increased in the future.
23-28: LGTM!The
buildRealtimeChannelfunction provides a convenient way to create realtimeChannelobjects for testing purposes. The function initializes the client with a dummy API key and sets the auto-connect option to false, which is appropriate for testing scenarios.The function allows for easy retrieval of a channel by name, facilitating the interaction with Ably's real-time messaging capabilities.
chat-android/src/main/java/com/ably/chat/ChatClient.kt (1)
40-41: LGTM!The change to provide a default value for the
clientOptionsparameter in theChatClientfunction signature is a good enhancement. It improves the usability of the function by making theclientOptionsargument optional, allowing for simpler instantiation when default options suffice. The change is backward compatible and does not introduce any breaking changes.chat-android/src/test/java/com/ably/chat/TestUtils.kt (4)
8-14: LGTM!The function correctly mocks the
AsyncHttpPaginatedResponseobject using the MockK library. The implementation is accurate and serves its purpose for testing.
16-25: LGTM!The function correctly mocks the API response for retrieving messages from a chat room. It intercepts the GET request and invokes the callback with a mocked response, which is suitable for testing purposes.
27-38: LGTM!The function correctly mocks the API response for sending a message to a chat room. It intercepts the POST request and returns a mocked response, which is suitable for testing purposes.
40-51: LGTM!The function correctly mocks the API response for retrieving occupancy information of a chat room. It intercepts the GET request to the occupancy endpoint and returns a mocked response, which is suitable for testing purposes.
chat-android/src/main/java/com/ably/chat/RoomOptions.kt (2)
24-24: LGTM!The change in the
reactionsproperty initialization and the corresponding change in theRoomReactionsOptionsclass declaration to an object declaration are consistent. TreatingRoomReactionsOptionsas a singleton object is a valid approach.
30-30: LGTM!The change in the
occupancyproperty initialization and the corresponding change in theOccupancyOptionsclass declaration to an object declaration are consistent. TreatingOccupancyOptionsas a singleton object is a valid approach.chat-android/src/main/java/com/ably/chat/Rooms.kt (4)
27-27: LGTM!Providing a default value for the
optionsparameter enhances the flexibility of thegetfunction. It allows the method to be called without explicitly passingRoomOptions, making it easier to retrieve a room without needing to specify options every time.
50-50: LGTM!Changing the type of the
roomIdToRoommap fromMutableMap<String, Room>toMutableMap<String, DefaultRoom>indicates a more specific implementation of theRoominterface. This change aligns with the principle of using the most specific type possible and may provide additional functionality or optimizations specific to the default implementation.
75-75: LGTM!Removing the room from the
roomIdToRoommap in thereleasemethod ensures that the room is no longer accessible through theRoomsinterface. This change is part of the proper resource management and cleanup process when a room is released.
76-76: LGTM!Calling the
releasefunction of theroomwhen it is removed from theroomIdToRoommap ensures proper resource management. It invokes the release logic on the room being removed, which may help prevent memory leaks or other resource-related issues. This change ensures that the room is properly cleaned up when it is no longer needed.chat-android/src/main/java/com/ably/chat/Utils.kt (2)
42-45: LGTM!The change handles potential null values for
options.paramsmore gracefully by using the Elvis operator to provide a default empty map. This is a good practice to avoid null pointer exceptions.
48-103: Excellent work!The new
DeferredValue<T>class is a well-designed and thread-safe implementation for deferring the evaluation of a value. The class is thoroughly documented, making its purpose and usage clear.Key highlights:
- The class is marked as
internal, which is appropriate since it's not intended for public use.- The implementation is thread-safe, using a lock object to ensure thread safety during value assignment and observer notifications.
- The
completeWithmethod ensures that the value can only be set once, throwing anIllegalStateExceptionif called again.- The
awaitmethod allows a coroutine to wait until the value is completed, resuming with the completed value once available.Overall, this is a solid addition to the codebase.
chat-android/src/main/java/com/ably/chat/JsonUtils.kt (9)
11-12: LGTM!The function correctly converts a nullable
JsonElementto anHttpCore.RequestBodyusing the appropriate utility function.
14-16: LGTM!The function correctly converts a map of string key-value pairs into a
JsonObjectusing theaddPropertyfunction.
18-20: LGTM!The function correctly converts a
JsonElementinto a map of strings, ensuring that only JSON primitives are included.
22-29: LGTM!The function correctly ensures that a
JsonElementis aJsonObjectand throws anAblyExceptionwith an appropriate error message when the expected JSON structure is not met.
31-42: LGTM!The function correctly retrieves a string value from a
JsonElementfor a given member name, ensuring that the value is aJsonPrimitiveand throwing anAblyExceptionwith an appropriate error message when the expected JSON structure is not met.
44-54: LGTM!The function correctly retrieves a long value from a
JsonElementfor a given member name, ensuring that the value is a valid long and throwing anAblyExceptionwith an appropriate error message when the expected JSON structure is not met or the value is not a valid long.
56-66: LGTM!The function correctly retrieves an int value from a
JsonElementfor a given member name, ensuring that the value is a valid int and throwing anAblyExceptionwith an appropriate error message when the expected JSON structure is not met or the value is not a valid int.
68-79: LGTM!The function correctly retrieves a
JsonPrimitivevalue from aJsonElementfor a given member name, ensuring that the value is aJsonPrimitiveand throwing anAblyExceptionwith an appropriate error message when the expected JSON structure is not met.
81-84: LGTM!The function correctly retrieves a
JsonElementvalue from aJsonElementfor a given member name, ensuring that the field exists and throwing anAblyExceptionwith an appropriate error message when the required field is missing.chat-android/src/main/java/com/ably/chat/Room.kt (3)
93-95: LGTM!The introduction of a private backing property
_messagesis a good practice for encapsulation. The initialization of_messageswith the required dependencies is correct.
99-100: LGTM!Overriding the
messagesproperty to return the private_messagesproperty is a good practice for encapsulation. This change is consistent with the introduction of the private_messagesproperty.
135-138: LGTM!The introduction of the
release()function is a good practice for resource management. Calling therelease()function on the private_messagesproperty is correct. This change is consistent with the list of alterations provided in the AI-generated summary.chat-android/src/main/java/com/ably/chat/ChatApi.kt (2)
17-17: LGTM!Changing the class visibility to internal is a good practice to encapsulate the implementation details and restrict the usage of the class to the same module.
24-30: LGTM!The addition of the
fromSerialparameter provides more granular control over message retrieval, which enhances the flexibility of the message-fetching logic. The changes to theparamsvariable construction based on thefromSerialparameter are implemented correctly.example/src/main/java/com/ably/chat/example/MainActivity.kt (5)
17-17: LGTM!The import statement is necessary and correctly added.
24-24: LGTM!The import statement is necessary and correctly added.
Line range hint
35-42: LGTM!The import statements are necessary and correctly added.
49-58: LGTM!The
realtimeClientandchatClientare correctly created with the necessary configurations.
Line range hint
75-134: LGTM!The changes in the
Chatcomposable function are well-implemented and align with the transition fromChatApitoChatClient. The usage ofDisposableEffectfor managing the subscription lifecycle and retrieving previous messages is correct. The updates to theLazyColumnimprove the user experience by automatically scrolling to the latest message. The message sending functionality is correctly updated to useroom.messages.send.Overall, the changes enhance the chat functionality and provide a better structure for message handling.
chat-android/src/test/java/com/ably/chat/MessagesTest.kt (5)
55-86: LGTM!The test function correctly verifies the ability to send a message and ensures that the response matches the expected structure. The mocking of the API response and the assertion of the sent message against the expected
Messageobject are implemented correctly.
88-140: LGTM!The test function correctly verifies the ability to subscribe to incoming messages and handle the received messages. The mocking of the subscription, the simulation of an incoming message, and the assertion of the received message event against the expected
MessageEventobject are implemented correctly.
142-153: LGTM!The test function correctly verifies that accessing message history when not subscribed throws an
AblyExceptionwith the expected error code. The subscription and immediate cancellation, followed by the assertion of the exception, are implemented correctly.
155-168: LGTM!The test function correctly verifies that each subscription maintains its own channel serial. The setting of the
channelSerialproperty, the creation of multiple subscriptions, and the assertions of thefromSerialProviderfor each subscription are implemented correctly.
170-188: LGTM!The test function correctly verifies that the subscription updates the channel serial after a reattach with
resume = false. The setting of thechannelSerialproperty, the creation of the subscription, the simulation of the channel state change, and the assertion of the updatedfromSerialProviderare implemented correctly.chat-android/src/main/java/com/ably/chat/Messages.kt (7)
182-183: LGTM!The change to the
getPreviousMessagesmethod signature in theMessagesSubscriptioninterface improves usability by allowing callers to specify message retrieval criteria directly. The default parameter values ensure backward compatibility.
186-205: Looks good!The
DefaultMessagesSubscriptionclass correctly implements the updatedMessagesSubscriptioninterface. The use offromSerialProviderto associate deferred channel serial values with the current channel's state helps manage message continuity and prevent duplication, aligning with the PR objective of improving message handling.
213-231: Great improvements!The changes to the
DefaultMessagesclass enhance the messaging functionality by providing better management of listeners and handling of channel state changes. The use ofupdateChannelSerialsAfterDiscontinuityensures that listeners are updated correctly after any discontinuities, improving the robustness of message handling and aligning with the PR objectives.
234-272: Nicely done!The updates to the
subscribemethod in theDefaultMessagesclass improve the robustness of message handling by parsing the message data and emitting it to listeners. The use ofassociateWithCurrentChannelSerialhelps manage message continuity and prevent duplication, aligning with the PR objective of enhancing the chat functionality.
283-285: Good addition!The
releasemethod is a useful addition for cleaning up resources when theDefaultMessagesinstance is no longer needed.
303-308: Helpful utility method!The
requireChannelSerialmethod is a helpful utility for retrieving the channel serial or throwing an exception if it's not available, improving code readability and error handling.
341-352: Nice utility types for parsing message data!The addition of the
PubSubMessageDatadata class andparsePubSubMessageDatafunction improves code organization and readability by encapsulating the parsing logic for message data from the Pub/Sub channel. TheparsePubSubMessageDatafunction handles invalid data by throwing an exception, ensuring that only valid message data is processed. These changes align with the PR objective of enhancing the chat functionality and improving message handling.
Comments failed to post (1)
chat-android/src/main/java/com/ably/chat/Messages.kt (1)
293-301: > [!TIP]
Codebase Verification
Based on the analysis of the provided code snippets and the potential race condition, we can now generate a final response.
Race condition in
associateWithCurrentChannelSerialconfirmed, impactinggetPreviousMessagesThe race condition mentioned in the review comment is indeed present and can potentially lead to duplicated messages in
getPreviousMessagescalls. Here's a summary of the issue:
- The
associateWithCurrentChannelSerialmethod sets the channel serial when the channel is attached or becomes attached.- The
getPreviousMessagesmethod relies on thefromSerialProvider, which is set byassociateWithCurrentChannelSerial.- If the channel state changes between checking the state and setting the serial, it could lead to inconsistencies.
To address this issue, consider the following recommendations:
- Implement a synchronization mechanism to ensure atomic operations when setting and reading the channel serial.
- Use a thread-safe data structure or a concurrent primitive to store and update the channel serial.
- Consider implementing a retry mechanism in
getPreviousMessagesto handle cases where the channel serial might be inconsistent.- Add logging or telemetry to track occurrences of this race condition in production.
These changes will help improve the reliability of the messaging system and reduce the likelihood of duplicated messages.
Analysis chain
Potential race condition in
associateWithCurrentChannelSerial.The
associateWithCurrentChannelSerialmethod helps manage message continuity, but the comment indicates a potential race condition that could lead to duplicated messages ingetPreviousMessagescalls.Please verify the impact of this race condition on message duplication and consider addressing it to ensure the reliability of the messaging system. You can use the following script to search for instances of
getPreviousMessagesand analyze how the race condition might affect its behavior:
To gather more information about the implementation and usage of
getPreviousMessagesand its relation to the potential race condition, let's examine the Messages.kt file more closely.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for `getPreviousMessages` usage and analyze the impact of the race condition. # Test: Search for `getPreviousMessages` usage. rg --type kotlin $'getPreviousMessages'Length of output: 758
Script:
#!/bin/bash # Description: Examine the implementation of getPreviousMessages and related methods in Messages.kt # Search for the getPreviousMessages implementation and surrounding context rg --type kotlin -A 20 -B 10 'suspend fun getPreviousMessages' chat-android/src/main/java/com/ably/chat/Messages.kt # Search for usage of associateWithCurrentChannelSerial rg --type kotlin 'associateWithCurrentChannelSerial' chat-android/src/main/java/com/ably/chat/Messages.kt # Search for usage of channel serials in message retrieval rg --type kotlin 'channelSerial' chat-android/src/main/java/com/ably/chat/Messages.ktLength of output: 2466
ttypic
force-pushed
the
ECO-4943/sending-receiving-messages
branch
from
4d3c92c
to
8d136b2
Compare
.gitignore
Outdated
| @@ -96,6 +96,7 @@ publish.properties | |||
| /.idea/jarRepositories.xml | |||
| /.idea/misc.xml | |||
| /.idea/shelf | |||
| /.idea/uiDesigner.xml | |||
There was a problem hiding this comment.
Should we just ignore all of .idea?
There was a problem hiding this comment.
Yes, IDE specific settings should be ignored .idea/**
There was a problem hiding this comment.
No, we shouldn't ignore all files in .idea folder. It contains dictionary and codestyles settings that are useful if you use jetbrains IDE (Android Studio or IDEA). It's a good practice to add this settings for kotlin projects. But I obviously copied really bad gitignore pattern from some kotlin library template, didn't put much thought to it. I'll remove changes to gitignore in this PR and add separate PR that introduce updates to gitignore
There was a problem hiding this comment.
| channelStateListener = ChannelStateListener { | ||
| if (!it.resumed) updateChannelSerialsAfterDiscontinuity() | ||
| } | ||
| channel.on(ChannelState.attached, channelStateListener) |
There was a problem hiding this comment.
We also need to handle ChannelEvent.update here - which is RTL2g in the core feature spec
There was a problem hiding this comment.
Thanks, nice catch!
| val pubSubMessage = it ?: throw AblyException.fromErrorInfo( | ||
| ErrorInfo("Got empty pubsub channel message", HttpStatusCodes.BadRequest, ErrorCodes.BadRequest), | ||
| ) | ||
| val data = parsePubSubMessageData(pubSubMessage.data) |
There was a problem hiding this comment.
Possible optimisation we can do here - rather than parsing data for every subscription, can have an "internal" subscriber that's put on the channel which then translates once and emits to all "actual" subscribers.
In JS we did this by registering the "internal" subscriber with the realtime channel, then registering user listeners with an EventEmitter instance on this class.
There was a problem hiding this comment.
Looking at code further down, won't this cause duplicate messages on multiple subscriptions?
As multiple instances of messageListener will call emitMessage which iterates all listeners?
There was a problem hiding this comment.
Yes, you right, I'll add tests :) I was thinking to use internal eventbus, but decided to do this later, because we need to decide on implementation details. I'll create an issue for this
| emitMessage(chatMessage) | ||
| } | ||
|
|
||
| channel.subscribe(MessageEventType.Created.eventName, messageListener) |
There was a problem hiding this comment.
This would cause an implicit channel attach which will break room lifecycle - subscribing should cause no side-effects.
I assume there's an ably-java ticket that needs doing first? :)
There was a problem hiding this comment.
Yes, of course, ably/ably-java#1027, I think until it release we can leave it as is. I'll add separate issue for this, too
| val data = parsePubSubMessageData(pubSubMessage.data) | ||
| val chatMessage = Message( | ||
| roomId = roomId, | ||
| createdAt = pubSubMessage.timestamp, |
There was a problem hiding this comment.
In JS we've tended towards native date types for createdAt - not sure what's better for Kotlin?
There was a problem hiding this comment.
I don't have strong opinion right now. Kotlin standard library doesn't have date type out of the box, and I still believe that one day we can provide Kotlin multiplatform support for all our products :) I'll think about this, look how easy to use timestamps in example app. Also want to research how libraries with kotlin multiplatform support like apollo-graphql deals with this. For now I suggest keeping date as Long
There was a problem hiding this comment.
wdyt about https://github.com/Kotlin/kotlinx-datetime. It's a multiplatform datetime library
There was a problem hiding this comment.
If I’m not mistaken, it’s not stable yet, so I don’t think we should depend on it right now. We can decide this later—it’s not an architectural design decision that will cause issues for us later, and it will be easy to refactor whenever we want.
ttypic
force-pushed
the
ECO-4943/sending-receiving-messages
branch
3 times, most recently
from
9b4a5e8
to
60813de
Compare
There was a problem hiding this comment.
Overall looks good. More tests can be added in the future.
( Not sure if we support sandbox integration tests yet )
Also, need to update example app with clean code as per comment #16 (comment)
ttypic
force-pushed
the
ECO-4943/sending-receiving-messages
branch
2 times, most recently
from
17cdb51
to
e6ead2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- chat-android/src/main/java/com/ably/chat/ChatApi.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/ChatClient.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/JsonUtils.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/Messages.kt (4 hunks)
- chat-android/src/main/java/com/ably/chat/Room.kt (2 hunks)
- chat-android/src/main/java/com/ably/chat/RoomOptions.kt (2 hunks)
- chat-android/src/main/java/com/ably/chat/Rooms.kt (3 hunks)
- chat-android/src/main/java/com/ably/chat/Utils.kt (1 hunks)
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt (5 hunks)
- chat-android/src/test/java/com/ably/chat/MessagesTest.kt (1 hunks)
- chat-android/src/test/java/com/ably/chat/TestUtils.kt (1 hunks)
- chat-android/src/test/java/io/ably/lib/realtime/RealtimeUtils.kt (1 hunks)
- example/src/main/java/com/ably/chat/example/MainActivity.kt (6 hunks)
Files skipped from review due to trivial changes (1)
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
Files skipped from review as they are similar to previous changes (11)
- chat-android/src/main/java/com/ably/chat/ChatApi.kt
- chat-android/src/main/java/com/ably/chat/ChatClient.kt
- chat-android/src/main/java/com/ably/chat/JsonUtils.kt
- chat-android/src/main/java/com/ably/chat/Messages.kt
- chat-android/src/main/java/com/ably/chat/Room.kt
- chat-android/src/main/java/com/ably/chat/RoomOptions.kt
- chat-android/src/main/java/com/ably/chat/Rooms.kt
- chat-android/src/main/java/com/ably/chat/Utils.kt
- chat-android/src/test/java/com/ably/chat/MessagesTest.kt
- chat-android/src/test/java/com/ably/chat/TestUtils.kt
- chat-android/src/test/java/io/ably/lib/realtime/RealtimeUtils.kt
Additional comments not posted (1)
example/src/main/java/com/ably/chat/example/MainActivity.kt (1)
190-219: Great use of@Previewannotations for UI componentsAdding
@Previewcomposables forMessageBubbleandChatInputFieldenhances the development experience by allowing real-time previews of these UI components. This addresses previous concerns about the lack of local previews and facilitates UI adjustments without the need for repeated app runs.
| DisposableEffect(Unit) { | ||
| val subscription = room.messages.subscribe { | ||
| messages += it.message | ||
| coroutineScope.launch { | ||
| messages = chatApi.getMessages(roomId, QueryOptions(orderBy = OldestFirst)).items | ||
| listState.animateScrollToItem(messages.size - 1) | ||
| } | ||
| }) { | ||
| Text("Load") | ||
| } | ||
|
|
||
| coroutineScope.launch { | ||
| messages = subscription.getPreviousMessages().items.reversed() | ||
| if (messages.isNotEmpty()) listState.animateScrollToItem(messages.size - 1) | ||
| } | ||
|
|
||
| onDispose { | ||
| subscription.unsubscribe() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Replace DisposableEffect with LaunchedEffect for coroutine management
Using DisposableEffect in this context may lead to coroutines that outlive the composable's lifecycle, potentially causing memory leaks. Instead, consider using LaunchedEffect, which automatically cancels coroutines when the composable leaves the composition. This ensures that resources are properly managed.
Apply this diff to refactor the code:
-DisposableEffect(Unit) {
- val subscription = room.messages.subscribe {
- messages += it.message
- coroutineScope.launch {
- listState.animateScrollToItem(messages.size - 1)
- }
- }
-
- coroutineScope.launch {
- messages = subscription.getPreviousMessages().items.reversed()
- if (messages.isNotEmpty()) listState.animateScrollToItem(messages.size - 1)
- }
-
- onDispose {
- subscription.unsubscribe()
- }
-}
+LaunchedEffect(Unit) {
+ val subscription = room.messages.subscribe {
+ messages += it.message
+ listState.animateScrollToItem(messages.size - 1)
+ }
+
+ messages = subscription.getPreviousMessages().items.reversed()
+ if (messages.isNotEmpty()) listState.animateScrollToItem(messages.size - 1)
+
+ awaitDispose {
+ subscription.unsubscribe()
+ }
+}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.
| DisposableEffect(Unit) { | |
| val subscription = room.messages.subscribe { | |
| messages += it.message | |
| coroutineScope.launch { | |
| messages = chatApi.getMessages(roomId, QueryOptions(orderBy = OldestFirst)).items | |
| listState.animateScrollToItem(messages.size - 1) | |
| } | |
| }) { | |
| Text("Load") | |
| } | |
| coroutineScope.launch { | |
| messages = subscription.getPreviousMessages().items.reversed() | |
| if (messages.isNotEmpty()) listState.animateScrollToItem(messages.size - 1) | |
| } | |
| onDispose { | |
| subscription.unsubscribe() | |
| } | |
| } | |
| LaunchedEffect(Unit) { | |
| val subscription = room.messages.subscribe { | |
| messages += it.message | |
| listState.animateScrollToItem(messages.size - 1) | |
| } | |
| messages = subscription.getPreviousMessages().items.reversed() | |
| if (messages.isNotEmpty()) listState.animateScrollToItem(messages.size - 1) | |
| awaitDispose { | |
| subscription.unsubscribe() | |
| } | |
| } |
There was a problem hiding this comment.
There is no method awaitDispose in the CoroutineScope. coroutineScope in the code is produced by rememberCoroutineScope function, so it can't outlive the composable's lifecycle
There was a problem hiding this comment.
@ttypic, thank you for the clarification. You're absolutely right—awaitDispose isn't applicable here, and since coroutineScope from rememberCoroutineScope is tied to the composable's lifecycle, it won't outlive the composable. Using DisposableEffect as you've implemented is appropriate. I appreciate you pointing that out.
(｡•́︿•̀｡)
Learnings added
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-09-17T15:07:06.194Z
Learning: When using `rememberCoroutineScope`, the `CoroutineScope` is tied to the composable's lifecycle, so replacing `DisposableEffect` with `LaunchedEffect` is unnecessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Sure thing @sacOO7!
(｡•́︿•̀｡)
Learnings used
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-09-17T15:07:06.349Z
Learning: When using `rememberCoroutineScope`, the `CoroutineScope` is tied to the composable's lifecycle, so replacing `DisposableEffect` with `LaunchedEffect` is unnecessary.
There was a problem hiding this comment.
Can you describe what it exactly represents @coderabbitai
There was a problem hiding this comment.
@sacOO7, the face (｡•́︿•̀｡) represents a look of sadness or disappointment. It conveys feelings of being sorry or empathetic in response to the situation.
Learnings used
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-09-17T15:07:06.349Z
Learning: When using `rememberCoroutineScope`, the `CoroutineScope` is tied to the composable's lifecycle, so replacing `DisposableEffect` with `LaunchedEffect` is unnecessary.
There was a problem hiding this comment.
Can you add to the learnings to make happy face when you feel empathetic next time
…th coroutine cancellation
ttypic
force-pushed
the
feat-use-cancelation-object
branch
from
5ef6411
to
f5b2078
Compare
In this commit, we introduce the basic implementation for sending and receiving messages. Corner cases and potential race conditions are not the focus at this stage and will be addressed later, once Room lifecycle management is implemented and a unified test suite is set up.
ttypic
force-pushed
the
ECO-4943/sending-receiving-messages
branch
from
e6ead2d
to
14b1cfc
Compare
Resolves #2
In this PR, we introduce the basic implementation for sending and receiving messages. Corner cases and potential race conditions are not the focus at this stage and will be addressed later, once Room lifecycle management is implemented and a unified test suite is set up.
Summary by CodeRabbit
Release Notes
New Features
ChatClientfor message handling.Improvements
Refactor