chore(web): address Copilot review on #2128

Seven of the eight comments, plus the three suppressed ones (the same
double-cast class):

- Narrow the toast hooks to `InspectorClientEventSource`, the client's
  typed event-listener surface. A bare `EventTarget` cannot satisfy it
  (its listener takes an `Event`, the typed one a `TypedEventGeneric`
  with `detail`), so both tests now construct a real
  `InspectorClientEventTarget` and dispatch through `dispatchTypedEvent`
  rather than casting a stand-in. That failed to compile immediately and
  caught two bad fixtures: `task()` was a `Pick<Task, "status">` where
  the event map wants a full `Task`, and `toolCallTaskUpdated` was
  dispatched with no `task` at all.
- Narrow `useExportActions`'s three store parameters to the single
  clearing method each handler calls, so the test doubles are
  structurally checked instead of cast away.
- Build real `MessageEntry` fixtures (`timestamp` is a `Date`, not `0`),
  removing four more double casts.
- Fix a tautological assertion in useTabUiState.test: it compared a
  callback to itself, so it held however the identity changed. Every
  callback is now captured before the update and compared to those
  references.
- Reword `resetTabUiState`'s doc: `exhaustive-deps` cannot infer
  stability across a hook boundary, so the dependency IS named — the
  stability buys no churn, not omission.
- Justify the one remaining double cast in place, per the type-safety
  rule.

`useProgressToasts` still returns `void` rather than an object,
deliberately: it has nothing to return, and `{}` would imply an API that
is not there. Noted on the PR as the one deviation from #2128's
"returns one object" criterion.

Signed-off-by: cliffhall <cliff@futurescale.com>

clients/web/src/hooks/useExportActions.test.tsx

@@ -32,21 +32,21 @@ const { replayProtocolRequest } = vi.hoisted(() => ({
 }));
 vi.mock("../lib/protocolReplay", () => ({ replayProtocolRequest }));
 
-const request = (id: string, method: string): MessageEntry =>
-  ({
-    id,
-    direction: "request",
-    timestamp: 0,
-    message: { jsonrpc: "2.0", id: 1, method, params: { a: 1 } },
-  }) as unknown as MessageEntry;
+const AT = new Date(0);
 
-const logNotification = (id: string): MessageEntry =>
-  ({
-    id,
-    direction: "notification",
-    timestamp: 0,
-    message: { jsonrpc: "2.0", method: "notifications/message" },
-  }) as unknown as MessageEntry;
+const request = (id: string, method: string): MessageEntry => ({
+  id,
+  direction: "request",
+  timestamp: AT,
+  message: { jsonrpc: "2.0", id: 1, method, params: { a: 1 } },
+});
+
+const logNotification = (id: string): MessageEntry => ({
+  id,
+  direction: "notification",
+  timestamp: AT,
+  message: { jsonrpc: "2.0", method: "notifications/message" },
+});
 
 function harness(overrides: Partial<UseExportActionsParams> = {}) {
   const messageLogState = { clearMessages: vi.fn() };
@@ -55,12 +55,11 @@ function harness(overrides: Partial<UseExportActionsParams> = {}) {
   const setPinnedProtocolIds = vi.fn();
   const params: UseExportActionsParams = {
     activeServerId: "srv",
-    messageLogState:
-      messageLogState as unknown as UseExportActionsParams["messageLogState"],
-    fetchRequestLogState:
-      fetchRequestLogState as unknown as UseExportActionsParams["fetchRequestLogState"],
-    stderrLogState:
-      stderrLogState as unknown as UseExportActionsParams["stderrLogState"],
+    // Structurally checked against the hook's narrowed store parameters — each
+    // names only the clearing method these handlers call.
+    messageLogState,
+    fetchRequestLogState,
+    stderrLogState,
     messages: [],
     fetchRequests: [],
     logs: [],
@@ -281,9 +280,9 @@ describe("useExportActions", () => {
           {
             id: "r1",
             direction: "response",
-            timestamp: 0,
+            timestamp: AT,
             message: { jsonrpc: "2.0", id: 1, result: {} },
-          } as unknown as MessageEntry,
+          },
         ],
       });
       result.run((api) => api.onReplayProtocol("r1"));
@@ -298,9 +297,9 @@ describe("useExportActions", () => {
           {
             id: "r1",
             direction: "request",
-            timestamp: 0,
+            timestamp: AT,
             message: { jsonrpc: "2.0", id: 1, method: "ping" },
-          } as unknown as MessageEntry,
+          },
         ],
       });
       h.run((api) => api.onReplayProtocol("r1"));

clients/web/src/hooks/useExportActions.ts

@@ -20,9 +20,14 @@ export type ProtocolSection = "pinned" | "history";
 export interface UseExportActionsParams {
   /** The active server's id, stamped into every export filename. */
   activeServerId?: string;
-  messageLogState: MessageLogState | null;
-  fetchRequestLogState: FetchRequestLogState | null;
-  stderrLogState: StderrLogState | null;
+  /**
+   * Only the clearing method of each store is named, rather than the whole
+   * class: that is all these handlers touch, and a narrower parameter lets a
+   * test supply a structurally-checked double instead of a cast-away stand-in.
+   */
+  messageLogState: Pick<MessageLogState, "clearMessages"> | null;
+  fetchRequestLogState: Pick<FetchRequestLogState, "clearFetchRequests"> | null;
+  stderrLogState: Pick<StderrLogState, "clearStderrLogs"> | null;
   /** The Protocol view's entries (the message log). */
   messages: MessageEntry[];
   /** The Network view's entries. */

clients/web/src/hooks/useInspectorStores.test.tsx

@@ -127,7 +127,16 @@ vi.mock("@inspector/core/react/useStderrLog.js", () => ({
   useStderrLog: () => ({ stderrLogs: [{ id: "s1" }] }),
 }));
 
-const client = () => ({ id: "client" }) as unknown as InspectorClient;
+/**
+ * A per-call identity token standing in for the client. Every store
+ * constructor is mocked away below, so nothing here ever reads a property off
+ * it — the assertions only check *which* value each constructor received, and
+ * that a second `createStores` passes a different one. The double cast is what
+ * that isolation buys: building a real `InspectorClient` would drag in a
+ * transport and an environment to assert on object identity.
+ */
+const client = () =>
+  ({ marker: Symbol("client") }) as unknown as InspectorClient;
 
 const fetchLogOptions = {
   maxFetchRequests: 42,

clients/web/src/hooks/useProgressToasts.test.tsx

@@ -1,6 +1,6 @@
 import { describe, it, expect, beforeEach, vi } from "vitest";
 import { act } from "react";
-import type { InspectorClient } from "@inspector/core/mcp/index.js";
+import { InspectorClientEventTarget } from "@inspector/core/mcp/inspectorClientEventTarget.js";
 import { renderWithMantine } from "../test/renderWithMantine";
 import { progressToastId } from "../utils/toasts/progressToasts";
 import { useProgressToasts } from "./useProgressToasts";
@@ -20,16 +20,16 @@ vi.mock("@mantine/notifications", () => ({
 }));
 
 /**
- * The hook only ever calls `add/removeEventListener`, so a bare `EventTarget`
- * is a faithful stand-in for the client — the events are real DOM events and
- * the listener wiring under test is the real wiring.
+ * The client's own typed event target, which is exactly the surface the hook
+ * declares — so this is the real wiring under test, not a stand-in: the events
+ * dispatched below are the same typed events the client emits.
  */
-function fakeClient(): InspectorClient & EventTarget {
-  return new EventTarget() as unknown as InspectorClient & EventTarget;
+function fakeClient(): InspectorClientEventTarget {
+  return new InspectorClientEventTarget();
 }
 
-function harness(client: InspectorClient & EventTarget) {
-  function Probe({ c }: { c: InspectorClient | null }) {
+function harness(client: InspectorClientEventTarget) {
+  function Probe({ c }: { c: InspectorClientEventTarget | null }) {
     useProgressToasts(c);
     return null;
   }
@@ -37,13 +37,13 @@ function harness(client: InspectorClient & EventTarget) {
   return {
     progress: (progressToken: string, progress: number, total?: number) =>
       act(() => {
-        client.dispatchEvent(
-          new CustomEvent("progressNotification", {
-            detail: { progressToken, progress, total },
-          }),
-        );
+        client.dispatchTypedEvent("progressNotification", {
+          progressToken,
+          progress,
+          total,
+        });
       }),
-    swapClient: (next: InspectorClient | null) =>
+    swapClient: (next: InspectorClientEventTarget | null) =>
       act(() => rerender(<Probe c={next} />)),
     unmount: () => act(() => unmount()),
   };
@@ -125,11 +125,10 @@ describe("useProgressToasts", () => {
     h.swapClient(fakeClient());
     notificationsMock.show.mockClear();
     act(() => {
-      first.dispatchEvent(
-        new CustomEvent("progressNotification", {
-          detail: { progressToken: "tok-1", progress: 1 },
-        }),
-      );
+      first.dispatchTypedEvent("progressNotification", {
+        progressToken: "tok-1",
+        progress: 1,
+      });
     });
     expect(notificationsMock.show).not.toHaveBeenCalled();
   });

clients/web/src/hooks/useProgressToasts.ts

@@ -1,9 +1,7 @@
 import { useEffect, useRef } from "react";
 import { notifications } from "@mantine/notifications";
-import type {
-  InspectorClient,
-  InspectorClientEventMap,
-} from "@inspector/core/mcp/index.js";
+import type { InspectorClientEventMap } from "@inspector/core/mcp/index.js";
+import type { InspectorClientEventTarget } from "@inspector/core/mcp/inspectorClientEventTarget.js";
 import type { TypedEventGeneric } from "@inspector/core/mcp/typedEventTarget.js";
 import {
   formatProgressToastMessage,
@@ -11,6 +9,20 @@ import {
   progressToastId,
 } from "../utils/toasts/progressToasts";
 
+/**
+ * The only part of the client the toast hooks touch: its typed event-listener
+ * surface. Naming that rather than the whole `InspectorClient` states the real
+ * dependency, and lets a test drive them with a plain
+ * `InspectorClientEventTarget` instead of a stand-in cast to a full client.
+ *
+ * Declared here (rather than in a module of its own) because it has no runtime
+ * half, and `useTaskToasts` — its only other consumer — is a sibling.
+ */
+export type InspectorClientEventSource = Pick<
+  InspectorClientEventTarget,
+  "addEventListener" | "removeEventListener"
+>;
+
 /**
  * Surface incoming `notifications/progress` as toasts so the user can watch a
  * long-running tool's progress while staying on the tool view — the v2
@@ -21,7 +33,9 @@ import {
  * (see `progressToastId`) and replaced per tick, so a chatty server updates one
  * toast rather than stacking one per tick.
  */
-export function useProgressToasts(inspectorClient: InspectorClient | null) {
+export function useProgressToasts(
+  inspectorClient: InspectorClientEventSource | null,
+) {
   // Which progress streams currently have a live toast. Entries are removed
   // when their toast closes (auto-dismiss or user). A ref (not state) because
   // it is incidental bookkeeping that must not trigger re-renders.

clients/web/src/hooks/useTabUiState.test.tsx

@@ -87,14 +87,22 @@ describe("useTabUiState", () => {
     expect(h.api().activeTab).toBe("Tools");
   });
 
-  it("keeps resetTabUiState stable so a []-dep session reset can call it", () => {
+  it("keeps its callbacks stable, so naming them in a dependency array costs no churn", () => {
     const h = harness();
-    const before = h.api().resetTabUiState;
+    // Captured BEFORE the update: comparing the post-render values to each
+    // other would hold however the identities changed.
+    const resetBefore = h.api().resetTabUiState;
+    const toggleBefore = h.api().togglePinProtocol;
+    const setPinsBefore = h.api().setPinnedProtocolIds;
+    const setTabBefore = h.api().setActiveTab;
+
     h.run((api) => api.setActiveTab("Logs"));
-    expect(h.api().resetTabUiState).toBe(before);
-    expect(h.api().togglePinProtocol).toBe(
-      // togglePin is likewise stable — it is passed straight into the JSX.
-      h.api().togglePinProtocol,
-    );
+    h.run((api) => api.togglePinProtocol("m1"));
+
+    expect(h.renders()).toBeGreaterThan(1);
+    expect(h.api().resetTabUiState).toBe(resetBefore);
+    expect(h.api().togglePinProtocol).toBe(toggleBefore);
+    expect(h.api().setPinnedProtocolIds).toBe(setPinsBefore);
+    expect(h.api().setActiveTab).toBe(setTabBefore);
   });
 });

clients/web/src/hooks/useTabUiState.ts

@@ -44,9 +44,12 @@ export interface TabUiStateResult {
   togglePinProtocol: (id: string) => void;
   /**
    * Reset every screen's UI object and the pin set to their empty defaults.
-   * Stable, so the session-reset callback that drives it needs no dependency
-   * on this hook. Deliberately leaves `activeTab` alone: the tab the user is
-   * on is shell state, reset separately on an *explicit* disconnect.
+   * Its identity never changes, so naming it in the dependency array of the
+   * session-reset callback that drives it — which `exhaustive-deps` requires,
+   * since the rule cannot infer stability across a custom-hook boundary —
+   * costs that callback no churn. Deliberately leaves `activeTab` alone: the
+   * tab the user is on is shell state, reset separately on an *explicit*
+   * disconnect.
    */
   resetTabUiState: () => void;
 }

clients/web/src/hooks/useTaskToasts.test.tsx

@@ -1,7 +1,7 @@
 import { describe, it, expect, beforeEach, vi } from "vitest";
 import { act } from "react";
 import type { Task } from "@modelcontextprotocol/client";
-import type { InspectorClient } from "@inspector/core/mcp/index.js";
+import { InspectorClientEventTarget } from "@inspector/core/mcp/inspectorClientEventTarget.js";
 import { renderWithMantine } from "../test/renderWithMantine";
 import { taskToastId } from "../utils/toasts/taskToasts";
 import { useTaskToasts, type TaskToasts } from "./useTaskToasts";
@@ -18,46 +18,72 @@ vi.mock("@mantine/notifications", () => ({
   notifications: notificationsMock,
 }));
 
-/** The hook only calls `add/removeEventListener`, so a bare EventTarget does. */
-function fakeClient(): InspectorClient & EventTarget {
-  return new EventTarget() as unknown as InspectorClient & EventTarget;
+/**
+ * The client's own typed event target — exactly the surface the hook declares,
+ * so the events dispatched below are the same typed events the client emits.
+ */
+function fakeClient(): InspectorClientEventTarget {
+  return new InspectorClientEventTarget();
 }
 
-const task = (status: Task["status"]): Pick<Task, "status"> => ({ status });
+/** A complete `Task`; only `status` varies across these tests. */
+const task = (taskId: string, status: Task["status"]): Task => ({
+  taskId,
+  status,
+  ttl: null,
+  createdAt: "2026-01-01T00:00:00.000Z",
+  lastUpdatedAt: "2026-01-01T00:00:00.000Z",
+});
 
-function harness(client: InspectorClient & EventTarget) {
+function harness(client: InspectorClientEventTarget) {
   let latest: TaskToasts | undefined;
-  function Probe({ c }: { c: InspectorClient | null }) {
+  function Probe({ c }: { c: InspectorClientEventTarget | null }) {
     latest = useTaskToasts(c);
     return null;
   }
   const { rerender, unmount } = renderWithMantine(<Probe c={client} />);
-  const fire = (type: string, detail: unknown) =>
-    act(() => {
-      client.dispatchEvent(new CustomEvent(type, { detail }));
-    });
+  const dispatch = (fn: () => void) => act(fn);
   return {
     api: () => {
       if (!latest) throw new Error("hook did not render");
       return latest;
     },
     taskProgress: (taskId: string, progress: number, total?: number) =>
-      fire("requestorTaskProgress", {
-        taskId,
-        progress: { progress, total },
-      }),
+      dispatch(() =>
+        client.dispatchTypedEvent("requestorTaskProgress", {
+          taskId,
+          progress: { progress, total },
+        }),
+      ),
     statusChange: (taskId: string, status: Task["status"]) =>
-      fire("taskStatusChange", { taskId, task: task(status) }),
+      dispatch(() =>
+        client.dispatchTypedEvent("taskStatusChange", {
+          taskId,
+          task: task(taskId, status),
+        }),
+      ),
     requestorUpdate: (taskId: string, status: Task["status"]) =>
-      fire("requestorTaskUpdated", { taskId, task: task(status) }),
-    cancelled: (taskId: string) => fire("taskCancelled", { taskId }),
-    toolCallTask: (taskId: string) => fire("toolCallTaskUpdated", { taskId }),
+      dispatch(() =>
+        client.dispatchTypedEvent("requestorTaskUpdated", {
+          taskId,
+          task: task(taskId, status),
+        }),
+      ),
+    cancelled: (taskId: string) =>
+      dispatch(() => client.dispatchTypedEvent("taskCancelled", { taskId })),
+    toolCallTask: (taskId: string) =>
+      dispatch(() =>
+        client.dispatchTypedEvent("toolCallTaskUpdated", {
+          taskId,
+          task: task(taskId, "working"),
+        }),
+      ),
     run: (fn: (api: TaskToasts) => void) =>
       act(() => {
         if (!latest) throw new Error("hook did not render");
         fn(latest);
       }),
-    swapClient: (next: InspectorClient | null) =>
+    swapClient: (next: InspectorClientEventTarget | null) =>
       act(() => rerender(<Probe c={next} />)),
     unmount: () => act(() => unmount()),
   };
@@ -213,9 +239,10 @@ describe("useTaskToasts", () => {
       const h = harness(first);
       h.swapClient(null);
       act(() => {
-        first.dispatchEvent(
-          new CustomEvent("toolCallTaskUpdated", { detail: { taskId: "t9" } }),
-        );
+        first.dispatchTypedEvent("toolCallTaskUpdated", {
+          taskId: "t9",
+          task: task("t9", "working"),
+        });
       });
       expect(h.api().activeToolCallTaskIdRef.current).toBeUndefined();
     });
@@ -235,11 +262,10 @@ describe("useTaskToasts", () => {
       h.swapClient(fakeClient());
       notificationsMock.show.mockClear();
       act(() => {
-        first.dispatchEvent(
-          new CustomEvent("taskStatusChange", {
-            detail: { taskId: "t1", task: task("working") },
-          }),
-        );
+        first.dispatchTypedEvent("taskStatusChange", {
+          taskId: "t1",
+          task: task("t1", "working"),
+        });
       });
       expect(notificationsMock.show).not.toHaveBeenCalled();
     });
@@ -256,11 +282,10 @@ describe("useTaskToasts", () => {
       const h = harness(first);
       h.swapClient(null);
       act(() => {
-        first.dispatchEvent(
-          new CustomEvent("requestorTaskProgress", {
-            detail: { taskId: "t1", progress: { progress: 1 } },
-          }),
-        );
+        first.dispatchTypedEvent("requestorTaskProgress", {
+          taskId: "t1",
+          progress: { progress: 1 },
+        });
       });
       expect(h.api().progressByTaskId).toEqual({});
     });

clients/web/src/hooks/useTaskToasts.ts

@@ -1,10 +1,8 @@
 import { useCallback, useEffect, useRef, useState } from "react";
 import { notifications } from "@mantine/notifications";
-import type {
-  InspectorClient,
-  InspectorClientEventMap,
-} from "@inspector/core/mcp/index.js";
+import type { InspectorClientEventMap } from "@inspector/core/mcp/index.js";
 import type { TypedEventGeneric } from "@inspector/core/mcp/typedEventTarget.js";
+import type { InspectorClientEventSource } from "./useProgressToasts";
 import type { TaskProgress } from "../components/groups/TaskCard/TaskCard";
 import {
   formatTaskToastMessage,
@@ -47,7 +45,7 @@ export interface TaskToasts {
  * status history still lives in the Protocol view.
  */
 export function useTaskToasts(
-  inspectorClient: InspectorClient | null,
+  inspectorClient: InspectorClientEventSource | null,
 ): TaskToasts {
   // One live task-status toast per taskId, so each `notifications/tasks/status`
   // tick replaces the existing toast rather than stacking a fresh one.