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..1e308bc61 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -25,6 +25,22 @@ import { TextInputHint } from './TextInput' export type ComboboxItem = { value: string; label: ReactNode; selectedLabel: string } +// HUI's virtual render prop only accepts one child, so we surface "No items +// match" via a disabled sentinel option instead of a sibling element. +const NO_MATCH_VALUE = '__combobox_no_match__' +const NO_MATCH_ITEM: ComboboxItem = { + value: NO_MATCH_VALUE, + label: 'No items match', + selectedLabel: '', +} + +// HUI's virtualizer needs the scroll container to have a non-zero height +// before it'll render any items, but it renders no items until the +// container has a height. Setting an explicit height breaks the cycle. +// 40 matches HUI's internal estimateSize default. +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 +52,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 +83,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 +95,6 @@ export const Combobox = ({ items = [], label, selectedItemValue, - selectedItemLabel, placeholder, required, hasError, @@ -98,36 +107,23 @@ 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'], sorter: (items) => items, // preserve original order, don't sort by match }) - // In the arbitraryValues case, clear the query whenever the value is cleared. - // this is necessary, e.g., for the firewall rules form when you submit the - // targets subform and clear the field. Two possible changes we might want to make - // here if we run into issues: - // - // 1. do it all the time, not just in the arbitraryValues case - // 2. do it on all value changes, not just on clear - // - // Currently, I don't think there are any arbitraryValues=false cases where we - // set the value from outside. There is an arbitraryvalues=true case where we - // setValue to something other than empty string, but we don't need the - // sync because that setValue is done in onInputChange and we already are - // doing setQuery in here along with it. + // Clear the query when the parent clears the value (e.g. firewall rules + // form on subform submit). Only needed for allowArbitraryValues; without + // it, parent-driven clears leave the input still showing the old query. useEffect(() => { if (allowArbitraryValues && !selectedItemValue) { setQuery('') } }, [allowArbitraryValues, selectedItemValue]) - // If the user has typed in a value that isn't in the list, - // add it as an option if `allowArbitraryValues` is true if ( allowArbitraryValues && query.length > 0 && @@ -143,35 +139,55 @@ export const Combobox = ({ selectedLabel: query, }) } + const virtualOptions: ComboboxItem[] = + filteredItems.length === 0 && !allowArbitraryValues ? [NO_MATCH_ITEM] : filteredItems + + // 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 - // distinguish Enter-to-select (dropdown open, let HUI handle it) from - // Enter-to-submit (dropdown closed, fire onEnter). We use a ref instead - // of HUI's `open` render prop because our handler runs before HUI's - // (useRender merges user props first) and the render prop can be stale. - // The ref stays current because onClose sets it synchronously during - // HUI's own keydown handler. With the stale render prop, the handler - // could see the combobox as closed one keydown too late, firing onEnter - // instead of letting HUI select — hard to notice manually but caused - // consistent e2e flakes in Firefox. + // Lets onKeyDown distinguish Enter-to-select (open) from Enter-to-submit + // (closed). HUI's `open` render prop is stale by one keydown in our + // handler ordering — caused Firefox e2e flakes. const isOpenRef = useRef(false) return ( onChange(val || '')} + // items are re-created each render, so compare by value field + by="value" + value={selectedItem} + onChange={(item) => { + if (!item || item.value === NO_MATCH_VALUE) { + onChange('') + return + } + onChange(item.value) + }} onClose={() => { isOpenRef.current = false if (!allowArbitraryValues) setQuery('') }} disabled={disabled || isLoading} immediate - {...props} + virtual={{ + options: virtualOptions, + // HUI types this with the same union as `value`, so item may be null + disabled: (item) => item?.value === NO_MATCH_VALUE, + }} > {({ open }) => { - // Sync open state to ref on render (handles the opening side) if (open) isOpenRef.current = true return (
@@ -201,39 +217,30 @@ export const Combobox = ({ : 'bg-default', disabled && hasError && 'border-error-secondary!' )} - // Putting the inputRef on the div makes it so the div can be focused by RHF when there's an error. - // We want to focus on the div (rather than the input) so the combobox doesn't open automatically - // and obscure the error message. + // ref on the wrapper, not the input, so RHF can focus on + // error without opening the dropdown and hiding the message ref={inputRef} - // tabIndex=-1 is necessary to make the div focusable tabIndex={-1} > { 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) => { - // When the dropdown is open, Enter should select the - // highlighted option (HUI handles this). When closed, - // Enter should submit the subform via onEnter. if (e.key === 'Enter' && !isOpenRef.current) { e.preventDefault() onEnter?.(e) @@ -264,37 +271,37 @@ export const Combobox = ({ {(items.length > 0 || allowArbitraryValues) && ( - {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 isNoMatch = option.value === NO_MATCH_VALUE + return ( + + {({ focus, selected }) => ( +
+ {option.label} +
+ )} + + ) + }} )}
diff --git a/test/e2e/instance-create.e2e.ts b/test/e2e/instance-create.e2e.ts index e7ed4d785..e9d1f0500 100644 --- a/test/e2e/instance-create.e2e.ts +++ b/test/e2e/instance-create.e2e.ts @@ -566,10 +566,13 @@ 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 (in the hundreds for + // this seeded dataset). await expect(page.getByRole('option', { name: 'disk-0005' })).toBeVisible() await expect(page.getByRole('option', { name: 'disk-0007' })).toBeVisible() - await expect(page.getByRole('option', { name: 'disk-0988' })).toBeVisible() + await expect(page.getByRole('option').first()).toHaveAttribute('aria-setsize', /\d{2,}/) // type in a string to use as a filter await selectADisk.fill('disk-02')