Streamlined launch dependencies retrieval flow#569
Closed
unbelievableflavour wants to merge 6 commits intoutkarshdalal:masterfrom
Closed
Streamlined launch dependencies retrieval flow#569unbelievableflavour wants to merge 6 commits intoutkarshdalal:masterfrom
unbelievableflavour wants to merge 6 commits intoutkarshdalal:masterfrom
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:
📝 WalkthroughWalkthroughThis PR refactors the game launch pre-flight dependency handling by consolidating scattered download and extraction logic into a centralized LaunchDependencies system. It replaces ~108 lines of step-by-step operations in PluviaMain with a single Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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: 6
🧹 Nitpick comments (6)
app/src/main/java/app/gamenative/utils/launchdependencies/LaunchDependency.kt (1)
28-33:CoroutineScopeparameter in asuspendfunction is an atypical structured-concurrency pattern.The
installimplementations launch child work on the caller-suppliedscope(viaSteamService.download*(..., scope, ...)) and thenawait()the result. In Kotlin structured concurrency, asuspendfunction's implicitcoroutineContextis the idiomatic way to inherit scope — passing an explicitCoroutineScopeas a parameter makes the child jobs siblings of (rather than true children of) this call site's coroutine. As long as callers always pass their ownthisscope (aspreLaunchAppdoes today), cancellation still propagates correctly; but it's an API footgun for future callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/launchdependencies/LaunchDependency.kt` around lines 28 - 33, The suspend function install currently takes an explicit CoroutineScope parameter which is an anti-pattern: remove the CoroutineScope parameter from the install signature and from all implementations, and use the calling coroutine's implicit scope via coroutineContext or by launching children with coroutineScope { ... } / withContext as needed so launched work becomes proper child coroutines; update calls to SteamService.download*(...) to accept the current coroutineContext (or be invoked inside coroutineScope) instead of the removed scope param and adjust callers such as preLaunchApp to stop passing their scope.app/src/main/java/app/gamenative/utils/launchdependencies/DrmExtrasDependency.kt (1)
10-10: Date-stamped filename is a maintenance footgun.
"experimental-drm-20260116.tzst"will need a code change every time the DRM asset is refreshed. Consider centralising asset filenames in a single constants file or deriving the name from a config/manifest fetched at runtime, so adding a new DRM file doesn't require touching each dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/launchdependencies/DrmExtrasDependency.kt` at line 10, The hard-coded, date-stamped constant FILE in DrmExtrasDependency.kt ("experimental-drm-20260116.tzst") forces code changes when the DRM asset is refreshed; replace this ad-hoc string with a centralized, maintainable source (e.g., read the filename from a shared constants/config/manifest or a runtime config service) by removing the literal FILE value and instead referencing a single maintained constant or a config lookup (e.g., Config.DRM_FILE or Manifest.getDrmFilename()) so future DRM updates only require changing one place or the external config rather than this class.app/src/main/java/app/gamenative/utils/LaunchDependencies.kt (2)
53-53: Extract the-1fsentinel into a named constant.
-1fis an opaque magic number. A named constant makes the intent clear and prevents the same literal being scattered across call sites.♻️ Proposed fix
+private const val PROGRESS_INDETERMINATE = -1f ... - setLoadingProgress(-1f) + setLoadingProgress(PROGRESS_INDETERMINATE)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/LaunchDependencies.kt` at line 53, Replace the magic literal -1f passed into setLoadingProgress with a named constant to convey intent: add a descriptive constant (e.g., LOADING_PROGRESS_UNKNOWN or LOADING_PROGRESS_SENTINEL) in LaunchDependencies.kt (top-level or inside a companion object) and use that constant in the call to setLoadingProgress instead of the literal; update any other occurrences of -1f sentinel used for loading progress to reference the same constant so the meaning is centralized and consistent.
46-53: Missingtry-finally: UI stays stuck in loading state on install failure.If any
dep.install()throws (e.g., network error, cancellation), lines 52–53 are skipped and the loading message/progress are never reset, leaving the UI in a perpetual loading state. The caller in PluviaMain (line 1070) does not provide exception handling. Wrap the loop in atry-finally:♻️ Proposed fix
val callbacks = LaunchDependencyCallbacks(setLoadingMessage, setLoadingProgress) + try { for (dep in getLaunchDependencies(container)) { if (!dep.isSatisfied(context, container)) { setLoadingMessage(dep.getLoadingMessage(context, container)) dep.install(context, container, scope, callbacks) } } + } finally { + setLoadingMessage(context.getString(R.string.main_loading)) + setLoadingProgress(-1f) + } - setLoadingMessage(context.getString(R.string.main_loading)) - setLoadingProgress(-1f)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/LaunchDependencies.kt` around lines 46 - 53, The loop over getLaunchDependencies(container) can throw inside dep.install(...) and currently prevents setLoadingMessage(...) and setLoadingProgress(...) from running; wrap the entire for (dep in getLaunchDependencies(container)) { ... } block in a try-finally and move the final setLoadingMessage(context.getString(R.string.main_loading)) and setLoadingProgress(-1f) into the finally block so the UI is always reset even when dep.install throws; keep existing calls to dep.isSatisfied, dep.getLoadingMessage and dep.install unchanged and decide whether to rethrow or log the exception inside the try as appropriate.app/src/main/java/app/gamenative/utils/launchdependencies/SteamTokenDependency.kt (2)
14-14: Extract hardcoded loading message to string resource for localization.
"Downloading steam-token"should use a string resource likeSteamClientDependencydoes withR.string.main_downloading_steam. This ensures the message is properly localized across the app's 13+ supported languages.♻️ Proposed fix
- override fun getLoadingMessage(context: Context, container: Container) = "Downloading steam-token" + override fun getLoadingMessage(context: Context, container: Container) = + context.getString(R.string.downloading_steam_token)Add the corresponding entry to
strings.xmland all locale variants:<string name="downloading_steam_token">Downloading steam-token</string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/launchdependencies/SteamTokenDependency.kt` at line 14, Replace the hardcoded "Downloading steam-token" in SteamTokenDependency.getLoadingMessage with a string resource lookup (use context.getString(R.string.downloading_steam_token) similar to how SteamClientDependency uses R.string.main_downloading_steam), and add the downloading_steam_token entry to strings.xml and all localized strings XML files with the translated text "Downloading steam-token".
15-27: Prefer structured concurrency withcoroutineScope { }for safer lifecycle management.The current implementation passes an external
scopeparameter todownloadFile(), which relies on that scope being tied to the calling coroutine's lifecycle. While this works correctly when PluviaMain passesthis, usingcoroutineScope { }makes the structured-concurrency intent explicit and prevents accidental lifecycle mismatches if the calling pattern changes:♻️ Suggested refactor
override suspend fun install( context: Context, container: Container, scope: CoroutineScope, callbacks: LaunchDependencyCallbacks, ) { + coroutineScope { SteamService.downloadFile( onDownloadProgress = callbacks.setLoadingProgress, - scope, + scope = this, context = context, FILE, ).await() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/launchdependencies/SteamTokenDependency.kt` around lines 15 - 27, The install method in SteamTokenDependency currently forwards an external scope into SteamService.downloadFile which risks lifecycle mismatch; wrap the download call inside a coroutineScope { } within SteamTokenDependency.install so the download is launched in the calling coroutine's structured scope (use coroutineScope { SteamService.downloadFile(..., scope = this, ...).await() } or pass a child coroutine context derived from coroutineScope) and remove reliance on the external scope parameter to ensure the download is cancelled with the install coroutine; update references to SteamService.downloadFile and the install signature accordingly.
🤖 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/PluviaMain.kt`:
- Around line 1068-1076: The call to ensureLaunchDependencies from inside the
CoroutineScope(Dispatchers.IO).launch can throw and leave the loading UI
visible; wrap the invocation of ensureLaunchDependencies in a try/catch/finally
inside that coroutine (or surrounding suspend function) so any exception is
caught, call setLoadingDialogVisible(false) in finally to guarantee the loading
overlay is dismissed, and in catch use the existing UI helpers (e.g.,
setLoadingMessage or an error dialog) to surface the error to the user;
reference ensureLaunchDependencies and the coroutine started with
CoroutineScope(Dispatchers.IO).launch to locate where to add the
try/catch/finally.
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/ImageFsBaseDependency.kt`:
- Line 13: The hardcoded string in ImageFsBaseDependency.getLoadingMessage
should be replaced with a localized string resource; update
getLoadingMessage(context: Context, container: Container) to return
context.getString(R.string.<appropriate_key>) (create a new string resource
entry like "downloading_first_time_files" if needed) to match the localization
approach used in SteamClientDependency and ImageFsInstallDependency; do the same
audit/fix for ImageFsPatchesDependency and DrmExtrasDependency so all loading
messages use context.getString(...) and consistent resource keys.
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/ImageFsInstallDependency.kt`:
- Line 12: The isSatisfied override currently always returns false causing
ensureLaunchDependencies to always show the loading message and call install();
change isSatisfied(context: Context, container: Container) to query the
installer for whether installation is actually required (e.g., call a new or
existing ImageFsInstaller.needsInstall() or similar API) and return that result;
if ImageFsInstaller lacks such a method, add a lightweight
needsInstall()/isInstallNeeded() that checks the installation state without
performing work, and have isSatisfied delegate to that so
setLoadingMessage("Installing glibc/bionic") and install() are only invoked when
real work is needed.
- Around line 25-27: The call to
ImageFsInstaller.installIfNeededFuture(...).get() blocks a thread and ignores
coroutine cancellation; replace the blocking get with a coroutine-friendly await
by wrapping ImageFsInstaller.installIfNeededFuture(...) in
suspendCancellableCoroutine (or use withContext(Dispatchers.IO) and
suspendCancellableCoroutine) so the callback progress still calls
callbacks.setLoadingProgress(progress / 100f), and ensure you cancel the
underlying Future in the coroutine's invokeOnCancellation handler; locate the
call site invoking ImageFsInstaller.installIfNeededFuture and swap .get() for
the cancellable suspension that cancels the Future when the coroutine is
cancelled.
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/ProtonPackageDependency.kt`:
- Around line 33-40: The call to
callbacks.setLoadingMessage(getLoadingMessage(...)) inside
ProtonPackageDependency.install is redundant because ensureLaunchDependencies
already sets the loading message before calling dep.install; remove that
duplicate call from the install method (look for ProtonPackageDependency.install
and the callbacks.setLoadingMessage invocation) so install only performs the
download via SteamService.downloadFile and progress updates without resetting
the loading message.
- Around line 17-18: The appliesTo implementation uses
wineVersion.contains(fileName.removeSuffix(".txz")) which can match a divergent
wineVersion and later cause extraction to look for the wrong
"<protonVersion>.txz"; change appliesTo to check against fileName (or
fileName.removeSuffix only when you also derive the exact archive name
consistently) so the matched container corresponds exactly to the downloaded
fileName, and when calling TarCompressorUtils.extract (or any extractor) wrap
the call in try/catch to log and rethrow or surface errors via the installer
flow (use processLogger or the class logger) so extraction failures are not
swallowed; update references in ProtonPackageDependency (appliesTo, extraction
call sites) to use the same fileName value and add exception handling around
TarCompressorUtils.extract to log the archive path and the caught exception.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/utils/LaunchDependencies.kt`:
- Line 53: Replace the magic literal -1f passed into setLoadingProgress with a
named constant to convey intent: add a descriptive constant (e.g.,
LOADING_PROGRESS_UNKNOWN or LOADING_PROGRESS_SENTINEL) in LaunchDependencies.kt
(top-level or inside a companion object) and use that constant in the call to
setLoadingProgress instead of the literal; update any other occurrences of -1f
sentinel used for loading progress to reference the same constant so the meaning
is centralized and consistent.
- Around line 46-53: The loop over getLaunchDependencies(container) can throw
inside dep.install(...) and currently prevents setLoadingMessage(...) and
setLoadingProgress(...) from running; wrap the entire for (dep in
getLaunchDependencies(container)) { ... } block in a try-finally and move the
final setLoadingMessage(context.getString(R.string.main_loading)) and
setLoadingProgress(-1f) into the finally block so the UI is always reset even
when dep.install throws; keep existing calls to dep.isSatisfied,
dep.getLoadingMessage and dep.install unchanged and decide whether to rethrow or
log the exception inside the try as appropriate.
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/DrmExtrasDependency.kt`:
- Line 10: The hard-coded, date-stamped constant FILE in DrmExtrasDependency.kt
("experimental-drm-20260116.tzst") forces code changes when the DRM asset is
refreshed; replace this ad-hoc string with a centralized, maintainable source
(e.g., read the filename from a shared constants/config/manifest or a runtime
config service) by removing the literal FILE value and instead referencing a
single maintained constant or a config lookup (e.g., Config.DRM_FILE or
Manifest.getDrmFilename()) so future DRM updates only require changing one place
or the external config rather than this class.
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/LaunchDependency.kt`:
- Around line 28-33: The suspend function install currently takes an explicit
CoroutineScope parameter which is an anti-pattern: remove the CoroutineScope
parameter from the install signature and from all implementations, and use the
calling coroutine's implicit scope via coroutineContext or by launching children
with coroutineScope { ... } / withContext as needed so launched work becomes
proper child coroutines; update calls to SteamService.download*(...) to accept
the current coroutineContext (or be invoked inside coroutineScope) instead of
the removed scope param and adjust callers such as preLaunchApp to stop passing
their scope.
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/SteamTokenDependency.kt`:
- Line 14: Replace the hardcoded "Downloading steam-token" in
SteamTokenDependency.getLoadingMessage with a string resource lookup (use
context.getString(R.string.downloading_steam_token) similar to how
SteamClientDependency uses R.string.main_downloading_steam), and add the
downloading_steam_token entry to strings.xml and all localized strings XML files
with the translated text "Downloading steam-token".
- Around line 15-27: The install method in SteamTokenDependency currently
forwards an external scope into SteamService.downloadFile which risks lifecycle
mismatch; wrap the download call inside a coroutineScope { } within
SteamTokenDependency.install so the download is launched in the calling
coroutine's structured scope (use coroutineScope {
SteamService.downloadFile(..., scope = this, ...).await() } or pass a child
coroutine context derived from coroutineScope) and remove reliance on the
external scope parameter to ensure the download is cancelled with the install
coroutine; update references to SteamService.downloadFile and the install
signature accordingly.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 11 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java">
<violation number="1" location="app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java:201">
P1: NullPointerException in 'needsInstall' and 'installIfNeededFuture' when container is null - the method dereferences container.getContainerVariant() without null checking, but the 2-arg overload explicitly passes null</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
unbelievableflavour
force-pushed
the
streamlined-launch-dependencies-flow
branch
from
ef179f6 to
5928c54
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/PluviaMain.kt`:
- Around line 1069-1089: The catch/finally block around ensureLaunchDependencies
must be changed: after logging and calling
setMessageDialogState(MessageDialogState(...)) inside the catch, add a
return@launch to stop further execution (prevent reaching
containerManager.activateContainer and onSuccess), remove the unconditional
setLoadingDialogVisible(false) from the finally block and instead hide the
loading dialog only on the success path after Steam cloud sync completes, and
change DialogType.NONE to DialogType.SYNC_FAIL when building the
MessageDialogState so the dismiss handler is wired up; reference
ensureLaunchDependencies, setMessageDialogState, MessageDialogState,
DialogType.SYNC_FAIL, return@launch, setLoadingDialogVisible, and
containerManager.activateContainer to locate the affected code.
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/ImageFsInstallDependency.kt`:
- Around line 23-40: The install function currently uses an expression body
causing the coroutine to infer a Boolean return and silently discards install
failures; change the implementation to a block body that uses
suspendCancellableCoroutine<Unit> and resume the continuation with Unit on
success, but if ImageFsInstaller.installIfNeededFuture(...) returns false then
resumeWithException (throw a descriptive IOException or IllegalStateException)
so failures are propagated instead of ignored; keep the existing
cont.invokeOnCancellation { future.cancel(true) } and the
callbacks.setLoadingProgress(progress / 100f) progress reporting unchanged.
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/ImageFsPatchesDependency.kt`:
- Around line 16-17: The build fails because
ImageFsPatchesDependency.getLoadingMessage references a missing string resource
R.string.main_downloading_wine; add a new string entry with the key
"main_downloading_wine" to your strings.xml (and any locale variants) containing
an appropriate user-visible message like "Downloading Wine…" so the call in
getLoadingMessage(Context, Container) resolves.
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/ProtonPackageDependency.kt`:
- Line 29: Replace the hardcoded user-facing strings in
ProtonPackageDependency.kt by using Android string resources with placeholders:
change getLoadingMessage(context, container) to return
context.getString(R.string.downloading_proton, displayName) and replace
callbacks.setLoadingMessage("Extracting $protonVersion") with
callbacks.setLoadingMessage(context.getString(R.string.extracting_proton,
protonVersion)); add the corresponding entries (with %1$s placeholders) to
strings.xml and use the same resource IDs where other dependencies follow this
pattern so localization is preserved.
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/SteamTokenDependency.kt`:
- Around line 15-16: The code references a missing string resource
R.string.main_downloading_steam_token in SteamTokenDependency.getLoadingMessage;
add a new string entry with that key to your app's strings.xml (and replicate
into all locale variants or provide placeholders) — e.g., create <string
name="main_downloading_steam_token">Downloading Steam token…</string> in the
main strings.xml and ensure localized strings.xml files are updated so the
resource resolves at build time.
In `@app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java`:
- Around line 201-211: The needsInstall method can NPE when container is null;
update needsInstall(Context, Container) to handle a null container (e.g., treat
null as "no variant" or return true if container is required) by guarding the
final comparison against container.getContainerVariant(), and update
installIfNeededFuture(Context, AssetManager, Container, Callback) to not call
container.getContainerVariant() without a null check; also remove the dead local
ImageFs imageFs in installIfNeededFuture since needsInstall calls
ImageFs.find(context) internally. Ensure references: needsInstall,
installIfNeededFuture, ImageFs.find, imageFs.getVariant(), and
container.getContainerVariant().
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/Constants.kt`:
- Line 37: Confirm whether the "experimental" prefix in the constant
DRM_EXTRAS_FILENAME is intentional for production; check the upstream/download
endpoint to verify the canonical filename it serves and, if a non-experimental
variant is required, update the DRM_EXTRAS_FILENAME value in Constants.kt to the
correct production filename and update any dependent code, tests, and
documentation that reference this constant so they use the canonical name.
In `@app/src/main/res/values/strings.xml`:
- Line 760: Update the user-facing string resource main_downloading_steam_token
so the value is user-friendly prose: change "Downloading steam-token" to
"Downloading Steam token" (capitalize "Steam" and replace the hyphen with a
space) to match other platform references and normal sentence style.
- Around line 757-760: The four new in-progress string resources
(main_downloading_first_time_files, main_downloading_wine,
main_downloading_extras, main_downloading_steam_token) are missing the trailing
ellipsis used elsewhere; update each string value to append "…" (or "..." if
using ASCII) so they match the other loading strings (e.g.,
main_downloading_steam, main_installing_glibc, main_installing_bionic,
main_loading) and restore visual consistency in the launch progress UI.
21d33a4 to
ab17c50
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/utils/launchdependencies/ImageFsInstallDependency.kt (1)
23-48: Previous build failure and silent-failure issues are resolved; two minor residual notes.The block body,
suspendCancellableCoroutine<Unit>,cont.invokeOnCancellation, and the explicit check ofresult == false(throwingIllegalStateException) all look correct.Two minor items:
future.get() as Booleanunchecked cast (line 35): At runtime, Java generic erasure means.get()returnsObject. The cast succeeds for a normalFuture<Boolean>but would throwClassCastException(caught by thecatchblock below) if the future ever returns a non-Boolean. A slightly more defensive form avoids the explicit cast:♻️ Optional: safer cast
- val result = future.get() as Boolean + val result = future.get() == true // null/non-Boolean → treated as failure if (cont.isActive) { - if (result) { + if (result) { cont.resume(Unit) } else { cont.resumeWithException(IllegalStateException("ImageFs install completed but returned false")) } }
- Fully-qualified
kotlinx.coroutines.Dispatchers.IO(line 28): The other files in this package import and useDispatchersdirectly. An import would be consistent:♻️ Optional: add import
+import kotlinx.coroutines.Dispatchers ... - withContext(kotlinx.coroutines.Dispatchers.IO) { + withContext(Dispatchers.IO) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/launchdependencies/ImageFsInstallDependency.kt` around lines 23 - 48, The suspend block in ImageFsInstallDependency.install uses an unchecked cast (future.get() as Boolean) and a fully-qualified kotlinx.coroutines.Dispatchers.IO; update the code to defensively check the returned value's type instead of casting (e.g., call val result = future.get(); if (result is Boolean) { if (result) cont.resume(Unit) else cont.resumeWithException(IllegalStateException(...)) } else cont.resumeWithException(IllegalStateException("Unexpected result type: ${result?.javaClass}"))) and keep the existing cancellation handling (cont.invokeOnCancellation). Also add an import for Dispatchers and replace kotlinx.coroutines.Dispatchers.IO with Dispatchers.IO to match the package convention.app/src/main/java/app/gamenative/utils/LaunchDependencies.kt (1)
46-56:finallyblock unconditionally resets loading state even on success.On a fully successful run, the
finallyblock sets the loading message back tomain_loadingand resets progress toLOADING_PROGRESS_UNKNOWNbefore returning to the caller. Since PluviaMain then proceeds directly to game launch, the "Loading" message may flash briefly before the game starts. On failure, the same reset happens before the error dialog is shown.Consider only resetting on failure to avoid the spurious visual reset:
♻️ Proposed refactor
suspend fun ensureLaunchDependencies(...) { val callbacks = LaunchDependencyCallbacks(setLoadingMessage, setLoadingProgress) + var succeeded = false try { for (dep in getLaunchDependencies(container)) { if (!dep.isSatisfied(context, container)) { setLoadingMessage(dep.getLoadingMessage(context, container)) dep.install(context, container, callbacks) } } + succeeded = true } finally { - setLoadingMessage(context.getString(R.string.main_loading)) - setLoadingProgress(LOADING_PROGRESS_UNKNOWN) + if (!succeeded) { + setLoadingMessage(context.getString(R.string.main_loading)) + setLoadingProgress(LOADING_PROGRESS_UNKNOWN) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/LaunchDependencies.kt` around lines 46 - 56, The finally block currently always resets the UI (setLoadingMessage and setLoadingProgress) even on success; change control flow so the reset only happens on failure—e.g., introduce a boolean flag (e.g., success) set to true after the dependency loop in getLaunchDependencies finishes without throwing, then in the finally block only call setLoadingMessage(context.getString(R.string.main_loading)) and setLoadingProgress(LOADING_PROGRESS_UNKNOWN) if success is false (or alternatively move those resets into a catch block); adjust references to dep.isSatisfied, dep.getLoadingMessage, dep.install and callbacks as needed to keep behavior identical on exceptions.
🤖 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/PluviaMain.kt`:
- Around line 1152-1172: The catch block handling ensureLaunchDependencies
currently shows update-specific strings (R.string.main_update_failed_title /
R.string.main_update_failed_message) which misleads users; update the dialog to
use a dependency-specific string (e.g., add
R.string.main_dependency_failed_message and use it as the fallback) or reuse the
existing sync-error title/message instead, and update the setMessageDialogState
call (MessageDialogState, DialogType.SYNC_FAIL) to reference the new/appropriate
string resource so dependency/download errors display a correct message.
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/ProtonPackageDependency.kt`:
- Around line 23-29: The isSatisfied check in
ProtonPackageDependency.isSatisfied currently gates readiness on the archive via
SteamService.isFileInstallable; remove that archive existence check and instead
return true solely when the extracted bin directory exists (use
ImageFs.find(context) to build outFile = File(imageFs.rootDir,
"/opt/${container.wineVersion}") and then check File(outFile, "bin").exists() &&
isDirectory). Leave install() unchanged (it will still use the archive during
extraction) so cleanup of the archive after extraction won’t cause false
negatives.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/Constants.kt`:
- Around line 36-38: The DRM_EXTRAS_FILENAME constant under object LaunchAssets
currently uses an "experimental" prefixed filename; verify the canonical
filename served by the production download endpoint and update
DRM_EXTRAS_FILENAME in LaunchAssets to match that canonical name (remove or
change the "experimental" prefix if the production file differs), and add a
brief comment above DRM_EXTRAS_FILENAME noting the source/endpoint used to
confirm the filename so future reviewers can trace the value.
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/ImageFsPatchesDependency.kt`:
- Around line 14-15: The isSatisfied implementation in ImageFsPatchesDependency
currently returns SteamService.isFileInstallable(context, FILE), which mirrors
the same inverted logic as DrmExtrasDependency: if isFileInstallable(...)
actually indicates the file "needs to be installed" (i.e. not yet present), flip
the boolean so isSatisfied means the dependency is met; change
ImageFsPatchesDependency.isSatisfied to return
!SteamService.isFileInstallable(context, FILE) (or, alternatively, call a
positively named helper such as SteamService.isFileInstalled) so the semantics
align (reference: ImageFsPatchesDependency.isSatisfied,
SteamService.isFileInstallable, FILE).
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/utils/LaunchDependencies.kt`:
- Around line 46-56: The finally block currently always resets the UI
(setLoadingMessage and setLoadingProgress) even on success; change control flow
so the reset only happens on failure—e.g., introduce a boolean flag (e.g.,
success) set to true after the dependency loop in getLaunchDependencies finishes
without throwing, then in the finally block only call
setLoadingMessage(context.getString(R.string.main_loading)) and
setLoadingProgress(LOADING_PROGRESS_UNKNOWN) if success is false (or
alternatively move those resets into a catch block); adjust references to
dep.isSatisfied, dep.getLoadingMessage, dep.install and callbacks as needed to
keep behavior identical on exceptions.
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/ImageFsInstallDependency.kt`:
- Around line 23-48: The suspend block in ImageFsInstallDependency.install uses
an unchecked cast (future.get() as Boolean) and a fully-qualified
kotlinx.coroutines.Dispatchers.IO; update the code to defensively check the
returned value's type instead of casting (e.g., call val result = future.get();
if (result is Boolean) { if (result) cont.resume(Unit) else
cont.resumeWithException(IllegalStateException(...)) } else
cont.resumeWithException(IllegalStateException("Unexpected result type:
${result?.javaClass}"))) and keep the existing cancellation handling
(cont.invokeOnCancellation). Also add an import for Dispatchers and replace
kotlinx.coroutines.Dispatchers.IO with Dispatchers.IO to match the package
convention.
ab17c50 to
abff57a
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/utils/launchdependencies/ProtonPackageDependency.kt (1)
24-29:isSatisfiedstill gates on the archive file — causes unnecessary re-downloads if archive is cleaned up after extraction.If the archive (
proton-9.0-arm64ec.txz) is deleted after successful extraction (e.g., storage cleanup),isFileInstallablereturnsfalse, causingisSatisfiedto returnfalseand triggering a full re-download even though/opt/<wineVersion>/binalready exists. CheckingbinDir.exists() && binDir.isDirectoryalone is sufficient, sinceinstall()already handles the "archive missing but needs extraction" path independently.♻️ Proposed fix
override fun isSatisfied(context: Context, container: Container, gameSource: GameSource, gameId: Int): Boolean { - if (!SteamService.isFileInstallable(context, fileName)) return false val imageFs = ImageFs.find(context) val outFile = File(imageFs.rootDir, "/opt/${container.wineVersion}") val binDir = File(outFile, "bin") return binDir.exists() && binDir.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/utils/launchdependencies/ProtonPackageDependency.kt` around lines 24 - 29, The isSatisfied method currently returns false if SteamService.isFileInstallable(context, fileName) is false, causing re-downloads when the archive is removed after extraction; change isSatisfied (in ProtonPackageDependency) to stop gating on SteamService.isFileInstallable and instead only verify the installation directory: use ImageFs.find(context) to build outFile = File(imageFs.rootDir, "/opt/${container.wineVersion}") and return binDir.exists() && binDir.isDirectory (where binDir = File(outFile, "bin")), removing the call to SteamService.isFileInstallable and the fileName dependency.
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/utils/LaunchDependencies.kt (1)
25-63:LaunchDependenciesis stateless — consider making it anobjectinstead of aclass.The class has no instance state; the dependency list lives in the
companion object, andensureLaunchDependenciesdoesn't usethis. A new instance is allocated on every launch call inPluviaMain. Converting to a top-levelobject(or just making both functions top-level/companion) would avoid the unnecessary allocation and make the singleton nature explicit.♻️ Suggested refactor
-class LaunchDependencies { - companion object { - private val launchDependencies: List<LaunchDependency> = listOf( +object LaunchDependencies { + private val launchDependencies: List<LaunchDependency> = listOf( ImageFsBaseDependency, ... GogScriptInterpreterDependency, ) - } - fun getLaunchDependencies(...) = ... - suspend fun ensureLaunchDependencies(...) { ... } + fun getLaunchDependencies(...) = ... + suspend fun ensureLaunchDependencies(...) { ... } }Then in
PluviaMain.kt:-LaunchDependencies().ensureLaunchDependencies(...) +LaunchDependencies.ensureLaunchDependencies(...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/LaunchDependencies.kt` around lines 25 - 63, The LaunchDependencies class is effectively stateless; replace the class+companion pattern with a singleton object by converting "class LaunchDependencies" into "object LaunchDependencies" and moving the existing companion contents and methods (getLaunchDependencies and ensureLaunchDependencies) into that object, removing the redundant companion; update call sites (e.g. in PluviaMain) to call LaunchDependencies.getLaunchDependencies(...) and LaunchDependencies.ensureLaunchDependencies(...) directly instead of instantiating LaunchDependencies.app/src/main/java/app/gamenative/ui/PluviaMain.kt (1)
1187-1211: Re-throwCancellationExceptionto preserve coroutine cancellation semantics.The
catch (e: Exception)block will interceptCancellationExceptionfrom the suspend functionensureLaunchDependencies(), which breaks proper coroutine cancellation. Since the TODO comment indicates cancellation support will be added, implement the standard Kotlin pattern of re-throwingCancellationException.Suggested fix
} catch (e: Exception) { + if (e is kotlinx.coroutines.CancellationException) throw e Timber.tag("preLaunchApp").e(e, "ensureLaunchDependencies failed") setLoadingDialogVisible(false)🤖 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/PluviaMain.kt` around lines 1187 - 1211, The catch currently swallows CancellationException from the suspend call LaunchDependencies().ensureLaunchDependencies which breaks coroutine cancellation; update the error handling in the surrounding try/catch so that CancellationException is re-thrown (either add an explicit catch(e: CancellationException) { throw e } before the general Exception catch, or check if e is a CancellationException and rethrow it) and keep the existing logging/UI error handling for non-cancellation exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/ProtonPackageDependency.kt`:
- Around line 24-29: The isSatisfied method currently returns false if
SteamService.isFileInstallable(context, fileName) is false, causing re-downloads
when the archive is removed after extraction; change isSatisfied (in
ProtonPackageDependency) to stop gating on SteamService.isFileInstallable and
instead only verify the installation directory: use ImageFs.find(context) to
build outFile = File(imageFs.rootDir, "/opt/${container.wineVersion}") and
return binDir.exists() && binDir.isDirectory (where binDir = File(outFile,
"bin")), removing the call to SteamService.isFileInstallable and the fileName
dependency.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/PluviaMain.kt`:
- Around line 1187-1211: The catch currently swallows CancellationException from
the suspend call LaunchDependencies().ensureLaunchDependencies which breaks
coroutine cancellation; update the error handling in the surrounding try/catch
so that CancellationException is re-thrown (either add an explicit catch(e:
CancellationException) { throw e } before the general Exception catch, or check
if e is a CancellationException and rethrow it) and keep the existing logging/UI
error handling for non-cancellation exceptions.
In `@app/src/main/java/app/gamenative/utils/LaunchDependencies.kt`:
- Around line 25-63: The LaunchDependencies class is effectively stateless;
replace the class+companion pattern with a singleton object by converting "class
LaunchDependencies" into "object LaunchDependencies" and moving the existing
companion contents and methods (getLaunchDependencies and
ensureLaunchDependencies) into that object, removing the redundant companion;
update call sites (e.g. in PluviaMain) to call
LaunchDependencies.getLaunchDependencies(...) and
LaunchDependencies.ensureLaunchDependencies(...) directly instead of
instantiating LaunchDependencies.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/utils/LaunchDependencies.ktapp/src/main/java/app/gamenative/utils/launchdependencies/DrmExtrasDependency.ktapp/src/main/java/app/gamenative/utils/launchdependencies/ImageFsBaseDependency.ktapp/src/main/java/app/gamenative/utils/launchdependencies/ImageFsInstallDependency.ktapp/src/main/java/app/gamenative/utils/launchdependencies/ImageFsPatchesDependency.ktapp/src/main/java/app/gamenative/utils/launchdependencies/ProtonPackageDependency.ktapp/src/main/java/app/gamenative/utils/launchdependencies/SteamClientDependency.ktapp/src/main/java/app/gamenative/utils/launchdependencies/SteamTokenDependency.ktapp/src/main/java/com/winlator/xenvironment/ImageFsInstaller.javaapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (6)
- app/src/main/res/values/strings.xml
- app/src/main/java/app/gamenative/utils/launchdependencies/SteamTokenDependency.kt
- app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java
- app/src/main/java/app/gamenative/utils/launchdependencies/ImageFsInstallDependency.kt
- app/src/main/java/app/gamenative/utils/launchdependencies/ImageFsBaseDependency.kt
- app/src/main/java/app/gamenative/utils/launchdependencies/DrmExtrasDependency.kt
unbelievableflavour
force-pushed
the
streamlined-launch-dependencies-flow
branch
from
abff57a to
8d1be23
Compare
unbelievableflavour
force-pushed
the
streamlined-launch-dependencies-flow
branch
from
8d1be23 to
047fc11
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.
Extracted retrieving launch dependencies + streamlined the flow.
If we have another (doesn't matter which platform) Just add a new launch dependency file and configure it in the
ALL_LAUNCH_DEPENDENCIESlist.Summary by CodeRabbit
Bug Fixes
New Features