feat(firmware): let missing firmware be found, cleaned up, and kept out of the player - #4077
feat(firmware): let missing firmware be found, cleaned up, and kept out of the player#4077Spinnich wants to merge 4 commits into
Conversation
A scan flags firmware whose file vanished, but nothing could act on the flag: stale BIOS entries could only be found by opening each platform's Firmware tab one at a time, the player still offered them (auto-selecting the sole entry, so the game failed to boot with nothing pointing at the BIOS), and platform firmware counts included them. Adds a `missing` filter to the firmware list endpoint, a `cleanup_missing_firmware` manual task alongside the ROM one, and a Missing firmware tab in Library management that lists every flagged entry library-wide with a bulk cleanup. The player now asks for present firmware only, and firmware selection moved to a `playerFirmware` util that never picks a missing entry. `firmware_count` counts only firmware present on disk; the rows themselves still ship so the Firmware tab can strike them through. `FirmwareSchema` gains `platform_id` so the library-wide list can group by platform. Fixes rommapp#4075 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fold `selectableFirmware` back into `resolveInitialFirmware`. It had no call site of its own, so the export existed only for its tests, and the filtering it did is already covered through `resolveInitialFirmware`. Clear the post-cleanup refetch timer on unmount so leaving the tab within the delay can't surface a fetch error over an unrelated page. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Greptile SummaryThe PR makes missing firmware queryable and cleanable, excludes unavailable firmware from platform counts and the v2 player, and adds a library-wide management tab.
Confidence Score: 4/5The cleanup refresh race should be fixed before merging because a queued job can leave the new management tab showing stale firmware indefinitely after reporting success. The backend returns as soon as cleanup is queued, while the new component refreshes exactly once after 1.5 seconds and has no task-completion-driven reconciliation, so normal worker delays produce persistently incorrect UI state. Files Needing Attention: frontend/src/v2/components/Settings/MissingFirmwareSection.vue Important Files Changed
Prompt To Fix All With AI### Issue 1
frontend/src/v2/components/Settings/MissingFirmwareSection.vue:129-134
**Fixed-delay cleanup reconciliation**
If the low-priority worker takes more than 1.5 seconds to start or finish this job, `runTask` returns after enqueueing and the sole refresh runs before deletion completes, leaving stale firmware rows displayed indefinitely after the success notification.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "refactor(firmware): tighten the player f..." | Re-trigger Greptile |
| await taskApi.runTask("cleanup_missing_firmware", body); | ||
| snackbar.success(t("settings.cleanup-firmware-queued")); | ||
| // Give the queued task a moment to land before reflecting the result. | ||
| refetchTimer = setTimeout(() => { | ||
| void fetchMissingFirmware(); | ||
| }, 1500); |
There was a problem hiding this comment.
Fixed-delay cleanup reconciliation
If the low-priority worker takes more than 1.5 seconds to start or finish this job, runTask returns after enqueueing and the sole refresh runs before deletion completes, leaving stale firmware rows displayed indefinitely after the success notification.
Knowledge Base Used: Tasks and Scheduler
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/v2/components/Settings/MissingFirmwareSection.vue
Line: 129-134
Comment:
**Fixed-delay cleanup reconciliation**
If the low-priority worker takes more than 1.5 seconds to start or finish this job, `runTask` returns after enqueueing and the sole refresh runs before deletion completes, leaving stale firmware rows displayed indefinitely after the success notification.
**Knowledge Base Used:** [Tasks and Scheduler](https://app.greptile.com/romm/-/custom-context/knowledge-base/rommapp/romm/-/docs/tasks-and-scheduler.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Both cleanup tabs refreshed on a fixed 1.5s delay after enqueueing their task. The run endpoint returns as soon as the job is queued, so whenever the low-priority worker took longer than that, the refresh ran against pre-cleanup data and the tab kept showing rows that were already gone, behind a success toast. Add useTaskCompletion, which polls the job's own status until it reaches a terminal state, and refresh off that instead. Backs off from 400ms to 5s between polls, gives up after 5 minutes, and treats a job that can no longer be fetched as done since its result has aged out of Redis. It cancels on scope dispose so leaving the tab mid-wait can't refresh or raise an error over another page. Applies to the missing ROMs cleanup too, which has the same race. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Valid catch @greptileai, fixed in 872afb2. The refresh was on a fixed 1.5s delay, but Replaced the delay with a new
Polling rather than sockets is deliberate: there's no task-completion socket Worth noting the missing ROMs cleanup in Added 7 unit tests for the composable and 2 component tests (refreshes once the |
|
Clean fix. The A few minor observations worth noting:
The 7 composable tests cover the meaningful branches well, including the supersession and scope-dispose cases that are easy to miss. Looks good. |
Every failed status lookup counted as completion, so a timeout or a 5xx mid-cleanup refreshed the tab against a job that was still running. Only a 404 says the job is gone; anything else says nothing about it, so keep polling and let the existing deadline bound the wait. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Good catch on point 1 @greptileai — fixed in c594222. It also surfaced a mismatch between the comment and the code: the comment said a Now narrowed to a genuine 404 via Two tests pin it: a 503 followed by Points 2 and 3 need no change — the immediate first poll is already asserted by One correction to the read on the generation counter: it's right now, but only Frontend suite 643 passed / 50 files, typecheck and trunk clean. |
|
The fix is exactly right. The narrowed
The two pinning tests are well-chosen:
And the LGTM on c594222. |
Description
Fixes #4075
A scan flags firmware whose file has vanished with
missing_from_fs, but nothing acted on that flag. The result was three separate problems:firmware_countonPlatformSchemacounted rows, so a platform whose only BIOS file had been deleted still advertised firmware it could not serve.This PR makes the flag actionable end to end:
GET /api/firmwaregains a?missing=filter (true/false/ omitted for everything), so callers can select on the flag instead of taking the whole set and filtering client-side.cleanup_missing_firmwaremanual task, modelled oncleanup_missing_romsand registered next to it. Optionally scoped to oneplatform_id. It only deletes database rows — the files are already gone, so nothing is removed from disk.missing: false, and BIOS selection moved into a pure util so the "sole entry auto-select" rule can only ever fire on an entry that actually exists.firmware_countcounts only firmware present on disk, so the platform stat matches what is usable.Files changed
Backend
backend/handler/database/firmware_handler.pylist_firmware()gains amissing: bool | Nonefilter; stacks with the existingplatform_idand hidden-platform filtersbackend/endpoints/firmware.py?missing=query param onGET /api/firmware, forwarded to the handlerbackend/endpoints/responses/firmware.pyFirmwareSchemagainsplatform_idso a library-wide list can group by platform without a round trip per rowbackend/endpoints/responses/platform.pyfirmware_countexcludes entries flaggedmissing_from_fsbackend/tasks/manual/cleanup_missing_firmware.pyCleanupMissingFirmwareTask+ stats dataclass, mirroring the ROM cleanupbackend/endpoints/tasks.pycleanup_missing_firmwareinmanual_tasksFrontend
frontend/src/v2/components/Settings/MissingFirmwareSection.vuefrontend/src/v2/views/Settings/LibraryManagement.vuemissing-firmwaretab to theTabunion, nav, and bodyfrontend/src/v2/utils/playerFirmware.tsresolveInitialFirmware()— storage → core config → sole option, all restricted to firmware that existsfrontend/src/v2/views/Player/EmulatorJS.vuemissing: falseand delegates initial selection to the utilfrontend/src/v2/components/Gallery/FirmwareTab.vuefirmware_countnow matches what the server derivesfrontend/src/services/api/firmware.tsgetFirmware()forwards an optionalmissingparamfrontend/src/__generated__/models/FirmwareSchema.tsplatform_id)frontend/src/locales/*/settings.jsonTesting notes
Automated:
missingfilter (including that it stacks withplatform_idand with hidden platforms), 6 endpoint tests forGET /api/firmware, 5 task tests (deletes only flagged rows, scopes to one platform, counts delete failures, no-op stats), plusfirmware_counton the platform endpoint and a registry test asserting the task is actually reachable by name.resolveInitialFirmware(the key one: a sole entry whose file is gone resolves tonullrather than being auto-selected) and 7 component tests for the new tab.npm run typecheck,trunk fmt && trunk check,check_i18n_locales.py,check_i18n_sorted.pyall clean.Manual, against a dev library with firmware rows flagged missing:
platform_id; the list refreshes and empties.Things worth a reviewer's attention
firmware_countis a behaviour change to an existing field. Platforms holding stale firmware rows will report a lower count than before. That is what the issue asks for, but it is user-visible.FirmwareSchema.platform_idis an additive API field — the regenerated types are included in this PR.requireTyped: "DELETE". Per the v2 patterns guide, type-to-confirm is required when an action affects the filesystem; this one only drops rows pointing at files that are already gone, whereas the ROM cleanup additionally removes resource directories from disk. Happy to add the gate if you'd rather the two cleanups look identical.frontend/src/views/Player/EmulatorJS/Base.vue, a frozen v1 path thatcheck-v1-frozenblocks, so it is untouched here. It would need theallow-v1-changeslabel as a follow-up if you want it fixed before v1 is retired.cleanup_missing_roms; both are gated onScope.TASKS_RUN.platform_id. There is no index onmissing_from_fs— deliberately, at this table size.Checklist
AI assistance disclosure
This PR was written primarily by Claude Code (Opus 5), under my direction and review: issue analysis, tests, implementation, and translations. I reviewed the diff, and this PR description was also AI-generated and edited by me.
Screenshots (if applicable)