Conversation
…s screen Remove the process()/handleCallback()/driver-useEffect orchestration engine plus its exhaustive-deps eslint-disable, ahead of the XState migration (Expensify#81197). With the engine gone the flow is inert, so executeScenario now opens the modal and navigates straight to the success outcome screen (SET_FLOW_COMPLETE so back-press/backdrop close directly) to keep the modal and outcome UI testable until the state machine lands.
…actor/3ds/mfa-cleanup-for-state-machine
Add xstate@5.31.1 + @xstate/react@5.0.5 and a machine whose top-level states map 1:1 to the visible screen (idle -> preparing -> outcome -> closing); INIT short-circuits to outcome.success in this slice. Teardown owns its own state: CLOSE_MODAL (guarded on isModalOpen) enters closing, the navigator reports MODAL_CLOSED, and entering idle wipes the context plus the mfaNavigation buffer. A machine-owned closeFallback delay re-enters idle even if the navigator unmounts mid-close and cancels its transition callback, so flow data (validate code, challenges, scenario response) cannot outlive the modal. The Provider becomes a thin typed facade over send() with no flow logic; OutcomeScreenBase's close button and the modal navigator wire into it, and the RESET event/reset() API is gone (the navigator was its only consumer). Refs Expensify#81197
isModalOpen was context data duplicating what the chart already knows.
A parent `open` compound state now hosts the screen states; CLOSE_MODAL
is declared on it, so the isModalOpen guard and the closeModal action
are gone and duplicate events while idle or closing are ignored by the
chart structure itself. Shared flow events of later slices (SET_ERROR,
...) get a natural home on `open`.
The machine context drops the flag (Omit) and a new snapshotToState
mapper restores the legacy shape for consumers, deriving isModalOpen as
matches('open') - false in `closing`, which is what starts the
navigator's close animation. The cancel-confirm dialog is still hidden
on CLOSE_MODAL so it cannot stay up during the slide-out.
Refs Expensify#81197
The navigator's local Phase useState + render-time sync duplicated the machine's top level (open/closing/idle). snapshotToState now derives modalPhase from the chart and the navigator mounts, animates teardown, and unmounts off that single value; the close callback only reports MODAL_CLOSED. A cancelled close report can no longer strand the navigator in 'closing': the machine's closeFallback re-enters idle, which now also unmounts the view. Refs Expensify#81197
The backdrop faded out over ANIMATED_TRANSITION (300ms) while the screen's slide-out - the close spec of SLIDE_FROM_RIGHT - takes 100ms on web. The navigator unmounts on transitionEnd, so the fade was ripped out at a third of its run. Fade with the slide's own duration and easing instead, the way the RHP overlay rides the card progress. Refs Expensify#81197
Comment-only review pass over the machine slices:
- the Provider API doc claimed every method is a thin send() wrapper
and that state is the bare legacy shape; executeScenario's telemetry
and the modalPhase field made both claims false
- MfaMachineContext doc still said snapshotToState returns the "full
legacy shape" (stale since modalPhase landed)
- the machine header repeated the teardown contract and the modalPhase
derivation that live next to the closing state and in snapshotToState;
it also referenced the removed engine's process()+useEffect race,
meaningless to a post-merge reader
- dropped history notes ("Relocated here (PR-3)", "the old
handleCallback had"), a caller list on closeModal, and the RHP-overlay
analogy in the backdrop fade comment
Refs Expensify#81197
Follow-up review pass on mfaMachine.ts: - the header shrank to the structure convention (top level = modal lifecycle, open children = visible screens) and the no-final-state rationale; per-state bullets restated the chart below - the wipe rationale moved onto idle's entry, the dropped-INIT note onto idle's INIT transition, and the deferred-push note inside navigateToSuccessOutcome next to runAfterTransition - closeFallback now just says what the delay is for; the closing comment spells out that the navigator unmounting mid-close means MODAL_CLOSED never comes - dropped the hideCancelConfirm comment, which described nothing the assign showed Refs Expensify#81197
React Compiler now compiles this file, so manual useCallback/useMemo wrappers are redundant. Addresses review feedback from roryabraham on Expensify#93055.
…e' into dariusz-biela/refactor/3ds/mfa-state-machine-setup # Conflicts: # src/components/MultifactorAuthentication/Context/MultifactorAuthenticationMainContext.tsx
…-biela/refactor/3ds/mfa-cleanup-for-state-machine [NO QA] refactor(mfa): remove flow engine, scaffold executeScenario to success screen
The machine Provider exposed a single context bundling executeScenario with flow-internal controls, and the hook lived in the Provider module. Any config-reachable screen importing it closed an import cycle (config/scenarios -> screens -> Context -> config): OutcomeScreenBase did exactly that through the Context barrel, crashing every jest entry that loads a scenario module first (sortTransactionsPending3DSReview.test.ts). Split the API into two runtime-leaf contexts; react is their only runtime import, so they are safe to consume from anywhere: - external (useMultifactorAuthentication, barrel-exported): executeScenario only. App code starting flows keeps its current import and stops re-rendering on machine transitions. - internal (useMultifactorAuthenticationInternal, imported from the leaf directly, deliberately not in the barrel): machine state + modal lifecycle for the screens and navigator hosting a flow. Also route ChangePIN's state hook through its existing leaf instead of the barrel, which closed the same cycle via the barrel's usePromptContent edge.
`{} as T` inside setup({types}) is XState v5's documented typing idiom; the
values are erased at runtime and only carry types, so there is no
assertion-free way to express it.
…essor The per-scenario action signatures make the config record's value union non-narrowable, so every lookup needed its own unsafe assertion. Move it into a single getScenarioConfig() accessor in the config module and use it from both the provider and the reducer; this also removes the grandfathered seatbelt entry for stateReducer.ts. Rename the local Configs record to match its public SCREAMING_SNAKE export name.
Knip compares exports against main, so symbols exported for future slices fail CI now. Trim to what is consumed today (mfaMachine, snapshotToState, MfaState); re-add the rest in the slices that use them. mfaMachine becomes a default export to satisfy import/prefer-default-export once DEFAULT_CONTEXT is module-local.
PR/slice numbers (PR-5, PR-11, Slice 1) reference the implementation plan, which reviewers have no access to, and they rot as the stack evolves. Rephrase as "a later slice adds X".
…Expensify/App into dariusz-biela/refactor/3ds/mfa-state-machine-setup
Drop the stored isModalOpen flag: the machine already models the modal lifecycle, so useSyncMfaModalNavigatorWithHistory now takes modalPhase and derives the boolean locally. Also inline the now-identity MfaMachineContext alias.
Dev-only Stately Browser Inspector for XState machines, opened on demand from Troubleshoot > XState inspector in the test tool menu. - sanitizeContext/sanitizeEvent mask every subtree under a sensitive key (payload, pin, password, token, otp, secret, validateCode) at any depth before events leave the app to stately.ai - useInspectedMachine hook pre-wires the inspector so new machines adopt it with one line; MFA main context switched to it - guarded require keeps @statelyai/inspect out of production bundles; native/Jest resolve a stub via index.native.ts - unit tests cover the masking contract
Future MFA machine slices store the scenario response in context and run registration as an invoked actor, so inspection events will carry the REVEAL_CARD_DETAILS body (pan/expiration/cvv) and the HSM/passkey keyInfo (whose clientDataJSON embeds the challenge). Mask the body wholesale - keeping httpStatusCode/reason/message visible for debugging - and add pan/cvv as standalone keys in case the body is ever destructured into an event.
useMachine creates its actor during render and XState announces actors to inspectors in the Actor constructor, so a render pass React discards and redoes (concurrent restart, Strict Mode, machine hot reload) leaves a ghost actor in the Stately UI - a duplicate machine forever stuck in its initial state with no events. Hold each @xstate.actor registration until that session emits its first other inspection event (the actor actually started) and forward it then, still ahead of the triggering event. Registrations that never activate are never forwarded, so ghosts no longer reach the inspector.
Make getScenarioConfig generic so each scenario's action signature survives the lookup, dropping the no-unsafe-type-assertion suppression and the as cast it guarded. - Add scenarios/types.ts holding the per-scenario ScenarioPayloadMap, guarded against drift from the scenario union by AssertTypesEqual (same idiom as the Onyx keys check). - Name the lookup result MultifactorAuthenticationScenarioConfigFor<T> and the dynamic union MultifactorAuthenticationResolvedScenarioConfig, both defined next to the config const to keep deps one-way. - Move the INIT machine event to machine/types.ts and correlate its scenarioName, scenario, and payload through T. - Require executeScenario params only for scenarios that need them via a conditional args tuple.
Add MFA_STATE (machine node IDs) and MODAL_PHASE (view-layer phase) under CONST.MULTIFACTOR_AUTHENTICATION and reference them from the machine, snapshotToState, and the MfaModalPhase type instead of bare string literals. De-duplicates the literals and guards reference typos. Note: this does not make a wrong machine 'initial'/'target' a compile error - XState v5 does not type-check those against the real node IDs. 'matches()' is already type-checked, so snapshotToState stays guarded.
The masker keys off field names, so a state node named like a sensitive key (validateCode, pin, ...) had its state value masked - which hides the machine's current state and breaks the inspector's active-state highlight. Exempt snapshot.value from key-based masking via a maskByKey flag while keeping its structural safety (cycle, depth, postMessage). The exemption is scoped to that one path; a value key in context, events or child input/output is still masked.
Replace the 5-parameter `maskDeep` recursion - called with cryptic literals like `maskDeep(event, false, 0, new WeakSet(), true)` - with a `serialize(value, maskSensitiveKeys)` core that closure-captures the visited set and runs an inner `walk()`. Two named wrappers, `maskEvent` and `serializeStateValue`, name the two modes so the bare boolean no longer appears at any call site. Trim the explanatory comments from ~30 to ~17 lines now that the names carry the intent, and drop the serialize-vs-sanitize-hooks prose already documented in index.ts. Behavior is unchanged; the unit suite passes untouched.
open/closing already matched the machine's top-level state names; only the resting state diverged. Rename the machine state idle -> closed so all three top-level states share the view's vocabulary, delete the now-redundant MODAL_PHASE const, and source MfaModalState from MFA_STATE. Renames MfaModalPhase -> MfaModalState and modalPhase -> modalState; the navigator and history hook compare against CONST.MULTIFACTOR_AUTHENTICATION.MFA_STATE.
Address review feedback to contextualize each machine action, preferring self-documenting names over comments: - initFromEvent -> initFlow - hideCancelConfirm -> hideCancelConfirmModal (also avoids the clash with the same-named context method) - resetNavigationBuffer -> clearModalOpenNavigationState Add comments only where the name cannot carry a non-obvious detail: initFlow (why the non-INIT type guard exists) and hideCancelConfirmModal (why it runs on CLOSE_MODAL).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| import Button from './Button'; | ||
| import TestToolRow from './TestToolRow'; | ||
|
|
||
| type XStateInspectorTestToolRowProps = { |
Contributor
There was a problem hiding this comment.
❌ CONSISTENCY-13 (docs)
The new XStateInspectorTestToolRowProps type declares a prop (ready) with no /** ... */ block comment above it, and none of its props are documented. Per STYLE.md, every component prop must be documented with a JSDoc block comment at its definition site so its purpose is clear.
Add a block comment above the prop:
type XStateInspectorTestToolRowProps = {
/** Resolves to the loaded Stately inspector handle, or null if the inspector chunk failed to load */
ready: Promise<LoadedInspector | null>;
};Reviewed at: 50b8e1e | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
5 tasks
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50b8e1ef51
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| initial: MFA_STATE.RESOLVING_OUTCOME, | ||
| states: { | ||
| [MFA_STATE.RESOLVING_OUTCOME]: { | ||
| always: [{guard: 'hasError', target: MFA_STATE.FAILURE}, {target: MFA_STATE.SUCCESS}], |
There was a problem hiding this comment.
Reaching OUTCOME with no error now immediately falls through to the success screen, but the new machine has no happy-path state that requests an authorization challenge, invokes the biometric/passkey authorization, calls scenario.action, or runs the scenario callback. Both the already-accepted-soft-prompt path and the prompt-approved path target OUTCOME directly, so a user with existing credentials (or one who just enters the validate code) can see success for flows like 3DS authorization or PIN changes without the backend operation ever happening.
Useful? React with 👍 / 👎.
| const notifyValidateCodeChanged = () => send({type: 'VALIDATE_CODE_CHANGED'}); | ||
|
|
||
| // There is no cancel-confirmation dialog yet, so every cancel path closes the modal directly. | ||
| const requestCancel = () => send({type: 'CLOSE_MODAL'}); |
There was a problem hiding this comment.
For an active online flow this now closes the MFA modal directly, and confirmCancel does the same while isCancelConfirmVisible is never set. Scenarios such as AUTHORIZE_TRANSACTION provide a cancel-confirm modal and an onCancel handler that calls denyTransaction; pressing Back/Escape/the backdrop now bypasses that handler, so the transaction is not denied and no cancel outcome is shown.
Useful? React with 👍 / 👎.
JmillsExpensify
left a comment
JmillsExpensify
left a comment
Contributor
There was a problem hiding this comment.
Product review not required.
…registration [No QA] feat(mfa): add credential creation to the state machine
Contributor
|
@jakubstec seems like this branch has a huge diff, can you please think of ways to split this up? also can you link the relevant issue please? |
Member
Author
|
hey @mountiny, this PR is currently just a draft to give a high-level overview of the refactor's current state. we are actually already splitting the work into smaller slices using our local branch as a base. There is:
here is link to the issue #81197 😄 |
Contributor
|
Oh ok got it, thanks |
amyevans
removed their request for review
August 17, 2026 11:04
Seven sites built the same canceled failure result inline. The factory's return type is declared explicitly: an inferred `success: false` widens to `boolean`, which stops fitting MFAResult's failure branch.
…hots
The exemption fired on any `{snapshot: {value}}` shape, so actor logic keeping
runtime data under its own `value` had that subtree serialized unmasked at every
depth. xstate's `isMachineSnapshot` narrows it to snapshots whose `value` really
holds state-node names.
Addressing the findings from self-review.
Distinguish a missing HSM credential from an unreadable keystore
`getLocalCredentialID()` collapsed three outcomes into `string | undefined`: a
credential that was found, one the keystore confirmed does not exist, and a read
the keystore failed to answer at all. During authorization the last two both
became `NO_MATCHING_LOCAL_CREDENTIAL`, which is treated as recoverable and
deletes the account's key. A transient keystore failure therefore took the same
destructive path as a genuinely absent credential, forcing a re-registration the
device did not need.
The error was already being computed - `decodeLibraryError()` ran for the
breadcrumb and its result was then discarded - so the fix carries it instead of
dropping it. `getLocalCredential()` now returns a discriminated
`LocalCredentialLookup` of `found` / `absent` / `readFailed`, and `authorize()`
maps them separately:
- `readFailed` -> the decoded error, credential left untouched
- `absent` -> `HSM.KEY_NOT_FOUND`, credential deletable
- mismatch -> `HSM.NO_MATCHING_LOCAL_CREDENTIAL`, credential deletable
The library rejects with `KEY_NOT_FOUND` rather than returning an empty key list
when the alias does not exist, so that specific rejection is mapped to `absent`.
Treating every rejection as a read failure would have stopped routing a
first-time device into registration.
`KEY_ACCESS_FAILED` is removed from `CREDENTIAL_FAILURES_REQUIRING_LOCAL_DELETION`
for the same reason, and consequently from `RECOVERABLE_CREDENTIAL_FAILURES`,
which is derived from it: a keystore that could not be read has not proven the
credential unusable, so it must not trigger deletion or re-registration.
`areLocalCredentialsKnownToServer()` still answers `false` on a read failure and
still routes to registration. It returns a plain boolean and has no way to fail
the flow instead; that belongs with the recovery slice and is called out in a
comment at the call site.
Stop the web ceremony from mutating local credentials
`authorize()` on web called `reconcileLocalPasskeysWithBackend()`, which writes
the pruned credential list back to Onyx whenever it differs from what it read.
On a full miss it persisted an empty list, so the ceremony cleared the
credentials before the actor could decide - contradicting its own doc comment -
and did so without consulting the abort signal, leaving a hole in the
cancellation-aware cleanup the actor is careful about.
The ceremony now filters against the challenge's `allowCredentials` in place and
persists nothing. `createCredential()` keeps calling `reconcileLocalPasskeysWithBackend()`,
so stale local entries are still pruned on the registration path. The trade-off
is that a partial mismatch is no longer pruned during authorization; those
entries are inert, since every ceremony filters them out anyway.
Convert an aborted Onyx read into a canceled result
The local-passkey read in `authorize()` rejects with the signal's reason when the
flow is cancelled mid-read. Nothing caught it, so that one path left the actor as
a rejected promise and reached the machine as `UNHANDLED_EXCEPTION`, an anomalous
failure, while every other cancellation point returns `createCanceledMFAResult()`.
It is now caught and converted, and genuine read errors are rethrown unchanged.
Make the web credential deletion actually await its write
`deleteLocalPasskeyCredentials()` returned `void` while performing an `Onyx.set()`,
so `deleteLocalCredentials()` on web resolved before the write landed even though
its native twin serializes properly. It now returns the write's promise.
Pair the scenario action with its own payload
`processScenarioAction()` took a widened action plus the union of every scenario's
parameters and cast between them, so a mismatched (action, payload) pair
type-checked and would only fail at the API. The pairing is now captured at INIT,
where the scenario generic is still known: `createScenarioActionRunner()` binds
the action to its payload and the machine stores the resulting `runScenarioAction`
in context. The actor supplies only the values the ceremony owns - the signed
challenge and the authentication method - so caller-provided values still cannot
replace them, and the unsafe cast is gone.
Drop the unreachable signed-challenge guard
`processScenarioAction()` opened with a `!signedChallenge` check returning
`SIGNATURE_MISSING`. The field was already required on both sides of the call -
`MultifactorAuthenticationActionParams` picks it from
`AllMultifactorAuthenticationBaseParameters`, where it is declared non-optional -
so the branch was already unreachable before this slice. Its only test proved it:
reaching the branch required `'' as unknown as Parameters<...>[1]`.
Binding the scenario runner moved the challenge out of the params bag and into an
explicit parameter, which made that deadness visible: `signedChallenge` was then
the function's only unused input, present solely to feed the guard. Guard,
parameter, and the double-cast test are removed, along with the imports they were
the last users of. The backend validates the signature regardless, so nothing
enforced here is lost.
`SIGNATURE_MISSING` is intentionally left in `VALUES` and in `ANOMALOUS_FAILURES`.
It predates this work and reason codes are a telemetry vocabulary shared beyond
this repository, so retiring one is a wider decision than this cleanup.
Keep the prompt content stable through the close animation
`CLOSE_MODAL` ran `resetPresentationPhases`, clearing the presentation phase while
the `closing` state still renders the outgoing screen. Cancelling during
authorization therefore swapped the spinner back for the Confirm button for the
duration of the close animation - the exact flicker the phase exists to prevent -
and contradicted the invariant documented on `closing`. The action is dropped;
`closed` already runs `resetContext`, so no phase survives into the next flow.
Record two non-obvious invariants that were previously undocumented
- The credential cleanup in the authorization actor is awaited on purpose. The
deletion checks the abort signal inside its serialized queue, so returning
first would let the actor stop and abort that signal before the check runs,
silently skipping the cleanup.
- The HSM mutation queue deliberately covers only `createKeys`/`deleteKeys`. The
authorization ceremony neither creates nor destroys the key, and queueing it
would hold the biometric prompt behind a keystore write without changing its
outcome.
Tests
- authorization reports the decoded read error, and that reason is not in the
deletion set, when the keystore cannot be read
- authorization reports `KEY_NOT_FOUND`, which is in the deletion set, when the
keystore confirms the account has no key
- a `KEY_NOT_FOUND` rejection still keeps a first-time device on the registration
path
- the actor leaves the credential in place after a read failure
- the actor stays active until the cleanup resolves, pinning the awaited deletion
- web authorization persists nothing on a reconciliation miss
- a cancelled local-credential read produces a canceled result rather than an
unhandled exception
- the scenario runner is bound at INIT and receives the ceremony-owned values
…borts on actor stop
…auth [No QA] feat(mfa): add auth to the state machine
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx bun ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
This PR tracks the MFA flow to XState migration (#81197) and merges the finished
dariusz-81197-mfa-state-machineintegration branch intomain. Right now, it is a progress tracker, not a reviewable diff: the branch lands as a stack of vertical slices, each reviewed and merged into the integration branch on its own, and nothing here reachesmainuntil the whole stack is done.executeScenarioto the success screenAll merged slices target
dariusz-81197-mfa-state-machineon thesoftware-mansion-labs/expensify-app-forkremote, notmain— that's why most of them aren't visible as commits on this PR's branch comparison view directly against Expensify/App history until the final merge.Why this migration
MultifactorAuthenticationContextProviderused to drive the whole MFA flow (device check, registration, soft prompt, authorization) from a singleuseEffectthat watched state and ran an asyncprocess()step function, withreact-hooks/exhaustive-depsdisabled to keep it from re-entering mid-step. That pattern works only as long as two invariants hold — the effect's dependencies never change outside step boundaries, and every step dispatches its state update only after all its async work finishes — and both are easy to violate silently. We hit exactly that: an optimistic update on key registration re-enteredprocess()early and raced the authorization challenge request against a backend write that hadn't landed yet. Full background, the two approaches considered, and the acceptance criteria are in #81197.This migration replaces that implicit step-machine with an explicit XState machine: every state, transition and side effect is declared, so a race is a missing/incorrect transition instead of a dependency-array footgun.
Current state of the flow
With every merged slice plus the slice in review, the machine drives the full happy path:
Every step above dispatches through the machine's own transitions; the legacy reducer, its split contexts, and the composed provider that used to own this state no longer exist (removed in slice #9). Failures at any step (challenge request, platform refusal, malformed WebAuthn response, signing failure, backend rejection, scenario-action rejection, unexpected exception) resolve to the failure outcome with the specific MFA error, and an authorization failure caused by a stale/unusable local credential clears it so the next flow attempt re-registers instead of repeating the same failure.
What's left
Fixed Issues
$ #81197
Tests
No independent test steps here — this PR is a tracker, not a diff to review. See each slice PR's own
Tests/QA Stepssections for how that piece of the flow was verified; the manual test steps in #360 currently exercise the full happy path (fresh registration, returning-user authorization, refusal, cancellation, re-registration after an unusable credential) end to end.QA Steps
[No QA] - tracking PR for an internal migration. Nothing here reaches staging/production until the stack is fully merged into
main, at which point this description will be replaced with real QA steps for the complete flow.