From 7cb2e9ed4fe3c72f5fa4a7dd6a73ac4fc635e309 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20H=C3=B6glund?= Date: Thu, 15 Oct 2026 14:16:35 +0200 Subject: [PATCH 1/4] Refactor Reverification component to take controller instead of spread --- packages/mosaic/src/blocks/confirm/confirm-shell.tsx | 2 +- .../__tests__/reverification.feature.test.tsx | 4 ++-- .../mosaic/src/features/reverification/reverification.tsx | 7 ++++--- .../user-profile-device-details.dialog.tsx | 2 +- .../(clerk)/live/reverification/reverification/page.tsx | 4 ++-- packages/swingset/src/stories/reverification.mdx | 4 ++-- 6 files changed, 12 insertions(+), 11 deletions(-) diff --git a/packages/mosaic/src/blocks/confirm/confirm-shell.tsx b/packages/mosaic/src/blocks/confirm/confirm-shell.tsx index ce53ff08276..9c8b094194e 100644 --- a/packages/mosaic/src/blocks/confirm/confirm-shell.tsx +++ b/packages/mosaic/src/blocks/confirm/confirm-shell.tsx @@ -88,7 +88,7 @@ function ConfirmCard({ {confirmStep} controller.onOpenChange(false)} /> diff --git a/packages/mosaic/src/features/reverification/__tests__/reverification.feature.test.tsx b/packages/mosaic/src/features/reverification/__tests__/reverification.feature.test.tsx index e3427b184e4..be976823958 100644 --- a/packages/mosaic/src/features/reverification/__tests__/reverification.feature.test.tsx +++ b/packages/mosaic/src/features/reverification/__tests__/reverification.feature.test.tsx @@ -142,7 +142,7 @@ function Host({ action }: { action: () => Promise }) { {outcome} {running ? ( - + {reverification.onCancel ? ( - {outcome} {String(reverification.visible)} @@ -1093,8 +1087,8 @@ describe('Reverification', () => { return renderWithClerk(); }; - it('keeps the last card on screen, inert, after a dismissal until it is reset', async () => { - await renderHolding(guardedAction()); + it('keeps the last card on screen, inert, after a dismissal, until the next challenge replaces it', async () => { + await renderHolding(vi.fn().mockResolvedValue(reverificationError())); const user = await startVerification(); await user.type(await screen.findByLabelText('Password'), 'half-typed'); @@ -1106,24 +1100,36 @@ describe('Reverification', () => { expect(passwordField()).toHaveAttribute('aria-disabled', 'true'); expect(passwordField()).toHaveAttribute('readonly'); - await user.click(screen.getByRole('button', { name: 'Reset' })); - expect(screen.queryByText(STEP.password)).toBeNull(); + await user.click(screen.getByRole('button', { name: 'Run action' })); + await waitFor(() => expect(passwordField()).toBeEnabled()); + expect(passwordField()).toHaveValue(''); + expect(screen.getByRole('button', { name: 'Dismiss' })).toBeInTheDocument(); }); - it('keeps the last card on screen after the action completes until the next run', async () => { - await renderHolding(guardedAction()); + it('keeps the last card on screen after the action completes, until the next challenge replaces it', async () => { + await renderHolding( + vi + .fn() + .mockResolvedValueOnce(reverificationError()) + .mockResolvedValueOnce({ done: true }) + .mockResolvedValueOnce(reverificationError()) + .mockResolvedValue({ done: true }), + ); const user = await startVerification(); await user.type(await screen.findByLabelText('Password'), `${PASSWORD}{Enter}`); await untilResolved(); expect(screen.getByText(STEP.password)).toBeVisible(); expect(screen.queryByRole('button', { name: 'Dismiss' })).toBeNull(); + expect(passwordField()).toHaveAttribute('aria-disabled', 'true'); await user.click(screen.getByRole('button', { name: 'Run action' })); - await waitFor(() => expect(screen.queryByText(STEP.password)).toBeNull()); + await waitFor(() => expect(passwordField()).toBeEnabled()); + expect(passwordField()).toHaveValue(''); + expect(screen.getByRole('button', { name: 'Dismiss' })).toBeInTheDocument(); }); - it('is visible once there is a card of its own to show, until it is reset', async () => { + it('is visible once there is a card of its own to show, until the run ends', async () => { await renderHolding(guardedAction()); const start = holdRequests('post', VERIFY); const visible = () => screen.getByLabelText('Visible').textContent; @@ -1140,10 +1146,8 @@ describe('Reverification', () => { await dismiss(user); await waitFor(() => expect(outcome()).toBe('rejected: reverification_cancelled')); - expect(visible()).toBe('true'); - - await user.click(screen.getByRole('button', { name: 'Reset' })); expect(visible()).toBe('false'); + expect(screen.getByText(STEP.password)).toBeVisible(); }); it('does not keep anything on screen when no reverification was needed', async () => { diff --git a/packages/mosaic/src/features/reverification/__tests__/use-reverification-with-state.test.tsx b/packages/mosaic/src/features/reverification/__tests__/use-reverification-with-state.test.tsx index 7a0760ee2eb..0448f4213b4 100644 --- a/packages/mosaic/src/features/reverification/__tests__/use-reverification-with-state.test.tsx +++ b/packages/mosaic/src/features/reverification/__tests__/use-reverification-with-state.test.tsx @@ -90,7 +90,7 @@ describe('useReverificationWithState', () => { expect(result.current[1]).toEqual({ phase: 'inactive' }); }); - it('moves inactive → active → retrying → settled when the retry succeeds', async () => { + it('moves inactive → active → retrying → inactive when the retry succeeds', async () => { const retry = deferred<{ ok: true }>(); const fetcher = vi .fn() @@ -118,11 +118,11 @@ describe('useReverificationWithState', () => { await act(async () => { await expect(pending).resolves.toEqual({ ok: true }); }); - await waitFor(() => expect(result.current[1]).toEqual({ phase: 'settled' })); + await waitFor(() => expect(result.current[1]).toEqual({ phase: 'inactive' })); expect(challengeCancel).not.toHaveBeenCalled(); }); - it('settles when the delayed retry fails', async () => { + it('returns to inactive when the delayed retry fails', async () => { const retry = deferred(); const fetcher = vi .fn() @@ -146,11 +146,11 @@ describe('useReverificationWithState', () => { await act(async () => { await expect(pending).rejects.toMatchObject({ message: 'Mock delete failed.' }); }); - await waitFor(() => expect(result.current[1]).toEqual({ phase: 'settled' })); + await waitFor(() => expect(result.current[1]).toEqual({ phase: 'inactive' })); expect(challengeCancel).not.toHaveBeenCalled(); }); - it('settles immediately when the active challenge is cancelled', async () => { + it('returns to inactive at once when the active challenge is cancelled', async () => { const fetcher = vi.fn().mockResolvedValue({ reverificationLevel: undefined } satisfies Hint); const { result } = renderHook(() => useReverificationWithState(fetcher)); @@ -164,78 +164,17 @@ describe('useReverificationWithState', () => { assertActive(result.current[1]); result.current[1].cancel(); }); - expect(result.current[1]).toEqual({ phase: 'settled' }); + expect(result.current[1]).toEqual({ phase: 'inactive' }); await act(async () => { await expect(pending).rejects.toMatchObject({ code: 'reverification_cancelled' }); }); expect(isReverificationCancelledError(await pending.catch(error => error))).toBe(true); - expect(result.current[1]).toEqual({ phase: 'settled' }); + await waitFor(() => expect(result.current[1]).toEqual({ phase: 'inactive' })); expect(fetcher).toHaveBeenCalledOnce(); expect(challengeCancel).toHaveBeenCalledOnce(); }); - it('stays settled until the handler is called again', async () => { - const fetcher = vi - .fn() - .mockResolvedValueOnce({ reverificationLevel: 'first_factor' } satisfies Hint) - .mockResolvedValueOnce({ reverificationLevel: 'first_factor' } satisfies Hint) - .mockResolvedValue('ok'); - const { result } = renderHook(() => useReverificationWithState(fetcher)); - - let first!: Promise; - act(() => { - first = result.current[0](); - }); - await waitFor(() => expect(result.current[1].phase).toBe('active')); - act(() => { - assertActive(result.current[1]); - result.current[1].cancel(); - }); - await act(async () => { - await first.catch(() => undefined); - }); - expect(result.current[1]).toEqual({ phase: 'settled' }); - - let second!: Promise; - act(() => { - second = result.current[0](); - }); - expect(result.current[1]).toEqual({ phase: 'inactive' }); - await waitFor(() => expect(result.current[1].phase).toBe('active')); - act(() => { - assertActive(result.current[1]); - result.current[1].cancel(); - }); - await act(async () => { - await second.catch(() => undefined); - }); - }); - - it('releases a settled run with reset', async () => { - const fetcher = vi.fn().mockResolvedValue({ reverificationLevel: 'first_factor' } satisfies Hint); - const { result } = renderHook(() => useReverificationWithState(fetcher)); - - let first!: Promise; - act(() => { - first = result.current[0](); - }); - await waitFor(() => expect(result.current[1].phase).toBe('active')); - act(() => { - assertActive(result.current[1]); - result.current[1].cancel(); - }); - - act(() => result.current[2]()); - expect(result.current[1]).toEqual({ phase: 'settled' }); - - await act(async () => { - await first.catch(() => undefined); - }); - act(() => result.current[2]()); - expect(result.current[1]).toEqual({ phase: 'inactive' }); - }); - it('rejects a second call during the initial request without calling the fetcher again', async () => { const gate = deferred(); const fetcher = vi.fn(() => gate.promise); @@ -296,10 +235,10 @@ describe('useReverificationWithState', () => { await act(async () => { await expect(first).resolves.toBe('ok'); }); - await waitFor(() => expect(result.current[1]).toEqual({ phase: 'settled' })); + await waitFor(() => expect(result.current[1]).toEqual({ phase: 'inactive' })); }); - it('starts another challenge after the first invocation settles', async () => { + it('starts another challenge after the first invocation ends', async () => { const retry = deferred(); const fetcher = vi .fn() @@ -323,7 +262,7 @@ describe('useReverificationWithState', () => { await act(async () => { await expect(first).resolves.toBe('ok'); }); - await waitFor(() => expect(result.current[1]).toEqual({ phase: 'settled' })); + await waitFor(() => expect(result.current[1]).toEqual({ phase: 'inactive' })); act(() => { first = result.current[0](); @@ -353,7 +292,7 @@ describe('useReverificationWithState', () => { session = { id: 'sess_2' }; rerender(); await settled; - expect(result.current[1]).toEqual({ phase: 'settled' }); + await waitFor(() => expect(result.current[1]).toEqual({ phase: 'inactive' })); expect(challengeCancel).toHaveBeenCalledOnce(); }); @@ -398,7 +337,7 @@ describe('useReverificationWithState', () => { session = null; rerender(); await settled; - expect(result.current[1]).toEqual({ phase: 'settled' }); + await waitFor(() => expect(result.current[1]).toEqual({ phase: 'inactive' })); }); it('does not cancel a retry when the session changes', async () => { @@ -498,7 +437,7 @@ describe('useReverificationWithState', () => { await expect(first).resolves.toBe('ok'); }); - it('does not cancel again on unmount after the flow already settled', async () => { + it('does not cancel again on unmount after the flow already ended', async () => { const fetcher = vi.fn().mockResolvedValue({ reverificationLevel: 'first_factor' } satisfies Hint); const { result, unmount } = renderHook(() => useReverificationWithState(fetcher)); diff --git a/packages/mosaic/src/features/reverification/reverification.controller.ts b/packages/mosaic/src/features/reverification/reverification.controller.ts index 43ea26a788a..b810370cff7 100644 --- a/packages/mosaic/src/features/reverification/reverification.controller.ts +++ b/packages/mosaic/src/features/reverification/reverification.controller.ts @@ -1,4 +1,4 @@ -import { useEffect, useState } from 'react'; +import { useEffect, useMemo, useState } from 'react'; import type { FlowDirection } from '../../components/flow'; import { useMessages } from '../../localization'; @@ -26,10 +26,9 @@ export type ReverificationController = ReverificationControllerState & { /** * Whether reverification has a card of its own to show: a factor or an error. The pending card * shown before a factor exists is not counted, so a host can keep its own pending state on - * screen until this turns true. Stays true after the run ends until the next run or `reset`. + * screen until this turns true. False again as soon as the run ends or the user cancels. */ visible: boolean; - reset: () => void; }; type OverlayFrom = 'factor' | 'method-picker'; @@ -51,13 +50,11 @@ interface ReverificationContext { frozenActiveMethodId: string | undefined; overlayFrom: OverlayFrom; supportEmail: string; - heldFrom: string; deps: ReverificationDeps; } type ReverificationEvent = | { type: 'START' } - | { type: 'SETTLE' } | { type: 'RESET' } | { type: 'TYPE'; value: string } | { type: 'SUBMIT' } @@ -90,13 +87,6 @@ function unlockResend(): Pick { return { resendAvailableAt: undefined }; } -function settleFrom(state: string) { - return { - target: 'settled' as const, - actions: assign(() => ({ heldFrom: state })), - }; -} - function factorError(error: unknown): FactorError { return { detail: errorDetail(error) }; } @@ -195,7 +185,6 @@ export const reverificationMachine = createMachine({ frozenActiveMethodId: undefined, overlayFrom: 'factor', supportEmail: '', - heldFrom: '', deps: unseatedDeps, }, states: { @@ -210,7 +199,7 @@ export const reverificationMachine = createMachine({ }, starting: { - on: { RESET: 'inactive', SETTLE: settleFrom('starting') }, + on: { RESET: 'inactive' }, invoke: fromPromise(ctx => ctx.deps.start(), { onDone: afterResult, onError: 'failed', @@ -226,7 +215,6 @@ export const reverificationMachine = createMachine({ preparing: { on: { ...factorEvents, - SETTLE: settleFrom('preparing'), SUBMIT: { actions: assign(() => ({ submitRequested: true })), }, @@ -259,7 +247,7 @@ export const reverificationMachine = createMachine({ */ methodPickerPreparing: { entry: assign(() => lockResend()), - on: { RESET: 'inactive', SETTLE: settleFrom('methodPickerPreparing') }, + on: { RESET: 'inactive' }, invoke: fromPromise(prepareActive, { onDone: 'verifying', onError: { @@ -273,7 +261,6 @@ export const reverificationMachine = createMachine({ always: [{ guard: ctx => ctx.activeMethod === null, target: 'unavailable' }], on: { ...factorEvents, - SETTLE: settleFrom('verifying'), SUBMIT: 'submitting', RESEND: { target: 'preparing', @@ -291,7 +278,7 @@ export const reverificationMachine = createMachine({ }, submitting: { - on: { RESET: 'inactive', SETTLE: settleFrom('submitting') }, + on: { RESET: 'inactive' }, invoke: fromPromise(submit, { onDone: afterResult, onError: { @@ -303,7 +290,6 @@ export const reverificationMachine = createMachine({ methodPicker: { on: { - SETTLE: settleFrom('methodPicker'), SELECT_METHOD: [ { target: 'methodPickerPreparing', @@ -326,7 +312,6 @@ export const reverificationMachine = createMachine({ help: { on: { - SETTLE: settleFrom('help'), BACK: [ { target: 'methodPicker', @@ -340,15 +325,15 @@ export const reverificationMachine = createMachine({ }, unavailable: { - on: { RESET: 'inactive', SETTLE: settleFrom('unavailable') }, + on: { RESET: 'inactive' }, }, failed: { - on: { RESET: 'inactive', SETTLE: settleFrom('failed') }, + on: { RESET: 'inactive' }, }, finishing: { - on: { RESET: 'inactive', SETTLE: settleFrom('finishing') }, + on: { RESET: 'inactive' }, invoke: fromPromise(ctx => ctx.deps.finish(), { onDone: 'retrying', onError: 'failed', @@ -356,17 +341,8 @@ export const reverificationMachine = createMachine({ }, // This means we are retrying the action after successful reverification - // This state is usually left by the model state going to 'settled' once the retry has completed + // This state is left by the model returning to inactive once the retry has completed retrying: { - on: { RESET: 'inactive', SETTLE: settleFrom('retrying') }, - }, - - /* - The action has ended and the caller has not moved on yet. The machine keeps the last - frame (heldFrom) so the controller can keep rendering it, inert, until the model - leaves 'settled' and the machine is reset. - */ - settled: { on: { RESET: 'inactive' }, }, }, @@ -424,14 +400,17 @@ function viewStep(value: string, method: ReverificationMethod | null): Reverific * retried, so it keeps the last factor pending and has no `onCancel`. The factor machine runs * while the model is active and resets when the model returns to inactive. * - * When the run ends after reverification was shown (completed, failed or cancelled), the model is - * 'settled' and the controller keeps returning the last frame, pending and without `onCancel`, - * until the next run starts or `reset` is called. Hosts decide when to stop rendering it. + * When the run ends or the user cancels, the model returns to inactive and the controller to + * 'idle' in the same render. Continuity of the card on screen is the `Reverification` component's + * job: it keeps rendering the last card it showed, pending and without `onCancel`, for as long as + * it stays mounted, so a host can animate the card away without it going blank. */ -export function useReverificationController(model: ReverificationModel, reset: () => void): ReverificationController { +export function useReverificationController(model: ReverificationModel): ReverificationController { const state = useReverificationState(model); - const visible = state.status === 'ready' || state.status === 'retrying' || state.status === 'error'; - return { ...state, visible, reset }; + return useMemo( + () => ({ ...state, visible: state.status === 'ready' || state.status === 'retrying' || state.status === 'error' }), + [state], + ); } function useReverificationState(model: ReverificationModel): ReverificationControllerState { @@ -467,88 +446,82 @@ function useReverificationState(model: ReverificationModel): ReverificationContr (model.status === 'loading' || model.status === 'active') && snapshot.value !== 'retrying' ? model.cancel : undefined; - const onCancel = cancelModel - ? () => { - send({ type: 'SETTLE' }); - cancelModel(); - } - : undefined; - const settled = model.status === 'settled'; + const onCancel = cancelModel; const needsStart = active !== null && snapshot.value === 'inactive'; - const needsSettle = settled && snapshot.value !== 'inactive' && snapshot.value !== 'settled'; - const needsReset = - (model.status === 'inactive' && snapshot.value !== 'inactive') || (!settled && snapshot.value === 'settled'); + const needsReset = model.status === 'inactive' && snapshot.value !== 'inactive'; useEffect(() => { if (needsStart) { send({ type: 'START' }); - } else if (needsSettle) { - send({ type: 'SETTLE' }); } else if (needsReset) { send({ type: 'RESET' }); } - }, [needsStart, needsSettle, needsReset, send]); + }, [needsStart, needsReset, send]); - if (model.status === 'inactive') { - return { status: 'idle' }; - } + const inactive = model.status === 'inactive'; - const { context } = snapshot; - const activeMethod = context.activeMethod; - const state = settled && snapshot.value === 'settled' ? context.heldFrom : snapshot.value; + return useMemo((): ReverificationControllerState => { + if (inactive) { + return { status: 'idle' }; + } - if (state === 'unavailable') { - return { status: 'error', reason: 'noFactors', onCancel }; - } + const { context } = snapshot; + const activeMethod = context.activeMethod; + const state = snapshot.value; - if (state === 'failed') { - return { status: 'error', reason: 'generic', onCancel }; - } + if (state === 'unavailable') { + return { status: 'error', reason: 'noFactors', onCancel }; + } - const step = viewStep(state, activeMethod); + if (state === 'failed') { + return { status: 'error', reason: 'generic', onCancel }; + } - if (!step) { - return { status: 'loading', onCancel }; - } + const step = viewStep(state, activeMethod); - // If we are currently on the alternative methods screen and preparing a factor, activeMethod will - // have transitioned to the factor we are now preparing, so the one we want to hide is the old one - const excludeId = state === 'methodPickerPreparing' ? context.frozenActiveMethodId : activeMethod?.id; - const methods = context.methods.filter(method => method.id !== excludeId); - const pendingMethodId = state === 'methodPickerPreparing' ? activeMethod?.id : undefined; - - const view: ReverificationViewProps = { - step, - direction: context.direction, - value: context.inputValue, - onValueChange: (value: string) => send({ type: 'TYPE', value }), - errorMessage: context.error ? (context.error.detail ?? m.unstable__errors__generic) : undefined, - isPending: settled || pendingStates.has(state), - onSubmit: () => send({ type: 'SUBMIT' }), - onShowMethods: () => send({ type: 'SHOW_METHODS' }), - onShowHelp: () => send({ type: 'SHOW_HELP' }), - onBack: () => send({ type: 'BACK' }), - onEmailSupport: () => { - if (context.supportEmail) { - window.location.assign(`mailto:${context.supportEmail}`); - } - }, - methods, - pendingMethodId, - onSelectMethod: (id: string) => send({ type: 'SELECT_METHOD', id }), - otpChannel: activeMethod ? otpChannelFor(activeMethod.strategy) : undefined, - onResend: () => send({ type: 'RESEND' }), - canResend, - // We clamp this to 30s because the first render after locking resend - // will have the old `now` state set, which would result in a value above - // 30s. There are other solutions like reading Date.now() in render and - // letting the interval only retrigger render, but that makes the render - // impure which is something the React compiler would warn about. - // useSyncExternalStore feels unnecessarily complex here - resendRemainingSeconds: - countingDown && resendAvailableAt - ? Math.min(RESEND_COOLDOWN_MS / 1000, Math.max(0, Math.ceil((resendAvailableAt - now) / 1000))) - : undefined, - }; + if (!step) { + return { status: 'loading', onCancel }; + } - return state === 'retrying' ? { status: 'retrying', ...view } : { status: 'ready', onCancel, ...view }; + // If we are currently on the alternative methods screen and preparing a factor, activeMethod will + // have transitioned to the factor we are now preparing, so the one we want to hide is the old one + const excludeId = state === 'methodPickerPreparing' ? context.frozenActiveMethodId : activeMethod?.id; + const methods = context.methods.filter(method => method.id !== excludeId); + const pendingMethodId = state === 'methodPickerPreparing' ? activeMethod?.id : undefined; + + const view: ReverificationViewProps = { + step, + direction: context.direction, + value: context.inputValue, + onValueChange: (value: string) => send({ type: 'TYPE', value }), + errorMessage: context.error ? (context.error.detail ?? m.unstable__errors__generic) : undefined, + isPending: pendingStates.has(state), + onSubmit: () => send({ type: 'SUBMIT' }), + onShowMethods: () => send({ type: 'SHOW_METHODS' }), + onShowHelp: () => send({ type: 'SHOW_HELP' }), + onBack: () => send({ type: 'BACK' }), + onEmailSupport: () => { + if (context.supportEmail) { + window.location.assign(`mailto:${context.supportEmail}`); + } + }, + methods, + pendingMethodId, + onSelectMethod: (id: string) => send({ type: 'SELECT_METHOD', id }), + otpChannel: activeMethod ? otpChannelFor(activeMethod.strategy) : undefined, + onResend: () => send({ type: 'RESEND' }), + canResend, + // We clamp this to 30s because the first render after locking resend + // will have the old `now` state set, which would result in a value above + // 30s. There are other solutions like reading Date.now() in render and + // letting the interval only retrigger render, but that makes the render + // impure which is something the React compiler would warn about. + // useSyncExternalStore feels unnecessarily complex here + resendRemainingSeconds: + countingDown && resendAvailableAt + ? Math.min(RESEND_COOLDOWN_MS / 1000, Math.max(0, Math.ceil((resendAvailableAt - now) / 1000))) + : undefined, + }; + + return state === 'retrying' ? { status: 'retrying', ...view } : { status: 'ready', onCancel, ...view }; + }, [inactive, snapshot, onCancel, send, m, canResend, countingDown, resendAvailableAt, now]); } diff --git a/packages/mosaic/src/features/reverification/reverification.model.tsx b/packages/mosaic/src/features/reverification/reverification.model.tsx index 8f572df4dea..6d15d121b76 100644 --- a/packages/mosaic/src/features/reverification/reverification.model.tsx +++ b/packages/mosaic/src/features/reverification/reverification.model.tsx @@ -32,8 +32,7 @@ export type ReverificationModel = | { status: 'inactive' } | { status: 'loading'; cancel: () => void } | ReverificationActiveModel - | { status: 'retrying' } - | { status: 'settled' }; + | { status: 'retrying' }; function toMethod( factor: SessionVerificationFirstFactor | SessionVerificationSecondFactor, @@ -123,10 +122,6 @@ export function useReverificationModel(reverificationState: ReverificationState) return { status: 'retrying' }; } - if (reverificationState.phase === 'settled') { - return { status: 'settled' }; - } - const { level, cancel, complete } = reverificationState; if (!session || !environment || supportEmail === undefined) { diff --git a/packages/mosaic/src/features/reverification/reverification.tsx b/packages/mosaic/src/features/reverification/reverification.tsx index bd933a19ae7..14463ca4cf4 100644 --- a/packages/mosaic/src/features/reverification/reverification.tsx +++ b/packages/mosaic/src/features/reverification/reverification.tsx @@ -1,3 +1,5 @@ +import { useState } from 'react'; + import { type ReverificationController, useReverificationController } from './reverification.controller'; import { useReverificationModel } from './reverification.model'; import { ReverificationError, ReverificationPending, ReverificationView } from './reverification.view'; @@ -15,9 +17,9 @@ export type UseReverificationFlowResult( fetcher: F, ): UseReverificationFlowResult { - const [wrappedFetcher, reverificationState, reset] = useReverificationWithState(fetcher); + const [wrappedFetcher, reverificationState] = useReverificationWithState(fetcher); const model = useReverificationModel(reverificationState); - const controller = useReverificationController(model, reset); + const controller = useReverificationController(model); return [wrappedFetcher, controller]; } @@ -27,21 +29,42 @@ export interface ReverificationProps { onClose?: () => void; } +function held(controller: ReverificationController): ReverificationController { + switch (controller.status) { + case 'ready': + return { ...controller, onCancel: undefined, isPending: true }; + case 'loading': + case 'error': + return { ...controller, onCancel: undefined }; + case 'retrying': + case 'idle': + return controller; + } +} + export function Reverification({ controller, onClose }: ReverificationProps) { - if (controller.status === 'idle') { + const [lastShown, setLastShown] = useState(null); + // Reverification controller is memoized, but even if that breaks, this + // component still only does a cheap immediate re-render and then continues + if (controller.status !== 'idle' && controller !== lastShown) { + setLastShown(controller); + } + const shown = controller.status === 'idle' ? (lastShown ? held(lastShown) : null) : controller; + + if (!shown || shown.status === 'idle') { return null; } - if (controller.status === 'loading') { + if (shown.status === 'loading') { return ; - } else if (controller.status === 'error') { + } else if (shown.status === 'error') { return ( ); } - return ; + return ; } diff --git a/packages/mosaic/src/features/reverification/reverification.types.ts b/packages/mosaic/src/features/reverification/reverification.types.ts index e55cda4b1af..ada8be5fc0d 100644 --- a/packages/mosaic/src/features/reverification/reverification.types.ts +++ b/packages/mosaic/src/features/reverification/reverification.types.ts @@ -47,8 +47,7 @@ export type ReverificationViewProps = ReverificationViewFields & { export type ReverificationState = | { phase: 'inactive' } | { phase: 'active'; complete: () => void; cancel: () => void; level: SessionVerificationLevel | undefined } - | { phase: 'retrying' } - | { phase: 'settled' }; + | { phase: 'retrying' }; export type ReverificationResult = { status: 'needs_first_factor' | 'needs_second_factor' | 'complete'; diff --git a/packages/mosaic/src/features/reverification/use-reverification-with-state.ts b/packages/mosaic/src/features/reverification/use-reverification-with-state.ts index b332f98f39d..a9948f81f6b 100644 --- a/packages/mosaic/src/features/reverification/use-reverification-with-state.ts +++ b/packages/mosaic/src/features/reverification/use-reverification-with-state.ts @@ -9,7 +9,6 @@ export type ReverificationFetcher = Parameters[0]; export type UseReverificationWithStateResult = readonly [ ReturnType>, ReverificationState, - () => void, ]; type RuntimeOperation = @@ -30,10 +29,6 @@ type Runtime = { sessionId: string | null; }; -function releaseSettled(state: ReverificationState): ReverificationState { - return state.phase === 'settled' ? { phase: 'inactive' } : state; -} - const REQUEST_ALREADY_IN_PROGRESS_CODE = 'request_already_in_progress'; function requestAlreadyInProgressError(): ClerkRuntimeError { @@ -45,12 +40,11 @@ function requestAlreadyInProgressError(): ClerkRuntimeError { /** * Wraps useReverification without the default UI. Returns [handler, state]: call handler * to run the action, and render your own UI from state.phase ('inactive' | 'active' | - * 'retrying' | 'settled'). While 'active', state.complete and state.cancel end the challenge. + * 'retrying'). While 'active', state.complete and state.cancel end the challenge. * - * A run that was interrupted by a reverification ends in 'settled', not 'inactive', so the - * UI that was showing can be held until the caller moves on. 'settled' ends when the handler - * is called again, or when the third return value, reset, is called. A run that was never - * interrupted stays 'inactive' throughout. + * The state follows the run: 'active' while a challenge is pending, 'retrying' while the fetcher + * runs again after it, and 'inactive' otherwise. Cancelling returns to 'inactive' at once; the + * run rejects shortly after. * * The handler runs one call at a time. A call made while another is pending rejects with a * ClerkRuntimeError with code 'request_already_in_progress'. @@ -93,7 +87,7 @@ export function useReverificationWithState( return; } runtimeRef.current.operation = { status: 'cancelling', promise: operation.promise }; - setReverificationState({ phase: 'settled' }); + setReverificationState({ phase: 'inactive' }); operation.cancel(); }, []); @@ -130,8 +124,6 @@ export function useReverificationWithState( return Promise.reject(requestAlreadyInProgressError()); } - setReverificationState(releaseSettled); - const invocation = Promise.resolve().then(() => wrapped(...args)); runtimeRef.current.operation = { status: 'requesting', promise: invocation }; void invocation @@ -141,7 +133,7 @@ export function useReverificationWithState( return; } runtimeRef.current.operation = { status: 'idle' }; - setReverificationState({ phase: operation.status === 'requesting' ? 'inactive' : 'settled' }); + setReverificationState({ phase: 'inactive' }); }) // The original error is meant to be handled outside, but .finally() creates // a new promise that errors the same way, so we swallow that duplicate error silently @@ -152,13 +144,6 @@ export function useReverificationWithState( [wrapped], ) as ReturnType>; - const reset = useCallback(() => { - if (runtimeRef.current.operation.status !== 'idle') { - return; - } - setReverificationState(releaseSettled); - }, []); - const phase = reverificationState.phase; useEffect(() => { if (phase !== 'active') { @@ -198,5 +183,5 @@ export function useReverificationWithState( }; }, []); - return [singleFlight, reverificationState, reset]; + return [singleFlight, reverificationState]; } diff --git a/packages/swingset/src/stories/reverification.mdx b/packages/swingset/src/stories/reverification.mdx index 95e5f516859..1ba03fdef6b 100644 --- a/packages/swingset/src/stories/reverification.mdx +++ b/packages/swingset/src/stories/reverification.mdx @@ -46,22 +46,13 @@ const step = isRunning && reverification.visible ? 'verify' : 'confirm'; ### When the run ends -The reverification card does not disappear when the action settles. If the user was shown a card during the run, the run ends in a settled state, whether it succeeded, failed, or was cancelled. The controller keeps returning the last card, inert: it is pending and has no `onCancel`. `visible` stays true unless the last thing shown was the pending card. +The controller follows the run. When the action resolves or fails, `status` returns to `idle` and `visible` to false. `` keeps showing the last card it rendered, pending and without a way to cancel, for as long as it stays mounted, so a parent that animates the card away does not show an empty card. Gate the card on your own running state and `visible`, as above, so the held card is not shown again before the next run. -This lets the parent finish its own work after the action returns, such as refetching or closing a dialog, without the screen falling back to an earlier step in between. The parent leaves the card by unmounting `` or changing its own step when it is done. - -A run that never asked for reverification never settles in this way. `status` stays `idle`, and `visible` stays false for the whole run. - -The settled card is released by whichever happens first. - -- The action is run again. -- `reverification.reset()` is called. - -Call `reset()` when a parent leaves the flow without running the action again and wants a clean state, so the next render does not show a stale card. +A run that never asked for reverification shows no card. `status` stays `idle`, and `visible` stays false for the whole run. ### Cancelling -`reverification.onCancel` is a request. It is undefined when the user cannot cancel, which is the case while the action is being retried and once the run has settled. Calling it rejects the action with a `reverification_cancelled` error and settles the card. +`reverification.onCancel` is a request. It is undefined when the user cannot cancel, which is the case while the action is being retried. Calling it rejects the action with a `reverification_cancelled` error. The controller returns to `idle` at once and the rejection reaches the parent shortly after, so close from the same handler that calls `onCancel` rather than from the rejection, or the parent's own step can change before it closes. The parent decides what cancelling means for its own flow. Pass `onClose` to `` to run that instead of `onCancel` when the user presses the Close button on the error card. The parent is then responsible for calling `onCancel` itself, as `Destructive` does when its dialog closes. This lets the parent close or leave the step at the moment of the click, instead of waiting for its own action to unwind. From 94061f2b390d357c1caa93dd266ea993fb3ee50a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20H=C3=B6glund?= Date: Thu, 15 Oct 2026 14:28:31 +0200 Subject: [PATCH 3/4] Add empty changeset --- .changeset/funky-suns-press.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changeset/funky-suns-press.md diff --git a/.changeset/funky-suns-press.md b/.changeset/funky-suns-press.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/funky-suns-press.md @@ -0,0 +1,2 @@ +--- +--- From 61062aa3aab114a8379431792fa6d246a39df2ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20H=C3=B6glund?= Date: Thu, 15 Oct 2026 15:58:36 +0200 Subject: [PATCH 4/4] Fix failing CI --- packages/mosaic/eslint-suppressions.json | 5 ----- .../__tests__/confirmation.feature.test.tsx | 16 +++++++--------- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/packages/mosaic/eslint-suppressions.json b/packages/mosaic/eslint-suppressions.json index 245446609fe..4ea77788f19 100644 --- a/packages/mosaic/eslint-suppressions.json +++ b/packages/mosaic/eslint-suppressions.json @@ -29,11 +29,6 @@ "count": 1 } }, - "src/features/reverification/reverification.controller.ts": { - "sonarjs/cognitive-complexity": { - "count": 1 - } - }, "src/features/reverification/use-reverification-with-state.ts": { "@typescript-eslint/consistent-type-assertions": { "count": 1 diff --git a/packages/mosaic/src/blocks/confirmation/__tests__/confirmation.feature.test.tsx b/packages/mosaic/src/blocks/confirmation/__tests__/confirmation.feature.test.tsx index dfd860f6fc9..16dbd7199c1 100644 --- a/packages/mosaic/src/blocks/confirmation/__tests__/confirmation.feature.test.tsx +++ b/packages/mosaic/src/blocks/confirmation/__tests__/confirmation.feature.test.tsx @@ -297,12 +297,10 @@ describe('Confirmation shared by rows', () => { function ReverifiedHost({ action, cleanupMs, - settleMs, beforeRunMs, }: { action: () => Promise; cleanupMs?: number; - settleMs?: number; beforeRunMs?: number; }) { const [run, reverification] = useReverificationFlow(action); @@ -312,11 +310,7 @@ function ReverifiedHost({ await new Promise(resolve => setTimeout(resolve, beforeRunMs)); } try { - const result = await run(); - if (settleMs !== undefined) { - await new Promise(resolve => setTimeout(resolve, settleMs)); - } - return result; + return await run(); } catch (error) { if (cleanupMs !== undefined) { await new Promise(resolve => setTimeout(resolve, cleanupMs)); @@ -435,9 +429,13 @@ describe('Confirmation with reverification', () => { } for (const settleMs of [undefined, 400]) { - it(`does not go back to the confirm step after a successful verification (host settles after ${settleMs ?? 0}ms)`, async () => { + it(`does not go back to the confirm step after a successful verification (action settles after ${settleMs ?? 0}ms)`, async () => { serveFapi(passwordVerification); - const { user } = await renderReverified({ action: interruptedOnce(), settleMs }); + const action = vi + .fn() + .mockResolvedValueOnce(reverificationError()) + .mockImplementation(() => new Promise(resolve => setTimeout(() => resolve({ done: true }), settleMs ?? 0))); + const { user } = await renderReverified({ action }); await confirmIntoVerification(user); const field = await screen.findByLabelText('Password');