feat: add GOG cloud save conflict resolution#1273
Draft
kiequoo wants to merge 3 commits into
Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a GOG cloud-save pre-flight check: a new read-only Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant PluviaMain
participant GOGService
participant GOGManager
participant GOGCloudSavesManager
participant Database
User->>PluviaMain: Launch game
PluviaMain->>GOGService: checkPreLaunchConflict(appId)
activate GOGService
GOGService->>GOGManager: checkPreLaunchConflict(appId)
activate GOGManager
GOGManager->>Database: Resolve game & save locations
Database-->>GOGManager: Game + locations
loop per save location
GOGManager->>GOGCloudSavesManager: checkSync(localPath, dirname, clientId, clientSecret, lastSyncTimestamp)
activate GOGCloudSavesManager
GOGCloudSavesManager->>GOGCloudSavesManager: Read local files & mtimes
GOGCloudSavesManager->>GOGCloudSavesManager: List cloud files (API)
GOGCloudSavesManager->>GOGCloudSavesManager: Classify files -> SyncAction
GOGCloudSavesManager->>GOGCloudSavesManager: Compute max local/remote timestamps (ms)
GOGCloudSavesManager-->>GOGManager: SyncCheckResult?/null
deactivate GOGCloudSavesManager
alt Conflict
GOGManager->>GOGManager: Track largest timestamp delta
end
end
GOGManager-->>GOGService: SyncCheckResult? (selected conflict or null)
deactivate GOGManager
GOGService-->>PluviaMain: SyncCheckResult? (conflict or null)
deactivate GOGService
alt Conflict found
PluviaMain->>PluviaMain: Hide loading dialog
PluviaMain->>User: Show SYNC_CONFLICT (localMs, remoteMs)
else No conflict
PluviaMain->>User: Proceed to sync/launch
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/service/gog/GOGService.kt (1)
431-466: Consider returning a named type instead ofPair<Long, Long>.Callers have no type-level guarantee of the
(local, remote)order — a mistaken destructuring would silently swap the timestamps shown in the conflict dialog. A small data class (e.g.,data class ConflictTimestamps(val localMs: Long, val remoteMs: Long)) would make this symmetric pair self-describing, especially as the result flows acrossGOGService→PluviaMain→ the dialog.🤖 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/GOGService.kt` around lines 431 - 466, The function checkPreLaunchConflict currently returns Pair<Long, Long>, which is ambiguous; define a small data class (e.g., data class ConflictTimestamps(val localMs: Long, val remoteMs: Long)) and change the return type of checkPreLaunchConflict to ConflictTimestamps? (or List/optional of that type) so callers get named fields instead of relying on Pair order; update where checkPreLaunchConflict is called (e.g., usages in GOGService → PluviaMain → dialog) to destructure or reference .localMs and .remoteMs accordingly and adjust the construction at the return site to create ConflictTimestamps(bestLocal, bestRemote) (return null if no conflict).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt`:
- Around line 352-382: checkSync is calling scanLocalFiles which always computes
gzipped MD5 via withMetadata() causing heavy I/O/CPU; change scanLocalFiles to
accept a flag (e.g., skipHash: Boolean) or add a lightweight variant that only
fills relativePath and updateTimestamp (no GZIP/MD5) and use that lighter scan
in checkSync; keep the full hashing behavior for callers that need md5 (preserve
withMetadata() and the original scanLocalFiles behavior) and update checkSync to
call the new lightweight scan so classifyFiles still receives relativePath and
updateTimestamp but no unnecessary md5Hash computation.
In `@app/src/main/java/app/gamenative/service/gog/GOGManager.kt`:
- Around line 1172-1187: The sync flow can leak an incomplete
CompletableDeferred if doSyncCloudSavesForApp throws a non-Exception throwable;
change the code that calls doSyncCloudSavesForApp so that endSync(appId, ...) is
invoked from a finally block regardless of what is thrown. Concretely, declare a
mutable result boolean before a try, assign it inside the try (and set false in
the catch for Exception), and call serviceInstance.gogManager.endSync(appId,
result) in finally; reference the existing methods startSync,
doSyncCloudSavesForApp, and endSync to locate the code to update.
In `@app/src/main/java/app/gamenative/service/gog/GOGService.kt`:
- Around line 487-502: The per-app sync lock may never be released if a
non-Exception Throwable escapes or if coroutine cancellation is swallowed; move
the call to serviceInstance.gogManager.endSync(appId, result) into a finally
block so it always runs, ensure result is set appropriately before finally, and
let CancellationException (and other relevant CancellationExceptions) rethrow
instead of being caught — adjust the try/catch around doSyncCloudSavesForApp to
catch only non-cancellation Exceptions and propagate cancellations, using the
existing startSync/appId and endSync(appId, result) APIs on gogManager to always
complete the CompletableDeferred in activeSyncs.
In `@app/src/main/java/app/gamenative/ui/PluviaMain.kt`:
- Around line 1793-1805: The code currently proceeds to launch even if
syncCloudSaves failed; when the user explicitly chose a direction (preferredSave
== Remote or preferredSave == Local) we must abort the launch on sync failure to
respect that choice. Modify the block after calling GOGService.syncCloudSaves so
that if syncSuccess is false AND preferredSave indicates an explicit user choice
(check preferredSave == Remote or preferredSave == Local), log an error and stop
the launch flow (return/exit from the launch path) instead of continuing; only
allow continuation for failure when preferredSave is not an explicit choice
(e.g., Automatic/None). Keep existing log messages for success and non-fatal
failures.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/gog/GOGService.kt`:
- Around line 431-466: The function checkPreLaunchConflict currently returns
Pair<Long, Long>, which is ambiguous; define a small data class (e.g., data
class ConflictTimestamps(val localMs: Long, val remoteMs: Long)) and change the
return type of checkPreLaunchConflict to ConflictTimestamps? (or List/optional
of that type) so callers get named fields instead of relying on Pair order;
update where checkPreLaunchConflict is called (e.g., usages in GOGService →
PluviaMain → dialog) to destructure or reference .localMs and .remoteMs
accordingly and adjust the construction at the return site to create
ConflictTimestamps(bestLocal, bestRemote) (return null if no conflict).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2cbc62b-0c5a-47a9-9164-66049d7b841c
📒 Files selected for processing (7)
app/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/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/appscreen/GOGAppScreen.ktapp/src/test/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreenTest.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt (1)
217-238:⚠️ Potential issue | 🟠 MajorPropagate forced transfer failures before returning success.
For
SaveLocation.Remote/Local,downloadFileanduploadFileonly log failures and returnUnit, so this block can returncurrentTimestamp()even when the chosen conflict resolution did not transfer files. That makesGOGService.syncCloudSavesreport success andpreLaunchAppcan launch despite a failed “Keep Local/Keep Remote” action.Consider making the transfer helpers return
Booleanand returning0Lif any forced transfer fails.🛡️ Sketch
if (preferredSave == SaveLocation.Remote) { if (downloadableCloud.isNotEmpty()) { Timber.tag("GOG-CloudSaves").i("Forcing download of ${downloadableCloud.size} file(s) (user requested)") - downloadableCloud.forEach { file -> - downloadFile(credentials.userId, clientId, dirname, file, syncDir, credentials.accessToken) + val allDownloaded = downloadableCloud.all { file -> + downloadFile(credentials.userId, clientId, dirname, file, syncDir, credentials.accessToken) + } + if (!allDownloaded) { + return@withContext 0L } return@withContext currentTimestamp() } else { // All cloud files are deletion markers; user asked for remote but there's nothing to download. // Fall through to auto-sync so we don't silently ignore the request. Timber.tag("GOG-CloudSaves").w("preferredSave=Remote but no downloadable cloud files; falling through to auto-sync") } } if (preferredSave == SaveLocation.Local && localFiles.isNotEmpty()) { Timber.tag("GOG-CloudSaves").i("Forcing upload of ${localFiles.size} file(s) (user requested)") - localFiles.forEach { file -> - uploadFile(credentials.userId, clientId, dirname, file, credentials.accessToken) + val allUploaded = localFiles.all { file -> + uploadFile(credentials.userId, clientId, dirname, file, credentials.accessToken) + } + if (!allUploaded) { + return@withContext 0L } return@withContext currentTimestamp() }This also requires changing
uploadFile(...)anddownloadFile(...)to returnBoolean.🤖 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 217 - 238, The forced-transfer paths in GOGCloudSavesManager currently call downloadFile(...) and uploadFile(...) which only log failures, so the method can still return currentTimestamp() even if transfers failed; change downloadFile(...) and uploadFile(...) to return Boolean (true on success, false on failure), update the loops in the preferredSave==SaveLocation.Remote and preferredSave==SaveLocation.Local branches to collect the results and if any transfer returns false then return 0L instead of currentTimestamp(), otherwise return currentTimestamp() as before; keep Timber logging in the helpers but use the Boolean result to propagate failure to the caller.
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/service/gog/GOGService.kt (1)
481-493:⚠️ Potential issue | 🟠 MajorKeep
endSyncin afinallyso waiters always complete.The cancellation path now releases the lock, but any non-
ExceptionThrowablefromdoSyncCloudSavesForAppstill skips Line 491, leaving theCompletableDeferredinactiveSyncsincomplete for future callers. Put the release infinallyand rethrow cancellation from the catch.🛡️ Proposed fix
- val result = try { - doSyncCloudSavesForApp(context, serviceInstance, appId, preferredSave) - } catch (e: CancellationException) { - serviceInstance.gogManager.endSync(appId, false) - Timber.tag("GOG").d("[Cloud Saves] Sync completed and lock released for $appId") - throw e - } catch (e: Exception) { - Timber.tag("GOG").e(e, "[Cloud Saves] Failed to sync cloud saves for App ID: $appId") - false - } - serviceInstance.gogManager.endSync(appId, result) - Timber.tag("GOG").d("[Cloud Saves] Sync completed and lock released for $appId") - result + var result = false + try { + result = doSyncCloudSavesForApp(context, serviceInstance, appId, preferredSave) + result + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + Timber.tag("GOG").e(e, "[Cloud Saves] Failed to sync cloud saves for App ID: $appId") + false + } finally { + serviceInstance.gogManager.endSync(appId, result) + Timber.tag("GOG").d("[Cloud Saves] Sync completed and lock released for $appId") + }🤖 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/GOGService.kt` around lines 481 - 493, The endSync call must be moved into a finally block so any Throwable (including non-Exception errors) releases the lock; change the current try/catch to set a local result boolean (default false), run doSyncCloudSavesForApp inside try to assign result, catch CancellationException only to rethrow (after any needed log), catch Exception to log and keep result=false, and then in finally call serviceInstance.gogManager.endSync(appId, result) and the Timber release log; keep doSyncCloudSavesForApp, gogManager.endSync, and the Timber tag references so the fix is applied to the same logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/PluviaMain.kt`:
- Around line 1800-1805: When syncSuccess is false and the user chose an
explicit preferredSave (SaveLocation.Local or SaveLocation.Remote) the code
currently logs and aborts (Timber.tag("GOG").e(...),
setLoadingDialogVisible(false), return@launch) but does not surface an error to
the user; before returning from the coroutine in that branch, call the same
user-visible sync failure dialog routine (the function that shows the sync
failure UI) or dispatch the existing dialog/path used for non-explicit failures
so the user sees an explanation, then hide loading
(setLoadingDialogVisible(false)) and return@launch; update the branch that
checks preferredSave to invoke the dialog/showError method prior to the return.
---
Outside diff comments:
In `@app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt`:
- Around line 217-238: The forced-transfer paths in GOGCloudSavesManager
currently call downloadFile(...) and uploadFile(...) which only log failures, so
the method can still return currentTimestamp() even if transfers failed; change
downloadFile(...) and uploadFile(...) to return Boolean (true on success, false
on failure), update the loops in the preferredSave==SaveLocation.Remote and
preferredSave==SaveLocation.Local branches to collect the results and if any
transfer returns false then return 0L instead of currentTimestamp(), otherwise
return currentTimestamp() as before; keep Timber logging in the helpers but use
the Boolean result to propagate failure to the caller.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/service/gog/GOGService.kt`:
- Around line 481-493: The endSync call must be moved into a finally block so
any Throwable (including non-Exception errors) releases the lock; change the
current try/catch to set a local result boolean (default false), run
doSyncCloudSavesForApp inside try to assign result, catch CancellationException
only to rethrow (after any needed log), catch Exception to log and keep
result=false, and then in finally call serviceInstance.gogManager.endSync(appId,
result) and the Timber release log; keep doSyncCloudSavesForApp,
gogManager.endSync, and the Timber tag references so the fix is applied to the
same logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43d27570-11ac-431a-90d4-b61f90cb31bd
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.ktapp/src/main/java/app/gamenative/service/gog/GOGService.ktapp/src/main/java/app/gamenative/ui/PluviaMain.kt
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt:97">
P2: Metadata error path drops local timestamps, which can hide updated local files from sync classification.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt:388">
P2: `scanLocalFilesLite` duplicates recursive scan/path-normalization logic from `scanLocalFiles`, creating drift risk between preflight and sync behavior.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/gog/GOGService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/GOGService.kt:478">
P2: Concurrent sync de-duplication by `appId` can drop a later caller’s forced `preferredSave` direction, causing user-selected conflict resolution (upload/download) to be ignored.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/PluviaMain.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/PluviaMain.kt:1789">
P2: Forced GOG conflict-resolution sync failure aborts launch silently, leaving users without a visible error after selecting local/remote save.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
kiequoo
force-pushed
the
feat/cloud-saves-gog-conflict
branch
3 times, most recently
from
1331f75 to
98c9809
Compare
kiequoo
changed the title
kiequoo
force-pushed
the
feat/cloud-saves-gog-conflict
branch
5 times, most recently
from
995cd2f to
13cc95a
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/service/gog/GOGManager.kt (1)
1152-1162: Optional: parallelize per-locationcheckSynccalls.Each save location triggers an independent HTTP round-trip inside
checkSync(cloud file listing). Running them sequentially directly on the pre-launch path adds latency proportional to location count. For games with multiple save locations, awaiting these in parallel would cut pre-launch wait time without changing semantics, since the locations are independent and the reducer ismaxByOrNull.♻️ Sketch
- val best = saveLocations.mapNotNull { location -> - val timestamp = getCloudSaveSyncTimestamp(appId, location.name).toLongOrNull() ?: 0L - cloudSavesManager.checkSync( - clientId = location.clientId, - clientSecret = location.clientSecret, - localPath = location.location, - dirname = location.name, - lastSyncTimestamp = timestamp, - )?.takeIf { it.action == GOGCloudSavesManager.SyncAction.CONFLICT } - }.maxByOrNull { kotlin.math.abs(it.localTimestampMs - it.remoteTimestampMs) } + val best = coroutineScope { + saveLocations.map { location -> + async { + val timestamp = getCloudSaveSyncTimestamp(appId, location.name).toLongOrNull() ?: 0L + cloudSavesManager.checkSync( + clientId = location.clientId, + clientSecret = location.clientSecret, + localPath = location.location, + dirname = location.name, + lastSyncTimestamp = timestamp, + )?.takeIf { it.action == GOGCloudSavesManager.SyncAction.CONFLICT } + } + }.awaitAll() + }.filterNotNull().maxByOrNull { kotlin.math.abs(it.localTimestampMs - it.remoteTimestampMs) }🤖 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/GOGManager.kt` around lines 1152 - 1162, The sequential mapNotNull over saveLocations calls GOGCloudSavesManager.checkSync per location and causes serial HTTP latency; make these independent checks run concurrently by launching them as coroutines (e.g., using coroutineScope/Dispatchers.IO and async for each location), await all results (awaitAll), then perform the same filtering (.takeIf { it.action == GOGCloudSavesManager.SyncAction.CONFLICT }) and compute the max with .maxByOrNull on the completed results to produce best; reference GOGCloudSavesManager, checkSync, saveLocations, and best when applying this change.app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt (1)
370-373: Consider lettingCancellationExceptionpropagate.The broad
catch (e: Exception)here swallowskotlinx.coroutines.CancellationException(it extendsException), turning coroutine cancellation into a benignnullresult. On the pre-launch path that invokescheckSync, if the caller scope is cancelled mid-request the cancellation is consumed and never re-thrown. RethrowingCancellationExceptionbefore the generic handler keeps cancellation cooperative:♻️ Sketch
- } catch (e: Exception) { + } catch (e: kotlinx.coroutines.CancellationException) { + throw e + } catch (e: Exception) { Timber.tag("GOG-CloudSaves").e(e, "checkSync failed") null }🤖 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 370 - 373, The catch block in GOGCloudSavesManager.checkSync is swallowing kotlinx.coroutines.CancellationException by catching Exception; update the error handling to rethrow CancellationException (e.g., detect if e is CancellationException and throw it) before handling/logging the generic Exception so coroutine cancellation can propagate correctly while still logging other failures from checkSync.
🤖 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 370-373: The catch block in GOGCloudSavesManager.checkSync is
swallowing kotlinx.coroutines.CancellationException by catching Exception;
update the error handling to rethrow CancellationException (e.g., detect if e is
CancellationException and throw it) before handling/logging the generic
Exception so coroutine cancellation can propagate correctly while still logging
other failures from checkSync.
In `@app/src/main/java/app/gamenative/service/gog/GOGManager.kt`:
- Around line 1152-1162: The sequential mapNotNull over saveLocations calls
GOGCloudSavesManager.checkSync per location and causes serial HTTP latency; make
these independent checks run concurrently by launching them as coroutines (e.g.,
using coroutineScope/Dispatchers.IO and async for each location), await all
results (awaitAll), then perform the same filtering (.takeIf { it.action ==
GOGCloudSavesManager.SyncAction.CONFLICT }) and compute the max with
.maxByOrNull on the completed results to produce best; reference
GOGCloudSavesManager, checkSync, saveLocations, and best when applying this
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f594ccb4-aacb-43bf-9904-04dc796b7451
📒 Files selected for processing (4)
app/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/GOGService.ktapp/src/main/java/app/gamenative/ui/PluviaMain.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/ui/PluviaMain.kt
Contributor
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/service/gog/GOGManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/GOGManager.kt:1155">
P2: `checkPreLaunchConflict` lacks exception handling in a pre-launch path, so sync-check errors can escape and interrupt game launch instead of returning `null` gracefully.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/gog/GOGManager.kt:1162">
P1: Pre-launch conflict check drops additional conflicting save locations by selecting only one “best” conflict.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt`:
- Around line 221-235: The forced "download"/"upload" branches currently use
downloadableCloud.all { ... } / localFiles.all { ... } which short‑circuits on
the first false and skips remaining files; change them to iterate over every
file (e.g., a for loop or forEach) calling downloadFile(...) / uploadFile(...)
for each item, track a cumulative success flag (initialize true and set to false
if any call returns false), optionally log per-file failures with
Timber.tag("GOG-CloudSaves"), and after the loop return currentTimestamp() if
the cumulative flag is true else 0L; this fixes preferredAction handling in the
block that references downloadableCloud, localFiles, downloadFile, uploadFile
and the withContext return.
- Around line 367-369: The remoteMax currently includes deletion tombstones from
cloudFiles; update the computation so it ignores deleted cloud entries (those
with md5Hash == DELETION_MD5 or where isDeleted is true) when computing
remoteMax so the SyncCheckResult(remoteTimestampMs) reflects only real save
payloads — mirror the same filter used by syncSaves/downloadableCloud
(!isDeleted) before calling cloudFiles.maxOfOrNull and then pass remoteMax *
1000 into SyncCheckResult.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9014e775-3cf8-4c8a-8e2e-6e816138ca89
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.ktapp/src/main/java/app/gamenative/ui/PluviaMain.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/ui/PluviaMain.kt
Comment on lines
221
to
235
| if (preferredAction == "download" && downloadableCloud.isNotEmpty()) { | ||
| Timber.tag("GOG-CloudSaves").i("Forcing download of ${downloadableCloud.size} file(s) (user requested)") | ||
| downloadableCloud.forEach { file -> | ||
| val allDownloaded = downloadableCloud.all { file -> | ||
| downloadFile(credentials.userId, clientId, dirname, file, syncDir, credentials.accessToken) | ||
| } | ||
| return@withContext currentTimestamp() | ||
| return@withContext if (allDownloaded) currentTimestamp() else 0L | ||
| } | ||
|
|
||
| if (preferredAction == "upload" && localFiles.isNotEmpty()) { | ||
| Timber.tag("GOG-CloudSaves").i("Forcing upload of ${localFiles.size} file(s) (user requested)") | ||
| localFiles.forEach { file -> | ||
| val allUploaded = localFiles.all { file -> | ||
| uploadFile(credentials.userId, clientId, dirname, file, credentials.accessToken) | ||
| } | ||
| return@withContext currentTimestamp() | ||
| return@withContext if (allUploaded) currentTimestamp() else 0L | ||
| } |
Contributor
There was a problem hiding this comment.
Forced Keep Local/Keep Remote aborts on first failure, leaving a partial sync.
.all { ... } short‑circuits: the moment a single downloadFile/uploadFile returns false, the remaining files are never attempted. With the new preLaunchApp behavior that surfaces SYNC_FAIL on a 0L return, a transient failure on file #1 means:
- Files
#2..#N are silently skipped even though they would likely have succeeded. - The user sees
SYNC_FAILand retries, but local/cloud state is already partially transferred (anything done before the failure persists), and the next retry may itself conflict because only a subset of files was propagated.
For an explicit user decision ("Keep Local" / "Keep Remote"), prefer best‑effort: attempt every file, then report overall success.
♻️ Proposed fix
if (preferredAction == "download" && downloadableCloud.isNotEmpty()) {
Timber.tag("GOG-CloudSaves").i("Forcing download of ${downloadableCloud.size} file(s) (user requested)")
- val allDownloaded = downloadableCloud.all { file ->
- downloadFile(credentials.userId, clientId, dirname, file, syncDir, credentials.accessToken)
- }
+ val allDownloaded = downloadableCloud.fold(true) { acc, file ->
+ val ok = downloadFile(credentials.userId, clientId, dirname, file, syncDir, credentials.accessToken)
+ acc && ok
+ }
return@withContext if (allDownloaded) currentTimestamp() else 0L
}
if (preferredAction == "upload" && localFiles.isNotEmpty()) {
Timber.tag("GOG-CloudSaves").i("Forcing upload of ${localFiles.size} file(s) (user requested)")
- val allUploaded = localFiles.all { file ->
- uploadFile(credentials.userId, clientId, dirname, file, credentials.accessToken)
- }
+ val allUploaded = localFiles.fold(true) { acc, file ->
+ val ok = uploadFile(credentials.userId, clientId, dirname, file, credentials.accessToken)
+ acc && ok
+ }
return@withContext if (allUploaded) currentTimestamp() else 0L
}🤖 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 221 - 235, The forced "download"/"upload" branches currently use
downloadableCloud.all { ... } / localFiles.all { ... } which short‑circuits on
the first false and skips remaining files; change them to iterate over every
file (e.g., a for loop or forEach) calling downloadFile(...) / uploadFile(...)
for each item, track a cumulative success flag (initialize true and set to false
if any call returns false), optionally log per-file failures with
Timber.tag("GOG-CloudSaves"), and after the loop return currentTimestamp() if
the cumulative flag is true else 0L; this fixes preferredAction handling in the
block that references downloadableCloud, localFiles, downloadFile, uploadFile
and the withContext return.
Comment on lines
+367
to
+369
| val localMax = localFiles.maxOfOrNull { it.updateTimestamp ?: 0L } ?: 0L | ||
| val remoteMax = cloudFiles.maxOfOrNull { it.updateTimestamp ?: 0L } ?: 0L | ||
| SyncCheckResult(action, localMax * 1000, remoteMax * 1000) |
Contributor
There was a problem hiding this comment.
remoteTimestampMs includes deletion tombstones.
cloudFiles still contains entries with md5Hash == DELETION_MD5; their last_modified contributes to remoteMax. The value flows into the SYNC_CONFLICT dialog as the remote save time, so if the most recent cloud entry is a tombstone the user sees a misleading "cloud save" timestamp that doesn't correspond to any real save payload. syncSaves already uses downloadableCloud (!isDeleted) for the analogous empty-cloud check — mirror that here.
♻️ Proposed fix
- val localMax = localFiles.maxOfOrNull { it.updateTimestamp ?: 0L } ?: 0L
- val remoteMax = cloudFiles.maxOfOrNull { it.updateTimestamp ?: 0L } ?: 0L
+ val localMax = localFiles.maxOfOrNull { it.updateTimestamp ?: 0L } ?: 0L
+ val remoteMax = cloudFiles
+ .filter { !it.isDeleted }
+ .maxOfOrNull { it.updateTimestamp ?: 0L } ?: 0L
SyncCheckResult(action, localMax * 1000, remoteMax * 1000)🤖 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 367 - 369, The remoteMax currently includes deletion tombstones from
cloudFiles; update the computation so it ignores deleted cloud entries (those
with md5Hash == DELETION_MD5 or where isDeleted is true) when computing
remoteMax so the SyncCheckResult(remoteTimestampMs) reflects only real save
payloads — mirror the same filter used by syncSaves/downloadableCloud
(!isDeleted) before calling cloudFiles.maxOfOrNull and then pass remoteMax *
1000 into SyncCheckResult.
Contributor
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt:223">
P1: `Iterable.all` short-circuits on the first `false` return, so if one file fails to download/upload, the remaining files are silently skipped. For an explicit user-initiated sync direction (Keep Local / Keep Remote), this leaves local and cloud state partially transferred, which can cause spurious conflicts on retry. Use `fold` (or similar) to attempt every file before reporting overall success.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt:231">
P1: Same short-circuit issue as the download path: `localFiles.all` will skip remaining uploads after the first failure. Use `fold` to attempt every file.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
kiequoo
force-pushed
the
feat/cloud-saves-gog-conflict
branch
from
bf0f431 to
9f6a335
Compare
Owner
|
Hey @kiequoo , still working on this? |
Contributor
Author
Yes, just been busy recently. I'll try and pick it up again this week! |
Owner
|
cheers |
…sync failure As pointed out by CodeRabbit and Cubic in PR review: - uploadFile/downloadFile now return Boolean so forced-transfer paths (preferredAction download/upload) can detect and surface failures - forced download/upload now attempts every file before reporting overall failure, instead of short-circuiting on the first transfer error - preLaunchApp now aborts launch with a SYNC_FAIL dialog when an explicit conflict resolution sync (Keep Local/Keep Remote) fails, instead of silently proceeding - pre-launch conflict timestamps now ignore deleted cloud tombstones so the dialog reflects real save payload timestamps
kiequoo
force-pushed
the
feat/cloud-saves-gog-conflict
branch
from
9f6a335 to
3217efa
Compare
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.
Description
This PR now contains only the remaining GOG cloud save conflict dialog work.
It follows up on the now-closed #1094 and brings GOG conflict handling in line with the existing Steam sync flow. Epic parity is intended to follow separately.
Changes in this PR:
Recording
N/A for now. Main change is the pre-launch conflict dialog flow.
Type of Change
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by CodeRabbit
New Features
Bug Fixes / Reliability