Added fix for not showing partial downloads for GOG#624
Merged
utkarshdalal merged 1 commit intoutkarshdalal:masterfrom Mar 2, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds marker-aware download/install state and partial-delete handling, captures download coroutine jobs in DownloadInfo, and updates UI flows to confirm/cancel partial downloads and wire deletion/pause/resume through GOGService/GOGManager. Changes
Sequence DiagramsequenceDiagram
participant User
participant GOGAppScreen as GOGAppScreen
participant Dialog as Dialog Handler
participant GOGService
participant GOGManager
participant FileSystem
User->>GOGAppScreen: Click "Delete / Cancel Download"
GOGAppScreen->>Dialog: Show CANCEL_APP_DOWNLOAD dialog
User->>Dialog: Confirm
Dialog->>GOGAppScreen: Confirm action
GOGAppScreen->>GOGService: request deletePartialDownload(libraryItem)
GOGService->>GOGManager: deletePartialDownload(libraryItem)
GOGManager->>FileSystem: check/remove markers, delete manifest & install dir
FileSystem-->>GOGManager: result
GOGManager-->>GOGService: Result.success()/Result.failure
GOGService-->>GOGAppScreen: Return result
GOGAppScreen->>User: Show success or error toast / update UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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: 2
🧹 Nitpick comments (4)
app/src/main/java/app/gamenative/service/gog/GOGManager.kt (1)
521-559: Consider extracting shared cleanup logic withdeleteGame.
deletePartialDownloadshares the same core operations asdeleteGame(delete install dir, delete manifest, remove markers, emit event). Extracting a private helper (e.g.,cleanupGameFiles(gameId, libraryItem)) that both functions call would reduce duplication and ensure consistency when the cleanup logic changes.Also applies to: 464-519
🤖 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/gog/GOGManager.kt` around lines 521 - 559, The deletePartialDownload implementation duplicates cleanup steps found in deleteGame; extract a private helper (e.g., cleanupGameFiles) that accepts the gameId and LibraryItem (or appId/name) and performs getGameInstallPath -> deleteRecursively, delete manifest (manifests/$gameId), call MarkerUtils.removeMarker for DOWNLOAD_COMPLETE_MARKER and DOWNLOAD_IN_PROGRESS_MARKER (using getAppDirPath), and emit the LibraryInstallStatusChanged event via app.gamenative.PluviaApp.events.emitJava; then replace the bodies of deletePartialDownload and deleteGame to call this helper and return the same Result flow (preserve logging and exception handling in the caller or have the helper throw and callers wrap it).app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (3)
202-212: Partial-download detection delegates correctly, but check is file-system only.The check uses
java.io.File(installPath).exists()which doesn't distinguish between an empty directory and one with actual partial content. IfdeleteRecursively()ever fails midway (leaving an empty dir), this would perpetually report a partial download. Consider also checkinginstallDir.list()?.isNotEmpty() == truefor a more robust signal.Proposed refinement
private fun hasPartialDownloadForDelete(context: Context, libraryItem: LibraryItem): Boolean { if (isDownloading(context, libraryItem) || isInstalled(context, libraryItem)) return false val installPath = GOGConstants.getGameInstallPath(libraryItem.name) - return java.io.File(installPath).exists() + val dir = java.io.File(installPath) + return dir.exists() && dir.isDirectory }🤖 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/GOGAppScreen.kt` around lines 202 - 212, The current hasPartialDownloadForDelete uses java.io.File(installPath).exists() which returns true for empty dirs; update hasPartialDownloadForDelete (called by hasPartialDownload) to ensure the installPath is a directory and contains files by checking that File(installPath).isDirectory and File(installPath).list()?.isNotEmpty() == true (handle null safely), so only non-empty install dirs are treated as partial downloads; keep the early return for isDownloading/isInstalled and reuse GOGConstants.getGameInstallPath to locate the folder.
241-265: UnstructuredCoroutineScope(Dispatchers.IO).launchis a fire-and-forget coroutine.If the user navigates away before the coroutine completes, it will keep running with a captured
contextreference, risking a leaked Activity context. This pattern repeats in theCANCEL_APP_DOWNLOADhandler (line 603). Consider using a lifecycle-aware scope (e.g.,viewModelScopeor a scope tied to the screen lifecycle) instead.🤖 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/GOGAppScreen.kt` around lines 241 - 265, The fire-and-forget CoroutineScope(kotlinx.coroutines.Dispatchers.IO).launch in GOGAppScreen (the block that calls GOGService.getGOGGameOf and builds the install dialog) can leak the Activity context if the user navigates away; replace this ad-hoc scope with a lifecycle-aware scope (for example use a ViewModel's viewModelScope or the screen's lifecycleScope/rememberCoroutineScope) and launch the IO work via withContext(Dispatchers.IO) or similar so the coroutine is cancelled when the screen/ViewModel is destroyed; apply the same change for the similar coroutine in the CANCEL_APP_DOWNLOAD handler to ensure both use a lifecycle-bound scope instead of CoroutineScope(Dispatchers.IO).launch and do not capture the Activity context.
238-265: Use existing imports instead of fully qualified names.Lines 241, 244-246, 253-259 use fully qualified references (
kotlinx.coroutines.CoroutineScope,app.gamenative.utils.StorageUtils,app.gamenative.ui.component.dialog.state.MessageDialogState, etc.) even though some of these are already imported (e.g.,CoroutineScope,Dispatchers,launchat lines 27-29). The remaining FQNs should either be imported or the existing imports should be used consistently. Same pattern recurs in theCANCEL_APP_DOWNLOADhandler (lines 603-605).🤖 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/GOGAppScreen.kt` around lines 238 - 265, Replace the fully-qualified type and member references in showGOGInstallConfirmationDialog with the project’s existing imports: use CoroutineScope, Dispatchers.IO and launch (instead of kotlinx.coroutines.CoroutineScope/Dispatchers/launch), call StorageUtils.formatBinarySize and StorageUtils.getAvailableSpace via the imported StorageUtils (instead of app.gamenative.utils.StorageUtils), construct MessageDialogState using the imported class (instead of app.gamenative.ui.component.dialog.state.MessageDialogState), and reference GOGConstants.defaultGOGGamesPath via the imported GOGConstants; then call BaseAppScreen.showInstallDialog as before. Do the same cleanup in the CANCEL_APP_DOWNLOAD handler (replace app.gamenative.* FQNs with the corresponding imported symbols) so all references use the existing imports consistently.
🤖 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/gog/GOGManager.kt`:
- Around line 521-559: deletePartialDownload currently treats
installDir.deleteRecursively() returning false as non-fatal and still returns
Result.success; change the logic in the suspend fun deletePartialDownload so
that when installDir.exists() and deleteRecursively() returns false you log the
failure (Timber.w) and return Result.failure(...) (e.g., an IOException with a
message including installPath) instead of proceeding to Result.success; ensure
the failure is returned before emitting the LibraryInstallStatusChanged event so
callers (like GOGAppScreen) can detect the error.
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Around line 588-616: The CANCEL_APP_DOWNLOAD branch currently calls
downloadInfo.cancel() then launches deletePartialDownload asynchronously,
risking race between cancellation and file deletion; change the flow to
synchronously await confirmation that the download has stopped before deleting
(e.g., have GOGService.cleanupDownload or the object returned by
GOGService.getDownloadInfo expose a Job or suspendable confirmation and call
job.join() or await cleanup completion after downloadInfo.cancel()), then
perform GOGService.deletePartialDownload; also remove the suppressive error
check (result.isFailure && !isDownloading) and instead show the failure Toast
for any result.isFailure so deletion failures are not silently swallowed (adjust
references in this block: BaseAppScreen.hideInstallDialog,
GOGService.getDownloadInfo, downloadInfo.cancel(), GOGService.cleanupDownload,
GOGService.deletePartialDownload, and the result.isFailure check).
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/gog/GOGManager.kt`:
- Around line 521-559: The deletePartialDownload implementation duplicates
cleanup steps found in deleteGame; extract a private helper (e.g.,
cleanupGameFiles) that accepts the gameId and LibraryItem (or appId/name) and
performs getGameInstallPath -> deleteRecursively, delete manifest
(manifests/$gameId), call MarkerUtils.removeMarker for DOWNLOAD_COMPLETE_MARKER
and DOWNLOAD_IN_PROGRESS_MARKER (using getAppDirPath), and emit the
LibraryInstallStatusChanged event via app.gamenative.PluviaApp.events.emitJava;
then replace the bodies of deletePartialDownload and deleteGame to call this
helper and return the same Result flow (preserve logging and exception handling
in the caller or have the helper throw and callers wrap it).
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Around line 202-212: The current hasPartialDownloadForDelete uses
java.io.File(installPath).exists() which returns true for empty dirs; update
hasPartialDownloadForDelete (called by hasPartialDownload) to ensure the
installPath is a directory and contains files by checking that
File(installPath).isDirectory and File(installPath).list()?.isNotEmpty() == true
(handle null safely), so only non-empty install dirs are treated as partial
downloads; keep the early return for isDownloading/isInstalled and reuse
GOGConstants.getGameInstallPath to locate the folder.
- Around line 241-265: The fire-and-forget
CoroutineScope(kotlinx.coroutines.Dispatchers.IO).launch in GOGAppScreen (the
block that calls GOGService.getGOGGameOf and builds the install dialog) can leak
the Activity context if the user navigates away; replace this ad-hoc scope with
a lifecycle-aware scope (for example use a ViewModel's viewModelScope or the
screen's lifecycleScope/rememberCoroutineScope) and launch the IO work via
withContext(Dispatchers.IO) or similar so the coroutine is cancelled when the
screen/ViewModel is destroyed; apply the same change for the similar coroutine
in the CANCEL_APP_DOWNLOAD handler to ensure both use a lifecycle-bound scope
instead of CoroutineScope(Dispatchers.IO).launch and do not capture the Activity
context.
- Around line 238-265: Replace the fully-qualified type and member references in
showGOGInstallConfirmationDialog with the project’s existing imports: use
CoroutineScope, Dispatchers.IO and launch (instead of
kotlinx.coroutines.CoroutineScope/Dispatchers/launch), call
StorageUtils.formatBinarySize and StorageUtils.getAvailableSpace via the
imported StorageUtils (instead of app.gamenative.utils.StorageUtils), construct
MessageDialogState using the imported class (instead of
app.gamenative.ui.component.dialog.state.MessageDialogState), and reference
GOGConstants.defaultGOGGamesPath via the imported GOGConstants; then call
BaseAppScreen.showInstallDialog as before. Do the same cleanup in the
CANCEL_APP_DOWNLOAD handler (replace app.gamenative.* FQNs with the
corresponding imported symbols) so all references use the existing imports
consistently.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/service/gog/GOGManager.ktapp/src/main/java/app/gamenative/service/gog/GOGService.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues 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/ui/screen/library/appscreen/GOGAppScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt:599">
P2: Hardcoded UI strings should use string resources for localization/maintainability</violation>
<violation number="2" location="app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt:606">
P2: The condition `result.isFailure && !isDownloading` silently swallows delete failures when a download was active. If cancel succeeds but `deletePartialDownload` subsequently fails, the user will never see an error toast. Consider removing the `!isDownloading` guard or replacing it with more targeted logic (e.g., only suppress specific cancellation-related errors).</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/gog/GOGManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/GOGManager.kt:537">
P1: `deletePartialDownload` returns `Result.success(Unit)` even when `deleteRecursively()` returns `false`. The caller in `GOGAppScreen` checks `result.isFailure` to show an error toast, but this path will never trigger when the directory deletion partially fails. Return `Result.failure(...)` in the `else` branch so the caller can properly surface the error to the user.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
Outdated
Show resolved
Hide resolved
unbelievableflavour
force-pushed
the
gog-partial-downloads-support
branch
2 times, most recently
from
419cdd1 to
4a5fea5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (1)
588-617: Race condition and silent-failure issues from the previous review are now addressed.
downloadInfo?.awaitCompletion()before the delete (line 596) resolves the cancel-vs-delete race, andresult.isFailure(line 607) now unconditionally surfaces deletion errors regardless of whether a download was active. Both concerns from the prior review are fixed here.🤖 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/GOGAppScreen.kt` around lines 588 - 617, The logic correctly waits for cancellation and surfaces delete errors, but there are unnecessary nested braces and a raw CoroutineScope usage; remove the redundant extra `{ { ... } }` wrapper around the block that starts with BaseAppScreen.hideInstallDialog(appId) and replace CoroutineScope(Dispatchers.IO).launch { ... } with a lifecycle-aware scope (e.g., lifecycleScope.launch(Dispatchers.IO) { ... }) to avoid leaks; keep calls to GOGService.getDownloadInfo, downloadInfo?.cancel(), downloadInfo?.awaitCompletion(), GOGService.cleanupDownload, and GOGService.deletePartialDownload unchanged.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (1)
241-241: Use the already-importedCoroutineScope(Dispatchers.IO)— fully-qualified names are redundant here.
CoroutineScopeandDispatchersare already imported at lines 27–28. The fully-qualified form is inconsistent with every other call site in the same file.♻️ Proposed fix
- kotlinx.coroutines.CoroutineScope(kotlinx.coroutines.Dispatchers.IO).launch { + CoroutineScope(Dispatchers.IO).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/GOGAppScreen.kt` at line 241, Replace the fully-qualified coroutine constructor call in GOGAppScreen (the line using kotlinx.coroutines.CoroutineScope(kotlinx.coroutines.Dispatchers.IO).launch) with the already-imported symbols: use CoroutineScope(Dispatchers.IO).launch { ... } so the code matches other call sites and removes redundant qualification of CoroutineScope and Dispatchers.
🤖 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/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Around line 600-605: The toast uses a hardcoded message inside the
wasDownloading branch in GOGAppScreen (the Toast.makeText call); replace the
literal "Download cancelled" with a string resource (e.g.,
context.getString(R.string.download_cancelled)) and add the corresponding entry
in strings.xml (download_cancelled) so localization works; ensure you update any
imports/usage so the Toast still calls show() with the resource-backed string.
- Line 320: The inline `isDownloading` boolean uses only `downloadInfo != null
&& (downloadInfo.getProgress() ?: 0f) < 1f` which diverges from the class's
`isDownloading()` override that also requires `downloadInfo.isActive()`; update
both inline checks (the one that defines `isDownloading` and the one inside the
`CANCEL_APP_DOWNLOAD` handler) to use the unified check by calling the existing
helper `isDownloading(context, libraryItem)` (or, if you prefer to keep inline
logic, add the `downloadInfo.isActive()` check so the condition matches the
override). This ensures both code paths use the same three-part condition
(`downloadInfo != null`, `progress < 1f`, and `downloadInfo.isActive()`) and
prevents inconsistent handling of paused/stalled downloads.
- Around line 592-598: The coroutine re-checks downloadInfo after the user
confirms, so before calling GOGService.deletePartialDownload(gameId,
libraryItem) ensure the current installation state is validated: fetch/update
the latest state via GOGService.getDownloadInfo(gameId) and/or check
libraryItem.isInstalled (or call an explicit GOGService.isInstalled(gameId)) and
if the game is now fully installed, skip deletePartialDownload and instead call
performUninstall or return early; update the logic around downloadInfo,
wasDownloading, and the deletion call to bail out on a now-installed game to
avoid TOCTOU race issues.
- Around line 208-212: hasPartialDownloadForDelete currently repeats service
calls and performs blocking file I/O on the main thread; change its signature to
accept precomputed flags (e.g., isDownloadingFlag: Boolean, isInstalledFlag:
Boolean) so callers (like onDeleteDownloadClick) pass the results from
GOGService.getDownloadInfo() and isInstalled(), and remove internal calls to
isDownloading() and isInstalled(); also move the install-path existence check
off the main thread by performing File(installPath).exists() inside a
Dispatchers.IO coroutine (or suspend function) and return the result; lastly
ensure the boolean you pass for downloading uses the same semantics as the
existing isDownloading(context, libraryItem) (i.e., include the isActive()
requirement) to avoid behavioral differences.
---
Duplicate comments:
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Around line 588-617: The logic correctly waits for cancellation and surfaces
delete errors, but there are unnecessary nested braces and a raw CoroutineScope
usage; remove the redundant extra `{ { ... } }` wrapper around the block that
starts with BaseAppScreen.hideInstallDialog(appId) and replace
CoroutineScope(Dispatchers.IO).launch { ... } with a lifecycle-aware scope
(e.g., lifecycleScope.launch(Dispatchers.IO) { ... }) to avoid leaks; keep calls
to GOGService.getDownloadInfo, downloadInfo?.cancel(),
downloadInfo?.awaitCompletion(), GOGService.cleanupDownload, and
GOGService.deletePartialDownload unchanged.
---
Nitpick comments:
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Line 241: Replace the fully-qualified coroutine constructor call in
GOGAppScreen (the line using
kotlinx.coroutines.CoroutineScope(kotlinx.coroutines.Dispatchers.IO).launch)
with the already-imported symbols: use CoroutineScope(Dispatchers.IO).launch {
... } so the code matches other call sites and removes redundant qualification
of CoroutineScope and Dispatchers.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/service/gog/GOGManager.ktapp/src/main/java/app/gamenative/service/gog/GOGService.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/app/gamenative/service/gog/GOGService.kt
- app/src/main/java/app/gamenative/service/gog/GOGManager.kt
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
Outdated
Show resolved
Hide resolved
unbelievableflavour
force-pushed
the
gog-partial-downloads-support
branch
from
4a5fea5 to
5aa68ff
Compare
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/ui/screen/library/appscreen/GOGAppScreen.kt (1)
93-101:⚠️ Potential issue | 🟠 MajorEvent listener registered in
LaunchedEffectis never unregistered.The
installListenerregistered at line 100 viaPluviaApp.events.on<...>has no correspondingoff<...>call in aDisposableEffectoronDispose. Each recomposition ofgetGameDisplayInfowith a newgameIdwill register a new listener without removing the old one, leading to listener leaks.♻️ Suggested fix: use DisposableEffect
- LaunchedEffect(gameId) { - val installListener: (...) -> Unit = { event -> - ... - } - app.gamenative.PluviaApp.events.on<...>(installListener) - } + DisposableEffect(gameId) { + val installListener: (...) -> Unit = { event -> + if (event.appId == libraryItem.gameId) { + refreshTrigger++ + } + } + app.gamenative.PluviaApp.events.on<app.gamenative.events.AndroidEvent.LibraryInstallStatusChanged, Unit>(installListener) + onDispose { + app.gamenative.PluviaApp.events.off<app.gamenative.events.AndroidEvent.LibraryInstallStatusChanged, Unit>(installListener) + } + }🤖 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/GOGAppScreen.kt` around lines 93 - 101, The listener registered inside LaunchedEffect (installListener) is never removed, causing leaks; replace LaunchedEffect(gameId) with DisposableEffect(gameId) in GOGAppScreen.kt, register the listener via app.gamenative.PluviaApp.events.on<app.gamenative.events.AndroidEvent.LibraryInstallStatusChanged, Unit>(installListener) inside the DisposableEffect block and call app.gamenative.PluviaApp.events.off<app.gamenative.events.AndroidEvent.LibraryInstallStatusChanged, Unit>(installListener) in onDispose so the exact installListener instance is unregistered when gameId changes or the composable is disposed.
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (1)
598-641:⚠️ Potential issue | 🟡 MinorGood fix for the cancel/delete race and TOCTOU concerns.
The flow now correctly: (1) cancels the download, (2) awaits completion via
awaitCompletion(), (3) checksisGameInstalledbefore deciding whether to delete partial files. This addresses the previously flagged race condition and TOCTOU issues.Two remaining nits:
Hardcoded "Download cancelled" at lines 615 and 627 — should use a string resource for localization (flagged in a prior review).
Error toast uses
LENGTH_SHORTat line 635 — error messages benefit fromLENGTH_LONGto give users time to read the failure reason.♻️ Fix for toast duration
if (result.isFailure) { android.widget.Toast.makeText( context, "Failed to delete download: ${result.exceptionOrNull()?.message}", - android.widget.Toast.LENGTH_SHORT, + android.widget.Toast.LENGTH_LONG, ).show() },
🤖 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/GOGAppScreen.kt` around lines 598 - 641, In the CANCEL_APP_DOWNLOAD branch inside GOGAppScreen (the lambda handling DialogType.CANCEL_APP_DOWNLOAD), replace the two hardcoded "Download cancelled" Toast texts with string resources (e.g., context.getString(R.string.download_cancelled)) and change the error toast duration from android.widget.Toast.LENGTH_SHORT to android.widget.Toast.LENGTH_LONG when showing the "Failed to delete download" message after GOGService.deletePartialDownload(context, libraryItem) so the error message uses the localized string resource and a longer display duration.
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/service/gog/GOGService.kt (1)
300-359: Minor race window between coroutine launch andsetDownloadJob.The coroutine starts executing on
Dispatchers.IOat line 300 beforesetDownloadJob(job)runs at line 359. If the download fails near-instantly (e.g., network error), thefinallyblock (line 352) could remove the entry fromactiveDownloadsbefore the job is wired in. Any caller that obtained theDownloadInforeference and callsawaitCompletion()beforesetDownloadJobcompletes would see a null job.In practice the window is tiny, but if you want to close it:
💡 Suggested tightening
+ val downloadInfo = DownloadInfo(jobCount = 1, gameId = 0, downloadingAppIds = CopyOnWriteArrayList<Int>()) + val deferredJob = CompletableDeferred<Job>() + downloadInfo.setDownloadJob(/* wrapper that delegates to deferredJob */) + // Track in activeDownloads first instance.activeDownloads[gameId] = downloadInfo // Launch download in service scope so it runs independently val job = instance.scope.launch { ... } - downloadInfo.setDownloadJob(job) + deferredJob.complete(job)Alternatively, simply assign the job before the coroutine body can reach
finally, e.g. by wrapping the launch body in a smallyield()orensureActive()preamble.🤖 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/gog/GOGService.kt` around lines 300 - 359, The coroutine can run before downloadInfo.setDownloadJob(job) executes, creating a race where the finally block removes activeDownloads before the job is recorded; fix by creating the job in a non-started state (use launch with start = CoroutineStart.LAZY), call downloadInfo.setDownloadJob(job) immediately, then start the job (job.start()) so the DownloadInfo always has the Job before the coroutine body (and its finally block) can execute; reference instance.scope.launch, the job local, downloadInfo.setDownloadJob, activeDownloads and the finally block when applying this change.app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (1)
243-271: Unnecessary fully-qualified names and unstructured coroutine scope.Lines 246-265 use fully-qualified references (
kotlinx.coroutines.CoroutineScope,kotlinx.coroutines.Dispatchers.IO,app.gamenative.utils.StorageUtils,app.gamenative.service.gog.GOGConstants,app.gamenative.ui.component.dialog.state.MessageDialogState,app.gamenative.ui.enums.DialogType) despite the imports at lines 27-30 already covering the coroutine types. Also,CoroutineScope(Dispatchers.IO).launchcreates a fire-and-forget scope not tied to any lifecycle — if the user navigates away mid-operation, the coroutine continues and theshowInstallDialogcall on line 266 may act on a stale UI.This same pattern appears in
performDownload(line 276),onDeleteDownloadClick(line 345), andperformUninstall(line 371). Consider injecting or passing a lifecycle-aware scope.♻️ Minimal cleanup for this method
private fun showGOGInstallConfirmationDialog(context: Context, libraryItem: LibraryItem) { val gameId = libraryItem.gameId.toString() Timber.tag(TAG).i("Showing install confirmation dialog for: ${libraryItem.appId}") - kotlinx.coroutines.CoroutineScope(kotlinx.coroutines.Dispatchers.IO).launch { + CoroutineScope(Dispatchers.IO).launch { try { val game = GOGService.getGOGGameOf(gameId) - val downloadSize = app.gamenative.utils.StorageUtils.formatBinarySize(game?.downloadSize ?: 0L) - val availableSpace = app.gamenative.utils.StorageUtils.formatBinarySize( - app.gamenative.utils.StorageUtils.getAvailableSpace(app.gamenative.service.gog.GOGConstants.defaultGOGGamesPath) + val downloadSize = StorageUtils.formatBinarySize(game?.downloadSize ?: 0L) + val availableSpace = StorageUtils.formatBinarySize( + StorageUtils.getAvailableSpace(GOGConstants.defaultGOGGamesPath) )🤖 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/GOGAppScreen.kt` around lines 243 - 271, The method showGOGInstallConfirmationDialog uses fully-qualified type names and creates an unmanaged CoroutineScope; replace those fully-qualified references with the already-imported types (StorageUtils, GOGConstants, MessageDialogState, DialogType, Dispatchers/CoroutineScope) and move the coroutine work into a lifecycle-aware scope (e.g., a passed-in CoroutineScope or a viewModelScope / lifecycleScope from the owning component) so the coroutine is cancellable when the UI is destroyed; update showGOGInstallConfirmationDialog to accept/use that injected scope and perform the GOGService.getGOGGameOf call and BaseAppScreen.showInstallDialog inside scope.launch(Dispatchers.IO) (or use withContext) instead of CoroutineScope(Dispatchers.IO).launch; apply the same fix pattern to performDownload, onDeleteDownloadClick, and performUninstall to avoid fire-and-forget coroutines and remove unnecessary fully-qualified names.
🤖 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/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Around line 202-206: hasPartialDownload currently does blocking file I/O
(java.io.File(...).exists()) during composition; change this to an immediate
non-blocking pattern: in GOGAppScreen's remember initialize
hasPartialDownloadState to false and remove the direct disk check from
hasPartialDownload, then perform the exists() check off the main thread (e.g.,
coroutine / background task) and push the result into the UI by calling
observeGameState's callback onHasPartialDownloadChanged (or similar) to update
hasPartialDownloadState; reference the hasPartialDownload function,
GOGAppScreen, observeGameState, and onHasPartialDownloadChanged when making the
change.
---
Outside diff comments:
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Around line 93-101: The listener registered inside LaunchedEffect
(installListener) is never removed, causing leaks; replace
LaunchedEffect(gameId) with DisposableEffect(gameId) in GOGAppScreen.kt,
register the listener via
app.gamenative.PluviaApp.events.on<app.gamenative.events.AndroidEvent.LibraryInstallStatusChanged,
Unit>(installListener) inside the DisposableEffect block and call
app.gamenative.PluviaApp.events.off<app.gamenative.events.AndroidEvent.LibraryInstallStatusChanged,
Unit>(installListener) in onDispose so the exact installListener instance is
unregistered when gameId changes or the composable is disposed.
---
Duplicate comments:
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Around line 598-641: In the CANCEL_APP_DOWNLOAD branch inside GOGAppScreen
(the lambda handling DialogType.CANCEL_APP_DOWNLOAD), replace the two hardcoded
"Download cancelled" Toast texts with string resources (e.g.,
context.getString(R.string.download_cancelled)) and change the error toast
duration from android.widget.Toast.LENGTH_SHORT to
android.widget.Toast.LENGTH_LONG when showing the "Failed to delete download"
message after GOGService.deletePartialDownload(context, libraryItem) so the
error message uses the localized string resource and a longer display duration.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/gog/GOGService.kt`:
- Around line 300-359: The coroutine can run before
downloadInfo.setDownloadJob(job) executes, creating a race where the finally
block removes activeDownloads before the job is recorded; fix by creating the
job in a non-started state (use launch with start = CoroutineStart.LAZY), call
downloadInfo.setDownloadJob(job) immediately, then start the job (job.start())
so the DownloadInfo always has the Job before the coroutine body (and its
finally block) can execute; reference instance.scope.launch, the job local,
downloadInfo.setDownloadJob, activeDownloads and the finally block when applying
this change.
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Around line 243-271: The method showGOGInstallConfirmationDialog uses
fully-qualified type names and creates an unmanaged CoroutineScope; replace
those fully-qualified references with the already-imported types (StorageUtils,
GOGConstants, MessageDialogState, DialogType, Dispatchers/CoroutineScope) and
move the coroutine work into a lifecycle-aware scope (e.g., a passed-in
CoroutineScope or a viewModelScope / lifecycleScope from the owning component)
so the coroutine is cancellable when the UI is destroyed; update
showGOGInstallConfirmationDialog to accept/use that injected scope and perform
the GOGService.getGOGGameOf call and BaseAppScreen.showInstallDialog inside
scope.launch(Dispatchers.IO) (or use withContext) instead of
CoroutineScope(Dispatchers.IO).launch; apply the same fix pattern to
performDownload, onDeleteDownloadClick, and performUninstall to avoid
fire-and-forget coroutines and remove unnecessary fully-qualified names.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/service/gog/GOGManager.ktapp/src/main/java/app/gamenative/service/gog/GOGService.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/service/gog/GOGManager.kt
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
Outdated
Show resolved
Hide resolved
unbelievableflavour
force-pushed
the
gog-partial-downloads-support
branch
from
5aa68ff to
d48e023
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (1)
601-605: Hardcoded "Download cancelled" string should use a string resource.This was flagged in a previous review and is still present in two locations (lines 601–605 and 613–617). The same applies to other hardcoded toast messages in
performDownload(line 282) andperformUninstall(line 369).Also applies to: 613-617
🤖 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/GOGAppScreen.kt` around lines 601 - 605, Replace all hardcoded toast messages in GOGAppScreen.kt with string resources: locate the toasts in performDownload, performUninstall and the two occurrences around the cancel lines (currently showing "Download cancelled" at both places) and change calls to use context.getString(R.string.<key>) (e.g., R.string.download_cancelled). Add corresponding entries to strings.xml for each message key used (download_cancelled and any other messages currently hardcoded in performDownload and performUninstall) to keep messages centralized and localizable.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (1)
236-267: UnscopedCoroutineScope(Dispatchers.IO)is not lifecycle-aware.
CoroutineScope(Dispatchers.IO).launchon line 239 creates a fire-and-forget coroutine with no cancellation tie to the Activity/Fragment lifecycle. If the user navigates away before it completes, the coroutine continues running and callsBaseAppScreen.showInstallDialogon a stale screen.The same pattern appears in
performDownload(line 272) andperformUninstall(line 359). Consider using a sharedviewModelScopeor similar lifecycle-bound scope if one is available. Low risk here since the coroutine only sets dialog state, but worth noting for consistency.🤖 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/GOGAppScreen.kt` around lines 236 - 267, The coroutine launched in showGOGInstallConfirmationDialog uses an unscoped CoroutineScope(Dispatchers.IO).launch which is not lifecycle-aware; replace this fire-and-forget usage with a lifecycle-bound scope (e.g., viewModelScope or lifecycleScope from the associated ViewModel/Activity) so the work is cancelled if the UI goes away, and apply the same fix for the similar usages in performDownload and performUninstall; specifically locate the CoroutineScope(Dispatchers.IO).launch calls in showGOGInstallConfirmationDialog, performDownload, and performUninstall and move their logic to be launched via the lifecycle-aware scope (or expose a suspend function on the ViewModel that performs the GOGService call and dialog state update, then call that from the UI using viewModelScope).
🤖 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/gog/GOGDownloadManager.kt`:
- Around line 11-12: GOGDownloadManager currently sets DOWNLOAD_COMPLETE_MARKER
in finalizeInstallSuccess but never sets DOWNLOAD_IN_PROGRESS_MARKER, so
GOGManager.isGameInstalled (which checks for DOWNLOAD_IN_PROGRESS_MARKER) can’t
detect in-progress downloads; to fix, add a call to
MarkerUtils.addMarker(installPath, Marker.DOWNLOAD_IN_PROGRESS_MARKER) at the
start of the download flow in GOGDownloadManager.downloadGame (before kicking
off download tasks), mirroring EpicDownloadManager’s behavior, and ensure any
error/cleanup paths remove this marker or replace it with
DOWNLOAD_COMPLETE_MARKER in finalizeInstallSuccess.
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Around line 586-629: Update the cancel-flow UI logic in the coroutine that
handles app.gamenative.ui.screen.library.appscreen.GOGAppScreen: (1) change the
deletion-failure toast (the android.widget.Toast.makeText call that checks
result.isFailure) to use Toast.LENGTH_LONG to match other error toasts; (2) only
show the "Download cancelled" toast when the download was actually cancelled
(i.e., wasDownloading is true and the game is not installed after
awaitCompletion) — for example, replace the current uses of wasDownloading in
the withContext(Dispatchers.Main) blocks with a condition that also checks
!GOGService.isGameInstalled(gameId) (or check installation state captured after
awaitCompletion) so you don’t show "Download cancelled" when the download
already completed.
---
Duplicate comments:
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Around line 601-605: Replace all hardcoded toast messages in GOGAppScreen.kt
with string resources: locate the toasts in performDownload, performUninstall
and the two occurrences around the cancel lines (currently showing "Download
cancelled" at both places) and change calls to use
context.getString(R.string.<key>) (e.g., R.string.download_cancelled). Add
corresponding entries to strings.xml for each message key used
(download_cancelled and any other messages currently hardcoded in
performDownload and performUninstall) to keep messages centralized and
localizable.
---
Nitpick comments:
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Around line 236-267: The coroutine launched in
showGOGInstallConfirmationDialog uses an unscoped
CoroutineScope(Dispatchers.IO).launch which is not lifecycle-aware; replace this
fire-and-forget usage with a lifecycle-bound scope (e.g., viewModelScope or
lifecycleScope from the associated ViewModel/Activity) so the work is cancelled
if the UI goes away, and apply the same fix for the similar usages in
performDownload and performUninstall; specifically locate the
CoroutineScope(Dispatchers.IO).launch calls in showGOGInstallConfirmationDialog,
performDownload, and performUninstall and move their logic to be launched via
the lifecycle-aware scope (or expose a suspend function on the ViewModel that
performs the GOGService call and dialog state update, then call that from the UI
using viewModelScope).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.ktapp/src/main/java/app/gamenative/service/gog/GOGManager.ktapp/src/main/java/app/gamenative/service/gog/GOGService.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/service/gog/GOGService.kt
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
Show resolved
Hide resolved
unbelievableflavour
force-pushed
the
gog-partial-downloads-support
branch
from
d48e023 to
bdea327
Compare
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/ui/screen/library/appscreen/GOGAppScreen.kt (1)
502-512:⚠️ Potential issue | 🟡 Minor
onHasPartialDownloadChanged?.invoke(false)may leave the UI in an inconsistent state during download cancellation.When
DownloadStatusChanged(false)fires (line 510), the partial-download flag is unconditionally set tofalsebeforeGOGService.deleteGameruns. For the cancellation path, partial files can still exist (noDOWNLOAD_COMPLETE_MARKER) at this point. IfdeleteGamesubsequently fails, the actual state (partial files remain) will diverge from the UI state (false) untilonStateChanged()asynchronously re-evaluateshasPartialDownloadStateviahasPartialDownload().While
BaseAppScreen.onStateChanged()does eventually correct the state (it callsrequestStateRefresh()which re-evaluateshasPartialDownloadState), the current approach creates a window of inconsistency. Consider invokinghasPartialDownload(context, libraryItem)directly instead of passing a hardcodedfalse, ensuring the UI state matches the actual disk state immediately.🤖 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/GOGAppScreen.kt` around lines 502 - 512, The code unconditionally calls onHasPartialDownloadChanged?.invoke(false) in the DownloadStatusChanged(false) branch which can misreport UI state if deleteGame fails; change that call to compute the real disk state and pass it instead by invoking hasPartialDownload(context, libraryItem) (or otherwise re-evaluating hasPartialDownloadState) so the UI is updated to the actual partial-download presence immediately; adjust the DownloadStatusChanged handler around onHasPartialDownloadChanged, leaving onStateChanged() as-is for the eventual refresh.
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (1)
608-612: Hardcoded "Download cancelled" string — use a string resource.This was flagged in a prior review but remains unaddressed.
"Download cancelled"at line 610 should use a string resource (e.g.,context.getString(R.string.download_cancelled)) to support localization, consistent with other UI strings in this file.🤖 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/GOGAppScreen.kt` around lines 608 - 612, Replace the hardcoded "Download cancelled" Toast text in GOGAppScreen (the android.widget.Toast.makeText(...) call) with a localized string resource by calling context.getString(R.string.download_cancelled); also add the corresponding "download_cancelled" entry to your strings.xml. Locate the Toast invocation in GOGAppScreen.kt and update the second parameter to use context.getString(R.string.download_cancelled) and create the string resource in res/values/strings.xml to match other UI strings.
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (1)
269-315:onClickPlayparameter inperformDownloadis declared but never used.The
onClickPlay: (Boolean) -> Unitparameter is passed through at every call site (lines 583, where it's{}) but is never invoked or captured inside the function body. If it serves a future purpose, a comment explaining that intent would help; otherwise the parameter can be removed.♻️ Proposed cleanup
- private fun performDownload(context: Context, libraryItem: LibraryItem, onClickPlay: (Boolean) -> Unit) { + private fun performDownload(context: Context, libraryItem: LibraryItem) {and update the call site at line 583:
- performDownload(context, libraryItem) {} + performDownload(context, libraryItem)🤖 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/GOGAppScreen.kt` around lines 269 - 315, The onClickPlay parameter on performDownload is unused; remove the unused parameter to clean up the API and callers: update the performDownload function signature (remove onClickPlay: (Boolean) -> Unit) and delete any references inside the function, then update all call sites that pass an empty lambda (e.g., the caller that currently passes `{}`) to call the new two-argument performDownload(context, libraryItem) signature; alternatively, if the parameter is intentionally reserved, add a short TODO comment inside performDownload explaining its planned use to avoid removal.app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt (1)
181-192: Gen 1 download path skips theDOWNLOAD_IN_PROGRESS_MARKER.
downloadGameGen1returns at line 182 before the marker is written at line 369. This means Gen 1 games never receiveDOWNLOAD_IN_PROGRESS_MARKER.hasPartialDownloadonly checks for absence ofDOWNLOAD_COMPLETE_MARKER, so partial-download detection still works. However, any code that relies onDOWNLOAD_IN_PROGRESS_MARKERto identify an active Gen 1 install (e.g.,GOGManager.isGameInstalled) will not detect the in-progress state correctly.Consider setting the marker at the top of
downloadGameGen1(and removing it in its catch block) to keep the two paths symmetric.🤖 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/gog/GOGDownloadManager.kt` around lines 181 - 192, The Gen1 branch returns from within the withContext before writing DOWNLOAD_IN_PROGRESS_MARKER, so Gen1 downloads never get the in-progress marker; update the flow so the marker is created before handing off to downloadGameGen1 (or alternatively set the marker at the very start of downloadGameGen1) and ensure it is removed in downloadGameGen1's catch/failure path and replaced by DOWNLOAD_COMPLETE_MARKER on success; reference the symbols DOWNLOAD_IN_PROGRESS_MARKER, DOWNLOAD_COMPLETE_MARKER and the method downloadGameGen1 (and the early-return on selectedBuild.generation == 1) to make the two download paths symmetric.
🤖 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/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Around line 236-267: showGOGInstallConfirmationDialog is launching on
Dispatchers.IO but calls BaseAppScreen.showInstallDialog which mutates Compose
state (installDialogStates); move the UI state mutation onto the main thread:
after computing game/downloadSize/availableSpace, switch to the main dispatcher
(e.g., withContext(Dispatchers.Main) or launch(Dispatchers.Main)) and then call
BaseAppScreen.showInstallDialog(libraryItem.appId, state); keep the I/O work
(GOGService.getGOGGameOf and storage calculations) on IO and only perform the
Compose state update on Dispatchers.Main to avoid
StateModificationInWrongDispatcherException.
---
Outside diff comments:
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Around line 502-512: The code unconditionally calls
onHasPartialDownloadChanged?.invoke(false) in the DownloadStatusChanged(false)
branch which can misreport UI state if deleteGame fails; change that call to
compute the real disk state and pass it instead by invoking
hasPartialDownload(context, libraryItem) (or otherwise re-evaluating
hasPartialDownloadState) so the UI is updated to the actual partial-download
presence immediately; adjust the DownloadStatusChanged handler around
onHasPartialDownloadChanged, leaving onStateChanged() as-is for the eventual
refresh.
---
Duplicate comments:
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Around line 608-612: Replace the hardcoded "Download cancelled" Toast text in
GOGAppScreen (the android.widget.Toast.makeText(...) call) with a localized
string resource by calling context.getString(R.string.download_cancelled); also
add the corresponding "download_cancelled" entry to your strings.xml. Locate the
Toast invocation in GOGAppScreen.kt and update the second parameter to use
context.getString(R.string.download_cancelled) and create the string resource in
res/values/strings.xml to match other UI strings.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt`:
- Around line 181-192: The Gen1 branch returns from within the withContext
before writing DOWNLOAD_IN_PROGRESS_MARKER, so Gen1 downloads never get the
in-progress marker; update the flow so the marker is created before handing off
to downloadGameGen1 (or alternatively set the marker at the very start of
downloadGameGen1) and ensure it is removed in downloadGameGen1's catch/failure
path and replaced by DOWNLOAD_COMPLETE_MARKER on success; reference the symbols
DOWNLOAD_IN_PROGRESS_MARKER, DOWNLOAD_COMPLETE_MARKER and the method
downloadGameGen1 (and the early-return on selectedBuild.generation == 1) to make
the two download paths symmetric.
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt`:
- Around line 269-315: The onClickPlay parameter on performDownload is unused;
remove the unused parameter to clean up the API and callers: update the
performDownload function signature (remove onClickPlay: (Boolean) -> Unit) and
delete any references inside the function, then update all call sites that pass
an empty lambda (e.g., the caller that currently passes `{}`) to call the new
two-argument performDownload(context, libraryItem) signature; alternatively, if
the parameter is intentionally reserved, add a short TODO comment inside
performDownload explaining its planned use to avoid removal.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.ktapp/src/main/java/app/gamenative/service/gog/GOGManager.ktapp/src/main/java/app/gamenative/service/gog/GOGService.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/service/gog/GOGManager.kt
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
Show resolved
Hide resolved
unbelievableflavour
force-pushed
the
gog-partial-downloads-support
branch
from
bdea327 to
bae689f
Compare
ObfuscatedVoid
pushed a commit
to ObfuscatedVoid/GameNative
that referenced
this pull request
Mar 4, 2026
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.
Summary by cubic
Show partial GOG downloads using install-folder markers, and add a safer cancel/delete flow for incomplete installs. Resume now restarts the install when not downloading, with confirmation dialogs, job-aware cancellation, and clearer errors.
Written for commit bae689f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes