Support local unread counts for channels with read events disabled - #6609
Draft
gpunto wants to merge 21 commits into
Draft
Support local unread counts for channels with read events disabled#6609gpunto wants to merge 21 commits into
gpunto wants to merge 21 commits into
Conversation
…server-data merge
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
SDK Size Comparison 📏
|
Walkthrough
ChangesLocal unread-count support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateImpl.kt (1)
1188-1213: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMark-read outcomes are not in parity between the two channel-state implementations. The same mark-read decision table is implemented twice, and the two copies disagree on the empty-message case and on the
unreadMessagescheck. A channel behaves differently depending on which state implementation is active.
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateImpl.kt#L1188-L1213: add|| currentUserRead.unreadMessages > 0to the final condition, and align the empty-message-list outcome with the legacy implementation.stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateLegacyImpl.kt#L595-L624: keep theunreadMessagescheck, and align the empty-message-list outcome with the chosen behavior. Add a test for each branch in both implementations.🤖 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/state/channel/internal/ChannelStateImpl.kt` around lines 1188 - 1213, Align mark-read decision logic across ChannelStateImpl.markRead and ChannelStateLegacyImpl.markRead: include currentUserRead.unreadMessages > 0 in the final condition, and make both empty-message branches return the same chosen outcome. Add coverage for each decision branch in both implementations. Update ChannelStateImpl.kt lines 1188-1213 and ChannelStateLegacyImpl.kt lines 595-624; tests should cover both implementations and all affected branches.
🧹 Nitpick comments (3)
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/ChannelLogicImpl.kt (1)
250-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLocal-read persistence is implemented twice. Both channel-logic implementations read the current reads, skip empty lists, and launch
upsertChannelReadsonMarkReadResult.HandledLocally. Neither handles a persistence failure, so a failed upsert silently loses the local reset.
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/ChannelLogicImpl.kt#L250-L265: extract the persistence rule into one shared helper and log a failure from the launched coroutine.stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/legacy/ChannelLogicLegacyImpl.kt#L203-L215: call the shared helper instead of repeating the block inline.🤖 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/ChannelLogicImpl.kt` around lines 250 - 265, Local-read persistence is duplicated and silently ignores upsert failures. In ChannelLogicImpl.kt lines 250-265, extract the reads-empty check and coroutine upsert into a shared helper that logs failures from the launched coroutine; in ChannelLogicLegacyImpl.kt lines 203-215, replace the inline persistence block with a call to that helper while preserving the HandledLocally behavior.stream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/feature/channel/list/ChannelsActivity.kt (1)
109-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated channel-list filter/sort construction into a shared helper.
Both activities build the same
ChannelListViewModelFactorywith identical KDoc, the sameisLocalUnreadCountEnabledbranch, and the sameFilters/QuerySortByFieldconfiguration. Any future change to this filter (for example, adding another channel type) must be made in both places, and the two copies can drift.
stream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/feature/channel/list/ChannelsActivity.kt#L109-L140: replace this block with a call to a shared helper function that takeschatClientandsettingsand returns the builtChannelListViewModelFactory.stream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/ui/chats/ChatsActivity.kt#L137-L168: replace this block with the same shared helper call.♻️ Suggested shared helper
fun buildSampleChannelListViewModelFactory( chatClient: ChatClient, settings: CustomSettings, ): ChannelListViewModelFactory { val currentUserId = chatClient.getCurrentUser()?.id ?: "" return if (settings.isLocalUnreadCountEnabled) { ChannelListViewModelFactory( chatClient = chatClient, querySort = QuerySortByField<Channel>().desc("pinned_at").desc("last_updated"), filters = Filters.and( Filters.`in`("type", listOf("messaging", "livestream")), Filters.`in`("members", listOf(currentUserId)), Filters.or(Filters.notExists("draft"), Filters.eq("draft", false)), ), chatEventHandlerFactory = CustomChatEventHandlerFactory(), ) } else { ChannelListViewModelFactory( chatClient = chatClient, predefinedFilterName = "android_sample_filter", filterValues = mapOf( "channel_type" to "messaging", "user_id" to currentUserId, ), chatEventHandlerFactory = CustomChatEventHandlerFactory(), ) } }🤖 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-sample/src/main/java/io/getstream/chat/android/compose/sample/feature/channel/list/ChannelsActivity.kt` around lines 109 - 140, Extract the duplicated channel-list factory construction into a shared buildSampleChannelListViewModelFactory helper accepting ChatClient and CustomSettings, preserving the existing local-unread filter/sort and predefined-filter branches. In stream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/feature/channel/list/ChannelsActivity.kt lines 109-140 and stream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/ui/chats/ChatsActivity.kt lines 137-168, replace each duplicated block with a call to this helper using the activity’s chatClient and settings.stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/event/handler/internal/EventHandlerSequentialTest.kt (1)
424-473: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
NotificationMessageNewEventbranch.These two tests cover
persistLocallyTrackedReadsonly throughrandomNewMessageEvent. The productionwhenblock inpersistLocallyTrackedReadsalso matchesNotificationMessageNewEvent. Add a test usingrandomNotificationMessageNewEventto confirm that branch also triggersrepos.upsertChannelReads.🤖 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/event/handler/internal/EventHandlerSequentialTest.kt` around lines 424 - 473, Add a test alongside the existing local unread tracking tests that invokes persistLocallyTrackedReads through randomNotificationMessageNewEvent, with local unread tracking enabled and locally tracked reads configured, then verify repos.upsertChannelReads receives the channel ID and reads. Keep the setup and assertions consistent with the existing randomNewMessageEvent coverage.
🤖 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/src/main/java/io/getstream/chat/android/client/internal/offline/repository/domain/channel/internal/DatabaseChannelRepository.kt`:
- Around line 71-98: Move all fallback DB/user resolution out of
cacheMutex.withLock: preload the required channel rows and resolved reads before
the lock in preserveLocallyTrackedReads and the upsertChannelReads flow,
including selectChannel. Pass the preloaded in-memory data into the locked merge
so cacheMutex only protects synchronous cache updates; preserve existing merge
behavior and avoid any channelDao.select or getUser calls while the lock is
held.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/listener/internal/ChannelMarkReadListenerState.kt`:
- Around line 45-55: Update the HandledLocally branch in
ChannelMarkReadListenerState to return Result.Success(Unit) after refreshing
active query-channel state. Preserve the existing refreshChannelState call and
ensure callers treat a locally completed mark-read as successful rather than
returning Error.GenericError.
---
Outside diff comments:
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateImpl.kt`:
- Around line 1188-1213: Align mark-read decision logic across
ChannelStateImpl.markRead and ChannelStateLegacyImpl.markRead: include
currentUserRead.unreadMessages > 0 in the final condition, and make both
empty-message branches return the same chosen outcome. Add coverage for each
decision branch in both implementations. Update ChannelStateImpl.kt lines
1188-1213 and ChannelStateLegacyImpl.kt lines 595-624; tests should cover both
implementations and all affected branches.
---
Nitpick comments:
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/ChannelLogicImpl.kt`:
- Around line 250-265: Local-read persistence is duplicated and silently ignores
upsert failures. In ChannelLogicImpl.kt lines 250-265, extract the reads-empty
check and coroutine upsert into a shared helper that logs failures from the
launched coroutine; in ChannelLogicLegacyImpl.kt lines 203-215, replace the
inline persistence block with a call to that helper while preserving the
HandledLocally behavior.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/event/handler/internal/EventHandlerSequentialTest.kt`:
- Around line 424-473: Add a test alongside the existing local unread tracking
tests that invokes persistLocallyTrackedReads through
randomNotificationMessageNewEvent, with local unread tracking enabled and
locally tracked reads configured, then verify repos.upsertChannelReads receives
the channel ID and reads. Keep the setup and assertions consistent with the
existing randomNewMessageEvent coverage.
In
`@stream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/feature/channel/list/ChannelsActivity.kt`:
- Around line 109-140: Extract the duplicated channel-list factory construction
into a shared buildSampleChannelListViewModelFactory helper accepting ChatClient
and CustomSettings, preserving the existing local-unread filter/sort and
predefined-filter branches. In
stream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/feature/channel/list/ChannelsActivity.kt
lines 109-140 and
stream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/ui/chats/ChatsActivity.kt
lines 137-168, replace each duplicated block with a call to this helper using
the activity’s chatClient and settings.
🪄 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: 742011a2-2f22-40a3-b7b8-24b4e4b64927
📒 Files selected for processing (37)
stream-chat-android-client/api/stream-chat-android-client.apistream-chat-android-client/src/main/java/io/getstream/chat/android/client/api/ChatClientConfig.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/api/state/StateRegistry.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/offline/repository/domain/channel/internal/DatabaseChannelRepository.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/offline/repository/factory/internal/DatabaseRepositoryFactory.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/event/handler/internal/EventHandlerSequential.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/factory/StreamStatePluginFactory.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/listener/internal/ChannelMarkReadListenerState.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/ChannelLogic.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/ChannelLogicImpl.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/legacy/ChannelEventHandlerLegacyImpl.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/legacy/ChannelLogicLegacyImpl.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/logic/querychannels/internal/QueryChannelsStateLogic.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/main/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/MarkReadResult.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/persistance/repository/ChannelRepository.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/offline/repository/domain/channel/internal/ChannelRepositoryImplTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/event/TotalUnreadCountTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/event/handler/internal/EventHandlerSequentialTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/event/handler/internal/EventHandlerSequentialUserMessagesDeletedTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/listener/internal/ChannelMarkReadListenerStateTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/ChannelLogicImplTest.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/logic/querychannels/internal/QueryChannelsStateLogicTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateImplLocalUnreadCountTest.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/ChannelStateLegacyImplLocalUnreadCountTest.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/main/java/io/getstream/chat/android/compose/sample/ChatHelper.ktstream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/data/CustomSettings.ktstream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/feature/channel/list/ChannelsActivity.ktstream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/ui/chats/ChatsActivity.ktstream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/ui/login/CustomLoginActivity.ktstream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/ui/login/UserLoginActivity.ktstream-chat-android-compose-sample/src/main/res/values/strings.xml
Comment on lines
+71
to
+98
| val channelsToInsert = cacheMutex.withLock { | ||
| val updatedChannels = channels | ||
| .map { channelCache[it.cid]?.let { cachedChannel -> it.combine(cachedChannel) } ?: it } | ||
| .map { if (it.config.readEventsEnabled) it else it.preserveLocallyTrackedReads() } | ||
| updatedChannels | ||
| .filter { channelCache[it.cid] != it } | ||
| .also { cacheChannel(updatedChannels) } | ||
| } | ||
| scope.launchWithMutex(dbMutex) { | ||
| logger.v { | ||
| "[insertChannels] inserting ${channelToInsert.size} entities on DB, " + | ||
| "updated ${updatedChannels.size} on cache" | ||
| } | ||
| channelToInsert | ||
| logger.v { "[insertChannels] inserting ${channelsToInsert.size} entities on DB" } | ||
| channelsToInsert | ||
| .takeUnless { it.isEmpty() } | ||
| // Re-read the cache at write time: DAO writes are not guaranteed to run in launch | ||
| // order, so the last write must persist the latest merged state, not its snapshot | ||
| ?.map { channel -> (channelCache[channel.cid] ?: channel).toEntity() } | ||
| ?.let { channelDao.insertMany(it) } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Keeps the stored current user read of read-events-disabled channels: it is tracked on-device and | ||
| * must never be overwritten by server data (a recency check is not enough - server reads carry | ||
| * lastReceivedEventDate = last_message_at, tying it). Other users' reads are merged by recency. | ||
| */ | ||
| private suspend fun Channel.preserveLocallyTrackedReads(): Channel { | ||
| val storedReads = channelCache[cid]?.read | ||
| ?: channelDao.select(cid)?.reads?.values?.map { it.toModel(getUser) } | ||
| ?: return this |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Avoid DB reads while holding cacheMutex.
preserveLocallyTrackedReads() (line 97) and selectChannel() inside upsertChannelReads (line 126) can call channelDao.select, and preserveLocallyTrackedReads() additionally resolves users via getUser for each stored read. Both run inside cacheMutex.withLock.
cacheMutex serializes every cache mutation, including insertChannels and upsertChannelReads. Holding it across suspending DB and user-resolution calls blocks unrelated channel/read updates for the duration of that I/O. In insertChannels, this happens sequentially for every cache-miss channel in the batch, which is most costly during cold start when the cache is empty and many channels load at once — exactly the scenario this PR targets.
Fetch the fallback data (DB rows, users) before acquiring cacheMutex, then perform the merge with only in-memory data under the lock.
Also applies to: 119-137
🤖 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/offline/repository/domain/channel/internal/DatabaseChannelRepository.kt`
around lines 71 - 98, Move all fallback DB/user resolution out of
cacheMutex.withLock: preload the required channel rows and resolved reads before
the lock in preserveLocallyTrackedReads and the upsertChannelReads flow,
including selectChannel. Pass the preloaded in-memory data into the locked merge
so cacheMutex only protects synchronous cache updates; preserve existing merge
behavior and avoid any channelDao.select or getUser calls while the lock is
held.
gpunto
force-pushed
the
feat/local-unread-count
branch
from
July 31, 2026 13:26
a5e0cc0 to
e77e987
Compare
|
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
Closes AND-1358
Provide an unread mechanism for channels where server-side read events are disabled (e.g. livestreams). Android port of stream-chat-swift#4102.
Adds an opt-in
ChatClientConfig.isLocalUnreadCountEnabledflag (defaultfalse). When on, channels withreadEventsEnabled = falseget a per-channel unread count tracked entirely on-device. The user-level total unread count is unaffected.Implementation
markReadon a locally tracked channel resets the count in state without a network request (markRead()returns a newMarkReadResultdescribing whether a remote request is still needed) and refreshes the channel-list rows, since no server read event will follow.lastReceivedEventDate = last_message_at, which ties the local value.ChannelRepository.upsertChannelReads(default no-op) that bypasses the server-data merge, so the count survives an app restart.Demo app
The compose sample gets a "Local unread count" toggle (advanced login options). When enabled it also includes livestream channels in the channel list so the feature is testable.
Testing
spotlessCheck,detekt,apiCheck(only additive: the newChatClientConfig/StateRegistrytrailing flag; source-compatible), and the full clienttestDebugUnitTestsuite pass./readrequest, survives a sync and an app restart, and does not double-count offline messages on relaunch.Summary by CodeRabbit
New Features
Bug Fixes