Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
2.99k stars 2.5k forks source link

[HOLD for payment 2023-10-24] [$2000] There is a delay of 1-2 seconds when a new workspace is created in the Android app. #16935

Closed kavimuru closed 6 months ago

kavimuru commented 1 year ago

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


Action Performed:

  1. Open the "Settings" of the app.
  2. Tap on "Workspaces".
  3. Select "New workspace". Notice the app almost freezes when the "New workspace" button is pressed for couple of seconds.

Expected Result

There should not be any delay, and the app should function smoothly like web.

Actual Result

The app freezes or experiences a delay of 1-2 seconds when the "New workspace" button is pressed.

Workaround:

unknown

Platforms:

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

Version Number: 1.2.94-0 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/43996225/229895085-7d308e86-cfac-4fd2-add0-75177d0c238f.mp4

https://user-images.githubusercontent.com/43996225/229895377-88435974-047b-4a15-a0e0-60efb547e4d8.mp4

Expensify/Expensify Issue URL: Issue reported by: @usmantariq96 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1680610152510499

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0174b5420e1b80f716
  • Upwork Job ID: 1645860223341436928
  • Last Price Increase: 2023-09-21
  • Automatic offers:
    • s77rt | Reviewer | 27148524
MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

NicMendonca commented 1 year ago

@usmantariq96 I tested this on a pixel 6 pro, and my delay isn't as slow as in your video. I agree there is a 0.5 second delay, and not as quick as web, but I wonder if hardware matters here?

NicMendonca commented 1 year ago

assigning @Christinadobrzyn to watch over this while I am OOO!

usmantariq96 commented 1 year ago

I am not sure, tested it on Realme 3.

MelvinBot commented 1 year ago

📣 @usmantariq96! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
Christinadobrzyn commented 1 year ago

I can replicate this in all our platforms some of the platforms have less of a delay than others (from what I can generally tell). I wonder if this is an expected behaviour. Asking the team here - 291788

Christinadobrzyn commented 1 year ago

Puneet mentioned this might be part of a bigger API command update. Not sure if this should be closed in favour of a different GH

cc @iwiznia @flodnv

MelvinBot commented 1 year ago

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

Christinadobrzyn commented 1 year ago

Based on this convo - https://expensify.slack.com/archives/C049HHMV9SM/p1681225595834389?thread_ts=1680610152.510499&cid=C049HHMV9SM

Testing this offline has the same delayed behaviour when creating a workspace. So going to add the external label for some help on this.

MelvinBot commented 1 year ago

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue. cc @thienlnam

MelvinBot commented 1 year ago

Current assignees @NicMendonca and @Christinadobrzyn are eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Triggered auto assignment to @marcochavezf (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Current assignees @NicMendonca and @Christinadobrzyn are eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

Current assignee @thesahindia is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

Current assignee @marcochavezf is eligible for the External assigner, not assigning anyone new.

OlimpiaZurek commented 1 year ago

Hi, I'm Olimpia from Callstack - expert contributor group - I'll be happy to work on this issue.

MelvinBot commented 1 year ago

📣 @OlimpiaZurek You have been assigned to this job by @mountiny! Please apply to this job in Upwork 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 📖

OlimpiaZurek commented 1 year ago

Update: I started investigating this issue today. I did some performance profiling and found some possible improvements to speed things up. I will continue tomorrow.

MelvinBot commented 1 year ago

@marcochavezf, @OlimpiaZurek, @NicMendonca, @Christinadobrzyn, @thesahindia Whoops! This issue is 2 days overdue. Let's get this updated quick!

OlimpiaZurek commented 1 year ago

Update: I'm still investigating this issue and working on a proposal

MelvinBot commented 1 year ago

@marcochavezf @OlimpiaZurek @NicMendonca @Christinadobrzyn @thesahindia this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

OlimpiaZurek commented 1 year ago

Update: I found the root cause of this issue. Basically the problem is that creating a new workspace on button press requires a lot of calculations under the hood and causes unnecessary re-rendering to other components. I am testing a possible solutions to fix it.

OlimpiaZurek commented 1 year ago

Update: I'm finalizing the proposal . I will post it on Monday as tomorrow we are having Upskilling Day, which means my Team and I will not be working on the project.

MelvinBot commented 1 year ago

@marcochavezf, @OlimpiaZurek, @NicMendonca, @thesahindia Whoops! This issue is 2 days overdue. Let's get this updated quick!

NicMendonca commented 1 year ago

Thanks @OlimpiaZurek 👌

OlimpiaZurek commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

The app freezes for a few seconds after pressing the "New Workspace" button.

What is the root cause of that problem?

When a new workspace is being created, the Policy.createWorkspace() function is invoked, leading to the execution of two functions: SidebarUtils.getOrderedReportIDs() and SidebarUtils.getOptionData(). These functions are essential for rendering the options within the <OptionRowLHN /> component, which is utilized by the <SidebarLinks /> component. Consequently, the <OptionRowLHN /> and <SidebarLinks /> components need to be re-rendered, which results in a slight delay when clicking on the "New workspace" button.

What changes do you think we should make in order to solve the problem?

When a user clicks the “New workspace” button in the context of a <Flatlist />, the app must perform several operations, including updating the list data, re-rendering the list view, and possibly performing additional layout calculations. These operations can be particularly taxing on older and slower devices that have limited processing power and memory. Several performance improvements should be made to address this issue and make the click button work faster. First of all, I would suggest replacing FlatList with FlashList. Thanks to this, the items with<OptionRowLHN /> will render faster and smoother, and we will avoid long delays during clicking “New workspaces” button. Also to prevent unnecessary re-rendering, I would suggest adding the React.memo and React.useMemo HOCs to the components consumed by <OptionRowLHN /> and <SidebarLinks />. By reducing the number of re-renderings, we can improve the overall performance of the sidebar screen and provide a smoother user experience.

Performance when creating new workspace without Components improvements:

As you can see in the screenshots below, using FlashList can reduce the number of re-renderings and most importantly reduce the rendering time of each element. after

perf_after

Performance when creating new workspace without Components improvements:

before perf_before

What alternative solutions did you explore? (Optional)

Additionally, I would suggest refactoring these two methods SidebarUtils.getOrderedReportIDs() and SidebarUtils.getOptionData(). There seem to be relatively large and complex functions, and they have a lot of logic. I would consider simplifying the logic by breaking the functions into smaller, more manageable functions.

MelvinBot commented 1 year ago

@marcochavezf @OlimpiaZurek @NicMendonca @thesahindia this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

thesahindia commented 1 year ago

Hi, I will be away for one or two days. I've made a post on the channel to check if anyone else would be willing to take care of this.

s77rt commented 1 year ago

I will take it!

MelvinBot commented 1 year ago

📣 @s77rt You have been assigned to this job by @marcochavezf! Please apply to this job in Upwork 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 📖

marcochavezf commented 1 year ago

Assigning @s77rt as C+

s77rt commented 1 year ago

@marcochavezf Thank you!

s77rt commented 1 year ago

@OlimpiaZurek Thanks for the proposal. I will review asap!

OlimpiaZurek commented 1 year ago

I will be off for a few days (due to public holidays in Poland). I'll be back on Thursday and continue working on this issue.

s77rt commented 1 year ago

@OlimpiaZurek Thanks for the proposal. Can you please elaborate on how SidebarUtils.getOrderedReportIDs() and SidebarUtils.getOptionData() are the offending functions?

Although your RCA makes sense in general, I'm not sure it's complete. There is indeed a lot of work going on when re-rendering components but the delay is very noticeable only on the workspace creation button. Something I found interesting and worth investigating is the behavior of react-native-onyx, onyx method to be specific. If you change the onyx method from SET to MERGE on the optimisticData you will get a faster feedback. Looks that onyx set method is blocking while onyx merge method is not. I'm not sure if any should be blocking/non-blocking, so I'm gonna ask on Slack.

Of course even if we make SET a non-blocking method, the re-rendering time would still be the same but the user may not notice the huge lag.

So I guess the plan is:

  1. Ask whether onyx set method should be non-blocking (or if merge is non-blocking by accident) (Slack thread)
  2. Try to reduce the number of re-renders and/or render time
s77rt commented 1 year ago

Melvin, it's not overdue, I was holding to post till Monday to get some :eyes: on the post. Today is Monday (and 1st May), Slack looks like :desert: yet I posted :grin:. https://expensify.slack.com/archives/C01GTK53T8Q/p1682938682704609

marcochavezf commented 1 year ago

I believe the remaining items we need to address are:

As for the FlashList, I remember Margelo is working on a new list called WishList, if I recall correctly. So, I'm unsure if replacing FlashList right now would be the best long-term strategy.

Is there a way to resolve this issue without replacing the FlashList?

When a user clicks the “New workspace” button in the context of a , the app must perform several operations, including updating the list data, re-rendering the list view, and possibly performing additional layout calculations.

MelvinBot commented 1 year ago

@marcochavezf @s77rt @OlimpiaZurek @NicMendonca this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

MelvinBot commented 1 year ago

Current assignee @s77rt is eligible for the Internal assigner, not assigning anyone new.

MelvinBot commented 1 year ago

@marcochavezf @s77rt @OlimpiaZurek @NicMendonca this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks!

s77rt commented 1 year ago

@marcochavezf Please reapply External label.

MelvinBot commented 1 year ago

Current assignee @NicMendonca is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

Current assignee @s77rt is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

Current assignee @marcochavezf is eligible for the External assigner, not assigning anyone new.

marcochavezf commented 1 year ago

Changed to External

OlimpiaZurek commented 1 year ago
  1. At first glance, these functions seem to do a significant amount of work, including multiple calls to other functions and libraries, iterations, and conditional logic. In addition, these functions seem to do most of the text processing, including concatenation, translation, and formatting. If the input is very large and the functions are called frequently, this can cause significant slowdowns.
    Since functions are called frequently, you might want to consider memoizing the results to avoid redundant calculations. Finally, it is possible that some of the lodash functions used in this function can be replaced with native JavaScript equivalents to improve performance.
    I considered refactoring these two functions as optional and complementary to my main suggestion which is to reduce the number of re-renders and reduce the render time. Replacing FlatList with FlashList or change SET to MERGE method seem to be enough to resolve this issue .

  2. I replaced SET with onyx's MERGED method and indeed the feedback is faster and the latency is not noticeable. But there is one thing to consider. After changing this method, I noticed one thing that can cause regression. After adding a new workspace and going to the workspace screen, we see the Page not found screen for a few seconds because the policy is an empty object before the data arrives.

https://user-images.githubusercontent.com/19835085/236212169-73caf919-ff21-4168-87fa-90e28369959e.mov

We could fix this by adding a loading indicator while waiting for data. This would be a better solution from the user experience point of view than saying that the page was not found.

  1. In my opinion there is no other good way to reduce number of re-renders and reduce the render time without replacing the FlatList with Flashlist.
    We could solve this issue by replacing SET with the MERGE method, but as I mentioned this causes a small problem described in point 2.
s77rt commented 1 year ago

@OlimpiaZurek Can we go with the MERGE solution? How can we know if the workspace is loading or does not exist?