Skip to content
73 changes: 50 additions & 23 deletions app/components/form/fields/ComboboxField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* Copyright Oxide Computer Company
*/

import { useState } from 'react'
import { useCallback, useMemo } from 'react'
import {
useController,
type Control,
Expand All @@ -25,7 +25,7 @@ import { capitalize } from '~/util/str'

import { ErrorMessage } from './ErrorMessage'

export type ComboboxFieldProps<
type ComboboxFieldProps<
TFieldValues extends FieldValues,
TName extends FieldPath<TFieldValues>,
> = {
Expand All @@ -45,20 +45,10 @@ export function ComboboxField<
label = capitalize(name),
required,
onChange,
onInputChange,
allowArbitraryValues,
placeholder,
// Intent is to not show both a placeholder and a description, while still having good defaults; prefer a description to a placeholder
/*
* If description is provided, use it.
* If not, but a placeholder is provided, the default description should be undefined.
* If no placeholder is provided and arbitrary values are allowed, the default description should be 'Select an option or enter a custom value'.
* If no placeholder is provided and arbitrary values are not allowed, the default description should be 'Select an option'.
*/
description = placeholder
? undefined
: allowArbitraryValues
? 'Select an option or enter a custom value'
: 'Select an option',
description,
items,
transform,
validate,
Expand All @@ -69,25 +59,62 @@ export function ComboboxField<
control,
rules: { required, validate },
})
const [selectedItemLabel, setSelectedItemLabel] = useState(
getSelectedLabelFromValue(items, field.value || '')

// Memoize description computation to avoid recalculating on every render
const memoizedDescription = useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be surprised if this memo has a meaningful effect on performance

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could even be more computation than not memoizing — the useMemo call and its dep checks are work too. The point of this would be to prevent the value changing to avoid triggering renders on <Combobox> below, but this is just a string and it wouldn't be changing its value between renders, so it is almost certainly triggering re-renders.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would this trigger a re-render – since description, placeholder and allowArbitraryValues are likely the same values across the lifetime of this component. So the prop on <Combobox> wouldn't change.

Copy link
Collaborator

@david-crespo david-crespo Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's what I meant about it being a string. If it was constructing an object or array with a spread like the one in disk create, then it would be a new one every time (even if the underlying values are the same) and then it would trigger renders when passed down the chain. So the other ones are more plausible, though in those cases it's still worth determining whether they have much effect. Triggering renders is not always a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the memoization explosion was from before I added in the virtualization; was going to try backing them out to see whether they were having a noticeable effect, but will read the other comments on this first

// Intent is to not show both a placeholder and a description, while still having good defaults; prefer a description to a placeholder
/*
* If description is provided, use it.
* If not, but a placeholder is provided, the default description should be undefined.
* If no placeholder is provided and arbitrary values are allowed, the default description should be 'Select an option or enter a custom value'.
* If no placeholder is provided and arbitrary values are not allowed, the default description should be 'Select an option'.
*/
return description !== undefined
? description
: placeholder
? undefined
: allowArbitraryValues
? 'Select an option or enter a custom value'
: 'Select an option'
}, [description, placeholder, allowArbitraryValues])

// Memoize the selected label calculation to avoid recalculation when items/value haven't changed
const selectedItemLabel = useMemo(
() => getSelectedLabelFromValue(items, field.value || ''),
[items, field.value]
)

// Memoize onChange callback to prevent unnecessary re-renders
const handleChange = useCallback(
(value: string) => {
field.onChange(value)
onChange?.(value)
},
[field, onChange]
)

// Memoize onInputChange callback to prevent unnecessary re-renders
const handleInputChange = useCallback(
(value: string) => {
// If the user edits the input field, if arbitrary values are allowed, set the field's value; otherwise, clear the selected value
field.onChange(allowArbitraryValues ? value : undefined)
onInputChange?.(value)
},
[field, allowArbitraryValues, onInputChange]
)
return (
<div className="max-w-lg">
<Combobox
label={label}
placeholder={placeholder}
description={description}
description={memoizedDescription}
items={items}
required={required}
selectedItemValue={field.value}
selectedItemValue={field.value || ''}
selectedItemLabel={selectedItemLabel}
hasError={fieldState.error !== undefined}
onChange={(value) => {
field.onChange(value)
onChange?.(value)
setSelectedItemLabel(getSelectedLabelFromValue(items, value))
}}
onChange={handleChange}
onInputChange={handleInputChange}
allowArbitraryValues={allowArbitraryValues}
inputRef={field.ref}
transform={transform}
Expand Down
73 changes: 51 additions & 22 deletions app/components/form/fields/ImageSelectField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*
* Copyright Oxide Computer Company
*/
import { useCallback, useMemo } from 'react'
import { useController, type Control } from 'react-hook-form'

import type { Image } from '@oxide/api'
Expand All @@ -31,6 +32,29 @@ export function BootDiskImageSelectField({
name,
}: ImageSelectFieldProps) {
const diskSizeField = useController({ control, name: 'bootDiskSize' }).field

// Memoize the expensive toImageComboboxItem mapping to avoid recalculating on every render
const imageComboboxItems = useMemo(
() => images.map((i) => toImageComboboxItem(i)),
[images]
)

// Memoize onChange callback to prevent function recreation on every render
const handleChange = useCallback(
(id: string) => {
const image = images.find((i) => i.id === id)
// the most likely scenario where image would be undefined is if the user has
// manually cleared the ComboboxField; they will need to pick a boot disk image
// in order to submit the form, so we don't need to do anything here
if (!image) return
const imageSizeGiB = image.size / GiB
if (diskSizeField.value < imageSizeGiB) {
diskSizeField.onChange(diskSizeNearest10(imageSizeGiB))
}
},
[images, diskSizeField]
)

return (
<ComboboxField
disabled={disabled}
Expand All @@ -40,42 +64,47 @@ export function BootDiskImageSelectField({
placeholder={
name === 'siloImageSource' ? 'Select a silo image' : 'Select a project image'
}
items={images.map((i) => toImageComboboxItem(i))}
items={imageComboboxItems}
required
onChange={(id) => {
const image = images.find((i) => i.id === id)
// the most likely scenario where image would be undefined is if the user has
// manually cleared the ComboboxField; they will need to pick a boot disk image
// in order to submit the form, so we don't need to do anything here
if (!image) return
const imageSizeGiB = image.size / GiB
if (diskSizeField.value < imageSizeGiB) {
diskSizeField.onChange(diskSizeNearest10(imageSizeGiB))
}
}}
onChange={(id) => handleChange(id || '')}
/>
)
}

// Memoize expensive bytesToGiB calculations
const sizeCache = new Map<number, string>()

function getCachedSizeGiB(size: number): string {
if (!sizeCache.has(size)) {
sizeCache.set(size, `${bytesToGiB(size, 1)} GiB`)
}
return sizeCache.get(size)!
}

export function toImageComboboxItem(
image: Image,
includeProjectSiloIndicator = false
): ComboboxItem {
const { id, name, os, projectId, size, version } = image

// for metadata showing in the dropdown's options, include the project / silo indicator if requested
const projectSiloIndicator = includeProjectSiloIndicator
? `${projectId ? 'Project' : 'Silo'} image`
: null
// filter out undefined metadata and create a `<Slash />`-separated list for each comboboxitem
const itemMetadata = [os, version, `${bytesToGiB(size, 1)} GiB`, projectSiloIndicator]
.filter((i) => !!i)
.map((i, index) => (
<span key={`${i}`}>
{index > 0 ? <Slash /> : ''}
{i}
</span>
))

// Build metadata array more efficiently
const metadata = []
if (os) metadata.push(os)
if (version) metadata.push(version)
if (size) metadata.push(getCachedSizeGiB(size))
if (projectSiloIndicator) metadata.push(projectSiloIndicator)

const itemMetadata = metadata.map((item, index) => (
<span key={item}>
{index > 0 && <Slash />}
{item}
</span>
))

return {
value: id,
selectedLabel: name,
Expand Down
111 changes: 81 additions & 30 deletions app/forms/disk-create.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ import {
type Image,
} from '@oxide/api'

import { ComboboxField } from '~/components/form/fields/ComboboxField'
import { DescriptionField } from '~/components/form/fields/DescriptionField'
import { DiskSizeField } from '~/components/form/fields/DiskSizeField'
import { toImageComboboxItem } from '~/components/form/fields/ImageSelectField'
import { ListboxField } from '~/components/form/fields/ListboxField'
import { NameField } from '~/components/form/fields/NameField'
import { RadioField } from '~/components/form/fields/RadioField'
import { SideModalForm } from '~/components/form/SideModalForm'
Expand Down Expand Up @@ -85,32 +85,66 @@ export function CreateDiskSideModalForm({
const projectImages = useApiQuery('imageList', { query: { project } })
const siloImages = useApiQuery('imageList', {})

// Memoize real images array to prevent recreation on every render
// put project images first because if there are any, there probably aren't
// very many and they're probably relevant
const images = useMemo(
const realImages = useMemo(
() => [...(projectImages.data?.items || []), ...(siloImages.data?.items || [])],
[projectImages.data, siloImages.data]
[projectImages.data?.items, siloImages.data?.items]
)

// Memoize mock images array (only create once)
const mockImages = useMemo((): Image[] => {
// TODO: REMOVE THIS AFTER STRESS TESTING IS DONE
// Generate 1000 mock items for stress testing
return Array.from({ length: 1000 }, (_, i) => ({
id: `mock-image-${i}`,
name: `Mock Image ${i.toString().padStart(4, '0')}`,
size: 1073741824, // 1GB
version: '1.0.0',
description: `This is mock image ${i} for stress testing the combobox`,
digest: {
type: 'sha256',
value: '0'.repeat(64),
},
blockSize: 512,
timeCreated: new Date(),
timeModified: new Date(),
os: 'linux',
}))
}, [])

// Memoize combined images array
const images = useMemo(() => [...realImages, ...mockImages], [realImages, mockImages])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again suspect an improvement is negligible here, suspect spreading two arrays is relatively inexpensive

const areImagesLoading = projectImages.isPending || siloImages.isPending

const snapshotsQuery = useApiQuery('snapshotList', { query: { project } })
const snapshots = snapshotsQuery.data?.items || []
const snapshots = useMemo(
() => snapshotsQuery.data?.items || [],
[snapshotsQuery.data?.items]
)

// validate disk source size
const diskSource = form.watch('diskSource').type
// Use useController for targeted watching instead of form.watch() to reduce re-renders
const diskSourceController = useController({ control: form.control, name: 'diskSource' })
const diskSource = diskSourceController.field.value.type

let validateSizeGiB: number | undefined = undefined
if (diskSource === 'snapshot') {
const selectedSnapshotId = form.watch('diskSource.snapshotId')
const selectedSnapshotSize = snapshots.find(
(snapshot) => snapshot.id === selectedSnapshotId
)?.size
validateSizeGiB = selectedSnapshotSize ? bytesToGiB(selectedSnapshotSize) : undefined
} else if (diskSource === 'image') {
const selectedImageId = form.watch('diskSource.imageId')
const selectedImageSize = images.find((image) => image.id === selectedImageId)?.size
validateSizeGiB = selectedImageSize ? bytesToGiB(selectedImageSize) : undefined
}
// Memoize size validation to avoid expensive lookups on every render
const validateSizeGiB = useMemo(() => {
if (diskSource === 'snapshot') {
const selectedSnapshotId = diskSourceController.field.value.snapshotId
if (!selectedSnapshotId) return undefined
const selectedSnapshotSize = snapshots.find(
(snapshot) => snapshot.id === selectedSnapshotId
)?.size
return selectedSnapshotSize ? bytesToGiB(selectedSnapshotSize) : undefined
} else if (diskSource === 'image') {
const selectedImageId = diskSourceController.field.value.imageId
if (!selectedImageId) return undefined
const selectedImageSize = images.find((image) => image.id === selectedImageId)?.size
return selectedImageSize ? bytesToGiB(selectedImageSize) : undefined
}
return undefined
}, [diskSource, diskSourceController.field.value, snapshots, images])

return (
<SideModalForm
Expand Down Expand Up @@ -172,6 +206,12 @@ const DiskSourceField = ({
} = useController({ control, name: 'diskSource' })
const diskSizeField = useController({ control, name: 'size' }).field

// Memoize the expensive toImageComboboxItem mapping to avoid recalculating on every render
const imageComboboxItems = useMemo(
() => images.map((i) => toImageComboboxItem(i, true)),
[images]
)

return (
<>
<div className="max-w-lg space-y-2">
Expand Down Expand Up @@ -210,16 +250,17 @@ const DiskSourceField = ({
/>
)}
{value.type === 'image' && (
<ListboxField
<ComboboxField
control={control}
name="diskSource.imageId"
label="Source image"
placeholder="Select an image"
isLoading={areImagesLoading}
items={images.map((i) => toImageComboboxItem(i, true))}
items={imageComboboxItems}
required
onChange={(id) => {
const image = images.find((i) => i.id === id)! // if it's selected, it must be present
const image = images.find((i) => i.id === id)
if (!image) return
const imageSizeGiB = image.size / GiB
if (diskSizeField.value < imageSizeGiB) {
diskSizeField.onChange(diskSizeNearest10(imageSizeGiB))
Expand Down Expand Up @@ -250,16 +291,16 @@ const SnapshotSelectField = ({ control }: { control: Control<DiskCreate> }) => {
const { project } = useProjectSelector()
const snapshotsQuery = useApiQuery('snapshotList', { query: { project } })

const snapshots = snapshotsQuery.data?.items || []
const snapshots = useMemo(
() => snapshotsQuery.data?.items || [],
[snapshotsQuery.data?.items]
)
const diskSizeField = useController({ control, name: 'size' }).field

return (
<ListboxField
control={control}
name="diskSource.snapshotId"
label="Source snapshot"
placeholder="Select a snapshot"
items={snapshots.map((i) => {
// Memoize the expensive snapshot ComboboxItem mapping to avoid recalculating on every render
const snapshotComboboxItems = useMemo(
() =>
snapshots.map((i) => {
const formattedSize = filesize(i.size, { base: 2, output: 'object' })
return {
value: i.id,
Expand All @@ -275,7 +316,17 @@ const SnapshotSelectField = ({ control }: { control: Control<DiskCreate> }) => {
</>
),
}
})}
}),
[snapshots]
)

return (
<ComboboxField
control={control}
name="diskSource.snapshotId"
label="Source snapshot"
placeholder="Select a snapshot"
items={snapshotComboboxItems}
isLoading={snapshotsQuery.isPending}
required
onChange={(id) => {
Expand Down
Loading
Loading