Adjust MFA flow for hybrid - #92837
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@madmax330 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee38cba9f2
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR adjusts the hybrid-app MFA overlay navigation behavior to prevent NewDot from being closed mid-MFA and to ensure terminal MFA outcome screens reliably render when running inside an independent navigation container in the HybridApp modal flow.
Changes:
- Prevent
ScreenWrapperfrom closing the hybrid React Native app while the MFA overlay navigation tree is mounted/closing. - In HybridApp, switch MFA terminal outcome navigation from
StackActions.replacetoStackActions.pushto avoid a replace-state-commit failure.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/components/ScreenWrapper/index.tsx | Skips closeReactNativeApp() when the MFA overlay navigation ref is ready to avoid exiting NewDot during MFA teardown. |
| src/components/MultifactorAuthentication/mfaNavigation.ts | Uses push (HybridApp only) for MFA outcome screens where replace can fail in the independent modal navigation container. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
Here are the videos for the retest after changes Happy-Path.movEUR-Ask-Details.mov |
|
🚧 @madmax330 has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/madmax330 in version: 9.4.9-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 No help site changes required. I reviewed the changes in this PR and confirmed they do not require any updates to the help site files under Why: This is an internal navigation bug fix scoped entirely to the HybridApp, with no user-facing behavior change:
The user-facing card-details reveal flow (Wallet → virtual card → Reveal → validate code / Face ID → see details) already exists and behaves the same from the user's perspective — this PR only repairs broken navigation in the hybrid build. The existing help articles that cover virtual cards and revealing card details remain accurate, so no draft docs PR was created. @heekinho, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR Note: no help site PR was created because no documentation changes are needed. If you believe a specific article should be updated, let me know which one and I'll draft it. |
|
@mitarachim This App fix is just preparing ground for |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.4.9-0 🚀
|
|
Deploy Blocker #94094 was identified to be related to the PR https://github.com/Expensify/Mobile-Expensify/pull/13968 that attempted to fix this issue. |
|
Not actually related to this PR as mentioned here #94094 (comment) but I created a small bugfix PR here: https://github.com/Expensify/Mobile-Expensify/pull/13981 |
Explanation of Change
As discussed here, we need to update OD virtual card Reveal Details button to link to the card details page in ND. While adding support to OD, it was noticed an odd behavior when trying to reveal the card details. It would close ND and return the user to OD without showing.
Notice that these changes are guarded by
CONFIG.IS_HYBRID_APPso we are not affecting the ordinary flow of App.Fixed Issues
Part of https://github.com/Expensify/Expensify/issues/639960
MOBILE-EXPENSIFY: https://github.com/Expensify/Mobile-Expensify/pull/13968
Tests
test-eur-details.mov
Offline tests
QA Steps
Do regression tests in App:
Repeat the same flow for a UK/EU card
For the Mobile App build:
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