mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-10 05:12:14 +00:00
fix(settings): close a confirmation when its request settles
The confirming click on both alert dialogs in the TOTP card calls preventDefault(), which is what keeps the dialog up and its button disabled while the request is in flight. Nothing took the dialog down again, so a refused rotation left the confirmation open over the card, and the card is the only place the refusal explains itself: the step-up message painted behind the overlay, inside the aria-hidden region the modal creates, with the confirm button live again. A success hid it, because the fresh-codes panel replaces the branch the dialog lives in and it unmounted anyway. Both dialogs are controlled now, and each mutation closes its own when the request settles, whichever way it went. The disable arm carried the same shape and a mistyped six-digit code there is routine, so it is fixed with it. The card test pins the sentence a step-up refusal shows and the wiring that takes the dialog down; what a person sees is asserted against a real browser in e2e/mfa-step-up-journey.spec.ts.
This commit is contained in:
1 parent
50a65b6f80
commit
36410831ed
2 files changed
+98
-8
No files matched your search
@@ -8,6 +8,26 @@
|
||||
* Project convention is SSR-only tests (no `@testing-library/react`); the
|
||||
* dialog's open-state behaviour is Radix's own, so we assert the source wires
|
||||
* the trigger + consequence copy rather than driving a portal at runtime.
|
||||
*
|
||||
* ## The refused mutation
|
||||
*
|
||||
* A refusal used to leave the confirmation up: the confirming click
|
||||
* `preventDefault()`s Radix's close so the request can stay in flight behind a
|
||||
* disabled button, and nothing took the dialog down again. On success that was
|
||||
* invisible — the fresh-codes panel replaces the branch the dialog lives in,
|
||||
* so it unmounted anyway — but a refused rotation (or a mistyped code at
|
||||
* disable) left an overlay covering the one place the card explains itself.
|
||||
*
|
||||
* Two halves are pinned below: the sentence a step-up refusal puts on the
|
||||
* card, called directly; and the wiring that takes the dialog down when the
|
||||
* request settles, read off the source. The second half is structural because
|
||||
* the DOM is out of reach here — what a person actually sees, a closed dialog
|
||||
* over a visible message, is asserted in `e2e/mfa-step-up-journey.spec.ts`
|
||||
* against a real browser.
|
||||
*
|
||||
* Mutation check: drop either `onSettled` and the wiring case goes red; drop
|
||||
* an `open=` binding and the controlled case goes red; return `err.message`
|
||||
* from `describeError`'s step-up arm and the sentence case goes red.
|
||||
*/
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { readFileSync } from "node:fs";
|
||||
@@ -15,8 +35,12 @@ import { resolve } from "node:path";
|
||||
import { renderToStaticMarkup } from "react-dom/server";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
|
||||
import { ApiError } from "@/lib/api/api-fetch";
|
||||
import { I18nProvider } from "@/lib/i18n/context";
|
||||
import { TotpCard } from "../totp-card";
|
||||
import en from "@/../messages/en.json";
|
||||
import { describeError, TotpCard } from "../totp-card";
|
||||
|
||||
const STEP_UP_SENTENCE = en.settings.security.stepUpRequired;
|
||||
|
||||
function render(node: React.ReactNode) {
|
||||
const client = new QueryClient({
|
||||
@@ -29,6 +53,10 @@ function render(node: React.ReactNode) {
|
||||
);
|
||||
}
|
||||
|
||||
function source(): string {
|
||||
return readFileSync(resolve(__dirname, "../totp-card.tsx"), "utf8");
|
||||
}
|
||||
|
||||
describe("<TotpCard> — recovery-code regeneration", () => {
|
||||
it("SSRs the enabled card with the regenerate action", () => {
|
||||
const html = render(<TotpCard enabled recoveryCodesRemaining={3} />);
|
||||
@@ -36,7 +64,7 @@ describe("<TotpCard> — recovery-code regeneration", () => {
|
||||
});
|
||||
|
||||
it("wraps regenerate in a confirmation dialog with a consequence line", () => {
|
||||
const src = readFileSync(resolve(__dirname, "../totp-card.tsx"), "utf8");
|
||||
const src = source();
|
||||
// The regenerate action is a dialog trigger, not a bare mutate-on-click.
|
||||
expect(src).toContain("settings.security.recovery.regenerateTitle");
|
||||
expect(src).toContain("settings.security.recovery.regenerateConfirm");
|
||||
@@ -44,3 +72,39 @@ describe("<TotpCard> — recovery-code regeneration", () => {
|
||||
expect(src).toContain("regenerate.mutate()");
|
||||
});
|
||||
});
|
||||
|
||||
describe("<TotpCard> — a refused mutation", () => {
|
||||
it("puts the step-up sentence on the card, not the server's prose", () => {
|
||||
const refusal = new ApiError(
|
||||
"Recent second-factor verification required",
|
||||
401,
|
||||
{ errorCode: "auth.stepup.required" },
|
||||
);
|
||||
|
||||
const shown = describeError(refusal, "fallback", STEP_UP_SENTENCE);
|
||||
|
||||
expect(shown).toBe(STEP_UP_SENTENCE);
|
||||
expect(shown).not.toContain("second-factor verification required");
|
||||
});
|
||||
|
||||
it("closes each confirmation when its request settles", () => {
|
||||
const src = source();
|
||||
// Whatever the answer was — the refusal is the case that needs it, and a
|
||||
// handler that only closed on success would leave the refusal covered.
|
||||
expect(src).toContain("onSettled: () => setRegenerateOpen(false)");
|
||||
expect(src).toContain("onSettled: () => setDisableOpen(false)");
|
||||
});
|
||||
|
||||
it("leaves no confirmation for Radix to close on its own", () => {
|
||||
const src = source();
|
||||
// Every dialog on this card is controlled, so none of them can be left
|
||||
// holding an overlay after a `preventDefault()`ed confirm.
|
||||
const dialogs = src.match(/<AlertDialog[\s>]/g);
|
||||
const controlled = src.match(/<AlertDialog\s+open=\{/g);
|
||||
expect(dialogs).toHaveLength(2);
|
||||
expect(controlled).toHaveLength(dialogs?.length ?? 0);
|
||||
// And the confirming click still holds the dialog open while the request
|
||||
// is in flight — that is what `onSettled` exists to release.
|
||||
expect(src).toContain("e.preventDefault()");
|
||||
});
|
||||
});
|
||||
@@ -45,8 +45,13 @@ interface ConfirmResult {
|
||||
recoveryCodesRemaining: number;
|
||||
}
|
||||
|
||||
/** Map a step-up 401 to a clear re-login message; otherwise the raw message. */
|
||||
function describeError(
|
||||
/**
|
||||
* Map a step-up 401 to a clear re-login message; otherwise the raw message.
|
||||
*
|
||||
* Exported for the unit suite: this is the sentence a refused mutation leaves
|
||||
* on the card, and the card is where a person reads it.
|
||||
*/
|
||||
export function describeError(
|
||||
err: unknown,
|
||||
fallback: string,
|
||||
stepUpMsg: string,
|
||||
@@ -89,6 +94,22 @@ export function TotpCard({
|
||||
"totp",
|
||||
);
|
||||
|
||||
// Both confirmations are controlled rather than left to Radix.
|
||||
//
|
||||
// The confirming click has to `preventDefault()` — that is what keeps the
|
||||
// dialog up, and the button disabled, while the request is in flight. But
|
||||
// suppressing the close means nothing closes it afterwards either, and on a
|
||||
// REFUSAL the branch below survives the answer: the dialog stayed open over
|
||||
// a card whose message the overlay covered, with the confirm button live
|
||||
// again, so the same refusal could be re-triggered forever with no way to
|
||||
// read why. On success it went unnoticed only because the fresh-codes panel
|
||||
// replaces this branch and the dialog unmounts with it.
|
||||
//
|
||||
// So each mutation puts its own dialog down when the request settles,
|
||||
// whatever the answer was, and the message lands on an uncovered card.
|
||||
const [regenerateOpen, setRegenerateOpen] = useState(false);
|
||||
const [disableOpen, setDisableOpen] = useState(false);
|
||||
|
||||
function resetWizard() {
|
||||
setSetup(null);
|
||||
setQrDataUrl(null);
|
||||
@@ -162,6 +183,7 @@ export function TotpCard({
|
||||
t("settings.security.stepUpRequired"),
|
||||
),
|
||||
),
|
||||
onSettled: () => setDisableOpen(false),
|
||||
});
|
||||
|
||||
const regenerate = useMutation({
|
||||
@@ -183,6 +205,7 @@ export function TotpCard({
|
||||
t("settings.security.stepUpRequired"),
|
||||
),
|
||||
),
|
||||
onSettled: () => setRegenerateOpen(false),
|
||||
});
|
||||
|
||||
async function copySecret() {
|
||||
@@ -356,7 +379,10 @@ export function TotpCard({
|
||||
)}
|
||||
</p>
|
||||
<div className="flex flex-wrap gap-2">
|
||||
<AlertDialog>
|
||||
<AlertDialog
|
||||
open={regenerateOpen}
|
||||
onOpenChange={setRegenerateOpen}
|
||||
>
|
||||
<AlertDialogTrigger asChild>
|
||||
<Button
|
||||
type="button"
|
||||
@@ -371,7 +397,7 @@ export function TotpCard({
|
||||
{t("settings.security.recovery.regenerate")}
|
||||
</Button>
|
||||
</AlertDialogTrigger>
|
||||
<AlertDialogContent>
|
||||
<AlertDialogContent data-testid="recovery-regenerate-dialog">
|
||||
<AlertDialogHeader>
|
||||
<AlertDialogTitle>
|
||||
{t("settings.security.recovery.regenerateTitle")}
|
||||
@@ -400,7 +426,7 @@ export function TotpCard({
|
||||
</AlertDialogContent>
|
||||
</AlertDialog>
|
||||
|
||||
<AlertDialog>
|
||||
<AlertDialog open={disableOpen} onOpenChange={setDisableOpen}>
|
||||
<AlertDialogTrigger asChild>
|
||||
<Button
|
||||
type="button"
|
||||
@@ -410,7 +436,7 @@ export function TotpCard({
|
||||
{t("settings.security.totp.disable")}
|
||||
</Button>
|
||||
</AlertDialogTrigger>
|
||||
<AlertDialogContent>
|
||||
<AlertDialogContent data-testid="totp-disable-dialog">
|
||||
<AlertDialogHeader>
|
||||
<AlertDialogTitle>
|
||||
{t("settings.security.totp.disableTitle")}
|
||||
|
||||
Reference in new issue
Block a user