Update splash screen logic by zoontek · Pull Request #6478 · Expensify/App

zoontek · 2021-11-25T16:32:30Z

Following #5620

Details

There are several issue with the current splash integrations:

Multiple UIViewController could be mounted at the same time (splash screen & growl notifications, could cause [IOS] Stuck on Splash Screen with Code Push zoontek/react-native-bootsplash#119)
bootsplash.hide() could reject, which is not handled (see iOS 15 throws "react-native-bootsplash has not been initialized" zoontek/react-native-bootsplash#264, which happen on tapping notification on iOS 15+)
It doesn't wait for NavigationContainer onReady callback to hide.
It will not hide if something happen during context providers initialisation.

Fixed Issues

#5620

Tests

QA Steps

Starts the app on Android >= 5, check if the splash screen is visible
Starts the app on iOS >= 10, check if the splash screen is visible
Try popping an alert / a native modal once the screen is loaded
Open the app using a push notification tap (on both iOS and Android)

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions · 2021-11-25T16:32:48Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

zoontek · 2021-11-25T16:48:24Z

I have read the CLA Document and I hereby sign the CLA

zoontek · 2021-11-26T11:18:17Z

android/app/src/main/res/values/colors.xml

Fix status bar being a different color than the splash screen background.

zoontek · 2021-11-26T11:18:37Z

android/app/src/main/res/values/styles.xml

android:windowLightStatusBar and android:windowLightNavigationBar are not available on Android 21 (the min version), tools:ignore silents the Android studio linter.

zoontek · 2021-11-26T11:19:19Z

src/Expensify.js

We wait for react-navigation to be ready. So there's no need to hide the splash screen here.

zoontek · 2021-11-26T11:19:31Z

src/components/ErrorBoundary/BaseErrorBoundary.js

Renamed because it's not only used for logging anymore.
Naming taken from https://github.com/bvaughn/react-error-boundary

src/components/ErrorBoundary/index.native.js

zoontek · 2021-11-26T13:13:38Z

Even if it wasn't implemented in this PR, I strongly recommend adding a default error screen (even without a CTA, just asking the user to restart the app) on error boundary catched error. Previously, if it happen during init (Onyx, etc) user might be stuck on the splash screen without any explanation.

Now the splash screen will always disappear (at least, if it's not a complete native thread crash, just a JS error), having something else than a blank screen as error boundary fallback could be nicer for the user 🙂

zoontek · 2021-11-28T13:41:08Z

Added screenshots for web / desktop where the splash screen module is a noop.

roryabraham · 2021-11-29T23:16:54Z

@zoontek You've got a merge conflict

zoontek · 2021-11-30T12:05:41Z

@roryabraham Fixed after @NikkiWines's feedback

roryabraham · 2021-11-30T22:35:29Z

I think we need to tag in @Expensify/design for a review here since you changed the appBG color.

shawnborton · 2021-11-30T22:38:32Z

It looks like the app background is now gray instead of white. The background of the right chat area should be white.

roryabraham · 2021-11-30T22:53:47Z

Even if it wasn't implemented in this PR, I strongly recommend adding a default error screen (even without a CTA, just asking the user to restart the app) on error boundary catched error.

Good suggestion @zoontek, I've created an issue to implement this suggestion.

zoontek · 2021-11-30T23:41:24Z

It looks like the app background is now gray instead of white. The background of the right chat area should be white.

Does the sign in process should also have a white background then? (It was grey, I reused this color for all screens)

I can also switch back to appBG: white for all screens or just keep the color change (currently it would be sign in in grey -> loading view in white -> authenticated app in white), but as the colors are too close, it felt a bit weird 😕

shawnborton · 2021-11-30T23:43:43Z

Let's keep all of the colors as they were before this PR was created. Thanks!

zoontek · 2021-11-30T23:44:45Z

Let's keep all of the colors as they were before this PR was created. Thanks!

No problem, I will made the change tomorrow.

zoontek · 2021-12-01T09:52:17Z

@shawnborton @roryabraham Colors are rolled back, I fixed the Podfile.lock conflict, updated the screenshots

shawnborton · 2021-12-01T16:19:28Z

Thank you!

mallenexpensify · 2021-12-04T00:51:16Z

@zoontek I think you should be compensated for reporting this above so I added a comment to this issue.. Payment will be issues via Upwork once the the issue is fixed internally or by a contributor

roryabraham · 2021-12-06T18:47:13Z

@zoontek You've got a merge conflict here

zoontek · 2021-12-07T13:04:57Z

src/App.js

As an error could happen on context provider initialization, ErrorBoundary should wrap them to hide the splash screen is something goes wrong.

If there's something that might cause an Error in these components can you point it out?

The parameters used in these components are static, so if an error is to happen it would happen all the time. Meaning if someone introduces and error they'll see it right away

If this change is accepted we should at least add a reminder doc in ErrorBoundary that certain context based functionality is not available inside

Though I would prefer to keep the error boundary access the full context (e.g. withNetwork, withPersonalDetails and others), and only relinquish it if there's a proven possibility that certain context can cause an error

react-native-onyx might have a bug, no software is perfect. To display an error fallback, it's not worth it to try (imho)

Yeah, the code in ErrorBoundary might cause an error as well, we're obviously not adding another ErrorBoundary to guard from that

In the future more context components will be created it would be easier if those are kept on the same place and on the same level

people won't wonder where to put them

people would not mistakenly put them to the incorrect place

In the feature more components will be created and some of these component would rely on context,
Adding such a component (Like a network status indicator) in the error boundary would not work for seemingly no reason

If there's a bug in the initialization of some Context it fail with a bang any way and never make it to production. Wrapping the error boundary around the context is not necessary at least the currently defined contexts

To sum it up Moving the ErrorBoundary up would work, but introduces restrictions, while it's uncertain that it will help
Not changing the Error boundary position would work as well, and it's not necessary to move it to fix the splash screen bug

I agree with @kidroca here, this change is accounting for an error that we haven't even encountered yet and it has larger implications than just catching potential initialization errors.

kidroca

Some items that I've noticed

I think addressing them would further reduce the changes in this PR

kidroca · 2021-12-07T15:13:52Z

src/App.js

If there's something that might cause an Error in these components can you point it out?

The parameters used in these components are static, so if an error is to happen it would happen all the time. Meaning if someone introduces and error they'll see it right away

If this change is accepted we should at least add a reminder doc in ErrorBoundary that certain context based functionality is not available inside

Though I would prefer to keep the error boundary access the full context (e.g. withNetwork, withPersonalDetails and others), and only relinquish it if there's a proven possibility that certain context can cause an error

src/components/ErrorBoundary/index.native.js

kidroca · 2021-12-07T15:29:57Z

src/libs/BootSplash/index.native.js

If we're no longer using show, and won't be able to use it in the next major version we might as well remove it

This would leave only a single Log call here, so we might leave it in the original place where Log is imported anyway

Also this export configuration does not match the style guide. I think it's specified somewhere, but we first define functions and then export them

kidroca · 2021-12-07T15:34:34Z

src/Expensify.js

This is the same as

setNavigationReady() { this.setState({ isNavigationReady: true }); }

When the state is already true it would just do nothing

But do we expect multiple onReady calls to be triggered anyway?

Didn't see that it was a PureComponent, so it performs shallow compare on states comparaison anyway indeed. And lie you say, it shouldn't be called more than once.

zoontek · 2021-12-07T17:07:37Z

@kidroca I've taken a few feedbacks, wrote some comments for the rest.

NikkiWines

Some small comments but overall looking pretty good

package-lock.json

src/App.js

src/Expensify.js

NikkiWines · 2021-12-07T22:58:18Z

src/App.js

I agree with @kidroca here, this change is accounting for an error that we haven't even encountered yet and it has larger implications than just catching potential initialization errors.

zoontek · 2021-12-08T09:44:24Z

@NikkiWines I've taken your feedbacks 🙂

src/Expensify.js

NikkiWines

👍 Looks good

NikkiWines · 2021-12-10T18:45:56Z

@zoontek this PR looks good but, unfortunately, the commits aren't signed so we're unable to merge this. Would you mind re-implementing the same changes in a new PR with signed commits? Sorry about that.

roryabraham · 2021-12-10T19:53:33Z

@zoontek I think it might also be possible to retroactively sign your commits via rebase: https://webdevstudios.com/2020/05/26/retroactively-sign-git-commits/

zoontek · 2021-12-11T09:44:46Z

@NikkiWines @roryabraham Thanks for the article! I signed every commit. The reviews has been dismissed, it will need your approval again (sorry about that)

NikkiWines · 2021-12-13T18:33:44Z

@zoontek looks like those commits are unverified

zoontek · 2021-12-13T18:50:39Z

@zoontek looks like those commits are unverified

This is weird, they were two days ago 🤔. Maybe because I pushed unverified commits on another repos since. I will re-perform the operation tomorrow.

kidroca · 2021-12-13T19:04:40Z

Maybe you changed your key or perhaps enabled some strict settings that makes it expire (or created a key that would expire?)

zoontek · 2021-12-13T19:40:54Z

Maybe you changed your key or perhaps enabled some strict settings that makes it expire (or created a key that would expire?)

Ah yes, I disabled it after, I didn't know it will do that.

zoontek · 2021-12-14T10:13:15Z

@NikkiWines @kidroca It's all good now, sorry about the inconvenience. It was my first time using GPG with git.

OSBotify · 2021-12-14T15:36:09Z

@zoontek, Great job getting your first Expensify/App pull request over the finish line! 🎉

I know there's a lot of information in our contributing guidelines, so here are some points to take note of 📝:

Now that your first PR has been merged, you can be hired for another issue. Once you've completed a few issues, you may be eligible to work on more than one job at a time.
Once your PR is deployed to our staging servers, it will undergo quality assurance (QA) testing. If we find that it doesn't work as expected or causes a regression, you'll be responsible for fixing it. Typically, we would revert this PR and give you another chance to create a similar PR without causing a regression.
Once your PR is deployed to production, we start a 7-day timer ⏰. After it has been on production for 7 days without causing any regressions, then we pay out the Upwork job. 💰

So it might take a while before you're paid for your work, but we typically post multiple new jobs every day, so there's plenty of opportunity. I hope you've had a positive experience contributing to this repo! 😊

OSBotify · 2021-12-14T15:36:33Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2021-12-16T01:26:23Z

🚀 Deployed to staging by @roryabraham in version: 1.1.20-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2021-12-20T12:08:37Z

🚀 Deployed to production by @Julesssss in version: 1.1.21-1 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

zoontek requested a review from a team as a code owner November 25, 2021 16:32

MelvinBot requested review from NikkiWines and removed request for a team November 25, 2021 16:32

zoontek mentioned this pull request Nov 25, 2021

[HOLD for payment 2022-02-02][$8000] - iOS - User stuck on Splash screen when launching the app #5620

Closed

zoontek commented Nov 26, 2021

View reviewed changes

zoontek changed the title ~~Update splash screen logic + Android 12 support~~ Update splash screen logic Nov 30, 2021

roryabraham assigned zoontek Nov 30, 2021

roryabraham requested a review from shawnborton November 30, 2021 22:35

roryabraham mentioned this pull request Nov 30, 2021

[HOLD for payment 2022-01-25] Create Expensify branded generic error page #6552

Closed

shawnborton previously approved these changes Dec 1, 2021

View reviewed changes

zoontek dismissed shawnborton’s stale review via 9bb43d1 December 7, 2021 12:34

zoontek commented Dec 7, 2021

View reviewed changes

kidroca suggested changes Dec 7, 2021

View reviewed changes

NikkiWines reviewed Dec 7, 2021

View reviewed changes

roryabraham previously approved these changes Dec 8, 2021

View reviewed changes

src/Expensify.js Outdated Show resolved Hide resolved

zoontek dismissed roryabraham’s stale review via 49d9200 December 9, 2021 09:13

NikkiWines previously approved these changes Dec 9, 2021

View reviewed changes

roryabraham previously approved these changes Dec 10, 2021

View reviewed changes

zoontek dismissed stale reviews from roryabraham and NikkiWines via 714e332 December 11, 2021 09:43

zoontek added 5 commits December 14, 2021 11:11

Fix splash screen implementation

4f50206

Keep some context providers for the error boundary fallback screen

0918fa0

Include feedbacks

f35d8df

Handle @NikkiWines feedbacks

a3f37f8

Add JSdoc

f62d740

roryabraham approved these changes Dec 14, 2021

View reviewed changes

roryabraham merged commit 30e66c2 into Expensify:main Dec 14, 2021

OSBotify mentioned this pull request Dec 15, 2021

Deploy Checklist: New Expensify 2021-12-15 #6778

Closed

37 tasks

parasharrajat mentioned this pull request Sep 4, 2025

[NO QA] Better RHP view v3 - Implement wide RHP functionality #69668

Merged

51 tasks

Conversation

zoontek commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

github-actions bot commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zoontek commented Nov 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zoontek commented Nov 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zoontek commented Nov 28, 2021

Uh oh!

roryabraham commented Nov 29, 2021

Uh oh!

zoontek commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roryabraham commented Nov 30, 2021

Uh oh!

shawnborton commented Nov 30, 2021

Uh oh!

roryabraham commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zoontek commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shawnborton commented Nov 30, 2021

Uh oh!

zoontek commented Nov 30, 2021

Uh oh!

zoontek commented Dec 1, 2021

Uh oh!

shawnborton commented Dec 1, 2021

Uh oh!

mallenexpensify commented Dec 4, 2021

Uh oh!

roryabraham commented Dec 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidroca left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kidroca Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

zoontek commented Nov 25, 2021 •

edited

Loading

github-actions bot commented Nov 25, 2021 •

edited

Loading

zoontek commented Nov 26, 2021 •

edited

Loading

zoontek commented Nov 30, 2021 •

edited

Loading

roryabraham commented Nov 30, 2021 •

edited

Loading

zoontek commented Nov 30, 2021 •

edited

Loading

kidroca Dec 7, 2021 •

edited

Loading

zoontek commented Dec 14, 2021 •

edited

Loading