From 9aab94596438c4da83f801d024f622e007a954f9 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 20:57:02 -0400 Subject: [PATCH] feat(web): custom headers on the manual server-config form (#1915) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding a server by hand offered no way to set custom HTTP headers, so the only route to a cookie or a routing header was importing a config file. The manual "Add server" form now carries a Custom Headers key/value editor for the sse / streamable-http transports, pre-populated when editing or cloning and submitted alongside the config. Headers are not part of `MCPServerConfig` — they live on the entry's `settings` — so they travel as `onSubmit`'s third argument rather than folded into the config, and `addServer` / `updateServer` gained an optional `settings` parameter to carry them to the backend (`POST /api/servers` already accepted one). `updateServer`'s omission semantics are unchanged: with no settings passed, the route still preserves the node on disk. `KeyValueRows` moves out of ServerSettingsForm into a shared element so the two editors cannot drift, and each row's controls gain a row-scoped `aria-label` ("header value, Cookie", "Remove header, Cookie") — previously every remove button announced only "X", indistinguishable across rows. Signed-off-by: cliffhall --- clients/web/src/App.tsx | 61 ++++-- .../KeyValueRows/KeyValueRows.stories.tsx | 60 ++++++ .../KeyValueRows/KeyValueRows.test.tsx | 87 +++++++++ .../elements/KeyValueRows/KeyValueRows.tsx | 91 +++++++++ .../ServerConfigModal.stories.tsx | 40 +++- .../ServerConfigModal.test.tsx | 180 ++++++++++++++++-- .../ServerConfigModal/ServerConfigModal.tsx | 128 ++++++++++--- .../ServerSettingsForm.test.tsx | 20 +- .../ServerSettingsForm/ServerSettingsForm.tsx | 51 +---- .../ServerSettingsModal.test.tsx | 19 +- .../src/test/core/react/useServers.test.tsx | 87 +++++++++ core/react/useServers.ts | 44 ++++- 12 files changed, 732 insertions(+), 136 deletions(-) create mode 100644 clients/web/src/components/elements/KeyValueRows/KeyValueRows.stories.tsx create mode 100644 clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx create mode 100644 clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 72d8bd1f..e3d2557a 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -138,6 +138,7 @@ import { ServerConfigModal, type ServerConfigModalMode, } from "./components/groups/ServerConfigModal/ServerConfigModal"; +import type { KeyValuePair } from "./components/elements/KeyValueRows/KeyValueRows"; import { ServerSettingsModal } from "./components/groups/ServerSettingsModal/ServerSettingsModal"; import { ClientSettingsModal } from "./components/groups/ClientSettingsModal/ClientSettingsModal"; import { @@ -3895,30 +3896,17 @@ function App() { // each id), deduped. The batch is reset to empty when an add/import modal // opens (see the menu handlers). const addServerHighlighted = useCallback( - async (id: string, config: MCPServerConfig) => { - await addServer(id, config); + async ( + id: string, + config: MCPServerConfig, + settings?: InspectorServerSettings, + ) => { + await addServer(id, config, settings); setHighlightedServerIds((ids) => (ids.includes(id) ? ids : [...ids, id])); }, [addServer], ); - // 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 addServerHighlighted(id, config); - }, - [configModal, addServerHighlighted, 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. @@ -3935,6 +3923,40 @@ function App() { return servers.find((s) => s.id === configModal.targetId); }, [configModal, servers]); + // On rename of the active server, keep activeServerId pointed at the new id. + const onConfigSubmit = useCallback( + async (id: string, config: MCPServerConfig, headers: KeyValuePair[]) => { + // Headers live on the entry's `settings`, not on the transport config, + // so they're folded back in here (#1915). Send `settings` only when it + // would change something: with no headers either way there is nothing to + // write, and omitting the key is what makes the backend preserve the + // rest of the settings node. + const existing = configModalTarget?.settings; + const settingsChanged = + headers.length > 0 || (existing?.headers.length ?? 0) > 0; + const settings = settingsChanged + ? { ...(existing ?? EMPTY_SETTINGS), headers } + : undefined; + if (configModal?.mode === "edit" && configModal.targetId) { + const originalId = configModal.targetId; + await updateServer(originalId, id, config, settings); + if (originalId === activeServerId && id !== originalId) { + setActiveServerId(id); + } + return; + } + // add or clone + await addServerHighlighted(id, config, settings); + }, + [ + configModal, + configModalTarget, + addServerHighlighted, + updateServer, + activeServerId, + ], + ); + const settingsModalTarget = useMemo(() => { if (!settingsModalTargetId) return undefined; return servers.find((s) => s.id === settingsModalTargetId); @@ -4550,6 +4572,7 @@ function App() { mode={configModal?.mode ?? "add"} initialId={configModalTarget?.id} initialConfig={configModalTarget?.config} + initialHeaders={configModalTarget?.settings?.headers} existingIds={existingIds} onClose={() => setConfigModal(null)} onSubmit={onConfigSubmit} diff --git a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.stories.tsx b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.stories.tsx new file mode 100644 index 00000000..05c5ca14 --- /dev/null +++ b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.stories.tsx @@ -0,0 +1,60 @@ +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { expect, fn, userEvent, within } from "storybook/test"; +import { KeyValueRows } from "./KeyValueRows"; + +const meta: Meta = { + title: "Elements/KeyValueRows", + component: KeyValueRows, + args: { + entityLabel: "header", + onChange: fn(), + onRemove: fn(), + }, +}; + +export default meta; +type Story = StoryObj; + +// Each row's controls carry a row-scoped accessible name, so a screen reader +// can tell one row's key box from another's. +export const Populated: Story = { + args: { + items: [ + { key: "Cookie", value: "branch=feature-x" }, + { key: "X-Env", value: "dev" }, + ], + }, + play: async ({ args, canvasElement }) => { + const canvas = within(canvasElement); + const value = await canvas.findByRole("textbox", { + name: "header value, Cookie", + }); + await expect(value).toHaveValue("branch=feature-x"); + + await userEvent.click( + canvas.getByRole("button", { name: "Remove header, X-Env" }), + ); + await expect(args.onRemove).toHaveBeenCalledWith(1); + }, +}; + +// A row whose key is still blank falls back to a positional name rather than +// announcing nothing. +export const BlankKeyRow: Story = { + args: { items: [{ key: "", value: "" }] }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await expect( + await canvas.findByRole("textbox", { name: "header name, row 1" }), + ).toBeInTheDocument(); + }, +}; + +// An empty list renders nothing — callers draw their own empty-state hint. +export const Empty: Story = { + args: { items: [] }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await expect(canvas.queryAllByRole("textbox")).toHaveLength(0); + }, +}; diff --git a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx new file mode 100644 index 00000000..ef96bb8d --- /dev/null +++ b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx @@ -0,0 +1,87 @@ +import { describe, it, expect, vi } from "vitest"; +import userEvent from "@testing-library/user-event"; +import { renderWithMantine, screen } from "../../../test/renderWithMantine"; +import { KeyValueRows } from "./KeyValueRows"; + +describe("KeyValueRows", () => { + function setup(items: { key: string; value: string }[]) { + const onChange = vi.fn(); + const onRemove = vi.fn(); + renderWithMantine( + , + ); + return { onChange, onRemove }; + } + + it("renders nothing for an empty list", () => { + setup([]); + expect(screen.queryAllByRole("textbox")).toHaveLength(0); + }); + + it("names each control by entity and row so rows are distinguishable", () => { + setup([ + { key: "Cookie", value: "a=1" }, + { key: "", value: "" }, + ]); + expect( + screen.getByRole("textbox", { name: "header name, Cookie" }), + ).toHaveValue("Cookie"); + expect( + screen.getByRole("textbox", { name: "header value, Cookie" }), + ).toHaveValue("a=1"); + // A key that is blank (or only whitespace) falls back to its position. + expect( + screen.getByRole("textbox", { name: "header name, row 2" }), + ).toBeInTheDocument(); + }); + + it("reports key and value edits with the row index", async () => { + const user = userEvent.setup({ delay: null }); + const { onChange } = setup([{ key: "X", value: "1" }]); + + await user.type( + screen.getByRole("textbox", { name: "header name, X" }), + "Y", + ); + expect(onChange).toHaveBeenLastCalledWith(0, "XY", "1"); + + await user.type( + screen.getByRole("textbox", { name: "header value, X" }), + "2", + ); + expect(onChange).toHaveBeenLastCalledWith(0, "X", "12"); + }); + + it("clears a key or a value through its Clear button", async () => { + const user = userEvent.setup({ delay: null }); + const { onChange } = setup([{ key: "X", value: "1" }]); + + const clearButtons = screen.getAllByRole("button", { name: "Clear" }); + await user.click(clearButtons[0]!); + expect(onChange).toHaveBeenLastCalledWith(0, "", "1"); + + await user.click(clearButtons[1]!); + expect(onChange).toHaveBeenLastCalledWith(0, "X", ""); + }); + + it("omits the Clear button for an empty key or value", () => { + setup([{ key: "", value: "" }]); + expect(screen.queryAllByRole("button", { name: "Clear" })).toHaveLength(0); + }); + + it("removes the clicked row", async () => { + const user = userEvent.setup({ delay: null }); + const { onRemove } = setup([ + { key: "A", value: "1" }, + { key: "B", value: "2" }, + ]); + + await user.click(screen.getByRole("button", { name: "Remove header, B" })); + expect(onRemove).toHaveBeenCalledWith(1); + }); +}); diff --git a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx new file mode 100644 index 00000000..c41bd9c3 --- /dev/null +++ b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx @@ -0,0 +1,91 @@ +import { ActionIcon, Group, TextInput } from "@mantine/core"; +import { ClearButton } from "../ClearButton/ClearButton"; + +export interface KeyValuePair { + key: string; + value: string; +} + +export interface KeyValueRowsProps { + items: KeyValuePair[]; + /** + * Singular noun for one row ("header", "environment variable", …). Used only + * to build each control's `aria-label`: the section heading and the "Key" / + * "Value" placeholders are not programmatically associated with the inputs, + * so without it an assistive technology cannot tell one list's key box from + * another's, and the remove button announces only "X". + */ + entityLabel: string; + onChange: (index: number, key: string, value: string) => void; + onRemove: (index: number) => void; +} + +// Optional (non-required) clearable field — keeps the ClearButton clickable. +const ClearableTextInput = TextInput.withProps({ + rightSectionPointerEvents: "auto", +}); + +const RemoveIcon = ActionIcon.withProps({ + color: "red", + variant: "subtle", +}); + +/** + * Controlled editor for a list of `{ key, value }` pairs — the shape the + * Inspector persists for custom headers, request metadata, and stdio + * environment variables. Owns no state: every keystroke is reported through + * `onChange(index, key, value)` and the caller re-renders with the new list. + * + * Shared by ServerSettingsForm (headers / metadata / env) and ServerConfigModal + * (headers on the manual add form, #1915) so the two cannot drift. + */ +export function KeyValueRows({ + items, + entityLabel, + onChange, + onRemove, +}: KeyValueRowsProps) { + return ( + <> + {items.map((item, index) => { + const rowLabel = item.key.trim() || `row ${index + 1}`; + return ( + + + onChange(index, e.currentTarget.value, item.value) + } + rightSection={ + item.key ? ( + onChange(index, "", item.value)} + /> + ) : null + } + /> + onChange(index, item.key, e.currentTarget.value)} + rightSection={ + item.value ? ( + onChange(index, item.key, "")} /> + ) : null + } + /> + onRemove(index)} + > + X + + + ); + })} + + ); +} diff --git a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.stories.tsx b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.stories.tsx index c180a95b..0b990f43 100644 --- a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.stories.tsx +++ b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.stories.tsx @@ -19,8 +19,8 @@ function InteractiveRender(args: ServerConfigModalProps) { args.onClose(); updateArgs({ opened: false }); }} - onSubmit={async (id, config) => { - await args.onSubmit(id, config); + onSubmit={async (id, config, headers) => { + await args.onSubmit(id, config, headers); updateArgs({ opened: false }); }} /> @@ -134,7 +134,39 @@ export const EditSse: Story = { const body = within(canvasElement.ownerDocument.body); const dialog = within(await findDialog(body, "Edit server")); await expect(await dialog.findByLabelText(/^URL/)).toBeInTheDocument(); - // Headers are no longer entered here — they live in ServerSettingsForm. - await expect(dialog.queryByLabelText(/Headers/i)).toBeNull(); + // The headers editor starts collapsed to a bare "+ Add Header" control + // when the server has none stored. + await expect( + dialog.getByRole("button", { name: "+ Add Header" }), + ).toBeInTheDocument(); + await expect( + dialog.queryAllByRole("textbox", { name: /header name/ }), + ).toHaveLength(0); + }, +}; + +// Custom headers on the manual form (#1915) — the reason the field exists is a +// cookie routing requests to one developer's branch on a shared dev server. +export const EditSseWithHeaders: Story = { + args: { + mode: "edit", + initialId: "remote", + initialConfig: sseConfig, + initialHeaders: [{ key: "Cookie", value: "branch=feature-x" }], + existingIds: [], + }, + play: async ({ canvasElement }) => { + const body = within(canvasElement.ownerDocument.body); + const dialog = within(await findDialog(body, "Edit server")); + const value = (await dialog.findByRole("textbox", { + name: "header value, Cookie", + })) as HTMLInputElement; + await expect(value.value).toBe("branch=feature-x"); + + // Adding a row appends an empty pair, named by position until it is keyed. + await userEvent.click(dialog.getByRole("button", { name: "+ Add Header" })); + await expect( + await dialog.findByRole("textbox", { name: "header name, row 2" }), + ).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx index 2f6f4e40..9bd373ef 100644 --- a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx +++ b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx @@ -66,11 +66,11 @@ describe("ServerConfigModal", () => { 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"], - }); + expect(props.onSubmit).toHaveBeenCalledWith( + "alpha", + { type: "stdio", command: "node", args: ["x.js", "--port=3000"] }, + [], + ); }); it("requires a command for stdio submission", async () => { @@ -117,7 +117,7 @@ describe("ServerConfigModal", () => { expect(screen.queryByLabelText(/^Command/)).not.toBeInTheDocument(); }); - it("submits an sse config with just the url (headers move to the settings form)", async () => { + it("submits an sse config with just the url and no headers", async () => { const user = userEvent.setup({ delay: null }); const props = base(); renderWithMantine( @@ -133,10 +133,11 @@ describe("ServerConfigModal", () => { 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", - }); + expect(props.onSubmit).toHaveBeenCalledWith( + "remote", + { type: "sse", url: "https://x.test/sse" }, + [], + ); }); it("submits a streamable-http config", async () => { @@ -152,10 +153,11 @@ describe("ServerConfigModal", () => { ); 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", - }); + expect(props.onSubmit).toHaveBeenCalledWith( + "http-srv", + { type: "streamable-http", url: "https://x.test/mcp" }, + [], + ); }); it("loads the url from a streamable-http config", () => { @@ -275,11 +277,11 @@ describe("ServerConfigModal", () => { 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", - }); + expect(props.onSubmit).toHaveBeenCalledWith( + "alpha", + { type: "stdio", command: "node", cwd: "/tmp/cwd" }, + [], + ); }); it("clears the Server ID field via its Clear button", async () => { @@ -470,4 +472,144 @@ describe("ServerConfigModal", () => { await user.click(screen.getByRole("button", { name: /Cancel/ })); expect(props.onClose).toHaveBeenCalledOnce(); }); + + describe("custom headers (#1915)", () => { + it("hides the headers section for stdio", () => { + renderWithMantine(); + expect(screen.queryByText("Custom headers")).not.toBeInTheDocument(); + }); + + // Every keystroke re-renders the whole modal, and this case types four + // fields, so it runs several times slower under the coverage project's v8 + // instrumentation than in a plain unit run — enough to trip the 5s default. + // The extra ceiling is headroom for the instrumented run, not a hang guard. + const TYPING_HEAVY_TIMEOUT_MS = 20000; + + it( + "submits headers added on the manual add form", + async () => { + const user = userEvent.setup({ delay: null }); + const props = base(); + // Seeded on the http transport rather than driven through the + // Transport select — that select has its own test above, and walking + // its combobox here only added to the cost described above. + renderWithMantine( + , + ); + + await user.type(screen.getByLabelText(/Server ID/i), "remote"); + await user.type(screen.getByLabelText(/^URL/), "https://x.test/mcp"); + + await user.click(screen.getByRole("button", { name: "+ Add Header" })); + await user.type( + screen.getByRole("textbox", { name: /header name, row 1/ }), + "Cookie", + ); + await user.type( + screen.getByRole("textbox", { name: /header value, Cookie/ }), + "branch=feature-x", + ); + await user.click(screen.getByRole("button", { name: /^Add$/ })); + + await waitFor(() => expect(props.onSubmit).toHaveBeenCalledOnce()); + expect(props.onSubmit).toHaveBeenCalledWith( + "remote", + { type: "streamable-http", url: "https://x.test/mcp" }, + [{ key: "Cookie", value: "branch=feature-x" }], + ); + }, + TYPING_HEAVY_TIMEOUT_MS, + ); + + it("pre-populates the rows from initialHeaders and submits an edit", async () => { + const user = userEvent.setup({ delay: null }); + const props = base(); + renderWithMantine( + , + ); + + const valueInput = screen.getByRole("textbox", { + name: /header value, X-Env/, + }); + expect(valueInput).toHaveValue("dev"); + await user.type(valueInput, "-2"); + 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" }, + [{ key: "X-Env", value: "dev-2" }], + ); + }); + + it("drops blank-key rows and removes a row on X", async () => { + const user = userEvent.setup({ delay: null }); + const props = base(); + renderWithMantine( + , + ); + + // An empty row is the form's "still typing" placeholder — it must not + // reach the caller. + await user.click(screen.getByRole("button", { name: "+ Add Header" })); + // Editing one row of several must leave its siblings untouched. + await user.type( + screen.getByRole("textbox", { name: "header value, Keep" }), + "9", + ); + await user.click( + screen.getByRole("button", { name: "Remove header, Drop" }), + ); + 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" }, + [{ key: "Keep", value: "19" }], + ); + }); + + it("submits no headers for stdio even when initialHeaders is set", async () => { + const user = userEvent.setup({ delay: null }); + const props = base(); + renderWithMantine( + , + ); + + await user.click(screen.getByRole("button", { name: /^Save$/ })); + + await waitFor(() => expect(props.onSubmit).toHaveBeenCalledOnce()); + expect(props.onSubmit).toHaveBeenCalledWith( + "local", + { type: "stdio", command: "node" }, + [], + ); + }); + }); }); diff --git a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx index e1e823a6..24089ff5 100644 --- a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx +++ b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx @@ -10,6 +10,10 @@ import { Textarea, } from "@mantine/core"; import { ClearButton } from "../../elements/ClearButton/ClearButton"; +import { + KeyValueRows, + type KeyValuePair, +} from "../../elements/KeyValueRows/KeyValueRows"; import { useValueChange } from "../../../hooks/useValueChange"; import type { MCPServerConfig, @@ -28,11 +32,27 @@ export interface ServerConfigModalProps { initialId?: string; /** When editing or cloning, the existing config to pre-populate. */ initialConfig?: MCPServerConfig; + /** + * Custom HTTP headers already stored for the target server, pre-populated + * when editing or cloning. Headers are not part of `MCPServerConfig` — they + * live on the entry's `settings`, so they arrive (and leave, via `onSubmit`) + * as their own value rather than folded into the config. (#1915) + */ + initialHeaders?: KeyValuePair[]; /** 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; + /** + * `headers` carries the edited custom-header rows (blank-key rows already + * dropped). It is always `[]` for stdio, which has no HTTP request to attach + * headers to. + */ + onSubmit: ( + id: string, + config: MCPServerConfig, + headers: KeyValuePair[], + ) => Promise | void; } type TransportChoice = "stdio" | "sse" | "streamable-http"; @@ -47,6 +67,8 @@ interface FormState { cwd: string; // sse / streamable-http url: string; + /** Custom HTTP headers — sse / streamable-http only. (#1915) */ + headers: KeyValuePair[]; } // The `string`-valued FormState keys, which all share the same text-input @@ -84,6 +106,10 @@ const EnvTextarea = Textarea.withProps({ minRows: 2, rightSectionPointerEvents: "auto", }); +const AddHeaderButton = Button.withProps({ size: "xs", variant: "light" }); +const HeadersHeader = Group.withProps({ justify: "space-between", gap: "sm" }); +const HeadersLabel = Text.withProps({ size: "sm", fw: 500 }); +const HeadersHint = Text.withProps({ size: "xs", c: "dimmed" }); const MODE_TITLES: Record = { add: "Add server", @@ -94,9 +120,14 @@ const MODE_TITLES: Record = { function configToFormState( initialId: string | undefined, initialConfig: MCPServerConfig | undefined, + initialHeaders: KeyValuePair[] | undefined, mode: ServerConfigModalMode, ): FormState { const id = mode === "edit" ? (initialId ?? "") : ""; + // Copy the rows rather than aliasing the caller's array — the form mutates + // this list as its own state, and a clone keeps a cancelled edit from + // reaching the entry the caller passed in. + const headers = (initialHeaders ?? []).map((h) => ({ ...h })); const transport: TransportChoice = initialConfig?.type === undefined ? "stdio" : initialConfig.type; if (!initialConfig) { @@ -108,6 +139,7 @@ function configToFormState( envText: "", cwd: "", url: "", + headers, }; } if (transport === "stdio") { @@ -122,9 +154,9 @@ function configToFormState( .join("\n"), cwd: c.cwd ?? "", url: "", + headers, }; } - // sse / streamable-http — custom headers live in ServerSettingsForm now. const url = initialConfig.type === "sse" || initialConfig.type === "streamable-http" ? initialConfig.url @@ -137,6 +169,7 @@ function configToFormState( envText: "", cwd: "", url: url ?? "", + headers, }; } @@ -185,13 +218,14 @@ export function ServerConfigModal({ mode, initialId, initialConfig, + initialHeaders, existingIds, onClose, onSubmit, }: ServerConfigModalProps) { const initial = useMemo( - () => configToFormState(initialId, initialConfig, mode), - [initialId, initialConfig, mode], + () => configToFormState(initialId, initialConfig, initialHeaders, mode), + [initialId, initialConfig, initialHeaders, mode], ); const [form, setForm] = useState(initial); const [submitError, setSubmitError] = useState(undefined); @@ -210,6 +244,22 @@ export function ServerConfigModal({ const clearTextField = (field: TextField) => () => setForm((f) => ({ ...f, [field]: "" })); + // Header rows are a pair array rather than a plain string, so they get their + // own handlers instead of riding `setTextField` (which `TextField` excludes + // by type). + const addHeader = () => + setForm((f) => ({ ...f, headers: [...f.headers, { key: "", value: "" }] })); + const removeHeader = (index: number) => + setForm((f) => ({ + ...f, + headers: f.headers.filter((_, i) => i !== index), + })); + const changeHeader = (index: number, key: string, value: string) => + setForm((f) => ({ + ...f, + headers: f.headers.map((h, i) => (i === index ? { key, value } : h)), + })); + // Reset the form whenever the modal opens, or whenever `initial` changes // while it is open. Keying on `opened ? initial : undefined` collapses both // triggers into one value: it flips to `initial` on open, tracks `initial` @@ -263,9 +313,9 @@ export function ServerConfigModal({ return { ok: false, error: "URL is required for sse / streamable-http." }; } const base = { url: form.url.trim() }; - // Custom headers live in ServerSettingsForm now (persisted under - // settings.headers on the entry); the SSE / streamable-http config here - // only carries the canonical transport fields. + // Custom headers are persisted under `settings.headers` on the entry, not + // on the config — they leave through `onSubmit`'s third argument, so the + // SSE / streamable-http config here carries only transport fields. const config: MCPServerConfig = form.transport === "sse" ? { type: "sse", ...base } @@ -288,9 +338,19 @@ export function ServerConfigModal({ setSubmitError(built.error); return; } + // Blank-key rows are the form's placeholder for "row being typed" and mean + // nothing on the wire — the persist layer drops them anyway, so drop them + // here too and keep the caller's payload honest. stdio has no HTTP request + // to carry headers, so it always submits none. + const headers = + form.transport === "stdio" + ? [] + : form.headers + .map((h) => ({ key: h.key.trim(), value: h.value })) + .filter((h) => h.key.length > 0); setSubmitting(true); try { - await onSubmit(trimmedId, built.config); + await onSubmit(trimmedId, built.config, headers); onClose(); } catch (err) { setSubmitError(err instanceof Error ? err.message : String(err)); @@ -412,21 +472,43 @@ export function ServerConfigModal({ /> ) : ( - - ) : null - } - /> + <> + + ) : null + } + /> + + + Custom headers + + + Add Header + + + + Sent with every HTTP request to this server — cookies + included. If OAuth is configured later, the `Authorization` + header is owned by the OAuth flow and any value set here is + ignored. + + + + )} diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx index d238228a..daf31021 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx @@ -329,8 +329,9 @@ describe("ServerSettingsForm", () => { expandedSections={["headers"]} />, ); - const removeButtons = screen.getAllByRole("button", { name: "X" }); - await user.click(removeButtons[0]); + await user.click( + screen.getByRole("button", { name: "Remove header, Authorization" }), + ); expect(onRemoveHeader).toHaveBeenCalledWith(0); }); @@ -351,8 +352,9 @@ describe("ServerSettingsForm", () => { await user.type(valueInput, "Z"); expect(onMetadataChange).toHaveBeenCalled(); - const removeButtons = screen.getAllByRole("button", { name: "X" }); - await user.click(removeButtons[0]); + await user.click( + screen.getByRole("button", { name: "Remove metadata entry, userId" }), + ); expect(onRemoveMetadata).toHaveBeenCalledWith(0); }); @@ -676,9 +678,13 @@ describe("ServerSettingsForm", () => { await user.type(keyInput, "2"); expect(onEnvChange).toHaveBeenLastCalledWith(0, "API_KEY2", "secret"); - // The remove ("X") button sits alongside the row's key/value inputs. - const removeButtons = screen.getAllByRole("button", { name: "X" }); - await user.click(removeButtons[removeButtons.length - 1]!); + // The remove button sits alongside the row's key/value inputs; its + // accessible name identifies which row it belongs to. + await user.click( + screen.getByRole("button", { + name: "Remove environment variable, API_KEY", + }), + ); expect(onRemoveEnv).toHaveBeenCalledWith(0); }); }); diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx index 276c9cf9..22410d5e 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx @@ -13,6 +13,7 @@ import { TextInput, } from "@mantine/core"; import { ClearButton } from "../../elements/ClearButton/ClearButton"; +import { KeyValueRows } from "../../elements/KeyValueRows/KeyValueRows"; import type { ProtocolEra } from "@modelcontextprotocol/client"; import type { InspectorServerSettings, @@ -223,53 +224,6 @@ const ClearStoredOAuthHint = Text.withProps({ miw: "12rem", }); -function KeyValueRows({ - items, - onChange, - onRemove, -}: { - items: { key: string; value: string }[]; - onChange: (index: number, key: string, value: string) => void; - onRemove: (index: number) => void; -}) { - /* v8 ignore next 3 -- unreachable: every caller guards with `length === 0` - and renders an EmptyHint instead, so KeyValueRows is only mounted with - a non-empty list. */ - if (items.length === 0) { - return null; - } - - return ( - <> - {items.map((item, index) => ( - - onChange(index, e.currentTarget.value, item.value)} - rightSection={ - item.key ? ( - onChange(index, "", item.value)} /> - ) : null - } - /> - onChange(index, item.key, e.currentTarget.value)} - rightSection={ - item.value ? ( - onChange(index, item.key, "")} /> - ) : null - } - /> - onRemove(index)}>X - - ))} - - ); -} - // Reserved-key rejection is inline per row (#2018): the reason is passed as the // key input's `error` **string**, so Mantine renders it in the input's own error // slot and wires the `aria-describedby` association — a bare boolean would mark @@ -641,6 +595,7 @@ export function ServerSettingsForm({ ) : ( @@ -667,6 +622,7 @@ export function ServerSettingsForm({ ) : ( @@ -690,6 +646,7 @@ export function ServerSettingsForm({ ) : ( diff --git a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx index a6faac5e..8c29c418 100644 --- a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx @@ -276,8 +276,9 @@ describe("ServerSettingsModal", () => { />, ); await user.click(screen.getByRole("button", { name: "Custom Headers" })); - const removeButtons = screen.getAllByRole("button", { name: "X" }); - await user.click(removeButtons[0]); + await user.click( + screen.getByRole("button", { name: "Remove header, Authorization" }), + ); expect(onSettingsChange).toHaveBeenCalledWith({ ...initialSettings, headers: [], @@ -342,10 +343,11 @@ describe("ServerSettingsModal", () => { />, ); await user.click(screen.getByRole("button", { name: "Request Metadata" })); - const removeButtons = screen.getAllByRole("button", { name: "X" }); - // After expanding metadata, both header and metadata X buttons exist; - // the metadata X is the last one. - await user.click(removeButtons[removeButtons.length - 1]); + // Both the header and metadata rows have a remove button; each is named + // for the row it belongs to, so no positional guess is needed. + await user.click( + screen.getByRole("button", { name: "Remove metadata entry, userId" }), + ); expect(onSettingsChange).toHaveBeenCalledWith({ ...initialSettings, metadata: [], @@ -764,8 +766,9 @@ describe("ServerSettingsModal", () => { await user.click( screen.getByRole("button", { name: "Environment Variables" }), ); - const removeButtons = screen.getAllByRole("button", { name: "X" }); - await user.click(removeButtons[removeButtons.length - 1]); + await user.click( + screen.getByRole("button", { name: "Remove environment variable, A" }), + ); expect(onSettingsChange).toHaveBeenCalledWith({ ...emptySettings, env: [], diff --git a/clients/web/src/test/core/react/useServers.test.tsx b/clients/web/src/test/core/react/useServers.test.tsx index bb95402d..49b423f2 100644 --- a/clients/web/src/test/core/react/useServers.test.tsx +++ b/clients/web/src/test/core/react/useServers.test.tsx @@ -123,6 +123,93 @@ describe("useServers", () => { }); }); + it("addServer persists a settings node when one is supplied (#1915)", async () => { + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn: h.fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + + await act(async () => { + // The manual add form carries custom headers here — they live on + // `settings`, not on the transport config. + await result.current.addServer( + "remote", + { type: "streamable-http", url: "https://x.test/mcp" }, + { + headers: [{ key: "Cookie", value: "branch=feature-x" }], + env: [], + metadata: [], + connectionTimeout: 0, + requestTimeout: 0, + taskTtl: 60000, + maxFetchRequests: 1000, + roots: [], + }, + ); + }); + + await waitFor(() => { + const added = result.current.servers.find((srv) => srv.id === "remote"); + expect(added?.settings?.headers).toEqual([ + { key: "Cookie", value: "branch=feature-x" }, + ]); + }); + const stored = readConfig(h.configPath).mcpServers + .remote as unknown as Record; + expect(stored.headers).toEqual({ Cookie: "branch=feature-x" }); + }); + + it("updateServer replaces the settings node when one is supplied (#1915)", async () => { + writeFileSync( + h.configPath, + JSON.stringify({ + mcpServers: { + alpha: { + type: "streamable-http", + url: "https://x.test/mcp", + headers: { "X-Old": "1" }, + metadata: [{ key: "trace", value: "abc" }], + }, + }, + }), + ); + + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn: h.fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + + await act(async () => { + // Editing headers in ServerConfigModal sends the whole settings node, + // so the caller is responsible for carrying the untouched fields + // (here: metadata) forward. + await result.current.updateServer( + "alpha", + "alpha", + { type: "streamable-http", url: "https://x.test/mcp" }, + { + headers: [{ key: "X-New", value: "2" }], + env: [], + metadata: [{ key: "trace", value: "abc" }], + connectionTimeout: 0, + requestTimeout: 0, + taskTtl: 60000, + maxFetchRequests: 1000, + roots: [], + }, + ); + }); + + await waitFor(() => { + expect(result.current.servers[0]?.settings?.headers).toEqual([ + { key: "X-New", value: "2" }, + ]); + }); + expect(result.current.servers[0]?.settings?.metadata).toEqual([ + { key: "trace", value: "abc" }, + ]); + }); + it("importSource returns a result for a known source type", async () => { const { result } = renderHook(() => useServers({ baseUrl: "http://test.local", fetchFn: h.fetchFn }), diff --git a/core/react/useServers.ts b/core/react/useServers.ts index 3a2a5002..69935dd9 100644 --- a/core/react/useServers.ts +++ b/core/react/useServers.ts @@ -29,11 +29,28 @@ export interface UseServersResult { loading: boolean; error: string | undefined; refresh: () => Promise; - addServer: (id: string, config: MCPServerConfig) => Promise; + /** + * `settings` is optional: omitted, no settings node is persisted for the new + * entry. Passed, it is written alongside the config in the same POST — the + * add form needs this to carry custom headers, which live on `settings` + * rather than on the transport config (#1915). + */ + addServer: ( + id: string, + config: MCPServerConfig, + settings?: InspectorServerSettings, + ) => Promise; + /** + * `settings` is optional and asymmetric with `addServer`'s: **omitting** it + * tells the backend to preserve the entry's existing settings node, so a + * config-only save cannot silently wipe persisted headers / metadata / OAuth + * credentials. Pass a full settings object to replace it. + */ updateServer: ( originalId: string, newId: string, config: MCPServerConfig, + settings?: InspectorServerSettings, ) => Promise; /** * Patch only the `settings` node on an existing server entry, leaving the @@ -202,11 +219,17 @@ export function useServers(opts: UseServersOptions): UseServersResult { }, [base, authToken, doFetch, refreshInternal]); const addServer = useCallback( - async (id: string, config: MCPServerConfig): Promise => { + async ( + id: string, + config: MCPServerConfig, + settings?: InspectorServerSettings, + ): Promise => { const res = await doFetch(`${base}/api/servers`, { method: "POST", headers: buildHeaders(authToken, true), - body: JSON.stringify({ id, config }), + // `settings` is omitted from the body when undefined (JSON.stringify + // drops undefined values), which the route reads as "no settings node". + body: JSON.stringify({ id, config, settings }), }); if (!res.ok) { throw new Error(await readErrorMessage(res)); @@ -235,18 +258,21 @@ export function useServers(opts: UseServersOptions): UseServersResult { originalId: string, newId: string, config: MCPServerConfig, + settings?: InspectorServerSettings, ): Promise => { - // `settings` is intentionally omitted from the body. The backend route - // treats omission as "preserve the existing settings node on disk", so - // a config-only save (e.g. ServerConfigModal) cannot silently wipe - // persisted headers / metadata / OAuth credentials. To explicitly - // clear settings, send `settings: null`. + // With `settings` undefined, `JSON.stringify` drops the key entirely and + // the backend route treats that omission as "preserve the existing + // settings node on disk" — so a config-only save cannot silently wipe + // persisted headers / metadata / OAuth credentials. A caller editing a + // field that lives on settings (e.g. ServerConfigModal's custom headers) + // passes the full settings object it wants written. To explicitly clear + // settings, send `settings: null`. const res = await doFetch( `${base}/api/servers/${encodeURIComponent(originalId)}`, { method: "PUT", headers: buildHeaders(authToken, true), - body: JSON.stringify({ id: newId, config }), + body: JSON.stringify({ id: newId, config, settings }), }, ); if (!res.ok) {