From 5a2e2be38f693f5ac62b89fc64cf5f2a2dcfea89 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Mon, 18 May 2026 14:26:26 -0700 Subject: [PATCH 1/2] Add virtualization via Headless UI's virtual prop --- app/ui/lib/Combobox.tsx | 170 ++++++++++++++++++-------------- test/e2e/instance-create.e2e.ts | 7 +- 2 files changed, 102 insertions(+), 75 deletions(-) diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index 0abe2b54c..687e6a888 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -15,7 +15,15 @@ import { } from '@headlessui/react' import cn from 'classnames' import { matchSorter } from 'match-sorter' -import { useEffect, useId, useRef, useState, type ReactNode, type Ref } from 'react' +import { + useEffect, + useId, + useMemo, + useRef, + useState, + type ReactNode, + type Ref, +} from 'react' import { SelectArrows6Icon } from '@oxide/design-system/icons/react' @@ -25,6 +33,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). @@ -107,27 +131,15 @@ export const Combobox = ({ 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 +155,56 @@ 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 = useMemo(() => { + 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 + }, [items, selectedItemValue, allowArbitraryValues]) + 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 + virtual={{ + options: virtualOptions, + // HUI types this with the same union as `value`, so item may be null + disabled: (item) => item?.value === NO_MATCH_VALUE, + }} {...props} > {({ open }) => { - // Sync open state to ref on render (handles the opening side) if (open) isOpenRef.current = true return (
@@ -201,19 +234,15 @@ 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} > { - // 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 +288,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') From 928eaa373c0ab386a6c9f0d2ef3efbeca1e6fbd2 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Wed, 20 May 2026 14:11:02 -0700 Subject: [PATCH 2/2] refactoring --- app/components/form/fields/ComboboxField.tsx | 12 +-------- app/ui/lib/Combobox.tsx | 27 ++++---------------- 2 files changed, 6 insertions(+), 33 deletions(-) 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 687e6a888..1e308bc61 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -15,15 +15,7 @@ import { } from '@headlessui/react' import cn from 'classnames' import { matchSorter } from 'match-sorter' -import { - useEffect, - useId, - useMemo, - useRef, - useState, - type ReactNode, - type Ref, -} from 'react' +import { useEffect, useId, useRef, useState, type ReactNode, type Ref } from 'react' import { SelectArrows6Icon } from '@oxide/design-system/icons/react' @@ -60,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 @@ -96,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 @@ -109,7 +95,6 @@ export const Combobox = ({ items = [], label, selectedItemValue, - selectedItemLabel, placeholder, required, hasError, @@ -122,10 +107,9 @@ 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 @@ -159,7 +143,7 @@ export const Combobox = ({ filteredItems.length === 0 && !allowArbitraryValues ? [NO_MATCH_ITEM] : filteredItems // Arbitrary values may not be in `items`, so synthesize a stand-in. - const selectedItem: ComboboxItem | null = useMemo(() => { + const selectedItem: ComboboxItem | null = (() => { if (!selectedItemValue) return null const found = items.find((i) => i.value === selectedItemValue) if (found) return found @@ -171,7 +155,7 @@ export const Combobox = ({ } } return null - }, [items, selectedItemValue, allowArbitraryValues]) + })() const zIndex = usePopoverZIndex() const id = useId() @@ -202,7 +186,6 @@ export const Combobox = ({ // HUI types this with the same union as `value`, so item may be null disabled: (item) => item?.value === NO_MATCH_VALUE, }} - {...props} > {({ open }) => { if (open) isOpenRef.current = true @@ -247,7 +230,7 @@ export const Combobox = ({ selectedItemValue ? allowArbitraryValues ? selectedItemValue - : selectedItemLabel + : (selectedItem?.selectedLabel ?? '') : query } onChange={(event) => {