diff --git a/app/components/form/fields/NumberField.tsx b/app/components/form/fields/NumberField.tsx index aded8dc8f..a8bea7201 100644 --- a/app/components/form/fields/NumberField.tsx +++ b/app/components/form/fields/NumberField.tsx @@ -65,6 +65,7 @@ export const NumberFieldInner = < name, label = capitalize(name), validate, + deps, control, required, id: idProp, @@ -83,6 +84,7 @@ export const NumberFieldInner = < control, rules: { required, + deps, // it seems we need special logic to enforce required on NaN validate(value, values) { if (required && Number.isNaN(value)) return `${label} is required` diff --git a/app/components/form/fields/TextField.tsx b/app/components/form/fields/TextField.tsx index c306d42ef..f31445dea 100644 --- a/app/components/form/fields/TextField.tsx +++ b/app/components/form/fields/TextField.tsx @@ -12,6 +12,7 @@ import { type FieldPath, type FieldPathValue, type FieldValues, + type RegisterOptions, type Validate, } from 'react-hook-form' @@ -45,6 +46,7 @@ export interface TextFieldProps< placeholder?: string units?: string validate?: Validate, TFieldValues> + deps?: RegisterOptions['deps'] control: Control /** Alters the value of the input during the field's onChange event. */ transform?: (value: string) => string @@ -98,6 +100,7 @@ export const TextFieldInner = < type = 'text', label = capitalize(name), validate, + deps, control, required, id: idProp, @@ -109,7 +112,7 @@ export const TextFieldInner = < const { field: { onChange, ...fieldRest }, fieldState: { error }, - } = useController({ name, control, rules: { required, validate } }) + } = useController({ name, control, rules: { required, validate, deps } }) return ( <> { - const first = parseIp(values.first) - const last = parseIp(values.last) - - const errors: FieldErrors = {} - - // Validate first address matches pool version - if (first.type === 'error') { - errors.first = { type: 'pattern', message: first.message } - } else if (first.type === 'v4' && poolVersion === 'v6') { - errors.first = { - type: 'pattern', - message: 'IPv4 address not allowed in IPv6 pool', - } - } else if (first.type === 'v6' && poolVersion === 'v4') { - errors.first = { - type: 'pattern', - message: 'IPv6 address not allowed in IPv4 pool', - } - } - - // Validate last address matches pool version - if (last.type === 'error') { - errors.last = { type: 'pattern', message: last.message } - } else if (last.type === 'v4' && poolVersion === 'v6') { - errors.last = { - type: 'pattern', - message: 'IPv4 address not allowed in IPv6 pool', - } - } else if (last.type === 'v6' && poolVersion === 'v4') { - errors.last = { - type: 'pattern', - message: 'IPv6 address not allowed in IPv4 pool', - } - } - - // TODO: if we were really cool we could check first <= last but it would add - // 6k gzipped to the bundle with ip-num - - // no errors - return Object.keys(errors).length > 0 ? { values: {}, errors } : { values, errors: {} } +const validateAddress = (value: string, poolVersion: IpVersion) => { + const parsed = parseIp(value) + if (parsed.type === 'error') return parsed.message + if (parsed.type !== poolVersion) { + return `IP${parsed.type} address not allowed in IP${poolVersion} pool` } } @@ -107,10 +61,7 @@ export default function IpPoolAddRange() { }, }) - const form = useForm({ - defaultValues, - resolver: createResolver(poolData.ipVersion), - }) + const form = useForm({ defaultValues }) return ( validateAddress(value, poolData.ipVersion)} /> validateAddress(value, poolData.ipVersion)} /> diff --git a/app/forms/subnet-pool-member-add.spec.ts b/app/forms/subnet-pool-member-add.spec.ts index 67e297e0a..af4c7e8ca 100644 --- a/app/forms/subnet-pool-member-add.spec.ts +++ b/app/forms/subnet-pool-member-add.spec.ts @@ -7,91 +7,93 @@ */ import { describe, expect, it } from 'vitest' -import { createResolver } from './subnet-pool-member-add' +import { validateMember } from './subnet-pool-member-add' -const resolve = createResolver('v4') -const resolve6 = createResolver('v6') +const validate = (values: Parameters[1]) => + validateMember('v4', values) +const validate6 = (values: Parameters[1]) => + validateMember('v6', values) const valid = { subnet: '10.0.0.0/16', minPrefixLength: 20, maxPrefixLength: 28 } -type Result = ReturnType +type Field = 'subnet' | 'minPrefixLength' | 'maxPrefixLength' -function errMsg(result: Result, field: keyof Result['errors']) { - return result.errors[field]?.message +function errMsg(result: ReturnType, field: Field) { + return result[field] } -describe('createResolver', () => { +describe('validateMember', () => { it('accepts valid v4 input', () => { - expect(Object.keys(resolve(valid).errors)).toEqual([]) + expect(validate(valid)).toEqual({}) }) it('accepts valid v6 input', () => { - const result = resolve6({ + const result = validate6({ subnet: 'fd00:1000::/32', minPrefixLength: 48, maxPrefixLength: 64, }) - expect(Object.keys(result.errors)).toEqual([]) + expect(result).toEqual({}) }) it('accepts omitted prefix lengths', () => { - const result = resolve({ + const result = validate({ subnet: '10.0.0.0/16', minPrefixLength: NaN, maxPrefixLength: NaN, }) - expect(Object.keys(result.errors)).toEqual([]) + expect(result).toEqual({}) }) it('rejects invalid CIDR', () => { - const result = resolve({ ...valid, subnet: 'not-a-cidr' }) + const result = validate({ ...valid, subnet: 'not-a-cidr' }) expect(errMsg(result, 'subnet')).toMatch(/IP address/) }) it('rejects v6 subnet in v4 pool', () => { - const result = resolve({ ...valid, subnet: 'fd00::/32' }) + const result = validate({ ...valid, subnet: 'fd00::/32' }) expect(errMsg(result, 'subnet')).toBe('IPv6 subnet not allowed in IPv4 pool') }) it('rejects v4 subnet in v6 pool', () => { - const result = resolve6({ ...valid, subnet: '10.0.0.0/16' }) + const result = validate6({ ...valid, subnet: '10.0.0.0/16' }) expect(errMsg(result, 'subnet')).toBe('IPv4 subnet not allowed in IPv6 pool') }) it('rejects min > max prefix length', () => { - const result = resolve({ ...valid, minPrefixLength: 28, maxPrefixLength: 20 }) + const result = validate({ ...valid, minPrefixLength: 28, maxPrefixLength: 20 }) expect(errMsg(result, 'minPrefixLength')).toMatch(/≤/) }) it('rejects min prefix length < subnet width', () => { - const result = resolve({ ...valid, minPrefixLength: 8 }) + const result = validate({ ...valid, minPrefixLength: 8 }) expect(errMsg(result, 'minPrefixLength')).toMatch(/≥ subnet prefix length \(16\)/) }) it('rejects max prefix length < subnet width', () => { - const result = resolve({ ...valid, maxPrefixLength: 8 }) + const result = validate({ ...valid, maxPrefixLength: 8 }) expect(errMsg(result, 'maxPrefixLength')).toMatch(/≥ subnet prefix length \(16\)/) }) it('rejects prefix length above max bound (v4: 32)', () => { - const result = resolve({ ...valid, minPrefixLength: 33 }) + const result = validate({ ...valid, minPrefixLength: 33 }) expect(errMsg(result, 'minPrefixLength')).toBe('Must be between 0 and 32') }) it('rejects prefix length below 0', () => { - const result = resolve({ ...valid, maxPrefixLength: -1 }) + const result = validate({ ...valid, maxPrefixLength: -1 }) expect(errMsg(result, 'maxPrefixLength')).toBe('Must be between 0 and 32') }) it('shows min-≤-max error even when min is also below subnet width', () => { // min(12) > max(10) AND min(12) < subnetWidth(16): the min-≤-max error // should take priority over the subnet-width error - const result = resolve({ ...valid, minPrefixLength: 12, maxPrefixLength: 10 }) + const result = validate({ ...valid, minPrefixLength: 12, maxPrefixLength: 10 }) expect(errMsg(result, 'minPrefixLength')).toMatch(/≤/) }) it('rejects prefix length above max bound (v6: 128)', () => { - const result = resolve6({ + const result = validate6({ subnet: 'fd00::/32', minPrefixLength: 48, maxPrefixLength: 200, diff --git a/app/forms/subnet-pool-member-add.tsx b/app/forms/subnet-pool-member-add.tsx index 9327c9091..b1709fe41 100644 --- a/app/forms/subnet-pool-member-add.tsx +++ b/app/forms/subnet-pool-member-add.tsx @@ -5,7 +5,7 @@ * * Copyright Oxide Computer Company */ -import { useForm, type FieldErrors } from 'react-hook-form' +import { useForm } from 'react-hook-form' import { useNavigate } from 'react-router' import { @@ -41,65 +41,59 @@ const defaultValues: MemberAddForm = { maxPrefixLength: NaN, } -// Using a resolver overrides all field-level validation (required, min, max, -// etc.), so this function must cover everything. Field-level props like -// `required` on subnet and `min`/`max` on NumberFields still affect UI display -// and stepper behavior, but their RHF validation rules are inert. -export function createResolver(poolVersion: IpVersion) { - return (values: MemberAddForm) => { - const errors: FieldErrors = {} - const maxBound = poolVersion === 'v4' ? 32 : 128 - - const parsed = parseIpNet(values.subnet) - if (parsed.type === 'error') { - errors.subnet = { type: 'pattern', message: parsed.message } - } else if (parsed.type !== poolVersion) { - errors.subnet = { - type: 'pattern', - message: `IP${parsed.type} subnet not allowed in IP${poolVersion} pool`, - } - } - - const { minPrefixLength: minPL, maxPrefixLength: maxPL } = values - const subnetWidth = parsed.type !== 'error' ? parsed.width : undefined - const inRange = (v: number) => !Number.isNaN(v) && v >= 0 && v <= maxBound - - // min and max prefix length are optional, and NaN is the value they have - // when they're unset (matching NumberField) - - // min prefix: bounds → ordering → subnet width - if (!Number.isNaN(minPL) && !inRange(minPL)) { - errors.minPrefixLength = { - type: 'validate', - message: `Must be between 0 and ${maxBound}`, - } - } else if (inRange(minPL) && inRange(maxPL) && minPL > maxPL) { - errors.minPrefixLength = { - type: 'validate', - message: 'Min prefix length must be ≤ max prefix length', - } - } else if (inRange(minPL) && subnetWidth !== undefined && minPL < subnetWidth) { - errors.minPrefixLength = { - type: 'validate', - message: `Must be ≥ subnet prefix length (${subnetWidth})`, - } - } - - // max prefix: bounds → subnet width - if (!Number.isNaN(maxPL) && !inRange(maxPL)) { - errors.maxPrefixLength = { - type: 'validate', - message: `Must be between 0 and ${maxBound}`, - } - } else if (inRange(maxPL) && subnetWidth !== undefined && maxPL < subnetWidth) { - errors.maxPrefixLength = { - type: 'validate', - message: `Must be ≥ subnet prefix length (${subnetWidth})`, - } - } - - return { values: Object.keys(errors).length > 0 ? {} : values, errors } +type ValidationErrors = Partial> + +/** + * This function is a sneaky way to back into cross-field validation while only + * hooking into the field-level `validate` callback. This function looks at all + * the form `values` together and sets errors for each field in the form, and + * then the callsites look like this: they all call it the same way and just + * pluck their own error off the result. + * + * ```ts + * validate={(_maxPrefixLength, values) => + * validateMember(poolData.ipVersion, values).maxPrefixLength + * } + * ``` + */ +export function validateMember( + poolVersion: IpVersion, + values: MemberAddForm +): ValidationErrors { + const maxBound = poolVersion === 'v4' ? 32 : 128 + const parsed = parseIpNet(values.subnet) + const { minPrefixLength: minPL, maxPrefixLength: maxPL } = values + const subnetWidth = parsed.type !== 'error' ? parsed.width : undefined + const inRange = (v: number) => !Number.isNaN(v) && v >= 0 && v <= maxBound + + const errors: ValidationErrors = {} + + if (parsed.type === 'error') { + errors.subnet = parsed.message + } else if (parsed.type !== poolVersion) { + errors.subnet = `IP${parsed.type} subnet not allowed in IP${poolVersion} pool` + } + + // min and max prefix length are optional, and NaN is the value they have + // when they're unset (matching NumberField) + + // min prefix: bounds → ordering → subnet width + if (!Number.isNaN(minPL) && !inRange(minPL)) { + errors.minPrefixLength = `Must be between 0 and ${maxBound}` + } else if (inRange(minPL) && inRange(maxPL) && minPL > maxPL) { + errors.minPrefixLength = 'Min prefix length must be ≤ max prefix length' + } else if (inRange(minPL) && subnetWidth !== undefined && minPL < subnetWidth) { + errors.minPrefixLength = `Must be ≥ subnet prefix length (${subnetWidth})` + } + + // max prefix: bounds → subnet width + if (!Number.isNaN(maxPL) && !inRange(maxPL)) { + errors.maxPrefixLength = `Must be between 0 and ${maxBound}` + } else if (inRange(maxPL) && subnetWidth !== undefined && maxPL < subnetWidth) { + errors.maxPrefixLength = `Must be ≥ subnet prefix length (${subnetWidth})` } + + return errors } export const handle = titleCrumb('Add Member') @@ -123,11 +117,7 @@ export default function SubnetPoolMemberAdd() { }, }) - const form = useForm({ - defaultValues, - // doesn't need to be memoized, doesn't trigger renders - resolver: createResolver(poolData.ipVersion), - }) + const form = useForm({ defaultValues }) const maxBound = poolData.ipVersion === 'v4' ? 32 : 128 @@ -161,6 +151,8 @@ export default function SubnetPoolMemberAdd() { description="CIDR notation (e.g., 10.0.0.0/16)" control={form.control} required + validate={(_subnet, values) => validateMember(poolData.ipVersion, values).subnet} + deps={['minPrefixLength', 'maxPrefixLength']} /> + validateMember(poolData.ipVersion, values).minPrefixLength + } /> + validateMember(poolData.ipVersion, values).maxPrefixLength + } + deps="minPrefixLength" /> diff --git a/package-lock.json b/package-lock.json index 9e9d088bc..f0d0c8079 100644 --- a/package-lock.json +++ b/package-lock.json @@ -36,7 +36,7 @@ "react-aria": "^3.44.0", "react-dom": "^19.2.0", "react-error-boundary": "^4.0.13", - "react-hook-form": "^7.53.0", + "react-hook-form": "^7.72.1", "react-is": "^19.2.0", "react-merge-refs": "^2.1.1", "react-router": "^7.13.0", @@ -11077,9 +11077,9 @@ } }, "node_modules/react-hook-form": { - "version": "7.53.0", - "resolved": "https://registry.npmjs.org/react-hook-form/-/react-hook-form-7.53.0.tgz", - "integrity": "sha512-M1n3HhqCww6S2hxLxciEXy2oISPnAzxY7gvwVPrtlczTM/1dDadXgUxDpHMrMTblDOcm/AXtXxHwZ3jpg1mqKQ==", + "version": "7.72.1", + "resolved": "https://registry.npmjs.org/react-hook-form/-/react-hook-form-7.72.1.tgz", + "integrity": "sha512-RhwBoy2ygeVZje+C+bwJ8g0NjTdBmDlJvAUHTxRjTmSUKPYsKfMphkS2sgEMotsY03bP358yEYlnUeZy//D9Ig==", "license": "MIT", "engines": { "node": ">=18.0.0" diff --git a/package.json b/package.json index 6f138d505..9163a8ad3 100644 --- a/package.json +++ b/package.json @@ -60,7 +60,7 @@ "react-aria": "^3.44.0", "react-dom": "^19.2.0", "react-error-boundary": "^4.0.13", - "react-hook-form": "^7.53.0", + "react-hook-form": "^7.72.1", "react-is": "^19.2.0", "react-merge-refs": "^2.1.1", "react-router": "^7.13.0", diff --git a/test/e2e/subnet-pools.e2e.ts b/test/e2e/subnet-pools.e2e.ts index d7a95a08e..71d6e6952 100644 --- a/test/e2e/subnet-pools.e2e.ts +++ b/test/e2e/subnet-pools.e2e.ts @@ -110,6 +110,35 @@ test('Subnet pool add member', async ({ page }) => { await expectRowVisible(table, { Subnet: '172.16.0.0/12' }) }) +test('Subnet pool add member updates prefix length validation across fields', async ({ + page, +}) => { + await page.goto('/system/networking/subnet-pools/default-v4-subnet-pool/members-add') + + await page.getByRole('textbox', { name: 'Subnet' }).fill('172.16.0.0/12') + await fillNumberInput(page.getByRole('textbox', { name: 'Min prefix length' }), '28') + await fillNumberInput(page.getByRole('textbox', { name: 'Max prefix length' }), '20') + + const dialog = page.getByRole('dialog', { name: 'Add member' }) + await dialog.getByRole('button', { name: 'Add member' }).click() + + await expect(dialog).toBeVisible() + await expect( + dialog.getByText('Min prefix length must be ≤ max prefix length') + ).toBeVisible() + + await fillNumberInput(page.getByRole('textbox', { name: 'Max prefix length' }), '30') + await expect( + dialog.getByText('Min prefix length must be ≤ max prefix length') + ).toBeHidden() + + await page.getByRole('textbox', { name: 'Subnet' }).fill('172.16.0.0/29') + await expect(dialog.getByText('Must be ≥ subnet prefix length (29)')).toBeVisible() + + await fillNumberInput(page.getByRole('textbox', { name: 'Min prefix length' }), '29') + await expect(dialog.getByText('Must be ≥ subnet prefix length (29)')).toBeHidden() +}) + test('Subnet pool remove member', async ({ page }) => { // Use secondary pool — default pool's member has external subnets allocated from it await page.goto('/system/networking/subnet-pools/secondary-v4-subnet-pool')