From 4dd8ed4498c4b53313a651fc3b7ec66624d41166 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 22:57:34 -0400 Subject: [PATCH] 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 a32ecb90..72d8bd1f 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 055a148b..00000000 --- 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 60e6916d..00000000 --- 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 776ce5e9..00000000 --- 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 d44cd75e..c180a95b 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 0a3416c6..2f6f4e40 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 105bb121..e1e823a6 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 5faf82c2..48e67c05 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 d66beb4c..bb95402d 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 2625e651..00000000 --- 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 06c892cf..00000000 --- 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 b7500c44..00000000 --- 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 f206a0d5..3a2a5002 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) {