Skip to content

feat(analytics-react-native): Android native connectivity module + Robolectric tests (SDKRN-5) [3/5]#1831

Merged
Mercy811 merged 46 commits into
mainfrom
sdkrn-5-offline-4-android
Jun 25, 2026
Merged

feat(analytics-react-native): Android native connectivity module + Robolectric tests (SDKRN-5) [3/5]#1831
Mercy811 merged 46 commits into
mainfrom
sdkrn-5-offline-4-android

Conversation

@Mercy811

@Mercy811 Mercy811 commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Part of SDKRN-24 (Android native connectivity module) and SDKRN-25 (Android Robolectric unit tests).

PR 3 of 5 in the SDKRN-5 offline-mode stack. Stacked on #1819 (base branch sdkrn-5-offline-2-ios).

Replaces the no-op Android placeholder (shipped in #1802) with the real Android connectivity module — the Android analog of the iOS module in #1819folds in the [4/5] Robolectric unit tests, and wires those tests into CI (a new android-unit job in rn-smoke). Offline connectivity is scoped to API 23+.

After this PR both platforms have real offline detection: JS (#1802) + iOS (#1819) + Android (this) all in place, so the SDK is fully functional on every platform at this point in the stack.

Design — framework-free ConnectivityChecker + thin bridge

The risky part (the ConnectivityManager networking logic) is extracted into a new framework-free ConnectivityChecker (no React imports — only android.net.* + a (Boolean) -> Unit listener), so it can be unit-tested with plain Robolectric + mockk without React Native on the classpath. AmplitudeReactNativeConnectivityModule is reduced to a thin React bridge that delegates to it and owns only the React surface (promise resolution + RCTDeviceEventEmitter emit + the hasActiveCatalystInstance() guard). This mirrors Amplitude-Kotlin's AndroidNetworkConnectivityChecker.

Key behavior:

  • getNetworkConnectivityStatus answers the JS seed with a real ConnectivityManager read (required on Android because registerDefaultNetworkCallback fires nothing when the device is offline at registration — unlike iOS, which seeds optimistically and corrects via the monitor's first update).
  • Offline connectivity requires API 23+ (it relies on NetworkCapabilities validation). Below 23, currentConnectivity() best-effort reports online and monitoring no-ops — consistent with React Native's own floor (minSdk 23 since RN 0.74, 24 since RN 0.76). hasInternetCapability() requires both INTERNET and VALIDATED, so captive portals read as offline → events queue rather than get lost.
  • start() uses registerDefaultNetworkCallback on API 24+ and a request-scoped registerNetworkCallback on API 23; it is idempotent and swallows SecurityException. Same-state callbacks are deduped.
  • currentConnectivity() is best-effort: assumes online whenever it can't tell — no ConnectivityManager, or a query that throws (missing ACCESS_NETWORK_STATESecurityException, or a device ConnectivityManager crash) — so a probe failure never pins the SDK offline. (Mirrors Amplitude-Kotlin.)
  • Each NetworkCallback event (onAvailable/onLost/onCapabilitiesChanged) re-derives state via currentConnectivity() rather than trusting the event type — so we don't flip online before a network is VALIDATED (premature flush into a captive portal), and don't flip offline when one matching network drops while another is still active on the API 23 request-scoped callback. (Addresses codex review.)
  • The callback is released in invalidate()/onCatalystInstanceDestroy().
  • ACCESS_NETWORK_STATE is added to both manifests and auto-merges into the host app.

The package registration (AmplitudeReactNativePackage) already binds the module from the [1/5] stub, so it is unchanged here.

Why [3/5] + [4/5] together

The Robolectric suite is two focused classes (ConnectivityCheckerTest, AmplitudeReactNativeConnectivityModuleTest — 25 tests) plus standard gradle test deps; build.gradle's entire diff is test-only infra. They test exactly the two classes this PR adds, so shipping them together keeps the module and its coverage reviewable as one unit. (Tracked as SDKRN-25 / the [4/5] row below.)

PR Adds Base
#1802 [1/5] JS plugin + wiring + tests + no-op native stubs main
#1819 [2/5] Real iOS module (replaces iOS stub) #1802
#1823 [2.1/5] Native iOS XCTest coverage #1819
this [3/5] Real Android module (replaces Android stub) + [4/5] Robolectric tests + CI #1819
[4/5] Robolectric tests — folded into this PR
[5/5] Example app — already merged (#1803)

Changes

  • ConnectivityChecker.kt (new): framework-free connectivity logic — API 23+ only (best-effort online below); registration branch (API 24+ default callback vs API 23 request-scoped), seed via currentConnectivity(), hasInternetCapability() requires INTERNET && VALIDATED, same-state dedupe, callbacks re-derive via currentConnectivity(), stop() unregister.
  • AmplitudeReactNativeConnectivityModule.kt: stub → real thin bridge — delegates to ConnectivityChecker, seeds JS via getNetworkConnectivityStatus, emits AmplitudeNetworkConnectivityChanged (guarded on an active catalyst instance), tears down in invalidate()/onCatalystInstanceDestroy().
  • AndroidManifest.xml / AndroidManifestNew.xml: add ACCESS_NETWORK_STATE.
  • build.gradle: junit:junit:4.13.2, org.robolectric:robolectric:4.10.3, io.mockk:mockk:1.13.3 test deps + testOptions { unitTests { includeAndroidResources; returnDefaultValues } }.
  • ConnectivityCheckerTest.kt (new, 20 tests) / AmplitudeReactNativeConnectivityModuleTest.kt (new, 5 tests): Robolectric coverage — per-API currentConnectivity/hasInternetCapability, registration branch, dedupe, captive-portal "wait for validation", multi-network "stay online", best-effort "query throws", and teardown.
  • CI.github/workflows/rn-smoke.yml: new android-unit job runs :amplitude_analytics-react-native:testDebugUnitTest from the bare example app on ubuntu-latest (JDK 17), reusing the existing check-affected gate so it runs only when the RN SDK is affected (+ main pushes). examples/react-native/app/package.json: adds @react-native-community/cli, …/cli-platform-android, @react-native/gradle-plugin devDeps so RN autolinking's gradle scripts resolve under pnpm's layout.

Test plan

  • Robolectric — run locally + now gated in CI: ./gradlew :amplitude_analytics-react-native:testDebugUnitTest from examples/react-native/app/android25 tests pass (20 + 5, 0 failures, 0 skipped). The new rn-smoke android-unit job runs exactly this. (API 21–22 cases were dropped: RN 0.74's react-android forces minSdk 23, so Robolectric can't simulate API 21 there — and RN itself dropped API <23 in 0.74.)
  • JS suite unaffected (Android-only logic change): pnpm --filter @amplitude/analytics-react-native typecheck clean; lint 0 errors; test (web + mobile) 108 tests / 11 suites pass (after building workspace deps).

Local test:
https://amplitude.zoom.us/clips/share/xNgag7YrSiWNxqUUJf0K7w

Steps:

  1. cd examples/react-native/expo-app, run pnpm start to start Metro
  2. In another terminal, pnpm run android to start Android app
  3. In another terminal, adb shell to enter adb
  4. Attach the running app with App Inspection in Android Studio
  5. Click "Online Test" button and observe the event is sent
  6. Go to the adb terminal, svc wifi disable && svc data disable to disconnect network
  7. Click "Offline Test" button and observe no event is sent
  8. Go to the adb terminal, svc wifi enable && svc data enable to reconnect
  9. Observe offline events are sent automatically

🤖 Generated with Claude Code


Note

Medium Risk
Changes when the RN SDK treats the device as offline on Android, which directly affects event upload vs queuing; behavior is heavily tested and biased toward online on uncertainty, but edge cases (e.g. dropped emits when the bridge is inactive) remain documented.

Overview
Replaces the Android no-op connectivity native module with real ConnectivityManager monitoring so offline mode can queue events on Android (API 23+), matching iOS in the offline stack.

Networking logic lives in a new framework-free ConnectivityChecker: validated internet checks (captive portals stay offline), callbacks re-read currentConnectivity() instead of trusting event types, deduped state updates, and best-effort “online” when probes or registration fail so analytics are not wrongly suppressed. AmplitudeReactNativeConnectivityModule is a thin bridge—real seed via getNetworkConnectivityStatus, AmplitudeNetworkConnectivityChanged events when listeners subscribe, teardown on invalidate(). ACCESS_NETWORK_STATE is declared in both library manifests for host merge.

Adds Robolectric + mockk unit tests (25) and Gradle test wiring; rn-smoke gains an android-unit job running :amplitude_analytics-react-native:testDebugUnitTest from the example app, with RN CLI/Gradle devDeps on the example app for pnpm autolinking.

Reviewed by Cursor Bugbot for commit 3b45dda. Configure here.

Mercy811 and others added 19 commits June 9, 2026 13:48
… (SDKRN-5)

PR 1 of a stacked series splitting the SDKRN-5 offline feature for review.

Adds the cross-platform pieces of offline mode:
- `networkConnectivityCheckerPlugin` (BeforePlugin): seeds and flips
  `config.offline` from the native connectivity module (or `navigator.onLine`
  on web), flushing on reconnect; falls back to online when no connectivity
  source is available.
- Client wiring in `_init` (installs before `Destination`, skips on the
  `OfflineDisabled` sentinel).
- Plugin unit tests + end-to-end `offline-integration.test.ts` (native bridge
  mocked in the jest setups).

The native `AmplitudeReactNativeConnectivity` module ships here as a **no-op
placeholder** on both platforms (always reports connected, never emits), so the
SDK is fully runnable after this PR — offline mode is simply a no-op (identical
to pre-feature behavior). Follow-up PRs replace the iOS and Android stubs with
the real `NWPathMonitor` / `ConnectivityManager` implementations.

Verified: analytics-react-native typecheck + lint clean; jest 106/106 pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… type

Stop omitting `offline` from the `ReactNativeConfig` interface now that RN
supports offline mode. The property always existed on the underlying core
`Config` instance at runtime; the type omission only forced casts.

This removes the `OfflineConfig` intersection in the connectivity plugin and
the inline cast in the client, and makes `offline` (including the
`OfflineDisabled` sentinel) a typed, public init option via ReactNativeOptions.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The connectivity handler compares incoming state against the current
`config.offline` and skips no-op updates; it is a state-transition guard,
not a time-windowed debounce.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Guard the web `online` handler so it only flushes on an actual
  offline->online transition (mirrors the native path); add a test.
- Drop now-stale "offline is omitted from the public type" comments and
  the casts they justified, since `offline` is now typed on
  ReactNativeConfig/ReactNativeOptions.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mirror the native path on both directions: the web `offline` handler now
early-returns when already offline, so repeated `offline` events are no-ops
(matching the `online` guard). Add a test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the fixed setTimeout(50) wait (timing-dependent, flaky on slow CI)
with fake timers. Because the react-native jest env deadlocks the async timer
helpers (runAllTimersAsync/advanceTimersByTimeAsync) on its setImmediate
polyfill, drain the Timeline's setTimeout(0) apply chain by alternating
jest.runOnlyPendingTimers() with microtask flushes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…imers

Replace the timer-draining loop with an event-driven wait: spy on
storageProvider.set and resolve once both offline events are persisted. The
Destination persists as it queues each event, so this is fully deterministic
with no fixed delay, no timer polling, and no fake timers (whose async helpers
deadlock under the react-native jest env anyway).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Unify the web online/offline handlers through handleConnectivityChange with a
  deferFlush option (web defers the reconnect flush by flushIntervalMillis;
  native flushes immediately).
- Log the incoming connectivity status at debug at the top of the handler.
- Raise the failed-status-read and no-connectivity-source logs to warn.
- Read navigator off the cached globalScope.
- DRY the tests with beforeEach (client install tests; native-mode plugin tests)
  and rename the "web fallback" suite to "web mode".

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
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>
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>
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>
@linear-code

linear-code Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

SDKRN-5

SDKRN-24

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6f3ccecebf

ℹ️ 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".

@github-actions

github-actions Bot commented Jun 15, 2026

Copy link
Copy Markdown

size-limit report 📦

Path Size
packages/analytics-browser/lib/scripts/amplitude-min.js.gz 60.57 KB (0%)
packages/session-replay-browser/lib/scripts/session-replay-browser-min.js.gz 133.19 KB (0%)
packages/unified/lib/scripts/amplitude-min.umd.js.gz 213.16 KB (0%)
@amplitude/element-selector (gzipped esm) 2.67 KB (0%)

@Mercy811 Mercy811 force-pushed the sdkrn-5-offline-4-android branch 4 times, most recently from 4cb3632 to cbabb77 Compare June 15, 2026 18:40
Mercy811 and others added 2 commits June 15, 2026 14:51
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>
Mercy811 and others added 5 commits June 24, 2026 10:51
… teardown hook

The framework only ever drives invalidate() at module teardown (verified in
ModuleHolder.destroy() across RN 0.70/0.73/0.74), so the deprecated
onCatalystInstanceDestroy() override was redundant: on RN <=0.73 it fired a
second no-op stop() via the base invalidate() delegation, and on RN 0.74+ it
was never invoked. Keep invalidate() as the sole teardown path.

Also swap @RequiresApi for @SuppressLint on the ConnectivityChecker API-gated
helpers so lint stays quiet on minSdk-21 hosts.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…back

- removeListeners: track a listener count and stop monitoring on the last
  unsubscribe (mirrors iOS RCTEventEmitter start/stopObserving) so the
  ConnectivityManager callback isn't left registered after the plugin tears
  down. invalidate() stays as the context-teardown backstop.
- currentConnectivity: catch Exception instead of Throwable so unrecoverable
  Errors (OOM, linkage) propagate instead of being masked as online; the
  real-world ConnectivityManager failures are all Exceptions, so best-effort
  behavior is unchanged.
- Update module tests: last-listener removal unregisters the callback, and
  monitoring stays active while listeners remain.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…handling

Address review feedback on the Android connectivity module:

- start(): catch RuntimeException (not just SecurityException) and best-effort set
  isConnected = true, so registerNetworkCallback's "Too many NetworkRequests filed"
  RuntimeException can't crash addListener and a failed registration never pins offline.
- currentConnectivity(): treat a missing ACCESS_NETWORK_STATE as online (the
  ConnectivityManager queries can return null instead of throwing, which would pin the
  SDK offline), and catch LinkageError (OEM-forked ConnectivityManager/NetworkCapabilities
  can throw NoSuchMethodError etc.) as best-effort online, while letting VirtualMachineError
  (OOM/StackOverflow) propagate.
- build.gradle: drop the misleading mockk version-pin comment.
- Tests: registration RuntimeException, LinkageError, and permission-denied best-effort cases.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…vity event name

addListener now returns early unless eventName == CONNECTIVITY_EVENT_NAME, so an
unexpected subscription can't start monitoring, and the previously-unused eventName
parameter no longer trips the "never used" warning. Addresses review feedback.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Mercy811

Copy link
Copy Markdown
Contributor Author

Design note: opposite initial-seed philosophy on iOS vs Android

Recording a deliberate cross-platform difference in how the two native connectivity modules seed their initial state, since it looks inconsistent at a glance but is intentional.

iOS — seed false (offline). The required network API (NWPathMonitor) is always available on supported iOS versions, so we can trust it. We seed disconnected so startup events buffer until the monitor reports a real path. The philosophy: don't send until we know we're online, to prevent event loss from exhausting max retries while connectivity is still unknown.

Android — seed true (online). The required networking APIs (ConnectivityManager / NetworkCapabilities validation, API 23+) can be unavailable or unreliable on some devices/OEM forks (missing ACCESS_NETWORK_STATE, getSystemService returning null, query throwing, pre-API-23). The philosophy is inverted: stay online unless we get a clear offline signal, so a failed/unavailable probe never pins the SDK offline forever and silently suppresses all sends. Every best-effort fallback in currentConnectivity() returns true for the same reason.

Net: iOS optimizes against premature sends on a trustworthy API; Android optimizes against a permanent false-offline on an untrustworthy one.

…ments

Drop comments that restated the code (NativeEventEmitter requirement,
getNetworkConnectivityStatus doc, start() seed) and correct the isConnected
note — it's only used for dedupe; JS is seeded via currentConnectivity().

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Mercy811 Mercy811 force-pushed the sdkrn-5-offline-4-android branch from ac58f6d to db724bf Compare June 24, 2026 18:28
@Mercy811 Mercy811 changed the base branch from sdkrn-5-offline-2-ios to sdkrn-5-offline-3-ios-tests June 24, 2026 18:28
…or start

JS reads its seed via getNetworkConnectivityStatus and then subscribes, which
runs ConnectivityChecker.start(). A connectivity flip in that gap left JS
stranded on the stale seed: Android delivers no immediate callback when offline
and dedupes its own immediate onAvailable against the freshly seeded state, so
no reconciling event was sent — the SDK could send while offline or keep
queuing while online.

start() now emits the authoritative post-start state once (best-effort online
if registration fails). Because JS awaits its seed before subscribing, this
emit is unambiguously the last write; JS dedupes a same-state value, so it is a
no-op when nothing changed. Mirrors how NWPathMonitor's first update reconciles
JS on iOS.

Update the affected unit tests for the leading seed emit and add coverage for
the seed-vs-start race and the registration-failure path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Mercy811

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 3b45dda. Configure here.

@Mercy811 Mercy811 requested a review from polbins June 24, 2026 20:24
@polbins

polbins commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

@codex review this PR from an Amplitude-Kotlin Android SDK maintainer perspective. Focus on: (1) parity with AndroidNetworkConnectivityChecker and AndroidNetworkListener, (2) ConnectivityChecker stop()/teardown error handling, (3) permission API (checkSelfPermission vs checkCallingOrSelfPermission), (4) cross-SDK behavioral differences (VALIDATED vs transport, captive portals, Ethernet/VPN), (5) seed-vs-start race and post-start emit, (6) test gaps.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@polbins polbins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amplitude-Kotlin maintainer review (local Codex pass — GitHub Codex connector requires account setup)

Verdict: Request changes for one P1; otherwise solid.

P1 — ConnectivityChecker.stop() teardown: Only catches IllegalArgumentException. Kotlin AndroidNetworkListener.stopListening() also handles IllegalStateException and broader failures (#220, #197). invalidate() calls stop() during bridge teardown — broaden catch and clear networkCallback in finally.

P2 (non-blocking): INTERNET+VALIDATED semantics align with Kotlin listener, not transport-only checker — document if intentional. Missing onBlockedStatusChanged vs Kotlin listener.

P3: checkSelfPermission is fine for RN context.

Recommend adding Robolectric tests for stop() swallowing IllegalStateException / generic teardown failures.

@Mercy811

Copy link
Copy Markdown
Contributor Author

Thanks @polbins, P1 looks valid, I will fix it shortly and add tests.

… registration

stop() only caught IllegalArgumentException and cleared networkCallback outside
the try, so any other device ConnectivityManager failure on unregister
(IllegalStateException, generic RuntimeException, OEM-fork LinkageError) escaped
and left the callback set — wedging start()/stop() and surfacing through
invalidate() during bridge teardown. Broaden the catch and clear networkCallback
in a finally so teardown is always best-effort.

Add a matching LinkageError catch to start()'s registration (RuntimeException
already covered Amplitude-Kotlin #220/#197) and move the post-start seed emit
into a finally, aligning start() and stop() on the same try/catch/finally shape
as currentConnectivity()'s defensive posture.

Add Robolectric coverage: stop() swallowing IllegalStateException / generic
RuntimeException and clearing the callback, start() re-registering after a failed
unregister (no wedge), and start() swallowing a registration LinkageError.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Mercy811

Copy link
Copy Markdown
Contributor Author

@codex re-review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8f93fb2e35

ℹ️ 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".

Comment thread .github/workflows/rn-smoke.yml
A validated network can still have this app's traffic blocked by the OS (Data
Saver, background data restriction). Reporting online there means sends fail,
retry, and eventually drop at max-retry -- exactly the loss offline mode exists
to prevent. Track a blocked flag from onBlockedStatusChanged (API 29+) and emit
currentConnectivity() && !blocked from the live callbacks, so a block counts as
offline and we queue instead. Mirrors Amplitude-Kotlin's available && !blocked.

Below API 29 the OS never reports block status, so behavior is unchanged there.
stop() resets the flag so a stale block status can't leak into the next
registration. Add Robolectric coverage for block -> offline, unblock -> online,
and no flip-flop on a capabilities change while blocked.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Mercy811

Copy link
Copy Markdown
Contributor Author

@codex

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 775368c5a7

ℹ️ 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".

Comment thread .github/workflows/rn-smoke.yml
@Mercy811 Mercy811 requested a review from polbins June 25, 2026 15:52

@polbins polbins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

Base automatically changed from sdkrn-5-offline-3-ios-tests to main June 25, 2026 19:28
Mercy811 and others added 2 commits June 25, 2026 12:28
…tivity module

The "Merge branch 'main'" commit left unresolved conflict markers in
AmplitudeReactNativeConnectivityModule.kt -- main still carries the old no-op
placeholder, which conflicted with this PR's real module. Kotlin couldn't parse
the markers, failing compileDebugKotlin in the Robolectric CI job. Keep the real
module (this PR replaces the placeholder) and drop main's version.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Mercy811 Mercy811 merged commit 93df7dc into main Jun 25, 2026
13 checks passed
@Mercy811 Mercy811 deleted the sdkrn-5-offline-4-android branch June 25, 2026 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants