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

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

Closed
1 of 6 tasks
IuliiaHerets opened this issue Aug 8, 2024 · 74 comments
Assignees
Labels
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 Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Aug 8, 2024

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 #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?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6565130_1723080112279.20240808_091738.mp4

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
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 8, 2024
Copy link

melvin-bot bot commented Aug 8, 2024

Triggered auto assignment to @puneetlath (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.

@IuliiaHerets
Copy link
Author

We think that this bug might be related to #vip-vsb

@IuliiaHerets
Copy link
Author

@puneetlath FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@puneetlath puneetlath added the External Added to denote the issue can be worked on by a contributor label Aug 8, 2024
Copy link

melvin-bot bot commented Aug 8, 2024

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

@melvin-bot melvin-bot bot changed the title Web- User avatar & animation do not load after switching from Inbox to Settings with animation [$250] Web- User avatar & animation do not load after switching from Inbox to Settings with animation Aug 8, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 8, 2024
Copy link

melvin-bot bot commented Aug 8, 2024

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

@codewaseem
Copy link
Contributor

codewaseem commented Aug 9, 2024

It seems that this bug is caused by unnecessary state updates, leading to a much more complicated issue than it initially appears. There's an infinite re-rendering loop occurring when navigating from Settings -> Inbox -> Settings.

The top-level component is continuously re-rendering, which could be the underlying cause of performance issues and the heating problem noted in issue #36645.
Screenshot 2024-08-09 at 18 33 38

cI've been unable to pinpoint the exact cause of these unnecessary re-renders, but I suspect it mainly stems from react-native-onyx/withOnyx().
See : #47041 (comment)

@kaushiktd
Copy link
Contributor

@puneetlath after pull new code i don't reproduce this issue
screen-capture.webm

@melvin-bot melvin-bot bot added the Overdue label Aug 12, 2024
@ntdiary
Copy link
Contributor

ntdiary commented Aug 12, 2024

Interesting, I can still reproduce it 😂

test.mp4

@melvin-bot melvin-bot bot removed the Overdue label Aug 12, 2024
@ntdiary
Copy link
Contributor

ntdiary commented Aug 13, 2024

The Expensify component is rendering repeatedly, and we haven't got a complete proposal.

@JKobrynski
Copy link
Contributor

Hi, I'm Julian from Callstack - expert agency - and I would like to work on this issue.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 14, 2024
@JKobrynski
Copy link
Contributor

@codewaseem I've removed withOnyx from Expensify.tsx and used useOnyx instead and I can still reproduce the issue, I will keep digging!

@codewaseem
Copy link
Contributor

codewaseem commented Aug 14, 2024

@JKobrynski, ignore my previous comment. I think the root cause is from LottieView (lottie-react-native). I think it doesn't get cleaned up when we navigate away from the settings page. When we return back to the settings page again, it causes infinite renders. If you turn off animation by removing the autoPlay prop, it doesn't cause infinite rerenders anymore.

I tried manually starting and stopping the animation with the ref. Used setTimeout, requestIdleCallback, etc, but nothing seemed to work.

@dominictb
Copy link
Contributor

This issue come up while I'm working on another issue and I want to post a proposal on this one. Can we reapply the Help Wanted label?

@JKobrynski
Copy link
Contributor

@dominictb I applied a patch to react-native-web based on your proposal as well as the alternative solution, and none of them worked for me in this exact scenario. I think there is a different reason to it

@melvin-bot melvin-bot bot added the Overdue label Aug 16, 2024
@dominictb
Copy link
Contributor

No, I mean I figured out the root cause of this issue while working on that issue. The root cause is different, and I want to post a proposal here instead of tampering with the other issue.

@puneetlath
Copy link
Contributor

@dominictb sure, go ahead.

@melvin-bot melvin-bot bot removed Help Wanted Apply this label when an issue is open to proposals by contributors labels Sep 9, 2024
Copy link

melvin-bot bot commented Sep 9, 2024

📣 @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
Copy link
Contributor

ntdiary commented Sep 10, 2024

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. 🙂

test-render.mp4

@dominictb
Copy link
Contributor

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

@ntdiary
Copy link
Contributor

ntdiary commented Sep 25, 2024

@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
Copy link
Contributor

@puneetlath This is on HOLD for payment.

@mountiny mountiny added Daily KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review labels Oct 7, 2024
@mountiny
Copy link
Contributor

mountiny commented Oct 7, 2024

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

@puneetlath
Copy link
Contributor

Ok @ntdiary can you complete the checklist?

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

@dominictb
Copy link
Contributor

dominictb commented Oct 7, 2024

@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
Copy link
Contributor

ntdiary commented Oct 8, 2024

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:

  • [@ntdiary] The PR that introduced the bug has been identified. Link to the PR: Enable React concurrent mode #42592
  • [@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
  • [@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
  • [@ntdiary] Determine if we should create a regression test for this bug. Yes
  • [@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
Copy link
Contributor

puneetlath commented Oct 9, 2024

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
Copy link
Contributor

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

@puneetlath
Copy link
Contributor

What do you think is fair $500?

@dominictb
Copy link
Contributor

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

cc @ntdiary

@ntdiary
Copy link
Contributor

ntdiary commented Oct 10, 2024

Completely agree. :)

@puneetlath
Copy link
Contributor

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
Copy link
Contributor

dominictb commented Oct 10, 2024

@puneetlath I did, thanks 🙏

@puneetlath
Copy link
Contributor

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

@JmillsExpensify
Copy link

$500 approved for @ntdiary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 Weekly KSv2
Projects
Development

No branches or pull requests