chore: More UI friendly errors #1994
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.
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:
type Response struct {
// An actionable message that depicts actions the request took. Examples:
// - "A user has been created."
// - "Failed to create a user."
Message string `json:"message"`
// A debug message that provides further insight into why the action failed.
// - "database: too many open connections"
// - "stat: too many open files"
Detail string `json:"detail"`
}I don't know what to name Errors. since it's form-specific I'd lean towards Validations maybe?
I'm curious for your take @presleyp, you're a magician with words! 🪄
There was a problem hiding this comment.
@Kira-Pilot suggested Verbose instead of Internal and I like that; Detail works too.
I like Validations better than Errors - 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.
I can change that now, that's easy with IDE tools
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>
Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>
There was a problem hiding this comment.
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.
| httpapi.Write(rw, http.StatusForbidden, httpapi.Response{ | ||
| Message: err.Error(), | ||
| }) | ||
| httpapi.Forbidden(rw) |
There was a problem hiding this comment.
question: what allowed us to make this change? Does it mean that we no longer send those random Message errors on forbidden routes?
There was a problem hiding this comment.
@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.
Mainly capitlization
* chore: More UI friendly errors Mainly capitlization + messages prefix error
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 failingwas 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