msglist: Suppress duplicate message on fetch/event race

Fixes #929.

When the user navigates to a narrow, the message fetch can race with
the event queue: a newly sent message appears in the fetch response
and then later arrives as a message event. Before now, the message
would get shown twice in the message list; this commit fixes that.

On handling a message event, when the message is already in the list,
skip adding a duplicate.  Do adopt the event's copy of the message,
though, because the message store has done the same, and the list's
messages should remain identical to the store's so that the message
reflects subsequent update events.

Do this adoption before the narrow/haveNewest checks that gate adding
a *new* message.  Those checks answer whether a new message belongs in
the list; but a message we already have must stay in sync with the
store regardless -- including in a search narrow (where containsMessage
is null) or when the list hasn't yet reached the newest message.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: chrisbobbe

lib/model/message_list.dart

@@ -282,6 +282,18 @@ mixin _MessageSequence {
     _processMessage(messages.length - 1);
   }
 
+  /// Replace the message at [index] with [message],
+  /// which must have the same message ID,
+  /// and update [contents] accordingly.
+  ///
+  /// The caller is responsible for updating [items],
+  /// for example by calling [_reprocessAll].
+  void _replaceMessage(int index, Message message) {
+    assert(messages[index].id == message.id);
+    messages[index] = message;
+    contents[index] = parseMessageContent(message);
+  }
+
   /// Removes all messages from the list that satisfy [test].
   ///
   /// Returns true if any messages were removed, false otherwise.
@@ -1154,9 +1166,37 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     }
   }
 
-  /// Add [MessageEvent.message] to this view, if it belongs here.
+  /// Add [MessageEvent.message] to this view if it belongs here,
+  /// or adopt the event's copy if we already have the message.
   void handleMessageEvent(MessageEvent event) {
     final message = event.message;
+
+    final index = _findMessageWithId(message.id);
+    if (index != -1) {
+      // We already have the message, from a fetch whose response was
+      // computed after the message was sent:
+      //   https://github.com/zulip/zulip-flutter/issues/929
+      // Instead of adding a duplicate, adopt the event's copy of the message,
+      // like the message store has (see [MessageStoreImpl.handleMessageEvent]),
+      // so that this view continues to see updates
+      // that are applied to the store's copy.
+      //
+      // This runs regardless of [narrow] and [haveNewest]:
+      // those decide whether a *new* message belongs in this view,
+      // but this message is already here and must stay in sync with the store.
+      _replaceMessage(index, message);
+      // If we sent this message, drop its outbox counterpart.
+      // (When [haveNewest] is false there are no [outboxMessages], so this
+      // is a no-op; see [_syncOutboxMessagesFromStore].)
+      _removeOutboxMessageOfEvent(event);
+      // This is a rare race, so the cost of rebuilding [items] is fine.
+      // [_reprocessAll] also rebuilds the outbox items, so there's no need
+      // to reprocess them separately.
+      _reprocessAll();
+      notifyListeners();
+      return;
+    }
+
     if (narrow.containsMessage(message) != true || !_messageVisible(message)) {
       assert(event.localMessageId == null || outboxMessages.none((message) =>
         message.localMessageId == int.parse(event.localMessageId!, radix: 10)));

test/model/message_list_test.dart

@@ -880,6 +880,116 @@ void main() {
       async.elapse(kLocalEchoDebounceDuration);
       checkNotNotified();
     }));
+
+    group('message already in list from fetch/event race', () {
+      // Regression tests for: https://github.com/zulip/zulip-flutter/issues/929
+
+      test('no duplicate; adopt event copy of message', () async {
+        final stream = eg.stream();
+        await prepare(narrow: ChannelNarrow(stream.streamId), stream: stream);
+        final messages = List.generate(30, (i) => eg.streamMessage(stream: stream));
+        final message = eg.streamMessage(stream: stream);
+        await prepareMessages(foundOldest: true, messages: [...messages, message]);
+
+        check(model).messages.length.equals(31);
+        await store.handleEvent(eg.messageEvent(message));
+        checkNotifiedOnce();
+        check(model).messages.length.equals(31);
+        check(model.messages.last).identicalTo(message);
+      });
+
+      test('event copy of message has different content', () async {
+        // The fetched copy can be newer than the event's copy,
+        // as when the message was edited just after being sent;
+        // the event queue will separately deliver an update event.
+        final stream = eg.stream();
+        await prepare(narrow: ChannelNarrow(stream.streamId), stream: stream);
+        final messages = List.generate(30, (i) => eg.streamMessage(stream: stream));
+        final message = eg.streamMessage(stream: stream, content: '<p>edited</p>');
+        await prepareMessages(foundOldest: true, messages: [...messages, message]);
+
+        final eventMessage = Message.fromJson(
+          message.toJson()..['content'] = '<p>original</p>');
+        await store.handleEvent(eg.messageEvent(eventMessage));
+        checkNotifiedOnce();
+        check(model).messages.length.equals(31);
+        check(model.messages.last).identicalTo(eventMessage);
+        check(model.contents.last).isA<ZulipContent>()
+          .equalsNode(parseContent('<p>original</p>'));
+      });
+
+      test('with corresponding outbox message', () => awaitFakeAsync((async) async {
+        final stream = eg.stream();
+        await prepare(narrow: ChannelNarrow(stream.streamId), stream: stream);
+        await prepareOutboxMessages(count: 1, stream: stream);
+        final localMessageId = store.outboxMessages.keys.single;
+        async.elapse(kLocalEchoDebounceDuration);
+        checkNotNotified();
+
+        final messages = List.generate(30, (i) => eg.streamMessage(stream: stream));
+        final message = eg.streamMessage(stream: stream);
+        await prepareMessages(foundOldest: true, messages: [...messages, message]);
+        check(model)
+          ..messages.length.equals(31)
+          ..outboxMessages.single.localMessageId.equals(localMessageId);
+
+        await store.handleEvent(eg.messageEvent(message,
+          localMessageId: localMessageId));
+        checkNotifiedOnce();
+        check(model)
+          ..messages.length.equals(31)
+          ..outboxMessages.isEmpty();
+      }));
+
+      test('adopt even when narrow.containsMessage is null (e.g. search)', () async {
+        // A narrow like [KeywordSearchNarrow] can't say whether a message
+        // belongs (containsMessage returns null), but a message the view
+        // already has must still adopt the event's copy.
+        final stream = eg.stream();
+        await prepare(narrow: KeywordSearchNarrow('hello'), stream: stream);
+        final messages = List.generate(30, (i) => eg.streamMessage(stream: stream));
+        final message = eg.streamMessage(stream: stream, content: '<p>edited</p>');
+        await prepareMessages(foundOldest: true,
+          messages: [...messages, message]);
+        check(model).haveNewest.isTrue();
+
+        final eventMessage = Message.fromJson(
+          message.toJson()..['content'] = '<p>original</p>');
+        await store.handleEvent(eg.messageEvent(eventMessage));
+        checkNotifiedOnce();
+        check(model).messages.length.equals(31);
+        check(model.messages.last).identicalTo(eventMessage);
+        check(model.contents.last).isA<ZulipContent>()
+          .equalsNode(parseContent('<p>original</p>'));
+      });
+
+      test('adopt even when mid-history (!haveNewest)', () async {
+        // The store clobbers its own copy on every message event, so a view must
+        // adopt any message it already holds -- even when it isn't caught up to
+        // the newest message.
+        final stream = eg.stream();
+        await prepare(narrow: ChannelNarrow(stream.streamId), stream: stream,
+          anchor: NumericAnchor(1000));
+        final message = eg.streamMessage(id: 1029, stream: stream,
+          content: '<p>edited</p>');
+        final messages = [
+          for (int i = 0; i < 29; i++) eg.streamMessage(id: 1000 + i, stream: stream),
+          message,
+        ];
+        await prepareMessages(foundOldest: true, foundNewest: false,
+          messages: messages);
+        check(model).haveNewest.isFalse();
+
+        final eventMessage = Message.fromJson(
+          message.toJson()..['content'] = '<p>original</p>');
+        await store.handleEvent(eg.messageEvent(eventMessage));
+        checkNotifiedOnce();
+        check(model).messages.length.equals(30);
+        check(model.messages.last).identicalTo(eventMessage);
+        check(model.contents.last).isA<ZulipContent>()
+          .equalsNode(parseContent('<p>original</p>'));
+      });
+    });
   });
 
   group('addOutboxMessage', () {