From 48651956e530ada6b78e7c43fbc6ca481c00fabc Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 15:37:28 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Copilot=20review=20round=2039?= =?UTF-8?q?=20=E2=80=94=20identify=20branches=20by=20their=20schemas,=20co?= =?UTF-8?q?ver=20the=20deep-link=20path?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - branchesKey is built from the resolved alternatives themselves, not from labels and field names: two branches can share both while pinning different discriminators or typing a field differently, and a reorder of those went unnoticed — the form then displayed one branch and submitted another. - InspectorView gains a deep-link test with root-union appArgs: they name the second branch, and the assertions pin the picker, the read-only discriminator, THAT branch's default (not the first branch's), and the absence of the other branch's field. The old shallow spread fails it. Signed-off-by: cliffhall --- .../groups/SchemaForm/SchemaForm.tsx | 9 ++- .../InspectorView/InspectorView.test.tsx | 64 +++++++++++++++++++ 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index c62641e0..9fbb3d69 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -631,9 +631,12 @@ export function SchemaForm({ // reordered or rewritten — and a numeric index then points at a different // branch than the one whose values are held, showing SMS while submitting // email. Re-derived from the values, which is where the answer actually is. - const branchesKey = branches - .map((branch) => `${branch.label}:${branch.declaredFields.join(",")}`) - .join("|"); + // The whole resolved alternative, not just its label and field names: two + // branches can share both while pinning different discriminators or typing a + // field differently, and a reorder of those would otherwise go unnoticed. + const branchesKey = serializeJson( + branches.map((branch) => [branch.label, branch.schema]), + ); useValueChange(branchesKey, () => setBranchIndex(selectBranchIndex(branches, values) ?? 0), ); diff --git a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx index d7e2e5c1..fc34c4bb 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx @@ -867,6 +867,70 @@ describe("InspectorView", () => { expect(screen.getByDisplayValue("retention")).toBeInTheDocument(); }); + it("deep-link appArgs select their root-union branch and keep its defaults (#2123)", async () => { + const unionAppTool: Tool = { + name: "notify", + title: "Notify", + inputSchema: { + type: "object", + properties: { note: { type: "string" } }, + anyOf: [ + { + type: "object", + properties: { + kind: { type: "string", const: "email" }, + address: { type: "string" }, + retries: { type: "number", default: 1 }, + }, + required: ["kind", "address"], + }, + { + type: "object", + properties: { + kind: { type: "string", const: "sms" }, + phone: { type: "string" }, + retries: { type: "number", default: 3 }, + }, + required: ["kind", "phone"], + }, + ], + }, + _meta: { ui: { resourceUri: "ui://apps/notify" } }, + }; + renderWithMantine( + , + ); + // The picker opens on the branch the args describe… + expect(await screen.findByDisplayValue("555-0100")).toBeInTheDocument(); + // Twice over: the Variant picker names the branch, and the read-only + // discriminator carries the value that will be submitted. + expect(screen.getAllByDisplayValue("sms")).toHaveLength(2); + // …with THAT branch's default, not the first branch's `1`. + expect(screen.getByDisplayValue("3")).toBeInTheDocument(); + // …and nothing from the branch this call is not making. + expect(screen.queryByRole("textbox", { name: /address/i })).toBeNull(); + }); + it("ignores a deep-link openApp whose tool is not an app (no tab switch)", async () => { renderWithMantine(