diff --git a/.changeset/required-when-submit-verdict-4161.md b/.changeset/required-when-submit-verdict-4161.md new file mode 100644 index 0000000000..45256dc6d3 --- /dev/null +++ b/.changeset/required-when-submit-verdict-4161.md @@ -0,0 +1,11 @@ +--- +'@object-ui/components': patch +--- + +Conditional required (`requiredWhen`) now decides at SUBMIT time too — the star and the validator can no longer disagree + +A `requiredWhen` predicate that flipped to FALSE after the dialog mounted updated only half the form. The display layer re-evaluated correctly — the asterisk and `aria-required` both disappeared — while submit stayed refused with " is required" and no write was ever issued. The user saw an optional field and a form that would not save, with nothing on screen naming the field it was still waiting on (objectui#4161). + +The cause is not a mount-time snapshot, which is what the symptom looks like. The renderer hands react-hook-form its per-field rules as a `` prop, and RHF *merges* that object into the field descriptor it already holds — `_f: { ...previous._f, ...options }`. A rule key that stops being spelled is therefore never removed. Rules could be ADDED live (a predicate flipping TRUE after mount did start enforcing, correctly) but never withdrawn: the `validate.required` entry installed the first time the predicate evaluated TRUE outlived every later FALSE verdict. The validation layer was append-only, latched on the first TRUE the field ever produced. + +The `validate.required` entry is now registered unconditionally and decides required-ness when it *runs*, reading the live verdict the renderer publishes on every render — the same single `resolveFieldRuleState` result that draws the asterisk, not a second evaluation of the predicate with its own copy of the record assembly. Both directions are pinned: a predicate flipping FALSE re-opens submit, a predicate flipping TRUE starts enforcing, and statically required fields are unaffected. diff --git a/packages/components/src/renderers/form/__tests__/form-required-when-submit.test.tsx b/packages/components/src/renderers/form/__tests__/form-required-when-submit.test.tsx new file mode 100644 index 0000000000..97818475fd --- /dev/null +++ b/packages/components/src/renderers/form/__tests__/form-required-when-submit.test.tsx @@ -0,0 +1,193 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * Conditional required (`requiredWhen`) has to reach BOTH layers, and until + * objectui#4161 it reached only one. + * + * The renderer recomputes `resolveFieldRuleState` on every render, so the + * DISPLAY layer — the asterisk and `aria-required` — tracks the predicate + * live. The VALIDATION layer did not: the `validate.required` entry is handed + * to react-hook-form as a `` prop, and RHF snapshots those + * rules into `control._fields[name]._f` from a `useEffect` keyed + * `[name, control, isArrayField, shouldUnregister]` — i.e. once, at mount. + * A predicate that flips FALSE after mount therefore dropped the star while + * the mount-time required validator kept firing: submit was refused with + * " is required" and no write was ever issued. + * + * These tests pin the two layers to the SAME verdict, in both directions — + * a fix that merely stopped enforcing conditional required would pass the + * flip-to-FALSE case and fail the flip-to-TRUE one. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, fireEvent, waitFor, cleanup } from '@testing-library/react'; +import { ComponentRegistry } from '@object-ui/core'; +// Module scope, not `beforeAll` — the cold transform must not be billed to +// `hookTimeout`. See object-ui/no-dynamic-import-in-test-hook (objectui#3010). +import '../../../renderers'; + +beforeEach(() => { + if (!(Element.prototype as any).scrollIntoView) { + (Element.prototype as any).scrollIntoView = () => {}; + } +}); + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +function renderForm( + fields: any[], + defaultValues: Record, + onSubmit: (data: any) => Promise | unknown, + extra: Record = {}, +) { + const Form = ComponentRegistry.get('form')!; + return render( +
, + ); +} + +const submit = () => fireEvent.click(screen.getByRole('button', { name: /create/i })); + +/** The reporter's exact predicate shape: `B` is required until `A` is "x". */ +const REPORTED_PREDICATE = '!(has(record.a) && record.a == "x")'; + +describe('form renderer — `requiredWhen` at SUBMIT time (objectui#4161)', () => { + it('lets submit through once the predicate flips FALSE after mount', async () => { + const onSubmit = vi.fn().mockResolvedValue(undefined); + renderForm( + [ + { name: 'a', label: 'Kind', type: 'input' }, + { name: 'b', label: 'Owner', type: 'input', requiredWhen: REPORTED_PREDICATE }, + ], + { a: '', b: '' }, + onSubmit, + ); + + const owner = () => screen.getByLabelText(/owner/i); + // Mount: `a` is blank, so the predicate is TRUE and `b` is required. + expect(owner()).toHaveAttribute('aria-required', 'true'); + + fireEvent.change(screen.getByLabelText(/kind/i), { target: { value: 'x' } }); + + // The DISPLAY layer flips — this half always worked, and asserting it here + // is what makes the failure below a *divergence* between the two layers + // rather than "the predicate never re-evaluated at all". + await waitFor(() => expect(owner()).not.toHaveAttribute('aria-required')); + + submit(); + + await waitFor(() => expect(onSubmit).toHaveBeenCalled()); + expect(onSubmit.mock.calls[0][0]).toMatchObject({ a: 'x', b: '' }); + expect(screen.queryByText(/is required/i)).toBeNull(); + }); + + it('still refuses submit when the predicate flips TRUE after mount', async () => { + // The symmetric direction. Dropping conditional required entirely would + // make the case above pass and this one fail. + const onSubmit = vi.fn().mockResolvedValue(undefined); + renderForm( + [ + { name: 'a', label: 'Kind', type: 'input' }, + { name: 'b', label: 'Owner', type: 'input', requiredWhen: REPORTED_PREDICATE }, + ], + { a: 'x', b: '' }, + onSubmit, + ); + + const owner = () => screen.getByLabelText(/owner/i); + expect(owner()).not.toHaveAttribute('aria-required'); + + fireEvent.change(screen.getByLabelText(/kind/i), { target: { value: 'other' } }); + await waitFor(() => expect(owner()).toHaveAttribute('aria-required', 'true')); + + submit(); + + await waitFor(() => expect(screen.getByText(/owner is required/i)).toBeInTheDocument()); + expect(onSubmit).not.toHaveBeenCalled(); + }); + + it('re-opens submit after a blocked attempt once the predicate flips FALSE', async () => { + // The reporter's live sequence: the refusal is already on screen when the + // user fixes the condition. Clearing the stale message is not enough — + // the next submit has to actually reach the write. + const onSubmit = vi.fn().mockResolvedValue(undefined); + renderForm( + [ + { name: 'a', label: 'Kind', type: 'input' }, + { name: 'b', label: 'Owner', type: 'input', requiredWhen: REPORTED_PREDICATE }, + ], + { a: '', b: '' }, + onSubmit, + ); + + submit(); + await waitFor(() => expect(screen.getByText(/owner is required/i)).toBeInTheDocument()); + expect(onSubmit).not.toHaveBeenCalled(); + + fireEvent.change(screen.getByLabelText(/kind/i), { target: { value: 'x' } }); + await waitFor(() => expect(screen.queryByText(/owner is required/i)).toBeNull()); + + submit(); + await waitFor(() => expect(onSubmit).toHaveBeenCalled()); + }); + + it('keeps enforcing a STATICALLY required field (control)', async () => { + const onSubmit = vi.fn().mockResolvedValue(undefined); + renderForm( + [ + { name: 'a', label: 'Kind', type: 'input' }, + { name: 'b', label: 'Owner', type: 'input', required: true }, + ], + { a: '', b: '' }, + onSubmit, + ); + + fireEvent.change(screen.getByLabelText(/kind/i), { target: { value: 'x' } }); + + submit(); + + await waitFor(() => expect(screen.getByText(/owner is required/i)).toBeInTheDocument()); + expect(onSubmit).not.toHaveBeenCalled(); + }); + + it('saves an edit form whose predicate was already FALSE at mount (control)', async () => { + // The reporter's own control experiment: the edit dialog opened with + // `a = "x"` already set saves fine, which is what localized the defect to + // the mount-time snapshot rather than to the predicate itself. + const onSubmit = vi.fn().mockResolvedValue(undefined); + renderForm( + [ + { name: 'a', label: 'Kind', type: 'input' }, + { name: 'b', label: 'Owner', type: 'input', requiredWhen: REPORTED_PREDICATE }, + ], + { a: 'x', b: '' }, + onSubmit, + ); + + submit(); + + await waitFor(() => expect(onSubmit).toHaveBeenCalled()); + expect(screen.queryByText(/is required/i)).toBeNull(); + }); +}); diff --git a/packages/components/src/renderers/form/form.tsx b/packages/components/src/renderers/form/form.tsx index 6f10f8b979..09e859c106 100644 --- a/packages/components/src/renderers/form/form.tsx +++ b/packages/components/src/renderers/form/form.tsx @@ -1284,6 +1284,19 @@ ComponentRegistry.register('form', const fieldTabsPosition = schema.fieldTabsPosition || 'top'; const isVerticalFieldTabs = fieldTabsPosition === 'left' || fieldTabsPosition === 'right'; + // The LIVE required verdict, per field name: present ⇒ required right now, + // and the value is the message to show; absent ⇒ not required. Republished + // by `renderFormField` below on every render, from the very same + // `resolveFieldRuleState` result that draws the asterisk — so the + // submit-time check and the star can never disagree (objectui#4161). A ref + // rather than state: nothing renders off it, it must be readable from + // inside a validator react-hook-form may have captured at mount, and + // writing it must not itself schedule a render. Lazily initialised rather + // than `useRef(new Map())`, whose argument is re-evaluated (and discarded) + // on every render — the same React footgun documented at the call site. + const requiredMessagesRef = React.useRef | undefined>(undefined); + const requiredMessages = (requiredMessagesRef.current ??= new Map()); + // --- Field rendering --------------------------------------------------- // Renders ONE field row (or a virtual section divider). Hoisted out of the // field loop so a tabbed layout can place the very same row inside a tab @@ -1403,21 +1416,54 @@ ComponentRegistry.register('form', // object-form validate failure under its key, so the error still // surfaces as `type: 'required'` for the conditional-required cleanup // above. + // + // The entry is registered UNCONDITIONALLY and decides required-ness when + // it RUNS, instead of being added only while the field is required + // (objectui#4161). react-hook-form merges a Controller's `rules` INTO the + // field descriptor it already holds — `_f: { ...previous._f, ...options }` + // — so a rule key that simply stops being spelled is never removed: the + // validator installed the first time `requiredWhen` evaluated TRUE + // outlived every later FALSE. The two layers then disagreed: the display + // layer re-evaluated (asterisk and `aria-required` both disappeared) + // while submit stayed refused with " is required" and no write was + // ever issued. Always spelling the key gives that merge something to + // overwrite, and reading the verdict out of `requiredMessages` at call + // time keeps the answer correct even for the closure RHF captured at + // mount — the fix must not rest on RHF re-registering per render, which + // it does today only as a side effect of `useRef(control.register(…))` + // re-evaluating its (discarded) initializer argument. + // + // Deliberately NOT re-evaluating the predicate inside the validator: that + // would be a second evaluation site with its own copy of the record + // assembly (the `null` seeding, the `previousRecord` overlay), i.e. the + // exact drift this issue is about. The verdict published here IS the one + // the asterisk was drawn from. delete rules.required; if (required) { - const requiredMessage = typeof validation.required === 'string' - ? validation.required - : t('validation.required', { field: label || name }); - const authoredValidate = rules.validate; - rules.validate = { - // A field-authored `validate` keeps running, and keeps its own - // `type: 'validate'` error key. - ...(typeof authoredValidate === 'function' - ? { validate: authoredValidate } - : (authoredValidate ?? {})), - required: (value: unknown) => !isMissingForRequired(value) || requiredMessage, - }; + requiredMessages.set( + name, + typeof validation.required === 'string' + ? validation.required + : t('validation.required', { field: label || name }), + ); + } else { + requiredMessages.delete(name); } + const authoredValidate = rules.validate; + rules.validate = { + // A field-authored `validate` keeps running, and keeps its own + // `type: 'validate'` error key. + ...(typeof authoredValidate === 'function' + ? { validate: authoredValidate } + : (authoredValidate ?? {})), + required: (value: unknown) => { + const requiredMessage = requiredMessages.get(name); + // Absent ⇒ not required as of NOW, whatever it was when this + // validator was registered. + if (requiredMessage === undefined) return true; + return !isMissingForRequired(value) || requiredMessage; + }, + }; // Localize the standard validation messages emitted by // buildValidationRules. Each such rule carries a `messageKey`