Skip to content

Commit

Permalink
Merge pull request #21406 from margelo/perf/21022-sidebarlinks-perfor…
Browse files Browse the repository at this point in the history
…mance

[Fix #21022] Improve performance by reducing re-renders SidebarLinks
  • Loading branch information
mountiny authored Jul 19, 2023
2 parents 56e69e6 + 9ba0811 commit 18841da
Show file tree
Hide file tree
Showing 15 changed files with 500 additions and 319 deletions.
27 changes: 10 additions & 17 deletions src/components/LHNOptionsList/LHNOptionsList.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import PropTypes from 'prop-types';
import React, {useMemo} from 'react';
import React from 'react';
import {FlatList, View} from 'react-native';
import _ from 'underscore';
import CONST from '../../CONST';
import styles from '../../styles/styles';
import OptionRowLHNData from './OptionRowLHNData';
import variables from '../../styles/variables';
import OptionRowLHN from './OptionRowLHN';

const propTypes = {
/** Extra styles for the section list container */
Expand All @@ -15,9 +15,6 @@ const propTypes = {
/** Sections for the section list */
data: PropTypes.arrayOf(PropTypes.string).isRequired,

/** Index for option to focus on */
focusedIndex: PropTypes.number.isRequired,

/** Callback to fire when a row is selected */
onSelectRow: PropTypes.func.isRequired,

Expand All @@ -32,9 +29,7 @@ const defaultProps = {
shouldDisableFocusOptions: false,
};

function LHNOptionsList(props) {
const data = useMemo(() => props.data, [props.data]);

function LHNOptionsList({contentContainerStyles, data, onSelectRow, optionMode, shouldDisableFocusOptions}) {
/**
* This function is used to compute the layout of any given item in our list. Since we know that each item will have the exact same height, this is a performance optimization
* so that the heights can be determined before the options are rendered. Otherwise, the heights are determined when each option is rendering and it causes a lot of overhead on large
Expand All @@ -46,7 +41,7 @@ function LHNOptionsList(props) {
* @returns {Object}
*/
const getItemLayout = (itemData, index) => {
const optionHeight = props.optionMode === CONST.OPTION_MODE.COMPACT ? variables.optionRowHeightCompact : variables.optionRowHeight;
const optionHeight = optionMode === CONST.OPTION_MODE.COMPACT ? variables.optionRowHeightCompact : variables.optionRowHeight;
return {
length: optionHeight,
offset: index * optionHeight,
Expand All @@ -59,16 +54,15 @@ function LHNOptionsList(props) {
*
* @param {Object} params
* @param {Object} params.item
* @param {Number} params.index
*
* @return {Component}
*/
const renderItem = ({item, index}) => (
<OptionRowLHN
const renderItem = ({item}) => (
<OptionRowLHNData
reportID={item}
viewMode={props.optionMode}
isFocused={!props.shouldDisableFocusOptions && props.focusedIndex === index}
onSelectRow={props.onSelectRow}
viewMode={optionMode}
shouldDisableFocusOptions={shouldDisableFocusOptions}
onSelectRow={onSelectRow}
/>
);

Expand All @@ -77,14 +71,13 @@ function LHNOptionsList(props) {
<FlatList
indicatorStyle="white"
keyboardShouldPersistTaps="always"
contentContainerStyle={props.contentContainerStyles}
contentContainerStyle={contentContainerStyles}
showsVerticalScrollIndicator={false}
data={data}
keyExtractor={(item) => item}
stickySectionHeadersEnabled={false}
renderItem={renderItem}
getItemLayout={getItemLayout}
extraData={props.focusedIndex}
initialNumToRender={5}
maxToRenderPerBatch={5}
windowSize={5}
Expand Down
56 changes: 18 additions & 38 deletions src/components/LHNOptionsList/OptionRowLHN.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import _ from 'underscore';
import React, {useEffect, useState} from 'react';
import React, {useState} from 'react';
import PropTypes from 'prop-types';
import {View, StyleSheet} from 'react-native';
import lodashGet from 'lodash/get';
import * as optionRowStyles from '../../styles/optionRowStyles';
import styles from '../../styles/styles';
import * as StyleUtils from '../../styles/StyleUtils';
Expand All @@ -12,30 +11,22 @@ import MultipleAvatars from '../MultipleAvatars';
import Hoverable from '../Hoverable';
import DisplayNames from '../DisplayNames';
import colors from '../../styles/colors';
import withLocalize, {withLocalizePropTypes} from '../withLocalize';
import {withReportCommentDrafts} from '../OnyxProvider';
import Text from '../Text';
import SubscriptAvatar from '../SubscriptAvatar';
import CONST from '../../CONST';
import themeColors from '../../styles/themes/default';
import SidebarUtils from '../../libs/SidebarUtils';
import OfflineWithFeedback from '../OfflineWithFeedback';
import PressableWithSecondaryInteraction from '../PressableWithSecondaryInteraction';
import * as ReportActionContextMenu from '../../pages/home/report/ContextMenu/ReportActionContextMenu';
import * as ContextMenuActions from '../../pages/home/report/ContextMenu/ContextMenuActions';
import * as OptionsListUtils from '../../libs/OptionsListUtils';
import compose from '../../libs/compose';
import ONYXKEYS from '../../ONYXKEYS';
import * as Report from '../../libs/actions/Report';
import useLocalize from '../../hooks/useLocalize';

const propTypes = {
/** Style for hovered state */
// eslint-disable-next-line react/forbid-prop-types
hoverStyle: PropTypes.object,

/** The comment left by the user */
comment: PropTypes.string,

/** The ID of the report that the option is for */
reportID: PropTypes.string.isRequired,

Expand All @@ -50,29 +41,25 @@ const propTypes = {

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

...withLocalizePropTypes,
/** The item that should be rendered */
// eslint-disable-next-line react/forbid-prop-types
optionItem: PropTypes.object,
};

const defaultProps = {
hoverStyle: styles.sidebarLinkHover,
viewMode: 'default',
onSelectRow: () => {},
isFocused: false,
style: null,
comment: '',
optionItem: null,
isFocused: false,
};

function OptionRowLHN(props) {
const optionItem = SidebarUtils.getOptionData(props.reportID);
const [isContextMenuActive, setIsContextMenuActive] = useState(false);
const localize = useLocalize();

useEffect(() => {
if (!optionItem || optionItem.hasDraftComment || !props.comment || props.comment.length <= 0 || props.isFocused) {
return;
}
Report.setReportWithDraft(props.reportID, true);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
const optionItem = props.optionItem;
const [isContextMenuActive, setIsContextMenuActive] = useState(false);

if (!optionItem) {
return null;
Expand Down Expand Up @@ -169,7 +156,7 @@ function OptionRowLHN(props) {
(hovered || isContextMenuActive) && !props.isFocused ? props.hoverStyle : null,
]}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON}
accessibilityLabel={props.translate('accessibilityHints.navigatesToChat')}
accessibilityLabel={localize.translate('accessibilityHints.navigatesToChat')}
>
<View style={sidebarInnerRowStyle}>
<View style={[styles.flexRow, styles.alignItemsCenter]}>
Expand Down Expand Up @@ -197,7 +184,7 @@ function OptionRowLHN(props) {
<View style={contentContainerStyles}>
<View style={[styles.flexRow, styles.alignItemsCenter, styles.mw100, styles.overflowHidden]}>
<DisplayNames
accessibilityLabel={props.translate('accessibilityHints.chatUserDisplayNames')}
accessibilityLabel={localize.translate('accessibilityHints.chatUserDisplayNames')}
fullTitle={optionItem.text}
displayNamesWithTooltips={optionItem.displayNamesWithTooltips}
tooltipEnabled
Expand All @@ -212,7 +199,7 @@ function OptionRowLHN(props) {
<Text
style={alternateTextStyle}
numberOfLines={1}
accessibilityLabel={props.translate('accessibilityHints.lastChatMessagePreview')}
accessibilityLabel={localize.translate('accessibilityHints.lastChatMessagePreview')}
>
{optionItem.alternateText}
</Text>
Expand Down Expand Up @@ -246,15 +233,15 @@ function OptionRowLHN(props) {
{optionItem.hasDraftComment && (
<View
style={styles.ml2}
accessibilityLabel={props.translate('sidebarScreen.draftedMessage')}
accessibilityLabel={localize.translate('sidebarScreen.draftedMessage')}
>
<Icon src={Expensicons.Pencil} />
</View>
)}
{!shouldShowGreenDotIndicator && optionItem.isPinned && (
<View
style={styles.ml2}
accessibilityLabel={props.translate('sidebarScreen.chatPinned')}
accessibilityLabel={localize.translate('sidebarScreen.chatPinned')}
>
<Icon src={Expensicons.Pin} />
</View>
Expand All @@ -271,13 +258,6 @@ OptionRowLHN.propTypes = propTypes;
OptionRowLHN.defaultProps = defaultProps;
OptionRowLHN.displayName = 'OptionRowLHN';

export default compose(
withLocalize,
withReportCommentDrafts({
propName: 'comment',
transformValue: (drafts, props) => {
const draftKey = `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${props.reportID}`;
return lodashGet(drafts, draftKey, '');
},
}),
)(OptionRowLHN);
export default React.memo(OptionRowLHN);

export {propTypes, defaultProps};
142 changes: 142 additions & 0 deletions src/components/LHNOptionsList/OptionRowLHNData.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
import {withOnyx} from 'react-native-onyx';
import lodashGet from 'lodash/get';
import _ from 'underscore';
import PropTypes from 'prop-types';
import React, {useEffect, useRef, useMemo} from 'react';
import {deepEqual} from 'fast-equals';
import {withReportCommentDrafts} from '../OnyxProvider';
import SidebarUtils from '../../libs/SidebarUtils';
import compose from '../../libs/compose';
import ONYXKEYS from '../../ONYXKEYS';
import withCurrentReportID, {withCurrentReportIDPropTypes, withCurrentReportIDDefaultProps} from '../withCurrentReportID';
import OptionRowLHN, {propTypes as basePropTypes, defaultProps as baseDefaultProps} from './OptionRowLHN';
import * as Report from '../../libs/actions/Report';
import * as UserUtils from '../../libs/UserUtils';
import participantPropTypes from '../participantPropTypes';
import CONST from '../../CONST';

const propTypes = {
/** If true will disable ever setting the OptionRowLHN to focused */
shouldDisableFocusOptions: PropTypes.bool,

/** List of users' personal details */
personalDetails: PropTypes.objectOf(participantPropTypes),

/** The preferred language for the app */
preferredLocale: PropTypes.string,

/** The full data of the report */
// eslint-disable-next-line react/forbid-prop-types
fullReport: PropTypes.object,

...withCurrentReportIDPropTypes,
...basePropTypes,
};

const defaultProps = {
shouldDisableFocusOptions: false,
personalDetails: {},
fullReport: {},
preferredLocale: CONST.LOCALES.DEFAULT,
...withCurrentReportIDDefaultProps,
...baseDefaultProps,
};

/*
* This component gets the data from onyx for the actual
* OptionRowLHN component.
* The OptionRowLHN component is memoized, so it will only
* re-render if the data really changed.
*/
function OptionRowLHNData({shouldDisableFocusOptions, currentReportID, fullReport, personalDetails, preferredLocale, comment, ...propsToForward}) {
const reportID = propsToForward.reportID;
// We only want to pass a boolean to the memoized component,
// instead of a changing number (so we prevent unnecessary re-renders).
const isFocused = !shouldDisableFocusOptions && currentReportID === reportID;

const optionItemRef = useRef();
const optionItem = useMemo(() => {
// Note: ideally we'd have this as a dependent selector in onyx!
const item = SidebarUtils.getOptionData(fullReport, personalDetails, preferredLocale);
if (deepEqual(item, optionItemRef.current)) {
return optionItemRef.current;
}
optionItemRef.current = item;
return item;
}, [fullReport, preferredLocale, personalDetails]);

useEffect(() => {
if (!optionItem || optionItem.hasDraftComment || !comment || comment.length <= 0 || isFocused) {
return;
}
Report.setReportWithDraft(reportID, true);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

return (
<OptionRowLHN
// eslint-disable-next-line react/jsx-props-no-spreading
{...propsToForward}
isFocused={isFocused}
optionItem={optionItem}
/>
);
}

OptionRowLHNData.propTypes = propTypes;
OptionRowLHNData.defaultProps = defaultProps;
OptionRowLHNData.displayName = 'OptionRowLHNData';

/**
* @param {Object} [personalDetails]
* @returns {Object|undefined}
*/
const personalDetailsSelector = (personalDetails) =>
_.reduce(
personalDetails,
(finalPersonalDetails, personalData, accountID) => {
// It's OK to do param-reassignment in _.reduce() because we absolutely know the starting state of finalPersonalDetails
// eslint-disable-next-line no-param-reassign
finalPersonalDetails[accountID] = {
accountID: Number(accountID),
login: personalData.login,
displayName: personalData.displayName,
firstName: personalData.firstName,
avatar: UserUtils.getAvatar(personalData.avatar, personalData.accountID),
};
return finalPersonalDetails;
},
{},
);

/**
* This component is rendered in a list.
* On scroll we want to avoid that a item re-renders
* just because the list has to re-render when adding more items.
* Thats also why the React.memo is used on the outer component here, as we just
* use it to prevent re-renders from parent re-renders.
*/
export default React.memo(
compose(
withCurrentReportID,
withReportCommentDrafts({
propName: 'comment',
transformValue: (drafts, props) => {
const draftKey = `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${props.reportID}`;
return lodashGet(drafts, draftKey, '');
},
}),
withOnyx({
fullReport: {
key: (props) => ONYXKEYS.COLLECTION.REPORT + props.reportID,
},
personalDetails: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
selector: personalDetailsSelector,
},
preferredLocale: {
key: ONYXKEYS.NVP_PREFERRED_LOCALE,
},
}),
)(OptionRowLHNData),
);
5 changes: 3 additions & 2 deletions src/libs/ReportActionsUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,14 @@ function hasCommentThread(reportAction) {
* Returns the parentReportAction if the given report is a thread/task.
*
* @param {Object} report
* @param {Object} [allReportActionsParam]
* @returns {Object}
*/
function getParentReportAction(report) {
function getParentReportAction(report, allReportActionsParam = {}) {
if (!report || !report.parentReportID || !report.parentReportActionID) {
return {};
}
return lodashGet(allReportActions, [report.parentReportID, report.parentReportActionID], {});
return lodashGet(allReportActionsParam || allReportActions, [report.parentReportID, report.parentReportActionID], {});
}

/**
Expand Down
Loading

0 comments on commit 18841da

Please sign in to comment.