Fix silent cloud save overwrite when sync cache is missing#1169
Merged
utkarshdalal merged 4 commits intoApr 13, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces an upgrade-gated conflict check with a cache-presence-based check: if the sync cache is absent/empty and local user files exist, the code now unconditionally treats this as a local-change conflict and sets the conflict UFS version; tests expanded to cover many cache/local/cloud scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SteamAutoCloud
participant SyncDB
participant LocalFS
participant CloudAPI
Client->>SteamAutoCloud: trigger syncUserFiles()
SteamAutoCloud->>SyncDB: read cached change numbers & snapshot
SteamAutoCloud->>LocalFS: list local user files
alt cache present
SteamAutoCloud->>CloudAPI: fetch cloud change number & file list
CloudAPI-->>SteamAutoCloud: cloud change number + file list
SteamAutoCloud->>SyncDB: compare change numbers -> decide download/upload/up-to-date
else cache absent or empty AND local files exist (new flow)
SteamAutoCloud->>SteamAutoCloud: set hasLocalChanges = true\nconflictUfsVersion = CURRENT_UFS_PARSE_VERSION
SteamAutoCloud->>Client: surface conflict handling / Conflict result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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
Sync Decision Matrix — Test Coverage
Key:
|
Contributor
Author
Sync Decision Matrix — Test Coverage
Key: Scenarios 1, 7, 9, 10 are the bug fixes — previously these silently downloaded cloud saves instead of asking the user. |
![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 22 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/SteamService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamService.kt:2653">
P2: runBlocking without IO dispatcher can block the caller thread during Room I/O; this helper is invoked from UI flows, risking main-thread jank/ANR.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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 (1)
app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt (1)
1570-1577: UsetoResponseBody()consistently in the new download fixtures.These branches reintroduce the deprecated
ResponseBody.create(...)right after other tests in the same file switched totoResponseBody(). In OkHttp 4,ResponseBody.create()is deprecated in favor of thetoResponseBody()extension function. Standardizing on the modern API avoids technical debt and prevents future cleanup churn.Suggested cleanup
- val responseBody = ResponseBody.create(null, cloudContent) + val responseBody = cloudContent.toResponseBody()Also applies to: 1704-1711, 1953-1960, 2022-2029
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt` around lines 1570 - 1577, Replace the deprecated ResponseBody.create(...) usage with the OkHttp extension toResponseBody() for the new download fixtures: when building the mocked response in the block that assigns responseBody and constructs Response via Response.Builder (the variables responseBody, cloudContent and the Response.Builder() call), call cloudContent.toResponseBody(null) (or with an appropriate MediaType) and pass that into .body(...) so the code uses the modern OkHttp 4 API consistently; apply the same change to the other similar blocks referenced (around lines noted in the review).
🤖 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/PrefManager.kt`:
- Around line 481-488: The getter/setter for PrefManager.pendingCloudResync is
non-atomic and risks lost updates under concurrent reads/writes; implement
atomic mutation helpers (e.g., PrefManager.addPendingCloudResync(gameId: Int)
and PrefManager.removePendingCloudResync(gameId: Int)) that perform
DataStore.updateData transactions to read, modify (add/remove the ID) and write
the updated set atomically, and replace all direct assignments like
"PrefManager.pendingCloudResync = PrefManager.pendingCloudResync ± gameId" in
ContainerUtils and PluviaMain with calls to these new methods (refer to the
existing pendingCloudResync property name when locating code to change).
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 2647-2655: The clearCloudSyncCache function currently blocks
callers via runBlocking; change its signature to a suspend fun
clearCloudSyncCache(context: Context, appId: Int) and remove runBlocking, then
perform the delete on the IO dispatcher (e.g., withContext(Dispatchers.IO) {
dao.deleteByAppId(appId) }) or call the Room suspend DAO directly; update
callers (e.g., places that use ContainerUtils.applyToContainer(...)) to invoke
this suspend API from the existing background save/sync coroutine path that
shares the same serialization boundary used by SteamAutoCloud.syncUserFiles to
avoid UI blocking and race conditions with the sync flow.
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt`:
- Around line 256-267: In the SyncResult.Conflict branch inside
SteamAppScreen.kt you’re calling Date(syncResult.localTimestamp).toString() and
Date(syncResult.remoteTimestamp).toString(); replace those with a locale-aware
date/time formatter and format the timestamps with the device locale and 12/24
settings (e.g. use java.text.DateFormat.getDateTimeInstance(...) or Android’s
android.text.format.DateFormat/DateUtils to produce localized strings) before
passing them into MessageDialogState; update the localDate and remoteDate
assignments used in showInstallDialog so the dialog displays locale-aware
timestamps.
In `@app/src/main/res/values-es/strings.xml`:
- Line 30: Update the string with name steam_verify_files_message_local_saves to
reference the setting label explicitly by quoting or renaming it as "Solo
guardados locales" (e.g., La sincronización en la nube se omitirá durante la
verificación porque "Solo guardados locales" está activado.) so the option name
appears clearly and matches the conflict-dialog phrasing.
In `@app/src/main/res/values-fr/strings.xml`:
- Line 30: The French string steam_verify_files_message_local_saves has
incorrect verb/adjective agreement; update the sentence so its verbs and
adjectives agree in plural with "sauvegardes" (for example rephrase to "La
synchronisation cloud sera ignorée pendant la vérification car seules les
sauvegardes locales sont activées.") — edit the string value for
steam_verify_files_message_local_saves accordingly.
In `@app/src/main/res/values-it/strings.xml`:
- Line 30: The Italian string steam_verify_files_message_local_saves has
incorrect number/agreement and awkward word order; update its value to use
correct plural agreement and natural phrasing, e.g. replace the text with: "La
sincronizzazione cloud verrà saltata durante la verifica perché sono attivi solo
i salvataggi locali." Ensure you modify the string entry for
steam_verify_files_message_local_saves accordingly.
- Around line 1363-1364: The message string cloud_sync_local_saves_only_message
is awkwardly phrased; update its value to explicitly reference the "Solo
salvataggi locali" option for clearer Italian (modify the <string
name="cloud_sync_local_saves_only_message"> entry) so it reads naturally and
asks which saves to keep while mentioning the option name; keep the translation
concise and user-friendly.
In `@app/src/main/res/values-pt-rBR/strings.xml`:
- Line 1079: The string resource steam_verify_files_message_local_saves has
grammatical agreement errors; update its value to a natural Portuguese phrase
such as replacing "A sincronização na nuvem será ignorada durante a verificação
porque apenas saves locals está ativado." with a corrected sentence like "A
sincronização na nuvem será ignorada durante a verificação porque apenas os
saves locais estão ativados." to fix article and verb agreement while keeping
the same string name.
In `@app/src/main/res/values-ro/strings.xml`:
- Line 30: The Romanian string steam_verify_files_message_local_saves uses “este
activat” with incorrect agreement; update the text to explicitly mention the
option and use feminine agreement (e.g., “opțiunea ... este activată”) so the
setting reads correctly; locate the string name
steam_verify_files_message_local_saves (and the duplicate occurrence at the
other keys around the same block) and replace the phrase “este activat” with
“opțiunea … este activată” preserving the rest of the sentence and punctuation.
---
Nitpick comments:
In `@app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt`:
- Around line 1570-1577: Replace the deprecated ResponseBody.create(...) usage
with the OkHttp extension toResponseBody() for the new download fixtures: when
building the mocked response in the block that assigns responseBody and
constructs Response via Response.Builder (the variables responseBody,
cloudContent and the Response.Builder() call), call
cloudContent.toResponseBody(null) (or with an appropriate MediaType) and pass
that into .body(...) so the code uses the modern OkHttp 4 API consistently;
apply the same change to the other similar blocks referenced (around lines noted
in the review).
🪄 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: cd39f151-fedb-4952-90ff-595e70dea28e
📒 Files selected for processing (22)
app/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/service/SteamAutoCloud.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/res/values-da/strings.xmlapp/src/main/res/values-de/strings.xmlapp/src/main/res/values-es/strings.xmlapp/src/main/res/values-fr/strings.xmlapp/src/main/res/values-it/strings.xmlapp/src/main/res/values-ko/strings.xmlapp/src/main/res/values-pl/strings.xmlapp/src/main/res/values-pt-rBR/strings.xmlapp/src/main/res/values-ro/strings.xmlapp/src/main/res/values-ru/strings.xmlapp/src/main/res/values-uk/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values-zh-rTW/strings.xmlapp/src/main/res/values/strings.xmlapp/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt
Comment on lines
+256
to
+267
| SyncResult.Conflict -> { | ||
| val localDate = Date(syncResult.localTimestamp).toString() | ||
| val remoteDate = Date(syncResult.remoteTimestamp).toString() | ||
| withContext(Dispatchers.Main) { | ||
| showInstallDialog( | ||
| gameId, | ||
| MessageDialogState( | ||
| visible = true, | ||
| type = DialogType.SYNC_CONFLICT, | ||
| title = context.getString(R.string.main_save_conflict_title), | ||
| message = context.getString(R.string.main_save_conflict_message, localDate, remoteDate), | ||
| confirmBtnText = context.getString(R.string.main_keep_remote), |
Contributor
There was a problem hiding this comment.
Use locale-aware timestamps in the conflict dialog.
Date.toString() ignores the device’s locale and 12/24-hour settings, so this new prompt will show raw JVM-formatted dates even in translated builds.
Suggested fix
+import java.text.DateFormat
import java.util.Date
...
- val localDate = Date(syncResult.localTimestamp).toString()
- val remoteDate = Date(syncResult.remoteTimestamp).toString()
+ val formatter = DateFormat.getDateTimeInstance(
+ DateFormat.MEDIUM,
+ DateFormat.SHORT,
+ )
+ val localDate = formatter.format(Date(syncResult.localTimestamp))
+ val remoteDate = formatter.format(Date(syncResult.remoteTimestamp))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| SyncResult.Conflict -> { | |
| val localDate = Date(syncResult.localTimestamp).toString() | |
| val remoteDate = Date(syncResult.remoteTimestamp).toString() | |
| withContext(Dispatchers.Main) { | |
| showInstallDialog( | |
| gameId, | |
| MessageDialogState( | |
| visible = true, | |
| type = DialogType.SYNC_CONFLICT, | |
| title = context.getString(R.string.main_save_conflict_title), | |
| message = context.getString(R.string.main_save_conflict_message, localDate, remoteDate), | |
| confirmBtnText = context.getString(R.string.main_keep_remote), | |
| SyncResult.Conflict -> { | |
| val formatter = DateFormat.getDateTimeInstance( | |
| DateFormat.MEDIUM, | |
| DateFormat.SHORT, | |
| ) | |
| val localDate = formatter.format(Date(syncResult.localTimestamp)) | |
| val remoteDate = formatter.format(Date(syncResult.remoteTimestamp)) | |
| withContext(Dispatchers.Main) { | |
| showInstallDialog( | |
| gameId, | |
| MessageDialogState( | |
| visible = true, | |
| type = DialogType.SYNC_CONFLICT, | |
| title = context.getString(R.string.main_save_conflict_title), | |
| message = context.getString(R.string.main_save_conflict_message, localDate, remoteDate), | |
| confirmBtnText = context.getString(R.string.main_keep_remote), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt`
around lines 256 - 267, In the SyncResult.Conflict branch inside
SteamAppScreen.kt you’re calling Date(syncResult.localTimestamp).toString() and
Date(syncResult.remoteTimestamp).toString(); replace those with a locale-aware
date/time formatter and format the timestamps with the device locale and 12/24
settings (e.g. use java.text.DateFormat.getDateTimeInstance(...) or Android’s
android.text.format.DateFormat/DateUtils to produce localized strings) before
passing them into MessageDialogState; update the localDate and remoteDate
assignments used in showInstallDialog so the dialog displays locale-aware
timestamps.
Contributor
Author
There was a problem hiding this comment.
pre-existing, we can unify this once this work has been merged
Contributor
There was a problem hiding this comment.
@jeremybernstein, understood — I'll note this as a pre-existing pattern to address in a follow-up. Thanks for the context!
(´• ω •`) ♡
🧠 Learnings used
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 1146
File: app/src/main/java/app/gamenative/service/SteamAutoCloud.kt:832-840
Timestamp: 2026-04-08T12:05:53.660Z
Learning: In `app/src/main/java/app/gamenative/service/SteamAutoCloud.kt`, the `isCacheCleared` branch (around lines 832-840) intentionally reads and removes `appInfo.id` from `PrefManager.pendingCloudResync` at that point — before conflict resolution — because the presence/absence of the ID determines which dialog wording to display (upgrade-specific `conflictUfsVersion` vs generic local-saves-only text). This classification must happen pre-dialog. Do not flag this as needing to be deferred until after the resync succeeds in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 1146
File: app/src/main/java/app/gamenative/utils/ContainerUtils.kt:474-482
Timestamp: 2026-04-08T12:06:10.401Z
Learning: In `app/src/main/java/app/gamenative/utils/ContainerUtils.kt`, within `applyToContainer(...)`, Steam-specific side-effect calls (e.g. `SteamService.getAppDirPath`, `MarkerUtils.removeMarker`, `SteamService.clearCloudSyncCache`, `PrefManager.pendingCloudResync`) are intentionally NOT guarded by an explicit `GameSource.STEAM` check. This is consistent with the existing pattern in the function. For non-Steam containers these calls are no-ops (DAO deletes find no rows; resync entries for non-Steam IDs are never consumed). Do not flag the absence of a Steam-source guard in this function as an issue in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 801
File: app/src/main/java/app/gamenative/ui/util/ContainerConfigTransfer.kt:105-143
Timestamp: 2026-03-11T07:31:50.139Z
Learning: In `app/src/main/java/app/gamenative/ui/util/ContainerConfigTransfer.kt`, the force-apply path inside `BaseAppScreen.showMissingComponentsDialog`'s `onApplyAnyway` callback (re-parsing with `forceApply = true`, resolving manifest installs, and writing the container) is intentional and user-initiated. It only runs when the user explicitly taps "Apply anyway" in the missing-components dialog. Do not flag this as contradicting the reject-on-missing-components goal; the default is rejection and this is an explicit user override.
Learnt from: unbelievableflavour
Repo: utkarshdalal/GameNative PR: 810
File: app/src/main/java/app/gamenative/ui/screen/library/appscreen/CustomGameAppScreen.kt:391-417
Timestamp: 2026-03-18T08:52:14.786Z
Learning: Do not flag lifecycle-safety concerns for the pattern of launching coroutines with CoroutineScope(Dispatchers.IO).launch inside AppMenuOption click handlers (e.g., FetchSteamGridDBImages, CreateShortcut) in AppScreen files under app/src/main/java/app/gamenative/ui/screen/library/appscreen/. This is an established, intentional pattern in these files; treat it as a supported approach in reviews of these files.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 1136
File: app/src/main/java/app/gamenative/MainActivity.kt:273-284
Timestamp: 2026-04-07T21:36:12.480Z
Learning: In the GameNative repository (`utkarshdalal/GameNative`), `SteamService.keepAlive` is set to `true` in only one place: `ContainerConfigDialog.kt:1032` (inside a folder-picker flow). During the normal XServerScreen game-launch bootstrap (`XServerScreen.kt` lines ~1529-1680), `keepAlive` is `false` throughout. The bootstrap uses a raw Java `Executors.newSingleThreadExecutor().submit {}` (not a coroutine/LaunchedEffect), so Compose teardown does not cancel it. However, since `keepAlive` is `false` during bootstrap, a task-swipe in that window does not produce a stale `keepAlive=true` on the next launch, and therefore does not cause the "stuck on container screen" bug. Do not flag onDestroy's `if (SteamService.keepAlive)` guard as failing to cover the bootstrap-window race in future reviews.
Learnt from: utkarshdalal
Repo: utkarshdalal/GameNative PR: 344
File: app/src/main/java/app/gamenative/service/SteamAutoCloud.kt:415-419
Timestamp: 2025-12-17T05:14:05.133Z
Learning: In SteamAutoCloud.kt, when uploading files to Steam Cloud API (beginFileUpload and commitFileUpload), the filename parameter intentionally uses different formats: `file.path + file.filename` (relative path without placeholder) when `appInfo.ufs.saveFilePatterns.isEmpty()` is true (fallback case), and `file.prefixPath` (includes placeholder like %SteamUserData%) when patterns are configured. This difference is by design.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 1136
File: app/src/main/java/app/gamenative/MainActivity.kt:147-150
Timestamp: 2026-04-07T21:37:25.757Z
Learning: In `app/src/main/java/app/gamenative/MainActivity.kt`, the `onCreate` stale-session check `if (SteamService.keepAlive && PluviaApp.xEnvironment == null)` is intentionally kept as-is. The only non-game flow that sets `keepAlive = true` with `xEnvironment == null` is the folder-picker in `ContainerConfigDialog.kt` (line ~1032); if the app is killed in that state, resetting `keepAlive = false` on the next `onCreate` is harmless. Do not flag this guard as incorrectly matching non-XServer flows in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 0
File: :0-0
Timestamp: 2026-03-18T16:30:26.610Z
Learning: In `app/src/main/java/app/gamenative/ui/PluviaMain.kt`, the three sites that call `MainActivity.setPendingLaunchRequest`, toggle `shownPendingLaunchSnackbar`, and conditionally call `SnackbarManager.show` (cold-start pending branch, ExternalGameLaunch pending branch, and non-terminal SteamDisconnected branch) are intentionally kept separate — the surrounding logic differs enough that extracting a shared helper would add more complexity than it removes. Do not suggest refactoring these into a helper in future reviews.
Learnt from: utkarshdalal
Repo: utkarshdalal/GameNative PR: 159
File: app/src/main/java/app/gamenative/service/DownloadService.kt:6-6
Timestamp: 2025-09-19T17:07:27.941Z
Learning: In app/src/main/java/app/gamenative/service/DownloadService.kt, the getSizeFromStoreDisplay() function uses runBlocking to call SteamService.getDownloadableDepots() because the latter was converted from synchronous to suspend in this PR. The user confirmed the previous synchronous implementation worked fine, and if performance is acceptable, keeping runBlocking is a valid approach.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 801
File: app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt:174-188
Timestamp: 2026-03-10T22:11:59.430Z
Learning: In SteamAppScreen.kt, the applyConfigForContainer function calls installMissingComponentsForConfig before BestConfigService.parseConfigToContainerData. This ordering is pre-existing behavior and not introduced by any recent PR. Do not flag this specific call order as an issue in future reviews of this file.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 1005
File: app/src/main/java/app/gamenative/ui/component/dialog/ControllerPresetManager.kt:54-166
Timestamp: 2026-03-24T12:55:22.905Z
Learning: In `app/src/main/java/app/gamenative/ui/component/dialog/ControllerPresetManager.kt`, the three hardcoded factory presets (`PRESET_DEFAULT`, `PRESET_FPS`, `PRESET_PLATFORMER`) with embedded English names are a known, pre-existing limitation. The lack of centralization with profile assets and the absence of localized display names are acknowledged but intentionally out of scope. Do not flag the hardcoded English preset names or the duplication between factory presets and seeded profile assets as issues in future reviews of this file.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 801
File: app/src/main/java/app/gamenative/utils/BestConfigService.kt:45-51
Timestamp: 2026-03-10T21:46:14.981Z
Learning: In `app/src/main/java/app/gamenative/utils/BestConfigService.kt`, the singleton mutable state pattern (`lastMissingComponents` / `consumeLastMissingComponents()`) — where `parseConfigToContainerData()` writes a global and callers read it via `consume*()` — is a pre-existing design. Refactoring it to return missing components as part of the parse result has a large blast radius and is intentionally deferred. Do not flag this pattern as an issue in future reviews of this file.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 1005
File: app/src/main/java/app/gamenative/ui/component/dialog/PhysicalControllerConfigSection.kt:706-738
Timestamp: 2026-03-24T12:56:02.025Z
Learning: In `app/src/main/java/app/gamenative/ui/component/dialog/PhysicalControllerConfigSection.kt`, the `needsCopy` heuristic in `copyElementsIfNeeded` (around lines 706-738) treats any destination layout lacking `GAMEPAD_*` bindings as safe to overwrite with source elements, even if it contains non-GAMEPAD custom bindings. This imperfect heuristic is a pre-existing limitation acknowledged by the maintainer and intentionally out of scope. Do not flag this overwrite-without-merge behavior as an issue in future reviews of this file.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 0
File: :0-0
Timestamp: 2026-03-27T10:36:22.491Z
Learning: In `app/src/main/java/app/gamenative/service/SteamService.kt`, around lines 1549-1559, a full integration regression test for the `CaseInsensitiveFileSystem` wired into `DepotDownloader` is not feasible: `DepotDownloader` connects to a real online depot server that cannot be controlled in tests. The appropriate test strategy is a simulated-response fixture that exercises the filesystem wiring directly. Such tests only pass on case-sensitive filesystems (Linux CI) and should guard with an `assumeTrue(isCaseSensitiveFilesystem())` check. Do not ask for a live-depot integration fixture at this constructor site in future reviews.
Learnt from: jeremybernstein
Repo: utkarshdalal/GameNative PR: 581
File: app/src/main/java/app/gamenative/ui/PluviaMain.kt:1154-1176
Timestamp: 2026-02-21T22:26:41.871Z
Learning: In Kotlin coroutines, suspend functions automatically suspend and resume within a coroutine scope. Only Deferred objects produced by async { ... } require .await() to obtain the result. Do not call .await() on direct suspend function calls or on non-Deferred results; use await() only when you have a Deferred that represents a value you need.
Learnt from: phobos665
Repo: utkarshdalal/GameNative PR: 1072
File: app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt:1309-1315
Timestamp: 2026-04-02T07:43:14.973Z
Learning: In the GameNative repository, when logging/telemetry includes Wine prefix/container paths that contain the hardcoded Wine username `xuser`, do not treat that as a PII/security issue (it is not a real user identity and contains no personally identifiable information). Only flag PII concerns for other user-identifying data not limited to the `xuser` hardcoded Wine username in prefix paths.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt (1)
1441-1452: Fail fast on unexpected prefixes in the shared resolver.The
else -> tempDir.absolutePathfallback can mask a broken prefix contract and make these decision-matrix tests exercise the wrong directory instead of failing where the regression starts. Prefer throwing on unknown prefixes here.Suggested change
private fun makePrefixToPath(): (String) -> String = { prefix -> // getPathTypePairs passes placeholders with % signs, production uses PathType.from() val stripped = prefix.removePrefix("%").removeSuffix("%") when (stripped) { "WinMyDocuments" -> { val imageFs = ImageFs.find(context) val wineprefix = File(imageFs.wineprefix) File(wineprefix, "dosdevices/c:/users/xuser/Documents").absolutePath } - else -> tempDir.absolutePath + else -> error("Unexpected prefix in test: $prefix") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt` around lines 1441 - 1452, The test helper makePrefixToPath currently falls back to tempDir.absolutePath for unknown prefixes which hides regressions; change it to throw an explicit exception when an unexpected prefix is encountered so tests fail fast: in makePrefixToPath (the lambda taking prefix) validate and strip the percent signs as now, then replace the else -> tempDir.absolutePath branch with throwing an IllegalArgumentException (or AssertionError) that includes the received prefix string to make failures obvious.
🤖 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/test/java/app/gamenative/service/SteamAutoCloudTest.kt`:
- Around line 2077-2101: The test mutates the singleton
PrefManager.pendingCloudResync and currently resets it only at the end of the
test body, which leaks state if an assertion fails; fix by ensuring reset always
runs: either move the cleanup into a dedicated `@After` teardown method that sets
PrefManager.pendingCloudResync = emptySet(), or wrap the mutation and assertions
in a try/finally inside the test so PrefManager.pendingCloudResync is restored
in the finally block; reference the existing use of
PrefManager.pendingCloudResync in this test and apply the guarantee-reset
pattern to prevent cross-test state leakage.
---
Nitpick comments:
In `@app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt`:
- Around line 1441-1452: The test helper makePrefixToPath currently falls back
to tempDir.absolutePath for unknown prefixes which hides regressions; change it
to throw an explicit exception when an unexpected prefix is encountered so tests
fail fast: in makePrefixToPath (the lambda taking prefix) validate and strip the
percent signs as now, then replace the else -> tempDir.absolutePath branch with
throwing an IllegalArgumentException (or AssertionError) that includes the
received prefix string to make failures obvious.
🪄 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: d689e7ff-8709-4a70-9698-d072f29cf3c1
📒 Files selected for processing (1)
app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt
jeremybernstein
force-pushed
the
jb/fix-cloud-overwrite-after-update
branch
from
e72ffa5 to
f60ca63
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/res/values-ro/strings.xml (1)
30-30:⚠️ Potential issue | 🟡 MinorPolish the Romanian local-saves-only copy for consistency.
Line 30 still has a mismatched closing quote, and Line 1373 still uses the older awkward wording (
este activat). Please align both strings on the same phrasing.✍️ Suggested wording
- <string name="steam_verify_files_message_local_saves">Sincronizarea cloud va fi omisă în timpul verificării deoarece opțiunea „Doar salvări locale" este activată.</string> + <string name="steam_verify_files_message_local_saves">Sincronizarea cloud va fi omisă în timpul verificării deoarece opțiunea „Doar salvări locale” este activată.</string> ... - <string name="cloud_sync_local_saves_only_message">"Doar salvări locale" este activat pentru acest joc. Salvările din cloud pot fi diferite de cele locale. Pe care vrei să le păstrezi?</string> + <string name="cloud_sync_local_saves_only_message">Opțiunea „Doar salvări locale” este activată pentru acest joc. Salvările din cloud pot fi diferite de cele locale. Pe care vrei să le păstrezi?</string>Also applies to: 1372-1373
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/res/values-ro/strings.xml` at line 30, Fix the mismatched closing quote and unify the phrasing: update the string steam_verify_files_message_local_saves to use matching Romanian typographic quotes (e.g., „Doar salvări locale”) instead of mixed quotes, and change the ending from "este activat" to the correct gendered form "este activată"; apply the same revised phrasing to the other corresponding string that currently uses "este activat" so both entries match exactly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/res/values-ro/strings.xml`:
- Line 30: Fix the mismatched closing quote and unify the phrasing: update the
string steam_verify_files_message_local_saves to use matching Romanian
typographic quotes (e.g., „Doar salvări locale”) instead of mixed quotes, and
change the ending from "este activat" to the correct gendered form "este
activată"; apply the same revised phrasing to the other corresponding string
that currently uses "este activat" so both entries match exactly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb0b87ca-7edb-41e1-a3f0-9733e62d4a21
📒 Files selected for processing (8)
app/src/main/java/app/gamenative/service/SteamAutoCloud.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/res/values-es/strings.xmlapp/src/main/res/values-fr/strings.xmlapp/src/main/res/values-it/strings.xmlapp/src/main/res/values-pt-rBR/strings.xmlapp/src/main/res/values-ro/strings.xmlapp/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt
✅ Files skipped from review due to trivial changes (4)
- app/src/main/res/values-es/strings.xml
- app/src/main/res/values-it/strings.xml
- app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt
- app/src/main/res/values-fr/strings.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/res/values-pt-rBR/strings.xml
- app/src/main/java/app/gamenative/service/SteamService.kt
jeremybernstein
force-pushed
the
jb/fix-cloud-overwrite-after-update
branch
from
f60ca63 to
04769d5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test/java/app/gamenative/service/SteamAutoCloudTest.kt`:
- Around line 79-85: The test currently sets PrefManager.dataStore via
reflection but then calls PrefManager.init(context) which overwrites it with
context.datastore; fix by calling PrefManager.init(context) first and then using
the reflected dataStoreField.set(PrefManager, mockDataStore) to replace the real
store, or alternatively modify PrefManager.init to accept an injectable
DataStore parameter and pass mockDataStore; ensure you reference the
PrefManager.init(context) call and the reflective
dataStoreField.set(PrefManager, mockDataStore) so the mock DataStore remains in
place for the test.
🪄 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: 3bd88087-549c-4e65-9913-3424eda52855
📒 Files selected for processing (1)
app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt
…s of change number the old gate (localAppChangeNumber >= 0) skipped the conflict dialog for first-time offline players whose change number is -1. this meant local saves were silently overwritten by cloud on reconnect. remove the gate: if cache is absent and local files exist, always treat as conflict.
12 scenarios covering: cache present/absent, local changes/none, cloud ahead/same, preferred save location, first-time offline play. also fixes existing download test (cloud filenames, mock params, deprecated API).
jeremybernstein
force-pushed
the
jb/fix-cloud-overwrite-after-update
branch
from
1a249e9 to
c9a120d
Compare
jeremybernstein
changed the title
9 tasks
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Owner
|
Good catch. I think showing the conflict dialog always when there is no cache may be overkill though, and concering to users. I think a better question to ask is why would we not have the cache? Is there a way to force the cache when there are local changes? Maybe the cache should be persisted somewhere? Best to try to align with how Steam does it, that's what we do right now |
Contributor
Author
The issue with the cache is that we can't reliably reconstruct it from local files: we don't know what changed (there might have been online changes while we were offline, too). So in this situation, the app update blew away the cache. If we reconstruct the cache, we're saying "these files haven't changed since last sync." And then when cloud is ahead:
Which is exactly the bug we're fixing. The user may have played offline or on this device since the last sync. By rebuilding the cache from current files, we're asserting "nothing changed locally" which we don't/can't know. The only safe options when cache is absent are:
Then, to your other question: I think we're already doing what Steam does. Pretty sure that, even if we write the change number on boot, the cache would still be absent in this situation (cleared by upgrade, never existed). And without the cache, we still can't tell if local files changed, and we still need the conflict dialog. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt (1)
1545-2152: Consider extracting shared download stubs to reduce test drift.
FileDownloadInfo/OkHttpsetup is repeated across several scenarios. A tiny helper would make intent clearer and lower maintenance when the download path changes.♻️ Example helper extraction
+private fun stubSingleDownload(content: ByteArray, urlPath: String = "/download/file1") { + val mockDownloadInfo = mock<FileDownloadInfo>() + whenever(mockDownloadInfo.urlHost).thenReturn("test.example.com") + whenever(mockDownloadInfo.urlPath).thenReturn(urlPath) + whenever(mockDownloadInfo.useHttps).thenReturn(true) + whenever(mockDownloadInfo.requestHeaders).thenReturn(emptyList()) + whenever(mockDownloadInfo.fileSize).thenReturn(content.size) + whenever(mockDownloadInfo.rawFileSize).thenReturn(content.size) + every { mockSteamCloud.clientFileDownload(any(), any(), any(), any(), any()) } returns + CompletableFuture.completedFuture(mockDownloadInfo) + + val mockHttpClient = mock<OkHttpClient>() + val mockCall = mock<Call>() + whenever(mockHttpClient.newCall(any())).thenReturn(mockCall) + whenever(mockCall.execute()).thenReturn( + Response.Builder() + .request(okhttp3.Request.Builder().url("https://test.example.com$urlPath").build()) + .protocol(Protocol.HTTP_1_1) + .code(200) + .message("OK") + .body(content.toResponseBody()) + .build() + ) + val mockConfig = mock<SteamConfiguration>() + whenever(mockSteamService.steamClient!!.configuration).thenReturn(mockConfig) + whenever(mockConfig.httpClient).thenReturn(mockHttpClient) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt` around lines 1545 - 2152, Many tests repeat the same FileDownloadInfo + OkHttp client/Call stubbing (mockDownloadInfo, mockHttpClient, mockCall and the every { mockSteamCloud.clientFileDownload(...) } / whenever(mockCall.execute()) blocks); extract a small helper inside the test class (e.g. stubDownload(content: ByteArray): FileDownloadInfo or setupDownloadStubs(contentList: List<ByteArray>)) that creates and returns the mocked FileDownloadInfo(s), configures mockSteamCloud.clientFileDownload to return them, and wires mockHttpClient.newCall / mockCall.execute to return responses for the given content(s), then replace the repeated inline stubbing in tests (those around makeCloudFileChangeList and calls to SteamAutoCloud.syncUserFiles) with calls to the helper to reduce duplication and maintenance.
🤖 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/test/java/app/gamenative/service/SteamAutoCloudTest.kt`:
- Around line 1545-2152: Many tests repeat the same FileDownloadInfo + OkHttp
client/Call stubbing (mockDownloadInfo, mockHttpClient, mockCall and the every {
mockSteamCloud.clientFileDownload(...) } / whenever(mockCall.execute()) blocks);
extract a small helper inside the test class (e.g. stubDownload(content:
ByteArray): FileDownloadInfo or setupDownloadStubs(contentList:
List<ByteArray>)) that creates and returns the mocked FileDownloadInfo(s),
configures mockSteamCloud.clientFileDownload to return them, and wires
mockHttpClient.newCall / mockCall.execute to return responses for the given
content(s), then replace the repeated inline stubbing in tests (those around
makeCloudFileChangeList and calls to SteamAutoCloud.syncUserFiles) with calls to
the helper to reduce duplication and maintenance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75bb39d9-e204-4307-be68-062e989a7884
📒 Files selected for processing (1)
app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt
Contributor
Author
|
@utkarshdalal I think this is good to go. I'm ignoring the nitpick from CR. |
linkq
pushed a commit
to linkq/GameNative
that referenced
this pull request
Apr 13, 2026
…lal#1169) * fix: show conflict dialog when cloud sync cache is missing, regardless of change number the old gate (localAppChangeNumber >= 0) skipped the conflict dialog for first-time offline players whose change number is -1. this meant local saves were silently overwritten by cloud on reconnect. remove the gate: if cache is absent and local files exist, always treat as conflict. * test: cloud sync decision matrix covering all cache/CN/cloud states 12 scenarios covering: cache present/absent, local changes/none, cloud ahead/same, preferred save location, first-time offline play. also fixes existing download test (cloud filenames, mock params, deprecated API). * fix: split cache-absent conflict into upgrade vs first-offline cases * test: revert modifications to existing tests, keep only new additions
This was referenced Apr 15, 2026
xXJSONDeruloXx
pushed a commit
to xXJSONDeruloXx/GameNative
that referenced
this pull request
Apr 28, 2026
…lal#1169) * fix: show conflict dialog when cloud sync cache is missing, regardless of change number the old gate (localAppChangeNumber >= 0) skipped the conflict dialog for first-time offline players whose change number is -1. this meant local saves were silently overwritten by cloud on reconnect. remove the gate: if cache is absent and local files exist, always treat as conflict. * test: cloud sync decision matrix covering all cache/CN/cloud states 12 scenarios covering: cache present/absent, local changes/none, cloud ahead/same, preferred save location, first-time offline play. also fixes existing download test (cloud filenames, mock params, deprecated API). * fix: split cache-absent conflict into upgrade vs first-offline cases * test: revert modifications to existing tests, keep only new additions
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
Fix silent cloud save overwrite when sync cache is missing.
The old
isUpgradeConflictguard requiredlocalAppChangeNumber >= 0, whichskipped the conflict dialog for first-time offline players (changeNumber = -1).
Their local saves were silently overwritten by cloud on reconnect.
The fix: if cache is absent and local files exist, always treat as conflict —
regardless of change number. The user picks which save to keep.
One production line changed. 12 test scenarios covering the full sync decision
matrix (cache present/absent × local changes/none × cloud ahead/same ×
preferred save location).
Closes #1168
Recording
n/a — backend sync logic, no UI change
Test plan
./gradlew testDebugUnitTest --tests "*.SteamAutoCloudTest"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
Bug Fixes
Tests