refactor: deduplicate network connectivity checks#648
Open
jeremybernstein wants to merge 5 commits intoutkarshdalal:masterfrom
Open
refactor: deduplicate network connectivity checks#648jeremybernstein wants to merge 5 commits intoutkarshdalal:masterfrom
jeremybernstein wants to merge 5 commits intoutkarshdalal:masterfrom
Conversation
move WiFi/Ethernet tracking from SteamService into MainActivity's existing NetworkCallback. SteamService keeps its callback only for download-pausing side effect. LibraryAppScreen reads both states from MainActivity instead of doing a one-shot ConnectivityManager query.
switch connectivity state from AtomicBoolean to StateFlow so LibraryAppScreen recomposes when network changes — install button updates in real-time without navigating away and back
Contributor
📝 WalkthroughWalkthroughIntroduce a process-wide NetworkMonitor singleton exposing StateFlows for internet and Wi‑Fi connectivity; remove MainActivity-local network callback/state and update consumers (SteamService, LibraryAppScreen, Coil interceptor) to read NetworkMonitor; initialize NetworkMonitor in PluviaApp and add two download-related string resources. Changes
Sequence Diagram(s)sequenceDiagram
participant App as PluviaApp
participant NM as NetworkMonitor
participant Steam as SteamService
participant UI as LibraryAppScreen
participant Coil as CoilInterceptor
App->>NM: init(context)
NM->>NM: register ConnectivityManager callback\ntrack per-network caps, compute StateFlows
NM--)Steam: expose isWifiConnected StateFlow / .value
NM--)UI: expose hasInternet & isWifiConnected StateFlows
NM--)Coil: expose hasInternet (.value) for interceptor checks
Steam->>Steam: checkWifiOrNotify() reads NM state -> allow or pause downloads
UI->>UI: collect StateFlows -> recompute UI gating
Coil->>Coil: read hasInternet -> alter image loading/offline behavior
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
2615-2623:⚠️ Potential issue | 🟡 MinorAvoid pausing on every
onLostevent without current-state guard.Line 2615 pauses all downloads whenever any tracked network is lost. If another Wi‑Fi/LAN network is still active, this causes unnecessary pauses.
Suggested fix
- if (PrefManager.downloadOnWifiOnly) { + if (PrefManager.downloadOnWifiOnly && !isWifiConnected) { for ((appId, info) in downloadJobs.entries.toList()) { Timber.d("Cancelling job") info.cancel() PluviaApp.events.emit(AndroidEvent.DownloadPausedDueToConnectivity(appId)) removeDownloadJob(appId) } notificationHelper.notify("Download paused – waiting for Wi-Fi/LAN") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/SteamService.kt` around lines 2615 - 2623, The current onLost handler unconditionally pauses all downloads when any tracked network is lost; change it to first check the actual current connectivity state and only pause/remove jobs if there is no longer any active Wi‑Fi/LAN connection. In practice, add a guard before iterating downloadJobs (e.g., call your ConnectivityManager helper or implement an isOnWifiOrLan() check) and only run the loop that calls info.cancel(), emits AndroidEvent.DownloadPausedDueToConnectivity(appId) and removeDownloadJob(appId) when that check returns false; keep the PrefManager.downloadOnWifiOnly branch but make it conditional on the updated connectivity check so transient losses of one network won’t pause downloads if another Wi‑Fi/LAN is still available.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
1012-1015: Deduplicate the Wi‑Fi-only gate logic into one helper.The same guard+notification block appears twice. Extracting it reduces drift risk and keeps behavior consistent.
Refactor sketch
+ private fun enforceWifiOnlyGate(): Boolean { + if (PrefManager.downloadOnWifiOnly && !isWifiConnected) { + instance?.notificationHelper?.notify("Not connected to Wi‑Fi/LAN") + return false + } + return true + } - if (PrefManager.downloadOnWifiOnly && !isWifiConnected) { - instance?.notificationHelper?.notify("Not connected to Wi‑Fi/LAN") - return null - } + if (!enforceWifiOnlyGate()) return null ... - if (PrefManager.downloadOnWifiOnly && !isWifiConnected) { - instance?.notificationHelper?.notify("Not connected to Wi‑Fi/LAN") - return null - } + if (!enforceWifiOnlyGate()) return nullAlso applies to: 1364-1367
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/SteamService.kt` around lines 1012 - 1015, Create a single helper in SteamService (e.g., checkWifiOrNotify or enforceWifiOnly) that encapsulates the PrefManager.downloadOnWifiOnly && !isWifiConnected check and calls instance?.notificationHelper?.notify("Not connected to Wi‑Fi/LAN") when blocked; have it return a Boolean (true = ok to proceed, false = blocked). Replace both duplicated guard blocks that currently use PrefManager.downloadOnWifiOnly, isWifiConnected, instance and notificationHelper.notify with a single call like if (!checkWifiOrNotify()) return null so behavior and return semantics remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/MainActivity.kt`:
- Around line 91-93: The code currently sets _isWifiConnected from the raw
wifiNetworks set, which can include unvalidated/captive Wi‑Fi; change the logic
to derive Wi‑/LAN connectivity from the validated networks instead. After
computing validated (the list/collection of validated networks), compute whether
any of those validated entries satisfy isWifiOrEthernet(caps) and set
_isWifiConnected based on that (e.g., replace wifiNetworks.isNotEmpty() with a
check over validated using isWifiOrEthernet), while keeping the existing
wifiNetworks updates if you still need the full transport set.
---
Outside diff comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 2615-2623: The current onLost handler unconditionally pauses all
downloads when any tracked network is lost; change it to first check the actual
current connectivity state and only pause/remove jobs if there is no longer any
active Wi‑Fi/LAN connection. In practice, add a guard before iterating
downloadJobs (e.g., call your ConnectivityManager helper or implement an
isOnWifiOrLan() check) and only run the loop that calls info.cancel(), emits
AndroidEvent.DownloadPausedDueToConnectivity(appId) and removeDownloadJob(appId)
when that check returns false; keep the PrefManager.downloadOnWifiOnly branch
but make it conditional on the updated connectivity check so transient losses of
one network won’t pause downloads if another Wi‑Fi/LAN is still available.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 1012-1015: Create a single helper in SteamService (e.g.,
checkWifiOrNotify or enforceWifiOnly) that encapsulates the
PrefManager.downloadOnWifiOnly && !isWifiConnected check and calls
instance?.notificationHelper?.notify("Not connected to Wi‑Fi/LAN") when blocked;
have it return a Boolean (true = ok to proceed, false = blocked). Replace both
duplicated guard blocks that currently use PrefManager.downloadOnWifiOnly,
isWifiConnected, instance and notificationHelper.notify with a single call like
if (!checkWifiOrNotify()) return null so behavior and return semantics remain
identical.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/MainActivity.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt
- derive isWifiConnected from validated networks only (excludes captive portals) - SteamService onLost checks actual state before pausing (multi-network safe) - extract checkWifiOrNotify() helper for duplicated WiFi guard - move notification strings to strings.xml for localization
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/service/SteamService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamService.kt:294">
P2: SteamService now relies on MainActivity.isWifiConnected for Wi‑Fi gating, but that StateFlow only updates while MainActivity is alive. If the activity is not running, the flow stays at its default/stale value and downloads can be incorrectly blocked or allowed in the background.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
app/src/main/res/values/strings.xml (1)
753-754: Move new strings to their own section and align the prefix.Both strings are SteamService notification messages (per the PR description: "Move notification strings to strings.xml for localization"), yet they are placed inside
<!-- LibraryAppScreen: UI Labels -->whose every other entry carries thelibrary_prefix. Keeping notification strings in the wrong section makes the file harder to navigate and may mislead translators.♻️ Suggested reorganisation
<string name="library_file_count">File %1$d of %2$d</string> - <string name="download_no_wifi">Not connected to Wi\u2011Fi/LAN</string> - <string name="download_paused_wifi">Download paused – waiting for Wi-Fi/LAN</string> <!-- PluviaMain: Update & App Info -->Add a dedicated section further down (or near the
<!-- Login & Connection -->block):+ <!-- SteamService: Notifications --> + <string name="download_no_wifi">Not connected to Wi-Fi/LAN</string> + <string name="download_paused_wifi">Download paused – waiting for Wi-Fi/LAN</string> + <!-- PluviaMain: Update & App Info -->🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/res/values/strings.xml` around lines 753 - 754, Move the two SteamService notification strings out of the "LibraryAppScreen: UI Labels" block and into a new dedicated notification section (e.g., near the existing "Login & Connection" block); specifically relocate the <string name="download_no_wifi"> and <string name="download_paused_wifi"> entries into that new section and align their naming/prefix with other notification keys if your convention requires it (ensure the new section comment clearly indicates SteamService/notification strings so translators can find them).app/src/main/java/app/gamenative/service/SteamService.kt (1)
1019-1021:checkWifiOrNotify()called twice in the same download dispatch path
downloadApp(appId, dlcAppIds, isUpdateOrVerify)(line 1019) callscheckWifiOrNotify()at line 1020, and if it passes, delegates to the full-parameter overloaddownloadApp(appId, downloadableDepots, …)which callscheckWifiOrNotify()again at line 1368. In a race where connectivity drops between the two calls, the user receives a duplicate notification. Removing the redundant guard at line 1020 (or line 1368 if this call site is the only entry point) removes the ambiguity.♻️ Proposed fix — remove the redundant guard at the shallower overload
fun downloadApp(appId: Int, dlcAppIds: List<Int>, isUpdateOrVerify: Boolean): DownloadInfo? { - if (!checkWifiOrNotify()) return null return getAppInfoOf(appId)?.let { appInfo ->🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/SteamService.kt` around lines 1019 - 1021, The shallow overload downloadApp(appId: Int, dlcAppIds: List<Int>, isUpdateOrVerify: Boolean) currently calls checkWifiOrNotify() and then delegates to the deeper overload which also calls checkWifiOrNotify(), causing duplicate notifications; remove the redundant guard and early return from the shallow overload (keep the call to getAppInfoOf and the delegation to the full-parameter downloadApp), so that the deeper downloadApp(appId, downloadableDepots, ...) remains the single place that performs the connectivity check via checkWifiOrNotify().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 2614-2626: SteamService.onLost currently reads the shared
StateFlow isWifiConnected which can be stale due to unordered NetworkCallback
delivery; instead, inside the onLost override query ConnectivityManager directly
to determine whether any Wi‑Fi/LAN transports remain before pausing downloads:
obtain the ConnectivityManager, iterate its current networks
(connectivityManager.allNetworks or getAllNetworks()) and for each check
connectivityManager.getNetworkCapabilities(network) for TRANSPORT_WIFI or
TRANSPORT_ETHERNET; compute a live wifiConnected boolean and then, if
PrefManager.downloadOnWifiOnly && !wifiConnected, cancel downloadJobs (call
info.cancel,
PluviaApp.events.emit(AndroidEvent.DownloadPausedDueToConnectivity(appId)),
removeDownloadJob(appId)) and call
notificationHelper.notify(getString(R.string.download_paused_wifi)) as currently
done.
- Around line 296-303: The checkWifiOrNotify() method currently suppresses the
user notification when instance is null; update this method to explicitly handle
the null branch by logging a warning (e.g., via Log.w or your app logger) before
returning false so the failure is observable; keep the existing behavior of
calling instance.notificationHelper.notify(...) when instance is non-null, and
reference checkWifiOrNotify(), instance, notificationHelper.notify,
PrefManager.downloadOnWifiOnly and isWifiConnected when making the change.
- Line 294: MainActivity.isWifiConnected is stale when MainActivity unregisters
its NetworkCallback, so update SteamService to own/observe network state instead
of relying on MainActivity: add onAvailable and/or onCapabilitiesChanged
handlers to SteamService's NetworkCallback (in addition to the existing onLost)
to maintain an internal wifiConnected flag and change the isWifiConnected getter
to read that flag (or use the shared NetworkMonitor singleton referenced in
comments) so downloads respect current connectivity even when MainActivity is
destroyed; also remove the redundant call to checkWifiOrNotify in the nested
downloadApp overload (leave a single check prior to starting downloads) to avoid
duplicate notifications.
In `@app/src/main/res/values/strings.xml`:
- Line 753: The string resource download_no_wifi contains a non‑breaking hyphen
(U+2011) in "Wi\u2011Fi/LAN"; replace that character with a standard
hyphen-minus '-' so the value becomes "Not connected to Wi-Fi/LAN" to match
other Wi‑Fi strings (e.g., those used elsewhere) and avoid invisible character
issues.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 1019-1021: The shallow overload downloadApp(appId: Int, dlcAppIds:
List<Int>, isUpdateOrVerify: Boolean) currently calls checkWifiOrNotify() and
then delegates to the deeper overload which also calls checkWifiOrNotify(),
causing duplicate notifications; remove the redundant guard and early return
from the shallow overload (keep the call to getAppInfoOf and the delegation to
the full-parameter downloadApp), so that the deeper downloadApp(appId,
downloadableDepots, ...) remains the single place that performs the connectivity
check via checkWifiOrNotify().
In `@app/src/main/res/values/strings.xml`:
- Around line 753-754: Move the two SteamService notification strings out of the
"LibraryAppScreen: UI Labels" block and into a new dedicated notification
section (e.g., near the existing "Login & Connection" block); specifically
relocate the <string name="download_no_wifi"> and <string
name="download_paused_wifi"> entries into that new section and align their
naming/prefix with other notification keys if your convention requires it
(ensure the new section comment clearly indicates SteamService/notification
strings so translators can find them).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/MainActivity.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/MainActivity.kt
|
|
||
| internal var instance: SteamService? = null | ||
|
|
||
| val isWifiConnected: Boolean get() = MainActivity.isWifiConnected.value |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for MainActivity.isWifiConnected declaration and initialization
rg -n "isWifiConnected" app/src/main/java/app/gamenative/MainActivity.kt -B5 -A5Repository: utkarshdalal/GameNative
Length of output: 1224
🏁 Script executed:
# Search for SteamService START_STICKY declaration
rg -n "START_STICKY" --type=ktRepository: utkarshdalal/GameNative
Length of output: 93
🏁 Script executed:
# Search for StateFlow declarations in MainActivity
rg -n "MutableStateFlow|StateFlow" app/src/main/java/app/gamenative/MainActivity.kt -B2 -A2Repository: utkarshdalal/GameNative
Length of output: 757
🏁 Script executed:
# Check the callback registration in SteamService
rg -n "registerNetworkCallback\|onNetworkAvailable\|onLost" app/src/main/java/app/gamenative/service/SteamService.kt -B3 -A3Repository: utkarshdalal/GameNative
Length of output: 49
🏁 Script executed:
# Search for registerNetworkCallback and callback registration
rg "registerNetworkCallback" --type=java --type=ktRepository: utkarshdalal/GameNative
Length of output: 93
🏁 Script executed:
# Search for START_STICKY in service declarations
rg "START_STICKY" --type=java --type=ktRepository: utkarshdalal/GameNative
Length of output: 93
🏁 Script executed:
# Check MainActivity onCreate and callback registration
cat -n app/src/main/java/app/gamenative/MainActivity.java | head -150Repository: utkarshdalal/GameNative
Length of output: 149
🏁 Script executed:
# Check if MainActivity has a companion object and how the callback is registered
rg "NetworkCallback\|registerNetworkCallback" app/src/main/java/app/gamenative/MainActivity.java -B5 -A10Repository: utkarshdalal/GameNative
Length of output: 157
🏁 Script executed:
# Search for registerNetworkCallback and callback setup
rg "registerNetworkCallback|NetworkCallback" -B3 -A5Repository: utkarshdalal/GameNative
Length of output: 7252
🏁 Script executed:
# Search for START_STICKY
rg "START_STICKY"Repository: utkarshdalal/GameNative
Length of output: 846
🏁 Script executed:
# Check MainActivity.kt for callback registration details
head -200 app/src/main/java/app/gamenative/MainActivity.ktRepository: utkarshdalal/GameNative
Length of output: 8257
🏁 Script executed:
# Check SteamService companion properties around line 294
sed -n '290,310p' app/src/main/java/app/gamenative/service/SteamService.ktRepository: utkarshdalal/GameNative
Length of output: 889
🏁 Script executed:
# Check the full context of isWifiConnected in SteamService
rg -n "val isWifiConnected" app/src/main/java/app/gamenative/service/SteamService.kt -B2 -A2Repository: utkarshdalal/GameNative
Length of output: 306
🏁 Script executed:
# Check where isWifiConnected is used in SteamService to understand the flow
rg -n "isWifiConnected" app/src/main/java/app/gamenative/service/SteamService.ktRepository: utkarshdalal/GameNative
Length of output: 303
🏁 Script executed:
# Check if SteamService's networkCallback has onAvailable or onCapabilitiesChanged
sed -n '2610,2650p' app/src/main/java/app/gamenative/service/SteamService.ktRepository: utkarshdalal/GameNative
Length of output: 1901
🏁 Script executed:
# Also check around line 2617 where the callback is used
rg -n "onAvailable\|onCapabilitiesChanged" app/src/main/java/app/gamenative/service/SteamService.ktRepository: utkarshdalal/GameNative
Length of output: 49
MainActivity's callback unregistration leaves SteamService with stale Wi-Fi state
When MainActivity is destroyed, its onDestroy() unregisters the NetworkCallback, but MainActivity.isWifiConnected (a static MutableStateFlow) retains its last value. If the flow becomes false while MainActivity is still alive, then MainActivity is destroyed, and Wi-Fi becomes available while MainActivity remains dead, SteamService will permanently read stale false from MainActivity.isWifiConnected. Since SteamService's own callback only implements onLost() (no recovery handler for network availability), downloads with downloadOnWifiOnly enabled will be permanently blocked until MainActivity returns and its callback resumes monitoring.
The TODO comment in MainActivity acknowledges the underlying design issue. Consider having SteamService maintain its own Wi-Fi state via onAvailable() and onCapabilitiesChanged() handlers in its callback, or use a shared NetworkMonitor singleton as suggested in the codebase.
Additionally, checkWifiOrNotify() is called redundantly at both lines 1020 and 1368 in the nested downloadApp overloads—consider removing the second call or restructuring to avoid duplicate checks and notification risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/service/SteamService.kt` at line 294,
MainActivity.isWifiConnected is stale when MainActivity unregisters its
NetworkCallback, so update SteamService to own/observe network state instead of
relying on MainActivity: add onAvailable and/or onCapabilitiesChanged handlers
to SteamService's NetworkCallback (in addition to the existing onLost) to
maintain an internal wifiConnected flag and change the isWifiConnected getter to
read that flag (or use the shared NetworkMonitor singleton referenced in
comments) so downloads respect current connectivity even when MainActivity is
destroyed; also remove the redundant call to checkWifiOrNotify in the nested
downloadApp overload (leave a single check prior to starting downloads) to avoid
duplicate notifications.
Contributor
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/service/SteamService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamService.kt:298">
P2: Wi‑Fi loss handling in SteamService now depends on MainActivity.isWifiConnected, which stops updating when MainActivity is destroyed. This can leave the Wi‑Fi state stale while the foreground service keeps running, causing Wi‑Fi‑only downloads to continue or stay blocked incorrectly. Consider deriving connectivity state inside the service (or a process‑wide NetworkMonitor) instead of Activity‑scoped state.</violation>
</file>
<file name="app/src/main/res/values/strings.xml">
<violation number="1" location="app/src/main/res/values/strings.xml:753">
P3: Inconsistent hyphen character: `download_no_wifi` uses `\u2011` (non-breaking hyphen) for "Wi‑Fi" while `download_paused_wifi` and all other Wi-Fi strings in the file use a plain hyphen-minus (`-`). This invisible difference can confuse translation tools and cause visual inconsistency. Use a plain hyphen for consistency.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
2619-2630:⚠️ Potential issue | 🟠 MajorTOCTOU gap in
onLost— reading staleNetworkMonitorstate.
SteamServiceregisters its ownNetworkCallback(filtered toTRANSPORT_WIFI | TRANSPORT_ETHERNET) whileNetworkMonitorindependently registers a separate callback (filtered toNET_CAPABILITY_INTERNET). Android does not guarantee delivery order between two independently registered callbacks for the same underlying network loss event. IfSteamService.onLostfires beforeNetworkMonitorprocesses the same event and clears_isWifiConnected, thenisWifiConnected(line 2621) still returnstrue, the guard!isWifiConnectedisfalse, and active downloads are not paused even though Wi-Fi is gone.The previously suggested fix — querying
ConnectivityManagerdirectly insideonLostfor live state instead of reading the StateFlow — still applies:🛡️ Proposed fix — live ConnectivityManager query in onLost
override fun onLost(network: Network) { - // only pause if no WiFi/LAN remains (avoids false pause on multi-network) - if (PrefManager.downloadOnWifiOnly && !isWifiConnected) { + val hasRemainingWifi = connectivityManager.allNetworks.any { n -> + val caps = connectivityManager.getNetworkCapabilities(n) ?: return@any false + (caps.hasTransport(NetworkCapabilities.TRANSPORT_WIFI) || + caps.hasTransport(NetworkCapabilities.TRANSPORT_ETHERNET)) && + caps.hasCapability(NetworkCapabilities.NET_CAPABILITY_VALIDATED) + } + if (PrefManager.downloadOnWifiOnly && !hasRemainingWifi) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/SteamService.kt` around lines 2619 - 2630, SteamService.onLost currently reads the possibly-stale StateFlow isWifiConnected (used with PrefManager.downloadOnWifiOnly) which can miss a network-loss if NetworkMonitor hasn't updated yet; instead, inside onLost query ConnectivityManager.getNetworkCapabilities/getActiveNetwork (or use ConnectivityManager#getNetworkInfo fallback) to determine live Wi‑Fi/LAN connectivity and only cancel downloadJobs (iterate downloadJobs, call info.cancel(), PluviaApp.events.emit(AndroidEvent.DownloadPausedDueToConnectivity(appId)), removeDownloadJob(appId)) and call notificationHelper.notify(...) when that live check shows no Wi‑Fi/LAN remains; keep the same behavior but replace the isWifiConnected check with this immediate ConnectivityManager query to avoid the TOCTOU race.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/NetworkMonitor.kt (1)
25-63: Add an idempotency guard toinit().
networkCapsis a local variable captured by the registered callback. Ifinit()is ever called a second time (accidental double-call, test setup, etc.) a second callback is registered against a brand-new, empty map while the first callback keeps its own map. Both callbacks write to the same_hasInternet/_isWifiConnectedflows, but with diverging views of the network inventory — they will race and produce inconsistent state. The KDoc says "call once", but there is no enforcement.🛡️ Proposed fix — atomic init guard
object NetworkMonitor { + `@Volatile` private var initialized = false + private val _hasInternet = MutableStateFlow(false) val hasInternet: StateFlow<Boolean> = _hasInternet.asStateFlow() private val _isWifiConnected = MutableStateFlow(false) val isWifiConnected: StateFlow<Boolean> = _isWifiConnected.asStateFlow() fun init(context: Context) { + if (initialized) return + initialized = true val cm = context.getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/NetworkMonitor.kt` around lines 25 - 63, Add an idempotency guard to prevent registering multiple callbacks: introduce a private AtomicBoolean (e.g., initialized) at the class level and in init() do a compareAndSet(false, true) and return immediately if it fails; keep networkCaps as-is (local to the single callback) and proceed to call cm.registerNetworkCallback only when initialization succeeds so you never register a second callback that would capture a separate networkCaps map and race on _hasInternet/_isWifiConnected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/NetworkMonitor.kt`:
- Around line 19-23: The public StateFlows _hasInternet/hasInternet and
_isWifiConnected/isWifiConnected start false and can cause a false-negative on
cold start; in the NetworkMonitor initialization (where you
registerNetworkCallback) perform an immediate synchronous seed read using
ConnectivityManager.activeNetwork and NetworkCapabilities to set _hasInternet
and _isWifiConnected before registering the callback (or document the
snapshot-read caveat on the public properties). Update the code that sets
initial state (e.g., in the NetworkMonitor init/constructor or a
seedInitialNetworkState method) to compute
networkCapabilities.hasCapability(INTERNET) and whether TRANSPORT_WIFI is
present and assign those values to _hasInternet and _isWifiConnected so early
synchronous reads reflect the current connectivity.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 2619-2630: SteamService.onLost currently reads the possibly-stale
StateFlow isWifiConnected (used with PrefManager.downloadOnWifiOnly) which can
miss a network-loss if NetworkMonitor hasn't updated yet; instead, inside onLost
query ConnectivityManager.getNetworkCapabilities/getActiveNetwork (or use
ConnectivityManager#getNetworkInfo fallback) to determine live Wi‑Fi/LAN
connectivity and only cancel downloadJobs (iterate downloadJobs, call
info.cancel(),
PluviaApp.events.emit(AndroidEvent.DownloadPausedDueToConnectivity(appId)),
removeDownloadJob(appId)) and call notificationHelper.notify(...) when that live
check shows no Wi‑Fi/LAN remains; keep the same behavior but replace the
isWifiConnected check with this immediate ConnectivityManager query to avoid the
TOCTOU race.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/NetworkMonitor.kt`:
- Around line 25-63: Add an idempotency guard to prevent registering multiple
callbacks: introduce a private AtomicBoolean (e.g., initialized) at the class
level and in init() do a compareAndSet(false, true) and return immediately if it
fails; keep networkCaps as-is (local to the single callback) and proceed to call
cm.registerNetworkCallback only when initialization succeeds so you never
register a second callback that would capture a separate networkCaps map and
race on _hasInternet/_isWifiConnected.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/main/java/app/gamenative/MainActivity.ktapp/src/main/java/app/gamenative/NetworkMonitor.ktapp/src/main/java/app/gamenative/PluviaApp.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.ktapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt
- app/src/main/res/values/strings.xml
process-wide network state lives in NetworkMonitor, initialized once in PluviaApp.onCreate. fixes stale state when activity is destroyed while SteamService still runs. also addresses review feedback: validate WiFi state (captive portal), guard onLost for multi-network, extract checkWifiOrNotify helper, localize notification strings, seed initial state, add idempotency guard.
use NetworkMonitor to detect no-internet on launch and skip the 10s connection timeout, going straight to offline mode
jeremybernstein
force-pushed
the
jb/dedup-network-check
branch
from
4189ded to
abdab69
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
2619-2622:⚠️ Potential issue | 🟠 MajorUse live network query in
onLostto avoid stale Wi‑Fi state.Line 2621 reads a shared flow snapshot that can still be stale when callback delivery order differs between registrations, so downloads may fail to pause on actual Wi‑Fi loss.
🛠️ Suggested fix
networkCallback = object : ConnectivityManager.NetworkCallback() { override fun onLost(network: Network) { - // only pause if no WiFi/LAN remains (avoids false pause on multi-network) - if (PrefManager.downloadOnWifiOnly && !isWifiConnected) { + // Query live state here to avoid callback-order races. + val hasRemainingValidatedWifi = connectivityManager.allNetworks.any { n -> + val caps = connectivityManager.getNetworkCapabilities(n) ?: return@any false + (caps.hasTransport(NetworkCapabilities.TRANSPORT_WIFI) || + caps.hasTransport(NetworkCapabilities.TRANSPORT_ETHERNET)) && + caps.hasCapability(NetworkCapabilities.NET_CAPABILITY_VALIDATED) + } + if (PrefManager.downloadOnWifiOnly && !hasRemainingValidatedWifi) { for ((appId, info) in downloadJobs.entries.toList()) { Timber.d("Cancelling job") info.cancel() PluviaApp.events.emit(AndroidEvent.DownloadPausedDueToConnectivity(appId)) removeDownloadJob(appId) } notificationHelper.notify(getString(R.string.download_paused_wifi)) } } }#!/bin/bash # Verify whether onLost still depends on shared snapshot state instead of live ConnectivityManager query. rg -n -C3 "override fun onLost|isWifiConnected|allNetworks|getNetworkCapabilities" \ app/src/main/java/app/gamenative/service/SteamService.kt \ app/src/main/java/app/gamenative/NetworkMonitor.ktAlso applies to: 2628-2629
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/SteamService.kt` around lines 2619 - 2622, The onLost callback currently uses the shared snapshot isWifiConnected which can be stale; change onLost in SteamService to query the live network state from ConnectivityManager for remaining networks (e.g., iterate ConnectivityManager.allNetworks and call getNetworkCapabilities(network) to check for NET_CAPABILITY_WIFI) instead of relying on isWifiConnected, then if PrefManager.downloadOnWifiOnly is true and no Wi‑Fi capability is found pause/handle entries in downloadJobs as before; ensure the check runs inside override fun onLost(network: Network) and remove dependence on the shared-flow snapshot so pause logic is based on live connectivity.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt (1)
239-242:PrefManager.downloadOnWifiOnlyis not reactive —wifiAllowedwon't recompose if the pref changes in-session.
wifiConnectedis aStateFlowand will trigger recomposition correctly, butPrefManager.downloadOnWifiOnlyis read as a plain property. In practice this is low-risk because changing the WiFi-only pref requires navigating to Settings (which will cause recomposition on return), but if the pref is ever mutated without navigation, the derivedwifiAllowedand all downstream button states will be stale.Consider exposing
downloadOnWifiOnlyas aStateFlow(or aderivedStateOfon a snapshot-observable value) so the entire expression recomposes reactively:♻️ Suggested approach
- val wifiAllowed = !PrefManager.downloadOnWifiOnly || wifiConnected + val downloadOnWifiOnly by PrefManager.downloadOnWifiOnlyFlow.collectAsState() + val wifiAllowed = !downloadOnWifiOnly || wifiConnected🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt` around lines 239 - 242, The code reads PrefManager.downloadOnWifiOnly as a plain property so wifiAllowed won’t recompose when the preference changes; make the preference reactive (e.g., expose PrefManager.downloadOnWifiOnly as a StateFlow/Flow or provide a snapshot-observable/derivedState) and consume it with collectAsState()/observeAsState() when computing wifiAllowed (keep the existing wifiConnected from NetworkMonitor.isWifiConnected.collectAsState); update the expression that defines wifiAllowed to use the reactive state so changes to downloadOnWifiOnly trigger recomposition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 2619-2622: The onLost callback currently uses the shared snapshot
isWifiConnected which can be stale; change onLost in SteamService to query the
live network state from ConnectivityManager for remaining networks (e.g.,
iterate ConnectivityManager.allNetworks and call getNetworkCapabilities(network)
to check for NET_CAPABILITY_WIFI) instead of relying on isWifiConnected, then if
PrefManager.downloadOnWifiOnly is true and no Wi‑Fi capability is found
pause/handle entries in downloadJobs as before; ensure the check runs inside
override fun onLost(network: Network) and remove dependence on the shared-flow
snapshot so pause logic is based on live connectivity.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt`:
- Around line 239-242: The code reads PrefManager.downloadOnWifiOnly as a plain
property so wifiAllowed won’t recompose when the preference changes; make the
preference reactive (e.g., expose PrefManager.downloadOnWifiOnly as a
StateFlow/Flow or provide a snapshot-observable/derivedState) and consume it
with collectAsState()/observeAsState() when computing wifiAllowed (keep the
existing wifiConnected from NetworkMonitor.isWifiConnected.collectAsState);
update the expression that defines wifiAllowed to use the reactive state so
changes to downloadOnWifiOnly trigger recomposition.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/main/java/app/gamenative/MainActivity.ktapp/src/main/java/app/gamenative/NetworkMonitor.ktapp/src/main/java/app/gamenative/PluviaApp.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.ktapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/PluviaApp.kt
Contributor
Author
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Thank you for the clarification!
🧠 Learnings used |
phobos665
reviewed
Feb 25, 2026
Contributor
There was a problem hiding this comment.
Is this something we could also apply to other places such as Epic's offline launcher?
Contributor
Author
There was a problem hiding this comment.
Yes, it's initialized as a singleton on PluviaApp (so is always available) and should be importable by anything.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #647
Summary
NetworkMonitorsingleton (process-wide, never unregistered) — removes duplicate one-shotConnectivityManagerquery from LibraryAppScreen and state tracking from SteamServiceAtomicBooleantoStateFlowso install button updates in real-time when network changesTest plan
Summary by CodeRabbit
Refactor
New Features
Behavior