[HOLD for payment 2023-11-17] [Performance] ReportScreen loading times

[kacper-mikolajczak]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

What performance issue do we need to solve?

Report screen suffers high loading times, especially with a growing amount of comments in a report.

What is the impact of this on end-users?

Report screen should load faster.

List any benchmarks that show the severity of the issue

Proposed solution (if any)

Analysis

Each comment in ReportScreen is assembled of some components whose rendering times exceed expected values.

Suboptimal rendering times of those components stem from a "retrieval" of heavy personalDetails object from Onyx. This call is made for every item in the list, in two separate places (ReportActionItem and ReportActionItemSingle) thus when there are many items, the list loading time slows down a ton.

Solution

The idea is to cache those direct Onyx calls with a help of React context, by using the alternative of already established withPersonalDetails HOC, usePersonalDetails hook. This way, we would have only a single instance of personalDetails that every item can easily access, instead of triggering a new costly retrieval process.

List any benchmarks after implementing the changes to show impacts of the proposed solution (if any)

On iOS, HT account, the difference in render times of ReportActionsList component are as follows:
First render: ~2800ms vs ~560ms
Second render: ~615ms vs ~450ms
Third render: ~540ms vs ~360ms
The render count remains the same.

Recording:

report_screen_loading-ios-comparison.mov

Platforms:

Which of our officially supported platforms is this issue occurring on?
Probably all of them.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010cdf03125ac6d05d
• Upwork Job ID: 1715142044114538496
• Last Price Increase: 2023-10-19

[melvin-bot]

Triggered auto assignment to @thienlnam (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[kacper-mikolajczak]

CC: @mountiny

[kacper-mikolajczak]

Initial Slack discussion: https://callstack-hq.slack.com/archives/C05LX9D6E07/p1697711577751729

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~010cdf03125ac6d05d

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

[melvin-bot]

📣 @eVoloshchak Please request via NewDot manual requests for the Reviewer role ($500)

[kacper-mikolajczak]

Hi @eVoloshchak! PR is ready to be reviewed - let me know if you need any info on this one 👍

[eVoloshchak]

@kacper-mikolajczak, reviewing now!

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.90-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [Perf] ReportScreen rendering optimisation #30063

If no regressions arise, payment will be issued on 2023-11-01. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @eVoloshchak does not require payment (Eligible for Manual Requests)
• @kacper-mikolajczak does not require payment (Contractor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

@eVoloshchak, @thienlnam, @kacper-mikolajczak Whoops! This issue is 2 days overdue. Let's get this updated quick!

[thienlnam]

Looks like we don't have a BZ member assigned to handle payment here - adding the label

[melvin-bot]

Triggered auto assignment to @sonialiap (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[thienlnam]

We're ready for a payment for C+ review to @eVoloshchak

[eVoloshchak]

Requested the $500 payment via NewDot

[eVoloshchak]

Just realized there were 2 PRs, the second one hasn't been deployed to production yet
@thienlnam, are we treating them as two separate PR's or as one PR divided into two?
In both cases, we should probably wait for the 7-day regression period on the second PR, deleting the payment request I sent above

[sonialiap]

The second PR deployed to prod on Nov 10, updating pay date to 17

@thienlnam bumping Eugene's question about how we're treating the PRs

[sonialiap]

@thienlnam why did we have two PRs? Did we miss something and needed to open a new one or did the scope grow? Asking to know if this should be billed as one review or two.

[kacper-mikolajczak]

Hi @sonialiap! 👋 Looking at the diffs of these 2 PRs, it looks like there was more to be achieved with further improvements under the same scope (this issue). I would think of it as a continuation of the task that spanned into another PR.

Hope that helps! 👍

[thienlnam]

Was OOO, but yeah let's just treat this as the one PR divided into 2 and use the latest PR for the payment date

[eVoloshchak]

Requested the payment!

[JmillsExpensify]

This issue lacks a payment summary.

[sonialiap]

Payment summary:

• @eVoloshchak PR review = $500

[JmillsExpensify]

$500 payment approved for @eVoloshchak based on summary above.