[Awaiting Payment 30th May] [Search v1] Chat - "From" link in IOU report details page navigates to the main chat twice

[izarutskaya]

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: 1.4.67-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Launch New Expensify app.
2. Submit an expense to a user.
3. In 1:1 DM, tap on the IOU preview.
4. Tap on the chat header.
5. Tap "From" link.
6. Tap on the back button.

Expected Result:

App will return to report details page.

Actual Result:

App returns to the same main chat. It only returns to report details page after tapping on the back button again.

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

Bug6464770_1714364034837.RPReplay_Final1714363561.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c414e0c471cc81d1
• Upwork Job ID: 1786350359808901120
• Last Price Increase: 2024-05-03
• Automatic offers:
• s77rt | Reviewer | 0
• bernhardoj | Contributor | 0

Issue Owner

Current Issue Owner: @s77rt

[melvin-bot]

Triggered auto assignment to @trjExpensify (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 #collect project.

[bernhardoj]

Proposal

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

The navigation to the parent report happens twice.

What is the root cause of that problem?

When we press the "From" link, it will call goBack with the report screen as the fallback which will be used twice. One without the report action ID and the other one with the report action ID for comment linking.

App/src/components/ParentNavigationSubtitle.tsx

Lines 38 to 43 in 35890a0

	// Pop the thread report screen before navigating to the chat report.
	Navigation.goBack(ROUTES.REPORT_WITH_ID.getRoute(parentReportID));
	if (isVisibleAction && !isOffline) {
	// Pop the chat report screen before navigating to the linked report action.
	Navigation.goBack(ROUTES.REPORT_WITH_ID.getRoute(parentReportID, parentReportActionID));
	}

The issue happens after this PR where we will push the screen if the params is different, in this case, the report action ID param.

App/src/libs/Navigation/linkTo.ts

Lines 155 to 176 in 35890a0

	const isTargetScreenDifferentThanCurrent = Boolean(topmostCentralPaneRoute && topmostCentralPaneRoute.name !== action.payload.params?.screen);
	const areParamsDifferent = !shallowCompare(topmostCentralPaneRoute?.params, action.payload.params?.params);
	// In case if type is 'FORCED_UP' we replace current screen with the provided. This means the current screen no longer exists in the stack
	if (type === CONST.NAVIGATION.TYPE.FORCED_UP) {
	action.type = CONST.NAVIGATION.ACTION_TYPE.REPLACE;
	// If this action is navigating to the report screen and the top most navigator is different from the one we want to navigate - PUSH the new screen to the top of the stack
	} 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;

Previously, we only compare the report ID to decide whether to push the report screen or not.

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

I think we want to have the old behavior, that is to push the report screen only if the topmost report ID is different than the destination report ID, if it's other screen, then we will use the current condition.

const areParamsDifferent = action.payload.params?.screen === SCREENS.REPORT
    ? getTopmostReportId(rootState) !== getTopmostReportId(stateFromPath)
    : !shallowCompare(topmostCentralPaneRoute?.params, action.payload.params?.params);

[melvin-bot]

@trjExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[trjExpensify]

@luacmartins @grgia putting this on your radar! I'll file it under Search v1.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01c414e0c471cc81d1

[melvin-bot]

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

[s77rt]

@bernhardoj Thanks for the proposal. Your RCA makes sense. Can we get ride of the double navigation? That change is also recent #40315.

[bernhardoj]

@s77rt I think we can't get rid of the double goBack. If I understand correctly from that PR, when we press the "From" link, we want to GO BACK to the parent thread. The PR previously attempted to have 1 go back as shown below,

but it doesn't work because getDistanceFromPathInRootNavigator compares the fallback route path with the parent thread route path in the root stack.

App/src/libs/Navigation/Navigation.ts

Lines 121 to 124 in 90b67a5

	const pathFromState = getPathFromState(currentState, linkingConfig.config);
	if (path === pathFromState.substring(1)) {
	return index;
	}

App/src/libs/Navigation/Navigation.ts

Lines 224 to 236 in 90b67a5

	const distanceFromPathInRootNavigator = getDistanceFromPathInRootNavigator(fallbackRoute ?? '');
	// Allow CentralPane to use UP with fallback route if the path is not found in root navigator.
	if (isCentralPaneFocused && fallbackRoute && distanceFromPathInRootNavigator === -1) {
	navigate(fallbackRoute, CONST.NAVIGATION.TYPE.FORCED_UP);
	return;
	}
	// Add possibility to go back more than one screen in root navigator if that screen is on the stack.
	if (isCentralPaneFocused && fallbackRoute && distanceFromPathInRootNavigator > 0) {
	navigationRef.current.dispatch(StackActions.pop(distanceFromPathInRootNavigator));
	return;
	}

The fallback route path contains a reportActionID, while the parent thread in the stack doesn't contain a reportActionID, so FORCED_UP navigation is used instead as shown in this video.

[luacmartins]

CC @adamgrzybowski @WojtekBoman @Kicu if you wanna take a look at this one.

[s77rt]

@bernhardoj With the two goBack we still end up with a FORCED_UP navigation. With one goBack are you still able to reproduce this bug #40315 (review)?

[bernhardoj]

Yes, I can reproduce it with one goBack.

Screen.Recording.2024-05-07.at.11.56.35.mov

The real issue is actually the send button becomes unresponsive when coming back from a thread. We have an issue for that here. So, the double goBack is a workaround so the user doesn't need to press back to go back to the parent chat.

If we fix the unresponsive button issue, then I think we won't need the double goBack anymore. The issue might be related to fabric updates. The gesture here doesn't respond at all.

App/src/pages/home/report/ReportActionCompose/SendButton.tsx

Lines 26 to 28 in 84118d8

	const Tap = Gesture.Tap().onEnd(() => {
	handleSendMessage();
	});

With the two goBack we still end up with a FORCED_UP navigation.

With the solution that I propose, at least there won't be 2 report screen pushes to the nav stack anymore. There will only be 1 report screen pushed and it will be replaced with a report screen that has a reportActionID param.

[s77rt]

@bernhardoj Thanks for checking. I was able to reproduce the bug too. I think we should hold on #41528 and then remove the double goBack.

With the solution that I propose, at least there won't be 2 report screen pushes to the nav stack anymore

This could break #40280

[bernhardoj]

Why will it break #40280? The areParamsDifferent condition will still compare the whole params if it's not a report screen. The solution will fix this too.

[s77rt]

@bernhardoj What I meant to say is that change is intended and it's probably needed to fix some case, reverting may cause an issue but perhaps we can fix the issue they faced in another way. I have left a comment here #40280 (comment)

[s77rt]

@bernhardoj We got some context; That change is only needed for the new search page, for the reports we should rely only on the report ids as you suggested 👍

🎀 👀 🎀 C+ reviewed
Link to proposal

[melvin-bot]

Current assignee @luacmartins is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

📣 @s77rt 🎉 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: @s77rt

[melvin-bot]

This issue has not been updated in over 15 days. @trjExpensify, @s77rt, @luacmartins, @bernhardoj eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[trjExpensify]

Payment summary as follows:

• $250 to @bernhardoj for the fix
• $250 to @s77rt for the C+ review

Paid. Closing!