-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HOLD for payment 2024-11-22] [$250] iOS Xero - Navigation issue when tapping app back button three times after exiting 2FA setup #50695
Comments
Triggered auto assignment to @twisterdotcom ( |
We think that this bug might be related to #wave-collect - Release 2 |
@twisterdotcom FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors |
Edited by proposal-police: This proposal was edited at 2024-10-20 17:59:03 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.
What is the root cause of that problem?we pass the backTo route to it here:
What changes do you think we should make in order to solve the problem?We don't need to pass the backTo
just write: this change solve the issue mentioned in the OP, and does not cause any regression for 2FA used in the Security page,. How does the above change work when visiting 2FA page directly via URL - How is the accounting page added as the backTo in the navigation stack? When we open the 2FA page directly through the URL with the
So even when accessing the 2FA page directly, the We can actually verify this behavior by testing: if we remove the if (route.params && 'backTo' in route.params && typeof route.params.backTo === 'string') {
const stateForBackTo = getStateFromPath(route.params.backTo, config);
if (stateForBackTo) {
// If we know that backTo targets the root route (full screen) we want to use it.
const fullScreenNavigator = stateForBackTo.routes.find((rt) => rt.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR);
if (fullScreenNavigator && fullScreenNavigator.state) {
return fullScreenNavigator as NavigationPartialRoute<CentralPaneName | typeof NAVIGATORS.FULL_SCREEN_NAVIGATOR>;
}
}
} FullScreenNavigator will have settings/workspaces/{workspaceId}/accounting as path with it. App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts Lines 127 to 131 in 608ec61
here is all the related code: App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts Lines 116 to 126 in 608ec61
Note: For better verification, we can add a breakpoint at Screen.20Recording.202024-10-20.20at.2011.mp4iOS Screen.Recording.2024-10-14.at.12.26.00.AM.mov |
ProposalPlease re-state the problem that we are trying to solve in this issue.Navigation issue when tapping app back button three times after exiting 2FA setup What is the root cause of that problem?When we press the go back button inside the TwoFactorAuth page, we invoke this function App/src/libs/actions/TwoFactorAuthActions.ts Lines 21 to 24 in 2329c78
We pass the fallback route which leads to this issue App/src/libs/Navigation/Navigation.ts Lines 201 to 205 in 2329c78
What changes do you think we should make in order to solve the problem?Remove the backTo(fallbackRoute) What alternative solutions did you explore? (Optional) |
Job added to Upwork: https://www.upwork.com/jobs/~021845826997685288640 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @hoangzinh ( |
Edited by proposal-police: This proposal was edited at 2024-10-14 20:31:49 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.iOS Xero - Navigation issue when tapping app back button three times after exiting 2FA setup What is the root cause of that problem?When we press back button goBack is called with fallbackRoute
but because it is first route in the navigator it will navigate to the fallbackRoute here App/src/libs/Navigation/Navigation.ts Line 235 in 8ff185a
which will replace the 2FA route with the accounting page. That will create a duplicate workspace accounting route to the navigation state. What changes do you think we should make in order to solve the problem?Instead of navigating to the fallBackRoute here with Replace action we should App/src/libs/Navigation/Navigation.ts Line 235 in 8ff185a
This will create the same effect as the navigating with What alternative solutions did you explore? (Optional)We can also avoid having backTo here
for the case of Xero so change it to
because we navigate to App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts Lines 127 to 132 in 5103b4b
and as we will not remove the backTo from the route it will work perfect. |
Thanks for all your proposals.
Btw, I feel that we haven't dug deep enough the root cause yet |
@Shahidullah-Muffakir, @NJ-2020 oe, @FitseTLT - do any of you want to take another stab at this or respond to @hoangzinh? |
@twisterdotcom @hoangzinh , I have updated my proposal #50695 (comment) , thank you. |
@Shahidullah-Muffakir, thank you for the updates. Your proposal still does not explain why passing |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@hoangzinh Updated |
@hoangzinh,Sorry for the delayed reply. I'm not 100% sure if my response will fully clarify things, but here's what I observed:
this triggers the following condition in the Navigation.goBack function: App/src/libs/Navigation/Navigation.ts Lines 234 to 237 in 65b739d
This seems to add the accounting page twice in the navigation stack, causing the issue where we keep getting stuck on that page. However, when we remove the backTo parameter from this line:
the following line executes in Navigation.goBack: App/src/libs/Navigation/Navigation.ts Line 278 in 65b739d
It seems that when we open the page via the direct URL, the backTo is in the URL, and the app correctly navigates back to the accounting page using navigationRef.current.goBack() without needing to explicitly pass the backTo parameter. Attaching a screen recording of accessing the page directly via the URL in Mobile Safari Screen.Recording.2024-10-23.at.2.35.03.AM.mov |
Thanks for updates @FitseTLT @Shahidullah-Muffakir I will manage to review both soon. |
@FitseTLT, Thanks for updates. Can you explain this a little bit? When did we use |
@Shahidullah-Muffakir Thanks for updates. Yep, that is the thing we need to find out how this magic works. RCA needs to be clear to ensure we reach the root cause. Can you help to deep dive to understand your assumption here? "without needing to explicitly pass the backTo parameter" Thanks |
@hoangzinh when we navigate to it with type App/src/libs/Navigation/Navigation.ts Lines 234 to 235 in f46bce0
we will set the action to REPLACE hereApp/src/libs/Navigation/linkTo/index.ts Lines 114 to 115 in f46bce0
therefore the nav action will replace the current route with the fallbackRoute as it doesn't know the route before that is the same as the fallbackRoute we will end up having duplicate routes in nav state. So what we should do is goBack to remove the current route from the nav state and then navigate to the fallBackRoute in which case if the prevRoute is the same as the fallbackRoute it will leave it as it is and not create a duplicate route which will solve similar issues for the future too.
|
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@hoangzinh I have updated to give an alternative approach of passing empty backTo (for 2FA page only for the case of Xero) as I have proposed initially along with the analysis for the reason it will work for deep link case 👍 |
@hoangzinh , When we open the 2FA page directly through the URL with the
So even when accessing the 2FA page directly, the We can actually verify this behavior by testing: if we remove the if (route.params && 'backTo' in route.params && typeof route.params.backTo === 'string') {
const stateForBackTo = getStateFromPath(route.params.backTo, config);
if (stateForBackTo) {
// If we know that backTo targets the root route (full screen) we want to use it.
const fullScreenNavigator = stateForBackTo.routes.find((rt) => rt.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR);
if (fullScreenNavigator && fullScreenNavigator.state) {
return fullScreenNavigator as NavigationPartialRoute<CentralPaneName | typeof NAVIGATORS.FULL_SCREEN_NAVIGATOR>;
}
}
} FullScreenNavigator will have settings/workspaces/{workspaceId}/accounting as path with it. App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts Lines 127 to 131 in 608ec61
here is all the related code: App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts Lines 116 to 126 in 608ec61
Note: For better verification, we can add a breakpoint at |
@FitseTLT @Shahidullah-Muffakir Thanks for keeping us updated. I appreciate it. After reviewing, testing both solutions and based on the suggestion here. I think @Shahidullah-Muffakir is good for now. Can you add what you found here in your proposal? |
|
@hoangzinh updated my Proposal, Thank you. |
@FitseTLT Oh wait, you're right. @Shahidullah-Muffakir's solution will cause a regression bug in the Reimbursement flow Screen.Recording.2024-10-30.at.05.28.29.mov |
@FitseTLT is correct - while I only tested the regression on the Security page navigation, I missed testing the Reimbursement flow mentioned. Thank you. |
Cool. Thanks to everyone for putting your effort into finding a root cause and a good solution for now. We can go with the alternative solution of @FitseTLT Link to proposal #50695 (comment) 🎀👀🎀 C+ reviewed |
Triggered auto assignment to @stitesExpensify, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
📣 @FitseTLT 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.62-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-11-22. 🎊 For reference, here are some details about the assignees on this issue:
|
@hoangzinh @twisterdotcom @hoangzinh The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button] |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 9.0.48-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team
Action Performed:
Expected Result:
App will return to workspace list.
Actual Result:
App returns to Accounting.
Workaround:
Unknown
Platforms:
Screenshots/Videos
Bug6633335_1728817384645.ScreenRecording_10-13-2024_18-53-12_1.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @twisterdotcomThe text was updated successfully, but these errors were encountered: