From d411e68eb96c444262b069e41126b781a580a916 Mon Sep 17 00:00:00 2001 From: Benjamin Leonard Date: Mon, 11 Aug 2025 11:22:32 +0100 Subject: [PATCH 01/10] Mini table input first pass --- app/components/form/fields/ComboboxField.tsx | 38 +- app/components/form/fields/ErrorMessage.tsx | 29 + app/components/form/fields/ListboxField.tsx | 4 +- app/components/form/fields/TextField.tsx | 33 +- app/forms/firewall-rules-common.tsx | 710 ++++++++----------- app/forms/firewall-rules-create.tsx | 1 + app/ui/lib/Combobox.tsx | 17 +- app/ui/lib/DatePicker.tsx | 2 +- app/ui/lib/DateRangePicker.tsx | 2 +- app/ui/lib/InputMiniTable.tsx | 163 +++++ app/ui/lib/Listbox.tsx | 2 +- app/ui/lib/TextInput.tsx | 18 +- app/ui/lib/Tooltip.tsx | 34 +- app/ui/styles/components/mini-table.css | 4 +- app/ui/styles/components/tooltip.css | 37 - app/ui/styles/index.css | 1 - 16 files changed, 617 insertions(+), 478 deletions(-) create mode 100644 app/ui/lib/InputMiniTable.tsx delete mode 100644 app/ui/styles/components/tooltip.css diff --git a/app/components/form/fields/ComboboxField.tsx b/app/components/form/fields/ComboboxField.tsx index efa0558293..4c505583c4 100644 --- a/app/components/form/fields/ComboboxField.tsx +++ b/app/components/form/fields/ComboboxField.tsx @@ -75,7 +75,6 @@ export function ComboboxField< return (
) } + +export const ComboboxFieldCell = < + TFieldValues extends FieldValues, + TName extends FieldPath, +>({ + name, + label = capitalize(name), + validate, + items, + ...props +}: ComboboxFieldProps) => { + const { + field, + fieldState: { error }, + } = useController({ name, control: props.control, rules: { validate } }) + + const [selectedItemLabel, setSelectedItemLabel] = useState( + getSelectedLabelFromValue(items, field.value || '') + ) + + return ( + <> + { + setSelectedItemLabel(getSelectedLabelFromValue(items, value)) + field.onChange(value) + }} + hasError={!!error} + /> + + + ) +} diff --git a/app/components/form/fields/ErrorMessage.tsx b/app/components/form/fields/ErrorMessage.tsx index f804f46189..9d697aea1a 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,27 @@ 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..f4ca7119d4 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -6,15 +6,14 @@ * 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 } from 'react-hook-form' import { usePrefetchedApiQuery, type ApiError, type Instance, type Vpc, - type VpcFirewallIcmpFilter, type VpcFirewallRuleAction, type VpcFirewallRuleDirection, type VpcFirewallRuleHostFilter, @@ -30,26 +29,19 @@ 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' @@ -65,43 +57,84 @@ import { type FirewallRuleValues } from './firewall-rules-util' * CommonFields component. */ -type TargetAndHostFilterType = - | VpcFirewallRuleTarget['type'] - | VpcFirewallRuleHostFilter['type'] - -type TargetAndHostFormValues = { - type: TargetAndHostFilterType - value: string -} +// type TargetAndHostFilterType = +// | VpcFirewallRuleTarget['type'] +// | VpcFirewallRuleHostFilter['type'] // 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', +// todo: add to placeholder? info icon instead? +// 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 }: { control: Control }) => ( + <> + 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. +

+ } - } -} + /> -const TargetAndHostFilterSubform = ({ - sectionType, +
+ [ + , + , + ]} + emptyState={{ title: 'No targets', body: 'Add a target to see it here' }} + defaultValue={{ type: 'vpc', value: '' }} + columnWidths={['1.25fr', '2fr']} + /> +
+ +) + +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 +148,46 @@ 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) + } + /> + ) : ( + + (type === 'ip' && validateIp(value)) || + (type === 'ip_net' && validateIpNet(value)) || + undefined + } + popoverError + /> ) } @@ -258,21 +210,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 +249,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 => { @@ -402,119 +297,166 @@ const icmpCodeValidation = (value: string | undefined) => { } const ProtocolFilters = ({ control }: { control: Control }) => { - const protocols = useController({ name: 'protocols', control }).field - const protocolForm = useForm({ - defaultValues: { protocolType: '' }, - }) + 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 [ + , + selectedType === 'icmp' ? ( + { + if (!value) return undefined + const result = parseIcmpType(String(value)) + if (!result.success) return result.message + }} + /> + ) : ( + + ), + 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 }: { control: Control }) => { + 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 [ + , + selectedType === 'vpc' || selectedType === 'subnet' || selectedType === 'instance' ? ( + validateName(value, `Host filter name`, false)} + /> + ) : ( + + (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} - /> - )} - - )} -
+ Host filters - protocolForm.reset()} - onSubmit={submitProtocol} - /> -
+ + 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. + + } + /> - {protocols.value.length > 0 && ( - `Remove ${getProtocolDisplayName(protocol)}`} - /> - )} + ) } @@ -522,26 +464,23 @@ const ProtocolFilters = ({ control }: { control: Control }) type CommonFieldsProps = { control: Control nameTaken: (name: string) => boolean - error: ApiError | null + error?: ApiError + getValues: () => FirewallRuleValues + trigger: (name: string) => Promise } -const targetAndHostDefaultValues: TargetAndHostFormValues = { - type: 'vpc', - value: '', -} +export const CommonFields = ({ control, trigger, nameTaken, error }: CommonFieldsProps) => { + const ports = useWatch({ name: 'ports', control }) + const { errors } = useFormState({ control }) -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() - }) + // 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 ( <> @@ -613,23 +552,7 @@ 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. -

- - } - /> + @@ -647,67 +570,40 @@ 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 - - + 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' - }} + [ + { + 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="" />
- 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}`} - /> - )} - - 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. - - } - /> + {error && ( <> diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index 6aeacbee67..588047a09b 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -128,6 +128,7 @@ export default function CreateFirewallRuleForm() { control={form.control} // error if name is already in use nameTaken={(name) => !!existingRules.find((r) => r.name === name)} + trigger={form.trigger} error={updateRules.error} // TODO: there should also be a form-level error so if the name is off // screen, it doesn't look like the submit button isn't working. Maybe diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index d146c09129..bde96e79a7 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -47,7 +47,7 @@ export type ComboboxBaseProps = { disabled?: boolean isLoading?: boolean items: Array - label: string + label?: string placeholder?: string required?: boolean hideOptionalTag?: boolean @@ -68,6 +68,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 = { @@ -98,6 +100,7 @@ export const Combobox = ({ hideOptionalTag, inputRef, transform, + matchDropdownWidth = true, ...props }: ComboboxProps) => { const [query, setQuery] = useState(selectedItemValue || '') @@ -176,7 +179,7 @@ export const Combobox = ({ )}
{filteredItems.map((item) => ( 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..336163ebbe --- /dev/null +++ b/app/ui/lib/InputMiniTable.tsx @@ -0,0 +1,163 @@ +/* + * 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} +
+
+
+ )) + .concat([ + // Remove button cell +
+
+ +
+
, + ]) + }) + .flat() + ) : 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..e7a6d21923 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,13 +62,14 @@ export function TextInput({ copyable, as: asProp, ref, + popoverError, ...fieldProps -}: TextInputBaseProps & TextAreaProps) { +}: TextInputBaseProps & TextAreaProps & { popoverError?: FieldError }) { const Component = asProp || 'input' 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,7 +118,11 @@ export const Tooltip = ({ delay = 250, children, content, placement }: TooltipPr {open && (
@@ -106,8 +132,8 @@ export const Tooltip = ({ delay = 250, children, content, placement }: TooltipPr height={8} strokeWidth={1} tipRadius={2} - stroke="var(--stroke-secondary)" - fill="var(--surface-raise)" + stroke={config.stroke} + fill={config.fill} ref={arrowRef} context={context} /> diff --git a/app/ui/styles/components/mini-table.css b/app/ui/styles/components/mini-table.css index 3bf03222fa..45986bb44f 100644 --- a/app/ui/styles/components/mini-table.css +++ b/app/ui/styles/components/mini-table.css @@ -23,7 +23,7 @@ } /* a fake left border for all cells that aren't first */ - & td + td:before { + &:not(.interactive) td + td:before { @apply absolute bottom-[2px] top-[calc(0.5rem+1px)] block w-[1px] border-l border-secondary; content: ' '; } @@ -38,7 +38,7 @@ /* all divs */ & td > div { - @apply flex h-9 items-center border border-y border-r-0 py-3 pl-3 pr-6 border-default; + @apply flex h-10 items-center border border-y border-r-0 py-3 pl-3 pr-6 border-default; } /* first cell's div */ 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'; From 6d9647c75dff403c9f4910bfcdb9502cdedadc4b Mon Sep 17 00:00:00 2001 From: Benjamin Leonard Date: Mon, 11 Aug 2025 12:39:33 +0100 Subject: [PATCH 02/10] Truncation tweaks --- app/ui/lib/Combobox.tsx | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index bde96e79a7..816b699e8c 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -139,9 +139,10 @@ export const Combobox = ({ filteredItems.push({ value: query, label: ( - <> - Custom: {query} - +
+ Custom:{' '} + {query} +
), selectedLabel: query, }) @@ -254,9 +255,9 @@ export const Combobox = ({ // 13px gap is presumably because it's measured from inside the outline or something className={cn( 'ox-menu pointer-events-auto relative overflow-y-auto border !outline-none border-secondary [--anchor-gap:13px] empty:hidden', - matchDropdownWidth // todo: set max and dont extend outside of continer + matchDropdownWidth // todo: be a bit smarter about the width so it doesnt extend outside of container ? 'w-[calc(var(--input-width)+var(--button-width))]' - : 'min-w-[calc(var(--input-width)+var(--button-width))]', + : 'min-w-[15rem]', zIndex )} modal={false} From 911871ec79d914aaa251c499364cea96edfce1e8 Mon Sep 17 00:00:00 2001 From: Benjamin Leonard Date: Mon, 11 Aug 2025 15:06:43 +0100 Subject: [PATCH 03/10] Little clean --- app/components/form/fields/ComboboxField.tsx | 37 -------------------- app/ui/styles/components/mini-table.css | 4 +-- 2 files changed, 2 insertions(+), 39 deletions(-) diff --git a/app/components/form/fields/ComboboxField.tsx b/app/components/form/fields/ComboboxField.tsx index 4c505583c4..eb6e0d736d 100644 --- a/app/components/form/fields/ComboboxField.tsx +++ b/app/components/form/fields/ComboboxField.tsx @@ -96,40 +96,3 @@ export function ComboboxField<
) } - -export const ComboboxFieldCell = < - TFieldValues extends FieldValues, - TName extends FieldPath, ->({ - name, - label = capitalize(name), - validate, - items, - ...props -}: ComboboxFieldProps) => { - const { - field, - fieldState: { error }, - } = useController({ name, control: props.control, rules: { validate } }) - - const [selectedItemLabel, setSelectedItemLabel] = useState( - getSelectedLabelFromValue(items, field.value || '') - ) - - return ( - <> - { - setSelectedItemLabel(getSelectedLabelFromValue(items, value)) - field.onChange(value) - }} - hasError={!!error} - /> - - - ) -} diff --git a/app/ui/styles/components/mini-table.css b/app/ui/styles/components/mini-table.css index 45986bb44f..3bf03222fa 100644 --- a/app/ui/styles/components/mini-table.css +++ b/app/ui/styles/components/mini-table.css @@ -23,7 +23,7 @@ } /* a fake left border for all cells that aren't first */ - &:not(.interactive) td + td:before { + & td + td:before { @apply absolute bottom-[2px] top-[calc(0.5rem+1px)] block w-[1px] border-l border-secondary; content: ' '; } @@ -38,7 +38,7 @@ /* all divs */ & td > div { - @apply flex h-10 items-center border border-y border-r-0 py-3 pl-3 pr-6 border-default; + @apply flex h-9 items-center border border-y border-r-0 py-3 pl-3 pr-6 border-default; } /* first cell's div */ From d857a517226667599cd4ec74dc44723cfc7092f4 Mon Sep 17 00:00:00 2001 From: Benjamin Leonard Date: Tue, 12 Aug 2025 10:40:14 +0100 Subject: [PATCH 04/10] Fix missing text input label --- app/components/form/fields/DescriptionField.tsx | 11 ++++++++++- app/ui/lib/TextInput.tsx | 9 ++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) 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/ui/lib/TextInput.tsx b/app/ui/lib/TextInput.tsx index e7a6d21923..6f45421593 100644 --- a/app/ui/lib/TextInput.tsx +++ b/app/ui/lib/TextInput.tsx @@ -66,15 +66,17 @@ export function TextInput({ ...fieldProps }: TextInputBaseProps & TextAreaProps & { popoverError?: FieldError }) { const Component = asProp || 'input' + const isTextArea = asProp === 'textarea' return (
Date: Tue, 12 Aug 2025 10:40:33 +0100 Subject: [PATCH 05/10] Remove overzealous conditional rendering --- app/ui/lib/Combobox.tsx | 98 ++++++++++++++++++++--------------------- 1 file changed, 47 insertions(+), 51 deletions(-) diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index 816b699e8c..8385ed45c8 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -237,60 +237,56 @@ export const Combobox = ({ hasError && 'focus-error' )} /> - {items.length > 0 && ( - - - - )} -
- {(items.length > 0 || allowArbitraryValues) && ( - - {filteredItems.map((item) => ( - - {({ focus, selected }) => ( - // This *could* be done with data-[focus] and data-[selected] instead, but - // it would be a lot more verbose. those can only be used with TW classes, - // not our .is-selected and .is-highlighted, so we'd have to copy the pieces - // of those rules one by one. Better to rely on the shared classes. -
- {item.label} -
- )} - - ))} - {!allowArbitraryValues && filteredItems.length === 0 && ( - -
No items match
- - )} - - )} + + +
+ + {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
+ + )} +
)} From b3a37f3355008b8c09b282245f606b05d485aedd Mon Sep 17 00:00:00 2001 From: Benjamin Leonard Date: Tue, 12 Aug 2025 10:40:56 +0100 Subject: [PATCH 06/10] Pass form instead of control --- app/forms/firewall-rules-common.tsx | 99 +++++++++++++++-------------- app/forms/firewall-rules-create.tsx | 3 +- app/forms/firewall-rules-edit.tsx | 2 +- 3 files changed, 53 insertions(+), 51 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index f4ca7119d4..ddc46d30e4 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -7,7 +7,7 @@ */ import { useEffect } from 'react' -import { useFormState, useWatch, type Control } from 'react-hook-form' +import { useFormState, useWatch, type Control, type UseFormReturn } from 'react-hook-form' import { usePrefetchedApiQuery, @@ -46,43 +46,21 @@ 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. - */ +const valuePlaceholders = { + vpc: 'example-vpc', + subnet: 'example-subnet', + instance: 'example-inst', + ip: 'IPv4 or IPv6 address', + ip_net: 'IP network', +} -// type TargetAndHostFilterType = -// | VpcFirewallRuleTarget['type'] -// | VpcFirewallRuleHostFilter['type'] - -// these are part of the target and host filter form; -// the specific values depend on the target or host filter type selected -// todo: add to placeholder? info icon instead? -// 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 }: { control: Control }) => ( +const TargetFilter = ({ + control, + onTypeChange, +}: { + control: Control + onTypeChange: (index: number) => void +}) => ( <> Targets @@ -114,6 +92,7 @@ const TargetFilter = ({ control }: { control: Control }) => items={targetHostTypeItems} name={`targets.${index}.type`} control={control} + onChange={() => onTypeChange(index)} />, ) : ( ) } @@ -296,7 +277,13 @@ const icmpCodeValidation = (value: string | undefined) => { return 'ICMP code must be a number or numeric range' } -const ProtocolFilters = ({ control }: { control: Control }) => { +const ProtocolFilters = ({ + control, + onTypeChange, +}: { + control: Control + onTypeChange: (index: number) => void +}) => { const protocols = useWatch({ name: 'protocols', control }) || [] const renderProtocolRow = ( @@ -313,6 +300,7 @@ const ProtocolFilters = ({ control }: { control: Control }) control={control} items={protocolTypeItems} hideOptionalTag + onChange={() => onTypeChange(index)} />, selectedType === 'icmp' ? ( }) ) } -const HostFilters = ({ control }: { control: Control }) => { +const HostFilters = ({ + control, + onTypeChange, +}: { + control: Control + onTypeChange: (index: number) => void +}) => { const { project, vpc } = useVpcSelector() const { data: { items: instances }, @@ -406,6 +400,7 @@ const HostFilters = ({ control }: { control: Control }) => { control={control} items={targetHostTypeItems} hideOptionalTag + onChange={() => onTypeChange(index)} />, selectedType === 'vpc' || selectedType === 'subnet' || selectedType === 'instance' ? ( }) => { } type CommonFieldsProps = { - control: Control + form: UseFormReturn nameTaken: (name: string) => boolean - error?: ApiError - getValues: () => FirewallRuleValues - trigger: (name: string) => Promise + error: ApiError | null } -export const CommonFields = ({ control, trigger, nameTaken, error }: CommonFieldsProps) => { - const ports = useWatch({ name: 'ports', control }) +export const CommonFields = ({ form, nameTaken, error }: CommonFieldsProps) => { + const { control, trigger, setValue } = form const { errors } = useFormState({ control }) + const ports = useWatch({ name: 'ports', control }) // Helps catch errors that span multiple fields // without this, errors won't be cleared on one duplicate // field if the other is edited @@ -552,7 +546,10 @@ export const CommonFields = ({ control, trigger, nameTaken, error }: CommonField - + setValue(`targets.${index}.value`, '')} + /> @@ -599,11 +596,17 @@ export const CommonFields = ({ control, trigger, nameTaken, error }: CommonField
- + setValue(`protocols.${index}.value`, null)} + /> - + setValue(`hosts.${index}.value`, '')} + /> {error && ( <> diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index 588047a09b..b2cfb4883e 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -125,10 +125,9 @@ export default function CreateFirewallRuleForm() { submitLabel="Add rule" > !!existingRules.find((r) => r.name === name)} - trigger={form.trigger} error={updateRules.error} // TODO: there should also be a form-level error so if the name is off // screen, it doesn't look like the submit button isn't working. Maybe 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} From b23406e0e03bf7d9c3ea5a028dcbcb03dbd9660e Mon Sep 17 00:00:00 2001 From: Benjamin Leonard Date: Tue, 12 Aug 2025 11:01:45 +0100 Subject: [PATCH 07/10] Extracting `PortsFilters` --- app/forms/firewall-rules-common.tsx | 97 +++++++++++++++++------------ 1 file changed, 58 insertions(+), 39 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index ddc46d30e4..fd35fff447 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -7,7 +7,13 @@ */ import { useEffect } from 'react' -import { useFormState, useWatch, type Control, type UseFormReturn } from 'react-hook-form' +import { + useFormState, + useWatch, + type Control, + type UseFormReturn, + type UseFormTrigger, +} from 'react-hook-form' import { usePrefetchedApiQuery, @@ -456,17 +462,16 @@ const HostFilters = ({ ) } -type CommonFieldsProps = { - form: UseFormReturn - nameTaken: (name: string) => boolean - error: ApiError | null -} - -export const CommonFields = ({ form, nameTaken, error }: CommonFieldsProps) => { - const { control, trigger, setValue } = form +const PortFilters = ({ + control, + trigger, +}: { + control: Control + trigger: UseFormTrigger +}) => { + const ports = useWatch({ name: 'ports', control }) const { errors } = useFormState({ control }) - const ports = useWatch({ name: 'ports', control }) // Helps catch errors that span multiple fields // without this, errors won't be cleared on one duplicate // field if the other is edited @@ -476,6 +481,48 @@ export const CommonFields = ({ form, nameTaken, error }: CommonFieldsProps) => { } }, [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 + /> +
+
+ ) +} + +type CommonFieldsProps = { + form: UseFormReturn + nameTaken: (name: string) => boolean + error: ApiError | null +} + +export const CommonFields = ({ form, nameTaken, error }: CommonFieldsProps) => { + const { control, trigger, setValue } = form + 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="" - /> -
-
+ Date: Tue, 12 Aug 2025 16:35:32 +0100 Subject: [PATCH 08/10] Test fixes --- app/components/form/fields/ComboboxField.tsx | 6 +- app/components/form/fields/ErrorMessage.tsx | 11 +- app/forms/firewall-rules-common.tsx | 3 + app/ui/lib/Combobox.tsx | 10 +- app/ui/lib/InputMiniTable.tsx | 109 ++-- app/ui/lib/TextInput.tsx | 2 +- app/ui/lib/Tooltip.tsx | 1 + test/e2e/firewall-rules.e2e.ts | 642 ++++++++++--------- 8 files changed, 413 insertions(+), 371 deletions(-) diff --git a/app/components/form/fields/ComboboxField.tsx b/app/components/form/fields/ComboboxField.tsx index eb6e0d736d..752cd61380 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, @@ -91,8 +92,9 @@ export function ComboboxField< inputRef={field.ref} transform={transform} {...props} + popoverError={popoverError ? fieldState.error : undefined} /> - + {!popoverError && } ) } diff --git a/app/components/form/fields/ErrorMessage.tsx b/app/components/form/fields/ErrorMessage.tsx index 9d697aea1a..bffe02b84d 100644 --- a/app/components/form/fields/ErrorMessage.tsx +++ b/app/components/form/fields/ErrorMessage.tsx @@ -40,14 +40,17 @@ export function PopoverErrorMessage({ return ( -
- -
+ ) } diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index fd35fff447..4adfc6bb94 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -162,6 +162,7 @@ const TargetsValueField = ({ validateName(value, `${capitalize(type)} name`, false) } placeholder={valuePlaceholders[type]} + popoverError /> ) : ( ) : ( @@ -417,6 +419,7 @@ const HostFilters = ({ allowArbitraryValues hideOptionalTag validate={(value) => validateName(value, `Host filter name`, false)} + popoverError /> ) : ( void /** Necessary if you want RHF to be able to focus it on error */ inputRef?: Ref + popoverError?: FieldError } & ComboboxBaseProps export const Combobox = ({ @@ -101,6 +105,7 @@ export const Combobox = ({ inputRef, transform, matchDropdownWidth = true, + popoverError, ...props }: ComboboxProps) => { const [query, setQuery] = useState(selectedItemValue || '') @@ -180,7 +185,7 @@ export const Combobox = ({ )}
+ {popoverError && ( + + )} +
{/* Grid Container */}
{/* Header Row */} - {headers.map((header, index) => ( -
+
+ {headers.map((header, index) => (
- {header} -
- ))} - {/* Header for remove button column */} -
+ key={index} + className="relative mb-1 flex h-9 items-center border-b pl-5 pr-3 text-left font-semibold text-mono-sm text-secondary bg-secondary border-default first:pl-3" + > +
+ {header} +
+ ))} + {/* Header for remove button column */} +
+
{/* Body Rows */} {hasItems ? ( - fields - .map((field, rowIndex) => { - const rowCells = renderRow(field, rowIndex, name) - return rowCells - .map((input, cellIndex) => ( + fields.map((field, rowIndex) => { + const rowCells = renderRow(field, rowIndex, name) + return ( +
+ {rowCells.map((input, cellIndex) => (
@@ -112,31 +129,29 @@ export function InputMiniTable<
- )) - .concat([ - // Remove button cell -
-
- -
-
, - ]) - }) - .flat() + ))} + {/* Remove button cell */} +
+
+ +
+
+
+ ) + }) ) : emptyState ? (
)} {copyable && ( diff --git a/app/ui/lib/Tooltip.tsx b/app/ui/lib/Tooltip.tsx index 2b1aeba633..b15a8eb0dd 100644 --- a/app/ui/lib/Tooltip.tsx +++ b/app/ui/lib/Tooltip.tsx @@ -125,6 +125,7 @@ export const Tooltip = ({ )} {...getFloatingProps()} style={floatingStyles} + role="tooltip" > {content} { 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() From f5bd39b7b88eb754294bdc895f0faf7e73875c2c Mon Sep 17 00:00:00 2001 From: Benjamin Leonard Date: Wed, 13 Aug 2025 10:59:10 +0100 Subject: [PATCH 09/10] Test fix --- app/components/form/fields/ComboboxField.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/app/components/form/fields/ComboboxField.tsx b/app/components/form/fields/ComboboxField.tsx index 752cd61380..a3d17299ff 100644 --- a/app/components/form/fields/ComboboxField.tsx +++ b/app/components/form/fields/ComboboxField.tsx @@ -76,6 +76,7 @@ export function ComboboxField< return (
Date: Wed, 13 Aug 2025 11:01:12 +0100 Subject: [PATCH 10/10] Update SiloImagesPage.tsx --- app/pages/SiloImagesPage.tsx | 1 + 1 file changed, 1 insertion(+) 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 }) => {