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 all 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
7 changes: 6 additions & 1 deletion src/components/LHNOptionsList/OptionRowLHNData.js
Original file line number Diff line number Diff line change
Expand Up @@ -168,14 +168,17 @@ export default React.memo(
},
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 @@ -186,15 +189,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
1 change: 1 addition & 0 deletions src/libs/OptionsListUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,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
7 changes: 5 additions & 2 deletions src/libs/ReportActionsUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -406,9 +406,12 @@ function getLastVisibleMessage(reportID: string, actionsToMerge: ReportActions =
};
}

const messageText = 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
26 changes: 22 additions & 4 deletions src/libs/ReportUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -817,8 +817,15 @@ function isOneOnOneChat(report) {
* @returns {Object}
*/
function getReport(reportID) {
// Deleted reports are set to null and lodashGet will still return null in that case, so we need to add an extra check
return lodashGet(allReports, `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {}) || {};
/**
* Using typical string concatenation here due to performance issues
* with template literals.
*/
if (!allReports) {
return {};
}

return allReports[ONYXKEYS.COLLECTION.REPORT + reportID] || {};
}

/**
Expand Down Expand Up @@ -1518,14 +1525,25 @@ function getMoneyRequestSpendBreakdown(report, allReportsDict = null) {
* @returns {String}
*/
function getPolicyExpenseChatName(report, policy = undefined) {
const reportOwnerDisplayName = getDisplayNameForParticipant(report.ownerAccountID) || lodashGet(allPersonalDetails, [report.ownerAccountID, 'login']) || report.reportName;
const ownerAccountID = report.ownerAccountID;
const personalDetails = allPersonalDetails[ownerAccountID];
const login = personalDetails ? personalDetails.login : null;
const reportOwnerDisplayName = getDisplayNameForParticipant(report.ownerAccountID) || login || report.reportName;

// If the policy expense chat is owned by this user, use the name of the policy as the report name.
if (report.isOwnPolicyExpenseChat) {
return getPolicyName(report, false, policy);
}

const policyExpenseChatRole = lodashGet(allPolicies, [`${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`, 'role']) || 'user';
let policyExpenseChatRole = 'user';
/**
* Using typical string concatenation here due to performance issues
* with template literals.
*/
const policyItem = allPolicies[ONYXKEYS.COLLECTION.POLICY + report.policyID];
if (policyItem) {
policyExpenseChatRole = policyItem.role || 'user';
}

// If this user is not admin and this policy expense chat has been archived because of account merging, this must be an old workspace chat
// of the account which was merged into the current user's account. Use the name of the policy as the name of the report.
Expand Down
22 changes: 10 additions & 12 deletions src/libs/SidebarUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,18 +156,6 @@ function getOrderedReportIDs(
}
}

// There are a few properties that need to be calculated for the report which are used when sorting reports.
reportsToDisplay.forEach((report) => {
// Normally, the spread operator would be used here to clone the report and prevent the need to reassign the params.
// However, this code needs to be very performant to handle thousands of reports, so in the interest of speed, we're just going to disable this lint rule and add
// the reportDisplayName property to the report object directly.
// eslint-disable-next-line no-param-reassign
report.displayName = ReportUtils.getReportName(report);

// eslint-disable-next-line no-param-reassign
report.iouReportAmount = ReportUtils.getMoneyRequestReimbursableTotal(report, allReports);
});

// The LHN is split into four distinct groups, and each group is sorted a little differently. The groups will ALWAYS be in this order:
// 1. Pinned/GBR - Always sorted by reportDisplayName
// 2. Drafts - Always sorted by reportDisplayName
Expand All @@ -181,7 +169,17 @@ function getOrderedReportIDs(
const draftReports: Report[] = [];
const nonArchivedReports: Report[] = [];
const archivedReports: Report[] = [];

reportsToDisplay.forEach((report) => {
// Normally, the spread operator would be used here to clone the report and prevent the need to reassign the params.
// However, this code needs to be very performant to handle thousands of reports, so in the interest of speed, we're just going to disable this lint rule and add
// the reportDisplayName property to the report object directly.
// eslint-disable-next-line no-param-reassign
report.displayName = ReportUtils.getReportName(report);

// eslint-disable-next-line no-param-reassign
report.iouReportAmount = ReportUtils.getMoneyRequestReimbursableTotal(report, allReports);

const isPinned = report.isPinned ?? false;
if (isPinned || ReportUtils.requiresAttentionFromCurrentUser(report)) {
pinnedAndGBRReports.push(report);
Expand Down
27 changes: 25 additions & 2 deletions src/libs/UnreadIndicatorUpdater/index.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,38 @@
import {InteractionManager} from 'react-native';
import Onyx from 'react-native-onyx';
import _ from 'underscore';
import * as ReportUtils from '@libs/ReportUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import updateUnread from './updateUnread/index';

let previousUnreadCount = 0;

Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
callback: (reportsFromOnyx) => {
const unreadReports = _.filter(reportsFromOnyx, (report) => ReportUtils.isUnread(report) && report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN);
updateUnread(_.size(unreadReports));
if (!reportsFromOnyx) {
return;
}

/**
* We need to wait until after interactions have finished to update the unread count because otherwise
* the unread count will be updated while the interactions/animations are in progress and we don't want
* to put more work on the main thread.
*
* For eg. On web we are manipulating DOM and it makes it a better candidate to wait until after interactions
* have finished.
*
* More info: https://reactnative.dev/docs/interactionmanager
*/
InteractionManager.runAfterInteractions(() => {
const unreadReports = _.filter(reportsFromOnyx, (report) => ReportUtils.isUnread(report) && report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN);
const unreadReportsCount = _.size(unreadReports);
if (previousUnreadCount !== unreadReportsCount) {
previousUnreadCount = unreadReportsCount;
updateUnread(unreadReportsCount);
}
});
},
});
1 change: 1 addition & 0 deletions src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ Onyx.connect({
const currentReportData = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
hurali97 marked this conversation as resolved.
Show resolved Hide resolved
callback: (data, key) => {
if (!key || !data) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,8 +226,14 @@ function ComposerWithSuggestions({
}
}
const newCommentConverted = convertToLTRForComposer(newComment);
const isNewCommentEmpty = !!newCommentConverted.match(/^(\s)*$/);
const isPrevCommentEmpty = !!commentRef.current.match(/^(\s)*$/);

/** Only update isCommentEmpty state if it's different from previous one */
if (isNewCommentEmpty !== isPrevCommentEmpty) {
setIsCommentEmpty(isNewCommentEmpty);
}
emojisPresentBefore.current = emojis;
setIsCommentEmpty(!!newCommentConverted.match(/^(\s)*$/));
setValue(newCommentConverted);
if (commentValue !== newComment) {
const remainder = ComposerUtils.getCommonSuffixLength(commentValue, newComment);
Expand Down Expand Up @@ -504,9 +510,7 @@ function ComposerWithSuggestions({
if (value.length === 0) {
return;
}

Report.setReportWithDraft(reportID, true);

// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

Expand Down
5 changes: 5 additions & 0 deletions src/pages/home/sidebar/SidebarLinksData.js
Original file line number Diff line number Diff line change
Expand Up @@ -198,23 +198,28 @@ export default compose(
chatReports: {
key: ONYXKEYS.COLLECTION.REPORT,
selector: chatReportSelector,
initialValue: {},
},
isLoadingReportData: {
key: ONYXKEYS.IS_LOADING_REPORT_DATA,
},
priorityMode: {
key: ONYXKEYS.NVP_PRIORITY_MODE,
initialValue: CONST.PRIORITY_MODE.DEFAULT,
},
betas: {
key: ONYXKEYS.BETAS,
initialValue: [],
},
allReportActions: {
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
selector: reportActionsSelector,
initialValue: {},
},
policies: {
key: ONYXKEYS.COLLECTION.POLICY,
selector: policySelector,
initialValue: {},
},
}),
)(SidebarLinksData);
Loading