Battle.net Integration, Shared Containers, and Legacy Optimizations - #1750
Closed
Meloon33 wants to merge 0 commit into
Closed
Battle.net Integration, Shared Containers, and Legacy Optimizations#1750Meloon33 wants to merge 0 commit into
Meloon33 wants to merge 0 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds Battle.net platform integration (data model, DAO, database migration, auth/sync services, OAuth activity, library UI), a shared-container system for Wine environments with per-app launch overrides and storage-manager UI, custom game master-directory scanning, stability fixes, and documentation/localization updates. ChangesBattle.net Integration
Estimated code review effort: 4 (Complex) | ~60 minutes Shared Containers Feature
Estimated code review effort: 4 (Complex) | ~50 minutes Game Management UX, Stability Fixes, and Localization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant BattleNetOAuthActivity
participant BattleNetAuthManager
participant PlatformOAuthHandlers
participant BattleNetService
participant BattleNetSyncManager
User->>BattleNetOAuthActivity: Initiate login
BattleNetOAuthActivity->>BattleNetAuthManager: saveCookies / saveCredentials
BattleNetOAuthActivity->>PlatformOAuthHandlers: handleBattleNetAuthentication(authCode)
PlatformOAuthHandlers->>BattleNetService: start() / triggerSync()
BattleNetService->>BattleNetSyncManager: syncLibrary(context, dao)
BattleNetSyncManager-->>BattleNetService: Result.success/failure
sequenceDiagram
participant XServerScreen
participant ContainerUtils
participant AppLaunchConfigManager
participant Container
XServerScreen->>ContainerUtils: getOrCreateContainer(appId)
ContainerUtils->>ContainerUtils: calculateSharedContainerId(data)
ContainerUtils->>Container: getContainer(sharedId)
ContainerUtils->>AppLaunchConfigManager: applyToContainerData(appId, data)
AppLaunchConfigManager-->>ContainerUtils: overridden ContainerData
ContainerUtils-->>XServerScreen: Container with per-app overrides
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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.
40 issues found across 78 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/battle_net/BattleNetConstants.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/battle_net/BattleNetConstants.kt:35">
P0: Battle.net connection cannot begin because every authorization request sends the literal `PLACEHOLDER` client ID. Use a registered/configured client ID (and its matching redirect URI) when building this URL.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/battle_net/BattleNetService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/battle_net/BattleNetService.kt:32">
P0: Battle.net connect and refresh crash/fail when starting this service because `BattleNetService` is not declared in the app manifest. Add a non-exported `dataSync` service declaration alongside the other library services.</violation>
</file>
<file name="app/src/main/res/values-da/strings.xml">
<violation number="1" location="app/src/main/res/values-da/strings.xml:1976">
P1: The anti-cheat warning at battle_net_login_warning is untranslated (English) in the Danish locale file. This is a safety/legal notice that Danish users must be able to read. Translate to Danish alongside the 17 other untranslated added strings in this file.</violation>
</file>
<file name="app/src/main/java/app/gamenative/data/BattleNetGame.kt">
<violation number="1" location="app/src/main/java/app/gamenative/data/BattleNetGame.kt:12">
P1: Battle.net launch entries fall back to `Game.exe` because the app ID carries `uid`, but the launcher parses that suffix as `productCode`. Preserve/use the actual product code when constructing or resolving the launch ID so executable mapping works.</violation>
</file>
<file name="app/src/main/java/app/gamenative/utils/ContainerUtils.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/ContainerUtils.kt:456">
P1: Editing one shared game's settings also overwrites the shared prefix configuration. `saveConfig` does not intercept the subsequent mutation and `container.saveData()`, so later games inherit settings meant only for this app; return/use a separate per-app-only save path before applying persistent container changes.</violation>
</file>
<file name="app/src/main/java/app/gamenative/utils/GameFeedbackUtils.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/GameFeedbackUtils.kt:70">
P1: Submitting "Battle.net Game" as the game name renders the feedback feature non-functional for all Battle.net titles. The API records every report under this generic string, so users cannot distinguish performance data for Diablo III vs WoW vs StarCraft II. Implement a real name lookup (e.g., via BattleNetGameDao.getByUid()) like the Steam/GOG/Epic/Amazon branches do, or remove the placeholder and handle the missing-name case.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:3612">
P1: Bionic containers now kill `services.exe` on their default startup path. Preserve the Bionic fallback to `STARTUP_SELECTION_ESSENTIAL`; only Glibc should send this kill request.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:3960">
P1: First launch of a Steam title can persist its executable path into the shared container, causing another occupant without a saved override to launch that title. When shared containers are enabled, save only `AppLaunchConfigManager` here and skip `container.saveData()`.</violation>
</file>
<file name="app/src/main/res/values-zh-rTW/strings.xml">
<violation number="1" location="app/src/main/res/values-zh-rTW/strings.xml:2111">
P1: All 18 newly added strings in the Traditional Chinese (Taiwan) locale file are untranslated — they appear in English while every surrounding string in this file is properly localized. Users in this locale will see English UI text for Battle.net integration, Shared Containers, and Storage Manager features.</violation>
</file>
<file name="app/src/main/res/values-zh-rCN/strings.xml">
<violation number="1" location="app/src/main/res/values-zh-rCN/strings.xml:2123">
P2: Translate these Battle.net login UI strings to Chinese Simplified following the Steam string pattern (brand name kept, surrounding text in Chinese).</violation>
<violation number="2" location="app/src/main/res/values-zh-rCN/strings.xml:2129">
P1: Translate this placeholder instructional text to Chinese Simplified. Users navigating a Chinese-language UI will see English guidance.</violation>
<violation number="3" location="app/src/main/res/values-zh-rCN/strings.xml:2137">
P2: Translate these Storage Manager UI strings to Chinese Simplified. Users in a Chinese locale will see English button labels and empty-state text.</violation>
</file>
<file name="app/src/main/res/values-es/strings.xml">
<violation number="1" location="app/src/main/res/values-es/strings.xml:2103">
P1: 14 of the 16 new strings in this Spanish locale file are in English, not Spanish. Users with es locale will see English UI text for the new Battle.net and Shared Containers features: login dialogs, warning messages, storage badges, and manager labels all remain untranslated. Translate each string to Spanish before merging.</violation>
</file>
<file name="app/src/main/res/values-pt-rBR/strings.xml">
<violation number="1" location="app/src/main/res/values-pt-rBR/strings.xml:1972">
P1: All 18 newly added strings are in English — this is the Brazilian Portuguese locale file, so they need translations.
The existing strings here follow the correct pattern ("Coleções da Steam", "Limpar", "Adicione ou jogue alguns jogos"), but the Battle.net, Shared Containers, and Storage Manager strings were copied verbatim from the default values/strings.xml without localization.
Translate each string to Brazilian Portuguese to match the rest of the file. For example: `battle_net_login_title` → "Conectar Battle.net", `container_storage_shared_badge` → "Compartilhado", etc.</violation>
</file>
<file name="app/src/main/res/values-de/strings.xml">
<violation number="1" location="app/src/main/res/values-de/strings.xml:2042">
P1: Alle 18 neu hinzugefügten Zeichenketten in der deutschen Lokalisierung sind auf Englisch statt übersetzt. Das betrifft Shared Containers-Einstellungen, Storage-Manager-Beschriftungen und Battle.net-Fluss.</violation>
</file>
<file name="app/src/main/res/values-uk/strings.xml">
<violation number="1" location="app/src/main/res/values-uk/strings.xml:2096">
P1: New strings added to the Ukrainian locale are in English rather than Ukrainian. Ukrainian users will see untranslated English text for all new Battle.net, Shared Containers, and Storage Manager features. The PR claims full 14-language support, but the Ukrainian file still needs translations for these 18 strings.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/auth/BattleNetOAuthActivity.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/auth/BattleNetOAuthActivity.kt:62">
P1: OAuth callbacks are accepted without validating `state`, allowing an authorization-response CSRF to link the session to a code not initiated by this login flow. Compare the callback state with `oauthState` before extracting or returning the code.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/settings/ContainerStorageManagerDialog.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/settings/ContainerStorageManagerDialog.kt:380">
P1: Storage tab now stays empty until user presses Refresh: standalone `ContainerStorageManagerContent` no longer starts its inventory load, while `HomeDownloadsScreen` does not load its state. Restore loading in the shared content or trigger it from that caller.</violation>
</file>
<file name=".artifacts/08f1994d-07c3-4379-93c3-78af500e4868/walkthrough_legacy_opt.artifact.md">
<violation number="1" location=".artifacts/08f1994d-07c3-4379-93c3-78af500e4868/walkthrough_legacy_opt.artifact.md:8">
P1: The walkthrough describes using `System.gc()` and `System.runFinalization()` in `PluviaApp.shutdownEnvironment` to reclaim native X11 memory after game exit. On Android, `System.gc()` is only a hint that ART/Dalvik may ignore, and calling it explicitly is strongly discouraged in production because it can block the UI thread without guaranteeing cleanup. `System.runFinalization()` is deprecated for removal (JDK 18+) and also unreliable. Neither method directly reclaims native (non-Java-heap) memory used by the X11 environment — native resources must be released through explicit teardown in the native layer. If this has already been implemented, it adds unnecessary GC pressure without achieving the stated goal; if it's still planned, consider removing both calls and using explicit native resource cleanup instead.</violation>
</file>
<file name="app/src/main/java/app/gamenative/utils/CustomGameScanner.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/CustomGameScanner.kt:518">
P1: Removing a master directory can leave its previously discovered games launchable/resolved because `CustomGameCache` neither indexes nor invalidates master-folder occupants. Include discovered master folders in cache construction and cache-key invalidation so `getFolderPathFromAppId` reflects current settings.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/utils/CustomGameScanner.kt:526">
P2: Large master directories can stall or exhaust memory during library refresh because this eagerly loads every child, then enumerates each candidate directory twice. Move discovery off the UI-building path and use bounded/streamed scanning or a persisted scan cache.</violation>
</file>
<file name=".artifacts/08f1994d-07c3-4379-93c3-78af500e4868/walkthrough_game_mgmt_opt.artifact.md">
<violation number="1" location=".artifacts/08f1994d-07c3-4379-93c3-78af500e4868/walkthrough_game_mgmt_opt.artifact.md:13">
P1: Terminating `services.exe` is unsafe and risks destabilizing the entire Wine prefix. This process is the Service Control Manager — killing it breaks IPC, can crash dependent services (plug-and-play, rpcss), and may leave the prefix in an inconsistent state. Consider safer strategies: set Wine's `DisableProcessWide` optimizations, limit background processes via the prefix config, or stall on `wineserver -w` to let children settle, rather than killing a critical system process.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/PluviaMain.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/PluviaMain.kt:1653">
P1: The "Game.exe" fallback for Battle.net games bypasses the executable-not-found error dialog. If `container.executablePath` is empty, the code attempts to launch a non-existent file instead of showing the graceful EXECUTABLE_NOT_FOUND dialog that all other platforms use. Remove the `ifEmpty { ... }` fallback so missing executables trigger the proper error dialog.</violation>
</file>
<file name="implementation_plan.artifact.md">
<violation number="1" location="implementation_plan.artifact.md:32">
P1: Occupancy checking must scan Battle.net database too, or shared containers used by Battle.net games could be deleted when the last non-Battle.net occupant is removed. The plan lists Amazon but omits Battle.net despite the PR introducing Battle.net as a new platform. Add `Battle Net` to the platform scan list.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/battle_net/BattleNetAuthManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/battle_net/BattleNetAuthManager.kt:24">
P2: Credential-only state is shown as logged in, but every library sync fails because sync authenticates exclusively with `loadCookies`. Use stored OAuth credentials in the sync flow, or only report authentication when a usable cookie session exists.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/battle_net/BattleNetAuthManager.kt:83">
P1: Signing out and connecting a different Battle.net account can expose titles retained from the prior account. Clear or account-scope Battle.net library rows as part of logout/account switching while preserving any required local install metadata.</violation>
<violation number="3" location="app/src/main/java/app/gamenative/service/battle_net/BattleNetAuthManager.kt:90">
P2: A user can start a new login before WebView cookie removal completes and be silently reauthenticated as the logged-out account. Surface completion from `removeAllCookies`, then flush cookies and enable a new login only after that callback.</violation>
</file>
<file name="app/src/main/res/values-ro/strings.xml">
<violation number="1" location="app/src/main/res/values-ro/strings.xml:2103">
P2: Localize these Shared Containers setting strings to Romanian. The values are in English in the `values-ro` locale file; users will see untranslated UI text.</violation>
<violation number="2" location="app/src/main/res/values-ro/strings.xml:2105">
P2: Localize all Battle.net-related strings to Romanian in this `values-ro` resource file. Users will see English text for login actions, warnings, and status messages.</violation>
<violation number="3" location="app/src/main/res/values-ro/strings.xml:2113">
P2: Localize these Storage Manager container strings to Romanian. Entries like 'Shared', 'Used by:', 'Remove Container', and 'No storage entries found' will display in English to Romanian users.</violation>
</file>
<file name="app/src/main/java/app/gamenative/db/dao/BattleNetGameDao.kt">
<violation number="1" location="app/src/main/java/app/gamenative/db/dao/BattleNetGameDao.kt:41">
P2: `upsertPreservingInstallStatus` uses an N+1 query pattern — one `getByUid` + one `insert` per game in a loop. For a library sync of 60+ Battle.net titles, this produces 120 sequential DB operations. The Epic and Amazon DAOs already use an optimized pattern: batch-fetch all existing records with a single `IN` query, map in memory, then call `insertAll` once. Apply the same approach here to keep sync fast for large libraries.</violation>
</file>
<file name="app/src/main/res/values-ko/strings.xml">
<violation number="1" location="app/src/main/res/values-ko/strings.xml:2104">
P2: All 18 new strings in values-ko/strings.xml are in English instead of Korean. Every other string in this Korean locale file is properly translated — users will see English labels, warnings, and messages for Shared Containers and Battle.net features. Translate these entries to match the rest of the file so Korean users get a fully localized experience.</violation>
</file>
<file name="app/src/main/java/app/gamenative/utils/AppLaunchConfigManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/AppLaunchConfigManager.kt:38">
P2: A game's per-app XInput setting is discarded: `inputType` is written but never loaded or applied. Shared games therefore inherit the container's `enableXInput` value rather than their own input configuration; model and copy this boolean explicitly.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt:1014">
P2: Connecting from the Battle.net tab bypasses the mandatory anti-cheat warning shown by the System Menu path. Route this action through the same acknowledgement state before launching OAuth.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt:1287">
P2: After Battle.net logout, synced games remain in the database and therefore remain visible in the All library until another filtering event occurs. Use a logout flow that stops the service and clears or re-filters Battle.net library entries.</violation>
</file>
<file name="app/src/main/java/app/gamenative/utils/ContainerStorageManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/ContainerStorageManager.kt:617">
P2: Installed Battle.net entries expose Uninstall, but clicking it always fails as an unknown source because `detectGameSource` does not recognize `BATTLE_NET_` IDs. Hide this action until deletion is implemented, or add Battle.net ID detection and a real delete path.</violation>
</file>
<file name="app/src/main/res/values-pl/strings.xml">
<violation number="1" location="app/src/main/res/values-pl/strings.xml:2100">
P2: All 17 new string entries in the Polish locale file are in English. Users with the Polish locale will see English UI text for Battle.net auth flows, Shared Container settings, and Storage Manager features. Translate each string to Polish so the full localization matches the rest of the file.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/library/appscreen/BattleNetAppScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/library/appscreen/BattleNetAppScreen.kt:49">
P1: loadContainerData() returns a bare empty ContainerData() instead of using ContainerUtils.getOrCreateContainer/toContainerData like every other AppScreen. No container is created on disk, so any caller reading fields like language, rootDir, or environmentVars gets empty defaults.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/battle_net/BattleNetSyncManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/battle_net/BattleNetSyncManager.kt:36">
P2: Failed Battle.net sync responses can leak network resources because the OkHttp `Response` is not closed before the early return. Wrapping the execute block in `use {}` ensures both success and error paths release the connection cleanly.</violation>
</file>
<file name="app/src/main/res/values-ru/strings.xml">
<violation number="1" location="app/src/main/res/values-ru/strings.xml:2031">
P2: 15 of 17 new strings in the Russian locale file are in English, not Russian. The PR claims 'Full localization across 14 languages' but the Battle.net login flow, anti-cheat warning, library prompt, and container storage UI (badges, dialogs, manager title) will all display in English to Russian-speaking users. Existing strings in the same file follow proper Russian translation — these new ones were left untranslated. Either provide Russian translations before merging or update the PR description to reflect placeholder status.</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
| // Note: Battle.net requires a valid client_id. | ||
| // For individual integration, users may need to provide their own. | ||
| // We'll use a placeholder and allow the user to see the login page. | ||
| val url = "$AUTH_URL?client_id=PLACEHOLDER&redirect_uri=$REDIRECT_URI&response_type=code&state=$state" |
Contributor
There was a problem hiding this comment.
P0: Battle.net connection cannot begin because every authorization request sends the literal PLACEHOLDER client ID. Use a registered/configured client ID (and its matching redirect URI) when building this URL.
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/battle_net/BattleNetConstants.kt, line 35:
<comment>Battle.net connection cannot begin because every authorization request sends the literal `PLACEHOLDER` client ID. Use a registered/configured client ID (and its matching redirect URI) when building this URL.</comment>
<file context>
@@ -0,0 +1,38 @@
+ // Note: Battle.net requires a valid client_id.
+ // For individual integration, users may need to provide their own.
+ // We'll use a placeholder and allow the user to see the login page.
+ val url = "$AUTH_URL?client_id=PLACEHOLDER&redirect_uri=$REDIRECT_URI&response_type=code&state=$state"
+ return url to state
+ }
</file context>
| fun start(context: Context) { | ||
| if (isRunning) return | ||
| val intent = Intent(context, BattleNetService::class.java) | ||
| context.startForegroundService(intent) |
Contributor
There was a problem hiding this comment.
P0: Battle.net connect and refresh crash/fail when starting this service because BattleNetService is not declared in the app manifest. Add a non-exported dataSync service declaration alongside the other library services.
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/battle_net/BattleNetService.kt, line 32:
<comment>Battle.net connect and refresh crash/fail when starting this service because `BattleNetService` is not declared in the app manifest. Add a non-exported `dataSync` service declaration alongside the other library services.</comment>
<file context>
@@ -0,0 +1,76 @@
+ fun start(context: Context) {
+ if (isRunning) return
+ val intent = Intent(context, BattleNetService::class.java)
+ context.startForegroundService(intent)
+ }
+
</file context>
| <string name="settings_emulation_shared_containers_subtitle">Allow games with matching system configurations to share the same container directory to save storage space (experimental)</string> | ||
| <string name="tab_battle_net">Battle.net</string> | ||
| <string name="battle_net_login_title">Connect Battle.net</string> | ||
| <string name="battle_net_login_warning">CAUTION: Battle.net anti-cheat (Warden/Ricochet) may flag emulation as unauthorized software. Online play carries a HIGH RISK of account bans. Use at your own risk.</string> |
Contributor
There was a problem hiding this comment.
P1: The anti-cheat warning at battle_net_login_warning is untranslated (English) in the Danish locale file. This is a safety/legal notice that Danish users must be able to read. Translate to Danish alongside the 17 other untranslated added strings in this file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/res/values-da/strings.xml, line 1976:
<comment>The anti-cheat warning at battle_net_login_warning is untranslated (English) in the Danish locale file. This is a safety/legal notice that Danish users must be able to read. Translate to Danish alongside the 17 other untranslated added strings in this file.</comment>
<file context>
@@ -1969,4 +1969,22 @@
+ <string name="settings_emulation_shared_containers_subtitle">Allow games with matching system configurations to share the same container directory to save storage space (experimental)</string>
+ <string name="tab_battle_net">Battle.net</string>
+ <string name="battle_net_login_title">Connect Battle.net</string>
+ <string name="battle_net_login_warning">CAUTION: Battle.net anti-cheat (Warden/Ricochet) may flag emulation as unauthorized software. Online play carries a HIGH RISK of account bans. Use at your own risk.</string>
+ <string name="battle_net_login_success">Successfully connected to Battle.net</string>
+ <string name="battle_net_login_fail">Failed to connect to Battle.net</string>
</file context>
| data class BattleNetGame( | ||
| @PrimaryKey | ||
| @ColumnInfo("uid") | ||
| val uid: String, // Internal Blizzard UID |
Contributor
There was a problem hiding this comment.
P1: Battle.net launch entries fall back to Game.exe because the app ID carries uid, but the launcher parses that suffix as productCode. Preserve/use the actual product code when constructing or resolving the launch ID so executable mapping works.
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/data/BattleNetGame.kt, line 12:
<comment>Battle.net launch entries fall back to `Game.exe` because the app ID carries `uid`, but the launcher parses that suffix as `productCode`. Preserve/use the actual product code when constructing or resolving the launch ID so executable mapping works.</comment>
<file context>
@@ -0,0 +1,50 @@
+data class BattleNetGame(
+ @PrimaryKey
+ @ColumnInfo("uid")
+ val uid: String, // Internal Blizzard UID
+
+ @ColumnInfo("title")
</file context>
| // If shared containers are enabled, we might want to save the app-specific config elsewhere | ||
| val appId = extractAppIdFromContainer(container) // Helper to get the actual game ID if possible | ||
| if (appId != null && saveToDisk && PrefManager.sharedContainersEnabled) { | ||
| AppLaunchConfigManager.saveConfig(context, appId, containerData) |
Contributor
There was a problem hiding this comment.
P1: Editing one shared game's settings also overwrites the shared prefix configuration. saveConfig does not intercept the subsequent mutation and container.saveData(), so later games inherit settings meant only for this app; return/use a separate per-app-only save path before applying persistent container changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/utils/ContainerUtils.kt, line 456:
<comment>Editing one shared game's settings also overwrites the shared prefix configuration. `saveConfig` does not intercept the subsequent mutation and `container.saveData()`, so later games inherit settings meant only for this app; return/use a separate per-app-only save path before applying persistent container changes.</comment>
<file context>
@@ -449,6 +449,13 @@ object ContainerUtils {
+ // If shared containers are enabled, we might want to save the app-specific config elsewhere
+ val appId = extractAppIdFromContainer(container) // Helper to get the actual game ID if possible
+ if (appId != null && saveToDisk && PrefManager.sharedContainersEnabled) {
+ AppLaunchConfigManager.saveConfig(context, appId, containerData)
+ Timber.d("[ContainerUtils]: Intercepted save to shared container; redirected to per-app config for $appId")
+ }
</file context>
| for (master in masterFolders) { | ||
| val root = File(master) | ||
| if (root.exists() && root.isDirectory) { | ||
| root.listFiles()?.filter { it.isDirectory }?.forEach { subDir -> |
Contributor
There was a problem hiding this comment.
P2: Large master directories can stall or exhaust memory during library refresh because this eagerly loads every child, then enumerates each candidate directory twice. Move discovery off the UI-building path and use bounded/streamed scanning or a persisted scan cache.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/utils/CustomGameScanner.kt, line 526:
<comment>Large master directories can stall or exhaust memory during library refresh because this eagerly loads every child, then enumerates each candidate directory twice. Move discovery off the UI-building path and use bounded/streamed scanning or a persisted scan cache.</comment>
<file context>
@@ -515,18 +515,35 @@ object CustomGameScanner {
+ for (master in masterFolders) {
+ val root = File(master)
+ if (root.exists() && root.isDirectory) {
+ root.listFiles()?.filter { it.isDirectory }?.forEach { subDir ->
+ // Check if subDir contains an exe
+ if (findAllValidExeFiles(subDir).isNotEmpty()) {
</file context>
| }, | ||
| onBattleNetLogoutClick = { | ||
| // battleNetLogout(context, lifecycleScope) | ||
| app.gamenative.service.battle_net.BattleNetAuthManager.logout(context) |
Contributor
There was a problem hiding this comment.
P2: After Battle.net logout, synced games remain in the database and therefore remain visible in the All library until another filtering event occurs. Use a logout flow that stops the service and clears or re-filters Battle.net library entries.
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/LibraryScreen.kt, line 1287:
<comment>After Battle.net logout, synced games remain in the database and therefore remain visible in the All library until another filtering event occurs. Use a logout flow that stops the service and clears or re-filters Battle.net library entries.</comment>
<file context>
@@ -1234,6 +1279,13 @@ private fun LibraryScreenContent(
+ },
+ onBattleNetLogoutClick = {
+ // battleNetLogout(context, lifecycleScope)
+ app.gamenative.service.battle_net.BattleNetAuthManager.logout(context)
+ },
)
</file context>
| if (cookiesFile.exists()) cookiesFile.delete() | ||
|
|
||
| // Also clear WebView cookies | ||
| android.webkit.CookieManager.getInstance().removeAllCookies(null) |
Contributor
There was a problem hiding this comment.
P2: A user can start a new login before WebView cookie removal completes and be silently reauthenticated as the logged-out account. Surface completion from removeAllCookies, then flush cookies and enable a new login only after that callback.
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/battle_net/BattleNetAuthManager.kt, line 90:
<comment>A user can start a new login before WebView cookie removal completes and be silently reauthenticated as the logged-out account. Surface completion from `removeAllCookies`, then flush cookies and enable a new login only after that callback.</comment>
<file context>
@@ -0,0 +1,94 @@
+ if (cookiesFile.exists()) cookiesFile.delete()
+
+ // Also clear WebView cookies
+ android.webkit.CookieManager.getInstance().removeAllCookies(null)
+
+ Timber.tag("BattleNet").i("Logged out")
</file context>
| } | ||
|
|
||
| fun hasStoredCredentials(context: Context): Boolean { | ||
| return File(getCredentialsFilePath(context)).exists() || File(getCookiesFilePath(context)).exists() |
Contributor
There was a problem hiding this comment.
P2: Credential-only state is shown as logged in, but every library sync fails because sync authenticates exclusively with loadCookies. Use stored OAuth credentials in the sync flow, or only report authentication when a usable cookie session exists.
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/battle_net/BattleNetAuthManager.kt, line 24:
<comment>Credential-only state is shown as logged in, but every library sync fails because sync authenticates exclusively with `loadCookies`. Use stored OAuth credentials in the sync flow, or only report authentication when a usable cookie session exists.</comment>
<file context>
@@ -0,0 +1,94 @@
+ }
+
+ fun hasStoredCredentials(context: Context): Boolean {
+ return File(getCredentialsFilePath(context)).exists() || File(getCookiesFilePath(context)).exists()
+ }
+
</file context>
| <string name="settings_emulation_shared_containers_title">Общие контейнеры</string> | ||
| <string name="settings_emulation_shared_containers_subtitle">Разрешить играм с одинаковыми системными конфигурациями использовать одну и ту же директорию контейнера для экономии места (экспериментально)</string> | ||
| <string name="tab_battle_net">Battle.net</string> | ||
| <string name="battle_net_login_title">Connect Battle.net</string> |
Contributor
There was a problem hiding this comment.
P2: 15 of 17 new strings in the Russian locale file are in English, not Russian. The PR claims 'Full localization across 14 languages' but the Battle.net login flow, anti-cheat warning, library prompt, and container storage UI (badges, dialogs, manager title) will all display in English to Russian-speaking users. Existing strings in the same file follow proper Russian translation — these new ones were left untranslated. Either provide Russian translations before merging or update the PR description to reflect placeholder status.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/res/values-ru/strings.xml, line 2031:
<comment>15 of 17 new strings in the Russian locale file are in English, not Russian. The PR claims 'Full localization across 14 languages' but the Battle.net login flow, anti-cheat warning, library prompt, and container storage UI (badges, dialogs, manager title) will all display in English to Russian-speaking users. Existing strings in the same file follow proper Russian translation — these new ones were left untranslated. Either provide Russian translations before merging or update the PR description to reflect placeholder status.</comment>
<file context>
@@ -2025,4 +2025,22 @@ https://gamenative.app
+ <string name="settings_emulation_shared_containers_title">Общие контейнеры</string>
+ <string name="settings_emulation_shared_containers_subtitle">Разрешить играм с одинаковыми системными конфигурациями использовать одну и ту же директорию контейнера для экономии места (экспериментально)</string>
+ <string name="tab_battle_net">Battle.net</string>
+ <string name="battle_net_login_title">Connect Battle.net</string>
+ <string name="battle_net_login_warning">CAUTION: Battle.net anti-cheat (Warden/Ricochet) may flag emulation as unauthorized software. Online play carries a HIGH RISK of account bans. Use at your own risk.</string>
+ <string name="battle_net_login_success">Successfully connected to Battle.net</string>
</file context>
![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: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/app/gamenative/utils/ContainerStorageManager.kt (1)
1022-1029: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
detectGameSourcedoesn't recognizeBATTLE_NET_(orSHARED_) ids.Callers that use
detectGameSourcedirectly (e.g.uninstallGameAndContainerat line 426,getStorageLocation(context, entry)at line 214) will getnullfor a Battle.net container and surface a misleading "Unknown game source" instead of the intended unsupported-operation result. Add aBATTLE_NET_branch for consistency.Proposed addition
containerId.startsWith("AMAZON_") -> GameSource.AMAZON + containerId.startsWith("BATTLE_NET_") -> GameSource.BATTLE_NET else -> null🤖 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/utils/ContainerStorageManager.kt` around lines 1022 - 1029, Update detectGameSource to recognize container IDs beginning with BATTLE_NET_ and return the corresponding GameSource value, preserving the existing null fallback for unknown prefixes. Do not add SHARED_ handling unless an established GameSource value exists for it.
🟡 Minor comments (9)
app/src/main/res/values-it/strings.xml-2093-2110 (1)
2093-2110: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winProvide localized translations for the new strings. The newly added string resources for the Shared Containers and Battle.net integration features have been pasted in English across the localized files without being translated.
app/src/main/res/values-it/strings.xml#L2093-L2110: translate the new strings into Italian.app/src/main/res/values-ja/strings.xml#L2059-L2076: translate the new strings into Japanese.app/src/main/res/values-ko/strings.xml#L2100-L2117: translate the new strings into Korean.app/src/main/res/values-pl/strings.xml#L2100-L2117: translate the new strings into Polish.app/src/main/res/values-pt-rBR/strings.xml#L1972-L1989: translate the new strings into Portuguese.app/src/main/res/values-ro/strings.xml#L2103-L2120: translate the new strings into Romanian.🤖 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/res/values-it/strings.xml` around lines 2093 - 2110, Translate the newly added Shared Containers, Battle.net integration, and container storage strings into the appropriate language in app/src/main/res/values-it/strings.xml (2093-2110), app/src/main/res/values-ja/strings.xml (2059-2076), app/src/main/res/values-ko/strings.xml (2100-2117), app/src/main/res/values-pl/strings.xml (2100-2117), app/src/main/res/values-pt-rBR/strings.xml (1972-1989), and app/src/main/res/values-ro/strings.xml (2103-2120), preserving every resource name and format placeholder while replacing the English text with natural Italian, Japanese, Korean, Polish, Brazilian Portuguese, and Romanian translations respectively.app/src/main/java/app/gamenative/ui/screen/settings/ContainerStorageManagerDialog.kt-640-652 (1)
640-652: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHardcoded English button/dialog labels bypass localization.
"Rename"/"Clone"here (and"Rename Container"/"Container Name"in the rename dialog at Lines 321, 326) are literal strings, while the rest of this dialog and the PR usestringResource. These won't be translated by the 14-language localization added in this PR.🤖 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/settings/ContainerStorageManagerDialog.kt` around lines 640 - 652, Replace the hardcoded “Rename” and “Clone” labels in the entry.hasContainer action buttons, and “Rename Container” and “Container Name” in the rename dialog, with stringResource lookups using the appropriate existing or newly added localized string keys. Ensure all four labels participate in the app’s localization system consistently with the surrounding dialog UI.app/src/main/java/app/gamenative/utils/AppLaunchConfigManager.kt-38-38 (1)
38-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
enableXInputis saved but never loaded/applied.
saveConfigwritesdata.enableXInputunder the"inputType"key, butloadConfignever reads that key andLaunchConfig/applyToContainerDatahave no corresponding field. The per-app input-type override is silently dropped on the read path, so this field is dead data.🤖 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/utils/AppLaunchConfigManager.kt` at line 38, Update the LaunchConfig model and loadConfig/applyToContainerData flow to read and apply the per-app input-type override stored under "inputType". Ensure saveConfig’s enableXInput value is restored into the corresponding container data field, preserving the existing behavior when the key is absent.app/src/main/java/app/gamenative/ui/model/LibraryViewModel.kt-1141-1179 (1)
1141-1179: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
findLinkedInstallSourcenever checks Battle.net installs.The function is now also invoked for Battle.net entries and for all other sources, but it only inspects Steam, GOG, Epic, and Amazon. A game installed via Battle.net won't be reported as a linked install for a matching Steam/GOG/Epic/Amazon library item.
battleNetGameListis already available here, so add a symmetric branch.Proposed addition
// Check Amazon if (currentSource != GameSource.AMAZON) { val amazonInstalled = amazonGameList.any { it.title.unaccent().lowercase().replace(Regex("[^a-z0-9]"), "") == normalizedTitle && it.isInstalled } if (amazonInstalled) return "Amazon" } + + // Check Battle.net + if (currentSource != GameSource.BATTLE_NET) { + val battleNetInstalled = battleNetGameList.any { + it.title.unaccent().lowercase().replace(Regex("[^a-z0-9]"), "") == normalizedTitle && it.isInstalled + } + if (battleNetInstalled) return "Battle.net" + } return null🤖 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/model/LibraryViewModel.kt` around lines 1141 - 1179, Update findLinkedInstallSource to also inspect battleNetGameList when currentSource is not GameSource.BATTLENET, matching the normalized game title and requiring the Battle.net entry to be installed; return the appropriate Battle.net source label when a match is found, while preserving the existing checks and ordering.app/src/main/java/app/gamenative/utils/GameFeedbackUtils.kt-68-71 (1)
68-71: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBattle.net feedback will always be attributed to the placeholder name "Battle.net Game".
Deferring to the cross-file consolidated comment together with the equivalent TODOs in
ContainerUtils.kt.🤖 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/utils/GameFeedbackUtils.kt` around lines 68 - 71, Replace the hardcoded "Battle.net Game" fallback in the GameSource.BATTLE_NET branch of the game-name lookup with the actual Battle.net game-name resolution, consistent with the implementation and TODOs in ContainerUtils.kt. Ensure Battle.net feedback is attributed to the specific game name rather than a generic placeholder.app/src/main/java/app/gamenative/ui/screen/library/components/SystemMenu.kt-729-750 (1)
729-750: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBattle.net menu item needs a logout label
The logged-in branch still uses
R.string.battle_net_login_title, and there’s no Battle.net logout string resource yet. Add a localized logout title and switch the logged-in branch to it so the text matches the logout icon/destructive state.🤖 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/components/SystemMenu.kt` around lines 729 - 750, Add a localized Battle.net logout title string resource, then update the logged-in branch of the Battle.net SystemMenuItem text selection to use it instead of R.string.battle_net_login_title; keep the logged-out branch using the login title.app/src/main/java/app/gamenative/service/battle_net/BattleNetAuthManager.kt-38-49 (1)
38-49: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSwallowed exception in
loadCookies.The catch block silently returns
emptyMap()without logging, unlike every other method in this file (saveCookies,saveCredentials,loadCredentials,logout) which logs viaTimber. This hides parsing/IO failures (e.g. corruptedcookies.json) that would otherwise explain unauthenticated sync failures.🩹 Proposed fix
} catch (e: Exception) { + Timber.tag("BattleNet").e(e, "Failed to load cookies") emptyMap() }🤖 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/battle_net/BattleNetAuthManager.kt` around lines 38 - 49, Update the catch block in loadCookies to log the caught exception with Timber before returning emptyMap(), matching the error-logging behavior of saveCookies, saveCredentials, loadCredentials, and logout while preserving the existing fallback.Source: Linters/SAST tools
app/src/main/java/app/gamenative/service/battle_net/BattleNetSyncManager.kt-36-43 (1)
36-43: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose the OkHttp response on the failure path too.
response.bodyis only implicitly closed via.string()on the success path; the!response.isSuccessfulearly return leaves the response body unclosed, leaking the connection back to the pool. Wrap the call in.use { }.🩹 Proposed fix
- val response = httpClient.newCall(request).execute() - if (!response.isSuccessful) { - Timber.tag("BattleNet").e("Sync failed: HTTP ${response.code}") - return@withContext Result.failure(Exception("Battle.net sync failed: HTTP ${response.code}")) - } - - val body = response.body?.string() ?: "" - val json = JSONObject(body) + val body = httpClient.newCall(request).execute().use { response -> + if (!response.isSuccessful) { + Timber.tag("BattleNet").e("Sync failed: HTTP ${response.code}") + return@withContext Result.failure(Exception("Battle.net sync failed: HTTP ${response.code}")) + } + response.body?.string() ?: "" + } + val json = JSONObject(body)🤖 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/battle_net/BattleNetSyncManager.kt` around lines 36 - 43, Wrap the OkHttp response returned by httpClient.newCall(request).execute() in use within the sync flow, including the !response.isSuccessful early-return branch. Keep the existing status handling and body parsing behavior unchanged while ensuring the response is closed on every path.app/src/main/java/app/gamenative/ui/screen/library/components/LibraryGridCard.kt-547-578 (1)
547-578: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLocalize the user-facing "Installed" label.
The text "Installed" is hardcoded and will not translate with the rest of the application. Please use the existing
R.string.installedresource to ensure proper localization.🌐 Proposed fix to use string resource
- text = "Installed (${appInfo.linkedInstallSource})", + text = "${stringResource(R.string.installed)} (${appInfo.linkedInstallSource})",🤖 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/components/LibraryGridCard.kt` around lines 547 - 578, Update the Text in the linkedInstallSource block of LibraryGridCard to use the existing R.string.installed resource for the “Installed” label while preserving the linked install source value and current formatting.
🧹 Nitpick comments (6)
app/src/main/res/values-da/strings.xml (1)
1972-1989: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove untranslated English strings or provide translations.
Adding untranslated English strings directly into localized
strings.xmlfiles defeats Android's built-in resource fallback mechanism and creates duplicate string definitions that add unnecessary overhead to translation platforms. If translations are not yet available for these languages, these English strings should be removed entirely from the localized files; the system will automatically fall back to the defaults inres/values/strings.xml.
app/src/main/res/values-da/strings.xml#L1972-L1989: Remove these English string entries or translate them into Danish.app/src/main/res/values-de/strings.xml#L2042-L2059: Remove these English string entries or translate them into German.app/src/main/res/values-es/strings.xml#L2100-L2117: Remove these English string entries or translate them into Spanish.app/src/main/res/values-fr/strings.xml#L2102-L2119: Remove these English string entries or translate them into French.🤖 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/res/values-da/strings.xml` around lines 1972 - 1989, Remove the untranslated English entries identified by the string names settings_emulation_shared_containers_title through container_storage_manager_title from app/src/main/res/values-da/strings.xml lines 1972-1989, app/src/main/res/values-de/strings.xml lines 2042-2059, app/src/main/res/values-es/strings.xml lines 2100-2117, and app/src/main/res/values-fr/strings.xml lines 2102-2119, or replace them with accurate Danish, German, Spanish, and French translations respectively; rely on the default resources for any removed entries.app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt (1)
619-619: 📐 Maintainability & Code Quality | 🔵 TrivialBattle.net pause/resume/cancel are intentional
// TODO: implementno-ops. These are inert today becauserefreshDownloadsSnapshotnever enqueues Battle.net downloads, so no behavioral risk. Want me to open a tracking issue for the Battle.net download control flow?Also applies to: 670-670, 770-773
🤖 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/model/DownloadsViewModel.kt` at line 619, Open a tracking issue for implementing Battle.net download controls, covering the intentional no-op branches in the pause, resume, and cancel flows of DownloadsViewModel. Keep the current no-op behavior unchanged until Battle.net downloads are supported by refreshDownloadsSnapshot.app/src/main/java/app/gamenative/db/PluviaDatabase.kt (1)
98-98: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueRemove the redundant
AutoMigration(from = 23, to = 24)inPluviaDatabase.kt:98
ROOM_MIGRATION_V23_to_V24is already registered inDatabaseModule, so this auto-migration is ignored and only makes the version history harder to follow.🤖 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/db/PluviaDatabase.kt` at line 98, Remove the redundant AutoMigration entry for the 23-to-24 transition from the PluviaDatabase configuration, leaving ROOM_MIGRATION_V23_to_V24 as the sole registration in DatabaseModule. Preserve all other migration entries and database configuration unchanged.app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt (2)
1282-1288: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove stale commented-out line.
// battleNetLogout(context, lifecycleScope)at Line 1286 is dead code left over from an earlier implementation attempt.🧹 Proposed cleanup
onBattleNetLogoutClick = { - // battleNetLogout(context, lifecycleScope) app.gamenative.service.battle_net.BattleNetAuthManager.logout(context) },🤖 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/LibraryScreen.kt` around lines 1282 - 1288, Remove the stale commented-out battleNetLogout call from the onBattleNetLogoutClick handler, leaving the active BattleNetAuthManager.logout(context) invocation unchanged.
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
BattleNetAuthManageris referenced by fully-qualified name everywhere instead of an import. Every other platform manager (GOGService,EpicService,AmazonService) is imported and used unqualified;BattleNetAuthManageris spelled out with its full package path at each call site, which is inconsistent and noisier to read/maintain.
app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt#L982-985: replaceapp.gamenative.service.battle_net.BattleNetAuthManager.hasStoredCredentials(context)with an import + unqualified call.app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt#L1235-1238: same for thebattleNetLoggedIncomputation.app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt#L1282-1288: same for theonBattleNetLogoutClickcall.app/src/main/java/app/gamenative/ui/screen/library/components/LibraryListPane.kt#L110-116: same forcalculateInstalledCount's Battle.net branch.app/src/main/java/app/gamenative/ui/screen/library/components/LibraryListPane.kt#L244-259: same for thetotalSkeletonCountBattle.net branch.🤖 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/LibraryScreen.kt` at line 1, The BattleNetAuthManager references in LibraryScreen and LibraryListPane use fully qualified names inconsistently. Import BattleNetAuthManager in each file and replace every fully qualified reference, including hasStoredCredentials, onBattleNetLogoutClick, and calculateInstalledCount/totalSkeletonCount Battle.net branches, with the unqualified symbol.app/src/main/java/app/gamenative/service/battle_net/BattleNetConstants.kt (1)
30-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
LoginUrlWithStateto camelCase.Regular (non-Composable) function uses PascalCase.
As per coding guidelines, "Name Composable functions with PascalCase, variables and functions with camelCase."✏️ Proposed rename
- fun LoginUrlWithState(): Pair<String, String> { + fun loginUrlWithState(): Pair<String, String> {(update the call site in
BattleNetOAuthActivity.kt)🤖 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/battle_net/BattleNetConstants.kt` around lines 30 - 37, Rename the non-Composable function LoginUrlWithState to LoginUrlWithState’s camelCase form loginUrlWithState in BattleNetConstants, and update its call site in BattleNetOAuthActivity accordingly.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/app/gamenative/MainActivity.kt`:
- Around line 506-509: Update the onTrimMemory handling around the
TRIM_MEMORY_COMPLETE branch so appImageLoader?.diskCache?.clear() runs on a
background thread rather than the Main thread. Preserve the existing
memory-threshold condition and System.gc() behavior, ensuring only the blocking
disk-cache operation is dispatched asynchronously.
In `@app/src/main/java/app/gamenative/service/battle_net/BattleNetAuthManager.kt`:
- Around line 27-81: Update saveCookies, loadCookies, saveCredentials, and
loadCredentials to persist session cookies and accessToken/refreshToken through
encrypted, keystore-backed storage such as EncryptedFile or
EncryptedSharedPreferences instead of plaintext JSON files under
filesDir/battle_net/. Preserve the existing load/save behavior and credential
fields while ensuring these secrets are encrypted at rest.
In `@app/src/main/java/app/gamenative/service/battle_net/BattleNetService.kt`:
- Around line 53-66: Add a manifest service declaration for BattleNetService
with android:foregroundServiceType set to dataSync, then update
BattleNetService.onStartCommand to call the typed startForeground overload
appropriate for a data-sync foreground service while preserving the existing
notification setup and sync behavior.
In `@app/src/main/java/app/gamenative/ui/screen/auth/BattleNetOAuthActivity.kt`:
- Around line 71-74: Update the cookie parsing in the cookieMap construction to
split each cookie entry at only the first “=” so values containing additional
“=” characters remain intact when passed to saveCookies and later used by
BattleNetSyncManager. Preserve trimming and the existing empty-value handling.
- Around line 61-89: Validate the redirect’s OAuth state before accepting the
authorization code in the onUrlChange callback. Extract the state query
parameter alongside extractedCode, compare it with the previously saved
oauthState, and only save cookies, return RESULT_OK, and finish when they match;
reject redirects with missing or mismatched state.
In
`@app/src/main/java/app/gamenative/ui/screen/settings/ContainerStorageManagerDialog.kt`:
- Around line 183-227: The confirmDetach flow performs the potentially large
FileUtils.copy operation on the Main dispatcher and silently exits when dstDir
already exists. Move the container lookup, destination check, and copy work in
confirmDetach to Dispatchers.IO, then switch back to the UI context for refresh
and loading-state updates; when the destination exists, provide explicit
user-visible handling instead of only logging and returning.
- Around line 145-159: Update confirmRename to retrieve the target with
ContainerManager.getContainerById(entry.containerId) instead of
ContainerUtils.getContainer, matching the lookup used by confirmClone and
confirmDetach; preserve the existing rename, save, refresh, and error-handling
flow.
- Around line 183-220: The detach flow in confirmDetach must ensure subsequent
container resolution uses the dedicated appId container rather than the shared
container ID. Update ContainerUtils.getContainerId(), hasContainer(), and
getContainer() so an existing dedicated appId container takes precedence when
shared containers are enabled, while preserving shared-container fallback when
no dedicated container exists.
In
`@app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.kt`:
- Around line 486-491: Replace the hardcoded title and subtitle text in the
SettingsMenuLink for “Manage Game Folders” with stringResource(...) lookups. Add
or reuse localized string resources for both labels across the supported
locales, following the existing resource naming and localization patterns used
in SettingsGroupInterface.kt.
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 4004-4014: The Battle.net launch branch in XServerScreen must pass
the extracted productCode through the launch configuration and set workingDir to
the game’s install directory when using a relative Blizzard executable path.
Update the BattleNetConstants lookup and return path to preserve the
product-code mapping, while keeping the fallback executable behavior intact.
In `@app/src/main/java/app/gamenative/utils/ContainerStorageManager.kt`:
- Around line 368-391: Replace the stubbed `filter { true }` logic with
resolution of each installed game's shared-container ID, and compare it to the
requested `containerId` in both `ContainerStorageManager.kt` sites at lines
368-391 and 769-779. Use the resulting occupant set to report accurate users,
and gate or refuse physical deletion in the shared-container removal flow when
other installed games resolve to the same ID.
In `@app/src/main/java/app/gamenative/utils/ContainerUtils.kt`:
- Around line 450-458: Update applyToContainer so shared-container saves never
mutate the shared Container or call saveData(). Detect shared-container mode
independently of current_app_id, resolve the app identity from the available
launch/container context, and route the complete ContainerData through
AppLaunchConfigManager, expanding its persisted fields as needed. Apply the same
guard to the shared-container save path near the other save handling so missing
current_app_id cannot fall back to shared storage.
---
Outside diff comments:
In `@app/src/main/java/app/gamenative/utils/ContainerStorageManager.kt`:
- Around line 1022-1029: Update detectGameSource to recognize container IDs
beginning with BATTLE_NET_ and return the corresponding GameSource value,
preserving the existing null fallback for unknown prefixes. Do not add SHARED_
handling unless an established GameSource value exists for it.
---
Minor comments:
In `@app/src/main/java/app/gamenative/service/battle_net/BattleNetAuthManager.kt`:
- Around line 38-49: Update the catch block in loadCookies to log the caught
exception with Timber before returning emptyMap(), matching the error-logging
behavior of saveCookies, saveCredentials, loadCredentials, and logout while
preserving the existing fallback.
In `@app/src/main/java/app/gamenative/service/battle_net/BattleNetSyncManager.kt`:
- Around line 36-43: Wrap the OkHttp response returned by
httpClient.newCall(request).execute() in use within the sync flow, including the
!response.isSuccessful early-return branch. Keep the existing status handling
and body parsing behavior unchanged while ensuring the response is closed on
every path.
In `@app/src/main/java/app/gamenative/ui/model/LibraryViewModel.kt`:
- Around line 1141-1179: Update findLinkedInstallSource to also inspect
battleNetGameList when currentSource is not GameSource.BATTLENET, matching the
normalized game title and requiring the Battle.net entry to be installed; return
the appropriate Battle.net source label when a match is found, while preserving
the existing checks and ordering.
In
`@app/src/main/java/app/gamenative/ui/screen/library/components/LibraryGridCard.kt`:
- Around line 547-578: Update the Text in the linkedInstallSource block of
LibraryGridCard to use the existing R.string.installed resource for the
“Installed” label while preserving the linked install source value and current
formatting.
In `@app/src/main/java/app/gamenative/ui/screen/library/components/SystemMenu.kt`:
- Around line 729-750: Add a localized Battle.net logout title string resource,
then update the logged-in branch of the Battle.net SystemMenuItem text selection
to use it instead of R.string.battle_net_login_title; keep the logged-out branch
using the login title.
In
`@app/src/main/java/app/gamenative/ui/screen/settings/ContainerStorageManagerDialog.kt`:
- Around line 640-652: Replace the hardcoded “Rename” and “Clone” labels in the
entry.hasContainer action buttons, and “Rename Container” and “Container Name”
in the rename dialog, with stringResource lookups using the appropriate existing
or newly added localized string keys. Ensure all four labels participate in the
app’s localization system consistently with the surrounding dialog UI.
In `@app/src/main/java/app/gamenative/utils/AppLaunchConfigManager.kt`:
- Line 38: Update the LaunchConfig model and loadConfig/applyToContainerData
flow to read and apply the per-app input-type override stored under "inputType".
Ensure saveConfig’s enableXInput value is restored into the corresponding
container data field, preserving the existing behavior when the key is absent.
In `@app/src/main/java/app/gamenative/utils/GameFeedbackUtils.kt`:
- Around line 68-71: Replace the hardcoded "Battle.net Game" fallback in the
GameSource.BATTLE_NET branch of the game-name lookup with the actual Battle.net
game-name resolution, consistent with the implementation and TODOs in
ContainerUtils.kt. Ensure Battle.net feedback is attributed to the specific game
name rather than a generic placeholder.
In `@app/src/main/res/values-it/strings.xml`:
- Around line 2093-2110: Translate the newly added Shared Containers, Battle.net
integration, and container storage strings into the appropriate language in
app/src/main/res/values-it/strings.xml (2093-2110),
app/src/main/res/values-ja/strings.xml (2059-2076),
app/src/main/res/values-ko/strings.xml (2100-2117),
app/src/main/res/values-pl/strings.xml (2100-2117),
app/src/main/res/values-pt-rBR/strings.xml (1972-1989), and
app/src/main/res/values-ro/strings.xml (2103-2120), preserving every resource
name and format placeholder while replacing the English text with natural
Italian, Japanese, Korean, Polish, Brazilian Portuguese, and Romanian
translations respectively.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/db/PluviaDatabase.kt`:
- Line 98: Remove the redundant AutoMigration entry for the 23-to-24 transition
from the PluviaDatabase configuration, leaving ROOM_MIGRATION_V23_to_V24 as the
sole registration in DatabaseModule. Preserve all other migration entries and
database configuration unchanged.
In `@app/src/main/java/app/gamenative/service/battle_net/BattleNetConstants.kt`:
- Around line 30-37: Rename the non-Composable function LoginUrlWithState to
LoginUrlWithState’s camelCase form loginUrlWithState in BattleNetConstants, and
update its call site in BattleNetOAuthActivity accordingly.
In `@app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt`:
- Line 619: Open a tracking issue for implementing Battle.net download controls,
covering the intentional no-op branches in the pause, resume, and cancel flows
of DownloadsViewModel. Keep the current no-op behavior unchanged until
Battle.net downloads are supported by refreshDownloadsSnapshot.
In `@app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt`:
- Around line 1282-1288: Remove the stale commented-out battleNetLogout call
from the onBattleNetLogoutClick handler, leaving the active
BattleNetAuthManager.logout(context) invocation unchanged.
- Line 1: The BattleNetAuthManager references in LibraryScreen and
LibraryListPane use fully qualified names inconsistently. Import
BattleNetAuthManager in each file and replace every fully qualified reference,
including hasStoredCredentials, onBattleNetLogoutClick, and
calculateInstalledCount/totalSkeletonCount Battle.net branches, with the
unqualified symbol.
In `@app/src/main/res/values-da/strings.xml`:
- Around line 1972-1989: Remove the untranslated English entries identified by
the string names settings_emulation_shared_containers_title through
container_storage_manager_title from app/src/main/res/values-da/strings.xml
lines 1972-1989, app/src/main/res/values-de/strings.xml lines 2042-2059,
app/src/main/res/values-es/strings.xml lines 2100-2117, and
app/src/main/res/values-fr/strings.xml lines 2102-2119, or replace them with
accurate Danish, German, Spanish, and French translations respectively; rely on
the default resources for any removed entries.
🪄 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: e802e16e-73cb-4117-ab94-8f09df475865
📒 Files selected for processing (78)
.artifacts/08f1994d-07c3-4379-93c3-78af500e4868/task.artifact.md.artifacts/08f1994d-07c3-4379-93c3-78af500e4868/task_battlenet.artifact.md.artifacts/08f1994d-07c3-4379-93c3-78af500e4868/walkthrough.artifact.md.artifacts/08f1994d-07c3-4379-93c3-78af500e4868/walkthrough_battlenet.artifact.md.artifacts/08f1994d-07c3-4379-93c3-78af500e4868/walkthrough_battlenet_phase2.artifact.md.artifacts/08f1994d-07c3-4379-93c3-78af500e4868/walkthrough_game_mgmt_opt.artifact.md.artifacts/08f1994d-07c3-4379-93c3-78af500e4868/walkthrough_legacy_opt.artifact.md.artifacts/08f1994d-07c3-4379-93c3-78af500e4868/walkthrough_shared_robustness.artifact.mdAGENTS.mdARCHITECTURE.mdCLAUDE.mdDEVELOPMENT_NOTES.mdROADMAP_RECOMMENDED.mdapp/schemas/app.gamenative.db.PluviaDatabase/25.jsonapp/src/main/AndroidManifest.xmlapp/src/main/java/app/gamenative/Constants.ktapp/src/main/java/app/gamenative/MainActivity.ktapp/src/main/java/app/gamenative/PluviaApp.ktapp/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/data/BattleNetGame.ktapp/src/main/java/app/gamenative/data/LibraryItem.ktapp/src/main/java/app/gamenative/db/PluviaDatabase.ktapp/src/main/java/app/gamenative/db/dao/BattleNetGameDao.ktapp/src/main/java/app/gamenative/di/DatabaseModule.ktapp/src/main/java/app/gamenative/events/AndroidEvent.ktapp/src/main/java/app/gamenative/service/NotificationHelper.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/service/battle_net/BattleNetAuthManager.ktapp/src/main/java/app/gamenative/service/battle_net/BattleNetConstants.ktapp/src/main/java/app/gamenative/service/battle_net/BattleNetService.ktapp/src/main/java/app/gamenative/service/battle_net/BattleNetSyncManager.ktapp/src/main/java/app/gamenative/sync/FrontendSyncManager.ktapp/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/ui/component/dialog/ContainerConfigDialog.ktapp/src/main/java/app/gamenative/ui/component/dialog/CustomGameFoldersDialog.ktapp/src/main/java/app/gamenative/ui/data/LibraryState.ktapp/src/main/java/app/gamenative/ui/enums/LibraryTab.ktapp/src/main/java/app/gamenative/ui/model/DownloadsViewModel.ktapp/src/main/java/app/gamenative/ui/model/LibraryViewModel.ktapp/src/main/java/app/gamenative/ui/model/MainViewModel.ktapp/src/main/java/app/gamenative/ui/screen/auth/BattleNetOAuthActivity.ktapp/src/main/java/app/gamenative/ui/screen/downloads/DownloadsScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/BattleNetAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/components/LibraryAppItem.ktapp/src/main/java/app/gamenative/ui/screen/library/components/LibraryGridCard.ktapp/src/main/java/app/gamenative/ui/screen/library/components/LibraryListPane.ktapp/src/main/java/app/gamenative/ui/screen/library/components/SystemMenu.ktapp/src/main/java/app/gamenative/ui/screen/settings/ContainerStorageManagerDialog.ktapp/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupEmulation.ktapp/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XAudioUtils.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/AppLaunchConfigManager.ktapp/src/main/java/app/gamenative/utils/ContainerStorageManager.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/app/gamenative/utils/CustomGameScanner.ktapp/src/main/java/app/gamenative/utils/GameFeedbackUtils.ktapp/src/main/java/app/gamenative/utils/PlatformOAuthHandlers.ktapp/src/main/java/com/winlator/container/Container.javaapp/src/main/res/values-da/strings.xmlapp/src/main/res/values-de/strings.xmlapp/src/main/res/values-es/strings.xmlapp/src/main/res/values-fr/strings.xmlapp/src/main/res/values-it/strings.xmlapp/src/main/res/values-ja/strings.xmlapp/src/main/res/values-ko/strings.xmlapp/src/main/res/values-pl/strings.xmlapp/src/main/res/values-pt-rBR/strings.xmlapp/src/main/res/values-ro/strings.xmlapp/src/main/res/values-ru/strings.xmlapp/src/main/res/values-uk/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values-zh-rTW/strings.xmlapp/src/main/res/values/strings.xmlimplementation_plan.artifact.md
Comment on lines
+506
to
+509
| if (level >= ComponentCallbacks2.TRIM_MEMORY_COMPLETE) { | ||
| appImageLoader?.diskCache?.clear() | ||
| System.gc() | ||
| } |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Move disk cache clearing to a background thread.
appImageLoader?.diskCache?.clear() performs blocking file I/O. Calling it directly on the Main thread inside onTrimMemory can trigger StrictMode disk-write violations and potential ANRs. As per coding guidelines, "avoid blocking calls on the Main thread".
⚡ Proposed fix
// Extremely aggressive for Legacy devices under critical pressure
if (level >= ComponentCallbacks2.TRIM_MEMORY_COMPLETE) {
- appImageLoader?.diskCache?.clear()
+ androidx.lifecycle.lifecycleScope.launch(kotlinx.coroutines.Dispatchers.IO) {
+ appImageLoader?.diskCache?.clear()
+ }
System.gc()
}🧰 Tools
🪛 detekt (1.23.8)
[warning] 508-508: An explicit call to the Garbage Collector as in System should not be made.
(detekt.potential-bugs.ExplicitGarbageCollectionCall)
🤖 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/MainActivity.kt` around lines 506 - 509,
Update the onTrimMemory handling around the TRIM_MEMORY_COMPLETE branch so
appImageLoader?.diskCache?.clear() runs on a background thread rather than the
Main thread. Preserve the existing memory-threshold condition and System.gc()
behavior, ensuring only the blocking disk-cache operation is dispatched
asynchronously.
Source: Coding guidelines
Comment on lines
+27
to
+81
| fun saveCookies(context: Context, cookies: Map<String, String>) { | ||
| try { | ||
| val json = JSONObject() | ||
| cookies.forEach { (k, v) -> json.put(k, v) } | ||
| File(getCookiesFilePath(context)).writeText(json.toString()) | ||
| Timber.tag("BattleNet").d("Saved session cookies") | ||
| } catch (e: Exception) { | ||
| Timber.tag("BattleNet").e(e, "Failed to save cookies") | ||
| } | ||
| } | ||
|
|
||
| fun loadCookies(context: Context): Map<String, String> { | ||
| return try { | ||
| val file = File(getCookiesFilePath(context)) | ||
| if (!file.exists()) return emptyMap() | ||
| val json = JSONObject(file.readText()) | ||
| val map = mutableMapOf<String, String>() | ||
| json.keys().forEach { map[it] = json.getString(it) } | ||
| map | ||
| } catch (e: Exception) { | ||
| emptyMap() | ||
| } | ||
| } | ||
|
|
||
| fun saveCredentials(context: Context, credentials: BattleNetCredentials) { | ||
| try { | ||
| val json = JSONObject().apply { | ||
| put("accessToken", credentials.accessToken) | ||
| put("refreshToken", credentials.refreshToken) | ||
| put("accountId", credentials.accountId) | ||
| put("battleTag", credentials.battleTag) | ||
| } | ||
| File(getCredentialsFilePath(context)).writeText(json.toString()) | ||
| Timber.tag("BattleNet").i("Saved credentials for ${credentials.battleTag}") | ||
| } catch (e: Exception) { | ||
| Timber.tag("BattleNet").e(e, "Failed to save credentials") | ||
| } | ||
| } | ||
|
|
||
| fun loadCredentials(context: Context): BattleNetCredentials? { | ||
| return try { | ||
| val file = File(getCredentialsFilePath(context)) | ||
| if (!file.exists()) return null | ||
| val json = JSONObject(file.readText()) | ||
| BattleNetCredentials( | ||
| accessToken = json.getString("accessToken"), | ||
| refreshToken = json.getString("refreshToken"), | ||
| accountId = json.getString("accountId"), | ||
| battleTag = json.getString("battleTag") | ||
| ) | ||
| } catch (e: Exception) { | ||
| Timber.tag("BattleNet").e(e, "Failed to load credentials") | ||
| null | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether other platform auth managers in this repo already encrypt credentials at rest.
fd -a AuthManager.kt --full-path app/src/main/java/app/gamenative/service | xargs -I{} sh -c 'echo "== {} =="; rg -n "EncryptedFile|EncryptedSharedPreferences|writeText|JSONObject\(\).apply" {}'Repository: utkarshdalal/GameNative
Length of output: 1370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for encrypted storage helpers =="
rg -n "EncryptedFile|EncryptedSharedPreferences|MasterKey|androidx.security" app/src/main/java app/src/test java kotlin || true
echo
echo "== Inspect BattleNetAuthManager and sibling auth managers for storage paths =="
for f in \
app/src/main/java/app/gamenative/service/battle_net/BattleNetAuthManager.kt \
app/src/main/java/app/gamenative/service/amazon/AmazonAuthManager.kt \
app/src/main/java/app/gamenative/service/epic/EpicAuthManager.kt \
app/src/main/java/app/gamenative/service/gog/GOGAuthManager.kt
do
echo "---- $f ----"
sed -n '1,260p' "$f" | rg -n "writeText|readText|File\\(|JSONObject|cookies|token|credential|auth"
doneRepository: utkarshdalal/GameNative
Length of output: 20352
Persist these secrets with encrypted storage.
accessToken, refreshToken, and session cookies are written as plaintext JSON under filesDir/battle_net/. If they must stay on disk, move them to EncryptedFile/EncryptedSharedPreferences or a keystore-backed wrapper.
🧰 Tools
🪛 detekt (1.23.8)
[warning] 46-46: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 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/battle_net/BattleNetAuthManager.kt`
around lines 27 - 81, Update saveCookies, loadCookies, saveCredentials, and
loadCredentials to persist session cookies and accessToken/refreshToken through
encrypted, keystore-backed storage such as EncryptedFile or
EncryptedSharedPreferences instead of plaintext JSON files under
filesDir/battle_net/. Preserve the existing load/save behavior and credential
fields while ensuring these secrets are encrypted at rest.
Comment on lines
+53
to
+66
| override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int { | ||
| val notificationHelper = NotificationHelper(applicationContext) | ||
| val notification = notificationHelper.createServiceNotification(NotificationHelper.NOTIFICATION_ID_BATTLE_NET, "Battle.net Service") | ||
| startForeground(NotificationHelper.NOTIFICATION_ID_BATTLE_NET, notification) | ||
|
|
||
| if (intent?.action == "SYNC") { | ||
| syncJob?.cancel() | ||
| syncJob = scope.launch { | ||
| BattleNetSyncManager.syncLibrary(applicationContext, battleNetGameDao) | ||
| } | ||
| } | ||
|
|
||
| return START_STICKY | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "BattleNetService" app/src/main/AndroidManifest.xml
rg -n "foregroundServiceType" app/src/main/AndroidManifest.xml
rg -n "targetSdk" app/build.gradle.kts app/build.gradle 2>/dev/nullRepository: utkarshdalal/GameNative
Length of output: 669
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Manifest service declarations =="
rg -n -C 3 "BattleNetService|SteamService|foregroundServiceType" app/src/main/AndroidManifest.xml
echo
echo "== BattleNetService implementation =="
sed -n '1,140p' app/src/main/java/app/gamenative/service/battle_net/BattleNetService.kt
echo
echo "== SteamService implementation =="
sed -n '1,180p' app/src/main/java/app/gamenative/service/steam/SteamService.ktRepository: utkarshdalal/GameNative
Length of output: 3947
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== BattleNetService references =="
rg -n "BattleNetService|service\\.battle_net" .
echo
echo "== Manifest files containing BattleNet =="
fd -a 'AndroidManifest.xml' .Repository: utkarshdalal/GameNative
Length of output: 4218
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== BattleNetService declarations across manifests =="
rg -n -C 2 "BattleNetService|foregroundServiceType" app/src/main/AndroidManifest.xml app/src/legacy/AndroidManifest.xml app/src/modern/AndroidManifest.xml app/src/modernXr/AndroidManifest.xml 2>/dev/null || true
echo
echo "== app/src/main/AndroidManifest.xml service block =="
sed -n '130,190p' app/src/main/AndroidManifest.xml
echo
echo "== build variants =="
rg -n "productFlavors|sourceSets|flavorDimensions|applicationIdSuffix" app/build.gradle.ktsRepository: utkarshdalal/GameNative
Length of output: 4554
Add a manifest entry for BattleNetService
BattleNetService isn't declared in app/src/main/AndroidManifest.xml, so startForegroundService() will fail when this code path runs. Add the <service> entry with android:foregroundServiceType="dataSync" and use the typed startForeground(...) overload for this foreground service.
🤖 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/battle_net/BattleNetService.kt`
around lines 53 - 66, Add a manifest service declaration for BattleNetService
with android:foregroundServiceType set to dataSync, then update
BattleNetService.onStartCommand to call the typed startForeground overload
appropriate for a data-sync foreground service while preserving the existing
notification setup and sync behavior.
Comment on lines
+61
to
+89
| onUrlChange = { currentUrl: String -> | ||
| if (isValidRedirectUrl(currentUrl)) { | ||
| val extractedCode = extractAuthCode(currentUrl) | ||
| if (extractedCode != null) { | ||
| Timber.tag("BattleNet").d("Extracted auth code") | ||
|
|
||
| // Capture cookies for session sync | ||
| val cookieManager = CookieManager.getInstance() | ||
| val cookiesStr = cookieManager.getCookie("https://account.battle.net") | ||
| if (cookiesStr != null) { | ||
| val cookieMap = cookiesStr.split(";").associate { | ||
| val parts = it.split("=") | ||
| parts[0].trim() to (if (parts.size > 1) parts[1].trim() else "") | ||
| } | ||
| BattleNetAuthManager.saveCookies(this@BattleNetOAuthActivity, cookieMap) | ||
| } | ||
|
|
||
| val resultIntent = Intent().apply { | ||
| putExtra(EXTRA_AUTH_CODE, extractedCode) | ||
| } | ||
| setResult(Activity.RESULT_OK, resultIntent) | ||
| finish() | ||
| } | ||
| } | ||
| }, | ||
| ) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Missing OAuth state (CSRF) validation on redirect.
oauthState is generated and saved but never compared against the state query parameter on the redirect URL before accepting extractedCode and returning RESULT_OK. This is required to prevent CSRF/session-fixation in the OAuth flow.
🔐 Proposed fix
onUrlChange = { currentUrl: String ->
if (isValidRedirectUrl(currentUrl)) {
val extractedCode = extractAuthCode(currentUrl)
- if (extractedCode != null) {
+ val returnedState = runCatching { Uri.parse(currentUrl).getQueryParameter("state") }.getOrNull()
+ if (extractedCode != null && returnedState == oauthState) {
Timber.tag("BattleNet").d("Extracted auth code")
...
+ } else if (extractedCode != null) {
+ Timber.tag("BattleNet").w("OAuth state mismatch; rejecting redirect")
+ setResult(Activity.RESULT_CANCELED)
+ finish()
}
}
},📝 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
| onUrlChange = { currentUrl: String -> | |
| if (isValidRedirectUrl(currentUrl)) { | |
| val extractedCode = extractAuthCode(currentUrl) | |
| if (extractedCode != null) { | |
| Timber.tag("BattleNet").d("Extracted auth code") | |
| // Capture cookies for session sync | |
| val cookieManager = CookieManager.getInstance() | |
| val cookiesStr = cookieManager.getCookie("https://account.battle.net") | |
| if (cookiesStr != null) { | |
| val cookieMap = cookiesStr.split(";").associate { | |
| val parts = it.split("=") | |
| parts[0].trim() to (if (parts.size > 1) parts[1].trim() else "") | |
| } | |
| BattleNetAuthManager.saveCookies(this@BattleNetOAuthActivity, cookieMap) | |
| } | |
| val resultIntent = Intent().apply { | |
| putExtra(EXTRA_AUTH_CODE, extractedCode) | |
| } | |
| setResult(Activity.RESULT_OK, resultIntent) | |
| finish() | |
| } | |
| } | |
| }, | |
| ) | |
| } | |
| } | |
| } | |
| onUrlChange = { currentUrl: String -> | |
| if (isValidRedirectUrl(currentUrl)) { | |
| val extractedCode = extractAuthCode(currentUrl) | |
| val returnedState = runCatching { Uri.parse(currentUrl).getQueryParameter("state") }.getOrNull() | |
| if (extractedCode != null && returnedState == oauthState) { | |
| Timber.tag("BattleNet").d("Extracted auth code") | |
| // Capture cookies for session sync | |
| val cookieManager = CookieManager.getInstance() | |
| val cookiesStr = cookieManager.getCookie("https://account.battle.net") | |
| if (cookiesStr != null) { | |
| val cookieMap = cookiesStr.split(";").associate { | |
| val parts = it.split("=") | |
| parts[0].trim() to (if (parts.size > 1) parts[1].trim() else "") | |
| } | |
| BattleNetAuthManager.saveCookies(this@BattleNetOAuthActivity, cookieMap) | |
| } | |
| val resultIntent = Intent().apply { | |
| putExtra(EXTRA_AUTH_CODE, extractedCode) | |
| } | |
| setResult(Activity.RESULT_OK, resultIntent) | |
| finish() | |
| } else if (extractedCode != null) { | |
| Timber.tag("BattleNet").w("OAuth state mismatch; rejecting redirect") | |
| setResult(Activity.RESULT_CANCELED) | |
| finish() | |
| } | |
| } | |
| }, |
🤖 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/auth/BattleNetOAuthActivity.kt`
around lines 61 - 89, Validate the redirect’s OAuth state before accepting the
authorization code in the onUrlChange callback. Extract the state query
parameter alongside extractedCode, compare it with the previously saved
oauthState, and only save cookies, return RESULT_OK, and finish when they match;
reject redirects with missing or mismatched state.
Comment on lines
+71
to
+74
| val cookieMap = cookiesStr.split(";").associate { | ||
| val parts = it.split("=") | ||
| parts[0].trim() to (if (parts.size > 1) parts[1].trim() else "") | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cookie value truncated when it contains =.
it.split("=") without a limit means a cookie value containing = (e.g. a base64/JWT-like XSRF-TOKEN) is truncated to the segment right after the first =, corrupting the value saved via saveCookies. This value is later sent as X-XSRF-TOKEN in BattleNetSyncManager.kt.
🩹 Proposed fix
val cookieMap = cookiesStr.split(";").associate {
- val parts = it.split("=")
+ val parts = it.split("=", limit = 2)
parts[0].trim() to (if (parts.size > 1) parts[1].trim() else "")
}📝 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
| val cookieMap = cookiesStr.split(";").associate { | |
| val parts = it.split("=") | |
| parts[0].trim() to (if (parts.size > 1) parts[1].trim() else "") | |
| } | |
| val cookieMap = cookiesStr.split(";").associate { | |
| val parts = it.split("=", limit = 2) | |
| parts[0].trim() to (if (parts.size > 1) parts[1].trim() else "") | |
| } |
🤖 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/auth/BattleNetOAuthActivity.kt`
around lines 71 - 74, Update the cookie parsing in the cookieMap construction to
split each cookie entry at only the first “=” so values containing additional
“=” characters remain intact when passed to saveCookies and later used by
BattleNetSyncManager. Preserve trimming and the existing empty-value handling.
Comment on lines
+183
to
+227
| fun confirmDetach(entry: ContainerStorageManager.Entry) { | ||
| val appId = entry.appId ?: return | ||
| scope.launch { | ||
| isLoading = true | ||
| try { | ||
| val cm = com.winlator.container.ContainerManager(appContext) | ||
| val sharedContainer = cm.getContainerById(entry.containerId) | ||
| if (sharedContainer != null) { | ||
| // 1. Create a unique ID for this app's private container | ||
| val newId = appId // The standard appId | ||
|
|
||
| // 2. Clone the shared container directory to the standard appId path | ||
| val homeDir = File(com.winlator.xenvironment.ImageFs.find(appContext).getRootDir(), "home") | ||
| val srcDir = sharedContainer.getRootDir() | ||
| val dstDir = File(homeDir, com.winlator.xenvironment.ImageFs.USER + "-" + newId) | ||
|
|
||
| if (dstDir.exists()) { | ||
| Timber.tag("ContainerStorageManager").w("Destination for detach already exists: %s", newId) | ||
| } else { | ||
| com.winlator.core.FileUtils.copy(srcDir, dstDir) { file -> com.winlator.core.FileUtils.chmod(file, 0x1ED) } // 0755 | ||
|
|
||
| // 3. Update the new container's config to have the correct ID | ||
| val newContainer = com.winlator.container.Container(newId) | ||
| newContainer.setRootDir(dstDir) | ||
| newContainer.name = entry.displayName + " (Dedicated)" | ||
|
|
||
| // 4. Save app-specific launch config back into the .container file | ||
| val appConfig = app.gamenative.utils.AppLaunchConfigManager.applyToContainerData(appContext, appId, com.winlator.container.ContainerData()) | ||
| newContainer.executablePath = appConfig.executablePath | ||
| newContainer.execArgs = appConfig.execArgs | ||
| newContainer.saveData() | ||
|
|
||
| // 5. Optionally delete the per-app override file since it's now dedicated | ||
| app.gamenative.utils.AppLaunchConfigManager.deleteConfig(appContext, appId) | ||
|
|
||
| refresh() | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| Timber.tag("ContainerStorageManager").e(e, "Failed to detach container") | ||
| } finally { | ||
| isLoading = false | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'rememberCoroutineScope|Dispatchers\.(IO|Main)' app/src/main/java/app/gamenative/ui/screen/settings/ContainerStorageManagerDialog.kt
rg -nP 'static .*copy\(' app/src/main/java/com/winlator/core/FileUtils.java -A 3Repository: utkarshdalal/GameNative
Length of output: 810
Offload the container copy work from Main rememberCoroutineScope() launches on Main, so FileUtils.copy(srcDir, dstDir) can block the UI during detach and risks an ANR on large containers. Move the copy to Dispatchers.IO or another background dispatcher. The dstDir.exists() branch also only logs and returns, so the action silently does nothing.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 196-196: A File / FileInputStream / FileReader / FileOutputStream / FileWriter is constructed from a path built with string concatenation ("baseDir" + input) or string interpolation ("$baseDir/$input"). If any segment is attacker-controlled, a value such as "../../etc/passwd" escapes the intended directory (path traversal). Validate and canonicalize the resolved path and confirm it stays under the intended base directory (e.g. compare File(baseDir, name).canonicalFile against baseDir.canonicalFile), or reject inputs containing path separators and "..".
Context: File(homeDir, com.winlator.xenvironment.ImageFs.USER + "-" + newId)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(path-traversal-file-concat-kotlin)
🤖 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/settings/ContainerStorageManagerDialog.kt`
around lines 183 - 227, The confirmDetach flow performs the potentially large
FileUtils.copy operation on the Main dispatcher and silently exits when dstDir
already exists. Move the container lookup, destination check, and copy work in
confirmDetach to Dispatchers.IO, then switch back to the UI context for refresh
and loading-state updates; when the destination exists, provide explicit
user-visible handling instead of only logging and returning.
Source: Coding guidelines
Comment on lines
+486
to
+491
| SettingsMenuLink( | ||
| colors = settingsTileColorsAlt(), | ||
| title = { Text(text = "Manage Game Folders") }, | ||
| subtitle = { Text(text = "Add master directories or individual games") }, | ||
| onClick = { showCustomFoldersDialog = true } | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Hardcoded strings bypass this PR's own localization effort.
The new "Manage Game Folders" link's title/subtitle are literal English strings instead of stringResource(...), unlike every other label in this file. Given this PR adds strings.xml entries across 14 locales for the rest of the Battle.net/shared-container UI, these two strings will stay untranslated everywhere.
🌐 Proposed fix
SettingsMenuLink(
colors = settingsTileColorsAlt(),
- title = { Text(text = "Manage Game Folders") },
- subtitle = { Text(text = "Add master directories or individual games") },
+ title = { Text(text = stringResource(R.string.settings_manage_game_folders_title)) },
+ subtitle = { Text(text = stringResource(R.string.settings_manage_game_folders_subtitle)) },
onClick = { showCustomFoldersDialog = true }
)📝 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
| SettingsMenuLink( | |
| colors = settingsTileColorsAlt(), | |
| title = { Text(text = "Manage Game Folders") }, | |
| subtitle = { Text(text = "Add master directories or individual games") }, | |
| onClick = { showCustomFoldersDialog = true } | |
| ) | |
| SettingsMenuLink( | |
| colors = settingsTileColorsAlt(), | |
| title = { Text(text = stringResource(R.string.settings_manage_game_folders_title)) }, | |
| subtitle = { Text(text = stringResource(R.string.settings_manage_game_folders_subtitle)) }, | |
| onClick = { showCustomFoldersDialog = true } | |
| ) |
🤖 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/settings/SettingsGroupInterface.kt`
around lines 486 - 491, Replace the hardcoded title and subtitle text in the
SettingsMenuLink for “Manage Game Folders” with stringResource(...) lookups. Add
or reuse localized string resources for both labels across the supported
locales, following the existing resource naming and localization patterns used
in SettingsGroupInterface.kt.
Comment on lines
+4004
to
+4014
| } else if (isBattleNetGame) { | ||
| // Battle.net specific bypass | ||
| Timber.tag("XServerScreen").i("Launching Battle.net game: $appId") | ||
|
|
||
| val productCode = appId.substringAfter("BATTLE_NET_", "").lowercase() | ||
| val mappedExe = if (productCode.isNotEmpty()) BattleNetConstants.getExecutablePath(productCode) else "" | ||
|
|
||
| val exe = container.executablePath.ifEmpty { mappedExe }.ifEmpty { "Game.exe" } | ||
|
|
||
| // Always append -launch for Blizzard games to skip the unstable launcher | ||
| return "\"$exe\" -launch" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "BATTLE_NET" app/src/main/java/app/gamenative/utils/ContainerUtils.kt app/src/main/java/app/gamenative/data/LibraryItem.kt 2>/dev/null
rg -n "productCode|uid" app/src/main/java/app/gamenative/data/BattleNetGame.kt 2>/dev/nullRepository: utkarshdalal/GameNative
Length of output: 1178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '780,860p' app/src/main/java/app/gamenative/utils/ContainerUtils.kt
printf '\n---\n'
sed -n '1100,1155p' app/src/main/java/app/gamenative/utils/ContainerUtils.kt
printf '\n---\n'
sed -n '1,120p' app/src/main/java/app/gamenative/data/BattleNetGame.kt
printf '\n---\n'
sed -n '3980,4025p' app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
printf '\n---\n'
sed -n '1,180p' app/src/main/java/app/gamenative/data/LibraryItem.ktRepository: utkarshdalal/GameNative
Length of output: 12179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "getExecutablePath\\(|BATTLE_NET_|BattleNetConstants" app/src/main/java/app/gamenative -g '!**/build/**'Repository: utkarshdalal/GameNative
Length of output: 3096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' app/src/main/java/app/gamenative/data/BattleNetGame.kt
printf '\n---\n'
sed -n '780,840p' app/src/main/java/app/gamenative/utils/ContainerUtils.kt
printf '\n---\n'
sed -n '1270,1335p' app/src/main/java/app/gamenative/utils/ContainerUtils.kt
printf '\n---\n'
rg -n "workingDir =" app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt app/src/main/java/app/gamenative -g '!**/build/**'Repository: utkarshdalal/GameNative
Length of output: 8867
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "object BattleNetConstants|class BattleNetConstants|getExecutablePath" app/src/main/java/app/gamenative -g '!**/build/**'Repository: utkarshdalal/GameNative
Length of output: 584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,200p' app/src/main/java/app/gamenative/service/battle_net/BattleNetConstants.kt
printf '\n---\n'
rg -n "executablePath\\s*=|setExecutablePath|container\\.executablePath|BattleNetGameDao|productCode" app/src/main/java/app/gamenative -g '!**/build/**'Repository: utkarshdalal/GameNative
Length of output: 12624
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "BattleNetConstants|getExecutablePath|workingDir = File\\(|container\\.executablePath" app/src/main/java/app/gamenative/service app/src/main/java/app/gamenative/ui/screen/xserver app/src/main/java/app/gamenative/utils -g '!**/build/**'Repository: utkarshdalal/GameNative
Length of output: 6701
Battle.net launch needs the product code and install directory
appId is the source-prefixed uid, but BattleNetConstants.getExecutablePath() keys off productCode (wow, d3, etc.). Thread productCode through here, and set workingDir for the relative Blizzard exe path; otherwise most Battle.net titles won’t resolve or launch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` around
lines 4004 - 4014, The Battle.net launch branch in XServerScreen must pass the
extracted productCode through the launch configuration and set workingDir to the
game’s install directory when using a relative Blizzard executable path. Update
the BattleNetConstants lookup and return path to preserve the product-code
mapping, while keeping the fallback executable behavior intact.
Comment on lines
+368
to
+391
| // Shared Container Safeguard: Don't delete physical directory if other games use it. | ||
| if (containerId.startsWith("SHARED_")) { | ||
| val entryPoint = EntryPointAccessors.fromApplication( | ||
| context.applicationContext, | ||
| StorageManagerDaoEntryPoint::class.java, | ||
| ) | ||
| val installedGames = loadInstalledGames(context, entryPoint) | ||
|
|
||
| // Check if any OTHER installed game would resolve to this same SHARED ID | ||
| val occupants = installedGames.values.filter { game -> | ||
| val gameAppId = game.appId | ||
| // We need to calculate what the shared ID WOULD be for this game. | ||
| // For now, since sharedContainersEnabled is global, any installed game | ||
| // using the current default infrastructure config is an occupant. | ||
| // This is a simplified check. A robust check would load each game's config. | ||
| // Let's assume for now if it's installed and we are in shared mode, it's a potential occupant. | ||
| true | ||
| } | ||
|
|
||
| // Realistically, we should check if ANY app config points to this shared ID. | ||
| // Since we don't have a reverse mapping yet, we'll check if the directory is actually "empty" | ||
| // of registered users in the future. For now, let's keep it safe. | ||
| Timber.tag("ContainerStorageManager").i("Shared container removal requested for %s. Checking occupancy...", containerId) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Shared-container occupancy is never actually computed (filter { true }). Both sites depend on a reverse mapping from an installed app/its launch config to the shared container id it resolves to, which does not exist yet — so occupant detection is stubbed to "all games", making removal unsafe and occupant display wrong.
app/src/main/java/app/gamenative/utils/ContainerStorageManager.kt#L368-L391: compute the real occupant set for the shared id and refuse (or gate) deletion when other installed games still resolve to it, instead of logging and falling through to unconditional delete.app/src/main/java/app/gamenative/utils/ContainerStorageManager.kt#L769-L779: replace thefilter { true }occupant list with games whose resolved shared-container id equals this container's id, so the Storage Manager shows accurate occupants.
📍 Affects 1 file
app/src/main/java/app/gamenative/utils/ContainerStorageManager.kt#L368-L391(this comment)app/src/main/java/app/gamenative/utils/ContainerStorageManager.kt#L769-L779
🤖 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/utils/ContainerStorageManager.kt` around
lines 368 - 391, Replace the stubbed `filter { true }` logic with resolution of
each installed game's shared-container ID, and compare it to the requested
`containerId` in both `ContainerStorageManager.kt` sites at lines 368-391 and
769-779. Use the resulting occupant set to report accurate users, and gate or
refuse physical deletion in the shared-container removal flow when other
installed games resolve to the same ID.
Comment on lines
+450
to
+458
| fun applyToContainer(context: Context, container: Container, containerData: ContainerData, saveToDisk: Boolean) { | ||
| Timber.d("Applying containerData to container. execArgs: '${containerData.execArgs}', saveToDisk: $saveToDisk") | ||
|
|
||
| // If shared containers are enabled, we might want to save the app-specific config elsewhere | ||
| val appId = extractAppIdFromContainer(container) // Helper to get the actual game ID if possible | ||
| if (appId != null && saveToDisk && PrefManager.sharedContainersEnabled) { | ||
| AppLaunchConfigManager.saveConfig(context, appId, containerData) | ||
| Timber.d("[ContainerUtils]: Intercepted save to shared container; redirected to per-app config for $appId") | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files 'app/src/main/java/app/gamenative/utils/ContainerUtils.kt' \
'app/src/main/java/app/gamenative/utils/AppLaunchConfigManager.kt' \
'app/src/main/java/app/gamenative/**/*.java' \
'app/src/main/java/app/gamenative/**/*.kt' | sed 's#^`#-` #'
echo
echo "== outline ContainerUtils.kt =="
ast-grep outline app/src/main/java/app/gamenative/utils/ContainerUtils.kt --view expanded || true
echo
echo "== outline AppLaunchConfigManager.kt =="
ast-grep outline app/src/main/java/app/gamenative/utils/AppLaunchConfigManager.kt --view expanded || true
echo
echo "== search shared-container related symbols =="
rg -n "sharedContainersEnabled|extractAppIdFromContainer|current_app_id|saveConfig\\(|applyToContainer\\(" app/src/main/java/app/gamenative -SRepository: utkarshdalal/GameNative
Length of output: 44328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ContainerUtils around applyToContainer =="
sed -n '430,490p' app/src/main/java/app/gamenative/utils/ContainerUtils.kt | cat -n
echo
echo "== ContainerUtils around extractAppIdFromContainer =="
sed -n '1188,1210p' app/src/main/java/app/gamenative/utils/ContainerUtils.kt | cat -n
echo
echo "== AppLaunchConfigManager =="
sed -n '1,130p' app/src/main/java/app/gamenative/utils/AppLaunchConfigManager.kt | cat -n
echo
echo "== ContainerData definition and fields =="
rg -n "data class ContainerData|class ContainerData|ContainerData\\(" app/src/main/java/app/gamenative -SRepository: utkarshdalal/GameNative
Length of output: 21868
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== applyToContainer rest =="
sed -n '490,620p' app/src/main/java/app/gamenative/utils/ContainerUtils.kt | cat -n
echo
echo "== saveToDisk usage in ContainerUtils =="
rg -n "saveToDisk|save\\(|commit\\(|apply\\(" app/src/main/java/app/gamenative/utils/ContainerUtils.kt -n -S
echo
echo "== shared container paths that call applyToContainer(saveToDisk = true) =="
rg -n "applyToContainer\\(context, container, .*saveToDisk = true\\)|applyToContainer\\(context, container, .*\\)" app/src/main/java/app/gamenative -S | sed -n '1,120p'Repository: utkarshdalal/GameNative
Length of output: 11469
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== getContainer/getOrCreateContainer paths =="
sed -n '676,710p' app/src/main/java/app/gamenative/utils/ContainerUtils.kt | cat -n
echo
echo "== getOrCreateContainerWithOverride =="
sed -n '1066,1095p' app/src/main/java/app/gamenative/utils/ContainerUtils.kt | cat -n
echo
echo "== intent launch save path =="
sed -n '1168,1188p' app/src/main/java/app/gamenative/utils/ContainerUtils.kt | cat -n
echo
echo "== current_app_id writes =="
rg -n 'putSessionMetadata\("current_app_id"|current_app_id' app/src/main/java/app/gamenative/utils/ContainerUtils.kt app/src/main/java/app/gamenative -SRepository: utkarshdalal/GameNative
Length of output: 5771
Shared-container saves still write most settings back to the shared container. AppLaunchConfigManager only isolates a subset of ContainerData; applyToContainer(..., saveToDisk = true) still mutates the Container and calls saveData(), so fields outside that allowlist leak across apps sharing the same container. The redirect also only works when current_app_id is present, so any shared-container save path that misses that metadata falls back to the shared container. ContainerUtils.kt:450-458, 1200-1204
🤖 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/utils/ContainerUtils.kt` around lines 450 -
458, Update applyToContainer so shared-container saves never mutate the shared
Container or call saveData(). Detect shared-container mode independently of
current_app_id, resolve the app identity from the available launch/container
context, and route the complete ContainerData through AppLaunchConfigManager,
expanding its persisted fields as needed. Apply the same guard to the
shared-container save path near the other save handling so missing
current_app_id cannot fall back to shared storage.
Owner
|
@Meloon33 - your PRs need to be scoped down to a single feature, and you need to discuss it on Discord before making PRs. Don't remove the PR template. Secondly, this seems like it was completely done by Claude Code - which is fine, but please reply to this to prove that you are a human. |
Author
|
@utkarshdalal I am a human, this was done mostly by Gemini. I am working on scoping this down to one feature of shared containers and will reach out on Discord. |
Owner
|
Thanks |
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.
Summary
This PR introduces foundational support for the Battle.net platform, a major "Shared Containers" storage optimization, and comprehensive stability improvements for the Legacy flavor.
Key Features
1. Battle.net Integration (Phase 1 & 2)
-launch) for all major Blizzard titles (Diablo III, SC2, WoW, etc.) to skip the unstable desktop app.2. Robust Shared Game Containers
3. Legacy & UX Optimizations
4. Project Health
ARCHITECTURE.md,DEVELOPMENT_NOTES.md, and a technical roadmap.Summary by cubic
Adds Battle.net account connection, library sync, and smart launching, plus Shared Containers to cut storage per game and several stability/UX improvements for Legacy. This also updates the database to v25 and localizes new UI in 14 languages.
New Features
Legacy & Performance
Written for commit 7127305. Summary will update on new commits.
Summary by CodeRabbit