Fix wrong behavior on Android web #17962

staszekscp · 2023-04-25T11:22:17Z

Details

Fix for wrong freeze behavior on android web - it introduces wrapper that provides Freeze component to mobile apps (where it is really needed)

Part of navigation rework by Software Mansion

BUG: Web: ANDROID: LHN is reloading when we open any page from the FAB menu.

The problem was caused by the Freeze component - on Android web the the freezing was triggered too early (before screen's transition end). Instead of implementing some hacky solutions I decided to remove the Freeze component from the web flow, because it was causing more problems without giving much perfromance advantage there.

Fixed Issues

Related to #16581 (comment)

Tests

Verify that no errors appear in the JS console

Open the app on the mobile web.
Open the Fab menu and click on any menu.
Observe that images/page are reloaded in the background.

Offline tests

QA Steps

Verify that no errors appear in the JS console

Open the app on the mobile web.
Open the Fab menu and click on any menu.
Observe that images/page are reloaded in the background.

PR Author Checklist

Screenshots/Videos

Web

Mobile Web - Chrome

Sorry for the quality

Screen.Recording.2023-04-26.at.21.17.37.2.mov

Mobile Web - Safari

Desktop

iOS

Android

* test flatlist for android * fix linter * fix name * fix not working features * limit rerenders

MelvinBot · 2023-04-25T11:22:56Z

@neil-marcellini 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]

neil-marcellini · 2023-04-25T17:01:23Z

@staszekscp please follow our contributing guidelines. Were you hired for an issue with an accepted proposal? If so please include those in the description and re-open the PR.

staszekscp · 2023-04-26T06:22:21Z

cc: Hey @mountiny, could you have a look at that?

neil-marcellini · 2023-04-26T17:59:12Z

Thanks for linking the related issue and now I see that you're from Software Mansion so I'll reopen this. I was going quickly and thought this was a random PR opened unrelated to an issue.

mountiny · 2023-04-26T18:22:33Z

@staszekscp thanks! it would be great to add a video too and always try to give more context for the PR :D

mountiny

@staszekscp You got some lint issues here again, hopefully we will get rid of this with Prettier and typestript haha but make sure to check if the npm run lint is fine before submitting ready for a review

mountiny · 2023-04-26T18:27:25Z

@s77rt could you give this one a look sooner than later, thank you very much 🙇

neil-marcellini · 2023-04-26T18:35:10Z

src/pages/home/sidebar/SidebarLinks.js

-                <Freeze
-                    freeze={shouldFreeze}
-                    placeholder={skeletonPlaceholder}
-                >


Instead of putting this in the LHNOptionList and making a platform specific implementation for that entire component, could you please create a SidebarFreeze component that wraps its children in Freeze on all platforms except for web, where it wraps the children in nothing <>?

I think that isolates the platform specific part better. What do you think?

Sure, will do!

That sounds reasonable to me, probably just a preference though. @s77rt what do you think?

I will go thought the files in depth but just a quick response. I assume LHNOptionList is exactly the same and the only difference is Freeze is that right? Then It would be better to isolate that component instead as @neil-marcellini suggested. BUT why not remove the Freeze component, is it really doing something significantly good? It's anti react's life-cycle and we will remove it on navigation reboot anyway.

Just realised this is a part of navigation reboot 😁 I think It was planned to remove Freeze at this point, why we are still using it. Am I missing something here? cc @neil-marcellini @mountiny (and @roryabraham I guess you know about this)

staszekscp · 2023-04-26T18:55:01Z

Sorry @mountiny for the lint issues, I keep forgetting that, because in previous projects I couldn't commit anything without passing a lint check via husky script :P I'll keep that in mind, though, so hopefully I will pass the lint check next time!

neil-marcellini · 2023-04-26T19:14:00Z

Thanks for the SidebarFreeze update! I think as a general rule we should always keep platform specific components as small as possible.

Please include testing videos as described in the PR author checklist. I'm not able to observe the LHN reloading under the global create menu on main. @parasharrajat is that easy to observe or do we need to add logging or otherwise debug to see if the component is updating or remounting? Also could you please write a couple sentences about the problem and how this fixes it, it's not super clear to me.

I'll let @s77rt take the lead on the reviews and come back after he approves.

staszekscp · 2023-04-26T19:15:27Z

@neil-marcellini It is a bug specific to the navigation rework, that's why it is not possible to notice it on the main :)

s77rt · 2023-04-26T19:17:41Z

@neil-marcellini I still have concerns about the use of Freeze, please check comment #17962 (comment) in case you missed it

s77rt

The custom FlatList is a workaround. bug is upstream: software-mansion/react-freeze#31
Why not just remove Freeze?

cc @neil-marcellini

s77rt · 2023-04-26T19:42:27Z

src/pages/home/sidebar/SidebarLinks.js

+                        focusedIndex={_.findIndex(optionListItems, (
+                            option => option.toString() === this.props.currentReportId
+                        ))}
+                        isLoading={isLoading}


I think this is an outdated change. LHNOptionsList does not accept isLoading prop.

s77rt · 2023-04-26T19:43:39Z

src/components/LHNOptionsList/index.js

I don't see why the renamed is required here (another outdated change?)

This is outdated, there was a web file before

s77rt · 2023-04-26T20:10:31Z

src/components/FlatList/index.android.js

All of this is a workaround. The bug is upstream and it has been reported already a while ago software-mansion/react-freeze#31

mountiny

@s77rt I think we want to keep Freeze to limit the number of rerender in the stack. All the navigation will be stack based so if we wont have Freeze around the LHN, it would be rerendering in the background

mountiny · 2023-04-26T20:45:03Z

src/components/LHNOptionsList/index.js

This is outdated, there was a web file before

mountiny · 2023-04-26T20:47:17Z

@s77rt do you want me to make internal release of this branch for testing or are you fine with emulators?

s77rt · 2023-04-26T21:01:12Z

@mountiny I'm fine with emulators. Unfortunately we will have to discuss some more before moving with this PR

How much performance are we actually gaining by using Freeze, is it that significant?
We have a couple of issues that are on hold in favor of the removal of Freeze, how are we supposed to handle those?

Besides the custom flat list here is a workaround, we have a bug report for that #14125

mountiny · 2023-04-26T21:10:20Z

Essenatilly the implementation of the refactor means we will be pushing many reports to the stack, they wont be reused so if you will use app for a long time you will have a lot of pages. I think we need to use the Freeze for ReportScreens at least for now to ensure that those are not being rerendered in the background.

I am sure also we can work with SWM to fix the react-freeze issues, they are maintainers of the library

Also is this really only occurring on Android web or also on other platforms?

s77rt · 2023-04-26T21:41:37Z

@mountiny

On Android - Chrome it's causing the issue pointed by @parasharrajat on [Navigation Reboot] Swap the DrawerNavigator for the ResponsiveNavigator #16581 (comment). We are fixing this by not using Freeze.
On Android - Native it's causing [HOLD for payment 2023-06-15] [HOLD for payment 2023-05-31] [$4000] App does not hold scroll position and scrolls to top in LHN on tapping back button from chat page #14125. We are fixing this by applying a workaround.

mountiny · 2023-04-26T22:25:46Z

Thanks, I understand that.

I think what we need to do is completely test what will be influenced (performance mainly) if we remove the freeze completely. Especially Android app hopes to get better performance with dropping the use of drawer so we need to be cautious with what will be the performance consequence of these changes.

Could anyone try to some quick performance audit for removing the freeze? at least in terms of FPS?

mountiny · 2023-04-26T22:50:30Z

Asked in Slack to discuss this https://expensify.slack.com/archives/C01GTK53T8Q/p1682548765642099

parasharrajat · 2023-04-27T02:09:13Z

I'm not able to observe the LHN reloading under the global create menu on main. @parasharrajat is that easy to observe or do we need to add logging or otherwise debug to see if the component is updating or remounting?

@neil-marcellini When you follow the steps from #16581 (comment). Notice that avatars will go grey which means they are reloading. I could also see the skeleton loader on chats.

staszekscp · 2023-04-27T09:47:32Z

I think in the SidebarLinks we don't need the Freeze component so much, because we will not push more than one SidebarLinks screen on the stack. There shouldn't be a huge amount of updates there either.

On the other hand I think it is worth to keep ReportScreen wrapped in the Freeze component, because there may be multiple chats opened, and their re-renders could cause performance harm, especially on slower devices.

An alternative solution that would work to fix this specific Android web issue is to leave the Freeze component, but add a check to see if the screen transition finished. I haven't tested that, but if you prefer this kind of solution I may check this out.

mountiny · 2023-04-27T19:31:13Z

So I feel like:

if we are going to keep the Freeze for the ReportScreen then there is no benefit in removing it from the Sidebar too

We can work with SWM to fix the react-freeze issues, they are keen to fix anything which is wrong with it

mountiny

@parasharrajat @s77rt @staszekscp @neil-marcellini after the rect discussion I think we should be bale to use freeze for the report screen but skip its usage for the sidebar, what do you think?

s77rt · 2023-04-29T16:17:05Z

@mountiny I'm okay with keeping Freeze I'm just questioning how to handle the issues that we assumed will be gone after navigation reboot. However I would still discourage the use of Custom FlatList, I would really prefer to keep a bug than apply a workaround.

neil-marcellini · 2023-05-01T17:22:25Z

So, is this ready for review again? Please message me on NewDot when it is.

mountiny

@s77rt Can you please give this one a review again assuming we will keep the Freeze so we know clearly what we need to address, got a bit lost in the conversation cc @staszekscp thank!

s77rt

One additional change request. And please address the other unresolved comments.

s77rt · 2023-05-02T11:14:05Z

src/components/FlatList/index.android.js

Let remove the CustomFlatList and just use the react-native FlatList

mountiny · 2023-05-05T15:48:57Z

@staszekscp lets follow up on this one next week

mountiny · 2023-05-22T12:43:46Z

Closing this PR in favour of the final one we have here #19263, the changes should be incorporated there.

staszekscp added 2 commits April 24, 2023 10:57

Fix flatlist glitch for android (#9)

dcd9711

* test flatlist for android * fix linter * fix name * fix not working features * limit rerenders

add wrapper for LHN

14bd5e6

staszekscp requested a review from a team as a code owner April 25, 2023 11:22

melvin-bot bot requested review from neil-marcellini and removed request for a team April 25, 2023 11:22

neil-marcellini closed this Apr 25, 2023

neil-marcellini reopened this Apr 26, 2023

mountiny assigned staszekscp Apr 26, 2023

mountiny self-requested a review April 26, 2023 18:15

mountiny reviewed Apr 26, 2023

View reviewed changes

mountiny requested a review from s77rt April 26, 2023 18:27

revert accidentaly deleted comment

f030a59

neil-marcellini requested changes Apr 26, 2023

View reviewed changes

staszekscp added 2 commits April 26, 2023 20:55

switch to SidebarFreeze

15834b9

simplify web SidebarFreeze

e8fc48f

s77rt suggested changes Apr 26, 2023

View reviewed changes

mountiny reviewed Apr 26, 2023

View reviewed changes

src/components/LHNOptionsList/index.js

Copy link

Contributor

mountiny Apr 26, 2023

Choose a reason for hiding this comment

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

This is outdated, there was a web file before

mountiny mentioned this pull request Apr 26, 2023

Remove close button from HeaderWithCloseButton #17963

Closed

56 tasks

mountiny requested review from neil-marcellini and s77rt April 29, 2023 00:29

mountiny reviewed Apr 29, 2023

View reviewed changes

mountiny reviewed May 2, 2023

View reviewed changes

s77rt suggested changes May 2, 2023

View reviewed changes

src/components/FlatList/index.android.js

Copy link

Member

s77rt May 2, 2023

Choose a reason for hiding this comment

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

Let remove the CustomFlatList and just use the react-native FlatList

mountiny closed this May 22, 2023

Fix wrong behavior on Android web #17962

Fix wrong behavior on Android web #17962

Uh oh!

Conversation

staszekscp commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

MelvinBot commented Apr 25, 2023

Uh oh!

neil-marcellini commented Apr 25, 2023

Uh oh!

staszekscp commented Apr 26, 2023

Uh oh!

neil-marcellini commented Apr 26, 2023

Uh oh!

mountiny commented Apr 26, 2023

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

mountiny commented Apr 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staszekscp commented Apr 26, 2023

Uh oh!

neil-marcellini commented Apr 26, 2023

Uh oh!

staszekscp commented Apr 26, 2023

Uh oh!

s77rt commented Apr 26, 2023

Uh oh!

s77rt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mountiny commented Apr 26, 2023

Uh oh!

s77rt commented Apr 26, 2023

Uh oh!

mountiny commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s77rt commented Apr 26, 2023

Uh oh!

mountiny commented Apr 26, 2023

Uh oh!

mountiny commented Apr 26, 2023

Uh oh!

parasharrajat commented Apr 27, 2023

Uh oh!

staszekscp commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

staszekscp commented Apr 25, 2023 •

edited

Loading

mountiny commented Apr 26, 2023 •

edited

Loading

staszekscp commented Apr 27, 2023 •

edited

Loading