Migrate E/App to use `PlatformStackNavigation` #49937

chrispader · 2024-09-30T14:32:08Z

@mountiny @ishpaul777 @chiragsalian

Details

This PR migrates the App to use PlatformStackNavigation types and navigator components implemented in #37891

Fixed Issues

$ #37891
PROPOSAL: #37891 (comment)

Tests

Open app
press on search filed
be sure that:
- screen transition is from left to right (Android/iOS/mWeb)
- drawer tranistion is from left to right + content behind drawer is still visible (web)
- screen search can be loaded (i. e. it's not infinite loading) -> [HOLD for payment 2024-03-07] [Android] infinite loading search screen #37156
close screen
- on iOS test that it can be dismissed via gesture from right to left [HOLD for payment 2024-03-14] Gesture to hide the search page should be right to left #37354 and back button
- on mWeb iOS it can be dismissed via gesture from left to right and back button
- on Android can be closed via system gestures and back button
- on web can be closed via back button
open any chat
tap on composer
go to previous screen
- on iOS/mWeb iOS do it via gesture from left to right and back button
- on Android/web do it via back button
be sure there is no keyboard jumps on previous screen -> [HOLD for payment 2024-10-17] [HOLD for payment 2024-06-06] [HOLD #37891] Keyboard jumps around when swiping back from chat to LHN with gesture #37923
open chat again
tap on task
tap on Assignee
be sure there is no infinite loading for the screen -> [HOLD for payment 2024-03-07] Android - Task-Tapping assignee shows infinite loading page #37252
go back to task
tap on Title
be sure keyboard is opened automatically -> [HOLD for payment 2024-03-07] [$500] Android - Task - Keyboard does not open after selecting task title #37273
go back to main page
tap on user avatar (to open Account settings)
tap on wallet, tap on "Add bank account"
tap on "Debit card"
be sure keyboard gets opened automatically -> [HOLD for payment 2024-03-07] [$500] Android - Wallet - The keyboard does not open automatically when the debit card is opened #37325
go back to main page
press "+" in TabBar
tap Request Money
enter any amount
press "Next"
be sure that on participant screen keyboard appears from the bottom -> [HOLD for payment 2024-03-07] [$500] IOU - Keyboard comes out from right to left on participant screen #37257
Verify that no errors appear in the JS console

Offline tests

Same as tests

QA Steps

Same as tests

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

melvin-bot · 2024-09-30T14:32:42Z

@srikarparsi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

srikarparsi · 2024-09-30T16:50:25Z

Re-assigning to @chiragsalian and @mountiny for review since it looks like they have more context.

chrispader · 2024-10-01T13:08:10Z

@mountiny or @chiragsalian please from now on create ad-hoc builds from here. I fixed all remaining reported errors yesterday, so we can probably trigger a new build already.

chrispader · 2024-10-01T16:56:46Z

The ad-hoc iOS build fail is also going to be related to the patch from #49936

ishpaul777 · 2024-10-03T11:05:15Z

@mountiny Please run adhoc builds here when you have a moment, i plan to work on testing this fully today

mountiny · 2024-10-03T23:10:53Z

Running

github-actions · 2024-10-03T23:44:31Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
❌ FAILED ❌	❌ FAILED ❌
The QR code can't be generated, because the android build failed	The QR code can't be generated, because the iOS build failed
Desktop 💻	Web 🕸️
❌ FAILED ❌	https://49937.pr-testing.expensify.com
The QR code can't be generated, because the Desktop build failed

👀 View the workflow run that generated this build 👀

chrispader · 2024-10-04T08:55:47Z

@mountiny Please run adhoc builds here when you have a moment, i plan to work on testing this fully today

The PR didn't have the changes to the react-native-vision-camera patches up until now. Please trigger another set of ad-hoc builds :)

mountiny · 2024-10-04T11:46:12Z

https://github.com/Expensify/App/actions/runs/11179336829

github-actions · 2024-10-04T12:29:22Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/49937/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/49937/index.html

Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/49937/NewExpensify.dmg	https://49937.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

mountiny · 2024-10-04T12:50:24Z

Bug on the workspace page

mountiny · 2024-10-04T12:52:06Z

@chrispader Looking great! I think there are two things I have noticed:

There is a noticeable delay of idk 0.5s after clicking on the chat row for the animation to start
The chat finder modal is opening from the bottom, we want to keep the same animation we have now in main - animating from left

ishpaul777 · 2024-10-04T17:46:26Z

package-lock.json

commenting so this shows on my k2 for easy access

chrispader · 2024-10-06T18:10:57Z

Bug on the workspace page

@chrispader Looking great! I think there are two things I have noticed:

There is a noticeable delay of idk 0.5s after clicking on the chat row for the animation to start

The chat finder modal is opening from the bottom, we want to keep the same animation we have now in main - animating from left

@mountiny Both bugs are fixed!

I couldn't really see a difference between Stack navigator and Native-Stack while testing in this PR: (you meant when clicking on an item in the Sidebar right?)

Stack

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-10-06.at.17.38.30.mp4

Native Stack

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-10-06.at.20.09.51.mp4

chrispader · 2024-10-06T18:12:27Z

I will be OOO the next week, but i will hand this over to one of our other Margelo engineers. If there are any urgent issues i can also help with that, but i'm not gonna be on my Macbook unfortunately

ishpaul777 · 2024-10-06T18:19:38Z

I am now available for around ~2 hours i'll pull and start testing this locally

chrispader · 2024-10-30T19:09:41Z

Just found the reason for the white flash in dark mode and dark flash in light mode. For some reason the SafeArea component only on iOS used themeColors.inverse in styles.iPhoneXSafeArea. I think this is either a mistake or a relic from some old code.

I fixed the background color, but i still think the current going back animation is not optimal. The screen content should stay rendered until the screen is unmounted:

Light theme	Dark theme
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-10-30.at.20.01.53.mp4	Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-10-30.at.20.01.27.mp4

…-app

dannymcclain · 2024-10-30T19:40:28Z

i still think the current going back animation is not optimal. The screen should stay rendered until the screen is unmounted

Definitely agree. That 1 second of blankness is scary haha.

chrispader · 2024-10-30T20:17:16Z

i still think the current going back animation is not optimal. The screen should stay rendered until the screen is unmounted

Definitely agree. That 1 second of blankness is scary haha.

Yea, UX-wise not really nice... The problem is mostly about how we display modals on top of multiple screens. We want to be able to display any modal outside of the screens, so that they can continue rendering and animating out of the viewport, while the underlying screen is unmounting.

I'm working on a solution to get the same/similar behaviour as on main:

Animation	In detail
ScreenRecording_10-30-2024.20-57-13_1.MP4	Screen.Recording.2024-10-30.at.21.23.32.mov

ishpaul777 · 2024-10-31T21:14:40Z

@chrispader Ping me when this ready for testing again

mountiny · 2024-11-01T11:42:08Z

Thanks for digging, we are getting there 🤞

chrispader · 2024-11-01T14:44:04Z

cc @mountiny @ishpaul777

Ok so here's broader overview of the problem with modals under @react-navigation/native-stack:

I created a simple reproducer repo using the same environment as in E/App in this PR. (RN 0.75.2, NativeStack, RNModal, etc.)

The flow i was testing is the following: The user clicks on the "Confirm delete" button inside a modal in a nested screen. Once the button is clicked, that modal (react-native-modal) will be dismissed while simultaneously the screen gets dismissed (navigation.goBack()).

I tested whether the flicker/content disappearing during the screen unmount animation only occurs on new arch or also old arch. Additionally i tested the same flow with modal screens from @react-navigation instead of react-native-modal component.

I can now verify, that the screen content disappears only under new architecture. I'm now also pretty certain, that this must be caused by react-native-screens, since it also happens with modal screens rather than the RN modal component.

These videos show the results of my testing (to see the issue in detail, you might want to slowly go through the video manually)

react-native-modal component:

Old arch	New arch
RNModal.old.arch.mp4	RNModal.new.arch.mp4

@react-navigation modal screens:

Old arch	New arch
ModalScreen.old.arch.mp4	ModalScreen.new.arch.mp4

Also tagging @WoLewicki and @satya164 here, since you might have a better understanding of the (native) cause of this issue under new architecture :)

chrispader · 2024-11-01T14:54:32Z

We can already see the problem with react-native-modal components in use cases where the screen is dismissed at the same time as the modal.

My general suggestion would be to completely ditch react-native-modal and instead use modal screens and react-native-bottom-sheet for the most part of the app, but that would mean re-structuring a lot of code.

Also the way forward with modal libraries has been discussed recently in this thread, and i think we decided on keeping react-native-modal and continue maintaining it ourselves...

A fix for the issue in the context of this PR would be to replace only the modals in screens that are currently affected by this simultaneous dismissal of screen and modal. E.g.

TagSettingsPage.tsx
CategorySettingsPage.tsx
(we would need to identify all other potentially problematic screens)

chrispader · 2024-11-04T10:16:50Z

cc @mountiny @ishpaul777

Ok so here's broader overview of the problem with modals under @react-navigation/native-stack:
...
I can now verify, that the screen content disappears only under new architecture. I'm now also pretty certain, that this must be caused by react-native-screens, since it also happens with modal screens rather than the RN modal component.

Also tagging @WoLewicki and @satya164 here, since you might have a better understanding of the (native) cause of this issue under new architecture :)

@WoLewicki @satya164 do you guys know what could be causing this issue natively (on new arch)? I feel like this is a more low-level issue, since the screen content disappearing is also easily reproduible in a simple repro app.

…-app

ishpaul777 · 2024-11-04T14:23:27Z

Started a thread on slack to have a real time conversation https://expensify.enterprise.slack.com/archives/C01GTK53T8Q/p1730730136540929

mountiny · 2024-11-04T15:02:06Z

Thanks, bumped the thread

WoLewicki · 2024-11-05T14:48:15Z

Ok I think I got to the root of the issue. It consists of 2 things.

First is that we still have unnecessary logic in react-native-screens concerning creating a snapshot of view on navigating back from a screen. After a patch I got it working 🚀. It fixes the problem when having modals from react-native-screens.
Second one is tougher. We have a modal from react-native-modal that is not controlled by react-native-screens so we don't know that we should make a snapshot of it when we dismiss the whole screen. I will try and investigate it if we can handle it anyhow but looks tricky 😕

chrispader · 2024-11-05T17:08:26Z

First is that we still have unnecessary logic in react-native-screens concerning creating a snapshot of view on navigating back from a screen. After a patch I got it working 🚀. It fixes the problem when having modals from react-native-screens.

@WoLewicki Amazing, thanks for fixing this! 🚀❤️

Second one is tougher. We have a modal from react-native-modal that is not controlled by react-native-screens so we don't know that we should make a snapshot of it when we dismiss the whole screen. I will try and investigate it if we can handle it anyhow but looks tricky 😕

I thought this one might be tricker. I think the optimal solution would be to use modal screens (presentation: 'transparentModal') from react-navigation anyway, but if it's possible to make this work with react-native-modal as well, that would be awesome.

@mountiny if it's not possible to make the second issue work, i think we might have to adapt the modal change in a separate PR - at least if we want migrate consistently for the whole app.

We could also just switch to modal screens for the screens that are currently affected by this problem (only modals that get dismissed at the same time as the screen)

mountiny · 2024-11-06T02:21:38Z

I agree let's just explore this for a bit but we can look into this in a follow up if its too complex

WoLewicki · 2024-11-06T15:35:41Z

After testing and thinking about it a little more, I think the problem is kinda impossible to solve in sensible way with new architecture. Navigating back just removes one of Screen components from React view hierarchy. There are a few key points to that:

That action is interpreted in native stack as a dismissal of modal or normal screen, the snapshot of the current view hierarchy is made (otherwise we would animate empty screen since there are no react views there now) and the going back animation is carried out on that snapshot: https://github.com/software-mansion/react-native-screens/blob/960873aa03b9e22136d2f7b3b272e407011568d2/ios/RNSScreen.mm#L1833-L1843.
Pure Modal component from react-native does something similar to react-native-screens. When it sees that the component has been removed on the native side, it makes the snapshot and removes the view with native animation if it was specified.
react-native-modal is not using the native iOS transitions for showing/dismissing the modal and it is the biggest problem of it. It pushes/dismisses the modal without animation and uses Animated under the hood to animate the views: https://github.com/react-native-modal/react-native-modal/blob/6624b5abb326470820f9fedec8b3ecdb35520869/src/modal.tsx#L834. Because of this, when we navigate back, the react views are dropped, there is no native animation specified and the modal just disappears immediately.

Because of all of this, there is simply no way to have the animation of modal from react-native-modal hiding when navigating back, and it can also be seen on old arch. We could theoretically try and animate the modal as swiping to the right same as the screen below, but it is problematic also, since modal is bigger than the screen when having native header in that screen underneath (it is so because the header is not part of the screen and it does not take part in the go back animation). It would probably work in Expensify since we don't use the native header here but I am not sure if it is what users would expect either way.

The possible solutions I see is either not using react-native-modal library, or, what's probably easier, wait till the modal hides and then do the navigation on onModalHide callback, which seems the natural way of how it should be done. I might be missing something though.

kirillzyusko

A new release of react-native-screens was just released: https://github.com/software-mansion/react-native-screens/releases/tag/4.0.0

I'll leave comments which patches can be removed after upgrade (@chrispader I think it would be good to update now and remove patches?)

This patch is not present in this PR, but also can be removed: https://github.com/Expensify/App/blob/main/patches/react-native-screens%2B3.34.0%2B002%2Bios_from_left_animation.patch

kirillzyusko · 2024-11-07T10:03:57Z

patches/react-native-screens+3.34.0+003+fabric-flat-list-fix.patch

+                     startTransitionRecursive(child.toolbar)
+                 }
+                 if (child is ViewGroup) {
+                    // a combination of https://github.com/software-mansion/react-native-screens/pull/2307/files and https://github.com/software-mansion/react-native-screens/pull/2383/files


This patch can be removed if we switch to RNS 4.0

kirillzyusko · 2024-11-07T10:04:37Z

patches/react-native-screens+3.34.0+004+ios-custom-animations-native-transitions.patch

+-static const float RNSSlideCloseTransitionDurationProportion = 0.25 / 0.35;
+-static const float RNSFadeCloseTransitionDurationProportion = 0.15 / 0.35;
+-static const float RNSFadeCloseDelayTransitionDurationProportion = 0.1 / 0.35;
+static const float RNSFadeOpenTransitionDurationProportion = 0.2 / 0.5;


This patch can be removed if we switch to RNS 4.0

simple_push animation curve transition has been fixed, so this patch is not needed anymore

mountiny · 2024-11-07T10:24:54Z

Thanks for looking into this, i think we will try not to hold on this and get this merged. Lets do a final check over, update the screens version and remove patches - then merge main and i will make a new build and test it some. Also lets verify one last time after those changes it works in hybrid app (and no specific changes are required there) and we can ship it

WoLewicki · 2024-11-07T10:50:26Z

Are you sure you want to bump react-native-screens to v4? Officially it only supports @react-navigation v7: https://github.com/software-mansion/react-native-screens#usage-with-react-navigation. Wouldn't you want to do it in separate PR? There are many many things that come with that new version so it sounds pretty reasonable to do it in a follow-up PR. Or am I missing something here?

kirillzyusko · 2024-11-07T11:04:36Z

Are you sure you want to bump react-native-screens to v4? Officially it only supports @react-navigation v7: https://github.com/software-mansion/react-native-screens#usage-with-react-navigation. Wouldn't you want to do it in separate PR? There are many many things that come with that new version so it sounds pretty reasonable to do it in a follow-up PR. Or am I missing something here?

Oh, then you are probably right - I didn't know v4 is compatible only with react-navigation v7. Then it definitely makes sense to handle it in a follow up PR 👀

mountiny · 2024-11-07T11:16:52Z

+1 actually, this will be better to handle in a separate PR as this is already quite large

chrispader requested a review from a team as a code owner September 30, 2024 14:32

melvin-bot bot requested review from srikarparsi and removed request for a team September 30, 2024 14:32

chrispader changed the title ~~[WIP] Use PlatformStackNavigation in E/App (introduce native-stack navigation on native)~~ [WIP] Use PlatformStackNavigation in E/App (introduce native-stack navigation on mobile) Sep 30, 2024

chrispader changed the title ~~[WIP] Use PlatformStackNavigation in E/App (introduce native-stack navigation on mobile)~~ [WIP] Migrate E/App to use PlatformStackNavigation Sep 30, 2024

kirillzyusko mentioned this pull request Sep 30, 2024

[NoQA] Perf: PlatormStackNavigation - Use @react-navigation/native-stack on mobile platforms #37891

Merged

48 tasks

chrispader changed the title ~~[WIP] Migrate E/App to use PlatformStackNavigation~~ [HOLD on #37891] Migrate E/App to use PlatformStackNavigation Sep 30, 2024

chrispader force-pushed the @chrispader/use-platform-stack-navigation-in-app branch from 3b921d3 to 5701c46 Compare September 30, 2024 16:30

srikarparsi requested review from chiragsalian and mountiny and removed request for srikarparsi September 30, 2024 16:50

mountiny assigned chrispader Oct 1, 2024

mountiny added the Ready To Build label Oct 1, 2024

This comment has been minimized.

Sign in to view

ishpaul777 reviewed Oct 4, 2024

View reviewed changes

package-lock.json Outdated

Copy link

Contributor

ishpaul777 Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commenting so this shows on my k2 for easy access

chrispader added 4 commits October 30, 2024 20:11

fix: use correct Navigation theme based on theme

f6409c4

remove unused onSubmit prop

6a7b166

fix: remove onSubmit prop type

17a76e3

fix: modal stack background style

4b420a5

chrispader requested a review from a team as a code owner October 30, 2024 19:12

Merge branch 'main' into @chrispader/use-platform-stack-navigation-in…

2409969

…-app

chrispader added 3 commits October 30, 2024 21:31

fix: use shouldUseNarrowLayout instead of isSmallScreenWidth

756fd60

fix: typo

8538b30

fix: unused onSubmit

00a9c75

add empty line at the end

5485e44

Merge branch 'main' into @chrispader/use-platform-stack-navigation-in…

a0404e6

…-app

kirillzyusko reviewed Nov 7, 2024

View reviewed changes

Migrate E/App to use PlatformStackNavigation #49937

Are you sure you want to change the base?

Migrate E/App to use PlatformStackNavigation #49937

Conversation

chrispader commented Sep 30, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Sep 30, 2024

srikarparsi commented Sep 30, 2024

chrispader commented Oct 1, 2024

This comment has been minimized.

chrispader commented Oct 1, 2024

ishpaul777 commented Oct 3, 2024

mountiny commented Oct 3, 2024

github-actions bot commented Oct 3, 2024

chrispader commented Oct 4, 2024

mountiny commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

mountiny commented Oct 4, 2024

mountiny commented Oct 4, 2024 • edited Loading

ishpaul777 Oct 4, 2024

Choose a reason for hiding this comment

chrispader commented Oct 6, 2024 • edited Loading

Stack

Native Stack

chrispader commented Oct 6, 2024

ishpaul777 commented Oct 6, 2024 • edited Loading

chrispader commented Oct 30, 2024 • edited Loading

dannymcclain commented Oct 30, 2024

chrispader commented Oct 30, 2024 • edited Loading

ishpaul777 commented Oct 31, 2024

mountiny commented Nov 1, 2024

chrispader commented Nov 1, 2024 • edited Loading

chrispader commented Nov 1, 2024

chrispader commented Nov 4, 2024

ishpaul777 commented Nov 4, 2024 • edited Loading

mountiny commented Nov 4, 2024

WoLewicki commented Nov 5, 2024

chrispader commented Nov 5, 2024 • edited Loading

mountiny commented Nov 6, 2024

WoLewicki commented Nov 6, 2024

kirillzyusko left a comment

Choose a reason for hiding this comment

kirillzyusko Nov 7, 2024

Choose a reason for hiding this comment

kirillzyusko Nov 7, 2024

Choose a reason for hiding this comment

mountiny commented Nov 7, 2024

WoLewicki commented Nov 7, 2024

kirillzyusko commented Nov 7, 2024

mountiny commented Nov 7, 2024

Migrate E/App to use `PlatformStackNavigation` #49937

Migrate E/App to use `PlatformStackNavigation` #49937

chrispader commented Sep 30, 2024 •

edited

Loading

mountiny commented Oct 4, 2024 •

edited

Loading

chrispader commented Oct 6, 2024 •

edited

Loading

ishpaul777 commented Oct 6, 2024 •

edited

Loading

chrispader commented Oct 30, 2024 •

edited

Loading

chrispader commented Oct 30, 2024 •

edited

Loading

chrispader commented Nov 1, 2024 •

edited

Loading

ishpaul777 commented Nov 4, 2024 •

edited

Loading

chrispader commented Nov 5, 2024 •

edited

Loading