From 55734f37764420c01cbd17fd2fb7f5e951d6d953 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 21:28:59 -0400 Subject: [PATCH] fix: accessible names for the authorization-parameter rows (#2018) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot round 2 (suppressed comments). Five of the seven findings were the accessibility of the new row list and one stray cast; all are addressed here. - The reserved-key reason is now the key input's `error` **string** rather than a bare boolean plus free-standing text. Mantine renders it in the input's own error slot and wires the `aria-describedby` association, so a screen reader learns *why* the field is invalid; a boolean only set `aria-invalid`. This also deletes the duplicate `ReservedParamError` text and its Stack wrapper. - Each control carries a per-row `aria-label` — the section heading and the placeholders are not programmatically associated with the inputs, so the key box, the value box, and a remove button announcing only "X" were indistinguishable to assistive technology. The label names the row by its key, falling back to "row N" while the key is still blank. - Dropped an unnecessary `as unknown as Record` in the servers-route integration test: `MCPConfig.mcpServers` is already typed as `Record`, so the entry's `oauth` reads directly. Three tests cover the new behavior (accessible names, the blank-key row-number fallback, and the aria-describedby association), each verified to fail against the pre-fix component. No change to the reserved-key rule or the EMA boundary. Signed-off-by: cliffhall --- .../ServerSettingsForm.test.tsx | 77 +++++++++++++++++++ .../ServerSettingsForm/ServerSettingsForm.tsx | 46 ++++++----- .../mcp/remote/servers-route.test.ts | 5 +- 3 files changed, 105 insertions(+), 23 deletions(-) diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx index b66d76ac..a9ff4f5d 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx @@ -826,6 +826,83 @@ describe("ServerSettingsForm", () => { ); }); + // #2018 — the section heading and the placeholders are not programmatically + // associated with these inputs, so each control carries its own aria-label and + // the reserved-key reason is the input's `error` string (which Mantine wires + // to the input via aria-describedby) rather than free-standing text. + it("gives each authorization-parameter control an accessible name", () => { + renderWithMantine( + , + ); + expect( + screen.getByRole("textbox", { + name: "Authorization parameter name, kc_idp_hint", + }), + ).toHaveValue("kc_idp_hint"); + expect( + screen.getByRole("textbox", { + name: "Authorization parameter value, kc_idp_hint", + }), + ).toHaveValue("corp"); + expect( + screen.getByRole("button", { + name: "Remove authorization parameter kc_idp_hint", + }), + ).toBeInTheDocument(); + }); + + it("falls back to a row number in the accessible name when the key is blank", () => { + renderWithMantine( + , + ); + expect( + screen.getByRole("textbox", { + name: "Authorization parameter name, row 1", + }), + ).toBeInTheDocument(); + }); + + // The reason must reach assistive tech through the input, not just as text on + // the page: Mantine emits the `error` string with an id the input references. + it("associates the reserved-key reason with the key input", () => { + renderWithMantine( + , + ); + const keyInput = screen.getByRole("textbox", { + name: "Authorization parameter name, state", + }); + const describedBy = keyInput.getAttribute("aria-describedby"); + expect(describedBy).toBeTruthy(); + const reason = describedBy + ?.split(" ") + .map((id) => document.getElementById(id)?.textContent ?? "") + .join(" "); + expect(reason).toContain( + '"state" is set by the authorization flow and cannot be overridden.', + ); + }); + it("does not flag a blank authorization-parameter row as reserved", () => { renderWithMantine( - {params.map((param, index) => ( - - + {params.map((param, index) => { + const rowLabel = param.key.trim() || `row ${index + 1}`; + return ( + onChange(index, e.currentTarget.value, param.value) } @@ -301,6 +306,7 @@ function AuthorizationParamRows({ } /> @@ -312,15 +318,15 @@ function AuthorizationParamRows({ ) : null } /> - onRemove(index)}>X + onRemove(index)} + > + X + - {authorizationParamKeyError(param.key) ? ( - - {authorizationParamKeyError(param.key)} - - ) : null} - - ))} + ); + })} ); } diff --git a/clients/web/src/test/integration/mcp/remote/servers-route.test.ts b/clients/web/src/test/integration/mcp/remote/servers-route.test.ts index 9e0f8a5a..18803b5a 100644 --- a/clients/web/src/test/integration/mcp/remote/servers-route.test.ts +++ b/clients/web/src/test/integration/mcp/remote/servers-route.test.ts @@ -471,9 +471,8 @@ describe("/api/servers routes", () => { }), }); expect(res.status).toBe(200); - const stored = readConfig(h.configPath).mcpServers - .authparams as unknown as Record; - expect(stored.oauth).toEqual({ + const stored = readConfig(h.configPath).mcpServers.authparams; + expect(stored?.oauth).toEqual({ authorizationParams: { kc_idp_hint: "corp" }, }); });