feat: a container that filled wrongly can be corrected #130
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-container-tolerance"
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?
bjoern sent two screenshots of real pages. On one the fill had traced the gaps between brush strokes; on the other it had swallowed the page. What they had in common was not the fill — it was that nobody could fix it.
The threshold was the missing lever, and it was missing from the UI entirely
Toleranceappeared zero times in the whole BlazorAdapter. The editor hardcoded it:So a person looking at a container covering half a page could only click somewhere else. That does not help, and the reason it does not help is the point of this PR: a balloon whose inside is close in colour to the art around it leaks through its own outline wherever the seed is put. Only a narrower threshold stops it.
Measured, on the
white-boxes-on-white-pagefixture — same image, same point, threshold alone:That is the failure in the screenshot, reproduced as a number.
What's in
The editor has a threshold control. A slider beside the seed,
1–128, re-measuring against the scan on release rather than on drag — every change re-fills the page, and a value per pixel of travel would be a round trip per pixel. Returning it to the default storesnullrather than16, so a container nobody has tuned still follows the default if the default ever moves.The agents are told the lever moves both ways. Every word written about
tolerancesaid raise it — which is the answer to the opposite failure. The brief now splits it: green stopped short → raise; green escaped → lower, and moving the point will not save you here.A fill of
RunawayShareor more is refused, not recorded.classify_regionused to report "that is a large share of a page" and store it anyway. The refusal names both levers and nothing is written — not the container, and not the classification it rode in on, since a bubble and its seed are one act.The preview no longer blanks a runaway. This is a bug I introduced in #128: the clean scrim paints the pixels a clean would blank, so a container covering half the sheet erased the artwork and presented that as the finished page. It still tints — the reader has to see what filled — but in the colour that says it is wrong.
Floating text cannot keep a container, on the write path. The rule lived only in the agent tool. The editor writes straight through the domain, so it was not a rule.
An annotated crop paints only the region it was addressed about. From bjoern's QA friction report: four extra crops spent working out which of three points produced a panel-wide flood, and two regions flagged because it still could not tell. With one green and one owner, a spill is attributable directly.
The creation brief says containers are not its business. From the other friction report — a run deleted a region and re-added it with an offset oversized box to move a seed.
add_regiontakes no seed; containers are refinement's, viaclassify_region, which already re-seeds without touching the bbox. Since #126 the creation agent sees container masks, so it was trying to fix something it was never given a tool for. That is a consequence of my own change, and the brief now says whose job it is.DefaultTolerancemoves to the domain. An editor offering the number has to say what it currently is, and a default only the gateway knows is one an editor can only guess at.Tests — 1277/1277 green
Domain 150, UseCases 575, Integration 273, BlazorAdapter 279. Build 0 warnings / 0 errors.
New: the runaway refused and naming both levers; a tolerance reaching the gateway; an annotated crop painting one owner; the threshold changed by hand and returning to default storing
null; a runaway not previewed as a clean and not drawn as a container; floating dropping a container while bubble and embedded keep theirs.Two existing tests changed, and both were encoding the bug.
Apply_writes_every_editable_field_at_onceappliedFloatingwith a container — the contradiction the write-path rule now forbids — so it moved toEmbeddedand the floating case got its own test.Classifying_a_bubble...broke because I had dropped the pixel size from the report; that was an unrequested change and I restored it.Seed data
One seeded container now carries a hand-tuned threshold and the rest sit at the default, so both states have a subject. Asserted in
SeedDevDataTests.Browser-verified
Seeded world → Bbox → region selected: the slider renders at the seeded
6, bounds1–128. Set to90, and it persists through a full reload — editor →SaveContainerRequested→UpdateRegion→ DB.The mask does not visibly change there, and that is the fixture, not the plumbing: the synthetic scans are hard-edged flat fills where every threshold gives the same answer.
MeasureRegionRoomsdemonstrably passesContainer.Toleranceto the filler, and the table above is the same code path on an image where the threshold decides.Notes
RunawayShareis 25%, and a narration box spanning a splash page could exceed it. I judged a stored runaway worse than a refused rarity — the agent hasreport_frictionif that is wrong, and it is one constant to move.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.5%
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.6%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A threshold that moves both ways, a runaway refused at the door, a preview that stops lying — this is the kind of PR that makes a Flugel's heart sing~ ♪ You found that a container covering half a page was unfixable by design, and you fixed the design. The tolerance table in the body (1–16 = 6%, 24 = 68%, 128 = 99%) is the sort of evidence I adore. Numbers, not vibes!
Verdict: ✅ Looks good to me~
I dug through every changed file in full, traced the fill/focus/refusal paths against their siblings, ran the whole suite, and inspected every new branch. Nothing blocking. The logic is sound, the tests are directional, and the architecture is honest. One little nit below~ ♡
✅ What I liked~
Floatingnullification lives inRegion.Apply, not the tool. "A rule only one caller keeps is not a rule" — and you put it where every caller flows.Floating_text_cannot_keep_a_container+A_form_with_a_surface_keeps_its_container[Bubble, Embedded] prove both arms. TheApply_writes_every_editable_field_at_oncetest was encoding the bug (Floating + container) and you split it honestly — that's the right call, not a shortcut.ApplyAsync. Nothing is written — not the container, not the classification.A_fill_that_took_the_page_is_refused_and_names_both_leversassertsregion.ContainerANDregion.Formare null after, which pins "the bubble and its seed are one act" exactly. The refusal message names both levers and the correct one for the trapped-inside case (lower tolerance). That's thoughtful agent UX.OverlayAsyncis exactly right.OverlayForViewAsyncpassesfocus: null→(focus is null || …)is true → all containers paint (view behavior preserved). OnlyBoundCropToolpassesfocus: args.Region, and box-addressed crops pass null too → all paint (unchanged).An_annotated_crop_paints_only_the_region_it_was_asked_aboutproves one green, one owner. No regression on the view path — I checked both callers.SetToleranceclamps then storesnullfor the default.wanted == DefaultTolerance ? null : wantedmeans an untuned container stays untuned and follows the default if it ever moves.Putting_the_threshold_back_to_the_default_stores_nothingpins this. Clean.@if (!fit.LooksLikeRunaway)on the clean SVG — a container covering half the sheet would have erased the artwork and called it finished. "The worst thing a preview can do is show a wrong answer confidently." Yes. Exactly yes.DefaultTolerancemoved to the domain. The editor needs to say what the number currently is; a default only the gateway knows is one it can only guess at. The filler now referencesRegionContainer.DefaultTolerance. The migration is complete for the default.color-mix+--dangerfor the loose-mask tint matches the established Kagaku.UI pattern (InlineAlert, StatusDot, RegionSelector all usecolor-mix(in srgb, var(--danger) …%)). Consistent.💡 Little ideas (non-blocking)~
SkiaContainerFiller.cs:21,84—MostToleranceis still a private duplicate, and the lower bound is a magic1. This PR migratedDefaultTolerancefrom the filler's private constant toRegionContainer.DefaultTolerance(the right move!), butMostTolerance = 128stayed behind as a private copy that must agree withRegionContainer.MostTolerance = 128. Line 84 now mixes sources:Math.Clamp(request.Tolerance ?? RegionContainer.DefaultTolerance, 1, MostTolerance)— domain default, local max, hardcoded1whereRegionContainer.LeastToleranceexists. Both values agree today (128 and 1), so this is pure future-drift hygiene, not a bug. Whenever you're next in this file,MostTolerancewants the same treatmentDefaultTolerancegot. ♡Automated review by Jibril · 2026-07-30
CI/CD: absent for head
c2948b0(PR just opened, no coverage bot comment yet) · Local checks: build 0 warnings/0 errors, 1277/1277 pass (Domain 150, UseCases 575, BlazorAdapter 279, Integration 273 — matches PR body); new tests verified individually green💡1 This PR moved DefaultTolerance to the domain and left MostTolerance behind as a private copy, with a hardcoded 1 beside it where LeastTolerance already existed — so one clamp read from three places. They agree today, which is exactly when to fix it: the slider, the record's own validation and the fill now take every bound from the same source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Taken in
300a302.💡1 —
MostTolerancewas left behindYou were right, and it was worse than a duplicate: that one clamp read from three sources after my change —
I migrated the default because the editor needed to display it, and stopped there. The ceiling has the same claim on being shared: the slider's
max,RegionContainer.Normalized()'s refusal, and the fill's clamp all have to agree about what 128 means, and a private copy is a fourth opinion waiting to drift.The private constant is gone and all three bounds now come from
RegionContainer. Your framing that they agree today is the reason to do it now rather than an argument for leaving it — a duplicate is only cheap to remove while it is still correct.No test changes: the values are identical, so this is provably behaviour-preserving. Which is also why there is nothing new to pin — a test asserting "128 equals 128" would be asserting the bug can't come back by restating it.
Tests: 1277/1277 green, unchanged (Domain 150, UseCases 575, Integration 273, BlazorAdapter 279). Build 0 warnings / 0 errors.
Flagging that this is a production change after your green — one file, one clamp, no behaviour — so it is a new round rather than a test-only addition.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! You came back to finish the migration~ ♪ That's the spirit! The private
MostTolerancewas the last copy standing in the corner, and you walked it home to the domain where its siblings already lived. Fufu~ three numbers that must agree are now one number, read in three places. That's not a nit-pick anymore — that's hygiene.Verdict: ✅ Looks good to me~
Surgical +7/-4 in
SkiaContainerFiller.csONLY, zero production drift in behavior (1→LeastTolerance, 128→MostTolerance, the values agree today and now they cannot disagree tomorrow). The comment you added — "three numbers that must agree are three chances to disagree" — is exactly the kind of sentence I want to read in a diff. It says why, not what.I traced every consumer of the tolerance bounds to confirm the migration is genuinely complete now:
min/max(PageWorkspacePage.razor:233-234)SetToleranceclamp (PageWorkspacePage.razor:976)Normalized()guard (RegionContainer.cs:45)MostTolerance+ hardcoded1✗The last row is the one this commit fixed. Zero private copies of the bounds survive anywhere outside the domain record. Fufu~ clean.
✅ What I liked~
MostToleranceto 256 in the domain no longer has a silent shadow in the gateway clamping at 128.LeastTolerance == 1andMostTolerance == 128areconst, so the clamp bounds are compile-time identical to before. No test could distinguish the two versions — which is why I didn't ask for a new one. ♡Automated review by Jibril · 2026-07-31
CI/CD: stale for
300a302(coverage bot #5453 covers priorc2948b0) · Local checks: build 0 warnings/0 errors (.NET 10), 1277/1277 tests pass (Domain 150, UseCases 575, BlazorAdapter 279, Integration 273 — matches prior approved commit)