diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx index 09ded1716..7ea0e3963 100644 --- 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"); diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx index b2a786c64..18f0c5222 100644 --- 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) => ( - - 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 - - ))} + {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 + + + ); + })} ); } @@ -651,6 +682,7 @@ export function ServerSettingsForm({ ) : ( @@ -666,9 +698,11 @@ export function ServerSettingsForm({ - 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. + Add Header @@ -677,6 +711,7 @@ export function ServerSettingsForm({ ) : ( @@ -700,6 +735,7 @@ export function ServerSettingsForm({ ) : ( diff --git a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx index 1e14d6415..e627beb74 100644 --- 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: [],