Remove code that explicitly pushed to MFA outcome screens - #94103
Conversation
|
@cretadn22 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] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
Your solution works, but before we merge it, I want to analyze the problem in depth, because the proposed solution:
- stops using animations in react-navigation, which could cause the animations to become desynchronized in the future
- and adds quite a bit of complexity
Why has the code been working so far, and why did you encounter an error with this issue #94101?
A PR #92837 was merged into main that replaces replace with push for navigation:
// In HybridApp the MFA overlay renders in an independent BaseNavigationContainer presented inside a native
// modal, where StackActions.replace silently fails to commit (the computed state is never applied), stranding
// the user on the spinner-bearing Prompt screen. For the terminal outcome screens we push instead, which
// commits reliably; outcome screens end the flow (the stack is reset when the overlay closes) so the extra
// stack entry has no downside. Standalone keeps replace, where it works as expected.
if (CONFIG.IS_HYBRID_APP && OUTCOME_SCREENS.has(screen)) {
mfaNavigationRef.dispatch(StackActions.push(screen, params));
return;
}As a result, the animation to the initial screen stopped working because it had previously been implemented using:
if (mfaNavigationRef.isReady() && mfaNavigationRef.canGoBack()) {
mfaNavigationRef.goBack();
}A minimal fix that restores the previous behavior is to always force a return to the initial screen using popToTop (This still needs to be tested):
if (mfaNavigationRef.isReady() && mfaNavigationRef.canGoBack()) {
mfaNavigationRef.dispatch(StackActions.popToTop());
}However, I’m still wondering why the PR #92837 stated that a “replace” action cannot be performed in a hybrid app, and I plan to investigate this, as it could cause potential problems in the future.
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product 👍
|
Hey @heekinho, can you provide more context on this change to comment.out.mfa.push.od-.nd.transition.mp4The issue is that |
|
Hmmm. @chuckdries, with replace it was closing the App and returning to OD instead of returning to card details. |
|
@heekinho yes, when the biometrics succeeds, it works as expected (though in this case reveal details doesn't go to an OUTCOME_SCREEN, so that if statement wouldn't hit anyway). For reveal details, the only time the user sees an outcome screen is if it fails. Do you recall the specific flow where you spotted that behavior? Might've been a red herring |
|
Given the commit "Adjust MFA cancel or not matched flow for hybrid app" it was indeed for the failure. Have you try it for iOS? Maybe that was platform specific? Lemme configure it here and try to help you figuring it out. |
|
Success and failure cases work fine in my testing on iOS ios.od-.nd.mfa.nav.mp4I went ahead and reverted "animating the whole MFA stack", and committed removing the .push for outcome screens, so that this PR accurately reflects what we're talking about doing |
|
Ok, after testing it just figured it out what happened: testing-push-issue.movThe video shows the process of me trying to fix that issue. In the first moment when MFA is canceled it just goes straight to OD. That change you commented fixed that first bit. After that, the second issue was that hitting "Got it" was taking it to OD. The second change in that PR fixed that second problem. But the first one seems indeed unnecessary and even has a bad flash. Removing it fixes everything and doesn't have that bad flash. But trying to figure it out those navigation issues was not so clean as this recorded video atm 🫠 . I did two changes in that commit, the second seems to be the important one. So this one you removed is indeed unnecessary and I can see the bad flash with it. So yep, @chuckdries, you probably cannot reproduce it because the second change is the actual fix. I apologize for that. Indeed it is a code that I'm not super familiar and got going trying to fix all edge cases I had while testing. |
|
@chuckdries I think you need to remove the unused variables now |
|
It's great that you were able to find this solution. I'm wondering if it might still be worth implementing the change below, since it makes logical sense and could potentially protect us from future bugs. On the other hand, it might also mask an underlying issue. I'll leave that up to you to consider.
|
It is semantically correct and makes the code less brittle. I went ahead and committed that change. |
heekinho
left a comment
There was a problem hiding this comment.
Tested the last changes with popToTop(). All good
testing-pop-to-top.mov
|
Sweet, thanks for confirming! @DylanDylann would you mind reviewing and checklisting? I'd like to get this visual polish merged sooner rather than later. (feel free to say no if you're too busy!) |
|
@chuckdries The code changes look good to me, but I'm unable to test the transaction approval flow on my end. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
DylanDylann
left a comment
There was a problem hiding this comment.
I'm okay with merging this, since most of the changes simply revert the previous PR, and the author has also confirmed the fix. Note that I haven't re-tested this flow, as I don't have access to it.
|
@neil-marcellini 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] |
neil-marcellini
left a comment
There was a problem hiding this comment.
Testing videos and code look good, and there was a great discussion on this PR which explains why the removed comment and code was invalid. Great work!
|
🚧 neil-marcellini 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/neil-marcellini in version: 9.4.21-2 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.21-9 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
When dismissing the MFA navigator, using
goBackwhen the outcome screen waspushed on top of the prompt screen reveals the prompt screen underneath it. We should be usingreplacehere.Fixed Issues
$ #94101
PROPOSAL:
Tests
Also confirm other MFA flows look right
Offline tests
N/A
QA Steps
N/A - QA cannot test 3DS for now
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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.3ds.challenge.mfa.prompt.fixed.mp4
Android: mWeb Chrome
android.chrome.3ds.challenge.mfa.fixed.mp4
iOS: Native
mobile.3ds.challenge.mfa.prompt.fixed.mp4
iOS: mWeb Safari
ios.safari.3ds.challenge.mfa.fixed.mp4
MacOS: Chrome / Safari
web.3ds.challenge.mfa.prompt.still.works.mp4