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.33k stars 2.76k forks source link

[$250] Android - Login – Login page blinks when open app #46232

Closed lanitochka17 closed 2 weeks ago

lanitochka17 commented 1 month 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: 9.0.12-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: N/A Email or phone of affected tester (no customers):gocemate@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

  1. Sign out and kill the NewDot android app
  2. Open the NewDot app
  3. Take a note when login page opens

Expected Result:

App should not blink

Actual Result:

Login page blinks when open app

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/1cbd2549-47e8-4e5e-80ab-61f013d30d7f

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~014fc22ce9eab9144e
  • Upwork Job ID: 1819152387493807005
  • Last Price Increase: 2024-08-01
  • Automatic offers:
    • ikevin127 | Reviewer | 103430665
Issue OwnerCurrent Issue Owner: @ikevin127
melvin-bot[bot] commented 1 month ago

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

lanitochka17 commented 1 month ago

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

lanitochka17 commented 1 month ago

@kevinksullivan 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

bernhardoj commented 1 month ago

Proposal

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

Login page blinks in native.

What is the root cause of that problem?

The page blinks is caused by the sign in hand animation. In sign in hand animation, if the splash screen isn't hidden yet, we render a plain View with the same size as the lottie animation to prevent any jump because the sign in page is centered vertically.. https://github.com/Expensify/App/blob/8728fc060fa1ccb4e299bda2e25d6bdaacf38c22/src/pages/signin/SignInPageLayout/SignInHeroImage.tsx#L27-L43

It was added in https://github.com/Expensify/App/pull/35185 to improve the performance by delaying the lottie. When the splash screen is hidden, we start rendering the Lottie component. However, in Lottie component, we don't render the lottie animation immediately.

https://github.com/Expensify/App/blob/8728fc060fa1ccb4e299bda2e25d6bdaacf38c22/src/components/Lottie/index.tsx#L37-L47

The animationFile value is undefined initially and will be set in the useEffect. https://github.com/Expensify/App/blob/8728fc060fa1ccb4e299bda2e25d6bdaacf38c22/src/components/Lottie/index.tsx#L22-L26

It was added in https://github.com/Expensify/App/pull/45211 to also improve the performance by delaying the lottie. But since we render nothing for a brief moment, the page moves down and up.

Placeholder view -> null -> Lottie

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

We need to return a placeholder view too when the animationFile isn't ready to prevent any jump. We already have that here https://github.com/Expensify/App/blob/8728fc060fa1ccb4e299bda2e25d6bdaacf38c22/src/components/Lottie/index.tsx#L33-L35

so we just need to add || !animationFile to the existing condition.

I think we can also move the isSplashHidden condition to Lottie. Lmk if you agree!

What alternative solutions did you explore? (Optional)

Pass a new props to Lottie to set the animationFile immediately for sign in hand animation

OR

Wrap the Lottie with a View with the same size. https://github.com/Expensify/App/blob/8728fc060fa1ccb4e299bda2e25d6bdaacf38c22/src/pages/signin/SignInPageLayout/SignInHeroImage.tsx#L35-L43

melvin-bot[bot] commented 1 month ago

@kevinksullivan Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 month ago

@kevinksullivan Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

kevinksullivan commented 1 month ago

asking about if this fits in newdot quality here

https://expensify.slack.com/archives/C05LX9D6E07/p1722554976014829

ikevin127 commented 1 month ago

Will review today.

ikevin127 commented 1 month ago

@bernhardoj's proposal looks good to me. The root cause was correctly identified and the main solution fixes the issue.

I think we can also move the isSplashHidden condition to Lottie. Lmk if you agree!

If you're talking about the isSplashHidden check from within the SignInHeroImage component I agree, I tested this and it makes sense to check all conditions in that one if within the Lottie component. Just make sure to adjust the comment above that if to explain the newly added conditions 👍

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

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

ikevin127 commented 1 month ago

cc @MariaHCD for final assignment review when you get a chance, thank you! 🧑‍💻

melvin-bot[bot] commented 1 month ago

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

Offer link Upwork job

bernhardoj commented 1 month ago

PR is ready

cc: @ikevin127

ikevin127 commented 3 weeks ago

⚠️ Production deploy automation failed here -> this should've been paid on 2024-08-20 according to production deploy confirmed in https://github.com/Expensify/App/pull/46947#issuecomment-2289861858 and deploy checklist.

Payment was due yesterday!

cc @kevinksullivan @MariaHCD

bernhardoj commented 3 weeks ago

@kevinksullivan I think we need a payment summary here.

Requested in ND.

JmillsExpensify commented 2 weeks ago

Reporting for payment summary.

kevinksullivan commented 2 weeks ago

Summary

JmillsExpensify commented 1 week ago

$250 approved for @bernhardoj