From 139563f7203594e6e1b9abc32ff9a61a98962030 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 14:02:22 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Copilot=20review=20round=2035?= =?UTF-8?q?=20=E2=80=94=20reseed=20on=20an=20in-place=20schema=20change,?= =?UTF-8?q?=20label=20every=20primitive=20const?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The seeding effect is keyed by the fixed values themselves as well as the entity and branch. A tool refreshed in place keeps its `resetKey` and its branch while its schema changes underneath, so a newly pinned field was rendered read-only and never seeded — leaving a required one permanently unsubmittable. The schema object's identity says nothing (callers rebuild it every render), which is why the key is derived from what has to be seeded. - branchLabel stringifies every primitive constant. `hasDiscriminator` accepts distinct booleans and nulls, so a `true`/`false` union was discriminated perfectly well and still labelled "Option 1"/"Option 2". Signed-off-by: cliffhall --- .../groups/SchemaForm/SchemaForm.test.tsx | 39 +++++++++++++++++++ .../groups/SchemaForm/SchemaForm.tsx | 13 +++++-- clients/web/src/test/core/rootUnion.test.ts | 14 +++++++ core/json/rootUnion.ts | 19 ++++++--- 4 files changed, 76 insertions(+), 9 deletions(-) diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 0d492714..8e42c452 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -2316,6 +2316,45 @@ describe("SchemaForm multiline strings (#2042)", () => { expect(onChange).toHaveBeenCalledWith({ count: undefined }); }); + it("seeds a newly pinned field when the schema changes in place", async () => { + // A tool refreshed in place keeps its `resetKey` and its branch while the + // schema changes underneath — the new read-only field must still be + // seeded, or a required one leaves submit disabled with no way to fix it. + const onChange = vi.fn(); + const before: InspectorFormSchema = { + type: "object", + properties: { a: { type: "string", title: "A" } }, + }; + const { rerender } = renderWithMantine( + , + ); + await Promise.resolve(); + onChange.mockClear(); + + rerender( + , + ); + await waitFor(() => + expect(onChange).toHaveBeenCalledWith({ version: "2" }), + ); + }); + 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 7cd2b174..047502bb 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -719,6 +719,11 @@ export function SchemaForm({ // 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. + // The fixed values themselves, as a stable key: callers rebuild the schema + // object every render, so its identity says nothing, while this changes + // exactly when what has to be seeded changes. + const seedKey = serializeJson(collectSchemaDefaults(effectiveSchema)); + 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. @@ -737,9 +742,11 @@ export function SchemaForm({ 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]); + // Keyed by the entity and branch being edited, and by the fixed values + // themselves: a tool refreshed in place keeps its `resetKey` and its branch + // while its schema changes underneath, and a newly pinned field would + // otherwise be rendered read-only and never seeded. + }, [draftKey, seedKey]); // 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 diff --git a/clients/web/src/test/core/rootUnion.test.ts b/clients/web/src/test/core/rootUnion.test.ts index 1fddfb0c..69100561 100644 --- a/clients/web/src/test/core/rootUnion.test.ts +++ b/clients/web/src/test/core/rootUnion.test.ts @@ -583,6 +583,20 @@ describe("resolveRootUnion", () => { expect(branches[0].label).toBe("email"); }); + it("labels boolean and null constants by their values", () => { + // `hasDiscriminator` accepts these, so the label must too — otherwise a + // perfectly discriminated union reads as "Option 1"/"Option 2". + const { branches } = resolveRootUnion({ + type: "object", + required: ["v"], + oneOf: [ + { type: "object", properties: { v: { const: true }, a: {} } }, + { type: "object", properties: { v: { const: null }, b: {} } }, + ], + }); + expect(branches.map((branch) => branch.label)).toEqual(["true", "null"]); + }); + it("labels a numeric const by its value", () => { const { branches } = resolveRootUnion({ type: "object", diff --git a/core/json/rootUnion.ts b/core/json/rootUnion.ts index 01db8290..b480f4e8 100644 --- a/core/json/rootUnion.ts +++ b/core/json/rootUnion.ts @@ -586,20 +586,27 @@ function branchLabel( return branch.title; } const properties = propertiesOf(branch); - const constOf = (name: string): string | null => { + // Every primitive constant is a usable label, not just the string and number + // cases: `hasDiscriminator` accepts distinct booleans and nulls, so a + // `true`/`false` union would otherwise be labelled "Option 1"/"Option 2" + // while being discriminated perfectly well. An object or array constant is + // not a label — `undefined` marks "nothing usable here", which lets a `null` + // constant be a value rather than the sentinel. + const constOf = (name: string): string | undefined => { const property = toBranch(properties[name]) as { const?: unknown } | null; - const value = property?.const; - return typeof value === "string" || typeof value === "number" + if (property === null || !("const" in property)) return undefined; + const value = property.const; + return value === null || typeof value !== "object" ? String(value) - : null; + : undefined; }; if (discriminatorProperty !== undefined) { const value = constOf(discriminatorProperty); - if (value !== null) return value; + if (value !== undefined) return value; } const constants = Object.keys(properties) .map((name) => constOf(name)) - .filter((value): value is string => value !== null); + .filter((value): value is string => value !== undefined); if (constants.length === 1) return constants[0]; return `Option ${index + 1}`; }