fix(components): requiredWhen decides at submit time, not on the first TRUE it ever saw (#4161)

.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 "<field> 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 `<Controller rules>` 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.

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 `<Controller rules>` 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
+ * "<field> 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<string, unknown>,
+  onSubmit: (data: any) => Promise<unknown> | unknown,
+  extra: Record<string, unknown> = {},
+) {
+  const Form = ComponentRegistry.get('form')!;
+  return render(
+    <Form
+      schema={{
+        type: 'form',
+        mode: 'create',
+        showSubmit: true,
+        showCancel: false,
+        submitLabel: 'Create',
+        defaultValues,
+        fields,
+        onSubmit,
+        ...extra,
+      }}
+    />,
+  );
+}
+
+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();
+  });
+});

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<Map<string, string> | undefined>(undefined);
+    const requiredMessages = (requiredMessagesRef.current ??= new Map<string, string>());
+
     // --- 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 "<field> 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`