Fix: navigation to report screen from the SideModals #6207

parasharrajat · 2021-11-04T11:39:22Z

Details

#4756 (comment)
It fixes various issues

Preserving the proper history
Clearing the old Draft message from the composer

Fixed Issues

$ #4756

Tests | QA Steps

Open the App on any device.
Go to any report.
type a draft message.
Switch the report from SearchPage, you should not see the draft comment from the previous report.

Open the App on any device.
Go to any report.
Try switching reports from LHN, search page, etc.
You should be able to switch reports.

Tested On

Screenshots

Web | Desktop

output_file.mp4

Mobile Web

output_file.mp4

iOS

Android

output_file.mp4

It fixes various issues - Preserving the proper history - Clearing the old Draft message from the composer

parasharrajat · 2021-11-04T11:55:19Z

@mountiny Could you please test it on IOS? I think it works but just in case.

mountiny · 2021-11-04T12:00:33Z

@parasharrajat I think my environment is also messed up the last time I tried 😄 but I will try.

However, in your screen recordings you have not shown that the original problem has been resolved. There was no draft in the reports before switching to see if the draft was not carried over. I cannot test right now, but I will get to it hopefully by tomorrow. Would you be able to provide video of the QA steps you describe in the PR body?

parasharrajat · 2021-11-04T12:24:27Z

Oops, I forgot to show that. I will add another video for android and web to show that.

parasharrajat · 2021-11-04T12:29:21Z

@mountiny Added more videos to Web and android

marcaaron · 2021-11-04T18:19:43Z

Adding myself as a reviewer so that we can find a longer term solution here. It feels like we are making changes to this file on a weekly basis 😄

parasharrajat · 2021-11-04T18:33:47Z

Haha yeah. This is just changed a while back but all changes are progressive. The next change will happen when the working back handler PR will be merged. But again that will be progressive. Lol. Thanks for reviewing it, I am always afraid of breaking something.

mountiny · 2021-11-05T16:50:20Z

@parasharrajat Fixed my environment and tested on iOS, seems like everything works as it should 🎉

Simulator.Screen.Recording.-.iPhone.12.-.2021-11-05.at.16.43.07.mp4

parasharrajat · 2021-11-05T16:54:39Z

Thanks, @mountiny. I also fixed my network interface on Mac today. There are a couple of other issues that need it...

mountiny

Code-wise, I can't think of any improvements, but I am sure eagle 👀 @marcaaron will find something 😅 also havent worked much with Navigation so I will leave this one for you @marcaaron to review and merge.

mountiny · 2021-11-16T01:29:58Z

Marc is OOO this week, so I would expect his review coming next week. I will wait for it unless we get more reports of users being annoyed by this bug which would prove urgency of this fix.

mountiny · 2021-11-22T17:40:33Z

@parasharrajat Bumping merge conflicts here. Thank you for your patience.

parasharrajat · 2021-11-22T17:45:37Z

Oh, this conflict is again from my changes on other PR. Lol... resolving...

marcaaron

@parasharrajat can you explain why these changes fix the linked issue? I don't really get what is happening and feel like we are just digging ourselves into a hole with all the CustomActions stuff...

parasharrajat · 2021-11-22T20:16:04Z

This change will pass the navigation to drawer job to last code which reset the state and remount the report screen just like the Navigation from LHN.

Now mounting clears the draft from previous report.

marcaaron · 2021-11-22T20:18:18Z

Hmm sorry I don't really understand. Can you try to maybe rephrase that? What do you mean by "drawer job to last code"?

parasharrajat · 2021-11-22T20:23:56Z

Ah, sorry. So whenever we navigate to a screen on the drawer navigator, new changes will remount the ReportScreen.
Previously, only navigating from LHN used to do that via resetting the state. Now, navigating from the Search page will also work the same as navigating via LHN which was previously using CommonActions.navigate.

marcaaron · 2021-11-22T20:29:24Z

So whenever we navigate to a screen on drawer navigator, new changes will remount that ReportScreen.

Which changes are we talking about exactly?

Previously, only navigating from LHN used to do that via resetting the state

I'm stuck on this part. What "state" are we talking about here specifically and what is the "that" which we are only doing when navigating from the LHN?

parasharrajat · 2021-11-22T20:34:17Z

Oops.

Which changes are we talking about exactly?

I am talking about the new changes in this PR.

I'm stuck on this part. What "state" are we talking about here specifically and what is the "that" which we are only doing when navigating from the LHN?

Navigation state. That is used for remount the ReportScreen

marcaaron · 2021-11-22T21:02:00Z

Hmm something still doesn't fit here. We are altering the navigation code so that the report screen remounts. But why should this matter? Either we should be able to update the report and have the report screen re-render (i.e. run whatever initialization logic it needs) or have it re-mount by default.

Maybe it would be useful to break down step by step exactly what happens?

parasharrajat · 2021-11-23T13:43:51Z

Sure. I have added a detailed explanation for why I chose this path instead of fixing the race conditions (mentioned on the link) in componentDidUpdate #4756 (comment). I will try to explain it further if that is not sufficient.

We will not need these changes if we can deliver the new comment for a report as soon as the report changes. I don't see a straight way to re-render the component when the draft prop is changed due to report change. Draft prop will change often, precisely on each keystroke. But how can we know that prop contains the draft for the new report? Please let me know if you see a way to do this, I would be happy to refactor the PR.

marcaaron · 2021-11-23T16:58:16Z

Hmm yeah I think I just disagree with your assessment of this issue. It doesn't quite feel like a navigation problem to me and as I've expressed previously - we are digging ourselves into a deep hole with a lot of custom navigation logic that is not easy to understand. But I could be wrong about that...

From your original proposal:

We do have logic in place to manage this but that does not work as the report and comment prop does not update in a sync manner

Why do the report and comment prop not update in a sync manner? It seems like we should be trying to solve that problem.

To fix this, I think we can remount the ReportScreen just like LHN, this issue will be fixed and we don't have to worry about the race conditions. (race conditions are harder to solve)

Again, I could be off here - but it sounds like we are avoiding the real solution because it is "hard". But if there's a race condition we should figure out what it is and whether there's something we can do about it?

marcaaron · 2021-11-23T17:00:27Z

We will not need these changes if we can deliver the new comment for a report as soon as the report changes. I don't see a straight way to re-render the component when the draft prop is changed due to report change. Draft prop will change often, precisely on each keystroke. But how can we know that prop contains the draft for the new report? Please let me know if you see a way to do this, I would be happy to refactor the PR.

Sorry, I didn't respond to this because I'm not really sure what you are asking.

parasharrajat · 2021-11-23T17:13:18Z

Why do the report and comment prop not update in a sync manner? It seems like we should be trying to solve that problem.

This is why this issue is occurring. in ComponentDidUpdate, we take the draft prop value and push it to the composer when the report id changes

App/src/pages/home/report/ReportActionCompose.js

Lines 201 to 206 in 0dfdf36

    
               if (this.props.report.reportID === prevProps.report.reportID) { 
        
                   return; 
        
               } 
        
               this.updateComment(this.props.comment); 
        
           }

.
But we do not get the draft of the new report when this happens. this.prop.comment still contains the old draft. We should find a solution to it but I didn't find any. It could be an Onyx issue. I just thought of LHN behaviour and tried to use the same every time we navigate to a report from any navigator.

Sorry, I didn't respond to this because I'm not really sure what you are asking.

Please ignore it.

marcaaron · 2021-11-23T17:48:42Z

this.prop.comment still contains the old draft. We should find a solution to it but I didn't find any. It could be an Onyx issue. I just thought of LHN behaviour and tried to use the same every time we navigate to a report from any navigator.

Ok it's starting to come into focus for me, thanks for your patience.

Let me see if I've got it right... we are correct to perform a side effect and the reportID prop does change. However, the comment itself has not yet updated (because it's also dependent on the reportID prop changing). Therefore, the code incorrectly assumes that if the reportID has changed then the comment must also have changed.

Just have a couple more questions now...

Should the expected behavior of Onyx be that if a prop changes then any prop dependent keys (example) should have their updates batched so we can make "safer" assumptions when performing side-effects? Asked another way, if A and B depends on C then a change to C should create a single update (setState call) for A and B?
If we don't agree with that then this code just does not work correctly. And we confirmed it does not work correctly in our investigation. So should we fix the bad code or remove it? It seems like in this proposed solution we are just leaving it there for someone else to trip over.

parasharrajat · 2021-11-23T18:07:50Z

Should the expected behavior of Onyx be that if a prop changes then any prop-dependent keys (example) should have their updates batched so we can make "safer" assumptions when performing side-effects? Asked another way, if A and B depend on C then a change to C should create a single update (setState call) for A and B?

I think it should if possible. It could resolve a few bugs in the future and help us remove code complexity for such scenarios.

Basically, we have two subscriptions A and B. and both A and B depend on prop C.
So when prop C updates, Both A and B should update together in a single batch.

If we don't agree with that then this code just does not work correctly. And we confirmed it does not work correctly in our investigation. So should we fix the bad code or remove it? It seems like in this proposed solution we are just leaving it there for someone else to trip over.

Yeah, I think the same. I should clean the componentDidupdate code and it won't be used anymore. But that depends where this PR is leading too.

marcaaron · 2021-11-23T18:12:33Z

Should we do A or B?

A:

Proceed with the current solution
Clean up the code that should not be used anymore
Only remounting the report screen should set the draft

B:

Fix Onyx so that the current solution is not necessary
Allow the report screen to be updated without requiring it to be remounted

parasharrajat · 2021-11-23T18:22:07Z

I think B is better if we can do that.

But I also think there is added advantage of using A. It would help fix the History of navigation. There is an open issue that I think is related and caused by messed-up history. #4612.
On that issue it is mentioned that it is reproducible from the SearchPage but not from the LHN.

We added the custom navigation action for the drawer so that we can reset the state and manage the history stack when the report id changes and I think we need the same for the Chat switcher (Search page) as well.

So if it does fix the history then we should do A to make it consistent for the whole app to navigate the report screen in the same way.

marcaaron · 2021-11-23T18:43:08Z

But I also think there is added advantage of using A. It would help fix the History of navigation. There is an open issue that I think is related and caused by messed-up history. #4612.

Alright, I left a comment in that issue, but basically I really want to see if we can move away from the custom logic we have and try to influence some changes to react-navigation and fix things there. The custom action still feels really hacky to me. I know, it was my idea in the first place - but I had never planned for it to be a long term solution and it seems wrong to keep investing in it.

marcaaron · 2021-11-23T18:57:22Z

I think a good general plan of action here would be to:

Identify why we need the custom action and the various contexts where it's used.
Document issues caused when it is not is use (i.e. why we need it)
See if the latest update to react-navigation solves any of those issues
Open issues with react-navigation for ones that are not solved
If we still need the custom action to solve these issues then add code comments to explain which issues the custom action solves

FWIW, that is the sort of work that came to mind when reading this comment:

Obviously, I will refactor the code in a good way. I have already improved the navigation earlier.

parasharrajat · 2021-11-23T19:06:46Z

FWIW, that is the sort of work that came to mind when reading this comment:

Now, I get it. 😆

parasharrajat · 2021-11-26T15:50:03Z

Issues without use of

App/src/libs/Navigation/Navigation.js

Lines 121 to 125 in 2aaa8fd

const {reportID, isParticipantsRoute} = ROUTES.parseReportRouteParams(route);

if (reportID && !isParticipantsRoute) {

navigationRef.current.dispatch(CustomActions.pushDrawerRoute(SCREENS.REPORT, {reportID}, navigationRef));

return;

}

History is not preserved properly while navigating.


Navigating in LHN
When we switch reports from LHN, no history is preserved at all, It simply updates the reported and rerenders the ReportScreen
Navigating between LHN and SideModals (SearchPage)
When we navigate to a report from the SideModal, Report is updated to selected but Id in the URL is immediately reverted to the old report. So if we press the browser navigation button, the first attempt does nothing just update the reportId and the second will take you back to the Old report.

  	**User navigation**  > report 1 > Modal > report 2 > Modal > report 3.
  	**Actual** > report3 > blank > report 2 >  Report 1

We use them for.

Close the SideModal immediately and switch to drawer navigator clearing the history of Stack navigator.
Navigate to the already opened report in the Main drawer, we just close the All the navigators in the history till we reach the Main Drawer.
Create a history entry for each navigation to report.

Does the latest `react-navigation` solves any of the issues

No, it does not. I think navigation History is created by the routers in react-navigation . As Drawer router is based on TabRouter. It does not create history for screen changes on I(Drawer navigator or tab navigator).

But react-navigation provides options to customize the Router.

Open issues with react-navigation for ones that are not solved

There are a few already
react-navigation/react-navigation#9744
react-navigation/react-navigation#10134
react-navigation/react-navigation#10143
I think we can upvote this feature request based on the Maintainer comment We have a small team and I don't think it's a common enough use case to support and maintain by default
Feature Request

My suggestion

I think it could take a very long time till the maintainer decides to fix the browser history and consider it important. So

Upvote the feature request.
Track that active issue and actively participate if this can fix our problem.
Merge this PR as this currently fix two problems. [HOLD for payment 2021-12-15] $500 Chat Switcher - Draft message from previous conv is displayed after using chat switcher #4756 New Group - Drafted message is not saved if the group doesn't have any messages yet #6490
Remove unused code.
Explain with comments.
Remove the custom logic when ready.

parasharrajat · 2021-11-26T19:09:34Z

cc: @marcaaron ⬆️

parasharrajat · 2021-11-30T19:55:41Z

cc: @marcaaron ⬆️

parasharrajat · 2021-12-03T16:05:53Z

Waiting for further instructions...

marcaaron · 2021-12-03T17:40:41Z

Thanks @parasharrajat! Great overview and appreciate you taking the time to put this together and test out the latest version of react-navigation to see if that fixes any of the issues. Seems like there has not been much progress on web so it might be up to us to do something here.

I think we can upvote this feature request based on the Maintainer comment We have a small team and I don't think it's a common enough use case to support and maintain by default

I took a look at Satyajit's response in that first issue and it sounds very reasonable, but also sounds like fixing the browser history isn't a priority yet and the "web support" is still being improved.

Track that active issue and actively participate if this can fix our problem.

Yeah I like this. Upvoting that issue is good. But might also be interesting to try and propose a problem / solution to react-navigation and see if we can get our "custom" action incorporated somehow or perhaps at a minimum offer it as a workaround in the linked issue or a gist.

Problem: react-navigation offers support for react-native-web, but browser history works in unpredictable ways with more advanced navigator setups (e.g. nested drawers, updating params for a drawer screen, etc).

Solution: Include a custom action that manages browser history in a predictable way and one that fits the "web" paradigm and more complex navigator setups.

Maybe there is some way we can generalize this action so it can work for anyone who experiences this problem?

My last remaining concern here is that the custom action we're working on is still super hard to follow - but maybe it would be best to merge this improvement and then I will try to clean it up a bit?

marcaaron

Ok LGTM and tests well.

There is one issue that I noticed while testing - but it exists on main already. Reproduction steps:

Setup web app with more than one chat in LHN
Navigate to / in new tab and ensure there is no history (i.e. cannot press -> in browser)
Open search
Select chat that is not currently in view
Press back button
💥 - chat does not navigate to previous chat in view

(adding additional pages to the history stack will fix this so seems like it only happens if the very first chat we navigate to after the initial is accessed via the Search screen)

OSBotify · 2021-12-03T17:57:22Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

parasharrajat · 2021-12-03T18:02:31Z

Thanks, @marcaaron for the instructions. I would keep an eye on those active issues and Propose a workaround so that it can help others.

My last remaining concern here is that the custom action we're working on is still super hard to follow - but maybe it would be best to merge this improvement and then I will try to clean it up a bit?

Yeah. But I am used to it now. 🤣

There is one issue that I noticed while testing - but it exists on main already

I faced it too sometimes but I thought its a development glitch during switching between branches. I will try to debug.

Thanks for merging it.

OSBotify · 2021-12-07T00:20:43Z

🚀 Deployed to staging by @marcaaron in version: 1.1.17-8 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-12-08T20:12:30Z

🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Fix: navigation to report screen from the SideModals

d1354d9

It fixes various issues - Preserving the proper history - Clearing the old Draft message from the composer

parasharrajat marked this pull request as ready for review November 4, 2021 11:54

parasharrajat requested a review from a team as a code owner November 4, 2021 11:54

MelvinBot removed the request for review from a team November 4, 2021 11:54

botify requested a review from mountiny November 4, 2021 11:54

marcaaron self-requested a review November 4, 2021 18:17

mountiny previously approved these changes Nov 5, 2021

View reviewed changes

parasharrajat mentioned this pull request Nov 8, 2021

[HOLD #11768] [$16000] Desktop - The Back ⌘[ and Forward ⌘] shortcuts keys are not working as expected in LHN #4612

Closed

Merge latest changes from main

4592508

parasharrajat dismissed mountiny’s stale review via 4592508 November 22, 2021 18:22

marcaaron requested changes Nov 22, 2021

View reviewed changes

marcaaron approved these changes Dec 3, 2021

View reviewed changes

marcaaron merged commit 96fff44 into Expensify:main Dec 3, 2021

parasharrajat mentioned this pull request Dec 3, 2021

New Group - Drafted message is not saved if the group doesn't have any messages yet #6490

Closed

marcaaron mentioned this pull request Dec 3, 2021

Improve react-navigation CustomAction #6581

Merged

5 tasks

laurenreidexpensify mentioned this pull request Dec 6, 2021

Focus is lost on compose box when a new message is received #6391

Closed

OSBotify mentioned this pull request Dec 6, 2021

Deploy Checklist: New Expensify 2021-12-06 #6605

Closed

95 tasks

botify mentioned this pull request Dec 8, 2021

[HOLD for payment 2021-12-15] $500 Chat Switcher - Draft message from previous conv is displayed after using chat switcher #4756

Closed

parasharrajat deleted the fix-drafts branch November 20, 2023 13:07

Fix: navigation to report screen from the SideModals #6207

Fix: navigation to report screen from the SideModals #6207

Conversation

parasharrajat commented Nov 4, 2021 • edited

Details

Fixed Issues

Tests | QA Steps

Tested On

Screenshots

Web | Desktop

Mobile Web

iOS

Android

parasharrajat commented Nov 4, 2021

mountiny commented Nov 4, 2021

parasharrajat commented Nov 4, 2021

parasharrajat commented Nov 4, 2021

marcaaron commented Nov 4, 2021

parasharrajat commented Nov 4, 2021 • edited

mountiny commented Nov 5, 2021

parasharrajat commented Nov 5, 2021 • edited

mountiny left a comment

Choose a reason for hiding this comment

mountiny commented Nov 16, 2021

mountiny commented Nov 22, 2021

parasharrajat commented Nov 22, 2021

marcaaron left a comment

Choose a reason for hiding this comment

parasharrajat commented Nov 22, 2021 • edited

marcaaron commented Nov 22, 2021

parasharrajat commented Nov 22, 2021 • edited

marcaaron commented Nov 22, 2021

parasharrajat commented Nov 22, 2021

marcaaron commented Nov 22, 2021

parasharrajat commented Nov 23, 2021 • edited

marcaaron commented Nov 23, 2021

marcaaron commented Nov 23, 2021

parasharrajat commented Nov 23, 2021 • edited

marcaaron commented Nov 23, 2021

parasharrajat commented Nov 23, 2021 • edited

marcaaron commented Nov 23, 2021

parasharrajat commented Nov 23, 2021 • edited

marcaaron commented Nov 23, 2021

marcaaron commented Nov 23, 2021

parasharrajat commented Nov 23, 2021 • edited

parasharrajat commented Nov 26, 2021 • edited

We use them for.

Does the latest react-navigation solves any of the issues

Open issues with react-navigation for ones that are not solved

My suggestion

parasharrajat commented Nov 26, 2021

parasharrajat commented Nov 30, 2021

parasharrajat commented Dec 3, 2021

marcaaron commented Dec 3, 2021

marcaaron left a comment

Choose a reason for hiding this comment

OSBotify commented Dec 3, 2021

parasharrajat commented Dec 3, 2021

OSBotify commented Dec 7, 2021

OSBotify commented Dec 8, 2021

parasharrajat commented Nov 4, 2021 •

edited

parasharrajat commented Nov 4, 2021 •

edited

parasharrajat commented Nov 5, 2021 •

edited

parasharrajat commented Nov 22, 2021 •

edited

parasharrajat commented Nov 22, 2021 •

edited

parasharrajat commented Nov 23, 2021 •

edited

parasharrajat commented Nov 23, 2021 •

edited

parasharrajat commented Nov 23, 2021 •

edited

parasharrajat commented Nov 23, 2021 •

edited

parasharrajat commented Nov 23, 2021 •

edited

parasharrajat commented Nov 26, 2021 •

edited

Does the latest `react-navigation` solves any of the issues