fix: set ColdClientLoader.ini ExeRunDir to exe directory when no workingDir in appinfo#1012
Merged
utkarshdalal merged 1 commit intoutkarshdalal:masterfrom Mar 28, 2026
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPasses LaunchInfo into ColdClientLoader.ini generation, refactors INI creation into a pure generator that computes ExeRunDir, normalizes paths, and conditionally includes DLL injection; adds unit tests for the generated INI behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 3 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/test/java/app/gamenative/utils/SteamUtilsColdClientIniTest.kt">
<violation number="1" location="app/src/test/java/app/gamenative/utils/SteamUtilsColdClientIniTest.kt:28">
P2: Regression test for `ExeRunDir` is too permissive: prefix-based `contains` can pass even when an incorrect subdirectory is appended.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
app/src/test/java/app/gamenative/utils/SteamUtilsColdClientIniTest.kt
Outdated
Show resolved
Hide resolved
![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: 1
🧹 Nitpick comments (1)
app/src/test/java/app/gamenative/utils/SteamUtilsColdClientIniTest.kt (1)
24-73: Strengthen these tests to reduce brittleness and cover trailing-slash regression.Line 34 depends on a formatting newline, and Line 71 uses
assertTrue(!...); both can be made clearer. Also, add an explicitworkingDirtrailing-slash test to lock in the trim behavior.🧪 Suggested test refinements
-import org.junit.Assert.assertTrue +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue @@ fun `ExeRunDir is game root when workingDir is empty`() { val ini = generate(workingDir = "") - assertTrue(ini.contains("ExeRunDir=steamapps\\common\\Batman Arkham Asylum GOTY\n")) + assertTrue(ini.contains("ExeRunDir=steamapps\\common\\Batman Arkham Asylum GOTY")) } @@ fun `DllsToInjectFolder is absent when isUnpackFiles is false`() { val ini = generate(isUnpackFiles = false) - assertTrue(!ini.contains("DllsToInjectFolder")) + assertFalse(ini.contains("DllsToInjectFolder")) } + + `@Test` + fun `workingDir trailing slash is trimmed`() { + val ini = generate(workingDir = "Binaries/") + assertTrue(ini.contains("ExeRunDir=steamapps\\common\\Batman Arkham Asylum GOTY\\Binaries")) + }🤖 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/SteamUtilsColdClientIniTest.kt` around lines 24 - 73, Tests in SteamUtilsColdClientIniTest.kt are brittle: remove the dependency on a trailing newline in the "`ExeRunDir is game root when workingDir is empty`" test by checking for the substring without "\n" (use generate(...) and assert that ini.contains("ExeRunDir=steamapps\\common\\Batman Arkham Asylum GOTY")), replace negated assertTrue(!...) in "`DllsToInjectFolder is absent when isUnpackFiles is false`" with assertFalse(ini.contains("DllsToInjectFolder")), and add a new test using generate(workingDir = "Binaries/") (or similar) to verify that workingDir trailing slashes are normalized (assert ini.contains("ExeRunDir=...\\Binaries") and not a trailing backslash) so Generate (the generate(...) helper) and the existing test functions validate trimming/normalization behavior consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Line 2874: The code is passing a possibly-mismatched LaunchInfo (appLaunchInfo
from firstOrNull()) into SteamUtils.writeColdClientIni which can set the wrong
ExeRunDir for multi-launch games; update the call to first locate the LaunchInfo
that corresponds to the selected executable (match container.executablePath
against the LaunchInfo's executablePath/launch entry in container.launches) and
use that matched LaunchInfo when calling SteamUtils.writeColdClientIni, falling
back to the existing firstOrNull() behavior only if no exact match is found.
---
Nitpick comments:
In `@app/src/test/java/app/gamenative/utils/SteamUtilsColdClientIniTest.kt`:
- Around line 24-73: Tests in SteamUtilsColdClientIniTest.kt are brittle: remove
the dependency on a trailing newline in the "`ExeRunDir is game root when
workingDir is empty`" test by checking for the substring without "\n" (use
generate(...) and assert that ini.contains("ExeRunDir=steamapps\\common\\Batman
Arkham Asylum GOTY")), replace negated assertTrue(!...) in "`DllsToInjectFolder
is absent when isUnpackFiles is false`" with
assertFalse(ini.contains("DllsToInjectFolder")), and add a new test using
generate(workingDir = "Binaries/") (or similar) to verify that workingDir
trailing slashes are normalized (assert ini.contains("ExeRunDir=...\\Binaries")
and not a trailing backslash) so Generate (the generate(...) helper) and the
existing test functions validate trimming/normalization behavior consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a2fbaf2-2ab3-499a-8e41-fe945bcdf2de
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/SteamUtils.ktapp/src/test/java/app/gamenative/utils/SteamUtilsColdClientIniTest.kt
| if (!container.isUseLegacyDRM){ | ||
| // Create ColdClientLoader.ini file | ||
| SteamUtils.writeColdClientIni(gameId, container) | ||
| SteamUtils.writeColdClientIni(gameId, container, appLaunchInfo) |
Contributor
There was a problem hiding this comment.
Match LaunchInfo to the selected executable before writing ColdClientLoader.ini.
On Line 2874, passing appLaunchInfo (currently derived via firstOrNull()) can apply the wrong workingDir for multi-launch games where container.executablePath is not the first launch entry. That can set an incorrect ExeRunDir and break runtime CWD-sensitive titles.
💡 Suggested fix
- SteamUtils.writeColdClientIni(gameId, container, appLaunchInfo)
+ val normalizedSelectedExe = container.executablePath.replace('\\', '/')
+ val matchedLaunchInfo = SteamService.getWindowsLaunchInfos(gameId).firstOrNull { info ->
+ info.executable.replace('\\', '/').equals(normalizedSelectedExe, ignoreCase = true)
+ } ?: appLaunchInfo
+ SteamUtils.writeColdClientIni(gameId, container, matchedLaunchInfo)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| SteamUtils.writeColdClientIni(gameId, container, appLaunchInfo) | |
| val normalizedSelectedExe = container.executablePath.replace('\\', '/') | |
| val matchedLaunchInfo = SteamService.getWindowsLaunchInfos(gameId).firstOrNull { info -> | |
| info.executable.replace('\\', '/').equals(normalizedSelectedExe, ignoreCase = true) | |
| } ?: appLaunchInfo | |
| SteamUtils.writeColdClientIni(gameId, container, matchedLaunchInfo) |
🤖 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` at line
2874, The code is passing a possibly-mismatched LaunchInfo (appLaunchInfo from
firstOrNull()) into SteamUtils.writeColdClientIni which can set the wrong
ExeRunDir for multi-launch games; update the call to first locate the LaunchInfo
that corresponds to the selected executable (match container.executablePath
against the LaunchInfo's executablePath/launch entry in container.launches) and
use that matched LaunchInfo when calling SteamUtils.writeColdClientIni, falling
back to the existing firstOrNull() behavior only if no exact match is found.
| val exePath = "steamapps\\common\\$gameName\\${executablePath.replace("/", "\\")}" | ||
| val gameRoot = "steamapps\\common\\$gameName" | ||
| val normalizedWorkingDir = workingDir?.replace("/", "\\")?.trim('\\') | ||
| val exeRunDir = if (!normalizedWorkingDir.isNullOrEmpty()) "$gameRoot\\$normalizedWorkingDir" else gameRoot |
Owner
There was a problem hiding this comment.
leave blank (legacy behaviour) instead of explicitly setting gameroot
kiequoo
force-pushed
the
fix/cold-client-ini-workingdir
branch
from
6d8a6cc to
84e5fcf
Compare
kiequoo
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/test/java/app/gamenative/utils/SteamUtilsColdClientIniTest.kt (1)
27-44: Consider adding test coverage forisUnpackFilesbehavior.The tests thoroughly cover the
workingDirscenarios affectingExeRunDir, which is the core fix in this PR. However, the PR description mentions that "only includes the injection section when unpackFiles is enabled" — this behavior isn't currently tested.🧪 Suggested additional tests for injection section
`@Test` fun `injection section excludes DllsToInjectFolder when unpackFiles is false`() { val ini = generate(isUnpackFiles = false) assert(!ini.contains("DllsToInjectFolder")) } `@Test` fun `injection section includes DllsToInjectFolder when unpackFiles is true`() { val ini = generate(isUnpackFiles = true) assert(ini.contains("DllsToInjectFolder=extra_dlls")) }🤖 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/SteamUtilsColdClientIniTest.kt` around lines 27 - 44, Add tests to cover the isUnpackFiles behavior described in the PR: call the existing generate(...) helper with isUnpackFiles = false and assert the produced ini does not contain the "DllsToInjectFolder" key, and call generate(...) with isUnpackFiles = true and assert the ini contains "DllsToInjectFolder=extra_dlls" (or the expected value); use the same test helpers (generate and iniValue / contains on the returned ini) as existing tests so the new tests mirror the style of `ExeRunDir` tests and verify the injection section is included only when isUnpackFiles is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/test/java/app/gamenative/utils/SteamUtilsColdClientIniTest.kt`:
- Around line 27-44: Add tests to cover the isUnpackFiles behavior described in
the PR: call the existing generate(...) helper with isUnpackFiles = false and
assert the produced ini does not contain the "DllsToInjectFolder" key, and call
generate(...) with isUnpackFiles = true and assert the ini contains
"DllsToInjectFolder=extra_dlls" (or the expected value); use the same test
helpers (generate and iniValue / contains on the returned ini) as existing tests
so the new tests mirror the style of `ExeRunDir` tests and verify the injection
section is included only when isUnpackFiles is enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3254de9f-92c5-4d29-b76e-25c366ba26d6
📒 Files selected for processing (1)
app/src/test/java/app/gamenative/utils/SteamUtilsColdClientIniTest.kt
…orkingDir in appinfo Games like New Star GP whose exe lives in a subdirectory (e.g. release/NSGP.exe) need ExeRunDir set to that subdirectory so CWD-relative asset and save paths resolve correctly. When workingDir is present in appinfo, ExeRunDir is left blank to preserve legacy ColdClientLoader behaviour. Extracts generateColdClientIni() for testability and adds unit tests.
kiequoo
force-pushed
the
fix/cold-client-ini-workingdir
branch
from
3ad5c87 to
2ac21e0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR #775 introduced ExeRunDir to fix save loading in New Star GP, whose exe lives in a subdirectory (
release/NSGP.exe) and opens saves via a relative path from the game install root. Without ExeRunDir set correctly, ColdClientLoader defaults the working directory to the exe's parent, so the save file was never found.However, hardcoding ExeRunDir to the game root broke games like Batman: Arkham Asylum GOTY (PR #950), which declares
workingdir: Binariesin its appinfo. For these games the correct CWD is the subdirectory, not the root — and the previous empty ExeRunDir happened to work because ColdClientLoader defaulted to the exe's parent.This fix derives ExeRunDir from the exe path when
workingDiris absent in appinfo, so New Star GP getsrelease/as its CWD. WhenworkingDiris present, ExeRunDir is left blank to preserve legacy behaviour (ColdClientLoader uses the loader's own CWD).The INI content is extracted into
generateColdClientIni()for testability, with unit tests covering the New Star GP case and the workingDir-set case.Fixes: #775
Fixes: #950
Summary by CodeRabbit
New Features
Tests