client: Fix the unread count being overwritten by a stale channel snapshot - #6605
Open
andremion wants to merge 11 commits into
Open
client: Fix the unread count being overwritten by a stale channel snapshot#6605andremion wants to merge 11 commits into
andremion wants to merge 11 commits into
Conversation
The count check asserted only that the expected text exists somewhere, so a CI failure carried no information about what the button actually showed. Comparing against the rendered count inside the button keeps the same strictness and makes the failure message carry the actual value.
The retry rule attaches logcat, screenshot, and hierarchy to every failed attempt, and the e2e lane already extracts the allure results to the runner, but the failure artifact only carried the mock server logs. TestOps deduplicates same-history results, so a failure on a single API level loses its attachments when the other levels pass; the artifact is the only reliable place to read them.
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
WalkthroughThe PR updates channel read-state processing and tests, preserves server-confirmed read dates, supports queued unread events, parameterizes E2E runs, uploads Allure results, and improves unread-count UI matching. ChangesRead-state and E2E updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant Fastlane
participant E2ETests
participant Artifacts
Workflow->>Fastlane: pass api_level and optional test_class
Fastlane->>E2ETests: run selected class or batched tests
E2ETests->>Artifacts: write mock-server logs and Allure results
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-compose-sample/src/androidTestE2eDebug/kotlin/io/getstream/chat/android/compose/robots/UserRobotMessageListAsserts.kt`:
- Around line 341-344: Update the selector in the message-list unread-count
assertion near MessageListPage.MessageList.scrollToFirstUnreadButton to avoid
the hardcoded English “unread” text. Use a locale-neutral matcher that
identifies the numeric count within the scoped button, while preserving the
existing expectedText comparison and wait behavior.
🪄 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 Plus
Run ID: de39816b-b9d1-4e46-a28b-c655b996794b
📒 Files selected for processing (3)
.github/workflows/e2e-test-cron.yml.github/workflows/e2e-test.ymlstream-chat-android-compose-sample/src/androidTestE2eDebug/kotlin/io/getstream/chat/android/compose/robots/UserRobotMessageListAsserts.kt
Contributor
SDK Size Comparison 📏
|
andremion
changed the title
Marking a channel read optimistically stamped the read dates with the newest loaded message's created_at, and updateCurrentUserRead dropped any event not strictly newer than that clock. On a slow device the optimistic mark-read can execute in the middle of an incoming burst, so every event still queued behind it was discarded as outdated and the unread count settled below the real value. The server's correct count could not repair it, because the read-state merge prefers the local side with the newer event clock. The optimistic update now only zeroes the count: the read dates stay anchored to server-confirmed values, advanced by query responses and read events. The counter skips messages already covered by last_read, matching the iOS SDK's seen check, counts everything else exactly once via the processed-id cache, and the event clock only moves forward. Applies to both the current and the legacy state implementations.
andremion
force-pushed
the
andrerego/e2e-failure-diagnostics
branch
from
July 31, 2026 08:44
10e2458 to
7e17c9a
Compare
The count is the only text inside the button, so matching any text scoped to the button avoids assuming the English wording while keeping the comparison against the localized expected string.
andremion
changed the title
The count could be wrong with no way to tell which condition suppressed it. Every skip now names its reason and every increment logs the new value, so a wrong count is traceable from a logcat. The conditions are collected into one function in their original order, mirroring how the iOS SDK reports the same reasons.
Reproducing a failure that only appears on one API level meant dispatching the nightly matrix and waiting for all levels to finish. The PR workflow now takes an api_level and a test_class on manual dispatch: with a class set, batching is skipped so each batch job runs that class and gives an independent sample, which is what a flaky test needs.
Two instances of the same channel logged under one tag, so a read state that looked stale could not be told from a second instance holding its own. The tag now carries a sequence number, as the legacy implementation already does, and a reads update logs the current user's unread transition with the incoming value.
The channel list pushes its own copy of a channel into each channel's state, and that copy carries a read whose count was captured earlier. It was written over the count the channel state had built from message events, so an unread count of 25 became 11 and the server could not correct it afterwards. Two changes make the count hold. The merge that arbitrates an incoming read now decides on the read position: a count is only taken when it arrives with a newer lastRead, which is what happens when the channel is read elsewhere. lastReceivedEventDate cannot decide it, since the server never sends that field and mapping synthesises it. And the local increment no longer goes through that merge at all: it writes into the read map atomically, because it is the local count rather than an incoming read, and a burst of events must not lose increments.
A mark unread moves the read position backwards on purpose and the count it reports is the truth, but it was applied through the same path that protects the local count against stale payloads, so the count was kept and the unread state did not appear. The event now replaces the read state directly, and the arbitration stays conservative for payloads: an incoming count is taken only when the read position moved forward.
andremion
changed the title
Contributor
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
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/src/test/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateImplReadReceiptsTest.kt (1)
237-482: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAdd a concurrency test for the atomic increment claim.
The fixture updates and new tests (Lines 430-448, 564-587) correctly validate the new staleness and mark-read semantics.
updateCurrentUserRead's production comment states the read-modify-write "has to be atomic to survive a burst of events," but this suite only calls it sequentially. Add a test that launches several concurrentupdateCurrentUserReadcalls (different message IDs) underrunTestand confirms the finalunreadMessagescount equals the number of distinct messages, with no lost increments.As per path instructions: "For concurrency-sensitive logic such as uploads, synchronization, and message state, add deterministic tests using
runTestand virtual time."🤖 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/internal/state/plugin/state/channel/internal/ChannelStateImplReadReceiptsTest.kt` around lines 237 - 482, Add a deterministic concurrency test for ChannelStateImpl.updateCurrentUserRead that launches multiple concurrent calls under runTest, using distinct message IDs and a shared initial read state. Await all launched jobs, then assert unreadMessages equals the number of unique messages to verify no increments are lost.Source: Path instructions
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/legacy/ChannelStateLogic.kt (1)
191-216: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPort the
lastRead-based read arbitration and atomic local increments into the legacy channel state path.
ChannelStateLogic.ktstill treatslastReceivedEventDateas the “more recent” arbiter inmergeCurrentUserRead(line 230), so stale query-channel reads can preserve a stale unread count. Use thelastRead-based comparison shared withChannelStateImpl.kt. Also addChannelStateLegacyImpl.replaceRead(...)and use it forNotificationMarkUnreadEventinChannelEventHandlerLegacyImpl.kt; the legacy path still callsupdateRead, which sends the replace throughupdateReads/mergeCurrentUserReadand can preserve an older unread count.ChannelStateLogicTest.ktneeds matchinglastReadassertions.🤖 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/internal/state/plugin/logic/channel/internal/legacy/ChannelStateLogic.kt` around lines 191 - 216, Update legacy read arbitration in mergeCurrentUserRead and updateReads to compare lastRead using the same behavior as ChannelStateImpl, rather than lastReceivedEventDate, and preserve the corresponding unread-count outcome. Add ChannelStateLegacyImpl.replaceRead and route NotificationMarkUnreadEvent handling in ChannelEventHandlerLegacyImpl through it instead of updateRead, ensuring the replacement is applied atomically. Extend ChannelStateLogicTest with matching lastRead-based assertions.
🧹 Nitpick comments (1)
fastlane/Fastfile (1)
110-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for branch precedence and empty input.
Test
use_backend: true, a non-emptytest_class,nil, and"". Assert that backend selection wins, targeted selection bypassesbatch_tests, and empty input preserves batching.As per coding guidelines,
**/*: “add or refresh tests for changed behavior.”🤖 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 `@fastlane/Fastfile` around lines 110 - 116, Add or update Fastfile tests covering the test-filter branching around targeted_class: verify use_backend: true takes precedence over a non-empty test_class, non-empty test_class bypasses batch_tests, and both nil and "" preserve batching. Use the existing test symbols and assert the resulting filter and batching behavior.Source: Coding guidelines
🤖 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 `@fastlane/Fastfile`:
- Around line 110-116: Validate options[:test_class] against the documented
fully qualified test-class-name pattern before assigning targeted_class or
constructing test_filter, rejecting invalid workflow input. Update the targeted
test execution path so the validated class value is passed to sh as a separate
argument rather than interpolated into a shell command string; keep the
backend_test_class path unchanged.
---
Outside diff comments:
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/legacy/ChannelStateLogic.kt`:
- Around line 191-216: Update legacy read arbitration in mergeCurrentUserRead
and updateReads to compare lastRead using the same behavior as ChannelStateImpl,
rather than lastReceivedEventDate, and preserve the corresponding unread-count
outcome. Add ChannelStateLegacyImpl.replaceRead and route
NotificationMarkUnreadEvent handling in ChannelEventHandlerLegacyImpl through it
instead of updateRead, ensuring the replacement is applied atomically. Extend
ChannelStateLogicTest with matching lastRead-based assertions.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateImplReadReceiptsTest.kt`:
- Around line 237-482: Add a deterministic concurrency test for
ChannelStateImpl.updateCurrentUserRead that launches multiple concurrent calls
under runTest, using distinct message IDs and a shared initial read state. Await
all launched jobs, then assert unreadMessages equals the number of unique
messages to verify no increments are lost.
---
Nitpick comments:
In `@fastlane/Fastfile`:
- Around line 110-116: Add or update Fastfile tests covering the test-filter
branching around targeted_class: verify use_backend: true takes precedence over
a non-empty test_class, non-empty test_class bypasses batch_tests, and both nil
and "" preserve batching. Use the existing test symbols and assert the resulting
filter and batching behavior.
🪄 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 Plus
Run ID: 6e10894e-595a-44ab-bf63-baa9412e0398
📒 Files selected for processing (11)
.github/workflows/e2e-test.ymlfastlane/Fastfilestream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/ChannelEventHandlerImpl.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/legacy/ChannelStateLogic.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateImpl.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateLegacyImpl.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/ChannelEventHandlerImplTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/legacy/ChannelStateLogicTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateImplReadReceiptsTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateLegacyImplTest.ktstream-chat-android-compose-sample/src/androidTestE2eDebug/kotlin/io/getstream/chat/android/compose/robots/UserRobotMessageListAsserts.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- stream-chat-android-compose-sample/src/androidTestE2eDebug/kotlin/io/getstream/chat/android/compose/robots/UserRobotMessageListAsserts.kt
- .github/workflows/e2e-test.yml
The value is interpolated into the instrumentation shell command, so it is now checked to be a class name with an optional method. Dispatching already requires write access, but a typo used to fail somewhere in the shell instead of with a clear message.
andremion
marked this pull request as ready for review
July 31, 2026 15:20
andremion
enabled auto-merge
July 31, 2026 15:21
|
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
1 participant
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
Fix the unread count settling below the real number and never recovering. Reproduced consistently on API 28: 25 incoming messages rendered as "12 unread" on the jump-to-unread button, and the server's correct count could not repair it.
Root cause, from the CI logs rather than inspection: the channel list pushes its own copy of a channel into that channel's state (
QueryChannelsStateLogic.addChannelsState→ChannelLogicImpl.updateDataForChannel), which writeschannel.readunconditionally. That copy's count was captured earlier, so it overwrote the count the channel state had built from message events (25 -> 11, logged). The arbitration that exists to protect the local count could not stop it, because it comparedlastReceivedEventDate, a field the server never sends and that mapping synthesises fromlast_message_at ?: last_read, against a locally advanced clock.Resolves AND-1355
Implementation
mergeCurrentUserReadnow decides on the read position: an incoming count is taken only whenlastReadmoved forward, which is what happens when the channel is read on another device. At the same or an earlier position the incoming count may be stale, so the local count is kept.ChannelStateImpl.replaceRead. It moves the read position backwards on purpose and its count is authoritative, so it must not be arbitrated. This is the one case that cannot be decided from the data alone, since a deliberate mark unread and a stale payload both carry an older position; only the caller knows which it is.updateCurrentUserReadlogs why a message does not count and logs each new value, the channel state's log tag carries an instance number as the legacy implementation already does, a reads update logs the current user's count transition, and the jump-to-unread assertion compares the rendered count so a failure names the actual value.api_leveland atest_classon manual dispatch, so reproducing a single-API-level failure takes 13 minutes and yields three independent samples instead of a 90 minute matrix.Testing
ui-commonandcomposesuites, detekt, apiCheck and spotless pass.OK (4 tests), so 12 clean executions of the unread suite. https://github.com/GetStream/stream-chat-android/actions/runs/30634966638Summary by CodeRabbit
Bug Fixes
Tests