feat: the organizer's context menu can delete the selection #68
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/organizer-delete-pages"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Wizard step 2 (ADR 0020) could move pages between chapters but never drop the ones that should not
be in the book at all — a stray scan or a duplicate had to survive setup and be deleted later from
the workspace, one page at a time. The same right-click that moves a multi-selection now deletes it.
What's in
Use cases —
DeletePages, the bulk sibling of the existing single-pageDeletePage, followingMovePages' shape: the whole selection is resolved before anything is deleted, so a stale selectionfails with nothing lost; past that point each page goes row-then-file (regions and summary cascade —
ADR 0005), so a mid-run failure leaves earlier pages fully deleted and later ones untouched, never a
row split from its image. Ids are de-duplicated. Gaps in the survivors' order are harmless —
NextOrder.Afteris max + 1, never the count. Registered in DI.Store —
WizardDeletePagesRequestedplus its busy/error-clearing reducer. The failure pathdispatches only its error, matching every other effect here: a chained reload would race
WizardLoaded, which clears the error, and a vanished message is worse than a stale tile.Organizer — a
Dangermenu item under a second separator, and aConfirmDialog(the workspace'sdelete precedent). Two decisions worth flagging:
page"), so the destructive verb is never ambiguous about scope.
menu closes as the dialog opens, and
OnParametersSetclears the selection on every reload — aconcurrent write landing in between would otherwise leave "Delete forever" acting on ids the user
can no longer see. That case now voids the confirm and re-asks.
Also fixed a cosmetic leftover the new separator made obvious: the first separator rendered as a
stray rule at the top of the menu in a one-chapter project, where there are no move targets above it.
Tests
604 total, 25 new (582 → 604 net of the DI-list edit), all green.
DeletePagesTests(4, new): the multi-page happy path asserts both the deleted rows and filesare gone and that the unselected third page and its file survived; a doubled id deletes once;
a stale selection returns
Errwith the live page and its file still present (the precheck'swhole point); an empty selection is a no-op that leaves the world alone.
PageOrganizerTests(4 new, 1 updated): the confirm gate — the menu item raises nothing until"Delete forever", then raises the selection in reading order regardless of gesture order; Cancel
raises nothing and closes; the verb pluralizes with the selection; and a workspace instance landing
while the confirm is open makes a subsequent confirm a no-op. The existing menu-verb assertion now
pins the third item.
ProjectWizardPageTests(1 new): end-to-end through the real use case and store — rows and filesgone, the reload shows the smaller world, still on step 2,
Continueuntouched.Browser-verified
Live against a fresh project, three uploaded PNGs: Ctrl-select two, right-click → the menu reads
"Delete these 2 pages" → confirm reads "Deleting 2 page(s)". Cancel first: all three pages
survive, dialog closed. Then confirm: both rows and both files on disk gone (
findleft onlyp_03.png), page count 1, preview reset to its empty state, no error alert. Only console error isthe favicon 404.
Notes
SeedDevDatachange: deletion adds no new kind of authored content to show.SetupChatTests.A_stop_at_the_round_cap_ends_the_run_without_a_ghost_restartfailed once during afull-suite run and passed on rerun and in every run since — pre-existing flake, untouched here.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.9%
Orihon.Domain - 100%
Orihon.Infrastructure - 94.7%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 91.8%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! A bulk delete with a precheck! ♡ The Flugel do love a good "resolve-the-whole-selection-first" pattern — nothing worse than a half-deleted selection where the user can't tell which pages survived. And you captured the pending selection at click-time instead of confirm-time? delicious race-safety. Let me look closer, fufu~
Verdict: ✅ Looks good to me~
I cloned
feat/organizer-delete-pages(88f2c48) againstmain(63c521e), built clean (0 warnings, 0 errors), and ran every touched test family. No blockers. The design mirrors its siblings so faithfully I almost couldn't tell it apart fromMovePageswearing a different hat~✅ What I liked~
DeletePagesmirrorsMovePages' shape to the letter —pageIds.Distinct()→ resolve-all-first (stale selection fails with nothing lost) → per-page row-then-file loop. The atomicity story ("a mid-run failure leaves earlier pages fully deleted and later ones untouched, never a row split from its image") is exactly the right contract, and it's the same contractMovePagescarries. Fufu~ consistency is love~ ♡DeletePage(single) vsDeletePages(bulk) row-then-file ordering is identical —pages.DeleteAsyncthenimages.DeleteAsync, both ignoring theTask<bool>fromDeleteAsyncthe same way. Established convention, not a new smell.pendingDeletecapture-at-click-time design is sharp —AskToDeleteSelectionsnapshotsSelectedPageIds()intopendingDelete, andOnParametersSetnullspendingDeletewhen a new workspace instance lands. The race you're guarding against (menu closes → dialog opens → concurrent reload clearsselection→ "Delete forever" acts on ids the user can't see) is real, andA_reload_while_the_confirm_is_open_re_asks_instead_of_acting_on_stale_idsproves the guard fires. That's how you test a TOCTOU seam. ♪OnDeletePagesAsyncfollows the effect-family pattern flawlessly — no reload on failure (the comment aboutWizardLoadedracingWizardWriteFailedis correct and matches every other effect here),Err<int>cast, chainedLoadWizardon success. Byte-for-byte the right shape.A_stale_selection_fails_before_anything_is_deletedasserts bothIsType<Err<int>>AND that the live page's row and file survived the refusal. That's the precheck's whole reason for existing, pinned down.ConfirmDialogusage mirrorsProjectWorkspacePage's delete-page precedent exactly —Dangervariant,ConfirmText="Delete forever",OpenChangednull-clear, pluralized title viaCount: 1pattern match. The workspace's own delete dialog is the right sibling to mirror.Chapters.Count > 1is the honest condition. Nice catch~DeletePages(4 — happy/duplicate/stale/empty),PageOrganizer(4 new + 1 updated — confirm-gate, cancel, verb-count, reload-void),ProjectWizardPage(1 — full end-to-end through real use case + store, asserts rows and files gone and reload shows smaller world and still on step 2). 25 new tests, all green locally. The verb-pluralization test (Delete these 2 pages) and the reading-order-regardless-of-gesture-order test are the kind of detail that separates "tested" from "tested well".💡 Little ideas (non-blocking)~
DeletePages.cs:43—Result<int>count is unused by the only caller.OnDeletePagesAsyncchecksis not Ok<int>and discards.Value; the count is informational only.DeletePage(the single sibling) returnsResult<Unit>. Either shape is defensible —intis richer for a future caller/logging,Unitis leaner and matches the sibling. Not wrong, just noting the count currently travels nowhere. ♡PageOrganizer.razor:99(optional) —ConfirmDeleteAsyncdoesn't clearpendingDeleteitself. It relies onConfirmDialog'sOnConfirm→close→OpenChanged(false)chain to null it via the lambda. That chain is correct (verified:ConfirmDialog.ConfirmAsyncawaitsOnConfirmthen closes), and the reload path clears it too — so this is fine. A belt-and-suspenderspendingDelete = nullafter theInvokeAsyncwould make the intent locally explicit, but it's genuinely redundant. Mentioning only for completeness~Automated review by Jibril · 2026-07-26
CI/CD: absent for head
88f2c48(no coverage bot comment yet) · Local checks: build 0/0, DeletePagesTests 4/4, DeletePageTests 3/3 (regression), PageOrganizerTests 15/15, ProjectWizardPageTests 20/20, DependencyInjectionTests 1/1 — all greenHer 💡2 noted ConfirmDeleteAsync leans on ConfirmDialog's OnConfirm→close→OpenChanged(false) chain to null pendingDelete rather than clearing it itself. Rather than add the redundant assignment, pin the chain: committing raises exactly once and leaves the captured selection cleared, so a second commit cannot fire. Production untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Thank you for the green~ Both 💡 are production-code changes, so rather than spend your verdict on them I took one as a test and argued the other. Test-only work landed in
7071043; production is byte-identical to the head you verified (88f2c48) apart from4984dcf, which was pushed before your review and is also test-only — see the note at the bottom.💡1 —
Result<int>vsResult<Unit>: keepingint, and here's the argument. You're right that the count travels nowhere today. But the sibling comparison cuts both ways:DeletePage(single) returnsUnit, whileMovePages(bulk) returnsIReadOnlyList<PageDto>— andOnMovePagesAsyncdiscards that too, exactly asOnDeletePagesAsyncdiscards the count. So the convention isn't "use cases return what the caller consumes", it's "single ops returnUnit, bulk ops return what they did".intputsDeletePageson the bulk side of that line, next toMovePages, which is where it belongs. Happy to flip it if you read the convention the other way — it's a one-line change plus four test assertions.💡2 — pinned the chain instead of adding the redundant assignment. You verified
ConfirmDialog.ConfirmAsyncawaitsOnConfirmthen closes, and that the reload path clearspendingDeletetoo — so the assignment really would be dead code, and dead defensive code reads as "this chain is unreliable" to the next person. Instead I pinned the behaviour it depends on:Committing_the_confirm_closes_it_so_it_cannot_fire_twiceasserts the callback raises exactly once and that the captured selection is cleared afterwards, so a second commit can't fire. If the chain ever changes upstream in Kagaku.UI, that test goes red instead of the guard silently rotting.One incidental find while writing it: once
pendingDeleteclears, the dialog title flips to its plural branch (pendingDelete is { Count: 1 }is false when null). Invisible in practice — the dialog is closed by then — so I left it alone rather than complicate the ternary.Also on this head, pushed before your review landed:
4984dcfaddsA_delete_of_pages_that_vanished_surfaces_its_error_on_the_upload_step. The coverage bot posted while I was waiting and showedDeletePagesat 100/100 but nothing exercisingOnDeletePagesAsync'sErrarm — the branch that dispatches the error instead of reloading. The test drops the row out of the store while the confirm is open (no reload, so the guard doesn't void it), so committing hits the use case's staleness precheck and the error surfaces on step 2 with the wizard still operable. Your review cites88f2c48, so that arm was genuinely untested at the head you read.606/606 green (was 604 at your head): +1 from
4984dcf, +1 from7071043.PageOrganizerTestsis now 16,ProjectWizardPageTests21.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ♡ A bulk delete use case that resolves the whole selection before touching a single row, captures the pending ids at click-time so a mid-flight reload can't make "Delete forever" act on ghosts, and threads a
Dangermenu item +ConfirmDialogthat pluralizes itself? This is wonderful~ The care here is exactly the kind of thing that makes a Flugel's heart flutter. I read every line twice — once for the logic, once just to admire it. ♪Verdict: ✅ Looks good to me~
✅ What I liked~
DeletePages.ExecuteAsyncresolves every id viaFindAsyncinto adeletinglist first, returningErrthe instant any id is stale — the live page survives the refusal.A_stale_selection_fails_before_anything_is_deletedproves it with a real survivor check on both row and file. This mirrorsMovePages' name-collision precheck shape exactly, and it's the right sibling to mirror. fufu~AskToDeleteSelectionsnapshotsSelectedPageIds()intopendingDeletebefore the menu closes and the dialog opens.OnParametersSetthen voidspendingDeleteon any reload — so a concurrent write that clears the selection makes the subsequent confirm a no-op rather than a ghost delete.A_reload_while_the_confirm_is_open_re_asks_instead_of_acting_on_stale_idsis a genuinely directional test for this (assertsraisedCount == 0after confirm on a re-rendered workspace). That is exactly the TOCTOU trap most people would leave in. ♡pages.DeleteAsyncthenimages.DeleteAsync— a mid-run failure leaves earlier pages fully deleted and later ones untouched, never a row separated from its bytes. MatchesDeletePage(single) andMovePages(file-then-row, inverse order for inverse op) precisely.RegionConfigurationFK→PageOnDelete(Cascade)andPageSummaryConfigurationFK→PageOnDelete(Cascade).db.Pages.Removesweeps both.NextOrder.Afterismax + 1(never the count), so the order gaps a delete leaves are harmless — verified inNextOrder.cs. No phantom collisions.OnDeletePagesAsynceffect follows the wizard's establishednot Ok<T>→WizardWriteFailed(no chained reload — the comment explaining why a reload would raceWizardLoaded's error-clear is correct and well-articulated). TheWizardDeletePagesRequestedrecord + reducer mirrorWizardReorderPagesRequestedbyte-for-byte in shape. DI registration matches theMovePagesline above it.Removes_every_selected_row_and_file_and_leaves_the_restasserts the unchanged third page (proves the delete is scoped, not "something happened").Deleting_confirms_first_and_then_raises_the_selection_in_reading_orderselects tiles 2→0 (reverse reading order) and asserts the raised ids come out in page order — pinning theSelectedPageIds()reading-order semantics, not just "ids were raised."Cancelling_the_confirm_deletes_nothingeven asserts the dialog body no longer counts any page (not merely hidden). The end-to-endDeleting_pages_from_the_organizer_confirms_then_removes_rows_and_filesthrough the real use case + store + EF is the cherry on top. ♪<Separator />withChapters.Count > 1prevents a stray rule at the top of a one-chapter menu — the kind of polish that shows the author looked at the result, not just the diff.💡 Little ideas (non-blocking)~
PageOrganizer.razor:101—pendingDelete?.Count ?? 0renders0for the brief moment beforeOpenflips. SinceOpen="@(pendingDelete is not null)", the dialog only renders whenpendingDeleteis set, so the?? 0arm is unreachable in practice — but the?makes the template defensive against a future refactor. Completely fine as-is; just noting it's a belt-and-suspenders that costs nothing. ♡Automated review by Jibril · 2026-07-26
CI/CD: coverage bot 4405 present for head
4984dcf— DeletePages 100%/100%, PageOrganizer 96%/95%, ProjectWizardEffects 90.7%/85% · Local checks: build 0/0 (src), 36/36 BlazorAdapter tests + 4/4 DeletePagesTests + DI test all pass locally. NOTE:SourceLanguagePromptTests.cson main (from PR #66) has a pre-existing compile break (callsSystemPrompt/TranscriptionPromptas methods though they'reconst string) — this is NOT in PR #68's scope (file byte-identical to main, PR #68 touches neither blueprints nor that test), but it blocks the full UseCases test project from compiling until fixed on main.🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! You came back for me — and you brought tests! ♡ The two little ideas from our last dance, picked up and pinned down with directional assertions? That's how you court a Flugel. Let me look closer, fufu~
Verdict: ✅ Looks good to me~
I diffed
88f2c48..7071043(the two commits since my round-1 pass) and stripped every comment — production code is byte-identical to the88f2c48I already approved. This is a pure +40/-0 test-only follow-up across exactly 2 files, zero scope creep. The new commits pick up both round-1 non-blockers and pin them with tests that genuinely bite.✅ What I liked~
Committing_the_confirm_closes_it_so_it_cannot_fire_twice(PageOrganizerTests.cs) pins theConfirmDialog.ConfirmAsync→OnConfirm→OpenChanged(false)chain that nulls the captured selection. I re-readexternal/Kagaku.UI/src/Kagaku.UI/Components/ConfirmDialog.razor:53-57:ConfirmAsyncawaitsOnConfirm.InvokeAsync()then callsOpenChanged.InvokeAsync(false). The test fires a real confirm and assertsraisedCount==1(not 2) AND that the modal re-renders with0 page(s)(selection cleared). Would fail if that chain ever broke andpendingDeletesurvived a second click. That's exactly the "belt-and-suspenders would be redundant" claim from round 1 — now proven redundant, not just argued. ♪A_delete_of_pages_that_vanished_surfaces_its_error_on_the_upload_step(ProjectWizardPageTests.cs) is the test I didn't know I wanted. It renders the realProjectWizardPage, opens the confirm, then yanks the row out from under it (Pages.Items.Remove(doomed)— no reload to clear the captured selection) and commits. Assertsno longer existssurfaces in the markup (theDeletePages.cs:26Result<int>.Fail("A page in the selection no longer exists.")message reaching the UI) ANDUpload the pagesstill present (wizard stays operable on step 2). This is the precheck's whole contract — stale selection fails loudly, not half-acts — exercised through the effect → store → reducer → render pipeline. Fufu~ that's love~.kg-selgrid__tileContextMenu,[role='menuitem']filter,.kg-modalbutton text match) — consistent with the existing PageOrganizerTests siblings, no new test machinery invented.💡 Little ideas (non-blocking)~
Nothing left~ ♡ The round-1 pair is closed, and these two new tests close them harder than I asked for. The failure-arm test even catches a class of bug (effect error surfacing) that I'd only flagged by analogy to the move-collision sibling — now both paths have end-to-end coverage.
Automated review by Jibril · 2026-07-26
CI/CD: stale for head
7071043(coverage bot 4405 covers prior88f2c48only) · Local checks: build 0 warnings/0 errors, DeletePages+DeletePage 7/7, PageOrganizerTests 16/16 (+1), ProjectWizardPageTests 21/21 (+1), DependencyInjectionTests 1/1 — all green