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

refactor: LHN dataflow rework #30242

Merged
merged 16 commits into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
160 changes: 140 additions & 20 deletions src/components/LHNOptionsList/LHNOptionsList.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,20 @@
import lodashGet from 'lodash/get';
import PropTypes from 'prop-types';
import React from 'react';
import React, {useCallback} from 'react';
import {FlatList, View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import participantPropTypes from '@components/participantPropTypes';
import withCurrentReportID, {withCurrentReportIDDefaultProps, withCurrentReportIDPropTypes} from '@components/withCurrentReportID';
import compose from '@libs/compose';
import * as OptionsListUtils from '@libs/OptionsListUtils';
import reportActionPropTypes from '@pages/home/report/reportActionPropTypes';
import reportPropTypes from '@pages/reportPropTypes';
import styles from '@styles/styles';
import variables from '@styles/variables';
import CONST from '@src/CONST';
import OptionRowLHNDataWithFocus from './OptionRowLHNDataWithFocus';
import ONYXKEYS from '@src/ONYXKEYS';
import OptionRowLHNData from './OptionRowLHNData';

const propTypes = {
/** Wrapper style for the section list */
Expand All @@ -27,14 +36,72 @@ const propTypes = {

/** Whether to allow option focus or not */
shouldDisableFocusOptions: PropTypes.bool,

/** The policy which the user has access to and which the report could be tied to */
policy: PropTypes.shape({
/** The ID of the policy */
id: PropTypes.string,
/** Name of the policy */
name: PropTypes.string,
/** Avatar of the policy */
avatar: PropTypes.string,
}),

/** All reports shared with the user */
reports: PropTypes.objectOf(reportPropTypes),

/** Array of report actions for this report */
reportActions: PropTypes.objectOf(PropTypes.shape(reportActionPropTypes)),

/** Indicates which locale the user currently has selected */
preferredLocale: PropTypes.string,

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

/** The transaction from the parent report action */
transactions: PropTypes.objectOf(
PropTypes.shape({
/** The ID of the transaction */
transactionID: PropTypes.string,
}),
),
/** List of draft comments */
draftComments: PropTypes.objectOf(PropTypes.string),
...withCurrentReportIDPropTypes,
};

const defaultProps = {
style: styles.flex1,
shouldDisableFocusOptions: false,
reportActions: {},
reports: {},
policy: {},
preferredLocale: CONST.LOCALES.DEFAULT,
personalDetails: {},
transactions: {},
draftComments: {},
...withCurrentReportIDDefaultProps,
};

function LHNOptionsList({style, contentContainerStyles, data, onSelectRow, optionMode, shouldDisableFocusOptions}) {
const keyExtractor = (item) => item;

function LHNOptionsList({
style,
contentContainerStyles,
data,
onSelectRow,
optionMode,
shouldDisableFocusOptions,
reports,
reportActions,
policy,
preferredLocale,
personalDetails,
transactions,
draftComments,
currentReportID,
}) {
/**
* 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 @@ -45,14 +112,17 @@ function LHNOptionsList({style, contentContainerStyles, data, onSelectRow, optio
*
* @returns {Object}
*/
const getItemLayout = (itemData, index) => {
const optionHeight = optionMode === CONST.OPTION_MODE.COMPACT ? variables.optionRowHeightCompact : variables.optionRowHeight;
return {
length: optionHeight,
offset: index * optionHeight,
index,
};
};
const getItemLayout = useCallback(
(itemData, index) => {
const optionHeight = optionMode === CONST.OPTION_MODE.COMPACT ? variables.optionRowHeightCompact : variables.optionRowHeight;
return {
length: optionHeight,
offset: index * optionHeight,
index,
};
},
[optionMode],
);

/**
* Function which renders a row in the list
Expand All @@ -62,13 +132,38 @@ function LHNOptionsList({style, contentContainerStyles, data, onSelectRow, optio
*
* @return {Component}
*/
const renderItem = ({item}) => (
<OptionRowLHNDataWithFocus
reportID={item}
viewMode={optionMode}
shouldDisableFocusOptions={shouldDisableFocusOptions}
onSelectRow={onSelectRow}
/>
const renderItem = useCallback(
({item: reportID}) => {
const itemFullReport = reports[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`] || {};
const itemReportActions = reportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`];
const itemParentReportActions = reportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${itemFullReport.parentReportID}`];
Copy link
Contributor

Choose a reason for hiding this comment

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

As we know already, there was crash after this refactor. itemParentReportActions was expected to be object but there's case of null.

const itemPolicy = policy[`${ONYXKEYS.COLLECTION.POLICY}${itemFullReport.policyID}`];
const itemTransaction = `${ONYXKEYS.COLLECTION.TRANSACTION}${lodashGet(
itemParentReportActions,
[itemFullReport.parentReportActionID, 'originalMessage', 'IOUTransactionID'],
'',
)}`;
const itemComment = draftComments[`${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`] || '';
const participantPersonalDetailList = _.values(OptionsListUtils.getPersonalDetailsForAccountIDs(itemFullReport.participantAccountIDs, personalDetails));
Copy link
Contributor

Choose a reason for hiding this comment

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

This misses an edge case where a user sent a last message in a room and then they leave the room. Then the report participant accountIDs do not include this user accountID and the user's email instead of their username is displayed in LHN. Issue #35477

return (
adhorodyski marked this conversation as resolved.
Show resolved Hide resolved
<OptionRowLHNData
reportID={reportID}
fullReport={itemFullReport}
reportActions={itemReportActions}
parentReportActions={itemParentReportActions}
policy={itemPolicy}
personalDetails={participantPersonalDetailList}
transaction={itemTransaction}
receiptTransactions={transactions}
viewMode={optionMode}
isFocused={!shouldDisableFocusOptions && reportID === currentReportID}
onSelectRow={onSelectRow}
preferredLocale={preferredLocale}
Copy link
Contributor

Choose a reason for hiding this comment

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

After adding preferredLocale to renderItem we should have added it to the FlatList's extraData too otherwise the list won't re-render

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a false positive, the above is true for FlashList only. For FlatList the change in renderItem itself seems enough

comment={itemComment}
/>
);
},
[currentReportID, draftComments, onSelectRow, optionMode, personalDetails, policy, preferredLocale, reportActions, reports, shouldDisableFocusOptions, transactions],
);

return (
Expand All @@ -80,7 +175,7 @@ function LHNOptionsList({style, contentContainerStyles, data, onSelectRow, optio
showsVerticalScrollIndicator={false}
data={data}
testID="lhn-options-list"
keyExtractor={(item) => item}
keyExtractor={keyExtractor}
stickySectionHeadersEnabled={false}
renderItem={renderItem}
getItemLayout={getItemLayout}
Expand All @@ -96,4 +191,29 @@ LHNOptionsList.propTypes = propTypes;
LHNOptionsList.defaultProps = defaultProps;
LHNOptionsList.displayName = 'LHNOptionsList';

export default LHNOptionsList;
export default compose(
withCurrentReportID,
withOnyx({
reports: {
key: ONYXKEYS.COLLECTION.REPORT,
},
reportActions: {
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
},
policy: {
key: ONYXKEYS.COLLECTION.POLICY,
},
preferredLocale: {
key: ONYXKEYS.NVP_PREFERRED_LOCALE,
},
personalDetails: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
},
transactions: {
key: ONYXKEYS.COLLECTION.TRANSACTION,
},
draftComments: {
key: ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT,
},
}),
)(LHNOptionsList);
90 changes: 7 additions & 83 deletions src/components/LHNOptionsList/OptionRowLHNData.js
Original file line number Diff line number Diff line change
@@ -1,27 +1,22 @@
import {deepEqual} from 'fast-equals';
import lodashGet from 'lodash/get';
import PropTypes from 'prop-types';
import React, {useEffect, useMemo, useRef} from 'react';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import participantPropTypes from '@components/participantPropTypes';
import compose from '@libs/compose';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import SidebarUtils from '@libs/SidebarUtils';
import * as TransactionUtils from '@libs/TransactionUtils';
import * as UserUtils from '@libs/UserUtils';
import reportActionPropTypes from '@pages/home/report/reportActionPropTypes';
import * as Report from '@userActions/Report';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import OptionRowLHN, {defaultProps as baseDefaultProps, propTypes as basePropTypes} from './OptionRowLHN';

const propTypes = {
/** Whether row should be focused */
isFocused: PropTypes.bool,

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

/** The preferred language for the app */
preferredLocale: PropTypes.string,
Expand All @@ -44,10 +39,8 @@ const propTypes = {
parentReportActions: PropTypes.objectOf(PropTypes.shape(reportActionPropTypes)),

/** The transaction from the parent report action */
transaction: PropTypes.shape({
/** The ID of the transaction */
transactionID: PropTypes.string,
}),
transactionID: PropTypes.string,

...basePropTypes,
};

Expand All @@ -57,7 +50,7 @@ const defaultProps = {
fullReport: {},
policy: {},
parentReportActions: {},
transaction: {},
transactionID: undefined,
preferredLocale: CONST.LOCALES.DEFAULT,
...baseDefaultProps,
};
Expand All @@ -78,11 +71,10 @@ function OptionRowLHNData({
policy,
receiptTransactions,
parentReportActions,
transaction,
transactionID,
...propsToForward
}) {
const reportID = propsToForward.reportID;

const parentReportAction = parentReportActions[fullReport.parentReportActionID];

const optionItemRef = useRef();
Expand All @@ -105,7 +97,7 @@ function OptionRowLHNData({
// Listen parentReportAction to update title of thread report when parentReportAction changed
// Listen to transaction to update title of transaction report when transaction changed
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [fullReport, linkedTransaction, reportActions, personalDetails, preferredLocale, policy, parentReportAction, transaction]);
}, [fullReport, linkedTransaction, reportActions, personalDetails, preferredLocale, policy, parentReportAction, transactionID]);

useEffect(() => {
if (!optionItem || optionItem.hasDraftComment || !comment || comment.length <= 0 || isFocused) {
Expand All @@ -129,79 +121,11 @@ 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,
status: personalData.status,
avatar: UserUtils.getAvatar(personalData.avatar, personalData.accountID),
fallbackIcon: personalData.fallbackIcon,
};
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(
withOnyx({
comment: {
key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`,
},
fullReport: {
key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
},
reportActions: {
key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
canEvict: false,
},
personalDetails: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
selector: personalDetailsSelector,
},
preferredLocale: {
key: ONYXKEYS.NVP_PREFERRED_LOCALE,
},
}),
// eslint-disable-next-line rulesdir/no-multiple-onyx-in-file
withOnyx({
parentReportActions: {
key: ({fullReport}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${fullReport.parentReportID}`,
canEvict: false,
},
policy: {
key: ({fullReport}) => `${ONYXKEYS.COLLECTION.POLICY}${fullReport.policyID}`,
},
// Ideally, we aim to access only the last transaction for the current report by listening to changes in reportActions.
// In some scenarios, a transaction might be created after reportActions have been modified.
// This can lead to situations where `lastTransaction` doesn't update and retains the previous value.
// However, performance overhead of this is minimized by using memos inside the component.
receiptTransactions: {key: ONYXKEYS.COLLECTION.TRANSACTION},
}),
// eslint-disable-next-line rulesdir/no-multiple-onyx-in-file
withOnyx({
transaction: {
key: ({fullReport, parentReportActions}) =>
`${ONYXKEYS.COLLECTION.TRANSACTION}${lodashGet(parentReportActions, [fullReport.parentReportActionID, 'originalMessage', 'IOUTransactionID'], '')}`,
},
}),
)(OptionRowLHNData),
);
export default React.memo(OptionRowLHNData);
Loading
Loading