Fix: Custom memo for parent message of thread on LHN by dukenv0307 · Pull Request #24351 · Expensify/App

dukenv0307 · 2023-08-10T05:15:30Z

Details

Create memo custom comparator mentioned in this discussion: #23777 (comment)

Fixed Issues

$ #23424
PROPOSAL: #23424 (comment)

Tests

Go to new chat
Send New message
Go to the thread chat of the above message
Send new message on the thread chat
Update the parent message
Verify that the title of thread chat on LHN is updated correctly

Verify that no errors appear in the JS console

Offline tests

QA Steps

Go to new chat
Send New message
Go to the thread chat of the above message
Send new message on the thread chat
Update the parent message
Verify that the title of thread chat on LHN is updated correctly

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

chrome.mov

Mobile Web - Safari

safari.1.mp4

Desktop

desktop.mov

iOS

ios.1.mp4

Android

android.2.mp4

melvin-bot · 2023-08-11T10:15:00Z

@parasharrajat 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]

s77rt · 2023-08-12T23:53:32Z

+        },
+    }),
+)(
+    memo(OptionRowLHNData, (prevProps, nextProps) => {


Why the memo is not used at the top level? The current way it's used seems useless as we won't have access to any of the onyx props yet.

Actually, we can have access to all of the onyx props when using memo like this( I just console.log prevProps and nextProps here and can verify that all props are available here )
Please correct me if I am wrong

My bad on that one. I don't know what I was thinking 😅 But is there a reason/benefit to move the memo?

Because we can not have access to all Onyx props if keep the memo at the top level, only the default props

Got it! Can we still add a memo for the outer component?

@s77rt I believe that either the 'memo inside' or the 'outer' should be sufficient. I just double-checked the code, and we are only using one of the two.

Is there any harm in adding a memo for the outer component? I think we should still use it to prevent the parent from unnecessary re-renders

@s77rt On second thought, I think you are right, we should keep the outer memo.
Updated the PR once again :v

s77rt · 2023-08-13T00:01:47Z

+    memo(OptionRowLHNData, (prevProps, nextProps) => {
+        const prevParentReportActions = prevProps.parentReportActions[prevProps.fullReport.parentReportActionID];
+        const nextParentReportActions = nextProps.parentReportActions[nextProps.fullReport.parentReportActionID];
+        return prevParentReportActions === nextParentReportActions && _.isEqual(_.omit(prevProps, 'parentReportActions'), _.omit(nextProps, 'parentReportActions'));


For readability I think it would better to write this as

if (prevParentReportActions !== nextParentReportActions) { return false; } return _.isEqual(_.omit(prevProps, 'parentReportActions'), _.omit(nextProps, 'parentReportActions'));

s77rt · 2023-08-13T00:03:47Z

+        const prevParentReportActions = prevProps.parentReportActions[prevProps.fullReport.parentReportActionID];
+        const nextParentReportActions = nextProps.parentReportActions[nextProps.fullReport.parentReportActionID];
+        return prevParentReportActions === nextParentReportActions && _.isEqual(_.omit(prevProps, 'parentReportActions'), _.omit(nextProps, 'parentReportActions'));


Can we do the same for policies, I think we only care about one policy (fullReport.policyID)

Added. Please help check again

s77rt · 2023-08-13T00:04:58Z

+    memo(OptionRowLHNData, (prevProps, nextProps) => {
+        const prevParentReportActions = prevProps.parentReportActions[prevProps.fullReport.parentReportActionID];
+        const nextParentReportActions = nextProps.parentReportActions[nextProps.fullReport.parentReportActionID];
+        return prevParentReportActions === nextParentReportActions && _.isEqual(_.omit(prevProps, 'parentReportActions'), _.omit(nextProps, 'parentReportActions'));


Instead of using _.isEqual which performs a deep compare, can we use Object.is as that's the default that memo uses.

parasharrajat · 2023-08-14T12:49:48Z

+            key: ONYXKEYS.COLLECTION.POLICY,
+        },
+    }),
+    withOnyx({


Let's add a comment here explaining why two different subscriptions are needed.

s77rt · 2023-08-16T18:38:09Z

+        if (prevParentReportActions !== nextParentReportActions || prevPolicy !== nextPolicy) {
+            return false;
+        }


NAB, but splitting this into two if conditions is more readable

@s77rt updated. Please check again

s77rt · 2023-08-16T19:01:05Z

+        },
+    }),
+)(
+    memo(OptionRowLHNData, (prevProps, nextProps) => {


Got it! Can we still add a memo for the outer component?

s77rt · 2023-08-16T19:20:02Z

+        if (prevParentReportActions !== nextParentReportActions || prevPolicy !== nextPolicy) {
+            return false;
+        }
+        return _.omit(prevProps, 'parentReportActions', 'policies') === _.omit(nextProps, 'parentReportActions', 'policies');


We should be using Object.is and not ===. Object.is should be applied to each prop. It may be easier to just use _.isEqual.

@s77rt Sorry, I am a little bit confused. So your suggestion is using Object.is or .isEqual?

use _.isEqual

s77rt

LGTM! 🚀

s77rt · 2023-08-18T14:02:49Z

Oops I guess I shouldn't have approved 😅 @puneetlath This is a follow up for #23777 (comment). @parasharrajat Would you please complete the checklist here

parasharrajat · 2023-08-18T21:08:06Z

+            const prevParentReportActions = prevProps.parentReportActions[prevProps.fullReport.parentReportActionID];
+            const nextParentReportActions = nextProps.parentReportActions[nextProps.fullReport.parentReportActionID];


is there a possibility that parentReportActions is null or undefined?

Yes, parentReportActions can be undefined

Then won't this line throw the error as we are trying to access properties on a undefined value.

parasharrajat · 2023-08-18T21:17:28Z

I am thinking. Can this memo hurt app performance? DeepEqual check is costly

dukenv0307 · 2023-08-21T15:51:22Z

@parasharrajat I agree with you. DeepEqual in this case can be a really expensive operation since many deep layers have to be compared

dukenv0307 · 2023-08-23T02:51:10Z

@parasharrajat I wonder if any further action should we do to address the issue

parasharrajat · 2023-08-23T10:35:30Z

I don't think this PR is urgent or blocking anything. I will like to do some performance testing on this before we move forward. Unfortunately, I don't have much experience in this area so It can take some time for me to get to that point. If you think that we should merge this PR quickly, let me know.

parasharrajat · 2023-09-07T16:30:18Z

Update: I don't think we should merge this PR. Memo on each LHN row will become expensive quite easily. Onyx does not give immutable data so deep comparison is really expensive.

I think we are fine with one-row updates on each report action. Anyways, we do update this row for updating the last message, etc. on each action.

parasharrajat · 2023-09-07T16:32:36Z

We will also be performing various optimizations on the app thus if this is a problem, it will be taken care of.

Thanks, @dukenv0307 @s77rt for creating this PR. Appreciate your efforts.

s77rt · 2023-09-18T13:13:19Z

@dukenv0307 Can you close this please?

dukenv0307 added 3 commits August 10, 2023 12:01

fix: add custom memo for parent message of thread in LHN

e8011d5

fix: custom memo comparator

7bc57b2

add new line

9c51ecd

dukenv0307 changed the title ~~Fix/custom memo for parent message of thread on LHN~~ Fix: Custom memo for parent message of thread on LHN Aug 10, 2023

dukenv0307 added 2 commits August 11, 2023 10:50

ad custom comparator to LHN

ad6cbd0

fix logic for memo custom comparator

4ac83ed

dukenv0307 marked this pull request as ready for review August 11, 2023 10:14

dukenv0307 requested a review from a team as a code owner August 11, 2023 10:14

melvin-bot Bot requested review from parasharrajat and removed request for a team August 11, 2023 10:14

dukenv0307 mentioned this pull request Aug 11, 2023

Parent Message of thread chat on LHN doesn't updated when the parent message is updated #23777

Merged

58 tasks

s77rt suggested changes Aug 13, 2023

View reviewed changes

parasharrajat reviewed Aug 14, 2023

View reviewed changes

fix: memo logic

5fd2b09

s77rt suggested changes Aug 16, 2023

View reviewed changes

dukenv0307 added 2 commits August 17, 2023 15:48

fix: split if condition

ea5b89e

add outer memo

4931139

s77rt approved these changes Aug 18, 2023

View reviewed changes

melvin-bot Bot requested a review from puneetlath August 18, 2023 14:00

parasharrajat reviewed Aug 18, 2023

View reviewed changes

dukenv0307 closed this Sep 18, 2023

		const prevParentReportActions = prevProps.parentReportActions[prevProps.fullReport.parentReportActionID];
		const nextParentReportActions = nextProps.parentReportActions[nextProps.fullReport.parentReportActionID];

Conversation

dukenv0307 commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot Bot commented Aug 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s77rt left a comment

Choose a reason for hiding this comment

Uh oh!

s77rt commented Aug 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat commented Aug 18, 2023

Uh oh!

dukenv0307 commented Aug 21, 2023

Uh oh!

dukenv0307 commented Aug 23, 2023

Uh oh!

parasharrajat commented Aug 23, 2023

Uh oh!

parasharrajat commented Sep 7, 2023

Uh oh!

dukenv0307 commented Aug 10, 2023 •

edited

Loading