feat(llc): Implement local unread counts - #2804
Conversation
renefloor
changed the title
renefloor
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdds an opt-in local unread-count mode for channels without read events, including local read mutations, message-based count updates, remote-state preservation, tests, mocks, and changelog documentation. ChangesLocal unread count tracking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MessageEvent
participant Channel
participant MessageRules
participant ChannelClientState
MessageEvent->>Channel: Receive message.new or message.deleted
Channel->>MessageRules: Check unread eligibility
MessageRules-->>Channel: Return eligibility
Channel->>ChannelClientState: Update local unread count
Channel->>ChannelClientState: Apply local read boundary
ChannelClientState-->>Channel: Return recomputed unread state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
82e91e4 to
a495169
Compare
![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
🧹 Nitpick comments (1)
packages/stream_chat/test/src/client/channel_test.dart (1)
9810-10089: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive-path test for
Channel.markUnreadlocal recompute.The new group thoroughly covers
markReadandmarkUnreadByTimestamplocal recompute, plusmarkUnread's throw-when-unknown-message case, but there's no test exercising a successfulmarkUnread(messageId)call and asserting the resultingunreadCount— the anchor/microsecond-offset arithmetic inChannel.markUnread(subtracting 1 microsecond from the anchor'screatedAt) is currently only covered indirectly.🤖 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 `@packages/stream_chat/test/src/client/channel_test.dart` around lines 9810 - 10089, The local unread-count tests lack a successful-path case for Channel.markUnread. Add a test in the “Local unread count” group that creates locally known messages with distinct timestamps, calls markUnread with a valid message ID, asserts unreadCount reflects only messages after the anchor (including the one-microsecond offset behavior), and verifies no network request via client.markChannelUnread.
🤖 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 `@packages/stream_chat/lib/src/client/channel.dart`:
- Around line 3615-3637: The markReadLocally method does not reconcile pending
delivery receipts after updating read state. Add
_client.channelDeliveryReporter.reconcileDelivery([_channel]) to markReadLocally
after updateRead, matching the delivery reconciliation behavior used by related
mark-read flows.
---
Nitpick comments:
In `@packages/stream_chat/test/src/client/channel_test.dart`:
- Around line 9810-10089: The local unread-count tests lack a successful-path
case for Channel.markUnread. Add a test in the “Local unread count” group that
creates locally known messages with distinct timestamps, calls markUnread with a
valid message ID, asserts unreadCount reflects only messages after the anchor
(including the one-microsecond offset behavior), and verifies no network request
via client.markChannelUnread.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 030f7148-3f52-4392-aa77-629db23c3680
📒 Files selected for processing (8)
docs/docs_screenshots/test/src/mocks.dartpackages/stream_chat/CHANGELOG.mdpackages/stream_chat/lib/src/client/channel.dartpackages/stream_chat/lib/src/client/client.dartpackages/stream_chat/lib/src/core/util/message_rules.dartpackages/stream_chat/test/src/client/channel_test.dartpackages/stream_chat/test/src/core/util/message_rules_test.dartpackages/stream_chat/test/src/mocks.dart
| /// Marks the channel as read locally, without making a network request. | ||
| /// | ||
| /// Used for channels that track unread counts locally (see | ||
| /// [Channel.usesLocalUnreadCount]), since the server rejects the mark-read | ||
| /// endpoint for channels that have read events disabled. | ||
| void markReadLocally({String? messageId}) { | ||
| final currentUser = _client.state.currentUser; | ||
| if (currentUser == null) return; | ||
|
|
||
| final now = DateTime.now(); | ||
| final lastReadMessageId = messageId ?? messages.lastOrNull?.id; | ||
|
|
||
| final existingUserRead = currentUserRead; | ||
| updateRead([ | ||
| Read( | ||
| user: currentUser, | ||
| lastRead: now, | ||
| lastReadMessageId: lastReadMessageId, | ||
| lastDeliveredAt: existingUserRead?.lastDeliveredAt, | ||
| lastDeliveredMessageId: existingUserRead?.lastDeliveredMessageId, | ||
| ), | ||
| ]); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant symbols and surrounding code
rg -n "_listenReadEvents|reconcileDelivery|markReadLocally|markRead\\(" packages/stream_chat/lib/src/client/channel.dart packages/stream_chat/lib/src -S
# Show the relevant slice around markReadLocally
sed -n '3570,3665p' packages/stream_chat/lib/src/client/channel.dart
# Show the read event listener slice
sed -n '3000,3095p' packages/stream_chat/lib/src/client/channel.dartRepository: GetStream/stream-chat-flutter
Length of output: 8499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "canUseDeliveryReceipts|canUseReadReceipts|delivery receipts|read receipts" packages/stream_chat/lib -SRepository: GetStream/stream-chat-flutter
Length of output: 3601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "void updateRead|updateRead\\(|currentUserRead|channelDeliveryReporter|reconcileDelivery" packages/stream_chat/lib/src -SRepository: GetStream/stream-chat-flutter
Length of output: 4513
🌐 Web query:
stream_chat ChannelDeliveryReporter reconcileDelivery markReadLocally read receipts delivery receipts
💡 Result:
In the Stream Chat SDK (specifically the Dart/Flutter implementation), ChannelDeliveryReporter is a utility class responsible for managing and throttling delivery receipt reporting to the server [1][2][3]. Core Components and Functionality: ChannelDeliveryReporter: This class collects channels requiring delivery acknowledgments and reports them to the server efficiently [1][2]. It includes a throttle mechanism to prevent excessive API calls [2][3]. - reconcileDelivery(Iterable channels): Reconciles the delivery reporting state for a set of channels with their current internal state [1][2]. - submitForDelivery(Iterable channels): Submits channels to the reporter queue for delivery receipt processing [1][2]. - cancelDelivery(Iterable channels): Cancels pending delivery reports for specific channels [1][2]. Delivery Receipts vs. Read Receipts: - Delivery Receipts track when a message has been successfully delivered to a recipient's device [4][5]. This is confirmed via the message.delivered WebSocket event [4]. It is disabled by default and may require specific configuration or support contact [4][5]. - Read Receipts track when a user has actively viewed a message or marked a channel as read [4][5]. This is confirmed via the message.read WebSocket event [4]. Implementation Notes: - The SDK handles delivery confirmation automatically, including throttling and duplicate prevention [4][5]. - Developers can access the full read and delivery state (e.g., lastDeliveredAt, lastDeliveredMessageId) via the channel.state.read object, which provides a list of members and their respective read/delivery statuses [6]. - While markReadLocally is often used in local state management to update UI immediately before the server confirms the action, the SDK provides explicit methods like markChannelRead, markAllRead, and markChannelsDelivered to sync state with the Stream server [7]. The MarkChannelsDelivered typedef is the callback function used to transmit these delivery acknowledgments to the backend [8].
Citations:
- 1: https://pub.dev/documentation/stream_chat/latest/stream_chat/ChannelDeliveryReporter-class.html
- 2: https://pub.dev/documentation/stream_chat_flutter_core/latest/stream_chat_flutter_core/ChannelDeliveryReporter-class.html
- 3: https://pub.dev/documentation/stream_chat_flutter/latest/stream_chat_flutter/ChannelDeliveryReporter-class.html
- 4: https://getstream.io/chat/docs/flutter-dart/message-delivery-and-read-status/
- 5: https://getstream.io/chat/docs/android/message-delivery-and-read-status/
- 6: https://getstream.io/chat/docs/sdk/flutter/stream-chat-flutter/message-list/read-indicators/
- 7: https://pub.dev/documentation/stream_chat_flutter_core/latest/stream_chat_flutter_core/StreamChatClient-class.html
- 8: https://pub.dev/documentation/stream_chat/latest/stream_chat/MarkChannelsDelivered.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1848,1875p' packages/stream_chat/lib/src/client/channel.dart
printf '\n----\n'
sed -n '3288,3335p' packages/stream_chat/lib/src/client/channel.dart
printf '\n----\n'
sed -n '3398,3492p' packages/stream_chat/lib/src/client/channel.dart
printf '\n----\n'
sed -n '4698,4768p' packages/stream_chat/lib/src/client/channel.dart
printf '\n----\n'
sed -n '96,145p' packages/stream_chat/lib/src/client/channel_delivery_reporter.dartRepository: GetStream/stream-chat-flutter
Length of output: 10239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "markReadLocally\\(" packages/stream_chat/lib -SRepository: GetStream/stream-chat-flutter
Length of output: 373
markReadLocally should reconcile delivery receipts too.
It only updates read state; for channels with delivery receipts enabled, skipping _client.channelDeliveryReporter.reconcileDelivery([_channel]) leaves pending delivery state stale after a local mark-read.
🤖 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 `@packages/stream_chat/lib/src/client/channel.dart` around lines 3615 - 3637,
The markReadLocally method does not reconcile pending delivery receipts after
updating read state. Add
_client.channelDeliveryReporter.reconcileDelivery([_channel]) to markReadLocally
after updateRead, matching the delivery reconciliation behavior used by related
mark-read flows.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2804 +/- ##
==========================================
+ Coverage 71.00% 71.04% +0.04%
==========================================
Files 429 429
Lines 26891 26938 +47
==========================================
+ Hits 19093 19138 +45
- Misses 7798 7800 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // keep their slot. Only applies to channels that track unread counts | ||
| // locally (see [Channel.usesLocalUnreadCount]) — server-driven | ||
| // channels get corrected counts from server read events instead. | ||
| if (hardDelete && _channel.usesLocalUnreadCount && MessageRules.canCountAsUnread(message, _channel)) { |
There was a problem hiding this comment.
Just curious: Don't soft-deletes also decrement the unread count?
There was a problem hiding this comment.
No — intentionally, and this matches iOS exactly.
Swift short-circuits soft deletes with a dedicated skip reason before any of the other eligibility checks run:
// ChannelReadUpdaterMiddleware.swift:216-247
if let skipReason = !event.hardDelete
? .messageIsSoftDeleted
: unreadCountUpdateSkippingReason(...) { return log.debug(...) }
channelRead.unreadMessageCount = max(0, channelRead.unreadMessageCount - 1)
The reasoning: a soft-deleted message keeps its slot in the message list (it renders as "This message was deleted"), so there is still an item the user hasn't seen — the count should keep pointing at it. A hard delete removes the row entirely, so the count has to shrink or it points at nothing. There's a test for both directions (does not decrement unreadCount when a message is soft-deleted / decrements unreadCount when a counted message is hard-deleted).
One divergence worth naming, though it's out of scope for this PR: Swift's decrement isn't gated on local unread counting at all — it runs for every channel, mirroring the backend. Flutter gates it on usesLocalUnreadCount, so a server-driven channel keeps a stale unread count after a hard delete until the next query/read event. That's a pre-existing gap in Flutter (nothing decremented before this PR either).
Shall I remove the _channel.usesLocalUnreadCount check?
| _checkInitialized(); | ||
|
|
||
| if (usesLocalUnreadCount) { | ||
| state!.markUnreadLocally(lastRead: timestamp); |
There was a problem hiding this comment.
In the markUnread(String messageId) method, we call markUnreadLocally with a -1ms of the message timestamp -> but here we call the markUnreadLocally with the exact timestamp. I am curious whether we might be missing some edge-case here, because this method will usually be called with the exact createdAt timestamp of a specific message. So if we call:
final message = targetMessage;
channel.markUnread(message.id);
channel.markUnreadByTimestamp(message.createdAt);Would result in calling markUnreadLocally with a different timestamp. Probably not a big deal, but I wanted to flag it nonetheless!
| updateRead([ | ||
| Read( | ||
| user: currentUser, | ||
| lastRead: now, |
There was a problem hiding this comment.
Question: Does the lastRead mark the moment the channel was read, or the timestamp of the last read message? (I think we should align this with how the server populates this field)
There was a problem hiding this comment.
The moment of reading. DateTime.now() is correct and aligned with the server.
The server treats the two as separate fields, and we already follow that for remote events (channel.dart:3393-3421):
- message.read → lastRead: event.createdAt (when the read happened)
- notification.mark_read → lastRead: event.createdAt, plus last_read_message_id carried independently
Swift is the same: resetChannelRead(lastReadAt: event.createdAt), and its local variant uses markChannelAsRead(cid:userId:at: .init()) — i.e. Date(), now (ChannelRepository.swift:74). So lastRead = timestamp, lastReadMessageId = which message. ✅
However, this question does surface a real gap in markReadLocally. When called as markRead(messageId: X) with an older X, we set lastRead = now and reset the count to 0 (Read's unreadMessages defaults to 0). The server instead recomputes unread as the number of messages after X — so messages between X and now stay unread. Locally we clear the whole count and silently ignore the messageId for counting purposes.
Swift sidesteps this by not accepting a message id in markReadLocally at all. Two options:
- When messageId is provided, use the anchor's createdAt as lastRead and recompute via the same messages.where(MessageRules.canCountAsUnread).length pass that markUnreadLocally already uses — symmetric with the unread path and matches the server.
- Keep it simple and document that messageId only affects lastReadMessageId locally, never the count.
|
|
||
| final localRead = currentUserRead; | ||
| final remoteReads = remoteState.read; | ||
| if (localRead == null || remoteReads == null) return remoteState; |
There was a problem hiding this comment.
Question: Will we always get an initial Read object in the response for a channel which has read/delivery receipts disabled? I am worried that if we do not get an initial object, we won't have a baseline to update on message.new/message.deleted events.
There was a problem hiding this comment.
No, we might not — but we don't need one. Three layers cover it, and iOS handles it the same defensive way.
- MessageRules.canCountAsUnread returns true when there's no read object (message_rules.dart:120-121), so the first message still counts rather than being dropped.
- The unreadCount setter lazily synthesizes the baseline when none exists (channel.dart:3597-3610), using lastRead: lastMessageAt ?? DateTime.now(). Because addNewMessage calls updateMessage (which advances lastMessageAt) before incrementing, the synthesized boundary lands on the message that was just counted — count is 1, no double-count, no loss.
- _preserveLocalUnreadState no-ops when localRead == null, so the remote payload wins on first sync. And if the remote payload has no read array at all, updateChannelState merges reads by user id (channel.dart:3735-3738), so an existing local read survives regardless.
iOS is defensive in the same places: the middleware uses loadOrCreateChannelRead, markChannelAsRead creates a read for members when one is missing, saveChannelRead only preserves local values when !dto.isInserted (ChannelReadDTO.swift:105-114 — the direct analogue of _preserveLocalUnreadState, including the "first insert takes the payload" rule), and ChatChannelVC returns true // no read state, always mark as read.
One genuine (minor) edge: if a message arrives while isUpToDate == false, updateMessage is skipped so lastMessageAt isn't advanced, and the synthesized lastRead sits before the counted message. The count is still right (1); only the boundary timestamp is slightly conservative, which could let a later recompute re-count that message. If we want to be strict, messages.lastOrNull?.createdAt ?? lastMessageAt ?? DateTime.now() closes it.
2 participants
Submit a pull request
Linear: FLU-587
CLA
Description of the pull request
This adds an option for local only unread counts, especially interesting for things like livestreams with many updates.
Summary by CodeRabbit
isLocalUnreadCountEnabled).markRead/markUnread/markUnreadByTimestampupdate local unread state without network requests.