fix(web): only write settings on a real header edit; label rows by position (#1915)

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

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: "" }] },

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

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 (
           <Group key={index} grow>
             <ClearableTextInput

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

@@ -159,7 +159,7 @@ export const EditSseWithHeaders: Story = {
     const body = within(canvasElement.ownerDocument.body);
     const dialog = within(await findDialog(body, "Edit server"));
     const value = (await dialog.findByRole("textbox", {
-      name: "header value, Cookie",
+      name: "header value, Cookie, row 1",
     })) as HTMLInputElement;
     await expect(value.value).toBe("branch=feature-x");
 

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

@@ -509,7 +509,7 @@ describe("ServerConfigModal", () => {
           "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$/ }));
 

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

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,

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,

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`
+ * 2. **`undefined` means "don't send the key at all", and that is the
- *    is what makes the backend preserve the node it already has, so with no
+ *    default.** Omitting `settings` makes the backend preserve the node it
- *    headers on either side there is nothing to write and nothing to clear.
+ *    already has, which matters beyond convenience: what this modal holds is
- *    Clearing the last header still sends — `existing` had headers, so the
+ *    a snapshot taken when it opened, so writing it back on a save that did
- *    node must be rewritten without them.
+ *    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 =
+  if (sameHeaders(existing?.headers ?? [], headers)) return undefined;
-    headers.length > 0 || (existing?.headers.length ?? 0) > 0;
-  if (!changesSomething) 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);
+}