ShareWrapper: Abort upload and show feedback when file upload fails #5661
Conversation
src/sharing/ShareWrapper.js
Outdated
| let msg = undefined; | ||
| if (error instanceof ApiError && error.message.length > 0) { | ||
| msg = error.message; | ||
| } | ||
|
|
||
| // TODO: handle other kinds of errors | ||
|
|
||
| this.setState({ sending: false }); | ||
| showErrorAlert(_('Failed to upload file: {fileName}', { fileName }), msg); |
There was a problem hiding this comment.
How about using error.message for all Error instances, not just ApiError?
There was a problem hiding this comment.
Sure. The message won't always be good user-facing text, but at least it'll help us debug when we get reports.
src/sharing/ShareWrapper.js
Outdated
| if (!(error instanceof Error)) { | ||
| logging.error('ShareWrapper: Unexpected non-error thrown'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
If this somehow happens, seems best to still give the user feedback. Just won't have a message for them.
chrisbobbe
force-pushed
the
pr-sharing-feedback-upload-fails
branch
from
0767b8f to
64a5a8c
Compare
|
Thanks for the review! Revision pushed. |
src/sharing/ShareWrapper.js
Outdated
| error instanceof ApiError | ||
| && error.code === 413 // 413 Payload Too Large: | ||
| // https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/.2Fuser_uploads.3A.20.60413.20Payload.20Too.20Large.60/near/1332958 |
There was a problem hiding this comment.
This should say instanceof RequestError, rather than ApiError.
When we get this status code, it may not be from the Zulip server application with its well-formed Zulip API error blob — it may be from something like nginx sitting in front of it. So that won't be an ApiError.
Oh also 413 isn't a Zulip API error code, so it won't be a value of ApiError.code:
export class ApiError extends RequestError {
+code: ApiErrorCode;
// …
/**
* An identifier-like string identifying a Zulip API error. …
*/
export type ApiErrorCode = string;Instead this should be error.httpStatus. See RequestError.
src/sharing/ShareWrapper.js
Outdated
| && error.code === 413 // 413 Payload Too Large: | ||
| // https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/.2Fuser_uploads.3A.20.60413.20Payload.20Too.20Large.60/near/1332958 | ||
| ) { | ||
| msg = _('This file is too large.'); |
There was a problem hiding this comment.
Let's make this say: "The server said the file is too large."
I.e., make clear that it's the server that made this judgement, not the client app.
In a lot of apps, where the server and client are provided by the same organization, making that distinction would be bad UI practice. But for Zulip I think it's appropriate because it helps the user figure out who to complain to.
(At first I wrote "The server rejected the file. The file is too large." (I considered "The server rejected the file as too large", but I think that syntax is a bit too complex for good UI writing.) But then while writing the next comment I realized that this gets shown below a heading saying "Failed to upload file". So it can be simplified.)
| } else if (error instanceof Error && error.message.length > 0) { | ||
| msg = error.message; | ||
| } |
There was a problem hiding this comment.
That gives me a related thought: let's have this generic Error case, but before this let's say something like: if it's an ApiError, then msg = _('The server said: {errorMessage}', { errorMessage: error.message }).
That way again the attribution is clear.
There was a problem hiding this comment.
Makes sense. I put \n\n in this revision for consistency with a similar message we show in the login flow: The server at {realm} said:\n\n{message}
chrisbobbe
force-pushed
the
pr-sharing-feedback-upload-fails
branch
from
64a5a8c to
9dfeb2d
Compare
|
Thanks for the review! Revision pushed, and I also just sent #5663 for the compose box. |
|
Thanks! Looks good; merging. I'll look at that other one next. |
gnprice
force-pushed
the
pr-sharing-feedback-upload-fails
branch
from
9dfeb2d to
22129e9
Compare
|
Thanks! |
Related: #5660