fix(envvars): escape spaces on round-trip; fix crash on save#1429
Open
jeremybernstein wants to merge 2 commits into
Open
fix(envvars): escape spaces on round-trip; fix crash on save#1429jeremybernstein wants to merge 2 commits into
jeremybernstein wants to merge 2 commits into
Conversation
Contributor
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughEnvVars now escapes/unescapes backslashes and spaces for persistent serialization and parsing; ChangesEnvironment Variable Parsing and Serialization (Fixes
Sequence DiagramsequenceDiagram
participant Input as Serialized String
participant Split as splitOnUnescapedSpaces
participant Parse as putAll loop
participant Unescape as unescape
participant Store as EnvVars map
participant Output as toString
Input->>Split: split on unescaped spaces
Split->>Parse: iterate tokens ("KEY=VALUE")
Parse->>Parse: find '=' delimiter
Parse->>Unescape: unescape key & value
Unescape->>Store: put(key, value)
Store->>Output: reconstruct escaped string via escape()
Output->>Input: emits escaped persistence form
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
EnvVars.toString() joined K=V pairs with " " but putAll() split on " "
and substring'd at indexOf("="). A value with a space (e.g. DXVK_CONFIG=
"d3d9.presentInterval = 1;") produced tokens with no "=", indexOf
returned -1, and substring(0, -1) threw StringIndexOutOfBoundsException
on the next recompose of the Environment dialog.
Make toString()/putAll() escape-aware (backslash-escape \ and space);
keep toStringArray() raw for execve. toEscapedString() now aliases
toString() since the new format is shell-safe too.
jeremybernstein
force-pushed
the
jb/fix-env-var-input
branch
from
38dd417 to
3f34bea
Compare
Contributor
Author
|
sorry, accidentally pushed with my dev environment still on the branch. re-pushed with it removed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/src/main/java/app/gamenative/ui/components/BootingSplash.kt (1)
165-167: ⚡ Quick winAvoid redundant LocalContext.current accesses.
LocalContext.currentis accessed twice on consecutive lines. In Compose, reading composition locals has a small cost, and the value doesn't change within the same composition.♻️ Use a local variable
- val appLabel = androidx.compose.ui.platform.LocalContext.current.applicationInfo.loadLabel( - androidx.compose.ui.platform.LocalContext.current.packageManager - ).toString() + val context = LocalContext.current + val appLabel = context.applicationInfo.loadLabel(context.packageManager).toString()🤖 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/components/BootingSplash.kt` around lines 165 - 167, The code redundantly calls LocalContext.current twice when computing appLabel; capture LocalContext.current once into a local val (e.g., val context = LocalContext.current) and then call context.applicationInfo.loadLabel(context.packageManager).toString() so appLabel uses the single cached context value (update the expression that creates appLabel accordingly).app/src/main/java/app/gamenative/ui/screen/login/UserLoginScreen.kt (1)
353-355: ⚡ Quick winAvoid redundant LocalContext.current accesses.
LocalContext.currentis accessed twice on consecutive lines. In Compose, reading composition locals has a small cost, and the value doesn't change within the same composition.♻️ Use a local variable
+ val context = LocalContext.current Text( - text = androidx.compose.ui.platform.LocalContext.current.applicationInfo.loadLabel( - androidx.compose.ui.platform.LocalContext.current.packageManager - ).toString(), + text = context.applicationInfo.loadLabel(context.packageManager).toString(), style = MaterialTheme.typography.headlineSmall.copy(🤖 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/login/UserLoginScreen.kt` around lines 353 - 355, Redundant accesses to LocalContext.current are used when computing the app label; capture the composition local once into a local val (e.g., val context = LocalContext.current) inside the UserLoginScreen scope and replace both occurrences with context, then call context.applicationInfo.loadLabel(context.packageManager).toString(); this reduces repeated composition-local reads while preserving behavior.app/src/main/cpp/extras/evshim.c (2)
95-97: 💤 Low valueRemove temporary "NEW" comments.
The
/* NEW */comments on lines 95 and 97 are no longer needed now that the change is complete.♻️ Clean up temporary markers
- pthread_mutex_lock(&shm_mutex); /* NEW */ + pthread_mutex_lock(&shm_mutex); ssize_t w = pwrite(rumble_fd[idx], vals, sizeof(vals), 32); - pthread_mutex_unlock(&shm_mutex); /* NEW */ + pthread_mutex_unlock(&shm_mutex);🤖 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/cpp/extras/evshim.c` around lines 95 - 97, Remove the temporary "/* NEW */" markers left around the mutex operations: delete the comment tokens adjacent to pthread_mutex_lock(&shm_mutex) and pthread_mutex_unlock(&shm_mutex) in the evshim.c change so the lines read simply pthread_mutex_lock(&shm_mutex); and pthread_mutex_unlock(&shm_mutex); surrounding the pwrite call that uses rumble_fd[idx], vals and sizeof(vals).
133-156: ⚡ Quick winVolatile qualifier is stripped by casts, defeating synchronization intent.
Declaring
curas volatile (line 133) but then casting it tovoid*(line 140) andconst void*(lines 142, 156) strips the volatile qualifier. This defeats the purpose of volatile, which is to prevent the compiler from optimizing away accesses.Since you're already using
shm_mutexto protect this critical section (lines 138, 162), volatile is unnecessary. The mutex provides the required memory ordering guarantees.♻️ Remove unnecessary volatile qualifier
- volatile struct gamepad_io cur; + struct gamepad_io cur; struct gamepad_io last_state = {0}; - memset((void*)&cur, 0, sizeof cur); + memset(&cur, 0, sizeof cur); for (;;) { pthread_mutex_lock(&shm_mutex); - ssize_t n = pread(fd, (void*)&cur, sizeof cur, 0); + ssize_t n = pread(fd, &cur, sizeof cur, 0); - if (n == (ssize_t)sizeof cur && memcmp((const void*)&cur, &last_state, sizeof cur) != 0) { + if (n == (ssize_t)sizeof cur && memcmp(&cur, &last_state, sizeof cur) != 0) { p_SDL_JoystickSetVirtualAxis (js, 0, cur.lx); p_SDL_JoystickSetVirtualAxis (js, 1, cur.ly); @@ -152,7 +152,7 @@ p_SDL_JoystickSetVirtualHat(js, 0, cur.hat); - memcpy(&last_state, (const void*)&cur, sizeof cur); + memcpy(&last_state, &cur, sizeof cur); }🤖 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/cpp/extras/evshim.c` around lines 133 - 156, The local variable declared as "volatile struct gamepad_io cur;" should drop the volatile qualifier—remove "volatile" from the declaration of cur and update the pointer uses so you're casting to plain (void*)/ (const void*) rather than casting away volatile; keep the existing pthread_mutex_lock(&shm_mutex) critical section and leave the pread(fd, &cur, ...), memcmp(&cur, &last_state, ...), and memcpy(&last_state, &cur, ...) calls inside it to preserve synchronization. Ensure symbols touched: cur, last_state, pread, memcmp, memcpy, pthread_mutex_lock(&shm_mutex), and the p_SDL_JoystickSetVirtual* calls remain unchanged.
🤖 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/build.gradle.kts`:
- Around line 130-131: The release build is incorrectly inheriting debug
identity and label; remove or relocate applicationIdSuffix = ".debug" and
manifestPlaceholders["appLabel"] = "GameNative Dev" from the shared config and
instead set them only inside the debug buildType (or debug productFlavor) block
so that release keeps the clean applicationId and proper appLabel; update the
buildTypes { debug { applicationIdSuffix = ".debug";
manifestPlaceholders["appLabel"] = "GameNative Dev" } release { /* no debug
suffix/label */ } } ensuring applicationIdSuffix and
manifestPlaceholders["appLabel"] are not applied to the release variant.
In `@app/src/test/java/com/winlator/core/envvars/EnvVarsTest.kt`:
- Line 69: Replace usages of Kotlin's assert with JUnit assertions to ensure
tests run regardless of JVM assertion settings: import and use
org.junit.Assert.assertTrue (or assertFalse/assertEquals as appropriate) in
EnvVarsTest, e.g. replace the call that checks parsed.has("EMPTY") and the other
assert(...) calls with assertTrue(parsed.has("EMPTY")) (and corresponding
assertTrue/assertFalse for the other two assertions) so the test framework
enforces them reliably.
---
Nitpick comments:
In `@app/src/main/cpp/extras/evshim.c`:
- Around line 95-97: Remove the temporary "/* NEW */" markers left around the
mutex operations: delete the comment tokens adjacent to
pthread_mutex_lock(&shm_mutex) and pthread_mutex_unlock(&shm_mutex) in the
evshim.c change so the lines read simply pthread_mutex_lock(&shm_mutex); and
pthread_mutex_unlock(&shm_mutex); surrounding the pwrite call that uses
rumble_fd[idx], vals and sizeof(vals).
- Around line 133-156: The local variable declared as "volatile struct
gamepad_io cur;" should drop the volatile qualifier—remove "volatile" from the
declaration of cur and update the pointer uses so you're casting to plain
(void*)/ (const void*) rather than casting away volatile; keep the existing
pthread_mutex_lock(&shm_mutex) critical section and leave the pread(fd, &cur,
...), memcmp(&cur, &last_state, ...), and memcpy(&last_state, &cur, ...) calls
inside it to preserve synchronization. Ensure symbols touched: cur, last_state,
pread, memcmp, memcpy, pthread_mutex_lock(&shm_mutex), and the
p_SDL_JoystickSetVirtual* calls remain unchanged.
In `@app/src/main/java/app/gamenative/ui/components/BootingSplash.kt`:
- Around line 165-167: The code redundantly calls LocalContext.current twice
when computing appLabel; capture LocalContext.current once into a local val
(e.g., val context = LocalContext.current) and then call
context.applicationInfo.loadLabel(context.packageManager).toString() so appLabel
uses the single cached context value (update the expression that creates
appLabel accordingly).
In `@app/src/main/java/app/gamenative/ui/screen/login/UserLoginScreen.kt`:
- Around line 353-355: Redundant accesses to LocalContext.current are used when
computing the app label; capture the composition local once into a local val
(e.g., val context = LocalContext.current) inside the UserLoginScreen scope and
replace both occurrences with context, then call
context.applicationInfo.loadLabel(context.packageManager).toString(); this
reduces repeated composition-local reads while preserving behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c705470-48bf-48cc-9f24-26f8edd6cf15
⛔ Files ignored due to path filters (1)
app/src/main/jniLibs/arm64-v8a/libevshim.sois excluded by!**/*.so
📒 Files selected for processing (15)
ARCHITECTURE.mdapp/build.gradle.ktsapp/src/main/AndroidManifest.xmlapp/src/main/cpp/evshim/CMakeLists.txtapp/src/main/cpp/extras/evshim.capp/src/main/java/app/gamenative/ui/components/BootingSplash.ktapp/src/main/java/app/gamenative/ui/screen/login/UserLoginScreen.ktapp/src/main/java/app/gamenative/utils/IntentLaunchManager.ktapp/src/main/java/com/winlator/container/Container.javaapp/src/main/java/com/winlator/core/DXVKHelper.javaapp/src/main/java/com/winlator/core/WineUtils.javaapp/src/main/java/com/winlator/core/envvars/EnvVars.javaapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.javaapp/src/test/java/com/winlator/core/envvars/EnvVarsTest.kt
kotlin assert() is a no-op when JVM assertions aren't enabled (gradle's Test task default). switch to org.junit.Assert.assertTrue so the framework enforces the checks regardless of -ea.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 16 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/ui/components/BootingSplash.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/components/BootingSplash.kt:165">
P2: App label resolved from PackageManager on every animation-driven recomposition instead of being memoized</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:48">
P2: Existing E: drive targets are not migrated to the new APPLICATION_ID-based storage path, so upgraded containers can keep a stale/broken E: mapping.</violation>
</file>
<file name="app/src/main/AndroidManifest.xml">
<violation number="1" location="app/src/main/AndroidManifest.xml:60">
P2: Changing the launch action to `${applicationId}.LAUNCH_GAME` breaks the existing hardcoded sender that still uses `app.gamenative.LAUNCH_GAME`, so launcher shortcuts can stop resolving in non-release variants.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes a crash when saving an environment variable whose value contains a space (e.g.
DXVK_CONFIG = d3d9.presentInterval = 1;).EnvVars.toString()joinedKEY=valuepairs with a literal space, butEnvVars.putAll(String)split on a literal space and thensubstring(0, indexOf("="))on each token. A spaced value produced tokens without=, soindexOfreturned-1andsubstring(0, -1)threwStringIndexOutOfBoundsExceptionon the recompose right after the user tapped OK.Made
toString()/putAll(String)escape-aware (backslash-escapes\andtoStringArray()stays raw — that's the env vector handed toexecve, which must not see escapes.toEscapedString()(used to composeenv KEY=val ... cmdshell strings) now aliasestoString()since the new format is also shell-safe and was previously slightly different (didn't escape\).Closes #1428
Recording
N/A — crash fix, no visible behavior change beyond "doesn't crash". Happy to attach a before/after if needed.
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.Tests
New
EnvVarsTestcovers the round-trip contract: plain values, value-with-spaces (the crash repro), value with multiple spaces +=+;(multi-clause DXVK config), semicolon-only values, embedded backslashes, pre-escaped\<space>literal in source, empty values, and thattoStringArray()returns raw values forexecve.Verified on device
Built
:app:assembleDebug, installed on a physical device, manually enteredDXVK_CONFIG = d3d9.presentInterval = 1;(with and without surrounding quotes) — no crash, value persists across dialog open/close.Summary by cubic
Fixes a crash when saving environment variables with spaces by making
EnvVarsparse/serialize escape-aware. Round-trips are now lossless whileexecveenv values stay raw.EnvVars: escape-awaretoString()andputAll(String)for spaces and backslashes; tolerates stray tokens without=.toEscapedString()now aliasestoString();toStringArray()stays raw forexecve.EnvVarsTestcovering spaces, backslashes, empty values, order, and rawtoStringArray(); switched to JUnitassertTrueso checks run without-ea.Written for commit d5d38be. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests