From 85153c6232d4d14cc41899ce3db3602994447ba7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 9 Jul 2026 13:12:32 -0400 Subject: [PATCH] feat(web): open the monitoring column on a failed connection (#1621) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On a connection failure the user was left without an obvious way to see why. Open the monitoring column automatically on the transition into the `error` status (mirroring the connect-success behavior from #1616) so the diagnostics surface right when they're needed. - Broaden the auto-open effect to fire on the transition into `error` as well as `connected`, tracked via a general previous-status ref so it fires once per transition and never on a mount that starts errored. - A failed connect never negotiated capabilities, so the failure column offers History (the local client-side message log) and — for non-stdio transports — Network (HTTP requests), the entries that explain the failure. Logs is omitted (it needs the server `logging` capability). - `effectivePinned` now allows the column while `error`, not only while `connected`; it still closes on a clean disconnect or narrowing. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw --- .../InspectorView/InspectorView.test.tsx | 80 +++++++++++++++++++ .../views/InspectorView/InspectorView.tsx | 68 ++++++++++------ 2 files changed, 125 insertions(+), 23 deletions(-) diff --git a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx index 75e5ba7c..9879a5c5 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx @@ -1376,6 +1376,86 @@ describe("InspectorView", () => { ).toBeNull(); }); + // A failed connect leaves the errored server's id set but no + // initializeResult (capabilities were never negotiated). + function failedHttp(overrides: Partial = {}) { + return makeProps({ + servers: [httpServer], + activeServer: "beta", + connectionStatus: "error", + initializeResult: undefined, + ...overrides, + }); + } + + it("opens the monitoring column on a failed connection attempt (#1621)", async () => { + monitorWide.value = true; + const { rerender } = renderWithMantine( + , + ); + // Disconnected: the column is closed. + expect( + screen.queryByRole("button", { name: "Close monitoring column" }), + ).toBeNull(); + + // A connection failure (→ error) opens it to surface the diagnostics. + rerender(); + expect( + await screen.findByRole("button", { name: "Close monitoring column" }), + ).toBeInTheDocument(); + + // The failure column offers the History + Network diagnostics, but not + // Logs (a failed connect never negotiated the logging capability). + expect(screen.getByRole("radio", { name: "History" })).toBeChecked(); + expect( + screen.getByRole("radio", { name: "Network" }), + ).toBeInTheDocument(); + expect(screen.queryByRole("radio", { name: "Logs" })).toBeNull(); + }); + + it("omits Network from the failure column for a stdio server (#1621)", async () => { + monitorWide.value = true; + const stdioErr: ServerEntry = { + id: "beta", + name: "Beta", + config: { type: "stdio", command: "missing-bin" }, + connection: { status: "disconnected" }, + }; + const { rerender } = renderWithMantine( + , + ); + rerender( + , + ); + // stdio has no HTTP traffic, so the failure column shows History only. + expect( + await screen.findByRole("radio", { name: "History" }), + ).toBeInTheDocument(); + expect(screen.queryByRole("radio", { name: "Network" })).toBeNull(); + }); + + it("does not auto-open on a mount that starts already errored (#1621)", () => { + // No transition into error, and no stored preference, so the column stays + // closed — a user who closed it isn't fought on remount. + monitorWide.value = true; + renderWithMantine(); + expect( + screen.queryByRole("button", { name: "Close monitoring column" }), + ).toBeNull(); + }); + it("pins the monitor group into the column and removes it from the header", async () => { monitorWide.value = true; renderWithMantine(); diff --git a/clients/web/src/components/views/InspectorView/InspectorView.tsx b/clients/web/src/components/views/InspectorView/InspectorView.tsx index b61f1ed3..74dddfad 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.tsx @@ -654,21 +654,26 @@ export function InspectorView({ getInitialValueInEffect: false, }); - // Open the monitoring column when a connection is established (#1616). Gated on - // the disconnected → connected *transition* (via the ref) rather than the - // "connected" state itself, so it fires on an actual connect — not on every - // render while connected, and not on a mount that starts already-connected - // (which would fight a user who closed it). The column still only *appears* - // when wide + a monitor tab is available (`effectivePinned`); this just sets - // the preference. Closing it stays closed until the next connect, since this + // Open the monitoring column when a connection is established (#1616) OR when a + // connection attempt fails (#1621). Gated on the *transition into* the target + // status (via the ref) rather than the status itself, so it fires once on an + // actual connect/failure — not on every render, and not on a mount that starts + // already in that status (which would fight a user who closed it). On success + // the column surfaces the live stream; on failure it surfaces the History / + // Network entries that explain what went wrong. The column still only *appears* + // when wide + a monitor tab is available (`effectivePinned`); this just sets the + // preference. Closing it stays closed until the next connect/failure, since this // effect only re-runs when `connectionStatus` changes. - const wasConnectedRef = useRef(connectionStatus === "connected"); + const prevStatusRef = useRef(connectionStatus); useEffect(() => { - const isConnected = connectionStatus === "connected"; - if (isConnected && !wasConnectedRef.current) { + const prev = prevStatusRef.current; + prevStatusRef.current = connectionStatus; + const becameConnected = + connectionStatus === "connected" && prev !== "connected"; + const becameError = connectionStatus === "error" && prev !== "error"; + if (becameConnected || becameError) { setMonitorPinned(true); } - wasConnectedRef.current = isConnected; }, [connectionStatus, setMonitorPinned]); const appTools = useMemo(() => { @@ -740,20 +745,37 @@ export function InspectorView({ appTools, ]); - // Monitoring column, derived (#1616). The monitor group is pinned into the - // right column only when: the user asked for it, the viewport is wide enough, - // a server is connected, and at least one monitor tab is actually available - // (capability/stdio aware). Narrowing, disconnecting, or losing the last - // monitor capability flips `effectivePinned` false and closes the column — - // WITHOUT clearing `monitorPinned`, so it re-opens when the condition returns. - // Only the column's close button writes `monitorPinned = false`. + // Monitoring column, derived (#1616, #1621). The monitor group is pinned into + // the right column only when: the user asked for it, the viewport is wide + // enough, the session is connected OR a connect attempt failed, and at least + // one monitor tab is actually available (capability/stdio aware). Narrowing, + // returning to a clean disconnect, or losing the last monitor capability flips + // `effectivePinned` false and closes the column — WITHOUT clearing + // `monitorPinned`, so it re-opens when the condition returns. Only the column's + // close button writes `monitorPinned = false`. const connected = connectionStatus === "connected"; - const monitorAvailable = useMemo( - () => availableTabs.filter((t) => MONITOR_TABS.includes(t)), - [availableTabs], - ); + // A failed connection attempt (#1621) keeps the column available so the user + // can see why, even though no live session exists. + const failed = connectionStatus === "error"; + const monitorAvailable = useMemo(() => { + if (connected) return availableTabs.filter((t) => MONITOR_TABS.includes(t)); + if (failed) { + // A failed connect never negotiated capabilities, so Logs (gated on the + // server's `logging` capability) isn't meaningful. History (the local + // client-side message log) always captured the attempt, and Network (HTTP + // requests) did too for non-stdio transports — those are what explain the + // failure, so offer exactly them. + const active = serversInput.find((s) => s.id === activeServer); + const isStdio = active ? getServerType(active.config) === "stdio" : false; + return isStdio ? [HISTORY_TAB] : [HISTORY_TAB, NETWORK_TAB]; + } + return []; + }, [connected, failed, availableTabs, serversInput, activeServer]); const effectivePinned = - monitorPinned && !!isWide && connected && monitorAvailable.length > 0; + monitorPinned && + !!isWide && + (connected || failed) && + monitorAvailable.length > 0; // The header loses the monitor group while the column is open (its screens // live in the column instead); otherwise it shows every available tab.