Re-enable local saves only with cloud sync safety nets#1146
Open
jeremybernstein wants to merge 4 commits intoutkarshdalal:masterfrom
Open
Re-enable local saves only with cloud sync safety nets#1146jeremybernstein wants to merge 4 commits intoutkarshdalal:masterfrom
jeremybernstein wants to merge 4 commits intoutkarshdalal:masterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a per-game "local saves only" toggle and integrates it into cloud-sync flows: re-enables the UI switch, adds persisted pending-resync tracking, provides a Hilt entrypoint + helper to clear per-app cloud-cache, updates force-sync/conflict flows to surface choices for local-only cases, adjusts auto-cloud detection, expands localized strings, and adds tests. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Screen as SteamAppScreen
participant Dialog as SyncConflictDialog
participant Service as SteamService
participant DB as FileChangeListsDao
participant Pref as PrefManager
participant Cloud as Steam Cloud
User->>Screen: Tap ForceCloudSync
Screen->>Screen: Check container.isLocalSavesOnly
alt local-saves-only
Screen->>Dialog: Show SYNC_CONFLICT
User->>Dialog: Choose Local or Remote
Dialog->>Screen: Confirm(choice)
Screen->>Service: performForceCloudSync(choice)
else normal
Screen->>Service: performForceCloudSync()
end
Service->>Pref: Check pendingCloudResync(gameId)
Service->>DB: Optionally clear/read cache (deleteByAppId)
Service->>Cloud: forceSyncUserFiles(prefixToPath, chosenLocation)
Cloud->>Service: Return SyncResult (Success/UpToDate/Conflict)
alt Conflict
Service->>Screen: Show conflict dialog / open install dialog
Service->>Pref: Remove gameId from pendingCloudResync (when dialog shown)
else Success/UpToDate
Service->>Screen: Show success/up-to-date snackbar
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 19 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:2638">
P2: Cloud-sync cache invalidation silently no-ops when `SteamService.instance` is null, which can skip the intended toggle-off cache reset path.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/SteamService.kt:2639">
P2: `clearCloudSyncCache` is fire-and-forget, so cache deletion may not complete before subsequent sync reads stale cache state.</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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 2637-2644: The clearCloudSyncCache currently launches
deleteByAppId asynchronously so callers (e.g., ContainerUtils.applyToContainer)
may proceed to setLocalSavesOnly(false) before deletion finishes; change
clearCloudSyncCache to be awaitable: replace the fire-and-forget launch with a
suspend API (e.g., suspend fun clearCloudSyncCache(appId: Int)) or have it
return the Job/Deferred so the caller can await completion, call the DAO
deleteByAppId directly (or with withContext if needed) inside that suspend
function, and update the caller in ContainerUtils.applyToContainer to invoke and
await clearCloudSyncCache before calling setLocalSavesOnly(false).
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt`:
- Around line 1112-1126: The verify-files flow currently skips cloud sync when
container.isLocalSavesOnly but still shows the generic "upload to cloud first"
confirmation; update the confirmation shown in SteamAppScreen (the branch around
container.isLocalSavesOnly and SteamService.forceSyncUserFiles) to use a
different string when container.isLocalSavesOnly is true (e.g., a
local-saves-only-specific message or a generic "please back up your saves before
verifying" copy), add/replace the appropriate string resource, and ensure the UI
path that previously referenced the cloud-upload wording now conditionally
selects the new local-saves-only string.
- Around line 257-270: Replace the hardcoded SimpleDateFormat usage in the
SyncResult.Conflict branch with a locale-aware formatter: create a DateFormat
via DateFormat.getDateTimeInstance() and use it to format
syncResult.localTimestamp and syncResult.remoteTimestamp before constructing the
MessageDialogState for showInstallDialog; update the local variable currently
named fmt and ensure imports/reference to SimpleDateFormat are removed so the
dialog message shows user-localized date/time.
In `@app/src/main/java/app/gamenative/utils/ContainerUtils.kt`:
- Around line 474-478: The call site in applyToContainer currently calls
SteamService.clearCloudSyncCache(gameId) fire-and-forget, which may not finish
(or run at all if instance is null) before applyToContainer returns; change this
to awaitable, lifecycle-independent deletion by adding/using a synchronous or
suspend API on SteamService (e.g., clearCloudSyncCacheBlocking or suspend
clearCloudSyncCacheAwait) that ensures fileChangeListsDao.deleteByAppId(appId)
completes even if instance is not present, and call that from applyToContainer
so the delete finishes before returning; reference
SteamService.clearCloudSyncCache, SteamService.instance,
fileChangeListsDao.deleteByAppId, and the applyToContainer call site to
implement the blocking/suspend alternative and replace the fire-and-forget call.
In `@app/src/main/res/values-es/strings.xml`:
- Around line 1370-1371: Add the missing Spanish translations for the toggle
strings so the UI doesn't fall back to English: insert localized values for the
string resource names "local_saves_only" (setting label) and
"local_saves_only_description" (setting subtitle/description) into the
strings.xml file alongside the newly added conflict-dialog strings; use concise
Spanish copy matching tone of existing entries (e.g., "Solo guardados locales"
for the label and a short explanatory sentence for the description).
🪄 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: 861220f9-7b06-4f24-a64b-16542271d4c5
📒 Files selected for processing (19)
app/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.ktapp/src/main/java/app/gamenative/ui/enums/DialogType.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.xml
app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt
Show resolved
Hide resolved
jeremybernstein
force-pushed
the
jb/fix-local-saves-only
branch
from
6163e8e to
d89e4f9
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/utils/ContainerUtils.kt (1)
474-478:⚠️ Potential issue | 🟠 MajorCache invalidation can still silently no-op when service is not running.
At Line 477, this depends on
SteamService.clearCloudSyncCache(gameId), but that helper currently skips deletion whenSteamService.instanceis null. In that case, disabling local-saves-only won’t reliably force divergence detection on next sync.Suggested direction
- if (container.isLocalSavesOnly && !containerData.localSavesOnly) { + if (container.isLocalSavesOnly && !containerData.localSavesOnly) { val gameId = extractGameIdFromContainerId(container.id) - SteamService.clearCloudSyncCache(gameId) + SteamService.clearCloudSyncCacheBlocking(gameId) // lifecycle-independent, guaranteed delete }And make the SteamService API lifecycle-independent (not tied to
SteamService.instanceavailability) so this path is guaranteed.🤖 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/ContainerUtils.kt` around lines 474 - 478, The cache-clear call in ContainerUtils (when container.isLocalSavesOnly changes) can silently no-op because SteamService.clearCloudSyncCache(gameId) currently returns early if SteamService.instance is null; change the SteamService API so clearing the cloud-sync cache does not depend on runtime instance availability (either implement clearCloudSyncCache to perform deletion against the persistent cache/storage directly when instance==null, or add a lifecycle-independent method such as forceClearCloudSyncCache(gameId) and call that from ContainerUtils after extractGameIdFromContainerId(container.id)) so disabling local-saves-only always forces divergence detection on next sync.
🤖 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/java/app/gamenative/utils/ContainerUtils.kt`:
- Around line 474-478: The cache-clear call in ContainerUtils (when
container.isLocalSavesOnly changes) can silently no-op because
SteamService.clearCloudSyncCache(gameId) currently returns early if
SteamService.instance is null; change the SteamService API so clearing the
cloud-sync cache does not depend on runtime instance availability (either
implement clearCloudSyncCache to perform deletion against the persistent
cache/storage directly when instance==null, or add a lifecycle-independent
method such as forceClearCloudSyncCache(gameId) and call that from
ContainerUtils after extractGameIdFromContainerId(container.id)) so disabling
local-saves-only always forces divergence detection on next sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6498522c-9707-405e-91ce-f3994f51773f
📒 Files selected for processing (19)
app/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.ktapp/src/main/java/app/gamenative/ui/enums/DialogType.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.xml
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/app/gamenative/ui/enums/DialogType.kt
🚧 Files skipped from review as they are similar to previous changes (14)
- app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt
- app/src/main/java/app/gamenative/service/SteamService.kt
- app/src/main/res/values/strings.xml
- app/src/main/res/values-pl/strings.xml
- app/src/main/res/values-de/strings.xml
- app/src/main/res/values-zh-rTW/strings.xml
- app/src/main/res/values-ko/strings.xml
- app/src/main/res/values-pt-rBR/strings.xml
- app/src/main/res/values-fr/strings.xml
- app/src/main/res/values-zh-rCN/strings.xml
- app/src/main/res/values-uk/strings.xml
- app/src/main/res/values-es/strings.xml
- app/src/main/res/values-it/strings.xml
- app/src/main/res/values-ro/strings.xml
jeremybernstein
force-pushed
the
jb/fix-local-saves-only
branch
from
d89e4f9 to
7b47807
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/utils/ContainerUtils.kt`:
- Around line 475-478: Only clear the Steam cloud sync cache for real persisted
toggles off: wrap the existing condition in an additional check for the
persistence flag so it runs only when saveToDisk is true; i.e. change the logic
that uses container.isLocalSavesOnly and containerData.localSavesOnly in
ContainerUtils (the block calling SteamService.clearCloudSyncCache and
extractGameIdFromContainerId) to also require saveToDisk (or the actual
persistence parameter name) so temporary overrides (saveToDisk == false) do not
trigger cache invalidation.
In `@app/src/main/res/values-de/strings.xml`:
- Line 29: Update the German localized string identified by name
"steam_verify_files_message_local_saves": change the verb from "aktiviert ist"
to the correct plural form "aktiviert sind" so the message reads with proper
subject-verb agreement for "Speicherstände"; locate and edit the string value
for steam_verify_files_message_local_saves in the strings.xml entry and replace
the ending accordingly.
🪄 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: 6c04acc0-c617-47f0-9392-446f6f2c3c99
📒 Files selected for processing (19)
app/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.ktapp/src/main/java/app/gamenative/ui/enums/DialogType.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.xml
✅ Files skipped from review due to trivial changes (6)
- app/src/main/java/app/gamenative/ui/enums/DialogType.kt
- app/src/main/res/values-ko/strings.xml
- app/src/main/res/values-uk/strings.xml
- app/src/main/res/values-zh-rCN/strings.xml
- app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt
- app/src/main/res/values-it/strings.xml
🚧 Files skipped from review as they are similar to previous changes (8)
- app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt
- app/src/main/res/values-pl/strings.xml
- app/src/main/java/app/gamenative/service/SteamService.kt
- app/src/main/res/values-ro/strings.xml
- app/src/main/res/values-fr/strings.xml
- app/src/main/res/values-da/strings.xml
- app/src/main/res/values-pt-rBR/strings.xml
- app/src/main/res/values-zh-rTW/strings.xml
jeremybernstein
force-pushed
the
jb/fix-local-saves-only
branch
from
7b47807 to
f96ac80
Compare
Contributor
Author
|
added some unification of the dialogs in most recent force-push -- this add a bit of complexity for arguably a cleaner design. |
jeremybernstein
force-pushed
the
jb/fix-local-saves-only
branch
from
f96ac80 to
6388a7b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/app/gamenative/ui/PluviaMain.kt (1)
791-816:⚠️ Potential issue | 🟠 MajorReplay the original launch flags when resolving
SAVE_CONFLICT.These callbacks re-enter
preLaunchApp()with default options, so context from the original launch is lost. If this dialog was reached from anignorePendingOperations = truepath, the user can get bounced back into the pending-operations dialog; if it came from an external intent withuseTemporaryOverride = true, the retry launches with the persisted container config instead of the temporary override. Please stash the originalpreLaunchApp()args and reuse them for both Keep Remote and Keep Local.🤖 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/PluviaMain.kt` around lines 791 - 816, The SAVE_CONFLICT handlers are re-calling preLaunchApp with fresh/default args and losing original launch flags; capture the original preLaunchApp arguments when the dialog is shown (e.g., into a local/remembered data structure) and reuse them here, only overriding preferredSave to SaveLocation.Remote or SaveLocation.Local; call preLaunchApp(originalArgs.copy(preferredSave = ...)) (preserving flags like ignorePendingOperations, useTemporaryOverride, persisted container config, and the viewModel callbacks such as setLoadingDialogVisible, setLoadingDialogProgress, setLoadingDialogMessage, setMessageDialogState, and onSuccess = viewModel::launchApp), then clear msgDialogState as before.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/PrefManager.kt (1)
481-488: MakependingCloudResyncupdates atomic to avoid lost set mutations.Call sites perform read-modify-write operations:
ContainerUtils.kt:478, 482(pendingCloudResync = pendingCloudResync ± gameId) andSteamAutoCloud.kt:838(pendingCloudResync = pending - appInfo.id). Concurrent updates can race and clobber IDs. Wrap add/remove logic indataStore.edit { }transactions.Proposed refactor
- var pendingCloudResync: Set<Int> + val pendingCloudResync: Set<Int> get() { val raw = getPref(PENDING_CLOUD_RESYNC, "") return if (raw.isEmpty()) emptySet() else raw.split(',').mapNotNull { it.toIntOrNull() }.toSet() } - set(value) { - setPref(PENDING_CLOUD_RESYNC, value.joinToString(",")) - } + + fun addPendingCloudResync(appId: Int) { + scope.launch { + dataStore.edit { pref -> + val current = + pref[PENDING_CLOUD_RESYNC] + ?.split(',') + ?.mapNotNull { it.toIntOrNull() } + ?.toMutableSet() + ?: mutableSetOf() + current.add(appId) + pref[PENDING_CLOUD_RESYNC] = current.joinToString(",") + } + } + } + + fun removePendingCloudResync(appId: Int) { + scope.launch { + dataStore.edit { pref -> + val current = + pref[PENDING_CLOUD_RESYNC] + ?.split(',') + ?.mapNotNull { it.toIntOrNull() } + ?.toMutableSet() + ?: mutableSetOf() + current.remove(appId) + pref[PENDING_CLOUD_RESYNC] = current.joinToString(",") + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/PrefManager.kt` around lines 481 - 488, The current read-modify-write use of the pendingCloudResync property can race and lose updates; add atomic add/remove methods in PrefManager that perform the mutation inside dataStore.edit { } (e.g., fun addPendingCloudResync(gameId: Int) and fun removePendingCloudResync(gameId: Int)) where you read the current PENDING_CLOUD_RESYNC value, modify the Set<Int>, and write the new comma‑joined string back to the preference within the same transaction; then update callers in ContainerUtils.kt and SteamAutoCloud.kt to call these new methods instead of reading the property, modifying it, and reassigning it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/SteamAutoCloud.kt`:
- Around line 832-840: The removal of appInfo.id from
PrefManager.pendingCloudResync in the isCacheCleared branch should be made
atomic and deferred until the resync actually succeeds: replace the
read-modify-write sequence around PrefManager.pendingCloudResync with an atomic
update (e.g., a synchronized/compare-and-set helper or add a
PrefManager.removePendingCloudResync(id):Boolean method) and move the call to
remove the id out of the early branch and into the success path of the
resync/confirmation flow (i.e., only after the conflict
resolution/upload/download completes successfully in the SteamAutoCloud resync
logic that sets conflictUfsVersion/CURRENT_UFS_PARSE_VERSION), ensuring
concurrent updates from other syncs or the toggle-off path are not lost.
In `@app/src/main/java/app/gamenative/utils/ContainerUtils.kt`:
- Around line 474-482: The Steam-specific cache and resync bookkeeping (calls to
SteamService.clearCloudSyncCache(...) and modifications of
PrefManager.pendingCloudResync) must be executed only for Steam containers; wrap
both branches that currently run those lines with a guard that checks the
container is a Steam container (e.g., container.isSteam or container.platform ==
ContainerPlatform.STEAM) before calling
extractGameIdFromContainerId(container.id),
SteamService.clearCloudSyncCache(context, gameId) or touching
PrefManager.pendingCloudResync, leaving non-Steam containers untouched.
In `@app/src/main/res/values-es/strings.xml`:
- Line 696: The string resource local_saves_only_description reads like an
unfinished action; change its Spanish text to an imperative sentence to match
surrounding subtitles (e.g., "Desactívala para mantener los guardados solo en el
dispositivo" or similar), updating the value of local_saves_only_description so
it reads as a complete command/imperative rather than an infinitive fragment.
---
Outside diff comments:
In `@app/src/main/java/app/gamenative/ui/PluviaMain.kt`:
- Around line 791-816: The SAVE_CONFLICT handlers are re-calling preLaunchApp
with fresh/default args and losing original launch flags; capture the original
preLaunchApp arguments when the dialog is shown (e.g., into a local/remembered
data structure) and reuse them here, only overriding preferredSave to
SaveLocation.Remote or SaveLocation.Local; call
preLaunchApp(originalArgs.copy(preferredSave = ...)) (preserving flags like
ignorePendingOperations, useTemporaryOverride, persisted container config, and
the viewModel callbacks such as setLoadingDialogVisible,
setLoadingDialogProgress, setLoadingDialogMessage, setMessageDialogState, and
onSuccess = viewModel::launchApp), then clear msgDialogState as before.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/PrefManager.kt`:
- Around line 481-488: The current read-modify-write use of the
pendingCloudResync property can race and lose updates; add atomic add/remove
methods in PrefManager that perform the mutation inside dataStore.edit { }
(e.g., fun addPendingCloudResync(gameId: Int) and fun
removePendingCloudResync(gameId: Int)) where you read the current
PENDING_CLOUD_RESYNC value, modify the Set<Int>, and write the new comma‑joined
string back to the preference within the same transaction; then update callers
in ContainerUtils.kt and SteamAutoCloud.kt to call these new methods instead of
reading the property, modifying it, and reassigning it.
🪄 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: 27b8a54c-21a8-4519-be68-024a32d5ac69
📒 Files selected for processing (23)
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/component/dialog/state/MessageDialogState.ktapp/src/main/java/app/gamenative/ui/enums/DialogType.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.xml
✅ Files skipped from review due to trivial changes (7)
- app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt
- app/src/main/res/values-zh-rCN/strings.xml
- app/src/main/res/values-it/strings.xml
- app/src/main/res/values-uk/strings.xml
- app/src/main/res/values-ko/strings.xml
- app/src/main/res/values-pt-rBR/strings.xml
- app/src/main/res/values-ru/strings.xml
🚧 Files skipped from review as they are similar to previous changes (7)
- app/src/main/java/app/gamenative/ui/enums/DialogType.kt
- app/src/main/res/values/strings.xml
- app/src/main/res/values-ro/strings.xml
- app/src/main/res/values-fr/strings.xml
- app/src/main/res/values-zh-rTW/strings.xml
- app/src/main/res/values-da/strings.xml
- app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt
jeremybernstein
force-pushed
the
jb/fix-local-saves-only
branch
from
6388a7b to
da9ec17
Compare
utkarshdalal
reviewed
Apr 8, 2026
Comment on lines
-2037
to
-2056
| val localDate = Date(postSyncInfo.localTimestamp).toString() | ||
| val remoteDate = Date(postSyncInfo.remoteTimestamp).toString() | ||
| val (conflictTitle, conflictMessage) = postSyncInfo.conflictUfsVersion | ||
| ?.let { v -> | ||
| val titleId = context.resources.getIdentifier( | ||
| "main_save_conflict_upgrade_v${v}_title", "string", context.packageName, | ||
| ) | ||
| val msgId = context.resources.getIdentifier( | ||
| "main_save_conflict_upgrade_v${v}_message", "string", context.packageName, | ||
| ) | ||
| if (titleId != 0 && msgId != 0) { | ||
| context.getString(titleId) to context.getString(msgId, localDate, remoteDate) | ||
| } else { | ||
| null | ||
| } | ||
| } | ||
| ?: run { | ||
| context.getString(R.string.main_save_conflict_title) to | ||
| context.getString(R.string.main_save_conflict_message, localDate, remoteDate) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
this was refactored to MessageDialogState.saveConflict to avoid duplication. @xXJSONDeruloXx requested that I unify the dialog usage, which is why those changes exist. I can back out to a previous version which is less invasive, but has some amount of dialog-related duplication. The actual sync conflict resolution shouldn't be affected at all.
Owner
|
This should not affect the existing sync conflict at all. Please separate it. |
7 tasks
jeremybernstein
force-pushed
the
jb/fix-local-saves-only
branch
from
d4a1637 to
b423966
Compare
jeremybernstein
changed the title
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt (1)
2069-2128: Assert thatpendingCloudResyncis consumed, not just inspected.These tests prove the wording branch, but they would still pass if the implementation regressed from “read-and-remove” to a plain membership check. Add a post-call assertion that
steamAppId !in PrefManager.pendingCloudResyncin the pending-resync scenarios so the one-shot behavior is locked down too.Based on learnings,
SteamAutoCloud.ktintentionally reads and removesappInfo.idfromPrefManager.pendingCloudResyncbefore conflict resolution because that one-shot classification drives the dialog wording.🤖 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 2069 - 2128, The tests check wording branches but don't assert the one-shot flag is consumed; after calling SteamAutoCloud.syncUserFiles in the pending-resync test (function cacheCleared_pendingResync_conflictWithoutUpgradeText) add an assertion that steamAppId is removed from PrefManager.pendingCloudResync (e.g. assertFalse(steamAppId in PrefManager.pendingCloudResync) or equivalent) so the test verifies the code both reads-and-removes the id; locate the similar non-pending test cacheCleared_noPendingResync_conflictWithUpgradeText and do NOT add the removal assertion there (only assert removal in the pending-resync scenario).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/SteamAutoCloud.kt`:
- Around line 826-835: The current logic treats any cache-absent-but-local-files
case (isCacheCleared) as an "upgrade" conflict and sets conflictUfsVersion,
which incorrectly surfaces upgrade wording for a first-offline-play; instead,
differentiate "cache never existed" from "cache was cleared by an upgrade" by
checking whether this app ever had a cloud cache before (e.g., a persisted
flag/lookup such as a new PrefManager.hasSeenCloudCache(appInfo.id) or existing
metadata) and only assign conflictUfsVersion = CURRENT_UFS_PARSE_VERSION when
cachePreviouslyExisted is true and appInfo.id is not in
PrefManager.pendingCloudResync; leave conflicts generic for first-run (cache
never existed) so users see non-upgrade wording.
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt`:
- Around line 256-269: The dialog currently hard-codes the generic timestamp
message for SyncResult.Conflict and loses the original conflict classification;
instead, read syncResult.conflictUfsVersion and build the MessageDialogState
using that classification (e.g., select the appropriate
title/message/confirm/dismiss strings based on conflictUfsVersion or pass the
classification into MessageDialogState) in the branch that calls
showInstallDialog(gameId, MessageDialogState(...)) so the dialog wording matches
the pending-resync vs app-update distinction (preserving the behavior intended
by SteamAutoCloud.kt).
---
Nitpick comments:
In `@app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt`:
- Around line 2069-2128: The tests check wording branches but don't assert the
one-shot flag is consumed; after calling SteamAutoCloud.syncUserFiles in the
pending-resync test (function
cacheCleared_pendingResync_conflictWithoutUpgradeText) add an assertion that
steamAppId is removed from PrefManager.pendingCloudResync (e.g.
assertFalse(steamAppId in PrefManager.pendingCloudResync) or equivalent) so the
test verifies the code both reads-and-removes the id; locate the similar
non-pending test cacheCleared_noPendingResync_conflictWithUpgradeText and do NOT
add the removal assertion there (only assert removal in the pending-resync
scenario).
🪄 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: 7ee4ecc3-358c-4004-8de4-be02a2ea9453
📒 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
✅ Files skipped from review due to trivial changes (9)
- app/src/main/res/values/strings.xml
- app/src/main/res/values-ru/strings.xml
- app/src/main/res/values-fr/strings.xml
- app/src/main/res/values-de/strings.xml
- app/src/main/res/values-da/strings.xml
- app/src/main/res/values-uk/strings.xml
- app/src/main/res/values-zh-rCN/strings.xml
- app/src/main/res/values-zh-rTW/strings.xml
- app/src/main/res/values-pl/strings.xml
🚧 Files skipped from review as they are similar to previous changes (8)
- app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt
- app/src/main/java/app/gamenative/ui/PluviaMain.kt
- app/src/main/res/values-ro/strings.xml
- app/src/main/java/app/gamenative/service/SteamService.kt
- app/src/main/res/values-ko/strings.xml
- app/src/main/res/values-it/strings.xml
- app/src/main/res/values-pt-rBR/strings.xml
- app/src/main/res/values-es/strings.xml
app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt
Show resolved
Hide resolved
Contributor
Author
|
PR has been rejiggered in terms of #1169 rather than the other way around. |
jeremybernstein
force-pushed
the
jb/fix-local-saves-only
branch
from
b423966 to
515343e
Compare
Contributor
Author
|
update to fix conflicts on current master (GOG sync). First four commits are from fix-cloud-overwrite-on-update PR (#1169) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt (1)
257-258:⚠️ Potential issue | 🟡 MinorFormat the conflict timestamps with a locale-aware API.
Date.toString()still renders the raw JVM format here, so localized builds won't get user-region date/time formatting in this dialog.💡 Suggested fix
- val localDate = Date(syncResult.localTimestamp).toString() - val remoteDate = Date(syncResult.remoteTimestamp).toString() + val formatter = java.text.DateFormat.getDateTimeInstance( + java.text.DateFormat.MEDIUM, + java.text.DateFormat.SHORT, + ) + val localDate = formatter.format(Date(syncResult.localTimestamp)) + val remoteDate = formatter.format(Date(syncResult.remoteTimestamp))🤖 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 257 - 258, The timestamps are being formatted with Date.toString() which is not locale-aware; replace the usages that assign localDate and remoteDate (in SteamAppScreen) to instead convert the epoch millis (syncResult.localTimestamp / remoteTimestamp) to a java.time.Instant and format via DateTimeFormatter.ofLocalizedDateTime(...) (or DateTimeFormatter.ofLocalizedDate(...) as appropriate) with the current Locale (e.g., Locale.getDefault() or the Compose LocalContext locale) so the displayed times are localized for the user.
🤖 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/screen/library/appscreen/SteamAppScreen.kt`:
- Around line 1178-1195: The SYNC_CONFLICT branches call hideInstallDialog(...)
and performForceCloudSync(..., SaveLocation.Remote/Local) but never clear the
pendingCloudResync marker; update both DialogType.SYNC_CONFLICT handlers (the
anonymous lambdas created in the primary click branch and in
effectiveDismissClick) to also consume/clear pendingCloudResync for the affected
id (use the same key used elsewhere, e.g., libraryItem.appId) after triggering
performForceCloudSync; mirror the cleanup approach used in PluviaMain.kt so the
pendingCloudResync entry is removed when the conflict resolution finishes.
---
Duplicate comments:
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt`:
- Around line 257-258: The timestamps are being formatted with Date.toString()
which is not locale-aware; replace the usages that assign localDate and
remoteDate (in SteamAppScreen) to instead convert the epoch millis
(syncResult.localTimestamp / remoteTimestamp) to a java.time.Instant and format
via DateTimeFormatter.ofLocalizedDateTime(...) (or
DateTimeFormatter.ofLocalizedDate(...) as appropriate) with the current Locale
(e.g., Locale.getDefault() or the Compose LocalContext locale) so the displayed
times are localized for the user.
🪄 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: 5e489910-1b42-4497-9db0-bd337756c97f
📒 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
✅ Files skipped from review due to trivial changes (8)
- app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt
- app/src/main/res/values/strings.xml
- app/src/main/res/values-pl/strings.xml
- app/src/main/res/values-ro/strings.xml
- app/src/main/res/values-pt-rBR/strings.xml
- app/src/main/res/values-uk/strings.xml
- app/src/main/res/values-es/strings.xml
- app/src/main/res/values-it/strings.xml
🚧 Files skipped from review as they are similar to previous changes (7)
- app/src/main/java/app/gamenative/PrefManager.kt
- app/src/main/res/values-da/strings.xml
- app/src/main/java/app/gamenative/service/SteamService.kt
- app/src/main/java/app/gamenative/utils/ContainerUtils.kt
- app/src/main/res/values-zh-rCN/strings.xml
- app/src/main/res/values-ko/strings.xml
- app/src/main/res/values-de/strings.xml
app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt
Show resolved
Hide resolved
jeremybernstein
force-pushed
the
jb/fix-local-saves-only
branch
from
515343e to
2f6424b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt (1)
257-258:⚠️ Potential issue | 🟡 MinorFormat the conflict timestamps with a locale-aware formatter.
Date.toString()renders the fixed JVM English format, so this regresses translated builds back to raw English timestamps inside the conflict dialog.✏️ Suggested fix
- val localDate = Date(syncResult.localTimestamp).toString() - val remoteDate = Date(syncResult.remoteTimestamp).toString() + val formatter = java.text.DateFormat.getDateTimeInstance( + java.text.DateFormat.MEDIUM, + java.text.DateFormat.SHORT, + ) + val localDate = formatter.format(Date(syncResult.localTimestamp)) + val remoteDate = formatter.format(Date(syncResult.remoteTimestamp))🤖 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 257 - 258, The conflict dialog currently builds timestamps using Date(syncResult.localTimestamp).toString() and Date(syncResult.remoteTimestamp).toString() which produces JVM English-only output; update SteamAppScreen.kt to format these timestamps with a locale-aware formatter instead (e.g. use java.time.Instant.ofEpochMilli(...).atZone(ZoneId.systemDefault()) and DateTimeFormatter.ofLocalizedDateTime(FormatStyle.SHORT/MEDIUM).withLocale(Locale.getDefault()), or java.text.DateFormat.getDateTimeInstance(...).format(Date(...))) so localDate and remoteDate are produced via the locale-aware formatter using syncResult.localTimestamp and syncResult.remoteTimestamp.
🧹 Nitpick comments (3)
app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt (3)
2057-2105: Make theKeep Remotetests verify the replacement path.Unlike the other new download fixtures, these two mocks drop
/SaveGamesfrompathPrefixesand then only assertfilesDownloaded > 0. A wrong-path download would still pass, so the tests do not prove that the conflicting local save was actually replaced.Apply the same tightening to both `preferRemote` scenarios.Suggested tightening
- val cloudFileChangeList = makeCloudFileChangeList( - cloudChangeNumber = localCn + 1, - files = listOf(mockFile), - pathPrefixes = listOf("%WinMyDocuments%/My Games/TestGame/Steam/76561198025127569"), - ) + val cloudFileChangeList = makeCloudFileChangeList( + cloudChangeNumber = localCn + 1, + files = listOf(mockFile), + pathPrefixes = listOf("%WinMyDocuments%/My Games/TestGame/Steam/76561198025127569/SaveGames"), + ) … - assertTrue("Should have downloaded files", result.filesDownloaded > 0) + assertTrue("Should have downloaded files", result.filesDownloaded > 0) + assertArrayEquals( + "Remote save should replace the conflicting local file", + cloudContent, + File(saveFilesDir, "SaveData_0.sav").readBytes(), + )Also applies to: 2125-2174
🤖 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 2057 - 2105, The Keep Remote tests currently only assert result.filesDownloaded > 0 while the cloud fixtures drop "/SaveGames" from pathPrefixes, so they don't verify the actual replacement path; update the tests that call SteamAutoCloud.syncUserFiles (both preferRemote scenarios around the makeCloudFileChangeList calls) to assert that the downloaded file was written to the expected local path (the path computed from the original pathPrefix including the "/SaveGames" segment) or that the conflicting local save file was overwritten/removed—locate the makeCloudFileChangeList invocation and the testApp/local file setup and add an assertion checking the specific file path content or existence to ensure the wrong-path download would fail the test.
2181-2207: Exercise the real one-shotpendingCloudResyncpath here.Both tests delete the change-number row, so they currently look more like “never synced” than “cache cleared after toggle-off”. They also do not verify that the app ID is consumed from
PrefManager.pendingCloudResyncafter the first classification.Based on learnings, `clearCloudSyncCache(context, appId)` deletes the cache row before writing `PrefManager.pendingCloudResync`, and the `isCacheCleared` branch intentionally reads and removes that app ID before conflict resolution.Suggested tightening
`@Test` fun cacheCleared_pendingResync_conflictWithoutUpgradeText() = runBlocking { db.appChangeNumbersDao().deleteByAppId(steamAppId) db.appFileChangeListsDao().deleteByAppId(steamAppId) + db.appChangeNumbersDao().insert(app.gamenative.data.ChangeNumbers(steamAppId, 5L)) assertTrue("Precondition: local save files exist", saveFilesDir.listFiles()!!.isNotEmpty()) // simulate local-saves toggle-off: appId in pendingCloudResync PrefManager.pendingCloudResync = setOf(steamAppId) … assertNull( "conflictUfsVersion should be null — generic text, not upgrade", result.conflictUfsVersion, ) + assertFalse( + "pendingCloudResync should be consumed after the first classification", + steamAppId in PrefManager.pendingCloudResync, + ) }Also applies to: 2213-2240
🤖 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 2181 - 2207, The test currently deletes the change-number row which makes it a "never synced" case; instead simulate the real one-shot pendingCloudResync path by not deleting the DB row and calling clearCloudSyncCache(context, steamAppId) to model "cache cleared before pendingCloudResync is written", then set PrefManager.pendingCloudResync = setOf(steamAppId) and call SteamAutoCloud.syncUserFiles(...) as before; after the call assert the result is Conflict and also assert that PrefManager.pendingCloudResync no longer contains steamAppId (i.e., the appId was consumed/removed by the isCacheCleared branch). Ensure the test references cacheCleared_pendingResync_conflictWithoutUpgradeText (and the sibling test at lines ~2213-2240) and does not delete the change-number row via db.appChangeNumbersDao().deleteByAppId.
1442-1452: Fail fast on unexpected prefixes inmakePrefixToPath().Returning
tempDirfrom the default branch can turn a prefix-resolution regression into a harmless-looking “no files found” path. Throw here instead so these scenario tests fail at the real cause.Suggested tightening
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 SteamAutoCloudTest: $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 1442 - 1452, The current makePrefixToPath() silently returns tempDir for unknown prefixes which hides regressions; update its default branch (after handling "WinMyDocuments") to throw an exception (e.g., IllegalArgumentException or IllegalStateException) that includes the original prefix/stripped value so tests fail fast on unknown placeholders instead of returning tempDir; keep the existing handling for "WinMyDocuments" and preserve the comment about % placeholders.
🤖 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/screen/library/appscreen/SteamAppScreen.kt`:
- Around line 274-280: The code currently falls through to the generic error
case when SteamService.forceSyncUserFiles() returns SyncResult.InProgress, which
shows the raw "InProgress" text; add an explicit branch for
SyncResult.InProgress in the when handling inside SteamAppScreen (where
SnackbarManager.show is called) to call SnackbarManager.show with the
appropriate "sync already in progress" string resource (e.g.,
R.string.library_cloud_sync_already_in_progress) instead of the generic error
string so the user sees the correct human-readable message.
In `@app/src/main/res/values-ro/strings.xml`:
- Line 30: The Romanian string steam_verify_files_message_local_saves (and the
other affected Romanian string at the same file around the later occurrence)
uses an opening Romanian quote „ but a straight ASCII double-quote " as the
closer; update the closing quote to the proper Romanian closing quote ” so the
phrase “Doar salvări locale” uses matching „… ” punctuation (search for
steam_verify_files_message_local_saves and the other localized string around
lines ~1373-1374 to apply the fix).
---
Duplicate comments:
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt`:
- Around line 257-258: The conflict dialog currently builds timestamps using
Date(syncResult.localTimestamp).toString() and
Date(syncResult.remoteTimestamp).toString() which produces JVM English-only
output; update SteamAppScreen.kt to format these timestamps with a locale-aware
formatter instead (e.g. use
java.time.Instant.ofEpochMilli(...).atZone(ZoneId.systemDefault()) and
DateTimeFormatter.ofLocalizedDateTime(FormatStyle.SHORT/MEDIUM).withLocale(Locale.getDefault()),
or java.text.DateFormat.getDateTimeInstance(...).format(Date(...))) so localDate
and remoteDate are produced via the locale-aware formatter using
syncResult.localTimestamp and syncResult.remoteTimestamp.
---
Nitpick comments:
In `@app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt`:
- Around line 2057-2105: The Keep Remote tests currently only assert
result.filesDownloaded > 0 while the cloud fixtures drop "/SaveGames" from
pathPrefixes, so they don't verify the actual replacement path; update the tests
that call SteamAutoCloud.syncUserFiles (both preferRemote scenarios around the
makeCloudFileChangeList calls) to assert that the downloaded file was written to
the expected local path (the path computed from the original pathPrefix
including the "/SaveGames" segment) or that the conflicting local save file was
overwritten/removed—locate the makeCloudFileChangeList invocation and the
testApp/local file setup and add an assertion checking the specific file path
content or existence to ensure the wrong-path download would fail the test.
- Around line 2181-2207: The test currently deletes the change-number row which
makes it a "never synced" case; instead simulate the real one-shot
pendingCloudResync path by not deleting the DB row and calling
clearCloudSyncCache(context, steamAppId) to model "cache cleared before
pendingCloudResync is written", then set PrefManager.pendingCloudResync =
setOf(steamAppId) and call SteamAutoCloud.syncUserFiles(...) as before; after
the call assert the result is Conflict and also assert that
PrefManager.pendingCloudResync no longer contains steamAppId (i.e., the appId
was consumed/removed by the isCacheCleared branch). Ensure the test references
cacheCleared_pendingResync_conflictWithoutUpgradeText (and the sibling test at
lines ~2213-2240) and does not delete the change-number row via
db.appChangeNumbersDao().deleteByAppId.
- Around line 1442-1452: The current makePrefixToPath() silently returns tempDir
for unknown prefixes which hides regressions; update its default branch (after
handling "WinMyDocuments") to throw an exception (e.g., IllegalArgumentException
or IllegalStateException) that includes the original prefix/stripped value so
tests fail fast on unknown placeholders instead of returning tempDir; keep the
existing handling for "WinMyDocuments" and preserve the comment about %
placeholders.
🪄 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: 3f263e6d-9f88-48c1-bbe5-95d270d53993
📒 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
✅ Files skipped from review due to trivial changes (7)
- app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt
- app/src/main/res/values-pl/strings.xml
- app/src/main/res/values-ru/strings.xml
- app/src/main/res/values/strings.xml
- app/src/main/res/values-zh-rCN/strings.xml
- app/src/main/res/values-fr/strings.xml
- app/src/main/java/app/gamenative/utils/ContainerUtils.kt
🚧 Files skipped from review as they are similar to previous changes (10)
- app/src/main/java/app/gamenative/ui/PluviaMain.kt
- app/src/main/java/app/gamenative/PrefManager.kt
- app/src/main/res/values-da/strings.xml
- app/src/main/res/values-pt-rBR/strings.xml
- app/src/main/java/app/gamenative/service/SteamService.kt
- app/src/main/res/values-ko/strings.xml
- app/src/main/java/app/gamenative/service/SteamAutoCloud.kt
- app/src/main/res/values-zh-rTW/strings.xml
- app/src/main/res/values-de/strings.xml
- app/src/main/res/values-uk/strings.xml
app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt
Show resolved
Hide resolved
jeremybernstein
force-pushed
the
jb/fix-local-saves-only
branch
from
2f6424b to
d55787a
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt (1)
261-262:⚠️ Potential issue | 🟡 MinorUse a locale-aware formatter for the conflict timestamps.
Date.toString()will render JVM-style English timestamps even in translated builds, so this dialog regresses to non-localized date/time output.Proposed fix
- val localDate = Date(syncResult.localTimestamp).toString() - val remoteDate = Date(syncResult.remoteTimestamp).toString() + val formatter = java.text.DateFormat.getDateTimeInstance( + java.text.DateFormat.MEDIUM, + java.text.DateFormat.SHORT, + java.util.Locale.getDefault(), + ) + val localDate = formatter.format(Date(syncResult.localTimestamp)) + val remoteDate = formatter.format(Date(syncResult.remoteTimestamp))🤖 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 261 - 262, Replace the plain Date(...).toString() calls in SteamAppScreen where localDate and remoteDate are computed so the timestamps are formatted with the user's locale; specifically, use a locale-aware formatter (e.g. java.text.DateFormat.getDateTimeInstance(...) with Locale.getDefault() or Android's android.text.format.DateFormat.getDateFormat(context) combined with getTimeFormat(context)) to format Date(syncResult.localTimestamp) and Date(syncResult.remoteTimestamp) instead of calling toString(); update the code that sets localDate and remoteDate in SteamAppScreen.kt to call the chosen formatter with the Date objects so the dialog shows localized date/time.
🤖 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/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt`:
- Around line 261-262: Replace the plain Date(...).toString() calls in
SteamAppScreen where localDate and remoteDate are computed so the timestamps are
formatted with the user's locale; specifically, use a locale-aware formatter
(e.g. java.text.DateFormat.getDateTimeInstance(...) with Locale.getDefault() or
Android's android.text.format.DateFormat.getDateFormat(context) combined with
getTimeFormat(context)) to format Date(syncResult.localTimestamp) and
Date(syncResult.remoteTimestamp) instead of calling toString(); update the code
that sets localDate and remoteDate in SteamAppScreen.kt to call the chosen
formatter with the Date objects so the dialog shows localized date/time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7fe40f73-68ac-4b33-859a-4b3f6da0801a
📒 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
✅ Files skipped from review due to trivial changes (2)
- app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt
- app/src/main/res/values-zh-rCN/strings.xml
🚧 Files skipped from review as they are similar to previous changes (13)
- app/src/main/res/values/strings.xml
- app/src/main/res/values-pl/strings.xml
- app/src/main/res/values-pt-rBR/strings.xml
- app/src/main/java/app/gamenative/utils/ContainerUtils.kt
- app/src/main/res/values-ro/strings.xml
- app/src/main/res/values-da/strings.xml
- app/src/main/java/app/gamenative/service/SteamAutoCloud.kt
- app/src/main/res/values-de/strings.xml
- app/src/main/res/values-fr/strings.xml
- app/src/main/res/values-uk/strings.xml
- app/src/main/res/values-it/strings.xml
- app/src/main/res/values-zh-rTW/strings.xml
- app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt
jeremybernstein
force-pushed
the
jb/fix-local-saves-only
branch
from
d55787a to
8eeb7df
Compare
Contributor
Author
|
rebased on master (where the base commit is now merged). |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt (1)
1056-1061:⚠️ Potential issue | 🟡 MinorConsume
pendingCloudResyncon theonDismissRequestpath too.Line 1056 still routes
DialogType.SYNC_CONFLICTthrough the generichideInstallDialog(gameId)path, so back/outside dismissal bypasses the cleanup added on Lines 1188 and 1199. IfMessageDialogis dismissible that way, the stale marker survives and the next cache-cleared conflict can be mislabeled. Either clear the marker inonDismissRequestas well, or make this dialog non-dismissible except via the explicit buttons.💡 Possible fix
- val onDismissRequest: (() -> Unit)? = { - hideInstallDialog(gameId) - } + val onDismissRequest: (() -> Unit)? = when (installDialogState.type) { + DialogType.SYNC_CONFLICT -> { + { + PrefManager.pendingCloudResync = PrefManager.pendingCloudResync - gameId + hideInstallDialog(gameId) + } + } + else -> { + { hideInstallDialog(gameId) } + } + }Based on learnings, in
app/src/main/java/app/gamenative/service/SteamAutoCloud.kt, the presence ofPrefManager.pendingCloudResyncdetermines which conflict wording is shown, so the marker should be consumed on every exit path once this dialog has been surfaced.#!/bin/bash set -euo pipefail echo "== SteamAppScreen conflict-dialog paths ==" sed -n '1050,1210p' app/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt | nl -ba echo echo "== MessageDialog implementation ==" fd 'MessageDialog.kt' app/src/main/java -x sh -c ' echo "-- $1 --" sed -n "1,240p" "$1" | nl -ba ' sh {}Also applies to: 1185-1203
🤖 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 1056 - 1061, The DialogType.SYNC_CONFLICT dismissal path currently calls hideInstallDialog(gameId) in onDismissRequest/onDismissClick and thus can bypass the cleanup that consumes PrefManager.pendingCloudResync; update the onDismissRequest handler for the MessageDialog used for SYNC_CONFLICT so it also clears/consumes pendingCloudResync (the same cleanup done around lines 1188/1199) before or instead of calling hideInstallDialog(gameId), or alternatively make MessageDialog non-dismissible so only the explicit button handlers (which already consume pendingCloudResync) can close it; reference the onDismissRequest/onDismissClick handlers, hideInstallDialog(gameId), MessageDialog, and PrefManager.pendingCloudResync to locate the change.
🤖 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/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.kt`:
- Around line 1056-1061: The DialogType.SYNC_CONFLICT dismissal path currently
calls hideInstallDialog(gameId) in onDismissRequest/onDismissClick and thus can
bypass the cleanup that consumes PrefManager.pendingCloudResync; update the
onDismissRequest handler for the MessageDialog used for SYNC_CONFLICT so it also
clears/consumes pendingCloudResync (the same cleanup done around lines
1188/1199) before or instead of calling hideInstallDialog(gameId), or
alternatively make MessageDialog non-dismissible so only the explicit button
handlers (which already consume pendingCloudResync) can close it; reference the
onDismissRequest/onDismissClick handlers, hideInstallDialog(gameId),
MessageDialog, and PrefManager.pendingCloudResync to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b28dc60-e920-408a-ae90-66258d802fed
📒 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
✅ Files skipped from review due to trivial changes (7)
- app/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.kt
- app/src/main/res/values/strings.xml
- app/src/main/res/values-pl/strings.xml
- app/src/main/res/values-ru/strings.xml
- app/src/main/res/values-zh-rCN/strings.xml
- app/src/main/res/values-de/strings.xml
- app/src/main/res/values-zh-rTW/strings.xml
🚧 Files skipped from review as they are similar to previous changes (7)
- app/src/main/java/app/gamenative/PrefManager.kt
- app/src/main/res/values-ro/strings.xml
- app/src/main/res/values-ko/strings.xml
- app/src/main/res/values-pt-rBR/strings.xml
- app/src/main/res/values-uk/strings.xml
- app/src/main/res/values-fr/strings.xml
- app/src/main/java/app/gamenative/utils/ContainerUtils.kt
Contributor
Author
|
@coderabbitai, the current code is correct: if the user backs out (maybe she wants to back up files manually or something before committing to a choice), the flag still needs to remain active -- a sync still needs to happen and a decision made. please stop bothering me about this. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
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
Re-enable "local saves only" with proper cloud sync safety nets.
Depends on #1169 (the sync cache fix).
pendingCloudResyncflag distinguishes "cache cleared by toggle" from "cache cleared by app update" — the former gets generic conflict text, the latter gets upgrade-specific textCloses #1145
Recording
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
New Features
Behavior Changes
Localization
Tests