Nointro rom verification

[Pfuenzle]

Database Migration

YES - 011, 012

Description

Adds No-Intro catalog coverage for Nintendo Game Boy, Game Boy Color, Game Boy Advance, and Nintendo DS ROM libraries, including platform detection, ROM extension handling, and default catalog source seeding.

Extends No-Intro catalog sync with numbered release metadata so rename profiles can produce DAT-style filenames such as 0201 - Mario Kart DS (Europe) (En,Fr,De,Es,It).nds. DAT-o-MATIC numbered DATs are used when available, with ADVANsCEne release-number fallback for DS/GBA when DAT-o-MATIC cannot provide a download token.

Improves No-Intro visibility in the API and UI by surfacing catalog status in Metadata settings and exact ROM hash matches in game component tooltips.

Screenshot (if UI related)

Not included.

Todos

• Tests
• Translation Keys (./src/NzbDrone.Core/Localization/Core/en.json)
• Wiki Updates

[gamarr-bot2]

Did a first read-through — the shape is exactly what we discussed on #149 (slots only for confirmed catalog entries), and the catalog/verification split looks clean. Full review when you mark it ready, but two early notes from tonight's Sentry triage, since one of them bit your own instance:

1. Per-file hash errors abort the whole verification run. Sentry caught IOException: Input/output error from NoIntroRomHasher reading Mega Man - Dr. Wily's Revenge — one unreadable file (flaky share, permissions, mid-copy) currently kills the snapshot. Suggest wrapping the per-file hash in a try/catch and recording that file as an error/unknown result so the rest of the set still verifies.

2. Migration numbers: keep 008–015, they're yours. Main just gained a migration and I deliberately numbered it 16 (016_import_exclusions_allow_steam_only) so your 008–015 block merges without renumbering. Gaps are fine for FluentMigrator.

Also for context from the same triage: the read-only-filesystem errors from your /games/Game Boy share showed up too — those were environment (the .recycle write failed because the mount is read-only), not your code.

[gamarr-bot2]

Early review part 2 — I went deeper into the component-side integration since that's the code I know best. Three performance items and one behavior item, all in the reconciliation path, plus a rebase heads-up. Flagging them now because reconciliation runs on every import/refresh/scan event, so they'll matter on real libraries:

1. Full catalog load per reconcile: EnsureComponents calls _noIntroCatalogEntryRepository.All() every time it runs. With the seeded catalogs (GB/GBC/GBA/DS/3DS/PSP/… DATs) that's tens of thousands of rows materialized per event, per game — a refresh-all over a 500-game library loads the catalog 500 times. A GetByPlatformFamily(...) repository query (the platform is known from the game) would cut it to the relevant slice.

2. Catalog plan rebuilt per file: FindSlotForFileName calls GetSlots(...), which runs BuildCatalogPlan over the platform's entries — and GetComponentForFile invokes it for every file (twice, via the second linking pass). Computing GetSlots once per EnsureComponents and passing the list down makes the whole thing O(catalog) instead of O(files × catalog).

3. Disk enumeration per folder-backed file: FindSlotForFolderBackedFile does a recursive GetFiles(game.Path) inside the per-file loop. Hoisting the enumeration out (or computing it lazily once) avoids re-walking the same folder for every record.

4. New region slots default to monitored: true — that makes the missing-search backfill actively hunt every region/language variant the catalog confirms for every matched game. From the Support separate imported update files/versions and multiple regional or language variants for the same game #149 discussion we'd sketched the opposite default (slots exist unmonitored, users opt into the regions they want — same as DLC slots today). Suggest monitored: false here, or monitored-only-for-the-entry's-own-region if you want a smarter default.

Also a logistics note: tonight's main moved a lot in the files you touch (GameComponentService got source-keyed slots steam:<appid> + cross-source dedup, DiskScanService gained a constructor dependency and bundled-DLC splitting, GameResource.TitleSlug is now platform-suffixed). Your branch will conflict on rebase — happy to do that rebase for you and push it to your branch once you're at a good stopping point, just say the word.

None of this changes my overall take: the architecture is right and the catalog/verification split is clean. With items 1–2 addressed this should scale to real ROM libraries comfortably.

[gamarr-bot2]

Thanks for this — it's a genuinely well-built subsystem. I rebased it onto current main and took it to green so it's unblocked, and did a review pass. Details below.

Green branch: nointro-rom-green

The branch had drifted ~16 commits behind main and its CI was red. I put the current PR tree on top of latest main (branch nointro-rom-green, pushed) and it now passes everything locally:

• ✅ Full solution build — 0 warnings / 0 errors (API + Host RomCatalog controllers compile)
• ✅ Backend tests — 5176 passed, 0 failed (Gamarr.Core.Test), incl. all 36 RomCatalog tests
• ✅ Frontend — eslint 0 errors, prettier clean, stylelint clean, jest 129/129, production webpack build compiles

Only three things were needed to green it:

1. One merge conflict in GameComponentService.cs — your FindSlotForFolderBackedFile and main's new IsMetadataDlcKey landed at the same spot but don't actually conflict; I kept both.
2. Stylelint — GameFileEditorRow.css needed display → align-items → flex-direction order.
3. Your CI's earlier NoIntroCatalogSyncServiceFixture "0 Warns expected but 1 logged" failures resolved themselves against current main (the test-common's warn handling evolved since the branch was cut) — no code change needed.

Feel free to pull nointro-rom-green or cherry-pick those two fixes.

Review feedback (worth addressing before merge)

1. NoIntroRomHasher.Compute allocates per 8 KB chunk.

crc.Update(buffer.Take(bytesRead).ToArray());  // new array every chunk

For disc-based systems (PS3/PS2/Wii images run into many GB) this is hundreds of thousands of throwaway allocations and heavy GC pressure. SharpZipLib's Crc32.Update takes an ArraySegment<byte>:

crc.Update(new ArraySegment<byte>(buffer, 0, bytesRead));

Also consider a CancellationToken so hashing a huge image can be aborted.

2. NoIntroCatalogDocumentClient.FetchDatOMaticNumbered hardcodes a dated form field.

.AddFormParameter("dat_dl_2026-05-30", "Prepare")

DAT-o-MATIC rotates that date-stamped submit name periodically, and the download flow also scrapes an HTML submit button via regex — both will silently break when the site changes. This is the main long-term maintenance risk in the PR. Options: derive the dat_dl_* field name from the prepare page's HTML instead of hardcoding, and treat a fetch failure as a clearly-logged, recoverable "source needs updating" state (the sync service already records LastSyncError, which is nice).

3. Minor robustness in the same client.

• int.Parse(sourceUrl.Substring(prefix.Length)) throws on a malformed datomatic://system/…; a TryParse with a clear error is friendlier.
• ExtractDat falls back to archive.Entries.First(), which throws on an empty zip.

None of these block correctness — (1) is a real perf fix, (2) is the thing most likely to break in the field, (3) is polish.

Suggested next step

The subsystem is architecturally sound (clean entity/repo/service split, scheduled sync via TaskManager, rename-token integration) and now green on current main. If you want to take it the rest of the way, addressing (1) and (2) would make it solid to merge. Happy to help review the follow-ups.

(Reviewed and greened by an automated assistant on behalf of the maintainer.)

[gamarr-bot2]

Live smoke test — the feature works, with one confirmed break

Ran the greened branch against real sources (fresh DB, triggered NoIntroCatalogSync):

• ✅ The 27 libretro GitHub .dat sources sync cleanly — full sync produced 52,639 catalog entries / 157,908 hashes, correct canonical names (10-Pin Bowling (USA) (Proto)) and CRC32/MD5/SHA1 triplets. The fetch → parse → hash → persist pipeline is solid.
• ❌ All 5 datomatic:// sources fail with DAT-o-MATIC did not return a numbered DAT download token — confirming review point Bump fork-ts-checker-webpack-plugin from 8.0.0 to 9.1.0 #2 is a live break, not hypothetical. The FetchDatOMaticNumbered flow (hardcoded dated form field dat_dl_2026-05-30 + HTML-submit-button regex) no longer matches the current site.

Fix applied on nointro-rom-green

So the feature doesn't ship with 5 permanently-erroring sources, I dropped the 5 DAT-o-MATIC-only niche seeds (GBA Multiboot/e-Reader/Play-Yan/Video, DSvision) from both the seed migration and NoIntroCatalogDefaults. The datomatic:// support code stays — this is just seeding, so it's a one-line restore once the scrape is hardened. Verified: a fresh install now seeds 27 sources and a full sync completes 27/27 with zero errored sources (and EnsureDefaultSources no longer re-adds them). Test updated to assert they're not seeded.

Where this leaves the PR

Greened branch nointro-rom-green now passes full build + 5176 backend tests + frontend (lint/stylelint/jest/webpack) and a live functional sync. Remaining before merge:

• (recommended) Harden the DAT-o-MATIC scrape if you want those 5 niche systems back — derive the form field from the prepare page instead of hardcoding.
• (nice-to-have) Hasher perf — NoIntroRomHasher allocates per 8 KB chunk; use ArraySegment (review point Bump react and @types/react #1).
• Still untested end-to-end: verifying a real ROM against the catalog (hashing is unit-tested and the catalog side is now proven, so low risk).

(Smoke-tested and fixed by an automated assistant on behalf of the maintainer.)

[gamarr-bot2]

@Pfuenzle — over to you to land this. 👋

Thanks again for this — it's genuinely well-built (the dual XML/ClrMamePro DAT parsing, the raw-file + single-zip verification handling, the bad-dump status flagging). We'd like to see it merged.

Where it stands: I rebased your work onto current main, resolved the one conflict (GameComponentService.cs), and greened it on branch nointro-rom-green — full build + 5,176 backend tests + all frontend checks pass, and a live sync works: 27 libretro sources pulled 52,639 entries / 157,908 hashes with zero errors. Feel free to pull that branch or cherry-pick the fixes.

What's needed to land it (details in the two comments above):

1. DAT-o-MATIC scrape is currently broken — all 5 datomatic:// sources fail live (did not return a numbered DAT download token); the hardcoded dated form field + HTML-button regex no longer match the site. I've unseeded those 5 niche systems on the green branch so a fresh install is clean, but if you want them back, FetchDatOMaticNumbered needs to derive that field from the prepare page instead of hardcoding it.
2. NoIntroRomHasher allocates per 8 KB chunk (buffer.Take(n).ToArray()) — rough on multi-GB disc images; crc.Update(new ArraySegment<byte>(buffer, 0, bytesRead)) fixes it.

Both are optional-for-merge (the feature works without them — #1 just means those 5 systems stay unseeded), so land what you're comfortable with.

Timing: no rush, but if the PR goes quiet for ~a week we'll likely adopt the greened branch as-is so it doesn't bit-rot against main again. Happy to review your follow-ups whenever you push them.

[gamarr-bot2]

Small heads-up from Sentry: a dev build (0.0.1.0, so a local Release build — I'm guessing yours) hit the PathEquals rescan crash tonight (ArgumentException: value [DLC/...] is not a valid *nix path). That's fixed on current main (9466767) — your branch predates it, so rescans of any game with tracked DLC folders will keep aborting on your dev instance until the branch is rebased. The offer stands: happy to do the rebase (including the conflicts in GameComponentService/DiskScanService I mentioned above) and push it to your branch whenever you like.

[Pfuenzle]

I’ve now rebased the branch onto the green nointro-rom-green baseline and addressed the follow-up review points. The PR now tolerates per-file hash/I/O failures without aborting verification or the component API, avoids rebuilding/loading the full No-Intro catalog on every reconciliation pass, hardens the DAT-o-MATIC numbered fetch path, and surfaces version/release-group metadata in the component UI with the more generic “Catalog” labeling.

[gamarr-bot2]

The rebase looks great and every review point is addressed properly — nice work on the catalog scoping in particular. CI was red though, so I checked the branch out and pushed a fix (aac4e38) rather than bounce it back to you. What was failing, in case it's useful:

• The sync tests weren't failing on logic — the new fault-tolerance logs warnings when enrichment fetches fail, and the test framework fails any test with unexpected warnings. The unmocked FetchDatOMaticNumbered/FetchAdvanscene calls returned null → caught → warned. I gave the fixture default empty-document mocks and added ExceptionVerification.ExpectedWarns(1) to the two tests that fail fetches on purpose.
• One real find while I was in there: ADVANsCEne enrichment downloaded its zip even when DAT-o-MATIC had already numbered every entry — it now skips when there's nothing left to number.
• Two rename regressions: requiring the on-disk filename to already match the catalog meant a Gamarr-named file could never be renamed into the catalog scheme (your should_use_nointro_catalog_filename… test caught it) — filename match is now a disambiguation preference with title/key fallback. And the Gamarr profile now restores the canonical variant filename for catalog-matched files instead of keeping the current name, per your …preserve_original_nointro_variant_filename test — that's what stops two region variants collapsing onto one Gamarr name.

Full suite is green locally (5,179 tests). Whenever you're happy with it, flip it out of draft and I'll do the final review pass.

[gamarr-bot2]

Full final pass done — approving. Everything from the earlier reviews is addressed (per-file fault tolerance, catalog scoping, the DAT-o-MATIC hardening, unmonitored-by-default region slots), the rename semantics landed in a better place than my interim fix (original-filename identity means only verifiably catalog-named files are renamed into the scheme), and I verified the branch merged with current main: full backend suite (5,183 tests), frontend lint/tsc/stylelint/jest, and a live smoke — all 16 migrations apply cleanly on a fresh database, the seven catalog tables and 27 seeded sources come up, and the instance starts without errors. Thanks for a genuinely excellent contribution — this is the biggest outside feature Gamarr has taken and it sets up region variants exactly the way we sketched on #149.