Prevent creating multiple disks with the same name #2541
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
charliepark
changed the title
|
|
…n the same process; add test
| */ | ||
| onDismiss: (navigate: NavigateFunction) => void | ||
| onSuccess?: (disk: Disk) => void | ||
| unavailableDiskNames?: string[] |
There was a problem hiding this comment.
I do like this a lot better without adding disks from the API in here. I'd add a comment here on the prop saying this is for client-side use-cases like the form embedded in instance create, where submitting the form does not tell you whether it's taken.
app/forms/instance-create.tsx
Outdated
| disabled={isSubmitting} | ||
| validate={(name) => { | ||
| // don't allow the user to use an existing disk name for the boot disk's name | ||
| if ([...namesOfDisksToBeCreated, ...existingDiskNames].includes(name)) { |
There was a problem hiding this comment.
Very small thing, but I think it would be easier to follow if this list concatenation was done next to the other disk name list logic. You can even wrap it in a function to avoid mapping over a potentially large list of disks on every render.
const otherDisks = useWatch({ control, name: 'otherDisks' })
function unavailableDiskNames() {
const existingDiskNames = allDisks.map((disk) => disk.name)
const namesOfDisksToBeCreated = otherDisks
.filter((disk) => disk.type === 'create')
.map((disk) => disk.name)
return [...namesOfDisksToBeCreated, ...existingDiskNames]
}There was a problem hiding this comment.
Hm, actually, depending on whether every keypress causes a render, this could be worse because you're mapping on every validate. Initially I pictured this as only happening on submit, but once you're revalidating after an error, it runs on every keypress.
There was a problem hiding this comment.
I just confirmed that typing in the disk name field of disk create does not render the overall form, so it is better to leave it as-is.
oxidecomputer/console@48fc079...0517a28 * [0517a281](oxidecomputer/console@0517a281) oxidecomputer/console#2541 * [510c6457](oxidecomputer/console@510c6457) oxidecomputer/console#2555 * [d7908192](oxidecomputer/console@d7908192) oxidecomputer/console#2553 * [16e86508](oxidecomputer/console@16e86508) oxidecomputer/console#2552
oxidecomputer/console@48fc079...0517a28 * [0517a281](oxidecomputer/console@0517a281) oxidecomputer/console#2541 * [510c6457](oxidecomputer/console@510c6457) oxidecomputer/console#2555 * [d7908192](oxidecomputer/console@d7908192) oxidecomputer/console#2553 * [16e86508](oxidecomputer/console@16e86508) oxidecomputer/console#2552
Currently, it's possible to "create" multiple new disks with the same name in the instance create form. This is a little goofy, because the user isn't actually creating the new disks at this point in the form, they're queuing them up to be created shortly. So we have an issue where multiple matching domain names are problematic, and deleting one of those disks will end up deleting both disks.
This PR fixes that by adding validation check to the name field for disk-create, as well as a validation on the boot disk name in the instance-create form. It was already the case that an existing disk name couldn't be used for the boot disk name, but we were not checking the names of the liminal disks (set to be created, but not yet actually created). We are now.
Closes #2519