diff --git a/app/components/form/fields/ComboboxField.tsx b/app/components/form/fields/ComboboxField.tsx index efa0558293..a3d17299ff 100644 --- a/app/components/form/fields/ComboboxField.tsx +++ b/app/components/form/fields/ComboboxField.tsx @@ -62,8 +62,9 @@ export function ComboboxField< items, transform, validate, + popoverError, ...props -}: ComboboxFieldProps) { +}: ComboboxFieldProps & { popoverError?: boolean }) { const { field, fieldState } = useController({ name, control, @@ -92,8 +93,9 @@ export function ComboboxField< inputRef={field.ref} transform={transform} {...props} + popoverError={popoverError ? fieldState.error : undefined} /> - + {!popoverError && } ) } diff --git a/app/components/form/fields/DescriptionField.tsx b/app/components/form/fields/DescriptionField.tsx index 51b2b4ed36..9cb0ad332e 100644 --- a/app/components/form/fields/DescriptionField.tsx +++ b/app/components/form/fields/DescriptionField.tsx @@ -7,6 +7,8 @@ */ import type { FieldPath, FieldValues } from 'react-hook-form' +import { capitalize } from '~/util/str' + import { TextField, type TextFieldProps } from './TextField' // TODO: Pull this from generated types @@ -16,7 +18,14 @@ export function DescriptionField< TFieldValues extends FieldValues, TName extends FieldPath, >(props: Omit, 'validate'>) { - return + return ( + + ) } // TODO Update JSON schema to match this, add fuzz testing between this and name pattern diff --git a/app/components/form/fields/ErrorMessage.tsx b/app/components/form/fields/ErrorMessage.tsx index f804f46189..bffe02b84d 100644 --- a/app/components/form/fields/ErrorMessage.tsx +++ b/app/components/form/fields/ErrorMessage.tsx @@ -5,13 +5,18 @@ * * Copyright Oxide Computer Company */ +import cn from 'classnames' import type { FieldError } from 'react-hook-form' +import { Info12Icon } from '@oxide/design-system/icons/react' + import { TextInputError } from '~/ui/lib/TextInput' +import { Tooltip } from '~/ui/lib/Tooltip' type ErrorMessageProps = { error: FieldError | undefined label: string + isSmall?: boolean } export function ErrorMessage({ error, label }: ErrorMessageProps) { @@ -22,3 +27,30 @@ export function ErrorMessage({ error, label }: ErrorMessageProps) { return {message} } + +export function PopoverErrorMessage({ + error, + label, + className, +}: ErrorMessageProps & { className?: string }) { + if (!error) return null + + const message = error.type === 'required' ? `${label} is required` : error.message + if (!message) return null + + return ( + + + + ) +} diff --git a/app/components/form/fields/ListboxField.tsx b/app/components/form/fields/ListboxField.tsx index 445c8e810c..0bd13648e8 100644 --- a/app/components/form/fields/ListboxField.tsx +++ b/app/components/form/fields/ListboxField.tsx @@ -44,7 +44,7 @@ export function ListboxField< items, name, placeholder, - label = capitalize(name), + label, disabled, required, description, @@ -81,7 +81,7 @@ export function ListboxField< buttonRef={field.ref} hideOptionalTag={hideOptionalTag} /> - + ) } diff --git a/app/components/form/fields/TextField.tsx b/app/components/form/fields/TextField.tsx index 5348b7c273..f0fa1f81b3 100644 --- a/app/components/form/fields/TextField.tsx +++ b/app/components/form/fields/TextField.tsx @@ -55,26 +55,29 @@ export function TextField< TName extends FieldPath, >({ name, - label = capitalize(name), + label, units, description, required, ...props -}: Omit, 'id'> & UITextAreaProps) { +}: Omit, 'id'> & + UITextAreaProps & { popoverError?: boolean }) { // id is omitted from props because we generate it here const id = useId() return (
-
- - {label} {units && ({units})} - - {description && ( - - {description} - - )} -
+ {label && ( +
+ + {label} {units && ({units})} + + {description && ( + + {description} + + )} +
+ )} {/* passing the generated id is very important for a11y */}
@@ -102,8 +105,9 @@ export const TextFieldInner = < required, id: idProp, transform, + popoverError, ...props -}: TextFieldProps & UITextAreaProps) => { +}: TextFieldProps & UITextAreaProps & { popoverError?: boolean }) => { const generatedId = useId() const id = idProp || generatedId const { @@ -119,10 +123,11 @@ export const TextFieldInner = < error={!!error} aria-labelledby={`${id}-label ${id}-help-text`} onChange={(e) => onChange(transform ? transform(e.target.value) : e.target.value)} + popoverError={popoverError ? error : undefined} {...fieldRest} {...props} /> - + {!popoverError && } ) } diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 65a5e397f7..4adfc6bb94 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -6,15 +6,20 @@ * Copyright Oxide Computer Company */ -import { useEffect, type ReactNode } from 'react' -import { useController, useForm, type Control } from 'react-hook-form' +import { useEffect } from 'react' +import { + useFormState, + useWatch, + type Control, + type UseFormReturn, + type UseFormTrigger, +} from 'react-hook-form' import { usePrefetchedApiQuery, type ApiError, type Instance, type Vpc, - type VpcFirewallIcmpFilter, type VpcFirewallRuleAction, type VpcFirewallRuleDirection, type VpcFirewallRuleHostFilter, @@ -30,78 +35,91 @@ import { ListboxField } from '~/components/form/fields/ListboxField' import { NameField, validateName } from '~/components/form/fields/NameField' import { NumberField } from '~/components/form/fields/NumberField' import { RadioField } from '~/components/form/fields/RadioField' -import { TextField, TextFieldInner } from '~/components/form/fields/TextField' +import { TextField } from '~/components/form/fields/TextField' import { useVpcSelector } from '~/hooks/use-params' -import { - ProtocolCell, - ProtocolCodeCell, - ProtocolTypeCell, -} from '~/table/cells/ProtocolCell' -import { Badge } from '~/ui/lib/Badge' import { toComboboxItems } from '~/ui/lib/Combobox' import { FormDivider } from '~/ui/lib/Divider' import { FieldLabel } from '~/ui/lib/FieldLabel' +import { InputMiniTable } from '~/ui/lib/InputMiniTable' import { Message } from '~/ui/lib/Message' -import { ClearAndAddButtons, MiniTable } from '~/ui/lib/MiniTable' import { SideModal } from '~/ui/lib/SideModal' -import { TextInputHint } from '~/ui/lib/TextInput' -import { KEYS } from '~/ui/util/keys' +import { TextInput, TextInputHint } from '~/ui/lib/TextInput' import { ALL_ISH } from '~/util/consts' import { validateIp, validateIpNet } from '~/util/ip' import { links } from '~/util/links' -import { getProtocolDisplayName, getProtocolKey, ICMP_TYPES } from '~/util/protocol' +import { ICMP_TYPES } from '~/util/protocol' import { capitalize, normalizeDashes } from '~/util/str' import { type FirewallRuleValues } from './firewall-rules-util' -/** - * This is a large file. The main thing to be aware of is that the firewall rules - * form is made up of two main sections: Targets and Filters. Filters, then, has - * 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 TargetAndHostFilterSubform components. - * We also then set up the Targets / Ports / Hosts variables at the top of the - * CommonFields component. - */ - -type TargetAndHostFilterType = - | VpcFirewallRuleTarget['type'] - | VpcFirewallRuleHostFilter['type'] - -type TargetAndHostFormValues = { - type: TargetAndHostFilterType - value: string +const valuePlaceholders = { + vpc: 'example-vpc', + subnet: 'example-subnet', + instance: 'example-inst', + ip: 'IPv4 or IPv6 address', + ip_net: 'IP network', } -// these are part of the target and host filter form; -// the specific values depend on the target or host filter type selected -const getFilterValueProps = (targetOrHostType: TargetAndHostFilterType) => { - switch (targetOrHostType) { - case 'vpc': - return { label: 'VPC name' } - case 'subnet': - return { label: 'Subnet name' } - case 'instance': - return { label: 'Instance name' } - case 'ip': - return { label: 'IP address', description: 'Enter an IPv4 or IPv6 address' } - case 'ip_net': - return { - label: 'IP network', - description: 'Looks like 192.168.0.0/16 or fd00:1122:3344:0001::1/64', +const TargetFilter = ({ + control, + onTypeChange, +}: { + control: Control + onTypeChange: (index: number) => void +}) => ( + <> + Targets + + +

+ Targets determine the instances to which this rule applies. You can target + instances directly or specify a VPC, VPC subnet, IP, or IP subnet, which will + apply the rule to traffic going to all matching instances. +

+

+ Targets are additive: the rule applies to instances matching{' '} + any target. +

+ } - } -} + /> + +
+ [ + onTypeChange(index)} + />, + , + ]} + emptyState={{ title: 'No targets', body: 'Add a target to see it here' }} + defaultValue={{ type: 'vpc', value: '' }} + columnWidths={['1.25fr', '2fr']} + /> +
+ +) -const TargetAndHostFilterSubform = ({ - sectionType, +const TargetsValueField = ({ + index, control, - messageContent, }: { - sectionType: 'target' | 'host' + index: number control: Control - messageContent: ReactNode }) => { const { project, vpc } = useVpcSelector() // prefetchedApiQueries below are prefetched in firewall-rules-create and -edit @@ -115,127 +133,49 @@ const TargetAndHostFilterSubform = ({ 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 type = useWatch({ name: `targets.${index}.type`, control }) + const values = useWatch({ name: `targets`, control }) - 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), + vpc: availableItems(values, 'vpc', vpcs), + subnet: availableItems(values, 'subnet', vpcSubnets), + instance: availableItems(values, '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) - } - - const noun = sectionType === 'target' ? 'target' : 'host filter' - const nounTitle = capitalize(noun) + 's' - - return ( - <> - {nounTitle} - - - - {/* In the firewall rules form, a few types get comboboxes instead of text fields */} - {valueType === 'vpc' || valueType === 'subnet' || valueType === 'instance' ? ( - - // required: false arg is desirable because otherwise if you put in - // a bad name and submit, causing it to revalidate onChange, then - // clear the field you're left with a BS "Target name is required" - // error message - validateName(value, `${capitalize(sectionType)} name`, false) - } - /> - ) : ( - { - if (e.key === KEYS.enter) { - e.preventDefault() // prevent full form submission - submitSubform(e) - } - }} - validate={(value) => - (valueType === 'ip' && validateIp(value)) || - (valueType === 'ip_net' && validateIpNet(value)) || - undefined - } - /> - )} - subform.reset()} - onSubmit={submitSubform} - /> - - `${type}|${value}` - } - onRemoveItem={({ - type, - value, - }: VpcFirewallRuleTarget | VpcFirewallRuleHostFilter) => { - field.onChange(field.value.filter((i) => !(i.value === value && i.type === type))) - }} - /> - + const items = toComboboxItems(sectionItems[type]) + + return type === 'vpc' || type === 'subnet' || type === 'instance' ? ( + + // required: false arg is desirable because otherwise if you put in + // a bad name and submit, causing it to revalidate onChange, then + // clear the field you're left with a BS "Target name is required" + // error message + validateName(value, `${capitalize(type)} name`, false) + } + placeholder={valuePlaceholders[type]} + popoverError + /> + ) : ( + + (type === 'ip' && validateIp(value)) || + (type === 'ip_net' && validateIpNet(value)) || + undefined + } + popoverError + placeholder={valuePlaceholders[type]} + /> ) } @@ -258,21 +198,17 @@ const availableItems = ( } // Protocol selection form values for the subform -type ProtocolFormValues = { - protocolType: VpcFirewallRuleProtocol['type'] | '' - icmpType?: string // ComboboxField with allowArbitraryValues can return strings - icmpCode?: string -} const targetHostTypeItems: Array<{ value: VpcFirewallRuleHostFilter['type'] label: string + selectedLabel: string }> = [ - { value: 'vpc', label: 'VPC' }, - { value: 'subnet', label: 'VPC subnet' }, - { value: 'instance', label: 'Instance' }, - { value: 'ip', label: 'IP' }, - { value: 'ip_net', label: 'IP subnet' }, + { value: 'vpc', label: 'VPC', selectedLabel: 'VPC' }, + { value: 'subnet', label: 'VPC subnet', selectedLabel: 'VPC subnet' }, + { value: 'instance', label: 'Instance', selectedLabel: 'Instance' }, + { value: 'ip', label: 'IP', selectedLabel: 'IP' }, + { value: 'ip_net', label: 'IP subnet', selectedLabel: 'IP subnet' }, ] const actionItems: Array<{ value: VpcFirewallRuleAction; label: string }> = [ @@ -301,59 +237,6 @@ const icmpTypeItems = [ })), ] -const targetAndHostTableColumns = [ - { - header: 'Type', - cell: (item: VpcFirewallRuleTarget | VpcFirewallRuleHostFilter) => ( - {item.type} - ), - }, - { - header: 'Value', - cell: (item: VpcFirewallRuleTarget | VpcFirewallRuleHostFilter) => item.value, - }, -] - -const portTableColumns = [{ header: 'Port ranges', cell: (p: string) => p }] - -const protocolTableColumns = [ - { - header: 'Protocol', - cell: (protocol: VpcFirewallRuleProtocol) => , - }, - { - header: 'Type', - cell: (protocol: VpcFirewallRuleProtocol) => , - }, - { - header: 'Code', - cell: (protocol: VpcFirewallRuleProtocol) => , - }, -] - -const isDuplicateProtocol = ( - newProtocol: VpcFirewallRuleProtocol, - existingProtocols: VpcFirewallRuleProtocol[] -) => { - if (newProtocol.type === 'tcp' || newProtocol.type === 'udp') { - return existingProtocols.some((p) => p.type === newProtocol.type) - } - - if (newProtocol.type === 'icmp') { - if (newProtocol.value === null) { - return existingProtocols.some((p) => p.type === 'icmp' && p.value === null) - } - return existingProtocols.some( - (p) => - p.type === 'icmp' && - p.value?.icmpType === newProtocol.value?.icmpType && - p.value?.code === newProtocol.value?.code - ) - } - - return false -} - type ParseResult = { success: true; data: T } | { success: false; message: string } const parseIcmpType = (value: string | undefined): ParseResult => { @@ -401,147 +284,247 @@ const icmpCodeValidation = (value: string | undefined) => { return 'ICMP code must be a number or numeric range' } -const ProtocolFilters = ({ control }: { control: Control }) => { - const protocols = useController({ name: 'protocols', control }).field - const protocolForm = useForm({ - defaultValues: { protocolType: '' }, - }) +const ProtocolFilters = ({ + control, + onTypeChange, +}: { + control: Control + onTypeChange: (index: number) => void +}) => { + const protocols = useWatch({ name: 'protocols', control }) || [] - const selectedProtocolType = protocolForm.watch('protocolType') - const selectedIcmpType = protocolForm.watch('icmpType') + const renderProtocolRow = ( + _field: Record & { id: string }, + index: number + ) => { + const selectedType = protocols?.[index]?.type - const addProtocolIfUnique = (newProtocol: VpcFirewallRuleProtocol) => { - if (!isDuplicateProtocol(newProtocol, protocols.value)) { - protocols.onChange([...protocols.value, newProtocol]) - } + return [ + onTypeChange(index)} + />, + selectedType === 'icmp' ? ( + { + if (!value) return undefined + const result = parseIcmpType(String(value)) + if (!result.success) return result.message + }} + popoverError + /> + ) : ( + + ), + selectedType === 'icmp' ? ( + { + if (value === null || value === undefined) return undefined + return icmpCodeValidation(value) + }} + transform={normalizeDashes} + popoverError + /> + ) : ( + + ), + ] } - const submitProtocol = protocolForm.handleSubmit((values) => { - if (values.protocolType === 'tcp' || values.protocolType === 'udp') { - addProtocolIfUnique({ type: values.protocolType }) - } else if (values.protocolType === 'icmp') { - // this parse should never fail because we've already validated, but doing - // it this way keeps the just-in-case early return logic consistent - const parseResult = parseIcmpType(values.icmpType) - if (!parseResult.success) return - - const icmpType = parseResult.data - if (icmpType === undefined) { - // All ICMP types - addProtocolIfUnique({ type: 'icmp', value: null }) - } else { - // Specific ICMP type - const icmpValue: VpcFirewallIcmpFilter = { icmpType } - if (values.icmpCode) { - icmpValue.code = values.icmpCode - } - addProtocolIfUnique({ type: 'icmp', value: icmpValue }) - } + return ( +
+ Protocol filters + + Choose protocol types and configure ICMP settings if needed + + +
+ ) +} + +const HostFilters = ({ + control, + onTypeChange, +}: { + control: Control + onTypeChange: (index: number) => void +}) => { + const { project, vpc } = useVpcSelector() + 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 hosts = useWatch({ name: 'hosts', control }) || [] + + const renderHostRow = ( + _field: Record & { id: string }, + index: number + ) => { + const selectedType = hosts?.[index]?.type + + const sectionItems = { + vpc: availableItems(hosts, 'vpc', vpcs), + subnet: availableItems(hosts, 'subnet', vpcSubnets), + instance: availableItems(hosts, 'instance', instances), + ip: [], + ip_net: [], } - protocolForm.reset() - }) + const items = toComboboxItems(sectionItems[selectedType] || []) - const removeProtocol = (protocolToRemove: VpcFirewallRuleProtocol) => { - const newProtocols = protocols.value.filter((protocol) => protocol !== protocolToRemove) - protocols.onChange(newProtocols) + return [ + onTypeChange(index)} + />, + selectedType === 'vpc' || selectedType === 'subnet' || selectedType === 'instance' ? ( + validateName(value, `Host filter name`, false)} + popoverError + /> + ) : ( + + (selectedType === 'ip' && validateIp(value)) || + (selectedType === 'ip_net' && validateIpNet(value)) || + undefined + } + popoverError + /> + ), + ] } return ( <> -
-
- - - {selectedProtocolType === 'icmp' && ( - <> - protocolForm.setValue('icmpType', value)} - items={icmpTypeItems} - validate={(value) => { - const result = parseIcmpType(value) - if (!result.success) return result.message - }} - /> - - {selectedIcmpType !== undefined && selectedIcmpType !== '' && ( - - Enter a code (0) or range (e.g. 1–3). Leave blank for all - traffic of type {selectedIcmpType}. - - } - placeholder="" - validate={icmpCodeValidation} - transform={normalizeDashes} - /> - )} - - )} -
- - protocolForm.reset()} - onSubmit={submitProtocol} - /> -
+ Host filters - {protocols.value.length > 0 && ( - `Remove ${getProtocolDisplayName(protocol)}`} - /> - )} + + Host filters match the “other end” of traffic from the + target’s perspective: for an inbound rule, they match the source of + traffic. For an outbound rule, they match the destination. + + } + /> + + ) } -type CommonFieldsProps = { +const PortFilters = ({ + control, + trigger, +}: { control: Control - nameTaken: (name: string) => boolean - error: ApiError | null + trigger: UseFormTrigger +}) => { + const ports = useWatch({ name: 'ports', control }) + const { errors } = useFormState({ control }) + + // Helps catch errors that span multiple fields + // without this, errors won't be cleared on one duplicate + // field if the other is edited + useEffect(() => { + if (errors['ports']) { + trigger('ports') + } + }, [trigger, ports, errors]) + + return ( +
+
+ Port filters + + A single destination port (1234) or a range (1234–2345) + + [ + { + if (!parsePortRange(value)) return 'Not a valid port range' + // Filter out the current port to check for duplicates + const otherPorts = ports.filter((_, i) => i !== index) + if (otherPorts.includes(value.trim())) return 'Port range already added' + }} + popoverError + />, + ]} + emptyState={{ title: 'No ports', body: 'Add a port to see it here' }} + defaultValue={new String('') as string} // if this simply '' it will not make new rows + /> +
+
+ ) } -const targetAndHostDefaultValues: TargetAndHostFormValues = { - type: 'vpc', - value: '', +type CommonFieldsProps = { + form: UseFormReturn + nameTaken: (name: string) => boolean + error: ApiError | null } -export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) => { - // Ports - const portRangeForm = useForm({ defaultValues: { portRange: '' } }) - const ports = useController({ name: 'ports', control }).field - const portValue = portRangeForm.watch('portRange') - const submitPortRange = portRangeForm.handleSubmit(({ portRange }) => { - const portRangeValue = portRange.trim() - // at this point we've already validated in validate() that it parses and - // that it is not already in the list - ports.onChange([...ports.value, portRangeValue]) - portRangeForm.reset() - }) +export const CommonFields = ({ form, nameTaken, error }: CommonFieldsProps) => { + const { control, trigger, setValue } = form return ( <> @@ -613,22 +596,9 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = - -

- Targets determine the instances to which this rule applies. You can target - instances directly or specify a VPC, VPC subnet, IP, or IP subnet, which will - apply the rule to traffic going to all matching instances. -

-

- Targets are additive: the rule applies to instances matching{' '} - any target. -

- - } + onTypeChange={(index: number) => setValue(`targets.${index}.value`, '')} /> @@ -646,67 +616,18 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = } /> -
- {/* We have to blow this up instead of using TextField to get better - text styling on the label */} -
- - Port filters - - - A single destination port (1234) or a range (1234–2345) - - { - if (e.key === KEYS.enter) { - e.preventDefault() // prevent full form submission - submitPortRange(e) - } - }} - validate={(value) => { - if (!parsePortRange(value)) return 'Not a valid port range' - if (ports.value.includes(value.trim())) return 'Port range already added' - }} - /> -
- portRangeForm.reset()} - onSubmit={submitPortRange} - /> -
- {ports.value.length > 0 && ( - port} - emptyState={{ title: 'No ports', body: 'Add a port to see it here' }} - onRemoveItem={(p) => ports.onChange(ports.value.filter((p1) => p1 !== p))} - removeLabel={(port) => `remove port ${port}`} - /> - )} + - + setValue(`protocols.${index}.value`, null)} + /> - - Host filters match the “other end” of traffic from the - target’s perspective: for an inbound rule, they match the source of - traffic. For an outbound rule, they match the destination. - - } + onTypeChange={(index) => setValue(`hosts.${index}.value`, '')} /> {error && ( diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index 6aeacbee67..b2cfb4883e 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -125,7 +125,7 @@ export default function CreateFirewallRuleForm() { submitLabel="Add rule" > !!existingRules.find((r) => r.name === name)} error={updateRules.error} diff --git a/app/forms/firewall-rules-edit.tsx b/app/forms/firewall-rules-edit.tsx index b0b62a3fe9..12d7a16d8c 100644 --- a/app/forms/firewall-rules-edit.tsx +++ b/app/forms/firewall-rules-edit.tsx @@ -134,7 +134,7 @@ export default function EditFirewallRuleForm() { submitError={updateRules.error} > !!otherRules.find((r) => r.name === name)} error={updateRules.error} diff --git a/app/pages/SiloImagesPage.tsx b/app/pages/SiloImagesPage.tsx index 3936c3593a..6665296b92 100644 --- a/app/pages/SiloImagesPage.tsx +++ b/app/pages/SiloImagesPage.tsx @@ -187,6 +187,7 @@ const PromoteImageModal = ({ onDismiss }: { onDismiss: () => void }) => { - label: string + label?: string placeholder?: string required?: boolean hideOptionalTag?: boolean @@ -68,6 +71,8 @@ export type ComboboxBaseProps = { * Optional function to transform the value entered into the input as the user types. */ transform?: (value: string) => string + // todo: document + matchDropdownWidth?: boolean } type ComboboxProps = { @@ -78,6 +83,7 @@ type ComboboxProps = { onChange: (value: string) => void /** Necessary if you want RHF to be able to focus it on error */ inputRef?: Ref + popoverError?: FieldError } & ComboboxBaseProps export const Combobox = ({ @@ -98,6 +104,8 @@ export const Combobox = ({ hideOptionalTag, inputRef, transform, + matchDropdownWidth = true, + popoverError, ...props }: ComboboxProps) => { const [query, setQuery] = useState(selectedItemValue || '') @@ -136,9 +144,10 @@ export const Combobox = ({ filteredItems.push({ value: query, label: ( - <> - Custom: {query} - +
+ Custom:{' '} + {query} +
), selectedLabel: query, }) @@ -176,7 +185,7 @@ export const Combobox = ({ )}
- {items.length > 0 && ( - - - + {popoverError && ( + )} -
- {(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
- + - )} + aria-hidden + > + + + + + {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} +
+ )} + + ))} + {filteredItems.length === 0 && ( + +
No items match
+ + )} + )} diff --git a/app/ui/lib/DatePicker.tsx b/app/ui/lib/DatePicker.tsx index ae50282ff2..e4ab8ac3a1 100644 --- a/app/ui/lib/DatePicker.tsx +++ b/app/ui/lib/DatePicker.tsx @@ -55,7 +55,7 @@ export function DatePicker(props: DatePickerProps) { type="button" className={cn( state.isOpen && 'z-10 ring-2', - 'relative flex h-11 items-center rounded-l rounded-r border text-sans-md border-default focus-within:ring-2 hover:border-raise focus:z-10', + 'relative flex h-10 items-center rounded-l rounded-r border text-sans-md border-default focus-within:ring-2 hover:border-raise focus:z-10', state.isInvalid ? 'focus-error border-error ring-error-secondary' : 'border-default ring-accent-secondary' diff --git a/app/ui/lib/DateRangePicker.tsx b/app/ui/lib/DateRangePicker.tsx index ff7e2c71c7..0f696e30d0 100644 --- a/app/ui/lib/DateRangePicker.tsx +++ b/app/ui/lib/DateRangePicker.tsx @@ -63,7 +63,7 @@ export function DateRangePicker(props: DateRangePickerProps) { type="button" className={cn( state.isOpen && 'z-10 ring-2', - 'relative flex h-11 items-center rounded-l rounded-r border text-sans-md border-default focus-within:ring-2 hover:border-raise focus:z-10', + 'relative flex h-10 items-center rounded-l rounded-r border text-sans-md border-default focus-within:ring-2 hover:border-raise focus:z-10', state.isInvalid ? 'focus-error border-error ring-error-secondary hover:border-error' : 'border-default ring-accent-secondary' diff --git a/app/ui/lib/InputMiniTable.tsx b/app/ui/lib/InputMiniTable.tsx new file mode 100644 index 0000000000..8db1ccc1b7 --- /dev/null +++ b/app/ui/lib/InputMiniTable.tsx @@ -0,0 +1,178 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * Copyright Oxide Computer Company + */ +import cn from 'classnames' +import { + useFieldArray, + type Control, + type FieldPath, + type FieldValues, + type PathValue, +} from 'react-hook-form' + +import { Error16Icon } from '@oxide/design-system/icons/react' + +import { Button } from './Button' +import { EMBody } from './EmptyMessage' + +type InputMiniTableProps< + TFieldValues extends FieldValues, + TName extends FieldPath, +> = { + headers: string[] + name: TName + control: Control + renderRow: ( + field: Record & { id: string }, + index: number, + name: string + ) => React.ReactNode[] + emptyState?: { title: string; body: string } + className?: string + defaultValue: PathValue[number] + /** + * Custom column widths using CSS Grid values. Defaults to equal width. + * @example ['1fr', '2fr'] for 1/3 and 2/3 ratio + */ + columnWidths?: string[] +} + +export function InputMiniTable< + TFieldValues extends FieldValues, + TName extends FieldPath, +>({ + headers, + name, + control, + renderRow, + emptyState, + className, + defaultValue, + columnWidths, +}: InputMiniTableProps) { + const { fields, append, remove } = useFieldArray({ + control: control as Control, + name, + }) + + const hasItems = fields.length > 0 + const columnCount = headers.length + 1 // +1 for remove button column + + return ( +
+ {/* Grid Container */} +
+ {/* Header Row */} +
+ {headers.map((header, index) => ( +
+
+ {header} +
+ ))} + {/* Header for remove button column */} +
+
+ + {/* Body Rows */} + {hasItems ? ( + fields.map((field, rowIndex) => { + const rowCells = renderRow(field, rowIndex, name) + return ( +
+ {rowCells.map((input, cellIndex) => ( +
+
+
+ {input} +
+
+
+ ))} + {/* Remove button cell */} +
+
+ +
+
+
+ ) + }) + ) : emptyState ? ( +
+
+

{emptyState.title}

+ {emptyState.body} +
+
+ ) : null} +
+ +
+ +
+
+ ) +} diff --git a/app/ui/lib/Listbox.tsx b/app/ui/lib/Listbox.tsx index 71aef4a94f..b6be2bc60a 100644 --- a/app/ui/lib/Listbox.tsx +++ b/app/ui/lib/Listbox.tsx @@ -101,7 +101,7 @@ export const Listbox = ({ id={id} name={name} className={cn( - `flex h-11 items-center justify-between rounded border text-sans-md`, + `flex h-10 items-center justify-between rounded border text-sans-md`, hasError ? 'focus-error border-error-secondary hover:border-error' : 'border-default hover:border-hover', diff --git a/app/ui/lib/TextInput.tsx b/app/ui/lib/TextInput.tsx index d602f7ffde..20e71b2cd0 100644 --- a/app/ui/lib/TextInput.tsx +++ b/app/ui/lib/TextInput.tsx @@ -8,8 +8,11 @@ import { announce } from '@react-aria/live-announcer' import cn from 'classnames' import { useEffect } from 'react' +import { type FieldError } from 'react-hook-form' import type { Merge } from 'type-fest' +import { PopoverErrorMessage } from '~/components/form/fields/ErrorMessage' + import { CopyToClipboard } from './CopyToClipboard' /** @@ -59,18 +62,21 @@ export function TextInput({ copyable, as: asProp, ref, + popoverError, ...fieldProps -}: TextInputBaseProps & TextAreaProps) { +}: TextInputBaseProps & TextAreaProps & { popoverError?: FieldError }) { const Component = asProp || 'input' + const isTextArea = asProp === 'textarea' return (
+ {/* todo: interferes with copy to clipboard */} + {popoverError && ( + + )} {copyable && ( { +export const Tooltip = ({ + variant = 'default', + delay = 250, + children, + content, + placement, +}: TooltipProps) => { const [open, setOpen] = useState(false) const arrowRef = useRef(null) @@ -87,6 +94,21 @@ export const Tooltip = ({ delay = 250, children, content, placement }: TooltipPr const zIndex = usePopoverZIndex() + const variantConfig = { + default: { + classes: 'bg-raise border-secondary text-default', + stroke: 'var(--stroke-secondary)', + fill: 'var(--surface-raise)', + }, + error: { + classes: 'bg-error-secondary border-error-tertiary text-error', + stroke: 'var(--stroke-error-tertiary)', + fill: 'var(--surface-error-secondary)', + }, + } + + const config = variantConfig[variant] || variantConfig.default + if (!content) return child return ( @@ -96,9 +118,14 @@ export const Tooltip = ({ delay = 250, children, content, placement }: TooltipPr {open && (
{content} diff --git a/app/ui/styles/components/tooltip.css b/app/ui/styles/components/tooltip.css deleted file mode 100644 index 71ef14b081..0000000000 --- a/app/ui/styles/components/tooltip.css +++ /dev/null @@ -1,37 +0,0 @@ -/* - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, you can obtain one at https://mozilla.org/MPL/2.0/. - * - * Copyright Oxide Computer Company - */ - -.ox-tooltip { - @apply rounded border p-2 text-sans-md text-default bg-raise border-secondary elevation-2; -} - -.ox-tooltip-arrow { - position: absolute; - height: 12px; - width: 12px; - transform: rotate(-135deg); - @apply bg-raise border-secondary; -} - -.ox-tooltip[data-placement^='top'] .ox-tooltip-arrow { - @apply border-l border-t; - margin-top: 2.5px; -} - -.ox-tooltip[data-placement^='bottom'] .ox-tooltip-arrow { - @apply -top-[6.5px] border-b border-r; - margin-top: 0px; -} - -.ox-tooltip[data-placement^='right'] .ox-tooltip-arrow { - @apply -left-[6.5px] border-r border-t; -} - -.ox-tooltip[data-placement^='left'] .ox-tooltip-arrow { - @apply -right-[6.5px] border-b border-l; -} diff --git a/app/ui/styles/index.css b/app/ui/styles/index.css index cbfba33207..697ffc63ec 100644 --- a/app/ui/styles/index.css +++ b/app/ui/styles/index.css @@ -35,7 +35,6 @@ @import './components/side-modal.css'; @import './components/spinner.css'; @import './components/table.css'; -@import './components/tooltip.css'; @import './themes/selection.css'; diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index fbe3e4a5c0..ba0fe7266c 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -8,7 +8,7 @@ import { expect, test, type Locator, type Page } from '@playwright/test' -import { clickRowAction, expectRowVisible, selectOption, sleep } from './utils' +import { clickRowAction, expectRowVisible } from './utils' const defaultRules = ['allow-internal-inbound', 'allow-ssh', 'allow-icmp'] @@ -37,53 +37,92 @@ test('can create firewall rule', async ({ page }) => { await page.fill('role=textbox[name="Priority"]', '5') - // add targets with overlapping names and types to test delete - const targets = page.getByRole('table', { name: 'Targets' }) + // add new target + const targetsFieldset = page.getByRole('group', { name: 'targets' }) + await targetsFieldset.getByRole('button', { name: 'Add item' }).click() - await selectOption(page, 'Target type', 'IP') - await page.getByRole('textbox', { name: 'IP address' }).fill('192.168.0.1') - await page.getByRole('button', { name: 'Add target' }).click() - await expectRowVisible(targets, { Type: 'ip', Value: '192.168.0.1' }) + // change target type to IP and enter value + const targetsTable = targetsFieldset.getByRole('table') + await targetsTable.getByRole('button').first().click() // Open listbox - // add host filter instance "host-filter-instance" - await selectOption(page, 'Host type', 'Instance') - await page.getByRole('combobox', { name: 'Instance name' }).fill('host-filter-instance') + // changing type should reset the value + await page.getByRole('option', { name: 'Instance' }).click() + await targetsTable.getByRole('combobox').first().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 - const hosts = page.getByRole('table', { name: 'Host filters' }) - await expectRowVisible(hosts, { Type: 'instance', Value: 'host-filter-instance' }) - - const portRangeField = page.getByRole('textbox', { name: 'Port filters' }) - const invalidPort = page.getByRole('dialog').getByText('Not a valid port range') - const addPortButton = page.getByRole('button', { name: 'Add port filter' }) - await portRangeField.fill('abc') - await expect(invalidPort).toBeHidden() - await addPortButton.click() - await expect(invalidPort).toBeVisible() - - // test clear button - await page.getByRole('button', { name: 'Clear' }).nth(1).click() - await expect(portRangeField).toHaveValue('') - - await portRangeField.fill('123-456') - await addPortButton.click() - await expect(invalidPort).toBeHidden() - - // port range is added to port ranges table - const ports = page.getByRole('table', { name: 'Port filters' }) - await expectRowVisible(ports, { 'Port ranges': '123-456' }) - - const dupePort = page.getByRole('dialog').getByText('Port range already added') - await expect(dupePort).toBeHidden() - await portRangeField.fill('123-456') - // don't need to click because we're already validating onChange - await expect(dupePort).toBeVisible() - - // select UDP from protocol dropdown - await selectOption(page, 'Protocol filters', 'UDP') - await page.getByRole('button', { name: 'Add protocol' }).click() + await expect(targetsTable.getByRole('combobox')).toHaveValue('host-filter-instance') + await targetsTable.getByRole('button').first().click() + await page.getByRole('option', { name: 'VPC subnet', exact: true }).click() + await expect(page.getByText('host-filter-instance')).toBeHidden() + + await targetsTable.getByRole('button').first().click() // Open listbox + await page.getByRole('option', { name: 'IP', exact: true }).click() + + await targetsTable.getByRole('textbox').first().fill('192.168.0.1') + + // add host filter + const hostsFieldset = page.getByRole('group', { name: 'hosts' }) + await hostsFieldset.getByRole('button', { name: 'Add item' }).click() + + // change host type to Instance and enter value + const hostsTable = hostsFieldset.getByRole('table') + await hostsTable.getByRole('button').first().click() // Open listbox + await page.getByRole('option', { name: 'Instance' }).click() + await hostsTable.getByRole('combobox').first().fill('host-filter-instance') + await page.getByText('host-filter-instance').click() + + // add port filters + const portsFieldset = page.getByRole('group', { name: 'ports' }) + const portsTable = portsFieldset.getByRole('table') + + // add invalid port first + await portsFieldset.getByRole('button', { name: 'Add item' }).click() + await portsTable.getByRole('textbox').first().fill('abc') + + // add valid port + await portsFieldset.getByRole('button', { name: 'Add item' }).click() + await portsTable.getByRole('textbox').nth(1).fill('123-456') + + // add duplicate port + await portsFieldset.getByRole('button', { name: 'Add item' }).click() + await portsTable.getByRole('textbox').nth(2).fill('123-456') + + // add protocol filter + const protocolsFieldset = page.getByRole('group', { name: 'protocols' }) + await protocolsFieldset.getByRole('button', { name: 'Add item' }).click() + + // change protocol type to UDP + const protocolsTable = protocolsFieldset.getByRole('table') + await protocolsTable.getByRole('button').first().click() // Open listbox + await page.getByRole('option', { name: 'UDP' }).click() + + // try to submit with invalid data - should show validation errors + await page.getByRole('button', { name: 'Add rule' }).click() + + const invalidPortButton = page.getByRole('button', { + name: 'Error: Not a valid port range', + }) + await expect(invalidPortButton).toBeVisible() + await invalidPortButton.hover() + const invalidPortTooltip = page + .getByRole('tooltip') + .filter({ hasText: 'Not a valid port range' }) + await expect(invalidPortTooltip).toBeVisible() + + const dupePortButton = page + .getByRole('button', { + name: 'Error: Port range already added', + }) + .first() + await expect(dupePortButton).toBeVisible() + await dupePortButton.hover() + const dupePortTooltip = page + .getByRole('tooltip') + .filter({ hasText: 'Port range already added' }) + await expect(dupePortTooltip).toBeVisible() + + // remove invalid entries + await portsTable.getByRole('button', { name: 'Remove item 1' }).click() // Remove 'abc' + await portsTable.getByRole('button', { name: 'Remove item 2' }).click() // Remove duplicate '123-456' // submit the form await page.getByRole('button', { name: 'Add rule' }).click() @@ -156,66 +195,43 @@ test('firewall rule form targets table', async ({ page }) => { // open modal await page.getByRole('link', { name: 'New rule' }).click() - const targets = page.getByRole('table', { name: 'Targets' }) - const targetVpcNameField = page.getByRole('combobox', { name: 'VPC name' }).first() - - const addButton = page.getByRole('button', { name: 'Add target' }) - - // addButton should be disabled until a value is added - await expect(addButton).toBeDisabled() - - // 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 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' }) - - // add VPC subnet targets - const subnetNameField = page.getByRole('combobox', { name: 'Subnet name' }) - - // add a subnet by selecting from a dropdown; make sure 'mock-subnet' is present in the dropdown, - // even though VPC:'mock-subnet' has already been added - await selectOption(page, 'Target type', 'VPC subnet') - // addButton should be disabled again, as type has changed but no value has been entered - await expect(addButton).toBeDisabled() - await selectOption(page, subnetNameField, 'mock-subnet') - await expect(addButton).toBeEnabled() - await addButton.click() - await expectRowVisible(targets, { Type: 'subnet', Value: 'mock-subnet' }) - - // 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') + const targetsFieldset = page.getByRole('group', { name: 'targets' }) + const targets = targetsFieldset.getByRole('table') - // pressing enter twice here in quick succession causes test flake in firefox - // specifically and this fixes it - await sleep(300) + // add VPC target + await targetsFieldset.getByRole('button', { name: 'Add item' }).click() + await targets.getByRole('button').first().click() // Open VPC listbox + await page.getByRole('option', { name: 'VPC', exact: true }).click() + await targets.getByRole('combobox').first().fill('abc') + await page.getByText('abc').click() - await subnetNameField.press('Enter') - await expectRowVisible(targets, { Type: 'subnet', Value: 'abc-123' }) + // add another VPC target + await targetsFieldset.getByRole('button', { name: 'Add item' }).click() + await targets.getByRole('button').nth(2).click() // Open second VPC listbox + await page.getByRole('option', { name: 'VPC', exact: true }).click() + const targetsField2 = targets.getByRole('combobox').nth(1) + await targetsField2.fill('mock-vpc') + await targetsField2.press('Enter') + + // add VPC subnet target + await targetsFieldset.getByRole('button', { name: 'Add item' }).click() + await targets.getByRole('button').nth(4).click() // Open third listbox + await page.getByRole('option', { name: 'VPC subnet' }).click() + await targets.getByRole('combobox').nth(2).fill('mock-subnet') + await page.getByText('mock-subnet').click() + + // add another subnet by typing + await targetsFieldset.getByRole('button', { name: 'Add item' }).click() + await targets.getByRole('button').nth(6).click() // Open fourth listbox + await page.getByRole('option', { name: 'VPC subnet' }).click() + await targets.getByRole('combobox').nth(3).fill('abc-123') + await page.getByText('abc-123').click() // add IP target - await selectOption(page, 'Target type', 'IP') - await page.getByRole('textbox', { name: 'IP address' }).fill('192.168.0.1') - await addButton.click() - await expectRowVisible(targets, { Type: 'ip', Value: '192.168.0.1' }) + await targetsFieldset.getByRole('button', { name: 'Add item' }).click() + await targets.getByRole('button').nth(8).click() // Open fifth listbox + await page.getByRole('option', { name: 'IP', exact: true }).click() + await targets.getByRole('textbox').first().fill('192.168.0.1') // test table row delete; only keep the IP one // we start with 6 rows, because the header row counts as one @@ -225,68 +241,54 @@ test('firewall rule form targets table', async ({ page }) => { await deleteRowAndVerifyRowCount(targets, 3) await deleteRowAndVerifyRowCount(targets, 2) - // we still have the IP target - await expectRowVisible(targets, { Type: 'ip', Value: '192.168.0.1' }) + // verify the IP target remains + await expect(targets.getByRole('textbox')).toHaveValue('192.168.0.1') }) test('firewall rule form target validation', async ({ page }) => { await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules-new') - const addButton = page.getByRole('button', { name: 'Add target' }) - const targets = page.getByRole('table', { name: 'Targets' }) - - const formModal = page.getByRole('dialog', { name: 'Add firewall rule' }) - const nameError = formModal.getByText('Must end with a letter or number') - - // 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() - - // Change to IP, error should disappear and value field should be empty - await selectOption(page, 'Target type', 'IP') - await expect(nameError).toBeHidden() - const ipField = page.getByRole('textbox', { name: 'IP address' }) - await expect(ipField).toHaveValue('') - - // Type '1', error should not appear immediately (back to validating onSubmit) - await ipField.fill('1') - const ipError = formModal.getByText('Not a valid IP address') - await expect(ipError).toBeHidden() - await addButton.click() - await expect(ipError).toBeVisible() - - // test clear button - await page.getByRole('button', { name: 'Clear' }).first().click() - await expect(ipField).toHaveValue('') - - // Change back to VPC, enter valid value - await selectOption(page, 'Target type', 'VPC') - 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' }) + await page.fill('input[name=name]', 'my-new-rule') - // Switch to IP again - await selectOption(page, 'Target type', 'IP') + const targetsFieldset = page.getByRole('group', { name: 'targets' }) + const targets = targetsFieldset.getByRole('table') + + // add invalid VPC target + await targetsFieldset.getByRole('button', { name: 'Add item' }).click() + await targets.getByRole('button').first().click() // Open listbox + await page.getByRole('option', { name: 'VPC', exact: true }).click() + const vpcField = targets.getByRole('combobox').first() + await vpcField.fill('ab-') + await vpcField.press('Enter') + + // add invalid IP target + await targetsFieldset.getByRole('button', { name: 'Add item' }).click() + await targets.getByRole('button').nth(2).click() // Open second listbox + await page.getByRole('option', { name: 'IP', exact: true }).click() + const ipField = targets.getByRole('textbox').first() await ipField.fill('1') - // No error means we're back to validating on submit - await expect(ipError).toBeHidden() + // submit form to trigger validation + await page.getByRole('button', { name: 'Add rule' }).click() - // Hit submit to get error - await addButton.click() + // check for validation error buttons + const vpcError = targetsFieldset.getByRole('button', { + name: 'Error: Must end with a letter or number', + }) + const ipError = targetsFieldset.getByRole('button', { + name: 'Error: Not a valid IP address', + }) + await expect(vpcError).toBeVisible() await expect(ipError).toBeVisible() - // Fill out valid IP and submit + // fix the errors by changing values + await vpcField.fill('abc') + await vpcField.press('Enter') await ipField.fill('1.1.1.1') - await addButton.click() - await expect(ipError).toBeHidden() - await expectRowVisible(targets, { Type: 'ip', Value: '1.1.1.1' }) + + // submit again - should succeed + await page.getByRole('button', { name: 'Add rule' }).click() + await expect(page.getByRole('dialog', { name: 'Add firewall rule' })).toBeHidden() }) // This test may appear redundant because host and target share their logic, but @@ -297,67 +299,61 @@ test('firewall rule form target validation', async ({ page }) => { test('firewall rule form host validation', async ({ page }) => { await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules-new') - const addButton = page.getByRole('button', { name: 'Add host filter' }) - const hosts = page.getByRole('table', { name: 'Host filters' }) + await page.fill('input[name=name]', 'my-new-rule') + const hostsFieldset = page.getByRole('group', { name: 'hosts' }) + const hosts = hostsFieldset.getByRole('table') const formModal = page.getByRole('dialog', { name: 'Add firewall rule' }) - const nameError = formModal.getByText('Must end with a letter or number') - - // 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() - - // Change to IP, error should disappear and value field should be empty - await selectOption(page, 'Host type', 'IP') - await expect(nameError).toBeHidden() - const ipField = page.getByRole('textbox', { name: 'IP address' }) - await expect(ipField).toHaveValue('') - - // Type '1', error should not appear immediately (back to validating onSubmit) - await ipField.fill('1') - const ipError = formModal.getByText('Not a valid IP address') - await expect(ipError).toBeHidden() - await addButton.click() - await expect(ipError).toBeVisible() - - // test clear button - await page.getByRole('button', { name: 'Clear' }).nth(3).click() - await expect(ipField).toHaveValue('') - - // Change back to VPC, enter valid value - await selectOption(page, 'Host type', 'VPC') - 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' }) - // use subnet to be slightly different from the target validation test + // add invalid VPC host + await hostsFieldset.getByRole('button', { name: 'Add item' }).click() + await hosts.getByRole('button').first().click() // Open listbox + await page.getByRole('option', { name: 'VPC', exact: true }).click() + const vpcField = hosts.getByRole('combobox').first() + await vpcField.fill('ab-') + await vpcField.press('Enter') + + // add invalid IP host + await hostsFieldset.getByRole('button', { name: 'Add item' }).click() + await hosts.getByRole('button').nth(2).click() // Open second listbox + await page.getByRole('option', { name: 'IP', exact: true }).click() + const ipField = hosts.getByRole('textbox').first() + await ipField.fill('1') - // Switch to IP subnet - await selectOption(page, 'Host type', 'IP subnet') - const ipSubnetField = page.getByRole('textbox', { name: 'IP network' }) + // add invalid IP subnet host + await hostsFieldset.getByRole('button', { name: 'Add item' }).click() + await hosts.getByRole('button').nth(4).click() // Open third listbox + await page.getByRole('option', { name: 'IP subnet' }).click() + const ipSubnetField = hosts.getByRole('textbox').nth(1) await ipSubnetField.fill('1') + await vpcField.press('Enter') - // No error means we're back to validating on submit - const ipNetError = formModal.getByText( - 'Must contain an IP address and a width, separated by a /' - ) - await expect(ipNetError).toBeHidden() + // submit form to trigger validation + await page.getByRole('button', { name: 'Add rule' }).click() - // Hit submit to get error - await addButton.click() + // check for validation error buttons + const vpcError = hostsFieldset.getByRole('button', { + name: 'Error: Must end with a letter or number', + }) + const ipError = hostsFieldset.getByRole('button', { + name: 'Error: Not a valid IP address', + }) + const ipNetError = hostsFieldset.getByRole('button', { + name: 'Error: Must contain an IP address and a width, separated by a /', + }) + await expect(vpcError).toBeVisible() + await expect(ipError).toBeVisible() await expect(ipNetError).toBeVisible() - // Fill out valid IP and submit + // fix the errors + await vpcField.fill('abc') + await vpcField.press('Enter') + await ipField.fill('1.1.1.1') await ipSubnetField.fill('1.1.1.1/1') - await addButton.click() - await expect(ipNetError).toBeHidden() - await expectRowVisible(hosts, { Type: 'ip_net', Value: '1.1.1.1/1' }) + + // submit again - should succeed + await page.getByRole('button', { name: 'Add rule' }).click() + await expect(formModal).toBeHidden() }) test('firewall rule form hosts table', async ({ page }) => { @@ -367,37 +363,46 @@ test('firewall rule form hosts table', async ({ page }) => { // open modal await page.getByRole('link', { name: 'New rule' }).click() - const hosts = page.getByRole('table', { name: 'Host filters' }) - const hostFiltersVpcNameField = page.getByRole('combobox', { name: 'VPC name' }).nth(1) - const addButton = page.getByRole('button', { name: 'Add host filter' }) - - // 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' }) - - await selectOption(page, 'Host type', 'VPC subnet') - await selectOption(page, 'Subnet name', 'mock-subnet') - await addButton.click() - 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') - await addButton.click() - await expectRowVisible(hosts, { Type: 'subnet', Value: 'abc' }) + await page.fill('input[name=name]', 'my-new-rule') - await selectOption(page, 'Host type', 'IP') - await page.getByRole('textbox', { name: 'IP address' }).fill('192.168.0.1') - await addButton.click() - await expectRowVisible(hosts, { Type: 'ip', Value: '192.168.0.1' }) + const hostsFieldset = page.getByRole('group', { name: 'hosts' }) + const hosts = hostsFieldset.getByRole('table') + + // add VPC hosts + await hostsFieldset.getByRole('button', { name: 'Add item' }).click() + await hosts.getByRole('button').first().click() // Open first listbox + await page.getByRole('option', { name: 'VPC', exact: true }).click() + const hostsField1 = hosts.getByRole('combobox').first() + await hostsField1.fill('abc') + await hostsField1.press('Enter') + + await hostsFieldset.getByRole('button', { name: 'Add item' }).click() + await hosts.getByRole('button').nth(2).click() // Open second listbox + await page.getByRole('option', { name: 'VPC', exact: true }).click() + const hostsField2 = hosts.getByRole('combobox').nth(1) + await hostsField2.fill('def') + await hostsField2.press('Enter') + + // add subnet hosts + await hostsFieldset.getByRole('button', { name: 'Add item' }).click() + await hosts.getByRole('button').nth(4).click() // Open third listbox + await page.getByRole('option', { name: 'VPC subnet' }).click() + const hostsField3 = hosts.getByRole('combobox').nth(2) + await hostsField3.fill('mock-subnet') + await hostsField3.press('Enter') + + await hostsFieldset.getByRole('button', { name: 'Add item' }).click() + await hosts.getByRole('button').nth(6).click() // Open fourth listbox + await page.getByRole('option', { name: 'VPC subnet' }).click() + const hostsField4 = hosts.getByRole('combobox').nth(3) + await hostsField4.fill('abc') + await hostsField4.press('Enter') + + // add IP host + await hostsFieldset.getByRole('button', { name: 'Add item' }).click() + await hosts.getByRole('button').nth(8).click() // Open fifth listbox + await page.getByRole('option', { name: 'IP', exact: true }).click() + await hosts.getByRole('textbox').first().fill('192.168.0.1') // test table row delete; only keep the IP one await expect(hosts.getByRole('row')).toHaveCount(6) @@ -406,8 +411,8 @@ test('firewall rule form hosts table', async ({ page }) => { await deleteRowAndVerifyRowCount(hosts, 3) await deleteRowAndVerifyRowCount(hosts, 2) - // we still have the IP target - await expectRowVisible(hosts, { Type: 'ip', Value: '192.168.0.1' }) + // verify the IP host remains + await expect(hosts.getByRole('textbox')).toHaveValue('192.168.0.1') }) test('can update firewall rule', async ({ page }) => { @@ -440,35 +445,29 @@ test('can update firewall rule', async ({ page }) => { await expect(page.getByRole('textbox', { name: 'Priority' })).toHaveValue('65534') // protocol is populated in the table - const protocolTable = page.getByRole('table', { name: 'Protocol filters' }) - await expect(protocolTable.getByText('ICMP')).toBeVisible() + const protocolsFieldset = page.getByRole('group', { name: 'protocols' }) + const protocolTable = protocolsFieldset.getByRole('table') + await expect(protocolTable.getByRole('button', { name: 'ICMP' })).toBeVisible() // remove the existing ICMP protocol filter - await protocolTable.getByRole('button', { name: 'remove' }).click() - - // 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') - await page.getByRole('textbox', { name: 'ICMP Code' }).fill('0') - await page.getByRole('button', { name: 'Add protocol' }).click() + await protocolTable.getByRole('button', { name: 'Remove item 1' }).click() - // targets default vpc - // screen.getByRole('cell', { name: 'vpc' }) - // screen.getByRole('cell', { name: 'default' }) + // add a new TCP protocol filter + await protocolsFieldset.getByRole('button', { name: 'Add item' }).click() + await protocolTable.getByRole('button').first().click() // Open listbox + await page.getByRole('option', { name: 'TCP' }).click() // update name await page.getByRole('textbox', { name: 'Name' }).fill('new-rule-name') // add host filter - await selectOption(page, 'Host type', 'VPC subnet') - await page.getByRole('combobox', { name: 'Subnet name' }).fill('edit-filter-subnet') + const hostsFieldset = page.getByRole('group', { name: 'Hosts' }) + await hostsFieldset.getByRole('button', { name: 'Add item' }).click() + const hosts = hostsFieldset.getByRole('table') + await hosts.getByRole('button').first().click() // Open listbox + await page.getByRole('option', { name: 'VPC subnet' }).click() + await hosts.getByRole('combobox').first().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 - const hosts = page.getByRole('table', { name: 'Host filters' }) - await expectRowVisible(hosts, { Type: 'subnet', Value: 'edit-filter-subnet' }) // submit the form await page.getByRole('button', { name: 'Update rule' }).click() @@ -482,16 +481,10 @@ test('can update firewall rule', async ({ page }) => { await expect(rows).toHaveCount(3) - // new host filter shows up in filters cell, along with the new ICMP protocol - await expect(page.locator('text=subnetedit-filter-subnetICMP')).toBeVisible() + // new host filter shows up in filters cell, along with the new TCP protocol + await expect(page.locator('text=subnetedit-filter-subnetTCP')).toBeVisible() - // scroll table sideways past the filters cell to see the full content - await page.getByText('Enabled').first().scrollIntoViewIfNeeded() - - // Look for the new ICMP type 3 code 0 in the filters cell using ProtocolBadge format - await expect(page.getByText('TYPE 3CODE 0')).toBeVisible() - - // other 3 rules are still there + // other rules are still there const rest = defaultRules.filter((r) => r !== 'allow-icmp') for (const name of rest) { await expect(page.locator(`text="${name}"`)).toBeVisible() @@ -513,18 +506,20 @@ test('create from existing rule', async ({ page }) => { 'allow-icmp-copy' ) - // protocol is populated in the table - const protocolTable = modal.getByRole('table', { name: 'Protocol filters' }) - await expect(protocolTable.getByText('ICMP')).toBeVisible() - await expect(protocolTable.getByText('TCP')).toBeHidden() - await expect(protocolTable.getByText('UDP')).toBeHidden() + // protocol is populated in the modal + const protocolsFieldset = modal.getByRole('group', { name: 'protocols' }) + const protocolTable = protocolsFieldset.getByRole('table') + await expect(protocolTable.getByRole('button', { name: 'ICMP' })).toBeVisible() // no port filters - const portFilters = modal.getByRole('table', { name: 'Port filters' }) - await expect(portFilters).toBeHidden() + const portFieldset = modal.getByRole('group', { name: 'ports' }) + const portTable = portFieldset.getByRole('table') + await expect(portTable.getByText('No ports')).toBeVisible() - const targets = modal.getByRole('table', { name: 'Targets' }) - await expect(targets.getByRole('row', { name: 'vpc default' })).toBeVisible() + // targets contain default VPC + const targetsFieldset = modal.getByRole('group', { name: 'targets' }) + const targets = targetsFieldset.getByRole('table') + await expect(targets.getByRole('combobox').first()).toHaveValue('default') // close the modal await page.keyboard.press('Escape') @@ -538,14 +533,17 @@ test('create from existing rule', async ({ page }) => { 'allow-ssh-copy' ) - await expect(portFilters.getByRole('cell', { name: '22', exact: true })).toBeVisible() + await expect(portTable.getByRole('textbox').first()).toHaveValue('22') - // protocol is populated in the table - await expect(protocolTable.getByText('TCP')).toBeVisible() - await expect(protocolTable.getByText('UDP')).toBeHidden() - await expect(protocolTable.getByText('ICMP')).toBeHidden() + // protocol is populated in the modal + await expect(protocolTable.getByRole('button', { name: 'TCP' })).toBeVisible() + + await expect(targets.getByRole('combobox').first()).toHaveValue('default') - await expect(targets.getByRole('row', { name: 'vpc default' })).toBeVisible() + // submit the form + await page.getByRole('button', { name: 'Add rule' }).click() + + await expect(page.locator('tbody >> tr')).toHaveCount(4) }) const rulePath = '/projects/mock-project/vpcs/mock-vpc/firewall-rules/allow-icmp/edit' @@ -580,32 +578,30 @@ test('name conflict error on create', async ({ page }) => { // when editing a rule, on the other hand, we only check for conflicts against rules // other than the one being edited, because of course its name can stay the same test('name conflict error on edit', async ({ page }) => { - await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules/allow-icmp/edit') + await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules/allow-ssh/edit') // changing the name to one taken by another rule is an error const nameField = page.getByRole('textbox', { name: 'Name', exact: true }) - await nameField.fill('allow-ssh') + await nameField.fill('allow-icmp') + await page.getByRole('button', { name: 'Update rule' }).click() + // change name back + await nameField.fill('allow-ssh') const error = page.getByRole('dialog').getByText('Name taken') await expect(error).toBeHidden() + // wait for modal to close after successful update await page.getByRole('button', { name: 'Update rule' }).click() - await expect(error).toBeVisible() - - // change name back - await nameField.fill('allow-icmp') + const modal = page.getByRole('dialog', { name: 'Edit rule' }) + await expect(modal).toBeHidden() - // changing a value _without_ changing the name is allowed - await page.getByRole('textbox', { name: 'Priority' }).fill('37') - await page.getByRole('button', { name: 'Update rule' }).click() - await expect(error).toBeHidden() - await expectRowVisible(page.getByRole('table'), { Name: 'allow-icmp', Priority: '37' }) + await expectRowVisible(page.getByRole('table'), { Name: 'allow-ssh' }) // changing the name to a non-conflicting name is allowed - await page.getByRole('link', { name: 'allow-icmp' }).click() - await nameField.fill('allow-icmp2') + await page.getByRole('link', { name: 'allow-ssh' }).click() + await nameField.fill('allow-ssh2') await page.getByRole('button', { name: 'Update rule' }).click() - await expectRowVisible(page.getByRole('table'), { Name: 'allow-icmp2', Priority: '37' }) + await expectRowVisible(page.getByRole('table'), { Name: 'allow-ssh2' }) }) async function expectOptions(page: Page, options: string[]) { @@ -619,8 +615,13 @@ async function expectOptions(page: Page, options: string[]) { test('arbitrary values combobox', async ({ page }) => { await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules-new') + // Add a target row first to access the VPC input + const targetsFieldset = page.getByRole('group', { name: 'targets' }) + await targetsFieldset.getByRole('button', { name: 'Add item' }).click() + const targets = targetsFieldset.getByRole('table') + // 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() + const vpcInput = targets.getByRole('combobox').first() await vpcInput.focus() await expectOptions(page, ['mock-vpc']) @@ -628,15 +629,20 @@ test('arbitrary values combobox', async ({ page }) => { await expectOptions(page, ['Custom: d']) await vpcInput.blur() - page.getByRole('button', { name: 'Add target' }).click() - await expect(vpcInput).toHaveValue('') + await expect(vpcInput).toHaveValue('d') await vpcInput.focus() + await expectOptions(page, ['Custom: d']) + + // clear and test again + await vpcInput.clear() 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' }) + // test keeping query around on blur - change type to Instance + await targets.getByRole('button').first().click() // Open type listbox for first row + await page.getByRole('option', { name: 'Instance' }).click() + + const input = targets.getByRole('combobox').first() await input.focus() await expectOptions(page, ['db1', 'you-fail', 'not-there-yet', 'db2']) @@ -653,18 +659,19 @@ test('arbitrary values combobox', async ({ page }) => { // same options show up after blur (there was a bug around this) await expectOptions(page, ['db1', 'db2', 'Custom: d']) - // make sure typing in ICMP filter input actually updates the underlying value, - // triggering a validation error for bad input. without onInputChange binding - // the input value to the form value, this does not trigger an error because - // the form thinks the input is empyt. - await selectOption(page, 'Protocol filters', 'ICMP') - await page.getByRole('combobox', { name: 'ICMP type' }).pressSequentially('abc') - const error = page - .getByRole('dialog') - .getByText('ICMP type must be a number between 0 and 255') - await expect(error).toBeHidden() - await page.getByRole('button', { name: 'Add protocol filter' }).click() - await expect(error).toBeVisible() + // test typing in ICMP filter input triggers validation error for bad input + const protocolsFieldset = page.getByRole('group', { name: 'protocols' }) + await protocolsFieldset.getByRole('button', { name: 'Add item' }).click() + const protocolTable = protocolsFieldset.getByRole('table') + await protocolTable.getByRole('button').first().click() + await page.getByRole('option', { name: 'ICMP' }).click() + await protocolTable.getByRole('combobox').first().fill('abc') + + // Submit form to trigger validation + await page.getByRole('button', { name: 'Add rule' }).click() + + // Check that the form submission was prevented + await expect(page.getByRole('dialog', { name: 'Add firewall rule' })).toBeVisible() }) test("esc in combobox doesn't close form", async ({ page }) => { @@ -684,17 +691,20 @@ test("esc in combobox doesn't close form", async ({ page }) => { const formModal = page.getByRole('dialog', { name: 'Add firewall rule' }) await expect(formModal).toBeVisible() - const input = page.getByRole('combobox', { name: 'VPC name' }).first() - await input.focus() + // add a target to access the VPC combobox + const targetsFieldset = page.getByRole('group', { name: 'targets' }) + await targetsFieldset.getByRole('button', { name: 'Add item' }).click() + const targets = targetsFieldset.getByRole('table') - await expect(page.getByRole('option').first()).toBeVisible() - await expectOptions(page, ['mock-vpc']) + // VPC is the default type, so we can directly access the combobox + const input = targets.getByRole('combobox').first() + await input.focus() await page.keyboard.press('Escape') - // options are closed, but the whole form modal is not + // form modal should still be visible, not closed by escape in input await expect(confirmModal).toBeHidden() - await expect(page.getByRole('option')).toBeHidden() await expect(formModal).toBeVisible() + // now press esc again to leave the form await page.keyboard.press('Escape') await expect(confirmModal).toBeVisible()