diff --git a/app/components/form/fields/ComboboxField.tsx b/app/components/form/fields/ComboboxField.tsx index efa055829..e2c71bd4a 100644 --- a/app/components/form/fields/ComboboxField.tsx +++ b/app/components/form/fields/ComboboxField.tsx @@ -6,7 +6,6 @@ * Copyright Oxide Computer Company */ -import { useState } from 'react' import { useController, type Control, @@ -16,11 +15,7 @@ import { type Validate, } from 'react-hook-form' -import { - Combobox, - getSelectedLabelFromValue, - type ComboboxBaseProps, -} from '~/ui/lib/Combobox' +import { Combobox, type ComboboxBaseProps } from '~/ui/lib/Combobox' import { capitalize } from '~/util/str' import { ErrorMessage } from './ErrorMessage' @@ -69,9 +64,6 @@ export function ComboboxField< control, rules: { required, validate }, }) - const [selectedItemLabel, setSelectedItemLabel] = useState( - getSelectedLabelFromValue(items, field.value || '') - ) return (
{ field.onChange(value) onChange?.(value) - setSelectedItemLabel(getSelectedLabelFromValue(items, value)) }} allowArbitraryValues={allowArbitraryValues} inputRef={field.ref} diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index 0abe2b54c..a26e598e9 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -25,6 +25,18 @@ import { TextInputHint } from './TextInput' export type ComboboxItem = { value: string; label: ReactNode; selectedLabel: string } +// use a pseudo sentinel option when 0 matches, to play nice with Headless UI's virtualization +const NO_MATCH_ITEM: ComboboxItem = { + value: '__combobox_no_match__', + label: 'No items match', + selectedLabel: '', +} +const isNoMatch = (item: ComboboxItem | null) => item === NO_MATCH_ITEM + +// HUI's virtualizer needs the scroll container to have a non-zero height +const ITEM_HEIGHT = 40 +const MAX_PANEL_HEIGHT = 280 + /** Convert an array of items with a `name` attribute to an array of ComboboxItems * Useful when the rendered label and value are the same; in more complex cases, * you may want to create a custom ComboboxItem object (see toImageComboboxItem). @@ -36,11 +48,6 @@ export const toComboboxItems = (items?: Array<{ name: string }>): Array, - selectedValue: string -): string => items.find((item) => item.value === selectedValue)?.selectedLabel || '' - /** Simple non-generic props shared with ComboboxField */ export type ComboboxBaseProps = { description?: React.ReactNode @@ -72,7 +79,6 @@ export type ComboboxBaseProps = { type ComboboxProps = { selectedItemValue: string - selectedItemLabel: string hasError?: boolean /** Fires when the user *selects* an item from the list */ onChange: (value: string) => void @@ -85,7 +91,6 @@ export const Combobox = ({ items = [], label, selectedItemValue, - selectedItemLabel, placeholder, required, hasError, @@ -98,12 +103,11 @@ export const Combobox = ({ hideOptionalTag, inputRef, transform, - ...props }: ComboboxProps) => { const [query, setQuery] = useState(selectedItemValue || '') - const q = query.toLowerCase().replace(/\s*/g, '') + const q = query.toLowerCase().replace(/\s+/g, '') const filteredItems = matchSorter(items, q, { - keys: ['selectedLabel', 'label'], + keys: ['selectedLabel'], sorter: (items) => items, // preserve original order, don't sort by match }) @@ -143,6 +147,26 @@ export const Combobox = ({ selectedLabel: query, }) } + + const virtualOptions: ComboboxItem[] = + filteredItems.length === 0 && !allowArbitraryValues ? [NO_MATCH_ITEM] : filteredItems + const minHeight = Math.min(virtualOptions.length * ITEM_HEIGHT, MAX_PANEL_HEIGHT) + + // Arbitrary values may not be in `items`, so synthesize a stand-in. + const selectedItem: ComboboxItem | null = (() => { + if (!selectedItemValue) return null + const found = items.find((i) => i.value === selectedItemValue) + if (found) return found + if (allowArbitraryValues) { + return { + value: selectedItemValue, + label: selectedItemValue, + selectedLabel: selectedItemValue, + } + } + return null + })() + const zIndex = usePopoverZIndex() const id = useId() // Tracks whether the dropdown is open so the onKeyDown handler can @@ -158,17 +182,18 @@ export const Combobox = ({ const isOpenRef = useRef(false) return ( onChange(val || '')} + onChange={(item) => onChange(item?.value ?? '')} onClose={() => { isOpenRef.current = false if (!allowArbitraryValues) setQuery('') }} disabled={disabled || isLoading} immediate - {...props} + virtual={{ options: virtualOptions, disabled: isNoMatch }} > {({ open }) => { // Sync open state to ref on render (handles the opening side) @@ -218,16 +243,14 @@ export const Combobox = ({ selectedItemValue ? allowArbitraryValues ? selectedItemValue - : selectedItemLabel + : (selectedItem?.selectedLabel ?? '') : query } onChange={(event) => { const value = transform ? transform(event.target.value) : event.target.value - // updates the query state as the user types, in order to filter the list of items setQuery(value) - // if the parent component wants to know about input changes, call the callback onInputChange?.(value) }} onKeyDown={(e) => { @@ -261,40 +284,40 @@ export const Combobox = ({ )}
- {(items.length > 0 || allowArbitraryValues) && ( + {(items.length > 0 || allowArbitraryValues) && virtualOptions.length > 0 && ( - {filteredItems.map((item) => ( - - {({ focus, selected }) => ( - // This *could* be done with data-[focus] and data-[selected] instead, but - // it would be a lot more verbose. those can only be used with TW classes, - // not our .is-selected and .is-highlighted, so we'd have to copy the pieces - // of those rules one by one. Better to rely on the shared classes. -
- {item.label} -
- )} -
- ))} - {!allowArbitraryValues && filteredItems.length === 0 && ( - -
No items match
-
- )} + {({ option }: { option: ComboboxItem }) => { + const noMatch = option === NO_MATCH_ITEM + return ( + + {({ focus, selected }) => ( + // This *could* be done with data-[focus] and data-[selected] instead, but + // it would be a lot more verbose. those can only be used with TW classes, + // not our .is-selected and .is-highlighted, so we'd have to copy the pieces + // of those rules one by one. Better to rely on the shared classes. +
+ {option.label} +
+ )} +
+ ) + }}
)} diff --git a/test/e2e/instance-create.e2e.ts b/test/e2e/instance-create.e2e.ts index e7ed4d785..04af72db0 100644 --- a/test/e2e/instance-create.e2e.ts +++ b/test/e2e/instance-create.e2e.ts @@ -566,9 +566,20 @@ test('attaching additional disks allows for combobox filtering', async ({ page } await attachExistingDiskButton.click() await selectADisk.click() - // several disks should be shown + // several disks should be shown in the visible window. the combobox + // virtualizes, so only the first ~20 options live in the DOM at once; + // aria-setsize reports the full attachable count. await expect(page.getByRole('option', { name: 'disk-0005' })).toBeVisible() await expect(page.getByRole('option', { name: 'disk-0007' })).toBeVisible() + await expect(page.getByRole('option').first()).toHaveAttribute('aria-setsize', /\d{2,}/) + + // Pressing End jumps the active option to the last entry, which forces + // the virtualizer to mount it. disk-0988 is the last detached + // (attachable) disk in the seeded set and isn't in the DOM on first + // open — toBeHidden confirms we're genuinely virtualizing rather than + // just rendering everything. + await expect(page.getByRole('option', { name: 'disk-0988' })).toBeHidden() + await selectADisk.press('End') await expect(page.getByRole('option', { name: 'disk-0988' })).toBeVisible() // type in a string to use as a filter @@ -580,12 +591,13 @@ test('attaching additional disks allows for combobox filtering', async ({ page } await expect(page.getByRole('option', { name: 'disk-0220' })).toBeHidden() await expect(page.getByRole('option', { name: 'disk-1000' })).toBeHidden() - // select one - await page.getByRole('option', { name: 'disk-0211' }).click() + // filter down to a single late-in-the-list disk and select it + await selectADisk.fill('disk-0988') + await page.getByRole('option', { name: 'disk-0988' }).click() // now options hidden and only the selected one is visible in the button/input await expect(page.getByRole('option')).toBeHidden() - await expect(page.getByRole('combobox', { name: 'Disk name' })).toHaveValue('disk-0211') + await expect(page.getByRole('combobox', { name: 'Disk name' })).toHaveValue('disk-0988') // a random string should give a disabled option await selectADisk.click()