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.

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

[Fix #21022] Improve performance by reducing re-renders SidebarLinks #21406

Merged
merged 62 commits into from
Jul 19, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
82974c6
Connect OptionRowLHN manually
hannojg Jun 23, 2023
eada423
don't rerender SidebarLinks just because modal opens
hannojg Jun 23, 2023
c31f7de
Remove currentReportId from shouldReportBeInOptionList
hannojg Jun 23, 2023
32c2b70
add active report functionality back
hannojg Jun 23, 2023
6a38ee8
optimize optionRowLHN to not depend on current report id
hannojg Jun 23, 2023
a651118
fix
hannojg Jun 26, 2023
c8ea565
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Jun 28, 2023
99c8a1d
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Jul 6, 2023
27f8eb0
fix issues after merge
hannojg Jul 6, 2023
f3a39b6
fix import issues
hannojg Jul 6, 2023
99d2d40
change back name
hannojg Jul 6, 2023
4bb3a19
remove currentReportId from params
hannojg Jul 6, 2023
8bfad23
wip: data wrapper component
hannojg Jul 7, 2023
0bdd608
logging
hannojg Jul 7, 2023
4d9bb25
wip: fixing tests
hannojg Jul 7, 2023
640753c
add currentReportID back
hannojg Jul 8, 2023
a84e26f
wip(test): simpler context usage
hannojg Jul 8, 2023
b7cb24d
wip: tests
hannojg Jul 8, 2023
cdb46d6
wip: tests
hannojg Jul 8, 2023
69e46ae
include lastReadTime in wrapper component as its needed to compute GS…
hannojg Jul 8, 2023
4128c82
fiz style in focus mode
hannojg Jul 8, 2023
8a1a5c2
fix SidebarFilterTests
hannojg Jul 8, 2023
f0d54a7
wip: fixed all tests temporarily
hannojg Jul 8, 2023
7c5521a
removed focused index as its obsolete
hannojg Jul 8, 2023
8e4f2c2
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Jul 10, 2023
657fad8
removed logging statements
hannojg Jul 10, 2023
bd086ab
split up components + use deepEqual in react memo
hannojg Jul 10, 2023
6c296ad
fix prop types
hannojg Jul 10, 2023
8625724
cleaned up tests
hannojg Jul 10, 2023
db85064
fix prop types
hannojg Jul 10, 2023
0afc57e
fixed last onyx data connections
hannojg Jul 10, 2023
1a88a51
clean
hannojg Jul 10, 2023
b9c6c0f
wip: moving comment logic over to data component
hannojg Jul 10, 2023
f6d7bd0
fix option row not upating when personal details changing
hannojg Jul 10, 2023
3b5bd35
Update src/libs/SidebarUtils.js
hannojg Jul 11, 2023
4a8f494
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Jul 14, 2023
c53cf6d
fix issues after merge
hannojg Jul 14, 2023
00a4b5c
dont use this
hannojg Jul 14, 2023
26e661a
removed unused imports
hannojg Jul 14, 2023
448878d
fix comment
hannojg Jul 14, 2023
7bc90eb
commented tests
hannojg Jul 14, 2023
336d5fd
inline skeleton
hannojg Jul 14, 2023
8180e87
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Jul 17, 2023
5c35629
destructure props
hannojg Jul 17, 2023
a73fa9c
fix highlight active chat
hannojg Jul 17, 2023
c4fed2b
add displayName and iouReportAmount for sorting
hannojg Jul 17, 2023
2a73540
include total, remove displayName
hannojg Jul 17, 2023
de7209f
added other properties considered for sorting
hannojg Jul 17, 2023
de5fe1d
use prop spreaing
hannojg Jul 17, 2023
ecf4c0a
move navigation focus to sidebarlinksdata
hannojg Jul 17, 2023
b76a569
removed debug comment
hannojg Jul 17, 2023
c0fba28
remove withNavigation usage
hannojg Jul 17, 2023
31a1167
add other missing fields
hannojg Jul 17, 2023
f2f3c9d
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Jul 17, 2023
0bb1e62
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Jul 17, 2023
36c8913
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Jul 19, 2023
478288d
removed unused onLayout prop
hannojg Jul 19, 2023
3d60095
Memoize OptionRowLHNData as well
hannojg Jul 19, 2023
9f88371
fix lint
hannojg Jul 19, 2023
e25fd20
Update src/components/LHNOptionsList/OptionRowLHNData.js
hannojg Jul 19, 2023
17ed2a1
Update src/libs/ReportActionsUtils.js
hannojg Jul 19, 2023
9ba0811
change comment format
hannojg Jul 19, 2023
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
43 changes: 40 additions & 3 deletions src/components/LHNOptionsList/OptionRowLHN.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import _ from 'underscore';
import React, {useEffect} from 'react';
import PropTypes from 'prop-types';
import {View, StyleSheet} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import lodashGet from 'lodash/get';
import * as optionRowStyles from '../../styles/optionRowStyles';
import styles from '../../styles/styles';
Expand All @@ -27,6 +28,7 @@ import * as ContextMenuActions from '../../pages/home/report/ContextMenu/Context
import * as OptionsListUtils from '../../libs/OptionsListUtils';
import compose from '../../libs/compose';
import ONYXKEYS from '../../ONYXKEYS';
import withCurrentReportId from '../withCurrentReportId';
import * as Report from '../../libs/actions/Report';

const propTypes = {
Expand All @@ -51,6 +53,8 @@ const propTypes = {

style: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]),

optionItem: PropTypes.shape({}),

...withLocalizePropTypes,
};

Expand All @@ -60,11 +64,12 @@ const defaultProps = {
onSelectRow: () => {},
isFocused: false,
style: null,
optionItem: null,
comment: '',
};

function OptionRowLHN(props) {
const optionItem = SidebarUtils.getOptionData(props.reportID);
function BaseOptionRowLHN(props) {
const optionItem = props.optionItem;

useEffect(() => {
if (!optionItem || optionItem.hasDraftComment || !props.comment || props.comment.length <= 0 || props.isFocused) {
Expand Down Expand Up @@ -267,12 +272,44 @@ function OptionRowLHN(props) {
);
}

BaseOptionRowLHN.propTypes = propTypes;
BaseOptionRowLHN.defaultProps = defaultProps;
BaseOptionRowLHN.displayName = 'BaseOptionRowLHN';

const MemoedOptionRowLHN = React.memo(
compose(
withLocalize,
withOnyx({
optionItem: {
key: (props) => ONYXKEYS.COLLECTION.REPORT + props.reportID,
selector: SidebarUtils.getOptionData,
},
}),
)(BaseOptionRowLHN),
);

// We only want to pass a boolean to the memoized
// component, thats why we have this intermediate component.
// (We don't want to fully re-render all items, just because the active report changed).
function OptionRowLHN(props) {
const isFocused = props.currentReportId === props.reportID;

return (
<MemoedOptionRowLHN
// eslint-disable-next-line react/jsx-props-no-spreading
{..._.omit(props, 'currentReportId')}
isFocused={isFocused}
/>
);
}

OptionRowLHN.propTypes = propTypes;
OptionRowLHN.defaultProps = defaultProps;
OptionRowLHN.displayName = 'OptionRowLHN';
OptionRowLHN.displayName = 'OptionRowIsFocusedSupport';

export default compose(
withLocalize,
withCurrentReportId,
withReportCommentDrafts({
propName: 'comment',
transformValue: (drafts, props) => {
Expand Down
12 changes: 2 additions & 10 deletions src/libs/ReportUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -1891,14 +1891,13 @@ function canSeeDefaultRoom(report, policies, betas) {
* filter out the majority of reports before filtering out very specific minority of reports.
*
* @param {Object} report
* @param {String} currentReportId
* @param {Boolean} isInGSDMode
* @param {Object} iouReports
* @param {String[]} betas
* @param {Object} policies
* @returns {boolean}
*/
function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouReports, betas, policies) {
function shouldReportBeInOptionList(report, isInGSDMode, iouReports, betas, policies) {
const isInDefaultMode = !isInGSDMode;

// Exclude reports that have no data because there wouldn't be anything to show in the option item.
Expand All @@ -1916,13 +1915,6 @@ function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouRep
return false;
}

// Include the currently viewed report. If we excluded the currently viewed report, then there
hannojg marked this conversation as resolved.
Show resolved Hide resolved
// would be no way to highlight it in the options list and it would be confusing to users because they lose
// a sense of context.
if (report.reportID === currentReportId) {
return true;
}

// Include reports if they have a draft, are pinned, or have an outstanding IOU
// These are always relevant to the user no matter what view mode the user prefers
if (report.hasDraft || report.isPinned || hasOutstandingIOU(report, iouReports)) {
Expand All @@ -1945,7 +1937,7 @@ function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouRep
return true;
}

// Include policy expense chats if the user isn't in the policy expense chat beta
// Exclude policy expense chats if the user isn't in the policy expense chat beta
if (isPolicyExpenseChat(report) && !Permissions.canUsePolicyExpenseChat(betas)) {
return false;
}
Expand Down
12 changes: 4 additions & 8 deletions src/libs/SidebarUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,15 +117,14 @@ function setIsSidebarLoadedReady() {
}

/**
* @param {String} currentReportId
* @returns {String[]} An array of reportIDs sorted in the proper order
*/
function getOrderedReportIDs(currentReportId) {
function getOrderedReportIDs() {
const isInGSDMode = priorityMode === CONST.PRIORITY_MODE.GSD;
const isInDefaultMode = !isInGSDMode;

// Filter out all the reports that shouldn't be displayed
const reportsToDisplay = _.filter(allReports, (report) => ReportUtils.shouldReportBeInOptionList(report, currentReportId, isInGSDMode, allReports, betas, policies));
const reportsToDisplay = _.filter(allReports, (report) => ReportUtils.shouldReportBeInOptionList(report, isInGSDMode, allReports, betas, policies));
if (_.isEmpty(reportsToDisplay)) {
// Display Concierge chat report when there is no report to be displayed
const conciergeChatReport = _.find(allReports, ReportUtils.isConciergeChatReport);
Expand Down Expand Up @@ -213,13 +212,10 @@ function getOrderedReportIDs(currentReportId) {
/**
* Gets all the data necessary for rendering an OptionRowLHN component
*
* @param {String} reportID
* @param {Object} report
* @returns {Object}
*/
function getOptionData(reportID) {
const reportKey = `${ONYXKEYS.COLLECTION.REPORT}${reportID}`;
const report = allReports[reportKey];

function getOptionData(report) {
// When a user signs out, Onyx is cleared. Due to the lazy rendering with a virtual list, it's possible for
// this method to be called after the Onyx data has been cleared out. In that case, it's fine to do
// a null check here and return early.
Expand Down
12 changes: 12 additions & 0 deletions src/libs/onyxSubscribe.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import Onyx from 'react-native-onyx';

/**
* Connect to onyx data. Same params as Onyx.connect(), but returns a function to unsubscribe.
*
* @param {Object} mapping Same as for Onyx.connect()
* @return {function(): void} Unsubscribe callback
*/
export default (mapping) => {
const connectionId = Onyx.connect(mapping);
return () => Onyx.disconnect(connectionId);
};
24 changes: 14 additions & 10 deletions src/pages/home/sidebar/SidebarLinks.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import SidebarUtils from '../../../libs/SidebarUtils';
import reportPropTypes from '../../reportPropTypes';
import OfflineWithFeedback from '../../../components/OfflineWithFeedback';
import withNavigationFocus from '../../../components/withNavigationFocus';
import withCurrentReportId, {withCurrentReportIdPropTypes} from '../../../components/withCurrentReportId';
import withNavigation, {withNavigationPropTypes} from '../../../components/withNavigation';
import Header from '../../../components/Header';
import defaultTheme from '../../../styles/themes/default';
Expand All @@ -40,6 +39,7 @@ import * as Session from '../../../libs/actions/Session';
import Button from '../../../components/Button';
import * as UserUtils from '../../../libs/UserUtils';
import KeyboardShortcut from '../../../libs/KeyboardShortcut';
import onyxSubscribe from '../../../libs/onyxSubscribe';

const propTypes = {
/** Toggles the navigation menu open and closed */
Expand Down Expand Up @@ -88,7 +88,6 @@ const propTypes = {
}),

...withLocalizePropTypes,
...withCurrentReportIdPropTypes,
...withNavigationPropTypes,
};

Expand Down Expand Up @@ -121,11 +120,19 @@ class SidebarLinks extends React.Component {
SidebarUtils.setIsSidebarLoadedReady();
this.isSidebarLoaded = true;

let modal = {};
this.unsubscribeOnyxModal = onyxSubscribe({
key: ONYXKEYS.MODAL,
callback: (modalArg) => {
modal = modalArg;
},
});
Comment on lines +89 to +95
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we don't use withOnyx ? this looks like an anti-pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of the performance improvements. The modal state is needed for a logic, that has nothing to do with the render/UI output. Thus, the component shouldn't re-render (because it's a really expensive to render one) just because we open a modal.

Long term I want to remove the modal state from onyx and move it e.g. to a react context, as I think it doesn't belong in the onyx store.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for the react context approach.


hannojg marked this conversation as resolved.
Show resolved Hide resolved
const shortcutConfig = CONST.KEYBOARD_SHORTCUTS.ESCAPE;
this.unsubscribeEscapeKey = KeyboardShortcut.subscribe(
shortcutConfig.shortcutKey,
() => {
if (this.props.modal.willAlertModalBecomeVisible) {
if (modal.willAlertModalBecomeVisible) {
return;
}

Expand All @@ -143,6 +150,9 @@ class SidebarLinks extends React.Component {
if (this.unsubscribeEscapeKey) {
this.unsubscribeEscapeKey();
}
if (this.unsubscribeOnyxModal) {
this.unsubscribeOnyxModal();
}
}

showSearchPage() {
Expand Down Expand Up @@ -180,7 +190,7 @@ class SidebarLinks extends React.Component {

render() {
const isLoading = _.isEmpty(this.props.personalDetails) || _.isEmpty(this.props.chatReports);
const optionListItems = SidebarUtils.getOrderedReportIDs(this.props.currentReportId);
const optionListItems = SidebarUtils.getOrderedReportIDs();

const skeletonPlaceholder = <OptionsListSkeletonView shouldAnimate />;

Expand Down Expand Up @@ -274,8 +284,6 @@ const chatReportSelector = (report) =>
errorFields: {
addWorkspaceRoom: report.errorFields && report.errorFields.addWorkspaceRoom,
},
lastReadTime: report.lastReadTime,
lastMentionedTime: report.lastMentionedTime,
hannojg marked this conversation as resolved.
Show resolved Hide resolved
lastMessageText: report.lastMessageText,
lastVisibleActionCreated: report.lastVisibleActionCreated,
iouReportID: report.iouReportID,
Expand Down Expand Up @@ -335,7 +343,6 @@ export default compose(
withCurrentUserPersonalDetails,
withNavigationFocus,
withWindowDimensions,
withCurrentReportId,
withNavigation,
withOnyx({
// Note: It is very important that the keys subscribed to here are the same
Expand Down Expand Up @@ -368,8 +375,5 @@ export default compose(
preferredLocale: {
key: ONYXKEYS.NVP_PREFERRED_LOCALE,
},
modal: {
key: ONYXKEYS.MODAL,
},
}),
)(SidebarLinks);