[HOLD for payment 2022-03-22] [$1,000] Back arrow on chat doesn't work after decreasing the browser size - Reported by @Tushu17

[mvtglobally]

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

---

Action Performed:

1. Open app
2. Navigate to any chat
3. Resize browser window to mobile view

Expected Result:

User should be able to use Back button

Actual Result:

User is unable to use back button. User has to resize browser back to navigate to LHN

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web

Version Number: 1.1.26-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

screen-capture.A.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Tushu17
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1640369999109800

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @yuwenmemon (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[kakajann]

@parasharrajat I think this issue is a side effect of this PR #6356

[parasharrajat]

Ok. Thanks for pointing out @kakajann. Let this issue be triaged and we can find a solution.

[MelvinBot]

Triggered auto assignment to @bfitzexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[bfitzexpensify]

Upwork posting here

Reporting job here @Tushu17

[MelvinBot]

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

[MelvinBot]

Triggered auto assignment to @thienlnam (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[brianmuks]

I have been looking into this issue. The bug is coming from defaultStatus in the Drawer.Navigator component. The method that is handling the state of the defaultStatus returns true if the screen is small otherwise false. However, according to the documentation on https://reactnavigation.org/docs/drawer-navigator it states that the defaultStatus should always be set to open .

From the docs:

You can also specify other props such as drawerStyle based on screen size to customize the behavior. For example, you can combine it with defaultStatus="open" to achieve a master-detail layout.

Please let me know if this proposal sounds good so I can take on the job.

[parasharrajat]

Would you like to link some code in our app? So that I know which component are you talking about?

[brianmuks]

Would you like to link some code in our app? So that I know which component are you talking about?

App/src/libs/Navigation/AppNavigator/BaseDrawerNavigator.js

Line 39 in 49cb0dd

<Drawer.Navigator

[parasharrajat]

Ok, Thanks.

The method that is handling the state of the defaultStatus returns true if the screen is small otherwise false.

I don't think this is correct.

[brianmuks]

Ok, Thanks.

The method that is handling the state of the defaultStatus returns true if the screen is small otherwise false.

I don't think this is correct.

I tested the code and it's working just fine.

[brianmuks]

Ok, Thanks.

The method that is handling the state of the defaultStatus returns true if the screen is small otherwise false.

I don't think this is correct.

Ok, Thanks.

The method that is handling the state of the defaultStatus returns true if the screen is small otherwise false.

I don't think this is correct.

https://www.awesomescreenshot.com/video/6823807?key=90ed83be4bafc44948931864ee2bffba

[parasharrajat]

Proposal

After debugging I found out that changing the defaultStatus on Base causes the navigation to break.

it is happening when we remount the navigator and change the defaultStatus. defaultStatus is supposed to be a first-time value (not controlled) and changes to this prop, do not update the default status of the drawer. It is only used on the mount.

we need remounting to fix the layout issues when the browser is resized.

So to fix this issue, we have to make sure that we don't update the defaultStatus when the browser is resized.
Thus something like this fixes it and does not break navigation.

const BaseDrawerNavigator = (props) => {
    const [defaultStatus] = useState(Navigation.getDefaultDrawerState(props.isSmallScreenWidth));

    const content = (
        <Drawer.Navigator
            backBehavior="none"
            key={`BaseDrawerNavigator${props.isSmallScreenWidth}`}
            defaultStatus={defaultStatus}

I will use the class component.

---

I can tell that @brianmuks was close to finding the solution and his proposal, discussed about defaultStatus. But his explanation is not correct and his suggestions either break the navigation or ask to update the behavior of the prop. Thus I rejected his proposal.

But I consider his contributions to be valuable.

I would like to fix this issue cc: @thienlnam (Some discussion about the issue )

🎀 👀 🎀 C+ reviewed and proposed.

[thienlnam]

Thanks for the proposal and the comment @parasharrajat. I've looked over your proposal and it looks good, and I agree that @brianmuks was close to finding the solution and had an investigation that proved useful to your proposal. With that being said, @parasharrajat You have the 🟢 from me to start on the PR and @brianmuks we're discussing a compensation for your efforts towards the solution

[brianmuks]

Thanks for the proposal and the comment @parasharrajat. I've looked over your proposal and it looks good, and I agree that @brianmuks was close to finding the solution and had an investigation that proved useful to your proposal. With that being said, @parasharrajat You have the 🟢 from me to start on the PR and @brianmuks we're discussing a compensation for your efforts towards the solution

@thienlnam thanks for your consideration.

[parasharrajat]

Update: I think the solution that I found is not sufficient. It does not work on the Desktop. I will debug more. But for now, I retract my review.

[thienlnam]

It looks like this issue is back on the table and ready to take more proposals. @bfitzexpensify Could we get this doubled?

[bfitzexpensify]

You bet - doubled to $1,000.

[parasharrajat]

@thienlnam

Updated Proposal:

I spent more time debugging this. I found out the real cause of the issue.
TLDR:

The issue is caused by the wrong drawer history

App/src/libs/Navigation/CustomActions.js

Lines 104 to 108 in 0e3bbe7

	history.push({
	type: 'drawer',
	status: 'closed',
	});

.

This history is used to close the drawer when we navigate to the report screen so that users don't see LHN instead of chats. But this logic is incorrect.
Correct logic would be:
https://github.com/parasharrajat/Expensify.cash/blob/d99a95ca2a2e4a8f913961f5c933b4a54b3c7b08/src/libs/Navigation/CustomActions.js#L103-L114.

        // Force drawer to close
        // https://github.com/react-navigation/react-navigation/blob/94ab791cae5061455f036cd3f6bc7fa63167e7c7/packages/routers/src/DrawerRouter.tsx#L142
        const hasDrawerhistory = _.find(state.history || [], h => h.type === 'drawer');
        if (!hasDrawerhistory || currentState.type !== 'drawer') {
            history.push({
                type: 'drawer',

                // If current state is not from drawer navigator then always use closed status to close the drawer
                status: currentState.type !== 'drawer' || currentState.default === 'open' ? 'closed' : 'open',
            });
        }

React-navigation usage following logic to close the drawer and we should use the same. https://github.com/react-navigation/react-navigation/blob/94ab791cae5061455f036cd3f6bc7fa63167e7c7/packages/routers/src/DrawerRouter.tsx#L142

According to this,

• if drawer default status is open, then only add the history to close it.
• otherwise, remove the history,

In the change above, I am doing the following:

1. If history already has drawer status, do not override it.
2. If no history is present or the state is not from drawer navigation (i.e. RHN modals) then use closed status history to close the drawer.

I have tested it in the following scene:

1. Open the app directly on the web. and navigate from both LHN and RHN in various combinations - ✔️
2. Resize the browser to mobile view and try out navigations - ✔️
3. Open the app directly on mobile view and run navigations - ✔️
4. resize to desktop view - run navigations - ✔️

It works well. I found this solution after going through many iterations.

[thienlnam]

@parasharrajat The proposal looks good to me, thanks for the thorough explanation!

[parasharrajat]

PR is ready.

[thienlnam]

@bfitzexpensify I've just merged the PR, leaving this note here for you once the PR is deployed and the regression period is complete

Tushu17 - Reporting payout - $250
brianmuks - Original assistance to help find solution - $250
parasharrajat - Issue completion - $1000

[botify]

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

• Fix: back arrow for drawer navigation when browser is resized #7801
• Fix the back arrow on intial Load #8067

If no regressions arise, payment will be issued on 2022-03-22. 🎊

[bfitzexpensify]

Looks like the posts expired because they were posted a while back. Have recreated them:

@brianmuks please apply here

@parasharrajat please apply here

[brianmuks]

Looks like the posts expired because they were posted a while back. Have recreated them:

@brianmuks please apply here

@parasharrajat please apply here

I have done so thanks.

[bfitzexpensify]

Thanks! OK, all paid out and complete. Thanks everyone!