Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-09-11] [$250] Opening a report from Search panel is not smooth #47724

Closed
kacper-mikolajczak opened this issue Aug 20, 2024 · 10 comments
Assignees
Labels
AutoAssignerNewDotQuality Used to assign quality issues to engineers Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kacper-mikolajczak
Copy link
Contributor

kacper-mikolajczak commented Aug 20, 2024

Problem

Transition between Search panel and a Report screen is not smooth. After user selects a report, it causes:

[iOS] Long rendering process (~270ms) [Web] Noticeable jittering of animation
open-report-from-finder.with-timing.baseline.ios.mov
open-report-from-finder.baseline.web.mov

We would like to have app as responsive to user input as possible, while currently it creates a noticeable delay or jittering.

Testing setup:

Reproduction steps:

  1. Open Search panel (aka ChatFinder)
  2. Search for empty report
  3. Wait until network request finishes (spinner is gone)
  4. Open empty report

Solution

Initial investigations show that the potential villain is a piece of selectReport callback in ChatFinderPage component:

updateSearchValue('');
Navigation.dismissModal(option.reportID);

The dismissModal helper calls originalDismissModalWithReport, which in turn calls getStateFromPath.

On a regular HT account, single call to getStateFromPath takes about 20ms to complete on the web and about 40ms to complete on iOS.

The getStateFromPath internally calls original getStateFromPath implementation from React Navigation, which is quite complex. It may lead to extensive computation resulting in low performance.

The Navigation.dimissModal is called many time across the codebase (75 usages in 52 files).

The implementation of this proposal would aim to improve dimissModal performance (also targeting additional improvements like reduction of unnecessary re-renders).

Potential gains (defining an upper-bound)

In order to prove it is indeed responsible for low-quality performance during transition, dimissModal was replaced with regular call to navigate method and unnecessary updateSearchValue call was removed entirely:

Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(option.reportID));

Keep in mind, this change is not a ready solution (it changes how navigation works) and is here to define how much we could gain after implementing actual solution.

The results of proposed change are as follows:

[iOS] Rendering time reduced by ~15% [Web] No jittery animation
open-report-from-finder.with-timing.upperbound.ios.mov
open-report-from-finder.upperbound.web.mov

Profiling web application shows promising results:

  • ~1050ms time of scripting reduced to ~400ms (~60% reduction)
  • big chunk of RAM was released (possibly GC - further measurements are needed to prove)
[Web] Before [Web] After
open-report-from-finder baseline js web )open-report-from-finder upperbound js web

PR with changes: #47726

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021833637723306135397
  • Upwork Job ID: 1833637723306135397
  • Last Price Increase: 2024-09-10
  • Automatic offers:
    • DylanDylann | Contributor | 103906105
Issue OwnerCurrent Issue Owner: @anmurali
@melvin-bot melvin-bot bot added Monthly KSv2 Reviewing Has a PR in review Weekly KSv2 and removed Monthly KSv2 labels Aug 23, 2024
@mountiny mountiny added the AutoAssignerNewDotQuality Used to assign quality issues to engineers label Aug 26, 2024
Copy link

melvin-bot bot commented Aug 26, 2024

Current assignee @mountiny is eligible for the AutoAssignerNewDotQuality assigner, not assigning anyone new.

@DylanDylann
Copy link
Contributor

@mountiny Could you please process the payment for this issue?

@mountiny mountiny added Daily KSv2 Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. and removed Weekly KSv2 labels Sep 10, 2024
@mountiny mountiny added the External Added to denote the issue can be worked on by a contributor label Sep 10, 2024
Copy link

melvin-bot bot commented Sep 10, 2024

Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

Copy link

melvin-bot bot commented Sep 10, 2024

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

@melvin-bot melvin-bot bot changed the title Opening a report from Search panel is not smooth [$250] Opening a report from Search panel is not smooth Sep 10, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 10, 2024
Copy link

melvin-bot bot commented Sep 10, 2024

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

@mountiny mountiny assigned DylanDylann and unassigned ntdiary Sep 10, 2024
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 10, 2024
Copy link

melvin-bot bot commented Sep 10, 2024

📣 @DylanDylann 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@mountiny
Copy link
Contributor

@anmurali can you please pay $250 to @DylanDylann for their help with this PR? Thanks!

@mountiny mountiny changed the title [$250] Opening a report from Search panel is not smooth [HOLD for payment 2024-09-11] [$250] Opening a report from Search panel is not smooth Sep 10, 2024
@melvin-bot melvin-bot bot added Daily KSv2 and removed Daily KSv2 labels Sep 17, 2024
@DylanDylann
Copy link
Contributor

@anmurali Please help process payment for me

@mountiny mountiny removed the Reviewing Has a PR in review label Sep 24, 2024
@melvin-bot melvin-bot bot added the Overdue label Sep 24, 2024
Copy link

melvin-bot bot commented Sep 24, 2024

@anmurali, @mountiny, @kacper-mikolajczak, @DylanDylann Still overdue 6 days?! Let's take care of this!

@anmurali
Copy link

Done.

@melvin-bot melvin-bot bot removed the Overdue label Sep 25, 2024
@github-project-automation github-project-automation bot moved this from LOW to Done in [#whatsnext] #quality Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoAssignerNewDotQuality Used to assign quality issues to engineers Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Status: Done
Development

No branches or pull requests

5 participants