feat: steam achievements viewer (updated #1511) - #1695
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSteam achievements are fetched from Steam, mapped into localized display models, loaded for Steam library items, and shown through an overview row and animated detail dialog. Shared ChangesSteam achievements
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BaseAppScreen
participant SteamService
participant SteamUserStats
participant AppScreenContent
participant AchievementsDialog
BaseAppScreen->>SteamService: fetchAchievementsForDisplay(appId)
SteamService->>SteamUserStats: fetch expanded achievements
SteamUserStats-->>SteamService: achievement data
SteamService-->>BaseAppScreen: List<Achievement> or null
BaseAppScreen->>AppScreenContent: pass achievements state
AppScreenContent->>AchievementsDialog: open detailed achievement list
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt (2)
1250-1250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
grayMatrixconstruction inAchievementsRowandAchievementsDialog.Both composables independently build the same
ColorMatrix().apply { setToSaturation(0f) }. Consider hoisting it to a single top-levelprivate val(it's stateless) to avoid the duplication.Also applies to: 1380-1380
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt` at line 1250, Hoist the duplicated grayscale matrix into a single top-level private ColorMatrix value, then update both AchievementsRow and AchievementsDialog to reference it instead of constructing separate instances with remember.
1237-1506: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
contentDescriptionto achievement icon images for accessibility.Landscapist's
ImageOptionssupports acontentDescriptionfield, but none of the threeCoilImagecalls here (row icons, "+N" stack icon, dialog list icon) set it. Screen-reader users get no label for achievement artwork.♿ Suggested addition
CoilImage( imageModel = { iconUrl ?: "" }, imageOptions = ImageOptions( contentScale = ContentScale.Crop, + contentDescription = ach.displayName ?: ach.name, colorFilter = if (ach.isUnlocked) null else ColorFilter.colorMatrix(grayMatrix), ), ... )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt` around lines 1237 - 1506, Add accessibility labels to all three achievement CoilImage calls in AchievementsRow and AchievementsDialog by setting ImageOptions.contentDescription using each achievement’s display name or name, with a suitable fallback for the stacked “+N” preview icon.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 3074-3113: Update fetchAchievementsForDisplay to rethrow
CancellationException before the generic Exception handler, preserving coroutine
cancellation. Bound the Steam operations, including getUserStats(...).await()
and achievement expansion, with an appropriate coroutine timeout such as
withTimeout, while keeping existing error logging and null fallback behavior.
In `@app/src/main/java/app/gamenative/ui/component/InfoCard.kt`:
- Around line 61-66: Remove the unnecessary rememberCoroutineScope and nested
scope.launch from LaunchedEffect(isFocused); call the suspend
bringIntoViewRequester.bringIntoView() directly when isFocused is true so it
remains tied to the effect lifecycle. Remove the now-unused coroutine scope and
launch imports if they are not used elsewhere.
In `@app/src/main/res/values-de/strings.xml`:
- Around line 1571-1577: Translate the German localization value for the
achievements_total string from “Total” to an appropriate German term, preferably
“Gesamt” or “Insgesamt,” while leaving the other achievement strings unchanged.
In `@app/src/main/res/values-ru/strings.xml`:
- Around line 1558-1563: Translate the achievements_total string in the Russian
resources from the English “Total” to its Russian equivalent, matching the
surrounding Cyrillic translations.
In `@app/src/main/res/values-uk/strings.xml`:
- Around line 1626-1631: Translate the achievements_total string in the
Ukrainian achievements resource block from "Total" to the appropriate Ukrainian
equivalent, keeping the existing string name and resource structure unchanged.
In `@app/src/main/res/values-zh-rCN/strings.xml`:
- Around line 1650-1655: Translate the `achievements_total` string in the Steam
achievements resource block from “Total” to the appropriate Simplified Chinese
text, keeping the existing string name unchanged.
In `@app/src/main/res/values-zh-rTW/strings.xml`:
- Around line 1641-1646: Translate the achievements_total string in the
achievements resource block from “Total” to the appropriate Traditional Chinese
wording, matching the existing zh-rTW translations and the corresponding zh-rCN
resource.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt`:
- Line 1250: Hoist the duplicated grayscale matrix into a single top-level
private ColorMatrix value, then update both AchievementsRow and
AchievementsDialog to reference it instead of constructing separate instances
with remember.
- Around line 1237-1506: Add accessibility labels to all three achievement
CoilImage calls in AchievementsRow and AchievementsDialog by setting
ImageOptions.contentDescription using each achievement’s display name or name,
with a suitable fallback for the stacked “+N” preview icon.
🪄 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: dc134ec6-8031-43de-91b7-5cca0bb9a71d
📒 Files selected for processing (21)
app/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/component/InfoCard.ktapp/src/main/java/app/gamenative/ui/data/Achievement.ktapp/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.ktapp/src/main/java/app/gamenative/utils/SteamUtils.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-ja/strings.xmlapp/src/main/res/values-ko/strings.xmlapp/src/main/res/values-pl/strings.xmlapp/src/main/res/values-pt-rBR/strings.xmlapp/src/main/res/values-ro/strings.xmlapp/src/main/res/values-ru/strings.xmlapp/src/main/res/values-uk/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values-zh-rTW/strings.xmlapp/src/main/res/values/strings.xml
d605396 to
3bce1dc
Compare
There was a problem hiding this comment.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
ee695a0 to
85e961b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 3085-3130: Update fetchAchievementsForDisplay after obtaining
userStats to inspect its result status and return null when userStats.result is
not OK. Keep the existing achievement mapping for successful responses unchanged
so only failed Steam fetches trigger the retry path.
🪄 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: 0a567253-c28e-4396-b215-2a8a0f3a0bf4
📒 Files selected for processing (21)
app/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/component/InfoCard.ktapp/src/main/java/app/gamenative/ui/data/Achievement.ktapp/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.ktapp/src/main/java/app/gamenative/utils/SteamUtils.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-ja/strings.xmlapp/src/main/res/values-ko/strings.xmlapp/src/main/res/values-pl/strings.xmlapp/src/main/res/values-pt-rBR/strings.xmlapp/src/main/res/values-ro/strings.xmlapp/src/main/res/values-ru/strings.xmlapp/src/main/res/values-uk/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values-zh-rTW/strings.xmlapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (16)
- app/src/main/res/values-pl/strings.xml
- app/src/main/res/values-ru/strings.xml
- app/src/main/java/app/gamenative/ui/data/Achievement.kt
- app/src/main/res/values-es/strings.xml
- app/src/main/res/values-pt-rBR/strings.xml
- app/src/main/res/values-zh-rTW/strings.xml
- app/src/main/res/values-da/strings.xml
- app/src/main/res/values-ro/strings.xml
- app/src/main/res/values-zh-rCN/strings.xml
- app/src/main/res/values-fr/strings.xml
- app/src/main/res/values-ko/strings.xml
- app/src/main/java/app/gamenative/utils/SteamUtils.kt
- app/src/main/res/values/strings.xml
- app/src/main/res/values-ja/strings.xml
- app/src/main/res/values-de/strings.xml
- app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.kt
4e4e190 to
b22dac2
Compare
There was a problem hiding this comment.
All reported issues were addressed across 19 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
bcaaa66 to
99caba3
Compare
…ements # Conflicts: # app/src/main/res/values-da/strings.xml # app/src/main/res/values-de/strings.xml # app/src/main/res/values-es/strings.xml # app/src/main/res/values-fr/strings.xml # app/src/main/res/values-it/strings.xml # app/src/main/res/values-ja/strings.xml # app/src/main/res/values-ko/strings.xml # app/src/main/res/values-pl/strings.xml # app/src/main/res/values-pt-rBR/strings.xml # app/src/main/res/values-ro/strings.xml # app/src/main/res/values-ru/strings.xml # app/src/main/res/values-uk/strings.xml # app/src/main/res/values-zh-rCN/strings.xml # app/src/main/res/values-zh-rTW/strings.xml # gradle/libs.versions.toml
|
@VinceBT, please use this JavaSteam version: |
|
@utkarshdalal I think we should get this in now that 1.1.1 is out. This is a massively requested feature and helps with user retention for GN. We can then eventually expand it for GOG & Epic as well once there's room to do shim work for those. |
|
@VinceBT Please fix the conflicts and we should be able to get this in :) |
# Conflicts: # app/src/main/java/app/gamenative/service/SteamService.kt # app/src/main/res/values/strings.xml
Description
This is @phobos665's Steam achievements viewer from #1511, brought up to current
masterwith some refinements on top. Most of the credit here belongs to @phobos665 — the viewer, the data plumbing and the schema handling are all theirs; I rebased the work onto currentmasterand polished the UI and localization.Quoting the original PR (#1511):
On top of that, this PR:
x / ycount.x / ycount for stat-linked achievements (e.g. 45 / 100), read from the game's stats.Implementation note: this replaces the local
InfoCardinLibraryAppScreenwith the sharedui/component/InfoCardcomponent (a superset that addsonClick/content), so a few neighbouring cards move to it as well. Progress bars need the achievement progress fields added in JavaSteam, sojavasteamis bumped to1.8.0.1-23-SNAPSHOT.Recording
screen_recording_compressed.mp4
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
Adds a Steam achievements viewer to Game Details with a compact card and a fullscreen list, including progress bars, hidden-achievement reveal, and a detail dialog. Text is localized with English fallbacks and icons load from the Steam CDN.
New Features
Refactors
ui/data/AchievementandSteamService.fetchAchievementsForDisplay()with per-string English fallbacks and a 15s timeout; the screen retries failed fetches up to 3 times with a short delay.SteamUtils.steamLanguageForAppLocale()locale-injectable and added unit tests (Spanish split, Chinese scripts, Korean, Portuguese); addedSteamUtils.getBaseAchievementIconUrl().ui/component/InfoCard; grouped download flags intoui/data/DownloadDisplayDetailsto avoid an ART VerifyError.javasteamto1.8.0.1-24-SNAPSHOTto surface achievement progress.Written for commit 0288ff6. Summary will update on new commits.
Summary by CodeRabbit