diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index da348ee214..2d31a3504e 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -6,13 +6,8 @@ * Copyright Oxide Computer Company */ -import { useEffect } from 'react' -import { - useController, - useForm, - type Control, - type ControllerRenderProps, -} from 'react-hook-form' +import { useEffect, type ReactNode } from 'react' +import { useController, useForm, type Control } from 'react-hook-form' import { usePrefetchedApiQuery, @@ -34,7 +29,7 @@ import { RadioField } from '~/components/form/fields/RadioField' import { TextField, TextFieldInner } from '~/components/form/fields/TextField' import { useVpcSelector } from '~/hooks/use-params' import { Badge } from '~/ui/lib/Badge' -import { toComboboxItems, type ComboboxItem } from '~/ui/lib/Combobox' +import { toComboboxItems } from '~/ui/lib/Combobox' import { FormDivider } from '~/ui/lib/Divider' import { FieldLabel } from '~/ui/lib/FieldLabel' import { Message } from '~/ui/lib/Message' @@ -55,7 +50,7 @@ import { type FirewallRuleValues } from './firewall-rules-util' * a few sub-sections (Ports, Protocols, and Hosts). * * The Targets section and the Filters:Hosts section are very similar, so we've - * pulled common code to the DynamicTypeAndValueFields components. + * pulled common code to the TargetAndHostFilterSubform components. * We also then set up the Targets / Ports / Hosts variables at the top of the * CommonFields component. */ @@ -89,31 +84,81 @@ const getFilterValueProps = (targetOrHostType: TargetAndHostFilterType) => { } } -const DynamicTypeAndValueFields = ({ +const TargetAndHostFilterSubform = ({ sectionType, control, - valueType, - items, - disabled, - onInputChange, - onTypeChange, - onSubmitTextField, + messageContent, }: { sectionType: 'target' | 'host' - control: Control - valueType: TargetAndHostFilterType - items: Array - disabled?: boolean - onInputChange?: (value: string) => void - onTypeChange: () => void - onSubmitTextField: (e: React.KeyboardEvent) => void + control: Control + messageContent: ReactNode }) => { + const { project, vpc } = useVpcSelector() + // prefetchedApiQueries below are prefetched in firewall-rules-create and -edit + const { + data: { items: instances }, + } = usePrefetchedApiQuery('instanceList', { query: { project, limit: ALL_ISH } }) + const { + data: { items: vpcs }, + } = usePrefetchedApiQuery('vpcList', { query: { project, limit: ALL_ISH } }) + const { + data: { items: vpcSubnets }, + } = usePrefetchedApiQuery('vpcSubnetList', { query: { project, vpc, limit: ALL_ISH } }) + + const subform = useForm({ defaultValues: targetAndHostDefaultValues }) + const field = useController({ name: `${sectionType}s`, control }).field + + const submitSubform = subform.handleSubmit(({ type, value }) => { + if (!type || !value) return + if (field.value.some((f) => f.type === type && f.value === value)) return + field.onChange([...field.value, { type, value }]) + subform.reset(targetAndHostDefaultValues) + }) + + // HACK: we need to reset the target form completely after a successful submit, + // including especially `isSubmitted`, because that governs whether we're in + // the validate regime (which doesn't validate until submit) or the reValidate + // regime (which validate on every keypress). The reset inside `handleSubmit` + // doesn't do that for us because `handleSubmit` set `isSubmitted: true` after + // running the callback. So we have to watch for a successful submit and call + // the reset again here. + // https://github.com/react-hook-form/react-hook-form/blob/9a497a70a/src/logic/createFormControl.ts#L1194-L1203 + const { isSubmitSuccessful: subformSubmitSuccessful } = subform.formState + useEffect(() => { + if (subformSubmitSuccessful) subform.reset(targetAndHostDefaultValues) + }, [subformSubmitSuccessful, subform]) + + const [valueType, value] = subform.watch(['type', 'value']) + const sectionItems = { + vpc: availableItems(field.value, 'vpc', vpcs), + subnet: availableItems(field.value, 'subnet', vpcSubnets), + instance: availableItems(field.value, 'instance', instances), + ip: [], + ip_net: [], + } + const items = toComboboxItems(sectionItems[valueType]) + const subformControl = subform.control + // HACK: reset the whole subform, keeping type (because we just set + // it). most importantly, this resets isSubmitted so the form can go + // back to validating on submit instead of change. Also resets readyToSubmit. + const onTypeChange = () => { + subform.reset({ type: subform.getValues('type'), value: '' }) + } + const onInputChange = (value: string) => { + subform.setValue('value', value) + } + return ( <> + + {sectionType === 'target' ? 'Targets' : 'Host filters'} + + + { if (e.key === KEYS.enter) { e.preventDefault() // prevent full form submission - onSubmitTextField(e) + submitSubform(e) } }} validate={(value) => @@ -162,51 +209,52 @@ const DynamicTypeAndValueFields = ({ } /> )} + subform.reset()} + onSubmit={submitSubform} + /> + {field.value.length > 0 && ( + + + Type + Value + {/* For remove button */} + + + + {field.value.map(({ type, value }, index) => ( + + + {type} + + {value} + + field.onChange( + field.value.filter((i) => !(i.value === value && i.type === type)) + ) + } + label={`remove ${sectionType} ${value}`} + /> + + ))} + + + )} ) } -type TypeAndValueTableProps = { - sectionType: 'target' | 'host' - items: ControllerRenderProps -} -const TypeAndValueTable = ({ sectionType, items }: TypeAndValueTableProps) => ( - - - Type - Value - {/* For remove button */} - - - - {items.value.map(({ type, value }, index) => ( - - - {type} - - {value} - - items.onChange( - items.value.filter((i) => !(i.value === value && i.type === type)) - ) - } - label={`remove ${sectionType} ${value}`} - /> - - ))} - - -) - /** Given an array of *committed* items (VPCs, Subnets, Instances) and a list of *all* items, * return the items that are available */ const availableItems = ( @@ -249,53 +297,6 @@ const targetAndHostDefaultValues: TargetAndHostFormValues = { } export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) => { - const { project, vpc } = useVpcSelector() - - // prefetchedApiQueries below are prefetched in firewall-rules-create and -edit - const { - data: { items: instances }, - } = usePrefetchedApiQuery('instanceList', { query: { project, limit: ALL_ISH } }) - const { - data: { items: vpcs }, - } = usePrefetchedApiQuery('vpcList', { query: { project, limit: ALL_ISH } }) - const { - data: { items: vpcSubnets }, - } = usePrefetchedApiQuery('vpcSubnetList', { query: { project, vpc } }) - - // Targets - const targetForm = useForm({ defaultValues: targetAndHostDefaultValues }) - const targets = useController({ name: 'targets', control }).field - const [targetType, targetValue] = targetForm.watch(['type', 'value']) - // get the list of items that are not already in the list of targets - const targetItems = { - vpc: availableItems(targets.value, 'vpc', vpcs), - subnet: availableItems(targets.value, 'subnet', vpcSubnets), - instance: availableItems(targets.value, 'instance', instances), - ip: [], - ip_net: [], - } - const submitTarget = targetForm.handleSubmit(({ type, value }) => { - // TODO: do this with a normal validation - // ignore click if empty or a duplicate - // TODO: show error instead of ignoring click - if (!type || !value) return - if (targets.value.some((t) => t.value === value && t.type === type)) return - targets.onChange([...targets.value, { type, value }]) - targetForm.reset(targetAndHostDefaultValues) - }) - // HACK: we need to reset the target form completely after a successful submit, - // including especially `isSubmitted`, because that governs whether we're in - // the validate regime (which doesn't validate until submit) or the reValidate - // regime (which validate on every keypress). The reset inside `handleSubmit` - // doesn't do that for us because `handleSubmit` set `isSubmitted: true` after - // running the callback. So we have to watch for a successful submit and call - // the reset again here. - // https://github.com/react-hook-form/react-hook-form/blob/9a497a70a/src/logic/createFormControl.ts#L1194-L1203 - const { isSubmitSuccessful: targetSubmitSuccessful } = targetForm.formState - useEffect(() => { - if (targetSubmitSuccessful) targetForm.reset(targetAndHostDefaultValues) - }, [targetSubmitSuccessful, targetForm]) - // Ports const portRangeForm = useForm({ defaultValues: { portRange: '' } }) const ports = useController({ name: 'ports', control }).field @@ -308,32 +309,6 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = portRangeForm.reset() }) - // Host Filters - const hostForm = useForm({ defaultValues: targetAndHostDefaultValues }) - const hosts = useController({ name: 'hosts', control }).field - const [hostType, hostValue] = hostForm.watch(['type', 'value']) - // get the list of items that are not already in the list of host filters - const hostFilterItems = { - vpc: availableItems(hosts.value, 'vpc', vpcs), - subnet: availableItems(hosts.value, 'subnet', vpcSubnets), - instance: availableItems(hosts.value, 'instance', instances), - ip: [], - ip_net: [], - } - const submitHost = hostForm.handleSubmit(({ type, value }) => { - // ignore click if empty or a duplicate - // TODO: show error instead of ignoring click - if (!type || !value) return - if (hosts.value.some((t) => t.value === value && t.type === type)) return - hosts.onChange([...hosts.value, { type, value }]) - hostForm.reset(targetAndHostDefaultValues) - }) - // HACK: see comment above about doing the same for target form - const { isSubmitSuccessful: hostSubmitSuccessful } = hostForm.formState - useEffect(() => { - if (hostSubmitSuccessful) hostForm.reset(targetAndHostDefaultValues) - }, [hostSubmitSuccessful, hostForm]) - return ( <> - {/* Really this should be its own
, but you can't have a form inside a form, - so we just stick the submit handler in a button onClick */} - Targets - -

Targets determine the instances to which this rule applies. You can target @@ -436,28 +408,6 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = } /> - - targetForm.reset({ type: targetForm.getValues('type'), value: '' }) - } - onInputChange={(value) => targetForm.setValue('value', value)} - onSubmitTextField={submitTarget} - /> - targetForm.reset()} - onSubmit={submitTarget} - /> - {!!targets.value.length && } - Filters @@ -507,7 +457,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = onSubmit={submitPortRange} /> - {!!ports.value.length && ( + {ports.value.length > 0 && ( Port ranges @@ -540,11 +490,10 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = - Host filters - - Host filters match the “other end” of traffic from the target’s perspective: for an inbound rule, they match the source of @@ -552,25 +501,6 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = } /> - hostForm.reset({ type: hostForm.getValues('type'), value: '' })} - onInputChange={(value) => hostForm.setValue('value', value)} - onSubmitTextField={submitHost} - /> - hostForm.reset()} - onSubmit={submitHost} - /> - {!!hosts.value.length && } {error && ( <> diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index 35aee97230..01f14317b2 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -61,7 +61,9 @@ CreateFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { apiQueryClient.prefetchQuery('vpcFirewallRulesView', { query: { project, vpc } }), apiQueryClient.prefetchQuery('instanceList', { query: { project, limit: ALL_ISH } }), apiQueryClient.prefetchQuery('vpcList', { query: { project, limit: ALL_ISH } }), - apiQueryClient.prefetchQuery('vpcSubnetList', { query: { project, vpc } }), + apiQueryClient.prefetchQuery('vpcSubnetList', { + query: { project, vpc, limit: ALL_ISH }, + }), ]) return null diff --git a/app/forms/firewall-rules-edit.tsx b/app/forms/firewall-rules-edit.tsx index bbea4f975e..b9bd16fa9d 100644 --- a/app/forms/firewall-rules-edit.tsx +++ b/app/forms/firewall-rules-edit.tsx @@ -39,7 +39,9 @@ EditFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { apiQueryClient.fetchQuery('vpcFirewallRulesView', { query: { project, vpc } }), apiQueryClient.prefetchQuery('instanceList', { query: { project, limit: ALL_ISH } }), apiQueryClient.prefetchQuery('vpcList', { query: { project, limit: ALL_ISH } }), - apiQueryClient.prefetchQuery('vpcSubnetList', { query: { project, vpc } }), + apiQueryClient.prefetchQuery('vpcSubnetList', { + query: { project, vpc, limit: ALL_ISH }, + }), ]) const originalRule = firewallRules.rules.find((r) => r.name === rule) diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index f90c903fb4..05523b7054 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -62,12 +62,15 @@ export type ComboboxBaseProps = { * For example, if a form value should be set when the user types, use `onInputChange`. */ onInputChange?: (value: string) => void + /** Fires whenever the Enter key is pressed while Combobox input has focus */ + onEnter?: (event: React.KeyboardEvent) => void } type ComboboxProps = { selectedItemValue: string selectedItemLabel: string hasError?: boolean + /** Fires when the user *selects* an item from the list */ onChange: (value: string) => void /** Necessary if you want RHF to be able to focus it on error */ inputRef?: Ref @@ -85,6 +88,7 @@ export const Combobox = ({ disabled, isLoading, onChange, + onEnter, onInputChange, allowArbitraryValues = false, hideOptionalTag, @@ -137,121 +141,132 @@ export const Combobox = ({ const zIndex = usePopoverZIndex() const id = useId() return ( - <> - onChange(val || '')} - // we only want to keep the query on close when arbitrary values are allowed - onClose={allowArbitraryValues ? undefined : () => setQuery('')} - disabled={disabled || isLoading} - immediate - {...props} - > - {label && ( - // TODO: FieldLabel needs a real ID -

- - {label} - - {description && ( - {description} - )} -
- )} -
onChange(val || '')} + // we only want to keep the query on close when arbitrary values are allowed + onClose={allowArbitraryValues ? undefined : () => setQuery('')} + disabled={disabled || isLoading} + immediate + {...props} + > + {({ open }) => ( + <> + {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} - > - - allowArbitraryValues ? selectedItemValue : selectedItemLabel - } - onChange={(event) => { - // updates the query state as the user types, in order to filter the list of items - setQuery(event.target.value) - // if the parent component wants to know about input changes, call the callback - onInputChange?.(event.target.value) - }} - placeholder={placeholder} - disabled={disabled || isLoading} +
- {items.length > 0 && ( - - - - )} -
- {(items.length > 0 || allowArbitraryValues) && ( - - {filteredItems.map((item) => ( - + allowArbitraryValues ? selectedItemValue : selectedItemLabel + } + onChange={(event) => { + // updates the query state as the user types, in order to filter the list of items + setQuery(event.target.value) + // if the parent component wants to know about input changes, call the callback + onInputChange?.(event.target.value) + }} + onKeyDown={(e) => { + // Prevent form submission when the user presses Enter inside a combobox. + // The combobox component already handles Enter keypresses to select items, + // so we only preventDefault when the combobox is closed. + if (e.key === 'Enter' && !open) { + e.preventDefault() + onEnter?.(e) + } + }} + placeholder={placeholder} + disabled={disabled || isLoading} + className={cn( + `h-10 w-full rounded !border-none px-3 py-2 !outline-none text-sans-md text-default placeholder:text-quaternary`, + disabled + ? 'cursor-not-allowed text-disabled bg-disabled !border-default' + : 'bg-default', + hasError && 'focus-error' + )} + /> + {items.length > 0 && ( + - {({ 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 1cd693b813..7fd100fd34 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -163,8 +163,10 @@ test('firewall rule form targets table', async ({ page }) => { // 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') + // hit enter a second time to submit the subform 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 @@ -186,8 +188,9 @@ 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() + // hit enter to submit the subform + await subnetNameField.press('Enter') + await subnetNameField.press('Enter') await expectRowVisible(targets, { Type: 'subnet', Value: 'abc' }) // add IP target