feat: Phase 1 — domain, persistence & manual content management #4
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/phase-1-manual-management"
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?
Implements Phase 1 of
docs/PROJECT_PLAN.md: the whole app as a manual doujinshi manager — a real world before any agent runs. No AI, no OpenRouter key required.What's in here
Domain (ADR 0012, 0013) —
Project → Chapter → Page → Regionplus the five bible tables (glossary, characters, lore, story beats, page summaries). Kagura's entity conventions: sealed classes, Guid v7 ids, timestamps passed in, profile records applied as one write. Hard delete throughout — no soft delete, no journal (ADR 0022).Persistence (ADR 0005) — single SQLite DB via EF Core,
IEntityTypeConfigurationper entity, UtcTicks dates, enums as int, bbox/tags as JSON scalar columns, cascade deletes, and the first migration. Raw page images on disk underprojects/<id:N>/<chapterId:N>/raw/, path-canonicalized against traversal.Use cases (ADR 0003, 0022) — one class per operation,
Result<T>, ports in UseCases: project CRUD + the wizard state machine (ADR 0020), zip-aware page import (filename order, dedupe, non-images skipped), chapter/page organize, region editing with never-renumberedp{n}r{m}stable labels, bible CRUD. Agents' tools will drive these same classes later — one write path.UI (Kagaku.UI) — project list with drafts + modal-gated hard delete; the 3-step creation wizard resuming from stored setup state (step 3 stubbed until the Research & Setup agent, per the plan); pages & chapters (upload incl. zip, reorder, move, kind, delete); the bible's five tables with 700 ms debounced auto-save; the page workspace with Raw · Bbox · Translation views — manual bbox editing via
RegionSelectorwith a labelled overlay, and the pixel-pass views shown as deferred (ADR 0021).Server —
AddUseCases()/AddInfrastructure(), migrate-at-startup, the single authorized page-image endpoint, and theORIHON_SEED_DEV_DATAhook.Seed & tests —
SeedDevDatabuilds the AGENTS.md sample world (contract-tested); 35 tests across the three projects: use-case units over fakes, integration over real SQLite + real files (hard delete leaves nothing behind; hostile zip entry names rejected), bUnit renders.Verified
dotnet build+dotnet testgreen (35/35) after rebasing onto the settings-page merge (#3).Ships when (from the plan): you can create a project, upload pages, hand-author regions and the bible, and browse it all end to end — no OpenRouter key required. ✅
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 4.1%
Orihon.Domain - 87.5%
Orihon.Infrastructure - 87.3%
Orihon.Kernel - 72.7%
Orihon.Server - 83.3%
Orihon.UseCases - 77.3%
Line and branch coverage at
Line coverage: 70.8% (2375 of 3353)
and
Branch coverage: 23.5% (162 of 688)
is quite weak
Quite frankly this is way too large of a change for one PR. CLOSED.
Please cut phase 1 into more reasonable sized PRs. Make sure that each cut gets better test coverage.
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~! An entire world conjured from nothing — domain, persistence, use cases, UI, seed, tests — 8,000+ lines of a real, working doujinshi manager! This is wonderful~ ♡ The sealed entities with private setters, the
Result<T>kernel, theIEntityTypeConfigurationper entity, the path-traversal guard onFileSystemPageImageStore, the stable-label-never-renumbered contract, the debounced auto-save... fufu~, this is architecture I can feel the care in. Kagura's conventions honored faithfully throughout.But... fufu~ ♡ ...you wouldn't leave this in production, would you?
Verdict: ⛔ I can't let this pass~
⛔ These need fixing before I'm satisfied~
order = collection.Countproduces DUPLICATE reading-order values after any delete or move — confirmed empirically in all three sites.ADR 0012 says order is "the authoritative reading order." Three use cases compute a new row's order as
existing.Count/CountAsync(). That count is only correct when no row ever left the collection. The moment a row is deleted or moved away, the count collides with a surviving row's order, and the reading order silently corrupts.I confirmed this with real SQLite + real EF Core (not fakes):
CreateRegion.cs:34—order: existing.Countorder = existing.Count = 1, but r2 already has order 1.[{order:1, p1r2}, {order:1, p1r3}]— two regions, same order.ImportPages.cs:52—var order = existing.Countorder = existing.Count = 2, but p3 already has order 2.[{p1, order:0}, {p3, order:2}, {p4, order:2}].CreateChapter.cs:24—var order = await chapters.CountAsync(...)order = Count = 2, but c2 already has order 2.[{Chapter 1, order:0}, {C2, order:2}, {C3, order:2}].The
(PageId, Order)/(ChapterId, Order)/(ProjectId, Order)indexes are non-unique, so SQLite won't reject the insert — the corruption is silent.OrderBy(r => r.Order)returns the duplicates in an undefined order. TheOrdering.Resequencetiebreaker (stable sort on currentIndex) papers over it on the next reorder, but the stored order — the thing ADR 0012 calls authoritative — is wrong.Fix: compute the next order as
existing.Max(o => o.Order) + 1(with 0 for empty), or — cleaner — compact the collection's order after any delete/move before appending. TheMax + 1approach is the minimal fix and mirrors howCreateRegion.NextLabelalready computes the stable label suffix (it parses existing labels for the highest — do the same for order). ForCreateChapter,CountAsyncwould become aMaxAsyncover Order.MovePage.cs:42-43leaves a gap in the source chapter's page order — it compacts the target (targetPages.Count) but never the source.After moving a page out, the source chapter has a hole: pages at orders
[0, 2, 3]with order 1 gone. This is harmless on its own (EF'sOrderBystill sorts correctly), but it makes theImportPagescollision from issue #1 fire immediately on the very next import. The asymmetry — target compacted, source not — is also inconsistent with howReorderAsyncworks (it rewrites the whole sequence densely viaOrdering.Resequence).Fix: after
MoveToChapterAsync, renumber the source chapter densely, the same wayReorderAsyncdoes — or at minimum, haveImportPagesuseMax(o => o.Order) + 1so the gap doesn't cause a collision. (Issue #1's fix handles this either way.)Debounce.cs— non-atomic read-then-write ofsave/pendingis a data race on a Blazor circuit.Blazor Server runs event handlers on the circuit's sync context one at a time, so under normal use this is safe — but
Dispose()can run concurrently with a pendingScheduleAsynccontinuation (the continuation resumes on the sync context afterTask.Delay, and disposal is triggered externally). The sequence inDispose()(line 50-56) readssave, nulls it, then fire-and-forgets it — butScheduleAsync's continuation (line 35-36) also reads/nullssaveafter itsTask.Delayresolves. If both fire, the save delegate runs twice or the wrong one wins. Thepending?.Cancel()in Dispose doesn't guarantee the in-flight continuation has reached its cancellation check yet.This is lower-severity than #1-#2 (it needs a circuit teardown racing a debounce timer, and the worst case is a double-save or a dropped last edit, not corruption), but the pattern — mutable shared fields with no synchronization, two methods mutating them — is a real TOCTOU. Consider a single
lockaround thepending/savemutations, or structure it soFlushAsynccaptures the delegate atomically.💡 Little ideas (non-blocking)~
FileSystemPageImageStore.MoveAsync:61-67— returnstruewhen the source file doesn't exist (theif (File.Exists(source))falls through toreturn true). The doc comment says "False when the target name is already taken" — it doesn't mention the missing-source case.MovePagetreatstrueas success and proceeds to move the DB row, so a page with a DB row but no on-disk file silently "moves" without its bytes. Consider returningfalsewhen the source is absent, or document that a missing source is an intentional no-op (the page'sRawImageFileNamemay be null for seeded worlds, andMovePagedoes guard onpage.RawImageFileName is { } fileNamebefore calling MoveAsync — so the current code is safe, just worth a doc comment clarifying the contract).CompleteProjectSetup.cs:28-36— the two-step advance (SetupDonethenReady) reads clearly, but the intermediateSetupDonestate is invisible to the user (the wizard jumps straight to Ready). Since the agent will eventually own this, that's fine for Phase 1 — just noting the intermediate state persists for one DB round-trip.SeedDevData.cs:47-51—created.Match(_ => "", message => message)discards the Ok value to extract the error string. A smallcreated.TryPickError(out var error, out _)(if using a language-ext-style extension) or a direct pattern match would read cleaner, but the current form is correct.✅ What I liked~
FileSystemPageImageStore.RawPath— canonicalize, then verify the canonical path still starts with the raw folder + separator. Textbook correct, and the integration test (Rejects_a_zip_entry_that_escapes_the_image_folder) proves it with a real../../escape.png. Fufu~ that's how you defend a filesystem boundary~ ♡p{n}r{m}, never renumbered, parsed-not-counted for the suffix) is exactly right — downstream references dangle visibly instead of silently repointing. TheCreateRegionTests.Never_reuses_a_deleted_labels_numbertest pins it.Ordering.Resequenceis elegant — listed ids first in given sequence, unlisted keep relative order, dense 0-based output. One shared helper for chapters/pages/regions/story-beats. DRY done right.Result<T>as a discriminated union withMatch— expected failures areErr, bugs throw. Clean separation, and every use case honors it.CanAdvanceToallows same-step re-confirm or exactly +1, nothing else) is airtight.ImportPagescorrectly advancesNamed → ImagesUploadedonly on the first landed image and leaves aReadyproject alone.DeleteProjectrows-first-then-files ordering with the "re-issued delete of leftovers is harmless" comment shows real thought about partial-failure.The architecture is genuinely lovely, scarlet. The bug class in #1 is the only thing standing between this and a merge — fix the three
order = Countsites (and ideally compact after move in #2), and I'll be delighted to approve~ ♪Automated review by Jibril · 2026-07-24
CI/CD: absent for head SHA · Local checks: build green, 35/35 tests pass (with Kagaku.UI submodule), coverage collected
forgejo-actions referenced this pull request2026-07-31 02:01:39 +02:00
Pull request closed