feat(web): History — Pin & Replay, plus two history-status fixes (#1438)

[cliffhall]

History-screen work. Three related changes on one branch.

1. Pin & Replay (#1438)

Both HistoryEntry actions were rendered but wired to todoNoop in App.

• Pin — App owns a session-scoped Set<string> of pinned entry ids, toggled by onTogglePinHistory and passed down as pinnedHistoryIds. HistoryListPanel already sorts pinned entries to the top. The set resets with the rest of the per-screen state on disconnect / server-switch.
• Replay — onReplayHistory re-issues the entry's original request by method: tools/call → callTool, prompts/get → getPrompt, resources/read → readResource. The call flows through InspectorClient → tracked transport → message log, so the fresh request+response surface as a new History entry (history-local) without touching the Tools/Prompts/Resources panels. An unsupported method or a removed tool surfaces a toast.

2. Notifications no longer stuck "Pending" (#1439)

extractStatus derived the badge purely from whether a response was attached, so a fire-and-forget notification (no id, no response) — and any unmatched standalone response — showed Pending forever. The pending→OK/Error lifecycle only applies to direction === "request" entries; non-request entries now render no request-style badge (the method badge already labels them).

3. Server→client requests resolve in history (#1440)

MessageTrackingTransport.send only tracked outgoing requests, so the client's response to a server→client request (roots/list, sampling, elicitation) was never recorded — the request sat Pending with no response body. send is now symmetric with onmessage: it tracks outgoing responses (by id + result/error) too, so the response folds into its request entry and resolves to OK/Error. (Verified the integration "no orphan responses" assertion still holds.)

Testing

• npm run validate — 2070 unit/integration tests pass; per-file coverage gate green.
• npm run test:storybook — 333 pass.

Closes #1438, closes #1439, closes #1440.

🤖 Generated with Claude Code

4. Direction badge on History (#1437)

A dual-state badge at each History entry's top-left shows travel direction: client → server (green, inspector-originated) / client ← server (violet, server-originated), plus a matching "Filter by Message Direction" control. It derives from a new MessageEntry.origin field set at tracking time (MessageTrackingTransport tags outgoing send as client, incoming onmessage as server). (Network traffic is always inspector-originated — even SSE is a client-initiated stream — so there's no direction badge/filter there.)

Closes #1437.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 17s —— View job

---

Code Review — PR #1441 (History: Pin & Replay + status fixes)

I reviewed the full diff against origin/v2/main. Overall this is clean, well-factored, and well-tested — single source of truth for replayable methods, a shared MessageDirectionBadge/ExpandToggle/PinToggle extracted from the duplicated subtle-buttons, and the core transport fix is symmetric and correct. Below are findings, none blocking.

✅ What's solid

• messageTrackingTransport.send (History: server→client requests (roots/list, sampling, elicitation) stuck Pending with no response body #1440) — the new outgoing-response branch correctly uses message.id !== null && message.id !== undefined rather than a falsy check, so a JSON-RPC id of 0 is still tracked. The classification (result/error → response, else method → request) mirrors onmessage exactly, and the new test file covers request/response/error/notification cases plus base-transport forwarding.
• extractStatus → "none" (History: server notifications show a permanent PENDING status badge #1439) — gating the pending→OK/Error lifecycle on direction === "request" is the right fix; notifications and unmatched standalone responses no longer render a permanent "Pending". The method badge already labels them.
• REPLAYABLE_HISTORY_METHODS single source of truth — HistoryEntry (button visibility) and App.replayHistoryRequest (dispatch gate) both read it, so they can't drift. Nice.
• clearMessages predicate semantics — verified against messageLogState.ts:130 (predicate = remove). onClearHistorySection passes has(id) for pinned / !has(id) for history, which is correct, and the pinned set is reset when the pinned section is cleared.
• EMPTY_HISTORY_UI was updated with visibleDirections: { client: true, server: true } and is reset on disconnect — no stale-state leak.

🔸 Observations / minor

1. Outgoing client notifications remain untracked — the one residual asymmetry. send only tracks messages with an id, so the client's own notifications (notifications/initialized, notifications/progress, notifications/roots/list_changed) never reach history, while incoming server notifications are tracked via onmessage. Your test documents this as intentional, but it slightly undercuts the "send is now symmetric with onmessage" framing in the PR body. Worth confirming this is deliberate — if outgoing notifications should appear in History, send needs an else if ("method" in message) → trackNotification(…, "client") branch.

2. Per-section Clear/Export ignore the active direction & search filters. onClearHistorySection/onExportHistorySection operate on the full unfiltered log by pin membership. So clearing the "pinned" section while a direction filter hides some pinned entries still removes all pinned entries — including the ones not currently visible. Defensible ("clear the section = clear the section"), but it can surprise a user who's filtered the view. Consider either respecting the visible set or leaving as-is intentionally.

3. PR description vs. code: incoming badge color. Section 4 of the description says incoming is yellow; the code uses violet (and MessageDirectionBadge's comment explains why not yellow — green+yellow reads as ok/caution status, not direction). Code is internally consistent (HistoryControls also uses violet); just the prose is stale.

4. replayHistoryRequest default: branch is dead code. It's gated by isReplayableHistoryMethod and the switch enumerates every member of the set, so default is unreachable. Harmless defensive belt-and-suspenders, but you could drop it (or keep for safety — your call).

5. Replaying a tools/call re-runs side effects with no confirmation. Inherent to the feature and fine, but a destructive tool will execute again silently on Replay. Not a change request — just flagging the behavior is by design.

Nits

• MessageDirectionBadge color names (green/violet) and HistoryControls' Text c={color} use Mantine palette names — consistent with the AGENTS.md token rules (named colors, no hex). 👍
• New element components aren't enumerated in any README, and the READMEs don't list individual elements, so no doc update is owed.

No correctness blockers found. Items 1 and 2 are the only ones I'd ask you to consciously confirm-or-tweak before merge.

[cliffhall]

Thanks for the thorough review! Addressed in 96471f8d.

1. Outgoing client notifications untracked — fixed. Good call; I made send truly symmetric with onmessage. It now also tracks the client's own notifications (initialized, progress, roots/list_changed) as origin client via the else if ("method" in message) branch, so they surface in History (showing client → server) just like incoming server notifications. Updated the transport test (the old "does not track an outgoing notification" case now asserts it is tracked as a client notification). Integration suite still green.

2. Per-section Clear/Export ignore active filters — intentional, left as-is. They operate on the full section by pin membership, matching the existing panel-level Clear/Export (which also ignore the search/method/direction filters). Keeping all three consistent felt less surprising than having section actions respect filters while the panel ones don't — "clear the section = clear the section." Happy to revisit if we later make the panel-level ones filter-aware too.

3. PR description (yellow vs violet) — fixed. Updated section 4 of the description to say violet; the code/comment were already correct.

4. replayHistoryRequest default branch — keeping it. It's unreachable at runtime (gated by isReplayableHistoryMethod + the switch enumerates every set member), but TS can't prove the gate guarantees membership, so without default the function fails "not all code paths return." So it stays as the required return — belt-and-suspenders that also satisfies the compiler.

5. Replay re-runs side effects — by design, acknowledged. Replaying a tools/call re-executes it (including destructive tools) with no confirmation, same as invoking it from the Tools screen. Noting it's intentional; a confirm-on-destructive affordance would be a separate enhancement.

Nits — agreed, no changes owed.

Re-validated: npm run validate (2094) + npm run test:storybook (339) green.

@claude review

[claude]

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

---

Follow-up review — 96471f8d (outgoing client notifications)

• Review outgoing client notification tracking change
• Confirm transport send/onmessage symmetry
• Check test coverage for new branch
• Post review

Thanks for the thorough write-up — I re-read the diff against origin/v2/main and the follow-up commit. All five items are handled correctly; nothing left to change.

✅ Item 1 — outgoing client notifications (the one substantive change)

MessageTrackingTransport.send is now genuinely symmetric with onmessage:

if ("id" in message && message.id !== null && message.id !== undefined) {
  if ("result" in message || "error" in message) trackResponse(…, "client");
  else if ("method" in message)                  trackRequest(…, "client");
} else if ("method" in message) {
  trackNotification(message as JSONRPCNotification, "client");   // ← new
}

This now mirrors the onmessage branch structure exactly (messageTrackingTransport.ts:56-70 vs :115-136), the only difference being the "client" vs "server" origin tag — which is precisely the intended asymmetry. I traced it through the rest of the pipeline and it holds end-to-end:

• No permanent "Pending". A client notification lands as direction: "notification", and extractStatus returns "none" for anything that isn't a request (HistoryEntry.tsx:83) — so no request-style badge. ✔
• Correct direction badge. origin: "client" → MessageDirectionBadge direction="outgoing" → green client → server (HistoryEntry.tsx:127-129). ✔
• Respects the direction filter. matchesFilters keys visibility off entry.origin (HistoryListPanel.tsx:156), so the new client notifications honor the client/server toggle. ✔
• Test updated, not just flipped. The old "does not track" case is now a positive assertion that trackNotification is called with (notification, "client") while trackRequest/trackResponse are not — a precise replacement, not a loosened one (messageTrackingTransport.test.ts:66-79). ✔

One thing worth a passing note (no action needed): send will now also track notifications/initialized and any notifications/cancelled the client emits, not just the three you listed — which is the correct, fully-symmetric behavior, just slightly broader than the commit message's examples imply.

✅ Items 2–5

• 2 (per-section Clear/Export ignore filters) — agreed, consistency with the panel-level actions is the right call. "Clear the section = clear the section" is defensible and now matches the existing behavior rather than diverging from it.
• 3 (yellow→violet prose) — description updated; code/comment were already correct.
• 4 (default: branch) — correct, it's the required return that satisfies TS's "not all code paths return," not redundant defensiveness. Keep it.
• 5 (replay side effects) — by design, acknowledged; confirm-on-destructive is a fair follow-up.

Verdict

No correctness issues. The transport fix is the load-bearing change and it's clean, symmetric, and covered. 👍 LGTM.
• feat/history-pin-replay-1438

[cliffhall]

Thanks — appreciate the careful re-trace. Confirming your passing note: yes, send now tracks all client-originated notifications (initialized, cancelled, progress, roots/list_changed, …), not just the three I listed — that's the intended fully-symmetric behavior; the commit examples were just illustrative.

No further changes. Ready to merge whenever. 🙏

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 38s —— View job

---

Follow-up review — commits since 96471f8d

• Diff new commits (MessageDirectionFilter extraction, section-header pleat, single-section accordion, ExpandToggle icons, Network direction revert)
• Read changed files + trace the panel/section Clear semantics
• Post review

Re-reviewed the five commits layered on since the last pass. The refactors are clean — MessageDirectionFilter is a faithful extraction (with a nice aria-pressed added that the inline version lacked), the sectionHeader Group variant correctly reuses the existing list-item hover class, and the single-section !collapsible branch is well-reasoned (no stale collapsed state can hide a lone section). One thing I'd ask you to confirm before merge.

🔸 Panel-level Clear scope vs. its disabled gating (worth a conscious decision)

HistoryListPanel.tsx:248-254 gates the panel Clear button on the unpinned count:

<Button variant="default" onClick={onClearAll}
  disabled={unpinnedEntries.length === 0}>
  Clear
</Button>

…which frames it as "clear the History (unpinned) section." But the handler it fires, onClearHistory (App.tsx:1694-1696), clears everything with no predicate — pinned entries included — and (unlike onClearHistorySection("pinned") at App.tsx:1726) does not reset pinnedHistoryIds:

const onClearHistory = useCallback(() => {
  messageLogState?.clearMessages();          // wipes pinned too
}, [messageLogState]);                        // …and leaves pinnedHistoryIds stale

So the gating and the action disagree on whether pins are in scope. Two concrete consequences:

1. Both sections present → clicking the panel Clear silently wipes the Pinned section too, even though the user has a separate per-section Pinned Clear and the disabled rule implies this button only touches history. Surprising scope.
2. All entries pinned (unpinnedEntries.length === 0) → panel Clear is disabled, and the per-section actions are hidden (single section, bothSections === false), so there's no affordance to clear an all-pinned history except unpinning entries one at a time.

Plus the residual stale-set point: after a panel Clear, pinnedHistoryIds still holds the deleted ids (harmless today since nothing matches, but inconsistent with the section-clear path that resets it).

Pick one of two consistent intents:

• Clear = nuclear (also clears pins): gate on disabled={!hasResults} instead of the unpinned count, and reset the pin set in onClearHistory (setPinnedHistoryIds(new Set())). Fixes case 2 and the stale set.
• Clear = history-only (keep pins): give onClearHistory the (m) => !pinnedHistoryIds.has(m.id) predicate so it matches the gating and the section model.

Fix this →

Nits

• Stale docstring. MessageDirectionFilter.tsx:35-37 says it's "Shared by the History and Network controls so the two stay identical," but the Network direction filter/badge was reverted (0424f48d, 626aa726 → revert). It's History-only now — drop the Network mention.

Everything else (icon swap, section pleat, extraction, tests) looks good. Item above is the only one I'd ask you to consciously confirm-or-tweak.
• feat/history-pin-replay-1438

[cliffhall]

Thanks — both addressed in c1ca5576.

Panel Clear scope — went history-only (keep pins). You're right the gating and handler disagreed. I made onClearHistory clear only the unpinned set: messageLogState.clearMessages((m) => !pinnedHistoryIds.has(m.id)). Rationale: pinning's whole purpose is to protect an entry, so Clear shouldn't nuke pins. This now matches the disabled={unpinnedEntries.length === 0} gating and the per-section model, and resolves the stale-set point — the pins survive, so pinnedHistoryIds stays valid (no reset needed).

On your case 2 (all entries pinned → Clear disabled, no per-section action): that's now intentional rather than a bug — pins are protected, so "clear an all-pinned history" means unpin first. The per-section Pinned Clear still covers the both-sections case. If we later want a true "nuke everything" affordance I'd add it as a distinct control rather than overload the history Clear.

Added an App test asserting the panel Clear predicate keeps pinned ids (false = don't remove) and removes unpinned (true).

Nit — fixed. Dropped the "and Network" from the MessageDirectionFilter docstring; it's History-only now.

Re-validated: npm run validate (2101) + npm run test:storybook (341) green.

@claude review

[claude]

Claude finished @cliffhall's task in 42s —— View job

---

Follow-up review — c1ca5576 (panel Clear keeps pinned entries)

• Diff the latest commit
• Verify the panel Clear handler now matches its gating
• Check the new test + docstring nit
• Post review

Re-read the commit against origin/v2/main. The previous review's one item is now resolved correctly, and the nit is dropped. No issues — LGTM.

✅ Panel Clear reconciliation (history-only)

You took the history-only branch, and it's internally consistent end-to-end:

• onClearHistory now passes (m) => !pinnedHistoryIds.has(m.id) (App.tsx:1699). I checked the predicate semantics against the store — clearMessages does filter((m) => !predicate(m)), i.e. predicate true = remove (messageLogState.ts:130-133). So !has(id) keeps pinned (false → survives), removes unpinned (true → dropped). Matches the button's disabled={unpinnedEntries.length === 0} gating and the per-section model. ✔
• Stale-set point resolved without a reset. Because the pinned entries survive the clear, every id in pinnedHistoryIds still points at a live message — no setPinnedHistoryIds(new Set()) needed, and the pendingRequestEntries bookkeeping in clearMessages (the predicate branch) correctly only evicts removed entries. ✔
• The new test asserts the predicate behavior, not just that clearMessages was called — predicate({id:"hist-1"}) (pinned) → false, predicate({id:"other"}) → true (App.test.tsx:1091-1093). That's the right level to lock this in, and the comment correctly explains the "should remove?" inversion. ✔

✅ Case 2 (all-pinned → Clear disabled) — agreed it's now by-design

With history-only semantics, "Clear an all-pinned history" → unpin first is the consistent rule, and the per-section Pinned Clear still covers the both-sections case. Deferring a distinct "nuke everything" control rather than overloading Clear is the right call.

✅ Docstring nit fixed

MessageDirectionFilter's docstring no longer claims History-and-Network sharing; it's accurate now that the Network direction filter was reverted (0424f48d).

Verdict

The gating and the action now agree, the pin set stays valid, and the test pins the behavior precisely. Clean. 👍
• feat/history-pin-replay-1438

[cliffhall]

Thanks for the close re-trace — glad the history-only Clear and the predicate-level test land cleanly. No further changes; ready to merge. 🙏