From 67e7a4ca1a3aaf9529711db6452cef449dff05b5 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 12:01:08 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Copilot=20review=20round=2030?= =?UTF-8?q?=20=E2=80=94=20seed=20a=20read-only=20const=20for=20callers=20t?= =?UTF-8?q?hat=20do=20not?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rendering every `const` read-only assumed the caller had already seeded it, which SchemaForm never did itself: `InlineElicitationRequest` passes its values straight through and gates submit on hasMissingRequiredFields, so a required `const` — a root union's discriminator among them — was displayed and never supplied, leaving Submit disabled with no way to fix it. The form now reports the fixed values upward once per entity, in an effect (the parent's `onChange` cannot be called during our render), adding only names absent from `values` — so a caller that already seeds sees no call at all and the report cannot loop. Signed-off-by: cliffhall --- .../groups/SchemaForm/SchemaForm.test.tsx | 27 ++++++++++++++ .../groups/SchemaForm/SchemaForm.tsx | 35 +++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index a0ed9b2f..575c52d0 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -7,6 +7,7 @@ import { fireEvent, renderWithMantine, screen, + waitFor, } from "../../../test/renderWithMantine"; import { SchemaForm } from "./SchemaForm"; @@ -2264,6 +2265,32 @@ describe("SchemaForm multiline strings (#2042)", () => { expect(screen.getByRole("textbox", { name: /Alpha/ })).toBeTruthy(); }); + it("reports the branch's fixed values upward when mounted with none", async () => { + // A read-only `const` the caller never seeded would otherwise be + // displayed and, if required, keep submit disabled forever. + const onChange = vi.fn(); + renderWithMantine( + , + ); + await waitFor(() => + expect(onChange).toHaveBeenCalledWith({ kind: "email" }), + ); + }); + + it("reports nothing when the caller already seeded them", async () => { + const onChange = vi.fn(); + renderWithMantine( + , + ); + // Nothing is missing, so the form does not touch the caller's values. + await Promise.resolve(); + expect(onChange).not.toHaveBeenCalled(); + }); + it("renders no picker for a single-branch union but still shows its fields", () => { const schema: InspectorFormSchema = { type: "object", diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index 155f5205..8dfde16f 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -706,6 +706,41 @@ export function SchemaForm({ [], ); + // Report the schema's own fixed values upward once per entity, for a caller + // that mounts the form with nothing seeded. + // + // A `const` field is rendered read-only, so the user cannot supply it — and a + // required one then leaves submit disabled forever, on a value that was never + // in doubt. Most callers seed through `collectSchemaDefaults` before mounting; + // this covers the ones that do not, rather than making every caller + // responsible for a value the form is already displaying. + // + // An effect, not a render-time update: `onChange` belongs to the parent, and + // calling it during our render would update another component mid-render. + // Only names absent from `values` are added, so a caller that has already + // seeded them sees no call at all, and re-running cannot loop. + const latestSeed = useRef({ schema: effectiveSchema, values, onChange }); + // Written in an effect, not during render — the same shape the validity + // reporter below uses, and what `react-hooks/refs` requires. + useEffect(() => { + latestSeed.current = { schema: effectiveSchema, values, onChange }; + }); + useEffect(() => { + const { + schema: current, + values: held, + onChange: report, + } = latestSeed.current; + const missing = Object.entries(collectSchemaDefaults(current)).filter( + ([name]) => !Object.hasOwn(held, name), + ); + if (missing.length > 0) { + report({ ...held, ...Object.fromEntries(missing) }); + } + // Keyed by the entity and branch being edited: a different one has + // different fixed values, and the same one needs this only once. + }, [draftKey]); + // Read through a ref so the callback's identity is not a dependency. It has // to be one or the other, and a *stable* dependency is what this needs: a // nested form is handed a fresh closure every render, and re-running the