Repository navigation
Conversation
…onnect Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
@Krishna2323 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 252ef99d05
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR fixes a case where the app could remain stuck in an “offline” state (INTERNET_UNREACHABLE hard stop) even while real API requests are succeeding (e.g., on VPN/proxy networks where NetInfo’s Ping can time out indefinitely). It does so by letting any successful request act as proof of connectivity, clearing hard stops and triggering a reconnect to backfill missed updates.
Changes:
- Added a “success” signal to
FailureTracker(onSuccess) that fires on every successful request. - Wired
NetworkStateto clear theINTERNET_UNREACHABLEhard stop (and reset sustained-failure state) when a request succeeds, then schedule a jittered reconnect. - Added unit tests covering recovery-by-success behavior, including cases where both hard stops are set.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/unit/NetworkStateReachabilityTest.ts | Adds unit tests verifying successful-request recovery clears INTERNET_UNREACHABLE, resets hard stops, and triggers a single (jittered) reconnect. |
| src/libs/NetworkState.ts | Subscribes to request-success events to clear hard stops when INTERNET_UNREACHABLE is set, centralizes hard-stop clearing + jittered reconnect scheduling. |
| src/libs/FailureTracker.ts | Introduces onSuccess listeners and emits them on each recordSuccess() call. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No product review needed. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMonosnap.screencast.2026-07-28.18-07-38.mp4 |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.46-0 🚀
|
|
🤖 Help site review: no changes required I reviewed the changes in this PR and no updates to Expensify's help site files under Why: This is a purely internal, non-UI networking fix. It touches I also confirmed the help site has no existing articles describing the offline indicator, internet reachability, or connectivity recovery that would need to be kept in sync ( @adhorodyski, since no docs changes were required, there is no linked help site PR to review. If you believe a user-facing behavior here should be documented, let me know and I'll draft one. |
|
Hi @adhorodyski For QA team only step 2, right?
|
|
@izarutskaya yes |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.46-10 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
The app could get stuck showing the offline indicator: only NetInfo's Ping could clear the
INTERNET_UNREACHABLEhard stop, but on some networks (VPNs) that check can time out forever while every other request succeeds. Reads and side-effect commands bypass the paused queue and keep working, proving the app is online — the code just ignored that proof. This wiresFailureTrackerto announce every successful request, andNetworkStatenow clears both hard stops (INTERNET_UNREACHABLEandSUSTAINED_FAILURES) and reconnects when one arrives while marked unreachable. Recovery via Ping is unchanged.Note: this should land together with (or after) #96883. The stuck offline pill was accidentally warning users that the Pusher websocket is dead — once the pill is honest, that failure needs its own fix (the pong watchdog in #96883).
Fixed Issues
$ #93966
PROPOSAL:
Tests
npx jest tests/unit/NetworkStateReachabilityTest.ts(4 new tests cover the recovery).Offline tests
This change is itself part of the offline detection system (recovering from the
INTERNET_UNREACHABLEhard stop) and is covered by the unit tests intests/unit/NetworkStateReachabilityTest.ts.QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A — non-UI change, verified via unit tests.
Android: mWeb Chrome
N/A — non-UI change, verified via unit tests.
iOS: Native
N/A — non-UI change, verified via unit tests.
iOS: mWeb Safari
N/A — non-UI change, verified via unit tests.
MacOS: Chrome / Safari
N/A — non-UI change, verified via unit tests.