feat: Phase 1 · 7/7 — workspace, bible & page editor #11
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/p1-ui-content"
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?
The final cut of the Phase-1 stack (after #10) — the manual manager is complete, and the ready-project route gap flagged on #10 closes.
Scope
DragReorderList), kind select (cover impliesskipTypeset), move-to-chapter, modal-gated delete, zip upload; annotated and no summary badges — the load-bearing gap of ADR 0013, visible per page.RegionSelectorwith a labelled ghost overlay: a commit with a selection moves that box (saved on pointer-up, no debounce), with none it drag-creates at the end of reading order with the next never-reused label. The region editor saves the whole profile per ADR 0022, recorded-only types droptypesetin the same write (ADR 0012), and the translation overlay falls back to JP where EN is missing.region.jsin App.razor; theDebouncehelper with direct latest-write-wins / flush tests.Coverage
30 new bUnit tests (51 adapter total) over the shared
AdapterTestContext— including theRegionSelectorcommit contract invoked directly (the live drag is JS; the contract is the committed rect), the modal-scoping subtlety (closed<dialog>s keep their buttons in the DOM), and grip-keyboard reordering. BlazorAdapter 78.4% line / 73.9% branch in isolation; the remainder is JS-gesture plumbing and error arms real use cases can't produce. 227 tests green solution-wide.Verified in a browser
Seeded world driven end to end on this branch: gate → list → ready project opens its workspace (the #10 seam, closed) → page editor with regions/tabs → bible. The only console noise is the seeded imageless pages' expected thumbnail 404s.
Ships when (PROJECT_PLAN Phase 1): you can create a project, upload pages, hand-author regions and the bible, and browse it all end to end — no OpenRouter key required. ✅ With this merge, that's true on main.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 80.7%
Orihon.Domain - 100%
Orihon.Infrastructure - 98.8%
Orihon.Kernel - 90.9%
Orihon.Server - 91.3%
Orihon.UseCases - 99.3%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ The final cut of Phase 1 — the workspace, the bible, AND the page editor, all in one stroke! The whole manual manager, complete. This is a lot of surface area, scarlet, and I read every single line of it, the sibling pages, the use cases, the domain, the design-system components it binds against… fufu~ I'm genuinely impressed by how cleanly it hangs together. The architecture is beautiful work.
But ♡ — you know I wouldn't let it slide just because it's pretty. I found two real bugs, and one of them is sitting right next to its own sibling that does it correctly.
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
ProjectWorkspacePage.razor—UploadAsynclacks thetry/finallythat its sibling has, sobusysticks on any exception.Your own
ProjectWizardPage.UploadAsync(this same PR stack, #10 lineage — lines 197–225) wraps the identical upload pattern intry { … } finally { busy = false; }. This PR's copy at lines 257–269 doesbusy = true; …; busy = false;with no guard around the await:ImportPagesreads browser upload streams and unpacks zips —OpenReadStreamthrowsIOExceptionpast the size cap, and zip expansion is genuinely exception-prone. If any of that throws,busystaystrueforever: the<InputFile … disabled="@busy" />(line ~1385) is permanently disabled and the user cannot recover without a full reload. TheResult<T>return handles expected failures, not exceptional I/O. The sibling gets this right; this copy must match it.Fix: wrap the body in
try { … } finally { busy = false; }exactly likeProjectWizardPage.UploadAsync.ProjectWorkspacePage.razor:312—MoveTargetstakescurrentbut never uses it, so the "move to chapter" dropdown lists the chapter the page is already in.The
currentparameter is dead — the body doesn't filter it out. The dropdown (lines ~370–374) renders every chapter including the one the page already belongs to, with that chapter pre-selected (Value="chapter.Chapter.Id"). Selecting the current chapter firesMoveAsync, which early-returns ontargetChapterId == page.ChapterId— so the user's selection is silently swallowed and the dropdown snaps back. That's a misleading control: a "move to" list that offers "move to where you already are" and silently ignores it. Either filtercurrentout (the parameter name says you meant to) or, if listing-all is intentional, drop the dead parameter and the pre-select. Given the parameter exists and is namedcurrent, filtering is the fix.Fix:
.Where(c => c.Chapter.Id != current).💡 Little ideas (non-blocking)~
Debounce.cs:560—Disposefire-and-forgets the flush._ = flushing?.Invoke();discards the Task; a throw would surface asUnobservedTaskException. This is the standard teardown pattern and the use cases it calls returnResult<T>(no expected exceptions), so it's fine in practice — but atry/catcharound it (logging or swallowing) would make the "the save still belongs on the wire" comment load-bearing rather than aspirational. The siblingFlushAsyncis awaited by callers so it doesn't have this gap. Truly optional.PR body says "227 tests green"; actual is 229 (51 BlazorAdapter + 56 Domain + 44 Integration + 78 UseCases, verified locally). Off-by-two, no concern — just a heads-up the count drifted.
✅ What I liked~
RegionRect↔BoundingBoxboundary translation is textbook. Kagaku.UI stays domain-free (RegionRectis pure geometry), and the page converts at the edge inOnRegionRectAsyncandSelectedRect. That's exactly the seam ADR 0030 asks for, and it's clean.OnRegionRectAsync: "no selection → create at end of reading order, selection → move that box, saved on pointer-up, no debounce." ReadingRegionSelector.razor, the JS lives there and Blazor only hears the committed rect — the testDrawing_on_the_page_creates_a_region_at_the_end_of_reading_orderinvokes the contract directly and is genuinely behavioral (asserts order=2, type=Dialogue, the exact bbox). That is how you test a JS-bound component without a browser. ♪Debouncelatest-write-wins is correct and the two tests (Only_the_latest_scheduled_save_runs,Flush_runs_the_pending_save_without_waiting_out_the_delay) pin it. Thepending?.Cancel(); pending?.Dispose();before re-arm is the right sequence.Typeset = falsedrop inSetTypeAsync(ADR 0012) is wired into the same write as the rest of the profile — oneSaveAsync, not two. Clean.<dialog>s stay in the DOM while closed, andConfirmIntargets by title text. That comment inProjectWorkspacePageTests("Both confirm dialogs sit in the DOM even while closed") saved me a re-read; it's a real bUnit gotcha and you handled it precisely.TouchSummaryblanking → delete (row.SummaryId = null; Report(await DeletePageSummary...)) is the right semantics: an empty summary re-blocks the page, visibly. ADR 0013's gap is load-bearing and this honors it.ReloadAsyncdraft guard redirecting to/setup(ADR 0020) — closes the #10 seam exactly as promised.Build: 0 warnings, 0 errors. Tests: 229/229 pass (51 BlazorAdapter + 56 Domain + 44 Integration + 78 UseCases). CI absent for head
9ed4634— local verification used throughout.Two blockers, both small, both next to the sibling that already does it right. Fix those and this ships. Fufu~ ♡
Automated review by Jibril · 2026-07-24
CI/CD: absent for head SHA · Local checks: build 0/0, 229/229 tests pass
Both blockers fixed in
b810b4f— and you called the lineage exactly right: the workspace copy was sliced from the reference branch before #10 taught its sibling the lesson.ProjectWorkspacePage.UploadAsyncnow wears the sametry/finally { busy = false; }as the wizard's, with the same circuit-survival comment. AnIOExceptionpast the size cap or a zip-expansion throw can no longer freeze the intake.MoveTargetsnow actually usescurrent—.Where(c => c.Chapter.Id != current)— and the dropdown shows a "Move to…" placeholder instead of pre-selecting the page's own chapter, so there is no silently-swallowed selection left to have.MoveAsync's same-chapter early-return stays as defense. New testThe_move_dropdown_offers_only_other_chapters_and_moves_the_pagepins both the offered options (exactly the other chapter, placeholder excluded) and the move itself.Debounce.Dispose's teardown flush now observes its faults via aOnlyOnFaultedcontinuation — noUnobservedTaskExceptionfrom a failed last save.Build 0/0, 230/230 green.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? OH~ scarlet came back and fixed everything — and I mean everything, including the non-blocking nit I said was "truly optional"! Fufu~ you wouldn't leave anything for me to sharpen my nails on, would you? ♡ Let me verify each one~
Verdict: ✅ Looks good to me~
✅ Every finding from my last pass — resolved~
UploadAsyncnow wears itstry/finallylike the wizard sibling taught it~ (ProjectWorkspacePage.razor:260-277)Byte-for-byte the same shape as
ProjectWizardPage.UploadAsync:197-225—busy = trueoutside thetry, reset infinally. AnIOExceptionfromOpenReadStreamor a zip explosion can no longer permanently disable the<InputFile>. The comment "Like the wizard's upload" is honest documentation, not ceremony. Bug is gone. ♪MoveTargetsfinally uses itscurrentparameter~ (ProjectWorkspacePage.razor:323-326)The dead parameter lives. And you went further than my fix — you swapped the pre-selected
Value="chapter.Chapter.Id"forPlaceholder="Move to…", so the control now reads as an action ("Move to…") rather than a state display. That's the better UX call: a placeholder invites intent, a pre-selected value implies "this is where you are." TheMoveAsyncearly-return guard (targetChapterId == page.ChapterId) is still there as defense-in-depth — correct, never remove it.Debounce.Disposenow observes its faults~ (Debounce.cs:56-60) — I said this was optional!You didn't just
try/catchit — you usedContinueWith(OnlyOnFaulted)to observe theExceptionproperly. That's the correct TAP idiom for observing a fire-and-forget task's fault without blocking the teardown thread.UnobservedTaskExceptioncan no longer fire. The comment "Faults are observed so a failed teardown save never surfaces as UnobservedTaskException" makes the intent load-bearing. This is even cleaner than what I suggested. Fufu~✅ The new test is genuinely directional~
The_move_dropdown_offers_only_other_chapters_and_moves_the_page— I checked: against the oldMoveTargets(no.Where), the offered list would be["Chapter 1", "Chapter 2"]andAssert.Equal(["Chapter 2"], offered)would FAIL. Against the new code, only Chapter 2 is offered. That's a real regression guard, not a tautology. And it goes further:move.Change("0")thenWaitForAssertion(() => Assert.Equal(second.Id, Pages.Items.Single().ChapterId))— it pins the actual move too, not just the options. Both the contract and the behavior in one test. Beautiful. ♡✅ What I liked~
Placeholder="Move to…"is the subtle right call I didn't even ask for. A move-to control should invite the action, not assert the current state. Pre-selectedValuewould have still been "fixed" but the UX would have stayed slightly confusing. You fixed the bug and the smell.option:not([disabled])selector in the test is precise — it skips any placeholder/disabled option theSelectcomponent might render, so the assertion targets only real, selectable chapters. That's someone who knows bUnit + AngleSharp, not someone copy-pasting.Build: 0 warnings, 0 errors. Tests: 230/230 pass (52 BlazorAdapter + 56 Domain + 44 Integration + 78 UseCases — the +1 from 229 is your new test). CI coverage comment (3534) covers the initial head only; local verification used throughout.
All blockers from round 1 are closed. All three fixes verified against sibling code and the full suite. This ships. Fufu~ ♡
Phase 1 is complete. Seven PRs, every one of them reviewed and approved. That's a beautiful stack, scarlet — take the win. ♪
Automated review by Jibril · 2026-07-24
CI/CD: stale for head
b810b4f(coverage bot covers initial head) · Local checks: build 0/0, 230/230 tests pass