[HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$250] Chat - Unable to navigate to LHN, back button loops between WS details & main chat page

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.70-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): applausetester+1088ck@applause.expensifail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL: Applause-Internal team
Issue reported by: Applause-Internal team

Action Performed:

1. Navigate to staging.new.expensify.com or ND app
2. Go to any Workspace chat page
3. Tap + icon >> select Assign task or Submit expense >> Complete the flow
4. Select the created task or expense to go report page
5. Tap on "From" link in header to go back to chat page
6. Tap on header to go to WS details page
7. Tap on back button (User redirected to main chat page)
8. Tap on back button (User again taken back to details page)
9. Repeat steps 7 & 8

Expected Result:

Unable should redirected to LHN, on tapping back button in main chat page

Actual Result:

Unable to navigate to LHN, back button loops between WS details & main chat page

Workaround:

Unknown

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

Bug6468992_1714664353275.RPReplay_Final1714664285.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0110f0cb9e34f0e6c4
• Upwork Job ID: 1787970200405659648
• Last Price Increase: 2024-05-14
• Automatic offers:
• rayane-djouah | Reviewer | 0
• bernhardoj | Contributor | 0

Issue Owner

Current Issue Owner: @johncschuster

[melvin-bot]

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[izarutskaya]

We think this issue might be related to the #vip-vsb.

[bernhardoj]

Proposal

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

The back button loops if we open the report detail page after visiting a linked message of a report.

What is the root cause of that problem?

If we open the detail page from a report, shouldNavigateToTopMostReport will be true.

App/src/pages/ReportDetailsPage.tsx

Lines 269 to 270 in 7483d10

	onBackButtonPress={Navigation.goBack}
	shouldNavigateToTopMostReport={!(route.params && 'backTo' in route.params)}

shouldNavigateToTopMostReport will navigate to the topmost report ID.

App/src/components/HeaderWithBackButton/index.tsx

Lines 154 to 157 in 7483d10

	if (shouldNavigateToTopMostReport && topmostReportId) {
	Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(topmostReportId));
	} else {
	onBackButtonPress();

But the navigation type will be changed to PUSH because the params are different. The topmost report has reportID and reportActionID in the params (linked message), but we navigate to a report without reportActionID.

App/src/libs/Navigation/linkTo.ts

Lines 166 to 179 in 7483d10

	} else if (action.payload.name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR && (isTargetScreenDifferentThanCurrent || areParamsDifferent)) {
	// 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;

If we update the logic to only compares the reportID, this issue won't happen anymore, but it will remove the reportActionID from the param, so the message won't be linked anymore.

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

To solve the issue correctly, we just need to remove shouldNavigateToTopMostReport from the details page.

if we want the user to see the report screen when going back after refresh, then it should be handled in this issue

[melvin-bot]

@johncschuster Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0110f0cb9e34f0e6c4

[melvin-bot]

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

[rayane-djouah]

Reviewing now

[rayane-djouah]

@bernhardoj, Thank you for the proposal. The root cause in your proposal makes sense. For the solution, can you ensure that removing shouldNavigateToTopMostReport from the details page will not cause a regression. specefically, can you check if removing it will not bring back #30101 as shouldNavigateToTopMostReport was introduced in #30289

[rayane-djouah]

if we want the user to see the report screen when going back after refresh, then it should be handled in this issue

This bug is not related to this issue, right?

[bernhardoj]

specefically, can you check if removing it will not bring back #30101 as shouldNavigateToTopMostReport was introduced in #30289

#30101 introduced the shouldNavigateToTopMostReport prop by taking the logic from ReportDetailsPage to the header component (and modified it), but the logic is first introduced in #29268 to fix #29056 and yes it will bring back the issue because pressing back will simply close the current page, that is the details page. If the user refresh while on the details page and press back, the user will be navigated back to the LHN which will be fixed by #41017.

If you want, we can hold for https://www.github.com/Expensify/App/issues/41017

[rayane-djouah]

@bernhardoj, we can hold for that, but, I think the expected result is that when we click the back button, we should navigate to the linked reportAction. Can we find a solution to consider the linked reportAction when navigating to the top most report? I think of using backTo param or a linkedReportActionID prop. Wdyt? This way we don't need to remove shoudNavigateToTopMostReport prop

[bernhardoj]

when we click the back button, we should navigate to the linked reportAction

If you don't refresh the page, then you will be navigated to the previous report. If the previous report has a linked action, then you will see the linked action.

If you refresh and wait for #41017, then going back will show the report without the linked report action ID. This is the behavior on main too, so it's completely oos. We can achieve it by having a new param for the reportActionID for many report-related pages, but it's better to just stick the scope of this issue.

[rayane-djouah]

@bernhardoj - what I mean is provided that the root cause of this issue is when we click the back button we navigate to the top most report with a push type because the params are differents, can we just include the reportActionID (I think we can pass it as a prop) in navigate call?:

App/src/components/HeaderWithBackButton/index.tsx

Lines 154 to 157 in 7483d10

	if (shouldNavigateToTopMostReport && topmostReportId) {
	Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(topmostReportId));
	} else {
	onBackButtonPress();

[bernhardoj]

Ah I see, but that means you would need to modify getTopmostReportId to get the topmost report action ID too. It just makes things more complicated and inconsistent since shouldNavigateToTopMostReport is only enabled in flag and details RHP pages. I actually think shouldNavigateToTopMostReport is not really needed. If you read the logic,

App/src/components/HeaderWithBackButton/index.tsx

Lines 153 to 158 in 1e5c18c

	const topmostReportId = Navigation.getTopmostReportId();
	if (shouldNavigateToTopMostReport && topmostReportId) {
	Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(topmostReportId));
	} else {
	onBackButtonPress();
	}

if there is a topmost report, we want to navigate to the report. It can be achieved by simply going back because going back will go back to the topmost report most of the time. If there is no topmost report, then we also go back.

This also means we don't need to wait for #41017 (my bad) because it's already happening on main for details page too.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[rayane-djouah]

Yeah I tried removing shouldNavigateToTopMostReport and I'm not able to reproduce the bugs that it was added for, so I think it's not needed anymore and we can remove it to fix this bug.

[rayane-djouah]

@bernhardoj's proposal looks good to me.

🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @hayata-suenaga, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[hayata-suenaga]

I agree that we can remove the shouldNavigateToTopMostReport 👍

[bernhardoj]

PR is ready

cc: @rayane-djouah

[rayane-djouah]

Weekly update: PR C+ approved

[rayane-djouah]

Heads up, I'll be mostly offline until June 5th, 2024. I can still review this issue, but my response might be slower. If there is something urgent, please reassign it. Thanks!

[hayata-suenaga]

thank you for the update. we're just waiting for the merge freeze to be lifted so probably not much we need from you 😄

[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.81-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix go back from details page remove the linked report action  #42239

If no regressions arise, payment will be issued on 2024-06-18. 🎊

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

• @bernhardoj requires payment automatic offer (Contributor)
• @rayane-djouah requires payment automatic offer (Reviewer)

[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:

• [@hayata-suenaga] The PR that introduced the bug has been identified. Link to the PR:
• [@hayata-suenaga] 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:
• [@hayata-suenaga] 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:
• [@bernhardoj / @rayane-djouah] Determine if we should create a regression test for this bug.
• [@bernhardoj / @rayane-djouah] 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.
• [@johncschuster] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[johncschuster]

Payment Summary:

Contributor: @bernhardoj - $250 - paid via Upwork
Contributor+: @rayane-djouah - $250 - paid via Upwork

[melvin-bot]

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

• Fix go back from details page remove the linked report action  #42239

If no regressions arise, payment will be issued on 2024-06-20. 🎊

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

• @bernhardoj requires payment automatic offer (Contributor)
• @rayane-djouah requires payment automatic offer (Reviewer)

[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:

• [@rayane-djouah] The PR that introduced the bug has been identified. Link to the PR:
• [@rayane-djouah] 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:
• [@rayane-djouah] 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:
• [@rayane-djouah] Determine if we should create a regression test for this bug.
• [@rayane-djouah] 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.
• [@johncschuster] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[rayane-djouah]

• The PR that introduced the bug has been identified. Link to the PR: Update thread headers and ancestry to deep link back to the original comment using comment linking #39454
• 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: Update thread headers and ancestry to deep link back to the original comment using comment linking #39454 (comment)
• 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: N/A
• Determine if we should create a regression test for this bug. Yes
• 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.

Regression Test Proposal

1. Open a workspace chat
2. Request money
3. Press the request preview
4. Press the "From" link on the header
5. Press the chat header to open the details
6. Go back
7. Verify the request preview is still highlighted
8. (small screen) Go back again
9. Verify you are navigated to the LHN

Do we agree 👍 or 👎

[johncschuster]

I've added the QA issue and will work on payment shortly.

[johncschuster]

@hayata-suenaga I'm trying to make sense of the payment here. As far as I can tell, it looks like @rayane-djouah introduced the bug in this PR, which was reviewed by @rushatgabhane.

It also looks like @bernhardoj has worked on the fix for the regression (here), and @rayane-djouah reviewed that PR. Have I understood that correctly?

[hayata-suenaga]

Yes that seems to be correct. The C+ authored a PR that introduced the regression and the same C+ reviewed the PR that fixed it.

[hayata-suenaga]

Melvin, we're just handling payments 😄

[johncschuster]

Ok! Thanks for confirming my understanding, @hayata-suenaga!

Payment Summary:

@rayane-djouah was originally paid in this issue
@rushatgabhane was originally paid in this issue
@bernhardoj needs to be compensated - $250 via Upwork - THIS HAS BEEN PAID