- Notifications
You must be signed in to change notification settings - Fork928
chore: More UI friendly errors#1994
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Mainly capitlization + messages prefix error
coderd/httpapi/httpapi.go Outdated
// Message is for general user-friendly error messages. This message will | ||
// be shown at the top/bottom of a form, or in a toast on the UI. | ||
Message string `json:"message"` | ||
// Internal has the technical error information (err.Error()). These details | ||
// might come from external packages and might not be user friendly. | ||
// Do not populate this error field with any sensitive information or | ||
// any errors that may be a security implication. These details are still | ||
// available to more technical users. | ||
Internal string `json:"internal"` | ||
// Errors are form field-specific friendly error messages. They will be | ||
// shown on a form field in the UI. These can also be used to add additional | ||
// context if there is a set of errors in the primary 'Message'. | ||
Errors []Error `json:"errors,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this will be difficult to understand at a glance how I'd display an error to the user.
Here's an alternative structure that just tweaks the names a bit and adds examples, but could help:
typeResponsestruct {// An actionable message that depicts actions the request took. Examples:// - "A user has been created."// - "Failed to create a user."Messagestring`json:"message"`// A debug message that provides further insight into why the action failed.// - "database: too many open connections"// - "stat: too many open files"Detailstring`json:"detail"`}
I don't know what to nameErrors
. since it's form-specific I'd lean towardsValidations
maybe?
I'm curious for your take@presleyp, you're a magician with words! 🪄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Kira-Pilot suggestedVerbose
instead ofInternal
and I like that;Detail
works too.
I likeValidations
better thanErrors
- more specific! Right now I feel like it requires an explanatory comment on the frontend.
But I know@Emyrk doesn't want this PR to be open for long, maybe these tweaks can be a followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I can change that now, that's easy with IDE tools
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>
Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>
Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>
Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>
Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There are a few actual comments in there mixed in with the committable suggestions, the most important of which IMO is how to handle cases where the message is redundant with the field error. But I'm cool with merge after the committable suggestions are in, we can be iterative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM with@presleyp 's suggestions
httpapi.Write(rw, http.StatusForbidden, httpapi.Response{ | ||
Message: err.Error(), | ||
}) | ||
httpapi.Forbidden(rw) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
question: what allowed us to make this change? Does it mean that we no longer send those randomMessage
errors on forbidden routes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@vapurrmaid for security reasons all forbidden messages should be identical. If they were different, then the different errors allow the end user to gain information.
There is a line of "security" vs "usability" that we will need to decide on these endpoints. As it's unhelpful by design.
misskniss commentedJun 3, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
very much welcome this addition
Mainly capitlization
* chore: More UI friendly errorsMainly capitlization + messages prefix error
Uh oh!
There was an error while loading.Please reload this page.
If you see a type in this PR, feel free to just commit the spelling fix to the branch
I might have missed something. And errors can be improved from here.
Some detail about postgres failing
was a hardcoded error to show what it looks like in the cli.NOTE
All these errors were my best effort giving like 20 seconds per error. I imagine we can improve from here, and as we come across anything vague, we can do that. This is just the baseline work for the UI team to be unblocked.
I don't think I made anything more vague. So anything not specific is status quo, just with the new format.
Future work