From 0eb29014598ddad3ddfb978738b9646e5e202f25 Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Thu, 30 Apr 2026 10:06:26 +0200 Subject: [PATCH 1/4] Fix doubled OpenApp on delegate account switch --- src/libs/NetworkState.ts | 23 +++++++++++++++++------ src/libs/actions/Delegate.ts | 18 +++++++++++++++--- src/libs/actions/Session/index.ts | 4 ++-- 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/src/libs/NetworkState.ts b/src/libs/NetworkState.ts index b94e45a31d96..1028f4103065 100644 --- a/src/libs/NetworkState.ts +++ b/src/libs/NetworkState.ts @@ -273,6 +273,12 @@ function configureAndSubscribe() { unsubscribeNetInfo = null; } + // Reset to the same "fresh subscription" state as boot. The listener's `prev !== undefined` + // guard then blocks the synthetic null→true transition that NetInfo emits while reconfiguring + // (which would otherwise look like a recovery and fire a duplicate openApp/reconnectApp, + // e.g. on accountID change for a delegate switch). + prevIsInternetReachable = undefined; + if (!CONFIG.IS_USING_LOCAL_WEB) { NetInfo.configure({ reachabilityUrl: `${CONFIG.EXPENSIFY.DEFAULT_API_ROOT}api/Ping?accountID=${accountID ?? 'unknown'}`, @@ -309,16 +315,21 @@ function configureAndSubscribe() { setInternetUnreachable(true); } - // Treat false→true and null→true as genuine recovery. Both mean NetInfo previously - // lost reachability (false = confirmed unreachable, null = lost track during outage) - // and has now confirmed it's back. Only block undefined→true — that's the initial - // NetInfo event on subscribe which delivers current state, not a recovery. Firing - // onReachabilityRestored() on boot would duplicate openApp()/reconnectApp(). + // Treat false→true as genuine recovery (NetInfo previously confirmed unreachable, now + // confirmed back), and null→true when prev was explicitly reset to null by debug-tool + // toggles (setForceOffline / setFailAllRequests / simulatePoorConnection) that need to + // force a recovery on the next definitive state. Block undefined→true — that's the + // first event on a fresh subscription, not a recovery. if (!shouldForceOffline && state.isInternetReachable === true && prevIsInternetReachable !== true && prevIsInternetReachable !== undefined) { Log.info(`[NetworkState] Internet reachability restored (${prevIsInternetReachable}→true)`); onReachabilityRestored(); } - prevIsInternetReachable = state.isInternetReachable; + // Only track definitive reachable/unreachable states. Ignoring null/undefined keeps the + // synthetic true→null→true sequence NetInfo emits during reconfigure from looking like a + // recovery transition (and lets configureAndSubscribe's `prev = undefined` reset stick). + if (state.isInternetReachable === true || state.isInternetReachable === false) { + prevIsInternetReachable = state.isInternetReachable; + } }); } diff --git a/src/libs/actions/Delegate.ts b/src/libs/actions/Delegate.ts index 4cbacabd6034..3da4ee1b8602 100644 --- 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 { + 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(authenticateResponse: Response) { - 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 index 90da86594b56..a5a44a83e43d 100644 --- 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, From f794cbc1b83c363b3ab3fb9c5a3cb17426989cf4 Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Thu, 30 Apr 2026 10:10:58 +0200 Subject: [PATCH 2/4] Add tests for doubled-OpenApp delegate-switch fix --- tests/actions/DelegateTest.ts | 60 +++++++++++++++- tests/unit/NetworkStateReachabilityTest.ts | 80 ++++++++++++++++++++-- 2 files changed, 135 insertions(+), 5 deletions(-) diff --git a/tests/actions/DelegateTest.ts b/tests/actions/DelegateTest.ts index 0081ba0d192f..e9e30aa3e31e 100644 --- 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((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((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((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 index 53f510189e3c..d93ee7a37779 100644 --- 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 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) { @@ -33,12 +36,21 @@ function fireNetInfoState(overrides: Partial) { } 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('@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('@src/libs/NetworkState'); @@ -99,15 +112,31 @@ describe('NetworkState — reachability recovery triggers reconnect', () => { expect(reconnectListener).toHaveBeenCalledTimes(1); }); - test('null→true fires reconnect listener', () => { + test('null→true on a fresh subscription does NOT fire reconnect listener', () => { const reconnectListener = jest.fn(); onReachabilityConfirmed(reconnectListener); - // Simulate losing reachability tracking (null) then recovering + // After a fresh subscription, the first events deliver current state. NetInfo may + // emit null first while the initial Ping is in flight, then true once it completes. + // This is not a recovery — only false→true (or an explicit prev=null reset) is. fireNetInfoState({isInternetReachable: null}); fireNetInfoState({isInternetReachable: true}); - expect(reconnectListener).toHaveBeenCalledTimes(1); + expect(reconnectListener).not.toHaveBeenCalled(); + }); + + test('true→null→true does NOT fire reconnect listener (transient state, no confirmed offline)', () => { + const reconnectListener = jest.fn(); + onReachabilityConfirmed(reconnectListener); + + // Establish a baseline reachable state + fireNetInfoState({isInternetReachable: true}); + // NetInfo briefly loses track without confirming offline (e.g. during a poll gap) + fireNetInfoState({isInternetReachable: null}); + // Reachability comes back. Since we never went through false, this is not a recovery. + fireNetInfoState({isInternetReachable: true}); + + expect(reconnectListener).not.toHaveBeenCalled(); }); test('undefined→true does NOT fire reconnect listener (boot event)', () => { @@ -143,6 +172,49 @@ 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('turning off force-offline resets prevIsInternetReachable so next refresh triggers reconnect', () => { const reconnectListener = jest.fn(); onReachabilityConfirmed(reconnectListener); From ae8fe8e08aeff51b8dd85c906c151b3484a83881 Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Thu, 30 Apr 2026 10:16:59 +0200 Subject: [PATCH 3/4] Scope reachability suppression to reconfigure only --- src/libs/NetworkState.ts | 41 +++++++++++++--------- tests/unit/NetworkStateReachabilityTest.ts | 22 ++---------- 2 files changed, 28 insertions(+), 35 deletions(-) diff --git a/src/libs/NetworkState.ts b/src/libs/NetworkState.ts index 1028f4103065..48b794c45a4c 100644 --- 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,16 +269,20 @@ 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). + const isReconfigure = unsubscribeNetInfo !== null; if (unsubscribeNetInfo) { unsubscribeNetInfo(); unsubscribeNetInfo = null; } - // Reset to the same "fresh subscription" state as boot. The listener's `prev !== undefined` - // guard then blocks the synthetic null→true transition that NetInfo emits while reconfiguring - // (which would otherwise look like a recovery and fire a duplicate openApp/reconnectApp, - // e.g. on accountID change for a delegate switch). - prevIsInternetReachable = undefined; + if (isReconfigure) { + suppressNextReachabilityRestored = true; + } if (!CONFIG.IS_USING_LOCAL_WEB) { NetInfo.configure({ @@ -315,21 +320,25 @@ function configureAndSubscribe() { setInternetUnreachable(true); } - // Treat false→true as genuine recovery (NetInfo previously confirmed unreachable, now - // confirmed back), and null→true when prev was explicitly reset to null by debug-tool - // toggles (setForceOffline / setFailAllRequests / simulatePoorConnection) that need to - // force a recovery on the next definitive state. Block undefined→true — that's the - // first event on a fresh subscription, not a recovery. + // Treat false→true and null→true as genuine recovery. Both mean NetInfo previously + // lost reachability (false = confirmed unreachable, null = lost track during outage) + // and has now confirmed it's back. Only block undefined→true — that's the initial + // 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(); + } } - // Only track definitive reachable/unreachable states. Ignoring null/undefined keeps the - // synthetic true→null→true sequence NetInfo emits during reconfigure from looking like a - // recovery transition (and lets configureAndSubscribe's `prev = undefined` reset stick). + // 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) { - prevIsInternetReachable = state.isInternetReachable; + suppressNextReachabilityRestored = false; } + prevIsInternetReachable = state.isInternetReachable; }); } diff --git a/tests/unit/NetworkStateReachabilityTest.ts b/tests/unit/NetworkStateReachabilityTest.ts index d93ee7a37779..2674379ce8ea 100644 --- a/tests/unit/NetworkStateReachabilityTest.ts +++ b/tests/unit/NetworkStateReachabilityTest.ts @@ -112,31 +112,15 @@ describe('NetworkState — reachability recovery triggers reconnect', () => { expect(reconnectListener).toHaveBeenCalledTimes(1); }); - test('null→true on a fresh subscription does NOT fire reconnect listener', () => { + test('null→true fires reconnect listener', () => { const reconnectListener = jest.fn(); onReachabilityConfirmed(reconnectListener); - // After a fresh subscription, the first events deliver current state. NetInfo may - // emit null first while the initial Ping is in flight, then true once it completes. - // This is not a recovery — only false→true (or an explicit prev=null reset) is. + // Simulate losing reachability tracking (null) then recovering fireNetInfoState({isInternetReachable: null}); fireNetInfoState({isInternetReachable: true}); - expect(reconnectListener).not.toHaveBeenCalled(); - }); - - test('true→null→true does NOT fire reconnect listener (transient state, no confirmed offline)', () => { - const reconnectListener = jest.fn(); - onReachabilityConfirmed(reconnectListener); - - // Establish a baseline reachable state - fireNetInfoState({isInternetReachable: true}); - // NetInfo briefly loses track without confirming offline (e.g. during a poll gap) - fireNetInfoState({isInternetReachable: null}); - // Reachability comes back. Since we never went through false, this is not a recovery. - fireNetInfoState({isInternetReachable: true}); - - expect(reconnectListener).not.toHaveBeenCalled(); + expect(reconnectListener).toHaveBeenCalledTimes(1); }); test('undefined→true does NOT fire reconnect listener (boot event)', () => { From 775ea64b501d70a8bf45ac115b6c0f577f39826c Mon Sep 17 00:00:00 2001 From: Tomasz Misiukiewicz Date: Thu, 30 Apr 2026 12:22:55 +0200 Subject: [PATCH 4/4] Don't suppress recovery when offline before reconfigure --- src/libs/NetworkState.ts | 5 ++++- tests/unit/NetworkStateReachabilityTest.ts | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/libs/NetworkState.ts b/src/libs/NetworkState.ts index 48b794c45a4c..6ea480ae3946 100644 --- a/src/libs/NetworkState.ts +++ b/src/libs/NetworkState.ts @@ -274,13 +274,16 @@ function configureAndSubscribe() { // 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) { + if (isReconfigure && prevIsInternetReachable !== false) { suppressNextReachabilityRestored = true; } diff --git a/tests/unit/NetworkStateReachabilityTest.ts b/tests/unit/NetworkStateReachabilityTest.ts index 2674379ce8ea..2fbae232f391 100644 --- a/tests/unit/NetworkStateReachabilityTest.ts +++ b/tests/unit/NetworkStateReachabilityTest.ts @@ -199,6 +199,26 @@ describe('NetworkState — reachability recovery triggers reconnect', () => { 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);