diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 000000000..bb88ceca0 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,16 @@ +{ + "hooks": { + "PostToolUse": [ + { + "matcher": "Edit|Write|NotebookEdit", + "hooks": [ + { + "type": "command", + "command": "jq -r '.tool_input.file_path' | xargs ./node_modules/.bin/prettier --write --ignore-unknown", + "timeout": 10 + } + ] + } + ] + } +} diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index 96aaa0eaf..f1ceddfd2 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -15,7 +15,7 @@ import { } from '@headlessui/react' import cn from 'classnames' import { matchSorter } from 'match-sorter' -import { useEffect, useId, 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' @@ -145,146 +145,161 @@ export const Combobox = ({ } 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. + const isOpenRef = useRef(false) return ( onChange(val || '')} - // we only want to keep the query on close when arbitrary values are allowed - onClose={allowArbitraryValues ? undefined : () => setQuery('')} + onClose={() => { + isOpenRef.current = false + if (!allowArbitraryValues) setQuery('') + }} disabled={disabled || isLoading} immediate {...props} > - {({ open }) => ( -
- {label && ( - // TODO: FieldLabel needs a real ID -
- - {label} - - {description && ( - {description} - )} -
- )} -
{ + // Sync open state to ref on render (handles the opening side) + if (open) isOpenRef.current = true + return ( +
+ {label && ( + // TODO: FieldLabel needs a real ID +
+ + {label} + + {description && ( + {description} + )} +
)} - // 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={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 combobox is open, Enter is handled internally by - // Headless UI (selects the highlighted item). When closed, - // we need to prevent the default behavior to avoid submitting - // the containing form. We also considered always preventing - // default on Enter regardless of open status, but it appears - // to break the combobox handling. Headless UI likely checks - // e.defaultPrevented before processing item selection. - if (e.key === 'Enter' && !open) { - e.preventDefault() - onEnter?.(e) - } - }} - placeholder={placeholder} - disabled={disabled || isLoading} +
- {items.length > 0 && ( - + { + 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) + } + }} + placeholder={placeholder} + disabled={disabled || isLoading} className={cn( - 'border-secondary my-1.5 flex items-center border-l px-3', - disabled ? 'bg-disabled cursor-not-allowed' : 'bg-default' + `text-sans-md text-raise placeholder:text-tertiary h-10 w-full rounded-md border-none! px-3 py-2 outline-hidden!`, + disabled + ? 'text-disabled bg-disabled border-default! cursor-not-allowed' + : 'bg-default', + hasError && 'focus-error' )} - aria-hidden + /> + {items.length > 0 && ( + + + + )} +
+ {(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
+ + )} + )}
- {(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
- - )} - - )} -
- )} + ) + }} ) } diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index 5d1c1f941..d1993d3c6 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -8,7 +8,20 @@ import { expect, test, type Locator, type Page } from '@playwright/test' -import { clickRowAction, expectRowVisible, selectOption, sleep } from './utils' +import { clickRowAction, expectRowVisible, selectOption } from './utils' + +/** + * Fill a combobox and click a dropdown option. Scrolls the combobox toward the + * center of the viewport first so the Floating UI anchored dropdown has room to + * render on-screen. Without this, Safari/WebKit can place the dropdown outside + * the viewport when the combobox is near the bottom of a tall form, causing + * Playwright's click to fail. + */ +async function fillAndSelect(input: Locator, page: Page, text: string, optionName: string) { + await input.evaluate((el) => el.scrollIntoView({ block: 'center' })) + await input.fill(text) + await page.getByRole('option', { name: optionName }).click() +} const defaultRules = ['allow-internal-inbound', 'allow-ssh', 'allow-icmp'] @@ -149,13 +162,7 @@ const deleteRowAndVerifyRowCount = async (table: Locator, expectedCount: number) await expect(rows).toHaveCount(expectedCount) } -test('firewall rule form targets table', async ({ page, browserName }) => { - // eslint-disable-next-line playwright/no-skipped-test - test.skip( - browserName === 'firefox', - 'This test flakes too much in Firefox and we are totally changing this form very soon.' - ) - +test('firewall rule form targets table', async ({ page }) => { await page.goto('/projects/mock-project/vpcs/mock-vpc') await page.getByRole('tab', { name: 'Firewall Rules' }).click() @@ -172,21 +179,12 @@ test('firewall rule form targets table', async ({ page, browserName }) => { // add targets with overlapping names and types to test delete - await targetVpcNameField.fill('abc') - // hit enter one time to choose the custom value - await targetVpcNameField.press('Enter') - - // pressing enter twice here in quick succession causes test flake in firefox - // specifically and this fixes it - await sleep(300) - - // hit enter a second time to submit the subform - await targetVpcNameField.press('Enter') + await fillAndSelect(targetVpcNameField, page, 'abc', 'Custom: abc') + await addButton.click() await expectRowVisible(targets, { Type: 'vpc', Value: 'abc' }) // enter a VPC called 'mock-subnet', even if that doesn't make sense here, to test dropdown later - await targetVpcNameField.fill('mock-subnet') - await targetVpcNameField.press('Enter') + await fillAndSelect(targetVpcNameField, page, 'mock-subnet', 'mock-subnet') await addButton.click() await expectRowVisible(targets, { Type: 'vpc', Value: 'mock-subnet' }) @@ -206,15 +204,8 @@ test('firewall rule form targets table', async ({ page, browserName }) => { // now add a subnet by entering text await selectOption(page, 'Target type', 'VPC subnet') // test that the name typed in is normalized - await subnetNameField.fill('abc-123') - // hit enter to submit the subform - await subnetNameField.press('Enter') - - // pressing enter twice here in quick succession causes test flake in firefox - // specifically and this fixes it - await sleep(300) - - await subnetNameField.press('Enter') + await fillAndSelect(subnetNameField, page, 'abc-123', 'Custom: abc-123') + await addButton.click() await expectRowVisible(targets, { Type: 'subnet', Value: 'abc-123' }) // add IP target @@ -246,8 +237,7 @@ test('firewall rule form target validation', async ({ page }) => { // Enter invalid VPC name const vpcNameField = page.getByRole('combobox', { name: 'VPC name' }).first() - await vpcNameField.fill('ab-') - await vpcNameField.press('Enter') + await fillAndSelect(vpcNameField, page, 'ab-', 'Custom: ab-') await addButton.click() await expect(nameError).toBeVisible() @@ -311,8 +301,7 @@ test('firewall rule form host validation', async ({ page }) => { // Enter invalid VPC name const vpcNameField = page.getByRole('combobox', { name: 'VPC name' }).nth(1) - await vpcNameField.fill('ab-') - await vpcNameField.press('Enter') + await fillAndSelect(vpcNameField, page, 'ab-', 'Custom: ab-') await addButton.click() await expect(nameError).toBeVisible() @@ -379,13 +368,11 @@ test('firewall rule form hosts table', async ({ page }) => { // add hosts with overlapping names and types to test delete - await hostFiltersVpcNameField.fill('abc') - await hostFiltersVpcNameField.press('Enter') + await fillAndSelect(hostFiltersVpcNameField, page, 'abc', 'Custom: abc') await addButton.click() await expectRowVisible(hosts, { Type: 'vpc', Value: 'abc' }) - await hostFiltersVpcNameField.fill('def') - await hostFiltersVpcNameField.press('Enter') + await fillAndSelect(hostFiltersVpcNameField, page, 'def', 'Custom: def') await addButton.click() await expectRowVisible(hosts, { Type: 'vpc', Value: 'def' }) @@ -395,8 +382,8 @@ test('firewall rule form hosts table', async ({ page }) => { await expectRowVisible(hosts, { Type: 'subnet', Value: 'mock-subnet' }) await selectOption(page, 'Host type', 'VPC subnet') - await page.getByRole('combobox', { name: 'Subnet name' }).fill('abc') - await page.getByRole('combobox', { name: 'Subnet name' }).press('Enter') + const subnetNameField2 = page.getByRole('combobox', { name: 'Subnet name' }) + await fillAndSelect(subnetNameField2, page, 'abc', 'Custom: abc') await addButton.click() await expectRowVisible(hosts, { Type: 'subnet', Value: 'abc' }) @@ -454,8 +441,8 @@ test('can update firewall rule', async ({ page }) => { // add a new ICMP protocol filter with type 3 and code 0 await selectOption(page, 'Protocol filters', 'ICMP') - await page.getByRole('combobox', { name: 'ICMP Type' }).fill('3') - await page.getByRole('combobox', { name: 'ICMP Type' }).press('Enter') + const icmpTypeField = page.getByRole('combobox', { name: 'ICMP Type' }) + await fillAndSelect(icmpTypeField, page, '3', '3 - Destination Unreachable') await page.getByRole('textbox', { name: 'ICMP Code' }).fill('0') await page.getByRole('button', { name: 'Add protocol' }).click() @@ -464,9 +451,13 @@ test('can update firewall rule', async ({ page }) => { // add host filter await selectOption(page, 'Host type', 'VPC subnet') - await page.getByRole('combobox', { name: 'Subnet name' }).fill('edit-filter-subnet') - await page.getByRole('combobox', { name: 'Subnet name' }).press('Enter') - await page.getByRole('combobox', { name: 'Subnet name' }).blur() + const editSubnetField = page.getByRole('combobox', { name: 'Subnet name' }) + await fillAndSelect( + editSubnetField, + page, + 'edit-filter-subnet', + 'Custom: edit-filter-subnet' + ) await page.getByRole('button', { name: 'Add host filter' }).click() // new host is added to hosts table @@ -670,6 +661,32 @@ test('arbitrary values combobox', async ({ page }) => { await expect(error).toBeVisible() }) +// Regression test: when typing a partial match in an arbitrary-values combobox, +// arrowing to a dropdown option, and pressing Enter, the selected option's value +// should end up in the field — not the raw typed query. +test('combobox Enter selects highlighted item, not raw query', async ({ page }) => { + await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules-new') + + const targets = page.getByRole('table', { name: 'Targets' }) + const vpcInput = page.getByRole('combobox', { name: 'VPC name' }).first() + + // Type "mock" — dropdown shows mock-vpc and Custom: mock + await vpcInput.fill('mock') + await expect(page.getByRole('option', { name: 'mock-vpc' })).toBeVisible() + await expect(page.getByRole('option', { name: 'Custom: mock' })).toBeVisible() + + // Arrow up to highlight mock-vpc and press Enter to select it + await page.keyboard.press('ArrowUp') + await page.keyboard.press('Enter') + + // The selected value should be mock-vpc, not the raw query "mock" + await expect(vpcInput).toHaveValue('mock-vpc') + + // Verify it can be submitted to the targets table + await page.getByRole('button', { name: 'Add target' }).click() + await expectRowVisible(targets, { Type: 'vpc', Value: 'mock-vpc' }) +}) + test("esc in combobox doesn't close form", async ({ page }) => { await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules-new')