-
Notifications
You must be signed in to change notification settings - Fork 3k
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
perf: make switch between chat list and workspaces smoother #36420
perf: make switch between chat list and workspaces smoother #36420
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'm pretty skeptical about introducing some of the patterns from this PR without broader discussion. For example, I believe useOrderedReportIDs
and useReports
could leverage the (coming-soon) useOnyx
hook: Expensify/react-native-onyx#462. Personally I'd rather wait for that hook to land before adding a useReports
hook that adds a new context and leverages Onyx.connect
internally.
Another piece of feedback is that this PR has a number of different changes and could be more effective if split up into a series of smaller, more focused PRs.
For context, that is the plan. The full PR was created so benchmarks/ measurements could be taken altogether as the individual changes might not have very noticeable change in their own.
Niiice, yeah I agree it would be great to use that hook. I was not aware we are getting so close with that implementation. Thanks! |
@roryabraham thank you for reminding us of the upcoming hook. Does the new hook |
It sounds like this PR allowed the investigation to get much closer to the root cause of the problem (the re-rendering). I think this is the best next step:
I also am not a fan of rolling out a change this big just so web can use flatlist for now. I think keeping the bug live gives us a good #urgency to get this problem solved. |
Thank you guys, really appreciate your input 👍 To summarise, we are moving forward with the following action items:
|
is the performance issue stemming from the fact that if the former is the case, I see how using the new
anyway, this seems to be the priority ⬆️ This task will be worked on by Adam from Software Mansion as part of this GH ticket |
@hurali97, is it reproducible on each switching to the settings page? |
After some intensive digging, here's the update from me: The issue with rendering the 15k list items when switching from LHN to settings was very hard to reproduce - I only managed to observe it few times over the last 2 days. That said, it's not really worth digging much deeper into as it's 99% related to It's a symptom, rather than a cause, namely because of LHN screen not being properly frozen and triggering rerenders on focus change, when the screen goes out of view. This is something we pointed out in the Report Screen Load Time analysis as part of the Performance Audit done by Callstack. The fix from @perunt looks great on web - removing the pre-rendering completely thus removing the possibility of the issue happening in the first place and allows us to retain Flashlist on the web. On mobile, changes in the FreezeWrapper don’t cause any issue whatsoever. With that potentially settled, the only agreed action point for this PR that remains is using the As for the second issue, where the app hangs during LHN load after chat switch, I wasn’t able to reproduce it even once. I suspect it might be the same case as above, where the list wasn’t frozen and in rare cases rendered 15k items. Let me know whether I should cleanup this PR & revert to using Flashlist. |
@jbroma thank you so much for providing the update. So the remaining optimization task is to use I love the work you put into this PR, but do we need to keep this PR? Are fixes addressed in other PRs enough to solve the major causes of the performance issues? |
@hayata-suenaga not sure about abandoning this PR yet, I'll summarise the things done in this PR in more detail so they can be picked into separate small PRs. I'll post that in a separate comment. This PR could stay and contain just the changes with the hooks and contexts. Let me know what you think about that. The hanging issue looks closely related to the freeze mechanism and |
Done in this PR:
Changes made obsolete by freeze fix
|
@jbroma thank you for much. Because there are so many conversations going on about this performance issue, your summary of this PR is really helpful ❤️ I have some questions.
I think @mountiny expressed concerns about this part that this might introduce a new pattern of creating a context for each Onyx key. I know we're just doing this for I think we're also discussing about using
Could you remind me why we're creating a context for I think we can definitely keep the following items:
And if I remember this correctly, we already merged a PR for the following item, right...?
|
@hayata-suenaga first of all, I totally agree with the concerns about this pattern and it is indeed not a preferred a way of doing things. When it comes to When it comes to Cache removal from |
@jbroma thank you so much for the explanation. Reading closely your explanation about the need for creating a context for The only question remaining is about your statement about Anyway, this question is not a blocker. Once you'r ready, could you remove the following parts from this PR?
And we can bench mark performance improvement against this PR. Do you think that is a good plan? |
@hayata-suenaga I'll resume work on this PR on Monday. |
@jbroma I am ooo for 2 weeks, but it would be great to first post a Proposal with Problem / Solution format if we want to add a context for onyx key, that is quite a substantial shift in approach and such change would need to be highlighted more publicly to get agreement on this solution/ path forwards being the best at this point for the given problem. If you do not want to create context here and apply more subtle improvements then its fine to just implement it |
@hayata-suenaga The reason we extracted the calculation for @jbroma Great work continuing this PR ❤️ . I will stay active, in-case you need anything from me/ any comments. I have also tested this PR and the one here which improves the freeze wrapper and these two PRs work great together. See here. |
@jbroma thank you for taking over this issue from Muhammad if you think we still need a context for the I'm still having trouble understanding the need for |
@hayata-suenaga @roryabraham PR is ready to be re-reviewed after the usage of |
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.
Other than this comment, the PR looks good to me. Thank you for pushing this PR forward!
@situchan is OOO and @ntdiary was unassigned from the issue because of the lack of the bandwidth. |
Reviewing this as per the message |
Just finished with the code review, this is the last pending comment. I am going to start with the checklist as this is a small comment to fix. |
When I tried testing this on Web, I see chat not found error. Screen.Recording.2024-04-25.at.3.04.44.AM.mov |
…y-App into hur/perf/issue-35704
@mananjadhav I couldn't repro it on the local. As far as I recall, I have faced the similar issue on main as well but that happens randomly. repro.mp4 |
Let me take a look at this again. |
I was still able to reproduce this consistently on one account on Web and Desktop. But after I signout and signin again, it works fine. I am not sure if this is an issue, because I can't reproduce it on other accounts that were already logged in. Hence, I am going to continue with the checklist. web-chat-switcher-bug.movdesktop-chat-switcher-bug.mov |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb Chromemweb-chrome-switcher.moviOS: Nativeios-chat-switcher.moviOS: mWeb Safarimweb-safari-chat-switcher.movMacOS: Chrome / Safariweb-chat-switcher-bug.movMacOS: Desktopdesktop-chat-switcher.mov |
✋ 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/roryabraham in version: 1.4.67-0 🚀
|
Hey all, there are a lot of changes here so I can't be sure, but it seems possible that this new issue: 'App redirects to Concierge chat when leaving group chat and room' might have been introduced by these changes. Would you mind taking a look at the issue please |
@Julesssss I will try look into this 👍 |
Hey @hurali97, thanks but no need. I just confirmed the issue doesn't occuron the commit where this PR was merged to main 👍 |
@Julesssss Thanks for letting me know 👍 |
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.67-7 🚀
|
1 similar comment
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.67-7 🚀
|
Details
This PR only makes the switch between Chat lists and Settings->Workspaces smoother. Fix of the hang behaviour which happens sometimes when switching, is fixed by #36927.
Fixed Issues
$ #35704
PROPOSAL: #35704 (comment)
Tests
Log in on Web
Switch between Chat list and Settings->Workspaces
Switch should be smooth
Verify that no errors appear in the JS console
Offline tests
QA Steps
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.mov
Android: mWeb Chrome
android-web.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov