diff --git a/contributingGuides/NETWORK_STATE_DETECTION.md b/contributingGuides/NETWORK_STATE_DETECTION.md index 0b91a1f69d82..03cd4afe0be0 100644 --- a/contributingGuides/NETWORK_STATE_DETECTION.md +++ b/contributingGuides/NETWORK_STATE_DETECTION.md @@ -76,8 +76,7 @@ This layer uses `@react-native-community/netinfo` to detect whether the device h The NetInfo listener tracks `isInternetReachable` transitions in both directions: - **any non-`false` → `false`** (`true→false`, `null→false`, `undefined→false`) — `api/Ping` failed. Sets the `internetUnreachable` hard stop. Only `false→false` is skipped (already offline). This covers cold start with no internet (`null→false` after the initial indeterminate event), post-recovery resets, and normal online→offline transitions. Without this, an idle user would never see the offline indicator because no API requests are failing. -- **`false` → `true`** and **`null` → `true`** — `api/Ping` succeeded after a previous failure. Triggers `onReachabilityRestored()` which clears all hard stops and fires reconnect listeners. -- **`undefined` → `true`** — the initial event on subscribe, delivering current state. This is **not** treated as a recovery to prevent duplicate `openApp()`/`reconnectApp()` calls on boot. +- **any non-`true` → `true`** — `api/Ping` succeeded. This counts as a recovery only when there is something to recover from: the app was offline at that moment, or a debug tool just cleared its own hard stop and asked for a recovery (`pendingReachabilityRecovery`). A recovery calls `onReachabilityRestored()`, which clears all hard stops and fires reconnect listeners. Any other confirmation (boot, re-subscription after a reconfigure, a `refresh()` re-emit) is a re-read of a network that never went down and is ignored, so it cannot fire duplicate `openApp()`/`reconnectApp()` calls. **Platform behavior:** diff --git a/src/libs/NetworkState.ts b/src/libs/NetworkState.ts index bf42ab856d38..360a811f3029 100644 --- a/src/libs/NetworkState.ts +++ b/src/libs/NetworkState.ts @@ -24,7 +24,8 @@ let unsubscribeNetInfo: (() => void) | null = null; let prevIsInternetReachable: boolean | null | undefined; let isPoorConnectionSimulated: boolean | undefined; let networkTimeSkew = 0; -let suppressNextReachabilityRestored = false; +let pendingReachabilityRecovery = false; +let configuredReachabilityUrl: string | undefined; // Subscriber sets const listeners = new Set<() => void>(); @@ -150,6 +151,17 @@ function setSustainedFailures(active: boolean) { } } +/** + * The debug paths clear their hard stop right away, so when the refreshed Ping confirms + * reachability the app already looks online and the listener would ignore it. The token + * allows that one recovery. Resetting prev makes the listener see a transition again. + */ +function armReachabilityRecovery() { + prevIsInternetReachable = null; + pendingReachabilityRecovery = true; + NetInfo.refresh(); +} + /** * Called when shouldForceOffline changes in Onyx (debug tool). */ @@ -159,12 +171,7 @@ function setForceOffline(force: boolean) { updateState(); if (!force) { - // Reset so the NetInfo listener sees a genuine transition (e.g. null→true) - // and fires onReachabilityRestored(). Without this, prevIsInternetReachable - // is already true (we track real state during force-offline) so the listener - // sees true→true and skips reconnect. - prevIsInternetReachable = null; - NetInfo.refresh(); + armReachabilityRecovery(); } } @@ -186,8 +193,7 @@ function setFailAllRequests(failAll: boolean) { resetFailureCounters(); updateState(); - prevIsInternetReachable = null; - NetInfo.refresh(); + armReachabilityRecovery(); } } @@ -247,9 +253,7 @@ function simulatePoorConnection(shouldSimulate: boolean) { simulatedOffline = false; updateState(); - // Reset so NetInfo listener sees a transition and fires onReachabilityRestored(). - prevIsInternetReachable = null; - NetInfo.refresh(); + armReachabilityRecovery(); } } @@ -270,32 +274,25 @@ function setRandomNetworkStatus(initialCall = false) { // --- NetInfo configuration and subscription --- +function buildReachabilityUrl(): string { + return `${getCommandURL({command: 'Ping'})}accountID=${accountID ?? 'unknown'}`; +} + /** * Configure NetInfo with the reachability URL and subscribe to state changes. * 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; - } + configuredReachabilityUrl = buildReachabilityUrl(); if (!CONFIG.IS_USING_LOCAL_WEB) { NetInfo.configure({ - reachabilityUrl: `${getCommandURL({command: 'Ping'})}accountID=${accountID ?? 'unknown'}`, + reachabilityUrl: configuredReachabilityUrl, reachabilityMethod: 'GET', reachabilityTest: (response) => { if (!response.ok) { @@ -329,24 +326,18 @@ 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(). - if (!shouldForceOffline && state.isInternetReachable === true && prevIsInternetReachable !== true && prevIsInternetReachable !== undefined) { - if (suppressNextReachabilityRestored) { - Log.info(`[NetworkState] Suppressing recovery on first stable state after reconfigure (${prevIsInternetReachable}→true)`); - } else { + // Treat a confirmed-reachable event as a recovery only when the app was actually offline + // or a debug path asked for one. Everything else (boot, re-subscription, refresh() re-emits) + // is a re-read of an unchanged network and must not fire reconnectApp. + if (!shouldForceOffline && state.isInternetReachable === true && prevIsInternetReachable !== true) { + if (getIsOffline() || pendingReachabilityRecovery) { + pendingReachabilityRecovery = false; Log.info(`[NetworkState] Internet reachability restored (${prevIsInternetReachable}→true)`); onReachabilityRestored(); + } else { + Log.info(`[NetworkState] Ignoring reachability re-confirmation (${prevIsInternetReachable}→true) since the app was never offline`); } } - // 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; }); } @@ -374,15 +365,19 @@ Onyx.connectWithoutView({ }, }); -// Re-target the reachability ping when the in-app staging-server toggle flips at runtime. -// queueMicrotask defers configureAndSubscribe so ApiUtils' Onyx callback for the same key — -// registered later and therefore firing later on the same tick — has already updated -// shouldUseStagingServer before we re-sample getApiRoot(). Removing the defer causes -// configureAndSubscribe to read the previous toggle state and invert the URL on every flip. +// Re-target the reachability ping when the staging-server toggle flips at runtime. +// queueMicrotask waits for ApiUtils' callback on the same key, which owns the flag behind +// getApiRoot(). Skip the rebuild when the URL is unchanged: rebuilding tears down NetInfo +// state and fires extra Pings, and the raw toggle can flip without changing the URL. Onyx.connectWithoutView({ key: ONYXKEYS.SHOULD_USE_STAGING_SERVER, callback: () => { - queueMicrotask(configureAndSubscribe); + queueMicrotask(() => { + if (buildReachabilityUrl() === configuredReachabilityUrl) { + return; + } + configureAndSubscribe(); + }); }, }); diff --git a/tests/unit/NetworkStateReachabilityTest.ts b/tests/unit/NetworkStateReachabilityTest.ts index f8c2b0d48540..56183567ee15 100644 --- a/tests/unit/NetworkStateReachabilityTest.ts +++ b/tests/unit/NetworkStateReachabilityTest.ts @@ -113,17 +113,45 @@ describe('NetworkState — reachability recovery triggers reconnect', () => { expect(reconnectListener).toHaveBeenCalledTimes(1); }); - test('null→true fires reconnect listener', () => { + test('null→true does NOT fire reconnect listener when the app was never offline (fake recovery)', () => { + // Boot, post-configure, and refresh() all look like this: + // NetInfo emits null while a Ping is in flight, then true when it succeeds. + // No hard stop was active, so there is nothing to recover from. const reconnectListener = jest.fn(); onReachabilityConfirmed(reconnectListener); - // Simulate losing reachability tracking (null) then recovering + fireNetInfoState({isInternetReachable: null}); + fireNetInfoState({isInternetReachable: true}); + + expect(reconnectListener).not.toHaveBeenCalled(); + }); + + test('false→null→true fires reconnect listener once (real outage with lost tracking)', () => { + const reconnectListener = jest.fn(); + onReachabilityConfirmed(reconnectListener); + + // Confirmed unreachable, so the internetUnreachable hard stop is active + fireNetInfoState({isInternetReachable: false}); + // Tracking lost mid-outage, then recovery confirmed fireNetInfoState({isInternetReachable: null}); fireNetInfoState({isInternetReachable: true}); expect(reconnectListener).toHaveBeenCalledTimes(1); }); + test('repeated re-confirmations after a fake-recovery shape never fire reconnect', () => { + // Re-emitting null/true pairs should not result in a reconnect + const reconnectListener = jest.fn(); + onReachabilityConfirmed(reconnectListener); + + for (let i = 0; i < 5; i++) { + fireNetInfoState({isInternetReachable: null}); + fireNetInfoState({isInternetReachable: true}); + } + + expect(reconnectListener).not.toHaveBeenCalled(); + }); + test('undefined→true does NOT fire reconnect listener (boot event)', () => { const reconnectListener = jest.fn(); onReachabilityConfirmed(reconnectListener); diff --git a/tests/unit/NetworkStateTest.ts b/tests/unit/NetworkStateTest.ts index 5fcba98f2510..8a6bc9d5987b 100644 --- a/tests/unit/NetworkStateTest.ts +++ b/tests/unit/NetworkStateTest.ts @@ -1,3 +1,5 @@ +import {getCommandURL} from '@libs/ApiUtils'; + import { getDBTimeWithSkew, getIsOffline, @@ -36,6 +38,7 @@ jest.mock('@react-native-community/netinfo', () => ({ })); const mockPingUrl = 'https://test-api.expensify.com/api/Ping?'; +const mockStagingPingUrl = 'https://staging-test-api.expensify.com/api/Ping?'; jest.mock('@libs/ApiUtils', () => ({ __esModule: true, getApiRoot: jest.fn(() => 'https://test-api.expensify.com/'), @@ -538,6 +541,7 @@ describe('NetworkState', () => { const configureMock = NetInfo.configure as jest.Mock; beforeEach(async () => { + jest.mocked(getCommandURL).mockReturnValue(mockPingUrl); configureMock.mockClear(); await Onyx.clear(); await waitForBatchedUpdates(); @@ -563,16 +567,43 @@ describe('NetworkState', () => { expect(configureMock.mock.calls.at(-1)?.[0].reachabilityUrl).toBe(`${mockPingUrl}accountID=2`); }); - test('SHOULD_USE_STAGING_SERVER change triggers a reconfigure', async () => { + test('SHOULD_USE_STAGING_SERVER change triggers a reconfigure when the reachability URL changes', async () => { const callsBefore = configureMock.mock.calls.length; + jest.mocked(getCommandURL).mockReturnValue(mockStagingPingUrl); await Onyx.set(ONYXKEYS.SHOULD_USE_STAGING_SERVER, true); await waitForBatchedUpdates(); expect(configureMock.mock.calls.length).toBeGreaterThan(callsBefore); + expect(configureMock.mock.calls.at(-1)?.[0].reachabilityUrl).toBe(`${mockStagingPingUrl}accountID=unknown`); + }); + + test('SHOULD_USE_STAGING_SERVER same-value rewrite does NOT reconfigure', async () => { + jest.mocked(getCommandURL).mockReturnValue(mockStagingPingUrl); + await Onyx.set(ONYXKEYS.SHOULD_USE_STAGING_SERVER, true); + await waitForBatchedUpdates(); + const callsAfterFlip = configureMock.mock.calls.length; + + await Onyx.set(ONYXKEYS.SHOULD_USE_STAGING_SERVER, true); + await waitForBatchedUpdates(); + + expect(configureMock.mock.calls.length).toBe(callsAfterFlip); + }); + + test('SHOULD_USE_STAGING_SERVER flip that does not change the reachability URL does NOT reconfigure', async () => { + // e.g. production, where ApiUtils forces the effective flag off regardless of the toggle + const callsBefore = configureMock.mock.calls.length; + + await Onyx.set(ONYXKEYS.SHOULD_USE_STAGING_SERVER, true); + await waitForBatchedUpdates(); + + expect(configureMock.mock.calls.length).toBe(callsBefore); }); test('reachability URL falls back to accountID=unknown when SESSION has no accountID', async () => { + await Onyx.merge(ONYXKEYS.SESSION, {accountID: 123}); + await waitForBatchedUpdates(); + await Onyx.set(ONYXKEYS.SESSION, null); await waitForBatchedUpdates(); expect(configureMock.mock.calls.at(-1)?.[0].reachabilityUrl).toBe(`${mockPingUrl}accountID=unknown`);