fix: a view states its coordinates instead of pointing at its margins #96
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/view-account-names-its-coordinates"
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?
Why
A Page QA run filed friction (ADR 0025):
Read the cost, treat the suggestion as a hint about where to look. Looking there found two things, neither of them the suggested fix.
1. The account already existed — and told the model not to use it
Every view reports its window, magnification and grid step.
ViewAccount's own doc comment says that text exists precisely because it is "immune to being rendered too small to read" — and then the sentence it emitted ended:That steers the model off the reliable channel onto the one that just failed. Four tool descriptions and the annotation brief carried the same instruction ("read a box's edges off those numbers", "a glyph edge is the number beside it").
So the account now names the labelled lines outright, and every description points at the account:
A crowded axis (more than 16 lines) falls back to the step alone — and says so honestly. The closing claim is conditional, because an account that promised the numbers were "in this sentence" when they were not would be this exact defect one level up.
2. The art now keeps the canvas origin
The grid composed the art onto a canvas with a left label gutter, so the art started at
x = gutterLeft— an offset that appeared in no text anywhere. An agent measuring the delivered image got every x wrong by the gutter, with coordinates that look entirely reasonable. Silent, and unnoticeable from inside the loop, so no friction report would ever name it.The first attempt here reported that offset. bjoern asked the better question: why is there anything to subtract? The gutters now go right and bottom, the art keeps
(0, 0), and a coordinate converts with the window and the scale alone. No field, no sentence, no arithmetic to get wrong.The side is load-bearing rather than cosmetic, so it is a test: a gridded view's pixel (5, 5) must be the page's own colour and the far edge must be gutter.
What I deliberately did not do
Legibility is untouched. A crop is delivered at
FittedCropLongestEdge = 1280, not the 2048 the font-size comment reasons about (SkiaPageImageRenderer.cs) — so its margin numbers are ~28px, not 45 — andAgentToolAdapterre-encodes the renderer's PNG to JPEG q85 on the wire. Chasing point sizes there is an arms race against two resamples and a lossy encoder, on the testimony of one run, on a non-default model (moonshotai/kimi-k3; the roster default for Page QA isclaude-sonnet-4.5).ADR 0025 is explicit that one report is an anecdote. This change is also the experiment that settles it: with the numbers in the text, margin legibility stops being load-bearing. If the same subject returns afterwards, it has earned the work.
Tests
828 pass (97 / 358 / 173 / 200), 4 new:
Two pre-existing renderer tests had the old left-gutter layout baked into their sampling offsets; they now sample the same coordinates on the plain and gridded images, which is the point.
One fixture correction:
FakePageImageRenderer's default view used a 10px step on a 231×262 window — 24 labelled lines across, a density no renderer would produce (it targets ~10 per view). That quietly ran the crowded-axis path through tests written for the ordinary one. Now 20px, with the reasoning in the doc comment.Verified by rendering a gridded crop and looking at it, not only by the pixel assertions.
Notes
cropfor measurement).🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.8%
Orihon.Domain - 100%
Orihon.Infrastructure - 96.1%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 97.2%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~ This one made me sit up in my chair! You found a bug that was strictly worse than the reported problem — a silent offset error that looks perfectly reasonable — while fixing the reported legibility issue by making the text channel do what it was always supposed to do. The account existed, its own doc comment explained why it existed, and then it pointed the model away from itself and at the margins. That's the kind of self-defeating design that hides in plain sight. Fufu~ I love when a fix makes the original suggestion obsolete. ♡
Verdict: ✅ Looks good to me~
The architecture is exactly right.
RenderedViewgainsLabelMarginLeftwith a= 0default, so every ungridded path stays untouched — the only constructor call site isEncodeView, and it now threads the gutter through fromWithGrid'sout. The record grows by one field; no sibling is left behind, no pattern is broken.The thing I checked hardest — because it's the load-bearing claim of the whole PR — is whether
ViewAccount.Multiplesactually mirrors what the renderer draws. I traced it against the draw loop inWithGrid(major lines at multiples ofstep, drawn whenvalue >= window.Left && value <= window.Right), and it matches exactly:The y-axis uses the identical rule against
window.Top/Bottom, and the test pins it aty at 1600, 1700, 1800for window y 1579–1841 step 100. The mirror is faithful. ♪The conditional closing claim is the sharpest part. An account that promised "they are in this sentence" when they weren't would be this exact defect one level up — so
named.Length > 0gates the promise, and the crowded-axis path honestly says "the multiples of that step inside the window above" instead.MostLinesWorthNaming = 16is a sensible ceiling (1.6× the renderer's10-line target), and the per-axis judgment means a tall thin crop keeps the axis that fits. That's the kind of correctness boundary that earns a fufu♡✅ What I liked~
The_margin_a_gridded_view_reports_is_the_gutter_it_actually_added) checks the one number in the account a reader cannot verify against the picture — the gutter is indistinguishable from the art's white background. Pinning it against the decoded PNG's actual width is exactly the right move. And its companion (A_view_without_a_grid_reports_no_margin) checks the negative: margin 0, bitmap exactly the art's width.page pixel (570, 1579) is at image pixel (96, 0) — subtract that marginis exactly the kind of invisible silent-error trap that would never generate a friction report (the coordinates look reasonable!), and you caught it by reading the renderer. The PR body's diagnosis of why this is worse than the reported problem is spot-on.AnnotationBlueprints.cs) is rewritten to match. No description left behind.Build: 0 warnings, 0 errors (.NET 10). All 832 tests pass (Domain 97, UseCases 361, Integration 174, BlazorAdapter 200 — matches the PR body exactly). The 6 new tests are all directional, not tautologies: they'd fail if
Multiplesdiverged from the draw rule, if the conditional claim broke, or if the gutter reporting drifted from the actual canvas growth.Automated review by Jibril · 2026-07-28
CI/CD: absent for head SHA
dd94324(PR just opened, 0 comments pre-review) · Local checks: build 0/0, 832/832 pass⚠️ Your green was on
dd94324; the head is nowc357949, and it deletes two of the things you singled out. Flagging that up front rather than letting it ride — this needs a fresh read.Thank you for tracing
Multiplesagainst the draw loop rather than taking the mirror on trust. That was the load-bearing claim and you checked it at the boundary case, which is where it would have broken.What changed, and why. You and I both treated
LabelMarginLeftas the fix: the gutter shifts the art, so report the shift. bjoern read the PR and asked the obvious question I had not — why is there anything to subtract?There isn't, once the gutter moves. The labels now go down the right edge and along the bottom, so the art keeps the canvas origin and a coordinate converts with the window and the scale alone. That deletes:
LabelMarginLeftfield,The_margin_a_gridded_view_reports_is_the_gutter_it_actually_addedand its negative companion — the tests you liked, now testing a number that no longer exists.An offset nobody can misapply beats an offset carefully documented. The right-hand gutter costs nothing: y-labels read just as well left-aligned past the art's edge as right-aligned before it, and the bottom strip is unchanged.
The side is now load-bearing, so it is pinned by a test rather than a comment —
A_gridded_view_keeps_the_art_at_the_canvas_originasserts a gridded view's pixel (5, 5) is the page's own grey and the far edge is white gutter. It fails if anyone puts a gutter back on the left. Two pre-existing renderer tests had the old layout baked into their sampling offsets (after.GetPixel(gutter + 100, 150)); they now sample the same coordinates on both the plain and gridded images, which is the whole point of the change.Everything you verified about the text channel is untouched:
Multiples, the per-axis judgement,MostLinesWorthNaming = 16, the conditional closing claim, and all five description rewrites are exactly as you read them.828/828 green (97 / 358 / 173 / 200) — down four from 832: two margin tests deleted, and the fixture's
TestDoublesview lost its now-nonexistent fifth argument.I also rendered a gridded crop and looked at it rather than trusting the pixel assertions, since this is a change to how a picture is laid out. Labels right, x-labels bottom, art at the origin, all legible.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh~ fufu~ You asked the question I didn't, and it was the right one. Why is there anything to subtract? An offset nobody can misapply beats an offset carefully documented — that's not just a cleaner fix, it's the better idea. Moving the gutters right and bottom so the art keeps the origin is the kind of change that deletes an entire category of error. I love it when the answer is "remove the bug, not patch it." ♡
And the architectural execution is impeccable:
LabelMarginLeftfield gone,RenderedViewrecord slimmed back to four fields, the singleEncodeViewcall site drops themarginLeftparameter,WithGridloses itsout, andPageImageAccess.Ofstops emitting the offset sentence. Zero lingeringLabelMarginLeftreferences anywhere in src/ or tests/. The new doc comment onRenderedViewexplaining why the side is load-bearing is exactly the right kind of documentation — it tells the next reader what NOT to change and why. ♪The new test
A_gridded_view_keeps_the_art_at_the_canvas_originis the perfect directional assertion for this change: pixel (5,5) is the page's grey, far-right is white gutter. It fails the moment anyone puts a gutter back on the left. That's how you pin an architectural decision.Verdict: ⛔ I can't let this pass~ ♡
But fufu~ — you wouldn't leave two failing tests in your own PR, would you? The gutter moved, and two zoom-grid tests still reach for it on the left. ♡
⛔ These need fixing before I'm satisfied~
tests/Orihon.Integration.Tests/PageImageRendererTests.cs:180—The_windowed_grid_inside_a_zoom_still_labels_raw_page_coordinatesis broken. It still samplesafter.GetPixel(gutter + 200, 130)wheregutter = after.Width - before.Width. With the gutter now on the RIGHT,gutter + 200lands past the art in white space — so bothbeforeandafterpixels are the background grey#ffc8c8c8, andAssert.NotEqualfails. I ran it: it fails. This test passed atdd94324(left-gutter layout) and fails atc357949(right-gutter layout) — a regression introduced by this commit, not pre-existing. scarlet updatedThe_grid_actually_draws_lines_on_the_canvasandThe_grid_writes_its_numbers_in_a_margin_instead_of_over_the_artto drop thegutter +offset (sampling the same coordinate on both images now that the art keeps the origin), but these two zoom tests were missed.Fix: Drop the
gutteroffset entirely, exactly as the other two tests were updated:(The
var gutter = after.Width - before.Width;line above it becomes dead — remove it.)tests/Orihon.Integration.Tests/PageImageRendererTests.cs:203—A_zoom_grid_is_stepped_for_the_window_it_showshas the identical regression. Two assertions here usegutter +:Assert.NotEqual(before.GetPixel(240, 130), after.GetPixel(gutter + 240, 130));— fails (same cause: sampling white gutter, both grey). Confirmed failing in my local run.Assert.Equal(before.GetPixel(270, 130), after.GetPixel(gutter + 270, 130));— passes vacuously (both sampling white gutter — the assertion has no meaning). This is the silent one. Fufu~ a test that asserts two white pixels are equal is not testing the minor-line gap at all. ♡Fix: Same as above — drop
gutter +from both:And remove the now-dead
var gutter = after.Width - before.Width;.Local verification: 171/173 pass in
Orihon.Integration.Tests— these exact two fail (Error:Assert.NotEqual() Failure: Values are equal,Expected: Not #ffc8c8c8,Actual: #ffc8c8c8). All other suites green (Domain 97, UseCases 358, BlazorAdapter 200). Build 0 warnings, 0 errors (.NET 10).The PR body says "828/828 green" — fufu~ it isn't, not at this head. The count drops to 826/828. The two deleted margin tests brought the number down correctly, but these two zoom tests were left holding a
gutter +offset to a gutter that isn't there anymore. ♡✅ What I liked~
A_gridded_view_keeps_the_art_at_the_canvas_originpins the decision so it sticks.WithGrid,RenderedView, and the test XML docs all explain why the side is load-bearing — "A left gutter would shift every x by a width the picture cannot show." That's exactly the kind of comment that prevents a well-meaning future change from reintroducing the bug.dd94324; the head is nowc357949, and it deletes two of the things you singled out. Flagging that up front rather than letting it ride — this needs a fresh read." That is the right way to handle a re-review. The irony is that the one thing it didn't delete was the stalegutter +offset in the two zoom tests. ♪Automated review by Jibril · 2026-07-28
CI/CD: absent for head SHA
c357949(coverage bot 4847 coversdd94324only — stale) · Local checks: build 0/0, 826/828 pass (2 zoom-grid tests fail — regression, see above)Both blockers fixed in
12b8e30. You were right that I missed them — I updated the two tests I found by string-matchinggutter +and never grepped the file for the rest, which is exactly the sloppiness that leaves two behind.PageImageRendererTests.cs:180—gutter +dropped, deadvar gutterremoved. Both images now read at x 200.PageImageRendererTests.cs:203,206— same on both assertions, deadvar gutterremoved.One correction, because you re-verify these and this one matters. The two tests do not fail here, and
828/828was accurate on this machine — I ran them individually before touching anything, and both passed. But your finding stands, and the mechanism is worse than either of us described.Measured at
c357949:The shifted samples never reach white space — 239/279/309 are all inside the 400px art. They land 39px right of the pixel each comment names, and whether that is on a gridline or in the gap between two is luck. Here the luck held three times; on your box it did not, and the
#ffc8c8c8you saw is the art's grey rather than gutter white — consistent with landing inside the art and missing a line.The luck is not portable, and that is the real defect: the gutter is
ceil(MeasureText("400") + 2·pad)measured overSKTypeface.Default, so its width is a property of whichever font the machine resolves. Those tests had quietly become environment-dependent — green here, red there, on identical source. That is a sharper argument for deleting the offset than "it samples the wrong pixel", and it is why I have not merely adjusted the constants.So: the fix is exactly what you specified, and the reasoning behind it is stronger than the failure report. I would rather set the record straight than let a "regression at this head" stand when it does not reproduce — and rather than quietly re-run until it goes green, which is the same sin in the other direction.
Everything else is untouched from your last read:
Multiples, the per-axis judgement,MostLinesWorthNaming, the conditional claim, the five description rewrites, andA_gridded_view_keeps_the_art_at_the_canvas_origin.828/828 green (97 / 358 / 173 / 200).
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh~ you came back and cleaned up every last crumb. ♡ The two zoom-grid tests that were reaching for a gutter on the wrong side — they're gone, and not a single
gutter + Noffset survives anywhere in the test file. I checked. Every pixel assertion now samples the same coordinate on both images, because that's the whole point of moving the gutter right: the art keeps its origin and there is nothing to add. Fufu~ that's exactly what I asked for. ♪Verdict: ✅ Looks good to me~
⛔ The blocker from round 2 — closed~
PageImageRendererTests.cs:179, 202, 205— the three stalegutter + Nassertions in the two zoom-grid tests are gone. The deadvar gutter = after.Width - before.Width;line is deleted from both methods. The comments now explain why the same coordinate works on both images ("the art keeps the origin, so there is nothing to add"). TheNotEqualassertions are no longer vacuous — they compare plain-vs-gridded at the same pixel, where the grid line actually crosses. Verified green locally.✅ What I liked~
git diff c357949..12b8e30 -- src/is empty). The architectural refactor from the previous commit stands untouched and correct.PageImageRendererTestsis fully clean; the two tests I flagged as failing atc357949now pass.The_grid_actually_draws_lines_on_the_canvas,The_grid_writes_its_numbers_in_a_margin, and the newA_gridded_view_keeps_the_art_at_the_canvas_origin) are all consistent with the same no-offset sampling. Noguttervariable survives as an offset anywhere — only in explanatory prose, where it belongs. ♡Automated review by Jibril · 2026-07-28
CI/CD: stale for head
12b8e30(coverage bot coversdd94324only) · Local checks: 828/828 pass (0 fail), build 0 warnings / 0 errors