Closed
Conversation
utf8 string output handling (match gbe format) json formatting, key order
# Conflicts: # app/src/main/java/app/gamenative/utils/SteamUtils.kt
utf8 string output handling (match gbe format) json formatting, key order
…/GameNative into feat/steam-achievement
# Conflicts: # app/src/main/java/app/gamenative/service/SteamService.kt # app/src/main/java/app/gamenative/utils/SteamUtils.kt
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds achievement support: new preference and UI toggle, per-container propagation, VDF binary parser, stats/achievement generator, SteamService wiring for generation/sync/storage, utility trigger calls, localized strings, and a javasteam version bump. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI Layer
participant VM as MainViewModel
participant Service as SteamService
participant Parser as VdfParser
participant Generator as StatsAchievementsGenerator
participant FS as File I/O
UI->>VM: user triggers close / DLL replace
VM->>Service: closeApp(context, appId, isOffline, pathFn)
Service->>Service: findSteamSettingsDir(context, appId)
Service->>Service: generateAchievements(appId, configDir)
Service->>Parser: binaryLoads(schemaBytes)
Parser-->>Generator: parsed map
Generator->>FS: write achievements.json and stats.json
Generator-->>Service: ProcessingResult (name→block map)
Service->>Service: syncAchievementsFromGoldberg(context, appId)
Service->>FS: read Goldberg achievements.json
Service->>Service: storeAchievementUnlocks(appId, configDir, unlockedNames)
Service-->>VM: completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.
8 issues found across 25 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/statsgen/VdfParser.kt">
<violation number="1" location="app/src/main/java/app/gamenative/statsgen/VdfParser.kt:34">
P2: Unsupported VDF type bytes are not handled; the parser continues with a desynchronized stream and can silently return corrupted data.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/statsgen/VdfParser.kt:100">
P1: UINT64 values are parsed as signed `Long`, which corrupts values above `Long.MAX_VALUE` (they become negative).</violation>
</file>
<file name="app/src/main/java/app/gamenative/statsgen/StatsAchievementsGenerator.kt">
<violation number="1" location="app/src/main/java/app/gamenative/statsgen/StatsAchievementsGenerator.kt:17">
P1: Backslashes are not JSON-escaped correctly, which can produce malformed JSON output.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/statsgen/StatsAchievementsGenerator.kt:220">
P1: Fallback parsing uses `stat.min.toInt()` without validation, which can throw and abort generation.</violation>
<violation number="3" location="app/src/main/java/app/gamenative/statsgen/StatsAchievementsGenerator.kt:253">
P2: `progress` is added to achievement output but never serialized, so progress metadata is lost in `achievements.json`.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/SteamService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamService.kt:2683">
P2: `syncAchievementsFromGoldberg` reads and parses achievements but the `storeAchievementUnlocks` call is commented out, making it a no-op. The log message "syncing to Steam" is misleading since nothing is actually synced. If this is intentionally WIP, consider either removing the call from `closeApp` until the sync is ready, or at minimum fixing the log to say "found (not yet synced)".</violation>
</file>
<file name="app/src/main/java/app/gamenative/utils/SteamUtils.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/SteamUtils.kt:1337">
P2: Avoid `runBlocking` in this path; it blocks the caller thread during achievement generation.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/utils/SteamUtils.kt:1338">
P1: Guard achievement generation behind a valid Steam service/session state to prevent NPE crashes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return ByteBuffer.wrap(bytes).order(ByteOrder.LITTLE_ENDIAN).long | ||
| } | ||
|
|
||
| private fun readUInt64(stream: DataInputStream): Long { |
Contributor
There was a problem hiding this comment.
P1: UINT64 values are parsed as signed Long, which corrupts values above Long.MAX_VALUE (they become negative).
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/statsgen/VdfParser.kt, line 100:
<comment>UINT64 values are parsed as signed `Long`, which corrupts values above `Long.MAX_VALUE` (they become negative).</comment>
<file context>
@@ -0,0 +1,105 @@
+ return ByteBuffer.wrap(bytes).order(ByteOrder.LITTLE_ENDIAN).long
+ }
+
+ private fun readUInt64(stream: DataInputStream): Long {
+ val bytes = ByteArray(8)
+ stream.readFully(bytes)
</file context>
| char.code < 32 || char.code > 126 -> { | ||
| sb.append(String.format("\\u%04x", char.code)) | ||
| } | ||
| char == '\\' -> sb.append("\\") |
Contributor
There was a problem hiding this comment.
P1: Backslashes are not JSON-escaped correctly, which can produce malformed JSON output.
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/statsgen/StatsAchievementsGenerator.kt, line 17:
<comment>Backslashes are not JSON-escaped correctly, which can produce malformed JSON output.</comment>
<file context>
@@ -0,0 +1,349 @@
+ char.code < 32 || char.code > 126 -> {
+ sb.append(String.format("\\u%04x", char.code))
+ }
+ char == '\\' -> sb.append("\\")
+ char == '"' -> sb.append("\\\"")
+ else -> sb.append(char)
</file context>
Suggested change
| char == '\\' -> sb.append("\\") | |
| char == '\\' -> sb.append("\\\\") |
| globalNum = globalFloat.toString() | ||
| } catch (e2: NumberFormatException) { | ||
| if (!stat.min.isNullOrEmpty()) { | ||
| defaultNum = stat.min.toInt().toString() |
Contributor
There was a problem hiding this comment.
P1: Fallback parsing uses stat.min.toInt() without validation, which can throw and abort generation.
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/statsgen/StatsAchievementsGenerator.kt, line 220:
<comment>Fallback parsing uses `stat.min.toInt()` without validation, which can throw and abort generation.</comment>
<file context>
@@ -0,0 +1,349 @@
+ globalNum = globalFloat.toString()
+ } catch (e2: NumberFormatException) {
+ if (!stat.min.isNullOrEmpty()) {
+ defaultNum = stat.min.toInt().toString()
+ globalNum = "0"
+ } else {
</file context>
| } | ||
|
|
||
| runBlocking { | ||
| SteamService.generateAchievements(steamAppId, settingsDir.absolutePathString()) |
Contributor
There was a problem hiding this comment.
P1: Guard achievement generation behind a valid Steam service/session state to prevent NPE crashes.
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/utils/SteamUtils.kt, line 1338:
<comment>Guard achievement generation behind a valid Steam service/session state to prevent NPE crashes.</comment>
<file context>
@@ -1306,5 +1326,17 @@ object SteamUtils {
+ }
+
+ runBlocking {
+ SteamService.generateAchievements(steamAppId, settingsDir.absolutePathString())
+ }
+ }
</file context>
Suggested change
| SteamService.generateAchievements(steamAppId, settingsDir.absolutePathString()) | |
| if (SteamService.instance != null && SteamService.isLoggedIn) { | |
| runCatching { | |
| SteamService.generateAchievements(steamAppId, settingsDir.absolutePathString()) | |
| }.onFailure { error -> | |
| Timber.w(error, "Failed to generate achievements for $steamAppId") | |
| } | |
| } |
| break | ||
| } | ||
|
|
||
| when (dataType) { |
Contributor
There was a problem hiding this comment.
P2: Unsupported VDF type bytes are not handled; the parser continues with a desynchronized stream and can silently return corrupted data.
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/statsgen/VdfParser.kt, line 34:
<comment>Unsupported VDF type bytes are not handled; the parser continues with a desynchronized stream and can silently return corrupted data.</comment>
<file context>
@@ -0,0 +1,105 @@
+ break
+ }
+
+ when (dataType) {
+ VDF_END -> break
+ VDF_SUBSECTION -> {
</file context>
| jsonBuilder.append("[\n") | ||
|
|
||
| val orderedKeys = listOf( | ||
| "hidden", "displayName", "description", "icon", "icon_gray", "name", |
Contributor
There was a problem hiding this comment.
P2: progress is added to achievement output but never serialized, so progress metadata is lost in achievements.json.
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/statsgen/StatsAchievementsGenerator.kt, line 253:
<comment>`progress` is added to achievement output but never serialized, so progress metadata is lost in `achievements.json`.</comment>
<file context>
@@ -0,0 +1,349 @@
+ jsonBuilder.append("[\n")
+
+ val orderedKeys = listOf(
+ "hidden", "displayName", "description", "icon", "icon_gray", "name",
+ "unlocked", "unlockTimestamp", "formattedUnlockTime"
+ )
</file context>
| return | ||
| } | ||
|
|
||
| Timber.i("Found ${unlockedNames.size} earned achievements for appId=$appId, syncing to Steam") |
Contributor
There was a problem hiding this comment.
P2: syncAchievementsFromGoldberg reads and parses achievements but the storeAchievementUnlocks call is commented out, making it a no-op. The log message "syncing to Steam" is misleading since nothing is actually synced. If this is intentionally WIP, consider either removing the call from closeApp until the sync is ready, or at minimum fixing the log to say "found (not yet synced)".
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/SteamService.kt, line 2683:
<comment>`syncAchievementsFromGoldberg` reads and parses achievements but the `storeAchievementUnlocks` call is commented out, making it a no-op. The log message "syncing to Steam" is misleading since nothing is actually synced. If this is intentionally WIP, consider either removing the call from `closeApp` until the sync is ready, or at minimum fixing the log to say "found (not yet synced)".</comment>
<file context>
@@ -2600,6 +2611,164 @@ class SteamService : Service(), IChallengeUrlChanged {
+ return
+ }
+
+ Timber.i("Found ${unlockedNames.size} earned achievements for appId=$appId, syncing to Steam")
+// val result = storeAchievementUnlocks(appId, configDirectory, unlockedNames)
+// result.onSuccess {
</file context>
Suggested change
| Timber.i("Found ${unlockedNames.size} earned achievements for appId=$appId, syncing to Steam") | |
| Timber.i("Found ${unlockedNames.size} earned achievements for appId=$appId (sync not yet implemented)") |
| Files.createDirectories(settingsDir) | ||
| } | ||
|
|
||
| runBlocking { |
Contributor
There was a problem hiding this comment.
P2: Avoid runBlocking in this path; it blocks the caller thread during achievement generation.
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/utils/SteamUtils.kt, line 1337:
<comment>Avoid `runBlocking` in this path; it blocks the caller thread during achievement generation.</comment>
<file context>
@@ -1306,5 +1326,17 @@ object SteamUtils {
+ Files.createDirectories(settingsDir)
+ }
+
+ runBlocking {
+ SteamService.generateAchievements(steamAppId, settingsDir.absolutePathString())
+ }
</file context>
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/service/SteamService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/SteamService.kt:2755">
P1: `storeAchievementUnlocks` no longer persists unlock stats because the `storeUserStats(...)` call is commented out.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+2755
to
+2769
| // val callback = instance?._steamUserStats!!.storeUserStats( | ||
| // appId, statsToStore, mySteamId, mySteamId, userStats.crcStats | ||
| // ).await() | ||
| // if (callback.result != EResult.OK) { | ||
| // throw IllegalStateException("storeUserStats failed: ${callback.result}") | ||
| // } | ||
| // if (callback.statsOutOfDate) { | ||
| // Timber.w("Stats were out of date on server for appId=$appId") | ||
| // } | ||
| // if (callback.statsFailedValidation.isNotEmpty()) { | ||
| // Timber.w("${callback.statsFailedValidation.size} stats failed validation for appId=$appId") | ||
| // callback.statsFailedValidation.forEach { f -> | ||
| // Timber.w(" statId=${f.statId} reverted to ${f.revertedStatValue}") | ||
| // } | ||
| // } |
Contributor
There was a problem hiding this comment.
P1: storeAchievementUnlocks no longer persists unlock stats because the storeUserStats(...) call is commented out.
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/SteamService.kt, line 2755:
<comment>`storeAchievementUnlocks` no longer persists unlock stats because the `storeUserStats(...)` call is commented out.</comment>
<file context>
@@ -2752,21 +2752,21 @@ class SteamService : Service(), IChallengeUrlChanged {
- Timber.w(" statId=${f.statId} reverted to ${f.revertedStatValue}")
- }
- }
+// val callback = instance?._steamUserStats!!.storeUserStats(
+// appId, statsToStore, mySteamId, mySteamId, userStats.crcStats
+// ).await()
</file context>
Suggested change
| // val callback = instance?._steamUserStats!!.storeUserStats( | |
| // appId, statsToStore, mySteamId, mySteamId, userStats.crcStats | |
| // ).await() | |
| // if (callback.result != EResult.OK) { | |
| // throw IllegalStateException("storeUserStats failed: ${callback.result}") | |
| // } | |
| // if (callback.statsOutOfDate) { | |
| // Timber.w("Stats were out of date on server for appId=$appId") | |
| // } | |
| // if (callback.statsFailedValidation.isNotEmpty()) { | |
| // Timber.w("${callback.statsFailedValidation.size} stats failed validation for appId=$appId") | |
| // callback.statsFailedValidation.forEach { f -> | |
| // Timber.w(" statId=${f.statId} reverted to ${f.revertedStatValue}") | |
| // } | |
| // } | |
| val callback = instance?._steamUserStats!!.storeUserStats( | |
| appId, statsToStore, mySteamId, mySteamId, userStats.crcStats | |
| ).await() | |
| if (callback.result != EResult.OK) { | |
| throw IllegalStateException("storeUserStats failed: ${callback.result}") | |
| } | |
| if (callback.statsOutOfDate) { | |
| Timber.w("Stats were out of date on server for appId=$appId") | |
| } | |
| if (callback.statsFailedValidation.isNotEmpty()) { | |
| Timber.w("${callback.statsFailedValidation.size} stats failed validation for appId=$appId") | |
| callback.statsFailedValidation.forEach { f -> | |
| Timber.w(" statId=${f.statId} reverted to ${f.revertedStatValue}") | |
| } | |
| } |
![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: 13
🧹 Nitpick comments (3)
app/build.gradle.kts (1)
213-214: Derive the local JavaSteam JAR names from the version catalog.These paths duplicate the snapshot string that already lives in
libs.versions.toml, so the next JavaSteam bump can silently breaklocalBuilduntil someone remembers to update this block too.Suggested cleanup
// JavaSteam val localBuild = false // Change to 'true' needed when building JavaSteam manually + val javaSteamVersion = libs.versions.javasteam.get() if (localBuild) { - implementation(files("../../JavaSteam/build/libs/javasteam-1.8.0-12-SNAPSHOT.jar")) - implementation(files("../../JavaSteam/javasteam-depotdownloader/build/libs/javasteam-depotdownloader-1.8.0-12-SNAPSHOT.jar")) + implementation(files("../../JavaSteam/build/libs/javasteam-$javaSteamVersion.jar")) + implementation(files("../../JavaSteam/javasteam-depotdownloader/build/libs/javasteam-depotdownloader-$javaSteamVersion.jar")) implementation(libs.bundles.javasteam.dev)Please verify the exact version-catalog accessor syntax available in this Gradle Kotlin DSL module before applying the diff.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/build.gradle.kts` around lines 213 - 214, The two implementation(files(...)) entries hardcode the JavaSteam snapshot version string; change them to derive the version from the version catalog instead of embedding "1.8.0-12-SNAPSHOT". Use the version-catalog accessor available in Kotlin DSL (e.g. consult libs.findVersion("...") or libs.versions.<alias>.get() in this module) to read the JavaSteam version and interpolate it into the local JAR filenames for both the javasteam and javasteam-depotdownloader paths, updating the implementation(files(...)) calls to use that constructed filename; verify the exact libs.* accessor syntax in this Gradle Kotlin DSL module before applying.app/src/main/java/app/gamenative/service/SteamService.kt (2)
2683-2690: Achievement sync to Steam is disabled.The
storeAchievementUnlockscall is commented out, so achievements are detected but not actually synced to Steam. If this is intentional WIP, consider adding a TODO comment explaining when this will be enabled. If unintentional, uncomment the code.📝 Suggested documentation
Timber.i("Found ${unlockedNames.size} earned achievements for appId=$appId, syncing to Steam") -// val result = storeAchievementUnlocks(appId, configDirectory, unlockedNames) -// result.onSuccess { -// Timber.i("Successfully synced achievements to Steam for appId=$appId") -// }.onFailure { e -> -// Timber.e(e, "Failed to sync achievements to Steam for appId=$appId") -// } + // TODO: Enable when storeAchievementUnlocks is tested and ready for production + // val result = storeAchievementUnlocks(appId, configDirectory, unlockedNames) + // result.onSuccess { + // Timber.i("Successfully synced achievements to Steam for appId=$appId") + // }.onFailure { e -> + // Timber.e(e, "Failed to sync achievements to Steam for appId=$appId") + // }Would you like me to open an issue to track enabling achievement sync?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/SteamService.kt` around lines 2683 - 2690, The achievement-sync code is commented out: the call to storeAchievementUnlocks(appId, configDirectory, unlockedNames) inside the SteamService flow is disabled so detected unlockedNames are never sent to Steam; either uncomment the block that calls storeAchievementUnlocks and restores the onSuccess/onFailure Timber logging (keeping the existing error handling with Timber.e(e,...)), or if this is intentional WIP add a clear TODO comment above the Timber.i line explaining when/why syncing is disabled and that storeAchievementUnlocks (and its result.onSuccess/onFailure handlers) will be re-enabled later; reference the storeAchievementUnlocks call, the unlockedNames variable and the surrounding Timber.i log to locate the code.
164-164: Unused import.The
Achievementimport does not appear to be used anywhere in this file. Consider removing it to keep imports clean.🧹 Proposed fix
-import app.gamenative.statsgen.Achievement🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/SteamService.kt` at line 164, Remove the unused import statement for Achievement (import app.gamenative.statsgen.Achievement) from SteamService.kt; locate the import at the top of the file, delete it, and run a build/IDE cleanup to ensure no usages remain and imports are reorganized.
🤖 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`:
- Line 221: clearValues() currently nulls other Steam handlers but omits
`_steamUserStats`, leaving a stale reference; update the `clearValues()` method
to set `_steamUserStats` to null alongside `_steamUser`, `_steamApps`,
`_steamFriends`, and `_steamCloud` so the SteamService no longer retains the
handler after logout/disconnect and allows proper GC of the SteamUserStats
instance.
- Around line 2692-2709: The call to ContainerUtils.getContainer in
findSteamSettingsDir can throw and needs to be guarded so it doesn't crash
syncAchievementsFromGoldberg; wrap the getContainer(context, "STEAM_$appId")
invocation in a try-catch that catches Exception, log or report the error (using
the existing logging mechanism) and treat it as a missing container by returning
null or continuing, ensuring findSteamSettingsDir returns null when the
container lookup fails rather than letting the exception propagate.
- Around line 2615-2633: The generateAchievements function uses unsafe
force-unwraps and doesn’t validate the getUserStats result; update
generateAchievements to first null-check instance, instance._steamUser and
instance._steamUserStats and return early with a log if any are null (avoid
using !!), then call getUserStats(appId, steamUser.steamID).await() and validate
the returned object for success and non-null schema before accessing
schema.toByteArray(), and handle/ log failure cases; after obtaining a valid
schema call StatsAchievementsGenerator.generateStatsAchievements and verify its
result (non-null nameToBlockBit) before writing the
achievement_name_to_block.json (ensure configDirectory creation still occurs),
and ensure callers (e.g., SteamUtils.generateAchievementsFile) only runBlocking
this method after the service is initialized or add an internal initialized
check to fail-fast with a clear log instead of crashing.
- Around line 2730-2756: This code uses unsafe non-null asserts (instance!!,
_steamUserStats!!) and throws exceptions directly—refactor the storeUserStats
flow in the same way as generateAchievements: null-check instance and
instance._steamUserStats and return Result.failure(Exception(...)) if missing,
avoid NPEs by using safe calls (?.) and early Result.failure returns, and
convert the getUserStats error path (userStats.result != EResult.OK) to return
Result.failure(...) instead of throwing; update references in this block
(steamUser, userStats, blockBitmasks, and the call to
instance._steamUserStats.storeUserStats) to use the validated local variables or
safe operators so no !! remains.
In `@app/src/main/java/app/gamenative/statsgen/StatsAchievementsGenerator.kt`:
- Around line 10-23: The serializer is dropping the added fields and
mis-escaping backslashes: update the code that builds orderedKeys so it includes
"progress" and "icongray" in the emitted keys (ensure the same fix is applied to
the other identical key-building sites referenced in the diff), and fix
escapeUnicode(text: String) so backslashes are escaped as "\\" (i.e., emit a
double backslash) instead of a single "\"; reference the function escapeUnicode
and the orderedKeys/ outputAch construction sites to locate and apply these
changes.
- Around line 309-339: The current write block is skipped when outputStats is
empty, leaving a stale stats.json; modify the logic around outputStats so that
statsFile (constructed with configDir and "stats.json") is always updated: if
outputStats.isEmpty() write an empty JSON array "[]" (or delete the file) to
statsFile, otherwise perform the existing JSON-building and call
statsFile.writeText(..., Charsets.UTF_8); ensure this change touches the same
symbols (outputStats, statsFile, configDir, writeText) so no stale file remains.
In `@app/src/main/java/app/gamenative/statsgen/VdfParser.kt`:
- Around line 34-66: The when block handling VDF tag bytes in VdfParser.kt
currently omits a default case, so unknown dataType values are silently ignored
and cause misaligned parsing; update the when(dataType) in the parsing loop to
include an else (or default) branch that throws a clear exception (e.g.,
IllegalStateException or IOException) including the unexpected dataType and
context (e.g., current stream position or parent key) so parsing fails fast;
reference the same local symbols (dataType, readString(stream),
parseVdfData(stream), readInt32/Float32/Int64/readUInt64) when adding the guard.
- Around line 100-103: readUInt64 currently returns a signed Long which corrupts
unsigned 64-bit fields; change the signature of readUInt64 to return ULong and
convert the read signed long to unsigned (e.g. call
ByteBuffer.wrap(...).order(...).long.toULong() or otherwise reinterpret the
bytes into a ULong) so VDF_UINT64 is preserved; update any callers of readUInt64
to expect ULong (or add a separate readUInt64Unsigned helper if you need to
preserve the existing Long-returning readInt64).
In `@app/src/main/java/app/gamenative/utils/ContainerUtils.kt`:
- Line 473: The toggle persistence misses invalidation: when
container.putExtra("showAchievementPopups", containerData.showAchievementPopups)
changes, clear the Steam setup markers so the Steam setup reruns; specifically
detect when showAchievementPopups has flipped compared to stored state and
remove/reset the STEAM_DLL_REPLACED and STEAM_COLDCLIENT_USED markers (and any
related configs.overlay.ini short-circuit flags) so the Steam setup path (in the
same ContainerUtils handling) will regenerate the overlay config on next launch.
In `@app/src/main/java/app/gamenative/utils/SteamUtils.kt`:
- Around line 224-225: The generateAchievementsFile call currently uses
rootPath.resolve("steam_settings") which writes achievements.json to the app
root instead of next to each discovered DLL; change the call inside the loop
that runs ensureSteamSettings(...) to pass the per-DLL settings directory (the
same 'path' used for ensureSteamSettings, e.g., path.resolve("steam_settings"))
so generateAchievementsFile writes achievements.json beside each steam_api*.dll;
update the invocation in the loop where ensureSteamSettings and appId are
available (referencing generateAchievementsFile and ensureSteamSettings) to use
the per-DLL path rather than rootPath.
In `@app/src/main/java/com/winlator/container/ContainerData.kt`:
- Line 89: The merged ContainerData constructed in IntentLaunchManager (the
manual merge in IntentLaunchManager.kt around the ContainerData(...) call) is
missing the new showAchievementPopups field, so any temporary override drops the
saved setting; update that merged ContainerData construction to carry
showAchievementPopups through by selecting the override value when present or
falling back to the base container value (i.e., set showAchievementPopups =
override.showAchievementPopups ?: base.showAchievementPopups or the equivalent
logic used for other fields) so the popup preference is preserved during merges.
In `@app/src/main/res/values-it/strings.xml`:
- Line 537: Update the Italian string resource for show_achievement_popups to a
more natural phrasing; replace the current value "Mostra popup obiettivi" with
either "Mostra popup degli obiettivi" or "Mostra notifiche degli obiettivi" in
the strings.xml entry for show_achievement_popups so the settings label reads
fluently in Italian.
In `@app/src/main/res/values-ro/strings.xml`:
- Line 538: The Romanian string value for resource show_achievement_popups reads
unnaturally; update the value of the string resource named
show_achievement_popups to a more natural phrasing such as "Afișează popup‑uri
pentru realizări" or "Afișează notificări de realizări" so the label reads
correctly in Romanian while keeping the resource name unchanged.
---
Nitpick comments:
In `@app/build.gradle.kts`:
- Around line 213-214: The two implementation(files(...)) entries hardcode the
JavaSteam snapshot version string; change them to derive the version from the
version catalog instead of embedding "1.8.0-12-SNAPSHOT". Use the
version-catalog accessor available in Kotlin DSL (e.g. consult
libs.findVersion("...") or libs.versions.<alias>.get() in this module) to read
the JavaSteam version and interpolate it into the local JAR filenames for both
the javasteam and javasteam-depotdownloader paths, updating the
implementation(files(...)) calls to use that constructed filename; verify the
exact libs.* accessor syntax in this Gradle Kotlin DSL module before applying.
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 2683-2690: The achievement-sync code is commented out: the call to
storeAchievementUnlocks(appId, configDirectory, unlockedNames) inside the
SteamService flow is disabled so detected unlockedNames are never sent to Steam;
either uncomment the block that calls storeAchievementUnlocks and restores the
onSuccess/onFailure Timber logging (keeping the existing error handling with
Timber.e(e,...)), or if this is intentional WIP add a clear TODO comment above
the Timber.i line explaining when/why syncing is disabled and that
storeAchievementUnlocks (and its result.onSuccess/onFailure handlers) will be
re-enabled later; reference the storeAchievementUnlocks call, the unlockedNames
variable and the surrounding Timber.i log to locate the code.
- Line 164: Remove the unused import statement for Achievement (import
app.gamenative.statsgen.Achievement) from SteamService.kt; locate the import at
the top of the file, delete it, and run a build/IDE cleanup to ensure no usages
remain and imports are reorganized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ccd485fd-c7d1-4fca-9ced-1695713cc12f
📒 Files selected for processing (25)
app/build.gradle.ktsapp/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/statsgen/Models.ktapp/src/main/java/app/gamenative/statsgen/StatsAchievementsGenerator.ktapp/src/main/java/app/gamenative/statsgen/VdfParser.ktapp/src/main/java/app/gamenative/ui/component/dialog/GeneralTab.ktapp/src/main/java/app/gamenative/ui/model/MainViewModel.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/app/gamenative/utils/SteamUtils.ktapp/src/main/java/com/winlator/container/ContainerData.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-uk/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values-zh-rTW/strings.xmlapp/src/main/res/values/strings.xmlgradle/libs.versions.toml
| private var _steamApps: SteamApps? = null | ||
| private var _steamFriends: SteamFriends? = null | ||
| private var _steamCloud: SteamCloud? = null | ||
| private var _steamUserStats: SteamUserStats? = null |
Contributor
There was a problem hiding this comment.
Missing cleanup of _steamUserStats in clearValues().
Other Steam handlers (_steamUser, _steamApps, _steamFriends, _steamCloud) are set to null in clearValues() around line 3007-3010, but _steamUserStats is not. This could lead to holding a stale reference after logout or disconnect.
🛠️ Proposed fix in clearValues()
Add this line alongside the other handler cleanup:
_steamUserStats = null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/service/SteamService.kt` at line 221,
clearValues() currently nulls other Steam handlers but omits `_steamUserStats`,
leaving a stale reference; update the `clearValues()` method to set
`_steamUserStats` to null alongside `_steamUser`, `_steamApps`, `_steamFriends`,
and `_steamCloud` so the SteamService no longer retains the handler after
logout/disconnect and allows proper GC of the SteamUserStats instance.
Comment on lines
+2615
to
+2633
| suspend fun generateAchievements(appId: Int, configDirectory: String) { | ||
| val steamUser = instance!!._steamUser!! | ||
| val userStats = instance?._steamUserStats!!.getUserStats(appId, steamUser.steamID!!).await() | ||
| val schemaArray = userStats.schema.toByteArray() | ||
| val generator = StatsAchievementsGenerator() | ||
| val result = generator.generateStatsAchievements(schemaArray, configDirectory) | ||
|
|
||
| val nameToBlockBit = result.nameToBlockBit | ||
| Timber.d("nameToBlockBit size=${nameToBlockBit.size} for appId=$appId") | ||
| if (nameToBlockBit.isNotEmpty()) { | ||
| val configDir = File(configDirectory) | ||
| if (!configDir.exists()) configDir.mkdirs() | ||
| val mappingJson = JSONObject() | ||
| nameToBlockBit.forEach { (name, pair) -> | ||
| mappingJson.put(name, JSONArray(listOf(pair.first, pair.second))) | ||
| } | ||
| File(configDir, "achievement_name_to_block.json").writeText(mappingJson.toString(), Charsets.UTF_8) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Improve null safety and add result validation.
This function has several null safety concerns:
- Line 2616-2617: Mixing
instance!!andinstance?._steamUserStats!!is inconsistent and risky. Ifinstanceor_steamUserStatsis null, this will crash. - No result validation:
getUserStatsreturns a callback that should be checked for success before accessingschema. - Called from
runBlocking: PerSteamUtils.generateAchievementsFile, this is called viarunBlockingwithout checking if the service is initialized, which could cause crashes during early container setup.
🛠️ Proposed fix with proper null safety and result checking
suspend fun generateAchievements(appId: Int, configDirectory: String) {
- val steamUser = instance!!._steamUser!!
- val userStats = instance?._steamUserStats!!.getUserStats(appId, steamUser.steamID!!).await()
+ val steamUser = instance?._steamUser ?: run {
+ Timber.w("Cannot generate achievements: steamUser not initialized")
+ return
+ }
+ val steamUserStats = instance?._steamUserStats ?: run {
+ Timber.w("Cannot generate achievements: steamUserStats not initialized")
+ return
+ }
+ val steamId = steamUser.steamID ?: run {
+ Timber.w("Cannot generate achievements: steamID is null")
+ return
+ }
+ val userStats = steamUserStats.getUserStats(appId, steamId).await()
+ if (userStats.result != EResult.OK) {
+ Timber.w("getUserStats failed for appId=$appId: ${userStats.result}")
+ return
+ }
val schemaArray = userStats.schema.toByteArray()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| suspend fun generateAchievements(appId: Int, configDirectory: String) { | |
| val steamUser = instance!!._steamUser!! | |
| val userStats = instance?._steamUserStats!!.getUserStats(appId, steamUser.steamID!!).await() | |
| val schemaArray = userStats.schema.toByteArray() | |
| val generator = StatsAchievementsGenerator() | |
| val result = generator.generateStatsAchievements(schemaArray, configDirectory) | |
| val nameToBlockBit = result.nameToBlockBit | |
| Timber.d("nameToBlockBit size=${nameToBlockBit.size} for appId=$appId") | |
| if (nameToBlockBit.isNotEmpty()) { | |
| val configDir = File(configDirectory) | |
| if (!configDir.exists()) configDir.mkdirs() | |
| val mappingJson = JSONObject() | |
| nameToBlockBit.forEach { (name, pair) -> | |
| mappingJson.put(name, JSONArray(listOf(pair.first, pair.second))) | |
| } | |
| File(configDir, "achievement_name_to_block.json").writeText(mappingJson.toString(), Charsets.UTF_8) | |
| } | |
| } | |
| suspend fun generateAchievements(appId: Int, configDirectory: String) { | |
| val steamUser = instance?._steamUser ?: run { | |
| Timber.w("Cannot generate achievements: steamUser not initialized") | |
| return | |
| } | |
| val steamUserStats = instance?._steamUserStats ?: run { | |
| Timber.w("Cannot generate achievements: steamUserStats not initialized") | |
| return | |
| } | |
| val steamId = steamUser.steamID ?: run { | |
| Timber.w("Cannot generate achievements: steamID is null") | |
| return | |
| } | |
| val userStats = steamUserStats.getUserStats(appId, steamId).await() | |
| if (userStats.result != EResult.OK) { | |
| Timber.w("getUserStats failed for appId=$appId: ${userStats.result}") | |
| return | |
| } | |
| val schemaArray = userStats.schema.toByteArray() | |
| val generator = StatsAchievementsGenerator() | |
| val result = generator.generateStatsAchievements(schemaArray, configDirectory) | |
| val nameToBlockBit = result.nameToBlockBit | |
| Timber.d("nameToBlockBit size=${nameToBlockBit.size} for appId=$appId") | |
| if (nameToBlockBit.isNotEmpty()) { | |
| val configDir = File(configDirectory) | |
| if (!configDir.exists()) configDir.mkdirs() | |
| val mappingJson = JSONObject() | |
| nameToBlockBit.forEach { (name, pair) -> | |
| mappingJson.put(name, JSONArray(listOf(pair.first, pair.second))) | |
| } | |
| File(configDir, "achievement_name_to_block.json").writeText(mappingJson.toString(), Charsets.UTF_8) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/service/SteamService.kt` around lines 2615 -
2633, The generateAchievements function uses unsafe force-unwraps and doesn’t
validate the getUserStats result; update generateAchievements to first
null-check instance, instance._steamUser and instance._steamUserStats and return
early with a log if any are null (avoid using !!), then call getUserStats(appId,
steamUser.steamID).await() and validate the returned object for success and
non-null schema before accessing schema.toByteArray(), and handle/ log failure
cases; after obtaining a valid schema call
StatsAchievementsGenerator.generateStatsAchievements and verify its result
(non-null nameToBlockBit) before writing the achievement_name_to_block.json
(ensure configDirectory creation still occurs), and ensure callers (e.g.,
SteamUtils.generateAchievementsFile) only runBlocking this method after the
service is initialized or add an internal initialized check to fail-fast with a
clear log instead of crashing.
Comment on lines
+2692
to
+2709
| private fun findSteamSettingsDir(context: Context, appId: Int): String? { | ||
| val appDirPath = getAppDirPath(appId) | ||
| val appDirSettings = File(appDirPath, "steam_settings") | ||
| if (File(appDirSettings, "achievement_name_to_block.json").exists()) { | ||
| return appDirSettings.absolutePath | ||
| } | ||
|
|
||
| val container = ContainerUtils.getContainer(context, "STEAM_$appId") | ||
| val coldclientSettings = File( | ||
| container.rootDir, | ||
| ".wine/drive_c/Program Files (x86)/Steam/steam_settings" | ||
| ) | ||
| if (File(coldclientSettings, "achievement_name_to_block.json").exists()) { | ||
| return coldclientSettings.absolutePath | ||
| } | ||
|
|
||
| return null | ||
| } |
Contributor
There was a problem hiding this comment.
Missing exception handling for getContainer call.
ContainerUtils.getContainer throws an Exception if the container doesn't exist for the given appId. This exception will propagate up and could crash the achievement sync flow in syncAchievementsFromGoldberg.
🛡️ Proposed fix with try-catch
private fun findSteamSettingsDir(context: Context, appId: Int): String? {
val appDirPath = getAppDirPath(appId)
val appDirSettings = File(appDirPath, "steam_settings")
if (File(appDirSettings, "achievement_name_to_block.json").exists()) {
return appDirSettings.absolutePath
}
- val container = ContainerUtils.getContainer(context, "STEAM_$appId")
- val coldclientSettings = File(
- container.rootDir,
- ".wine/drive_c/Program Files (x86)/Steam/steam_settings"
- )
- if (File(coldclientSettings, "achievement_name_to_block.json").exists()) {
- return coldclientSettings.absolutePath
+ try {
+ val container = ContainerUtils.getContainer(context, "STEAM_$appId")
+ val coldclientSettings = File(
+ container.rootDir,
+ ".wine/drive_c/Program Files (x86)/Steam/steam_settings"
+ )
+ if (File(coldclientSettings, "achievement_name_to_block.json").exists()) {
+ return coldclientSettings.absolutePath
+ }
+ } catch (e: Exception) {
+ Timber.d("Container not found for appId=$appId: ${e.message}")
}
return null
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/service/SteamService.kt` around lines 2692 -
2709, The call to ContainerUtils.getContainer in findSteamSettingsDir can throw
and needs to be guarded so it doesn't crash syncAchievementsFromGoldberg; wrap
the getContainer(context, "STEAM_$appId") invocation in a try-catch that catches
Exception, log or report the error (using the existing logging mechanism) and
treat it as a missing container by returning null or continuing, ensuring
findSteamSettingsDir returns null when the container lookup fails rather than
letting the exception propagate.
Comment on lines
+2730
to
+2756
| val steamUser = instance!!._steamUser!! | ||
| val userStats = instance?._steamUserStats!!.getUserStats(appId, steamUser.steamID!!).await() | ||
| if (userStats.result != EResult.OK) { | ||
| throw IllegalStateException("getUserStats failed: ${userStats.result}") | ||
| } | ||
| val rawBlocks = userStats.achievementBlocks ?: emptyList() | ||
| val blockBitmasks = mutableMapOf<Int, Int>() | ||
| for (block in rawBlocks) { | ||
| val blockId = (block.achievementId as? Number)?.toInt() ?: continue | ||
| var bitmask = 0 | ||
| val unlockTimes = block.unlockTime ?: emptyList() | ||
| for (i in unlockTimes.indices) { | ||
| val t = unlockTimes[i] | ||
| if ((t as? Number)?.toLong() != 0L) bitmask = bitmask or (1 shl i) | ||
| } | ||
| blockBitmasks[blockId] = bitmask | ||
| } | ||
| for (name in unlockedNames) { | ||
| val (blockId, bitIndex) = nameToBlockBit[name] ?: continue | ||
| val current = blockBitmasks.getOrDefault(blockId, 0) | ||
| blockBitmasks[blockId] = current or (1 shl bitIndex) | ||
| } | ||
| val statsToStore = blockBitmasks.map { (id, mask) -> Stats(statId = id, statValue = mask) } | ||
| Timber.d("storeUserStats: appId=$appId, crcStats=${userStats.crcStats}, stats=$statsToStore") | ||
| val mySteamId = steamUser.steamID!! | ||
| val callback = instance?._steamUserStats!!.storeUserStats( | ||
| appId, statsToStore, mySteamId, mySteamId, userStats.crcStats |
Contributor
There was a problem hiding this comment.
Same null safety issues as generateAchievements.
Lines 2730-2731 and 2754-2756 use the same risky instance!! and _steamUserStats!! patterns. Since this function returns Result<Unit>, failures should be captured as Result.failure() rather than throwing NPEs.
🛠️ Proposed fix with proper null safety
suspend fun storeAchievementUnlocks(
appId: Int,
configDirectory: String,
unlockedNames: Set<String>
): Result<Unit> = runCatching {
val mappingFile = File(configDirectory, "achievement_name_to_block.json")
if (!mappingFile.exists()) {
throw IllegalStateException("achievement_name_to_block.json not found in $configDirectory")
}
// ... existing JSON parsing ...
if (nameToBlockBit.isEmpty()) return@runCatching
- val steamUser = instance!!._steamUser!!
- val userStats = instance?._steamUserStats!!.getUserStats(appId, steamUser.steamID!!).await()
+ val steamUser = instance?._steamUser
+ ?: throw IllegalStateException("SteamUser not initialized")
+ val steamUserStats = instance?._steamUserStats
+ ?: throw IllegalStateException("SteamUserStats not initialized")
+ val steamId = steamUser.steamID
+ ?: throw IllegalStateException("SteamID is null")
+ val userStats = steamUserStats.getUserStats(appId, steamId).await()
// ... rest of function ...
- val mySteamId = steamUser.steamID!!
- val callback = instance?._steamUserStats!!.storeUserStats(
+ val callback = steamUserStats.storeUserStats(
- appId, statsToStore, mySteamId, mySteamId, userStats.crcStats
+ appId, statsToStore, steamId, steamId, userStats.crcStats
).await()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val steamUser = instance!!._steamUser!! | |
| val userStats = instance?._steamUserStats!!.getUserStats(appId, steamUser.steamID!!).await() | |
| if (userStats.result != EResult.OK) { | |
| throw IllegalStateException("getUserStats failed: ${userStats.result}") | |
| } | |
| val rawBlocks = userStats.achievementBlocks ?: emptyList() | |
| val blockBitmasks = mutableMapOf<Int, Int>() | |
| for (block in rawBlocks) { | |
| val blockId = (block.achievementId as? Number)?.toInt() ?: continue | |
| var bitmask = 0 | |
| val unlockTimes = block.unlockTime ?: emptyList() | |
| for (i in unlockTimes.indices) { | |
| val t = unlockTimes[i] | |
| if ((t as? Number)?.toLong() != 0L) bitmask = bitmask or (1 shl i) | |
| } | |
| blockBitmasks[blockId] = bitmask | |
| } | |
| for (name in unlockedNames) { | |
| val (blockId, bitIndex) = nameToBlockBit[name] ?: continue | |
| val current = blockBitmasks.getOrDefault(blockId, 0) | |
| blockBitmasks[blockId] = current or (1 shl bitIndex) | |
| } | |
| val statsToStore = blockBitmasks.map { (id, mask) -> Stats(statId = id, statValue = mask) } | |
| Timber.d("storeUserStats: appId=$appId, crcStats=${userStats.crcStats}, stats=$statsToStore") | |
| val mySteamId = steamUser.steamID!! | |
| val callback = instance?._steamUserStats!!.storeUserStats( | |
| appId, statsToStore, mySteamId, mySteamId, userStats.crcStats | |
| val steamUser = instance?._steamUser | |
| ?: throw IllegalStateException("SteamUser not initialized") | |
| val steamUserStats = instance?._steamUserStats | |
| ?: throw IllegalStateException("SteamUserStats not initialized") | |
| val steamId = steamUser.steamID | |
| ?: throw IllegalStateException("SteamID is null") | |
| val userStats = steamUserStats.getUserStats(appId, steamId).await() | |
| if (userStats.result != EResult.OK) { | |
| throw IllegalStateException("getUserStats failed: ${userStats.result}") | |
| } | |
| val rawBlocks = userStats.achievementBlocks ?: emptyList() | |
| val blockBitmasks = mutableMapOf<Int, Int>() | |
| for (block in rawBlocks) { | |
| val blockId = (block.achievementId as? Number)?.toInt() ?: continue | |
| var bitmask = 0 | |
| val unlockTimes = block.unlockTime ?: emptyList() | |
| for (i in unlockTimes.indices) { | |
| val t = unlockTimes[i] | |
| if ((t as? Number)?.toLong() != 0L) bitmask = bitmask or (1 shl i) | |
| } | |
| blockBitmasks[blockId] = bitmask | |
| } | |
| for (name in unlockedNames) { | |
| val (blockId, bitIndex) = nameToBlockBit[name] ?: continue | |
| val current = blockBitmasks.getOrDefault(blockId, 0) | |
| blockBitmasks[blockId] = current or (1 shl bitIndex) | |
| } | |
| val statsToStore = blockBitmasks.map { (id, mask) -> Stats(statId = id, statValue = mask) } | |
| Timber.d("storeUserStats: appId=$appId, crcStats=${userStats.crcStats}, stats=$statsToStore") | |
| val callback = steamUserStats.storeUserStats( | |
| appId, statsToStore, steamId, steamId, userStats.crcStats |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/service/SteamService.kt` around lines 2730 -
2756, This code uses unsafe non-null asserts (instance!!, _steamUserStats!!) and
throws exceptions directly—refactor the storeUserStats flow in the same way as
generateAchievements: null-check instance and instance._steamUserStats and
return Result.failure(Exception(...)) if missing, avoid NPEs by using safe calls
(?.) and early Result.failure returns, and convert the getUserStats error path
(userStats.result != EResult.OK) to return Result.failure(...) instead of
throwing; update references in this block (steamUser, userStats, blockBitmasks,
and the call to instance._steamUserStats.storeUserStats) to use the validated
local variables or safe operators so no !! remains.
Comment on lines
+10
to
+23
| private fun escapeUnicode(text: String): String { | ||
| val sb = StringBuilder() | ||
| for (char in text) { | ||
| when { | ||
| char.code < 32 || char.code > 126 -> { | ||
| sb.append(String.format("\\u%04x", char.code)) | ||
| } | ||
| char == '\\' -> sb.append("\\") | ||
| char == '"' -> sb.append("\\\"") | ||
| else -> sb.append(char) | ||
| } | ||
| } | ||
| return sb.toString() | ||
| } |
Contributor
There was a problem hiding this comment.
The achievements serializer is already dropping fields and mis-escaping strings.
progress and icongray are added to outputAch, but orderedKeys never emits them. Also escapeUnicode() only writes a single \ for backslashes, so any path or localized text containing \ produces invalid JSON.
Minimal fix
- char == '\\' -> sb.append("\\")
+ char == '\\' -> sb.append("\\\\")
...
val orderedKeys = listOf(
- "hidden", "displayName", "description", "icon", "icon_gray", "name",
- "unlocked", "unlockTimestamp", "formattedUnlockTime"
+ "hidden", "displayName", "description", "icon", "icon_gray", "icongray", "name",
+ "progress", "unlocked", "unlockTimestamp", "formattedUnlockTime"
)Also applies to: 180-187, 252-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/statsgen/StatsAchievementsGenerator.kt`
around lines 10 - 23, The serializer is dropping the added fields and
mis-escaping backslashes: update the code that builds orderedKeys so it includes
"progress" and "icongray" in the emitted keys (ensure the same fix is applied to
the other identical key-building sites referenced in the diff), and fix
escapeUnicode(text: String) so backslashes are escaped as "\\" (i.e., emit a
double backslash) instead of a single "\"; reference the function escapeUnicode
and the orderedKeys/ outputAch construction sites to locate and apply these
changes.
| container.setExternalDisplaySwap(containerData.externalDisplaySwap) | ||
| container.setForceDlc(containerData.forceDlc) | ||
| container.setSteamOfflineMode(containerData.steamOfflineMode) | ||
| container.putExtra("showAchievementPopups", containerData.showAchievementPopups) |
Contributor
There was a problem hiding this comment.
Changing this toggle won't take effect after the first Steam setup.
We persist the extra, but we never clear STEAM_DLL_REPLACED / STEAM_COLDCLIENT_USED when showAchievementPopups flips. After the first launch the Steam setup paths short-circuit, so configs.overlay.ini keeps the old state until some unrelated setting invalidates those markers.
Suggested invalidation
+ val previousShowAchievementPopups =
+ container.getExtra("showAchievementPopups", "false").toBoolean()
...
container.putExtra("showAchievementPopups", containerData.showAchievementPopups)
...
+ if (previousShowAchievementPopups != containerData.showAchievementPopups) {
+ val steamAppId = extractGameIdFromContainerId(container.id)
+ val appDirPath = SteamService.getAppDirPath(steamAppId)
+ MarkerUtils.removeMarker(appDirPath, Marker.STEAM_DLL_REPLACED)
+ MarkerUtils.removeMarker(appDirPath, Marker.STEAM_COLDCLIENT_USED)
+ Timber.i(
+ "showAchievementPopups changed from '$previousShowAchievementPopups' to '${containerData.showAchievementPopups}'. " +
+ "Cleared Steam markers for container ${container.id}."
+ )
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| container.putExtra("showAchievementPopups", containerData.showAchievementPopups) | |
| val previousShowAchievementPopups = | |
| container.getExtra("showAchievementPopups", "false").toBoolean() | |
| container.putExtra("showAchievementPopups", containerData.showAchievementPopups) | |
| if (previousShowAchievementPopups != containerData.showAchievementPopups) { | |
| val steamAppId = extractGameIdFromContainerId(container.id) | |
| val appDirPath = SteamService.getAppDirPath(steamAppId) | |
| MarkerUtils.removeMarker(appDirPath, Marker.STEAM_DLL_REPLACED) | |
| MarkerUtils.removeMarker(appDirPath, Marker.STEAM_COLDCLIENT_USED) | |
| Timber.i( | |
| "showAchievementPopups changed from '$previousShowAchievementPopups' to '${containerData.showAchievementPopups}'. " + | |
| "Cleared Steam markers for container ${container.id}." | |
| ) | |
| } |
🤖 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` at line 473, The
toggle persistence misses invalidation: when
container.putExtra("showAchievementPopups", containerData.showAchievementPopups)
changes, clear the Steam setup markers so the Steam setup reruns; specifically
detect when showAchievementPopups has flipped compared to stored state and
remove/reset the STEAM_DLL_REPLACED and STEAM_COLDCLIENT_USED markers (and any
related configs.overlay.ini short-circuit flags) so the Steam setup path (in the
same ContainerUtils handling) will regenerate the overlay config on next launch.
Comment on lines
+224
to
+225
| // Generate achievements.json | ||
| generateAchievementsFile(rootPath.resolve("steam_settings"), appId) |
Contributor
There was a problem hiding this comment.
Generate achievements.json beside the same DLL you just configured.
ensureSteamSettings() inside the loop writes steam_settings next to each discovered steam_api*.dll, but Line 225 always targets <appDir>/steam_settings. Games whose Steam DLL lives under bin/ or another subdirectory will miss the generated achievements file, and multiple DLL directories get collapsed into one output location. Call the generator with the same path/settings directory you use in the loop.
🤖 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/SteamUtils.kt` around lines 224 - 225,
The generateAchievementsFile call currently uses
rootPath.resolve("steam_settings") which writes achievements.json to the app
root instead of next to each discovered DLL; change the call inside the loop
that runs ensureSteamSettings(...) to pass the per-DLL settings directory (the
same 'path' used for ensureSteamSettings, e.g., path.resolve("steam_settings"))
so generateAchievementsFile writes achievements.json beside each steam_api*.dll;
update the invocation in the loop where ensureSteamSettings and appId are
available (referencing generateAchievementsFile and ensureSteamSettings) to use
the per-DLL path rather than rootPath.
| val language: String = "english", | ||
| val forceDlc: Boolean = false, | ||
| val steamOfflineMode: Boolean = false, | ||
| val showAchievementPopups: Boolean = false, |
Contributor
There was a problem hiding this comment.
Carry showAchievementPopups through manual ContainerData merges.
Line 89 adds the new field, but app/src/main/java/app/gamenative/utils/IntentLaunchManager.kt:145-190 still builds a merged ContainerData(...) without it. Any launch path that uses a temporary override will silently drop the base container’s saved popup setting back to false.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/winlator/container/ContainerData.kt` at line 89, The
merged ContainerData constructed in IntentLaunchManager (the manual merge in
IntentLaunchManager.kt around the ContainerData(...) call) is missing the new
showAchievementPopups field, so any temporary override drops the saved setting;
update that merged ContainerData construction to carry showAchievementPopups
through by selecting the override value when present or falling back to the base
container value (i.e., set showAchievementPopups =
override.showAchievementPopups ?: base.showAchievementPopups or the equivalent
logic used for other fields) so the popup preference is preserved during merges.
| <string name="launch_steam_client_beta">Avvia Client Steam (Beta)</string> | ||
| <string name="steam_offline_mode">Modalità Offline Steam</string> | ||
| <string name="steam_offline_mode_description">Avvia i giochi Steam in modalità offline</string> | ||
| <string name="show_achievement_popups">Mostra popup obiettivi</string> |
Contributor
There was a problem hiding this comment.
Polish the Italian label wording.
Line 537 is understandable, but Mostra popup obiettivi reads a bit awkwardly in Italian. Mostra popup degli obiettivi or Mostra notifiche degli obiettivi is more natural for a settings label.
✏️ Suggested copy tweak
- <string name="show_achievement_popups">Mostra popup obiettivi</string>
+ <string name="show_achievement_popups">Mostra popup degli obiettivi</string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <string name="show_achievement_popups">Mostra popup obiettivi</string> | |
| <string name="show_achievement_popups">Mostra popup degli obiettivi</string> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/res/values-it/strings.xml` at line 537, Update the Italian
string resource for show_achievement_popups to a more natural phrasing; replace
the current value "Mostra popup obiettivi" with either "Mostra popup degli
obiettivi" or "Mostra notifiche degli obiettivi" in the strings.xml entry for
show_achievement_popups so the settings label reads fluently in Italian.
| <string name="launch_steam_client_beta">Pornește Steam Client (Beta)</string> | ||
| <string name="steam_offline_mode">Mod Offline Steam</string> | ||
| <string name="steam_offline_mode_description">Pornește jocurile Steam în mod offline</string> | ||
| <string name="show_achievement_popups">Afișează popup-uri realizări</string> |
Contributor
There was a problem hiding this comment.
Romanian label reads unnaturally.
Afișează popup-uri realizări is awkward Romanian. Consider Afișează popup-uri pentru realizări or Afișează notificări de realizări.
💬 Suggested wording
- <string name="show_achievement_popups">Afișează popup-uri realizări</string>
+ <string name="show_achievement_popups">Afișează popup-uri pentru realizări</string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <string name="show_achievement_popups">Afișează popup-uri realizări</string> | |
| <string name="show_achievement_popups">Afișează popup-uri pentru realizări</string> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/res/values-ro/strings.xml` at line 538, The Romanian string
value for resource show_achievement_popups reads unnaturally; update the value
of the string resource named show_achievement_popups to a more natural phrasing
such as "Afișează popup‑uri pentru realizări" or "Afișează notificări de
realizări" so the label reads correctly in Romanian while keeping the resource
name unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
2754-2754:⚠️ Potential issue | 🟡 MinorUnused variable due to commented-out code.
mySteamIdis assigned but never used since thestoreUserStatscall is commented out. Either remove this line or add a suppression annotation if this is intentional for a future uncomment.- val mySteamId = steamUser.steamID!!🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/SteamService.kt` at line 2754, The assignment to mySteamId (val mySteamId = steamUser.steamID!!) is unused because the subsequent storeUserStats call is commented out; either remove the mySteamId line to eliminate the dead variable or, if you intend to keep it for future use, add a `@Suppress`("UNUSED_VARIABLE") (or more specific suppression) above the declaration to silence the warning—locate the code around steamUser.steamID and storeUserStats to apply the change.
🤖 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 2683-2689: The log claims achievements are being synced but the
call to storeAchievementUnlocks is commented out; either re-enable the sync by
uncommenting and restoring the storeAchievementUnlocks(appId, configDirectory,
unlockedNames) call and its result handling (onSuccess/onFailure) or change the
Timber.i message to accurately reflect that syncing is disabled (e.g., "would
sync to Steam" or "sync disabled"), and add a TODO referencing the decision
and/or an issue ID; look for unlockedNames, appId, storeAchievementUnlocks, and
the Timber.i call to apply the fix.
- Line 164: Remove the unused import of Achievement from SteamService.kt: delete
the line importing app.gamenative.statsgen.Achievement since the class is not
referenced in the file (only appears inside a string literal), ensuring no other
references to Achievement exist in methods like those around the import area so
compilation and imports remain clean.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Line 2754: The assignment to mySteamId (val mySteamId = steamUser.steamID!!)
is unused because the subsequent storeUserStats call is commented out; either
remove the mySteamId line to eliminate the dead variable or, if you intend to
keep it for future use, add a `@Suppress`("UNUSED_VARIABLE") (or more specific
suppression) above the declaration to silence the warning—locate the code around
steamUser.steamID and storeUserStats to apply the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2cf2be0-9283-41b5-808a-49ad3ffbe54e
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/service/SteamService.kt
| import org.json.JSONArray | ||
| import org.json.JSONObject | ||
| import com.winlator.container.ContainerManager | ||
| import app.gamenative.statsgen.Achievement |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Achievement class is used anywhere in SteamService.kt
rg -n '\bAchievement\b' app/src/main/java/app/gamenative/service/SteamService.kt | grep -v 'import'Repository: utkarshdalal/GameNative
Length of output: 184
Remove unused import Achievement at line 164.
The Achievement class is imported but never used in this file. The only occurrence of "Achievement" outside the import statement is in a string literal at line 2138.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/service/SteamService.kt` at line 164, Remove
the unused import of Achievement from SteamService.kt: delete the line importing
app.gamenative.statsgen.Achievement since the class is not referenced in the
file (only appears inside a string literal), ensuring no other references to
Achievement exist in methods like those around the import area so compilation
and imports remain clean.
Comment on lines
+2683
to
+2689
| Timber.i("Found ${unlockedNames.size} earned achievements for appId=$appId, syncing to Steam") | ||
| // val result = storeAchievementUnlocks(appId, configDirectory, unlockedNames) | ||
| // result.onSuccess { | ||
| // Timber.i("Successfully synced achievements to Steam for appId=$appId") | ||
| // }.onFailure { e -> | ||
| // Timber.e(e, "Failed to sync achievements to Steam for appId=$appId") | ||
| // } |
Contributor
There was a problem hiding this comment.
Incomplete implementation: sync call is commented out.
The function logs "Found ${unlockedNames.size} earned achievements for appId=$appId, syncing to Steam" but the actual storeAchievementUnlocks call is commented out. Per the commit message, this was done to make the code compile, but:
- The log message is misleading as no actual sync occurs
- This leaves the achievement sync feature non-functional
Consider either:
- Updating the log to reflect the actual state (e.g., "would sync to Steam" or "sync disabled")
- Adding a TODO comment explaining when this will be enabled
- Tracking this in an issue for follow-up
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/service/SteamService.kt` around lines 2683 -
2689, The log claims achievements are being synced but the call to
storeAchievementUnlocks is commented out; either re-enable the sync by
uncommenting and restoring the storeAchievementUnlocks(appId, configDirectory,
unlockedNames) call and its result handling (onSuccess/onFailure) or change the
Timber.i message to accurately reflect that syncing is disabled (e.g., "would
sync to Steam" or "sync disabled"), and add a TODO referencing the decision
and/or an issue ID; look for unlockedNames, appId, storeAchievementUnlocks, and
the Timber.i call to apply the fix.
Owner
Author
|
Oh no, it doesn't work! |
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.
Summary by cubic
Adds Steam achievements support. We generate per‑game
achievements.json/stats.json, persist a name→stat‑bit map, and optionally show achievement popups. On game exit we read Goldberg unlocks and stage a sync to Steam; upload is disabled and never blocks cloud saves.New Features
steam_settingsat setup usingSteamUserStats.getUserStats,StatsAchievementsGenerator, andVdfParser(UTF‑8 and key order match Goldberg format).achievement_name_to_block.json(name → [stat block, bit]) for future unlock uploads.achievements.json(GSE Saves or userdata fallback) and resolve the game’ssteam_settings; sync calls are scaffolded but commented out.configs.overlay.inito enable the experimental overlay; localized strings included.in.dragonbra:javasteamto1.8.0-12-SNAPSHOT.Bug Fixes
SteamUtilsFileSearchTest.Written for commit f8f24ab. Summary will update on new commits.
Summary by CodeRabbit
New Features
Chores
Tests