From 217b0f41d815a6fc6f88fcbd8b84b063954446c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Fri, 7 Aug 2026 03:55:15 +0200 Subject: [PATCH] fix: say what is wrong with a field where somebody will read it - a refused field gets its own announced node in content contrast; the standing hint stays muted beside it, rather than being overwritten - the timezone refusal is the only explanation for a submit button that will not press, so it leaves the picker's muted hint slot --- .../managed-profile-affordances.test.tsx | 143 +++++++++++++++++- .../access/managed-profile-create-form.tsx | 121 ++++++++++++--- 2 files changed, 232 insertions(+), 32 deletions(-) diff --git a/src/components/settings/access/__tests__/managed-profile-affordances.test.tsx b/src/components/settings/access/__tests__/managed-profile-affordances.test.tsx index 77020d4ca..f75482bf7 100644 --- a/src/components/settings/access/__tests__/managed-profile-affordances.test.tsx +++ b/src/components/settings/access/__tests__/managed-profile-affordances.test.tsx @@ -100,10 +100,17 @@ vi.mock("@/lib/queries/use-account-grants", async (importOriginal) => { }; }); -/** The roster read, as each profile row sees it. */ -const rosterRef: { value: ManagedProfileGuardian[] | undefined } = { - value: [], -}; +/** + * The roster read, as each profile row sees it. + * + * Three states, not two. `"loading"` and `"error"` used to share `undefined`, + * which is precisely the conflation the error branch exists to end: a failed + * read that renders as a slow one is a person waiting for something that is + * never coming. + */ +const rosterRef: { + value: ManagedProfileGuardian[] | "loading" | "error"; +} = { value: [] }; /** The deletion mutation, idle unless a case says otherwise. */ const deleteRef = { value: idle() }; /** The guardian invitation, idle unless a case says otherwise. */ @@ -125,9 +132,9 @@ vi.mock("@/lib/queries/use-managed-profiles", async (importOriginal) => { useInviteManagedProfileGuardian: () => inviteRef.value, useRemoveManagedProfileGuardian: () => removeGuardianRef.value, useManagedProfileGuardians: () => ({ - data: rosterRef.value, - isLoading: false, - isError: rosterRef.value === undefined, + data: Array.isArray(rosterRef.value) ? rosterRef.value : undefined, + isLoading: rosterRef.value === "loading", + isError: rosterRef.value === "error", refetch: vi.fn(), }), }; @@ -177,6 +184,7 @@ import { RecordSettingsSectionGate } from "@/components/settings/record-settings import { createManagedProfileErrorKey, displayNameIssue, + FieldGuidance, } from "../managed-profile-create-form"; import { activeGuardianCount, @@ -324,6 +332,80 @@ describe("who can reach the section at all", () => { }); }); +describe("how a field says what is wrong with it", () => { + /** + * The form's invalid states are not reachable from a static render — there + * is no click, so nothing is ever invalid — which is why the two nodes were + * pulled out into a pure component. These legs are the whole of the claim + * the HIGH finding was about: a refusal is content and it is announced. + */ + it("renders the hint as muted meta and nothing else when the field is fine", () => { + const html = render( + , + ); + expect(html).toContain('data-slot="f-hint"'); + expect(html).toContain("text-muted-foreground"); + expect(html).not.toContain('data-slot="f-error"'); + expect(html).not.toContain('role="alert"'); + }); + + it("announces the refusal, in content contrast, on its own node", () => { + const html = render( + , + ); + const alert = /]*data-slot="f-error"[^>]*>/.exec(html)?.[0]; + expect(alert).toBeDefined(); + // Announced, because it MOUNTS with the role rather than a class being + // swapped onto a node that was already there. + expect(alert).toContain('role="alert"'); + // Content, not meta. UI-STANDARDS §3, and the same reasoning + // `grant-action-error.tsx` spells out: a refusal rendered as muted is a + // refusal nobody reads. + expect(alert).toContain("text-destructive"); + expect(alert).not.toContain("text-muted-foreground"); + // And the hint survives beside it rather than being overwritten, so the + // bound the person is being held to is still on screen. + expect(html).toContain('data-slot="f-hint"'); + expect(html).toContain("Up to 80 characters."); + }); + + it("renders the error alone for a control that brings its own hint", () => { + // The timezone picker labels itself; the form owes only the refusal, and + // that refusal is the only explanation for a submit button that will not + // press. + const html = render( + , + ); + expect(html).toContain('data-slot="tz-error"'); + expect(html).toContain('role="alert"'); + expect(html).not.toContain('data-slot="tz-hint"'); + }); + + it("keeps the standing hint muted on the rendered form", () => { + // The other half, on the real component: the default state is guidance, + // and guidance IS meta. + authRef.value = { accounts: [], active: null }; + const html = renderGatedSection(); + const hint = /]*data-slot="managed-profile-name-hint"[^>]*>/.exec( + html, + )?.[0]; + expect(hint).toBeDefined(); + expect(hint).toContain("text-muted-foreground"); + expect(html).not.toContain('data-slot="managed-profile-name-error"'); + expect(html).not.toContain('data-slot="managed-profile-timezone-error"'); + }); +}); + describe("what the form decides before it sends", () => { it("treats a whitespace-only name as empty, because the route does", () => { // The route trims and then requires 1..80. A client that did not trim @@ -415,7 +497,7 @@ describe("ending a managed profile", () => { // the sentence's presence at the mercy of which request landed first — // the defect the retention disclosure already paid for once. authRef.value = { accounts: [entry()], active: null }; - rosterRef.value = undefined; + rosterRef.value = "loading"; const html = renderGatedSection(); expect(html).toContain('data-managed-profile-id="p1"'); expect(html).not.toContain('data-slot="managed-profile-delete"'); @@ -452,6 +534,51 @@ describe("ending a managed profile", () => { 'data-slot="grant-action-error"', ); }); + + it("names the step-up gate rather than reporting a failure", () => { + // The likeliest refusal of all, and the one that read as a defect. Both + // destructive acts sit behind a `ConfirmButton`, so the request that + // reaches the server is the second click — minutes after the panel was + // opened, and easily outside the five-minute window. + authRef.value = { accounts: [entry()], active: null }; + deleteRef.value = failed( + new ApiError("refused", 401, { errorCode: "auth.stepup.required" }), + "p1", + ); + const html = renderGatedSection(); + expect(html).toContain("Confirm your second factor"); + expect(html).not.toContain("The profile could not be deleted"); + }); + + it("tells a guardian with no second factor where to get one", () => { + // `requireFreshMfa` refuses an unenrolled account with its own code, and + // "confirm your second factor" to somebody who has none is a dead end + // offered by the surface that owns the way out of it. + authRef.value = { accounts: [entry()], active: null }; + deleteRef.value = failed( + new ApiError("refused", 401, { + errorCode: "auth.stepup.mfa_not_enrolled", + }), + "p1", + ); + const html = renderGatedSection(); + expect(html).toContain("does not have one yet"); + expect(html).toContain("Security"); + }); + + it("says a failed roster read failed, rather than looking like a slow one", () => { + // Without this branch a 500 renders exactly as a still-loading roster — + // no panel, no delete control — and the person waits for something that is + // never coming. + authRef.value = { accounts: [entry()], active: null }; + rosterRef.value = "error"; + const html = renderGatedSection(); + expect(html).toContain('data-slot="query-error-card"'); + expect(html).toContain("Could not load who looks after this record"); + // And the way back is reachable, which is what UI-STANDARDS §6 asks of an + // error state. + expect(html).toContain('data-slot="query-error-retry"'); + }); }); describe("what the confirmation says before it deletes", () => { diff --git a/src/components/settings/access/managed-profile-create-form.tsx b/src/components/settings/access/managed-profile-create-form.tsx index d28fbf5fe..eb75007ad 100644 --- a/src/components/settings/access/managed-profile-create-form.tsx +++ b/src/components/settings/access/managed-profile-create-form.tsx @@ -8,6 +8,7 @@ import { Input } from "@/components/ui/input"; import { Label } from "@/components/ui/label"; import { NativeSelect } from "@/components/ui/native-select"; import { TimezonePicker } from "@/components/settings/timezone-picker"; +import { stepUpErrorKey } from "@/components/settings/access/grant-action-error"; import { ApiError } from "@/lib/api/api-fetch"; import { locales, localeLabels, type Locale } from "@/lib/i18n/config"; import { useTranslations } from "@/lib/i18n/context"; @@ -138,15 +139,16 @@ export function ManagedProfileCreateForm() { } aria-describedby="managed-profile-name-hint" /> -

- {displayName.length > 0 && nameIssue !== null - ? t(nameIssue) - : t("recordSharing.managed.nameHint")} -

+ 0 && nameIssue !== null ? t(nameIssue) : null + } + />
@@ -191,20 +193,28 @@ export function ManagedProfileCreateForm() {

- { - setTimezone(next); - setError(null); - }} - // The picker's own list comes from the engine, so the select branch - // cannot produce an unknown zone. Its free-text fallback (older - // engines without `Intl.supportedValuesOf`) can, and the submit button - // is inert for one — so the hint slot says why rather than leaving a - // dead button with no explanation beside it. - hint={t(timezoneIssue ?? "recordSharing.managed.timezoneHint")} - /> +
+ { + setTimezone(next); + setError(null); + }} + hint={t("recordSharing.managed.timezoneHint")} + /> + {/* The picker's own list comes from the engine, so the select branch + cannot produce an unknown zone. Its free-text fallback (older + engines without `Intl.supportedValuesOf`) can, and this sentence is + then the ONLY explanation for a submit button that will not press. + It rides beside the picker rather than through its `hint` prop, + because that prop renders muted meta and a refusal rendered as meta + is a refusal nobody reads. */} + +