From 9aab94596438c4da83f801d024f622e007a954f9 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 20:57:02 -0400 Subject: [PATCH 1/6] 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 72d8bd1f3..e3d2557af 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 000000000..05c5ca140 --- /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 000000000..ef96bb8d3 --- /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 000000000..c41bd9c3c --- /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 c180a95b3..0b990f432 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 2f6f4e402..9bd373ef9 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 e1e823a69..24089ff52 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 d238228a1..daf31021e 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 276c9cf9d..22410d5ea 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 a6faac5e6..8c29c4188 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 bb95402d9..49b423f20 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 3a2a50028..69935dd91 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) { From 82ce02b516aee46a3b75173934aaef0b39b85297 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 21:14:33 -0400 Subject: [PATCH 2/6] fix(web): don't copy a cloned server's other settings (#1915) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on #2038. `configModalTarget` is the *source* server in clone mode, so spreading its settings copied that server's OAuth client secret, metadata, roots and behavior flags onto a new entry the user had only given a URL and headers. Only an edit carries the other fields forward now — it has to, since `settings` replaces the node wholesale — while add and clone build from the empty settings shape. Also drops an unnecessary `as unknown as` in the new useServers test (`mcpServers` already holds `StoredMCPServer`, which types `headers`), and removes a `settings: null` instruction the parameter's type does not allow. Signed-off-by: cliffhall --- clients/web/src/App.tsx | 19 ++++++++++++++----- .../src/test/core/react/useServers.test.tsx | 8 +++++--- core/react/useServers.ts | 4 ++-- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index e3d2557af..4c6598bf1 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -3927,11 +3927,20 @@ function App() { 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; + // so they're folded back in here (#1915). + // + // Only an EDIT carries the target's other settings forward. `settings` + // replaces the whole node, so an edit must re-send the fields this modal + // doesn't expose (metadata, timeouts, OAuth credentials, roots) or they + // would be dropped. A CLONE must not: `configModalTarget` there is the + // *source* server, and spreading it would copy that server's OAuth + // client secret and every behavior flag onto a new entry the user only + // gave a URL and some headers. + const isEdit = configModal?.mode === "edit" && !!configModal.targetId; + const existing = isEdit ? configModalTarget?.settings : undefined; + // Send `settings` only when it would change something: with no headers + // on either side there is nothing to write, and omitting the key is what + // makes the backend preserve the node it already has. const settingsChanged = headers.length > 0 || (existing?.headers.length ?? 0) > 0; const settings = settingsChanged diff --git a/clients/web/src/test/core/react/useServers.test.tsx b/clients/web/src/test/core/react/useServers.test.tsx index 49b423f20..d66beb4c6 100644 --- a/clients/web/src/test/core/react/useServers.test.tsx +++ b/clients/web/src/test/core/react/useServers.test.tsx @@ -154,9 +154,11 @@ describe("useServers", () => { { 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" }); + // On disk headers are the flat `Record` form, a direct key + // on the entry (post-#1358) rather than a nested settings node. + expect(readConfig(h.configPath).mcpServers.remote?.headers).toEqual({ + Cookie: "branch=feature-x", + }); }); it("updateServer replaces the settings node when one is supplied (#1915)", async () => { diff --git a/core/react/useServers.ts b/core/react/useServers.ts index 69935dd91..f206a0d59 100644 --- a/core/react/useServers.ts +++ b/core/react/useServers.ts @@ -265,8 +265,8 @@ export function useServers(opts: UseServersOptions): UseServersResult { // 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`. + // passes the full settings object it wants written, which replaces the + // node wholesale — so it must carry forward the fields it isn't editing. const res = await doFetch( `${base}/api/servers/${encodeURIComponent(originalId)}`, { From ed99617c53619a8d62e2b0b506b826bf1feaac21 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 21:40:07 -0400 Subject: [PATCH 3/6] fix(web): lock header rows while submitting, name their clear buttons (#1915) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review follow-ups on #2038, all three from the suppressed set. The header rows stayed editable while a save was in flight, unlike the rest of the modal — a slow request let a user edit a row whose value had already been captured, and the modal then closed having persisted the earlier one. `KeyValueRows` takes a `disabled` prop and the modal passes `submitting`. Each row's clear buttons were still named the bare "Clear", so a screen reader saw six indistinguishable buttons across three rows. They now name the field they empty ("Clear header name, Cookie"). The App-level headers-into-settings merge — the seam that distinguishes an edit from a clone, and where the credential copy lived — moves into `utils/serverSettingsPatch.ts` with its own tests. App.tsx is outside the coverage gate, so inline it could not be covered at all. Signed-off-by: cliffhall --- clients/web/src/App.tsx | 29 ++---- .../KeyValueRows/KeyValueRows.test.tsx | 34 +++++-- .../elements/KeyValueRows/KeyValueRows.tsx | 21 ++++- .../ServerConfigModal/ServerConfigModal.tsx | 1 + .../ServerSettingsForm.test.tsx | 15 +-- .../web/src/utils/serverSettingsPatch.test.ts | 93 +++++++++++++++++++ clients/web/src/utils/serverSettingsPatch.ts | 42 +++++++++ 7 files changed, 202 insertions(+), 33 deletions(-) create mode 100644 clients/web/src/utils/serverSettingsPatch.test.ts create mode 100644 clients/web/src/utils/serverSettingsPatch.ts diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 4c6598bf1..a32ecb908 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -139,6 +139,7 @@ import { type ServerConfigModalMode, } from "./components/groups/ServerConfigModal/ServerConfigModal"; import type { KeyValuePair } from "./components/elements/KeyValueRows/KeyValueRows"; +import { buildHeaderSettingsPatch } from "./utils/serverSettingsPatch"; import { ServerSettingsModal } from "./components/groups/ServerSettingsModal/ServerSettingsModal"; import { ClientSettingsModal } from "./components/groups/ClientSettingsModal/ClientSettingsModal"; import { @@ -3927,25 +3928,15 @@ function App() { 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). - // - // Only an EDIT carries the target's other settings forward. `settings` - // replaces the whole node, so an edit must re-send the fields this modal - // doesn't expose (metadata, timeouts, OAuth credentials, roots) or they - // would be dropped. A CLONE must not: `configModalTarget` there is the - // *source* server, and spreading it would copy that server's OAuth - // client secret and every behavior flag onto a new entry the user only - // gave a URL and some headers. - const isEdit = configModal?.mode === "edit" && !!configModal.targetId; - const existing = isEdit ? configModalTarget?.settings : undefined; - // Send `settings` only when it would change something: with no headers - // on either side there is nothing to write, and omitting the key is what - // makes the backend preserve the node it already has. - const settingsChanged = - headers.length > 0 || (existing?.headers.length ?? 0) > 0; - const settings = settingsChanged - ? { ...(existing ?? EMPTY_SETTINGS), headers } - : undefined; + // so they're folded back in here (#1915). The edit-vs-clone rules live + // in the helper, which is unit-tested — this file is outside the + // coverage gate and that distinction is where credentials can leak. + const settings = buildHeaderSettingsPatch( + configModal?.mode ?? "add", + configModalTarget?.settings, + headers, + EMPTY_SETTINGS, + ); if (configModal?.mode === "edit" && configModal.targetId) { const originalId = configModal.targetId; await updateServer(originalId, id, config, settings); diff --git a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx index ef96bb8d3..09fceca7b 100644 --- a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx +++ b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx @@ -4,13 +4,14 @@ import { renderWithMantine, screen } from "../../../test/renderWithMantine"; import { KeyValueRows } from "./KeyValueRows"; describe("KeyValueRows", () => { - function setup(items: { key: string; value: string }[]) { + function setup(items: { key: string; value: string }[], disabled?: boolean) { const onChange = vi.fn(); const onRemove = vi.fn(); renderWithMantine( , @@ -57,21 +58,42 @@ describe("KeyValueRows", () => { expect(onChange).toHaveBeenLastCalledWith(0, "X", "12"); }); - it("clears a key or a value through its Clear button", async () => { + it("clears a key or a value through its own named 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]!); + // A bare "Clear" repeated per field would be indistinguishable across + // rows, so each clear button names the field it empties. + await user.click( + screen.getByRole("button", { name: "Clear header name, X" }), + ); expect(onChange).toHaveBeenLastCalledWith(0, "", "1"); - await user.click(clearButtons[1]!); + await user.click( + screen.getByRole("button", { name: "Clear header value, X" }), + ); 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); + expect(screen.queryAllByRole("button", { name: /^Clear/ })).toHaveLength(0); + }); + + it("locks every control when disabled", () => { + setup([{ key: "X", value: "1" }], true); + expect( + screen.getByRole("textbox", { name: "header name, X" }), + ).toBeDisabled(); + expect( + screen.getByRole("textbox", { name: "header value, X" }), + ).toBeDisabled(); + expect( + screen.getByRole("button", { name: "Clear header name, X" }), + ).toBeDisabled(); + expect( + screen.getByRole("button", { name: "Remove header, X" }), + ).toBeDisabled(); }); it("removes the clicked row", async () => { diff --git a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx index c41bd9c3c..cf03e8e15 100644 --- a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx +++ b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx @@ -13,9 +13,16 @@ export interface KeyValueRowsProps { * 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". + * another's, and the remove button announces only "X". The clear buttons are + * named the same way — a bare "Clear" repeated six times across three rows + * tells a screen-reader user nothing about which field it empties. */ entityLabel: string; + /** + * Lock every control in the list — used while a submit is in flight, so the + * rows can't drift out of sync with the payload the caller already captured. + */ + disabled?: boolean; onChange: (index: number, key: string, value: string) => void; onRemove: (index: number) => void; } @@ -42,6 +49,7 @@ const RemoveIcon = ActionIcon.withProps({ export function KeyValueRows({ items, entityLabel, + disabled, onChange, onRemove, }: KeyValueRowsProps) { @@ -55,12 +63,15 @@ export function KeyValueRows({ placeholder="Key" aria-label={`${entityLabel} name, ${rowLabel}`} value={item.key} + disabled={disabled} onChange={(e) => onChange(index, e.currentTarget.value, item.value) } rightSection={ item.key ? ( onChange(index, "", item.value)} /> ) : null @@ -70,15 +81,21 @@ export function KeyValueRows({ placeholder="Value" aria-label={`${entityLabel} value, ${rowLabel}`} value={item.value} + disabled={disabled} onChange={(e) => onChange(index, item.key, e.currentTarget.value)} rightSection={ item.value ? ( - onChange(index, item.key, "")} /> + onChange(index, item.key, "")} + /> ) : null } /> onRemove(index)} > X diff --git a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx index 24089ff52..0361aa328 100644 --- a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx +++ b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx @@ -504,6 +504,7 @@ export function ServerConfigModal({ diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx index daf31021e..8bc001c77 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx @@ -8,12 +8,15 @@ import { type ServerSettingsSection, } from "./ServerSettingsForm"; -/** Find the "Clear" button living in the rightSection of `input`'s field. */ +/** Find the clear button living in the rightSection of `input`'s field. The + * name is a prefix match because a KeyValueRows field names its clear button + * for the row it belongs to ("Clear header name, Cookie"), while a standalone + * field keeps the bare "Clear". */ function clearButtonFor(input: HTMLElement): HTMLElement { const root = input.closest('[class*="mantine-TextInput-root"]') ?? input.closest('[class*="Input-wrapper"]'); - return within(root as HTMLElement).getByRole("button", { name: "Clear" }); + return within(root as HTMLElement).getByRole("button", { name: /^Clear/ }); } const emptySettings: InspectorServerSettings = { @@ -1391,12 +1394,12 @@ describe("ServerSettingsForm", () => { expect( within( keyInput.closest('[class*="Input-wrapper"]') as HTMLElement, - ).queryByRole("button", { name: "Clear" }), + ).queryByRole("button", { name: /^Clear/ }), ).toBeNull(); expect( within( valueInput.closest('[class*="Input-wrapper"]') as HTMLElement, - ).queryByRole("button", { name: "Clear" }), + ).queryByRole("button", { name: /^Clear/ }), ).toBeNull(); }); @@ -1419,12 +1422,12 @@ describe("ServerSettingsForm", () => { expect( within( uriInput.closest('[class*="Input-wrapper"]') as HTMLElement, - ).queryByRole("button", { name: "Clear" }), + ).queryByRole("button", { name: /^Clear/ }), ).toBeNull(); expect( within( nameInput.closest('[class*="Input-wrapper"]') as HTMLElement, - ).queryByRole("button", { name: "Clear" }), + ).queryByRole("button", { name: /^Clear/ }), ).toBeNull(); // Typing into the URI threads the empty name through (`root.name ?? ""`). await user.type(uriInput, "f"); diff --git a/clients/web/src/utils/serverSettingsPatch.test.ts b/clients/web/src/utils/serverSettingsPatch.test.ts new file mode 100644 index 000000000..12d7c0814 --- /dev/null +++ b/clients/web/src/utils/serverSettingsPatch.test.ts @@ -0,0 +1,93 @@ +import { describe, it, expect } from "vitest"; +import type { InspectorServerSettings } from "@inspector/core/mcp/types.js"; +import { buildHeaderSettingsPatch } from "./serverSettingsPatch"; + +const EMPTY: InspectorServerSettings = { + headers: [], + env: [], + metadata: [], + connectionTimeout: 0, + requestTimeout: 0, + taskTtl: 60000, + autoRefreshOnListChanged: false, + paginatedLists: false, + maxFetchRequests: 1000, + roots: [], +}; + +// A server carrying settings the modal never shows — the fields an edit must +// preserve and a clone must not copy. +const POPULATED: InspectorServerSettings = { + ...EMPTY, + headers: [{ key: "X-Old", value: "1" }], + metadata: [{ key: "trace", value: "abc" }], + connectionTimeout: 5000, + oauthClientId: "cid", + oauthClientSecret: "shhh", + roots: [{ uri: "file:///project", name: "Project" }], +}; + +const NEW_HEADERS = [{ key: "Cookie", value: "branch=x" }]; + +describe("buildHeaderSettingsPatch", () => { + it("sends nothing when there are no headers on either side", () => { + expect( + buildHeaderSettingsPatch("add", undefined, [], EMPTY), + ).toBeUndefined(); + expect(buildHeaderSettingsPatch("edit", EMPTY, [], EMPTY)).toBeUndefined(); + }); + + it("carries the target's other settings forward on an edit", () => { + const patch = buildHeaderSettingsPatch( + "edit", + POPULATED, + NEW_HEADERS, + EMPTY, + ); + expect(patch).toEqual({ ...POPULATED, headers: NEW_HEADERS }); + // The node is replaced wholesale, so the fields the modal doesn't show + // have to travel with it. + expect(patch?.metadata).toEqual([{ key: "trace", value: "abc" }]); + expect(patch?.oauthClientSecret).toBe("shhh"); + expect(patch?.connectionTimeout).toBe(5000); + }); + + it("still sends on an edit that clears the last header", () => { + expect(buildHeaderSettingsPatch("edit", POPULATED, [], EMPTY)).toEqual({ + ...POPULATED, + headers: [], + }); + }); + + it("does not copy the source server's settings on a clone", () => { + // The regression this guards: `configModalTarget` in clone mode is the + // SOURCE entry, so spreading it put that server's OAuth client secret on a + // brand-new one. + const patch = buildHeaderSettingsPatch( + "clone", + POPULATED, + NEW_HEADERS, + EMPTY, + ); + expect(patch).toEqual({ ...EMPTY, headers: NEW_HEADERS }); + expect(patch?.oauthClientSecret).toBeUndefined(); + expect(patch?.oauthClientId).toBeUndefined(); + expect(patch?.metadata).toEqual([]); + expect(patch?.roots).toEqual([]); + expect(patch?.connectionTimeout).toBe(0); + }); + + it("builds from the empty shape on an add", () => { + expect( + buildHeaderSettingsPatch("add", undefined, NEW_HEADERS, EMPTY), + ).toEqual({ ...EMPTY, headers: NEW_HEADERS }); + }); + + it("ignores a stale target on a clone with no headers", () => { + // A clone of a server that HAS headers, submitted with none: nothing about + // the new entry needs a settings node. + expect( + buildHeaderSettingsPatch("clone", POPULATED, [], EMPTY), + ).toBeUndefined(); + }); +}); diff --git a/clients/web/src/utils/serverSettingsPatch.ts b/clients/web/src/utils/serverSettingsPatch.ts new file mode 100644 index 000000000..b2b51a81a --- /dev/null +++ b/clients/web/src/utils/serverSettingsPatch.ts @@ -0,0 +1,42 @@ +import type { InspectorServerSettings } from "@inspector/core/mcp/types.js"; +import type { ServerConfigModalMode } from "../components/groups/ServerConfigModal/ServerConfigModal"; +import type { KeyValuePair } from "../components/elements/KeyValueRows/KeyValueRows"; + +/** + * Decide what `settings` node ServerConfigModal's submit should send for the + * custom headers it just collected (#1915). + * + * Two rules, both load-bearing: + * + * 1. **Only an edit carries the target's other settings forward.** The + * backend replaces the whole node when one is sent, so an edit must + * re-send the fields the modal doesn't expose (metadata, timeouts, OAuth + * credentials, roots) or they'd be dropped. An **add or clone must not**: + * the modal's target in clone mode is the *source* server, so spreading it + * would copy that server's OAuth client secret and behavior flags onto a + * new entry the user only gave a URL and some headers. + * 2. **`undefined` means "don't send the key at all."** Omitting `settings` + * is what makes the backend preserve the node it already has, so with no + * headers on either side there is nothing to write and nothing to clear. + * Clearing the last header still sends — `existing` had headers, so the + * node must be rewritten without them. + * + * Lives here rather than inline in App.tsx so this seam is unit-testable: + * App.tsx is outside the coverage gate, and the edit-vs-clone distinction is + * exactly where credentials leaked before. + * + * @param emptySettings the app's blank settings shape, used as the base for an + * add or clone. + */ +export function buildHeaderSettingsPatch( + mode: ServerConfigModalMode, + existingSettings: InspectorServerSettings | undefined, + headers: KeyValuePair[], + emptySettings: InspectorServerSettings, +): InspectorServerSettings | undefined { + const existing = mode === "edit" ? existingSettings : undefined; + const changesSomething = + headers.length > 0 || (existing?.headers.length ?? 0) > 0; + if (!changesSomething) return undefined; + return { ...(existing ?? emptySettings), headers }; +} From d60a5c713d43d0f2af729ee11bb3484eff16c72d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 21:57:09 -0400 Subject: [PATCH 4/6] fix(web): only write settings on a real header edit; label rows by position (#1915) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review follow-ups on #2038. `buildHeaderSettingsPatch` treated the mere presence of stored headers as a change, so saving an id or URL edit re-sent the modal's settings snapshot — taken when it opened — and could overwrite a metadata or OAuth change made in the settings form since. It now compares the submitted headers against the stored ones and omits `settings` when they match, which is what makes the PUT route preserve the node. Order counts as a difference: it is what the form round-trips and what the user sees. Row labels carried the key alone, so two rows sharing a key — mid-edit, or a duplicate a server persisted — gave both rows' controls identical accessible names, the thing the labelling exists to prevent. The row number is now always part of the label rather than a blank-key fallback. Signed-off-by: cliffhall --- .../KeyValueRows/KeyValueRows.stories.tsx | 6 +-- .../KeyValueRows/KeyValueRows.test.tsx | 41 ++++++++++++------ .../elements/KeyValueRows/KeyValueRows.tsx | 8 +++- .../ServerConfigModal.stories.tsx | 2 +- .../ServerConfigModal.test.tsx | 8 ++-- .../ServerSettingsForm.test.tsx | 12 ++++-- .../ServerSettingsModal.test.tsx | 12 ++++-- .../web/src/utils/serverSettingsPatch.test.ts | 42 +++++++++++++++++++ clients/web/src/utils/serverSettingsPatch.ts | 26 ++++++++---- 9 files changed, 121 insertions(+), 36 deletions(-) diff --git a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.stories.tsx b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.stories.tsx index 05c5ca140..055a148bb 100644 --- a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.stories.tsx +++ b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.stories.tsx @@ -27,18 +27,18 @@ export const Populated: Story = { play: async ({ args, canvasElement }) => { const canvas = within(canvasElement); const value = await canvas.findByRole("textbox", { - name: "header value, Cookie", + name: "header value, Cookie, row 1", }); await expect(value).toHaveValue("branch=feature-x"); await userEvent.click( - canvas.getByRole("button", { name: "Remove header, X-Env" }), + canvas.getByRole("button", { name: "Remove header, X-Env, row 2" }), ); await expect(args.onRemove).toHaveBeenCalledWith(1); }, }; -// A row whose key is still blank falls back to a positional name rather than +// A row whose key is still blank is named by position alone rather than // announcing nothing. export const BlankKeyRow: Story = { args: { items: [{ key: "", value: "" }] }, diff --git a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx index 09fceca7b..60e6916d3 100644 --- a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx +++ b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx @@ -30,12 +30,12 @@ describe("KeyValueRows", () => { { key: "", value: "" }, ]); expect( - screen.getByRole("textbox", { name: "header name, Cookie" }), + screen.getByRole("textbox", { name: "header name, Cookie, row 1" }), ).toHaveValue("Cookie"); expect( - screen.getByRole("textbox", { name: "header value, Cookie" }), + screen.getByRole("textbox", { name: "header value, Cookie, row 1" }), ).toHaveValue("a=1"); - // A key that is blank (or only whitespace) falls back to its position. + // A blank (or whitespace-only) key leaves the position alone as the name. expect( screen.getByRole("textbox", { name: "header name, row 2" }), ).toBeInTheDocument(); @@ -46,13 +46,13 @@ describe("KeyValueRows", () => { const { onChange } = setup([{ key: "X", value: "1" }]); await user.type( - screen.getByRole("textbox", { name: "header name, X" }), + screen.getByRole("textbox", { name: "header name, X, row 1" }), "Y", ); expect(onChange).toHaveBeenLastCalledWith(0, "XY", "1"); await user.type( - screen.getByRole("textbox", { name: "header value, X" }), + screen.getByRole("textbox", { name: "header value, X, row 1" }), "2", ); expect(onChange).toHaveBeenLastCalledWith(0, "X", "12"); @@ -65,12 +65,12 @@ describe("KeyValueRows", () => { // A bare "Clear" repeated per field would be indistinguishable across // rows, so each clear button names the field it empties. await user.click( - screen.getByRole("button", { name: "Clear header name, X" }), + screen.getByRole("button", { name: "Clear header name, X, row 1" }), ); expect(onChange).toHaveBeenLastCalledWith(0, "", "1"); await user.click( - screen.getByRole("button", { name: "Clear header value, X" }), + screen.getByRole("button", { name: "Clear header value, X, row 1" }), ); expect(onChange).toHaveBeenLastCalledWith(0, "X", ""); }); @@ -83,19 +83,34 @@ describe("KeyValueRows", () => { it("locks every control when disabled", () => { setup([{ key: "X", value: "1" }], true); expect( - screen.getByRole("textbox", { name: "header name, X" }), + screen.getByRole("textbox", { name: "header name, X, row 1" }), ).toBeDisabled(); expect( - screen.getByRole("textbox", { name: "header value, X" }), + screen.getByRole("textbox", { name: "header value, X, row 1" }), ).toBeDisabled(); expect( - screen.getByRole("button", { name: "Clear header name, X" }), + screen.getByRole("button", { name: "Clear header name, X, row 1" }), ).toBeDisabled(); expect( - screen.getByRole("button", { name: "Remove header, X" }), + screen.getByRole("button", { name: "Remove header, X, row 1" }), ).toBeDisabled(); }); + it("distinguishes two rows that share a key", () => { + // Duplicate keys happen mid-edit, and a server can persist duplicate + // metadata, so the key alone cannot identify a row. + setup([ + { key: "Set-Cookie", value: "a=1" }, + { key: "Set-Cookie", value: "b=2" }, + ]); + expect( + screen.getByRole("textbox", { name: "header value, Set-Cookie, row 1" }), + ).toHaveValue("a=1"); + expect( + screen.getByRole("textbox", { name: "header value, Set-Cookie, row 2" }), + ).toHaveValue("b=2"); + }); + it("removes the clicked row", async () => { const user = userEvent.setup({ delay: null }); const { onRemove } = setup([ @@ -103,7 +118,9 @@ describe("KeyValueRows", () => { { key: "B", value: "2" }, ]); - await user.click(screen.getByRole("button", { name: "Remove header, B" })); + await user.click( + screen.getByRole("button", { name: "Remove header, B, row 2" }), + ); expect(onRemove).toHaveBeenCalledWith(1); }); }); diff --git a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx index cf03e8e15..9a44585d9 100644 --- a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx +++ b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx @@ -56,7 +56,13 @@ export function KeyValueRows({ return ( <> {items.map((item, index) => { - const rowLabel = item.key.trim() || `row ${index + 1}`; + // The row number is always part of the label, not just a fallback for + // a blank key: two rows can carry the SAME key (mid-edit, or a + // duplicate a server legitimately persisted), and a key-only label + // would give both rows' controls identical accessible names — the very + // thing this labelling exists to prevent. + const key = item.key.trim(); + const rowLabel = key ? `${key}, row ${index + 1}` : `row ${index + 1}`; return ( { "Cookie", ); await user.type( - screen.getByRole("textbox", { name: /header value, Cookie/ }), + screen.getByRole("textbox", { name: /header value, Cookie, row 1/ }), "branch=feature-x", ); await user.click(screen.getByRole("button", { name: /^Add$/ })); @@ -538,7 +538,7 @@ describe("ServerConfigModal", () => { ); const valueInput = screen.getByRole("textbox", { - name: /header value, X-Env/, + name: /header value, X-Env, row 1/, }); expect(valueInput).toHaveValue("dev"); await user.type(valueInput, "-2"); @@ -573,11 +573,11 @@ describe("ServerConfigModal", () => { 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" }), + screen.getByRole("textbox", { name: "header value, Keep, row 1" }), "9", ); await user.click( - screen.getByRole("button", { name: "Remove header, Drop" }), + screen.getByRole("button", { name: "Remove header, Drop, row 2" }), ); await user.click(screen.getByRole("button", { name: /^Save$/ })); diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx index 8bc001c77..d313c00ac 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx @@ -10,7 +10,7 @@ import { /** Find the clear button living in the rightSection of `input`'s field. The * name is a prefix match because a KeyValueRows field names its clear button - * for the row it belongs to ("Clear header name, Cookie"), while a standalone + * for the row it belongs to ("Clear header name, Cookie, row 1"), while a standalone * field keeps the bare "Clear". */ function clearButtonFor(input: HTMLElement): HTMLElement { const root = @@ -333,7 +333,9 @@ describe("ServerSettingsForm", () => { />, ); await user.click( - screen.getByRole("button", { name: "Remove header, Authorization" }), + screen.getByRole("button", { + name: "Remove header, Authorization, row 1", + }), ); expect(onRemoveHeader).toHaveBeenCalledWith(0); }); @@ -356,7 +358,9 @@ describe("ServerSettingsForm", () => { expect(onMetadataChange).toHaveBeenCalled(); await user.click( - screen.getByRole("button", { name: "Remove metadata entry, userId" }), + screen.getByRole("button", { + name: "Remove metadata entry, userId, row 1", + }), ); expect(onRemoveMetadata).toHaveBeenCalledWith(0); }); @@ -685,7 +689,7 @@ describe("ServerSettingsForm", () => { // accessible name identifies which row it belongs to. await user.click( screen.getByRole("button", { - name: "Remove environment variable, API_KEY", + name: "Remove environment variable, API_KEY, row 1", }), ); expect(onRemoveEnv).toHaveBeenCalledWith(0); diff --git a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx index 8c29c4188..a5dbcb0db 100644 --- a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx @@ -277,7 +277,9 @@ describe("ServerSettingsModal", () => { ); await user.click(screen.getByRole("button", { name: "Custom Headers" })); await user.click( - screen.getByRole("button", { name: "Remove header, Authorization" }), + screen.getByRole("button", { + name: "Remove header, Authorization, row 1", + }), ); expect(onSettingsChange).toHaveBeenCalledWith({ ...initialSettings, @@ -346,7 +348,9 @@ describe("ServerSettingsModal", () => { // 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" }), + screen.getByRole("button", { + name: "Remove metadata entry, userId, row 1", + }), ); expect(onSettingsChange).toHaveBeenCalledWith({ ...initialSettings, @@ -767,7 +771,9 @@ describe("ServerSettingsModal", () => { screen.getByRole("button", { name: "Environment Variables" }), ); await user.click( - screen.getByRole("button", { name: "Remove environment variable, A" }), + screen.getByRole("button", { + name: "Remove environment variable, A, row 1", + }), ); expect(onSettingsChange).toHaveBeenCalledWith({ ...emptySettings, diff --git a/clients/web/src/utils/serverSettingsPatch.test.ts b/clients/web/src/utils/serverSettingsPatch.test.ts index 12d7c0814..06c892cfb 100644 --- a/clients/web/src/utils/serverSettingsPatch.test.ts +++ b/clients/web/src/utils/serverSettingsPatch.test.ts @@ -52,6 +52,48 @@ describe("buildHeaderSettingsPatch", () => { expect(patch?.connectionTimeout).toBe(5000); }); + it("sends nothing when an edit leaves the headers untouched", () => { + // The modal holds a snapshot taken when it opened, so writing it back on + // an id/URL-only save would clobber a metadata or OAuth change made in the + // settings form since. Omitting the key is what preserves it. + expect( + buildHeaderSettingsPatch( + "edit", + POPULATED, + // A fresh array with equal contents — the modal always rebuilds it. + [{ key: "X-Old", value: "1" }], + EMPTY, + ), + ).toBeUndefined(); + }); + + it("sends when an edit only reorders the headers", () => { + const two: InspectorServerSettings = { + ...POPULATED, + headers: [ + { key: "A", value: "1" }, + { key: "B", value: "2" }, + ], + }; + expect( + buildHeaderSettingsPatch( + "edit", + two, + [ + { key: "B", value: "2" }, + { key: "A", value: "1" }, + ], + EMPTY, + ), + ).toEqual({ + ...two, + headers: [ + { key: "B", value: "2" }, + { key: "A", value: "1" }, + ], + }); + }); + it("still sends on an edit that clears the last header", () => { expect(buildHeaderSettingsPatch("edit", POPULATED, [], EMPTY)).toEqual({ ...POPULATED, diff --git a/clients/web/src/utils/serverSettingsPatch.ts b/clients/web/src/utils/serverSettingsPatch.ts index b2b51a81a..25fe7709c 100644 --- a/clients/web/src/utils/serverSettingsPatch.ts +++ b/clients/web/src/utils/serverSettingsPatch.ts @@ -15,11 +15,14 @@ import type { KeyValuePair } from "../components/elements/KeyValueRows/KeyValueR * the modal's target in clone mode is the *source* server, so spreading it * would copy that server's OAuth client secret and behavior flags onto a * new entry the user only gave a URL and some headers. - * 2. **`undefined` means "don't send the key at all."** Omitting `settings` - * is what makes the backend preserve the node it already has, so with no - * headers on either side there is nothing to write and nothing to clear. - * Clearing the last header still sends — `existing` had headers, so the - * node must be rewritten without them. + * 2. **`undefined` means "don't send the key at all", and that is the + * default.** Omitting `settings` makes the backend preserve the node it + * already has, which matters beyond convenience: what this modal holds is + * a snapshot taken when it opened, so writing it back on a save that did + * not touch a header would overwrite a metadata or OAuth change made in + * the settings form in the meantime. So the patch is sent only when the + * submitted headers actually differ from the stored ones — including the + * case where the last one was cleared, which does need the node rewritten. * * Lives here rather than inline in App.tsx so this seam is unit-testable: * App.tsx is outside the coverage gate, and the edit-vs-clone distinction is @@ -35,8 +38,15 @@ export function buildHeaderSettingsPatch( emptySettings: InspectorServerSettings, ): InspectorServerSettings | undefined { const existing = mode === "edit" ? existingSettings : undefined; - const changesSomething = - headers.length > 0 || (existing?.headers.length ?? 0) > 0; - if (!changesSomething) return undefined; + if (sameHeaders(existing?.headers ?? [], headers)) return undefined; return { ...(existing ?? emptySettings), headers }; } + +/** + * Order-sensitive pair-list equality. Order matters because it is what the + * form round-trips and what the user sees, so a reorder is a real edit. + */ +function sameHeaders(a: KeyValuePair[], b: KeyValuePair[]): boolean { + if (a.length !== b.length) return false; + return a.every((h, i) => h.key === b[i]?.key && h.value === b[i]?.value); +} From 51d35d864b97497b80a7b54b2227724cd25abff3 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 22:14:40 -0400 Subject: [PATCH 5/6] fix(web): correct the OAuth header hint, keep utils off components (#1915) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 review follow-ups on #2038. The Custom Headers hint claimed a custom `Authorization` value is "ignored" once OAuth is configured. Verified against the installed @modelcontextprotocol/client: both SSE and streamable-HTTP `_commonHeaders()` set the bearer first and then spread `requestInit.headers`, so a custom value *overrides* the OAuth token. The hint told users the opposite of what would happen, and could have them silently break their own auth. Corrected in both places — the modal's new hint and the pre-existing one in ServerSettingsForm, which carried the same wrong claim. `utils/serverSettingsPatch.ts` imported both its shared types from components, reversing the one-way `components -> utils` direction AGENTS.md sets. `KeyValuePair` moves to `utils/keyValuePairs.ts` (KeyValueRows re-exports it), and the mode is spelled as a literal union — still checked, since `ServerConfigModalMode` must stay assignable to it at the call site. Signed-off-by: cliffhall --- .../elements/KeyValueRows/KeyValueRows.tsx | 8 ++++---- .../ServerConfigModal/ServerConfigModal.tsx | 6 +++--- .../ServerSettingsForm/ServerSettingsForm.tsx | 8 +++++--- clients/web/src/utils/keyValuePairs.ts | 15 +++++++++++++++ clients/web/src/utils/serverSettingsPatch.ts | 9 ++++++--- 5 files changed, 33 insertions(+), 13 deletions(-) create mode 100644 clients/web/src/utils/keyValuePairs.ts diff --git a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx index 9a44585d9..776ce5e9b 100644 --- a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx +++ b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx @@ -1,10 +1,10 @@ import { ActionIcon, Group, TextInput } from "@mantine/core"; import { ClearButton } from "../ClearButton/ClearButton"; +import type { KeyValuePair } from "../../../utils/keyValuePairs"; -export interface KeyValuePair { - key: string; - value: string; -} +// Re-exported for call sites that already import this component, so they need +// not know the type lives a layer down. +export type { KeyValuePair }; export interface KeyValueRowsProps { items: KeyValuePair[]; diff --git a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx index 0361aa328..105bb121c 100644 --- a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx +++ b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx @@ -497,9 +497,9 @@ export function ServerConfigModal({ 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. + included. A custom `Authorization` header takes precedence + over an OAuth access token, so remove it if you configure + OAuth for this server later. - Headers sent with every HTTP request to this server. If OAuth is - configured below, the `Authorization` header is owned by the - OAuth flow and any value set here is ignored. + Headers sent with every HTTP request to this server. A custom + `Authorization` header takes precedence over an OAuth access + token — the SDK transports apply these headers last — so remove + it once OAuth is configured, or the flow's token never gets + sent. + Add Header diff --git a/clients/web/src/utils/keyValuePairs.ts b/clients/web/src/utils/keyValuePairs.ts new file mode 100644 index 000000000..2625e6515 --- /dev/null +++ b/clients/web/src/utils/keyValuePairs.ts @@ -0,0 +1,15 @@ +/** + * One editable `{ key, value }` row — the in-memory shape the Inspector uses + * for custom headers, request metadata, and stdio environment variables + * (`InspectorServerSettings.headers` and friends). A pair *array* rather than + * a record specifically so a controlled form can hold a half-typed row with a + * blank key; the persist layer collapses it to a record and drops the blanks. + * + * Declared here, in `utils`, because it is a pure domain type shared by a + * component (`KeyValueRows`, which re-exports it) and non-UI logic + * (`serverSettingsPatch`). Imports point `components -> utils`, never back. + */ +export interface KeyValuePair { + key: string; + value: string; +} diff --git a/clients/web/src/utils/serverSettingsPatch.ts b/clients/web/src/utils/serverSettingsPatch.ts index 25fe7709c..b7500c44b 100644 --- a/clients/web/src/utils/serverSettingsPatch.ts +++ b/clients/web/src/utils/serverSettingsPatch.ts @@ -1,6 +1,5 @@ import type { InspectorServerSettings } from "@inspector/core/mcp/types.js"; -import type { ServerConfigModalMode } from "../components/groups/ServerConfigModal/ServerConfigModal"; -import type { KeyValuePair } from "../components/elements/KeyValueRows/KeyValueRows"; +import type { KeyValuePair } from "./keyValuePairs"; /** * Decide what `settings` node ServerConfigModal's submit should send for the @@ -32,7 +31,11 @@ import type { KeyValuePair } from "../components/elements/KeyValueRows/KeyValueR * add or clone. */ export function buildHeaderSettingsPatch( - mode: ServerConfigModalMode, + // Spelled as a literal union rather than imported from ServerConfigModal: + // `utils` must not depend on `components`. It stays honest because + // `ServerConfigModalMode` is checked against it at the call site — adding a + // fourth mode there is a compile error here. + mode: "add" | "edit" | "clone", existingSettings: InspectorServerSettings | undefined, headers: KeyValuePair[], emptySettings: InspectorServerSettings, From 4dd8ed4498c4b53313a651fc3b7ec66624d41166 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 22:57:34 -0400 Subject: [PATCH 6/6] revert(web): drop custom headers from the manual add form (#1915) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Server Settings already has a Custom Headers editor, so adding one to the manual add form created a second place to edit the same field. The reporter had also conditionally withdrawn #1915 — "editing that information from the server settings dialog is sufficient" once #1848 closed, which it has. Reverts the add-form editor, the shared KeyValueRows element, the optional `settings` argument on addServer / updateServer, and the App-level merge helper. What remains are two defects found while working in ServerSettingsForm, which this PR now closes instead (#2040): the Custom Headers hint misstating OAuth precedence, and the key/value rows' missing accessible names. Signed-off-by: cliffhall --- clients/web/src/App.tsx | 61 ++---- .../KeyValueRows/KeyValueRows.stories.tsx | 60 ------ .../KeyValueRows/KeyValueRows.test.tsx | 126 ------------ .../elements/KeyValueRows/KeyValueRows.tsx | 114 ----------- .../ServerConfigModal.stories.tsx | 40 +--- .../ServerConfigModal.test.tsx | 180 ++---------------- .../ServerConfigModal/ServerConfigModal.tsx | 129 +++---------- .../ServerSettingsForm/ServerSettingsForm.tsx | 79 +++++++- .../src/test/core/react/useServers.test.tsx | 89 --------- clients/web/src/utils/keyValuePairs.ts | 15 -- .../web/src/utils/serverSettingsPatch.test.ts | 135 ------------- clients/web/src/utils/serverSettingsPatch.ts | 55 ------ core/react/useServers.ts | 44 +---- 13 files changed, 152 insertions(+), 975 deletions(-) delete mode 100644 clients/web/src/components/elements/KeyValueRows/KeyValueRows.stories.tsx delete mode 100644 clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx delete mode 100644 clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx delete mode 100644 clients/web/src/utils/keyValuePairs.ts delete mode 100644 clients/web/src/utils/serverSettingsPatch.test.ts delete mode 100644 clients/web/src/utils/serverSettingsPatch.ts diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index a32ecb908..72d8bd1f3 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -138,8 +138,6 @@ import { ServerConfigModal, type ServerConfigModalMode, } from "./components/groups/ServerConfigModal/ServerConfigModal"; -import type { KeyValuePair } from "./components/elements/KeyValueRows/KeyValueRows"; -import { buildHeaderSettingsPatch } from "./utils/serverSettingsPatch"; import { ServerSettingsModal } from "./components/groups/ServerSettingsModal/ServerSettingsModal"; import { ClientSettingsModal } from "./components/groups/ClientSettingsModal/ClientSettingsModal"; import { @@ -3897,17 +3895,30 @@ 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, - settings?: InspectorServerSettings, - ) => { - await addServer(id, config, settings); + async (id: string, config: MCPServerConfig) => { + await addServer(id, config); 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. @@ -3924,39 +3935,6 @@ 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). The edit-vs-clone rules live - // in the helper, which is unit-tested — this file is outside the - // coverage gate and that distinction is where credentials can leak. - const settings = buildHeaderSettingsPatch( - configModal?.mode ?? "add", - configModalTarget?.settings, - headers, - EMPTY_SETTINGS, - ); - 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); @@ -4572,7 +4550,6 @@ 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 deleted file mode 100644 index 055a148bb..000000000 --- a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.stories.tsx +++ /dev/null @@ -1,60 +0,0 @@ -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, row 1", - }); - await expect(value).toHaveValue("branch=feature-x"); - - await userEvent.click( - canvas.getByRole("button", { name: "Remove header, X-Env, row 2" }), - ); - await expect(args.onRemove).toHaveBeenCalledWith(1); - }, -}; - -// A row whose key is still blank is named by position alone 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 deleted file mode 100644 index 60e6916d3..000000000 --- a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.test.tsx +++ /dev/null @@ -1,126 +0,0 @@ -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 }[], disabled?: boolean) { - 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, row 1" }), - ).toHaveValue("Cookie"); - expect( - screen.getByRole("textbox", { name: "header value, Cookie, row 1" }), - ).toHaveValue("a=1"); - // A blank (or whitespace-only) key leaves the position alone as the name. - 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, row 1" }), - "Y", - ); - expect(onChange).toHaveBeenLastCalledWith(0, "XY", "1"); - - await user.type( - screen.getByRole("textbox", { name: "header value, X, row 1" }), - "2", - ); - expect(onChange).toHaveBeenLastCalledWith(0, "X", "12"); - }); - - it("clears a key or a value through its own named Clear button", async () => { - const user = userEvent.setup({ delay: null }); - const { onChange } = setup([{ key: "X", value: "1" }]); - - // A bare "Clear" repeated per field would be indistinguishable across - // rows, so each clear button names the field it empties. - await user.click( - screen.getByRole("button", { name: "Clear header name, X, row 1" }), - ); - expect(onChange).toHaveBeenLastCalledWith(0, "", "1"); - - await user.click( - screen.getByRole("button", { name: "Clear header value, X, row 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("locks every control when disabled", () => { - setup([{ key: "X", value: "1" }], true); - expect( - screen.getByRole("textbox", { name: "header name, X, row 1" }), - ).toBeDisabled(); - expect( - screen.getByRole("textbox", { name: "header value, X, row 1" }), - ).toBeDisabled(); - expect( - screen.getByRole("button", { name: "Clear header name, X, row 1" }), - ).toBeDisabled(); - expect( - screen.getByRole("button", { name: "Remove header, X, row 1" }), - ).toBeDisabled(); - }); - - it("distinguishes two rows that share a key", () => { - // Duplicate keys happen mid-edit, and a server can persist duplicate - // metadata, so the key alone cannot identify a row. - setup([ - { key: "Set-Cookie", value: "a=1" }, - { key: "Set-Cookie", value: "b=2" }, - ]); - expect( - screen.getByRole("textbox", { name: "header value, Set-Cookie, row 1" }), - ).toHaveValue("a=1"); - expect( - screen.getByRole("textbox", { name: "header value, Set-Cookie, row 2" }), - ).toHaveValue("b=2"); - }); - - 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, row 2" }), - ); - expect(onRemove).toHaveBeenCalledWith(1); - }); -}); diff --git a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx deleted file mode 100644 index 776ce5e9b..000000000 --- a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.tsx +++ /dev/null @@ -1,114 +0,0 @@ -import { ActionIcon, Group, TextInput } from "@mantine/core"; -import { ClearButton } from "../ClearButton/ClearButton"; -import type { KeyValuePair } from "../../../utils/keyValuePairs"; - -// Re-exported for call sites that already import this component, so they need -// not know the type lives a layer down. -export type { KeyValuePair }; - -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". The clear buttons are - * named the same way — a bare "Clear" repeated six times across three rows - * tells a screen-reader user nothing about which field it empties. - */ - entityLabel: string; - /** - * Lock every control in the list — used while a submit is in flight, so the - * rows can't drift out of sync with the payload the caller already captured. - */ - disabled?: boolean; - 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, - disabled, - onChange, - onRemove, -}: KeyValueRowsProps) { - return ( - <> - {items.map((item, index) => { - // The row number is always part of the label, not just a fallback for - // a blank key: two rows can carry the SAME key (mid-edit, or a - // duplicate a server legitimately persisted), and a key-only label - // would give both rows' controls identical accessible names — the very - // thing this labelling exists to prevent. - const key = item.key.trim(); - const rowLabel = key ? `${key}, row ${index + 1}` : `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 d44cd75ee..c180a95b3 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, headers) => { - await args.onSubmit(id, config, headers); + onSubmit={async (id, config) => { + await args.onSubmit(id, config); updateArgs({ opened: false }); }} /> @@ -134,39 +134,7 @@ 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(); - // 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, row 1", - })) 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(); + // Headers are no longer entered here — they live in ServerSettingsForm. + await expect(dialog.queryByLabelText(/Headers/i)).toBeNull(); }, }; diff --git a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx index 0a3416c63..2f6f4e402 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 and no headers", async () => { + it("submits an sse config with just the url (headers move to the settings form)", async () => { const user = userEvent.setup({ delay: null }); const props = base(); renderWithMantine( @@ -133,11 +133,10 @@ 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 () => { @@ -153,11 +152,10 @@ 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", () => { @@ -277,11 +275,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 () => { @@ -472,144 +470,4 @@ 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, row 1/ }), - "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, row 1/, - }); - 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, row 1" }), - "9", - ); - await user.click( - screen.getByRole("button", { name: "Remove header, Drop, row 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: "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 105bb121c..e1e823a69 100644 --- a/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx +++ b/clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx @@ -10,10 +10,6 @@ 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, @@ -32,27 +28,11 @@ 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; - /** - * `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; + onSubmit: (id: string, config: MCPServerConfig) => Promise | void; } type TransportChoice = "stdio" | "sse" | "streamable-http"; @@ -67,8 +47,6 @@ 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 @@ -106,10 +84,6 @@ 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", @@ -120,14 +94,9 @@ 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) { @@ -139,7 +108,6 @@ function configToFormState( envText: "", cwd: "", url: "", - headers, }; } if (transport === "stdio") { @@ -154,9 +122,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 @@ -169,7 +137,6 @@ function configToFormState( envText: "", cwd: "", url: url ?? "", - headers, }; } @@ -218,14 +185,13 @@ export function ServerConfigModal({ mode, initialId, initialConfig, - initialHeaders, existingIds, onClose, onSubmit, }: ServerConfigModalProps) { const initial = useMemo( - () => configToFormState(initialId, initialConfig, initialHeaders, mode), - [initialId, initialConfig, initialHeaders, mode], + () => configToFormState(initialId, initialConfig, mode), + [initialId, initialConfig, mode], ); const [form, setForm] = useState(initial); const [submitError, setSubmitError] = useState(undefined); @@ -244,22 +210,6 @@ 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` @@ -313,9 +263,9 @@ export function ServerConfigModal({ return { ok: false, error: "URL is required for sse / streamable-http." }; } const base = { url: form.url.trim() }; - // 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. + // 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. const config: MCPServerConfig = form.transport === "sse" ? { type: "sse", ...base } @@ -338,19 +288,9 @@ 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, headers); + await onSubmit(trimmedId, built.config); onClose(); } catch (err) { setSubmitError(err instanceof Error ? err.message : String(err)); @@ -472,44 +412,21 @@ export function ServerConfigModal({ /> ) : ( - <> - - ) : null - } - /> - - - Custom headers - - + Add Header - - - - Sent with every HTTP request to this server — cookies - included. A custom `Authorization` header takes precedence - over an OAuth access token, so remove it if you configure - OAuth for this server later. - - - - + + ) : null + } + /> )} diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx index 5faf82c23..48e67c055 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx @@ -13,7 +13,6 @@ 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, @@ -224,6 +223,84 @@ const ClearStoredOAuthHint = Text.withProps({ miw: "12rem", }); +function KeyValueRows({ + items, + entityLabel, + onChange, + onRemove, +}: { + items: { key: string; value: string }[]; + /** + * 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 section's key box + * from another's, and every remove button announces only "X". The row number + * rides along because two rows can carry the same key — mid-edit, or a + * duplicate a server persisted — and a key-only name would leave both rows' + * controls indistinguishable, which is the thing this exists to prevent. + */ + entityLabel: 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) => { + const key = item.key.trim(); + const rowLabel = key ? `${key}, row ${index + 1}` : `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 + + + ); + })} + + ); +} + // 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 diff --git a/clients/web/src/test/core/react/useServers.test.tsx b/clients/web/src/test/core/react/useServers.test.tsx index d66beb4c6..bb95402d9 100644 --- a/clients/web/src/test/core/react/useServers.test.tsx +++ b/clients/web/src/test/core/react/useServers.test.tsx @@ -123,95 +123,6 @@ 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" }, - ]); - }); - // On disk headers are the flat `Record` form, a direct key - // on the entry (post-#1358) rather than a nested settings node. - expect(readConfig(h.configPath).mcpServers.remote?.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/clients/web/src/utils/keyValuePairs.ts b/clients/web/src/utils/keyValuePairs.ts deleted file mode 100644 index 2625e6515..000000000 --- a/clients/web/src/utils/keyValuePairs.ts +++ /dev/null @@ -1,15 +0,0 @@ -/** - * One editable `{ key, value }` row — the in-memory shape the Inspector uses - * for custom headers, request metadata, and stdio environment variables - * (`InspectorServerSettings.headers` and friends). A pair *array* rather than - * a record specifically so a controlled form can hold a half-typed row with a - * blank key; the persist layer collapses it to a record and drops the blanks. - * - * Declared here, in `utils`, because it is a pure domain type shared by a - * component (`KeyValueRows`, which re-exports it) and non-UI logic - * (`serverSettingsPatch`). Imports point `components -> utils`, never back. - */ -export interface KeyValuePair { - key: string; - value: string; -} diff --git a/clients/web/src/utils/serverSettingsPatch.test.ts b/clients/web/src/utils/serverSettingsPatch.test.ts deleted file mode 100644 index 06c892cfb..000000000 --- a/clients/web/src/utils/serverSettingsPatch.test.ts +++ /dev/null @@ -1,135 +0,0 @@ -import { describe, it, expect } from "vitest"; -import type { InspectorServerSettings } from "@inspector/core/mcp/types.js"; -import { buildHeaderSettingsPatch } from "./serverSettingsPatch"; - -const EMPTY: InspectorServerSettings = { - headers: [], - env: [], - metadata: [], - connectionTimeout: 0, - requestTimeout: 0, - taskTtl: 60000, - autoRefreshOnListChanged: false, - paginatedLists: false, - maxFetchRequests: 1000, - roots: [], -}; - -// A server carrying settings the modal never shows — the fields an edit must -// preserve and a clone must not copy. -const POPULATED: InspectorServerSettings = { - ...EMPTY, - headers: [{ key: "X-Old", value: "1" }], - metadata: [{ key: "trace", value: "abc" }], - connectionTimeout: 5000, - oauthClientId: "cid", - oauthClientSecret: "shhh", - roots: [{ uri: "file:///project", name: "Project" }], -}; - -const NEW_HEADERS = [{ key: "Cookie", value: "branch=x" }]; - -describe("buildHeaderSettingsPatch", () => { - it("sends nothing when there are no headers on either side", () => { - expect( - buildHeaderSettingsPatch("add", undefined, [], EMPTY), - ).toBeUndefined(); - expect(buildHeaderSettingsPatch("edit", EMPTY, [], EMPTY)).toBeUndefined(); - }); - - it("carries the target's other settings forward on an edit", () => { - const patch = buildHeaderSettingsPatch( - "edit", - POPULATED, - NEW_HEADERS, - EMPTY, - ); - expect(patch).toEqual({ ...POPULATED, headers: NEW_HEADERS }); - // The node is replaced wholesale, so the fields the modal doesn't show - // have to travel with it. - expect(patch?.metadata).toEqual([{ key: "trace", value: "abc" }]); - expect(patch?.oauthClientSecret).toBe("shhh"); - expect(patch?.connectionTimeout).toBe(5000); - }); - - it("sends nothing when an edit leaves the headers untouched", () => { - // The modal holds a snapshot taken when it opened, so writing it back on - // an id/URL-only save would clobber a metadata or OAuth change made in the - // settings form since. Omitting the key is what preserves it. - expect( - buildHeaderSettingsPatch( - "edit", - POPULATED, - // A fresh array with equal contents — the modal always rebuilds it. - [{ key: "X-Old", value: "1" }], - EMPTY, - ), - ).toBeUndefined(); - }); - - it("sends when an edit only reorders the headers", () => { - const two: InspectorServerSettings = { - ...POPULATED, - headers: [ - { key: "A", value: "1" }, - { key: "B", value: "2" }, - ], - }; - expect( - buildHeaderSettingsPatch( - "edit", - two, - [ - { key: "B", value: "2" }, - { key: "A", value: "1" }, - ], - EMPTY, - ), - ).toEqual({ - ...two, - headers: [ - { key: "B", value: "2" }, - { key: "A", value: "1" }, - ], - }); - }); - - it("still sends on an edit that clears the last header", () => { - expect(buildHeaderSettingsPatch("edit", POPULATED, [], EMPTY)).toEqual({ - ...POPULATED, - headers: [], - }); - }); - - it("does not copy the source server's settings on a clone", () => { - // The regression this guards: `configModalTarget` in clone mode is the - // SOURCE entry, so spreading it put that server's OAuth client secret on a - // brand-new one. - const patch = buildHeaderSettingsPatch( - "clone", - POPULATED, - NEW_HEADERS, - EMPTY, - ); - expect(patch).toEqual({ ...EMPTY, headers: NEW_HEADERS }); - expect(patch?.oauthClientSecret).toBeUndefined(); - expect(patch?.oauthClientId).toBeUndefined(); - expect(patch?.metadata).toEqual([]); - expect(patch?.roots).toEqual([]); - expect(patch?.connectionTimeout).toBe(0); - }); - - it("builds from the empty shape on an add", () => { - expect( - buildHeaderSettingsPatch("add", undefined, NEW_HEADERS, EMPTY), - ).toEqual({ ...EMPTY, headers: NEW_HEADERS }); - }); - - it("ignores a stale target on a clone with no headers", () => { - // A clone of a server that HAS headers, submitted with none: nothing about - // the new entry needs a settings node. - expect( - buildHeaderSettingsPatch("clone", POPULATED, [], EMPTY), - ).toBeUndefined(); - }); -}); diff --git a/clients/web/src/utils/serverSettingsPatch.ts b/clients/web/src/utils/serverSettingsPatch.ts deleted file mode 100644 index b7500c44b..000000000 --- a/clients/web/src/utils/serverSettingsPatch.ts +++ /dev/null @@ -1,55 +0,0 @@ -import type { InspectorServerSettings } from "@inspector/core/mcp/types.js"; -import type { KeyValuePair } from "./keyValuePairs"; - -/** - * Decide what `settings` node ServerConfigModal's submit should send for the - * custom headers it just collected (#1915). - * - * Two rules, both load-bearing: - * - * 1. **Only an edit carries the target's other settings forward.** The - * backend replaces the whole node when one is sent, so an edit must - * re-send the fields the modal doesn't expose (metadata, timeouts, OAuth - * credentials, roots) or they'd be dropped. An **add or clone must not**: - * the modal's target in clone mode is the *source* server, so spreading it - * would copy that server's OAuth client secret and behavior flags onto a - * new entry the user only gave a URL and some headers. - * 2. **`undefined` means "don't send the key at all", and that is the - * default.** Omitting `settings` makes the backend preserve the node it - * already has, which matters beyond convenience: what this modal holds is - * a snapshot taken when it opened, so writing it back on a save that did - * not touch a header would overwrite a metadata or OAuth change made in - * the settings form in the meantime. So the patch is sent only when the - * submitted headers actually differ from the stored ones — including the - * case where the last one was cleared, which does need the node rewritten. - * - * Lives here rather than inline in App.tsx so this seam is unit-testable: - * App.tsx is outside the coverage gate, and the edit-vs-clone distinction is - * exactly where credentials leaked before. - * - * @param emptySettings the app's blank settings shape, used as the base for an - * add or clone. - */ -export function buildHeaderSettingsPatch( - // Spelled as a literal union rather than imported from ServerConfigModal: - // `utils` must not depend on `components`. It stays honest because - // `ServerConfigModalMode` is checked against it at the call site — adding a - // fourth mode there is a compile error here. - mode: "add" | "edit" | "clone", - existingSettings: InspectorServerSettings | undefined, - headers: KeyValuePair[], - emptySettings: InspectorServerSettings, -): InspectorServerSettings | undefined { - const existing = mode === "edit" ? existingSettings : undefined; - if (sameHeaders(existing?.headers ?? [], headers)) return undefined; - return { ...(existing ?? emptySettings), headers }; -} - -/** - * Order-sensitive pair-list equality. Order matters because it is what the - * form round-trips and what the user sees, so a reorder is a real edit. - */ -function sameHeaders(a: KeyValuePair[], b: KeyValuePair[]): boolean { - if (a.length !== b.length) return false; - return a.every((h, i) => h.key === b[i]?.key && h.value === b[i]?.value); -} diff --git a/core/react/useServers.ts b/core/react/useServers.ts index f206a0d59..3a2a50028 100644 --- a/core/react/useServers.ts +++ b/core/react/useServers.ts @@ -29,28 +29,11 @@ export interface UseServersResult { loading: boolean; error: string | undefined; refresh: () => 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. - */ + addServer: (id: string, config: MCPServerConfig) => Promise; updateServer: ( originalId: string, newId: string, config: MCPServerConfig, - settings?: InspectorServerSettings, ) => Promise; /** * Patch only the `settings` node on an existing server entry, leaving the @@ -219,17 +202,11 @@ export function useServers(opts: UseServersOptions): UseServersResult { }, [base, authToken, doFetch, refreshInternal]); const addServer = useCallback( - async ( - id: string, - config: MCPServerConfig, - settings?: InspectorServerSettings, - ): Promise => { + async (id: string, config: MCPServerConfig): Promise => { const res = await doFetch(`${base}/api/servers`, { method: "POST", headers: buildHeaders(authToken, true), - // `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 }), + body: JSON.stringify({ id, config }), }); if (!res.ok) { throw new Error(await readErrorMessage(res)); @@ -258,21 +235,18 @@ export function useServers(opts: UseServersOptions): UseServersResult { originalId: string, newId: string, config: MCPServerConfig, - settings?: InspectorServerSettings, ): Promise => { - // 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, which replaces the - // node wholesale — so it must carry forward the fields it isn't editing. + // `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`. const res = await doFetch( `${base}/api/servers/${encodeURIComponent(originalId)}`, { method: "PUT", headers: buildHeaders(authToken, true), - body: JSON.stringify({ id: newId, config, settings }), + body: JSON.stringify({ id: newId, config }), }, ); if (!res.ok) {