edit-message: Show error dialog on edit-message request error #1792
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I'm fairly certain this is NFC. The two calls do operate on shared state, in the controller, but it should suffice to just capture `messageId` and `newContent` from the ComposeBoxController before endMessageEdit disposes it. Soon we'd like `store.editMessage` to return a Future, which we'd like to await and add some error feedback, just like we have in the send-message UX.
See added dartdoc for motivation. I don't think there's a behavior change here; we don't use the passed BuildContext after an async gap. We'd like to do that soon, though: to show an error dialog on API errors from the edit-message request; such errors will arrive after the banner has disappeared.
I'm not finding the discussion where we said we wanted this (for consistency with the send-message UX), but I think we did. Error-feedback logic copied from the tap-"Send" button (_SendButtonState._send) modulo the error dialog's title, "Message not saved" vs. "Message not sent".
chrisbobbe
added
the
integration review
gnprice
reviewed
Aug 14, 2025
Comment on lines
+1800
to
1802
| final messageId = controller.messageId; | ||
| final newContent = controller.content.textNormalized; | ||
| composeBoxState.endEditInteraction(); |
There was a problem hiding this comment.
compose_box: Reorder two calls in edit-message "Save" tap handler
I'm fairly certain this is NFC. The two calls do operate on shared
state, in the controller, but it should suffice to just capture
`messageId` and `newContent` from the ComposeBoxController before
endMessageEdit disposes it.
I don't quite follow this part — which two calls operate on shared state in the controller? I don't think store.editMessage operates on the controller; it's down in the model layer and shouldn't know about UI state like a compose-box controller anyway.
Comment on lines
+61
to
+62
| /// The returned [Future] resolves or rejects with the edit-message request, | ||
| /// irrespective of when the edit-message event arrives (if it does). |
There was a problem hiding this comment.
I think the actual behavior is good, but this description doesn't quite capture it:
- If the request fails, but only after the event has already arrived, the future resolves rather than rejects.
- Similarly if the request fails but the message has been deleted.
- As of this commit, the future actually never rejects (short of a bug, like reaching the
throw StateErrorline); that only happens when therethrowis added in a later commit. Probably that rethrow can just be part of this commit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With #188, the message-list UI for an unsubscribed channel will become more prominent, which calls for fixing at least the following glitch:
Since we don't get events when the self-user sends or edits a message in an unsubscribed channel, we'd like to refetch messages on success of those actions. Discussion: #mobile > Message sending in unsubscribed channels @ 💬 So, on the path to that, make
store.editMessagesimilar tostore.sendMessagein returningFuture<void>instead ofvoid, where the returned Future resolves or rejects with the API request, irrespective of the event.While we're at it, show an error dialog when the edit fails, as we've been meaning to do, again for consistency with the send-message UX.
Compare error feedback on sending an invalid message (in
mainand unchanged here) vs. trying to save an invalid message edit (new here; inmainwe show no dialog):The outbox logic is unchanged. As in
main, we show "MESSAGE NOT SENT" and "EDIT NOT SAVED" in the message list, and these are tappable to restore the composing session and try again.