fix(web): lock header rows while submitting, name their clear buttons (#1915)

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 <cliff@futurescale.com>

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).
+      // 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
-      // Only an EDIT carries the target's other settings forward. `settings`
+      // coverage gate and that distinction is where credentials can leak.
-      // replaces the whole node, so an edit must re-send the fields this modal
+      const settings = buildHeaderSettingsPatch(
-      // doesn't expose (metadata, timeouts, OAuth credentials, roots) or they
+        configModal?.mode ?? "add",
-      // would be dropped. A CLONE must not: `configModalTarget` there is the
+        configModalTarget?.settings,
-      // *source* server, and spreading it would copy that server's OAuth
+        headers,
-      // client secret and every behavior flag onto a new entry the user only
+        EMPTY_SETTINGS,
-      // 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;
       if (configModal?.mode === "edit" && configModal.targetId) {
         const originalId = configModal.targetId;
         await updateServer(originalId, id, config, settings);

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(
       <KeyValueRows
         items={items}
         entityLabel="header"
+        disabled={disabled}
         onChange={onChange}
         onRemove={onRemove}
       />,
@@ -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" });
+    // A bare "Clear" repeated per field would be indistinguishable across
-    await user.click(clearButtons[0]!);
+    // 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 () => {

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 ? (
                   <ClearButton
+                    aria-label={`Clear ${entityLabel} name, ${rowLabel}`}
+                    disabled={disabled}
                     onClick={() => 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 ? (
-                  <ClearButton onClick={() => onChange(index, item.key, "")} />
+                  <ClearButton
+                    aria-label={`Clear ${entityLabel} value, ${rowLabel}`}
+                    disabled={disabled}
+                    onClick={() => onChange(index, item.key, "")}
+                  />
                 ) : null
               }
             />
             <RemoveIcon
               aria-label={`Remove ${entityLabel}, ${rowLabel}`}
+              disabled={disabled}
               onClick={() => onRemove(index)}
             >
               X

clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx

@@ -504,6 +504,7 @@ export function ServerConfigModal({
                 <KeyValueRows
                   items={form.headers}
                   entityLabel="header"
+                  disabled={submitting}
                   onChange={changeHeader}
                   onRemove={removeHeader}
                 />

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");

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();
+  });
+});

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 };
+}