[$1000] After completing "Request money" journey the chat randomly redirects to other chat Reported by @mananjadhav

[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. Have multiple chat discussions and groups
3. Pick the top discussion on LHN
4. Select Request money from "+" menu
5. Select same user as your current open chat and send request

Expected Result:

User should stay on the same chat

Actual Result:

User is redirected randomly to another chat

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.1.23-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.Recording.2021-12-27.at.9.30.48.PM.mov

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

View all open jobs on GitHub

[MelvinBot]

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

[MelvinBot]

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

[Gonals]

Setting tags and sending to the pool!

[MelvinBot]

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

[MelvinBot]

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

[mallenexpensify]

Job posted https://www.upwork.com/jobs/~01b794160880fbf187
Will keep 👀 on for @Christinadobrzyn til she's back (her OOO isn't showing up correctly so she got assigned to this)

[dklymenk]

Hi, I managed to narrow the issue down to this line:

App/src/libs/Navigation/CustomActions.js

Line 108 in 615f5b3

return DrawerActions.closeDrawer();

I couldn't figure out the actual root cause, but for some reason dispatching a CLOSE_DRAWER action sometimes gets followed up with a RESET action with a reportID for a wrong chat. I only managed to get that information with useReduxDevToolsExtension

I wasn't able to put that information to good use and I haven't found what exactly causes that follow-up action to get dispatched, but deleting this code that sends a CLOSE_DRAWER action if the user is already on the target scene fixes the issue:

App/src/libs/Navigation/CustomActions.js

Lines 102 to 109 in 615f5b3

	// If we're trying to navigate to the same screen that is already active there's nothing more to do except close the drawer.
	// This prevents unnecessary re-rendering the screen and adding duplicate items to the browser history.
	const activeState = getActiveState();
	const activeScreenName = getScreenNameFromState(activeState);
	const activeScreenParams = getParamsFromState(activeState);
	if (newScreenName === activeScreenName && _.isEqual(activeScreenParams, newScreenParams)) {
	return DrawerActions.closeDrawer();
	}

I understand that it's far from optimal solution, so hopefully someone manages to find a better and I hope my findings will help with that.

Thanks.

[parasharrajat]

Thanks, @dklymenk for putting an outline for the issue. I was known that this action is causing multiple issues. Currently two. I will investigate myself as I refactored it a few days back and then we cleaned up the code to make it more robust. Somehow, that cleanup has caused bugs.

cc: @marcaaron

[K4tsuki]

proposal

I think the problem is navigateBackToRootDrawer inside CustomActions.pushDrawerRoute

App/src/libs/Navigation/CustomActions.js

Lines 98 to 100 in a87b53c

	if (currentState.type !== 'drawer') {
	navigateBackToRootDrawer();
	}

I think we shouldn't do other navigation beside returning navigation actions in CustomAction pushDrawerRoute.

We could do navigateBackToRootDrawer right before pushDrawerRoute in navigate.
Pulling navigateBackToRootDrawer into navigate:
with additional check, something like:

if (isDrawerRoute(route)) {
    const windowPath = window && window.location ? window.location.pathname.substr(1) : null;
    if (isDrawerRoute(windowPath)) {
        CustomActions.navigateBackToRootDrawer();
    }
    navigationRef.current.dispatch(CustomActions.pushDrawerRoute(route));
}

or

if (isDrawerRoute(route)) {
    const rootState = navigationRef.getRootState();
    if (rootState.routes.length > 1) {
        CustomActions.navigateBackToRootDrawer();
    }
    navigationRef.current.dispatch(CustomActions.pushDrawerRoute(route));
}

or waiting for some time (around 500ms) after dismissModal and before next navigation action or something similar along the line.

This solve the issue for me.

[marcaaron]

I think we shouldn't do other navigation beside returning navigation actions in CustomAction pushDrawerRoute.

Any reason for this conclusion? Definitely open to hearing proposals for how to fix this and am happy to hear we have found a solution, but we should be able to explain what is happening as well. Every time this code is modified something else breaks.

[K4tsuki]

https://reactnavigation.org/docs/navigation-prop/#advanced-api-reference

Stated:

The dispatch function is much less commonly used
We recommend to avoid using the dispatch method often unless absolutely necessary.

Dispatch function:

The dispatch method lets us send a navigation action object which determines how the navigation state will be updated

There is no clear not allowed or forbidden message on documentation on calling other navigation actions inside dispatch. But I think react-navigation treated it as single navigation action (or maybe only focusing on how navigation state will be updated).
In CustomActions examples, there is no other navigation actions calling.

I am not sure, I am raising stackoverflow question about this.

[murataka]

Thanks, I was looking into the same. I tried to debug the cause yesterday. I may have found some other issues but I am still evaluating your solution. So far, I think it will cause issues but let's see.

removing Navigator.navigate methods from requestmoney IOU ( https://github.com/murataka/App/commit/f5e5a72bd6a499339e96ed0ecda488fbccde0a6e )

would solve the issue when we want to focus the bugs only and only about this topic . I can move the rest to some other navigation related issues, because I solved some bugs unintentionally while trying this one. That might be confusing while evaluating this proposal ( like: "hey why did you need to change that ") ...

And yes, there seems to be many issues to fix about navigation topic , that might not be all , just some of them. I'm looking forward if you did find some problems on my commit(s).

[murataka]

Proposal

removing Navigator.navigate methods from requestmoney IOU ( murataka@f5e5a72 )

fixes the related bug , and it would be easier to evaluate this proposal.

So I created this proposal again which is just focusing on this bug to make things simpler.

What it does is just remove the

navigation.navigate

methods which are causing the actual bug.

The rest is linter removing unused code.

As the title says

After completing "Request money" journey the chat randomly redirects to other chat

This is the solution.

[parasharrajat]

No, Update. Still holding...

[parasharrajat]

@Expensify/applause Is this issue still reproducible? If yes, can I get fresh reproduction steps?

[JalalMitali]

I just tried and I cant see this problem I have 3 chats :)

[parasharrajat]

I have researched this one over the weekend and I have some theory but I am collecting some data to validate it. Overall, I was not able to reproduce this issue since the start but I have a feeling that it exists. I have asked for new steps and QA to retest it.

[isagoico]

We were able to reproduce this on our side on Mac 10.15.7 / Chrome Version 98.0.4758.80

1. Request money from user 1
2. Repeat request with different user 2

After second request user redirect to user 1 conversation.

jump.RM.mp4

[parasharrajat]

Waiting for some data from the team before running some tests...

[michaelhaxhiu]

So this issue is still un-assigned right now as far as I can see, right? Mind providing a link to what we are waiting on right now @parasharrajat?

I'm trying to discern if this hold should be on hold for some reason, or doubling in value this week.

[parasharrajat]

I don't feel this needs doubling this week. I will update you next week on the status. I haven't completed my testing and it's not on hold now. I am debugging this. I have some data but am not yet ready to be shared.

[michaelhaxhiu]

Thanks for sharing the context, that's helpful!

[parasharrajat]

Ok, there is really something going on but unfortunately, I am not able to reproduce it. So back to proposals.

I don't like @murataka 's proposal. You are suggesting to move away from navigation.navigate to navigate to report in this case. And rely on currentlyViewedReportID to manage it. I don't think this is a good approach.

[murataka]

@parasharrajat , thanks for examining the proposals , In fact, This issue was my first and I had trouble to getting used to here at first, so , me trying to handle more than one case above is misleading.

The only change I suggest is this one :

https://github.com/murataka/App/commit/f5e5a72

currentlyViewedReportID is not related to that one .

What https://github.com/murataka/App/commit/f5e5a72 does is , we are removing the unessential navigate methods. After that , it works because , we are not redirecting to wrong report, even we are not (don't need to ) redirecting all in all .

[parasharrajat]

Thanks for clarifying the changes. There are two ways Request Money works:

1. Clicking ➕ FAB button.
2. From the active report, ➕ on the Composer.

I think your proposal may work while using 2 but it won't when using 1. Am I missing something?

[murataka]

Let me check it and confirm. I shall pull main again and test, also will add the video recording ...

[murataka]

To clarify what changed :

We just delete those two lines (87 and 137). After that , we shall have unused variables and libraries.

https://github.com/murataka/App/commit/f5e5a72#r67467635

https://github.com/murataka/App/commit/f5e5a72#r67467659

What we get in last total pr is the above lines deleted + unused variables and libraries removed.

sendmoneywithoutredirect.mov

[parasharrajat]

This is open for proposals...

[mallenexpensify]

I'm unable to reproduce and Rajat mentioned about he's not able to either on Desktop nor staging/web/Chrome.
@mananjadhav and/or @mvtglobally , can you see if you can reproduce? If not, we can close this

[mallenexpensify]

Gonna close this. If anyone can reproduce, please add a comment with steps and a video screenshot, then we'll reopen