GOG Kotlin Migration - Game Management and Downloads #418
Conversation
…ing auth information and doing REST API requests correctly.
… into gog-kotlin-migration-initial
… how co-routines work with batch-api requests.
…updated dependencies.
…hobos665/GameNative into gog-kotlin-migration-game-manager
…hobos665/GameNative into gog-kotlin-migration-game-manager
…parser. Also added an exclude property so we don't always iterate over ignored items.
Also added directory for adding the commonredist path to the game.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt:70">
P2: This pattern may throw `JSONException` if the JSON contains an explicit null value (e.g., `"branch": null`). `has()` returns true for keys with null values, but `getString()` throws on null. Consider using `json.isNull()` check:
```kotlin
branch = if (json.has("branch") && !json.isNull("branch")) json.getString("branch") else null
```</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt:72">
P2: Same issue: `getString()` will throw if `legacy_build_id` is explicitly null in JSON. Add `isNull()` check.</violation>
<violation number="3" location="app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt:348">
P2: Same issue: `getString()` will throw if `md5` is explicitly null in JSON. Add `isNull()` check.</violation>
<violation number="4" location="app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt:349">
P2: Same issue: `getString()` will throw if `sha256` is explicitly null in JSON. Add `isNull()` check.</violation>
<violation number="5" location="app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt:351">
P2: Same issue: `getString()` will throw if `productId` is explicitly null in JSON. Add `isNull()` check.</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:34">
P3: Typo in documentation: "GOGDataMdoels" should be "GOGDataModels".</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/java/app/gamenative/service/gog/GOGService.kt (1)
146-148: Preserve installed-game metadata on logout.
deleteAllGames()erases all game metadata from the database. While GOG has re-detection logic viadetectAndUpdateExistingInstallations(), it only runs during library refresh, not on logout. Users won't see their installed games until the next library sync, creating a poor UX and inconsistency with SteamService (which explicitly preserves AppInfo during logout). Keep game installation state and only clear auth/user-specific data like credentials and download metadata.app/src/main/java/app/gamenative/service/gog/GOGManager.kt (1)
521-545:runBlockingblocks the calling thread and risks ANR when invoked from main thread.
isGameInstalledis called fromLaunchedEffectinLibraryAppItem.kt:317,329(which runs on the main thread) and usesrunBlockingtwice internally for DB operations (lines 533, 537). This pattern causes thread blocking on the main thread and can trigger ANR. Consider making this asuspend funto integrate properly with coroutines, or if synchronous behavior is required for API compatibility, ensure it's explicitly called only from background threads/coroutines via explicit thread dispatchers.
🤖 Fix all issues with AI agents
In `@app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt`:
- Around line 65-73: The JSON parsing for optional fields in GOGDataModels
(e.g., the branch and legacyBuildId assignments) currently uses json.has() and
json.getString(), which can yield the literal "null" when the API returns
JSONObject.NULL; change these to null-safe checks using
json.isNull("branch")/json.isNull("legacy_build_id") or use
json.optString("branch", null) / json.optString("legacy_build_id", null) so that
branch and legacyBuildId become actual nulls instead of the string "null"; apply
the same null-safe fix to the other occurrence noted (around lines 345-351)
where optional JSON strings are handled.
In `@app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt`:
- Around line 470-471: The log call in GOGDownloadManager that prints the full
CDN URL (e.g., Timber.tag("GOG").d("Chunk $chunkMd5 URL: $url")) can leak
time‑limited tokens; update these log sites (including the similar block around
lines 872-882) to avoid printing the full URL—either log only non‑sensitive
parts (hostname and path without query string) or just log chunkMd5 and a
redacted URL placeholder; make the change where downloadChunkWithRetry is
invoked and any other usages in GOGDownloadManager so logs never contain URL
query parameters or tokens.
- Around line 1045-1048: The inflate loop in GOGDownloadManager (the loop
calling inflater.inflate(buffer) and writing to outputStream) can spin forever
when inflate() returns 0 for malformed or incomplete data; update the loop to
detect when count == 0 and then check inflater.needsInput() and
inflater.needsDictionary() and throw a clear IOException (or otherwise abort)
instead of looping endlessly, so replace the current unguarded while
(!inflater.finished()) { val count = inflater.inflate(buffer) ... } with logic
that breaks/wraps up on zero by checking needsInput()/needsDictionary() and
handling those cases explicitly.
♻️ Duplicate comments (6)
app/src/main/java/app/gamenative/service/gog/api/GOGManifestParser.kt (1)
268-272: Remove hardcoded “problematic chunk” logging.This is leftover debug logic for a specific hash and should be removed before merge.
🧹 Proposed cleanup
- // Debug logging for the problematic chunk - if (chunkMd5 == "50501137663066eeeaa987b0ac228bc2") { - Timber.tag(TAG).w("Built URL for problematic chunk: productId=$productId, baseCdnUrl=$baseCdnUrl, finalUrl=$chunkUrl") - }app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt (1)
357-359: Replace the unprofessional TODO comment.Please keep comments professional and action‑oriented.
🧹 Suggested replacement
- // TODO: Use the dependencies, and download them etc. - // What the fuck are supportFiles??????? + // TODO: Confirm supportFiles purpose (redistributables) and wire into dependency download/install.app/src/main/java/app/gamenative/service/gog/GOGApiClient.kt (2)
150-152: Handle numeric IDs in theownedarray safely.The API commonly returns integers;
getString()can throw. Convert viaget(it).toString()instead.🔧 Safer conversion
- val gameIds = List(ownedGames.length()) { - ownedGames.getString(it) - } + val gameIds = List(ownedGames.length()) { + ownedGames.get(it).toString() + }
460-461: Avoid defaulting missinggame_typeto"dlc".If the field is absent, this marks normal games as DLC. Default to
"game"or empty and only treat"dlc"as DLC.🔧 Suggested fix
- val gameType = rawResponse.optString("game_type", "dlc") + val gameType = rawResponse.optString("game_type", "game")app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt (1)
356-359: Support-file detection should use__supportpath.Tests identify support files by path, not flags; current logic misclassifies them.
🔧 Proposed fix
- fun isSupportFile(): Boolean = flags.contains("support") + fun isSupportFile(): Boolean = path.contains("__support")app/src/main/java/app/gamenative/service/gog/GOGManager.kt (1)
279-281: Confirm owned DLC exclusion is intentional.The
excludecondition filters all items whereisDlc == true, regardless of ownership status. This hides DLC from the library view, which may be intentional if DLC is downloaded alongside base games. Please confirm this is the expected behavior.
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/service/gog/GOGManager.kt (2)
203-207: Extract magic game ID to a named constant.The hardcoded
"1801418160"is a magic value. Consider extracting it toGOGConstantswith a descriptive name likeGOG_GALAXY_HIDDEN_IDto improve readability and maintainability.♻️ Suggested refactor
In
GOGConstants:const val GOG_GALAXY_HIDDEN_ID = "1801418160" // Hidden ID for GOG Galaxy, should be ignoredThen in
GOGManager:- val ignoredGameId = "1801418160" // Hidden ID for GOG Galaxy that we should ignore. - // Get existing game IDs from database to avoid re-fetching val existingGameIds = gogGameDao.getAllGameIdsIncludingExcluded().toMutableSet() - existingGameIds.add(ignoredGameId) + existingGameIds.add(GOGConstants.GOG_GALAXY_HIDDEN_ID)
1072-1078: Disk I/O on caller's thread may cause jank.
setCloudSaveSyncTimestampcallssaveCloudSaveTimestampsToDisk()synchronously. If invoked from the main thread, this file write could cause UI jank. Consider launching the save in a coroutine or making this a suspend function.♻️ Suggested fix - async disk write
+ private val scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + fun setCloudSaveSyncTimestamp(appId: String, locationName: String, timestamp: String) { val key = "${appId}_$locationName" syncTimestamps[key] = timestamp Timber.d("Stored sync timestamp for $key: $timestamp") - // Persist to disk - saveCloudSaveTimestampsToDisk() + // Persist to disk asynchronously + scope.launch { saveCloudSaveTimestampsToDisk() } }Note: Ensure the scope is properly cancelled when GOGManager is destroyed if needed.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/main/java/app/gamenative/service/gog/GOGApiClient.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/service/gog/api/GOGDataModels.ktapp/src/main/java/app/gamenative/service/gog/api/GOGManifestParser.kt
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: unbelievableflavour
Repo: utkarshdalal/GameNative PR: 157
File: app/src/main/java/app/gamenative/service/GameManager.kt:60-63
Timestamp: 2025-09-18T12:38:38.471Z
Learning: User prefers to keep PR scope small and focused, deferring non-critical refactoring suggestions to future PRs when the functionality is more fully implemented.
📚 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/gog/GOGService.ktapp/src/main/java/app/gamenative/service/gog/GOGDownloadManager.ktapp/src/main/java/app/gamenative/service/gog/GOGManager.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/gog/GOGService.ktapp/src/main/java/app/gamenative/service/gog/GOGManager.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/gog/api/GOGDataModels.kt
📚 Learning: 2025-12-17T05:14:05.133Z
Learnt from: utkarshdalal
Repo: utkarshdalal/GameNative PR: 344
File: app/src/main/java/app/gamenative/service/SteamAutoCloud.kt:415-419
Timestamp: 2025-12-17T05:14:05.133Z
Learning: In SteamAutoCloud.kt, when uploading files to Steam Cloud API (beginFileUpload and commitFileUpload), the filename parameter intentionally uses different formats: `file.path + file.filename` (relative path without placeholder) when `appInfo.ufs.saveFilePatterns.isEmpty()` is true (fallback case), and `file.prefixPath` (includes placeholder like %SteamUserData%) when patterns are configured. This difference is by design.
Applied to files:
app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt
🧬 Code graph analysis (1)
app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt (2)
app/src/main/java/app/gamenative/utils/FileUtils.kt (1)
calculateDirectorySize(26-45)app/src/main/java/app/gamenative/service/gog/api/GOGManifestParser.kt (1)
buildChunkUrlMap(202-222)
⏰ 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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: build
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<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:1059">
P2: Potential infinite loop when `inflate()` returns 0 but neither `needsInput()` nor `needsDictionary()` is true. If malformed zlib data causes the inflater to never reach `finished()` state while continuously returning 0 bytes, this loop will spin forever. Consider adding a consecutive zero-count iteration limit to detect and break out of stuck states.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| "Zlib data requires a preset dictionary which is not supported" | ||
| ) | ||
| } | ||
| // If neither condition is true, inflater is still processing internally |
There was a problem hiding this comment.
P2: Potential infinite loop when inflate() returns 0 but neither needsInput() nor needsDictionary() is true. If malformed zlib data causes the inflater to never reach finished() state while continuously returning 0 bytes, this loop will spin forever. Consider adding a consecutive zero-count iteration limit to detect and break out of stuck states.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt, line 1059:
<comment>Potential infinite loop when `inflate()` returns 0 but neither `needsInput()` nor `needsDictionary()` is true. If malformed zlib data causes the inflater to never reach `finished()` state while continuously returning 0 bytes, this loop will spin forever. Consider adding a consecutive zero-count iteration limit to detect and break out of stuck states.</comment>
<file context>
@@ -1043,7 +1043,22 @@ class GOGDownloadManager @Inject constructor(
+ "Zlib data requires a preset dictionary which is not supported"
+ )
+ }
+ // If neither condition is true, inflater is still processing internally
+ // Continue loop, but this should be rare
+ }
</file context>
There was a problem hiding this comment.
This is where the proper meat of the refactor comes in.
I will breakdown the download logic as such:
- Get full game Details from DB (as well as IDs for DLC we own for depot filtering)
- Fetch the builds for a game
- Get the builds from build URL (JSON)
- Pull the appropriate buildId & manifest link from the JSON response
- Get the Manifest payload, decompress and parse the JSON
- Grab the list of dependencies (which are a string array, and will match nicely to our XServer's installDep as I've made it install to the correct folder, which is configurable).
- Grab all depots for products that we own (This will include DLC, so I have it filtering out what we don't own). filter by lanuage & 32/64, but generally most don't specify this
- Create the chunkURL map and download the chunks based on BASEURL/{FIRST_2_CHARS}/{NEXT_2_CHARS}/$chunkMD5) (Do that until there are no more chars left or it's just a MD5 left)
- Download the chunks, combine together to make the files
- Repeat steps 9 and 10 for the DLC
- Similarly (but with some extra steps), do the same with dependencies if any were specified
…or getting the gameIds so we don't cause a potential error with comparisons.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt`:
- Around line 378-381: The parsing logic for compressedSize in GOGDataModels.kt
currently treats both a missing field and a present zero value the same by using
json.optLong("compressedSize", 0) and converting 0 to null; change it to first
check json.has("compressedSize") and only set compressedSize to null when the
field is absent, otherwise use the actual long value (including 0) from
json.optLong("compressedSize") so that a legitimate 0 compressedSize is
preserved; update the assignment that references compressedSize in the relevant
data class/ factory/parser to use json.has("compressedSize") and optLong
accordingly.
In `@app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt`:
- Around line 113-115: The event emission in GOGDownloadManager is converting
gameId with gameId.toIntOrNull() ?: 0 which can map invalid or huge string IDs
to 0 and misroute events; update the emission to preserve/validate the original
identifier instead: either emit the DownloadStatusChanged with a string ID
variant (pass gameId) or parse and validate before emitting (e.g., convert with
toIntOrNull and if null emit a separate error/unknown-id value or include both
numeric and original string), modifying the call site where
app.gamenative.PluviaApp.events.emitJava(...) is invoked and adjusting
DownloadStatusChanged consumers to accept the string or optional Int
accordingly.
- Line 327: In GOGDownloadManager replace the misspelled log string in the
Timber tag call — change Timber.tag("GOG").i("Downoading Chunks for game
$gameId") to use the correct spelling "Downloading" so the message reads
"Downloading Chunks for game $gameId"; keep the same Timber.tag("GOG").i(...)
call and interpolation of gameId.
♻️ Duplicate comments (3)
app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt (1)
871-882: Secure CDN URLs still logged with tokens.Lines 871 and 880 log the full URL which may contain time-limited tokens. Per previous review feedback, consider redacting the URL or logging only the chunk identifier.
app/src/main/java/app/gamenative/service/gog/GOGApiClient.kt (2)
14-16: Typo in documentation: "Formartted" → "Formatted".
460-461:game_typedefault still set to"dlc".Per previous discussion, this should be changed so that games without a
game_typefield are not incorrectly marked as DLC. The author indicated this would be addressed.
🧹 Nitpick comments (3)
app/src/main/java/app/gamenative/service/gog/api/GOGManifestParser.kt (2)
30-54: UnusedpreferredGenerationparameter.The parameter is documented but never used in the function body. The code always filters for
generation == 2(line 42). Consider removing the parameter or implementing the intended logic.♻️ Option: Remove unused parameter
fun selectBuild( builds: List<GOGBuild>, - preferredGeneration: Int? = null, platform: String = "windows" ): GOGBuild? {
63-70: Redundant wildcard language check.
depot.matchesLanguage(language)at line 65 already checks forlanguages.contains("*")(see Depot.matchesLanguage at GOGDataModels.kt line 255). The additional|| (depot.languages?.contains("*") == true)is redundant.♻️ Simplify
fun filterDepotsByLanguage(manifest: GOGManifestMeta, language: String): List<Depot> { val filtered = manifest.depots.filter { depot -> - depot.matchesLanguage(language) || (depot.languages?.contains("*") == true) + depot.matchesLanguage(language) }app/src/main/java/app/gamenative/service/gog/GOGApiClient.kt (1)
245-253: UnusedinstallPathparameter ingetClientSecret.The parameter is documented as "for platform detection" but the function always uses
"windows"(line 253). Consider removing the parameter or implementing platform detection.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/main/java/app/gamenative/service/gog/GOGApiClient.ktapp/src/main/java/app/gamenative/service/gog/GOGDownloadManager.ktapp/src/main/java/app/gamenative/service/gog/api/GOGDataModels.ktapp/src/main/java/app/gamenative/service/gog/api/GOGManifestParser.ktapp/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/test/java/app/gamenative/service/gog/api/GOGManifestParserTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/ui/PluviaMain.kt
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: unbelievableflavour
Repo: utkarshdalal/GameNative PR: 157
File: app/src/main/java/app/gamenative/service/GameManager.kt:60-63
Timestamp: 2025-09-18T12:38:38.471Z
Learning: User prefers to keep PR scope small and focused, deferring non-critical refactoring suggestions to future PRs when the functionality is more fully implemented.
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.
📚 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/gog/api/GOGManifestParser.ktapp/src/main/java/app/gamenative/service/gog/GOGDownloadManager.ktapp/src/test/java/app/gamenative/service/gog/api/GOGManifestParserTest.kt
📚 Learning: 2026-01-16T17:32:04.262Z
Learnt from: phobos665
Repo: utkarshdalal/GameNative PR: 418
File: app/src/main/java/app/gamenative/service/gog/GOGManager.kt:270-282
Timestamp: 2026-01-16T17:32:04.262Z
Learning: In the GOG implementation for GameNative, DLC items should be excluded from the library view using the `isDlc` flag in the exclude condition. The library should only display base games. DLC download decisions are handled separately via the `withDlcs` parameter in GOGDownloadManager and related filtering logic, not through library visibility.
Applied to files:
app/src/main/java/app/gamenative/service/gog/GOGApiClient.ktapp/src/main/java/app/gamenative/service/gog/GOGDownloadManager.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/gog/api/GOGDataModels.kt
📚 Learning: 2025-12-17T05:14:05.133Z
Learnt from: utkarshdalal
Repo: utkarshdalal/GameNative PR: 344
File: app/src/main/java/app/gamenative/service/SteamAutoCloud.kt:415-419
Timestamp: 2025-12-17T05:14:05.133Z
Learning: In SteamAutoCloud.kt, when uploading files to Steam Cloud API (beginFileUpload and commitFileUpload), the filename parameter intentionally uses different formats: `file.path + file.filename` (relative path without placeholder) when `appInfo.ufs.saveFilePatterns.isEmpty()` is true (fallback case), and `file.prefixPath` (includes placeholder like %SteamUserData%) when patterns are configured. This difference is by design.
Applied to files:
app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt
📚 Learning: 2025-09-18T12:38:38.471Z
Learnt from: unbelievableflavour
Repo: utkarshdalal/GameNative PR: 157
File: app/src/main/java/app/gamenative/service/GameManager.kt:60-63
Timestamp: 2025-09-18T12:38:38.471Z
Learning: User prefers to keep PR scope small and focused, deferring non-critical refactoring suggestions to future PRs when the functionality is more fully implemented.
Applied to files:
app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.ktapp/src/test/java/app/gamenative/service/gog/api/GOGManifestParserTest.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/test/java/app/gamenative/service/gog/api/GOGManifestParserTest.kt
⏰ 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 (11)
app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt (2)
1-76: Well-structured data models with proper null-safe JSON parsing.The
fromJsonimplementations correctly usejson.has(key) && !json.isNull(key)pattern for nullable fields likebranchandlegacyBuildId. This properly handles both missing keys and explicit JSON nulls.
424-462: SecureLinksResponse URL construction looks correct.The template interpolation logic properly replaces
{param}placeholders with values from the parameters object and handles escaped slashes. The empty URL guard on line 451 is a good defensive check.app/src/main/java/app/gamenative/service/gog/api/GOGManifestParser.kt (1)
373-387: Decompression loop now properly guards against infinite loops.The added checks for
needsInput()(line 377) andneedsDictionary()(line 378-380) correctly prevent infinite spinning on malformed zlib data.app/src/main/java/app/gamenative/service/gog/GOGDownloadManager.kt (2)
1044-1062: Decompression loop now correctly handles edge cases.The checks for
needsInput()andneedsDictionary()prevent infinite loops on malformed data. The comment at line 1059-1060 appropriately notes the rare case where the inflater is still processing internally.
474-478: HTTP status detection now uses typed exception.The switch from parsing exception message strings to using
HttpStatusExceptionwith a typedstatusCodeproperty is a significant reliability improvement.app/src/main/java/app/gamenative/service/gog/GOGApiClient.kt (2)
102-162:getGameIdsproperly handles authentication and error cases.The function correctly validates credentials, handles HTTP errors, and now properly uses
get(it).toString()for the owned games array to handle integer values from the API.
176-243:getGameByIdhas proper error handling with explicit returns.The catch block now correctly uses
return@withContext Result.failure(e)to properly return errors from the coroutine.app/src/test/java/app/gamenative/service/gog/api/GOGManifestParserTest.kt (4)
1-86: Well-organized test setup with reusable helper methods.The helper functions
createTestChunk,createTestDepotFile,createTestDepot,createTestProduct, andcreateTestBuildprovide clean, consistent test data creation and improve test maintainability.
370-396:testParseBuildsassertions now correct.The assertion at line 395 correctly verifies
productIdas"456"(a String), matching the JSON data and theGOGBuilddata class definition.
467-486:testParseSecureLinksJSON structure now matches parser expectations.The test JSON now correctly provides
urlsas a JSONArray with objects containingurl_formatandparametersfields, matching whatSecureLinksResponse.fromJson()expects.
488-535: Decompression tests cover all three formats.Good coverage of gzip, zlib, and plain text decompression paths. The test setup correctly uses standard Java compression utilities to create realistic test data.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt
Outdated
Show resolved
Hide resolved
phobos665
changed the title
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt:378">
P2: Missing `isNull` check for nullable field. The codebase pattern for nullable JSON fields is `json.has("key") && !json.isNull("key")`. Without the null check, if the API returns `"compressedSize": null`, this will incorrectly return `0L` instead of `null`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt
Outdated
Show resolved
Hide resolved
|
Cubic and Rabbit are now happy with the PR. I'm just going to add in some more testing. I've done some testing on my own devices and things look quite good so far. I'd love to get this into the community's hands for some proper testing. |
… into gog-kotlin-download-manager
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt`:
- Around line 444-448: The loop that replaces URL placeholders uses
paramsObj.get(key).toString(), which turns JSONObject.NULL into the literal
"null"; change this to detect nulls (e.g., paramsObj.isNull(key) or
paramsObj.get(key) == JSONObject.NULL) and use an empty string or skip the
replacement instead of "null". Update the block in the method performing the
replacement (the while loop that iterates keys and updates constructedUrl) so
value = if (paramsObj.isNull(key)) "" else paramsObj.get(key).toString(), then
apply constructedUrl = constructedUrl.replace("{$key}", value).
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt (1)
15-22: Consider extracting common array parsing to extension functions.The pattern of iterating JSON arrays with index-based loops appears throughout the file (lines 18-22, 106-107, 175-177, 183-186, etc.). An extension function would reduce boilerplate.
♻️ Example helper extension
// Add to a utils file or at top of this file inline fun <T> JSONArray.mapObjects(transform: (JSONObject) -> T): List<T> { return (0 until length()).map { transform(getJSONObject(it)) } } inline fun JSONArray.mapStrings(): List<String> { return (0 until length()).map { getString(it) } } // Usage example: val items = json.optJSONArray("items")?.mapObjects { GOGBuild.fromJson(it) } ?: emptyList() val languages = json.optJSONArray("languages")?.mapStrings() ?: emptyList()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/build.gradle.ktsapp/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt
💤 Files with no reviewable changes (1)
- app/build.gradle.kts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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/gog/api/GOGDataModels.kt
📚 Learning: 2025-12-17T05:14:05.133Z
Learnt from: utkarshdalal
Repo: utkarshdalal/GameNative PR: 344
File: app/src/main/java/app/gamenative/service/SteamAutoCloud.kt:415-419
Timestamp: 2025-12-17T05:14:05.133Z
Learning: In SteamAutoCloud.kt, when uploading files to Steam Cloud API (beginFileUpload and commitFileUpload), the filename parameter intentionally uses different formats: `file.path + file.filename` (relative path without placeholder) when `appInfo.ufs.saveFilePatterns.isEmpty()` is true (fallback case), and `file.prefixPath` (includes placeholder like %SteamUserData%) when patterns are configured. This difference is by design.
Applied to files:
app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt
📚 Learning: 2025-09-18T12:38:38.471Z
Learnt from: unbelievableflavour
Repo: utkarshdalal/GameNative PR: 157
File: app/src/main/java/app/gamenative/service/GameManager.kt:60-63
Timestamp: 2025-09-18T12:38:38.471Z
Learning: User prefers to keep PR scope small and focused, deferring non-critical refactoring suggestions to future PRs when the functionality is more fully implemented.
Applied to files:
app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt
⏰ 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 (5)
app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt (5)
62-76: LGTM! Nullable field handling is correct.The
branchandlegacyBuildIdfields properly usehas() && !isNull()checks before callinggetString(), correctly handling both missing keys and explicit null values from the API.
100-156: LGTM! Comprehensive depot parsing with proper null safety.The nested parsing for
DependencyDepotcorrectly handles optional arrays (languages,osBitness) and the nullableexecutable.argumentsfield. The pattern is consistent with the rest of the codebase.
251-259: LGTM! Good helper implementation.The
matchesLanguage()function correctly handles the wildcard"*"case and performs case-insensitive language matching, which aligns with typical GOG manifest behavior.
346-360: LGTM! Nullable fields and support file detection are correctly implemented.The
md5,sha256, andproductIdfields properly use thehas() && !isNull()pattern. TheisSupportFile()implementation using flags is correct per project convention.
388-418: LGTM!Simple and correct implementations for directory and symlink parsing with appropriate path normalization.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| while (keys.hasNext()) { | ||
| val key = keys.next() | ||
| val value = paramsObj.get(key).toString() | ||
| constructedUrl = constructedUrl.replace("{$key}", value) | ||
| } |
There was a problem hiding this comment.
Handle potential JSONObject.NULL in parameter values.
paramsObj.get(key) can return JSONObject.NULL for explicit null values. Calling .toString() on it yields the literal string "null", which would produce malformed URLs like ...token=null....
🔧 Suggested fix
while (keys.hasNext()) {
val key = keys.next()
- val value = paramsObj.get(key).toString()
+ val value = if (paramsObj.isNull(key)) "" else paramsObj.get(key).toString()
constructedUrl = constructedUrl.replace("{$key}", value)
}🤖 Prompt for AI Agents
In `@app/src/main/java/app/gamenative/service/gog/api/GOGDataModels.kt` around
lines 444 - 448, The loop that replaces URL placeholders uses
paramsObj.get(key).toString(), which turns JSONObject.NULL into the literal
"null"; change this to detect nulls (e.g., paramsObj.isNull(key) or
paramsObj.get(key) == JSONObject.NULL) and use an empty string or skip the
replacement instead of "null". Update the block in the method performing the
replacement (the while loop that iterates keys and updates constructedUrl) so
value = if (paramsObj.isNull(key)) "" else paramsObj.get(key).toString(), then
apply constructedUrl = constructedUrl.replace("{$key}", value).
|
I will need to do a final test going from 0.7 to this to see if it migrates without any issue before we do the merge |
app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.kt
Show resolved
Hide resolved
2 participants
Overview
This PR removes all Python code and replaces it with a native Kotlin implementation.
Features:
** Bug Fixes**
Architecture
GOGDownloadManager - Handles download Logic
GOGManifestParser - Handles manifest parsing logic
GOGDataModels - where we hold GOG Data models (especially for API calls)
GOGApiClient - Handles most API calls for GOG
This is also quite in-line with how I've done EGS, using my learnings from doing EGS fully in Kotlin has given me the knowledge to get this done much quicker than I anticipated and with only data-parsing headaches really being the biggest pain-point (which has been solved).
Notes
DLC, Cloud Saves and Downloading Dependencies now work as they did in the python version.
PR is now ready for dev-review
Summary by CodeRabbit
New Features
Improvements
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.