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

PIG208 · 2025-04-01T22:03:48Z

Stacked atop #1472.

(This branch can be used to preview the full implementation)

screenshots

message sent	got message event

message failed to send	message content tapped	recipient headers

This is where the progress bar for outbox messages will go, so this is for consistency with that. Discussion: zulip#1453 (comment) [chris: fixed to maintain 4px bottom padding in the common case where the progress bar is absent] Co-authored-by: Chris Bobbe <[email protected]>

chrisbobbe · 2025-05-28T21:27:04Z

And I've sent revised versions of the next chunk, in #1535.

This is where the progress bar for outbox messages will go, so this is for consistency with that. Discussion: zulip#1453 (comment) [chris: fixed to maintain 4px bottom padding in the common case where the progress bar is absent] Co-authored-by: Chris Bobbe <[email protected]>

Different from the Figma design, the bottom padding below the progress bar is changed from 0.5px to 2px, as discussed here: zulip#1453 (comment) Fixes: zulip#1441 Co-authored-by: Chris Bobbe <[email protected]>

chrisbobbe · 2025-05-30T21:30:15Z

OK, and now rebased with a revision ready for review! This is stacked atop #1538, which is a separate PR because it's a substantial commit.

gnprice

Thanks for picking this up! I've read through the commits after #1538:
72ffbd8 compose: Remove a redundant TypingNotifier.stoppedComposing call
3b9366e compose [nfc]: Remove obsoleted TODO(#720)s
d56e20f compose [nfc]: Expand a comment to include an edge case
419e17b msglist: Support retrieving failed outbox message content

except the tests of the last/main commit. All looks good except one small comment below. (And I should read the tests too.)

gnprice · 2025-06-11T06:01:36Z

lib/widgets/message_list.dart

+  Widget build(BuildContext context) {
+    final message = item.message;
+    final localMessageId = message.localMessageId;
+    final state = item.message.state;


Maybe just say message.state in the two places this is used. I feel like a bare name state could be confusing in the context of a widget — sounds like the state of some stateful widget.

Different from the Figma design, the bottom padding below the progress bar is changed from 0.5px to 2px, as discussed here: zulip#1453 (comment) Fixes: zulip#1441 Co-authored-by: Chris Bobbe <[email protected]>

chrisbobbe · 2025-06-12T20:04:11Z

Thanks for the review! Revision pushed, atop the current #1538.

(edit: and again, resolving a small conflict)

Different from the Figma design, the bottom padding below the progress bar is changed from 0.5px to 2px, as discussed here: zulip#1453 (comment) Fixes: zulip#1441 Co-authored-by: Chris Bobbe <[email protected]>

chrisbobbe · 2025-06-13T01:04:47Z

Rebased on current main; conflicts resolved.

gnprice

Thanks! Also just read through those tests; a handful of comments below.

gnprice · 2025-06-13T00:53:49Z

lib/widgets/compose_box.dart

+      final controller = this.controller;
+      controller
+        ..content.value = TextEditingValue(text: outboxMessage.contentMarkdown)
+        ..contentFocusNode.requestFocus();


This bit should ideally get a test.

In the interest of merging this for launch, let's just leave a TODO comment for it in the test code.

This is easy to test :) done.

Cool, looks good :)

gnprice · 2025-06-13T01:02:00Z

test/widgets/message_list_test.dart

+      // Navigate back to the message list page without a compose box,
+      // where the failed to send message should be visible.
+      await tester.pageBack();
+      await tester.pump(); // handle tap
+      await tester.pump(const Duration(milliseconds: 500)); // wait for navigation


Should this be adapted to not specify the duration, per that upstream PR?

Because customer_testing pins the version of our repo, I guess it's not an immediate problem (even for that upstream PR) if we merge it this way. But if the upstream PR lands and this is already merged, it'd break our CI.

gnprice · 2025-06-13T01:03:40Z

test/widgets/message_list_test.dart

+      await checkTapNotRestoreMessage(tester);
+    });
+
+    testWidgets('hidden -> failed, tap to restore message', (tester) async {


nit: perhaps put this above the "tapping does nothing if compose box is not offered" case — this is a lot simpler and serves as a sort of baseline for understanding the more complex cases

Different from the Figma design, the bottom padding below the progress bar is changed from 0.5px to 2px, as discussed here: zulip#1453 (comment) Fixes: zulip#1441 Co-authored-by: Chris Bobbe <[email protected]>

chrisbobbe · 2025-06-13T01:22:35Z

Thanks for the review! Revision pushed.

gnprice · 2025-06-13T01:27:43Z

Thanks! Looks good; merging.

… perhaps after #1568 lands, in order to pin that one down with certainty without another CI cycle.

Issue zulip#720 is superseded by zulip#1441, in which we'll still clear the compose box when the send button is tapped. (We'll still preserve the composing progress in case the send fails, but we'll do so in an OutboxMessage instead of within the compose input in a disabled state.)

Issue zulip#720 is superseded by zulip#1441, and these don't apply... I guess with the exception of a note on how a generic "x" button could be laid out, so we leave that.

Different from the Figma design, the bottom padding below the progress bar is changed from 0.5px to 2px, as discussed here: zulip#1453 (comment) Fixes: zulip#1441 Co-authored-by: Chris Bobbe <[email protected]>

This was referenced Apr 1, 2025

local echo (1/n): Add API support for local_id/local_message_id and queue_id #1454

Merged

local echo (2/n): Rearrange queueId and sendMessage within store #1455

Merged

PIG208 changed the title ~~Support simplified version a local echo (n/n)~~ Support simplified version of local echo (n/n) Apr 1, 2025

PIG208 changed the title ~~Support simplified version of local echo (n/n)~~ locale echo (n/n): Support simplified version of local echo Apr 1, 2025

PIG208 force-pushed the pr-echo branch from 8d1f962 to b73dc22 Compare April 1, 2025 22:14

PIG208 changed the title ~~locale echo (n/n): Support simplified version of local echo~~ local echo (n/n): Support simplified version of local echo Apr 2, 2025

PIG208 mentioned this pull request Apr 3, 2025

local echo (3/n): Pull out MessageBase #1463

Merged

PIG208 force-pushed the pr-echo branch 16 times, most recently from 988c615 to f297a65 Compare April 15, 2025 01:45

PIG208 marked this pull request as ready for review April 15, 2025 01:49

PIG208 force-pushed the pr-echo branch 5 times, most recently from aa81e2f to 28d545f Compare April 17, 2025 18:49

PIG208 changed the title ~~local echo (n/n): Support simplified version of local echo~~ local echo (6/6): Support simplified version of local echo Apr 17, 2025

chrisbobbe mentioned this pull request May 28, 2025

local echo (6.5/7): Revised prep commits for local echo #1535

Merged

chrisbobbe mentioned this pull request May 30, 2025

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

Merged

chrisbobbe force-pushed the pr-echo branch from 1fa9f24 to 419e17b Compare May 30, 2025 21:29

gnprice added the integration review Added by maintainers when PR may be ready for integration label Jun 10, 2025

gnprice reviewed Jun 11, 2025

View reviewed changes

chrisbobbe force-pushed the pr-echo branch from 419e17b to 64cdc7d Compare June 12, 2025 20:03

chrisbobbe force-pushed the pr-echo branch from 64cdc7d to d0c8988 Compare June 12, 2025 20:06

chrisbobbe force-pushed the pr-echo branch from d0c8988 to 8f3ca6b Compare June 13, 2025 01:04

gnprice reviewed Jun 13, 2025

View reviewed changes

chrisbobbe force-pushed the pr-echo branch from 8f3ca6b to 8c74297 Compare June 13, 2025 01:22

chrisbobbe and others added 4 commits June 12, 2025 18:59

compose [nfc]: Remove obsoleted TODO(zulip#720)s

a670966

Issue zulip#720 is superseded by zulip#1441, and these don't apply... I guess with the exception of a note on how a generic "x" button could be laid out, so we leave that.

compose [nfc]: Expand a comment to include an edge case

2003b63

msglist: Support retrieving failed outbox message content

7e29108

Different from the Figma design, the bottom padding below the progress bar is changed from 0.5px to 2px, as discussed here: zulip#1453 (comment) Fixes: zulip#1441 Co-authored-by: Chris Bobbe <[email protected]>

gnprice force-pushed the pr-echo branch from 8c74297 to 7e29108 Compare June 13, 2025 01:59

gnprice merged commit 7e29108 into zulip:main Jun 13, 2025
1 check passed

PIG208 deleted the pr-echo branch June 13, 2025 02:47

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

chrisbobbe commented May 30, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

chrisbobbe commented Jun 13, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jun 13, 2025

Uh oh!

gnprice commented Jun 13, 2025

Uh oh!

Uh oh!

Uh oh!

PIG208 commented Apr 1, 2025 •

edited by gnprice

Loading

chrisbobbe commented May 28, 2025 •

edited

Loading

chrisbobbe commented Jun 12, 2025 •

edited

Loading