From 62844aea85a482d7d3fb301f3da7e877c4244083 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Mon, 6 Jan 2025 13:56:39 -0800 Subject: [PATCH 1/3] Add logic to prevent form abandonment --- app/forms/firewall-rules-common.tsx | 38 +++++++++++++++++++++++++---- app/forms/firewall-rules-create.tsx | 25 +++++++++++++++++++ 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 809799b722..bd5d79a8ce 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -42,6 +42,7 @@ import { validateIp, validateIpNet } from '~/util/ip' import { links } from '~/util/links' import { capitalize } from '~/util/str' +import { type ActiveSubforms } from './firewall-rules-create' import { type FirewallRuleValues } from './firewall-rules-util' /** @@ -88,10 +89,12 @@ const TargetAndHostFilterSubform = ({ sectionType, control, messageContent, + updateSubformStates, }: { sectionType: 'target' | 'host' control: Control messageContent: ReactNode + updateSubformStates: (subform: keyof ActiveSubforms, value: boolean) => void }) => { const { project, vpc } = useVpcSelector() // prefetchedApiQueries below are prefetched in firewall-rules-create and -edit @@ -125,8 +128,11 @@ const TargetAndHostFilterSubform = ({ // https://github.com/react-hook-form/react-hook-form/blob/9a497a70a/src/logic/createFormControl.ts#L1194-L1203 const { isSubmitSuccessful: subformSubmitSuccessful } = subform.formState useEffect(() => { - if (subformSubmitSuccessful) subform.reset(targetAndHostDefaultValues) - }, [subformSubmitSuccessful, subform]) + if (subformSubmitSuccessful) { + subform.reset(targetAndHostDefaultValues) + updateSubformStates(sectionType, false) + } + }, [subformSubmitSuccessful, subform, updateSubformStates, sectionType]) const [valueType, value] = subform.watch(['type', 'value']) const sectionItems = { @@ -143,9 +149,15 @@ const TargetAndHostFilterSubform = ({ // back to validating on submit instead of change. Also resets readyToSubmit. const onTypeChange = () => { subform.reset({ type: subform.getValues('type'), value: '' }) + updateSubformStates(sectionType, false) } const onInputChange = (value: string) => { subform.setValue('value', value) + updateSubformStates(sectionType, value.length > 0) + } + const onClear = () => { + subform.reset() + updateSubformStates(sectionType, false) } return ( @@ -178,6 +190,7 @@ const TargetAndHostFilterSubform = ({ description="Select an option or enter a custom value" control={subformControl} onEnter={submitSubform} + onChange={() => updateSubformStates(sectionType, true)} onInputChange={onInputChange} items={items} allowArbitraryValues @@ -212,7 +225,7 @@ const TargetAndHostFilterSubform = ({ subform.reset()} + onClear={onClear} onSubmit={submitSubform} /> {field.value.length > 0 && ( @@ -289,6 +302,7 @@ type CommonFieldsProps = { control: Control nameTaken: (name: string) => boolean error: ApiError | null + updateSubformStates: (subform: keyof ActiveSubforms, value: boolean) => void } const targetAndHostDefaultValues: TargetAndHostFormValues = { @@ -296,7 +310,12 @@ const targetAndHostDefaultValues: TargetAndHostFormValues = { value: '', } -export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) => { +export const CommonFields = ({ + control, + nameTaken, + error, + updateSubformStates, +}: CommonFieldsProps) => { // Ports const portRangeForm = useForm({ defaultValues: { portRange: '' } }) const ports = useController({ name: 'ports', control }).field @@ -307,7 +326,11 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = // that it is not already in the list ports.onChange([...ports.value, portRangeValue]) portRangeForm.reset() + updateSubformStates('port', false) }) + useEffect(() => { + updateSubformStates('port', portValue.length > 0) + }, [updateSubformStates, portValue]) return ( <> @@ -406,6 +429,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =

} + updateSubformStates={updateSubformStates} /> @@ -453,7 +477,10 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = portRangeForm.reset()} + onClear={() => { + portRangeForm.reset() + updateSubformStates('port', false) + }} onSubmit={submitPortRange} /> @@ -500,6 +527,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = traffic. For an outbound rule, they match the destination. } + updateSubformStates={updateSubformStates} /> {error && ( diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index df1b74642e..a7ee28e078 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -5,6 +5,7 @@ * * Copyright Oxide Computer Company */ +import { useState } from 'react' import { useForm } from 'react-hook-form' import { useNavigate, useParams, type LoaderFunctionArgs } from 'react-router' @@ -23,6 +24,7 @@ import { getVpcSelector, useVpcSelector } from '~/hooks/use-params' import { addToast } from '~/stores/toast' import { ALL_ISH } from '~/util/consts' import { pb } from '~/util/path-builder' +import { commaSeries } from '~/util/str' import { CommonFields } from './firewall-rules-common' import { valuesToRuleUpdate, type FirewallRuleValues } from './firewall-rules-util' @@ -69,7 +71,24 @@ CreateFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { return null } +export type ActiveSubforms = { target: boolean; port: boolean; host: boolean } +const defaultActiveSubforms: ActiveSubforms = { target: false, port: false, host: false } + export function CreateFirewallRuleForm() { + const [subformStates, setSubformStates] = useState(defaultActiveSubforms) + const updateSubformStates = (subform: keyof ActiveSubforms, value: boolean) => { + setSubformStates({ + ...subformStates, + [subform]: value, + }) + } + const activeSubformList = commaSeries( + Object.keys(subformStates).filter((key) => subformStates[key as keyof ActiveSubforms]), + 'and' + ) + .replace('port', 'port filter') + .replace('host', 'host filter') + const vpcSelector = useVpcSelector() const queryClient = useApiQueryClient() @@ -120,12 +139,18 @@ export function CreateFirewallRuleForm() { loading={updateRules.isPending} submitError={updateRules.error} submitLabel="Add rule" + submitDisabled={ + activeSubformList.length + ? `You have an unsaved ${activeSubformList} entry; save or clear ${activeSubformList.includes('and') ? 'them' : 'it'} to create this firewall rule` + : undefined + } > !!existingRules.find((r) => r.name === name)} error={updateRules.error} + updateSubformStates={updateSubformStates} // 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 From 7f68453621ea6fcd0c3b44f188d7fb7ada015242 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Mon, 6 Jan 2025 16:02:01 -0800 Subject: [PATCH 2/3] Move logic into common file --- app/forms/firewall-rules-common.tsx | 35 ++++++++++++++++++++++++++--- app/forms/firewall-rules-create.tsx | 32 +++++++------------------- app/forms/firewall-rules-edit.tsx | 11 ++++++++- 3 files changed, 50 insertions(+), 28 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index bd5d79a8ce..176ec2afcb 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -6,7 +6,7 @@ * Copyright Oxide Computer Company */ -import { useEffect, type ReactNode } from 'react' +import { useEffect, useState, type ReactNode } from 'react' import { useController, useForm, type Control } from 'react-hook-form' import { @@ -40,9 +40,8 @@ import { KEYS } from '~/ui/util/keys' import { ALL_ISH } from '~/util/consts' import { validateIp, validateIpNet } from '~/util/ip' import { links } from '~/util/links' -import { capitalize } from '~/util/str' +import { capitalize, commaSeries } from '~/util/str' -import { type ActiveSubforms } from './firewall-rules-create' import { type FirewallRuleValues } from './firewall-rules-util' /** @@ -539,3 +538,33 @@ export const CommonFields = ({ ) } + +export type ActiveSubforms = { target: boolean; port: boolean; host: boolean } +export const defaultActiveSubforms: ActiveSubforms = { + target: false, + port: false, + host: false, +} + +export function useSubformStates(defaultActiveSubforms: ActiveSubforms) { + const [subformStates, setSubformStates] = useState(defaultActiveSubforms) + const updateSubformStates = (subform: keyof ActiveSubforms, value: boolean) => { + setSubformStates((prev) => ({ ...prev, [subform]: value })) + } + return { subformStates, updateSubformStates } +} + +export const getActiveSubformList = (subformStates: ActiveSubforms) => + commaSeries( + Object.keys(subformStates).filter((key) => subformStates[key as keyof ActiveSubforms]), + 'and' + ) + .replace('port', 'port filter') + .replace('host', 'host filter') + +export const submitDisabledMessage = (subformStates: ActiveSubforms) => { + const activeSubformList = getActiveSubformList(subformStates) + return activeSubformList.length > 0 + ? `You have an unsaved ${activeSubformList} entry; save or clear ${activeSubformList.includes('and') ? 'them' : 'it'} to create this firewall rule` + : undefined +} diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index a7ee28e078..0c500b803b 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -5,7 +5,6 @@ * * Copyright Oxide Computer Company */ -import { useState } from 'react' import { useForm } from 'react-hook-form' import { useNavigate, useParams, type LoaderFunctionArgs } from 'react-router' @@ -24,9 +23,13 @@ import { getVpcSelector, useVpcSelector } from '~/hooks/use-params' import { addToast } from '~/stores/toast' import { ALL_ISH } from '~/util/consts' import { pb } from '~/util/path-builder' -import { commaSeries } from '~/util/str' -import { CommonFields } from './firewall-rules-common' +import { + CommonFields, + defaultActiveSubforms, + submitDisabledMessage, + useSubformStates, +} from './firewall-rules-common' import { valuesToRuleUpdate, type FirewallRuleValues } from './firewall-rules-util' /** Empty form for when we're not creating from an existing rule */ @@ -71,23 +74,8 @@ CreateFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { return null } -export type ActiveSubforms = { target: boolean; port: boolean; host: boolean } -const defaultActiveSubforms: ActiveSubforms = { target: false, port: false, host: false } - export function CreateFirewallRuleForm() { - const [subformStates, setSubformStates] = useState(defaultActiveSubforms) - const updateSubformStates = (subform: keyof ActiveSubforms, value: boolean) => { - setSubformStates({ - ...subformStates, - [subform]: value, - }) - } - const activeSubformList = commaSeries( - Object.keys(subformStates).filter((key) => subformStates[key as keyof ActiveSubforms]), - 'and' - ) - .replace('port', 'port filter') - .replace('host', 'host filter') + const { subformStates, updateSubformStates } = useSubformStates(defaultActiveSubforms) const vpcSelector = useVpcSelector() const queryClient = useApiQueryClient() @@ -139,11 +127,7 @@ export function CreateFirewallRuleForm() { loading={updateRules.isPending} submitError={updateRules.error} submitLabel="Add rule" - submitDisabled={ - activeSubformList.length - ? `You have an unsaved ${activeSubformList} entry; save or clear ${activeSubformList.includes('and') ? 'them' : 'it'} to create this firewall rule` - : undefined - } + submitDisabled={submitDisabledMessage(subformStates)} > { @@ -55,6 +60,8 @@ export function EditFirewallRuleForm() { const vpcSelector = useVpcSelector() const queryClient = useApiQueryClient() + const { subformStates, updateSubformStates } = useSubformStates(defaultActiveSubforms) + const { data: firewallRules } = usePrefetchedApiQuery('vpcFirewallRulesView', { query: { project, vpc }, }) @@ -122,6 +129,7 @@ export function EditFirewallRuleForm() { // validationSchema={validationSchema} // validateOnBlur loading={updateRules.isPending} + submitDisabled={submitDisabledMessage(subformStates)} submitError={updateRules.error} > !!otherRules.find((r) => r.name === name)} error={updateRules.error} + updateSubformStates={updateSubformStates} /> ) From 14f5c146c1ebe7439e1ca7ef867b2a824be7abc7 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Mon, 6 Jan 2025 18:09:09 -0800 Subject: [PATCH 3/3] Working on sidemodal closing issue --- app/forms/firewall-rules-common.tsx | 2 +- app/forms/firewall-rules-create.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 176ec2afcb..7a48be93cb 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -566,5 +566,5 @@ export const submitDisabledMessage = (subformStates: ActiveSubforms) => { const activeSubformList = getActiveSubformList(subformStates) return activeSubformList.length > 0 ? `You have an unsaved ${activeSubformList} entry; save or clear ${activeSubformList.includes('and') ? 'them' : 'it'} to create this firewall rule` - : undefined + : '' } diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index 0c500b803b..46a05b142d 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -125,9 +125,9 @@ export function CreateFirewallRuleForm() { }) }} loading={updateRules.isPending} - submitError={updateRules.error} submitLabel="Add rule" submitDisabled={submitDisabledMessage(subformStates)} + submitError={updateRules.error} >