Add GroupedQueryChannels and grouped unread counts#6437
Open
VelikovPetar wants to merge 52 commits into
Open
Conversation
…Handler.handleChatEvent`.
…to feature/grouped-channels-endpoint
# Conflicts: # stream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogicTest.kt # stream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsStateLogicTest.kt
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
SDK Size Comparison 📏
|
Contributor
|
DB Entities have been updated. Do we need to upgrade DB Version? |
- Guard GroupAwareChatEventHandlerFactory auto-install in LogicRegistry to be idempotent (preserve any factory already installed on the state). - Defensively reset channelsOffset on first-page applyGroupedResult. - Rename QueryChannelsIdentifier.Grouped.group to .groupKey for naming consistency with the rest of the grouped surface. - Add grouped-channels tests: QueryChannelsLogicGroupedTest covers applyGroupedResult and loadOfflineGroupedChannels; extend LogicRegistryTest with Grouped identifier coverage. - Doc tweaks: SyncManager.updateGroupedQueryChannels assumption, DefaultChannelGroupResolver currentGroup intent, drop stale @Suppress("LongMethod") on observeQueryChannelsInternal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # stream-chat-android-state/src/test/java/io/getstream/chat/android/state/event/handler/internal/EventHandlerSequentialTest.kt
gpunto
reviewed
May 25, 2026
Contributor
gpunto
left a comment
gpunto
left a comment
There was a problem hiding this comment.
Leaving a batch of early comments from a first pass since I'm out of office tomorrow and I don't want to block you.
| ConcurrentHashMap() | ||
| private val threads: ConcurrentHashMap<String, ThreadMutableState> = ConcurrentHashMap() | ||
|
|
||
| private val watchedChannelRecords = mutableListOf<WeakReference<WatchedChannelRecord>>() |
Contributor
There was a problem hiding this comment.
Do we need WatchedChannelRecord? Maybe we can use a weak map here with the flow as a key and the cid as value:
private val watchedChannelFlows: MutableMap<WatchedChannelStateFlow, String> = Collections.synchronizedMap(WeakHashMap())This should simplify the implementation a bit as the map handles the weak references for us and getting the cids is just watchedChannelFlows.values.toSet(). It also eliminates the unused property, which I worry could be optimized away by R8.
I also considered using the original flow directly as the key, but that would couple us to a coroutines implementation detail. So I think we should still keep the wrapper which we guarantee is a fresh instance with its own equality semantics.
Contributor
Author
There was a problem hiding this comment.
I like the suggestion - let me test it to make it works properly, and I will simplify it!
Comment on lines
+448
to
+449
| * Assumes all active grouped queries share the same request-level `limit`, `watch`, and | ||
| * `presence` flags — the first captured config wins. |
Contributor
There was a problem hiding this comment.
Do we know that there's always at most one set of limit + watch + presence? What if an integrator calls chatClient.queryGroupedChannels twice for different groups with different params?
Contributor
Author
There was a problem hiding this comment.
Well the idea is that GroupedQueryChannels would replace multiple QueryChannels invocations - at the moment it is not intended to neither have more than one call of GroupedQueryChannels, nor GroupedQueryChannels + QueryChannels.
Do you think we should re-build the request by keeping track of each invocation (and argument)?
| public data class QueryChannelsSpec( | ||
| val filter: FilterObject, | ||
| val querySort: QuerySorter<Channel>, | ||
| var cids: Set<String> = emptySet(), |
Contributor
There was a problem hiding this comment.
Do we have to move this to the constructor? Given that it is a data class, all constructor properties are going to be used for equality/hash code and having a var makes them unstable in time, e.g. if QueryChannelsSpec is used as key in a map, the hash code can change while the item is in the map so trying to reinsert the same spec would duplicate it in the map.
Ideally we would make it a val, but that's a breaking change 😅 Should we move it back in the body?
Contributor
Author
There was a problem hiding this comment.
Yes OK, I can move it back to the body (the PredefinedFilters PR would also be affected by this)
| result.value.groups.forEach { (key, group) -> | ||
| // A request without a `next` cursor for this key (or no per-group query at all) is | ||
| // a first-page request → replace channels. With a `next` cursor → paginated → append. | ||
| val isFirstPage = groups?.get(key)?.next == null |
Contributor
There was a problem hiding this comment.
Should we check also prev? We do that at line 69 above.
Contributor
Author
There was a problem hiding this comment.
Ah yes, I missed that (although we prev is currently never used)
| * Replaces channels on the first page, appends on subsequent pages. | ||
| * Updates the next-page cursor and persists fresh data to the local database. | ||
| */ | ||
| internal suspend fun applyGroupedResult(group: GroupedChannelsGroup, isFirstPage: Boolean) { |
Contributor
There was a problem hiding this comment.
I wonder if we need some kind of synchronization here. We are mutating queryChannelsStateLogic via several functions so the whole function is not atomic. Maybe having a mutex would help. Do you think it's a realistic worry?
Contributor
Author
There was a problem hiding this comment.
Hmm it is probably a good idea to add synchronisation - at least to prevent sync bugs if the related methods are misused. Let me see what we can do.
Comment on lines
+414
to
+415
| val hasGroupedQueries = allLogics.any { it.groupKey() != null } | ||
| val hasStandardQueries = allLogics.any { it.groupKey() == null } |
Contributor
There was a problem hiding this comment.
The previous code was always calling updateActiveChannels, now we do only if we have elements in the standard queries list. Could there be cases where the list is empty but we still have to call it. I see it calls getActiveChannelStates which reads from StateRegistry.channels. Could that be not empty?
Contributor
Author
There was a problem hiding this comment.
For the GroupedQueryChannels - we explicitly want to re-watch ONLY the channels which are currently active/opened (that's why I introduced the weak-reference tracker). This is a bit different from the current QueryChannels logic, where we would re-watch ALL channels that have been instantiated.
So I made this like this intentionally, but to be honest I am not 100% it is correct approach.
Although there is another point about getActiveChannelStates - it might not be fully correct, because it results in a QueryChannels call with arbitrary CIDs, which might not be expected and optimised on the BE. (I think we should eventually revisit the whole SyncManager, we might be doing some not optimised calls here)
| public data class GroupedChannelsGroup( | ||
| public val groupKey: String, | ||
| public val channels: List<Channel>, | ||
| public val unreadChannels: Int = 0, |
Contributor
There was a problem hiding this comment.
Should this maybe be unreadChannelsCount? The current name suggests we return channels
Contributor
Author
There was a problem hiding this comment.
I followed the API naming, but let me actually align this naming with iOS, because this is part of the public API which should be aligned.
If it is OK with you, I would keep it as unreadChannels so that we are aligned with iOS: https://github.com/GetStream/stream-chat-swift/pull/4076/changes#diff-2d936aad31714a18b3ad01e719db554b74b31681975075c230c5652d23fd6ffeR23
WalkthroughAdds grouped channel query APIs, models, and Retrofit endpoint; propagates grouped unread counts in events; extends state engine with grouped identifiers, logic, persistence, and recovery; integrates grouped mode in Compose ChannelListViewModel/Factory; updates global state; and adds comprehensive unit tests and fixtures. ChangesClient API, identifiers, and core models
State engine, persistence, and recovery
Events and DTOs: grouped unread propagation
Compose ChannelListViewModel and factory (grouped mode)
Client grouped query tests and adapters
State logic tests for grouped features
Sequence Diagram(s)sequenceDiagram
participant VM as ChannelListViewModel (Grouped)
participant Client as ChatClient
participant API as ChatApi
participant HTTP as ChannelApi
VM->>Client: queryGroupedChannelsInternal(limit, groups, watch, presence)
Client->>API: queryGroupedChannels(...)
API->>HTTP: POST /channels/grouped (body: groups, cursors)
HTTP-->>API: QueryGroupedChannelsResponse
API-->>Client: GroupedChannels
Client-->>VM: Result<GroupedChannels>
VM->>VM: applyGroupedResult, update cursors/state
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
stream-chat-android-client/api/stream-chat-android-client.api (1)
1788-1812:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve constructor/copy ABI for existing event types.
Adding
groupedUnreadChannelsdirectly to these public event data classes changes their constructor,copy, andcomponentNsignatures. That is a source/binary breaking API change for consumers who instantiate or clone these events. Please expose grouped unread counts additively instead of changing the shape of existing public event types in place.Also applies to: 1854-1875, 1901-1922, 2010-2035, 2050-2078, 2094-2116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/api/stream-chat-android-client.api` around lines 1788 - 1812, You changed the public data-class shape for events like NewMessageEvent (its constructor, copy and componentN signatures), which is a binary-breaking change; revert adding groupedUnreadChannels as a constructor parameter and instead expose it additively — implement getGroupedUnreadChannels as a derived accessor that reads the value from existing extraData/metadata (or provide a new wrapper/subclass like NewMessageEventWithGroupedUnreadChannels or a helper extension function) so the original NewMessageEvent constructor/copy/componentN remain untouched; update usages to call the new accessor/wrapper rather than changing the event primary constructor or copy methods.stream-chat-android-client/src/test/java/io/getstream/chat/android/client/api2/mapping/EventMappingTestArguments.kt (1)
538-552:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a null-count NotificationMarkUnread fixture to validate fallback mapping.
Line 1216 and Line 1217 added
?: 0, but the only fixture (Line 549 and Line 550) still provides non-null counts, so this new behavior is currently untested inarguments().✅ Suggested test fixture addition
+ private val notificationMarkUnreadDtoWithNullCounts = NotificationMarkUnreadEventDto( + type = EventType.NOTIFICATION_MARK_UNREAD, + created_at = EXACT_DATE, + user = USER, + cid = CID, + channel_type = CHANNEL_TYPE, + channel_id = CHANNEL_ID, + first_unread_message_id = FIRST_UNREAD_MESSAGE_ID, + last_read_message_id = LAST_READ_MESSAGE_ID, + last_read_at = EXACT_DATE, + unread_messages = UNREAD_MESSAGES, + total_unread_count = null, + unread_channels = null, + grouped_unread_channels = GROUPED_UNREAD_CHANNELS, + ) + + private val notificationMarkUnreadWithNullCounts = NotificationMarkUnreadEvent( + type = notificationMarkUnreadDtoWithNullCounts.type, + createdAt = notificationMarkUnreadDtoWithNullCounts.created_at.date, + rawCreatedAt = notificationMarkUnreadDtoWithNullCounts.created_at.rawDate, + user = with(domainMapping) { notificationMarkUnreadDtoWithNullCounts.user.toDomain() }, + cid = notificationMarkUnreadDtoWithNullCounts.cid, + channelType = notificationMarkUnreadDtoWithNullCounts.channel_type, + channelId = notificationMarkUnreadDtoWithNullCounts.channel_id, + firstUnreadMessageId = notificationMarkUnreadDtoWithNullCounts.first_unread_message_id, + lastReadMessageId = notificationMarkUnreadDtoWithNullCounts.last_read_message_id, + lastReadMessageAt = notificationMarkUnreadDtoWithNullCounts.last_read_at.date, + unreadMessages = notificationMarkUnreadDtoWithNullCounts.unread_messages, + totalUnreadCount = 0, + unreadChannels = 0, + groupedUnreadChannels = notificationMarkUnreadDtoWithNullCounts.grouped_unread_channels, + ) ... + Arguments.of(notificationMarkUnreadDtoWithNullCounts, notificationMarkUnreadWithNullCounts),Also applies to: 1204-1219, 1559-1626
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/api2/mapping/EventMappingTestArguments.kt` around lines 538 - 552, The tests added a fallback mapping using the Elvis operator (?: 0) for unread counts but the existing NotificationMarkUnreadEventDto fixture (notificationMarkUnreadDto) still supplies non-null counts, so arguments() doesn't exercise the null-to-zero fallback; add a new fixture instance of NotificationMarkUnreadEventDto with first_unread_message_id, last_read_message_id, unread_messages, total_unread_count, and unread_channels set to null (or omit them) and include it in the arguments() list used by EventMappingTestArguments to validate the mapping fallback for notificationMarkUnreadDto; repeat the same pattern for similar fixtures referenced around the other affected ranges.
🧹 Nitpick comments (8)
stream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientGroupedChannelsApiTests.kt (1)
131-155: ⚡ Quick winAssert failure path dispatches the result hook too.
This test currently verifies only
onQueryGroupedChannelsRequest. Add an assertion thatonQueryGroupedChannelsResultis also invoked on failure so regressions in failure-result propagation are caught.Proposed test extension
fun `queryGroupedChannelsInternal dispatches request hook even when the call fails`() = runTest { @@ verify(plugin).onQueryGroupedChannelsRequest( limit = eq(30), groups = eq(groupsParam), watch = eq(true), presence = eq(false), ) + verify(plugin).onQueryGroupedChannelsResult( + result = any(), + limit = eq(30), + groups = eq(groupsParam), + watch = eq(true), + presence = eq(false), + ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientGroupedChannelsApiTests.kt` around lines 131 - 155, The test currently only verifies the request hook; extend it to also verify that plugin.onQueryGroupedChannelsResult(...) is invoked when the call fails by adding a verify(plugin) call after the await that checks onQueryGroupedChannelsResult was called with the same limit, groups, watch, and presence arguments and a failure/result indicating the RetroError (or use an argument matcher like any() for the error payload if exact type is hard to match). Update the test method `queryGroupedChannelsInternal dispatches request hook even when the call fails` to include this verification so failure result propagation is asserted.stream-chat-android-client/src/test/java/io/getstream/chat/android/client/api2/MoshiChatApiTest.kt (1)
1914-1923: ⚡ Quick winAdd a non-null
groupscase to validate nested request mapping.This test only checks
groups = null. Please add a case withgroupscontaining per-grouplimit/next/prevand assert the exactQueryGroupedChannelsRequestmapping sent toChannelApi.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/api2/MoshiChatApiTest.kt` around lines 1914 - 1923, Add a new test similar to the existing null-groups case that calls sut.queryGroupedChannels with a non-null groups map (e.g., mapOf("team" to ChannelPaginationParams(limit=5, next="n", prev="p"), "project" to ChannelPaginationParams(limit=3, next=null, prev="x")) and await the result; build an expected QueryGroupedChannelsRequest with the same groups structure and other params (limit/watch/presence) and assert the result type equals expected, then verify(api, times(1)).queryGroupedChannels(connectionId, expectedPayload) to ensure the nested mapping is sent to ChannelApi.stream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/QueryGroupedChannelsResponseAdapterTest.kt (1)
33-87: ⚡ Quick winInclude and assert group
next/prevcursors in the JSON fixture.Pagination cursors are part of grouped query behavior; asserting them here would harden parser coverage against regressions.
Also applies to: 145-181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/QueryGroupedChannelsResponseAdapterTest.kt` around lines 33 - 87, Add pagination cursors to the JSON fixture under the grouped entry and assert them in the test: extend the "groups" -> "all-open" object to include "next" and "prev" cursor fields with sample values, then update the assertions in QueryGroupedChannelsResponseAdapterTest (where the variable json is used) to verify parsedGroup.next and parsedGroup.prev (or the actual properties returned by the parser) match those sample values; ensure both the primary test and the additional case referenced (lines ~145-181) include the same cursors and assertions to cover regressions.stream-chat-android-core/src/main/java/io/getstream/chat/android/models/GroupedChannels.kt (1)
51-55: ⚡ Quick winEnforce
next/prevmutual exclusivity in constructorThe KDoc documents these as mutually exclusive, but the model currently allows both to be set. Add an
initguard to fail fast on invalid requests.Proposed change
public data class GroupedChannelsGroupQuery( public val limit: Int? = null, public val next: String? = null, public val prev: String? = null, -) +) { + init { + require(next == null || prev == null) { + "GroupedChannelsGroupQuery: `next` and `prev` are mutually exclusive." + } + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-core/src/main/java/io/getstream/chat/android/models/GroupedChannels.kt` around lines 51 - 55, The GroupedChannelsGroupQuery data class allows both next and prev simultaneously despite KDoc saying they are mutually exclusive; add an init block in GroupedChannelsGroupQuery that checks if next != null && prev != null and throw an IllegalArgumentException (or similar) with a clear message to fail fast; update the init to reference the class name GroupedChannelsGroupQuery and validate the constructor parameters (limit can remain unchanged).stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt (1)
3150-3160: ⚡ Quick winExpand KDoc with thread and state expectations for the new public API.
The new grouped-query KDocs describe params/behavior, but they don’t state threading and state expectations (for example, calling thread guarantees and required client/socket state when
watch/presenceare enabled).As per coding guidelines
stream-chat-android-client/src/main/**/*.kt: “Document public APIs with KDoc, including thread expectations and state notes”.Also applies to: 3174-3189
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt` around lines 3150 - 3160, Update the KDoc for the public grouped-query API (ChatClient.queryGroupedChannels and its overloads) to explicitly state thread and state expectations: say whether the method is safe to call from background threads or must be called on the main thread, which thread/coroutine context results/callbacks are delivered on, and that when watch==true or presence==true the ChatClient must be connected/authenticated and the socket open (or document that the call will attempt to open the socket and what guarantees that gives); add the same notes to the second KDoc block referenced (around the other overload at lines ~3174-3189) so both public docs mention calling-thread, callback-thread, and required client/socket state for watch/presence.stream-chat-android-client/src/main/java/io/getstream/chat/android/client/persistance/repository/QueryChannelsRepository.kt (1)
36-39: ⚡ Quick winExpand KDoc for
selectBy(groupKey)with threading/state expectations.Please add a short note about expected coroutine/threading usage and state semantics (e.g., nullable when absent) to keep public repository APIs consistently documented.
As per coding guidelines "Document public APIs with KDoc, including thread expectations and state notes".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/persistance/repository/QueryChannelsRepository.kt` around lines 36 - 39, Add KDoc to the public suspend function selectBy(groupKey: String) in QueryChannelsRepository describing coroutine/threading and state semantics: note that it is a suspending call executed on the caller's coroutine context (not confined to a specific thread), should be safe to call from any coroutine (including Main), and that the returned QueryChannelsSpec? is nullable to indicate the spec is absent in storage (not a live/observable state update). Mention any thread-safety expectations (e.g., repository handles internal synchronization) if applicable and keep the note concise.stream-chat-android-client/src/main/java/io/getstream/chat/android/client/plugin/listeners/QueryGroupedChannelsListener.kt (1)
23-62: ⚡ Quick winAdd explicit callback thread/coroutine-context expectations in KDoc.
The new listener docs explain state behavior, but they don’t state where callbacks execute (thread/coroutine context). Please document that explicitly for both callbacks so plugin implementations can safely handle side effects.
As per coding guidelines "Document public APIs with KDoc, including thread expectations and state notes".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/plugin/listeners/QueryGroupedChannelsListener.kt` around lines 23 - 62, Update the KDoc for both onQueryGroupedChannelsRequest and onQueryGroupedChannelsResult to explicitly state the coroutine/thread context where implementations will be invoked (e.g., "Invoked on the ChatClient's internal coroutine context/dispatcher; not guaranteed to be Main/UI — switch to Dispatchers.Main for UI work" or the exact client dispatcher used). Mention any guarantees (serial/in-order invocation) if applicable and advise callers to switch context for side effects. Ensure these sentences are added to each function’s KDoc so plugin authors know where to run UI or blocking operations.stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModel.kt (1)
663-667: 💤 Low valueConsider guarding against null user in grouped search filter.
If
user.valueis null whenoptimizedChannelSearchFilteris called,orEmpty()produces an empty string, resulting in a filter that matches no channels. While unlikely during normal operation, this could occur during logout races. The search would silently return no results.You may want to add an early return in
buildQueryChannelsRequestfor Grouped mode similar to the Standard mode's null-filter check, or document that this is expected behavior.🔧 Optional defensive guard
is QueryMode.Grouped -> QueryChannelsRequest( - filter = optimizedChannelSearchFilter(searchQuery), + filter = user.value?.id?.let { optimizedChannelSearchFilter(searchQuery, it) } ?: return null, limit = channelLimit, messageLimit = messageLimit, memberLimit = memberLimit, ) } - private fun optimizedChannelSearchFilter(text: String): FilterObject = + private fun optimizedChannelSearchFilter(text: String, userId: String): FilterObject = Filters.and( Filters.autocomplete("name", text), - Filters.`in`("members", user.value?.id.orEmpty()), + Filters.`in`("members", userId), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModel.kt` around lines 663 - 667, The grouped-search path can call optimizedChannelSearchFilter when user.value is null, producing an empty-string member filter that matches nothing; update buildQueryChannelsRequest (the grouped-mode branch) to guard against a null user.value.id the same way the Standard mode does—if user.value?.id is null, return early (e.g., empty query/response) or skip applying the member filter instead of calling optimizedChannelSearchFilter; alternatively adjust optimizedChannelSearchFilter to return a safe FilterObject (or throw) when user.value?.id is absent. Ensure you reference optimizedChannelSearchFilter and buildQueryChannelsRequest when making the change so the grouped search no longer silently returns no results during logout races.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@stream-chat-android-client/api/stream-chat-android-client.api`:
- Line 3092: An abstract method selectBy(String, Continuation) was added to
QueryChannelsRepository which breaks binary compatibility for custom
implementations supplied via RepositoryFactory; remove the new abstract
declaration from QueryChannelsRepository and instead provide either a default
implementation on QueryChannelsRepository (e.g., a default selectBy(...) body)
or declare a new extension/sub-interface that contains selectBy so existing
implementers aren’t forced to change; update usages to call the
default/extension method while leaving RepositoryFactory and existing
implementers untouched.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt`:
- Around line 3162-3172: The public API queryGroupedChannels currently accepts
an empty groups list (sending groups = emptyMap()), violating the KDoc
precondition; add a guard at the start of queryGroupedChannels that validates
groups.isNotEmpty() and throws an IllegalArgumentException (or uses require())
with a clear message like "groups must contain at least one group" before
calling queryGroupedChannelsInternal, so callers cannot send an empty map;
reference the public function queryGroupedChannels and the internal helper
queryGroupedChannelsInternal when making the change.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/api2/MoshiChatApiTestArguments.kt`:
- Line 459: The success fixture sets unread_channels = randomInt(), which can
produce negative counts; update the generator to ensure a non-negative value
(e.g., use a non-negative helper or bounded call instead of randomInt()). Locate
the unread_channels assignment in MoshiChatApiTestArguments and replace
randomInt() with a non-negative generator such as randomInt(max = N) or a
dedicated randomNonNegativeInt()/abs(randomInt()) so unread_channels is always
>= 0.
In `@stream-chat-android-state/api/stream-chat-android-state.api`:
- Line 212: You added new abstract getters (getGroupedUnreadChannels) to public
interfaces (GlobalState and QueryChannelsState) which breaks binary
compatibility; instead, add a default implementation or introduce a new derived
sub-interface: provide a default-backed method implementation in the interface
(e.g., default getGroupedUnreadChannels() returning an empty StateFlow or a
pre-existing fallback) or create a new interface (e.g.,
GlobalStateWithGroupedUnread / QueryChannelsStateWithGroupedUnread) that extends
the original and declares getGroupedUnreadChannels, update usages to the new
type, and then regenerate the public API dump with ./gradlew apiDump so
stream-chat-android-state.api is updated rather than hand-editing it.
In
`@stream-chat-android-state/src/main/java/io/getstream/chat/android/state/event/handler/internal/EventHandlerSequential.kt`:
- Around line 438-449: The grouped-unread update is being applied twice when an
event carries authoritative groupedUnreadChannels: currently
HasGroupedUnreadChannels is processed first and then
ChannelUpdatedEvent/ChannelUpdatedByUserEvent apply delta changes again; reorder
the logic so the delta handlers (ChannelUpdatedEvent and
ChannelUpdatedByUserEvent) run before the authoritative handler
(HasGroupedUnreadChannels) to ensure authoritative groupedUnreadChannels
provided by the event (variable groupedUnreadChannels and updater
groupedUnreadChannelsUpdater) overwrite deltas rather than being mutated
afterwards.
In
`@stream-chat-android-state/src/main/java/io/getstream/chat/android/state/extensions/ChatClient.kt`:
- Around line 208-213: The coroutine currently holds a strong reference to
watchedFlow while waiting for initialization (coroutineScope.launch {
clientState.initializationState.first { ... };
state.trackWatchedChannel(watchedFlow) }), which can keep the flow alive
indefinitely; change the API to accept a provider/lambda (e.g.,
watchedFlowProvider: () -> Flow<...>) and update this code to wait for
clientState.initializationState.first { it == InitializationState.COMPLETE }
first and only then invoke state.trackWatchedChannel(watchedFlowProvider()), so
the actual Flow instance is created/retrieved after init completes and not
strongly captured during the wait.
In
`@stream-chat-android-state/src/main/java/io/getstream/chat/android/state/sync/internal/SyncManager.kt`:
- Around line 469-476: The current mapping over activeGroupedLogics skips a
group when logic.groupedQueryConfig() returns null, which causes recovery to
omit groups whose config wasn't captured; update the logic building groupsParam
so that for each activeGroupedLogics entry with a non-null groupKey() you always
include an entry using groupedQueryConfig() when present otherwise fallback to a
default GroupedChannelsGroupQuery() (e.g., new/default instance) instead of
returning null; adjust the mapNotNull block around activeGroupedLogics to only
filter out entries with null groupKey() but never filter when cfg is null, and
construct GroupedChannelsGroupQuery(limit = cfg.pageSize) when cfg != null or
GroupedChannelsGroupQuery() when cfg == null.
In
`@stream-chat-android-state/src/test/java/io/getstream/chat/android/state/internal/SyncManagerTest.kt`:
- Around line 87-88: The `@Suppress`("LargeClass") usage was added without
rationale; either remove that suppression or add a one-line justification
comment directly above it explaining why the test class (SyncManagerTest) is
intentionally large (e.g., "test class aggregates many scenarios for SyncManager
integration, keeping related tests together") so suppression is auditable; keep
the `@OptIn`(InternalStreamChatApi::class) as-is and ensure the justification
comment references the suppression symbol so reviewers can see why
`@Suppress`("LargeClass") is necessary.
---
Outside diff comments:
In `@stream-chat-android-client/api/stream-chat-android-client.api`:
- Around line 1788-1812: You changed the public data-class shape for events like
NewMessageEvent (its constructor, copy and componentN signatures), which is a
binary-breaking change; revert adding groupedUnreadChannels as a constructor
parameter and instead expose it additively — implement getGroupedUnreadChannels
as a derived accessor that reads the value from existing extraData/metadata (or
provide a new wrapper/subclass like NewMessageEventWithGroupedUnreadChannels or
a helper extension function) so the original NewMessageEvent
constructor/copy/componentN remain untouched; update usages to call the new
accessor/wrapper rather than changing the event primary constructor or copy
methods.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/api2/mapping/EventMappingTestArguments.kt`:
- Around line 538-552: The tests added a fallback mapping using the Elvis
operator (?: 0) for unread counts but the existing
NotificationMarkUnreadEventDto fixture (notificationMarkUnreadDto) still
supplies non-null counts, so arguments() doesn't exercise the null-to-zero
fallback; add a new fixture instance of NotificationMarkUnreadEventDto with
first_unread_message_id, last_read_message_id, unread_messages,
total_unread_count, and unread_channels set to null (or omit them) and include
it in the arguments() list used by EventMappingTestArguments to validate the
mapping fallback for notificationMarkUnreadDto; repeat the same pattern for
similar fixtures referenced around the other affected ranges.
---
Nitpick comments:
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt`:
- Around line 3150-3160: Update the KDoc for the public grouped-query API
(ChatClient.queryGroupedChannels and its overloads) to explicitly state thread
and state expectations: say whether the method is safe to call from background
threads or must be called on the main thread, which thread/coroutine context
results/callbacks are delivered on, and that when watch==true or presence==true
the ChatClient must be connected/authenticated and the socket open (or document
that the call will attempt to open the socket and what guarantees that gives);
add the same notes to the second KDoc block referenced (around the other
overload at lines ~3174-3189) so both public docs mention calling-thread,
callback-thread, and required client/socket state for watch/presence.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/persistance/repository/QueryChannelsRepository.kt`:
- Around line 36-39: Add KDoc to the public suspend function selectBy(groupKey:
String) in QueryChannelsRepository describing coroutine/threading and state
semantics: note that it is a suspending call executed on the caller's coroutine
context (not confined to a specific thread), should be safe to call from any
coroutine (including Main), and that the returned QueryChannelsSpec? is nullable
to indicate the spec is absent in storage (not a live/observable state update).
Mention any thread-safety expectations (e.g., repository handles internal
synchronization) if applicable and keep the note concise.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/plugin/listeners/QueryGroupedChannelsListener.kt`:
- Around line 23-62: Update the KDoc for both onQueryGroupedChannelsRequest and
onQueryGroupedChannelsResult to explicitly state the coroutine/thread context
where implementations will be invoked (e.g., "Invoked on the ChatClient's
internal coroutine context/dispatcher; not guaranteed to be Main/UI — switch to
Dispatchers.Main for UI work" or the exact client dispatcher used). Mention any
guarantees (serial/in-order invocation) if applicable and advise callers to
switch context for side effects. Ensure these sentences are added to each
function’s KDoc so plugin authors know where to run UI or blocking operations.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/api2/MoshiChatApiTest.kt`:
- Around line 1914-1923: Add a new test similar to the existing null-groups case
that calls sut.queryGroupedChannels with a non-null groups map (e.g.,
mapOf("team" to ChannelPaginationParams(limit=5, next="n", prev="p"), "project"
to ChannelPaginationParams(limit=3, next=null, prev="x")) and await the result;
build an expected QueryGroupedChannelsRequest with the same groups structure and
other params (limit/watch/presence) and assert the result type equals expected,
then verify(api, times(1)).queryGroupedChannels(connectionId, expectedPayload)
to ensure the nested mapping is sent to ChannelApi.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientGroupedChannelsApiTests.kt`:
- Around line 131-155: The test currently only verifies the request hook; extend
it to also verify that plugin.onQueryGroupedChannelsResult(...) is invoked when
the call fails by adding a verify(plugin) call after the await that checks
onQueryGroupedChannelsResult was called with the same limit, groups, watch, and
presence arguments and a failure/result indicating the RetroError (or use an
argument matcher like any() for the error payload if exact type is hard to
match). Update the test method `queryGroupedChannelsInternal dispatches request
hook even when the call fails` to include this verification so failure result
propagation is asserted.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/QueryGroupedChannelsResponseAdapterTest.kt`:
- Around line 33-87: Add pagination cursors to the JSON fixture under the
grouped entry and assert them in the test: extend the "groups" -> "all-open"
object to include "next" and "prev" cursor fields with sample values, then
update the assertions in QueryGroupedChannelsResponseAdapterTest (where the
variable json is used) to verify parsedGroup.next and parsedGroup.prev (or the
actual properties returned by the parser) match those sample values; ensure both
the primary test and the additional case referenced (lines ~145-181) include the
same cursors and assertions to cover regressions.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModel.kt`:
- Around line 663-667: The grouped-search path can call
optimizedChannelSearchFilter when user.value is null, producing an empty-string
member filter that matches nothing; update buildQueryChannelsRequest (the
grouped-mode branch) to guard against a null user.value.id the same way the
Standard mode does—if user.value?.id is null, return early (e.g., empty
query/response) or skip applying the member filter instead of calling
optimizedChannelSearchFilter; alternatively adjust optimizedChannelSearchFilter
to return a safe FilterObject (or throw) when user.value?.id is absent. Ensure
you reference optimizedChannelSearchFilter and buildQueryChannelsRequest when
making the change so the grouped search no longer silently returns no results
during logout races.
In
`@stream-chat-android-core/src/main/java/io/getstream/chat/android/models/GroupedChannels.kt`:
- Around line 51-55: The GroupedChannelsGroupQuery data class allows both next
and prev simultaneously despite KDoc saying they are mutually exclusive; add an
init block in GroupedChannelsGroupQuery that checks if next != null && prev !=
null and throw an IllegalArgumentException (or similar) with a clear message to
fail fast; update the init to reference the class name GroupedChannelsGroupQuery
and validate the constructor parameters (limit can remain unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 922bbeab-6322-4345-ac89-07eeac429ee4
📒 Files selected for processing (73)
stream-chat-android-client-test/src/main/java/io/getstream/chat/android/client/test/Mother.ktstream-chat-android-client/api/stream-chat-android-client.apistream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/api/ChatApi.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/api2/MoshiChatApi.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/api2/endpoint/ChannelApi.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/api2/mapping/EventMapping.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/api2/model/dto/EventDtos.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/api2/model/requests/QueryGroupedChannelsRequest.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/api2/model/response/QueryGroupedChannelsResponse.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/events/ChatEvent.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/QueryChannelsIdentifier.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/persistance/repository/QueryChannelsRepository.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/persistance/repository/noop/NoOpQueryChannelsRepository.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/plugin/Plugin.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/plugin/listeners/QueryGroupedChannelsListener.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/query/QueryChannelsSpec.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientGroupedChannelsApiTests.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/EventChatJsonProvider.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/api2/MoshiChatApiTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/api2/MoshiChatApiTestArguments.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/api2/mapping/EventMappingTestArguments.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser/EventArguments.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/parser2/QueryGroupedChannelsResponseAdapterTest.ktstream-chat-android-compose/api/stream-chat-android-compose.apistream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModel.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channels/ChannelViewModelFactory.ktstream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModelTest.ktstream-chat-android-core/api/stream-chat-android-core.apistream-chat-android-core/src/main/java/io/getstream/chat/android/models/GroupedChannels.ktstream-chat-android-offline/src/main/java/io/getstream/chat/android/offline/repository/database/internal/ChatDatabase.ktstream-chat-android-offline/src/main/java/io/getstream/chat/android/offline/repository/domain/queryChannels/internal/DatabaseQueryChannelsRepository.ktstream-chat-android-offline/src/main/java/io/getstream/chat/android/offline/repository/domain/queryChannels/internal/QueryChannelsEntity.ktstream-chat-android-state/api/stream-chat-android-state.apistream-chat-android-state/src/main/java/io/getstream/chat/android/state/event/handler/grouped/internal/ChannelGroupExtensions.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/event/handler/grouped/internal/ChannelGroupResolver.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/event/handler/grouped/internal/DefaultChannelGroupResolver.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/event/handler/grouped/internal/GroupAwareChatEventHandler.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/event/handler/grouped/internal/GroupAwareChatEventHandlerFactory.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/event/handler/grouped/internal/GroupedUnreadChannelsUpdater.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/event/handler/internal/EventHandlerSequential.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/extensions/ChatClient.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/model/querychannels/pagination/internal/Mapper.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/factory/StreamStatePluginFactory.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/internal/StatePlugin.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/listener/internal/QueryChannelsListenerState.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/listener/internal/QueryGroupedChannelsListenerState.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/logic/internal/LogicRegistry.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsDatabaseLogic.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogic.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsStateLogic.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/state/StateRegistry.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/state/global/GlobalState.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/state/global/internal/MutableGlobalState.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/state/internal/ChatClientStateCalls.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/state/internal/WatchedChannelStateFlow.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/state/querychannels/GroupedQueryConfig.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/state/querychannels/QueryChannelsState.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/state/querychannels/internal/QueryChannelsMutableState.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/sync/internal/SyncManager.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/event/handler/grouped/internal/DefaultChannelGroupResolverTest.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/event/handler/grouped/internal/GroupAwareChatEventHandlerTest.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/event/handler/grouped/internal/GroupedUnreadChannelsUpdaterTest.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/event/handler/internal/EventHandlerSequentialTest.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/internal/SyncManagerTest.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/listener/internal/QueryGroupedChannelsListenerStateTest.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/internal/LogicRegistryTest.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogicGroupedTest.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogicTest.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsStateLogicTest.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/state/StateRegistryTest.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/state/internal/ChatClientStateCallsTest.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/state/querychannels/internal/QueryChannelsMutableStateTest.kt
| public abstract fun clear (Lkotlin/coroutines/Continuation;)Ljava/lang/Object; | ||
| public abstract fun insertQueryChannels (Lio/getstream/chat/android/client/query/QueryChannelsSpec;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; | ||
| public abstract fun selectBy (Lio/getstream/chat/android/models/FilterObject;Lio/getstream/chat/android/models/querysort/QuerySorter;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; | ||
| public abstract fun selectBy (Ljava/lang/String;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; |
There was a problem hiding this comment.
Avoid adding an abstract method to QueryChannelsRepository.
selectBy(String, ...) makes this public interface incompatible for any custom repository implementation supplied through RepositoryFactory. A default interface implementation or a new extension interface would preserve existing implementers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@stream-chat-android-client/api/stream-chat-android-client.api` at line 3092,
An abstract method selectBy(String, Continuation) was added to
QueryChannelsRepository which breaks binary compatibility for custom
implementations supplied via RepositoryFactory; remove the new abstract
declaration from QueryChannelsRepository and instead provide either a default
implementation on QueryChannelsRepository (e.g., a default selectBy(...) body)
or declare a new extension/sub-interface that contains selectBy so existing
implementers aren’t forced to change; update usages to call the
default/extension method while leaving RepositoryFactory and existing
implementers untouched.
Comment on lines
+3162
to
+3172
| public fun queryGroupedChannels( | ||
| groups: List<String>, | ||
| limit: Int? = null, | ||
| watch: Boolean = false, | ||
| presence: Boolean = false, | ||
| ): Call<GroupedChannels> = queryGroupedChannelsInternal( | ||
| limit = limit, | ||
| groups = groups.associateWith { GroupedChannelsGroupQuery() }, | ||
| watch = watch, | ||
| presence = presence, | ||
| ) |
There was a problem hiding this comment.
Enforce non-empty groups precondition.
Line 3163 allows an empty list even though the KDoc says at least one group is required. Right now this sends groups = emptyMap(), which can produce an avoidable invalid request path.
Suggested fix
`@CheckResult`
public fun queryGroupedChannels(
groups: List<String>,
limit: Int? = null,
watch: Boolean = false,
presence: Boolean = false,
-): Call<GroupedChannels> = queryGroupedChannelsInternal(
- limit = limit,
- groups = groups.associateWith { GroupedChannelsGroupQuery() },
- watch = watch,
- presence = presence,
-)
+): Call<GroupedChannels> {
+ if (groups.isEmpty()) {
+ return ErrorCall(
+ userScope,
+ Error.GenericError("groups must contain at least one group name."),
+ )
+ }
+ return queryGroupedChannelsInternal(
+ limit = limit,
+ groups = groups.associateWith { GroupedChannelsGroupQuery() },
+ watch = watch,
+ presence = presence,
+ )
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public fun queryGroupedChannels( | |
| groups: List<String>, | |
| limit: Int? = null, | |
| watch: Boolean = false, | |
| presence: Boolean = false, | |
| ): Call<GroupedChannels> = queryGroupedChannelsInternal( | |
| limit = limit, | |
| groups = groups.associateWith { GroupedChannelsGroupQuery() }, | |
| watch = watch, | |
| presence = presence, | |
| ) | |
| `@CheckResult` | |
| public fun queryGroupedChannels( | |
| groups: List<String>, | |
| limit: Int? = null, | |
| watch: Boolean = false, | |
| presence: Boolean = false, | |
| ): Call<GroupedChannels> { | |
| if (groups.isEmpty()) { | |
| return ErrorCall( | |
| userScope, | |
| Error.GenericError("groups must contain at least one group name."), | |
| ) | |
| } | |
| return queryGroupedChannelsInternal( | |
| limit = limit, | |
| groups = groups.associateWith { GroupedChannelsGroupQuery() }, | |
| watch = watch, | |
| presence = presence, | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt`
around lines 3162 - 3172, The public API queryGroupedChannels currently accepts
an empty groups list (sending groups = emptyMap()), violating the KDoc
precondition; add a guard at the start of queryGroupedChannels that validates
groups.isNotEmpty() and throws an IllegalArgumentException (or uses require())
with a clear message like "groups must contain at least one group" before
calling queryGroupedChannelsInternal, so callers cannot send an empty map;
reference the public function queryGroupedChannels and the internal helper
queryGroupedChannelsInternal when making the change.
| draft = randomDownstreamDraftDto(), | ||
| ), | ||
| ), | ||
| unread_channels = randomInt(), |
There was a problem hiding this comment.
Use a non-negative value for unread_channels in success fixtures.
unread_channels is a count; using randomInt() can generate invalid negative values in the “success” path fixture. Prefer a non-negative generator.
Proposed tweak
- unread_channels = randomInt(),
+ unread_channels = kotlin.math.abs(randomInt()),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| unread_channels = randomInt(), | |
| unread_channels = kotlin.math.abs(randomInt()), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/api2/MoshiChatApiTestArguments.kt`
at line 459, The success fixture sets unread_channels = randomInt(), which can
produce negative counts; update the generator to ensure a non-negative value
(e.g., use a non-negative helper or bounded call instead of randomInt()). Locate
the unread_channels assignment in MoshiChatApiTestArguments and replace
randomInt() with a non-negative generator such as randomInt(max = N) or a
dedicated randomNonNegativeInt()/abs(randomInt()) so unread_channels is always
>= 0.
| public abstract fun getChannelMutes ()Lkotlinx/coroutines/flow/StateFlow; | ||
| public abstract fun getChannelUnreadCount ()Lkotlinx/coroutines/flow/StateFlow; | ||
| public abstract fun getCurrentUserActiveLiveLocations ()Lkotlinx/coroutines/flow/StateFlow; | ||
| public abstract fun getGroupedUnreadChannels ()Lkotlinx/coroutines/flow/StateFlow; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is adding a new abstract method to an existing public Kotlin interface on JVM binary compatible for already compiled implementations?
💡 Result:
No, adding a new abstract method to an existing public Kotlin interface is not binary compatible for already compiled implementations [1][2]. While the JVM specification allows for adding abstract methods to interfaces without causing immediate link-time failures [3][1], this change is binary incompatible at runtime for any existing class that implements that interface [2]. When an existing implementation (compiled against the old version of the interface) is loaded by the JVM and used, it will lack an implementation for the newly added abstract method [1][2]. If the code attempts to call this method, or if the class is instantiated and used in a way that requires the interface to be fully satisfied, the JVM will throw an AbstractMethodError [1][2]. To maintain binary compatibility, library authors should avoid adding abstract methods to interfaces that are intended to be implemented by consumers (e.g., listeners, callbacks, or extensible service interfaces) [2]. If a new method is required, consider the following alternatives: 1. Add a default method instead of an abstract method [3][1]. While this is generally binary compatible, it may still cause issues if multiple interfaces provide conflicting default implementations, potentially leading to IncompatibleClassChangeError [3][1]. 2. Create a new sub-interface that extends the original interface and includes the new abstract method [4]. Existing implementations can continue to use the original interface, while new or updated code can use the sub-interface. 3. Use the Kotlin binary compatibility validator plugin to monitor and prevent accidental binary-incompatible changes in your public API [5][6].
Citations:
- 1: https://docs.oracle.com/javase/specs/jls/se24/html/jls-13.html
- 2: https://wiki.eclipse.org/Evolving_Java-based_APIs_2
- 3: https://docs.oracle.com/javase/specs/jls/se11/html/jls-13.html
- 4: https://stackoverflow.com/questions/43493452/can-i-recompile-a-public-api-with-a-sub-interface-and-keep-binary-compatibility
- 5: https://kotlinlang.org/docs/gradle-binary-compatibility-validation.html
- 6: https://github.com/kotlin/binary-compatibility-validator
Avoid breaking binary compatibility by adding new abstract interface members
New abstract getters added to existing public interfaces (GlobalState at line 212, QueryChannelsState at lines 273 and 276) are binary incompatible for already-compiled third-party implementations on JVM and can lead to AbstractMethodError at runtime. Prefer alternatives like default-backed methods, or introducing a new derived sub-interface for the added API.
Also ensure stream-chat-android-state/api/stream-chat-android-state.api is regenerated via ./gradlew apiDump after making the public API change (don’t hand-edit *.api files).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@stream-chat-android-state/api/stream-chat-android-state.api` at line 212, You
added new abstract getters (getGroupedUnreadChannels) to public interfaces
(GlobalState and QueryChannelsState) which breaks binary compatibility; instead,
add a default implementation or introduce a new derived sub-interface: provide a
default-backed method implementation in the interface (e.g., default
getGroupedUnreadChannels() returning an empty StateFlow or a pre-existing
fallback) or create a new interface (e.g., GlobalStateWithGroupedUnread /
QueryChannelsStateWithGroupedUnread) that extends the original and declares
getGroupedUnreadChannels, update usages to the new type, and then regenerate the
public API dump with ./gradlew apiDump so stream-chat-android-state.api is
updated rather than hand-editing it.
Comment on lines
+438
to
+449
| (event as? HasGroupedUnreadChannels)?.let { e -> | ||
| groupedUnreadChannels = groupedUnreadChannelsUpdater | ||
| .calculateUpdatedCounts(groupedUnreadChannels, e) | ||
| } | ||
| (event as? ChannelUpdatedEvent)?.let { e -> | ||
| groupedUnreadChannels = groupedUnreadChannelsUpdater | ||
| .calculateUpdatedCounts(groupedUnreadChannels, e) | ||
| } | ||
| (event as? ChannelUpdatedByUserEvent)?.let { e -> | ||
| groupedUnreadChannels = groupedUnreadChannelsUpdater | ||
| .calculateUpdatedCounts(groupedUnreadChannels, e) | ||
| } |
There was a problem hiding this comment.
Avoid double-applying grouped unread updates on channel update events.
On Line 438 you first apply HasGroupedUnreadChannels (authoritative replacement), then on Line 442 and Line 446 you apply ChannelUpdated* delta migration again. If a channel update event includes non-null groupedUnreadChannels, this second step mutates already-authoritative counts and can drift state.
💡 Proposed fix (process channel deltas first, authoritative payload last)
- (event as? HasGroupedUnreadChannels)?.let { e ->
- groupedUnreadChannels = groupedUnreadChannelsUpdater
- .calculateUpdatedCounts(groupedUnreadChannels, e)
- }
(event as? ChannelUpdatedEvent)?.let { e ->
groupedUnreadChannels = groupedUnreadChannelsUpdater
.calculateUpdatedCounts(groupedUnreadChannels, e)
}
(event as? ChannelUpdatedByUserEvent)?.let { e ->
groupedUnreadChannels = groupedUnreadChannelsUpdater
.calculateUpdatedCounts(groupedUnreadChannels, e)
}
+ (event as? HasGroupedUnreadChannels)?.let { e ->
+ groupedUnreadChannels = groupedUnreadChannelsUpdater
+ .calculateUpdatedCounts(groupedUnreadChannels, e)
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@stream-chat-android-state/src/main/java/io/getstream/chat/android/state/event/handler/internal/EventHandlerSequential.kt`
around lines 438 - 449, The grouped-unread update is being applied twice when an
event carries authoritative groupedUnreadChannels: currently
HasGroupedUnreadChannels is processed first and then
ChannelUpdatedEvent/ChannelUpdatedByUserEvent apply delta changes again; reorder
the logic so the delta handlers (ChannelUpdatedEvent and
ChannelUpdatedByUserEvent) run before the authoritative handler
(HasGroupedUnreadChannels) to ensure authoritative groupedUnreadChannels
provided by the event (variable groupedUnreadChannels and updater
groupedUnreadChannelsUpdater) overwrite deltas rather than being mutated
afterwards.
Comment on lines
+208
to
+213
| coroutineScope.launch { | ||
| runCatching { | ||
| clientState.initializationState.first { it == InitializationState.COMPLETE } | ||
| state.trackWatchedChannel(watchedFlow) | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid strongly capturing watchedFlow while waiting for initialization
Line 210 can wait indefinitely, and Line 211 keeps watchedFlow strongly referenced the whole time. If callers drop the returned flow before init completes, it still can’t be collected.
💡 Proposed fix
- coroutineScope.launch {
- runCatching {
- clientState.initializationState.first { it == InitializationState.COMPLETE }
- state.trackWatchedChannel(watchedFlow)
- }
- }
+ val watchedFlowRef = java.lang.ref.WeakReference(watchedFlow)
+ coroutineScope.launch {
+ runCatching {
+ clientState.initializationState.first { it == InitializationState.COMPLETE }
+ watchedFlowRef.get()?.let(state::trackWatchedChannel)
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| coroutineScope.launch { | |
| runCatching { | |
| clientState.initializationState.first { it == InitializationState.COMPLETE } | |
| state.trackWatchedChannel(watchedFlow) | |
| } | |
| } | |
| val watchedFlowRef = java.lang.ref.WeakReference(watchedFlow) | |
| coroutineScope.launch { | |
| runCatching { | |
| clientState.initializationState.first { it == InitializationState.COMPLETE } | |
| watchedFlowRef.get()?.let(state::trackWatchedChannel) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@stream-chat-android-state/src/main/java/io/getstream/chat/android/state/extensions/ChatClient.kt`
around lines 208 - 213, The coroutine currently holds a strong reference to
watchedFlow while waiting for initialization (coroutineScope.launch {
clientState.initializationState.first { ... };
state.trackWatchedChannel(watchedFlow) }), which can keep the flow alive
indefinitely; change the API to accept a provider/lambda (e.g.,
watchedFlowProvider: () -> Flow<...>) and update this code to wait for
clientState.initializationState.first { it == InitializationState.COMPLETE }
first and only then invoke state.trackWatchedChannel(watchedFlowProvider()), so
the actual Flow instance is created/retrieved after init completes and not
strongly captured during the wait.
Comment on lines
+469
to
+476
| val groupsParam = activeGroupedLogics | ||
| .mapNotNull { logic -> | ||
| val key = logic.groupKey() ?: return@mapNotNull null | ||
| val cfg = logic.groupedQueryConfig() ?: return@mapNotNull null | ||
| key to GroupedChannelsGroupQuery(limit = cfg.pageSize) | ||
| } | ||
| .toMap() | ||
| .takeIf { it.isNotEmpty() } |
There was a problem hiding this comment.
Groups without captured config are silently skipped during recovery.
Lines 471-473 skip a group if groupedQueryConfig() returns null, but the comment at lines 465-467 states that an empty {} per-group entry is meaningful and "filtering it out would cause re-sync to skip that group entirely." This contradicts the current logic.
If a grouped logic was initialized but its config was never captured (e.g., due to an early disconnect), recovery will skip that group. Consider using a default GroupedChannelsGroupQuery() when cfg is null:
Proposed fix
val groupsParam = activeGroupedLogics
.mapNotNull { logic ->
val key = logic.groupKey() ?: return@mapNotNull null
- val cfg = logic.groupedQueryConfig() ?: return@mapNotNull null
- key to GroupedChannelsGroupQuery(limit = cfg.pageSize)
+ val cfg = logic.groupedQueryConfig()
+ key to GroupedChannelsGroupQuery(limit = cfg?.pageSize)
}
.toMap()
.takeIf { it.isNotEmpty() }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val groupsParam = activeGroupedLogics | |
| .mapNotNull { logic -> | |
| val key = logic.groupKey() ?: return@mapNotNull null | |
| val cfg = logic.groupedQueryConfig() ?: return@mapNotNull null | |
| key to GroupedChannelsGroupQuery(limit = cfg.pageSize) | |
| } | |
| .toMap() | |
| .takeIf { it.isNotEmpty() } | |
| val groupsParam = activeGroupedLogics | |
| .mapNotNull { logic -> | |
| val key = logic.groupKey() ?: return@mapNotNull null | |
| val cfg = logic.groupedQueryConfig() | |
| key to GroupedChannelsGroupQuery(limit = cfg?.pageSize) | |
| } | |
| .toMap() | |
| .takeIf { it.isNotEmpty() } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@stream-chat-android-state/src/main/java/io/getstream/chat/android/state/sync/internal/SyncManager.kt`
around lines 469 - 476, The current mapping over activeGroupedLogics skips a
group when logic.groupedQueryConfig() returns null, which causes recovery to
omit groups whose config wasn't captured; update the logic building groupsParam
so that for each activeGroupedLogics entry with a non-null groupKey() you always
include an entry using groupedQueryConfig() when present otherwise fallback to a
default GroupedChannelsGroupQuery() (e.g., new/default instance) instead of
returning null; adjust the mapNotNull block around activeGroupedLogics to only
filter out entries with null groupKey() but never filter when cfg is null, and
construct GroupedChannelsGroupQuery(limit = cfg.pageSize) when cfg != null or
GroupedChannelsGroupQuery() when cfg == null.
Comment on lines
+87
to
+88
| @Suppress("LargeClass") | ||
| @OptIn(InternalStreamChatApi::class) |
There was a problem hiding this comment.
Document or remove the new @Suppress("LargeClass").
This suppression was introduced without rationale. Please add a short justification comment near it (or remove it) to keep suppression usage auditable.
As per coding guidelines: "Use @OptIn annotations explicitly in Kotlin code; avoid suppressions unless documented".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@stream-chat-android-state/src/test/java/io/getstream/chat/android/state/internal/SyncManagerTest.kt`
around lines 87 - 88, The `@Suppress`("LargeClass") usage was added without
rationale; either remove that suppression or add a one-line justification
comment directly above it explaining why the test class (SyncManagerTest) is
intentionally large (e.g., "test class aggregates many scenarios for SyncManager
integration, keeping related tests together") so suppression is auditable; keep
the `@OptIn`(InternalStreamChatApi::class) as-is and ensure the justification
comment references the suppression symbol so reviewers can see why
`@Suppress`("LargeClass") is necessary.
…t' into feature/grouped-channels-endpoint
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goal
Add support for the server-driven grouped-channels API (
POST /channels/grouped), where the backend partitions the channel list into named groups (e.g.direct,support) and returns per-group channels, pagination cursors, and unread counts. Surface those grouped unread counts on relevant chat events, and provide a ComposeChannelListViewModelpath that drives a UI off a group key without the consumer needing to know about filter/sort.Implementation
ChatClient.queryGroupedChannels(limit, groups, watch, presence)returningGroupedChannels(per-groupchannels+unreadChannels+next/prevcursors). Per-group request options viaGroupedChannelsGroupQuery. Backed byPOST /channels/grouped(ChannelApi).QueryGroupedChannelsListener; theStatePluginimplementation merges returned per-group unread counts intoGlobalState.groupedUnreadChannelsand routes each returned group into a state keyed by a new sealedQueryChannelsIdentifier:QueryChannelsIdentifier.Standard(filter, sort)— existing offset-paginated pathQueryChannelsIdentifier.Grouped(groupKey)— new cursor-paginated pathQueryChannelsLogicbranches on identifier.applyGroupedResultreplaces channels on the first page (resettingchannelsOffsetdefensively to keep the Standard offset paginator from picking up stale state), appends on subsequent pages (driven off the request'snextcursor), and persists per-group state under agroupKey-derived DB key.HasGroupedUnreadChannelsmarker onNewMessageEvent,NotificationMessageNewEvent,NotificationMarkReadEvent,NotificationMarkUnreadEvent,NotificationChannelDeletedEvent,NotificationChannelTruncatedEvent.EventHandlerSequentialupdatesGlobalState.groupedUnreadChannelswhenever an inbound event carries the map.GroupedUnreadChannelsUpdateris the single calculator: events with a non-null map replace the current state,channel.updated/channel.updated_by_userevents migrate per-group counts when the channel'sgroupfield changes, andqueryGroupedChannelsresults merge per-group counts.GroupAwareChatEventHandlerclassifies channel-bearing events using a pluggableChannelGroupResolver. The default resolver readschannel.extraData["group"]and always includes an"all"sentinel. Channels are routed Add/Remove/Skip per inbound group. TheLogicRegistryauto-install of the default factory is idempotent — it won't clobber a factory another caller has already installed on the state. Member/CID events delegate toDefaultChatEventHandlerunchanged.ChannelListViewModel(chatClient, groupKey, ...)constructor + matchingChannelViewModelFactory(chatClient, groupKey, ...). Wires the VM to the identifier-keyed state viainitGroupedQueryChannelsAsState, with a group-aware event handler factory keyed ongroupKey. Pagination uses cursor-basedqueryGroupedChannels(groups = mapOf(groupKey to GroupedChannelsGroupQuery(next = cursor))). The Standard path is untouched.SyncManager.restoreActiveChannels()splits standard vs grouped reconnect paths. Grouped queries are refreshed via a singlequeryGroupedChannels()call; manually-watched channels are re-watched viaWatchedChannelRecord/WatchedChannelStateFlow(weak-referenced fromStateRegistry). Recovery assumes all active grouped queries share the same request-levellimit/watch/presenceflags — the first captured config wins.QueryChannelsSpec: new optionalgroupKeyfield for grouped identity.cidsremains a mutablevarfor backward compatibility with prior versions; the two-arg constructor and 2-argcopyare preserved for source/binary compat.groupKeycolumn onQueryChannelsEntity. Uses the existingfallbackToDestructiveMigrationstrategy.Testing
Unit-test coverage added for each layer:
ChatClientGroupedChannelsApiTestsMoshiChatApiTest,QueryGroupedChannelsResponseAdapterTestgrouped_unread_channelsfield):EventMappingTestArgumentsGroupAwareChatEventHandlerTest,DefaultChannelGroupResolverTestGroupedUnreadChannelsUpdaterTestQueryGroupedChannelsListenerStateTestSyncManagerTestStateRegistryTest,QueryChannelsMutableStateTestLogicRegistryTestQueryChannelsLogicgrouped behavior:QueryChannelsLogicGroupedTestcoversapplyGroupedResult(first-page replace, subsequent-page append, cursor/end-of-channels, DB persistence, defensivechannelsOffsetreset, no-op on Standard) andloadOfflineGroupedChannels(cache load, race-condition guard, null cache, no-op on Standard)ChatClientStateCallsTestgroupedUnreadChannelspropagation:EventHandlerSequentialTestManually verified the Compose sample app in both Standard and Grouped modes: initial render, cursor pagination, event-driven Add/Remove/Skip across groups, reconnect/recovery, and grouped unread counts updating from inbound events.
Summary by CodeRabbit
Release Notes