From 54f3ae16fc127c3b3afbf5fb60c85c6803802fa5 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Thu, 27 Jun 2024 22:57:07 -0400 Subject: [PATCH 01/42] Add means to type new entries into combobox --- app/forms/firewall-rules-create.tsx | 47 ++++++++++++++++++++++++++++- app/ui/lib/Combobox.tsx | 16 ++++++++-- 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index 2ec6340e5f..71b5e8f73d 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -25,6 +25,7 @@ import { } from '@oxide/api' import { CheckboxField } from '~/components/form/fields/CheckboxField' +import { ComboboxField } from '~/components/form/fields/ComboboxField' import { DescriptionField } from '~/components/form/fields/DescriptionField' import { ListboxField } from '~/components/form/fields/ListboxField' import { NameField } from '~/components/form/fields/NameField' @@ -210,6 +211,45 @@ export const CommonFields = ({ error, control }: CommonFieldsProps) => { targetForm.reset() }) + // todo: replace this dummy data with actual vpc, etc. data + const items = { + vpc: [ + { value: 'vpc-1', label: 'vpc-one' }, + { value: 'vpc-2', label: 'vpc-two' }, + { value: 'vpc-3', label: 'vpc-three' }, + { value: 'vpc-4', label: 'vpc-four' }, + { value: 'vpc-5', label: 'vpc-five' }, + ], + subnet: [ + { value: 'subnet-1', label: 'subnet-one' }, + { value: 'subnet-2', label: 'subnet-two' }, + { value: 'subnet-3', label: 'subnet-three' }, + { value: 'subnet-4', label: 'subnet-four' }, + { value: 'subnet-5', label: 'subnet-five' }, + ], + instance: [ + { value: 'instance-1', label: 'instance-one' }, + { value: 'instance-2', label: 'instance-two' }, + { value: 'instance-3', label: 'instance-three' }, + { value: 'instance-4', label: 'instance-four' }, + { value: 'instance-5', label: 'instance-five' }, + ], + ip: [ + { value: 'ip-1', label: 'ip-one' }, + { value: 'ip-2', label: 'ip-two' }, + { value: 'ip-3', label: 'ip-three' }, + { value: 'ip-4', label: 'ip-four' }, + { value: 'ip-5', label: 'ip-five' }, + ], + ip_net: [ + { value: 'ip_net-1', label: 'ip_net-one' }, + { value: 'ip_net-2', label: 'ip_net-two' }, + { value: 'ip_net-3', label: 'ip_net-three' }, + { value: 'ip_net-4', label: 'ip_net-four' }, + { value: 'ip_net-5', label: 'ip_net-five' }, + ], + } + return ( <> @@ -482,7 +522,7 @@ export const CommonFields = ({ error, control }: CommonFieldsProps) => { So we should probably have the label on this field change when the host type changes. Also need to confirm that it's just an IP and not a block. */} - { submitHost(e) } }} + onInputChange={(value) => { + hostForm.setValue('value', value) + }} + items={items[hostForm.watch('type')]} + showNoMatchPlaceholder={false} // TODO: validate here, but it's complicated because it's conditional // on which type is selected /> diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index 623a010fd7..65ce2fb323 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -36,6 +36,11 @@ export type ComboboxBaseProps = { placeholder?: string required?: boolean tooltipText?: string + onInputChange?: (value: string) => void + onKeyDown?: (event: React.KeyboardEvent) => void + // set to false in situations where the user should be able to type in new values + // to decide: should we even give a placeholder / warning? + showNoMatchPlaceholder?: boolean } type ComboboxProps = { @@ -56,6 +61,9 @@ export const Combobox = ({ isDisabled, isLoading, onChange, + onInputChange, + onKeyDown, + showNoMatchPlaceholder = true, }: ComboboxProps) => { const [query, setQuery] = useState(selected || '') @@ -102,7 +110,11 @@ export const Combobox = ({ (selected ? selected : query)} - onChange={(event) => setQuery(event.target.value)} + onChange={(event) => { + setQuery(event.target.value) + onInputChange?.(event.target.value) + }} + onKeyDown={onKeyDown} placeholder={placeholder} disabled={isDisabled || isLoading} className={cn( @@ -123,7 +135,7 @@ export const Combobox = ({ className={`ox-menu pointer-events-auto ${zIndex} relative w-[var(--button-width)] overflow-y-auto border !outline-none border-secondary [--anchor-gap:14px] empty:hidden`} modal={false} > - {filteredItems.length === 0 && ( + {showNoMatchPlaceholder && filteredItems.length === 0 && (
No items match
 From b65263dd7cc25e997573f9118c4caaf2197204ce Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 28 Jun 2024 09:37:39 -0400 Subject: [PATCH 02/42] make items prop optional; without items, combobox acts as regular input --- app/forms/firewall-rules-create.tsx | 8 +-- app/ui/lib/Combobox.tsx | 91 +++++++++++++++-------------- 2 files changed, 49 insertions(+), 50 deletions(-) diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index 71b5e8f73d..cddd5c8c24 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -234,13 +234,7 @@ export const CommonFields = ({ error, control }: CommonFieldsProps) => { { value: 'instance-4', label: 'instance-four' }, { value: 'instance-5', label: 'instance-five' }, ], - ip: [ - { value: 'ip-1', label: 'ip-one' }, - { value: 'ip-2', label: 'ip-two' }, - { value: 'ip-3', label: 'ip-three' }, - { value: 'ip-4', label: 'ip-four' }, - { value: 'ip-5', label: 'ip-five' }, - ], + ip: [], ip_net: [ { value: 'ip_net-1', label: 'ip_net-one' }, { value: 'ip_net-2', label: 'ip_net-two' }, diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index 65ce2fb323..48c89c5e53 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -31,7 +31,8 @@ export type ComboboxBaseProps = { description?: React.ReactNode isDisabled?: boolean isLoading?: boolean - items: ComboboxItem[] + // without items, the combobox acts as a regular text input + items?: ComboboxItem[] label: string placeholder?: string required?: boolean @@ -51,7 +52,7 @@ type ComboboxProps = { export const Combobox = ({ description, - items, + items = [], selected, label, placeholder, @@ -125,48 +126,52 @@ export const Combobox = ({ hasError && 'focus-error' )} /> -
- -
- - - {showNoMatchPlaceholder && filteredItems.length === 0 && ( - -
No items match
- + {items.length > 0 && ( +
+ +
)} - {filteredItems.map((item) => ( - { - onChange(item.label) - setQuery(item.label) - }} - > - {({ 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} -
- )} - - ))} - + + {items.length > 0 && ( + + {showNoMatchPlaceholder && filteredItems.length === 0 && ( + +
No items match
+ + )} + {filteredItems.map((item) => ( + { + onChange(item.label) + setQuery(item.label) + }} + > + {({ 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} +
+ )} + + ))} + + )} ) From a8c92847ec948be73e7c5cffd00c24d7620a5554 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 28 Jun 2024 10:23:09 -0400 Subject: [PATCH 03/42] add data fetching for instances and VPCs --- app/forms/firewall-rules-create.tsx | 81 +++++++++++++++++------------ 1 file changed, 49 insertions(+), 32 deletions(-) diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index cddd5c8c24..42f7b4ff76 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -18,6 +18,8 @@ import { useApiQueryClient, usePrefetchedApiQuery, type ApiError, + type Instance, + type Vpc, type VpcFirewallRule, type VpcFirewallRuleHostFilter, type VpcFirewallRuleTarget, @@ -33,8 +35,15 @@ import { NumberField } from '~/components/form/fields/NumberField' import { RadioField } from '~/components/form/fields/RadioField' import { TextField, TextFieldInner } from '~/components/form/fields/TextField' import { SideModalForm } from '~/components/form/SideModalForm' -import { getVpcSelector, useForm, useVpcSelector } from '~/hooks' +import { + getProjectSelector, + getVpcSelector, + useForm, + useProjectSelector, + useVpcSelector, +} from '~/hooks' import { addToast } from '~/stores/toast' +import { PAGE_SIZE } from '~/table/QueryTable' import { Badge } from '~/ui/lib/Badge' import { Button } from '~/ui/lib/Button' import { FormDivider } from '~/ui/lib/Divider' @@ -124,6 +133,8 @@ const targetDefaultValues: TargetFormValues = { type CommonFieldsProps = { error: ApiError | null control: Control + instances: Array + vpcs: Array } function getFilterValueProps(hostType: VpcFirewallRuleHostFilter['type']) { @@ -175,7 +186,7 @@ const DocsLinkMessage = () => ( /> ) -export const CommonFields = ({ error, control }: CommonFieldsProps) => { +export const CommonFields = ({ error, control, instances, vpcs }: CommonFieldsProps) => { const portRangeForm = useForm({ defaultValues: portRangeDefaultValues }) const ports = useController({ name: 'ports', control }).field const submitPortRange = portRangeForm.handleSubmit(({ portRange }) => { @@ -211,15 +222,10 @@ export const CommonFields = ({ error, control }: CommonFieldsProps) => { targetForm.reset() }) - // todo: replace this dummy data with actual vpc, etc. data - const items = { - vpc: [ - { value: 'vpc-1', label: 'vpc-one' }, - { value: 'vpc-2', label: 'vpc-two' }, - { value: 'vpc-3', label: 'vpc-three' }, - { value: 'vpc-4', label: 'vpc-four' }, - { value: 'vpc-5', label: 'vpc-five' }, - ], + const hostFilterItems = { + vpc: vpcs.map((v) => ({ value: v.name, label: v.name })), + // todo: this should be a list of subnets in the selected VPC, + // so we'll need to have the user first specify a VPC and then load these in subnet: [ { value: 'subnet-1', label: 'subnet-one' }, { value: 'subnet-2', label: 'subnet-two' }, @@ -227,21 +233,9 @@ export const CommonFields = ({ error, control }: CommonFieldsProps) => { { value: 'subnet-4', label: 'subnet-four' }, { value: 'subnet-5', label: 'subnet-five' }, ], - instance: [ - { value: 'instance-1', label: 'instance-one' }, - { value: 'instance-2', label: 'instance-two' }, - { value: 'instance-3', label: 'instance-three' }, - { value: 'instance-4', label: 'instance-four' }, - { value: 'instance-5', label: 'instance-five' }, - ], + instance: instances.map((i) => ({ value: i.name, label: i.name })), ip: [], - ip_net: [ - { value: 'ip_net-1', label: 'ip_net-one' }, - { value: 'ip_net-2', label: 'ip_net-two' }, - { value: 'ip_net-3', label: 'ip_net-three' }, - { value: 'ip_net-4', label: 'ip_net-four' }, - { value: 'ip_net-5', label: 'ip_net-five' }, - ], + ip_net: [], } return ( @@ -530,7 +524,7 @@ export const CommonFields = ({ error, control }: CommonFieldsProps) => { onInputChange={(value) => { hostForm.setValue('value', value) }} - items={items[hostForm.watch('type')]} + items={hostFilterItems[hostForm.watch('type')]} showNoMatchPlaceholder={false} // TODO: validate here, but it's complicated because it's conditional // on which type is selected @@ -599,13 +593,22 @@ export const CommonFields = ({ error, control }: CommonFieldsProps) => { } CreateFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { - await apiQueryClient.prefetchQuery('vpcFirewallRulesView', { - query: getVpcSelector(params), - }) + const { project } = getProjectSelector(params) + await Promise.all([ + apiQueryClient.prefetchQuery('vpcFirewallRulesView', { + query: getVpcSelector(params), + }), + apiQueryClient.prefetchQuery('instanceList', { + query: { project, limit: PAGE_SIZE }, + }), + apiQueryClient.prefetchQuery('vpcList', { query: { project, limit: PAGE_SIZE } }), + ]) + return null } export function CreateFirewallRuleForm() { + const { project } = useProjectSelector() const vpcSelector = useVpcSelector() const queryClient = useApiQueryClient() @@ -620,10 +623,19 @@ export function CreateFirewallRuleForm() { }, }) - const { data } = usePrefetchedApiQuery('vpcFirewallRulesView', { + const { data: vpcFirewallRules } = usePrefetchedApiQuery('vpcFirewallRulesView', { query: vpcSelector, }) - const existingRules = useMemo(() => R.sortBy(data.rules, (r) => r.priority), [data]) + const existingRules = useMemo( + () => R.sortBy(vpcFirewallRules.rules, (r) => r.priority), + [vpcFirewallRules] + ) + const { data: instances } = usePrefetchedApiQuery('instanceList', { + query: { project, limit: PAGE_SIZE }, + }) + const { data: vpcs } = usePrefetchedApiQuery('vpcList', { + query: { project, limit: PAGE_SIZE }, + }) const form = useForm({ defaultValues }) @@ -651,7 +663,12 @@ export function CreateFirewallRuleForm() { submitError={updateRules.error} submitLabel="Add rule" > - + ) } From 35b46bf92b0ae4643b45c821e030a088af2a5735 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 28 Jun 2024 17:23:44 -0400 Subject: [PATCH 04/42] Update with working combobox and dynamic data loading; still a few rough spots --- app/forms/firewall-rules-create.tsx | 73 +++++++++++++++++++---------- app/forms/firewall-rules-edit.tsx | 33 ++++++++++--- app/ui/lib/Combobox.tsx | 13 ++++- 3 files changed, 85 insertions(+), 34 deletions(-) diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index 42f7b4ff76..f038dc9452 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -15,6 +15,7 @@ import { firewallRuleGetToPut, parsePortRange, useApiMutation, + useApiQuery, useApiQueryClient, usePrefetchedApiQuery, type ApiError, @@ -46,6 +47,7 @@ import { addToast } from '~/stores/toast' import { PAGE_SIZE } from '~/table/QueryTable' import { Badge } from '~/ui/lib/Badge' import { Button } from '~/ui/lib/Button' +import { toComboboxItem } from '~/ui/lib/Combobox' import { FormDivider } from '~/ui/lib/Divider' import { Message } from '~/ui/lib/Message' import * as MiniTable from '~/ui/lib/MiniTable' @@ -113,6 +115,7 @@ const portRangeDefaultValues: PortRangeFormValues = { type HostFormValues = { type: VpcFirewallRuleHostFilter['type'] value: string + subnetVpc?: string } const hostDefaultValues: HostFormValues = { @@ -133,6 +136,7 @@ const targetDefaultValues: TargetFormValues = { type CommonFieldsProps = { error: ApiError | null control: Control + project: string instances: Array vpcs: Array } @@ -186,7 +190,13 @@ const DocsLinkMessage = () => ( /> ) -export const CommonFields = ({ error, control, instances, vpcs }: CommonFieldsProps) => { +export const CommonFields = ({ + error, + control, + project, + instances, + vpcs, +}: CommonFieldsProps) => { const portRangeForm = useForm({ defaultValues: portRangeDefaultValues }) const ports = useController({ name: 'ports', control }).field const submitPortRange = portRangeForm.handleSubmit(({ portRange }) => { @@ -222,22 +232,25 @@ export const CommonFields = ({ error, control, instances, vpcs }: CommonFieldsPr targetForm.reset() }) + const queryClient = useApiQueryClient() + const hostType = hostForm.watch('type') + const hostSubnetVpc = hostForm.watch('subnetVpc') + + const { data: vpcSubnets } = useApiQuery( + 'vpcSubnetList', + { query: { project, vpc: hostSubnetVpc } }, + { enabled: !!hostSubnetVpc } + ) + const hostFilterItems = { vpc: vpcs.map((v) => ({ value: v.name, label: v.name })), - // todo: this should be a list of subnets in the selected VPC, - // so we'll need to have the user first specify a VPC and then load these in - subnet: [ - { value: 'subnet-1', label: 'subnet-one' }, - { value: 'subnet-2', label: 'subnet-two' }, - { value: 'subnet-3', label: 'subnet-three' }, - { value: 'subnet-4', label: 'subnet-four' }, - { value: 'subnet-5', label: 'subnet-five' }, - ], - instance: instances.map((i) => ({ value: i.name, label: i.name })), + subnet: vpcSubnets?.items.map((s) => toComboboxItem(s.name)) || [], + instance: instances.map((i) => toComboboxItem(i.name)), ip: [], ip_net: [], } + const isHostFilterInputDisabled = hostType === 'subnet' && !hostSubnetVpc return ( <> @@ -280,9 +293,7 @@ export const CommonFields = ({ error, control, instances, vpcs }: CommonFieldsPr required control={control} /> - - {/* Really this should be its own
, but you can't have a form inside a form, so we just stick the submit handler in a button onClick */}

Targets

@@ -312,7 +323,6 @@ export const CommonFields = ({ error, control, instances, vpcs }: CommonFieldsPr required control={targetForm.control} /> -
- {!!targets.value.length && ( @@ -380,11 +389,8 @@ export const CommonFields = ({ error, control, instances, vpcs }: CommonFieldsPr )} - -

Filters

- } /> -
{/* We have to blow this up instead of using TextField to get better text styling on the label */} @@ -439,7 +444,6 @@ export const CommonFields = ({ error, control, instances, vpcs }: CommonFieldsPr
- {!!ports.value.length && ( @@ -460,7 +464,6 @@ export const CommonFields = ({ error, control, instances, vpcs }: CommonFieldsPr )} -
Protocol filters
@@ -479,7 +482,6 @@ export const CommonFields = ({ error, control, instances, vpcs }: CommonFieldsPr
-

Host filters

+ {/* + If the user is trying to specify a subnet, they must + first select the VPC that owns the subnet + */} + {hostType === 'subnet' && ( + toComboboxItem(v.name))} + // when this changes, we need to re-fetch the subnet list + onChange={() => { + queryClient.invalidateQueries('vpcSubnetList') + }} + /> + )} + {/* For everything but IP this is a name, but for IP it's an IP. So we should probably have the label on this field change when the host type changes. Also need to confirm that it's just an IP and not a block. */} { @@ -524,7 +545,7 @@ export const CommonFields = ({ error, control, instances, vpcs }: CommonFieldsPr onInputChange={(value) => { hostForm.setValue('value', value) }} - items={hostFilterItems[hostForm.watch('type')]} + items={hostFilterItems[hostType]} showNoMatchPlaceholder={false} // TODO: validate here, but it's complicated because it's conditional // on which type is selected @@ -535,7 +556,7 @@ export const CommonFields = ({ error, control, instances, vpcs }: CommonFieldsPr variant="ghost" size="sm" className="mr-2.5" - disabled={!hostForm.formState.isDirty} + disabled={!hostForm.watch('value').length} onClick={() => hostForm.reset()} > Clear @@ -581,7 +602,6 @@ export const CommonFields = ({ error, control, instances, vpcs }: CommonFieldsPr )}
- {error && ( <> @@ -666,6 +686,7 @@ export function CreateFirewallRuleForm() { diff --git a/app/forms/firewall-rules-edit.tsx b/app/forms/firewall-rules-edit.tsx index 2c1086d641..5b53fbcaa3 100644 --- a/app/forms/firewall-rules-edit.tsx +++ b/app/forms/firewall-rules-edit.tsx @@ -23,6 +23,7 @@ import { useForm, useVpcSelector, } from '~/hooks' +import { PAGE_SIZE } from '~/table/QueryTable' import { invariant } from '~/util/invariant' import { pb } from '~/util/path-builder' @@ -35,11 +36,17 @@ import { EditFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { const { project, vpc, rule } = getFirewallRuleSelector(params) - const data = await apiQueryClient.fetchQuery('vpcFirewallRulesView', { + const firewallRules = await apiQueryClient.fetchQuery('vpcFirewallRulesView', { query: { project, vpc }, }) - - const originalRule = data.rules.find((r) => r.name === rule) + await Promise.all([ + apiQueryClient.prefetchQuery('instanceList', { + query: { project, limit: PAGE_SIZE }, + }), + apiQueryClient.prefetchQuery('vpcList', { query: { project, limit: PAGE_SIZE } }), + ]) + + const originalRule = firewallRules.rules.find((r) => r.name === rule) if (!originalRule) throw trigger404 return null @@ -50,11 +57,17 @@ export function EditFirewallRuleForm() { const vpcSelector = useVpcSelector() const queryClient = useApiQueryClient() - const { data } = usePrefetchedApiQuery('vpcFirewallRulesView', { + const { data: firewallRules } = usePrefetchedApiQuery('vpcFirewallRulesView', { query: { project, vpc }, }) + const { data: instances } = usePrefetchedApiQuery('instanceList', { + query: { project, limit: PAGE_SIZE }, + }) + const { data: vpcs } = usePrefetchedApiQuery('vpcList', { + query: { project, limit: PAGE_SIZE }, + }) - const originalRule = data.rules.find((r) => r.name === rule) + const originalRule = firewallRules.rules.find((r) => r.name === rule) // we shouldn't hit this because of the trigger404 in the loader invariant(originalRule, 'Firewall rule must exist') @@ -99,7 +112,7 @@ export function EditFirewallRuleForm() { onSubmit={(values) => { // note different filter logic from create: filter out the rule with the // *original* name because we need to overwrite that rule - const otherRules = data.rules + const otherRules = firewallRules.rules .filter((r) => r.name !== originalRule.name) .map(firewallRuleGetToPut) @@ -115,7 +128,13 @@ export function EditFirewallRuleForm() { loading={updateRules.isPending} submitError={updateRules.error} > - + ) } diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index 48c89c5e53..b354a90ef4 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -20,12 +20,18 @@ import { useState } from 'react' import { SelectArrows6Icon } from '@oxide/design-system/icons/react' +import { KEYS } from '../util/keys' import { FieldLabel } from './FieldLabel' import { usePopoverZIndex } from './SideModal' import { TextInputHint } from './TextInput' export type ComboboxItem = { label: string; value: string } +export const toComboboxItem = (value: string, label?: string): ComboboxItem => ({ + label: label || value, + value, +}) + /** Simple non-generic props shared with ComboboxField */ export type ComboboxBaseProps = { description?: React.ReactNode @@ -115,7 +121,12 @@ export const Combobox = ({ setQuery(event.target.value) onInputChange?.(event.target.value) }} - onKeyDown={onKeyDown} + onKeyDown={(e) => { + onKeyDown && onKeyDown(e) + if (e.key === KEYS.enter) { + setQuery('') + } + }} placeholder={placeholder} disabled={isDisabled || isLoading} className={cn( From fcd2d739a5a4318e87beed041c4f2b37c725d052 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 28 Jun 2024 17:32:45 -0400 Subject: [PATCH 05/42] items should be required --- app/ui/lib/Combobox.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index b354a90ef4..3c79aab369 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -37,8 +37,7 @@ export type ComboboxBaseProps = { description?: React.ReactNode isDisabled?: boolean isLoading?: boolean - // without items, the combobox acts as a regular text input - items?: ComboboxItem[] + items: ComboboxItem[] label: string placeholder?: string required?: boolean From 7a579c0f94ed07e234720205f722dbce56ecafd5 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Mon, 1 Jul 2024 10:20:23 -0400 Subject: [PATCH 06/42] Tweak onKeyDown, as enter key shouldn't get overridden with comboboxes the way it is for normal inputs --- app/forms/firewall-rules-create.tsx | 6 ------ app/ui/lib/Combobox.tsx | 9 --------- 2 files changed, 15 deletions(-) diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index f038dc9452..6be817dd5b 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -536,12 +536,6 @@ export const CommonFields = ({ {...getFilterValueProps(hostType)} required control={hostForm.control} - onKeyDown={(e) => { - if (e.key === KEYS.enter) { - e.preventDefault() // prevent full form submission - submitHost(e) - } - }} onInputChange={(value) => { hostForm.setValue('value', value) }} diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index 3c79aab369..15ff140956 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -20,7 +20,6 @@ import { useState } from 'react' import { SelectArrows6Icon } from '@oxide/design-system/icons/react' -import { KEYS } from '../util/keys' import { FieldLabel } from './FieldLabel' import { usePopoverZIndex } from './SideModal' import { TextInputHint } from './TextInput' @@ -43,7 +42,6 @@ export type ComboboxBaseProps = { required?: boolean tooltipText?: string onInputChange?: (value: string) => void - onKeyDown?: (event: React.KeyboardEvent) => void // set to false in situations where the user should be able to type in new values // to decide: should we even give a placeholder / warning? showNoMatchPlaceholder?: boolean @@ -68,7 +66,6 @@ export const Combobox = ({ isLoading, onChange, onInputChange, - onKeyDown, showNoMatchPlaceholder = true, }: ComboboxProps) => { const [query, setQuery] = useState(selected || '') @@ -120,12 +117,6 @@ export const Combobox = ({ setQuery(event.target.value) onInputChange?.(event.target.value) }} - onKeyDown={(e) => { - onKeyDown && onKeyDown(e) - if (e.key === KEYS.enter) { - setQuery('') - } - }} placeholder={placeholder} disabled={isDisabled || isLoading} className={cn( From a5d0ecd2b0aff6f6c8395b21394d9c854545a585 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Mon, 1 Jul 2024 11:01:00 -0400 Subject: [PATCH 07/42] test refactor --- test/e2e/firewall-rules.e2e.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index 0467c81ef6..c40daa6a05 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -45,10 +45,10 @@ test('can create firewall rule', async ({ page }) => { await expectRowVisible(targets, { Type: 'ip', Value: '192.168.0.1' }) // add host filter instance "host-filter-instance" - await page.locator('role=button[name*="Host type"]').click() - await page.locator('role=option[name="Instance"]').click() - await page.fill('role=textbox[name="Instance name"]', 'host-filter-instance') - await page.locator('text="Add host filter"').click() + await page.getByRole('button', { name: 'Host type' }).click() + await page.getByRole('option', { name: 'Instance' }).click() + await page.getByRole('combobox', { name: 'Instance name' }).fill('host-filter-instance') + await page.getByRole('button', { name: 'Add host filter' }).click() // host is added to hosts table const hosts = page.getByRole('table', { name: 'Host filters' }) From cec67a657f3c4f69a62475d03c122826b39527a5 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Mon, 15 Jul 2024 15:44:12 -0400 Subject: [PATCH 08/42] Make form clearable when field is empty --- app/forms/firewall-rules-create.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index 6be817dd5b..c773071ae5 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -550,7 +550,6 @@ export const CommonFields = ({ variant="ghost" size="sm" className="mr-2.5" - disabled={!hostForm.watch('value').length} onClick={() => hostForm.reset()} > Clear From c6517915d441f206ed6dcf6196ad35b1dc01e29a Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 16 Jul 2024 10:57:06 -0400 Subject: [PATCH 09/42] test updates --- test/e2e/firewall-rules.e2e.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index c40daa6a05..4f6e22e99a 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -199,23 +199,25 @@ test('firewall rule form hosts table', async ({ page }) => { // add hosts with overlapping names and types to test delete // there are two of these because the targets table also defaults to VPC - await page.getByRole('textbox', { name: 'VPC name' }).nth(1).fill('abc') + await page.getByRole('combobox', { name: 'VPC' }).fill('abc') await addButton.click() await expectRowVisible(hosts, { Type: 'vpc', Value: 'abc' }) - await page.getByRole('textbox', { name: 'VPC name' }).nth(1).fill('def') + await page.getByRole('combobox', { name: 'VPC' }).fill('def') await addButton.click() await expectRowVisible(hosts, { Type: 'vpc', Value: 'def' }) await page.getByRole('button', { name: 'Host type' }).click() await page.getByRole('option', { name: 'VPC Subnet' }).click() - await page.getByRole('textbox', { name: 'Subnet name' }).fill('abc') + await page.getByRole('button', { name: 'VPC' }).nth(2).click() + await page.getByRole('option', { name: 'mock-vpc' }).click() + await page.getByRole('combobox', { name: 'Subnet name' }).fill('abc') await addButton.click() await expectRowVisible(hosts, { Type: 'subnet', Value: 'abc' }) await page.getByRole('button', { name: 'Host type' }).click() await page.getByRole('option', { name: 'IP', exact: true }).click() - await page.getByRole('textbox', { name: 'IP address' }).fill('192.168.0.1') + await page.getByRole('combobox', { name: 'IP address' }).fill('192.168.0.1') await addButton.click() await expectRowVisible(hosts, { Type: 'ip', Value: '192.168.0.1' }) From 7e0174eacc96d085c7ab5e976d4d19c798fa325c Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Thu, 15 Aug 2024 19:00:07 -0700 Subject: [PATCH 10/42] Add more controls on Target section of firewall rules create form --- app/forms/firewall-rules-create.tsx | 125 +++++++++++++++++++++------- 1 file changed, 94 insertions(+), 31 deletions(-) diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index 76bb4507f5..7fc4609a65 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -107,6 +107,7 @@ const hostDefaultValues: HostFormValues = { type TargetFormValues = { type: VpcFirewallRuleTarget['type'] value: string + subnetVpc?: string } const targetDefaultValues: TargetFormValues = { @@ -123,6 +124,14 @@ type CommonFieldsProps = { nameTaken: (name: string) => boolean } +const targetAndHostItems = [ + { value: 'vpc', label: 'VPC' }, + { value: 'subnet', label: 'VPC Subnet' }, + { value: 'instance', label: 'Instance' }, + { value: 'ip', label: 'IP' }, + { value: 'ip_net', label: 'IP subnet' }, +] + function getFilterValueProps(hostType: VpcFirewallRuleHostFilter['type']) { switch (hostType) { case 'vpc': @@ -204,6 +213,28 @@ export const CommonFields = ({ const targetForm = useForm({ defaultValues: targetDefaultValues }) const targets = useController({ name: 'targets', control }).field + const targetType = targetForm.watch('type') + const targetSubnetVpc = targetForm.watch('subnetVpc') + + const { data: targetVpcSubnets } = useApiQuery( + 'vpcSubnetList', + { query: { project, vpc: targetSubnetVpc } }, + { enabled: !!targetSubnetVpc } + ) + const targetFilterItems = { + vpc: vpcs.map((v) => ({ value: v.name, label: v.name })), + subnet: + targetVpcSubnets?.items + // filter out subnets that are already targets + .filter((i) => !targets.value.map((t) => t.value).includes(i.name)) + .map((s) => toComboboxItem(s.name)) || [], + instance: instances.map((i) => toComboboxItem(i.name)), + ip: [], + ip_net: [], + } + + const isTargetFilterInputDisabled = targetType === 'subnet' && !targetSubnetVpc + const submitTarget = targetForm.handleSubmit(({ type, value }) => { // TODO: do this with a normal validation // ignore click if empty or a duplicate @@ -219,15 +250,18 @@ export const CommonFields = ({ const hostType = hostForm.watch('type') const hostSubnetVpc = hostForm.watch('subnetVpc') - const { data: vpcSubnets } = useApiQuery( + const { data: hostVpcSubnets } = useApiQuery( 'vpcSubnetList', { query: { project, vpc: hostSubnetVpc } }, { enabled: !!hostSubnetVpc } ) - const hostFilterItems = { vpc: vpcs.map((v) => ({ value: v.name, label: v.name })), - subnet: vpcSubnets?.items.map((s) => toComboboxItem(s.name)) || [], + subnet: + hostVpcSubnets?.items + // filter out subnets that are already targets + .filter((i) => !hosts.value.map((h) => h.value).includes(i.name)) + .map((s) => toComboboxItem(s.name)) || [], instance: instances.map((i) => toComboboxItem(i.name)), ip: [], ip_net: [], @@ -305,31 +339,66 @@ export const CommonFields = ({ { + targetForm.setValue('value', '') // clear the value when the type changes + }} />
- { - if (e.key === KEYS.enter) { - e.preventDefault() // prevent full form submission - submitTarget(e) - } - }} - // TODO: validate here, but it's complicated because it's conditional - // on which type is selected - /> + {/* + If the user is trying to specify a subnet, they must + first select the VPC that owns the subnet + */} + {targetType === 'subnet' && ( + toComboboxItem(v.name))} + // when this changes, we need to re-fetch the subnet list + onChange={() => { + queryClient.invalidateQueries('vpcSubnetList') + }} + /> + )} + {/* For everything but IP this is a name, but for IP it's an IP. + So we should probably have the label on this field change when the + host type changes. Also need to confirm that it's just an IP and + not a block. */} + {targetType === 'subnet' ? ( + { + targetForm.setValue('value', value) + }} + items={targetFilterItems[targetType]} + showNoMatchPlaceholder={false} + // TODO: validate here, but it's complicated because it's conditional + // on which type is selected + /> + ) : ( + { + if (e.key === KEYS.enter) { + e.preventDefault() // prevent full form submission + submitTarget(e) + } + }} + // TODO: validate here, but it's complicated because it's conditional + // on which type is selected + /> + )}
+ +
+) + export const CommonFields = ({ error, control, @@ -217,6 +244,128 @@ export const CommonFields = ({ // In the firewall rules form, these types get comboboxes instead of text fields const comboboxTypes = ['vpc', 'subnet', 'instance'] + // The DynamicType and DynamicValue fields allow the user to select the type of + // filter (e.g. VPC, subnet, instance) and then input the value of that filter. + // TODO: make ListboxField smarter with the values like RadioField is + const DynamicTypeField = ({ + label, + control, + onChange, + }: { + label: string + control: Control + onChange: () => void + }) => ( + + ) + + // If the type is 'subnet', the user must first select the VPC that owns the subnet + const SubnetVpcField = ({ + control, + }: { + control: Control + }) => { + return ( + toComboboxItem(v.name))} + // when this changes, we need to re-fetch the subnet list + onChange={() => { + queryClient.invalidateQueries('vpcSubnetList') + }} + /> + ) + } + + const DynamicValueField = ({ + sectionType, + control, + items, + onInputChange, + isDisabled, + }: { + sectionType: VpcFirewallRuleHostFilter['type'] + control: Control + items: Array<{ value: string; label: string }> + onInputChange?: (value: string) => void + isDisabled?: boolean + }) => + comboboxTypes.includes(sectionType) ? ( + + ) : ( + { + if (e.key === KEYS.enter) { + e.preventDefault() // prevent full form submission + submitTarget(e) + } + }} + // TODO: validate here, but it's complicated because it's conditional + // on which type is selected + /> + ) + + const TypeAndValueTableHeader = () => ( + + Type + Value + {/* For remove button */} + + + ) + + const TypeAndValueTableRow = ({ + type, + value, + index, + onRemove, + targetOrHost, + }: { + type: string + value: string + index: number + onRemove: () => void + targetOrHost: 'target' | 'host' + }) => ( + + + {type} + + {value} + + + ) + return ( <> @@ -273,132 +422,61 @@ export const CommonFields = ({ {/* Really this should be its own , but you can't have a form inside a form, so we just stick the submit handler in a button onClick */} -

Targets

- - Targets determine the instances to which this rule applies. You can target - instances directly by name, 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. - - } - /> - {/* TODO: make ListboxField smarter with the values like RadioField is */} - { - targetForm.setValue('value', '') // clear the value when the type changes - }} - /> -
- {/* - If the user is trying to specify a subnet, they must - first select the VPC that owns the subnet - */} - {targetType === 'subnet' && ( - toComboboxItem(v.name))} - // when this changes, we need to re-fetch the subnet list - onChange={() => { - queryClient.invalidateQueries('vpcSubnetList') - }} - /> - )} - {/* For everything but IP this is a name, but for IP it's an IP. - So we should probably have the label on this field change when the - host type changes. Also need to confirm that it's just an IP and - not a block. */} - {comboboxTypes.includes(targetType) ? ( - { - targetForm.setValue('value', value) - }} - items={targetFilterItems[targetType]} - showNoMatchPlaceholder={false} - // TODO: validate here, but it's complicated because it's conditional - // on which type is selected - /> - ) : ( - { - if (e.key === KEYS.enter) { - e.preventDefault() // prevent full form submission - submitTarget(e) - } - }} - // TODO: validate here, but it's complicated because it's conditional - // on which type is selected - /> - )} - -
- - -
+

Targets

+ + Targets determine the instances to which this rule applies. You can target + instances directly by name, 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. + + } + /> + { + targetForm.setValue('value', '') // clear the value when the type changes + }} + /> + {/* If specifying a subnet, they must first select the VPC that owns the subnet */} + {targetType === 'subnet' && } + targetForm.setValue('value', value)} + isDisabled={isTargetFilterInputDisabled} + /> + targetForm.reset()} + onSubmit={submitTarget} + buttonCopy="Add target filter" + />
{!!targets.value.length && ( - - Type - Value - {/* For remove button */} - - + {targets.value.map((t, index) => ( - - - {t.type} - - {t.value} - - targets.onChange( - targets.value.filter( - (i) => !(i.value === t.value && i.type === t.type) - ) - ) - } - label={`remove target ${t.value}`} - /> - + type={t.type} + value={t.value} + index={index} + onRemove={() => + targets.onChange( + targets.value.filter((i) => !(i.value === t.value && i.type === t.type)) + ) + } + targetOrHost="target" + /> ))} @@ -447,20 +525,12 @@ export const CommonFields = ({ }} />
-
- - -
+ {!!ports.value.length && ( @@ -515,132 +585,44 @@ export const CommonFields = ({ } /> - hostForm.setValue('value', '')} // clear the value when the type changes /> - - {/* - If the user is trying to specify a subnet, they must - first select the VPC that owns the subnet - */} - {hostType === 'subnet' && ( - toComboboxItem(v.name))} - // when this changes, we need to re-fetch the subnet list - onChange={() => { - queryClient.invalidateQueries('vpcSubnetList') - }} - /> - )} - - {/* For everything but IP this is a name, but for IP it's an IP. - So we should probably have the label on this field change when the - host type changes. Also need to confirm that it's just an IP and - not a block. */} - - {comboboxTypes.includes(hostType) ? ( - { - hostForm.setValue('value', value) - }} - items={hostFilterItems[hostType]} - showNoMatchPlaceholder={false} - // TODO: validate here, but it's complicated because it's conditional - // on which type is selected - /> - ) : ( - { - if (e.key === KEYS.enter) { - e.preventDefault() // prevent full form submission - submitTarget(e) - } - }} - // TODO: validate here, but it's complicated because it's conditional - // on which type is selected - /> - )} - - {/* } + { - hostForm.setValue('value', value) - }} - isLoading={hostVpcSubnetsIsLoading} items={hostFilterItems[hostType]} - showNoMatchPlaceholder={false} - // TODO: validate here, but it's complicated because it's conditional - // on which type is selected - /> */} - -
- - -
+ onInputChange={(value) => hostForm.setValue('value', value)} + isDisabled={isHostFilterInputDisabled} + /> + hostForm.reset()} + onSubmit={submitHost} + buttonCopy="Add host filter" + /> {!!hosts.value.length && ( - - Type - Value - {/* For remove button */} - - + {hosts.value.map((h, index) => ( - - - {h.type} - - {h.value} - - hosts.onChange( - hosts.value.filter( - (i) => !(i.value === h.value && i.type === h.type) - ) - ) - } - label={`remove host ${h.value}`} - /> - + type={h.type} + value={h.value} + index={index} + onRemove={() => + hosts.onChange( + hosts.value.filter((i) => !(i.value === h.value && i.type === h.type)) + ) + } + targetOrHost="host" + /> ))} From dd2ccb0b1da8a81ed34a9c399744d924c2536a65 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Thu, 22 Aug 2024 13:47:46 -0700 Subject: [PATCH 14/42] e2e utils --- .../form/fields/dropdownItemsList.ts | 5 +- app/forms/firewall-rules-common.tsx | 104 +++++++++++++++--- test/e2e/utils.ts | 16 +++ 3 files changed, 106 insertions(+), 19 deletions(-) diff --git a/app/components/form/fields/dropdownItemsList.ts b/app/components/form/fields/dropdownItemsList.ts index e9e2633084..baa1bc28f0 100644 --- a/app/components/form/fields/dropdownItemsList.ts +++ b/app/components/form/fields/dropdownItemsList.ts @@ -27,10 +27,11 @@ export const useCustomRouterItems = () => { // This is a custom hook that returns a list of subnets for a given VPC // If no VPC is provided, it will use the VPC from the VPC selector -export const useVpcSubnetItems = ({ project, vpc }: { project: string; vpc?: string }) => { +export const useVpcSubnetItems = ({ project, vpc }: { project?: string; vpc?: string }) => { + const vpcSelector = useVpcSelector() const vpcSubnets = useApiQuery( 'vpcSubnetList', - { query: { project, vpc } }, + { query: { project: project || vpcSelector.project, vpc: vpc || vpcSelector.vpc } }, { enabled: !!vpc } ) const vpcSubnetItems = useMemo(() => { diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 688ea2a059..6d8a7362e9 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -36,6 +36,7 @@ import * as MiniTable from '~/ui/lib/MiniTable' import { TextInputHint } from '~/ui/lib/TextInput' import { KEYS } from '~/ui/util/keys' import { links } from '~/util/links' +import { capitalize } from '~/util/str' import { type FirewallRuleValues } from './firewall-rules-util' @@ -86,20 +87,28 @@ const targetAndHostItems = [ { value: 'ip_net', label: 'IP subnet' }, ] -function getFilterValueProps(hostType: VpcFirewallRuleHostFilter['type']) { +const getFilterValueProps = ( + hostType: VpcFirewallRuleHostFilter['type'], + sectionType: 'target' | 'host' +) => { switch (hostType) { case 'vpc': - return { label: 'VPC name' } + return { label: 'VPC name', ariaLabel: `Select ${sectionType} VPC name` } case 'subnet': - return { label: 'Subnet name' } + return { label: 'Subnet name', ariaLabel: `Select ${sectionType} subnet name` } case 'instance': - return { label: 'Instance name' } + return { label: 'Instance name', ariaLabel: `Select ${sectionType} instance name` } case 'ip': - return { label: 'IP address', helpText: 'An IPv4 or IPv6 address' } + return { + label: 'IP address', + helpText: 'An IPv4 or IPv6 address', + ariaLabel: `Enter ${sectionType} IP address`, + } case 'ip_net': return { label: 'IP network', helpText: 'Looks like 192.168.0.0/16 or fd00:1122:3344:0001::1/64', + ariaLabel: `Enter ${sectionType} IP network`, } } } @@ -248,17 +257,17 @@ export const CommonFields = ({ // filter (e.g. VPC, subnet, instance) and then input the value of that filter. // TODO: make ListboxField smarter with the values like RadioField is const DynamicTypeField = ({ - label, + sectionType, control, onChange, }: { - label: string + sectionType: 'target' | 'host' control: Control onChange: () => void }) => ( + sectionType: 'target' | 'host' }) => { return ( toComboboxItem(v.name))} @@ -289,12 +301,14 @@ export const CommonFields = ({ const DynamicValueField = ({ sectionType, + valueType, control, items, onInputChange, isDisabled, }: { - sectionType: VpcFirewallRuleHostFilter['type'] + sectionType: 'target' | 'host' + valueType: VpcFirewallRuleHostFilter['type'] control: Control items: Array<{ value: string; label: string }> onInputChange?: (value: string) => void @@ -304,7 +318,7 @@ export const CommonFields = ({ { @@ -330,6 +345,46 @@ export const CommonFields = ({ /> ) + const DynamicTypeAndValueFields = ({ + sectionType, + control, + onTypeChange, + valueType, + items, + onInputChange, + isDisabled, + }: { + sectionType: 'target' | 'host' + control: Control + onTypeChange: () => void + valueType: VpcFirewallRuleHostFilter['type'] + items: Array<{ value: string; label: string }> + onInputChange?: (value: string) => void + isDisabled?: boolean + }) => { + return ( + <> + + {/* If specifying a subnet, they must first select the VPC that owns the subnet */} + {hostType === 'subnet' && ( + + )} + + + ) + } + const TypeAndValueTableHeader = () => ( Type @@ -437,16 +492,19 @@ export const CommonFields = ({ } /> { targetForm.setValue('value', '') // clear the value when the type changes }} /> {/* If specifying a subnet, they must first select the VPC that owns the subnet */} - {targetType === 'subnet' && } + {targetType === 'subnet' && ( + + )} targetForm.setValue('value', value)} @@ -586,15 +644,27 @@ export const CommonFields = ({ } /> hostForm.setValue('value', '')} // clear the value when the type changes /> {/* If specifying a subnet, they must first select the VPC that owns the subnet */} - {hostType === 'subnet' && } + {hostType === 'subnet' && ( + + )} hostForm.setValue('value', value)} + isDisabled={isHostFilterInputDisabled} + /> + hostForm.setValue('value', '')} + valueType={hostType} items={hostFilterItems[hostType]} onInputChange={(value) => hostForm.setValue('value', value)} isDisabled={isHostFilterInputDisabled} diff --git a/test/e2e/utils.ts b/test/e2e/utils.ts index 998a561937..04950dd0a9 100644 --- a/test/e2e/utils.ts +++ b/test/e2e/utils.ts @@ -145,6 +145,22 @@ export async function clickRowAction(page: Page, rowText: string, actionName: st await page.getByRole('menuitem', { name: actionName }).click() } +/** + * Select an option from a dropdown + * buttonLocator can either be the drodown's label text or a more elaborate Locator */ +export async function selectOption( + page: Page, + buttonLocator: string | Locator, + option: string +) { + if (typeof buttonLocator === 'string') { + page.getByRole('button', { name: buttonLocator }).click() + } else { + buttonLocator.click() + } + await page.getByRole('option', { name: option }).click() +} + export async function getPageAsUser( browser: Browser, user: 'Hans Jonas' | 'Simone de Beauvoir' From cac1ac324b16f7f4b3b15fa4bdf68d9d26a33fb5 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Thu, 22 Aug 2024 13:57:45 -0700 Subject: [PATCH 15/42] Remove duplicate section --- app/forms/firewall-rules-common.tsx | 41 +++++------------------------ 1 file changed, 7 insertions(+), 34 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 6d8a7362e9..b105623f8c 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -144,7 +144,7 @@ const DocsLinkMessage = () => ( /> ) -const ClearAndAddButton = ({ +const ClearAndAddButtons = ({ isDirty, onClear, onSubmit, @@ -314,7 +314,7 @@ export const CommonFields = ({ onInputChange?: (value: string) => void isDisabled?: boolean }) => - comboboxTypes.includes(sectionType) ? ( + comboboxTypes.includes(valueType) ? ( } /> - { - targetForm.setValue('value', '') // clear the value when the type changes - }} - /> - {/* If specifying a subnet, they must first select the VPC that owns the subnet */} - {targetType === 'subnet' && ( - - )} - targetForm.setValue('value', '')} valueType={targetType} - sectionType="target" - control={targetForm.control} items={targetFilterItems[targetType]} onInputChange={(value) => targetForm.setValue('value', value)} isDisabled={isTargetFilterInputDisabled} /> - targetForm.reset()} onSubmit={submitTarget} @@ -583,7 +573,7 @@ export const CommonFields = ({ }} /> - } /> - hostForm.setValue('value', '')} // clear the value when the type changes - /> - {/* If specifying a subnet, they must first select the VPC that owns the subnet */} - {hostType === 'subnet' && ( - - )} - hostForm.setValue('value', value)} - isDisabled={isHostFilterInputDisabled} - /> hostForm.setValue('value', value)} isDisabled={isHostFilterInputDisabled} /> - hostForm.reset()} onSubmit={submitHost} From 8202ebfd1c6abc2dad4dc8345a71ea28e3be7c21 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Thu, 22 Aug 2024 18:27:19 -0700 Subject: [PATCH 16/42] Refactor duplicated code --- .../form/fields/dropdownItemsList.ts | 61 ------ app/components/form/fields/useItemsList.ts | 20 ++ app/forms/firewall-rules-common.tsx | 188 +++++++----------- 3 files changed, 91 insertions(+), 178 deletions(-) delete mode 100644 app/components/form/fields/dropdownItemsList.ts diff --git a/app/components/form/fields/dropdownItemsList.ts b/app/components/form/fields/dropdownItemsList.ts deleted file mode 100644 index baa1bc28f0..0000000000 --- a/app/components/form/fields/dropdownItemsList.ts +++ /dev/null @@ -1,61 +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 - */ - -import { useMemo } from 'react' - -import { useApiQuery } from '~/api' -import { useVpcSelector } from '~/hooks' - -export const useCustomRouterItems = () => { - const vpcSelector = useVpcSelector() - const routers = useApiQuery('vpcRouterList', { query: { ...vpcSelector } }) - const routerItems = useMemo(() => { - return ( - routers?.data?.items - .filter((item) => item.kind === 'custom') - .map((router) => ({ value: router.id, label: router.name })) || [] - ) - }, [routers]) - - return { isLoading: routers.isLoading, items: routerItems } -} - -// This is a custom hook that returns a list of subnets for a given VPC -// If no VPC is provided, it will use the VPC from the VPC selector -export const useVpcSubnetItems = ({ project, vpc }: { project?: string; vpc?: string }) => { - const vpcSelector = useVpcSelector() - const vpcSubnets = useApiQuery( - 'vpcSubnetList', - { query: { project: project || vpcSelector.project, vpc: vpc || vpcSelector.vpc } }, - { enabled: !!vpc } - ) - const vpcSubnetItems = useMemo(() => { - return ( - vpcSubnets?.data?.items.map((subnet) => ({ - value: subnet.name, - label: subnet.name, - })) || [] - ) - }, [vpcSubnets]) - - return { isLoading: vpcSubnets.isLoading, items: vpcSubnetItems } -} - -export const useInstanceItems = () => { - const instances = useApiQuery('instanceList', {}) - const instanceItems = useMemo(() => { - return ( - instances?.data?.items.map((instance) => ({ - value: instance.id, - label: instance.name, - })) || [] - ) - }, [instances]) - - return { isLoading: instances.isLoading, items: instanceItems } -} diff --git a/app/components/form/fields/useItemsList.ts b/app/components/form/fields/useItemsList.ts index f364899c84..b2de3fbd9e 100644 --- a/app/components/form/fields/useItemsList.ts +++ b/app/components/form/fields/useItemsList.ts @@ -42,3 +42,23 @@ export const useCustomRouterItems = () => { return { isLoading, items: routerItems } } + +// Get a list of subnets for a given VPC; If no project/VPC is provided, use the VPC from the VpcSelector +export const useVpcSubnetItems = ({ project, vpc }: { project?: string; vpc?: string }) => { + const vpcSelector = useVpcSelector() + const vpcSubnets = useApiQuery( + 'vpcSubnetList', + { query: { project: project || vpcSelector.project, vpc: vpc || vpcSelector.vpc } }, + { enabled: !!vpc } + ) + const vpcSubnetItems = useMemo(() => { + return ( + vpcSubnets?.data?.items.map((subnet) => ({ + value: subnet.name, + label: subnet.name, + })) || [] + ) + }, [vpcSubnets]) + + return { isLoading: vpcSubnets.isLoading, items: vpcSubnetItems } +} diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index b105623f8c..7948fb02b8 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -20,12 +20,12 @@ import { parsePortRange } from '~/api/util' import { CheckboxField } from '~/components/form/fields/CheckboxField' import { ComboboxField } from '~/components/form/fields/ComboboxField' import { DescriptionField } from '~/components/form/fields/DescriptionField' -import { useVpcSubnetItems } from '~/components/form/fields/dropdownItemsList' import { ListboxField } from '~/components/form/fields/ListboxField' import { NameField } 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 { useVpcSubnetItems } from '~/components/form/fields/useItemsList' import { useForm } from '~/hooks/use-form' import { Badge } from '~/ui/lib/Badge' import { Button } from '~/ui/lib/Button' @@ -93,21 +93,35 @@ const getFilterValueProps = ( ) => { switch (hostType) { case 'vpc': - return { label: 'VPC name', ariaLabel: `Select ${sectionType} VPC name` } + return { + label: 'VPC name', + placeholder: 'Select a VPC', + ariaLabel: `Select ${sectionType} VPC name`, + } case 'subnet': - return { label: 'Subnet name', ariaLabel: `Select ${sectionType} subnet name` } + return { + label: 'Subnet name', + placeholder: 'Select a VPC subnet', + ariaLabel: `Select ${sectionType} subnet name`, + } case 'instance': - return { label: 'Instance name', ariaLabel: `Select ${sectionType} instance name` } + return { + label: 'Instance name', + placeholder: 'Select an instance', + ariaLabel: `Select ${sectionType} instance name`, + } case 'ip': return { label: 'IP address', helpText: 'An IPv4 or IPv6 address', + placeholder: 'Enter an IP address', ariaLabel: `Enter ${sectionType} IP address`, } case 'ip_net': return { label: 'IP network', helpText: 'Looks like 192.168.0.0/16 or fd00:1122:3344:0001::1/64', + placeholder: 'Enter an IP network', ariaLabel: `Enter ${sectionType} IP network`, } } @@ -144,6 +158,7 @@ const DocsLinkMessage = () => ( /> ) +// The "Clear" and "Add …" buttons that appear below the filter input fields const ClearAndAddButtons = ({ isDirty, onClear, @@ -219,8 +234,6 @@ export const CommonFields = ({ ip_net: [], } - const isTargetFilterInputDisabled = targetType === 'subnet' && !targetSubnetVpc - const submitTarget = targetForm.handleSubmit(({ type, value }) => { // TODO: do this with a normal validation // ignore click if empty or a duplicate @@ -248,103 +261,6 @@ export const CommonFields = ({ ip_net: [], } - const isHostFilterInputDisabled = hostType === 'subnet' && !hostSubnetVpc - - // In the firewall rules form, these types get comboboxes instead of text fields - const comboboxTypes = ['vpc', 'subnet', 'instance'] - - // The DynamicType and DynamicValue fields allow the user to select the type of - // filter (e.g. VPC, subnet, instance) and then input the value of that filter. - // TODO: make ListboxField smarter with the values like RadioField is - const DynamicTypeField = ({ - sectionType, - control, - onChange, - }: { - sectionType: 'target' | 'host' - control: Control - onChange: () => void - }) => ( - - ) - - // If the type is 'subnet', the user must first select the VPC that owns the subnet - const SubnetVpcField = ({ - control, - sectionType, - }: { - control: Control - sectionType: 'target' | 'host' - }) => { - return ( - toComboboxItem(v.name))} - // when this changes, we need to re-fetch the subnet list - onChange={() => { - queryClient.invalidateQueries('vpcSubnetList') - }} - /> - ) - } - - const DynamicValueField = ({ - sectionType, - valueType, - control, - items, - onInputChange, - isDisabled, - }: { - sectionType: 'target' | 'host' - valueType: VpcFirewallRuleHostFilter['type'] - control: Control - items: Array<{ value: string; label: string }> - onInputChange?: (value: string) => void - isDisabled?: boolean - }) => - comboboxTypes.includes(valueType) ? ( - - ) : ( - { - if (e.key === KEYS.enter) { - e.preventDefault() // prevent full form submission - submitTarget(e) - } - }} - // TODO: validate here, but it's complicated because it's conditional - // on which type is selected - /> - ) - const DynamicTypeAndValueFields = ({ sectionType, control, @@ -364,23 +280,61 @@ export const CommonFields = ({ }) => { return ( <> - {/* If specifying a subnet, they must first select the VPC that owns the subnet */} - {hostType === 'subnet' && ( - + {valueType === 'subnet' && ( + toComboboxItem(v.name))} + // when this changes, we need to re-fetch the subnet list + onChange={() => { + queryClient.invalidateQueries('vpcSubnetList') + }} + placeholder="Select a VPC" + /> + )} + {/* In the firewall rules form, these types get comboboxes instead of text fields */} + {['vpc', 'subnet', 'instance'].includes(valueType) ? ( + + ) : ( + { + if (e.key === KEYS.enter) { + e.preventDefault() // prevent full form submission + submitTarget(e) + } + }} + // TODO: validate here, but it's complicated because it's conditional + // on which type is selected + /> )} - ) } @@ -498,7 +452,7 @@ export const CommonFields = ({ valueType={targetType} items={targetFilterItems[targetType]} onInputChange={(value) => targetForm.setValue('value', value)} - isDisabled={isTargetFilterInputDisabled} + isDisabled={targetType === 'subnet' && !targetSubnetVpc} /> hostForm.setValue('value', value)} - isDisabled={isHostFilterInputDisabled} + isDisabled={hostType === 'subnet' && !hostSubnetVpc} /> Date: Thu, 22 Aug 2024 19:13:54 -0700 Subject: [PATCH 17/42] More cleanup --- app/components/form/fields/useItemsList.ts | 16 +++-- app/forms/firewall-rules-common.tsx | 80 ++++++++++++---------- 2 files changed, 54 insertions(+), 42 deletions(-) diff --git a/app/components/form/fields/useItemsList.ts b/app/components/form/fields/useItemsList.ts index b2de3fbd9e..077d64cce0 100644 --- a/app/components/form/fields/useItemsList.ts +++ b/app/components/form/fields/useItemsList.ts @@ -44,21 +44,27 @@ export const useCustomRouterItems = () => { } // Get a list of subnets for a given VPC; If no project/VPC is provided, use the VPC from the VpcSelector -export const useVpcSubnetItems = ({ project, vpc }: { project?: string; vpc?: string }) => { +export const useVpcSubnets = ({ project, vpc }: { project?: string; vpc?: string }) => { const vpcSelector = useVpcSelector() - const vpcSubnets = useApiQuery( + const { isLoading, data } = useApiQuery( 'vpcSubnetList', { query: { project: project || vpcSelector.project, vpc: vpc || vpcSelector.vpc } }, { enabled: !!vpc } ) + return { isLoading, items: data?.items } +} + +// Get a list of subnets for a given VPC; If no project/VPC is provided, use the VPC from the VpcSelector +export const useVpcSubnetItems = ({ project, vpc }: { project?: string; vpc?: string }) => { + const { isLoading, items } = useVpcSubnets({ project, vpc }) const vpcSubnetItems = useMemo(() => { return ( - vpcSubnets?.data?.items.map((subnet) => ({ + items?.map((subnet) => ({ value: subnet.name, label: subnet.name, })) || [] ) - }, [vpcSubnets]) + }, [items]) - return { isLoading: vpcSubnets.isLoading, items: vpcSubnetItems } + return { isLoading, items: vpcSubnetItems } } diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 7948fb02b8..6df84b64cc 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -25,7 +25,7 @@ import { NameField } 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 { useVpcSubnetItems } from '~/components/form/fields/useItemsList' +import { useVpcSubnetItems, useVpcSubnets } from '~/components/form/fields/useItemsList' import { useForm } from '~/hooks/use-form' import { Badge } from '~/ui/lib/Badge' import { Button } from '~/ui/lib/Button' @@ -194,38 +194,15 @@ export const CommonFields = ({ instances, vpcs, }: CommonFieldsProps) => { - const portRangeForm = useForm({ defaultValues: portRangeDefaultValues }) - const ports = useController({ name: 'ports', control }).field - 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() - }) - - const hostForm = useForm({ defaultValues: hostDefaultValues }) - const hosts = useController({ name: 'hosts', control }).field - const submitHost = hostForm.handleSubmit(({ type, value }) => { - // ignore click if empty or a duplicate - // TODO: show error instead of ignoring click - if (!type || !value) return - if (hosts.value.some((t) => t.value === value && t.type === type)) return - - hosts.onChange([...hosts.value, { type, value }]) - hostForm.reset() - }) - + // Targets const targetForm = useForm({ defaultValues: targetDefaultValues }) const targets = useController({ name: 'targets', control }).field - const targetType = targetForm.watch('type') const targetSubnetVpc = targetForm.watch('subnetVpc') - + // get the list of subnets for the VPC selected in the form const { items: targetVpcSubnets } = useVpcSubnetItems({ project, vpc: targetSubnetVpc }) - const targetFilterItems = { - vpc: vpcs.map((v) => ({ value: v.name, label: v.name })), + vpc: vpcs.map((v) => toComboboxItem(v.name)), subnet: targetVpcSubnets?.filter( ({ label }: { label: string }) => !targets.value.map((t) => t.value).includes(label) ), @@ -233,33 +210,61 @@ export const CommonFields = ({ ip: [], ip_net: [], } - const submitTarget = targetForm.handleSubmit(({ type, value }) => { // TODO: do this with a normal validation // ignore click if empty or a duplicate // TODO: show error instead of ignoring click if (!type || !value) return if (targets.value.some((t) => t.value === value && t.type === type)) return - targets.onChange([...targets.value, { type, value }]) targetForm.reset() }) - const queryClient = useApiQueryClient() + // Ports + const portRangeForm = useForm({ defaultValues: portRangeDefaultValues }) + const ports = useController({ name: 'ports', control }).field + 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() + }) + + // Hosts + const hostForm = useForm({ defaultValues: hostDefaultValues }) + const hosts = useController({ name: 'hosts', control }).field const hostType = hostForm.watch('type') const hostSubnetVpc = hostForm.watch('subnetVpc') + // get the list of subnets for the VPC selected in the form + const { items: hostVpcSubnets } = useVpcSubnets({ project, vpc: hostSubnetVpc }) + const isNotAlreadyAHost = (name: string) => + !hosts.value.map((h) => h.value).includes(name) - const { items: hostVpcSubnets } = useVpcSubnetItems({ project, vpc: hostSubnetVpc }) + // ROUGH EDGE: Deduping the map/filter/map below, fleshing out `isNotAlreadyAHost` above const hostFilterItems = { - vpc: vpcs.map((v) => ({ value: v.name, label: v.name })), - // filter out subnets that are already targets - subnet: hostVpcSubnets?.filter( - ({ label }: { label: string }) => !hosts.value.map((h) => h.value).includes(label) - ), - instance: instances.map((i) => toComboboxItem(i.name)), + vpc: vpcs + .map((v) => v.name) + // filter out VPCs that are already hosts + .filter((name) => isNotAlreadyAHost(name)) + .map((name) => toComboboxItem(name)), + // filter out subnets that are already hosts + subnet: hostVpcSubnets?.map((s) => s.name)?.filter((name) => isNotAlreadyAHost(name)), + instance: instances + .map((i) => i.name) + .filter((name) => isNotAlreadyAHost(name)) + .map((name) => toComboboxItem(name)), ip: [], ip_net: [], } + const submitHost = hostForm.handleSubmit(({ type, value }) => { + // ignore click if empty or a duplicate + // TODO: show error instead of ignoring click + if (!type || !value) return + if (hosts.value.some((t) => t.value === value && t.type === type)) return + hosts.onChange([...hosts.value, { type, value }]) + hostForm.reset() + }) const DynamicTypeAndValueFields = ({ sectionType, @@ -278,6 +283,7 @@ export const CommonFields = ({ onInputChange?: (value: string) => void isDisabled?: boolean }) => { + const queryClient = useApiQueryClient() return ( <> Date: Fri, 23 Aug 2024 13:36:19 -0700 Subject: [PATCH 18/42] More cleanup --- app/forms/firewall-rules-common.tsx | 137 ++++++++++++++-------------- 1 file changed, 68 insertions(+), 69 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 6df84b64cc..71c23cd496 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -15,6 +15,7 @@ import { type Vpc, type VpcFirewallRuleHostFilter, type VpcFirewallRuleTarget, + type VpcSubnet, } from '~/api' import { parsePortRange } from '~/api/util' import { CheckboxField } from '~/components/form/fields/CheckboxField' @@ -25,7 +26,7 @@ import { NameField } 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 { useVpcSubnetItems, useVpcSubnets } from '~/components/form/fields/useItemsList' +import { useVpcSubnets } from '~/components/form/fields/useItemsList' import { useForm } from '~/hooks/use-form' import { Badge } from '~/ui/lib/Badge' import { Button } from '~/ui/lib/Button' @@ -40,6 +41,17 @@ import { capitalize } from '~/util/str' import { type FirewallRuleValues } from './firewall-rules-util' +type TargetFormValues = { + type: VpcFirewallRuleTarget['type'] + value: string + subnetVpc?: string +} + +const targetDefaultValues: TargetFormValues = { + type: 'vpc', + value: '', +} + type PortRangeFormValues = { portRange: string } @@ -59,17 +71,6 @@ const hostDefaultValues: HostFormValues = { value: '', } -type TargetFormValues = { - type: VpcFirewallRuleTarget['type'] - value: string - subnetVpc?: string -} - -const targetDefaultValues: TargetFormValues = { - type: 'vpc', - value: '', -} - type CommonFieldsProps = { error: ApiError | null control: Control @@ -186,6 +187,54 @@ const ClearAndAddButtons = ({ ) +const TypeAndValueTableHeader = () => ( + + Type + Value + {/* For remove button */} + + +) + +const TypeAndValueTableRow = ({ + type, + value, + index, + onRemove, + targetOrHost, +}: { + type: string + value: string + index: number + onRemove: () => void + targetOrHost: 'target' | 'host' +}) => ( + + + {type} + + {value} + + +) + +const availableItems = ( + committedItems: Array, + // Items is conditional because VPC Subnet fetching isn't 100% guaranteed + items?: Array +) => { + if (!items) return [] + return items + .map((i) => i.name) + .filter((name) => !committedItems.map((ci) => ci.value).includes(name)) + .map((name) => toComboboxItem(name)) +} + export const CommonFields = ({ error, control, @@ -200,13 +249,11 @@ export const CommonFields = ({ const targetType = targetForm.watch('type') const targetSubnetVpc = targetForm.watch('subnetVpc') // get the list of subnets for the VPC selected in the form - const { items: targetVpcSubnets } = useVpcSubnetItems({ project, vpc: targetSubnetVpc }) + const { items: targetVpcSubnets } = useVpcSubnets({ project, vpc: targetSubnetVpc }) const targetFilterItems = { - vpc: vpcs.map((v) => toComboboxItem(v.name)), - subnet: targetVpcSubnets?.filter( - ({ label }: { label: string }) => !targets.value.map((t) => t.value).includes(label) - ), - instance: instances.map((i) => toComboboxItem(i.name)), + vpc: availableItems(targets.value, vpcs), + subnet: availableItems(targets.value, targetVpcSubnets), + instance: availableItems(targets.value, instances), ip: [], ip_net: [], } @@ -238,22 +285,10 @@ export const CommonFields = ({ const hostSubnetVpc = hostForm.watch('subnetVpc') // get the list of subnets for the VPC selected in the form const { items: hostVpcSubnets } = useVpcSubnets({ project, vpc: hostSubnetVpc }) - const isNotAlreadyAHost = (name: string) => - !hosts.value.map((h) => h.value).includes(name) - - // ROUGH EDGE: Deduping the map/filter/map below, fleshing out `isNotAlreadyAHost` above const hostFilterItems = { - vpc: vpcs - .map((v) => v.name) - // filter out VPCs that are already hosts - .filter((name) => isNotAlreadyAHost(name)) - .map((name) => toComboboxItem(name)), - // filter out subnets that are already hosts - subnet: hostVpcSubnets?.map((s) => s.name)?.filter((name) => isNotAlreadyAHost(name)), - instance: instances - .map((i) => i.name) - .filter((name) => isNotAlreadyAHost(name)) - .map((name) => toComboboxItem(name)), + vpc: availableItems(hosts.value, vpcs), + subnet: availableItems(hosts.value, hostVpcSubnets), + instance: availableItems(hosts.value, instances), ip: [], ip_net: [], } @@ -345,42 +380,6 @@ export const CommonFields = ({ ) } - const TypeAndValueTableHeader = () => ( - - Type - Value - {/* For remove button */} - - - ) - - const TypeAndValueTableRow = ({ - type, - value, - index, - onRemove, - targetOrHost, - }: { - type: string - value: string - index: number - onRemove: () => void - targetOrHost: 'target' | 'host' - }) => ( - - - {type} - - {value} - - - ) - return ( <> From 757695acb57113b6b0226943805581c500ea78a6 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 23 Aug 2024 14:33:15 -0700 Subject: [PATCH 19/42] A bit more sorting --- app/forms/firewall-rules-common.tsx | 178 +++++++++++++++------------- 1 file changed, 93 insertions(+), 85 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 71c23cd496..b78268a30a 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -235,6 +235,89 @@ const availableItems = ( .map((name) => toComboboxItem(name)) } +const DynamicTypeAndValueFields = ({ + sectionType, + control, + valueType, + items, + vpcs, + isDisabled, + onInputChange, + onTypeChange, + onSubmitTextField, +}: { + sectionType: 'target' | 'host' + control: Control + valueType: VpcFirewallRuleTarget['type'] | VpcFirewallRuleHostFilter['type'] + items: Array<{ value: string; label: string }> + vpcs: Array + isDisabled?: boolean + onInputChange?: (value: string) => void + onTypeChange: () => void + onSubmitTextField: (e: React.KeyboardEvent) => void +}) => { + const queryClient = useApiQueryClient() + return ( + <> + + {/* If specifying a subnet, they must first select the VPC that owns the subnet */} + {valueType === 'subnet' && ( + toComboboxItem(v.name))} + // when this changes, we need to re-fetch the subnet list + onChange={() => { + queryClient.invalidateQueries('vpcSubnetList') + }} + placeholder="Select a VPC" + /> + )} + {/* In the firewall rules form, these types get comboboxes instead of text fields */} + {['vpc', 'subnet', 'instance'].includes(valueType) ? ( + + ) : ( + { + if (e.key === KEYS.enter) { + e.preventDefault() // prevent full form submission + onSubmitTextField(e) + } + }} + // TODO: validate here, but it's complicated because it's conditional + // on which type is selected + /> + )} + + ) +} + export const CommonFields = ({ error, control, @@ -250,7 +333,7 @@ export const CommonFields = ({ const targetSubnetVpc = targetForm.watch('subnetVpc') // get the list of subnets for the VPC selected in the form const { items: targetVpcSubnets } = useVpcSubnets({ project, vpc: targetSubnetVpc }) - const targetFilterItems = { + const targetItems = { vpc: availableItems(targets.value, vpcs), subnet: availableItems(targets.value, targetVpcSubnets), instance: availableItems(targets.value, instances), @@ -301,85 +384,6 @@ export const CommonFields = ({ hostForm.reset() }) - const DynamicTypeAndValueFields = ({ - sectionType, - control, - onTypeChange, - valueType, - items, - onInputChange, - isDisabled, - }: { - sectionType: 'target' | 'host' - control: Control - onTypeChange: () => void - valueType: VpcFirewallRuleHostFilter['type'] - items: Array<{ value: string; label: string }> - onInputChange?: (value: string) => void - isDisabled?: boolean - }) => { - const queryClient = useApiQueryClient() - return ( - <> - - {/* If specifying a subnet, they must first select the VPC that owns the subnet */} - {valueType === 'subnet' && ( - toComboboxItem(v.name))} - // when this changes, we need to re-fetch the subnet list - onChange={() => { - queryClient.invalidateQueries('vpcSubnetList') - }} - placeholder="Select a VPC" - /> - )} - {/* In the firewall rules form, these types get comboboxes instead of text fields */} - {['vpc', 'subnet', 'instance'].includes(valueType) ? ( - - ) : ( - { - if (e.key === KEYS.enter) { - e.preventDefault() // prevent full form submission - submitTarget(e) - } - }} - // TODO: validate here, but it's complicated because it's conditional - // on which type is selected - /> - )} - - ) - } - return ( <> @@ -453,11 +457,13 @@ export const CommonFields = ({ targetForm.setValue('value', '')} valueType={targetType} - items={targetFilterItems[targetType]} - onInputChange={(value) => targetForm.setValue('value', value)} + items={targetItems[targetType]} + vpcs={vpcs} isDisabled={targetType === 'subnet' && !targetSubnetVpc} + onTypeChange={() => targetForm.setValue('value', '')} + onInputChange={(value) => targetForm.setValue('value', value)} + onSubmitTextField={submitTarget} /> hostForm.setValue('value', '')} valueType={hostType} items={hostFilterItems[hostType]} - onInputChange={(value) => hostForm.setValue('value', value)} + vpcs={vpcs} isDisabled={hostType === 'subnet' && !hostSubnetVpc} + onTypeChange={() => hostForm.setValue('value', '')} + onInputChange={(value) => hostForm.setValue('value', value)} + onSubmitTextField={submitHost} /> Date: Fri, 23 Aug 2024 15:52:48 -0700 Subject: [PATCH 20/42] Last cleanup before tests, hopefully --- app/forms/firewall-rules-common.tsx | 341 ++++++++++++++-------------- 1 file changed, 168 insertions(+), 173 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index b78268a30a..8f9a440375 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -41,58 +41,34 @@ import { capitalize } from '~/util/str' import { type FirewallRuleValues } from './firewall-rules-util' -type TargetFormValues = { - type: VpcFirewallRuleTarget['type'] - value: string - subnetVpc?: string -} - -const targetDefaultValues: TargetFormValues = { - type: 'vpc', - value: '', -} - -type PortRangeFormValues = { - portRange: string -} +/** + * 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 DynamicTypeAndValueFields and ClearAndAddButtons + * components. We also then set up the Targets / Ports / Hosts variables at the + * top of the CommonFields component. + */ -const portRangeDefaultValues: PortRangeFormValues = { - portRange: '', -} +type TargetAndHostFilterType = + | VpcFirewallRuleTarget['type'] + | VpcFirewallRuleHostFilter['type'] -type HostFormValues = { - type: VpcFirewallRuleHostFilter['type'] +type TargetAndHostFormValues = { + type: TargetAndHostFilterType value: string subnetVpc?: string } -const hostDefaultValues: HostFormValues = { - type: 'vpc', - value: '', -} - -type CommonFieldsProps = { - error: ApiError | null - control: Control - project: string - instances: Array - vpcs: Array - nameTaken: (name: string) => boolean -} - -const targetAndHostItems = [ - { value: 'vpc', label: 'VPC' }, - { value: 'subnet', label: 'VPC Subnet' }, - { value: 'instance', label: 'Instance' }, - { value: 'ip', label: 'IP' }, - { value: 'ip_net', label: 'IP subnet' }, -] - +// these are part of the target and host filter form; +// the specific values depend on the target or host filter type selected const getFilterValueProps = ( - hostType: VpcFirewallRuleHostFilter['type'], + targetOrHostType: TargetAndHostFilterType, sectionType: 'target' | 'host' ) => { - switch (hostType) { + switch (targetOrHostType) { case 'vpc': return { label: 'VPC name', @@ -128,36 +104,94 @@ const getFilterValueProps = ( } } -const DocsLinkMessage = () => ( - - Read the{' '} - - guest networking guide - {' '} - and{' '} - - API docs - {' '} - to learn more about firewall rules. - - } - /> -) +const DynamicTypeAndValueFields = ({ + sectionType, + control, + valueType, + items, + vpcs, + isDisabled, + onInputChange, + onTypeChange, + onSubmitTextField, +}: { + sectionType: 'target' | 'host' + control: Control + valueType: TargetAndHostFilterType + items: Array<{ value: string; label: string }> + vpcs: Array + isDisabled?: boolean + onInputChange?: (value: string) => void + onTypeChange: () => void + onSubmitTextField: (e: React.KeyboardEvent) => void +}) => { + const queryClient = useApiQueryClient() + return ( + <> + + {/* If specifying a subnet, they must first select the VPC that owns the subnet */} + {valueType === 'subnet' && ( + toComboboxItem(v.name))} + // when this changes, we need to re-fetch the subnet list + onChange={() => { + queryClient.invalidateQueries('vpcSubnetList') + }} + placeholder="Select a VPC" + /> + )} + {/* In the firewall rules form, these types get comboboxes instead of text fields */} + {['vpc', 'subnet', 'instance'].includes(valueType) ? ( + + ) : ( + { + if (e.key === KEYS.enter) { + e.preventDefault() // prevent full form submission + onSubmitTextField(e) + } + }} + // TODO: validate here, but it's complicated because it's conditional + // on which type is selected + /> + )} + + ) +} // The "Clear" and "Add …" buttons that appear below the filter input fields const ClearAndAddButtons = ({ @@ -235,104 +269,49 @@ const availableItems = ( .map((name) => toComboboxItem(name)) } -const DynamicTypeAndValueFields = ({ - sectionType, +const ProtocolField = ({ control, - valueType, - items, - vpcs, - isDisabled, - onInputChange, - onTypeChange, - onSubmitTextField, + protocol, }: { - sectionType: 'target' | 'host' - control: Control - valueType: VpcFirewallRuleTarget['type'] | VpcFirewallRuleHostFilter['type'] - items: Array<{ value: string; label: string }> - vpcs: Array - isDisabled?: boolean - onInputChange?: (value: string) => void - onTypeChange: () => void - onSubmitTextField: (e: React.KeyboardEvent) => void + control: Control + protocol: 'TCP' | 'UDP' | 'ICMP' }) => { - const queryClient = useApiQueryClient() return ( - <> - - {/* If specifying a subnet, they must first select the VPC that owns the subnet */} - {valueType === 'subnet' && ( - toComboboxItem(v.name))} - // when this changes, we need to re-fetch the subnet list - onChange={() => { - queryClient.invalidateQueries('vpcSubnetList') - }} - placeholder="Select a VPC" - /> - )} - {/* In the firewall rules form, these types get comboboxes instead of text fields */} - {['vpc', 'subnet', 'instance'].includes(valueType) ? ( - - ) : ( - { - if (e.key === KEYS.enter) { - e.preventDefault() // prevent full form submission - onSubmitTextField(e) - } - }} - // TODO: validate here, but it's complicated because it's conditional - // on which type is selected - /> - )} - +
+ + {protocol} + +
) } +type CommonFieldsProps = { + control: Control + project: string + instances: Array + vpcs: Array + nameTaken: (name: string) => boolean + error: ApiError | null +} + export const CommonFields = ({ - error, control, - nameTaken, project, instances, vpcs, + nameTaken, + error, }: CommonFieldsProps) => { + const targetAndHostDefaultValues: TargetAndHostFormValues = { type: 'vpc', value: '' } + // Targets - const targetForm = useForm({ defaultValues: targetDefaultValues }) + const targetForm = useForm({ defaultValues: targetAndHostDefaultValues }) const targets = useController({ name: 'targets', control }).field const targetType = targetForm.watch('type') const targetSubnetVpc = targetForm.watch('subnetVpc') - // get the list of subnets for the VPC selected in the form + // get the list of subnets for the specific VPC selected in the form const { items: targetVpcSubnets } = useVpcSubnets({ project, vpc: targetSubnetVpc }) + // get the list of items that are not already in the list of targets const targetItems = { vpc: availableItems(targets.value, vpcs), subnet: availableItems(targets.value, targetVpcSubnets), @@ -351,7 +330,7 @@ export const CommonFields = ({ }) // Ports - const portRangeForm = useForm({ defaultValues: portRangeDefaultValues }) + const portRangeForm = useForm({ defaultValues: { portRange: '' } }) const ports = useController({ name: 'ports', control }).field const submitPortRange = portRangeForm.handleSubmit(({ portRange }) => { const portRangeValue = portRange.trim() @@ -362,12 +341,13 @@ export const CommonFields = ({ }) // Hosts - const hostForm = useForm({ defaultValues: hostDefaultValues }) + const hostForm = useForm({ defaultValues: targetAndHostDefaultValues }) const hosts = useController({ name: 'hosts', control }).field const hostType = hostForm.watch('type') const hostSubnetVpc = hostForm.watch('subnetVpc') - // get the list of subnets for the VPC selected in the form + // get the list of subnets for the specific PC selected in the form const { items: hostVpcSubnets } = useVpcSubnets({ project, vpc: hostSubnetVpc }) + // get the list of items that are not already in the list of host filters const hostFilterItems = { vpc: availableItems(hosts.value, vpcs), subnet: availableItems(hosts.value, hostVpcSubnets), @@ -386,7 +366,34 @@ export const CommonFields = ({ return ( <> - + + Read the{' '} + + guest networking guide + {' '} + and{' '} + + API docs + {' '} + to learn more about firewall rules. + + } + /> {/* omitting value prop makes it a boolean value. beautiful */} {/* TODO: better text or heading or tip or something on this checkbox */} @@ -469,7 +476,7 @@ export const CommonFields = ({ isDirty={targetForm.formState.isDirty} onClear={() => targetForm.reset()} onSubmit={submitTarget} - buttonCopy="Add target filter" + buttonCopy="Add target" /> @@ -569,21 +576,9 @@ export const CommonFields = ({
Protocol filters -
- - TCP - -
-
- - UDP - -
-
- - ICMP - -
+ + +
From 6808ba5b76c0860ff3eb4e6ad503817b0f80ccf8 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 23 Aug 2024 19:11:47 -0700 Subject: [PATCH 21/42] jk optimized the mini table --- app/forms/firewall-rules-common.tsx | 145 ++++++++++------------------ 1 file changed, 52 insertions(+), 93 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 8f9a440375..7f152062ac 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -6,7 +6,7 @@ * Copyright Oxide Computer Company */ -import { useController, type Control } from 'react-hook-form' +import { useController, type Control, type ControllerRenderProps } from 'react-hook-form' import { useApiQueryClient, @@ -221,42 +221,49 @@ const ClearAndAddButtons = ({
) -const TypeAndValueTableHeader = () => ( - - Type - Value - {/* For remove button */} - - -) - -const TypeAndValueTableRow = ({ - type, - value, - index, - onRemove, - targetOrHost, -}: { - type: string - value: string - index: number - onRemove: () => void - targetOrHost: 'target' | 'host' -}) => ( - +} +const TypeAndValueTable = ({ sectionType, items }: TypeAndValueTableProps) => ( + - - {type} - - {value} - - + + Type + Value + {/* For remove button */} + + + + {items.value.map(({ type, value }, index) => ( + + + {type} + + {value} + + items.onChange( + items.value.filter((i) => !(i.value === value && i.type === type)) + ) + } + label={`remove ${sectionType} ${value}`} + /> + + ))} + + ) +// Given an array of committed items (VPCs, Subnets, Instances) and +// a list of all items, return the items that are available const availableItems = ( committedItems: Array, // Items is conditional because VPC Subnet fetching isn't 100% guaranteed @@ -264,26 +271,22 @@ const availableItems = ( ) => { if (!items) return [] return items - .map((i) => i.name) + .map((item) => item.name) .filter((name) => !committedItems.map((ci) => ci.value).includes(name)) .map((name) => toComboboxItem(name)) } -const ProtocolField = ({ - control, - protocol, -}: { +type ProtocolFieldProps = { control: Control protocol: 'TCP' | 'UDP' | 'ICMP' -}) => { - return ( -
- - {protocol} - -
- ) } +const ProtocolField = ({ control, protocol }: ProtocolFieldProps) => ( +
+ + {protocol} + +
+) type CommonFieldsProps = { control: Control @@ -479,33 +482,11 @@ export const CommonFields = ({ buttonCopy="Add target" /> - - {!!targets.value.length && ( - - - - {targets.value.map((t, index) => ( - - targets.onChange( - targets.value.filter((i) => !(i.value === t.value && i.type === t.type)) - ) - } - targetOrHost="target" - /> - ))} - - - )} + {!!targets.value.length && }

Filters

- - {!!ports.value.length && ( @@ -610,29 +590,8 @@ export const CommonFields = ({ onSubmit={submitHost} buttonCopy="Add host filter" /> - - {!!hosts.value.length && ( - - - - {hosts.value.map((h, index) => ( - - hosts.onChange( - hosts.value.filter((i) => !(i.value === h.value && i.type === h.type)) - ) - } - targetOrHost="host" - /> - ))} - - - )} + {!!hosts.value.length && } {error && ( <> From c93bbcaf27d3f2043018d8d0b0a053dac111f6a1 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 23 Aug 2024 20:26:38 -0700 Subject: [PATCH 22/42] Update aria-label for Combobox --- app/forms/firewall-rules-common.tsx | 1 - app/ui/lib/Combobox.tsx | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 7f152062ac..7323663fda 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -175,7 +175,6 @@ const DynamicTypeAndValueFields = ({ ) : ( { const [query, setQuery] = useState(selected || '') @@ -111,7 +113,7 @@ export const Combobox = ({ )} > (selected ? selected : query)} onChange={(event) => { setQuery(event.target.value) From 91a7b9e461aa53ba5dec3d13fc3c245bcf662c6d Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Mon, 26 Aug 2024 14:32:34 -0700 Subject: [PATCH 23/42] Update tests --- app/forms/firewall-rules-common.tsx | 10 ++- test/e2e/firewall-rules.e2e.ts | 96 ++++++++++++++++++----------- test/e2e/utils.ts | 16 +++-- 3 files changed, 79 insertions(+), 43 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 7323663fda..88f1de0ab4 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -64,6 +64,7 @@ type TargetAndHostFormValues = { // these are part of the target and host filter form; // the specific values depend on the target or host filter type selected +// sectionType used for screenreader clarity, and e2e test targeting const getFilterValueProps = ( targetOrHostType: TargetAndHostFilterType, sectionType: 'target' | 'host' @@ -310,6 +311,7 @@ export const CommonFields = ({ const targetForm = useForm({ defaultValues: targetAndHostDefaultValues }) const targets = useController({ name: 'targets', control }).field const targetType = targetForm.watch('type') + const targetValue = targetForm.watch('value') const targetSubnetVpc = targetForm.watch('subnetVpc') // get the list of subnets for the specific VPC selected in the form const { items: targetVpcSubnets } = useVpcSubnets({ project, vpc: targetSubnetVpc }) @@ -334,6 +336,7 @@ export const CommonFields = ({ // 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 @@ -346,6 +349,7 @@ export const CommonFields = ({ const hostForm = useForm({ defaultValues: targetAndHostDefaultValues }) const hosts = useController({ name: 'hosts', control }).field const hostType = hostForm.watch('type') + const hostValue = hostForm.watch('value') const hostSubnetVpc = hostForm.watch('subnetVpc') // get the list of subnets for the specific PC selected in the form const { items: hostVpcSubnets } = useVpcSubnets({ project, vpc: hostSubnetVpc }) @@ -475,7 +479,7 @@ export const CommonFields = ({ onSubmitTextField={submitTarget} /> targetForm.reset()} onSubmit={submitTarget} buttonCopy="Add target" @@ -526,7 +530,7 @@ export const CommonFields = ({ /> hostForm.reset()} onSubmit={submitHost} buttonCopy="Add host filter" diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index 498896d84e..b7a85ef49d 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -6,7 +6,14 @@ * Copyright Oxide Computer Company */ -import { clickRowAction, expect, expectRowVisible, test } from './utils' +import { + clickRowAction, + expect, + expectRowVisible, + selectOption, + test, + type Page, +} from './utils' const defaultRules = ['allow-internal-inbound', 'allow-ssh', 'allow-icmp'] @@ -38,15 +45,13 @@ test('can create firewall rule', async ({ page }) => { // add targets with overlapping names and types to test delete const targets = page.getByRole('table', { name: 'Targets' }) - await page.getByRole('button', { name: 'Target type' }).click() - await page.getByRole('option', { name: 'IP', exact: true }).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' }) // add host filter instance "host-filter-instance" - await page.getByRole('button', { name: 'Host type' }).click() - await page.getByRole('option', { name: 'Instance' }).click() + await selectOption(page, 'Host type', 'Instance') await page.getByRole('combobox', { name: 'Instance name' }).fill('host-filter-instance') await page.getByRole('button', { name: 'Add host filter' }).click() @@ -136,6 +141,15 @@ test('firewall rule targets and filters overflow', async ({ page }) => { await expect(tooltip).toBeVisible() }) +const setupSubnetSelection = async (page: Page, sectionType: 'Host' | 'Target') => { + // first by selecting from a dropdown + await selectOption(page, `${sectionType} type`, 'VPC Subnet') + // select the VPC so you can then add a subnet at the callsite + await selectOption(page, 'VPC Select a VPC', 'mock-vpc') + // select the subnet from the dropdown + // await page.getByRole('option', { name: 'VPC Subnet' }).click() +} + test('firewall rule form targets table', async ({ page }) => { await page.goto('/projects/mock-project/vpcs/mock-vpc') await page.getByRole('tab', { name: 'Firewall Rules' }).click() @@ -144,33 +158,37 @@ test('firewall rule form targets table', async ({ page }) => { await page.getByRole('link', { name: 'New rule' }).click() const targets = page.getByRole('table', { name: 'Targets' }) + const targetVpcNameField = page.getByRole('combobox', { name: 'VPC name' }).nth(0) const addButton = page.getByRole('button', { name: 'Add target' }) // add targets with overlapping names and types to test delete // there are two of these because the hosts table also defaults to VPC - await page.getByRole('textbox', { name: 'VPC name' }).nth(0).fill('abc') + await targetVpcNameField.fill('abc') await addButton.click() await expectRowVisible(targets, { Type: 'vpc', Value: 'abc' }) - await page.getByRole('textbox', { name: 'VPC name' }).nth(0).fill('def') + await targetVpcNameField.fill('def') await addButton.click() await expectRowVisible(targets, { Type: 'vpc', Value: 'def' }) - // select the target type as VPC Subnet - await page.getByRole('button', { name: 'Target type' }).click() - await page.getByRole('option', { name: 'VPC Subnet' }).click() - // select the VPC - await page.getByLabel('VPC', { exact: true }).nth(0).click() - await page.getByRole('option', { name: 'mock-vpc' }).click() - // select the subnet + // add VPC Subnet targets + + await setupSubnetSelection(page, 'Target') + // and now you can select the option await page.getByRole('combobox', { name: 'Subnet name' }).nth(0).click() await page.getByRole('option', { name: 'mock-subnet' }).click() + // add it and verify that it worked await addButton.click() await expectRowVisible(targets, { Type: 'subnet', Value: 'mock-subnet' }) - await page.getByRole('button', { name: 'Target type' }).click() - await page.getByRole('option', { name: 'IP', exact: true }).click() + // now add a subnet by entering text + await setupSubnetSelection(page, 'Target') + await page.getByRole('combobox', { name: 'Subnet name' }).fill('abc') + await addButton.click() + await expectRowVisible(targets, { Type: 'subnet', Value: 'abc' }) + + 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' }) @@ -180,6 +198,9 @@ test('firewall rule form targets table', async ({ page }) => { .getByRole('row') .nth(1) .getByRole('button', { name: 'remove' }) + // we start with 6 rows, because the header row counts as one + await expect(targets.getByRole('row')).toHaveCount(6) + await firstDeleteButton.click() await expect(targets.getByRole('row')).toHaveCount(5) await firstDeleteButton.click() await expect(targets.getByRole('row')).toHaveCount(4) @@ -200,30 +221,32 @@ test('firewall rule form hosts table', async ({ page }) => { 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 - // there are two of these because the targets table also defaults to VPC - await page.getByRole('combobox', { name: 'VPC' }).fill('abc') + // there are two of these because the target section also defaults to VPC + await hostFiltersVpcNameField.fill('abc') await addButton.click() await expectRowVisible(hosts, { Type: 'vpc', Value: 'abc' }) - await page.getByRole('combobox', { name: 'VPC' }).fill('def') + await hostFiltersVpcNameField.fill('def') await addButton.click() await expectRowVisible(hosts, { Type: 'vpc', Value: 'def' }) - await page.getByRole('button', { name: 'Host type' }).click() - await page.getByRole('option', { name: 'VPC Subnet' }).click() - await page.getByRole('button', { name: 'VPC' }).nth(2).click() - await page.getByRole('option', { name: 'mock-vpc' }).click() + await setupSubnetSelection(page, 'Host') + await selectOption(page, 'Subnet name', 'mock-subnet') + await addButton.click() + await expectRowVisible(hosts, { Type: 'subnet', Value: 'mock-subnet' }) + + await setupSubnetSelection(page, 'Host') await page.getByRole('combobox', { name: 'Subnet name' }).fill('abc') await addButton.click() await expectRowVisible(hosts, { Type: 'subnet', Value: 'abc' }) - await page.getByRole('button', { name: 'Host type' }).click() - await page.getByRole('option', { name: 'IP', exact: true }).click() - await page.getByRole('combobox', { name: 'IP address' }).fill('192.168.0.1') + 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' }) @@ -232,6 +255,8 @@ test('firewall rule form hosts table', async ({ page }) => { .getByRole('row') .nth(1) .getByRole('button', { name: 'remove' }) + await expect(hosts.getByRole('row')).toHaveCount(6) + await firstDeleteButton.click() await expect(hosts.getByRole('row')).toHaveCount(5) await firstDeleteButton.click() await expect(hosts.getByRole('row')).toHaveCount(4) @@ -270,10 +295,10 @@ test('can update firewall rule', async ({ page }) => { // TODO: get these by their label when that becomes easier to do // name is populated - await expect(page.locator('input[name=name]')).toHaveValue('allow-icmp') + await expect(page.getByRole('textbox', { name: 'Name' })).toHaveValue('allow-icmp') // priority is populated - await expect(page.locator('role=textbox[name=Priority]')).toHaveValue('65534') + await expect(page.getByRole('textbox', { name: 'Priority' })).toHaveValue('65534') // protocol is populated await expect(page.locator('label >> text=ICMP')).toBeChecked() @@ -285,19 +310,20 @@ test('can update firewall rule', async ({ page }) => { // screen.getByRole('cell', { name: 'default' }) // update name - await page.fill('input[name=name]', 'new-rule-name') + await page.getByRole('textbox', { name: 'Name' }).fill('new-rule-name') // add host filter - await page.locator('role=button[name*="Host type"]').click() - await page.locator('role=option[name="VPC Subnet"]').click() - await page.fill('role=textbox[name="Subnet name"]', 'edit-filter-subnet') - await page.locator('text="Add host filter"').click() + await setupSubnetSelection(page, 'Host') + await page.getByRole('combobox', { name: 'Subnet name' }).fill('edit-filter-subnet') + await page.getByRole('button', { name: 'Add host filter' }).click() // new host is added to hosts table - await expect(page.locator('role=cell >> text="edit-filter-subnet"')).toBeVisible() + const hosts = page.getByRole('table', { name: 'Host filters' }) + // await expect(page.locator('role=cell >> text="edit-filter-subnet"')).toBeVisible() + await expectRowVisible(hosts, { Type: 'subnet', Value: 'edit-filter-subnet' }) // submit the form - await page.locator('text="Update rule"').click() + await page.getByRole('button', { name: 'Update rule' }).click() // modal closes again await expect(modal).toBeHidden() diff --git a/test/e2e/utils.ts b/test/e2e/utils.ts index fe886930bd..285bb313ca 100644 --- a/test/e2e/utils.ts +++ b/test/e2e/utils.ts @@ -142,18 +142,24 @@ export async function clickRowAction(page: Page, rowText: string, actionName: st /** * Select an option from a dropdown - * buttonLocator can either be the drodown's label text or a more elaborate Locator */ + * buttonLocator can either be the drodown's label text or a more elaborate Locator + * optionLocator can either be the drodown's label text or a more elaborate Locator + * */ export async function selectOption( page: Page, buttonLocator: string | Locator, - option: string + optionLocator: string | Locator ) { if (typeof buttonLocator === 'string') { - page.getByRole('button', { name: buttonLocator }).click() + await page.getByRole('button', { name: buttonLocator }).click() } else { - buttonLocator.click() + await buttonLocator.click() + } + if (typeof optionLocator === 'string') { + await page.getByRole('option', { name: optionLocator, exact: true }).click() + } else { + await optionLocator.click() } - await page.getByRole('option', { name: option }).click() } export async function getPageAsUser( From 84353436ae06fb1f50c3646191cf4d7b995104f2 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Mon, 26 Aug 2024 16:53:55 -0700 Subject: [PATCH 24/42] Cleanup --- app/forms/firewall-rules-common.tsx | 15 ++----- app/forms/firewall-rules-edit.tsx | 8 ++-- test/e2e/firewall-rules.e2e.ts | 62 +++++++++++------------------ 3 files changed, 31 insertions(+), 54 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 88f1de0ab4..ce5cad14fd 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -64,43 +64,34 @@ type TargetAndHostFormValues = { // these are part of the target and host filter form; // the specific values depend on the target or host filter type selected -// sectionType used for screenreader clarity, and e2e test targeting -const getFilterValueProps = ( - targetOrHostType: TargetAndHostFilterType, - sectionType: 'target' | 'host' -) => { +const getFilterValueProps = (targetOrHostType: TargetAndHostFilterType) => { switch (targetOrHostType) { case 'vpc': return { label: 'VPC name', placeholder: 'Select a VPC', - ariaLabel: `Select ${sectionType} VPC name`, } case 'subnet': return { label: 'Subnet name', placeholder: 'Select a VPC subnet', - ariaLabel: `Select ${sectionType} subnet name`, } case 'instance': return { label: 'Instance name', placeholder: 'Select an instance', - ariaLabel: `Select ${sectionType} instance name`, } case 'ip': return { label: 'IP address', helpText: 'An IPv4 or IPv6 address', placeholder: 'Enter an IP address', - ariaLabel: `Enter ${sectionType} IP address`, } case 'ip_net': return { label: 'IP network', helpText: 'Looks like 192.168.0.0/16 or fd00:1122:3344:0001::1/64', placeholder: 'Enter an IP network', - ariaLabel: `Enter ${sectionType} IP network`, } } } @@ -164,7 +155,7 @@ const DynamicTypeAndValueFields = ({ { diff --git a/app/forms/firewall-rules-edit.tsx b/app/forms/firewall-rules-edit.tsx index 1f32f8bfbb..a182366865 100644 --- a/app/forms/firewall-rules-edit.tsx +++ b/app/forms/firewall-rules-edit.tsx @@ -36,6 +36,9 @@ EditFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { const firewallRules = await apiQueryClient.fetchQuery('vpcFirewallRulesView', { query: { project, vpc }, }) + const originalRule = firewallRules.rules.find((r) => r.name === rule) + if (!originalRule) throw trigger404 + await Promise.all([ apiQueryClient.prefetchQuery('instanceList', { query: { project, limit: PAGE_SIZE }, @@ -43,14 +46,11 @@ EditFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { apiQueryClient.prefetchQuery('vpcList', { query: { project, limit: PAGE_SIZE } }), ]) - const originalRule = firewallRules.rules.find((r) => r.name === rule) - if (!originalRule) throw trigger404 - return null } export function EditFirewallRuleForm() { - const { vpc, project, rule } = useFirewallRuleSelector() + const { project, vpc, rule } = useFirewallRuleSelector() const vpcSelector = useVpcSelector() const queryClient = useApiQueryClient() diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index b7a85ef49d..2ae374419a 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -12,6 +12,7 @@ import { expectRowVisible, selectOption, test, + type Locator, type Page, } from './utils' @@ -146,8 +147,13 @@ const setupSubnetSelection = async (page: Page, sectionType: 'Host' | 'Target') await selectOption(page, `${sectionType} type`, 'VPC Subnet') // select the VPC so you can then add a subnet at the callsite await selectOption(page, 'VPC Select a VPC', 'mock-vpc') - // select the subnet from the dropdown - // await page.getByRole('option', { name: 'VPC Subnet' }).click() +} + +const deleteRowAndVerifyRowCount = async (table: Locator, expectedCount: number) => { + const rows = table.getByRole('row') + // skip the header row + await rows.nth(1).getByRole('button', { name: 'remove' }).click() + await expect(rows).toHaveCount(expectedCount) } test('firewall rule form targets table', async ({ page }) => { @@ -163,7 +169,6 @@ test('firewall rule form targets table', async ({ page }) => { // add targets with overlapping names and types to test delete - // there are two of these because the hosts table also defaults to VPC await targetVpcNameField.fill('abc') await addButton.click() await expectRowVisible(targets, { Type: 'vpc', Value: 'abc' }) @@ -173,41 +178,33 @@ test('firewall rule form targets table', async ({ page }) => { await expectRowVisible(targets, { Type: 'vpc', Value: 'def' }) // add VPC Subnet targets + const subnetNameField = page.getByRole('combobox', { name: 'Subnet name' }) + // add a subnet by selecting from a dropdown await setupSubnetSelection(page, 'Target') - // and now you can select the option - await page.getByRole('combobox', { name: 'Subnet name' }).nth(0).click() - await page.getByRole('option', { name: 'mock-subnet' }).click() - // add it and verify that it worked + await selectOption(page, subnetNameField, 'mock-subnet') await addButton.click() await expectRowVisible(targets, { Type: 'subnet', Value: 'mock-subnet' }) // now add a subnet by entering text await setupSubnetSelection(page, 'Target') - await page.getByRole('combobox', { name: 'Subnet name' }).fill('abc') + await subnetNameField.fill('abc') await addButton.click() await expectRowVisible(targets, { Type: 'subnet', Value: 'abc' }) + // 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' }) - // test table row delete, only keep the IP one - const firstDeleteButton = targets - .getByRole('row') - .nth(1) - .getByRole('button', { name: 'remove' }) + // test table row delete; only keep the IP one // we start with 6 rows, because the header row counts as one await expect(targets.getByRole('row')).toHaveCount(6) - await firstDeleteButton.click() - await expect(targets.getByRole('row')).toHaveCount(5) - await firstDeleteButton.click() - await expect(targets.getByRole('row')).toHaveCount(4) - await firstDeleteButton.click() - await expect(targets.getByRole('row')).toHaveCount(3) - await firstDeleteButton.click() - await expect(targets.getByRole('row')).toHaveCount(2) + await deleteRowAndVerifyRowCount(targets, 5) + await deleteRowAndVerifyRowCount(targets, 4) + await deleteRowAndVerifyRowCount(targets, 3) + await deleteRowAndVerifyRowCount(targets, 2) // we still have the IP target await expectRowVisible(targets, { Type: 'ip', Value: '192.168.0.1' }) @@ -250,20 +247,12 @@ test('firewall rule form hosts table', async ({ page }) => { await addButton.click() await expectRowVisible(hosts, { Type: 'ip', Value: '192.168.0.1' }) - // test table row delete, only keep the IP one - const firstDeleteButton = hosts - .getByRole('row') - .nth(1) - .getByRole('button', { name: 'remove' }) + // test table row delete; only keep the IP one await expect(hosts.getByRole('row')).toHaveCount(6) - await firstDeleteButton.click() - await expect(hosts.getByRole('row')).toHaveCount(5) - await firstDeleteButton.click() - await expect(hosts.getByRole('row')).toHaveCount(4) - await firstDeleteButton.click() - await expect(hosts.getByRole('row')).toHaveCount(3) - await firstDeleteButton.click() - await expect(hosts.getByRole('row')).toHaveCount(2) + await deleteRowAndVerifyRowCount(hosts, 5) + await deleteRowAndVerifyRowCount(hosts, 4) + await deleteRowAndVerifyRowCount(hosts, 3) + await deleteRowAndVerifyRowCount(hosts, 2) // we still have the IP target await expectRowVisible(hosts, { Type: 'ip', Value: '192.168.0.1' }) @@ -276,7 +265,7 @@ test('can update firewall rule', async ({ page }) => { const rows = page.locator('tbody >> tr') await expect(rows).toHaveCount(3) - // allow-icmp is the one we're doing to change + // allow-icmp is the one we're going to change const oldNameCell = page.locator('td >> text="allow-icmp"') await expect(oldNameCell).toBeVisible() @@ -292,8 +281,6 @@ test('can update firewall rule', async ({ page }) => { // modal is now open await expect(modal).toBeVisible() - // TODO: get these by their label when that becomes easier to do - // name is populated await expect(page.getByRole('textbox', { name: 'Name' })).toHaveValue('allow-icmp') @@ -319,7 +306,6 @@ test('can update firewall rule', async ({ page }) => { // new host is added to hosts table const hosts = page.getByRole('table', { name: 'Host filters' }) - // await expect(page.locator('role=cell >> text="edit-filter-subnet"')).toBeVisible() await expectRowVisible(hosts, { Type: 'subnet', Value: 'edit-filter-subnet' }) // submit the form From ef44da220e13f7b2118b642b0186cfa625aa390d Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Mon, 26 Aug 2024 17:29:57 -0700 Subject: [PATCH 25/42] showNoMatchPlaceholder -> allowNewItems and change default --- app/forms/firewall-rules-common.tsx | 2 +- app/forms/firewall-rules-create.tsx | 1 - app/ui/lib/Combobox.tsx | 10 +++++----- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index ce5cad14fd..11ee34e240 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -160,7 +160,7 @@ const DynamicTypeAndValueFields = ({ control={control} onInputChange={onInputChange} items={items} - showNoMatchPlaceholder={false} + allowNewItems // TODO: validate here, but it's complicated because it's conditional // on which type is selected /> diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index db66218a17..c03fe49283 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -147,7 +147,6 @@ export function CreateFirewallRuleForm() { vpcs={vpcs.items} // error if name is already in use nameTaken={(name) => !!existingRules.find((r) => r.name === name)} - // 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 // instead of setting a root error, it would be more robust to show a diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index d9e5fe00eb..6772d0e1c8 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -42,9 +42,9 @@ export type ComboboxBaseProps = { required?: boolean tooltipText?: string onInputChange?: (value: string) => void - // set to false in situations where the user should be able to type in new values - // to decide: should we even give a placeholder / warning? - showNoMatchPlaceholder?: boolean + // pass in allowNewItems as true in situations where the user should be + // able to type in new values that aren't in the list (default is false) + allowNewItems?: boolean ariaLabel?: string } @@ -67,7 +67,7 @@ export const Combobox = ({ isLoading, onChange, onInputChange, - showNoMatchPlaceholder = true, + allowNewItems = false, ariaLabel, }: ComboboxProps) => { const [query, setQuery] = useState(selected || '') @@ -142,7 +142,7 @@ export const Combobox = ({ className={`ox-menu pointer-events-auto ${zIndex} relative w-[var(--button-width)] overflow-y-auto border !outline-none border-secondary [--anchor-gap:14px] empty:hidden`} modal={false} > - {showNoMatchPlaceholder && filteredItems.length === 0 && ( + {!allowNewItems && filteredItems.length === 0 && (
No items match
 From 83e419b2127ee46bab17003f770511d4bbf0e150 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Thu, 29 Aug 2024 13:25:25 -0700 Subject: [PATCH 26/42] Add test for 'no items match' --- test/e2e/firewall-rules.e2e.ts | 1 - test/e2e/silos.e2e.ts | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index 2ae374419a..c040e47490 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -223,7 +223,6 @@ test('firewall rule form hosts table', async ({ page }) => { // add hosts with overlapping names and types to test delete - // there are two of these because the target section also defaults to VPC await hostFiltersVpcNameField.fill('abc') await addButton.click() await expectRowVisible(hosts, { Type: 'vpc', Value: 'abc' }) diff --git a/test/e2e/silos.e2e.ts b/test/e2e/silos.e2e.ts index 136057e8e1..b9e5a9279a 100644 --- a/test/e2e/silos.e2e.ts +++ b/test/e2e/silos.e2e.ts @@ -263,6 +263,10 @@ test('Silo IP pools link pool', async ({ page }) => { await page.getByRole('button', { name: 'Link pool' }).click() await expect(modal).toBeVisible() + // verify that combobox's "no items match" appears when addNewItems prop is false or missing + await page.getByPlaceholder('Select a pool').fill('x') + await expect(page.getByText('No items match')).toBeVisible() + // select silo in combobox and click link await page.getByPlaceholder('Select a pool').fill('ip-pool') await page.getByRole('option', { name: 'ip-pool-3' }).click() From b00a78af29aa400eb41dd61e84f506a02b94416f Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 3 Sep 2024 17:28:58 -0700 Subject: [PATCH 27/42] Refactoring post-review --- app/components/form/fields/useItemsList.ts | 15 ------------ app/forms/firewall-rules-common.tsx | 27 ++++++++++++---------- app/forms/firewall-rules-create.tsx | 20 ++-------------- app/forms/firewall-rules-edit.tsx | 11 +-------- app/ui/lib/Combobox.tsx | 12 ++++++---- 5 files changed, 25 insertions(+), 60 deletions(-) diff --git a/app/components/form/fields/useItemsList.ts b/app/components/form/fields/useItemsList.ts index 077d64cce0..78488ca46c 100644 --- a/app/components/form/fields/useItemsList.ts +++ b/app/components/form/fields/useItemsList.ts @@ -53,18 +53,3 @@ export const useVpcSubnets = ({ project, vpc }: { project?: string; vpc?: string ) return { isLoading, items: data?.items } } - -// Get a list of subnets for a given VPC; If no project/VPC is provided, use the VPC from the VpcSelector -export const useVpcSubnetItems = ({ project, vpc }: { project?: string; vpc?: string }) => { - const { isLoading, items } = useVpcSubnets({ project, vpc }) - const vpcSubnetItems = useMemo(() => { - return ( - items?.map((subnet) => ({ - value: subnet.name, - label: subnet.name, - })) || [] - ) - }, [items]) - - return { isLoading, items: vpcSubnetItems } -} diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 11ee34e240..f2bdb3597f 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -10,6 +10,7 @@ import { useController, type Control, type ControllerRenderProps } from 'react-h import { useApiQueryClient, + usePrefetchedApiQuery, type ApiError, type Instance, type Vpc, @@ -27,7 +28,9 @@ import { NumberField } from '~/components/form/fields/NumberField' import { RadioField } from '~/components/form/fields/RadioField' import { TextField, TextFieldInner } from '~/components/form/fields/TextField' import { useVpcSubnets } from '~/components/form/fields/useItemsList' +import { useProjectSelector } from '~/hooks' import { useForm } from '~/hooks/use-form' +import { PAGE_SIZE } from '~/table/QueryTable' import { Badge } from '~/ui/lib/Badge' import { Button } from '~/ui/lib/Button' import { toComboboxItem } from '~/ui/lib/Combobox' @@ -160,7 +163,7 @@ const DynamicTypeAndValueFields = ({ control={control} onInputChange={onInputChange} items={items} - allowNewItems + allowArbitraryValues // TODO: validate here, but it's complicated because it's conditional // on which type is selected /> @@ -281,22 +284,22 @@ const ProtocolField = ({ control, protocol }: ProtocolFieldProps) => ( type CommonFieldsProps = { control: Control - project: string - instances: Array - vpcs: Array nameTaken: (name: string) => boolean error: ApiError | null } -export const CommonFields = ({ - control, - project, - instances, - vpcs, - nameTaken, - error, -}: CommonFieldsProps) => { +export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) => { const targetAndHostDefaultValues: TargetAndHostFormValues = { type: 'vpc', value: '' } + const { project } = useProjectSelector() + // prefetchedApiQueries below are prefetched in firewall-rules-create and -edit + const { data: instancesData } = usePrefetchedApiQuery('instanceList', { + query: { project, limit: PAGE_SIZE }, + }) + const instances = instancesData?.items ?? [] + const { data: vpcData } = usePrefetchedApiQuery('vpcList', { + query: { project, limit: PAGE_SIZE }, + }) + const vpcs = vpcData?.items ?? [] // Targets const targetForm = useForm({ defaultValues: targetAndHostDefaultValues }) diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index c03fe49283..acc3523c1a 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -19,13 +19,7 @@ import { } from '@oxide/api' import { SideModalForm } from '~/components/form/SideModalForm' -import { - getProjectSelector, - getVpcSelector, - useForm, - useProjectSelector, - useVpcSelector, -} from '~/hooks' +import { getProjectSelector, getVpcSelector, useForm, useVpcSelector } from '~/hooks' import { addToast } from '~/stores/toast' import { PAGE_SIZE } from '~/table/QueryTable' import { pb } from '~/util/path-builder' @@ -77,7 +71,6 @@ CreateFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { } export function CreateFirewallRuleForm() { - const { project } = useProjectSelector() const vpcSelector = useVpcSelector() const queryClient = useApiQueryClient() @@ -99,12 +92,6 @@ export function CreateFirewallRuleForm() { () => R.sortBy(vpcFirewallRules.rules, (r) => r.priority), [vpcFirewallRules] ) - const { data: instances } = usePrefetchedApiQuery('instanceList', { - query: { project, limit: PAGE_SIZE }, - }) - const { data: vpcs } = usePrefetchedApiQuery('vpcList', { - query: { project, limit: PAGE_SIZE }, - }) // The :rule path param is optional. If it is present, we are creating a // rule from an existing one, so we find that rule and copy it into the form @@ -140,13 +127,10 @@ export function CreateFirewallRuleForm() { submitLabel="Add rule" > !!existingRules.find((r) => r.name === name)} + 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 // instead of setting a root error, it would be more robust to show a diff --git a/app/forms/firewall-rules-edit.tsx b/app/forms/firewall-rules-edit.tsx index a182366865..8d7f41818c 100644 --- a/app/forms/firewall-rules-edit.tsx +++ b/app/forms/firewall-rules-edit.tsx @@ -57,12 +57,6 @@ export function EditFirewallRuleForm() { const { data: firewallRules } = usePrefetchedApiQuery('vpcFirewallRulesView', { query: { project, vpc }, }) - const { data: instances } = usePrefetchedApiQuery('instanceList', { - query: { project, limit: PAGE_SIZE }, - }) - const { data: vpcs } = usePrefetchedApiQuery('vpcList', { - query: { project, limit: PAGE_SIZE }, - }) const originalRule = firewallRules.rules.find((r) => r.name === rule) @@ -128,13 +122,10 @@ export function EditFirewallRuleForm() { submitError={updateRules.error} > !!otherRules.find((r) => r.name === name)} + error={updateRules.error} /> ) diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index 6772d0e1c8..8301cac834 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -42,9 +42,11 @@ export type ComboboxBaseProps = { required?: boolean tooltipText?: string onInputChange?: (value: string) => void - // pass in allowNewItems as true in situations where the user should be - // able to type in new values that aren't in the list (default is false) - allowNewItems?: boolean + /** + * pass in allowArbitraryValues as `true` when the user should be able to + * type in new values that aren't in the list [default is `false`] + */ + allowArbitraryValues?: boolean ariaLabel?: string } @@ -67,7 +69,7 @@ export const Combobox = ({ isLoading, onChange, onInputChange, - allowNewItems = false, + allowArbitraryValues = false, ariaLabel, }: ComboboxProps) => { const [query, setQuery] = useState(selected || '') @@ -142,7 +144,7 @@ export const Combobox = ({ className={`ox-menu pointer-events-auto ${zIndex} relative w-[var(--button-width)] overflow-y-auto border !outline-none border-secondary [--anchor-gap:14px] empty:hidden`} modal={false} > - {!allowNewItems && filteredItems.length === 0 && ( + {!allowArbitraryValues && filteredItems.length === 0 && (
No items match
 From 089a0561083292889faf5d3574f03139cc85dcd2 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 3 Sep 2024 18:29:44 -0700 Subject: [PATCH 28/42] Add prop to selectively hide 'optional' tag on FieldLabel --- app/components/form/fields/ComboboxField.tsx | 1 - app/forms/firewall-rules-common.tsx | 11 ++++++----- app/ui/lib/Combobox.tsx | 10 +++++++++- app/ui/lib/FieldLabel.tsx | 4 +++- 4 files changed, 18 insertions(+), 8 deletions(-) diff --git a/app/components/form/fields/ComboboxField.tsx b/app/components/form/fields/ComboboxField.tsx index 923baca235..421d3717da 100644 --- a/app/components/form/fields/ComboboxField.tsx +++ b/app/components/form/fields/ComboboxField.tsx @@ -47,7 +47,6 @@ export function ComboboxField< { diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index f2bdb3597f..79d94aa60b 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -72,28 +72,28 @@ const getFilterValueProps = (targetOrHostType: TargetAndHostFilterType) => { case 'vpc': return { label: 'VPC name', - placeholder: 'Select a VPC', + placeholder: 'Select a VPC or type to add', } case 'subnet': return { label: 'Subnet name', - placeholder: 'Select a VPC subnet', + placeholder: 'Select a VPC subnet or type to add', } case 'instance': return { label: 'Instance name', - placeholder: 'Select an instance', + placeholder: 'Select an instance or type to add', } case 'ip': return { label: 'IP address', - helpText: 'An IPv4 or IPv6 address', + description: 'An IPv4 or IPv6 address', placeholder: 'Enter an IP address', } case 'ip_net': return { label: 'IP network', - helpText: 'Looks like 192.168.0.0/16 or fd00:1122:3344:0001::1/64', + description: 'Looks like 192.168.0.0/16 or fd00:1122:3344:0001::1/64', placeholder: 'Enter an IP network', } } @@ -164,6 +164,7 @@ const DynamicTypeAndValueFields = ({ onInputChange={onInputChange} items={items} allowArbitraryValues + showOptionalTag={false} // TODO: validate here, but it's complicated because it's conditional // on which type is selected /> diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index 8301cac834..ed7a15a74a 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -48,6 +48,7 @@ export type ComboboxBaseProps = { */ allowArbitraryValues?: boolean ariaLabel?: string + showOptionalTag?: boolean } type ComboboxProps = { @@ -71,6 +72,7 @@ export const Combobox = ({ onInputChange, allowArbitraryValues = false, ariaLabel, + showOptionalTag, }: ComboboxProps) => { const [query, setQuery] = useState(selected || '') @@ -95,7 +97,13 @@ export const Combobox = ({ {label && ( // TODO: FieldLabel needs a real ID
- + {description && {description}} diff --git a/app/ui/lib/FieldLabel.tsx b/app/ui/lib/FieldLabel.tsx index 5fb298051c..5051b40be1 100644 --- a/app/ui/lib/FieldLabel.tsx +++ b/app/ui/lib/FieldLabel.tsx @@ -17,6 +17,7 @@ interface FieldLabelProps { tip?: string optional?: boolean className?: string + showOptionalTag?: boolean } export const FieldLabel = ({ @@ -27,13 +28,14 @@ export const FieldLabel = ({ optional, as, className, + showOptionalTag = true, }: PropsWithChildren) => { const Component = as || 'label' return (
{children} - {optional && ( + {optional && showOptionalTag && ( // Announcing this optional text is unnecessary as the required attribute on the // form will be used
diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 79d94aa60b..6bf40689f3 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -126,7 +126,6 @@ const DynamicTypeAndValueFields = ({ {/* If specifying a subnet, they must first select the VPC that owns the subnet */} {valueType === 'subnet' && ( @@ -143,7 +143,6 @@ const DynamicTypeAndValueFields = ({ name="subnetVpc" label="VPC" aria-label={`Select ${sectionType} VPC`} - required control={control} items={vpcs.map((v) => toComboboxItem(v.name))} // when this changes, we need to re-fetch the subnet list @@ -151,6 +150,7 @@ const DynamicTypeAndValueFields = ({ queryClient.invalidateQueries('vpcSubnetList') }} placeholder="Select a VPC" + showOptionalTag={false} /> )} {/* In the firewall rules form, these types get comboboxes instead of text fields */} @@ -159,7 +159,6 @@ const DynamicTypeAndValueFields = ({ disabled={isDisabled} name="value" {...getFilterValueProps(valueType)} - required control={control} onInputChange={onInputChange} items={items} @@ -172,7 +171,6 @@ const DynamicTypeAndValueFields = ({ { if (e.key === KEYS.enter) { diff --git a/app/ui/lib/Listbox.tsx b/app/ui/lib/Listbox.tsx index 32289a81b4..016f76a899 100644 --- a/app/ui/lib/Listbox.tsx +++ b/app/ui/lib/Listbox.tsx @@ -44,6 +44,7 @@ export interface ListboxProps { isLoading?: boolean /** Necessary if you want RHF to be able to focus it on error */ buttonRef?: Ref + showOptionalTag?: boolean } export const Listbox = ({ @@ -62,6 +63,7 @@ export const Listbox = ({ disabled, isLoading = false, buttonRef, + showOptionalTag, ...props }: ListboxProps) => { const selectedItem = selected && items.find((i) => i.value === selected) @@ -83,7 +85,13 @@ export const Listbox = ({ <> {label && (
- + {description && {description}} From 045c58a90f66c3b5eab8d2d504272536b307a7ef Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Wed, 4 Sep 2024 12:32:00 -0700 Subject: [PATCH 30/42] Updated placeholder/description text --- app/components/form/fields/ComboboxField.tsx | 19 ++++++++++++++++--- app/forms/disk-attach.tsx | 1 + app/forms/firewall-rules-common.tsx | 9 ++------- app/forms/snapshot-create.tsx | 1 + app/pages/system/SiloImagesPage.tsx | 1 + app/pages/system/networking/IpPoolPage.tsx | 1 - app/pages/system/silos/SiloIpPoolsTab.tsx | 1 - 7 files changed, 21 insertions(+), 12 deletions(-) diff --git a/app/components/form/fields/ComboboxField.tsx b/app/components/form/fields/ComboboxField.tsx index 923baca235..4be1afdf1e 100644 --- a/app/components/form/fields/ComboboxField.tsx +++ b/app/components/form/fields/ComboboxField.tsx @@ -25,7 +25,7 @@ export type ComboboxFieldProps< name: TName control: Control onChange?: (value: string | null | undefined) => void - disabled?: boolean + allowArbitraryValues?: boolean } & ComboboxBaseProps export function ComboboxField< @@ -38,15 +38,27 @@ export function ComboboxField< label = capitalize(name), required, onChange, - disabled, + allowArbitraryValues, + placeholder, + // Intent is to not show both a placeholder and a description, while still having good defaults; prefer a description to a placeholder + // If description is provided, use it + // If not, but a placeholder is provided, the default description should be undefined + // If no placeholder is provided and arbitrary values are allowed, the default description should be 'Select an option or enter a custom value' + // If no placeholder is provided and arbitrary values are not allowed, the default description should be 'Select an option' + description = placeholder + ? undefined + : allowArbitraryValues + ? 'Select an option or enter a custom value' + : 'Select an option', ...props }: ComboboxFieldProps) { const { field, fieldState } = useController({ name, control, rules: { required } }) return (
diff --git a/app/forms/disk-attach.tsx b/app/forms/disk-attach.tsx index 7895c81b91..b5ab3b22d3 100644 --- a/app/forms/disk-attach.tsx +++ b/app/forms/disk-attach.tsx @@ -57,6 +57,7 @@ export function AttachDiskSideModalForm({ ({ value: name, label: name }))} required diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 6bf40689f3..97c09b9e2d 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -72,29 +72,24 @@ const getFilterValueProps = (targetOrHostType: TargetAndHostFilterType) => { case 'vpc': return { label: 'VPC name', - placeholder: 'Select a VPC or type to add', } case 'subnet': return { label: 'Subnet name', - placeholder: 'Select a VPC subnet or type to add', } case 'instance': return { label: 'Instance name', - placeholder: 'Select an instance or type to add', } case 'ip': return { label: 'IP address', - description: 'An IPv4 or IPv6 address', - placeholder: 'Enter an 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', - placeholder: 'Enter an IP network', } } } @@ -156,7 +151,7 @@ const DynamicTypeAndValueFields = ({ {/* In the firewall rules form, these types get comboboxes instead of text fields */} {['vpc', 'subnet', 'instance'].includes(valueType) ? ( void }) => { void }) { /> void }) { /> Date: Wed, 4 Sep 2024 13:32:04 -0700 Subject: [PATCH 31/42] Refactoring --- app/components/form/fields/ComboboxField.tsx | 10 ++++++---- app/forms/disk-attach.tsx | 1 - app/forms/firewall-rules-common.tsx | 10 ++++++---- app/forms/snapshot-create.tsx | 1 - app/pages/system/SiloImagesPage.tsx | 1 - 5 files changed, 12 insertions(+), 11 deletions(-) diff --git a/app/components/form/fields/ComboboxField.tsx b/app/components/form/fields/ComboboxField.tsx index 4be1afdf1e..df580fdd5a 100644 --- a/app/components/form/fields/ComboboxField.tsx +++ b/app/components/form/fields/ComboboxField.tsx @@ -41,10 +41,12 @@ export function ComboboxField< allowArbitraryValues, placeholder, // Intent is to not show both a placeholder and a description, while still having good defaults; prefer a description to a placeholder - // If description is provided, use it - // If not, but a placeholder is provided, the default description should be undefined - // If no placeholder is provided and arbitrary values are allowed, the default description should be 'Select an option or enter a custom value' - // If no placeholder is provided and arbitrary values are not allowed, the default description should be 'Select an option' + /* + * If description is provided, use it. + * If not, but a placeholder is provided, the default description should be undefined. + * If no placeholder is provided and arbitrary values are allowed, the default description should be 'Select an option or enter a custom value'. + * If no placeholder is provided and arbitrary values are not allowed, the default description should be 'Select an option'. + */ description = placeholder ? undefined : allowArbitraryValues diff --git a/app/forms/disk-attach.tsx b/app/forms/disk-attach.tsx index b5ab3b22d3..7895c81b91 100644 --- a/app/forms/disk-attach.tsx +++ b/app/forms/disk-attach.tsx @@ -57,7 +57,6 @@ export function AttachDiskSideModalForm({ ({ value: name, label: name }))} required diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 97c09b9e2d..4edc950ed5 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -286,14 +286,16 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = const targetAndHostDefaultValues: TargetAndHostFormValues = { type: 'vpc', value: '' } const { project } = useProjectSelector() // prefetchedApiQueries below are prefetched in firewall-rules-create and -edit - const { data: instancesData } = usePrefetchedApiQuery('instanceList', { + const { + data: { items: instances }, + } = usePrefetchedApiQuery('instanceList', { query: { project, limit: PAGE_SIZE }, }) - const instances = instancesData?.items ?? [] - const { data: vpcData } = usePrefetchedApiQuery('vpcList', { + const { + data: { items: vpcs }, + } = usePrefetchedApiQuery('vpcList', { query: { project, limit: PAGE_SIZE }, }) - const vpcs = vpcData?.items ?? [] // Targets const targetForm = useForm({ defaultValues: targetAndHostDefaultValues }) diff --git a/app/forms/snapshot-create.tsx b/app/forms/snapshot-create.tsx index 50514aea83..0080251420 100644 --- a/app/forms/snapshot-create.tsx +++ b/app/forms/snapshot-create.tsx @@ -77,7 +77,6 @@ export function CreateSnapshotSideModalForm() { label="Disk" name="disk" placeholder="Select a disk" - description="" items={diskItems} required control={form.control} diff --git a/app/pages/system/SiloImagesPage.tsx b/app/pages/system/SiloImagesPage.tsx index d2611830cd..c8f92d5937 100644 --- a/app/pages/system/SiloImagesPage.tsx +++ b/app/pages/system/SiloImagesPage.tsx @@ -170,7 +170,6 @@ const PromoteImageModal = ({ onDismiss }: { onDismiss: () => void }) => { Date: Wed, 4 Sep 2024 14:53:22 -0700 Subject: [PATCH 32/42] more cleanup; not totally sold on microcopy on firewall rule combobox --- app/forms/firewall-rules-common.tsx | 33 +++++++++++++++++------------ app/forms/firewall-rules-edit.tsx | 16 ++++++-------- 2 files changed, 26 insertions(+), 23 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 4edc950ed5..179ca564ea 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -70,22 +70,13 @@ type TargetAndHostFormValues = { const getFilterValueProps = (targetOrHostType: TargetAndHostFilterType) => { switch (targetOrHostType) { case 'vpc': - return { - label: 'VPC name', - } + return { label: 'VPC name' } case 'subnet': - return { - label: 'Subnet name', - } + return { label: 'Subnet name' } case 'instance': - return { - label: 'Instance name', - } + return { label: 'Instance name' } case 'ip': - return { - label: 'IP address', - description: 'Enter an IPv4 or IPv6 address', - } + return { label: 'IP address', description: 'Enter an IPv4 or IPv6 address' } case 'ip_net': return { label: 'IP network', @@ -116,6 +107,13 @@ const DynamicTypeAndValueFields = ({ onSubmitTextField: (e: React.KeyboardEvent) => void }) => { const queryClient = useApiQueryClient() + const valueTypeLabel = { + vpc: 'a VPC', + subnet: 'a VPC subnet', + instance: 'an instance', + ip: 'an IP', + ip_net: 'an IP subnet', + }[valueType] return ( <> + Select an option or enter a custom value;{' '} + {sectionType === 'target' ? 'target' : 'host filter'} can match{' '} + {valueTypeLabel} created in the future + + } control={control} onInputChange={onInputChange} items={items} diff --git a/app/forms/firewall-rules-edit.tsx b/app/forms/firewall-rules-edit.tsx index 8d7f41818c..f7f65f042f 100644 --- a/app/forms/firewall-rules-edit.tsx +++ b/app/forms/firewall-rules-edit.tsx @@ -33,19 +33,17 @@ import { valuesToRuleUpdate, type FirewallRuleValues } from './firewall-rules-ut EditFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { const { project, vpc, rule } = getFirewallRuleSelector(params) - const firewallRules = await apiQueryClient.fetchQuery('vpcFirewallRulesView', { - query: { project, vpc }, - }) - const originalRule = firewallRules.rules.find((r) => r.name === rule) - if (!originalRule) throw trigger404 - - await Promise.all([ - apiQueryClient.prefetchQuery('instanceList', { - query: { project, limit: PAGE_SIZE }, + const [firewallRules] = await Promise.all([ + apiQueryClient.fetchQuery('vpcFirewallRulesView', { + query: { project, vpc }, }), + apiQueryClient.prefetchQuery('instanceList', { query: { project, limit: PAGE_SIZE } }), apiQueryClient.prefetchQuery('vpcList', { query: { project, limit: PAGE_SIZE } }), ]) + const originalRule = firewallRules.rules.find((r) => r.name === rule) + if (!originalRule) throw trigger404 + return null } From c29cdf1194a87115e549b301d89ade1b7cc74a47 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Wed, 4 Sep 2024 14:59:43 -0700 Subject: [PATCH 33/42] Revert placeholder in two spots --- app/pages/system/networking/IpPoolPage.tsx | 1 + app/pages/system/silos/SiloIpPoolsTab.tsx | 1 + 2 files changed, 2 insertions(+) diff --git a/app/pages/system/networking/IpPoolPage.tsx b/app/pages/system/networking/IpPoolPage.tsx index 78628b8a66..1c77e114d7 100644 --- a/app/pages/system/networking/IpPoolPage.tsx +++ b/app/pages/system/networking/IpPoolPage.tsx @@ -369,6 +369,7 @@ function LinkSiloModal({ onDismiss }: { onDismiss: () => void }) { /> void }) { /> Date: Wed, 4 Sep 2024 17:27:30 -0700 Subject: [PATCH 34/42] fix capitalization on test --- test/e2e/firewall-rules.e2e.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index c040e47490..33ce62cbaf 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -144,7 +144,7 @@ test('firewall rule targets and filters overflow', async ({ page }) => { const setupSubnetSelection = async (page: Page, sectionType: 'Host' | 'Target') => { // first by selecting from a dropdown - await selectOption(page, `${sectionType} type`, 'VPC Subnet') + await selectOption(page, `${sectionType} type`, 'VPC subnet') // select the VPC so you can then add a subnet at the callsite await selectOption(page, 'VPC Select a VPC', 'mock-vpc') } @@ -177,7 +177,7 @@ test('firewall rule form targets table', async ({ page }) => { await addButton.click() await expectRowVisible(targets, { Type: 'vpc', Value: 'def' }) - // add VPC Subnet targets + // add VPC subnet targets const subnetNameField = page.getByRole('combobox', { name: 'Subnet name' }) // add a subnet by selecting from a dropdown From bf7b3c54b59849c18dc97f915c13ebe61dccc1ed Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Thu, 5 Sep 2024 10:30:59 -0700 Subject: [PATCH 35/42] Default to current VPC in VPC subnet form --- app/components/form/fields/useItemsList.ts | 7 ++--- app/forms/firewall-rules-common.tsx | 32 ++++++++-------------- 2 files changed, 15 insertions(+), 24 deletions(-) diff --git a/app/components/form/fields/useItemsList.ts b/app/components/form/fields/useItemsList.ts index 78488ca46c..6603393a13 100644 --- a/app/components/form/fields/useItemsList.ts +++ b/app/components/form/fields/useItemsList.ts @@ -43,12 +43,11 @@ export const useCustomRouterItems = () => { return { isLoading, items: routerItems } } -// Get a list of subnets for a given VPC; If no project/VPC is provided, use the VPC from the VpcSelector -export const useVpcSubnets = ({ project, vpc }: { project?: string; vpc?: string }) => { - const vpcSelector = useVpcSelector() +/** Get a list of subnets for a given VPC */ +export const useVpcSubnets = ({ project, vpc }: { project: string; vpc: string }) => { const { isLoading, data } = useApiQuery( 'vpcSubnetList', - { query: { project: project || vpcSelector.project, vpc: vpc || vpcSelector.vpc } }, + { query: { project, vpc } }, { enabled: !!vpc } ) return { isLoading, items: data?.items } diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 179ca564ea..0fcb89bd5e 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -28,7 +28,7 @@ import { NumberField } from '~/components/form/fields/NumberField' import { RadioField } from '~/components/form/fields/RadioField' import { TextField, TextFieldInner } from '~/components/form/fields/TextField' import { useVpcSubnets } from '~/components/form/fields/useItemsList' -import { useProjectSelector } from '~/hooks' +import { useVpcSelector } from '~/hooks' import { useForm } from '~/hooks/use-form' import { PAGE_SIZE } from '~/table/QueryTable' import { Badge } from '~/ui/lib/Badge' @@ -107,13 +107,6 @@ const DynamicTypeAndValueFields = ({ onSubmitTextField: (e: React.KeyboardEvent) => void }) => { const queryClient = useApiQueryClient() - const valueTypeLabel = { - vpc: 'a VPC', - subnet: 'a VPC subnet', - instance: 'an instance', - ip: 'an IP', - ip_net: 'an IP subnet', - }[valueType] return ( <> - Select an option or enter a custom value;{' '} - {sectionType === 'target' ? 'target' : 'host filter'} can match{' '} - {valueTypeLabel} created in the future - - } + description="Select an option or enter a custom value" control={control} onInputChange={onInputChange} items={items} @@ -288,8 +275,13 @@ type CommonFieldsProps = { } export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) => { - const targetAndHostDefaultValues: TargetAndHostFormValues = { type: 'vpc', value: '' } - const { project } = useProjectSelector() + const { project, vpc } = useVpcSelector() + const targetAndHostDefaultValues: TargetAndHostFormValues = { + type: 'vpc', + value: '', + // only becomes relevant when the type is 'VPC subnet'; ignored otherwise + subnetVpc: vpc, + } // prefetchedApiQueries below are prefetched in firewall-rules-create and -edit const { data: { items: instances }, @@ -307,7 +299,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = const targets = useController({ name: 'targets', control }).field const targetType = targetForm.watch('type') const targetValue = targetForm.watch('value') - const targetSubnetVpc = targetForm.watch('subnetVpc') + const targetSubnetVpc = targetForm.watch('subnetVpc') || vpc // get the list of subnets for the specific VPC selected in the form const { items: targetVpcSubnets } = useVpcSubnets({ project, vpc: targetSubnetVpc }) // get the list of items that are not already in the list of targets @@ -340,12 +332,12 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = portRangeForm.reset() }) - // Hosts + // Host Filters const hostForm = useForm({ defaultValues: targetAndHostDefaultValues }) const hosts = useController({ name: 'hosts', control }).field const hostType = hostForm.watch('type') const hostValue = hostForm.watch('value') - const hostSubnetVpc = hostForm.watch('subnetVpc') + const hostSubnetVpc = hostForm.watch('subnetVpc') || vpc // get the list of subnets for the specific PC selected in the form const { items: hostVpcSubnets } = useVpcSubnets({ project, vpc: hostSubnetVpc }) // get the list of items that are not already in the list of host filters From 107371e9395a5a819a44c2080578cf6303a4cce1 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Thu, 5 Sep 2024 10:48:35 -0700 Subject: [PATCH 36/42] Fix tests --- test/e2e/firewall-rules.e2e.ts | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index 33ce62cbaf..a1595019ab 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -13,7 +13,6 @@ import { selectOption, test, type Locator, - type Page, } from './utils' const defaultRules = ['allow-internal-inbound', 'allow-ssh', 'allow-icmp'] @@ -142,13 +141,6 @@ test('firewall rule targets and filters overflow', async ({ page }) => { await expect(tooltip).toBeVisible() }) -const setupSubnetSelection = async (page: Page, sectionType: 'Host' | 'Target') => { - // first by selecting from a dropdown - await selectOption(page, `${sectionType} type`, 'VPC subnet') - // select the VPC so you can then add a subnet at the callsite - await selectOption(page, 'VPC Select a VPC', 'mock-vpc') -} - const deleteRowAndVerifyRowCount = async (table: Locator, expectedCount: number) => { const rows = table.getByRole('row') // skip the header row @@ -181,13 +173,13 @@ test('firewall rule form targets table', async ({ page }) => { const subnetNameField = page.getByRole('combobox', { name: 'Subnet name' }) // add a subnet by selecting from a dropdown - await setupSubnetSelection(page, 'Target') + await selectOption(page, 'Target type', 'VPC subnet') await selectOption(page, subnetNameField, 'mock-subnet') await addButton.click() await expectRowVisible(targets, { Type: 'subnet', Value: 'mock-subnet' }) // now add a subnet by entering text - await setupSubnetSelection(page, 'Target') + await selectOption(page, 'Target type', 'VPC subnet') await subnetNameField.fill('abc') await addButton.click() await expectRowVisible(targets, { Type: 'subnet', Value: 'abc' }) @@ -231,12 +223,12 @@ test('firewall rule form hosts table', async ({ page }) => { await addButton.click() await expectRowVisible(hosts, { Type: 'vpc', Value: 'def' }) - await setupSubnetSelection(page, 'Host') + 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 setupSubnetSelection(page, 'Host') + await selectOption(page, 'Host type', 'VPC subnet') await page.getByRole('combobox', { name: 'Subnet name' }).fill('abc') await addButton.click() await expectRowVisible(hosts, { Type: 'subnet', Value: 'abc' }) @@ -299,7 +291,7 @@ test('can update firewall rule', async ({ page }) => { await page.getByRole('textbox', { name: 'Name' }).fill('new-rule-name') // add host filter - await setupSubnetSelection(page, 'Host') + await selectOption(page, 'Host type', 'VPC subnet') await page.getByRole('combobox', { name: 'Subnet name' }).fill('edit-filter-subnet') await page.getByRole('button', { name: 'Add host filter' }).click() From e48a3af10d2582c1d3a7077a8246c0a01f271190 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 6 Sep 2024 17:28:16 -0700 Subject: [PATCH 37/42] Remove VPC selection step from VPC Subnet targeting; refactor and add tests --- app/components/form/fields/ComboboxField.tsx | 1 - app/components/form/fields/useItemsList.ts | 10 --- app/forms/firewall-rules-common.tsx | 67 +++++++------------- app/forms/firewall-rules-create.tsx | 5 +- app/forms/firewall-rules-edit.tsx | 1 + app/ui/lib/Combobox.tsx | 22 +++---- test/e2e/firewall-rules.e2e.ts | 8 ++- 7 files changed, 41 insertions(+), 73 deletions(-) diff --git a/app/components/form/fields/ComboboxField.tsx b/app/components/form/fields/ComboboxField.tsx index df580fdd5a..3b9ee63f51 100644 --- a/app/components/form/fields/ComboboxField.tsx +++ b/app/components/form/fields/ComboboxField.tsx @@ -25,7 +25,6 @@ export type ComboboxFieldProps< name: TName control: Control onChange?: (value: string | null | undefined) => void - allowArbitraryValues?: boolean } & ComboboxBaseProps export function ComboboxField< diff --git a/app/components/form/fields/useItemsList.ts b/app/components/form/fields/useItemsList.ts index 6603393a13..f364899c84 100644 --- a/app/components/form/fields/useItemsList.ts +++ b/app/components/form/fields/useItemsList.ts @@ -42,13 +42,3 @@ export const useCustomRouterItems = () => { return { isLoading, items: routerItems } } - -/** Get a list of subnets for a given VPC */ -export const useVpcSubnets = ({ project, vpc }: { project: string; vpc: string }) => { - const { isLoading, data } = useApiQuery( - 'vpcSubnetList', - { query: { project, vpc } }, - { enabled: !!vpc } - ) - return { isLoading, items: data?.items } -} diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 0fcb89bd5e..09983ff4c7 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -9,7 +9,6 @@ import { useController, type Control, type ControllerRenderProps } from 'react-hook-form' import { - useApiQueryClient, usePrefetchedApiQuery, type ApiError, type Instance, @@ -27,13 +26,11 @@ import { NameField } 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 { useVpcSubnets } from '~/components/form/fields/useItemsList' import { useVpcSelector } from '~/hooks' import { useForm } from '~/hooks/use-form' import { PAGE_SIZE } from '~/table/QueryTable' import { Badge } from '~/ui/lib/Badge' import { Button } from '~/ui/lib/Button' -import { toComboboxItem } from '~/ui/lib/Combobox' import { FormDivider } from '~/ui/lib/Divider' import { Message } from '~/ui/lib/Message' import * as MiniTable from '~/ui/lib/MiniTable' @@ -90,7 +87,6 @@ const DynamicTypeAndValueFields = ({ control, valueType, items, - vpcs, isDisabled, onInputChange, onTypeChange, @@ -100,13 +96,11 @@ const DynamicTypeAndValueFields = ({ control: Control valueType: TargetAndHostFilterType items: Array<{ value: string; label: string }> - vpcs: Array isDisabled?: boolean onInputChange?: (value: string) => void onTypeChange: () => void onSubmitTextField: (e: React.KeyboardEvent) => void }) => { - const queryClient = useApiQueryClient() return ( <> - {/* If specifying a subnet, they must first select the VPC that owns the subnet */} - {valueType === 'subnet' && ( - toComboboxItem(v.name))} - // when this changes, we need to re-fetch the subnet list - onChange={() => { - queryClient.invalidateQueries('vpcSubnetList') - }} - placeholder="Select a VPC" - showOptionalTag={false} - /> - )} - {/* In the firewall rules form, these types get comboboxes instead of text fields */} + {/* In the firewall rules form, a few types get comboboxes instead of text fields */} {['vpc', 'subnet', 'instance'].includes(valueType) ? ( ( // a list of all items, return the items that are available const availableItems = ( committedItems: Array, - // Items is conditional because VPC Subnet fetching isn't 100% guaranteed - items?: Array + itemType: 'vpc' | 'subnet' | 'instance', + items: Array ) => { if (!items) return [] - return items - .map((item) => item.name) - .filter((name) => !committedItems.map((ci) => ci.value).includes(name)) - .map((name) => toComboboxItem(name)) + return ( + items + .map((item) => item.name) + // remove any items that match the committed items on both type and value + .filter( + (name) => + !committedItems.filter((ci) => ci.type === itemType && ci.value === name).length + ) + .map((name) => ({ label: name, value: name })) + ) } type ProtocolFieldProps = { @@ -293,20 +277,20 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = } = usePrefetchedApiQuery('vpcList', { query: { project, limit: PAGE_SIZE }, }) + const { + data: { items: vpcSubnets }, + } = usePrefetchedApiQuery('vpcSubnetList', { query: { project, vpc } }) // Targets const targetForm = useForm({ defaultValues: targetAndHostDefaultValues }) const targets = useController({ name: 'targets', control }).field const targetType = targetForm.watch('type') const targetValue = targetForm.watch('value') - const targetSubnetVpc = targetForm.watch('subnetVpc') || vpc - // get the list of subnets for the specific VPC selected in the form - const { items: targetVpcSubnets } = useVpcSubnets({ project, vpc: targetSubnetVpc }) // get the list of items that are not already in the list of targets const targetItems = { - vpc: availableItems(targets.value, vpcs), - subnet: availableItems(targets.value, targetVpcSubnets), - instance: availableItems(targets.value, instances), + vpc: availableItems(targets.value, 'vpc', vpcs), + subnet: availableItems(targets.value, 'subnet', vpcSubnets), + instance: availableItems(targets.value, 'instance', instances), ip: [], ip_net: [], } @@ -337,14 +321,11 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = const hosts = useController({ name: 'hosts', control }).field const hostType = hostForm.watch('type') const hostValue = hostForm.watch('value') - const hostSubnetVpc = hostForm.watch('subnetVpc') || vpc - // get the list of subnets for the specific PC selected in the form - const { items: hostVpcSubnets } = useVpcSubnets({ project, vpc: hostSubnetVpc }) // get the list of items that are not already in the list of host filters const hostFilterItems = { - vpc: availableItems(hosts.value, vpcs), - subnet: availableItems(hosts.value, hostVpcSubnets), - instance: availableItems(hosts.value, instances), + vpc: availableItems(hosts.value, 'vpc', vpcs), + subnet: availableItems(hosts.value, 'subnet', vpcSubnets), + instance: availableItems(hosts.value, 'instance', instances), ip: [], ip_net: [], } @@ -459,8 +440,6 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = control={targetForm.control} valueType={targetType} items={targetItems[targetType]} - vpcs={vpcs} - isDisabled={targetType === 'subnet' && !targetSubnetVpc} onTypeChange={() => targetForm.setValue('value', '')} onInputChange={(value) => targetForm.setValue('value', value)} onSubmitTextField={submitTarget} @@ -568,8 +547,6 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = control={hostForm.control} valueType={hostType} items={hostFilterItems[hostType]} - vpcs={vpcs} - isDisabled={hostType === 'subnet' && !hostSubnetVpc} onTypeChange={() => hostForm.setValue('value', '')} onInputChange={(value) => hostForm.setValue('value', value)} onSubmitTextField={submitHost} diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index acc3523c1a..78cb97ed44 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -19,7 +19,7 @@ import { } from '@oxide/api' import { SideModalForm } from '~/components/form/SideModalForm' -import { getProjectSelector, getVpcSelector, useForm, useVpcSelector } from '~/hooks' +import { getVpcSelector, useForm, useVpcSelector } from '~/hooks' import { addToast } from '~/stores/toast' import { PAGE_SIZE } from '~/table/QueryTable' import { pb } from '~/util/path-builder' @@ -56,7 +56,7 @@ const ruleToValues = (rule: VpcFirewallRule): FirewallRuleValues => ({ }) CreateFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { - const { project } = getProjectSelector(params) + const { project, vpc } = getVpcSelector(params) await Promise.all([ apiQueryClient.prefetchQuery('vpcFirewallRulesView', { query: getVpcSelector(params), @@ -65,6 +65,7 @@ CreateFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { query: { project, limit: PAGE_SIZE }, }), apiQueryClient.prefetchQuery('vpcList', { query: { project, limit: PAGE_SIZE } }), + apiQueryClient.prefetchQuery('vpcSubnetList', { query: { project, vpc } }), ]) return null diff --git a/app/forms/firewall-rules-edit.tsx b/app/forms/firewall-rules-edit.tsx index f7f65f042f..90d311d8f6 100644 --- a/app/forms/firewall-rules-edit.tsx +++ b/app/forms/firewall-rules-edit.tsx @@ -39,6 +39,7 @@ EditFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { }), apiQueryClient.prefetchQuery('instanceList', { query: { project, limit: PAGE_SIZE } }), apiQueryClient.prefetchQuery('vpcList', { query: { project, limit: PAGE_SIZE } }), + apiQueryClient.prefetchQuery('vpcSubnetList', { query: { project, vpc } }), ]) const originalRule = firewallRules.rules.find((r) => r.name === rule) diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index ed7a15a74a..812a754966 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -24,31 +24,29 @@ import { FieldLabel } from './FieldLabel' import { usePopoverZIndex } from './SideModal' import { TextInputHint } from './TextInput' -export type ComboboxItem = { label: string; value: string } - -export const toComboboxItem = (value: string, label?: string): ComboboxItem => ({ - label: label || value, - value, -}) - /** Simple non-generic props shared with ComboboxField */ export type ComboboxBaseProps = { description?: React.ReactNode isDisabled?: boolean isLoading?: boolean - items: ComboboxItem[] + items: Array<{ label: string; value: string }> label: string placeholder?: string required?: boolean tooltipText?: string - onInputChange?: (value: string) => void + ariaLabel?: string + showOptionalTag?: boolean /** - * pass in allowArbitraryValues as `true` when the user should be able to + * Pass in `allowArbitraryValues` as `true` when the user should be able to * type in new values that aren't in the list [default is `false`] */ allowArbitraryValues?: boolean - ariaLabel?: string - showOptionalTag?: boolean + /** + * Pass in `onInputChange` when an event should be triggered when the user types in the field; + * This is distinct from `onChange` which is triggered when the user selects an item from the list. + * For example, if a form value should be set when the user types, use `onInputChange`. + */ + onInputChange?: (value: string) => void } type ComboboxProps = { diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index a1595019ab..7046fedbd8 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -165,14 +165,16 @@ test('firewall rule form targets table', async ({ page }) => { await addButton.click() await expectRowVisible(targets, { Type: 'vpc', Value: 'abc' }) - await targetVpcNameField.fill('def') + // enter a VPC called 'mock-subnet', even if that doesn't make sense here, to test dropdown later + await targetVpcNameField.fill('mock-subnet') await addButton.click() - await expectRowVisible(targets, { Type: 'vpc', Value: 'def' }) + 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 + // 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') await selectOption(page, subnetNameField, 'mock-subnet') await addButton.click() From 5e515cbf4e28a33386f3492485356ce71584aa59 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 6 Sep 2024 17:47:28 -0700 Subject: [PATCH 38/42] Update logic around show/hideOptionalTag --- app/components/form/fields/ListboxField.tsx | 6 +++--- app/forms/firewall-rules-common.tsx | 4 ++-- app/ui/lib/Combobox.tsx | 7 +++---- app/ui/lib/FieldLabel.tsx | 6 +++--- app/ui/lib/Listbox.tsx | 7 +++---- 5 files changed, 14 insertions(+), 16 deletions(-) diff --git a/app/components/form/fields/ListboxField.tsx b/app/components/form/fields/ListboxField.tsx index da7bb53fbf..38e644acb6 100644 --- a/app/components/form/fields/ListboxField.tsx +++ b/app/components/form/fields/ListboxField.tsx @@ -35,7 +35,7 @@ export type ListboxFieldProps< onChange?: (value: string | null | undefined) => void isLoading?: boolean noItemsPlaceholder?: string - showOptionalTag?: boolean + hideOptionalTag?: boolean } export function ListboxField< @@ -55,7 +55,7 @@ export function ListboxField< onChange, isLoading, noItemsPlaceholder, - showOptionalTag, + hideOptionalTag, }: ListboxFieldProps) { // TODO: recreate this logic // validate: (v) => (required && !v ? `${name} is required` : undefined), @@ -82,7 +82,7 @@ export function ListboxField< hasError={fieldState.error !== undefined} isLoading={isLoading} buttonRef={field.ref} - showOptionalTag={showOptionalTag} + hideOptionalTag={hideOptionalTag} />
diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 09983ff4c7..840329b81c 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -115,7 +115,7 @@ const DynamicTypeAndValueFields = ({ { value: 'ip_net', label: 'IP subnet' }, ]} onChange={onTypeChange} - showOptionalTag={false} + hideOptionalTag /> {/* In the firewall rules form, a few types get comboboxes instead of text fields */} {['vpc', 'subnet', 'instance'].includes(valueType) ? ( @@ -128,7 +128,7 @@ const DynamicTypeAndValueFields = ({ onInputChange={onInputChange} items={items} allowArbitraryValues - showOptionalTag={false} + hideOptionalTag // TODO: validate here, but it's complicated because it's conditional // on which type is selected /> diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index 812a754966..6850320600 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -35,7 +35,7 @@ export type ComboboxBaseProps = { required?: boolean tooltipText?: string ariaLabel?: string - showOptionalTag?: boolean + hideOptionalTag?: boolean /** * Pass in `allowArbitraryValues` as `true` when the user should be able to * type in new values that aren't in the list [default is `false`] @@ -70,7 +70,7 @@ export const Combobox = ({ onInputChange, allowArbitraryValues = false, ariaLabel, - showOptionalTag, + hideOptionalTag, }: ComboboxProps) => { const [query, setQuery] = useState(selected || '') @@ -99,8 +99,7 @@ export const Combobox = ({ id="FieldLabel" as="div" tip={tooltipText} - optional={!required} - showOptionalTag={showOptionalTag} + optional={!required && !hideOptionalTag} > diff --git a/app/ui/lib/FieldLabel.tsx b/app/ui/lib/FieldLabel.tsx index 5051b40be1..4a06d80e39 100644 --- a/app/ui/lib/FieldLabel.tsx +++ b/app/ui/lib/FieldLabel.tsx @@ -17,7 +17,7 @@ interface FieldLabelProps { tip?: string optional?: boolean className?: string - showOptionalTag?: boolean + hideOptionalTag?: boolean } export const FieldLabel = ({ @@ -28,14 +28,14 @@ export const FieldLabel = ({ optional, as, className, - showOptionalTag = true, + hideOptionalTag = true, }: PropsWithChildren) => { const Component = as || 'label' return (
{children} - {optional && showOptionalTag && ( + {optional && !hideOptionalTag && ( // Announcing this optional text is unnecessary as the required attribute on the // form will be used