feat(retroachievements): flag which ROM version RA actually supports - #4134
feat(retroachievements): flag which ROM version RA actually supports#4134sdornan wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
ra_hash_match=False is currently likely to be dropped during scan metadata application due to truthy-only copying, preventing the intended persisted "definite no" state from being stored.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds a per-ROM-version RetroAchievements support signal (ra_hash_match) so the UI and backend "verified" logic can accurately reflect whether RetroAchievements will unlock for a specific dump, not just for the game-level ra_id.
Changes:
- Persist
roms.ra_hash_match(nullable tri-state) and expose it through ROM and sibling ROM schemas. - Update backend verified filtering and frontend verification utilities to prefer RetroAchievements' own hash verdict over Hasheous when available.
- Surface support in the v2 VersionSwitcher and add tests across frontend and backend.
File summaries
| File | Description |
|---|---|
| frontend/src/v2/utils/romVerification.ts | Updates verification logic to incorporate ra_hash_match precedence over Hasheous ra_match. |
| frontend/src/v2/utils/romVerification.test.ts | Adds coverage for RetroAchievements precedence and ra_hash_match behavior. |
| frontend/src/v2/components/GameDetails/VersionSwitcher.vue | Shows a RetroAchievements mark per sibling version based on ra_hash_match. |
| frontend/src/v2/components/GameDetails/VersionSwitcher.test.ts | New tests validating per-version RetroAchievements marking and tooltip binding. |
| frontend/src/v2/components/GameDetails/RelatedGameCard.vue | Extends synthetic SimpleRom stub with ra_hash_match. |
| frontend/src/v2/components/GameDetails/MetadataTab.vue | Drives verification chips via db.matches (including RetroAchievements precedence). |
| frontend/src/locales/en_US/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/en_GB/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/bg_BG/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/cs_CZ/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/de_DE/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/es_ES/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/fr_FR/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/hu_HU/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/it_IT/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/ja_JP/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/ko_KR/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/pl_PL/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/pt_BR/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/ro_RO/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/ru_RU/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/tr_TR/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/zh_CN/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/locales/zh_TW/rom.json | Adds rom.retroachievements-supported string. |
| frontend/src/generated/models/SimpleRomSchema.ts | Adds generated ra_hash_match field to SimpleRom schema. |
| frontend/src/generated/models/SiblingRomSchema.ts | Adds generated ra_hash_match field to sibling schema. |
| frontend/src/generated/models/DetailedRomSchema.ts | Adds generated ra_hash_match field to detailed schema. |
| backend/models/rom.py | Adds Rom.ra_hash_match column mapping and documentation. |
| backend/alembic/versions/0108_roms_ra_hash_match.py | Migration creating roms.ra_hash_match nullable boolean column. |
| backend/handler/metadata/ra_handler.py | Adds hash index memoization and hash_is_known helper for per-file RA support. |
| backend/handler/scan_handler.py | Populates ra_hash_match into RA handler result during scans. |
| backend/handler/database/roms_handler.py | Ensures ra_hash_match is loaded for siblings and updates verified filter precedence. |
| backend/endpoints/responses/rom.py | Exposes ra_hash_match on ROM and sibling ROM response schemas. |
| backend/tests/handler/metadata/test_ra_handler.py | Adds unit tests for hash_is_known and hash index caching behavior. |
| backend/tests/handler/database/test_roms_verified_filter.py | Adds tests to lock verified-filter precedence between RA and Hasheous, including Postgres SQL branch. |
| backend/tests/endpoints/roms/test_rom.py | Ensures endpoints include ra_hash_match for sibling ROMs. |
Review details
Files not reviewed (3)
- frontend/src/generated/models/DetailedRomSchema.ts: Generated file
- frontend/src/generated/models/SiblingRomSchema.ts: Generated file
- frontend/src/generated/models/SimpleRomSchema.ts: Generated file
Suppressed comments (1)
frontend/src/v2/utils/romVerification.ts:28
- Repo guidance forbids em dashes in comments. Replace the em dash here with a comma or rewrite into two sentences.
- Files reviewed: 33/36 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Good catch, and it was worse than "likely dropped" - it made the feature's headline behaviour unreachable. The metadata priority loop in for key, field_value in handler_data.items():
if field_value:
rom_attrs[key] = field_valueSo Fixed in bb77311 by applying the tri-state explicitly after that loop. It also can't be gated on Added a Also took the suppressed em-dash comment: fixed in the same commit. For what it's worth, that was the only em-dash in the added lines of this PR (checked with |
There was a problem hiding this comment.
🟡 Changes recommended
A transient failure reading the RA hash index can currently cause scans to overwrite an existing ra_hash_match value with NULL, unintentionally erasing known support state.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (3)
- frontend/src/generated/models/DetailedRomSchema.ts: Generated file
- frontend/src/generated/models/SiblingRomSchema.ts: Generated file
- frontend/src/generated/models/SimpleRomSchema.ts: Generated file
Suppressed comments (1)
backend/handler/metadata/ra_handler.py:262
- When the hash list can't be read,
hash_is_knownreturnsNone, and the scan code persists that, which can unintentionally clear a previously knownra_hash_matchvalue (turning a known True or False into NULL) due to a transient IO or parse error. Consider keeping the existing DB value in this error path so scans do not erase known RA support state on temporary failures.
"Couldn't read the RetroAchievements hash list, "
"leaving hash support unknown: %s",
exc,
)
return None
- Files reviewed: 35/38 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The tri-state RA support signal is implemented end-to-end (scan persistence, API exposure, filtering, and v2 UI) with targeted tests covering precedence and regression cases.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (3)
- frontend/src/generated/models/DetailedRomSchema.ts: Generated file
- frontend/src/generated/models/SiblingRomSchema.ts: Generated file
- frontend/src/generated/models/SimpleRomSchema.ts: Generated file
- Files reviewed: 35/38 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Achievements only unlock for the exact dump RetroAchievements hashed, but nothing in RomM said which of a game's versions that was. `ra_id` cannot answer it: on the Hasheous path it comes from the matched game's metadata list, so every sibling of a title resolves to the same id. Record the per-file fact instead. `hash_is_known` asks RetroAchievements' own hash list, and the scan now asks it for every ROM on an RA platform rather than skipping the lookup whenever Hasheous already supplied an id. The result persists as `roms.ra_hash_match`, nullable so "never checked" stays distinct from "checked, absent". The version switcher marks the versions RA knows, and the Verification section's RetroAchievements tag reads the same field, falling back to Hasheous' `ra_match` only when RA was never asked. A definite no from RA outranks a Hasheous hit, since deferring there would promise achievements for a file RA has never seen. Also memoise the hash list per platform. It was re-read and re-parsed once per ROM, and it is multiple megabytes on large systems. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The metadata priority loop in `scan_rom` copies truthy values only, so `ra_hash_match=False` was dropped and the column stayed NULL. NULL means never-checked and falls back to Hasheous, which made the "RetroAchievements has never seen this dump" branch unreachable in practice: the precedence logic was right, but the data never arrived. Apply the tri-state explicitly, after that loop. It also can't be gated on `ra_id` the way the loop is, since a ROM can carry the game's id from Hasheous while RA has never seen the file. Covered by a scan_rom test over all three states, which fails on the True and False cases without this fix. Also drop an em-dash from a comment, per the repo's style rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The parent-attach pattern renders a `display: none` anchor span, not a comment node. RTooltip's own comment said comment node, which is what the VersionSwitcher comment repeated. The layout conclusion is unchanged; only the mechanism was described wrong. Also drop the em-dash from romVerification's opening comment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The same fact, that `ra_id` is the game and every version shares it, was spelled out at six sites. It now lives once on the column it describes, with a line at each use site. Also fixes a stale "comment node" reference in the VersionSwitcher test that the previous commit missed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… check `hash_is_known` returns None when there is nothing to check against: no RA hash computed for this pass (a scan that doesn't rehash, or SKIP_HASH_CALCULATION), or a hash list that couldn't be read. Persisting that turned a known True or False back into NULL, and NULL falls back to Hasheous, so a single unlucky rescan discarded the answer. Only write an answer that was actually reached. Leaving the key out of `rom_attrs` is what preserves the stored value: `add_rom` merges a detached `Rom(**rom_attrs)`, and merge skips attributes the instance never set. The test asserts through that merge rather than on `scan_rom`'s return value, which can't distinguish an absent key from an explicit None. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… require achievements Two ways the mark could lie. RetroAchievements adds hashes continuously, but the local hash list is only refetched every REFRESH_RETROACHIEVEMENTS_CACHE_DAYS, which defaults to 30. A miss against a month-old list was being recorded as a definite no, and that no outranks Hasheous. Refetch before reporting one: at most a single request per platform per scan, since the fresh list is memoised and written to disk. A failed refetch leaves the answer unknown rather than negative. The list also covers games RetroAchievements has no achievements for, so a hash match alone promised unlocks that could never happen. Gate the mark on the game having achievements, read off the metadata the details page already loads. A per-sibling count would mean hydrating `ra_metadata` for every sibling, which the sibling load_only lists exist to avoid. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
388ce16 to
fa3935c
Compare
Sorry for opening so many PRs in such quick succession. Just wanting to fix and improve things as I import my library. I intend for this one to be the last one for the time being! Also, a future enhancement I am considering is showing the RA badge or a trophy on the game list. Let me know your thoughts on that!
Description
Explain the changes or enhancements you are proposing with this pull request.
RetroAchievements only unlocks achievements for the exact dump it has hashed, but nothing in RomM surfaced which of a game's versions that was. Picking between
Star Fox 64 (USA)andStar Fox 64 (USA) (Rev 1)meant guessing, or opening each one in turn.ra_idcan't answer this. On the Hasheous path it comes from the matched game's metadata list (hasheous_handler.py), so it identifies the game, and every sibling of a title resolves to the same id. A per-version marker driven by it would light up on every row.So this records the per-file fact instead:
RAHandler.hash_is_knownasks RetroAchievements' own hash list (thera_hashes_v2.jsonindex built fromget_game_list(..., include_hashes=True)), never another provider.scan_handlernow asks it for every ROM on an RA platform. Previously a Hasheous-suppliedra_idshort-circuited the RA lookup entirely, which is how the per-file answer got lost.roms.ra_hash_match(migration0108), nullable so "never checked" stays distinct from "checked, and RA doesn't have it".Two surfaces read it:
ra_matchonly when RA was never asked. A definite no from RA outranks a Hasheous hit, since deferring there would promise achievements for a file RA has never seen._filter_by_verifiedencodes the same precedence so the library filter can't drift from the badges.Also included: the platform hash list is now memoised per platform with a short TTL. It was previously re-read and re-parsed from disk once per ROM during a scan, and it runs to several megabytes on large systems.
Note that
ra_hash_matchstarts asNULL, so existing libraries show nothing new until those ROMs are rescanned or metadata-refreshed.AI assistance disclosure
This PR was written with AI assistance (Claude Code). The investigation, implementation, migration, and tests were produced in a session with an agent, and reviewed by me before opening. Per
CONTRIBUTING.md, disclosing this in full: the agent wrote the code and tests; I directed the approach, caught thatra_idwas game-level rather than per-file, and chose to persist the flag at scan time rather than compute it per request.Checklist
Please check all that apply.
Verification run locally:
vue-tscclean,npm run buildclean, 279 v2 vitest tests, 1902 backend tests acrosstests/handlerandtests/endpoints, both i18n scripts, and ruff/black/prettier/eslint on the touched files. Not yet verified in a browser against a live library, so the screenshot below is a faithful mock rather than a capture (see below).Screenshots (if applicable)
Composed mock, not a live capture: built from the real
ra.pngasset and the generated v2 theme tokens, with the CSS copied verbatim fromRMenu,RMenuItem, andVersionSwitcher. The RA-marked row is the base USA dump, which is the version RetroAchievements actually hashed for Star Fox 64. The light pane also shows the mark alongside the existing default-version bookmark.The mark is a bare 14px logo rather than the gallery's tiled provider chip: the menu panel is
width: max-contentwith no cap, so anything wider grows the whole dropdown. At 14px it costs exactly what the bookmark already in that slot costs.