[CHA-801] Flow and Jetpack Compose support #116
Conversation
WalkthroughThe changes implement reactive, flow-based APIs across multiple chat components. Default property values have been introduced alongside new extension functions (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI Component
participant Conn as Connection
participant FlowUtil as transformCallbackAsFlow
participant Channel as Channel
UI->>Conn: Call statusAsFlow()
Conn->>FlowUtil: Provide callback lambda
FlowUtil->>Channel: Create Channel with unlimited buffer
Conn->>FlowUtil: Emit status update via callback
FlowUtil->>Channel: Send event to Channel
Channel->>UI: Emit event to subscriber as Flow
UI->>Channel: Cancel subscription
Channel->>FlowUtil: Trigger unsubscribe() on cancellation
sequenceDiagram
participant ComposeUI as Composable UI
participant Collect as collectAsStatus()
participant Conn as Connection
participant Flow as statusAsFlow Flow
ComposeUI->>Collect: Invoke collectAsStatus()
Collect->>Conn: Call statusAsFlow()
Conn->>Flow: Emit connection status updates
Flow->>Collect: Update mutable state via LaunchedEffect
Collect->>ComposeUI: Render updated connection status
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
Code Coverage
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (21)
chat-android/src/main/java/com/ably/chat/annotations/Annotations.kt (3)
1-1: Consider more specific suppression reason and file naming.The
@file:Suppress("Filename")suggests there's a naming convention issue being suppressed. Consider either:
- Renaming the file to match the contained class (e.g.,
ExperimentalChatApi.kt)- Using a more specific suppression message that explains why this naming convention is being violated
10-26: Good implementation of experimental API annotation.The annotation is well-defined with appropriate metadata:
@MustBeDocumentedensures API docs include this annotation@RequiresOptInwith warning level is appropriate for experimental features- Clear explanation message about potential future changes
- Comprehensive target list covering all relevant Kotlin constructs
Consider adding a usage example in the documentation to show how developers should opt-in to use APIs marked with this annotation.
17-25: Missing annotation targets for potential use cases.The current targets don't include
PROPERTY_GETTER,VALUE_PARAMETER, orTYPE, which might be needed in the future for more comprehensive coverage.@Target( AnnotationTarget.CLASS, AnnotationTarget.TYPEALIAS, AnnotationTarget.FUNCTION, AnnotationTarget.PROPERTY, AnnotationTarget.FIELD, AnnotationTarget.CONSTRUCTOR, AnnotationTarget.PROPERTY_SETTER, + AnnotationTarget.PROPERTY_GETTER, + AnnotationTarget.VALUE_PARAMETER, + AnnotationTarget.TYPE )chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt (1)
22-35: Consider optimizing presence update flow.The implementation correctly handles state management, but there's a potential performance issue in how updates are processed.
Line 30 makes a full
get()call after each flow emission, which might be redundant if the flow event already contains all the information needed. This could cause unnecessary network calls or processing.Consider using the data from the flow event directly if possible, or document why the additional
get()call is necessary:asFlow().collect { if (initialPresenceGet.isActive) initialPresenceGet.cancelAndJoin() - presenceMembers = get() + // If the event already contains the updated list, use it directly + // Otherwise, fetch the latest complete list + presenceMembers = get() // Comment explaining why a fresh get() is needed }example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt (1)
24-24: Reconsider the LongMethod suppressionSince the implementation has been significantly simplified by using the new Flow-based APIs, it may no longer be necessary to suppress the LongMethod warning.
-@Suppress("LongMethod")chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/RoomReactions.kt (1)
19-22: Consider adding a complementary "removeReaction" function.Currently,
addReactiononly increments the counter. If the underlying domain requires decrementing or toggling reactions, you may need an equivalent removal operation.chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/ConnectionTest.kt (4)
19-19: Minor inconsistency in variable declaration.For consistency with other test classes in this module, consider using
private valinstead of justvalfor the connection instance variable.- val connection = EmittingConnection() + private val connection = EmittingConnection()
19-19: Add the private modifier for consistency.The
connectionproperty should be marked asprivatefor better encapsulation and consistency with other test files.- val connection = EmittingConnection() + private val connection = EmittingConnection()
16-34: Add test for automatic unsubscription.Add a test to verify that
statusAsFlow()automatically unsubscribes when the flow completes, similar to what's needed in other test classes.class ConnectionTest { private val connection = EmittingConnection() @Test fun `should return live value for the current connection`() = runTest { // existing test code... } + + @Test + fun `asFlow() should automatically unsubscribe when it's done`() = runTest { + var isUnsubscribed = false + val subscription = Subscription { isUnsubscribed = true } + + val connection = mockk<Connection>() + every { connection.onStatusChange(any()) } returns subscription + + val flow = connection.statusAsFlow() + + // Cancel the flow and check if unsubscription happened + flow.collect { } + + assertTrue(isUnsubscribed) + } }
38-53: Add class documentation.Add documentation to explain the purpose of the
EmittingConnectionclass for improved code readability.+/** + * A test implementation of Connection that allows manually emitting connection status changes. + * Used for testing reactive components that depend on connection status updates. + */ class EmittingConnection(mock: Connection) : Connection by mock { // existing implementation... }chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt (2)
58-59: Consider initializing Mutex with a parameter instead of property initializerThe mutex initialization can be simplified by using the constructor parameter.
- private val mutex = Mutex(locked = false) + private val mutex = Mutex()
69-75: Use withLock pattern for pause/resume functionsConsider using the more idiomatic
withLockpattern instead of directly callinglock()/unlock()for better error handling and consistency with theget()method.- suspend fun pause() { - mutex.lock() - } - - fun resume() { - mutex.unlock() - } + suspend fun pause() { + mutex.lock() + } + + suspend fun resume() { + if (mutex.isLocked) { + mutex.unlock() + } + }chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt (3)
19-19: Add the private modifier for consistency.The
roomproperty should be marked asprivatefor better encapsulation and consistency with other test files.- private val room = EmittingRoom() + private val room = EmittingRoom()
16-34: Add a test for automatic unsubscription.Similar to other test classes in this PR, add a test to verify that
statusAsFlow()automatically unsubscribes when the flow completes.class RoomTest { private val room = EmittingRoom() @Test fun `should return live value for room status change`() = runTest { moleculeFlow(RecompositionMode.Immediate) { room.asComposable() }.test { assertEquals(RoomStatus.Initialized, awaitItem()) room.emit(RoomStatusChange(current = RoomStatus.Attached, previous = RoomStatus.Initialized)) assertEquals(RoomStatus.Attached, awaitItem()) room.emit(RoomStatusChange(current = RoomStatus.Failed, previous = RoomStatus.Attached)) assertEquals(RoomStatus.Failed, awaitItem()) cancel() } } + + @Test + fun `asFlow() should automatically unsubscribe when it's done`() = runTest { + var isUnsubscribed = false + val subscription = Subscription { isUnsubscribed = true } + + val room = mockk<Room>() + every { room.onStatusChange(any()) } returns subscription + + val flow = room.statusAsFlow() + + // Cancel the flow and check if unsubscription happened + flow.collect { } + + assertTrue(isUnsubscribed) + } }
38-53: Add class documentation.Add a doc comment explaining the purpose of the
EmittingRoomclass to improve readability and understanding for other developers.+/** + * A test implementation of Room that allows manually emitting status change events. + * Used for testing reactive components that depend on Room status changes. + */ class EmittingRoom(mock: Room) : Room by mock { private val listeners = mutableListOf<Room.Listener>() override val status: RoomStatus = RoomStatus.Initialized override fun onStatusChange(listener: Room.Listener): Subscription { listeners.add(listener) return Subscription { listeners.remove(listener) } } fun emit(event: RoomStatusChange) { listeners.forEach { it.roomStatusChanged(event) } } }chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt (3)
115-115: Mark parameter as private.The
mockparameter doesn't need to be exposed publicly. Mark it as private to improve encapsulation.-class EmittingPresence(val mock: Presence) : Presence by mock { +class EmittingPresence(private val mock: Presence) : Presence by mock {
20-111: Add test for automatic unsubscription.Add a test to verify that
asFlow()automatically unsubscribes when the flow completes, similar to what's needed in other test classes.class PresenceTest { private val presence = EmittingPresence() @Test fun `should return active presence set`() = runTest { // existing test code... } @Test fun `should cancel getting the initial presence set if event comes faster`() = runTest { // existing test code... } + + @Test + fun `asFlow() should automatically unsubscribe when it's done`() = runTest { + var isUnsubscribed = false + val subscription = Subscription { isUnsubscribed = true } + + val presence = mockk<Presence>() + every { presence.subscribe(any()) } returns subscription + + val flow = presence.asFlow() + + // Cancel the flow and check if unsubscription happened + flow.collect { } + + assertTrue(isUnsubscribed) + } }
115-149: Add class documentation.Add documentation to explain the purpose of the
EmittingPresenceclass to improve code readability.+/** + * A test implementation of Presence that allows manually emitting presence events. + * Uses a mutex to control the flow of presence updates for testing different scenarios. + */ class EmittingPresence(private val mock: Presence) : Presence by mock { // existing implementation... }chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomReactionsTest.kt (3)
19-19: Add the private modifier for consistency.The
roomReactionsproperty should be marked asprivatefor better encapsulation and consistency.- private val roomReactions = EmittingRoomReactions() + private val roomReactions = EmittingRoomReactions()
16-49: Add test for automatic unsubscription.Add a test to verify that
asFlow()automatically unsubscribes when the flow is completed, similar to what's needed in other test classes.class RoomReactionsTest { private val roomReactions = EmittingRoomReactions() @Test fun `should correctly count reactions`() = runTest { // existing test code... } + + @Test + fun `asFlow() should automatically unsubscribe when it's done`() = runTest { + var isUnsubscribed = false + val subscription = Subscription { isUnsubscribed = true } + + val reactions = mockk<RoomReactions>() + every { reactions.subscribe(any()) } returns subscription + + val flow = reactions.asFlow() + + // Cancel the flow and check if unsubscription happened + flow.collect { } + + assertTrue(isUnsubscribed) + } }
53-66: Add class documentation.Add documentation to explain the purpose of the
EmittingRoomReactionsclass for better code readability.+/** + * A test implementation of RoomReactions that allows manually emitting reaction events. + * Used for testing reactive components that depend on reaction updates. + */ class EmittingRoomReactions(mock: RoomReactions) : RoomReactions by mock { // existing implementation... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
chat-android/src/main/java/com/ably/chat/Connection.kt(3 hunks)chat-android/src/main/java/com/ably/chat/FlowUtils.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Occupancy.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Presence.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Room.kt(2 hunks)chat-android/src/main/java/com/ably/chat/RoomReactions.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Typing.kt(3 hunks)chat-android/src/main/java/com/ably/chat/annotations/Annotations.kt(1 hunks)chat-android/src/test/java/com/ably/chat/ConnectionTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/OccupancyTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/PresenceTest.kt(3 hunks)chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/RoomTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/TypingTest.kt(3 hunks)chat-extensions-compose/build.gradle.kts(1 hunks)chat-extensions-compose/gradle.properties(1 hunks)chat-extensions-compose/proguard-rules.pro(1 hunks)chat-extensions-compose/src/main/AndroidManifest.xml(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/RoomReactions.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/ConnectionTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomReactionsTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/TypingTest.kt(1 hunks)example/build.gradle.kts(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(7 hunks)example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt(1 hunks)gradle/libs.versions.toml(3 hunks)settings.gradle.kts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- chat-extensions-compose/src/main/AndroidManifest.xml
- chat-extensions-compose/gradle.properties
- chat-extensions-compose/proguard-rules.pro
🧰 Additional context used
🧠 Learnings (6)
chat-android/src/main/java/com/ably/chat/Typing.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/main/java/com/ably/chat/Typing.kt:178-183
Timestamp: 2024-11-28T11:10:20.947Z
Learning: In `chat-android/src/main/java/com/ably/chat/Typing.kt`, within the `DefaultTyping` class, the `stop()` method must execute within `typingScope` (a `CoroutineScope` with parallelism set to 1) to avoid race conditions when setting and cancelling `typingJob`.
chat-android/src/test/java/com/ably/chat/RoomTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomReactionsTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/OccupancyTest.kt:20-31
Timestamp: 2024-11-28T11:12:06.843Z
Learning: In `OccupancyTest.kt`, additional room state initialization using `setState` is unnecessary for testing occupancy functionality, as the specification doesn't require it.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: coverage
- GitHub Check: check
🔇 Additional comments (70)
chat-android/src/main/java/com/ably/chat/annotations/Annotations.kt (1)
5-9: Clarify the intended audience for this annotation.The KDoc states this API is "internal" and "not intended to be used outside Ably," but experimental APIs are typically intended for external users to try with caution. Consider clarifying whether this is:
- Truly internal (then it may not need
@RequiresOptIn)- Experimental but available for users (then update the documentation)
settings.gradle.kts (1)
5-5: New module added to support Compose integration.The addition of the
chat-extensions-composemodule aligns with the PR objective to add Jetpack Compose support to the Ably Chat Kotlin library.chat-android/src/main/java/com/ably/chat/RoomReactions.kt (2)
11-11: Added Flow support for reactive programming.The import of Kotlin Flow aligns with the PR objective to support reactive programming.
64-69: Flow extension function implementation looks good.The implementation follows a clean and consistent pattern for transforming callback-based APIs into Flow-based APIs, which matches the PR objective.
chat-android/src/main/java/com/ably/chat/Typing.kt (3)
22-22: Added Flow support for reactive programming.The import of Kotlin Flow aligns with the PR objective to support reactive programming.
92-97: Flow extension function implementation looks good.The implementation follows a clean and consistent pattern for transforming callback-based APIs into Flow-based APIs, which matches the PR objective.
192-192: Good fix for potential memory leak.Setting
typingJobto null after cancellation ensures proper cleanup of resources and prevents potential memory leaks. This also helps address potential race conditions when setting and cancellingtypingJobas noted in the retrieved learning.chat-android/src/main/java/com/ably/chat/Messages.kt (2)
12-12: Added Flow support for reactive programming.The import of Kotlin Flow aligns with the PR objective to support reactive programming.
125-130: Flow extension function implementation looks good.The implementation follows a clean and consistent pattern for transforming callback-based APIs into Flow-based APIs, which matches the PR objective.
chat-android/src/main/java/com/ably/chat/Room.kt (2)
10-10: Import addition for Flow APIThe import for
kotlinx.coroutines.flow.Flowsupports the new Flow-based API being introduced.
118-123: New function to expose Room status events as a FlowThis extension function provides a reactive way to observe room status changes, aligning with Kotlin's coroutines and Flow API standards. The implementation correctly leverages the existing callback-based API and properly handles subscription lifecycle.
chat-android/src/main/java/com/ably/chat/Presence.kt (2)
11-11: Import addition for Flow APIThe import for
kotlinx.coroutines.flow.Flowsupports the new Flow-based API being introduced.
85-90: New function to expose presence events as a FlowThis extension function provides a reactive way to observe presence events using Kotlin's Flow API. The implementation correctly transforms the callback-based subscription into a Flow and ensures proper resource cleanup through the underlying
transformCallbackAsFlowutility.chat-android/src/main/java/com/ably/chat/FlowUtils.kt (1)
1-23: Well-implemented utility for converting callbacks to flowsThis utility function elegantly transforms callback-based APIs into Kotlin Flows. The implementation includes several important aspects:
- Proper documentation explaining its purpose
- Use of
Channel.UNLIMITEDto handle backpressure without dropping events- Correct resource cleanup in the
finallyblock to prevent subscription leaks- Generic type parameter allowing reuse across different event types
The implementation aligns with reactive programming best practices and enables a consistent pattern for Flow conversion throughout the codebase.
chat-android/src/main/java/com/ably/chat/Occupancy.kt (2)
15-15: Import addition for Flow APIThe import for
kotlinx.coroutines.flow.Flowsupports the new Flow-based API being introduced.
59-64: New function to expose occupancy events as a FlowThis extension function provides a reactive way to observe occupancy events using Kotlin's Flow API. The implementation correctly transforms the callback-based subscription into a Flow, consistent with other reactive interfaces in the library.
chat-android/src/main/java/com/ably/chat/Connection.kt (3)
13-13: New Flow import added for reactive programming supportThe addition of the Flow import enables the new reactive programming features being added to the library.
75-75: Good addition of default values to simplify API usageAdding default null values to these optional parameters is a good improvement that simplifies the API usage. This change allows consumers to omit these parameters when they're not needed, making the code more concise.
Also applies to: 80-80
116-121: Great addition of Flow-based APIThe new
statusAsFlow()extension function provides a clean, reactive way to consume connection status changes. This aligns with modern Kotlin development practices and will significantly improve the developer experience when working with state changes.The function properly leverages the existing callback mechanism while providing a more composable, reactive alternative.
example/build.gradle.kts (1)
62-62: Correctly integrated new Compose extensions moduleAdding the new
:chat-extensions-composemodule as a dependency to the example project is necessary to demonstrate and test the new Compose integration features.chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
1-28: Well-implemented Compose integration for Room statusThis implementation follows Jetpack Compose best practices:
- Properly marked as experimental with
@ExperimentalChatApi- Uses
rememberwith the room instance as key to avoid recreating state unnecessarily- Correctly uses
LaunchedEffectwith the room instance as key to manage the lifecycle of the Flow collection- Efficiently updates and returns room status
The implementation will automatically recompose UI elements when room status changes, making it very easy to build reactive UIs with the Ably Chat library.
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt (1)
1-28: Well-implemented Compose integration for typing indicatorsThis implementation follows the same solid pattern as the Room composable:
- Properly marked as experimental with
@ExperimentalChatApi- Uses
rememberwith the typing instance as key for state stability- Correctly uses
LaunchedEffectto manage Flow collection lifecycle- Returns a reactive set of currently typing users
The code is clean, focused, and will efficiently update the UI when typing status changes. This is a valuable addition for chat UIs that need to show typing indicators.
chat-android/src/test/java/com/ably/chat/OccupancyTest.kt (1)
156-177: Well-structured test for Flow API integration!This test effectively verifies that the
asFlow()extension function properly manages the subscription lifecycle by unsubscribing when the Flow is canceled. The approach using Turbine for testing flows is clean and follows best practices.chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt (1)
17-27: Good implementation of Compose integrationThe implementation follows Jetpack Compose best practices:
- Properly annotated with
@ExperimentalChatApito indicate experimental status- Using
rememberwith the connection instance as a key to prevent unnecessary state recreation- Using
LaunchedEffectto manage flow subscription within the composition lifecycle- Clear return type and documentation
chat-android/src/test/java/com/ably/chat/TypingTest.kt (1)
139-160: Consistent test approach for Flow APIThis test maintains consistency with the testing approach used in OccupancyTest, properly verifying that the
asFlow()extension function automatically unsubscribes when the flow is canceled. This consistent pattern across different components demonstrates good testing practices.chat-android/src/test/java/com/ably/chat/RoomTest.kt (1)
11-35: Good test coverage for Room status flowThe test effectively verifies the automatic unsubscription behavior of
statusAsFlow(). This is important to prevent memory leaks and ensure proper resource cleanup. The test structure follows the same pattern as in other components, which is good for maintainability.chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (4)
1-14: Well-structured imports and package organization.The imports are well-organized and include all necessary components for Compose integration with proper separation between core Android, Compose, and Ably Chat imports.
15-18: Good use of data class with default values.The
CurrentOccupancydata class is appropriately defined with default values, making it easy to instantiate and use in Compose UIs.
23-25: Appropriate annotation usage.The
@ExperimentalChatApiannotation clearly indicates this is an experimental feature, and the@Composableannotation properly marks this function for use within Compose UI.
26-46: Well-implemented Compose state management.The implementation properly manages state through:
- Using
rememberwith the correct key to reset state when the Occupancy instance changes- Proper coroutine cancellation with
cancelAndJoinwhen new data arrives- Using
LaunchedEffectscoped to the Occupancy instance for proper lifecycle managementThis ensures that the occupancy data stays up-to-date and prevents memory leaks.
chat-extensions-compose/build.gradle.kts (5)
1-7: Appropriate plugins for Compose module.The selection of plugins is appropriate for a Compose-enabled Android library module, including the necessary Compose compiler plugin.
11-32: Properly configured Android library setup.The Android configuration is well-structured with:
- Appropriate namespace for Compose extensions
- Modern compileSdk (34) with reasonable minSdk (24)
- Properly enabled Compose features
- ConsumerProguardFiles for library consumption
This ensures good compatibility while maintaining a modern feature set.
34-48: Compatible Java and Kotlin configuration.The Java and Kotlin version configurations are aligned at 1.8, which is appropriate for maintaining compatibility with a wide range of Android versions while enabling modern language features.
51-53: Good use of Kotlin explicit API mode.The
explicitApi()configuration enhances API clarity and forces developers to be intentional about public API surfaces.
55-64: Comprehensive dependency setup.The dependencies include all necessary components for Compose development and testing:
- Base chat-android module dependency
- Core Compose libraries
- Comprehensive testing libraries (JUnit, MockK, Turbine, etc.)
This ensures the module can be properly developed and tested.
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt (2)
1-15: Well-structured imports and package organization.The imports are well-organized and include all necessary components for Compose integration.
19-21: Appropriate annotation usage.The experimental API is properly annotated, making it clear to developers that this API might change in future releases.
chat-android/src/test/java/com/ably/chat/ConnectionTest.kt (2)
3-13: Appropriate test imports added.The new imports support the flow testing needs with Turbine and MockK.
151-172: Well-structured flow unsubscription test.The test effectively verifies that the flow automatically unsubscribes when canceled:
- Properly mocks the Connection and Subscription
- Captures the callback for verification
- Uses Turbine for testing flow emissions
- Verifies the unsubscription behavior with exact verification count
This ensures that resources are properly cleaned up when flows are canceled, preventing potential memory leaks.
chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt (1)
95-116: Good test coverage for the Flow APIThis test properly verifies that when using
asFlow()and canceling the Flow, the underlying subscription is automatically unsubscribed, preventing memory leaks and unnecessary resource consumption.Consider renaming the test to "asFlow() should automatically unsubscribe when it's done" (changing "then" to "when") for better clarity.
chat-android/src/test/java/com/ably/chat/MessagesTest.kt (2)
3-3: Good addition of turbine for Flow testingThe turbine library is the appropriate choice for testing Flow-based APIs.
240-261: Good test coverage for the Messages Flow APIThis test correctly verifies the automatic unsubscription behavior when a Flow is canceled, following a consistent pattern with other similar tests in the codebase.
Consider renaming the test to "asFlow() should automatically unsubscribe when it's done" (changing "then" to "when") for better clarity.
chat-android/src/test/java/com/ably/chat/PresenceTest.kt (1)
150-171: Good test coverage for the Presence Flow APIThis test correctly verifies that the Flow properly handles subscription lifecycle, ensuring resources are cleaned up when the Flow is canceled.
Consider renaming the test to "asFlow() should automatically unsubscribe when it's done" (changing "then" to "when") for better clarity.
example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt (2)
18-21: Good addition of necessary imports for Flow & Compose integrationThe new imports support the changes to use Compose-specific extensions and the experimental API annotation.
25-31: Excellent simplification using the new Compose extensionsThe refactoring significantly simplifies the implementation by:
- Changing the parameter from
ChatClienttoRoomfor more direct access- Using the new
asComposable()extension to automatically handle presence state updates- Properly marking usage of experimental APIs with
@OptInThis is a great example of how the new Flow and Compose support improves code quality.
gradle/libs.versions.toml (1)
17-17: Dependency updates look good.All newly introduced library references (Compose Foundation, Molecule, and Android-specific MockK) appear consistent with project requirements. If you haven’t already, consider verifying that these versions are stable or compatible with your Android Gradle plugin before finalizing.
Also applies to: 23-23, 40-40, 53-53, 61-61
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/RoomReactions.kt (2)
14-17: Data class definitions look solid.Default values for
ReactionCounterfields provide flexibility and simplify instantiation.
30-39: Validate map-based state updates for concurrency and removal scenarios.
- Each new reaction results in a newly copied map, which is typically acceptable in Compose but can be expensive if reaction volume is high.
- There's no removal or decrement logic if a reaction is retracted. Confirm that this one-sided incrementation aligns with your app’s requirements.
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (2)
74-79: Interface structure is clear.The
PaginatedMessagesinterface straightforwardly exposes loaded data, scrolling state, loading status, and pagination info.
81-86: Implementation ofPaginatedMessagesis clean and straightforward.Returning a simple data class ensures immutability and easy usage in Compose contexts.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt (1)
1-54: Well-structured test for Room's Compose integration!The test effectively verifies that room status changes are properly reflected in Compose. The
EmittingRoommock implementation follows good practices for testing reactive streams.A few observations:
- Good use of the Molecule library for testing composable state updates
- Clean implementation of the test double with proper listener management
- Appropriate use of
@OptIn(ExperimentalChatApi::class)annotation to acknowledge the experimental nature of the APIThe test coverage is focused on the "happy path" but adequately verifies the fundamental functionality.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt (1)
1-150: Strong implementation of Presence testing with Compose!The
PresenceTestclass provides thorough testing of presence functionality in a Compose context, with two well-structured test cases that cover both normal operation and an edge case (when events arrive before initial presence set is loaded).I particularly appreciate:
- The use of
Mutexfor thread safety in the test implementation- The comprehensive testing of presence state changes with detailed assertions
- The pause/resume functionality to simulate asynchronous behavior scenarios
This is a strong example of how to test reactive state management with Compose.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomReactionsTest.kt (1)
1-67: Good implementation of reaction counting for Compose integration!The test effectively verifies that reaction events are correctly counted and reflect the appropriate state in Compose. The implementation correctly handles:
- Initial empty state
- Updating counts when new reactions arrive
- Tracking whether the current user has reacted
This test properly verifies the core functionality needed for displaying reaction counts in a UI.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/ConnectionTest.kt (1)
1-54: Well-structured test for Connection's Compose integration!The test effectively verifies that connection status changes are properly reflected in Compose UI components. The approach follows the same pattern as other Compose integration tests, which maintains consistency across the codebase.
The test progression through different connection states (Initialized → Connecting → Connected) provides good coverage of the typical connection lifecycle.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt (4)
4-5: Good use of Molecule for testing Composable stateUsing Molecule's
moleculeFlowfunction is an excellent choice for testing reactive Composable state transformations, as it allows for controlled testing of state changes over time.
17-18: Appropriate use of ExperimentalChatApi annotationUsing the OptIn annotation for ExperimentalChatApi correctly signals to consumers that they're using APIs that might change.
21-34: Well-structured test with clear assertionsThis test effectively verifies the reactive behavior of occupancy updates through the Composable API. The sequential emission and verification pattern makes the test intent clear.
36-50: Good edge case testingTesting the cancellation of initial occupancy retrieval when a new event arrives demonstrates thorough consideration of concurrent scenarios that might occur in real applications.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/TypingTest.kt (4)
15-19: Consistent experimental API opt-in approachThe use of
@OptIn(ExperimentalChatApi::class)matches the pattern seen in other files, maintaining consistency across the codebase.
20-32: Clear test structure with explicit state verificationThis test effectively validates the reactive typing updates through the Composable API, with clear assertions at each step of the state evolution.
37-43: Clean delegate pattern implementationGood use of the Kotlin delegation pattern (
by mock) combined with overriding specific methods for the test fixture. This keeps the test class focused and minimal.
45-49:❓ Verification inconclusive
Consider adding a missing initial state
The
EmittingTypingclass doesn't have an initial state likeEmittingOccupancydoes. Consider adding one for consistency or documenting why it's not needed.
🏁 Script executed:
#!/bin/bash # Let's check if the Typing interface has a get() method for initial state like Occupancy does rg -A 5 "interface Typing" || echo "Typing interface not found"Length of output: 553
Action: Evaluate the need for an initial state in EmittingTyping
The current implementation in
TypingTest.kt(lines 45–49) properly emits typing events, but note that unlikeEmittingOccupancy, there isn’t any provision for an initial state. Our verification confirms that theTypinginterface (inchat-android/src/main/java/com/ably/chat/Typing.kt) only defines achannelproperty and does not offer a method or property to retrieve an initial state. For consistency across event types, please consider either:
- Adding a default initial state to
EmittingTyping, similar to what exists for occupancy events, or- Documenting explicitly why an initial state isn’t applicable for typing events.
example/src/main/java/com/ably/chat/example/MainActivity.kt (8)
47-49: Good import organization for new functionalityThe imports clearly reflect the addition of Flow and Compose support, with appropriate imports for experimental annotations and the new extension functions.
Also applies to: 53-54, 56-57
77-77: Improved configuration with explicit log levelUsing the new
ChatClientOptionswith an explicit log level improves code clarity and makes configuration intent more obvious.
92-93: Smart use of safe call with elvis operator for nullability handlingThe
?.safe call operator with the?:elvis operator provides a clean way to handle the initially null room state while preventing null pointer exceptions.
95-102: Improved lifecycle management with LaunchedEffectReplacing (presumably) DisposableEffect with LaunchedEffect simplifies the room lifecycle management. The coroutine-based approach is more idiomatic for Kotlin and Jetpack Compose.
131-133: Consistent nullability checksBoth UI sections correctly use the
room?.let { ... }pattern to ensure null safety when working with the room object.Also applies to: 136-140
153-154: Effective use of Compose extensions for reactive dataUsing
asComposable()extensions for messages, reactions, and typing provides a clean reactive programming model that integrates well with Compose's state management.
187-188: Proper integration with pagination stateThe integration with
paginatedMessages.listStateand the use ofpaginatedMessages.loadedfor accessing messages shows a good understanding of the pagination APIs.Also applies to: 189-195
229-232: Improved reaction formatting displayThe formatting of received reactions now includes counts, which provides more valuable information to users.
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt
Outdated
Show resolved
Hide resolved
ttypic
force-pushed
the
CHA-801/flow-and-compose
branch
from
328be26 to
f8dce90
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (1)
52-59: 🛠️ Refactor suggestionAdd error handling for
getPreviousMessages()failures.If fetching previous messages fails, the UI remains in the loading state without displaying any errors. Provide a retry option or user notification to improve user experience.
52 coroutineScope.launch { 53 loading = true - val receivedPaginatedResult = subscription.getPreviousMessages() - lastReceivedPaginatedResult = receivedPaginatedResult - loaded = receivedPaginatedResult.items.reversed() + loaded - loading = false - if (loaded.isNotEmpty()) listState.animateScrollToItem(loaded.size - 1) + try { + val receivedPaginatedResult = subscription.getPreviousMessages() + lastReceivedPaginatedResult = receivedPaginatedResult + loaded = receivedPaginatedResult.items.reversed() + loaded + } catch (e: Exception) { + // Handle error, e.g., logging or presenting a retry option + } finally { + loading = false + } + if (loaded.isNotEmpty()) { + listState.animateScrollToItem(loaded.size - 1) + }
🧹 Nitpick comments (14)
example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt (1)
28-28: Simplified presence subscription with reactive paradigm.The use of
asComposable()elegantly replaces what was likely complex subscription management code. This declarative approach aligns perfectly with Compose's reactive paradigm.Consider adding a comment explaining that this automatically handles subscription lifecycle with Compose, which might help other developers understand this pattern.
example/src/main/java/com/ably/chat/example/MainActivity.kt (2)
95-102: Improved lifecycle management with LaunchedEffectReplacing DisposableEffect with LaunchedEffect simplifies the room attachment logic. However, consider adding error handling for the attachment process.
LaunchedEffect(Unit) { - val chatRoom = chatClient.rooms.get( - Settings.ROOM_ID, - RoomOptions.default, - ) - chatRoom.attach() - room = chatRoom + try { + val chatRoom = chatClient.rooms.get( + Settings.ROOM_ID, + RoomOptions.default, + ) + chatRoom.attach() + room = chatRoom + } catch (e: Exception) { + // Handle error (e.g., log, show error message) + println("Failed to attach to room: ${e.message}") + } }
228-233: Enhanced reaction display with count informationThe formatting of received reactions now includes counts, providing more useful information to users.
Consider extracting this formatting logic into a separate function for better readability:
- Text( - "Received reactions: ${receivedReactions.map { "${it.key} - ${it.value.count}" }.joinToString()}", - modifier = Modifier.padding(16.dp), - ) + Text( + "Received reactions: ${formatReactions(receivedReactions)}", + modifier = Modifier.padding(16.dp), + ) // Add this function outside the composable + private fun formatReactions(reactions: Map<String, ReactionSummary>) = + reactions.map { "${it.key} - ${it.value.count}" }.joinToString()chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (3)
36-36: Avoid unbounded growth inloadedmessages.Repeatedly appending messages to
loadedcan lead to unbounded memory usage if old messages are never pruned. Consider adding periodic cleanup or pagination boundaries to maintain a manageable list size over time.
58-58: Consider user experience before forcibly scrolling to the last item.Automatically scrolling to the bottom on every addition might interrupt users reading older messages. Consider conditionally scrolling only when the user is already near the end or when they desire to jump to the latest message.
66-72: Implement a "load more" mechanism withhasMore.You’re exposing
hasMorebut not providing a function to load additional messages when this flag is true. Add a method to gracefully load older messages once the user scrolls up or triggers a "Load More" action.chat-android/src/test/java/com/ably/chat/PresenceTest.kt (1)
150-171: Typo in test name - "then" should be "when".The test correctly verifies that flows automatically unsubscribe when completed, which is crucial for preventing memory leaks. However, there's a small typo in the function name:
asFlow() should automatically unsubscribe then it's doneshould beasFlow() should automatically unsubscribe when it's done.-fun `asFlow() should automatically unsubscribe then it's done`() = runTest { +fun `asFlow() should automatically unsubscribe when it's done`() = runTest {chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt (2)
125-135: Consider adding timeout to mutex lock operation.The mutex lock in the
getmethod could potentially cause indefinite blocking if the mutex is locked and never unlocked. Consider adding a timeout parameter to thewithLockcall to prevent potential deadlocks in tests.-override suspend fun get(waitForSync: Boolean, clientId: String?, connectionId: String?): List<PresenceMember> = mutex.withLock { +override suspend fun get(waitForSync: Boolean, clientId: String?, connectionId: String?): List<PresenceMember> = mutex.withLock(timeoutMillis = 5000) {
137-148: Consider clarifying the emit method name.The
emitmethod name is somewhat generic. Consider renaming it to be more specific about what type of event is being emitted, such asemitPresenceEventto improve code readability and maintainability.-fun emit(event: PresenceEvent) { +fun emitPresenceEvent(event: PresenceEvent) {Remember to update all call sites accordingly.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/ConnectionTest.kt (1)
48-52: Consider clarifying the emit method name.Similar to the PresenceTest, the
emitmethod name is generic. Consider renaming it to be more specific about what type of event is being emitted, such asemitStatusChangeto improve code readability.-fun emit(event: ConnectionStatusChange) { +fun emitStatusChange(event: ConnectionStatusChange) {Remember to update all call sites accordingly.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt (2)
48-52: Consider clarifying the emit method name.As with the other test classes, the
emitmethod name could be more specific. Consider renaming it toemitStatusChangefor consistency and improved code readability.-fun emit(event: RoomStatusChange) { +fun emitStatusChange(event: RoomStatusChange) {Remember to update all call sites accordingly.
36-37: Consider adding documentation to factory function.The factory function
EmittingRoom()would benefit from KDoc documentation explaining its purpose and usage in tests.+/** + * Creates an instance of EmittingRoom with a mock Room implementation. + * Used for testing room status changes in a composable context. + */ fun EmittingRoom() = EmittingRoom(mockk())chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/TypingTest.kt (2)
37-50: Add auto-unsubscription test for consistencyThe
EmittingTypingimplementation is correct, but unlike what's mentioned in the AI summary, there's no test to verify that flows automatically unsubscribe when they're done.For consistency with other tests (like the one mentioned for Occupancy), consider adding an explicit test for this behavior.
+ @Test + fun `asFlow() should automatically unsubscribe then it's done`() = runTest { + val typing = mockk<Typing>() + val subscription = mockk<Subscription>() + val listener = slot<Typing.Listener>() + val typingEvent = TypingEvent(setOf("client_1")) + + every { typing.subscribe(capture(listener)) } answers { + subscription + } + every { subscription.unsubscribe() } just runs + + val flow = typing.asFlow() + flow.test { + listener.captured.onEvent(typingEvent) + assertEquals(typingEvent, awaitItem()) + cancel() + } + + verify(exactly = 1) { subscription.unsubscribe() } + }
37-50: Consider adding thread safety for consistencyUnlike
EmittingOccupancy, this class doesn't use a Mutex for thread safety. While it may not be necessary for these specific tests, adding it would provide consistency between the test implementations.Consider adding a mutex similar to the OccupancyTest implementation if thread safety might be a concern, especially if these tests could run in parallel.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
chat-android/src/main/java/com/ably/chat/Connection.kt(3 hunks)chat-android/src/main/java/com/ably/chat/FlowUtils.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Occupancy.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Presence.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Room.kt(2 hunks)chat-android/src/main/java/com/ably/chat/RoomReactions.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Typing.kt(3 hunks)chat-android/src/main/java/com/ably/chat/annotations/Annotations.kt(1 hunks)chat-android/src/test/java/com/ably/chat/ConnectionTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/OccupancyTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/PresenceTest.kt(3 hunks)chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/RoomTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/TypingTest.kt(3 hunks)chat-extensions-compose/build.gradle.kts(1 hunks)chat-extensions-compose/gradle.properties(1 hunks)chat-extensions-compose/proguard-rules.pro(1 hunks)chat-extensions-compose/src/main/AndroidManifest.xml(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/RoomReactions.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/ConnectionTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomReactionsTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/TypingTest.kt(1 hunks)example/build.gradle.kts(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(7 hunks)example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt(1 hunks)gradle/libs.versions.toml(3 hunks)settings.gradle.kts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (25)
- chat-extensions-compose/src/main/AndroidManifest.xml
- chat-extensions-compose/proguard-rules.pro
- chat-android/src/main/java/com/ably/chat/RoomReactions.kt
- chat-android/src/main/java/com/ably/chat/Messages.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt
- settings.gradle.kts
- chat-android/src/test/java/com/ably/chat/TypingTest.kt
- chat-android/src/main/java/com/ably/chat/Presence.kt
- chat-android/src/main/java/com/ably/chat/Typing.kt
- chat-android/src/main/java/com/ably/chat/Room.kt
- chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt
- chat-extensions-compose/build.gradle.kts
- chat-extensions-compose/gradle.properties
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt
- example/build.gradle.kts
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/RoomReactions.kt
- chat-android/src/main/java/com/ably/chat/Connection.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt
- chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomReactionsTest.kt
- gradle/libs.versions.toml
- chat-android/src/main/java/com/ably/chat/annotations/Annotations.kt
- chat-android/src/test/java/com/ably/chat/MessagesTest.kt
- chat-android/src/main/java/com/ably/chat/FlowUtils.kt
🧰 Additional context used
🧠 Learnings (3)
chat-android/src/test/java/com/ably/chat/RoomTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/OccupancyTest.kt:20-31
Timestamp: 2024-11-28T11:12:06.843Z
Learning: In `OccupancyTest.kt`, additional room state initialization using `setState` is unnecessary for testing occupancy functionality, as the specification doesn't require it.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: check
- GitHub Check: coverage
- GitHub Check: build
🔇 Additional comments (33)
example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt (5)
18-20: Good integration of Compose extensions.The addition of these imports shows proper integration with the new Compose extensions module. The explicit import of the experimental API annotation is good practice for awareness.
25-25: Appropriate use of OptIn annotation.Using the
@OptInannotation is the correct approach when working with experimental APIs, as it acknowledges the potential for API changes in future releases.
27-27: Improved parameter specificity.Changing from
chatClienttoroomparameter is a good design decision. It provides a more specific scope for the component, better following the principle of least privilege by only providing access to what's needed.
30-30: Direct access to presence is consistent with the new parameter.This change correctly aligns with the parameter update from
chatClienttoroom.
18-87: Verify end-to-end function compatibility.The refactoring looks solid, but it would be good to verify that the functionality remains the same with the new Flow-based implementation, especially regarding:
- Initial loading of presence data
- Reactive updates when members join/leave/update status
- Proper cleanup of resources when the component is disposed
Can you confirm this component has been manually tested in the sample app to ensure the presence functionality works as expected with the new Flow-based implementation?
example/src/main/java/com/ably/chat/example/MainActivity.kt (10)
34-35: Great job updating the imports for the new Flow and Compose supportThe imports appropriately include the new
ExperimentalChatApiannotation and theasComposableextension function, which are central to the Jetpack Compose integration.Also applies to: 47-49, 53-54, 56-57
77-78: Improved ChatClient initialization with LogLevel configurationAdding the
ChatClientOptionswith explicit log level is a good practice for the example app, making it clearer to users how to configure logging.
88-89: Appropriate usage of OptIn annotation for experimental APIThe
@OptIn(ExperimentalChatApi::class)annotation correctly signals to users that the new Compose integration APIs are experimental.
92-94: Clean implementation of typing indicator with Flow conversionGood use of the new
asComposable()extension for the typing indicator, with appropriate null-safety handling using the Elvis operator.
131-134: Cleaner room handling with null safetyThe room null safety check with the let scope function ensures UI components are only rendered when the room is available.
136-140: Updated presence popup to use room directlyPassing the room object directly to the PresencePopup instead of chatClient aligns with the new approach of having component-specific data management.
145-146: Properly marked Chat composable with experimental annotationThe experimental annotation makes it clear to users that the Compose integration APIs are still under development.
150-155: Excellent use of Flow-based APIs for messages and reactionsThe implementation properly leverages the new Flow-based APIs through the
asComposable()extensions, which simplifies state management and reactivity.
187-188: Smart integration with Compose's LazyColumn stateUsing the paginated messages list state directly provides automatic scrolling behavior, a great example of the benefits of the new integration.
189-195: Simplified message iteration with pagination supportThe new approach properly handles the paginated message list, making it easy to display messages with their indices.
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (1)
33-50: Be mindful of re-subscription behavior during recomposition.When the composable is recomposed with a new
Messagesinstance, this disposable effect will re-subscribe. Verify that this behavior is intentional and that previous subscriptions do not accumulate or conflict.chat-android/src/main/java/com/ably/chat/Occupancy.kt (2)
15-15: LGTM: Added Flow import for new asFlow extension functionThis import is necessary for the newly added flow functionality, consistent with the PR objective to add Flow support across callback-based subscriptions.
59-64: Well-implemented Flow transformation for Occupancy eventsThe
asFlow()extension function provides a clean reactive programming interface for the Occupancy events, transforming the callback-based subscription model into Kotlin's Flow API. This implementation aligns perfectly with the PR's goal to support Kotlin Flow across callback-based subscriptions.The documentation is clear and concise, though consider adding a small usage example in the future for developers unfamiliar with Flow APIs.
chat-android/src/test/java/com/ably/chat/OccupancyTest.kt (2)
3-3: LGTM: Added turbine test import for Flow testingThis import supports the new test case for the asFlow functionality.
156-177: LGTM: Good test coverage for automatic unsubscription behaviorThis test properly verifies that the Flow implementation automatically unsubscribes when the flow is canceled, preventing potential memory leaks and resource issues. The test structure is clean and follows the established patterns in the codebase.
chat-android/src/test/java/com/ably/chat/RoomTest.kt (1)
1-36: LGTM: Well-structured test class for Room statusAsFlow functionalityThis new test class follows the same pattern as other Flow-related tests in the codebase. It properly verifies that:
- The statusAsFlow function correctly emits room status changes
- The subscription is automatically unsubscribed when the flow is canceled
This test is important for ensuring proper resource management when using the reactive Flow-based API for room status updates.
chat-android/src/test/java/com/ably/chat/ConnectionTest.kt (2)
3-3: LGTM: Added necessary imports for Flow testingThese imports support the new test case for the statusAsFlow functionality.
Also applies to: 10-10, 13-13
151-172: LGTM: Consistent test implementation for connection status FlowThis test properly verifies that the Flow implementation automatically unsubscribes when the flow is canceled, maintaining the pattern established in other components. The test structure is clean and follows the established patterns in the codebase.
chat-android/src/test/java/com/ably/chat/PresenceTest.kt (1)
3-16: Good addition of necessary imports.The imports are correctly added to support the new flow testing functionality, particularly
app.cash.turbine.testfor flow testing andio.mockk.verifyfor verification.chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt (2)
20-111: Well-structured tests for Compose integration.The tests effectively verify that
asComposable()correctly transforms presence events into a reactive state that can be observed in Jetpack Compose. The use ofmoleculeFlowwithRecompositionMode.Immediateis appropriate for testing composable state, and the tests cover both the happy path and a potential race condition.
129-135: Risk of deadlock with manual mutex operations.The
pause()andresume()methods directly calllock()andunlock()on the mutex. This approach is risky as it could lead to deadlocks if not used carefully, especially in test scenarios. Consider using a more structured approach such as aChannelor a flow-based solution that doesn't require manual mutex handling.chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/ConnectionTest.kt (1)
16-33: Good test coverage for connection status in Compose.The test properly verifies that the Connection status is correctly observed in a composable context, with appropriate assertions for each state transition. The use of
moleculeFlowwithRecompositionMode.Immediateensures immediate updates when testing.chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt (1)
16-33: Good test coverage for room status in Compose.The test effectively verifies that the Room status changes are correctly observed in a composable context, with appropriate assertions for each state transition.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt (3)
17-20: Good use of ExperimentalChatApi annotationThe class is properly annotated with
@OptIn(ExperimentalChatApi::class)to acknowledge the experimental API usage.
21-34: Test logic looks goodThe test correctly verifies that occupancy events are emitted and processed appropriately in the composable state. Good use of moleculeFlow and the testing pattern looks solid.
36-50: Good test for cancellation behaviorThis test effectively verifies that the composable handles the cancellation of initial occupancy retrieval when events arrive faster. The pause/resume mechanism provides good control over the test flow.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/TypingTest.kt (2)
15-19: Good use of ExperimentalChatApi annotationThe class is properly annotated with
@OptIn(ExperimentalChatApi::class)to acknowledge the experimental API usage.
20-32: Thorough testing of typing updatesThe test effectively verifies that typing events are correctly propagated to the composable state. The assertions properly check both the initial empty state and subsequent updates.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt
Show resolved
Hide resolved
|
I am going through the related DR and related implementation to understand context properly. It will take bit of time, but once we agree on this, we don't need to worry about future flow related issues. |
chat-android/src/main/java/com/ably/chat/annotations/Annotations.kt
Outdated
Show resolved
Hide resolved
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt
Outdated
Show resolved
Hide resolved
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt
Outdated
Show resolved
Hide resolved
| */ | ||
| @ExperimentalChatApi | ||
| @Composable | ||
| public fun Messages.asComposable(): PaginatedMessages { |
There was a problem hiding this comment.
It looks like this component doesn't handle discontinuity in messages. Is this something we can easily add?
There was a problem hiding this comment.
Added discontinuity support
|
|
||
| coroutineScope.launch { | ||
| loading = true | ||
| val receivedPaginatedResult = subscription.getPreviousMessages() |
There was a problem hiding this comment.
Users should probably be able to specify how far back they want to go with a getPreviousMessages call
There was a problem hiding this comment.
It's connected to the scroll state, it will get previous message if mobile user scrolling, sdk users can specify page size
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt
Show resolved
Hide resolved
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt
Outdated
Show resolved
Hide resolved
| */ | ||
| @ExperimentalChatApi | ||
| @Composable | ||
| public fun RoomReactions.asComposable(): Map<String, ReactionCounter> { |
There was a problem hiding this comment.
Perhaps on this one, we should keep it to just ephemeral events for now - we've got a whole feature in the works for counted reactions?
There was a problem hiding this comment.
it makes sense, removed this extension for now
Added `kotlinx.coroutines.flow.Flow` support for all callback-based subscription
ttypic
force-pushed
the
CHA-801/flow-and-compose
branch
from
f8dce90 to
1ce91a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt (1)
1-157: 💡 Verification agent🧩 Analysis chain
Missing test for unsubscription behavior.
According to the AI summary, there should be a test for the
asFlow()function that verifies automatic unsubscription, but this test is missing from the implementation.
🏁 Script executed:
#!/bin/bash # Search for any implementation of asFlow() function or any files mentioning unsubscription testing echo "Searching for asFlow() implementation..." rg -t kotlin "fun .*asFlow\(\)" --glob "!**/build/**" echo "Searching for any unsubscription tests..." rg -t kotlin "unsubscribe.*test|test.*unsubscribe" --glob "!**/build/**"Length of output: 1664
Action Required: Add Automatic Unsubscription Test for asFlow()
The current test in
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.ktdoes not verify that theasFlow()functionality automatically unsubscribes after collection completes. Notably, similar unsubscription tests exist in thechat-androidmodule (e.g., inchat-android/src/test/java/com/ably/chat/PresenceTest.kt), which can serve as a reference.
- Add a test case to verify that when using the
asFlow()function (or its equivalent in the compose extension), the flow properly unsubscribes once it's done.- Refer to existing tests in the
chat-androidmodule (like the test namedasFlow() should automatically unsubscribe then it's done) to guide the implementation.
🧹 Nitpick comments (20)
chat-android/src/main/java/com/ably/chat/Discontinuities.kt (1)
46-48: Fix KDoc documentation inconsistency.The KDoc mentions returning
[ConnectionStatusChange] events as a [Flow], but the actual method returnsFlow<ErrorInfo?>. This appears to be a copy-paste error from another similar extension function.-/** - * @return [ConnectionStatusChange] events as a [Flow] - */ +/** + * @return discontinuity events as a [Flow] of [ErrorInfo] + */chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt (2)
84-89: Consider adding mutex protection for currentOccupancyEvent updatesThe
emitmethod updatescurrentOccupancyEventwithout mutex protection, which could potentially lead to race conditions in a concurrent environment. Consider wrapping this operation withmutex.withLockfor consistency with the rest of the implementation.-fun emit(event: OccupancyEvent) { - currentOccupancyEvent = event - listeners.forEach { - it.onEvent(event) - } -} +suspend fun emit(event: OccupancyEvent) { + mutex.withLock { + currentOccupancyEvent = event + } + listeners.forEach { + it.onEvent(event) + } +}
62-62: Add documentation to the EmittingOccupancy classConsider adding KDoc comments to explain the purpose and usage of this test helper class and its methods to improve maintainability and readability.
+/** + * A test implementation of the Occupancy interface that allows controlling + * occupancy events emission and timing for testing purposes. + */ class EmittingOccupancy(val mock: Occupancy) : Occupancy by mock {chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt (2)
14-16: Enhance documentation for this experimental APIThe current documentation is minimal. Since this is marked with
@ExperimentalChatApi, more comprehensive documentation would be beneficial to explain:
- The purpose of this composable function
- Examples of how it should be used
- Potential lifecycle considerations
22-24: Consider adding error handling for the flow collectionThe current implementation doesn't handle potential errors in the flow collection. If
statusAsFlow()encounters an error, it might crash the application.LaunchedEffect(this) { - statusAsFlow().collect { status = it.current } + statusAsFlow().catch { e -> + // Log error or handle appropriately + }.collect { status = it.current } }chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/ConnectionTest.kt (2)
36-37: Simplify EmittingConnection creationThe factory function for
EmittingConnectioncreates a mock that isn't actually used in the implementation.-fun EmittingConnection() = EmittingConnection(mockk()) +fun EmittingConnection() = EmittingConnection()Then update the class definition:
-class EmittingConnection(mock: Connection) : Connection by mock { +class EmittingConnection : Connection {
16-19: Consider adding more comprehensive testsWhile the current test verifies basic functionality, consider adding tests for:
- Recomposition behavior
- Rapid status changes
- Error scenarios
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt (4)
17-43: Consider enhancing error handling and displaying loading states.The
collectAsPresenceMembers()function swallows exceptions withrunCatchingbut doesn't provide any error state for the UI to react to. Users of this function have no way to know if an error occurred or if the list is empty because there are genuinely no members present.Consider enhancing the function to return a data class that includes loading and error states:
data class PresenceMembersState( val members: List<PresenceMember> = emptyList(), val isLoading: Boolean = false, val error: Throwable? = null ) @ExperimentalChatApi @Composable public fun Room.collectAsPresenceMembers(): PresenceMembersState { var state by remember(this) { mutableStateOf(PresenceMembersState(isLoading = true)) } val roomStatus = collectAsStatus() LaunchedEffect(this, roomStatus) { if (roomStatus != Attached) { state = PresenceMembersState(isLoading = roomStatus.isTransitioning()) return@LaunchedEffect } val initialPresenceGet = launch { state = state.copy(isLoading = true) runCatching { presence.get() }.fold( onSuccess = { members -> state = PresenceMembersState(members = members) }, onFailure = { error -> state = PresenceMembersState(error = error) } ) } presence.asFlow().collect { if (initialPresenceGet.isActive) initialPresenceGet.cancelAndJoin() state = state.copy(isLoading = true) runCatching { presence.get() }.fold( onSuccess = { members -> state = PresenceMembersState(members = members) }, onFailure = { error -> state = PresenceMembersState(error = error) } ) } } return state }
34-39: Consider debouncing or throttling presence updates.Each presence event triggers a potentially expensive
presence.get()call that fetches the entire member list. In busy rooms with many presence events, this could lead to performance issues.Consider adding debouncing to reduce the frequency of calls:
import kotlinx.coroutines.flow.debounce // Inside the LaunchedEffect block presence.asFlow() .debounce(300) // Debounce for 300ms .collect { if (initialPresenceGet.isActive) initialPresenceGet.cancelAndJoin() runCatching { presenceMembers = presence.get() } }
22-24: Consider caching presence member results by room ID.The current implementation uses
remember(this)which recreates state when the Room reference changes, but Room instances might be recreated for the same underlying room.Consider using a more stable key like the room ID:
var presenceMembers by remember(id) { mutableStateOf(emptyList<PresenceMember>()) }
1-16: Function documentation could be more descriptive.The current documentation just states the return value but doesn't explain when it will be updated or any conditions/limitations.
Consider enhancing the KDoc to be more informative:
/** * Collects and provides the current list of presence members in this room as a Compose state. * * This function: * - Returns an empty list until the room is attached * - Updates the list whenever presence changes are detected * - Requires the room to be in Attached state to fetch members * * @return Currently present members in the room as a Compose-aware state */chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (2)
14-16: Enhance documentation to be more descriptive.The current documentation is minimal and doesn't provide enough context about what the function does and how it should be used. Consider expanding it to explain that this function collects room status updates and recomposes when changes occur.
/** - * @return room status + * Collects the current status of the Room as a state that will be updated whenever + * the status changes, triggering recomposition. + * + * @return The current room status that will be updated when changes occur + * @see RoomStatus */
19-27: Consider adding error handling to the flow collection.The current implementation doesn't handle potential errors that might be emitted from
statusAsFlow(). Adding error handling would make the component more robust.LaunchedEffect(this) { - statusAsFlow().collect { status = it.current } + statusAsFlow().collect { + try { + status = it.current + } catch (e: Exception) { + // Consider logging the error or providing fallback behavior + // Log.e("Room", "Error collecting room status", e) + } + } }chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (3)
74-81: RenamescrollIsAtTheTopvariable for clarityThe variable name
scrollIsAtTheTopis misleading since it's checking if the user has scrolled far enough from the beginning of the list (not at the top). Consider renaming it to something likeshouldLoadMoreBasedOnScrollorreachedScrollThresholdto better reflect its purpose.- val scrollIsAtTheTop = listState.firstVisibleItemIndex > loaded.size - scrollThreshold + val reachedScrollThreshold = listState.firstVisibleItemIndex > loaded.size - scrollThreshold - val shouldGetPreviousMessages = scrollIsAtTheTop && hasMoreItems + val shouldGetPreviousMessages = reachedScrollThreshold && hasMoreItems
67-72: Add visual indicator for message discontinuityWhile the code correctly handles clearing messages when discontinuity occurs, there's no indication to the user that a break in the message history has happened. Consider adding a state property to indicate discontinuity that UI consumers can use to display a visual separator or notification.
public interface PagingMessagesState { public val loaded: SnapshotStateList<Message> public val listState: LazyListState public val loading: Boolean public val hasMore: Boolean public val error: ErrorInfo? + public val hasDiscontinuity: Boolean public suspend fun refresh() } private data class DefaultPagingMessagesState( override val loaded: SnapshotStateList<Message>, override val listState: LazyListState, override val loading: Boolean, override val hasMore: Boolean, override val error: ErrorInfo?, + override val hasDiscontinuity: Boolean, private val refreshLambda: suspend () -> Unit, ) : PagingMessagesState { override suspend fun refresh() = refreshLambda() }
37-112: Consider breaking down the long composable functionThe function is currently suppressed with
@Suppress("LongMethod"). Consider extracting some of the functionality into separate helper functions to improve readability and maintainability, such as:
- Message subscription logic
- Pagination logic
- Error handling logic
This would make the main function more concise and easier to understand.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt (4)
23-29: Good test setup but missing documentation.The test setup is properly structured with appropriate mocking of the Room object and presence configuration. The use of
@OptIn(ExperimentalChatApi::class)correctly acknowledges that you're working with experimental APIs.Consider adding a brief class-level KDoc comment explaining the purpose of these tests, especially since they're testing experimental functionality.
132-134: Incomplete mock implementation for presence filtering.The
get()method ignores theclientIdandconnectionIdfilter parameters, which means the tests aren't verifying that filtering works correctly when these parameters are used.override suspend fun get(waitForSync: Boolean, clientId: String?, connectionId: String?): List<PresenceMember> = mutex.withLock { - clientIdToPresenceMember.values.toList() + clientIdToPresenceMember.values + .filter { member -> clientId == null || member.clientId == clientId } + // Note: You would need to add connectionId to the PresenceMember class to filter by it + .toList() }
136-142: Potential for test hangs in mutex handling.The
pause()andresume()methods don't include error handling or timeouts, which could potentially cause tests to hang if there's an issue.suspend fun pause() { - mutex.lock() + // Add timeout to prevent test hangs + val acquired = mutex.tryLock(timeoutMillis = 5000) + if (!acquired) { + throw IllegalStateException("Failed to acquire mutex lock within timeout") + } } fun resume() { + // Only unlock if currently locked to prevent IllegalStateException + if (mutex.isLocked) { mutex.unlock() + } }
3-21: Consider organizing imports.The imports are grouped logically, but they could be better organized for readability by grouping by package source (standard Kotlin, Ably, testing libraries, etc.).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
chat-android/src/main/java/com/ably/chat/Connection.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Discontinuities.kt(2 hunks)chat-android/src/main/java/com/ably/chat/FlowUtils.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Occupancy.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Presence.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Room.kt(2 hunks)chat-android/src/main/java/com/ably/chat/RoomReactions.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Typing.kt(3 hunks)chat-android/src/main/java/com/ably/chat/annotations/Annotations.kt(1 hunks)chat-android/src/test/java/com/ably/chat/ConnectionTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/OccupancyTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/PresenceTest.kt(3 hunks)chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/RoomTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/TypingTest.kt(3 hunks)chat-extensions-compose/build.gradle.kts(1 hunks)chat-extensions-compose/gradle.properties(1 hunks)chat-extensions-compose/proguard-rules.pro(1 hunks)chat-extensions-compose/src/main/AndroidManifest.xml(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/ConnectionTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/TypingTest.kt(1 hunks)example/build.gradle.kts(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(7 hunks)example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt(1 hunks)gradle/libs.versions.toml(3 hunks)settings.gradle.kts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (24)
- chat-extensions-compose/src/main/AndroidManifest.xml
- example/build.gradle.kts
- settings.gradle.kts
- chat-android/src/main/java/com/ably/chat/annotations/Annotations.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt
- chat-android/src/main/java/com/ably/chat/RoomReactions.kt
- chat-extensions-compose/gradle.properties
- chat-android/src/main/java/com/ably/chat/Occupancy.kt
- chat-android/src/main/java/com/ably/chat/Presence.kt
- chat-android/src/main/java/com/ably/chat/Messages.kt
- chat-android/src/main/java/com/ably/chat/Room.kt
- chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt
- chat-extensions-compose/build.gradle.kts
- chat-android/src/main/java/com/ably/chat/FlowUtils.kt
- chat-android/src/main/java/com/ably/chat/Typing.kt
- chat-android/src/test/java/com/ably/chat/TypingTest.kt
- chat-android/src/main/java/com/ably/chat/Connection.kt
- chat-android/src/test/java/com/ably/chat/MessagesTest.kt
- chat-android/src/test/java/com/ably/chat/PresenceTest.kt
- gradle/libs.versions.toml
- chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt
- chat-extensions-compose/proguard-rules.pro
- example/src/main/java/com/ably/chat/example/MainActivity.kt
- chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/TypingTest.kt
🧰 Additional context used
🧠 Learnings (3)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/main/java/com/ably/chat/Occupancy.kt:145-145
Timestamp: 2024-11-28T11:11:20.423Z
Learning: In `chat-android/src/main/java/com/ably/chat/Occupancy.kt`, within the `DefaultOccupancy` class, when methods use `room.chatApi`, which utilizes the REST API, there's no need to call `room.ensureAttached()` before performing the operation.
chat-android/src/test/java/com/ably/chat/RoomTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/OccupancyTest.kt:20-31
Timestamp: 2024-11-28T11:12:06.843Z
Learning: In `OccupancyTest.kt`, additional room state initialization using `setState` is unnecessary for testing occupancy functionality, as the specification doesn't require it.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check
- GitHub Check: coverage
🔇 Additional comments (29)
example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt (5)
18-20: Clean API migration with appropriate imports.The imports have been properly updated to support the new Flow-based API, including the experimental API annotation and the compose extension function.
25-25: Take note of experimental API usage.The
@OptInannotation correctly indicates that this code uses experimental API features that may change in future releases. This is good practice for API evolution awareness.
27-27: Improved function signature with direct Room access.The signature change from accepting
ChatClientto acceptingRoomdirectly is a positive change that follows the principle of least privilege - providing only what's necessary for the component to function.
28-28: Simplified state management with Compose extensions.The refactoring greatly simplifies presence state handling by using the
collectAsPresenceMembers()extension function, eliminating manual subscription management and state updates that were previously required. This change improves code readability and reduces potential bugs related to lifecycle management.
30-30: Direct access to presence from Room object.Accessing presence directly from the room object follows a more intuitive object hierarchy and maintains consistency with the new API design.
chat-android/src/main/java/com/ably/chat/Discontinuities.kt (2)
49-51: Well-implemented Flow extension function.The implementation effectively converts the callback-based API to a Flow-based approach, which aligns well with modern Kotlin coroutines patterns. This provides a more reactive way to handle discontinuity events.
6-6: Appropriate import for Flow API.The import for
kotlinx.coroutines.flow.Flowis correctly added to support the new flow-based functionality.chat-android/src/test/java/com/ably/chat/OccupancyTest.kt (1)
156-177: Good test pattern for verifying Flow unsubscription.The test correctly verifies that the
asFlow()method properly unsubscribes when the flow is canceled. This is important for preventing memory leaks by ensuring resources are cleaned up properly.chat-android/src/test/java/com/ably/chat/RoomTest.kt (1)
1-36: LGTM - Clean test implementation for Flow unsubscription.This test follows good practices for verifying that resources are properly cleaned up when the Flow is canceled. The mocking setup is concise and the verification is clear.
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (2)
16-19: LGTM - Good data model with default values.The
CurrentOccupancydata class is well-designed with appropriate default values, making it safe to use in UI components even before data is loaded.
26-53: Well-structured Composable with proper cleanup handling.The implementation follows Compose best practices:
- Uses
rememberkeyed on the room instance- Uses
LaunchedEffectfor side effects- Properly handles the room's attachment state
- Cancels the initial request if a streaming update arrives first
Regarding the previous comment about "Should we be doing the initial collect if we're not attached?", this is now correctly addressed with the early return on line 31.
chat-android/src/test/java/com/ably/chat/ConnectionTest.kt (1)
151-172: LGTM - Consistent test pattern for Flow unsubscription.This test follows the same good pattern used in other Flow tests, verifying that resources are properly cleaned up when the Flow is canceled.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt (3)
28-41: LGTM! Well-structured test for occupancy collectionThe test appropriately validates that occupancy events are correctly emitted and collected through the composable function. The use of
moleculeFlowwithRecompositionMode.Immediateis a good approach for testing composable functions.
43-57: LGTM! Good edge case testThis test effectively verifies the cancellation behavior when an occupancy event arrives during initial occupancy retrieval. The use of
pause()andresume()to control the timing is a clean approach.
62-90: Add an explicit test for auto-unsubscriptionThe
EmittingOccupancyimplementation is well-structured with proper thread safety using Mutex. However, there's no explicit test for verifying that the flow automatically unsubscribes when completed.According to the AI summary, there should be a test method named "asFlow() should automatically unsubscribe then it's done", but it's not present in this file. Similar tests are already implemented in the chat-android module.
#!/bin/bash # Check if there's a test for auto-unsubscription in other files rg -A 3 "asFlow\(\) should automatically unsubscribe" --type kotlinchat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt (1)
17-27: Good implementation of the Compose composable functionThe
collectAsStatus()composable function is well-implemented, using appropriate Compose patterns:
- Properly uses
rememberwith the connection instance as a key- Correctly implements
LaunchedEffectfor side-effect handling- Cleanly transforms the Flow-based API into a Compose state
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/ConnectionTest.kt (2)
21-33: Well-structured test using Molecule for composable testingThe test effectively verifies the composable function behavior:
- Uses Molecule to test the composable outside of a UI environment
- Properly asserts state changes using Turbine
- Tests multiple status transitions
- Ensures proper cleanup with cancel()
38-53: The mock implementation is clean and effectiveThe
EmittingConnectionclass is well-implemented:
- Properly tracks and notifies listeners
- Handles subscription cleanup correctly
- Provides a simple API for emitting events in tests
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt (1)
30-32: Handle errors when getting presence members.The
runCatchingblock swallows any exceptions without logging or providing error feedback to the caller. This matches the concern raised in a previous review aboutget()failing if not attached.While you do check for
Attachedstatus in line 27, there could still be race conditions or other failure reasons. Consider how to communicate these errors to consumers of this API.chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (4)
17-18: LGTM! Good use of experimental API annotation.Correctly marking this API as experimental with
@ExperimentalChatApiallows for future modifications while warning users about potential changes.
20-20: Good use of the remember key parameter.Using
remember(this)ensures the state is correctly reset if the Room instance changes, which is important for proper lifecycle management.
22-24: Verify flow resource cleanup.The
LaunchedEffectwill handle cancellation of the flow collection when the composition leaves scope, which is the correct approach for resource management in Compose.
1-27:✅ Verification successful
Verify integration with statusAsFlow extension.
Ensure that the
statusAsFlow()extension function (imported from line 12) is properly implemented and tested. The success of this composable depends on it working correctly.
🏁 Script executed:
#!/bin/bash # Search for the implementation of statusAsFlow to verify it exists and works as expected echo "Searching for statusAsFlow implementation..." rg -p "fun.*statusAsFlow" --type kotlin # Also check for any tests for this functionality echo "Checking for tests of the statusAsFlow functionality..." rg -p "test.*statusAsFlow|statusAsFlow.*test" --type kotlinLength of output: 1105
StatusAsFlow Integration Verified
The integration with thestatusAsFlow()extension has been validated. The implementation is present in the main code (e.g., inchat-android/src/main/java/com/ably/chat/Room.kt), and the tests confirming its behavior are available (notably inchat-android/src/test/java/com/ably/chat/RoomTest.ktand the corresponding connection tests). No changes are required here.chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (4)
34-37: Good use of experimental API annotationThe
@ExperimentalChatApiannotation clearly indicates to users that this API might change in the future. Consider adding documentation that explains what aspects of the API might change and how users can prepare for potential breaking changes.
39-44: Consider implementing message count limiting mechanismThe current implementation stores all loaded messages in memory without any upper bound, which could potentially lead to memory issues with very large chat histories. Consider implementing a mechanism to limit the total number of messages kept in memory, such as removing older messages when a certain threshold is reached.
92-98: Enhance error handling with retry mechanismWhile the code captures errors, the user must manually call
refresh()to retry after an error. Consider enhancing this with automatic retry logic with exponential backoff or providing more context in the error state to help users understand and recover from failures.
36-43: Add support for custom limit in getPreviousMessagesWhile the function accepts a
limitparameter, it doesn't allow consumers to change this value dynamically after initial composition. Consider adding a way for users to control how many messages to fetch when loading more, perhaps through a method in thePagingMessagesStateinterface.chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt (2)
31-78: Well-structured test for presence state collection.This test effectively validates that presence events are properly collected into state using Molecule's composable testing utilities. The assertions correctly verify both the initial empty state and subsequent updates.
80-117: Good edge case testing for concurrent event handling.This test properly verifies an important edge case where presence events arrive while the initial presence set is being loaded. The use of mutex pause/resume is an effective technique for controlling the test flow.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt
Show resolved
Hide resolved
| */ | ||
| @ExperimentalChatApi | ||
| @Composable | ||
| public fun Room.collectAsPresenceMembers(): List<PresenceMember> { |
There was a problem hiding this comment.
I
| public fun Room.collectAsPresenceMembers(): List<PresenceMember> { | |
| public fun Room.collectAsPresenceMembers(): List<PresenceMember> { |
the method returns current presence members, so is it possible to rename it that way.
Not sure if there's specific method syntax for @ Composable methods
There was a problem hiding this comment.
like Room.getCurrentPresenceMembers()
Same goes for other methods
There was a problem hiding this comment.
get is a very generic name, and it can also conflict with property getters and setters. Extension functions that transform an observable state into composable data usually include this information in their name. See https://developer.android.com/develop/ui/compose/state#use-other-types-of-state-in-jetpack-compose.
All Android extensions follow one of these naming patterns:
• collectAs
• subscribeAs
• observeAs
I chose collectAs because, in these composable extensions, we are mostly collecting a Flow.
There was a problem hiding this comment.
Regarding currentPresenceMembers, I get the impression that "presence" already implies currently present members. @AndyTWF wdyt?
There was a problem hiding this comment.
I agree, "presence" in its own right applies "whoever is currently present".
OOI, do we want to move these properties to the respective classes, e.g. Presence.collectAsMembers, rather than having it at the Room level?
There was a problem hiding this comment.
The main reason is that having them at the room level makes it possible to use the room’s status internally. I don’t want to introduce a backlink to the room in the public API of a specific "feature". I also don’t think it looks ugly or misleading.
Additionally, it’s more consistent to have a custom extension that combines data from two different "features" —something like CurrentlyTypingPresenceMember. I’m not sure if it’s useful, but who knows? :)
There was a problem hiding this comment.
Ah, can we not pass down the RoomStatus object to avoid using the rooms API internally?
There was a problem hiding this comment.
We can, but I think it will look weird :)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt
Show resolved
Hide resolved
| @ExperimentalChatApi | ||
| @Composable | ||
| @Suppress("LongMethod") | ||
| public fun Room.collectAsPagingMessagesState(scrollThreshold: Int = 10, limit: Int = 100): PagingMessagesState { |
There was a problem hiding this comment.
Maybe call this fetchSize or something like that? limit has implications that may not be clear I think
|
|
||
| LaunchedEffect(this) { | ||
| messages.discontinuityAsFlow().collect { | ||
| loaded.clear() |
There was a problem hiding this comment.
Is it worth doing another fetch here to try and repopulate ourself?
There was a problem hiding this comment.
It should be done automatically, we reset state here, and if we are good (room is attached, we instantly reinitialize messages)
| */ | ||
| @ExperimentalChatApi | ||
| @Composable | ||
| public fun Room.collectAsPresenceMembers(): List<PresenceMember> { |
There was a problem hiding this comment.
Ah, can we not pass down the RoomStatus object to avoid using the rooms API internally?
| /** | ||
| * @return [ConnectionStatusChange] events as a [Flow] | ||
| */ | ||
| public fun EmitsDiscontinuities.discontinuityAsFlow(): Flow<ErrorInfo?> = transformCallbackAsFlow { |
There was a problem hiding this comment.
I didn't work on flows yet.
So, had a question if there are libraries returning cold flows.
I am thinking, most kotlin SDKs return shared flows, so instance can be shared by multiple subscribers.
There was a problem hiding this comment.
Okay, I asked perplexity, and it seems most kotlin public APIs return cold flows by default : )
So, we are okay there, but you can double check 👍
There was a problem hiding this comment.
Btw, feature level discontinuity will be move to room level. So, we will not need this
https://ably-real-time.slack.com/archives/C02NY1VT3LY/p1741626477605909
There was a problem hiding this comment.
Maybe you like to remove discontinuityAsFlow to avoid introducing breaking public API changes.
There was a problem hiding this comment.
We are not going to do next release before single channel is landed, so there'll be no breaking changes for users. I think we even should preserve interface, it's generic enough, maybe some other API will have discontinuities as well
There was a problem hiding this comment.
Okay, then we will update once single channel changes are introduced
|
Overall looks okay to me, apart from few posted comments ( you can take a look 👍 ) |
Added `com.ably.chat:chat-extensions-compose` package with extension functions for better integration with Jetpack Compose.
ttypic
force-pushed
the
CHA-801/flow-and-compose
branch
from
828e13e to
d710621
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/ConnectionTest.kt (3)
19-19: Consider adding a descriptive comment explaining the test setup.A brief comment explaining that
EmittingConnectionis a test double that simulates connection status changes would improve readability and maintainability of the test.
36-36: Consider making this a private function.This factory function is only used internally within this test class. Consider making it private to restrict its scope.
-fun EmittingConnection() = EmittingConnection(mockk()) +private fun EmittingConnection() = EmittingConnection(mockk())
48-52: Consider adding a test for edge case: empty listeners list.The
emitmethod works correctly when there are listeners, but there's no test verifying behavior when the listeners list is empty. While this isn't an issue functionally (it will simply do nothing), adding an assertion might make the test more comprehensive.chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt (2)
36-36: Rename factory function to avoid overshadowing class name.Currently, the
fun EmittingRoom() = EmittingRoom(mockk())factory function has the same name as the class, which could lead to confusion. Consider renaming it (e.g.,createEmittingRoom()) for greater clarity.-fun EmittingRoom() = EmittingRoom(mockk()) +fun createEmittingRoom() = EmittingRoom(mockk())
41-52: Ensurestatustracks the latest status change.The
statusproperty is never updated after initialization, so readingroom.statusdirectly can be misleading if events have changed its state. Consider synchronizing the property with the last reportedcurrentstatus in theemitmethod.class EmittingRoom(mock: Room) : Room by mock { private val listeners = mutableListOf<Room.Listener>() - override val status: RoomStatus = RoomStatus.Initialized + private var currentStatus: RoomStatus = RoomStatus.Initialized + override val status: RoomStatus get() = currentStatus override fun onStatusChange(listener: Room.Listener): Subscription { listeners.add(listener) return Subscription { listeners.remove(listener) } } fun emit(event: RoomStatusChange) { + currentStatus = event.current listeners.forEach { it.roomStatusChanged(event) } } }chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt (2)
28-41: Tests look good, but tests for edge cases would be beneficial.The test effectively validates the basic functionality of
collectAsOccupancy()by verifying that it correctly processes and emits occupancy events. The use ofmoleculeFlowto test Compose integration is appropriate.Consider adding tests for edge cases like:
- Multiple concurrent emissions
- Error handling scenarios
- Cases where
Room.statusis notAttached
60-60: Consider adding documentation to the factory function.The factory function is helpful but lacks documentation explaining its purpose.
-fun EmittingOccupancy() = EmittingOccupancy(mockk()) +/** + * Factory function that creates an EmittingOccupancy instance with a mocked underlying Occupancy. + * Useful for testing scenarios that require controlling occupancy events. + */ +fun EmittingOccupancy() = EmittingOccupancy(mockk())chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (3)
26-33: Revise the KDoc to accurately reflect the state return type.The comment states that this function returns a list of messages, but in reality, it returns a
PagingMessagesState, which provides additional loading and error info. Consider revising the KDoc so that it explicitly describes returning a state object rather than just a list.
74-82: RenamescrollIsAtTheTopto match its logic.
scrollIsAtTheTopis calculated usinglistState.firstVisibleItemIndex > loaded.size - scrollThreshold, which actually checks if the user is near the end of the list. Inverting the name or logic would reduce confusion; for example, considerscrollIsAtTheBottomorscrollIsPastThreshold.
84-102: Clarify the null-safety expectations forsubscription.Relying on
subscription!!can risk a NullPointerException if disposal or re-composition logic sets it to null unexpectedly. Even though your conditions presumably guard against that, consider explicitly checking for null or using an assertion to make your assumptions clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
chat-android/src/main/java/com/ably/chat/Connection.kt(3 hunks)chat-android/src/main/java/com/ably/chat/FlowUtils.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Typing.kt(3 hunks)chat-android/src/main/java/com/ably/chat/annotations/Annotations.kt(1 hunks)chat-extensions-compose/build.gradle.kts(1 hunks)chat-extensions-compose/gradle.properties(1 hunks)chat-extensions-compose/proguard-rules.pro(1 hunks)chat-extensions-compose/src/main/AndroidManifest.xml(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/ConnectionTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/TypingTest.kt(1 hunks)example/build.gradle.kts(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(7 hunks)example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt(1 hunks)gradle/libs.versions.toml(3 hunks)settings.gradle.kts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- chat-extensions-compose/src/main/AndroidManifest.xml
- settings.gradle.kts
- example/build.gradle.kts
- chat-android/src/main/java/com/ably/chat/Typing.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt
- chat-android/src/main/java/com/ably/chat/annotations/Annotations.kt
- chat-extensions-compose/proguard-rules.pro
- chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt
- chat-android/src/main/java/com/ably/chat/FlowUtils.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt
- chat-android/src/main/java/com/ably/chat/Connection.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt
- chat-extensions-compose/build.gradle.kts
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt
- chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/TypingTest.kt
- gradle/libs.versions.toml
- example/src/main/java/com/ably/chat/example/MainActivity.kt
- chat-extensions-compose/gradle.properties
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt
🧰 Additional context used
🧠 Learnings (2)
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/OccupancyTest.kt:20-31
Timestamp: 2025-03-12T13:33:27.491Z
Learning: In `OccupancyTest.kt`, additional room state initialization using `setState` is unnecessary for testing occupancy functionality, as the specification doesn't require it.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2025-03-12T13:33:27.491Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: coverage
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (12)
example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt (5)
18-20: Appropriate imports for new Flow-based approachThe imports align with the PR objectives to add Flow and Jetpack Compose support to the Ably Chat library. The inclusion of the
collectAsPresenceMembersextension function is particularly relevant for simplifying presence management.
25-25: Proper usage of OptIn annotation for experimental APIGood practice to explicitly opt-in to experimental APIs. This makes it clear to other developers that this functionality might change in future releases.
27-27: Improved function signature with Room parameterChanging the parameter from
ChatClienttoRoomis a good design decision as it:
- Better follows the single responsibility principle
- Makes the code more direct (presence is a property of a room)
- Aligns with the Flow-based architecture being implemented
This change improves the API's usability and makes the code more maintainable.
28-28: Simplified presence handling with FlowExcellent simplification! The new implementation using
collectAsPresenceMembers()eliminates the need for manual subscription management and state updates. This makes the code:
- More concise
- Less error-prone
- More idiomatic Kotlin
The previous implementation required complex DisposableEffect and subscription management code that has been completely removed.
30-30: Direct access to room.presenceAccessing presence directly from the room object simplifies the code and follows a more intuitive object hierarchy, improving readability and maintainability.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/ConnectionTest.kt (3)
22-33: Well-structured test using modern testing libraries.This test effectively validates that the connection status changes are properly propagated through the Flow and collected in a composable context. The use of Molecule and Turbine for testing flows in a composable environment is a good practice.
38-53: Add test for initial status notification.The
EmittingConnectionclass effectively simulates a connection with status change capability, but doesn't test whether the initial status is notified to a listener when it's first added. Consider adding a test that verifies a listener receives the initial status when registered.Does the actual
Connectionimplementation notify listeners of the current status immediately upon registration? If so, this test double should mimic that behavior.
43-46: Good implementation of subscription management.The subscription pattern is well implemented, ensuring proper cleanup when the subscription is disposed, preventing memory leaks.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt (3)
43-57: Well-structured test for race condition handling.This test elegantly verifies that ongoing occupancy retrieval is correctly canceled when new events arrive, which is an important edge case for reactive implementations. Using the mutex pause/resume mechanism provides good control over the test flow.
62-90: Well-implemented test utility with proper thread safety.The
EmittingOccupancyclass is well-designed for testing purposes with:
- Proper thread safety using Mutex
- Clear subscription management
- Simple event emission mechanism
20-90: Add explicit test for auto-unsubscription when flow completes.While the existing tests validate the normal operation flow, there's no explicit test verifying that the flow automatically unsubscribes when completed.
Consider adding an equivalent auto-unsubscription test to this file to ensure consistent behavior across the codebase. This would follow the pattern seen in other test files like
chat-android/src/test/java/com/ably/chat/OccupancyTest.kt.chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (1)
49-58: Reevaluate handling ofMessageEventType.Deleted.Deleted messages are currently replaced with a new “deleted” message object instead of being removed from the list, which some UIs might find unintuitive. Consider whether you actually intend to remove the entry from the UI entirely or mark it in a different way.
kotlinx.coroutines.flow.Flowsupport for all callback-based subscriptioncom.ably.chat:chat-extensions-composepackage with extension functions for better integration with Jetpack Compose.Summary by CodeRabbit
New Features
Bug Fixes