Skip to content

fix app crashes intermittently when switching to classic view on ios #91828

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.

Sign up for GitHub

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

Draft
jakubstec wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix app crashes intermittently when switching to classic view on ios #91828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/components/CustomStatusBarAndBackground/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ function CustomStatusBarAndBackground({isNested = false}: CustomStatusBarAndBack
// This callback is triggered every time the route changes or the theme changes
const updateStatusBarStyle = useCallback(
(listenerID?: number) => {
if (closingReactNativeApp) {
return;
}

// Check if this function is either called through the current navigation listener
// react-navigation library has a bug internally, where it can't keep track of the listeners, therefore, sometimes when the useEffect would re-render and we run navigationRef.removeListener the listener isn't removed and we end up with two or more listeners.
// https://github.com/Expensify/App/issues/34154#issuecomment-1898519399
Expand Down Expand Up @@ -146,6 +150,7 @@ function CustomStatusBarAndBackground({isNested = false}: CustomStatusBarAndBack
isRootStatusBarEnabled,
statusBarStyle,
statusBarAnimation,
closingReactNativeApp,
],
);

Expand Down
24 changes: 17 additions & 7 deletions src/libs/actions/HybridApp/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import HybridAppModule from '@expensify/react-native-hybrid-app';
import Onyx from 'react-native-onyx';
import type {OnyxEntry} from 'react-native-onyx';
import Log from '@libs/Log';
import Navigation from '@libs/Navigation/Navigation';
import {isLockedToNewApp, shouldBlockOldAppExit} from '@libs/TryNewDotUtils';
import {setIsGPSInProgressModalOpen} from '@userActions/isGPSInProgressModalOpen';
Expand Down Expand Up @@ -109,13 +110,22 @@ function closeReactNativeApp({shouldSetNVP, isTrackingGPS, shouldIgnoreTryNewDot
return;
}

Navigation.clearPreloadedRoutes();
if (CONFIG.IS_HYBRID_APP) {
Onyx.merge(ONYXKEYS.HYBRID_APP, {closingReactNativeApp: true});
}

// eslint-disable-next-line no-restricted-properties
HybridAppModule.closeReactNativeApp({shouldSetNVP});
const closingPromise = CONFIG.IS_HYBRID_APP ? Onyx.merge(ONYXKEYS.HYBRID_APP, {closingReactNativeApp: true}) : Promise.resolve();

closingPromise
.then(() => {
requestAnimationFrame(() => {
Navigation.clearPreloadedRoutes();
// eslint-disable-next-line no-restricted-properties
HybridAppModule.closeReactNativeApp({shouldSetNVP});
});
})
.catch((error) => {
Log.warn('[HybridApp] Failed to merge closingReactNativeApp before close', {error});
Navigation.clearPreloadedRoutes();
// eslint-disable-next-line no-restricted-properties
HybridAppModule.closeReactNativeApp({shouldSetNVP});
});
}

/*
Expand Down
31 changes: 18 additions & 13 deletions tests/unit/HybridAppActionsTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ describe('HybridApp actions', () => {
const {closeReactNativeApp} = require('@libs/actions/HybridApp') as HybridAppActionsModule;
let closeNativeAppSpy: jest.SpiedFunction<HybridAppModuleWithClose['closeReactNativeApp']>;

const closeAndWait = async (params: Parameters<HybridAppActionsModule['closeReactNativeApp']>[0]) => {
closeReactNativeApp(params);
await waitForBatchedUpdatesWithAct();
};

beforeEach(async () => {
jest.clearAllMocks();
closeNativeAppSpy = jest.spyOn(HybridAppModule, 'closeReactNativeApp').mockImplementation(() => {});
Expand Down Expand Up @@ -86,7 +91,7 @@ describe('HybridApp actions', () => {
await Onyx.set(ONYXKEYS.IS_LOADING_APP, false);
await waitForBatchedUpdatesWithAct();

closeReactNativeApp({shouldSetNVP: true, isTrackingGPS: false});
await closeAndWait({shouldSetNVP: true, isTrackingGPS: false});

expect(Navigation.clearPreloadedRoutes).toHaveBeenCalled();
expect(closeNativeAppSpy).toHaveBeenCalledWith({shouldSetNVP: true});
Expand All @@ -96,7 +101,7 @@ describe('HybridApp actions', () => {
await Onyx.set(ONYXKEYS.IS_LOADING_APP, true);
await waitForBatchedUpdatesWithAct();

closeReactNativeApp({shouldSetNVP: true, isTrackingGPS: false, shouldIgnoreTryNewDotLoading: true});
await closeAndWait({shouldSetNVP: true, isTrackingGPS: false, shouldIgnoreTryNewDotLoading: true});

expect(Navigation.clearPreloadedRoutes).toHaveBeenCalled();
expect(closeNativeAppSpy).toHaveBeenCalledWith({shouldSetNVP: true});
Expand Down Expand Up @@ -137,23 +142,23 @@ describe('HybridApp actions', () => {
});
await waitForBatchedUpdatesWithAct();

closeReactNativeApp({shouldSetNVP: true, isTrackingGPS: false});
await closeAndWait({shouldSetNVP: true, isTrackingGPS: false});
expect(closeNativeAppSpy).toHaveBeenCalledWith({shouldSetNVP: true});

jest.clearAllMocks();

await Onyx.clear([ONYXKEYS.IS_LOADING_APP]);
await waitForBatchedUpdatesWithAct();

closeReactNativeApp({shouldSetNVP: true, isTrackingGPS: false});
await closeAndWait({shouldSetNVP: true, isTrackingGPS: false});
expect(closeNativeAppSpy).not.toHaveBeenCalled();

await Onyx.set(ONYXKEYS.IS_LOADING_APP, true);
await waitForBatchedUpdatesWithAct();
await Onyx.set(ONYXKEYS.IS_LOADING_APP, false);
await waitForBatchedUpdatesWithAct();

closeReactNativeApp({shouldSetNVP: true, isTrackingGPS: false});
await closeAndWait({shouldSetNVP: true, isTrackingGPS: false});
expect(closeNativeAppSpy).toHaveBeenCalledWith({shouldSetNVP: true});
});

Expand All @@ -172,7 +177,7 @@ describe('HybridApp actions', () => {
});
await waitForBatchedUpdatesWithAct();

closeReactNativeApp({shouldSetNVP: true, isTrackingGPS: false});
await closeAndWait({shouldSetNVP: true, isTrackingGPS: false});
expect(closeNativeAppSpy).toHaveBeenCalledWith({shouldSetNVP: true});

jest.clearAllMocks();
Expand All @@ -183,7 +188,7 @@ describe('HybridApp actions', () => {
});
await waitForBatchedUpdatesWithAct();

closeReactNativeApp({shouldSetNVP: true, isTrackingGPS: false});
await closeAndWait({shouldSetNVP: true, isTrackingGPS: false});
expect(closeNativeAppSpy).not.toHaveBeenCalled();

await Onyx.set(ONYXKEYS.NVP_TRY_NEW_DOT, {
Expand All @@ -193,7 +198,7 @@ describe('HybridApp actions', () => {
});
await waitForBatchedUpdatesWithAct();

closeReactNativeApp({shouldSetNVP: true, isTrackingGPS: false});
await closeAndWait({shouldSetNVP: true, isTrackingGPS: false});
expect(closeNativeAppSpy).toHaveBeenCalledWith({shouldSetNVP: true});
});

Expand All @@ -218,7 +223,7 @@ describe('HybridApp actions', () => {

// closeReactNativeApp should still work because the initial session load
// must not blank the already-populated currentTryNewDot
closeReactNativeApp({shouldSetNVP: true, isTrackingGPS: false});
await closeAndWait({shouldSetNVP: true, isTrackingGPS: false});
expect(closeNativeAppSpy).toHaveBeenCalledWith({shouldSetNVP: true});
});

Expand All @@ -237,7 +242,7 @@ describe('HybridApp actions', () => {
});
await waitForBatchedUpdatesWithAct();

closeReactNativeApp({shouldSetNVP: true, isTrackingGPS: false});
await closeAndWait({shouldSetNVP: true, isTrackingGPS: false});
expect(closeNativeAppSpy).toHaveBeenCalledWith({shouldSetNVP: true});

jest.clearAllMocks();
Expand All @@ -247,12 +252,12 @@ describe('HybridApp actions', () => {
});
await waitForBatchedUpdatesWithAct();

closeReactNativeApp({shouldSetNVP: true, isTrackingGPS: false});
await closeAndWait({shouldSetNVP: true, isTrackingGPS: false});
expect(closeNativeAppSpy).toHaveBeenCalledWith({shouldSetNVP: true});
});

it('preserves shouldSetNVP false exits for existing non-force-mobile flows', () => {
closeReactNativeApp({shouldSetNVP: false, isTrackingGPS: false});
it('preserves shouldSetNVP false exits for existing non-force-mobile flows', async () => {
await closeAndWait({shouldSetNVP: false, isTrackingGPS: false});

expect(Navigation.clearPreloadedRoutes).toHaveBeenCalled();
expect(closeNativeAppSpy).toHaveBeenCalledWith({shouldSetNVP: false});
Expand Down
Loading