Skip to content

Fix bugs and slow paths found in a codebase-wide review - #21

Merged
milnet01 merged 8 commits into
mainfrom
claude/modest-goodall-5faq39
Oct 29, 2026
Merged

milnet01 merged 8 commits into
mainfrom
claude/modest-goodall-5faq39

Conversation

@milnet01

Copy link
Copy Markdown
Owner

What

This PR fixes about 55 bugs, slow code paths and cleanups found by reviewing routes, services, background jobs, scrapers, JS, CSS and templates. Each fix is small and has its own commit for its area. Nearly every behavioural fix has a regression test, in four new test files (tests/test_*review*.py, tests/test_scraper_lookup_defaults.py) and in additions to tests/test_metadata_merger.py. The changes go into the open 3.33.0 changelog entry, which now has 10 items.

Why

The review asked for bugs, performance, refactoring and efficiency fixes. The most important user-facing problems fixed:

  • Clear scraped data never saved the title reset: close() without commit() undid it.
  • Per-console RetroAchievements refresh failed with a TypeError (a 500 error) every time.
  • IGDB age ratings for CERO, USK, GRAC, ClassInd and ACB used the wrong codes. IGDB's rating codes are one list shared by every board; the code read them as 1–5 for each board.
  • Made-up player counts:
    • ES-DE turned a missing <players> into players=1 / Single-Player, and wrote that over curated values. A missing <players> was always replaced with 1, so COALESCE could not keep the old value.
    • TGDB, RAWG and IGDB defaulted to 1 player the same way.
  • ScreenScraper as primary source lost most fields after the 10-minute search cache expired. Its video and manual file names were built from the URL's query string, which contains the account credentials.
  • TGDB lookups: one failed lookup-table fetch was cached as empty for the life of the process, so publishers and developers were saved as bare numbers.
  • Screenshot dedup looked in STATIC_PATH instead of IMAGE_PATH, so it found nothing in the packaged app.
  • Multi-disc scan counted every file twice on Windows and macOS. Case-only ROM renames were refused there.
  • Auto-assign controllers gave SNES the NES controller (and GBA→GB, Wii U→Wii, Xbox 360→Xbox).
  • Last admin: the only admin could be demoted or deactivated, leaving nobody able to manage settings or users.
  • Masking: the ScreenScraper passwords were returned unmasked by GET /api/scraper-settings and on the settings page.
  • Card grid on a phone: one card was followed by a gap thousands of pixels tall. The column count read from the computed grid includes implicit tracks created by the cards' own inline placement, so the grid never switched to one column.
  • Jobs:
    • swapped or demoted bulk scrapes left queued rows that came back as "interrupted" after a restart;
    • bulk scrape and museum image jobs could be left marked running;
    • a player-core request could be lost if it arrived just as the worker stopped;
    • recovery dedup threw away all but one multi-system scrape;
    • the HLTB and alt-title jobs held SQLite's write lock across 25 network calls.

How

  • Performance:
    • Per-item queries are replaced by one query plus a dict: /api/controllers, the Steam/Xbox/PSN library duplicate check, /trophies matching, the collector-trophies refresh and the orphan-media scan.
    • Writes are batched with execute_many where the code committed once per row: storage calculation, PSN trophy sync and collector trophies.
    • The collector rank is cached per request on g.
  • Connections: query / execute now close connections they open outside an app context. Inside a request nothing changes.
  • Frontend:
    • Late fetch replies can no longer overwrite newer state: game detail modal, version search, log viewer, official names, museum status and job-progress pop-ups.
    • CHD tool file names are escaped before going into the page.
    • Undefined CSS variables are replaced with existing tokens. The skip-link focus outline, the settings tab bar and the rename modal were missing their colours or backgrounds.
    • The card grid clears inline placement on a full layout, and a ResizeObserver on the grid re-lays it out once its width settles. A window-resize timer fired while the sidebar was still animating and read the wrong column count.
  • Docs: docs/specs/jobs.md §8 is updated for the multi-system scrape dedup change.

Each fill-only UPDATE in the scrapers was checked and already used COALESCE. The fixes here remove the made-up defaults that slipped past it.

Mandatory-workflow checklist

  • Version — 3.33.0 has no tag yet, so its entry stays open. Only APP_LAST_UPDATE was bumped in config.example.py (config.py is gitignored).
  • Changelog entry — 8 items added to 3.33.0 (10 total). The per-locale changelogs are not touched: 3.33.0 is translated at release.
  • CSS rebuilt
  • JS rebuilt
  • Tests run — pytest -n 8: 2269 passed (the baseline was 2225 passed, 1 skipped).
  • Lockfile — requirements.txt was not changed.
  • CLAUDE.md — no routes, templates, bundles or asset wiring changed.

Verification

  • Tests: the full suite passes, and so do the tests that read markdown docs. check_i18n_fresh.py passed after the JS rebuild; the only later JS change adds no t() strings, and services/js_i18n_strings.py is unchanged.
  • Browser: I set up a fresh install with 60 fake ROMs in two systems and walked /games in Chromium at 1400px, 900px and 375px, then back to 1400px.
    • At 375px the cards stack in one column, about 206px apart; before, a gap of about 6,000px followed the first row.
    • Desktop and 900px give two columns, the same as a fresh page load.
    • There is no horizontal scroll at any width.
    • The walk caught a regression introduced earlier in this branch (debounce used before the deferred core bundle loads, so no cards rendered). It is fixed in 41415e2.
  • Game detail modal: a late reply for a closed game no longer paints over the open one. After an error, the next open shows the spinner again.
  • Pages: /settings, /systems, /logs, /analytics, /museum and /collector-trophies all return 200 with no page errors. The settings tab bar now has a background and the skip-link focus outline is visible.

Not verified:

  • Live scraper APIs (IGDB, ScreenScraper, TGDB, RAWG). These fixes are covered by unit tests only. The IGDB newer organization / rating_category text mapping is based on IGDB's documented tables and has not been checked against live responses. The legacy code table is IGDB's documented enum.
  • The case-insensitive rename fix: Linux CI cannot reproduce it.
  • The GUI installer error path, which needs tkinter.

Found but not fixed (left for a follow-up):

  • /api/jobs/dismiss has no ownership check, unlike resume.
  • The settings page puts the real API keys into the page HTML for non-masked fields.
  • templates/settings.html inserts admin-entered names via innerHTML without escaping.
  • Imported ES-DE logos under IMAGE_PATH are not picked up by refresh-logos.
  • Demoting the last admin reuses the existing "Cannot delete the last admin user" message, to avoid a new translation string.

This PR touches several subsystems, so consider running /code-review ultra before merging.


Generated by Claude Code

- Screenshot dedup scans/deletes under IMAGE_PATH, not the bundle STATIC_PATH
- Multi-disc scan lists each disc file once (case-insensitive FS double-count)
- Default-controller auto-assign tries longest folder pattern first (snes != nes)
- /api/controllers: one systems query instead of one per controller
- Calculate storage: one batched write instead of a commit per game
- Local search clamps negative limit; similar-games escapes LIKE wildcards
- AI Fill invalidates the Library filter cache
- Case-only ROM renames allowed (samefile check)
- Database optimize closes its connection when VACUUM fails
- ROM-tools task cleanup iterates a snapshot; browse-folders single scandir

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013s5ztLFVCcdTjjTeVcTnJr
services:
- Clear scraped data commits the filename-title reset (close() rolled it back)
- Analytics "not started" sums NULL and 'not_started' groups
- query/execute helpers close connections they open outside an app context
- JPEG save converts P/LA/PA/I/F modes, not only RGBA
- Orphan-media scan looks basenames up in a set (was orphans x references)

jobs:
- Per-system RA refresh accepted system_name (TypeError -> 500 before)
- Swapped/demoted bulk scrapes drop their 'queued' job_queue row
- Bulk scrape settings load inside the try (no stuck 'running' job)
- HLTB / alt-titles jobs commit per game, not holding the writer lock
  across 25 network calls
- Player-core worker re-runs a request that raced its shutdown
- Museum image fetch: import/connect failures end the job cleanly
- Recovery dedup no longer collapses distinct multi-system scrapes
- GUI installer surfaces worker errors; .desktop Exec paths are quoted

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013s5ztLFVCcdTjjTeVcTnJr
- Card grid: one-column phone layout no longer gets inline row lines;
  re-layout when a resize changes the column count; measure all cards
  before placing (one forced layout instead of one per card)
- Game detail modal: late replies no longer paint a closed/other game;
  error message no longer replaces the spinner for good; AI Fill only
  reopens the game still shown
- Job toasts: a slow status reply is no longer aborted by the next poll
- Stale-reply guards: museum status, version search, log viewer,
  official-names proposals; Kill closes the Now-playing popover
- CHD converter/verify escape disk file names in innerHTML
- Replace undefined CSS variables with existing tokens (skip-link focus
  outline, settings tab bar, rename modal backgrounds were invisible)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013s5ztLFVCcdTjjTeVcTnJr
- The last active admin can no longer be demoted or deactivated
- /api/psn/games clamps page/per_page (per_page=0 was a 500)
- Steam/Xbox/PSN library fetch: duplicate check uses one index built
  from one query instead of re-reading the system per fetched title
- /trophies reads and cleans PlayStation titles once, not per trophy set;
  PSN full sync cleans each title once
- PSN single-game sync writes trophies in one batch; detail-page debug
  logging moved from INFO to DEBUG
- Collector trophies refresh: one read and one transaction (~140 round
  trips before); sidebar rank cached per request

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013s5ztLFVCcdTjjTeVcTnJr
- IGDB age ratings read IGDB's single global rating enum (CERO/USK/GRAC/
  ClassInd/ACB were mapped as 1-5) and the newer organization /
  rating_category shape; hybrid fetch requests those fields
- IGDB players: null offlinemax no longer aborts the merge; no made-up 1
- ScreenScraper primary reads the flat get_game_info dict on a cache miss
- ScreenScraper media extensions come from the URL path, not the query
  string (which carried account credentials into file names)
- TGDB lookup tables: a failed fetch is retried, not cached empty;
  unknown IDs are dropped instead of written as bare numbers
- TGDB / RAWG / ES-DE no longer invent players=1 (ES-DE overwrote
  curated players/modes through COALESCE)
- ES-DE gamelist: full-path match wins over a same-filename match
- RetroAchievements: curly apostrophes normalised; search title once
- Scans keep an existing system logo; scan_roms finds SVG logos

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013s5ztLFVCcdTjjTeVcTnJr
- all-games-controller: the resize handler called debounce() at load,
  before the deferred core bundle defines it, so the Library and System
  pages threw and rendered no cards. Use a local timer.
- The column count read from computed grid-template-columns includes the
  implicit tracks the cards' own inline placement created, so a grid
  narrowed to one column (phone) kept laying out two columns with a huge
  gap. A full layout clears inline placement first; a ResizeObserver on
  the grid re-lays out once its width settles (a window-resize timer
  fired mid sidebar animation and read the wrong count).
- screenscraper_password / screenscraper_devpassword join
  SECRET_API_KEY_FIELDS: masked on GET /api/scraper-settings and the
  settings page, kept on a masked echo.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013s5ztLFVCcdTjjTeVcTnJr
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013s5ztLFVCcdTjjTeVcTnJr
- /api/jobs/dismiss checks the same role / ownership as resume
  (_may_manage_job, shared); any signed-in user could dismiss any job
- The dashboard recovery banner leaves out jobs the user may neither
  resume nor dismiss (a player no longer sees an interrupted bulk scrape)
- Demoting or deactivating the last admin gets its own message;
  catalogs regenerated
- CLAUDE.md: a release needs at least 5 changelog items, no upper limit
  (user, 2026-10-27)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013s5ztLFVCcdTjjTeVcTnJr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants