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.36k stars 2.78k forks source link

[Crashlytics] TypeError: Cannot read property 'routes' of undefined #46118

Closed CortneyOfstad closed 2 weeks ago

CortneyOfstad commented 2 months ago

Coming from this GH β€” https://github.com/Expensify/App/issues/45054#issuecomment-2238548276

Reported by @TMisiukiewicz

Fatal Exception: com.facebook.react.common.JavascriptException: TypeError: Cannot read property 'routes' of undefined, js engine: hermes, stack:
onBackPress@1:7675381
anonymous@1:1137735
emit@1:194390
emit@1:190480
__callFunction@1:200223
anonymous@1:197709
__guard@1:199579
callFunctionReturnFlushedQueue@1:197667

       at com.facebook.react.modules.core.ExceptionsManagerModule.reportException(ExceptionsManagerModule.java:65)
       at com.facebook.jni.NativeRunnable.run(NativeRunnable.java)
       at android.os.Handler.handleCallback(Handler.java:959)
       at android.os.Handler.dispatchMessage(Handler.java:100)
       at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:29)
       at android.os.Looper.loopOnce(Looper.java:232)
       at android.os.Looper.loop(Looper.java:317)
       at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:234)
       at java.lang.Thread.run(Thread.java:1012)
Issue OwnerCurrent Issue Owner: @CortneyOfstad
melvin-bot[bot] commented 2 months ago

Current assignee @CortneyOfstad is eligible for the Bug assigner, not assigning anyone new.

melvin-bot[bot] commented 2 months ago

@CortneyOfstad Eep! 4 days overdue now. Issues have feelings too...

CortneyOfstad commented 2 months ago

@TMisiukiewicz it was indicated on one of the other Crashlytic GHs that the logs are truncated. Any way you could pull up the full logs? Thanks!

TMisiukiewicz commented 1 month ago

I tried following the breadcrumbs from firebase, but couldn't reproduce it. Looks like when using system back button on Android, sometimes getRootState returns undefined. According to the docs, the returned state will be undefined if there are no navigators currently rendered, but i'm not sure how to reproduce this state.

We could prevent crashing by returning false when the root state is undefined. If opening a PR without repro steps is fine, I can create it

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

@CortneyOfstad this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 1 month ago

@CortneyOfstad Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] commented 1 month ago

@CortneyOfstad Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

CortneyOfstad commented 1 month ago

@TMisiukiewicz sorry for the delay here! For this comment, I would be leery to create a PR without the workflow, so leaning towards closing this. However, if you feel strongly that the PR should still be created, let me know and we can work it out with the team. Thanks!

melvin-bot[bot] commented 1 month ago

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

hurali97 commented 1 month ago

Hey @CortneyOfstad πŸ‘‹

I took it over from @TMisiukiewicz and I also can't reproduce this, however I think we should still go about creating a PR fixing this.

We can opt for a simpler solution, which is to use the optional chaining, see below:

diff --git a/src/libs/Navigation/setupCustomAndroidBackHandler/index.android.ts b/src/libs/Navigation/setupCustomAndroidBackHandler/index.android.ts
index 10aa8b99a4..107dae2bb7 100644
--- a/src/libs/Navigation/setupCustomAndroidBackHandler/index.android.ts
+++ b/src/libs/Navigation/setupCustomAndroidBackHandler/index.android.ts
@@ -13,8 +13,7 @@ type SearchPageProps = StackScreenProps<BottomTabNavigatorParamList, typeof SCRE
 function setupCustomAndroidBackHandler() {
     const onBackPress = () => {
         const rootState = navigationRef.getRootState();
-
-        const bottomTabRoute = rootState.routes.find((route) => route.name === NAVIGATORS.BOTTOM_TAB_NAVIGATOR);
+        const bottomTabRoute = rootState?.routes?.find((route) => route.name === NAVIGATORS.BOTTOM_TAB_NAVIGATOR);
         const bottomTabRoutes = bottomTabRoute?.state?.routes;
         const focusedRoute = findFocusedRoute(rootState);

@@ -23,7 +22,7 @@ function setupCustomAndroidBackHandler() {
             return false;
         }

-        const isLastScreenOnStack = bottomTabRoutes.length === 1 && rootState.routes.length === 1;
+        const isLastScreenOnStack = bottomTabRoutes.length === 1 && rootState?.routes?.length === 1;

         if (NativeModules.HybridAppModule && isLastScreenOnStack) {
             NativeModules.HybridAppModule.exitApp();
@@ -35,7 +34,7 @@ function setupCustomAndroidBackHandler() {
             navigationRef.dispatch({...StackActions.pop(), target: bottomTabRoute?.state?.key});
             navigationRef.dispatch({...StackActions.pop()});

-            const centralPaneRouteAfterPop = getTopmostCentralPaneRoute({routes: [rootState.routes.at(-2)]} as State<RootStackParamList>);
+            const centralPaneRouteAfterPop = getTopmostCentralPaneRoute({routes: [rootState?.routes?.at(-2)]} as State<RootStackParamList>);
             const bottomTabRouteAfterPop = bottomTabRoutes.at(-2);

             // It's possible that central pane search is desynchronized with the bottom tab search.
@@ -57,7 +56,7 @@ function setupCustomAndroidBackHandler() {
         // It's possible that central pane search is desynchronized with the bottom tab search.
         // e.g. opening a tab different than search will wipe out central pane screens.
         // In that case we have to push the proper one.
-        if (bottomTabRoutes && bottomTabRoutes?.length >= 2 && bottomTabRoutes[bottomTabRoutes.length - 2].name === SCREENS.SEARCH.BOTTOM_TAB && rootState.routes.length === 1) {
+        if (bottomTabRoutes && bottomTabRoutes?.length >= 2 && bottomTabRoutes[bottomTabRoutes.length - 2].name === SCREENS.SEARCH.BOTTOM_TAB && rootState?.routes?.length === 1) {
             const {policyID, ...restParams} = bottomTabRoutes[bottomTabRoutes.length - 2].params as SearchPageProps['route']['params'];
             navigationRef.dispatch({...StackActions.push(SCREENS.SEARCH.CENTRAL_PANE, {...restParams, policyIDs: policyID})});
             navigationRef.dispatch({...StackActions.pop(), target: bottomTabRoute?.state?.key});

This won't introduce any regressions and is more of a safety check. Ideally, when accessing nested optional properties, we use optional chaining, which was missing and causing this issue.


As for the testing, I manually set rootState = undefined and tested this code, which doesn't crash on hardware back press on Android.

CortneyOfstad commented 1 month ago

That sounds great @hurali97! Thank you!

melvin-bot[bot] commented 1 month ago

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

mananjadhav commented 3 weeks ago

@blimpich Can you confirm if this was deployed to production? I can't make out that. I can see it's deployed to staging 2 weeks back. If it is deployed then this would be ready for payout.

blimpich commented 2 weeks ago

Yes looks like it was deployed to production πŸ‘

blimpich commented 2 weeks ago

Sorry for the delay, automation must have messed up with the deploy notification

blimpich commented 2 weeks ago

Ah, part of this is probably because there was never external label applied to this issue. @CortneyOfstad can we treat this issue as a normal $250 issue and pay out to @mananjadhav as the C+ who reviewed the associated PR?

mananjadhav commented 2 weeks ago

thanks for taking a look @blimpich. @CortneyOfstad would be great if yo can share the payout summary.

CortneyOfstad commented 2 weeks ago

Sorry for the delay here!

CortneyOfstad commented 2 weeks ago

Payment Summary

@mananjadhav β€” to be paid $250 via NewDot

garrettmknight commented 2 weeks ago

$250 approved for @mananjadhav