From 4b119c9a958ad702e512af1fa1fe3c06d4fd2ea4 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 18:24:09 -0400 Subject: [PATCH] chore(web): address Copilot review on #2127 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move REPLAYABLE_PROTOCOL_METHODS / isReplayableProtocolMethod out of components/groups/protocolUtils.ts into utils/replayableProtocolMethods.ts. It is a pure predicate over a string set, so by the project's own lib-vs-utils rule it never belonged under components/ — and keeping it there was what made lib/protocolReplay.ts import 'up' into the component graph. Both consumers now import from utils/; the predicate keeps its own test. - Give replayProtocolRequest an exported ReplayClient = Pick naming the nine methods it calls, so a caller (and the test) can satisfy the real contract instead of casting a stub into a whole client. - Rebuild the protocolReplay fixtures as plain typed MessageEntry values. The double cast had been hiding a real defect: MessageEntry.timestamp is a Date and the fixture passed a number, so the assertion confirmed a shape messagesToLogEntries cannot produce. - Replace the window casts in authToken.test.ts with Reflect.set / Reflect.deleteProperty. No 'as unknown as' remains in either test. - Drop the stale 'floats top-right' sentence on ReAuthBannerBar, which moved verbatim and arrived contradicting the paragraph below it. Declined: re-homing ReAuthBannerBar's transform/zIndex as a theme variant. 'styles' is the Styles API rather than an inline style, Mantine exposes neither property as a prop, and a styling change is out of scope for a phase whose safety argument is that nothing was edited. Signed-off-by: cliffhall --- .../groups/ProtocolEntry/ProtocolEntry.tsx | 2 +- .../groups/ReAuthBanner/ReAuthBannerBar.tsx | 12 +-- .../src/components/groups/protocolUtils.ts | 24 ----- clients/web/src/lib/authToken.test.ts | 20 ++-- clients/web/src/lib/protocolReplay.test.ts | 93 +++++++++++-------- clients/web/src/lib/protocolReplay.ts | 25 ++++- .../utils/replayableProtocolMethods.test.ts | 37 ++++++++ .../src/utils/replayableProtocolMethods.ts | 28 ++++++ 8 files changed, 155 insertions(+), 86 deletions(-) create mode 100644 clients/web/src/utils/replayableProtocolMethods.test.ts create mode 100644 clients/web/src/utils/replayableProtocolMethods.ts diff --git a/clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.tsx b/clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.tsx index 73edda6c..0df02f05 100644 --- a/clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.tsx +++ b/clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.tsx @@ -30,8 +30,8 @@ import { extractMethod, extractResultType, extractSubscriptionId, - isReplayableProtocolMethod, } from "../protocolUtils.js"; +import { isReplayableProtocolMethod } from "../../../utils/replayableProtocolMethods"; export interface ProtocolEntryProps { entry: MessageEntry; diff --git a/clients/web/src/components/groups/ReAuthBanner/ReAuthBannerBar.tsx b/clients/web/src/components/groups/ReAuthBanner/ReAuthBannerBar.tsx index 2fda87de..be0b51dd 100644 --- a/clients/web/src/components/groups/ReAuthBanner/ReAuthBannerBar.tsx +++ b/clients/web/src/components/groups/ReAuthBanner/ReAuthBannerBar.tsx @@ -3,12 +3,12 @@ import { Paper } from "@mantine/core"; // The re-auth popup. A `Paper` so every static style is a prop; the stacking // order goes through `styles.root` since Mantine has no `z` prop. // -// Floats top-right rather than spanning the top as a sticky full-bleed bar. -// The bar cost the whole view a band of vertical space for what is a -// notification about one server, and it sat directly above the monitoring -// sidebar that an OAuth failure now opens (#2108) — the two things a user needs -// at once here are this affordance and those requests, so it must not push them -// around. `fixed`, not `sticky`, so scrolling the server list leaves it put. +// Floats rather than spanning the top as a sticky full-bleed bar. The bar cost +// the whole view a band of vertical space for what is a notification about one +// server, and it sat directly above the monitoring sidebar that an OAuth +// failure now opens (#2108) — the two things a user needs at once here are this +// affordance and those requests, so it must not push them around. `fixed`, not +// `sticky`, so scrolling the server list leaves it put. // // Centered, and deliberately WITHOUT an overlay. Every corner is spoken for — // the toast stack owns bottom-right (see `main.tsx`), the right edge is the diff --git a/clients/web/src/components/groups/protocolUtils.ts b/clients/web/src/components/groups/protocolUtils.ts index 845f3921..cc778ff2 100644 --- a/clients/web/src/components/groups/protocolUtils.ts +++ b/clients/web/src/components/groups/protocolUtils.ts @@ -13,30 +13,6 @@ export function extractMethod(entry: MessageEntry): MessageMethod { return "response"; } -/** - * Request methods the Protocol Replay action can re-issue (client→server reads - * and calls). Server→client requests (roots/list, sampling, elicitation) and - * side-effectful methods (logging/setLevel, subscribe) are intentionally - * excluded. Single source of truth: `ProtocolEntry` hides the Replay button for - * anything not listed here, and App's `replayProtocolRequest` gates dispatch on - * the same set. - */ -export const REPLAYABLE_PROTOCOL_METHODS: ReadonlySet = new Set([ - "tools/call", - "prompts/get", - "resources/read", - "tools/list", - "prompts/list", - "resources/list", - "resources/templates/list", - "tasks/list", - "ping", -]); - -export function isReplayableProtocolMethod(method: string): boolean { - return REPLAYABLE_PROTOCOL_METHODS.has(method); -} - // --- Modern-era (2026-07-28) message vocabulary ----------------------------- // // The modern era changes the over-the-wire conversation (spec §7.2–7.4): every diff --git a/clients/web/src/lib/authToken.test.ts b/clients/web/src/lib/authToken.test.ts index 07282e7e..dc63bb6b 100644 --- a/clients/web/src/lib/authToken.test.ts +++ b/clients/web/src/lib/authToken.test.ts @@ -44,9 +44,7 @@ describe("getAuthToken", () => { beforeEach(() => { setSearch(""); window.sessionStorage.clear(); - delete (window as unknown as Record)[ - INSPECTOR_API_TOKEN_GLOBAL - ]; + Reflect.deleteProperty(window, INSPECTOR_API_TOKEN_GLOBAL); }); afterEach(() => { @@ -54,14 +52,11 @@ describe("getAuthToken", () => { restoreSessionStorage(); setSearch(""); window.sessionStorage.clear(); - delete (window as unknown as Record)[ - INSPECTOR_API_TOKEN_GLOBAL - ]; + Reflect.deleteProperty(window, INSPECTOR_API_TOKEN_GLOBAL); }); it("prefers the injected global and persists it", () => { - (window as unknown as Record)[INSPECTOR_API_TOKEN_GLOBAL] = - "from-global"; + Reflect.set(window, INSPECTOR_API_TOKEN_GLOBAL, "from-global"); setSearch(`?${KEY}=from-url`); window.sessionStorage.setItem(KEY, "from-storage"); @@ -70,14 +65,12 @@ describe("getAuthToken", () => { }); it("ignores a non-string or empty global and falls through to the URL", () => { - (window as unknown as Record)[INSPECTOR_API_TOKEN_GLOBAL] = - ""; + Reflect.set(window, INSPECTOR_API_TOKEN_GLOBAL, ""); setSearch(`?${KEY}=from-url`); expect(getAuthToken()).toBe("from-url"); - (window as unknown as Record)[INSPECTOR_API_TOKEN_GLOBAL] = - 123; + Reflect.set(window, INSPECTOR_API_TOKEN_GLOBAL, 123); expect(getAuthToken()).toBe("from-url"); expect(window.sessionStorage.getItem(KEY)).toBe("from-url"); }); @@ -96,8 +89,7 @@ describe("getAuthToken", () => { throw new Error("blocked"); }); stubSessionStorage({ setItem }); - (window as unknown as Record)[INSPECTOR_API_TOKEN_GLOBAL] = - "from-global"; + Reflect.set(window, INSPECTOR_API_TOKEN_GLOBAL, "from-global"); expect(getAuthToken()).toBe("from-global"); expect(setItem).toHaveBeenCalledWith(KEY, "from-global"); diff --git a/clients/web/src/lib/protocolReplay.test.ts b/clients/web/src/lib/protocolReplay.test.ts index 619deba9..a3b90fb2 100644 --- a/clients/web/src/lib/protocolReplay.test.ts +++ b/clients/web/src/lib/protocolReplay.test.ts @@ -1,16 +1,24 @@ import { describe, expect, it, vi } from "vitest"; import type { Tool } from "@modelcontextprotocol/client"; -import type { InspectorClient } from "@inspector/core/mcp/index.js"; import type { MessageEntry } from "@inspector/core/mcp/types.js"; -import { messagesToLogEntries, replayProtocolRequest } from "./protocolReplay"; +import { + messagesToLogEntries, + replayProtocolRequest, + type ReplayClient, +} from "./protocolReplay"; describe("messagesToLogEntries", () => { - const notification = (method: string, params: unknown): MessageEntry => - ({ - direction: "notification", - timestamp: 1000, - message: { jsonrpc: "2.0", method, params }, - }) as unknown as MessageEntry; + const at = new Date("2026-01-01T00:00:00Z"); + + const notification = ( + method: string, + params: Record, + ): MessageEntry => ({ + id: `n-${method}`, + timestamp: at, + direction: "notification", + message: { jsonrpc: "2.0", method, params }, + }); it("keeps only notifications/message and carries the timestamp", () => { const entries = messagesToLogEntries([ @@ -18,49 +26,62 @@ describe("messagesToLogEntries", () => { notification("notifications/progress", { progress: 1 }), ]); expect(entries).toEqual([ - { receivedAt: 1000, params: { level: "info", data: "hello" } }, + { receivedAt: at, params: { level: "info", data: "hello" } }, ]); }); it("skips requests and responses", () => { - const request = { + const request: MessageEntry = { + id: "r-1", + timestamp: at, direction: "request", - timestamp: 1, + // A request frame carrying the same method — only `direction` should + // decide, so this must still be skipped. message: { jsonrpc: "2.0", id: 1, method: "notifications/message" }, - } as unknown as MessageEntry; - const response = { + }; + const response: MessageEntry = { + id: "r-2", + timestamp: at, direction: "response", - timestamp: 2, message: { jsonrpc: "2.0", id: 1, result: {} }, - } as unknown as MessageEntry; + }; expect(messagesToLogEntries([request, response])).toEqual([]); }); - it("skips a notification carrying no method", () => { - const malformed = { + it("skips a frame labelled a notification that carries no method", () => { + // `direction` and the frame shape can disagree — the log records what + // arrived, so the `"method" in message` guard has to hold on its own. + const malformed: MessageEntry = { + id: "m-1", + timestamp: at, direction: "notification", - timestamp: 3, - message: { jsonrpc: "2.0", result: {} }, - } as unknown as MessageEntry; + message: { jsonrpc: "2.0", id: 1, result: {} }, + }; expect(messagesToLogEntries([malformed])).toEqual([]); }); }); describe("replayProtocolRequest", () => { - const tool = { name: "echo", inputSchema: { type: "object" } } as Tool; + const tool: Tool = { name: "echo", inputSchema: { type: "object" } }; + // Typed against `ReplayClient`, so the mock has to satisfy the same contract + // the function declares rather than being cast into a whole InspectorClient. + // Each mock is typed against the real method signature, so a drift in + // `InspectorClient` surfaces here rather than being absorbed by a cast. None + // of them needs a resolved value: `replayProtocolRequest` awaits the call and + // discards the result, so what is asserted is the dispatch, not the payload. function makeClient() { return { - callTool: vi.fn().mockResolvedValue({}), - getPrompt: vi.fn().mockResolvedValue({}), - readResource: vi.fn().mockResolvedValue({}), - listTools: vi.fn().mockResolvedValue({}), - listPrompts: vi.fn().mockResolvedValue({}), - listResources: vi.fn().mockResolvedValue({}), - listResourceTemplates: vi.fn().mockResolvedValue({}), - listRequestorTasks: vi.fn().mockResolvedValue({}), - ping: vi.fn().mockResolvedValue({}), - }; + callTool: vi.fn(), + getPrompt: vi.fn(), + readResource: vi.fn(), + listTools: vi.fn(), + listPrompts: vi.fn(), + listResources: vi.fn(), + listResourceTemplates: vi.fn(), + listRequestorTasks: vi.fn(), + ping: vi.fn(), + } satisfies ReplayClient; } const replay = ( @@ -68,13 +89,7 @@ describe("replayProtocolRequest", () => { method: string, params?: Record, tools: Tool[] = [tool], - ) => - replayProtocolRequest( - client as unknown as InspectorClient, - method, - params, - tools, - ); + ) => replayProtocolRequest(client, method, params, tools); it("refuses a method outside the shared replayable set", async () => { const client = makeClient(); @@ -153,7 +168,7 @@ describe("replayProtocolRequest", () => { expect(client[fn]).toHaveBeenCalledWith("page-2"); }); - it("passes no cursor when the entry carried none", async () => { + it("passes no cursor when the entry carried none, or carried a non-string", async () => { const client = makeClient(); await expect(replay(client, "tools/list", undefined)).resolves.toBeNull(); expect(client.listTools).toHaveBeenCalledWith(undefined); diff --git a/clients/web/src/lib/protocolReplay.ts b/clients/web/src/lib/protocolReplay.ts index 8d874093..2eff86a5 100644 --- a/clients/web/src/lib/protocolReplay.ts +++ b/clients/web/src/lib/protocolReplay.ts @@ -5,8 +5,11 @@ import type { import type { InspectorClient } from "@inspector/core/mcp/index.js"; import type { JsonValue } from "@inspector/core/mcp/index.js"; import type { MessageEntry } from "@inspector/core/mcp/types.js"; +// Type-only, so this creates no runtime edge into `components/` and the +// `components → lib → utils` direction still holds: `LogEntryData` is the shape +// the Logs screen renders, and this module exists to produce exactly that. import type { LogEntryData } from "../components/elements/LogEntry/LogEntry"; -import { isReplayableProtocolMethod } from "../components/groups/protocolUtils.js"; +import { isReplayableProtocolMethod } from "../utils/replayableProtocolMethods"; // Derive `LogEntryData[]` from the MessageLog by filtering for the // `notifications/message` notifications the server emits in response to @@ -37,8 +40,26 @@ export function messagesToLogEntries(messages: MessageEntry[]): LogEntryData[] { // intentionally does NOT touch the Tools/Prompts/Resources panels. Returns a // human-readable reason when the entry can't be replayed (unsupported method, // or a tool that's no longer present), or null on a dispatched replay. +/** + * The slice of `InspectorClient` a replay actually reaches. Naming it — rather + * than taking the whole client — is what lets a caller (and a test) supply a + * value that satisfies the real contract instead of casting one in. + */ +export type ReplayClient = Pick< + InspectorClient, + | "callTool" + | "getPrompt" + | "readResource" + | "listTools" + | "listPrompts" + | "listResources" + | "listResourceTemplates" + | "listRequestorTasks" + | "ping" +>; + export async function replayProtocolRequest( - client: InspectorClient, + client: ReplayClient, method: string, params: Record | undefined, tools: Tool[], diff --git a/clients/web/src/utils/replayableProtocolMethods.test.ts b/clients/web/src/utils/replayableProtocolMethods.test.ts new file mode 100644 index 00000000..153a35ac --- /dev/null +++ b/clients/web/src/utils/replayableProtocolMethods.test.ts @@ -0,0 +1,37 @@ +import { describe, expect, it } from "vitest"; +import { + isReplayableProtocolMethod, + REPLAYABLE_PROTOCOL_METHODS, +} from "./replayableProtocolMethods"; + +describe("isReplayableProtocolMethod", () => { + it("admits every client→server read and call in the set", () => { + for (const method of REPLAYABLE_PROTOCOL_METHODS) { + expect(isReplayableProtocolMethod(method)).toBe(true); + } + expect([...REPLAYABLE_PROTOCOL_METHODS].sort()).toEqual([ + "ping", + "prompts/get", + "prompts/list", + "resources/list", + "resources/read", + "resources/templates/list", + "tasks/list", + "tools/call", + "tools/list", + ]); + }); + + it("excludes server→client requests and side-effectful methods", () => { + for (const method of [ + "roots/list", + "sampling/createMessage", + "elicitation/create", + "logging/setLevel", + "resources/subscribe", + "initialize", + ]) { + expect(isReplayableProtocolMethod(method)).toBe(false); + } + }); +}); diff --git a/clients/web/src/utils/replayableProtocolMethods.ts b/clients/web/src/utils/replayableProtocolMethods.ts new file mode 100644 index 00000000..87b27284 --- /dev/null +++ b/clients/web/src/utils/replayableProtocolMethods.ts @@ -0,0 +1,28 @@ +/** + * Request methods the Protocol Replay action can re-issue (client→server reads + * and calls). Server→client requests (roots/list, sampling, elicitation) and + * side-effectful methods (logging/setLevel, subscribe) are intentionally + * excluded. Single source of truth: `ProtocolEntry` hides the Replay button for + * anything not listed here, and `replayProtocolRequest` gates dispatch on the + * same set. + * + * Lives in `utils/` rather than beside the Protocol components because it is a + * pure predicate over a string set with two consumers on opposite sides of the + * layering: a component and `lib/protocolReplay.ts`. Keeping it here is what + * lets the adapter reach it without importing "up" into `components/`. + */ +export const REPLAYABLE_PROTOCOL_METHODS: ReadonlySet = new Set([ + "tools/call", + "prompts/get", + "resources/read", + "tools/list", + "prompts/list", + "resources/list", + "resources/templates/list", + "tasks/list", + "ping", +]); + +export function isReplayableProtocolMethod(method: string): boolean { + return REPLAYABLE_PROTOCOL_METHODS.has(method); +}