Edit-message (8/n): Implement edit-message! #1498

chrisbobbe · 2025-05-03T02:55:21Z

Edit-message! 🎉

Fixes: #126

Screenshots

Light	Dark

chrisbobbe · 2025-05-05T22:51:21Z

Oh, and if you tap "Edit message" in the action sheet but there's already text in the compose box, here's the dialog you get. Giving the "Discard" button a destructive/red style is future work, #1032.

Light	Dark

alya · 2025-05-06T07:00:54Z

Did we discuss the strings anywhere? I would maybe write something like this:

Discard the message you're writing?
When you edit a message, the content that was previously in the compose box is discarded.
[ Cancel] [Discard]

alya · 2025-05-06T07:44:58Z

I think it's good to put a . at the end of user-visible error strings.

chrisbobbe · 2025-05-06T19:19:32Z

Makes sense; I started a discussion on CZO: #mobile-team > Edit-message: UI strings @ 💬

chrisbobbe · 2025-05-06T22:27:21Z

Revision pushed, with those UI string updates, and I've updated the screenshots.

chrisbobbe · 2025-05-06T23:04:11Z

(Oops, fixed a failing test.)

PIG208

Thanks for the PR! Looks pretty good to me overall. Left some comments.

PIG208 · 2025-05-06T22:39:36Z

lib/widgets/compose_box.dart

+    designVariables.bannerBgIntInfo;
+
+  @override
+  Widget? buildTrailing(context) {


nit: this can be

Suggested change

Widget? buildTrailing(context) {

Widget buildTrailing(context) {

PIG208 · 2025-05-06T22:59:44Z

lib/widgets/message_list.dart

+        case true:
+          content = _withRestoreEditMessageGestureDetector(context, content);
+        case false:
+          content = IgnorePointer(child: content);


What's the motivation of IgnorePointer here? At first, I thought it's for disabling the long press menu when the edit request is pending. But it seems like that menu is still available.

It's meant to neutralize interactive message content like links, spoilers, etc.; I thought it made sense along with the faded 0.6-opacity appearance in the Figma. I can add a comment about that.

PIG208 · 2025-05-06T23:03:04Z

lib/widgets/action_sheet.dart

+    editLimit != null
+    && editLimit != 0 // TODO(server-6) remove (pre-FL 138, 0 represents no limit)
+    && now - message.timestamp > editLimit;
+  final showEditButton = message.senderId == store.selfUserId


One other thing we can check for it whether the message contains a poll, since polls cannot be edited.

Oh, TIL; thanks.

PIG208 · 2025-05-06T23:05:21Z

lib/widgets/message_list.dart

+    if (editMessageErrorStatus != null) {
+      content = Opacity(opacity: 0.6, child: content);
+      switch (editMessageErrorStatus) {
+        case true:


nit: perhaps match the order of the previous switch since they are parallel to each other

PIG208 · 2025-05-06T23:13:38Z

lib/widgets/message_list.dart

+        final controller = composeBoxState?.controller;
+        final editMessageInProgress =
+          controller is EditMessageComposeBoxController
+          || editMessageErrorStatus != null;


The controller is EditMessageComposeBoxController check seems quite interesting. editMessageErrorStatus != null sounds reasonable, but I can't really grasp why controller needs to be of certain type. Maybe this needs a comment?

This condition means whether the compose box is in edit-message mode instead of the default write-new-message mode; I'll write a comment :)

PIG208 · 2025-05-07T00:06:11Z

lib/widgets/action_sheet.dart

+    && editLimit != 0 // TODO(server-6) remove (pre-FL 138, 0 represents no limit)
+    && now - message.timestamp > editLimit;
+  final showEditButton = message.senderId == store.selfUserId
+    && messageListHasComposeBox


How about

Suggested change

&& messageListHasComposeBox

&& isComposeBoxOffered

and we remove messageListHasComposeBox?

Oh indeed, I didn't see that we already have the value we need.

PIG208 · 2025-05-07T00:21:41Z

lib/widgets/message_list.dart

+    Widget content = MessageContent(message: message, content: item.content);
+
+    if (editMessageErrorStatus != null) {
+      content = Opacity(opacity: 0.6, child: content);


We should also apply the opacity to the sender row, if there is one:

Left a comment about this after asking Greg in the office:

// The Figma also fades the sender row: // https://github.com/zulip/zulip-flutter/pull/1498#discussion_r2076574000 // We've decided to just fade the message content because that's the only // thing that's changing.

PIG208 · 2025-05-07T00:38:39Z

test/widgets/action_sheet_test.dart

+        /// Test whether the edit-message button is visible, given params.
+        ///
+        /// The message timestamp is 60s before the current time
+        /// ([TestBinding.utcNow]) as of the start of the test run.


nit: TestZulipBinding

Indeed—also, thanks for creating this; you must have noticed I cherry-picked your commits :)

Glad that helps!

PIG208 · 2025-05-07T00:49:43Z

test/widgets/action_sheet_test.dart

+              realmMessageContentEditLimitSeconds: limit,
+            );
+
+            if (!composing && errorStatus == null) {


Hmm, I don't quite understand the purpose of composing. It's said to be "editing compose box active", but this condition seems to be the only place that checks the value of the flag (other than an assert). It feels a bit different from the other test params.

I've added some comments to try to clarify.

PIG208 · 2025-05-07T00:52:38Z

test/widgets/action_sheet_test.dart

+            message: eg.streamMessage(content: 'foo')).toJson());
+          await tester.tap(find.byIcon(ZulipIcons.edit));
+          await tester.pump(); // [MenuItemButton.onPressed] called in a post-frame callback: flutter/flutter@e4a39fa2e
+          await tester.pump(Duration.zero);


nit: tapEditAndPump (when moved out) might be useful here and later

gnprice

Exciting! A few quick comments from an early skim.

gnprice · 2025-05-07T05:02:07Z

lib/widgets/message_list.dart

                  if ((message.reactions?.total ?? 0) > 0)
                    ReactionChipsList(messageId: message.id, reactions: message.reactions!),
-                  if (editStateText != null)
+                  if (editMessageErrorStatusRow != null) editMessageErrorStatusRow


I think this would go well as its own widget:

Suggested change

if (editMessageErrorStatusRow != null) editMessageErrorStatusRow

if (editMessageErrorStatus != null)

_EditMessageStatusRow(messageId: message.id),

That'd help keep this build method mostly clear of details that are specific to a message currently being edited.

gnprice · 2025-05-07T05:02:52Z

lib/widgets/message_list.dart

+      content = Opacity(opacity: 0.6, child: content);
+      switch (editMessageErrorStatus) {
+        case true:
+          content = _withRestoreEditMessageGestureDetector(context, content);


This also seems like a good candidate for its own widget:

Suggested change

content = _withRestoreEditMessageGestureDetector(context, content);

content = _RestoreEditMessageGestureDetector(messageId: item.message.id, child: content);

gnprice · 2025-05-07T05:05:21Z

lib/widgets/message_list.dart

+        final composeBoxState = MessageListPage.ancestorOf(context).composeBoxState;
+        final controller = composeBoxState?.controller;
+        final editMessageInProgress =
+          controller is EditMessageComposeBoxController
+          || editMessageErrorStatus != null;


Can this logic all move into showMessageActionSheet? Seems like between context and message it has all the inputs it needs in order to do the same calculations.

chrisbobbe · 2025-05-10T00:52:49Z

Thanks for the reviews! Revision pushed, still for maintainer review this time (but I've incorporated both reviews in this revision).

PIG208 · 2025-05-10T01:58:10Z

Looks like there are some compilation errors in lib/widgets/message_list.dart, probably from some non-local merge conflicts.

PIG208 · 2025-05-10T02:19:10Z

test/widgets/action_sheet_test.dart

+          // It's impossible for the compose box to be in editing mode
+          // while, for the same message, the message-edit request is either
+          // in progress or in the error state.
+          assert(!composing || errorStatus == null);


Oh I see! Yeah this clears up the meaning of composing.

I think it is still possible if you initiate the edit in both the channel narrow and the topic narrow containing it, then save the edit in the topic narrow, navigate back and save in the channel narrow, on the same message. If the first request takes long enough to finish. But that's quite an edge case, and is guarded by a containsKey check already.

Oh, good find! I can address this edge case in my next revision.

PIG208 · 2025-05-10T02:23:55Z

Thanks for the update! This looks good to me other than the compilation errors.

gnprice

Thanks to you both! Comments below. I haven't yet read the last commit's tests; I've read everything else in the branch.

gnprice · 2025-05-12T17:40:39Z

test/example_data.dart

-    editTimestamp: editTimestamp ?? 1234567890, // TODO generate timestamp
+    editTimestamp: editTimestamp ?? utcTimestamp(),


This is effectively still a constant, right? Just with more computation first.

The TODO here is to generate distinct timestamps for different messages, rather than have all the edits happen in the same second.

There's no rush to resolve that TODO (the fact that all the timestamps are the same hasn't caused a problem yet). But as long as the behavior is still a constant, I think adding this indirection isn't an improvement.

gnprice · 2025-05-12T17:41:54Z

lib/widgets/message_list.dart

@@ -1446,6 +1446,26 @@ class MessageWithPossibleSender extends StatelessWidget {
        child: Icon(ZulipIcons.star_filled, size: 16, color: designVariables.star));
    }

+    Widget content = MessageContent(message: message, content: item.content);


nit: this two-line change can be squashed into the later commit that uses it

msglist [nfc]: Pull out `content` var in MessageWithPossibleSender.build

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

lib/api/model/submessage.dart

+  ///
+  /// This exists because [toJson] would just return [] otherwise.
+  @visibleForTesting
+  List<Submessage>? debugSubmessagesForToJson;


This works, but I wonder if we can do it a bit more cleanly and directly. How about:

@@ -61,12 +61,13 @@ class Submessage { switch (widgetData) { case PollWidgetData(): return Poll.fromSubmessages( widgetData: widgetData, pollEventSubmessages: submessages.skip(1), messageSenderId: messageSenderId, + debugSubmessages: kDebugMode ? submessages : null, );

Then Poll.handleSubmessageEvent could append to debugSubmessages, but only in debug mode.

That way eg.streamMessage and eg.dmMessage wouldn't have to separately set this; it'd be set to the intended value automatically.

This would also make it present in debug builds of the app, as well as in tests. That seems potentially helpful anyway. It's in a similar spirit to ContentNode.debugHtmlNode.

gnprice · 2025-05-12T18:51:11Z

test/widgets/action_sheet_test.dart

+  // because its render box effectively has HitTestBehavior.deferToChild, and
+  // the long-press might land on a child such that it doesn't hit-test as true,
+  // like if it's in padding around a Paragraph.


nit:

Suggested change

// because its render box effectively has HitTestBehavior.deferToChild, and

// the long-press might land on a child such that it doesn't hit-test as true,

// like if it's in padding around a Paragraph.

// because its render box effectively has HitTestBehavior.deferToChild, and

// the long-press might land where no child hit-tests as true,

// like if it's in padding around a Paragraph.

I'd think of that case as not really landing on any child at all — after all, the outcome of the hit-testing is precisely that it didn't hit any child at all.

gnprice · 2025-05-12T19:05:57Z

lib/widgets/action_sheet.dart

+    final composeBoxState = findMessageListPage().composeBoxState;
+    if (composeBoxState == null) {
+      throw StateError('Compose box unexpectedly absent when edit-message button pressed');
+    }
+    final editMessageErrorStatus = store.getEditMessageErrorStatus(message.id);
+    if (editMessageErrorStatus != null) {
+      throw StateError('Message edit already in progress when edit-message button pressed');
+    }


These checks would be good after awaiting the raw content. That's a time when either of them really could fail — e.g. maybe the request took a while, and the user gave up and navigated away.

Then I think we don't actually need them before that await. That request itself doesn't rely on the facts these are checking; the part that does rely on those is when we attempt to start the edit interaction.

gnprice · 2025-05-12T19:59:57Z

lib/widgets/compose_box.dart

+        onPressed: () {
+          composeBoxState.endEditInteraction();
+        }),


nit: can reduce to a tearoff:

Suggested change

onPressed: () {

composeBoxState.endEditInteraction();

}),

onPressed: composeBoxState.endEditInteraction),

gnprice · 2025-05-12T20:01:56Z

lib/widgets/compose_box.dart

+      // TODO(#1481) disabled appearance when there are validation errors
+      ZulipWebUiKitButton(label: zulipLocalizations.composeBoxBannerButtonSave,
+        attention: ZulipWebUiKitButtonAttention.high,
+        onPressed: () {


nit: this callback is about 20 lines; once it's more than a small handful of lines, I think it's a bit easier to read as a separate named function, so here a private method on this class.

gnprice · 2025-05-12T20:08:37Z

lib/widgets/message_list.dart

+      switch (editMessageErrorStatus) {
+        case false:


nit: How about if (!editMessageErrorStatus)?

That'll have basically the same exhaustiveness-checking effect, too — if requires a bool and will reject a bool?.

gnprice · 2025-05-12T20:11:27Z

lib/widgets/message_list.dart

+          // TODO instead place within outer padding; see Figma:
+          //   https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4026-8775&m=dev
+          LinearProgressIndicator(


I don't quite follow this comment. Which padding should this be placed within?

Here are screenshots of the Figma with and without that outer padding highlighted:

The progress indicator sits inside that outer padding.

I think it looks fine as-is and would be OK to remove the TODO, revisiting if needed in the future.

Hmm, those screenshots don't really help. 🙂 How does this version differ from what's in Figma?

Looking into it a bit more, I think you're referring to the 4px of bottom padding on the MessageWithPossibleSender.

So in the Figma design, the indicator encroaches on that padding and gets closer to whatever's below (like the next message, or a date separator). In this version, when the indicator appears, it instead makes the message taller and stays 4px away from the sibling below.

I see the appeal of the version in Figma, but the difference is subtle. Happy to leave it as a TODO.

gnprice · 2025-05-12T20:18:48Z

lib/widgets/message_list.dart

+        composeBoxState.startEditInteraction(messageId: messageId,
+          originalRawContent: store.takeFailedMessageEdit(messageId));


Hmm. This originalRawContent value is the right value to initialize the text input to — but it's not the right value to use when sending the resulting edit request to the server. For that, we want the original original raw content, because that's the value the user is (implicitly) expecting the server will have.

Probably the right fix is to add another field on _EditMessageRequestStatus.

I see; yeah, thanks for catching this.

I've had some more thoughts prompted by this—will share later today

chrisbobbe · 2025-05-15T22:31:39Z

Thanks for the review! Revision pushed, PTAL.

As discussed in the office on Friday, this makes some improvements over the last revision—but omits the refetch-raw-content request on restoring a failed edit, which I'd been considering. 🙂 (Omits it because the edge case it addresses is minuscule, and it adds time and risk of failure in bad network connections.)

In particular:

Now if you tap to restore a failed edit but there's already text in the compose box, and you cancel the "Discard…?" confirmation dialog, the failed edit hasn't been consumed.
The edit-message compose box has a disabled "Preparing…" state for when the fetch-raw-content request is in progress (which happens just when you start an edit from the action sheet).

Here are screenshots of that "Preparing…" state, with the normal state for comparison:

Dark	Light

Giving the "Save" button a disabled appearance, in this state, is #1481; we don't have a design for that.

gnprice

Thanks for the revision! This behavior all now looks good, except one glitch mentioned below which I'm fine with for launch.

I haven't yet read the tests in the last/main commit.

gnprice · 2025-05-16T02:20:55Z

test/widgets/compose_box_test.dart

-      await tester.pump();
+      await tester.pumpAndSettle();


Can this be reduced to e.g. two pumps? (Perhaps one of them with a duration.)

gnprice · 2025-05-16T02:23:33Z

lib/api/model/submessage.dart

  }) {
    for (int index = 0; index < options.length; index += 1) {
      // Initial poll options use a placeholder senderId.
      // See [PollEventSubmessage.optionKey] for details.
      _addOption(senderId: null, idx: index, option: options[index]);
    }
+    _debugSubmessages = debugSubmessages;


Let's put this inside an assert (or use kDebugMode). That way we can be confident it's clear to the optimizer, in release builds, that this field is always a constant null and so can be left out of the class entirely.

gnprice · 2025-05-16T02:40:17Z

lib/widgets/compose_box.dart

+    final editMessageErrorStatus = store.getEditMessageErrorStatus(messageId);
+    switch (editMessageErrorStatus) {


nit: simpler inlined

gnprice · 2025-05-16T02:45:28Z

lib/widgets/compose_box.dart

+    final confirmationDialogResult = await _discardDraftConfirmationDialog();
+    if (!confirmationDialogResult) return;


This is a bit hard to understand — partly because it's often/usually not actually the result of any dialog, and also because it's not obvious what the boolean result of the dialog means.

I think a rename would do it. Perhaps flipping the sense of the bool if that makes the naming easier: _canDiscardDraft?

gnprice · 2025-05-16T02:46:42Z

lib/widgets/compose_box.dart

+    if (!confirmationDialogResult) return;
+    if (!mounted) return;
+
+    PerAccountStore store = PerAccountStoreWidget.of(context);


nit: final?

gnprice · 2025-05-16T03:09:38Z

lib/widgets/compose_box.dart

+    if (fetchedRawContent == null) {
+      // Fetch-raw-content failed; abort the edit session.
+      // An error dialog was already shown, by fetchRawContentWithFeedback.


It's regrettable that this all leads to so many lines of code (over 100) within _ComposeBoxState, which is also about composing new messages with no editing of existing messages involved.

But this logic does have a lot of spots where it's setting the _controller field on this state, either to an edit-message controller or back to a normal new-message controller. So I don't see an easy way it could be cleanly moved off to another class. Oh well, then.

gnprice · 2025-05-16T03:14:52Z

lib/widgets/compose_box.dart

+      final validationErrorMessages =
+        controller.content.validationErrors.map((error) =>
+          error.message(zulipLocalizations)).toList();
+      showErrorDialog(context: context,
+        title: zulipLocalizations.errorMessageEditNotSaved,
+        message: validationErrorMessages.join('\n\n'));


nit: no need to materialize List before .join

Suggested change

final validationErrorMessages =

controller.content.validationErrors.map((error) =>

error.message(zulipLocalizations)).toList();

showErrorDialog(context: context,

title: zulipLocalizations.errorMessageEditNotSaved,

message: validationErrorMessages.join('\n\n'));

final validationErrorMessages =

controller.content.validationErrors.map((error) =>

error.message(zulipLocalizations));

showErrorDialog(context: context,

title: zulipLocalizations.errorMessageEditNotSaved,

message: validationErrorMessages.join('\n\n'));

gnprice · 2025-05-16T03:16:02Z

lib/widgets/compose_box.dart

+    ]);
+  }
+
+  void _handleTapSave (BuildContext context) {


nit: callback above build and other build methods

gnprice · 2025-05-16T04:40:19Z

lib/widgets/compose_box.dart

+      (_controller as EditMessageComposeBoxController)
+        ..originalRawContent = fetchedRawContent
+        ..content.value = TextEditingValue(text: fetchedRawContent)
+        ..contentFocusNode.requestFocus();


This step doesn't seem to work for me empirically (on my Android 15 phone). When I hit "Edit message" on the message action sheet, the input doesn't get focused — I have to tap it to focus (and to cause the keyboard to appear).

OTOH the corresponding step in the _byRestoringFailedEdit case does work.

Do you reproduce?

This is a perfectly acceptable glitch for launch: it just means an extra tap, and I think users are accustomed to having to sometimes tap an input field to get the keyboard to open when it seems like it should happen automatically. So let's only spend a small amount of effort now on diagnosing or fixing, and if that isn't enough then just file a short issue to record the point for post-launch.

Debugged and fixed! 🎉

(FTR, the fix was the addPostFrameCallback here, for the reason its comment explains:

setState(() { // setState to refresh the input, upload buttons, etc. // out of the disabled "Preparing…" state. editMessageController.originalRawContent = fetchedRawContent; }); editMessageController.content.value = TextEditingValue(text: fetchedRawContent); SchedulerBinding.instance.addPostFrameCallback((_) { // post-frame callback so this happens after the input is enabled editMessageController.contentFocusNode.requestFocus(); });

)

gnprice · 2025-05-16T04:48:16Z

lib/widgets/compose_box.dart

+        // This can happen if you start an edit interaction on one
+        // MessageListPage and then do an edit on a different MessageListPage,
+        // and the second edit is still saving when you return to the first.
+        //
+        // Abort rather than sending a request with a prevContentSha256
+        // that the server might not accept, and don't clear the compose
+        // box, so the user can try again after the request settles.
+        // TODO write a test for this
+        showErrorDialog(context: context,


Yeah, I'm OK with skipping a test for this case.

When we set up the message list in tests, we do it by preparing the API response for a message fetch or new-message event, via JSON. But {Stream,Dm}Message.toJson drops poll data on the floor, which defeats setting up a poll-style message in the message list. Until now, anyway, with this workaround. A reference in a dartdoc to something called `prepareMessageWithSubmessages` was dangling; it seemed to be dangling when it first appeared, too, in 5af5c76; there's nothing by that name.

We're about to add a test that renders a poll-style message, and we'd get this warning on that.

…param

…null

Greg points out that we'll need both values when restoring a failed message edit: originalRawContent for the eventual edit-message request (for prevContentSha256), and newContent to fill the edit-message compose box, so the user can restore the edit session to what it was before it failed.

In the upcoming edit-message feature, the edit-message compose box will have a "Preparing..." state after you tap "Edit message" in the action sheet, while we're fetching the raw message content. The edit-message compose box shouldn't be interactable in this state.

Fixes: zulip#126

chrisbobbe · 2025-05-16T23:56:46Z

Thanks for the review! Revision pushed.

gnprice

Thanks! Glad that glitch turned out to have a nice clean fix.

All looks good now except one nit — oh and I should read the tests 🙂

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

lib/widgets/compose_box.dart

+      case null:
+        _editFromRawContentFetch(messageId);


bump #1498 (comment): put the two non-error cases next to each other

Ah! I put null and false together as the "two non-error cases", thinking of "error" as it's used in getEditMessageErrorStatus which produces this value:

@override bool? getEditMessageErrorStatus(int messageId) => _editMessageRequests[messageId]?.hasError;

But I see now what you mean, that the error case is the one where we show an error dialog. 😛

gnprice · 2025-05-17T00:24:05Z

lib/widgets/compose_box.dart

+      (_controller as EditMessageComposeBoxController)
+        ..originalRawContent = fetchedRawContent
+        ..content.value = TextEditingValue(text: fetchedRawContent)
+        ..contentFocusNode.requestFocus();


(FTR, the fix was the addPostFrameCallback here, for the reason its comment explains:

setState(() { // setState to refresh the input, upload buttons, etc. // out of the disabled "Preparing…" state. editMessageController.originalRawContent = fetchedRawContent; }); editMessageController.content.value = TextEditingValue(text: fetchedRawContent); SchedulerBinding.instance.addPostFrameCallback((_) { // post-frame callback so this happens after the input is enabled editMessageController.contentFocusNode.requestFocus(); });

)

gnprice

OK, and here's a review of the remaining portion: the tests. They look good! Only small comments below.

These are definitely thorough. 😅 It's good to know that we can write such thorough tests of complex UI flows when we need to, and that we can do so while keeping them readable as these tests are. In general, though, I think our balance should be more in the direction of completing features faster, by spending less effort on comprehensive tests, than this represents.

One other aspect of these tests is that they have a noticeable runtime cost when running this test file. Here's timings on my office desktop (which is somewhat less speedy than my home desktop) with and without the edit message test group (knocking it out by inserting a return;):

$ time flutter test test/widgets/compose_box_test.dart 
00:17 +116: All tests passed!                                                              

time: 19.781s wall (25.325s u, 3.213s s)


$ time flutter test test/widgets/compose_box_test.dart 
00:10 +92: All tests passed!                                                               

time: 13.379s wall (16.966s u, 2.759s s)

So the 24 new test cases add about 6 seconds, or 50%, to the time to run the test file. Hardly a catastrophe, but enough to be a noticeable drag in the future when working on unrelated tests in this file (or unrelated changes to the compose box) and so repeatedly running this file.

I think one contributor to that runtime is that there's the beginnings of a combinatorial explosion: each test scenario gets run under three different narrows, including for interactions that seem unlikely to behave differently depending on the narrow.

Are there any of these where you could delete the doTestFoo calls for some of the narrows and it'd feel like all the same codepaths are effectively tested? If so, it'd be worth doing that.

If not, then the next point might be too much change to be worth doing before merging this, but would be good to have in mind for future tests: some of the additional checks like these, within some of the doTestFoo functions:

        // Now that we have the raw content, check the input is interactive
        // but no typing notifications are sent…
        // […]
        await enterContent(tester, 'some new content');
        // […]

        // …and the upload buttons work.
        // […]
        await tester.tap(find.byIcon(ZulipIcons.attach_file), warnIfMissed: false);

seem like they'd be just as effective if they were run for only one narrow, and only one value of other parameters like start. Separating them out is basically a form of making test cases more focused; and among the benefits of that sort of separation is that it can help a cluster of test cases end up doing N + M work, instead of N * M, while still effectively covering all the logic.

gnprice · 2025-05-17T00:37:28Z

test/widgets/action_sheet_test.dart

+            if (!boxInEditMode && errorStatus == null) {
+              checkButtonIsPresent(expected);
+              return;
+            }


This structure took me a bit to get. Perhaps be more explicit about it earlier:

Suggested change

if (!boxInEditMode && errorStatus == null) {

checkButtonIsPresent(expected);

return;

}

if (!boxInEditMode && errorStatus == null) {

// The state we're testing is present on the original action sheet.

checkButtonIsPresent(expected);

return;

}

// The state we're testing requires a previous "edit message" action

// in order to set up. Use the first action sheet for that setup step.

gnprice · 2025-05-17T00:40:47Z

test/widgets/action_sheet_test.dart

+              // We're testing the request-failed state. Prepare a failure
+              // and tap Save.
+              connection.prepare(
+                json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1));


This comment and code don't seem to match.

Eep, thanks!

gnprice · 2025-05-17T00:41:35Z

test/widgets/action_sheet_test.dart

+            check(find.byType(BottomSheet)).findsOne();
+            checkButtonIsPresent(expected);
+
+            await tester.pump(Duration(milliseconds: 500));


What's this delay do?

It seems like not enough time to expire the timer in the errorStatus true case.

gnprice · 2025-05-17T00:43:31Z

test/widgets/action_sheet_test.dart

+          check(findComposeBoxController(tester))
+            .isNotNull()
+            .isA<FixedDestinationComposeBoxController>();


nit: redundancy

Suggested change

check(findComposeBoxController(tester))

.isNotNull()

.isA<FixedDestinationComposeBoxController>();

check(findComposeBoxController(tester))

.isA<FixedDestinationComposeBoxController>();

gnprice · 2025-05-17T00:53:12Z

test/widgets/compose_box_test.dart

+    }
+
+    void doSmokeTest({required Narrow narrow, required _EditInteractionStart start}) {
+      final description = 'smoke: ${[narrow.toString(), start.message()].join(', ')}';


Suggested change

final description = 'smoke: ${[narrow.toString(), start.message()].join(', ')}';

final description = 'smoke: $narrow, ${start.message()}';

That's equivalent, right?

gnprice · 2025-05-17T00:53:46Z

test/widgets/compose_box_test.dart

+      final description = 'smoke: ${[narrow.toString(), start.message()].join(', ')}';
+      testWidgets(description, (tester) async {


nit: then can also inline description

gnprice · 2025-05-17T00:56:53Z

test/widgets/compose_box_test.dart

+    }
+
+
+    /// Starts an interaction from the action sheet's 'Edit message' button.


nit: double blank line

gnprice · 2025-05-17T01:03:38Z

test/widgets/compose_box_test.dart

+    // Test the "Discard…?" confirmation dialog when there's text in the compose
+    // box for a new message, but you've said you want to edit a message.
+    void doTestInterruptComposingFromFailedEdit({required Narrow narrow}) {


This comment doesn't seem to match the name. (The comment seems to say the same thing as the comment on doTestInterruptComposingFromActionSheet above.)

gnprice · 2025-05-17T01:22:28Z

test/widgets/compose_box_test.dart

+        check(connection.takeRequests()).deepEquals([
+          (Subject<Object?> it) => it.isA<http.Request>()


nit: this sort of deepEquals argument is a bit shorter using Condition:

Suggested change

check(connection.takeRequests()).deepEquals([

(Subject<Object?> it) => it.isA<http.Request>()

check(connection.takeRequests()).deepEquals(<Condition<Object?>>[

(it) => it.isA<http.Request>()

gnprice · 2025-05-17T01:28:05Z

test/widgets/compose_box_test.dart

+        checkNotInEditingMode(tester, narrow: narrow);
+      });
+    }
+    doTestInterruptComposingFromFailedEdit(narrow: channelNarrow);


nit: for helpers that define a test case like test and testWidgets do, name them testFoo to match those:

Suggested change

doTestInterruptComposingFromFailedEdit(narrow: channelNarrow);

testInterruptComposingFromFailedEdit(narrow: channelNarrow);

We're mostly consistent about that practice already; for examples, see git grep -P '\btest[^W]\w+\('. (Or refine the pattern to possibly find more examples; it's excluding testWidgets but crudely so.)

chrisbobbe changed the title ~~compose: Support editing an already-sent message~~ Edit-message (8/n): Implement edit-message! May 4, 2025

chrisbobbe force-pushed the pr-edit-message-n branch from 212ba0b to c23b526 Compare May 6, 2025 05:47

chrisbobbe marked this pull request as ready for review May 6, 2025 05:47

chrisbobbe requested a review from PIG208 May 6, 2025 05:47

chrisbobbe added the a-compose Compose box, autocomplete, attaching files/images label May 6, 2025

chrisbobbe assigned PIG208 May 6, 2025

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label May 6, 2025

chrisbobbe force-pushed the pr-edit-message-n branch from c23b526 to 0bf8739 Compare May 6, 2025 22:27

chrisbobbe force-pushed the pr-edit-message-n branch from 0bf8739 to 19b96de Compare May 6, 2025 23:04

PIG208 reviewed May 7, 2025

View reviewed changes

gnprice reviewed May 7, 2025

View reviewed changes

PIG208 mentioned this pull request May 7, 2025

Support saved snippets #1391

Open

3 tasks

chrisbobbe force-pushed the pr-edit-message-n branch 2 times, most recently from d519086 to 9953ac1 Compare May 10, 2025 00:51

PIG208 reviewed May 10, 2025

View reviewed changes

PIG208 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 12, 2025

PIG208 assigned gnprice and unassigned PIG208 May 12, 2025

PIG208 requested a review from gnprice May 12, 2025 17:01

gnprice reviewed May 12, 2025

View reviewed changes

gnprice mentioned this pull request May 12, 2025

local echo (4.5/n): Prep for creating outbox messages on send #1509

Merged

chrisbobbe force-pushed the pr-edit-message-n branch from 9953ac1 to db0a671 Compare May 15, 2025 22:22

gnprice reviewed May 16, 2025

View reviewed changes

gnprice mentioned this pull request May 16, 2025

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

Open

chrisbobbe added 12 commits May 16, 2025 16:03

compose_box test: Use full MessageListPage in compose box widget tests

131a4e9

msglist [nfc]: s/localizations/zulipLocalizations/

9e27a11

compose_box test [nfc]: Pull out sendButtonFinder helper

bf38693

dialog: Sweep through error dialogs, giving message final punctuation

c1fbbe5

action_sheet test: Silence a noisy warning in an upcoming new test

18fee48

We're about to add a test that renders a poll-style message, and we'd get this warning on that.

compose [nfc]: Give Compose{Content,Topic}Controller a starting-text …

c7863c0

…param

test: eg.initialSnapshot.realmMessageContentEditLimitSeconds default …

ba65dc5

…null

test: Add FakeApiConnection.clearPreparedResponses

99c530a

compose: Support editing an already-sent message

44a1df7

Fixes: zulip#126

chrisbobbe force-pushed the pr-edit-message-n branch from db0a671 to 44a1df7 Compare May 16, 2025 23:56

gnprice reviewed May 17, 2025

View reviewed changes

	Widget? buildTrailing(context) {
	Widget buildTrailing(context) {

	if (editMessageErrorStatusRow != null) editMessageErrorStatusRow
	if (editMessageErrorStatus != null)
	_EditMessageStatusRow(messageId: message.id),

	content = _withRestoreEditMessageGestureDetector(context, content);
	content = _RestoreEditMessageGestureDetector(messageId: item.message.id, child: content);

		editTimestamp: editTimestamp ?? 1234567890, // TODO generate timestamp
		editTimestamp: editTimestamp ?? utcTimestamp(),

		composeBoxState.startEditInteraction(messageId: messageId,
		originalRawContent: store.takeFailedMessageEdit(messageId));

		final editMessageErrorStatus = store.getEditMessageErrorStatus(messageId);
		switch (editMessageErrorStatus) {

		final confirmationDialogResult = await _discardDraftConfirmationDialog();
		if (!confirmationDialogResult) return;

	final description = 'smoke: ${[narrow.toString(), start.message()].join(', ')}';
	final description = 'smoke: $narrow, ${start.message()}';

		final description = 'smoke: ${[narrow.toString(), start.message()].join(', ')}';
		testWidgets(description, (tester) async {

		}


		/// Starts an interaction from the action sheet's 'Edit message' button.

		check(connection.takeRequests()).deepEquals([
		(Subject<Object?> it) => it.isA<http.Request>()

	doTestInterruptComposingFromFailedEdit(narrow: channelNarrow);
	testInterruptComposingFromFailedEdit(narrow: channelNarrow);

Edit-message (8/n): Implement edit-message! #1498

Are you sure you want to change the base?

Edit-message (8/n): Implement edit-message! #1498

Conversation

chrisbobbe commented May 3, 2025 • edited Loading

Screenshots

chrisbobbe commented May 5, 2025 • edited Loading

alya commented May 6, 2025 • edited Loading

alya commented May 6, 2025

chrisbobbe commented May 6, 2025

chrisbobbe commented May 6, 2025

chrisbobbe commented May 6, 2025

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe May 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe May 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented May 10, 2025 • edited Loading

PIG208 commented May 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented May 10, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented May 15, 2025 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented May 16, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented May 3, 2025 •

edited

Loading

chrisbobbe commented May 5, 2025 •

edited

Loading

alya commented May 6, 2025 •

edited

Loading

chrisbobbe May 9, 2025 •

edited

Loading

chrisbobbe May 9, 2025 •

edited

Loading

chrisbobbe commented May 10, 2025 •

edited

Loading

chrisbobbe commented May 15, 2025 •

edited

Loading