refactor(web): content-gate History in the failure column (#1621)
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
This commit is contained in:
parent
87630f2356
commit
a759777ceb
3 changed files with 138 additions and 27 deletions
|
|
@ -513,11 +513,13 @@ export const Connected: Story = {
|
||||||
export const ConnectionError: Story = {
|
export const ConnectionError: Story = {
|
||||||
// demoServers[0] is a stdio server; a failed launch has an empty History (the
|
// 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
|
// 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 —
|
// auto-opened column keys off the captured stderr and leads with Console —
|
||||||
// the process's startup error (#1621). `network: []` keeps it a pure stdio
|
// the process's startup error (#1621). `erroredServerId` is the parent's
|
||||||
// failure so Network isn't offered.
|
// 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: {
|
args: {
|
||||||
activeServer: demoServers[0]!.id,
|
activeServer: demoServers[0]!.id,
|
||||||
|
erroredServerId: demoServers[0]!.id,
|
||||||
connectionStatus: "error",
|
connectionStatus: "error",
|
||||||
network: [],
|
network: [],
|
||||||
stderrLogs: demoStderr,
|
stderrLogs: demoStderr,
|
||||||
|
|
|
||||||
|
|
@ -1436,13 +1436,13 @@ describe("InspectorView", () => {
|
||||||
await screen.findByRole("button", { name: "Close monitoring column" }),
|
await screen.findByRole("button", { name: "Close monitoring column" }),
|
||||||
).toBeInTheDocument();
|
).toBeInTheDocument();
|
||||||
|
|
||||||
// The failure column offers History + Network (the captured request), but
|
// The failure column leads with Network (the captured request) — the only
|
||||||
// not Logs (a failed connect never negotiated the logging capability) nor
|
// diagnostic with content. History is content-gated and empty here (the
|
||||||
// Console (an HTTP transport has no child-process stderr).
|
// message log clears on the error transition), so it isn't offered; nor is
|
||||||
expect(screen.getByRole("radio", { name: "History" })).toBeChecked();
|
// Logs (no logging capability was negotiated) or Console (HTTP has no
|
||||||
expect(
|
// child-process stderr).
|
||||||
screen.getByRole("radio", { name: "Network" }),
|
expect(screen.getByRole("radio", { name: "Network" })).toBeChecked();
|
||||||
).toBeInTheDocument();
|
expect(screen.queryByRole("radio", { name: "History" })).toBeNull();
|
||||||
expect(screen.queryByRole("radio", { name: "Logs" })).toBeNull();
|
expect(screen.queryByRole("radio", { name: "Logs" })).toBeNull();
|
||||||
expect(screen.queryByRole("radio", { name: "Console" })).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
|
// The captured stderr means it was a stdio launch: the column leads with
|
||||||
// Console (the process's stderr) and defaults to it — that's where the
|
// Console (the process's stderr) — that's where the spawn error is — with
|
||||||
// spawn error is — with no Network (no HTTP traffic). The stderr renders.
|
// no Network (no HTTP traffic) and no History (content-gated, empty on a
|
||||||
|
// fresh failure). The stderr line renders.
|
||||||
expect(
|
expect(
|
||||||
await screen.findByRole("radio", { name: "Console" }),
|
await screen.findByRole("radio", { name: "Console" }),
|
||||||
).toBeChecked();
|
).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(
|
||||||
|
<StatefulInspectorViewHost
|
||||||
|
{...makeProps({
|
||||||
|
servers: [stdioErr],
|
||||||
|
activeServer: undefined,
|
||||||
|
connectionStatus: "disconnected",
|
||||||
|
})}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
rerender(
|
||||||
|
<StatefulInspectorViewHost
|
||||||
|
{...failedHttp({
|
||||||
|
servers: [stdioErr],
|
||||||
|
activeServer: undefined,
|
||||||
|
stderrLogs: [
|
||||||
|
{ timestamp: new Date(), message: "ModuleNotFoundError: boom" },
|
||||||
|
],
|
||||||
|
})}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
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(
|
||||||
|
<StatefulInspectorViewHost
|
||||||
|
{...makeProps({
|
||||||
|
servers: [httpServer],
|
||||||
|
activeServer: undefined,
|
||||||
|
connectionStatus: "disconnected",
|
||||||
|
})}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
rerender(
|
||||||
|
<StatefulInspectorViewHost
|
||||||
|
{...failedHttp({ network: [], history: [], stderrLogs: [] })}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
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(
|
||||||
|
<StatefulInspectorViewHost
|
||||||
|
{...makeProps({
|
||||||
|
servers: [httpServer],
|
||||||
|
activeServer: undefined,
|
||||||
|
connectionStatus: "disconnected",
|
||||||
|
})}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
rerender(
|
||||||
|
<StatefulInspectorViewHost
|
||||||
|
{...failedHttp({
|
||||||
|
network: [
|
||||||
|
{
|
||||||
|
id: "f1",
|
||||||
|
timestamp: new Date(),
|
||||||
|
method: "POST",
|
||||||
|
url: "http://localhost:3000/mcp",
|
||||||
|
requestHeaders: {},
|
||||||
|
error: "boom",
|
||||||
|
category: "transport",
|
||||||
|
},
|
||||||
|
],
|
||||||
|
history: [
|
||||||
|
{
|
||||||
|
id: "h1",
|
||||||
|
timestamp: new Date(),
|
||||||
|
direction: "request",
|
||||||
|
message: { jsonrpc: "2.0", id: 1, method: "initialize" },
|
||||||
|
},
|
||||||
|
],
|
||||||
|
})}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
expect(
|
||||||
|
await screen.findByRole("radio", { name: "Network" }),
|
||||||
|
).toBeChecked();
|
||||||
expect(
|
expect(
|
||||||
screen.getByRole("radio", { name: "History" }),
|
screen.getByRole("radio", { name: "History" }),
|
||||||
).toBeInTheDocument();
|
).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)", () => {
|
it("does not auto-open on a mount that starts already errored (#1621)", () => {
|
||||||
|
|
|
||||||
|
|
@ -817,27 +817,29 @@ export function InspectorView({
|
||||||
if (connected) return availableTabs.filter((t) => MONITOR_TABS.includes(t));
|
if (connected) return availableTabs.filter((t) => MONITOR_TABS.includes(t));
|
||||||
if (failed) {
|
if (failed) {
|
||||||
// A failed connect never negotiated capabilities, so Logs (gated on the
|
// A failed connect never negotiated capabilities, so Logs (gated on the
|
||||||
// server's `logging` capability) isn't meaningful. What explains the
|
// server's `logging` capability) isn't meaningful. Offer exactly the tabs
|
||||||
// failure is whichever diagnostic actually captured something:
|
// 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
|
// • stdio → the process's stderr (Console): the spawn/startup error the
|
||||||
// child printed before dying.
|
// child printed before dying.
|
||||||
// • HTTP → the failed requests (Network).
|
// • HTTP → the failed requests (Network).
|
||||||
// We key off *captured content*, not the server's declared transport,
|
// • History only if it has entries — the message log is cleared on the
|
||||||
// because a connect failure fires the client `disconnect` event, which
|
// error transition, so on a fresh connect failure it's empty; offering
|
||||||
// clears `activeServer` — so transport can't be read back reliably here.
|
// it anyway would let an empty tab lead over (and hide) the real
|
||||||
// History is always offered, though a pre-handshake failure usually
|
// diagnostic. Content-gating it keeps the actual diagnostic first.
|
||||||
// leaves it empty. Console leads (it's the stdio failure's story); Network
|
// If nothing captured anything yet, `monitorAvailable` is empty and the
|
||||||
// trails History (matching the connected order). This memo re-runs as
|
// column stays closed rather than opening onto an empty pane; this memo
|
||||||
// stderr/fetch entries stream in, so the diagnostic tab appears as soon as
|
// re-runs as stderr/fetch entries stream in, so the column opens (and the
|
||||||
// the failing process/request produces output.
|
// diagnostic tab appears) the moment the failing process/request emits.
|
||||||
const tabs: string[] = [];
|
const tabs: string[] = [];
|
||||||
if (stderrLogs.length > 0) tabs.push(CONSOLE_TAB);
|
if (stderrLogs.length > 0) tabs.push(CONSOLE_TAB);
|
||||||
tabs.push(HISTORY_TAB);
|
|
||||||
if (network.length > 0) tabs.push(NETWORK_TAB);
|
if (network.length > 0) tabs.push(NETWORK_TAB);
|
||||||
|
if (history.length > 0) tabs.push(HISTORY_TAB);
|
||||||
return tabs;
|
return tabs;
|
||||||
}
|
}
|
||||||
return [];
|
return [];
|
||||||
}, [connected, failed, availableTabs, stderrLogs, network]);
|
}, [connected, failed, availableTabs, stderrLogs, network, history]);
|
||||||
const effectivePinned =
|
const effectivePinned =
|
||||||
monitorPinned &&
|
monitorPinned &&
|
||||||
!!isWide &&
|
!!isWide &&
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue