refactor!: promote compose extensions to stable #176
Conversation
WalkthroughCompose collector APIs and paging messages were refactored to return androidx.compose.runtime.State wrappers (status, occupancy, presence, typing, connection). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI
participant Collector as Compose Collector
participant Room as Room
participant Flow as Room Flow
Note over Collector: remember { mutableStateOf(initial) }
UI->>Collector: call collectAsOccupancy()
Collector->>Room: occupancy.get()
Room-->>Collector: OccupancyData
Collector->>Collector: state.value = OccupancyData
Collector->>Flow: subscribe to occupancy.asFlow()
loop updates
Flow-->>Collector: OccupancyData
Collector->>Collector: state.value = OccupancyData
end
Collector-->>UI: returns State<OccupancyData>
sequenceDiagram
autonumber
actor UI
participant Messages as MessagesComposable
participant MsgStream as MessageStream
participant Reactions as ReactionsFlow
UI->>Messages: mount (collectAsPagingMessagesState)
Messages->>MsgStream: subscribe()
Messages->>Reactions: asFlow() subscribe
par Message events
MsgStream-->>Messages: ChatMessageEvent
Messages->>Messages: replaceFirstWith / append to loaded
and Reaction events
Reactions-->>Messages: MessageReactionSummaryEvent
Messages->>Messages: mergeWith -> replaceFirstWith
end
UI->>Messages: call refresh()
Messages->>Messages: refresh() triggers reload / clears error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (22)
🚧 Files skipped from review as they are similar to previous changes (10)
🧰 Additional context used🧠 Learnings (5)📚 Learning: 2024-11-28T11:12:06.843ZApplied to files:
📚 Learning: 2024-11-28T11:11:20.423ZApplied to files:
📚 Learning: 2024-11-28T11:08:42.524ZApplied to files:
📚 Learning: 2024-11-28T11:09:34.042ZApplied to files:
📚 Learning: 2024-11-28T11:08:38.559ZApplied to files:
🧬 Code graph analysis (3)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt (1)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (2)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (23)
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 |
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: 2
🧹 Nitpick comments (6)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (2)
134-142: LGTM! Helper functions correctly update messages.Both helper functions properly:
- Find messages by serial using
indexOfFirst- Check if the message exists before updating
- Merge event data using the
withextensionThe silent no-op when a message isn't found (e.g., it's in an unloaded page) is appropriate for a paging composable.
Optional performance consideration:
The
indexOfFirstoperations are O(n) for each update. If the loaded list grows very large (thousands of messages) and updates become frequent, consider maintaining aMap<Serial, Int>index for O(1) lookups. However, for typical chat usage patterns, the current implementation should be sufficient.
56-58: Align subscription API patterns.The message subscription (
messages.subscribe) returns aMessagesSubscriptionwhile the reaction subscription (messages.reactions.subscribe) returns aSubscriptionwith a destructuringcomponent1()—this inconsistency may confuse users. Either unify both return types or clearly document the differing unsubscribe patterns.chat/src/commonMain/kotlin/com/ably/chat/annotations/Annotations.kt (1)
26-45: Add PROPERTY_GETTER to targets (and consider FILE).To ensure opt-in applies when accessing getters (esp. for top‑level vals), include PROPERTY_GETTER in the @target. FILE is optional but useful for file‑level scopes.
@Target( AnnotationTarget.CLASS, AnnotationTarget.TYPEALIAS, AnnotationTarget.FUNCTION, AnnotationTarget.PROPERTY, + AnnotationTarget.PROPERTY_GETTER, AnnotationTarget.FIELD, AnnotationTarget.CONSTRUCTOR, AnnotationTarget.PROPERTY_SETTER, ) public annotation class InternalChatApichat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt (1)
120-122: Annotate getter and add brief KDoc for EmptyOccupancyData.Annotating the getter makes the opt‑in explicit at accessor level; a short KDoc clarifies usage.
-@InternalChatApi -public val EmptyOccupancyData: OccupancyData = DefaultOccupancyData(0, 0) +/** + * An empty occupancy value (0 connections, 0 presence members). + * Internal; subject to change. + */ +@get:InternalChatApi +@InternalChatApi +public val EmptyOccupancyData: OccupancyData = DefaultOccupancyData(0, 0)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (2)
21-25: Prefer function-level @OptIn over local usage.Annotate the function to cover all InternalChatApi usages within the body.
@Composable -public fun Room.collectAsOccupancy(): OccupancyData { - @OptIn(InternalChatApi::class) - var currentOccupancy by remember(this) { mutableStateOf(EmptyOccupancyData) } +@OptIn(InternalChatApi::class) +public fun Room.collectAsOccupancy(): OccupancyData { + var currentOccupancy by remember(this) { mutableStateOf(EmptyOccupancyData) }
27-34: Optional: fetch initial occupancy without waiting for Attached.get() uses REST and doesn’t require attachment; you can prefetch before Attached to reduce latency, while still gating event subscription on Attached. Based on learnings.
📜 Review details
Configuration used: CodeRabbit 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 (12)
chat-extensions-compose/api/chat-extensions-compose.api(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt(0 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt(3 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(0 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt(0 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt(0 hunks)chat/api/android/chat.api(2 hunks)chat/api/jvm/chat.api(2 hunks)chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt(2 hunks)chat/src/commonMain/kotlin/com/ably/chat/annotations/Annotations.kt(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(1 hunks)
💤 Files with no reviewable changes (4)
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-28T11:11:20.423Z
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.
Applied to files:
chat/api/jvm/chat.apichat-extensions-compose/api/chat-extensions-compose.apichat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.ktchat/api/android/chat.api
📚 Learning: 2024-11-28T11:12:06.843Z
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.
Applied to files:
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt
🧬 Code graph analysis (2)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (2)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt (1)
collectAsStatus(16-26)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
collectAsStatus(16-26)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (1)
chat/src/commonMain/kotlin/com/ably/chat/Messages.kt (2)
get(67-67)get(551-554)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test (Android Release tests, testReleaseUnitTest)
- GitHub Check: coverage
- GitHub Check: test (Android Debug tests, testDebugUnitTest)
- GitHub Check: check
- GitHub Check: test (JVM tests, jvmTest)
- GitHub Check: build
🔇 Additional comments (8)
example/src/main/java/com/ably/chat/example/MainActivity.kt (1)
90-90: LGTM! Correctly removes experimental opt-in.The removal of
ExperimentalChatApifrom the opt-in annotation aligns with the PR objective of promoting compose extensions to stable. The compose functions used in this file (collectAsCurrentlyTypingandcollectAsPagingMessagesState) no longer require the experimental annotation.chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (3)
16-16: LGTM! Imports support new reaction handling.The added imports (
ChatMessageEvent,MessageReactionSummaryEvent, andwithextension) are necessary for the new reaction subscription feature and the refactored message update helpers.Also applies to: 20-20, 26-26
51-52: LGTM! Cleaner event handling.The refactored event handling for Updated and Deleted message types correctly delegates to the helper function, which finds and updates messages by serial.
60-64: LGTM! Proper cleanup order.The disposal correctly unsubscribes from both reactions and messages, ensuring no resource leaks. The order (reactions before messages) is appropriate.
chat/api/jvm/chat.api (2)
372-373: API surface addition for getEmptyOccupancyData looks good.Getter presence aligns with new top‑level EmptyOccupancyData.
Please confirm the intended stability level is internal/opt‑in only and that downstreams are not relying on this symbol without opting in.
630-632: New annotation type InternalChatApi exported.Matches common module; no issues spotted.
chat/api/android/chat.api (2)
372-373: Android API: getEmptyOccupancyData addition mirrors JVM.Consistent and OK.
630-632: Android API: InternalChatApi annotation exported.Consistent and OK.
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt
Show resolved
Hide resolved
chat/src/commonMain/kotlin/com/ably/chat/annotations/Annotations.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| occupancy.asFlow().collect { | ||
| if (initialOccupancyGet.isActive) initialOccupancyGet.cancelAndJoin() |
There was a problem hiding this comment.
Per coderabbit - do we need to more gracefully handle trying to use this with occupancy disabled?
There was a problem hiding this comment.
it will throw SDK exception right now that will result in a crush, do you think we should prevent it and log it instead?
There was a problem hiding this comment.
If this error is thrown it would indicate a coding error by the developer, so I think throwing the SDK exception being thrown and uncaught is appropriate behaviour (logging would somewhat mask the problem)
| private inline fun <T> MutableList<T>.replaceFirstWith(replacement: T, predicate: (T) -> Boolean) { | ||
| val index = indexOfFirst(predicate) | ||
| if (index != -1) set(index, replacement) | ||
| private fun MutableList<Message>.replaceFirstWith(event: ChatMessageEvent) { |
There was a problem hiding this comment.
Would it make more sense to do this in reverse order, as it's likely that the most recent messages will be updated?
There was a problem hiding this comment.
Yeah, it's implemented this way, the list is reversed. I need to add more documentation about this. The reason is how scrolling works for lazy lists on Android: it's tightly coupled with the list index, and it's really hard to make it behave naturally with standard order where we add new messages to the end.
Small explanation: when we're at the very bottom, we typically want the view to scroll down when a new message arrives, that works out of the box with reversed order and requires some action for standard order. But the worst case when we're lazy-loading older messages. It's impossible to avoid glitches. We add messages all indexes shift, which causes the view to scroll up automatically. If we try to scroll down back, glitches appear.
There was a problem hiding this comment.
Ah nice, if we can have a comment to that effect that would be great!
ttypic
force-pushed
the
compose-update
branch
from
bcbc1af to
1ea3272
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (1)
36-39: Guard against crashes when occupancy events are disabledOccupancy.subscribe throws when events are disabled; unguarded collect will crash the composition. Guard with options.occupancy.enableEvents and/or catch to degrade gracefully.
Apply:
val roomStatus = collectAsStatus() LaunchedEffect(this, roomStatus) { if (roomStatus != Attached) return@LaunchedEffect val initialOccupancyGet = launch { runCatching { currentOccupancy = occupancy.get() } } - occupancy.asFlow().collect { - if (initialOccupancyGet.isActive) initialOccupancyGet.cancelAndJoin() - currentOccupancy = it.occupancy - } + // If events are disabled, rely on initial GET and exit. + if (!options.occupancy.enableEvents) { + initialOccupancyGet.join() + return@LaunchedEffect + } + + runCatching { + occupancy.asFlow().collect { + if (initialOccupancyGet.isActive) initialOccupancyGet.cancelAndJoin() + currentOccupancy = it.occupancy + } + }.onFailure { + // Swallow; initial GET already provided a value or EmptyOccupancyData will remain. + } }
🧹 Nitpick comments (6)
chat/src/commonMain/kotlin/com/ably/chat/Room.kt (1)
116-118: Document clientId contract; confirm resolver behaviorPlease add brief KDoc stating when clientId is available and what exceptions (if any) can be thrown. Also confirm clientIdResolver.get() is fast/non-blocking; if it can fail, ensure callers are prepared to handle it.
Example KDoc:
+ /** + * The authenticated clientId for this room/session. + * Throws [ChatException] if unavailable or unauthenticated. + */ @InternalChatApi public val clientId: StringAlso applies to: 196-199
chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt (1)
120-122: LGTM; ensure intent of public empty valuePublic EmptyOccupancyData under @InternalChatApi looks fine. If the goal is to keep external usage gated, consider ensuring the annotation setup also covers getter usage for completeness (see comment in Annotations.kt).
chat/src/commonMain/kotlin/com/ably/chat/annotations/Annotations.kt (1)
36-45: Include PROPERTY_GETTER in target for completenessTo fully cover property access use-sites, add AnnotationTarget.PROPERTY_GETTER to InternalChatApi’s @target list.
@Target( AnnotationTarget.CLASS, AnnotationTarget.TYPEALIAS, AnnotationTarget.FUNCTION, AnnotationTarget.PROPERTY, + AnnotationTarget.PROPERTY_GETTER, AnnotationTarget.FIELD, AnnotationTarget.CONSTRUCTOR, AnnotationTarget.PROPERTY_SETTER, ) public annotation class InternalChatApichat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (2)
265-281: Map merge may drop keys present only in ‘other’mapValues() iterates only this map’s keys. Any reaction names present only in other are lost. Prefer a union.
Consider:
-@JvmName("mergeWithSummaryClientIdList") -private fun Map<String, SummaryClientIdList>.mergeWith(other: Map<String, SummaryClientIdList>): Map<String, SummaryClientIdList> = - mapValues { +@JvmName("mergeWithSummaryClientIdList") +private fun Map<String, SummaryClientIdList>.mergeWith(other: Map<String, SummaryClientIdList>): Map<String, SummaryClientIdList> = + (this.mapValues { if (it.value.clipped && !it.value.clientIds.containsAll(other[it.key]?.clientIds ?: listOf())) { SummaryClientIdList( it.value.total, buildSet { addAll(it.value.clientIds) other[it.key]?.clientIds?.let(::addAll) }.toList(), it.value.clipped, ) } else { it.value } - } + } + other.filterKeys { it !in this })
282-300: Same union issue for SummaryClientIdCounts mergeKeys present only in other are not included.
Apply:
-@JvmName("mergeWithSummaryClientIdCounts") -private fun Map<String, SummaryClientIdCounts>.mergeWith(other: Map<String, SummaryClientIdCounts>): Map<String, SummaryClientIdCounts> = - mapValues { +@JvmName("mergeWithSummaryClientIdCounts") +private fun Map<String, SummaryClientIdCounts>.mergeWith(other: Map<String, SummaryClientIdCounts>): Map<String, SummaryClientIdCounts> = + (this.mapValues { if (it.value.clipped && !it.value.clientIds.keys.containsAll(other[it.key]?.clientIds?.keys ?: setOf())) { SummaryClientIdCounts( total = it.value.total, clientIds = buildMap { putAll(it.value.clientIds) putAll(other[it.key]?.clientIds ?: emptyMap()) }, totalUnidentified = it.value.totalUnidentified, clipped = it.value.clipped, totalClientIds = it.value.totalClientIds, ) } else { it.value } - } + } + other.filterKeys { it !in this })chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (1)
63-69: Duplicate reaction subscriptions; risk of double updates/racesYou both subscribe via callbacks and collect asFlow() (which itself subscribes). This can cause redundant updates or overwrite enriched data.
Remove the direct subscribe and rely solely on asFlow() with enrichment:
- val (unsubscribeReactions) = messages.reactions.subscribe { event -> - loaded.replaceFirstWith(event) - } - onDispose { - unsubscribeReactions() subscription?.unsubscribe() subscription = null }
📜 Review details
Configuration used: CodeRabbit 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)
chat-extensions-compose/api/chat-extensions-compose.api(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt(0 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt(5 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(0 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt(0 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt(0 hunks)chat/api/android/chat.api(4 hunks)chat/api/jvm/chat.api(4 hunks)chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt(4 hunks)chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt(2 hunks)chat/src/commonMain/kotlin/com/ably/chat/Room.kt(3 hunks)chat/src/commonMain/kotlin/com/ably/chat/annotations/Annotations.kt(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(1 hunks)
💤 Files with no reviewable changes (4)
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.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/Room.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- example/src/main/java/com/ably/chat/example/MainActivity.kt
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-04-28T11:41:59.070Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#124
File: chat-android/src/main/java/com/ably/chat/Utils.kt:0-0
Timestamp: 2025-04-28T11:41:59.070Z
Learning: The `AwaitableSharedFlow` class in `com.ably.chat` intentionally uses `Channel.UNLIMITED` buffer capacity to ensure `tryEmit()` never fails due to buffer constraints, which is an important design requirement for the Ably Chat Kotlin SDK.
Applied to files:
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt
📚 Learning: 2024-11-28T11:11:20.423Z
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.
Applied to files:
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt
📚 Learning: 2024-11-28T11:12:06.843Z
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.
Applied to files:
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.ktchat-extensions-compose/api/chat-extensions-compose.api
📚 Learning: 2025-05-12T10:30:51.682Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#130
File: chat-android/src/main/java/com/ably/chat/RoomOptions.kt:197-201
Timestamp: 2025-05-12T10:30:51.682Z
Learning: In the RoomOptions.channelOptions() method, there won't be any existing params for the channel when setting occupancy parameters, so direct assignment with `params = mapOf("occupancy" to "metrics")` is appropriate without needing to merge with existing params.
Applied to files:
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt
🧬 Code graph analysis (2)
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (1)
chat/src/commonMain/kotlin/com/ably/chat/FlowUtils.kt (1)
transformCallbackAsFlow(12-15)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (1)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
collectAsStatus(16-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test (JVM tests, jvmTest)
- GitHub Check: check
- GitHub Check: test (Android Debug tests, testDebugUnitTest)
- GitHub Check: test (Android Release tests, testReleaseUnitTest)
- GitHub Check: coverage
- GitHub Check: build
🔇 Additional comments (5)
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (1)
96-101: Good Flow wrapper; buffer guarantees align with SDK designUsing transformCallbackAsFlow over subscribe unsubscribes correctly and, via Channel.UNLIMITED, prevents backpressure drops. LGTM.
Based on learnings
chat/api/jvm/chat.api (1)
275-281: API surface additions look consistent
- MessagesReactionsKt.asFlow and mergeWith are exposed.
- Room.getClientId getter added (used by compose).
- OccupancyKt.getEmptyOccupancyData and annotations.InternalChatApi surfaced.
No issues spotted.
Also applies to: 372-375, 449-450, 633-635
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (1)
54-61: Message update/delete handling looks correctreplaceFirstWith(event) via Message.with(event) is a clean approach for updates and deletes.
chat-extensions-compose/api/chat-extensions-compose.api (2)
10-11: Return type change to OccupancyData is appropriateMatches core API change away from CurrentOccupancy. LGTM.
13-21: PagingMessagesState as a concrete class with refresh()API surface reflects the implementation. No issues.
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt
Show resolved
Hide resolved
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt
Outdated
Show resolved
Hide resolved
ttypic
force-pushed
the
compose-update
branch
from
1ea3272 to
799974e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (1)
251-257: Fix mergeWith to include existing summary
reactions.mergeWith(reactions)merges the parameter with itself and drops the event’s current summary, so every call returns only the incoming fragment. Use the event’s existingreactionsas the left-hand side.Apply:
@InternalChatApi public fun MessageReactionSummaryEvent.mergeWith(reactions: MessageReactionSummary): MessageReactionSummaryEvent = DefaultMessageReactionSummaryEvent( messageSerial = messageSerial, type = type, - reactions = reactions.mergeWith(reactions), + reactions = this.reactions.mergeWith(reactions), )chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (2)
73-83: Fix undefined clientId reference.The
clientIdvariable is not defined in this scope. Usethis.clientIdto reference the Room's clientId property.Apply this diff:
LaunchedEffect(this) { @OptIn(InternalChatApi::class) messages.reactions.asFlow().collect { event -> - val enrichedEvent = if (event.hasClippedWithoutMyClientId(clientId)) { + val enrichedEvent = if (event.hasClippedWithoutMyClientId([email protected])) { event.mergeWith(messages.reactions.clientReactions(event.messageSerial)) } else { event } loaded.replaceFirstWith(enrichedEvent) } }
132-144: Fix inverted logic in hasClippedWithoutMyClientId.The predicate uses
allwhich returns true only if every entry passes. It should useanyto return true when at least one entry is clipped and does not contain the clientId.Apply this diff:
-private fun <T> Map<String, T>.hasClippedWithoutMyClientId(clientId: String) = all { - when (val value = it.value) { - is SummaryClientIdCounts -> !value.clipped || value.clientIds.contains(clientId) - is SummaryClientIdList -> !value.clipped || value.clientIds.contains(clientId) - else -> false - } -} +private fun <T> Map<String, T>.hasClippedWithoutMyClientId(clientId: String) = any { + when (val value = it.value) { + is SummaryClientIdCounts -> value.clipped && !value.clientIds.contains(clientId) + is SummaryClientIdList -> value.clipped && !value.clientIds.contains(clientId) + else -> false + } +}
🧹 Nitpick comments (1)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (1)
41-44: Consider extracting helper functions to reduce complexity.The suppression for
LongMethodandCognitiveComplexMethodindicates this function could benefit from extracting discrete responsibilities (e.g., reactions subscription setup, enrichment logic, pagination handling) into separate helper functions.
📜 Review details
Configuration used: CodeRabbit 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)
chat-extensions-compose/api/chat-extensions-compose.api(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt(0 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt(5 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(0 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt(0 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt(0 hunks)chat/api/android/chat.api(4 hunks)chat/api/jvm/chat.api(4 hunks)chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt(4 hunks)chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt(2 hunks)chat/src/commonMain/kotlin/com/ably/chat/Room.kt(3 hunks)chat/src/commonMain/kotlin/com/ably/chat/annotations/Annotations.kt(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(1 hunks)
💤 Files with no reviewable changes (4)
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- example/src/main/java/com/ably/chat/example/MainActivity.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-28T11:12:06.843Z
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.
Applied to files:
chat-extensions-compose/api/chat-extensions-compose.api
📚 Learning: 2025-04-28T11:41:59.070Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#124
File: chat-android/src/main/java/com/ably/chat/Utils.kt:0-0
Timestamp: 2025-04-28T11:41:59.070Z
Learning: The `AwaitableSharedFlow` class in `com.ably.chat` intentionally uses `Channel.UNLIMITED` buffer capacity to ensure `tryEmit()` never fails due to buffer constraints, which is an important design requirement for the Ably Chat Kotlin SDK.
Applied to files:
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt
🧬 Code graph analysis (1)
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (1)
chat/src/commonMain/kotlin/com/ably/chat/FlowUtils.kt (1)
transformCallbackAsFlow(12-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: coverage
- GitHub Check: build
- GitHub Check: check
- GitHub Check: test (Android Debug tests, testDebugUnitTest)
- GitHub Check: test (JVM tests, jvmTest)
- GitHub Check: test (Android Release tests, testReleaseUnitTest)
🔇 Additional comments (7)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (5)
16-31: LGTM!The new imports support the reaction enrichment functionality introduced in this PR.
53-70: LGTM!The reactions subscription is properly set up and disposed. The pattern correctly handles both message updates and reaction updates.
122-130: LGTM!The construction of PagingMessagesState with an internal constructor is correct and properly encapsulates the refresh logic.
146-155: LGTM! Breaking API change noted.Converting PagingMessagesState from an interface to a class with an internal constructor is appropriate and aligns with the PR's goal of promoting compose extensions to stable. The public
refresh()method provides the necessary functionality while maintaining encapsulation.
157-165: LGTM!The
replaceFirstWithoverloads correctly handle both message and reaction events by finding the message by serial and merging the event data using thewith()extension.chat-extensions-compose/api/chat-extensions-compose.api (2)
9-11: LGTM!The return type change from
CurrentOccupancytoOccupancyDataaligns with the PR's goal of promoting compose extensions to stable. The previously flagged test issue was resolved in commit 1ea3272.
13-21: LGTM!The conversion of
PagingMessagesStatefrom interface to class is reflected correctly in the API surface. The$stablefield is a Compose compiler marker for stability tracking.
ttypic
force-pushed
the
compose-update
branch
from
799974e to
eb33149
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (1)
138-144: Fix inverted clipping predicate (use any, not all)Return true when at least one entry is clipped and missing my clientId. Current all(...) does the opposite.
-private fun <T> Map<String, T>.hasClippedWithoutMyClientId(clientId: String) = all { - when (val value = it.value) { - is SummaryClientIdCounts -> !value.clipped || value.clientIds.contains(clientId) - is SummaryClientIdList -> !value.clipped || value.clientIds.contains(clientId) - else -> false - } -} +private fun <T> Map<String, T>.hasClippedWithoutMyClientId(clientId: String) = any { + when (val value = it.value) { + is SummaryClientIdCounts -> value.clipped && !value.clientIds.contains(clientId) + is SummaryClientIdList -> value.clipped && !value.clientIds.contains(clientId) + else -> false + } +}chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (1)
22-25: Guard against subscription when occupancy events are disabled (prevents Compose crash).If events are disabled, Occupancy.subscribe throws; collecting the flow will crash the composition. Guard or catch to degrade gracefully.
Apply:
@@ val roomStatus = collectAsStatus() @@ - val initialOccupancyGet = launch { + val initialOccupancyGet = launch { runCatching { currentOccupancy = occupancy.get() } } - occupancy.asFlow().collect { + // If events are disabled, rely on the initial GET and skip subscription. + if (!options.occupancy.enableEvents) { + initialOccupancyGet.join() + return@LaunchedEffect + } + + runCatching { + occupancy.asFlow().collect { if (initialOccupancyGet.isActive) initialOccupancyGet.cancelAndJoin() - currentOccupancy = it.occupancy - } + currentOccupancy = it.occupancy + } + }Also applies to: 32-39
🧹 Nitpick comments (4)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (2)
73-83: Qualify clientId to Room receiver to avoid shadowing in nested lambdasSafer to reference the Room receiver explicitly.
- val enrichedEvent = if (event.hasClippedWithoutMyClientId(clientId)) { + val enrichedEvent = if (event.hasClippedWithoutMyClientId([email protected])) {
122-129: refresh() only clears error; consider triggering a re-fetchAs written, refresh() won’t request more data unless shouldRequestMessages is already true. Resetting lastReceivedPaginatedResult will re-init pagination after an error.
- refreshLambda = { error = null }, + refreshLambda = { + error = null + lastReceivedPaginatedResult = null + },chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt (1)
23-24: Remove unnecessary ExperimentalChatApi opt-in (if compose APIs are now stable)If collectAsOccupancy and related APIs are no longer experimental, drop the opt-in to reduce noise.
-@OptIn(ExperimentalChatApi::class) -class OccupancyTest { +class OccupancyTest {chat/src/commonMain/kotlin/com/ably/chat/annotations/Annotations.kt (1)
26-45: InternalChatApi annotation looks good; consider including PROPERTY_GETTER in targetsThis helps when annotating only getters.
@Target( AnnotationTarget.CLASS, AnnotationTarget.TYPEALIAS, AnnotationTarget.FUNCTION, AnnotationTarget.PROPERTY, AnnotationTarget.FIELD, AnnotationTarget.CONSTRUCTOR, - AnnotationTarget.PROPERTY_SETTER, + AnnotationTarget.PROPERTY_SETTER, + AnnotationTarget.PROPERTY_GETTER, ) public annotation class InternalChatApi
📜 Review details
Configuration used: CodeRabbit 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 (15)
chat-extensions-compose/api/chat-extensions-compose.api(1 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt(0 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt(5 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(0 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt(0 hunks)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt(0 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt(3 hunks)chat/api/android/chat.api(4 hunks)chat/api/jvm/chat.api(4 hunks)chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt(4 hunks)chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt(2 hunks)chat/src/commonMain/kotlin/com/ably/chat/Room.kt(3 hunks)chat/src/commonMain/kotlin/com/ably/chat/annotations/Annotations.kt(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(1 hunks)
💤 Files with no reviewable changes (4)
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.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
🚧 Files skipped from review as they are similar to previous changes (1)
- example/src/main/java/com/ably/chat/example/MainActivity.kt
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-11-28T11:11:20.423Z
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.
Applied to files:
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt
📚 Learning: 2024-11-28T11:12:06.843Z
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.
Applied to files:
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.ktchat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.ktchat-extensions-compose/api/chat-extensions-compose.api
📚 Learning: 2025-04-28T11:41:59.070Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#124
File: chat-android/src/main/java/com/ably/chat/Utils.kt:0-0
Timestamp: 2025-04-28T11:41:59.070Z
Learning: The `AwaitableSharedFlow` class in `com.ably.chat` intentionally uses `Channel.UNLIMITED` buffer capacity to ensure `tryEmit()` never fails due to buffer constraints, which is an important design requirement for the Ably Chat Kotlin SDK.
Applied to files:
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt
🧬 Code graph analysis (2)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (1)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
collectAsStatus(16-26)
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (1)
chat/src/commonMain/kotlin/com/ably/chat/FlowUtils.kt (1)
transformCallbackAsFlow(12-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: coverage
- GitHub Check: build
- GitHub Check: check
- GitHub Check: test (Android Debug tests, testDebugUnitTest)
- GitHub Check: test (Android Release tests, testReleaseUnitTest)
- GitHub Check: test (JVM tests, jvmTest)
🔇 Additional comments (11)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (2)
53-60: Message update/delete replacement looks good
157-165: replaceFirstWith helpers are correct and efficientchat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt (3)
37-43: Assertions updated to new OccupancyData shape look good
53-59: Second test aligns with new flow semantics
95-104: Private OccupancyEvent factory and toPair helper are clear and scoped correctlychat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt (1)
120-122: EmptyOccupancyData addition is fine; annotation appropriately guards usagechat/src/commonMain/kotlin/com/ably/chat/Room.kt (2)
92-94: New Room.clientId surface is reasonable under InternalChatApi
169-172: DefaultRoom.clientId getter correctly delegates to resolverchat-extensions-compose/api/chat-extensions-compose.api (1)
10-11: Verify migration docs and call sitesRipgrep found no stale references to
CurrentOccupancyorinterface PagingMessagesState. Ensure the migration guide/CHANGELOG is updated and all call sites align with the breaking API changes.chat/api/android/chat.api (1)
274-281: Sync new API methods across all platform descriptors
Android and JVM now exposeMessagesReactionsKt.asFlow,mergeWith;OccupancyKt.getEmptyOccupancyData;Room.getClientId; and theInternalChatApiannotation. No iOS descriptor found in this repo—please confirm that any iOS (or other) API surfaces are updated accordingly.chat/api/jvm/chat.api (1)
274-281: APIs synchronized across available platforms
JVM and Android descriptors contain matchingasFlowandmergeWithentries; no additional platform descriptors found.
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt
Outdated
Show resolved
Hide resolved
ttypic
force-pushed
the
compose-update
branch
2 times, most recently
from
311df56 to
914a8b1
Compare
ttypic
force-pushed
the
compose-update
branch
from
914a8b1 to
45bc648
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt (1)
20-41: Guard against subscribe when presence events are disabled; wrap collect to avoid crashing.presence.asFlow().collect will throw if events are disabled; this crashes composition. Guard with options.presence.enableEvents and catch failures.
Apply:
@Composable -public fun Room.collectAsPresenceMembers(): State<List<PresenceMember>> { +public fun Room.collectAsPresenceMembers(): State<List<PresenceMember>> { val presenceMembers = remember(this) { mutableStateOf(emptyList<PresenceMember>()) } val roomStatus by collectAsStatus() LaunchedEffect(this, roomStatus) { - if (roomStatus != Attached) return@LaunchedEffect + if (roomStatus != Attached) return@LaunchedEffect val initialPresenceGet = launch { runCatching { presenceMembers.value = presence.get() } } - presence.asFlow().collect { - if (initialPresenceGet.isActive) initialPresenceGet.cancelAndJoin() - runCatching { - presenceMembers.value = presence.get() - } - } + // If events are disabled, just rely on the initial GET and skip subscription. + if (!options.presence.enableEvents) { + initialPresenceGet.join() + return@LaunchedEffect + } + runCatching { + presence.asFlow().collect { + if (initialPresenceGet.isActive) initialPresenceGet.cancelAndJoin() + presenceMembers.value = presence.get() + } + }.onFailure { + // Ignore; initial GET already provided a value. + } } return presenceMembers }Optional: to reduce network churn, consider updating presenceMembers from the Presence events instead of calling presence.get() for every event.
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (1)
69-94: KDoc example uses outdated API name.Use clientReactions(...) instead of getClientReactionSummary(...).
Apply:
- * val clientReactions = room.messages.reactions.getClientReactionSummary( + * val clientReactions = room.messages.reactions.clientReactions( * event.messageSerial, * myClientId * )
♻️ Duplicate comments (5)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (1)
22-43: Handle disabled occupancy events to prevent crashes; prefer stable EmptyOccupancyData accessor.
- Critical: occupancy.asFlow().collect will throw if events are disabled; guard or catch to avoid composition crash. [duplicate of prior feedback]
- Prefer getEmptyOccupancyData() to avoid InternalChatApi opt-in in Compose code.
Apply:
-import androidx.compose.runtime.State +import androidx.compose.runtime.State import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember -import com.ably.chat.EmptyOccupancyData +import com.ably.chat.getEmptyOccupancyData import com.ably.chat.OccupancyData import com.ably.chat.Room import com.ably.chat.RoomStatus.Attached -import com.ably.chat.annotations.InternalChatApi import com.ably.chat.asFlow @@ @Composable public fun Room.collectAsOccupancy(): State<OccupancyData> { - @OptIn(InternalChatApi::class) - val currentOccupancy = remember(this) { mutableStateOf(EmptyOccupancyData) } + val currentOccupancy = remember(this) { mutableStateOf(getEmptyOccupancyData()) } val roomStatus by collectAsStatus() @@ - occupancy.asFlow().collect { - if (initialOccupancyGet.isActive) initialOccupancyGet.cancelAndJoin() - currentOccupancy.value = it.occupancy - } + // If events are disabled, just rely on the initial GET and skip subscription. + if (!options.occupancy.enableEvents) { + initialOccupancyGet.join() + return@LaunchedEffect + } + runCatching { + occupancy.asFlow().collect { + if (initialOccupancyGet.isActive) initialOccupancyGet.cancelAndJoin() + currentOccupancy.value = it.occupancy + } + }.onFailure { + // Ignore; initial GET already provided a value. + } }chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (2)
251-257: Fix self-merge bug in MessageReactionSummaryEvent.mergeWith.Currently merges the parameter with itself, ignoring existing event summary.
Apply:
@InternalChatApi public fun MessageReactionSummaryEvent.mergeWith(reactions: MessageReactionSummary): MessageReactionSummaryEvent = DefaultMessageReactionSummaryEvent( messageSerial = messageSerial, type = type, - reactions = reactions.mergeWith(reactions), + reactions = this.reactions.mergeWith(reactions), )
265-300: Preserve incoming-only reaction keys when merging maps.mapValues iterates only current keys; incoming-only keys are dropped. Build union and merge per key.
Apply:
@JvmName("mergeWithSummaryClientIdList") -private fun Map<String, SummaryClientIdList>.mergeWith(other: Map<String, SummaryClientIdList>): Map<String, SummaryClientIdList> = - mapValues { - if (it.value.clipped && !it.value.clientIds.containsAll(other[it.key]?.clientIds ?: listOf())) { - SummaryClientIdList( - it.value.total, - buildSet { - addAll(it.value.clientIds) - other[it.key]?.clientIds?.let(::addAll) - }.toList(), - it.value.clipped, - ) - } else { - it.value - } - } +private fun Map<String, SummaryClientIdList>.mergeWith(other: Map<String, SummaryClientIdList>): Map<String, SummaryClientIdList> = + buildMap { + putAll(this@mergeWith) + other.forEach { (key, incoming) -> + val current = this@mergeWith[key] + when { + current == null -> put(key, incoming) + current.clipped && !current.clientIds.containsAll(incoming.clientIds) -> put( + key, + SummaryClientIdList( + total = current.total, + clientIds = (current.clientIds + incoming.clientIds).distinct(), + clipped = current.clipped, + ), + ) + else -> put(key, current) + } + } + } @JvmName("mergeWithSummaryClientIdCounts") -private fun Map<String, SummaryClientIdCounts>.mergeWith(other: Map<String, SummaryClientIdCounts>): Map<String, SummaryClientIdCounts> = - mapValues { - if (it.value.clipped && !it.value.clientIds.keys.containsAll(other[it.key]?.clientIds?.keys ?: setOf())) { - SummaryClientIdCounts( - total = it.value.total, - clientIds = buildMap { - putAll(it.value.clientIds) - putAll(other[it.key]?.clientIds ?: emptyMap()) - }, - totalUnidentified = it.value.totalUnidentified, - clipped = it.value.clipped, - totalClientIds = it.value.totalClientIds, - ) - } else { - it.value - } - } +private fun Map<String, SummaryClientIdCounts>.mergeWith(other: Map<String, SummaryClientIdCounts>): Map<String, SummaryClientIdCounts> = + buildMap { + putAll(this@mergeWith) + other.forEach { (key, incoming) -> + val current = this@mergeWith[key] + when { + current == null -> put(key, incoming) + current.clipped && !current.clientIds.keys.containsAll(incoming.clientIds.keys) -> put( + key, + SummaryClientIdCounts( + total = current.total, + clientIds = current.clientIds + incoming.clientIds, + totalUnidentified = current.totalUnidentified, + clipped = current.clipped, + totalClientIds = current.totalClientIds, + ), + ) + else -> put(key, current) + } + } + }chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (2)
69-79: Qualify clientId to avoid unresolved/ambiguous referenceUse the receiver-qualified property to ensure resolution inside the extension.
Apply:
- LaunchedEffect(this) { - @OptIn(InternalChatApi::class) - messages.reactions.asFlow().collect { event -> - val enrichedEvent = if (event.hasClippedWithoutMyClientId(clientId)) { + LaunchedEffect(this) { + @OptIn(InternalChatApi::class) + messages.reactions.asFlow().collect { event -> + val enrichedEvent = if (event.hasClippedWithoutMyClientId([email protected])) { event.mergeWith(messages.reactions.clientReactions(event.messageSerial)) } else { event } loaded.replaceFirstWith(enrichedEvent) } }
134-140: Fix inverted clipping predicate (use any + clipped && missing clientId)Currently returns true only if all entries are either not clipped or include my clientId. It should be true when at least one entry is clipped and does not include my clientId.
Apply:
-private fun <T> Map<String, T>.hasClippedWithoutMyClientId(clientId: String) = all { - when (val value = it.value) { - is SummaryClientIdCounts -> !value.clipped || value.clientIds.contains(clientId) - is SummaryClientIdList -> !value.clipped || value.clientIds.contains(clientId) - else -> false - } -} +private fun <T> Map<String, T>.hasClippedWithoutMyClientId(clientId: String) = any { + when (val value = it.value) { + is SummaryClientIdCounts -> value.clipped && !value.clientIds.contains(clientId) + is SummaryClientIdList -> value.clipped && !value.clientIds.contains(clientId) + else -> false + } +}
🧹 Nitpick comments (5)
chat/src/commonMain/kotlin/com/ably/chat/annotations/Annotations.kt (1)
26-45: Consider expanding @target to include PROPERTY_GETTER and FILE (optional).Helps annotate getters and entire files when needed; keeps parity with PROPERTY_SETTER.
Apply:
@Target( AnnotationTarget.CLASS, AnnotationTarget.TYPEALIAS, AnnotationTarget.FUNCTION, AnnotationTarget.PROPERTY, AnnotationTarget.FIELD, AnnotationTarget.CONSTRUCTOR, - AnnotationTarget.PROPERTY_SETTER, + AnnotationTarget.PROPERTY_SETTER, + AnnotationTarget.PROPERTY_GETTER, + AnnotationTarget.FILE, ) public annotation class InternalChatApichat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (4)
55-61: Ensure state mutations happen on the main threadIf messages.subscribe invokes callbacks off the main thread, mutating SnapshotStateList will crash. Dispatch to the composition scope.
Example:
val scope = rememberCoroutineScope() subscription = messages.subscribe { event -> scope.launch { when (event.type) { ChatMessageEventType.Created -> loaded.add(0, event.message) ChatMessageEventType.Updated -> loaded.replaceFirstWith(event) ChatMessageEventType.Deleted -> loaded.replaceFirstWith(event) } } }Import androidx.compose.runtime.rememberCoroutineScope and kotlinx.coroutines.launch.
118-126: Make refresh() actually refresh pagingCurrently refresh clears only error; it won’t re-request from the beginning. Also clear items and reset pagination pointer.
Apply:
- refreshLambda = { error = null }, + refreshLambda = { + error = null + loaded.clear() + lastReceivedPaginatedResult = null + },
42-44: Drop ExperimentalChatApi if promoting to stableThis API is still annotated experimental. If this module is now stable, remove the annotation and its import.
88-96: Optional: threshold comparisonConsider >= instead of > so fetching triggers exactly when remaining = scrollThreshold.
📜 Review details
Configuration used: CodeRabbit 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 (20)
chat-extensions-compose/api/chat-extensions-compose.api(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(5 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(3 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt(2 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)chat/api/android/chat.api(4 hunks)chat/api/jvm/chat.api(4 hunks)chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt(4 hunks)chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt(2 hunks)chat/src/commonMain/kotlin/com/ably/chat/Room.kt(3 hunks)chat/src/commonMain/kotlin/com/ably/chat/annotations/Annotations.kt(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(1 hunks)example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt
- chat/api/jvm/chat.api
- example/src/main/java/com/ably/chat/example/MainActivity.kt
- chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt
- chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2024-11-28T11:11:20.423Z
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.
Applied to files:
chat-extensions-compose/api/chat-extensions-compose.apichat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt
📚 Learning: 2024-11-28T11:12:06.843Z
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.
Applied to files:
chat-extensions-compose/api/chat-extensions-compose.apichat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.ktchat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt
📚 Learning: 2024-11-28T11:08:42.524Z
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`.
Applied to files:
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt
📚 Learning: 2024-12-02T12:10:18.954Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#75
File: chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt:43-45
Timestamp: 2024-12-02T12:10:18.954Z
Learning: In the `RoomFeatureSharedChannelTest.kt` tests, prefer using assertions like `Assert.assertEquals` that provide detailed error messages over general assertions like `Assert.assertTrue`, to aid in debugging when a test fails due to a missing mode.
Applied to files:
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt
📚 Learning: 2024-11-28T11:09:34.042Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/main/java/com/ably/chat/Room.kt:0-0
Timestamp: 2024-11-28T11:09:34.042Z
Learning: In the `DefaultRoom` class, the `ensureAttached()` method checks `statusLifecycle.status` again inside `roomScope.launch` to handle possible state changes, ensuring robustness.
Applied to files:
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt
📚 Learning: 2024-11-28T11:08:38.559Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/PresenceTest.kt:33-33
Timestamp: 2024-11-28T11:08:38.559Z
Learning: The `RoomEnsureAttachedTest.kt` file contains tests that verify room operations are only performed when the room is in the `ATTACHED` state, including scenarios where operations fail when the room is not attached, succeed when it is attached, and proper error handling for invalid room states.
Applied to files:
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt
📚 Learning: 2025-04-28T11:41:59.070Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#124
File: chat-android/src/main/java/com/ably/chat/Utils.kt:0-0
Timestamp: 2025-04-28T11:41:59.070Z
Learning: The `AwaitableSharedFlow` class in `com.ably.chat` intentionally uses `Channel.UNLIMITED` buffer capacity to ensure `tryEmit()` never fails due to buffer constraints, which is an important design requirement for the Ably Chat Kotlin SDK.
Applied to files:
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt
🧬 Code graph analysis (6)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
chat/src/commonMain/kotlin/com/ably/chat/Room.kt (1)
statusAsFlow(123-125)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt (1)
chat/src/commonMain/kotlin/com/ably/chat/Room.kt (1)
statusAsFlow(123-125)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (1)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
collectAsStatus(15-25)
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (2)
chat/src/commonMain/kotlin/com/ably/chat/FlowUtils.kt (1)
transformCallbackAsFlow(12-15)chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt (3)
subscribe(28-51)subscribe(35-35)subscribe(172-184)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (2)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt (1)
collectAsStatus(15-25)chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
collectAsStatus(15-25)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt (1)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
collectAsStatus(15-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check
- GitHub Check: build
- GitHub Check: coverage
- GitHub Check: test (Android Debug tests, testDebugUnitTest)
- GitHub Check: test (JVM tests, jvmTest)
- GitHub Check: test (Android Release tests, testReleaseUnitTest)
🔇 Additional comments (12)
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt (1)
36-36: LGTM! Test correctly updated for State-wrapped API.The test properly accesses
.valueon the State-wrapped result fromcollectAsPresenceMembers(), aligning with the API change from returningList<PresenceMember>toState<List<PresenceMember>>.Also applies to: 83-83
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt (1)
26-26: LGTM! Test correctly updated for State-wrapped API.The test properly accesses
.valueon the State-wrapped result fromcollectAsStatus(), aligning with the API change from returningRoomStatustoState<RoomStatus>.chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/ConnectionTest.kt (1)
26-26: LGTM! Test correctly updated for State-wrapped API.The test properly accesses
.valueon the State-wrapped result fromcollectAsStatus(), aligning with the API change from returningConnectionStatustoState<ConnectionStatus>.example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt (1)
14-14: LGTM! Idiomatic use of delegated property pattern.The change from
val members = room.collectAsPresenceMembers()toval members by room.collectAsPresenceMembers()correctly uses Compose's delegated property pattern for State observation. The addedgetValueimport enables this delegation, providing automatic recomposition when presence members change.Also applies to: 30-30
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/TypingTest.kt (1)
31-31: LGTM! Test correctly updated for State-wrapped API.The test properly accesses
.valueon the State-wrapped result fromcollectAsCurrentlyTyping(), aligning with the API change from returningSet<String>toState<Set<String>>.chat/src/commonMain/kotlin/com/ably/chat/Room.kt (2)
4-4: LGTM! Appropriate use of internal API annotation.The addition of the
clientIdproperty with@InternalChatApiannotation correctly exposes internal functionality while signaling that this API is not intended for general use.Also applies to: 92-93
169-171: LGTM! Correct implementation of clientId property.The implementation correctly delegates to
clientIdResolver.get()and maintains the@InternalChatApiannotation on the getter, ensuring consistent internal API boundaries.chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt (1)
5-5: LGTM! Well-executed State-wrapper pattern.The refactoring correctly:
- Returns
State<ConnectionStatus>instead ofConnectionStatus- Uses
rememberto create a stable state holder keyed to the Connection instance- Initializes state before collecting updates in
LaunchedEffect- Removes the
@ExperimentalChatApiannotation, properly promoting to stable APIThe implementation follows idiomatic Compose patterns and ensures proper recomposition when the connection status changes.
Also applies to: 16-24
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
5-5: LGTM! Well-executed State-wrapper pattern.The refactoring correctly:
- Returns
State<RoomStatus>instead ofRoomStatus- Uses
rememberto create a stable state holder keyed to the Room instance- Initializes state before collecting updates in
LaunchedEffect- Removes the
@ExperimentalChatApiannotation, properly promoting to stable APIThe implementation follows idiomatic Compose patterns and ensures proper recomposition when the room status changes.
Also applies to: 16-24
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (1)
96-102: LGTM: asFlow helper is idiomatic and non-blocking.Uses transformCallbackAsFlow with UNLIMITED buffer upstream; unsubscribe handled via awaitClose.
chat/api/android/chat.api (1)
274-281: Android and JVM API descriptors are in sync. All new entries (asFlow,mergeWith,getEmptyOccupancyData,getClientId,InternalChatApi) appear in bothchat/api/android/chat.apiandchat/api/jvm/chat.api. Verify the iOS descriptor for the same entries to avoid binary-compat CI failures.chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (1)
69-79: Nice: single reactions source via asFlow()Removing the direct reactions subscription avoids double updates/races. Good change.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (3)
251-257: Bug: self-merge of reactions; should merge current with incomingYou’re merging the parameter with itself, ignoring the event’s existing reactions.
Apply:
@InternalChatApi public fun MessageReactionSummaryEvent.mergeWith(reactions: MessageReactionSummary): MessageReactionSummaryEvent = DefaultMessageReactionSummaryEvent( messageSerial = messageSerial, type = type, - reactions = reactions.mergeWith(reactions), + reactions = this.reactions.mergeWith(reactions), )
265-281: Map merge drops incoming-only reaction keys; build union and mergeIterating only current keys via mapValues drops reactions present only in the incoming snapshot. Merge the union and preserve non-overlapping keys.
@JvmName("mergeWithSummaryClientIdList") private fun Map<String, SummaryClientIdList>.mergeWith(other: Map<String, SummaryClientIdList>): Map<String, SummaryClientIdList> = - mapValues { - if (it.value.clipped && !it.value.clientIds.containsAll(other[it.key]?.clientIds ?: listOf())) { - SummaryClientIdList( - it.value.total, - buildSet { - addAll(it.value.clientIds) - other[it.key]?.clientIds?.let(::addAll) - }.toList(), - it.value.clipped, - ) - } else { - it.value - } - } + buildMap { + putAll(this@mergeWith) + other.forEach { (key, incoming) -> + val current = this@mergeWith[key] + when { + current == null -> put(key, incoming) + current.clipped && !current.clientIds.containsAll(incoming.clientIds) -> put( + key, + SummaryClientIdList( + total = current.total, + clientIds = (current.clientIds + incoming.clientIds).distinct(), + clipped = current.clipped, + ), + ) + else -> put(key, current) + } + } + }
282-300: Counts map merge also drops incoming-only keys; use union and combine mapsSame issue as above; ensure incoming-only keys are preserved and merge clientId maps when clipped.
@JvmName("mergeWithSummaryClientIdCounts") private fun Map<String, SummaryClientIdCounts>.mergeWith(other: Map<String, SummaryClientIdCounts>): Map<String, SummaryClientIdCounts> = - mapValues { - if (it.value.clipped && !it.value.clientIds.keys.containsAll(other[it.key]?.clientIds?.keys ?: setOf())) { - SummaryClientIdCounts( - total = it.value.total, - clientIds = buildMap { - putAll(it.value.clientIds) - putAll(other[it.key]?.clientIds ?: emptyMap()) - }, - totalUnidentified = it.value.totalUnidentified, - clipped = it.value.clipped, - totalClientIds = it.value.totalClientIds, - ) - } else { - it.value - } - } + buildMap { + putAll(this@mergeWith) + other.forEach { (key, incoming) -> + val current = this@mergeWith[key] + when { + current == null -> put(key, incoming) + current.clipped && !current.clientIds.keys.containsAll(incoming.clientIds.keys) -> put( + key, + SummaryClientIdCounts( + total = current.total, + clientIds = current.clientIds + incoming.clientIds, + totalUnidentified = current.totalUnidentified, + clipped = current.clipped, + totalClientIds = current.totalClientIds, + ), + ) + else -> put(key, current) + } + } + }chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (2)
69-79: Undefined symbol: use Room.clientId for clipping checkReference the Room’s clientId in this scope.
- @OptIn(InternalChatApi::class) - messages.reactions.asFlow().collect { event -> - val enrichedEvent = if (event.hasClippedWithoutMyClientId(clientId)) { + @OptIn(InternalChatApi::class) + messages.reactions.asFlow().collect { event -> + val enrichedEvent = if (event.hasClippedWithoutMyClientId([email protected])) { event.mergeWith(messages.reactions.clientReactions(event.messageSerial)) } else { event } loaded.replaceFirstWith(enrichedEvent) }
134-140: Predicate logic inverted; use any(...) and test for clipped-and-missingShould return true if at least one entry is clipped and missing my clientId.
-private fun <T> Map<String, T>.hasClippedWithoutMyClientId(clientId: String) = all { - when (val value = it.value) { - is SummaryClientIdCounts -> !value.clipped || value.clientIds.contains(clientId) - is SummaryClientIdList -> !value.clipped || value.clientIds.contains(clientId) - else -> false - } -} +private fun <T> Map<String, T>.hasClippedWithoutMyClientId(clientId: String) = any { + when (val value = it.value) { + is SummaryClientIdCounts -> value.clipped && !value.clientIds.contains(clientId) + is SummaryClientIdList -> value.clipped && !value.clientIds.contains(clientId) + else -> false + } +}chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (1)
36-39: Guard subscription failures to avoid crashing when occupancy events are disabledOccupancy.subscribe may throw; unhandled, this LaunchedEffect will crash the composition.
Apply:
- occupancy.asFlow().collect { - if (initialOccupancyGet.isActive) initialOccupancyGet.cancelAndJoin() - currentOccupancy.value = it.occupancy - } + runCatching { + occupancy.asFlow().collect { + if (initialOccupancyGet.isActive) initialOccupancyGet.cancelAndJoin() + currentOccupancy.value = it.occupancy + } + }.onFailure { + // Swallow; rely on initial GET result. + }
🧹 Nitpick comments (2)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt (1)
15-24: Prefer produceState with initial current() and distinctUntilChangedAvoid empty-first render and redundant recompositions by mapping the flow to sets, distincting, and using produceState.
Apply this diff to simplify the body:
-@Composable -public fun Room.collectAsCurrentlyTyping(): State<Set<String>> { - val currentlyTyping = remember(this) { mutableStateOf(emptySet<String>()) } - - LaunchedEffect(this) { - typing.asFlow().collect { - currentlyTyping.value = it.currentlyTyping - } - } - - return currentlyTyping -} +@Composable +public fun Room.collectAsCurrentlyTyping(): State<Set<String>> = + produceState(initialValue = typing.current(), key1 = this) { + typing.asFlow() + .map { it.currentlyTyping } + .distinctUntilChanged() + .collect { value = it } + }Also add imports:
import androidx.compose.runtime.produceState import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.mapchat/src/commonMain/kotlin/com/ably/chat/annotations/Annotations.kt (1)
31-45: Add explicit Retention(BINARY) to InternalChatApi (and ExperimentalChatApi for consistency)Opt-in markers don’t need runtime retention. Using BINARY reduces runtime surface and matches common practice.
@MustBeDocumented @RequiresOptIn( level = RequiresOptIn.Level.ERROR, message = "This API is not intended for external use and is subject to change or removal in future releases.", ) +@Retention(AnnotationRetention.BINARY) @Target( AnnotationTarget.CLASS, AnnotationTarget.TYPEALIAS, AnnotationTarget.FUNCTION, AnnotationTarget.PROPERTY, AnnotationTarget.FIELD, AnnotationTarget.CONSTRUCTOR, AnnotationTarget.PROPERTY_SETTER, ) public annotation class InternalChatApiOptionally apply the same Retention to ExperimentalChatApi.
📜 Review details
Configuration used: CodeRabbit 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 (20)
chat-extensions-compose/api/chat-extensions-compose.api(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(5 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(3 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt(2 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)chat/api/android/chat.api(4 hunks)chat/api/jvm/chat.api(4 hunks)chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt(4 hunks)chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt(2 hunks)chat/src/commonMain/kotlin/com/ably/chat/Room.kt(3 hunks)chat/src/commonMain/kotlin/com/ably/chat/annotations/Annotations.kt(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(1 hunks)example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/TypingTest.kt
- example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt
- chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/ConnectionTest.kt
- chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2024-11-28T11:08:42.524Z
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`.
Applied to files:
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt
📚 Learning: 2024-11-28T11:12:06.843Z
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.
Applied to files:
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.ktchat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.ktchat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.ktchat-extensions-compose/api/chat-extensions-compose.api
📚 Learning: 2024-11-28T11:09:34.042Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/main/java/com/ably/chat/Room.kt:0-0
Timestamp: 2024-11-28T11:09:34.042Z
Learning: In the `DefaultRoom` class, the `ensureAttached()` method checks `statusLifecycle.status` again inside `roomScope.launch` to handle possible state changes, ensuring robustness.
Applied to files:
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt
📚 Learning: 2024-11-28T11:08:38.559Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/PresenceTest.kt:33-33
Timestamp: 2024-11-28T11:08:38.559Z
Learning: The `RoomEnsureAttachedTest.kt` file contains tests that verify room operations are only performed when the room is in the `ATTACHED` state, including scenarios where operations fail when the room is not attached, succeed when it is attached, and proper error handling for invalid room states.
Applied to files:
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt
📚 Learning: 2024-11-28T11:11:20.423Z
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.
Applied to files:
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.ktchat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt
📚 Learning: 2025-04-28T11:41:59.070Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#124
File: chat-android/src/main/java/com/ably/chat/Utils.kt:0-0
Timestamp: 2025-04-28T11:41:59.070Z
Learning: The `AwaitableSharedFlow` class in `com.ably.chat` intentionally uses `Channel.UNLIMITED` buffer capacity to ensure `tryEmit()` never fails due to buffer constraints, which is an important design requirement for the Ably Chat Kotlin SDK.
Applied to files:
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt
🧬 Code graph analysis (5)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
chat/src/commonMain/kotlin/com/ably/chat/Room.kt (1)
statusAsFlow(123-125)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (1)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
collectAsStatus(15-25)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt (1)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
collectAsStatus(15-25)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (1)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
collectAsStatus(15-25)
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (1)
chat/src/commonMain/kotlin/com/ably/chat/FlowUtils.kt (1)
transformCallbackAsFlow(12-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test (JVM tests, jvmTest)
- GitHub Check: test (Android Debug tests, testDebugUnitTest)
- GitHub Check: test (Android Release tests, testReleaseUnitTest)
- GitHub Check: check
- GitHub Check: build
- GitHub Check: coverage
🔇 Additional comments (14)
chat-extensions-compose/api/chat-extensions-compose.api (2)
2-2: LGTM! State-based API migration is consistent.All collection functions now properly return
Statewrappers instead of raw types. This aligns with Compose best practices and makes the API more stable and reactive.Also applies to: 10-10, 24-24, 28-28, 32-32
13-21: PagingMessagesState class transition looks good.Converting from an interface to a concrete class with
$stablefield is appropriate for Compose's stability inference system, which helps optimize recomposition.chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt (1)
26-26: Test correctly adapted to State-based API.Accessing
.valueon theStatewrapper is the correct pattern for observing state changes in tests usingmoleculeFlow.chat/src/commonMain/kotlin/com/ably/chat/Room.kt (1)
92-93: Clean implementation of clientId property.The property is appropriately annotated with
@InternalChatApiand delegates toclientIdResolver.get(), ensuring consistent client ID resolution across the Room API.Also applies to: 169-171
chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt (1)
120-121: EmptyOccupancyData constant is well-defined.Providing a reusable empty occupancy constant initialized to zero values is clean and follows the pattern of other data classes in the codebase. The
@InternalChatApiannotation appropriately marks this for internal use.chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt (2)
35-35: Tests correctly adapted to State-based API.Accessing
.valueon theStatewrapper is the proper pattern for testing the new API surface.Also applies to: 51-51
37-42: Pair-based comparison pattern is clean and readable.The
toPair()extension function provides a simple way to compare occupancy data in tests, making assertions more concise while maintaining clarity.Also applies to: 53-58, 104-104
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Connection.kt (1)
16-24: State-based implementation follows Compose best practices.The pattern is correct:
remember(this)ties state lifecycle to the Connection instance- Initial state synchronized with current status
LaunchedEffectproperly handles side effects and flow collection- State updates trigger recomposition as expected
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
16-24: Consistent State-based implementation for Room status.The implementation mirrors the pattern used in
Connection.kt, ensuring consistency across the codebase. The use ofstatusAsFlow()extension properly integrates with the existing Room status change mechanism.example/src/main/java/com/ably/chat/example/MainActivity.kt (2)
90-90: API graduation to stable is complete.Removing
ExperimentalChatApiconfirms the Compose extensions are now stable and ready for production use.
95-95: Example correctly uses State-based typing API.Accessing
.valueoncollectAsCurrentlyTyping()demonstrates the proper usage pattern for the new stable API.chat/api/jvm/chat.api (1)
274-281: API surface additions look consistent with Android descriptorMessagesReactionsKt.asFlow/mergeWith, OccupancyKt.getEmptyOccupancyData, Room.getClientId, and InternalChatApi are present. LGTM.
Also applies to: 372-375, 445-451, 633-635
chat/api/android/chat.api (1)
274-281: Ensure iOS API descriptor is updated with new symbolsNo
chat/api/ios/chat.apifile was found in this branch. Please verify that the iOS API descriptor (wherever it’s maintained) includes:
- abstract fun getClientId()
- OccupancyKt.getEmptyOccupancyData
- MessagesReactionsKt.asFlow
- MessagesReactionsKt.mergeWith
- com/ably/chat/annotations/InternalChatApi
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (1)
96-101: asFlow() wrapper looks goodCorrectly wraps subscribe into Flow using transformCallbackAsFlow with unlimited buffering. Based on learnings
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt
Show resolved
Hide resolved
| * loading progress, availability of additional data, and any errors encountered | ||
| * during the loading process. | ||
| * | ||
| * @property loaded A list of messages that have been loaded so far in reversed order (the most recent message is first). |
There was a problem hiding this comment.
@AndyTWF commented here on the returned type, wdyt?
ttypic
force-pushed
the
compose-update
branch
from
45bc648 to
e56b97a
Compare
ttypic
force-pushed
the
compose-update
branch
from
e56b97a to
1d695c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (1)
265-300: Still dropping incoming-only reaction keys—merge must cover the union.
The new merge helpers still iterate only the current map (mapValues), so any reaction that appears exclusively in the incoming snapshot is thrown away (e.g. merging{}with{ "👍": … }yields{}). That makes the merged summaries lose newly observed reactions. Build the union of keys and merge each entry so both existing and incoming reactions survive.Apply:
@JvmName("mergeWithSummaryClientIdList") private fun Map<String, SummaryClientIdList>.mergeWith(other: Map<String, SummaryClientIdList>): Map<String, SummaryClientIdList> = - mapValues { - if (it.value.clipped && !it.value.clientIds.containsAll(other[it.key]?.clientIds ?: listOf())) { - SummaryClientIdList( - it.value.total, - buildSet { - addAll(it.value.clientIds) - other[it.key]?.clientIds?.let(::addAll) - }.toList(), - it.value.clipped, - ) - } else { - it.value - } - } + buildMap { + (keys + other.keys).forEach { key -> + val current = this@mergeWith[key] + val incoming = other[key] + put( + key, + when { + current == null -> incoming!! + incoming == null -> current + current.clipped && !current.clientIds.containsAll(incoming.clientIds) -> + SummaryClientIdList( + total = current.total, + clientIds = (current.clientIds + incoming.clientIds).distinct(), + clipped = current.clipped, + ) + else -> current + }, + ) + } + } @JvmName("mergeWithSummaryClientIdCounts") private fun Map<String, SummaryClientIdCounts>.mergeWith(other: Map<String, SummaryClientIdCounts>): Map<String, SummaryClientIdCounts> = - mapValues { - if (it.value.clipped && !it.value.clientIds.keys.containsAll(other[it.key]?.clientIds?.keys ?: setOf())) { - SummaryClientIdCounts( - total = it.value.total, - clientIds = buildMap { - putAll(it.value.clientIds) - putAll(other[it.key]?.clientIds ?: emptyMap()) - }, - totalUnidentified = it.value.totalUnidentified, - clipped = it.value.clipped, - totalClientIds = it.value.totalClientIds, - ) - } else { - it.value - } - } + buildMap { + (keys + other.keys).forEach { key -> + val current = this@mergeWith[key] + val incoming = other[key] + put( + key, + when { + current == null -> incoming!! + incoming == null -> current + current.clipped && !current.clientIds.keys.containsAll(incoming.clientIds.keys) -> + SummaryClientIdCounts( + total = current.total, + clientIds = current.clientIds + incoming.clientIds, + totalUnidentified = current.totalUnidentified, + clipped = current.clipped, + totalClientIds = current.totalClientIds, + ) + else -> current + }, + ) + } + }chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt (1)
27-37: Stop calling presence.get() on every event; update incrementally from eventsFetch once, then update state from PresenceEvent to avoid repeated network/work overhead and recompositions.
Apply:
- val initialPresenceGet = launch { - runCatching { - presenceMembers.value = presence.get() - } - } - presence.asFlow().collect { - if (initialPresenceGet.isActive) initialPresenceGet.cancelAndJoin() - runCatching { - presenceMembers.value = presence.get() - } - } + // Initial snapshot + presenceMembers.value = runCatching { presence.get() }.getOrDefault(emptyList()) + // Incremental updates + presence.asFlow().collect { event -> + presenceMembers.value = presenceMembers.value.updateWith(event) + }Add helper in this file:
import com.ably.chat.PresenceEvent import com.ably.chat.PresenceEventType import com.ably.chat.PresenceMember private fun List<PresenceMember>.updateWith(event: PresenceEvent): List<PresenceMember> = when (event.type) { PresenceEventType.Enter, PresenceEventType.Present -> { val updated = toMutableList() val idx = indexOfFirst { it.clientId == event.member.clientId && it.connectionId == event.member.connectionId } if (idx >= 0) updated[idx] = event.member else updated += event.member updated } PresenceEventType.Update -> { val updated = toMutableList() val idx = indexOfFirst { it.clientId == event.member.clientId && it.connectionId == event.member.connectionId } if (idx >= 0) updated[idx] = event.member updated } PresenceEventType.Leave -> { filterNot { it.clientId == event.member.clientId && it.connectionId == event.member.connectionId } } else -> this }Optionally add distinctUntilChanged() on the flow to reduce recompositions.
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (1)
85-94: Compile error: unresolved ‘clientId’Use Room.clientId from the receiver.
- LaunchedEffect(this) { - @OptIn(InternalChatApi::class) - messages.reactions.asFlow().map { event -> - if (event.hasClippedWithoutMyClientId(clientId)) { + LaunchedEffect(this) { + @OptIn(InternalChatApi::class) + messages.reactions.asFlow().map { event -> + if (event.hasClippedWithoutMyClientId([email protected])) { event.mergeWith(messages.reactions.clientReactions(event.messageSerial)) } else { event } }.collect { loaded.replaceFirstWith(it) } }
🧹 Nitpick comments (1)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (1)
36-39: Reduce recompositions: distinctUntilChanged on occupancy updates (optional)Avoid emitting identical OccupancyData repeatedly.
- occupancy.asFlow().collect { - if (initialOccupancyGet.isActive) initialOccupancyGet.cancelAndJoin() - currentOccupancy.value = it.occupancy - } + occupancy.asFlow() + .map { it.occupancy } + .distinctUntilChanged() + .collect { occ -> + if (initialOccupancyGet.isActive) initialOccupancyGet.cancelAndJoin() + currentOccupancy.value = occ + }Add imports:
import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.distinctUntilChanged
📜 Review details
Configuration used: CodeRabbit 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 (22)
chat-extensions-compose/api/chat-extensions-compose.api(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(3 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/MessagesTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt(3 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt(2 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)chat/api/android/chat.api(4 hunks)chat/api/jvm/chat.api(4 hunks)chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt(4 hunks)chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt(2 hunks)chat/src/commonMain/kotlin/com/ably/chat/Room.kt(3 hunks)chat/src/commonMain/kotlin/com/ably/chat/annotations/Annotations.kt(1 hunks)chat/src/commonTest/kotlin/com/ably/chat/MessagesReactionsKtTest.kt(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(2 hunks)example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- chat/src/commonMain/kotlin/com/ably/chat/Room.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt
- chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt
- chat/api/android/chat.api
- chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt
- chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt
- chat/src/commonMain/kotlin/com/ably/chat/annotations/Annotations.kt
- example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt
- chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/ConnectionTest.kt
- chat/api/jvm/chat.api
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-11-28T11:12:06.843Z
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.
Applied to files:
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.ktchat-extensions-compose/api/chat-extensions-compose.apichat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt
📚 Learning: 2024-11-28T11:11:20.423Z
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.
Applied to files:
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.ktchat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt
📚 Learning: 2025-04-28T11:41:59.070Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#124
File: chat-android/src/main/java/com/ably/chat/Utils.kt:0-0
Timestamp: 2025-04-28T11:41:59.070Z
Learning: The `AwaitableSharedFlow` class in `com.ably.chat` intentionally uses `Channel.UNLIMITED` buffer capacity to ensure `tryEmit()` never fails due to buffer constraints, which is an important design requirement for the Ably Chat Kotlin SDK.
Applied to files:
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt
🧬 Code graph analysis (4)
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (1)
chat/src/commonMain/kotlin/com/ably/chat/FlowUtils.kt (1)
transformCallbackAsFlow(12-15)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (2)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
collectAsStatus(15-25)chat/src/commonMain/kotlin/com/ably/chat/Discontinuity.kt (1)
discontinuityAsFlow(70-72)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt (1)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
collectAsStatus(15-25)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (1)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
collectAsStatus(15-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check
- GitHub Check: coverage
- GitHub Check: test (JVM tests, jvmTest)
- GitHub Check: test (Android Release tests, testReleaseUnitTest)
- GitHub Check: test (Android Debug tests, testDebugUnitTest)
- GitHub Check: build
🔇 Additional comments (5)
example/src/main/java/com/ably/chat/example/MainActivity.kt (3)
90-90: LGTM: Experimental API annotation removed.The removal of
ExperimentalChatApifrom the opt-in annotation correctly reflects the promotion of compose extensions to stable API status.
95-95: LGTM: State value access correctly implemented.The addition of
.valueproperly accesses the current value from theStateobject returned bycollectAsCurrentlyTyping(), which aligns with the new State-based compose extensions API.
153-153: Verify the scrollThreshold reduction is intentional.The
scrollThresholdhas been reduced from 10 to 1, which will trigger pagination much more aggressively (when the user is just 1 item away from the end instead of 10). This could lead to more frequent network requests as users scroll.Please confirm this change is intentional and aligns with the desired UX behavior.
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/MessagesTest.kt (1)
13-280: Good coverage for hasClippedWithoutMyClientIdScenarios read well and align with any(...) semantics across unique/distinct/multiple.
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (1)
43-46: Confirm stability: drop ExperimentalChatApi?PR title suggests promoting to stable. Should this function shed @ExperimentalChatApi and its import?
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt
Show resolved
Hide resolved
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt
Show resolved
Hide resolved
ttypic
force-pushed
the
compose-update
branch
2 times, most recently
from
532d9c4 to
88a302d
Compare
ttypic
force-pushed
the
compose-update
branch
from
88a302d to
d21e5f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
chat/api/android/chat.api (1)
272-279: API additions look good; please re-verify cross-descriptor syncAndroid’s descriptor reflects the new APIs. Reconfirm jvm/ios descriptors include the same entries to avoid binary-compat CI failures.
#!/bin/bash set -euo pipefail echo "Checking API additions across all platform descriptors..." rg -nP -C2 'com/ably/chat/MessagesReactionsKt[^\n]*asFlow|com/ably/chat/MessagesReactionsKt[^\n]*mergeWith|com/ably/chat/OccupancyKt[^\n]*getEmptyOccupancyData|com/ably/chat/Room[^\n]*getClientId\(\)|com/ably/chat/annotations/InternalChatApi' chat/apiAlso applies to: 370-373, 447-448, 631-633
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt (1)
24-37: Don’t call presence.get() on every event; fetch once, then incrementally updateFetch initial members once, then update state from each PresenceEvent to avoid repeated network/SDK calls on every emission.
- LaunchedEffect(this, roomStatus) { - if (roomStatus != Attached) return@LaunchedEffect - - val initialPresenceGet = launch { - runCatching { - presenceMembers.value = presence.get() - } - } - presence.asFlow().collect { - if (initialPresenceGet.isActive) initialPresenceGet.cancelAndJoin() - runCatching { - presenceMembers.value = presence.get() - } - } - } + LaunchedEffect(this, roomStatus) { + if (roomStatus != Attached) return@LaunchedEffect + // Initial snapshot + presenceMembers.value = runCatching { presence.get() }.getOrDefault(emptyList()) + // Incremental updates + presence.asFlow().collect { event -> + presenceMembers.value = presenceMembers.value.updateWith(event) + } + }Add helper in the same file:
import com.ably.chat.PresenceEvent import com.ably.chat.PresenceEventType private fun List<PresenceMember>.updateWith(event: PresenceEvent): List<PresenceMember> = when (event.type) { PresenceEventType.Enter, PresenceEventType.Present -> { val updated = toMutableList() val idx = indexOfFirst { it.clientId == event.member.clientId && it.connectionId == event.member.connectionId } if (idx >= 0) updated[idx] = event.member else updated += event.member updated } PresenceEventType.Update -> { val updated = toMutableList() val idx = indexOfFirst { it.clientId == event.member.clientId && it.connectionId == event.member.connectionId } if (idx >= 0) updated[idx] = event.member updated } PresenceEventType.Leave -> { filterNot { it.clientId == event.member.clientId && it.connectionId == event.member.connectionId } } else -> this }Optionally apply distinctUntilChanged() on the flow to reduce recompositions.
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (1)
265-300: Fix map merges: preserve incoming-only keys and properly union entriesCurrent implementations iterate only current keys, dropping keys that exist only in the incoming map. Build the union and merge per key.
@JvmName("mergeWithSummaryClientIdList") -private fun Map<String, SummaryClientIdList>.mergeWith(other: Map<String, SummaryClientIdList>): Map<String, SummaryClientIdList> = - mapValues { - if (it.value.clipped && !it.value.clientIds.containsAll(other[it.key]?.clientIds ?: listOf())) { - SummaryClientIdList( - it.value.total, - buildSet { - addAll(it.value.clientIds) - other[it.key]?.clientIds?.let(::addAll) - }.toList(), - it.value.clipped, - ) - } else { - it.value - } - } +private fun Map<String, SummaryClientIdList>.mergeWith(other: Map<String, SummaryClientIdList>): Map<String, SummaryClientIdList> = + buildMap { + putAll(this@mergeWith) + other.forEach { (key, incoming) -> + val current = this@mergeWith[key] + when { + current == null -> put(key, incoming) + current.clipped && !current.clientIds.containsAll(incoming.clientIds) -> put( + key, + SummaryClientIdList( + total = current.total, + clientIds = (current.clientIds + incoming.clientIds).distinct(), + clipped = current.clipped, + ), + ) + else -> put(key, current) + } + } + } @JvmName("mergeWithSummaryClientIdCounts") -private fun Map<String, SummaryClientIdCounts>.mergeWith(other: Map<String, SummaryClientIdCounts>): Map<String, SummaryClientIdCounts> = - mapValues { - if (it.value.clipped && !it.value.clientIds.keys.containsAll(other[it.key]?.clientIds?.keys ?: setOf())) { - SummaryClientIdCounts( - total = it.value.total, - clientIds = buildMap { - putAll(it.value.clientIds) - putAll(other[it.key]?.clientIds ?: emptyMap()) - }, - totalUnidentified = it.value.totalUnidentified, - clipped = it.value.clipped, - totalClientIds = it.value.totalClientIds, - ) - } else { - it.value - } - } +private fun Map<String, SummaryClientIdCounts>.mergeWith(other: Map<String, SummaryClientIdCounts>): Map<String, SummaryClientIdCounts> = + buildMap { + putAll(this@mergeWith) + other.forEach { (key, incoming) -> + val current = this@mergeWith[key] + when { + current == null -> put(key, incoming) + current.clipped && !current.clientIds.keys.containsAll(incoming.clientIds.keys) -> put( + key, + SummaryClientIdCounts( + total = current.total, + clientIds = current.clientIds + incoming.clientIds, + totalUnidentified = current.totalUnidentified, + clipped = current.clipped, + totalClientIds = current.totalClientIds, + ), + ) + else -> put(key, current) + } + } + }chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (3)
125-143: Fix undefined 'error' reference and reset loading on failure.Line 125 and 126 reference
errorwhich is not defined in the current scope. The variable iserrorState, so you should useerrorState.value. Additionally, line 137 should resetloadingState.valuetofalsebefore setting the error, ensuring the loading indicator is properly cleared on failure.Apply this diff:
- LaunchedEffect(this, error, shouldRequestMessages) { - if (!shouldRequestMessages || error != null) { + LaunchedEffect(this, errorState.value, shouldRequestMessages) { + if (!shouldRequestMessages || errorState.value != null) { loadingState.value = false return@LaunchedEffect } loadingState.value = true val receivedPaginatedResult = try { lastReceivedPaginatedResult?.next() ?: subscription!!.historyBeforeSubscribe(limit = fetchSize) } catch (exception: ChatException) { + loadingState.value = false errorState.value = exception.errorInfo return@LaunchedEffect } lastReceivedPaginatedResult = receivedPaginatedResult loaded += receivedPaginatedResult.items loadingState.value = false }
85-94: Fix undefined 'clientId' reference.Line 88 references
clientIdwhich is not defined in the current scope. Based on the room's API, you should use the room'sclientIdproperty.Apply this diff to fix the reference:
LaunchedEffect(this) { @OptIn(InternalChatApi::class) messages.reactions.asFlow().map { event -> - if (event.hasClippedWithoutMyClientId(clientId)) { + if (event.hasClippedWithoutMyClientId([email protected])) { event.mergeWith(messages.reactions.clientReactions(event.messageSerial)) } else { event } }.collect { loaded.replaceFirstWith(it) } }
103-114: Guard against empty visibleItemsInfo.Line 110 calls
visibleItemsInfo.last()which will throwNoSuchElementExceptionif the list is empty. The current guard only checkstotalItemsCount == 0, butvisibleItemsInfocan be empty even when there are items in the list.Apply this diff to add the guard:
val scrollIsAtTheTop by remember(this) { derivedStateOf { val layoutInfo = listState.layoutInfo val visibleItemsInfo = layoutInfo.visibleItemsInfo - if (layoutInfo.totalItemsCount == 0) { + if (layoutInfo.totalItemsCount == 0 || visibleItemsInfo.isEmpty()) { false } else { val lastVisibleItem = visibleItemsInfo.last() lastVisibleItem.index >= layoutInfo.totalItemsCount - scrollThreshold - 1 } } }
🧹 Nitpick comments (3)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
15-25: Simplify with produceState and drop redundant assignmentCurrent implementation is fine, but
produceStatereduces boilerplate and avoids double-initialization.@Composable -public fun Room.collectAsStatus(): State<RoomStatus> { - val statusState = remember(this) { mutableStateOf(status) } - - LaunchedEffect(this) { - statusState.value = status - statusAsFlow().collect { statusState.value = it.current } - } - - return statusState -} +public fun Room.collectAsStatus(): State<RoomStatus> = + produceState(initialValue = status, key1 = this) { + statusAsFlow().collect { value = it.current } + }Add import:
import androidx.compose.runtime.produceStatechat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Typing.kt (1)
15-21: Initialize state with current typing snapshot for immediate UISeed the State from typing.current() to avoid a transient empty set before the first event.
- val currentlyTyping = remember(this) { mutableStateOf(emptySet<String>()) } + val currentlyTyping = remember(this) { mutableStateOf(typing.current()) }chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt (1)
13-14: Clean up imports if you adopt the above changecancelAndJoin and launch become unused; remove them to keep the file tidy.
-import kotlinx.coroutines.cancelAndJoin -import kotlinx.coroutines.launch
📜 Review details
Configuration used: CodeRabbit 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 (22)
chat-extensions-compose/api/chat-extensions-compose.api(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(3 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/MessagesTest.kt(1 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt(3 hunks)chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt(2 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)chat/api/android/chat.api(4 hunks)chat/api/jvm/chat.api(4 hunks)chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt(4 hunks)chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt(2 hunks)chat/src/commonMain/kotlin/com/ably/chat/Room.kt(3 hunks)chat/src/commonMain/kotlin/com/ably/chat/annotations/Annotations.kt(1 hunks)chat/src/commonTest/kotlin/com/ably/chat/MessagesReactionsKtTest.kt(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(2 hunks)example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt
- chat/src/commonTest/kotlin/com/ably/chat/MessagesReactionsKtTest.kt
- chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/ConnectionTest.kt
- example/src/main/java/com/ably/chat/example/MainActivity.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/TypingTest.kt
- chat/src/commonMain/kotlin/com/ably/chat/Room.kt
- chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/RoomTest.kt
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-04-28T11:41:59.070Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#124
File: chat-android/src/main/java/com/ably/chat/Utils.kt:0-0
Timestamp: 2025-04-28T11:41:59.070Z
Learning: The `AwaitableSharedFlow` class in `com.ably.chat` intentionally uses `Channel.UNLIMITED` buffer capacity to ensure `tryEmit()` never fails due to buffer constraints, which is an important design requirement for the Ably Chat Kotlin SDK.
Applied to files:
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt
📚 Learning: 2024-11-28T11:12:06.843Z
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.
Applied to files:
chat-extensions-compose/api/chat-extensions-compose.apichat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.ktchat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt
📚 Learning: 2024-11-28T11:11:20.423Z
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.
Applied to files:
chat-extensions-compose/api/chat-extensions-compose.apichat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.ktchat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt
🧬 Code graph analysis (5)
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (1)
chat/src/commonMain/kotlin/com/ably/chat/FlowUtils.kt (1)
transformCallbackAsFlow(12-15)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
chat/src/commonMain/kotlin/com/ably/chat/Room.kt (1)
statusAsFlow(123-125)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Presence.kt (1)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
collectAsStatus(15-25)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Occupancy.kt (1)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
collectAsStatus(15-25)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Messages.kt (2)
chat-extensions-compose/src/main/kotlin/com/ably/chat/extensions/compose/Room.kt (1)
collectAsStatus(15-25)chat/src/commonMain/kotlin/com/ably/chat/Discontinuity.kt (1)
discontinuityAsFlow(70-72)
🔇 Additional comments (8)
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/PresenceTest.kt (1)
36-36: Compose State adoption in tests looks correctUsing .value from State<List> fits the updated API; assertions remain valid.
Also applies to: 83-83
example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt (1)
14-14: Good switch to delegated StateUsing
val members by collectAsPresenceMembers()withgetValueimport is the right Compose pattern.Also applies to: 30-30
chat-extensions-compose/src/test/kotlin/com/ably/chat/extensions/compose/OccupancyTest.kt (2)
35-43: State-based occupancy observation is correctUsing
.valueand a lightweighttoPair()makes the tests align with the newOccupancyDataAPI cleanly.Also applies to: 51-59
95-104: Nice test helpersPrivate
OccupancyEventfactory andOccupancyData.toPair()keep tests focused on semantics and reduce duplication.chat/src/commonMain/kotlin/com/ably/chat/annotations/Annotations.kt (1)
26-45: InternalChatApi annotation looks goodOpt-in level, targets, and documentation are appropriate.
chat/src/commonMain/kotlin/com/ably/chat/MessagesReactions.kt (2)
96-101: asFlow() wrapper is solidGood use of transformCallbackAsFlow; unsubscribe handled via awaitClose, and buffering is handled upstream.
251-258: mergeWith now uses current event reactions — correctUsing this.reactions.mergeWith(reactions) fixes the self-merge bug.
chat/api/jvm/chat.api (1)
631-633: No duplicate InternalChatApi declarationsOne occurrence in each API dump as expected; no deduplication needed.
ttypic
force-pushed
the
compose-update
branch
from
d21e5f7 to
7572efb
Compare
- Updated `Room.collectAsOccupancy` to use `OccupancyData` - Updated messages helper to subscribe for reaction event - Improved `collectAsPagingMessagesState` implementation, listen to message reactions updates - Made all extensions return `State<*>` instead of plain objects to avoid unnecessary recompositions
ttypic
force-pushed
the
compose-update
branch
from
7572efb to
7839600
Compare
2 participants
Room.collectAsOccupancyto useOccupancyDatacollectAsPagingMessagesStateimplementation, listen to message reactions updatesState<*>instead of plain objects to avoid unnecessary recompositionsSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests