Optimize report ordering in the LHN #10784

tgolen · 2022-09-02T10:23:16Z

This is the first step in optimizing the ordering logic of the sidebar. It does these things:

Removes all state data from the SidebarLinks component, which removes the need for setDerivedStateFromProps and any other static methods
It makes the rendering so efficient that it doesn't really matter how many times it's re-renderes or how many items it has
It optimizes all the stuff happening in OptionsListUtils so that it's very efficient and only doing the minimum amount of logic in the fastest way

Fixed Issues

Part of #7948

Tests

Everything is covered by unit tests, but in general you need to test:

Chats in the LHN show as draft, pinned, unread, and have outstanding IOUs and that changes those states updates the LHN appropriately
Switching view mode changes the views as expected

Screenshots

Web

Mobile Web

Desktop

iOS

Android

tgolen

Updated and ready for another review!

tgolen · 2022-09-08T11:01:06Z

src/pages/home/sidebar/SidebarLinks.js

-                    focusedIndex={_.findIndex(this.state.orderedReports, (
-                        option => option.reportID === activeReportID
+                    focusedIndex={_.findIndex(this.orderedReports, (
+                        option => option.reportID.toString() === this.props.currentlyViewedReportID.toString()


option.reportID comes from Onyx, where it's stored as a number, because PHP is sending it as a number. The second one is already a string, so I've removed the unnecessary .toString() from it.

This will ultimately be cleaned up in https://github.com/Expensify/Expensify/issues/227425

tgolen · 2022-09-08T11:01:37Z

tests/unit/LHNOrderTest.js

@@ -158,18 +158,37 @@ Onyx.init({
 });

 function getDefaultRenderedSidebarLinks() {
+    class ErrorBoundary extends React.Component {
+        // Error boundaries have to implement this method. It's for providing a fallback UI, but
+        // we don't need that for unit testing, so this is basically a no-op.


I've added a big code comment to explain the why behind why this is necessary.

tgolen · 2022-09-08T16:18:45Z

I haven't had the time to test this on all platforms, but I would like to do that before this is merged. Maybe I'll have the time to do this tomorrow.

marcaaron · 2022-09-09T07:33:21Z

Tested on iOS and Desktop. Seems to be working very well.

marcaaron

Changes are looking good to me. I'd be curious to see some before and after snapshots to get a better idea of which changes impacted performance the most, but that is NAB.

I also think it would be good to distill any of the things we learned here and add them to the PERFORMANCE.md doc with some tips on when some of the strategies used in this PR would be appropriate to use.

tgolen · 2022-09-09T08:21:24Z

Thanks, Marc! I'm trying to test on Android... and it's, totally unusable. I get the feeling though that it's not related to my branch and only related to the sheer number of reports. For example, I'm seeing errors like this:

tgolen · 2022-09-09T08:23:05Z

I also see these logs, which indicate that while it takes ~220ms on web to sort and filter the reports, it's taking almost 30 seconds on android.

Timing:expensify.cash.sidebar_links_filter_reports 31150

tgolen · 2022-09-09T08:32:39Z

Hm, OK. Same thing is happening on main, so I don't think this branch is causing it (or fixing it unfortunately). I think maybe when I get the second PR done, that's when it might start working 🤞

tgolen · 2022-09-09T09:31:03Z

As I was doing some performance testing this morning, to get you snapshots of main vs. this branch, it's pretty interesting.

This branch actually doesn't change much the rendering time of the LHN. BUT it achieves the same performance as main without having all of the memoized methods and the serDerivedStateFromProps().

Where we're really going to see the performance gains is in #10800

tgolen · 2022-09-13T15:36:10Z

I'm going to self merge since it's got a couple of reviews. This will let me move onto my next step of optimizing. The two failing tests are OK because there are no contributors involved with this one.

melvin-bot · 2022-09-13T15:36:20Z

@tgolen looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

OSBotify · 2022-09-13T15:40:19Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

francoisl · 2022-09-14T22:28:59Z

🚀 Cherry-picked to staging by @francoisl in version: 1.2.0-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

Note: we had a conflict on the staging branch which ended up needing to be manually fixed, so this PR doesn't automatically appear in the diff between production and staging - so I'm manually adding the note that it was CPed to staging here.

…barordering" This reverts commit 23f4fc8, reversing changes made to c614dfc.

OSBotify · 2022-09-16T02:47:14Z

🚀 Deployed to staging by @tgolen in version: 1.2.1-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

tgolen added 30 commits August 25, 2022 20:03

Remove the last pieces out of state

fed705a

Simplify the check for unread reports

44a2ff6

Simplify the map of unread reports

57a4c2e

Correct outdated comment

4b40492

Rename method to be more apparent

00ebe57

Access filtered report options direction from render

103b126

Remove the import of unused library

bb57fb0

Fix a prop warning

f3a7fa0

Add some performance timing for filtering the reports

35cfd23

Rename method to be more clear

8e8ba1f

Remove mobile preoptimization

6a26a03

Correct the language in a comment

d973776

Remove old code

b770d0c

Merge branch 'main' into tgolen-remove-deriveState

8a6ea06

Rename method to make more sense and add comments

9626972

Add comments to the logic for ordering reports

6091608

Add a todo

56d6d34

Merge branch 'main' into tgolen-optimize-sidebarordering

853f8b8

Fix proptype warnings

44e258c

Remove unnecessary binding

bd05cec

Remove debug code that is not needed

475f0ed

Fix proptype warning

80615c9

Don't cast activeReportID to string

de15223

Remove all data processing

8512aa0

Add report actions to the propTypes

ee56ec4

WIP on profiling what makes things rerender when switching reports

e7f7b33

Fix proptype warning

8057f21

WIP optimizing create option

ff49948

WIP optimizing create option

7c51a86

WIP optimizing create option

fd60c45

tgolen added 4 commits September 8, 2022 11:46

Add a comment about why we use an error boundary

629c4c4

Fix return data type

55bd67f

Remove an unnecessary string cast

84b1be4

Add JSDocs

5ca3b5e

tgolen commented Sep 8, 2022

View reviewed changes

marcaaron approved these changes Sep 9, 2022

View reviewed changes

luacmartins approved these changes Sep 9, 2022

View reviewed changes

tgolen merged commit 23f4fc8 into main Sep 13, 2022

tgolen deleted the tgolen-optimize-sidebarordering branch September 13, 2022 15:36

melvin-bot bot added the Emergency label Sep 13, 2022

tgolen removed the Emergency label Sep 13, 2022

francoisl mentioned this pull request Sep 14, 2022

Deploy Checklist: New Expensify 2022-09-13 #10958

Closed

30 tasks

This was referenced Sep 15, 2022

Android - Chat - App crashed after sign in with specific account #11003

Closed

[$2000] Web/mweb - Chat - LHN does not display after signing out/signing in #11011

Closed

francoisl added a commit that referenced this pull request Sep 15, 2022

Revert "Merge pull request #10784 from Expensify/tgolen-optimize-side…

bfdc7bb

…barordering" This reverts commit 23f4fc8, reversing changes made to c614dfc.

This was referenced Sep 15, 2022

Revert #10784 and #11006 #11018

Closed

Add missing alternateText option in option #11023

Merged

OSBotify mentioned this pull request Sep 16, 2022

Deploy Checklist: New Expensify 2022-09-16 #11043

Closed

27 tasks

ishpaul777 mentioned this pull request Jun 28, 2024

[HOLD for payment 2024-06-28] [$250] IOU-User is shown option to submit expense using group. #42261

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize report ordering in the LHN #10784

Optimize report ordering in the LHN #10784

tgolen commented Sep 2, 2022 •

edited

Loading

tgolen left a comment

tgolen Sep 8, 2022

tgolen Sep 8, 2022

tgolen commented Sep 8, 2022

marcaaron commented Sep 9, 2022

marcaaron left a comment

tgolen commented Sep 9, 2022 •

edited

Loading

tgolen commented Sep 9, 2022

tgolen commented Sep 9, 2022

tgolen commented Sep 9, 2022

tgolen commented Sep 13, 2022

melvin-bot bot commented Sep 13, 2022

OSBotify commented Sep 13, 2022

francoisl commented Sep 14, 2022

OSBotify commented Sep 16, 2022

Optimize report ordering in the LHN #10784

Optimize report ordering in the LHN #10784

Conversation

tgolen commented Sep 2, 2022 • edited Loading

Fixed Issues

Tests

Screenshots

Web

Mobile Web

Desktop

iOS

Android

tgolen left a comment

Choose a reason for hiding this comment

tgolen Sep 8, 2022

Choose a reason for hiding this comment

tgolen Sep 8, 2022

Choose a reason for hiding this comment

tgolen commented Sep 8, 2022

marcaaron commented Sep 9, 2022

marcaaron left a comment

Choose a reason for hiding this comment

tgolen commented Sep 9, 2022 • edited Loading

tgolen commented Sep 9, 2022

tgolen commented Sep 9, 2022

tgolen commented Sep 9, 2022

tgolen commented Sep 13, 2022

melvin-bot bot commented Sep 13, 2022

OSBotify commented Sep 13, 2022

francoisl commented Sep 14, 2022

OSBotify commented Sep 16, 2022

tgolen commented Sep 2, 2022 •

edited

Loading

tgolen commented Sep 9, 2022 •

edited

Loading