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

perf: refactor heavy operations when user starts to type #28469

Merged
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
2cde96c
feat: add new key in onyx for draft report ids
hurali97 Sep 29, 2023
877a5f9
feat: add singleton class for draft report utils
hurali97 Sep 29, 2023
6903d5b
feat: confirm to new draft repord id onyx key
hurali97 Sep 29, 2023
64b095f
feat: add waitForCollectionCallback
hurali97 Sep 29, 2023
268e57a
perf: call updateUnread method if count is changed
hurali97 Sep 29, 2023
740700d
perf: use dict keys instead of whole dict and refactor the foreach loop
hurali97 Sep 29, 2023
cd0346e
fix: prettier fixes
hurali97 Oct 2, 2023
03b6221
test: fix unit tests
hurali97 Oct 3, 2023
dbeef98
test: fix Sidebar tests
hurali97 Oct 3, 2023
fcad1ec
fix: use allReportsDict in getordeeredReportIDs
hurali97 Oct 3, 2023
8fbd7a9
test: fix unit test
hurali97 Oct 3, 2023
713a35c
test: add test for draft report utils
hurali97 Oct 3, 2023
f7e50d6
fix: linting
hurali97 Oct 3, 2023
359a003
fix: replace lodashGet
hurali97 Oct 3, 2023
fb56966
docs: add js doc to DraftReportUtil
hurali97 Oct 3, 2023
c4c356e
Merge branch 'main' of https://github.com/hurali97/Expensify-App into…
hurali97 Oct 4, 2023
fbfe6d2
fix: revert changes due to unit test fixes
hurali97 Oct 4, 2023
bf44d95
Merge branch 'main' of https://github.com/hurali97/Expensify-App into…
hurali97 Oct 16, 2023
5c1b12b
fix: add initialValue to reduce re-renders
hurali97 Oct 16, 2023
783c6dc
Merge branch 'main' of https://github.com/hurali97/Expensify-App into…
hurali97 Oct 16, 2023
371c621
Merge branch 'main' of https://github.com/hurali97/Expensify-App into…
hurali97 Oct 18, 2023
5c9cbbe
Merge branch 'main' of https://github.com/hurali97/Expensify-App into…
hurali97 Oct 19, 2023
30a9064
fix: PR feedbacks
hurali97 Oct 20, 2023
c846ff1
fix: PR feedbacks
hurali97 Oct 20, 2023
1ec6d16
Merge branch 'main' of https://github.com/hurali97/Expensify-App into…
hurali97 Oct 25, 2023
a330467
revert: new onyx key for draft status
hurali97 Oct 25, 2023
ba8d66f
Merge branch 'main' of https://github.com/hurali97/Expensify-App into…
hurali97 Oct 30, 2023
d66e573
fix: typecheck
hurali97 Oct 31, 2023
3b77445
Merge branch 'main' of https://github.com/hurali97/Expensify-App into…
hurali97 Nov 3, 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
4 changes: 4 additions & 0 deletions src/ONYXKEYS.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ const ONYXKEYS = {
/** Holds information about the users account that is logging in */
ACCOUNT: 'account',

/** Holds the reportIDs which are currently in draft */
DRAFT_REPORT_IDS: 'draftReportIDs',

/** Holds the reportID for the report between the user and their account manager */
ACCOUNT_MANAGER_REPORT_ID: 'accountManagerReportID',

Expand Down Expand Up @@ -309,6 +312,7 @@ type OnyxKey = DeepValueOf<Omit<OnyxKeysMap, 'COLLECTION'>>;

type OnyxValues = {
[ONYXKEYS.ACCOUNT]: OnyxTypes.Account;
[ONYXKEYS.DRAFT_REPORT_IDS]: Record<string, boolean>;
[ONYXKEYS.ACCOUNT_MANAGER_REPORT_ID]: string;
[ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER]: boolean;
[ONYXKEYS.ACTIVE_CLIENTS]: string[];
Expand Down
18 changes: 16 additions & 2 deletions src/components/LHNOptionsList/OptionRowLHN.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import React, {useState, useRef, useCallback} from 'react';
import PropTypes from 'prop-types';
import {View, StyleSheet} from 'react-native';
import lodashGet from 'lodash/get';
import {withOnyx} from 'react-native-onyx';
import {useFocusEffect} from '@react-navigation/native';
import * as optionRowStyles from '../../styles/optionRowStyles';
import styles from '../../styles/styles';
Expand All @@ -26,6 +27,7 @@ import * as ReportUtils from '../../libs/ReportUtils';
import useLocalize from '../../hooks/useLocalize';
import Permissions from '../../libs/Permissions';
import Tooltip from '../Tooltip';
import ONYXKEYS from '../../ONYXKEYS';
import DomUtils from '../../libs/DomUtils';
import useWindowDimensions from '../../hooks/useWindowDimensions';
import ReportActionComposeFocusManager from '../../libs/ReportActionComposeFocusManager';
Expand Down Expand Up @@ -55,6 +57,9 @@ const propTypes = {
/** The item that should be rendered */
// eslint-disable-next-line react/forbid-prop-types
optionItem: PropTypes.object,

// eslint-disable-next-line react/forbid-prop-types
draftReportIDs: PropTypes.object,
};

const defaultProps = {
Expand All @@ -65,6 +70,7 @@ const defaultProps = {
optionItem: null,
isFocused: false,
betas: [],
draftReportIDs: {},
};

function OptionRowLHN(props) {
Expand Down Expand Up @@ -153,6 +159,7 @@ function OptionRowLHN(props) {
const formattedDate = DateUtils.getStatusUntilDate(statusClearAfterDate);
const statusContent = formattedDate ? `${statusText} (${formattedDate})` : statusText;
const isStatusVisible = Permissions.canUseCustomStatus(props.betas) && !!emojiCode && ReportUtils.isOneOnOneChat(optionItem);
const isDraft = props.draftReportIDs[props.reportID];
hurali97 marked this conversation as resolved.
Show resolved Hide resolved

return (
<OfflineWithFeedback
Expand Down Expand Up @@ -288,7 +295,7 @@ function OptionRowLHN(props) {
/>
</View>
)}
{optionItem.hasDraftComment && optionItem.isAllowedToComment && (
{isDraft && optionItem.isAllowedToComment && (
<View
style={styles.ml2}
accessibilityLabel={translate('sidebarScreen.draftedMessage')}
Expand Down Expand Up @@ -316,6 +323,13 @@ OptionRowLHN.propTypes = propTypes;
OptionRowLHN.defaultProps = defaultProps;
OptionRowLHN.displayName = 'OptionRowLHN';

export default React.memo(OptionRowLHN);
export default React.memo(
withOnyx({
draftReportIDs: {
key: ONYXKEYS.DRAFT_REPORT_IDS,
initialValue: {},
},
})(OptionRowLHN),
);

export {propTypes, defaultProps};
17 changes: 13 additions & 4 deletions src/components/LHNOptionsList/OptionRowLHNData.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,15 @@ import SidebarUtils from '../../libs/SidebarUtils';
import compose from '../../libs/compose';
import ONYXKEYS from '../../ONYXKEYS';
import OptionRowLHN, {propTypes as basePropTypes, defaultProps as baseDefaultProps} from './OptionRowLHN';
import * as Report from '../../libs/actions/Report';
import * as UserUtils from '../../libs/UserUtils';
import * as ReportActionsUtils from '../../libs/ReportActionsUtils';
import * as TransactionUtils from '../../libs/TransactionUtils';

import participantPropTypes from '../participantPropTypes';
import CONST from '../../CONST';
import reportActionPropTypes from '../../pages/home/report/reportActionPropTypes';
import setDraftStatusForReportID from '../../libs/actions/DraftReports';
import DraftReportUtils from '../../libs/DraftReportUtils';

const propTypes = {
/** Whether row should be focused */
Expand Down Expand Up @@ -110,10 +111,13 @@ function OptionRowLHNData({
}, [fullReport, linkedTransaction, reportActions, personalDetails, preferredLocale, policy, parentReportAction, transaction]);

useEffect(() => {
if (!optionItem || optionItem.hasDraftComment || !comment || comment.length <= 0 || isFocused) {
const draftReportIDs = DraftReportUtils.getInstance().getDraftReportIDs();
hurali97 marked this conversation as resolved.
Show resolved Hide resolved
const hasDraft = draftReportIDs[reportID];

if (!optionItem || hasDraft || !comment || comment.length <= 0 || isFocused) {
hurali97 marked this conversation as resolved.
Show resolved Hide resolved
return;
}
Report.setReportWithDraft(reportID, true);
setDraftStatusForReportID(reportID, true);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

Expand Down Expand Up @@ -174,14 +178,17 @@ export default React.memo(
withOnyx({
fullReport: {
key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
initialValue: {},
hurali97 marked this conversation as resolved.
Show resolved Hide resolved
},
reportActions: {
key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
canEvict: false,
initialValue: {},
},
personalDetails: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
selector: personalDetailsSelector,
initialValue: {},
},
preferredLocale: {
key: ONYXKEYS.NVP_PREFERRED_LOCALE,
Expand All @@ -192,15 +199,17 @@ export default React.memo(
parentReportActions: {
key: ({fullReport}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${fullReport.parentReportID}`,
canEvict: false,
initialValue: {},
},
policy: {
key: ({fullReport}) => `${ONYXKEYS.COLLECTION.POLICY}${fullReport.policyID}`,
initialValue: {},
},
// 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},
receiptTransactions: {key: ONYXKEYS.COLLECTION.TRANSACTION, initialValue: {}},
}),
// eslint-disable-next-line rulesdir/no-multiple-onyx-in-file
withOnyx({
Expand Down
3 changes: 0 additions & 3 deletions src/components/optionPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,6 @@ export default PropTypes.shape({
// reportID (only present when there is a matching report)
reportID: PropTypes.string,

// Whether the report has a draft comment or not
hasDraftComment: PropTypes.bool,

// Key used internally by React
keyForList: PropTypes.string,

Expand Down
54 changes: 54 additions & 0 deletions src/libs/DraftReportUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import Onyx from 'react-native-onyx';
import ONYXKEYS from '../ONYXKEYS';

/**
* A singleton class to manage the draft report IDs
* @class DraftReportUtils
* @singleton
*/
class DraftReportUtils {
private static instance: DraftReportUtils;

private draftReportIDs: Record<string, boolean>;

private constructor() {
DraftReportUtils.instance = this;

this.draftReportIDs = {};

this.subscribeToDraftReportIDs();
}

/**
* @returns The singleton instance
*/
public static getInstance(): DraftReportUtils {
// Ensure singleton instance
return DraftReportUtils.instance ?? new DraftReportUtils();
}

/**
* Subscribe to the draft report IDs
*/
private subscribeToDraftReportIDs() {
Onyx.connect({
key: ONYXKEYS.DRAFT_REPORT_IDS,
callback: (val) => {
if (!val) {
return;
}

this.draftReportIDs = val;
},
});
}

/**
* @returns The draft report IDs
*/
getDraftReportIDs() {
return this.draftReportIDs;
}
}

export default DraftReportUtils;
3 changes: 1 addition & 2 deletions src/libs/OptionsListUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ Onyx.connect({
const policyExpenseReports = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
Copy link
Contributor

@situchan situchan Nov 3, 2023

Choose a reason for hiding this comment

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

This caused serious regression. App crashes when request expense

Screen.Recording.2023-11-03.at.8.40.55.PM.mov

Anyone please raise quick fix? Or can I?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just encountered this issue too. Pushing a fix soon

Copy link
Contributor

Choose a reason for hiding this comment

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

@Gonals being reverted in #30852

callback: (report, key) => {
if (!ReportUtils.isPolicyExpenseChat(report)) {
return;
Expand Down Expand Up @@ -433,7 +434,6 @@ function createOption(accountIDs, personalDetails, report, reportActions = {}, {
login: null,
reportID: null,
phoneNumber: null,
hasDraftComment: false,
keyForList: null,
searchText: null,
isDefaultRoom: false,
Expand Down Expand Up @@ -480,7 +480,6 @@ function createOption(accountIDs, personalDetails, report, reportActions = {}, {
result.ownerAccountID = report.ownerAccountID;
result.reportID = report.reportID;
result.isUnread = ReportUtils.isUnread(report);
result.hasDraftComment = report.hasDraft;
result.isPinned = report.isPinned;
result.iouReportID = report.iouReportID;
result.keyForList = String(report.reportID);
Expand Down
38 changes: 29 additions & 9 deletions src/libs/ReportActionsUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import isReportMessageAttachment from './isReportMessageAttachment';
const allReports = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
callback: (report, key) => {
if (!key || !report) {
return;
Expand Down Expand Up @@ -47,7 +48,7 @@ Onyx.connect({
* @returns {Boolean}
*/
function isCreatedAction(reportAction) {
return lodashGet(reportAction, 'actionName') === CONST.REPORT.ACTIONS.TYPE.CREATED;
return reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED;
}

/**
Expand All @@ -56,16 +57,27 @@ function isCreatedAction(reportAction) {
*/
function isDeletedAction(reportAction) {
// A deleted comment has either an empty array or an object with html field with empty string as value
const message = lodashGet(reportAction, 'message', []);
return message.length === 0 || lodashGet(message, [0, 'html']) === '';
let message = [];
if (reportAction.message) {
message = reportAction.message;
}

if (message.length === 0) {
return true;
}

return message[0].html === '';
}

/**
* @param {Object} reportAction
* @returns {Boolean}
*/
function isDeletedParentAction(reportAction) {
return lodashGet(reportAction, ['message', 0, 'isDeletedParentAction'], false) && lodashGet(reportAction, 'childVisibleActionCount', 0) > 0;
const isDeleted = (reportAction && reportAction.message && reportAction.message[0] && reportAction.message[0].isDeletedParentAction) || false;
const childVisibleActionCount = (reportAction && reportAction.childVisibleActionCount) || 0;

return isDeleted && childVisibleActionCount > 0;
}

/**
Expand All @@ -81,7 +93,7 @@ function isReversedTransaction(reportAction) {
* @returns {Boolean}
*/
function isPendingRemove(reportAction) {
return lodashGet(reportAction, 'message[0].moderationDecision.decision') === CONST.MODERATION.MODERATOR_DECISION_PENDING_REMOVE;
return reportAction['message[0].moderationDecision.decision'] === CONST.MODERATION.MODERATOR_DECISION_PENDING_REMOVE;
}

/**
Expand Down Expand Up @@ -421,7 +433,10 @@ function getLastVisibleAction(reportID, actionsToMerge = {}) {
*/
function getLastVisibleMessage(reportID, actionsToMerge = {}) {
const lastVisibleAction = getLastVisibleAction(reportID, actionsToMerge);
const message = lodashGet(lastVisibleAction, ['message', 0], {});
let message = {};
if (lastVisibleAction.message) {
message = lastVisibleAction.message[0];
}

if (isReportMessageAttachment(message)) {
return {
Expand All @@ -437,9 +452,14 @@ function getLastVisibleMessage(reportID, actionsToMerge = {}) {
};
}

const messageText = lodashGet(message, 'text', '');
let messageText = message.text || '';

if (messageText) {
messageText = String(messageText).replace(CONST.REGEX.AFTER_FIRST_LINE_BREAK, '').substring(0, CONST.REPORT.LAST_MESSAGE_TEXT_MAX_LENGTH).trim();
}

return {
lastMessageText: String(messageText).replace(CONST.REGEX.AFTER_FIRST_LINE_BREAK, '').substring(0, CONST.REPORT.LAST_MESSAGE_TEXT_MAX_LENGTH).trim(),
lastMessageText: messageText,
};
}

Expand Down Expand Up @@ -638,7 +658,7 @@ function isTaskAction(reportAction) {
* @returns {[Object]}
*/
function getAllReportActions(reportID) {
return lodashGet(allReportActions, reportID, []);
return allReportActions[reportID] || [];
}

/**
Expand Down
Loading
Loading