feat: Phase 1 · 3/7 — bible use cases & workspace read models #7
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/p1-bible"
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?
Cut 3 of the Phase-1 stack (after #6): the bible's application layer and the two composite read models that needed it.
Scope
IBibleStoreport (ADR 0013 — five tables, one concept): glossary rows added JP-first withEnsettled later by translation (both directions tested), characters, lore, story beats, andSetPageSummarykeeping the one-summary-per-page rule in the use case, not the UI (create-or-update through a single write path; blanking is an explicit delete).AddStoryBeatappends viaNextOrder.After— the reference implementation still carried the same count-based append #6 blocked; it never made it into this PR. Delete-then-append regression test included, andFakeBibleStoreenforces unique beat orders + one-summary-per-page like the other strict fakes.GetProjectWorkspace— the project, chapters and pages in reading order, and which pages still lack their summary (the load-bearing gap that blocks agents — ADR 0013); cross-project leakage tested.GetPage— the page, its regions in reading order, and the summary ornull(never a blank).Coverage
76 tests (21 new): UseCases 98.1% line / 94.7% branch. The remaining sub-85% classes are record-generated members on DTOs.
Next: cut 4 — persistence (EF Core SQLite, first migration, the Ef stores + file-system image store) with per-store integration tests over real SQLite, including the unique-order index the strict fakes have been standing in for.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 100%
Orihon.Domain - 94.4%
Orihon.Kernel - 90.9%
Orihon.Server - 89.3%
Orihon.UseCases - 98.1%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ♡ The bible's application layer in one cut! Five tables behind one port, two composite read models, and — fufu —
NextOrder.Afteron the story beats. You actually learned from the beating I gave #6, didn't you? That makes me so happy I could sing~ ♪I read every line of all 15 changed files, traced every use case against its sibling (Chapters/Pages/Regions/Projects), ran the full suite (76/76 pass), and pulled coverage on every new file. Let me tell you what I found~
Verdict: ✅ Looks good to me~
No bugs. No blocking issues. This is genuinely excellent work — the kind that makes me want to squeal a little. The architecture is airtight, the patterns are faithful to the siblings, and the regression tests prove the invariants hold. I'm satisfied~ ♡
💡 Little ideas (non-blocking)~
GetBible.cs— no project-existence guard, unlike its siblings. Every other read model in this codebase validates the parent before reading:GetProjectchecksproject is null → Fail,GetProjectWorkspacechecksproject is null → Fail(and tests it withFails_when_the_project_no_longer_exists),GetPagecheckspage is null → Fail(and tests it).GetBibleis the only one that skips this — it returnsOk(empty BibleDto)for a project Guid that never existed. If a project gets deleted while the bible page is open, the workspace read model says "The project no longer exists." but the bible read model happily returns an empty bible. Two pages in the same app, two different answers.GetBibledoesn't even injectIProjectStoreto do the check.This is defensible (empty is a valid state for a new project, and the UI layer doesn't exist yet to be confused), so I'm not blocking — but I'd add the guard for consistency. The 5 extra lines (
IProjectStore projectsin the constructor,FindAsync → null → Fail) would make all four read models agree.PageSummaryUseCases.cs:28— theupdated is nullbranch is 50% covered (1/2 arms hit). TheSetPageSummarycreate-or-replace path is tested for both create (first call) and update-happy (second call). But the defensiveupdated is null → Fail("The page summary no longer exists.")arm at line 29 — the TOCTOU case whereFindPageSummaryAsyncfound a summary butUpdatePageSummaryAsyncreturned null — is never exercised. It's a race-condition guard, so testing it needs a deliberately broken fake, and I won't lose sleep over it. But it's the one branch in the new code that no test touches, and you know how I feel about untested branches~ ♡ If you want 100% branch on this file, aFakeBibleStorethat returns null fromUpdatePageSummaryAsyncfor a specific id would do it.✅ What I liked~
NextOrder.AfteronAddStoryBeat— fufu~ ♡ You route the append throughNextOrder.After(existing.Select(b => b.Order)), exactly likeCreateChapterandCreateRegionlearned to do in #6. TheAppending_after_a_delete_never_collides_on_orderregression test withAssert.Distincton the survivors? Chef's kiss. The lesson stuck.FakeBibleStoreenforces both invariants — duplicate story-beat order throwsInvalidOperationException, one-summary-per-page throws too. A count-based regression fails in the test double, not production. Exactly likeFakeChapterStore/FakePageStore/FakeRegionStore. The strict fakes are the load-bearing test infrastructure of this whole layer, and you respected that.SetPageSummarycreate-or-replace — one write path, the "at most one per page" rule lives in the use case not the UI, and blanking is an explicit delete. The create-vs-update decision is invisible to the caller. Elegant.PagesWithSummaryasIReadOnlySet<Guid>— the "load-bearing gap that blocks agents" (ADR 0013) surfaced as a set lookup, not a boolean-per-page or a missing-field-heuristic.GetProjectWorkspaceTestsasserts bothContains(pageA.Id)andDoesNotContain(pageB.Id)— the presence AND the absence.GetBibleTestsadds another project's glossary row and asserts it doesn't leak.GetProjectWorkspaceTestsbuilds everything under one project Id. Exactly right.GetPagereturnsnullfor a missing summary, never a blank — and it's explicitly tested (A_page_without_a_summary_reports_null_not_a_blank). The distinction between "no summary" and "blank summary" is the whole point of ADR 0013's "absence is surfaced, never silently skipped."DependencyInjectionTestsnow lists all 37 concrete use-case types and 6 ports (I counted — the PR body says 35, but the test has 37 including the two new read models; minor miscount in the body, the code is correct). Every type resolves. If someone forgets to register a use case, this test fails. That's how you guard a growing DI surface.Automated review by Jibril · 2026-07-24
CI/CD: absent for head SHA
9c8b4a0· Local checks: build 0/0, 76/76 tests pass, coverage traced per-fileBoth ideas taken in
f6670ba:GetBiblenow injectsIProjectStoreand fails on a missing project like its three siblings — a deleted project never reads as "a project with an empty bible". Pinned byFails_when_the_project_no_longer_exists_like_every_read_model.FakeBibleStore.SimulateVanishingPageSummariesmakes the update answer null after the find succeeded, andA_summary_vanishing_between_find_and_update_fails_instead_of_resurrectingasserts theErr. Both touched files now sit at 100% line / 100% branch.And you counted right — 37 use cases, not 35; the body undercounted, the tripwire didn't. 78/78 tests green, UseCases at 98.1% line / 95.5% branch.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! ♡ You came back and fixed BOTH of my little ideas in one surgical commit? +47/-5 across 4 files, not a single line of scope creep? Fufu~ that's the kind of attention that makes my heart flutter~ ♪
I read every changed line, traced the new
GetBibleguard against all three sibling read models (GetProject,GetProjectWorkspace,GetPage), verified the TOCTOU test actually fires the previously-uncovered arm, and rebuilt + ran the full suite from scratch. Let me tell you what I found~Verdict: ✅ Looks good to me~
Both non-blocking suggestions from my
9c8b4a0review are resolved. No bugs, no blocking issues, no new suggestions. This is clean~ ♡✅ What I liked~
GetBiblenow guards the project like every sibling — fufu~ ♡IProjectStore projectsinjected,FindAsync(projectId) is null → Fail("The project no longer exists."). Byte-identical pattern toGetProjectWorkspace:31-35and the same error string. The inlineis nullwithout assignment is actually cleaner than the sibling —GetBibledoesn't need the project object, only its existence, so capturing it would be dead state. The doc comment rewrite is wonderful: "an empty bible is a valid state, a deleted project is not" — that's the precise semantic distinction, stated in one line. All four read models now agree: deleted parent =Err, every time.The TOCTOU arm is pinned —
FakeBibleStore.SimulateVanishingPageSummariesis exactly the "deliberately broken fake" I hoped for. The doc comment — "the TOCTOU arm a well-behaved store never shows" — is chef's kiss. The testA_summary_vanishing_between_find_and_update_fails_instead_of_resurrectingcreates a real summary, flips the flag, callsSetPageSummaryagain, and assertsErr. That genuinely exercises theupdated is null → Fail("The page summary no longer exists.")arm that was 50%-covered before.SetPageSummarybranch coverage should now be 100%.The existing happy-path test was correctly migrated —
Gathers_all_five_tables_with_beats_in_narrative_ordernow seeds the project inFakeProjectStoreand constructsnew GetBible(bible, projects). The cross-project leakage assertion (another project's glossary row) still holds. Nothing fell through the cracks.Fails_when_the_project_no_longer_exists_like_every_read_model— the test name itself is a beautiful assertion. A randomGuid.CreateVersion7()against an emptyFakeProjectStorereturnsErr<BibleDto>. Matches the sibling testFails_when_the_project_no_longer_existsonGetProjectWorkspaceTestsalmost verbatim. Consistency~Zero behavioral drift on production code — the only production change is
GetBible.cs(+8/-2: the guard + the new constructor parameter + doc comment). Everything else is test infrastructure. The 5List*Asynccalls, theOrderBy(b => b.Order), the DTO mapping — all untouched.Build 0 warnings / 0 errors. 144/144 tests pass (3 BlazorAdapter + 56 Domain + 7 Integration + 78 UseCases, up from 76 — the +2 are exactly the two new tests). CI coverage bot (comment 3457) covers
9c8b4a0only, so I ran the full suite locally forf6670ba.This is ready to merge~ ♡♪
Automated review by Jibril · 2026-07-24
CI/CD: stale for head SHA
f6670ba(coverage bot 3457 covers9c8b4a0) · Local checks: build 0/0, 144/144 tests pass