Dlc manager #320
Dlc manager #320
Conversation
WalkthroughAdds companion-level SteamService APIs for DLC enable/disable, app rebuild and diagnostics; introduces a DlcManagerDialog UI for managing DLC; extends SteamAppScreen to surface the dialog via a new ManageDLC menu option; adds two enum entries to AppOptionMenuType (one file contains unresolved merge conflict markers). Changes
Sequence DiagramsequenceDiagram
autonumber
participant User
participant SteamAppScreen
participant DlcManagerDialog
participant SteamService
participant AppInfoDB as AppInfo/DB
participant Container
User->>SteamAppScreen: Click "Manage DLC"
SteamAppScreen->>SteamAppScreen: showDlcManager = true
SteamAppScreen->>DlcManagerDialog: Render(appId, visible=true)
DlcManagerDialog->>SteamService: getDownloadableDepotsDebug(appId)
SteamService-->>DlcManagerDialog: downloadableDepots + reasons
Note over DlcManagerDialog: Filter/deduplicate by DLC app id\nDetect installed depots\nShow checkboxes
User->>DlcManagerDialog: Toggle selections, then "Save"
DlcManagerDialog->>SteamService: setAppSelection(appId, downloadedDepots, dlcDepots)
SteamService->>AppInfoDB: Persist AppInfo updates
DlcManagerDialog->>SteamService: rebuildApp(appId)
SteamService->>SteamService: Wi‑Fi check
alt Wi‑Fi allowed
SteamService->>SteamService: Delete install dir
SteamService->>SteamService: Download selected depots
SteamService->>SteamService: Clear DOWNLOAD_COMPLETE / STEAM_DLL_* markers
SteamService->>Container: Trigger unpacking state
SteamService-->>DlcManagerDialog: Rebuild complete
else Wi‑Fi denied
SteamService-->>DlcManagerDialog: Rebuild aborted (Wi‑Fi guard)
end
DlcManagerDialog->>SteamAppScreen: onDismissRequest()
SteamAppScreen->>SteamAppScreen: showDlcManager = false
SteamAppScreen->>User: Dialog closed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/ui/screen/library/LibraryAppScreen.kt (1)
183-932: Critical: Resolve merge conflict before merging.This file contains unresolved merge conflict markers spanning lines 183-932. The conflict appears to be between two fundamentally different architectural approaches:
- HEAD branch: Uses a delegation pattern where
AppScreendelegates toSteamAppScreen()orCustomGameAppScreen()based on game source- Incoming branch (d79b495): Implements all state management and UI logic directly inline within
AppScreenThe code cannot compile with these conflict markers present. You must resolve the conflict by choosing one approach or carefully merging the two implementations.
Recommendation: Based on the retrieved learnings and the AI summary mentioning that this PR introduces DLC management, it appears the incoming branch contains the new DLC features. Consider:
- Adopting the delegation pattern from HEAD (cleaner architecture)
- Moving the DLC management state and dialog integration into the delegated
SteamAppScreenclass- Ensuring the
DlcManagerDialogimport (line 84) and related logic (lines 229, 582-588, 900-906) are preserved in the final resolution
🧹 Nitpick comments (5)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
1379-1383: Working directory for modern DRM path looks consistent; consider small reuseSetting
guestProgramLauncherComponent.workingDirfor the non‑legacy Steam path aligns this branch with the legacy/custom branches and should help games that rely on CWD; the logging is also useful for debugging. If you touch this again, you might want to extract theappDirPath + executablePath.substringBeforeLast("/")pattern into a small helper to avoid duplication with the legacy DRM block, but it’s fine as‑is.app/src/main/java/app/gamenative/utils/KeyValueUtils.kt (1)
107-127: DLC ID parsing looks solid; only minor, optional cleanups.The parsing behavior here is robust: it handles null/absent, empty, partially invalid, and fully invalid
listofdlcvalues correctly, returningemptyList()when appropriate and logging helpful warnings for non-empty unparseable tokens. This should be safe for downstream DLC flows.A couple of small, optional refinements:
- Use the imported
Timberfor consistency with the rest of the file (e.g.,printAllKeyValues), instead of the fully qualifiedtimber.log.Timber:- timber.log.Timber.w("Failed to parse DLC ID: '$trimmed'") + Timber.w("Failed to parse DLC ID: '$trimmed'") @@ - timber.log.Timber.d("Parsed ${list.size} DLC IDs from listofdlc: $list") + Timber.d("Parsed ${list.size} DLC IDs from listofdlc: $list") @@ - timber.log.Timber.d("listofdlc field was empty or invalid") + Timber.d("listofdlc field was empty or invalid") @@ - timber.log.Timber.d("No listofdlc field found (value=$rawValue)") + Timber.d("No listofdlc field found (value=$rawValue)")
(Optional) Avoid double lookup of the same key path by caching the raw value once, e.g. with a local
val rawListOfDlc = this["common"]["extended"]["listofdlc"].value, then usingrawListOfDlcin both the parsing chain and the fallback logging. This keeps the code a bit clearer and avoids repeating the nested index chain.(Optional) Consider whether duplicates should be collapsed with
.distinct()at the end of the chain if the source data might contain duplicate DLC IDs and that’s undesirable for your manager logic.Overall, no blocking issues here; these are just polish items.
app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt (2)
83-228: Avoid mutating Compose state fromDispatchers.IOin the initial load effectWithin
LaunchedEffect(appId, visible), thewithContext(Dispatchers.IO)block both performs IO and mutates Compose state holders (dlcList,knownDlcList,installedDepots,checkedMap,loading). While this often “works”, it defeats the usual main‑thread invariants for snapshot state and can lead to subtle snapshot consistency issues.A safer pattern is:
- Do all SteamService / PICS / depot discovery work into local collections inside
withContext(Dispatchers.IO).- After the IO block completes, update
dlcList,knownDlcList,installedDepots,checkedMap, andloadingback on the main thread (i.e., afterwithContextreturns), or via an innerwithContext(Dispatchers.Main)for the assignments.This keeps heavy work on the IO dispatcher but ensures all state mutations stay on the main/UI thread.
71-81: ClarifyknownDlcListboolean semantics vs user selection
knownDlcListis documented and populated with triples of(dlcAppId, name, owned)— the Boolean flag currently reflects “owned” status (e.g.,Triple(did, name, did in owned)and for manual additions via PICS).However, in the Save path:
val selectedManualDlc = knownDlcList.filter { it.third }.map { it.first } val selectedDlcAppIds = (selectedDlcFromDepots + selectedManualDlc).distinct() SteamService.setAppSelection(appId, newSelected, selectedDlcAppIds)
it.thirdis effectively treated as “selected”, not just “owned”, and there is no UI affordance to toggle that Boolean for auto‑discovered entries. The result is:
- Any “owned” DLC in
knownDlcListis always treated as selected on Save.- For manual App IDs, a user cannot “add but not select” or later deselect an entry.
If the intent is “owned == selected” this is fine, but if the design is “show owned DLC and allow the user to choose which to enable”, you’ll likely want a separate selection flag (e.g.,
data class KnownDlc(val appId: Int, val name: String, val owned: Boolean, val selected: Boolean)) and explicit UI controls (checkbox or switch) to toggleselectedindependently ofowned.Also applies to: 178-204, 320-368, 528-537, 643-646
app/src/main/java/app/gamenative/service/SteamService.kt (1)
773-829: Diagnostics helpers and PICS refresh look good; document non‑UI usage
getDownloadableDepotsDebugandgetPotentialDownloadableDepotsmirrorgetDownloadableDepots’s logic and provide useful insight into why depots are included or excluded, plus a relaxed “potential” set for diagnostics.- Both helpers call
runBlocking { getOwnedAppDlc(appId) }, which in turn does network/DB work; they’re intended for diagnostics (as used by the DLC manager’s “Show diagnostics” toggle), so they should be considered non‑UI APIs and only called from background threads.refreshPICSForApp(appId)correctly:
- Fetches fresh PICS data for the app.
- Preserves packageId/ownerAccountId/licenseFlags from existing DB entries.
- Updates
lastChangeNumberandreceivedPICS.- Logs the number of DLC IDs discovered.
Functionally these helpers look sound; it may be worth documenting (KDoc or
@WorkerThread) that the debug methods are blocking and should not be invoked on the main thread, especially since they’re now wired into a UI dialog.Also applies to: 830-875, 2244-2283
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/src/main/java/app/gamenative/enums/Marker.kt(1 hunks)app/src/main/java/app/gamenative/service/SteamService.kt(5 hunks)app/src/main/java/app/gamenative/ui/enums/AppOptionMenuType.kt(1 hunks)app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt(4 hunks)app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt(5 hunks)app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt(1 hunks)app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt(1 hunks)app/src/main/java/app/gamenative/utils/KeyValueUtils.kt(1 hunks)app/src/main/java/app/gamenative/utils/SteamUtils.kt(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-28T13:54:16.048Z
Learnt from: utkarshdalal
Repo: utkarshdalal/GameNative PR: 176
File: app/src/main/java/app/gamenative/utils/SteamUtils.kt:651-655
Timestamp: 2025-09-28T13:54:16.048Z
Learning: In the GameNative project, when creating depots.txt files in SteamUtils.ensureSteamSettings, only create the file when SteamService.getInstalledDepotsOf() returns actual data (non-null). Don't create empty files or write empty strings when no data is available.
Applied to files:
app/src/main/java/app/gamenative/utils/SteamUtils.ktapp/src/main/java/app/gamenative/utils/KeyValueUtils.ktapp/src/main/java/app/gamenative/service/SteamService.kt
📚 Learning: 2025-09-19T17:07:27.941Z
Learnt from: utkarshdalal
Repo: utkarshdalal/GameNative PR: 159
File: app/src/main/java/app/gamenative/service/DownloadService.kt:6-6
Timestamp: 2025-09-19T17:07:27.941Z
Learning: In app/src/main/java/app/gamenative/service/DownloadService.kt, the getSizeFromStoreDisplay() function uses runBlocking to call SteamService.getDownloadableDepots() because the latter was converted from synchronous to suspend in this PR. The user confirmed the previous synchronous implementation worked fine, and if performance is acceptable, keeping runBlocking is a valid approach.
Applied to files:
app/src/main/java/app/gamenative/utils/SteamUtils.ktapp/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.ktapp/src/main/java/app/gamenative/service/SteamService.kt
📚 Learning: 2025-09-19T17:13:01.017Z
Learnt from: utkarshdalal
Repo: utkarshdalal/GameNative PR: 159
File: app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt:136-136
Timestamp: 2025-09-19T17:13:01.017Z
Learning: In LibraryAppScreen.kt, the user prefers to use runBlocking to maintain synchronous APIs when underlying methods have been converted to suspend functions, rather than refactoring all calling code to be async. This approach prevents UI breakage and maintains API compatibility. The user confirmed this is acceptable when performance is not a concern.
Applied to files:
app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt
📚 Learning: 2025-09-28T13:56:06.888Z
Learnt from: utkarshdalal
Repo: utkarshdalal/GameNative PR: 176
File: app/src/main/java/app/gamenative/service/SteamService.kt:179-180
Timestamp: 2025-09-28T13:56:06.888Z
Learning: In the GameNative project, the AppInfo table (with AppInfoDao) tracks local game installation state including which apps are downloaded and which depots were installed. This data should NOT be cleared during logout in clearDatabase() because games remain physically installed on the device and users should see their installed games when logging back in. Only user-specific Steam account data should be cleared on logout.
Applied to files:
app/src/main/java/app/gamenative/service/SteamService.kt
🧬 Code graph analysis (2)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt (1)
app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt (1)
DlcManagerDialog(63-677)
app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt (8)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt (1)
isDownloading(280-283)app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.kt (1)
isDownloading(72-72)app/src/main/java/app/gamenative/ui/screen/library/appscreen/CustomGameAppScreen.kt (1)
isDownloading(191-194)app/src/main/java/app/gamenative/service/SteamService.kt (1)
getAppDirPath(886-908)app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt (1)
DlcManagerDialog(63-677)app/src/main/java/app/gamenative/ui/component/dialog/LoadingDialog.kt (1)
LoadingDialog(26-60)app/src/main/java/app/gamenative/utils/ShortcutUtils.kt (1)
createPinnedShortcut(87-157)app/src/main/java/com/winlator/container/ContainerManager.java (1)
ContainerManager(35-422)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt (1)
34-34: DLC manager dialog wiring looks consistent and matches existing patternsThe shared
dlcManagerAppIdsstate,ManageDLCmenu option, andAdditionalDialogsintegration mirror the uninstall dialog approach and correctly distinguish betweenlibraryItem.appId(container id used for UI state) andlibraryItem.gameId(Steam app id passed intoDlcManagerDialog). No functional issues stand out here.Also applies to: 102-105, 695-703, 753-762, 1117-1124
app/src/main/java/app/gamenative/utils/SteamUtils.kt (1)
148-156: FORCE_FIRST_RUN and ColdClient ini updates look consistent with the new flow
- Additional logging and the
STEAM_DLL_REPLACEDmarker checks inreplaceSteamApiare non‑intrusive and help trace setup issues.replaceSteamclientDll’sFORCE_FIRST_RUNhandling correctly overrides theSTEAM_COLDCLIENT_USEDshort‑circuit once, then clears the marker, which matches the intent of a one‑shot forced setup.- The new
ExeRunDirfield inwriteColdClientInicorrectly derives the directory fromcontainer.executablePathand falls back to empty when the exe is at the game root.- Writing an extra
steam_appid.txtalongside the executable inensureSteamSettingsis defensive and safely wrapped inrunCatching, without changing the existing depots.txt behavior (per earlier learnings).No functional regressions spotted here.
Also applies to: 215-248, 254-259, 738-748
app/src/main/java/app/gamenative/enums/Marker.kt (1)
3-9: FORCE_FIRST_RUN marker addition is consistent with existing marker conventions
FORCE_FIRST_RUN(".force_first_run")fits the established naming/file pattern and aligns with its usage in SteamService and SteamUtils to drive the new first‑run/setup flow.app/src/main/java/app/gamenative/service/SteamService.kt (2)
637-664:rebuildAppWithDepotsshould be invoked from a background dispatcher to avoid blocking the UI thread
rebuildAppWithDepotsperforms synchronous filesystem operations including:
appDir.deleteRecursively()on potentially large directoriesforceFirstRunSetup(appId)which touches markers and container data- These run on the caller's thread before
downloadAppdispatches its coroutineCalling this from UI code (e.g., DlcManagerDialog handlers) risks stalling the main thread. Ensure all call sites invoke this from a background dispatcher, either by:
- Wrapping in
instance?.scope?.launch(Dispatchers.IO) { rebuildAppWithDepots(...) }, or- Using
withContext(Dispatchers.IO)at call sites before invokingThis maintains consistency with how
rebuildAppis invoked fromdisableDlc.
289-316: Ensure AppInfo.dlcDepots reflects user selection and isn't overwritten on download completionThe new DLC APIs (
setAppSelection,disableDlc,rebuildApp/rebuildAppWithDepots) plusforceFirstRunSetupform a coherent story, but there's a potential persistence gap:
setAppSelectionanddisableDlcupdateAppInfo.dlcDepotsto reflect user‑chosen DLC (anddisableDlctriggers arebuildApp(appId)afterward).AppDownloadListener.onDownloadCompletedmay unconditionally resetdlcDepotsback to all owned DLC detected bygetOwnedAppDlc()every time a download finishes, which would undo any prior manual selection or disables recorded in AppInfo.If the intent is for DLC selection/disable to be user‑controllable and persistent, consider:
- Reading the existing AppInfo row in
onDownloadCompletedand preserving itsdlcDepotswhen present.- Or, alternatively, delegating
dlcDepotsupdates entirely tosetAppSelectionand avoiding overwriting it inonDownloadCompletedonce an AppInfo row exists.This would make the new DLC manager and
disableDlcbehavior durable across rebuilds.app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt (1)
260-307: Move heavy SteamService and filesystem work off the main thread in refresh/diagnostics/save flowsThe following blocks all run in a
rememberCoroutineScope()launched coroutine, which by default uses the main dispatcher:
- "Refresh ownership" button: calls
SteamService.refreshPICSForApp,SteamService.getDownloadableDepots,SteamService.getAppDlc,SteamService.getOwnedGames, etc.- "Show diagnostics" button: calls
SteamService.getAppDlc,SteamService.getDownloadableDepotsDebug,SteamService.getPotentialDownloadableDepots,SteamService.checkDlcOwnershipViaPICSBatch,SteamService.getOwnedGames,SteamService.getInstalledDepotsOf, etc.- "Save" button: calls
SteamService.setAppSelectionandSteamService.rebuildAppWithDepots.These helpers ultimately invoke synchronous work (including database access and
deleteRecursivelyon the install directory) and can easily block the UI for noticeable periods on large titles.The synchronous nature of these calls is acceptable when used from background threads. Restructure these handlers as:
- Launch with
scope.launch { loading = true; try { withContext(Dispatchers.IO) { /* all SteamService & filesystem work */ } } finally { loading = false } }.- Within the IO block, accumulate results into local collections/booleans.
- After the IO section, switch back to main for any Compose state changes and
Toastcalls (withContext(Dispatchers.Main)or implicitly afterwithContextreturns).- Ensure
rebuildAppWithDepotsand similar calls happen inside the IO section, not on main.This will keep the dialog responsive even when DLC discovery or rebuilds are expensive.
Also applies to: 380-497, 625-673
| <<<<<<< HEAD | ||
| ForceUploadLocal("Force upload local saves"), | ||
| FetchSteamGridDBImages("Fetch game images") | ||
| FetchSteamGridDBImages("Fetch game images"), | ||
| ======= | ||
| ForceUploadLocal("Force upload local saves") | ||
| , | ||
| >>>>>>> d79b495de29440bf25c29fb8fc3b393913be82da | ||
| ManageDLC("Manage DLC") |
There was a problem hiding this comment.
Unresolved merge conflict markers will cause a compilation failure.
The file contains Git merge conflict markers (<<<<<<<, =======, >>>>>>>) that must be resolved before this code can compile.
Apply this diff to resolve the merge conflict:
-<<<<<<< HEAD
- ForceUploadLocal("Force upload local saves"),
- FetchSteamGridDBImages("Fetch game images"),
-=======
- ForceUploadLocal("Force upload local saves")
- ,
->>>>>>> d79b495de29440bf25c29fb8fc3b393913be82da
- ManageDLC("Manage DLC")
+ ForceUploadLocal("Force upload local saves"),
+ FetchSteamGridDBImages("Fetch game images"),
+ ManageDLC("Manage DLC")🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/ui/enums/AppOptionMenuType.kt around lines
20 to 27, there are unresolved Git conflict markers (<<<<<<<, =======, >>>>>>>)
that will break compilation; remove the conflict markers and merge the two
variants into a single valid Kotlin enum list by keeping both entries
(ForceUploadLocal and FetchSteamGridDBImages) as separate enum constants, ensure
proper commas between constants so the enum reads e.g. ForceUploadLocal("Force
upload local saves"), FetchSteamGridDBImages("Fetch game images"),
ManageDLC("Manage DLC"), and remove any stray whitespace or markers.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
630-655: Same issues asrebuildApp: redundant marker removal and premature DRM reset.This method has the same two issues identified in
rebuildApp(Lines 535-584):
- Line 633 removes the marker before
deleteRecursively()on Line 634, which is redundant- Lines 643-654 reset DRM markers and container state immediately after scheduling the download, not after completion
Apply the same fix as suggested for
rebuildApp:try { val appDir = File(getAppDirPath(appId)) if (appDir.exists()) { - MarkerUtils.removeMarker(appDir.path, Marker.DOWNLOAD_COMPLETE_MARKER) appDir.deleteRecursively() } } catch (t: Throwable) { Timber.w(t, "Failed to delete install dir for rebuildWithDepots of $appId") } val di = downloadApp(appId, depotIds, "public") val success = di != null - if (success) { - // Mirror Reset DRM timing: only after successful delete + redownload - try { - val appDirPath = getAppDirPath(appId) - MarkerUtils.removeMarker(appDirPath, Marker.STEAM_DLL_REPLACED) - MarkerUtils.removeMarker(appDirPath, Marker.STEAM_DLL_RESTORED) - MarkerUtils.removeMarker(appDirPath, Marker.STEAM_COLDCLIENT_USED) - val containerId = "STEAM_" + appId - val container = ContainerUtils.getOrCreateContainer(instance!!.applicationContext, containerId) - container.isNeedsUnpacking = true - container.saveData() - } catch (_: Throwable) { } - } success
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt (1)
395-396: Remove unused variables.The
removedandaddedvariables are computed but never used. Remove them to reduce noise.Apply this diff:
val finalSelected = (baseDepotIds + selectedDlcDepotIds).distinct() - val removed = installedDepots.filterNot { it in finalSelected } - val added = finalSelected.filterNot { it in installedDepots } val selectedDlcFromDepots = dlcList.filter { (depotId, _) -> depotId in selectedDlcDepotIds }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/service/SteamService.kt(3 hunks)app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt(3 hunks)app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-19T17:07:27.941Z
Learnt from: utkarshdalal
Repo: utkarshdalal/GameNative PR: 159
File: app/src/main/java/app/gamenative/service/DownloadService.kt:6-6
Timestamp: 2025-09-19T17:07:27.941Z
Learning: In app/src/main/java/app/gamenative/service/DownloadService.kt, the getSizeFromStoreDisplay() function uses runBlocking to call SteamService.getDownloadableDepots() because the latter was converted from synchronous to suspend in this PR. The user confirmed the previous synchronous implementation worked fine, and if performance is acceptable, keeping runBlocking is a valid approach.
Applied to files:
app/src/main/java/app/gamenative/service/SteamService.kt
📚 Learning: 2025-09-28T13:54:16.048Z
Learnt from: utkarshdalal
Repo: utkarshdalal/GameNative PR: 176
File: app/src/main/java/app/gamenative/utils/SteamUtils.kt:651-655
Timestamp: 2025-09-28T13:54:16.048Z
Learning: In the GameNative project, when creating depots.txt files in SteamUtils.ensureSteamSettings, only create the file when SteamService.getInstalledDepotsOf() returns actual data (non-null). Don't create empty files or write empty strings when no data is available.
Applied to files:
app/src/main/java/app/gamenative/service/SteamService.kt
📚 Learning: 2025-09-28T13:56:06.888Z
Learnt from: utkarshdalal
Repo: utkarshdalal/GameNative PR: 176
File: app/src/main/java/app/gamenative/service/SteamService.kt:179-180
Timestamp: 2025-09-28T13:56:06.888Z
Learning: In the GameNative project, the AppInfo table (with AppInfoDao) tracks local game installation state including which apps are downloaded and which depots were installed. This data should NOT be cleared during logout in clearDatabase() because games remain physically installed on the device and users should see their installed games when logging back in. Only user-specific Steam account data should be cleared on logout.
Applied to files:
app/src/main/java/app/gamenative/service/SteamService.kt
🪛 detekt (1.23.8)
app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt
[warning] 74-74: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 116-116: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 152-152: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 392-392: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 410-410: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
507-512: VerifyisDownloadedlogic when disabling DLC.Line 509 sets
isDownloaded = newDownloaded.isNotEmpty(), which may incorrectly mark the app as not downloaded if the last DLC depot is removed but base game depots are still installed. Clarify whetherdownloadedDepotscontains both base and DLC depots, and whetherisDownloadedshould reflect the presence of any installed depots or only selected ones.
| try { | ||
| val appDir = File(getAppDirPath(appId)) | ||
| if (appDir.exists()) { | ||
| MarkerUtils.removeMarker(appDir.path, Marker.DOWNLOAD_COMPLETE_MARKER) | ||
| appDir.deleteRecursively() | ||
| } | ||
| } catch (t: Throwable) { | ||
| Timber.w(t, "Failed to delete install dir for rebuild of $appId") | ||
| } | ||
|
|
||
| val di = downloadApp(appId, depotIds, "public") | ||
| val success = di != null | ||
|
|
||
| // After delete + redownload completes for DLC changes, mirror Reset DRM poke | ||
| if (success) { | ||
| try { | ||
| val appDirPath = getAppDirPath(appId) | ||
| MarkerUtils.removeMarker(appDirPath, Marker.STEAM_DLL_REPLACED) | ||
| MarkerUtils.removeMarker(appDirPath, Marker.STEAM_DLL_RESTORED) | ||
| MarkerUtils.removeMarker(appDirPath, Marker.STEAM_COLDCLIENT_USED) | ||
| val containerId = "STEAM_" + appId | ||
| val container = ContainerUtils.getOrCreateContainer(instance!!.applicationContext, containerId) | ||
| container.isNeedsUnpacking = true | ||
| container.saveData() | ||
| } catch (_: Throwable) { } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Marker removal is redundant, and DRM reset timing may be premature.
Line 555 removes the DOWNLOAD_COMPLETE_MARKER before calling deleteRecursively() on Line 556, which removes the entire directory including all markers. The explicit marker removal is redundant.
Additionally, Lines 566-577 reset DRM markers and set container.isNeedsUnpacking = true immediately after scheduling the download (Line 562), not after the download completes. This means the container state is marked as needing unpacking before any files are downloaded, which could lead to issues if the rebuild is cancelled or fails.
Consider:
- Remove the redundant marker removal before
deleteRecursively() - Move the DRM reset and container state update to the download completion handler (similar to Lines 1440-1441 in
AppDownloadListener.onDownloadCompleted)
Apply this diff:
// Delete the existing install directory so we get a clean rebuild
try {
val appDir = File(getAppDirPath(appId))
if (appDir.exists()) {
- MarkerUtils.removeMarker(appDir.path, Marker.DOWNLOAD_COMPLETE_MARKER)
appDir.deleteRecursively()
}
} catch (t: Throwable) {
Timber.w(t, "Failed to delete install dir for rebuild of $appId")
}
val di = downloadApp(appId, depotIds, "public")
val success = di != null
- // After delete + redownload completes for DLC changes, mirror Reset DRM poke
- if (success) {
- try {
- val appDirPath = getAppDirPath(appId)
- MarkerUtils.removeMarker(appDirPath, Marker.STEAM_DLL_REPLACED)
- MarkerUtils.removeMarker(appDirPath, Marker.STEAM_DLL_RESTORED)
- MarkerUtils.removeMarker(appDirPath, Marker.STEAM_COLDCLIENT_USED)
- val containerId = "STEAM_" + appId
- val container = ContainerUtils.getOrCreateContainer(instance!!.applicationContext, containerId)
- container.isNeedsUnpacking = true
- container.saveData()
- } catch (_: Throwable) { }
- }
successThen in AppDownloadListener.onDownloadCompleted (around Line 1440), the DRM markers are already being reset, so the timing is correct there.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/SteamService.kt around lines 552 to
577, remove the redundant MarkerUtils.removeMarker call that occurs immediately
before appDir.deleteRecursively(), and do not reset DRM-related markers or set
container.isNeedsUnpacking here; instead move that DRM marker reset +
container.isNeedsUnpacking update into the download completion flow (use the
existing logic in AppDownloadListener.onDownloadCompleted around line ~1440 or
call the same helper from that handler) so the markers and container state are
only updated after a successful download completes.
| // Try to auto-discover DLC app ids from the user's owned games (match by name) as a fast path | ||
| val baseName = SteamService.getAppInfoOf(appId)?.name ?: "" | ||
| val discovered = mutableSetOf<Int>() | ||
| try { | ||
| val userSteamId = SteamService.userSteamId | ||
| if (userSteamId != null) { | ||
| val ownedGames = SteamService.getOwnedGames(userSteamId.convertToUInt64()) | ||
| val ownedIds = ownedGames.map { it.appId } | ||
| ownedIds.forEach { cid -> | ||
| val info = SteamService.getAppInfoOf(cid) | ||
| val nm = info?.name ?: "" | ||
| if (baseName.isNotEmpty() && nm.contains(baseName, ignoreCase = true) && cid != appId) { | ||
| discovered.add(cid) | ||
| } | ||
| } | ||
| } | ||
| } catch (_: Throwable) { | ||
| // ignore | ||
| } |
There was a problem hiding this comment.
Potential false positives in name-based DLC discovery.
The name-based discovery uses a simple substring match (nm.contains(baseName, ignoreCase = true)) which could match unrelated games, especially when the base game name is short or generic (e.g., "The Game", "Pro", "HD"). Consider adding safeguards:
- Skip discovery if
baseNameis too short (e.g., < 5 characters) - Use more restrictive matching (e.g., startsWith, or word boundary checks)
- Limit the candidate pool to avoid performance issues with large libraries
Apply this diff to add a minimum length check:
// Try to auto-discover DLC app ids from the user's owned games (match by name) as a fast path
val baseName = SteamService.getAppInfoOf(appId)?.name ?: ""
val discovered = mutableSetOf<Int>()
- try {
+ if (baseName.length >= 5) {
+ try {
val userSteamId = SteamService.userSteamId
if (userSteamId != null) {
val ownedGames = SteamService.getOwnedGames(userSteamId.convertToUInt64())
val ownedIds = ownedGames.map { it.appId }
ownedIds.forEach { cid ->
val info = SteamService.getAppInfoOf(cid)
val nm = info?.name ?: ""
if (baseName.isNotEmpty() && nm.contains(baseName, ignoreCase = true) && cid != appId) {
discovered.add(cid)
}
}
}
- } catch (_: Throwable) {
- // ignore
+ } catch (_: Throwable) {
+ // ignore
+ }
}🤖 Prompt for AI Agents
app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt
around lines 161 to 179: the current name-based DLC discovery can produce false
positives because it uses a simple substring match against very short or generic
base names; update the logic to skip discovery when baseName length is below a
threshold (e.g., < 5), narrow matching to more restrictive checks (e.g.,
startsWith or word-boundary regex) instead of plain contains, and limit the
candidate pool (e.g., cap ownedIds iteration to the first N entries or filter by
genre/publisher if available) to avoid performance issues with large libraries;
implement these checks before adding cid to discovered and keep the existing
try/catch behavior.
| val setOk = SteamService.setAppSelection(appId, finalSelected, selectedDlcAppIds) | ||
| if (!setOk) { | ||
| Toast.makeText(ctx, "Failed to save selection", Toast.LENGTH_SHORT).show() | ||
| } else { | ||
| Toast.makeText(ctx, "Applying changes and rebuilding...", Toast.LENGTH_SHORT).show() | ||
| SteamService.rebuildAppWithDepots(appId, finalSelected) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid unnecessary rebuilds when no changes are made.
The Save button always calls rebuildAppWithDepots, which deletes the install directory and redownloads all depots, even if the user made no changes. This wastes bandwidth and time. Consider checking whether finalSelected differs from installedDepots before triggering the rebuild.
Apply this diff:
val setOk = SteamService.setAppSelection(appId, finalSelected, selectedDlcAppIds)
if (!setOk) {
Toast.makeText(ctx, "Failed to save selection", Toast.LENGTH_SHORT).show()
- } else {
+ } else if (finalSelected.toSet() != installedDepots.toSet()) {
Toast.makeText(ctx, "Applying changes and rebuilding...", Toast.LENGTH_SHORT).show()
SteamService.rebuildAppWithDepots(appId, finalSelected)
+ } else {
+ Toast.makeText(ctx, "No changes detected", Toast.LENGTH_SHORT).show()
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt
around lines 403 to 409, the Save path always calls
SteamService.rebuildAppWithDepots which forces a full reinstall even when
nothing changed; modify the logic so after SteamService.setAppSelection returns
true you compare finalSelected to the current installedDepots (or another
canonical representation of currently installed depots) and only call
SteamService.rebuildAppWithDepots(appId, finalSelected) if they differ; if they
are equal, show a toast like "No changes to apply" and skip the rebuild to avoid
unnecessary downloads.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt (2)
129-133: Name-based DLC discovery can still produce noisy matches and unnecessary work.This block will happily treat any owned app whose name
contains(baseName, ignoreCase = true)as a DLC candidate. For short or generic base titles this can easily over‑match, and iterating allownedGameswithout constraints can be costly for very large libraries.The previous bot already flagged this; I’d still recommend tightening it up:
- Skip discovery when
baseNameis very short (e.g.< 5chars).- Optionally cap how many owned IDs you scan, to avoid worst‑case performance on huge libraries.
- If you keep substring matching, at least gate it behind the length check; you can later refine to a stricter match (e.g.
startsWithor simple word‑boundary regex) if needed.Minimal change based on the earlier suggestion:
- // Try to auto-discover DLC app ids from the user's owned games (match by name) as a fast path - val baseName = SteamService.getAppInfoOf(appId)?.name ?: "" - val discovered = mutableSetOf<Int>() - try { - val userSteamId = SteamService.userSteamId - if (userSteamId != null) { - val ownedGames = SteamService.getOwnedGames(userSteamId.convertToUInt64()) - val ownedIds = ownedGames.map { it.appId } - ownedIds.forEach { cid -> - val info = SteamService.getAppInfoOf(cid) - val nm = info?.name ?: "" - if (baseName.isNotEmpty() && nm.contains(baseName, ignoreCase = true) && cid != appId) { - discovered.add(cid) - } - } - } - } catch (_: Throwable) { - // ignore - } + // Try to auto-discover DLC app ids from the user's owned games (match by name) as a fast path + val baseName = SteamService.getAppInfoOf(appId)?.name ?: "" + val discovered = mutableSetOf<Int>() + if (baseName.length >= 5) { + try { + val userSteamId = SteamService.userSteamId + if (userSteamId != null) { + val ownedGames = SteamService.getOwnedGames(userSteamId.convertToUInt64()) + val ownedIds = ownedGames.map { it.appId } + ownedIds.forEach { cid -> + val info = SteamService.getAppInfoOf(cid) + val nm = info?.name ?: "" + if (nm.contains(baseName, ignoreCase = true) && cid != appId) { + discovered.add(cid) + } + } + } + } catch (_: Throwable) { + // ignore + } + }You can further refine matching or add a cap on
ownedIdsif this proves noisy in practice.Also applies to: 165-169, 179-194, 196-201
394-421: Skip rebuild when depot selection hasn’t changed, and consider keeping dialog open on failure.Right now the Save path always calls:
SteamService.rebuildAppWithDepots(appId, finalSelected)and then dismisses the dialog in
finally, even whenfinalSelectedis identical toinstalledDepotsorsetAppSelectionfails. That means:
- Unnecessary full reinstall when the user didn’t actually change anything.
- The dialog disappears even after a failure, which can feel jarring.
The earlier bot already suggested gating the rebuild on an actual change; I’d still recommend something like:
- val setOk = SteamService.setAppSelection(appId, finalSelected, selectedDlcAppIds) - if (!setOk) { - Toast.makeText(ctx, "Failed to save selection", Toast.LENGTH_SHORT).show() - } else { - Toast.makeText(ctx, "Applying changes and rebuilding...", Toast.LENGTH_SHORT).show() - SteamService.rebuildAppWithDepots(appId, finalSelected) - } + val setOk = SteamService.setAppSelection(appId, finalSelected, selectedDlcAppIds) + if (!setOk) { + Toast.makeText(ctx, "Failed to save selection", Toast.LENGTH_SHORT).show() + } else if (finalSelected.toSet() != installedDepots.toSet()) { + Toast.makeText(ctx, "Applying changes and rebuilding...", Toast.LENGTH_SHORT).show() + SteamService.rebuildAppWithDepots(appId, finalSelected) + } else { + Toast.makeText(ctx, "No changes detected", Toast.LENGTH_SHORT).show() + }Optionally, you could also only call
onDismissRequest()whensetOkis true, so the user can retry or adjust their choices after a failure.
🧹 Nitpick comments (4)
app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt (4)
47-53: KDoc is stale relative to current implementation.The doc still talks about
getOwnedAppDlc, directly starting depot downloads, and removing depot IDs fromAppInfo, but the implementation now works viagetDownloadableDepots,setAppSelection, andrebuildAppWithDepots. Consider updating the KDoc to match the current behavior so future readers don’t have to reverse‑engineer intent.
79-83: Swallowed exceptions reduce diagnosability; consider lightweight logging.Several
catch (_: Throwable)orcatch (t: Throwable)blocks either return empty collections or show a very generic toast, effectively discarding the underlying error. That makes field debugging hard and is what detekt is warning about.At minimum, I’d suggest:
- Log the exception (e.g., to Logcat or your existing
MarkerUtils/Markerinfrastructure) before falling back.- Where user‑visible, keep the friendly toast but still record the root cause for developers.
For example, one representative pattern could be:
val downloadables = try { SteamService.getDownloadableDepots(appId) } catch (t: Throwable) { // e.g. MarkerUtils.log(Marker.DlcManager, "getDownloadableDepots failed", t) emptyMap<Int, DepotInfo>() }And similarly for the other sites flagged by detekt.
Also applies to: 129-133, 165-169, 231-233, 254-256, 262-264, 268-271, 398-402, 417-418
287-320: Confirm intent ofknownDlcList.thirdbeing reused as “selected” in Save.
knownDlcListstores(appId, name, owned)andthirdis populated from PICS ownership checks or manual add:
- For manifest DLC:
Triple(did, name, did in owned)- For manual DLC:
Triple(id, name, ownedFromPICS)In the Save handler you compute:
val selectedManualDlc = knownDlcList.filter { it.third }.map { it.first }which effectively treats “owned according to PICS” as “selected/enabled”, with no way for the user to opt out per‑entry in the UI.
If
selectedDlcAppIdsis meant to represent “enabled DLCs”, you might want a separate boolean inTriple(or a data class) that tracks user selection independently of ownership status, plus corresponding UI controls. If it’s intentionally “all owned DLCs (manifest+manual)”, the current code is fine but worth documenting so future readers don’t confuse ownership with user choice.Also applies to: 404-408
368-387: Avoid invokingSteamService.getAppInfoOffrom inside LazyColumn item rendering if it’s not guaranteed cheap.Each row does:
Text(text = (SteamService.getAppInfoOf(depotInfo.dlcAppId)?.name ?: "DLC ${depotInfo.dlcAppId}"))If
getAppInfoOftouches disk or IPC under the hood, this can cause jank during scroll or recomposition.If possible, consider:
- Passing the DLC name along with
DepotInfo(or pre‑resolving it once when buildingdlcList), and- Using that cached name in the
itemsblock instead of re‑querying SteamService per item.If
getAppInfoOfis known to be a fast in‑memory lookup, this is fine as‑is; just double‑check the implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-28T13:54:16.048Z
Learnt from: utkarshdalal
Repo: utkarshdalal/GameNative PR: 176
File: app/src/main/java/app/gamenative/utils/SteamUtils.kt:651-655
Timestamp: 2025-09-28T13:54:16.048Z
Learning: In the GameNative project, when creating depots.txt files in SteamUtils.ensureSteamSettings, only create the file when SteamService.getInstalledDepotsOf() returns actual data (non-null). Don't create empty files or write empty strings when no data is available.
Applied to files:
app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt
📚 Learning: 2025-09-19T17:07:27.941Z
Learnt from: utkarshdalal
Repo: utkarshdalal/GameNative PR: 159
File: app/src/main/java/app/gamenative/service/DownloadService.kt:6-6
Timestamp: 2025-09-19T17:07:27.941Z
Learning: In app/src/main/java/app/gamenative/service/DownloadService.kt, the getSizeFromStoreDisplay() function uses runBlocking to call SteamService.getDownloadableDepots() because the latter was converted from synchronous to suspend in this PR. The user confirmed the previous synchronous implementation worked fine, and if performance is acceptable, keeping runBlocking is a valid approach.
Applied to files:
app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt
🪛 detekt (1.23.8)
app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt
[warning] 81-81: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 131-131: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 167-167: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 402-402: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 417-417: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
app/src/main/java/app/gamenative/ui/screen/library/components/DlcManagerDialog.kt (2)
75-104: Initial load flow and depot dedup look solid.Using
LaunchedEffect(appId, visible)with a suspend‑friendlygetDownloadableDepotscall, deduping per DLC app ID and preferring 64‑bit depots, and seedingcheckedMapfrominstalledDepotsis coherent and matches the waygetDownloadableDepotswas intended to be used in prior PRs. No changes needed here.
Based on learnings, this aligns with the suspend API usage pattern forSteamService.getDownloadableDepots.
220-274: Diagnostics panel is very helpful; no functional issues spotted.The diagnostics toggle and the way you snapshot
getAppDlc,getDownloadableDepotsDebug, PICS ownership, and installed depots into a scrollable area is a nice touch for debugging DLC behavior. The guarded try/catch blocks keep this non‑critical path from breaking the dialog. No changes needed.
1 participant
Woo!
Summary by CodeRabbit
New Features
Bug Fixes / Tweaks
✏️ Tip: You can customize this high-level summary in your review settings.