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 35 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
13 changes: 4 additions & 9 deletions src/components/LHNOptionsList/LHNOptionsList.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import React, {Component} from 'react';
import {View, FlatList} from 'react-native';
import PropTypes from 'prop-types';
import styles from '../../styles/styles';
import OptionRowLHN from './OptionRowLHN';
import OptionRowLHNData from './OptionRowLHNData';
import variables from '../../styles/variables';
import CONST from '../../CONST';

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 Down Expand Up @@ -65,16 +62,15 @@ class LHNOptionsList extends Component {
*
* @param {Object} params
* @param {Object} params.item
* @param {Number} params.index
*
* @return {Component}
*/
renderItem({item, index}) {
renderItem({item}) {
return (
<OptionRowLHN
<OptionRowLHNData
reportID={item}
viewMode={this.props.optionMode}
isFocused={!this.props.shouldDisableFocusOptions && this.props.focusedIndex === index}
shouldDisableFocusOptions={this.props.shouldDisableFocusOptions}
onSelectRow={this.props.onSelectRow}
/>
);
Expand All @@ -98,7 +94,6 @@ class LHNOptionsList extends Component {
stickySectionHeadersEnabled={false}
renderItem={this.renderItem}
getItemLayout={this.getItemLayout}
extraData={this.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 @@ -161,7 +148,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 @@ -189,7 +176,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 @@ -204,7 +191,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 @@ -238,15 +225,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 @@ -263,13 +250,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};
132 changes: 132 additions & 0 deletions src/components/LHNOptionsList/OptionRowLHNData.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
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(props) {
hannojg marked this conversation as resolved.
Show resolved Hide resolved
// We only want to pass a boolean to the memoized component,
// instead of a changing number (so we prevent unnecessary re-renders).
const isFocused = !props.shouldDisableFocusOptions && props.currentReportId === props.reportID;

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

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

return (
<OptionRowLHN
// eslint-disable-next-line react/jsx-props-no-spreading
{..._.omit(props, 'currentReportID', 'comment', 'personalDetails', 'preferredLocale', 'fullReport')}
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;
},
{},
);

export default 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) {
hannojg marked this conversation as resolved.
Show resolved Hide resolved
if (!report || !report.parentReportID || !report.parentReportActionID) {
return {};
}
return lodashGet(allReportActions, [report.parentReportID, report.parentReportActionID], {});
return lodashGet(allReportActionsParam || allReportActions, [report.parentReportID, report.parentReportActionID], {});
}

/**
Expand Down
12 changes: 7 additions & 5 deletions src/libs/ReportUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -2017,10 +2017,11 @@ function canSeeDefaultRoom(report, policies, betas) {
* @param {Object} report
* @param {Array<Object>} policies
* @param {Array<String>} betas
* @param {Object} allReportActions
* @returns {Boolean}
*/
function canAccessReport(report, policies, betas) {
if (isThread(report) && ReportActionsUtils.isPendingRemove(ReportActionsUtils.getParentReportAction(report))) {
function canAccessReport(report, policies, betas, allReportActions) {
if (isThread(report) && ReportActionsUtils.isPendingRemove(ReportActionsUtils.getParentReportAction(report, allReportActions))) {
return false;
}

Expand All @@ -2045,9 +2046,10 @@ function canAccessReport(report, policies, betas) {
* @param {Object} iouReports
* @param {String[]} betas
* @param {Object} policies
* @param {Object} allReportActions
* @returns {boolean}
*/
function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouReports, betas, policies) {
function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouReports, betas, policies, allReportActions) {
const isInDefaultMode = !isInGSDMode;

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

if (!canAccessReport(report, policies, betas)) {
if (!canAccessReport(report, policies, betas, allReportActions)) {
return false;
}

Expand Down Expand Up @@ -2094,7 +2096,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
Loading