[ECUK In-App 3DS] MFA: isolate 3DS flow into self-contained modal navigator#89992
Open
dariusz-biela wants to merge 65 commits into
Open
Conversation
MFA screens (magic code, prompt, outcome) lived inside RightModalNavigator as regular RHP routes. This created coupling between MFA flow lifecycle and app navigation state — cancelling, refreshing, or deep-linking could leave stale MFA routes in the stack. Move MFA to a self-contained overlay with its own NavigationIndependentTree and dedicated mfaNavigation module. The overlay mounts/unmounts based on MFA context state, uses the Expensify modal card-style interpolator for slide-from-right animation (same as RHP), and is fully decoupled from the app navigation tree. - Remove MFA routes from ROUTES.ts, linking config, and RHP navigator - Delete BiometricsTestPage (test button now calls executeScenario directly) - Add mfaNavigation.ts with deferred push for first-screen slide animation - Add MultifactorAuthenticationOverlay as sibling to RootStack Refs: Expensify#81021
- Remove unreachable NOT_FOUND screen (registered but never navigated to) - Replace offscreen style hack with early return null when not visible - Trim verbose comments to essential context - Remove redundant conditional guards (component returns null early)
CLOSE_MODAL sets isModalOpen=false without clearing scenario data, letting the overlay play its exit animation while screens remain mounted. RESET fires only after the animation completes, preventing the snap-disappear on close. - Add isModalOpen to MFA state, set true on INIT - Add CLOSE_MODAL action (keeps scenario/data intact) - Overlay drives visibility from isModalOpen instead of !!scenario - Overlay dispatches RESET after close animation callback - UI dispatch sites (outcome close, cancel, skip-outcome) use CLOSE_MODAL instead of RESET
goBack() on the inner navigator triggers the Stack's reverse slide-from-right animation while the backdrop fades simultaneously. Previously the screen stayed static during the backdrop fade and then snap-disappeared on unmount.
dariusz-biela
commented
May 8, 2026
|
|
||
| return ( | ||
| <NarrowPaneContextProvider> | ||
| <MultifactorAuthenticationContextProviders> |
Contributor
Author
There was a problem hiding this comment.
In this file, I only removed the MultifactorAuthenticationContextProviders, and because it was a wrapper, the GitHub diff shows that many changes.
- Use scheduleOnRN instead of deprecated runOnJS - Use progress.set()/get() instead of deprecated .value - Add sentryLabel via CONST.SENTRY_LABEL.MFA_OVERLAY.BACKDROP - Fix import alias in TestToolMenu (@components → ./) - Remove unused eslint-disable in stateReducer - Suppress set-state-in-effect in overlay and ValidateCodePage with justification comments
Replace the imperative isVisible setState-in-effect pattern with a render-time prevIsModalOpen mirror and a derived isVisible. The effect now lists every referenced dep, removing the exhaustive-deps and set-state-in-effect disables.
Three iOS-specific issues addressed: - Navigation buffer race: on iOS native-stack, INITIAL.onLayout fires synchronously on mount — before process() reaches navigate() — so applyPendingNavigation runs with an empty buffer, and the buffered push set by navigate() never fires. Add a hasInitialLaidOut module flag; once true, navigate() pushes directly instead of buffering. - White flash on first open: INITIAL_SCREEN inherited the opaque app background from screenOptions.native.contentStyle. Set explicit transparent contentStyle (and cardStyle for web) per-screen. - Crash on close: the original close effect used a withTiming completion callback that called scheduleOnRN to update React state on the JS thread. The worklet → JS bridge raced against the native pop animation triggered by goBack(), and the resulting unmount hit an intermediate native-stack state and exited the app. Replace the worklet callback path with a plain setTimeout, and return a cleanup that clears the timer if the component unmounts mid-animation.
Use TRANSPARENT_MODAL presentation, disable the default react-navigation card overlay, and switch the cardStyleInterpolator to forHorizontalIOS with a Safari-only fallback to the Expensify modal interpolator. The previous always-custom interpolator was justified by a width=0 race that no longer applies — the TransparentScreen placeholder guarantees a measured layout before the first push.
Dispatch SET_FLOW_COMPLETE alongside CLOSE_MODAL so the state-machine guard in process() short-circuits if any dep field changes during the 300ms exit animation (e.g. user taps the fading backdrop and triggers cancel(), which dispatches SET_ERROR).
Stack history is always [INITIAL, <current>] because non-initial navigations replace the top. iOS swipe-back / Android hardware back popped the active screen and left the transparent INITIAL placeholder focused with the overlay still open, trapping the user on narrow layouts where no backdrop exists. Setting gestureEnabled: false on the shared screenOptions blocks the pop; users still exit via the header back button or backdrop, both of which dispatch CLOSE_MODAL.
clearPendingNavigation() reset pendingNavigation but left hasInitialLaidOut as true across reopens. On web, ResizeObserver-backed onLayout fires async after isReady becomes true, opening a window where navigate() would see a stale hasInitialLaidOut and dispatch a direct push before the INITIAL_SCREEN had laid out for the new session, breaking the slide-in interpolator measurement.
Single `as MultifactorAuthenticationScenarioConfig` is sufficient — the `as unknown as` form weakens type safety with no compiler benefit.
…nd overlay The overlay-internal param list type was duplicated. Export it from mfaNavigation.ts and import it in MultifactorAuthenticationOverlay so the type has a single source of truth.
Add mfaOverlayZIndex to styles/variables and use it in the overlay root style instead of a hardcoded 1000.
Drop useMemo/useCallback usage in MultifactorAuthenticationOverlay. The project relies on React Compiler for automatic memoization, so the manual wrappers added complexity without a clear correctness benefit.
Align with the rest of the overlay components (MultifactorAuthenticationOverlay, OutcomeScreenBase, ...) that all set displayName for nicer debug output.
Replace the `as Record<string, unknown> | undefined` cast with a typed variable annotation so TypeScript proves the assignment instead of forcing it.
Project convention (contributingGuides/STYLING.md) requires non-theme static styles to live in src/styles. Remove the local StyleSheet.create in MultifactorAuthenticationOverlay and expose mfaOverlayRoot through useThemeStyles instead.
…odalNavigator Align the standalone MFA navigator with the *ModalNavigator naming used by peers in src/libs/Navigation/AppNavigator/Navigators/ (RightModalNavigator, OnboardingModalNavigator, etc.). The component owns a stack of screens, so "ModalNavigator" describes it more accurately than "Overlay" and avoids collision with the existing Overlay component (backdrop dimmer). Renames: - File MultifactorAuthenticationOverlay.tsx -> MultifactorAuthenticationModalNavigator.tsx - Component MultifactorAuthenticationOverlay -> MultifactorAuthenticationModalNavigator - Type MultifactorAuthenticationOverlayParamList -> MultifactorAuthenticationModalNavigatorParamList - Type MfaOverlayInternalParamList -> MultifactorAuthenticationModalNavigatorInternalParamList - Style mfaOverlayRoot -> mfaModalNavigatorRoot - Variable mfaOverlayZIndex -> mfaModalNavigatorZIndex "Overlay" is reserved for the backdrop layer (MFA_OVERLAY.BACKDROP Sentry label, backdropAnimatedStyle), which is its actual role here.
…r self-doc - clearPendingNavigation -> resetMfaNavigation. The function resets two pieces of module state (pendingNavigation and hasInitialLaidOut), not just the pending request. The old name invited callers to assume a narrower contract, which risked silently zeroing the laid-out flag. - applyPendingNavigation -> handleInitialScreenLayout. It is wired to the placeholder's onLayout; naming it as a layout handler matches the callsite and surfaces the side effect (flushing buffered nav) as a layout consequence rather than the primary semantic. - progress -> backdropProgress. The shared value drives only the backdrop opacity; the slide is owned by the Stack. The specific name prevents future reuse for card-style transitions.
WojtekBoman
reviewed
May 11, 2026
WojtekBoman
reviewed
May 11, 2026
Browser back and hardware back now surface the scenario's cancel-confirm modal instead of immediately tearing down the MFA flow. The synthetic CUSTOM_HISTORY_ENTRY_MFA_MODAL_NAVIGATOR marker mirrors the existing side-panel pattern, and the new useSyncMfaModalNavigatorWithHistory hook isolates the marker push/pop, back-press detection, and confirm-state plumbing from the navigator. Confirming runs cancel; rejecting keeps the flow open with the URL pointer unchanged. Forward stack is truncated on confirm so the cancelled flow cannot be resurrected. addRootHistoryRouterExtension now preserves the contiguous trailing run of known markers through rehydration, generalising the previous side-panel-only branch.
Each MFA screen previously owned its own cancel-confirmation state and back-press logic. Move it into the context so there is a single decision point shared by hardware/browser back, header back button, focus-trap escape, and the backdrop press. - Add requestCancel/hideCancelConfirm/confirmCancel to the MFA context. requestCancel decides — based on isFlowComplete, scenario, isOffline — whether to close the modal, cancel directly, or surface the confirm. - Move isCancelConfirmVisible into the reducer state to keep the state/actions context split lint rule happy. - Render the cancel-confirmation modal once in the MFA navigator; PromptPage and ValidateCodePage no longer own confirm-modal state. - Move useSyncMfaModalNavigatorWithHistory into the context and split it into two effects so the marker lifecycle is not churned when requestCancel changes. - Collapse handleCallback so SET_FLOW_COMPLETE dispatches once and the outcome-screen branch uses a ternary.
… modalBaseZIndex - Rename INITIAL_SCREEN to MFA_INITIAL_SCREEN to disambiguate at import sites. - Replace one-off mfaModalNavigatorZIndex with the existing modalBaseZIndex variable; drop the now-unused entry from variables.ts.
Resolved conflicts: - src/CONST/index.ts: kept both MFA_OVERLAY and DOMAIN SENTRY_LABEL additions - src/libs/Navigation/AppNavigator/Navigators/RightModalNavigator.tsx: took main version, removed MultifactorAuthenticationContextProviders wrap and its import (provider hoisted to AuthScreens by this PR)
Offline requestCancel previously dispatched SET_ERROR, but process() short-circuits on isOffline, leaving the modal stuck open with no path to handleCallback. Centralize the offline-close decision in cancel() so every entry point (backdrop, hardware back, browser back, confirmCancel) exits the flow immediately when offline.
TestToolMenu called useMultifactorAuthentication() unconditionally, but the provider is only mounted under AuthScreens. Reaching the test-tools modal from SignInPage (4-tap PanResponder) threw "must be used within a MultifactorAuthenticationContextProviders". Move the biometrics row into BiometricsTestToolRow, rendered only when isAuthenticated === true, so the context hook is not invoked pre-auth. Update TestToolMenuBiometricsTest.tsx: mock the new context, drop dead mocks (useSingleExecution, useWaitForNavigation, Navigation), and add executeScenario(BIOMETRICS_TEST) assertion to the Test-button case.
The MFA outcome screen is pushed by the Context after the scenario callback resolves. On Android, when a callback initiates an RHP transition (e.g. CHANGE_PIN's goBack to pop ChangePINPage), the RHP slide-out raced the outcome-screen push and leaked through the outgoing screen. Wrap mfaNavigate in TransitionTracker.runAfterTransitions so the outcome push waits for any active transition to end; if none are active it fires synchronously and other scenarios are unaffected.
…s/internal-navigation-v3
After main's refactor of ModalCardStyleInterpolatorProps, enter became required.
Pass {kind: 'slide-from-width'} to preserve the previous behavior — interpolator
uses variables.sideBarWidth as the slide range on wide layout, matching the
comment about stable slide range regardless of layout timing.
…vel API ESLint restricts direct imports of TransitionTracker — it's an internal primitive that should only be accessed through Navigation/KeyboardUtils/etc. Add a thin Navigation.runAfterTransition helper that wraps TransitionTracker.runAfterTransitions and use it from the MFA Context so the import restriction is satisfied without changing behavior.
…s/internal-navigation-v3 # Conflicts: # src/components/MultifactorAuthentication/config/scenarios/RevealPIN.tsx
Contributor
Author
|
@chuckdries, I resolved the conflicts and fixed the ESLint error. However, I only did a very quick test of the entire flow at the very end, and I’m not sure if the screen flickering bugs are actually fixed in this PR. Before you merge this, it would be good to test everything thoroughly, and if any more bugs come up, please report them to me. I’ll try to make a final round of fixes on Monday, and if everything looks good, you can go ahead and merge it. |
Contributor
|
Reported android issues are both solved 🟢 Reveal PIN worksMFA.refactor.reveal.pin.android.2.success.mp4🟢 change PIN cancel shows correct transitionMFA.refactor.change.pin.android.2.pass.mp4Proceeding with testing on other platforms |
Contributor
|
Android mweb chrome tests - mostly working. One main issue is that reveal PAN has the same issue that reveal PIN had yesterday. A minor NAB I've seen is that pressing the hardware back button dismisses the outcome page and navigates whatever is underneath. 🔴 reveal PAN does not reveal PANMFA.refactor.reveal.PAN.mweb.chrome.fail.mp4⚪ authorize transaction "got it" button vs hardware back button behave differently on failure outcome page (works fine otherwise)MFA.refactor.authorize.transaction.mweb.chrome.back.button.vs.got.it.mp4⚪ change PIN "got it" button vs hardware back button behave differently on failure outcome page (works fine otherwise)MFA.refactor.change.PIN.mweb.chrome.back.button.vs.got.it.behave.differently.mp4🟢 test scenario works fineMFA.refactor.test.mweb.chrome.success.mp4MFA.refactor.test.mweb.chrome.back.buttons.mp4🟢 reveal PIN works fineMFA.refactor.reveal.PIN.mweb.chrome.pass.mp4 |
Contributor
|
iOS native has the same issue with reveal PAN not working. Also, I have twice observed an issue where, after navigating away from the failure outcome screen, the whole app apparently freezes, but I can't get a consistent reproduction 🟢 test scenario works fineMFA.refactor.test.ios.pass.mp4🟢 reveal PIN works fineMFA.refactor.reveal.PIN.ios.success.pass.mp4MFA.refactor.reveal.PIN.ios.back.button.pass.mp4🟢 change PIN works fineMFA.refactor.change.PIN.ios.reject.mp4MFA.refactor.change.PIN.ios.success.pass.mp4🟢 set PIN works, but is a case where I caught the whole app freeze behavior on videoMFA.refactor.set.PIN.success.pass.mp4MFA.refactor.set.PIN.ios.whole.app.freeze.after.reject.mp4🟢 authorize transaction works fineMFA.refactor.authorize.transaction.ios.success.pass.mp4🔴 reveal PAN does not workMFA.refactor.reveal.PAN.ios.fail.mp4MFA.refactor.reveal.PAN.ios.reject.pass.mp4 |
Contributor
|
MacOS Chrome has three issues, two of them already documented on other platforms (and only one blocking) 🔴 Reveal PAN doesn't workMFA.refactor.reveal.PAN.macos.chrome.outcome.page.history.buttons.mp4⚪ Using the browser back button to dismiss the outcome page when there's an RHP below it closes the RHP (vs "Got it" just closes the MFA modal)MFA.refactor.change.PIN.macos.chrome.back.reject.outcome.button.behavior.diff.mp4⚪ Using the browser back button to dismiss the outcome page causes the forward button to become enabled, though pressing it does nothingMFA.refactor.test.macos.chrome.browser.forward.button.enabled.mp4 |
Same root cause as de770f8 for REVEAL_PIN. The success callback called Navigation.closeRHPFlow() + Navigation.navigate(SETTINGS_WALLET_DOMAIN_CARD) right after setRevealedVirtualCardDetails(). With MFA now mounted as a sibling navigator, ExpensifyCardPage stays focused throughout the flow, so closing the RHP unmounted it and ran its useFocusEffect cleanup, which calls clearRevealedVirtualCardDetails() and wiped the PAN/expiration/CVV before the page remounted. Drop the navigation calls — the underlying card page stays put and just re-renders with the new details. SKIP_OUTCOME_SCREEN still closes the MFA modal cleanly.
Contributor
Author
Fixed. |
Contributor
Author
Unfortunately, this is the only trade-off we had to accept, because it’s not possible to remove items from the browser history. So we'll have to leave it at that. |
The MFA navigator refactor left a stale entry for RightModalNavigator.tsx in eslint.seatbelt.tsv. The current code no longer triggers react/jsx-props-no-spreading, so the entry was unreachable and SEATBELT_FROZEN=1 lint passes after removal.
When an MFA scenario callback pops a route before the outcome screen (e.g. ChangePIN), `useLinking` reconciles the new focused path via `history.go(delta)` and discards the marker's synthetic browser entry. `state.history` still carries the marker (router extensions preserve it), but the browser no longer has a matching entry — so a later browser-back lands on an older snapshot and skips past the screen the user expects. Bracket the pop with a strip → pop → re-attach cycle on the trailing marker (`popAndRealignMfaMarker`). Each toggle drives useLinking to push or replace a fresh browser entry mapped to the post-pop path. The MFA sync hook reads `isMfaMarkerStripInProgress` so it does not treat the transient removal as a back-press and fire `requestCancel` mid-pop. Implementation notes: - `pop` callback is `() => void`; the `canGoBack` guard is hoisted out of the wrapper so its semantics are "always pops". - `pop` runs inside try/finally so the re-attach + flag reset still fire if the pop callback throws (no stuck `stripInProgress` flag). - Marker dispatch is centralized in `toggleMfaMarker` and reused by the sync hook to drop a duplicate inline dispatch.
Contributor
Author
That should be fixed by now, after a lot of trouble. Overall, the browser state and React Navigation got out of sync in this flow. So after calling |
`popAndRealignMfaMarker` schedules a re-attach after pop's transition. If `isModalOpen` flips to false during that window (e.g. a future scenario whose callback calls `Navigation.goBack` and returns `SKIP_OUTCOME_SCREEN`), the scheduled re-attach would inject the marker back into history after the modal is already gone. Expose `cancelPendingMfaMarkerReattach` and call it from the sync hook's close cleanup. The cancel is implemented by flipping `stripInProgress` to false — the scheduled callback then no-ops on its own gate check, so no separate handle reference is needed.
…elper Move the transition-aware scheduler out of mfaModalMarkerPreservation and into the goBack call site, so the helper no longer imports the internal TransitionTracker primitive that no-restricted-imports forbids outside Navigation itself.
Contributor
|
On the failure/success screen, clicking outside the RHP causes the card detail page to appear instead of closing all RHPs Screen.Recording.2026-05-25.at.23.50.51.mov |
Contributor
|
Transition to success screen doesn't close the discard modal (not happening on main) Screen.Recording.2026-05-25.at.23.55.24.mov |
Contributor
|
Testing preferences modal should overlap above the MFA RHPs 
Updated: ahh wait... I am not sure if this is a bug because there is a button to test MFA in the testing preferences modal Screen.Recording.2026-05-26.at.00.16.21.mov |
DylanDylann
reviewed
May 25, 2026
Comment on lines
+123
to
+127
| const cleanupTimer = setTimeout(() => { | ||
| resetMfaNavigation(); | ||
| setPhase('closed'); | ||
| dispatch({type: 'RESET'}); | ||
| }, CONST.ANIMATED_TRANSITION); |
Contributor
There was a problem hiding this comment.
Could we replace the setTimeout in the cleanup with something else, like a transition-end listener, so the reset isn't tied to wall clock time?
Contributor
Author
There was a problem hiding this comment.
I fixed the problem with this commit 77075bb
Contributor
Author
Fixed. |
Contributor
Author
@DylanDylann noticed some potential differences/issues compared to the implementation in the main branch. Personally, I think the current implementation in this PR is fine, and possibly even better than what’s in the main branch, but I don’t have any strong opinions on the matter. What do you all think about this? How should we ultimately implement this? |
Contributor
|
It's great to confirm again 💯 |
Contributor
I don't mind this problem; either way is fine to me |
Contributor
For this problem, I think we should only redirect back to the card detail page if users click on the back button or the Got it button |
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
4 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
MFA screens (3DS prompt, magic code, outcome) used to live inside
RightModalNavigatoras regular RHP routes. Because MFA state is held in React context (not Onyx), URL-driven routes could outlive the state they depended on — a reload, deep link, or unrelated RHP navigation could leave the user on a half-initialized MFA screen, exactly the kind of broken state #81021 asks for guards against.New architecture. MFA is moved out of the RHP into a self-contained
MultifactorAuthenticationModalNavigator, mounted as a sibling of the root stack inside aNavigationIndependentTree. The MFA context provider wraps the authenticated app, so any screen can callexecuteScenarioand the navigator mounts on top of whatever is currently visible. MFA routes are removed fromROUTES/SCREENS/ linking config; internal navigation between MFA screens goes through a dedicatedmfaNavigationmodule instead of the globalNavigationAPI.How it behaves. The MFA flow no longer touches the URL or the app navigation state — the page underneath (e.g.
transaction-preview) stays put while MFA runs on top.CUSTOM_HISTORY_ENTRY_MFA_MODAL_NAVIGATOR, mirroring the side-panel pattern) instead of tearing the flow down.All cancel entry points — backdrop, header back, hardware back, browser back, offline — funnel through a single
requestCanceldecision in the MFA context, so there is one source of truth for "should this attempt to close prompt a confirm, run cancel, or just close?".Side cleanups.
BiometricsTestPageremoved — the dev tool now callsexecuteScenariodirectly, so the standalone page no longer has a reason to exist.useMultifactorAuthentication()is only valid underAuthScreens; reaching the test-tools modal fromSignInPagepreviously crashed.process()short-circuits onisOffline, so without the guard the user would be stranded on the transparent placeholder with no way out.onCloseoverride onOutcomeScreenBase—AuthorizeTransactionPage(deny outcome) andChangePINAtATMPagerender the screen inside the RHP, outside the MFA navigator; the defaultCLOSE_MODALdispatch was a no-op there and left the buttons inert.customConfig(undefined)at module-load and crashed thesortTransactionsPending3DSReviewtest suite.Fixed Issues
$ #81021
PROPOSAL:
Tests
General checks (run for every scenario below):
Per-scenario (happy + failure + any scenario-specific checks):
BIOMETRICS-TEST — happy / failure
AUTHORIZE-TRANSACTION — happy / failure; deny outcome rendered inline in RHP — "Got it" / header back close the RHP (no dead button)
SET-PIN-ORDER-CARD — happy / failure
REVEAL-PIN — happy / failure
CHANGE-PIN — happy / failure
Verify that no errors appear in the JS console
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari