fix(web): re-enable other server cards when a connection errors (#1521)

[cliffhall]

Summary

Fixes #1521. When a server's connection settled into the terminal Error state, the other server cards stayed dimmed/inert — their Connect toggles and action buttons disabled — so the user couldn't interact with any other server.

Root cause

ServerCard dims any card whose id differs from activeServer:

// ServerCard.tsx
const isDimmed = activeServer !== undefined && activeServer !== id;

activeServer (= activeServerId in App.tsx) is cleared only on the InspectorClient disconnect event (App.tsx, the effect at ~line 889). But a terminal error state does not always fire disconnect:

• a connect-time handshake failure sets status = "error" and re-throws, dispatching only statusChange (InspectorClient connect() catch), and
• a mid-session transport onerror that fires without an accompanying onclose dispatches error + statusChange only.

In those cases activeServerId lingers, so every other card stays inert. We can't simply clear activeServerId on error — the errored card's status, the connected header, and the connection-info modal are all keyed off it.

Fix

Dim the other cards only while the active session is genuinely live (connecting or connected). InspectorView now passes the active id to ServerListScreen as the dim source only when connectionStatus is connecting/connected; once the session reaches a terminal state (error or disconnected) the other cards re-enable. The errored card itself still reflects its real status via the merged servers list, which keys off the real activeServer — so the "Error" indicator is unaffected.

One-line behavioural change; everything else (the merged status list, the connected header, the modal) keeps using the real activeServer.

Testing

• New regression test (InspectorView.test.tsx): with connectionStatus === "error" and activeServer still set, the non-active card is not aria-disabled; with connectionStatus === "connected" it is. Verified the test fails without the fix and passes with it.
• npm run validate (web): format, lint, build, 2000 unit tests pass; 370 Storybook play-function tests pass.

Manual verification

Drove the app against a 3-server catalog. Confirmed the intended dimming during a live session (one connecting → the other two correctly greyed/disabled) and that cards re-enable once the session is no longer live. Note: the three error triggers I could reproduce with the test servers (stdio connect-timeout, SDK request-timeout, HTTP server kill) all fire onclose → disconnect and self-recover via the existing reset; the persistent-error-without-disconnect state from the issue is exercised by the unit regression test above.

🤖 Generated with Claude Code

[cliffhall]

Related to #1520 (fixes #1516). Both PRs touch clients/web/src/components/views/InspectorView/InspectorView.tsx and its test file, but in non-overlapping regions:

• This PR (fix(web): re-enable other server cards when a connection errors (#1521) #1522) adds a liveness-gated dimCardsAgainst value after the servers merge and changes the ServerListScreen activeServer prop, plus card-dimming tests near the listChanged block.
• feat(web): gate header tabs on advertised server capabilities (#1516) #1520 edits the availableTabs memo (and its tests) to gate header tabs on advertised server capabilities.

They should merge cleanly in either order; whichever lands second is worth a quick rebase check.

[cliffhall]

@claude review

[claude]

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

---

Review — re-enable other server cards on error (#1521)

• Read PR diff against origin/v2/main
• Review InspectorView.tsx change in context
• Verify ServerListScreen/ServerCard dim semantics
• Check test coverage & edge cases
• Post review

---

Verdict: correct, well-targeted, and well-documented fix. The change is sound and I'd be comfortable merging it. One concrete reuse suggestion and a couple of optional notes below.

Why the fix is correct

I traced the dim path end-to-end. ServerCard uses its activeServer prop for exactly one thing — the isDimmed flag (ServerCard.tsx:143), which drives variant="disabled", aria-disabled/inert (:168–170), and the toggle's disabled (:185). Everything the active card actually displays — status indicator, protocol version, toggle state — flows from connection, which InspectorView builds from the real activeServer in the servers merge (InspectorView.tsx:554–577). So routing only the dim-source through dimCardsAgainst cleanly lifts the other cards' inert state without touching the errored card's status.

Also worth noting: the active card is never self-dimmed regardless (isDimmed requires activeServer !== id), so passing undefined on a terminal state can only ever re-enable the other cards — there's no behavioral risk to the errored card. ✅

Suggestion: reuse the existing isTerminalStatus predicate

core/mcp/types.ts:171 already exports exactly the inverse of sessionLive:

export function isTerminalStatus(status: ConnectionStatus | undefined): boolean {
  return status === "disconnected" || status === "error";
}

Its doc comment is the canonical convention for "session is over" branching (added in #1490) and explicitly warns against hand-rolling status comparisons. sessionLive = connectionStatus === "connecting" || connectionStatus === "connected" is precisely !isTerminalStatus(connectionStatus). Reusing it keeps this gate aligned with the teardown convention and means a future fifth status can't silently desync the two checks:

import { isTerminalStatus } from "@inspector/core/mcp/types.js"; // currently type-only import at line 14
// ...
const dimCardsAgainst = isTerminalStatus(connectionStatus) ? undefined : activeServer;

(InspectorView.tsx:14 currently imports from that module as import type — you'd add a value import.) Fix this →

Optional notes (non-blocking)

• Prop naming / semantic overload. The ServerListScreen/ServerCard activeServer prop now means "the id to dim against", and you deliberately pass undefined even when a server is active (just errored). The thorough comment makes this safe to follow, but the cleanest long-term shape is to rename the prop to something like dimAgainst so the call site reads honestly. Out of scope for a one-line bugfix — fine to leave.
• Test robustness. The new tests assert on .closest(".mantine-Card-root") + aria-disabled, which couples them to Mantine's internal class name. That matches the existing convention in this file, so it's consistent — just flagging that these tests would break on a Mantine card-internals change. The rerender-based transition test is a nice touch and directly reproduces the issue.
• Coverage of the "live" branch. Good that you added the positive case (connected → beta dimmed) alongside the regression (error → beta enabled), so both sides of the gate are exercised.

Nice, surgical fix with a comment that actually explains why activeServer lingers on error — that context is the part a future reader would otherwise miss.
· v2/1521-reenable-cards-after-error

[cliffhall]

Thanks for the thorough review! Applied the actionable suggestion in b4482fe:

✅ Reuse isTerminalStatus — replaced the hand-rolled sessionLive check with the existing predicate from @inspector/core/mcp/types.js:

const dimCardsAgainst = isTerminalStatus(connectionStatus) ? undefined : activeServer;

Added a value import for it (the existing import from that module was import type), and the inline comment now points at the #1490 convention. Format/lint/build clean; all 36 InspectorView tests pass.

On the optional notes (leaving as-is, with reasoning):

• Prop rename activeServer → dimAgainst — agreed it'd read more honestly, but it's a cross-component rename (ServerListScreen + ServerCard + their tests/stories) beyond this one-line bugfix; out of scope here.
• .mantine-Card-root coupling in tests — kept for consistency with the existing assertions in this file (e.g. the protocol-version test uses the same approach); a Mantine internals change would be a broad, visible break rather than a silent one.

[cliffhall]

@claude review

[claude]

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

---

Re-review — isTerminalStatus refactor applied (#1521)

• Read PR diff against origin/v2/main
• Review InspectorView.tsx change in context
• Verify dim semantics + isTerminalStatus reuse
• Check test coverage
• Post review

---

Verdict: ✅ approve. The suggestion from the prior round was applied cleanly and the result is exactly right. Nothing blocking; I'd merge this.

What changed since last review

dimCardsAgainst now derives from the shared predicate instead of a hand-rolled liveness check:

const dimCardsAgainst = isTerminalStatus(connectionStatus) ? undefined : activeServer;

with a value import added at InspectorView.tsx:21 (the existing import type block at :14 is untouched). This is the correct inversion: isTerminalStatus returns true for "disconnected" | "error" (core/mcp/types.ts:171), so dimCardsAgainst is undefined precisely on terminal states and activeServer only while connecting/connected — identical behavior to the old sessionLive form, now sourced from the #1490 teardown convention so a future fifth ConnectionStatus can't silently desync the gate.

Verification

• Dim path is still single-purpose. ServerCard consumes activeServer only for isDimmed (activeServer !== undefined && activeServer !== id); the active card's status/protocol/toggle all flow from connection, which InspectorView builds from the real activeServer in the servers merge. So routing only the dim-source through dimCardsAgainst re-enables the other cards without touching the errored card's "Error" indicator. The active card can never self-dim (isDimmed requires activeServer !== id), so passing undefined is purely additive-safe.
• Tests cover both branches of the gate. connected → beta card aria-disabled="true" (positive), and the rerender-based connected → error transition → beta aria-disabled is null (regression for the exact issue). Good that the terminal-state path asserts the transition rather than just a static error render.

Notes (non-blocking, unchanged from last round)

• The ServerListScreen/ServerCard prop is still literally named activeServer while now carrying "id to dim against" semantics. The thorough inline comment makes this safe; a dimAgainst rename remains a reasonable future cleanup but is out of scope for this one-liner — agree with leaving it.
• Tests couple to .mantine-Card-root, consistent with the existing assertions in this file. Fine.

Clean, surgical, well-commented. 👍
· v2/1521-reenable-cards-after-error