Skip to content

Commit e42968b

Browse files
committed
chore(web): extract App.tsx's leaf hooks (#2128)
Phase 1 of #2126. Six hooks move out of `clients/web/src/App.tsx`, taking its component body from 4,891 to 4,267 lines with no behavior change: - useThemeToggle — the light/dark toggle - useTabUiState — the nine per-screen `ui` objects, the active tab, the pinned-Protocol ids, and the session reset - useInspectorStores — the twelve state managers as one create/destroy lifecycle, their `core/react` hooks, and the pagination-mode selection - useExportActions — the eleven Clear / Export / Replay handlers - useProgressToasts — `notifications/progress` toasts - useTaskToasts — task-status toasts, the cancel confirmation, the taskId -> progress map, and the in-flight tool-call taskId ref Holding the stores in one slot collapses `setupClientForServer`'s and `onConfirmRemove`'s dependency arrays from twelve entries to two stable callbacks; `destroyStores` reads the live set through a ref mirror rather than the state it also writes. All six hooks are at 100% on all four coverage dimensions, no `v8 ignore` needed. Also refreshes the now-stale "~4.5k-line" description of App.tsx in AGENTS.md and the coverage `include` comment. Signed-off-by: cliffhall <cliff@futurescale.com>
1 parent 78bad6a commit e42968b

15 files changed

Lines changed: 2382 additions & 773 deletions

‎AGENTS.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1058,7 +1058,7 @@ The top-level **`src/types/`** is a sibling of both and is not the place for new
10581058

10591059
Cross-directory imports point **one way, `lib → utils`** (infra depends on pure helpers, never the reverse). Keep it that way: if a `utils/` module needs a type currently exported from a `lib/` module, declare the type in `utils/` and re-export it from `lib/` (as `pendingReauth` owns `OAuthResumeAuthKind` and `oauthResume` re-exports it), rather than importing "up" from `utils` into `lib`.
10601060

1061-
Nothing **enforces** the boundary: no path alias keys off it, and the coverage `include` in `clients/web/vite.config.ts` lists **both** `src/lib/**` and `src/utils/**`, so a move between them is coverage-neutral (this is why the refactor was gate-safe). It's a human-legible signal at import time, valuable in a codebase this test-heavy (the ≥90% per-file gate). Note that `include` is a **whitelist** — it names `components`/`hooks`/`theme`/`lib`/`utils`/`server` (plus the `core/*` runtime; `hooks` and `theme` were added in #1787), so a module placed **outside** those directories (`types/`, `App.tsx`, or a brand-new grab-bag) falls out of the ≥90 gate entirely, silently. The **deliberate, documented** top-level-file exceptions are `src/App.tsx` — a ~4.5k-line composition root at ~42% branch coverage (gating it is a dedicated testing/decomposition effort, not a whitelist tweak) — and the `src/main.tsx` / `src/index.ts` bootstraps (browser `createRoot` render and the bin `runWeb` re-export, the analog of `clients/cli`'s excluded `src/index.ts`). All three are called out in a comment on the `include` array itself rather than left silent. When adding a module, place it by the rule and keep it inside a gated directory; when it genuinely mixes both (e.g. `downloadFile` bundles DOM-side-effect helpers with a couple of pure ones), keep it whole on its dominant side (`lib`) rather than splitting hairs.
1061+
Nothing **enforces** the boundary: no path alias keys off it, and the coverage `include` in `clients/web/vite.config.ts` lists **both** `src/lib/**` and `src/utils/**`, so a move between them is coverage-neutral (this is why the refactor was gate-safe). It's a human-legible signal at import time, valuable in a codebase this test-heavy (the ≥90% per-file gate). Note that `include` is a **whitelist** — it names `components`/`hooks`/`theme`/`lib`/`utils`/`server` (plus the `core/*` runtime; `hooks` and `theme` were added in #1787), so a module placed **outside** those directories (`types/`, `App.tsx`, or a brand-new grab-bag) falls out of the ≥90 gate entirely, silently. The **deliberate, documented** top-level-file exceptions are `src/App.tsx` — a ~4.3k-line composition root at ~42% branch coverage (gating it is a dedicated testing/decomposition effort, not a whitelist tweak) — and the `src/main.tsx` / `src/index.ts` bootstraps (browser `createRoot` render and the bin `runWeb` re-export, the analog of `clients/cli`'s excluded `src/index.ts`). All three are called out in a comment on the `include` array itself rather than left silent. When adding a module, place it by the rule and keep it inside a gated directory; when it genuinely mixes both (e.g. `downloadFile` bundles DOM-side-effect helpers with a couple of pure ones), keep it whole on its dominant side (`lib`) rather than splitting hairs.
10621062

10631063
## React instructions
10641064

‎clients/web/src/App.tsx‎

Lines changed: 150 additions & 771 deletions
Large diffs are not rendered by default.
Lines changed: 317 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,317 @@
1+
import { describe, it, expect, beforeEach, vi } from "vitest";
2+
import { act } from "react";
3+
import type { Tool } from "@modelcontextprotocol/client";
4+
import type { MessageEntry } from "@inspector/core/mcp/types.js";
5+
import type { InspectorClient } from "@inspector/core/mcp/index.js";
6+
import { renderWithMantine } from "../test/renderWithMantine";
7+
import {
8+
useExportActions,
9+
type ExportActions,
10+
type UseExportActionsParams,
11+
} from "./useExportActions";
12+
13+
const { notificationsMock } = vi.hoisted(() => ({
14+
notificationsMock: { show: vi.fn(), update: vi.fn(), hide: vi.fn() },
15+
}));
16+
vi.mock("@mantine/notifications", () => ({
17+
notifications: notificationsMock,
18+
}));
19+
20+
// The download is a DOM side effect; the assertions here are about *what* was
21+
// serialized and under which filename, so the writer itself is spied on.
22+
const { downloadJsonFile } = vi.hoisted(() => ({
23+
downloadJsonFile: vi.fn(),
24+
}));
25+
vi.mock("../lib/downloadFile", async (importOriginal) => ({
26+
...(await importOriginal<typeof import("../lib/downloadFile")>()),
27+
downloadJsonFile,
28+
}));
29+
30+
const { replayProtocolRequest } = vi.hoisted(() => ({
31+
replayProtocolRequest: vi.fn(),
32+
}));
33+
vi.mock("../lib/protocolReplay", () => ({ replayProtocolRequest }));
34+
35+
const request = (id: string, method: string): MessageEntry =>
36+
({
37+
id,
38+
direction: "request",
39+
timestamp: 0,
40+
message: { jsonrpc: "2.0", id: 1, method, params: { a: 1 } },
41+
}) as unknown as MessageEntry;
42+
43+
const logNotification = (id: string): MessageEntry =>
44+
({
45+
id,
46+
direction: "notification",
47+
timestamp: 0,
48+
message: { jsonrpc: "2.0", method: "notifications/message" },
49+
}) as unknown as MessageEntry;
50+
51+
function harness(overrides: Partial<UseExportActionsParams> = {}) {
52+
const messageLogState = { clearMessages: vi.fn() };
53+
const fetchRequestLogState = { clearFetchRequests: vi.fn() };
54+
const stderrLogState = { clearStderrLogs: vi.fn() };
55+
const setPinnedProtocolIds = vi.fn();
56+
const params: UseExportActionsParams = {
57+
activeServerId: "srv",
58+
messageLogState:
59+
messageLogState as unknown as UseExportActionsParams["messageLogState"],
60+
fetchRequestLogState:
61+
fetchRequestLogState as unknown as UseExportActionsParams["fetchRequestLogState"],
62+
stderrLogState:
63+
stderrLogState as unknown as UseExportActionsParams["stderrLogState"],
64+
messages: [],
65+
fetchRequests: [],
66+
logs: [],
67+
stderrLogs: [],
68+
pinnedProtocolIds: new Set<string>(),
69+
setPinnedProtocolIds,
70+
inspectorClient: {} as InspectorClient,
71+
tools: [] as Tool[],
72+
...overrides,
73+
};
74+
let latest: ExportActions | undefined;
75+
function Probe() {
76+
latest = useExportActions(params);
77+
return null;
78+
}
79+
renderWithMantine(<Probe />);
80+
return {
81+
messageLogState,
82+
fetchRequestLogState,
83+
stderrLogState,
84+
setPinnedProtocolIds,
85+
run: (fn: (api: ExportActions) => void) =>
86+
act(() => {
87+
if (!latest) throw new Error("hook did not render");
88+
fn(latest);
89+
}),
90+
/** The JSON handed to the writer, parsed. */
91+
exported: () => JSON.parse(downloadJsonFile.mock.calls[0][1] as string),
92+
filename: () => downloadJsonFile.mock.calls[0][0] as string,
93+
};
94+
}
95+
96+
describe("useExportActions", () => {
97+
beforeEach(() => {
98+
downloadJsonFile.mockClear();
99+
replayProtocolRequest.mockReset();
100+
notificationsMock.show.mockClear();
101+
});
102+
103+
describe("clear", () => {
104+
it("clears only the log notifications from the message log", () => {
105+
const h = harness();
106+
h.run((api) => api.onClearLogs());
107+
const predicate = h.messageLogState.clearMessages.mock.calls[0][0] as (
108+
m: MessageEntry,
109+
) => boolean;
110+
expect(predicate(logNotification("n1"))).toBe(true);
111+
expect(predicate(request("r1", "tools/call"))).toBe(false);
112+
});
113+
114+
it("keeps pinned entries when clearing the protocol panel", () => {
115+
const h = harness({ pinnedProtocolIds: new Set(["keep"]) });
116+
h.run((api) => api.onClearProtocol());
117+
const predicate = h.messageLogState.clearMessages.mock.calls[0][0] as (
118+
m: MessageEntry,
119+
) => boolean;
120+
expect(predicate(request("keep", "ping"))).toBe(false);
121+
expect(predicate(request("other", "ping"))).toBe(true);
122+
});
123+
124+
it("clears one protocol section by pin membership", () => {
125+
const h = harness({ pinnedProtocolIds: new Set(["keep"]) });
126+
h.run((api) => api.onClearProtocolSection("history"));
127+
let predicate = h.messageLogState.clearMessages.mock.calls[0][0] as (
128+
m: MessageEntry,
129+
) => boolean;
130+
expect(predicate(request("keep", "ping"))).toBe(false);
131+
expect(h.setPinnedProtocolIds).not.toHaveBeenCalled();
132+
133+
h.run((api) => api.onClearProtocolSection("pinned"));
134+
predicate = h.messageLogState.clearMessages.mock.calls[1][0] as (
135+
m: MessageEntry,
136+
) => boolean;
137+
expect(predicate(request("keep", "ping"))).toBe(true);
138+
// Clearing the pinned section drops the now-stale id set.
139+
expect(h.setPinnedProtocolIds).toHaveBeenCalledWith(new Set());
140+
});
141+
142+
it("clears the network and console stores", () => {
143+
const h = harness();
144+
h.run((api) => {
145+
api.onClearNetwork();
146+
api.onClearConsole();
147+
});
148+
expect(h.fetchRequestLogState.clearFetchRequests).toHaveBeenCalled();
149+
expect(h.stderrLogState.clearStderrLogs).toHaveBeenCalled();
150+
});
151+
152+
it("no-ops when the stores are absent", () => {
153+
const h = harness({
154+
messageLogState: null,
155+
fetchRequestLogState: null,
156+
stderrLogState: null,
157+
});
158+
h.run((api) => {
159+
api.onClearLogs();
160+
api.onClearProtocol();
161+
api.onClearNetwork();
162+
api.onClearConsole();
163+
api.onClearProtocolSection("pinned");
164+
});
165+
expect(h.messageLogState.clearMessages).not.toHaveBeenCalled();
166+
});
167+
});
168+
169+
describe("export", () => {
170+
it("stamps the server id and the kind into the filename", () => {
171+
const h = harness({ logs: [{ level: "info" }] as never });
172+
h.run((api) => api.onExportLogs());
173+
expect(h.filename()).toMatch(/^inspector-logs-srv-.*\.json$/);
174+
});
175+
176+
it("writes each view's entries", () => {
177+
const cases: [keyof ExportActions, Partial<UseExportActionsParams>][] = [
178+
["onExportProtocol", { messages: [request("r1", "ping")] }],
179+
["onExportNetwork", { fetchRequests: [{ id: "f1" }] as never }],
180+
["onExportLogs", { logs: [{ id: "l1" }] as never }],
181+
["onExportConsole", { stderrLogs: [{ id: "s1" }] as never }],
182+
];
183+
for (const [action, params] of cases) {
184+
downloadJsonFile.mockClear();
185+
const h = harness(params);
186+
h.run((api) => (api[action] as () => void)());
187+
expect(h.exported()).toHaveLength(1);
188+
}
189+
});
190+
191+
it("writes nothing when the view is empty", () => {
192+
const h = harness();
193+
h.run((api) => {
194+
api.onExportLogs();
195+
api.onExportProtocol();
196+
api.onExportNetwork();
197+
api.onExportConsole();
198+
api.onExportProtocolSection("pinned");
199+
api.onExportProtocolSection("history");
200+
});
201+
expect(downloadJsonFile).not.toHaveBeenCalled();
202+
});
203+
204+
it("exports one protocol section by pin membership", () => {
205+
const h = harness({
206+
messages: [request("keep", "ping"), request("other", "ping")],
207+
pinnedProtocolIds: new Set(["keep"]),
208+
});
209+
h.run((api) => api.onExportProtocolSection("pinned"));
210+
expect(h.filename()).toContain("protocol-pinned");
211+
expect(h.exported()).toEqual([expect.objectContaining({ id: "keep" })]);
212+
213+
downloadJsonFile.mockClear();
214+
h.run((api) => api.onExportProtocolSection("history"));
215+
expect(h.filename()).toContain("protocol-unpinned");
216+
expect(h.exported()).toEqual([expect.objectContaining({ id: "other" })]);
217+
});
218+
});
219+
220+
describe("replay", () => {
221+
it("re-issues the entry's own method and params", async () => {
222+
replayProtocolRequest.mockResolvedValue(null);
223+
const h = harness({ messages: [request("r1", "tools/list")] });
224+
h.run((api) => api.onReplayProtocol("r1"));
225+
await vi.waitFor(() => {
226+
expect(replayProtocolRequest).toHaveBeenCalledWith(
227+
expect.anything(),
228+
"tools/list",
229+
{ a: 1 },
230+
[],
231+
);
232+
});
233+
expect(notificationsMock.show).not.toHaveBeenCalled();
234+
});
235+
236+
it("surfaces a pre-flight refusal as a toast", async () => {
237+
replayProtocolRequest.mockResolvedValue("Nope.");
238+
const h = harness({ messages: [request("r1", "tools/list")] });
239+
h.run((api) => api.onReplayProtocol("r1"));
240+
await vi.waitFor(() => {
241+
expect(notificationsMock.show).toHaveBeenCalledWith(
242+
expect.objectContaining({ title: "Can't replay", message: "Nope." }),
243+
);
244+
});
245+
});
246+
247+
it("surfaces a thrown failure as a toast", async () => {
248+
replayProtocolRequest.mockRejectedValue(new Error("boom"));
249+
const h = harness({ messages: [request("r1", "tools/list")] });
250+
h.run((api) => api.onReplayProtocol("r1"));
251+
await vi.waitFor(() => {
252+
expect(notificationsMock.show).toHaveBeenCalledWith(
253+
expect.objectContaining({ title: "Replay failed", message: "boom" }),
254+
);
255+
});
256+
});
257+
258+
it("stringifies a non-Error rejection", async () => {
259+
replayProtocolRequest.mockRejectedValue("plain");
260+
const h = harness({ messages: [request("r1", "tools/list")] });
261+
h.run((api) => api.onReplayProtocol("r1"));
262+
await vi.waitFor(() => {
263+
expect(notificationsMock.show).toHaveBeenCalledWith(
264+
expect.objectContaining({ message: "plain" }),
265+
);
266+
});
267+
});
268+
269+
it("does nothing when disconnected, when the id is unknown, or when the entry carries no method", () => {
270+
const noClient = harness({
271+
inspectorClient: null,
272+
messages: [request("r1", "tools/list")],
273+
});
274+
noClient.run((api) => api.onReplayProtocol("r1"));
275+
276+
const unknown = harness({ messages: [request("r1", "tools/list")] });
277+
unknown.run((api) => api.onReplayProtocol("nope"));
278+
279+
const result = harness({
280+
messages: [
281+
{
282+
id: "r1",
283+
direction: "response",
284+
timestamp: 0,
285+
message: { jsonrpc: "2.0", id: 1, result: {} },
286+
} as unknown as MessageEntry,
287+
],
288+
});
289+
result.run((api) => api.onReplayProtocol("r1"));
290+
291+
expect(replayProtocolRequest).not.toHaveBeenCalled();
292+
});
293+
294+
it("passes no params when the entry declares none", async () => {
295+
replayProtocolRequest.mockResolvedValue(null);
296+
const h = harness({
297+
messages: [
298+
{
299+
id: "r1",
300+
direction: "request",
301+
timestamp: 0,
302+
message: { jsonrpc: "2.0", id: 1, method: "ping" },
303+
} as unknown as MessageEntry,
304+
],
305+
});
306+
h.run((api) => api.onReplayProtocol("r1"));
307+
await vi.waitFor(() => {
308+
expect(replayProtocolRequest).toHaveBeenCalledWith(
309+
expect.anything(),
310+
"ping",
311+
undefined,
312+
[],
313+
);
314+
});
315+
});
316+
});
317+
});

0 commit comments

Comments
 (0)