Android 35#1458
Merged
Merged
Conversation
added 8 commits
March 5, 2026 19:41
# Conflicts: # app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt
Introduce a `legacy` (targetSdk 28) and `modern` (minSdk 29, targetSdk 36) product flavor on both :app and :ubuntufs, exposing MODERN_ANDROID and PRELOAD_BIONIC_SO via BuildConfig. The modern-only workarounds added for running on newer Android — /system/bin/linker64 prefix, REDIRECT_EXEC env, extra Wine /lib on LD_LIBRARY_PATH, and the wx preload .so name — are now gated behind BuildConfig.MODERN_ANDROID so the legacy build reproduces master behavior exactly. FOREGROUND_SERVICE_TYPE_DATA_SYNC is gated at runtime on Build.VERSION.SDK_INT >= Q since both builds must run on API 26-28 (legacy) or newer. Bumps compileSdk to 36 (Android 16, current latest stable).
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 ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds legacy/modern product flavors (compileSdk 36), propagates BuildConfig gates across runtime components, refactors per-service foreground notifications and service lifecycle, updates storage permissions/permission UI for modern Android, and updates CI/CD workflows to build, sign, and release both legacy and modern APK variants. ChangesDual-flavor app with legacy/modern runtime variants
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
phobos665
reviewed
May 19, 2026
Contributor
There was a problem hiding this comment.
Out of curiosity, would we not also be able to do variants after this?
Owner
Author
There was a problem hiding this comment.
Sorry for the delay - when you say variants what do you mean?
Owner
Author
There was a problem hiding this comment.
We do need to remove glibc for the modern build, and some changes to external storage.
added 6 commits
May 20, 2026 22:41
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/tagged-release.yml (1)
9-11:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit least-privilege
permissionsfor this workflow.This workflow relies on default token scopes, which is broader than necessary for a release pipeline. Declare minimal job-level (or workflow-level) permissions explicitly.
🛡️ Suggested baseline
jobs: build: + permissions: + contents: read + actions: write runs-on: ubuntu-latest ... release: + permissions: + contents: write + actions: read needs: build runs-on: ubuntu-latest🤖 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 @.github/workflows/tagged-release.yml around lines 9 - 11, The workflow currently relies on default token scopes; add an explicit least-privilege permissions block at the workflow or job level (e.g., under the top-level jobs or under the "build" job) to restrict the GITHUB_TOKEN to only the necessary scopes for releases (for example: contents: write, packages: write, actions: read, id-token: write as needed) and remove reliance on defaults so the "jobs" -> "build" -> "runs-on" job uses the declared minimal permissions instead of broad defaults.
🧹 Nitpick comments (2)
app/src/main/java/com/winlator/xenvironment/components/GlibcProgramLauncherComponent.java (1)
192-195: 💤 Low valueModern branches in the GLIBC launcher appear unreachable.
Per the PR description, GLIBC containers are disabled on modern builds (
ContainerConfigDialogfilters the GLIBC variant andBestConfigServicerejects GLIBC configs whenMODERN_ANDROID). That makesBuildConfig.MODERN_ANDROIDalways-false-in-practice inside this class, so theREDIRECT_EXEC__PROC_SELF_EXE, Wine-/lib, andlinker64-prefix branches here are dead code on legacy and never exercised on modern. Either drop these branches to keep the launcher honest about its supported flavor, or document why they're retained (e.g., future GLIBC-on-modern support) so reviewers don't have to re-derive the contract.Also applies to: 305-308, 340-340
app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java (1)
304-312: ⚡ Quick winNo empty
PRELOAD_BIONIC_SOissue for current flavors; optional hardening ofLD_PRELOADIn
BionicProgramLauncherComponent(~304-312 and ~701-707),replacePathis always appended, butBuildConfig.PRELOAD_BIONIC_SOis explicitly non-empty for both flavors (libredirect-bionic.sovslibredirect-bionic-wx.soinapp/build.gradle.kts) andImageFsInstaller.installGuestLibs()installs the matching files intoimagefs/usr/lib/(bionic always; wx only for modern). The “imageFs.getLibDir() + '/'directory entry” scenario doesn’t apply to current builds.Optional robustness: gate the
LD_PRELOADappend on file existence (and consider avoiding malformed/empty entries ifsysvPathis missing).🤖 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/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java` around lines 304 - 312, The current code in BionicProgramLauncherComponent always appends replacePath (constructed from imageFs.getLibDir() + "/" + BuildConfig.PRELOAD_BIONIC_SO) into ld_preload even when the PRELOAD_BIONIC_SO might be empty or the file is missing; update the logic to first check that BuildConfig.PRELOAD_BIONIC_SO is non-empty and that new File(replacePath).exists() before appending it to ld_preload (similar checks should be applied around sysvPath), and build ld_preload so you only add ":" separators when a previous entry exists to avoid malformed empty segments before calling envVars.put("LD_PRELOAD", ld_preload); also verify consistency with ImageFsInstaller.installGuestLibs() expectations.
🤖 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 @.github/workflows/app-release-signed.yml:
- Around line 127-137: The workflow currently uses tag refs for the upload
action in the steps named "Upload Legacy APK" and "Upload Modern APK" (uses:
actions/upload-artifact@v4); replace those tag-based refs with the corresponding
immutable commit SHAs for the actions/upload-artifact repository (e.g., uses:
actions/upload-artifact@<full-commit-sha>) so each step pins to a specific
commit; update both occurrences to the same verified SHA and commit the change.
In `@app/src/main/java/app/gamenative/service/gog/GOGService.kt`:
- Around line 731-735: The log in GOGService.onTimeout is misleading: it says
"restarting" but the method only calls stopSelf(); update the behavior to be
consistent by changing the Timber.w message in GOGService.onTimeout to indicate
the service is stopping (e.g., "Foreground service timeout reached,
stopping...") or, if a true restart is required, implement a restart path
instead of stopSelf() (for example schedule a restart via
startService/JobScheduler/AlarmManager) and ensure the log matches the
implemented action; reference: GOGService.onTimeout, Timber.w(...), stopSelf().
In `@app/src/main/java/app/gamenative/service/NotificationHelper.kt`:
- Line 37: activeServices is an instance-local mutableSet in NotificationHelper
so when multiple NotificationHelper instances are used across services their
per-instance state can desynchronize summaries; change activeServices to a
single shared, thread-safe collection (e.g., move it into a companion object or
a singleton and use a concurrent set or synchronize access) and update all
methods that reference activeServices (including the logic in NotificationHelper
methods around lines referenced 64-109) to use the shared collection so service
start/stop and summary clearing reflect global active service state.
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt`:
- Around line 927-944: initialStoragePermissionGranted is computed once and then
used to seed hasStoragePermission, so changes to All Files Access or legacy
storage permissions outside the composable are not observed; update
hasStoragePermission from platform state when the dialog opens and on lifecycle
resume. Fix by replacing the single fixed remember block with a re-check path:
create a helper that computes current storage permission (reuse the same
BuildConfig/SDK checks used in initialStoragePermissionGranted), call that
helper to set hasStoragePermission when the install/manage dialog is shown and
also observe lifecycle ON_RESUME (via a LifecycleEventObserver in SteamAppScreen
or a LaunchedEffect with lifecycleOwner) to refresh hasStoragePermission; ensure
the same helper is used wherever initialStoragePermissionGranted was referenced
so permission state always reflects current
Environment.isExternalStorageManager()/ContextCompat checks.
In `@app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java`:
- Around line 196-200: The installer currently calls FileUtils.copy(ctx,
"libredirect-bionic-wx.so", wxDest) (a void method that swallows IOExceptions)
then blindly chmods wxDest; add a post-copy verification in ImageFsInstaller
(the installGuestLibs flow around the FileUtils.copy call) to check that
wxDest.exists() and wxDest.length() > 0, and if the check fails log an error via
the same logger and abort/throw from installGuestLibs (or return a failure) so
the rest of the install does not append
imageFs.getLibDir()+"/"+BuildConfig.PRELOAD_BIONIC_SO to LD_PRELOAD for
MODERN_ANDROID; only chmod(wxDest) when the file verification succeeds.
---
Outside diff comments:
In @.github/workflows/tagged-release.yml:
- Around line 9-11: The workflow currently relies on default token scopes; add
an explicit least-privilege permissions block at the workflow or job level
(e.g., under the top-level jobs or under the "build" job) to restrict the
GITHUB_TOKEN to only the necessary scopes for releases (for example: contents:
write, packages: write, actions: read, id-token: write as needed) and remove
reliance on defaults so the "jobs" -> "build" -> "runs-on" job uses the declared
minimal permissions instead of broad defaults.
---
Nitpick comments:
In
`@app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java`:
- Around line 304-312: The current code in BionicProgramLauncherComponent always
appends replacePath (constructed from imageFs.getLibDir() + "/" +
BuildConfig.PRELOAD_BIONIC_SO) into ld_preload even when the PRELOAD_BIONIC_SO
might be empty or the file is missing; update the logic to first check that
BuildConfig.PRELOAD_BIONIC_SO is non-empty and that new
File(replacePath).exists() before appending it to ld_preload (similar checks
should be applied around sysvPath), and build ld_preload so you only add ":"
separators when a previous entry exists to avoid malformed empty segments before
calling envVars.put("LD_PRELOAD", ld_preload); also verify consistency with
ImageFsInstaller.installGuestLibs() expectations.
🪄 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: b9fd0e8b-39dd-4b37-b4d7-c78efbe3f069
⛔ Files ignored due to path filters (3)
app/src/main/assets/libredirect-bionic-wx.sois excluded by!**/*.soapp/src/main/assets/libredirect-wx.sois excluded by!**/*.soapp/src/modern/assets/libredirect-bionic-wx.sois excluded by!**/*.so
📒 Files selected for processing (28)
.github/workflows/app-release-signed.yml.github/workflows/pluvia-pr-check.yml.github/workflows/tagged-release.ymlapp/build.gradle.ktsapp/src/legacy/AndroidManifest.xmlapp/src/main/AndroidManifest.xmlapp/src/main/java/app/gamenative/MainActivity.ktapp/src/main/java/app/gamenative/service/NotificationHelper.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/service/amazon/AmazonService.ktapp/src/main/java/app/gamenative/service/epic/EpicService.ktapp/src/main/java/app/gamenative/service/gog/GOGService.ktapp/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.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/BestConfigService.ktapp/src/main/java/com/winlator/core/ProcessHelper.javaapp/src/main/java/com/winlator/xenvironment/ImageFsInstaller.javaapp/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.javaapp/src/main/java/com/winlator/xenvironment/components/GlibcProgramLauncherComponent.javaapp/src/main/java/com/winlator/xenvironment/components/NetworkInfoUpdateComponent.javaapp/src/main/res/values-v35/themes.xmlapp/src/test/java/app/gamenative/service/gog/GOGDownloadManagerTest.ktapp/src/test/java/app/gamenative/service/gog/api/GOGManifestParserTest.ktapp/src/test/java/app/gamenative/utils/BestConfigServiceTest.ktapp/src/test/java/app/gamenative/utils/ManifestIdCorrelationTest.ktapp/src/testModern/resources/robolectric.propertiesubuntufs/build.gradle.kts
💤 Files with no reviewable changes (2)
- app/src/test/java/app/gamenative/utils/ManifestIdCorrelationTest.kt
- app/src/test/java/app/gamenative/service/gog/api/GOGManifestParserTest.kt
Comment on lines
+127
to
+137
| - name: Upload Legacy APK | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: app-release-signed | ||
| path: ${{ github.workspace }}/universal.apk | ||
|
|
||
| - name: Upload Modern APK | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: app-release-signed-modern | ||
| path: ${{ github.workspace }}/universal-modern.apk |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify tag-based (unpinned) refs in this workflow.
rg -nP 'uses:\s*actions/upload-artifact@v[0-9]+' .github/workflows/app-release-signed.ymlRepository: utkarshdalal/GameNative
Length of output: 153
Pin actions/upload-artifact to immutable commit SHAs.
app-release-signed.yml uses actions/upload-artifact@v4 at lines 128 and 134 (tag-based refs), which weakens supply-chain guarantees.
🔒 Suggested update
- uses: actions/upload-artifact@v4
+ uses: actions/upload-artifact@<full_commit_sha_for_v4>
...
- uses: actions/upload-artifact@v4
+ uses: actions/upload-artifact@<full_commit_sha_for_v4>🧰 Tools
🪛 zizmor (1.25.2)
[error] 128-128: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 134-134: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/app-release-signed.yml around lines 127 - 137, The
workflow currently uses tag refs for the upload action in the steps named
"Upload Legacy APK" and "Upload Modern APK" (uses: actions/upload-artifact@v4);
replace those tag-based refs with the corresponding immutable commit SHAs for
the actions/upload-artifact repository (e.g., uses:
actions/upload-artifact@<full-commit-sha>) so each step pins to a specific
commit; update both occurrences to the same verified SHA and commit the change.
Comment on lines
+731
to
+735
| override fun onTimeout(startId: Int, fgsType: Int) { | ||
| super.onTimeout(startId, fgsType) | ||
| Timber.w("[GOGService] Foreground service timeout reached, restarting...") | ||
| stopSelf() | ||
| } |
Contributor
There was a problem hiding this comment.
onTimeout says “restarting” but only stops the service.
Line 733 logs a restart, but Line 734 only calls stopSelf() with no restart path. This creates misleading runtime signals during timeout debugging.
Suggested minimal fix
- Timber.w("[GOGService] Foreground service timeout reached, restarting...")
+ Timber.w("[GOGService] Foreground service timeout reached, stopping service")
stopSelf()📝 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
| override fun onTimeout(startId: Int, fgsType: Int) { | |
| super.onTimeout(startId, fgsType) | |
| Timber.w("[GOGService] Foreground service timeout reached, restarting...") | |
| stopSelf() | |
| } | |
| override fun onTimeout(startId: Int, fgsType: Int) { | |
| super.onTimeout(startId, fgsType) | |
| Timber.w("[GOGService] Foreground service timeout reached, stopping service") | |
| stopSelf() | |
| } |
🤖 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/service/gog/GOGService.kt` around lines 731
- 735, The log in GOGService.onTimeout is misleading: it says "restarting" but
the method only calls stopSelf(); update the behavior to be consistent by
changing the Timber.w message in GOGService.onTimeout to indicate the service is
stopping (e.g., "Foreground service timeout reached, stopping...") or, if a true
restart is required, implement a restart path instead of stopSelf() (for example
schedule a restart via startService/JobScheduler/AlarmManager) and ensure the
log matches the implemented action; reference: GOGService.onTimeout,
Timber.w(...), stopSelf().
| private val notificationManager: NotificationManager = | ||
| context.getSystemService(NOTIFICATION_SERVICE) as NotificationManager | ||
|
|
||
| private val activeServices = mutableSetOf<Int>() |
Contributor
There was a problem hiding this comment.
Shared summary state is instance-local, so multi-service grouping can desync.
activeServices is stored per NotificationHelper instance, but this PR now drives notifications from multiple services. If different helper instances are used, one service can clear the summary while another is still active.
Suggested fix
class NotificationHelper `@Inject` constructor(`@ApplicationContext` private val context: Context) {
companion object {
@@
private const val NOTIFICATION_ID_SUMMARY = 100
+ private val activeServices = mutableSetOf<Int>()
+ private val activeServicesLock = Any()
@@
- private val activeServices = mutableSetOf<Int>()
-
@@
- `@Synchronized`
fun notify(content: String, id: Int = NOTIFICATION_ID_STEAM) {
- val notification = createServiceNotification(id, content)
- notificationManager.notify(id, notification)
- activeServices.add(id)
- refreshSummary()
+ synchronized(activeServicesLock) {
+ val notification = createServiceNotification(id, content)
+ notificationManager.notify(id, notification)
+ activeServices.add(id)
+ refreshSummary()
+ }
}
- `@Synchronized`
fun cancel(id: Int = NOTIFICATION_ID_STEAM) {
- notificationManager.cancel(id)
- if (activeServices.remove(id)) refreshSummary()
+ synchronized(activeServicesLock) {
+ notificationManager.cancel(id)
+ if (activeServices.remove(id)) refreshSummary()
+ }
}
@@
- `@Synchronized`
fun markActive(id: Int) {
- if (activeServices.add(id)) refreshSummary()
+ synchronized(activeServicesLock) {
+ if (activeServices.add(id)) refreshSummary()
+ }
}Also applies to: 64-109
🤖 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/service/NotificationHelper.kt` at line 37,
activeServices is an instance-local mutableSet in NotificationHelper so when
multiple NotificationHelper instances are used across services their
per-instance state can desynchronize summaries; change activeServices to a
single shared, thread-safe collection (e.g., move it into a companion object or
a singleton and use a concurrent set or synchronize access) and update all
methods that reference activeServices (including the logic in NotificationHelper
methods around lines referenced 64-109) to use the shared collection so service
start/stop and summary clearing reflect global active service state.
Comment on lines
927
to
944
| val initialStoragePermissionGranted = remember { | ||
| val writePermissionGranted = ContextCompat.checkSelfPermission( | ||
| context, | ||
| Manifest.permission.WRITE_EXTERNAL_STORAGE, | ||
| ) == PackageManager.PERMISSION_GRANTED | ||
| val readPermissionGranted = ContextCompat.checkSelfPermission( | ||
| context, | ||
| Manifest.permission.READ_EXTERNAL_STORAGE, | ||
| ) == PackageManager.PERMISSION_GRANTED | ||
| writePermissionGranted && readPermissionGranted | ||
| when { | ||
| BuildConfig.MODERN_ANDROID -> true | ||
| Build.VERSION.SDK_INT >= Build.VERSION_CODES.R -> Environment.isExternalStorageManager() | ||
| else -> { | ||
| val writePermissionGranted = ContextCompat.checkSelfPermission( | ||
| context, | ||
| Manifest.permission.WRITE_EXTERNAL_STORAGE, | ||
| ) == PackageManager.PERMISSION_GRANTED | ||
| val readPermissionGranted = ContextCompat.checkSelfPermission( | ||
| context, | ||
| Manifest.permission.READ_EXTERNAL_STORAGE, | ||
| ) == PackageManager.PERMISSION_GRANTED | ||
| writePermissionGranted && readPermissionGranted | ||
| } | ||
| } | ||
| } | ||
| var hasStoragePermission by remember { mutableStateOf(initialStoragePermissionGranted) } |
Contributor
There was a problem hiding this comment.
Re-read storage access instead of treating it as fixed screen state.
initialStoragePermissionGranted is captured once and then reused as the source of truth. If the user changes All Files Access or legacy storage perms outside these launchers, the next install/manage flow can take the wrong branch until this composable is recreated. Refresh this from platform state when the dialog opens or on ON_RESUME.
🤖 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/library/appscreen/SteamAppScreen.kt`
around lines 927 - 944, initialStoragePermissionGranted is computed once and
then used to seed hasStoragePermission, so changes to All Files Access or legacy
storage permissions outside the composable are not observed; update
hasStoragePermission from platform state when the dialog opens and on lifecycle
resume. Fix by replacing the single fixed remember block with a re-check path:
create a helper that computes current storage permission (reuse the same
BuildConfig/SDK checks used in initialStoragePermissionGranted), call that
helper to set hasStoragePermission when the install/manage dialog is shown and
also observe lifecycle ON_RESUME (via a LifecycleEventObserver in SteamAppScreen
or a LaunchedEffect with lifecycleOwner) to refresh hasStoragePermission; ensure
the same helper is used wherever initialStoragePermissionGranted was referenced
so permission state always reflects current
Environment.isExternalStorageManager()/ContextCompat checks.
Comment on lines
+196
to
+200
| if (BuildConfig.MODERN_ANDROID) { | ||
| File wxDest = new File(imagefs, "usr/lib/libredirect-bionic-wx.so"); | ||
| FileUtils.copy(ctx, "libredirect-bionic-wx.so", wxDest); | ||
| chmod(wxDest); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'libredirect-bionic-wx\.so'
ast-grep --pattern 'public static boolean copy($_, $_, $_) { $$$ }'
rg -nP '\bcopy\s*\(' app/src/main/java/com/winlator/core/FileUtils.java -C2 || trueRepository: utkarshdalal/GameNative
Length of output: 1688
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the implementation of the Context->assetFile->dstFile copy() overload
# (the one used in ImageFsInstaller.java)
rg -n "public static void copy\(Context context, String assetFile, File dstFile\)" -S app/src/main/java/com/winlator/core/FileUtils.java
sed -n '210,290p' app/src/main/java/com/winlator/core/FileUtils.java
# Find and show chmod helper used by the snippet
rg -n "\bvoid chmod\(" app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java app/src/main/java/com/winlator/core/FileUtils.java || true
rg -n "\bchmod\s*\(" app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java -n
sed -n '1,260p' app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.javaRepository: utkarshdalal/GameNative
Length of output: 15592
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where LD_PRELOAD is constructed and where PRELOAD_BIONIC_SO is referenced
rg -n "BionicProgramLauncherComponent|PRELOAD_BIONIC_SO|LD_PRELOAD" app/src/main/java/com/winlator -SRepository: utkarshdalal/GameNative
Length of output: 5802
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact LD_PRELOAD construction around the reported lines
sed -n '260,360p' app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java
sed -n '660,740p' app/src/main/java/com/winlator/xenvironment/components/BionicProgramLauncherComponent.java
# Locate PRELOAD_BIONIC_SO definition/uses
rg -n "PRELOAD_BIONIC_SO" -S .
# Show BuildConfig generation if present
rg -n "buildConfigField|PRELOAD_BIONIC_SO" -S app build || trueRepository: utkarshdalal/GameNative
Length of output: 11274
Verify modern bionic preload staging and fail loudly if it can’t be staged.
FileUtils.copy(ctx, "libredirect-bionic-wx.so", wxDest) is a void method and swallows IOException (logs only); the installer then only chmods when wxDest.exists(). If the asset copy fails/missing, BionicProgramLauncherComponent still unconditionally appends imageFs.getLibDir() + "/" + BuildConfig.PRELOAD_BIONIC_SO to LD_PRELOAD (and in MODERN_ANDROID, PRELOAD_BIONIC_SO is "libredirect-bionic-wx.so"), pointing the dynamic linker at a non-existent preload on every modern launch. Add a post-copy existence (and ideally non-empty) check and log/abort installGuestLibs (or skip adding the LD_PRELOAD path) when staging fails.
File: app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java
Lines: 196-200
if (BuildConfig.MODERN_ANDROID) {
File wxDest = new File(imagefs, "usr/lib/libredirect-bionic-wx.so");
FileUtils.copy(ctx, "libredirect-bionic-wx.so", wxDest);
chmod(wxDest);
}🤖 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/com/winlator/xenvironment/ImageFsInstaller.java` around
lines 196 - 200, The installer currently calls FileUtils.copy(ctx,
"libredirect-bionic-wx.so", wxDest) (a void method that swallows IOExceptions)
then blindly chmods wxDest; add a post-copy verification in ImageFsInstaller
(the installGuestLibs flow around the FileUtils.copy call) to check that
wxDest.exists() and wxDest.length() > 0, and if the check fails log an error via
the same logger and abort/throw from installGuestLibs (or return a failure) so
the rest of the install does not append
imageFs.getLibDir()+"/"+BuildConfig.PRELOAD_BIONIC_SO to LD_PRELOAD for
MODERN_ANDROID; only chmod(wxDest) when the file verification succeeds.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
8 issues found across 31 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/service/NotificationHelper.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/NotificationHelper.kt:37">
P2: `activeServices` is instance-local, so notification group summary state can desync when multiple services use different `NotificationHelper` instances. Use process-wide shared state (with synchronization) for active service tracking.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/NotificationHelper.kt:113">
P3: Move notification text to `strings.xml`. Hardcoded UI text makes localization and consistency harder.
(Based on your team's feedback about not hardcoding UI strings or colors.) [FEEDBACK_USED]</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/gog/GOGService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/GOGService.kt:733">
P3: This log message is inaccurate: the timeout path calls `stopSelf()` and does not restart the service. Update the text to avoid misleading timeout diagnostics.</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:452">
P2: Modern external path can be skipped from scan list. `allInstallPaths` still gates on `externalStoragePath` string, but new root comes from `baseExternalAppDirPath`. Use resolved root/readiness check instead.</violation>
</file>
<file name="app/src/main/java/com/winlator/core/ProcessHelper.java">
<violation number="1" location="app/src/main/java/com/winlator/core/ProcessHelper.java:134">
P2: Modern linker wrap only in one launch path. `startProcess()` still skips it. This can make modern builds behave different depending on which helper is called. Apply same command normalization in both helpers.</violation>
</file>
<file name="app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java">
<violation number="1" location="app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java:196">
P2: New modern shim only installs on full image update. Version not bumped, so existing v28 installs can miss `libredirect-bionic-wx.so`. Bump image version or add migration path.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt:1039">
P2: Guard this settings launch. Some devices have no handler for this action, so launch can crash. Add fallback path when app-specific all-files screen is missing.</violation>
</file>
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
| * external app dir isn't available yet (e.g. before populateDownloadService runs). | ||
| */ | ||
| private val externalAppInstallRoot: String | ||
| get() = if (BuildConfig.MODERN_ANDROID && DownloadService.baseExternalAppDirPath.isNotBlank()) { |
Contributor
There was a problem hiding this comment.
P2: Modern external path can be skipped from scan list. allInstallPaths still gates on externalStoragePath string, but new root comes from baseExternalAppDirPath. Use resolved root/readiness check instead.
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/service/SteamService.kt, line 452:
<comment>Modern external path can be skipped from scan list. `allInstallPaths` still gates on `externalStoragePath` string, but new root comes from `baseExternalAppDirPath`. Use resolved root/readiness check instead.</comment>
<file context>
@@ -440,8 +441,22 @@ class SteamService : Service(), IChallengeUrlChanged {
+ * external app dir isn't available yet (e.g. before populateDownloadService runs).
+ */
+ private val externalAppInstallRoot: String
+ get() = if (BuildConfig.MODERN_ANDROID && DownloadService.baseExternalAppDirPath.isNotBlank()) {
+ DownloadService.baseExternalAppDirPath + "/files"
+ } else {
</file context>
| int pid = -1; | ||
| java.lang.Process process = null; | ||
| try { | ||
| if (BuildConfig.MODERN_ANDROID) command = "/system/bin/linker64 " + command; |
Contributor
There was a problem hiding this comment.
P2: Modern linker wrap only in one launch path. startProcess() still skips it. This can make modern builds behave different depending on which helper is called. Apply same command normalization in both helpers.
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/ProcessHelper.java, line 134:
<comment>Modern linker wrap only in one launch path. `startProcess()` still skips it. This can make modern builds behave different depending on which helper is called. Apply same command normalization in both helpers.</comment>
<file context>
@@ -129,6 +131,7 @@ public static int exec(String command, String[] envp, File workingDir, Callback<
int pid = -1;
java.lang.Process process = null;
try {
+ if (BuildConfig.MODERN_ANDROID) command = "/system/bin/linker64 " + command;
Log.d("ProcessHelper", "Executing: " + Arrays.toString(splitCommand(command)) + ", " + Arrays.toString(envp) + ", " + workingDir);
process = Runtime.getRuntime().exec(splitCommand(command), envp, workingDir);
</file context>
Owner
Author
There was a problem hiding this comment.
what do you mean? Which other helper?
Contributor
There was a problem hiding this comment.
@utkarshdalal Ah, sorry — I was a bit vague there. I meant the startProcess() method in the same file. Here's the situation:
You added the linker64 prefix in exec() (line 134), but startProcess() (around line 173) also launches commands via Runtime.getRuntime().exec() and doesn't have it:
public static java.lang.Process startProcess(String command, String[] envp, File workingDir) {
try {
Log.d("ProcessHelper", "Executing: " + Arrays.toString(splitCommand(command)) + ...);
java.lang.Process process = Runtime.getRuntime().exec(splitCommand(command), envp, workingDir);
...
}
}If any caller on a modern build goes through startProcess() instead of exec(), the command won't get the /system/bin/linker64 prefix, and it could behave differently (or fail) on Android 14+. So either both methods should apply the same normalization, or you should have a single internal helper they both delegate to.
Tip: get faster answers by chatting with cubic’s review copilot in the review UI
| // Modern flavor ships an additional bionic preload shipped as a flat asset | ||
| // (src/modern/assets/) until it's folded into redirect.tzst. Copy it next to | ||
| // the tarball-extracted variant so BionicProgramLauncherComponent can find it. | ||
| if (BuildConfig.MODERN_ANDROID) { |
Contributor
There was a problem hiding this comment.
P2: New modern shim only installs on full image update. Version not bumped, so existing v28 installs can miss libredirect-bionic-wx.so. Bump image version or add migration path.
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/xenvironment/ImageFsInstaller.java, line 196:
<comment>New modern shim only installs on full image update. Version not bumped, so existing v28 installs can miss `libredirect-bionic-wx.so`. Bump image version or add migration path.</comment>
<file context>
@@ -189,6 +190,15 @@ private static void installGuestLibs(Context ctx) {
+ // Modern flavor ships an additional bionic preload shipped as a flat asset
+ // (src/modern/assets/) until it's folded into redirect.tzst. Copy it next to
+ // the tarball-extracted variant so BionicProgramLauncherComponent can find it.
+ if (BuildConfig.MODERN_ANDROID) {
+ File wxDest = new File(imagefs, "usr/lib/libredirect-bionic-wx.so");
+ FileUtils.copy(ctx, "libredirect-bionic-wx.so", wxDest);
</file context>
| ), | ||
| ) | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) { | ||
| val intent = Intent(Settings.ACTION_MANAGE_APP_ALL_FILES_ACCESS_PERMISSION).apply { |
Contributor
There was a problem hiding this comment.
P2: Guard this settings launch. Some devices have no handler for this action, so launch can crash. Add fallback path when app-specific all-files screen is missing.
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/ui/screen/library/appscreen/SteamAppScreen.kt, line 1039:
<comment>Guard this settings launch. Some devices have no handler for this action, so launch can crash. Add fallback path when app-specific all-files screen is missing.</comment>
<file context>
@@ -1007,12 +1035,19 @@ class SteamAppScreen : BaseAppScreen() {
- ),
- )
+ if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) {
+ val intent = Intent(Settings.ACTION_MANAGE_APP_ALL_FILES_ACCESS_PERMISSION).apply {
+ data = Uri.parse("package:${context.packageName}")
+ }
</file context>
| private val notificationManager: NotificationManager = | ||
| context.getSystemService(NOTIFICATION_SERVICE) as NotificationManager | ||
|
|
||
| private val activeServices = mutableSetOf<Int>() |
Contributor
There was a problem hiding this comment.
P2: activeServices is instance-local, so notification group summary state can desync when multiple services use different NotificationHelper instances. Use process-wide shared state (with synchronization) for active service tracking.
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/service/NotificationHelper.kt, line 37:
<comment>`activeServices` is instance-local, so notification group summary state can desync when multiple services use different `NotificationHelper` instances. Use process-wide shared state (with synchronization) for active service tracking.</comment>
<file context>
@@ -20,18 +20,34 @@ class NotificationHelper @Inject constructor(@ApplicationContext private val con
private val notificationManager: NotificationManager =
context.getSystemService(NOTIFICATION_SERVICE) as NotificationManager
+ private val activeServices = mutableSetOf<Int>()
+
init {
</file context>
|
|
||
| private fun buildSummary(): Notification = buildNotification( | ||
| title = context.getString(R.string.app_name), | ||
| content = "Connected", |
Contributor
There was a problem hiding this comment.
P3: Move notification text to strings.xml. Hardcoded UI text makes localization and consistency harder.
(Based on your team's feedback about not hardcoding UI strings or colors.)
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/service/NotificationHelper.kt, line 113:
<comment>Move notification text to `strings.xml`. Hardcoded UI text makes localization and consistency harder.
(Based on your team's feedback about not hardcoding UI strings or colors.) </comment>
<file context>
@@ -45,16 +61,60 @@ class NotificationHelper @Inject constructor(@ApplicationContext private val con
+
+ private fun buildSummary(): Notification = buildNotification(
+ title = context.getString(R.string.app_name),
+ content = "Connected",
+ isSummary = true,
+ )
</file context>
|
|
||
| override fun onTimeout(startId: Int, fgsType: Int) { | ||
| super.onTimeout(startId, fgsType) | ||
| Timber.w("[GOGService] Foreground service timeout reached, restarting...") |
Contributor
There was a problem hiding this comment.
P3: This log message is inaccurate: the timeout path calls stopSelf() and does not restart the service. Update the text to avoid misleading timeout diagnostics.
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/service/gog/GOGService.kt, line 733:
<comment>This log message is inaccurate: the timeout path calls `stopSelf()` and does not restart the service. Update the text to avoid misleading timeout diagnostics.</comment>
<file context>
@@ -722,6 +728,12 @@ class GOGService : Service() {
+ override fun onTimeout(startId: Int, fgsType: Int) {
+ super.onTimeout(startId, fgsType)
+ Timber.w("[GOGService] Foreground service timeout reached, restarting...")
+ stopSelf()
+ }
</file context>
Suggested change
| Timber.w("[GOGService] Foreground service timeout reached, restarting...") | |
| Timber.w("[GOGService] Foreground service timeout reached, stopping service") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Recording
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
Adds Android 29–36 support by splitting into legacy and modern flavors, updating runtime shims, and fixing storage and foreground-service behavior. Also fixes gesture-back on modern and routes the notification “Exit” action through a broadcast to avoid FGS timeouts/crashes.
New Features
legacy(targetSdk 28) andmodern(minSdk 29, targetSdk 36) flavors withBuildConfigflags (MODERN_ANDROID,PRELOAD_BIONIC_SO); modern uses updatedlibredirect-bionic-wx.so, prefixes commands with/system/bin/linker64, setsREDIRECT_EXEC__PROC_SELF_EXE, and adds Wine/libtoLD_LIBRARY_PATH(applied to:appand:ubuntufs).FOREGROUND_SERVICE_TYPE_DATA_SYNC, FGS timeout handled viaonTimeout, and the “Exit” action now goes through aBroadcastReceiverto avoid FGS start-in-time crashes.legacyandmodern(modern Robolectric pinned to SDK 34); releases attach both APKs and include separate legacy/modern links with a modern incompatibility note.Compatibility
MANAGE_EXTERNAL_STORAGEwithREAD/WRITEgated atmaxSdkVersion=29. UI routes to All Files Access settings on Android 11+, and requests runtime perms on older versions.registerReceiverusesRECEIVER_NOT_EXPORTED; manifest addsqueriesfor APK VIEW intent;compileSdk36 with v35 theme tweaks.modernBuild, Compatibility requests also sendmodernBuild, and tests skip GLIBC cases on modern.Written for commit 99067af. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Chores