fix(llc): Don't upsert out-of-window messages on message.updated/message.deleted events#2794
Open
VelikovPetar wants to merge 1 commit into
Open
Conversation
…ted events Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds an ChangesUpsert guard for message reconciliation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WS as Websocket Event
participant Channel as ChannelClientState
participant Update as _updateMessages
participant Merge as _mergeMessagesIntoExisting
participant State as messages/threads/pinnedMessages
WS->>Channel: messageUpdated / soft messageDeleted
Channel->>Update: _updateMessages([message], upsert: false)
Update->>Merge: _mergeMessagesIntoExisting(upsert: false)
Merge->>Merge: check if message id exists in existing list
alt id exists in loaded window
Merge->>State: update message in place
else id not in loaded window
Merge->>State: skip insertion, return existing list
end
Channel->>State: still update pinnedMessages/activeLiveLocations if applicable
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 |
VelikovPetar
changed the title
message.updated/message.deleted events
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stream_chat/lib/src/client/channel.dart (1)
3963-3975: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSkip creating empty thread entries when
upsert: falsedrops an unloaded reply.At Lines 3973-3974, if
updatedThreads[thread]is absent and_mergeMessagesIntoExisting(..., upsert: false)returns an empty list, this still writesthread: []into_threads. That mutates/persists a thread that was never loaded and makesthreads.containsKey(parentId)report a phantom thread.Proposed fix
final updatedThreads = {...threads}; for (final MapEntry(key: thread, :value) in messagesByThread.entries) { - final threadMessages = updatedThreads[thread] ?? <Message>[]; + final existingThreadMessages = updatedThreads[thread]; + final threadMessages = existingThreadMessages ?? <Message>[]; final updatedThreadMessages = _mergeMessagesIntoExisting( existing: threadMessages, toMerge: value, update: update, upsert: upsert, ); // Update the thread with the modified message list. + if (existingThreadMessages == null && updatedThreadMessages.isEmpty) { + continue; + } updatedThreads[thread] = updatedThreadMessages.toList(); }🤖 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 3963 - 3975, Skip creating empty thread entries in the thread merge logic: in the channel message-thread update path, if `updatedThreads[thread]` is missing and `_mergeMessagesIntoExisting` returns no messages with `upsert: false`, do not write that thread back into `_threads`. Update the loop in `channel.dart` so `updatedThreads[thread] = ...` only happens when the merged list is non-empty or the thread already exists, preserving the behavior of `threads.containsKey(parentId)` and avoiding phantom unloaded threads.
🤖 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.
Outside diff comments:
In `@packages/stream_chat/lib/src/client/channel.dart`:
- Around line 3963-3975: Skip creating empty thread entries in the thread merge
logic: in the channel message-thread update path, if `updatedThreads[thread]` is
missing and `_mergeMessagesIntoExisting` returns no messages with `upsert:
false`, do not write that thread back into `_threads`. Update the loop in
`channel.dart` so `updatedThreads[thread] = ...` only happens when the merged
list is non-empty or the thread already exists, preserving the behavior of
`threads.containsKey(parentId)` and avoiding phantom unloaded threads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dad998b6-e7d7-448d-840f-95abb02e919a
📒 Files selected for processing (3)
packages/stream_chat/CHANGELOG.mdpackages/stream_chat/lib/src/client/channel.dartpackages/stream_chat/test/src/client/channel_test.dart
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2794 +/- ##
==========================================
- Coverage 70.04% 70.03% -0.01%
==========================================
Files 426 426
Lines 25680 25685 +5
==========================================
+ Hits 17987 17989 +2
- Misses 7693 7696 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Submit a pull request
Linear: FLU-560
Github Issue: #
CLA
Description of the pull request
message.updatedand softmessage.deletedevents were being unconditionally upserted intoChannelState.messages(and thread reply lists) viaupdateMessage/deleteMessage. When the target message wasn't in the currently loaded window — e.g. it lived on an older page the client hadn't paged in yet — this created a phantom entry in the sorted list, leaving a visible gap between the loaded slice and history.Fix
_updateMessages(and its_updateThreadMessages/_updateChannelMessages/_mergeMessageshelpers) now take anupsertflag.message.updatedevent handler passesupsert: false.message.deletedevent handler passesupsert: falsefor soft deletes (hard deletes still route throughdeleteMessageunchanged).upsert: falseand an id isn't already in the loaded list, the message is skipped for the channel/thread lists. Side effects such aspinnedMessagesmaintenance andactiveLiveLocationsexpiration still fire (they don't depend on the message being in the window).The guard is intentionally id-based and independent of
isUpToDate— even at the latest page we may have paginated past older history and receive an event for a message no longer in memory.Test plan
channel_test.dartgroups undermessageUpdatedandmessageDeleted:should NOT insert unknown message into messages list(out-of-window edit)should update message in place when it IS in the loaded windowshould still add to pinnedMessages when pinned:true even if not in loaded windowshould NOT insert unknown reply into threads[parentId]should still expire activeLiveLocations for out-of-window messageScreenshots / Videos
No UI changes.
Summary by CodeRabbit