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

isagoico commented 3 years 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!

Action Performed:

Force close the app or update the app
Open the app

Expected Result:

User should not be stuck on Splash screen

Actual Result:

User is stuck in splash screen

Workaround:

Closing and reopening app fixes the issue.

Platform:

Where is this issue occurring?

iOS

Version Number: 1.1.2-0

Reproducible in staging?: Yes Reproducible in production?: Unknown

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation There's no solid reproduction steps but from the testers reproductions it has been after updating the app.

Expensify/Expensify Issue URL:

Issue reported by: @puneetlath Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1632958388281500

View all open jobs on GitHub

sidferreira commented 2 years ago

@NikkiWines As mentioned somewhere else, we can use a branch + testflight build for this, getting only some users to use it and have the minimum data at it.

zoontek commented 2 years ago

Hello 👋

react-native-bootsplash author here. I dug a bit in your codebase, tried a few things and think I understand what's going on here. I could propose a solution.

sidferreira commented 2 years ago

@zoontek So you think the problem may be the splash it self?!

zoontek commented 2 years ago

@sidferreira Not really. It's a combination of the splash screen and your UpdateModal. A known issue with multiples UIViewController

sidferreira commented 2 years ago

@zoontek 😮really curious about this!

zoontek commented 2 years ago

I can write a rock solid implementation that solve your issue with update to react-native-bootsplash v4 and Android 12 support if needed 🙂

sidferreira commented 2 years ago

@zoontek TBH an issue like that explains why it happens only in some cases... native flakiness sometimes is a b**** :)

zoontek commented 2 years ago

It's much more a race condition issue, but I agree 😅

But when you understand what's going on, it's easily fixable

kidroca commented 2 years ago

Not really. It's a combination of the splash screen and your UpdateModal. A known issue with multiples UIViewController

I don't think this is the problem - <UpdateAppModal /> won't be shown on iOS - no OTA updates on that platform

props.updateAvailable is following ONYXKEYS.UPDATE_AVAILABLE https://github.com/Expensify/App/blob/f866f8b437994bdab608a66e1ba9594210bec7f2/src/Expensify.js#L162-L164
it's value is updated only by triggerUpdateAvailable https://github.com/Expensify/App/blob/f866f8b437994bdab608a66e1ba9594210bec7f2/src/libs/actions/App.js#L82-L84
and triggerUpdateAvailable is only called from Web/Desktop code: https://github.com/Expensify/App/blob/f866f8b437994bdab608a66e1ba9594210bec7f2/src/libs/Notification/LocalNotification/BrowserNotifications.js#L126-L134
there are no local notifications implemented for mobile: https://github.com/Expensify/App/blob/f866f8b437994bdab608a66e1ba9594210bec7f2/src/libs/Notification/LocalNotification/index.native.js#L1-L5

kidroca commented 2 years ago

Maybe the ticket description here should be updated

There's no solid reproduction steps but from the testers reproductions it has been after updating the app.

This means it happens after the app is updated through TestFlight and opened again

zoontek commented 2 years ago

Maybe the ticket description here should be updated

There's no solid reproduction steps but from the testers reproductions it has been after updating the app.

This means it happens after the app is updated through TestFlight and opened again

Does TestFlight open the app automatically after an update? Does this happen only on iOS 15+?

kidroca commented 2 years ago

It can still be a problem of multiples UIViewController - we might be displaying other popups that use UIViewController For example during development I've seen "Please reauthenticate" Growl/toast being shown to me when automatic re-authentication fails during re-launch

Does TestFlight open the app automatically after an update?

I don't think so. Some people would even kill the app if it's running then update and launch it It was reported that this happens even without an update, something like:

Use the app and kill it
Launch the app 2hrs later
You're stuck
Kill the app again
It's fixed

Does this happen only on iOS 15+?

I don't know, but I think the splash screen was added since iOS 15 was released so most people reporting it would be on iOS 15

NikkiWines commented 2 years ago

Hi @zoontek thanks for taking a look at this issue

Does TestFlight open the app automatically after an update?

No, the user has to manually open the app

Does this happen only on iOS 15+?

Hmmm, I'm not sure. This issue was opened after iOS 15 was released. @isagoico do you know what version the iPhones used for regression testing are on?

sidferreira commented 2 years ago

@kidroca I had no idea one had to kill the app to cause the issue... Tim told me that he would just let it rest...

zoontek commented 2 years ago

It can still be a problem of multiples UIViewController - we might be displaying other popups that use UIViewController For example during development I've seen "Please reauthenticate" Growl/toast being shown to me when automatic re-authentication fails during re-launch

I removed the usage of UIViewController to stick to RCTRootView.loadingView exclusively in v4 exactly because of this: most of the users don't wait until the hide Promise is resolved to show an alert / a modal / other (https://github.com/zoontek/react-native-bootsplash/releases/tag/4.0.0)

IMHO, what should be done:

Switch to v4
At start, do Onyx migrations, wait for NavigationContainer onReady to hide splash screen.
Once hide promise is resolved, mount what can trigger a UIViewController appearance (even if it should not cause an issue with v4, it's not nice to display them at the same time of the splash screen).
When switching from sign in flow to authenticated one, display a loading view (not a splash screen!) above the app.

kidroca commented 2 years ago

@kidroca I had no idea one had to kill the app to cause the issue... Tim told me that he would just let it rest...

It seems necessary to kill the app, I don't see how the Splash screen would become visible again otherwise We would only show the splash screen again as part of a transition from the Login Screen when you transition from no token to some token - a splash screen would be shown until your initial storage data is loaded

Since this can only happen when you input your password and login I assumed it's not the case The splash screen stuck situation is happening when you're already logged in (from last usage) and open the app again

But since I couldn't recreate it so far, I might be wrong about these rationales

@zoontek just mention that we shouldn't use the splash screen for this and I agree

When switching from sign in flow to authenticated one, display a loading view (not a splash screen!) above the app.

NikkiWines commented 2 years ago

@kidroca I had no idea one had to kill the app to cause the issue... Tim told me that he would just let it rest...

I believe some people have been able to reproduce this issue just by opening the New Expensify app, using other apps throughout the day, and then coming back to the New Expensify app. A more consistent - but still relatively inconsistent - reproduction method has been to try to open the app after test flight updates or by force closing the app and re-opening it.

When switching from sign in flow to authenticated one, display a loading view (not a splash screen!) above the app.

I agree that's a good modification to make - I wonder if we'd want to make the loading view look like the splash screen or if we'd want some alternate design altogether. Probably the former.

@zoontek, I reached out to our QA team about whether this issue was reproducable across issues and they came back with the following:

Heyo! I'm pretty sure we were able to reproduce this on iOS 15 and iOS 13. We haven't been able to reproduce this in a while tho.

I'll ask in our #expensify-open-source slack channel to see if Expensify employees and/or contributors are still experiencing this bug.

zoontek commented 2 years ago

@NikkiWines Thanks for the informations! I will make a proposal as a pull request with full explanations about how to improve the splash screen experience anyway. Feel free to have a look / approve it / reject it when it will land (ou could also create a specific build to test it using TestFlight 🙂)

Santhosh-Sellavel commented 2 years ago

@NikkiWines

Steps to Reproduce

Close the app
From another account send a message to the account logged in to the iOS Device!
Now open the app by tapping Push Notification received
Stuck on Splash screen
Until close and reopen the app again. To use the app

Video Added here

kidroca commented 2 years ago

Hey guys, I think I've found out what's going on. It's related to the multiple authentication calls and it's all tight to the code here:

https://github.com/Expensify/App/blob/2b7d6d0a4171319a9328dca14bcd5023a5352a94/src/Expensify.js#L106-L116

Recently I ended up on the Login Screen (Desktop) even though I was logged in the previous time I used the app I inspected the logs and I saw multiple reauthentication calls - 2 of them failed When re-authentication fails we're redirected to login - which clears storage - session.authToken becomes null But if the other re-authentication request succeeds (after storage was cleared) session.authToken is assigned again

Which would cause the splash screen to appear again

        const previousAuthToken = lodashGet(prevProps, 'session.authToken', null);
        if (this.getAuthToken() && !previousAuthToken) {
            BootSplash.show({fade: true});
        }

And then another reauthenticate request fails setting the authToken to null again - now there's nothing that would cause the splash screen to go away

I've presented evidence of multiple re-authenticate requests running in parallel here: https://github.com/Expensify/App/issues/5620#issuecomment-971591226 So far everytime the token expires I see multiple authentication requests being made simultaneously

The logs added in: https://github.com/Expensify/App/issues/6358 should be able to prove this. We should be looking for something like:

[hmmm] Redirecting to Sign In because we failed to reauthenticate - {"command":"Get","error":"404 No user with that partner/userID"}
[info] [BootSplash] showing splash screen - ""
[info] [BootSplash] Splash screen status - {"status":"visible"}
[alert] [BootSplash] Still visible. Current <Expensify /> props - "..."

We should make sure there's only one reauthenticate call, or if there are multiple calls we handle them in a way that doesn't create this error

We're using this prop transition to represent loading while new content is being loaded

        if (this.getAuthToken() && !previousAuthToken) {
            BootSplash.show({fade: true});
        }

We should maybe also add the reverse

        if (previousAuthToken && !this.getAuthToken()) {
            this.hideSplash();
        }

Or just

        if (!this.getAuthToken()) {
            this.hideSplash();
        }

So that if we "lost" the token while the boot splash is visible, we hide the splash instead of being stuck

I guess it might be better to capture state representing this

if (this.props.appInitializing && !this.state.splashShown) {
  this.showSplash();
}

if (!this.props.appInitializing && this.state.splashShown) {
  this.hideSplash();
}

Where appInitializing is controlled from external factors like initialReportDataLoaded and isSidebarLoaded and our authenticated state

zoontek commented 2 years ago

I published the PR here: https://github.com/Expensify/App/pull/6478

It updates the lib, add Android 12 support, probably fix the issue described by @Santhosh-Sellavel (which is this one: https://github.com/zoontek/react-native-bootsplash/issues/264). It also should be fine with alerts, modal and multiple hide calls since the iOS part 1) Doesn't uses UIViewController anymore 2) Doesn't reject promise anymore (which was unhandled in your app) - instead promises are pushed in a FIFO queue and resolved in order

I also removed the show splash screen call at the sign in -> authenticated transition. A loader is shown, I had to standardize app background a bit to avoid white / gray1 blinkings

EDIT: I added a bunch of comments so it's easier to understand the pull request EDIT 2: I cleaned up the PR commit history. It was a bit of a mess since I took a bit of time understanding the codebase (I'm used to using TS + hooks)

NikkiWines commented 2 years ago

@Santhosh-Sellavel thanks for submitting your reproduction steps here. I was also able to reproduce the issue on my device by following those steps. Sadly, these reproduction steps won't work for simulators (for now). I'll work with our team to determine compensation for finding reliable reproduction steps for this issue.

@kidroca, thanks for your investigation! I'll review https://github.com/Expensify/App/pull/6476 today so that we can get some logs + confirmation around what is happening.

@zoontek, thanks for submitting a PR! I'll look at your changes today as well.

NikkiWines commented 2 years ago

@zoontek sorry, I realized that there was never a reviewed proposal for this issue. Our process is to review proposals, then hire people via UpWork, then get started on PRs - sorry for not communicating that earlier.

Could you write up a brief proposal on your suggested changes and how they'll resolve this issue?

Since you already have a PR up, I gave it a quick look. One small note, as this bug is iOS only, I don't think we'll want to add support for Android 12 at this time. It's definitely something we should do at some point, but it's not integrally tied to this problem.

zoontek commented 2 years ago

@NikkiWines Ok, I added Android 12 support because react-native-bootsplash needs it and I wanted to upgrade it since I know (obviously) that it's a cleaner solution on iOS 😄

I rolled back these changes, updated to the latest v3.x version.

My proposed changes:

Catch and ignore bootsplash hide promise rejections as they occur when the splash screen is not "mounted" (aka the RCTRootView instance is nil - on iOS >= 15, when opening the app with a tap on a notification).
Wrap ComposeProviders in the main ErrorBoundary, hide the splash screen if the app crash (there is no fallback screen for now, I don't have the design, but at least the user will not be stuck on it)
Wait for react-navigation onReady callback to start hiding the splash screen. When it's done, update the component state and mount GrowlNotification / UpdateAppModal
Remove the need for Bootsplash.show, uniformize the app background colors to have a smooth transition from signin to authenticated flow using a FullScreenLoadingIndicator

They already are all implemented in the PR.

NikkiWines commented 2 years ago

@zoontek, thanks for the proposal. I think what you're saying looks fine (though it might be nice to have a fallback screen). Since we'd be modifying the splash screen hiding logic by waiting for the onReady callback, it might be wise for us to wait for @kidroca's PR (which is adding logs) to confirm some suspicions about the multiple authentication requests being root of this issue, before moving ahead with your suggested changes.

Other than that, I think this looks good. cc: @roryabraham since you took a quick look at @zoontek's PR as well.

roryabraham commented 2 years ago

Okay, just a few notes here:

https://github.com/Expensify/App/issues/5620#issuecomment-978569325

@Santhosh-Sellavel This isn't reproducing the problem for me (though (separately) some regression was introduced at some point since this PR was deployed, such that it's no longer opening to the correct chat). I'm not on iOS 15 yet, so that's probably related to why the reproduction steps are not consistent for me. That said, I have experienced the issue where the app gets stuck on the splash screen.

So that leads me conclude that there are (at least) two separate cases when the iOS app can get stuck on the splash screen:

When opening from a deeplink (push notification) on iOS 15
As a result of some other race condition. It sounds like @kidroca may have identified the root cause of this problem being parallel authentication calls, but we're not 100% sure. Is that correct?

So given that background, here's my analysis of @zoontek's proposal:

Catch and ignore bootsplash hide promise rejections as they occur when the splash screen is not "mounted" (aka the RCTRootView instance is nil - on iOS >= 15, when opening the app with a tap on a notification).

It sounds like this might resolve the case where we open the app via deeplink on iOS 15, so that's great! @zoontek Any additional context as to why would be appreciated 😄

Wrap ComposeProviders in the main ErrorBoundary, hide the splash screen if the app crash (there is no fallback screen for now, I don't have the design, but at least the user will not be stuck on it)

This seems like a good idea/best practice, though I'm unsure of it's value unless we implement a fallback screen – we'll just land on a white screen instead of the splash screen in the event of a JS crash. I did create an issue to get a design for a generic error screen from designer extraordinaire @megankelso. We can include this proposed change so that, once we're ready with the design, we can easily implement that screen and drop it in the ErrorBoundary.

I also should acknowledge that I'm not very familiar with the context providers introduced in this PR. @kidroca do you know if it's safe to move the ErrorBoundary outside of those? I think so, but would be good to double-check since I'm sure you're more familiar with the context for why those were introduced and how they work.

Wait for react-navigation onReady callback to start hiding the splash screen. When it's done, update the component state and mount GrowlNotification / UpdateAppModal

This also seems like a good idea, but I'm unsure how it's relevant. @zoontek can you please provide some additional context as to why you're proposing that change?

P.S: From personal experience I'm hesitant to trust react-navigation's onReady callback outright, see this issue for some context we should be wary of. It might be fine in this case.

Remove the need for Bootsplash.show

If I'm not mistaken, this should actually (at least partially) solve the issue that @kidroca identified with our authentication race conditions, and prevent us getting stuck on the BootSplash screen. In the case that @kidroca identified, I'd imagine that instead of being stuck in the splash screen, we would be booted to the login screen, which is a definite improvement.

So overall it seems like @zoontek's proposal will solve the two main cases where we get stuck on the splash screen. I'm giving a preliminary 👍 to @zoontek's proposal, provided that we get clarity on the few points listed above.

NikkiWines commented 2 years ago

Thanks for the notes @roryabraham 🙌

@Santhosh-Sellavel This isn't reproducing the problem for me (though (separately) some regression was introduced at some point since this PR was deployed, such that it's no longer opening to the correct chat). I'm not on iOS 15 yet, so that's probably related to why the reproduction steps are not consistent for me. That said, I have experienced the issue where the app gets stuck on the splash screen.

I've asked our QA team to test this on both iOS15 and iOS14 to see if it's version-specific, but it does look like these steps don't work consistently across the board 😞

Santhosh-Sellavel commented 2 years ago

Thanks, @NikkiWines & @roryabraham

I still can consistently produce on my personal devices (iOS 15).

I'm not sure there may be different causes other than this one in iOS 14 (I'm not sure here). But sure there is another cause here in iOS 15 causing the same issue.

Step as I mentioned here https://github.com/Expensify/App/issues/5620#issuecomment-978569325

Even when I tap the app Icon after the notification is received, I am getting stuck on Splash screen.

iPhone

https://user-images.githubusercontent.com/85645967/144498184-1ccd9da2-2d65-45e7-87f0-250f4ad23a94.MP4

iPad

https://user-images.githubusercontent.com/85645967/144498245-a3bd67dd-c9a4-471b-b1b6-f687fee0a6ca.mov

NikkiWines commented 2 years ago

@Santhosh-Sellavel, the QA team was unable to reproduce the issue using your steps on either iOS 14.2 or iOS 15.0.1. Similarly, I am now unable to use those steps to reproduce the issue on 15.1.1

https://user-images.githubusercontent.com/16747822/144519790-cefd6076-ccbd-405d-97df-c5a50284ae5f.mov

Given that our QA teasm, @roryabraham, and myself cannot reliably use these steps to reproduce the issue, I don't think we can count these as viable reproduction steps. Sorry about that!

kidroca commented 2 years ago

I think one path that is still unexplored is opening from a push notification after your token had expired Like receive a message 2hrs after you last opened the app and the pressing it to open the app

BTW I was able to trigger some fake push notifications on a simulator using:

$ xcrun simctl push <device> com.example.my-app ExamplePush.apns

https://betterprogramming.pub/how-to-send-push-notifications-to-the-ios-simulator-2988092ba931

But couldn't recreate either

@roryabraham Moving the ErrorBoundary is safe, but I don't think we should do it - we won't be able to use SafeArea context (Modals) and LocaleContext (translations) in the content we render from the ErrorBoundary The root level context should remain at the root and be available to any components including those rendered by the ErrorBoundary

The composed context components "just" render their children - if anything happens to the child it will still be caught by the boundary The only thing the boundary would miss is if something breaks in a lifecycle method or the constructor of a context provider, but we don't have much if any such logic

I think we're not using react-navigation's onReady in order to hide the empty LHN We delay hiding the splash/loader until we fetch (and even render) the initial reports onReady would be called earlier than that

Agree there might be more than one cause for the splash screen being stuck, because we're calling show again and it seems a bit dodgy (after all it's in componentDidUpdate) But the issue that @zoontek shared is very likely to have been the main cause, it should be easy to tell by the added logs https://github.com/zoontek/react-native-bootsplash/issues/264 containg

react-native-bootsplash has not been initialized

When that's the case then seems that no matter how many times we call hide it'll never go away @zoontek This seems like something that would not be called a 2nd time correct? https://github.com/zoontek/react-native-bootsplash/blob/0616410ffd5c1aae354d3c568b9ff74d0dc83aa8/ios/RNBootSplash.m#L43-L50

Maybe recent os/app updates made us less affected by the issue, but it's still there

Overall I agree with the updates proposed by @zoontek with the exceptions I mentioned

roryabraham commented 2 years ago

Moving the ErrorBoundary is safe, but I don't think we should do it - we won't be able to use SafeArea context (Modals) and LocaleContext (translations) in the content we render from the ErrorBoundary The root level context should remain at the root and be available to any components including those rendered by the ErrorBoundary

Sounds good @kidroca! Right now, we're not rendering anything in the ErrorBoundary, but when we do, we'll want those contexts (at least, we'll definitely want the LocaleContext).

Overall I agree with the updates proposed by @zoontek with the exceptions I mentioned

Awesome, thanks 👍

Santhosh-Sellavel commented 2 years ago

@Santhosh-Sellavel, the QA team was unable to reproduce the issue using your steps on either iOS 14.2 or iOS 15.0.1. Similarly, I am now unable to use those steps to reproduce the issue on 15.1.1 Given that our QA teasm, @roryabraham, and myself cannot reliably use these steps to reproduce the issue, I don't think we can count these as viable reproduction steps. Sorry about that!

Thanks, @NikkiWines. Actually, I am a little confused. I am using iOS - 15.0.2(iPad) & 15.1.1(iPhone) not 15.0.1.

I agree it may not count as a reproduction for this issue, since it was reproducible in the earlier version. But might be a new way to cause the same bug. If possible can you ask to QA verify it above versions I've mentioned? Thanks again!

Asked for help in slack thread here

zoontek commented 2 years ago

@roryabraham Yes, sorry! As I'm the maintainer of the bootsplash library I always forgot that not everyone has the context about it 😅

Catch and ignore bootsplash hide promise rejections as they occur when the splash screen is not "mounted" (aka the RCTRootView instance is nil - on iOS >= 15, when opening the app with a tap on a notification).

It sounds like this might resolve the case where we open the app via deeplink on iOS 15, so that's great! @zoontek Any additional context as to why would be appreciated 😄

More infos in this issue: https://github.com/zoontek/react-native-bootsplash/issues/264. It appears that on application:didFinishLaunchingWithOptions is not called in some cases, but the app still open itself correctly (see https://github.com/zoontek/react-native-bootsplash/issues/264#issuecomment-955203852). As the v3 of the library will throws react-native-bootsplash has not been initialized on these cases, it's important to catch and silent this error (the app needs to continue starting normally).

Wait for react-navigation onReady callback to start hiding the splash screen. When it's done, update the component state and mount GrowlNotification / UpdateAppModal

This also seems like a good idea, but I'm unsure how it's relevant. @zoontek can you please provide some additional context as to why you're proposing that change?

This one is not really related to the issue, it's for easier code organisation. It avoid showing <NavigationRoot /> to the user if it's still empty. It's also easier to handle the splash screen hidecall since it happen after this.state.isNavigationReady === true exclusively instead of after migrateOnyx (unauthenticated) or on componentUpdate, on this.getAuthToken() && this.props.initialReportDataLoaded && this.props.isSidebarLoaded (authenticated)

Remove the need for Bootsplash.show

If I'm not mistaken, this should actually (at least partially) solve the issue that @kidroca identified with our authentication race conditions, and prevent us getting stuck on the BootSplash screen. In the case that @kidroca identified, I'd imagine that instead of being stuck in the splash screen, we would be booted to the login screen, which is a definite improvement.

Yes, that was my original though too 🙂 https://github.com/Expensify/App/issues/5620#issuecomment-975739620 Also, I removed the show method in the v4, so it's better ditching it right now because I will be necessary to do it when adding support for Android 12 and their SplashScreen API

Moving the ErrorBoundary is safe, but I don't think we should do it - we won't be able to use SafeArea context (Modals) and LocaleContext (translations) in the content we render from the ErrorBoundary The root level context should remain at the root and be available to any components including those rendered by the ErrorBoundary

Sounds good @kidroca! Right now, we're not rendering anything in the ErrorBoundary, but when we do, we'll want those contexts (at least, we'll definitely want the LocaleContext).

I agree on that. Personally I would go for the minimal amount of providers outside of the ErrorBoundary (which are SafeAreaProvider and LocaleContextProvider, required to design this error screen). But OnyxProvider and HTMLEngineProvider does way too much to stays outside of it (especially onyx).

NikkiWines commented 2 years ago

Thanks, @NikkiWines. Actually, I am a little confused. I am using iOS - 15.0.2(iPad) & 15.1.1(iPhone) not 15.0.1.

I agree it may not count as a reproduction for this issue, since it was reproducible in the earlier version. But might be a new way to cause the same bug. If possible can you ask to QA verify it above versions I've mentioned? Thanks again!

@Santhosh-Sellavel sorry for the confusion, I wanted our QA team to test on iOS 14 and iOS 15, to see if it was reproducible on one major version and not the other. I wasn't looking to test on specific version numbers. I could ask QA to test on iOS 15.0.2 - if they have a device still on that version - but I don't really see a reason to do this, as we've already identified that we can't use these steps to reproduce the issue consistently. Additionally, I tested my phone on iOS15.1.1 - the same version as yours - and was not able to reproduce the issue either.

roryabraham commented 2 years ago

Thanks for the response and additional context @zoontek, your proposal looks good to me.

But OnyxProvider and HTMLEngineProvider does way too much to stays outside of it (especially onyx).

I think this makes sense – we'll just have to be careful to implement this page without the use of Onyx. Since (I think) it will be all static content, that should be fine.

zoontek commented 2 years ago

@roryabraham I rebased the PR and kept logs and reportBootSplashStatus: https://github.com/Expensify/App/pull/6478 I also kept non-critical context providers for the error fallback screen.

NikkiWines commented 2 years ago

Nice! @zoontek, since it looks like we want to move ahead with your proposal and PR, would you mind making an UpWork account? @kadiealexander will hire you for the associated job and handle compensation for resolving this issue through UpWork.

zoontek commented 2 years ago

@NikkiWines Here's my Upwork profile: https://www.upwork.com/freelancers/~01be4595436d743968 🙂

tgolen commented 2 years ago

I just wanted to stop by here to mention that while I was previously seeing this bug daily, I haven't experienced it in over a week now.

kadiealexander commented 2 years ago

@zoontek I've sent you a job offer in Upwork! Thanks for all your help on this one.

pateljoel commented 2 years ago

@zoontek does this mean that this issue is fixed over at https://github.com/zoontek/react-native-bootsplash as well? I too have been experiencing this issue when using your library and only come across this thread.

This in particular: https://github.com/zoontek/react-native-bootsplash/issues/264

zoontek commented 2 years ago

@pateljoel Did you catch promise rejection with v3? If you can, I recommend updating to v4, it will be easier to handle (no rejection, 100% RCTRootView.loadingView based) - or follow the steps I've taken care of in this PR. If an issue still occurs, reacts to https://github.com/zoontek/react-native-bootsplash/issues/264 with solid reproductions steps.

Same for the Expensify team. If needed, I will publish 3.x updates until you switch to v4.

botify commented 2 years ago

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

https://github.com/Expensify/App/pull/6478

If no regressions arise, payment will be issued on 2021-12-27. :confetti_ball:

NikkiWines commented 2 years ago

According to this issue it looks like people (including myself 😅 ) are still experiencing this even on v1.1.22-0. So it seems that, sadly, https://github.com/Expensify/App/pull/6478 is a fix for this.

That issue is triggered by the [alXt] [BootSplash] splash screen is still visible log line. Here's what it shows for 4 different users (in the last 4 hours). Pretty much the same across the board.

6c1280175fc97fb4-SAN | 2021-12-21 16:31:22 847 | [alXt] [BootSplash] splash screen is still visible ~~ props: '[preferredLocale: 'en' isSidebarLoaded: '1' initialReportDataLoaded: '1' updateAvailable: '' screenShareRequest: '' isAuthenticated: '1']' userAgent: 'New%20Expensify/1.1.22.0 CFNetwork/1325.0.1 Darwin/21.1.0'

6c12583edba87081-LAX | 2021-12-21 16:04:10 947 | [alXt] [BootSplash] splash screen is still visible ~~ props: '[preferredLocale: 'en' isSidebarLoaded: '1' initialReportDataLoaded: '1' updateAvailable: '' screenShareRequest: '' isAuthenticated: '1']' userAgent: 'New%20Expensify/1.1.22.0 CFNetwork/1312 Darwin/21.0.0'

6c11e55da999e6d0-SAN | 2021-12-21 14:45:45 329 | [alXt] [BootSplash] splash screen is still visible ~~ props: '[preferredLocale: 'en' isSidebarLoaded: '1' initialReportDataLoaded: '1' updateAvailable: '' screenShareRequest: '' isAuthenticated: '1']' userAgent: 'New%20Expensify/1.1.22.0 CFNetwork/1325.0.1 Darwin/21.1.0'

6c11aef95958706a-SJC | 2021-12-21 14:08:37 361 | [alXt] [BootSplash] splash screen is still visible ~~ props: '[preferredLocale: 'en' isSidebarLoaded: '1' initialReportDataLoaded: '1' updateAvailable: '' screenShareRequest: '' isAuthenticated: '1']' userAgent: 'New%20Expensify/1.1.22.0 CFNetwork/1325.0.1 Darwin/21.1.0'

zoontek commented 2 years ago

@NikkiWines Do you have an associated native crash? (I saw you use Firebase Crashlytics)

NikkiWines commented 2 years ago

@zoontek, sorry, I don't see any associated logs in Crashlytics unfortunately.

zoontek commented 2 years ago

@NikkiWines Hmmmm, OK 🤔 Do you have an issue reproduction process? (I don't have access to the issue you linked)

NikkiWines commented 2 years ago

That issue just aggregates logs associated with that particular alert. The issue reproduction steps are the same as they are for this issue: force close the app and re-open or update the app via testflight. Unfortunately, those steps don't work consistently for anyone - which is part of why this issue has been so difficult to debug. It's also why we added logs - so that we could hopefully gain some insight into what was causing this bug to pop up.

kidroca commented 2 years ago

It would help if we can see are there any hide logs near the alert

https://github.com/Expensify/App/blob/da70a0146863d0c3be70a2311b050a3121b904b9/src/libs/BootSplash/index.native.js#L7-L14

did the app ever attempt to hide the splash?
are there [BootSplash] hiding failed logs?

Is it possible to share about a minute of logging that happened at the time of the alert?

zoontek commented 2 years ago

I made some modifications to the module. When calling hide, instead of resolving immediately (the current behaviour) I append a task (aka a pending Promise) to the tasks queue.

If hide is indeed called before [RNBootSplash initWithStoryboard:] is some cases, the promise will stay in pending state to only resolve on [RNBootSplash initWithStoryboard:] or on RCTJavaScriptDidLoadNotification notification. I also handled cases where initWithStoryboard is called with nil as rootView (on iOS 15 with push notifications)

It needs a lot of extra testing but the WIP is here: https://github.com/zoontek/react-native-bootsplash/pull/295/files#diff-845e67db44156ac0a2f9af01cd7c2647560a5de9afa20574acbdf2ed6eaebd3e

I will perform a lot of tests before the end of the week. If it indeed works, I could publish a version under a specific npm tag for this project (at least until you migrate to v4 and Android 12)

PS: RNBootSplashStatus has been removed too, it was useful on v3 but not on v4. This change is not relevant to this issue.

Expensify / App