From 59e5f83d7e610dc7a808e6f862a4ccf2a06fc311 Mon Sep 17 00:00:00 2001 From: Jakub Kosmydel <104823336+kosmydel@users.noreply.github.com> Date: Mon, 10 Jun 2024 09:11:49 +0200 Subject: [PATCH 1/5] draft: measure times --- src/hooks/useReportIDs.tsx | 10 +++++++--- src/libs/SidebarUtils.ts | 11 +++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/hooks/useReportIDs.tsx b/src/hooks/useReportIDs.tsx index 46ffa7d999ee..d60e4b538c73 100644 --- a/src/hooks/useReportIDs.tsx +++ b/src/hooks/useReportIDs.tsx @@ -129,8 +129,9 @@ function ReportIDsContextProvider({ const policyMemberAccountIDs = getPolicyEmployeeListByIdWithoutCurrentUser(policies, activeWorkspaceID, accountID); const getOrderedReportIDs = useCallback( - (currentReportID?: string) => - SidebarUtils.getOrderedReportIDs( + (currentReportID?: string) => { + console.time('getOrderedReportIDs'); + const result = SidebarUtils.getOrderedReportIDs( currentReportID ?? null, chatReports, betas, @@ -140,7 +141,10 @@ function ReportIDsContextProvider({ transactionViolations, activeWorkspaceID, policyMemberAccountIDs, - ), + ); + console.timeEnd('getOrderedReportIDs'); + return result; + }, // we need reports draft in deps array for reloading of list when reportsDrafts will change // eslint-disable-next-line react-hooks/exhaustive-deps [chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs, reportsDrafts], diff --git a/src/libs/SidebarUtils.ts b/src/libs/SidebarUtils.ts index 3df823db22df..ba3978fa055f 100644 --- a/src/libs/SidebarUtils.ts +++ b/src/libs/SidebarUtils.ts @@ -79,6 +79,7 @@ function getOrderedReportIDs( const isInDefaultMode = !isInFocusMode; const allReportsDictValues = Object.values(allReports ?? {}); + console.time('getOrderedReportIDs 1: reportsToDisplay'); // Filter out all the reports that shouldn't be displayed let reportsToDisplay = allReportsDictValues.filter((report) => { if (!report) { @@ -115,6 +116,7 @@ function getOrderedReportIDs( includeSelfDM: true, }); }); + console.timeEnd('getOrderedReportIDs 1: reportsToDisplay'); // 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 @@ -130,11 +132,14 @@ function getOrderedReportIDs( const nonArchivedReports: Array> = []; const archivedReports: Array> = []; + console.time('getOrderedReportIDs 2: sort'); if (currentPolicyID || policyMemberAccountIDs.length > 0) { reportsToDisplay = reportsToDisplay.filter( (report) => report?.reportID === currentReportId || ReportUtils.doesReportBelongToWorkspace(report, policyMemberAccountIDs, currentPolicyID), ); } + console.timeEnd('getOrderedReportIDs 2: sort'); + console.time('getOrderedReportIDs 3: sort'); // There are a few properties that need to be calculated for the report which are used when sorting reports. reportsToDisplay.forEach((reportToDisplay) => { let report = reportToDisplay as OnyxEntry; @@ -157,6 +162,9 @@ function getOrderedReportIDs( nonArchivedReports.push(report); } }); + console.timeEnd('getOrderedReportIDs 3: sort'); + + console.time('getOrderedReportIDs 4: sort'); // Sort each group of reports accordingly pinnedAndGBRReports.sort((a, b) => (a?.displayName && b?.displayName ? localeCompare(a.displayName, b.displayName) : 0)); @@ -177,10 +185,13 @@ function getOrderedReportIDs( nonArchivedReports.sort((a, b) => (a?.displayName && b?.displayName ? localeCompare(a.displayName, b.displayName) : 0)); archivedReports.sort((a, b) => (a?.displayName && b?.displayName ? localeCompare(a.displayName, b.displayName) : 0)); } + console.timeEnd('getOrderedReportIDs 4: sort'); // 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. + console.time('getOrderedReportIDs 5: flatten'); const LHNReports = [...pinnedAndGBRReports, ...draftReports, ...nonArchivedReports, ...archivedReports].map((report) => report?.reportID ?? ''); + console.timeEnd('getOrderedReportIDs 5: flatten'); return LHNReports; } From 57771ff292ac379c39639edb36f326a1b08b3c96 Mon Sep 17 00:00:00 2001 From: Jakub Kosmydel <104823336+kosmydel@users.noreply.github.com> Date: Mon, 10 Jun 2024 09:13:14 +0200 Subject: [PATCH 2/5] feat: do not copy reports --- src/libs/SidebarUtils.ts | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/src/libs/SidebarUtils.ts b/src/libs/SidebarUtils.ts index ba3978fa055f..69a6a3ae8434 100644 --- a/src/libs/SidebarUtils.ts +++ b/src/libs/SidebarUtils.ts @@ -61,6 +61,12 @@ function compareStringDates(a: string, b: string): 0 | 1 | -1 { return 0; } +type MiniReport = { + reportID?: string; + displayName: string; + lastVisibleActionCreated?: string; +}; + /** * @returns An array of reportIDs sorted in the proper order */ @@ -127,10 +133,10 @@ function getOrderedReportIDs( // 4. Archived reports // - Sorted by lastVisibleActionCreated in default (most recent) view mode // - Sorted by reportDisplayName in GSD (focus) view mode - const pinnedAndGBRReports: Array> = []; - const draftReports: Array> = []; - const nonArchivedReports: Array> = []; - const archivedReports: Array> = []; + const pinnedAndGBRReports: MiniReport[] = []; + const draftReports: MiniReport[] = []; + const nonArchivedReports: MiniReport[] = []; + const archivedReports: MiniReport[] = []; console.time('getOrderedReportIDs 2: sort'); if (currentPolicyID || policyMemberAccountIDs.length > 0) { @@ -142,24 +148,23 @@ function getOrderedReportIDs( console.time('getOrderedReportIDs 3: sort'); // There are a few properties that need to be calculated for the report which are used when sorting reports. reportsToDisplay.forEach((reportToDisplay) => { - let report = reportToDisplay as OnyxEntry; - if (report) { - report = { - ...report, - displayName: ReportUtils.getReportName(report), - }; - } + const report = reportToDisplay as OnyxEntry; + const miniReport: MiniReport = { + reportID: report?.reportID, + displayName: ReportUtils.getReportName(report), + lastVisibleActionCreated: report?.lastVisibleActionCreated, + }; const isPinned = report?.isPinned ?? false; const reportAction = ReportActionsUtils.getReportAction(report?.parentReportID ?? '', report?.parentReportActionID ?? ''); if (isPinned || ReportUtils.requiresAttentionFromCurrentUser(report, reportAction)) { - pinnedAndGBRReports.push(report); + pinnedAndGBRReports.push(miniReport); } else if (hasValidDraftComment(report?.reportID ?? '')) { - draftReports.push(report); + draftReports.push(miniReport); } else if (ReportUtils.isArchivedRoom(report)) { - archivedReports.push(report); + archivedReports.push(miniReport); } else { - nonArchivedReports.push(report); + nonArchivedReports.push(miniReport); } }); console.timeEnd('getOrderedReportIDs 3: sort'); From aa2f789912b37ff311bdd48c7367ee444d28ca8f Mon Sep 17 00:00:00 2001 From: Jakub Kosmydel <104823336+kosmydel@users.noreply.github.com> Date: Mon, 10 Jun 2024 14:57:05 +0200 Subject: [PATCH 3/5] cleanup: remove redundant timings --- src/libs/SidebarUtils.ts | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/libs/SidebarUtils.ts b/src/libs/SidebarUtils.ts index 69a6a3ae8434..b8c56c17f723 100644 --- a/src/libs/SidebarUtils.ts +++ b/src/libs/SidebarUtils.ts @@ -85,7 +85,6 @@ function getOrderedReportIDs( const isInDefaultMode = !isInFocusMode; const allReportsDictValues = Object.values(allReports ?? {}); - console.time('getOrderedReportIDs 1: reportsToDisplay'); // Filter out all the reports that shouldn't be displayed let reportsToDisplay = allReportsDictValues.filter((report) => { if (!report) { @@ -122,7 +121,6 @@ function getOrderedReportIDs( includeSelfDM: true, }); }); - console.timeEnd('getOrderedReportIDs 1: reportsToDisplay'); // 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 @@ -138,14 +136,11 @@ function getOrderedReportIDs( const nonArchivedReports: MiniReport[] = []; const archivedReports: MiniReport[] = []; - console.time('getOrderedReportIDs 2: sort'); if (currentPolicyID || policyMemberAccountIDs.length > 0) { reportsToDisplay = reportsToDisplay.filter( (report) => report?.reportID === currentReportId || ReportUtils.doesReportBelongToWorkspace(report, policyMemberAccountIDs, currentPolicyID), ); } - console.timeEnd('getOrderedReportIDs 2: sort'); - console.time('getOrderedReportIDs 3: sort'); // There are a few properties that need to be calculated for the report which are used when sorting reports. reportsToDisplay.forEach((reportToDisplay) => { const report = reportToDisplay as OnyxEntry; @@ -167,9 +162,6 @@ function getOrderedReportIDs( nonArchivedReports.push(miniReport); } }); - console.timeEnd('getOrderedReportIDs 3: sort'); - - console.time('getOrderedReportIDs 4: sort'); // Sort each group of reports accordingly pinnedAndGBRReports.sort((a, b) => (a?.displayName && b?.displayName ? localeCompare(a.displayName, b.displayName) : 0)); @@ -190,13 +182,10 @@ function getOrderedReportIDs( nonArchivedReports.sort((a, b) => (a?.displayName && b?.displayName ? localeCompare(a.displayName, b.displayName) : 0)); archivedReports.sort((a, b) => (a?.displayName && b?.displayName ? localeCompare(a.displayName, b.displayName) : 0)); } - console.timeEnd('getOrderedReportIDs 4: sort'); // 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. - console.time('getOrderedReportIDs 5: flatten'); const LHNReports = [...pinnedAndGBRReports, ...draftReports, ...nonArchivedReports, ...archivedReports].map((report) => report?.reportID ?? ''); - console.timeEnd('getOrderedReportIDs 5: flatten'); return LHNReports; } From 11af4fdaf77a02f29daa81c17f94a6ba9090add0 Mon Sep 17 00:00:00 2001 From: Jakub Kosmydel <104823336+kosmydel@users.noreply.github.com> Date: Wed, 12 Jun 2024 10:01:10 +0200 Subject: [PATCH 4/5] cleanup: remove timings --- src/hooks/useReportIDs.tsx | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/hooks/useReportIDs.tsx b/src/hooks/useReportIDs.tsx index d60e4b538c73..46ffa7d999ee 100644 --- a/src/hooks/useReportIDs.tsx +++ b/src/hooks/useReportIDs.tsx @@ -129,9 +129,8 @@ function ReportIDsContextProvider({ const policyMemberAccountIDs = getPolicyEmployeeListByIdWithoutCurrentUser(policies, activeWorkspaceID, accountID); const getOrderedReportIDs = useCallback( - (currentReportID?: string) => { - console.time('getOrderedReportIDs'); - const result = SidebarUtils.getOrderedReportIDs( + (currentReportID?: string) => + SidebarUtils.getOrderedReportIDs( currentReportID ?? null, chatReports, betas, @@ -141,10 +140,7 @@ function ReportIDsContextProvider({ transactionViolations, activeWorkspaceID, policyMemberAccountIDs, - ); - console.timeEnd('getOrderedReportIDs'); - return result; - }, + ), // we need reports draft in deps array for reloading of list when reportsDrafts will change // eslint-disable-next-line react-hooks/exhaustive-deps [chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs, reportsDrafts], From cfaaf5c9a1476c2843bcde465aeb53d1e1d843ee Mon Sep 17 00:00:00 2001 From: Jakub Kosmydel <104823336+kosmydel@users.noreply.github.com> Date: Wed, 12 Jun 2024 10:39:20 +0200 Subject: [PATCH 5/5] address review: add JSDoc --- src/libs/SidebarUtils.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libs/SidebarUtils.ts b/src/libs/SidebarUtils.ts index 4af6ceb622af..a9239e6a4fa0 100644 --- a/src/libs/SidebarUtils.ts +++ b/src/libs/SidebarUtils.ts @@ -62,6 +62,10 @@ function compareStringDates(a: string, b: string): 0 | 1 | -1 { return 0; } +/** + * A mini report object that contains only the necessary information to sort reports. + * This is used to avoid copying the entire report object and only the necessary information. + */ type MiniReport = { reportID?: string; displayName: string;