fix: only treat a reachability confirmation as a recovery when the app was offline

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:**
 

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();
+        });
     },
 });
 

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);

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<void, [{reachabilityUrl: string}]>;
 
         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`);