Streamlined cloud sync for all platforms#576
Closed
unbelievableflavour wants to merge 4 commits intoutkarshdalal:masterfrom
Closed
Streamlined cloud sync for all platforms#576unbelievableflavour wants to merge 4 commits intoutkarshdalal:masterfrom
unbelievableflavour wants to merge 4 commits intoutkarshdalal:masterfrom
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt:71">
P3: User-facing dialogs format timestamps with `Date.toString()`, which is locale-independent and yields raw English date strings. Use a locale-aware formatter (e.g., `DateFormat.getDateTimeInstance(...)`) for UI strings.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
unbelievableflavour
force-pushed
the
streamlined-cloud-save-flow
branch
from
4b51968 to
60ccfeb
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces per-source cloud-save sync logic with a unified CloudSavePlatform framework. Pre-launch now calls Changes
Sequence Diagram(s)sequenceDiagram
participant PluviaMain
participant Dispatcher as CloudSavePlatforms
participant Platform as CloudSavePlatform
participant Service as PlatformService
participant UI
PluviaMain->>Dispatcher: syncCloudSaves(context, container, params, callbacks)
Dispatcher->>Dispatcher: getCloudSyncPlatforms(container)
alt platform found
Dispatcher->>Platform: callbacks.setLoadingMessage(...)
Dispatcher->>Platform: sync(context, container, params, callbacks)
Platform->>Service: perform platform-specific sync (download/merge/resolve)
Service-->>Platform: result (success/conflict/in-progress/error)
Platform-->>Dispatcher: CloudSyncOutcome (Proceed / ShowDialog / Retry)
Dispatcher-->>PluviaMain: CloudSyncOutcome
PluviaMain->>UI: when(outcome) { Proceed → onSuccess | ShowDialog → show dialog | Retry → retry preLaunchApp }
else no platform
Dispatcher-->>PluviaMain: Proceed
PluviaMain->>PluviaMain: launch app
end
sequenceDiagram
participant MainViewModel
participant Dispatcher as CloudSavePlatforms
participant Platform as CloudSavePlatform
participant Service as PlatformService
MainViewModel->>Dispatcher: uploadCloudSaves(context, appId, gameId, isOffline, prefixToPath)
Dispatcher->>Dispatcher: getCloudSyncPlatforms(container)
alt platform found
Dispatcher->>Platform: upload(context, appId, gameId, isOffline, prefixToPath)
Platform->>Service: perform upload (platform-specific)
Service-->>Platform: upload result / logs
else no platform
Dispatcher-->>MainViewModel: no-op
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 9
🧹 Nitpick comments (5)
app/src/main/java/app/gamenative/utils/cloudSync/EpicCloudSavePlatform.kt (2)
3-10:EpicCloudSavesManagershould be imported likeEpicService.
EpicCloudSavesManageris referenced by its full qualified name on lines 27 and 44, whileEpicServicefrom the same package is properly imported. Add the import for consistency.♻️ Proposed fix
import app.gamenative.service.epic.EpicService +import app.gamenative.service.epic.EpicCloudSavesManager import app.gamenative.utils.ContainerUtilsThen replace both inline usages:
- app.gamenative.service.epic.EpicCloudSavesManager.syncCloudSaves( + EpicCloudSavesManager.syncCloudSaves(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/cloudSync/EpicCloudSavePlatform.kt` around lines 3 - 10, Import EpicCloudSavesManager at the top of the file (same way EpicService is imported) and then replace the fully-qualified references to app.gamenative.service.epic.EpicCloudSavesManager used in the code (around usages tied to EpicCloudSavesManager on lines near where EpicService is referenced) with the simple class name EpicCloudSavesManager to match the existing import style and maintain consistency.
47-47: Magic string"upload"— prefer a named constant or enum.
preferredAction = "upload"is a stringly-typed value. IfEpicCloudSavesManager.syncCloudSavesever renames or changes this value, the discrepancy will be silent. A companion constant or a dedicated enum would make this safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/cloudSync/EpicCloudSavePlatform.kt` at line 47, Replace the magic string "upload" used in EpicCloudSavePlatform (the line setting preferredAction = "upload") with a named constant or enum to avoid silent breakage; add a companion object constant (e.g., PREFERRED_ACTION_UPLOAD) or an enum (e.g., PreferredAction.UPLOAD) and use that identifier where EpicCloudSavePlatform sets preferredAction and anywhere EpicCloudSavesManager.syncCloudSaves expects or compares this value so the contract is strongly-typed and centrally maintained.app/src/main/java/app/gamenative/utils/cloudSync/GOGCloudSavePlatform.kt (1)
26-29: Prefer an import over fully-qualified references forGOGService.
app.gamenative.service.gog.GOGServiceis referenced by its full package path twice instead of being imported at the top of the file. This is inconsistent with the file's other imports and hurts readability.♻️ Proposed refactor
+import app.gamenative.service.gog.GOGService import timber.log.TimberThen replace both FQN usages:
- app.gamenative.service.gog.GOGService.syncCloudSaves( + GOGService.syncCloudSaves(Also applies to: 41-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/cloudSync/GOGCloudSavePlatform.kt` around lines 26 - 29, The code uses the fully-qualified class name app.gamenative.service.gog.GOGService in multiple places (e.g., the calls in the syncCloudSaves blocks) instead of importing it; add an import for GOGService at the top of the file and replace the two FQN occurrences with the simple class name GOGService (update both call sites where app.gamenative.service.gog.GOGService.syncCloudSaves(...) is used).app/src/main/java/app/gamenative/utils/cloudSync/CloudSyncParams.kt (1)
11-20:CoroutineScopeembedded in adata classbreaks value semantics.
CoroutineScopehas no meaningfulequals()/hashCode()— it falls back to reference identity. As a result:
- Two otherwise-identical
CloudSyncParamswill compare as unequal if they hold different scope instances.copy()shares the same scope reference, which is often fine but surprising when the scope is cancelled mid-flow.- The lifecycle resource doesn't belong in a pure-value parameter bag.
The idiomatic fix is to remove
scopefrom the data class and pass it as a separate argument directly tosyncCloudSaves():♻️ Proposed refactor
data class CloudSyncParams( val appId: String, val gameId: Int, val ignorePendingOperations: Boolean = false, val preferredSave: SaveLocation = SaveLocation.None, val useTemporaryOverride: Boolean = false, val retryCount: Int = 0, val isOffline: Boolean = false, - val scope: CoroutineScope, )Then update the
syncCloudSavescall site to passscopeas a separate parameter:suspend fun syncCloudSaves( context: Context, container: Container, params: CloudSyncParams, scope: CoroutineScope, // ← lifted out setLoadingMessage: (String) -> Unit, setLoadingProgress: (Float) -> Unit, ): CloudSyncOutcome🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/cloudSync/CloudSyncParams.kt` around lines 11 - 20, Remove the CoroutineScope property from the CloudSyncParams data class (CloudSyncParams) because it breaks value semantics; update the data class to only hold pure-value fields and then change the signature of syncCloudSaves to accept a separate CoroutineScope parameter (e.g., add scope: CoroutineScope to syncCloudSaves) and update all call sites to pass the scope explicitly rather than embedding it in CloudSyncParams; ensure no other code relies on params.scope and adjust usages to use the new standalone scope argument.app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt (1)
79-83: Extract the retry limit5into a named constant.The value
5appears both in the condition (line 80) and the log message (line 81). Extracting it to a constant (e.g.,MAX_SYNC_RETRIES) avoids divergence if the limit is later changed in only one place.Proposed refactor
+private const val MAX_SYNC_RETRIES = 5 + ... - if (useTemporaryOverride && retryCount < 5) { - Timber.i("Sync in progress for intent launch, retrying in 2 seconds... (attempt ${retryCount + 1}/5)") + if (useTemporaryOverride && retryCount < MAX_SYNC_RETRIES) { + Timber.i("Sync in progress for intent launch, retrying in 2 seconds... (attempt ${retryCount + 1}/$MAX_SYNC_RETRIES)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt` around lines 79 - 83, Extract the literal retry limit "5" into a named constant (e.g., MAX_SYNC_RETRIES) and use it in both the condition and the log message inside SteamCloudSavePlatform where SyncResult.InProgress is handled; update the conditional from "retryCount < 5" to "retryCount < MAX_SYNC_RETRIES" and change the Timber.i message to interpolate MAX_SYNC_RETRIES instead of the hard-coded 5 so both uses remain consistent.
🤖 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 1226-1238: The Retry branch for CloudSyncOutcome.Retry calls
preLaunchApp without forwarding the isOffline flag, causing offline state to
reset; update the CloudSyncOutcome.Retry case to pass isOffline = isOffline into
the recursive preLaunchApp call (same parameter list as other branches) so
preLaunchApp and any created CloudSyncParams retain the original offline mode
and avoid re-enabling account/session checks or triggering online syncs.
In `@app/src/main/java/app/gamenative/utils/CloudSavePlatforms.kt`:
- Line 47: The code in syncCloudSaves and uploadCloudSaves uses
platforms.single() which will throw if multiple platforms match; change the
selection to use platforms.first() (or platforms.firstOrNull() with the existing
isEmpty() guard) to avoid IllegalArgumentException, and add a defensive warning
log when platforms.size > 1 to record the invariant violation; update the
platform variable assignment in both syncCloudSaves and uploadCloudSaves (where
platforms.single() is called) to use first()/firstOrNull() and emit a clear
warning via the existing logger if multiple matches are found.
- Around line 64-67: In uploadCloudSaves, wrap the call to
ContainerUtils.getContainer(context, appId) in a try-catch to handle the
exception thrown when the container doesn't exist: catch the exception, log or
report it (including the exception message) and return early so the rest of the
method (getCloudSyncPlatforms and platforms.single().upload) is not executed;
reference the ContainerUtils.getContainer call and the platforms variable to
ensure you only proceed when a container was successfully obtained.
In `@app/src/main/java/app/gamenative/utils/cloudSync/EpicCloudSavePlatform.kt`:
- Around line 26-33: Wrap the call to
app.gamenative.service.epic.EpicCloudSavesManager.syncCloudSaves in a
try/finally so EpicService.cleanupLaunchTokens(context) always runs even if
syncCloudSaves throws; place the cleanup call in the finally block and preserve
returning CloudSyncOutcome.Proceed after the sync/cleanup sequence to avoid
leaking launch tokens (update the block in EpicCloudSavePlatform where
syncCloudSaves and EpicService.cleanupLaunchTokens are invoked).
- Around line 36-49: The upload() override in EpicCloudSavePlatform ignores the
isOffline flag and always calls EpicCloudSavesManager.syncCloudSaves; change it
to respect isOffline by checking the isOffline parameter and only invoking
EpicCloudSavesManager.syncCloudSaves when the device is online (isOffline ==
false), otherwise log/skip (or queue) the upload attempt; reference upload(),
EpicCloudSavePlatform, and EpicCloudSavesManager.syncCloudSaves when making this
conditional change so behavior matches the Steam pattern that respects the
offline flag.
- Around line 26-30: The log in EpicCloudSavePlatform
(Timber.tag("Epic").i(...)) references params.appId while the call to
EpicCloudSavesManager.syncCloudSaves passes params.gameId, causing inconsistent
identifiers; update the log to reference the same identifier used for syncing
(params.gameId) or include both values for clarity (e.g., mention params.gameId
and optionally params.appId) so the log matches the argument passed to
EpicCloudSavesManager.syncCloudSaves.
In `@app/src/main/java/app/gamenative/utils/cloudSync/GOGCloudSavePlatform.kt`:
- Around line 19-31: The GOGCloudSavePlatform.sync (and similarly upload) must
catch any exceptions thrown by
app.gamenative.service.gog.GOGService.syncCloudSaves so they don't escape; wrap
the call in a try/catch, log the exception with Timber.tag("GOG"), and return
CloudSyncOutcome.ShowDialog (including a user-facing error message or the
exception message) instead of letting the exception bubble, ensuring the method
signatures sync(...) and upload(...) handle failures and return a ShowDialog
outcome when an error occurs.
In `@app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt`:
- Around line 37-39: Replace the force-unwrap in the prefixToPath lambda by
guarding against a missing Steam session: before using SteamService.userSteamId
in SteamCloudSavePlatform (where prefixToPath calls PathType.from(...,
params.gameId, SteamService.userSteamId!!.accountID)), check if
SteamService.userSteamId?.isValid == true (or that userSteamId is non-null) and
if not return early with CloudSyncOutcome.ShowDialog (or otherwise handle the
error); then use the safe userSteamId.accountID when calling PathType.from. This
keeps the null-safe pattern consistent with SteamService.kt and prevents an NPE
from the !! operator.
- Around line 125-183: The when on pro.operation (ECloudPendingRemoteOperation)
is non-exhaustive and will throw NoWhenBranchMatchedException for
UNRECOGNIZED/new enum values; add an else branch to the when in
SteamCloudSavePlatform.kt that returns a safe fallback MessageDialogState (e.g.,
visible = true, type = DialogType.PENDING_OPERATION_NONE or a generic error
dialog, with sensible title/message/dismissBtnText using existing string
resources) so unknown/UNRECOGNIZED enum values are handled gracefully.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/utils/cloudSync/CloudSyncParams.kt`:
- Around line 11-20: Remove the CoroutineScope property from the CloudSyncParams
data class (CloudSyncParams) because it breaks value semantics; update the data
class to only hold pure-value fields and then change the signature of
syncCloudSaves to accept a separate CoroutineScope parameter (e.g., add scope:
CoroutineScope to syncCloudSaves) and update all call sites to pass the scope
explicitly rather than embedding it in CloudSyncParams; ensure no other code
relies on params.scope and adjust usages to use the new standalone scope
argument.
In `@app/src/main/java/app/gamenative/utils/cloudSync/EpicCloudSavePlatform.kt`:
- Around line 3-10: Import EpicCloudSavesManager at the top of the file (same
way EpicService is imported) and then replace the fully-qualified references to
app.gamenative.service.epic.EpicCloudSavesManager used in the code (around
usages tied to EpicCloudSavesManager on lines near where EpicService is
referenced) with the simple class name EpicCloudSavesManager to match the
existing import style and maintain consistency.
- Line 47: Replace the magic string "upload" used in EpicCloudSavePlatform (the
line setting preferredAction = "upload") with a named constant or enum to avoid
silent breakage; add a companion object constant (e.g., PREFERRED_ACTION_UPLOAD)
or an enum (e.g., PreferredAction.UPLOAD) and use that identifier where
EpicCloudSavePlatform sets preferredAction and anywhere
EpicCloudSavesManager.syncCloudSaves expects or compares this value so the
contract is strongly-typed and centrally maintained.
In `@app/src/main/java/app/gamenative/utils/cloudSync/GOGCloudSavePlatform.kt`:
- Around line 26-29: The code uses the fully-qualified class name
app.gamenative.service.gog.GOGService in multiple places (e.g., the calls in the
syncCloudSaves blocks) instead of importing it; add an import for GOGService at
the top of the file and replace the two FQN occurrences with the simple class
name GOGService (update both call sites where
app.gamenative.service.gog.GOGService.syncCloudSaves(...) is used).
In `@app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt`:
- Around line 79-83: Extract the literal retry limit "5" into a named constant
(e.g., MAX_SYNC_RETRIES) and use it in both the condition and the log message
inside SteamCloudSavePlatform where SyncResult.InProgress is handled; update the
conditional from "retryCount < 5" to "retryCount < MAX_SYNC_RETRIES" and change
the Timber.i message to interpolate MAX_SYNC_RETRIES instead of the hard-coded 5
so both uses remain consistent.
Contributor
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/utils/cloudSync/GOGCloudSavePlatform.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/cloudSync/GOGCloudSavePlatform.kt:34">
P2: Broad `catch (Exception)` in a suspend function swallows `CancellationException`, breaking coroutine cancellation/structured concurrency. Rethrow cancellation before logging other failures.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
unbelievableflavour
force-pushed
the
streamlined-cloud-save-flow
branch
from
84f3a7b to
47d22e6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/utils/cloudSync/GOGCloudSavePlatform.kt (1)
50-67:upload()does not checkisOffline— inconsistent withEpicCloudSavePlatform.
EpicCloudSavePlatform.upload()early-returns whenisOfflineis true, butGOGCloudSavePlatform.upload()ignores the flag entirely and always attempts the upload. If the device is offline,GOGService.syncCloudSaveswill likely fail with a network error (caught by thetry/catch, so not a crash, but a wasted call + logged error). Consider aligning the behavior.♻️ Proposed fix — add isOffline guard
override suspend fun upload( context: Context, appId: String, gameId: Int, isOffline: Boolean, prefixToPath: (String) -> String, ) { + if (isOffline) { + Timber.tag("GOG").i("[Cloud Saves] Skipping upload for $appId (isOffline=true)") + return + } Timber.tag("GOG").i("[Cloud Saves] GOG Game detected for $appId — uploading cloud saves after close")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/cloudSync/GOGCloudSavePlatform.kt` around lines 50 - 67, GOGCloudSavePlatform.upload currently ignores the isOffline flag and always calls app.gamenative.service.gog.GOGService.syncCloudSaves, causing unnecessary network attempts and errors; modify GOGCloudSavePlatform.upload to mirror EpicCloudSavePlatform.upload by checking isOffline at the start and returning early when true (i.e., do not call GOGService.syncCloudSaves if isOffline), keeping the existing Timber logging and try/catch for the online path.
🤖 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/utils/cloudSync/EpicCloudSavePlatform.kt`:
- Around line 54-71: The upload method in EpicCloudSavePlatform calls
app.gamenative.service.epic.EpicCloudSavesManager.syncCloudSaves without error
handling, so wrap that call in a try/catch inside upload (maintain the existing
isOffline early return), catch Throwable (or Exception), and log the failure
using Timber.tag("Epic").e(...) including the exception and a clear message
(mirroring GOGCloudSavePlatform.upload's pattern) to prevent uncaught exceptions
from propagating to the orchestrator.
In `@app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt`:
- Around line 209-217: The upload() implementation in SteamCloudSavePlatform
directly calls SteamService.closeApp(...).await() with no error handling; wrap
the call in a try/catch (catch Exception) inside SteamCloudSavePlatform.upload
(same place as SteamService.closeApp.await invocation), log the failure with
context (appId and gameId) using the same logger/format used by
GOGCloudSavePlatform, and swallow the exception (do not rethrow) so
uploadCloudSaves() won’t crash; mirror GOGCloudSavePlatform’s error handling
behavior to keep platform behavior consistent.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/PluviaMain.kt`:
- Around line 1223-1239: The Retry branch of the CloudSyncOutcome switch was
previously dropping the offline flag; ensure the recursive call to preLaunchApp
forwards the isOffline parameter (as done here) by passing isOffline = isOffline
in the CloudSyncOutcome.Retry case so the offline state is preserved across
retries; locate the CloudSyncOutcome.Retry handling and the preLaunchApp call to
verify the isOffline argument is present.
In `@app/src/main/java/app/gamenative/utils/cloudSync/GOGCloudSavePlatform.kt`:
- Around line 22-48: The sync() implementation in GOGCloudSavePlatform (function
sync) correctly catches exceptions, rethrows CancellationException, logs the
error with Timber, and returns CloudSyncOutcome.ShowDialog with localized
messages on failure; no code changes are needed—keep the existing try/catch, the
call to app.gamenative.service.gog.GOGService.syncCloudSaves, and the
CloudSyncOutcome.Proceed/ShowDialog branches as implemented.
In `@app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt`:
- Around line 37-39: The code uses SteamService.userSteamId!! inside the
prefixToPath lambda which can throw an NPE if the Steam session is absent;
update SteamCloudSavePlatform (the prefixToPath lambda / any callers that
compute paths) to guard against a null SteamService.userSteamId before using
PathType.from(...).toAbsPath(...), and when null return a
CloudSyncOutcome.ShowDialog with a clear error message about the missing Steam
session instead of dereferencing the value; ensure callers of prefixToPath
handle the early-return outcome (or move the null-check out of the lambda into
the calling flow) so no force-unwrap is used.
- Around line 125-191: The when expression switching on pro.operation in
SteamCloudSavePlatform.kt did not handle protobuf's UNRECOGNIZED (or future)
enum values causing a possible NoWhenBranchMatchedException; add an else branch
that returns a safe default MessageDialogState (as done) to cover
ECloudPendingRemoteOperation.UNRECOGNIZED and any new enum cases, ensuring all
branches of the when on pro.operation (ECloudPendingRemoteOperation) produce a
MessageDialogState (use the same fallback
DialogType.PENDING_OPERATION_NONE/title/message/dismissBtnText currently
implemented).
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/utils/cloudSync/GOGCloudSavePlatform.kt`:
- Around line 50-67: GOGCloudSavePlatform.upload currently ignores the isOffline
flag and always calls app.gamenative.service.gog.GOGService.syncCloudSaves,
causing unnecessary network attempts and errors; modify
GOGCloudSavePlatform.upload to mirror EpicCloudSavePlatform.upload by checking
isOffline at the start and returning early when true (i.e., do not call
GOGService.syncCloudSaves if isOffline), keeping the existing Timber logging and
try/catch for the online path.
Owner
|
Big change, I'll review over weekend. |
unbelievableflavour
force-pushed
the
streamlined-cloud-save-flow
branch
from
47d22e6 to
40e1534
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt (2)
50-52: UseLOADING_PROGRESS_UNKNOWNinstead of the hardcoded-1f.Line 41 correctly uses the constant, but line 51 inlines the same magic value, breaking the single-source-of-truth guarantee the constant was introduced to provide.
♻️ Proposed fix
- callbacks.setLoadingProgress(if (progress < 0) -1f else progress) + callbacks.setLoadingProgress(if (progress < 0) LOADING_PROGRESS_UNKNOWN else progress)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt` around lines 50 - 52, Replace the hardcoded -1f in the callbacks.setLoadingProgress call with the existing LOADING_PROGRESS_UNKNOWN constant to maintain a single source of truth; locate the setLoadingProgress invocation inside SteamCloudSavePlatform (callbacks.setLoadingProgress(...)) and change the ternary branch that currently returns -1f to return LOADING_PROGRESS_UNKNOWN instead so both usages use the same constant.
79-83: Extract the retry-count ceiling5as a named constant.The magic number has no documentation at the call site, making it non-obvious why retries stop at 5 or whether this value is intentional for the intent-launch path.
♻️ Proposed fix
+private const val MAX_INTENT_LAUNCH_RETRIES = 5 + internal object SteamCloudSavePlatform : CloudSavePlatform { ... SyncResult.InProgress -> { - if (useTemporaryOverride && retryCount < 5) { + if (useTemporaryOverride && retryCount < MAX_INTENT_LAUNCH_RETRIES) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt` around lines 79 - 83, Extract the magic retry ceiling "5" into a clearly named constant (e.g., MAX_INTENT_LAUNCH_RETRIES) in SteamCloudSavePlatform.kt and replace the literal in the SyncResult.InProgress branch where useTemporaryOverride && retryCount < 5 is checked and in the retry-log message (attempt ${retryCount + 1}/5) so both the condition and the message reference the constant; ensure the constant is accessible in the scope where retryCount and CloudSyncOutcome.Retry are used and update the log string to interpolate the constant instead of the literal.app/src/main/java/app/gamenative/utils/cloudSync/CloudSyncParams.kt (1)
10-19: Consider movingscopeout of thedata classprimary constructor.
data classgeneratesequals(),hashCode(),toString(), andcopy()from all primary-constructor properties. SinceCoroutineScopedoes not overrideequals()/hashCode(), it falls back to reference identity. TwoCloudSyncParamsinstances with the same logical parameters but different scope instances will compare as unequal, andtoString()will emit a useless memory address for the scope. While this isn't a current correctness bug, it violates the spirit ofdata class(value semantics) and could cause subtle issues if the type is ever used in aSetor as aMapkey.Options:
- Move
scopeto a regular class body property (outside the primary constructor) so it's excluded from generated members.- Change
CloudSyncParamsto a plainclass.- Keep it as-is and document the identity-equality caveat.
♻️ Proposed approach: exclude scope from generated members
data class CloudSyncParams( val appId: String, val gameId: Int, val ignorePendingOperations: Boolean = false, val preferredSave: SaveLocation = SaveLocation.None, val useTemporaryOverride: Boolean = false, val retryCount: Int = 0, val isOffline: Boolean = false, - val scope: CoroutineScope, -) +) { + // Excluded from equals/hashCode/toString/copy since CoroutineScope uses reference identity + lateinit var scope: CoroutineScope +}Or simply promote to a plain
classif value-equality is never needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/cloudSync/CloudSyncParams.kt` around lines 10 - 19, CloudSyncParams is a data class that currently includes the CoroutineScope in its primary constructor, which makes the generated equals()/hashCode()/toString()/copy() use scope's identity; move scope out of the primary constructor to exclude it from generated members by removing "val scope: CoroutineScope" from the constructor and declaring a regular property inside the class body (e.g., val scope: CoroutineScope) initialized from a secondary constructor or an init block so external callers still pass a scope when constructing but it no longer participates in data class equality/printing; alternatively, if value-equality is not needed at all, convert CloudSyncParams from "data class" to a plain "class" and keep scope in the body—update any call sites that rely on the generated copy()/toString()/equals() accordingly.
🤖 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 1221-1239: The code unconditionally calls
setLoadingDialogVisible(false) before handling outcome, causing a flicker when
outcome is CloudSyncOutcome.Retry because preLaunchApp will re-show the dialog;
change the logic to skip hiding the dialog when outcome is Retry (i.e., only
call setLoadingDialogVisible(false) for non-Retry branches), and also convert
the when(outcome) statement into a when expression (e.g., assign a val result =
when(outcome) { ... } or directly return/handle branches) so the compiler
enforces exhaustiveness for the CloudSyncOutcome sealed type; adjust usage of
setMessageDialogState, onSuccess, and the preLaunchApp call accordingly so
behavior is identical aside from not hiding the dialog on Retry.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt`:
- Around line 37-39: The prefixToPath lambda in SteamCloudSavePlatform uses an
unguarded force-unwrap SteamService.userSteamId!! which can throw a
KotlinNullPointerException when no Steam session exists; change the code to
first validate SteamService.userSteamId (e.g., check
SteamService.userSteamId?.isValid == true) and only call
PathType.from(prefix).toAbsPath(context, params.gameId,
SteamService.userSteamId.accountID) when valid, otherwise handle the null case
(return a safe fallback path or throw a controlled exception) so that
SteamService.userSteamId is never force-unwrapped in prefixToPath.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/utils/cloudSync/CloudSyncParams.kt`:
- Around line 10-19: CloudSyncParams is a data class that currently includes the
CoroutineScope in its primary constructor, which makes the generated
equals()/hashCode()/toString()/copy() use scope's identity; move scope out of
the primary constructor to exclude it from generated members by removing "val
scope: CoroutineScope" from the constructor and declaring a regular property
inside the class body (e.g., val scope: CoroutineScope) initialized from a
secondary constructor or an init block so external callers still pass a scope
when constructing but it no longer participates in data class equality/printing;
alternatively, if value-equality is not needed at all, convert CloudSyncParams
from "data class" to a plain "class" and keep scope in the body—update any call
sites that rely on the generated copy()/toString()/equals() accordingly.
In `@app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt`:
- Around line 50-52: Replace the hardcoded -1f in the
callbacks.setLoadingProgress call with the existing LOADING_PROGRESS_UNKNOWN
constant to maintain a single source of truth; locate the setLoadingProgress
invocation inside SteamCloudSavePlatform (callbacks.setLoadingProgress(...)) and
change the ternary branch that currently returns -1f to return
LOADING_PROGRESS_UNKNOWN instead so both usages use the same constant.
- Around line 79-83: Extract the magic retry ceiling "5" into a clearly named
constant (e.g., MAX_INTENT_LAUNCH_RETRIES) in SteamCloudSavePlatform.kt and
replace the literal in the SyncResult.InProgress branch where
useTemporaryOverride && retryCount < 5 is checked and in the retry-log message
(attempt ${retryCount + 1}/5) so both the condition and the message reference
the constant; ensure the constant is accessible in the scope where retryCount
and CloudSyncOutcome.Retry are used and update the log string to interpolate the
constant instead of the literal.
40e1534 to
ad0109e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/src/main/java/app/gamenative/utils/cloudSync/EpicCloudSavePlatform.kt (1)
31-31: Add import forEpicCloudSavesManagerinstead of using fully-qualified names.
EpicServiceis already imported (line 5), butEpicCloudSavesManageris referenced with its full package path on lines 31 and 67. Adding an import keeps it consistent.♻️ Proposed refactor
import app.gamenative.service.epic.EpicService +import app.gamenative.service.epic.EpicCloudSavesManagerThen on lines 31 and 67:
- app.gamenative.service.epic.EpicCloudSavesManager.syncCloudSaves( + EpicCloudSavesManager.syncCloudSaves(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/cloudSync/EpicCloudSavePlatform.kt` at line 31, Add an import for EpicCloudSavesManager at the top of the file and replace the two fully-qualified references with the simple class name; specifically, import app.gamenative.service.epic.EpicCloudSavesManager and change calls like app.gamenative.service.epic.EpicCloudSavesManager.syncCloudSaves(...) to EpicCloudSavesManager.syncCloudSaves(...) (the occurrences are in the usages currently at lines where syncCloudSaves is invoked).app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt (1)
57-123:resolveResultre-extractsgameIdfromappId— could acceptgameIddirectly.Line 123 calls
ContainerUtils.extractGameIdFromContainerId(appId), but the call site at line 54 already hasparams.gameIdavailable. PassinggameId: Intinstead (or in addition toappId) eliminates the redundant extraction.♻️ Proposed refactor
private suspend fun resolveResult( postSyncInfo: PostSyncInfo, context: Context, appId: String, + gameId: Int, useTemporaryOverride: Boolean, retryCount: Int, ): CloudSyncOutcome = when (postSyncInfo.syncResult) {- val gameName = SteamService.getAppInfoOf(ContainerUtils.extractGameIdFromContainerId(appId))?.name ?: "" + val gameName = SteamService.getAppInfoOf(gameId)?.name ?: ""- resolveResult(postSyncInfo, context, params.appId, params.useTemporaryOverride, params.retryCount) + resolveResult(postSyncInfo, context, params.appId, params.gameId, params.useTemporaryOverride, params.retryCount)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt` around lines 57 - 123, resolveResult currently re-extracts the gameId from appId using ContainerUtils.extractGameIdFromContainerId(appId) before calling SteamService.getAppInfoOf; change resolveResult to accept a gameId: Int (or add an overload/extra parameter) so callers pass params.gameId directly, remove the redundant ContainerUtils.extractGameIdFromContainerId call, and update all callers to supply the new gameId parameter (ensure SteamService.getAppInfoOf is still called with the provided gameId).app/src/main/java/app/gamenative/utils/cloudSync/CloudSyncParams.kt (1)
10-19:CoroutineScopeas adata classfield participates inequals/hashCode/copy.
CoroutineScopedoesn't overrideequals/hashCode, so twoCloudSyncParamsinstances that are logically identical but were created with different scope objects will compare as unequal.copy()will silently share the same scope reference. Given that this class is used purely as a parameter bundle and never compared, there is no runtime impact today — but it is an unconventional contract for adata class.Consider either dropping
dataand making it a plainclass, or extractingscopefrom the constructor and having callers supply it separately at the call site.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/cloudSync/CloudSyncParams.kt` around lines 10 - 19, The CloudSyncParams data class includes a CoroutineScope which participates in equals/hashCode/copy unexpectedly; change CloudSyncParams to a plain class (remove the data modifier) or remove scope from the primary constructor so callers pass the CoroutineScope separately; specifically update the declaration of CloudSyncParams (the class name CloudSyncParams and the CoroutineScope parameter) to either: 1) replace "data class CloudSyncParams(..., val scope: CoroutineScope)" with "class CloudSyncParams(..., val scope: CoroutineScope)" to avoid auto-generated equals/hashCode/copy, or 2) remove "val scope: CoroutineScope" from the constructor and add scope as a separate non-data parameter at call sites. Ensure any existing usages of copy() or structural equality are updated accordingly.
🤖 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/utils/cloudSync/EpicCloudSavePlatform.kt`:
- Around line 66-74: The upload() path in EpicCloudSavePlatform.kt swallows
CancellationException; update the catch around
app.gamenative.service.epic.EpicCloudSavesManager.syncCloudSaves (used in
upload) to rethrow CancellationException (or check `if (e is
CancellationException) throw e`) before logging, mirroring the pattern used in
sync() so structured concurrency isn't broken; ensure you reference and adjust
the try/catch in the upload method that calls syncCloudSaves accordingly.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/PluviaMain.kt`:
- Around line 1222-1244: The current when (outcome) block in the preLaunchApp
call site is used as a statement so the compiler won't enforce exhaustiveness if
CloudSyncOutcome gains a new subtype; change it to a when expression by
assigning its result to a throwaway val (e.g., val _ = when (outcome) { ... })
so missing branches become a compile error. Keep the existing branches
(CloudSyncOutcome.Proceed calling setLoadingDialogVisible(false) and onSuccess,
CloudSyncOutcome.ShowDialog calling setLoadingDialogVisible(false) and
setMessageDialogState(outcome.state), and CloudSyncOutcome.Retry calling
preLaunchApp(..., retryCount = retryCount + 1, ...)) unchanged except for the
wrapping assignment to make the when an expression.
In `@app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt`:
- Around line 37-39: The code in SteamCloudSavePlatform.kt uses an unguarded
force-unwrap SteamService.userSteamId!! inside the prefixToPath lambda which can
NPE if no Steam session exists; update sync() (or the surrounding caller) to
check SteamService.userSteamId?.isValid == true before calling or constructing
prefixToPath, or rewrite prefixToPath to safely access SteamService.userSteamId
with a let/?. and provide a clear fallback or early return; ensure
PathType.from(...).toAbsPath(context, params.gameId,
SteamService.userSteamId.accountID) only runs when SteamService.userSteamId is
non-null and valid.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/utils/cloudSync/CloudSyncParams.kt`:
- Around line 10-19: The CloudSyncParams data class includes a CoroutineScope
which participates in equals/hashCode/copy unexpectedly; change CloudSyncParams
to a plain class (remove the data modifier) or remove scope from the primary
constructor so callers pass the CoroutineScope separately; specifically update
the declaration of CloudSyncParams (the class name CloudSyncParams and the
CoroutineScope parameter) to either: 1) replace "data class CloudSyncParams(...,
val scope: CoroutineScope)" with "class CloudSyncParams(..., val scope:
CoroutineScope)" to avoid auto-generated equals/hashCode/copy, or 2) remove "val
scope: CoroutineScope" from the constructor and add scope as a separate non-data
parameter at call sites. Ensure any existing usages of copy() or structural
equality are updated accordingly.
In `@app/src/main/java/app/gamenative/utils/cloudSync/EpicCloudSavePlatform.kt`:
- Line 31: Add an import for EpicCloudSavesManager at the top of the file and
replace the two fully-qualified references with the simple class name;
specifically, import app.gamenative.service.epic.EpicCloudSavesManager and
change calls like
app.gamenative.service.epic.EpicCloudSavesManager.syncCloudSaves(...) to
EpicCloudSavesManager.syncCloudSaves(...) (the occurrences are in the usages
currently at lines where syncCloudSaves is invoked).
In `@app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt`:
- Around line 57-123: resolveResult currently re-extracts the gameId from appId
using ContainerUtils.extractGameIdFromContainerId(appId) before calling
SteamService.getAppInfoOf; change resolveResult to accept a gameId: Int (or add
an overload/extra parameter) so callers pass params.gameId directly, remove the
redundant ContainerUtils.extractGameIdFromContainerId call, and update all
callers to supply the new gameId parameter (ensure SteamService.getAppInfoOf is
still called with the provided gameId).
ad0109e to
1313114
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt (1)
24-29: All three platforms return the samegetLoadingMessage— consider hoisting to a default in the interface or a shared constant.
GOGCloudSavePlatform,EpicCloudSavePlatform, andSteamCloudSavePlatformall returncontext.getString(R.string.main_syncing_cloud_saves). This could be a default implementation in theCloudSavePlatforminterface, reducing duplication and the chance of drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt` around lines 24 - 29, The three platform implementations (GOGCloudSavePlatform, EpicCloudSavePlatform, SteamCloudSavePlatform) duplicate the same getLoadingMessage implementation; add a default implementation in the CloudSavePlatform interface (provide fun getLoadingMessage(context: Context, container: Container) = context.getString(R.string.main_syncing_cloud_saves)) or a shared constant used by that default, then remove the redundant overrides from GOGCloudSavePlatform, EpicCloudSavePlatform, and SteamCloudSavePlatform so they inherit the interface default.
🤖 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/utils/cloudSync/SteamCloudSavePlatform.kt`:
- Around line 217-221: The catch in SteamCloudSavePlatform.upload around
SteamService.closeApp(...).await() currently swallows CancellationException;
update the catch to rethrow CancellationException before logging to preserve
structured concurrency (mirror GOG and Epic patterns). Specifically, in the
upload() method catch block for exceptions from SteamService.closeApp, check if
the caught exception is a CancellationException and rethrow it, otherwise log
via Timber.tag("Steam").e(...) and handle as before.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/PluviaMain.kt`:
- Around line 1222-1245: The when on the sealed CloudSyncOutcome should be
turned into an expression so the compiler enforces exhaustiveness: replace the
statement-style when with either an assignment (e.g. val action = when(outcome)
{ ... } followed by executing action) or use return@launch with when to return a
value, and ensure all CloudSyncOutcome branches are handled; update the branches
that currently call setLoadingDialogVisible, setMessageDialogState, onSuccess,
or recursively call preLaunchApp to instead be values/actions produced by the
when expression so missing future subclasses will fail compile-time (refer to
CloudSyncOutcome, setLoadingDialogVisible, setMessageDialogState, onSuccess, and
preLaunchApp).
In `@app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt`:
- Around line 37-39: The lambda uses SteamService.userSteamId!! which can throw
an NPE if the Steam session is absent; update SteamCloudSavePlatform (the
prefixToPath lambda and its caller in sync()/syncCloudSaves/preLaunchApp flow)
to first check SteamService.userSteamId for null and handle the absent session
by returning the ShowDialog result (or otherwise short-circuiting sync) instead
of using '!!'; ensure all usages of prefixToPath (and any direct accesses to
SteamService.userSteamId) are guarded so they never dereference a null
userSteamId.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/utils/cloudSync/SteamCloudSavePlatform.kt`:
- Around line 24-29: The three platform implementations (GOGCloudSavePlatform,
EpicCloudSavePlatform, SteamCloudSavePlatform) duplicate the same
getLoadingMessage implementation; add a default implementation in the
CloudSavePlatform interface (provide fun getLoadingMessage(context: Context,
container: Container) = context.getString(R.string.main_syncing_cloud_saves)) or
a shared constant used by that default, then remove the redundant overrides from
GOGCloudSavePlatform, EpicCloudSavePlatform, and SteamCloudSavePlatform so they
inherit the interface default.
1313114 to
1fa73b7
Compare
unbelievableflavour
force-pushed
the
streamlined-cloud-save-flow
branch
from
1fa73b7 to
d810fe1
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.
Extracted cloud saves feature.
If we have another (doesn't matter which platform) Just add a new cloud save platform file and configure it in the list.
Summary by cubic
Streamlines cloud save sync before launch and uploads after exit across Steam, GOG, and Epic with one modular flow. We run all matching platforms in order; the first non-proceed outcome decides continue, show dialog, or retry.
Refactors
New Features
Written for commit d810fe1. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements