Adds server error messages to form actions bar #788
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/oxidecomputer/console-ui-storybook/CvZu67VirkUcZ3qif8xfErAnQooP |
|
Preview will be deployed at https://console-git-form-error-msgs.internal.oxide.computer |
| <div className="text-destructive">{getServerError(error)}</div> | ||
| <div className="text-destructive">{error?.error.message}</div> |
There was a problem hiding this comment.
This'll go away once we convert this form
libs/api/errors.ts
Outdated
| const methodCodeMap: { [key in keyof Partial<ApiMethods>]: Record<string, string> } = { | ||
| organizationsPost: { | ||
| ObjectAlreadyExists: 'An organization with that name already exists', | ||
| }, | ||
| projectInstancesPost: { | ||
| ObjectAlreadyExists: 'An instance with that name already exists in this project', | ||
| }, | ||
| projectDisksPost: { | ||
| ObjectAlreadyExists: 'A disk with that name already exists in this project', | ||
| }, | ||
| projectVpcsPost: { | ||
| ObjectAlreadyExists: 'A VPC with that name already exists in this project', | ||
| }, | ||
| vpcSubnetsPost: { | ||
| ObjectAlreadyExists: 'A Subnet with that name already exists in this project', | ||
| }, | ||
| } |
There was a problem hiding this comment.
You can imagine how this could get incredibly verbose. Definitely need to come up with a better way to break it down.
There was a problem hiding this comment.
We could easily put this together dynamically if each endpoint was tagged with the name of the resource and the name of the parent, but that really feels like API logic. At least for this specific error, I don't see why the API couldn't produce this sentence for us. To me the fact that this mapping feels like it belongs in the API layer of the console suggests it could really go in the API itself.
That doesn't solve the problem in general, because I'm sure there will be cases where we want to say something more specific than would be appropriate for an API message. But for those cases maybe we don't need to think of them as API-layer errors but rather as part of the logic of the calling form, like I had before at the page level:
console/app/pages/ProjectCreatePage.tsx
Lines 18 to 21 in 3505cc3
Maybe the solution is to get as many of these messages as we can from the API, and for the remaining ones we encode them at the form level?
There was a problem hiding this comment.
I agree this should come more from the API. Any transformations or mapping that we have to do on the client are a bit of a smell. One clear way we could resolve this is to change the ObjectAlreadyExists error in Nexus to require the type of the object that already exists. I mean, if an ObjectAlreadyExists error is spawned in a saga it might not be clear what object doesn't exist as we've built it now.
I'm not necessarily convinced that spreading the errors out makes any real difference. I'm afraid that would lead to inconsistent implementation of error messaging because the only context you have is what's already in the form you're looking at (or another form you reference). Having them centralized makes it easy to see all the custom errors at a glance. From an architectural perspective I also don't feel like the forms should have any specific knowledge of an error. Ideally it just displays whatever is given.
There was a problem hiding this comment.
I think spreading them out into the callers would make sense only if there are just a few very context-specific cases we can't get the API to handle, in which case we would think of them as UI copy.
That's a good point about sagas. With composite endpoints like instance create, we really do need more info from the API no matter what, and I'm inclined toward having it just write a nice message instead of trying to come up with an abstract scheme of all the bits of info we might need to come up with such a message.
There was a problem hiding this comment.
Looking at the Error enum in omicron, it looks like some of these messages are already build out. I think they could be tweaked, but that much is promising.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
libs/api/errors.ts
Outdated
| (errorCode: string, error: Error): string | undefined => { | ||
| switch (errorCode) { |
There was a problem hiding this comment.
I'm not a huge fan of errorCode being untyped. Generally this is because dropshot is piping out a generic http error instead of more specific nexus errors.
I wrote about this in chat:
Currently the Error type isn't being exported via the OpenAPI spec. The exported error type is much more permissive. If I'm reading dropshot#286 correctly, the permissive type is just dropshot's default http error. Looking at http_entrypoints that'd make sense given that all our routes just return dropshot's HttpError. I'm wondering at this point if we could change dropshot's behavior to provide a customizable http error. Ideally if we could return like... NexusHttpError instead, we could likely get the types that we want.
Goals
error.messageresponse anywhere in app codeA note on client-side vs server-side errors
To re-cap a lot of in-chat discussions, we largely believe the client should do very little in the way of modifying error messages coming back from the server. Ideally the server would provide errors that are injectable directly into the form error. Further, if we need to map an error to a particular field, that information will ultimately need to be provided by the server too. The latter part is kind of covered by oxidecomputer/omicron#910. Once we've made server-side updates, it's very likely that
errorCodeFormattercould just go away entirely and most of what's happening in the client api layer can be simplified.Progress shots