fix(llc): Don't upsert out-of-window messages on message.updated/message.deleted events#2794
Open
VelikovPetar wants to merge 3 commits into
Open
Conversation
…ted events Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
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 (3)
✅ Files skipped from review due to trivial changes (1)
📝 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 updateMessage
participant Merge as _mergeMessagesIntoExisting
participant State as Channel state
WS->>Channel: message.updated or soft message.deleted
Channel->>Update: updateMessage(message, upsert: false)
Update->>Merge: merge with upsert disabled
alt message exists in loaded window
Merge->>State: update existing message
else message is outside loaded window
Merge->>State: retain existing messages
end
Channel->>State: update pinned and live-location state when 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 71.08% 71.08% -0.01%
==========================================
Files 429 429
Lines 26891 26898 +7
==========================================
+ Hits 19116 19120 +4
- Misses 7775 7778 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
xsahil03x
reviewed
Jul 6, 2026
| if (message == null) return; | ||
|
|
||
| return updateMessage(message); | ||
| return _updateMessages([message], upsert: false); |
Member
There was a problem hiding this comment.
should we add the upsert flag to updateMessage too and use it?
xsahil03x
reviewed
Jul 6, 2026
| return deleteMessage(message, hardDelete: hardDelete); | ||
| if (hardDelete) return deleteMessage(message, hardDelete: true); | ||
| // Soft delete, update the message only if loaded (upsert: false) | ||
| return _updateMessages([message], upsert: false); |
Member
There was a problem hiding this comment.
Let's use updateMessage here if we decide to do https://github.com/GetStream/stream-chat-flutter/pull/2794/changes#r3526391971
…rting_messages # Conflicts: # packages/stream_chat/CHANGELOG.md
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
2 participants
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