Skip to content
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.

Sign up for GitHub

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

[Audit][Implementation] - Memoize SidebarLinksData #37205

Merged
merged 10 commits into from
Mar 26, 2024
Merged

[Audit][Implementation] - Memoize SidebarLinksData #37205

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d4b7075
feat: memoize SidebarLinksData
rinej Feb 26, 2024
88c5fd8
add missing props to memo
rinej Feb 27, 2024
3aa2176
chore: add comments for memoization
rinej Feb 27, 2024
287b6ca
Merge branch 'main' into Audit-Implementation-memoize-SidebarLinksData
rinej Mar 11, 2024
f726b6b
chore: run prettier
rinej Mar 11, 2024
13c721e
Merge branch 'main' into Audit-Implementation-memoize-SidebarLinksData
rinej Mar 21, 2024
0f93e35
feat: adjust to recent main
rinej Mar 21, 2024
d787f1b
chore: lint fix
rinej Mar 21, 2024
9f2ee85
chore: update comment
rinej Mar 21, 2024
2b62829
chore: update comment
rinej Mar 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 33 additions & 14 deletions src/pages/home/sidebar/SidebarLinksData.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {deepEqual} from 'fast-equals';
import lodashGet from 'lodash/get';
import lodashMap from 'lodash/map';
import PropTypes from 'prop-types';
import React, {useCallback, useEffect, useMemo, useRef} from 'react';
import React, {memo, useCallback, useEffect, useMemo, useRef} from 'react';
import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
Expand Down Expand Up @@ -136,18 +136,14 @@ function SidebarLinksData({

const reportIDsRef = useRef(null);
const isLoading = isLoadingApp;

const optionItemsMemoized = useMemo(
() => SidebarUtils.getOrderedReportIDs(null, chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs),
[chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs],
);

const optionListItems = useMemo(() => {
const reportIDs = SidebarUtils.getOrderedReportIDs(
null,
chatReports,
betas,
policies,
priorityMode,
allReportActions,
transactionViolations,
activeWorkspaceID,
policyMemberAccountIDs,
);
const reportIDs = optionItemsMemoized;

if (deepEqual(reportIDsRef.current, reportIDs)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this additional layer still valid?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have a comment just below which explains about updating the reportIDsRef. But I think it's not adding any value 🤔 We might try to remove this layer altogether and see if it's not producing any weird output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the comment for this layer:

// 1. We need to update existing reports only once while loading because they are updated several times during loading and causes this regression: https://github.com/Expensify/App/issues/24596#issuecomment-1681679531
// 2. If the user is offline, we need to update the reports unconditionally, since the loading of report data might be stuck in this case.
// 3. Changing priority mode to Most Recent will call OpenApp. If there is an existing reports and the priority mode is updated, we want to immediately update the list instead of waiting the OpenApp request to complete

It is fixing some regression or bug form the past, I didn't want to mess with it and kept it as it was

return reportIDsRef.current;
Expand All @@ -160,7 +156,7 @@ function SidebarLinksData({
reportIDsRef.current = reportIDs;
}
return reportIDsRef.current || [];
}, [chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs, isLoading, network.isOffline, prevPriorityMode]);
}, [optionItemsMemoized, priorityMode, isLoading, network.isOffline, prevPriorityMode]);

// We need to make sure the current report is in the list of reports, but we do not want
// to have to re-generate the list every time the currentReportID changes. To do that
Expand Down Expand Up @@ -334,4 +330,27 @@ export default compose(
initialValue: {},
},
}),
)(SidebarLinksData);
)(
/*
While working on audit on the App Start App metric we noticed that by memoizing SidebarLinksData we can avoid 2 additional run of getOrderedReportIDs.
With that we can reduce app start up time by ~2s on heavy account.
More details - https://github.com/Expensify/App/issues/35234#issuecomment-1926914534
*/
memo(
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on how this looks like it'd be worth to comment it well in code - what these checks represent, why we need them so that it's easier to remove this once we solve the root cause later on.

SidebarLinksData,
(prevProps, nextProps) =>
_.isEqual(prevProps.chatReports, nextProps.chatReports) &&
_.isEqual(prevProps.allReportActions, nextProps.allReportActions) &&
prevProps.isLoadingApp === nextProps.isLoadingApp &&
prevProps.priorityMode === nextProps.priorityMode &&
_.isEqual(prevProps.betas, nextProps.betas) &&
_.isEqual(prevProps.policies, nextProps.policies) &&
prevProps.network.isOffline === nextProps.network.isOffline &&
_.isEqual(prevProps.insets, nextProps.insets) &&
prevProps.onLinkClick === nextProps.onLinkClick &&
_.isEqual(prevProps.policyMembers, nextProps.policyMembers) &&
_.isEqual(prevProps.transactionViolations, nextProps.transactionViolations) &&
_.isEqual(prevProps.currentUserPersonalDetails, nextProps.currentUserPersonalDetails) &&
prevProps.currentReportID === nextProps.currentReportID,
),
Comment on lines +341 to +355
Copy link
Contributor

Choose a reason for hiding this comment

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

How can we make sure there is no condition missing in the memo for the SidebarLinksData? Last time we added memo like this with @hurali97 there was bunch of props cases we missed and they caused regressions.

Did we add all the props here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added all the props that are coming to this component, also I noticed that when I missed some of them e.g. currentReportId or currentUserPersonalDetails the jest tests failed - it might be the indicator if we miss some props

);
Loading