[HOLD for payment 2024-02-26] [$500] [Wave 8] [Ideal Nav] Refreshing the browser on a Workspace settings page causes double navigation

[hayata-suenaga]

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:
Reproducible in staging?:
Reproducible in production?:
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

Action Performed:

1. Shrink your web browser
2. 🔧 Workspace Settings > Reimbursements > refresh the page > Press your browser's back button

Expected Result:

The back button should take you to the Workspace Settings page

Actual Result:

After the back button is pressed, you're still on the Reimbursements page. You need to click the back button once more to go back to the Settings page.

Workaround:

N/A

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

video -> #33280 (comment)

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010bd1212900e21158
• Upwork Job ID: 1753487643296509952
• Last Price Increase: 2024-02-02
• Automatic offers:
• ntdiary | Reviewer | 0
• bernhardoj | Contributor | 0

[melvin-bot]

Triggered auto assignment to @greg-schroeder (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[dukenv0307]

Proposal

Please re-state the problem that we are trying to solve in this issue.

After the back button is pressed, you're still on the Reimbursements page. You need to click the back button once more to go back to the Settings page.

What is the root cause of that problem?

When we refresh the page, we navigate to the page by deep link here

App/src/libs/actions/Report.ts

Line 2164 in d9fd412

Navigation.navigate(route as Route, CONST.NAVIGATION.ACTION_TYPE.PUSH);

In this case, we are setting the action type as push without checking isActiveRoute is true or not. So we have two page in the stack screen. And then when we click on back button, this page appears again.

App/src/libs/Navigation/linkTo.ts

Line 179 in d9fd412

action.type = CONST.NAVIGATION.ACTION_TYPE.PUSH;

What changes do you think we should make in order to solve the problem?

We should update action.type to replace if the route is currently active route. Otherwise, set it as push

if (isActiveRoute) {
    action.type = CONST.NAVIGATION.ACTION_TYPE.REPLACE;
} else {
    action.type = CONST.NAVIGATION.ACTION_TYPE.PUSH;
}

App/src/libs/Navigation/linkTo.ts

Line 179 in d9fd412

action.type = CONST.NAVIGATION.ACTION_TYPE.PUSH;

What alternative solutions did you explore? (Optional)

NA

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~010bd1212900e21158

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ntdiary (External)

[greg-schroeder]

@ntdiary do you mind reviewing the proposal above? thanks!

[bernhardoj]

My proposal here can solve this too.

[hayata-suenaga]

@bernhardoj could you double post the solution here if you can? and if you can explain how your solution solves both issues at the same time, that would be super helpful 🙇

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The app navigates to the reimbursement page twice when refreshing the web, so we need to press back twice to close it.

What is the root cause of that problem?

When we open a URL on the web, it will open the corresponding page and at the same time call Report.openReportFromDeeplink which will call an extra navigation call with a PUSH type.

App/src/libs/actions/Report.ts

Line 2164 in 3ab4e6e

Navigation.navigate(route as Route, CONST.NAVIGATION.ACTION_TYPE.PUSH);

We already have a logic to not apply the PUSH action if the route destination is the active route.

App/src/libs/Navigation/linkTo.ts

Lines 246 to 248 in 3ab4e6e

	if (!isActiveRoute && type === CONST.NAVIGATION.ACTION_TYPE.PUSH) {
	minimalAction.type = CONST.NAVIGATION.ACTION_TYPE.PUSH;
	}

But after the ideal nav PR, we modify this logic to change the action type to PUSH if the destination is a central pane navigator (which is true for reimbursement page) but the current central pane screen is not a report screen, so we always push another reimbursement page to the stack (or any other central pane route)

App/src/libs/Navigation/linkTo.ts

Lines 162 to 179 in 3ab4e6e

	} else if (
	action.payload.name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR &&
	topmostCentralPaneRoute &&
	(topmostCentralPaneRoute.name !== SCREENS.REPORT || getTopmostReportId(rootState) !== getTopmostReportId(stateFromPath))
	) {
	// We need to push a tab if the tab doesn't match the central pane route that we are going to push.
	const topmostBottomTabRoute = getTopmostBottomTabRoute(rootState);
	const matchingBottomTabRoute = getMatchingBottomTabRouteForState(stateFromPath, policyID);
	const isNewPolicyID =
	(topmostBottomTabRoute?.params as Record<string, string | undefined>)?.policyID !== (matchingBottomTabRoute?.params as Record<string, string | undefined>)?.policyID;
	if (topmostBottomTabRoute && (topmostBottomTabRoute.name !== matchingBottomTabRoute.name || isNewPolicyID)) {
	root.dispatch({
	type: CONST.NAVIGATION.ACTION_TYPE.PUSH,
	payload: matchingBottomTabRoute,
	});
	}
	action.type = CONST.NAVIGATION.ACTION_TYPE.PUSH;

What changes do you think we should make in order to solve the problem?

We should check whether the route destination is the same as the current central pane route instead of checking whether it's a report screen or not. If it's the same, then don't push it.

topmostCentralPaneRoute.name !== action.payload.params.screen

This will solve #35692 too with the same root cause explained there

[hayata-suenaga]

@ntdiary could you review the proposals when you have time 🙇 ?

[ntdiary]

Ah, I'm terribly sorry, I missed this issue, will review it as soon as possible.

[ntdiary]

So far, I think @bernhardoj's proposal is acceptable, but I'm not entirely sure if there are some reasons or guidelines supporting the current condition (i.e. topmostCentralPaneRoute.name !== SCREENS.REPORT) in the Ideal Nav design doc, could we request access to this doc? @hayata-suenaga

[hayata-suenaga]

I don't think there is a mention about the condition you mentioned in the doc. I'll ask @adamgrzybowski if there was any reason that the condition was modified to check against the report screen instead agains the currently active screen.

[adamgrzybowski]

@ntdiary I think this was just overlooked in the process. You are right we should to check if the destination is the same as current topmost central pane.

We still need to have a separate check for Report route though. We can push multiple different report screens and in that case we need to compare reportID.

[hayata-suenaga]

makes sense. @ntdiary if you still think @bernhardoj's proposal is the best, please post the approval comment 🙇

[ntdiary]

makes sense. @ntdiary if you still think @bernhardoj's proposal is the best, please post the approval comment 🙇

Sure, I think it's fine to move forward with @bernhardoj's proposal. :)

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Current assignee @hayata-suenaga is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

📣 @ntdiary 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[bernhardoj]

PR is ready

cc: @ntdiary

[greg-schroeder]

PR Merged and deployed to staging. Awaiting deploy to prod

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.42-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix refreshing page causes double navigation on workspace page #36206

If no regressions arise, payment will be issued on 2024-02-26. 🎊

For reference, here are some details about the assignees on this issue:

• @ntdiary requires payment automatic offer (Reviewer)
• @bernhardoj requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ntdiary] The PR that introduced the bug has been identified. Link to the PR:
• [@ntdiary] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@ntdiary] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@ntdiary] Determine if we should create a regression test for this bug.
• [@ntdiary] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[greg-schroeder]

Paid @bernhardoj

@ntdiary can you accept the job offer in Upwork and then complete the checklist? Thanks!

[ntdiary]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ntdiary] The PR that introduced the bug has been identified. Link to the PR: [Wave 8] Ideal nav  #33280
• [@ntdiary] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/33280/files#r1505780594
• [@ntdiary] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: No need
• [@ntdiary] Determine if we should create a regression test for this bug. Yes
• [@ntdiary] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again. as shown below
• [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

Regression test steps:

1. Log in with an account that has a workspace.
2. Go to settings page and select a workspace.
3. Open reimbursement menu.
4. Refresh the page.
5. Press back button in the top left corner of the modal.
6. Verify you are navigated to workspace setting page.

---

BTW, I'm applying to switch to NewDot for payment, so it might be better to hold the payment temporarily. :)

[greg-schroeder]

ahh, okay, works for me!

[greg-schroeder]

In that case the payment summary would be:

@ntdiary as C+ $500 - manual request
@bernhardoj as C $500

[greg-schroeder]

Given you'll be making a ND manual request and regression test is filed, closing! If you end up needing to be paid through Upwork, please reopen this.