message: Mutate messages for reaction events here, instead of in msglists

This looks potentially NFC, but in fact it fixes part of 455:
it fixes the issue as far as reactions are concerned.  The change
is NFC only if the message appears in just one message list.

In addition to the symptom explained at zulip#455, this fixes another,
which exists now that we've added the central message store with
`reconcileMessages`. It's arguably a bit of an inverse of zulip#455, but
fundamentally the same issue: if there are currently *zero* message
lists that contain the message, then we would drop the update. Then
on later fetching the same message, we'd use the version we had (for
the good reason explained in reconcileMessages) but in fact it'd be
stale.

This zero-message-lists symptom also reproduces with other kinds of
updates that we handle: editing a message, and adding/removing
flags. So, in the regression test for this symptom, I've tried to
make some reusable test code that we can use when we fix those other
ways of updating messages.

Co-authored-by: Chris Bobbe <[email protected]>
Fixes-partly: zulip#455

Author: gnprice

lib/model/message.dart

@@ -109,8 +109,30 @@ class MessageStoreImpl with MessageStore {
   }
 
   void handleReactionEvent(ReactionEvent event) {
+    final message = messages[event.messageId];
+    if (message == null) return;
+
+    switch (event.op) {
+      case ReactionOp.add:
+        (message.reactions ??= Reactions([])).add(Reaction(
+          emojiName: event.emojiName,
+          emojiCode: event.emojiCode,
+          reactionType: event.reactionType,
+          userId: event.userId,
+        ));
+      case ReactionOp.remove:
+        if (message.reactions == null) { // TODO(log)
+          return;
+        }
+        message.reactions!.remove(
+          reactionType: event.reactionType,
+          emojiCode: event.emojiCode,
+          userId: event.userId,
+        );
+    }
+
     for (final view in _messageListViews) {
-      view.maybeUpdateMessageReactions(event); // TODO update mainly in [messages] instead
+      view.notifyListenersIfMessagePresent(event.messageId);
     }
   }
 }

lib/model/message_list.dart

@@ -444,6 +444,14 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     notifyListeners();
   }
 
+  /// Notify listeners if the given message is present in this view.
+  void notifyListenersIfMessagePresent(int messageId) {
+    final index = _findMessageWithId(messageId);
+    if (index != -1) {
+      notifyListeners();
+    }
+  }
+
   static void _applyChangesToMessage(UpdateMessageEvent event, Message message) {
     // TODO(server-5): Cut this fallback; rely on renderingOnly from FL 114
     final isRenderingOnly = event.renderingOnly ?? (event.userId == null);
@@ -515,35 +523,6 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     notifyListeners();
   }
 
-  void maybeUpdateMessageReactions(ReactionEvent event) {
-    final index = _findMessageWithId(event.messageId);
-    if (index == -1) {
-      return;
-    }
-
-    final message = messages[index];
-    switch (event.op) {
-      case ReactionOp.add:
-        (message.reactions ??= Reactions([])).add(Reaction(
-          emojiName: event.emojiName,
-          emojiCode: event.emojiCode,
-          reactionType: event.reactionType,
-          userId: event.userId,
-        ));
-      case ReactionOp.remove:
-        if (message.reactions == null) { // TODO(log)
-          return;
-        }
-        message.reactions!.remove(
-          reactionType: event.reactionType,
-          emojiCode: event.emojiCode,
-          userId: event.userId,
-        );
-    }
-
-    notifyListeners();
-  }
-
   /// Called when the app is reassembled during debugging, e.g. for hot reload.
   ///
   /// This will redo from scratch any computations we can, such as parsing

test/model/message_list_test.dart

@@ -271,6 +271,41 @@ void main() {
     checkInvariants(model);
   });
 
+  group('notifyListenersIfMessagePresent', () {
+    test('message present', () async {
+      await prepare(narrow: const CombinedFeedNarrow());
+      await prepareMessages(foundOldest: false,
+        messages: List.generate(100, (i) => eg.streamMessage(id: 100 + i)));
+      model.notifyListenersIfMessagePresent(150);
+      checkNotifiedOnce();
+    });
+
+    test('message absent', () async {
+      await prepare(narrow: const CombinedFeedNarrow());
+      await prepareMessages(foundOldest: false,
+        messages: List.generate(100, (i) => eg.streamMessage(id: 100 + i))
+          .where((m) => m.id != 150).toList());
+      model.notifyListenersIfMessagePresent(150);
+      checkNotNotified();
+    });
+
+    test('message absent (older than window)', () async {
+      await prepare(narrow: const CombinedFeedNarrow());
+      await prepareMessages(foundOldest: false,
+        messages: List.generate(100, (i) => eg.streamMessage(id: 100 + i)));
+      model.notifyListenersIfMessagePresent(50);
+      checkNotNotified();
+    });
+
+    test('message absent (newer than window)', () async {
+      await prepare(narrow: const CombinedFeedNarrow());
+      await prepareMessages(foundOldest: false,
+        messages: List.generate(100, (i) => eg.streamMessage(id: 100 + i)));
+      model.notifyListenersIfMessagePresent(250);
+      checkNotNotified();
+    });
+  });
+
   group('maybeUpdateMessage', () {
     test('update a message', () async {
       final originalMessage = eg.streamMessage(
@@ -454,6 +489,86 @@ void main() {
     });
   });
 
+  group('regression tests for #455', () {
+    test('reaction events handled once, even when message is in two message lists', () async {
+      final stream = eg.stream();
+      store = eg.store();
+      await store.addStream(stream);
+      await store.addSubscription(eg.subscription(stream));
+      connection = store.connection as FakeApiConnection;
+
+      int notifiedCount1 = 0;
+      final model1 = MessageListView.init(store: store,
+          narrow: StreamNarrow(stream.streamId))
+        ..addListener(() => notifiedCount1++);
+
+      int notifiedCount2 = 0;
+      final model2 = MessageListView.init(store: store,
+          narrow: TopicNarrow(stream.streamId, 'hello'))
+        ..addListener(() => notifiedCount2++);
+
+      for (final m in [model1, model2]) {
+        connection.prepare(json: newestResult(
+          foundOldest: false,
+          messages: [eg.streamMessage(stream: stream, topic: 'hello')]).toJson());
+        await m.fetchInitial();
+      }
+
+      final message = eg.streamMessage(stream: stream, topic: 'hello');
+      await store.handleEvent(MessageEvent(id: 0, message: message));
+
+      await store.handleEvent(
+        eg.reactionEvent(eg.unicodeEmojiReaction, ReactionOp.add, message.id));
+
+      check(notifiedCount1).equals(3); // fetch, new-message event, reaction event
+      check(notifiedCount2).equals(3); // fetch, new-message event, reaction event
+
+      check(model1.messages.last)
+        ..identicalTo(model2.messages.last)
+        ..reactions.isNotNull().total.equals(1);
+    });
+
+    Future<void> checkApplied({
+      required Event Function(Message) mkEvent,
+      required void Function(Subject<Message>) doCheckMessageAfterFetch,
+    }) async {
+      final stream = eg.stream();
+      store = eg.store();
+      await store.addStream(stream);
+      await store.addSubscription(eg.subscription(stream));
+      connection = store.connection as FakeApiConnection;
+
+      final message = eg.streamMessage(stream: stream);
+
+      await store.addMessage(Message.fromJson(message.toJson()));
+      await store.handleEvent(mkEvent(message));
+
+      // init msglist *after* event was handled
+      model = MessageListView.init(store: store, narrow: const CombinedFeedNarrow());
+      checkInvariants(model);
+
+      connection.prepare(json:
+        newestResult(foundOldest: false, messages: [message]).toJson());
+      await model.fetchInitial();
+      checkInvariants(model);
+      doCheckMessageAfterFetch(
+        check(model).messages.single
+          ..id.equals(message.id)
+      );
+    }
+
+    test('ReactionEvent is applied even when message not in any msglists', () async {
+      await checkApplied(
+        mkEvent: (message) =>
+          eg.reactionEvent(eg.unicodeEmojiReaction, ReactionOp.add, message.id),
+        doCheckMessageAfterFetch:
+          (messageSubject) => messageSubject.reactions.isNotNull().total.equals(1),
+      );
+    });
+
+    // TODO(#455) message edits; message flags
+  });
+
   test('reassemble', () async {
     final stream = eg.stream();
     await prepare(narrow: StreamNarrow(stream.streamId));