[$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

https://user-images.githubusercontent.com/43995119/147601395-82f510e8-953d-43ab-a759-e5dd996c0e6b.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: https://github.com/Expensify/App/blob/615f5b3ff21cbdcf83ef2a050c875de2a0dbfb26/src/libs/Navigation/CustomActions.js#L108

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: https://github.com/Expensify/App/blob/615f5b3ff21cbdcf83ef2a050c875de2a0dbfb26/src/libs/Navigation/CustomActions.js#L102-L109

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

https://github.com/Expensify/App/blob/a87b53c273be288bdc887d618973404ba01ea22b/src/libs/Navigation/CustomActions.js#L98-L100

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.

[marcaaron]

Cool. Thanks for looking into it!

To give some background here, we have done the custom navigate method to try and get various things to work for our web implementation of react-navigation. At this point, it has led to some pretty tricky code. So, I feel more comfortable accepting proposals that clearly explain why a fix will do what we want and show a good understanding of how the current code works. "I changed this and it works now" or "this is done incorrectly, but I can't explain why" proposals will need to be improved.

[Christinadobrzyn]

Just checking on this - @parasharrajat and @marcaaron - can you confirm we're still reviewing proposals for this job?

[parasharrajat]

Yeah @Christinadobrzyn.

[Christinadobrzyn]

Increased Upwork job price to $500 - collecting proposals.

[Christinadobrzyn]

Increased job price to $1000 - collecting proposals.

[murataka]

Proposal

Hello ,

The issue happens when a user request money after switching between chat members .

The bug is happening because of two reasons:

1 : Onyx variable lifecycles configuration seems to be incomplete for some variables.

2: Misuse of componentDidMount functionality in ReportScreen.js ( should not make changes to non-local variables at that state listener(s) )

So the solution is to complete the part related to Onyx about active (selected) report lifecycle, and then changing the lifecycle of variables in such a way that we only make changes to global variables where the trigger is the event which we can easily track: In that case it is the user interaction handler function (s) .

Also, as I am new here , I needed to finish the debugging process to understand what is what . So I can also add a video of it working correctly in case you need it.

[parasharrajat]

Hey @murataka, I didn't understand your proposal. Could you please add more technical info about the things you are talking about? Use code references to indicate where you are suggesting the changes and what changes.

[murataka]

Hey @murataka, I didn't understand your proposal. Could you please add more technical info about the things you are talking about? Use code references to indicate where you are suggesting the changes and what changes.

Yes, The project uses Onyx library to handle updating of multiple renderable components. Updating multiple components upon a change of a variable is a difficult task on react apps, whereas you already are using Onyx to handle that known problem.

However, when you do not add the withOnyx({ ... to a component configuration , or not add the variable to that configuration to make it read props variables from the storage , it will assume that variable is undefined. This is a case for MaindrawerNavigator.js

The current implementation tracks not just the selected report(chat session) , but also tracks when we accessed them. So , the implementation fetches the last used chat session (In fact it becomes previous chat session due to the lifecycle of variables ) , so the developer(s) may got confused to using that lastaccessedreportid to store the current chat session, which leads to switching to previous instead of the last (current) chat session. This is an other case for MaindrawerNavigator.js

Also , componentdidmount and similar hook handlers should not be used for implementing pseudo algorithm to code:

we do not know when those functions will be called exactly, although we assume when they are called. For example , when a user clicks a chat button to switch between sessions , we should not implement changing a state variable in that hook functions , which will cause issues in some cases . It may trigger an infinite loop for example (changing state variables will cause react to render components which uses them ), and it will be very hard to find that kind of bug. This is the case for ReportScreen.js

As we have the Onyx library which I liked , we should be very careful when using variables from Onyx database. Those variables should be implemented carefully , and better developer should add a comment such as

const MainDrawerNavigator = (props) => {
// Onyx :props.reports 
    const initialParams = getInitialReportScreenParams(props.reports, !Permissions.canUseDefaultRooms(props.betas));

where possible. Tracking issues related to those variables will be harder after the project grows .

The solution does not require implementing new modules , nor making major changes.

You can still ask more specific questions if you like.

[murataka]

const getInitialReportScreenParams = (reports, ignoreDefaultRooms) => {
    const last = ReportUtils.findLastAccessedReport(reports, ignoreDefaultRooms);

This is the confusing part of the code which I mentioned above , causing the previous chat session to be fetched. There's a corrolation between multiple mistakes made , so this line does what it says , but still is a part of the bug although it is correct.

[parasharrajat]

Truly speaking, I didn't understand a single thing until https://github.com/Expensify/App/issues/6912#issuecomment-1018076712.

Maybe try to write small and clear bullet points to explain your thoughts next time. You are trying to mix suggestions with issue cause and solution. This makes it all confusing.

Back to the topic, Thanks, I see that https://github.com/Expensify/App/issues/6912#issuecomment-1018076712 is causing the wrong chat to load. I will try to test it and see if this is the main cause. And I may also know when it started happening. But How would you solve this issue while keeping the last accessed chat feature intact?

[murataka]

Maybe try to write small and clear bullet points to explain your thoughts next time. You are trying to mix suggestions with issue cause and solution. This makes it all confusing.

The fix does not require much changes, it is not more than 10-20 lines of change . So , I needed to explain the cause of the problems, as otherwise I shall need to share all the changes needed.

Thanks, I see that #6912 (comment) is causing the wrong chat to load. I will try to test it and see if this is the main cause. And I may also know when it started happening. But How would you solve this issue while keeping the last accessed chat feature intact?

It is not the single cause and tracking that will not be enough to find the root cause. As stated above , there's a corrolation between those mistakes and all together they create the bug .

I tried to explain the causes mostly , including the solution and how to prevent it happening again in future, whereas seeing the changes I made will make things more clear . But Instead I wanted to explain what mistakes caused that issue, and why it is difficult to debug it .

I even did not add a new variable or function , so you can be sure that the solution is a very clear implementation ,it is not messy.

[murataka]

I will also add comments to the code , and try to share as much details as possible , where I think that similar issues may appear and to prevent those mistakes, adding more comments to the code seems essential to prevent future bugs.

[parasharrajat]

I tried to explain the causes mostly , including the solution and how to prevent it happening again in future, whereas seeing the changes I made will make things more clear . But instead I wanted to explain what mistakes caused that issue, and why it is difficult to debug it .

That's great. If you have joined slack, you can start a discussion about this. And we will be glad to discuss it there in detail.

For now, let's reduce the scope of this issue to the problem it states and try to fix that first. You have explained the problem and the reason. But what I am asking is the implementation changes you would do to achieve this.

[murataka]

For now, let's reduce the scope of this issue to the problem it states and try to fix that first. You have explained the problem and the reason. But what I am asking is the implementation changes you would do to achieve this.

I already tested the solution and it works. I would share the changes made after the contract starts, as just a few lines need to be changed : nothing newly implemented.

I mean it was already implemented , but there were some mistakes. As I did not introduce a new scope , there's not much to discuss, let me find and join the slack channel . I will continue to chat there . I strongly feel that the shared information above would be enough for that bug, so adding more comments here will just confuse the readers and won't be good for benefiting from the explanations/ideas shared above . See you on slack 👍

[parasharrajat]

I already tested the solution and it works. I would share the changes made after the contract starts,

I see. If you are worried to share your solution before the contract, then rest assured that if your solution is best of all proposed fix the issue and does not cause regressions. You would be paid for it.

At Expensify, the process is a little different. we like to hear the solution first and analyze your approach to it. It helps us in finding the regressions and give you suggestions before you start working on the problem. It also helps us maintain visibility.

Let me know if you still have any doubts. You can read more about the process at CONTRIBUTING.md.

[murataka]

Yes , I understand that , but just a few hours passed since I have found that repo / work conditions. I made changes to 3-4 files, some of them just 1 line maybe ., and will make more checks / try to find any mistakes if I have made before sharing all the solution ,and I planned to do that after I have a contract. Discussing the solution after I submit the code for review will be very nice as I am very experienced . That is because I will have more chance to defend the solution I propose or prevent any misunderstanding.

It does not state in CONTRIBUTING.md that we must share all the solution before having the contract.

[murataka]

@parasharrajat

Contributing.md tells me to stop at that state ...

5. Pause at this step until someone from the Contributor-Plus team and / or someone from Expensify provides feedback on your proposal (do not create a pull request yet).

[parasharrajat]

Yeah, Correct. I am not asking you to create a PR rather the code changes you would do in brief.

[murataka]

@parashat , I counted 12 lines to be added ( including brackets ) and one deletion . I have no idea if you are the one to start the contract , but I want to add this as a summary: the solution is mostly finishing the previous incomplete implementation about the chat related Onyx library ( no required change in onyx db ). Still waiting for the slack invitation. Please let me know if the hiring process may take more than a week or if I need to take any other action .

ps: Take into account that , this is a bugfix , and no new feature is added, so I can not make the same discussion as I made at

https://github.com/Expensify/App/issues/7316

so a proposal like "why don't we approach that issue like ...." is not something you should expect . I would share any possible solutions if there were more than one approach possible , but for that one , solution is pretty straightforward.

[parasharrajat]

@iwiznia Could you please help me review the above proposal? Thanks.

[iwiznia]

Hi @murataka, thanks for investigating the issue! From the comments above, it seems you identified the problem and found a solution. Our approach on all our open source issues is to first discuss the solution with the contributors in order to make sure we all agree on the solution and that it is the best solution we can find. So we do not hire anyone before we can evaluate the solution. I know this is in contrast to other job offers published in Upwork, but it allows us to ensure the level of quality we want for our app and code while also diminishing the work thrown out by contributors. ie: if we waited till contributors submit a PR and then found out the solution is not acceptable or has some drawbacks, then that code would need to be thrown out, which is frustrating for contributors. So, would you mind to explain exactly what changes would you do to solve this problem?

[murataka]

So, would you mind to explain exactly what changes would you do to solve this problem?

I already tried to explain the root causes of the bug , so I think I shall share the code changes need to be made , in addition to my comments above , I hope that will be more clear for you.

ReportScreen.js

  componentDidMount() {
        this.prepareTransition();
        this.storeCurrentlyViewedReport();  // This line makes changes on Onyx variables , and should not be implemented here 
 // details explained above : search the "componentDidMount" keyword
    } 

SidebarLinks.js

onSelectRow={(option) => {
                        Report.updateCurrentlyViewedReportID(option.reportID);
// instead the deleted line should be added here , yes they are different  functions but does the same thing.

MainDrawerNavigator.js

 reports: {
        key: ONYXKEYS.COLLECTION.REPORT,
    },
     currentlyViewedReportID: {
      key: ONYXKEYS.CURRENTLY_VIEWED_REPORTID,
        },
    betas: {

// we need to integrate ONYXKEYS.CURRENTLY_VIEWED_REPORTID at his file, that was missing, 

// the developer who implemented it just got confused because there is some similar functionality which returns previous document instead of the last one .

if (!initialParams.reportID) {
        return <FullScreenLoadingIndicator />;
    }
    if (props.currentlyViewedReportID) { /// added that if clause not to break the existing functionality. and also the answer to 

the question which @parasharrajat asked above .

But How would you solve this issue while keeping the last accessed chat feature intact?

        initialParams.reportID=props.currentlyViewedReportID;
        initialParams.currentlyViewedReportID=props.currentlyViewedReportID;

        // one of the above 2 lines may not benecessary, wll examine that after I have a contract 🔢 
    }

This is pretty the whole solution, 

[murataka]

in case you want to test,

You shall also need the below lines at MainDrawerNavigator.js :

 reports: PropTypes.objectOf(PropTypes.shape({
        reportID: PropTypes.number,
    })),
    currentlyViewedReportID: PropTypes.string,
    /** Beta features list */
    betas: PropTypes.arrayOf(PropTypes.string),

const defaultProps = {
    reports: {},
    betas: [],
    currentlyViewedReportID: '',
};

[murataka]

let me know if you are trying to test it , there might be missing changes still . I don't know if this is the right way to evaluate the proposal in case you are trying to test it .

[murataka]

ReportScreen.js

const reportID = this.props.currentlyViewedReportID;

Please let me know if sharing the code will be mandatory for all bugfix issues for my next proposals, or if there's some way to not make that progress so long so I can take action about it ..

[murataka]

https://user-images.githubusercontent.com/5358438/150614123-7e6b89f9-4615-4cf4-9170-f9f24e73eaf4.mov

[brianmuks]

Proposal Navigation.dismissModal(); will fix the bug. We can call the method on this line: https://github.com/Expensify/App/blob/0bce09177d2aed8c5e377bc758f5241dd22886eb/src/pages/iou/IOUModal.js#L308 or when the transaction has succeeded.

[murataka]

Navigation.dismissModal(); will fix the bug.

that time the green (+) button to request money will cause chat to switch to the report chat for that last request made. I have no idea about the intended behavior though.

[parasharrajat]

I am having a hard time putting all the pieces together. @murataka. So I would request you to write down the complete proposal.

• Explain to me why and how you think that this.storeCurrentlyViewedReport() is causing the issue?
• Your proposed changes against the issue.
• Explain your changes, not just the code.

Most of the time testing the proposal is not required.

Note: you do not need to tell the whole solution you would do. Only the steps you would take.

[murataka]

I am having a hard time putting all the pieces together. @murataka. So I would request you to write down the complete proposal.

I apologize for being complicated, that is because I am so new here, still trying to get used to. Let me try to be more clear ( As much as I can )

[brianmuks]

@parasharrajat what do you think of my solution ?

[parasharrajat]

@brianmuks Please read this post and let me know what do you think?

[murataka]

• Explain to me why and how you think that this.storeCurrentlyViewedReport() is causing the issue?

This in fact might need further discussion maybe in the slack channel. Because updating the Onyx variables at the render flow is not a good idea ,and it is possibly done at multiple files. Example case :

class ChildComponent extends Component {
...
  componentDidMount() {
Onyx.merge(ONYXKEYS.someOnyxStateVariable, String(newValueOfsomeOnyxStateVariable));
}

<ParentComponent sompeproperty={someOnyxStateVariable}>

<ChildComponent > </ChildComponent  > 
</ParentComponent >

This will recursively cause the React renderer to render ParentComponent . I did not test that , but may do a quick test if I'm right if you ask so . Also look at this issue : https://github.com/Expensify/App/issues/5930

[brianmuks]

@brianmuks Please read this post and let me know what do you think?

Thanks very helpful 🙏🏾 .

So I discovered that whenever the modal is opened going back to the chats before closing the modal affects the state of the chat screen . On the other hand closing the modal then navigating back to the chat screen maintains the state of the chat screen. I am not yet sure of the underlying cause.

[murataka]

• Your proposed changes against the issue.

• Explain your changes, not just the code.

we are currently using this variable instead of currentlyViewedReportID const last = ReportUtils.findLastAccessedReport(reports, ignoreDefaultRooms); But this variable is not the same , so when we use the green (+) button to request transaction , the active chat window switches to last made transaction. Even if that is the intended behavior , we should still use currentlyViewedReportID in implementation to have more understandable code.

So , my proposal is to update SidebarLinks.js as

  onSelectRow={(option) => {
          +              Report.updateCurrentlyViewedReportID(option.reportID);

and use currentlyViewedReportID instead of

MaindrawerNavigator.js

const last = ReportUtils.findLastAccessedReport(reports, ignoreDefaultRooms); where essential . it is currently used in ReportScreen.js and SidebarLinks.js .

There might be some more little similar changes need to be made , I need to check further. But this is the general idea.

[parasharrajat]

@brianmuks

So I discovered that whenever the modal is opened going back to the chats before closing the modal affects the state of the chat screen. On the other hand, closing the modal then navigating back to the chat screen maintains the state of the chat screen. I am not yet sure of the underlying cause.

First, we need this behavior to fix another bug where drafts do not update on report change. So we reset the navigation state. Second, in the navigation action, first, the side modal is closed and then the drawer is loaded with the correct report. You can try to debug the navigation action and observe the same.

That post points out that I am not yet sure of the underlying cause. You should be able to explain. That is a complex piece of logic so we won't do any change until fully reasoned.

---

Thanks, @murataka, this could be the issue but do you have any explanation why it works sometimes.

[murataka]

Thanks, @murataka, this could be the issue but do you have any explanation why it works sometimes.

The bug only will happen if you previously switched between conversations before requesting money.

It switches to that variable :

const last = ReportUtils.findLastAccessedReport(reports, ignoreDefaultRooms);

So it is not random. After request is made , it points to the last made request report id

[parasharrajat]

Ah, I see. But I think currentlyViewedReportID is not synced across devices so it won't help us preserve the lastViewreport functionality.

1. User should land to the last viewed report on any device.
2. Then he should be able to switch to any report he wants which will become last viewed.
3. last Viewed report should only act when a user opens the app for the first time.
4. We pass const last = ReportUtils.findLastAccessedReport(reports, ignoreDefaultRooms); to the initialParams and remount the drawer when screen is resized. I am not sure but I think it also remounts when we navigate to chat.

So we need a way to achieve point 3.

[murataka]

Ah, I see. But I think currentlyViewedReportID is not synced across devices so it won't help us preserve the lastViewreport functionality.

I don't think this will be an issue because I'm not replacing all code where const last = ReportUtils.findLastAccessedReport(reports, ignoreDefaultRooms); is used.

It is still being used where it should be used, so functionality using last Viewed report should only act when a user opens the app for the first time. won't be affected . this is why I told that may need to check further , and that comment is helpful about that.