feat(gallery): pick a game's default version by region priority - #4260
Conversation
The group_by_meta_id dedup window ordered each sibling group by is_main_sibling then fs_name_no_ext, so absent a manual override the gallery entry went to whichever filename sorted first: "(Demo)" beat "(Europe)" beat "(USA)". Two ordering terms now sit between those: a full release outranks a pre-release, then the configured scan.priority.region order decides. Release status comes first because a demo is not a substitute for the game, so a Japanese release wins over a USA demo. scan.priority.region already existed but only steered metadata and artwork selection, so its description is reworded across all locales. Regions live in a JSON column that no covering index can carry, and 0107 measured what one uncovered reference costs this window: the plan drops from `type=index, Using index` to `type=ALL, key=NULL`, four times per gallery page load. 0108 adds a STORED generated column over regions[0] and covers it in idx_roms_sibling_cover instead, so the ranking reads a scalar. The engine keeps it in sync and ALTER TABLE populates existing rows, so no scan hook and no backfill are needed. Pre-release detection matches the filename rather than the parsed `tags` column: tags keep the dumper's casing, and fs_name_no_ext is already in the index. Reordering the priority list changes which rom represents a group, so the cached gallery sidecars are dropped when the setting changes. Closes rommapp#1528 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Greptile SummaryThe PR makes grouped gallery entries prefer full releases and then the configured region order, backed by a generated primary-region column and an expanded covering index.
Confidence Score: 4/5The region-priority feature needs correction before merging because valid Reverse expansion emits Holland and Netherlands as separate positions, and the query directly converts those positions into unequal ranks despite both names mapping to the same configured shortcode. Files Needing Attention: backend/handler/filesystem/base_handler.py, backend/handler/database/roms_handler.py Important Files Changed
Prompt To Fix All With AI### Issue 1
backend/handler/filesystem/base_handler.py:170-173
**Shortcode aliases receive unequal ranks**
When `nl` is configured and a sibling group contains Holland- and Netherlands-tagged ROMs, this expansion places the two names at different list positions, and `_region_rank()` converts those positions into unequal priorities. The Holland ROM therefore always outranks the Netherlands ROM even though both map to the same configured shortcode, causing the gallery to select the wrong default sibling instead of applying the filename tiebreaker.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(gallery): pick a game's default ver..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
This PR improves grouped gallery behavior when a game has multiple ROM variants by selecting the default (primary) ROM using release-status and configured region priority, rather than filename sort order alone. It also adds a generated scalar region column so the grouping window query can keep using the sibling covering index.
Changes:
- Update the
group_by_meta_iddedup window ordering to rank full releases ahead of pre-releases, then applyscan.priority.regionordering before a stable filename tiebreak. - Add
roms.generated_primary_region(STORED generated fromregions[0]) and include it inidx_roms_sibling_coverto keep the window query index-covered. - Reword the
scan.priority.regionsetting description across all locales, add tests for the new primary-ROM selection behavior, and invalidate cached gallery filter sidecars when region priority changes.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/locales/bg_BG/settings.json | Rewords region priority description to reflect new behavior. |
| frontend/src/locales/cs_CZ/settings.json | Rewords region priority description to reflect new behavior. |
| frontend/src/locales/de_DE/settings.json | Rewords region priority description to reflect new behavior. |
| frontend/src/locales/en_GB/settings.json | Rewords region priority description to reflect new behavior. |
| frontend/src/locales/en_US/settings.json | Source locale rewording for region priority description. |
| frontend/src/locales/es_ES/settings.json | Rewords region priority description to reflect new behavior. |
| frontend/src/locales/fr_FR/settings.json | Rewords region priority description to reflect new behavior. |
| frontend/src/locales/hu_HU/settings.json | Rewords region priority description to reflect new behavior. |
| frontend/src/locales/it_IT/settings.json | Rewords region priority description to reflect new behavior. |
| frontend/src/locales/ja_JP/settings.json | Rewords region priority description to reflect new behavior. |
| frontend/src/locales/ko_KR/settings.json | Rewords region priority description to reflect new behavior. |
| frontend/src/locales/pl_PL/settings.json | Rewords region priority description to reflect new behavior. |
| frontend/src/locales/pt_BR/settings.json | Rewords region priority description to reflect new behavior. |
| frontend/src/locales/ro_RO/settings.json | Rewords region priority description to reflect new behavior. |
| frontend/src/locales/ru_RU/settings.json | Rewords region priority description to reflect new behavior. |
| frontend/src/locales/tr_TR/settings.json | Rewords region priority description to reflect new behavior. |
| frontend/src/locales/zh_CN/settings.json | Rewords region priority description to reflect new behavior. |
| frontend/src/locales/zh_TW/settings.json | Rewords region priority description to reflect new behavior. |
| backend/models/base.py | Adds PRERELEASE_FILENAME_TAGS constants used for prerelease detection in ordering. |
| backend/handler/filesystem/base_handler.py | Adds shortcode-to-region-name expansion helper for region priority ordering. |
| backend/models/rom.py | Adds ORM field for generated_primary_region and includes it in the sibling covering index definition. |
| backend/handler/database/roms_handler.py | Implements prerelease and region ranking terms in the grouped dedup window ordering. |
| backend/endpoints/configs.py | Invalidates cached gallery filter sidecars when scan region priority is updated. |
| backend/alembic/versions/0108_roms_primary_region.py | Adds generated column + rebuilds idx_roms_sibling_cover to cover it. |
| backend/tests/handler/test_db_handler.py | Adds tests covering primary ROM choice behavior (prerelease vs release, region priority, etc.). |
| backend/tests/handler/database/test_roms_group_by_index.py | Strengthens index-coverage guard to include referenced columns inside expressions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
region_names_for_priority expanded the configured shortcodes into a flat
list of region names and _region_rank turned each name's index into its
rank. Two names claiming one shortcode ("nl" for both Holland and
Netherlands) therefore ranked unequally, so a Holland-tagged rom always
beat a Netherlands-tagged one instead of both falling through to the
filename tiebreak. The helper's own comment claimed otherwise.
It now returns name -> rank keyed on the shortcode's position, so aliases
tie. Unknown and repeated shortcodes still contribute nothing rather than
shifting the ranks after them, which is now covered by tests too.
Reported by Greptile and Copilot on rommapp#4260.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both bots flagged the same real defect in The helper now returns New tests: One correction on the Copilot comment: it said a Netherlands rom could rank below the next shortcode in the list. That part isn't reachable — with Backend suite: 1283 pass, same single pre-existing Redis-dependent auth failure as |
Description
Closes #1528.
When a game has several versions in the library (region variants, revisions, demos), the gallery collapses them into one entry with a dedup window. That window ordered each group by
is_main_siblingthenfs_name_no_ext, so absent a manual override the entry went to whichever filename sorted first:(Demo)beats(Europe)beats(USA). The issue reports this as the demo or the Japanese release being picked by default.Two ordering terms now sit between those two:
demo,beta,proto,sample,kiosk,preview).scan.priority.regionorder decides among what's left.Release status is ranked above region deliberately: a demo isn't a substitute for the game, so a Japanese release wins over a USA demo. A manually set main sibling still wins over everything, and a group whose only member is a pre-release or an unlisted region still gets its entry, so nothing disappears from the gallery.
scan.priority.regionalready existed, but it only steered metadata and artwork selection. Its description said as much, so that string is reworded across all 18 locales.Why the generated column
Ranking by region means the dedup window has to read each rom's region, and
roms.regionsis JSON that no covering index can carry (a prefix index can't answerJSON_CONTAINSon its own). #4054 (migration0107) measured what a single uncovered reference costs this window, and it is not subtle:generated_primary_regionindexidx_roms_sibling_coverroms.regions(JSON)ALLSo
0108adds a STORED generated column overregions[0]and covers it inidx_roms_sibling_cover, and the ranking reads that scalar instead. The engine computes it at write time and keeps it in sync, so there's no scan hook and no backfill:ALTER TABLEpopulates existing rows. Multi-region names like(USA, Europe)resolve to the first tag, which is the release's primary market. Index key length goes to 2451 bytes, inside both the InnoDB 3072-byte limit and PostgreSQL's btree tuple limit.Pre-release detection matches the filename rather than the parsed
tagscolumn, because tags keep whatever casing the dumper used andfs_name_no_extis already in the index.Reordering the priority list changes which rom represents a group, so the cached gallery sidecars are now dropped when the setting changes.
Known limitations
(US)/(EU)/(JP)are not, since those aliases were intentionally dropped in 9b9dba0 and re-adding them is coupled to atags→regionsbackfill. Worth its own PR if people hit it.Rom.revisionisString(100)and adding it to the covering index puts the declared key length at ~3036 of 3072, which is legal but leaves no room for the next column. It wants a shorter column or a prefix index, and that's a separate decision.scan.priority.regionis admin-level config, not per-user. The issue's own fallback suggests this is acceptable; per-user can layer onto the same ordering code later, and would live inUser.ui_settingswith no migration.Migration numbering
0108collides with #4241 (draft), which also adds0108. Master is at0107so this chain is valid as-is, but whichever lands second needs renaming to0109and re-chainingdown_revision.Checklist
Testing
Backend suite: 1277 pass (the one failure is a pre-existing Redis-dependent auth test that fails identically on
master). Frontendtypecheckclean, Settings tests 23/23, both i18n parity scripts pass, Trunk clean.New tests cover the primary-rom choice (pre-release vs full release, region priority, unlisted region, region ranked below release status) and the generated column mirroring
regions[0]. Each fails without the corresponding ordering term.test_roms_group_by_index.pygainedtest_dedup_window_expressions_read_only_covered_columns. The existing guard only inspected bare selected columns, so aCASEover an uncovered column passed it while silently dropping the plan to a full scan; the new check inspects the compiled SQL. It is what caught the JSON approach above.Migrations run clean on MariaDB 10.11 and PostgreSQL 15, matching the CI jobs, and
alembic downgrade -1reverses both.Verified end-to-end against a running instance with four siblings (
Demo,Europe,Japan,USA):us, wor, ss, eu, jp(default)Sonic (USA)jp, eu, usSonic (Japan)eu, us, jpSonic (Europe)Sonic (USA)Ungrouped still lists all four. The demo never wins in any configuration. Since reordering took effect immediately, the cache invalidation path is exercised too.
Screenshots (if applicable)
AI assistance disclosure
This change was written with AI assistance (Claude Code, Claude Opus 5), used extensively: investigating the issue, designing the approach, writing the implementation, migration, and tests, and running the verification described above. All of it was reviewed by me before submitting. Notably, the first implementation read
roms.regionsdirectly and was reverted after the covering-index constraint surfaced; the generated-column design and the strengthened test guard came out of that.