web: deep-link auto-connect (?serverUrl&transport&autoConnect) + connection-status (#1576)

[cliffhall]

Closes #1576

Summary

URL-driven auto-connect with a machine-readable status surface, so a driver reaches a connected inspector with one navigate:

http://127.0.0.1:6274/?serverUrl=<url>&transport=http|sse&autoConnect=<token>

• clients/web/src/utils/deepLink.ts — parseDeepLink() / deepLinkParseStatus() / deepLinkConfigEquals() / DEEP_LINK_SERVER_ID. serverUrl restricted to http(s) (rejects javascript:/data:/file:); autoConnect must equal the per-launch MCP_INSPECTOR_API_TOKEN (CSRF gate — same exposure as the existing ?MCP_INSPECTOR_API_TOKEN= param); stable deep-link server id so reloads reconnect to the same row.
• App.tsx — two-phase ensure/connect effects (upsert the full server config against the hydrated list, then connect via a fresh closure); serversRef read in onToggleConnection; connectErrorMessage records connection-level failures.
• InspectorView header — data-testid="connection-status" with data-status, data-error-message, and data-deeplink="parsed|rejected|none" so drivers can waitForSelector and read failure reasons without scraping toasts.
• Docs — deep-link scheme + data-* contract in clients/web/README.md.

Notes

Re-implemented informed by PR #1510 (33fac3f), adapted to the current codebase:

• The tab state is now lifted to App.tsx (Persist per-screen selection and search/filter across tab navigation; clear only on user action or disconnect #1417) and serversRef / onResourceError already exist post-Wave-2, so the wiring differs from the reference.
• The reference imported a normalizeServerUrl helper that does not exist here; validateServerUrl uses URL.href instead, which canonicalizes host case + trailing slash the same way.

The openApp/appArgs/autoOpen consumption is the follow-up #1577 (the parser already parses those params; this PR wires only auto-connect + status).

Coverage gate green (npm run test:coverage, EXIT 0; deepLink.ts 97.8/95.1/100/97.4). App.tsx is outside the coverage include globs.

Part of the #1579 decomposition (Wave 4). Per AGENTS.md the Closes line won't auto-fire on v2/main; the issue is closed and its board card moved on the Wave 4 rollup merge.

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

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

---

Code Review — deep-link auto-connect + connection-status

• Read changed files (deepLink.ts, App.tsx, InspectorView.tsx, tests, README)
• Analyze security (URL validation, CSRF gate)
• Analyze correctness (effects, reconnect, status surface)
• Post review

Reviewed against origin/v2/main. Overall this is a clean, well-documented change — the security model is sound, the two-phase effect is thoughtfully commented, and the data-* contract is a nice machine-readable surface. A few observations below, mostly minor.

---

👍 Strengths

• Security gating is correct. parseDeepLink requires autoConnect === authToken before doing anything, rejects non-http(s) schemes, and the tests cover javascript:/file:/data:/malformed. autoOpen reuses the same token gate. The threat model (a third-party page can't forge the per-launch token, same exposure as the existing ?MCP_INSPECTOR_API_TOKEN=) is spelled out and holds up.
• deepLinkConfigEquals comparing type as well as url is the right fix — a bare url compare would silently connect with a stale transport after an sse↔http param change.
• Stable DEEP_LINK_SERVER_ID avoids duplicate catalog rows across reloads. Good call.
• Test coverage on deepLink.ts is thorough (base64url appArgs, transport fallback, URL canonicalization, CSRF rejections).

---

🔍 Observations

1. addServer failures are swallowed silently — deep link can get permanently stuck (minor, but undercuts the machine-readable contract). App.tsx:2538

void addServer(deepLink.serverId, deepLink.serverConfig).catch(() => {
  // 409 = already on disk; hydration will surface it on a later render.
});

The comment assumes the only failure is a 409, but this .catch swallows every error. If the catalog is read-only (serverListWritable === false) or the backend returns a real 5xx, addServer throws, is swallowed, deepLinkEnsureRef stays true, and the row never appears — so the effect returns early forever at the if (!existing) guard. The connect never fires and nothing is written to data-error-message, so a driver polling the status surface just times out with data-deeplink="parsed" and no reason. Consider recording the error via recordConnectError for the non-409 case so the failure is observable through the same contract the rest of this PR builds. Fix this →

2. Possible stale-config connect in the updateServer branch (plausible, narrow). App.tsx:2547-2556

if (!deepLinkConfigEquals(existing.config, deepLink.serverConfig)) {
  await updateServer(deepLink.serverId, deepLink.serverId, deepLink.serverConfig);
}
if (activeServerId !== deepLink.serverId) {
  await onToggleConnection(deepLink.serverId);
}

onToggleConnection reads the target from serversRef.current and calls setupClientForServer(target). But serversRef.current is updated in a useEffect (App.tsx:1350), not synchronously by updateServer — updateServer awaits a refresh() that calls setServers, and the ref only catches up after React commits and runs the passive effect. In the promise continuation right after await updateServer(...), that commit hasn't happened yet, so serversRef.current still holds the pre-update config. Net effect: in the exact case deepLinkConfigEquals was added to handle (reload with a changed transport/serverUrl on the same deep-link id), the persisted config is correct but the live connection may use the stale transport. It self-corrects on a manual reconnect. This is narrow (only triggers on a param change across reloads), but it means the updateServer branch doesn't fully deliver its intended fix. Connecting against deepLink.serverConfig directly (rather than routing through the ref) would close the gap.

3. deepLinkParseStatus reports rejected for a lone autoConnect param. deepLink.ts:121

params.has("serverUrl") || params.has("autoConnect") means a URL carrying only ?autoConnect=… (no serverUrl) is classified rejected. That's defensible (it's a malformed deep link), but since autoConnect isn't a param a normal user would ever paste alone, it's unlikely to matter — noting for completeness.

4. No test exercises the App.tsx two-phase effect. The PR notes App.tsx is outside the coverage include globs, and deepLink.ts is well-covered. That's consistent with the repo's setup, but the ensure→connect handoff, the OAUTH_CALLBACK_PATH precedence guard, and the two refs are the trickiest logic here and go untested. Not a blocker given the constraint, just flagging the gap.

---

Nits

• deepLink.ts:82 validateServerUrl returns url.href, which preserves any embedded credentials/query/fragment (http://user:pass@host). Not a security issue (parity with the manual connect form), just worth being aware the whole href flows through.
• The InspectorView.test.tsx additions are good — they verify both the present and absent (data-error-message omitted) cases. 👍

None of these are blocking. #1 is the one I'd most encourage addressing, since it's a silent-failure hole in the exact automation contract this PR is establishing.
· branch v2/1576-deeplink-connect

[cliffhall]

Thanks for the review — addressed the two substantive findings in 0beb6e65:

• Progress notifications #1 (silent addServer failure): the ensure-phase catch now records non-409 failures (read-only catalog, backend 5xx) via recordConnectError so they surface on data-error-message instead of leaving the deep link stuck at the guard. A genuine 409 (already exists) is still swallowed since hydration surfaces the row and the connect phase proceeds.
• Request logging #2 (stale-config connect): split the update and connect into separate render phases (new deepLinkUpdateRef) — the effect now connects only once servers reflects the updated config, so onToggleConnection (reading serversRef, synced by an earlier passive effect) builds the client from the fresh transport.

#3 (lone autoConnect → rejected) and #4 (App.tsx effect untested — App.tsx is outside the coverage include globs) left as-is, as noted. The credentials-in-href nit is intentional parity with the manual connect form.

@claude review

[claude]

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

---

Re-review — fixes in 0beb6e65

• Read the fix commit
• Verify finding Progress notifications #1 fix (observable addServer failures)
• Verify finding Request logging #2 fix (stale-config connect / deepLinkUpdateRef)
• Post review

Both substantive findings are resolved correctly. Verified against the surrounding code, not just the diff.

---

✅ Finding #1 — silent addServer failure now observable

The ensure-phase catch now records non-409 failures on data-error-message:

if (!message.includes("already exists")) recordConnectError(message);

I traced the 409 path to confirm the string match is sound: core/mcp/remote/node/server.ts:1880 returns { error: "Server '<id>' already exists" } with a 409, readErrorMessage (core/react/useServers.ts:78) surfaces that error string verbatim, and addServer (useServers.ts:195) throws new Error(message). So a real 409 reliably contains "already exists" and is swallowed, while a read-only-catalog / 5xx failure is recorded. The silent-hole in the automation contract is closed. 👍

Minor (non-blocking): matching on the message text couples the client to the backend's exact 409 phrasing — if that string ever changes, a genuine 409 would start getting reported as an error. addServer only throws a message (the HTTP status isn't preserved on the Error), so a status-code check isn't available without a small plumbing change. Fine to leave; worth a comment-level awareness.

✅ Finding #2 — stale-config connect fixed via phase split

The update and connect are now separate render phases (deepLinkUpdateRef), and the effect returns after updateServer so it re-runs once servers reflects the new config before connecting. I confirmed the ordering the fix relies on actually holds:

• serversRef is synced in the passive effect at App.tsx:1350 ([activeServer, activeServerId, servers]).
• That effect is declared before the deep-link effect at App.tsx:2536, and both depend on servers, so React runs the ref-sync first within the same commit.
• Therefore, when the connect phase reaches onToggleConnection (which reads serversRef.current), the ref already holds the post-updateServer config — the client is built from the fresh transport, not the stale one.

This is the exact sse↔http reload case deepLinkConfigEquals was added for, and it now delivers. Good.

One narrow edge worth a mental note (not a fix request): if updateServer succeeds but the hydrated existing.config never compares equal to deepLink.serverConfig (e.g. some future backend-side normalization of url/type), the effect would sit in the update branch with deepLinkUpdateRef already true — no re-update, no connect, no error recorded. Today deepLinkConfigEquals compares only url+type and the backend persists them verbatim, so convergence holds; it's only a risk if persistence ever normalizes those fields.

Remaining items — dispositions reasonable

• Completion requests #3 (lone ?autoConnect → rejected) and SSE transport support #4 (App.tsx effect untested; App.tsx is outside the coverage include globs) left as-is — both were noted as non-blocking and the rationale stands.
• Credentials-in-href nit — intentional parity with the manual connect form. Agreed.

---

Nice, tight fixes — the phase-split comment block (App.tsx:2521–2535) documents the ordering dependency well, which is exactly the kind of thing that gets broken by a later refactor if it's undocumented. LGTM.
· branch v2/1576-deeplink-connect

[cliffhall]

Superseded by the Wave 4 rollup #1676, which contains this change (reviewed to LGTM here) merged base→tip. Closing in favor of the rollup.