feat: downloads storage manager tab#878
Merged
utkarshdalal merged 41 commits intoutkarshdalal:masterfrom Mar 28, 2026
Merged
Conversation
* added a back button to screen * changed game text color to white for easier readability
…orage-manager-tab
…orage-manager-tab
…ab' into feat/downloads-storage-manager-tab
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Room v14 schema with a new partial_install column; propagates partial‑install tracking across entities, DAOs, managers, and services; introduces a Downloads UI + DownloadsViewModel that aggregates active/partial downloads; and adds a Container Storage Manager with UI, filesystem move utilities, and supporting tests and strings. Changes
Sequence Diagram(s)sequenceDiagram
participant VM as DownloadsViewModel
participant SS as SteamService
participant ES as EpicService
participant GS as GOGService
participant AS as AmazonService
participant DAO as DAO Layer
participant UI as UI State
VM->>VM: startPolling() every 1s
loop Poll cycle
VM->>SS: getActiveDownloads()
SS->>DAO: read active download jobs
DAO-->>SS: active jobs
SS-->>VM: Map downloads
VM->>SS: getPartialDownloads()
SS->>DAO: query partial_install rows
DAO-->>SS: partial IDs
SS-->>VM: List partial IDs
par Other platforms
VM->>ES: getActiveDownloads()/getPartialDownloads()
ES-->>VM: data
VM->>GS: getActiveDownloads()/getPartialDownloads()
GS-->>VM: data
VM->>AS: getActiveDownloads()/getPartialDownloads()
AS-->>VM: data
end
VM->>DAO: fetch metadata (names, icons)
DAO-->>VM: metadata
VM->>UI: updateState(DownloadsState)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
16 issues found across 33 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/gog/GOGManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/GOGManager.kt:500">
P2: deleteGame() skips resetting the DB when an install was detected without markers, leaving isInstalled/installPath stale after deletion.</violation>
</file>
<file name="app/src/main/res/values/strings.xml">
<violation number="1" location="app/src/main/res/values/strings.xml:823">
P3: `container_storage_summary` uses a fixed "items" string with a count, which will render incorrect singular text (e.g., "1 items") and is not plural-aware for localization. Use an Android `<plurals>` resource and plural-aware formatting instead.</violation>
</file>
<file name="app/src/main/java/app/gamenative/db/dao/SteamAppDao.kt">
<violation number="1" location="app/src/main/java/app/gamenative/db/dao/SteamAppDao.kt:40">
P2: Owned-app SQL filtering rules are duplicated across two DAO methods, increasing risk of future rule drift and inconsistent results.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt:385">
P2: A non-critical `partialInstall` DB update is treated as fatal and can abort the whole download on DB/DAO exceptions.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt:385">
P2: Partial-install tracking is only applied to Gen 2 downloads; Gen 1 returns before this new flag update, causing inconsistent incomplete-install detection for legacy GOG titles.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/SteamService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamService.kt:554">
P2: `getPartialDownloads()` only reads DAO rows and returns empty if the service instance is null, so the downloads tab can miss resumable partials that `hasPartialDownload()` would detect from disk (e.g., after logout clears DAO rows or before the service is started).</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:121">
P2: Storage-manager loading performs unbounded recursive size scans for every entry, causing poor scalability and long blocking loads for large game libraries.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/utils/ContainerStorageManager.kt:849">
P2: Active container symlink is not relinked after deleting its target because `exists()` short-circuits for dangling symlinks.</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:108">
P2: Defaulting `onDownloadsClick` to `{}` masks missing wiring and already causes at least one call path to have a non-functional Downloads action.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt:87">
P2: `partialInstall` is set early but not reliably cleared on failure/finalization paths, allowing stale partial-install state in DB.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/data/DownloadsState.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/data/DownloadsState.kt:26">
P2: Downloads state uses `appId`-only map keys even though downloads are multi-source; IDs can collide across sources and overwrite entries.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/amazon/AmazonService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/amazon/AmazonService.kt:364">
P2: Amazon partial-download listing is tied to a running service and DB `partial_install` query, which can miss on-disk/resumable partial downloads and under-report items in the downloads tab.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/library/components/SystemMenu.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/library/components/SystemMenu.kt:572">
P2: A single `firstItemFocusRequester` is assigned to both Downloads and Settings, which breaks the single-target initial-focus intent and can make menu-open focus behavior ambiguous.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/amazon/AmazonDownloadManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/amazon/AmazonDownloadManager.kt:58">
P2: `partial_install` is set before validation/download and is not reverted on failure/cancellation, causing stale phantom partial-download state in DB.</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:127">
P2: `refresh()` marks the inventory as loaded even when `loadEntries` fails, so `ensureLoaded()` will never retry. A transient failure (or later inventory change while the state is remembered) can leave the dialog permanently stale with no recovery path.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/ui/screen/settings/ContainerStorageManagerDialog.kt:225">
P2: If moveGame throws, movingEntryName is never cleared, leaving the UI stuck in the moving state and blocking dialog dismissal.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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
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/components/SystemMenu.kt (1)
565-583:⚠️ Potential issue | 🟡 MinorDuplicate
FocusRequesterassignment on two menu items.Both the Downloads item (line 572) and the Settings item (line 582) use
firstItemFocusRequester. Sharing a singleFocusRequesteracross multiple Composables can cause undefined focus behavior.Since Downloads is now the first menu item, only it should receive
firstItemFocusRequester. Remove it from Settings.🐛 Proposed fix
SystemMenuItem( text = stringResource(R.string.settings_text), icon = Icons.Default.Settings, onClick = { onNavigateRoute(PluviaScreen.Settings.route) onDismiss() }, - focusRequester = firstItemFocusRequester, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/library/components/SystemMenu.kt` around lines 565 - 583, The Settings SystemMenuItem is incorrectly sharing firstItemFocusRequester with the Downloads item; remove the focusRequester = firstItemFocusRequester parameter from the Settings SystemMenuItem so only the Downloads SystemMenuItem retains firstItemFocusRequester. Locate the two SystemMenuItem calls (the one with text = stringResource(R.string.app_downloads) and the one with text = stringResource(R.string.settings_text) that calls onNavigateRoute(PluviaScreen.Settings.route)) and delete the focusRequester assignment from the Settings item while leaving the Downloads item and its onDownloadsClick/onDismiss behavior unchanged.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/service/amazon/AmazonDownloadManager.kt (1)
57-59: Consider clearingpartialInstallon early failures.The
partialInstallflag is set before validation checks (credentials, entitlement, download spec). If these fail early (lines 66-82), the flag remains set even though no files were downloaded. This could cause the game to appear in the Downloads screen with no resumable state.For cancellation (line 185-190), keeping
partialInstall=truemakes sense for resume capability. However, for early validation failures where nothing was actually downloaded, you may want to clear the flag incleanupOnFailure().💡 Optional: Clear partialInstall on early failures
// Helper to cleanup marker on early failure fun cleanupOnFailure() { MarkerUtils.removeMarker(installPath, Marker.DOWNLOAD_IN_PROGRESS_MARKER) + amazonManager.clearPartialInstall(productId) }This would require adding a
clearPartialInstall()method toAmazonManager.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/amazon/AmazonDownloadManager.kt` around lines 57 - 59, The code sets partialInstall via amazonManager.markAsPartialInstall(productId) before validation steps and never clears it on early failures; add a clearPartialInstall() method to AmazonManager and call it from cleanupOnFailure() when no download has started (i.e., after credential/entitlement/downloadSpec validation failures) so the DB flag is cleared for non-resumable failures, but keep the existing behavior for user cancellations where partialInstall should remain true to allow resume.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/db/dao/GOGGameDao.kt`:
- Around line 49-50: The query in getPartialDownloads currently returns rows
with partial_install = 1 even if they are hidden (exclude = 1); update the Room
Query in the suspend function getPartialDownloads to filter out hidden titles by
adding exclude = 0 (e.g., "SELECT * FROM gog_games WHERE partial_install = 1 AND
exclude = 0") so only non-hidden partial installations are returned.
In `@app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt`:
- Around line 382-386: The partialInstall flag isn't being set for legacy Gen1
downloads because downloadGameGen1(...) can return before the shared
post-download block runs; update the Gen1 flow to also mark the DB record as
partial. Inside downloadGameGen1 (where gameId is known and
resumable/early-return logic exists) call gogManager.getGameFromDbById(gameId)
and, if non-null, invoke gogManager.updateGame(currentGame.copy(partialInstall =
true)) so legacy Gen1 installs are marked the same way as Gen2; ensure you don't
duplicate logic by consolidating the DB-update into a small helper if needed.
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 553-558: getPartialDownloads currently only excludes active
downloadJobs, which lets stale downloading_app_info rows for completed installs
show up; update getPartialDownloads to also filter out appIds that are already
fully installed by consulting the install status/installed-app DAO.
Specifically, in getPartialDownloads(), keep the call to
downloadingAppInfoDao.getAll() and downloadJobs.containsKey check but
additionally call the appropriate install-status method (e.g.,
instance?.installStatusDao?.isComplete(appId) or
instance?.installedAppDao?.getAllInstalled() / contains) and exclude any appId
that reports completed/installed so only true resumable downloads are returned.
In `@app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt`:
- Around line 130-137: The items map in DownloadsViewModel is using raw
app/product IDs as keys causing cross-store collisions; change keying to include
the store prefix (e.g., "steam:$appId", "epic:$productId", "gog:$productId",
"amazon:$productId") wherever DownloadItemState entries are inserted (the
SteamService loop, Epic/GOG/Amazon loops) and update any containsKey(...) checks
and lookups to use the same prefixed keys (search for usages around
getSteamMetadata, getEpicMetadata, getGogMetadata, getAmazonMetadata and the
mutableMap items).
In `@app/src/main/java/app/gamenative/ui/screen/HomeScreen.kt`:
- Around line 55-64: The Friends branch renders HomeLibraryScreen but doesn't
forward the downloads callback, so the System menu's Downloads action is a
no-op; update the HomeDestination.Friends branch to pass the onDownloadsClick
prop through to HomeLibraryScreen (i.e., include onDownloadsClick =
onDownloadsClick when invoking HomeLibraryScreen) so the Downloads action from
the Friends placeholder navigates correctly.
In `@app/src/main/java/app/gamenative/utils/ContainerStorageManager.kt`:
- Around line 355-399: The uninstall branches currently ignore the Boolean
return from removeContainer(), causing a success result even if container
deletion fails; update each branch (SteamService.deleteApp handling and the
GOG/Epic/Amazon branches that call removeContainer when entry.hasContainer) to
capture the Boolean from removeContainer(context, entry.containerId) and if it
returns false convert the overall Result to a failure (e.g.,
Result.failure(Exception("Failed to remove container"))) instead of returning
success; ensure the Steam branch also returns a failure when removeContainer
fails and propagate that failure as the method result.
- Around line 389-394: The Amazon uninstall branch in ContainerStorageManager
(GameSource.AMAZON) must not depend on a running AmazonService singleton to
resolve productId; change the lookup to query the persisted mapping via
StorageManagerDaoEntryPoint / AmazonGameDao (or add a fallback in
AmazonService.getProductIdByAppId to do the same) so uninstall works after
restart. Specifically, in the branch using AmazonService.getProductIdByAppId,
replace or augment that call with a DAO query through
StorageManagerDaoEntryPoint -> AmazonGameDao to obtain the productId (and only
then call AmazonService.deleteGame and removeContainer), or implement DAO
fallback logic inside AmazonService.getProductIdByAppId so
ContainerStorageManager can remain unchanged.
- Around line 260-304: After a successful StorageUtils.moveDirectory(...) the
per-store metadata updates (the GameSource when-block that calls
PluviaApp.events.emitJava and entryPoint.* DAO update/markAsInstalled) must be
wrapped in a try/catch so any exception there returns Result.failure and does
not leave inconsistent state; on catch attempt to roll back the filesystem move
by calling StorageUtils.moveDirectory(targetDir.absolutePath,
sourceDir.absolutePath) (or otherwise restore files) and restore metadata if you
already mutated any DAO, then return Result.failure(exception) — ensure you
reference the same entry.containerId/entry.gameInstallSizeBytes and use the
existing DAO methods (gogGameDao().update, epicGameDao().update,
amazonGameDao().markAsInstalled) when performing rollback or compensating
updates so callers can reliably unwind.
- Around line 846-863: The code currently checks whether activeLink points to
deletedContainerDir after the deletion, which can miss a match; inside
relinkActiveSymlinkIfNeeded, read and capture the active link target before any
deletion occurs (e.g., val activeTarget = runCatching { activeLink.canonicalFile
}.getOrNull() or use Files.readSymbolicLink on activeLink.toPath() if you need
raw symlink target), then compare activeTarget to
deletedContainerDir.canonicalFile to decide if relinking is needed; if they
match, proceed to delete the symlink and create the fallback symlink as
currently implemented, and keep the existing try/catch (runCatching/.onFailure)
behavior around the relinking logic.
In `@app/src/main/java/app/gamenative/utils/StorageUtils.kt`:
- Around line 118-119: The progress callback is receiving the post-incremented
value so the moved-file count is one behind; update the code that calls
onProgressUpdate to pass the incremented count (either increment filesMoved
before calling or use ++filesMoved / filesMoved + 1) instead of filesMoved++ so
the first completed file reports 1; apply the same fix to the other
onProgressUpdate call site that also uses filesMoved.
- Around line 76-85: In StorageUtils.kt, before proceeding with moves use the
resolved canonical paths (sourceRootPath.toRealPath(LinkOption.NOFOLLOW_LINKS)
and targetRootPath.toRealPath(LinkOption.NOFOLLOW_LINKS) or fallback to
normalize()) to guard against path overlap: if the paths are equal or one
startsWith the other (sourceReal.startsWith(targetReal) ||
targetReal.startsWith(sourceReal)), return
Result.failure(IllegalArgumentException("Source and target directories
overlap")) so you avoid destructive behavior during move/copy fallback; include
an IOException catch around toRealPath and treat resolution errors as failures
with a clear message.
- Around line 221-229: The code calls moveDirectory(...) and ignores its
Result<Unit>, causing moveGamesFromOldPath to always call onComplete() even on
failure; capture the returned Result from moveDirectory, check result.isSuccess
/ result.isFailure, and on success invoke onComplete() on the Main dispatcher,
while on failure invoke an error handler (e.g., call an onError/onFailure
callback if available or propagate the Result/throw to the caller) passing the
failure.exceptionOrNull() for proper UI handling; update moveGamesFromOldPath
signature or callers (e.g., the caller in SteamAppScreen.kt) as needed so
failures are surfaced instead of silently closing the dialog.
---
Outside diff comments:
In `@app/src/main/java/app/gamenative/ui/screen/library/components/SystemMenu.kt`:
- Around line 565-583: The Settings SystemMenuItem is incorrectly sharing
firstItemFocusRequester with the Downloads item; remove the focusRequester =
firstItemFocusRequester parameter from the Settings SystemMenuItem so only the
Downloads SystemMenuItem retains firstItemFocusRequester. Locate the two
SystemMenuItem calls (the one with text = stringResource(R.string.app_downloads)
and the one with text = stringResource(R.string.settings_text) that calls
onNavigateRoute(PluviaScreen.Settings.route)) and delete the focusRequester
assignment from the Settings item while leaving the Downloads item and its
onDownloadsClick/onDismiss behavior unchanged.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/amazon/AmazonDownloadManager.kt`:
- Around line 57-59: The code sets partialInstall via
amazonManager.markAsPartialInstall(productId) before validation steps and never
clears it on early failures; add a clearPartialInstall() method to AmazonManager
and call it from cleanupOnFailure() when no download has started (i.e., after
credential/entitlement/downloadSpec validation failures) so the DB flag is
cleared for non-resumable failures, but keep the existing behavior for user
cancellations where partialInstall should remain true to allow resume.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46024a26-83c7-4a11-b5d2-2a31623f82b7
📒 Files selected for processing (33)
app/schemas/app.gamenative.db.PluviaDatabase/14.jsonapp/src/main/java/app/gamenative/data/AmazonGame.ktapp/src/main/java/app/gamenative/data/EpicGame.ktapp/src/main/java/app/gamenative/data/GOGGame.ktapp/src/main/java/app/gamenative/db/PluviaDatabase.ktapp/src/main/java/app/gamenative/db/dao/AmazonGameDao.ktapp/src/main/java/app/gamenative/db/dao/DownloadingAppInfoDao.ktapp/src/main/java/app/gamenative/db/dao/EpicGameDao.ktapp/src/main/java/app/gamenative/db/dao/GOGGameDao.ktapp/src/main/java/app/gamenative/db/dao/SteamAppDao.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/service/amazon/AmazonConstants.ktapp/src/main/java/app/gamenative/service/amazon/AmazonDownloadManager.ktapp/src/main/java/app/gamenative/service/amazon/AmazonManager.ktapp/src/main/java/app/gamenative/service/amazon/AmazonService.ktapp/src/main/java/app/gamenative/service/epic/EpicDownloadManager.ktapp/src/main/java/app/gamenative/service/epic/EpicManager.ktapp/src/main/java/app/gamenative/service/epic/EpicService.ktapp/src/main/java/app/gamenative/service/gog/GOGDownloadManager.ktapp/src/main/java/app/gamenative/service/gog/GOGManager.ktapp/src/main/java/app/gamenative/service/gog/GOGService.ktapp/src/main/java/app/gamenative/ui/data/DownloadsState.ktapp/src/main/java/app/gamenative/ui/model/DownloadsViewModel.ktapp/src/main/java/app/gamenative/ui/screen/HomeScreen.ktapp/src/main/java/app/gamenative/ui/screen/downloads/DownloadsScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/components/LibraryTabBar.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/utils/ContainerStorageManager.ktapp/src/main/java/app/gamenative/utils/StorageUtils.ktapp/src/main/res/values/strings.xmlapp/src/test/java/app/gamenative/utils/ContainerStorageManagerTest.kt
* potential race condition fixed regarding pollDownloads * improved game metadata caching for downloads view * refined getPartialDownloads query for GOG
Fixed portrait mode issue with download view
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt (1)
135-154:⚠️ Potential issue | 🟠 MajorCross-store key collisions can overwrite downloads in
items.The
itemsmap is keyed by rawappId/productIdstrings. If different stores happen to share the same numeric ID (e.g., Steam123and Epic123), one entry will overwrite the other.The UI correctly uses composite keys at line 387 (
"${it.gameSource}_${it.appId}"), but the ViewModel'sitemsmap does not, causing data loss before reaching the UI.Proposed fix: use composite keys in the items map
private suspend fun pollDownloads() { if (!pollMutex.tryLock()) return try { val items = mutableMapOf<String, DownloadItemState>() + fun downloadKey(source: GameSource, id: String) = "${source.name}_$id" // Steam downloads for ((appId, info) in SteamService.getActiveDownloads()) { val (name, icon) = getSteamMetadata(appId) val (downloaded, total) = info.getBytesProgress() - items[appId.toString()] = DownloadItemState( + items[downloadKey(GameSource.STEAM, appId.toString())] = DownloadItemState(Apply the same pattern for all stores (Steam partials, Epic, GOG, Amazon) and update
containsKeychecks accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt` around lines 135 - 154, The items map in DownloadsViewModel is keyed by raw IDs which allows collisions across stores; change the key creation for Steam entries (and similarly for Steam partials, Epic, GOG, Amazon entries) to use a composite key like "${GameSource.STEAM}_${appId}" (mirror the UI pattern "${it.gameSource}_${it.appId}") when inserting DownloadItemState into items and when checking containsKey; update references around SteamService.getActiveDownloads(), DownloadItemState creation, and any containsKey checks to use the composite key so entries from different GameSource values cannot overwrite each other.
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt (1)
293-311: Consider handling cancel/pause failures.The
cancelDownload()methods returnBooleanindicating success (per context snippets 2-4), but return values are ignored. If a pause fails silently, the UI may show stale state until the next poll.Optional: Log failures for debugging
GameSource.EPIC -> { val id = appId.toIntOrNull() ?: return - EpicService.cancelDownload(id) + if (!EpicService.cancelDownload(id)) { + Timber.w("Failed to pause Epic download: $id") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt` around lines 293 - 311, The onPauseDownload method currently ignores return values from cancel/pause calls; update it to check the Boolean results from SteamService.getAppDownloadInfo(id)?.cancel(), EpicService.cancelDownload(id), GOGService.cancelDownload(appId), and AmazonService.cancelDownload(appId) and handle failures by reverting any optimistic UI state and logging the failure (or scheduling a retry/update); specifically, capture the result of each cancel call, call processLogger or an injected logger with contextual info when false, and ensure the ViewModel triggers a state refresh (or emits an error state) when cancellation fails so the UI doesn't show stale download status.app/src/main/java/app/gamenative/ui/screen/downloads/DownloadsScreen.kt (1)
110-110: Replacevalues()withentriesfor Kotlin 1.9+ compatibility.
DownloadsSection.values()is deprecated in favor ofentriessince Kotlin 1.9. Additionally, sinceentriesalready returns an immutable list, the.toList()call is unnecessary and allocates memory on each access.Proposed fix
- val sections = remember { DownloadsSection.values().toList() } + val sections = remember { DownloadsSection.entries }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/downloads/DownloadsScreen.kt` at line 110, Replace the deprecated call DownloadsSection.values() with DownloadsSection.entries and remove the unnecessary .toList() allocation; update the remember block that defines val sections (currently using DownloadsSection.values().toList()) to use DownloadsSection.entries directly so it uses the Kotlin 1.9+ API and avoids extra memory allocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt`:
- Around line 135-154: The items map in DownloadsViewModel is keyed by raw IDs
which allows collisions across stores; change the key creation for Steam entries
(and similarly for Steam partials, Epic, GOG, Amazon entries) to use a composite
key like "${GameSource.STEAM}_${appId}" (mirror the UI pattern
"${it.gameSource}_${it.appId}") when inserting DownloadItemState into items and
when checking containsKey; update references around
SteamService.getActiveDownloads(), DownloadItemState creation, and any
containsKey checks to use the composite key so entries from different GameSource
values cannot overwrite each other.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt`:
- Around line 293-311: The onPauseDownload method currently ignores return
values from cancel/pause calls; update it to check the Boolean results from
SteamService.getAppDownloadInfo(id)?.cancel(), EpicService.cancelDownload(id),
GOGService.cancelDownload(appId), and AmazonService.cancelDownload(appId) and
handle failures by reverting any optimistic UI state and logging the failure (or
scheduling a retry/update); specifically, capture the result of each cancel
call, call processLogger or an injected logger with contextual info when false,
and ensure the ViewModel triggers a state refresh (or emits an error state) when
cancellation fails so the UI doesn't show stale download status.
In `@app/src/main/java/app/gamenative/ui/screen/downloads/DownloadsScreen.kt`:
- Line 110: Replace the deprecated call DownloadsSection.values() with
DownloadsSection.entries and remove the unnecessary .toList() allocation; update
the remember block that defines val sections (currently using
DownloadsSection.values().toList()) to use DownloadsSection.entries directly so
it uses the Kotlin 1.9+ API and avoids extra memory allocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: efb7e8a5-dd00-466d-9cd8-bc67831bec38
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/db/dao/GOGGameDao.ktapp/src/main/java/app/gamenative/ui/model/DownloadsViewModel.ktapp/src/main/java/app/gamenative/ui/screen/downloads/DownloadsScreen.kt
Contributor
There was a problem hiding this comment.
1 issue found across 13 files (changes from recent commits).
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/data/DownloadInfo.kt">
<violation number="1" location="app/src/main/java/app/gamenative/data/DownloadInfo.kt:203">
P2: Current download speed can report stale positive EMA during stalls because no-progress branches fall back to historical EMA instead of zero/null.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Owner
There was a problem hiding this comment.
translations will be needed
Owner
There was a problem hiding this comment.
what was changed here? looks like just renaming?
Contributor
Author
There was a problem hiding this comment.
Nah it's extracting the old migration helper to a new moveDirectory since storage manager needs to move game directories to and from external and internal, also needed to add a return result and validate source dir for storage manager to report back properly or error handle
Comment on lines
+57
to
+66
| HomeDestination.Friends -> { | ||
| // TODO: Friends screen | ||
| HomeLibraryScreen( | ||
| onClickPlay = onClickPlay, | ||
| onTestGraphics = onTestGraphics, | ||
| onNavigateRoute = onNavigateRoute, | ||
| onLogout = onLogout, | ||
| onGoOnline = onGoOnline, | ||
| isOffline = isOffline, | ||
| ) |
Owner
There was a problem hiding this comment.
we don't have a friends screen and aren't going to add one.
Contributor
Author
There was a problem hiding this comment.
Another rabbit recommendation, there were other todos in the code for friends so it wanted that here, will rm all
Comment on lines
+52
to
+56
| HomeDestination.Downloads -> HomeDownloadsScreen( | ||
| onBack = { viewModel.onDestination(HomeDestination.Library) }, | ||
| onClickPlay = onClickPlay, | ||
| onTestGraphics = onTestGraphics, | ||
| ) |
Owner
There was a problem hiding this comment.
not sure downloads should be a screen vs just a popup? what was the thinking behind making it a screen?
Contributor
Author
There was a problem hiding this comment.
Because it's one of two tabs across downloads and storage manager now, and with nav to game page on icon click it makes more sense as a stateful page imo. There's enough going on here to make it not just a modal
Contributor
There was a problem hiding this comment.
5 issues found across 21 files (changes from recent commits).
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/amazon/AmazonService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/amazon/AmazonService.kt:366">
P2: getPartialDownloads now performs per-game DB + filesystem checks for every non-installed game on each poll, which can cause significant IO load and potential jank for large libraries. Consider batching/caching or moving the heavy scan off the frequent polling path.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/gog/GOGService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/GOGService.kt:208">
P2: hasPartialDownload now performs a blocking DB lookup via runBlocking when called from UI composables, which can block the main thread and cause frame drops/ANR.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/gog/GOGService.kt:217">
P2: getPartialDownloads now performs a full non-installed library scan with per-game filesystem checks on each poll. Since DownloadsViewModel polls every second, this introduces repeated O(N) disk I/O for large libraries, likely degrading responsiveness and battery. Consider caching or using the previous DB-backed partial download list instead of scanning the filesystem each poll.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/epic/EpicService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/epic/EpicService.kt:186">
P2: getPartialDownloads now performs a blocking DB lookup per game via hasPartialDownload -> getEpicGameOf(runBlocking). This introduces an N+1 pattern and can cause latency/jank for large libraries.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/epic/EpicService.kt:189">
P2: Partial download detection always checks the default install path, so resumable downloads in custom install locations are missed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
xXJSONDeruloXx
force-pushed
the
feat/downloads-storage-manager-tab
branch
from
95b628e to
1525e13
Compare
Contributor
Author
|
@utkarshdalal addressed all comments |
Owner
|
The infinite poll in the downloads manager is an issue. We should be doing event-driven architecture like we do for the downloads on the *AppScreens. xXJSONDeruloXx#15 <- something like this (not tested, just asked AI to spin something up) Need to look at how the Downloads Manager looks, the UI seems a bit inconsistent with the rest of the app |
Owner
|
If I understand correctly, ContainerStorageManager is going through walk through every file in every folder to sum up the size of each container? Especially on external storage, this is pretty problematic, it is going to take really long to calculate. |
Owner
|
getPartialDownloads for Epic/Amazon/GOG also seems problematic if the user has large libraries. It's going to check for the marker for every non-installed game, again especially slow on external storage. I understand that we might be limited by how the current installs are done and tracked. Let's discuss if so. |
Contributor
There was a problem hiding this comment.
3 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/ui/screen/settings/ContainerStorageManagerDialog.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/settings/ContainerStorageManagerDialog.kt:247">
P2: Catching Exception here also catches CancellationException and prevents coroutine cancellation from propagating; this can show failure snackbars and run UI logic after the scope is cancelled. Rethrow CancellationException before converting to Result.failure.</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:622">
P2: Game install size is now only read from installSizeBytes, but Steam installs never set that field, so size becomes null (regression vs previous getPathSize fallback) and Storage Manager size display/sorting will be incorrect for Steam and any games without persisted size.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt:440">
P2: Refresh requests are dropped when a refresh is already running; with polling removed this can leave download state stale until another event fires.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
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/utils/ContainerStorageManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/ContainerStorageManager.kt:815">
P2: New install-size fallback can recursively scan every internal Steam install directory during entry loading, causing significant load-time and I/O scalability regression for large libraries.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt:447">
P2: Refresh coalescing has a lost-wakeup race: a pending refresh can be set after loop exit but before mutex unlock, then never processed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/model/DownloadsViewModel.kt:78">
P2: Initial downloads refresh can be lost due to `SharedFlow` timing, causing missing/empty initial state until another event triggers refresh.</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:789">
P2: Steam install-size estimation sums all depot manifests without depot eligibility filtering, causing inaccurate storage usage and ranking.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/utils/ContainerStorageManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/ContainerStorageManager.kt:790">
P2: Per-app depot service lookup in size estimation introduces avoidable O(N) heavy work during storage scan and can slow large Steam library loads.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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/utils/ContainerStorageManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/ContainerStorageManager.kt:796">
P2: Steam size estimation now filters depots with `ownedDlc = emptyMap()`, which causes DLC depots to be excluded unconditionally and can undercount install size.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
6 tasks
8 tasks
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
3 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 by cubic
Adds a unified Downloads & Storage tab to track and control downloads across Steam, Epic, GOG, and Amazon, plus a Storage Manager to review space and move installs. Improves reliability of partial-download detection and Steam size estimates for steadier progress and storage data.
New Features
partial_installcolumns (Epic/GOG/Amazon) and Steam viadownloading_app_info; services exposegetActiveDownloads/getPartialDownloads; resumes across restarts.StorageUtils.moveDirectory; Back returns to Library.Bug Fixes
MarkerUtils.hasPartialInstalland resumable detection; avoided full-library scans using targeted roots and DAO-backed non-installed queries.Written for commit f5e607e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests