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
3.52k stars 2.87k forks source link

[$250] Web- User avatar & animation do not load after switching from Inbox to Settings with animation #47041

Closed IuliiaHerets closed 3 weeks ago

IuliiaHerets commented 2 months 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!


Version Number: v9.0.18-1 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Exp https://github.com/Expensify/App/pull/46885 Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to Account settings.
  3. Go to any page with animation in the header (Preferences/Security/Troubleshoot/About/Save the world).
  4. Go to Inbox.
  5. Switch back to Account settings.

Expected Result:

User avatar and the animation will load after switching from Inbox to Settings with animation.

Actual Result:

User avatar and the animation do not load after switching from Inbox to Settings with animation.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/d7a16a4f-9317-4ade-a06e-0862087eefc6

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01721123276553a8a9
  • Upwork Job ID: 1821551089457405095
  • Last Price Increase: 2024-08-24
  • Automatic offers:
    • dominictb | Contributor | 103887884
Issue OwnerCurrent Issue Owner: @ntdiary
muttmuure commented 2 months ago

@dominictb are you and @ntdiary working on this one?

ntdiary commented 2 months ago

@dominictb are you and @ntdiary working on this one?

Uh, I thought we hadn't reached an agreement on the final ideal solution yet. However, @j-piasecki, I just noticed that you added a πŸ‘ to this comment, does that mean you agree with their idea as well? If so, I think we can assign the issue to @dominictb, and then they can raise a PR. πŸ™‚

So, in general, I think the Lottie animation get entangled with the React concurrent rendering algorithm and cause the app to freeze due to infinite re-renders, but the root cause is still in the way FreezeWrapper works causing lots of unnecessary re-rendering, and my solution would not only improve the overall performance but also avoid any extra debugging and fixing efforts in case there's some other animation library messing up with React concurrent rendering algorithm.

cc @puneetlath @muttmuure

ntdiary commented 2 months ago

Didn't receive any new input. @puneetlath, can we assign the issue to @dominictb to move forward with a PR? πŸ™‚

melvin-bot[bot] commented 2 months ago

@puneetlath @ntdiary @JKobrynski this issue is now 4 weeks old, please consider:

Thanks!

j-piasecki commented 2 months ago

Uh, I thought we hadn't reached an agreement on the final ideal solution yet. However, @j-piasecki, I just noticed that you added a πŸ‘ to this https://github.com/Expensify/App/issues/47041#issuecomment-2321516443, does that mean you agree with their idea as well? If so, I think we can assign the issue to @dominictb, and then they can raise a PR. πŸ™‚

Sorry, I was at React Universe Conf and completely missed the notification.

I think that it makes sense to go that way to fix this problem in production, while we continue to debug the root cause of it. It would be great to understand why lottie causes this behavior but at the same time reducing number of renders (potentially across the entire app since FreezeWrapper is used for every screen) is very desirable. That's assuming the benchmarks are correct as I haven't repeated them.

dominictb commented 1 month ago

I've only observed 25 renderings here

@ntdiary can you share your test step around this? Thank you!

melvin-bot[bot] commented 1 month ago

πŸ“£ @dominictb πŸŽ‰ 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 πŸ“–

ntdiary commented 1 month ago

I've only observed 25 renderings here

@ntdiary can you share your test step around this? Thank you!

@dominictb, In the latest code, it seems that the render count for InitialSettingsPage has decreased, but don't worry, this issue can still be reproduced. Please feel free to use useLayoutEffect to raise a PR, so that we can fix this issue on the web platform. πŸ™‚

https://github.com/user-attachments/assets/28e98a78-2813-4a86-bfdf-ee026ffed5bf

dominictb commented 1 month ago

@puneetlath @ntdiary this was on production 5 days ago.

ntdiary commented 1 month ago

@puneetlath @ntdiary this was on production 5 days ago.

@dominictb, yeah, we generally have a 7-day regression period after deploying to production, and the Melvin automation here seems to have failed. BTW, @puneetlath, curious is it possible to increase the bounty for this issue? I feel it's a relatively tough one. πŸ˜„ After it was posted on the 8th, it took several attempts before a seemingly valid proposal emerged on the 18th, and then, after two weeks of extensive digging and discussion, we finally arrived at a relatively good RCA and solution. πŸ˜‚

dominictb commented 1 month ago

@puneetlath This is on HOLD for payment.

mountiny commented 4 weeks ago

I believe we just need to pay this one out from the latest convo

puneetlath commented 4 weeks ago

Ok @ntdiary can you complete the checklist?

And @dominictb can you accept the upwork offer? https://www.upwork.com/nx/wm/offer/103887884

dominictb commented 4 weeks ago

@puneetlath Curious what's your thought on this comment?

BTW, @puneetlath, curious is it possible to increase the bounty for this issue? I feel it's a relatively tough one. πŸ˜„ After it was posted on the 8th, it took several attempts before a seemingly valid proposal emerged on the 18th, and then, after two weeks of extensive digging and discussion, we finally arrived at a relatively good RCA and solution. πŸ˜‚

cc @ntdiary

ntdiary commented 4 weeks ago

Ok @ntdiary can you complete the checklist?

@puneetlath, are you referring to the BugZero checklist? It seems like it hasn't been posted recently due to melvin's failure. πŸ˜‚

BugZero Checklist: The PR fixing this issue has been merged! The following checklist will need to be completed before the issue can be closed:

  • [x] [@ntdiary] The PR that introduced the bug has been identified. Link to the PR: #42592
  • [x] [@ntdiary] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/42592/files#r1791959554
  • [x] [@ntdiary] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
  • [x] [@ntdiary] Determine if we should create a regression test for this bug. Yes
  • [x] [@ntdiary] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again. As below
  • [ ] [@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

The testing steps are simple, it's fine to create a regression test. :)

Regression Test steps

  1. Go to staging.new.expensify.com
  2. Go to Account settings.
  3. Go to any page with animation in the header (Preferences/Security/Troubleshoot/About/Save the world).
  4. Go to Inbox.
  5. Switch back to Account settings.
  6. Verify avatar and animation can be displayed normally.
puneetlath commented 3 weeks ago

Regression test issue: https://github.com/Expensify/Expensify/issues/434734

@dominictb bump, can you accept the offer? https://www.upwork.com/nx/wm/offer/103887884. Please ping me here when you have.

Payment summary:

Thanks everyone!

dominictb commented 3 weeks ago

@puneetlath Could you have a look at this comment? Thanks!

puneetlath commented 3 weeks ago

What do you think is fair $500?

dominictb commented 3 weeks ago

@puneetlath Yes I agree that would be fair given the effort here

cc @ntdiary

ntdiary commented 3 weeks ago

Completely agree. :)

puneetlath commented 3 weeks ago

Ok sounds good. I reviewed the thread and it seems fair to me too. I've updated the payment summary.

@dominictb please accept the new offer and ping me here when you have: https://www.upwork.com/nx/wm/offer/104356817

dominictb commented 3 weeks ago

@puneetlath I did, thanks πŸ™

puneetlath commented 3 weeks ago

Ok good to go. @ntdiary request on NewDot please. Thanks y'all!

JmillsExpensify commented 3 weeks ago

$500 approved for @ntdiary