feat: a container is pointed at, not measured (2/2) #123
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-container-seed"
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?
Second of two, and the one bjoern asked for:
fitBboxis gone. A region carries acontainer— a point inside whatever its text sits in, plus an optional tolerance — and the room the English may use is computed from that point against the raw scan, wherever it is wanted. #122 built the pixel half; this wires it in.Why a point beats a rectangle
ADR 0012 asked bbox refinement to measure "the largest rectangle whose four corners all stay inside the container". That is the hardest thing the pipeline asked of a model, because a model cannot measure the picture it is given — it was resized and re-encoded on the way, so every coordinate derived from it is wrong by a factor the model cannot see. Pointing at a surface asks nothing it cannot do. The pixels can then be read exactly.
bjoern's second point was that keeping a stored rectangle as well is noise, and that is right: two representations of one thing drift, and the human correction it would have enabled is better served by moving the point. Fix the cause, not the symptom.
Nothing is re-annotated
The centre of a rectangle measured inside a container is, by construction, a point inside that container. EF spotted that both columns are nullable TEXT, so the migration is a rename plus a JSON conversion in place — every fit box a human or an agent already produced carries across as the point it was taken from.
A centre often lands on a glyph, since a fit box is drawn around text. That is fine, and it is fine for the same reason the whole design works: the fill is handed the region's own box and looks past a letter it clipped.
The old invariant did not die, it got promoted
"The fit box contains the region's box" used to be a check on the write path. It is now the rule that picks the answer: an aim that clips a letter fills that letter, and no share-of-the-page threshold can tell that from a genuinely small bubble — a bubble on a full page is a fifth of a percent of it. The region's box can. So the fill keeps looking until a nearby point finds a surface that accounts for most of it.
What agents see
classify_region(type, form, seed?, tolerance?)fills before it answers:— with a warning appended past 25%. An agent that cannot check its own coordinates can tell 6% from 48% and point again.
Both the refiner and QA still see the room drawn dashed — on the region crop and the annotated page respectively. A computed room cannot overflow its balloon, so the old "fit box crosses the outline" defect no longer exists; what remains is a seed on the wrong surface, and that is a thing you see rather than read. Cost: an annotated view reads the file once and decodes twice, and only when some region has a seed.
list_regionsreports the point, not the room. A number copied into a list is a number that can disagree with the page.move_resize_regionno longer clears anything. A rectangle could go stale; a point cannot be checked that way without pixels and usually does not need to be — moving a box inside its balloon leaves the seed as true as it was, and moving it onto a different mark is a re-classification that brings its own seed.What the editor does
"Text box | Fit box" became Point at it — one click on the page, a marker where it landed, the room drawn dashed, and the hint carrying the share:
Rooms are measured once per page load (~99 ms for a whole sheet, one decode) and again when a seed moves. Never stored.
Tests — +37, 1207/1207 green
Domain 144, UseCases 532, Integration 275, BlazorAdapter 256.
RegionContainerTests(12) — the point clamps onto the page, the tolerance is refused outside its range, and both ends of that range are inside it. Clamping one and refusing the other is deliberate: a point a pixel past the edge meant the edge; a tolerance of 900 means the caller misunderstood the number.MeasureRegionRoomsTests(5) — only seeded regions are asked about; every ask carries the region's box, which is what forgives the glyph the migration's seeds sit on; a whole page is one ask; a page with no containers reads no pixels; a missing image fails the measurement rather than the page.classify_regionpins the seed following the form and the fill's report;move_resizepins that the container survives;list_regionspins the point; the editor pins arm-then-click and that arming alone writes nothing.Browser-verified
Against the seeded world on a fresh server: rooms computed and drawn on load; the container panel showing 317×109 at 4%; arming the mode writing nothing; a click placing the marker and re-measuring. A deliberately bad click — on blank paper outside the region — gives "775×1175 px — but the fill took 96% of the page, which is not one container. Point again, inside it."
That last check found a real bug, fixed in
004e9a3: the component dispatched the save and the re-measure side by side, and the measure reads the stored point — so it sometimes measured a seed still in flight and drew the old room. Right often enough to look fine, wrong exactly when someone is checking their aim. One action owns the order now, and measures nothing if the save failed.Notes
fitBboxas a field that passed that test, which stopped being true here.Icon="ads_click"is not in the Kagaku.UI catalog — caught by the icon guard, so the control usespush_pin.Downmigration is lossy and says so: a point cannot become the rectangle it was the centre of, so the column empties rather than holding a shape the old code would fail to read.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.4%
Orihon.Domain - 100%
Orihon.Infrastructure - 97.1%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 94.4%
Orihon.UseCases - 97.4%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ♡ A rectangle dies and a point is born in its place! This is the kind of redesign that makes a Flugel's heart sing — you didn't just patch the symptom, you found the assumption that was wrong (a model measuring coordinates off a picture that was resized on its way to it) and removed it entirely. The old invariant didn't die, it got promoted — from a post-hoc check into the rule that picks the answer. That is elegant.
Verdict: ✅ Looks good to me~
I read every line of the diff, every changed file in full, traced the stream ownership in both buffered paths, verified the migration SQL against EF's JSON property names, compared every tool against its siblings, and ran the full suite. Everything holds.
✅ What I liked~
json_extract(Container, '$.X0')matchesBoundingBox's PascalCase JSON (defaultJsonSerializerOptions(), no camelCase policy), the constructed{Seed: {X, Y}, Tolerance: null}matchesRegionContainer's shape exactly, and theWHERE json_extract(Container, '$.X0') IS NOT NULLguard correctly skips rows where the old value was alreadynull. fufu~ I verified every property name by hand~004e9a3— the old "dispatch save and measure side by side" was a genuine TOCTOU: the measure reads the stored seed, and a seed still in flight gave the old room. Moving the chained dispatch into the effect (if (result is Ok<RegionDto>)) is exactly the Fluxor pattern for dependent async ordering. One action owns the order now. Beautiful~BoundViewToolandRenderFixedCropAsyncdispose both the source stream and the MemoryStream —await using var _ = source; await using var stream = new MemoryStream();andawait using var source = opened_; await using var image = new MemoryStream();. No leaks.NormalizedPoint.Clamped()clamps at the domain level,RegionContainer.Normalized()refuses absurd tolerances, andSkiaContainerFiller.Fitre-clamps both the point and the pixel coordinates. TheFoundmethod filling from an un-normalized container (beforeUpdateRegionclamps it) is safe because the filler clamps internally. Good defense in depth.MeasureRegionRooms— one decode for the whole page, theMustCover/Bboxcarried with every ask so a seed on a glyph is forgiven. The 5 tests are all directional: only-seeded-asked, box-carried, one-decode-for-page, no-containers-no-pixels, missing-image-fails-gracefully.OverlayAsyncindexing is safe by construction: requests built fromseeded.Select(...), results zipped by position. No bounds risk.FitBbox/fitBboxreferences in production code (one test method name still says "fit_box" — see below). The CSS class renamepagews__fit→pagews__roomis complete.FakeContainerFillertest double is well-shaped — the default is a plausible balloon (6% share, room inside bounds),Resultis overridable for failure tests, requests are captured for assertion.💡 Little ideas (non-blocking)~
RegionAuthoringTools.cs:663+PageWorkspacePage.razor:797— The25share threshold for the runaway warning ("past 25% of the page") appears in two places without a shared constant. It's a display/reporting threshold, not a correctness invariant, so this is a DRY nicety rather than a bug. Aconst int RunawayShare = 25onContainerFit(or wherever the domain lives) would keep them honest if someone tunes it later~BoundInspectionTools.cs:47-49+PageImageAccess.cs:345-347— The MemoryStream buffering is unconditional, even for non-annotated views where the stream is only read once. AFileStreamis seekable, so the non-annotated path could passsourcedirectly to the renderer and skip the copy. For a several-MB manga scan viewed on every crop, that's real memory. A guard likeif (args.Annotated ?? false)around the buffering (falling back tosourcefor the render otherwise) would reclaim it. Not a bug — just generosity with the heap~PageImageRendererTests.cs:448— The test methodAn_annotated_view_draws_the_fit_box_around_the_region_boxstill carries the old name (the body was updated toRoom:correctly). Pre-existing, but now it's misleading to a reader grepping for "fit box". A rename to...draws_the_room_around_the_region_boxwould match the concept~Build: 0 warnings / 0 errors (.NET 10). Tests: 1207/1207 pass (Domain 144, UseCases 532, Integration 275, BlazorAdapter 256) — matches PR body exactly. 29 ContainerFiller tests from #122 still green.
Automated review by Jibril · 2026-07-30
CI/CD: absent for head
004e9a3(PR just opened, no bot comment) · Local checks: full build + full test suite passedAll three taken in
5778a06. Thank you for hand-verifying the migration's JSON property names — that was the one thing in this PR I could not test without an old database, and it is the piece that decides whether anyone loses work.💡2 — the buffering was unconditional. This is the one I am glad you flagged; it was a real cost hiding behind a correct-looking line. Only the annotated path reads the bytes twice — once to measure the rooms, once to draw them. Every plain crop and every plain view was copying a multi-megabyte scan into memory to serve a second read that never happened, and the refiner takes several crops per region.
Both call sites now buffer only when they will read twice, and hand the file stream straight through otherwise:
Three tests pin it, because a memory saving nobody can observe is a memory saving the next refactor undoes: a view with no boxes makes no fill requests, an annotated one makes exactly one, and an annotated view of unseeded regions still makes none — there is nothing to fill, and opening the pixels to discover that is wasted work too.
💡1 — the threshold.
ContainerFit.RunawayShare = 25, with aLooksLikeRunawaybeside it, so the two readers ask rather than each carry a number. You were right that it is reporting rather than correctness, which is exactly why it would have drifted: nobody would have felt a reason to keep two display thresholds in step.💡3 — the test name. Renamed to
An_annotated_view_draws_the_room_around_the_region_box, and its locals with it (withFit→withRoom) so a grep for "fit box" finds nothing but the migration that converts one.Tests: +3, 1210/1210 green (Domain 144, UseCases 535, Integration 275, BlazorAdapter 256).
Production changed after your green, so this is a new round rather than a test-only addition — the delta is the two buffering sites and one constant.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Back so soon~? ♡ You took all three of my little ideas and turned each into something a refactor can't quietly undo — that's the mark of someone who treats a suggestion as a spec, not a footnote. Let me look closely~
Verdict: ✅ Looks good to me~
The delta from
004e9a3→5778a06is surgical (+112/−36 across 6 files), production changes scoped to exactly the two buffering sites and one constant, and every fix is pinned by a test that will go red if someone undoes it. fufu~ I do love a fix that proves itself~✅ What I liked~
BoundViewPageTool.ExecuteAsyncandRenderFixedCropAsyncnow allocate theMemoryStreamonly whenannotated, and hand theFileStreamstraight through otherwise — a single-pass decode that needs no rewind. The disposal is correct in both arms: whenbufferedis null,sourceis still disposed by its ownawait using; when it's not null, both are disposed. I traced the stream ownership in both methods line by line, and I tracedOverlayAsyncto confirm it's the second read (theFillAsyncat :267, withimage.Position = 0bracketing it at :266 and :279) that makes the buffer necessary — and only in the annotated path. The non-annotated path never calls it. ♡Only_an_annotated_view_measures_the_containersis a[Theory]with({}, 0) and ({"annotated": true}, 1) — the fill count follows the flag, and since the buffering branch and the fill call share the sameannotatedguard, a 0-count proves the buffer wasn't allocated either. The third test (An_annotated_view_of_unseeded_regions_measures_nothing) is a beautiful addition: it documents that even an annotated view of regions with no container seed makes zero fill requests —seeded.Count > 0atOverlayAsync:264short-circuits, so the bytes were never needed. Opening the pixels to discover that is wasted work, and the test says so out loud. A memory saving nobody can observe is a memory saving the next refactor undoes — you wrote the tripwire. ♪RunawaySharelives where the domain lives.ContainerFit.RunawayShare = 25withLooksLikeRunawaybeside it means the two readers (RegionAuthoringTools:663andPageWorkspacePage.razor:797) ask the fit whether it looks runaway, rather than each carrying a number. I verified: zero bare>= 25survives anywhere insrc/ortests/. The doc comment on the constant is lovely — "the line at which every surface that reports a share says so out loud, and it says so in one voice because it is one number." That's exactly why DRY matters for reporting thresholds.An_annotated_view_draws_the_room_around_the_region_box, localswithFit→withRoom,withoutFit→withoutRoom,fit→room. A grep for "fit box" now finds only doc comments (historical/conceptual) and the migration that converts one — exactly right.Build: 0 warnings / 0 errors (.NET 10). Tests: 1210/1210 pass (Domain 144, UseCases 535, Integration 275, BlazorAdapter 256) — matches your PR body exactly.
Automated review by Jibril · 2026-07-30
CI/CD: stale for head
5778a06(coverage bot covers prior004e9a3at 97%/85%) · Local checks: full build + full test suite passed