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

Improvement: Sidebar Links Performance #26447

Merged
merged 23 commits into from
Sep 19, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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
18 changes: 11 additions & 7 deletions src/libs/ReportActionsUtils.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
/* eslint-disable rulesdir/prefer-underscore-method */
import lodashGet from 'lodash/get';
import _ from 'underscore';
import lodashMerge from 'lodash/merge';
import {max, parseISO} from 'date-fns';
import lodashFindLast from 'lodash/findLast';
import Onyx from 'react-native-onyx';
import moment from 'moment';
waterim marked this conversation as resolved.
Show resolved Hide resolved
import * as CollectionUtils from './CollectionUtils';
import CONST from '../CONST';
import ONYXKEYS from '../ONYXKEYS';
Expand Down Expand Up @@ -371,14 +371,18 @@ function shouldReportActionBeVisibleAsLastAction(reportAction) {
* @return {Object}
*/
function getLastVisibleAction(reportID, actionsToMerge = {}) {
const actions = _.toArray(lodashMerge({}, allReportActions[reportID], actionsToMerge));
const visibleActions = _.filter(actions, (action) => shouldReportActionBeVisibleAsLastAction(action));
const actions = Object.values({
...allReportActions[reportID],
...actionsToMerge,
});
mountiny marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this change introduced this issue: #28941 fixed in this PR: #29939

const visibleActions = actions.filter((action) => shouldReportActionBeVisibleAsLastAction(action));
waterim marked this conversation as resolved.
Show resolved Hide resolved

if (_.isEmpty(visibleActions)) {
if (visibleActions.length === 0) {
return {};
}

return _.max(visibleActions, (action) => moment.utc(action.created).valueOf());
const maxDate = max(visibleActions.map((action) => parseISO(action.created)));
const maxAction = visibleActions.find((action) => parseISO(action.created).toString() === maxDate.toString());
waterim marked this conversation as resolved.
Show resolved Hide resolved
return maxAction;
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/libs/ReportUtils.js
waterim marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -2754,7 +2754,7 @@ function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, betas,
!report ||
!report.reportID ||
report.isHidden ||
(_.isEmpty(report.participantAccountIDs) && !isChatThread(report) && !isPublicRoom(report) && !isArchivedRoom(report) && !isMoneyRequestReport(report) && !isTaskReport(report))
(report.participantAccountIDs.length === 0 && !isChatThread(report) && !isPublicRoom(report) && !isArchivedRoom(report) && !isMoneyRequestReport(report) && !isTaskReport(report))
waterim marked this conversation as resolved.
Show resolved Hide resolved
) {
return false;
}
Expand Down Expand Up @@ -2790,7 +2790,7 @@ function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, betas,

// Include reports that have errors from trying to add a workspace
// If we excluded it, then the red-brock-road pattern wouldn't work for the user to resolve the error
if (report.errorFields && !_.isEmpty(report.errorFields.addWorkspaceRoom)) {
if (report.errorFields && report.errorFields.addWorkspaceRoom !== 0) {
return true;
}

Expand Down
81 changes: 47 additions & 34 deletions src/libs/SidebarUtils.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* eslint-disable rulesdir/prefer-underscore-method */
import Onyx from 'react-native-onyx';
import _ from 'underscore';
import lodashGet from 'lodash/get';
import lodashOrderBy from 'lodash/orderBy';
import Str from 'expensify-common/lib/str';
import ONYXKEYS from '../ONYXKEYS';
import * as ReportUtils from './ReportUtils';
Expand Down Expand Up @@ -75,6 +75,9 @@ function setIsSidebarLoadedReady() {
resolveSidebarIsReadyPromise();
}

// Define a cache object to store the memoized results
const reportIDsCache = new Map();
waterim marked this conversation as resolved.
Show resolved Hide resolved

/**
* @param {String} currentReportId
* @param {Object} allReportsDict
Expand All @@ -85,22 +88,37 @@ function setIsSidebarLoadedReady() {
* @returns {String[]} An array of reportIDs sorted in the proper order
*/
function getOrderedReportIDs(currentReportId, allReportsDict, betas, policies, priorityMode, allReportActions) {
// Generate a unique cache key based on the function arguments
const cachedReports = JSON.stringify([currentReportId, allReportsDict, betas, policies, priorityMode, Object.values(allReportActions).length], (key, value) => {
waterim marked this conversation as resolved.
Show resolved Hide resolved
// Exclude 'participantAccountIDs', 'participants' and 'lastMessageText' properties from all objects in the 'allReportsDict' array
if (key === 'participantAccountIDs' || key === 'participants' || key === 'lastMessageText') {
return undefined;
}
return value;
});
// // Check if the result is already in the cache
waterim marked this conversation as resolved.
Show resolved Hide resolved
if (reportIDsCache.has(cachedReports)) {
return reportIDsCache.get(cachedReports);
}

const isInGSDMode = priorityMode === CONST.PRIORITY_MODE.GSD;
const isInDefaultMode = !isInGSDMode;

// Filter out all the reports that shouldn't be displayed
const reportsToDisplay = _.filter(allReportsDict, (report) => ReportUtils.shouldReportBeInOptionList(report, currentReportId, isInGSDMode, betas, policies, allReportActions, true));
const reportsToDisplay = Object.values(allReportsDict).filter((report) =>
ReportUtils.shouldReportBeInOptionList(report, currentReportId, isInGSDMode, betas, policies, allReportActions, true),
);

if (_.isEmpty(reportsToDisplay)) {
if (reportsToDisplay.length === 0) {
// Display Concierge chat report when there is no report to be displayed
const conciergeChatReport = _.find(allReportsDict, ReportUtils.isConciergeChatReport);
const conciergeChatReport = Object.values(allReportsDict).find(ReportUtils.isConciergeChatReport);
waterim marked this conversation as resolved.
Show resolved Hide resolved
if (conciergeChatReport) {
reportsToDisplay.push(conciergeChatReport);
}
}

// There are a few properties that need to be calculated for the report which are used when sorting reports.
_.each(reportsToDisplay, (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.
Expand All @@ -121,43 +139,36 @@ function getOrderedReportIDs(currentReportId, allReportsDict, betas, policies, p
// 5. Archived reports
// - Sorted by lastVisibleActionCreated in default (most recent) view mode
// - Sorted by reportDisplayName in GSD (focus) view mode
let pinnedReports = [];
let outstandingIOUReports = [];
let draftReports = [];
let nonArchivedReports = [];
let archivedReports = [];
_.each(reportsToDisplay, (report) => {
const pinnedReports = [];
const outstandingIOUReports = [];
const draftReports = [];
const nonArchivedReports = [];
const archivedReports = [];
reportsToDisplay.forEach((report) => {
if (report.isPinned) {
pinnedReports.push(report);
return;
}

if (ReportUtils.isWaitingForIOUActionFromCurrentUser(report)) {
} else if (ReportUtils.isWaitingForIOUActionFromCurrentUser(report)) {
outstandingIOUReports.push(report);
return;
}

if (report.hasDraft) {
} else if (report.hasDraft) {
draftReports.push(report);
return;
}

if (ReportUtils.isArchivedRoom(report)) {
} else if (ReportUtils.isArchivedRoom(report)) {
archivedReports.push(report);
return;
} else {
nonArchivedReports.push(report);
}

nonArchivedReports.push(report);
});

// Sort each group of reports accordingly
pinnedReports = _.sortBy(pinnedReports, (report) => report.displayName.toLowerCase());
outstandingIOUReports = lodashOrderBy(outstandingIOUReports, ['iouReportAmount', (report) => report.displayName.toLowerCase()], ['desc', 'asc']);
draftReports = _.sortBy(draftReports, (report) => report.displayName.toLowerCase());
nonArchivedReports = isInDefaultMode
? lodashOrderBy(nonArchivedReports, ['lastVisibleActionCreated', (report) => report.displayName.toLowerCase()], ['desc', 'asc'])
: lodashOrderBy(nonArchivedReports, [(report) => report.displayName.toLowerCase()], ['asc']);
archivedReports = _.sortBy(archivedReports, (report) => (isInDefaultMode ? report.lastVisibleActionCreated : report.displayName.toLowerCase()));
pinnedReports.sort((a, b) => a.displayName.localeCompare(b.displayName));
outstandingIOUReports.sort((a, b) => b.iouReportAmount - a.iouReportAmount || a.displayName.localeCompare(b.displayName));
draftReports.sort((a, b) => a.displayName.localeCompare(b.displayName));
if (isInDefaultMode) {
nonArchivedReports.sort((a, b) => new Date(b.lastVisibleActionCreated) - new Date(a.lastVisibleActionCreated) || a.displayName.localeCompare(b.displayName));
archivedReports.sort((a, b) => new Date(b.lastVisibleActionCreated) - new Date(a.lastVisibleActionCreated));
waterim marked this conversation as resolved.
Show resolved Hide resolved
waterim marked this conversation as resolved.
Show resolved Hide resolved
} else {
nonArchivedReports.sort((a, b) => a.displayName.localeCompare(b.displayName));
archivedReports.sort((a, b) => a.displayName.localeCompare(b.displayName));
}

// For archived reports ensure that most recent reports are at the top by reversing the order of the arrays because underscore will only sort them in ascending order
waterim marked this conversation as resolved.
Show resolved Hide resolved
if (isInDefaultMode) {
Expand All @@ -166,7 +177,9 @@ function getOrderedReportIDs(currentReportId, allReportsDict, betas, policies, p

// Now that we have all the reports grouped and sorted, they must be flattened into an array and only return the reportID.
// The order the arrays are concatenated in matters and will determine the order that the groups are displayed in the sidebar.
return _.pluck([].concat(pinnedReports).concat(outstandingIOUReports).concat(draftReports).concat(nonArchivedReports).concat(archivedReports), 'reportID');
const LHNReports = [].concat(pinnedReports, outstandingIOUReports, draftReports, nonArchivedReports, archivedReports).map((report) => report.reportID);
reportIDsCache.set(cachedReports, LHNReports);
waterim marked this conversation as resolved.
Show resolved Hide resolved
return LHNReports;
}

/**
Expand Down