diff --git a/clients/web/src/components/groups/ServerCard/ServerCard.tsx b/clients/web/src/components/groups/ServerCard/ServerCard.tsx index 54fd2e78..03449c4a 100644 --- a/clients/web/src/components/groups/ServerCard/ServerCard.tsx +++ b/clients/web/src/components/groups/ServerCard/ServerCard.tsx @@ -9,6 +9,7 @@ import { ServerStatusIndicator } from "../../elements/ServerStatusIndicator/Serv import { TransportBadge } from "../../elements/TransportBadge/TransportBadge"; import { ConnectionToggle } from "../../elements/ConnectionToggle/ConnectionToggle"; import { ContentViewer } from "../../elements/ContentViewer/ContentViewer"; +import { FAILED_CARD_SCROLL_DELAY_MS } from "../../views/InspectorView/monitorColumnAnimation"; export interface ServerCardProps extends ServerEntry { activeServer?: string; @@ -101,14 +102,6 @@ const RemoveButton = Button.withProps({ color: "red.6", }); -// Delay before scrolling a newly-failed card into view (#1621). A connection -// failure both flags the card and opens the monitoring column, which reflows -// the server grid (fewer, narrower columns) and can push the failed card below -// the fold. Deferring past the column's open lets that reflow settle so -// `scrollIntoView` targets the card's final position rather than its pre-reflow -// one. Kept just above the column's slide duration. -const ERROR_SCROLL_DELAY_MS = 320; - function getTransport(config: MCPServerConfig): ServerType { return config.type ?? "stdio"; } @@ -165,9 +158,11 @@ export function ServerCard({ }, [highlighted, scrollOnHighlight]); // Scroll the failed card into view on the disconnected→errored transition - // (#1621), deferred so the monitoring column's open + grid reflow settle - // first. Guarded by a ref so a re-render while still errored doesn't re-scroll - // and fight the user if they've scrolled away. + // (#1621), deferred past the monitoring column's open (`FAILED_CARD_SCROLL_ + // DELAY_MS`, derived from the column's slide duration) so the grid reflow + // settles before `scrollIntoView` measures the card. Guarded by a ref so a + // re-render while still errored doesn't re-scroll and fight the user if they've + // scrolled away. const wasErroredRef = useRef(errored); useEffect(() => { const justErrored = errored && !wasErroredRef.current; @@ -175,7 +170,7 @@ export function ServerCard({ if (!justErrored) return; const timer = setTimeout(() => { rootRef.current?.scrollIntoView({ behavior: "smooth", block: "center" }); - }, ERROR_SCROLL_DELAY_MS); + }, FAILED_CARD_SCROLL_DELAY_MS); return () => clearTimeout(timer); }, [errored]); const transport = getTransport(config); diff --git a/clients/web/src/components/screens/ConsoleScreen/ConsoleScreen.tsx b/clients/web/src/components/screens/ConsoleScreen/ConsoleScreen.tsx index f29dfe1c..fd00982f 100644 --- a/clients/web/src/components/screens/ConsoleScreen/ConsoleScreen.tsx +++ b/clients/web/src/components/screens/ConsoleScreen/ConsoleScreen.tsx @@ -183,7 +183,7 @@ export function ConsoleScreen({ {filteredEntries.map((entry, index) => ( - + {formatTimestamp(entry.timestamp)} diff --git a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx index 02a9fa2a..216b8c2c 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx @@ -1383,11 +1383,14 @@ describe("InspectorView", () => { }); // A failed connect leaves the errored server's id set but no - // initializeResult (capabilities were never negotiated). + // initializeResult (capabilities were never negotiated). `erroredServerId` + // is the parent's connect-attempt-failure signal (it survives the failure's + // `disconnect` clearing `activeServer`), which gates the failure column. function failedHttp(overrides: Partial = {}) { return makeProps({ servers: [httpServer], activeServer: "beta", + erroredServerId: "beta", connectionStatus: "error", initializeResult: undefined, ...overrides, @@ -1498,6 +1501,41 @@ describe("InspectorView", () => { ).toBeNull(); }); + it("does not treat a mid-session crash as a connect-attempt failure (#1621)", async () => { + // A previously-connected server that crashes settles to `error` too, but + // with no `erroredServerId` (the parent sets that only for connect + // attempts). That must NOT reorganize the column into the failure tab set + // — it closes like any session teardown, so a user who closed the column + // mid-session isn't reopened onto diagnostics. + monitorWide.value = true; + window.localStorage.setItem("inspector.monitor.pinned", "true"); + const { rerender } = renderWithMantine( + , + ); + // Connected + pinned: the column is open with the live monitor tabs. + expect( + await screen.findByRole("button", { name: "Close monitoring column" }), + ).toBeInTheDocument(); + + // Crash: connected → error, activeServer cleared, NO erroredServerId. + rerender( + , + ); + // The column closes (after its slide-out) rather than switching to the + // failure tabs. + await waitFor(() => + expect( + screen.queryByRole("button", { name: "Close monitoring column" }), + ).toBeNull(), + ); + }); + const stdioServer: ServerEntry = { id: "gamma", name: "Gamma", diff --git a/clients/web/src/components/views/InspectorView/InspectorView.tsx b/clients/web/src/components/views/InspectorView/InspectorView.tsx index 1a7e494f..09c7132a 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.tsx @@ -88,6 +88,7 @@ import { MonitoringScreen } from "../../groups/MonitoringScreen/MonitoringScreen import { ResizeHandle } from "../../elements/ResizeHandle/ResizeHandle"; import { getServerType } from "@inspector/core/mcp/config.js"; import { INSPECTOR_SERVERS_TAB } from "../../../utils/inspectorTabs"; +import { MONITOR_COLUMN_ANIM_MS } from "./monitorColumnAnimation"; const SORT_DEFAULT: SortDirection = "newest-first"; @@ -283,8 +284,9 @@ const MonitoringColumn = Stack.withProps({ // side column sliding open rather than snapping in. The primary screen keeps its // standard `ScreenStage` transition. Mantine plays `out → in` on enter and // `in → out` on exit. `AppShell.Main`'s `overflow: hidden` clips the off-screen -// portion during the slide. -const COLUMN_ANIM_MS = 300; +// portion during the slide. The duration is shared (`ServerCard` waits on it +// before scrolling a failed card into view) so the two can't drift. +const COLUMN_ANIM_MS = MONITOR_COLUMN_ANIM_MS; const columnSlide: MantineTransition = { in: { opacity: 1, transform: "translateX(0)" }, out: { opacity: 0, transform: "translateX(100%)" }, @@ -695,22 +697,30 @@ export function InspectorView({ }); // Open the monitoring column when a connection is established (#1616) OR when a - // connection attempt fails (#1621). Gated on the *transition into* the target + // connect *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. + // the column surfaces the live stream; on failure it surfaces the diagnostics + // that explain what went wrong. The column still only *appears* when wide + a + // monitor tab is available (`effectivePinned`); this just sets the preference. + // + // `"error"` is also the resting status of a *mid-session crash* of a + // previously-connected server (per isTerminalStatus/#1490), but that is NOT a + // connect attempt: reopening a column the user closed mid-session (and swapping + // their live tab set for the failure set) would be surprising. So the error + // arm requires the previous status to NOT be `connected` — i.e. we went + // connecting/disconnected → error, never connected → error. This keeps the + // auto-open aligned with the red-border (`erroredServerId`), which the parent + // also sets only for connect-attempt failures. const prevStatusRef = useRef(connectionStatus); useEffect(() => { const prev = prevStatusRef.current; prevStatusRef.current = connectionStatus; const becameConnected = connectionStatus === "connected" && prev !== "connected"; - const becameError = connectionStatus === "error" && prev !== "error"; + const becameError = + connectionStatus === "error" && prev !== "error" && prev !== "connected"; if (becameConnected || becameError) { setMonitorPinned(true); } @@ -797,9 +807,12 @@ export function InspectorView({ // `monitorPinned`, so it re-opens when the condition returns. Only the column's // close button writes `monitorPinned = false`. const connected = connectionStatus === "connected"; - // 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"; + // A failed connection *attempt* (#1621) keeps the column available so the user + // can see why, even though no live session exists. Gated on `erroredServerId` + // (set by the parent only for connect-attempt failures, not mid-session + // crashes) so a crash of a previously-connected server doesn't reorganize the + // column into the failure tab set — matching the auto-open effect above. + const failed = connectionStatus === "error" && erroredServerId !== undefined; const monitorAvailable = useMemo(() => { if (connected) return availableTabs.filter((t) => MONITOR_TABS.includes(t)); if (failed) { diff --git a/clients/web/src/components/views/InspectorView/monitorColumnAnimation.ts b/clients/web/src/components/views/InspectorView/monitorColumnAnimation.ts new file mode 100644 index 00000000..25017544 --- /dev/null +++ b/clients/web/src/components/views/InspectorView/monitorColumnAnimation.ts @@ -0,0 +1,15 @@ +/** + * Duration (ms) of the monitoring column's open/close slide (#1616). Shared so + * dependent timings can't silently drift from the animation: + * - `InspectorView` drives the column's Mantine `Transition` with it. + * - `ServerCard` waits just past it before scrolling a newly-failed card into + * view (#1621), so the column's open + the resulting grid reflow settle + * before `scrollIntoView` measures the card's position. + */ +export const MONITOR_COLUMN_ANIM_MS = 300; + +/** + * Delay (ms) before scrolling a newly-failed server card into view — the column + * animation plus a small buffer for the reflow to commit. + */ +export const FAILED_CARD_SCROLL_DELAY_MS = MONITOR_COLUMN_ANIM_MS + 20;