mWeb - Refreshing the LHN page opens chat with recent user

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Launch the URL https://staging.new.expensify.com
2. Log in with any account.
3. Refresh the page

Expected Result:

User should stay in LHN

Actual Result:

After page refresh opens the chat conversation with recent user

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Mobile Web

Version Number:
1.92-0
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Bug5220978_2.09.21.mp4

View all open jobs on GitHub

[MelvinBot]

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

[isagoico]

Issue reproducible during KI retests

[MelvinBot]

@joelbettner Whoops! This issue is 2 days overdue. Let's get this updated quick!

[joelbettner]

Sorry. I was OOO the past two days. This looks like it has what it needs to be worked on, and can be worked on by an external contributor.

[MelvinBot]

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

[mallenexpensify]

Job posted https://www.upwork.com/jobs/~017d13f768d0c64b2b

[MelvinBot]

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

[mallenexpensify]

@joelbettner it looks like you accidentally unassigned me, I've had that happen before, Sonia gave me the tip to unassign myself first, then add the label to avoid the double unassign.

[isagoico]

Issue reproducible during KI retests.

[meetmangukiya]

This is caused by the routing we have. We are by default because of our linkingConfig(I think) always routed to the last active report. So when the user first logs in, and when onyx is all synced up and all, the link is rewritten to /r/:reportId and when we refresh, it automatically lands into the reports view because that'd be the intended behavior in case of that link being used.

Problem here is we do not have any special route for indicating homepage which probably makes sense trying to normalize this on all platforms. However, this creates issues like this. I believe solving this would require a fair bit of refactoring in our react-navigation related code. How would we want to go about this?

[roryabraham]

Hmmm I think you're close to understanding everything here, but just to clarify the flow:

1. Because we're logged in and have an auth token, we're taken to AuthScreens.js.
2. From there, the homepage is the MainDrawerNavigator.
3. Here the MainDrawerNavigator determines the last-viewed report and opens to that report page (i.e: /r/:reportID). This is expected and required for wide screen widths.
4. Then, MainDrawerNavigator renders the BaseDrawerNavigator.
5. The BaseDrawerNavigator provides a default status for the drawer, which determines if it will be open or closed. On narrow screens, if the drawer is open, then we display the chat switcher as expected (i.e: the LHN drawer covers the report page). If the drawer is closed, then we display the report page.
6. Navigation.getDefaultDrawerState determines whether the drawer should be open or closed, based on the screen width.

So I'm guessing what's happening here is a race condition – when the page is refreshed to r/:reportID, withWindowDimensions has not yet provided the isSmallScreenWidth prop to the BaseDrawerNavigator, which in turn causes Navigation.getDefaultDrawerState to return 'closed', because it is given isSmallScreenWidth=undefined.

I think we might be able to fix this by just changing Navigation.getDefaultDrawerState like so:

/**
 * @param {Boolean} isSmallScreenWidth
 * @returns {String}
 */
function getDefaultDrawerState(isSmallScreenWidth) {
    return didTapNotificationBeforeReady ? 'closed' : 'open';
}

Because on wide screen widths, whether or not the drawer is open or closed shouldn't matter too much, and on small screen widths we want to default to the drawer being closed? Maybe there are some cases I'm missing here though?

[mallenexpensify]

Job doubled a day early cuz I'm doing the resync now

[meetmangukiya]

@roryabraham even the current implementation would be returning open. I tried it as well, and that doesn't fix it.

function getDefaultDrawerState(isSmallScreenWidth) {
    if (didTapNotificationBeforeReady) {
        return 'closed';
    }
    return isSmallScreenWidth ? 'open' : 'closed';
}

Even if it had, it would still not give a consistent view I suppose, if I copy the link while on a report and open that link a new tab, it will rather open on the drawer, not on the report. Maybe we should add a query parameter isDrawerOpen to the url that can be used to decide what to do.

[roryabraham]

@meetmangukiya Have you seen this documentation?

[kadiealexander]

Please refer to this post for updated information on the n6 hold, we've raised the bonus to $250 for all issues where a PR is created before the N6 hold is lifted.

[parasharrajat]

@roryabraham As I previously mentioned that this is an issue from the Lib but I was not sure about it. I checked the router state and it was telling me to open the drawer by default. But Drawer was not opening which led me to the conclusion that this is an issue with lib rather than our configuration.

I also mentioned that we can set the route to Home when the drawer is closed we can fix this issue which was the solution posted by Meetmanguria. so he was hired.

Unfortunately, I deleted the comments for some reason.

Good news

I opened an issue for #4211.

As a result, the lib author found the real bug and fixed it react-navigation/react-navigation#9936 (comment).

Now as soon as I push the fix for #4211. This will be resolved as well.

Please assign this issue to me as I am fixing this in a PR and if you think that's okay. I know Meetmanguria is assigned but no other solution would be needed anymore. Also, he has not come up with a solution yet.

[mvtglobally]

Issue reproducible during KI retests.

[roryabraham]

After discussing internally, we have decided that we are indeed going to reassign this issue to @parasharrajat, because after nearly two weeks, @meetmangukiya has not produced a PR to fix the issue via the original approach. They also have not provided an update in the last 10 days. Now that we have a better approach (due to the bug fixed in react-navigation), we'll move forward with that solution.

@mallenexpensify Please close @meetmangukiya's Upwork contract and hire @parasharrajat for this issue.

[mallenexpensify]

@parasharrajat hired in Upwork, @meetmangukiya hadn't been hired yet.

[mvtglobally]

Issue reproducible during KI retests.

[mallenexpensify]

n6-hold is lifted, label removed!

[parasharrajat]

Posting a solution for review.

1. We need a way to tell if a user is on the LHN or on the report page via url. We can't have two URLs for LHN and Report page as we use Drawer.

2. But using query params, we can determine the above difference.

Keeping the URL and Drawer in sync based on the queryParams is not directly possible. We have to create a custom Router to manage this.

Here is an implementation of the DrawerRouter which uses home=Boolean query param for the purpose.

// It is used to update the queryParam based on the Drawer State.
function setRouteParam(state) {
    const drawerStatus = state.history
        .find(it => it.type === 'drawer')?.status ?? state.default;
    const drawerOpenParam = drawerStatus === 'open' ? 'true' : 'false';
    const params = {
        ...state.routes[state.index].params,
        home: drawerOpenParam,
    };

    // eslint-disable-next-line no-param-reassign
    state.routes[state.index] = {
        ...state.routes[state.index],
        params,
    };
    return state;
}


export function DrawerRouter(props) {
    const router = CoreRouter(props);
    const defaultStatus = props.defaultStatus;
    const isDrawerInHistory = state => Boolean(state.history?.some(it => it.type === 'drawer'));

    const addDrawerToHistory = (state) => {
        if (isDrawerInHistory(state)) {
            return state;
        }

        return {
            ...state,
            history: [
                ...state.history,
                {
                    type: 'drawer',
                    status: defaultStatus === 'open' ? 'closed' : 'open',
                },
            ],
        };
    };

    const removeDrawerFromHistory = (state) => {
        if (!isDrawerInHistory(state)) {
            return state;
        }

        return {
            ...state,
            history: state.history.filter(it => it.type !== 'drawer'),
        };
    };

    const openDrawer = (
        state,
    ) => {
        if (defaultStatus === 'open') {
            return removeDrawerFromHistory(state);
        }

        return addDrawerToHistory(state);
    };

    const closeDrawer = (
        state,
    ) => {
        if (defaultStatus === 'open') {
            return addDrawerToHistory(state);
        }

        return removeDrawerFromHistory(state);
    };

    return {
        ...router,
        getInitialState({routeNames, routeParamList, routeGetIdList}) {
            const state = router.getInitialState({
                routeNames,
                routeParamList,
                routeGetIdList,
            });
            return {
                ...setRouteParam(state),
            };
        },
        getRehydratedState(
            partialState,
            {routeNames, routeParamList, routeGetIdList},
        ) {
            let state = router.getRehydratedState(partialState, {
                routeNames,
                routeParamList,
                routeGetIdList,
            });
            const activeRoute = state.routes[state.index];
            const drawerOpenParam = activeRoute.params.home;
            switch (drawerOpenParam) {
                case 'true': state = openDrawer(state); break;
                case 'false': state = closeDrawer(state); break;
                default: state = closeDrawer(state); break;
            }
            state = setRouteParam(state, false);
            return state;
        },
        getStateForAction(state, action, options) {
            let result = router.getStateForAction(state, action, options);
            // We use reset action to navigate to the report on Drawer which needs to be handled here.
            if (result && action.type === 'RESET') {
                result = closeDrawer(result);
            }
            return result != null ? setRouteParam(result) : result;
        },

    };
}

This Router uses ditto implementation for opening and closing the drawer from the main DrawerRouter from the lib.

@roryabraham

[roryabraham]

Dang, that is pretty complicated 😅 I have think it looks like it will work, but we'll need to work through some of the details in the PR. @mallenexpensify Feel free to hire @parasharrajat on upwork.

This Router uses ditto implementation for opening and closing the drawer

What is ditto implementation?

[parasharrajat]

Dang, that is pretty complicated

There is more to it. We will have to create the custom navigator as well to utilize this Router which will be an exact copy of Drawernavigator Component.

I tried all the possible techniques but none of them were able to sync the URLs correctly. e.g. Linkingconfig https://reactnavigation.org/docs/configuring-links/#advanced-cases etc.

What is ditto implementation?

I mean it uses the same function definitions for opening and closing.

[mvtglobally]

Issue reproducible during KI retests.

[marcaaron]

Feels like we are maybe hitting some boundaries that will require fairly complicated code. Should we maybe reevaluate whether this is worth doing?

[roryabraham]

Feels like we are maybe hitting some boundaries that will require fairly complicated code. Should we maybe reevaluate whether this is worth doing?

Fair point, this probably isn't worth making a custom router and navigator for.

[parasharrajat]

Then, #5745 at least fixes the issue where LHN was not opened by default.

[mallenexpensify]

@roryabraham , let me know if you'd still like me to hire @parasharrajat, seems like we might be in limbo

[roryabraham]

Given the mild severity of the problem and the complexity of the solution, I'm going to go ahead and close this out. Thanks for the investigation @parasharrajat

[parasharrajat]

Thanks @roryabraham . I spent a ton of time into this and following it up since some time. It is sad see that we are not taking any action but yeah it seems complex. I think current behaviour is fine. But my other pr #5745 does effect the current behaviour. It fixes the part where LHN was not shown in opened state. Do we want to keep that behaviour?

[roryabraham]

Yeah, I think the new behavior from #5745 is better than what we have now.