diff --git a/.eslintrc.cjs b/.eslintrc.cjs
index 511c68aa9d..f630805bc7 100644
--- a/.eslintrc.cjs
+++ b/.eslintrc.cjs
@@ -116,7 +116,7 @@ module.exports = {
rules: {
'playwright/expect-expect': [
'warn',
- { assertFunctionNames: ['expectVisible', 'expectRowVisible'] },
+ { assertFunctionNames: ['expectVisible', 'expectRowVisible', 'expectOptions'] },
],
},
},
diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx
index b0935c893b..c9a6d3e9c7 100644
--- a/app/forms/firewall-rules-common.tsx
+++ b/app/forms/firewall-rules-common.tsx
@@ -468,7 +468,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =
/>
- {/* We have to blow this up instead of using TextField to get better
+ {/* We have to blow this up instead of using TextField to get better
text styling on the label */}
diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx
index bc7de2ff8a..f9cbc7cf45 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 { useId, useState, type ReactNode, type Ref } from 'react'
+import { useEffect, useId, useState, type ReactNode, type Ref } from 'react'
import { SelectArrows6Icon } from '@oxide/design-system/icons/react'
@@ -97,6 +97,43 @@ export const Combobox = ({
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.
+ 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 &&
+ !filteredItems.some((i) => i.selectedLabel === query)
+ ) {
+ filteredItems.push({
+ value: query,
+ label: (
+ <>
+ Custom: {query}
+ >
+ ),
+ selectedLabel: query,
+ })
+ }
const zIndex = usePopoverZIndex()
const id = useId()
return (
@@ -106,7 +143,8 @@ export const Combobox = ({
value={selectedItemValue}
// fallback to '' allows clearing field to work
onChange={(val) => onChange(val || '')}
- onClose={() => setQuery('')}
+ // we only want to keep the query on close when arbitrary values are allowed
+ onClose={allowArbitraryValues ? undefined : () => setQuery('')}
disabled={disabled || isLoading}
immediate
{...props}
@@ -177,18 +215,13 @@ export const Combobox = ({
)}
- {items.length > 0 && (
+ {(items.length > 0 || allowArbitraryValues) && (
- {!allowArbitraryValues && filteredItems.length === 0 && (
-
- No items match
-
- )}
{filteredItems.map((item) => (
@@ -211,6 +244,11 @@ export const Combobox = ({
)}
))}
+ {!allowArbitraryValues && filteredItems.length === 0 && (
+
+ No items match
+
+ )}
)}
diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts
index 44f3ba7731..1cd693b813 100644
--- a/test/e2e/firewall-rules.e2e.ts
+++ b/test/e2e/firewall-rules.e2e.ts
@@ -6,14 +6,9 @@
* Copyright Oxide Computer Company
*/
-import {
- clickRowAction,
- expect,
- expectRowVisible,
- selectOption,
- test,
- type Locator,
-} from './utils'
+import { expect, test, type Locator, type Page } from '@playwright/test'
+
+import { clickRowAction, expectRowVisible, selectOption } from './utils'
const defaultRules = ['allow-internal-inbound', 'allow-ssh', 'allow-icmp']
@@ -53,6 +48,7 @@ test('can create firewall rule', async ({ page }) => {
// add host filter instance "host-filter-instance"
await selectOption(page, 'Host type', 'Instance')
await page.getByRole('combobox', { name: 'Instance name' }).fill('host-filter-instance')
+ await page.getByText('host-filter-instance').click()
await page.getByRole('button', { name: 'Add host filter' }).click()
// host is added to hosts table
@@ -167,11 +163,13 @@ test('firewall rule form targets table', async ({ page }) => {
// add targets with overlapping names and types to test delete
await targetVpcNameField.fill('abc')
+ await targetVpcNameField.press('Enter')
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 addButton.click()
await expectRowVisible(targets, { Type: 'vpc', Value: 'mock-subnet' })
@@ -188,6 +186,7 @@ test('firewall rule form targets table', async ({ page }) => {
// now add a subnet by entering text
await selectOption(page, 'Target type', 'VPC subnet')
await subnetNameField.fill('abc')
+ await page.getByText('abc').first().click()
await addButton.click()
await expectRowVisible(targets, { Type: 'subnet', Value: 'abc' })
@@ -221,6 +220,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 addButton.click()
await expect(nameError).toBeVisible()
@@ -246,6 +246,7 @@ test('firewall rule form target validation', async ({ page }) => {
await expect(ipError).toBeHidden()
await expect(nameError).toBeHidden()
await vpcNameField.fill('abc')
+ await page.getByText('abc').click()
await addButton.click()
await expectRowVisible(targets, { Type: 'vpc', Value: 'abc' })
@@ -284,6 +285,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 addButton.click()
await expect(nameError).toBeVisible()
@@ -309,6 +311,7 @@ test('firewall rule form host validation', async ({ page }) => {
await expect(ipError).toBeHidden()
await expect(nameError).toBeHidden()
await vpcNameField.fill('abc')
+ await page.getByText('abc').click()
await addButton.click()
await expectRowVisible(hosts, { Type: 'vpc', Value: 'abc' })
@@ -350,10 +353,12 @@ 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 addButton.click()
await expectRowVisible(hosts, { Type: 'vpc', Value: 'abc' })
await hostFiltersVpcNameField.fill('def')
+ await hostFiltersVpcNameField.press('Enter')
await addButton.click()
await expectRowVisible(hosts, { Type: 'vpc', Value: 'def' })
@@ -364,6 +369,7 @@ test('firewall rule form hosts table', async ({ page }) => {
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')
await addButton.click()
await expectRowVisible(hosts, { Type: 'subnet', Value: 'abc' })
@@ -427,6 +433,7 @@ 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.getByText('edit-filter-subnet').click()
await page.getByRole('button', { name: 'Add host filter' }).click()
// new host is added to hosts table
@@ -561,3 +568,49 @@ test('name conflict error on edit', async ({ page }) => {
await page.getByRole('button', { name: 'Update rule' }).click()
await expectRowVisible(page.getByRole('table'), { Name: 'allow-icmp2', Priority: '37' })
})
+
+async function expectOptions(page: Page, options: string[]) {
+ const selector = page.getByRole('option')
+ await expect(selector).toHaveCount(options.length)
+ for (const option of options) {
+ await expect(page.getByRole('option', { name: option })).toBeVisible()
+ }
+}
+
+test('arbitrary values combobox', async ({ page }) => {
+ await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules-new')
+
+ // test for bug where we'd persist the d after add and only show 'Custom: d'
+ const vpcInput = page.getByRole('combobox', { name: 'VPC name' }).first()
+ await vpcInput.focus()
+ await expectOptions(page, ['mock-vpc'])
+
+ await vpcInput.fill('d')
+ await expectOptions(page, ['Custom: d'])
+
+ await vpcInput.blur()
+ page.getByRole('button', { name: 'Add target' }).click()
+ await expect(vpcInput).toHaveValue('')
+
+ await vpcInput.focus()
+ await expectOptions(page, ['mock-vpc']) // bug cause failure here
+
+ // test keeping query around on blur
+ await selectOption(page, 'Target type', 'Instance')
+ const input = page.getByRole('combobox', { name: 'Instance name' })
+
+ await input.focus()
+ await expectOptions(page, ['db1', 'you-fail', 'not-there-yet'])
+
+ await input.fill('d')
+ await expectOptions(page, ['db1', 'Custom: d'])
+
+ await input.blur()
+ await expect(page.getByRole('option')).toBeHidden()
+
+ await expect(input).toHaveValue('d')
+ await input.focus()
+
+ // same options show up after blur (there was a bug around this)
+ await expectOptions(page, ['db1', 'Custom: d'])
+})