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

[Due for payment 2025-03-17] [Due for payment 2025-03-13] [HybridApp] Improve boot splash screen #57194

Closed
AndrewGable opened this issue Feb 20, 2025 · 19 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@AndrewGable
Copy link
Contributor

AndrewGable commented Feb 20, 2025

Problem

On iOS (And possibly Android) the bootsplash when going to New Expensify via HybridApp is animating twice.

In this video it's subtle but you'll see the green start to animate, then go back to our brand green, then animate again to the New Expensify home screen.

bootsplash.mp4
1 #00CC76 2 #00D67A 3 #05D47D
Image Image Image

Solution

Only animate the bootsplash screen once no matter if you are on New Expensify or Expensify classic.

cc @Expensify/design - Just to confirm you agree with this bug

cc @war-in @mateuuszzzzz from https://github.com/Expensify/Mobile-Expensify/pull/13430

Issue OwnerCurrent Issue Owner: @JmillsExpensify
@AndrewGable AndrewGable added Bug Something is broken. Auto assigns a BugZero manager. Internal Requires API changes or must be handled by Expensify staff labels Feb 20, 2025
@AndrewGable AndrewGable self-assigned this Feb 20, 2025
Copy link

melvin-bot bot commented Feb 20, 2025

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

@melvin-bot melvin-bot bot added the Daily KSv2 label Feb 20, 2025
@shawnborton
Copy link
Contributor

10000% agree with it! I've been meaning to report this one but it doesn't always happen so it's hard to catch.

@dannymcclain
Copy link
Contributor

Definitely agree but I've also noticed this same issue when opening other apps, so I assumed this was more of an iOS thing than an our app thing.

You can see something similar here when I launch the BOA app:

boa-app-launch.mp4

Stills for reference:
Image

@war-in
Copy link
Contributor

war-in commented Feb 20, 2025

Hi 👋 Let me talk a little about the current and future HybridApp bootsplash setup

Currently, we basically have two native bootsplashes. One is used in the old app and the second one is for the NewDot. The flow looks like this (when opening the new app):

  • start OD bootsplash
  • start ND bootsplash
  • close OD bootsplash
  • close ND bootsplash
  • perform animation in ReactNative

As you can see, the bootsplash logic is duplicated which was necessary with the old Android setup (using two activities). After migration to fragments, we don't need two bootsplashes.
I managed to improve this behaviour in this draft PR and now we will only have one bootsplash controlled by both apps 🎉

Screen.Recording.2025-02-20.at.18.48.18.mov

There is only one issue left there - the RN animation.
When we're ready to show the app content (authentication passed) the SplashScreenHider is shown. At the same time, we hide the native bootsplash and when the promise resolves the animation starts. That's why we see blinking (it also happens in the standalone app) ->

Screen.Recording.Feb.20.2025.from.Marcin.Warchol.mp4

When I tried to remove the RN animation entirely there was no blinking and the bootsplash disappeared smoothly.
I think we should focus on improving the RN animation (or replacing it with the native one) to get the bootsplash experience as smooth as possible.

Please let me know what you think 🙏

@dubielzyk-expensify
Copy link
Contributor

Thanks for that detailed explanation @war-in !

When I tried to remove the RN animation entirely there was no blinking and the bootsplash disappeared smoothly.
I think we should focus on improving the RN animation (or replacing it with the native one) to get the bootsplash experience as smooth as possible.

This sounds right to me then. Are there any other alternatives in reality? When you say native do you mean code native to platform?

Ultimately I don't mind which approach we take as long as the output becomes what we want which is no flashing or noticeable changes between these screens.

@war-in
Copy link
Contributor

war-in commented Feb 21, 2025

When you say native do you mean code native to platform?

Yes, we could handle this on Mobile-Expensify side

Are there any other alternatives in reality?

I tried to find the cause of this blinking icon and it seems that simply adding setTimeout to the hide function resolves the problem. I know it's not the best solution and we probably want to find a better one but it shows that this could be related to the assets loading time.

See the comparison below

iOS

Before:

before.mov

After:

after.mov
Android

Before:

before.-.android.mov

After:

after.-.android.mov

@AndrewGable AndrewGable added Weekly KSv2 and removed Daily KSv2 labels Feb 21, 2025
@dubielzyk-expensify
Copy link
Contributor

Feels better to me. There seems to be a bit of choppyness on the iOS animation, but I'm not sure if this is a recording/simulator thing?

Also, sorry if I'm missing something but why is iOS doing a scale up + fade and Android doing just a fade?

I dunno what the right answer is on the technical side, but it feels like an improvement to me in general

@war-in
Copy link
Contributor

war-in commented Feb 26, 2025

There seems to be a bit of choppyness on the iOS animation, but I'm not sure if this is a recording/simulator thing?

@dubielzyk-expensify I would say so. It might also be due to the debug build I tested it on

why is iOS doing a scale up + fade and Android doing just a fade?

Didn't notice that earlier 🤦 I think adding a higher timeout would help because it might be due to asset loading time. But in general, they should be identical

@war-in
Copy link
Contributor

war-in commented Feb 26, 2025

@AndrewGable this issue should be resolved with those two PRs (OD, ND). For the other bug described here

There is only one issue left there - the RN animation.
When we're ready to show the app content (authentication passed) the SplashScreenHider is shown. At the same time, we hide the native bootsplash and when the promise resolves the animation starts. That's why we see blinking (it also happens in the standalone app) ->

we should create another issue to keep things separated because they're unrelated. Could we get one? 🙏

@AndrewGable
Copy link
Contributor Author

Created blink issue here!

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 6, 2025
@melvin-bot melvin-bot bot changed the title [HybridApp] Improve boot splash screen [Due for payment 2025-03-13] [HybridApp] Improve boot splash screen Mar 6, 2025
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 6, 2025
Copy link

melvin-bot bot commented Mar 6, 2025

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Mar 6, 2025

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.1.9-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2025-03-13. 🎊

For reference, here are some details about the assignees on this issue:

  • @war-in does not require payment (Contractor)

Copy link

melvin-bot bot commented Mar 6, 2025

@AndrewGable @JmillsExpensify @war-in The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

@AndrewGable
Copy link
Contributor Author

@Expensify/design - Is this looking like we expect on the TestFlight? I think I might still be seeing some colors change?

@shawnborton
Copy link
Contributor

Hmm really hard to tell, sometimes I think it does change colors - can we confirm which color values are being used for each splash?

And I am getting flash in the animation, but I think that is still expected right?

@ikevin127
Copy link
Contributor

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other:

Where bug was reported:

  • 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
  • 2b. Reported on staging (eg. found during regression or PR testing)
  • 2d. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [Contributor] 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: No specific offending PR, cause being outdated logic when it comes to OD vs ND in the context of the HybridApp - more details in #57194 (comment).

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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.

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

  1. Open the app and observe the bootsplash opening and closing.
  2. Verify that there's only 1 bootsplash opening, with 1 background color (see issue OP and #57194 (comment) for details on what to look for).
  3. Switch between apps and verify that the bootsplash behaves correctly.

Do we agree 👍 or 👎.

@ikevin127
Copy link
Contributor

@JmillsExpensify I will require payment here as C+ reviewer of PR #56953 mentioned in #57194 (comment).

🟢 Completed BZ Checklist in the above comment.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Mar 10, 2025
@melvin-bot melvin-bot bot changed the title [Due for payment 2025-03-13] [HybridApp] Improve boot splash screen [Due for payment 2025-03-17] [Due for payment 2025-03-13] [HybridApp] Improve boot splash screen Mar 10, 2025
Copy link

melvin-bot bot commented Mar 10, 2025

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.1.10-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2025-03-17. 🎊

For reference, here are some details about the assignees on this issue:

  • @war-in does not require payment (Contractor)

Copy link

melvin-bot bot commented Mar 10, 2025

@AndrewGable @JmillsExpensify @war-in The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

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. Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants