Move Free up card space to a dedicated /card-cleanup page - #1443
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The card-cleanup tool lived in a collapsed section on the import page, which is where nobody looks days after an import — and its enabling dependency (the integrity audit that stamps hash_status='ok') lived on yet another page, so a first run on a pre-existing archive dead-ended with everything kept for "run the integrity audit". - New page vireo/templates/card_cleanup.html at /card-cleanup, rendered expanded, with the whole scan → preview → delete flow moved over: same ids, same function names, same user-facing copy (confirmation dialog, incomplete-preview banner, summary states) byte for byte. It brings its own minimal single-select folder browser (same /api/browse and /api/volumes endpoints) and formatBytes rather than un-inlining the import page's. - Inline integrity-audit affordance: when kept entries carry the KEEP_NOT_VERIFIED reason, a callout above the buckets says how many files that is, explains why they can't be deleted, warns that verification re-reads the whole archive (slow over a VPN'd mount), and offers "Verify archive hashes" (the existing POST /api/jobs/verify-hashes job) with the page's normal progress rendering plus a "Re-scan card" follow-up on completion. - Import page keeps the card-safety pill's "Free up card space…" button; it now navigates to /card-cleanup?source=<first card folder> (with the remaining folders as `others=` so the multi-source hint survives). The moved markup, JS, CSS and the folder browser's third mode are gone; the browser is back to its pre-feature two-mode shape. - Navbar/registry entry "Card cleanup" (app.ALL_PAGES, db.ALL_NAV_IDS, _navbar.html's mirrored list), page route in web/pages.py, route contract snapshot regenerated. No backend or API changes to the card-cleanup endpoints. Tests: vireo/tests/test_card_cleanup_api.py gains a page-route test and an import-page test (section gone, pill link present). test_card_cleanup_api/test_card_cleanup/test_path_guard/ test_route_contract/test_tabs_api/tests/test_workspaces: pass. test_app.py: 423 passed, 1 pre-existing local failure (test_api_exiftool_status_reports_missing). node --check on both templates' inline JS: clean; no duplicate ids on either rendered page. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…upling
Review follow-ups to the /card-cleanup page move.
Important: the Delete button stayed live while a verify-hashes audit was
streaming. Clicking it hijacked the watch — cardCleanupWatchJob closed the
audit's EventSource and overwrote state.jobId, so cardCleanupFinishAudit
never ran: audit button stuck disabled, blank status, no "Re-scan card",
and the audit itself still running on the server with nothing on the page
tracking it. The same silent hole existed the other way round, where
"Scan card" and "Verify archive hashes" looked pressable during a run and
early-returned.
- New cardCleanupSetBusy(running, opts) owns the three start buttons for
the duration of any run (scan, delete, audit) and restores the Delete
button's prior enabled state and hint afterwards, since what Delete may
do depends on the manifest. During an audit the delete hint says why it
is unavailable ("Verification is running — deletion is available when it
finishes.") rather than greying out with no explanation.
- Defensive `if (cardCleanupState.jobId) return;` in
cardCleanupOpenConfirm, and cardCleanupConfirmDelete now closes the
dialog and reports "Another job is still running — nothing was deleted."
instead of stranding the running job.
- Pin the audit-callout coupling both ways: a comment on
card_cleanup.KEEP_NOT_VERIFIED explaining that card_cleanup.html matches
its tail, plus test_audit_callout_reason_stays_in_sync asserting the
served page carries the literal and the reason still contains it.
- Drop the dead `.card h3` rule (the page's card has no h3).
Tests: vireo/tests/test_card_cleanup_api.py + test_card_cleanup.py — 77
passed. test_route_contract + test_build_static pass. node --check on the
page's inline JS clean; ruff clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR moves card cleanup from the import page to ChangesCard cleanup workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ImportPage
participant CardCleanupPage
participant CleanupJobs
User->>ImportPage: complete card import
ImportPage->>CardCleanupPage: navigate with source and others
User->>CardCleanupPage: start scan
CardCleanupPage->>CleanupJobs: start and monitor scan
CleanupJobs-->>CardCleanupPage: return manifest preview
User->>CardCleanupPage: audit and confirm deletion
CardCleanupPage->>CleanupJobs: verify hashes and delete entries
CleanupJobs-->>CardCleanupPage: return audit and deletion results
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f41256d30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| document.getElementById('card-cleanup-audit-status').textContent = ''; | ||
| cardCleanupSetError(''); | ||
| try { | ||
| const resp = await fetch('/api/jobs/verify-hashes', { |
There was a problem hiding this comment.
When a card file matches an unverified catalog row belonging only to an inactive workspace, the scan still shows this audit callout because card_cleanup._load_catalog_by_hash() queries photos globally, but this endpoint captures only the active workspace and Database.get_integrity_photos() restricts verification to that workspace. Clicking Verify archive hashes therefore never verifies the archive copy that triggered the callout, so re-scanning leaves the file unchanged. Either scope cleanup candidates to the active workspace or run verification for every workspace represented by the pending entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in db739e2, taking the transparency route rather than either suggested restructuring: scoping cleanup candidates to the active workspace would contradict the parent spec (matching is deliberately global — photos are global), and fanning verification out across every represented workspace is a backend change out of this PR's scope. Instead the callout's cost line now states the audit covers only the current workspace's folders, and — once a verification has run — files still pending on the next scan get a specific hint (their archive copies were not covered; switch to the workspace holding those folders and verify there) instead of silently looping back to the same callout.
The card scan matches photos across all workspaces, but verify-hashes covers only the current workspace's folders — so the audit button could never verify some of the files the callout counts, and a re-scan would loop back to the same callout. The cost line now states the scope, and after a verification has run, files still pending on the next scan get a specific hint (other workspace's folders — switch there and verify) instead of a silent loop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db739e28e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| document.getElementById('card-cleanup-audit-text').textContent = | ||
| cardCleanupFileCount(pending) + | ||
| (pending === 1 ? ' is' : ' are') + | ||
| ' kept only because the archive ' + | ||
| 'copy has never been checksum-verified. Vireo will not delete a card ' + | ||
| 'file until its archive copy passes a checksum check, and the ' + | ||
| 'integrity audit is what performs that check.'; |
There was a problem hiding this comment.
When a card hash matches only catalog rows whose hash_status is modified, corrupt, or unreadable, qualify_rows() skips those rows but retains KEEP_NOT_VERIFIED, so this callout counts them too. Even in the active workspace, rerunning verification will reproduce the bad verdict rather than establish a verified copy, yet the UI says the copy has never been verified and later suggests the run merely missed it. Preserve the actual failed-verification reason or make the callout distinguish failed checks from rows that have never been checked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in ae456b8.
qualify_rows() now separates NULL hash_status from failed (modified / corrupt / unreadable) and returns a distinct KEEP_ARCHIVE_HASH_FAILED reason ("archive copy failed a prior integrity check — see the Audit page") when the only non-ok rows have already been checked. A mix of NULL + failed still routes to KEEP_NOT_VERIFIED, because a NULL row could turn ok on the next audit and unlock the file, so the verify remedy stays real for that case.
The page gains a distinct hash-failed callout (same chrome as the audit callout, no verify button) that surfaces the count and points users at the Audit page instead of pretending another verify pass would help. test_hash_failed_callout_reason_stays_in_sync pins the coupling between the manifest reason and the callout literal.
Card-cleanup suites: 83 passed / 1 skipped. Full CLAUDE.md suite: 2075 passed / 16 skipped / 1 pre-existing env failure (test_failed_import_does_not_chain — chmod 0 doesn't restrict root in this container; same as main).
Generated by Claude Code
Pre-existing coupling failure surfaced while running the routine's test suite after the maintainer's UI transparency fix (db739e2) addressed the Codex P2 finding on the audit-callout scope mismatch. The card_cleanup nav id was added to ALL_NAV_IDS earlier in this PR but the guard's expected set was not updated.
|
Codex P2 review addressed. Looks like you got there first with My initial pass took the other lever Codex suggested — scoping Pushed one small thing: the Test suite from Generated by Claude Code |
|
Acknowledged — nothing to push from this run. Sticking with the text-only fix in Test guard fix in Generated by Claude Code |
Codex flagged that qualify_rows() dropped rows whose hash_status was
'modified', 'corrupt', or 'unreadable' by continuing the loop but
leaving the default reason at KEEP_NOT_VERIFIED. Kept files backed
only by such rows then said "not verified — run the integrity audit"
in the manifest and, worse, added to the audit callout's count. But
running verification again just reproduces the same bad verdict; the
remedy for these files lives on the Audit page (accept / restore /
investigate), not in another verify run.
Fix: qualify_rows() now tracks NULL vs. failed hash_status separately
and falls back to a new KEEP_ARCHIVE_HASH_FAILED reason ("archive copy
failed a prior integrity check — see the Audit page") when the only
non-ok rows have already been checked. A mix of NULL + failed still
routes to KEEP_NOT_VERIFIED — a NULL row could turn "ok" on the next
audit and unlock the file, so the audit remedy is still real.
The card-cleanup page gains a distinct hash-failed callout (same
chrome as the audit callout, no button) that surfaces the count and
points users at the Audit page. Its coupling to the manifest reason
is pinned by test_hash_failed_callout_reason_stays_in_sync, mirroring
the existing audit-callout guard and asserting the two tail literals
do not overlap.
Also fix an unrelated pre-existing failure on this branch:
test_all_nav_ids_covers_every_page was missing 'card_cleanup' in its
expected set after the /card-cleanup page landed in 9c4ac52.
Test suite: 82 passed / 1 skipped in card-cleanup files (incl. three
new tests: modified/corrupt/unreadable each routing to the failed
reason, NULL+failed mix keeping the audit remedy, and the callout
sync guard). Full CLAUDE.md suite: 2074 passed / 1 pre-existing
machine-local exiftool failure unrelated to this diff.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae456b8823
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'would only reproduce that verdict — open the Audit page to accept ' + | ||
| 'the current bytes, restore from backup, or investigate.'; |
There was a problem hiding this comment.
When a card hash matches only a failed catalog row from an inactive workspace, this newly added callout sends the user to the Audit page, but _load_catalog_by_hash() scans photos globally while Database.get_integrity_flagged() filters to the active workspace. The flagged archive copy therefore will not appear on that Audit page, leaving the suggested accept/restore remedy unavailable. Include the same cross-workspace guidance used by the verification callout, or identify the workspace that owns the failed row.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1213fec, taking the same transparency route used for the verify-hashes callout in db739e2 rather than reshaping the scope of _load_catalog_by_hash / Database.get_integrity_flagged.
Rationale: scoping the card scan's catalog lookup to the active workspace would contradict the parent spec ("photos are global — matching is deliberately global"), and expanding get_integrity_flagged to cross workspaces is out of this PR's scope. Both would be design flips I shouldn't make unilaterally after the maintainer picked the transparency route in the previous round.
Fix: card-cleanup-hash-failed-scope span inside the hash-failed callout names the current-workspace scope of the Audit page and tells the user to switch workspaces if the flagged archive copy lives in another workspace's folders. test_hash_failed_callout_states_audit_workspace_scope pins the element id and the two literal phrases so a reword can't silently drop the guidance.
Generated by Claude Code
… P2) Third Codex P2 review on this PR points out the same workspace-scope asymmetry the verify-hashes callout already addresses: the card scan matches photos globally (_load_catalog_by_hash) but the Audit page's listing (Database.get_integrity_flagged) is filtered to the active workspace. A KEEP_ARCHIVE_HASH_FAILED entry backed only by a flagged row in an inactive workspace's folder would send the user to an empty Audit page — remedy unreachable. Same transparency route taken for the verify-hashes callout (db739e2): keep matching global (per the parent spec — photos are global), and be honest about the workspace scope of the suggested remediation. The hash-failed callout now names the current-workspace scope of the Audit page and directs the user to switch workspaces if the flagged copy lives elsewhere. Pinned in test_hash_failed_callout_states_audit_workspace_scope so a reword does not silently drop the guidance.
|
Addressed the third Codex P2 (workspace scope of the hash-failed callout's Audit remedy) with the same transparency route you took for the verify-hashes callout in
Rationale for choosing the text-only fix again (over scoping the catalog lookup or expanding the Audit query): matches the parent spec's "photos are global" design and the precedent you set in the previous round; both would be design flips I shouldn't make unilaterally. Tests: Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1213fec57d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'The verification is still running but this page lost track of it — ' + | ||
| 'check the Jobs page.'; | ||
| } | ||
| document.getElementById('card-cleanup-audit-rescan-btn').style.display = ''; |
There was a problem hiding this comment.
When a preview contains both deletable and unverified files, completing or cancelling Verify archive hashes restores the Delete button's pre-audit enabled state in cardCleanupSetBusy(false), while this handler only exposes the re-scan button. Because verification can change previously ok rows to modified/corrupt/unreadable, the old confirmation can then advertise stale file and byte totals; the backend rechecks and skips invalid rows, but it may still delete the remaining valid subset despite the user confirming a different set. Disable deletion and replace its hint here until the requested re-scan produces a current manifest.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 97ba478.
cardCleanupFinishAudit now disables the Delete button and replaces its hint with "Verification may have changed which files count as verified — re-scan the card before deleting." on every terminal outcome (completed, cancelled, failed, lost-track). The Re-scan button next to the audit status is the visible remedy; the next successful scan calls cardCleanupResetPreview → loadManifest, which re-enables Delete from the fresh totals.
This closes the window where cardCleanupSetBusy(false) at the top of cardCleanupFinishJob restored the Delete button's pre-verify enabled state — even though verification could have flipped previously-ok rows to modified / corrupt / unreadable and the old confirmation dialog would then advertise stale totals against a set the user never agreed to. Backend still re-checks and skips invalid rows on delete, so this is defence-in-depth for the confirmation dialog's honesty.
test_finish_audit_disables_delete_until_rescan pins the coupling: the served page's cardCleanupFinishAudit body must both disable the Delete button and carry the specific "re-scan the card before deleting" hint, so a reword can't quietly drop the guardrail.
Generated by Claude Code
Codex P2 on 1213fec: cardCleanupSetBusy(false) restores the Delete button's pre-verify enabled state at the top of cardCleanupFinishJob, but verification can flip previously-ok rows to modified/corrupt/ unreadable. The confirmation dialog would then advertise stale file and byte totals, and while the backend re-checks and skips invalid rows on delete, the user would still be confirming a set that no longer matches what would actually be deleted. cardCleanupFinishAudit now disables the Delete button and replaces its hint with a re-scan instruction on every terminal outcome (the re-scan button is already surfaced next to the audit status). The next successful scan calls cardCleanupResetPreview + loadManifest, which restores Delete based on the fresh totals — no other flow changes needed. test_finish_audit_disables_delete_until_rescan pins the coupling: the served page's cardCleanupFinishAudit body must both disable the Delete button and carry the specific "re-scan the card before deleting" hint, so a reword can't quietly drop the guardrail. Tests: CLAUDE.md suite — 2158 passed, 17 skipped, 1 failed. The one failure (test_failed_import_does_not_chain) also fails on main in this environment (chmod 0 does not restrict root, so the test cannot force an unreadable file); pre-existing, unrelated to this change.
|
Addressed Codex's P2 on 1213fec (Delete stays enabled after verify) in
Fix:
Tests: CLAUDE.md suite — 2158 passed, 17 skipped, 1 failed. The one failure ( Generated by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@vireo/templates/card_cleanup.html`:
- Around line 571-576: Update the error handling in cardCleanupState.es and the
related cardCleanupFinishJob flow so a transient SSE disconnect continues
polling while the job status remains running or queued. Do not permanently close
tracking or clear jobId during that state; only invoke the lost-track handling
when polling confirms the job has disappeared.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5acd31d1-c706-4346-bf53-9659cb0f37f3
📒 Files selected for processing (12)
docs/superpowers/specs/2026-08-08-card-cleanup-page-design.mdvireo/app.pyvireo/card_cleanup.pyvireo/db.pyvireo/templates/_navbar.htmlvireo/templates/card_cleanup.htmlvireo/templates/import.htmlvireo/tests/contracts/routes.txtvireo/tests/test_card_cleanup.pyvireo/tests/test_card_cleanup_api.pyvireo/tests/test_db.pyvireo/web/pages.py
The post-drop poll gave up after 30 seconds and re-enabled the buttons while the job kept running on the server — the exact orphaning the busy discipline exists to prevent, on a page whose scans and audits run for many minutes. Poll indefinitely (1s for 30 attempts, then 10s), render live progress from the polls, and stop only on a terminal status or several consecutive 404s (server restart pruned the job), which gets the honest lost-track copy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Routine fired on a review with an empty top-level body (the review's only inline entry was your own reply on the resolved SSE-polling thread, which CodeRabbit verified as fixed in All five prior review threads have landing commits — Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96137e3ba0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cardCleanupSetBusy(true); | ||
| cardCleanupWatchJob(data.job_id, 'delete', 'Starting deletion…'); |
There was a problem hiding this comment.
When a mixed preview exposes both Delete and Verify, a user can confirm deletion, dismiss the dialog while this POST is pending, and start verification or another scan because the page is not marked busy until after the response arrives. Both server jobs then run, and whichever response finishes later calls cardCleanupWatchJob, closes the other job's event stream, and overwrites jobId, leaving one job—including potentially the destructive deletion—without the correct progress, cancellation, or result UI. Set a pending/busy state before issuing the delete request and clear it if the request definitively fails.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 012aec2c.
cardCleanupConfirmDelete was issuing POST /api/card-cleanup/delete before cardCleanupSetBusy(true). Because the confirm dialog is dismissable while the request is in flight (backdrop click / Escape), Scan and Verify stayed live in that window — a second job could start and its watch would then overwrite the delete's jobId, orphaning the destructive path.
Fix: move cardCleanupSetBusy(true, { deleteHint: 'Deletion is starting…' }) to run before the fetch (mirroring cardCleanupStartScan / cardCleanupStartAudit). Both failure paths — !resp.ok and the network-error catch — now call cardCleanupSetBusy(false) so a definitive failure hands the buttons back instead of leaving a dead page. card_cleanup.html:1076–1108.
test_confirm_delete_locks_page_before_post pins the coupling: cardCleanupSetBusy(true must appear before the /api/card-cleanup/delete fetch inside the handler, and both failure paths must release the lock. Rewording is fine; re-opening the gap isn't.
Generated by Claude Code
…econd job (Codex P2) cardCleanupConfirmDelete previously issued POST /api/card-cleanup/delete before calling cardCleanupSetBusy(true). The confirm dialog can be dismissed (backdrop click or Escape) while the request is pending; in that gap Scan and Verify stayed live, so a user could kick off a second job whose watch would overwrite the delete's jobId — including the destructive path. Move the busy call to run before the fetch (mirroring cardCleanupStartScan / cardCleanupStartAudit), and add cardCleanupSetBusy(false) on both the !resp.ok branch and the catch so a definitive failure hands the buttons back instead of leaving a dead page. test_confirm_delete_locks_page_before_post pins the coupling: setBusy must appear before the /api/card-cleanup/delete fetch, and both failure paths must release the lock. Rewording is fine; re-opening the gap isn't.
|
Addressed Codex's latest P2 (commit The gap: Fix: move
Tests: Generated by Claude Code |
|
Re-read every outstanding review and comment per your
Nothing was missed and no new code changes were needed on this pass — the fixes were all in place, but the middle three Codex threads had no on-thread response yet, so the reviewer would have had to scroll the PR conversation to see where each landed. That's fixed now. Tests re-run to confirm state: CLAUDE.md suite from Generated by Claude Code |
|
Duplicate Re-verified on this pass anyway, since the fire is explicit:
Skipping a suite rerun since the branch head hasn't moved since the previous run's Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 012aec2c51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (e) { | ||
| cardCleanupCloseConfirm(); | ||
| cardCleanupSetBusy(false); |
There was a problem hiding this comment.
When the server accepts this POST and queues the deletion but the connection drops before its response arrives, fetch rejects here even though the destructive job is still running. This unconditional unlock leaves jobId unset and re-enables Scan and Verify, allowing another job to overwrite the page state while the deletion proceeds unseen. The newly added catch path is fresh evidence beyond the previously reported pending-POST race; only unlock after an HTTP response proves no job was queued, and treat network errors as an unknown-running state that directs the user to Jobs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 5afcb0b.
The catch block on the delete POST no longer calls cardCleanupSetBusy(false). fetch rejects for any network-layer failure, including the case where the server accepted the POST and queued the destructive job before the connection dropped — unlocking there was the exact orphan the single-busy-state owner exists to prevent, and it's especially wrong for the destructive path because a second job's watch would then overwrite jobId while the deletion ran unseen.
New behavior:
!resp.okbranch (unchanged): the server proved nothing was queued, so it still hands the buttons back.catch (e)branch: leaves Scan / Verify / Delete disabled and shows an unknown-outcome error telling the user to check the Jobs page for a delete queued around now, then reload this page to work with the card again. A page reload is the honest reset — it rebuilds the busy state from a fresh manifest once the user has confirmed what actually happened on the server.
test_confirm_delete_locks_page_before_post was updated to pin both invariants explicitly instead of a global >= 2 count:
- The
!resp.oksub-branch must callcardCleanupSetBusy(false). - The
catch (e)sub-branch must not callcardCleanupSetBusy(false), and its body must mention the Jobs page.
The >= 2 shape would have kept passing if a future refactor slid the release back into the catch, so the split-per-branch assertion is what actually pins the Codex P2 guarantee.
Card-cleanup API suite: 22 passed / 1 skipped. CLAUDE.md test suite: 2075 passed / 16 skipped / 1 pre-existing env failure (test_failed_import_does_not_chain — chmod 0 doesn't restrict root in this container; also fails on main).
Generated by Claude Code
…Codex P2) Codex P2 review on commit 012aec2 (card-cleanup delete flow): the newly added catch block on the POST /api/card-cleanup/delete request called cardCleanupSetBusy(false) unconditionally. `fetch` rejects for any network failure, including the case where the server accepted the POST and queued the destructive delete before the connection dropped. Unlocking there re-enables Scan and Verify while the deletion runs unseen server-side, so a second job's watch can overwrite this page's jobId — the exact orphan the single-busy-state owner exists to prevent. Change: the catch no longer unlocks. It leaves Scan / Verify / Delete disabled and shows an unknown-outcome error that tells the user to check the Jobs page for a delete queued around now, then reload this page to work with the card again. A page reload is the honest reset: it rebuilds the busy state from a fresh manifest once the user has confirmed what actually happened on the server. The !resp.ok branch is unchanged — an HTTP response proves nothing was queued, so it can still hand the buttons back. test_confirm_delete_locks_page_before_post now pins both invariants: - !resp.ok branch: cardCleanupSetBusy(false) must appear (unlock). - catch branch: cardCleanupSetBusy(false) must NOT appear, and the body must mention the Jobs page. Full card_cleanup_api suite: 22 passed / 1 skipped. CLAUDE.md test suite: 2075 passed / 16 skipped / 1 pre-existing env failure (test_failed_import_does_not_chain — chmod 0 doesn't restrict root in this container; also fails on main). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EES6embmjg1M1aY3zATMaM
|
Routine fired on jss367's empty review at 11:48:56 UTC (commit A parallel routine got there first: I independently produced the same fix locally before fetch surfaced the parallel commit, verified Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5afcb0ba52
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cardCleanupWatchJob(data.job_id, 'audit', 'Starting hash verification…'); | ||
| } catch (e) { | ||
| // Nothing started, so nothing is running: give the buttons back. | ||
| cardCleanupSetBusy(false); |
There was a problem hiding this comment.
When POST /api/jobs/verify-hashes reaches the server but its response is lost or cannot be parsed, this catch restores the previously enabled Delete button even though api_job_verify_hashes() may already have started verification. The user can then launch deletion concurrently; delete_verified() trusts the currently committed hash_status plus archive size/mtime rather than re-hashing archive bytes, so it can qualify a row on its old ok verdict while the audit is detecting same-metadata corruption and delete the good card copy. Unlock only after a definitive HTTP failure proves no job was queued; treat network/parsing failures as potentially running and direct the user to Jobs, as the delete-start path already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 16a3f8e.
cardCleanupStartAudit was catching every failure — network error, !resp.ok, JSON parse — through one arm that called cardCleanupSetBusy(false). That's the same gap the delete-start fix in 5afcb0b closed on the destructive path: only an HTTP response proves the server didn't queue the job. A fetch rejection or an unreadable 2xx body could mean api_job_verify_hashes() is already running, and unlocking there is dangerous because delete_verified trusts qualify_rows(fetch_rows_by_hash(...)) — i.e. the currently committed hash_status plus archive size/mtime — rather than re-hashing archive bytes, so a concurrent delete could qualify a row on its old ok verdict while the audit is flipping it to modified/corrupt/unreadable and remove the good card copy.
Fix (mirrors the confirm-delete pattern):
- Fetch-rejection catch: keeps Scan/Verify/Delete disabled, tells the user to check the Jobs page for a verify-hashes job that started around now, then reload this page to work with the card again.
!resp.okbranch: the server proved nothing was queued — hand the buttons back before showing the error. The error message still comes from the response body (best-effort JSON parse, default text otherwise) so a definitive failure isn't swallowed.- JSON-parse catch on a 2xx response: same reasoning as the fetch catch — the server returned OK, so the audit may already be running; keep locked, route to Jobs.
test_start_audit_keeps_page_locked_on_ambiguous_start pins all three branches explicitly (not a global count), so a future refactor can't quietly re-open any single gap: !resp.ok must unlock; both catches must not unlock and must mention the Jobs page.
Tests re-run against the branch: test_card_cleanup_api.py — 23 passed / 1 skipped (unchanged). Focused subset (test_card_cleanup*, test_app, test_config) — 586 passed / 3 skipped, no regressions.
Generated by Claude Code
Codex flagged that `cardCleanupStartAudit` released the page-wide busy lock in its catch on `POST /api/jobs/verify-hashes`, even though a `fetch` rejection (or an unreadable 2xx body) can't prove the server didn't already queue `api_job_verify_hashes()`. If the audit is silently running while Scan/Verify/Delete come back live, `delete_verified` trusts the currently committed `hash_status` plus archive size/mtime rather than re-hashing archive bytes — so a concurrent delete could qualify a row on its old `ok` verdict while the audit is flipping it to modified/corrupt/unreadable, and remove the good card copy of a file whose archive copy is silently rotting. Mirror the delete-start pattern from commit 5afcb0b: - Split the try/catch so the fetch failure is caught separately, keeps the page locked, and directs the user to the Jobs page with a reload-to-recover message. - Handle `!resp.ok` inline: the server proved nothing was queued, so hand the buttons back before showing the error. - Handle the JSON parse of a 2xx response with its own catch: the server returned OK, so the audit may already be running — keep locked, route to Jobs (defence-in-depth for an unlikely path). `test_start_audit_keeps_page_locked_on_ambiguous_start` pins the coupling: the !resp.ok branch must unlock, the fetch-rejection catch and the JSON-parse catch must not unlock and must mention the Jobs page. A refactor that quietly re-opens any of the three gaps fails. Tests: vireo/tests/test_card_cleanup_api.py — 23 passed, 1 skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K1RkzznNE4qYa9wfFwepBo
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16a3f8e4d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cardCleanupSetBusy(false); | ||
| const status = job && job.status; | ||
| const jobError = job && (job.errors || [])[0]; | ||
| if (kind === 'scan') { | ||
| if (status === 'completed') { | ||
| await cardCleanupLoadManifest(); |
There was a problem hiding this comment.
When a completed scan is still awaiting cardCleanupLoadManifest(), this unlocks Scan and clears jobId first. If the user changes the source and starts another scan during that request, scanJobId points to the second scan, but the first request can subsequently render its old manifest and re-enable Delete even while the second scan runs; if the second manifest becomes available before the click, the confirmation displays the first scan's source and totals while the delete POST operates on the second scan. Keep the first scan's ownership/busy state through manifest loading, or discard manifest responses whose job ID is no longer current.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 859214c.
The race: cardCleanupFinishJob unlocks the buttons and clears jobId before awaiting cardCleanupLoadManifest() for scan #1. During that await, a user who changes the source and clicks Scan again sets cardCleanupState.scanJobId = jobId2 (and re-locks the page via cardCleanupSetBusy(true)). When scan #1's response then arrives, cardCleanupRenderManifest renders scan #1's totals and source over scan #2's state and, worse, re-enables the Delete button based on scan #1's deletable.count — the confirmation dialog would then advertise scan #1's numbers while the delete POST (which uses cardCleanupState.scanJobId, now jobId2) would operate on a different set the user never confirmed. Two secondary paths have the same problem: the 404 handler nulls scanJobId (clobbering scan #2's identity), and the error banner would misattribute scan #1's failure to scan #2.
Fix (took the discard-stale-response lever rather than moving cardCleanupSetBusy(false) inside the manifest branch, so the timing of button re-enable stays as it is — the target invariant is "no stale render/mutation/error," not "buttons stay locked longer"): capture scanJobId into a local at the top of cardCleanupLoadManifest, then guard both the success and error paths with if (cardCleanupState.scanJobId !== requestedScanJobId) return;. On the success path the guard runs immediately after resp.json() — before the 404 branch, the !resp.ok throw, and cardCleanupRenderManifest — so a stale response touches nothing. On the catch path the guard drops fetch and JSON-parse errors that belong to scan #1 once scan #2 has taken over. card_cleanup.html:688–716.
test_load_manifest_discards_stale_scan_response pins both branches explicitly rather than a count: the success guard must appear before min(idx(scanJobId = null), idx(cardCleanupRenderManifest)), and the catch body must also contain a state.scanJobId !== comparison — so a refactor can't quietly re-open either.
Card-cleanup API suite: 24 passed / 1 skipped (existing skip).
Generated by Claude Code
Codex P2 review on commit 16a3f8e flagged a race in cardCleanupLoadManifest: cardCleanupFinishJob unlocks the buttons and clears jobId BEFORE awaiting the manifest fetch, so a user who changes the source and starts scan #2 during that await sets scanJobId to jobId2. When scan #1's response then arrives, cardCleanupRenderManifest renders scan #1's totals and source over scan #2's state — and, worse, re-enables the Delete button based on scan #1's deletable count. The confirmation dialog would advertise scan #1's numbers while the delete POST (which uses cardCleanupState.scanJobId, now jobId2) would operate on a different set the user never agreed to. Fix: capture scanJobId at request time and drop the response on both the success and error paths when the state has moved on by the time the response arrives. This also protects the 404-clears-scanJobId path from clobbering scan #2's identity, and prevents scan #1's fetch/parse error from surfacing as if scan #2 had failed. test_load_manifest_discards_stale_scan_response pins the coupling for both branches (success-path guard must run before ANY state mutation or render; catch guard must be present) so a refactor can't quietly re-open either. Tests: test_card_cleanup_api.py — 24 passed / 1 skipped (existing skip). Full CLAUDE.md suite green apart from the pre-existing environmental failure (test_failed_import_does_not_chain — chmod 0 doesn't restrict root in this container; also fails on main).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
vireo/tests/test_card_cleanup_api.py (1)
585-589: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a marker helper so template refactors fail with a readable message.
Line 588 bounds the
!resp.okarm at"let data;". Line 601 showsrespis assigned rather than declared at the fetch, solet resp;is hoisted above it. If a later edit hoistslet data;the same way,section.index("let data;", ok_branch_start)raisesValueErrorinstead of failing the assertion, and the report gives no hint about which marker moved.A shared helper keeps the same invariants and names the missing marker.
♻️ Proposed helper for marker lookup
def _marker(section, marker, start=0, *, what): idx = section.find(marker, start) assert idx != -1, f"expected {marker!r} in {what}; the template changed" return idx- ok_branch_start = section.index("if (!resp.ok)") + ok_branch_start = _marker(section, "if (!resp.ok)", + what="cardCleanupStartAudit") # The branch closes before the JSON re-parse below; scope to just that # arm by ending at the trailing "return;" and its closing brace. - ok_branch_end = section.index("let data;", ok_branch_start) + ok_branch_end = _marker(section, "let data;", ok_branch_start, + what="cardCleanupStartAudit")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vireo/tests/test_card_cleanup_api.py` around lines 585 - 589, In the test marker-scanning logic around the !resp.ok branch, add and reuse a shared _marker helper that uses find, asserts a missing marker with a descriptive template-change message, and returns its index. Replace direct section.index calls such as the "let data;" boundary lookup (and other nearby marker lookups) with this helper while preserving the existing search offsets and branch-scoping invariants.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@vireo/tests/test_card_cleanup_api.py`:
- Around line 585-589: In the test marker-scanning logic around the !resp.ok
branch, add and reuse a shared _marker helper that uses find, asserts a missing
marker with a descriptive template-change message, and returns its index.
Replace direct section.index calls such as the "let data;" boundary lookup (and
other nearby marker lookups) with this helper while preserving the existing
search offsets and branch-scoping invariants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2677897b-a6f6-47d1-a632-360ab21be8d0
📒 Files selected for processing (2)
vireo/templates/card_cleanup.htmlvireo/tests/test_card_cleanup_api.py
2 participants
What
Implements docs/superpowers/specs/2026-08-08-card-cleanup-page-design.md,
the relocation follow-up to #1436.
/card-cleanup(navbar entry) holding the entirescan → preview → delete flow, moved from the import page with all
user-facing copy byte-identical (confirmation dialog, incomplete-preview
banner, summary states — verified by scripted comparison).
a checksummed import" reason, the page shows the exact count, explains
the checksum gate, offers Verify archive hashes (the existing
verify-hashes job, with progress and an honest result summary), warns
about the SMB/VPN cost, and offers Re-scan card on completion.
This closes the first-run dead end observed in production (5,523/5,523
files kept with that reason and no path forward from the page).
pre-feature form; the card-safety-pill button now deep-links to
/card-cleanup?source=<path>(plusothers=so the multi-source hintsurvives verbatim).
scan/delete/verify buttons during any run (the Delete hint says why),
preventing a mid-audit delete from hijacking the job watch.
own minimal single-select folder browser against the same /api/browse
and /api/volumes endpoints (deliberate duplication per the spec).
Tests
Feature suites: 88 passed (incl. new page-route, import-page-trim, and
audit-callout coupling tests). test_app.py + tests/test_workspaces.py:
558 passed, 1 failed — the known pre-existing machine-local exiftool
test. Route-contract snapshot regenerated (+1 line). node --check clean
on both changed templates; no duplicate ids on either rendered page;
ruff clean. Manual visual QA (both themes) pending, as with #1436.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes