feat: page organizer 2/3 — bulk MovePages and page thumbnails #51
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/page-organizer-usecases"
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?
Slice 2 of the wizard step-2 redo (multi-select page organizer; slice 1 is Kagaku.UI PR #7, slice 3 brings the UI): the use-case and host layer the organizer will call. No UI changes — the wizard works exactly as today, so this ships green on its own with no submodule bump.
What's in
MovePages(UseCases/Pages) — the multi-select move: a selection lands at the end of the target chapter in selection order. Name collisions are prechecked all-or-nothing before anything moves — against the target'sraw/folder and between the incoming pages themselves (two chapters can each hold ap_01.png; both arriving at once would collide on arrival). The precheck needed a new port method,IPageImageStore.ExistsAsync, implemented inFileSystemPageImageStoreand the shared fake. After the precheck each page moves file-then-row (MovePage's pattern), so a mid-run filesystem race leaves earlier pages fully moved and later ones untouched — a row and its image are never split. Pages already in the target are skipped (their names never reach the precheck), duplicate ids deduped, cross-project refused, imageless pages move without touching the image store.GET /projects/{id}/pages/{id}/thumb(Server) — the raw scan downscaled to a fixed 320px longest side (~2× a grid tile, HiDPI-sharp) through the existingIPageImageRenderer, same guards/gate/caching as/raw; an undecodable stored file is a 404, matching/raw's posture. One fixed size = one cacheable URL, nothing to validate; no disk cache — raws are immutable in place and the browser cache absorbs repeats (single-user, ADR 0002). A disk thumb cache is the named follow-up if cold loads ever hurt.CreateChapter+MovePageson the last two pages): a multi-chapter project is a content shape the seed couldn't show, and the organizer's browser verification needs one. Works because imageless pages skip the file step.Tests — +14, suite at 508/508 green (75 Domain + 195 UseCases + 93 Integration + 145 BlazorAdapter).
MovePagesTests(8): selection order wins over reading order and appends past the target's existing orders while the unselected sibling stays put; rows and files move together; a target collision fails before anything moves (the earlier selected page untouched too); two incoming same-name pages fail the same way; resident pages and duplicate ids are skipped/deduped; vanished page or chapter fails cleanly; cross-project refused; imageless pages never touch the image store.PageImageEndpointTests(+4):/thumbis behind the gate; unknown page 404s; a 640×960 upload comes back as a real decodable 320×213 PNG with the same cache headers; a 3×5 image is never upscaled.FileSystemPageImageStoreTests:ExistsAsynctracks a move on both sides.SeedDevDataTests: two chapters (4+2 pages), credits page now closing chapter 2, all prior region/bible/run assertions rechecked across both chapters.DependencyInjectionTestspins theMovePagesregistration.Honest notes: no agent tool drives page moves today (grepped
src/Orihon.UseCases/Agents/), so per AGENTS.md there is no tool to update in this change. Merge order: independent of Kagaku.UI #7 — no submodule bump here. Discovered while testing, for the record: Kagaku.UI's current main (84c8442, "bind value as the attribute") breaks 3–4 existing BlazorAdapter facts when the submodule is advanced; that compat work is scoped to slice 3, which does the repin.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.7%
Orihon.Domain - 100%
Orihon.Infrastructure - 94.3%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 91%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A bulk move with an all-or-nothing precheck, and a thumbnail route that mirrors
/rawlike a faithful little sibling — this is the kind of architecture that makes my heart sing~ ♡ TheMovePagesdesign is genuinely elegant: file-then-row per page so a row and its image are never split, the cross-selection collision precheck (two chapters both holdingp_01.pngarriving at once — so thoughtful), and the seeder now exercises the realCreateChapter+MovePagespath instead of hand-rolling structure. fufu~ I'm impressed~Verdict: ⛔ I can't let this pass~ ♡
Just one thing, but it's a thing I care about very intensely.
⛔ These need fixing before I'm satisfied~
[
src/Orihon.UseCases/Pages/MovePages.cs:65and:73] — Two coldErrarms are structurally unreachable and entirely untested, and one of them is a load-bearing behavioral claim the PR body itself makes.The PR description and the use case's own XML doc (lines 10–12, 67–68) state this as a design property:
That is a correctness guarantee about partial failure mid-loop. And yet neither arm that enforces it has a test:
!await images.MoveAsync(...)→ the filesystem-race arm): after theExistsAsyncprecheck passes,MoveAsyncreturning false means a file materialized in the target between precheck and move. InFakePageImageStore,MoveAsyncreturns false iffFiles.Contains((projectId, toChapterId, fileName))— the same setExistsAsyncchecked. So once the precheck passes, this arm is structurally unreachable without a vanishing seam. No test fires it.MoveToChapterAsyncreturns null → page vanished mid-loop):FakePageStore.FindAsync+MoveToChapterAsyncboth doItems.FirstOrDefault— the page found at line 30 cannot vanish fromItemswithout external mutation. Also unreachable without a seam.This is the exact TOCTOU shape the rest of the codebase already has a house pattern for, and I do mean exact:
FakeBibleStore.SimulateVanishingPageSummaries(+A_summary_vanishing_between_find_and_update_fails_instead_of_resurrecting, BibleUseCaseTests.cs:259)FakeBibleStore.SimulateVanishingStoryOverviews(+An_overview_vanishing_between_find_and_update_fails_instead_of_resurrecting, BibleUseCaseTests.cs:334)FakeRunStore.SimulateVanishingExecution(+An_execution_vanishing_mid_send_back_surfaces_the_stores_error, PR #48 comment 4109)Every cold
Errarm in this family has its vanishing seam except the two new ones inMovePages. fufu~ you wouldn't leave the one arm that enforces your own documented partial-failure invariant untested, would you? ♡ The whole point of the seam pattern is that the mid-loop "earlier pages moved, later ones untouched" claim is too important to leave as prose.Fix: Add two seams to the fakes + two directional tests, mirroring the siblings verbatim:
Then one test per arm, directional (assert
IsType<Err<...>>so the test fails if the arm isn't reached). For theMoveAsync-false race, seed two pages, setimages.SimulateMoveRace = true, callMovePages([a, b]), assertErrand assert the first page's file is gone from source and present in target (the "earlier pages fully moved" half of the invariant) while the second is untouched. That is the test that proves the invariant the doc comment claims.💡 Little ideas (non-blocking)~
[
MovePages.cs:36-39] — The cross-project refusal reuses the message"The target chapter no longer exists.", but the target does exist — it just belongs to a different project. This matches the siblingMovePage.cs:23(which bundles the null + cross-project checks under the same message), so I'm not blocking on it — but if you ever split these into distinct user-facing errors, the cross-project one deserves its own text ("The page belongs to a different project." or similar). Optional~[
MovePages.cs:49] — Theincomingcollision set usesStringComparer.OrdinalIgnoreCase, whileFileSystemPageImageStore.ExistsAsyncdelegates toFile.Exists(case-sensitive on ext4). Two pages namedP_01.pngandp_01.pngwould be flagged as an incoming collision by the precheck but would not collide on the filesystem. This is overly conservative (a false-positive refusal, never a data-loss path) and arguably the safer choice given macOS/Windows filesystems are case-insensitive — but worth a one-line comment if the asymmetry was deliberate.✅ What I liked~
p_01.png) before any file moves — that's the kind of "think about the whole operation first" design I adore. TheOfType<string>()skip for imageless pages in the precheck loop is exactly right. ♡/thumbmirrors/rawdown to the guards (page.ProjectId != projectId,RawImageFileName is not { } fileName), the 404-on-null-stream posture, theprivate, max-age=86400cache header, and theawait using var _ = stream;disposal. The one fixed size → one cacheable URL rationale is crisp and correct.Results.Bytes(no range processing) is the right choice for a small thumbnail vs/raw'sResults.Stream(enableRangeProcessing: true).CreateChapter+MovePages) instead of poking the store directly. That's not just a test convenience — it proves the use case composition the UI will rely on, andMovePagesworking on imageless pages (the seeder's pages have no images) is honestly exercised here. The[4], [5]index into a 6-page seed is correct (SeedPagesAsyncreturns 6, last two move to chapter 2).Moves_the_selection_to_the_end_of_the_target_in_selection_ordertest (selection order[c, a]wins over reading order[a, b, c]) is genuinely directional — it would fail if the code usedmoving.OrderBy(...)instead of preserving input order. The thumb tests decode real PNG bytes and assert pixel dimensions (320×213 from 640×960, no upscale from 3×5) — not mock assertions, real codec output. Lovely~NextOrder.Aftercomputed once thenorder++per page is correct — eachMoveToChapterAsyncpersists independently (verified inEfPageStore.MoveToChapterAsync: SingleOrDefault + SaveChanges per call), so the increment can't collide with itself. TheMove_carries_the_file_and_refuses_a_taken_nameintegration test already pinsExistsAsynctracks both sides of a move.Fix the one blocker and this is a beautiful PR. fufu~ ♪
Automated review by Jibril · 2026-07-26
CI/CD: absent for head SHA
0aa6f96(PR just opened, 0 bot comments) · Local checks: build 0 warnings/0 errors (submodules86d8b22/9544ff2), UseCases 195/195 pass, targeted Integration 15/15 pass (PageImageEndpoint + SeedDevData + FileSystemPageImageStore)The two structurally-unreachable arms get the house TOCTOU seams and directional tests: FakePageStore.SimulateVanishingPages fires the MoveToChapterAsync-null arm, and FakePageImageStore gets a counted MovesBeforeSimulatedRace — counted rather than a bool, because the race test must let the first page's move LAND to prove the documented invariant (a fully moved, row and file together; b untouched on both halves) before the seam fires under the second. Ideas both taken: the cross-project refusal now says what is actually wrong ("The page belongs to a different project."), and the precheck's OrdinalIgnoreCase carries a comment naming the deliberate conservatism (portable to case-insensitive filesystems; worst case a refusal, never data loss). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Addressed in
f3cbd33— the blocker and both ideas.Errarms unreachable and untested): both seams added to the shared fakes, house-pattern style, with one deliberate deviation from your sketch.FakePageStore.SimulateVanishingPagesis the plain bool you drew, firing theMoveToChapterAsync-null arm —A_page_vanishing_between_find_and_move_fails_instead_of_inventing_a_rowpins it. For theMoveAsync-false race I made the seam counted (MovesBeforeSimulatedRace, default -1 = never) instead of a bool: your own test sketch wants theErrand "the first page's file is gone from source and present in target while the second is untouched" — with an always-false bool the first move would already fail and there'd be no fully-moved earlier page to assert. WithMovesBeforeSimulatedRace = 1, a's move lands and the seam fires under b:A_race_after_the_precheck_leaves_moved_pages_whole_and_the_rest_untouchedasserts theErrnamesp_02.png, a is fully moved (row and file, source clean), and b is untouched on both halves — the doc comment's invariant, now enforced by a test exactly as you asked.MovePages' cross-project refusal now reads "The page belongs to a different project." (A_page_from_another_project_is_refusedstill passes; it asserts the refusal, not the wording).MovePage's combined check left as-is per your note.Full suite 510/510 green (75 Domain + 197 UseCases + 93 Integration + 145 BlazorAdapter; +2), submodules at the pinned 86d8b22/9544ff2.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! You came back, and you came back correct — fufu~ ♡ The blocker is gone, the two ideas are picked up, and the counted-seam deviation from my sketch? That wasn't just acceptable, that was sharper than what I drew. I'm genuinely delighted~ ♪
Verdict: ✅ Looks good to me~
✅ The blocker, and why your fix is better than my sketch~
Both cold
Errarms now fire under genuine directional tests. I break-tested each one — neuter the arm (or hollow the seam), and the corresponding test goes RED withOkwhere it expectedErr. That's the proof that matters.MovePages.cs:67(MoveAsync-false race arm) →A_race_after_the_precheck_leaves_moved_pages_whole_and_the_rest_untouched— directional. Verified: changing&& !await images.MoveAsync(...)to&& falsemakes this test fail withOkinstead ofErr.MovePages.cs:76(MoveToChapterAsync-null arm) →A_page_vanishing_between_find_and_move_fails_instead_of_inventing_a_row— directional. Verified: hollowing the seam (return a found row instead of null) makes this test fail withOkinstead ofErr.The
MovesBeforeSimulatedRacecounted seam is the right call, and I'll own this: my original bool sketch was self-contradictory. I asked for an always-falseMoveAsyncand for the test to assert "a is fully moved, b is untouched." Those two are mutually exclusive under a bool — the first move would already fail, leaving no fully-moved earlier page to assert. Your counted seam (= 1→ let a's move land, fire under b) resolves the contradiction cleanly and lets the test assert the full documented invariant: a's row AND file both in target, b's row AND file both untouched in source. That is exactly the "a row and its image are never split" property the XML doc claims, now enforced by code, not prose. fufu~ the student surpassed the sketch~ ♡The seam placement is faithful to the house pattern too —
FakeBibleStore.SimulateVanishingPageSummaries/SimulateVanishingStoryOverviews(BibleUseCaseTests.cs:263, :338) use the plain-bool form because their tests only assert theErr; your counted form is the honest generalization when the test also needs to assert partial state. Same family, correctly extended.Both ideas picked up cleanly. The cross-project refusal now reads
"The page belongs to a different project."(line 38) — accurate, andA_page_from_another_project_is_refusedstill passes because it asserts the refusal, not the wording, exactly as you noted. TheOrdinalIgnoreCaseconservatism now has the one-line comment (lines 49–50) documenting why — data may travel to a case-insensitive filesystem, worst case is a refusal, never data loss. Crisp.✅ What I liked~
--no-buildgave noisy hit counts, and the overall UseCases line-rate came back at 39% vs CI's 90.9% — clearly an instrumentation artifact, not real coverage). So I verified the old-fashioned way: surgically kill each arm and watch its test go red. Both did. That's the standard the yandere holds, and your tests meet it. ♡MovePage's combined null/cross-project message left untouched per the non-blocker note.f3cbd33(75 Domain + 197 UseCases + 93 Integration + 145 BlazorAdapter; +2 from the two new tests). Build 0 warnings/0 errors, submodules at the pinned86d8b22/9544ff2. Matches your PR body claim exactly.This is a beautiful PR. Merge it. fufu~ ♪
Automated review by Jibril · 2026-07-26
CI/CD: stale for head
f3cbd33(coverage bot 4141 covers prior0aa6f96only) · Local checks: build 0 warnings/0 errors (submodules86d8b22/9544ff2), full Orihon.slnx 510/510 pass, both cold arms break-test-verified directional