msglist: Suppress duplicate message on fetch/event race

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.
@@ -387,6 +399,21 @@ mixin _MessageSequence {
     return true;
   }
 
+  /// Remove from [outboxMessages] the outbox message the event's message
+  /// was sent as, if any.
+  ///
+  /// The caller is responsible for updating the corresponding items,
+  /// for example by calling [_reprocessOutboxMessages].
+  ///
+  /// [outboxMessages] is expected to be short, so removing the corresponding
+  /// outbox message in linear time is efficient.
+  void _removeOutboxMessageOfEvent(MessageEvent event) {
+    if (event.localMessageId == null) return;
+    final localMessageId = int.parse(event.localMessageId!, radix: 10);
+    outboxMessages.removeWhere(
+      (message) => message.localMessageId == localMessageId);
+  }
+
   /// Remove all outbox messages that satisfy [test] from [outboxMessages].
   ///
   /// Returns true if any outbox messages were removed, false otherwise.
@@ -1139,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)));
@@ -1165,13 +1220,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     _removeOutboxMessageItems();
     // TODO insert in middle of [messages] instead, when appropriate
     _addMessage(message);
-    if (event.localMessageId != null) {
-      final localMessageId = int.parse(event.localMessageId!, radix: 10);
-      // [outboxMessages] is expected to be short, so removing the corresponding
-      // outbox message and reprocessing them all in linear time is efficient.
-      outboxMessages.removeWhere(
-        (message) => message.localMessageId == localMessageId);
-    }
+    _removeOutboxMessageOfEvent(event);
     _reprocessOutboxMessages();
     notifyListeners();
   }

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', () {