Fix auto-updating of boot disk size when selected image is larger than current disk size; add tests

[charliepark]

In the instance creation form, when a user selects an existing image, we want the disk size for the new instance to automatically increase so that it's large enough to successfully use that image. This PR updates the code so that it successfully gets increased to the next-largest increment of 10 GiB (relative to the selected image). It also adds a validator message and an e2e test to cover this behavior.

A few small UI updates in this PR as well that are worth mentioning:

1. This PR switches the order of the image lists, so that the silo images tab comes before the project images tab.
2. This PR adds a warning for the unlikely edge case that there are neither silo images nor project images. (It is possible to create an instance without a disk image through the API, but creating an instance via the console requires an image to be present. Accordingly, with this PR, the form is disabled when no images are available.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
console	✅ Ready (Inspect)	Visit Preview	Dec 7, 2023 5:16am

[zephraph]

Worth noting that we do want to support more cases (like creating boot disks from snapshots or existing disks) eventually.

See the designs here: https://www.figma.com/file/n8tWb6BCJsSQ3SXdvi7ZeG/Instances?type=design&node-id=2936-69630&mode=design&t=M5GLVRDZUIX5FO8b-4 (I'd defer to @benjaminleonard or @paryhin if updates are required to those).

I think changing the orders of the tabs is fine (though we may want to just collapse those down into a single interface at some point).

[zephraph]

This is a nice touch 👍

[zephraph]

Another thing we could do here is just update the min input threshold. We'd probably want to include an info icon with some details to indicate why the user can't go below the threshold though.

[david-crespo]

Yeah, that’s why we went with two thresholds, the message saying it’s due to the image is pretty helpful. See also #1824

[zephraph]

I'm not quite sure that we should should push the validation logic down into the DiskSizeField component. Disks, as a concept, aren't strictly speaking tied to images. Certainly in this case the disk size is semi-dependent on the image size, but that doesn't impact other situations in which a disk size may be specified.

We'll either continue pushing more context down into the DiskSizeField component or have to find some other way of composing the logic. I think my preference, for now, is to localize the validation logic to where it's used (similar to what we had here). If we need that logic to be more portable perhaps we could just have some reusable helper we could spread into the usage?

[david-crespo]

Yeah, even though the logic is currently the same between the two spots, we could model it as a shared validate function that’s passed in in both spots.

[charliepark]

Refactored this after chatting with @david-crespo … several of the validations are standard and will be needed for the DiskSizeField regardless of where we use it. But we also want to have the option of adding one-off validators. So we added an optional validate prop that we can pass in when calling the component; any validations passed in via that prop will get called after the standard validator functions are evaluated.

[david-crespo]

Love the test!

[david-crespo]

I thought we had also added imageSize to the DiskSizeField on the disk create form last week, but it seems we did not. You could do it in this PR since it should only be couple of lines. A slightly tricky bit in that one is that there isn't always an image (and unlike here, that's not a pathological case, because you might not be creating the disk from an image).

[david-crespo]

This is more of a FYI about React Hook form, not a suggestion, because I can't bring myself to suggest something so much worse than what you have, and I don't think it's really more typesafe in practice. But if you hover on the validate prop, you can see it is using some helpers from RHF to look up the value of the field using the field name.

If you wanted to match that type on the prop, you could do this:

  validate?: Validate<PathValue<TFieldValues, TName>, TFieldValues>

This produces the same result as what you have (the argument is a number, of course) except it also has a second argument, which is all the form values together:

This could be handy in theory because you could validate one field value based on the value of another. But we're not doing that — well, we're doing it, but we're doing it at the call site so this field doesn't have to know about it. So we don't need it, and the resulting code is dramatically harder to understand, plus the second arg is required (lol) so it actually gets mad at you for leaving it out.

All this is to say, I've already typed this so I might as well shoot it into the ether, but it is not a suggestion to use this horrible type.

[charliepark]

Ah, that's good to know, about, though! Thanks for writing that all out.

[david-crespo]

I'd inline this here and save a bunch of lines. It made more sense as a component when it was responsible for fetching the images. I think the idea there was you could avoid fetching them until the image picker was on screen, but we can't do that anymore since we need the size at top level.

Suggested change

	{value.type === 'image' && (
	<ImageSelectField
	control={control}
	images={images}
	areImagesLoading={areImagesLoading}
	/>
	)}
	{value.type === 'image' && (
	<ListboxField
	control={control}
	name="diskSource.imageId"
	label="Source image"
	placeholder="Select an image"
	isLoading={areImagesLoading}
	items={images.map((i) => toListboxItem(i, true))}
	required
	/>
	)}

This also seems to me an ok spot to do the auto size update thing we're doing in the instance create ImageSelectField. I think you can use setValue instead of the controller business in the other one, so pretty nice all around. You have to pass form instead of control though to get access to those. Probably better than passing the methods individually. Here is something that seems to work, though it's not ideal — see for example the spot where I'm duplicating the image size lookup.

diff --git a/app/forms/disk-create.tsx b/app/forms/disk-create.tsx
index 4d413aa9..1699991e 100644
--- a/app/forms/disk-create.tsx
+++ b/app/forms/disk-create.tsx
@@ -6,7 +6,7 @@
  * Copyright Oxide Computer Company
  */
 import { format } from 'date-fns'
-import { useController, type Control } from 'react-hook-form'
+import { useController, type Control, type UseFormReturn } from 'react-hook-form'
 import { useNavigate, type NavigateFunction } from 'react-router-dom'
 
 import {
@@ -110,11 +110,7 @@ export function CreateDiskSideModalForm({
       <NameField name="name" control={control} />
       <DescriptionField name="description" control={control} />
       <FormDivider />
-      <DiskSourceField
-        control={control}
-        images={images}
-        areImagesLoading={areImagesLoading}
-      />
+      <DiskSourceField form={form} images={images} areImagesLoading={areImagesLoading} />
       <DiskSizeField
         name="size"
         control={control}
@@ -129,17 +125,17 @@ export function CreateDiskSideModalForm({
 }
 
 const DiskSourceField = ({
-  control,
+  form,
   images,
   areImagesLoading,
 }: {
-  control: Control<DiskCreate>
+  form: UseFormReturn<DiskCreate>
   images: Image[]
   areImagesLoading: boolean
 }) => {
   const {
     field: { value, onChange },
-  } = useController({ control, name: 'diskSource' })
+  } = useController({ control: form.control, name: 'diskSource' })
 
   return (
     <>
@@ -169,7 +165,7 @@ const DiskSourceField = ({
             name="diskSource.blockSize"
             label="Block size"
             units="Bytes"
-            control={control}
+            control={form.control}
             parseValue={(val) => parseInt(val, 10) as BlockSize}
             items={[
               { label: '512', value: 512 },
@@ -179,41 +175,32 @@ const DiskSourceField = ({
           />
         )}
         {value.type === 'image' && (
-          <ImageSelectField
-            control={control}
-            images={images}
-            areImagesLoading={areImagesLoading}
+          <ListboxField
+            control={form.control}
+            name="diskSource.imageId"
+            label="Source image"
+            placeholder="Select an image"
+            isLoading={areImagesLoading}
+            items={images.map((i) => toListboxItem(i, true))}
+            required
+            onChange={(id) => {
+              const diskSizeGiB = form.getValues('size')
+              const size = images.find((image) => image.id === id)?.size
+              const imageSizeGiB = size ? bytesToGiB(size) : undefined
+              if (imageSizeGiB && diskSizeGiB < imageSizeGiB) {
+                const nearest10 = Math.ceil(imageSizeGiB / 10) * 10
+                form.setValue('size', nearest10)
+              }
+            }}
           />
         )}
 
-        {value.type === 'snapshot' && <SnapshotSelectField control={control} />}
+        {value.type === 'snapshot' && <SnapshotSelectField control={form.control} />}
       </div>
     </>
   )
 }
 
-const ImageSelectField = ({
-  control,
-  images,
-  areImagesLoading,
-}: {
-  control: Control<DiskCreate>
-  images: Image[]
-  areImagesLoading: boolean
-}) => {
-  return (
-    <ListboxField
-      control={control}
-      name="diskSource.imageId"
-      label="Source image"
-      placeholder="Select an image"
-      isLoading={areImagesLoading}
-      items={images.map((i) => toListboxItem(i, true))}
-      required
-    />
-  )
-}
-
 const DiskNameFromId = ({ disk }: { disk: string }) => {
   const { data, isPending, isError } = useApiQuery(
     'diskView',

[david-crespo]

I think you could do this in a followup since it's gnarly!

[charliepark]

Yeah, I think adding in a second PR will make it easier to focus in on the specific issue. Thank you for the proof of concept. Will iterate off of that in another branch.

[david-crespo]

I think it's good!