compose box: On failed message send, show error dialog

[chrisbobbe]

Fixes: #815

[gnprice]

Thanks for taking care of this quickly! Comments below.

[gnprice]

There's never a real message involved here, right? And in the real-user interaction this is simulating, this isn't a message that would exist at this stage — this is a message that might exist after the user hits send and the server processes that.

So it feels confusing for that test narrative to create a message object here. The test doesn't end up actually using much of it anyway — for example the content here doesn't affect the test. Instead we can use the stream and topic directly:

Suggested change

	final message = eg.streamMessage(
	stream: stream,
	topic: 'some topic',
	content: 'hello world',
	);
	await prepareComposeBox(tester, TopicNarrow.ofMessage(message));
	await prepareComposeBox(tester, TopicNarrow(stream.streamId, 'some topic'));

[chrisbobbe]

Ah indeed!

[gnprice]

This changes more than the error behavior — it also means we wait to clear the content input until the request completes.

Our promptly clearing the input isn't entirely a good thing; that's what #720 is about. But if we take that out without doing the other work called for in #720, I think the effect is that there's a lack of feedback so that it feels like the send button didn't do anything, or is sluggish.

Trying this, I notice that even on my fast home network connection. Then to demonstrate how it behaves when the request is especially slow, try this patch:

--- lib/api/core.dart
+++ lib/api/core.dart
@@ -188,6 +188,7 @@ class ApiConnection {
     if (params != null) {
       request.bodyFields = encodeParameters(params)!;
     }
+    await Future<void>.delayed(const Duration(seconds: 2));
     return send(routeName, fromJson, request, overrideUserAgent: overrideUserAgent);
   }
 

The way to fix that looks something like:

final sendFuture = store.sendMessage(…);
widget.contentController.clear();

try {
  await sendFuture;
} on …

Or I guess more straightforwardly, we can just call clear before sendMessage.

[chrisbobbe]

Makes sense.

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

nit: first clause of the TODO sounds like a description of what the code is already doing

[gnprice]

nit: can give a hint what the callback parameter means:

Suggested change

	required void Function(int) prepareResponse,
	required void Function(int messageId) prepareResponse,

(wasn't so necessary when the type was Message)

[gnprice]

Thanks for the revision! Two nits above, and otherwise LGTM.

[alya]

Maybe we can drop "The server said:" part? It doesn't seem necessary to me from an end user perspective.

[gnprice]

In general this error dialog is handling an error that the server gave us but we didn't anticipate. (Anticipating this particular error is #791, which is behind a couple of larger prerequisite issues.)

So that's meant to be a situation that's rare; and because we didn't anticipate the error, it's also possible that the text is somewhat obscure or out of context. That's why I think it's potentially helpful to disambiguate that this is text from the server and not some other error message from within the app.

We use the same pattern in handling some other errors too, so it'd be easy to change them all together.

[alya]

Ok. Might be worth discussing in #mobile, but definitely not a blocker for merging this PR, given that context.

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

Thanks! Looks good — merging.