Fix/steam achievement timestamp fix#1254
Conversation
📝 WalkthroughWalkthroughThe generator was refactored into parse → apply earned state → write steps and now accepts server-provided Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as SteamService
participant Gen as StatsAchievementsGenerator
participant FS as FileSystem/GSEDirs
participant Cache as LocalCache
Client->>Gen: generateStatsAchievements(schema, configDirectory, achievementBlocks)
Gen->>Gen: parseSchema(schema) → ProcessingResult
Gen->>Gen: applyEarnedState(result, achievementBlocks)
Gen->>FS: writeConfigFiles(result, configDirectory)
Gen-->>Client: ProcessingResult (achievements)
Client->>Cache: update cachedAchievements / cachedAchievementsAppId
Client->>Gen: writeGseAchievementFiles(result, getGseSaveDirs(instance, appId))
Gen->>FS: merge/upgrade Goldberg/GSE `achievements.json` in each save dir
FS-->>Gen: write confirmations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/statsgen/StatsAchievementsGenerator.kt (1)
228-230: Remove or lower level of per-achievement INFO log.This logs one INFO line per unlocked achievement every time
writeConfigFilesruns — for games with hundreds of achievements this is quite noisy in logcat and looks like leftover instrumentation from debugging this PR. ConsiderTimber.d(...)(or removing it) and dropping the custom"ach"tag so it flows through the standard Timber pipeline.♻️ Suggested change
- if(ach.unlocked == true){ - Timber.tag("ach").i("Achievement ${ach.name} is unlocked: ${ach.unlocked}, unlockTimestamp: ${ach.unlockTimestamp}, formattedUnlockTime: ${ach.formattedUnlockTime}") - } - + if (ach.unlocked == true) { + Timber.d("Achievement ${ach.name} unlocked at ${ach.unlockTimestamp}") + } + ach.unlocked?.let { outputAch["earned"] = it }🤖 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 228 - 230, The per-achievement info log in StatsAchievementsGenerator (inside writeConfigFiles) uses Timber.tag("ach").i and produces noisy INFO output; change this to a debug-level log (Timber.d) and remove the custom "ach" tag so it goes through the normal Timber pipeline (i.e., replace Timber.tag("ach").i(...) with a Timber.d(...) call that logs the same compact info, or remove the log entirely if not needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/app/gamenative/statsgen/StatsAchievementsGenerator.kt`:
- Around line 228-230: The per-achievement info log in
StatsAchievementsGenerator (inside writeConfigFiles) uses Timber.tag("ach").i
and produces noisy INFO output; change this to a debug-level log (Timber.d) and
remove the custom "ach" tag so it goes through the normal Timber pipeline (i.e.,
replace Timber.tag("ach").i(...) with a Timber.d(...) call that logs the same
compact info, or remove the log entirely if not needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d89e666-ebd6-4964-a349-90f20c3fa702
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/statsgen/StatsAchievementsGenerator.kt
…the GSE save location
|
All the code is now there. Will put in some testing, might take a bit due to there not being any tests in this file yet. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/statsgen/StatsAchievementsGenerator.kt (2)
389-418: GSE merge logic is correct; a few small polish suggestions.The "only write when not already earned" guard correctly prevents a server wipe from clearing a locally-earned achievement, which is the main goal of the fix. A few optional cleanups:
- Fully-qualified
org.json.JSONObjectis used five times here while the rest of this file avoids it. Consider importing it for readability.- Line 392: if
mkdirs()fails (e.g., path not writable under the Wine prefix before it's initialized), the subsequentachFile.writeTextwill throw and be caught, but you'll lose the earned state for that directory silently. ATimber.wwhen!gseDir.exists() && !gseDir.mkdirs()would make this diagnosable.- Line 408:
ach.unlockTimestamp?.toLong() ?: 0Lwill writeearned_time: 0for every non-earned achievement on a first run with no server data. That matches the PR objective (create the file so Goldberg doesn't re-trigger), but downstream parsers that dojson.optLong("earned_time", -1)would need to treat0as "never". Worth a brief comment near line 408 documenting the sentinel.Import-level cleanup
import `in`.dragonbra.javasteam.steam.handlers.steamuserstats.AchievementBlocks import java.io.File import timber.log.Timber +import org.json.JSONObject- val existing = if (achFile.exists()) { - try { - org.json.JSONObject(achFile.readText(Charsets.UTF_8)) - } catch (e: Exception) { - Timber.w(e, "Failed to parse existing GSE achievements.json in ${gseDir.absolutePath}, starting fresh") - org.json.JSONObject() - } - } else { - org.json.JSONObject() - } + val existing = if (achFile.exists()) { + try { + JSONObject(achFile.readText(Charsets.UTF_8)) + } catch (e: Exception) { + Timber.w(e, "Failed to parse existing GSE achievements.json in ${gseDir.absolutePath}, starting fresh") + JSONObject() + } + } else { + JSONObject() + } for (ach in result.achievements) { - val entry = existing.optJSONObject(ach.name) ?: org.json.JSONObject() + val entry = existing.optJSONObject(ach.name) ?: JSONObject()🤖 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 389 - 418, In writeGseAchievementFiles: replace repeated fully-qualified org.json.JSONObject with an import of JSONObject for readability; after attempting to create the directory (gseDir.mkdirs()), add a Timber.w when the directory still doesn't exist (!gseDir.exists() && !gseDir.mkdirs()) so failures to create the save dir are diagnosable; and add a brief inline comment next to the earned_time assignment (the ach.unlockTimestamp?.toLong() ?: 0L expression) documenting that 0 is used as the "never earned / sentinel" value for downstream parsers that call optLong(..., -1).
171-193: Out-of-rangebitIndexcould silently mask schema mismatches; consider adding a warning log.The
applyEarnedStatelogic correctly handles edge cases:
Duplicate achievementId entries (line 178): The
associatefunction silently drops duplicates, but the Steam API practically guarantees uniqueachievementIdper block list, so deduplication is unnecessary.Out-of-range
bitIndex(line 185): ThegetOrElse(bitIndex) { 0 }safely returns 0 (treated as locked/unearned). However, this silently masks potential schema or blocks desync. Adding aTimber.w()whenbitIndex >= unlockTimes.sizewould improve visibility without changing behavior.🤖 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 171 - 193, In applyEarnedState, add a warning when the computed bitIndex is outside the unlockTimes list to avoid silently masking schema/desync issues: when resolving val (blockId, bitIndex) from result.nameToBlockBit and fetching unlockTimes from blockUnlockTimes, check if bitIndex >= unlockTimes.size and call Timber.w(...) with context (achievement name, blockId, bitIndex, unlockTimes.size) before falling back to treating the bit as locked; keep current behavior (using 0/unlocked=false) but emit the warning to aid debugging.
🤖 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/statsgen/StatsAchievementsGenerator.kt`:
- Around line 228-230: The info-level log inside generateStatsAchievements (the
Timber.tag("ach").i(...) call for ach where ach.unlocked == true) is too noisy
and uses an inconsistent tag; change it to a debug-level log and use the
existing "GenerateAchievements" tag (or remove it entirely if not needed).
Locate the block that checks ach.unlocked in
StatsAchievementsGenerator.generateStatsAchievements and replace the
Timber.tag("ach").i(...) invocation with
Timber.tag("GenerateAchievements").d(...) (or delete the logging line) so
unlocked achievements no longer spam INFO-level logs.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/statsgen/StatsAchievementsGenerator.kt`:
- Around line 389-418: In writeGseAchievementFiles: replace repeated
fully-qualified org.json.JSONObject with an import of JSONObject for
readability; after attempting to create the directory (gseDir.mkdirs()), add a
Timber.w when the directory still doesn't exist (!gseDir.exists() &&
!gseDir.mkdirs()) so failures to create the save dir are diagnosable; and add a
brief inline comment next to the earned_time assignment (the
ach.unlockTimestamp?.toLong() ?: 0L expression) documenting that 0 is used as
the "never earned / sentinel" value for downstream parsers that call
optLong(..., -1).
- Around line 171-193: In applyEarnedState, add a warning when the computed
bitIndex is outside the unlockTimes list to avoid silently masking schema/desync
issues: when resolving val (blockId, bitIndex) from result.nameToBlockBit and
fetching unlockTimes from blockUnlockTimes, check if bitIndex >=
unlockTimes.size and call Timber.w(...) with context (achievement name, blockId,
bitIndex, unlockTimes.size) before falling back to treating the bit as locked;
keep current behavior (using 0/unlocked=false) but emit the warning to aid
debugging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66167545-448b-4503-ae35-9960565963bd
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/statsgen/StatsAchievementsGenerator.kt
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
2853-2862: Consider guarding onuserStats.result == EResult.OKbefore generating/writing.
storeAchievementUnlocksexplicitly checksif (userStats.result != EResult.OK)and throws (line 2984–2986), butgenerateAchievementsdoes not. IfgetUserStatsfails (offline, transient error), the schema would be invalid/empty, causingparseSchemato return empty results withnameToBlockBit.isEmpty() = true. This preventsachievement_name_to_block.jsonfrom being written (it's only written ifnameToBlockBit.isNotEmpty()at line 2866), yetwriteGseAchievementFilesstill executes unconditionally, materializingachievements.jsonfiles in GSE directories with all achievements locked (earned=false).On the next
syncAchievementsFromGoldbergcall,findSteamSettingsDirreturns null (since the mapping file doesn't exist) and the sync is silently skipped, leaving achievements in a locked state despite the PR's intent to populate earned state from the server. The existing-entry preservation inwriteGseAchievementFilesprevents data loss, but a failed stats fetch still produces inconsistent state where the achievements file exists but the mapping doesn't.Suggested guard
val userStats = instance?._steamUserStats!!.getUserStats(appId, steamUser.steamID!!).await() + if (userStats.result != EResult.OK) { + Timber.w("generateAchievements: getUserStats returned ${userStats.result} for appId=$appId, skipping") + return + } val schemaArray = userStats.schema.toByteArray()🤖 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 2853 - 2862, The code calls getUserStats(...).await() and immediately uses its schema and passes the result to StatsAchievementsGenerator.generateStatsAchievements and writeGseAchievementFiles; add a guard that checks userStats.result == EResult.OK (the same check used in storeAchievementUnlocks) and if not OK, skip generation/writing (or throw/return) to avoid creating achievements.json without the mapping; specifically, before calling generator.generateStatsAchievements and before assigning cachedAchievements/cachedAchievementsAppId and generator.writeGseAchievementFiles, validate userStats.result, and only proceed when it equals EResult.OK.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 2853-2862: The code calls getUserStats(...).await() and
immediately uses its schema and passes the result to
StatsAchievementsGenerator.generateStatsAchievements and
writeGseAchievementFiles; add a guard that checks userStats.result == EResult.OK
(the same check used in storeAchievementUnlocks) and if not OK, skip
generation/writing (or throw/return) to avoid creating achievements.json without
the mapping; specifically, before calling generator.generateStatsAchievements
and before assigning cachedAchievements/cachedAchievementsAppId and
generator.writeGseAchievementFiles, validate userStats.result, and only proceed
when it equals EResult.OK.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd5dc0da-e790-437b-b308-98b3af7ab33d
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/service/SteamService.kt
| } | ||
|
|
||
| fun generateStatsAchievements(schema: ByteArray, configDirectory: String): ProcessingResult { | ||
| fun parseSchema(schema: ByteArray): ProcessingResult { |
There was a problem hiding this comment.
Refactored for easier testing
| cachedAchievementsAppId = appId | ||
|
|
||
| // Create AchievementFile for each GSE save directory so that they can be tracked by goldberg | ||
| generator.writeGseAchievementFiles(result, getGseSaveDirs(instance!!, appId)) |
There was a problem hiding this comment.
Creates achievement files in the GSE save directories so GB can take it up.
There was a problem hiding this comment.
we were already doing this though. Please check.
The default gse saves folder has changed to C:\Program Files (x86)\Steam\userdata. Please double check that we are writing there and that it works.
There was a problem hiding this comment.
Will double-check, before my changes I believe this file was absent. But will get back to you and confirm the diff
| ach.unlocked?.let { outputAch["earned"] = it } | ||
| ach.unlockTimestamp?.let { outputAch["earned_time"] = it } |
There was a problem hiding this comment.
Need to use earned and earned_time as that's what's required by gb.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/statsgen/StatsAchievementsGenerator.kt (1)
385-414: Minor defensive polish inwriteGseAchievementFiles.The merge logic (don't downgrade an already-earned entry) is correct. A couple of small, optional improvements:
- Guard against an empty
ach.name—parseSchemadefaults the name to""whenach["name"]is null, which would create a bogus""key in the written JSON.org.json.JSONObjectis used fully qualified three times; a top-of-fileimport org.json.JSONObjectwould read more cleanly.- Existing keys not present in the current schema are preserved as-is (neither pruned nor re-written). This is likely intentional to survive schema churn, but worth confirming — if the intent is to keep the file canonical to the current schema, you'd want to drop unknown keys.
♻️ Proposed tweak
-import `in`.dragonbra.javasteam.steam.handlers.steamuserstats.AchievementBlocks import java.io.File import timber.log.Timber +import org.json.JSONObject +import `in`.dragonbra.javasteam.steam.handlers.steamuserstats.AchievementBlocks @@ fun writeGseAchievementFiles(result: ProcessingResult, gseSaveDirs: List<File>) { for (gseDir in gseSaveDirs) { try { if (!gseDir.exists()) gseDir.mkdirs() val achFile = File(gseDir, "achievements.json") val existing = if (achFile.exists()) { try { - org.json.JSONObject(achFile.readText(Charsets.UTF_8)) + JSONObject(achFile.readText(Charsets.UTF_8)) } catch (e: Exception) { Timber.w(e, "Failed to parse existing GSE achievements.json in ${gseDir.absolutePath}, starting fresh") - org.json.JSONObject() + JSONObject() } } else { - org.json.JSONObject() + JSONObject() } for (ach in result.achievements) { - val entry = existing.optJSONObject(ach.name) ?: org.json.JSONObject() + if (ach.name.isEmpty()) continue + val entry = existing.optJSONObject(ach.name) ?: JSONObject() if (!entry.optBoolean("earned", false)) { entry.put("earned", ach.unlocked == true) entry.put("earned_time", ach.unlockTimestamp?.toLong() ?: 0L) } existing.put(ach.name, entry) }🤖 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 385 - 414, The writeGseAchievementFiles method may create a bogus empty-string key when achievement.name is empty and repeats fully-qualified org.json.JSONObject; fix by skipping any achievement with a blank name (guarding ach.name.isNullOrBlank() before using it) and add a file-level import for org.json.JSONObject to replace fully-qualified references (replace org.json.JSONObject() with JSONObject()). Also decide whether to prune unknown existing keys: if you want the file canonical to current schema, filter existing keys to only include names present in result.achievements before writing; otherwise leave preservation behavior as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/app/gamenative/statsgen/StatsAchievementsGenerator.kt`:
- Around line 385-414: The writeGseAchievementFiles method may create a bogus
empty-string key when achievement.name is empty and repeats fully-qualified
org.json.JSONObject; fix by skipping any achievement with a blank name (guarding
ach.name.isNullOrBlank() before using it) and add a file-level import for
org.json.JSONObject to replace fully-qualified references (replace
org.json.JSONObject() with JSONObject()). Also decide whether to prune unknown
existing keys: if you want the file canonical to current schema, filter existing
keys to only include names present in result.achievements before writing;
otherwise leave preservation behavior as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d6c837a-6d0c-4900-a063-76dd402ba0fe
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/statsgen/StatsAchievementsGenerator.kt
|
How about a much simpler concept:
userStats.stats.forEach { stat ->
stat.statId
stat.statValue
}
For example, Vampire Survivors, the {
"Banish10Items": {
"earned": false,
"earned_time": 0
},
}Based on the |
So my worry about the timestamp was due to potentially getting invalidated in future. But very happy to see if we can go down a much simpler route. I believe that my JavaSteam implementation is bugged, and I'd like to ideally move the complexity back into that rather than us having to do complex processing with in GN, when it's clearly the domain of JavaSteam to give back rich achievements & stats correctly. (I tried my getExpandedAchievements function, but it clearly has a bug going on which blocks us from just using that). Thoughts? |
|
Is this still being worked on? |
Going to keep working on this, I think the approach can be made simpler based on some of JT's feedback here. Will get back to you on this one in the next couple of days. |
3 participants
Description
This PR fixes 3 bugs in the achievement generator code.
It was not supplying the achievements.json with the correct fields. It required "earned" and "earned_time" for the unlock and unlock time respectively.
We were not getting timestamps due to the fact that the schema for achievements & stats did not provide this. To fix this, I needed to create a new function called "applyEarnedState" which applies the earned state based on the bitIndex match with the raw achievementBlocks from userStatsCallback.
Goldberg requires us to put an achievements.json in the GSE save locations and populate them based on the above 2 fixes.
This now fixes the issue where games would re-trigger achievements on launch, such as Brotato.
Happy for feedback. The big one I'm sure could do with some critique is the file output code.
Out of Scope:
Recording
Achievement Fix Video:
https://github.com/user-attachments/assets/dccfc456-ddbf-421e-960b-e26a31c54f06
Type of Change
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by cubic
Fixes Steam achievement earned state and per‑achievement timestamps, and writes Goldberg-compatible achievements files so games don’t re-trigger achievements.
AchievementBlocksviaapplyEarnedState, avoiding the expanded‑achievements bug.SteamServicepassesuserStats.achievementBlocksinto generation and writes a Goldbergachievements.jsonto each GSE save dir; existing files are merged so earned progress isn’t downgraded.Written for commit 7346f83. Summary will update on new commits.
Summary by CodeRabbit