[HOLD for payment 2023-08-08] [Performance] Show cached onyx content right away

[hannojg]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

What performance issue do we need to solve?

We are trying to make Onyx more performant by default. Right now, onyx always starts in a loading state, then collects all the
requested keys and sets them to the component state. This is often times inefficient as all the data is already available in a cache which we can read synchronously.

For exact technical details see: Expensify/react-native-onyx#269

What is the impact of this on end-users?

• Unnecessary loading states: We often start by default in a loading state, although we could show the content right away. Showing the content directly provides better UX
• Performance impacts due to a lot of re-renders: Due to this architecture a component re-renders a lot until it reached its final loaded state. This comes with a performance cost creating janky transitions to screens.

List any benchmarks that show the severity of the issue

See: Expensify/react-native-onyx#269

Proposed solution (if any)

See: Expensify/react-native-onyx#269

Once we have the onyx changes in place we need to update onyx in the app and adjust a few screens to work correctly. Some screens are build with the assumption that we will always start in a loading state.
We will start by adjusting the ReportScreen. The following adjustments need to be made (as discussed here):

• Defer the loading of new chat messages to the next batch, so it doesn’t block the initial rendering by changing a state variable on the report.
• There was recently a firstRenderRef introduced, which delays the real rendering of the report screen after it has been mounted. We want to get rid of it to show the available content right away.

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

See: Expensify/react-native-onyx#269

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: x
Reproducible in staging?: yes
Reproducible in production?: yes
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
**Issue reported by: @hannojg **
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1688580518417739

[melvin-bot]

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

[iwiznia]

@hannojg I assume you want this assigned to you since you'll be working on it, right?

[hannojg]

Yes, correct, thank you!

[hannojg]

The PR is ready for review:

• [Fix #269] use cached data react-native-onyx#280

[melvin-bot]

@hannojg Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[hannojg]

This is on hold as the work and discussions are happening here:

• [Fix #269] use cached data react-native-onyx#280

[melvin-bot]

@hannojg Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[hannojg]

Hey @MelvinBot everyone is working eagerly on getting this merged!

We merged the PR in onyx and released version 1.0.56:

• [Fix #269] use cached data react-native-onyx#280

However, in Onyx version 1.0.55 a bug has been introduced which causes a crash on mobile native.
We are trying to fix that here:

• Update Onyx to version 1.0.55 #23151

Once that's done, we can get this merged:

• [NoQA] Perf/onyx upgrade use cache directly #23749

[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]

⚠️ 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]

⚠️ 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]

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.48-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [NoQA] Perf/onyx upgrade use cache directly #23749

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

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:

• @hannojg does not require payment (Contractor)
• @mollfpr requires payment

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]

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[mountiny]

We need to pay $1000 to @mollfpr for their review and testing

[slafortune]

Offer sent @mollfpr https://www.upwork.com/nx/wm/offer/26071692

[slafortune]

Paid

[hannojg]

The original PR for this issue was reverted and the new PR can be found here:

• Reapply onyx upgrade use cache with fixes #24041

Can we reopen this issue please @slafortune ?e