diff --git a/clients/web/src/App.css b/clients/web/src/App.css index 2d35f4b6..3a254bb9 100644 --- a/clients/web/src/App.css +++ b/clients/web/src/App.css @@ -53,6 +53,10 @@ /* Green border drawn around a freshly-added/highlighted server card (#1535) */ --inspector-highlight-border: var(--mantine-color-green-6); + /* Red border drawn around a server card whose last connection attempt failed, + until another server is connected/attempted (#1621) */ + --inspector-error-border: var(--mantine-color-red-6); + /* ── Log levels (RFC 5424) ─────────────────────────────── */ --inspector-log-debug: var(--mantine-color-gray-6); --inspector-log-info: var(--mantine-color-blue-6); diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 6257844d..a508512f 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -44,8 +44,18 @@ const { messageLogClear } = vi.hoisted(() => ({ messageLogClear: vi.fn() })); vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => { const actual = await importOriginal(); + // When set, the next `connect()` rejects with this value (one-shot), so a test + // can exercise the handshake-failure path. Cleared after it fires. + let nextConnectRejection: unknown = null; class FakeInspectorClient extends EventTarget { - connect = vi.fn().mockResolvedValue(undefined); + connect = vi.fn(() => { + if (nextConnectRejection !== null) { + const err = nextConnectRejection; + nextConnectRejection = null; + return Promise.reject(err); + } + return Promise.resolve(undefined); + }); disconnect = vi.fn().mockResolvedValue(undefined); callTool = vi .fn() @@ -88,6 +98,10 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => { }), // Test-only handle so the test can grab the live instance and fire events. __clientInstances: instances, + // Test-only: arm the next connect() to reject (handshake-failure path). + __rejectNextConnect: (err: unknown) => { + nextConnectRejection = err; + }, }; }); @@ -263,6 +277,7 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ readResourceState?: { status?: string }; currentLogLevel?: string; activeTab?: string; + erroredServerId?: string; onActiveTabChange: (tab: string) => void; onToggleConnection: (id: string) => void; onToolsUiChange: (next: { @@ -322,6 +337,9 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ {props.currentLogLevel} {props.activeTab ?? "none"} + + {props.erroredServerId ?? "none"} + @@ -450,10 +468,58 @@ const clientInstances = ( McpIndex as unknown as { __clientInstances: EventTarget[] } ).__clientInstances; +const rejectNextConnect = ( + McpIndex as unknown as { __rejectNextConnect: (err: unknown) => void } +).__rejectNextConnect; + const fetchLogInstances = ( FetchLogModule as unknown as { __fetchLogInstances: EventTarget[] } ).__fetchLogInstances; +describe("App failed-connection card border (#1621)", () => { + beforeEach(() => { + clientInstances.length = 0; + vi.mocked(useInspectorClient).mockReturnValue(DEFAULT_USE_INSPECTOR_CLIENT); + }); + + it("flags the server whose connect attempt fails as erroredServerId", async () => { + const user = userEvent.setup(); + renderWithMantine(); + expect(screen.getByTestId("errored-server")).toHaveTextContent("none"); + + // Arm the next connect() to reject with a plain (non-auth) handshake error. + rejectNextConnect(new Error("spawn failed")); + await user.click(screen.getByText("connect")); + + await waitFor(() => + expect(screen.getByTestId("errored-server")).toHaveTextContent("A"), + ); + }); + + it("clears the flag when a new connection attempt starts", async () => { + // Report status "error" so the *second* connect click is treated as a fresh + // attempt (not a disconnect of a live session), exercising the clear. + vi.mocked(useInspectorClient).mockReturnValue({ + ...DEFAULT_USE_INSPECTOR_CLIENT, + status: "error", + }); + const user = userEvent.setup(); + renderWithMantine(); + + rejectNextConnect(new Error("spawn failed")); + await user.click(screen.getByText("connect")); + await waitFor(() => + expect(screen.getByTestId("errored-server")).toHaveTextContent("A"), + ); + + // A new attempt (this one resolves) clears the red-border flag. + await user.click(screen.getByText("connect")); + await waitFor(() => + expect(screen.getByTestId("errored-server")).toHaveTextContent("none"), + ); + }); +}); + describe("App session-scoped state reset on disconnect", () => { beforeEach(() => { clientInstances.length = 0; diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 9e8f9a26..4608d55a 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -635,6 +635,16 @@ function App() { undefined, ); + // Id of the server whose last connection attempt failed (#1621). Drives the + // red border on that ServerCard. It is deliberately NOT reset by + // `resetSessionScopedUiState` (a failed connect fires the `disconnect` event, + // which would otherwise clear the flag the same tick we set it); instead it is + // cleared when a new connection attempt starts, and re-set if that attempt + // also fails. + const [failedServerId, setFailedServerId] = useState( + undefined, + ); + // InspectorClient + per-primitive state managers. All recreated together // whenever the user switches active servers, then destroyed when the // next switch happens (or when the component unmounts). @@ -2318,6 +2328,10 @@ function App() { if (id !== activeServerId) { setActiveServerId(id); } + // A new connection attempt has begun: clear any previous failure flag so + // the red border on the last-failed card is removed (#1621). If this + // attempt also fails, the catch below re-sets it for this server. + setFailedServerId(undefined); connectStartRef.current = Date.now(); try { @@ -2407,7 +2421,8 @@ function App() { // Non-auth handshake error: toast so the user sees what went wrong // instead of the ConnectionToggle silently reverting to - // "disconnected". + // "disconnected", and flag the card with a red border (#1621). + setFailedServerId(id); const message = err instanceof Error ? err.message : String(err); notifications.show({ title: `Failed to connect to "${target.name}"`, @@ -3657,6 +3672,7 @@ function App() { servers={servers} serverListWritable={serverListWritable} activeServer={activeServerId} + erroredServerId={failedServerId} connectionStatus={connectionStatus} initializeResult={initializeResult} latencyMs={latencyMs} diff --git a/clients/web/src/components/groups/ServerCard/ServerCard.stories.tsx b/clients/web/src/components/groups/ServerCard/ServerCard.stories.tsx index cd516b95..ac1265e6 100644 --- a/clients/web/src/components/groups/ServerCard/ServerCard.stories.tsx +++ b/clients/web/src/components/groups/ServerCard/ServerCard.stories.tsx @@ -87,6 +87,21 @@ export const Failed: Story = { }, }; +// The red border flagging a server whose last connection attempt failed +// (#1621). The status settles back to "disconnected" (the failure clears the +// active session), but the border persists until another server is +// connected/attempted. +export const ErroredBorder: Story = { + args: { + id: "3d2c1b0a-9f8e-4d6c-8b7a-1a2b3c4d5e6f", + name: "My MCP Server", + config: stdioConfig, + info: { name: "My MCP Server", version: "1.2.0" }, + connection: disconnected, + errored: true, + }, +}; + export const LongServerName: Story = { args: { id: "a1b2c3d4-e5f6-4789-9abc-def012345678", diff --git a/clients/web/src/components/groups/ServerCard/ServerCard.test.tsx b/clients/web/src/components/groups/ServerCard/ServerCard.test.tsx index ccf7c2b5..29b7bf91 100644 --- a/clients/web/src/components/groups/ServerCard/ServerCard.test.tsx +++ b/clients/web/src/components/groups/ServerCard/ServerCard.test.tsx @@ -363,4 +363,56 @@ describe("ServerCard", () => { expect(onClearHighlight).toHaveBeenCalledTimes(1); }); }); + + describe("connection-failed border (#1621)", () => { + it("draws the red errored-variant border when errored", () => { + const { container } = renderWithMantine( + , + ); + expect( + container.querySelector('[data-variant="errored"]'), + ).not.toBeNull(); + }); + + it("does not draw the errored border when not errored", () => { + const { container } = renderWithMantine( + , + ); + expect(container.querySelector('[data-variant="errored"]')).toBeNull(); + }); + + it("prefers the dimmed (disabled) variant over the errored border", () => { + // A dimmed card (another server active) is inert; that wins over the + // error border so the card can't look interactive. + const { container } = renderWithMantine( + , + ); + expect( + container.querySelector('[data-variant="disabled"]'), + ).not.toBeNull(); + expect(container.querySelector('[data-variant="errored"]')).toBeNull(); + }); + + it("prefers the errored border over the freshly-added highlight", () => { + const { container } = renderWithMantine( + , + ); + expect( + container.querySelector('[data-variant="errored"]'), + ).not.toBeNull(); + expect( + container.querySelector('[data-variant="highlighted"]'), + ).toBeNull(); + }); + }); }); diff --git a/clients/web/src/components/groups/ServerCard/ServerCard.tsx b/clients/web/src/components/groups/ServerCard/ServerCard.tsx index a93a6c30..6d2bfaa0 100644 --- a/clients/web/src/components/groups/ServerCard/ServerCard.tsx +++ b/clients/web/src/components/groups/ServerCard/ServerCard.tsx @@ -38,6 +38,12 @@ export interface ServerCardProps extends ServerEntry { * eye. Cleared by `onClearHighlight` on any click on the card. */ highlighted?: boolean; + /** + * When true, this server's last connection attempt failed: the card draws a + * red border to flag it (#1621). The parent clears this when another server + * is connected or a new connection is attempted. + */ + errored?: boolean; /** * Whether a highlighted card scrolls itself into view. When several cards are * highlighted at once (a batch import) only the first should scroll, so the @@ -135,6 +141,7 @@ export function ServerCard({ writable = true, dragHandle, highlighted = false, + errored = false, scrollOnHighlight = true, onClearHighlight, }: ServerCardProps) { @@ -155,12 +162,15 @@ export function ServerCard({ connection.status === "connected" ? connection.protocolVersion : undefined; // A dimmed card (another server is active) is inert, so the disabled variant - // wins; otherwise a freshly-added card draws the highlighted green border. + // wins; otherwise a failed-connection card draws the red error border, then a + // freshly-added card draws the highlighted green border. const variant = isDimmed ? "disabled" - : highlighted - ? "highlighted" - : undefined; + : errored + ? "errored" + : highlighted + ? "highlighted" + : undefined; return ( { }); }); + describe("connection-failed border (#1621)", () => { + it("draws the red errored border only on the failed server's card", () => { + const { container } = renderWithMantine( + , + ); + const errored = container.querySelectorAll('[data-variant="errored"]'); + expect(errored).toHaveLength(1); + // The errored card is Beta's — its name is within the flagged card. + expect(errored[0]).toHaveTextContent("Beta"); + }); + + it("draws no errored border when erroredServerId is unset", () => { + const { container } = renderWithMantine( + , + ); + expect( + container.querySelectorAll('[data-variant="errored"]'), + ).toHaveLength(0); + }); + }); + describe("freshly-added highlight", () => { it("draws a green highlight border on every highlighted server", () => { const { container } = renderWithMantine( diff --git a/clients/web/src/components/screens/ServerListScreen/ServerListScreen.tsx b/clients/web/src/components/screens/ServerListScreen/ServerListScreen.tsx index 4a94fbfc..9b903f77 100644 --- a/clients/web/src/components/screens/ServerListScreen/ServerListScreen.tsx +++ b/clients/web/src/components/screens/ServerListScreen/ServerListScreen.tsx @@ -39,6 +39,11 @@ export interface ServerListScreenProps { writable?: boolean; /** Id of the server the wiring layer treats as active (drives card dimming). */ activeServer?: string; + /** + * Id of the server whose last connection attempt failed (#1621). Its card + * draws a red border until another server is connected or attempted. + */ + erroredServerId?: string; onAddManually: () => void; onImportConfig: () => void; onImportServerJson: () => void; @@ -78,6 +83,7 @@ export function ServerListScreen({ servers, writable = true, activeServer, + erroredServerId, onAddManually, onImportConfig, onImportServerJson, @@ -138,6 +144,7 @@ export function ServerListScreen({ onClone, onRemove, highlighted: highlightedServerIds?.includes(server.id) ?? false, + errored: server.id === erroredServerId, scrollOnHighlight: server.id === firstHighlightedId, onClearHighlight: onClearHighlight ? () => onClearHighlight(server.id) diff --git a/clients/web/src/components/views/InspectorView/InspectorView.tsx b/clients/web/src/components/views/InspectorView/InspectorView.tsx index 07228bd3..1a7e494f 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.tsx @@ -347,6 +347,13 @@ export interface InspectorViewProps { // Connection state — driven by the parent via `useInspectorClient`. activeServer?: string; + /** + * Id of the server whose last connection attempt failed (#1621). Its card in + * the Servers screen draws a red border until another server is connected or + * a new connection is attempted. Independent of `activeServer`, which the + * parent clears on the failure's `disconnect` event. + */ + erroredServerId?: string; connectionStatus: ConnectionStatus; initializeResult?: InitializeResult; latencyMs?: number; @@ -524,6 +531,7 @@ export function InspectorView({ servers: serversInput, serverListWritable = true, activeServer, + erroredServerId, connectionStatus, initializeResult, latencyMs, @@ -1062,6 +1070,7 @@ export function InspectorView({ servers={servers} writable={serverListWritable} activeServer={dimCardsAgainst} + erroredServerId={erroredServerId} onAddManually={onServerAdd} onImportConfig={onServerImportConfig} onImportServerJson={onServerImportJson} diff --git a/clients/web/src/theme/Card.ts b/clients/web/src/theme/Card.ts index 1b597ec4..7e3a2c48 100644 --- a/clients/web/src/theme/Card.ts +++ b/clients/web/src/theme/Card.ts @@ -50,6 +50,19 @@ export const ThemeCard = Card.extend({ }, }; } + if (props.variant === "errored") { + // Server card whose last connection attempt failed: a red border flags it + // until another server is connected/attempted (#1621). Mirrors the + // `highlighted` variant; the border itself comes from the inherited + // `withBorder: true` default above. + return { + root: { + backgroundColor: "var(--inspector-surface-card)", + borderColor: "var(--inspector-error-border)", + borderWidth: 2, + }, + }; + } if (props.variant === "preview") { // Container for the resource preview / template form panels: sizes to // content (no forced height) but caps at the screen's available area