Improve fetch api handler to reject non-ok responses #4538
Conversation
and improve UpdatePasswordForm to give better error feedback
Users might run into this while they're typing a normal search, so a Red X seems strong and even a yellow Warning is probably too much. A green checkmark would be silly. It'd be nice to detect this with something other than a translated string comparison, but this will probably work, and yellow warning is an ok fallback.
It might even make sense to add timestamps.
jsapp/js/api.ts
Outdated
|
|
||
| // We only want to display the server issues notification once, even with | ||
| // multiple calls failing | ||
| notifyServerErrorThrottled(errorMessage); |
There was a problem hiding this comment.
I want to hold off on this for now—I thought about this some more and I don't throttling these is a good idea.
Two days ago, when we had a double submit issue, the first indication was that there were 2 identical toast notifications.
Seeing lots of errors when one thing is broken isn't fun, but discarding errors without showing them at all is usually worse — particularly if the errors aren't duplicates and some may be more informative than others.
Related: Once toast notifications disappear, you can't review them at all. At least for our own debugging purposes, I want to start logging them to the browser console.
| @@ -56,7 +56,7 @@ export default class LanguagesListStore { | |||
|
|
|||
| private onAnyFail(response: FailResponse) { | |||
| this.isLoading = false; | |||
| notify(response.responseText, 'error'); | |||
| notify(response.responseText || t('Unknown error'), 'error'); | |||
There was a problem hiding this comment.
I think it would be helpful to show HTTP status codes here — in case someone is seeking support for an issue they're encountering.
There was a problem hiding this comment.
Could we use utils.ts handleApiFail(FailResponse) here?
There was a problem hiding this comment.
I made a few changes:
- Tried to standardize around 'An error occurred' + status codes, rather than 'Unknown error'
- Start to log notify() calls to browser console, so when these things disappear we can still read them
- Turned off error notification throttling for now — I feel like in the near term this would do more harm than good.
Not super specific to this PR, but it came up during testing and was a fairly easy fix:
- Refined behavior of 'Query too short' message
- these are batched by id so you no longer get a whole stack of these as you type a query
- and it goes away when a successful query result comes back.
- Also changed the icon from the 'task failed successfully' green checkmark to no icon.
- If the assets fail to list for some other reason, it's a yellow warning box.
We're not 'handling' every kind of error consistently, and I think the code organization could use some improvement. But let's merge this because it fixes the password update problem.
Description
Make Update Password Form use more detailed error messages.