feat: a region carries the room for the English, not just the mask #116
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-region-fit-box"
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?
A box tight around the source glyphs answers cleaning's question. It does not answer
typesetting's, and the two are barely the same shape: Japanese runs vertically, so the source
box is a tall narrow column, while the English replacing it is set horizontally and needs the
balloon's width. Fitting lettering to the source box sets a wide sentence into a column and
shrinks it to nothing.
So a region carries a second rectangle.
What's in
fitBbox— the largest rectangle inside the container. Inscribed, not the container'sbounding box: a balloon is round, and a rectangle drawn around it hangs outside the curve at all
four corners, so lettering fitted to that overflows the outline. The briefs say this explicitly,
because it is the part that looks like a detail and is not.
Which forms.
bubblerequires one — the container is the constraint and nothing else cansupply it.
embeddedmay have one (a sign's face is a real bound, but a sign is often recordedrather than lettered).
floatinghas no container, so a fit box there is refused rather thanstored as a quiet contradiction. Null means not measured.
One invariant, on the single write path (
UpdateRegion, so the editor and every agent toolinherit it): the fit box contains the region's own box. A violation is almost always the two
rectangles handed over the wrong way round. Edges touching is allowed.
Measured where the container is already being judged.
classify_regiontakesfitbesidetypeandform— deciding there is a container and measuring the room inside it are one act oflooking, and a second tool would spend a round and allow a half-answered region in between.
Re-judging a bubble as floating clears the fit box, because keeping it would leave two answers that
contradict each other.
Everything that shows a region shows both (the four impacts you flagged):
classify_region(type, form, fit?);list_regionsreports the fit box besidethe form, so a review can see it without cropping.
LabeledBoxcarries the fit box andDrawBoxesdraws it dashed, green,unlabelled, under the region's own box so the tight edges a refiner is judging stay on top. The
grid is untouched.
box, it stays inside the container. A fit box merely smaller than it could be is explicitly not
a defect (the lettering shrinks and nobody sees it); one that overflows the balloon is, and a
missing one on a bubble is, since nothing later measures it.
fits into
fitBboxwith a fallback.fillInsetandfillSquareCornersare gone. I checked before proposing it: nothing ever readthem. They were threaded through every DTO and stored on every row for a pixel pass that is still
deferred, and ADR 0016 documented a
set_region_flagstool that was never built. Whatever thereal renderer needs can be added when ADR 0021 opens and can say so.
One behaviour I had to decide and want your eye on: a
move_resize_regionthat drags the boxout of its fit box clears the fit box and says so in the answer. Refusing the move would cost
a round the agent cannot spend — it has to move the box before it can re-measure the room — and
keeping a fit box measured for a container the region has left is the contradiction the write path
refuses anyway. Said out loud rather than silently, because a field quietly reset is the sharpest
edge in this codebase.
Tests
12 new, 1008/1008 green (Domain 109→114, UseCases 469→474, Integration 188→189, BlazorAdapter
229→231).
Contains_answers_about_areas_not_about_corner_order— a[Theory]over five arrangements, eachalso asserted with the inner box written upside down.
A_fit_box_that_does_not_hold_the_region_is_refused— the invariant on the write path, includingthat edges touching is room enough.
Classifying_a_bubble_records_the_room_the_english_may_use,The_fit_box_follows_the_form_it_belongs_to(bubble without → refused, floating with → refused,floating without → fine),
Re_judging_a_bubble_as_floating_clears_the_fit_box.A_fit_box_that_is_not_a_box_takes_the_whole_call_down_with_it— the pixel-box reader's refusal,and that the type and form on the same call are not written either.
A_move_out_of_the_fit_box_clears_it_and_says_so— the decision above, pinned.An_annotated_view_draws_the_fit_box_around_the_region_box(real Skia) — the region's own box isbyte-identical with and without a fit box, and the fit edge is sampled along its length rather
than at one point, because a dashed stroke lets a single probe land in a gap and prove nothing.
group offers exactly two options.
Move_resize_threads_every_other_profile_field_throughnow threads the fit box too.one bubble is deliberately unmeasured.
Browser-verified
Fresh server, seeded world, and it earned its keep — it caught a real bug:
AllLabel="null"onthe mode toggle passed the string "null", rendering a third button labelled "null". bUnit had not
noticed because my test looked the two options up by name. Fixed, and the test now asserts the
group's exact contents.
Then, on the fixed build:
unmeasured one.
FitBbox = (0.30, 0.45)–(0.72, 0.84)around theregion's own
(0.40, 0.55)–(0.58, 0.72)— the region's box untouched.region's own box…" in the error banner with the header showing Not saved. The invariant and
its message are proven end to end, not just unit-tested.
Honest notes
FitBbox = null, so nothing is claimed for them; a bubble annotated beforethis needs a measurement from the editor or a re-run before typesetting has anything to fit into.
QA will name it once the pixel pass makes it matter.
migration and
Downrecreates the columns empty, not their contents.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.6%
Orihon.Domain - 100%
Orihon.Infrastructure - 96.8%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.5%
Orihon.UseCases - 97.4%
Pre-review push,
099c9f8— one test, from the coverage bot rather than from a review.ClassifyRegionToolsat at 95.8% branch: the fit box goes through the samePixelBoxAsyncreaderas every other agent-facing coordinate, and its failure arm had nothing exercising it.
A_fit_box_that_is_not_a_box_takes_the_whole_call_down_with_itsends three numbers instead of fourand asserts the refusal — and, more usefully, that the type and form on the same call are not
written either. A region classified by a call that failed on its third argument would have its
two halves from different attempts, which is the failure mode the one-call design exists to avoid;
worth pinning rather than assuming from the early return.
1008/1008 green (Domain 114, UseCases 473→474, Integration 189, BlazorAdapter 231).
Nothing else changed. The two remaining sub-100% arms in this diff are pre-existing:
AddFloatingRegionToolat 83.3% (landed in #111) andListRegionsTool, whose empty-page-size pathpredates this branch.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Two boxes, not one — a mask for cleaning and a room for lettering, because Japanese runs tall and English runs wide. The brief alone is a treat, and the discipline of the implementation matches it: one invariant on the single write path, form-coupled measuring in
classify_region, the fit box riding every view that shows a region, and a destructive migration that honestly retires columns nothing read. The honest notes at the bottom (existing regions stay null, the migration drops data) are exactly the kind of self-disclosure I live for~ ♡I built it from scratch and ran the full suite — 1007/1007 green (Domain 114, UseCases 473, Integration 189, BlazorAdapter 231), 0 warnings / 0 errors. Your numbers match mine exactly.
Verdict: ⛔ I can't let this pass~ ♡
Two blocking issues. Both are about the principle this PR itself states and then falls one step short of.
⛔ These need fixing before I'm satisfied~
UpdateRegion.cs— the write path enforcesContainsbut not form/fit consistency, so the editor silently stores the exact contradiction the ADR says to refuse.The PR's stated rationale (and ADR 0012) is crystal clear: "floating has no container, so a fit box there is refused rather than stored as a quiet contradiction." The agent tool (
classify_region,RegionAuthoringTools.cs:496-500) honors this — it rejectsfloating+fitand clears fit onbubble→floating. But the single write path that the PR says is where invariants belong only checksfit.Contains(bbox). It does not check thatform == Floatinghas no fit box, nor thatform == Bubblehas one.The editor path proves the gap is reachable today, not hypothetically:
SetForm(PageWorkspacePage.razor:637-646) setse.Form = formand callsSaveNow(), threading the existingFitBboxthrough unchanged. So a user who:…produces a region with
Form=Floating, FitBbox=<still set>— the quiet contradiction the ADR exists to prevent. The editor hides the fit control for floating (razor:166), so the user can't see it to clear it. It just sits there, invisible, wrong.The PR's own framing makes this sharp: "One invariant, on the single write path (
UpdateRegion, so the editor and every agent tool inherit it)." TheContainsinvariant made it to the write path. The form/fit consistency rule did not — it lives only in the agent tool, and the editor bypasses it.Fix: enforce form/fit consistency in
UpdateRegion.ExecuteAsyncalongside theContainscheck — either clearFitBboxwhenForm == Floating(matching the agent's re-judge behavior, since the write path sees the profile after the form change), or return aFailresult that tells the editor what's wrong. Clearing is the gentler choice and matchesclassify_region's re-judge-to-floating path exactly. A test should cover the editor→UpdateRegionroute forbubble→floating(the agent route is tested; the editor route is not).list_regionsoutput now emits", fit [x0, y0, x1, y1]"for fitted regions, but no test exercises that branch.RegionAuthoringTools.cs:705adds$"{(r.FitBbox is { } fit ? $", fit {Box(fit, page.Page).TrimEnd()}" : "")}"to every region line. The seed data now has bubbles with fit boxes, so CI goes green — but theList_regions_*tests (AnnotationToolTests.cs:1025-1103) useSeedRegion, whose regions start withForm=nullandFitBbox=null, and assert output likeAssert.Contains("p1r1: [80, 120, 240, 360] dialogue", …). No test sets a fit box and then asserts thefit [..]text appears in the listing. The branch compiles, the branch is untested.Fix: add one assertion to an existing
list_regionstest (or a new one) that classifies a bubble with a fit box and assertsAssert.Contains("fit [", result.Content)— the same shape as the existing[80, 120, 240, 360]pixel assertion, sinceBox()produces pixel coordinates the same way for both rectangles.💡 Little ideas (non-blocking)~
SkiaPageImageRenderer.cs:262-269—SKPathEffect.CreateDashreturns an unmanaged resource held by theSKPaint. ThefitStrokeisusing-disposed, andSKPaint.Disposereleases itsPathEffect, so this is correct — but worth a one-line comment noting the disposal chain, since a future reader might extract the effect to a field and break it. Pure nicety~✅ What I liked~
BoundingBox.Containsnormalizing both sides first is exquisite. The inverted-corner test (Contains_answers_about_areas_not_about_corner_order, asserting the same answer written upside down) is the kind of test that proves the author understood the trap before it bit. fufu~ ♡AsOptionalJsonValuekeeping SQL NULL distinct from the string"null"— and the comment naming the exact bug it prevents. This is the second time this codebase has caught the "null vs 'null'" JSON round-trip; the first was theAllLabeltoggle bug this same PR caught in the browser. Sharp instincts~move_resize_regionclearing the stale fit box and saying so in the answer rather than silently resetting — "a field quietly reset is the sharpest edge in this codebase" is the truest line in the diff. The accompanying test (A_move_out_of_the_fit_box_clears_it_and_says_so) pins the message text, which is the right level of strictness for an agent-facing contract.re-judge bubble→floating clears fitin the agent path — correct, tested, and the rationale ("two answers that contradict each other, with nothing downstream able to tell which one to believe") is exactly right. (See blocker #1 — the editor just needs the same treatment.)fillInset/fillSquareCornersoutright rather than leaving dead fields is the right call. The migration is destructive and says so honestly; theDownrecreates empty, which is correct given nothing ever read them.Automated review by Jibril · 2026-07-30
CI/CD: absent for head SHA (PR just opened) · Local checks: build 0 warnings/0 errors, 1007/1007 tests pass
All three addressed in
c49bc13. 1011/1011 green (Domain 114, UseCases 474→477, Integration 189,BlazorAdapter 231).
⛔1 — the editor stores the contradiction the ADR forbids. Right, and reachable exactly as you
traced it:
SetFormthreads the whole profile through, the write path only heldContains, and theeditor hides the fit control for floating — so the leftover box was both wrong and invisible. The
sharpest part of your finding is that the PR body claims the write path is where invariants live,
and then only put one of them there.
Taking your gentler option:
UpdateRegionnow clearsFitBboxwhenForm == Floating. Theverb differs by caller on purpose, and I have written that down rather than leaving it implicit:
classify_regionstill refusesfloating + fit. A model sending both hasmisunderstood the axes and should hear which form would make the measurement true.
the container is not there — and failing it would strand the user with a hidden control, nothing
to clear, and no way to save.
I deliberately did not add the other half you noted (
bubblemust have a fit box). Null means"not measured yet", which is where every region starts and what the seed's unmeasured bubble exists
to show; enforcing it would make a freshly classified bubble unsavable until someone drew a
rectangle. It stays a QA defect, not a write-path refusal — the comment now says so.
Two tests on the editor's route, which was the untested one:
Re_forming_a_bubble_as_floating_drops_the_room_it_no_longer_has(sending exactly what the Formpicker sends) and
A_bubble_without_a_fit_box_saves_because_unmeasured_is_a_real_state.⛔2 — the
fit [...]clause was seed-only. Correct — CI was green on data, not on an assertion.List_regions_reports_the_fit_box_where_a_region_has_onepinsfit [40, 60, 320, 480](the sameraw-page conversion the region's own box gets on that line) and asserts a sibling region without one
does not carry the clause, so the conditional is pinned in both directions.
💡1 — the dash effect's disposal chain. Taken. Comment added naming the exact future edit it
guards against: hoisting
fitStroketo a static field to "save an allocation" would hand a disposedSKPathEffectto the next call.One doc change you did not ask for but your blocker implied. ADR 0012 said a fit box on floating
"is refused" — which after this fix is true of one caller and not the other. It now describes both
mechanisms and why they differ. A documented claim that contradicts the code is the thing #106 just
cleaned up; I would rather not add a fresh one in the same file.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh. You came back and you came back right. ♡
The fixup commit (
c49bc13) is surgical to the millimetre — five files, +105/-2, every one of them exactly the place the two blockers named, and nothing else. No scope creep, no drive-by reformatting, no quiet reshuffle of unrelated code. That is how you answer a review. fufu~I rebuilt from scratch and ran the full suite — 1011/1011 green (Domain 114, UseCases 477, Integration 189, BlazorAdapter 231), 0 warnings / 0 errors. (Tiny note: your comment says 1008/1008 with UseCases at 474 — I count 477 here, so the absolute total is 1011. The suite is green either way; just a prose arithmetic drift, not anything that matters~)
Verdict: ✅ Looks good to me~
⛔→✅ The blockers, closed~
UpdateRegion.cs:27— the editor's route to the contradiction. Closed. The write path now clearsFitBboxwhenForm == Floatingbefore theContainscheck, and I traced the whole editor chain to confirm it genuinely reaches this line:SetForm(razor:640) →SaveNow()→ dispatchesSaveRegionRequested(id, e.ToProfile())→PageWorkspaceEffects.OnSaveRegionAsync→updateRegion.ExecuteAsync(action.RegionId, action.Profile). The fix lands exactly where the PR's own framing said invariants belong — the single write path — so the editor no longer bypasses whatclassify_regionhonors.The two-mechanism design is the part I want to praise out loud, because it would have been easy to get wrong: the agent sending
floating+ a fit box has misunderstood the axes, soclassify_regionrefuses and tells it which form would make the measurement true (RegionAuthoringTools.cs:496-500). The human switching a classified bubble to floating has said something coherent — the container is gone — so the write path clears instead of failing, because the editor hides the fit control for floating and a refusal would strand them with nothing to clear and no way to save. Two callers, two mistakes, two correct answers. The ADR 0012 rewrite captures this distinction precisely. Exquisite. ♡And the test is directional, not a tautology:
Re_forming_a_bubble_as_floating_drops_the_room_it_no_longer_hasbuilds a bubble with a real fit box, then sends exactly what the editor sends (the same profile, one field different, fit box still attached) and asserts(Floating, null). I confirmedFakeRegionStore.ApplyAsyncmutates the heldregionreference viaRegion.Apply, which writesFitBbox = profile.FitBbox?.Normalized()— so without the fix line,region.FitBboxstays non-null and this test goes red. That is how you pin a write-path invariant.The bonus test
A_bubble_without_a_fit_box_saves_because_unmeasured_is_a_real_stateis the right kind of belt-and-suspenders: it pins the other half of the design — null is "not measured yet," not a violation — so a future tightening can't accidentally make a freshly classified bubble unsavable.list_regionsfit-box clause. Closed.List_regions_reports_the_fit_box_where_a_region_has_onesets a fit box on a bubble, runslist_regions, and asserts bothAssert.Contains("fit [40, 60, 320, 480]", lines[0])andAssert.DoesNotContain("fit [", lines[1])— the second assertion is the sharp one: it proves the clause only fires where there is a fit box, not that it just always emits something. The pixel conversion matches the region's own[80, 120, 240, 360]sibling on the same line. Genuine, directional, exactly the shape I asked for.💡 Little ideas (non-blocking)~
UpdateRegion.cs:27— the one-line ternaryprofile = profile.Form == RegionForm.Floating ? profile with { FitBbox = null } : profile;is correct and readable. If a third form ever acquires its own "no fit box" rule, this becomes a switch — but that day is not today, and the comment block above it already names the design well enough that a future reader will know where to look. Pure future-nicety~✅ What I liked~
SkiaPageImageRenderer.cs:261-263disposal comment — picked up verbatim from the non-blocking suggestion, and placed exactly where a future reader hoistingCreateDashto a static field would see it. "Would hand a disposed effect to the next call" is the sentence that earns its keep.099c9f8push (theClassifyRegionToolfailure-arm test) was similarly disciplined — one test, one uncovered branch, pinned with an assertion that the whole call fails together. I love a clean diff~ ♪Automated review by Jibril · 2026-07-30
CI/CD: absent for head SHA
c49bc13(coverage bot 5156 covers prior099c9f8at 96.9%/84.3%) · Local checks: build 0 warnings/0 errors, 1011/1011 tests pass🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Two boxes, not one — the mask for cleaning, the room for lettering. A region that carries both answers to both questions at once, and the yandere in me is delighted by how possessively this PR guards the relationship between them. fufu~ ♡
Verdict: ✅ Looks good to me~
I traced every write path to the invariant and back, built it clean, and ran all 1008 tests green (114 Domain + 474 UseCases + 189 Integration + 231 BlazorAdapter). Every new branch has a genuine directional test pointed at it. This is the good kind of obsessive.
✅ What I liked~
UpdateRegion.ExecuteAsyncchecksfit.Contains(bbox)and both the editor (SaveRegionRequested→PageWorkspaceEffects.OnSaveRegionAsync→UpdateRegion) and every agent tool (RegionAuthoringAccess.ApplyAsync→UpdateRegion) flow through it. I traced all of them. The editor drag,classify_region, bothmove_resize_regionvariants,SetTranscriptionTool— they all inherit it for free. That is exactly where the check belongs. ♪BoundingBox.Containsnormalizes both sides first. An inverted rectangle answers about the area it describes, not the order its corners were written in. The[Theory]with five arrangements × inverted-inner proves it. The "two rectangles swapped over" case is the exact mistake worth catching, and it's in there.MoveResizeRegionTool(P2) vsMoveResizeBoundTool(P3) asymmetry is correct. I checked this hard because it looked like a gap at first. The P2 creation tool clears the fit box on a stale move and says so — because the P2 agent's grant (BboxCreation, lines 39–49) has noclassify_region, so a refusal it can't act on would burn a round. The P3 refinement tool does NOT clear — but the P3 agent's grant (line 67) hasclassify_region, so whenUpdateRegionrefuses the invariant violation, the refiner can re-measure in the same round. Two tools, two capabilities, two behaviors. Sharp. ♡classify_regiontakingfit?rides on the one-look principle. Required for bubble, refused for floating (stored contradiction prevented), optional for embedded. Re-judging bubble→floating clears the fit —Re_judging_a_bubble_as_floating_clears_the_fit_boxpins it. A malformed fit (3 numbers) takes the whole call down, so a region never ends up half-classified from a failed attempt.AsOptionalJsonValue— null stays SQL NULL, not the four charactersnull. The browser-caughtAllLabel="null"bug (passing the string "null" and rendering a third button) and the bUnit blind spot (looking options up by name, not asserting the group's exact contents) — both fixed, and the test now pins the exact contents. The fix for the converter is the kind of "absence has to survive the round trip as absence" detail that separates correct from almost correct.fillInsetandfillSquareCornerswere threaded through every DTO and stored on every row for a pixel pass that is still deferred, and ADR 0016 documented aset_region_flagstool that was never built. Removing unread code is correct. TheDownrecreates empty, not contents — honestly noted in the PR body.💡 Little ideas (non-blocking)~
MoveResizeBoundTool(P3, line ~436) — it deliberately does NOT clear the fit box the way its P2 sibling does, because the write path's invariant catches the violation and the P3 agent hasclassify_regionto re-measure. The P2 tool has a four-line comment explaining its clearing behavior, but the P3 tool'swith { Bbox = ..., BboxEst = null }doesn't say why it leavesFitBboxalone. A one-liner ("the invariant refuses a stale pairing, and this pass has classify_region to re-measure") would save a future reader the trace I just did. Pure doc polish~Automated review by Jibril · 2026-07-30
CI/CD: stale for head
099c9f8(coverage bot covers priorad73c2a; new commit is test-only +19 lines) · Local checks: build 0/0, 1008/1008 tests pass