From a759777ceb64725eb55356267960743b46a7eece Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 9 Jul 2026 16:22:09 -0400 Subject: [PATCH] refactor(web): content-gate History in the failure column (#1621) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review follow-up (#1622): - Observation A: a returning user whose last-pinned monitor tab was History would land on the (empty) History tab on a stdio failure instead of the Console diagnostic. Root cause: History is *always* empty on a connect failure (the message log clears on the error transition), yet it was pushed unconditionally, so it could precede/hide the real diagnostic. Fix: content-gate History like Console/Network — only offer it when it has entries. On a fresh failure the diagnostic (Console/Network) is the only tab, so it always leads, without touching tab-switching or adding state. This also resolves the earlier "opens onto an empty column" note: when nothing has been captured yet, monitorAvailable is empty and the column stays closed, opening reactively once stderr/fetch content lands. Tests: stdio/HTTP failure now assert the diagnostic leads and History is absent; added "leads with Console even when stored tab was History", "keeps the column closed until a diagnostic has content", and "offers History when it has captured content". Verified live (time server → Console-only column). Full `npm run ci` passes. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw --- .../InspectorView/InspectorView.stories.tsx | 8 +- .../InspectorView/InspectorView.test.tsx | 131 ++++++++++++++++-- .../views/InspectorView/InspectorView.tsx | 26 ++-- 3 files changed, 138 insertions(+), 27 deletions(-) diff --git a/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx b/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx index b66eb265..9a83b55a 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx @@ -513,11 +513,13 @@ export const Connected: Story = { export const ConnectionError: Story = { // demoServers[0] is a stdio server; a failed launch has an empty History (the // message log clears on the error transition) and no HTTP traffic, so the - // auto-opened column keys off the captured stderr and defaults to Console — - // the process's startup error (#1621). `network: []` keeps it a pure stdio - // failure so Network isn't offered. + // auto-opened column keys off the captured stderr and leads with Console — + // the process's startup error (#1621). `erroredServerId` is the parent's + // connect-attempt-failure signal that gates the failure column; `network: []` + // (and empty History) keep it a pure stdio failure so only Console is offered. args: { activeServer: demoServers[0]!.id, + erroredServerId: demoServers[0]!.id, connectionStatus: "error", network: [], stderrLogs: demoStderr, diff --git a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx index 216b8c2c..d5eb9c29 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx @@ -1436,13 +1436,13 @@ describe("InspectorView", () => { await screen.findByRole("button", { name: "Close monitoring column" }), ).toBeInTheDocument(); - // The failure column offers History + Network (the captured request), but - // not Logs (a failed connect never negotiated the logging capability) nor - // Console (an HTTP transport has no child-process stderr). - expect(screen.getByRole("radio", { name: "History" })).toBeChecked(); - expect( - screen.getByRole("radio", { name: "Network" }), - ).toBeInTheDocument(); + // The failure column leads with Network (the captured request) — the only + // diagnostic with content. History is content-gated and empty here (the + // message log clears on the error transition), so it isn't offered; nor is + // Logs (no logging capability was negotiated) or Console (HTTP has no + // child-process stderr). + expect(screen.getByRole("radio", { name: "Network" })).toBeChecked(); + expect(screen.queryByRole("radio", { name: "History" })).toBeNull(); expect(screen.queryByRole("radio", { name: "Logs" })).toBeNull(); expect(screen.queryByRole("radio", { name: "Console" })).toBeNull(); }); @@ -1478,17 +1478,124 @@ describe("InspectorView", () => { })} />, ); - // The captured stderr means it was a stdio launch: the column surfaces - // Console (the process's stderr) and defaults to it — that's where the - // spawn error is — with no Network (no HTTP traffic). The stderr renders. + // The captured stderr means it was a stdio launch: the column leads with + // Console (the process's stderr) — that's where the spawn error is — with + // no Network (no HTTP traffic) and no History (content-gated, empty on a + // fresh failure). The stderr line renders. expect( await screen.findByRole("radio", { name: "Console" }), ).toBeChecked(); + expect(screen.queryByRole("radio", { name: "History" })).toBeNull(); + expect(screen.queryByRole("radio", { name: "Network" })).toBeNull(); + expect(screen.getByText("ModuleNotFoundError: boom")).toBeInTheDocument(); + }); + + it("leads with Console even when the stored tab was History, on a stdio failure (#1621)", async () => { + // Regression for the review note: a returning user whose last-pinned tab + // was History must still land on the Console diagnostic, not the (empty) + // History — History is content-gated out of the failure column. + window.localStorage.setItem("inspector.monitor.tab", "History"); + monitorWide.value = true; + const stdioErr: ServerEntry = { + id: "beta", + name: "Beta", + config: { type: "stdio", command: "missing-bin" }, + connection: { status: "disconnected" }, + }; + const { rerender } = renderWithMantine( + , + ); + rerender( + , + ); + expect( + await screen.findByRole("radio", { name: "Console" }), + ).toBeChecked(); + expect(screen.queryByRole("radio", { name: "History" })).toBeNull(); + }); + + it("keeps the failure column closed until a diagnostic has content (#1621)", () => { + // A connect failure with nothing captured yet (no stderr, no fetch, and + // History cleared on the error transition) opens onto nothing — so the + // column stays closed rather than showing an empty pane. + monitorWide.value = true; + const { rerender } = renderWithMantine( + , + ); + rerender( + , + ); + expect( + screen.queryByRole("button", { name: "Close monitoring column" }), + ).toBeNull(); + }); + + it("offers History in the failure column when it has captured content (#1621)", async () => { + // The failure History tab is content-gated, so when the message log *does* + // hold entries it's offered alongside the diagnostic (Network here). + monitorWide.value = true; + const { rerender } = renderWithMantine( + , + ); + rerender( + , + ); + expect( + await screen.findByRole("radio", { name: "Network" }), + ).toBeChecked(); expect( screen.getByRole("radio", { name: "History" }), ).toBeInTheDocument(); - expect(screen.queryByRole("radio", { name: "Network" })).toBeNull(); - expect(screen.getByText("ModuleNotFoundError: boom")).toBeInTheDocument(); }); it("does not auto-open on a mount that starts already errored (#1621)", () => { diff --git a/clients/web/src/components/views/InspectorView/InspectorView.tsx b/clients/web/src/components/views/InspectorView/InspectorView.tsx index 09c7132a..e9c8bbc5 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.tsx @@ -817,27 +817,29 @@ export function InspectorView({ 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. What explains the - // failure is whichever diagnostic actually captured something: + // server's `logging` capability) isn't meaningful. Offer exactly the tabs + // whose diagnostic actually *captured something* — keyed on content, not + // the declared transport (a connect failure fires the client `disconnect` + // event, which clears `activeServer`, so transport can't be read back): // • stdio → the process's stderr (Console): the spawn/startup error the // child printed before dying. // • HTTP → the failed requests (Network). - // We key off *captured content*, not the server's declared transport, - // because a connect failure fires the client `disconnect` event, which - // clears `activeServer` — so transport can't be read back reliably here. - // History is always offered, though a pre-handshake failure usually - // leaves it empty. Console leads (it's the stdio failure's story); Network - // trails History (matching the connected order). This memo re-runs as - // stderr/fetch entries stream in, so the diagnostic tab appears as soon as - // the failing process/request produces output. + // • History only if it has entries — the message log is cleared on the + // error transition, so on a fresh connect failure it's empty; offering + // it anyway would let an empty tab lead over (and hide) the real + // diagnostic. Content-gating it keeps the actual diagnostic first. + // If nothing captured anything yet, `monitorAvailable` is empty and the + // column stays closed rather than opening onto an empty pane; this memo + // re-runs as stderr/fetch entries stream in, so the column opens (and the + // diagnostic tab appears) the moment the failing process/request emits. const tabs: string[] = []; if (stderrLogs.length > 0) tabs.push(CONSOLE_TAB); - tabs.push(HISTORY_TAB); if (network.length > 0) tabs.push(NETWORK_TAB); + if (history.length > 0) tabs.push(HISTORY_TAB); return tabs; } return []; - }, [connected, failed, availableTabs, stderrLogs, network]); + }, [connected, failed, availableTabs, stderrLogs, network, history]); const effectivePinned = monitorPinned && !!isWide &&