Skip to content

Commit 4865195

Browse files
committed
fix: address Copilot review round 39 — identify branches by their schemas, cover the deep-link path
- 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 <cliff@futurescale.com>
1 parent ab85667 commit 4865195

2 files changed

Lines changed: 70 additions & 3 deletions

File tree

‎clients/web/src/components/groups/SchemaForm/SchemaForm.tsx‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -631,9 +631,12 @@ export function SchemaForm({
631631
// reordered or rewritten — and a numeric index then points at a different
632632
// branch than the one whose values are held, showing SMS while submitting
633633
// email. Re-derived from the values, which is where the answer actually is.
634-
const branchesKey = branches
635-
.map((branch) => `${branch.label}:${branch.declaredFields.join(",")}`)
636-
.join("|");
634+
// The whole resolved alternative, not just its label and field names: two
635+
// branches can share both while pinning different discriminators or typing a
636+
// field differently, and a reorder of those would otherwise go unnoticed.
637+
const branchesKey = serializeJson(
638+
branches.map((branch) => [branch.label, branch.schema]),
639+
);
637640
useValueChange(branchesKey, () =>
638641
setBranchIndex(selectBranchIndex(branches, values) ?? 0),
639642
);

‎clients/web/src/components/views/InspectorView/InspectorView.test.tsx‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -867,6 +867,70 @@ describe("InspectorView", () => {
867867
expect(screen.getByDisplayValue("retention")).toBeInTheDocument();
868868
});
869869

870+
it("deep-link appArgs select their root-union branch and keep its defaults (#2123)", async () => {
871+
const unionAppTool: Tool = {
872+
name: "notify",
873+
title: "Notify",
874+
inputSchema: {
875+
type: "object",
876+
properties: { note: { type: "string" } },
877+
anyOf: [
878+
{
879+
type: "object",
880+
properties: {
881+
kind: { type: "string", const: "email" },
882+
address: { type: "string" },
883+
retries: { type: "number", default: 1 },
884+
},
885+
required: ["kind", "address"],
886+
},
887+
{
888+
type: "object",
889+
properties: {
890+
kind: { type: "string", const: "sms" },
891+
phone: { type: "string" },
892+
retries: { type: "number", default: 3 },
893+
},
894+
required: ["kind", "phone"],
895+
},
896+
],
897+
},
898+
_meta: { ui: { resourceUri: "ui://apps/notify" } },
899+
};
900+
renderWithMantine(
901+
<StatefulInspectorViewHost
902+
{...makeProps({
903+
servers: [sampleServer],
904+
activeServer: "alpha",
905+
connectionStatus: "connected",
906+
initializeResult: connectedInit,
907+
tools: [unionAppTool],
908+
deepLink: {
909+
serverId: "deep-link",
910+
serverConfig: {
911+
type: "streamable-http",
912+
url: "https://example.com/mcp",
913+
},
914+
openApp: "notify",
915+
// Names the SECOND branch. A shallow default-then-overlay would
916+
// seed the first branch's fields underneath these values.
917+
appArgs: { kind: "sms", phone: "555-0100" },
918+
autoOpen: false,
919+
},
920+
})}
921+
/>,
922+
);
923+
// The picker opens on the branch the args describe…
924+
expect(await screen.findByDisplayValue("555-0100")).toBeInTheDocument();
925+
// Twice over: the Variant picker names the branch, and the read-only
926+
// discriminator carries the value that will be submitted.
927+
expect(screen.getAllByDisplayValue("sms")).toHaveLength(2);
928+
// …with THAT branch's default, not the first branch's `1`.
929+
expect(screen.getByDisplayValue("3")).toBeInTheDocument();
930+
// …and nothing from the branch this call is not making.
931+
expect(screen.queryByRole("textbox", { name: /address/i })).toBeNull();
932+
});
933+
870934
it("ignores a deep-link openApp whose tool is not an app (no tab switch)", async () => {
871935
renderWithMantine(
872936
<StatefulInspectorViewHost

0 commit comments

Comments
 (0)