Fix and Enhance Real Steam Client Functionality With Focus on Reducing Black Screens#1347
Open
TideGear wants to merge 36 commits into
Open
Fix and Enhance Real Steam Client Functionality With Focus on Reducing Black Screens#1347TideGear wants to merge 36 commits into
TideGear wants to merge 36 commits into
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 per-container Steam settings and an SDK‑cloud bridge (Ludusavi recommendations + mirroring), introduces a real‑Steam launch mode that alters cloud/launch flows, hardens Steam DLL/ACF handling and AutoCloud remote‑cache behavior, updates UI/launch prompts, and applies multiple robustness fixes across file, process, and container handling. ChangesReal‑Steam Mode, SDK‑Cloud Bridge & Cloud Sync
Infrastructure & Robustness
Sequence Diagram(s)sequenceDiagram
participant User
participant PluviaMain
participant Ludusavi
participant SteamUtils
participant SteamService
participant SteamAutoCloud
User->>PluviaMain: preLaunchApp(real‑Steam)
PluviaMain->>SteamUtils: shouldSuggestSdkCloudBridge(appId)
SteamUtils->>Ludusavi: lookup(appId)
Ludusavi-->>SteamUtils: recommendation?
SteamUtils-->>PluviaMain: recommendation
alt recommended
PluviaMain->>User: show SDK_CLOUD_BRIDGE_SUGGESTION
User->>PluviaMain: confirm/dismiss
PluviaMain->>Container: save sdkCloudSaveSubdir
end
PluviaMain->>SteamService: beginLaunchApp(isLaunchRealSteam=true)
SteamService->>SteamAutoCloud: sync & detect auto‑cloud
SteamAutoCloud->>SteamAutoCloud: rebase SteamUserData downloads (glob)
SteamAutoCloud->>SteamUtils: write remotecache.vdf
SteamService->>SteamUtils: mirrorSdkCloudRemoteToSave(appId)
SteamUtils-->>SteamService: mirror complete
SteamService-->>PluviaMain: launch complete
sequenceDiagram
participant User
participant XServerScreen
participant Wine
participant SteamService
User->>XServerScreen: EXIT_GAME (first tap)
XServerScreen->>Wine: steam.exe -shutdown
XServerScreen->>XServerScreen: poll for steam.exe termination
alt steam exits
XServerScreen->>SteamService: closeApp(isLaunchRealSteam=true)
SteamService->>XServerScreen: exit sync done
XServerScreen->>User: exit
else still running
XServerScreen->>User: show "still shutting down" dialog
alt user waits
XServerScreen->>Wine: continue polling
else user force quits
XServerScreen->>Wine: kill steam.exe
XServerScreen->>User: exit
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 20
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/FileUtils.kt (1)
136-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
matchesGlobis too permissive for exact and wildcarded filenames.This implementation only checks that each
*-separated token appears in order, so patterns likeDXSETUP.exe,*.sav, andCapture*.savcan match unintended filenames such asfooDXSETUP.exe.bakorCapture1.sav.tmp. That means bothfindFiles(...)andfindFilesRecursive(...)can return the wrong files.♻️ Proposed fix
fun matchesGlob(fileName: String, pattern: String): Boolean { if (pattern.isEmpty() || pattern == "*") return true - val patternParts = pattern.split("*").filter { it.isNotEmpty() } - var startIndex = 0 - for (part in patternParts) { - val index = fileName.indexOf(part, startIndex, ignoreCase = true) - if (index < 0) return false - startIndex = index + part.length - } - return true + val regex = buildString { + append('^') + pattern.split("*").forEachIndexed { idx, part -> + if (idx > 0) append(".*") + append(Regex.escape(part)) + } + append('$') + } + return Regex(regex, RegexOption.IGNORE_CASE).matches(fileName) }🤖 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/FileUtils.kt` around lines 136 - 176, matchesGlob is too permissive because it only ensures tokens appear in order; update matchesGlob to enforce anchors: if pattern has no leading '*', require the first token to match at fileName start; if pattern has no trailing '*', require the last token to align with fileName end; keep the existing ordered-internal-token checks (with ignoreCase) and preserve the special-case for empty or "*" and exact-match when pattern contains no '*'. Modify the matchesGlob function (used by findFiles and findFilesRecursive) to: parse tokens via split("*"), detect hasLeadingStar and hasTrailingStar, if no tokens return true, check prefix match when !hasLeadingStar by verifying fileName.startsWith(firstToken, ignoreCase=true), check suffix match when !hasTrailingStar by verifying fileName.endsWith(lastToken, ignoreCase=true), and still scan intermediate tokens in order updating startIndex after each match; this prevents matching names like "fooDXSETUP.exe.bak" for "DXSETUP.exe" and similar false positives.app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
5110-5120:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSteam extraction still writes via shared ImageFs root
Line 5118 still extracts
steam.tzstintoimageFs.getRootDir(). That path is tied to shared/global layout, while adjacent changes explicitly avoid global symlink-root writes for container isolation. This mismatch can still allow cross-container contamination under concurrent container activation.Please route Steam archive extraction through a container-scoped destination/path mapping so both existence check and write target are consistently per-container.
🤖 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/xserver/XServerScreen.kt` around lines 5110 - 5120, The extraction currently writes steam.tzst into the shared imageFs.getRootDir(), risking cross-container writes; change the extraction destination to a container-scoped path (same scope used in the existence check) by computing the target root off container.rootDir (or a container-specific subdir) and pass that path to TarCompressorUtils.extract; ensure the File downloaded variable and the TarCompressorUtils.extract call both use the container-scoped destination instead of imageFs.getRootDir() so existence check and write target match.
🧹 Nitpick comments (1)
app/src/main/java/com/winlator/container/Container.java (1)
342-344: ⚡ Quick winValidate
sdkCloudSaveSubdiragainst path separators/traversal.Given this value is used as a relative subdir contract, normalizing to a single safe path component in the setter will prevent invalid values from persisted config edits.
Suggested hardening
public void setSdkCloudSaveSubdir(String sdkCloudSaveSubdir) { - this.sdkCloudSaveSubdir = sdkCloudSaveSubdir == null ? "" : sdkCloudSaveSubdir; + String value = sdkCloudSaveSubdir == null ? "" : sdkCloudSaveSubdir.trim(); + if (value.contains("/") || value.contains("\\") || value.contains("..")) { + value = ""; + } + this.sdkCloudSaveSubdir = value; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/container/Container.java` around lines 342 - 344, The setter Container.setSdkCloudSaveSubdir currently accepts any string; instead validate and normalize sdkCloudSaveSubdir to a single safe path component: trim input, return empty string for null/empty, reject or sanitize any values containing path separators or traversal tokens (e.g., '/', '\', or ".."), and enforce a whitelist pattern (e.g., allow only letters, digits, hyphen, underscore); set this.sdkCloudSaveSubdir to the sanitized value and if invalid either set to empty string or throw an IllegalArgumentException depending on project policy—update Container.setSdkCloudSaveSubdir and document the constraint near the sdkCloudSaveSubdir field.
🤖 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/SteamAutoCloud.kt`:
- Around line 249-256: The rebase lambda (rebaseToAutoCloud) moves downloaded
files under an Auto-Cloud root but downstream hash validation still looks up
entries by the original cloud prefix from AppFileChangeList, so
hasHashConflicts(...) can miss mismatches; fix by resolving the
absolute/normalized path with getFullFilePath(...) for both the local downloaded
file and the corresponding AppFileChangeList entry before any hash lookup or
comparison (i.e., compute resolvedPath = getFullFilePath(prefix, filename) or
otherwise map the AppFileChangeList key to the rebase result and use that
resolved path when calling hasHashConflicts/getHash/etc.), and apply the
identical change to the duplicate logic later in the file (the block analogous
to lines 284-293) so both sides of the hash check use the same resolved path
key.
- Around line 221-247: The writeRemoteCache lambda currently skips writing when
entries.isEmpty(), leaving stale remotecache.vdf; update the lambda
(writeRemoteCache) to handle the zero-entry case by either deleting the existing
remotecache.vdf in the Steam user data directory (constructed from
steamUserDataBase and appInfo.id) or by invoking SteamUtils.writeRemoteCacheVdf
with an explicit empty files list/appropriate changeNumber to overwrite the
file; ensure the removal/overwrite happens only for SteamUserData-rooted entries
and keep the existing exception handling (the SteamUtils.writeRemoteCacheVdf
call and steamUserDataBase/appInfo.id symbols are the targets to modify).
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Line 2278: The call using the non-null assertion
SteamUtils.migrateGSESavesToSteamUserdata(instance?.applicationContext!!, appId,
...) can throw before the try/finally that calls releaseSync(appId), risking a
stuck sync flag; in both beginLaunchApp(...) and forceSyncUserFiles(...) remove
the !! and either (a) obtain and null-check applicationContext before acquiring
the sync (e.g., instance?.applicationContext?.let { ctx -> ... } and return/log
if null), or (b) move the migrateGSESavesToSteamUserdata call inside the try
block after acquireSync so that any exception still goes through the finally
that calls releaseSync(appId); ensure all early-return/error paths always call
releaseSync(appId) (or acquire only after validating context) and replace the !!
with safe null handling (let/if-null-return) to avoid leaking locks.
- Around line 2421-2425: The localhost filter is applied under the wrong
condition: change the pendingRemoteOperations assignment so rawPending.filterNot
{ it.machineName.equals("localhost", ignoreCase = true) } runs when
emulation/launch intent RPC is used (i.e., when isLaunchRealSteam is false) and
leave pendingRemoteOperations as rawPending when isLaunchRealSteam is true;
update the conditional around pendingRemoteOperations (the isLaunchRealSteam
check) to invert the boolean or swap branches accordingly.
- Around line 2314-2333: The code currently dismisses all pending ops whenever
probed.isNotEmpty(), which can remove legitimate cross-device conflicts; update
this to only auto-dismiss operations that are phantoms/local to this host:
compute a filtered list (e.g., val phantomOps = probed.filter { it.machineName
== ourMachineName || it.clientId == proactiveClientId || it.operation.isPhantom
/*or other local/phantom predicate*/ } ), and only call
steamCloud.signalAppLaunchIntent(..., ignorePendingOperations = true) and
steamCloud.signalAppExitSyncDone(...) when phantomOps.isNotEmpty(); if probed
contains non-phantom ops (probed.any { !isPhantomPredicate(it) }), do not set
ignorePendingOperations=true (or skip auto-dismiss) so real cross-device
conflicts are preserved; update references: probed, ourMachineName,
proactiveClientId, steamCloud.signalAppLaunchIntent and
steamCloud.signalAppExitSyncDone accordingly.
In `@app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt`:
- Around line 497-520: The async recommend/detect flow uses the snapshot
variable current (captured at click time) to decide whether to prompt/apply;
after the suspend call to SteamUtils.getRecommendedSdkCloudSaveSubdirAsync in
the TextButton handler (and the similar handler around lines 533-549), re-read
the latest value from state.config.value.sdkCloudSaveSubdir into a local
variable and use that up-to-date value instead of the earlier current before
setting pendingValue/showConfirmDialog or applying the subdir; update both
handlers (the coroutine launched via scope.launch that sets
recommendLoading/detectMessage and checks current) to fetch the fresh
state.config.value.sdkCloudSaveSubdir after the suspend and base the
prompt/apply decision on that.
In `@app/src/main/java/app/gamenative/ui/PluviaMain.kt`:
- Around line 793-845: The relaunch lambda discards original launch options
(temporary container overrides and bootToContainer) by calling preLaunchApp with
only appId and isOffline, causing a different target to be launched after the
dialog; update the code so relaunch captures and forwards the original launch
parameters used for the interrupted launch (e.g., the full LaunchOptions/intent
parameters or the same arguments that started the first preLaunchApp), not just
state.launchedAppId and viewModel.isOffline.value — specifically change relaunch
to accept or close over the original options and call preLaunchApp with those
same values (preserving skipBridgePrompt, bootToContainer/temporary container
override, setLoadingDialogVisible/Progress/Message, setMessageDialogState,
onSuccess = viewModel::launchApp, etc.) so the second attempt uses identical
launch data as the initial attempt.
In `@app/src/main/java/app/gamenative/utils/ContainerUtils.kt`:
- Line 103: createNewContainer() currently constructs a ContainerData manually
and omits the new default fields (e.g., disableSteamOverlay), causing fresh
containers to miss those defaults; update createNewContainer() to populate
disableSteamOverlay from PrefManager.disableSteamOverlay (or better, call the
shared default->ContainerData conversion routine used elsewhere) so the
fresh-container path uses the same defaults as the rest of the conversion path
and includes the new overlay/cloud-save defaults.
In `@app/src/main/java/app/gamenative/utils/SteamTokenLogin.kt`:
- Around line 67-68: The timeout logging and exception in SteamTokenLogin
currently print the full variable command (which may include raw refresh
tokens); change both the Timber.tag("SteamTokenLogin").e(...) call and the
IllegalStateException(...) to avoid leaking the command: log a static operation
label or only the executable name (e.g., "wine exec" or derive basename from
command) and include WINE_EXEC_TIMEOUT_SECONDS but not the full command string,
and throw the IllegalStateException with a redacted message (e.g., "wine exec
timed out" or include "[REDACTED_COMMAND]") while preserving the original caught
exception e as the cause so stack traces remain available; update references to
command, Timber.tag, WINE_EXEC_TIMEOUT_SECONDS, and the IllegalStateException in
the SteamTokenLogin code accordingly.
- Around line 60-67: The timeout logic in SteamTokenLogin (where
CompletableFuture.supplyAsync is used and future.cancel(true) is called) does
not stop the launched native process because
GlibcProgramLauncherComponent.execShellCommand and
BionicProgramLauncherComponent.execShellCommand block on process.waitFor() and
stream reads; update those execShellCommand implementations to be
interrupt-aware and to forcibly destroy the child process on cancellation (or
refactor SteamTokenLogin to use the project’s timeout-aware pattern such as
withTimeout): ensure execShellCommand tracks the Process returned by
Runtime.getRuntime().exec(), closes streams, checks
Thread.currentThread().isInterrupted() during I/O, kills/destroys the Process if
interrupted or timeout elapses (and propagates an exception), and make
SteamTokenLogin use WINE_EXEC_TIMEOUT_SECONDS with that timeout-aware call
instead of relying on future.cancel(true).
In `@app/src/main/java/app/gamenative/utils/SteamUtils.kt`:
- Around line 2569-2583: The current mirroring loop in SteamUtils.kt iterates
remoteDir.listFiles() and skips non-files, which drops nested directories (see
the remoteDir -> gameSaveDir copy loop using Files.copy and Timber logging and
the similar block around the mirror back at 2593-2610). Replace the flat loop
with a recursive copy (e.g., implement a helper like copyRecursively(srcRoot:
File, dstRoot: File) or use Files.walkFileTree) that visits directories and
files, creates destination directories as needed, and calls Files.copy for files
with StandardCopyOption.REPLACE_EXISTING and COPY_ATTRIBUTES, preserving
relative paths; update Timber logs to report each copied file path and keep the
existing catch/Timber.w behavior for exceptions.
- Around line 249-264: The verifyRestoredState function can incorrectly return
true when neither steam_api.dll nor steam_api64.dll are present; modify
verifyRestoredState to track whether any target Steam API DLLs were encountered
(e.g., a boolean foundTargetDll), set it to true inside the loop when n ==
"steam_api.dll" or "steam_api64.dll" is processed, and after the walkTopDown
loop return false if foundTargetDll is false (indicating missing DLLs despite
the RESTORED marker) before returning true; reference the existing function name
verifyRestoredState, the loop over Paths.get(appDirPath).toFile().walkTopDown(),
and the filename checks for "steam_api.dll"/"steam_api64.dll" when adding the
flag and final check.
- Around line 1330-1334: The createSteamCommonLink function currently uses
link.exists() which follows symlinks and misses broken or incorrect links;
change the logic to check for existence without following links using
Files.exists(link.toPath(), LinkOption.NOFOLLOW_LINKS), and if that returns true
use Files.readSymbolicLink(link.toPath()) to compare the existing symlink target
to target.toPath(); if they match, return, otherwise remove the existing link
entry with deleteTreeNoFollowSymlinks(link) before calling
Files.createSymbolicLink(link.toPath(), target.toPath()); ensure parent dirs are
created as before and handle any exceptions from readSymbolicLink/delete
appropriately.
In `@app/src/main/java/com/winlator/contents/AdrenotoolsManager.java`:
- Around line 136-145: Check the results of FileUtils.delete(dst) and
dst.mkdirs() in AdrenotoolsManager before calling TarCompressorUtils.extract: if
FileUtils.delete(dst) returns false when dst.exists() or dst.mkdirs() returns
false, log an error (use Log.e) and set hasExtracted=false (or return/abort the
extraction) so TarCompressorUtils.extract(TarCompressorUtils.Type.ZSTD,
mContext, src, dst) is not invoked on a failed/dirty target; ensure the existing
cleanup branch still deletes dst when hasExtracted is false.
In `@app/src/main/java/com/winlator/core/WineUtils.java`:
- Around line 199-201: The child path strings use a leading slash which causes
new File(imageFsRoot, "/opt/...") to ignore the parent; update the path
arguments in WineUtils so wineSystem32Dir and wineSysWoW64Dir are constructed
with relative child paths (e.g., "opt/wine/lib/wine/x86_64-windows" and
"opt/wine/lib/wine/i386-windows") instead of starting with "/" so the
directories resolve under ImageFs.find(context).getRootDir(); also scan and fix
the similar occurrences used by the DLL copy operations referenced around the
copy calls (the code that reads wineSystem32Dir/wineSysWoW64Dir) and other
ImageFs usages throughout the codebase.
In `@app/src/main/java/com/winlator/renderer/GLRenderer.java`:
- Around line 361-366: The current early "return" in GLRenderer's window-check
(the block using window.getClassName(), window.getProcessId(), and parent vs
xServer.windowManager.rootWindow) stops recursion into child windows and drops
real renderable descendants; change the logic to skip processing/adding the
current phantom window but do not return from the traversal—allow recursion to
continue into its children (i.e., remove the return and ensure the code still
recurses or continues the loop to visit window.getChildren() or the existing
recursive call that collects renderable windows).
In `@app/src/main/java/com/winlator/sysvshm/SysVSharedMemory.java`:
- Around line 83-90: The code holds the shmemories monitor while calling native
XConnectorEpoll.closeFd, which can deadlock; fix by retrieving the SHMemory and
its fd inside the synchronized(shmemories) block, set the SHMemory fd to -1 via
SHMemory.access$002(shmemory, -1) and remove the entry
(shmemories.remove(shmid)) while still synchronized, then release the lock and
call XConnectorEpoll.closeFd(fd) outside the synchronized block; use the local
fd variable (from SHMemory.access$000(shmemory)) so you don’t call the native
method while holding the shmemories monitor.
In `@app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java`:
- Around line 218-222: The code eagerly calls imageFs.getVersion() and
imageFs.getVariant() before checking imageFs.isValid(), which can trigger
failures for invalid state and also uses a non-null-safe String comparison;
change the logic in ImageFsInstaller so you first evaluate boolean valid =
imageFs.isValid() and short-circuit if false (avoid calling
getVersion/getVariant), then only call imageFs.getVersion() and
imageFs.getVariant() when valid is true, compare version against LATEST_VERSION,
and use a null-safe comparison (e.g., Objects.equals(currentVariant,
requestedVariant) or similar) for currentVariant vs
container.getContainerVariant().
In `@app/src/main/java/com/winlator/xserver/extensions/DRI3Extension.java`:
- Around line 161-164: createPixmap() can return null after a drawable was
already created, leaving a leaked/stale drawable when you throw BadIdChoice;
update both code paths (the block using
client.xServer.drawableManager.createDrawable(...) and the similar block at
lines 177–182) so that if client.xServer.pixmapManager.createPixmap(drawable)
returns null you first remove/unregister/destroy the previously created drawable
(e.g. call the drawable manager's removal method or drawable.destroy() —
reference the created Drawable from drawable variable) and only then throw new
BadIdChoice(pixmapId); ensure you use the same drawable manager API used
elsewhere in the class to unregister the drawable.
In `@app/src/main/java/com/winlator/xserver/WindowManager.java`:
- Around line 205-220: The leak reaper is currently collecting candidate windows
into matches without excluding mapped (active) windows, so destroyWindow(...)
can remove live surfaces; update the candidate-filtering logic (the loop that
checks cand.originClient, cand.isInputOutput(), cand.getClassName(),
cand.getWidth()/getHeight()) to also skip mapped windows by adding a
mapped-state guard such as if (cand.isMapped()) continue (or the inverse if your
API is isUnmapped/isVisible), ensuring only unmapped/inactive Window instances
are added to matches before sorting and reaping (symbols: matches,
LEAK_CLIENT_CAP, Window, cand, destroyWindow).
---
Outside diff comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 5110-5120: The extraction currently writes steam.tzst into the
shared imageFs.getRootDir(), risking cross-container writes; change the
extraction destination to a container-scoped path (same scope used in the
existence check) by computing the target root off container.rootDir (or a
container-specific subdir) and pass that path to TarCompressorUtils.extract;
ensure the File downloaded variable and the TarCompressorUtils.extract call both
use the container-scoped destination instead of imageFs.getRootDir() so
existence check and write target match.
In `@app/src/main/java/app/gamenative/utils/FileUtils.kt`:
- Around line 136-176: matchesGlob is too permissive because it only ensures
tokens appear in order; update matchesGlob to enforce anchors: if pattern has no
leading '*', require the first token to match at fileName start; if pattern has
no trailing '*', require the last token to align with fileName end; keep the
existing ordered-internal-token checks (with ignoreCase) and preserve the
special-case for empty or "*" and exact-match when pattern contains no '*'.
Modify the matchesGlob function (used by findFiles and findFilesRecursive) to:
parse tokens via split("*"), detect hasLeadingStar and hasTrailingStar, if no
tokens return true, check prefix match when !hasLeadingStar by verifying
fileName.startsWith(firstToken, ignoreCase=true), check suffix match when
!hasTrailingStar by verifying fileName.endsWith(lastToken, ignoreCase=true), and
still scan intermediate tokens in order updating startIndex after each match;
this prevents matching names like "fooDXSETUP.exe.bak" for "DXSETUP.exe" and
similar false positives.
---
Nitpick comments:
In `@app/src/main/java/com/winlator/container/Container.java`:
- Around line 342-344: The setter Container.setSdkCloudSaveSubdir currently
accepts any string; instead validate and normalize sdkCloudSaveSubdir to a
single safe path component: trim input, return empty string for null/empty,
reject or sanitize any values containing path separators or traversal tokens
(e.g., '/', '\', or ".."), and enforce a whitelist pattern (e.g., allow only
letters, digits, hyphen, underscore); set this.sdkCloudSaveSubdir to the
sanitized value and if invalid either set to empty string or throw an
IllegalArgumentException depending on project policy—update
Container.setSdkCloudSaveSubdir and document the constraint near the
sdkCloudSaveSubdir field.
🪄 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: 59159aa4-d344-48b3-923b-81d424d1aeea
📒 Files selected for processing (35)
app/src/main/assets/box86_64/lightsteam.box64rcapp/src/main/assets/box86_64/ultralightsteam.box64rcapp/src/main/java/app/gamenative/PluviaApp.ktapp/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/gamefixes/types/RegistryKeyFix.ktapp/src/main/java/app/gamenative/service/SteamAutoCloud.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.ktapp/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigState.ktapp/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.ktapp/src/main/java/app/gamenative/ui/enums/DialogType.ktapp/src/main/java/app/gamenative/ui/model/MainViewModel.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/app/gamenative/utils/FileUtils.ktapp/src/main/java/app/gamenative/utils/LudusaviRegistry.ktapp/src/main/java/app/gamenative/utils/PreInstallSteps.ktapp/src/main/java/app/gamenative/utils/SteamTokenLogin.ktapp/src/main/java/app/gamenative/utils/SteamUtils.ktapp/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.ktapp/src/main/java/com/winlator/container/Container.javaapp/src/main/java/com/winlator/container/ContainerData.ktapp/src/main/java/com/winlator/contents/AdrenotoolsManager.javaapp/src/main/java/com/winlator/core/WineThemeManager.javaapp/src/main/java/com/winlator/core/WineUtils.javaapp/src/main/java/com/winlator/renderer/GLRenderer.javaapp/src/main/java/com/winlator/sysvshm/SysVSharedMemory.javaapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/java/com/winlator/xenvironment/ImageFsInstaller.javaapp/src/main/java/com/winlator/xserver/WindowManager.javaapp/src/main/java/com/winlator/xserver/extensions/DRI3Extension.javaapp/src/main/res/values/strings.xml
Comment on lines
+361
to
+366
| if (window.getClassName().isEmpty() && window.getProcessId() == 0) { | ||
| Window parent = window.getParent(); | ||
| if (parent != null && parent != xServer.windowManager.rootWindow | ||
| && parent.getClassName().isEmpty() && parent.getProcessId() == 0) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid returning before visiting descendants of skipped phantom windows.
At Line 365, return prevents recursion into children. If a real renderable child exists under this placeholder chain, it will never be collected (black-screen risk). Skip only the current window, but keep traversing descendants.
Suggested fix
- if (window.getClassName().isEmpty() && window.getProcessId() == 0) {
+ boolean skipSelf = false;
+ if (window.getClassName().isEmpty() && window.getProcessId() == 0) {
Window parent = window.getParent();
if (parent != null && parent != xServer.windowManager.rootWindow
&& parent.getClassName().isEmpty() && parent.getProcessId() == 0) {
- return;
+ skipSelf = true;
}
}
boolean viewable = true;
@@
- if (viewable) {
+ if (viewable && !skipSelf) {
if (forceFullscreenWMClass != null) {
...
} else {
renderableWindows.add(new RenderableWindow(window.getContent(), x, y));
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/winlator/renderer/GLRenderer.java` around lines 361 -
366, The current early "return" in GLRenderer's window-check (the block using
window.getClassName(), window.getProcessId(), and parent vs
xServer.windowManager.rootWindow) stops recursion into child windows and drops
real renderable descendants; change the logic to skip processing/adding the
current phantom window but do not return from the traversal—allow recursion to
continue into its children (i.e., remove the return and ensure the code still
recurses or continues the loop to visit window.getChildren() or the existing
recursive call that collects renderable windows).
Comment on lines
+83
to
+90
| synchronized (shmemories) { | ||
| SHMemory shmemory = shmemories.get(shmid); | ||
| if (shmemory != null) { | ||
| if (SHMemory.access$000(shmemory) != -1) { | ||
| XConnectorEpoll.closeFd(SHMemory.access$000(shmemory)); | ||
| SHMemory.access$002(shmemory, -1); | ||
| } | ||
| shmemories.remove(shmid); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the SysVSharedMemory.java file
find . -type f -name "SysVSharedMemory.java" | head -5Repository: utkarshdalal/GameNative
Length of output: 130
🏁 Script executed:
# Also look for XConnectorEpoll class
find . -type f -name "XConnectorEpoll.java" | head -5Repository: utkarshdalal/GameNative
Length of output: 132
🏁 Script executed:
# And find SHMemory class
find . -type f -name "SHMemory.java" | head -5Repository: utkarshdalal/GameNative
Length of output: 49
🏁 Script executed:
# Examine XConnectorEpoll.java to check closeFd method
cat -n ./app/src/main/java/com/winlator/xconnector/XConnectorEpoll.java | head -100Repository: utkarshdalal/GameNative
Length of output: 4536
🏁 Script executed:
# Examine SysVSharedMemory.java - full file to understand structure
wc -l ./app/src/main/java/com/winlator/sysvshm/SysVSharedMemory.javaRepository: utkarshdalal/GameNative
Length of output: 134
🏁 Script executed:
# Examine SysVSharedMemory.java - read full content
cat -n ./app/src/main/java/com/winlator/sysvshm/SysVSharedMemory.javaRepository: utkarshdalal/GameNative
Length of output: 6087
🏁 Script executed:
# Search for SHMemory class to understand access$000 and access$002
fd -t f "SHMemory" | head -10Repository: utkarshdalal/GameNative
Length of output: 49
Avoid holding shmemories while calling the native close.
XConnectorEpoll.closeFd() is a native JNI call. Keeping the monitor held blocks concurrent get(), attach(), and detach() operations, and risks deadlock if the native implementation re-enters shared-memory code.
Suggested fix
public void delete(int shmid) {
- synchronized (shmemories) {
- SHMemory shmemory = shmemories.get(shmid);
- if (shmemory != null) {
- if (SHMemory.access$000(shmemory) != -1) {
- XConnectorEpoll.closeFd(SHMemory.access$000(shmemory));
- SHMemory.access$002(shmemory, -1);
- }
- shmemories.remove(shmid);
- }
- }
+ int fd = -1;
+ synchronized (shmemories) {
+ SHMemory shmemory = shmemories.get(shmid);
+ if (shmemory == null) return;
+ fd = SHMemory.access$000(shmemory);
+ SHMemory.access$002(shmemory, -1);
+ shmemories.remove(shmid);
+ }
+ if (fd != -1) {
+ XConnectorEpoll.closeFd(fd);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/winlator/sysvshm/SysVSharedMemory.java` around lines 83
- 90, The code holds the shmemories monitor while calling native
XConnectorEpoll.closeFd, which can deadlock; fix by retrieving the SHMemory and
its fd inside the synchronized(shmemories) block, set the SHMemory fd to -1 via
SHMemory.access$002(shmemory, -1) and remove the entry
(shmemories.remove(shmid)) while still synchronized, then release the lock and
call XConnectorEpoll.closeFd(fd) outside the synchronized block; use the local
fd variable (from SHMemory.access$000(shmemory)) so you don’t call the native
method while holding the shmemories monitor.
Comment on lines
+218
to
+222
| boolean valid = imageFs.isValid(); | ||
| int version = imageFs.getVersion(); | ||
| String currentVariant = imageFs.getVariant(); | ||
| String requestedVariant = container.getContainerVariant(); | ||
| if (!valid || version < LATEST_VERSION || !currentVariant.equals(requestedVariant)) { |
Contributor
There was a problem hiding this comment.
Avoid eager metadata reads before the validity gate.
On Line 219–222, getVersion()/getVariant() are now executed even when isValid() is false, which changes short-circuit behavior and can fail on invalid state. Also, Line 222 should use a null-safe variant comparison.
Suggested patch
- boolean valid = imageFs.isValid();
- int version = imageFs.getVersion();
- String currentVariant = imageFs.getVariant();
String requestedVariant = container.getContainerVariant();
- if (!valid || version < LATEST_VERSION || !currentVariant.equals(requestedVariant)) {
+ boolean valid = imageFs.isValid();
+ int version = valid ? imageFs.getVersion() : -1;
+ String currentVariant = valid ? imageFs.getVariant() : null;
+ if (!valid || version < LATEST_VERSION || !java.util.Objects.equals(currentVariant, requestedVariant)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java` around
lines 218 - 222, The code eagerly calls imageFs.getVersion() and
imageFs.getVariant() before checking imageFs.isValid(), which can trigger
failures for invalid state and also uses a non-null-safe String comparison;
change the logic in ImageFsInstaller so you first evaluate boolean valid =
imageFs.isValid() and short-circuit if false (avoid calling
getVersion/getVariant), then only call imageFs.getVersion() and
imageFs.getVariant() when valid is true, compare version against LATEST_VERSION,
and use a null-safe comparison (e.g., Objects.equals(currentVariant,
requestedVariant) or similar) for currentVariant vs
container.getContainerVariant().
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/PluviaMain.kt (1)
811-824: 💤 Low valuePrefer structured coroutine scope over standalone
CoroutineScope.
CoroutineScope(Dispatchers.IO).launchcreates an unstructured coroutine that won't be cancelled if the composable leaves composition (e.g., activity destroyed). TheAPP_UPDATEhandler at lines 1092-1117 usesscope.launchwith the existingrememberCoroutineScope()— use that pattern here for consistency and lifecycle safety.Suggested fix for onConfirmClick
onConfirmClick = { // Enable: write Ludusavi's suggested subdir, then continue the launch. msgDialogState = MessageDialogState(false) - CoroutineScope(Dispatchers.IO).launch { + scope.launch { val gameId = ContainerUtils.extractGameIdFromContainerId(state.launchedAppId) - val rec = runCatching { - SteamUtils.getRecommendedSdkCloudSaveSubdirAsync(context, gameId) - }.getOrNull() + val rec = withContext(Dispatchers.IO) { + runCatching { + SteamUtils.getRecommendedSdkCloudSaveSubdirAsync(context, gameId) + }.getOrNull() + } if (rec != null) { - runCatching { - val container = ContainerUtils.getContainer(context, state.launchedAppId) - container.sdkCloudSaveSubdir = rec.subdir - container.saveData() - }.onFailure { Timber.w(it, "Failed to persist sdkCloudSaveSubdir=${rec.subdir}") } + withContext(Dispatchers.IO) { + runCatching { + val container = ContainerUtils.getContainer(context, state.launchedAppId) + container.sdkCloudSaveSubdir = rec.subdir + container.saveData() + }.onFailure { Timber.w(it, "Failed to persist sdkCloudSaveSubdir=${rec.subdir}") } + } } - withContext(Dispatchers.Main) { relaunch() } + relaunch() } }Also applies to: 834-841
🤖 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 811 - 824, The coroutine launched with CoroutineScope(Dispatchers.IO).launch is unstructured and should be replaced with the composable's remembered scope to tie it to lifecycle; change to using the existing rememberCoroutineScope (e.g. scope.launch { withContext(Dispatchers.IO) { ... } } ) when calling ContainerUtils.extractGameIdFromContainerId(state.launchedAppId) and SteamUtils.getRecommendedSdkCloudSaveSubdirAsync(context, gameId), then persist via ContainerUtils.getContainer(...).saveData() on the IO dispatcher and finally call relaunch() on the Main dispatcher (or call relaunch() after the IO withContext block) so the coroutine is cancelled when the composable/activity is destroyed; apply the same replacement for the similar block around lines 834-841.
🤖 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/xserver/XServerScreen.kt`:
- Around line 5148-5150: extractSteamFiles() currently checks per-container
install presence using File(container.rootDir, ".wine/drive_c/Program Files
(x86)/Steam/steam.exe") but then extracts steam.tzst into imageFs.getRootDir()
(the shared imagefs root), allowing another container to flip the shared layout
during extraction; change the extraction target to a container-owned path (use
container.rootDir or a container-scoped ImageFs API) so all writes go under the
container prefix (or, if feasible, repack steam.tzst to be unpackable without
depending on the global root) and update any references in extractSteamFiles()
that call imageFs.getRootDir() to use the container-specific root instead.
- Around line 359-381: The loop in awaitSteamShutdown currently calls
requestWineProcessSnapshot(winHandler) repeatedly which swaps the shared
OnGetProcessInfoListener and can be clobbered by
startExitWatchForUnmappedGameWindow and the quick-menu flows; instead obtain an
immutable process snapshot per poll and use that for isSteamExeAlive checks.
Modify the code so awaitSteamShutdown calls a new non-listener API (e.g., add a
WinHandler method like requestProcessSnapshotSync or getProcessSnapshot) that
returns a serialized snapshot object, or change requestWineProcessSnapshot to
return a snapshot without swapping the global listener, then use that snapshot
inside the polling loop (keep references to awaitSteamShutdown,
requestWineProcessSnapshot, startExitWatchForUnmappedGameWindow, and
OnGetProcessInfoListener when updating callers). Ensure no listener swapping
occurs from awaitSteamShutdown so other flows aren’t clobbered.
- Around line 3137-3143: The per-container WINEPREFIX set with
envVars.put("WINEPREFIX", File(container.rootDir, ".wine").absolutePath) is
being overwritten by envVars.putAll(container.envVars); move or reapply that
WINEPREFIX assignment after the merge (i.e., call the WINEPREFIX envVars.put
after envVars.putAll(container.envVars)) so the per-container prefix always
wins, or alternatively remove WINEPREFIX from container.envVars before putAll
and then set it; update references around the envVars.putAll and the WINEPREFIX
put to ensure the per-container path is final.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/PluviaMain.kt`:
- Around line 811-824: The coroutine launched with
CoroutineScope(Dispatchers.IO).launch is unstructured and should be replaced
with the composable's remembered scope to tie it to lifecycle; change to using
the existing rememberCoroutineScope (e.g. scope.launch {
withContext(Dispatchers.IO) { ... } } ) when calling
ContainerUtils.extractGameIdFromContainerId(state.launchedAppId) and
SteamUtils.getRecommendedSdkCloudSaveSubdirAsync(context, gameId), then persist
via ContainerUtils.getContainer(...).saveData() on the IO dispatcher and finally
call relaunch() on the Main dispatcher (or call relaunch() after the IO
withContext block) so the coroutine is cancelled when the composable/activity is
destroyed; apply the same replacement for the similar block around lines
834-841.
🪄 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: 2c08ef91-4f82-44bd-b2d9-5ae99d94139d
📒 Files selected for processing (8)
app/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.ktapp/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigState.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/com/winlator/container/ContainerData.ktapp/src/main/java/com/winlator/xenvironment/ImageFsInstaller.javaapp/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (4)
- app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigState.kt
- app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java
- app/src/main/java/app/gamenative/utils/ContainerUtils.kt
- app/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt
- app/src/main/java/com/winlator/container/ContainerData.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.
10 issues found across 35 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.kt:60">
P1: Short-circuiting on a container-wide VCREDIST marker can skip redistributable installs needed by later games in the same prefix.</violation>
</file>
<file name="app/src/main/java/com/winlator/contents/AdrenotoolsManager.java">
<violation number="1" location="app/src/main/java/com/winlator/contents/AdrenotoolsManager.java:137">
P2: Deleting the existing driver directory before extraction can erase a working driver if extraction fails, with no rollback to the previous contents.</violation>
</file>
<file name="app/src/main/java/com/winlator/xserver/WindowManager.java">
<violation number="1" location="app/src/main/java/com/winlator/xserver/WindowManager.java:187">
P1: Leak-reaping can destroy legitimate same-client windows (including ancestors), corrupting the window tree and leaving createWindow() to return a destroyed window.</violation>
</file>
<file name="app/src/main/java/com/winlator/core/WineUtils.java">
<violation number="1" location="app/src/main/java/com/winlator/core/WineUtils.java:200">
P1: Absolute child paths make `imageFsRoot` ineffective, so Wine DLLs are copied from `/opt/...` on the host instead of the imagefs root.</violation>
</file>
<file name="app/src/main/java/app/gamenative/utils/SteamTokenLogin.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/SteamTokenLogin.kt:66">
P2: Timeout cancellation does not actually stop the underlying Wine subprocess, so hung commands can keep running after the timeout.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/SteamAutoCloud.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamAutoCloud.kt:395">
P1: SteamUserData files are skipped for mixed-root Auto-Cloud apps, so they never enter the local scan map and can fall out of sync/cache handling.</violation>
</file>
<file name="app/src/main/java/com/winlator/core/WineThemeManager.java">
<violation number="1" location="app/src/main/java/com/winlator/core/WineThemeManager.java:47">
P2: Per-container migration is incomplete: `user.reg` is moved to the container root, but wallpaper generation still uses the shared `ImageFs` root, which can desynchronize the registry and wallpaper asset paths across containers.</violation>
</file>
<file name="app/src/main/java/app/gamenative/utils/SteamUtils.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/SteamUtils.kt:2584">
P2: SDK cloud mirroring is non-recursive, so nested save directories are never synchronized.</violation>
</file>
<file name="app/src/main/java/com/winlator/xserver/extensions/DRI3Extension.java">
<violation number="1" location="app/src/main/java/com/winlator/xserver/extensions/DRI3Extension.java:162">
P2: The new `drawable == null` early-throw path skips SHM unmapping, leaking the mapped buffer when drawable creation fails.</violation>
</file>
<file name="app/src/main/java/com/winlator/renderer/GLRenderer.java">
<violation number="1" location="app/src/main/java/com/winlator/renderer/GLRenderer.java:365">
P2: Anonymous-window filtering returns before child traversal, so valid descendant windows under matching wrappers are skipped from rendering.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+200
to
+201
| File wineSystem32Dir = new File(imageFsRoot, "/opt/wine/lib/wine/x86_64-windows"); | ||
| File wineSysWoW64Dir = new File(imageFsRoot, "/opt/wine/lib/wine/i386-windows"); |
Contributor
There was a problem hiding this comment.
P1: Absolute child paths make imageFsRoot ineffective, so Wine DLLs are copied from /opt/... on the host instead of the imagefs root.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/com/winlator/core/WineUtils.java, line 200:
<comment>Absolute child paths make `imageFsRoot` ineffective, so Wine DLLs are copied from `/opt/...` on the host instead of the imagefs root.</comment>
<file context>
@@ -189,10 +196,11 @@ public static void applySystemTweaks(Context context, WineInfo wineInfo) {
- File containerSystem32Dir = new File(rootDir, ImageFs.WINEPREFIX+"/drive_c/windows/system32");
- File containerSysWoW64Dir = new File(rootDir, ImageFs.WINEPREFIX+"/drive_c/windows/syswow64");
+ File imageFsRoot = ImageFs.find(context).getRootDir();
+ File wineSystem32Dir = new File(imageFsRoot, "/opt/wine/lib/wine/x86_64-windows");
+ File wineSysWoW64Dir = new File(imageFsRoot, "/opt/wine/lib/wine/i386-windows");
+ File containerSystem32Dir = new File(containerRoot, ".wine/drive_c/windows/system32");
</file context>
Suggested change
| File wineSystem32Dir = new File(imageFsRoot, "/opt/wine/lib/wine/x86_64-windows"); | |
| File wineSysWoW64Dir = new File(imageFsRoot, "/opt/wine/lib/wine/i386-windows"); | |
| File wineSystem32Dir = new File(imageFsRoot, "opt/wine/lib/wine/x86_64-windows"); | |
| File wineSysWoW64Dir = new File(imageFsRoot, "opt/wine/lib/wine/i386-windows"); |
| // an interrupted prior extract that subsequent extracts can't cleanly overwrite. | ||
| if (dst.exists()) | ||
| dst.delete(); | ||
| FileUtils.delete(dst); |
Contributor
There was a problem hiding this comment.
P2: Deleting the existing driver directory before extraction can erase a working driver if extraction fails, with no rollback to the previous contents.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/com/winlator/contents/AdrenotoolsManager.java, line 137:
<comment>Deleting the existing driver directory before extraction can erase a working driver if extraction fails, with no rollback to the previous contents.</comment>
<file context>
@@ -131,15 +131,17 @@ private boolean extractDriverFromResources(String adrenotoolsDriverId) {
+ // an interrupted prior extract that subsequent extracts can't cleanly overwrite.
if (dst.exists())
- dst.delete();
+ FileUtils.delete(dst);
dst.mkdirs();
</file context>
| try { | ||
| future.get(WINE_EXEC_TIMEOUT_SECONDS, TimeUnit.SECONDS) | ||
| } catch (e: TimeoutException) { | ||
| future.cancel(true) |
Contributor
There was a problem hiding this comment.
P2: Timeout cancellation does not actually stop the underlying Wine subprocess, so hung commands can keep running after the timeout.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/utils/SteamTokenLogin.kt, line 66:
<comment>Timeout cancellation does not actually stop the underlying Wine subprocess, so hung commands can keep running after the timeout.</comment>
<file context>
@@ -43,8 +51,28 @@ class SteamTokenLogin(
+ try {
+ future.get(WINE_EXEC_TIMEOUT_SECONDS, TimeUnit.SECONDS)
+ } catch (e: TimeoutException) {
+ future.cancel(true)
+ // Don't log/throw the full command — `command` may contain a refresh JWT
+ // (steam-token.exe encrypt <user> <token>). Log just the executable name.
</file context>
| public static void apply(Context context, ThemeInfo themeInfo, ScreenInfo screenInfo, com.winlator.container.Container container) { | ||
| // Per-container path; xuser symlink is unsafe for writes (see | ||
| // WineUtils.applySystemTweaks rationale). | ||
| File userRegFile = new File(container.getRootDir(), ".wine/user.reg"); |
Contributor
There was a problem hiding this comment.
P2: Per-container migration is incomplete: user.reg is moved to the container root, but wallpaper generation still uses the shared ImageFs root, which can desynchronize the registry and wallpaper asset paths across containers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/com/winlator/core/WineThemeManager.java, line 47:
<comment>Per-container migration is incomplete: `user.reg` is moved to the container root, but wallpaper generation still uses the shared `ImageFs` root, which can desynchronize the registry and wallpaper asset paths across containers.</comment>
<file context>
@@ -41,9 +41,10 @@ public ThemeInfo(String value) {
+ public static void apply(Context context, ThemeInfo themeInfo, ScreenInfo screenInfo, com.winlator.container.Container container) {
+ // Per-container path; xuser symlink is unsafe for writes (see
+ // WineUtils.applySystemTweaks rationale).
+ File userRegFile = new File(container.getRootDir(), ".wine/user.reg");
String background = Color.red(themeInfo.backgroundColor)+" "+Color.green(themeInfo.backgroundColor)+" "+Color.blue(themeInfo.backgroundColor);
</file context>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
2545-2548:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove duplicate GSE→userdata migration in
forceSyncUserFiles.Migration is already done at Line 2542 with
isLaunchRealSteam. The second call at Line 2547 duplicates work and ignores the new mode flag (default path), which can cause inconsistent behavior.Suggested fix
- val context = instance?.applicationContext ?: return@async PostSyncInfo(SyncResult.UnknownFail) - // Migrate GSE Saves to Steam userdata - SteamUtils.migrateGSESavesToSteamUserdata(context, appId) + val context = instance?.applicationContext ?: return@async PostSyncInfo(SyncResult.UnknownFail)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/SteamService.kt` around lines 2545 - 2548, The duplicate migration call SteamUtils.migrateGSESavesToSteamUserdata found in forceSyncUserFiles should be removed — keep only the earlier invocation that runs under the isLaunchRealSteam path; delete the second call that unconditionally runs using the local context variable so migration respects the new mode flag and isn’t executed twice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 2277-2286: The call to SteamUtils.migrateGSESavesToSteamUserdata
is executed before the try/finally that calls releaseSync(appId), so any
exception thrown there can skip releaseSync and leak the sync flag; move the
null-guard and the call into the existing try block (inside the async block that
returns PostSyncInfo) so that releaseSync(appId) in the finally always runs,
keeping the null check for instance?.applicationContext and preserving the
existing return of PostSyncInfo(SyncResult.UnknownFail) when appropriate; apply
the same change for the other occurrence around lines 2535-2543.
In `@app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt`:
- Around line 463-477: The current handler opens the confirm dialog as soon as
the first non-whitespace character is typed because it uses wasBlank &&
nowNonBlank; change the condition so the dialog only opens when the new trimmed
value is meaningfully non-empty (e.g., trimmed.length > 1) to allow normal
typing: in the NoExtractOutlinedTextField onValueChange block, replace the
branch condition (wasBlank && nowNonBlank) with something like (wasBlank &&
nowNonBlank && trimmed.length > 1) and keep the existing
pendingValue/showConfirmDialog logic and the else path that writes
state.config.value = config.copy(sdkCloudSaveSubdir = trimmed).
---
Outside diff comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 2545-2548: The duplicate migration call
SteamUtils.migrateGSESavesToSteamUserdata found in forceSyncUserFiles should be
removed — keep only the earlier invocation that runs under the isLaunchRealSteam
path; delete the second call that unconditionally runs using the local context
variable so migration respects the new mode flag and isn’t executed twice.
🪄 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: e2c45603-2fe0-4849-8524-1b595e87adcf
📒 Files selected for processing (8)
app/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/app/gamenative/utils/FileUtils.ktapp/src/main/java/app/gamenative/utils/SteamTokenLogin.ktapp/src/main/java/app/gamenative/utils/SteamUtils.ktapp/src/main/java/com/winlator/xserver/WindowManager.javaapp/src/main/java/com/winlator/xserver/extensions/DRI3Extension.java
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/app/gamenative/utils/SteamTokenLogin.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/winlator/xserver/WindowManager.java
- app/src/main/java/app/gamenative/utils/SteamUtils.kt
Owner
|
Nice, great fixes - please undo the per container arch and variant because that's already in master |
File.delete() is a silent no-op on non-empty directories, so an extraction interrupted mid-stream (OOM, storage hiccup, process kill) leaves partial files behind. The next launch can't clear them, extract-over-stale returns false, and setDriverById falls through to the "Driver not found" branch — which excludes asset-bundled drivers from its installed-list fallback. ADRENOTOOLS_DRIVER_PATH/HOOKS_PATH/NAME never get set, wrapper_icd has nothing to dlopen, and the game renders a black screen with audio while the process appears healthy. Swap both dst.delete() calls for FileUtils.delete(dst), which clears directory contents before unlinking. Toggling the Graphics Driver Version dropdown off and on was the user-visible workaround — it just forced a retry that happened to not get interrupted.
…ation pixmapFromHardwareBuffer and pixmapFromFd dereferenced the return values of drawableManager.createDrawable / pixmapManager.createPixmap directly. Both return null on resource-ID conflicts (the requested ID was already in use), which produced an opaque NullPointerException at the next line instead of a proper X protocol error. Throw BadIdChoice on null returns so the client sees a meaningful failure and can recover, rather than getting a JVM stacktrace logged into System.err and a half-initialized swapchain. Note: this commit historically also added registerAsOwnerOfResource calls to make the pixmaps client-owned (preventing a leak on disconnect that caused BadIdChoice cascades). That half has been removed pending a soak test post-cascade-fix to determine whether the leak still triggers in normal use or was primarily exposed by cascade-driven Wine client cycling.
Wine's GLX/DRI3 helper (resourceIDBase 0x5C00000) under real-Steam mode can create blank input-output windows every ~10ms without ever destroying them, exhausting the 512MB Dalvik heap via 1.92MB per Drawable ByteBuffer and aborting in XConnectorEpoll. reapLeakedClientWindows runs at end of createWindow. When the same originClient has >= LEAK_CLIENT_CAP (8) blank-className, same-size input-output windows alive, destroy the oldest. Parent-agnostic — the leaked windows get reparented to a 1x1 orphanage before the check runs.
Blank-className pid=0 windows in the Wine winex11.drv/zink path cover two populations: the game's active GL render target (child of the game's top-level HWND, parent has real className+pid) and the orphan leak chain reparented to a 1x1 blank orphanage window. Only the orphan chain has a blank parent. Filter on that: skip a blank window only when its parent is also blank. The active render target falls through and gets drawn. A broader filter (skip all blank windows) hid the game's output. No filter let fresh leak children at (0,0) paint blank over the game every frame before their reparent.
SysVSharedMemory.delete(int) mutated the shmemories SparseArray without
synchronizing on it, while every other mutator (get, attach, detach,
deleteAll) held synchronized(shmemories). delete is invoked from the
SysV SHM connector thread (SysVSHMRequestHandler), and detach is invoked
from the X11 connector thread (MITSHMExtension), so the two can run
concurrently on the same array.
When delete ran mid-iteration in detach, SparseArray's internal DELETED
sentinel (a bare Object) could surface at valueAt(i), producing:
java.lang.ClassCastException: java.lang.Object cannot be cast to
com.winlator.sysvshm.SysVSharedMemory$SHMemory
at SysVSharedMemory.detach(SysVSharedMemory.java:117)
at SHMSegmentManager.detach(SHMSegmentManager.java:26)
at MITSHMExtension.detach(MITSHMExtension.java:74)
Fix: wrap the body of delete(int) in synchronized(shmemories). The lock
is reentrant, so deleteAll (which already holds it while calling delete
per-id) is unaffected.
Files:
app/src/main/java/com/winlator/sysvshm/SysVSharedMemory.java
Stabilizes the "real Steam client" launch path (`steam.exe -applaunch`) so
games like Darkest Dungeon and 868-HACK boot cleanly without the multi-minute
"gray Play" reconcile loop, and adds a per-container toggle to disable the
Steam overlay. Also hardens cloud sync and GSE→userdata migration against
mode switches between emu and real-Steam launches.
- `SteamUtils.writeSteamworksCommonManifest` now writes only the depots whose
`_CommonRedist/<folder>/installscript.vdf` is actually present on disk
(typically 228985 vcredist 2013 + 228989 vcredist 2022) instead of the full
24 PICS-declared depots. The SKIP check compares against the same
present-depot baseline, so a correct manifest survives across launches.
- `SteamUtils.createAppManifest` emits a `SharedDepots` block in the child
game's acf for any depot whose parent app id is known (via
`DepotInfo.depotFromApp`). Declaring shared ownership up front stops Steam
from reparenting depots and cascading `Update Queued` onto the child on
every launch.
- Removed the now-unused `canonical228980SizeOnDisk`,
`canonical228980BytesToDownload`, and `canonical228980BytesToStage`
constants; size-on-disk is summed from the present depots and
bytes-to-download is `0`.
Why: writing all 24 depots made Steam prune 22 phantoms every boot (~3 min of
gray Play). Writing an empty manifest made Steam try to download ~52 MB,
hang `Suspended`, and permanently cascade `Update Queued`. The 2-depot +
`SharedDepots` shape is the only combination that survives Steam's reconcile.
- New per-container `disableSteamOverlay` flag (`Container.java`,
`ContainerData.kt`, `PrefManager.kt`, `strings.xml`).
- Toggle surfaced in the container dialog's General tab, visible only when
"Launch Real Steam" is enabled (`GeneralTab.kt`).
- When enabled, `XServerScreen` exports `DISABLE_VK_LAYER_VALVE_steam_overlay_1=1`
and `SteamNoOverlayUIDrawing=1` into the Wine env so the overlay DLL never
injects.
- `SteamService.beginLaunchApp` / `syncUserFiles` / `closeApp` now take an
`isLaunchRealSteam` flag. In real-Steam mode the Goldberg achievement sync
and the GSE→Steam userdata migration are skipped so the real client owns
its own save state.
- `MainViewModel` tracks the last launch mode (`lastSteamMode` extra) and,
on real→emu transitions, cleans up artifacts from the previous mode to
avoid mixing GSE and real-client save layouts.
- `PluviaMain` threads the launch-mode flag through to `syncUserFiles`.
- `SteamAutoCloud` refactors the byte-identical detection into a shared
lambda and adds a "cache lost but local == remote" branch that silently
rehydrates the cache instead of surfacing a spurious conflict prompt.
- `MainViewModel` adds `SteamFixDiagnostics` (last mapped window class,
game-window-mapped flag, last unmatched window class) and a helper
window-class filter (`STEAM_HELPER_WINDOW_CLASSES`) so stall reports
distinguish the real game window from Steam's own helper UI.
- `XServerScreen` integrates a 60 s stall watchdog and logs window-mapping
events for post-mortem analysis of gray-Play hangs.
- `SteamTokenLogin`, `PreInstallSteps`, `preInstallSteps/VcRedistStep`,
`ContainerUtils`, and `ImageFsInstaller` receive small wiring changes to
support the real-Steam-client launch path (vcredist gating, container
defaults, ImageFs layout).
- [ ] Launch Darkest Dungeon (262060) three times in a row — Play button
should auto-dismiss; no multi-minute gray Play.
- [ ] Launch 868-HACK — clean boot via real Steam client.
- [ ] Switch a container between emu and real-Steam mode; verify saves from
the prior mode are not blended into the new one.
- [ ] Toggle "Disable Steam Overlay" on a real-Steam container and confirm
the overlay DLL does not inject at runtime.
- [ ] With a stale cloud cache matching remote, confirm no conflict dialog
appears.
Three fixes to real-Steam-client launch path, all in SteamUtils.kt: 1. applySteamInstallScriptShim (new): writes HKLM\Software\Valve\Steam\Apps and \InstallScripts entries for appId, 228980, and all 24 canonical 228980 depots with Installed=1 / Run=1. This stops Steam from re-running bundled vc_redist.x86.exe / vc_redist.x64.exe / DXSETUP.exe on every launch, which was racing against the game's own MSVC loader and (on Unity titles) triggering UnityCrashHandler. Called from restoreSteamApi after createAppManifest. 2. verifyRestoredState: no longer requires a .orig sibling next to steam_api*.dll. Games with useLegacyDRM=false never go through replaceSteamApi and so never produce a .orig, which caused the old check to log "DLL marker desync" on every launch and needlessly re-copy pipe DLLs. Now compares the on-disk DLL hash against the pipe asset hash: match = still pipe (bad), mismatch = restored (good). 3. createAppManifest regularDepots-empty path: downgraded from W to I when the existing appmanifest already has a valid buildId + depots + UpdateResult=0. The PICS "regularDepots empty" result is a transient refresh flake and not actionable when the acf is otherwise healthy. Validated on Shiren (2178480) and Darkest Dungeon (262060): shim writes 300 reg entries, vcredist windows no longer appear on 2nd+ launches, DLL hash check logs "DLL marker + hash ok, skipping DLL copy" instead of desync warnings.
The real-Steam launch path is stable across DD / 868-HACK / Shotgun King /
Baba Is You — fold in the remaining structural fixes and strip the
investigation-era diagnostic scaffolding that was left behind.
SteamUtils.kt:
- Replace hardcoded canonical228980Depots table with a runtime PICS resolver
driven by the child acf's SharedDepots. Fixes permanent "gray Play" on
games whose shared-redist depot set diverges from DD's (was silently
writing InstalledDepots{} + BytesToDownload=52MB on those).
- Add SHA-256 verify-and-reheal to STEAM_DLL_REPLACED / STEAM_DLL_RESTORED
markers in replaceSteamApi/restoreSteamApi so a stale marker can no
longer lie about on-disk state (2nd-launch black-screen repro).
- Add validateAcfShape() self-check: after writing any child or 228980
manifest, log Timber.e on shapes known to cause gray Play (Update
Required bit set, BytesToDownload > 0, InstalledDepots empty when it
shouldn't be). Catches future regressions loudly instead of silently.
Diagnostic cleanup:
- Drop SteamFixDiagnostics object, STEAM_HELPER_WINDOW_CLASSES,
REAL_STEAM_STALL_WATCHDOG_MS, and all Timber.tag("SteamFix") log lines
across SteamUtils, SteamService, SteamAutoCloud, SteamTokenLogin,
MainViewModel, XServerScreen, ImageFsInstaller.
- Rewrite the comments that referenced the investigation's numbered
SteamFix list (utkarshdalal#8, utkarshdalal#9, utkarshdalal#11, utkarshdalal#16–utkarshdalal#24) so they stand on their own.
UX default:
- Default disableSteamOverlay to true in Container.java, ContainerData.kt,
and PrefManager.kt. Overlay-on is an opt-in now.
Housekeeping:
- .gitignore: exclude .claude/ session state.
- Drop tracked .claude/scheduled_tasks.lock.
Addresses three related issues seen when launching games in real-Steam mode. 1. Suppress Wine-hosted Steam client's cloud sync (Option B) The Wine-hosted Steam client was racing GameNative's SteamKit cloud sync on every launch/exit. Per-app gates now disable the Wine client's cloud path in userdata/<steamID>/config/localconfig.vdf (cloud_enabled=0, cce=0) for both the existing-file and new-file branches, with a setOrReplaceKey helper to avoid duplicates. 2. Graceful red-exit with 5s grace window The quick-menu EXIT_GAME action previously went straight to a SIGKILL of the Wine process tree, giving games no chance to flush saves. It now sends WM_CLOSE to the game exe (and to steam.exe when real-Steam mode is on), waits 5s, then proceeds with the existing exit path. A second tap within the grace window cancels the wait and force-quits. 3. Filter localhost pending-operations in real-Steam mode Steam's server reports the Wine-hosted client's self-registered session as machineName="localhost" with UploadPending, causing a spurious "Pending Upload" dialog before every real-Steam launch. beginLaunchApp now filters pendingRemoteOperations by machineName="localhost" when isLaunchRealSteam is true. Genuine entries from other devices still surface the dialog, and the kickPlayingSession path is unaffected. 4. Fix inverted uploadsRequired flag on exit sync signalAppExitSyncDone was passing uploadsRequired = (... == false), producing impossible states like "upload succeeded but wasn't required". Flipped to == true so Steam's server gets a truthful signal and stops holding stale pending markers. Also: ignore .claude/ session state directory.
Three audit findings from the Fix-Steam-Client branch review: - XServerScreen: cancel gracefulExitJob in DisposableEffect.onDispose. If the screen disposed during the 5s force-quit grace window, the coroutine leaked (only exitWatchJob was cancelled). - SteamUtils.createAppManifest: filter shared depots whose depotFromApp is 0 before writing the SharedDepots block. Writing `"<id>" "0"` told Steam the depot belonged to a nonexistent app 0, which could re-trigger the PICS reconcile / gray-Play cascade this branch was built to prevent. Unowned shared depots are now logged and omitted. - SteamUtils.applySteamInstallScriptShim: narrow the blanket `catch (Exception)` to IOException + SecurityException so programming errors (NPE, ISE) surface instead of being silently swallowed.
In real-Steam mode the Wine-hosted Steam client logs into Steam's servers and registers its own AppSessionActive under machineName "localhost" (the container's hostname). With cloud_enabled=0 set on the Wine client's localconfig.vdf (Option B from the earlier real-Steam sync-race fix), the Wine client never signals upload state for its localhost session, so Steam's server parks it in UploadPending forever. Desktop Steam then reads that on next launch and shows "Cloud Out of Date — you played on localhost, upload not started" even after GameNative's own sync completed cleanly. After GameNative's post-exit SteamKit sync calls signalAppExitSyncDone, also call kickPlayingSession() to clear any lingering session for this user on Steam's server. The game is already exited at this point, so kicking is safe and only affects the stale Wine-client entry that would otherwise trigger the desktop dialog.
… modes Reconstructing the timeline of a broken Steam dir from ls -la mtimes wasted hours this session. A Timber line per launch with size+mtime of steam.exe, steamclient.dll, steamclient64.dll will surface any unexpected change the moment it happens. Also lift the steam.cfg writer out of restoreSteamApi (emu-only path) into an ensureSteamCfg helper called from the real-Steam extractSteamFiles path too, so update-inhibit keys land on fresh Wine-prefix seeds regardless of launch mode.
Real-Steam launches stalled after cloud sync because Steam's server saw a pending cloud operation from machine 'localhost' and refused to start the game (BYieldingAppLaunchIntent returned 1 pending remote operations). The phantom operation came from GameNative's own SteamKit client: on every beginLaunchApp it called signalAppLaunchIntent(machineName= 'localhost'), which registers a launch-intent server-side. In emulation mode this is correct — GameNative IS the client running the game. In real-Steam mode it's harmful — the Wine-hosted Steam client performs its own BYieldingAppLaunchIntent, sees the localhost entry we just created, treats it as a conflicting session, and sits forever waiting for it to clear. The earlier ef5b5b5 fix filtered localhost entries out of our local view of pendingRemoteOperations but never stopped creating them. This change skips the RPC entirely when isLaunchRealSteam=true, so the server never records the phantom operation in the first place. Verified by cloud_log.txt on a stuck Baba Is You (appid 736260) launch: [AppID 736260] BYieldingAppLaunchIntent returned 1 pending remote operations: Operation '3' from machine 'localhost' after which Steam never attempted to launch the game.
Two defense-in-depth additions to SteamService.beginLaunchApp that close gaps where a pending AppSessionActive op on Valve's servers can survive a bad launch and block future launches in either mode. 1. Proactive kick before real-Steam launch. When isLaunchRealSteam=true, call kickPlayingSession() up front. Wine-Steam establishes its own session via BYieldingAppLaunchIntent once it boots, so wiping any stale orphaned entry first costs nothing and clears phantoms left by a prior emulation crash that never reached the exit-cleanup path. 2. Finally-block cleanup on aborted emulation launches. Track whether we registered a launch intent via signalAppLaunchIntent, and in the finally block kick the session if the intent was registered but the flow didn't end in Success/UpToDate (PendingOperations dialog dismissed, exception partway through, caller gave up). Stops leaked phantoms from blocking subsequent launches; the successful-launch- then-exit case is still handled by closeApp as before. Motivating incident: Dead Cells black-screened after the fe0f313 localhost-skip fix because the phantom was under device name, not "localhost" — from an earlier emulation launch that errored mid-flight. Manual reset via desktop Steam cleared it. These hardening steps close that loop automatically.
When every pending remote operation returned by signalAppLaunchIntent is from our own machine name, treat them as self-phantoms from a prior session that died without cleaning up (Android-killed process, upload in-flight at force-close). Kick any stale AppSessionActive and re-signal with ignorePendingOperations=true in-flight, then proceed silently. Cross-device conflicts (different machine name) still surface the dialog so genuine "someone else is uploading from your PC" cases are preserved. Closes the last repro path where the user had to manually launch in emulation mode with "Play Anyway" to clear a stuck Pending Upload dialog before the next real-Steam launch would work. The proactive kick added in 79c112c only clears AppSessionActive; this handles UploadPending and the other upload-side markers that kickPlayingSession doesn't release, matching what the manual workaround was doing.
Fixes two real-Steam launch stalls that manifest as black screens after
cloud sync:
1. Cloud-conflict dialog hidden by -silent
STEAM_LAUNCH_FLAGS passed -silent to steam.exe, which not only hides
the main window but also suppresses the cloud-conflict resolution
dialog. Games with save-sync conflicts (or pending uploads) sat at a
black screen forever with no way to resolve them. -silent was being
injected from two sources; both are removed:
- app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
- app/src/main/assets/box86_64/lightsteam.box64rc
- app/src/main/assets/box86_64/ultralightsteam.box64rc
(the box64rc WINEARGS= entry was shadowing the Kotlin change)
2. localhost pending-operation phantoms
Wine-Steam's own BYieldingAppLaunchIntent was observing stale
UploadPending / UploadInProgress markers from a prior Wine-Steam
session (registered under machineName="localhost") that was killed
mid-cycle, and stalling the launch. The existing proactive
kickPlayingSession() only clears AppSessionActive; upload markers
survive it.
SteamService.beginLaunchApp now performs a full phantom-clearing
cycle in real-Steam mode before Wine-Steam is spawned:
- kickPlayingSession() — clears AppSessionActive
- signalAppLaunchIntent(ignore=true) — server-side "Play Anyway"
dismissal that also clears
UploadPending / UploadInProgress
regardless of origin (self or
stale localhost)
- signalAppExitSyncDone() — cleanly closes the session so
Wine-Steam's subsequent
BYieldingAppLaunchIntent sees
no GN-side session
Mirrors the existing self-phantom auto-clear technique, but runs it
proactively in the real-Steam path where signalAppLaunchIntent is
otherwise skipped entirely.
Also in SteamUtils.kt:
3. Shared-helper manifest writer refactor
createAppManifest now delegates to writeAllSharedAppManifests
(data-driven over a sharedHelperApps list) instead of the single-
purpose writeSteamworksCommonManifest. Lets future helper apps
plug in without touching the child-manifest path. 228980 is the
only entry for now; 241100 was tried, caused a 20s Steam reconcile
loop during gameplay, and is listed in retiredSharedHelperAppIds
so leftover acfs from the buggy build get cleaned up on next
launch. validateAcfShape now loops over the helper list rather
than hard-coding one appId.
Verified
- Shotgun King (1972440) launched cleanly with the new phantom-clearing
cycle after black-screening before the fix; cloud_log.txt confirmed
the "Operation '3' from machine 'localhost'" pending op no longer
appears on retry.
…Number The prior proactive phantom-clear always ran the full signalAppLaunchIntent(ignore=true) + signalAppExitSyncDone( uploadsCompleted=true) cycle on every real-Steam launch. On a clean launch with no phantom present this lied to Steam's cloud that a session had finished with uploads completed, which advanced the server-side ChangeNumber and caused Wine-Steam to "forget" steam_autocloud.vdf on the next launch. Games with Steamworks cloud integration (observed on 868-HACK) then exited with code 1 a few seconds after launch. Probe first: bare signalAppLaunchIntent with ignorePendingOperations= false returns pending ops without dismissing them. Only run the dismissal cycle when the probe actually reports a phantom. When we do run it, use honest signalAppExitSyncDone(uploadsCompleted=false, uploadsRequired=false) — we didn't upload anything. Always kickPlayingSession at the end to release the AppSessionActive the probe RPC registered under our machineName, so Wine-Steam's subsequent localhost intent doesn't see a conflicting session from us. Shotgun King phantom-clearing path still works (the probe detects the phantom and runs the dismissal); clean launches no longer touch ChangeNumber.
The red exit button was hard-killing steam.exe, which prevented Steam from flushing config/localconfig, installscript completion markers, and sending its own clean-shutdown IPC to the running game. Next launch of the same game would see the prefix in an incomplete state and re-run the game's vcredist/DirectX installscripts. Replace killProcess with `steam.exe -shutdown` — Steam's own graceful exit. Poll listProcesses at 1Hz to detect when steam.exe is gone, then proceed with the normal exit sequence. Cap the wait at 15s and show a modal asking "Keep waiting" or "Force quit" if Steam is still winding down (cloud saves, workshop sync, etc.). The dialog is a Compose overlay and does not pause emulation. Also adds a WinHandler.exec(filename, parameters) overload so callers can pass paths containing spaces (the existing single-string exec splits on the first space, which mangles "C:\Program Files (x86)\..." invocations).
Pluvia's SteamAutoCloud owns cloud in both modes with Wine-Steam
cloudenabled=0 + -no-browser. That left SDK-cloud games reading stale
local state because Wine-Steam's ISteamRemoteStorage had no index and
some games read saves from <install>/<subdir>/ rather than
<userdata>/<appid>/remote/.
SteamAutoCloud:
- writeRemoteCacheVdf called after every sync path (downloadUserFiles,
upload, rehydrate, no-change) so Wine-Steam's SDK sees cloud files.
- rebaseToAutoCloud redirects SteamUserData-rooted cloud entries to the
matching saveFilePattern target for Auto-Cloud games whose uploads
were misrooted.
- Skip the SteamUserData catch-all scan for Auto-Cloud apps so ghost
SteamUserData files don't get re-uploaded forever.
SteamUtils:
- RemoteCacheFile + writeRemoteCacheVdf writer.
- sdkCloudSaveMirrors registry + mirrorSdkCloudRemoteToSave /
mirrorSdkCloudSaveToRemote (seeded with 588650 -> "save").
- writeSharedConfigCloudDisabled mirrors cloudenabled=0 into
sharedconfig.vdf; both "cloudenabled" and "cloud_enabled" written,
last_sync_state / autocloud bookkeeping cleared.
- purgePhantomAppUserdata removes userdata/<sid>/<phantomAppId>/ so a
prior synthetic appmanifest can't leave an AutoCloud watcher behind
that blocks steam.exe -shutdown.
SteamService:
- beginLaunchApp calls mirrorSdkCloudRemoteToSave after a successful
sync so downloaded bytes reach the install dir before the game runs.
- closeApp calls mirrorSdkCloudSaveToRemote before the upload sync so
progress in <install>/save/ reaches remote/ before Pluvia uploads.
XServerScreen:
- steam.exe launched with -no-browser (webhelper is a documented cause
of shutdown hangs under Wine).
- STEAM_SHUTDOWN_WAIT_MS 15s -> 20s.
- purgePhantomAppUserdata("241100") on real-Steam container prep.
SteamAppScreen: thread isLaunchRealSteam through forceSyncUserFiles so
manual cloud sync uses the right GSE <-> SteamUserData migration path.
FileUtils: extract matchesGlob helper; drop per-file log spam from
findFiles / findFilesRecursive.
Adds per-container support for "Pattern B" SDK-cloud games (Dead Cells-style) that read saves from a subdirectory of their install directory rather than from <userdata>/<appid>/remote/. Replaces the hardcoded one-entry registry with the Ludusavi save-path manifest (~1944 Steam games matching Pattern B). Container setting: - New sdkCloudSaveSubdir field on Container/ContainerData, persisted through JSON and Compose Saver. Empty = bridge disabled. - "Cloud Save Bridge" section in GeneralTab (visible when Launch Steam Client is on) with text field + Use Recommended / Detect / Clear buttons, confirmation dialog on first activation, and validation rejecting path separators, .., and drive letters. Ludusavi integration (utils/LudusaviRegistry.kt): - Fetches manifest.yaml from mtkennerly/ludusavi-manifest via the existing OkHttp client. Streaming line-parser avoids the OOM that SnakeYAML's eager map load caused on the 5 MB file. - Filters to Steam-IDed entries with <base>/<subdir> save paths tagged "save" and applicable to Windows (or no OS filter). - Writes ~190 KB filtered JSON to filesDir/ludusavi_pattern_b.json, 7-day TTL, falls back to stale disk cache on fetch failure. - Primed in background at PluviaApp.onCreate so Use Recommended and the launch-time prompt are instant after the first session. Launch-time prompt: - preLaunchApp runs a Pattern B check (Ludusavi match AND no saveFilePatterns in PICS UFS) before cloud sync. Match fires an SDK_CLOUD_BRIDGE_SUGGESTION dialog with Enable / Skip / Don't ask again. Catches users who install with real-Steam default and never touch settings. - Don't-ask-again persists per-container as extraData.sdkCloudBridgePromptDismissed. Runtime mirror (utils/SteamUtils.kt): - sdkCloudGameSaveDir now reads the user-configured subdir only; no implicit fallback, to avoid REPLACE_EXISTING copies into a guessed dir that might be wrong. - detectSdkCloudSaveSubdir resolves paths via the container's own rootDir rather than the global xuser symlink so it works even when a different container is activated. Removed: - Bundled assets/sdk_cloud_save_bridge.json (single hardcoded entry); Ludusavi covers it plus ~1943 others. Cosmetic: replaced "Pluvia" with "GameNative" in comments/strings written on this branch. PluviaApp/PluviaTheme/PluviaPreferences class names and storage keys left alone.
DRI3Extension.pixmapFromFd - Move drawable.setOnDestroyListener(onDestroyDrawableListener) so it only fires after createPixmap succeeds. Registering it earlier caused a double-unmap on the failure path: removeDrawable() invoked the listener (unmap #1), then the finally block unmapped again because handedOffToDrawable was still false. (Bug introduced by my earlier SHM-leak fix.) GeneralTab SdkCloudSaveSubdirField - Drop the wasBlank/meaningful/confirm-dialog gate from the typing path entirely. The original wasBlank-on-first-keystroke check fired the confirm dialog on the first character and disrupted typing; my length>1 patch made it never fire because by char 2 wasBlank was already false. Manual typing is intentional — commit each keystroke directly. The first-activation confirmation is reserved for auto-filled values from the Recommend / Detect buttons below, which the user didn't type. SteamService phantom dismiss - Filter probed pending ops by machineName before auto-dismissing. Only entries from our machineName (or "localhost") are treated as phantoms; any cross-device entry blocks the auto-dismiss path so legitimate cloud conflicts surface to the SYNC_CONFLICT dialog instead of getting silently wiped by ignorePendingOperations=true. SteamService beginLaunchApp offline gate - Move the isOffline/!isConnected early-return below the GSE→userdata migrate, inside the existing try block. Local prep should run even offline (one-shot file moves, not cloud-dependent). The cloud RPCs below the gate continue to short-circuit when offline. PluviaMain SDK-cloud-bridge prompt - Don't fire the prompt for non-game launches: bootToContainer (Open Container), useTemporaryOverride, or skipCloudSync flows. The post- prompt `relaunch` lambda doesn't carry those flags forward, so an Open-Container that triggered the prompt would silently turn into a game launch on dismiss. Easier to gate the prompt out than thread every flag through the relaunch. SteamUtils verifyReplacedState / verifyRestoredState - Treat a missing pipe-asset hash as a verification FAILURE rather than a silent skip. The previous `?: return@forEach` would skip the file and leave the function returning true — meaning a future build that drops a pipe DLL asset would silently let the marker check pass and suppress DLL repair. Fail closed instead. LudusaviRegistry.ensureLoaded - Wrap the fetch + disk-write + memoryCache-populate path in a kotlinx.coroutines Mutex with a double-check on entry. Without it, concurrent primeCache() + lookup() callers could both pass the null check and both fetch the 5 MB manifest. Now the second waiter sees the populated cache after the first finishes. Findings flagged but not changed: - SteamUtils SDK cloud mirror nested directories (third time): mirror is by design for Dead Cells flat saves; recursive expansion remains scope creep.
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
1 similar comment
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/utils/SteamUtils.kt (1)
595-612:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCreate the per-container
Steam/configdirectory before writingloginusers.vdf.On a fresh isolated prefix,
writeText()can fail here because the parent directory has never been created. The catch just logs and skips the whole autologin/bootstrap write.♻️ Suggested fix
val winePrefixBase: File = container?.let { File(it.rootDir, ".wine") } ?: File(imageFs.wineprefix) val steamConfigDir = File(winePrefixBase, "drive_c/Program Files (x86)/Steam/config") try { + steamConfigDir.mkdirs() File(steamConfigDir, "loginusers.vdf").writeText(vdfFileText) val userRegFile = File(winePrefixBase, "user.reg") val steamRoot = "C:\\Program Files (x86)\\Steam"🤖 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/SteamUtils.kt` around lines 595 - 612, The code may fail when writing loginusers.vdf because the steamConfigDir doesn't exist; before calling File(steamConfigDir, "loginusers.vdf").writeText(...) ensure the directory exists by creating steamConfigDir (e.g., call mkdirs()/createDirectories on steamConfigDir) and check its success, then proceed to write; keep the existing try/catch around the file and registry operations (winePrefixBase, steamConfigDir, loginusers.vdf, user.reg, WineRegistryEditor) so failures are still logged.
♻️ Duplicate comments (3)
app/src/main/java/app/gamenative/utils/ContainerUtils.kt (1)
813-867:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFresh containers can still miss persisted defaults.
This branch still hand-builds
ContainerDatainstead of starting fromgetDefaultContainerData(). It already diverges again:localSavesOnly,useSteamInput, and the sharpness fields are present ingetDefaultContainerData()but not here, so brand-new containers won't inherit those saved defaults until the config is opened and saved again.♻️ Suggested fix
- } else { - // Use default config with drives - ContainerData( - screenSize = PrefManager.screenSize, - envVars = PrefManager.envVars, - cpuList = PrefManager.cpuList, - cpuListWoW64 = PrefManager.cpuListWoW64, - graphicsDriver = PrefManager.graphicsDriver, - graphicsDriverVersion = PrefManager.graphicsDriverVersion, - graphicsDriverConfig = PrefManager.graphicsDriverConfig, - dxwrapper = initialDxWrapper, - dxwrapperConfig = PrefManager.dxWrapperConfig, - audioDriver = PrefManager.audioDriver, - wincomponents = PrefManager.winComponents, - drives = drives, - execArgs = PrefManager.execArgs, - showFPS = false, - launchRealSteam = PrefManager.launchRealSteam, - disableSteamOverlay = PrefManager.disableSteamOverlay, - wow64Mode = PrefManager.wow64Mode, - startupSelection = PrefManager.startupSelection.toByte(), - box86Version = PrefManager.box86Version, - box64Version = PrefManager.box64Version, - box86Preset = PrefManager.box86Preset, - box64Preset = PrefManager.box64Preset, - desktopTheme = WineThemeManager.DEFAULT_DESKTOP_THEME, - language = PrefManager.containerLanguage, - containerVariant = PrefManager.containerVariant, - wineVersion = PrefManager.wineVersion, - emulator = PrefManager.emulator, - fexcoreVersion = PrefManager.fexcoreVersion, - fexcoreTSOMode = PrefManager.fexcoreTSOMode, - fexcoreX87Mode = PrefManager.fexcoreX87Mode, - fexcoreMultiBlock = PrefManager.fexcoreMultiBlock, - fexcorePreset = PrefManager.fexcorePreset, - renderer = PrefManager.renderer, - csmt = PrefManager.csmt, - videoPciDeviceID = PrefManager.videoPciDeviceID, - offScreenRenderingMode = PrefManager.offScreenRenderingMode, - strictShaderMath = PrefManager.strictShaderMath, - useDRI3 = PrefManager.useDRI3, - videoMemorySize = PrefManager.videoMemorySize, - mouseWarpOverride = PrefManager.mouseWarpOverride, - enableXInput = PrefManager.xinputEnabled, - enableDInput = PrefManager.dinputEnabled, - dinputMapperType = PrefManager.dinputMapperType.toByte(), - disableMouseInput = PrefManager.disableMouseInput, - forceDlc = PrefManager.forceDlc, - steamOfflineMode = PrefManager.steamOfflineMode, - useLegacyDRM = PrefManager.useLegacyDRM, - unpackFiles = PrefManager.unpackFiles, - suspendPolicy = PrefManager.suspendPolicy, - portraitMode = PrefManager.portraitMode, - externalDisplayMode = PrefManager.externalDisplayInputMode, - externalDisplaySwap = PrefManager.externalDisplaySwap, - ) - } + } else { + getDefaultContainerData().copy( + drives = drives, + dxwrapper = initialDxWrapper, + ) + }🤖 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/ContainerUtils.kt` around lines 813 - 867, The code constructs a new ContainerData instance manually which omits persisted default fields (e.g., localSavesOnly, useSteamInput, and sharpness-related fields) and thus new containers miss those saved defaults; instead, initialize from getDefaultContainerData() and then override only differing properties (or copyInto) so ContainerData created here includes all fields from getDefaultContainerData(), ensuring fields like localSavesOnly, useSteamInput, and sharpness settings are preserved; adjust the creation site where ContainerData(...) is constructed to start from getDefaultContainerData() and apply the drives and any required overrides.app/src/main/java/app/gamenative/utils/SteamUtils.kt (2)
256-277:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
verifyRestoredState()still false-passes when the Steam API DLLs are missing.If a stale
STEAM_DLL_RESTOREDmarker exists and neithersteam_api.dllnorsteam_api64.dllis present, this returnstrueand skips repair. That leaves the next launch with no Steam API DLL on disk.♻️ Suggested fix
private fun verifyRestoredState(context: Context, appDirPath: String): Boolean { return try { val assetHashes = pipeDllHashes(context) + var found = false Paths.get(appDirPath).toFile().walkTopDown().maxDepth(10).forEach { file -> if (!file.isFile) return@forEach val n = file.name.lowercase() if (n == "steam_api.dll" || n == "steam_api64.dll") { + found = true val pipeHash = assetHashes[n] if (pipeHash == null) { // Missing pipe-asset hash means we can't tell whether this is the pipe // DLL or the original Valve DLL — fail closed so the caller re-restores. Timber.w("DLL marker desync: pipe asset hash missing for %s, can't verify restored state", n) @@ } } } + if (!found) { + Timber.w("DLL marker desync: no steam_api DLL found under %s but RESTORED marker present", appDirPath) + return false + } true } catch (e: Exception) { Timber.w(e, "verifyRestoredState failed, treating as desync") 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/utils/SteamUtils.kt` around lines 256 - 277, verifyRestoredState currently returns true when neither steam_api.dll nor steam_api64.dll are present, which falsely signals a restored state; update verifyRestoredState to track whether any target DLLs were encountered while walking the directory (use the existing loop that checks file.name lowercase and references pipeDllHashes and sha256OfFile), and if none are found at the end of the check, log a warning and return false so the caller will re-restore the DLLs; keep the existing desync checks intact and only change the final return behavior based on a "found" flag.
2595-2609:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSDK-cloud mirroring still drops nested save trees.
Both directions still copy only direct files. Games that keep saves under subdirectories inside
remote/or the install save folder will silently lose part of their state during mirror.♻️ Suggested fix
- remoteDir.listFiles()?.forEach { src -> - if (!src.isFile) return@forEach - val dst = File(gameSaveDir, src.name) + remoteDir.walkTopDown().forEach { src -> + if (!src.isFile) return@forEach + val rel = remoteDir.toPath().relativize(src.toPath()) + val dst = gameSaveDir.toPath().resolve(rel).toFile() try { + Files.createDirectories(dst.toPath().parent) Files.copy( src.toPath(), dst.toPath(), StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.COPY_ATTRIBUTES, @@ - gameSaveDir.listFiles()?.forEach { src -> - if (!src.isFile) return@forEach + gameSaveDir.walkTopDown().forEach { src -> + if (!src.isFile) return@forEach // Skip local-only artifacts (e.g. Dead Cells writes backup-YYYY-MM-DD-N.zip snapshots // alongside saves; those aren't cloud-synced). if (src.name.startsWith("backup-") && src.name.endsWith(".zip")) return@forEach - val dst = File(remoteDir, src.name) + val rel = gameSaveDir.toPath().relativize(src.toPath()) + val dst = remoteDir.toPath().resolve(rel).toFile() try { + Files.createDirectories(dst.toPath().parent) Files.copy( src.toPath(), dst.toPath(), StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.COPY_ATTRIBUTES,Also applies to: 2619-2636
🤖 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/SteamUtils.kt` around lines 2595 - 2609, The mirror loop only copies direct files and skips nested directories, causing loss of saves; replace the per-file Files.copy logic in the remoteDir.listFiles()?.forEach block (and the corresponding block around lines 2619-2636) with a recursive copy that preserves directory structure and attributes (e.g., use Kotlin's File.copyRecursively(dst, overwrite = true) or a Files.walkFileTree-based copier) so subdirectories and their files are mirrored too; ensure you still skip non-regular entries appropriately, preserve attributes (equivalent to StandardCopyOption.COPY_ATTRIBUTES), and keep the Timber.i / Timber.w logging for each copied file or directory so failures are reported per-item.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/utils/FileUtils.kt (1)
136-163: ⚡ Quick winAdd focused tests for the anchoring and overlap cases.
This matcher is now the shared implementation behind both search APIs, and the prefix/suffix overlap rules are easy to regress. A small table-driven test set for cases like
ab*bcvsabc,*foo*foo*, and exact no-*matches would lock the behavior down.🤖 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/FileUtils.kt` around lines 136 - 163, Add focused, table-driven unit tests for matchesGlob to cover anchoring and overlap rules: verify exact-match behavior when pattern contains no '*' (e.g., "abc" vs "abc" and "ABC"), verify non-overlap for patterns like "ab*bc" against "abc" and "abxbc", verify trailing-anchor overlap rejection when prefix consumes into the last token, and test repeated tokens like "*foo*foo*" and variants; exercise hasLeadingStar/hasTrailingStar paths and middle-token scanning by calling matchesGlob with these patterns and asserting true/false expected outcomes.
🤖 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 811-823: The code always calls relaunch() (which sets
skipBridgePrompt = true) even when no recommendation was found or persisting
failed; change the flow so relaunch only proceeds with skipBridgePrompt=true
after a successful persistence of the chosen subdir: call
SteamUtils.getRecommendedSdkCloudSaveSubdirAsync and if it returns non-null
attempt to get the Container via ContainerUtils.getContainer(context,
state.launchedAppId) and call container.saveData(); only if container.saveData()
completes without throwing (or returns an explicit success) then call relaunch()
with skipBridgePrompt=true; otherwise either call relaunch() without
skipBridgePrompt or pass the chosen subdir in-memory into relaunch so the
one-time launch uses the intended subdir without marking the bridge enabled
permanently (reference symbols:
SteamUtils.getRecommendedSdkCloudSaveSubdirAsync, ContainerUtils.getContainer,
container.sdkCloudSaveSubdir, container.saveData, relaunch,
state.launchedAppId).
- Around line 1641-1648: The bridge dialog gate in preLaunchApp() allows the
SDK-cloud prompt even when re-entered with preferredSave or
ignorePendingOperations set, causing relaunch() to drop those choices; update
the conditional that checks skipBridgePrompt, bootToContainer,
useTemporaryOverride, skipCloudSync, and gameSource == GameSource.STEAM to also
require that preferredSave and ignorePendingOperations are not already set
(i.e., skip the prompt when either preferredSave or ignorePendingOperations is
true), so the bridge dialog is not shown on conflict/pending-op relaunches and
user choices are preserved.
In `@app/src/main/java/app/gamenative/utils/SteamUtils.kt`:
- Around line 1107-1117: The early-return reuse logic in the block that calls
parseAcfBuildId, parseAcfInstalledDepotIds, parseAcfInstallScriptDepotIds and
parseAcfUpdateResult ignores the manifest's LastOwner, allowing a stale
appmanifest with a different Steam account to be reused; update the condition to
also compare the manifest LastOwner (e.g. parseAcfLastOwner(staleAcf)) against
the current/shared last-owner value (the same source used for
sharedBuildId/sharedDepotIds), and only allow the early return (and skip
staleAcf.delete()) when depotsMatch, buildIdMatch, scriptsMatch AND
lastOwnerMatch; add the parseAcfLastOwner call and the lastOwnerMatch boolean to
the same if-check before returning.
---
Outside diff comments:
In `@app/src/main/java/app/gamenative/utils/SteamUtils.kt`:
- Around line 595-612: The code may fail when writing loginusers.vdf because the
steamConfigDir doesn't exist; before calling File(steamConfigDir,
"loginusers.vdf").writeText(...) ensure the directory exists by creating
steamConfigDir (e.g., call mkdirs()/createDirectories on steamConfigDir) and
check its success, then proceed to write; keep the existing try/catch around the
file and registry operations (winePrefixBase, steamConfigDir, loginusers.vdf,
user.reg, WineRegistryEditor) so failures are still logged.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/utils/ContainerUtils.kt`:
- Around line 813-867: The code constructs a new ContainerData instance manually
which omits persisted default fields (e.g., localSavesOnly, useSteamInput, and
sharpness-related fields) and thus new containers miss those saved defaults;
instead, initialize from getDefaultContainerData() and then override only
differing properties (or copyInto) so ContainerData created here includes all
fields from getDefaultContainerData(), ensuring fields like localSavesOnly,
useSteamInput, and sharpness settings are preserved; adjust the creation site
where ContainerData(...) is constructed to start from getDefaultContainerData()
and apply the drives and any required overrides.
In `@app/src/main/java/app/gamenative/utils/SteamUtils.kt`:
- Around line 256-277: verifyRestoredState currently returns true when neither
steam_api.dll nor steam_api64.dll are present, which falsely signals a restored
state; update verifyRestoredState to track whether any target DLLs were
encountered while walking the directory (use the existing loop that checks
file.name lowercase and references pipeDllHashes and sha256OfFile), and if none
are found at the end of the check, log a warning and return false so the caller
will re-restore the DLLs; keep the existing desync checks intact and only change
the final return behavior based on a "found" flag.
- Around line 2595-2609: The mirror loop only copies direct files and skips
nested directories, causing loss of saves; replace the per-file Files.copy logic
in the remoteDir.listFiles()?.forEach block (and the corresponding block around
lines 2619-2636) with a recursive copy that preserves directory structure and
attributes (e.g., use Kotlin's File.copyRecursively(dst, overwrite = true) or a
Files.walkFileTree-based copier) so subdirectories and their files are mirrored
too; ensure you still skip non-regular entries appropriately, preserve
attributes (equivalent to StandardCopyOption.COPY_ATTRIBUTES), and keep the
Timber.i / Timber.w logging for each copied file or directory so failures are
reported per-item.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/utils/FileUtils.kt`:
- Around line 136-163: Add focused, table-driven unit tests for matchesGlob to
cover anchoring and overlap rules: verify exact-match behavior when pattern
contains no '*' (e.g., "abc" vs "abc" and "ABC"), verify non-overlap for
patterns like "ab*bc" against "abc" and "abxbc", verify trailing-anchor overlap
rejection when prefix consumes into the last token, and test repeated tokens
like "*foo*foo*" and variants; exercise hasLeadingStar/hasTrailingStar paths and
middle-token scanning by calling matchesGlob with these patterns and asserting
true/false expected outcomes.
🪄 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: 14017118-f581-4a2b-9382-53273867a491
📒 Files selected for processing (32)
app/src/main/assets/box86_64/lightsteam.box64rcapp/src/main/assets/box86_64/ultralightsteam.box64rcapp/src/main/java/app/gamenative/PluviaApp.ktapp/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/service/SteamAutoCloud.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.ktapp/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigState.ktapp/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.ktapp/src/main/java/app/gamenative/ui/enums/DialogType.ktapp/src/main/java/app/gamenative/ui/model/MainViewModel.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/app/gamenative/utils/FileUtils.ktapp/src/main/java/app/gamenative/utils/LudusaviRegistry.ktapp/src/main/java/app/gamenative/utils/PreInstallSteps.ktapp/src/main/java/app/gamenative/utils/SteamTokenLogin.ktapp/src/main/java/app/gamenative/utils/SteamUtils.ktapp/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.ktapp/src/main/java/com/winlator/container/Container.javaapp/src/main/java/com/winlator/container/ContainerData.ktapp/src/main/java/com/winlator/contents/AdrenotoolsManager.javaapp/src/main/java/com/winlator/renderer/GLRenderer.javaapp/src/main/java/com/winlator/sysvshm/SysVSharedMemory.javaapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/java/com/winlator/xenvironment/ImageFsInstaller.javaapp/src/main/java/com/winlator/xserver/WindowManager.javaapp/src/main/java/com/winlator/xserver/extensions/DRI3Extension.javaapp/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (14)
- app/src/main/assets/box86_64/ultralightsteam.box64rc
- app/src/main/java/app/gamenative/PrefManager.kt
- app/src/main/assets/box86_64/lightsteam.box64rc
- app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigState.kt
- app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.kt
- app/src/main/java/app/gamenative/utils/PreInstallSteps.kt
- app/src/main/java/app/gamenative/ui/enums/DialogType.kt
- app/src/main/res/values/strings.xml
- app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java
- app/src/main/java/com/winlator/sysvshm/SysVSharedMemory.java
- app/src/main/java/com/winlator/xserver/WindowManager.java
- app/src/main/java/app/gamenative/service/SteamAutoCloud.kt
- app/src/main/java/com/winlator/winhandler/WinHandler.java
- app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
🚧 Files skipped from review as they are similar to previous changes (10)
- app/src/main/java/com/winlator/contents/AdrenotoolsManager.java
- app/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.kt
- app/src/main/java/app/gamenative/ui/model/MainViewModel.kt
- app/src/main/java/com/winlator/container/Container.java
- app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt
- app/src/main/java/com/winlator/container/ContainerData.kt
- app/src/main/java/com/winlator/renderer/GLRenderer.java
- app/src/main/java/app/gamenative/utils/LudusaviRegistry.kt
- app/src/main/java/app/gamenative/service/SteamService.kt
- app/src/main/java/app/gamenative/utils/SteamTokenLogin.kt
Set submodule.app/src/main/cpp/lsfg-vk-android.ignore=dirty in .gitmodules so git/GitHub Desktop stops flagging the parent repo as having uncommitted changes whenever the submodule's working tree is dirty. The dirtiness comes from a chain three levels deep: lsfg-vk-android pulls in thirdparty/pe-parse, which pulls in tests/assets/corkami-poc-dataset. The corkami dataset is a collection of intentionally-weird PE files used to test pe-parse — Windows Defender flags some of them as PUA and quarantines them, leaving the submodule with deleted files. Those files aren't consumed by our build (they're pe-parse's own test fixtures), so hiding the working-tree dirtiness is the right call. The submodule's commit pin is still tracked normally; only the 'this submodule has local modifications' notice is suppressed.
PluviaMain SDK-cloud-bridge dialog - relaunch() now takes a suppressPrompt flag; only set skipBridgePrompt=true when persistence actually succeeded. Previous behavior unconditionally suppressed the prompt for the next launch even when container.saveData() threw and the subdir wasn't actually saved — bridge ended up "off but prompt hidden" instead of either fully on or normal. - Tightened the prompt-fire gate to also skip when preferredSave != None or ignorePendingOperations is true. Those flags come from conflict- resolution dialogs; firing the bridge prompt during a conflict re-entry would drop the user's chosen save / dismiss-pending choice via the relaunch lambda. SteamUtils.writeSharedAppManifest - Added LastOwner to the early-return reuse criteria. Previously, depots/buildId/scripts could all match while the existing acf was attributed to a different signed-in account; reusing it would leave the manifest pointing at a stale LastOwner and break cloud/ownership checks for the current user. Force-rewrite when the owner doesn't match. New helper parseAcfLastOwner() and acfLastOwnerRegex. SteamUtils.autoLoginUserChanges (loginusers.vdf) - Ensure the Steam config dir exists before writeText(). On a fresh container the Wine prefix skeleton is laid down but Steam's own drive_c/Program Files (x86)/Steam/config/ subdir hasn't been populated yet — without mkdirs() the write throws FileNotFoundException and the auto-login fails silently. ContainerUtils.createNewContainer - Replace the open-coded ContainerData(...) construction in the default branch with getDefaultContainerData().copy(drives = drives, dxwrapper = initialDxWrapper). The manual builder was missing several fields that getDefaultContainerData() carries — localSavesOnly, useSteamInput, sharpnessEffect/Level/Denoise, vibrationMode/Intensity, lsfgEnabled — so new containers inherited Kotlin defaults instead of the user's PrefManager defaults. Inheriting through copy() also means future fields don't need to be added in two places. Findings flagged but not changed: - verifyRestoredState true on no DLLs: the function is meant to detect "DLLs are NOT the pipe-DLL" (i.e., they're the original Valve DLLs or absent). Returning false on absent would force re-extraction of pipe DLLs into game folders that don't ship them, which is wrong for non-Steamworks games. - SDK cloud mirror non-recursive (4th time): by design for Dead Cells flat saves; recursive expansion is scope creep. - matchesGlob unit-test nitpick: skipping; no test infra changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/utils/SteamUtils.kt (1)
2605-2657: ⚡ Quick winSDK-cloud mirroring only handles flat directories.
Both
mirrorSdkCloudRemoteToSaveandmirrorSdkCloudSaveToRemoteiterate withlistFiles()and skip non-files, meaning any subdirectory structure underremote/or the save subdir is silently ignored. Games that store saves in nested directories would lose part of their state during the mirror.If flat-directory-only is an intentional design constraint (matching the "single-component only" subdir validation), consider documenting this limitation in the function comments. Otherwise, replacing
listFiles()withwalkTopDown()and preserving relative paths would handle nested structures.♻️ Suggested recursive copy (if nested support is desired)
fun mirrorSdkCloudRemoteToSave(context: Context, appId: Int) { val gameSaveDir = sdkCloudGameSaveDir(context, appId) ?: return val remoteDir = sdkCloudRemoteDir(context, appId) ?: return if (!remoteDir.exists()) { Timber.i("mirrorSdkCloudRemoteToSave: remote/ missing for appId=$appId") return } if (!gameSaveDir.exists()) gameSaveDir.mkdirs() - remoteDir.listFiles()?.forEach { src -> - if (!src.isFile) return@forEach - val dst = File(gameSaveDir, src.name) + remoteDir.walkTopDown().forEach { src -> + if (!src.isFile) return@forEach + val rel = remoteDir.toPath().relativize(src.toPath()) + val dst = gameSaveDir.toPath().resolve(rel).toFile() try { + Files.createDirectories(dst.toPath().parent) Files.copy( src.toPath(), dst.toPath(), StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.COPY_ATTRIBUTES, )🤖 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/SteamUtils.kt` around lines 2605 - 2657, The current mirrorSdkCloudRemoteToSave and mirrorSdkCloudSaveToRemote only copy top-level files (using listFiles()) and thus ignore nested directories; change both to recursively traverse their source directories (use File.walkTopDown() or walk()) and for each visited file preserve its relative path when creating the destination (create intermediate directories with mkdirs before copying). Keep existing filters (skip non-files when copying, the "backup-*.zip" rule in mirrorSdkCloudSaveToRemote), and retain copy options (REPLACE_EXISTING, COPY_ATTRIBUTES) and logging/error handling, but compute dst paths by resolving the file's path relative to the source root so nested directories are mirrored correctly.
🤖 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 795-850: The dialog dismissal logic is reopening the prompt
because relaunch(false) keeps skipBridgePrompt=false; change the call sites so
the current relaunch always suppresses the prompt regardless of whether
persistence succeeded: in the onConfirmClick continuation (after computing
persisted), call relaunch(true) for the immediate launch; in onDismissClick,
onActionClick (after attempting to persist the "sdkCloudBridgePromptDismissed"
flag), and onDismissRequest call relaunch(true). Keep the existing
persistence/write-to-container code (ContainerUtils.getContainer,
container.saveData(), etc.) unchanged so future launches still respect persisted
fields, but always pass true into the relaunch lambda for the current attempt.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/utils/SteamUtils.kt`:
- Around line 2605-2657: The current mirrorSdkCloudRemoteToSave and
mirrorSdkCloudSaveToRemote only copy top-level files (using listFiles()) and
thus ignore nested directories; change both to recursively traverse their source
directories (use File.walkTopDown() or walk()) and for each visited file
preserve its relative path when creating the destination (create intermediate
directories with mkdirs before copying). Keep existing filters (skip non-files
when copying, the "backup-*.zip" rule in mirrorSdkCloudSaveToRemote), and retain
copy options (REPLACE_EXISTING, COPY_ATTRIBUTES) and logging/error handling, but
compute dst paths by resolving the file's path relative to the source root so
nested directories are mirrored correctly.
🪄 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: 5b0d164b-b580-4b1a-9437-d0c559faa68e
📒 Files selected for processing (4)
.gitmodulesapp/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/app/gamenative/utils/SteamUtils.kt
✅ Files skipped from review due to trivial changes (1)
- .gitmodules
Contributor
There was a problem hiding this comment.
7 issues found across 33 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/utils/LudusaviRegistry.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/LudusaviRegistry.kt:100">
P2: Fetched manifest data is cached without validating that the parser produced any usable entries, so a silent parse drift can poison both disk and memory caches with an empty dataset.</violation>
</file>
<file name="app/src/main/java/com/winlator/xserver/WindowManager.java">
<violation number="1" location="app/src/main/java/com/winlator/xserver/WindowManager.java:193">
P1: Heuristic leak cleanup can destroy legitimate unmapped windows from the same client because it uses only broad attributes (empty WM_CLASS, same size, unmapped) instead of a proven leak marker.</violation>
</file>
<file name="app/src/main/java/app/gamenative/utils/SteamUtils.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/SteamUtils.kt:256">
P1: `verifyRestoredState()` returns true even when no Steam API DLL is present, so a stale RESTORED marker can skip `putBackSteamDlls()` and leave the app directory without required DLLs.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/PluviaMain.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/PluviaMain.kt:833">
P1: Skip/dismiss relaunches preLaunchApp without suppressing the prompt, causing the same SDK cloud bridge dialog to reappear instead of continuing the launch.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/SteamService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamService.kt:2441">
P1: Real-Steam cross-device pending ops are detected but never propagated into `PendingOperations`, so the conflict UI cannot run.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/SteamAutoCloud.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamAutoCloud.kt:239">
P2: Skipping the remote-cache rewrite when the SteamUserData set is empty leaves stale `remotecache.vdf` data behind.</violation>
</file>
<file name="app/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.kt:60">
P1: Container-wide VC redist marker short-circuits later games and can skip needed per-game redistributable installs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Resolves PR utkarshdalal#1347 conflicts after upstream picked up: - 0592ae0: refactored Wine process snapshot into WineProcessSnapshotHelper - 45c9f0d: extracted hasStoredCredentials, awaitSteamLogin XServerScreen.kt: kept this PR's requestWineProcessSnapshot/isSteamExeAlive/ awaitSteamShutdown helpers (still used by graceful Steam exit at line 1303); upstream's QuickMenu refactor to WineProcessSnapshotHelper.readFromProc and onEndWineProcess callback merged cleanly. SteamUtils.kt: kept per-container .wine prefix routing on autoLoginUserChanges and setupLightweightSteamConfig (avoids xuser-symlink races during concurrent launches), and adopted upstream's offline steamId64 fallback chain plus the Steam3 accountID fix for userdata path (matches restoreSteamApi). PluviaMain.kt: dropped duplicate SteamUtils import auto-merge produced. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (3)
1291-1321:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a non-cancellation fallback so first-tap exit can’t become a no-op.
Only
CancellationExceptionis handled. IfwinHandler.exec(...)orawaitSteamShutdown(...)throws any other exception,exit(...)is skipped.Suggested fix
gracefulExitJob = CoroutineScope(Dispatchers.Main).launch { try { if (shutdownSteam) { @@ } exit(winHandler, frameRating, currentAppInfo, container, appId, onExit, navigateBack) - } catch (_: kotlinx.coroutines.CancellationException) { - // Force-quit path already called exit(). + } catch (t: Throwable) { + if (t is kotlinx.coroutines.CancellationException) { + // Force-quit path already called exit(). + throw t + } + Timber.w(t, "Graceful exit failed, falling back to immediate exit") + exit(winHandler, frameRating, currentAppInfo, container, appId, onExit, navigateBack) } finally { steamShutdownDialogResolver?.let { if (!it.isCompleted) it.cancel() } steamShutdownDialogResolver = null } }🤖 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/xserver/XServerScreen.kt` around lines 1291 - 1321, The coroutine launched into gracefulExitJob currently only catches CancellationException so any other exception from winHandler.exec(...) or awaitSteamShutdown(...) will skip calling exit(...); modify the try/catch/finally in the CoroutineScope launched block (gracefulExitJob) so that non-CancellationException errors are caught (e.g., catch Throwable or catch Exception) and still ensure exit(winHandler, frameRating, currentAppInfo, container, appId, onExit, navigateBack) is invoked (either by moving that call into a finally block or by calling it from the non-cancellation catch), and still preserve the existing CancellationException behavior and cleanup of steamShutdownDialogResolver.
3245-3247:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReapply
WINEPREFIXafter mergingcontainer.envVars.
envVars.putAll(container.envVars)can override the per-container prefix set earlier (Line 3135), which undermines container isolation when persisted env containsWINEPREFIX.Suggested fix
envVars.putAll(container.envVars) + envVars.put("WINEPREFIX", imageFs.wineprefix) if (!envVars.has("WINEESYNC")) envVars.put("WINEESYNC", "1")🤖 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/xserver/XServerScreen.kt` around lines 3245 - 3247, The merge using envVars.putAll(container.envVars) can overwrite the per-container WINEPREFIX set earlier; after calling envVars.putAll(container.envVars) reapply the intended per-container prefix by setting envVars.put("WINEPREFIX", <the-per-container-prefix-variable>) if that prefix variable is non-null (or otherwise ensure the originally established WINEPREFIX value is preserved), and then keep the existing WINEESYNC check (i.e., ensure envVars.has("WINEESYNC") still results in envVars.put("WINEESYNC","1") when absent); reference envVars, container.envVars, and WINEPREFIX to locate where to reapply the prefix.
313-353:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftProcess snapshot polling still races by swapping a shared
OnGetProcessInfoListener.
awaitSteamShutdown()still installs/removes a global listener every poll, which can clobberstartExitWatchForUnmappedGameWindow()(same listener slot) and make shutdown detection nondeterministic.Also applies to: 360-382
🤖 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/xserver/XServerScreen.kt` around lines 313 - 353, The listener swap in requestWineProcessSnapshot (using winHandler.setOnGetProcessInfoListener / getOnGetProcessInfoListener) races with other code that uses the same global slot (e.g., awaitSteamShutdown and startExitWatchForUnmappedGameWindow); fix by avoiding install/remove of the global listener on each poll—either add support for multiple listeners on WinHandler (addOnGetProcessInfoListener/removeOnGetProcessInfoListener) or, if you cannot change WinHandler, install a single persistent composite listener once that forwards events to registered snapshot collectors; update requestWineProcessSnapshot to register itself with that multi-listener mechanism (or register with the composite) instead of replacing previousListener, and remove only its own registration when done so it no longer clobbers other watchers.app/src/main/java/app/gamenative/ui/PluviaMain.kt (1)
800-855:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip/dismiss still traps the user in the same bridge prompt.
relaunch(false)keepsskipBridgePrompt = false, so the nextpreLaunchApp()call immediately re-hits the gate at Line 1655 and shows this dialog again. That makes Skip / outside-dismiss non-functional, and Enable / Don't ask again loop too whenever persistence fails. Suppress the prompt for the current relaunch attempt regardless of persistence; let persistence control only future launches.Possible fix
- val relaunch = { suppressPrompt: Boolean -> + val relaunch = { preLaunchApp( context = context, appId = state.launchedAppId, - // Only suppress the prompt next time when we actually persisted a subdir - // (or the user picked "Don't ask again"). If persistence failed, leave - // the prompt enabled so it fires again on the next attempt rather than - // silently disabling itself in memory. - skipBridgePrompt = suppressPrompt, + // Suppress only this immediate relaunch. Future launches should still be + // controlled by the persisted container fields. + skipBridgePrompt = true, setLoadingDialogVisible = viewModel::setLoadingDialogVisible, setLoadingProgress = viewModel::setLoadingDialogProgress, setLoadingMessage = viewModel::setLoadingDialogMessage, setMessageDialogState = setMessageDialogState, onSuccess = viewModel::launchApp, isOffline = viewModel.isOffline.value, ) } @@ - withContext(Dispatchers.Main) { relaunch(persisted) } + withContext(Dispatchers.Main) { relaunch() } @@ - relaunch(false) + relaunch() @@ - withContext(Dispatchers.Main) { relaunch(persisted) } + withContext(Dispatchers.Main) { relaunch() } @@ - relaunch(false) + relaunch()🤖 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 800 - 855, The bridge-prompt suppression flag is being used for the current relaunch instead of only for future launches: change calls to relaunch so the current run always suppresses the prompt (pass true) while keeping persistence logic unchanged to control future behavior; specifically, update onDismissClick and onDismissRequest to call relaunch(true) and change the withContext(...) callbacks in onConfirmClick and onActionClick to call relaunch(true) (leave ContainerUtils.getContainer/putExtra/saveData and the persisted boolean logic as-is so persistence still determines future suppression).
🤖 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/ui/PluviaMain.kt`:
- Around line 800-855: The bridge-prompt suppression flag is being used for the
current relaunch instead of only for future launches: change calls to relaunch
so the current run always suppresses the prompt (pass true) while keeping
persistence logic unchanged to control future behavior; specifically, update
onDismissClick and onDismissRequest to call relaunch(true) and change the
withContext(...) callbacks in onConfirmClick and onActionClick to call
relaunch(true) (leave ContainerUtils.getContainer/putExtra/saveData and the
persisted boolean logic as-is so persistence still determines future
suppression).
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1291-1321: The coroutine launched into gracefulExitJob currently
only catches CancellationException so any other exception from
winHandler.exec(...) or awaitSteamShutdown(...) will skip calling exit(...);
modify the try/catch/finally in the CoroutineScope launched block
(gracefulExitJob) so that non-CancellationException errors are caught (e.g.,
catch Throwable or catch Exception) and still ensure exit(winHandler,
frameRating, currentAppInfo, container, appId, onExit, navigateBack) is invoked
(either by moving that call into a finally block or by calling it from the
non-cancellation catch), and still preserve the existing CancellationException
behavior and cleanup of steamShutdownDialogResolver.
- Around line 3245-3247: The merge using envVars.putAll(container.envVars) can
overwrite the per-container WINEPREFIX set earlier; after calling
envVars.putAll(container.envVars) reapply the intended per-container prefix by
setting envVars.put("WINEPREFIX", <the-per-container-prefix-variable>) if that
prefix variable is non-null (or otherwise ensure the originally established
WINEPREFIX value is preserved), and then keep the existing WINEESYNC check
(i.e., ensure envVars.has("WINEESYNC") still results in
envVars.put("WINEESYNC","1") when absent); reference envVars, container.envVars,
and WINEPREFIX to locate where to reapply the prefix.
- Around line 313-353: The listener swap in requestWineProcessSnapshot (using
winHandler.setOnGetProcessInfoListener / getOnGetProcessInfoListener) races with
other code that uses the same global slot (e.g., awaitSteamShutdown and
startExitWatchForUnmappedGameWindow); fix by avoiding install/remove of the
global listener on each poll—either add support for multiple listeners on
WinHandler (addOnGetProcessInfoListener/removeOnGetProcessInfoListener) or, if
you cannot change WinHandler, install a single persistent composite listener
once that forwards events to registered snapshot collectors; update
requestWineProcessSnapshot to register itself with that multi-listener mechanism
(or register with the composite) instead of replacing previousListener, and
remove only its own registration when done so it no longer clobbers other
watchers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3dadb8a0-9c03-447c-b049-973490e42cc6
📒 Files selected for processing (5)
app/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/SteamUtils.ktapp/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.kt
- app/src/main/java/app/gamenative/utils/SteamUtils.kt
- app/src/main/java/app/gamenative/service/SteamService.kt
LudusaviRegistry: refuse to cache an empty parse result; fall through to the stale-disk fallback instead. Guards against silent schema drift in the upstream manifest poisoning both caches with a no-data result for CACHE_TTL_MS. WindowManager.reapLeakedClientWindows: tighten the orphan-chain predicate to match the compositor's signature from dd3987b (blank WM_CLASS, _NET_WM_PID==0, no WM_NAME, no children, parent is non-root with blank className+pid==0). The prior heuristic could destroy a legitimate unmapped popup from the same client that happened to share size and lack WM_CLASS at create time. SteamUtils.verifyRestoredState: fail closed when no steam_api*.dll is present under appDir. Mirrors the existing verifyReplacedState `found` guard so a stale RESTORED marker on a relocated/wiped game dir lets putBackSteamDlls re-attempt the restore on next launch. SteamService.beginLaunchApp: propagate detected real-Steam cross-device pending ops into PostSyncInfo(SyncResult.PendingOperations, …) so the conflict dialog in PluviaMain actually fires. Same-device phantom auto-dismiss is unchanged; the kickPlayingSession call still runs before the new bail. SteamAutoCloud.writeRemoteCache: rewrite remotecache.vdf even when the SteamUserData entry list is empty so a wipe of local saves truncates the manifest. Previously skipped, leaving stale entries on disk. VcRedistStep: replace the container-wide .vcredist_installed marker with per-year sidecars (_2005…_2022 plus _legacy). appliesTo only short-circuits when every required year is already covered; buildCommand skips already- installed years; one-shot legacy-marker migration converts the old marker on first encounter. Tests rewritten. PluviaMain SDK bridge prompt: every dialog handler now calls relaunch(true) so the current relaunch always suppresses; persistence (sdkCloudSaveSubdir, sdkCloudBridgePromptDismissed) still gates future launches via the preLaunchApp gate. The previous relaunch(persisted) / relaunch(false) calls re-fired the dialog when persistence failed or on Skip/Dismiss. XServerScreen graceful Steam exit: fall through to exit() on non-cancellation throwables from winHandler.exec()/awaitSteamShutdown() instead of leaving the game process stuck. XServerScreen env merge: re-pin envVars["WINEPREFIX"] = imageFs.wineprefix after envVars.putAll(container.envVars) so a stale WINEPREFIX baked into container.envVars doesn't redirect Wine to the wrong prefix. WinHandler + XServerScreen process-info listener: add multi-listener support (addOnGetProcessInfoListener / removeOnGetProcessInfoListener, backed by a CopyOnWriteArrayList) and migrate requestWineProcessSnapshot and startExitWatchForUnmappedGameWindow off the single-slot setter so concurrent shutdown polling and unmapped-window watching no longer trample each other's listener registration. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
4 issues found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.kt:206">
P1: VC redistributable markers are too coarse: x86 and x64 installers for the same year share one key, so installing one architecture can incorrectly skip the other.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.kt:211">
P1: All yearless VC redist payloads are collapsed to a single `legacy` marker, so installing one can incorrectly suppress other distinct x86/x64 redistributables.</violation>
</file>
<file name="app/src/main/java/com/winlator/winhandler/WinHandler.java">
<violation number="1" location="app/src/main/java/com/winlator/winhandler/WinHandler.java:436">
P1: Process-info listener callbacks are not exception-isolated; a bad listener can terminate the worker thread and break future polling.</violation>
</file>
<file name="app/src/main/java/app/gamenative/utils/LudusaviRegistry.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/LudusaviRegistry.kt:89">
P2: Stale disk-cache fallback still accepts an empty parsed map and repopulates `memoryCache`, so a poisoned `{}` cache can keep the registry empty despite the new empty-fetch guard.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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/com/winlator/winhandler/WinHandler.java (1)
199-207:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the single-string
exec(...)overload compatible with quoted paths.
split(" ", 2)turns a valid command like"C:\Program Files (x86)\Steam\steam.exe" -shutdowninto the wrong filename, so the legacy overload no longer works for executables whose path contains spaces. Since this overload is explicitly retained for backwards compatibility, it needs quoted-path parsing instead of a raw split.Possible fix
public void exec(String command) { + if (command == null) return; String command2 = command.trim(); if (command2.isEmpty()) { return; } - String[] cmdList = command2.split(" ", 2); - final String filename = cmdList[0]; - final String parameters = cmdList.length > 1 ? cmdList[1] : ""; + final String filename; + final String parameters; + if (command2.startsWith("\"")) { + int endQuote = command2.indexOf('"', 1); + if (endQuote <= 1) return; + filename = command2.substring(1, endQuote); + parameters = command2.substring(endQuote + 1).trim(); + } else { + String[] cmdList = command2.split(" ", 2); + filename = cmdList[0]; + parameters = cmdList.length > 1 ? cmdList[1] : ""; + } exec(filename, parameters); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 199 - 207, The single-argument exec(String command) incorrectly splits paths with spaces because it uses split(" ", 2); instead, update exec(String command) to handle quoted paths: trim the command, if it starts with a double quote (") find the matching closing quote and treat the quoted substring as filename and the rest (after optional whitespace) as parameters; otherwise split on the first whitespace as before. Keep the same variable names (command2, filename, parameters) and call exec(filename, parameters) unchanged so backwards compatibility is preserved.
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
313-355:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftConcurrent
listProcesses()polls still race on one uncorrelated event stream.The multi-listener change fixes slot clobbering, but
app/src/main/java/com/winlator/winhandler/WinHandler.java:419-440still broadcasts everyGET_PROCESSpacket to every listener without a request id. IfawaitSteamShutdown()andstartExitWatchForUnmappedGameWindow()poll at the same time, each listener resets on anyindex == 0packet and can complete from the other caller’s snapshot. Serialize snapshot collection perWinHandler, or add a request token to the wire format before allowing concurrent polls.🤖 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/xserver/XServerScreen.kt` around lines 313 - 355, requestWineProcessSnapshot races with other concurrent polls because WinHandler.listProcesses() broadcasts GET_PROCESS packets to all listeners with no request id; fix by serializing snapshot collection per WinHandler or by adding a request token to the wire protocol and plumbing it through listProcesses()/OnGetProcessInfoListener. Concretely: either (A) add a single-call mutex/lock field on WinHandler and have requestWineProcessSnapshot acquire it around addOnGetProcessInfoListener/listProcesses/removeOnGetProcessInfoListener to prevent concurrent polls (affecting functions requestWineProcessSnapshot, awaitSteamShutdown, startExitWatchForUnmappedGameWindow), or (B) extend WinHandler.listProcesses() to accept a requestId, include that ID in the GET_PROCESS packet from WinHandler.java broadcast logic and in the OnGetProcessInfoListener callback signature, then have requestWineProcessSnapshot provide a unique requestId and ignore events with mismatched IDs so multiple listeners can coexist safely.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/PluviaMain.kt (1)
824-829: 💤 Low valueUnused
persistedvariable.The
persistedvariable is computed but never read. While the comment explains the intent (persistence controls future launches), the variable itself is dead code. Consider either removing it or logging/acting on persistence failures more explicitly.♻️ Optional cleanup
onConfirmClick = { // Enable: write Ludusavi's suggested subdir, then continue the launch. msgDialogState = MessageDialogState(false) CoroutineScope(Dispatchers.IO).launch { val gameId = ContainerUtils.extractGameIdFromContainerId(state.launchedAppId) val rec = runCatching { SteamUtils.getRecommendedSdkCloudSaveSubdirAsync(context, gameId) }.getOrNull() - val persisted = rec != null && runCatching { + if (rec != null) { + runCatching { - val container = ContainerUtils.getContainer(context, state.launchedAppId) - container.sdkCloudSaveSubdir = rec.subdir - container.saveData() - }.onFailure { Timber.w(it, "Failed to persist sdkCloudSaveSubdir=${rec.subdir}") } - .isSuccess + val container = ContainerUtils.getContainer(context, state.launchedAppId) + container.sdkCloudSaveSubdir = rec.subdir + container.saveData() + }.onFailure { Timber.w(it, "Failed to persist sdkCloudSaveSubdir=${rec.subdir}") } + } withContext(Dispatchers.Main) { relaunch(true) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/PluviaMain.kt` around lines 824 - 829, The local variable persisted is computed but never used; remove the dead variable and either directly run the runCatching block for its side effects or act on the result (e.g., log or handle failure). Locate the block that calls ContainerUtils.getContainer(context, state.launchedAppId), sets container.sdkCloudSaveSubdir = rec.subdir and calls container.saveData(); replace the assignment to persisted with a direct runCatching { ... }.onFailure { Timber.w(it, "Failed to persist sdkCloudSaveSubdir=${rec.subdir}") } (or capture the boolean and explicitly log/handle when false) so there is no unused variable left.
🤖 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/xserver/XServerScreen.kt`:
- Around line 323-327: The current OnGetProcessInfoListener treats (count == 0
&& processInfo == null) as a successful empty snapshot and completes deferred
with emptyList(), which causes isSteamExeAlive() to return false on a
request/send failure; instead, detect this exact failure case in the listener
(the block inside synchronized(lock) that currently completes deferred with
emptyList()), do NOT complete deferred with emptyList(), and either
completeDeferred exceptionally (e.g.,
deferred.completeExceptionally(ProcessSnapshotException("request/send
failure"))) or simply log the failure and leave deferred uncompleted so the
caller can distinguish a request failure from an actual empty process list;
update references to listener/OnGetProcessInfoListener/lock/deferred and ensure
callers of isSteamExeAlive() handle the exceptional/failure path accordingly.
In `@app/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.kt`:
- Around line 153-217: requiredVersions() and versionKey() collapse x86/x64 into
the same year key, so add an architecture tag to the key and marker names to
avoid x64 installers being skipped after an x86 marker exists: change
versionKey(winPath: String) to detect architecture (e.g., look for "x64",
"amd64", "x86", "wow64", "x86_64", or explicit filenames like
"vc_redist.x64.exe"/"vc_redist.x86.exe") and return a string like
"$year-x64"/"$year-x86" (or "legacy-x64"/"legacy-x86" for non-yeared paths);
ensure requiredVersions() uses this new versionKey result when building the
marker name (used by recordInstalledVersions()) so markers are per-year-per-arch
rather than per-year-only.
- Around line 90-92: The current return short-circuits on
MarkerUtils.hasMarker(gameDirPath, Marker.VCREDIST_INSTALLED) even when
`missing` is non-empty, hiding newly missing VC runtimes; update the logic so
that if `missing` is not empty and `container.rootDir` is available you verify
per-year/container-level VCREDIST markers (use the container root markers as the
source of truth) instead of the coarse game-dir marker — only fall back to
Marker.VCREDIST_INSTALLED on the gameDirPath when `container.rootDir` is null or
when no per-year markers exist; adjust the return in the function containing
`missing`, `container.rootDir`, and the current MarkerUtils.hasMarker call to
reflect this conditional check.
In `@app/src/main/java/com/winlator/xserver/WindowManager.java`:
- Around line 225-237: Add a parent-size guard to the orphan reaper checks:
after obtaining candParent in the WindowManager orphan-reap logic (the block
that checks cand.getProcessId(), cand.getName(), cand.getChildren(), candParent
== null/rootWindow, candParent.getClassName(), candParent.getProcessId()),
verify candParent's size is exactly 1x1 (e.g., check width and height == 1 via
the parent window size accessors) and skip reaping unless that predicate holds;
place this check immediately after the candParent null/root check so the reaper
only targets children under a 1x1 blank-class/pid=0 orphanage.
---
Outside diff comments:
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 199-207: The single-argument exec(String command) incorrectly
splits paths with spaces because it uses split(" ", 2); instead, update
exec(String command) to handle quoted paths: trim the command, if it starts with
a double quote (") find the matching closing quote and treat the quoted
substring as filename and the rest (after optional whitespace) as parameters;
otherwise split on the first whitespace as before. Keep the same variable names
(command2, filename, parameters) and call exec(filename, parameters) unchanged
so backwards compatibility is preserved.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 313-355: requestWineProcessSnapshot races with other concurrent
polls because WinHandler.listProcesses() broadcasts GET_PROCESS packets to all
listeners with no request id; fix by serializing snapshot collection per
WinHandler or by adding a request token to the wire protocol and plumbing it
through listProcesses()/OnGetProcessInfoListener. Concretely: either (A) add a
single-call mutex/lock field on WinHandler and have requestWineProcessSnapshot
acquire it around
addOnGetProcessInfoListener/listProcesses/removeOnGetProcessInfoListener to
prevent concurrent polls (affecting functions requestWineProcessSnapshot,
awaitSteamShutdown, startExitWatchForUnmappedGameWindow), or (B) extend
WinHandler.listProcesses() to accept a requestId, include that ID in the
GET_PROCESS packet from WinHandler.java broadcast logic and in the
OnGetProcessInfoListener callback signature, then have
requestWineProcessSnapshot provide a unique requestId and ignore events with
mismatched IDs so multiple listeners can coexist safely.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/PluviaMain.kt`:
- Around line 824-829: The local variable persisted is computed but never used;
remove the dead variable and either directly run the runCatching block for its
side effects or act on the result (e.g., log or handle failure). Locate the
block that calls ContainerUtils.getContainer(context, state.launchedAppId), sets
container.sdkCloudSaveSubdir = rec.subdir and calls container.saveData();
replace the assignment to persisted with a direct runCatching { ... }.onFailure
{ Timber.w(it, "Failed to persist sdkCloudSaveSubdir=${rec.subdir}") } (or
capture the boolean and explicitly log/handle when false) so there is no unused
variable left.
🪄 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: be142422-4034-4bab-adb6-487dd2d0222f
📒 Files selected for processing (11)
app/src/main/java/app/gamenative/service/SteamAutoCloud.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/LudusaviRegistry.ktapp/src/main/java/app/gamenative/utils/PreInstallSteps.ktapp/src/main/java/app/gamenative/utils/SteamUtils.ktapp/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.ktapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/java/com/winlator/xserver/WindowManager.javaapp/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/app/gamenative/utils/LudusaviRegistry.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/app/gamenative/service/SteamAutoCloud.kt
- app/src/main/java/app/gamenative/utils/SteamUtils.kt
- app/src/main/java/app/gamenative/service/SteamService.kt
VcRedistStep: per-year markers now also encode architecture (`$year-x86` /
`$year-x64`, plus `legacy-x86` / `legacy-x64` for yearless paths). Without
this, an x86-only install (e.g. vc_redist.x86.exe for 2015) wrote a year-only
marker that incorrectly suppressed the matching x64 install on a later game.
Yearless redists (A:\redist\vcredist_*, A:\_CommonRedist\VC_redist.*) are
similarly distinguished. archKey() inspects the win path for x64/amd64/wow64/
x86_64 hints (and explicit .x64./.x86. filenames) and defaults to x86 for
ambiguous paths. Round-5 archless markers are migrated as `_$year-x86` only —
conservative: any actual x64 install will re-run once. appliesTo no longer
short-circuits on the coarse game-dir VCREDIST_INSTALLED marker when per-year/
arch markers indicate a missing (year, arch) pair; the game-dir marker is now
only a fallback when no per-year markers exist or containerRoot is null.
WinHandler.GET_PROCESS dispatch: each listener invocation (slot listener and
each multi-listener entry) is now wrapped in try/catch so one bad listener
can't kill the worker thread. Same fix applied to the listProcesses() send-
failure broadcast.
WinHandler.exec(String): handle quoted paths. A leading `"` triggers quoted-
path parsing (filename = quoted substring; parameters = trimmed remainder);
unquoted commands still split on the first whitespace. Without this,
"C:\Program Files (x86)\Steam\steam.exe" -shutdown shredded into a bogus
filename of "C:\Program".
XServerScreen.requestWineProcessSnapshot: distinguish request/send failure
from a legitimate empty process list. WinHandler.listProcesses() emits
(0, 0, null) only when sendPacket() fails — Wine itself never broadcasts
empty. Added ProcessSnapshotException, listener completes deferred
exceptionally on (count==0 && info==null), await maps it to null so
isSteamExeAlive(null) == true keeps the polling loop alive instead of
falsely concluding Steam exited.
WinHandler + XServerScreen: add a snapshot-collection mutex
(processSnapshotMutex). Both Kotlin pollers (requestWineProcessSnapshot and
startExitWatchForUnmappedGameWindow) now serialize their add-listener →
listProcesses → await → remove-listener sequence so concurrent listProcesses
calls don't cross-pollute each other's deferreds. Multi-listener support is
preserved for long-running watchers; only the per-iteration snapshot is
serialized.
LudusaviRegistry.ensureLoaded: extend the empty-set guard to the disk-cache
fallback. A poisoned `{}` cache file (e.g., one written before the round-5
empty-fetch guard shipped) used to populate memoryCache with emptyMap(),
after which the fast path short-circuited forever. Now: if the parsed disk
cache is empty, log and delete the poisoned file (best-effort), fall through
to null so the next ensureLoaded retries the fetch. Parser exceptions still
leave the file alone.
WindowManager.reapLeakedClientWindows: add the 1x1 parent-size predicate to
match the compositor's full orphan-chain marker (dd3987b). Without it, a
legitimate window whose parent happens to be blank-class+pid==0 but is NOT
a 1x1 orphanage stub could still be reaped.
PluviaMain SDK bridge prompt: drop the dead `persisted` boolean computed in
onConfirmClick / onActionClick (round-5 changed every relaunch(...) to
relaunch(true), so the Boolean is no longer read). Side effects and the
existing onFailure Timber.w log are preserved.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.kt:249">
P2: Legacy marker migration can drop install-state if replacement creation fails, because the old marker is deleted unconditionally.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (!migrated.exists()) { | ||
| runCatching { migrated.createNewFile() } | ||
| } | ||
| runCatching { f.delete() } |
Contributor
There was a problem hiding this comment.
P2: Legacy marker migration can drop install-state if replacement creation fails, because the old marker is deleted unconditionally.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.kt, line 249:
<comment>Legacy marker migration can drop install-state if replacement creation fails, because the old marker is deleted unconditionally.</comment>
<file context>
@@ -192,23 +199,55 @@ object VcRedistStep : PreInstallStep {
+ if (!migrated.exists()) {
+ runCatching { migrated.createNewFile() }
}
+ runCatching { f.delete() }
}
- // Generic / yearless installers — track under a shared key so they
</file context>
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 (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
846-896:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIgnore
steam.exein the unmapped-window exit watcher for real-Steam sessions.On real-Steam runs, Line 893 still treats
steam.exeas a non-essential process. Once the game window closes normally, the snapshot can legitimately contain only Steam, so this watcher never reachesexit(...)and the session can sit on a black screen until timeout/manual intervention. Addsteamto the allowlist whencontainer.isLaunchRealSteam, or special-case it in the predicate.Suggested fix
- val allowlist = buildEssentialProcessAllowlist() + val allowlist = buildEssentialProcessAllowlist().let { base -> + if (container.isLaunchRealSteam) base + "steam" else base + } @@ - val hasNonEssential = snapshot.any { - !allowlist.contains(normalizeProcessName(it.name)) - } + val hasNonEssential = snapshot.any { + !allowlist.contains(normalizeProcessName(it.name)) + }🤖 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/xserver/XServerScreen.kt` around lines 846 - 896, The exit watcher treats "steam.exe" as non-essential causing real-Steam sessions to hang; update the allowlist logic used in the snapshot check so that when container.isLaunchRealSteam is true "steam" (normalized via normalizeProcessName) is considered essential or special-case the predicate computing hasNonEssential to ignore "steam" for real-Steam launches; modify the code around allowlist, the snapshot any { ... } check (where hasNonEssential is computed), and use container.isLaunchRealSteam to include "steam" in the allowlist or bypass it in that predicate so the watcher will call exit(...) when only Steam remains.
🧹 Nitpick comments (2)
app/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt (2)
39-59: 💤 Low value
appliesTo_returnsFalse_whenGameDirMarkerExistsimplicitly exercises the legacy fallback — consider a brief inline commentWith
containerRootnow always non-null (wired in@Before), this test exercises the legacy game-dir marker path only becauseinstalledhappens to be empty (no.vcredist_installed_*files incontainerRoot). A future reader may not immediately see why the game-dir marker still "wins" here when the production code's primary branch checks container markers. A one-liner comment makes the intent explicit.📝 Suggested clarification comment
`@Test` fun appliesTo_returnsFalse_whenGameDirMarkerExists() { + // No container-level per-year markers exist, so the legacy game-dir marker + // fallback applies and short-circuits the step. seedInstaller() MarkerUtils.addMarker(gameDir.absolutePath, Marker.VCREDIST_INSTALLED) val applies = VcRedistStep.appliesTo(container, GameSource.STEAM, gameDir.absolutePath) assertFalse(applies) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt` around lines 39 - 59, The test appliesTo_returnsFalse_whenGameDirMarkerExists unintentionally exercises the legacy game-dir marker branch of VcRedistStep.appliesTo because containerRoot is non-null in the `@Before` setup and there are no .vcredist_installed_* files in the seeded container; add a one-line inline comment above the MarkerUtils.addMarker call (or at the start of the test) noting that this intentionally verifies the legacy game-dir marker fallback (Marker.VCREDIST_INSTALLED) path even though container markers are normally preferred by VcRedistStep.appliesTo so future readers understand why the game-dir marker "wins".
193-203: 💤 Low value
recordInstalledVersions_writesPerYearAndArchMarkersonly checks positive marker creation — no guard against spurious extra filesThe test verifies the three expected markers are created, but doesn't assert that no extra (e.g., stale or duplicate) marker files appear in
containerRoot. Adding a count orlistFiles()check would catch a regression whererecordInstalledVersionswrites unexpected markers.📝 Suggested negative assertion
assertTrue(File(containerRoot, ".vcredist_installed_2017-x86").isFile) assertTrue(File(containerRoot, ".vcredist_installed_2017-x64").isFile) assertTrue(File(containerRoot, ".vcredist_installed_2022-x86").isFile) + // Confirm no spurious extra marker files were written. + val markers = containerRoot.listFiles { f -> f.name.startsWith(".vcredist_installed_") } + assertEquals(3, markers?.size) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt` around lines 193 - 203, The test recordInstalledVersions_writesPerYearAndArchMarkers currently only asserts the expected three marker files exist but doesn't guard against extra markers; update the test to also assert that containerRoot contains exactly three vcredist marker files by listing files whose names start with ".vcredist_installed_" (use containerRoot.listFiles / Kotlin equivalent) and asserting the resulting array/list size equals 3 after calling VcRedistStep.recordInstalledVersions, keeping the existing positive assertions (method: recordInstalledVersions_writesPerYearAndArchMarkers, subject: VcRedistStep.recordInstalledVersions, variable: containerRoot).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 846-896: The exit watcher treats "steam.exe" as non-essential
causing real-Steam sessions to hang; update the allowlist logic used in the
snapshot check so that when container.isLaunchRealSteam is true "steam"
(normalized via normalizeProcessName) is considered essential or special-case
the predicate computing hasNonEssential to ignore "steam" for real-Steam
launches; modify the code around allowlist, the snapshot any { ... } check
(where hasNonEssential is computed), and use container.isLaunchRealSteam to
include "steam" in the allowlist or bypass it in that predicate so the watcher
will call exit(...) when only Steam remains.
---
Nitpick comments:
In `@app/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt`:
- Around line 39-59: The test appliesTo_returnsFalse_whenGameDirMarkerExists
unintentionally exercises the legacy game-dir marker branch of
VcRedistStep.appliesTo because containerRoot is non-null in the `@Before` setup
and there are no .vcredist_installed_* files in the seeded container; add a
one-line inline comment above the MarkerUtils.addMarker call (or at the start of
the test) noting that this intentionally verifies the legacy game-dir marker
fallback (Marker.VCREDIST_INSTALLED) path even though container markers are
normally preferred by VcRedistStep.appliesTo so future readers understand why
the game-dir marker "wins".
- Around line 193-203: The test
recordInstalledVersions_writesPerYearAndArchMarkers currently only asserts the
expected three marker files exist but doesn't guard against extra markers;
update the test to also assert that containerRoot contains exactly three
vcredist marker files by listing files whose names start with
".vcredist_installed_" (use containerRoot.listFiles / Kotlin equivalent) and
asserting the resulting array/list size equals 3 after calling
VcRedistStep.recordInstalledVersions, keeping the existing positive assertions
(method: recordInstalledVersions_writesPerYearAndArchMarkers, subject:
VcRedistStep.recordInstalledVersions, variable: containerRoot).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 125309c3-577b-47a7-b18d-7c245ef384cb
📒 Files selected for processing (7)
app/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/LudusaviRegistry.ktapp/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.ktapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/java/com/winlator/xserver/WindowManager.javaapp/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt
✅ Files skipped from review due to trivial changes (2)
- app/src/main/java/app/gamenative/utils/LudusaviRegistry.kt
- app/src/main/java/app/gamenative/ui/PluviaMain.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/app/gamenative/utils/preInstallSteps/VcRedistStep.kt
- app/src/main/java/com/winlator/xserver/WindowManager.java
Author
|
@utkarshdalal Found a nasty bug. Seems achievements aren't working with Launch Steam Client. Working on it. |
…dalal#1440 bionic Steam fixes) into Fix-Steam-Client
…m mode too The SdkCloudSaveSubdirField was nested under `if (config.launchRealSteam)`, and the launch-time bridge prompt in PluviaMain only fired for isLaunchRealSteam. Pattern B games (Dead Cells etc.) hit the same <install>/<subdir>/ vs <userdata>/<appid>/remote/ path mismatch in bionic mode as they do in Wine-Steam mode, so the bridge should be reachable from both. SteamAutoCloud's sync paths are already mode-agnostic; this just exposes the configuration UI/prompt.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 1343-1346: The current branch calls
winHandler.killProcess(gameExe) immediately which hard-kills the game before the
GRACEFUL_EXIT_GRACE_MS delay; change it to first attempt a graceful
WM_CLOSE-style shutdown (e.g., call a new or existing
winHandler.sendWmClose/closeWindow(gameExe) or similar graceful-close method)
then await delay(GRACEFUL_EXIT_GRACE_MS) and only call
winHandler.killProcess(gameExe) after the delay if the process is still running;
update XServerScreen.kt to send the close signal on first tap and reserve
killProcess(...) as the escalation after the timeout.
- Around line 4523-4538: The cache key steamExtractedKey only uses
wineVersion|containerVariant and must also include the Steam launch mode and a
bundle/image version so extractSteamFiles(...) and payload updates are correctly
invalidated; update the code that builds steamExtractedKey to append the
container's Steam launch mode (from the container state/metadata you use to
decide bionic vs real Steam) and a bundle/image fingerprint/version (obtainable
via ImageFs.find(context) or the container's bundle/image metadata), then
continue to use container.putExtra("steamExtractedForWine", steamExtractedKey)
so future checks (container.getExtra("steamExtractedForWine")) reflect those
dimensions and force re-extraction when launch mode or bundle changes.
🪄 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: 4b408510-3217-4365-95de-8b3e4e25879f
📒 Files selected for processing (16)
app/src/main/java/app/gamenative/PluviaApp.ktapp/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.ktapp/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.ktapp/src/main/java/app/gamenative/ui/model/MainViewModel.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/app/gamenative/utils/SteamUtils.ktapp/src/main/java/com/winlator/container/Container.javaapp/src/main/java/com/winlator/container/ContainerData.ktapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (1)
- app/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (14)
- app/src/main/java/app/gamenative/PluviaApp.kt
- app/src/main/java/com/winlator/container/Container.java
- app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.kt
- app/src/main/java/app/gamenative/PrefManager.kt
- app/src/main/java/app/gamenative/utils/ContainerUtils.kt
- app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt
- app/src/main/java/app/gamenative/ui/model/MainViewModel.kt
- app/src/main/java/app/gamenative/ui/PluviaMain.kt
- app/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.kt
- app/src/main/java/com/winlator/container/ContainerData.kt
- app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt
- app/src/main/java/com/winlator/winhandler/WinHandler.java
- app/src/main/java/app/gamenative/utils/SteamUtils.kt
- app/src/main/java/app/gamenative/service/SteamService.kt
Comment on lines
+4523
to
+4538
| val steamExtractedKey = "${container.wineVersion}|${container.containerVariant}" | ||
| val steamExtractedPrev = container.getExtra("steamExtractedForWine") | ||
| val steamExeFile = File(container.rootDir, ".wine/drive_c/Program Files (x86)/Steam/steam.exe") | ||
| if (steamExtractedPrev != steamExtractedKey && steamExeFile.exists()) { | ||
| // Symlink-safe delete: steamapps/common/<installdir> symlinks point at | ||
| // GameNative's own Steam dir, and a following recursive delete would wipe | ||
| // every installed game's files. | ||
| val steamDir = File(container.rootDir, ".wine/drive_c/Program Files (x86)/Steam") | ||
| SteamUtils.deleteTreeNoFollowSymlinks(steamDir) | ||
| } | ||
| extractSteamFiles(context, container, onExtractFileListener) | ||
| SteamUtils.ensureSteamCfg(ImageFs.find(context), container) | ||
| SteamUtils.purgePhantomAppUserdata(ImageFs.find(context), "241100", container) | ||
| SteamUtils.logSteamBinaryFingerprint(ImageFs.find(context), "prepareContainer:realSteam", container) | ||
| container.putExtra("steamExtractedForWine", steamExtractedKey) | ||
| containerDataChanged = true |
Contributor
There was a problem hiding this comment.
Broaden the Steam extraction cache key.
steamExtractedKey only tracks wineVersion|containerVariant. That misses launch-mode transitions and bundle updates: switching from bionic-Steam to real-Steam on the same Wine/variant leaves the cached bionic steam.exe in place because extractSteamFiles() sees an existing binary and returns early, and updated steam.tzst payloads also won't invalidate existing installs. Include at least the Steam launch mode and a bundle/image version in the key.
Suggested fix
- val steamExtractedKey = "${container.wineVersion}|${container.containerVariant}"
+ val steamModeKey = when {
+ container.isLaunchBionicSteam -> "bionic"
+ container.isLaunchRealSteam -> "real"
+ else -> "none"
+ }
+ val steamExtractedKey =
+ "${container.wineVersion}|${container.containerVariant}|$imgVersion|$steamModeKey"Also applies to: 5282-5284
🤖 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 4523 - 4538, The cache key steamExtractedKey only uses
wineVersion|containerVariant and must also include the Steam launch mode and a
bundle/image version so extractSteamFiles(...) and payload updates are correctly
invalidated; update the code that builds steamExtractedKey to append the
container's Steam launch mode (from the container state/metadata you use to
decide bionic vs real Steam) and a bundle/image fingerprint/version (obtainable
via ImageFs.find(context) or the container's bundle/image metadata), then
continue to use container.putExtra("steamExtractedForWine", steamExtractedKey)
so future checks (container.getExtra("steamExtractedForWine")) reflect those
dimensions and force re-extraction when launch mode or bundle changes.
Contributor
There was a problem hiding this comment.
2 issues found across 34 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/utils/SteamUtils.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/SteamUtils.kt:2669">
P1: SDK cloud mirroring routines `mirrorSdkCloudRemoteToSave` and `mirrorSdkCloudSaveToRemote` are non-recursive and silently skip nested directories/files, causing incomplete save restores or cloud uploads for games with nested save structures.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:1344">
P1: Non-real-Steam exit still force-kills the game on first tap. `killProcess(gameExe)` runs before the grace delay, so the app cannot execute normal shutdown/save hooks during the intended graceful window.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
…d mirror
Two issues raised by AI review on the merged Real-Steam branch:
1. XServerScreen.kt graceful exit (non-Steam path): killProcess ran
immediately before the GRACEFUL_EXIT_GRACE_MS delay, defeating the
purpose of the grace window. Swap the order so the delay precedes
the kill, giving any in-flight game-side exit (autosave, in-game
quit dialog) time to complete before hard-kill. Note: a proper
WM_CLOSE escalation requires a new winhandler.exe RequestCode
(native code change outside this commit's scope).
2. SteamUtils.kt mirrorSdkCloud{RemoteToSave,SaveToRemote} were
non-recursive — top-level listFiles + 'is regular file' filter
silently skipped any subdirectories. Pattern B games with nested
save structures (slots/profiles/etc.) would have partial or no
restore. Replaced with Files.walk + relativize + createDirectories
for full-tree recursion. backup-*.zip filter retained on filename
(so a backup-* zip nested in a saves hierarchy is still excluded,
matching the pre-recursive semantics for that one exclusion).
The `shutdownSteam` flag was gated only on `isLaunchRealSteam`, so
bionic-mode exits fell through to the else branch's hard-kill (with a
synthetic delay-then-kill that doesn't actually signal the game).
Bionic Steam runs the same steam.exe in Wine — just with the native
libsteamclient.so loaded in-process. `steam.exe -shutdown` works there
identically: Steam delivers SteamShutdown_t to running games via the
SteamAPI callback, games autosave + exit cleanly, Steam then flushes
its own state.
Broaden the flag to cover bionic mode so the existing graceful path
(winHandler.exec("steam.exe -shutdown") + awaitSteamShutdown loop)
runs for both Steam modes. Emu mode keeps the delay-then-kill
fallback — no Steam to talk to there.
Owner
|
Good changes here, but the PR needs to be greatly reduced in scope. The changes that apply to the actual steam client and anything around that should be kept, everything else is really out of scope here. SteamAutoCloud.kt should not be touched - cloud saves are not enabled when real steam is. SteamService should also not be touched unless there's a reason, I see some big changes in there but don't understand what they're doing. |
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.
This PR hardens the existing Launch Steam Client ("real-Steam") path so it stops vague-black-screening, becomes more compatible across a wider set of titles, and stops cascading state corruption between containers. Most of the work is in the real-Steam launch flow, but several lifecycle/correctness fixes that surfaced along the way also benefit standard cold-client launches.
What it does:
-silentfrom the Steam launch flags. All-silentwas really doing was hiding the cloud-conflict-resolution dialog, so failed boots looked identical to silent hangs. With it removed, you can actually see what Steam is doing when something doesn't work. Steam still launches behind the game window thanks to-vgui -tcp -no-browser -nobigpicture -nofriendsui -nochatui -nointro.SharedDepotsdeclaration, and writesappmanifest_241100.acfalongsideappmanifest_228980.acfso Steam doesn't reparent depots on every launch.Installed=1/Run=1install-script registry shims for the canonical 228980 depots. The reinstaller was racing the game's own MSVC loader and (on Unity titles) trippingUnityCrashHandlermid-init.ChangeNumber, and clears staleAppSessionActivephantoms inbeginLaunchApp. Cloud saves are still synced through GameNative's ownSteamAutoCloudrather than the in-Wine Steam Client — same as before — for stability. We mirror to/from<userdata>/<appid>/remote/ourselves before/after launch.WM_CLOSEto the game (andsteam.exein real-Steam mode), waits 5 s for clean save flush, then proceeds with the existing exit path. Second tap force-quits..archand Wine prefix paths to stop one container's wine/proton variant from cascading into another's. Several SteamUtils paths complete that migration end-to-end.WINEDLLOVERRIDES=gameoverlayrenderer=;gameoverlayrenderer64=so Wine refuses to load the overlay PE entirely (in addition to the existing Vulkan-layer skip andSteamNoOverlayUIDrawing=1). Real-Steam-only.disableSteamOverlayper-container toggle in the General tab when real-Steam is on.SysVSharedMemory.deletelock guard, X serverBadIdChoicedefensive null-check in DRI3, GLX leak chain skip in compositor, GLX-window OOM cap, recursive-delete fix inAdrenotoolsManager. The point of this branch is to reduce vague black screens when using Launch Steam Client; these are byproducts of that work.Known issues / things to be aware of:
steamclient64.dll → secur32 → sspicli) requires. BannerHub ships a full Samba + Kerberos stack to make this work; we don't, and adding the runtime files alone isn't enough — Wine itself needs to be built with--with-krb5 --with-samba. If anyone with proton/wine cross-build experience can take a swing at this, I'd be very grateful.I also added per-launch logging of
steam.exe/steamclient.dll/steamclient64.dllsize+mtime fingerprints so we can correlate "Steam updated under us" with launch failures.Recording
https://photos.app.goo.gl/5XTwUjdaQ2YKa7556
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
Reduces black screens and stalls in Steam launches by hardening cloud sync, SDK‑cloud bridging, overlay disabling, and shutdown flows. Extends the Cloud Save Bridge and graceful Steam shutdown to Bionic Steam, and makes SDK‑cloud mirroring recursive for nested saves.
Bug Fixes
remotecache.vdf(also on deletes); kick stale sessions on exit and before real‑Steam launches.WINEDLLOVERRIDESnow blocksgameoverlayrenderer(64)at Wine’s loader; Vulkan layer disabled; per‑container toggle defaults to on.WinHandler.exec(filename, parameters)now handles quoted paths; multi-listener support for process info with per-callback try/catch; snapshot collection serialized via mutex to prevent cross‑pollution; distinguish send failures from legitimate empty snapshots to avoid false “Steam exited”.PrefManagerdefaults.steam_api*.dllas verification failures to avoid silent desyncs.WINEPREFIXafter container vars; process‑info watchers no longer clobber each other; Bionic Steam now uses Steam’s graceful shutdown too; non‑Steam grace window now waits before kill.Dependencies
.gitmodules: setignore = dirtyfor submoduleapp/src/main/cpp/lsfg-vk-androidto suppress false dirty state from quarantined test assets in transitive deps.Written for commit f105fd5. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements