From db1916cd3410013c903ee947b6634ffce8b6276d Mon Sep 17 00:00:00 2001 From: Cliff Hall Date: Sat, 23 May 2026 14:26:23 -0400 Subject: [PATCH] feat(servers): persist server list to ~/.mcp-inspector/mcp.json (#1343) (#1349) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(servers): persist server list to ~/.mcp-inspector/mcp.json (#1343) Replaces the hardcoded SEED_SERVERS in App.tsx with a file-backed list at ~/.mcp-inspector/mcp.json. Full CRUD round-trips through new /api/servers routes; first launch writes the two existing seeds if the file is absent. Backend (core): - store-io: getDefaultMcpConfigPath() — sibling of getDefaultStorageDir() - core/mcp/serverList.ts: pure converters MCPConfig <-> ServerEntry[], DEFAULT_SEED_CONFIG, and normalizeServerType (moved from node/config.ts to keep serverList.ts Node-free) - core/mcp/remote/node/server.ts: GET/POST/PUT/:id/DELETE/:id /api/servers routes, plus RemoteServerOptions.mcpConfigPath defaulting to getDefaultMcpConfigPath(). Ids validated with validateStoreId. Frontend: - core/react/useServers.ts: { servers, loading, error, refresh, addServer, updateServer, removeServer } over the new endpoints - App.tsx: drops SEED_SERVERS, wires useServers, disconnects on remove of the active server, follows id on rename of the active server - ServerConfigModal: Add/Edit/Clone form (stdio: command/args/env/cwd; sse/streamable-http: url/headers as one-per-line text) - ServerRemoveConfirmModal: confirm-on-remove with id + transport summary Tests (52 new): - unit: getDefaultMcpConfigPath env-var permutations, serverList round-trip, hook flows against real Hono app.fetch (no TCP), modal forms + validation - integration: /api/servers routes against a real createRemoteApp + tmp file - stories with play functions for both new modals Co-Authored-By: Claude Opus 4.7 (1M context) * fix(servers): address PR #1349 review (correctness + nits) Bugs: - normalizeServerType: validate against the allowed set and default unknown / non-string types to "stdio" (matches the missing-type case). Prevents a hand-edited {"type":"websocket"} from leaking past `as ServerType` and surprising downstream narrowing. - onConfirmRemove: when removing the active server, also destroy() the 9 state managers and null them + the client out so they GC immediately instead of waiting for the next server switch. - /api/servers mutating routes: serialize POST/PUT/DELETE through an in-process write-lock so concurrent read-modify-write cycles can't lose updates. New integration test fires 25 parallel POSTs and asserts every entry lands. - readMcpConfig comment: update to match actual behavior (valid-JSON file with no `mcpServers` → empty; invalid JSON → 500 from the outer try, so corruption surfaces rather than silently emptying the list). New integration tests pin the 500 path on both GET and POST. Nits: - Add --inspector-surface-subtle token, use it in ServerRemoveConfirmModal instead of the raw Mantine var. - Rename useServers buildHeaders boolean param to includeJsonBody. - Add a comment on the env-value vs header-value trim asymmetry (env values preserve trailing whitespace; header values follow RFC 7230). Tests: 1630 unit + 404 integration + 306 storybook all green. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(ServerConfigModal): null currentTarget in functional setState updater React nulls SyntheticEvent.currentTarget after the synchronous handler returns. Reading e.currentTarget.value inside `setForm((f) => ...)` worked under happy-dom (which keeps currentTarget around) but threw "Cannot read properties of null (reading 'value')" on the first keystroke in a real browser. Capture the value before the functional updater closes over it. All seven onChange handlers in ServerConfigModal had the bug. Extended the AddEmpty storybook play to type into Server ID and Command — the real-Chromium runner now exercises this path so a future regression fails the test suite instead of the user. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(remote-server): surface stderr + transport_error when subprocess crashes during startup A stdio subprocess that spawns successfully but exits before the browser opens the SSE events stream used to vanish into a bare "Session not found" 404. The fix has three parts: 1. RemoteSession.markTransportDead always pushes the transport_error event (via pushEvent's queue) rather than dropping it when no consumer is attached. The queued event drains when the consumer eventually arrives. 2. /api/mcp/connect's transport.onclose handler no longer deletes the session immediately when no consumer is attached. Instead it holds the session (with queued stderr + transport_error) for a 30s grace window so the events endpoint can drain it. A timeout sweeps if no consumer ever connects. 3. /api/mcp/events detects isTransportDead after attaching the consumer (which just drained the queue), yields once to flush, then closes the stream and deletes the session. End result: the user sees the actual error (e.g. "Cannot find module" from stderr + the transport-closed reason) instead of a confusing 404. Tests: - New unit tests (clients/web/src/test/integration/mcp/remote/remote-session.test.ts) cover the queueing behavior, live delivery, and clearEventConsumer signaling under all transport-state permutations. - New integration test (connect-crash.test.ts) spawns a node subprocess that writes to stderr and exits, then verifies the events stream delivers both the stderr line and the transport_error event before closing — the exact scenario from PR #1349. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(state): gate listRequestorTasks on the server's `tasks` capability ManagedRequestorTasksState was calling tasks/list on every connect. Servers that don't advertise the `tasks` capability (the common case) reply with -32601 "Method not found", which then surfaces in the inspector's console for every connect. Skip the call when the server doesn't declare `tasks` — empty tasks list is the right semantics. Existing tests opt into capabilities so the live-path coverage is preserved; two new tests pin the gated behavior (direct refresh + connect-event-triggered refresh). Co-Authored-By: Claude Opus 4.7 (1M context) * fix(App): clear activeServerId on disconnect so other server cards re-enable ServerCard dims any card whose id differs from `activeServer` (and marks it inert), so cards stay non-interactive after a disconnect until the user reconnects elsewhere. activeServerId was set on connect but never reset on disconnect, leaving the whole screen locked to the just- disconnected card. Subscribe to InspectorClient's `disconnect` event and clear activeServerId there. Covers all three disconnect paths uniformly: - user toggles the connected card off - user clicks the header Disconnect button - mid-session transport failure (subprocess exit / network drop) Event-based instead of effect-based on connectionStatus to avoid the first-render-clobbers-new-id trap (status starts as "disconnected" for a freshly-created client before connect() runs). Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- clients/web/src/App.css | 3 + clients/web/src/App.tsx | 326 +++++++++---- .../ServerConfigModal.stories.tsx | 125 +++++ .../ServerConfigModal.test.tsx | 308 ++++++++++++ .../ServerConfigModal/ServerConfigModal.tsx | 438 +++++++++++++++++ .../ServerRemoveConfirmModal.stories.tsx | 88 ++++ .../ServerRemoveConfirmModal.test.tsx | 131 ++++++ .../ServerRemoveConfirmModal.tsx | 103 ++++ .../web/src/test/core/mcp/serverList.test.ts | 195 ++++++++ .../state/managedRequestorTasksState.test.ts | 37 +- .../react/useManagedRequestorTasks.test.tsx | 7 +- .../src/test/core/react/useServers.test.tsx | 294 ++++++++++++ .../storage/getDefaultMcpConfigPath.test.ts | 52 +++ .../mcp/remote/connect-crash.test.ts | 153 ++++++ .../mcp/remote/remote-session.test.ts | 78 ++++ .../mcp/remote/servers-route.test.ts | 439 ++++++++++++++++++ core/mcp/node/config.ts | 19 +- core/mcp/remote/node/remote-session.ts | 22 +- core/mcp/remote/node/server.ts | 220 ++++++++- core/mcp/serverList.ts | 100 ++++ core/mcp/state/managedRequestorTasksState.ts | 10 + core/react/useServers.ts | 156 +++++++ core/storage/store-io.ts | 9 + 23 files changed, 3176 insertions(+), 137 deletions(-) create mode 100644 clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.stories.tsx create mode 100644 clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx create mode 100644 clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx create mode 100644 clients/web/src/components/groups/ServerRemoveConfirmModal/ServerRemoveConfirmModal.stories.tsx create mode 100644 clients/web/src/components/groups/ServerRemoveConfirmModal/ServerRemoveConfirmModal.test.tsx create mode 100644 clients/web/src/components/groups/ServerRemoveConfirmModal/ServerRemoveConfirmModal.tsx create mode 100644 clients/web/src/test/core/mcp/serverList.test.ts create mode 100644 clients/web/src/test/core/react/useServers.test.tsx create mode 100644 clients/web/src/test/core/storage/getDefaultMcpConfigPath.test.ts create mode 100644 clients/web/src/test/integration/mcp/remote/connect-crash.test.ts create mode 100644 clients/web/src/test/integration/mcp/remote/remote-session.test.ts create mode 100644 clients/web/src/test/integration/mcp/remote/servers-route.test.ts create mode 100644 core/mcp/serverList.ts create mode 100644 core/react/useServers.ts diff --git a/clients/web/src/App.css b/clients/web/src/App.css index b73f7a19..916d6578 100644 --- a/clients/web/src/App.css +++ b/clients/web/src/App.css @@ -25,6 +25,9 @@ --inspector-surface-body: var(--mantine-color-dark-9); --inspector-surface-card: var(--mantine-color-gray-0); --inspector-surface-code: var(--mantine-color-white); + /* Subtle inset surface for modal call-outs / summary panels. Mantine's + `default-hover` already adapts to color scheme, so no dark override. */ + --inspector-surface-subtle: var(--mantine-color-default-hover); /* ── Inputs ────────────────────────────────────────────── */ --inspector-input-background: var(--mantine-color-white); diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index ddcc1f24..ba21cf19 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -9,7 +9,11 @@ import type { import type { AppBridge } from "@modelcontextprotocol/ext-apps/app-bridge"; import { InspectorClient } from "@inspector/core/mcp/index.js"; import type { JsonValue } from "@inspector/core/mcp/index.js"; -import type { MessageEntry, ServerEntry } from "@inspector/core/mcp/types.js"; +import type { + MCPServerConfig, + MessageEntry, + ServerEntry, +} from "@inspector/core/mcp/types.js"; import { API_SERVER_ENV_VARS } from "@inspector/core/mcp/remote/constants.js"; import { ManagedToolsState } from "@inspector/core/mcp/state/managedToolsState.js"; import { ManagedPromptsState } from "@inspector/core/mcp/state/managedPromptsState.js"; @@ -22,6 +26,7 @@ import { FetchRequestLogState } from "@inspector/core/mcp/state/fetchRequestLogS import { StderrLogState } from "@inspector/core/mcp/state/stderrLogState.js"; import type { RedirectUrlProvider } from "@inspector/core/auth/index.js"; import { useInspectorClient } from "@inspector/core/react/useInspectorClient.js"; +import { useServers } from "@inspector/core/react/useServers.js"; import { useManagedTools } from "@inspector/core/react/useManagedTools.js"; import { useManagedPrompts } from "@inspector/core/react/useManagedPrompts.js"; import { useManagedResources } from "@inspector/core/react/useManagedResources.js"; @@ -35,38 +40,13 @@ import type { GetPromptState } from "./components/screens/PromptsScreen/PromptsS import type { ReadResourceState } from "./components/screens/ResourcesScreen/ResourcesScreen"; import type { BridgeFactory } from "./components/elements/AppRenderer/AppRenderer"; import type { LogEntryData } from "./components/elements/LogEntry/LogEntry"; +import { + ServerConfigModal, + type ServerConfigModalMode, +} from "./components/groups/ServerConfigModal/ServerConfigModal"; +import { ServerRemoveConfirmModal } from "./components/groups/ServerRemoveConfirmModal/ServerRemoveConfirmModal"; import { createWebEnvironment } from "./lib/environmentFactory"; -// Hardcoded seed servers so the Servers screen has something to connect to. -// Persistence + an "Add server" UI are explicitly out of scope for #1244 (the -// useServers v2-only hook is a separate effort); follow-up work will replace -// this with a real `useServers` store. The two seeds here cover the common -// shapes a developer reaches for first: a real filesystem (scoped to /tmp so -// nothing destructive is possible by default) and the canonical "everything" -// reference server (tools / prompts / resources / sampling / completion). -const SEED_SERVERS: ServerEntry[] = [ - { - id: "filesystem-server-default", - name: "Local Filesystem (npx)", - config: { - type: "stdio", - command: "npx", - args: ["-y", "@modelcontextprotocol/server-filesystem", "/tmp"], - }, - connection: { status: "disconnected" }, - }, - { - id: "everything-server-default", - name: "Everything (npx)", - config: { - type: "stdio", - command: "npx", - args: ["-y", "@modelcontextprotocol/server-everything"], - }, - connection: { status: "disconnected" }, - }, -]; - // OAuth redirect URL provider — points at the dev backend's `/oauth/callback` // handler. The InspectorClient only consults this when the active server // requires OAuth; for stdio MCP servers it's never used. Created once and @@ -150,9 +130,24 @@ function App() { setColorScheme(isDark ? "light" : "dark"); }, [isDark, setColorScheme]); - // Server list — held locally; one seed entry per #1244's "hardcoded - // sample server" scope decision. Future work: useServers hook. - const [servers] = useState(SEED_SERVERS); + // Server list — sourced from ~/.mcp-inspector/mcp.json via the backend's + // `/api/servers` routes. First-launch seeds are written by the backend when + // the file is absent, so this hook returns a non-empty list on first load. + const { servers, addServer, updateServer, removeServer } = useServers({ + baseUrl: + typeof window !== "undefined" + ? window.location.origin + : "http://localhost", + authToken: getAuthToken(), + }); + + // CRUD-modal state. `configModal` drives Add / Edit / Clone via a single + // shared form modal; `removeTarget` drives the remove-confirmation modal. + const [configModal, setConfigModal] = useState<{ + mode: ServerConfigModalMode; + targetId?: string; + } | null>(null); + const [removeTarget, setRemoveTarget] = useState(null); // The active connection target. `null` between sessions; set as soon as // the user toggles a server card on. Drives state-manager lifetime. @@ -277,6 +272,25 @@ function App() { }; }, [inspectorClient]); + // Reset activeServerId whenever the live session ends. Without this the + // other ServerCards stay `inert` after disconnect — ServerCard dims any + // card whose id differs from `activeServer`. Subscribing to + // InspectorClient's own `disconnect` event covers all three paths + // (explicit toggle, header Disconnect button, mid-session transport + // failure / process exit) and avoids the first-render-clobbers-new-id + // trap that watching connectionStatus has (status starts as + // "disconnected" for the new client before connect() runs). + useEffect(() => { + if (!inspectorClient) return; + const onDisconnect = () => { + setActiveServerId(undefined); + }; + inspectorClient.addEventListener("disconnect", onDisconnect); + return () => { + inspectorClient.removeEventListener("disconnect", onDisconnect); + }; + }, [inspectorClient]); + // Build the InitializeResult the connected ViewHeader expects from the // hook's split fields. `protocolVersion` is hard-coded for now — the // useInspectorClient hook doesn't expose it. TODO(#1324): consume the @@ -591,6 +605,92 @@ function App() { /* TODO: not wired yet */ }, []); + // Remove handler — runs after the user confirms in the modal. When removing + // the active server, also tear down the session in-place so the client and + // its 9 state managers can be GC'd now instead of lingering until the next + // server switch. Mirrors the destroy sequence at the top of + // `setupClientForServer` (lines ~304-312) but additionally nulls every ref. + const onConfirmRemove = useCallback(async () => { + if (!removeTarget) return; + const id = removeTarget.id; + if (id === activeServerId) { + if (inspectorClient) { + await inspectorClient.disconnect(); + } + managedToolsState?.destroy(); + managedPromptsState?.destroy(); + managedResourcesState?.destroy(); + managedResourceTemplatesState?.destroy(); + managedRequestorTasksState?.destroy(); + resourceSubscriptionsState?.destroy(); + messageLogState?.destroy(); + fetchRequestLogState?.destroy(); + stderrLogState?.destroy(); + setInspectorClient(null); + setManagedToolsState(null); + setManagedPromptsState(null); + setManagedResourcesState(null); + setManagedResourceTemplatesState(null); + setManagedRequestorTasksState(null); + setResourceSubscriptionsState(null); + setMessageLogState(null); + setFetchRequestLogState(null); + setStderrLogState(null); + setActiveServerId(undefined); + } + await removeServer(id); + setRemoveTarget(null); + }, [ + removeTarget, + activeServerId, + inspectorClient, + managedToolsState, + managedPromptsState, + managedResourcesState, + managedResourceTemplatesState, + managedRequestorTasksState, + resourceSubscriptionsState, + messageLogState, + fetchRequestLogState, + stderrLogState, + removeServer, + ]); + + // Submit handler for the Add / Edit / Clone modal. Add and Clone both go + // through addServer; Edit uses updateServer (which supports id rename). + // On rename of the active server, keep activeServerId pointed at the new id. + const onConfigSubmit = useCallback( + async (id: string, config: MCPServerConfig) => { + if (configModal?.mode === "edit" && configModal.targetId) { + const originalId = configModal.targetId; + await updateServer(originalId, id, config); + if (originalId === activeServerId && id !== originalId) { + setActiveServerId(id); + } + return; + } + // add or clone + await addServer(id, config); + }, + [configModal, addServer, updateServer, activeServerId], + ); + + // Derive the existingIds list the modal uses for uniqueness validation. + // In edit mode the target's own id must be excluded so saving without + // renaming doesn't trip the "already exists" check. + const existingIds = useMemo(() => { + const ids = servers.map((s) => s.id); + if (configModal?.mode === "edit" && configModal.targetId) { + return ids.filter((id) => id !== configModal.targetId); + } + return ids; + }, [servers, configModal]); + + const configModalTarget = useMemo(() => { + if (!configModal?.targetId) return undefined; + return servers.find((s) => s.id === configModal.targetId); + }, [configModal, servers]); + // The Resources screen needs `isSubscribed` to flip the Subscribe button // label to "Unsubscribe". Derive it from the live subscriptions list rather // than threading it through every setReadResourceState site — that way the @@ -608,75 +708,95 @@ function App() { }, [readResourceState, subscriptions]); return ( - { - void onToggleConnection(id); - }} - onDisconnect={() => { - void onDisconnect(); - }} - onServerAdd={todoNoop} - onServerImportConfig={todoNoop} - onServerImportJson={todoNoop} - onServerInfo={todoNoop} - onServerSettings={todoNoop} - onServerEdit={todoNoop} - onServerClone={todoNoop} - onServerRemove={todoNoop} - onCallTool={(name, args) => { - void onCallTool(name, args); - }} - onClearToolResult={onClearToolResult} - onRefreshTools={onRefreshTools} - onGetPrompt={(name, args) => { - void onGetPrompt(name, args); - }} - onRefreshPrompts={onRefreshPrompts} - onReadResource={(uri) => { - void onReadResource(uri); - }} - onSubscribeResource={onSubscribeResource} - onUnsubscribeResource={onUnsubscribeResource} - onRefreshResources={onRefreshResources} - onCompleteArgument={onCompleteArgument} - completionsSupported={capabilities?.completions !== undefined} - onCancelTask={onCancelTask} - onClearCompletedTasks={todoNoop} - onRefreshTasks={onRefreshTasks} - onSetLogLevel={onSetLogLevel} - onClearLogs={onClearLogs} - onExportLogs={todoNoop} - onCopyAllLogs={todoNoop} - onClearHistory={onClearHistory} - onExportHistory={todoNoop} - onReplayHistory={todoNoop} - onTogglePinHistory={todoNoop} - onSelectApp={todoNoop} - onOpenApp={todoNoop} - onCloseApp={todoNoop} - onRefreshApps={onRefreshTools} - /> + <> + { + void onToggleConnection(id); + }} + onDisconnect={() => { + void onDisconnect(); + }} + onServerAdd={() => setConfigModal({ mode: "add" })} + onServerImportConfig={todoNoop} + onServerImportJson={todoNoop} + onServerInfo={todoNoop} + onServerSettings={todoNoop} + onServerEdit={(id) => setConfigModal({ mode: "edit", targetId: id })} + onServerClone={(id) => setConfigModal({ mode: "clone", targetId: id })} + onServerRemove={(id) => { + const target = servers.find((s) => s.id === id); + if (target) setRemoveTarget(target); + }} + onCallTool={(name, args) => { + void onCallTool(name, args); + }} + onClearToolResult={onClearToolResult} + onRefreshTools={onRefreshTools} + onGetPrompt={(name, args) => { + void onGetPrompt(name, args); + }} + onRefreshPrompts={onRefreshPrompts} + onReadResource={(uri) => { + void onReadResource(uri); + }} + onSubscribeResource={onSubscribeResource} + onUnsubscribeResource={onUnsubscribeResource} + onRefreshResources={onRefreshResources} + onCompleteArgument={onCompleteArgument} + completionsSupported={capabilities?.completions !== undefined} + onCancelTask={onCancelTask} + onClearCompletedTasks={todoNoop} + onRefreshTasks={onRefreshTasks} + onSetLogLevel={onSetLogLevel} + onClearLogs={onClearLogs} + onExportLogs={todoNoop} + onCopyAllLogs={todoNoop} + onClearHistory={onClearHistory} + onExportHistory={todoNoop} + onReplayHistory={todoNoop} + onTogglePinHistory={todoNoop} + onSelectApp={todoNoop} + onOpenApp={todoNoop} + onCloseApp={todoNoop} + onRefreshApps={onRefreshTools} + /> + setConfigModal(null)} + onSubmit={onConfigSubmit} + /> + setRemoveTarget(null)} + onConfirm={onConfirmRemove} + /> + ); } diff --git a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.stories.tsx b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.stories.tsx new file mode 100644 index 00000000..ba1d7310 --- /dev/null +++ b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.stories.tsx @@ -0,0 +1,125 @@ +import { AppShell } from "@mantine/core"; +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { expect, fn, userEvent, within } from "storybook/test"; +import { useArgs } from "storybook/preview-api"; +import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; +import { + ServerConfigModal, + type ServerConfigModalProps, +} from "./ServerConfigModal"; + +function InteractiveRender(args: ServerConfigModalProps) { + const [, updateArgs] = useArgs(); + return ( + + + { + args.onClose(); + updateArgs({ opened: false }); + }} + onSubmit={async (id, config) => { + await args.onSubmit(id, config); + updateArgs({ opened: false }); + }} + /> + + + ); +} + +const stdioConfig: MCPServerConfig = { + type: "stdio", + command: "npx", + args: ["-y", "@modelcontextprotocol/server-everything"], + env: { DEBUG: "1" }, +}; + +const sseConfig: MCPServerConfig = { + type: "sse", + url: "https://example.com/sse", + headers: { Authorization: "Bearer xxx" }, +}; + +const meta: Meta = { + title: "Groups/ServerConfigModal", + component: ServerConfigModal, + parameters: { layout: "fullscreen" }, + render: InteractiveRender, + args: { + opened: true, + existingIds: [], + onClose: fn(), + onSubmit: fn(), + }, +}; + +export default meta; +type Story = StoryObj; + +export const AddEmpty: Story = { + args: { mode: "add" }, + play: async ({ canvasElement }) => { + const body = within(canvasElement.ownerDocument.body); + await expect(await body.findByText("Add server")).toBeInTheDocument(); + const idInput = body.getByLabelText(/Server ID/i) as HTMLInputElement; + await expect(idInput).toBeInTheDocument(); + await expect(body.getByLabelText(/Command/i)).toBeInTheDocument(); + // Regression guard for the synthetic-event currentTarget bug — happy-dom + // doesn't null currentTarget after the handler returns, so unit tests + // sail past it. Real Chromium (here) does, so any future onChange that + // reads e.currentTarget inside a setState updater will throw here. + await userEvent.type(idInput, "my-server"); + await expect(idInput.value).toBe("my-server"); + const cmdInput = body.getByLabelText(/Command/i) as HTMLInputElement; + await userEvent.type(cmdInput, "node"); + await expect(cmdInput.value).toBe("node"); + }, +}; + +export const EditStdio: Story = { + args: { + mode: "edit", + initialId: "filesystem-server-default", + initialConfig: stdioConfig, + existingIds: ["other-server"], + }, + play: async ({ canvasElement }) => { + const body = within(canvasElement.ownerDocument.body); + await expect(await body.findByText("Edit server")).toBeInTheDocument(); + const idInput = body.getByLabelText(/Server ID/i) as HTMLInputElement; + await expect(idInput.value).toBe("filesystem-server-default"); + }, +}; + +export const CloneStdio: Story = { + args: { + mode: "clone", + initialId: "filesystem-server-default", + initialConfig: stdioConfig, + existingIds: ["filesystem-server-default"], + }, + play: async ({ canvasElement }) => { + const body = within(canvasElement.ownerDocument.body); + await expect(await body.findByText("Clone server")).toBeInTheDocument(); + const idInput = body.getByLabelText(/Server ID/i) as HTMLInputElement; + await expect(idInput.value).toBe(""); + const cmdInput = body.getByLabelText(/Command/i) as HTMLInputElement; + await expect(cmdInput.value).toBe("npx"); + }, +}; + +export const EditSse: Story = { + args: { + mode: "edit", + initialId: "remote", + initialConfig: sseConfig, + existingIds: [], + }, + play: async ({ canvasElement }) => { + const body = within(canvasElement.ownerDocument.body); + await expect(await body.findByLabelText(/^URL/)).toBeInTheDocument(); + await expect(body.getByLabelText(/Headers/i)).toBeInTheDocument(); + }, +}; diff --git a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx new file mode 100644 index 00000000..d07a3dac --- /dev/null +++ b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx @@ -0,0 +1,308 @@ +import { describe, it, expect, vi } from "vitest"; +import userEvent from "@testing-library/user-event"; +import { waitFor } from "@testing-library/react"; +import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; +import { renderWithMantine, screen } from "../../../test/renderWithMantine"; +import { ServerConfigModal } from "./ServerConfigModal"; + +describe("ServerConfigModal", () => { + function base(overrides: Partial<{ existingIds: string[] }> = {}) { + return { + opened: true, + mode: "add" as const, + existingIds: overrides.existingIds ?? [], + onClose: vi.fn(), + onSubmit: vi.fn().mockResolvedValue(undefined), + }; + } + + it("does not render when opened is false", () => { + renderWithMantine(); + expect(screen.queryByText("Add server")).not.toBeInTheDocument(); + }); + + it("renders the add title and stdio fields by default", () => { + renderWithMantine(); + expect(screen.getByText("Add server")).toBeInTheDocument(); + expect(screen.getByLabelText(/Server ID/i)).toBeInTheDocument(); + expect(screen.getByLabelText(/Command/i)).toBeInTheDocument(); + expect(screen.getByLabelText(/Arguments/i)).toBeInTheDocument(); + }); + + it("rejects an id containing illegal characters", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.type(screen.getByLabelText(/Server ID/i), "bad id!"); + expect( + screen.getByText(/letters, numbers, hyphens, and underscores/i), + ).toBeInTheDocument(); + }); + + it("flags duplicate ids against existingIds", async () => { + const user = userEvent.setup(); + renderWithMantine( + , + ); + await user.type(screen.getByLabelText(/Server ID/i), "alpha"); + expect(screen.getByText(/already exists/i)).toBeInTheDocument(); + }); + + it("calls onSubmit with a valid stdio config", async () => { + const user = userEvent.setup(); + const props = base(); + renderWithMantine(); + + await user.type(screen.getByLabelText(/Server ID/i), "alpha"); + await user.type(screen.getByLabelText(/Command/i), "node"); + await user.type(screen.getByLabelText(/Arguments/i), "x.js\n--port=3000"); + await user.click(screen.getByRole("button", { name: /^Add$/ })); + + await waitFor(() => expect(props.onSubmit).toHaveBeenCalledOnce()); + expect(props.onSubmit).toHaveBeenCalledWith("alpha", { + type: "stdio", + command: "node", + args: ["x.js", "--port=3000"], + }); + }); + + it("requires a command for stdio submission", async () => { + const user = userEvent.setup(); + const props = base(); + renderWithMantine(); + + await user.type(screen.getByLabelText(/Server ID/i), "alpha"); + await user.click(screen.getByRole("button", { name: /^Add$/ })); + + expect(screen.getByRole("alert")).toHaveTextContent(/Command is required/i); + expect(props.onSubmit).not.toHaveBeenCalled(); + }); + + it("rejects malformed env lines", async () => { + const user = userEvent.setup(); + const props = base(); + renderWithMantine(); + + await user.type(screen.getByLabelText(/Server ID/i), "alpha"); + await user.type(screen.getByLabelText(/Command/i), "node"); + await user.type(screen.getByLabelText(/Environment/i), "BAD_LINE"); + await user.click(screen.getByRole("button", { name: /^Add$/ })); + + expect(screen.getByRole("alert")).toHaveTextContent(/Invalid env/i); + expect(props.onSubmit).not.toHaveBeenCalled(); + }); + + // Mantine Select uses a portal-mounted dropdown that happy-dom doesn't + // open reliably; we exercise the sse rendering branch by seeding the form + // with an sse initialConfig (the same code path the Select onChange takes). + + it("renders url + headers (and hides stdio fields) when transport is sse", () => { + renderWithMantine( + , + ); + expect(screen.getByLabelText(/^URL/)).toBeInTheDocument(); + expect(screen.getByLabelText(/Headers/i)).toBeInTheDocument(); + expect(screen.queryByLabelText(/^Command/)).not.toBeInTheDocument(); + }); + + it("submits an sse config with parsed headers", async () => { + const user = userEvent.setup(); + const props = base(); + renderWithMantine( + , + ); + + await user.type(screen.getByLabelText(/^URL/), "https://x.test/sse"); + await user.type( + screen.getByLabelText(/Headers/i), + "Authorization: Bearer xxx", + ); + await user.click(screen.getByRole("button", { name: /^Save$/ })); + + await waitFor(() => expect(props.onSubmit).toHaveBeenCalledOnce()); + expect(props.onSubmit).toHaveBeenCalledWith("remote", { + type: "sse", + url: "https://x.test/sse", + headers: { Authorization: "Bearer xxx" }, + }); + }); + + it("rejects malformed header lines on sse submission", async () => { + const user = userEvent.setup(); + const props = base(); + renderWithMantine( + , + ); + await user.type(screen.getByLabelText(/Headers/i), "BAD_HEADER_LINE"); + await user.click(screen.getByRole("button", { name: /^Save$/ })); + expect(screen.getByRole("alert")).toHaveTextContent(/Invalid header/i); + expect(props.onSubmit).not.toHaveBeenCalled(); + }); + + it("submits a streamable-http config (with no headers)", async () => { + const user = userEvent.setup(); + const props = base(); + renderWithMantine( + , + ); + await user.click(screen.getByRole("button", { name: /^Save$/ })); + await waitFor(() => expect(props.onSubmit).toHaveBeenCalledOnce()); + expect(props.onSubmit).toHaveBeenCalledWith("http-srv", { + type: "streamable-http", + url: "https://x.test/mcp", + }); + }); + + it("loads initial headers + url from a streamable-http config", () => { + renderWithMantine( + , + ); + expect(screen.getByLabelText(/^URL/)).toHaveValue("https://x.test/mcp"); + expect(screen.getByLabelText(/Headers/i)).toHaveValue("X-Trace: abc"); + }); + + it("rejects an empty URL on sse submission", async () => { + const user = userEvent.setup(); + const props = base(); + renderWithMantine( + , + ); + await user.click(screen.getByRole("button", { name: /^Save$/ })); + expect(screen.getByRole("alert")).toHaveTextContent(/URL is required/i); + expect(props.onSubmit).not.toHaveBeenCalled(); + }); + + it("pre-populates fields in edit mode", () => { + const initialConfig: MCPServerConfig = { + type: "stdio", + command: "node", + args: ["server.js"], + env: { DEBUG: "1" }, + cwd: "/tmp/here", + }; + renderWithMantine( + , + ); + expect(screen.getByText("Edit server")).toBeInTheDocument(); + expect(screen.getByLabelText(/Server ID/i)).toHaveValue("alpha"); + expect(screen.getByLabelText(/Command/i)).toHaveValue("node"); + expect(screen.getByLabelText(/Arguments/i)).toHaveValue("server.js"); + expect(screen.getByLabelText(/Environment/i)).toHaveValue("DEBUG=1"); + expect(screen.getByLabelText(/Working directory/i)).toHaveValue( + "/tmp/here", + ); + }); + + it("clears the id field in clone mode (everything else carries over)", () => { + const initialConfig: MCPServerConfig = { + type: "stdio", + command: "node", + }; + renderWithMantine( + , + ); + expect(screen.getByText("Clone server")).toBeInTheDocument(); + expect(screen.getByLabelText(/Server ID/i)).toHaveValue(""); + expect(screen.getByLabelText(/Command/i)).toHaveValue("node"); + }); + + it("shows the error message and stays open when onSubmit rejects", async () => { + const user = userEvent.setup(); + const props = { + ...base(), + onSubmit: vi.fn().mockRejectedValue(new Error("server full")), + }; + renderWithMantine(); + + await user.type(screen.getByLabelText(/Server ID/i), "alpha"); + await user.type(screen.getByLabelText(/Command/i), "node"); + await user.click(screen.getByRole("button", { name: /^Add$/ })); + + await waitFor(() => { + expect(screen.getByRole("alert")).toHaveTextContent(/server full/); + }); + expect(props.onClose).not.toHaveBeenCalled(); + }); + + it("blocks submit and shows the id error message on bad-id submit click", async () => { + const user = userEvent.setup(); + const props = base({ existingIds: ["alpha"] }); + renderWithMantine(); + + await user.type(screen.getByLabelText(/Server ID/i), "alpha"); + await user.type(screen.getByLabelText(/Command/i), "node"); + await user.click(screen.getByRole("button", { name: /^Add$/ })); + + expect(screen.getByRole("alert")).toHaveTextContent(/already exists/i); + expect(props.onSubmit).not.toHaveBeenCalled(); + }); + + it("supports editing the working directory field", async () => { + const user = userEvent.setup(); + const props = base(); + renderWithMantine(); + + await user.type(screen.getByLabelText(/Server ID/i), "alpha"); + await user.type(screen.getByLabelText(/Command/i), "node"); + await user.type(screen.getByLabelText(/Working directory/i), "/tmp/cwd"); + await user.click(screen.getByRole("button", { name: /^Add$/ })); + + await waitFor(() => expect(props.onSubmit).toHaveBeenCalledOnce()); + expect(props.onSubmit).toHaveBeenCalledWith("alpha", { + type: "stdio", + command: "node", + cwd: "/tmp/cwd", + }); + }); + + it("calls onClose when Cancel is clicked", async () => { + const user = userEvent.setup(); + const props = base(); + renderWithMantine(); + await user.click(screen.getByRole("button", { name: /Cancel/ })); + expect(props.onClose).toHaveBeenCalledOnce(); + }); +}); diff --git a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx new file mode 100644 index 00000000..32be6826 --- /dev/null +++ b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx @@ -0,0 +1,438 @@ +import { useEffect, useMemo, useState } from "react"; +import { + Button, + Group, + Modal, + Select, + Stack, + Text, + TextInput, + Textarea, +} from "@mantine/core"; +import type { + MCPServerConfig, + SseServerConfig, + StdioServerConfig, + StreamableHttpServerConfig, +} from "@inspector/core/mcp/types.js"; + +/** Allowed id pattern — mirrors validateStoreId in core/storage/store-io.ts */ +const ID_PATTERN = /^[a-zA-Z0-9_-]+$/; + +export type ServerConfigModalMode = "add" | "edit" | "clone"; + +export interface ServerConfigModalProps { + opened: boolean; + mode: ServerConfigModalMode; + /** When editing, the existing id of the target server. */ + initialId?: string; + /** When editing or cloning, the existing config to pre-populate. */ + initialConfig?: MCPServerConfig; + /** Ids already in use — drives the uniqueness check (caller excludes the + * target id from this list when in 'edit' mode). */ + existingIds: string[]; + onClose: () => void; + onSubmit: (id: string, config: MCPServerConfig) => Promise | void; +} + +type TransportChoice = "stdio" | "sse" | "streamable-http"; + +interface FormState { + id: string; + transport: TransportChoice; + // stdio + command: string; + argsText: string; + envText: string; + cwd: string; + // sse / streamable-http + url: string; + headersText: string; +} + +const SectionStack = Stack.withProps({ gap: "md" }); +const FieldGrid = Stack.withProps({ gap: "sm" }); +const Actions = Group.withProps({ justify: "flex-end", gap: "sm", mt: "md" }); + +const MODE_TITLES: Record = { + add: "Add server", + edit: "Edit server", + clone: "Clone server", +}; + +function configToFormState( + initialId: string | undefined, + initialConfig: MCPServerConfig | undefined, + mode: ServerConfigModalMode, +): FormState { + const id = mode === "edit" ? (initialId ?? "") : ""; + const transport: TransportChoice = + initialConfig?.type === undefined ? "stdio" : initialConfig.type; + if (!initialConfig) { + return { + id, + transport: "stdio", + command: "", + argsText: "", + envText: "", + cwd: "", + url: "", + headersText: "", + }; + } + if (transport === "stdio") { + const c = initialConfig as StdioServerConfig; + return { + id, + transport, + command: c.command ?? "", + argsText: (c.args ?? []).join("\n"), + envText: Object.entries(c.env ?? {}) + .map(([k, v]) => `${k}=${v}`) + .join("\n"), + cwd: c.cwd ?? "", + url: "", + headersText: "", + }; + } + // sse / streamable-http + const c = initialConfig as SseServerConfig | StreamableHttpServerConfig; + return { + id, + transport, + command: "", + argsText: "", + envText: "", + cwd: "", + url: c.url ?? "", + headersText: Object.entries(c.headers ?? {}) + .map(([k, v]) => `${k}: ${v}`) + .join("\n"), + }; +} + +function parseArgs(raw: string): string[] { + return raw + .split("\n") + .map((s) => s.trim()) + .filter((s) => s.length > 0); +} + +function parseEnv(raw: string): + | { + ok: true; + value: Record; + } + | { + ok: false; + error: string; + } { + const out: Record = {}; + const lines = raw + .split("\n") + .map((s) => s.trim()) + .filter((s) => s.length > 0); + for (const line of lines) { + const eq = line.indexOf("="); + if (eq <= 0) { + return { ok: false, error: `Invalid env line "${line}". Use KEY=VALUE.` }; + } + const key = line.slice(0, eq).trim(); + // env values preserve trailing whitespace — they're shell-style strings + // where spaces / tabs can be load-bearing; header values are HTTP-style + // and parseHeaders below trims them per RFC 7230 §3.2.4. + const value = line.slice(eq + 1); + if (!key) + return { ok: false, error: `Invalid env line "${line}". Empty key.` }; + out[key] = value; + } + return { ok: true, value: out }; +} + +function parseHeaders(raw: string): + | { + ok: true; + value: Record; + } + | { + ok: false; + error: string; + } { + const out: Record = {}; + const lines = raw + .split("\n") + .map((s) => s.trim()) + .filter((s) => s.length > 0); + for (const line of lines) { + const colon = line.indexOf(":"); + if (colon <= 0) { + return { + ok: false, + error: `Invalid header line "${line}". Use "Header-Name: value".`, + }; + } + const key = line.slice(0, colon).trim(); + const value = line.slice(colon + 1).trim(); + if (!key) { + return { ok: false, error: `Invalid header line "${line}". Empty name.` }; + } + out[key] = value; + } + return { ok: true, value: out }; +} + +export function ServerConfigModal({ + opened, + mode, + initialId, + initialConfig, + existingIds, + onClose, + onSubmit, +}: ServerConfigModalProps) { + const initial = useMemo( + () => configToFormState(initialId, initialConfig, mode), + [initialId, initialConfig, mode], + ); + const [form, setForm] = useState(initial); + const [submitError, setSubmitError] = useState(undefined); + const [submitting, setSubmitting] = useState(false); + + // Reset form whenever the modal opens with new inputs. + useEffect(() => { + if (opened) { + setForm(initial); + setSubmitError(undefined); + setSubmitting(false); + } + }, [opened, initial]); + + const trimmedId = form.id.trim(); + const idIsValid = ID_PATTERN.test(trimmedId); + const idIsDuplicate = trimmedId.length > 0 && existingIds.includes(trimmedId); + const idError = !trimmedId + ? undefined + : !idIsValid + ? "Use only letters, numbers, hyphens, and underscores." + : idIsDuplicate + ? "A server with this id already exists." + : undefined; + + function buildConfig(): + | { + ok: true; + config: MCPServerConfig; + } + | { + ok: false; + error: string; + } { + if (form.transport === "stdio") { + if (!form.command.trim()) { + return { ok: false, error: "Command is required for stdio." }; + } + const env = parseEnv(form.envText); + if (!env.ok) return env; + const config: StdioServerConfig = { + type: "stdio", + command: form.command.trim(), + }; + const args = parseArgs(form.argsText); + if (args.length > 0) config.args = args; + if (Object.keys(env.value).length > 0) config.env = env.value; + const cwd = form.cwd.trim(); + if (cwd) config.cwd = cwd; + return { ok: true, config }; + } + if (!form.url.trim()) { + return { ok: false, error: "URL is required for sse / streamable-http." }; + } + const headers = parseHeaders(form.headersText); + if (!headers.ok) return headers; + const base = { url: form.url.trim() }; + const config: SseServerConfig | StreamableHttpServerConfig = + form.transport === "sse" + ? { type: "sse", ...base } + : { type: "streamable-http", ...base }; + if (Object.keys(headers.value).length > 0) config.headers = headers.value; + return { ok: true, config }; + } + + async function handleSubmit() { + setSubmitError(undefined); + if (!trimmedId) { + setSubmitError("Server id is required."); + return; + } + if (idError) { + setSubmitError(idError); + return; + } + const built = buildConfig(); + if (!built.ok) { + setSubmitError(built.error); + return; + } + setSubmitting(true); + try { + await onSubmit(trimmedId, built.config); + onClose(); + } catch (err) { + setSubmitError(err instanceof Error ? err.message : String(err)); + } finally { + setSubmitting(false); + } + } + + const isStdio = form.transport === "stdio"; + + return ( + + + + { + // Capture before the functional updater closes over it — React + // nulls SyntheticEvent.currentTarget after the synchronous + // handler returns, so reading inside `setForm((f) => ...)` + // throws "Cannot read properties of null". + const next = e.currentTarget.value; + setForm((f) => ({ ...f, id: next })); + }} + error={idError} + data-autofocus + required + disabled={submitting} + /> + +