Merge pull request #2038 from modelcontextprotocol/v2/feat/1915-manual-server-headers

fix(web): correct the Custom Headers OAuth hint, name key/value rows for a11y (#2040)
2026-08-16 23:36:32 -04:00 · 2026-08-16 23:36:32 -04:00 · e9a581d912
commit e9a581d912
parent 1e0fd88956 4d7405bea0
3 changed files with 107 additions and 49 deletions
--- 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, row 1"), 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 = {
@ -329,8 +332,11 @@ 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, row 1",
+      }),
+    );
    expect(onRemoveHeader).toHaveBeenCalledWith(0);
  });

@ -351,8 +357,11 @@ 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, row 1",
+      }),
+    );
    expect(onRemoveMetadata).toHaveBeenCalledWith(0);
  });

@ -676,9 +685,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, row 1",
+        }),
+      );
      expect(onRemoveEnv).toHaveBeenCalledWith(0);
    });
  });
@ -1489,12 +1502,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();
  });

@ -1517,12 +1530,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");
--- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx
+++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx
@ -226,10 +226,22 @@ const ClearStoredOAuthHint = Text.withProps({

 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;
 }) {
@ -242,31 +254,50 @@ function KeyValueRows({

  return (
    <>
-      {items.map((item, index) => (
-        <Group key={index} grow>
-          <ClearableTextInput
-            placeholder="Key"
-            value={item.key}
-            onChange={(e) => onChange(index, e.currentTarget.value, item.value)}
-            rightSection={
-              item.key ? (
-                <ClearButton onClick={() => onChange(index, "", item.value)} />
-              ) : null
-            }
-          />
-          <ClearableTextInput
-            placeholder="Value"
-            value={item.value}
-            onChange={(e) => onChange(index, item.key, e.currentTarget.value)}
-            rightSection={
-              item.value ? (
-                <ClearButton onClick={() => onChange(index, item.key, "")} />
-              ) : null
-            }
-          />
-          <RemoveIcon onClick={() => onRemove(index)}>X</RemoveIcon>
-        </Group>
-      ))}
+      {items.map((item, index) => {
+        const key = item.key.trim();
+        const rowLabel = key ? `${key}, row ${index + 1}` : `row ${index + 1}`;
+        return (
+          <Group key={index} grow>
+            <ClearableTextInput
+              placeholder="Key"
+              aria-label={`${entityLabel} name, ${rowLabel}`}
+              value={item.key}
+              onChange={(e) =>
+                onChange(index, e.currentTarget.value, item.value)
+              }
+              rightSection={
+                item.key ? (
+                  <ClearButton
+                    aria-label={`Clear ${entityLabel} name, ${rowLabel}`}
+                    onClick={() => onChange(index, "", item.value)}
+                  />
+                ) : null
+              }
+            />
+            <ClearableTextInput
+              placeholder="Value"
+              aria-label={`${entityLabel} value, ${rowLabel}`}
+              value={item.value}
+              onChange={(e) => onChange(index, item.key, e.currentTarget.value)}
+              rightSection={
+                item.value ? (
+                  <ClearButton
+                    aria-label={`Clear ${entityLabel} value, ${rowLabel}`}
+                    onClick={() => onChange(index, item.key, "")}
+                  />
+                ) : null
+              }
+            />
+            <RemoveIcon
+              aria-label={`Remove ${entityLabel}, ${rowLabel}`}
+              onClick={() => onRemove(index)}
+            >
+              X
+            </RemoveIcon>
+          </Group>
+        );
+      })}
    </>
  );
 }
@ -651,6 +682,7 @@ export function ServerSettingsForm({
              ) : (
                <KeyValueRows
                  items={settings.env}
+                  entityLabel="environment variable"
                  onChange={onEnvChange}
                  onRemove={onRemoveEnv}
                />
@ -666,9 +698,11 @@ export function ServerSettingsForm({
          <Stack gap="md">
            <Group justify="space-between">
              <HintText>
-                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.
              </HintText>
              <AddButton onClick={onAddHeader}>+ Add Header</AddButton>
            </Group>
@ -677,6 +711,7 @@ export function ServerSettingsForm({
            ) : (
              <KeyValueRows
                items={settings.headers}
+                entityLabel="header"
                onChange={onHeaderChange}
                onRemove={onRemoveHeader}
              />
@ -700,6 +735,7 @@ export function ServerSettingsForm({
            ) : (
              <KeyValueRows
                items={settings.metadata}
+                entityLabel="metadata entry"
                onChange={onMetadataChange}
                onRemove={onRemoveMetadata}
              />
--- a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx
+++ b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx
@ -276,8 +276,11 @@ 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, row 1",
+      }),
+    );
    expect(onSettingsChange).toHaveBeenCalledWith({
      ...initialSettings,
      headers: [],
@ -342,10 +345,13 @@ 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, row 1",
+      }),
+    );
    expect(onSettingsChange).toHaveBeenCalledWith({
      ...initialSettings,
      metadata: [],
@ -810,8 +816,11 @@ 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, row 1",
+        }),
+      );
      expect(onSettingsChange).toHaveBeenCalledWith({
        ...emptySettings,
        env: [],