Make it harder to forget to submit targets/filters subform

[charliepark]

Summary: This PR makes it harder to accidentally drop firewall targets/filters when creating/editing a firewall.

Context: It is not uncommon that a user trying to set up a new firewall rule will accidentally submit the overall form without having fully submitted sub-elements — the target, port filter, or host filter — within the form. You can imagine a user having filled in the Targets part of the form below, then moving their attention to Filters (and beyond), not knowing that they haven't actually added the target to their firewall rule:

This PR updates the firewall rules form so that if any of the three subforms have data entered-but-not-submitted, the overall form has its submit function disabled, with copy in a tooltip that directs the user to the appropriate spot on the form

Still writing a few tests for it.

Closes #2627

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
console	✅ Ready (Inspect)	Visit Preview	Apr 28, 2025 3:26pm

[benjaminleonard]

What if instead of disabling the form completely we require a confirmation press? Like "You have an unsaved target entry, are you sure you want to continue?". The design would require a little work.

[david-crespo]

I kind of like it feeling like a validation error and auto-scrolling to the offending field. In the confirmation version, I think you'd be curious what you left hanging, so you'd hit "no I don't want to continue" and go looking for it, which is the same thing with a lot more steps. Unless we put it right in the modal like "here, you dropped this", which is a weird thing to do, because if you do want to go back and add that thing, you still have to close the confirmation and go find it in the form.

Implementation-wise, I think we can do a lot better than manually tracking the subform dirty states: we already have a canonical spot for that that state, namely the subforms.

function useSubformsDirtyState() {
  return {
    target: !!useWatch({ control: targetControl, name: 'value' }).trim(),
    host: !!useWatch({ control: hostControl, name: 'value' }).trim(),
    port: !!useWatch({ control: portControl, name: 'portRange' }).trim(),
  }
}

This would require some refactoring because the target and host subforms are currently initialized inside a helper component.

console/app/forms/firewall-rules-common.tsx

Lines 87 to 108 in 9de391c

	const TargetAndHostFilterSubform = ({
	sectionType,
	control,
	messageContent,
	}: {
	sectionType: 'target' | 'host'
	control: Control<FirewallRuleValues>
	messageContent: ReactNode
	}) => {
	const { project, vpc } = useVpcSelector()
	// prefetchedApiQueries below are prefetched in firewall-rules-create and -edit
	const {
	data: { items: instances },
	} = usePrefetchedApiQuery('instanceList', { query: { project, limit: ALL_ISH } })
	const {
	data: { items: vpcs },
	} = usePrefetchedApiQuery('vpcList', { query: { project, limit: ALL_ISH } })
	const {
	data: { items: vpcSubnets },
	} = usePrefetchedApiQuery('vpcSubnetList', { query: { project, vpc, limit: ALL_ISH } })
	const subform = useForm({ defaultValues: targetAndHostDefaultValues })

This might be a little tough, though, because moving them up one level to CommonFields

console/app/forms/firewall-rules-common.tsx

Lines 299 to 301 in 9de391c

	export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) => {
	// Ports
	const portRangeForm = useForm({ defaultValues: { portRange: '' } })

isn't enough if what we need is to make the data available up here:

Manually tracking the state is a reasonable hack but I'd rather just get the hierarchy right and have access to the source of truth where we need it. It wouldn't be a big deal to init all the subforms here next to the top level form and pass them into CommonFields as props.

console/app/forms/firewall-rules-create.tsx

Lines 106 to 139 in 9de391c

	const form = useForm({ defaultValues })
	return (
	<SideModalForm
	form={form}
	formType="create"
	resourceName="rule"
	title="Add firewall rule"
	onDismiss={onDismiss}
	onSubmit={(values) => {
	updateRules.mutate({
	query: vpcSelector,
	body: {
	rules: [...existingRules.map(firewallRuleGetToPut), valuesToRuleUpdate(values)],
	},
	})
	}}
	loading={updateRules.isPending}
	submitError={updateRules.error}
	submitLabel="Add rule"
	>
	<CommonFields
	control={form.control}
	// error if name is already in use
	nameTaken={(name) => !!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
	// top level error if any errors are found in the form. We might want to
	// expand the submitError prop on SideModalForm for this
	/>
	</SideModalForm>
	)