fix(player): hide diagnostic breakdown UI and skip probes when offline

[syed-tp]

Why:

• When a device has no internet connection, displaying the full network diagnostic checklist (Internet, DNS, Server, CDN) on the error overlay is redundant and cluttered. Additionally, executing multi-probe network requests when offline wastes battery and causes unnecessary socket timeout delays.

What Changed:

• PlayerErrorViewController: Hide the diagnostic breakdown list and line divider when diagnostics.internetReachable is false.
• NetworkDiagnosticsManager: Inspect system network status via ConnectivityManager before starting probes; skip probe execution completely if offline.
• NetworkProbeRunner: Execute probeInternet first and bypass downstream DNS, Server, and CDN probes when internet reachability fails.
• NetworkInfoProvider: Add isSystemNetworkConnected helper method using NetworkCapabilities.

Impact:

• Delivers a cleaner error screen to offline users while eliminating unnecessary background network probe overhead.

[claude]

Claude finished @syed-tp's task in 4m 1s —— View job

---

Claude finished reviewing this PR

Todo list:

• Gather context (diff, related files, CLAUDE.md)
• Understand the request (code review)
• Review PlayerErrorViewController.kt changes
• Review NetworkDiagnosticsManager.kt changes
• Review NetworkInfoProvider.kt changes
• Review NetworkProbeRunner.kt changes
• Post final review summary with verdict

Summary

Reviewed all 4 changed files: PlayerErrorViewController.kt, NetworkDiagnosticsManager.kt, NetworkInfoProvider.kt, NetworkProbeRunner.kt.

• NetworkInfoProvider.isSystemNetworkConnected: Correctly fails open (returns true) on null context, missing ConnectivityManager, or any exception — so probes still run if the check itself is inconclusive. It only checks NET_CAPABILITY_INTERNET (not NET_CAPABILITY_VALIDATED), which is the right choice here: it can under-detect "connected but no real internet" (falls through to real probes, no regression), and could rarely over-detect "offline" during a brief network handover when activeNetwork is transiently null — but that's self-healing via NetworkRecoveryHandler's immediate onAvailable callback and 30s timeout fallback, so low risk.
• NetworkDiagnosticsManager.handleError: The new offline fast-path is reasonable and threading is consistent (playerScope is Dispatchers.Main, so the synchronous listener(...) call here matches the dispatch context of the existing async path). Left a non-blocking observability note about the Sentry breadcrumb no longer being recorded for offline errors (previously it was, even though the event itself was already suppressed for !internetReachable).
• NetworkProbeRunner.runProbes: Left an inline comment on a genuine latency regression — probeInternet is now awaited before starting the DNS/server/CDN probes (previously all 4 ran concurrently via async). For the common "online but degraded" diagnostic case (the scenario most in need of an accurate breakdown), this roughly doubles worst-case latency and makes it more likely to hit the outer 8s timeout and fall back to a generic "timeout" verdict instead of real probe results. Suggested keeping all four probes concurrent and cancelling the downstream ones once internet == false is known, which preserves both the "don't waste time probing DNS/server/CDN when offline" goal and the original concurrency for the online case.
• PlayerErrorViewController.showNetworkDiagnostics: Hiding diagnosticsContainer/errorDivider when offline is straightforward and correct; skipping buildDiagnosticsList leaves stale child views in the (now GONE) container, but since it's rebuilt (removeAllViews() + re-added) the next time diagnostics are shown, this has no visible effect.

Regression assessment: No regressions found in playback, DRM, offline downloads, seeking, track selection, or lifecycle handling — the changes are scoped to the error-diagnostics path only. The one real concern is the latency regression in NetworkProbeRunner for the online-but-degraded case, which is worth fixing but isn't a correctness-breaking bug (bounded by the existing 8s timeout, degrades gracefully to a fallback diagnostic).

Code quality: Clean, well-scoped diff with clear intent. NetworkInfoProvider.isSystemNetworkConnected mirrors the existing getNetworkInfo API-level branching style, keeping the codebase consistent.

Final Verdict

⚠️ APPROVED WITH SUGGESTIONS

No blocking correctness, regression, lifecycle, or security issues. The NetworkProbeRunner sequencing change is worth addressing before merge (or as a fast follow-up) since it works against the PR's own stated goal of reducing diagnostic latency, but it's not a functional break — it's bounded by existing timeout handling and degrades to an existing fallback path.

[syed-tp]

@claude review

Check the comments, review and post the verdict

[claude]

Claude finished @syed-tp's task in 3m 11s —— View job

---

Reviewing PR #140

• Gather context (PR diff, review comments)
• Review PlayerErrorViewController.kt changes
• Review NetworkDiagnosticsManager.kt changes (incl. author's telemetry-omission comment)
• Review NetworkInfoProvider.kt changes
• Review NetworkProbeRunner.kt changes (incl. author's sequential-probe comment)
• Post final verdict

Verdict: ✅ Approve, with two minor follow-ups worth addressing

The core change is sound: gating on ConnectivityManager before running probes, and hiding the diagnostic breakdown when internetReachable == false, is a reasonable way to cut socket-timeout delays and UI clutter for genuinely offline devices. Both of the author's own review comments (telemetry omission on the fast path, and sequential probeInternet to short-circuit downstream probes) are legitimate, intentional trade-offs and not concerns.

Two things I'd fix before/after merge:

1. Fast path overrides finalError inconsistently with the existing offline path — NetworkDiagnosticsManager.kt:113-115
The new system-offline fast path always calls listener(PlaybackError.NETWORK_CONNECTION_FAILED, "No internet connection", ...). But the existing full-probe offline path (classifyError, line ~195) preserves the original errorType passed into handleError (e.g. NETWORK_CONNECTION_TIMEOUT) when !internetReachable. Since handleError can be invoked with NETWORK_CONNECTION_TIMEOUT (or an UNSPECIFIED-mapped IOException — see PlaybackError.toError()), a caller whose device is offline will now get a different PlaybackError value on the public onNetworkError() callback depending on whether the fast path or the full-probe path caught it. Consumers who branch on the specific error enum could see this as a regression. Consider reusing classifyError(errorType, diagnostics) (or at least passing through errorType) in the fast path instead of hardcoding NETWORK_CONNECTION_FAILED.

2. Fast-path NetworkDiagnostics drops cdnHostname — NetworkDiagnosticsManager.kt:116-125
cdnHostname is a parameter of handleError(...), and every other NetworkDiagnostics construction (including NetworkProbeRunner's own internal offline early-return at NetworkProbeRunner.kt:36-49) sets cdnHostname = cdnHostname. The new manager-level fast path omits it, so it silently falls back to the default null. Since this NetworkDiagnostics object is forwarded straight to the public onNetworkError() listener, host apps reading diagnostics.cdnHostname would see null here even though the value was available. Minor, but easy to fix by adding cdnHostname = cdnHostname to that constructor call — and it's a good sign the two "offline diagnostics" object literals (in NetworkDiagnosticsManager and NetworkProbeRunner) have already started drifting; might be worth factoring into one shared helper.

Neither issue affects the offline-UI behavior described in the PR (diagnostics list is correctly hidden either way), so I wouldn't block on these, but they're worth a quick follow-up since they affect the public error-reporting contract.

Fix both →