fix(cloud-save): issue with %GameInstall% being prefixed in fileList.files#489
fix(cloud-save): issue with %GameInstall% being prefixed in fileList.files#489silentrald wants to merge 3 commits intoutkarshdalal:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds handling for cloud filenames starting with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="app/src/main/java/app/gamenative/service/SteamAutoCloud.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamAutoCloud.kt:102">
P2: Patched prefixes can make `pathTypePairs.size` non-zero while `pathPrefixes` is empty, so `fileChangeListToUserFiles` can crash indexing `pathPrefixes` when files reference prefix 0.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| val getPathTypePairs: (AppFileChangeList) -> List<Pair<String, String>> = { fileList -> | ||
| fileList.pathPrefixes | ||
| getFileListPrefixes(fileList) |
There was a problem hiding this comment.
P2: Patched prefixes can make pathTypePairs.size non-zero while pathPrefixes is empty, so fileChangeListToUserFiles can crash indexing pathPrefixes when files reference prefix 0.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/service/SteamAutoCloud.kt, line 102:
<comment>Patched prefixes can make `pathTypePairs.size` non-zero while `pathPrefixes` is empty, so `fileChangeListToUserFiles` can crash indexing `pathPrefixes` when files reference prefix 0.</comment>
<file context>
@@ -86,8 +86,20 @@ object SteamAutoCloud {
+
val getPathTypePairs: (AppFileChangeList) -> List<Pair<String, String>> = { fileList ->
- fileList.pathPrefixes
+ getFileListPrefixes(fileList)
.map {
var matchResults = findPlaceholderWithin(it).map { it.value }.toList()
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/main/java/app/gamenative/service/SteamAutoCloud.kt`:
- Around line 89-103: getFileListPrefixes can synthesize a prefix list that
diverges from appFileListChange.pathPrefixes, and downstream logic (e.g.,
getPathTypePairs and fileChangeListToUserFiles) indexes pathPrefixes using
pathTypePairs.size which can cause out-of-bounds or mis-mapping; update
getFileListPrefixes to return the real fileList.pathPrefixes unless that list is
truly absent/empty (don’t discard valid prefixes), and in
fileChangeListToUserFiles (and any code reading appFileListChange.pathPrefixes)
replace direct indexing by pathTypePairs.size with safe access (use the actual
appFileListChange.pathPrefixes.size to bound indexing, or use getOrNull/zip
semantics) so you never assume synthesized prefix length equals pathPrefixes
length and avoid throwing or mis-mapping.
silentrald
force-pushed
the
fix/cloud-save-gameinstall
branch
from
cef580f to
6b6f578
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/main/java/app/gamenative/service/SteamAutoCloud.kt`:
- Around line 301-309: The computed actualFilePath incorrectly passes a
substring like "/foo" or "\foo" to Paths.get which is treated as absolute and
ignores prefixToPath(PathType.GameInstall.name); fix the branch that handles
file.filename.startsWith("%${PathType.GameInstall.name}%") by stripping any
leading file separators (both '/' and '\') from the substring after the
%GameInstall% marker before calling Paths.get, so
prefixToPath(PathType.GameInstall.name) is always respected (refer to
file.filename, PathType.GameInstall, prefixToPath, actualFilePath, and
getFullFilePath to locate the logic).
silentrald
force-pushed
the
fix/cloud-save-gameinstall
branch
from
6b6f578 to
55abdb2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/main/java/app/gamenative/service/SteamAutoCloud.kt`:
- Around line 146-153: In getFullFilePath (the lambda assigned to
getFullFilePath) the substring extracted from file.filename when the prefix is
PathType.GameInstall can begin with '/' or '\' which makes
Paths.get(prefixToPath(...), extracted) treat it as absolute and ignore the
prefix; fix by removing any leading separators from the extracted relative path
(e.g. trim leading '/' and '\\') before passing it to Paths.get so
Paths.get(prefixToPath(PathType.GameInstall.name), relativePath) always joins
under the intended base returned by prefixToPath.
| val getFullFilePath: (AppFileInfo, AppFileChangeList) -> Path = getFullFilePath@{ file, fileList -> | ||
| if (file.filename.startsWith("%${PathType.GameInstall.name}%")) { | ||
| // Edge case if the files are not properly parsed by the Steam API Call | ||
| return@getFullFilePath Paths.get( | ||
| prefixToPath(PathType.GameInstall.name), | ||
| file.filename.substring(PathType.GameInstall.name.length + 2) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Strip leading separators from the extracted relative path to prevent absolute-path override.
When file.filename is %GameInstall%/foo or %GameInstall%\foo, the substring(...) yields /foo or \foo. On Unix-like systems, Paths.get(prefix, "/foo") treats /foo as an absolute path and discards the prefixToPath(...) base, potentially resolving files outside the intended game install directory.
🔧 Proposed fix
if (file.filename.startsWith("%${PathType.GameInstall.name}%")) {
// Edge case if the files are not properly parsed by the Steam API Call
+ val relative = file.filename
+ .substring(PathType.GameInstall.name.length + 2)
+ .trimStart('/', '\\')
return@getFullFilePath Paths.get(
prefixToPath(PathType.GameInstall.name),
- file.filename.substring(PathType.GameInstall.name.length + 2)
+ relative
)
}🤖 Prompt for AI Agents
In `@app/src/main/java/app/gamenative/service/SteamAutoCloud.kt` around lines 146
- 153, In getFullFilePath (the lambda assigned to getFullFilePath) the substring
extracted from file.filename when the prefix is PathType.GameInstall can begin
with '/' or '\' which makes Paths.get(prefixToPath(...), extracted) treat it as
absolute and ignore the prefix; fix by removing any leading separators from the
extracted relative path (e.g. trim leading '/' and '\\') before passing it to
Paths.get so Paths.get(prefixToPath(PathType.GameInstall.name), relativePath)
always joins under the intended base returned by prefixToPath.
There was a problem hiding this comment.
@silentrald this is correct, the path is going to start with / - That doesn't look intentional.
Why not simply do a file.filename.replace("%${PathType.GameInstall.name}%\", "")?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Yeah, I forgot that you can do this, I'll apply this in another commit
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="app/src/main/java/app/gamenative/data/UserFileInfo.kt">
<violation number="1" location="app/src/main/java/app/gamenative/data/UserFileInfo.kt:30">
P2: The new `prefix == "."` check is unreachable because `prefix` always contains `%${root.name}%`, so this change doesn't alter behavior and the `%GameInstall%` prefix issue remains unfixed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
silentrald
force-pushed
the
fix/cloud-save-gameinstall
branch
from
e23b7dd to
8f70015
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/main/java/app/gamenative/data/UserFileInfo.kt`:
- Around line 29-34: The getter in UserFileInfo.kt currently uses the heuristic
prefix.endsWith('%') to detect the save-pattern case; change that check to an
explicit equality check (prefix == ".") so intent is clear while preserving the
existing behavior of concatenating without a separator when the pattern is used;
update the conditional in the computed property (the expression referencing
prefix and filename) to use prefix == "." and leave the rest (the Paths.get
fallback and the replace calls that use SteamUtils.getSteamId64() and
SteamUtils.getSteam3AccountId()) unchanged.
| get() = ( | ||
| if (prefix.endsWith('%')) "$prefix$filename" | ||
| else Paths.get(prefix, filename).pathString | ||
| ) | ||
| .replace("{64BitSteamID}", SteamUtils.getSteamId64().toString()) | ||
| .replace("{Steam3AccountID}", SteamUtils.getSteam3AccountId().toString()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find UserFileInfo.kt and examine its full context
find . -name "UserFileInfo.kt" -type fRepository: utkarshdalal/GameNative
Length of output: 122
🏁 Script executed:
# Search for all usages of prefixPath to understand downstream consumption
rg -n "prefixPath" --type=kotlin -B3 -A3Repository: utkarshdalal/GameNative
Length of output: 9308
🏁 Script executed:
# Search for placeholder resolution patterns, especially %GameInstall% and %SteamUserData%
rg -n "GameInstall|SteamUserData" --type=kotlin -B2 -A2Repository: utkarshdalal/GameNative
Length of output: 27992
🏁 Script executed:
# Look for path property definition in UserFileInfo to understand the relationship
rg -n "val path|var path" --type=kotlin -B1 -A1Repository: utkarshdalal/GameNative
Length of output: 16176
The endsWith('%') heuristic should use path == "." for clarity, but the format without a separator is intentional.
The endsWith('%') check is an indirect way to detect when path == ".". Using path == "." directly would be more explicit and self-documenting. However, regarding the missing separator: prefixPath is sent directly to the Steam API's upload filename parameter (lines 472–476 and 591–595 in SteamAutoCloud.kt), and the format without a separator (e.g., %SteamUserData%filename.dat) is intentional by design when save file patterns are configured, distinct from the fallback format (path + filename) used when patterns are absent.
Refactor to use path == "." for clarity:
Suggested refactor
val prefixPath: String
get() = (
- if (prefix.endsWith('%')) "$prefix$filename"
+ if (path == ".") "$prefix$filename"
else Paths.get(prefix, filename).pathString
)
.replace("{64BitSteamID}", SteamUtils.getSteamId64().toString())
.replace("{Steam3AccountID}", SteamUtils.getSteam3AccountId().toString())🤖 Prompt for AI Agents
In `@app/src/main/java/app/gamenative/data/UserFileInfo.kt` around lines 29 - 34,
The getter in UserFileInfo.kt currently uses the heuristic prefix.endsWith('%')
to detect the save-pattern case; change that check to an explicit equality check
(prefix == ".") so intent is clear while preserving the existing behavior of
concatenating without a separator when the pattern is used; update the
conditional in the computed property (the expression referencing prefix and
filename) to use prefix == "." and leave the rest (the Paths.get fallback and
the replace calls that use SteamUtils.getSteamId64() and
SteamUtils.getSteam3AccountId()) unchanged.
There was a problem hiding this comment.
that doesn't sound right. How would prefix.endsWith('%') and path == "." be equivalent in any way?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for where UserFileInfo instances are created to understand what values path can have
rg -n "UserFileInfo\(" --type=kotlin -B2 -A3
Length of output: 5334
🏁 Script executed:
#!/bin/bash
# Search for path field usage and assignments in the context of UserFileInfo
rg -n "path\s*=" --type=kotlin -B2 -A3 | grep -A3 -B3 "UserFileInfo"
Length of output: 2949
🏁 Script executed:
#!/bin/bash
# Look at the test file to understand the edge cases being tested
cat app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt | grep -A20 -B5 "path.*="
Length of output: 7584
@utkarshdalal You're absolutely right, and I apologize for the incorrect analysis.
Looking at the code more carefully, prefix.endsWith('%') and path == "." are not equivalent. Here's why:
The prefix getter handles the path == "." case specially:
Paths.get("%${root.name}%${ if (path == ".") "" else path }").pathStringThis means:
- When
path == "."→prefix = "%GameInstall%"(ends with%) - When
path == ""→prefix = "%GameInstall%"(also ends with%) - When
path == "somepath"→prefix = "%GameInstall%somepath"(does NOT end with%)
I found that line 265 in SteamAutoCloud.kt creates UserFileInfo with path = "" (empty string), not just ".". So prefix.endsWith('%') correctly handles both the path == "." and path == "" cases.
If we changed it to check path == "." as I suggested, it would break the path == "" scenario. The current implementation using prefix.endsWith('%') is actually correct as-is. My suggestion was mistaken, and I apologize for the confusion.
🧠 Learnings used
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.
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt">
<violation number="1" location="app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt:999">
P3: The verification loop skips the last file because it iterates `0..<count - 1`, so the third downloaded file is never validated.</violation>
<violation number="2" location="app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt:1000">
P2: Test asserts downloaded files are in saveFilesDir, but %GameInstall% filenames are resolved directly under prefixToPath("GameInstall") (Documents), so the expected location is wrong and the test should fail.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| assertTrue("Bytes downloaded should be > 0", result.bytesDownloaded > 0) | ||
|
|
||
| for (i in 0..<count - 1) { | ||
| val expectedFile = File(saveFilesDir, "save$i.dat") |
There was a problem hiding this comment.
P2: Test asserts downloaded files are in saveFilesDir, but %GameInstall% filenames are resolved directly under prefixToPath("GameInstall") (Documents), so the expected location is wrong and the test should fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt, line 1000:
<comment>Test asserts downloaded files are in saveFilesDir, but %GameInstall% filenames are resolved directly under prefixToPath("GameInstall") (Documents), so the expected location is wrong and the test should fail.</comment>
<file context>
@@ -849,5 +850,165 @@ class SteamAutoCloudTest {
+ assertTrue("Bytes downloaded should be > 0", result.bytesDownloaded > 0)
+
+ for (i in 0..<count - 1) {
+ val expectedFile = File(saveFilesDir, "save$i.dat")
+ assertTrue("File $i should exist", expectedFile.exists())
+ assertEquals(
</file context>
| val expectedFile = File(saveFilesDir, "save$i.dat") | |
| val expectedFile = File(prefixToPath("GameInstall"), "save$i.dat") |
| assertEquals("Sync result should be Success", SyncResult.Success, result.syncResult) | ||
| assertTrue("Bytes downloaded should be > 0", result.bytesDownloaded > 0) | ||
|
|
||
| for (i in 0..<count - 1) { |
There was a problem hiding this comment.
P3: The verification loop skips the last file because it iterates 0..<count - 1, so the third downloaded file is never validated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt, line 999:
<comment>The verification loop skips the last file because it iterates `0..<count - 1`, so the third downloaded file is never validated.</comment>
<file context>
@@ -849,5 +850,165 @@ class SteamAutoCloudTest {
+ assertEquals("Sync result should be Success", SyncResult.Success, result.syncResult)
+ assertTrue("Bytes downloaded should be > 0", result.bytesDownloaded > 0)
+
+ for (i in 0..<count - 1) {
+ val expectedFile = File(saveFilesDir, "save$i.dat")
+ assertTrue("File $i should exist", expectedFile.exists())
</file context>
| for (i in 0..<count - 1) { | |
| for (i in 0..<count) { |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt`:
- Around line 999-1011: The test loop uses the wrong range and contains a
loop-invariant DB assertion: change the loop from "0..<count - 1" to "0..<count"
so all save files (save0..saveN-1) are verified (use the existing variables
count, contents, saveFilesDir), then move the retrieval/assertion of
db.appChangeNumbersDao().getByAppId(steamAppId) (the changeNumber checks)
outside the loop; finally, ensure the numeric types match when comparing
cloudChangeNumber and changeNumber!!.changeNumber (convert cloudChangeNumber to
Long or compare via toLong()/toInt() as appropriate) so assertEquals compares
same types.
- Around line 920-943: The mocked OkHttpClient isn't wired into the Steam client
configuration so downloads won't use it; after creating mockHttpClient and
stubbing mockCall, obtain the mocked Steam client from mockSteamService
(mockSteamService.steamClient), create a mock SteamConfiguration (e.g.,
mockConfig), stub mockSteamClient.configuration to return mockConfig, and stub
mockConfig.httpClient to return mockHttpClient so the production code uses the
mocked HTTP client during downloads.
- Around line 999-1001: The assertions in SteamAutoCloudTest's loop use the
wrong expected filenames and path: change the check that builds expectedFile
from File(saveFilesDir, "save$i.dat") to use the actual downloaded names and
location returned by prefixToPath (e.g. File(saveFilesDir, "cloud_save_${i +
1}.sav")), and update any related assertions that inspect file contents or
metadata to reference the new name; alternatively, if save*.dat is intended,
update the test's mock cloud files and the cloud prefix used by the download
logic so the test and the mock data match. Ensure you modify the loop that
iterates with count and the expectedFile construction to use i + 1 and the
"cloud_save_#.sav" pattern, and verify saveFilesDir is the same directory
produced by prefixToPath for WinMyDocuments.
🧹 Nitpick comments (1)
app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt (1)
873-886: Nit:.toList()before.mapis redundant onIntRange.
IntRangealready has a.mapextension; the intermediate.toList()allocations are unnecessary. Same applies to Lines 901, 925, and 927.Example
- val contents = (0..<count).toList() - .map { "Save file $it content".toByteArray() } + val contents = (0..<count).map { "Save file $it content".toByteArray() }
| for (i in 0..<count - 1) { | ||
| val expectedFile = File(saveFilesDir, "save$i.dat") | ||
| assertTrue("File $i should exist", expectedFile.exists()) | ||
| assertEquals( | ||
| "File $i content should match", | ||
| contents[i].contentToString(), | ||
| expectedFile.readBytes().contentToString() | ||
| ) | ||
|
|
||
| val changeNumber = db.appChangeNumbersDao().getByAppId(steamAppId) | ||
| assertNotNull("Change number should exist", changeNumber) | ||
| assertEquals("Change number should match cloud", cloudChangeNumber, changeNumber!!.changeNumber) | ||
| } |
There was a problem hiding this comment.
Off-by-one: loop skips the last file; DB assertion should be outside the loop.
0..<count - 1 iterates over indices 0, 1 when count = 3, so save2.dat is never verified. The range should be 0..<count.
Additionally, the changeNumber assertion (Lines 1008–1010) is loop-invariant and should be moved after the loop. There's also a likely type mismatch: cloudChangeNumber is Int but changeNumber!!.changeNumber is likely Long, which will cause assertEquals to fail due to boxed type inequality (Integer vs Long).
Proposed fix
- for (i in 0..<count - 1) {
+ for (i in 0..<count) {
val expectedFile = File(saveFilesDir, "save$i.dat")
assertTrue("File $i should exist", expectedFile.exists())
assertEquals(
"File $i content should match",
contents[i].contentToString(),
expectedFile.readBytes().contentToString()
)
-
- val changeNumber = db.appChangeNumbersDao().getByAppId(steamAppId)
- assertNotNull("Change number should exist", changeNumber)
- assertEquals("Change number should match cloud", cloudChangeNumber, changeNumber!!.changeNumber)
}
+
+ val changeNumber = db.appChangeNumbersDao().getByAppId(steamAppId)
+ assertNotNull("Change number should exist", changeNumber)
+ assertEquals("Change number should match cloud", cloudChangeNumber.toLong(), changeNumber!!.changeNumber)
}🤖 Prompt for AI Agents
In `@app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt` around lines
999 - 1011, The test loop uses the wrong range and contains a loop-invariant DB
assertion: change the loop from "0..<count - 1" to "0..<count" so all save files
(save0..saveN-1) are verified (use the existing variables count, contents,
saveFilesDir), then move the retrieval/assertion of
db.appChangeNumbersDao().getByAppId(steamAppId) (the changeNumber checks)
outside the loop; finally, ensure the numeric types match when comparing
cloudChangeNumber and changeNumber!!.changeNumber (convert cloudChangeNumber to
Long or compare via toLong()/toInt() as appropriate) so assertEquals compares
same types.
| for (i in 0..<count - 1) { | ||
| val expectedFile = File(saveFilesDir, "save$i.dat") | ||
| assertTrue("File $i should exist", expectedFile.exists()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Searching for GameInstall handling in SteamAutoCloud ==="
rg -n 'GameInstall' --type=kt app/src/main/java/app/gamenative/service/
echo ""
echo "=== Searching for prefixToPath implementation ==="
rg -n -A10 'fun prefixToPath' --type=kt app/src/main/java/
echo ""
echo "=== Searching for downloadFileFromSteamCloud or similar download methods ==="
rg -n -B2 -A15 'downloadFileFromSteamCloud|downloadFile.*Steam' --type=kt app/src/main/java/app/gamenative/service/
echo ""
echo "=== Checking context around SteamAutoCloudTest.kt lines 945-1001 ==="
sed -n '940,1010p' app/src/test/java/app/gamenative/service/SteamAutoCloudTest.ktRepository: utkarshdalal/GameNative
Length of output: 3866
🏁 Script executed:
#!/bin/bash
echo "=== Finding SteamAutoCloud.kt file ==="
find . -name "SteamAutoCloud.kt" -type f
echo ""
echo "=== Reading SteamAutoCloud.kt to understand download logic ==="
# Find and read the file
if [ -f "app/src/main/java/app/gamenative/service/SteamAutoCloud.kt" ]; then
wc -l app/src/main/java/app/gamenative/service/SteamAutoCloud.kt
echo "---"
# Search for download-related methods
grep -n "download\|prefixToPath\|%GameInstall%" app/src/main/java/app/gamenative/service/SteamAutoCloud.kt | head -30
fi
echo ""
echo "=== Checking for path resolution logic in syncUserFiles ==="
if [ -f "app/src/main/java/app/gamenative/service/SteamAutoCloud.kt" ]; then
grep -n -A20 "fun syncUserFiles" app/src/main/java/app/gamenative/service/SteamAutoCloud.kt | head -50
fiRepository: utkarshdalal/GameNative
Length of output: 3554
🏁 Script executed:
#!/bin/bash
echo "=== Lines 140-170: Path construction logic ==="
sed -n '140,170p' app/src/main/java/app/gamenative/service/SteamAutoCloud.kt
echo ""
echo "=== Lines 301-350: Download file logic ==="
sed -n '301,350p' app/src/main/java/app/gamenative/service/SteamAutoCloud.kt
echo ""
echo "=== Lines 220-260: basePath construction ==="
sed -n '220,260p' app/src/main/java/app/gamenative/service/SteamAutoCloud.kt
echo ""
echo "=== Search for how downloaded file path is determined ==="
grep -n "prefixedPath\|actualFilePath" app/src/main/java/app/gamenative/service/SteamAutoCloud.kt | head -20Repository: utkarshdalal/GameNative
Length of output: 7261
🏁 Script executed:
#!/bin/bash
echo "=== Finding where AppFileChangeList mock response is set up in test ==="
grep -n "AppFileChangeList\|fileList\|responses\[" app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt | head -40
echo ""
echo "=== Reading test setup to find mock response construction ==="
sed -n '975,1010p' app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt
echo ""
echo "=== Searching for PathType enum to understand prefix types ==="
find . -name "PathType.kt" -o -name "PathType.java" | xargs grep -l "GameInstall\|DEFAULT\|SteamUserData" 2>/dev/null
echo ""
echo "=== Lines 85-140 to see convertPrefixes function ==="
sed -n '85,140p' app/src/main/java/app/gamenative/service/SteamAutoCloud.ktRepository: utkarshdalal/GameNative
Length of output: 7408
🏁 Script executed:
#!/bin/bash
echo "=== Finding mockFile creation (mockFile1, mockFile2, mockFile3) ==="
grep -n -B5 -A10 "mockFile1\|mockFile2\|mockFile3" app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt | head -80
echo ""
echo "=== Reading PathType.kt to understand enum values ==="
cat app/src/main/java/app/gamenative/enums/PathType.ktRepository: utkarshdalal/GameNative
Length of output: 15886
🏁 Script executed:
#!/bin/bash
echo "=== Finding where saveFilesDir is created/set in test ==="
grep -n "saveFilesDir" app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt | head -20
echo ""
echo "=== Reading lines around saveFilesDir creation ==="
sed -n '300,330p' app/src/test/java/app/gamenative/service/SteamAutoCloudTest.ktRepository: utkarshdalal/GameNative
Length of output: 2385
🏁 Script executed:
#!/bin/bash
echo "=== Finding saveGames definition in test setup ==="
sed -n '50,130p' app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt
echo ""
echo "=== Looking for tempDir definition ==="
grep -n "tempDir\|saveGames" app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt | head -30Repository: utkarshdalal/GameNative
Length of output: 4199
🏁 Script executed:
#!/bin/bash
echo "=== Finding 'count' variable in test ==="
grep -n "val count\|count =" app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt
echo ""
echo "=== Full test function starting from line 900 to understand context ==="
sed -n '900,950p' app/src/test/java/app/gamenative/service/SteamAutoCloudTest.ktRepository: utkarshdalal/GameNative
Length of output: 2379
🏁 Script executed:
#!/bin/bash
echo "=== Understanding convertPrefixes and how prefixes work ==="
sed -n '102,140p' app/src/main/java/app/gamenative/service/SteamAutoCloud.kt
echo ""
echo "=== Check what prefixToPath returns in test for WinMyDocuments ==="
sed -n '948,990p' app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt
echo ""
echo "=== Find testApp definition to see save patterns ==="
sed -n '130,180p' app/src/test/java/app/gamenative/service/SteamAutoCloudTest.ktRepository: utkarshdalal/GameNative
Length of output: 6025
🏁 Script executed:
#!/bin/bash
echo "=== Verify the actual mock filenames one more time ==="
sed -n '333,353p' app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt
echo ""
echo "=== Verify assertions check correct filenames ==="
sed -n '997,1005p' app/src/test/java/app/gamenative/service/SteamAutoCloudTest.ktRepository: utkarshdalal/GameNative
Length of output: 1698
Fix file assertions to match downloaded cloud filenames and location.
The test downloads 3 files with names cloud_save_1.sav, cloud_save_2.sav, and cloud_save_3.sav but the assertions check for files named save0.dat, save1.dat, etc. Additionally, the prefixToPath function returns tempDir for WinMyDocuments, so the downloaded files will be written to tempDir/My Games/TestGame/Steam/76561198025127569/cloud_save_1.sav rather than saveFilesDir/save0.dat. Update the assertions to verify the correct filenames and location:
for (i in 0..<count - 1) {
val expectedFile = File(saveFilesDir, "cloud_save_${i + 1}.sav")
assertTrue("File $i should exist", expectedFile.exists())
// ... rest of assertion
}Or, if save*.dat filenames are intentional, update the mock files and cloud prefix accordingly.
🤖 Prompt for AI Agents
In `@app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt` around lines
999 - 1001, The assertions in SteamAutoCloudTest's loop use the wrong expected
filenames and path: change the check that builds expectedFile from
File(saveFilesDir, "save$i.dat") to use the actual downloaded names and location
returned by prefixToPath (e.g. File(saveFilesDir, "cloud_save_${i + 1}.sav")),
and update any related assertions that inspect file contents or metadata to
reference the new name; alternatively, if save*.dat is intended, update the
test's mock cloud files and the cloud prefix used by the download logic so the
test and the mock data match. Ensure you modify the loop that iterates with
count and the expectedFile construction to use i + 1 and the "cloud_save_#.sav"
pattern, and verify saveFilesDir is the same directory produced by prefixToPath
for WinMyDocuments.
a54e9f7 to
50bd882
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt`:
- Around line 999-1001: The test currently asserts files exist under
saveFilesDir but when pathPrefixes is empty and downloads use the %GameInstall%
prefix the production resolver in SteamAutoCloud.kt (see prefixToPath and the
path resolution used in the substring logic around lines 147–151) writes files
into the Documents folder; update the assertions in SteamAutoCloudTest.kt (the
loop that checks "save$i.dat") to expect files under the resolved GameInstall
path (i.e., the result of prefixToPath("GameInstall") combined with the filename
portion) rather than under saveFilesDir so the test matches the actual download
location.
🧹 Nitpick comments (1)
app/src/test/java/app/gamenative/service/SteamAutoCloudTest.kt (1)
914-918: No bounds-check fallback ifclientFileDownloadis called more thancounttimes.Unlike the pattern in
testDownloadCloudSavesOnFirstBoot(line 400) which has a fallback for unexpected extra calls, this mock will throwArrayIndexOutOfBoundsExceptionif invoked beyondcount. Same applies toresponses[callCount - 1]on line 942. Consider adding a defensive fallback or a bounds check.
silentrald
force-pushed
the
fix/cloud-save-gameinstall
branch
from
50bd882 to
3bb48d5
Compare
| val prefixPath: String | ||
| get() = Paths.get(prefix, filename).pathString | ||
| get() = ( | ||
| if (prefix.endsWith('%')) "$prefix$filename" |
There was a problem hiding this comment.
why are we checking if the prefix ends with %?
There was a problem hiding this comment.
Path.get appends the slashes even if the pathing which breaks the pathing. ie.
%GameInstall%/save0.dat
That's why I added it there to remove it, unless we edit the prefixToPath call to handle this case.
There was a problem hiding this comment.
I see. This change is a bit scary to me, because when we're uploading it will see that file as deleted and delete it from Steam's remote storage.
For instance, it will consider %GameInstall%/save0.dat deleted, and try to upload a new file called %GameInstall%save0.dat
There was a problem hiding this comment.
Are you sure this is required?
There was a problem hiding this comment.
For instance, I see file paths in the existing tests that look like:
"%WinMyDocuments%/My Games/TestGame/Steam/76561198025127569"
So was the upload failing for you when you had a /? Can you please check again?
| val saveFilePatterns = listOf( | ||
| SaveFilePattern( | ||
| root = PathType.GameInstall, | ||
| path = ".", | ||
| pattern = "save0.sav", | ||
| ), | ||
| SaveFilePattern( | ||
| root = PathType.GameInstall, | ||
| path = ".", | ||
| pattern = "save1.sav", | ||
| ), | ||
| SaveFilePattern( | ||
| root = PathType.GameInstall, | ||
| path = ".", | ||
| pattern = "save2.sav", | ||
| ), | ||
| ) |
There was a problem hiding this comment.
i'm not following why path has to be .? What are we introducing that's new that's making the path .?
| ) { | ||
| val prefix: String | ||
| get() = Paths.get("%${root.name}%$path").pathString | ||
| get() = Paths.get("%${root.name}%${ if (path == ".") "" else path }").pathString |
There was a problem hiding this comment.
Why is the path becoming . and not an empty string?
There was a problem hiding this comment.
Seems like in
userFile.substitutedPath returned here is the .. If this is the case would you prefer the handling of the dot in the SteamAutoCloud class instead?
There was a problem hiding this comment.
Got it. Looks like sometimes Steam sends "." instead of "" as the path.
|
Applied in #508. Closing PR, thanks @utkarshdalal |
2 participants
Summary by cubic
Fixes Steam Auto Cloud path parsing when %GameInstall% is embedded in filenames and when the path is ".". Ensures files are placed correctly during sync and upload.
Written for commit 3bb48d5. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests