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.

Sign up for GitHub

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

Jump to bottom

Issue 39091 - use waitForCollectionCallback #40233

Merged
merged 5 commits into from
Apr 17, 2024
Merged

Issue 39091 - use waitForCollectionCallback #40233

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3ce6084
use waitForCollectionCallback
EzraEllette Apr 15, 2024
3b50179
Update SidebarUtils reportActionsProcessing
EzraEllette Apr 15, 2024
0a16364
Merge branch 'Expensify:main' into iss-39091-use-waitForCollectionCal…
EzraEllette Apr 16, 2024
9fa396f
useWaitForCollectionCallback no iter
EzraEllette Apr 16, 2024
eb1f011
use waitForCollectionCallback no iter
EzraEllette Apr 16, 2024
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
11 changes: 5 additions & 6 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,16 +534,15 @@ Onyx.connect({
},
});

const reportActionsByReport: OnyxCollection<ReportActions> = {};
let reportActionsByReport: OnyxCollection<ReportActions> = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
callback: (actions, key) => {
if (!key || !actions) {
waitForCollectionCallback: true,
callback: (actions) => {
if (!actions) {
return;
}

const reportID = CollectionUtils.extractCollectionItemID(key);
reportActionsByReport[reportID] = actions;
reportActionsByReport = Object.fromEntries(Object.entries(actions).map(([key, value]) => [CollectionUtils.extractCollectionItemID(key as `reportActions_${string}`), value]));
Copy link
Contributor

Choose a reason for hiding this comment

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

From looking at other places in the code that use waitForCollectionCallback, is this .map() necessary? (same comment goes for the other places)

Copy link
Contributor Author

@EzraEllette EzraEllette Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I believe it is necessary unless we would prefer to access reports by report id using

[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`]

instead of the reportID, I think it is better that we don't change too much of the functionality of this. In fact I'm going to revert some changes I made to SidebarUtils, so that the file is closer to its original functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`] is the most common pattern used in the code. Looking at IOU.ts for example and the allReports object. All of the references look like:

iouReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${chatReport.iouReportID}`] ?? null;

I'm not able to find any waitForCollectionCallback: true instance that is using a map(). I think that's what allows for a performance gain. Since the code currently in this PR replaces n callbacks with a map that does n invocations, there really isn't any performance gain.

},
});

Expand Down
44 changes: 27 additions & 17 deletions src/libs/SidebarUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,31 +23,41 @@ import * as ReportUtils from './ReportUtils';
import * as TaskUtils from './TaskUtils';
import * as UserUtils from './UserUtils';

const reportActionsByReport: OnyxCollection<ReportActions> = {};
const visibleReportActionItems: ReportActions = {};
let reportActionsByReport: OnyxCollection<ReportActions> = {};
let visibleReportActionItems: ReportActions = {};

Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
callback: (actions, key) => {
if (!key || !actions) {
waitForCollectionCallback: true,
callback: (actions) => {
if (!actions) {
return;
}

const reportID = CollectionUtils.extractCollectionItemID(key);
reportActionsByReport[reportID] = actions;

const actionsArray: ReportAction[] = ReportActionsUtils.getSortedReportActions(Object.values(actions));
reportActionsByReport = Object.fromEntries(
Object.entries(actions).map(([key, reportActions]) => {
const reportID = CollectionUtils.extractCollectionItemID(key as `reportActions_${string}`);
return [reportID, reportActions];
}),
);

// The report is only visible if it is the last action not deleted that
// does not match a closed or created state.
const reportActionsForDisplay = actionsArray.filter(
(reportAction, actionKey) =>
ReportActionsUtils.shouldReportActionBeVisible(reportAction, actionKey) &&
!ReportActionsUtils.isWhisperAction(reportAction) &&
reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED &&
reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
visibleReportActionItems = Object.fromEntries(
Object.entries(reportActionsByReport).map(([reportID, reportActions]) => {
const actionsArray: ReportAction[] = ReportActionsUtils.getSortedReportActions(Object.values(reportActions ?? {}));

// The report is only visible if it is the last action not deleted that
// does not match a closed or created state.
const reportActionsForDisplay = actionsArray.filter(
(reportAction, actionKey) =>
ReportActionsUtils.shouldReportActionBeVisible(reportAction, actionKey) &&
!ReportActionsUtils.isWhisperAction(reportAction) &&
reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED &&
reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
);

return [reportID, reportActionsForDisplay[reportActionsForDisplay.length - 1]];
}),
);
visibleReportActionItems[reportID] = reportActionsForDisplay[reportActionsForDisplay.length - 1];
},
});

Expand Down
13 changes: 6 additions & 7 deletions src/libs/WorkspacesSettingsUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,15 @@ Onyx.connect({
},
});

const reportActionsByReport: OnyxCollection<ReportActions> = {};
let reportActionsByReport: OnyxCollection<ReportActions> = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
callback: (actions, key) => {
if (!key || !actions) {
waitForCollectionCallback: true,
callback: (actions) => {
if (!actions) {
return;
}

const reportID = CollectionUtils.extractCollectionItemID(key);
reportActionsByReport[reportID] = actions;
reportActionsByReport = Object.fromEntries(Object.entries(actions).map(([key, value]) => [CollectionUtils.extractCollectionItemID(key as `reportActions_${string}`), value]));
},
});

Expand All @@ -80,7 +79,7 @@ const getBrickRoadForPolicy = (report: Report): BrickRoad => {
// To determine if the report requires attention from the current user, we need to load the parent report action
let itemParentReportAction = {};
if (report.parentReportID) {
const itemParentReportActions = reportActionsByReport[report.parentReportID] ?? {};
const itemParentReportActions = reportActionsByReport?.[report.parentReportID] ?? {};
itemParentReportAction = report.parentReportActionID ? itemParentReportActions[report.parentReportActionID] : {};
}
const reportOption = {...report, isUnread: ReportUtils.isUnread(report), isUnreadWithMention: ReportUtils.isUnreadWithMention(report)};
Expand Down
11 changes: 5 additions & 6 deletions src/libs/actions/IOU.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,16 +259,15 @@ Onyx.connect({
callback: (value) => (allPolicies = value),
});

const reportActionsByReport: OnyxCollection<OnyxTypes.ReportActions> = {};
let reportActionsByReport: OnyxCollection<OnyxTypes.ReportActions> = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
callback: (actions, key) => {
if (!key || !actions) {
waitForCollectionCallback: true,
callback: (actions) => {
if (!actions) {
return;
}

const reportID = CollectionUtils.extractCollectionItemID(key);
reportActionsByReport[reportID] = actions;
reportActionsByReport = Object.fromEntries(Object.entries(actions).map(([key, value]) => [CollectionUtils.extractCollectionItemID(key as `reportActions_${string}`), value]));
},
});

Expand Down
11 changes: 5 additions & 6 deletions src/libs/actions/ReportActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,15 @@ function clearReportActionErrors(reportID: string, reportAction: ReportAction, k
});
}

const reportActionsByReport: OnyxCollection<ReportActions> = {};
let reportActionsByReport: OnyxCollection<ReportActions> = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
callback: (actions, key) => {
if (!key || !actions) {
waitForCollectionCallback: true,
callback: (actions) => {
if (!actions) {
return;
}

const reportID = CollectionUtils.extractCollectionItemID(key);
reportActionsByReport[reportID] = actions;
reportActionsByReport = Object.fromEntries(Object.entries(actions).map(([key, value]) => [CollectionUtils.extractCollectionItemID(key as `reportActions_${string}`), value]));
},
});

Expand Down
Loading