Bump react-navigation versions

[marcaaron]

Details

Update includes an upstream fix for the linked issue

Fixed Issues

$ #8101
$ #7618
$ #8641

Tests

Web navigation

1. Open a incognito window and navigate to New Expensify. Log in and close the tab.
2. In the new tab open one of these links:
3. http://localhost:8080/new/chat
4. http://localhost:8080/search
5. Open any chat
6. Verify chosen conversation opens and user is not navigated away from the app

Test mobile navigation for any obvious regressions

• Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • MacOS / Desktop
• I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained “why” the code was doing something instead of only explaining “what” the code was doing.
  • I verified any copy / text shown in the product was added in all src/languages/* files
  • I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named “index.js”. All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • Any functional components have the displayName property
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose and it is
• If a new CSS style is added I verified that:
  • A similar style doesn’t already exist
  • The style can’t be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.

PR Reviewer Checklist

• I verified the correct issue is linked in the ### Fixed Issues section above
• I verified testing steps are clear and they cover the changes made in this PR
  • I verified the steps for local testing are in the Tests section
  • I verified the steps for Staging and/or Production testing are in the QA steps section
  • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
• I checked that screenshots or videos are included for tests on all platforms
• I verified tests pass on all platforms & I tested again on:
  • iOS / native
  • Android / native
  • iOS / Safari
  • Android / Chrome
  • MacOS / Chrome
  • MacOS / Desktop
• I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
• I verified proper code patterns were followed (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained “why” the code was doing something instead of only explaining “what” the code was doing.
  • I verified any copy / text shown in the product was added in all src/languages/* files
  • I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named “index.js”. All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I verified that this PR follows the guidelines as stated in the Review Guidelines
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • Any functional components have the displayName property
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose and it is broken down into smaller components in order to separate concerns and functions
• If a new CSS style is added I verified that:
  • A similar style doesn’t already exist
  • The style can’t be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.

QA Steps

Web navigation

1. Open a incognito window and navigate to New Expensify. Log in and close the tab.
2. In the new tab open one of these links:
3. https://staging.new.expensify.com/new/chat
4. https://staging.new.expensify.com/search
5. Open any chat
6. Verify chosen conversation opens and user is not navigated away from the app

Test mobile navigation for any obvious regressions

• Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Desktop

iOS

Android

[marcaaron]

Sorry, I took this out of draft before testing on all platforms. Doing some testing on native now and putting this back on HOLD for now.

[marcaaron]

Adding @parasharrajat to help with testing.

[marcaaron]

Testing well on web, desktop, iOS and Android. The changes we are looking for should only affected web platforms so all that's left to check is mobile web.

[marcaaron]

Ok, something is off on mobile web with the test flow. Checking to see if the same happens on main seems like something to do with the "default drawer status" stuff. If it's the same on main then I think not a blocker but we should fix how this works, because it feels pretty broken (but at least does not exit the app anymore 😆).

2022-07-05_11-28-27.mp4

[marcaaron]

Hmm actually I can't test at all on main because of the issue described here 😅

[parasharrajat]

I will test this today. Testing.

[ahmdshrif]

@marcaaron I had the same issue ( #9718 (comment) )when I upgrade all react-navigation versions to lates
but if you only upgrade the @react-navigation/native to 6.0.11 which contains your fix.
it will work fine.

I guess the @react-navigation/drawer changed some logic in the late version. and we need to do some changes before we will be able to use it

that are the versions that work for me.

[marcaaron]

I guess the @react-navigation/drawer changed some logic in the late version. and we need to do some changes before we will be able to use it

Thanks. It seems reasonable to downgrade back to the current @react-navigation/drawer version.

But maybe we can create a new issue to look into why that happens.

[marcaaron]

Ok seems to be working fine now, thanks @ahmdshrif

[parasharrajat]

Tests:

1. [$8,000] When entering a url directly in browser on web, user is navigated away from the app on first interaction - Reported by: @ahmdshrif #8101 on Web. ✔️
2. Strange navigation behavior on web. Not sure if that counts in this PR.

screen-2022-07-08_03.15.11.mp4

There are two issues(navigation is not reliable).

1. Sometimes correct chat does not open from the search page (I previously reported this but QA was not able to reproduce). Not related to this issue.

2. Sometimes the back button takes you back to home page. Steps are:

• Open the app with localhost:8080 (home url).
• Open search page, go to any chat.
• press back once.
• Now open the search page again.
• Open any chat.
• Press Back button.
• Sometimes takes you to the home page of the browser.

Running more tests...

[marcaaron]

Nice, thanks @parasharrajat! Also please let me know if any issues you find are also on main? If they are not on main we can try to isolate what the reasons might be. If they are then we can create new issues to address them.

[parasharrajat]

Sorry, I didn't understand. Currently, nothing is decided. I am only reporting bugs.

[marcaaron]

@parasharrajat any update here?

[parasharrajat]

I will share the update in two hours. Although this does not affect native platforms, I am still testing them.

---

Edit: List is ready. Doing a check on MWeb.

[parasharrajat]

Ignoring the existing known navigational issues, the following is the report:

WEB

1

• Open app from home url (localhost:8080).
• Opening a new Chat from RHN(SearchPage)
• Navigating back.

[main] ✅ [PR] ✔️ Fixed.

2

• Open app from home url (localhost:8080).
• Opening a new Chat from RHN (SearchPage)
• Navigate to another chat from RHN.
• Go Back

[main] ✅ [PR] ❌ Not Fixed.

3

• Open app from home url (localhost:8080).
• Opening a new Chat from RHN (NewChatpage)
• Go Back

[main] ✅ [PR] ❌ Not Fixed.

4

• Open app from home url (localhost:8080).
• Opening a new Chat from RHN (NewChatpage)
• Repeat step 2.
• Sometimes correct chat does not open(I previously reported this but QA was not able to reproduce).

[main] ✅ [PR] ❌ Not Fixed. Not related to this issue.

5

• [$8,000] When entering a url directly in browser on web, user is navigated away from the app on first interaction - Reported by: @ahmdshrif #8101

[main] ✅ [PR] ✔️ Fixed.

6

• Open the app directly from RHN urls (/chat/new or /search).
• Go back from browser.

[main] ✅ [PR] ❌ Not Fixed. Not sure of this is a bug.

7 (Not reported yet)

• Open the Payments page directly via http://localhost:8080/settings/payments.
• Click on the back arrow on the Modal which should take you back to Settings page.
• Now press close icon on the Settings Modal.
• User is taken back to Payments page.

[main] ✅ [PR] ❌ Not Fixed.

mWeb

1 #7618

• Open app from home url (localhost:8080).
• Opening a new Chat from RHN(NewChatPage)
• Navigating back via pressing Back button on the Header.

[main] ✅ [PR] ✔️ Fixed.

2

• Open app from home url (localhost:8080).
• Opening a new Chat from RHN(NewChatPage)
• Navigating back via pressing Hardware Back button on Android Chrome.

[main] ✅ [PR] ❌ Not Fixed.

3

• Open the app directly from RHN urls (/chat/new or /search or /settings/payments).
• Go back from hardware back button.

[main] ✅ [PR] ❌ Not Fixed. Not sure of this is a bug.

4

• Open the Payments page directly via http://localhost:8080/settings/payments.
• Click on the back arrow on the Modal which should take you back to Settings page.
• Now press Hardware back button on Android Chrome.
• User is taken back to Payments page.

[main] ✅ [PR] ❌ Not Fixed.

5

• Open the Payments page directly via http://localhost:8080/settings/payments.
• Click on the back arrow on the Modal which should take you back to Settings page.
• Now press Hardware back button on Android Chrome.
• User is taken back to Payments page.
• Now press Close icon on the Payments page.
• User is taken back to home page.

[main] ✅ [PR] ❌ Not Fixed.

[marcaaron]

Hey @parasharrajat to clarify - are you saying that all of the issues where you put:

[main] ✅ [PR] ❌ Not Fixed

that the issue is not present on main and was introduced by updating the react-navigation version?

[parasharrajat]

Sorry for the confusion.

[main] ✅ [PR] ❌ Not Fixed.

means present on main and not fixed in this PR.

[main] ✅ [PR] ✔️ Fixed.

means present on main and fixed in this PR.

[marcaaron]

Nice, so no regressions? @mountiny shall we 🚢 this?

We can create issues to look into the remaining things discovered here.

[mountiny]

Great testing @parasharrajat!

At least getting 3/12 observed problems being fixed is very nice. We should create follow-up issues for those which do not have their own yet, although I am not sure if we should export them all separately as they will most likely have a similar solution.

[mountiny]

@marcaaron I have also updated the PR body to include the #7618 in the fixed issues sections as it seems that problem is being resolved by this PR.

Feel free to self-merge once ready :)

[melvin-bot]

@marcaaron looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

[marcaaron]

Tests passed. Get with it melv.

[marcaaron]

@parasharrajat are you able to produce videos for these failing test cases? I think it would be help us create tickets for them and determine if they are worth fixing or not.

[parasharrajat]

I will record them and share here as soon as possible.

[marcaaron]

Web:

2, 3 - Most likely have the same root cause, but I think the reason is because we are holding something wrong. When we navigate to a new report we are not adding it to the browser history. react-navigation only sees that we are going from a state where there were 2 routes [ [Home, Report], Search ] in a stack. And when we get rid of the Search route we are just back to [ [Home, Report] ] and going back again will take you to -1. I'm not entirely sure how we can solve this problem, but we've known about it for a while.

Probably we need to...

• First confirm that it even makes sense to expect that a drawer navigator would have an internal history. It seems like the answer from react-navigation is that NAVIGATE is the wrong action type to use. They suggested using PUSH. We have tried to get around this in the past with the CustomActions stuff but it's a mess.

• IMO we would need to present a strong argument that this belongs as a feature in react-navigation and specifically that the history stack could be enabled to work like this...

• User navigates to Home history = [[Home + chat]]

• User moves to search history = [[Home + chat], Search]

• Another chat is selected history = [[Home + previous chat], [Home + current chat]]

Then some logic to say that "going back" should update the "Report" screen without having to have multiple LHN instances. I think the problem we ran into with this is that when we are pushing a drawer route on top of another drawer then rendering gets really expensive since n chats and sidebars would get stacked on top of each other.

Preserving our own history could be a possible option here - but not entirely sure.

4 - Let's get a reliable repro before doing anything about that one.

6 - Not sure if this is a bug as we'd need to basically push the "home" route into the memory history. Who knows if that's what other people want to use react-navigation for - so we'd need to build a case that this is how it should work and pitch it to the maintainers. I think there is a possibly usability argument that if you land on a particular sub-route from somewhere else then "going back" should take you to whatever it is you were doing before - checking your email, watching youtube, etc.

mWeb

2 - Seems the same as web-2 and web-3

Have to consider the rest later... sure some of them are related.

[marcaaron]

I think this issue is also solved? #8641

Tested, but would appreciate a second check @parasharrajat

[parasharrajat]

I am not able to reproduce this on PROD. The mentioned steps in OP doesn't work for me.

[parasharrajat]

Ok, I see now. There is very strange behavior.

On Firefox, when you try to open a chat that you never chated before, sometimes it does not open and the user is taken back to the previously opened chat.

On Chrome, the new chat does open but when I click on the chat in LHN, reportID in URL changes but not the active chat.

Note: I am running all of these tests on Linux OS ATM.

screen-2022-07-14_01.24.26.mp4

[marcaaron]

Hmm that doesn't look quite like the bug that was reported. You were not navigated out of the app.

[OSBotify]

🚀 Deployed to production by @chiragsalian in version: 1.1.84-13 🚀

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

[parasharrajat]

Sorry for the delay here. Should I report these issues on Slack? cc: @mountiny

Issue: Navigating Back takes the user to the home screen on the web.

Steps:

1. Open the app from the home URL (https://staging.new.expensify.com).
2. Opening a new Chat from RHN (SearchPage)
3. Navigate to another chat from RHN (SearchPage).
4. Go Back.

Expected Result: The user should not leave the app.

screen-2022-07-26_04.25.02.mp4

---

Issue: Navigating Back takes the user to the home screen on the web.

Steps WEB:

1. Open the app from the home URL (https://staging.new.expensify.com).
2. Opening a new Chat from RHN (NewChat Page)
3. Go Back.

Steps mWEB:

1. Open the app from the home URL (https://staging.new.expensify.com) on Android Chrome.
2. Opening a new Chat from RHN(NewChatPage).
3. Navigate back via pressing the Hardware Back button.

Expected Result: The user should not leave the app.

screen-2022-07-26_04.30.40.mp4

screen-2022-07-26_05.27.55.mp4

---

Issue: Navigating Back takes the user to the home screen on the web when app is directly opened via RHN links. For example ( https://staging.new.expensify.com/new/chat)

Steps WEB:

1. Open the app from the RHN URL (https://staging.new.expensify.com/new/chat).
2. Go Back.

Steps MWeb:

1. Open the app from the RHN URL (https://staging.new.expensify.com/new/chat).
2. Press the hardware back button.

Expected Result: The user should not leave the app.

screen-2022-07-26_04.37.55.mp4

screen-2022-07-26_05.30.36.mp4

---

Not able to reproduce WEB 4, 7 on staging web app.

---

ISSUE MWeb 4, 5 are not reproducible on staging anymore.

[mountiny]

@parasharrajat I think sticking to the bug reporting process is good unless those issues already exist in GH and you know about them.

We might want to monitor it too and put it under one project or some tracking issue as we have done before. Thank you!

[marcaaron]

We have this project -> https://github.com/Expensify/App/projects/2 used for navigation related issues that I have been using as our "wish list".