local echo (5/n): Create outbox messages on send #1472

PIG208 · 2025-04-10T23:19:07Z

This is stacked atop #1463, toward #1441.

No UI/user-facing change in this PR.

chrisbobbe

Thanks! Comments below.

chrisbobbe · 2025-04-16T04:03:46Z

lib/model/store.dart

-  /// Always equal to `connection.zulipFeatureLevel`
-  /// and `account.zulipFeatureLevel`.
-  int get zulipFeatureLevel => connection.zulipFeatureLevel!;
-
  String get zulipVersion => account.zulipVersion;


How about also moving zulipVersion along with zulipFeatureLevel, so they stay together?

chrisbobbe · 2025-04-16T05:48:26Z

lib/model/message_list.dart

@@ -626,6 +627,17 @@ class MessageListView with ChangeNotifier, _MessageSequence {
    }
  }

+  void handleOutboxMessage(OutboxMessage outboxMessage) {


How about a name like _addOutboxMessage? There are multiple tasks that could accurately be described as a message list "handling" an outbox message.

chrisbobbe · 2025-04-16T05:49:05Z

lib/model/message_list.dart

+  /// Remove the [outboxMessage] from the view.
+  ///
+  /// This is a no-op if the message is not found.
+  void removeOutboxMessageIfExists(OutboxMessage outboxMessage) {


How about just removeOutboxMessage? I think we can leave the "if exists" part as implied.

chrisbobbe · 2025-04-16T05:57:14Z

lib/model/message.dart

+/// ```
+///
+/// During its lifecycle, it is guaranteed that the outbox message is deleted
+/// as soon an message event with a matching [MessageEvent.localMessageId]


nit: "a message event"

chrisbobbe · 2025-04-16T06:18:14Z

lib/model/message.dart

+/// ```
+///                              ┌─────────────────────────────────────┐
+///                              │                  Event received,    │
+///              Send            │                  or we abandoned    │
+///           immediately.       │      200.        the queue.         ▼
+/// (create) ──────────────► sending ────────► sent ──────────────► (delete)
+///                              │4xx,          │                      ▲
+///                              │other error,  │Reached       User    │
+///                              │or reached    │time limit.   cancels.│
+///                              │time limit.   ▼                      │
+///                              └───────────► failed ─────────────────┘
+/// ```


Can we leave out the "reached time limit" parts in this first version? The "failed" state doesn't feel accurate for when that happens (especially when coming there from "sent"), and it's not part of the spec for #133, the more complicated outbox design, so we'd have to remove it when implementing that. It's also not necessary for the main idea of #1441:

The point is to better handle the case where you type out a message and hit send, and it fails because you don't have working network at that moment.

It looks like "reached time limit" comes from a parenthetical in the #1441 spec, with "perhaps":

Then if a message fails to send, we show on the local-echo placeholder an option that lets you recover it. (Similarly perhaps if it's been a while, like 10s, since trying to send and the request hasn't completed one way or another.)

If we want to keep "reached time limit" in this PR, how about adding a new node for it in the diagram, separate from "failed"? Then I think it'll be easier to reason clearly about some things later: what should the UI say for this state (not "failed" because we haven't been told that it failed); what should happen if the send-message event arrives when in this state.

It looks like the "User cancels" label comes from #133. In that context, it means the user decided not to press a "Retry" button to retry the message-send request, and they want us to just forget about the message-send attempts and their failures.

That label doesn't feel accurate in this context, where a retry button isn't part of the picture. I think the word from the #1441 spec is "recover"; how about we say "User recovers the draft" or similar:

Then if a message fails to send, we show on the local-echo placeholder an option that lets you recover it. […]

You might want to retry sending, or just copy the text to save elsewhere. To cover both options, it can take the text and just put it back in the compose box. […] The placeholder in the message list then disappears.

Is the "sent" state needed? What would happen if we just ignored a 200 response and removed "sent" from the diagram, and didn't consider the message to be sent until we got its event?

I think the "Send immediately" label is implied and can be removed.

Combining all those points, what do you think of this as an updated diagram:

/// We abandoned the queue. /// ┌──────────────────────────────────────┐ /// │ │ /// │ Event received. ▼ /// (create) ─► sending ──────────────────────────────► (delete) /// │ ▲ /// │ 4xx or other User restores │ /// │ error. the draft. │ /// └──────────────► failed ───────────────┘

Regarding time limit, started a discussion here: #mobile > handle failed send @ 💬

For the diagram update, started a discussion here: #mobile > #F1441 Handle retry state machine @ 💬

chrisbobbe · 2025-04-16T08:39:49Z

test/model/message_test.dart

+      check(connection.lastRequest).isA<http.Request>()
+        ..bodyFields['queue_id'].equals(store.queueId)
+        ..bodyFields['local_id'].equals('${outboxMessage.localMessageId}');


Is this connection.lastRequest needed, and if so, should the other similar tests have a check like it too?

We could move these checks to the helper, but I moved them from there to this test in a previous revision since just checking it once seems fine to me; this de-duplicates some helper code and most tests focus on other things.

chrisbobbe · 2025-04-16T08:42:31Z

test/model/message_test.dart

+        ..hidden.isTrue();
+    }));
+
+    test('while message is being sent, message event arrives, then the send fails', () => awaitFakeAsync((async) async {


Perhaps worth a comment (if there isn't one in the implementation) that this can actually happen: the message-send can succeed, but then the message-send request has a network issue that doesn't affect the event-poll request.

chrisbobbe · 2025-04-16T08:51:38Z

test/model/message_test.dart

+
+      // Handle the event after the message is sent but before the debounce
+      // timeout.  The outbox message should remain hidden since the send
+      // request was sucessful.


nit: "successful"

chrisbobbe · 2025-04-16T09:12:09Z

test/model/message_test.dart

+      check(store.outboxMessages).isEmpty();
+      check(outboxMessage)
+        ..state.equals(OutboxMessageLifecycle.sent)
+        ..hidden.isTrue();


A lot of these tests have checks on outboxMessage after checking that store.outboxMessages is empty. Do they all need checks like that? Anyway, this test and later ones do it without a comment explaining why:

// […] The outbox message should no // longer get updated because it is not in the store any more.

The second sentence of the comment before store.handleEvent is meant to address this check:

// Handle the event after the message is sent but before the debounce // timeout. The outbox message should remain hidden since the send // request was successful.

Perhaps it will be clearer to move this right before the check.

chrisbobbe · 2025-04-16T09:17:13Z

test/model/message_test.dart

+        ..hidden.isFalse();
+    });
+
+    test('send request pending until after kSendMessageTimeLimit, completes successfully, then message event arrives', () => awaitFakeAsync((async) async {


(I skipped reading the tests with kSendMessageTimeLimit in their names, pending an earlier comment that might lead to removing them)

PIG208 · 2025-04-16T21:14:19Z

Updated the PR! Thanks for the review. There are some pending questions mainly with regards to the state diagram and the send time limit. I introduced the new waitPeriodExpired state in this revision, with some changes to the states.

PIG208 · 2025-04-17T19:29:39Z

Will be working on a new revision to reorganize some of the implementation code with further state machine changes.

PIG208 · 2025-04-17T21:29:56Z

The PR has been updated to implement the state diagram discussed in chat: #mobile > #F1441 Handle retry state machine @ 💬

gnprice

Thanks @PIG208 for building this, and @chrisbobbe for the previous reviews! Exciting to see this approaching ready.

Generally this logic all looks good; various comments below. I haven't yet read the tests.

gnprice · 2025-05-20T23:13:58Z

lib/model/message.dart

+}
+
+/// Manages the outbox messages portion of [MessageStore].
+mixin _OutboxMessageStore on PerAccountStoreBase {


Let's put all of this new code after MessageStoreImpl, instead of before.

That way in particular MessageStore remains right at the start of the file. It's basically the interface for the whole file from the rest of the code's perspective, so it's helpful to have it emphasized up front that way.

bump — after MessageStoreImpl, not only MessageStore, and including all the new code so the MessageStore interface remains at the top 🙂

(That way when the reader is thinking about aspects other than the outbox, for example the maintenance of the messages map itself, they can quickly go straight to those without skimming past the outbox logic; and conversely when they want to study how the outbox works, that's still all together in one place, namely the end of the file.)

gnprice · 2025-05-20T23:16:50Z

lib/model/message.dart

+const kLocalEchoDebounceDuration = Duration(milliseconds: 500);  // TODO(#1441) find the right value for this
+const kSendMessageOfferRestoreWaitPeriod = Duration(seconds: 10);  // TODO(#1441) find the right value for this


These should each get a bit of dartdoc saying what they mean; it's not evident from the names (particularly "debounce").

The answers are basically explained in the OutboxMessageState docs already, so a reference there may suffice.

gnprice · 2025-05-20T23:19:49Z

lib/model/message.dart

+  /// The [sendMessage] request has started but hasn't finished, and the
+  /// outbox message is hidden to the user.
+  ///
+  /// This is the initial state when an [OutboxMessage] is created.
+  hidden,


In this state, the actual request may or may not have finished, right? If I'm reading the code correctly, the state doesn't change when the request finishes successfully.

Rather than "request hasn't finished", I believe what this wants to say is that the resulting [MessageEvent] hasn't arrived, and nor has the request failed.

gnprice · 2025-05-20T23:33:01Z

lib/model/message.dart

+  /// The message was assumed not delivered after some time it was sent.
+  ///
+  /// This state can be reached when the message event hasn't arrived in
+  /// [kSendMessageOfferRestoreWaitPeriod] since the outbox message's creation.
+  waitPeriodExpired,


Hmm. What happens if

the message gets successfully sent — and we know it succeeded because we got a 200 response from the sendMessage request

but we don't get the event for a while?

For example this would happen if the event queue is gone; in that case the store will eventually get replaced after a fresh registerQueue request, but that can take some time.

This can also happen if the network is just intermittent, e.g. if the user is moving around.

It looks like in that case, this version would cause the outbox-message to appear as failed in the UI, and invite the user to retry sending it.

That seems like it's going to lead to double-sends. I think it'd be better for that case to be shown with a loading indicator, the same as we show before this timeout if we don't yet have a response from the sendMessage request. So in other words the logic would be:

if we got an error on the request, we may not know whether the request really went through, but we assume it didn't, and invite the user to retry

(if it was a 4xx error, then we actually know it didn't go through; but we don't make a distinction based on that)

if we got success on the request, then we know it went through, so we carry on showing the loading indicator indefinitely until we get the MessageEvent to replace it

otherwise, i.e. if the request hasn't yet completed either way, then after a timeout we effectively assume it failed, and so invite the user to retry.

Yeah, this proposed behavior sounds better to me.

I think an easy implementation will just require us to cancel the timer that leads to waitPeriodExpired when the request is successful. At first I thought we might want to re-introduce that sending state from a previous draft, but I believe that shouldn't be necessary.

Sounds good!

gnprice · 2025-05-20T23:40:56Z

lib/model/message.dart

+  /// The message could not be delivered.
+  ///
+  /// This state can be reached when we got a 4xx or other error in the HTTP
+  /// response.
+  failed,


As mentioned in my previous comment just above, I believe this state covers a broader set of situations than this description says: it could be that the message couldn't be delivered (a 4xx response, or a bit more specifically a ZulipApiException), or it could be that we don't know whether it was delivered (a 5xx response, or a bit more generally a Server5xxException; or a network error NetworkException). In the latter case, some of the time it will turn out that the message really was delivered.

Rather than "4xx or other error", I think a reference to [ApiRequestException] would help clarify the space of possibilities.

gnprice · 2025-05-21T00:06:11Z

lib/model/message.dart

+
+  /// A fresh ID to use for [OutboxMessage.localMessageId],
+  /// unique within this instance.
+  int _nextLocalMessageId = 0;


Let's start at 1 rather than 0. That matches the practice in all the server's autoincrementing database indexes.

gnprice · 2025-05-21T00:17:20Z

lib/model/message.dart

+    }
+    return _outboxSendMessage(
+      destination: destination, content: content,
+      realmEmptyTopicDisplayName: realmEmptyTopicDisplayName);


It looks like this doesn't conform to the constraint written on the realmEmptyTopicDisplayName getter, and enforced with an assert there. This will eagerly call that getter, before knowing whether the ZFL means its value will be needed.

I think the refactor that would be needed to make that assertion fully hold for this path is probably too much to be worth doing here directly. Instead, let's just bypass it (e.g., look at _realmEmptyTopicDisplayName directly) and leave a TODO comment.

I think probably the right solution is ultimately to have TopicName.processLikeServer (which is the method that consults this value and causes the need to pass it down here) to instead be a method on the per-account store, and more specifically on the RealmSettingStore (or similar) that we plan to set up in the future. Then it can consult that setting, as well as zulipFeatureLevel, directly off of that substore, and call the realmEmptyTopicDisplayName for itself only after it determines it's needed. But that substore is a post-launch refactor.

gnprice · 2025-05-21T00:18:22Z

lib/model/message.dart

+          topic.processLikeServer(
+            // Processing this just once on creating the outbox message
+            // allows an uncommon bug, because either of these values can change.
+            // During the outbox message's life, a topic processed from


Let's move this topic.processLikeServer call out to a little private helper method. That will keep the flow of the rest of this method's logic a bit easier to read.

gnprice · 2025-05-21T00:23:10Z

lib/model/message.dart

+            realmEmptyTopicDisplayName: realmEmptyTopicDisplayName),
+          displayRecipient: null),
+        content: content),
+      DmDestination(:final userIds) => DmOutboxMessage(


How about giving OutboxMessage a factory constructor that takes an arbitrary Conversation, and dispatches between these two subclass constructors? That would allow this switch to get pushed down into computing just conversation, and the rest of these two constructor calls to get deduplicated. I think that'd make the structure of what this is doing a bit clearer.

The existing OutboxMessage constructor could get renamed to OutboxMessage._ to make room for the nice factory constructor to have the plain OutboxMessage name.

gnprice · 2025-05-21T00:28:38Z

lib/model/message.dart

+    _outboxMessageDebounceTimers[localMessageId] = Timer(kLocalEchoDebounceDuration, () {
+      assert(!_disposed);
+      assert(outboxMessages.containsKey(localMessageId),
+        'The timer should have been canceled when the outbox message was removed.');
+      _outboxMessageDebounceTimers.remove(localMessageId);
+      _updateOutboxMessage(localMessageId, newState: OutboxMessageState.waiting);
+    });


Let's pull this callback out to its own method:

Suggested change

_outboxMessageDebounceTimers[localMessageId] = Timer(kLocalEchoDebounceDuration, () {

assert(!_disposed);

assert(outboxMessages.containsKey(localMessageId),

'The timer should have been canceled when the outbox message was removed.');

_outboxMessageDebounceTimers.remove(localMessageId);

_updateOutboxMessage(localMessageId, newState: OutboxMessageState.waiting);

});

_outboxMessageDebounceTimers[localMessageId] = Timer(

kLocalEchoDebounceDuration, () => _handleOutboxDebounce(localMessageId));

That will help shorten this method and make its overall structure easier to see; and it'll also make clear there's a narrow interface between this overall method and the callback, with just the one bit of data localMessageId passing between them (vs. all the other parameters and locals that this method otherwise would offer to the callback's closure).

PIG208 · 2025-05-21T02:58:07Z

Thanks for the review! Updated the PR.

gnprice

Thanks for the revision! Comments below. I'll also go take a quick initial look at the tests.

gnprice · 2025-05-21T04:11:10Z

lib/model/message.dart

+}
+
+/// Manages the outbox messages portion of [MessageStore].
+mixin _OutboxMessageStore on PerAccountStoreBase {


bump — after MessageStoreImpl, not only MessageStore, and including all the new code so the MessageStore interface remains at the top 🙂

(That way when the reader is thinking about aspects other than the outbox, for example the maintenance of the messages map itself, they can quickly go straight to those without skimming past the outbox logic; and conversely when they want to study how the outbox works, that's still all together in one place, namely the end of the file.)

gnprice · 2025-05-21T04:14:36Z

lib/model/message.dart

+      // TODO move [TopicName.processLikeServer] to a substore, eliminating this
+      realmEmptyTopicDisplayName: _realmEmptyTopicDisplayName);


nit: let's add a link to #1472 (comment) to expand on the details of this thought:

Suggested change

// TODO move [TopicName.processLikeServer] to a substore, eliminating this

realmEmptyTopicDisplayName: _realmEmptyTopicDisplayName);

// TODO move [TopicName.processLikeServer] to a substore, eliminating this;

// see https://github.com/zulip/zulip-flutter/pull/1472#discussion_r2099069276

realmEmptyTopicDisplayName: _realmEmptyTopicDisplayName);

gnprice · 2025-05-21T04:16:35Z

lib/model/message.dart

+  // TODO(dart): This has to be a plain static method, because factories/constructors
+  //   do not support type parameters: https://github.com/dart-lang/language/issues/647
+  static OutboxMessage fromConversation(Conversation conversation, {


Ah right, indeed — so it can't quite have the short OutboxMessage(…) name like I suggested at #1472 (comment) above.

gnprice · 2025-05-21T04:18:07Z

lib/model/message.dart

+        readBySender: true,
+        queueId: queueId,
+        localId: localMessageId.toString());
+      _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel();


Cool, I guess this line is the implementation of #1472 (comment) .

gnprice · 2025-05-21T04:30:57Z

lib/model/message.dart

+        readBySender: true,
+        queueId: queueId,
+        localId: localMessageId.toString());
+      _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel();


This feels a bit mysterious on its own, so let's add a comment explaining why it's here:

Suggested change

_outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel();

// The send request succeeded, so the message was definitely sent.

// Cancel the timer that would have had us start presuming that the

// send might have failed.

_outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel();

and nit: move this outside of the try/catch, to make clear the try/catch is focused on exceptions from _apiSendMessage (where exceptions are expected) and not potential exceptions from this line (which shouldn't be possible). Discussion here:
https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#catch-specific

Writing out that comment also caused me to have the thought: what happens if the request succeeds and the state has already moved to waitPeriodExpired? The expiry means we've already started presuming that it probably failed, but suddenly we know for sure it actually succeeded.

If there isn't a quick change to make that would handle that, it's fine to leave a TODO comment for it instead.

and nit: move this outside of the try/catch, to make clear the try/catch is focused on exceptions from _apiSendMessage (where exceptions are expected) and not potential exceptions from this line (which shouldn't be possible). Discussion here: https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#catch-specific

Ah. This reminds me of adding a if (_disposed) check.

Writing out that comment also caused me to have the thought: what happens if the request succeeds and the state has already moved to waitPeriodExpired? The expiry means we've already started presuming that it probably failed, but suddenly we know for sure it actually succeeded.

If there isn't a quick change to make that would handle that, it's fine to leave a TODO comment for it instead.

I think this would be a matter of implementing a new state transition:

Got an [ApiRequestException]. ┌──────┬────────────────────────────┬──────────► failed │ │ │ │ │ │ [sendMessage] │ │ (create) │ │ request succeeds. │ │ └► hidden waiting ◄─────────────── waitPeriodExpired ──┴─────► (delete) │ ▲ │ ▲ User restores └──────┘ └─────────────────────┘ the draft. Debounce [sendMessage] request timed out. not finished when wait period timed out. Event received. (any state) ─────────────────► (delete)

I guess we can't really convey that waitPeriodExpired cannot be reached again without having a variant of waiting, but we can imply that from the dart doc of waitPeriodExpired:

/// The [sendMessage] HTTP request did not finish in time and the user is /// invited to retry it. /// /// This state can be reached when the request has not finished /// [kSendMessageOfferRestoreWaitPeriod] since the outbox message's creation. waitPeriodExpired,

Added a new commit message: Avoid double-sends after send-message request succeeds addressing this. I found it to be a good way seeing how we can incrementally add features to this state machine.

gnprice

Cool, and generally the tests look good.

I've read the miscellaneous test changes outside the main block in message_test.dart, and skimmed over the latter; just a couple of comments below. I'll leave for another day reading through that main block of tests in full.

gnprice · 2025-05-21T04:45:50Z

test/model/message_test.dart

+    test('smoke DM: hidden -> waiting -> (delete)', () => awaitFakeAsync((async) async {
+      await prepareOutboxMessage(destination: DmDestination(
+        userIds: [eg.selfUser.userId, eg.otherUser.userId]));
+      checkState(expectedState: OutboxMessageState.hidden);


nit: can put these in more canonical checks style:

Suggested change

checkState(expectedState: OutboxMessageState.hidden);

checkState().equals(OutboxMessageState.hidden);

gnprice · 2025-05-21T04:49:07Z

test/model/message_test.dart

+    test('smoke stream message: hidden -> waiting -> (delete)', () => awaitFakeAsync((async) async {
+      await prepareOutboxMessage();
+      checkState(expectedState: OutboxMessageState.hidden);
+
+      async.elapse(kLocalEchoDebounceDuration);
+      checkState(expectedState: OutboxMessageState.waiting);


I like this structure where the helpers center on a single outbox-message. It helps keep the tests focused, since the bulk of them are all about a single outbox-message (because there's very little between-message interaction to be tested).

This implements the waitPeriodExpired -> waiting state transition. GitHub discussion: zulip#1472 (comment)

gnprice

Cool, and here's a full review of the remainder: the tests, and the new commit at the end. Generally all looks good.

gnprice · 2025-05-21T22:27:50Z

test/model/message_test.dart

+    test('smoke stream message: hidden -> waiting -> (delete)', () => awaitFakeAsync((async) async {
+      await prepareOutboxMessage();


nit: since the test description specifically calls for a stream message, the test should explicitly make it a stream message

(the bulk of the other tests below can cheerfully go on using the default that prepareOutboxMessage supplies, since they don't care about the specifics of the message)

gnprice · 2025-05-21T22:29:47Z

test/model/message_test.dart

+      await store.handleEvent(eg.messageEvent(
+        eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]),
+        localMessageId: store.outboxMessages.keys.single));


nit: could tighten these tests further (going in the same direction noted at #1472 (comment)) with a helper for this step:

Suggested change

await store.handleEvent(eg.messageEvent(

eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]),

localMessageId: store.outboxMessages.keys.single));

await receiveMessage(eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]));

Could even have that helper default to the shared message local, since that's what most of these tests use.

gnprice · 2025-05-21T22:35:05Z

test/model/message_test.dart

+      checkState().equals(OutboxMessageState.waiting);
+    }));
+
+    test('waitPeriodExpired -> waiting and never return to waitPeriodExpired', () => awaitFakeAsync((async) async {


nit:

Suggested change

test('waitPeriodExpired -> waiting and never return to waitPeriodExpired', () => awaitFakeAsync((async) async {

test('waiting -> waitPeriodExpired -> waiting and never return to waitPeriodExpired', () => awaitFakeAsync((async) async {

to express that this is the test case that covers that first arrow too (right? I don't see another that's more specific)

There are two cases of "waiting -> waitPeriodExpired -> (delete) …" that cover this. But I think including the first edge will still be helpful because it shows that there is a loop.

Cool. Yeah, I see those as focused on the later arrow that goes to "(delete)"; partly because they're tucked inside the group "… -> (delete)". When looking for the test that covers this arrow of the state diagram, this seems like the more natural spot to find it.

(Ideally in fact I think there'd be a case here that was explicitly focused on just that arrow, without a subsequent arrow.)

gnprice · 2025-05-21T22:38:49Z

test/model/message_test.dart

+        await check(outboxMessageFailFuture).throws();
+        async.elapse(Duration(seconds: 1));
+        checkState().equals(OutboxMessageState.failed);


After the future completes, that should mean the timer is done, right? Does this test need the additional 1s async.elapse?

In any case, it'd be good to put the check before the elapse — that strengthens the test a bit, and when the failure has happened the state should promptly be getting set to failed.

gnprice · 2025-05-21T22:40:23Z

test/model/message_test.dart

+        // Complete the send request.  There should be no error despite
+        // the send request failure, because the outbox message is not
+        // in the store any more.
+        await check(outboxMessageFailFuture).completes();


Cool, this is an important subtle case to test.

(Important because it could be fairly common if the user is on a flaky connection — if the rate of requests randomly failing is anywhere between like 20% and 80%, then a good fraction of the time, the send will make it through, the response won't and so the request will eventually throw when the connection times out, and in the meantime the event will make it through. Important because also it'd be pretty annoying to get a useless error dialog when nothing actually went wrong.)

Yeah, exactly. This is for covering the change from Chris here: #1472 (comment)

gnprice · 2025-05-21T22:48:19Z

test/model/message_test.dart

+      message = eg.streamMessage(stream: stream);
+      await prepare(stream: stream);
+      await prepareMessages([eg.streamMessage(stream: stream)]);
+      connection.prepare(apiException: eg.apiBadRequest(), delay: delay);


nit: for better realism, make it an error that's consistent with the request having actually succeeded on the server side:

Suggested change

connection.prepare(apiException: eg.apiBadRequest(), delay: delay);

connection.prepare(httpException: SocketException('failed'), delay: delay);

That way it's consistent with the test cases where a corresponding MessageEvent arrives even though the request failed. That also may help a future reader in understanding why that scenario can even happen (which is important to understand since we have some logic carefully written to specifically handle it).

gnprice · 2025-05-21T22:52:55Z

test/model/message_test.dart

+        destination: StreamDestination(stream.streamId, TopicName('(no topic)')),
+        zulipFeatureLevel: 370);
+      async.elapse(kLocalEchoDebounceDuration);
+      check(store.outboxMessages.values.single)


nit: use checks-getters

Suggested change

check(store.outboxMessages.values.single)

check(store.outboxMessages).values.single

gnprice · 2025-05-21T22:57:37Z

lib/model/message.dart

+    if (_outboxMessages[localMessageId]!.state
+          == OutboxMessageState.waitPeriodExpired) {


Neat, glad that change turned out to be clean to make! (Following up on #1472 (comment).)

The diffstat is:
lib/model/message.dart | 28 ++++++++++++++++++++--------
test/model/message_test.dart | 28 ++++++++++++++++++++++++++++
2 files changed, 48 insertions(+), 8 deletions(-)

and outside the new test, most of that is comments; I count 6+/1- other than tests and comments.

This implements the waitPeriodExpired -> waiting state transition. GitHub discussion: zulip#1472 (comment)

gnprice

Thanks for the revision! Just one nit remaining.

gnprice · 2025-05-26T05:00:42Z

test/model/message_test.dart

+
+    final localMessageIds = store.outboxMessages.keys.toList();
+    store.takeOutboxMessage(localMessageIds.removeAt(5));
+    check(store.outboxMessages.keys).deepEquals(localMessageIds);


nit: use checks-getter (like #1472 (comment) )

Suggested change

check(store.outboxMessages.keys).deepEquals(localMessageIds);

check(store.outboxMessages).keys.deepEquals(localMessageIds);

gnprice · 2025-05-26T05:05:37Z

test/model/message_test.dart

+      checkState().equals(OutboxMessageState.waiting);
+    }));
+
+    test('waitPeriodExpired -> waiting and never return to waitPeriodExpired', () => awaitFakeAsync((async) async {


Cool. Yeah, I see those as focused on the later arrow that goes to "(delete)"; partly because they're tucked inside the group "… -> (delete)". When looking for the test that covers this arrow of the state diagram, this seems like the more natural spot to find it.

(Ideally in fact I think there'd be a case here that was explicitly focused on just that arrow, without a subsequent arrow.)

This change should have no user-facing effect. The one spot where we have an `if (_disposed)` check in editMessage prevents a state update and a rebuild from happening. This only applies if the store is disposed before the edit request fails, but the MessageListView with the edited message should get rebuilt anyway (through onNewStore) when that happens.

While we do create outbox messages, there are in no way user-visible changes since the outbox messages don't end up in message list views. We create skeletons for helpers needed from message list view, but don't implement them yet, to make the diff smaller. For testing, similar to TypingNotifier.debugEnable, we add MessageStoreImpl.debugOutboxEnable for tests that do not intend to cover outbox messages.

PIG208 · 2025-05-26T20:33:34Z

In addition to the nit, added a test that targets "waiting -> waitPeriodExpired" specifically, for #1472 (comment), dropping the emphasis on "waiting -> waitPeriodExpired" from those two "waiting -> waitingPeriodExpired -> (delete)" tests.

This implements the waitPeriodExpired -> waiting state transition. GitHub discussion: zulip#1472 (comment)

gnprice · 2025-05-28T00:15:07Z

Thanks! Those changes all look good; merging.

PIG208 force-pushed the pr-echo-5 branch 15 times, most recently from eeb6ef2 to ac35860 Compare April 16, 2025 01:35

PIG208 requested a review from chrisbobbe April 16, 2025 01:35

PIG208 assigned chrisbobbe Apr 16, 2025

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Apr 16, 2025

chrisbobbe reviewed Apr 16, 2025

View reviewed changes

PIG208 force-pushed the pr-echo-5 branch from ac35860 to 05ea106 Compare April 16, 2025 21:06

PIG208 removed the maintainer review PR ready for review by Zulip maintainers label Apr 17, 2025

PIG208 force-pushed the pr-echo-5 branch from 05ea106 to eed5bf0 Compare April 17, 2025 21:29

PIG208 force-pushed the pr-echo-5 branch 2 times, most recently from b96c5e9 to 13edb83 Compare April 17, 2025 22:56

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Apr 18, 2025

PIG208 requested a review from chrisbobbe April 18, 2025 00:44

PIG208 force-pushed the pr-echo-5 branch from 13edb83 to ca25c0f Compare April 22, 2025 02:58

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels May 20, 2025

PIG208 force-pushed the pr-echo-5 branch 2 times, most recently from 401f7b7 to eddca30 Compare May 20, 2025 23:09

gnprice reviewed May 21, 2025

View reviewed changes

PIG208 force-pushed the pr-echo-5 branch from eddca30 to b346e49 Compare May 21, 2025 02:58

gnprice reviewed May 21, 2025

View reviewed changes

PIG208 added a commit to PIG208/zulip-flutter that referenced this pull request May 21, 2025

message: Avoid double-sends after send-message request succeeds

55092c8

This implements the waitPeriodExpired -> waiting state transition. GitHub discussion: zulip#1472 (comment)

PIG208 force-pushed the pr-echo-5 branch from b346e49 to 55092c8 Compare May 21, 2025 21:02

PIG208 added a commit to PIG208/zulip-flutter that referenced this pull request May 21, 2025

message: Avoid double-sends after send-message request succeeds

0cf5d51

This implements the waitPeriodExpired -> waiting state transition. GitHub discussion: zulip#1472 (comment)

PIG208 added a commit to PIG208/zulip-flutter that referenced this pull request May 21, 2025

message: Avoid double-sends after send-message request succeeds

1e13b81

This implements the waitPeriodExpired -> waiting state transition. GitHub discussion: zulip#1472 (comment)

PIG208 force-pushed the pr-echo-5 branch from 55092c8 to 1e13b81 Compare May 21, 2025 21:47

gnprice reviewed May 21, 2025

View reviewed changes

PIG208 added a commit to PIG208/zulip-flutter that referenced this pull request May 22, 2025

message: Avoid double-sends after send-message request succeeds

73957af

This implements the waitPeriodExpired -> waiting state transition. GitHub discussion: zulip#1472 (comment)

PIG208 force-pushed the pr-echo-5 branch from 1e13b81 to 73957af Compare May 22, 2025 00:02

PIG208 added a commit to PIG208/zulip-flutter that referenced this pull request May 22, 2025

message: Avoid double-sends after send-message request succeeds

3b1c3f1

This implements the waitPeriodExpired -> waiting state transition. GitHub discussion: zulip#1472 (comment)

PIG208 force-pushed the pr-echo-5 branch from 73957af to 3b1c3f1 Compare May 22, 2025 00:05

PIG208 added a commit to PIG208/zulip-flutter that referenced this pull request May 22, 2025

message: Avoid double-sends after send-message request succeeds

0fdc093

This implements the waitPeriodExpired -> waiting state transition. GitHub discussion: zulip#1472 (comment)

gnprice reviewed May 26, 2025

View reviewed changes

gnprice mentioned this pull request May 26, 2025

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

Merged

PIG208 added 2 commits May 26, 2025 16:30

PIG208 force-pushed the pr-echo-5 branch from 3b1c3f1 to 17a1a4c Compare May 26, 2025 20:33

message: Avoid double-sends after send-message request succeeds

61e343b

This implements the waitPeriodExpired -> waiting state transition. GitHub discussion: zulip#1472 (comment)

gnprice merged commit 61e343b into zulip:main May 28, 2025
1 check passed

PIG208 deleted the pr-echo-5 branch May 28, 2025 14:39

		const kLocalEchoDebounceDuration = Duration(milliseconds: 500); // TODO(#1441) find the right value for this
		const kSendMessageOfferRestoreWaitPeriod = Duration(seconds: 10); // TODO(#1441) find the right value for this

		// TODO move [TopicName.processLikeServer] to a substore, eliminating this
		realmEmptyTopicDisplayName: _realmEmptyTopicDisplayName);

-      _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel();
+      // The send request succeeded, so the message was definitely sent.
+      // Cancel the timer that would have had us start presuming that the
+      // send might have failed.
+      _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel();

	checkState(expectedState: OutboxMessageState.hidden);
	checkState().equals(OutboxMessageState.hidden);

		test('smoke stream message: hidden -> waiting -> (delete)', () => awaitFakeAsync((async) async {
		await prepareOutboxMessage();

	test('waitPeriodExpired -> waiting and never return to waitPeriodExpired', () => awaitFakeAsync((async) async {
	test('waiting -> waitPeriodExpired -> waiting and never return to waitPeriodExpired', () => awaitFakeAsync((async) async {

	connection.prepare(apiException: eg.apiBadRequest(), delay: delay);
	connection.prepare(httpException: SocketException('failed'), delay: delay);

	check(store.outboxMessages.values.single)
	check(store.outboxMessages).values.single

		if (_outboxMessages[localMessageId]!.state
		== OutboxMessageState.waitPeriodExpired) {

	check(store.outboxMessages.keys).deepEquals(localMessageIds);
	check(store.outboxMessages).keys.deepEquals(localMessageIds);

local echo (5/n): Create outbox messages on send #1472

local echo (5/n): Create outbox messages on send #1472

Uh oh!

Conversation

PIG208 commented Apr 10, 2025 • edited by gnprice Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe 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!

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!

PIG208 commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PIG208 commented Apr 17, 2025

Uh oh!

PIG208 commented Apr 17, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PIG208 commented Apr 10, 2025 •

edited by gnprice

Loading

PIG208 commented Apr 16, 2025 •

edited

Loading