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 Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds opt-in local unread-count tracking 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
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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
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?
There was a problem hiding this comment.
Aham interesting, I wasn't aware of this.
Shall I remove the _channel.usesLocalUnreadCount check?
I would leave this up to you, maybe perhaps we can do it separately in a different PR, to have a cleaner changelog.
| _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!
There was a problem hiding this comment.
Good catch on there being something off here — the differing timestamps turn out to be correct, but chasing it down surfaced a real bug next to it. Fixed in 07a5673.
Why the timestamps should differ. markUnreadLocally(lastRead: x) has one contract: everything strictly after x is unread — that's forced by canCountAsUnread, which treats !message.createdAt.isAfter(lastRead) as read. The two public methods feed it inputs of different inclusivity:
| Entry point | Input semantics | Conversion |
|---|---|---|
| markUnread(messageId) | inclusive — "from this message onwards" | anchor is in the unread set → boundary must sit just before it → -1µs |
| markUnreadByTimestamp(t) | exclusive — "all messages after t" | already a boundary → pass through |
This mirrors the two REST endpoints ({message_id} vs {message_timestamp}), and iOS resolves the anchor with the same split: .messageId → MessageDTO.loadMessage(before:) (strictly before), .messageTimestamp → loadMessage(beforeOrEqual:) (at-or-equal) in MessageRepository.swift:351-368. If the -1µs moved into markUnreadLocally, markUnreadByTimestamp(t) would start marking the message at t as unread, contradicting its own doc and the endpoint.
So your example producing two different boundaries is correct — the calls are asking for different things. markUnread(m.id) ≡ markUnreadByTimestamp(m.createdAt - 1µs), not markUnreadByTimestamp(m.createdAt). I've documented that on both methods and added a test that pins the difference.
I also checked the epsilon is web-safe, since Duration(microseconds: 1) against a millisecond-backed DateTime would silently be a no-op: modern dart2js/dart2wasm keeps a separate _microsecond field and subtract, isAfter, and DateTime.parse all honour it (js_shared/lib/date_time_patch.dart:132-145, 205-213). With sdk: ^3.11.0 we're clear.
The actual bug you flushed out. Neither local path was passing lastReadMessageId, so it stayed null — while markReadLocally sets it and the server path sets it from notification.mark_unread. StreamChannel.getFirstUnreadMessage falls back to getOldestRegularMessage() when it's null (stream_channel.dart:802-806), which returns null unless _topPaginationEnded. Net effect on a livestream channel: after a local mark-unread the unread divider and jump-to-unread button anchored to the oldest loaded message, or disappeared entirely if history wasn't fully paginated — which is the normal case. Count was right, anchor was wrong. Both call sites now resolve and pass the anchor, with tests.
| 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 boundaries without network requests.