feat(web): red border on a server card whose connection attempt failed (#1621)
Flags the server whose last connection attempt failed with a red card border, cleared when another server is connected or a new connection is attempted — a persistent, at-a-glance companion to the failure toast and the auto-opened monitoring column. - App tracks `failedServerId`: set in onToggleConnection's non-auth handshake catch, cleared at the start of any new connect attempt. It is deliberately kept out of resetSessionScopedUiState, since the failure's own `disconnect` event would otherwise clear it the same tick (that same event clears `activeServer`, which is why the flag is tracked separately). - New `errored` Card theme variant (red border via a new `--inspector-error-border` token), mirroring the `highlighted` green border. Precedence: disabled (dimmed) > errored > highlighted. - Plumbed `erroredServerId` through InspectorView → ServerListScreen → ServerCard (`errored` prop). Full `npm run ci` passes (validate + coverage + smoke + storybook). 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
46b311ad5c
commit
c6999dff96
10 changed files with 219 additions and 6 deletions
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -44,8 +44,18 @@ const { messageLogClear } = vi.hoisted(() => ({ messageLogClear: vi.fn() }));
|
|||
vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
|
||||
const actual =
|
||||
await importOriginal<typeof import("@inspector/core/mcp/index.js")>();
|
||||
// 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", () => ({
|
|||
</span>
|
||||
<span data-testid="log-level">{props.currentLogLevel}</span>
|
||||
<span data-testid="active-tab">{props.activeTab ?? "none"}</span>
|
||||
<span data-testid="errored-server">
|
||||
{props.erroredServerId ?? "none"}
|
||||
</span>
|
||||
<button onClick={() => props.onActiveTabChange("Servers")}>
|
||||
switch-servers-tab
|
||||
</button>
|
||||
|
|
@ -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(<App />);
|
||||
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(<App />);
|
||||
|
||||
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;
|
||||
|
|
|
|||
|
|
@ -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<string | undefined>(
|
||||
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}
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
<ServerCard {...baseProps} connection={disconnected} errored />,
|
||||
);
|
||||
expect(
|
||||
container.querySelector('[data-variant="errored"]'),
|
||||
).not.toBeNull();
|
||||
});
|
||||
|
||||
it("does not draw the errored border when not errored", () => {
|
||||
const { container } = renderWithMantine(
|
||||
<ServerCard {...baseProps} connection={disconnected} />,
|
||||
);
|
||||
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(
|
||||
<ServerCard
|
||||
{...baseProps}
|
||||
connection={disconnected}
|
||||
activeServer="other"
|
||||
errored
|
||||
/>,
|
||||
);
|
||||
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(
|
||||
<ServerCard
|
||||
{...baseProps}
|
||||
connection={disconnected}
|
||||
errored
|
||||
highlighted
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
container.querySelector('[data-variant="errored"]'),
|
||||
).not.toBeNull();
|
||||
expect(
|
||||
container.querySelector('[data-variant="highlighted"]'),
|
||||
).toBeNull();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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 (
|
||||
<Card
|
||||
|
|
|
|||
|
|
@ -136,6 +136,27 @@ describe("ServerListScreen", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("connection-failed border (#1621)", () => {
|
||||
it("draws the red errored border only on the failed server's card", () => {
|
||||
const { container } = renderWithMantine(
|
||||
<ServerListScreen {...baseProps} erroredServerId="beta" />,
|
||||
);
|
||||
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(
|
||||
<ServerListScreen {...baseProps} />,
|
||||
);
|
||||
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(
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in a new issue