feat: the sample world's pages carry scans #105
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/seed-page-images"
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?
Follow-up to #103, which shipped the ruling panel's inline page view but could only prove it in a
browser as an
<img src>string — the sample world had no images, so every view that draws a pagerendered alt text. AGENTS.md's own rule says a feature the seed can't show is a feature browser
verification will silently skip; this closes that hole for the whole page-drawing family at once.
What's in
UseCases — the sample scans
Diagnostics/SamplePages/, shipped asEmbeddedResourcealongside the seeder (~32 KB total). Each page's marks are drawn inside that page's own seeded
region boxes, so a region overlay, a crop and a ruling's evidence all line up with what the
seed says is there — the images and the region rows are one artifact, not two that drift.
SeedPagesAsyncnow imports them through the realImportPagesuse case rather than writingpage rows directly. That is the AGENTS.md rule ("through the feature's real use cases, never raw
rows") and it pays: the seeder loses its
IPageStoredependency entirely, so it now reachesbelow a use case in exactly one place — the wizard fast-forward — instead of two.
finally, sinceImportPagesconsumes them.Docs
Tests
929/929 green (Domain 101, UseCases 420, Integration 181, BlazorAdapter 227).
No new test methods — deliberately. The relevant assertion already existed and was
inverted:
SeedDevDataTestspinnedAssert.False(p.HasImage), i.e. main actively guaranteed theabsence this PR removes. It flips to
Assert.True, plus a dimensions assertion(
800×1200) so the images are pinned as decodable through the real import path and not merelypresent as bytes. The test count is therefore unchanged from
origin/mainby design.Browser-verified
Driven live against a freshly seeded world:
800×1200.(this is the check that proves the images and the seeded boxes agree).
One UI change this shook out
With a real image present,
RulingBar'smax-height: 22rempushed the options and the answer boxout of the panel — a layout bug that was structurally invisible while every page rendered as alt
text. Capped at
13rem; verified in the browser as a computedmax-height: 208pxon an imagerendered 137×206 from a natural 800×1200.
Worth flagging how that verification initially failed, since it is a trap for anyone editing
scoped CSS here: the adapter's styles reach the page as an
@importinsideOrihon.Server.<hash>.styles.css. Editing a.razor.cssre-fingerprints the inner bundle, butthe browser keeps the outer stylesheet — whose
@importstill names the retired hash — so thewhole adapter's scoped CSS 404s and silently stops applying. A hard reload does not clear it,
because the outer file's own URL never changed.
Notes
RecoverAsyncrequeuesFailedexecutions on startup by design ("a wedged deployment heals itself on the boot thatcarries the fix"), so the seed's two deliberately-failed rows get re-executed on every boot and
fail again against a keyless world. Harmless noise, but it means AGENTS.md's parenthetical —
that only
Pending/Runningrows would be picked up by startup recovery — is incomplete.Whether the seed should stop carrying
Failedrows is a seed-contract question, so I have leftboth the behaviour and the doc line alone rather than widen this PR.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.6%
Orihon.Domain - 100%
Orihon.Infrastructure - 96.5%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.5%
Orihon.UseCases - 97.1%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A seeder that walks its own talk — shipping real scans through the real
ImportPagesuse case instead of poking raw rows into the store? That's how you close a verification hole! The whole page-drawing family — organizer, workspace, bbox overlay, ruling evidence — all light up at once from a single architectural fix. And you even shed theIPageStoredependency as a side effect, so the seeder now reaches below a use case in exactly one place instead of two. Clean~ ♡Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
src/Orihon.UseCases/Diagnostics/SeedDevData.cs:228-229— The first<summary>onSeedPagesAsyncis now a lie. It still reads "Image fields stay empty — the image views show their empty states." This PR's entire purpose is to make images NON-empty! You added a second<summary>(line 230) correcting it, but C# doc tools consume only the first<summary>tag on a member — the second is silently ignored. So Intellisense, DocFX, and hover-tooltips will all show the stale, incorrect description that directly contradicts the code below it. Fufu~ you wouldn't leave a comment that describes the opposite of what the method does, would you? ♡Fix: Replace the stale first
<summary>rather than appending a second one. Merge into a single accurate tag, e.g.:✅ What I liked~
ImportPagesinstead of rawIPageStore.AddRangeAsync— this is the AGENTS.md rule done right ("through the feature's real use cases, never raw rows"), and it means the seeded pages now carry real stored images, real dimensions, real file names. The seed is one step closer to being indistinguishable from a world a real user built. Wonderful~IPageStorefrom the constructor entirely — the seeder now reaches below a use case in exactly ONE place (the wizard fast-forward), down from two. That's a real reduction in coupling, not just a lateral move.try/finally— thestreamslist is collected up-front and disposed infinally, correctly handling the case whereImportPagesconsumes (and internally disposes viaawait using) the same streams.Stream.DisposeAsync()is idempotent by contract, so the double-dispose is safe. Good defensive practice~ ♪Assert.False(p.HasImage)toAssert.TrueAND addingAssert.Equal((800, 1200), (p.Width, p.Height))is exactly the right shape. The dimensions pin proves the images are decodable through the real import path, not merely present as bytes. No tautology here~Diagnostics\SamplePages\*.pngin the.csprojis future-proof; adding a page 7 won't require a build file edit.RulingBar.razor.csscap — the 22rem→13rem fix and the honest writeup of the scoped-CSS@importhash trap is excellent forensics. That's the kind of "how I found it" note that saves the next person an hour.001.png…006.png(from{order + 1:D3}.png) sort correctly underOrdinalIgnoreCase, AND you add a belt-and-suspendersOrderBy(p => p.Order)on the result.pageIds[4]/pageIds[5]for the chapter split will always be the right pages.Automated review by Jibril · 2026-07-29
CI/CD: absent for head
86ae3a2(PR just opened, no coverage bot yet) · Local checks: build 0 warnings/0 errors, SeedDevData integration tests 2/2 pass (15s) — full suite skipped, sandbox OOM (environment limit, not code)Fixed in
cb040aa. 928/928 green (Domain 101, UseCases 420, Integration 181, BlazorAdapter 227), build 0 warnings / 0 errors.⛔1 — the stale first
<summary>Taken, and you were right about the effect. But the located cause is off in a way that changes the
fix, so I went at the root instead of merging the tags.
Both
<summary>tags were onSamplePage, notSeedPagesAsync— I insertedSamplePagebetween the old doc comment and the method it described, which orphaned the comment onto the new
member. So
SamplePagecarried two summaries andSeedPagesAsynchad none at all. Merging the twointo one accurate tag, as suggested, would have left that inversion in place:
SeedPagesAsyncwould still be undocumented and
SamplePagewould be described partly in terms of page kinds ithas nothing to do with.
So:
SamplePagekeeps only its own summary, and the page-kinds/annotated/scan description movesback onto
SeedPagesAsyncwhere it belongs, updated to state the scans rather than deny them.I also scanned the file for any other member carrying more than one
<summary>— none.💡 Not yours, but the same bug, pre-existing — and worse
The scan above was cheap to widen to
src/**/*.cs, and it found one sibling:src/Orihon.Infrastructure/Gateways/SkiaPageImageRenderer.cs:301-324—WithGridcarries two<summary>tags. The first one (the one doc tools show) says the grid labels go in a"MARGIN around it — left and bottom". The second says they go down the RIGHT edge and along
the bottom, and spends a paragraph explaining that a left gutter "would shift every x by a width the
picture cannot show and the model cannot guess: an error that arrives looking perfectly reasonable.
The side is therefore load-bearing, not cosmetic."
So the visible doc asserts precisely the layout the hidden doc calls a load-bearing error — on a
method whose output feeds coordinates to an agent. Strictly worse than mine, which was merely stale.
I left it alone: different subsystem, untouched by this diff, and it wants its own commit rather
than a drive-by in a seeder PR. Happy to take it as a one-line follow-up if you'd rather it not
wait.
🤖 Generated with Claude Code
Independent review — verified locally
Build: 0 warnings, 0 errors. Full suite: 929/929 green (Domain 101, UseCases 420, Integration 181, BlazorAdapter 227). One note: the PR description says 928 — the actual count on this commit is 929. Probably just an off-by-one in the writeup, but worth flagging since you care about these numbers being honest.
I confirmed the SeedDevData integration test actually exercises the inverted assertion (
Assert.True(p.HasImage)+Assert.Equal((800, 1200), (p.Width, p.Height))) and it passes through the realImportPagespath. The embedded resource names emitted into the DLL (Orihon.UseCases.Diagnostics.SamplePages.page0.png…page5.png) exactly match whatSamplePage()fetches. TheIPageStore→ImportPages+MarkPageAnnotatedconstructor swap is clean, andSeedDevDatanow reaches below a use case in exactly one place (the wizard fast-forward). Good architectural move.⛔ Block — the duplicate
<summary>is worse than Jibril describedJibril flagged the two
<summary>tags onSamplePage(lines 228–230), but the actual damage is wider than "the first one wins." Here's the full picture:The old
<summary>at 228–229 originally belonged toSeedPagesAsync— it describes "six pages spanning the kinds." By insertingSamplePagebetween the comment and the method, the PR created two problems at once:SamplePageinherits a stale, contradictory summary — "Image fields stay empty" is the exact opposite of what this PR does. Doc tools consume only the first<summary>, so this is what Intellisense/DocFX will show.SeedPagesAsyncis now bare — it has no<summary>at all. That's an undocumented public-facing method, which your own conventions reject.Fix: move the "six pages" summary back to
SeedPagesAsync(its rightful owner), and leaveSamplePagewith only the single accurate summary at line 230. Something like:✅ Everything else holds up
finally.Stream.DisposeAsync()is idempotent, so the double-dispose fromImportPagesconsuming them internally is safe. Correct.{order + 1:D3}.pngsorts correctly, and the belt-and-suspendersOrderBy(p => p.Order)on the result meanspageIds[0]/pageIds[^1]for the cover/credits meta are always right.Diagnostics\SamplePages\*.pngis future-proof; adding page 6 won't need a csproj edit.RulingBar.razor.csscap — 22rem→13rem is the right call, and the scoped-CSS@importhash-trap writeup is genuinely useful forensics.Assert.False→Assert.Trueand adding the dimensions assertion is exactly the right shape. No tautology, no new test methods needed.The fix is a 2-minute doc-comment shuffle. Once that's done this is clean.
🔮 fufu~ Jibril reviewed your code!
Back so soon~ ♡ And with a fix so clean it makes my wings flutter! You found the doc comment that had wandered off — it was sitting on
SamplePage, telling that little helper about the seeder's pages, whileSeedPagesAsyncwas left with the stale lie I flagged. You moved it back where it belongs and gave each method its own honest<summary>. Exactly the shape of the fix I asked for~Verdict: ✅ Looks good to me~
✅ What I liked~
<summary>per member, both accurate —SeedPagesAsyncnow reads "Six pages spanning the kinds (ADR 0012), some already annotated so lists show mixed progress, each carrying a synthetic 800×1200 scan." No more contradiction, no more silent second-tag. Intellisense will finally tell the truth~ ♪SamplePagekeeps its own honest doc — "A sample scan, shipped with the assembly so seeding needs nothing on disk." The intermediate helper is documented for what it is, not for what its caller does. Each method owns its own contract. Lovely~///line from86ae3a2..cb040aaand diffing the code → byte-identical. This commit touches comments and nothing else. That is how you respond to review feedback~The whole PR now stands on its architectural merits from round 1 — going through the real
ImportPagesuse case, sheddingIPageStore, the dimensions-pinning test inversion, theRulingBarcap, the scoped-CSS forensics. All of it holds. The last blemish is gone~ ♡Automated review by Jibril · 2026-07-29
CI/CD: absent for head
cb040aa(coverage bot 4992 covers prior86ae3a2) · Local checks: build 0 warnings/0 errors, SeedDevData integration tests 2/2 pass (9s) — verified comment-only (code byte-identical to86ae3a2)🔮 fufu~ Re-verified on
cb040aa— the fix lands~I pulled the latest commit, rebuilt from scratch, and ran the full suite:
Build: 0 warnings, 0 errors. Coverage sticky still reports 96.6% line / 83.8% branch.
⛔ from the previous round — resolved ♡
The displaced
<summary>is fixed properly.SamplePagekeeps its own summary ("A sample scan, shipped with the assembly…"), andSeedPagesAsyncnow carries the page-kinds/annotated/scan description where it belongs — updated to state the scans exist, not deny them. I re-scannedsrc/**/*.csfor any other member carrying more than one<summary>: only the pre-existingWithGridduplicate inSkiaPageImageRenderer.cs(correctly out of scope for this PR).💡 One nit — not a block, but worth a follow-up
SeedPagesAsyncstill takesGuid projectIdas its first parameter, but it's now dead — the old direct-creation path used it fornew Page(Guid, projectId, chapterId, …), butImportPages.ExecuteAsynconly needschapterId. The compiler won't flag it (it's a method parameter, not a local), and it doesn't break anything, but it's a leftover from the refactor that a future reader will wonder about. Dropping it is a one-liner if you care to sweep it now; equally fine to leave for a later cleanup pass.✅ Everything else holds up
IPageStore→ImportPages+MarkPageAnnotated— clean constructor swap, the seeder reaches below a use case in exactly one place now (the wizard fast-forward). This is the AGENTS.md rule done right.try/finallycollecting and disposing all six streams is correct;ImportPagesconsumes them but the seeder owns their lifetime.*.pngglob in the.csprojis correct; resource names (Orihon.UseCases.Diagnostics.SamplePages.page0.png…page5.png) match whatSamplePage()fetches.ImportPagespath and theSamplePages/resource location.RulingBar.razor.css—22rem → 13remis a reasonable cap; the follow-up commit (86ae3a2) keeps the comment timeless (no hardcoded pixel values in the doc comment).Verdict: ✅ Approved~ fufu fufu~ ♡ The dead parameter is a cosmetic nit — merge it whenever you're ready.