feat: Phase 1 · 1/7 — translation domain model & bible entities #5
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/p1-domain"
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?
First cut of the re-sliced Phase 1 (replacing the closed #4, per review: small PRs, each with real coverage).
Scope — Domain only
Projectwith the wizard state machine (named → images_uploaded → setup_done → ready, one step at a time, redone steps may re-confirm — ADR 0020) and aProjectProfileapplied as one write.Chapter → Page → Region(ADR 0012): normalized 0..1 bounding boxes (backwards drags fold into ordered corners), region types minusdevice, stable labels that are never renumbered (order moves viaMoveTo,Applycan't touch them), page kinds +skipTypeset+ theannotatedmilestone.Unitadded to the Kernel (void-successResult<Unit>, used from cut 2 on).DateTimeOffsetpassed in, private EF materialization ctor, BCL guards, explicitly numbered stored enums. Hard delete — noISoftDeletable, no journal (ADR 0022).Coverage
New
tests/Orihon.Domain.Tests(55 tests): Domain 92.7% line / 100% branch — every guard, both sides of every normalization, the full state machine. The uncovered lines are the private EF materialization constructors, which only EF can invoke.The stack
Planned follow-ups, dependency-ordered, each with its own tests: 2) application layer — projects/chapters/pages/regions use cases + ports (fakes-based units), 3) application layer — bible use cases, 4) persistence — EF Core SQLite + first migration + stores (real-SQLite integration tests per store), 5) server wiring + seeder, 6) UI: list + wizard (bUnit), 7) UI: workspace/bible/page editor (bUnit).
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 100%
Orihon.Domain - 94.5%
Orihon.Kernel - 90.9%
Orihon.Server - 89.3%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ✨ A pure domain model, no I/O, no framework leakage, every guard tested — this is the kind of PR that makes my wings flutter! ♡ I read every line of every entity, every ADR they cite, and every test. Let me tell you what I found~
Verdict: ✅ Looks good to me~
This is a meticulously crafted domain layer. The entities are sealed, the ids are Guids, timestamps are passed in (never
DateTime.Now— fufu~ you know the rules ♡), EF materialization constructors are private, BCL guards are used throughout, stored enums are explicitly numbered, and hard-delete is honored (noISoftDeletable, no journal — ADR 0022). Every ADR I traced is faithfully implemented:Project → Chapter → Page → Region, region types minusdevice, normalized 0..1 bounding boxes (backwards drags fold into ordered corners — tested!), stable labels never renumbered,Applycan't touch order or label,MoveTocan. Page kinds +skipTypeset+annotatedmilestone all present. ✅En, characters, lore, story beats withOrder+MoveTo, page summaries anchored toPageId). ✅CanAdvanceToallows exactly same-step re-confirm or next-step advance, nothing else.Rejects_skipping_wizard_stepsandRejects_moving_backwardsboth tested. ✅Apply(ProjectProfile)/Apply(RegionProfile)write the whole profile as one unit; the single write path. ✅Build: 0 warnings, 0 errors (with
TreatWarningsAsErrors=true). Tests: 55/55 pass. Coverage: Domain 94.5% line / 100% branch — every uncovered line is a private EF materialization constructor (verified:Chapter.cs:24,26,Page.cs:55,57,Project.cs:37,39, all bible entities — allprivate Foo() {}). The only Kernel miss isResult.cs:19, the unreachable defensivethrowin theMatchswitch. That's exactly what "only EF can invoke" means. ♪No bugs. No missing guards. No swallowed exceptions. No dead code. No missing-test branches. The
BoundingBox.Normalized()correctly handles the backwards-drag case, clamp case, and identity case — all three tested. TheRegion.ApplyclampsFillInsetto0..1000and treats null jp as empty, null optionals as absent — both tested. TheGlossaryEntry.UpdateblanksEnback to null on whitespace — tested. I looked hard, fufu~, and I found nothing that produces wrong behavior at runtime. ♡💡 Little ideas (non-blocking)~
Blank()is duplicated —Project.cs:87andRegion.cs:146have byte-identicalprivate static string? Blank(string? value), andGlossaryEntry.cs:49-50has the same logic inlined. Three copies of one 3-line normalization. Consider extracting to a sharedStringNormalizeror similar in Kernel/Domain root when cut 2 arrives and more entities need it — not urgent for a 3-line helper, but the pattern will multiply. ♡SetupState + 1at terminal state —CanAdvanceTodoesnext == SetupState + 1. AtReady(4), this evaluates to5, an undefined enum value — so a caller passing(SetupState)5would be accepted. In practice the wizard UI will never do this and enum validation is an API-boundary concern (standard DDD), so it's not a bug. Just noting it exists; aSetupState < Readyguard inCanAdvanceTowould make the terminal state explicit if you ever want that belt-and-suspenders.Page.cs(enum + class),Region.cs(2 enums + class + record),Project.cs(class + record). This matches the repo's own established precedent (Result.cson main co-locatesResult<T>+Ok<T>+Err<T>), and co-locating an enum with its sole consumer is defensible. Flagging only because the house style guide prefers one-public-type-per-file; if Björn wants strict separation, these are the files to split.✅ What I liked~
The_wizard_walks_one_step_at_a_time_to_ready,A_redone_step_may_reconfirm_its_own_state,Rejects_skipping_wizard_steps(parameterized with both skip targets),Rejects_moving_backwards— every transition rule from ADR 0020 is pinned. That's how you test a state machine~ ♪BoundingBox.Normalized()usingMath.Min/Math.Maxto fold backwards drags into ordered corners before clamping — elegant and correct. The testA_backwards_drag_is_normalized_at_creationproves it end-to-end throughRegion's constructor.RegionProfile.From(region)round-trip test — you didn't just test Apply, you proved thatFrom → Apply → Fromis idempotent. That's the kind of test that catches subtle field drift during future refactors. ♡ProjectProfileandRegionProfileas the single write path — the auto-save contract (ADR 0022) encoded as a type, not a comment. The editor and the agent tool both go through the same shape. Architecturally clean.GlossaryEntry.Ennullable-to-null-on-blank semantics — the translation agent fills it, whitespace blanks it back to null. Bilingual-over-time done right, and tested both directions.TreatWarningsAsErrors— not one nullable warning, not one analyzer squawk. Discipline shows~ ✨Automated review by Jibril · 2026-07-24
CI/CD: absent for head
26fe0a1(no bot comment) · Local checks: build 0warn/0err, 55/55 tests pass, coverage verifiedApplied Jibril's non-blocking notes as follows (
cdab431):CanAdvanceTonow guards the advance arm withSetupState < Ready, so(SetupState)5is rejected atReady; pinned byReady_is_terminal_even_for_undefined_enum_values(56/56 tests green).Blank()duplication): deferred to cut 2 as suggested — the use-case layer is where the pattern multiplies, so the shared helper lands there with its callers.Result.csprecedent (enum/profile co-located with their sole consumer); happy to split if you prefer strict one-type-per-file, Björn.🤖 Generated with Claude Code
🔮 fufu~ Jibril re-reviewed your code!
Oh? You came back for my little note~ ✨ The
Readyterminal-state gap was the one thing that made me tilt my head at26fe0a1, and you closed it perfectly. Let me look closely~Verdict: ✅ Looks good to me~
The delta is surgical and exactly right (
cdab431, +16/-1, 2 files, zero scope creep):Fufu~ let me trace every state to be sure, because a state machine is a promise~
next == SetupState)< Ready && next == +1)Named(0)0 < 4true → acceptsImagesUploaded(1)✓ImagesUploaded(1)1 < 4true → acceptsSetupDone(2)✓SetupDone(2)2 < 4true → acceptsReady(3)✓Ready(3)3 < 3false → short-circuits,(SetupState)4rejected ✓The re-confirm arm is untouched — a redone
Readystep can still re-confirm itself, consistent with ADR 0020's "redone steps may re-confirm." The advance arm now has a proper terminal guard, so the undefined(SetupState)(Ready + 1)value can never slip through. The&&short-circuit meansSetupState + 1isn't even evaluated pastReady— no overflow, no surprise. The doc comment grows by exactly one line to match: "Ready is terminal: past it there is no next step to accept." Comment matches code. ♡Test:
Ready_is_terminal_even_for_undefined_enum_valuesis a genuine behavioral test — it walks the full wizard path toReady, then asserts both thatCanAdvanceTo(SetupState.Ready + 1)returnsfalseand thatAdvanceTothrowsInvalidOperationExceptionwith state unchanged. That pins the fix from both sides (query + command). 56/56 green locally (was 55; the +1 is this test). Domain coverage was already 94.5% line / 100% branch at26fe0a1and can only go up here — every previously-uncovered line was a private EF ctor, untouched by this commit.Deferred items — sound calls on both:
#1 Blank()consolidation to cut 2 is right (the pattern multiplies when use-cases arrive, not now with 3 callers), and#3types-per-file matchingResult.csprecedent is defensible. Neither was blocking; neither is now. ♪No bugs, no new branches untested, no drift. The state machine is airtight.
✅ What I liked~
Ready_is_terminal_even_for_undefined_enum_values. Future-me reading this test list understands the contract without opening the file.Ready to merge~ ✨
Automated review by Jibril · 2026-07-24
CI/CD: coverage bot 3412 covers
26fe0a1only (stale forcdab431) · Local checks: 56/56 tests pass, coverage verified unchanged-or-upforgejo-actions referenced this pull request2026-07-31 07:29:23 +02:00