Phase 5 audit: annotate spec, close HistoryControls partial

[cliffhall]

Summary

End-of-effort audit of clients/web/src/components/ against specification/v2_ux_interfaces.md per the plan's "Validation strategy" step ("Read v2_ux_interfaces.md end-to-end and confirm every component's Internal refactors bullet is satisfied").

Result: 57 of 62 components satisfied, 5 partial, 0 unstarted.

The audit findings live in a new "Audit results" section near the top of the spec. Per-component status was not duplicated into each entry — the summary section + the existing per-entry definitions are enough signal.

While auditing, closed the HistoryControls partial in this same branch:

• Added RequestMethod union to core/mcp/types.ts, derived from the SDK's Client/ServerRequestSchema['method'] | Client/ServerNotificationSchema['method'] plus a synthetic "response" for result/error MessageEntry rows.
• Typed methodFilter and onMethodFilterChange in HistoryControls, HistoryListPanel, and historyUtils.extractMethod with RequestMethod.
• Lifted the hardcoded METHOD_OPTIONS const out of HistoryControls into a required availableMethods: RequestMethod[] prop. HistoryScreen now derives it from the live entries via useMemo + extractMethod.
• Updated HistoryControls.stories.tsx to pass availableMethods.

Remaining partials (5)

Listed in the spec's audit section. Two are blocked on the v2 core hook layer effort (InspectorUrlElicitRequest wrapper type was deferred from Phase 0.2):

• ElicitationUrlPanel — needs InspectorUrlElicitRequest wrapper
• InlineElicitationRequest — same blocker

Three are in-scope for follow-ups, each with a non-trivial design question:

• InlineSamplingRequest — needs draftResult: CreateMessageResult integration with SamplingRequestPanel's draft-editing UX
• ExperimentalFeaturesPanel — reconcile per-toggle metadata with freeform ClientCapabilities['experimental'] shape
• ResourceListItem — implement annotation rendering

Test plan

• npm run format clean
• npm run lint clean
• npm run build (includes tsc -b) clean
• Spot-check HistoryControls and HistoryScreen stories in Storybook — methodFilter dropdown should populate from availableMethods

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 33s —— View job

---

Code Review — Phase 5 audit / HistoryControls partial close

• Gather context and understand the request
• Read changed files
• Review core/mcp/types.ts — RequestMethod union type
• Review historyUtils.ts — extractMethod typing
• Review HistoryControls.tsx — availableMethods prop lift
• Review HistoryListPanel.tsx — prop typing update
• Review HistoryScreen.tsx — useMemo derivation
• Review HistoryControls.stories.tsx — story update
• Review specification/v2_ux_interfaces.md — audit annotations
• Post final review

---

Overall this is a clean, well-scoped refactor. The SDK-derived RequestMethod type is the right approach, the availableMethods lift-to-owner is correct, and the spec audit section is concise and actionable. A few things worth discussing:

---

core/mcp/types.ts — RequestMethod union

export type RequestMethod =
  | ClientRequest["method"]
  | ClientNotification["method"]
  | ServerRequest["method"]
  | ServerNotification["method"]
  | "response";

Good design — deriving from the SDK keeps it self-updating. One naming note: the type covers notifications and server-originated messages as well as requests. RequestMethod will be read by a future maintainer as "method of a request", not "method of any MCP traffic message". Consider MessageMethod or TrafficMethod — the doc-comment already says as much ("Method name for any MessageEntry traffic"). Not a blocker, just worth considering before the type spreads further.

---

historyUtils.ts — cast in extractMethod

return entry.message.method as RequestMethod;

JSONRPCRequest["method"] is typed as string in the base SDK JSONRPC types, so the cast is unavoidable here. It's a correct-by-invariant cast (traffic in this app always originates from the SDK), but it sidesteps the type system. A slightly tighter alternative would be a validation helper that checks the value is in the known set and falls back to "response", but that's likely overkill for an internal utility. Worth a brief comment documenting why the cast is safe:

// Safe: all traffic in this app is produced by the MCP SDK
return entry.message.method as RequestMethod;

---

HistoryControls.tsx — import path inconsistency

HistoryControls.tsx imports via the alias:

import type { RequestMethod } from "@inspector/core/mcp/types.js";

But HistoryListPanel.tsx and historyUtils.ts use a deep relative path:

import type { ... } from "../../../../../../core/mcp/types.js";

Both work, but mixing conventions in the same component folder is noisy. Worth normalising to the alias everywhere (or the reverse). Fix this →

---

HistoryControls.tsx — Select onChange cast

onChange={(value) =>
  onMethodFilterChange((value as RequestMethod | null) ?? undefined)
}

The cast is necessary because Mantine types Select.onChange as (value: string | null) => void. This is fine, but it would be slightly cleaner if the as cast were moved to where the value is consumed rather than inline:

onChange={(value) => onMethodFilterChange(value as RequestMethod | undefined ?? undefined)}

...actually the current form is clearer. Minor style point, no action needed.

---

HistoryScreen.tsx — stale filter edge case

const availableMethods = useMemo(
  () => Array.from(new Set(entries.map(extractMethod))).sort(),
  [entries],
);

When entries shrinks (e.g. "Clear" is pressed), a previously-selected methodFilter can become absent from the new availableMethods. The filter state is not reset, so the list will show zero results with no indication that the filter is the cause — the user has to manually clear it. A common fix:

// in the onClearAll handler or via a useEffect:
useEffect(() => {
  if (methodFilter && !availableMethods.includes(methodFilter)) {
    setMethodFilter(undefined);
  }
}, [availableMethods, methodFilter]);

This is a quality-of-life issue, not a regression (the old hardcoded list was always fully populated), so it's fine to defer to a follow-up. Worth a // TODO in the component or noting in the partials list. Fix this →

---

HistoryControls.stories.tsx — SAMPLE_METHODS type

const SAMPLE_METHODS = [
  "tools/call",
  ...
] as const;
...
availableMethods: [...SAMPLE_METHODS],

The spread loses the literal type narrowing — TypeScript infers string[] rather than RequestMethod[]. This is harmless since the story compiles fine (all literals are members of RequestMethod), but adding an explicit type annotation makes the story's intent clear and would catch any future drift:

const SAMPLE_METHODS: RequestMethod[] = [
  "tools/call",
  ...
];

---

Spec audit section

Well-written. The per-partial reasoning is clear and actionable, the two hook-layer blockers are correctly identified and forward-referenced to the upcoming core/ work. No issues.

---

Summary

The RequestMethod type and availableMethods prop lift are solid improvements. Three minor things worth a follow-up or in-place fix:

1. Import path inconsistency — normalise to @inspector/core/mcp/types.js alias across all files in this group.
2. Stale methodFilter when entries shrink — low-priority UX issue, worth a // TODO or follow-up task.
3. RequestMethod naming — consider MessageMethod before the type spreads; not a blocker.

[cliffhall]

Thanks for the review. Addressed in 6b3cecd.

1. RequestMethod naming — agreed; renamed to MessageMethod everywhere. The type covers notifications and synthesized response entries, not just requests, so the original name was misleading. Renamed in core/mcp/types.ts, all four consumers in clients/web/src/, and the spec entry.

2. Cast in extractMethod — added a one-line // Cast: ... comment explaining the invariant (SDK types message.method as string, but every MessageEntry in this app originates from MCP SDK schemas, so the narrowing is safe).

3. Import path inconsistency — fixed. HistoryListPanel.tsx, historyUtils.ts, and HistoryScreen.tsx all now use @inspector/core/mcp/types.js, matching HistoryControls.tsx.

4. Mantine Select inline cast — skipped (you self-resolved as "no action needed").

5. Stale methodFilter when entries shrink — agreed it's a real UX issue but deferring as a separate follow-up rather than landing it here. Reasoning: this PR is the Phase 5 audit closeout and the filter-staleness bug isn't a spec-conformance gap (the spec doesn't say anything about clear-all behavior). Also, the cleanest fix is debatable — a useEffect that resets state would trip react-hooks/set-state-in-effect, and a render-time derivation diverges the displayed-value-in-Select from the underlying state. Want to settle that design separately.

6. SAMPLE_METHODS type — fixed. Now const SAMPLE_METHODS: MessageMethod[] with the as const + spread dropped, so the literal-type narrowing isn't lost.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 24s —— View job

---

Code Review — Phase 5 audit / HistoryControls partial close (follow-up)

• Gather context
• Read changed files
• Review diff vs origin/v2/main
• Post final review

---

All five items from the previous review have been correctly addressed in 6b3cecd. The code is clean and ready to merge.

What was fixed — quick confirmation

Item	Status
RequestMethod → MessageMethod rename across all files	✅ Done — type, consumers, spec all updated
Import paths normalised to @inspector/core/mcp/types.js alias	✅ Done — HistoryListPanel.tsx, historyUtils.ts, HistoryScreen.tsx all use the alias now
Cast comment in extractMethod	✅ Present and accurate
SAMPLE_METHODS: MessageMethod[] explicit type	✅ Done — as const + spread dropped, literal narrowing preserved
Stale methodFilter on entries-clear	✅ Correctly deferred with solid reasoning (the useEffect path trips react-hooks/set-state-in-effect; the render-time divergence path is confusing UX)

One small nit (no action needed)

historyUtils.ts has no corresponding test file, and neither does the wider clients/web/src/ tree — so this isn't a regression introduced here. The 90% coverage rule in AGENTS.md is a project-level gap predating this PR. Worth tracking separately.

Spec audit section

Well-structured. The two hook-layer-blocked partials are clearly identified, the three in-scope follow-ups have enough detail for someone picking them up cold, and the forward-reference to the upcoming core/ hook layer effort is accurate.

---

LGTM. Nothing blocking.