fix: GOG cloud save fetch failure handling#1201
Open
kiequoo wants to merge 3 commits intoutkarshdalal:masterfrom
Open
fix: GOG cloud save fetch failure handling#1201kiequoo wants to merge 3 commits intoutkarshdalal:masterfrom
kiequoo wants to merge 3 commits intoutkarshdalal:masterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughGOG cloud saves handling is refactored to explicitly propagate failures as null values rather than silently using defaults. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kiequoo
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt (1)
415-451: Consider wrapping the loop to consistently returnnullon malformed items.The function explicitly returns
nullwhenJSONArray(responseBody)fails (line 420), but if an array element isn't a JSON object,getJSONObject(i)at line 427 throws an uncaught exception. While this is caught by the caller ingetCloudFiles, it's inconsistent with the explicitnullhandling and could cause issues if thisinternalfunction is called from other contexts (e.g., tests).♻️ Suggested refactor to handle malformed array elements
val files = mutableListOf<CloudFile>() for (i in 0 until items.length()) { - val fileObj = items.getJSONObject(i) + val fileObj = try { + items.getJSONObject(i) + } catch (e: Exception) { + Timber.tag("GOG").w("[Cloud Saves] Skipping malformed item at index $i") + continue + } val name = fileObj.optString("name", "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt` around lines 415 - 451, The loop in parseCloudFilesResponse uses items.getJSONObject(i) which will throw for non-object elements, making error handling inconsistent with the earlier JSON parse that returns null; update parseCloudFilesResponse to guard each element access by wrapping items.getJSONObject(i) (and subsequent per-item parsing) in a try/catch that logs the error (Timber.tag("GOG").e(...)) and returns null on any malformed item so the function consistently returns null for malformed responses (keep references to parseCloudTimestamp, CloudFile construction, dirname check and existing log messages intact), and note that callers like getCloudFiles will then continue to receive null for invalid arrays.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt`:
- Around line 415-451: The loop in parseCloudFilesResponse uses
items.getJSONObject(i) which will throw for non-object elements, making error
handling inconsistent with the earlier JSON parse that returns null; update
parseCloudFilesResponse to guard each element access by wrapping
items.getJSONObject(i) (and subsequent per-item parsing) in a try/catch that
logs the error (Timber.tag("GOG").e(...)) and returns null on any malformed item
so the function consistently returns null for malformed responses (keep
references to parseCloudTimestamp, CloudFile construction, dirname check and
existing log messages intact), and note that callers like getCloudFiles will
then continue to receive null for invalid arrays.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f37d95f-ccc0-4d57-a6e5-258d4cd8a7f3
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.ktapp/src/main/java/app/gamenative/service/gog/GOGManager.ktapp/src/test/java/app/gamenative/service/gog/GOGCloudSavesManagerTest.kt
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
|
Please make sure to use the PR template :) |
phobos665
reviewed
Apr 13, 2026
Comment on lines
+411
to
+460
| null | ||
| } | ||
| } | ||
|
|
||
| internal fun parseCloudFilesResponse(responseBody: String, dirname: String): List<CloudFile>? { | ||
| val items = try { | ||
| JSONArray(responseBody) | ||
| } catch (e: Exception) { | ||
| Timber.tag("GOG").e(e, "[Cloud Saves] Failed to parse JSON array response") | ||
| return null | ||
| } | ||
|
|
||
| Timber.tag("GOG").d("[Cloud Saves] Found ${items.length()} total items in cloud storage") | ||
|
|
||
| val files = mutableListOf<CloudFile>() | ||
| for (i in 0 until items.length()) { | ||
| val fileObj = items.getJSONObject(i) | ||
| val name = fileObj.optString("name", "") | ||
| val hash = fileObj.optString("hash", "") | ||
| val lastModified = fileObj.optString("last_modified") | ||
|
|
||
| Timber.tag("GOG").d("[Cloud Saves] Examining item $i: name='$name', dirname='$dirname'") | ||
|
|
||
| if (name.isNotEmpty() && hash.isNotEmpty() && name.startsWith("$dirname/")) { | ||
| val relativePath = name.removePrefix("$dirname/") | ||
| files.add( | ||
| CloudFile( | ||
| relativePath = relativePath, | ||
| md5Hash = hash, | ||
| updateTime = lastModified, | ||
| updateTimestamp = parseCloudTimestamp(lastModified), | ||
| ), | ||
| ) | ||
| Timber.tag("GOG").d("[Cloud Saves] ✓ Matched: relativePath='$relativePath'") | ||
| } else { | ||
| Timber.tag("GOG").d("[Cloud Saves] ✗ Skipped (doesn't match dirname or missing data)") | ||
| } | ||
| } | ||
|
|
||
| return files | ||
| } | ||
|
|
||
| internal fun parseCloudTimestamp(lastModified: String): Long? = | ||
| try { | ||
| // GOG returns timestamps like "2026-04-02T20:34:00.123456+00:00". | ||
| OffsetDateTime.parse(lastModified).toInstant().epochSecond | ||
| } catch (_: DateTimeParseException) { | ||
| null | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Nice, I like these being split out into internal files for easy unit testing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is a split-out PR from #1094 focused on the GOG cloud save fetch and timestamp fixes.
This PR fixes two bugs in GOG cloud save sync and one related support issue in save-location resolution.
Before this change,
getCloudFiles()treated request or parse failures the same as a valid empty cloud result. That could make sync logic believe the cloud was empty and incorrectly choose an upload path. It also parsed GOGlast_modifiedtimestamps withInstant.parse(...), which does not handle the offset-based format GOG returns, so remote timestamps could be dropped during sync decisions.It also removes the fake default GOG cloud-save path fallback when the API returns no save locations. That fallback is not useful, because in the same case we also have no
clientSecret, so cloud auth cannot succeed and the path can never be used for a real sync.Changes
nullfrom GOG cloud file listing on HTTP or parse failure instead ofemptyList()cloud is emptyOffsetDateTime.parse(...).toInstant()Summary by CodeRabbit
Bug Fixes
Tests