edit-message: Show error dialog on edit-message request error

lib/model/message.dart

@@ -53,15 +53,18 @@ mixin MessageStore {
   /// and the update-message event hasn't arrived.
   bool? getEditMessageErrorStatus(int messageId);
 
-  /// Edit a message's content, via a request to the server.
+  /// Makes an edit-message request and starts an edit-outbox lifecycle.
   ///
   /// Should only be called when there is no current edit request for [messageId],
   /// i.e., [getEditMessageErrorStatus] returns null for [messageId].
   ///
+  /// The returned [Future] resolves or rejects with the edit-message request,
+  /// irrespective of when the edit-message event arrives (if it does).
+  ///
   /// See also:
   ///   * [getEditMessageErrorStatus]
   ///   * [takeFailedMessageEdit]
-  void editMessage({
+  Future<void> editMessage({
     required int messageId,
     required String originalRawContent,
     required String newContent,
@@ -104,7 +107,7 @@ mixin ProxyMessageStore on MessageStore {
     return messageStore.getEditMessageErrorStatus(messageId);
   }
   @override
-  void editMessage({
+  Future<void> editMessage({
     required int messageId,
     required String originalRawContent,
     required String newContent,
@@ -315,7 +318,7 @@ class MessageStoreImpl extends HasRealmStore with MessageStore, _OutboxMessageSt
   final Map<int, _EditMessageRequestStatus> _editMessageRequests = {};
 
   @override
-  void editMessage({
+  Future<void> editMessage({
     required int messageId,
     required String originalRawContent,
     required String newContent,
@@ -349,6 +352,7 @@ class MessageStoreImpl extends HasRealmStore with MessageStore, _OutboxMessageSt
       }
       status.hasError = true;
       _notifyMessageListViewsForOneMessage(messageId);
+      rethrow;
     }
   }
 

lib/widgets/compose_box.dart

@@ -23,6 +23,7 @@ import 'color.dart';
 import 'dialog.dart';
 import 'icons.dart';
 import 'inset_shadow.dart';
+import 'page.dart';
 import 'store.dart';
 import 'text.dart';
 import 'theme.dart';
@@ -1659,6 +1660,9 @@ class EditMessageComposeBoxController extends ComposeBoxController {
   String? originalRawContent;
 }
 
+/// A banner to display over or instead of interactive compose-box content.
+///
+/// Must have a [PageRoot] ancestor.
 abstract class _Banner extends StatelessWidget {
   const _Banner();
 
@@ -1675,7 +1679,11 @@ abstract class _Banner extends StatelessWidget {
   ///   https://github.com/zulip/zulip-flutter/pull/1432#discussion_r2023907300
   ///
   /// To control the element's distance from the end edge, override [padEnd].
-  Widget? buildTrailing(BuildContext context);
+  ///
+  /// The passed [BuildContext] will be the result of [PageRoot.contextOf],
+  /// so it's expected to remain mounted until the whole page disappears,
+  /// which may be long after the banner disappears.
+  Widget? buildTrailing(BuildContext pageContext);
 
   /// Whether to apply `end: 8` in [SafeArea.minimum].
   ///
@@ -1694,7 +1702,7 @@ abstract class _Banner extends StatelessWidget {
       color: getLabelColor(designVariables),
     ).merge(weightVariableTextStyle(context, wght: 600));
 
-    final trailing = buildTrailing(context);
+    final trailing = buildTrailing(PageRoot.contextOf(context));
     return DecoratedBox(
       decoration: BoxDecoration(
         color: getBackgroundColor(designVariables)),
@@ -1740,7 +1748,7 @@ class _ErrorBanner extends _Banner {
     designVariables.bannerBgIntDanger;
 
   @override
-  Widget? buildTrailing(context) {
+  Widget? buildTrailing(pageContext) {
     // An "x" button can go here.
     // 24px square with 8px touchable padding in all directions?
     // and `bool get padEnd => false`; see Figma:
@@ -1766,17 +1774,17 @@ class _EditMessageBanner extends _Banner {
   Color getBackgroundColor(DesignVariables designVariables) =>
     designVariables.bannerBgIntInfo;
 
-  void _handleTapSave (BuildContext context) {
-    final store = PerAccountStoreWidget.of(context);
+  void _handleTapSave (BuildContext pageContext) async {
+    final store = PerAccountStoreWidget.of(pageContext);
     final controller = composeBoxState.controller;
     if (controller is! EditMessageComposeBoxController) return; // TODO(log)
-    final zulipLocalizations = ZulipLocalizations.of(context);
+    final zulipLocalizations = ZulipLocalizations.of(pageContext);
 
     if (controller.content.hasValidationErrors.value) {
       final validationErrorMessages =
         controller.content.validationErrors.map((error) =>
           error.message(zulipLocalizations));
-      showErrorDialog(context: context,
+      showErrorDialog(context: pageContext,
         title: zulipLocalizations.errorMessageEditNotSaved,
         message: validationErrorMessages.join('\n\n'));
       return;
@@ -1789,24 +1797,40 @@ class _EditMessageBanner extends _Banner {
       return;
     }
 
-    store.editMessage(
-      messageId: controller.messageId,
-      originalRawContent: originalRawContent,
-      newContent: controller.content.textNormalized);
+    final messageId = controller.messageId;
+    final newContent = controller.content.textNormalized;
     composeBoxState.endEditInteraction();
+
+    try {
+      await store.editMessage(
+        messageId: messageId,
+        originalRawContent: originalRawContent,
+        newContent: newContent);
+    } on ApiRequestException catch (e) {
+      if (!pageContext.mounted) return;
+      final zulipLocalizations = ZulipLocalizations.of(pageContext);
+      final message = switch (e) {
+        ZulipApiException() => zulipLocalizations.errorServerMessage(e.message),
+        _ => e.message,
+      };
+      showErrorDialog(context: pageContext,
+        title: zulipLocalizations.errorMessageEditNotSaved,
+        message: message);
+      return;
+    }
   }
 
   @override
-  Widget buildTrailing(context) {
-    final zulipLocalizations = ZulipLocalizations.of(context);
+  Widget buildTrailing(pageContext) {
+    final zulipLocalizations = ZulipLocalizations.of(pageContext);
     return Row(mainAxisSize: MainAxisSize.min, spacing: 8, children: [
       ZulipWebUiKitButton(label: zulipLocalizations.composeBoxBannerButtonCancel,
         onPressed: composeBoxState.endEditInteraction),
       // TODO(#1481) disabled appearance when there are validation errors
       //   or the original raw content hasn't loaded yet
       ZulipWebUiKitButton(label: zulipLocalizations.composeBoxBannerButtonSave,
         attention: ZulipWebUiKitButtonAttention.high,
-        onPressed: () => _handleTapSave(context)),
+        onPressed: () => _handleTapSave(pageContext)),
     ]);
   }
 }

test/model/message_test.dart

@@ -7,6 +7,7 @@ import 'package:crypto/crypto.dart';
 import 'package:fake_async/fake_async.dart';
 import 'package:http/http.dart' as http;
 import 'package:test/scaffolding.dart';
+import 'package:zulip/api/exception.dart';
 import 'package:zulip/api/model/events.dart';
 import 'package:zulip/api/model/model.dart';
 import 'package:zulip/api/model/submessage.dart';
@@ -603,8 +604,8 @@ void main() {
 
       connection.prepare(
         json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1));
-      store.editMessage(messageId: message.id,
-        originalRawContent: 'old content', newContent: 'new content');
+      unawaited(store.editMessage(messageId: message.id,
+        originalRawContent: 'old content', newContent: 'new content'));
       checkRequest(message.id,
         prevContent: 'old content',
         content: 'new content');
@@ -634,8 +635,8 @@ void main() {
 
       connection.prepare(
         json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1));
-      store.editMessage(messageId: message.id,
-        originalRawContent: 'old content', newContent: 'new content');
+      unawaited(store.editMessage(messageId: message.id,
+        originalRawContent: 'old content', newContent: 'new content'));
       checkRequest(message.id,
         prevContent: 'old content',
         content: 'new content');
@@ -647,8 +648,8 @@ void main() {
       check(store.getEditMessageErrorStatus(otherMessage.id)).isNull();
       connection.prepare(
         json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1));
-      store.editMessage(messageId: otherMessage.id,
-        originalRawContent: 'other message old content', newContent: 'other message new content');
+      unawaited(store.editMessage(messageId: otherMessage.id,
+        originalRawContent: 'other message old content', newContent: 'other message new content'));
       checkRequest(otherMessage.id,
         prevContent: 'other message old content',
         content: 'other message new content');
@@ -682,8 +683,8 @@ void main() {
       check(store.getEditMessageErrorStatus(message.id)).isNull();
 
       connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1));
-      store.editMessage(messageId: message.id,
-        originalRawContent: 'old content', newContent: 'new content');
+      unawaited(check(store.editMessage(messageId: message.id,
+        originalRawContent: 'old content', newContent: 'new content')).throws<ZulipApiException>());
       checkNotifiedOnce();
       async.elapse(Duration(seconds: 1));
       check(store.getEditMessageErrorStatus(message.id)).isNotNull().isTrue();
@@ -695,8 +696,8 @@ void main() {
       check(store.getEditMessageErrorStatus(message.id)).isNull();
 
       connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1));
-      store.editMessage(messageId: message.id,
-        originalRawContent: 'old content', newContent: 'new content');
+      unawaited(check(store.editMessage(messageId: message.id,
+        originalRawContent: 'old content', newContent: 'new content')).throws<ZulipApiException>());
       checkNotifiedOnce();
       async.elapse(Duration(seconds: 1));
       check(store.getEditMessageErrorStatus(message.id)).isNotNull().isTrue();
@@ -718,8 +719,8 @@ void main() {
 
       connection.prepare(
         json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1));
-      store.editMessage(messageId: message.id,
-        originalRawContent: 'old content', newContent: 'new content');
+      unawaited(store.editMessage(messageId: message.id,
+        originalRawContent: 'old content', newContent: 'new content'));
       async.elapse(Duration(milliseconds: 500));
       check(connection.takeRequests()).length.equals(1);
       checkNotifiedOnce();
@@ -738,8 +739,8 @@ void main() {
 
       connection.prepare(
         httpException: const SocketException('failed'), delay: Duration(seconds: 1));
-      store.editMessage(messageId: message.id,
-        originalRawContent: 'old content', newContent: 'new content');
+      unawaited(store.editMessage(messageId: message.id,
+        originalRawContent: 'old content', newContent: 'new content'));
       checkNotifiedOnce();
 
       async.elapse(Duration(milliseconds: 500));
@@ -760,8 +761,8 @@ void main() {
 
       connection.prepare(
         httpException: const SocketException('failed'), delay: Duration(seconds: 1));
-      store.editMessage(messageId: message.id,
-        originalRawContent: 'old content', newContent: 'new content');
+      unawaited(check(store.editMessage(messageId: message.id,
+        originalRawContent: 'old content', newContent: 'new content')).throws<NetworkException>());
       checkNotifiedOnce();
 
       async.elapse(Duration(seconds: 1));
@@ -781,8 +782,8 @@ void main() {
 
       connection.prepare(
         httpException: const SocketException('failed'), delay: Duration(seconds: 1));
-      store.editMessage(messageId: message.id,
-        originalRawContent: 'old content', newContent: 'new content');
+      unawaited(check(store.editMessage(messageId: message.id,
+        originalRawContent: 'old content', newContent: 'new content')).throws<NetworkException>());
       checkNotifiedOnce();
 
       async.elapse(Duration(seconds: 1));
@@ -801,8 +802,8 @@ void main() {
       check(store.getEditMessageErrorStatus(message.id)).isNull();
 
       connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1));
-      store.editMessage(messageId: message.id,
-        originalRawContent: 'old content', newContent: 'new content');
+      unawaited(check(store.editMessage(messageId: message.id,
+        originalRawContent: 'old content', newContent: 'new content')).throws<ZulipApiException>());
       checkNotifiedOnce();
       async.elapse(Duration(seconds: 1));
       check(store.getEditMessageErrorStatus(message.id)).isNotNull().isTrue();
@@ -818,8 +819,8 @@ void main() {
       check(store.getEditMessageErrorStatus(message.id)).isNull();
 
       connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1));
-      store.editMessage(messageId: message.id,
-        originalRawContent: 'old content', newContent: 'new content');
+      unawaited(store.editMessage(messageId: message.id,
+        originalRawContent: 'old content', newContent: 'new content'));
       checkNotifiedOnce();
 
       async.elapse(Duration(milliseconds: 500));
@@ -843,8 +844,8 @@ void main() {
 
       connection.prepare(
         json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1));
-      store.editMessage(messageId: message.id,
-        originalRawContent: 'old content', newContent: 'new content');
+      unawaited(store.editMessage(messageId: message.id,
+        originalRawContent: 'old content', newContent: 'new content'));
       checkNotifiedOnce();
 
       async.elapse(Duration(milliseconds: 500));

test/widgets/action_sheet_test.dart

@@ -1780,6 +1780,12 @@ void main() {
         await tester.pump(); // [MenuItemButton.onPressed] called in a post-frame callback: flutter/flutter@e4a39fa2e
       }
 
+      Future<void> takeErrorDialogAndPump(WidgetTester tester) async {
+        final errorDialog = checkErrorDialog(tester, expectedTitle: 'Message not saved');
+        await tester.tap(find.byWidget(errorDialog));
+        await tester.pump();
+      }
+
       group('present/absent appropriately', () {
         /// Test whether the edit-message button is visible, given params.
         ///
@@ -1870,6 +1876,7 @@ void main() {
               connection.prepare(apiException: eg.apiBadRequest());
               await tester.tap(find.widgetWithText(ZulipWebUiKitButton, 'Save'));
               await tester.pump(Duration.zero);
+              await takeErrorDialogAndPump(tester);
             } else if (errorStatus == false) {
               // We're testing the request-in-progress state. Prepare a delay,
               // tap Save, and wait through only part of the delay.

test/widgets/compose_box_test.dart

@@ -1748,6 +1748,12 @@ void main() {
       await tester.pump(); // message list updates
     }
 
+    Future<void> takeErrorDialogAndPump(WidgetTester tester) async {
+      final errorDialog = checkErrorDialog(tester, expectedTitle: 'Message not saved');
+      await tester.tap(find.byWidget(errorDialog));
+      await tester.pump();
+    }
+
     /// Check that the compose box is in the "Preparing…" state,
     /// awaiting the fetch-raw-content request.
     Future<void> checkAwaitingRawMessageContent(WidgetTester tester) async {
@@ -1770,6 +1776,7 @@ void main() {
       await tester.tap(
         find.widgetWithText(ZulipWebUiKitButton, 'Save'), warnIfMissed: false);
       await tester.pump(Duration.zero);
+      checkNoDialog(tester);
       check(connection.lastRequest).equals(lastRequest);
     }
 
@@ -1788,6 +1795,7 @@ void main() {
       connection.prepare(apiException: eg.apiBadRequest());
       await tester.tap(find.widgetWithText(ZulipWebUiKitButton, 'Save'));
       await tester.pump(Duration.zero);
+      await takeErrorDialogAndPump(tester);
       await tester.tap(find.text('EDIT NOT SAVED'));
       await tester.pump();
       connection.takeRequests();
@@ -1966,6 +1974,7 @@ void main() {
         await tester.tap(find.widgetWithText(ZulipWebUiKitButton, 'Save'));
         connection.takeRequests();
         await tester.pump(Duration.zero);
+        await takeErrorDialogAndPump(tester);
         checkNotInEditingMode(tester, narrow: narrow);
         check(find.text('EDIT NOT SAVED')).findsOne();