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
This commit is contained in:
Marc-André Bombeck committed 2026-08-07 03:55:15 +02:00
1 parent e9084d9f17
commit 217b0f41d8
2 files changed
+232 -32

No files matched your search

@@ -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(
<FieldGuidance
slot="f"
hintId="f-hint"
hint="Up to 80 characters."
error={null}
/>,
);
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(
<FieldGuidance
slot="f"
hintId="f-hint"
hint="Up to 80 characters."
error="That name is longer than 80 characters."
/>,
);
const alert = /<p[^>]*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(
<FieldGuidance slot="tz" error="This browser does not know that zone." />,
);
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 = /<p[^>]*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", () => {
@@ -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"
/>
<p
id="managed-profile-name-hint"
data-slot="managed-profile-name-hint"
className="text-muted-foreground text-xs"
>
{displayName.length > 0 && nameIssue !== null
? t(nameIssue)
: t("recordSharing.managed.nameHint")}
</p>
<FieldGuidance
slot="managed-profile-name"
hintId="managed-profile-name-hint"
hint={t("recordSharing.managed.nameHint")}
// Only once somebody has typed. "Give the record a name" on an empty
// field they have not reached yet is a telling-off, not guidance.
error={
displayName.length > 0 && nameIssue !== null ? t(nameIssue) : null
}
/>
</div>
<div className="space-y-1.5">
@@ -191,20 +193,28 @@ export function ManagedProfileCreateForm() {
</p>
</div>
<TimezonePicker
id="managed-profile-timezone"
value={timezone}
onChange={(next) => {
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")}
/>
<div className="space-y-1.5">
<TimezonePicker
id="managed-profile-timezone"
value={timezone}
onChange={(next) => {
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. */}
<FieldGuidance
slot="managed-profile-timezone"
error={timezoneIssue === null ? null : t(timezoneIssue)}
/>
</div>
<Button
type="submit"
@@ -238,6 +248,65 @@ export function ManagedProfileCreateForm() {
);
}
/**
* A field's standing hint, and its refusal when there is one.
*
* Two nodes, never one that swaps. The form used to render a single muted
* `<p>` whose TEXT changed from hint to error, which is wrong twice over:
*
* * a refusal is content somebody has to act on, and UI-STANDARDS §3
* reserves `text-muted-foreground` for meta. The same reasoning is written
* out at `grant-action-error.tsx` — a refusal rendered as meta is a
* refusal nobody reads;
* * a node whose text changes in place is not announced. `role="alert"` is
* reliable when the node MOUNTS, which is why the panel's own alert is a
* separate element rather than a class on an existing one. Somebody who
* cannot see the screen pressed a button and got nothing, again.
*
* The hint stays where it was, muted, and keeps the id `aria-describedby`
* points at. `hint` is optional for the one caller whose control renders its
* own (the timezone picker), which needs only the error half.
*
* Exported and pure because the form's error states are not reachable from a
* static render — there is no click, so no field is ever invalid — and a
* rendering nobody can test is a rendering that drifts back to muted.
*/
export function FieldGuidance({
slot,
hintId,
hint,
error,
}: {
/** `data-slot` stem: `<slot>-hint` and `<slot>-error`. */
slot: string;
hintId?: string;
hint?: string;
error: string | null;
}) {
return (
<>
{hint !== undefined && (
<p
id={hintId}
data-slot={`${slot}-hint`}
className="text-muted-foreground text-xs"
>
{hint}
</p>
)}
{error !== null && (
<p
data-slot={`${slot}-error`}
role="alert"
className="text-destructive text-sm"
>
{error}
</p>
)}
</>
);
}
/** The browser's zone, or the app default when the engine will not say. */
function browserTimezone(): string {
const detected = detectBrowserTimezone();
@@ -281,7 +350,11 @@ export function createManagedProfileErrorKey(err: unknown): string {
// The step-up gate. `requireFreshMfa` throws `StepUpRequiredError`, which is
// a 401 carrying a stable code, and the request was authenticated enough to
// reach it — so this is never a lapsed session.
if (err.status === 401) return "recordSharing.managed.errorStepUp";
if (err.status === 401) return stepUpErrorKey(err);
if (err.status === 422) return "recordSharing.managed.errorInvalid";
// Ten an hour, the same ceiling the guardian invitation carries. Waiting is
// the whole recovery, so it must not read as a failure — and it carries no
// `errorCode`, like every other rate-limit refusal in this family.
if (err.status === 429) return "recordSharing.managed.errorRateLimit";
return "recordSharing.managed.errorFailed";
}