diff --git a/clients/web/src/components/elements/ContentViewer/ContentViewer.test.tsx b/clients/web/src/components/elements/ContentViewer/ContentViewer.test.tsx index 53366fdf..bfd65add 100644 --- a/clients/web/src/components/elements/ContentViewer/ContentViewer.test.tsx +++ b/clients/web/src/components/elements/ContentViewer/ContentViewer.test.tsx @@ -232,6 +232,29 @@ describe("ContentViewer", () => { expect(screen.getByText(/"a": 1/)).toBeInTheDocument(); }); + // An expanded Protocol entry holds two of these and a list holds many pairs, + // so an unnamed editor leaves a screen reader tabbing through textboxes that + // all announce the same thing. + it("names the JSON editor from jsonLabel", () => { + const block: ContentBlock = { type: "text", text: '{"a":1}' }; + renderWithMantine( + , + ); + expect( + screen.getByLabelText(/tools\/call response JSON/), + ).toBeInTheDocument(); + }); + + it("falls back to a generic name when no label is given", () => { + const block: ContentBlock = { type: "text", text: '{"a":1}' }; + renderWithMantine(); + expect(screen.getByLabelText(/JSON content/)).toBeInTheDocument(); + }); + it("keeps the copy overlay above the JSON editor", () => { const block: ContentBlock = { type: "text", text: '{"a":1}' }; renderWithMantine( diff --git a/clients/web/src/components/elements/ContentViewer/ContentViewer.tsx b/clients/web/src/components/elements/ContentViewer/ContentViewer.tsx index 7d7fbfc3..59789532 100644 --- a/clients/web/src/components/elements/ContentViewer/ContentViewer.tsx +++ b/clients/web/src/components/elements/ContentViewer/ContentViewer.tsx @@ -15,8 +15,8 @@ import { ResourceLinkInfo } from "../ResourceLinkInfo/ResourceLinkInfo"; import { formatJson, formatXml, + formatJsonDocument, getMimeKind, - isJsonDocument, isSafeHref, isTextualKind, looksLikeJson, @@ -60,6 +60,19 @@ export interface ContentViewerProps { * one line — don't pass it for multi-line content. */ wrap?: boolean; + /** + * Accessible name for the read-only JSON editor a JSON payload renders in. + * + * Say what the payload *is* whenever a screen can hold more than one: an + * expanded Protocol entry carries both its parameters and its response, and + * a list of them carries many such pairs — so the default leaves a screen + * reader tabbing through textboxes that all announce the same thing, with + * only the visible section headings to tell them apart. + * + * Reaches the JSON branch only; every other renderer here is not a control + * and takes no name. + */ + jsonLabel?: string; } /** @@ -185,12 +198,24 @@ function HighlightedContent({ * It copies the *original* text, not the pretty-printed form, matching every * other branch here. */ -function JsonContent({ text, copyable }: { text: string; copyable: boolean }) { +function JsonContent({ + text, + formatted, + copyable, + label, +}: { + /** The original payload — what the copy button copies. */ + text: string; + /** Its pretty-printed form, already parsed once by whoever decided to use it. */ + formatted: string; + copyable: boolean; + label: string; +}) { return ( ; + // + // The formatted text comes back from the same call that decided, so the + // payload is parsed once rather than once to test and again to render. + const asJson = wrap ? formatJsonDocument(text) : null; + if (asJson !== null) { + return ( + + ); } const displayText = looksLikeJson(text) ? formatJson(text) : text; return ( @@ -262,11 +300,13 @@ function TextualContent({ mimeType, copyable, wrap, + jsonLabel, }: { text: string; mimeType: string | undefined; copyable: boolean; wrap: boolean; + jsonLabel: string; }) { const kind = mimeType ? getMimeKind(mimeType) : "text"; switch (kind) { @@ -278,9 +318,23 @@ function TextualContent({ // line is exactly what that prop is documented not to be used for, so // that caller keeps the plain renderer. return wrap ? ( - + ) : ( - + ); case "xml": return ( @@ -313,7 +367,14 @@ function TextualContent({ ); default: - return ; + return ( + + ); } } @@ -341,11 +402,13 @@ function ResourceContent({ mimeType, copyable, wrap, + jsonLabel, }: { contents: TextResourceContents | BlobResourceContents; mimeType: string; copyable: boolean; wrap: boolean; + jsonLabel: string; }) { if ("text" in contents) { return ( @@ -354,6 +417,7 @@ function ResourceContent({ mimeType={mimeType} copyable={copyable} wrap={wrap} + jsonLabel={jsonLabel} /> ); } @@ -382,6 +446,7 @@ function ResourceContent({ mimeType={mimeType} copyable={copyable} wrap={wrap} + jsonLabel={jsonLabel} /> ); } @@ -394,11 +459,13 @@ function BlockContent({ mimeType, copyable, wrap, + jsonLabel, }: { block: ContentBlock; mimeType: string | undefined; copyable: boolean; wrap: boolean; + jsonLabel: string; }) { switch (block.type) { case "text": @@ -408,6 +475,7 @@ function BlockContent({ mimeType={mimeType} copyable={copyable} wrap={wrap} + jsonLabel={jsonLabel} /> ); case "image": @@ -448,6 +516,7 @@ export function ContentViewer({ copyable = false, mimeType, wrap = true, + jsonLabel = "JSON content", }: ContentViewerProps) { if (contents) { const effective = @@ -458,6 +527,7 @@ export function ContentViewer({ mimeType={effective} copyable={copyable} wrap={wrap} + jsonLabel={jsonLabel} /> ); } @@ -468,6 +538,7 @@ export function ContentViewer({ mimeType={mimeType} copyable={copyable} wrap={wrap} + jsonLabel={jsonLabel} /> ); } diff --git a/clients/web/src/components/elements/ContentViewer/contentViewerUtils.test.ts b/clients/web/src/components/elements/ContentViewer/contentViewerUtils.test.ts index d7bc4340..731c15c4 100644 --- a/clients/web/src/components/elements/ContentViewer/contentViewerUtils.test.ts +++ b/clients/web/src/components/elements/ContentViewer/contentViewerUtils.test.ts @@ -7,7 +7,7 @@ import { getMimeKind, isSafeHref, isTextualKind, - isJsonDocument, + formatJsonDocument, looksLikeJson, PREVIEW_HTML_CSP, tryDecodeBase64ToBytes, @@ -113,26 +113,26 @@ describe("looksLikeJson", () => { }); }); -describe("isJsonDocument", () => { - it("accepts text that really parses as JSON", () => { - expect(isJsonDocument(' {"a":1}')).toBe(true); - expect(isJsonDocument("[1,2]")).toBe(true); +describe("formatJsonDocument", () => { + it("pretty-prints text that really parses as JSON", () => { + expect(formatJsonDocument(' {"a":1}')).toBe('{\n "a": 1\n}'); + expect(formatJsonDocument("[1,2]")).toBe("[\n 1,\n 2\n]"); }); // The stricter half of the heuristic (#2151). Text that merely *starts* like // JSON must not be handed to the JSON editor: a note beginning `{` would then // be presented as a document, in a JSON gutter, framed as malformed. - it("rejects text that only looks like JSON", () => { - expect(isJsonDocument("{ broken")).toBe(false); - expect(isJsonDocument("[unclosed")).toBe(false); - expect(isJsonDocument("plain")).toBe(false); + it("reports text that only looks like JSON as not JSON", () => { + expect(formatJsonDocument("{ broken")).toBeNull(); + expect(formatJsonDocument("[unclosed")).toBeNull(); + expect(formatJsonDocument("plain")).toBeNull(); }); // A bare scalar is valid JSON but never looked like a document, so it stays // on the plain renderer — otherwise ordinary prose that happens to be a // number would open an editor. it("rejects a scalar that never looked like a document", () => { - expect(isJsonDocument("42")).toBe(false); + expect(formatJsonDocument("42")).toBeNull(); }); }); diff --git a/clients/web/src/components/elements/ContentViewer/contentViewerUtils.ts b/clients/web/src/components/elements/ContentViewer/contentViewerUtils.ts index 32a425f9..5d73fe83 100644 --- a/clients/web/src/components/elements/ContentViewer/contentViewerUtils.ts +++ b/clients/web/src/components/elements/ContentViewer/contentViewerUtils.ts @@ -143,7 +143,8 @@ export function looksLikeJson(text: string): boolean { } /** - * Whether untyped text really is a JSON document, not merely shaped like one. + * Pretty-print untyped text if it really is a JSON document, or report that it + * is not. * * {@link looksLikeJson} only tests the first character, which is enough to * decide whether to *try* pretty-printing — a failed parse falls back to the @@ -152,14 +153,18 @@ export function looksLikeJson(text: string): boolean { * JSON document, in a JSON gutter, with the server's plain text framed as * malformed. A declared `application/json` is different — there the server said * what it sent, so its own broken payload is shown as JSON on purpose. + * + * Returns the formatted text rather than a boolean so the caller does not parse + * a second time to render what this already parsed to decide. Protocol and + * Network entries hand their serialized bodies over as untyped text and are not + * virtualized, so a long history pays for every parse on every render. */ -export function isJsonDocument(text: string): boolean { - if (!looksLikeJson(text)) return false; +export function formatJsonDocument(text: string): string | null { + if (!looksLikeJson(text)) return null; try { - JSON.parse(text); - return true; + return JSON.stringify(JSON.parse(text), null, 2); } catch { - return false; + return null; } } diff --git a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx index 5ba5d9d6..81cde850 100644 --- a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx +++ b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.test.tsx @@ -98,6 +98,44 @@ describe("EditReplayModal", () => { expect(screen.getByText(/Numbers must be finite/)).toBeInTheDocument(); }); + // Replay dispatches through the typed client methods, so a param they have no + // room for is dropped. Saying which, rather than seeding it into the editor, + // is what keeps the modal from inviting an edit that Send discards. + it("names the params replay cannot re-send", () => { + renderWithMantine( + , + ); + expect(screen.getByText(/`_meta` is not re-sent/)).toBeInTheDocument(); + }); + + it("says nothing when every param survives", () => { + renderWithMantine( + , + ); + expect(screen.queryByText(/not re-sent/)).toBeNull(); + }); + + it("agrees with its verb for several dropped params", () => { + renderWithMantine( + , + ); + expect( + screen.getByText(/`_meta`, `extra` are not re-sent/), + ).toBeInTheDocument(); + }); + it("closes without sending when cancelled", async () => { const user = userEvent.setup(); const onSend = vi.fn(); diff --git a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx index 784db9b7..be1dd877 100644 --- a/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx +++ b/clients/web/src/components/groups/EditReplayModal/EditReplayModal.tsx @@ -1,5 +1,5 @@ import { useState } from "react"; -import { Button, Group, Modal, Stack, Text } from "@mantine/core"; +import { Alert, Button, Group, Modal, Stack, Text } from "@mantine/core"; import { JsonEditor } from "../../elements/JsonEditor/JsonEditor"; import { isJsonObject } from "../../../utils/jsonObjectDraft"; import { isSerializableJson } from "@inspector/core/json/jsonUtils.js"; @@ -8,8 +8,22 @@ export interface EditReplayModalProps { opened: boolean; /** The method being replayed, shown so the editor is unambiguous. */ method: string; - /** The originating entry's params, seeded into the editor. */ + /** + * The originating entry's params, seeded into the editor — already reduced to + * the ones replay will actually read. See {@link droppedParamKeys}. + */ params?: Record; + /** + * Param names the entry carried that replay cannot re-send, so the modal can + * say so rather than leave the difference to be discovered. + * + * Replay dispatches through the typed `InspectorClient` methods rather than + * re-sending the captured frame, and those signatures have no room for + * `_meta` (or for anything but `cursor` on a list request). Seeding the + * editor with them would invite an edit that Send silently discards, which is + * the one thing an inspector must not do. + */ + droppedParamKeys?: string[]; /** * Re-issue the request with these params. `null` means "send no params", * which is a request an empty editor can legitimately express — see @@ -27,6 +41,17 @@ const MethodText = Text.withProps({ const Footer = Group.withProps({ justify: "flex-end", gap: "sm" }); +// Names the params replay cannot carry. `light` and gray rather than a warning +// colour: nothing is wrong and there is nothing for the user to fix — it is a +// property of how replay dispatches. +const DroppedNote = Alert.withProps({ variant: "light", color: "gray" }); + +/** "`a`, `b` are" / "`a` is" — the list, agreeing with its verb. */ +function formatDroppedKeys(keys: string[]): string { + const names = keys.map((key) => `\`${key}\``).join(", "); + return `${names} ${keys.length === 1 ? "is" : "are"}`; +} + /** * What the editor's text currently means: the params to send, or why they * cannot be sent. @@ -75,6 +100,7 @@ function parseParamsDraft( function EditReplayForm({ method, params, + droppedParamKeys, onSend, onClose, }: Omit) { @@ -86,6 +112,15 @@ function EditReplayForm({ return ( {method} + {droppedParamKeys && droppedParamKeys.length > 0 && ( + + + {`Replay re-issues this request through the client's own ${method} + method, which carries only the fields below, so + ${formatDroppedKeys(droppedParamKeys)} not re-sent.`} + + + )} diff --git a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx index 7ef790bc..399cdfe4 100644 --- a/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx +++ b/clients/web/src/components/groups/NetworkEntry/NetworkEntry.tsx @@ -371,9 +371,16 @@ const RevealButton = Button.withProps({ function BodyPreview({ body, contentType, + label, }: { body: string; contentType?: string; + /** + * What this body *is* ("Request body", "Response body"), so a JSON payload's + * editor is named. An expanded entry holds both, and the list holds many + * pairs, so unnamed they all announce identically to a screen reader. + */ + label: string; }) { // Reveal state for masked secrets. Hooks run before any early return so the // order stays stable across the too-large / has-secrets branches. The reveal @@ -413,7 +420,13 @@ function BodyPreview({ } if (!hasSecrets) { - return ; + return ( + + ); } const shown = revealed ? body : masked; @@ -432,7 +445,11 @@ function BodyPreview({ {revealed ? "Hide" : "Reveal"} - + ); } @@ -587,6 +604,7 @@ export function NetworkEntry({ key={`${entry.requestHeaders["content-type"] ?? ""}|${entry.requestBody}`} body={entry.requestBody} contentType={entry.requestHeaders["content-type"]} + label="Request body" /> )} @@ -604,6 +622,7 @@ export function NetworkEntry({ key={`${entry.responseHeaders?.["content-type"] ?? ""}|${entry.responseBody}`} body={entry.responseBody} contentType={entry.responseHeaders?.["content-type"]} + label="Response body" /> ) : ( diff --git a/clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.test.tsx b/clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.test.tsx index f86c6940..c7f19756 100644 --- a/clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.test.tsx +++ b/clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.test.tsx @@ -221,6 +221,28 @@ describe("ProtocolEntry", () => { }); }); + // The entry's frame carries `_meta.progressToken`, which replay cannot + // re-send — so it must not appear in the editor as though it could. + it("seeds only the params replay will actually read", async () => { + const user = userEvent.setup(); + const withMeta: MessageEntry = { + ...successEntry, + message: { + ...successEntry.message, + params: { + name: "get_weather", + arguments: { city: "San Francisco" }, + _meta: { progressToken: 2 }, + }, + }, + } as MessageEntry; + renderWithMantine(); + await openEditor(user); + + expect(getAceText()).not.toContain("_meta"); + expect(screen.getByText(/`_meta` is not re-sent/)).toBeInTheDocument(); + }); + it("replays with the edited params", async () => { const user = userEvent.setup(); const onReplay = vi.fn(); @@ -419,6 +441,24 @@ describe("ProtocolEntry", () => { ).toBeInTheDocument(); }); + // Both payloads render the same kind of control, so without distinct names a + // screen reader announces them identically. + it("names the parameters and response editors distinctly", () => { + renderWithMantine( + , + ); + expect( + screen.getByLabelText(/tools\/call request parameters JSON/), + ).toBeInTheDocument(); + expect( + screen.getByLabelText(/tools\/call response JSON/), + ).toBeInTheDocument(); + }); + it("does not render the Parameters section when message has no params", () => { renderWithMantine( setIsEditingReplay(false)} /> @@ -513,6 +520,10 @@ export function ProtocolEntry({ text: serializeMessage(requestParams), }} copyable + // Named, because an expanded entry holds two of these and a + // list holds many pairs — unnamed they all announce the same + // thing, leaving only the visible headings to tell them apart. + jsonLabel={`${method} request parameters JSON`} /> )} @@ -525,6 +536,7 @@ export function ProtocolEntry({ text: serializeMessage(entry.response), }} copyable + jsonLabel={`${method} response JSON`} /> )} diff --git a/clients/web/src/lib/protocolReplay.test.ts b/clients/web/src/lib/protocolReplay.test.ts index a3b90fb2..b60b0174 100644 --- a/clients/web/src/lib/protocolReplay.test.ts +++ b/clients/web/src/lib/protocolReplay.test.ts @@ -4,6 +4,7 @@ import type { MessageEntry } from "@inspector/core/mcp/types.js"; import { messagesToLogEntries, replayProtocolRequest, + replayableParams, type ReplayClient, } from "./protocolReplay"; @@ -61,6 +62,79 @@ describe("messagesToLogEntries", () => { }); }); +// The projection the Edit-and-replay editor seeds from. It is the other half +// of the dispatch switch: replay re-issues through the typed client methods +// rather than re-sending the frame, so anything those signatures have no room +// for is dropped — and an editor that offered it would invite an edit that Send +// silently discards. +describe("replayableParams", () => { + it("keeps the name and arguments of a tool call, and drops _meta", () => { + expect( + replayableParams("tools/call", { + name: "get_weather", + arguments: { city: "Boston" }, + _meta: { progressToken: 2 }, + }), + ).toEqual({ + params: { name: "get_weather", arguments: { city: "Boston" } }, + dropped: ["_meta"], + }); + }); + + it("keeps the name and arguments of a prompt get", () => { + expect( + replayableParams("prompts/get", { name: "greet", arguments: { a: 1 } }), + ).toEqual({ + params: { name: "greet", arguments: { a: 1 } }, + dropped: [], + }); + }); + + it("keeps only the uri of a resource read", () => { + expect( + replayableParams("resources/read", { uri: "file:///a", _meta: {} }), + ).toEqual({ params: { uri: "file:///a" }, dropped: ["_meta"] }); + }); + + it("keeps only a string cursor on a list request", () => { + expect( + replayableParams("tools/list", { cursor: "abc", _meta: {} }), + ).toEqual({ params: { cursor: "abc" }, dropped: ["_meta"] }); + }); + + // The dispatcher ignores a non-string cursor, so keeping it would put a value + // in the editor that changes nothing about what is sent. + it("drops a cursor that is not a string", () => { + expect(replayableParams("resources/list", { cursor: 7 })).toEqual({ + params: undefined, + dropped: ["cursor"], + }); + }); + + it("keeps nothing for ping", () => { + expect(replayableParams("ping", { _meta: {} })).toEqual({ + params: undefined, + dropped: ["_meta"], + }); + }); + + it("reports no params for a request that carried none", () => { + expect(replayableParams("tools/list", undefined)).toEqual({ + params: undefined, + dropped: [], + }); + }); + + // A name only counts as dropped if it was there: an absent `arguments` is not + // something the user is being told they will lose. + it("does not report absent names as dropped", () => { + expect(replayableParams("tools/call", { name: "echo" })).toEqual({ + params: { name: "echo" }, + dropped: [], + }); + }); +}); + describe("replayProtocolRequest", () => { const tool: Tool = { name: "echo", inputSchema: { type: "object" } }; diff --git a/clients/web/src/lib/protocolReplay.ts b/clients/web/src/lib/protocolReplay.ts index 609ea430..5bee9026 100644 --- a/clients/web/src/lib/protocolReplay.ts +++ b/clients/web/src/lib/protocolReplay.ts @@ -70,6 +70,69 @@ export type ReplayClient = Pick< */ export type ReplayParamsOverride = Record | null | undefined; +/** + * The params of a replayed request that `replayProtocolRequest` actually reads, + * per method. + * + * This is the other half of the switch below, and the two must move together. + * Replay does not re-send a captured frame verbatim — it dispatches through the + * typed `InspectorClient` methods, which take named arguments. Everything those + * signatures have no room for is dropped: `_meta` on any method (a captured + * `tools/call` carries `_meta.progressToken`), and every key of a list request + * other than `cursor`. + * + * That is invisible and harmless for one-click Replay, which re-sends what it + * was given. It is neither once the params are put in front of the user to + * edit (#2151): an editor showing `_meta` invites them to change it, and Send + * would then transmit something other than what they were looking at. So the + * Edit-and-replay modal seeds from *this* rather than from the raw frame, and + * names what it left out. + */ +export function replayableParams( + method: string, + params: Record | undefined, +): { params: Record | undefined; dropped: string[] } { + if (!params) return { params: undefined, dropped: [] }; + + const keep = (...names: string[]): string[] => + names.filter((name) => Object.hasOwn(params, name)); + + let kept: string[]; + switch (method) { + case "tools/call": + case "prompts/get": + kept = keep("name", "arguments"); + break; + case "resources/read": + kept = keep("uri"); + break; + case "tools/list": + case "prompts/list": + case "resources/list": + case "resources/templates/list": + case "tasks/list": + // Only a *string* cursor survives: the dispatcher ignores any other type, + // so keeping it would put a value in the editor that changes nothing. + kept = typeof params.cursor === "string" ? ["cursor"] : []; + break; + default: + // `ping` takes nothing, and an unreplayable method never reaches here. + kept = []; + break; + } + + const dropped = Object.keys(params).filter((name) => !kept.includes(name)); + // `fromEntries`, never an assignment loop: an argument legitimately named + // `__proto__` would otherwise reach the prototype setter. + return { + params: + kept.length > 0 + ? Object.fromEntries(kept.map((name) => [name, params[name]])) + : undefined, + dropped, + }; +} + export async function replayProtocolRequest( client: ReplayClient, method: string,