Edit-message (7/n): Implement edit-message methods on MessageStore #1484

chrisbobbe · 2025-04-24T03:12:30Z

This PR, toward the edit-message feature #126, implements three new methods on MessageStore:

editMessage, to be called when the user taps the "Edit message" button in the message action sheet:

getEditMessageErrorStatus, to be called when rendering the message list; showing a loading state when we haven't gotten the update-message event yet, and an error state for a failed edit request:

(The UI text isn't final; see updates on CZO: #mobile-design > edit message @ 💬)

takeFailedMessageEdit, to be called when you tap the error message pictured above, so we can put you back in the edit box, starting with the text you'd tried to edit to. This part is from CZO discussion: #mobile-design > edit message @ 💬

Related: #126

chrisbobbe · 2025-04-24T03:12:47Z

~~Marking as a draft because I still have a TODO for some tests about delete-message events.~~ Done; ready for review!

chrisbobbe · 2025-04-25T22:21:17Z

Revision pushed, this time putting the tests in test/model/message_test.dart instead of test/model/store_test.dart (but still testing against the whole PerAccountStore API, not the MessageStoreImpl substore, following the reasoning in ec9aa35).

PIG208

Thanks! Left some comments.

PIG208 · 2025-04-28T18:35:16Z

lib/model/message.dart

+  String? takeFailedMessageEdit(int messageId) {
+    final status = _editMessageRequests.remove(messageId);
+    if (status == null) {
+      throw StateError('called takeFailedEdit, but no edit');


nit: this doesn't match the name of the method: takeFailedMessageEdit

PIG208 · 2025-04-28T18:48:21Z

test/model/message_test.dart

+      // Request has succeeded; event hasn't arrived
+      check(store.getEditMessageErrorStatus(message.id)).isNotNull().isFalse();


I feel that we can be a bit more certain that the request has complete by awaiting its future. But I guess editMessage doesn't return it because the UI code doesn't need the future.

Yeah, there's nothing user-facing to do when the future succeeds (we take the event as signaling edit-message success), so editMessage doesn't return it.

PIG208 · 2025-04-28T18:55:50Z

lib/model/message.dart

+      await updateMessage(connection, messageId: messageId, content: content);
+      // On success, we'll clear `status` from editMessageRequests
+      // when we get the event.
+    } catch (e) {


I can see why an error dialog isn't needed. The error text is shown immediately when the edit request fails. I wonder if the e is something more interesting/unexpected, do we want to log it?

PIG208 · 2025-04-28T19:01:30Z

test/model/message_test.dart

+      check(store.getEditMessageErrorStatus(message.id)).isNull();
+    }));
+
+    test('request failure before event arrival; take failed edit in between', () => awaitFakeAsync((async) async {


nit: the format "request fails, then message deleted" seems preferable to me to help understand the sequence of events

PIG208 · 2025-04-28T19:06:27Z

test/model/message_test.dart

+      check(store.getEditMessageErrorStatus(message.id)).isNull();
+    }));
+
+    test('takeFailedMessageEdit throws StateError when nothing to take', () => awaitFakeAsync((async) async {


I think a similarly useful test would be repeated editMessage calls that operate on the same message.

PIG208 · 2025-04-28T19:12:47Z

test/model/message_test.dart

+      store = eg.store();
+      connection = store.connection as FakeApiConnection;
+      message = eg.streamMessage();
+      await store.addMessage(message);


I think we can also test if the listeners are notified whenever an update is expected.

PIG208 · 2025-04-28T19:27:19Z

lib/model/message.dart

+  /// See also:
+  ///   * [getEditMessageErrorStatus]
+  ///   * [takeFailedMessageEdit]
+  void editMessage({required int messageId, required String content});


I think we should document the condition when you should not call editMessage (like we do for takeFailedMessageEdit), i.e., when there is a current edit message request.

chrisbobbe · 2025-04-30T20:22:14Z

Thanks for the review! Revision pushed.

PIG208

Thanks! Just some small comments on tests (and the failing CI might need a fix). Marking this for Greg's review.

PIG208 · 2025-05-01T02:28:12Z

test/model/message_test.dart

+      // First request has succeeded; event hasn't arrived
+      // Mid-second request
+      check(store.getEditMessageErrorStatus(message.id)).isNotNull().isFalse();
+      check(store.getEditMessageErrorStatus(otherMessage.id)).isNotNull().isFalse();


How about having checkNotNotified calls here and later? So that we are extra sure that it's the edit event that triggers the notifications.

PIG208 · 2025-05-01T02:34:32Z

test/model/message_test.dart

+      check(connection.takeRequests()).length.equals(1);
+      checkNotifiedOnce();
+
+      connection.prepare(json: UpdateMessageResult().toJson());


I'm a bit surprised that this is needed, since I thought store.editMessage would fail before firing the request, which is confirmed by the later .isEmpty check on takeRequests(). If I missed something, maybe a comment here will be helpful?

Ah yeah, this can be removed.

PIG208 · 2025-05-01T02:37:09Z

test/model/message_test.dart

+
+      async.elapse(Duration(milliseconds: 500));
+      // Mid-request
+      check(store.getEditMessageErrorStatus(message.id)).isNotNull().isFalse();


Similarly, having a checkNotNotified call here might help.

PIG208 · 2025-05-01T02:37:32Z

test/model/message_test.dart

+
+      async.elapse(Duration(milliseconds: 500));
+      // Mid-request
+      check(store.getEditMessageErrorStatus(message.id)).isNotNull().isFalse();


gnprice

Thanks! This looks great; just nits below, and a few comments on the docs.

gnprice · 2025-05-01T21:18:13Z

lib/model/message.dart

+  /// and the update-message event hasn't arrived.
+  bool? getEditMessageErrorStatus(int messageId);
+
+  /// Edits a message's content.


Given the context of being on [MessageStore] (or [PerAccountStore]), this reads as if it's making an edit to the data we have locally. The fact that it's making an API request over the network is an important part of its meaning, so should be made explicit.

E.g.:

Suggested change

/// Edits a message's content.

/// Edit a message's content, via a request to the server.

gnprice · 2025-05-01T21:19:12Z

lib/model/message.dart

@@ -35,6 +35,37 @@ mixin MessageStore {
  /// All [Message] objects in the resulting list will be present in
  /// [this.messages].
  void reconcileMessages(List<Message> messages);
+
+  /// Whether the current edit request, if any, has failed.


This reads to me as if there's one "current edit request" on the store. How about:

Suggested change

/// Whether the current edit request, if any, has failed.

/// Whether the current edit request for the given message, if any, has failed.

gnprice · 2025-05-01T21:20:12Z

lib/model/message.dart

+  /// Will be null if there is no current edit request
+  /// or the message was deleted.


I think I'd want to conceptualize "the message was deleted" as one way to (re-)enter the state of there being no current edit request. If the message was deleted, then any edit request for it that had been outstanding no longer is.

gnprice · 2025-05-01T21:22:31Z

lib/model/message.dart

+  @override
+  bool? getEditMessageErrorStatus(int messageId) =>
+    _editMessageRequests[messageId]?.hasError;
+  final Map<int, _EditMessageRequestStatus> _editMessageRequests = {};


nit: this field is a bit more than the backing for this getter-like method:

Suggested change

@override

bool? getEditMessageErrorStatus(int messageId) =>

_editMessageRequests[messageId]?.hasError;

final Map<int, _EditMessageRequestStatus> _editMessageRequests = {};

@override

bool? getEditMessageErrorStatus(int messageId) =>

_editMessageRequests[messageId]?.hasError;

final Map<int, _EditMessageRequestStatus> _editMessageRequests = {};

because in particular it has more data than that exposes.

gnprice · 2025-05-01T21:25:00Z

lib/model/message.dart

+    if (_editMessageRequests.containsKey(messageId)) {
+      throw StateError('message ID already in editMessageRequests');


nit: editMessageRequests isn't quite the name of anything; and the nearest name to that is private so might not be clearest for an error message:

Suggested change

if (_editMessageRequests.containsKey(messageId)) {

throw StateError('message ID already in editMessageRequests');

if (_editMessageRequests.containsKey(messageId)) {

throw StateError('an edit request is already in progress');

(though a private identifier in this message wouldn't be terrible either, since it really should be only developer-facing)

gnprice · 2025-05-01T21:27:53Z

lib/model/message.dart

+      // On success, we'll clear `status` from editMessageRequests
+      // when we get the event.


Is there an identifier status this refers to? I don't see it.

gnprice · 2025-05-01T21:33:14Z

lib/model/store.dart

+  @override
+  void editMessage({required int messageId, required String content}) {
+    assert(!_disposed);
+    return _messages.editMessage(messageId: messageId, content: content);


nit: let's move these up to sit next to the other MessageStore proxy implementations

(Should do the same with sendMessage too. I guess that should have happened when we moved sendMessage to MessageStore, 942aa87 / #1455, oh well.)

gnprice · 2025-05-01T21:44:33Z

test/model/message_test.dart

+      // A function marked async needs an async check,
+      // but its return type is void, so, cast the return value to Future
+      final future = store.editMessage(messageId: message.id, content: 'newer content') as Future;
+      await check(future).throws<StateError>();


Hmm interesting. How about:

Suggested change

// A function marked async needs an async check,

// but its return type is void, so, cast the return value to Future

final future = store.editMessage(messageId: message.id, content: 'newer content') as Future;

await check(future).throws<StateError>();

await check(store.editMessage(messageId: message.id, content: 'newer content'))

.isA<Future<void>>().throws<StateError>();

Effectively that moves the cast into a package:checks condition.

gnprice · 2025-05-01T21:52:44Z

test/model/message_test.dart

+      async.elapse(Duration(milliseconds: 500));
+      check(store.getEditMessageErrorStatus(message.id)).isNull();
+      checkNotNotified();


This version is slightly delicate, in that these two checks are only effective given that the elapse calls add up to at least the delay on the error response. The test is correct, but vulnerable to being defused in a not-entirely-obvious way by a future edit to those numbers.

How about:

Suggested change

async.elapse(Duration(milliseconds: 500));

check(store.getEditMessageErrorStatus(message.id)).isNull();

checkNotNotified();

async.flushTimers();

check(store.getEditMessageErrorStatus(message.id)).isNull();

checkNotNotified();

(It's basically OK as is, though; if there weren't such an easy way to make it more robust in that direction, I wouldn't want to attempt anything complex to guard against that sort of edit.)

…impls Thanks Greg for noticing this: zulip#1484 (comment)

chrisbobbe · 2025-05-01T23:35:55Z

Thanks for the review! Revision pushed.

In this revision, I did a few more things:

s/content/newContent/
Added support for prev_content_sha256

Related: zulip#126

chrisbobbe · 2025-05-02T20:03:40Z

(updated takeFailedMessageEdit's return type from String? to String, because if it doesn't have a string to return, it always throws a StateError)

chrisbobbe · 2025-05-02T20:05:02Z

Hmm, a CI failure:

Run flutter precache --universal
Downloading Linux x64 Dart SDK from Flutter engine ae352af9d0fb96896310e[7](https://github.com/chrisbobbe/zulip-flutter/actions/runs/14802472286/job/41564208589#step:6:8)dff8504c761d45d519...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100   255  100   255    0     0   20[8](https://github.com/chrisbobbe/zulip-flutter/actions/runs/14802472286/job/41564208589#step:6:9)2      0 --:--:-- --:--:-- --:--:--  2090
[/home/runner/flutter/bin/cache/dart-sdk-linux-x64.zip]
  End-of-central-directory signature not found.  Either this file is not
  a zipfile, or it constitutes one disk of a multi-part archive.  In the
  latter case the central directory and zipfile comment will be found on
  the last disk(s) of this archive.
unzip:  cannot find zipfile directory in one of /home/runner/flutter/bin/cache/dart-sdk-linux-x64.zip or
        /home/runner/flutter/bin/cache/dart-sdk-linux-x64.zip.zip, and cannot find /home/runner/flutter/bin/cache/dart-sdk-linux-x64.zip.ZIP, period.

It appears that the downloaded file is corrupt; please try again.
If this problem persists, please report the problem at:
  https://github.com/flutter/flutter/issues/new?template=01_activation.yml

Error: Process completed with exit code 1.

I'll rerun and see if that fixes it.

gnprice

Thanks! The revision looks good. A couple of comments both on the new commits at the end.

We could merge the first part:
285c1f4 store [nfc]: Move store.sendMessage up near other MessageStore proxy impls
f1c0e0f model: Implement edit-message methods on MessageStore

and then do the next two as a follow-up PR:
2c69364 api: Add prevContentSha256 param to updateMessage route
1eeb197 model: Use prevContentSha256 in edit-message method

gnprice · 2025-05-02T22:13:55Z

lib/api/route/messages.dart

+    // TODO(server-11) remove FL condition and its mention in the dartdoc
+    if (prevContentSha256 != null && connection.zulipFeatureLevel! >= 379) 'prev_content_sha256': RawParameter(prevContentSha256),


We should be able to just send this unconditionally — generally the server ignores unexpected parameters in requests:
https://zulip.com/api/rest-error-handling#ignored-parameters

(That doc is about some feedback the server includes in responses when that happens, and the feedback is new in server-7. But I believe the server has tolerated such parameters since forever.)

gnprice · 2025-05-02T22:15:42Z

lib/api/route/messages.dart

  String? content,
  int? streamId,
+  String? prevContentSha256,


nit: follow order in API doc (which is also a logical order — groups the two content-related parameters together):

Suggested change

String? content,

int? streamId,

String? prevContentSha256,

String? content,

String? prevContentSha256,

int? streamId,

chrisbobbe · 2025-05-02T23:07:44Z

We could merge the first part:
285c1f4 store [nfc]: Move store.sendMessage up near other MessageStore proxy impls
f1c0e0f model: Implement edit-message methods on MessageStore

OK, merging; thanks for the review! And I've sent #1496 for prevContentSha256.

…impls Thanks Greg for noticing this: #1484 (comment)

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Apr 24, 2025

chrisbobbe requested a review from PIG208 April 24, 2025 03:12

chrisbobbe changed the title ~~Edit-message (6/n): Implement edit-message methods on MessageStore~~ Edit-message (7/n): Implement edit-message methods on MessageStore Apr 24, 2025

chrisbobbe force-pushed the pr-edit-message-7 branch from a5262ab to 45b4b05 Compare April 25, 2025 21:29

chrisbobbe marked this pull request as ready for review April 25, 2025 21:29

chrisbobbe assigned PIG208 Apr 25, 2025

chrisbobbe force-pushed the pr-edit-message-7 branch from 45b4b05 to ea74a55 Compare April 25, 2025 22:18

PIG208 reviewed Apr 28, 2025

View reviewed changes

chrisbobbe force-pushed the pr-edit-message-7 branch from ea74a55 to 9b35f98 Compare April 30, 2025 20:14

PIG208 reviewed May 1, 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 1, 2025

PIG208 assigned gnprice and unassigned PIG208 May 1, 2025

PIG208 requested a review from gnprice May 1, 2025 02:38

chrisbobbe force-pushed the pr-edit-message-7 branch from 9b35f98 to fd7cf3a Compare May 1, 2025 21:12

gnprice reviewed May 1, 2025

View reviewed changes

store [nfc]: Move store.sendMessage up near other MessageStore proxy …

285c1f4

…impls Thanks Greg for noticing this: zulip#1484 (comment)

chrisbobbe force-pushed the pr-edit-message-7 branch from fd7cf3a to 93e84ad Compare May 1, 2025 22:53

model: Implement edit-message methods on MessageStore

f1c0e0f

Related: zulip#126

chrisbobbe force-pushed the pr-edit-message-7 branch from 0b6fbe2 to 1eeb197 Compare May 2, 2025 20:03

gnprice reviewed May 2, 2025

View reviewed changes

chrisbobbe force-pushed the pr-edit-message-7 branch 2 times, most recently from 4338003 to f1c0e0f Compare May 2, 2025 22:51

chrisbobbe mentioned this pull request May 2, 2025

Edit-message (8/n): Add/use prevContentSha256 param #1496

Merged

chrisbobbe merged commit e7708d6 into zulip:main May 2, 2025
2 checks passed

chrisbobbe added a commit that referenced this pull request May 2, 2025

store [nfc]: Move store.sendMessage up near other MessageStore proxy …

237859d

…impls Thanks Greg for noticing this: #1484 (comment)

		// Request has succeeded; event hasn't arrived
		check(store.getEditMessageErrorStatus(message.id)).isNotNull().isFalse();

	/// Edits a message's content.
	/// Edit a message's content, via a request to the server.

	/// Whether the current edit request, if any, has failed.
	/// Whether the current edit request for the given message, if any, has failed.

		/// Will be null if there is no current edit request
		/// or the message was deleted.

		if (_editMessageRequests.containsKey(messageId)) {
		throw StateError('message ID already in editMessageRequests');

		// On success, we'll clear `status` from editMessageRequests
		// when we get the event.

		// TODO(server-11) remove FL condition and its mention in the dartdoc
		if (prevContentSha256 != null && connection.zulipFeatureLevel! >= 379) 'prev_content_sha256': RawParameter(prevContentSha256),

Edit-message (7/n): Implement edit-message methods on MessageStore #1484

Edit-message (7/n): Implement edit-message methods on MessageStore #1484

Conversation

chrisbobbe commented Apr 24, 2025

chrisbobbe commented Apr 24, 2025 • edited Loading

chrisbobbe commented Apr 25, 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

Choose a reason for hiding this comment

chrisbobbe commented Apr 30, 2025

PIG208 left a comment • 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

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

chrisbobbe commented May 1, 2025

chrisbobbe commented May 2, 2025 • edited Loading

chrisbobbe commented May 2, 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 2, 2025

chrisbobbe commented Apr 24, 2025 •

edited

Loading

PIG208 left a comment •

edited

Loading

chrisbobbe commented May 2, 2025 •

edited

Loading