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

kirillzyusko · 2024-03-07T12:34:04Z

Details

Implements a components and types for a PlatformStackNavigation, which uses Stack Navigator on web/desktop/mWeb and Native Stack Navigator on mobile.

There is another PR which migrates the App to use the PlatformStackNavigation API: #49937

Native Stack Navigator provides a way for your app to transition between screens where each new screen is placed on top of a stack. This creates a more natural feel on mobile platforms and improves performance, since native screen transitions don't get blocked if the JS thread is blocked.

This was discussed here: #vip-newdot-performance

Warning

This is a second revision of the implementation. The first one caused a lot of issues, especially when it comes to keyboard appearance on iOS, transition events detection on Android. A successor of #29991

The main difference comparing to the first version of implementation is the fact that now we use native-stack everywhere on mobile. Before, on different stack navigators level we were using different navigators.

Some libraries had to be patched in order for this PR to work:

feat(iOS): Add slide_from_left transition software-mansion/react-native-screens#2057 <- added slide_from_left transition for Search/Workspace pages
feat: added integration for InteractionManager with native-stack react-navigation/react-navigation#11887 <- fixed an integration with InteractionManager (used in useSingleExecution hook).
@react-navigation/core: Exposed types by exporting them and added/adapted some of the generic parameters, so that we can forward navigator's ParamLists
@react-navigation/native-stack: Add the keyboardHandlingEnabled prop on native-stack
react-native-screens: Add the ios_from_left animation <- This is already merged upstream but not in any stable release yet

Fixed Issues

$ #37923
$ #29948
PROPOSAL: #29948

Tests

No specific tests steps as this PR is not putting the new navigator in use yet. The testing will be done here #49937

Offline tests

Not needed.

QA Steps

No specific tests steps as this PR is not putting the new navigator in use yet. The testing will be done here #49937

PR Author Checklist

Screenshots/Videos

Android: Native

telegram-cloud-document-2-5442872807086113148.mp4

Android: mWeb Chrome

telegram-cloud-document-2-5449560084050887588.mp4

iOS: Native

76cd68bb-6ef8-46d7-9609-cf0ad3964289.mp4

iOS: mWeb Safari

5aa0fca4-72a3-4441-a56f-57da3d095883.mp4

MacOS: Chrome / Safari

a8d30d7e-8aad-4ac0-affe-83ab19584109.mp4

MacOS: Desktop

f6fe3999-d45a-49a2-9e6a-cc5c8e5764d3.mp4

MacOS Chrome (Arc) screen recording attached.

melvin-bot · 2024-03-12T16:30:53Z

@cubuspl42 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]

kirillzyusko · 2024-03-12T17:02:19Z

@mountiny I think this PR is ready for review 👀 And from my testing experience - the second version feels much more stable than initial implementation 💪

mountiny · 2024-03-12T22:56:35Z

@kirillzyusko for easier QA could you please list the test scenarios from the regressions in the PR body? QA will then have easier life once this hits staging

mountiny · 2024-03-12T22:57:04Z

@cubuspl42 Createing a new build, will you be able to test this one tomorrow?

github-actions · 2024-03-12T23:17:59Z

🧪🧪 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/37891/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/37891/index.html

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

👀 View the workflow run that generated this build 👀

src/libs/Navigation/AppNavigator/getRootNavigatorScreenOptions.ts

cubuspl42 · 2024-03-13T09:33:46Z

A tough one to review meaningfully. I left one minor comment.

cubuspl42 · 2024-03-13T10:44:30Z

@kirillzyusko I would also appreciate some summary of the testing steps! Especially if you redacted them for clarity.

kirillzyusko · 2024-03-13T12:40:48Z

@mountiny @cubuspl42 I update test steps 🙌 But you also can test any scenarios that you would like to test 👀

cubuspl42 · 2024-03-14T11:38:45Z

be sure screen transition is from left to right

What I observe: on iOS Native it's right-to-left, on Android Native it's left-to-right. Is this expected? If so, the testing steps should specify it.

kirillzyusko · 2024-03-14T11:57:50Z

What I observe: on iOS Native it's right-to-left, on Android Native it's left-to-right. Is this expected? If so, the testing steps should specify it.

@cubuspl42 I created a patch for that functionality on iOS, so you'll need to reinstall node_modules and re-assemble app again (because I modified native code). Can you confirm that you did it and still see right-to-left transition on iOS? 👀

cubuspl42 · 2024-03-14T12:17:42Z

My bad! I did as you said, works as expected now in that aspect.

cubuspl42 · 2024-03-14T12:18:31Z

Interestingly, on Android I'm able to go back using a gesture from both sides of the screen. Is it expected?

native-stack-v2-android-compressed.mp4

Actually, always takes two tries. Doesn't feel right.

kirillzyusko · 2024-03-14T12:34:20Z

Interestingly, on Android I'm able to go back using a gesture from both sides of the screen. Is it expected?

@cubuspl42 Yes. Actually this is system UI and yes, it can be triggered from any horizontal side of the app (i. e. left or right). And it's simply simulation of hardware/software button press (these buttons were widely adopted in Android < 10), but in Android 11 they decided to make navigation bar smaller and replace all buttons (back button/recent apps/home) with gestures.

So when you perform gesture one time - it'll close a keyboard. When you do it a second time - it'll close a screen.

So this is handled on OS level and it's not something that we can prevent (though we can override an action on the gesture and prevent screen from being hidden, but I wouldn't recommend to do that because many android users are using these system gestures to do a quick navigation in the app).

cubuspl42 · 2024-03-15T09:44:12Z

I understand! Makes sense.

mountiny · 2024-03-15T10:02:07Z

Thanks for chugging along, @cubuspl42 what is your ETA on this one?

cubuspl42 · 2024-03-15T10:33:28Z

I'm resuming the testing, ETA should be < 4h unless something bad is found

cubuspl42 · 2024-03-15T10:47:32Z

@kirillzyusko

You wrote "Not affected" in the Screenshots/Videos on non-Native platforms. Please don't do so in the future in cases like this. Your intention was not to affect these platforms, it's not clear from the change itself that they are not affected.

On a related, but separate topic, the "Tests" sections should be written according to Expensify's "cross-platform 99% of the time" philosophy. Pragmatically speaking, you should note which steps are platform-specfic and you should cover all platforms. Even when it's something as simple as "Ensure that this and this and this interaction (still) works".

cubuspl42 · 2024-03-15T10:49:33Z

Asking about this PR specifcially now: from which side should the search panel enter the screen on mobile web, by design and intention?

cubuspl42 · 2024-03-15T11:03:50Z

On iOS Web, the automatic keyboard opening doesn't work for me. It's not clear if this is a regression or an unrelated known issue. Would you please clarify it?

native-stack-v2-ios-web-compressed.mp4

adamgrzybowski · 2024-10-08T16:29:07Z

I reviewed the PR and I haven't found any change request candidates. Great job! 💪

ishpaul777 · 2024-10-08T16:32:05Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
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 included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- 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 is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- 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 other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
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 */
- 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 any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
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(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
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.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

WhatsApp.Video.2024-10-09.at.1.03.21.AM.mp4

Android: mWeb Chrome

iOS: Native

ScreenRecording_10-09-2024.00-27-42_1.MP4

iOS: mWeb Safari

Screen.Recording.2024-10-08.at.11.46.24.PM.mov

MacOS: Chrome / Safari

Screen.Recording.2024-10-08.at.11.01.55.PM-1.mov

MacOS: Desktop

Screen.Recording.2024-10-09.at.12.53.38.AM-1.mov

src/libs/Navigation/PlatformStackNavigation/types/NavigationOptions.ts

ishpaul777 · 2024-10-08T17:22:04Z

src/libs/Navigation/PlatformStackNavigation/types/NavigationOptions.ts

+    native?: NativeOnlyNavigationOptions;
+
+    keyboardHandlingEnabled?: boolean;
+    animation?: 'slide_from_left' | 'slide_from_right' | 'modal' | 'none';


we have more transition animantions in src/libs/Navigation/PlatformStackNavigation/navigationOptions/animation/index.ts we dont need it here ?

@ishpaul777 I think this type is used only inside withAnimation function and these 4 types acts as preset (for them we return animation + gesture-direction for example)

So I think they are kind of independent and I didn't refactor this part. Does it make sense?

okay makes sense, Thanks for clarifying!

these options for animation is what the user/developer used in order for the PlatformStackNavigation to translate into platform specific navigation options.

We must keep these props in the type (animations, presentation)

github-actions · 2024-10-08T18:39: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/37891/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/37891/index.html

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

👀 View the workflow run that generated this build 👀

src/libs/Navigation/AppNavigator/createCustomPlatformStackBottomTabNavigator/BottomTabBar.tsx

fabioh8010 · 2024-10-09T08:36:35Z

src/libs/Navigation/AppNavigator/createCustomPlatformStackBottomTabNavigator/BottomTabBar.tsx

+        if (selectedTab === SCREENS.HOME) {
+            return;
+        }
+        const route = activeWorkspaceID ? (`/w/${activeWorkspaceID}/${ROUTES.HOME}` as Route) : ROUTES.HOME;


Is possible to put /w/${activeWorkspaceID}/${ROUTES.HOME} on ROUTES.ts?

@fabioh8010 again, I think it's copied/pasted code, so not sure if we need to do that 🙈

And additionally - is it possible to create dynamic routes in ROUTES.ts? Or we want to add two routes and use ternary operator in BottomTabBar.tsx and choose between two routes?

Answered here

fabioh8010 · 2024-10-09T08:45:46Z

...igation/AppNavigator/createCustomPlatformStackFullScreenNavigator/CustomFullScreenRouter.tsx

+    // There should always be WORKSPACE.INITIAL screen in the state to make sure go back works properly if we deeplinkg to a subpage of settings.
+    if (!isAtLeastOneInState(state, SCREENS.WORKSPACE.INITIAL)) {
+        // @ts-expect-error Updating read only property
+        // noinspection JSConstantReassignment


What's the purpose of // noinspection JSConstantReassignment?

@fabioh8010 I think it's a copy-past of another code that was already present in the codebase.

We copied it, so now we have kind of two copies of this code, but I wouldn't worry too much, because in the second PR we will remove the original code 👀

Answered here

...ibs/Navigation/PlatformStackNavigation/navigationOptions/convertToNativeNavigationOptions.ts

src/libs/Navigation/PlatformStackNavigation/navigationOptions/convertToWebNavigationOptions.ts

...ibs/Navigation/PlatformStackNavigation/navigationOptions/convertToNativeNavigationOptions.ts

src/libs/Navigation/PlatformStackNavigation/navigationOptions/convertToWebNavigationOptions.ts

src/types/utils/Substract.ts

chrispader · 2024-10-09T12:23:56Z

i agree with @kirillzyusko that we shouldn't add extra types for the animations defined in navigationOptions directory. these are platform specific navigation options, the user just has to set animation to one of these (4) variants

fabioh8010

Approved from TS side

kirillzyusko · 2024-10-09T16:51:20Z

@mountiny do we wait a review from someone else? 👀

mountiny

LGTM!

mountiny · 2024-10-09T18:03:50Z

src/libs/Navigation/PlatformStackNavigation/navigationOptions/animation/withAnimation.ts

+            return slideFromLeft as PlatformSpecificNavigationOptions;
+        case Animations.SLIDE_FROM_RIGHT:
+            return slideFromRight as PlatformSpecificNavigationOptions;
+        case 'modal':


Would be nice to have this as const too

chiragsalian

Nice, code LGTM. Great work here. Im hype for the performance gains with this 🙂

chiragsalian · 2024-10-09T18:53:44Z

Gonna proceed and merge to keep this moving. Vit left a minor comment but we can address it in the follow up PR.

OSBotify · 2024-10-09T19:05:57Z

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

github-actions · 2024-10-09T20:46:50Z

🚀 Deployed to staging by https://github.com/chiragsalian in version: 9.0.47-1 🚀

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

github-actions · 2024-10-10T23:02:01Z

🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.47-4 🚀

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

mountiny added the Ready To Build label Mar 7, 2024

This comment has been minimized.

Sign in to view

kirillzyusko mentioned this pull request Mar 11, 2024

[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

Closed

6 tasks

mountiny assigned kirillzyusko Mar 11, 2024

kirillzyusko force-pushed the perf/native-stack-v2 branch from e178a2d to 73870c2 Compare March 12, 2024 11:51

kirillzyusko marked this pull request as ready for review March 12, 2024 16:30

kirillzyusko requested a review from a team as a code owner March 12, 2024 16:30

melvin-bot bot removed the request for review from a team March 12, 2024 16:30

melvin-bot bot requested a review from cubuspl42 March 12, 2024 16:30

cubuspl42 suggested changes Mar 13, 2024

View reviewed changes

src/libs/Navigation/AppNavigator/getRootNavigatorScreenOptions.ts Outdated Show resolved Hide resolved

kirillzyusko mentioned this pull request Mar 13, 2024

test: use version from Onyx PR (OOM handling) #37917

Closed

50 tasks

fix: eslint

3325a07

ishpaul777 reviewed Oct 8, 2024

View reviewed changes

src/libs/Navigation/PlatformStackNavigation/types/NavigationOptions.ts Outdated Show resolved Hide resolved

ishpaul777 reviewed Oct 8, 2024

View reviewed changes

src/libs/Navigation/PlatformStackNavigation/types/NavigationOptions.ts Outdated Show resolved Hide resolved

ishpaul777 reviewed Oct 8, 2024

View reviewed changes

thienlnam mentioned this pull request Oct 8, 2024

[$250] Android - Crash when kill the app and reopen it #50463

Open

6 tasks

fix: remove unused properties

6c22124

ishpaul777 approved these changes Oct 9, 2024

View reviewed changes

melvin-bot bot requested a review from mountiny October 9, 2024 09:53

fabioh8010 reviewed Oct 9, 2024

View reviewed changes

kirillzyusko added 2 commits October 9, 2024 13:41

fix: partially resolve new comments

bf06916

fix: removed unused files

3bc381c

fix: refactor types

707e207

fabioh8010 approved these changes Oct 9, 2024

View reviewed changes

kirillzyusko mentioned this pull request Oct 9, 2024

Re-work eslint ignore in certain places #50514

Open

mountiny approved these changes Oct 9, 2024

View reviewed changes

chiragsalian approved these changes Oct 9, 2024

View reviewed changes

chiragsalian merged commit 00b7426 into Expensify:main Oct 9, 2024
23 checks passed

github-actions bot mentioned this pull request Oct 9, 2024

Deploy Checklist: New Expensify 2024-10-09 #50518

Closed

58 tasks

mountiny changed the title ~~Perf: PlatormStackNavigation - Use @react-navigation/native-stack on mobile platforms~~ [NoQA] Perf: PlatormStackNavigation - Use @react-navigation/native-stack on mobile platforms Oct 10, 2024

staszekscp mentioned this pull request Oct 16, 2024

HOLD on Issue #44242 & PR #49937 [$250] IOS - ND hybrid - Back button in About page is not responsive after logging out and relogin #50832

Open

6 tasks

rezkiy37 mentioned this pull request Nov 21, 2024

BUG: Receipt is deleted if app is killed while request is in queue #51761

Open

8 tasks

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

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

Conversation

kirillzyusko commented Mar 7, 2024 • edited by mountiny Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

This comment has been minimized.

melvin-bot bot commented Mar 12, 2024

kirillzyusko commented Mar 12, 2024

mountiny commented Mar 12, 2024

mountiny commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

cubuspl42 commented Mar 13, 2024

cubuspl42 commented Mar 13, 2024

kirillzyusko commented Mar 13, 2024

cubuspl42 commented Mar 14, 2024 • edited Loading

kirillzyusko commented Mar 14, 2024

cubuspl42 commented Mar 14, 2024

cubuspl42 commented Mar 14, 2024 • edited Loading

kirillzyusko commented Mar 14, 2024

cubuspl42 commented Mar 15, 2024

mountiny commented Mar 15, 2024

cubuspl42 commented Mar 15, 2024

cubuspl42 commented Mar 15, 2024

cubuspl42 commented Mar 15, 2024

cubuspl42 commented Mar 15, 2024

adamgrzybowski commented Oct 8, 2024

ishpaul777 commented Oct 8, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrispader commented Oct 9, 2024

fabioh8010 left a comment

Choose a reason for hiding this comment

kirillzyusko commented Oct 9, 2024

mountiny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiragsalian left a comment • edited Loading

Choose a reason for hiding this comment

chiragsalian commented Oct 9, 2024

OSBotify commented Oct 9, 2024

github-actions bot commented Oct 9, 2024

github-actions bot commented Oct 10, 2024

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

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

kirillzyusko commented Mar 7, 2024 •

edited by mountiny

Loading

cubuspl42 commented Mar 14, 2024 •

edited

Loading

cubuspl42 commented Mar 14, 2024 •

edited

Loading

ishpaul777 commented Oct 8, 2024 •

edited

Loading

chiragsalian left a comment •

edited

Loading