Merge pull request Expensify#89254 from callstack-internal/perf/fix-doubled-OpenApp

mountiny · web-flow · commit 14a7d1b639e2 · 2026-05-01T08:18:31.000+02:00
diff --git a/src/libs/NetworkState.ts b/src/libs/NetworkState.ts
@@ -20,6 +20,7 @@ let unsubscribeNetInfo: (() => void) | null = null;
 let prevIsInternetReachable: boolean | null | undefined;
 let isPoorConnectionSimulated: boolean | undefined;
 let networkTimeSkew = 0;
+let suppressNextReachabilityRestored = false;
 
 // Subscriber sets
 const listeners = new Set<() => void>();
@@ -268,11 +269,24 @@ function setRandomNetworkStatus(initialCall = false) {
  * Must unsubscribe before calling configure() — configure tears down NetInfo internal state.
  */
 function configureAndSubscribe() {
+    // Treat this as a reconfigure (not an initial subscription) when there's already a listener.
+    // Reconfigure tears down NetInfo internal state, so the new subscription emits a synthetic
+    // null→true transition that would look like a recovery — suppress the next would-be recovery
+    // until reachability settles. Initial subscription is left untouched so boot behavior is
+    // unchanged (prev=undefined boot guard already covers it).
+    // Skip suppression when prev was already false: the app was genuinely offline before
+    // reconfigure, so the next true is a real recovery we must not drop (otherwise
+    // internetUnreachable stays set and the app is stuck offline until a new outage cycle).
+    const isReconfigure = unsubscribeNetInfo !== null;
     if (unsubscribeNetInfo) {
         unsubscribeNetInfo();
         unsubscribeNetInfo = null;
     }
 
+    if (isReconfigure && prevIsInternetReachable !== false) {
+        suppressNextReachabilityRestored = true;
+    }
+
     if (!CONFIG.IS_USING_LOCAL_WEB) {
         NetInfo.configure({
             reachabilityUrl: `${CONFIG.EXPENSIFY.DEFAULT_API_ROOT}api/Ping?accountID=${accountID ?? 'unknown'}`,
@@ -315,8 +329,17 @@ function configureAndSubscribe() {
         // NetInfo event on subscribe which delivers current state, not a recovery. Firing
         // onReachabilityRestored() on boot would duplicate openApp()/reconnectApp().
         if (!shouldForceOffline && state.isInternetReachable === true && prevIsInternetReachable !== true && prevIsInternetReachable !== undefined) {
-            Log.info(`[NetworkState] Internet reachability restored (${prevIsInternetReachable}→true)`);
-            onReachabilityRestored();
+            if (suppressNextReachabilityRestored) {
+                Log.info(`[NetworkState] Suppressing recovery on first stable state after reconfigure (${prevIsInternetReachable}→true)`);
+            } else {
+                Log.info(`[NetworkState] Internet reachability restored (${prevIsInternetReachable}→true)`);
+                onReachabilityRestored();
+            }
+        }
+        // End the post-reconfigure suppression window once reachability settles into a definitive
+        // state. Null/undefined are transient and should not end the window.
+        if (state.isInternetReachable === true || state.isInternetReachable === false) {
+            suppressNextReachabilityRestored = false;
         }
         prevIsInternetReachable = state.isInternetReachable;
     });
diff --git a/src/libs/actions/Delegate.ts b/src/libs/actions/Delegate.ts
@@ -41,6 +41,17 @@ const KEYS_TO_PRESERVE_DELEGATE_ACCESS = [
     ONYXKEYS.COLLECTION.DEVICE_BIOMETRICS,
 ];
 
+/**
+ * Atomically reset Onyx for a delegate-access transition while keeping IS_LOADING_APP=true
+ * so consumers never observe the post-clear state with HAS_LOADED_APP=true and
+ * IS_LOADING_APP=undefined. That combination falsely looks like a stuck app and triggers
+ * DelegateAccessHandler's recovery effect, producing a duplicate openApp queued behind the
+ * explicit openApp the caller is about to make.
+ */
+function clearOnyxForDelegateTransition(): Promise<void> {
+    return Onyx.merge(ONYXKEYS.IS_LOADING_APP, true).then(() => Onyx.clear([...KEYS_TO_PRESERVE_DELEGATE_ACCESS, ONYXKEYS.IS_LOADING_APP]));
+}
+
 type WithDelegatedAccess = {
     // Optional keeps call sites clean, but still encourages passing `account?.delegatedAccess`.
     delegatedAccess: DelegatedAccess | undefined;
@@ -195,7 +206,7 @@ function connect({email, delegatedAccess, credentials, session, activePolicyID,
                 })
                 .then(() => {
                     NetworkStore.setAuthToken(response?.restrictedToken ?? null);
-                    return Onyx.clear(KEYS_TO_PRESERVE_DELEGATE_ACCESS);
+                    return clearOnyxForDelegateTransition();
                 })
                 .then(() => {
                     confirmReadyToOpenApp();
@@ -294,7 +305,7 @@ function disconnect({stashedCredentials, stashedSession}: DisconnectParams) {
                 })
                 .then(() => {
                     NetworkStore.setAuthToken(response?.authToken ?? null);
-                    return Onyx.clear(KEYS_TO_PRESERVE_DELEGATE_ACCESS);
+                    return clearOnyxForDelegateTransition();
                 })
                 .then(() => {
                     Onyx.set(ONYXKEYS.CREDENTIALS, {
@@ -663,7 +674,7 @@ function updateDelegateRole({email, role, validateCode, delegatedAccess}: Update
 }
 
 function restoreDelegateSession<TKey extends OnyxKey>(authenticateResponse: Response<TKey>) {
-    Onyx.clear(KEYS_TO_PRESERVE_DELEGATE_ACCESS).then(() => {
+    clearOnyxForDelegateTransition().then(() => {
         updateSessionAuthTokens(authenticateResponse?.authToken, authenticateResponse?.encryptedAuthToken);
         updateSessionUser(authenticateResponse?.accountID, authenticateResponse?.email);
 
@@ -690,5 +701,6 @@ export {
     updateDelegateRole,
     removeDelegate,
     openSecuritySettingsPage,
+    clearOnyxForDelegateTransition,
     KEYS_TO_PRESERVE_DELEGATE_ACCESS,
 };
diff --git a/src/libs/actions/Session/index.ts b/src/libs/actions/Session/index.ts
@@ -51,7 +51,7 @@ import Timers from '@libs/Timers';
 import {hideContextMenu} from '@pages/inbox/report/ContextMenu/ReportActionContextMenu';
 import {confirmReadyToOpenApp, KEYS_TO_PRESERVE, openApp} from '@userActions/App';
 import {clearCachedAttachments} from '@userActions/Attachment';
-import {KEYS_TO_PRESERVE_DELEGATE_ACCESS} from '@userActions/Delegate';
+import {clearOnyxForDelegateTransition} from '@userActions/Delegate';
 import * as Device from '@userActions/Device';
 import type HybridAppSettings from '@userActions/HybridApp/types';
 import {close} from '@userActions/Modal';
@@ -718,7 +718,7 @@ function setupNewDotAfterTransitionFromOldDot(hybridAppSettings: HybridAppSettin
             }
 
             Log.info('[HybridApp] User switched account on OldDot side. Clearing onyx and applying delegate data');
-            return Onyx.clear(KEYS_TO_PRESERVE_DELEGATE_ACCESS)
+            return clearOnyxForDelegateTransition()
                 .then(() =>
                     Onyx.multiSet({
                         ...stashedData,
diff --git a/tests/actions/DelegateTest.ts b/tests/actions/DelegateTest.ts
@@ -1,5 +1,13 @@
 import Onyx from 'react-native-onyx';
-import {addDelegate, clearDelegateErrorsByField, clearDelegatorErrors, isConnectedAsDelegate, removeDelegate, updateDelegateRole} from '@libs/actions/Delegate';
+import {
+    addDelegate,
+    clearDelegateErrorsByField,
+    clearDelegatorErrors,
+    clearOnyxForDelegateTransition,
+    isConnectedAsDelegate,
+    removeDelegate,
+    updateDelegateRole,
+} from '@libs/actions/Delegate';
 import {pause, resetQueue} from '@libs/Network/SequentialQueue';
 import CONST from '@src/CONST';
 import OnyxUpdateManager from '@src/libs/actions/OnyxUpdateManager';
@@ -236,4 +244,54 @@ describe('actions/Delegate', () => {
             });
         });
     });
+
+    describe('clearOnyxForDelegateTransition', () => {
+        it('keeps IS_LOADING_APP=true after the clear so DelegateAccessHandler does not see HAS_LOADED_APP=true && IS_LOADING_APP=undefined and fire a duplicate openApp', async () => {
+            // Simulate the pre-switch state: app is fully loaded with the previous account.
+            await Onyx.multiSet({
+                [ONYXKEYS.HAS_LOADED_APP]: true,
+                [ONYXKEYS.IS_LOADING_APP]: false,
+                [ONYXKEYS.NVP_PRIORITY_MODE]: CONST.PRIORITY_MODE.DEFAULT,
+            });
+            await waitForBatchedUpdates();
+
+            await clearOnyxForDelegateTransition();
+            await waitForBatchedUpdates();
+
+            await new Promise<void>((resolve) => {
+                const conn = Onyx.connect({
+                    key: ONYXKEYS.IS_LOADING_APP,
+                    callback: (value) => {
+                        expect(value).toBe(true);
+                        Onyx.disconnect(conn);
+                        resolve();
+                    },
+                });
+            });
+
+            // HAS_LOADED_APP is in the preserve list and should remain true.
+            await new Promise<void>((resolve) => {
+                const conn = Onyx.connect({
+                    key: ONYXKEYS.HAS_LOADED_APP,
+                    callback: (value) => {
+                        expect(value).toBe(true);
+                        Onyx.disconnect(conn);
+                        resolve();
+                    },
+                });
+            });
+
+            // A non-preserved key should have been cleared.
+            await new Promise<void>((resolve) => {
+                const conn = Onyx.connect({
+                    key: ONYXKEYS.NVP_PRIORITY_MODE,
+                    callback: (value) => {
+                        expect(value).toBeUndefined();
+                        Onyx.disconnect(conn);
+                        resolve();
+                    },
+                });
+            });
+        });
+    });
 });
diff --git a/tests/unit/NetworkStateReachabilityTest.ts b/tests/unit/NetworkStateReachabilityTest.ts
@@ -2,6 +2,7 @@ import type {NetInfoState} from '@react-native-community/netinfo';
 import type * as NetworkState from '@src/libs/NetworkState';
 
 let netInfoListener: ((state: NetInfoState) => void) | null = null;
+const mockOnyxCallbacks = new Map<string, (value: unknown) => void>();
 
 jest.mock('@react-native-community/netinfo', () => ({
     addEventListener: jest.fn((cb: (state: NetInfoState) => void) => {
@@ -17,7 +18,9 @@ jest.mock('@react-native-community/netinfo', () => ({
 
 jest.mock('@src/libs/Log');
 jest.mock('react-native-onyx', () => ({
-    connectWithoutView: jest.fn(),
+    connectWithoutView: jest.fn(({key, callback}: {key: string; callback: (value: unknown) => void}) => {
+        mockOnyxCallbacks.set(key, callback);
+    }),
 }));
 
 function fireNetInfoState(overrides: Partial<NetInfoState>) {
@@ -33,12 +36,21 @@ function fireNetInfoState(overrides: Partial<NetInfoState>) {
     } as NetInfoState);
 }
 
+function fireSessionChange(accountID: number) {
+    const cb = mockOnyxCallbacks.get('session');
+    if (!cb) {
+        throw new Error('SESSION callback not registered');
+    }
+    cb({accountID});
+}
+
 describe('NetworkState — internetUnreachable hard stop via NetInfo', () => {
     let getIsOffline: typeof NetworkState.getIsOffline;
 
     beforeEach(() => {
         jest.resetModules();
         netInfoListener = null;
+        mockOnyxCallbacks.clear();
 
         const mod = require<typeof NetworkState>('@src/libs/NetworkState');
         getIsOffline = mod.getIsOffline;
@@ -81,6 +93,7 @@ describe('NetworkState — reachability recovery triggers reconnect', () => {
     beforeEach(() => {
         jest.resetModules();
         netInfoListener = null;
+        mockOnyxCallbacks.clear();
 
         // Fresh import each test so prevIsInternetReachable resets
         const mod = require<typeof NetworkState>('@src/libs/NetworkState');
@@ -143,6 +156,69 @@ describe('NetworkState — reachability recovery triggers reconnect', () => {
         expect(reconnectListener).not.toHaveBeenCalled();
     });
 
+    test('SESSION accountID change does NOT fire reconnect listener via the post-reconfigure synthetic transition', () => {
+        // Repro for the doubled-OpenApp bug on delegate switch: the SESSION accountID change
+        // re-runs configureAndSubscribe(), which tears down and re-subscribes to NetInfo. The
+        // new subscription emits null then true, which would look like a recovery. The fix
+        // resets prev to undefined on reconfigure so the new subscription's first transitions
+        // are treated like boot, not recovery.
+        const reconnectListener = jest.fn();
+        onReachabilityConfirmed(reconnectListener);
+
+        // Establish a baseline reachable state (boot)
+        fireNetInfoState({isInternetReachable: true});
+        expect(reconnectListener).not.toHaveBeenCalled();
+
+        // Delegate switch: SESSION accountID changes → reconfigure → new NetInfo subscription
+        fireSessionChange(42);
+
+        // New subscription's initial events: null while the first Ping is in flight, then true
+        fireNetInfoState({isInternetReachable: null});
+        fireNetInfoState({isInternetReachable: true});
+
+        expect(reconnectListener).not.toHaveBeenCalled();
+    });
+
+    test('genuine offline→online after a SESSION reconfigure still fires reconnect listener', () => {
+        // Make sure the reconfigure suppression doesn't swallow real recoveries that happen
+        // afterwards — only the synthetic post-reconfigure transition should be ignored.
+        const reconnectListener = jest.fn();
+        onReachabilityConfirmed(reconnectListener);
+
+        fireNetInfoState({isInternetReachable: true});
+        fireSessionChange(42);
+
+        // Settle on the reconfigured subscription
+        fireNetInfoState({isInternetReachable: true});
+        expect(reconnectListener).not.toHaveBeenCalled();
+
+        // Now a real outage and recovery
+        fireNetInfoState({isInternetReachable: false});
+        fireNetInfoState({isInternetReachable: true});
+
+        expect(reconnectListener).toHaveBeenCalledTimes(1);
+    });
+
+    test('genuine offline before reconfigure still recovers on the next true', () => {
+        // Boot offline scenario: NetInfo confirms unreachable BEFORE SESSION hydrates and triggers
+        // a reconfigure. The post-reconfigure true must NOT be suppressed — otherwise the app would
+        // remain stuck with internetUnreachable=true until a brand new outage cycle.
+        const reconnectListener = jest.fn();
+        onReachabilityConfirmed(reconnectListener);
+
+        // Cold boot: null then false → app is genuinely offline, prev=false
+        fireNetInfoState({isInternetReachable: null});
+        fireNetInfoState({isInternetReachable: false});
+
+        // SESSION hydrates → reconfigure happens while we are still offline
+        fireSessionChange(42);
+
+        // New subscription's first definitive event recovers
+        fireNetInfoState({isInternetReachable: true});
+
+        expect(reconnectListener).toHaveBeenCalledTimes(1);
+    });
+
     test('turning off force-offline resets prevIsInternetReachable so next refresh triggers reconnect', () => {
         const reconnectListener = jest.fn();
         onReachabilityConfirmed(reconnectListener);
