Good Old Games (GOG) Support #329
Conversation
…GoG installation.
Still need to verify the fields as they're currently not giving everything back.
…to gog-integration
|
Warning Rate limit exceeded@phobos665 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 9 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds full GOG integration: Chaquopy-based gogdl Python bundle, Room entity/DAO and DB migration, foreground GOGService with Python bridge, Kotlin GOG managers (auth, downloads, cloud-saves), UI/login/settings/library wiring, path/container helpers, DI bindings, prefs/state, and many localized strings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgb(235,245,255)
participant User
participant UI
participant Service as GOGService
participant Auth as GOGAuthManager
participant Py as GOGPythonBridge
participant DB as Room/GOGGameDao
end
User->>UI: Open GOG login / paste OAuth URL
UI->>Service: authenticateWithCode(authCode)
Service->>Auth: authenticateWithCode(context, authCode)
Auth->>Py: executeCommand("auth", authCode)
Py-->>Auth: credentials JSON
Auth->>DB: persist credentials file (gog_auth.json)
Auth-->>Service: Result<GOGCredentials>
Service->>Py: executeCommand("list")
Py-->>Service: games JSON
Service->>DB: replaceAll/upsertPreservingInstallStatus(games)
Service-->>UI: emit login success / library synced
sequenceDiagram
autonumber
rect rgb(245,255,235)
participant User
participant UI
participant Service as GOGService
participant Manager as GOGManager
participant Py as GOGPythonBridge
participant DB as Room/GOGGameDao
end
User->>UI: Tap download for gameId
UI->>Service: downloadGame(context, gameId, installPath)
Service->>Manager: downloadGame(...)
Manager->>Py: executeCommandWithCallback("download", ...)
Py->>Py: run gogdl workers & call ProgressCallback.update
Py-->>Manager: download result/manifest
Manager->>DB: insert/update GOGGame, mark installed
Manager-->>Service: progress updates / completion
Service-->>UI: emit progress / complete
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Will work on adding a signifier for games being installed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
app/src/main/java/app/gamenative/service/gog/GOGManager.kt (3)
1075-1088: Wrap HTTP response in.use {}to prevent resource leak.The OkHttp
Responseat line 1075 must be closed in all code paths. Whileresponse.body?.string()closes the body when called successfully, if an exception occurs before that, the response will leak connections.
506-509: Remove dead code: null check on non-nullableResult.
resultis a non-nullableResult<String>type, soif(result == null)is always false and this block is unreachable. The failure case is already handled at lines 500-502.🔎 Proposed fix
val output = result.getOrNull() ?: "" - if(result == null) { - Timber.w("Game $gameId not found in GOG library") - return Result.success(null) - } - val gameDetails = org.json.JSONObject(output.trim())
951-955: Handle potentialIllegalArgumentExceptionfromrelativeTo().
relativeTo()throwsIllegalArgumentExceptionif the paths don't share a common ancestor. While unlikely during normal operation, malformed paths or symlinks could trigger this and crash the launch flow.🔎 Proposed fix
val gameInstallDir = File(gameInstallPath) val execFile = File(gameInstallPath, executablePath) - val relativePath = execFile.relativeTo(gameInstallDir).path.replace('/', '\\') + val relativePath = try { + execFile.relativeTo(gameInstallDir).path.replace('/', '\\') + } catch (e: IllegalArgumentException) { + Timber.e(e, "Cannot compute relative path for executable, using absolute") + executablePath.replace('/', '\\') + } val windowsPath = "$gogDriveLetter:\\$relativePath"app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (1)
93-101: Memory leak: Event listener never unregistered.The
installListeneris registered but never unregistered when the composable leaves composition. Add cleanup usingawaitCancellation()in atry/finallyblock.🔎 Proposed fix
LaunchedEffect(gameId) { val installListener: (app.gamenative.events.AndroidEvent.LibraryInstallStatusChanged) -> Unit = { event -> if (event.appId == libraryItem.gameId) { Timber.tag(TAG).d("Install status changed, refreshing game data for $gameId") refreshTrigger++ } } app.gamenative.PluviaApp.events.on<app.gamenative.events.AndroidEvent.LibraryInstallStatusChanged, Unit>(installListener) + + try { + kotlinx.coroutines.awaitCancellation() + } finally { + app.gamenative.PluviaApp.events.off<app.gamenative.events.AndroidEvent.LibraryInstallStatusChanged, Unit>(installListener) + } }
🧹 Nitpick comments (5)
app/src/main/java/app/gamenative/service/gog/GOGConstants.kt (2)
40-47: Property getter with side effects.The
mkdirs()call in the getter executes on every property access, which is unnecessary after the first call and could cause minor performance overhead. Consider lazy initialization.🔎 Proposed refactor
+ private var _internalGOGGamesPath: String? = null + val internalGOGGamesPath: String get() { + _internalGOGGamesPath?.let { return it } val context = appContext ?: throw IllegalStateException("GOGConstants not initialized. Call init() first.") val path = Paths.get(context.filesDir.absolutePath, "GOG", "games", "common").toString() // Ensure directory exists for StatFs File(path).mkdirs() + _internalGOGGamesPath = path return path }
61-70: Logging on every property access may be noisy.
defaultGOGGamesPathlogs via Timber on every access. Consider removing these logs or making them debug-only with a flag, as they could flood logs during normal operation.app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt (1)
32-35: Consider sharing the HTTP client instance.Creating a new
OkHttpClientper manager instance is wasteful. Consider injecting a shared client or using a companion object singleton to reuse connection pools and reduce resource consumption.app/src/main/java/app/gamenative/service/gog/GOGAuthManager.kt (1)
172-177: Avoid creating newOkHttpClientper request.Line 177 creates a new
OkHttpClient()for each token request. This is inefficient as it doesn't reuse connection pools. Consider using a shared client instance.🔎 Proposed fix
object GOGAuthManager { + private val httpClient = OkHttpClient() // ... other code ... val request = okhttp3.Request.Builder() .url(tokenUrl) .get() .build() - val response = okhttp3.OkHttpClient().newCall(request).execute() + val response = httpClient.newCall(request).execute()app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (1)
263-265: Use structured concurrency instead of creating newCoroutineScope.Creating
CoroutineScope(Dispatchers.IO).launchin multiple places leads to unstructured concurrency - these coroutines are not tied to any lifecycle and could continue running after the composable is disposed.Consider passing a coroutine scope from the ViewModel or using
rememberCoroutineScope()in composable functions.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/src/main/java/app/gamenative/PluviaApp.ktapp/src/main/java/app/gamenative/data/GOGCloudSavesLocation.ktapp/src/main/java/app/gamenative/enums/PathType.ktapp/src/main/java/app/gamenative/service/gog/GOGAuthManager.ktapp/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.ktapp/src/main/java/app/gamenative/service/gog/GOGConstants.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/screen/library/appscreen/GOGAppScreen.ktapp/src/main/java/app/gamenative/utils/StorageUtils.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/app/gamenative/PluviaApp.kt
- app/src/main/java/app/gamenative/enums/PathType.kt
- app/src/main/java/app/gamenative/service/gog/GOGService.kt
🧰 Additional context used
🧠 Learnings (6)
📚 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/GOGConstants.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/data/GOGCloudSavesLocation.ktapp/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.ktapp/src/main/java/app/gamenative/service/gog/GOGManager.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.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/service/gog/GOGManager.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.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/service/gog/GOGManager.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.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/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/GOGManager.kt
🧬 Code graph analysis (3)
app/src/main/java/app/gamenative/service/gog/GOGAuthManager.kt (2)
app/src/main/python/gogdl/dl/objects/v1.py (1)
File(36-49)app/src/main/java/app/gamenative/service/gog/GOGService.kt (1)
hasStoredCredentials(113-115)
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt (2)
app/src/main/python/gogdl/dl/objects/v1.py (1)
File(36-49)app/src/main/python/gogdl/saves.py (2)
SyncFile(28-53)SyncClassifier(325-365)
app/src/main/java/app/gamenative/service/gog/GOGManager.kt (3)
app/src/main/python/gogdl/dl/objects/v1.py (1)
File(36-49)app/src/main/java/app/gamenative/service/gog/GOGService.kt (3)
refreshSingleGame(338-341)verifyInstallation(265-268)getInstalledExe(271-274)app/src/main/java/app/gamenative/service/gog/GOGConstants.kt (1)
getGameInstallPath(72-76)
⏰ 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 (8)
app/src/main/java/app/gamenative/data/GOGCloudSavesLocation.kt (1)
1-26: LGTM!Clean, well-documented data classes. The separation between template (pre-resolution) and resolved location is a good design choice for handling GOG path variables.
app/src/main/java/app/gamenative/service/gog/GOGConstants.kt (1)
72-76: LGTM!The sanitization logic is reasonable for filesystem compatibility while preserving readability.
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt (2)
488-524: LGTM!The file classification logic correctly mirrors the Python implementation's behavior for determining sync actions.
77-92: Bug: MD5 digest is never updated with data.The
MessageDigestis created butdigest.update()is never called. The hash will always be the MD5 of an empty byte array, causing incorrect sync comparisons with cloud files.🔎 Proposed fix
// Calculate MD5 of gzipped content (matching Python implementation) FileInputStream(file).use { fis -> val digest = MessageDigest.getInstance("MD5") val buffer = java.io.ByteArrayOutputStream() GZIPOutputStream(buffer).use { gzipOut -> val fileBuffer = ByteArray(8192) var bytesRead: Int while (fis.read(fileBuffer).also { bytesRead = it } != -1) { gzipOut.write(fileBuffer, 0, bytesRead) } } - md5Hash = digest.digest(buffer.toByteArray()) + val gzippedBytes = buffer.toByteArray() + digest.update(gzippedBytes) + md5Hash = digest.digest() .joinToString("") { "%02x".format(it) } }Likely an incorrect or invalid review comment.
app/src/main/java/app/gamenative/service/gog/GOGManager.kt (2)
733-757: LGTM -runBlockingusage is acceptable here.Based on learnings, using
runBlockingto maintain synchronous APIs is acceptable when performance is not a concern. This approach prevents UI breakage and maintains API compatibility.
1456-1467: LGTM!Timestamp persistence with proper exception handling.
app/src/main/java/app/gamenative/service/gog/GOGAuthManager.kt (1)
389-427: LGTM!Good improvement -
parseFullCredentialsFromFilenow properly returnsnullon failure instead of dummy credentials, allowing callers to handle authentication failures correctly.app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (1)
503-575: LGTM!The
observeGameStateimplementation properly tracks listeners in a disposables list and returns a cleanup function, correctly handling lifecycle management for event subscriptions.
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt (3)
319-340: Wrap HTTP response in.use {}to prevent resource leak.The OkHttp
Responseobject at line 319 is not closed on all paths, which can leak connections. This issue was previously flagged and remains unresolved.
412-420: Wrap HTTP response in.use {}to prevent resource leak.The OkHttp
Responseobject at line 412 is not closed on all paths, which can leak connections. This issue was previously flagged and remains unresolved.
450-460: Wrap HTTP response in.use {}to prevent resource leak.The OkHttp
Responseobject at line 450 is not closed on all paths, which can leak connections. Additionally, line 459 loads the entire response body into memory. This issue was previously flagged and remains unresolved.app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (3)
93-101: Memory leak: Event listener never unregistered.The
installListenerregistered at line 100 is never unregistered when the composable leaves composition. The LaunchedEffect doesn't include cleanup logic, causing potential memory leaks and callbacks to stale state.🔎 Proposed fix
LaunchedEffect(gameId) { val installListener: (app.gamenative.events.AndroidEvent.LibraryInstallStatusChanged) -> Unit = { event -> if (event.appId == libraryItem.gameId) { Timber.tag(TAG).d("Install status changed, refreshing game data for $gameId") refreshTrigger++ } } app.gamenative.PluviaApp.events.on<app.gamenative.events.AndroidEvent.LibraryInstallStatusChanged, Unit>(installListener) + + try { + kotlinx.coroutines.awaitCancellation() + } finally { + app.gamenative.PluviaApp.events.off<app.gamenative.events.AndroidEvent.LibraryInstallStatusChanged, Unit>(installListener) + } }
316-320: Inconsistent cancel/cleanup order may cause issues.Lines 318-319 call
cleanupDownloadbeforecancel(), butonDownloadInstallClick(lines 220-221) callscancel()beforecleanupDownload. IfcleanupDownloadremoves internal tracking, callingcancel()afterward may be ineffective.🔎 Proposed fix
if (downloadInfo != null) { Timber.tag(TAG).i("Cancelling GOG download: ${libraryItem.appId}") - GOGService.cleanupDownload(gameId) downloadInfo.cancel() + GOGService.cleanupDownload(gameId) }
332-337: Inconsistent cancel/cleanup order may cause issues.Lines 335-336 call
cleanupDownloadbeforecancel(), inconsistent withonDownloadInstallClick. Use the same order as lines 220-221 for consistency.🔎 Proposed fix
if (isDownloading) { // Cancel download immediately if currently downloading Timber.tag(TAG).i("Cancelling active download for GOG game: ${libraryItem.appId}") - GOGService.cleanupDownload(gameId) downloadInfo.cancel() + GOGService.cleanupDownload(gameId) android.widget.Toast.makeText(
🧹 Nitpick comments (3)
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt (3)
397-397: Consider streaming large files to avoid memory issues.Line 397 loads the entire file into memory with
readBytes(). For large save files (potentially hundreds of MB), this could cause OutOfMemoryError on memory-constrained devices.🔎 Consider using streaming upload
Replace:
-val requestBody = localFile.readBytes().toRequestBody("application/octet-stream".toMediaType()) +val requestBody = localFile.asRequestBody("application/octet-stream".toMediaType())This uses OkHttp's built-in streaming support which reads the file in chunks rather than loading it entirely into memory.
459-468: Consider streaming large downloads to avoid memory issues.Line 459 loads the entire download into memory with
bytes(). For large save files, this could cause OutOfMemoryError.🔎 Consider using streaming download
-val bytes = response.body?.bytes() ?: return@withContext -Timber.tag("GOG-CloudSaves").d("Downloaded ${bytes.size} bytes for ${file.relativePath}") - -// Save to local file -val localFile = File(syncDir, file.relativePath) -localFile.parentFile?.mkdirs() - -FileOutputStream(localFile).use { fos -> - fos.write(bytes) -} +val localFile = File(syncDir, file.relativePath) +localFile.parentFile?.mkdirs() + +response.body?.byteStream()?.use { inputStream -> + FileOutputStream(localFile).use { fos -> + inputStream.copyTo(fos) + } +} ?: return@withContext + +Timber.tag("GOG-CloudSaves").d("Downloaded ${localFile.length()} bytes for ${file.relativePath}")This streams the download directly to disk without buffering the entire file in memory.
140-268: Consider adding retry logic for transient network failures.The sync operation performs multiple HTTP requests but doesn't retry on transient network failures. While the current error handling (returning 0L on failure) prevents crashes, adding exponential backoff retry logic would improve reliability for users on unstable connections.
This is particularly relevant for mobile devices that may experience intermittent connectivity during cloud save operations.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
🧰 Additional context used
🧠 Learnings (3)
📚 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/GOGCloudSavesManager.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.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/appscreen/GOGAppScreen.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/appscreen/GOGAppScreen.kt
🧬 Code graph analysis (1)
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt (2)
app/src/main/python/gogdl/dl/objects/v1.py (1)
File(36-49)app/src/main/python/gogdl/saves.py (2)
SyncFile(28-53)SyncClassifier(325-365)
⏰ 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/GOGCloudSavesManager.kt (4)
25-38: LGTM: OkHttpClient configuration and constants.The HTTP client setup with 30-second timeouts is appropriate for cloud storage operations, and the constants are well-defined.
50-96: LGTM: Metadata calculation matches Python implementation.The MD5 calculation on gzipped content correctly mirrors the Python gogdl implementation shown in the relevant code snippets, ensuring compatibility with GOG's cloud save format.
485-521: LGTM: Safe file classification logic.The non-null assertions on lines 503 and 515 are protected by null checks in the same conditional expressions, making them safe from NPEs.
273-295: LGTM: Local file scanning implementation.The recursive directory scan and sequential metadata calculation is straightforward and appropriate for typical save file volumes.
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (7)
37-78: LGTM: Dialog state management and formatting helper.The companion object properly manages dialog state, and
formatBytescorrectly usesLocale.USfor consistent number formatting across locales.
158-205: LGTM: Install and download state methods.The state checking methods properly delegate to GOGService, and
hasPartialDownloadcorrectly returns false since GOG downloads cannot be paused/resumed.
207-258: LGTM: Download/install click handler.The method properly handles the download, cancel, and launch flows. The cleanup order at lines 220-221 (cancel before cleanup) is correct.
260-306: LGTM: Download initiation and user feedback.The method properly initiates downloads via GOGService and provides appropriate user feedback through toasts.
349-454: LGTM: Uninstall, container, and utility methods.The uninstall flow properly delegates to GOGService, container management uses ContainerUtils correctly, and the TODO for update functionality is documented.
456-528: LGTM: Proper event listener lifecycle management.The
observeGameStatemethod correctly tracks event listeners in a disposables list and returns a cleanup function. The progress listener management with add/remove is properly implemented.
534-630: LGTM: Dialog management with proper state observation.The dialogs properly use
snapshotFlowto observe state changes and render install/uninstall confirmations with appropriate callbacks.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/src/main/java/app/gamenative/service/gog/GOGManager.kt (1)
489-523: Remove impossible null check.Line 506 checks
if(result == null), butresultis a non-nullableResult<String>type. This condition is always false and represents dead code that should be removed. The failure case is already handled at lines 500-502 with an early return.🔎 Proposed fix
val output = result.getOrNull() ?: "" - if(result == null) { - Timber.w("Game $gameId not found in GOG library") - return Result.success(null) - } - val gameDetails = org.json.JSONObject(output.trim()) var game = parseGameObject(gameDetails)app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (1)
93-101: Critical: Memory leak - Event listener never unregistered.The
installListeneris registered withPluviaApp.events.on<...>but never unregistered when the composable leaves composition. This causes memory leaks and potential callbacks to stale state.Compare to
observeGameState(lines 456-528) which properly tracks listeners and returns a cleanup function.🔎 Proposed fix
LaunchedEffect(gameId) { val installListener: (app.gamenative.events.AndroidEvent.LibraryInstallStatusChanged) -> Unit = { event -> if (event.appId == libraryItem.gameId) { Timber.tag(TAG).d("Install status changed, refreshing game data for $gameId") refreshTrigger++ } } app.gamenative.PluviaApp.events.on<app.gamenative.events.AndroidEvent.LibraryInstallStatusChanged, Unit>(installListener) + + // Cleanup when composable leaves composition + try { + kotlinx.coroutines.awaitCancellation() + } finally { + app.gamenative.PluviaApp.events.off<app.gamenative.events.AndroidEvent.LibraryInstallStatusChanged, Unit>(installListener) + } }
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/service/gog/GOGService.kt (1)
359-516: Consider refactoring to eliminate duplicategetInstance()calls.Lines 364 and 376 both call
getInstance(). While functionally correct, this is inefficient. Consider capturing the instance once at the beginning of the function:🔎 Proposed refactor
suspend fun syncCloudSaves(context: Context, appId: String, preferredAction: String = "none"): Boolean = withContext(Dispatchers.IO) { try { Timber.tag("GOG").d("[Cloud Saves] syncCloudSaves called for $appId with action: $preferredAction") - // Check if there's already a sync in progress for this appId - val serviceInstance = getInstance() - if (serviceInstance == null) { + val instance = getInstance() + if (instance == null) { Timber.tag("GOG").e("[Cloud Saves] Service instance not available for sync start") return@withContext false } - if (!serviceInstance.gogManager.startSync(appId)) { + if (!instance.gogManager.startSync(appId)) { Timber.tag("GOG").w("[Cloud Saves] Sync already in progress for $appId, skipping duplicate sync") return@withContext false } try { - val instance = getInstance() - if (instance == null) { - Timber.tag("GOG").e("[Cloud Saves] Service instance not available") - return@withContext false - } - if (!GOGAuthManager.hasStoredCredentials(context)) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/main/java/app/gamenative/service/gog/GOGAuthManager.ktapp/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.ktapp/src/main/java/app/gamenative/service/gog/GOGManager.ktapp/src/main/java/app/gamenative/service/gog/GOGPythonBridge.ktapp/src/main/java/app/gamenative/service/gog/GOGService.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
🧰 Additional context used
🧠 Learnings (5)
📚 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/GOGCloudSavesManager.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.ktapp/src/main/java/app/gamenative/service/gog/GOGManager.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/appscreen/GOGAppScreen.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/GOGPythonBridge.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/appscreen/GOGAppScreen.ktapp/src/main/java/app/gamenative/service/gog/GOGManager.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/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/GOGManager.kt
🧬 Code graph analysis (3)
app/src/main/java/app/gamenative/service/gog/GOGAuthManager.kt (1)
app/src/main/java/app/gamenative/service/gog/GOGService.kt (1)
hasStoredCredentials(113-115)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (2)
app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt (1)
formatBytes(206-216)app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.kt (3)
isInstalled(79-79)isDownloading(89-89)resetContainerToDefaults(412-419)
app/src/main/java/app/gamenative/service/gog/GOGService.kt (1)
app/src/main/java/app/gamenative/service/gog/GOGAuthManager.kt (1)
clearStoredCredentials(261-273)
🪛 detekt (1.23.8)
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt
[warning] 357-357: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
app/src/main/java/app/gamenative/service/gog/GOGPythonBridge.kt
[warning] 255-255: 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 (12)
app/src/main/java/app/gamenative/service/gog/GOGPythonBridge.kt (1)
14-53: LGTM! Progress callback implementation is solid.The binary mebibyte conversion (1024*1024) correctly matches GOGDL's internal unit convention, addressing the previous review concern. Error handling is appropriate with try-catch wrapping and logging.
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt (1)
140-268: LGTM! Comprehensive cloud saves synchronization.The sync orchestration properly handles all scenarios (no cloud, upload-only, download-only, complex sync with classifier). All HTTP responses are now properly wrapped in
.use {}blocks as requested in previous reviews.app/src/main/java/app/gamenative/service/gog/GOGAuthManager.kt (1)
35-81: LGTM! Authentication flow is secure and robust.All previous security concerns (logging auth codes, dummy credentials) have been addressed. HTTP responses are properly managed with
.use {}blocks.app/src/main/java/app/gamenative/service/gog/GOGManager.kt (2)
733-757: LGTM! Installation detection is working correctly.The marker-based installation check is solid. The use of
runBlockinghere is acceptable per project conventions and learnings—this maintains synchronous API compatibility without requiring widespread refactoring.
1082-1151: LGTM! HTTP resources properly managed.All HTTP response objects are correctly wrapped in
.use {}blocks, ensuring connections are closed in all code paths. This addresses the previous resource leak concerns.app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (7)
38-78: LGTM: Clean dialog state management and locale-aware formatting.The companion object properly manages dialog state and the
formatByteshelper correctly usesLocale.USfor consistent formatting across devices.
158-205: LGTM: State checks are well-implemented.All installation and download state checks properly delegate to
GOGServicewith appropriate error handling. ThehasPartialDownloadreturningfalsecorrectly reflects GOG's all-or-nothing download architecture.
207-306: LGTM: Download and install logic is well-structured.The implementation correctly handles download cancellation, installation, and game launching. Good use of
BaseAppScreen.showInstallDialogfor consistency and proper error handling with user feedback.
308-387: LGTM: Consistent cancel/cleanup ordering and proper uninstall flow.All download cancellation paths correctly call
cancel()beforecleanupDownload(), ensuring internal state remains available during cancellation. The uninstall flow properly delegates toGOGServicewith appropriate error handling and user feedback.
389-454: LGTM: Container support and path methods are properly implemented.All methods correctly delegate to appropriate services (
GOGService,ContainerUtils). The TODO foronUpdateClick(lines 391-392) is acknowledged as future work in the PR objectives.
456-528: Excellent: Proper event listener lifecycle management.This implementation demonstrates correct resource management:
- All event listeners are tracked in the
disposableslist- Each registration has a corresponding cleanup function
- Progress listeners are properly added/removed based on download state
- Returns a cleanup function for the caller to invoke when done
This is a good reference pattern for event listener management in Compose.
530-630: LGTM: Dialog composition is well-structured and consistent.The dialog implementation properly:
- Uses reactive state observation with
snapshotFlow- Leverages shared
BaseAppScreendialog state for consistency- Provides appropriate confirm/dismiss handlers
- Uses localized strings throughout
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/src/main/java/app/gamenative/service/gog/GOGPythonBridge.kt (3)
122-134: Capture stderr to avoid losing error messages.Only stdout is captured while stderr is left at its default destination. If
gogdl.cli.main()writes error messages, stack traces, or warnings to stderr, they won't be included in the returned output or logs, making debugging difficult.🔎 Proposed enhancement to capture both stdout and stderr
// Capture stdout val stdoutCapture = io.callAttr("StringIO") val originalStdout = sys.get("stdout") sys.put("stdout", stdoutCapture) + + // Capture stderr + val stderrCapture = io.callAttr("StringIO") + val originalStderr = sys.get("stderr") + sys.put("stderr", stderrCapture) // Execute the main function gogdlCli.callAttr("main") // Get the captured output val output = stdoutCapture.callAttr("getvalue").toString() + val errors = stderrCapture.callAttr("getvalue").toString() // Restore original stdout sys.put("stdout", originalStdout) + sys.put("stderr", originalStderr) + + // Log any errors + if (errors.isNotEmpty()) { + Timber.w("GOGDL stderr output: %s", errors) + }
198-209: Use structured concurrency for the estimator job.Creating a new
CoroutineScope(Dispatchers.IO)for the estimator bypasses structured concurrency. While the job is cancelled in thefinallyblock, usingcoroutineScope { launch { ... } }would automatically handle cancellation propagation and is more idiomatic Kotlin coroutines.🔎 Proposed refactor
// Check for cancellation before starting ensureActive() - // Start a simple progress estimator in case callback doesn't work - val estimatorJob = CoroutineScope(Dispatchers.IO).launch { - estimateProgress(downloadInfo) - } - try { - // Execute the main function - gogdlCli.callAttr("main") - Timber.i("GOGDL execution completed successfully") - Result.success("Download completed") + // Start progress estimator concurrently with main execution + coroutineScope { + launch { estimateProgress(downloadInfo) } + + // Execute the main function + gogdlCli.callAttr("main") + Timber.i("GOGDL execution completed successfully") + } + Result.success("Download completed") - } finally { - estimatorJob.cancel() - } + } catch (e: Exception) { + throw e + }This ensures the estimator is cancelled automatically when the coroutine scope exits.
178-185: Document the fragile callback registration mechanism.The code directly sets
gogdl._progress_callbackwithout verifying that the Python module supports this interface. This relies on undocumented behavior and will silently fall back to estimation if the attribute can't be set. Consider adding a comment explaining this coupling and the expected Python-side integration.💡 Suggested documentation
// Get the gogdl module and set up callback val gogdlModule = python.getModule("gogdl") - // Try to set progress callback if gogdl supports it + // Attempt to register progress callback by setting gogdl._progress_callback. + // This relies on the bundled Python gogdl code checking for and invoking + // this attribute during downloads. If unsupported, the estimator provides + // fallback progress indication. try { gogdlModule.put("_progress_callback", progressCallback) } catch (e: Exception) { Timber.w(e, "Could not register progress callback, will use estimation") }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/service/gog/GOGPythonBridge.kt
🧰 Additional context used
🧠 Learnings (1)
📚 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/GOGPythonBridge.kt
🪛 detekt (1.23.8)
app/src/main/java/app/gamenative/service/gog/GOGPythonBridge.kt
[warning] 259-259: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 44-44: String.format("%.1f%%", percent) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
⏰ 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/service/gog/GOGPythonBridge.kt (2)
259-260: CancellationException handling is correct here.The static analysis warning about swallowing the exception can be safely ignored. Since this is a fallback estimation running in a background job, catching and logging
CancellationExceptionwithout re-throwing is appropriate—the job is being cancelled intentionally when the main download completes or fails.
26-35: Add thread-safety synchronization to DownloadInfo's byte tracking methods.The
getTotalExpectedBytes(),setTotalExpectedBytes(),getBytesDownloaded(), andupdateBytesDownloaded()methods operate on plain mutable Long variables without synchronization or@Volatileannotations. The check-then-set pattern at lines 26–35 creates race conditions when the callback executes on Python threads while other threads (estimator, UI) concurrently read or modify these fields. Ensure thread safety by either marking fields@Volatile, adding@Synchronizedto methods, or usingAtomicLong.
| if (eta.isNotEmpty() && eta != "00:00:00") { | ||
| downloadInfo.updateStatusMessage("ETA: $eta") | ||
| } else if (percent > 0f) { | ||
| downloadInfo.updateStatusMessage(String.format("%.1f%%", percent)) |
There was a problem hiding this comment.
Use explicit locale for user-facing formatted strings.
String.format uses the system default locale, which can cause inconsistent formatting across different regions (e.g., "0,5%" vs "0.5%").
🔎 Proposed fix
- downloadInfo.updateStatusMessage(String.format("%.1f%%", percent))
+ downloadInfo.updateStatusMessage(String.format(java.util.Locale.US, "%.1f%%", percent))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| downloadInfo.updateStatusMessage(String.format("%.1f%%", percent)) | |
| downloadInfo.updateStatusMessage(String.format(java.util.Locale.US, "%.1f%%", percent)) |
🧰 Tools
🪛 detekt (1.23.8)
[warning] 44-44: String.format("%.1f%%", percent) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/gog/GOGPythonBridge.kt around line
44, the call to String.format currently relies on the system default locale
which can produce inconsistent decimal separators; change it to call
String.format with an explicit locale appropriate for user-facing text (e.g.,
Locale.getDefault()) and import java.util.Locale so the percent is formatted
with the intended locale: use String.format(Locale.getDefault(), "%.1f%%",
percent).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/src/main/python/gogdl/api.py (2)
18-18: Remove unnecessary f-string prefix.The f-string on line 18 contains no placeholders, making the
fprefix superfluous.🔎 Proposed fix
- 'User-Agent': f'gogdl/1.0.0 (Android GameNative)' + 'User-Agent': 'gogdl/1.0.0 (Android GameNative)'
135-183: Consider refactoring to iterative retry pattern.While the bounded retry logic (lines 149-151) addresses the unbounded recursion issue from the previous review, the method still uses recursive calls (lines 172, 183). This can still risk stack overflow if
max_retriesis increased or if there are nested calls elsewhere. Additionally, line 177 should uselogging.exceptioninstead oflogging.errorto capture the full traceback.🔎 Proposed iterative implementation
def get_secure_link(self, product_id, path="", generation=2, root=None, attempt=0, max_retries=3): """Get secure download links from GOG API with bounded retry Args: product_id: GOG product ID path: Optional path parameter generation: API generation version (1 or 2) root: Optional root parameter - attempt: Current attempt number (internal, default: 0) max_retries: Maximum number of retry attempts (default: 3) Returns: List of secure URLs, or empty list if all retries exhausted """ - if attempt >= max_retries: - self.logger.error(f"Failed to get secure link after {max_retries} attempts for product {product_id}") - return [] - url = "" if generation == 2: url = f"{constants.GOG_CONTENT_SYSTEM}/products/{product_id}/secure_link?_version=2&generation=2&path={path}" elif generation == 1: url = f"{constants.GOG_CONTENT_SYSTEM}/products/{product_id}/secure_link?_version=2&type=depot&path={path}" if root: url += f"&root={root}" - try: - response = self.get_authenticated_request(url) - - if response.status_code != 200: - self.logger.warning( - f"Invalid secure link response: {response.status_code} " - f"(attempt {attempt + 1}/{max_retries}) for product {product_id}" - ) - sleep_time = 0.2 * (2 ** attempt) - time.sleep(sleep_time) - return self.get_secure_link(product_id, path, generation, root, attempt + 1, max_retries) - - return response.json().get('urls', []) - - except Exception as e: - self.logger.error( - f"Failed to get secure link: {e} " - f"(attempt {attempt + 1}/{max_retries}) for product {product_id}" - ) - sleep_time = 0.2 * (2 ** attempt) - time.sleep(sleep_time) - return self.get_secure_link(product_id, path, generation, root, attempt + 1, max_retries) + for attempt in range(max_retries): + try: + response = self.get_authenticated_request(url) + + if response.status_code == 200: + return response.json().get('urls', []) + + self.logger.warning( + f"Invalid secure link response: {response.status_code} " + f"(attempt {attempt + 1}/{max_retries}) for product {product_id}" + ) + + except Exception as e: + self.logger.exception( + f"Failed to get secure link: {e} " + f"(attempt {attempt + 1}/{max_retries}) for product {product_id}" + ) + + if attempt < max_retries - 1: + sleep_time = 0.2 * (2 ** attempt) + time.sleep(sleep_time) + + self.logger.error(f"Failed to get secure link after {max_retries} attempts for product {product_id}") + return []
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/python/gogdl/api.py
🧰 Additional context used
🪛 Ruff (0.14.10)
app/src/main/python/gogdl/api.py
18-18: f-string without any placeholders
Remove extraneous f prefix
(F541)
119-119: Do not catch blind exception: Exception
(BLE001)
176-176: Do not catch blind exception: Exception
(BLE001)
177-180: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ 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/python/gogdl/api.py (1)
100-123: LGTM! Defensive ownership check with sensible fallback.The method correctly implements a tiered ownership check: local cache first, then remote API, with a permissive fallback. The broad exception handling on line 119 is appropriate here since this is a best-effort check, and the default
Truereturn (line 123) is well-documented.
| if len(expanded) > 0: | ||
| expanded_arg += ','.join(expanded) | ||
| url += expanded_arg | ||
| response = self.session.get(url) |
There was a problem hiding this comment.
Add timeout parameter to all HTTP requests to prevent indefinite blocking.
Multiple session.get calls lack timeout parameters (lines 48, 57, 70, 82, 90, 98, 128). This issue was flagged in a previous review but remains unaddressed. Without timeouts, requests can block indefinitely if GOG servers become unresponsive, potentially freezing the application or triggering Android's watchdog.
🔎 Proposed fix
Add a module-level constant after the imports:
from gogdl.dl import dl_utils
import gogdl.constants as constants
+# HTTP request timeout in seconds
+REQUEST_TIMEOUT = 30
+Then update each session.get call to include the timeout:
def get_item_data(self, id, expanded=None):
...
- response = self.session.get(url)
+ response = self.session.get(url, timeout=REQUEST_TIMEOUT)
def get_game_details(self, id):
url = f'{constants.GOG_EMBED}/account/gameDetails/{id}.json'
- response = self.session.get(url)
+ response = self.session.get(url, timeout=REQUEST_TIMEOUT)
def get_user_data(self):
...
- response = self.session.get(url)
+ response = self.session.get(url, timeout=REQUEST_TIMEOUT)
def get_builds(self, product_id, platform):
url = f'{constants.GOG_CONTENT_SYSTEM}/products/{product_id}/os/{platform}/builds?generation=2'
- response = self.session.get(url)
+ response = self.session.get(url, timeout=REQUEST_TIMEOUT)
def get_manifest(self, manifest_id, product_id):
url = f'{constants.GOG_CONTENT_SYSTEM}/products/{product_id}/os/windows/builds/{manifest_id}'
- response = self.session.get(url)
+ response = self.session.get(url, timeout=REQUEST_TIMEOUT)
def get_authenticated_request(self, url):
"""Make an authenticated request with proper headers"""
- return self.session.get(url)
+ return self.session.get(url, timeout=REQUEST_TIMEOUT)
def get_dependencies_repo(self, depot_version=2):
self.logger.info("Getting Dependencies repository")
url = constants.DEPENDENCIES_URL if depot_version == 2 else constants.DEPENDENCIES_V1_URL
- response = self.session.get(url)
+ response = self.session.get(url, timeout=REQUEST_TIMEOUT)Based on previous review comments, this is a known issue that needs to be resolved.
Also applies to: 57-57, 70-70, 82-82, 90-90, 98-98, 128-128
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/api.py around lines 48,57,70,82,90,98 and 128, HTTP
requests using self.session.get lack a timeout causing potential indefinite
blocking; add a module-level constant (e.g. DEFAULT_REQUEST_TIMEOUT_SECONDS =
10) after the imports and update every self.session.get(...) call in this file
to pass timeout=DEFAULT_REQUEST_TIMEOUT_SECONDS so all requests fail fast
instead of hanging; ensure the constant is used uniformly and run tests to
confirm no behavioral regressions.
There was a problem hiding this comment.
Going to remove Python in a follow-up branch eventually. So I won't bother with this change.
There was a problem hiding this comment.
@phobos665, understood! Since you're planning to replace the Python implementation with a native Kotlin solution in a follow-up branch, it makes sense to defer this fix. The timeout issue will be resolved when the Python code is removed.
Good luck with the native implementation—that should provide better performance and integration with the Android app! 🚀
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/service/gog/GOGPythonBridge.kt (1)
44-44: Use explicit locale for user-facing formatted strings.This issue was flagged in a previous review but remains unaddressed.
String.formatwithout an explicit locale uses the system default, causing inconsistent decimal separators across regions (e.g., "0,5%" vs "0.5%").🔎 Proposed fix
- downloadInfo.updateStatusMessage(String.format("%.1f%%", percent)) + downloadInfo.updateStatusMessage(String.format(java.util.Locale.getDefault(), "%.1f%%", percent))
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/service/gog/GOGPythonBridge.kt (1)
166-224: Add stdout capture for consistency withexecuteCommand.Unlike
executeCommand(lines 122-134), this function doesn't capture or restore stdout. If Python code writes to stdout, it may interfere with logging or other operations. Consider capturing stdout as done in the non-callback variant for consistency.🔎 Proposed implementation
Add stdout capture/restoration similar to
executeCommand:val argsList = listOf("gogdl") + args.toList() val pythonList = python.builtins.callAttr("list", argsList.toTypedArray()) sys.put("argv", pythonList) + + // Capture stdout to prevent interference + val io = python.getModule("io") + val stdoutCapture = io.callAttr("StringIO") + val originalStdout = sys.get("stdout") + sys.put("stdout", stdoutCapture) // Check for cancellation before starting ensureActive() // Start a simple progress estimator in case callback doesn't work val estimatorJob = launch { estimateProgress(downloadInfo) } try { // Execute the main function gogdlCli.callAttr("main") Timber.i("GOGDL execution completed successfully") + + // Optionally log captured output + val output = stdoutCapture.callAttr("getvalue").toString() + if (output.isNotEmpty()) { + Timber.d("GOGDL output: $output") + } + Result.success("Download completed") } finally { estimatorJob.cancel() + sys.put("stdout", originalStdout) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/service/gog/GOGPythonBridge.kt
🧰 Additional context used
🧠 Learnings (1)
📚 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/GOGPythonBridge.kt
🪛 detekt (1.23.8)
app/src/main/java/app/gamenative/service/gog/GOGPythonBridge.kt
[warning] 283-283: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 44-44: String.format("%.1f%%", percent) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
⏰ 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/service/gog/GOGPythonBridge.kt (2)
101-155: LGTM! Well-structured Python command execution.The implementation properly manages Python resources with systematic cleanup of sys.argv and stdout in finally blocks, and uses appropriate Result types for error handling.
283-284: CancellationException handling is appropriate here.While detekt flags this as a swallowed exception, catching
CancellationExceptionhere is correct. This is a background estimator job that should gracefully stop when the parent coroutine completes or the real callback takes over. The debug log provides sufficient observability.
| val estimatorJob = CoroutineScope(Dispatchers.IO).launch { | ||
| estimateProgress(downloadInfo) | ||
| } |
There was a problem hiding this comment.
Use current coroutine scope to prevent lifecycle issues.
Creating a new CoroutineScope(Dispatchers.IO) produces an independent scope that may outlive the parent coroutine. If the parent is cancelled, the estimator job continues running. Use the current coroutine's scope instead.
🔎 Proposed fix
- // Start a simple progress estimator in case callback doesn't work
- val estimatorJob = CoroutineScope(Dispatchers.IO).launch {
- estimateProgress(downloadInfo)
- }
+ // Start a simple progress estimator in case callback doesn't work
+ val estimatorJob = launch {
+ estimateProgress(downloadInfo)
+ }Since we're already in withContext(Dispatchers.IO), the launched coroutine will inherit the correct dispatcher and scope.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/gog/GOGPythonBridge.kt around lines
198 to 200, the code creates an independent CoroutineScope(Dispatchers.IO) for
the estimator job which can outlive the parent coroutine; replace that with
launching in the current coroutine scope so cancellation and lifecycle are
propagated (e.g., use the current scope or simply call launch {
estimateProgress(downloadInfo) } while already inside
withContext(Dispatchers.IO) or use coroutineScope/launch from kotlinx.coroutines
to inherit the parent context and dispatcher).
There was a problem hiding this comment.
Will do in a follow-up branch
There was a problem hiding this comment.
@phobos665, sounds good! I appreciate you acknowledging this. The coroutine lifecycle concern can definitely be addressed in a follow-up.
If you'd like, I can create a GitHub issue to track this fix so it doesn't get lost. Just let me know!
🧠 Learnings used
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.
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.
…fixed an issue with the .exe not being applied correctly in the GOGManager for the getWineStartCommand.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/src/main/java/app/gamenative/service/gog/GOGManager.kt (2)
156-160: Missingreturn@withContextin catch block.Line 158 evaluates
Result.failure(e)but doesn't return it. The function will returnUnitimplicitly instead of the failure result, causing silent failures.🔎 Proposed fix
} catch (e: Exception) { Timber.e(e, "Failed to sync GOG library in background") - Result.failure(e) + return@withContext Result.failure(e) }
506-509: Dead code:resultis non-nullable.The
Result<String>type is non-nullable, soif(result == null)on line 506 is always false. This block is unreachable dead code. The failure case is already handled at lines 500-502.🔎 Proposed fix
val output = result.getOrNull() ?: "" - if(result == null) { - Timber.w("Game $gameId not found in GOG library") - return Result.success(null) - } - val gameDetails = org.json.JSONObject(output.trim())
🧹 Nitpick comments (6)
app/src/main/java/app/gamenative/service/gog/GOGManager.kt (6)
13-14: Unused import:SteamAppThe import
app.gamenative.data.SteamAppon line 13 doesn't appear to be used anywhere in this file. Consider removing it to keep imports clean.🔎 Proposed fix
import app.gamenative.data.LaunchInfo import app.gamenative.data.LibraryItem import app.gamenative.data.PostSyncInfo -import app.gamenative.data.SteamApp import app.gamenative.data.GameSource
20-27: Remove unused imports.Several imports appear unused in this file:
AppType,ControllerSupport,OS,ReleaseState,SyncResult, andEnumSet(line 37). Removing these will clean up the file.
177-177: Usevalinstead ofvarfor immutable reference.
gameIdListis never reassigned after initialization.🔎 Proposed fix
- var gameIdList = listGameIds(context) + val gameIdList = listGameIds(context)
596-596: Variable shadowing:gamedeclared twice.Line 596 declares
var gamewhich shadows thegamevariable from line 555. While functionally correct, this reduces readability. Consider using a different name likeexistingGameorupdatedGameEntry.🔎 Proposed fix
// Update or create database entry - var game = getGameById(gameId) - if (game != null) { + var existingGame = getGameById(gameId) + if (existingGame != null) { // Game exists - update install status Timber.d("Updating existing game install status: isInstalled=true, installPath=$installPath, installSize=$diskSize") - val updatedGame = game.copy( + val updatedGame = existingGame.copy( isInstalled = true, installPath = installPath, installSize = diskSize ) updateGame(updatedGame) - Timber.i("Updated GOG game install status in database for ${game.title}") + Timber.i("Updated GOG game install status in database for ${existingGame.title}") } else {
1477-1488: Consider synchronization for file I/O.
saveTimestampsToDiskcan be called concurrently from multiplesetSyncTimestampinvocations. While theConcurrentHashMapitself is thread-safe, concurrent file writes could cause issues. Consider using a mutex or debouncing writes.🔎 Example using a coroutine mutex
private val timestampFileMutex = kotlinx.coroutines.sync.Mutex() private suspend fun saveTimestampsToDisk() { timestampFileMutex.withLock { try { val json = org.json.JSONObject() syncTimestamps.forEach { (key, value) -> json.put(key, value) } timestampFile.writeText(json.toString()) Timber.tag("GOG").d("[Cloud Saves] Saved ${syncTimestamps.size} timestamps to disk") } catch (e: Exception) { Timber.tag("GOG").e(e, "[Cloud Saves] Failed to save timestamps to disk") } } }Note: This would require
setSyncTimestampto become a suspend function or useCoroutineScope.launch.
44-45: Unused imports:Flowandlaunch.
kotlinx.coroutines.flow.Flowandkotlinx.coroutines.launchare imported but not used in this file.🔎 Proposed fix
import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/MainActivity.ktapp/src/main/java/app/gamenative/service/gog/GOGManager.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/MainActivity.kt
🧰 Additional context used
🧠 Learnings (5)
📚 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/GOGManager.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/service/gog/GOGManager.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/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/GOGManager.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/GOGManager.kt
🧬 Code graph analysis (1)
app/src/main/java/app/gamenative/service/gog/GOGManager.kt (2)
app/src/main/java/app/gamenative/service/gog/GOGService.kt (3)
refreshSingleGame(338-341)verifyInstallation(265-268)getInstalledExe(271-274)app/src/main/java/app/gamenative/service/gog/GOGConstants.kt (1)
getGameInstallPath(72-76)
⏰ 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/service/gog/GOGManager.kt (6)
733-757: Acknowledged:runBlockingusage deferred for broader refactor.The
runBlockingcalls at lines 745 and 749 were discussed in previous reviews. As agreed, this refactor is deferred to a separate PR that will address both GOG and Steam services holistically.
862-896: Good error handling with Result type.The
getMainExecutableFromGOGInfofunction properly usesResult<String>for error handling, addressing the previous review feedback. The logic correctly handles the GOG info file parsing and executable discovery.
960-971: Good: Exception handling forrelativeTo()added.The
try-catchblock at lines 963-968 properly handlesIllegalArgumentExceptionthat can occur when paths don't share a common ancestor, as suggested in the previous review.
1091-1160: Good: HTTP response properly wrapped in.use{}.The OkHttp response is now properly wrapped in
.use {}block ensuring it's closed in all code paths, addressing the resource leak concern from the previous review.
1296-1322: Good: Nested HTTP call with proper resource management.The build metadata fetch correctly uses
.use {}for the response (line 1297), and the returnedmanifestLinkis used in a subsequent request. This pattern properly manages the response lifecycle.
1494-1508: Function logic is correct.The path resolution functions properly delegate to
GOGConstants.getGameInstallPath()for consistent path handling. TherunBlockingusage is part of the deferred refactor discussed in past reviews.
… into gog-integration
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @app/src/main/java/app/gamenative/service/gog/GOGService.kt:
- Around line 135-167: The logout function currently returns Result.failure when
getInstance() is null, preventing credential and DB cleanup if the service isn't
running; modify logout (suspend fun logout) to proceed even when getInstance()
is null by: remove the early return on null instance, still call
clearStoredCredentials(context), and when instance is null skip
instance.gogManager.deleteAllGames() and stop() (or start the service first if
you prefer) — if you choose to start the service internally, call getInstance()
after starting and then perform deleteAllGames() and stop(); always return
Result.success(Unit) on completed cleanup and only return Result.failure on
unexpected exceptions (preserve the try/catch and Timber logging).
In
@app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.kt:
- Around line 50-65: The companion object’s installDialogStates map
(installDialogStates, showInstallDialog, hideInstallDialog,
getInstallDialogState) can leak entries for the app lifetime; fix by ensuring
entries are removed when a screen is disposed — add a DisposableEffect tied to
the screen’s appId (e.g., in the Content composable where libraryItem.appId is
available) that calls hideInstallDialog(appId) in onDispose, or alternatively
wire a lifecycle-aware observer to remove the map entry when the associated
screen is destroyed; this guarantees automatic cleanup of stale dialog state.
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/appscreen/BaseAppScreen.kt (1)
662-667: Remove redundant delay and manual state refresh; event listeners already handle updates.The
onDownloadInstallClickandonPauseResumeClickcallbacks include 100ms delays followed by manualperformStateRefreshcalls. This is unnecessary because the established event-based architecture already handles state updates automatically:
- When
onDownloadInstallClicktriggersGOGService.downloadApp(), it emits aDownloadStatusChangedevent (GOGManager lines 571, 642, 657, 667).- The
observeGameStatesetup (BaseAppScreen lines 634–650) listens for this event and automatically callsonStateChanged(), which invokesrequestStateRefresh(true).- State refreshes reactively without manual delays or explicit calls.
For comparison,
onUpdateClick(line 681–683) callsperformStateRefresh(true)directly without a delay, andSteamAppScreenrelies entirely on event listeners for state updates—no manual delays. The delays in these two callbacks are artifacts that contradict the reactive pattern and should be removed.
♻️ Duplicate comments (2)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.kt (1)
669-674: Same delay anti-pattern asonDownloadInstallClick.The 100ms delay here has the same issues as in
onDownloadInstallClick(lines 662-667). Please apply the event-based refresh approach consistently across all download-related callbacks.Based on past review feedback and architectural consistency.
app/src/main/java/app/gamenative/service/gog/GOGManager.kt (1)
489-523: Remove unreachableif (result == null)branch inrefreshSingleGame.
resultis a non-nullResult<String>and failures are already handled via theisFailurecheck; theif (result == null)block is dead and can be dropped to simplify the flow.
🧹 Nitpick comments (5)
app/src/main/java/app/gamenative/utils/ContainerUtils.kt (1)
938-972: Remove or clarify the "legacy GOG format" comment.The comment at line 940 states that numeric-only IDs like
19283103represent a "legacy GOG format," butextractGameSourceFromContainerId(line 971) defaults toGameSource.STEAMfor unrecognized formats. This creates confusion about how numeric-only IDs are actually handled.Based on past review discussions indicating "there are no old GOG containers," the legacy format reference should be removed:
🔎 Proposed fix to clarify the comment
/** * Extracts the game ID from a container ID string * Handles formats like: * - STEAM_123456 -> 123456 * - CUSTOM_GAME_571969840 -> 571969840 * - GOG_19283103 -> 19283103 * - STEAM_123456(1) -> 123456 - * - 19283103 -> 19283103 (legacy GOG format) */ fun extractGameIdFromContainerId(containerId: String): Int {Alternatively, if numeric-only IDs should be treated as GOG sources, update
extractGameSourceFromContainerIdto handle them explicitly.app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.kt (1)
626-631: Refactor: abstract downloadInfo retrieval to preserve base class abstraction.The
whenblock directly callsSteamService.getAppDownloadInfoandGOGService.getDownloadInfo, couplingBaseAppScreento specific service implementations. This breaks the abstraction layer sinceBaseAppScreenshould remain source-agnostic.Consider introducing an abstract method like
getDownloadInfo(context: Context, libraryItem: LibraryItem): DownloadInfo?that each subclass implements, then call it here:val downloadInfo = getDownloadInfo(context, libraryItem)This keeps the base class decoupled from concrete services and aligns with the existing pattern of abstract methods for source-specific behavior.
app/src/main/java/app/gamenative/service/gog/GOGManager.kt (1)
1418-1488: Consider serializing writes togog_sync_timestamps.jsonto avoid races.
setSyncTimestampcan be called from multiple concurrent syncs, andsaveTimestampsToDiskwrites the same JSON file without coordination. WhileConcurrentHashMapavoids crashes in-memory, concurrentwriteTextcalls could interleave and corrupt the on-disk JSON. A simple mutex or single-writer coroutine for timestamp persistence would make this more robust.app/src/main/java/app/gamenative/ui/PluviaMain.kt (2)
406-411: Add import for GOGService to improve readability.The fully qualified class name
app.gamenative.service.gog.GOGServiceis used instead of importing the class. This is inconsistent with the rest of the file (e.g.,SteamServiceis imported at line 53) and reduces readability.🔎 Proposed fix
Add the import at the top of the file:
+import app.gamenative.service.gog.GOGServiceThen simplify the code:
// Start GOGService if user has GOG credentials -if (app.gamenative.service.gog.GOGService.hasStoredCredentials(context) && - !app.gamenative.service.gog.GOGService.isRunning) { +if (GOGService.hasStoredCredentials(context) && + !GOGService.isRunning) { Timber.d("[PluviaMain]: Starting GOGService for logged-in user") - app.gamenative.service.gog.GOGService.start(context) + GOGService.start(context) }
1155-1178: Consider extracting the action constant and adding GOGService import.Two minor improvements for consistency and maintainability:
- The fully qualified class name
app.gamenative.service.gog.GOGServiceis used again (same issue as lines 406-411).- The hardcoded string
"download"forpreferredAction(line 1165) would benefit from being a named constant, either defined inGOGServiceorGOGConstants.The logic itself is sound: GOG games get cloud save sync before launch, failures don't block the launch (consistent with the PR's pragmatic approach), and the early return correctly bypasses Steam cloud logic.
🔎 Proposed improvements
After adding the import (see previous comment), simplify line 1162:
-val syncSuccess = app.gamenative.service.gog.GOGService.syncCloudSaves( +val syncSuccess = GOGService.syncCloudSaves( context = context, appId = appId, preferredAction = "download" )If
GOGServiceorGOGConstantsdefines action constants, use them:// Example, verify actual constant name in GOGService/GOGConstants preferredAction = GOGService.ACTION_DOWNLOAD // or GOGConstants.CLOUD_ACTION_DOWNLOAD
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/build.gradle.ktsapp/src/main/java/app/gamenative/service/gog/GOGManager.ktapp/src/main/java/app/gamenative/service/gog/GOGService.ktapp/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/ui/model/MainViewModel.ktapp/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt
- app/build.gradle.kts
🧰 Additional context used
🧠 Learnings (6)
📚 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/ui/screen/library/appscreen/BaseAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.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/ui/model/MainViewModel.ktapp/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/service/gog/GOGManager.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/PluviaMain.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/app/gamenative/service/gog/GOGService.ktapp/src/main/java/app/gamenative/service/gog/GOGManager.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/utils/ContainerUtils.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/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/service/gog/GOGManager.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/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/service/gog/GOGManager.kt
🧬 Code graph analysis (3)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.kt (3)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt (1)
onDeleteDownloadClick(323-347)app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt (1)
onDeleteDownloadClick(476-499)app/src/main/java/app/gamenative/ui/screen/library/appscreen/CustomGameAppScreen.kt (1)
onDeleteDownloadClick(223-226)
app/src/main/java/app/gamenative/ui/PluviaMain.kt (1)
app/src/main/java/app/gamenative/ui/model/MainViewModel.kt (1)
setLoadingDialogVisible(182-184)
app/src/main/java/app/gamenative/service/gog/GOGService.kt (1)
app/src/main/java/app/gamenative/service/gog/GOGAuthManager.kt (1)
clearStoredCredentials(261-273)
⏰ 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 (8)
app/src/main/java/app/gamenative/utils/ContainerUtils.kt (2)
532-547: No action required. Line 535 correctly callsGOGService.getGOGGameOf()without an additional blocking wrapper becausegetGOGGameOfis already a synchronous function that internally usesrunBlocking(Dispatchers.IO). The implementation pattern is correct and does not present an ANR risk.Likely an incorrect or invalid review comment.
830-874: Remove redundant runBlocking wrapper around GOGService.getGOGGameOf().Line 833 unnecessarily wraps
GOGService.getGOGGameOf()inrunBlocking. This function is already synchronous and internally handles blocking operations. All other call sites in the codebase (GOGAppScreen.kt, earlier ContainerUtils code) call it directly without wrapping. Remove therunBlockingwrapper to match the established pattern:val game = GOGService.getGOGGameOf(gameId.toString())Likely an incorrect or invalid review comment.
app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.kt (1)
676-678: LGTM: improved readability.The reformatting to a multi-line block improves code readability without changing logic.
app/src/main/java/app/gamenative/ui/model/MainViewModel.kt (1)
274-352: GOG exit/cloud-sync wiring looks solid and non-blocking.The new GOG branch in
exitSteamAppcorrectly pushes cloud-save upload to a background coroutine and keeps the existing SteamcloseApp+ feedback flow intact, with Steam-only feedback gating for now. No functional issues stand out here.app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
1665-1813: GOG launch path integration intogetWineStartCommandlooks correct.The GOG branch cleanly detects
GameSource.GOG, delegates command construction toGOGService.getGogWineStartCommand, and wraps it withwinhandler.exein the same pattern as Steam/custom paths, without impacting existing Steam behavior.app/src/main/java/app/gamenative/service/gog/GOGManager.kt (1)
898-983: GOG Wine start command flow looks consistent with container/drive mapping.
getGogWineStartCommandverifies installation, falls back safely to"explorer.exe"on errors, resolves the drive letter from container drives, computes a relative Windows path from the install root, and setsworkingDir/WINEPATHappropriately. This should integrate cleanly with the XServer wrapper.app/src/main/java/app/gamenative/service/gog/GOGService.kt (2)
298-335: Download lifecycle handling and cleanup look correct.
downloadGameregisters theDownloadInfoinactiveDownloads, runs the actual work on the service scope, and always removes the entry infinally, sohasActiveDownload()and callers won’t see stale state even on failure or exception.
359-516: Cloud-save sync locking and error handling are robust.Using
gogManager.startSync/endSyncperappIdprevents concurrent syncs for the same title, and the function cleanly bails out on missing service, auth, game, or save locations while still releasing the lock infinally. The detailed before/after directory logging should also make field debugging much easier.
app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
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/xserver/XServerScreen.kt (1)
1770-1809: EnsuregameIdnull safety in Steam launch paths.The
gameIdis used on lines 1778, 1784, and 1789 without null checks. These uses are in the Steam game launch logic (theelsebranch starting at line 1770). If the null check is added at line 1685 as suggested, ensure it covers all Steam-specific code paths, or add an additional check here.🔎 Suggested fix
} else if (appLaunchInfo == null) { // For Steam games, we need appLaunchInfo Timber.tag("XServerScreen").w("appLaunchInfo is null for Steam game: $appId") "\"wfm.exe\"" } else { + if (gameId == null) { + Timber.tag("XServerScreen").e("Invalid gameId for Steam game: $appId") + return "winhandler.exe \"wfm.exe\"" + } + if (container.isLaunchRealSteam()) { // Launch Steam with the applaunch parameter to start the game "\"C:\\\\Program Files (x86)\\\\Steam\\\\steam.exe\" -silent -vgui -tcp " + "-nobigpicture -nofriendsui -nochatui -nointro -applaunch $gameId"
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
1685-1695: Add null safety check forgameIdbefore Steam operations.The
gameIdvariable is used on lines 1688 and 1693 without null checks, butContainerUtils.extractGameIdFromContainerId(appId)can return null. This could cause a crash if a malformedappIdis passed.🔎 Proposed null safety fix
val gameId = ContainerUtils.extractGameIdFromContainerId(appId) if (!isCustomGame && !isGOGGame) { // Steam-specific setup + if (gameId == null) { + Timber.tag("XServerScreen").e("Invalid gameId for Steam game: $appId") + return "winhandler.exe \"wfm.exe\"" + } + if (container.executablePath.isEmpty()){ container.executablePath = SteamService.getInstalledExe(gameId) container.saveData() } if (!container.isUseLegacyDRM){ // Create ColdClientLoader.ini file SteamUtils.writeColdClientIni(gameId, container) } }Additionally, consider refactoring the conditional to use a positive check for clarity:
💡 Optional: Refactor to use isSteamGame
- if (!isCustomGame && !isGOGGame) { - // Steam-specific setup + val isSteamGame = gameSource == GameSource.STEAM + if (isSteamGame) { + // Steam-specific setup if (gameId == null) { Timber.tag("XServerScreen").e("Invalid gameId for Steam game: $appId") return "winhandler.exe \"wfm.exe\"" }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
🧰 Additional context used
🧠 Learnings (4)
📚 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/ui/screen/xserver/XServerScreen.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/ui/screen/xserver/XServerScreen.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/ui/screen/xserver/XServerScreen.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/ui/screen/xserver/XServerScreen.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/ui/screen/xserver/XServerScreen.kt (5)
59-59: LGTM!The import additions for
LibraryItemandGOGServiceare necessary for the GOG integration functionality implemented in this file.Also applies to: 64-64
1679-1683: Game source extraction logic is clear.The extraction of
gameSource,isCustomGame,isGOGGame, andgameIdis well-structured. However, note thatgameIdcan be null and requires null safety checks before use (addressed in a separate comment below).
1701-1723: GOG launch logic looks good.The GOG game launch implementation follows a clear pattern:
- Proper game type detection with
isGOGGame- Appropriate logging for debugging
- Creates a minimal
LibraryItemfor the GOG service call- Delegates command construction to
GOGService- Early return prevents fall-through
The empty
namefield in theLibraryItem(line 1708) is intentional as noted in the comment.
1517-1518: Call site updated correctly.The
getWineStartCommandcall has been correctly updated to pass thecontextparameter, matching the new function signature. The necessity of thecontextparameter itself is addressed in a separate comment.
1664-1673: Thecontextparameter is actually necessary and being used.The
contextparameter is passed to bothgetGameInstallPath()(line 924) andgetInstalledExe()(line 937) in the GOGManager implementation, making it essential for the function to work correctly. The parameter should be retained.
…hly with users to understand how it should work. In a follow-up we should give them a notice on the front-end to ask them for their decision on what should happen.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/ui/PluviaMain.kt (1)
406-411: Consider importing GOGService for improved readability.The fully qualified class name
app.gamenative.service.gog.GOGServiceis used throughout. While functional, this reduces readability. Consider adding an import statement at the top of the file:import app.gamenative.service.gog.GOGServiceThen reference it simply as
GOGServicein the code.🔎 Proposed refactor
Add import at the top with other service imports:
import app.gamenative.service.SteamService +import app.gamenative.service.gog.GOGServiceThen simplify the usage:
- if (app.gamenative.service.gog.GOGService.hasStoredCredentials(context) && - !app.gamenative.service.gog.GOGService.isRunning) { + if (GOGService.hasStoredCredentials(context) && + !GOGService.isRunning) { Timber.d("[PluviaMain]: Starting GOGService for logged-in user") - app.gamenative.service.gog.GOGService.start(context) + GOGService.start(context) }app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt (1)
333-355: Consider parallelizing metadata calculation for better performance.The metadata calculation (line 352) processes files sequentially, which could be slow for games with many save files. Parallelizing with
async/awaitwould improve performance.🔎 Proposed optimization
scanRecursive(directory, directory.absolutePath) - // Calculate metadata for all files - files.forEach { it.calculateMetadata() } + // Calculate metadata for all files in parallel + coroutineScope { + files.map { file -> + async { file.calculateMetadata() } + }.awaitAll() + } files
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.ktapp/src/main/java/app/gamenative/ui/PluviaMain.kt
🧰 Additional context used
🧠 Learnings (2)
📚 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/GOGCloudSavesManager.ktapp/src/main/java/app/gamenative/ui/PluviaMain.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/PluviaMain.kt
🧬 Code graph analysis (2)
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt (2)
app/src/main/python/gogdl/dl/objects/v1.py (1)
File(36-49)app/src/main/python/gogdl/saves.py (2)
SyncFile(28-53)SyncClassifier(325-365)
app/src/main/java/app/gamenative/ui/PluviaMain.kt (1)
app/src/main/java/app/gamenative/ui/model/MainViewModel.kt (1)
setLoadingDialogVisible(182-184)
🪛 detekt (1.23.8)
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt
[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 (3)
app/src/main/java/app/gamenative/ui/PluviaMain.kt (1)
1167-1172: Verify that silent sync failure is the intended user experience.When GOG cloud save sync fails, only a warning is logged but no user-facing dialog is shown. This differs from the Steam cloud sync flow (lines 1194-1260) which presents detailed error dialogs for various failure scenarios.
Confirm this is intentional design:
- If intentional: Document why GOG failures are handled differently (e.g., GOG cloud saves are best-effort)
- If unintentional: Consider showing a dismissible notification to inform users their cloud saves couldn't sync, giving them the option to retry or continue
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt (2)
140-328: LGTM! Well-structured sync orchestration.The sync logic is comprehensive and handles multiple scenarios correctly:
- Simple cases (empty local/cloud) are handled first with early returns
- Preferred actions are respected
- Complex conflict resolution uses timestamp comparison
The extensive logging will be helpful for debugging cloud saves issues.
360-438: LGTM! Proper resource management.The response is correctly wrapped in
.use {}block, ensuring proper cleanup. The dirname filtering logic correctly identifies files belonging to the specified save location.
| // Calculate MD5 of gzipped content (matching Python implementation) | ||
| FileInputStream(file).use { fis -> | ||
| val digest = MessageDigest.getInstance("MD5") | ||
| val buffer = java.io.ByteArrayOutputStream() | ||
|
|
||
| GZIPOutputStream(buffer).use { gzipOut -> | ||
| val fileBuffer = ByteArray(8192) | ||
| var bytesRead: Int | ||
| while (fis.read(fileBuffer).also { bytesRead = it } != -1) { | ||
| gzipOut.write(fileBuffer, 0, bytesRead) | ||
| } | ||
| } | ||
|
|
||
| md5Hash = digest.digest(buffer.toByteArray()) | ||
| .joinToString("") { "%02x".format(it) } | ||
| } |
There was a problem hiding this comment.
Critical: MD5 hash mismatch with Python implementation due to gzip timestamp.
The GZIPOutputStream includes a modification timestamp in the gzip header by default, while the Python implementation uses gzip.compress(..., mtime=0) which suppresses the timestamp. This causes different compressed bytes and thus different MD5 hashes, breaking cloud saves synchronization between Kotlin and Python implementations.
🔎 Proposed fix to suppress gzip timestamp
Unfortunately, Java's GZIPOutputStream doesn't provide a direct way to set mtime=0. You'll need to either:
- Use a custom GZIP implementation or library that allows setting mtime to 0
- Match the Python approach by compressing the entire file at once with a library that supports mtime control
- Write the GZIP header manually with mtime=0, then use
DeflaterOutputStream
Here's an example using the third approach:
- // Calculate MD5 of gzipped content (matching Python implementation)
- FileInputStream(file).use { fis ->
- val digest = MessageDigest.getInstance("MD5")
- val buffer = java.io.ByteArrayOutputStream()
-
- GZIPOutputStream(buffer).use { gzipOut ->
- val fileBuffer = ByteArray(8192)
- var bytesRead: Int
- while (fis.read(fileBuffer).also { bytesRead = it } != -1) {
- gzipOut.write(fileBuffer, 0, bytesRead)
- }
- }
-
- md5Hash = digest.digest(buffer.toByteArray())
- .joinToString("") { "%02x".format(it) }
- }
+ // Calculate MD5 of gzipped content with mtime=0 (matching Python implementation)
+ FileInputStream(file).use { fis ->
+ val digest = MessageDigest.getInstance("MD5")
+ val buffer = java.io.ByteArrayOutputStream()
+
+ // Write GZIP header with mtime=0 to match Python's gzip.compress(..., mtime=0)
+ buffer.write(byteArrayOf(
+ 0x1f, 0x8b.toByte(), // Magic number
+ 0x08, // Compression method (DEFLATE)
+ 0x00, // Flags
+ 0x00, 0x00, 0x00, 0x00, // mtime = 0
+ 0x00, // Extra flags
+ 0xff.toByte() // OS (unknown)
+ ))
+
+ // Use DeflaterOutputStream for compression
+ val deflater = java.util.zip.Deflater(6, true) // level 6, nowrap=true for raw DEFLATE
+ java.util.zip.DeflaterOutputStream(buffer, deflater).use { deflaterOut ->
+ val fileBuffer = ByteArray(8192)
+ var bytesRead: Int
+ while (fis.read(fileBuffer).also { bytesRead = it } != -1) {
+ deflaterOut.write(fileBuffer, 0, bytesRead)
+ }
+ }
+
+ // Write CRC32 and uncompressed size (required by GZIP format)
+ val crc = java.util.zip.CRC32()
+ FileInputStream(file).use { crcFis ->
+ val fileBuffer = ByteArray(8192)
+ var bytesRead: Int
+ while (crcFis.read(fileBuffer).also { bytesRead = it } != -1) {
+ crc.update(fileBuffer, 0, bytesRead)
+ }
+ }
+ val crcValue = crc.value.toInt()
+ val fileSize = file.length().toInt()
+ buffer.write(byteArrayOf(
+ (crcValue and 0xff).toByte(),
+ ((crcValue shr 8) and 0xff).toByte(),
+ ((crcValue shr 16) and 0xff).toByte(),
+ ((crcValue shr 24) and 0xff).toByte(),
+ (fileSize and 0xff).toByte(),
+ ((fileSize shr 8) and 0xff).toByte(),
+ ((fileSize shr 16) and 0xff).toByte(),
+ ((fileSize shr 24) and 0xff).toByte()
+ ))
+
+ md5Hash = digest.digest(buffer.toByteArray())
+ .joinToString("") { "%02x".format(it) }
+ }Note: This implementation requires reading the file twice (once for compression, once for CRC32). For better performance, you could compute both in a single pass with a custom approach.
There was a problem hiding this comment.
So this issue hasn't materialised for me, so we'll keep an eye on it and potentially resolve in a follow-up if it does occur.
There was a problem hiding this comment.
@phobos665, understood! It's reasonable to defer this until it becomes a practical issue. Since your testing hasn't revealed any synchronization problems, the gzip timestamp differences might not be causing hash mismatches in your actual workflows.
The concern is documented here if you encounter cloud saves sync issues between Kotlin and Python implementations in the future. Feel free to revisit this if you observe unexpected behavior with cloud saves not syncing correctly or showing false conflicts.
Thanks for testing thoroughly!
🧠 Learnings used
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.
|
Merged in #393 |
|
Closing this as we have now merged in GOG in an updated branch that is based on this one. |
4 participants
This new PR is based on #145, So massive shoutout to the hard work UnbelievableFlavour did on that.
Features:
Future Features (We should do afterwards in a new PR):
Currently the UI for logging in is inside the Profile Settings (SettingsGroupInterface). If we do not want to expose users to this right now, we should hide this for now and we can expose it once we've fixed the Smart Auth Redirect Flow.
Let's break down the architecture a little:
We have:
Python files within python/gogdl -> These are python files that allow us to interact with GOG servers in a commandline fashion. (There's a lot of files, but ultimately we're just importing that whole project until we can have a more native solution).
Data-related files: GOGGame, LibraryItem, GOGConverter, GOGGameDao -> These are the data related files for parsing/converting and storing/retrieving (CRUD) on GOG games.
Adjustments to ContainerUtils, PluviaMain, LibraryState Pref Manager etc- > We need to essentially add in extra clauses to several switch statements to be treated as a new gameSource. similar to how we did it with CustomGames.
GOGAuthManager -> Handles all auth-related functionality such as logging in and handling sessions
GOGConstants -> keeps track of all constants for GOG
GOGManager -> Library/Game related functionality
GOGPythonBridge -> Abstraction for executing commands to Python/GOGDL to reduce footprint and to abstract out implementation logic
GOGService -> Facade pattern Interface for interacting with GOG-related functionality and to keep an open service so we're able to run asynchronous operations such as syncing library or handling sessions.
Note: The Python files are mostly unchanged from PR #145 except from 2 new functions to get the IDs and get details for a singular game.
**Long-term, we should move away from GOGDL with our own Kotlin implementation in order to optimise the performance. **
Summary by CodeRabbit
New Features
Platform
UI
Localization
Chores
✏️ Tip: You can customize this high-level summary in your review settings.