-
Notifications
You must be signed in to change notification settings - Fork 46
chat docs: Add jetpack compose support ( latest ) #3097
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request introduces Jetpack Compose as a supported language variant in the chat documentation and configuration system. Kotlin version is bumped from 1.0 to 1.1, with a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/docs/chat/getting-started/android.mdx (1)
720-733: Conditional composable function call violates Compose stability rules.The pattern
room?.collectAsPresenceMembers()conditionally invokes a@Composablefunction. Compose requires that composable functions are called unconditionally and in the same order on every recomposition to maintain call-site identity and stability.Restructure to guard the null case at the start and ensure the composable is called unconditionally:
Suggested restructure
`@Composable` fun PresenceStatusUi(room: Room?) { + if (room == null) { + Text( + text = "Online: 0", + style = MaterialTheme.typography.bodyMedium, + modifier = Modifier.padding(start = 8.dp) + ) + return + } + - val membersState = room?.collectAsPresenceMembers() - val members by membersState ?: remember { mutableStateOf(emptyList()) } + val members by room.presence.collectAsPresenceMembers() LaunchedEffect(room) { room?.presence?.enter() } Text( text = "Online: ${members.size}", style = MaterialTheme.typography.bodyMedium, modifier = Modifier.padding(start = 8.dp) ) }
🤖 Fix all issues with AI agents
In `@src/pages/docs/chat/rooms/presence.mdx`:
- Around line 217-228: Remove the unused coroutineScope variable declared in
EnterPresenceComponent; since LaunchedEffect provides a coroutine scope and you
call the suspend function room.presence.enter(...) inside it, delete the val
coroutineScope = rememberCoroutineScope() line so only LaunchedEffect remains
invoking room.presence.enter(...) in EnterPresenceComponent.
In `@src/pages/docs/chat/setup.mdx`:
- Around line 137-145: The code blocks show Groovy Gradle syntax but are labeled
as kotlin/jetpack; update the block language tags to ```groovy for both blocks
(or alternatively convert the dependency lines to the Kotlin DSL form
implementation("com.ably.chat:chat:1.1.0") and
implementation("com.ably.chat:chat-extensions-compose:1.1.0") to match a
```kotlin tag). Ensure the labels and the lines for the strings 'implementation
'com.ably.chat:chat:1.1.0'' and 'implementation
'com.ably.chat:chat-extensions-compose:1.1.0'' are consistent.
🧹 Nitpick comments (8)
src/pages/docs/chat/rooms/message-reactions.mdx (2)
481-509: Consider removing duplicate code block.The
jetpackcode block (lines 481-509) is identical to thekotlincode block (lines 451-479). Since theMessageandMessageReactionSummaryinterface definitions are the same for both Kotlin and Jetpack Compose, consider whether both blocks are needed or if thekotlinblock alone would suffice (with Jetpack users seeing the Kotlin syntax-highlighted version via the alias).
644-661: Potential race condition between LaunchedEffects.The two separate
LaunchedEffectblocks for loading initial messages (lines 644-647) and subscribing to reactions (lines 649-661) run concurrently. This could result in the subscription starting before messages are loaded, potentially missing early reaction events for those messages.Consider combining into a single
LaunchedEffector usingsnapshotFlowto ensure messages are loaded before subscribing:💡 Alternative approach
LaunchedEffect(room) { // init messages messages = room.messages.history(limit = 50).items - } - - LaunchedEffect(room) { // subscribe to message reactions summary events room.messages.reactions.asFlow().collect { event ->src/pages/docs/chat/rooms/occupancy.mdx (1)
202-215: Consider aligning example with section intent.This "Current room occupancy" section describes retrieving occupancy in a "one-off call" (line 183), but
CurrentOccupancyComponentusescollectAsOccupancy()which sets up a continuous subscription. For a true one-off read, you might want to accessroom.occupancy.currentdirectly similar to the Kotlin example on line 199:💡 Suggested alternative for one-off access
`@Composable` fun CurrentOccupancyComponent(room: Room) { - val occupancy by room.occupancy.collectAsOccupancy() + var occupancy by remember { mutableStateOf(room.occupancy.current) } Text("Connections: ${occupancy.connections}") Text("Presence members: ${occupancy.presenceMembers}") }Alternatively, if
collectAsOccupancy()is the idiomatic Compose approach and provides the same functionality, consider updating the section text to clarify that for Jetpack Compose, subscribing is the recommended pattern.src/pages/docs/chat/rooms/messages.mdx (1)
65-79: Add missing imports for completeness.The code example is missing several imports that are used in the snippet. For consistency with other Jetpack examples in this PR (e.g., lines 211-228 which include full imports), consider adding the missing imports.
Suggested imports
```jetpack import com.ably.chat.extensions.compose.collectAsPagingMessagesState +import androidx.compose.foundation.lazy.LazyColumn +import androidx.compose.foundation.lazy.items +import androidx.compose.material.Text +import androidx.compose.runtime.* +import com.ably.chat.Room `@Composable` fun MyComponent(room: Room) {src/pages/docs/chat/rooms/reactions.mdx (1)
121-130: Clarify when automatic vs manual cleanup applies.The comment "Jetpack Compose handles cleanup automatically" is accurate for the
asFlow().collectpattern shown earlier (lines 54-66), but this code block demonstrates the manualsubscribe()pattern which requires explicitunsubscribe()calls. Consider clarifying this distinction in the comment.Suggested clarification
```jetpack -// Jetpack Compose handles cleanup automatically -// When using subscribe directly: +// When using asFlow().collect in LaunchedEffect, cleanup is automatic. +// When using subscribe() directly, manual unsubscribe is needed: val (unsubscribe) = room.reactions.subscribe { event -> println("Received a reaction of type ${event.reaction.name}, and metadata ${event.reaction.metadata}") }src/pages/docs/chat/rooms/presence.mdx (2)
140-149: Consider clarifying the distinction between automatic and manual cleanup.The note at line 107 states Jetpack Compose handles cleanup automatically when using
collectAsPresenceMembers(), but this code block shows manualsubscribe()usage with explicitunsubscribe(). Consider adding a brief comment explaining when developers might choose the manual approach over the automatic one.
483-499: Jetpack example differs semantically from Kotlinpresence.get()example.The Kotlin example above demonstrates a one-shot
presence.get()call, while this Jetpack example usescollectAsPresenceMembers()which is reactive/subscription-based. Consider either:
- Adding a comment clarifying that Jetpack Compose idiomatically uses reactive state
- Or showing a one-shot pattern using
LaunchedEffectwithroom.presence.get()for parityThis helps developers understand the semantic difference between the imperative and reactive approaches.
src/pages/docs/chat/rooms/typing.mdx (1)
204-220: Consider noting SDK throttling behavior in the example.The example calls
keystroke()on everyonValueChange, which aligns with the documentation at line 292 that states "callkeystroke()with every keypress, and the SDK will handle when and if to send a typing indicator."A brief inline comment in the code example could reinforce this SDK-managed behavior for developers unfamiliar with the pattern.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
src/data/languages/languageData.tssrc/data/languages/languageInfo.tssrc/data/languages/types.tssrc/pages/docs/chat/connect.mdxsrc/pages/docs/chat/getting-started/android.mdxsrc/pages/docs/chat/rooms/history.mdxsrc/pages/docs/chat/rooms/index.mdxsrc/pages/docs/chat/rooms/message-reactions.mdxsrc/pages/docs/chat/rooms/messages.mdxsrc/pages/docs/chat/rooms/occupancy.mdxsrc/pages/docs/chat/rooms/presence.mdxsrc/pages/docs/chat/rooms/reactions.mdxsrc/pages/docs/chat/rooms/typing.mdxsrc/pages/docs/chat/setup.mdx
🔇 Additional comments (47)
src/data/languages/types.ts (1)
29-29: LGTM!The
jetpacklanguage key is correctly added to thelanguageKeysarray, enabling type-safe usage across the codebase.src/pages/docs/chat/rooms/message-reactions.mdx (1)
165-200: LGTM!The
SendMessageReactionComponentfollows Compose best practices with proper use ofrememberCoroutineScope()for launching coroutines from click handlers.src/data/languages/languageInfo.ts (1)
89-93: LGTM!The
jetpacklanguage entry is correctly configured with appropriate label, Kotlin syntax highlighting, and alias.src/data/languages/languageData.ts (1)
42-43: LGTM!The version updates are appropriate - both
kotlinandjetpackare set to version1.1for the chat product, reflecting that Jetpack Compose support is built on the chat-kotlin SDK.src/pages/docs/chat/rooms/occupancy.mdx (3)
62-75: LGTM!The
OccupancyComponentcorrectly usescollectAsOccupancy()for reactive occupancy updates in a Compose-idiomatic way.
262-281: LGTM!The
GetOccupancyComponentcorrectly demonstrates one-off occupancy retrieval usingLaunchedEffectwithroom.occupancy.get().
145-147: LGTM!Helpful lifecycle note clarifying that Compose handles cleanup automatically when using
collectAsOccupancy().src/pages/docs/chat/rooms/messages.mdx (8)
20-22: LGTM!Clear introduction of the Jetpack Compose approach with
collectAsPagingMessagesState()as the primary method and mentioning the alternativemessages.subscribe()option.
130-132: LGTM!Correctly notes that
collectAsPagingMessagesState()handles lifecycle and cleanup automatically, which is accurate for Compose's StateFlow collection patterns.
211-229: LGTM!Correct use of
rememberCoroutineScope()andcoroutineScope.launchpattern for calling the suspend functionroom.messages.send()from a button click handler.
273-275: LGTM!The Jetpack example matches the Kotlin example, which is appropriate for a simple API call demonstration.
339-363: LGTM!The Jetpack example correctly demonstrates the update pattern using
rememberCoroutineScope(). The comment clarifying thatoriginalMessageis assumed to be available is helpful for documentation purposes.
459-486: LGTM!Good demonstration of the
asFlow().collectpattern withinLaunchedEffectfor handling message events reactively in Compose. The version comparison logic for updates is correctly implemented.
586-610: LGTM!Consistent pattern with the update example, correctly using
rememberCoroutineScope()for the delete operation.
706-729: LGTM!Correct implementation of delete event handling using
filterNotwith proper version comparison to ensure only older messages are removed.src/pages/docs/chat/rooms/index.mdx (9)
73-76: LGTM!Basic room retrieval API call is appropriately identical to the Kotlin example.
127-140: LGTM!Room options configuration using the Kotlin DSL builder pattern is correctly demonstrated.
174-177: LGTM!Room release API call is appropriately identical to the Kotlin example.
228-231: LGTM!Room attach API call is appropriately identical to the Kotlin example.
259-262: LGTM!Room detach API call is appropriately identical to the Kotlin example.
295-297: LGTM!Clear introduction of the
collectAsStatus()composable function for observing room status in Jetpack Compose.
337-350: LGTM!Correct use of
collectAsStatus()with state delegation (by) and appropriate imports.
359-361: LGTM!Appropriately references
collectAsStatus()for reactive status observation.
381-398: LGTM!Good demonstration of using
LaunchedEffectkeyed onroomStatusto react to status changes. This is the idiomatic Compose approach for side effects triggered by state changes.src/pages/docs/chat/rooms/history.mdx (4)
14-16: LGTM!Clear description of using
collectAsPagingMessagesState()for paginated message history retrieval in Jetpack Compose.
78-100: LGTM!Well-structured example demonstrating
collectAsPagingMessagesState()with theorderByparameter and proper UI rendering in aLazyColumn.
120-122: LGTM!Clear description of using
historyBeforeSubscribe()for retrieving messages before subscription.
201-247: Consider simplifying the subscription and history fetch pattern.The current implementation uses separate
DisposableEffectandLaunchedEffectwith a state variable to coordinate subscription and history fetching. While this works, it's a more complex pattern.A few observations:
- The
DisposableEffect(room)correctly manages the subscription lifecycle- The
LaunchedEffect(subscription)correctly waits for subscription to be available before fetching history- The state accumulation pattern (
messages = messages + ...) creates new lists on each page, which is fine for documentation purposesThe example is functional and demonstrates the concept well.
src/pages/docs/chat/rooms/reactions.mdx (3)
54-66: LGTM!Correct use of
asFlow().collectwithinLaunchedEffectfor observing room reactions in Jetpack Compose.
87-88: LGTM!Correctly notes that Jetpack Compose handles lifecycle and cleanup automatically with
LaunchedEffect.
183-216: LGTM!Good demonstration of sending reactions with and without metadata using the
rememberCoroutineScope()pattern and thejsonObjectDSL builder.src/pages/docs/chat/connect.mdx (6)
31-33: LGTM!Clear description of using
collectAsStatus()for observing connection status in Jetpack Compose.
63-73: LGTM!Correct use of
collectAsStatus()onchatClient.connectionwith state delegation pattern.
102-104: LGTM!Appropriately references
collectAsStatus()for reactive connection status observation.
136-153: LGTM!Well-structured example demonstrating reactive connection status observation with
LaunchedEffectfor side effects on status changes.
188-190: LGTM!Clear introduction of
discontinuityAsFlow()for observing discontinuity events in Jetpack Compose.
228-243: LGTM!Correct use of
discontinuityAsFlow().collectwithinLaunchedEffectfor handling discontinuity events in Jetpack Compose.src/pages/docs/chat/rooms/presence.mdx (3)
67-83: LGTM! Clean Jetpack Compose presence subscription example.The code correctly uses the
collectAsPresenceMembers()extension function with proper state delegation viabykeyword. The composable follows Compose conventions.
289-304: LGTM! Correct coroutine usage for button click handlers.The
rememberCoroutineScope()is appropriately used here sinceonClickis not a suspend function, unlikeLaunchedEffectwhich provides its own scope.
522-532: LGTM! Correct one-shot query pattern.The
LaunchedEffectkeyed on bothroomandclientIdcorrectly triggers a re-query when either dependency changes. The state management withmutableStateOfis appropriate for this use case.src/pages/docs/chat/getting-started/android.mdx (2)
255-269: LGTM! Idiomatic Flow collection in Compose.The change from
DisposableEffectwith manual subscription toLaunchedEffectwith Flow collection (room?.messages?.asFlow()?.collect) is the recommended pattern for Compose. The coroutine automatically cancels when the effect leaves composition.
544-569: LGTM! Correct pattern for subscription with history.The two-phase approach is appropriate:
DisposableEffectestablishes the subscription and stores the referenceLaunchedEffect(subscription)fetches historical messages once the subscription is readyThis ensures messages before the subscription point are retrieved without race conditions.
src/pages/docs/chat/rooms/typing.mdx (2)
58-71: LGTM! Clean typing indicator subscription.The
collectAsCurrentlyTyping()extension follows the same pattern as other Compose extensions. ThejoinToStringusage is appropriate for displaying the set of typing users.
344-350: LGTM! Consistent with earlier example.This component provides a focused example for the "retrieve currently typing" section, even though it's functionally similar to
TypingComponentshown earlier. The repetition is appropriate given the documentation structure.src/pages/docs/chat/setup.mdx (3)
124-131: LGTM! Dependency setup is clear.The separation of standard Kotlin and Jetpack dependencies is appropriate. The Jetpack block correctly includes both the core
chatdependency and thechat-extensions-composeextension library.
202-215: LGTM! Consistent initialization pattern.The Jetpack initialization is identical to the standard Kotlin initialization, which is correct since
ChatClientsetup doesn't differ between regular Kotlin and Jetpack Compose usage. The separate block enables proper documentation filtering.
300-311: LGTM! Logging configuration example.The Jetpack logging example correctly demonstrates the
ChatClientconfiguration lambda with customlogHandlerandlogLevelsettings.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| @Composable | ||
| fun EnterPresenceComponent(room: Room) { | ||
| val coroutineScope = rememberCoroutineScope() | ||
| LaunchedEffect(room) { | ||
| room.presence.enter( | ||
| jsonObject { | ||
| put("status", "Online") | ||
| } | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused coroutineScope variable.
The rememberCoroutineScope() at line 219 is declared but never used. LaunchedEffect already provides a coroutine scope for the suspend call to room.presence.enter().
Proposed fix
`@Composable`
fun EnterPresenceComponent(room: Room) {
- val coroutineScope = rememberCoroutineScope()
-
LaunchedEffect(room) {
room.presence.enter(
jsonObject {
put("status", "Online")
}
)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Composable | |
| fun EnterPresenceComponent(room: Room) { | |
| val coroutineScope = rememberCoroutineScope() | |
| LaunchedEffect(room) { | |
| room.presence.enter( | |
| jsonObject { | |
| put("status", "Online") | |
| } | |
| ) | |
| } | |
| } | |
| `@Composable` | |
| fun EnterPresenceComponent(room: Room) { | |
| LaunchedEffect(room) { | |
| room.presence.enter( | |
| jsonObject { | |
| put("status", "Online") | |
| } | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/pages/docs/chat/rooms/presence.mdx` around lines 217 - 228, Remove the
unused coroutineScope variable declared in EnterPresenceComponent; since
LaunchedEffect provides a coroutine scope and you call the suspend function
room.presence.enter(...) inside it, delete the val coroutineScope =
rememberCoroutineScope() line so only LaunchedEffect remains invoking
room.presence.enter(...) in EnterPresenceComponent.
| ```kotlin | ||
| implementation 'com.ably.chat:chat:1.0.0' | ||
| implementation 'com.ably.chat:chat:1.1.0' | ||
| ``` | ||
|
|
||
| ```jetpack | ||
| implementation 'com.ably.chat:chat:1.1.0' | ||
| implementation 'com.ably.chat:chat-extensions-compose:1.1.0' | ||
| ``` | ||
| </Code> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code block language identifier mismatch.
The code blocks use ```kotlin and ```jetpack identifiers but contain Groovy syntax (using implementation 'string' instead of Kotlin DSL implementation("string")). Consider using ```groovy for accurate syntax highlighting, or adjust the documentation structure to clarify this is the Groovy equivalent.
🤖 Prompt for AI Agents
In `@src/pages/docs/chat/setup.mdx` around lines 137 - 145, The code blocks show
Groovy Gradle syntax but are labeled as kotlin/jetpack; update the block
language tags to ```groovy for both blocks (or alternatively convert the
dependency lines to the Kotlin DSL form
implementation("com.ably.chat:chat:1.1.0") and
implementation("com.ably.chat:chat-extensions-compose:1.1.0") to match a
```kotlin tag). Ensure the labels and the lines for the strings 'implementation
'com.ably.chat:chat:1.1.0'' and 'implementation
'com.ably.chat:chat-extensions-compose:1.1.0'' are consistent.
| ``` | ||
|
|
||
| ```jetpack | ||
| // Jetpack Compose handles cleanup automatically |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to remove similar jetpack snippets since cleanup is automatically handled as a part of compose lifecycle or give cleanup example when using subscribe directly
AndyTWF
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I run this in the review app, selecting jetpack as a language crashes the page with error
"console.js:36 Could not find the language 'jetpack', did you forget to load/include a language module?"
ttypic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! added couple of suggestions
| ```jetpack | ||
| @Composable | ||
| fun MyComponent(room: Room) { | ||
| var myMessageList by remember { mutableStateOf<List<Message>>(emptyList()) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Jetpack recommends
| var myMessageList by remember { mutableStateOf<List<Message>>(emptyList()) } | |
| val myMessageList by remember { mutableStateListOf<Message>() } |
| fun PresenceStatusUi(room: Room?) { | ||
| val members = room?.collectAsPresenceMembers() | ||
| val membersState = room?.collectAsPresenceMembers() | ||
| val members by membersState ?: remember { mutableStateOf(emptyList()) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid creating unnecessary "remembered" variable
| val members by membersState ?: remember { mutableStateOf(emptyList()) } | |
| val members = membersState?.value ?: emptyList() |
| @Composable | ||
| fun HistoryBeforeSubscribeComponent(room: Room) { | ||
| var messages by remember { mutableStateOf<List<String>>(emptyList()) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Jetpack recommends
| var messages by remember { mutableStateOf<List<String>>(emptyList()) } | |
| val myMessageList by remember { mutableStateListOf<Message>() } |
| Button(onClick = { | ||
| coroutineScope.launch { | ||
| // Send a 👍 reaction using the default type | ||
| room.messages.reactions.send(message, name = "👍") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking maybe we should wrap in try-catch block or somehow else show it can throw an unhandled exception that can lead to app crash if it won't be handled somewhere
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.