Download accuracy, hardening and UI tweaks#682
Open
ribbit384 wants to merge 2 commits intoutkarshdalal:masterfrom
Open
Download accuracy, hardening and UI tweaks#682ribbit384 wants to merge 2 commits intoutkarshdalal:masterfrom
ribbit384 wants to merge 2 commits intoutkarshdalal:masterfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors DownloadInfo into a stateful, concurrency-safe class with phase/status flows, ETA/speed tracking, and JSON persistence; extends SteamService to persist/load resume scopes and snapshots; updates UI for phase/speed/ETA display and changes pause/resume API across app screens. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as LibraryAppScreen
participant Steam as SteamService
participant DL as DownloadInfo
participant FS as FileSystem
User->>UI: Request resume/download (allowPersistedProgress)
UI->>Steam: downloadApp(..., allowPersistedProgress=true)
Steam->>FS: loadPersistedResumeScope(appDir)
FS-->>Steam: persisted DLC scope?
Steam->>DL: create DownloadInfo(...)
Steam->>FS: DL.loadPersistedResumeSnapshot(appDir)
FS-->>DL: ResumeSnapshot(bytes, completedDepots)
DL->>DL: initializeBytesDownloaded(...) / initializeCompletedDepotIds(...)
loop download chunks
Steam->>DL: updateBytesDownloaded(delta, ts, trackSpeed=true)
DL->>DL: addSpeedSample -> trimOldSamples -> recalc progress/ETA
DL-->>Steam: progress/state flows update
Steam-->>UI: UI observes flows -> render phase, speed, ETA
end
rect rgba(76, 175, 80, 0.5)
Steam->>DL: persistProgressSnapshot(force=true) on completion
DL->>FS: write temp JSON -> rename to depot_bytes.json
FS-->>DL: persist ack
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 10
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/ui/screen/library/appscreen/EpicAppScreen.kt (1)
436-443:⚠️ Potential issue | 🟠 MajorUse
awaitCompletion()before cleanup in Epic pause flow to avoid race condition.The pause flow cancels the download then immediately runs cleanup on the main dispatcher without waiting for the worker to unwind. This races against active file operations in the worker, risking inconsistent state. The same file uses the correct pattern in the delete dialog flow (lines 805), and Steam uses it consistently (line 705). Also,
cleanupDownloaddoes I/O work and should run onDispatchers.IO, notMain.immediate.🔧 Suggested fix
if (shouldPause) { val downloadInfo = EpicService.getDownloadInfo(libraryItem.gameId) // Cancel/pause download Timber.tag(TAG).i("Pausing Epic download: ${libraryItem.gameId}") - downloadInfo?.cancel() - CoroutineScope(Dispatchers.Main.immediate).launch { + CoroutineScope(Dispatchers.IO).launch { + downloadInfo?.cancel() + downloadInfo?.awaitCompletion() EpicService.cleanupDownload(context, libraryItem.gameId) }🤖 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/appscreen/EpicAppScreen.kt` around lines 436 - 443, The pause flow cancels the Epic download and immediately calls cleanup on the Main dispatcher, which can race with worker file ops; change the sequence in the shouldPause block to call EpicService.getDownloadInfo(...), invoke downloadInfo?.cancel(), then await the worker to finish via downloadInfo?.awaitCompletion() before calling EpicService.cleanupDownload, and run cleanupDownload inside a coroutine on Dispatchers.IO (not Dispatchers.Main.immediate) to avoid doing I/O on the main thread; update references in this block to use downloadInfo.awaitCompletion() and Dispatchers.IO when launching the cleanup coroutine.
🧹 Nitpick comments (1)
app/src/main/res/values-de/strings.xml (1)
1151-1158: Keep German phase wording style consistent across states.The new labels mix “Wird …” phrasing and infinitive-style labels. For smoother UI copy, consider using one style consistently across all phase strings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/res/values-de/strings.xml` around lines 1151 - 1158, The German phase strings are inconsistent (mixing "Wird …" and infinitive styles); pick one style and make all library_download_phase_* values consistent—for example, use the progressive "Wird ..." form for library_download_phase_downloading, _preparing, _patching, _verifying, _applying_data, _finalizing, _paused and _failed (or convert all to infinitives) so each string resource (library_download_phase_downloading, library_download_phase_preparing, library_download_phase_patching, library_download_phase_verifying, library_download_phase_applying_data, library_download_phase_finalizing, library_download_phase_paused, library_download_phase_failed) follows the same grammatical pattern.
🤖 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/data/DownloadInfo.kt`:
- Around line 193-195: The catch blocks inside DownloadInfo that currently only
call hasDirtyProgressSnapshot.set(true) are swallowing IO exceptions—update each
of those catches (the ones that set hasDirtyProgressSnapshot) to log the caught
Exception (include e.message and stacktrace and contextual info like downloadId
or snapshot path) using the module's logger (or Timber) so failures are visible;
if the failure should be fatal for resume persistence, consider rethrowing or
recording a persistent error state in addition to setting
hasDirtyProgressSnapshot. Ensure this change is applied to all occurrences (the
existing catch blocks around resume/snapshot persistence where
hasDirtyProgressSnapshot.set(true) is used).
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 2234-2239: The code currently calls clearFailedResumeState(appId)
for all failures in the DownloadFailedException handlers (the catch block
handling DownloadFailedException and the similar block around lines ~2253-2259),
which removes persisted resume artifacts even for transient errors; change the
logic to only clear resume state for non-transient/permanent failures (e.g.,
explicit user cancellations or errors classified as permanent). Implement or use
an isTransientFailure(e: Throwable): Boolean helper (or inspect properties on
DownloadFailedException) and only call clearFailedResumeState(appId) when
isTransientFailure(e) == false; always call removeDownloadJob(appId, di) and
update di.updateStatus / di.setActive(false) as currently done. Ensure the same
guarded behavior is applied to both failure sites that currently call
clearFailedResumeState.
- Around line 1715-1721: The check-then-insert around downloadJobs is not atomic
so concurrent callers can both pass the isActive() check and start duplicate
downloads; fix by making the claim atomic for a given appId: change downloadJobs
to a thread-safe map (e.g., ConcurrentHashMap) and use an atomic insertion
method (computeIfAbsent/putIfAbsent/compute) when creating the job instead of a
separate get + put, or wrap the check+insert in a synchronized block keyed by
appId; ensure removeDownloadJob and the isActive() logic still work with the
chosen atomic operation so only a single coroutine can be created for a given
appId.
- Around line 1929-1931: The deletion of stale persisted progress is currently
invoked asynchronously which can race with a subsequent forced snapshot write;
update the branch that checks allowPersistedProgress to perform the clear
operation synchronously (or await its completion) so
di.clearPersistedBytesDownloaded(appDirPath) finishes before any snapshot write
proceeds. Locate the check using allowPersistedProgress in SteamService (the
else-if block) and replace the async call with a blocking/awaiting call or call
the provided synchronous variant so the delete cannot remove newly written
progress.
- Around line 343-346: The partial-payload detection currently treats metadata
files like DOWNLOAD_INFO_FILE as evidence of content; update the predicate that
scans nestedFiles so it ignores metadata-only files by only counting real depot
payload files — e.g., change the check on nestedFiles (in SteamService.kt) to
require nested.isFile && nested.length() > 0 && nested.name !=
DOWNLOAD_INFO_FILE (or exclude any other known metadata filenames) so that
resume_scope.json/DOWNLOAD_INFO_FILE no longer triggers resume mode.
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt`:
- Around line 1427-1435: The code path in SteamAppScreen where downloadInfo is
null (after calling SteamService.downloadAppForVerify or
SteamService.downloadAppForUpdate) only logs a Timber warning and returns, which
causes a silent failure; update the null branch in the coroutine inside
SteamAppScreen (the block handling downloadInfo/operation) to show user feedback
(e.g., a Toast or Snackbar) explaining that starting the update/verify failed
and include the operation or gameId in the message, then return as before.
Ensure you reference the same symbols (operation, gameId, downloadInfo,
SteamService.downloadAppForUpdate, SteamService.downloadAppForVerify) and use
the UI context safe for coroutines (e.g., activity/requireContext() or
view?.context) when showing the Toast/Snackbar.
- Around line 693-713: The catch block in onPauseResumeClick currently only logs
exceptions so the user sees no feedback; modify the coroutine to post a
user-facing error (e.g., Toast or Snackbar) on the main thread when an exception
occurs while calling SteamService.getAppDownloadInfo or
SteamService.downloadApp. Keep the existing tryAcquirePauseResumeAction(gameId)
and releasePauseResumeAction(gameId) logic, but inside catch(e) call Timber.e(e,
...) and then switch to Dispatchers.Main (or use context.runOnUiThread) to show
a concise, UX-safe message like "Failed to pause/resume this app" using the
provided context parameter so the UI reflects the failure.
In `@app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt`:
- Around line 480-485: The ETA and placeholder text are hardcoded; update
formatStableEtaText to return a localized string resource (use context.getString
or stringResource with either a plural or formatted string like "%1$d m %2$d s
left") instead of inlining "${minutesLeft}m ${secondsPart}s left", and replace
any "0 / XX" or similar unknown-total placeholders (the other occurrences noted
around the "0 / XX" pattern) with resource-backed strings (including an "unknown
total" variant) so UI text is localizable and translatable.
- Around line 459-461: The numeric formatting calls (e.g., the bytes size
formatter lines that use String.format("%.2f GB", bytes / gb) and the network
speed formatter) are locale-implicit; update them to call the overload that
accepts an explicit Locale (for consistency, use Locale.US or another chosen
locale) — e.g., change to String.format(Locale.US, "...", ...) — and add the
necessary import (java.util.Locale); apply this change to all String.format uses
in the size and network speed helper methods so formatting behavior is
deterministic across device locales.
In `@app/src/main/res/values-it/strings.xml`:
- Line 1214: Update the string resource named library_download_phase_failed to
use the more idiomatic Italian label "Fallito" instead of "Non riuscito" so the
status chip reads naturally and matches common UI phrasing.
---
Outside diff comments:
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/EpicAppScreen.kt`:
- Around line 436-443: The pause flow cancels the Epic download and immediately
calls cleanup on the Main dispatcher, which can race with worker file ops;
change the sequence in the shouldPause block to call
EpicService.getDownloadInfo(...), invoke downloadInfo?.cancel(), then await the
worker to finish via downloadInfo?.awaitCompletion() before calling
EpicService.cleanupDownload, and run cleanupDownload inside a coroutine on
Dispatchers.IO (not Dispatchers.Main.immediate) to avoid doing I/O on the main
thread; update references in this block to use downloadInfo.awaitCompletion()
and Dispatchers.IO when launching the cleanup coroutine.
---
Nitpick comments:
In `@app/src/main/res/values-de/strings.xml`:
- Around line 1151-1158: The German phase strings are inconsistent (mixing "Wird
…" and infinitive styles); pick one style and make all library_download_phase_*
values consistent—for example, use the progressive "Wird ..." form for
library_download_phase_downloading, _preparing, _patching, _verifying,
_applying_data, _finalizing, _paused and _failed (or convert all to infinitives)
so each string resource (library_download_phase_downloading,
library_download_phase_preparing, library_download_phase_patching,
library_download_phase_verifying, library_download_phase_applying_data,
library_download_phase_finalizing, library_download_phase_paused,
library_download_phase_failed) follows the same grammatical pattern.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
app/src/main/java/app/gamenative/data/DownloadInfo.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/service/amazon/AmazonService.ktapp/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/AmazonAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/CustomGameAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/EpicAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.ktapp/src/main/res/values-da/strings.xmlapp/src/main/res/values-de/strings.xmlapp/src/main/res/values-es/strings.xmlapp/src/main/res/values-fr/strings.xmlapp/src/main/res/values-it/strings.xmlapp/src/main/res/values-ko/strings.xmlapp/src/main/res/values-pl/strings.xmlapp/src/main/res/values-pt-rBR/strings.xmlapp/src/main/res/values-ro/strings.xmlapp/src/main/res/values-uk/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values-zh-rTW/strings.xmlapp/src/main/res/values/strings.xml
Comment on lines
+193
to
+195
| } catch (e: Exception) { | ||
| hasDirtyProgressSnapshot.set(true) | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t swallow snapshot I/O exceptions silently.
These catches only flip flags (or ignore cleanup failure) without logging. When resume persistence fails repeatedly, root-cause analysis becomes very difficult.
Suggested fix
} catch (e: Exception) {
+ Timber.w(e, "Failed to persist progress snapshot (force)")
hasDirtyProgressSnapshot.set(true)
}
return
@@
} catch (e: Exception) {
+ Timber.w(e, "Failed to persist progress snapshot (async)")
hasDirtyProgressSnapshot.set(true)
} finally {
@@
} catch (e2: Exception) {
- // Ignore deletion errors
+ Timber.w(e2, "Failed to delete corrupt persisted snapshot at $appDirPath")
}Also applies to: 219-221, 526-528
🧰 Tools
🪛 detekt (1.23.8)
[warning] 193-193: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/data/DownloadInfo.kt` around lines 193 -
195, The catch blocks inside DownloadInfo that currently only call
hasDirtyProgressSnapshot.set(true) are swallowing IO exceptions—update each of
those catches (the ones that set hasDirtyProgressSnapshot) to log the caught
Exception (include e.message and stacktrace and contextual info like downloadId
or snapshot path) using the module's logger (or Timber) so failures are visible;
if the failure should be fatal for resume persistence, consider rethrowing or
recording a persistent error state in addition to setting
hasDirtyProgressSnapshot. Ensure this change is applied to all occurrences (the
existing catch blocks around resume/snapshot persistence where
hasDirtyProgressSnapshot.set(true) is used).
Comment on lines
+343
to
+346
| val nestedFiles = file.listFiles().orEmpty() | ||
| nestedFiles.any { nested -> | ||
| nested.name != DOWNLOAD_INFO_FILE | ||
| } |
Contributor
There was a problem hiding this comment.
Exclude metadata-only files from partial payload detection.
At Line [345], resume_scope.json is treated as evidence of partial payload, which can trigger resume mode even when no actual depot data exists. This can route users into incorrect resume behavior.
Suggested fix
- nestedFiles.any { nested ->
- nested.name != DOWNLOAD_INFO_FILE
- }
+ nestedFiles.any { nested ->
+ nested.name !in setOf(
+ DOWNLOAD_INFO_FILE,
+ DOWNLOAD_RESUME_SCOPE_FILE,
+ "$DOWNLOAD_INFO_FILE.tmp",
+ "$DOWNLOAD_RESUME_SCOPE_FILE.tmp",
+ )
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val nestedFiles = file.listFiles().orEmpty() | |
| nestedFiles.any { nested -> | |
| nested.name != DOWNLOAD_INFO_FILE | |
| } | |
| val nestedFiles = file.listFiles().orEmpty() | |
| nestedFiles.any { nested -> | |
| nested.name !in setOf( | |
| DOWNLOAD_INFO_FILE, | |
| DOWNLOAD_RESUME_SCOPE_FILE, | |
| "$DOWNLOAD_INFO_FILE.tmp", | |
| "$DOWNLOAD_RESUME_SCOPE_FILE.tmp", | |
| ) | |
| } |
🤖 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 343 -
346, The partial-payload detection currently treats metadata files like
DOWNLOAD_INFO_FILE as evidence of content; update the predicate that scans
nestedFiles so it ignores metadata-only files by only counting real depot
payload files — e.g., change the check on nestedFiles (in SteamService.kt) to
require nested.isFile && nested.length() > 0 && nested.name !=
DOWNLOAD_INFO_FILE (or exclude any other known metadata filenames) so that
resume_scope.json/DOWNLOAD_INFO_FILE no longer triggers resume mode.
Comment on lines
+1715
to
+1721
| val currentDownloadInfo = downloadJobs[appId] | ||
| if (currentDownloadInfo != null) { | ||
| if (currentDownloadInfo.isActive()) { | ||
| return currentDownloadInfo | ||
| } | ||
| removeDownloadJob(appId, currentDownloadInfo) | ||
| } |
Contributor
There was a problem hiding this comment.
Make download job claiming atomic per appId.
The active-check at Line [1715] and insertion at Line [2278] are not atomic. Concurrent callers can both pass the check and start two download coroutines for the same app before either writes to downloadJobs.
Also applies to: 2278-2280
🤖 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 1715 -
1721, The check-then-insert around downloadJobs is not atomic so concurrent
callers can both pass the isActive() check and start duplicate downloads; fix by
making the claim atomic for a given appId: change downloadJobs to a thread-safe
map (e.g., ConcurrentHashMap) and use an atomic insertion method
(computeIfAbsent/putIfAbsent/compute) when creating the job instead of a
separate get + put, or wrap the check+insert in a synchronized block keyed by
appId; ensure removeDownloadJob and the isActive() logic still work with the
chosen atomic operation so only a single coroutine can be created for a given
appId.
Comment on lines
+1929
to
+1931
| } else if (!allowPersistedProgress) { | ||
| di.clearPersistedBytesDownloaded(appDirPath) | ||
| } |
Contributor
There was a problem hiding this comment.
Clear stale snapshot synchronously on fresh-download setup.
At Line [1930], async cleanup can race with a later forced snapshot write (e.g., quick cancel right after start), causing the queued delete to remove newly persisted progress.
Suggested fix
- } else if (!allowPersistedProgress) {
- di.clearPersistedBytesDownloaded(appDirPath)
+ } else if (!allowPersistedProgress) {
+ di.clearPersistedBytesDownloaded(appDirPath, sync = true)
}🤖 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 1929 -
1931, The deletion of stale persisted progress is currently invoked
asynchronously which can race with a subsequent forced snapshot write; update
the branch that checks allowPersistedProgress to perform the clear operation
synchronously (or await its completion) so
di.clearPersistedBytesDownloaded(appDirPath) finishes before any snapshot write
proceeds. Locate the check using allowPersistedProgress in SteamService (the
else-if block) and replace the async call with a blocking/awaiting call or call
the provided synchronous variant so the delete cannot remove newly written
progress.
Comment on lines
+2234
to
+2239
| } catch (e: DownloadFailedException) { | ||
| Timber.d(e, "Download failed for app $appId via cancellation") | ||
| clearFailedResumeState(appId) | ||
| di.updateStatus(DownloadPhase.FAILED) | ||
| di.setActive(false) | ||
| removeDownloadJob(appId, di) |
Contributor
There was a problem hiding this comment.
Avoid wiping resume state on all failures.
Both failure paths call clearFailedResumeState, which removes persisted snapshot/scope and DB resume row. For transient failures, this defeats resumability and can force expensive rework on retry.
Also applies to: 2253-2259
🤖 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 2234 -
2239, The code currently calls clearFailedResumeState(appId) for all failures in
the DownloadFailedException handlers (the catch block handling
DownloadFailedException and the similar block around lines ~2253-2259), which
removes persisted resume artifacts even for transient errors; change the logic
to only clear resume state for non-transient/permanent failures (e.g., explicit
user cancellations or errors classified as permanent). Implement or use an
isTransientFailure(e: Throwable): Boolean helper (or inspect properties on
DownloadFailedException) and only call clearFailedResumeState(appId) when
isTransientFailure(e) == false; always call removeDownloadJob(appId, di) and
update di.updateStatus / di.setActive(false) as currently done. Ensure the same
guarded behavior is applied to both failure sites that currently call
clearFailedResumeState.
Comment on lines
+693
to
713
| override fun onPauseResumeClick(context: Context, libraryItem: LibraryItem, shouldPause: Boolean) { | ||
| val gameId = libraryItem.gameId | ||
| val downloadInfo = SteamService.getAppDownloadInfo(gameId) | ||
| val isDownloading = downloadInfo != null && (downloadInfo.getProgress() ?: 0f) < 1f | ||
| if (!tryAcquirePauseResumeAction(gameId)) { | ||
| Timber.d("Ignoring duplicate pause/resume tap for appId=$gameId") | ||
| return | ||
| } | ||
|
|
||
| if (isDownloading) { | ||
| downloadInfo?.cancel() | ||
| } else { | ||
| CoroutineScope(Dispatchers.IO).launch { | ||
| SteamService.downloadApp(gameId) | ||
| CoroutineScope(Dispatchers.IO).launch { | ||
| try { | ||
| if (shouldPause) { | ||
| val downloadInfo = SteamService.getAppDownloadInfo(gameId) | ||
| downloadInfo?.cancel() | ||
| downloadInfo?.awaitCompletion(timeoutMs = 1200L) | ||
| } else { | ||
| SteamService.downloadApp(gameId) | ||
| } | ||
| } catch (e: Exception) { | ||
| Timber.e(e, "Pause/resume action failed for appId=$gameId") | ||
| } finally { | ||
| releasePauseResumeAction(gameId) | ||
| } |
Contributor
There was a problem hiding this comment.
Surface pause/resume failures to the user, not only logs.
On Line 709, exceptions are logged but the UI gets no feedback, so failures look like ignored taps.
Suggested UX-safe error feedback
} catch (e: Exception) {
Timber.e(e, "Pause/resume action failed for appId=$gameId")
+ withContext(Dispatchers.Main) {
+ Toast.makeText(
+ context,
+ context.getString(R.string.download_failed_try_again),
+ Toast.LENGTH_SHORT,
+ ).show()
+ }
} finally {
releasePauseResumeAction(gameId)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| override fun onPauseResumeClick(context: Context, libraryItem: LibraryItem, shouldPause: Boolean) { | |
| val gameId = libraryItem.gameId | |
| val downloadInfo = SteamService.getAppDownloadInfo(gameId) | |
| val isDownloading = downloadInfo != null && (downloadInfo.getProgress() ?: 0f) < 1f | |
| if (!tryAcquirePauseResumeAction(gameId)) { | |
| Timber.d("Ignoring duplicate pause/resume tap for appId=$gameId") | |
| return | |
| } | |
| if (isDownloading) { | |
| downloadInfo?.cancel() | |
| } else { | |
| CoroutineScope(Dispatchers.IO).launch { | |
| SteamService.downloadApp(gameId) | |
| CoroutineScope(Dispatchers.IO).launch { | |
| try { | |
| if (shouldPause) { | |
| val downloadInfo = SteamService.getAppDownloadInfo(gameId) | |
| downloadInfo?.cancel() | |
| downloadInfo?.awaitCompletion(timeoutMs = 1200L) | |
| } else { | |
| SteamService.downloadApp(gameId) | |
| } | |
| } catch (e: Exception) { | |
| Timber.e(e, "Pause/resume action failed for appId=$gameId") | |
| } finally { | |
| releasePauseResumeAction(gameId) | |
| } | |
| override fun onPauseResumeClick(context: Context, libraryItem: LibraryItem, shouldPause: Boolean) { | |
| val gameId = libraryItem.gameId | |
| if (!tryAcquirePauseResumeAction(gameId)) { | |
| Timber.d("Ignoring duplicate pause/resume tap for appId=$gameId") | |
| return | |
| } | |
| CoroutineScope(Dispatchers.IO).launch { | |
| try { | |
| if (shouldPause) { | |
| val downloadInfo = SteamService.getAppDownloadInfo(gameId) | |
| downloadInfo?.cancel() | |
| downloadInfo?.awaitCompletion(timeoutMs = 1200L) | |
| } else { | |
| SteamService.downloadApp(gameId) | |
| } | |
| } catch (e: Exception) { | |
| Timber.e(e, "Pause/resume action failed for appId=$gameId") | |
| withContext(Dispatchers.Main) { | |
| Toast.makeText( | |
| context, | |
| context.getString(R.string.download_failed_try_again), | |
| Toast.LENGTH_SHORT, | |
| ).show() | |
| } | |
| } finally { | |
| releasePauseResumeAction(gameId) | |
| } | |
| } | |
| } |
🤖 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/appscreen/SteamAppScreen.kt`
around lines 693 - 713, The catch block in onPauseResumeClick currently only
logs exceptions so the user sees no feedback; modify the coroutine to post a
user-facing error (e.g., Toast or Snackbar) on the main thread when an exception
occurs while calling SteamService.getAppDownloadInfo or
SteamService.downloadApp. Keep the existing tryAcquirePauseResumeAction(gameId)
and releasePauseResumeAction(gameId) logic, but inside catch(e) call Timber.e(e,
...) and then switch to Dispatchers.Main (or use context.runOnUiThread) to show
a concise, UX-safe message like "Failed to pause/resume this app" using the
provided context parameter so the UI reflects the failure.
Comment on lines
+1427
to
+1435
| val downloadInfo = when (operation) { | ||
| AppOptionMenuType.VerifyFiles -> SteamService.downloadAppForVerify(gameId) | ||
| AppOptionMenuType.Update -> SteamService.downloadAppForUpdate(gameId) | ||
| else -> SteamService.downloadAppForUpdate(gameId) | ||
| } | ||
| if (downloadInfo == null) { | ||
| Timber.w("Failed to start $operation for appId=$gameId") | ||
| return@launch | ||
| } |
Contributor
There was a problem hiding this comment.
Add user feedback when update/verify start fails.
On Line 1432, a null downloadInfo only logs a warning and silently returns. Showing a toast would prevent silent failure UX.
Suggested feedback on start failure
if (downloadInfo == null) {
Timber.w("Failed to start $operation for appId=$gameId")
+ withContext(Dispatchers.Main) {
+ Toast.makeText(
+ context,
+ context.getString(R.string.download_failed_try_again),
+ Toast.LENGTH_SHORT,
+ ).show()
+ }
return@launch
}🤖 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/appscreen/SteamAppScreen.kt`
around lines 1427 - 1435, The code path in SteamAppScreen where downloadInfo is
null (after calling SteamService.downloadAppForVerify or
SteamService.downloadAppForUpdate) only logs a Timber warning and returns, which
causes a silent failure; update the null branch in the coroutine inside
SteamAppScreen (the block handling downloadInfo/operation) to show user feedback
(e.g., a Toast or Snackbar) explaining that starting the update/verify failed
and include the operation or gameId in the message, then return as before.
Ensure you reference the same symbols (operation, gameId, downloadInfo,
SteamService.downloadAppForUpdate, SteamService.downloadAppForVerify) and use
the UI context safe for coroutines (e.g., activity/requireContext() or
view?.context) when showing the Toast/Snackbar.
Comment on lines
480
to
485
| private fun formatStableEtaText(etaMs: Long): String { | ||
| val totalSeconds = ((etaMs + 999L) / 1000L).coerceAtLeast(0L) | ||
| val minutesLeft = totalSeconds / 60L | ||
| val secondsPart = totalSeconds % 60L | ||
| return "${minutesLeft}m ${secondsPart}s left" | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid hardcoded/non-localized ETA and placeholder text.
The new "Xm Ys left" and "XX" placeholders are user-facing but not localized. Please move these to string resources so they translate correctly.
💡 Suggested direction
-private fun formatStableEtaText(etaMs: Long): String {
+private fun formatStableEtaText(context: Context, etaMs: Long): String {
val totalSeconds = ((etaMs + 999L) / 1000L).coerceAtLeast(0L)
val minutesLeft = totalSeconds / 60L
val secondsPart = totalSeconds % 60L
- return "${minutesLeft}m ${secondsPart}s left"
+ return context.getString(R.string.library_download_eta_minutes_seconds_left, minutesLeft, secondsPart)
}And replace "0 / XX"-style literals with resource-backed strings (e.g., unknown-total variants).
Also applies to: 631-636
🤖 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 480 - 485, The ETA and placeholder text are hardcoded; update
formatStableEtaText to return a localized string resource (use context.getString
or stringResource with either a plural or formatted string like "%1$d m %2$d s
left") instead of inlining "${minutesLeft}m ${secondsPart}s left", and replace
any "0 / XX" or similar unknown-total placeholders (the other occurrences noted
around the "0 / XX" pattern) with resource-backed strings (including an "unknown
total" variant) so UI text is localizable and translatable.
| <string name="library_download_phase_applying_data">Applicazione dati</string> | ||
| <string name="library_download_phase_finalizing">Finalizzazione</string> | ||
| <string name="library_download_phase_paused">In pausa</string> | ||
| <string name="library_download_phase_failed">Non riuscito</string> |
Contributor
There was a problem hiding this comment.
Use a more idiomatic Italian failed-state label.
Line 1214 reads slightly unnatural for a status chip. Prefer "Fallito" for consistency with common Italian UI phrasing.
Suggested copy tweak
- <string name="library_download_phase_failed">Non riuscito</string>
+ <string name="library_download_phase_failed">Fallito</string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <string name="library_download_phase_failed">Non riuscito</string> | |
| <string name="library_download_phase_failed">Fallito</string> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/res/values-it/strings.xml` at line 1214, Update the string
resource named library_download_phase_failed to use the more idiomatic Italian
label "Fallito" instead of "Non riuscito" so the status chip reads naturally and
matches common UI phrasing.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
8 issues found across 23 files
Prompt for AI agents (unresolved 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/ui/screen/library/LibraryAppScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt:467">
P2: Download speed/ETA UI strings are hardcoded instead of using localized string resources</violation>
<violation number="2" location="app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt:498">
P2: COMPLETE phase incorrectly mapped to 'downloading' label - missing dedicated complete state string</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/SteamService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamService.kt:345">
P1: The partial-download detection only excludes `DOWNLOAD_INFO_FILE` (`depot_bytes.json`) from the `.DownloadInfo` directory contents. The newly introduced `resume_scope.json` (and any `.tmp` files from atomic writes) are metadata-only files, but their presence will be treated as evidence of actual depot data on disk. This can trigger resume mode even when no real download payload exists, routing users into incorrect resume behavior.
Exclude all known metadata filenames:
```kotlin
nestedFiles.any { nested ->
nested.name !in setOf(
DOWNLOAD_INFO_FILE,
DOWNLOAD_RESUME_SCOPE_FILE,
"$DOWNLOAD_INFO_FILE.tmp",
"$DOWNLOAD_RESUME_SCOPE_FILE.tmp",
)
}
```</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/SteamService.kt:1811">
P1: Both the `DownloadFailedException` and generic `Exception` catch blocks call `clearFailedResumeState(appId)`, which deletes all persisted progress (snapshot files + DB resume row). For transient failures like network timeouts or temporary server errors, this destroys resumable progress and forces a full re-download on retry. Consider preserving resume state for the generic `Exception` path (similar to the `CancellationException` handler which persists the snapshot) so that only explicit/permanent failures wipe the state.</violation>
<violation number="3" location="app/src/main/java/app/gamenative/service/SteamService.kt:1930">
P2: The `clearPersistedBytesDownloaded(appDirPath)` call here defaults to `sync = false`, which queues an asynchronous file deletion on `SNAPSHOT_PERSIST_EXECUTOR`. If the user cancels shortly after download starts, the cancel handler writes a forced snapshot synchronously, but the still-queued async delete can then remove the freshly written progress. Use `sync = true` to ensure the stale file is deleted before the download coroutine begins writing new snapshots.</violation>
<violation number="4" location="app/src/main/java/app/gamenative/service/SteamService.kt:2234">
P2: Failure handling is internally inconsistent: `onDownloadFailed` listener preserves resume state for retry, but outer `catch (DownloadFailedException)` clears it via `clearFailedResumeState()`, potentially breaking retry resume functionality</violation>
</file>
<file name="app/src/main/java/app/gamenative/data/DownloadInfo.kt">
<violation number="1" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:193">
P2: This `catch` block swallows the exception without logging. When forced snapshot persistence fails, the dirty flag is set back to `true` but no diagnostic information is recorded. Repeated silent failures make root-cause analysis very difficult. Add a `Timber.w(e, ...)` log before re-setting the dirty flag.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:498">
P1: No fallback/migration for old resume file format can cause download progress loss on app upgrade</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } else { | ||
| val nestedFiles = file.listFiles().orEmpty() | ||
| nestedFiles.any { nested -> | ||
| nested.name != DOWNLOAD_INFO_FILE |
Contributor
There was a problem hiding this comment.
P1: The partial-download detection only excludes DOWNLOAD_INFO_FILE (depot_bytes.json) from the .DownloadInfo directory contents. The newly introduced resume_scope.json (and any .tmp files from atomic writes) are metadata-only files, but their presence will be treated as evidence of actual depot data on disk. This can trigger resume mode even when no real download payload exists, routing users into incorrect resume behavior.
Exclude all known metadata filenames:
nestedFiles.any { nested ->
nested.name !in setOf(
DOWNLOAD_INFO_FILE,
DOWNLOAD_RESUME_SCOPE_FILE,
"$DOWNLOAD_INFO_FILE.tmp",
"$DOWNLOAD_RESUME_SCOPE_FILE.tmp",
)
}Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/service/SteamService.kt, line 345:
<comment>The partial-download detection only excludes `DOWNLOAD_INFO_FILE` (`depot_bytes.json`) from the `.DownloadInfo` directory contents. The newly introduced `resume_scope.json` (and any `.tmp` files from atomic writes) are metadata-only files, but their presence will be treated as evidence of actual depot data on disk. This can trigger resume mode even when no real download payload exists, routing users into incorrect resume behavior.
Exclude all known metadata filenames:
```kotlin
nestedFiles.any { nested ->
nested.name !in setOf(
DOWNLOAD_INFO_FILE,
DOWNLOAD_RESUME_SCOPE_FILE,
"$DOWNLOAD_INFO_FILE.tmp",
"$DOWNLOAD_RESUME_SCOPE_FILE.tmp",
)
}
```</comment>
<file context>
@@ -303,22 +309,243 @@ class SteamService : Service(), IChallengeUrlChanged {
+ } else {
+ val nestedFiles = file.listFiles().orEmpty()
+ nestedFiles.any { nested ->
+ nested.name != DOWNLOAD_INFO_FILE
+ }
+ }
</file context>
Suggested change
| nested.name != DOWNLOAD_INFO_FILE | |
| nested.name !in setOf( | |
| DOWNLOAD_INFO_FILE, | |
| DOWNLOAD_RESUME_SCOPE_FILE, | |
| "$DOWNLOAD_INFO_FILE.tmp", | |
| "$DOWNLOAD_RESUME_SCOPE_FILE.tmp", | |
| ) |
| app.gamenative.data.DownloadPhase.PATCHING -> R.string.library_download_phase_patching | ||
| app.gamenative.data.DownloadPhase.APPLYING_DATA -> R.string.library_download_phase_applying_data | ||
| app.gamenative.data.DownloadPhase.FINALIZING -> R.string.library_download_phase_finalizing | ||
| app.gamenative.data.DownloadPhase.COMPLETE -> R.string.library_download_phase_downloading |
Contributor
There was a problem hiding this comment.
P2: COMPLETE phase incorrectly mapped to 'downloading' label - missing dedicated complete state string
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt, line 498:
<comment>COMPLETE phase incorrectly mapped to 'downloading' label - missing dedicated complete state string</comment>
<file context>
@@ -452,13 +456,49 @@ private fun formatBytes(bytes: Long): String {
+ app.gamenative.data.DownloadPhase.PATCHING -> R.string.library_download_phase_patching
+ app.gamenative.data.DownloadPhase.APPLYING_DATA -> R.string.library_download_phase_applying_data
+ app.gamenative.data.DownloadPhase.FINALIZING -> R.string.library_download_phase_finalizing
+ app.gamenative.data.DownloadPhase.COMPLETE -> R.string.library_download_phase_downloading
+ }
+}
</file context>
| runBlocking { | ||
| instance?.downloadingAppInfoDao?.deleteApp(appId) | ||
| } | ||
| } catch (e: DownloadFailedException) { |
Contributor
There was a problem hiding this comment.
P2: Failure handling is internally inconsistent: onDownloadFailed listener preserves resume state for retry, but outer catch (DownloadFailedException) clears it via clearFailedResumeState(), potentially breaking retry resume functionality
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/service/SteamService.kt, line 2234:
<comment>Failure handling is internally inconsistent: `onDownloadFailed` listener preserves resume state for retry, but outer `catch (DownloadFailedException)` clears it via `clearFailedResumeState()`, potentially breaking retry resume functionality</comment>
<file context>
@@ -1477,276 +1946,406 @@ class SteamService : Service(), IChallengeUrlChanged {
runBlocking {
instance?.downloadingAppInfoDao?.deleteApp(appId)
}
+ } catch (e: DownloadFailedException) {
+ Timber.d(e, "Download failed for app $appId via cancellation")
+ clearFailedResumeState(appId)
</file context>
ribbit384
force-pushed
the
downloadrefactor
branch
from
dcba56d to
e574bcf
Compare
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
1 participant
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.
Confirmed behavior works as expected for Steam across multiple scenarios. This PR in Javasteam needs to be merged for this to work properly also: joshuatam/JavaSteam#3
Need to verify that Epic / GoG / Amazon stores are not affected by these changes. Easy fix if so, just don't have a way to test myself at the moment.
Replace mutable primitives in
DownloadInfo(bytesDownloaded,totalExpectedBytes,speed) withAtomicLongand@Volatileto eliminate race conditions where concurrent depot download threads could corrupt byte counters, causing inaccurate progress and broken ETA calculations.Track completed depots individually (
completedDepotIdsbacked byConcurrentHashMap) so resumed downloads skip fully downloaded depots instead of re downloading or re verifying them, fixing games with many depots (for example language and DLC depots) restarting from scratch after pause or cancel.Persist
resume_scope.jsonalongsidedepot_bytes.jsonto recover the exact DLC selection on resume even when the DB metadata row is lost due to cancellation races, preventing resumed downloads from either missing DLC depots or pulling in unwanted ones.Add
sanitizeResumeSnapshot()to validate persisted completed depot IDs against current manifest data, dropping depots that no longer exist or whose recorded byte totals are inconsistent, preventing corrupt resume state from stalling downloads.Clamp bytes based progress to
0.99fwhile any tracked depot is still incomplete, so users don't lose hope that the download froze or something.Throttle progress snapshot persistence to a dedicated
ExecutorServicewith dirty flag gating and generation counters, avoiding IO contention that could possibly cause level byte updates across parallel depot downloads.Split download entry points into
downloadAppForUpdate()anddownloadAppForVerify()with explicitincludeInstalledDepotsandenableVerifyflags, fixing verify and update operations incorrectly filtering out already installed depots or skipping verification.Add
DownloadPhaseenum andisActive()check to replace progress based< 1fdownload detection across all UI and service call sites, fixing false downloading state on completed or failed downloads.Show real time network speed, ETA, and download phase (
Preparing,Verifying,Patching,Finalizing) in the UI with localized strings. Will add to other stores in the future.Summary by cubic
Improves Steam download accuracy and resilience with thread-safe counters, robust resume, and localized phase/speed/ETA in the UI. Hardens pause/resume across stores and removes unlocalized text. Requires JavaSteam PR joshuatam/JavaSteam#3.
Bug Fixes
New Features
Written for commit e574bcf. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation