From 36cf58fe6c5c34a86fb4b36ba3a2b4cafd5d04d8 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 06:55:20 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Copilot=20review=20round=2018?= =?UTF-8?q?=20=E2=80=94=20empty-string=20constants,=20nested=20branch=20re?= =?UTF-8?q?set?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - TUI missingRequiredFields treats an empty string as supplied when the property's own `const` pins it there. The one-option control cannot produce anything else, so reporting the seeded value as missing made such a branch permanently uncallable. - A nested SchemaForm is reset by the branch-aware key too. Two outer alternatives can both carry the same nested object field, and a nested form left mounted across an outer switch kept displaying the inner branch chosen for the other one while the newly seeded values described another shape. Signed-off-by: cliffhall --- clients/tui/__tests__/schemaToForm.test.ts | 18 ++++++++ clients/tui/src/utils/schemaToForm.ts | 8 +++- .../groups/SchemaForm/SchemaForm.test.tsx | 45 +++++++++++++++++++ .../groups/SchemaForm/SchemaForm.tsx | 8 +++- 4 files changed, 76 insertions(+), 3 deletions(-) diff --git a/clients/tui/__tests__/schemaToForm.test.ts b/clients/tui/__tests__/schemaToForm.test.ts index 94f4990c..be0d2b10 100644 --- a/clients/tui/__tests__/schemaToForm.test.ts +++ b/clients/tui/__tests__/schemaToForm.test.ts @@ -837,6 +837,24 @@ describe("schemaToForm", () => { expect(missingRequiredFields(nullable, { a: null })).toEqual([]); }); + it("accepts an empty string a const pins the field to", () => { + // The one-option control cannot produce anything else, so reporting the + // seeded value as missing would make the branch permanently uncallable. + const pinnedEmpty = { + type: "object", + properties: { kind: { type: "string", const: "" } }, + required: ["kind"], + }; + expect(missingRequiredFields(pinnedEmpty, { kind: "" })).toEqual([]); + // An ordinary required string is still missing when left blank. + const ordinary = { + type: "object", + properties: { kind: { type: "string" } }, + required: ["kind"], + }; + expect(missingRequiredFields(ordinary, { kind: "" })).toEqual(["kind"]); + }); + it("checks the root's own required fields when there is no union", () => { const schema = { type: "object", diff --git a/clients/tui/src/utils/schemaToForm.ts b/clients/tui/src/utils/schemaToForm.ts index 03224bdd..178a9084 100644 --- a/clients/tui/src/utils/schemaToForm.ts +++ b/clients/tui/src/utils/schemaToForm.ts @@ -481,6 +481,12 @@ export function missingRequiredFields( !admitsNull(property) ); } - return value === undefined || value === ""; + if (value === "") { + // A branch may pin its discriminator to the empty string, and the + // one-option control cannot produce anything else — reporting the seeded + // value as missing would make that branch permanently uncallable. + return constOf(properties[name]) !== ""; + } + return value === undefined; }); } diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 044302ce..9787ced3 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -2215,6 +2215,51 @@ describe("SchemaForm multiline strings (#2042)", () => { ); }); + it("resets a nested form's own branch when the outer branch changes", async () => { + const user = userEvent.setup(); + const nested: InspectorFormSchema = { + type: "object", + properties: { + config: { + type: "object", + title: "Config", + properties: {}, + anyOf: [ + { + type: "object", + title: "Inner A", + properties: { alpha: { type: "string", title: "Alpha" } }, + }, + { + type: "object", + title: "Inner B", + properties: { beta: { type: "string", title: "Beta" } }, + }, + ], + }, + }, + anyOf: [ + { type: "object", title: "Outer A", properties: { x: {} } }, + { type: "object", title: "Outer B", properties: { y: {} } }, + ], + }; + renderWithMantine( + , + ); + const [outer, inner] = screen.getAllByRole("textbox", { + name: /Variant/, + }); + await user.click(inner!); + await user.click(screen.getByRole("option", { name: "Inner B" })); + expect(screen.getByRole("textbox", { name: /Beta/ })).toBeTruthy(); + + await user.click(outer!); + await user.click(screen.getByRole("option", { name: "Outer B" })); + // The nested form is still mounted, so only a changed reset key can stop + // it displaying a branch the newly seeded values do not describe. + expect(screen.getByRole("textbox", { name: /Alpha/ })).toBeTruthy(); + }); + 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 8ef66f11..699f2eaa 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -972,8 +972,12 @@ export function SchemaForm({ handleFieldChange(fieldName, nestedValues) } disabled={disabled} - // Sub-fields belong to the same entity, so they reset with it. - resetKey={resetKey} + // Sub-fields belong to the same entity, so they reset with it — + // and to the branch it is being edited under, since two outer + // alternatives can both carry this field and a nested form left + // mounted across the switch would keep displaying the nested + // branch chosen for the other one. + resetKey={draftKey} // A nested form's invalid draft is the outer form's invalid draft, // so it reports through the same channel under this field's name. onValidityChange={(nestedInvalid) =>