fix(llc): Don't upsert out-of-window messages on message.updated/message.deleted events#2794
Conversation
…ted events Co-Authored-By: Claude <noreply@anthropic.com>
|
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 (1)
📝 WalkthroughWalkthroughAdds an ChangesUpsert guard for message reconciliation
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant WS as Websocket event
participant Channel as ChannelClientState
participant Merge as _mergeMessagesIntoExisting
participant State as Channel state
WS->>Channel: message.updated or soft message.deleted
Channel->>Merge: reconcile with upsert: false
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
🚥 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}
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2794 +/- ##
=======================================
Coverage 71.27% 71.28%
=======================================
Files 430 430
Lines 26925 26930 +5
=======================================
+ Hits 19191 19196 +5
Misses 7734 7734 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| if (message == null) return; | ||
|
|
||
| return updateMessage(message); | ||
| return _updateMessages([message], upsert: false); |
There was a problem hiding this comment.
should we add the upsert flag to updateMessage too and use it?
| 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); |
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
| if (hardDelete) return deleteMessage(message, hardDelete: true); | ||
| // Soft delete, update the message only if loaded (upsert: false) | ||
| return updateMessage(message, upsert: false); |
There was a problem hiding this comment.
Should we do the check here or instead modify the _deleteMessages and pass upsert: false always?
void _deleteMessages(
Iterable<Message> messages, {
bool hardDelete = false,
}) {
if (messages.isEmpty) return;
if (hardDelete) return _removeMessages(messages);
return _updateMessages(messages, upsert: false);
}There was a problem hiding this comment.
I think this looks cleaner, I will update it like this! Thanks for the suggestion!
| _updatePinnedMessages(messages, update: update); | ||
| _updateActiveLiveLocations(messages); |
There was a problem hiding this comment.
Lets also add the flag to these two
There was a problem hiding this comment.
I believe that the _updatePinnedMessages method is fine if it always does an upsertion. For example, if a message gets pinned, we would get a message.updated event for that message with the pinned indicator flipped. In such case, we should anyway add the message to the pinned messages state. Otherwise, we need to explicitly check if the message.updated event was received for the purpose of pinning a message.
Similarly for the _updateActiveLiveLocation -> I think that we are also fine if we always upsert them? In this case it would mean that a message outside of the loaded window was updated with a live location, but that is (I believe) already a valid scenario? We should get all live locations when loading the channel, unrelated to which page we load?
| // Don't create a phantom entry for a thread that wasn't loaded: with | ||
| // `upsert: false` an out-of-window reply is dropped, leaving the merged | ||
| // list empty. Writing it back would make `threads.containsKey(parentId)` | ||
| // report a thread that was never paged in. | ||
| if (existingThreadMessages == null && updatedThreadMessages.isEmpty) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
We should probably early return if we are not upserting and there's no thread to update.
for (final MapEntry(key: thread, :value) in messagesByThread.entries) {
final threadMessages = updatedThreads[thread] ?? <Message>[];
// If the thread is not loaded and upsert is false, skip updating it.
if (threadMessages.isEmpty && !upsert) continue;
final updatedThreadMessages = _mergeMessagesIntoExisting(
existing: threadMessages,
toMerge: value,
update: update,
upsert: upsert,
);
// Update the thread with the modified message list.
updatedThreads[thread] = updatedThreadMessages.toList();
}
VelikovPetar
deleted the
bug/FLU-560_fix_message_updated_event_inserting_messages
branch
2 participants
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
Bug Fixes
message.updatedand softmessage.deletedfrom creating/insertions when the affected message is outside the currently loaded window, including correct handling for thread replies, pinned messages, and live-location expiry.New Features
upsertcontrol to message reconciliation so unknown out-of-window messages can be ignored (upsertdefaults totrue).Tests