Fix showing loading state when channels are loaded offline #6051
Conversation
WalkthroughfetchChannelsFromCache now returns null when the query spec is not present in the DB instead of an empty list. QueryOffline and callers were updated to treat "spec not found" (null) separately from "spec found but no channels", and tests were added covering these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client
participant Logic as QueryChannelsLogic
participant DBLogic as QueryChannelsDatabaseLogic
participant Repo as RepositoryFacade
participant State as QueryChannelsStateLogic
Caller->>Logic: queryOffline(pagination, spec)
alt query already loading
Logic->>Caller: return / skip
else not loading
Logic->>State: set loading (first/more)
Logic->>DBLogic: fetchChannelsFromCache(pagination, spec)
DBLogic->>Repo: findQuerySpec(spec)
alt spec not found
Repo-->>DBLogic: spec missing
DBLogic-->>Logic: null
Logic->>State: leave loading state (no cache update)
Note right of Logic: proceed to online fetch
else spec found
Repo-->>DBLogic: spec found
DBLogic->>Repo: selectChannels(cids, pagination)
Repo-->>DBLogic: channels[]
DBLogic-->>Logic: channels[]
Logic->>State: addChannels(channels)
Logic->>State: clear loading
Logic->>Repo: insertQueryChannels(spec)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
SDK Size Comparison 📏
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
35-37: Polish wording of the changelog entry for clarityThe current phrasing is understandable but a bit awkward. Consider tightening it to better describe the actual behavior change:
Suggested diff
- - Fix showing channels loading state when querying offline and no matching channels are found in the DB. [#6051](https://github.com/GetStream/stream-chat-android/pull/6051) + - Fix channel list remaining in loading state when querying offline and no matching channels are found in the DB. [#6051](https://github.com/GetStream/stream-chat-android/pull/6051)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)stream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsDatabaseLogic.kt(1 hunks)stream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogic.kt(2 hunks)stream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsDatabaseLogicTest.kt(1 hunks)stream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogicTest.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{kt,kts}
📄 CodeRabbit inference engine (AGENTS.md)
Format and apply Kotlin style with Spotless (4 spaces, no wildcard imports, licence headers)
Files:
stream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsDatabaseLogic.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsDatabaseLogicTest.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogic.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogicTest.kt
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Use@OptInannotations explicitly; avoid suppressions unless documented
Document public APIs with KDoc, including thread expectations and state notes
Files:
stream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsDatabaseLogic.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsDatabaseLogicTest.ktstream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogic.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogicTest.kt
**/src/test/**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/src/test/**/*.kt: Use backtick test names (for example:funmessage list filters muted channels()) for readability
Use deterministic tests withrunTest+ virtual time for concurrency-sensitive logic (uploads, sync, message state)
Keep helper extensions private/internal in test files
Files:
stream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsDatabaseLogicTest.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogicTest.kt
🧠 Learnings (4)
📚 Learning: 2025-12-17T15:00:07.506Z
Learnt from: CR
Repo: GetStream/stream-chat-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T15:00:07.506Z
Learning: Applies to **/src/test/**/*.kt : Use deterministic tests with `runTest` + virtual time for concurrency-sensitive logic (uploads, sync, message state)
Applied to files:
stream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsDatabaseLogicTest.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogicTest.kt
📚 Learning: 2025-12-17T15:00:07.506Z
Learnt from: CR
Repo: GetStream/stream-chat-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T15:00:07.506Z
Learning: Applies to **/stream-chat-android-compose/**/*Test.kt : Add Paparazzi snapshots for Compose UI regressions and run `verifyPaparazziDebug`
Applied to files:
stream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsDatabaseLogicTest.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogicTest.kt
📚 Learning: 2025-12-17T15:00:07.506Z
Learnt from: CR
Repo: GetStream/stream-chat-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T15:00:07.506Z
Learning: Applies to **/stream-chat-android-ui-components/**/*Test.kt : Record Shot baselines when behaviour changes in XML kit UI tests
Applied to files:
stream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsDatabaseLogicTest.ktstream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogicTest.kt
📚 Learning: 2025-12-17T15:00:07.506Z
Learnt from: CR
Repo: GetStream/stream-chat-android PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T15:00:07.506Z
Learning: Applies to **/src/test/**/*.kt : Use backtick test names (for example: `fun `message list filters muted channels`()`) for readability
Applied to files:
stream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsDatabaseLogicTest.kt
🧬 Code graph analysis (2)
stream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsDatabaseLogicTest.kt (2)
stream-chat-android-client-test/src/main/java/io/getstream/chat/android/client/test/Mother.kt (1)
randomQueryChannelsSpec(654-658)stream-chat-android-core/src/testFixtures/kotlin/io/getstream/chat/android/Mother.kt (2)
randomChannelConfig(584-585)randomString(94-98)
stream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogicTest.kt (1)
stream-chat-android-core/src/main/java/io/getstream/chat/android/models/Filters.kt (1)
eq(57-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test compose (2)
- GitHub Check: Test compose (1)
- GitHub Check: Test compose (0)
🔇 Additional comments (19)
stream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsDatabaseLogic.kt (1)
41-63: LGTM! Clear null semantics for cache miss.The updated signature and KDoc clearly distinguish between "spec not found in DB" (null) and "spec found with zero channels" (empty list). The implementation correctly uses safe navigation to return null when no cached spec exists.
stream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsDatabaseLogicTest.kt (8)
41-54: LGTM! Clean test setup with appropriate mocks.The test class correctly initializes all required dependencies as mocks and creates the system under test with proper dependency injection.
55-69: LGTM! Test follows guidelines and verifies delegation correctly.Uses backtick naming convention and
runTestas per coding guidelines.
71-82: LGTM! Correctly tests null spec scenario.The test validates the new behavior where a null
queryChannelsSpecinput returns null, indicating no cached data to fetch.
84-100: LGTM! Validates cache miss behavior.The test correctly verifies that when the repository returns null for a spec lookup,
fetchChannelsFromCachereturns null.
102-138: LGTM! Comprehensive happy path test.The test thoroughly validates the successful fetch scenario, including pagination parameters, cid matching, and repository interactions.
140-164: LGTM! Critical test for the PR's core fix.This test is essential as it validates the distinction between "spec not found" (null) and "spec found with zero channels" (empty list), which is the key to fixing the offline loading state issue described in the PR.
166-204: LGTM! Delegation tests are complete and correct.Both
selectChannelandselectChannelstests properly verify delegation to the channel repository.
206-235: LGTM! Insert operations properly tested.Both
insertQueryChannelsandinsertChannelConfigstests correctly verify delegation to their respective repositories.stream-chat-android-state/src/main/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogic.kt (2)
51-71: LGTM! Correctly implements the split offline query logic.The implementation properly distinguishes between two scenarios as intended:
- Null result: No cached spec found → maintains loading state and waits for online data
- Non-null result: Spec found with 0+ channels → optimistically updates state and clears loading indicator
This directly addresses the PR's objective of preventing prolonged loading screens when querying offline with no matching channels in the database.
91-107: LGTM! Proper handling of nullable result with helpful logging.The method correctly handles the nullable return type from the database logic and provides clear diagnostic logging to distinguish between "no cached spec" (null) and "spec found with N channels" (non-null).
stream-chat-android-state/src/test/java/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogicTest.kt (8)
42-77: LGTM! Well-structured test setup.The test class properly initializes all dependencies with mocks and uses
@BeforeEachto ensure a clean state for each test. The use ofTestCoroutineRulealigns with the coding guideline for deterministic coroutine tests.
79-93: LGTM! Correctly verifies early return guard.The test ensures that when a query is already in progress,
queryOfflinereturns early without performing any operations, preventing duplicate queries.
95-110: LGTM! Validates first page loading state.The test correctly verifies that when offset is 0, the first page loading indicator is set, not the "loading more" indicator.
112-127: LGTM! Validates pagination loading state.The test correctly verifies that when offset > 0, the "loading more" indicator is set, not the first page loading indicator.
129-147: LGTM! Verifies correct parameter passing.The test ensures that
fetchChannelsFromCachereceives the correct pagination request and query spec.
149-165: LGTM! Critical test for the PR's core fix.This test validates the essential behavior where, when no cached spec is found (null return), the loading state remains active to await online data. This prevents immediately showing an empty state when offline with no cached data, which was the PR's objective.
167-189: LGTM! Validates optimistic cache update flow.The test correctly verifies that when cached channels are found, they are added to the state and the loading indicator is cleared, providing immediate feedback to the user.
191-228: LGTM! Completes the test coverage for offline query scenarios.The first test validates the crucial case where a cached spec exists but has no channels (empty list), ensuring the loading state is properly cleared even with zero results. The second test verifies that the query spec is persisted after adding channels, maintaining consistency between in-memory state and database.
...va/io/getstream/chat/android/state/plugin/logic/querychannels/internal/QueryChannelsLogic.kt
Show resolved
Hide resolved
gpunto
left a comment
There was a problem hiding this comment.
Forgot to approve: just left a comment to understand a similar scenario (which is not changed by this PR)
|
Right! We were checking for emptiness 👍 |
…nels_are_loaded_offline
…nels_are_loaded_offline # Conflicts: # CHANGELOG.md
|
🎯 Goal
We currently have the following scenario: When opening the ChannelList offline, and there are no matching channels in the DB, we still show a loading screen for up to 5 seconds. This is caused by two seemingly unrelated things:
QueryChannelscall for up to 5 seconds, if there is no currentconnectionId(no WS connection)Because the DB query doesn't return channels we don't reset the loading state. Additionally, since the
QueryChannelscall is postponed for 5 seconds, the loading state is reset only after 5 seconds.In this PR we now separate the logic for updating the loading state in
QueryChannelsStatecovering two separate scenarios:Note: This has the drawback that if the first load of a specific query is done offline, the same 'bug' will appear again. But we currently cannot reliably know from the
QueryChannelsLogichow the actual query will behave (will it be postponed, will it fail or succeed), so we cannot pre-emptively hide the loading state.🛠 Implementation details
Implement different handling when we don't have any data in the DB for the current query (first run on the query), and different handling when we have data for the query (zero or more channels)
🎨 UI Changes
before.mp4
after-firstload.mp4
after-secondload.mp4
after-nonempty.mp4
🧪 Testing
No channelsscreen should be immediately visible without bigger delayProvide the patch summary here
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.