Optimize the left-hand-nav to only render options when they need to be visible #10800
Conversation
|
Ugh 🤦 I just realized I didn't push my changes since yesterday. I'm so sorry about that guys |
|
I was like... I know I just fixed those things! |
src/libs/SidebarUtils.js
Outdated
| // We make exceptions for defaultRooms and policyExpenseChats so we can immediately | ||
| // highlight them in the LHN when they are created and have no messsages yet. We do | ||
| // not give archived rooms this exception since they do not need to be higlihted. | ||
| && !(!ReportUtils.isArchivedRoom(report) && (isDefaultRoom || isPolicyExpenseChat)); |
There was a problem hiding this comment.
NAB: I added another exception for workspace rooms that failed to add in my refactor PR #10863. If you merge before I do I can update it, otherwise please update it.
App/src/libs/OptionsListUtils.js
Lines 491 to 500 in 6066771
There was a problem hiding this comment.
OK, I've added this.
I think at some point, I'd really like to simplify all these exceptions. Maybe at least add them to something like Report.shouldFilterInLHN()
|
Ah, I see. That makes more sense. It's worth looking into
…On Tue, Sep 20, 2022 at 1:56 PM Marc Glasser ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/libs/OptionsListUtilsLHN.js
<#10800 (comment)>:
> +import _ from 'underscore';
+import Str from 'expensify-common/lib/str';
+import ONYXKEYS from '../ONYXKEYS';
+import * as ReportUtils from './ReportUtils';
+import * as Localize from './Localize';
+import CONST from '../CONST';
+import * as OptionsListUtils from './OptionsListUtils';
+import * as CollectionUtils from './CollectionUtils';
+import Permissions from './Permissions';
+
+let reports;
+Onyx.connect({
+ key: ONYXKEYS.COLLECTION.REPORT,
+ waitForCollectionCallback: true,
+ callback: val => reports = val,
+});
Similar but the key differences is that we were calculating all of the
options before regardless of whether they were rendered.
Your changes make it so that we only calculate a few options.
So, just imagine that in the example I'm sharing all the data we are
subscribing to via Onyx.connect() in SidebarUtils is just passed to the
OptionsList as a prop and it can use them to create the 10 options that are
rendered instead of 4000.
—
Reply to this email directly, view it on GitHub
<#10800 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJMAB7NDAVMK4AX7G2EWMTV7IJG3ANCNFSM6AAAAAAQDMH4WQ>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
I believe the link here links to this PR, was it supposed to link to another? |
|
@yuwenmemon I deleted that comment :D I meant to add it on a GH issue instead of this PR :D |
|
All righty, I went through the code again this morning and it looks like all comments have been resolved (and my changes were all pushed). A final look again today and a merge would be appreciated! |
| import * as CollectionUtils from './CollectionUtils'; | ||
| import Permissions from './Permissions'; | ||
|
|
||
| // Note: It is very important that the keys subscribed to here are the same |
There was a problem hiding this comment.
NAB: Use a block comment here? Also the line lengths are a bit.. inconsistent 😅
| return false; | ||
| } | ||
|
|
||
| // We let Free Plan default rooms to be shown in the App - it's the one exception to the beta, otherwise do not show policy rooms in product |
There was a problem hiding this comment.
| // We let Free Plan default rooms to be shown in the App - it's the one exception to the beta, otherwise do not show policy rooms in product | |
| // We let Free Plan default rooms be shown in the App - it's the one exception to the beta, otherwise do not show policy rooms in product |
|
@marcaaron looks like this was merged without passing tests. Please add a note explaining why this was done and remove the |
|
Checklist was the only thing not passing. But it was fully checked. I think we are looking into improving this 🤷♂️ |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Thanks all! Onto the cleanup issues! |
|
🚀 Deployed to staging by @marcaaron in version: 1.2.4-0 🚀
|
|
🚀 Deployed to production by @luacmartins in version: 1.2.4-2 🚀
|
|
Leaving a note that this has caused a small visual issue in #14148 On this line we're using |
| ? [styles.optionDisplayName, ...textUnreadStyle, styles.optionDisplayNameCompact, styles.mr2] | ||
| : [styles.optionDisplayName, ...textUnreadStyle], props.style); | ||
| const alternateTextStyle = StyleUtils.combineStyles(props.viewMode === CONST.OPTION_MODE.COMPACT | ||
| ? [textStyle, styles.optionAlternateText, styles.textLabelSupporting, styles.optionAlternateTextCompact] |
There was a problem hiding this comment.
Turns out that on compact (focus) mode, because of the styles.optionAlternateText we had this emoji alignment issue #41367 on iOS: Native only.
Details
This PR builds off of previous work to make the sidebar re-rendering more performant. This was tested on an account with ~4000 reports.
The idea behind it is:
createOption()n times (n = number of reports) when the sidebar re-renders, the re-rendering of the sidebar only is concerned with the order of reportIDs being correct. The reportID is then passed as the data to the FlatList items, and then what used to happen increateOption()now happens when each FlatListItem is rendered (and only a few of them are rendered at once). This makes the re-rendering super performant for the sidebar. Here is a way of seeing the impact.Data processing
This is the timing of how long it took to prepare all the data necessary to render the FlatList. These profiles were taken while doing a single cold report switch.
Before:
You can see the component re-renders many times. The time it takes to get the data is ~250ms, and sometimes it's memoized to be done in 0ms.
After
The component re-renders fewer times. The time it takes to get the data is ~7ms, and no memoization is done anywhere.
Rendering time
This is what the performance profiles look like while switching to a cold report.
Before:
Total time: 1346ms
After:
Total time: 742ms
What's more is that during that time, the UI thread is released multiple times, which allows other things to run as they need to (like animations) which will help everything feel snappier.
Fixed Issues
$ GH_LINK
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Screenshots
Web
Mobile Web
Desktop
iOS
Android