Remove exported `getReport()` #40523

gijoe0295 · 2024-04-19T00:32:58Z

Details

Remove exported getReport.

Fixed Issues

$ #40316
PROPOSAL: #40316 (comment)

Tests

Test basic features of reports and report actions: New report, Order of reports in LHN, Send/Delete/Edit/Reply/Mark as unread message, Message context menu, Message notifications, Quick action, Hold IOU request,...
Some edge cases: Fix - Add BA thread is missing Add button, incomplete thread header and "Hidden" user #38884, prevent offline navigation on money request report deletion #34320, https://github.com/Expensify/App/pull/28702/files#r1343844837

Verify that no errors appear in the JS console

Offline tests

NA

QA Steps

Test basic features of reports and report actions: New report, Order of reports in LHN, Send/Delete/Edit/Reply/Mark as unread message, Message context menu, Message notifications, Quick action, Hold IOU request,...

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2024-04-22.at.03.19.21-source.mov

Android: mWeb Chrome

iOS: Native

Screen.Recording.2024-04-22.at.02.57.34-source.mov

Screen.Recording.2024-04-22.at.03.00.55-source.mov

iOS: mWeb Safari

Screen.Recording.2024-04-22.at.03.02.27-source.mov

MacOS: Chrome / Safari

MacOS: Desktop

desktop.mov

melvin-bot · 2024-04-21T20:07:28Z

@s77rt Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

gijoe0295 · 2024-04-21T20:08:11Z

@s77rt Initial testing and performance comparison seems promising. Ready for your review.

s77rt

Resolve conflicts

src/components/LHNOptionsList/OptionRowLHN.tsx

src/libs/Notification/PushNotification/subscribeToReportCommentPushNotifications.ts

src/libs/actions/IOU.ts

src/libs/actions/Report.ts

src/pages/home/report/ContextMenu/ContextMenuActions.tsx

src/libs/ReportUtils.ts

s77rt · 2024-04-23T16:32:49Z

src/pages/home/ReportScreen.tsx

+const parentReportSelector = (report: OnyxEntry<OnyxTypes.Report>): OnyxEntry<OnyxTypes.Report> =>
+    report && {
+        type: report.type,
+        reportID: report.reportID,
+        parentReportID: report.parentReportID,
+        parentReportActionID: report.parentReportActionID,
+    };
+


How did you choose this selector? As far as I can tell there is no indication on what fields we are using from parentReport

I just deduced it from parentReport's usages and thought that might improve performance. But it's trivial anyway.

I also saw a technique used for memoized report:

App/src/pages/home/ReportScreen.tsx

Lines 162 to 169 in a5989aa

/**

* Create a lightweight Report so as to keep the re-rendering as light as possible by

* passing in only the required props.

*

* Also, this plays nicely in contrast with Onyx,

* which creates a new object every time collection changes. Because of this we can't

* put this into onyx selector as it will be the same.

*/

I think we should apply to parentReport as well. Wdyt?

That sounds good to me

gijoe0295 · 2024-04-25T10:26:58Z

src/pages/home/report/ReportActionItem.tsx

+        if (!originalReportID || originalReportID === report.reportID) {
+            return;
+        }
+        const unsubscribeOnyx = onyxSubscribe({


@s77rt Instead of using reportOrID approach, I used onyxSubscribe to conditionally subscribe to originalReportID, subscription with withOnyx will increase the re-render count 40 times 😱.

Onyx.connect should not be used in components. Can you check for other options?

Hmm but this is onyxSubscribe which I see was used a lot in components, for example ReportActionItemParentAction

The usage there also does not follow our Onyx practices.

s77rt · 2024-05-06T05:06:14Z

@gijoe0295 Any update on the use of onyxSubscribe?

s77rt · 2024-05-06T05:25:27Z

src/pages/home/report/SystemChatReportFooterMessage.tsx

@@ -88,4 +84,13 @@ export default withOnyx<SystemChatReportFooterMessageProps, SystemChatReportFoot
        key: ONYXKEYS.NVP_ACTIVE_POLICY_ID,
        initialValue: null,
    },
+    adminChatReport: {
+        key: ({activePolicyID, policies}) => {


activePolicyID and policies won't be available here. They are populated after withOnyx. Let's use useOnyx and we can avoid such problem

s77rt · 2024-05-06T06:04:34Z

src/pages/home/report/ReportActionItem.tsx

@@ -555,7 +574,7 @@ function ReportActionItem({
                </ShowContextMenuContext.Provider>
            );
        } else if (action.actionName === CONST.REPORT.ACTIONS.TYPE.REIMBURSEMENT_QUEUED) {
-            const linkedReport = ReportUtils.isChatThread(report) ? ReportUtils.getReport(report.parentReportID) : report;
+            const linkedReport = ReportUtils.isChatThread(report) ? parentReport : report;


Looks we can get ride of the parentReport. We only need 2 props from the parent

reportID which can be replaced by report.parentReportID

ownerAccountID write an util function for that

s77rt · 2024-05-15T23:17:44Z

@gijoe0295 Any updates on this?

tgolen · 2024-05-17T15:39:55Z

@gijoe0295 What is the status of this PR? Can you please finish it up?

gijoe0295 · 2024-05-22T00:00:23Z

After the changes in #41465, reportDraft was included in getReport, I got to investigate further to see where it was used.

s77rt · 2024-06-03T20:04:36Z

@gijoe0295 Any updates here?

gijoe0295 added 2 commits April 19, 2024 07:24

remove getReport() usages

953375c

add test

3aa8402

gijoe0295 changed the title ~~Gijoe/40316~~ Remove exported getReport() Apr 19, 2024

gijoe0295 added 4 commits April 19, 2024 07:35

Resolve conflicts

8fba091

resolve conflicts

2b0b9c7

pass parentreport from reportscreen

b42d2ee

fix test

738c31f

gijoe0295 marked this pull request as ready for review April 21, 2024 20:07

gijoe0295 requested a review from a team as a code owner April 21, 2024 20:07

melvin-bot bot removed the request for review from a team April 21, 2024 20:07

melvin-bot bot requested a review from s77rt April 21, 2024 20:07

s77rt suggested changes Apr 23, 2024

View reviewed changes

gijoe0295 added 10 commits April 24, 2024 00:19

Resolve conflicts

89fa689

pass report from OptionRowLHNData

dd343b2

resolve minor comments

8c51a60

resolve minor comments

fe90f67

clear deleted entry from dictionary

9a2216a

remove parentReportSelector

a0e156f

resolve conflicts

0e3a0c0

memoize parentReport

3d16c4d

Merge branch 'main' of https://github.com/gijoe0295/App into gijoe/40316

6982795

subscribe to originalReport

e78b388

gijoe0295 commented Apr 25, 2024

View reviewed changes

gijoe0295 added 3 commits May 5, 2024 21:17

Resolve conflicts

c129073

Remove getReport from Task and SystemChatReportFooterMessage

1dfe6e5

fix lint

357bc03

s77rt suggested changes May 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove exported `getReport()` #40523

Remove exported `getReport()` #40523

gijoe0295 commented Apr 19, 2024 •

edited

melvin-bot bot commented Apr 21, 2024

gijoe0295 commented Apr 21, 2024

s77rt left a comment

s77rt Apr 23, 2024

gijoe0295 Apr 23, 2024

gijoe0295 Apr 23, 2024 •

edited

s77rt Apr 24, 2024

gijoe0295 Apr 25, 2024

gijoe0295 Apr 25, 2024

s77rt Apr 26, 2024

gijoe0295 Apr 26, 2024 •

edited

s77rt Apr 26, 2024

s77rt commented May 6, 2024

s77rt May 6, 2024

s77rt May 6, 2024

s77rt commented May 15, 2024

tgolen commented May 17, 2024

gijoe0295 commented May 22, 2024

s77rt commented Jun 3, 2024

	/**
	* Create a lightweight Report so as to keep the re-rendering as light as possible by
	* passing in only the required props.
	*
	* Also, this plays nicely in contrast with Onyx,
	* which creates a new object every time collection changes. Because of this we can't
	* put this into onyx selector as it will be the same.
	*/

Remove exported getReport() #40523

Are you sure you want to change the base?

Remove exported getReport() #40523

Conversation

gijoe0295 commented Apr 19, 2024 • edited

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Apr 21, 2024

gijoe0295 commented Apr 21, 2024

s77rt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gijoe0295 Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gijoe0295 Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s77rt commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s77rt commented May 15, 2024

tgolen commented May 17, 2024

gijoe0295 commented May 22, 2024

s77rt commented Jun 3, 2024

Remove exported `getReport()` #40523

Remove exported `getReport()` #40523

gijoe0295 commented Apr 19, 2024 •

edited

gijoe0295 Apr 23, 2024 •

edited

gijoe0295 Apr 26, 2024 •

edited