[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 contributors@expensify.com 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): Yokabdk+new37@gmail.com 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
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/a764eed7-53ea-4c8b-bdc0-33eefc151d31

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[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. https://github.com/Expensify/App/blob/4bce83817e052533e910e977c0055abb3ef7e180/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts#L188-L196

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. https://github.com/Expensify/App/blob/4bce83817e052533e910e977c0055abb3ef7e180/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts#L106-L131

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 https://github.com/Expensify/App/blob/4bce83817e052533e910e977c0055abb3ef7e180/src/components/MoneyRequestHeader.tsx#L111-L117

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. https://github.com/Expensify/App/blob/4bce83817e052533e910e977c0055abb3ef7e180/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts#L118-L124

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. https://github.com/Expensify/App/blob/4bce83817e052533e910e977c0055abb3ef7e180/src/libs/Navigation/linkingConfig/getAdaptedStateFromPath.ts#L109-L110

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[rayane-djouah]

Reviewing today 👀

[rayane-djouah]

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

https://github.com/user-attachments/assets/089a56a2-16f1-4b5d-b08f-72ce47aa9bf5

[luacmartins]

Yea, we should fix this for all these pages.

[rayane-djouah]

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

https://github.com/user-attachments/assets/5368fca3-1afe-4e66-a13c-7f517df85000

[rayane-djouah]

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

https://github.com/user-attachments/assets/e7b8edd2-ccc7-49ef-b230-277f352f71cc

[rayane-djouah]

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

[rayane-djouah]

Waiting on proposals

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

https://github.com/Expensify/App/blob/0980fcd287df09b5e45410a2c8ae00be931e963b/src/libs/Navigation/NavigationRoot.tsx#L93-L124

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:

https://github.com/Expensify/App/blob/0980fcd287df09b5e45410a2c8ae00be931e963b/src/libs/Navigation/NavigationRoot.tsx#L152

This will fix many refresh related issues.

Result:

https://github.com/user-attachments/assets/fa371312-f0fe-4399-bd79-7b055a10af5e

The test branch.

What alternative solutions did you explore? (Optional)

[lschurr]

@rayane-djouah could you review the latest proposal?

[rayane-djouah]

Will review today

[rayane-djouah]

@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[bot]]

@luacmartins, @lschurr, @rayane-djouah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[rayane-djouah]

I think this needs a wider discussion on Slack. I will try to start a thread on Monday

[melvin-bot[bot]]

@luacmartins @lschurr @rayane-djouah this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@luacmartins, @lschurr, @rayane-djouah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[rayane-djouah]

Started a discussion: https://expensify.slack.com/archives/C01GTK53T8Q/p1724095672150509

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[rayane-djouah]

Issue not reproducible during KI retests. (First week)

I'm still able to reproduce

[rayane-djouah]

Based on the Slack discussion, we prefer to use the backTo parameter instead of adding additional state persistence.

We can move forward with @bernhardoj's proposal. Please note that we should apply the backTo solution to the report details and other pages that can be accessed from the central pane navigator or the search navigator.

:ribbon::eyes::ribbon: C+ reviewed

[melvin-bot[bot]]

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

[adamgrzybowski]

I agree with the @bernhardoj proposal. FYI fix for the second issue you mentioned in your proposal (the one with the adapted state) was pushed 3 days ago here https://github.com/Expensify/App/pull/44635

[melvin-bot[bot]]

📣 @rayane-djouah 🎉 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

[bernhardoj]

PR is ready

cc: @rayane-djouah

[melvin-bot[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[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[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-djouah]

PR on production

[rayane-djouah]

[!NOTE] The production deploy automation failed: This should be on [HOLD for Payment 2024-10-07] according to https://github.com/Expensify/App/issues/49855 production deploy checklist, confirmed in https://github.com/Expensify/App/pull/47990#issuecomment-2384301462

cc @lschurr

[rayane-djouah]

BugZero Checklist

• [x] 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-djouah]

@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 (https://github.com/Expensify/App/pull/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 (https://github.com/Expensify/App/issues/46773#issuecomment-2273502379, https://github.com/Expensify/App/issues/46773#issuecomment-2273507549). Moreover, the original PR (https://github.com/Expensify/App/pull/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-djouah]

@lschurr - Offer Accepted

[lschurr]

All set!

[JmillsExpensify]

$250 approved for @bernhardoj