Skip to content

perf(home): stop counting the whole library for the two home rows - #4110

Merged
gantoine merged 1 commit into
rommapp:masterfrom
Spinnich:fix/home-rows-skip-total
Aug 4, 2026
Merged

perf(home): stop counting the whole library for the two home rows#4110
gantoine merged 1 commit into
rommapp:masterfrom
Spinnich:fix/home-rows-skip-total

Conversation

@Spinnich

@Spinnich Spinnich commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes #4099

The home page renders two fixed rows, "Recently added" and "Continue playing", each requesting 15 games. Both also asked the server to count the result set and then discarded the number, since the rows only ever draw 15 covers.

GET /api/roms already accepts with_total (added for the gallery in #4062). This passes with_total=false on both home requests.

Measured against a real 83,130-game library (RomM 5.1.1-beta.1), median of 8 alternated runs:

Home row count ON count OFF saved
Recently added 0.299s 0.163s 0.137s (~46%)
Continue playing 0.675s 0.689s none (within noise)

The win is concentrated in "Recently added", whose count spans the whole library. "Continue playing" carries last_played=true, so its count covers only the played subset (90 rows here) and was never expensive. The flag is applied there too for symmetry and because the saving scales with a user's played library, but it is not where the time goes: with the count already off, that request measures 0.168s unfiltered, 0.477s once the last_played filter is applied, and 0.692s once sorted by last_played. That cost belongs to #4067, not here.

#4062 deliberately left these two alone. The home rows go through the client-side Cache API layer, where clearCacheForPattern does a substring match against a cache key built as new URLSearchParams(params).toString(). The request params and the cache-clear pattern were two independent copies of the same literal map, so adding a parameter to one side and not the other, or at a different position, would silently stop those rows from ever being invalidated again.

So rather than introduce a fourth copy of the literal, each row's params now live in a single const that both the request and its clear pattern read from, which makes the desync unrepresentable rather than merely avoided.

Testing

  • npm run test - 53 files, 640 tests passing (up 4, all additive; no existing test changed).
  • npm run typecheck - clean. trunk fmt && trunk check - no issues.
  • The two invalidation tests were checked for vacuousness rather than assumed to work: injecting the exact half-done change the issue warns about (adding with_total to the clear pattern only) makes both fail, and they pass again once reverted.
  • Verified against a live 83,130-game 5.1.1-beta.1 instance using the exact query strings the branch emits. with_total=false returns total: null with the same 15 items in the same order; the response envelope keys and every per-item field are otherwise unchanged. Timings above.
  • Not exercised through the browser UI. There is no visual change and both rows already ignore total, but end-to-end confirmation would be watching the two home /api/roms requests carry with_total=false.

Anything a reviewer should pay attention to

  1. The parameter maps are the contract. Anything added to these two rows must be added to the const, never at a call site. A parameter present on the request but not the clear pattern (or in a different order) makes the cache key stop matching the clear pattern, and the row goes permanently stale with no error. That hazard is exactly why this was carved out of perf(roms): let a gallery window fetch skip the library count #4062, and the two invariant tests exist to catch it.
  2. total is now null for these two responses. Verified nothing consumes it: stores/roms.ts destructures only items, and both home views read the resulting store arrays. CustomLimitOffsetPage_SimpleRomSchema_.total is already typed number | null, so no regeneration is needed.
  3. No backend change. resolve_total() already returns None when with_total is false and the ROM id index is not being built, which is what both rows send.
  4. The shared maps are safe to share. Both consumers treat config.params as read-only: the axios paramsSerializer and CacheService.generateCacheKey each construct a fresh URLSearchParams. Nothing mutates the object, so the request and the clear pattern cannot diverge at runtime.
  5. Not a frozen path. The change is confined to src/services/**; the v1 freeze gate covers frontend/src/{views,components,console,layouts}/**. Both the v1 and v2 home pages benefit, since they share stores/roms.ts.

AI Assistance Disclosure

AI assistance (Claude Code) was used for this contribution, covering code generation, the unit tests, and this pull request description. All changes were reviewed and verified locally by me before submission. Any replies I post on this PR are written by me unless stated otherwise.

Checklist

  • I've tested the changes locally
  • I've updated relevant comments
  • I've assigned reviewers for this PR
  • I've added unit tests that cover the changes

The "Recently added" and "Recently played" rows each ask for 15 games and
render 15 covers, but both requests still made the server count every game
in the library and then discarded the number. On a large library that count
is the expensive part of the request, so opening the home page paid for two
whole-library counts nobody sees.

Both rows now send with_total=false, the option added for the gallery in
rommapp#4062.

The flag has to reach the request and the cache-clear pattern together: the
pattern is matched against the cache key by substring, so adding it to only
one side would leave the row permanently stale. The cached service now
builds both from a single parameter map per row so they cannot drift, and
the tests assert the clear pattern still matches the key the request writes.

Fixes rommapp#4099

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR avoids unnecessary whole-library counts for the two fixed-size home rows while preserving their cache invalidation contract.

  • Sends with_total=false for recently added and recently played requests.
  • Shares each cached request’s parameter map with its corresponding invalidation path.
  • Adds request-shape and cache-invalidation regression tests.

Confidence Score: 5/5

The PR appears safe to merge with no concrete correctness or security issues identified.

The backend explicitly supports omitting totals, affected consumers only use response items, and request/invalidation serialization remains synchronized through shared parameter objects.

Important Files Changed

Filename Overview
frontend/src/services/cache/api.ts Shares stable request and invalidation parameters for both home rows and disables unused total counts without changing item retrieval.
frontend/src/services/cache/api.test.ts Verifies both count opt-outs and confirms each invalidation pattern matches the cache key produced by its request.
frontend/src/services/api/rom.ts Adds the same total-count opt-out to the uncached recent-ROM helper functions for behavioral parity.

Reviews (1): Last reviewed commit: "perf(home): stop counting the whole libr..." | Re-trigger Greptile

@Spinnich
Spinnich requested a review from gantoine August 4, 2026 17:36
@gantoine
gantoine merged commit 5f61452 into rommapp:master Aug 4, 2026
8 checks passed
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.

[Bug] The home page counts the entire library twice on every load, for numbers it never shows

2 participants