msglist: Drop outbox message when its message arrives in a fetch

Fixes part of #2397.

When the event queue is stuck but other requests are succeeding
(see e.g. #1884 and #514 for ways that can happen), a sent message
can arrive in a message fetch with no message event to remove the
corresponding outbox message.  The user would see the message stuck
in a "sending" state, next to the delivered copy of the same message
if that copy is in view.

Now reconcileMessages drops any outbox message whose message is in
the store, recognized by the message ID recorded from the send
response (#2397).

Message lists respond by removing their copy of the outbox message,
through a new method named for this cause of removal.

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

Author: chrisbobbe

lib/model/message.dart

@@ -435,6 +435,8 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage
         ifAbsent: () => _reconcileUnrecognizedMessage(message),
         (current) => _reconcileRecognizedMessage(current, message));
     }
+
+    _removeDeliveredOutboxMessages();
   }
 
   Message _reconcileUnrecognizedMessage(Message incoming) {
@@ -952,15 +954,19 @@ const kSendMessageOfferRestoreWaitPeriod = Duration(seconds: 10);  // TODO(#1441
 ///         timed out.   not finished when
 ///                      wait period timed out.
 ///
-///              Event received.  Or [sendMessage]
-///              request succeeds and we're sending to
-///              an unsubscribed channel.
+///              Event received.  Or message found in a
+///              fetch after [sendMessage] request succeeds.
+///              Or [sendMessage] request succeeds and
+///              we're sending to an unsubscribed channel.
 /// (any state) ───────────────────────────────────────► (delete)
 /// ```
 ///
 /// During its lifecycle, it is guaranteed that the outbox message is deleted
 /// as soon a message event with a matching [MessageEvent.localMessageId]
 /// arrives.
+/// Once the [sendMessage] request has succeeded, the outbox message is also
+/// deleted as soon as a message with a matching [OutboxMessage.messageId]
+/// is found in a fetch; see [MessageStoreImpl.reconcileMessages].
 /// If we're sending to an unsubscribed channel, we don't expect an event
 /// (see "third buggy behavior" in #1798) so in that case
 /// the outbox message is deleted when the [sendMessage] request succeeds.
@@ -1001,8 +1007,9 @@ enum OutboxMessageState {
 ///
 /// A request remains "outstanding" even after the [sendMessage] HTTP request
 /// completes, whether with success or failure.
-/// The outbox-message persists until either the corresponding [MessageEvent]
-/// arrives to replace it, or the user discards it (perhaps to try again).
+/// The outbox-message persists until either the corresponding message
+/// arrives to replace it (in a [MessageEvent] or, once the request has
+/// succeeded, in a fetch), or the user discards it (perhaps to try again).
 /// For details, see the state diagram at [OutboxMessageState],
 /// and [MessageStore.takeOutboxMessage].
 sealed class OutboxMessage<T extends Conversation> extends MessageBase<T> {
@@ -1117,6 +1124,9 @@ mixin _OutboxMessageStore on HasChannelStore {
   /// unique within this instance.
   int _nextLocalMessageId = 1;
 
+  /// As in [MessageStoreImpl.messages].
+  Map<int, Message> get messages;
+
   /// As in [MessageStoreImpl._messageListViews].
   Set<MessageListView> get _messageListViews;
 
@@ -1317,6 +1327,40 @@ mixin _OutboxMessageStore on HasChannelStore {
     return removed;
   }
 
+  /// Remove any outbox messages whose anticipated [Message],
+  /// as identified by [OutboxMessage.messageId], is in [messages],
+  /// updating message-list views accordingly.
+  ///
+  /// This is how outbox messages get removed when their messages
+  /// are received through a fetch instead of a [MessageEvent];
+  /// see [MessageStoreImpl.reconcileMessages].
+  void _removeDeliveredOutboxMessages() {
+    assert(!_disposed);
+    // This runs on every fetch (see [MessageStoreImpl.reconcileMessages]);
+    // return cheaply in the common case of an empty outbox.
+    if (_outboxMessages.isEmpty) return;
+    final delivered = _outboxMessages.values
+      .where((outboxMessage) {
+        final messageId = outboxMessage._messageId;
+        return messageId != null && messages.containsKey(messageId);
+      })
+      .toList();
+    for (final outboxMessage in delivered) {
+      _removeDeliveredOutboxMessage(outboxMessage);
+    }
+  }
+
+  /// Remove [outboxMessage], whose anticipated [Message]
+  /// (see [OutboxMessage.messageId]) must be in [messages],
+  /// updating message-list views accordingly.
+  void _removeDeliveredOutboxMessage(OutboxMessage outboxMessage) {
+    assert(messages.containsKey(outboxMessage._messageId));
+    _removeOutboxMessage(outboxMessage.localMessageId);
+    for (final view in _messageListViews) {
+      view.handleOutboxMessageDelivered(outboxMessage);
+    }
+  }
+
   void _handleMessageEventOutbox(MessageEvent event) {
     if (event.localMessageId != null) {
       final localMessageId = int.parse(event.localMessageId!, radix: 10);

lib/model/message_list.dart

@@ -1100,6 +1100,16 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     }
   }
 
+  /// Remove the [outboxMessage] from the view, its anticipated message
+  /// (see [OutboxMessage.messageId]) having been received in a fetch.
+  ///
+  /// This is a no-op if the message is not found.
+  void handleOutboxMessageDelivered(OutboxMessage outboxMessage) {
+    if (_removeOutboxMessage(outboxMessage)) {
+      notifyListeners();
+    }
+  }
+
   void handleUserTopicEvent(UserTopicEvent event) {
     switch (_canAffectVisibility(event)) {
       case UserTopicVisibilityEffect.none:

test/model/message_test.dart

@@ -463,6 +463,29 @@ void main() {
         checkNotifiedOnce();
       }));
 
+      test('waiting -> (delete) because message found in fetch', () => awaitFakeAsync((async) async {
+        // Regression test for: https://github.com/zulip/zulip-flutter/issues/2397
+        await prepareOutboxMessage();
+        async.elapse(kLocalEchoDebounceDuration);
+        checkState().equals(OutboxMessageState.waiting);
+        checkNotifiedOnce();
+
+        store.reconcileMessages([message]);
+        check(store.outboxMessages).isEmpty();
+        checkNotifiedOnce();
+      }));
+
+      test('no delete when unrelated message found in fetch', () => awaitFakeAsync((async) async {
+        await prepareOutboxMessage();
+        async.elapse(kLocalEchoDebounceDuration);
+        checkState().equals(OutboxMessageState.waiting);
+        checkNotifiedOnce();
+
+        store.reconcileMessages([eg.streamMessage(stream: stream)]);
+        checkState().equals(OutboxMessageState.waiting);
+        checkNotNotified();
+      }));
+
       test('waitPeriodExpired -> (delete) when event arrives before send request fails', () => awaitFakeAsync((async) async {
         // Set up an error to fail `sendMessage` with a delay, leaving time for
         // the message event to arrive.