[HOLD for Payment 2024-10-07][$250][Search v2.1] Web - App navigates from search page to inbox when refreshing

[lanitochka17]

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.16
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Click on account setting
3. Click on FAB
4. Click Submit Expense
5. Input any amount & account and submit the expense
6. Go to search
7. Select the expense on the checkbox and click on the drop down on the top right corner and click on hold
8. Open the expense by clicking on it
9. Refresh the page
10. The background of the app navigates to the inbox

Expected Result:

App stays on inbox page when refreshing

Actual Result:

App navigates from search page to inbox when refreshing while its showing Hold banner 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

Add any screenshot/video evidence

Bug6554941_1722111871260.2024-07-27_23_01_54.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~015e208924e7bade60
• Upwork Job ID: 1820124885802480794
• Last Price Increase: 2024-08-18
• Automatic offers:
• rayane-djouah | Reviewer | 103652362

[melvin-bot]

Triggered auto assignment to @lschurr (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.

[lanitochka17]

@lschurr 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

[lanitochka17]

We think that this bug might be related to #vip-vsp

[bernhardoj]

Proposal

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

Inbox page is shown after refresh while hold educational page is showing in search page.

What is the root cause of that problem?

The hold educational page can be accessed from the report screen (central pane) and report RHP, so we don't have the central pane mapping for the hold educational page.

If a RHP page doesn't have the matching route from the mapping, then a report (of inbox) is shown by default when we refresh the web.

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Lines 188 to 196 in 4bce838

	let matchingRootRoute = getMatchingRootRouteForRHPRoute(focusedRHPRoute);
	const isRHPScreenOpenedFromLHN = focusedRHPRoute?.name && RHP_SCREENS_OPENED_FROM_LHN.includes(focusedRHPRoute?.name as RHPScreenOpenedFromLHN);
	// This may happen if this RHP doens't have a route that should be under the overlay defined.
	if (!matchingRootRoute || isRHPScreenOpenedFromLHN) {
	metainfo.isCentralPaneAndBottomTabMandatory = false;
	metainfo.isFullScreenNavigatorMandatory = false;
	// If matchingRootRoute is undefined and it's a narrow layout, don't add a report screen under the RHP.
	matchingRootRoute = matchingRootRoute ?? (!isNarrowLayout ? {name: SCREENS.REPORT} : undefined);
	}

But we actually have another way to show the correct central pane screen, that is by using backTo params which we don't use for hold educational page.

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Lines 106 to 131 in 4bce838

	if (route.params && 'backTo' in route.params && typeof route.params.backTo === 'string') {
	const stateForBackTo = getStateFromPath(route.params.backTo, config);
	if (stateForBackTo) {
	// eslint-disable-next-line @typescript-eslint/no-shadow
	const rhpNavigator = stateForBackTo.routes.find((route) => route.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR);
	const centralPaneOrFullScreenNavigator = stateForBackTo.routes.find(
	// eslint-disable-next-line @typescript-eslint/no-shadow
	(route) => isCentralPaneName(route.name) || route.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR,
	);
	// If there is rhpNavigator in the state generated for backTo url, we want to get root route matching to this rhp screen.
	if (rhpNavigator && rhpNavigator.state) {
	const isRHPinState = stateForBackTo.routes[0].name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR;
	if (isRHPinState) {
	return getMatchingRootRouteForRHPRoute(findFocusedRoute(stateForBackTo) as NavigationPartialRoute);
	}
	}
	// If we know that backTo targets the root route (central pane or full screen) we want to use it.
	if (centralPaneOrFullScreenNavigator && centralPaneOrFullScreenNavigator.state) {
	return centralPaneOrFullScreenNavigator as NavigationPartialRoute<CentralPaneName | typeof NAVIGATORS.FULL_SCREEN_NAVIGATOR>;
	}
	}
	}

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

First, we need to add the backTo param to the hold educational page route.

// in ROUTES.ts
PROCESS_MONEY_REQUEST_HOLD: {
    route: 'hold-expense-educational',
    getRoute: (backTo?: string) => getUrlWithBackToParam('hold-expense-educational', backTo),
},

Then, we need to update all the usages of ROUTES.PROCESS_MONEY_REQUEST_HOLD and make sure to pass the current active route, for example

App/src/components/MoneyRequestHeader.tsx

Lines 111 to 117 in 4bce838

	if (isSmallScreenWidth) {
	if (Navigation.getActiveRoute().slice(1) === ROUTES.PROCESS_MONEY_REQUEST_HOLD) {
	Navigation.goBack();
	}
	} else {
	Navigation.navigate(ROUTES.PROCESS_MONEY_REQUEST_HOLD);
	}

if (isSmallScreenWidth) {
    if (Navigation.getActiveRoute().slice(1) === ROUTES.PROCESS_MONEY_REQUEST_HOLD.route) {
        Navigation.goBack();
    }
} else {
    Navigation.navigate(ROUTES.PROCESS_MONEY_REQUEST_HOLD.getRoute(Navigation.getActiveRoute()));
}

This way, the backTo logic of getMatchingRootRouteForRHPRoute will be used, but there is currently another issue. When we open the hold educational page from the report RHP, the backTo will be the report RHP. If the backTo is an RHP, it will check if stateForBackTo.routes[0].name equals to NAVIGATORS.RIGHT_MODAL_NAVIGATOR.

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Lines 118 to 124 in 4bce838

	if (rhpNavigator && rhpNavigator.state) {
	const isRHPinState = stateForBackTo.routes[0].name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR;
	if (isRHPinState) {
	return getMatchingRootRouteForRHPRoute(findFocusedRoute(stateForBackTo) as NavigationPartialRoute);
	}
	}

However, the stateForBackTo of report RHP is

{
...,
routes: [{name: 'BottomTabNavigator'}, {name: 'RightModalNavigator', ...}],

so, isRHPinState is always false. I think we don't need to check for isRHPinState anymore because we already found the RHP navigator which means RHP is in the state, so I propose to remove isRHPinState.

App/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts

Lines 109 to 110 in 4bce838

	// eslint-disable-next-line @typescript-eslint/no-shadow
	const rhpNavigator = stateForBackTo.routes.find((route) => route.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR);

We can apply this backTo solution to report details and other pages too.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~015e208924e7bade60

[melvin-bot]

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

[rayane-d]

Reviewing today 👀

[rayane-d]

@luacmartins - This bug is reproducible if we open any transaction edit pages or details pages and then refresh. Do you think we should fix it for all pages?

Screen.Recording.2024-08-07.at.2.37.44.PM.mov

[luacmartins]

Yea, we should fix this for all these pages.

[rayane-d]

On a small screen, the app returns to the home page instead of the search page after the refresh:

Screen.Recording.2024-08-07.at.2.42.58.PM.mov

[rayane-d]

@bernhardoj - Could you please update your proposal to fix this for all report pages? thanks!

[raza-ak]

Edited by proposal-police: This proposal was edited at 2024-08-15 04:08:17 UTC.

Proposal

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

App navigates from search page to inbox when refreshing

What is the root cause of that problem?

The issue is the change of base url that will lead the app to navigate to report inbox section because of having only
report id in API data.

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

Report and Search Component Updates: We’ve updated the Report and Search components to navigate back to the search page and open the "hold-expense-educational" modal. However, after these updates, the app navigates back to the search page from the inbox page upon reload. Additionally, any report chat with an "on hold" status is redirected to the search page upon reload.

Types of Reports:

• Submit Expense: Users enter an amount and select a recipient. Upon completion, the expense is displayed in the expenses table.
• Split Expense: Similar to the submit expense, but the expense is split and shown in the expense table.
• Track Expense: Users can track ongoing expenses, which are also shown in the inbox. These can be submitted to someone as well.
• Pay Someone: Similar to submit expense, where the amount and payee are specified.

Navigation Issue Resolution: We need to update the condition to handle navigation issues, ensuring that when moving to other tabs in the search component, auto-navigation to the search section from the expense inbox section upon reload is correctly managed.

UseEffect in Search Component: Add a useEffect to check if the URL contains "hold-expense-educational" and "transaction". This will ensure that the current URL and type are consistent with the sidebar tabs of the search page (e.g., expenses with transaction type). This change will help in navigating to the correct tab and ensuring that on reload, the app shifts to the search > expenses tab.

UseEffect in ReportWelcomeText Component: Add a useEffect to check the initial URL of the hold state and automatically reload to the search page if necessary. This will also allow navigation to other inboxes with a hold status. If the URL contains "/r/", the user will remain on that URL; otherwise, they will be redirected to the search page.

In the src/components/ReportWelcomeText.tsx at line#82.

 useEffect(() => {
        const checkInitialURL = async () => {
            const initialURL = await Linking.getInitialURL();

            if (initialURL && initialURL.includes(ROUTES.PROCESS_MONEY_REQUEST_HOLD) && Navigation.getActiveRouteWithoutParams() && !Navigation.getActiveRouteWithoutParams().includes('/r/')) {
                Navigation.navigate(ROUTES.SEARCH_CENTRAL_PANE.getRoute({ query: CONST.SEARCH.TAB.EXPENSE.ALL }));
            }
        };

        checkInitialURL();
    }, []);

In the src/components/Search/index.tsx at line#113.

useEffect(() => {
        const checkInitialURL = async () => {
            const initialURL = await Linking.getInitialURL();
            if (initialURL && initialURL.includes(ROUTES.PROCESS_MONEY_REQUEST_HOLD) && type == 'transaction') {
                Navigation.navigate(ROUTES.PROCESS_MONEY_REQUEST_HOLD);
            }
        };

        if (shouldShowEmptyState !== false) {
            checkInitialURL();
        }

    }, [navigation]);

Above code will find the initial URL and then nagivate user to initial URL.

New.Expensify.mp4

[rayane-d]

@raza-ak we need to fix the bug for all report pages not just for the "hold-expense-
educational" page

[rayane-d]

Waiting on proposals

[melvin-bot]

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

[tsa321]

Edited by proposal-police: This proposal was edited at 2024-08-15 09:12:45 UTC.

Proposal

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

App navigates to different page when refreshing

What is the root cause of that problem?

The money request edit route can be accessed from SearchNavigator / report RHp and from the central pane navigator, but we don't currently save the last NavigationState. As a result, the app assumes the path is accessed from central pane navigator.

We can persist the navigation state similar to the approach outlined here: https://reactnavigation.org/docs/state-persistence/, but we haven't implemented it yet.

What changes should we make to solve the problem?

We can store the lastNavigationState in Onyx and restore the state as initialState:

App/src/libs/Navigation/NavigationRoot.tsx

Lines 93 to 124 in 0980fcd

	const initialState = useMemo(() => {
	if (!user || user.isFromPublicDomain) {
	return;
	}
	// If the user haven't completed the flow, we want to always redirect them to the onboarding flow.
	// We also make sure that the user is authenticated.
	if (!hasCompletedGuidedSetupFlow && authenticated && !shouldShowRequire2FAModal) {
	const {adaptedState} = getAdaptedStateFromPath(ROUTES.ONBOARDING_ROOT.route, linkingConfig.config);
	return adaptedState;
	}
	// If there is no lastVisitedPath, we can do early return. We won't modify the default behavior.
	if (!lastVisitedPath) {
	return undefined;
	}
	const path = initialUrl ? getPathFromURL(initialUrl) : null;
	// If the user opens the root of app "/" it will be parsed to empty string "".
	// If the path is defined and different that empty string we don't want to modify the default behavior.
	if (path) {
	return;
	}
	// Otherwise we want to redirect the user to the last visited path.
	const {adaptedState} = getAdaptedStateFromPath(lastVisitedPath, linkingConfig.config);
	return adaptedState;
	// The initialState value is relevant only on the first render.
	// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
	}, []);

We should check if the lastVisitedPath matches the path of the initialUrl and, if so, restore the navigation state.

To accomplish this, we can add library functions in userActions/App for updateLastNavigationState and getLastNavigationState. We will call updateLastNavigationState in handleStateChange or on event beforeunload or visibilitychange:

App/src/libs/Navigation/NavigationRoot.tsx

Line 152 in 0980fcd

const handleStateChange = (state: NavigationState | undefined) => {

This will fix many refresh related issues.

Result:

macos-web-d.mp4

The test branch.

What alternative solutions did you explore? (Optional)

[lschurr]

@rayane-djouah could you review the latest proposal?

[rayane-d]

Will review today

[rayane-d]

@tsa321 - I think that using navigation state persistence might be a breaking change. Do we have any simpler solutions?

[raza-ak]

@rayane-djouah Please have a look at my proposal because I've updated my original proposal and fixed the issue for all report pages and attached a new video.

[tsa321]

@rayane-djouah Sorry for the late reply. What exactly is broken?
I have created a test branch to highlight the code changes.
This is the simplest solution I have for now and should address many refresh issues on other pages as well.

[melvin-bot]

This issue has not been updated in over 15 days. @luacmartins, @lschurr, @bernhardoj, @rayane-djouah 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!

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[rayane-d]

PR on production

[rayane-d]

Note

The production deploy automation failed: This should be on [HOLD for Payment 2024-10-07] according to #49855 production deploy checklist, confirmed in #47990 (comment)

cc @lschurr

[rayane-d]

BugZero Checklist

• Please propose regression test steps to ensure the new feature will work correctly on production in further releases.

Regression Test Proposal

• Platforms: MacOS: Chrome / Safari, MacOS: Desktop
• Precondition: Account has many expenses and chats.

1. Navigate to the Search page.
2. Open a report in the Right-Hand Panel (RHP).
3. Access a report-related page in the RHP (e.g., report details page).
4. Refresh the page.
5. Verify that the Search page remains visible under the report RHP.
6. Click on the RHP overlay and verify that the app goes back to the search page.

Do we agree 👍 or 👎

[lschurr]

Thanks! @rayane-djouah @bernhardoj - was this a regression from the original PR?

[rayane-d]

@rayane-djouah @bernhardoj - was this a regression from the original PR?

@lschurr, did you mean the deploy blocker comment mentioned above? @bernhardoj and I addressed it in a follow-up PR (#49916), and the issue is now closed. I would argue that no penalty should be applied here because, in addition to tackling the regression in a follow-up (the regression did not reach production), we did extra work on this issue beyond just fixing the original reported bug. We addressed these types of bugs on all screens opened in RHP from the search page (#46773 (comment), #46773 (comment)). Moreover, the original PR (#47990) was substantial, involving 71 changed files, and required extensive work on both implementation and review sides, making the detection of the regression very challenging given that it's an edge case.
cc @luacmartins

[bernhardoj]

Agree with @rayane-djouah. In fact, I'm hoping the price could be increased 😅

[lschurr]

Payment summary:

• Contributor: $250 @bernhardoj (please request in NewDot)
• Reviewer: $250 @rayane-djouah (Paid in Upwork)

[bernhardoj]

Requested in ND.

[rayane-d]

@lschurr - Offer Accepted

[lschurr]

All set!

[JmillsExpensify]

$250 approved for @bernhardoj