Skip to content

fix: refresh the v2 gallery when a ROM is edited or matched - #4153

Open
sdornan wants to merge 1 commit into
rommapp:masterfrom
sdornan:claude/game-list-auto-update-f423c6
Open

fix: refresh the v2 gallery when a ROM is edited or matched#4153
sdornan wants to merge 1 commit into
rommapp:masterfrom
sdornan:claude/game-list-auto-update-f423c6

Conversation

@sdornan

@sdornan sdornan commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Note

This PR was written primarily by Claude Code (code, tests and this description). I reviewed the approach and directed the scope at each step.

Description

The v2 gallery ignored its own writes: editing or matching a ROM left the card showing the old name and cover.

The v2 gallery renders its cards from galleryRoms.byPosition, but every v2 write site only patched v1's stores/roms, whose _allRoms the gallery never reads. Editing or matching a ROM replaces it with a fresh object from the API response, so the card kept showing the pre-edit name and cover until its window happened to be refetched (navigating away and back, a filter change, a scan).

Optimistic toggles (favourite, playing status) appeared to work without this only by accident: they mutate the cached object in place, and the gallery holds that same reference.

galleryRoms.update() already existed for exactly this purpose and had no callers.

This adds useRomSync and routes the 20 v2 write sites through it:

  • syncRom fans a write out to both stores. Used by the optimistic toggles and the asset tabs, which keep the cheap in-place update — invalidating the windows costs skeletons and the scroll position, too much for a favourite flip or a screenshot upload.
  • applyRomWrite (edit / match dialogs) additionally refetches when an in-place swap would leave the list lying:
    • Membership — any active filter forces a refetch. A match rewrites provider ids, name, and the genre / company / tag metadata behind half the drawer, so guessing which filters it moved would mean re-deriving backend predicates on the client. This is what makes "filter to games not matched in IGDB → match one → it leaves the list" work.
    • Order — compares only the value the gallery is currently ordered by, so a rename under name-ascending moves the card while a summary-only edit leaves the gallery (and its scroll position) alone.
  • refreshAfterUserStateChange covers favourite / status writes, guarded narrowly on just the filters those can move (Favourites collection, favourites filter, status filter). Notably the existing "drop it from the Favourites view" branches in useFavoriteToggle and SelectionBar check v1's currentCollection, which v2 never sets — so un-hearting a game while viewing Favourites left the card in place. The bulk bar reconciles once per action rather than once per selected ROM.

The one remaining romsStore.update in useScanLifecycle is deliberate: it's the v1 gallery branch, and the v2 gallery is already refreshed by refreshGallery() a few lines above.

v1 is untouched.

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

Testing

11 unit tests in useRomSync/index.test.ts. Manually verified in a local instance (mock library, SNES + GBA), in both light and dark themes:

Case Result
Rename a ROM Card shows the new title and cover immediately
Same rename with the fix reverted Card stays stale — confirms the diagnosis
Rename under name-ascending, unfiltered Card moves to its new position; AlphaStrip follows
Summary-only edit Updates in place, no refetch, scroll preserved
Give a ROM an IGDB id while filtered to "not matched in IGDB" Row disappears
Unmatch while filtered to matched Row disappears

Not verified, needs a reviewer with provider credentials: the real Match ROM search-and-pick flow. My test instance had no IGDB/ScreenScraper/MobyGames keys, so provider search returned nothing. I exercised the identical applyRomWrite path through the edit dialog with the same membership change (gaining an igdb_id), and the match dialog's call is a single unconditional line after syncRom — but the search flow itself is untested.

Verification run: frontend typecheck, 686 frontend tests, npm run build, the full backend suite (2798 passed, 2 skipped), and trunk fmt && trunk check — all clean. No locale, token, or /lib primitive changes, and no response-schema or route-signature change, so the i18n check, build:tokens, Storybook, and npm run generate don't apply.

Out of scope: cross-client freshness. Everything here is same-client. There's no Socket.IO event for ROM mutations, so a second open browser stays stale until it refetches on its own. That follow-up is built and waiting on claude/rom-mutation-broadcast — deliberately kept out of this PR because it adds a backend event contract, a new request header, and a change to the shared axios interceptor, which is a different blast radius from a frontend cache fix, and because it hasn't been verified with two live browsers yet. Happy to open it as a follow-up PR once this lands. This PR is most of its client-side groundwork: the listener just hands each refetched ROM to applyRomWrite.

@sdornan

sdornan commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Resolved: the broadcast has been split back out — this PR is frontend-only again. The work lives on claude/rom-mutation-broadcast and can follow as its own PR once this lands.

Splitting because that change has a different blast radius: a backend Socket.IO event contract, a new X-Socket-Id request header, and a change to the shared axios interceptor every v1 and v2 request goes through. It also hasn't been verified with two live browsers yet, and I'd rather that uncertainty didn't sit on top of a frontend cache fix that has been.

Earlier notes on the broadcast design

The two design points, for whenever it gets reviewed:

  • Ids, not serialized ROMsrom_user is scoped to the requesting user, so a shared payload would show every client the actor's playing status, rating and last-played. Each client refetches and resolves its own.
  • Echo suppression per connection, not per user — two tabs of one account are two clients that each need the update, and on a single-user instance every client is the same user, so matching on the user would suppress the event everywhere.

@sdornan
sdornan force-pushed the claude/game-list-auto-update-f423c6 branch from 9005923 to e430f73 Compare August 7, 2026 17:35
@sdornan sdornan changed the title fix: refresh the v2 gallery when a ROM is edited or matched fix: keep the v2 gallery in sync when a ROM changes Aug 7, 2026
@sdornan
sdornan force-pushed the claude/game-list-auto-update-f423c6 branch 2 times, most recently from 436cebe to 4f07558 Compare August 7, 2026 18:08
@sdornan sdornan changed the title fix: keep the v2 gallery in sync when a ROM changes fix: refresh the v2 gallery when a ROM is edited or matched Aug 7, 2026
@sdornan
sdornan marked this pull request as ready for review August 7, 2026 18:09
Copilot AI lite review requested due to automatic review settings August 7, 2026 18:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes stale v2 gallery cards after ROM mutations by introducing a v2-side sync composable that updates both the legacy v1 ROM store and the v2 gallery window cache, and selectively triggers a gallery refetch when a write can change membership or ordering.

Changes:

  • Add useRomSync (syncRom, applyRomWrite, refreshAfterUserStateChange) to fan out ROM updates across v1 and v2 caches and refresh the gallery when required.
  • Route v2 write paths (edit, match, user toggles, asset refreshes, bulk actions) through useRomSync instead of updating only the v1 store.
  • Add unit tests for useRomSync behavior around filtering, ordering, and favourites/status membership.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
frontend/src/v2/composables/useRomSync/index.ts New composable to sync ROM mutations to both stores and refresh v2 gallery windows when needed.
frontend/src/v2/composables/useRomSync/index.test.ts Unit tests covering sync vs refetch behavior for filters, sorting, and user-state changes.
frontend/src/v2/composables/useGameActions/index.ts Switch optimistic user-state writes to syncRom and reconcile gallery membership post-write.
frontend/src/v2/composables/useGameActions/index.test.ts Update test mocking for the new useRomSync usage in useGameActions.
frontend/src/v2/components/GameDetails/ScreenshotsSubtab.vue Use syncRom when refetching the ROM after screenshot operations.
frontend/src/v2/components/GameDetails/SaveDataTab.vue Use syncRom when refetching the ROM after save/state operations.
frontend/src/v2/components/GameDetails/MediaTab.vue Use syncRom when refetching the ROM after media operations.
frontend/src/v2/components/GameDetails/ManualSubtab.vue Use syncRom when refetching the ROM after manual operations.
frontend/src/v2/components/GameDetails/MainSiblingToggle.vue Use syncRom for optimistic main-sibling toggling to keep the v2 gallery in sync.
frontend/src/v2/components/GameDetails/FilesTab/FilesTab.vue Use syncRom when refetching the ROM after file operations.
frontend/src/v2/components/Gallery/SelectionBar.vue Use syncRom for bulk status updates and refresh gallery membership once per action.
frontend/src/v2/components/Dialogs/MatchRomDialog.vue Use applyRomWrite after a successful match to update caches and refetch when needed.
frontend/src/v2/components/Dialogs/ManualUploadTargetDialog.vue Use syncRom when refetching the ROM after manual upload targeting.
frontend/src/v2/components/Dialogs/EditRomDialog.vue Use applyRomWrite after edit saves to keep the v2 gallery correct for membership and ordering.
frontend/src/v2/components/Dialogs/DeleteManualDialog.vue Use syncRom when refetching the ROM after manual deletion.
Suppressed comments (4)

frontend/src/v2/composables/useRomSync/index.ts:44

  • Repo rule: avoid em-dashes in comments/text (see CLAUDE.md:64). Use a comma or separate sentence instead of an em-dash.
   * ROM the gallery has never loaded — the gallery update is a no-op when

frontend/src/v2/composables/useRomSync/index.ts:65

  • Repo rule: avoid em-dashes in comments/text (see CLAUDE.md:64). Use a comma or separate sentence instead of an em-dash.
   * say what the new result set is — so any active filter forces a refetch

frontend/src/v2/composables/useRomSync/index.ts:92

  • Repo rule: avoid em-dashes in comments/text (see CLAUDE.md:64). Use a comma or separate sentence instead of an em-dash.
   * touches `rom_user` and collection membership — checking `isFiltered()`

frontend/src/v2/composables/useRomSync/index.test.ts:162

  • Repo rule: avoid em-dashes in comments/text (see CLAUDE.md:64). Use a comma or separate sentence instead of an em-dash.
  it("applyRomWrite leaves an unloaded ROM alone — no row on screen to reorder", () => {

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread frontend/src/v2/composables/useGameActions/index.test.ts
Comment thread frontend/src/v2/composables/useRomSync/index.ts Outdated
Comment thread frontend/src/v2/composables/useRomSync/index.test.ts Outdated
@greptile-apps

greptile-apps Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds a shared ROM synchronization composable and routes v2 edit, match, asset, favourite, and user-status writes through both the legacy ROM store and the windowed gallery cache.

  • Adds in-place synchronization across v1 and v2 ROM caches.
  • Invalidates filtered or reordered gallery windows after metadata writes.
  • Reconciles favourite and status-dependent gallery membership.
  • Adds focused unit coverage for synchronization, filtering, and ordering decisions.

Confidence Score: 3/5

The PR should not merge until failed reconciliation preserves or retries the gallery and clearing last_played correctly reorders the active gallery.

The new synchronization improves same-client freshness, but a transient reconciliation failure can leave the gallery cleared indefinitely, and removing a game from Continue Playing leaves last-played-sorted windows in the wrong order.

Files Needing Attention: frontend/src/v2/composables/useRomSync/index.ts; frontend/src/v2/composables/useGameActions/index.ts

Important Files Changed

Filename Overview
frontend/src/v2/composables/useRomSync/index.ts Introduces the central cache synchronization and reconciliation policy, but destructive invalidation has no recovery when the background fetch fails.
frontend/src/v2/composables/useGameActions/index.ts Routes user-state writes into the v2 cache, but clearing last_played does not reconcile a last-played-ordered gallery.
frontend/src/v2/components/Gallery/SelectionBar.vue Reconciles bulk favourite and status membership once each action and any status reverts have settled.
frontend/src/v2/composables/useRomSync/index.test.ts Covers successful synchronization and invalidation decisions but not failed reconciliation requests or the remove-from-continue-playing ordering case.
frontend/src/v2/components/Dialogs/EditRomDialog.vue Routes successful edit responses through metadata-aware gallery synchronization.
frontend/src/v2/components/Dialogs/MatchRomDialog.vue Routes successful match responses through metadata-aware gallery synchronization.

Fix All in Claude Code

Prompt To Fix All With AI
### Issue 1
frontend/src/v2/composables/useRomSync/index.ts:81-82
**Failed refresh leaves gallery empty**

When the metadata refresh encounters a transient network or server error, this path has already cleared every loaded window and the fetch only logs the failure, causing the gallery to remain empty or loading until another action triggers a refresh.

### Issue 2
frontend/src/v2/composables/useGameActions/index.ts:430
**Last-played ordering stays stale**

When the gallery is ordered by last played, clearing `rom.rom_user.last_played` through `syncRom` replaces the cached card without repositioning it, causing the ROM to remain at its former position until the gallery is refreshed.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "fix: refresh the v2 gallery when a ROM i..." | Re-trigger Greptile

Comment thread frontend/src/v2/composables/useRomSync/index.ts
Comment thread frontend/src/v2/composables/useGameActions/index.ts
The v2 gallery renders from `galleryRoms.byPosition`, but every v2 write site
only patched v1's `stores/roms`, whose `_allRoms` the gallery never reads.
Edits and matches replace the ROM with a fresh object from the API response,
so the card kept showing the pre-edit name and cover until its window happened
to be refetched. Optimistic toggles appeared to work only because they mutate
the cached object in place, and the gallery holds that same reference.

Add `useRomSync` and route the v2 write sites through it:

- `syncRom` fans a write out to both stores. `galleryRoms.update` already
  existed for exactly this and had no callers.
- `applyRomWrite` (edit / match dialogs) also refetches when an in-place swap
  would leave the list lying: any active filter, since a match rewrites
  provider ids, name and the metadata behind half the drawer, or a change to
  the value the gallery is currently ordered by.
- `refreshAfterUserStateChange` covers favourite / status writes, guarded
  narrowly on the filters those can actually move. The existing "drop it from
  the Favourites view" branches check v1's collection context, which v2 never
  sets, so they were dead under v2.
- `refreshIfOrderedBy` covers writes that mutate the cached ROM in place,
  leaving `applyRomWrite` nothing to diff: clearing `last_played` has to
  reorder a gallery sorted by it.

Optimistic toggles keep the cheap in-place update, since invalidating the
windows costs skeletons and the scroll position.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@sdornan
sdornan force-pushed the claude/game-list-auto-update-f423c6 branch from 4f07558 to c7ca9c5 Compare August 7, 2026 18:24
@sdornan

sdornan commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks both. Went through the four findings; three were valid and are fixed in the amended commit, one I'd like to scope out with reasoning.

Fixed: useRomSync mock missing refreshAfterUserStateChange (@copilot-pull-request-reviewer). Correct and worth catching. useGameActions destructures it, so it was undefined in tests. Nothing crashes today only because no test exercises setStatus / favorite yet, which made it a landmine rather than a failure. The mock now matches the composable's full surface.

Fixed: em-dashes violate CLAUDE.md:64. Also correct. Six added lines across the two new files, plus the commit message. Cleaned; I left the pre-existing em-dashes in the files I only touched lightly alone rather than reformatting unrelated comments.

Fixed: last-played ordering stays stale (@greptile-apps). Valid, and the interesting one. removeFromContinuePlaying clears last_played (a sort key) but went through syncRom, which never reorders. Worth noting the obvious fix does not work: swapping it to applyRomWrite would still miss it, because that call site mutates the cached ROM in place, so the before/after comparison sees one object and finds no change. Added refreshIfOrderedBy(key) for exactly this shape, where the caller names the field it wrote, and called it there. Two tests cover it.

Scoping out: failed refresh leaves the gallery empty (@greptile-apps). Real, but not introduced here and not fixable in the right place from this diff. invalidateWindows() followed by a fetchInitialMetadata() that only logs on failure is the pre-existing pattern in galleryRoms.remove() and useScanLifecycle.refreshGallery(); this PR reaches it from more places but doesn't create it. A proper fix needs new store state (the store tracks failedWindows but has no equivalent for a failed metadata bootstrap) plus a view-level retry or error surface, which is a bigger change than a cache-sync fix should carry. Happy to open a separate issue for it if a maintainer agrees that's the right split.

Re-verified after the changes: typecheck, 680 frontend tests, full backend suite (2798 passed, 2 skipped), and trunk fmt && trunk check all clean.

@greptile-apps

greptile-apps Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

The three fixes all look correct, and the refreshIfOrderedBy shape is a cleaner API than forcing applyRomWrite to handle a case where the before/after comparison is blind — good call there.

On scoping out the failed-refresh finding: the reasoning is sound. The pre-existing invalidateWindows + silent-log-on-failure pattern in galleryRoms.remove() and useScanLifecycle.refreshGallery() confirms this isn't a regression introduced here. And you're right that a proper fix — new store state for a failed metadata bootstrap, plus a retry/error surface in the view — is a materially different scope from a cache-sync fix. Carrying it here would make the blast radius harder to review and reason about.

Worth filing a tracking issue now so it doesn't get lost. The description you've already written ("store tracks failedWindows but has no equivalent for a failed metadata bootstrap") is a ready-made issue body. If you file it, linking it from the scoping note in this thread gives future reviewers a clean paper trail.

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