diff --git a/clients/web/src/components/elements/KeyValueRows/KeyValueRows.stories.tsx b/clients/web/src/components/elements/KeyValueRows/KeyValueRows.stories.tsx index 05c5ca14..055a148b 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 09fceca7..60e6916d 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 cf03e8e1..9a44585d 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 8bc001c7..d313c00a 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 8c29c418..a5dbcb0d 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 12d7c081..06c892cf 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 b2b51a81..25fe7709 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); +}