fix(web): address #1244 PR review feedback (JsonValue cast, client lifecycle, dead code)

- **#1 — Misleading `Record<string, never>` cast**: Imported `JsonValue`
from `@inspector/core/mcp/index.js` and changed `args as Record<string,
never>` to `args as Record<string, JsonValue>` in `onCallTool`. The new
cast honestly narrows from the screen-level `Record<string, unknown>`
to what `InspectorClient.callTool` actually requires. Added a comment
explaining the boundary.
- **#2 + #3 — Previous InspectorClient not disconnected on server switch
or unmount**: Added a `useEffect` keyed on `inspectorClient` whose
cleanup calls `inspectorClient.disconnect()`. One effect covers both
the swap-on-switch leak (prior session's transport stayed open until
GC) and the unmount-during-connected leak (HMR / tests).
`setupClientForServer`'s existing state-manager `destroy()` calls
handle the listener side; the new effect handles the transport side.
- **#4 — Dead `void` block at bottom of file**: Deleted the trailing
`void FetchRequestLogState; void StderrLogState;` block. The state
managers are already constructed via `new FetchRequestLogState(client)`
/ `new StderrLogState(client)`, so the imports are referenced by the
linter's reckoning. The `void` pair was dead code with a comment that
no longer matched reality.
- **#5 / #7 / #8 — Follow-up TODOs**: Added TODO refs to the new
follow-up issues:
- TODO(#1323): `error` event in InspectorClientEventMap so mid-session
transport failures surface (connect-only catch is incomplete).
- TODO(#1324): negotiated `protocolVersion` through `useInspectorClient`
(currently hard-coded `"2025-06-18"`).
- TODO(#1325): `useResourceSubscriptions` hook so subscribe/unsubscribe
buttons reflect their server-side effect.
Items #6 (useCallback dep churn), #9 (helper unit tests), and #10
(shared no-op bridge factory fixture) are acknowledged in the PR reply
as deferred — no functional impact and out of scope for a "wire the
hook layer" PR.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: cliffhall

clients/web/src/App.tsx

@@ -8,6 +8,7 @@ import type {
 }from"@modelcontextprotocol/sdk/types.js";
 importtype{AppBridge}from"@modelcontextprotocol/ext-apps/app-bridge";
 import{InspectorClient}from"@inspector/core/mcp/index.js";
+importtype{JsonValue}from"@inspector/core/mcp/index.js";
 importtype{MessageEntry,ServerEntry}from"@inspector/core/mcp/types.js";
 import{API_SERVER_ENV_VARS}from"@inspector/core/mcp/remote/constants.js";
 import{ManagedToolsState}from"@inspector/core/mcp/state/managedToolsState.js";
@@ -253,10 +254,26 @@ function App() {
 }
 },[connectionStatus]);
 
+// Disconnect the previous InspectorClient when it's replaced (server
+// switch) or when App unmounts (HMR, tests). Without this the prior
+// session's transport — a spawned stdio subprocess, an SSE stream, or
+// an HTTP session — stays open until GC eventually lets go. The
+// state-manager destroys in `setupClientForServer` only handle the
+// listener side; this effect handles the transport side. `disconnect()`
+// is the canonical lifecycle hook (InspectorClient has no `destroy()`);
+// it closes the transport, clears subscriptions, cancels receiver TTLs.
+useEffect(()=>{
+return()=>{
+if(inspectorClient){
+voidinspectorClient.disconnect();
+}
+};
+},[inspectorClient]);
+
 // Build the InitializeResult the connected ViewHeader expects from the
 // hook's split fields. `protocolVersion` is hard-coded for now — the
-// useInspectorClient hook doesn't expose it; revisit if/when InspectorView
-// grows a need for the real negotiated value.
+// useInspectorClient hook doesn't expose it. TODO(#1324): consume the
+// negotiated value once the hook surfaces it.
 constinitializeResult=useMemo<InitializeResult|undefined>(()=>{
 if(connectionStatus!=="connected"||!serverInfo)returnundefined;
 return{
@@ -358,6 +375,10 @@ function App() {
 try{
 awaitclient.connect();
 }catch(err){
+// Handshake-only. A mid-session transport failure transitions the
+// client status to "error" without rejecting any pending promise,
+// and `errorMessage` stays stale. TODO(#1323): consume an `error`
+// event from `InspectorClientEventMap` once it exists.
 connectStartRef.current=undefined;
 constmessage=errinstanceofError ? err.message : String(err);
 setErrorMessage(message);
@@ -386,9 +407,14 @@ function App() {
 if(!tool)return;
 setToolCallState({status: "pending"});
 try{
+// ToolsScreen types the args as `Record<string, unknown>` (it accepts
+// anything the user types into the schema form). `callTool` requires
+// `Record<string, JsonValue>` — narrow at the boundary instead of
+// claiming the object is empty (which the previous `as Record<string,
+// never>` cast did, misleadingly).
 constinvocation=awaitinspectorClient.callTool(
 tool,
-argsasRecord<string,never>,
+argsasRecord<string,JsonValue>,
 );
 setToolCallState({
 status: invocation.success ? "ok" : "error",
@@ -531,6 +557,9 @@ function App() {
 prompts={prompts}
 resources={resources}
 resourceTemplates={resourceTemplates}
+// TODO(#1325): drop the empty fallback once `useResourceSubscriptions`
+// surfaces the live subscription list — subscribe/unsubscribe buttons
+// currently fire but the screen never reflects the result.
 subscriptions={[]}
 logs={logs}
 tasks={tasks}
@@ -591,10 +620,3 @@ function App() {
 }
 
 exportdefaultApp;
-
-// Surface so the FetchRequestLog + Stderr state managers are kept alive
-// (their effects subscribe to client events for #1262 / future panels).
-// Marking them void here keeps the no-unused-vars lint quiet without
-// hiding the intent.
-voidFetchRequestLogState;
-voidStderrLogState;