Keep the loader up while a grouped channel list fetches its first page - #6674
Merged
gpunto merged 6 commits intoSep 4, 2026
Merged
Conversation
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
gpunto
force-pushed
the
gianmarcodavid/and-1460-grouped-channel-list-loading-state
branch
from
August 31, 2026 11:32
7a313f5 to
849bf1b
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. WalkthroughGrouped channel queries now track first-page loading per group. Offline cache misses preserve the loading state. Failed and incomplete responses finish pending first-page loads. Tests cover routing, pagination, failures, omitted groups, and emitted channel state. ChangesGrouped channel loading
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The change correctly keeps grouped lists in a loading state during initial network fetches, but overlapping requests for the same group could allow one response to hide the loader while another is still running. This is a bounded mergeable risk that warrants owner awareness and follow-up. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
…rst page
Opening a grouped channel list with nothing cached for that group showed the
empty state ("no channels") for the whole duration of the first
queryGroupedChannels round trip, instead of the loading indicator. On a slow
connection that is a visible "No channels" flash before the list populates.
Standard channel lists do not behave this way; they deliberately hold the loader
until the network answers.
loadOfflineGroupedChannels() cleared the first-page loading flag unconditionally,
so a cache miss moved channelsStateData from Loading to OfflineNoResults while
the request was still in flight. The unconditional clear was deliberate: grouped
state is initialised separately from the fetch, so that call site cannot tell an
in-flight first page from one that is never coming, and leaving the flag raised
there would spin forever for an app that inits grouped state without querying.
Raise the flag at the point that does know a fetch is starting instead. The
request listener sets it per named first-page group, and only for a group that
has never loaded. The guard is a null check rather than an emptiness check, and
the difference matters: a group that loaded and came back empty holds a non-null
empty map, because addChannelsState writes the merged map even for an empty
result. Treating that as "empty, so show the loader" would put a settled empty tab
back into a spinner on every reconnect, since SyncManager recovers grouped lists
through a cursor-less request that reaches this listener. Standard lists never do
that: their recovery goes through queryFirstPage, which is a no-op for grouped
identifiers and otherwise calls the API directly, with no listener and no flag.
Raising it over a populated list would be wrong for a second reason too, since
ChannelsStateData reports Loading whenever the flag is set regardless of content.
The offline load now clears the flag once there is something on screen, whether
that is the cached channels it just added or channels a concurrent result already
applied. A miss with nothing in state leaves the flag alone, so it stays raised
while a request is in flight and stays false when none is coming, in which case
the empty state shows immediately as before.
Ending a load that produced no channels of its own needs both halves:
ChannelsStateData reports Loading while the flag is set *or* while channels are
still null, and a request that fails before the offline load ran leaves them null.
finishFirstPageLoad() initialises the channel map and clears the flag together,
so a failure cannot swap the flash for a permanent spinner. It runs for failed
requests and for requested groups the response leaves out, which never reach
applyGroupedResult and would otherwise keep their loader forever.
State reads in the new paths share groupedResultMutex with the offline load and
the result apply, so the emptiness check cannot observe a half-applied update.
Verified on the grouped-channels sample against a locally published build. Cold
start with an empty database now goes Loading -> Result rather than
Loading -> OfflineNoResults -> Result, and a warm start still renders from cache.
The failure path is covered by unit tests only, as the sample cannot reach the
list without a working connection.
The flag is only raised when the request starts before the offline read. In the
opposite order the read misses, initializeChannelsIfNeeded() flips the channel map
from null to empty, and the group no longer counts as never-loaded, so the empty
state shows for the round trip as it does today. Nothing at that call site can
tell "a query is coming" from "none ever will", so this ordering is not fixable
there; an app that issues its grouped query at screen open, as the sample does,
gets the fix.
Known gaps, all pre-existing and unchanged here. A request with groups = null
relies on the server's default group set, so the keys are unknown until the
response and no loader can be raised for it. A grouped first page that fails does
not set recoveryNeeded, and recoverAll is false on the first connect, so such a
failure is not retried until a later connection event. And applyGroupedResult
replaces a first page by removing the existing channels before adding the new
ones, so a warm start emits a brief OfflineNoResults between the two writes -
reproduced on roughly two runs in five, with and without the persistence change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gpunto
force-pushed
the
gianmarcodavid/and-1460-grouped-channel-list-loading-state
branch
from
August 31, 2026 11:37
849bf1b to
ec43e90
Compare
Contributor
SDK Size Comparison 📏
|
gpunto
marked this pull request as ready for review
August 31, 2026 16:00
andremion
approved these changes
Sep 1, 2026
andremion
left a comment
andremion
left a comment
Contributor
There was a problem hiding this comment.
Looks good. One question on the ordering inline, plus a couple of nits.
Drop a comment duplicating the paragraph above it, call setLoadingFirstPage directly now that the loadingPerPage branch is dead at that call site, and extend the real-state test to cover the failure end of the chain rather than leaving it to mocks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng it The raise guard read `getChannels() == null`, which conflated "this group has never loaded" with "nothing has initialised the channel map yet". The offline read initialises the map on a cache miss, so a request arriving after it never raised the loader and the list showed the empty state for the whole round trip. Only the ordering where the request starts first was fixed. Track completion explicitly instead. hasCompletedAQuery is set when a grouped result is applied and when a load is ended, so it distinguishes a group that has never loaded from one that loaded and is genuinely empty, which the channel map alone cannot express: both are empty. The guard keeps the emptiness check alongside it. Without the marker a settled empty group returns to a spinner on every reconnect, since SyncManager recovers grouped lists through this listener. Without the emptiness check a request hides cached channels behind a spinner, because ChannelsStateData reports Loading whenever the flag is set regardless of content. A failed query counts as completion, so a retry shows the empty state rather than returning to the loader. That matches what a standard list does while recovering. Covered by a real-state test asserting the offline-read-first ordering now reaches Loading, which fails against the previous guard, plus mock tests for the cached and settled-empty cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
hasCompletedAQuery was set whenever a load ended, so a failed first page settled the group and a retry showed the empty state. A standard list does the opposite: after a failed empty first page the channel map is empty and loading is false, so currentLoading resolves to loading, queryOffline does not early-return, and the retry raises the loader. finishFirstPageLoad now takes `completed`. A group the response omits has been answered and settles; a failure has not, so the group stays never-loaded and a later request raises the loader again. Recovery matches either way, since the standard path does not raise there either. Also revert an unrelated local gradle.properties change that was committed by accident. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing dedupes concurrent grouped queries, so two first pages for one group can be in flight together. The loading flag is shared, so the earlier completion ends the load while the later request is still running, and the list shows the empty state for that window. That window is the behaviour this branch removes elsewhere, narrowed to the overlap, and it recovers on its own once the later request answers. Gating the clear on an in-flight count would close it, but the count is incremented in doOnStart and decremented in doOnResult, and DoOnResultCall.cancel cancels the scope the consumer runs in. A leaked start would never return to zero, so the loader would stop clearing at all, including on a later success. The test records that trade so a future count cannot land silently.
- Name the omitted-group branch as the guard it is. The server answers every requested group, with an empty channel list when the group has nothing, so it is expected to be a no-op; it stays because a group the response left out would never reach applyGroupedResult and its loader would never come down. - Note in KDoc that an explicit Call.cancel between the raise and the result leaves the group on the loader until a later grouped query finishes. Cancelling the calling coroutine or its scope still runs the result listener, and nothing in the SDK cancels this call. - Set hasCompletedAQuery inside groupedResultMutex, matching applyGroupedResult, so both writers hold the lock. - Drop a test that duplicated the stub, call and assertion of the one below it.
|
gpunto
deleted the
gianmarcodavid/and-1460-grouped-channel-list-loading-state
branch
September 4, 2026 10:32
Contributor
|
🚀 Available in v6.43.0 |
pull Bot
pushed a commit
to Siriusmene/stream-chat-android
that referenced
this pull request
Sep 4, 2026
GetStream#6679) * state: Keep the loader up while a grouped channel list fetches its first page (develop) Port of GetStream#6674 to develop. Opening a grouped channel list with nothing cached for that group showed the empty state ("no channels") for the whole duration of the first queryGroupedChannels round trip, instead of the loading indicator. Standard channel lists hold the loader until the network answers. loadOfflineGroupedChannels() cleared the first-page loading flag unconditionally, so a cache miss moved channelsStateData from Loading to OfflineNoResults while the request was still in flight, and nothing raised it again. Raise it in the request listener instead, for each named first-page group, guarded on !hasCompletedAQuery && getChannels().isNullOrEmpty(). Both halves matter: without the marker a settled empty group returns to a spinner on every reconnect, since SyncManager recovers grouped lists through this listener; without the emptiness check a request hides cached channels behind a spinner. A failure does not count as completion, so a retry raises the loader again, matching queryOffline for a standard list. Recovery of a group whose first page failed does differ from standard, which never raises there because queryFirstPage bypasses the listener; grouped showing a spinner while the retry runs is the more accurate of the two. Clear the flag only once there is something to show, and end the load on failure and for a group the response omits. Only the query-channels path is affected. The two channel-logic implementations on this branch, ChannelLogicImpl and ChannelLogicLegacyImpl, are the chat screen and are untouched here; that half is tracked separately in AND-1462. Differences from the v6 version are mechanical: the files live under stream-chat-android-client rather than stream-chat-android-state, ChannelsStateData is imported from client.api.state, and QueryChannelsStateLogic takes an extra isLocalUnreadCountEnabled parameter that the test harness now passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * state: Pin grouped first-page behaviour when two requests overlap Nothing dedupes concurrent grouped queries, so two first pages for one group can be in flight together. The loading flag is shared, so the earlier completion ends the load while the later request is still running, and the list shows the empty state for that window. That window is the behaviour this branch removes elsewhere, narrowed to the overlap, and it recovers on its own once the later request answers. Gating the clear on an in-flight count would close it, but the count is incremented in doOnStart and decremented in doOnResult, and DoOnResultCall.cancel cancels the scope the consumer runs in. A leaked start would never return to zero, so the loader would stop clearing at all, including on a later success. The test records that trade so a future count cannot land silently. * state: Address review on the grouped first-page loader - Name the omitted-group branch as the guard it is. The server answers every requested group, with an empty channel list when the group has nothing, so it is expected to be a no-op; it stays because a group the response left out would never reach applyGroupedResult and its loader would never come down. - Note in KDoc that an explicit Call.cancel between the raise and the result leaves the group on the loader until a later grouped query finishes. Cancelling the calling coroutine or its scope still runs the result listener, and nothing in the SDK cancels this call. - Set hasCompletedAQuery inside groupedResultMutex, matching applyGroupedResult, so both writers hold the lock. - Drop a test that duplicated the stub, call and assertion of the one below it. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
3 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
A grouped channel list with nothing cached for a group showed the empty state ("no channels") for the whole first
queryGroupedChannelsround trip instead of the loader. Standard channel lists do not behave this way: they hold the loader until the network answers.Closes AND-1460
Targets
v6.Implementation
loadOfflineGroupedChannelscleared the first-page loading flag unconditionally, so a cache miss movedchannelsStateDatafromLoadingtoOfflineNoResultswhile the request was still in flight, and nothing raised it again:onQueryGroupedChannelsRequestonly captured the query config.!hasCompletedAQuery && getChannels().isNullOrEmpty()and both halves are load-bearing. Without the marker a settled empty tab returns to a spinner on every reconnect, sinceSyncManagerrecovers grouped lists through a cursor-less request that reaches this listener. Without the emptiness check a request hides channels the offline read just loaded from cache, becauseChannelsStateDatareportsLoadingwhenever the flag is set regardless of content.hasCompletedAQueryis needed because state alone cannot express the difference: a group that has never loaded and one that loaded and is genuinely empty both hold an empty channel map.queryOfflinefor a standard list. An omitted group would count, since the server answered, but that branch is a guard rather than a live path: the server returns every requested group, with an empty channel list when the group has nothing. It stays because such a group would never reachapplyGroupedResultand the loader it raised would never come down.applyGroupedResult; ending it initialises the channel map as well as clearing the flag, becauseChannelsStateDatareportsLoadingwhile channels are still null.Scope is limited to grouped lists: the new methods are called only from
QueryGroupedChannelsListenerState, andloadOfflineGroupedChannelsreturns early for non-grouped identifiers.Two behaviours worth calling out. Recovery of a group whose first page failed raises the loader on grouped, where standard recovery never does, because
queryFirstPagebypasses the listener entirely. That is deliberate: a spinner while the retry runs is more accurate than a stale empty state. And a request withgroups = nullkeeps today's behaviour, since the keys are unknown until the response, so the mechanism is inert rather than half-engaged and cannot leave a list loading.Testing
Unit tests over a real
QueryChannelsMutableStateassert the emittedChannelsStateDatarather than setter calls: an in-flight first page on a cache miss staysLoading, a request arriving after the offline read still raises, and a failed load ends onOfflineNoResults. Reinstating the unconditional clear, or the earlier== nullguard, fails them.Mock tests cover the raise per first-page group, no raise while paginating, no raise over cached channels or a settled empty group, a failed page staying retryable, and the load being ended on failure and for an omitted group.
Verified on a grouped-channels sample built against this branch, cold start with an empty database on a throttled network:
OfflineNoResults -> Result(10) -> Result(20)Loading -> Result(10) -> Result(20)Also ran the sample with the ordering flipped so the offline read runs before the query, which is the case the completion marker adds:
Loading -> Result, so the new raise clears. A warm start still renders straight from cache.