From c250f543ba0006d25d6f9b1dac6ebc862b816368 Mon Sep 17 00:00:00 2001
From: Charlie Park <charlie@oxidecomputer.com>
Date: Sat, 2 Dec 2023 10:01:00 -0800
Subject: [PATCH 1/5] Fix disk size auto-updating when image changes; add tests

---
 app/components/form/fields/DiskSizeField.tsx | 15 ++++-
 app/components/form/fields/NumberField.tsx   |  5 +-
 app/forms/instance-create.tsx                | 61 +++++++++++---------
 app/test/e2e/instance-create.e2e.ts          | 37 ++++++++++++
 4 files changed, 86 insertions(+), 32 deletions(-)

diff --git a/app/components/form/fields/DiskSizeField.tsx b/app/components/form/fields/DiskSizeField.tsx
index 39afca5b3..b8098c408 100644
--- a/app/components/form/fields/DiskSizeField.tsx
+++ b/app/components/form/fields/DiskSizeField.tsx
@@ -16,13 +16,20 @@ interface DiskSizeProps<
   TFieldValues extends FieldValues,
   TName extends FieldPath<TFieldValues>,
 > extends TextFieldProps<TFieldValues, TName> {
+  imageSize?: number
   minSize?: number
 }
 
 export function DiskSizeField<
   TFieldValues extends FieldValues,
   TName extends FieldPathByValue<TFieldValues, number>,
->({ required = true, name, minSize = 1, ...props }: DiskSizeProps<TFieldValues, TName>) {
+>({
+  required = true,
+  name,
+  imageSize,
+  minSize = 1,
+  ...props
+}: DiskSizeProps<TFieldValues, TName>) {
   return (
     <NumberField
       units="GiB"
@@ -32,6 +39,12 @@ export function DiskSizeField<
       min={minSize}
       max={MAX_DISK_SIZE_GiB}
       validate={(diskSizeGiB) => {
+        if (Number.isNaN(diskSizeGiB)) {
+          return 'Disk size is required'
+        }
+        if (imageSize && diskSizeGiB < imageSize) {
+          return `Must be as large as selected image (min. ${imageSize} GiB)`
+        }
         if (diskSizeGiB < minSize) {
           return `Must be at least ${minSize} GiB`
         }
diff --git a/app/components/form/fields/NumberField.tsx b/app/components/form/fields/NumberField.tsx
index 238e88b11..3e3633d28 100644
--- a/app/components/form/fields/NumberField.tsx
+++ b/app/components/form/fields/NumberField.tsx
@@ -77,7 +77,7 @@ export const NumberFieldInner = <
       name={name}
       control={control}
       rules={{ required, validate }}
-      render={({ field: { value, ...fieldRest }, fieldState: { error } }) => {
+      render={({ field, fieldState: { error } }) => {
         return (
           <>
             <UINumberField
@@ -87,8 +87,7 @@ export const NumberFieldInner = <
                 [`${id}-help-text`]: !!description,
               })}
               aria-describedby={description ? `${id}-label-tip` : undefined}
-              defaultValue={value}
-              {...fieldRest}
+              {...field}
             />
             <ErrorMessage error={error} label={label} />
           </>
diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx
index 18c837e13..cd624d509 100644
--- a/app/forms/instance-create.tsx
+++ b/app/forms/instance-create.tsx
@@ -14,7 +14,6 @@ import {
   genName,
   INSTANCE_MAX_CPU,
   INSTANCE_MAX_RAM_GiB,
-  MAX_DISK_SIZE_GiB,
   useApiMutation,
   useApiQueryClient,
   usePrefetchedApiQuery,
@@ -140,6 +139,7 @@ export function CreateInstanceForm() {
 
   return (
     <FullPageForm
+      submitDisabled={allImages.length ? undefined : 'Image required'}
       id="create-instance-form"
       form={form}
       title="Create instance"
@@ -151,9 +151,10 @@ export function CreateInstanceForm() {
           values.presetId === 'custom'
             ? { memory: values.memory, ncpus: values.ncpus }
             : { memory: preset.memory, ncpus: preset.ncpus }
-
-        // we should also never have an image ID that's not in the list
         const image = allImages.find((i) => values.image === i.id)
+        // There should always be an image present, because …
+        // - The form is disabled unless there are images available.
+        // - The form defaults to including at least one image.
         invariant(image, 'Expected image to be defined')
 
         const bootDiskName = values.bootDiskName || genName(values.name, image.name)
@@ -287,37 +288,51 @@ export function CreateInstanceForm() {
       <FormDivider />
 
       <Form.Heading id="boot-disk">Boot disk</Form.Heading>
-      <Tabs.Root id="boot-disk-tabs" className="full-width" defaultValue="project">
+      <Tabs.Root
+        id="boot-disk-tabs"
+        className="full-width"
+        // default to the project images tab if there are only project images
+        defaultValue={
+          projectImages.length > 0 && siloImages.length === 0 ? 'project' : 'silo'
+        }
+      >
         <Tabs.List aria-describedby="boot-disk">
-          <Tabs.Trigger value="project">Project images</Tabs.Trigger>
           <Tabs.Trigger value="silo">Silo images</Tabs.Trigger>
+          <Tabs.Trigger value="project">Project images</Tabs.Trigger>
         </Tabs.List>
-        <Tabs.Content value="project" className="space-y-4">
-          {projectImages.length === 0 ? (
+        {allImages.length === 0 && (
+          <Message
+            className="mb-8 ml-10 max-w-lg"
+            variant="notice"
+            content="Images are required to create a boot disk."
+          />
+        )}
+        <Tabs.Content value="silo" className="space-y-4">
+          {siloImages.length === 0 ? (
             <div className="flex max-w-lg items-center justify-center rounded-lg border p-6 border-default">
               <EmptyMessage
                 icon={<Images16Icon />}
-                title="No project images found"
-                body="An image needs to be uploaded to be seen here"
-                buttonText="Upload image"
-                onClick={() => navigate(pb.projectImageNew(projectSelector))}
+                title="No silo images found"
+                body="Project images need to be promoted to be seen here"
               />
             </div>
           ) : (
-            <ImageSelectField images={projectImages} control={control} />
+            <ImageSelectField images={siloImages} control={control} />
           )}
         </Tabs.Content>
-        <Tabs.Content value="silo" className="space-y-4">
-          {siloImages.length === 0 ? (
+        <Tabs.Content value="project" className="space-y-4">
+          {projectImages.length === 0 ? (
             <div className="flex max-w-lg items-center justify-center rounded-lg border p-6 border-default">
               <EmptyMessage
                 icon={<Images16Icon />}
-                title="No silo images found"
-                body="Project images need to be promoted to be seen here"
+                title="No project images found"
+                body="An image needs to be uploaded to be seen here"
+                buttonText="Upload image"
+                onClick={() => navigate(pb.projectImageNew(projectSelector))}
               />
             </div>
           ) : (
-            <ImageSelectField images={siloImages} control={control} />
+            <ImageSelectField images={projectImages} control={control} />
           )}
         </Tabs.Content>
       </Tabs.Root>
@@ -328,17 +343,7 @@ export function CreateInstanceForm() {
         label="Disk size"
         name="bootDiskSize"
         control={control}
-        // Imitate API logic: only require that the disk is big enough to fit the image
-        validate={(diskSizeGiB) => {
-          if (!image) return true
-          if (diskSizeGiB < image.size / GiB) {
-            const minSize = Math.ceil(image.size / GiB)
-            return `Must be as large as selected image (min. ${minSize} GiB)`
-          }
-          if (diskSizeGiB > MAX_DISK_SIZE_GiB) {
-            return `Can be at most ${MAX_DISK_SIZE_GiB} GiB`
-          }
-        }}
+        imageSize={image?.size ? Math.ceil(image.size / GiB) : undefined}
       />
       <NameField
         name="bootDiskName"
diff --git a/app/test/e2e/instance-create.e2e.ts b/app/test/e2e/instance-create.e2e.ts
index 34cf965dd..e0b96c471 100644
--- a/app/test/e2e/instance-create.e2e.ts
+++ b/app/test/e2e/instance-create.e2e.ts
@@ -128,6 +128,43 @@ test('can create an instance with custom hardware', async ({ page }) => {
   ])
 })
 
+test('automatically updates disk size when larger image selected', async ({ page }) => {
+  await page.goto('/projects/mock-project/instances-new')
+
+  const instanceName = 'my-new-instance'
+  await page.fill('input[name=name]', instanceName)
+
+  // set the disk size larger than it needs to be, to verify it doesn't get reduced
+  const diskSizeInput = page.getByRole('textbox', { name: 'Disk size (GiB)' })
+  await diskSizeInput.fill('5')
+
+  // pick a disk image that's smaller than 5GiB (the first project image works [4GiB])
+  await page.getByRole('tab', { name: 'Project images' }).click()
+  await page.getByRole('button', { name: 'Image' }).click()
+  await page.getByRole('option', { name: images[0].name }).click()
+
+  // test that it still says 5, as that's larger than the given image
+  await expect(diskSizeInput).toHaveValue('5')
+
+  // pick a disk image that's larger than 5GiB (the third project image works [6GiB])
+  await page.getByRole('button', { name: 'Image' }).click()
+  await page.getByRole('option', { name: images[2].name }).click()
+
+  // test that it has been automatically increased to next-largest incremement of 10
+  await expect(diskSizeInput).toHaveValue('10')
+
+  // pick another image, just to verify that the diskSizeInput stays as it was
+  await page.getByRole('button', { name: 'Image' }).click()
+  await page.getByRole('option', { name: images[1].name }).click()
+  await expect(diskSizeInput).toHaveValue('10')
+
+  const submitButton = page.getByRole('button', { name: 'Create instance' })
+  await submitButton.click()
+
+  await expect(page).toHaveURL(`/projects/mock-project/instances/${instanceName}/storage`)
+  await expectVisible(page, [`h1:has-text("${instanceName}")`, 'text=10 GiB'])
+})
+
 test('with disk name already taken', async ({ page }) => {
   await page.goto('/projects/mock-project/instances-new')
   await page.fill('input[name=name]', 'my-instance')

From 688522041b9b3f2f116ae98d0fc1e67bf193391b Mon Sep 17 00:00:00 2001
From: Charlie Park <charlie@oxidecomputer.com>
Date: Wed, 6 Dec 2023 14:47:38 -0800
Subject: [PATCH 2/5] Refactor DiskSizeField so that it has both default
 validators and can optionally receive additional validators

---
 app/components/form/fields/DiskSizeField.tsx | 17 +++++++++++------
 app/forms/instance-create.tsx                |  8 +++++++-
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/app/components/form/fields/DiskSizeField.tsx b/app/components/form/fields/DiskSizeField.tsx
index b8098c408..7d97e3ca5 100644
--- a/app/components/form/fields/DiskSizeField.tsx
+++ b/app/components/form/fields/DiskSizeField.tsx
@@ -5,7 +5,12 @@
  *
  * Copyright Oxide Computer Company
  */
-import type { FieldPath, FieldPathByValue, FieldValues } from 'react-hook-form'
+import type {
+  FieldPath,
+  FieldPathByValue,
+  FieldValues,
+  ValidateResult,
+} from 'react-hook-form'
 
 import { MAX_DISK_SIZE_GiB } from '@oxide/api'
 
@@ -16,8 +21,8 @@ interface DiskSizeProps<
   TFieldValues extends FieldValues,
   TName extends FieldPath<TFieldValues>,
 > extends TextFieldProps<TFieldValues, TName> {
-  imageSize?: number
   minSize?: number
+  validate?(diskSizeGiB: number): ValidateResult
 }
 
 export function DiskSizeField<
@@ -26,8 +31,8 @@ export function DiskSizeField<
 >({
   required = true,
   name,
-  imageSize,
   minSize = 1,
+  validate,
   ...props
 }: DiskSizeProps<TFieldValues, TName>) {
   return (
@@ -39,18 +44,18 @@ export function DiskSizeField<
       min={minSize}
       max={MAX_DISK_SIZE_GiB}
       validate={(diskSizeGiB) => {
+        // Run a number of default validators
         if (Number.isNaN(diskSizeGiB)) {
           return 'Disk size is required'
         }
-        if (imageSize && diskSizeGiB < imageSize) {
-          return `Must be as large as selected image (min. ${imageSize} GiB)`
-        }
         if (diskSizeGiB < minSize) {
           return `Must be at least ${minSize} GiB`
         }
         if (diskSizeGiB > MAX_DISK_SIZE_GiB) {
           return `Can be at most ${MAX_DISK_SIZE_GiB} GiB`
         }
+        // Run any additional validators passed in from the callsite
+        return validate?.(diskSizeGiB)
       }}
       {...props}
     />
diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx
index cd624d509..444ba4842 100644
--- a/app/forms/instance-create.tsx
+++ b/app/forms/instance-create.tsx
@@ -136,6 +136,7 @@ export function CreateInstanceForm() {
 
   const imageInput = useWatch({ control: control, name: 'image' })
   const image = allImages.find((i) => i.id === imageInput)
+  const imageSize = image?.size ? Math.ceil(image.size / GiB) : undefined
 
   return (
     <FullPageForm
@@ -343,7 +344,12 @@ export function CreateInstanceForm() {
         label="Disk size"
         name="bootDiskSize"
         control={control}
-        imageSize={image?.size ? Math.ceil(image.size / GiB) : undefined}
+        validate={(diskSizeGiB: number) => {
+          // ensure the created disk is larger than the selected image
+          if (imageSize && diskSizeGiB < imageSize) {
+            return `Must be as large as selected image (min. ${imageSize} GiB)`
+          }
+        }}
       />
       <NameField
         name="bootDiskName"

From 74f5a22d0184beb1315f376a12397e264d4d4535 Mon Sep 17 00:00:00 2001
From: Charlie Park <charlie@oxidecomputer.com>
Date: Wed, 6 Dec 2023 14:51:48 -0800
Subject: [PATCH 3/5] Remove unnecessary comment

---
 app/forms/instance-create.tsx | 1 -
 1 file changed, 1 deletion(-)

diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx
index 444ba4842..2389e133a 100644
--- a/app/forms/instance-create.tsx
+++ b/app/forms/instance-create.tsx
@@ -345,7 +345,6 @@ export function CreateInstanceForm() {
         name="bootDiskSize"
         control={control}
         validate={(diskSizeGiB: number) => {
-          // ensure the created disk is larger than the selected image
           if (imageSize && diskSizeGiB < imageSize) {
             return `Must be as large as selected image (min. ${imageSize} GiB)`
           }

From ca5abc27f59e1cc9c2eb9464cc32802a723af2fd Mon Sep 17 00:00:00 2001
From: Charlie Park <charlie@oxidecomputer.com>
Date: Wed, 6 Dec 2023 17:19:45 -0800
Subject: [PATCH 4/5] Add validation on Disk Create for building off an image,
 that disk size > selected image size

---
 app/forms/disk-create.tsx | 75 +++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 18 deletions(-)

diff --git a/app/forms/disk-create.tsx b/app/forms/disk-create.tsx
index 2f231d835..4d413aa9c 100644
--- a/app/forms/disk-create.tsx
+++ b/app/forms/disk-create.tsx
@@ -17,9 +17,10 @@ import {
   type Disk,
   type DiskCreate,
   type DiskSource,
+  type Image,
 } from '@oxide/api'
 import { FieldLabel, FormDivider, Radio, RadioGroup } from '@oxide/ui'
-import { GiB } from '@oxide/util'
+import { bytesToGiB, GiB } from '@oxide/util'
 
 import {
   DescriptionField,
@@ -79,6 +80,19 @@ export function CreateDiskSideModalForm({
   })
 
   const form = useForm({ defaultValues })
+  const control = form.control
+  const { project } = useProjectSelector()
+  const projectImages = useApiQuery('imageList', { query: { project } })
+  const siloImages = useApiQuery('imageList', {})
+
+  // put project images first because if there are any, there probably aren't
+  // very many and they're probably relevant
+  const images = [...(projectImages.data?.items || []), ...(siloImages.data?.items || [])]
+  const areImagesLoading = projectImages.isPending || siloImages.isPending
+
+  const selectedImageId = control?._formValues?.diskSource?.imageId
+  const selectedImageSize = images.find((image) => image.id === selectedImageId)?.size
+  const imageSize = selectedImageSize ? bytesToGiB(selectedImageSize) : undefined
 
   return (
     <SideModalForm
@@ -93,16 +107,36 @@ export function CreateDiskSideModalForm({
       loading={createDisk.isPending}
       submitError={createDisk.error}
     >
-      <NameField name="name" control={form.control} />
-      <DescriptionField name="description" control={form.control} />
+      <NameField name="name" control={control} />
+      <DescriptionField name="description" control={control} />
       <FormDivider />
-      <DiskSourceField control={form.control} />
-      <DiskSizeField name="size" control={form.control} />
+      <DiskSourceField
+        control={control}
+        images={images}
+        areImagesLoading={areImagesLoading}
+      />
+      <DiskSizeField
+        name="size"
+        control={control}
+        validate={(diskSizeGiB: number) => {
+          if (imageSize && diskSizeGiB < imageSize) {
+            return `Must be as large as selected image (min. ${imageSize} GiB)`
+          }
+        }}
+      />
     </SideModalForm>
   )
 }
 
-const DiskSourceField = ({ control }: { control: Control<DiskCreate> }) => {
+const DiskSourceField = ({
+  control,
+  images,
+  areImagesLoading,
+}: {
+  control: Control<DiskCreate>
+  images: Image[]
+  areImagesLoading: boolean
+}) => {
   const {
     field: { value, onChange },
   } = useController({ control, name: 'diskSource' })
@@ -144,7 +178,13 @@ const DiskSourceField = ({ control }: { control: Control<DiskCreate> }) => {
             ]}
           />
         )}
-        {value.type === 'image' && <ImageSelectField control={control} />}
+        {value.type === 'image' && (
+          <ImageSelectField
+            control={control}
+            images={images}
+            areImagesLoading={areImagesLoading}
+          />
+        )}
 
         {value.type === 'snapshot' && <SnapshotSelectField control={control} />}
       </div>
@@ -152,23 +192,22 @@ const DiskSourceField = ({ control }: { control: Control<DiskCreate> }) => {
   )
 }
 
-const ImageSelectField = ({ control }: { control: Control<DiskCreate> }) => {
-  const { project } = useProjectSelector()
-
-  const projectImages = useApiQuery('imageList', { query: { project } })
-  const siloImages = useApiQuery('imageList', {})
-
-  // put project images first because if there are any, there probably aren't
-  // very many and they're probably relevant
-  const images = [...(projectImages.data?.items || []), ...(siloImages.data?.items || [])]
-
+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={projectImages.isPending || siloImages.isPending}
+      isLoading={areImagesLoading}
       items={images.map((i) => toListboxItem(i, true))}
       required
     />

From 093d59a56cbcf78207965dfd01089648cc9e73d8 Mon Sep 17 00:00:00 2001
From: Charlie Park <charlie@oxidecomputer.com>
Date: Wed, 6 Dec 2023 21:15:23 -0800
Subject: [PATCH 5/5] Refactor, plus use proper react-hook-form syntax

---
 app/forms/disk-create.tsx | 55 ++++++++++++++-------------------------
 1 file changed, 20 insertions(+), 35 deletions(-)

diff --git a/app/forms/disk-create.tsx b/app/forms/disk-create.tsx
index 4d413aa9c..536998043 100644
--- a/app/forms/disk-create.tsx
+++ b/app/forms/disk-create.tsx
@@ -6,6 +6,7 @@
  * Copyright Oxide Computer Company
  */
 import { format } from 'date-fns'
+import { useMemo } from 'react'
 import { useController, type Control } from 'react-hook-form'
 import { useNavigate, type NavigateFunction } from 'react-router-dom'
 
@@ -80,19 +81,21 @@ export function CreateDiskSideModalForm({
   })
 
   const form = useForm({ defaultValues })
-  const control = form.control
   const { project } = useProjectSelector()
   const projectImages = useApiQuery('imageList', { query: { project } })
   const siloImages = useApiQuery('imageList', {})
 
   // put project images first because if there are any, there probably aren't
   // very many and they're probably relevant
-  const images = [...(projectImages.data?.items || []), ...(siloImages.data?.items || [])]
+  const images = useMemo(
+    () => [...(projectImages.data?.items || []), ...(siloImages.data?.items || [])],
+    [projectImages.data, siloImages.data]
+  )
   const areImagesLoading = projectImages.isPending || siloImages.isPending
 
-  const selectedImageId = control?._formValues?.diskSource?.imageId
+  const selectedImageId = form.watch('diskSource.imageId')
   const selectedImageSize = images.find((image) => image.id === selectedImageId)?.size
-  const imageSize = selectedImageSize ? bytesToGiB(selectedImageSize) : undefined
+  const imageSizeGiB = selectedImageSize ? bytesToGiB(selectedImageSize) : undefined
 
   return (
     <SideModalForm
@@ -107,20 +110,20 @@ export function CreateDiskSideModalForm({
       loading={createDisk.isPending}
       submitError={createDisk.error}
     >
-      <NameField name="name" control={control} />
-      <DescriptionField name="description" control={control} />
+      <NameField name="name" control={form.control} />
+      <DescriptionField name="description" control={form.control} />
       <FormDivider />
       <DiskSourceField
-        control={control}
+        control={form.control}
         images={images}
         areImagesLoading={areImagesLoading}
       />
       <DiskSizeField
         name="size"
-        control={control}
+        control={form.control}
         validate={(diskSizeGiB: number) => {
-          if (imageSize && diskSizeGiB < imageSize) {
-            return `Must be as large as selected image (min. ${imageSize} GiB)`
+          if (imageSizeGiB && diskSizeGiB < imageSizeGiB) {
+            return `Must be as large as selected image (min. ${imageSizeGiB} GiB)`
           }
         }}
       />
@@ -179,10 +182,14 @@ const DiskSourceField = ({
           />
         )}
         {value.type === 'image' && (
-          <ImageSelectField
+          <ListboxField
             control={control}
-            images={images}
-            areImagesLoading={areImagesLoading}
+            name="diskSource.imageId"
+            label="Source image"
+            placeholder="Select an image"
+            isLoading={areImagesLoading}
+            items={images.map((i) => toListboxItem(i, true))}
+            required
           />
         )}
 
@@ -192,28 +199,6 @@ const DiskSourceField = ({
   )
 }
 
-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',