local echo (6.75/7): Revised commit for local echo (sans retrieving failed outbox content) #1538

chrisbobbe · 2025-05-30T00:06:47Z

Here's a revised version of a substantive commit from #1453:

9c0b955 msglist: Add and manage outbox message objects in message list view

That just leaves one more substantive commit from that PR:

1fa9f24 msglist: Support retrieving failed outbox message content

gnprice · 2025-05-30T00:25:38Z

Thanks! I won't get to reading this until I'm back after next week.

The title's numbering collides with #1535; I saw the title and initially thought this was a new version of that PR. Perhaps this is 6.75/7? 😛

chrisbobbe · 2025-05-30T00:31:34Z

Oh! Indeed.

Perhaps this is 6.75/7? 😛

Yes, that's how I named the branch and how I meant to name the PR. 😛

gnprice

Thanks! I've read through all the non-test changes and part of the tests. Generally this looks good; comments below.

gnprice · 2025-06-11T04:58:19Z

lib/model/message_list.dart

    messages.clear();
+    outboxMessages.clear();
    middleMessage = 0;


nit: keep messages and middleMessage together (like they are in their declarations)

Suggested change

messages.clear();

outboxMessages.clear();

middleMessage = 0;

messages.clear();

middleMessage = 0;

outboxMessages.clear();

gnprice · 2025-06-11T05:04:58Z

lib/model/message_list.dart

+    while (items.isNotEmpty && items.last is! MessageListMessageItem) {
+      items.removeLast();
+    }
+    assert(items.none((e) => e is MessageListOutboxMessageItem));


This will loop through the entire list of items, which may be long with a lot of message history.

Let's avoid that kind of expensive check even in debug builds of the app — the message list already feels somewhat sluggish in debug mode. Instead we rely on the tests and their checkInvariants function to confirm these invariants are satisfied.

gnprice · 2025-06-11T05:15:39Z

lib/model/message_list.dart

    // We'll make the bottom slice start at the last visible message, if any.
+    _removeOutboxMessageItems();
    for (final message in result.messages) {
      if (!_messageVisible(message)) continue;
      middleMessage = messages.length;
      _addMessage(message);
      // Now [middleMessage] is the last message (the one just added).
    }
+    _reprocessOutboxMessages();


It looks like this logic will have outboxMessages get filled in before the fetch even completes. That feels kind of odd. This is a view-model for the message list, and what we actually show in the message list in that state is just a loading indicator and no messages.

And in the future with #82, we won't necessarily want them even after the fetch, depending on the value that haveNewest ends up with after the fetch. (Just like you pointed out at #1515 (comment) for new actual messages.)

So how about we have _syncOutboxMessages get called only here in the fetch-result handler, rather than before the fetch? Then in #1515 we can straightforwardly make it conditional on haveNewest, and do the same in fetchNewer.

Probably that's best done as a separate commit on top, to facilitate merging the main commit.

Makes sense; I agree.

Probably that's best done as a separate commit on top, to facilitate merging the main commit.

Hmm, since #1515 is already merged, the main commit will need to check haveNewest before adding outbox messages to items.

gnprice · 2025-06-11T05:20:30Z

lib/model/message_list.dart

+        bool removed = _removeOutboxMessagesWhere((message) =>
+          message is StreamOutboxMessage
+          && message.conversation.streamId == event.streamId
+          && message.conversation.topic == event.topicName);
+
+        removed |= _removeMessagesWhere((message) =>


nit: handle the normal messages first, since they come earlier in the list

gnprice · 2025-06-11T05:24:24Z

lib/model/message_list.dart

+        for (final outboxMessage in store.outboxMessages.values) {
+          if (_shouldAddOutboxMessage(
+            outboxMessage,
+            wasUnmuted: outboxMessage is StreamOutboxMessage
+              && outboxMessage.conversation.streamId == event.streamId


I think this whole stanza goes away if we let fetchInitial take care of syncing in the outbox messages when appropriate.

gnprice · 2025-06-11T05:28:34Z

lib/model/message_list.dart

    _addMessage(message);
+    if (event.localMessageId != null) {
+      final localMessageId = int.parse(event.localMessageId!);


int.parse should always use explicit radix

gnprice · 2025-06-11T05:35:05Z

test/model/message_test.dart

      checkState().equals(OutboxMessageState.hidden);
+      checkNotNotified();

      async.elapse(kLocalEchoDebounceDuration);
      checkState().equals(OutboxMessageState.waiting);
+      checkNotifiedOnce();

      await receiveMessage(eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]));
      check(store.outboxMessages).isEmpty();
+      checkNotifiedOnce();


Maybe add all these checks as a prep commit? Then if there are any that change with the main change (or if there are none), that will be more visible.

gnprice · 2025-06-11T05:39:41Z

test/model/message_list_test.dart

+      // the bottom slice of `model.messages` is empty
+      check(model.items).length.equals(model.middleItem);


nit: this check is really about validating the behavior of middleItem, not items

Suggested change

// the bottom slice of `model.messages` is empty

check(model.items).length.equals(model.middleItem);

check(model.middleItem).equals(model.items.length);

gnprice · 2025-06-11T05:43:28Z

test/model/message_list_test.dart

+      check(model.items.sublist(model.middleItem))
+        ..first.isA<MessageListOutboxMessageItem>()
+            .message.identicalTo(model.outboxMessages.first)
+        ..every((item) => item.not((it) => it.isA<MessageListMessageItem>()));


This "is not MessageListMessageItem" check is redundant with the "is MessageListOutboxMessageItem" check just above, given the overall structure of items: all items for outbox-messages come after all items for normal messages. And that latter structure was verified in the big for (int j = 0; j < allMessages.length; j++) loop above.

So this every can be dropped. And then the .sublist plus .first can be simplified down.

chrisbobbe · 2025-06-12T07:58:24Z

Thanks for the review! Revision pushed. This revision postpones copying MessageStore.outboxMessages to the message-list's outboxMessages until haveNewest becomes true. It also doesn't add new, just-sent outbox messages unless haveNewest is true.

gnprice

Thanks! Looks great; small comments just on the tests.

gnprice · 2025-06-12T17:55:24Z

lib/model/message_list.dart

+  /// because outbox messages are considered newer than regular messages.
+  ///
+  /// This does not call [notifyListeners].
+  void _syncOutboxMessagesFromStore() {


Cool, I like this name change, too (adding "from store").

gnprice · 2025-06-12T18:05:43Z

test/model/message_list_test.dart

+      check(model.items[model.middleItem])
+        .isA<MessageListOutboxMessageItem>()
+          .message.identicalTo(model.outboxMessages.first);


nit: can make this a bit more visibly parallel to the normal-message case just below it:

Suggested change

check(model.items[model.middleItem])

.isA<MessageListOutboxMessageItem>()

.message.identicalTo(model.outboxMessages.first);

check(model.items[model.middleItem]).isA<MessageListOutboxMessageItem>()

.message.identicalTo(model.outboxMessages.first);

gnprice · 2025-06-12T18:11:56Z

test/model/message_list_test.dart

+      check(model)
+        ..fetched.isFalse()
+        ..outboxMessages.isEmpty();


This second check is implied by the first one plus an invariant, right?

Hmm, we could make that invariant more explicitly checked, though, in checkInvariants:

if (!model.fetched) { check(model) ..messages.isEmpty() + ..outboxMessages.isEmpty()

… I guess the existing test explicitly checks messages is empty, even though that's similarly redundant, because it's helpful for parallelism with the steps above and below. So this check makes sense to include for the same reason. For that same parallelism, though, it's probably clearest to leave it as a separate check call that comes after the checkHasMessageIds, rather than consolidate with the check here on fetched.

gnprice · 2025-06-12T18:15:13Z

test/model/message_list_test.dart

+      connection.prepare(json: newestResult(foundOldest: false,
+        messages: [eg.streamMessage(stream: stream, topic: 'topic')]).toJson());
+      final fetchFuture = model.fetchNewer();


nit for realism: use newerResult (the anchor will be different)

chrisbobbe · 2025-06-12T19:50:03Z

Thanks for the review! Revision pushed.

With comments on additional notifyListeners calls we expect when we start having the message list actually show the local echo in its various states.

This adds some overhead on message-event handling, linear in the number of outbox messages in a view. We rely on that number being small. We add outboxMessages as a list independent from messages on _MessageSequence. Because outbox messages are not rendered (the raw content is shown as plain text), we leave the 1-1 relationship between `messages` and `contents` unchanged. When computing `items`, we now start to look at `outboxMessages` as well, with the guarantee that the items related to outbox messages always come after those for other messages. Look for places that call `_processOutboxMessage(int index)` for references, and the changes to `checkInvariants` on how this affects the message list invariants. This implements minimal support to display outbox message message item widgets in the message list, without indicators for theirs states. Retrieving content from failed sent requests and the full UI are implemented in a later commit. Co-authored-by: Chris Bobbe <[email protected]>

gnprice · 2025-06-12T23:01:38Z

Thanks! Looks good; merging.

chrisbobbe requested a review from gnprice May 30, 2025 00:06

chrisbobbe assigned gnprice May 30, 2025

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label May 30, 2025

chrisbobbe changed the title ~~local echo (6.5/7): Revised commit for local echo (sans retrieving failed outbox content)~~ local echo (6.75/7): Revised commit for local echo (sans retrieving failed outbox content) May 30, 2025

chrisbobbe force-pushed the pr-local-echo-6.75 branch from f12e04f to 109562b Compare May 30, 2025 21:28

chrisbobbe mentioned this pull request May 30, 2025

local echo (7/7): Support simplified version of local echo #1453

Merged

gnprice reviewed Jun 11, 2025

View reviewed changes

chrisbobbe force-pushed the pr-local-echo-6.75 branch from 109562b to c7f1bea Compare June 12, 2025 07:56

gnprice reviewed Jun 12, 2025

View reviewed changes

chrisbobbe force-pushed the pr-local-echo-6.75 branch from c7f1bea to d0f454d Compare June 12, 2025 19:49

chrisbobbe and others added 2 commits June 12, 2025 16:00

message test: Add some is-message-list-notified checks

2a5c741

With comments on additional notifyListeners calls we expect when we start having the message list actually show the local echo in its various states.

gnprice force-pushed the pr-local-echo-6.75 branch from d0f454d to 11f05dc Compare June 12, 2025 23:01

gnprice merged commit 11f05dc into zulip:main Jun 12, 2025
1 check passed

chrisbobbe deleted the pr-local-echo-6.75 branch June 12, 2025 23:02

		// the bottom slice of `model.messages` is empty
		check(model.items).length.equals(model.middleItem);

	// the bottom slice of `model.messages` is empty
	check(model.items).length.equals(model.middleItem);
	check(model.middleItem).equals(model.items.length);

local echo (6.75/7): Revised commit for local echo (sans retrieving failed outbox content) #1538

local echo (6.75/7): Revised commit for local echo (sans retrieving failed outbox content) #1538

Uh oh!

Conversation

chrisbobbe commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented May 30, 2025

Uh oh!

chrisbobbe commented May 30, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jun 12, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jun 12, 2025

Uh oh!

gnprice commented Jun 12, 2025

Uh oh!

Uh oh!

Uh oh!

chrisbobbe commented May 30, 2025 •

edited

Loading