[HOLD for payment 2024-04-29][$250] Share log - Chat icon is highlighted in Settings after returning from share destination

[kbecciv]

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.61-0
Reproducible in staging?: y
Reproducible in production?: new feature
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to Settings > About > Troubleshoot.
3. Enable "Client side logging".
4. Click View console debug.
5. Click Share log.
6. Select any share destination.
7. Tap on the back button until app returns to Settings page.

Expected Result:

When returning to Settings page, the profile icon should be highlighted.

Actual Result:

When returning to Settings page, the chat icon is highlighted.
When clicking on profile icon, app reopens the same 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

Add any screenshot/video evidence

Bug6443177_1712647546277.Screen_Recording_20240409_151204_Chrome.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bd717be733d9cef5
• Upwork Job ID: 1777700496702263296
• Last Price Increase: 2024-04-09
• Automatic offers:
• DylanDylann | Reviewer | 0
• bernhardoj | Contributor | 0

[melvin-bot]

Triggered auto assignment to @amyevans (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[kbecciv]

We think that this bug might be related to #wave-collect - Release 1

[amyevans]

Not a blocker

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[bernhardoj]

Proposal

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

Chat icon of bottom tab bar is highlighted after sharing the client side logging to a chat. I can reproduce this on prod too.

What is the root cause of that problem?

The chat icon will be highlighted if the currentTabName is HOME.

App/src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar.tsx

Line 88 in 455dce4

fill={currentTabName === SCREENS.HOME ? theme.iconMenu : theme.icon}

currentTabName takes the topmost bottom tab navigator route name.

App/src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar.tsx

Lines 66 to 69 in 455dce4

	const currentTabName = useNavigationState<RootStackParamList, string | undefined>((state) => {
	const topmostBottomTabRoute = getTopmostBottomTabRoute(state);
	return topmostBottomTabRoute?.name ?? SCREENS.HOME;
	});

When we open settings, the bottom tab navigator will be:
[HOME, SETTINGS]

When we press "Share log" and select the chat, it will navigate to the report.

App/src/pages/settings/AboutPage/ShareLogList/index.tsx

Lines 17 to 18 in 455dce4

	const routeToNavigate = ROUTES.REPORT_WITH_ID.getRoute(reportID);
	Navigation.navigate(routeToNavigate);

Report screen is a central pane, so when we do the navigation, it will find the match bottom tab for report screen and it's HOME (LHN).

App/src/libs/Navigation/linkTo.ts

Lines 160 to 175 in 455dce4

	} else if (
	action.payload.name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR &&
	topmostCentralPaneRoute &&
	(topmostCentralPaneRoute.name !== action.payload.params?.screen || 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,
	});
	}

So, now we have 3 routes in the bottom tab navigator,
[HOME, SETTINGS, HOME]

When we press go back from the report screen, the bottom tab navigator stays unchanged. If we keep press back and arrive at the About page, the root navigator will look like this:

[
bottom tab navigator: [HOME, SETTINGS, HOME],
central pane navigator: [ABOUT],
]

In about page, we set a fallback route to SETTINGS.

App/src/pages/settings/AboutPage/AboutPage.tsx

Line 143 in 458de57

onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS)}

If we press back in about page, the fallback route will be used because of this logic. (about page is a central pane)

App/src/libs/Navigation/Navigation.ts

Lines 223 to 226 in 458de57

	if (isCentralPaneFocused && fallbackRoute && distanceFromPathInRootNavigator === -1) {
	navigate(fallbackRoute, CONST.NAVIGATION.TYPE.FORCED_UP);
	return;
	}

So, the central pane is replaced with the fallback route and that is SETTINGS which is a bottom tab navigator. Now, we have nav stack like this:

[
bottom tab navigator: [HOME, SETTINGS, HOME],
bottom tab navigator: [SETTINGS],
]

The reason the chat icon is highlighted is because getTopmostBottomTabRoute logic takes the first route in the stack and then the last sub-route, that is HOME.

App/src/libs/Navigation/getTopmostBottomTabRoute.ts

Lines 3 to 4 in 458de57

	function getTopmostBottomTabRoute(state: State<RootStackParamList> | undefined): NavigationPartialRoute<BottomTabName> | undefined {
	const bottomTabNavigatorRoute = state?.routes[0];

On large screen (web), when you press the browser back button, both the report screen and the bottom tab route (HOME) is removed.

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

We can update getTopmostBottomTabRoute so it takes the last found bottom tab navigator.

App/src/libs/Navigation/getTopmostBottomTabRoute.ts

Lines 3 to 4 in 458de57

	function getTopmostBottomTabRoute(state: State<RootStackParamList> | undefined): NavigationPartialRoute<BottomTabName> | undefined {
	const bottomTabNavigatorRoute = state?.routes[0];

const bottomTabNavigatorRoute = state?.routes.findLast(route => route.name === 'BottomTabNavigator');

What alternative solutions did you explore? (Optional)

Remove the fallback route from about page back press. This will have the same behavior as WorkspacesListPage.

[DylanDylann]

@bernhardoj I agree with your RCA, It is correct. Could you help to detail your solution?

1. Why should we do that?
2. Is there any effect to another feature?

[bernhardoj]

Why should we do that?

We don't need the extra bottom tab route on a small screen. On a large screen, we need to add it, otherwise, the user will see this:

Is there any effect to another feature?

I think no.

EDIT: oh actually, I found one case.

1. On web, resize the size to small screen
2. Share the log to a chat
3. Resize back to large screen
4. You will see the bottom tab bar shows setting instead of home just like in the image above

[bernhardoj]

Updated my proposal.

[DylanDylann]

@bernhardoj I am confused about your solution. Currently, getTopmostBottomTabRoute gets the last route in "BottomTabNavigator"

App/src/libs/Navigation/getTopmostBottomTabRoute.ts

Line 11 in c7d6926

const topmostBottomTabRoute = bottomTabNavigatorRoute.state.routes.at(-1);

[DylanDylann]

IMO, When going back from the report screen by clicking the back button. The bottom tab navigator should be updated to [HOME, SETTINGS]. What do you think about that?

[bernhardoj]

Currently, getTopmostBottomTabRoute gets the last route in "BottomTabNavigator"

It takes the last route from the found bottom tab navigator, but it takes the first route from the root navigator

The bottom tab navigator should be updated to [HOME, SETTINGS]. What do you think about that?

Not sure, how do we know when to pop the bottom tab? We can't always pop it when pressing back from the report screen.

I think maybe removing the fallback route is the best we can do for now. We previously removed all ROUTES.HOME from fallback here.

[DylanDylann]

@bernhardoj

It takes the last route from the found bottom tab navigator, but it takes the first route from the root navigator

Which is the last route that you mentioned? And also explain why should we do that

[bernhardoj]

[
bottom tab navigator: [HOME, SETTINGS, HOME],
central pane navigator: [ABOUT],
]

If this is the root state, then the first route is bottom tab navigator and the last sub-route is HOME. We do that because we want to get the focused bottom tab route.

[DylanDylann]

If this is the root state, then the first route is bottom tab navigator and the last sub-route is HOME

@bernhardoj It seems it is the current logic. I am not sure I understand your mean, please help to detail your solution

[bernhardoj]

I thought you are asking about the current logic, no?

It takes the last route from the found bottom tab navigator, but it takes the first route from the root navigator

Which is the last route that you mentioned? And also explain why should we do that

[DylanDylann]

@bernhardoj in your proposal, you suggest that:

We can update getTopmostBottomTabRoute so it takes the last found bottom tab navigator.

Please help to detail your solution

[bernhardoj]

Updated my proposal to include the code.

[DylanDylann]

@bernhardoj's proposal looks good to me

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

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

[alexpensify]

Heads up, I will be offline until Tuesday, April 23, 2024, and will not actively watch over this GitHub during that period.

If anything urgent is needed here, please ask for help in the #expensify-open-source Slack Room-- thanks!

[alexpensify]

I checked in and it looks like this PR was merged yesterday. Now, let's wait for automation to kick in.

[amyevans]

Hmm it was deployed to production already which is when the automation should post, so it seems like that failed. I'll update the title manually - due for payment 7 days after 4/22 so 4/29.

[alexpensify]

Thanks! Noted for April 29.

[alexpensify]

Payouts due: April 29, 2024

• Contributor: $250 @bernhardoj
• Contributor+: $250 @DylanDylann

Upwork job is here.

[alexpensify]

Closing - the list above has been paid via Upwork.