fix: only treat a reachability confirmation as a recovery when the app was offline#96504
fix: only treat a reachability confirmation as a recovery when the app was offline#96504adhorodyski wants to merge 7 commits into
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
… the effective URL Extract the repeated prev-reset/token/refresh sequence into armReachabilityRecovery(), and compare the effective reachability URL instead of mirroring the raw staging toggle — the raw value can change without changing the URL (and vice versa on staging, where null falls back to the environment default). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 588423ac34
ℹ️ 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".
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@mkhutornyi 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.movweb2.mov |
|
Suggested QA Steps: Setup (how to observe reconnects): Open the app in Chrome, open DevTools → Network tab, and type 1. Healthy boot — no phantom reconnects
2. Force Offline toggle → exactly one reconnect
3. Real connectivity loss → one reconnect on return
4. Rapid offline/online cycling doesn't storm
5. Staging-server toggle doesn't trigger a storm (staging/dev builds only)
In every test:
|
|
I can't run change requests for your access level. I can investigate, or file an issue in Failed after 0s · 0 tools used · view debug log |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 954a4140c0
ℹ️ 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".
| // 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) { |
There was a problem hiding this comment.
When the offline period was caused only by noRadioActive and NetInfo never reported isInternetReachable=false (for example isConnected=false with reachability still null, followed by isConnected=true/isInternetReachable=true), the listener calls setHasRadio(true) before this check, so getIsOffline() is already false and onReachabilityRestored() is skipped. Reconnect.ts only calls reconnectApp() from onReachabilityConfirmed, so this radio-only recovery can flush the queue but miss the data resync that fetches skipped Onyx updates; capture the pre-NetInfo-event offline state or explicitly treat radio recovery as something to recover from.
Useful? React with 👍 / 👎.
3 participants
Explanation of Change
Production traces show serial Ping+ReconnectApp storms (up to 23 ReconnectApp calls in 52 seconds in one session) where nothing about the network changed. Root cause:
NetworkStatefiresreconnectApp()from raw NetInfonull→truetransitions, and those get manufactured with no real connectivity change by:undefined→null→trueslipping past the dead!== undefinedguard.NetInfo.refresh()re-emitting cached state after the debug paths resetprevIsInternetReachable = null.SHOULD_USE_STAGING_SERVEROnyx callback rebuilding the NetInfo subscription on every write, where each rebuild fires its own extra Ping.Fix:
getIsOffline()is true at the moment reachability is confirmed, or when a one-shotpendingReachabilityRecoverytoken was armed. The three debug paths (setForceOffline(false),setFailAllRequests(false),simulatePoorConnection(false)) arm the token instead of exploiting the transition loophole, so debug recovery stays Ping-verified.suppressNextReachabilityRestored, the old post-reconfigure band-aid. The gate above covers it.SHOULD_USE_STAGING_SERVERcallback rebuilds the subscription only when the effective reachability URL changed. Same-value rewrites, and toggle writes that do not change the URL (like on production, where the staging flag is forced off), no longer tear down NetInfo.Tests: one existing test (
null→true fires reconnect listener) asserted the buggy behavior and was inverted. New regression tests pin that a barenull→trueand repeated fake pairs never fire a reconnect, thatfalse→trueandfalse→null→truefire exactly once, that force-offline debug recovery still works, and that staging toggle writes which do not change the URL do not reconfigure. All affected suites pass, includingSequentialQueueTestandAPITest.Fixed Issues
$ #96634
PROPOSAL:
Tests
Offline tests
Same as steps 2 and 3 above. Both exercise the offline→online recovery path.
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari