Show poll-failure details

[PIG208]

Ready for review.

This fixes #555.

[PIG208]

A question to be answered: how do we determine if an error is transient?

Currently we handle 3 classes of errors in UpdateMachine:

1. ZulipApiException: The event queue is lost. This is commonly caused by inactivity.
2. Server5xxException, NetworkException: A server-side bug, or hiccups due to unstable Internet connection.
3. default: Could be anything that happens in the client code during the getEvents call.

I think it is usually safe to assume that 1 is recoverable, while we are less certain about that for 2 and 3. An idea is to define a number of retries for the client to start displaying error messages. And then in a follow-up PR the client can try to do something different (which is #186).

[PIG208]

For non-transient errors, we will probably show a modal instead.

[gnprice]

If there's a ZulipApiException other than the event queue being gone, then that's likely non-transient — it's either a bug in the client, or some kind of unexpected incompatibility between server and client. A retry is likely to just hit the same error again.

(Those fall into the default case.)

[gnprice]

Other than that point, the description above all looks right.

[PIG208]

The PR has been updated. Thanks for the review!

I'm not entirely sure about the quality of the translation strings. These messages will appear more technical at the current stage for the ease of debugging. However, polishing user facing errors might be a post-launch task.

[PIG208]

Updated the PR to avoid wrapping test calls in checkRetry.

[PIG208]

Updated the PR to adjust comments on lib/log.dart and made dialog title a parameter. I also tweaked the formatting and made some other minor improvements.

[PIG208]

This update modifies the translation strings to include the realm URL in error messages.

[gnprice]

That fix LGTM on a skim, thanks. I'll leave @chrisbobbe to do the next round of full review.

[PIG208]

Thanks for the comments. This should be ready for review again!

[gnprice]

@PIG208 @chrisbobbe I recommend installing this PR version on your main devices for a while, with flutter run --release. I just installed it yesterday, and the output is interesting but can definitely use some tweaking to be less noisy (particularly with dialogs).

[PIG208]

Regarding the noisiness of the error dialog, I think a better UX is to present a snackbar with a brief error summary and a "Show more" button that brings up the dialog page with the full error message.

[PIG208]

Implemented the proposed change in a squash! commit. Before moving on to the next review, I would like to gather some feedback on this new UX (discussion)

[chrisbobbe]

Thanks! This revision LGTM. One thought is that it would be nice if the snackbar API gave us a way to identify a specific snackbar to remove, so we could be sure we're removing the one we intend to. But it looks like our choices are the "current" snackbar and all snackbars. The "current" snackbar I guess means the latest one from the queue, if it hasn't expired, and that could be right or wrong depending on the durations of the snackbars and how much time has passed.

[PIG208]

Yeah, I figured that tracking if the last snack bar has expired would reduce collateral dismissal of unrelared snackbars in most cases.

[PIG208]

Updated the PR to add a comment on the https://github.com/zulip/zulip-flutter/pull/868#discussion_r1733560511 implementation.

[PIG208]

Pushed to rebase.

[chrisbobbe]

Thanks, LGTM! Over to Greg for his review.

[PIG208]

Added a commit removing a stale TODO, and squashed the two of the error reporting commits (for transient and non-transient errors) together.

[PIG208]

The public message history from CZO can be downloaded from this protected dropbox link. We probably will think of a better way of reusing this in the future.

[gnprice]

The public message history from CZO can be downloaded from this protected dropbox link. We probably will think of a better way of reusing this in the future.

(Was this comment intended for #917?)

[PIG208]

Thanks for the review! The PR has been updated.

[gnprice]

Quick observation on the UX:

I reopened the app after a while. It showed the loading indicator at top for a while, then successfully reconnected.

But then for some time after that, it showed a chain of several rounds of snack bars saying there was an error, followed by one saying "reconnecting". Each one lasts several seconds, and there were about three of them that started after the loading indicator finished.

I hit the "details" link on a couple of the errors. Both were NetworkException, saying the host name chat.zulip.org couldn't be resolved.

[PIG208]

Opened #935 for the two commits implementing the report error feature, and #936 for the other nfc commit.

[PIG208]

Updated the PR to ignore NetworkException where the underlying cause is a SocketException, added some more documentation and cleaned up some diffs to make them easier to read. Feel free to install a build of this PR to test it out!

[gnprice]

Thanks for the update! Installing on my phone now.

[PIG208]

Thanks for the feedback! Dropped the last commit and previous changes relevant to the tests. A new group of tests is now responsible for testing the error reporting feature.

[PIG208]

Thanks! Ready for review now.

[gnprice]

Looks good — merging.