feat(analytics-react-native): iOS native connectivity module (SDKRN-5) [2/5]#1819
feat(analytics-react-native): iOS native connectivity module (SDKRN-5) [2/5]#1819Mercy811 wants to merge 13 commits into
Conversation
PR 2 of the stacked SDKRN-5 offline series (stacked on #1802). Replaces the no-op iOS placeholder with the real connectivity module: - `NWPathMonitor` (Network framework, iOS 12+) with an `SCNetworkReachability` fallback for iOS 10/11 (podspec targets iOS 10). - `currentConnectivity()` reads a fresh `SCNetworkReachability` probe on demand, so the initial state JS seeds via `getNetworkConnectivityStatus` is correct even before the path monitor has started (no shared mutable state to race on). - Emits `AmplitudeNetworkConnectivityChanged` only while there are JS listeners. After this PR: iOS has real offline detection; Android is still the no-op stub (replaced in the next PR), so the SDK remains runnable. The `.m` export and bridging header already shipped in #1802 and are unchanged here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
size-limit report 📦
|
Drop the SCNetworkReachability monitoring fallback for iOS 10/11. The podspec stays at iOS/tvOS 10 (no breaking deployment-target bump), but offline detection is now best-effort: below iOS/tvOS 12 the module reports connected unconditionally — both the initial seed and change events — which preserves the pre-offline SDK behavior (send always, existing retry handles failures). The seed guard matters: without a monitor running, seeding offline on an iOS 10/11 device that launches offline would buffer events forever. SCNetworkReachability is kept only for the fresh on-demand initial probe on iOS 12+; the C-callback monitoring path is deleted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Resolve getNetworkConnectivityStatus optimistically (always connected) and delete all SystemConfiguration code. The real initial state arrives via NWPathMonitor's first update: nw_path_monitor_set_update_handler is documented to call the handler "with the current path when start is called", so the first emit after JS subscribes corrects the seed within milliseconds — including the offline-at-launch case. The seed-call-plus-change-events bridge contract is kept (rather than seeding via the first event) to mirror the Amplitude-Kotlin and browser plugins: Android answers the seed with a real ConnectivityManager read, ported from Kotlin, since registerDefaultNetworkCallback fires nothing when the device is offline at registration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Trim the class and seed doc comments to the essentials and link the SDK-header mirror documenting NWPathMonitor's initial-update contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a real iOS React Native connectivity bridge for analytics-react-native, replacing the prior no-op stub so the JS offline-mode stack can react to native network changes on iOS/tvOS.
Changes:
- Implements connectivity monitoring via
NWPathMonitor(iOS/tvOS 12+) and emitsAmplitudeNetworkConnectivityChangedevents. - Starts/stops the monitor based on JS listener presence (
startObserving/stopObserving) to avoid emitting without subscribers. - Keeps
getNetworkConnectivityStatusas an always-connected seed, relying on the first monitor update to correct state shortly after subscription.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review: hasListeners is read by the pathUpdateHandler on monitorQueue but was written from start/stopObserving on the bridge thread, an unsynchronized cross-thread access that could let a queued update read a stale flag and emit after teardown. Write it on monitorQueue so all access is serialized; the monitor lifecycle stays synchronous so cancel() still fires immediately. Also trim the seed comment to drop the third-party SDK-header mirror link flagged in review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Raise the podspec deployment target from 10.0 to 12.0 and drop the now-dead best-effort fallback. iOS 12.0 is the lowest target Xcode 26 accepts (required for App Store uploads as of 2026-04-28) and is exactly NWPathMonitor's minimum, so the monitor can be used unconditionally: remove the #available(iOS 12) guards, the AnyObject workaround, and the canImport(Network) wrapper. No JS/TS change; the bridge contract is unchanged. https://developer.apple.com/news/upcoming-requirements/?id=02032026a Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mercy811
force-pushed
the
sdkrn-5-offline-2-ios
branch
from
d067263 to
83aefce
Compare
Per PR review: set hasListeners and start/stop the NWPathMonitor inside a single monitorQueue.sync block instead of writing the flag async and starting/stopping the monitor separately. sync (not async) guarantees the flag is set before the monitor's first update fires and the monitor is torn down before stopObserving returns, so neither method returns with stale state. monitorQueue is distinct from the calling thread and NWPathMonitor.start only schedules async updates, so there is no deadlock. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8fe90ab. Configure here.
![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: 8fe90abf2e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) -> Void { | ||
| resolve(["isConnected": true]) |
There was a problem hiding this comment.
When an app starts while offline on iOS, this native method still resolves isConnected: true, and the JS plugin awaits it before registering the NWPathMonitor listener to set config.offline. That leaves startup events eligible to be sent as online until the asynchronous monitor callback is delivered, which defeats the initial native connectivity seed that the plugin relies on for offline mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this is intentional, see pr description
There was a problem hiding this comment.
We have sync access to NWMonitor's current path - why would you always start this online?
There was a problem hiding this comment.
current path method seems not to be reliable until the first update. https://developer.apple.com/forums/thread/687477
Always start online here to satisfy the getNetworkConnectivityStatus API by falling back to default. The first update will immediately returns to reflect the current path
There was a problem hiding this comment.
pr description also explains this in more details
getNetworkConnectivityStatus (the seed JS reads once on setup) resolves connected unconditionally on iOS. The real initial state arrives via the monitor's first update: nw_path_monitor_set_update_handler is documented (SDK header) to call the handler "with the current path when start is called", so the first emit after JS subscribes corrects the seed within milliseconds — including the offline-at-launch case. The seed-call contract is kept (instead of seed-by-first-event) to mirror Amplitude-Kotlin and the browser plugin: Android will answer the seed with a real ConnectivityManager read, which is required there because registerDefaultNetworkCallback fires nothing when the device is offline at registration.
Source: /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk/System/Library/Frameworks/Network.framework/Headers/path_monitor.h
There was a problem hiding this comment.
Thanks Chris for the clear explanation. I got it why you against the current implementation now.
The latter might sound better on the context of how NWPathMonitor behaves but IHMO the former one is more practical. The latter one works perfectly in the assumption that network update listener is always available. This is true for iOS because of Apple Store requirement of min iOS version but not for Android. If no network update, the SDK will remain offline forever unless we add another interface to detect if network listener is available
There is no impact to waiting for connectivity check to complete. On the JS side, we actually already await getNetworkConnectivityStatus(). We could let iOS getNetworkConnectivityStatus() returns the real connectivity status but it's not worth it because the real status is in next update in a few ms.
In edge case, if the first update is somehow delayed, there will be a failed attempt. But most of the time, it behaves like starting with the real connectivity status. On the contrary, the latter means there will always be a failed attempt.
There was a problem hiding this comment.
Path monitoring works in a roughly consistent manner across platforms - only having failed requests mark the SDK offline isn't just an iOS only pattern, it's the more reliable approach to delivery on every platform. We don't have to adopt this now but we should consider it in a future update.
Is the concern that NWPathMonitor may not send a callback? Apple's documentation (especially for older C/ObjC apis) can be pretty inconsistent, but this is explicitly documented in the source header:
* @function nw_path_monitor_set_update_handler
* @abstract
* Sets the client update handler. This block will be called with the
* current path when start is called and any time the path changes.
Why would we want to make a request that we believe will fail? Especially if we are okay waiting for an actual connectivity status on other platforms.
The updated code to reuse NWPathHandler also can fail with the hardcoded connected: true state if react restarts the plugin - we won't get another state update if NWPathMonitor is never restarted.
There was a problem hiding this comment.
The point is that path monitoring is always available on the given iOS min version and above but it's not the true for Android. Example: on Android API 22, if start with a captive network, and then later gets real network, there won't be a network update event so the SDK will remain offline forever even though there is network.
The source header comment is exactly what I mentioned in PR description. The logic here is that since we know the first callback of NWPathMonitor represents the current status, so we just hardcoded connected: true and the actual callback will arrive soon
There was a problem hiding this comment.
Sync over zoom. We will keep the current pattern of get current status + network change update. RCTEventEmitter guarantees the lifecycle that startObserving is called once and callbacks are sent after pathMonitor.start().
I will update getNetworkConnectivityStatus() to use currentPath return the actual network status.
There was a problem hiding this comment.
currentPath is only valid after the first callback fires, after start(). Its stays .unsatisfied "until the first invocation. So just flip it to false. The rationale here is that we hold sending events until network connectivity is confirmed to close the small time window of being online before hearing from path monitor. Even the time window is small, chances are events being sent
…-5) [3/5] Replace the no-op Android connectivity stub (shipped in #1802) with a real ConnectivityManager-based implementation, the Android analog of the iOS module in #1819. - Extract the framework-free networking logic into ConnectivityChecker (no React imports) so it can be unit-tested with Robolectric. It registers a default-network callback on API 24+ and falls back to a request-scoped callback on API 21-23, seeds current state, dedupes same-state callbacks, and gates NET_CAPABILITY_VALIDATED behind API >= 23 so API 21-22 devices do not read as permanently offline. - Reduce AmplitudeReactNativeConnectivityModule to a thin React bridge that seeds JS via getNetworkConnectivityStatus and forwards changes as AmplitudeNetworkConnectivityChanged events, guarded on an active catalyst instance, and unregisters in invalidate()/onCatalystInstanceDestroy(). - Add the ACCESS_NETWORK_STATE permission to both manifests so it auto-merges into the host app. Part of SDKRN-24. The package registration (AmplitudeReactNativePackage) already binds the module from the [1/5] stub, so it is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-5) [3/5] Replace the no-op Android connectivity stub (shipped in #1802) with a real ConnectivityManager-based implementation, the Android analog of the iOS module in #1819. - Extract the framework-free networking logic into ConnectivityChecker (no React imports) so it can be unit-tested with Robolectric. It registers a default-network callback on API 24+ and falls back to a request-scoped callback on API 21-23, seeds current state, dedupes same-state callbacks, and gates NET_CAPABILITY_VALIDATED behind API >= 23 so API 21-22 devices do not read as permanently offline. - Re-derive the live state from the active network on every callback (via currentConnectivity()) rather than trusting the event type: onAvailable can fire before a network is VALIDATED (or on a captive portal), and the API 21-23 request-based callback reports onLost for a non-default network while another is still up. This avoids a premature online flush and a spurious offline while another network is active. - Reduce AmplitudeReactNativeConnectivityModule to a thin React bridge that seeds JS via getNetworkConnectivityStatus and forwards changes as AmplitudeNetworkConnectivityChanged events, guarded on an active catalyst instance, and unregisters in invalidate()/onCatalystInstanceDestroy(). - Add the ACCESS_NETWORK_STATE permission to both manifests so it auto-merges into the host app. Part of SDKRN-24. The package registration (AmplitudeReactNativePackage) already binds the module from the [1/5] stub, so it is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-5) [3/5] Replace the no-op Android connectivity stub (shipped in #1802) with a real ConnectivityManager-based implementation, the Android analog of the iOS module in #1819. - Extract the framework-free networking logic into ConnectivityChecker (no React imports) so it can be unit-tested with Robolectric. It registers a default-network callback on API 24+ and falls back to a request-scoped callback on API 21-23, seeds current state, dedupes same-state callbacks, and gates NET_CAPABILITY_VALIDATED behind API >= 23 so API 21-22 devices do not read as permanently offline. currentConnectivity() is best-effort: it assumes online when it can't tell (no ConnectivityManager, or a query that throws on a missing ACCESS_NETWORK_STATE), so it never wrongly pins the SDK offline. - Re-derive the live state from the active network on every callback (via currentConnectivity()) rather than trusting the event type: onAvailable can fire before a network is VALIDATED (or on a captive portal), and the API 21-23 request-based callback reports onLost for a non-default network while another is still up. This avoids a premature online flush and a spurious offline while another network is active. - Reduce AmplitudeReactNativeConnectivityModule to a thin React bridge that seeds JS via getNetworkConnectivityStatus and forwards changes as AmplitudeNetworkConnectivityChanged events, guarded on an active catalyst instance, and unregisters in invalidate()/onCatalystInstanceDestroy(). - Add the ACCESS_NETWORK_STATE permission to both manifests so it auto-merges into the host app. Part of SDKRN-24. The package registration (AmplitudeReactNativePackage) already binds the module from the [1/5] stub, so it is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| ) -> Void { | ||
| resolve(["isConnected": true]) |
There was a problem hiding this comment.
We have sync access to NWMonitor's current path - why would you always start this online?
Revert local-testing leaks flagged in review on #1819: DEVELOPMENT_TEAM, the iphoneos PRODUCT_BUNDLE_IDENTIFIER, and the run scheme (Release + no debugger) all back to the repo defaults so the shared example project isn't tied to a personal signing identity or Release-by-default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove the stopMonitoring() call at the top of startMonitoring. Per review on #1819: RCTEventEmitter only calls startObserving on the 0->1 listener transition and stopObserving already nils the monitor on 1->0, so pathMonitor is always nil here and the guard never fired. Also revert incidental CocoaPods pod-install churn (empty inputPaths/ outputPaths) from the example app pbxproj so the PR diff stays minimal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| // monitorQueue, where the pathUpdateHandler reads the flag. sync (not | ||
| // async) so the flag is set before the monitor's first update fires, and | ||
| // so the monitor is fully torn down before stopObserving returns. | ||
| monitorQueue.sync { |
There was a problem hiding this comment.
This will overwrite the current pathMonitor, which will either leak or deinit in a running state. It's fine (even preferable) to reuse the same monitor, but we shouldn't just replace it.
something like:
monitorQueue.sync {
guard !hasListeners else {
return
}
hasListeners = true
startMonitoring()
}
Though this also will affect the initial callback, as you'll no longer get it on a reused instance.
There was a problem hiding this comment.
I did a refactor to reuse the single long-live monitor
- initialize at the beginning but not start it
- start it in startObserving()
- no cancel() in stopObserving(). Only cancel it when deinit
| ) -> Void { | ||
| resolve(["isConnected": true]) |
There was a problem hiding this comment.
There's two basic strategies for connectivity monitoring:
- Conditionally attempt connections only when a connectivity monitor says you have a connection.
- Always attempt connections unless they've previously failed, use connectivity checking only to notify for new connection opportunities when offline.
We know connectivity monitoring is not exact. In either approach you still have update the sdk online status both from connectivity checks and actual connection results in your http client. Usually, the latter approach is recommended for mobile because of this uncertainty, though I don't think we currently use it in our SDKs.
Only for iOS in RN do we seem to be mixing these approaches. If the initial network call will already be delayed, is there any impact to waiting a few more ms for the connectivity check to complete? What are the actual effects on the SDK if we start offline vs online?
Hold one NWPathMonitor for the module's lifetime instead of recreating it per subscribe. It's created (with its pathUpdateHandler) in init, started in startObserving, and cancelled only in deinit — never replaced. stopObserving just clears hasListeners; it must not cancel, since a cancelled NWPathMonitor can't be restarted. Drops the startMonitoring/stopMonitoring helpers. Per review on #1819: removes the pathMonitor overwrite/leak (r3424522147), the replace-on-start pattern (r3416357802), and the off-queue deinit access (r3391719568) — pathMonitor is now a write-once let, so deinit's read is race-free without hopping onto monitorQueue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
getNetworkConnectivityStatus now resolves isConnected: false instead of true. Seeding offline means startup events buffer until the monitor's first update (delivered right after JS subscribes) reports the real status, rather than risk sending events while the device is actually offline at launch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-5) [3/5] Replace the no-op Android connectivity stub (shipped in #1802) with a real ConnectivityManager-based implementation, the Android analog of the iOS module in #1819. - Extract the framework-free networking logic into ConnectivityChecker (no React imports) so it can be unit-tested with Robolectric. It registers a default-network callback on API 24+ and falls back to a request-scoped callback on API 21-23, seeds current state, dedupes same-state callbacks, and gates NET_CAPABILITY_VALIDATED behind API >= 23 so API 21-22 devices do not read as permanently offline. currentConnectivity() is best-effort: it assumes online when it can't tell (no ConnectivityManager, or a query that throws on a missing ACCESS_NETWORK_STATE), so it never wrongly pins the SDK offline. - Re-derive the live state from the active network on every callback (via currentConnectivity()) rather than trusting the event type: onAvailable can fire before a network is VALIDATED (or on a captive portal), and the API 21-23 request-based callback reports onLost for a non-default network while another is still up. This avoids a premature online flush and a spurious offline while another network is active. - Reduce AmplitudeReactNativeConnectivityModule to a thin React bridge that seeds JS via getNetworkConnectivityStatus and forwards changes as AmplitudeNetworkConnectivityChanged events, guarded on an active catalyst instance, and unregisters in invalidate()/onCatalystInstanceDestroy(). - Add the ACCESS_NETWORK_STATE permission to both manifests so it auto-merges into the host app. Part of SDKRN-24. The package registration (AmplitudeReactNativePackage) already binds the module from the [1/5] stub, so it is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
PR 2 of 5 in the SDKRN-5 offline-mode stack. Stacked on #1802 (base branch
sdkrn-5-offline-1-js-core).Replaces the no-op iOS placeholder (shipped in #1802) with the real iOS connectivity module, and raises the deployment target so it can use
NWPathMonitordirectly:NWPathMonitor's minimum.getNetworkConnectivityStatus(the seed JS reads once on setup) resolves connected unconditionally on iOS. The real initial state arrives via the monitor's first update:nw_path_monitor_set_update_handleris documented (SDK header) to call the handler "with the current path when start is called", so the first emit after JS subscribes corrects the seed within milliseconds — including the offline-at-launch case. The seed-call contract is kept (instead of seed-by-first-event) to mirror Amplitude-Kotlin and the browser plugin: Android will answer the seed with a realConnectivityManagerread, which is required there becauseregisterDefaultNetworkCallbackfires nothing when the device is offline at registration.Source: /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk/System/Library/Frameworks/Network.framework/Headers/path_monitor.h
Non-breaking (behavior) / minimum bump
After this PR: iOS has real offline detection; Android is still the no-op stub (replaced in the next PR), so the SDK remains runnable at every step. The min iOS/tvOS bump (10→12) is conventionally a breaking change, but no shippable app can target below iOS 12 (Xcode 26's floor), so the practical impact is nil.
main[5/5]Test plan
The test has to be on a physical device because iOS simulators' network availability depends on the host network. It only checks whether it's connected to a router instead of checking whether it has network, and the network change doesn't accurately propagate from the host back to the iOS simulator via NWPathMonitor.
Test 1:
ScreenRecording_06-12-2026.14-23-58_1.mov
Test 2:
ScreenRecording_06-12-2026.14-26-32_1.MP4
🤖 Generated with Claude Code
Note
Medium Risk
Changes native event timing and offline gating on iOS and bumps the declared deployment target; JS API is unchanged but behavior now reflects real network state.
Overview
Replaces the iOS no-op
AmplitudeReactNativeConnectivitystub with real offline detection usingNWPathMonitor, emittingAmplitudeNetworkConnectivityChangedwhen path status changes.The podspec raises minimum iOS/tvOS from 10.0 to 12.0 so the Network framework can be used without availability guards (aligned with Xcode 26 / App Store floor). Monitoring starts and stops with React Native’s
startObserving/stopObserving, withhasListenersand monitor lifecycle synchronized on a dedicated queue to avoid stale emits after teardown.getNetworkConnectivityStatusstill seedsisConnected: true; the first monitor callback is expected to correct offline-at-launch shortly after subscribe.Reviewed by Cursor Bugbot for commit 8fe90ab. Configure here.