Execute Steam InstallScripts during container setup#1464
Open
playday3008 wants to merge 13 commits into
Open
Conversation
Install scripts are identified by the EDepotFileFlag.InstallScript flag on files in depot manifests, not from a depot-level metadata key. Rewrite collectScripts to load depot manifests and scan for flagged files. Remove old collectScripts unit tests that relied on the previous metadata-based API; keep utility method tests. Clean up diagnostic debug logging from XServerScreen.
Contributor
|
Too much diff to scan? Review this PR in Change Stack to start with the highest-impact changes. 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 Steam install-script support: data models, a VDF parser with env-var expansion, an executor that discovers scripts from depots and applies registry changes, generation of run-process commands, integration into a chained pre-launch flow, and tests. ChangesSteam Install-Script Support
Sequence Diagram(s)sequenceDiagram
participant XServerScreen
participant PreLaunchSetup
participant InstallScriptExecutor
participant InstallScriptParser
participant DepotManifest
participant WineRegistry
participant GuestProgram
XServerScreen->>PreLaunchSetup: buildChain(container, appId, gameSource, screenInfo, containerVariantChanged)
PreLaunchSetup->>InstallScriptExecutor: collectScripts(steamApp, appInfo, gameDir, installDir, language, appId)
InstallScriptExecutor->>DepotManifest: load depot manifests
DepotManifest-->>InstallScriptExecutor: install-script entries
InstallScriptExecutor->>InstallScriptParser: parse(scriptFile)
InstallScriptParser-->>InstallScriptExecutor: InstallScript (registryActions, runProcessActions)
PreLaunchSetup->>InstallScriptExecutor: applyRegistryKeys(container, scripts, language)
InstallScriptExecutor->>WineRegistry: write string/DWORD values (system.reg / user.reg)
PreLaunchSetup->>InstallScriptExecutor: getRunProcessCommands(container, scripts, screenInfo, is64Bit)
InstallScriptExecutor-->>PreLaunchSetup: RunProcessCommand list
XServerScreen->>GuestProgram: execute chained commands (pre-install, install-script runs, game)
GuestProgram->>InstallScriptExecutor: markRunProcessComplete(hasRunKey)
InstallScriptExecutor->>WineRegistry: write completion dword
sequenceDiagram
participant GameLaunch as Game Launch
participant PreLaunch as PreLaunchSetup
participant Chain as Command Chain
participant Guest as Guest Program
GameLaunch->>PreLaunch: buildChain (preInstall + installScript commands)
PreLaunch->>Chain: assemble ordered ChainedCommand list
Chain->>Guest: execute step N (guestExecutable)
Guest-->>Chain: termination callback (exit code)
Chain->>Guest: wineserver -k (between steps)
Chain->>Guest: execute final game executable
Guest-->>GameLaunch: game running
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 3256-3266: The termination callback currently marks chained steps
complete unconditionally (advancing the chain even on non-zero exit or
cancellation); update the ChainedCommand termination logic used when building
chainedPreInstall and chainedInstallScript so it only calls
PreInstallSteps.markStepDone(container, cmd.marker) or
InstallScriptExecutor.markRunProcessComplete(container, cmd.hasRunKey) after
verifying the process exited successfully (exit code == 0) and was not
cancelled; ensure any cancellation or non-zero exit prevents advancing the chain
and propagates failure so retries occur on next launch.
In
`@app/src/main/java/app/gamenative/utils/installscript/InstallScriptExecutor.kt`:
- Around line 121-133: The skip-check uses action.hasRunKey before the fallback
key is computed, so actions without an explicit hasRunKey always return false in
hasAlreadyRun and will rerun; fix by computing effectiveHasRunKey before calling
hasAlreadyRun and pass that same effectiveHasRunKey into both the hasAlreadyRun
check and the RunProcessCommand/markRunProcessComplete flow (i.e., compute
effectiveHasRunKey = action.hasRunKey ?:
"Software\\GameNative\\InstallScript\\${script.sourcePath.hashCode()}\\${action.name}"
up front, use it in hasAlreadyRun(effectiveHasRunKey) and in the
RunProcessCommand constructor), and apply the same change to the other similar
block that constructs commands (the second occurrence near the
wrapAsGuestExecutable/RunProcessCommand usage).
- Around line 139-147: markRunProcessComplete is always editing .wine/system.reg
regardless of the hive in hasRunKey; update it (and the corresponding
hasAlreadyRun reader) to select the correct reg file based on the hive prefix
(e.g., if hasRunKey startsWith "HKCU\\" use the per-user reg file instead of
system.reg), then open WineRegistryEditor on that chosen File before calling
setCreateKeyIfNotExist and setDwordValue (use the same hive-parsing logic you
already use with stripHivePrefix/keyPath/valueName so reads and writes target
the same hive).
- Around line 170-176: stripHivePrefix currently only removes uppercase hive
strings and so leaves lowercase prefixes like "hklm\" intact; update
stripHivePrefix to perform case-insensitive prefix removal (e.g., convert
keyPath to a consistent case for matching or use a case-insensitive regex) and
then return the original casing minus the matched prefix so behavior matches
applyRegistryKeys(ignoreCase = true). Target the internal fun
stripHivePrefix(keyPath: String) and ensure it strips any of "HKLM\\",
"HKEY_LOCAL_MACHINE\\", "HKCU\\", "HKEY_CURRENT_USER\\" regardless of case.
In `@app/src/main/java/app/gamenative/utils/installscript/InstallScriptParser.kt`:
- Around line 133-137: The language override keys are stored using the raw VDF
section name (childName) but looked up with language.lowercase() in
mergeWithLanguage(); change the storage to normalize the key (e.g.,
childName.lowercase()) when inserting into the languageOverrides map inside
InstallScriptParser (where RegistryValues is created) so lookups in
mergeWithLanguage() succeed for variants like "English" or "ENGLISH".
- Around line 60-61: The mapping for "%ROOTDRIVE%" in InstallScriptParser is
currently set to "C" which causes expansions like "%ROOTDRIVE%\foo" => "C\foo";
update the mapping for the "ROOTDRIVE" entry (the map or method in
InstallScriptParser that returns default variables) to use "C:" instead of "C"
so path expansions produce valid Windows paths (e.g., "C:\foo"). Ensure any
other places that synthesize ROOTDRIVE defaults use the same "C:" value.
In
`@app/src/test/java/app/gamenative/utils/installscript/InstallScriptExecutorTest.kt`:
- Around line 51-53: The test currently checks for a key named "FrenchValue"
which is actually a value; change the assertion to verify that the value isn't
present by replacing assertFalse(merged.strings.containsKey("FrenchValue")) with
an assertion that checks values (e.g.,
assertFalse(merged.strings.containsValue("FrenchValue")) or
assertNull/doesNotContain on merged.strings.values) so the test checks for
accidental leakage of the French value from the fallback-language logic; update
the assertion in the InstallScriptExecutorTest that uses merged.strings
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ae77802-9720-4b5a-9af1-ed3041ed30d5
📒 Files selected for processing (9)
app/src/main/java/app/gamenative/data/DepotInfo.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/KeyValueUtils.ktapp/src/main/java/app/gamenative/utils/PreInstallSteps.ktapp/src/main/java/app/gamenative/utils/installscript/InstallScriptData.ktapp/src/main/java/app/gamenative/utils/installscript/InstallScriptExecutor.ktapp/src/main/java/app/gamenative/utils/installscript/InstallScriptParser.ktapp/src/test/java/app/gamenative/utils/installscript/InstallScriptExecutorTest.ktapp/src/test/java/app/gamenative/utils/installscript/InstallScriptParserTest.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.
5 issues found across 9 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- Compute effectiveHasRunKey before skip-check so fallback-keyed actions don't re-run on every launch - Resolve registry file from hive prefix (HKCU → user.reg) instead of hardcoding system.reg in hasAlreadyRun/markRunProcessComplete - Make stripHivePrefix case-insensitive to match applyRegistryKeys - Lowercase language override keys when parsing to match mergeWithLanguage lookup
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/java/app/gamenative/utils/installscript/InstallScriptExecutor.kt (2)
139-147:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPersist the same threshold you use for
MinimumHasRunValue.Line 199 skips only when the stored DWORD reaches
minimumHasRunValue, but Line 147 always writes1. Any action withMinimumHasRunValue > 1will rerun on the next launch, and this can even downgrade a higher value written by the installer itself back to1.💡 Proposed direction
data class RunProcessCommand( val executable: String, val hasRunKey: String?, + val completionValue: Int, ) ... + val completionValue = maxOf(1, action.minimumHasRunValue) - commands.add(RunProcessCommand(wrapped, effectiveHasRunKey)) + commands.add(RunProcessCommand(wrapped, effectiveHasRunKey, completionValue)) ... - fun markRunProcessComplete(container: Container, hasRunKey: String) { + fun markRunProcessComplete(container: Container, hasRunKey: String, completionValue: Int) { ... - editor.setDwordValue(stripHivePrefix(keyPath), valueName, 1) + editor.setDwordValue(stripHivePrefix(keyPath), valueName, completionValue)The caller that consumes
RunProcessCommandshould passcompletionValuethrough when it marks the action complete.Also applies to: 189-199
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/app/gamenative/utils/installscript/InstallScriptExecutor.kt` around lines 139 - 147, markRunProcessComplete currently always writes a DWORD value of 1 which mismatches the threshold checked elsewhere (MinimumHasRunValue) and can downgrade higher values; change markRunProcessComplete (and the similar write in the other block referenced) to accept or be passed the same completionValue used by RunProcessCommand (or the MinimumHasRunValue) and write that value instead of 1 via WineRegistryEditor.setDwordValue(stripHivePrefix(keyPath), valueName, completionValue) so the persisted registry value matches the threshold used when deciding to skip reruns.
121-133:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQuote the executable before wrapping it in
cmd /c.Line 127 builds
cmdLinefrom the rawprocessvalue, so expanded paths under%INSTALLDIR%orC:\Program Files...are split on spaces unless the VDF already included quotes. That makes valid install scripts fail to launch.💡 Proposed fix
- val cmdLine = if (action.command.isNotEmpty()) { - "${action.process} ${action.command}" - } else { - action.process - } + val quotedProcess = if ( + action.process.startsWith("\"") && action.process.endsWith("\"") + ) { + action.process + } else { + "\"${action.process}\"" + } + val cmdLine = if (action.command.isNotEmpty()) { + "$quotedProcess ${action.command}" + } else { + quotedProcess + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/app/gamenative/utils/installscript/InstallScriptExecutor.kt` around lines 121 - 133, The cmdLine built in InstallScriptExecutor (variable cmdLine used to create RunProcessCommand and passed into wrapAsGuestExecutable) uses action.process raw which splits paths with spaces when wrapped in cmd /c; change construction to quote the executable path if it isn't already quoted (e.g., produce "\"<process>\" <command>" when action.command is present or just "\"<process>\"" otherwise), preserving existing surrounding quotes, then pass that quoted cmdLine into wrapAsGuestExecutable; this ensures executables under %INSTALLDIR% or "C:\Program Files..." are not split when run.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@app/src/main/java/app/gamenative/utils/installscript/InstallScriptExecutor.kt`:
- Around line 139-147: markRunProcessComplete currently always writes a DWORD
value of 1 which mismatches the threshold checked elsewhere (MinimumHasRunValue)
and can downgrade higher values; change markRunProcessComplete (and the similar
write in the other block referenced) to accept or be passed the same
completionValue used by RunProcessCommand (or the MinimumHasRunValue) and write
that value instead of 1 via
WineRegistryEditor.setDwordValue(stripHivePrefix(keyPath), valueName,
completionValue) so the persisted registry value matches the threshold used when
deciding to skip reruns.
- Around line 121-133: The cmdLine built in InstallScriptExecutor (variable
cmdLine used to create RunProcessCommand and passed into wrapAsGuestExecutable)
uses action.process raw which splits paths with spaces when wrapped in cmd /c;
change construction to quote the executable path if it isn't already quoted
(e.g., produce "\"<process>\" <command>" when action.command is present or just
"\"<process>\"" otherwise), preserving existing surrounding quotes, then pass
that quoted cmdLine into wrapAsGuestExecutable; this ensures executables under
%INSTALLDIR% or "C:\Program Files..." are not split when run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9af89a6a-f160-41d8-b9a6-1ac0878ad0e0
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/utils/installscript/InstallScriptExecutor.ktapp/src/main/java/app/gamenative/utils/installscript/InstallScriptParser.kt
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Aligned with the spec ✅
Potential gaps worth considering
Would you like to open a follow-up issue for the exit-code-based completion tracking or the uninstall cleanup behavior? |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Regarding my earlier questions — would you like me to open follow-up GitHub issues for:
|
Owner
|
Hi @playday3008. Thank you for the PR! Could you give me examples of games this fixes (besides NFS Rivals)? I want to try them out on my end too |
Author
In theory any game that has setup step in installScript.vdf, such as EA and Ubisoft titles, although I saw that we have Ubisoft step separately, this PR may allow to remove that "hacky" step |
Author
|
Also, need to check out: |
…sses Wrapper now captures installer exit code via &&/|| branching to a file before wineserver cleanup. Only marks HasRunKey complete on exit code 0, matching Steam behavior. Non-zero exits log a warning and retry next launch.
| ) | ||
| if (scripts.isNotEmpty()) { | ||
| Timber.tag("InstallScript").i("Applying registry keys from ${scripts.size} install script(s)") | ||
| InstallScriptExecutor.applyRegistryKeys(container, scripts, container.language) |
Owner
There was a problem hiding this comment.
It's valuable to add the registry keys, but the rest I'm not so sure about. We intentionally don't install some things (like xaudio on proton 9, directx on proton < 10). This should align with our preinstallsteps better. What kind of scripts have you seen here? And what errors get fixed?
Author
There was a problem hiding this comment.
From the testing with NFS Rivals, the InstallScript Run Process installs EA App (EAAppInstaller.exe). Ubisoft titles similarly declare Ubisoft Connect installer. These are launcher/client prerequisites, not redists like VCRedist/DirectX/PhysX (those are already handled by PreInstallSteps).
Concern about xaudio on proton 9 / directx on proton <10 is valid but may not apply here — InstallScript Run Processes are typically launcher installers, not the same redists that PreInstallSteps handles. That said, some games could declare DirectX/VCRedist in their InstallScript too, which would overlap.
Comment on lines
+3251
to
+3273
| data class ChainedCommand( | ||
| val executable: String, | ||
| val onComplete: () -> Unit, | ||
| ) | ||
|
|
||
| val chainedPreInstall = preInstallCommands.map { cmd -> | ||
| ChainedCommand(cmd.executable) { PreInstallSteps.markStepDone(container, cmd.marker) } | ||
| } | ||
| val chainedInstallScript = installScriptRunProcessCommands.map { cmd -> | ||
| ChainedCommand(cmd.executable) { | ||
| if (cmd.hasRunKey != null) { | ||
| val exitCode = InstallScriptExecutor.readExitCode(container) | ||
| if (exitCode == 0) { | ||
| InstallScriptExecutor.markRunProcessComplete(container, cmd.hasRunKey) | ||
| } else { | ||
| Timber.tag("InstallScript").w( | ||
| "Run process exited with code $exitCode, will retry next launch", | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| val allChainedCommands = chainedPreInstall + chainedInstallScript |
Owner
There was a problem hiding this comment.
InstallScript should just be a PreInstallStep, this doesn't belong here.
Author
There was a problem hiding this comment.
PreLaunchSetup orchestrator instead of making InstallScript a PreInstallStep — they're different abstractions. PreInstallStep is static 1:1 (marker → command, marker file tracking). InstallScript is dynamic N:M (parsed from VDF, registry-based HasRunKey with MinimumHasRunValue, pre-Wine registry phase). Forcing one into the other means either bloating the interface for all steps or losing Steam spec compliance.
XServerScreen now just calls PreLaunchSetup.buildChain() and gets a unified command list back — no domain logic in the screen anymore.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 3087-3094: PreLaunchSetup.buildChain is being invoked and its
first prerequisite is used to override
guestProgramLauncherComponent.guestExecutable even for desktop/container/test
modes (bootToContainer or testGraphics), causing installers to run instead of
the intended gameExecutable (wfm.exe/TestD3D.exe); change the logic so that when
bootToContainer or testGraphics is true you either skip calling
PreLaunchSetup.buildChain entirely or at minimum do not replace
guestProgramLauncherComponent.guestExecutable with
allChainedCommands.firstOrNull()?.executable — keep
guestProgramLauncherComponent.guestExecutable set to gameExecutable in those
modes and only apply the chained prerequisites for normal launches (use the
existing flags bootToContainer/testGraphics and the variables
allChainedCommands, PreLaunchSetup.buildChain, and
guestProgramLauncherComponent.guestExecutable to gate this behavior).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 819b7ea7-29e2-4713-a3b5-fd3fe8ceccf2
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/PreLaunchSetup.ktapp/src/test/java/app/gamenative/utils/PreLaunchSetupTest.kt
Comment on lines
+3087
to
+3094
| allChainedCommands = PreLaunchSetup.buildChain( | ||
| container = container, | ||
| appId = appId, | ||
| gameSource = gameSource, | ||
| screenInfo = xServer.screenInfo.toString(), | ||
| containerVariantChanged = containerVariantChanged, | ||
| ) | ||
| guestProgramLauncherComponent.guestExecutable = | ||
| preInstallCommands.firstOrNull()?.executable ?: gameExecutable | ||
| guestProgramLauncherComponent.guestExecutable = allChainedCommands.firstOrNull()?.executable ?: gameExecutable |
Contributor
There was a problem hiding this comment.
Skip app-specific prelaunch chaining for desktop/test launches.
Line 3087 builds PreLaunchSetup commands even when bootToContainer or testGraphics is active. In those modes gameExecutable is intentionally wfm.exe / TestD3D.exe, but Line 3094 still replaces it with the first prerequisite command, so opening a container or running the graphics test can unexpectedly execute game installers instead of the requested target.
💡 Suggested fix
- allChainedCommands = PreLaunchSetup.buildChain(
- container = container,
- appId = appId,
- gameSource = gameSource,
- screenInfo = xServer.screenInfo.toString(),
- containerVariantChanged = containerVariantChanged,
- )
+ val shouldRunPreLaunchChain = !bootToContainer && !testGraphics
+ allChainedCommands = if (shouldRunPreLaunchChain) {
+ PreLaunchSetup.buildChain(
+ container = container,
+ appId = appId,
+ gameSource = gameSource,
+ screenInfo = xServer.screenInfo.toString(),
+ containerVariantChanged = containerVariantChanged,
+ )
+ } else {
+ emptyList()
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` around
lines 3087 - 3094, PreLaunchSetup.buildChain is being invoked and its first
prerequisite is used to override guestProgramLauncherComponent.guestExecutable
even for desktop/container/test modes (bootToContainer or testGraphics), causing
installers to run instead of the intended gameExecutable (wfm.exe/TestD3D.exe);
change the logic so that when bootToContainer or testGraphics is true you either
skip calling PreLaunchSetup.buildChain entirely or at minimum do not replace
guestProgramLauncherComponent.guestExecutable with
allChainedCommands.firstOrNull()?.executable — keep
guestProgramLauncherComponent.guestExecutable set to gameExecutable in those
modes and only apply the chained prerequisites for normal launches (use the
existing flags bootToContainer/testGraphics and the variables
allChainedCommands, PreLaunchSetup.buildChain, and
guestProgramLauncherComponent.guestExecutable to gate this behavior).
Author
There was a problem hiding this comment.
Pre-existing behavior — the old code (preInstallCommands = PreInstallSteps.getPreInstallCommands(...) + firstChainedExecutable) had the same issue. Not a regression from our refactor.
Contributor
There was a problem hiding this comment.
@playday3008, acknowledged — you're right that the old PreInstallSteps.getPreInstallCommands(...) call had the same absent guard, so this PR is a clean refactor of existing behavior rather than a regression. I'll retract the severity of this comment.
That said, the latent issue (prerequisite installers potentially running during bootToContainer/testGraphics modes) is still present. Would you like me to open a follow-up GitHub issue to track adding the guard in a separate PR?
🧠 Learnings used
Learnt from: unbelievableflavour
Repo: utkarshdalal/GameNative PR: 698
File: app/src/main/java/app/gamenative/utils/LaunchSteps.kt:134-142
Timestamp: 2026-03-03T11:45:46.529Z
Learning: In the LaunchSteps orchestration (app/src/main/java/app/gamenative/utils/LaunchSteps.kt), the `wineserver -k` command in the runLauncher function is necessary to prevent the launcher from closing after each chained step. Without it, the launcher terminates prematurely between steps.
Learnt from: Catpotatos
Repo: utkarshdalal/GameNative PR: 1245
File: app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:1550-1550
Timestamp: 2026-04-19T11:44:04.419Z
Learning: In `app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`, `firstTimeBoot` is set to `true` only when `container.getExtra("containerPatchVersion").isEmpty() || containerVariantChanged`. This is intentional: (1) existing containers migrating from `appVersion`-based tracking will have an empty `containerPatchVersion`, triggering one final full rebuild; (2) brand-new containers always have an empty extra; (3) for patch-version mismatches on existing containers, `setupWineSystemFiles()` calls `applyGeneralPatches()` which rebuilds the prefix from scratch and clears all tracking extras — `firstTimeBoot = false` in this path is safe because fresh original DLLs are already present after `applyGeneralPatches()`. Do not suggest expanding `firstTimeBoot` to cover `containerPatchVersion` mismatches in future reviews.
Learnt from: joshuatam
Repo: utkarshdalal/GameNative PR: 1269
File: app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt:205-216
Timestamp: 2026-04-21T18:33:03.732Z
Learning: In `app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt`, the game (app) install directory is always mapped to the A: drive in the Wine container. Hardcoding `"A"` in `toWindowsPathForWine("A", appDir, cab)` inside `buildCabarcBatchScript` is correct and intentional — do not flag the hard-coded "A" drive letter for the `appDir`/cab path as a potential mapping issue in future reviews.
Learnt from: AndreVto
Repo: utkarshdalal/GameNative PR: 1389
File: app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:1580-1585
Timestamp: 2026-05-07T14:24:30.099Z
Learning: In the GameNative repository (utkarshdalal/GameNative), `PluviaApp.xEnvironment` is always destroyed (via `shutdownEnvironment`) when a container/game session closes. Therefore `existingXServer` (obtained via `PluviaApp.xEnvironment?.getComponent<XServerComponent>()?.xServer`) can only be non-null during the same active session (e.g., on activity recreation), never across separate game/container launches with potentially different variants.
Learnt from: Catpotatos
Repo: utkarshdalal/GameNative PR: 1245
File: app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:188-191
Timestamp: 2026-04-21T20:12:43.546Z
Learning: In `app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`, `ALWAYS_REEXTRACT = true` is intentionally kept for stability. The key improvement in PR `#1245` is that for x86_64 containers, DXVK/VKD3D no longer re-extracts on every boot — `needReextract` in `setupWineSystemFiles()` only triggers unconditional re-extract via `ALWAYS_REEXTRACT` for arm64ec containers (`isArm64EcContainer`). Graphics drivers and the Wrapper/Fex/cnc-ddraw path still use `ALWAYS_REEXTRACT` unconditionally for all containers. Do not flag `ALWAYS_REEXTRACT = true` as incorrect or suggest setting it to `false` in future reviews of this file.
Learnt from: Catpotatos
Repo: utkarshdalal/GameNative PR: 1245
File: app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.kt:629-630
Timestamp: 2026-04-18T19:13:41.015Z
Learning: In `app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.kt`, the `repairContainerFiles()` function intentionally clears `imgVersion` and `containerPatchVersion` extras (in addition to component extras and pre-install markers). Clearing these two extras triggers `XServerScreen.setupWineSystemFiles()` → `applyGeneralPatches()` on next launch, which performs a full prefix wipe and re-extraction. This is the intended behavior: "Repair container" is a deliberate, user-initiated full-repair recovery action equivalent to what `ALWAYS_REEXTRACT = true` did unconditionally, but now exposed as a targeted one-time option. Do not flag the clearing of `imgVersion`/`containerPatchVersion` in `repairContainerFiles()` as incorrectly routing through the prefix rebuild path in future reviews.
Learnt from: joshuatam
Repo: utkarshdalal/GameNative PR: 1269
File: app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt:144-145
Timestamp: 2026-04-21T18:32:25.546Z
Learning: In `app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt`, passing an Android absolute path (Linux path) directly to `wine cmd /c <path>` works correctly. Wine internally translates Unix/Linux absolute paths to the corresponding Windows drive paths, so `wine cmd /c ${batFile.absolutePath}` is valid. Do not flag usage of Android absolute paths in `wine cmd /c` invocations as incorrect in future reviews of this file.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 1136
File: app/src/main/java/app/gamenative/MainActivity.kt:273-284
Timestamp: 2026-04-07T21:36:14.886Z
Learning: In the GameNative repository (`utkarshdalal/GameNative`), `SteamService.keepAlive` is set to `true` in only one place: `ContainerConfigDialog.kt:1032` (inside a folder-picker flow). During the normal XServerScreen game-launch bootstrap (`XServerScreen.kt` lines ~1529-1680), `keepAlive` is `false` throughout. The bootstrap uses a raw Java `Executors.newSingleThreadExecutor().submit {}` (not a coroutine/LaunchedEffect), so Compose teardown does not cancel it. However, since `keepAlive` is `false` during bootstrap, a task-swipe in that window does not produce a stale `keepAlive=true` on the next launch, and therefore does not cause the "stuck on container screen" bug. Do not flag onDestroy's `if (SteamService.keepAlive)` guard as failing to cover the bootstrap-window race in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 1136
File: app/src/main/java/app/gamenative/MainActivity.kt:147-150
Timestamp: 2026-04-07T21:37:29.365Z
Learning: In `app/src/main/java/app/gamenative/MainActivity.kt`, the `onCreate` stale-session check `if (SteamService.keepAlive && PluviaApp.xEnvironment == null)` is intentionally kept as-is. The only non-game flow that sets `keepAlive = true` with `xEnvironment == null` is the folder-picker in `ContainerConfigDialog.kt` (line ~1032); if the app is killed in that state, resetting `keepAlive = false` on the next `onCreate` is harmless. Do not flag this guard as incorrectly matching non-XServer flows in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 0
File: :0-0
Timestamp: 2026-03-10T21:56:45.033Z
Learning: In `app/src/main/java/app/gamenative/utils/BestConfigService.kt`, the `available*` lists (availableDxvk, availableVkd3d, availableBox64Bionic, availableBox64Glibc, availableWowBox64, availableFexcore, availableWineBionic, availableWineGlibc, availableDrivers) passed to `validateComponentVersions()` intentionally include manifest entries (base + installed + manifest). This allows configs referencing installable-but-not-yet-on-disk versions to pass validation. A separate subsequent step, `resolveMissingManifestInstallRequests()`, handles actually installing any manifest items not yet present on disk. Do not suggest removing manifest entries from these lists in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 801
File: app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt:174-188
Timestamp: 2026-03-10T22:12:03.164Z
Learning: In `app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt`, the `applyConfigForContainer` function calls `installMissingComponentsForConfig` before `BestConfigService.parseConfigToContainerData`. This install-before-parse ordering is pre-existing behavior, not introduced by any recent PR. Do not flag this ordering as an issue in future reviews of this file.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 0
File: :0-0
Timestamp: 2026-03-10T22:13:54.453Z
Learning: In `app/src/main/java/app/gamenative/utils/BestConfigService.kt`, around lines 403-418, missing Box64/FEXCore preset failures (via `Box86_64PresetManager.getPreset("box64", ...)` and `FEXCorePresetManager.getPreset(...)`) are intentionally appended to the same `missingComponents` list as installable components, even though `resolveMissingManifestInstallRequests()` cannot install presets. This is a known limitation deferred for future work — the long-term direction is to serialize non-default presets into config files and auto-install them. Do not flag this mixing of installable vs. non-installable blockers in `missingComponents` as an issue in future reviews of this file.
Learnt from: Catpotatos
Repo: utkarshdalal/GameNative PR: 1245
File: app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java:375-388
Timestamp: 2026-04-21T20:15:41.555Z
Learning: In `app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java`, in `extractBox64Files()`, the `ContentProfile` check and `contentsManager.applyContent(profile)` call are intentionally placed before (and not gated by) the `box64BionicVersion` sentinel check. Profile-based Box64 installs re-apply on every non-Arm64EC launch by design — this is consistent with the unconditional `if (true)` profile application in `extractEmulatorsDlls()`. The `box64BionicVersion` sentinel only short-circuits the heavier direct ZSTD asset extraction path. Do not suggest hoisting the sentinel check above the profile application in `extractBox64Files()` in future reviews.
Learnt from: playday3008
Repo: utkarshdalal/GameNative PR: 1459
File: app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt:262-266
Timestamp: 2026-05-20T09:45:55.704Z
Learning: In `GeneralTab.kt` (`app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt`) of the GameNative repository, the `onLaunchOptionSelected` handler in `ExecutablePathDropdown` intentionally keeps the existing `config.execArgs` when `args` is `null` (i.e., when a scanned executable with no PICS arguments is selected). Do not flag this as a bug — it is deliberate so that manually entered arguments are not cleared when the user switches to a plain `.exe`/`.bat`/`.cmd` from the file scanner.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 1146
File: app/src/main/java/app/gamenative/utils/ContainerUtils.kt:474-482
Timestamp: 2026-04-08T12:06:12.947Z
Learning: In `app/src/main/java/app/gamenative/utils/ContainerUtils.kt`, within `applyToContainer(...)`, Steam-specific side-effect calls (e.g. `SteamService.getAppDirPath`, `MarkerUtils.removeMarker`, `SteamService.clearCloudSyncCache`, `PrefManager.pendingCloudResync`) are intentionally NOT guarded by an explicit `GameSource.STEAM` check. This is consistent with the existing pattern in the function. For non-Steam containers these calls are no-ops (DAO deletes find no rows; resync entries for non-Steam IDs are never consumed). Do not flag the absence of a Steam-source guard in this function as an issue in future reviews.
Learnt from: xXJSONDeruloXx
Repo: utkarshdalal/GameNative PR: 1195
File: app/src/main/java/app/gamenative/ui/PluviaMain.kt:1100-1102
Timestamp: 2026-04-13T05:42:50.742Z
Learning: In `app/src/main/java/app/gamenative/ui/PluviaMain.kt`, the `BackHandler(enabled = state.loadingDialogVisible && !SteamService.keepAlive)` with an empty body is intentionally a temporary mitigation. On master, pressing Back during the loading/prelaunch phase already leads to a broken state (animation persists, Wine procs stick, force-quit required), so swallowing Back is no worse and prevents UI thrash from impatient back-mashing. Proper cancellable prelaunch (job cancellation + state reset + timeout fallback) is deferred to a separate PR. Do not flag the empty BackHandler body as a dead-end in future reviews of this file.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 0
File: :0-0
Timestamp: 2026-03-18T16:30:26.610Z
Learning: In `app/src/main/java/app/gamenative/ui/PluviaMain.kt`, the three sites that call `MainActivity.setPendingLaunchRequest`, toggle `shownPendingLaunchSnackbar`, and conditionally call `SnackbarManager.show` (cold-start pending branch, ExternalGameLaunch pending branch, and non-terminal SteamDisconnected branch) are intentionally kept separate — the surrounding logic differs enough that extracting a shared helper would add more complexity than it removes. Do not suggest refactoring these into a helper in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 581
File: app/src/main/java/app/gamenative/ui/PluviaMain.kt:1154-1176
Timestamp: 2026-02-21T22:26:41.871Z
Learning: In Kotlin coroutines, suspend functions automatically suspend and resume within a coroutine scope. Only Deferred objects produced by async { ... } require .await() to obtain the result. Do not call .await() on direct suspend function calls or on non-Deferred results; use await() only when you have a Deferred that represents a value you need.
Learnt from: phobos665
Repo: utkarshdalal/GameNative PR: 1072
File: app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt:1309-1315
Timestamp: 2026-04-02T07:43:14.973Z
Learning: In the GameNative repository, when logging/telemetry includes Wine prefix/container paths that contain the hardcoded Wine username `xuser`, do not treat that as a PII/security issue (it is not a real user identity and contains no personally identifiable information). Only flag PII concerns for other user-identifying data not limited to the `xuser` hardcoded Wine username in prefix paths.
Learnt from: xXJSONDeruloXx
Repo: utkarshdalal/GameNative PR: 1332
File: app/src/main/java/com/winlator/widget/XServerView.java:59-64
Timestamp: 2026-04-29T17:49:49.925Z
Learning: When reviewing this repository and tracing Java method call sites (e.g., looking up usages of a Java API), also include Kotlin sources in the search results (do not omit Kotlin files or run searches with a Java-only filter). Kotlin “UI/orchestration” logic often lives in .kt files and may contain the relevant call paths.
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.
Description
Executes Steam InstallScript VDF files during container setup. Discovers install scripts from depot manifests via
EDepotFileFlag.InstallScript, parses them with JavaSteam'sKeyValue, then:Supports env var expansion, language-specific registry overrides, OS filtering, and
HasRunKeytracking.Tested with NFS Rivals — EA App installer ran successfully via chained Wine process.
Recording
Screen_Recording_20260521_043724_GameNative_1.mp4
Type of Change
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by cubic
Run Steam InstallScript VDFs during container setup to auto-install prerequisites before launch. Adds a pre-launch orchestrator that chains installers with exit-code tracking so they only run once.
New Features
EDepotFileFlag.InstallScriptand parse VDFs withKeyValue(env vars, language overrides, OS filters).system.reg/user.regbefore Wine starts.XServerScreennow uses it.Bug Fixes
HasRunKeybefore skip-check; resolve registry hive fromHKCU/HKLMfor reads/writes and completion.UbisoftConnectStepfor Steam games (handled by InstallScripts).Written for commit 8b11e4a. Summary will update on new commits.
Summary by CodeRabbit
New Features
Refactor
Improvements
Tests