fix: SD card games unrecoverable when external storage toggle off#809
Open
jeremybernstein wants to merge 1 commit intoutkarshdalal:masterfrom
Open
fix: SD card games unrecoverable when external storage toggle off#809jeremybernstein wants to merge 1 commit intoutkarshdalal:masterfrom
jeremybernstein wants to merge 1 commit intoutkarshdalal:masterfrom
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaces per-volume install-path scanning with a single aggregated list Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
394-402: Consider filtering out invalid paths whenexternalStoragePathis empty.When
PrefManager.externalStoragePathis empty (the exact scenario this PR fixes),externalAppInstallPathevaluates to"Steam/steamapps/common"— a relative path. This gets included inallInstallPathsand could cause:
- Unnecessary filesystem checks against invalid/relative paths
- Potential false positives if a relative path accidentally exists
♻️ Suggested fix to filter invalid paths
// all install paths: internal + configured external + all mounted volumes val allInstallPaths: List<String> get() { - val paths = mutableListOf(internalAppInstallPath, externalAppInstallPath) + val paths = mutableListOf(internalAppInstallPath) + if (PrefManager.externalStoragePath.isNotEmpty()) { + paths += externalAppInstallPath + } for (volPath in DownloadService.externalVolumePaths) { paths += Paths.get(volPath, "Steam", "steamapps", "common").pathString } return paths }🤖 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 394 - 402, The allInstallPaths getter currently includes externalAppInstallPath and mounted volumes even when PrefManager.externalStoragePath is empty, producing a relative path like "Steam/steamapps/common"; update the getter to only add externalAppInstallPath if it is not blank and resolves to an absolute path (or to a valid existing directory) and skip any blank/invalid entries from DownloadService.externalVolumePaths when building paths; reference the allInstallPaths getter, externalAppInstallPath, internalAppInstallPath, and DownloadService.externalVolumePaths and implement simple checks (e.g., string.isNotBlank() and Paths.get(...).isAbsolute or toFile().exists()) before adding each path.
🤖 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/main/java/app/gamenative/service/SteamService.kt`:
- Around line 394-402: The allInstallPaths getter currently includes
externalAppInstallPath and mounted volumes even when
PrefManager.externalStoragePath is empty, producing a relative path like
"Steam/steamapps/common"; update the getter to only add externalAppInstallPath
if it is not blank and resolves to an absolute path (or to a valid existing
directory) and skip any blank/invalid entries from
DownloadService.externalVolumePaths when building paths; reference the
allInstallPaths getter, externalAppInstallPath, internalAppInstallPath, and
DownloadService.externalVolumePaths and implement simple checks (e.g.,
string.isNotBlank() and Paths.get(...).isAbsolute or toFile().exists()) before
adding each path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 229b1219-0ba4-4751-926b-9c87ba8b58bb
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/service/DownloadService.ktapp/src/main/java/app/gamenative/service/SteamService.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 760-768: getAppDirPath currently returns the first existing
directory it sees (iterating allInstallPaths for appName/oldName), which can
pick a stale/partial folder; change the logic to scan allInstallPaths, collect
all existing candidate directories (using the same checks for appName and
oldName) and prefer a candidate that contains a completion marker (e.g., the
Steam install manifest file appmanifest_<appId>.acf if you have access to appId,
or at least a game executable or other known game data file), falling back to
the first candidate only if no completed marker is found; update related callers
like isAppInstalled() and deleteApp() to rely on the new getAppDirPath
semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a39f0a7-b7de-4dd9-9b9e-26131ac95cd2
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/service/SteamService.kt
Comment on lines
+760
to
+768
| // search internal, configured external, and all mounted volumes | ||
| for (basePath in allInstallPaths) { | ||
| val path = Paths.get(basePath, appName) | ||
| if (Files.exists(path)) return path.pathString | ||
| if (oldName.isNotEmpty()) { | ||
| val oldPath = Paths.get(basePath, oldName) | ||
| if (Files.exists(oldPath)) return oldPath.pathString | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Prefer completed installs over the first existing directory.
Line 763 and Line 766 return the first directory that exists. If an earlier-scanned volume has a stale/partial folder and a later volume has the real install, getAppDirPath() will resolve to the wrong copy, and isAppInstalled(), deleteApp(), etc. will act on that shadow directory instead of the playable install. With multi-volume scanning, this edge case becomes much more likely.
💡 Suggested fix
- for (basePath in allInstallPaths) {
- val path = Paths.get(basePath, appName)
- if (Files.exists(path)) return path.pathString
- if (oldName.isNotEmpty()) {
- val oldPath = Paths.get(basePath, oldName)
- if (Files.exists(oldPath)) return oldPath.pathString
- }
- }
+ val candidates = allInstallPaths.flatMap { basePath ->
+ buildList {
+ add(Paths.get(basePath, appName))
+ if (oldName.isNotEmpty()) {
+ add(Paths.get(basePath, oldName))
+ }
+ }
+ }
+
+ candidates.firstOrNull {
+ Files.exists(it) && MarkerUtils.hasMarker(it.pathString, Marker.DOWNLOAD_COMPLETE_MARKER)
+ }?.let { return it.pathString }
+
+ candidates.firstOrNull { Files.exists(it) }?.let { return it.pathString }🤖 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 760 -
768, getAppDirPath currently returns the first existing directory it sees
(iterating allInstallPaths for appName/oldName), which can pick a stale/partial
folder; change the logic to scan allInstallPaths, collect all existing candidate
directories (using the same checks for appName and oldName) and prefer a
candidate that contains a completion marker (e.g., the Steam install manifest
file appmanifest_<appId>.acf if you have access to appId, or at least a game
executable or other known game data file), falling back to the first candidate
only if no completed marker is found; update related callers like
isAppInstalled() and deleteApp() to rely on the new getAppDirPath semantics.
Contributor
Author
There was a problem hiding this comment.
this is pretty unlikely. we can fix it if someone reports it as a real-world issue.
Contributor
There was a problem hiding this comment.
@jeremybernstein, fair enough — that scenario does require an unlikely combination of conditions (same game directory existing on two volumes simultaneously, with the first scanned being stale). Happy to leave it as-is for now.
( ˘•ω•˘ )
🧠 Learnings used
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 781
File: app/src/main/java/app/gamenative/service/DownloadService.kt:40-44
Timestamp: 2026-03-10T08:42:55.765Z
Learning: In the GameNative app (utkarshdalal/GameNative), games installed on external storage (SD cards, USB) are stored inside the app's `files` directory: `.../Android/data/app.gamenative/files/Steam/steamapps/common`. Therefore, `context.getExternalFilesDirs(null).map { it.absolutePath }` correctly provides the base paths for scanning external volume installs in `DownloadService.kt`. The `parentFile` usage in `baseExternalAppDirPath` is only for the write-path concern (creating siblings of the `files` dir) and is unrelated to scan paths.
Learnt from: utkarshdalal
Repo: utkarshdalal/GameNative PR: 344
File: app/src/main/java/app/gamenative/service/SteamAutoCloud.kt:415-419
Timestamp: 2025-12-17T05:14:05.133Z
Learning: In SteamAutoCloud.kt, when uploading files to Steam Cloud API (beginFileUpload and commitFileUpload), the filename parameter intentionally uses different formats: `file.path + file.filename` (relative path without placeholder) when `appInfo.ufs.saveFilePatterns.isEmpty()` is true (fallback case), and `file.prefixPath` (includes placeholder like %SteamUserData%) when patterns are configured. This difference is by design.
Learnt from: unbelievableflavour
Repo: utkarshdalal/GameNative PR: 698
File: app/src/main/java/app/gamenative/service/gog/GOGManager.kt:796-798
Timestamp: 2026-03-03T11:51:05.324Z
Learning: In GOG game installations at _CommonRedist/ISI/rootdir, the rootdir entry will never exist as a regular file or directory—it either doesn't exist or already exists as a symlink. Therefore, defensive code to handle and remove existing non-symlink entries at that path is unnecessary when creating the scriptinterpreter rootdir symlink in app/src/main/java/app/gamenative/service/gog/GOGManager.kt.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 0
File: :0-0
Timestamp: 2026-03-10T21:56:45.006Z
Learning: In `app/src/main/java/app/gamenative/utils/BestConfigService.kt`, the `available*` lists (availableDxvk, availableVkd3d, availableBox64Bionic, availableBox64Glibc, availableWowBox64, availableFexcore, availableWineBionic, availableWineGlibc, availableDrivers) passed to `validateComponentVersions()` intentionally include manifest entries (base + installed + manifest). This allows configs referencing installable-but-not-yet-on-disk versions to pass validation. A separate subsequent step, `resolveMissingManifestInstallRequests()`, handles actually installing any manifest items not yet present on disk. Do not suggest removing manifest entries from these lists in future reviews.
Learnt from: shahmir-k
Repo: utkarshdalal/GameNative PR: 751
File: app/src/main/java/app/gamenative/provider/InstalledGamesProvider.kt:89-91
Timestamp: 2026-03-08T00:46:13.038Z
Learning: In the `InstalledGamesProvider` (`app/src/main/java/app/gamenative/provider/InstalledGamesProvider.kt`), returning a partial `MatrixCursor` on exception from individual source queries (Steam, GOG, Epic, Amazon) is intentional. If one source (e.g. Epic) fails, callers still receive valid results from the remaining sources. Failing the entire query with an exception is considered a worse outcome for the shortcut-generator use case described in issue `#750`. Errors are logged via Timber for debugging.
Learnt from: shahmir-k
Repo: utkarshdalal/GameNative PR: 751
File: app/src/main/java/app/gamenative/provider/InstalledGamesProvider.kt:140-149
Timestamp: 2026-03-08T02:43:32.765Z
Learning: In `InstalledGamesProvider` (`app/src/main/java/app/gamenative/provider/InstalledGamesProvider.kt`), the column `icon_ref` (renamed from `icon_hash` in commit 5ca5a2c of PR `#751`) is a generic icon reference whose format depends on `game_source`: it is a hash string for STEAM, and a URL for GOG, EPIC, and AMAZON. Callers must check `game_source` to know how to use `icon_ref`.
Learnt from: Catpotatos
Repo: utkarshdalal/GameNative PR: 760
File: app/src/main/java/app/gamenative/utils/ContainerUtils.kt:1039-1048
Timestamp: 2026-03-09T16:05:48.277Z
Learning: In the GameNative app (utkarshdalal/GameNative), all Wine containers are always stored under `context.filesDir/imagefs/home` (which maps to Z:/Home). This is the single fixed location — ImageFs is not mounted elsewhere. Code that scans for container directories (e.g. in ContainerUtils.deleteContainer) can safely hardcode `java.io.File(context.filesDir, "imagefs/home")` as the home directory rather than deriving it from `ImageFs.find(context).getRootDir()`.
Learnt from: shahmir-k
Repo: utkarshdalal/GameNative PR: 751
File: app/src/main/java/app/gamenative/provider/InstalledGamesProvider.kt:84-88
Timestamp: 2026-03-08T00:46:03.962Z
Learning: In the GameNative project, custom/locally-added games (CUSTOM_GAME source) are filesystem-based and are NOT stored in the Room database. They are discovered by `CustomGameScanner`, which performs filesystem I/O and depends on `PrefManager` initialization. As a result, custom games cannot be queried from `InstalledGamesProvider` (the ContentProvider) the same way as Steam, GOG, Epic, and Amazon games, which are all database-backed. Custom game support in the ContentProvider would require first persisting custom game metadata to Room.
Learnt from: unbelievableflavour
Repo: utkarshdalal/GameNative PR: 778
File: app/src/main/java/app/gamenative/service/gog/GOGManager.kt:853-868
Timestamp: 2026-03-10T18:27:06.002Z
Learning: In GOGManager.kt (app/src/main/java/app/gamenative/service/gog/GOGManager.kt), the GOG scriptinterpreter.exe command arguments (e.g., /DIR, /supportDir, /buildId, /versionName, /lang-code) must NOT be quoted even when the values could theoretically contain whitespace. Quoting these arguments breaks scriptinterpreter.exe argument parsing. Raw interpolation (e.g., "/DIR=$dirAndSupport") is the correct form.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 801
File: app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt:174-188
Timestamp: 2026-03-10T22:11:59.430Z
Learning: In `app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt`, the `applyConfigForContainer` function calls `installMissingComponentsForConfig` before `BestConfigService.parseConfigToContainerData`. This install-before-parse ordering is pre-existing behavior, not introduced by any recent PR. Do not flag this ordering as an issue in future reviews of this file.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 0
File: :0-0
Timestamp: 2026-03-10T22:13:54.429Z
Learning: In `app/src/main/java/app/gamenative/utils/BestConfigService.kt`, around lines 403-418, missing Box64/FEXCore preset failures (via `Box86_64PresetManager.getPreset("box64", ...)` and `FEXCorePresetManager.getPreset(...)`) are intentionally appended to the same `missingComponents` list as installable components, even though `resolveMissingManifestInstallRequests()` cannot install presets. This is a known limitation deferred for future work — the long-term direction is to serialize non-default presets into config files and auto-install them. Do not flag this mixing of installable vs. non-installable blockers in `missingComponents` as an issue in future reviews of this file.
Learnt from: unbelievableflavour
Repo: utkarshdalal/GameNative PR: 778
File: app/src/main/java/app/gamenative/service/gog/GOGManager.kt:791-808
Timestamp: 2026-03-10T18:28:23.211Z
Learning: In GOG game installations, the `rootdir` symlink at `_CommonRedist/ISI/rootdir` (in `GOGManager.kt`) will either not exist yet or already be a symlink — it will never be a regular file or directory that could cause `WinlatorFileUtils.symlink` to fail. Therefore, fail-closed logic in `ensureScriptInterpreterRootDirSymlink` (returning false and aborting command building) is unnecessary; logging the exception and continuing is sufficient.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 0
File: :0-0
Timestamp: 2026-03-10T11:01:21.903Z
Learning: In `app/src/main/java/app/gamenative/ui/model/LibraryViewModel.kt` (around lines 371-396), the `Loading` parental state in `SteamService.parentalControlState` intentionally maps to `PrefManager.parentalAllowedAppIds` (which can be null) rather than `emptyList()`. This fail-open behavior on the `Loading` state is by design: on first launch/login the account type (managed child vs. non-child) is unknown, so showing all apps is preferable to incorrectly blocking everything. Subsequent sessions use the PrefManager cache, so restrictions are enforced correctly after the first login. Do not flag this as a security/correctness issue in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 801
File: app/src/main/java/app/gamenative/utils/BestConfigService.kt:45-51
Timestamp: 2026-03-10T21:46:10.135Z
Learning: In `app/src/main/java/app/gamenative/utils/BestConfigService.kt`, the singleton mutable state pattern (`lastMissingComponents` / `consumeLastMissingComponents()`) — where `parseConfigToContainerData()` writes a global and callers read it via `consume*()` — is a pre-existing design. Refactoring it to return missing components as part of the parse result has a large blast radius and is intentionally deferred. Do not flag this pattern as an issue in future reviews of this file.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 581
File: app/src/main/java/app/gamenative/ui/PluviaMain.kt:1154-1176
Timestamp: 2026-02-21T22:26:41.871Z
Learning: In Kotlin coroutines, suspend functions automatically suspend and resume within a coroutine scope. Only Deferred objects produced by async { ... } require .await() to obtain the result. Do not call .await() on direct suspend function calls or on non-Deferred results; use await() only when you have a Deferred that represents a value you need.
jeremybernstein
force-pushed
the
jb/fix-sd-card-getappdirpath
branch
from
64badf1 to
7775fa9
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
394-408: Consider deduplicating paths to avoid redundant I/O.If
PrefManager.externalStoragePathmatches one of the mounted volume paths inDownloadService.externalVolumePaths, the same install path will appear twice in the list. This causes redundant filesystem checks when iterating.♻️ Suggested improvement
val allInstallPaths: List<String> get() { val paths = mutableListOf(internalAppInstallPath) // only include configured external path if it's a real absolute path if (PrefManager.externalStoragePath.isNotBlank()) { paths += externalAppInstallPath } for (volPath in DownloadService.externalVolumePaths) { if (volPath.isNotBlank()) { paths += Paths.get(volPath, "Steam", "steamapps", "common").pathString } } - return paths + return paths.distinct() }🤖 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 394 - 408, The allInstallPaths getter may produce duplicate entries when PrefManager.externalStoragePath equals one of DownloadService.externalVolumePaths; update the logic in the allInstallPaths property to deduplicate the computed paths before returning (e.g., collect into an ordered set or call distinct() on the list) so internalAppInstallPath, externalAppInstallPath and the Paths.get(...) entries from DownloadService.externalVolumePaths are returned once and original order is preserved.
🤖 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/main/java/app/gamenative/service/SteamService.kt`:
- Around line 394-408: The allInstallPaths getter may produce duplicate entries
when PrefManager.externalStoragePath equals one of
DownloadService.externalVolumePaths; update the logic in the allInstallPaths
property to deduplicate the computed paths before returning (e.g., collect into
an ordered set or call distinct() on the list) so internalAppInstallPath,
externalAppInstallPath and the Paths.get(...) entries from
DownloadService.externalVolumePaths are returned once and original order is
preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49ac3af0-9895-48d2-8598-051c4447cb18
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/service/DownloadService.ktapp/src/main/java/app/gamenative/service/SteamService.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/service/DownloadService.kt
getAppDirPath only checked PrefManager.externalStoragePath which is empty when toggle is off — games on SD card couldn't be found, making them unplayable/undeletable/unreinstallable. Add allInstallPaths (internal + configured external + all mounted volumes) and use it in both getAppDirPath and getDownloadDirectoryApps. Skip blank/relative paths and deduplicate to avoid redundant checks.
jeremybernstein
force-pushed
the
jb/fix-sd-card-getappdirpath
branch
from
7775fa9 to
b924ddd
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
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.
Fixes #808
Summary
getAppDirPathusedPrefManager.externalStoragePathwhich is""when toggle is off — games on SD card couldn't be found, making them unplayable/undeletable/unreinstallableallInstallPathsto SteamService (internal + configured external + all mounted volumes) and use it in bothgetAppDirPathandgetDownloadDirectoryApps, consolidating the volume scanning from Fix: scan all external volumes for installed games #781Test plan
Summary by cubic
Fixes #808 by keeping SD card installs playable and manageable when the external storage toggle is off. We now search all valid install paths and dedupe results.
allInstallPathstoSteamService(internal, configured external, all mounted volumes) and skip blank paths.getAppDirPathto searchallInstallPathsand default to the preferred location based on the toggle.DownloadService.getDownloadDirectoryAppsto iterateSteamService.allInstallPaths, deduping directory scans.Written for commit b924ddd. Summary will update on new commits.
Summary by CodeRabbit