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>
This commit is contained in:
cliffhall 2026-08-26 15:37:28 -04:00
commit 48651956e5
2 changed files with 70 additions and 3 deletions

View file

@ -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),
);

View file

@ -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(
<StatefulInspectorViewHost
{...makeProps({
servers: [sampleServer],
activeServer: "alpha",
connectionStatus: "connected",
initializeResult: connectedInit,
tools: [unionAppTool],
deepLink: {
serverId: "deep-link",
serverConfig: {
type: "streamable-http",
url: "https://example.com/mcp",
},
openApp: "notify",
// Names the SECOND branch. A shallow default-then-overlay would
// seed the first branch's fields underneath these values.
appArgs: { kind: "sms", phone: "555-0100" },
autoOpen: false,
},
})}
/>,
);
// 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(
<StatefulInspectorViewHost