-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LHN list re-render #36927
LHN list re-render #36927
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments, please answer them 🎉
return ( | ||
<FreezeWrapper keepVisible={!isSmallScreenWidth}> | ||
<FreezeWrapper> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove this? Is the sidebar still clickable if you e.g. navigate to different reports twice? I think it was handling a case where you have the sidebar visible all the time and navigate deeper in the main stack, but maybe the navigation structure changed in a way that it is not necessary right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I tested this fix locally and the list is still fully clickable on the web, and causes no issues on mobile. It seems plausible that the nav structure changed and this is no loner a case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keepVisible
completely disables freezing for a 'big' screen. I believe the original idea of this chunk was to freeze it directly in such cases. I wonder why we used this approach before. It might be a leftover from the previous navigation structure
@@ -28,8 +27,8 @@ function FreezeWrapper({keepVisible = false, children}: FreezeWrapperProps) { | |||
// if the screen is more than 1 screen away from the current screen, freeze it, | |||
// we don't want to freeze the screen if it's the previous screen because the freeze placeholder | |||
// would be visible at the beginning of the back animation then | |||
if (navigation.getState().index - (screenIndexRef.current ?? 0) > 1) { | |||
InteractionManager.runAfterInteractions(() => setIsScreenBlurred(true)); | |||
if (navigation.getState().index - (screenIndexRef.current ?? 0) >= 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the issue with beginning of animation not occur anymore? And did you try to run it in the new arch? I think the case with InteractionManager
was needed there only and I think we will want to enable it on web ASAP too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't pinpoint the any issues with the animations. I haven't tested with new arch though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Animation of goBack works ok for me. I haven't tried it with a new arch yet, but it shouldn't be an issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I also gave it a spin and wasn't able to experience anything weird related to animations.
@perunt, is the purpose of this PR to ensure that the hidden screen freezes when there are two or more screens in the stack navigator? |
I would say the purpose was to freeze the hidden screen once we have two or more screens in the stack. One reliable method to reproduce this issue is to log out and then log in again. It occurs when the stack has two screens. In all other cases, it works as intended. |
okay thank you for the confirmation @situchan please review the PR when you have time 🙇 |
Hey @perunt 👋 Great work ! I was testing out your PR alongside with this PR and It works great. Only thing I want to flag is these two PR appears to go together. For example, only with your PR we still see the slowness between switch but there's no such hang observed. Now, to improve the slowness between switch, the linked PR is important. I just wanted to flag the above and also below is how these two PRs look together. Beforebefore.mp4Afterafter.mp4 |
hey @hurali97 If not, have you had a chance to test the switching on the main branch, where the issue with multiple renderItem calls doesn't seem to reproduce? Is the performance still an issue in that scenario? The key concern here is that on the first switch, we encounter a page that completely freezes for 15-20 seconds, which we need to avoid. If there's an alternative solution, I'd be eager to explore it. The difference might be due to |
I am referring to both the initial switch and the subsequent switches. If you notice in the After video, both the initial and subsequent switches are quick which happens because of the PR I linked. What I am trying to say is your PR for sure fixes the multiple renderItem calls but we still have some slowness in screen rendering after the switch. The switch happens quickly but not the rendering. The PR I linked fixes that and now with both of these PRs we have quick switching, no extra renderItem calls and quick rendering as well.
Below you can see how main behaves with your PR changes only. We have no extra renderItem calls and quick switching but delayed rendering. Mainmain_.mp4Both PRs combinedafter.mp4 |
@hurali97 at least this PR won't cause another performance regression right?
this is already existing bug on main right? |
From an overview, doesn't look like there will be a regression but we need to test here because in this PR we have some changes to the Freeze wrapper.
That's correct and this is what's addressed in this PR |
Ahh, now I understand—I was concerned my PR might be causing some slowness. Great work on your part! The page loading appears much faster now. I'm confident users will definitely notice the improvemen |
Indeed, this PR should not lead to any further performance regression. At least, I haven't observed any such issues so far. |
@situchan, can you please take a look at this? |
yes reviewing today |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / Safariweb.movMacOS: Desktop |
@perunt can you please add platform screenshots? |
And there's conflict |
it was a web issue, i have screen records for it in |
…into perunt/perf-lhn-list
This also happens on main so out of scope. But is this bug already reported or being fixed elsewhere? After switching to wrench tab and back, not found page shows Screen.Recording.2024-03-08.at.7.49.04.PM.mov |
if ((navigation.getState()?.index ?? 0) - (screenIndexRef.current ?? 0) > 1) { | ||
InteractionManager.runAfterInteractions(() => setIsScreenBlurred(true)); | ||
|
||
if ((navigation.getState()?.index ?? 0) - (screenIndexRef.current ?? 0) >= 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you changed to >= 1
from > 1
. Isn't this opposed to this comment?
// we don't want to freeze the screen if it's the previous screen because the freeze placeholder
// would be visible at the beginning of the back animation then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that we no longer use the placeholder component. I guess this comment was not changed after the placeholder was removed. I will do it now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@perunt this actually caused regression.
As this affects all platforms, please test and add remaining platforms, not only web.
this branch:
Screen.Recording.2024-03-11.at.10.54.20.PM.mov
main:
Screen.Recording.2024-03-11.at.10.54.42.PM.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@situchan, thanks for pointing it out.
I retested it, and it looks like a freshly added nullish coalescing operator can let me remove this check, so I assume we can't receive navigation immediately in some cases. I'm adding the rest of the recordings.
It doesn't make sense to link #35602 in |
switched to the correct one #36420 (comment) @situchan, can I somehow speed up merging this PR? |
I am just waiting for #36927 (comment) from @getusha to confirm that it's safe. |
There's performance lag a bit when opening Search page on both main and this branch. I don't find any difference. Screen.Recording.2024-03-11.at.10.36.53.PM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks good to me
are there blocking issues or conversations to be resolved before we merge this PR?
I don't know about such issues, so it's clean from my side |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@perunt please check the regression described in this comment.
@hayata-suenaga I'm resolving this, thanks! |
@situchan could you retest when you have time? 🙇 |
testing |
The code is now simplified and affects only web platform. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you so much for your work @perunt 😄
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/hayata-suenaga in version: 1.4.52-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.52-6 🚀
|
here I'm trying to address the bug described: #36420 (comment)
discussion: https://expensify.slack.com/archives/C05LX9D6E07/p1708011570713419
Details
Fixed Issues
$ #36420 (comment)
PROPOSAL:
Tests
Offline tests
QA Steps
NOTE: This issue occurs only once when you navigate to this screen for the first time.
Alternate method to reproduce the issue:
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-03-12.at.08.31.32.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-03-12.at.08.33.24.mp4
MacOS: Chrome / Safari
Untitled.mov
MacOS: Desktop
Untitled.mov