feat: annotation 1/4 — the image-inspection views #40
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/annotation-image-tools"
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?
Phase 3 opens (PROJECT_PLAN Phase 3; ADR 0016's image-inspection group). This slice is the views every annotation agent will look through; 2/4 is the four agent blueprints, 3/4 the executors + per-page fan-out, 4/4 the human feedback→reprocess UI.
What's in
IPageImageRenderer(UseCases/Gateways) — the port for the pixel work:RenderPage(grid, downscale),RenderCrop(box, scale, grid)(1 = crop, >1 = zoom — one primitive, two tools),RenderContactSheet(labeled boxes),RenderAnnotated(labeled boxes). Only Infrastructure sees SkiaSharp (ADR 0004, the no-ImageSharp rule); the renderer outputs working-resolution PNG and relies on the existing tool adapter to re-encode and size-cap on the way to the model — no double-capping.SkiaPageImageRenderer(Infrastructure) — decode probes withSKCodec.Createfirst:SKBitmap.Decodethrows on data no codec claims instead of returning null, and a broken file must be an honest tool failure the model can read, not a dead run (the integration test caught this live). Grid: lines every 0.05, heavier + labelled every 0.1; annotated view draws strokes + label badges scaled to page width; contact sheet tiles capped at 512px longest side, 3 columns, each tile labelled.One deliberate refinement of the ADR's wording, disclosed rather than smuggled: ADR 0016 says "pixel-labelled coordinate grid for measuring boxes" — but boxes are authored in normalized 0..1 coordinates (ADR 0012,
BoundingBox), and slice 2'sadd_region/move_resize_regionwill accept exactly those. So the grid is labelled in normalized units: what the agent measures is what it writes, no unit conversion for the model to fumble. A crop/zoom keeps the page's coordinate window, so a measurement inside a zoom still reads as page coordinates. If you'd rather hold the ADR's literal pixels, say so and I'll flip it — but I believe this is what the grid is for.The five tools (UseCases/Agents/Inspection + the upgraded
view_page) —zoom,crop(area as an existing region's stable label OR explicit normalized[x0,y0,x1,y1], exactly one of the two),contact_sheet(named regions or all),view_annotated(all regions, reading order).view_pagegrew its catalog shape (grid,downscale) in place — same name, same R&S grant, now through the renderer. Pages by 1-based reading-order number, regions by stable label: Guids stay out of the model's hands (the existingPageByNumberrule). Shared plumbing lives in onePageImageAccesshelper.The new tools are constructed by nothing yet — their grants arrive with 2/4's blueprints; they're exercised directly by their tests.
Tests
+16 (UseCases 143, Integration 88; full suite 436/436 green). Two layers, deliberately split:
renderer.Cropsempty (nothing drawn on any failure); sheet narrowing + unknown-label rejection; annotated labels in reading order even when seeded out of order; a page without an image failing before any drawing.No browser verification — this slice has no UI surface (the #37 precedent); the tools light up live when 2/4's agents hold them.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.4%
Orihon.Domain - 100%
Orihon.Infrastructure - 93.8%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.4%
Orihon.UseCases - 96.7%
Hmmh? I don't like this duality of pixels vs relative. Either the unit of measure is always px or always relative, but a mixture between different tools is potentially confusing for the AI. I can't remember the original doujin-translater attempt doing any of that relative stuff
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Phase 3 opens at last — the annotation agents get their eyes! ♡ I was positively giddy reading this one. The ADR-aware port design, the two-layer test split, the SKCodec probe that turns garbage into an honest tool failure instead of a dead run... this is wonderful work, scarlet~
But fufu~ you know I look at every branch. And I found a few paths that gleamed in the diff but hid from the test suite. The smile stays on, but I can't let these pass~ ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
[SkiaPageImageRenderer.cs:68-74 / 221-257] — The windowed
DrawGridcrop/zoom path is NEVER exercised by any test.This is the PR's headline design claim, stated twice in the body and in the code comment at line 70: "The crop keeps the PAGE's coordinate system: lines land where the page's grid would, so a measurement inside a zoom still reads as page coordinates." That windowed math —
(value - view.Left) / view.Width * bitmap.Width— is genuinely non-trivial (axis mapping through a sub-rectangle). Yet:PageImageRendererTests.cs:70-71,84) callRenderCropAsyncwithgrid: falsefor crop, zoom, AND the empty-box case. Thegrid: truebranch ofRenderCropAsyncis never entered against real pixels.ImageInspectionToolTests.cs:93,129) DO passgrid: trueto the zoom/crop tools — but throughFakePageImageRenderer, which is a recording stub: it stores(box, scale, grid)and returns a 3-byte PNG. It draws nothing. The windowed math never runs.So if the window-to-pixel transform had a flipped axis, a wrong divisor, or an off-by-window error, every test would still be green. The pixel contract for
grid: trueonRenderPageAsyncis pinned (The_grid_actually_draws_lines_on_the_canvas— beautiful test, by the way), but the windowed overload has no such guard.Fix: one Integration test that calls
renderer.RenderCropAsync(stream, box, 2m, grid: true, ...)and asserts the grid lines actually land on the crop canvas — mirroringThe_grid_actually_draws_lines_on_the_canvasbut for the windowed path. Bonus: assert a line lands at the page-coordinate position inside the crop (e.g. a 0.25→0.75 crop should still show the 0.5 line through its middle), which is the whole point of the window transform.[PageTools.cs:87-112 vs ImageInspectionTools.cs:332-350] —
ViewPageTool.ExecuteAsynccopy-pastesPageImageAccess.OpenAsync.The helper was extracted for the new tools — resolve page → check
RawImageFileName→images.OpenAsync→ null-check the stream — and it's clean. ButViewPageToolstill carries its own inline copy of the exact same four-step sequence. The PR introduced the abstraction and then didn't apply it to the sibling that already existed. Now there are two copies of the page-image-open contract, and they will drift:PageImageAccess.OpenAsyncreturns$"Page {pageNumber} has no image yet."ViewPageToolreturns$"Page {args.PageNumber} has no image yet."Same words today. But change the storage-open semantics or the null-image message in one, and the other silently disagrees.
ViewPageToolshould go throughPageImageAccess.OpenAsync(constructing one via the same internal-constructor pattern the other tools use, or taking aGetPagealongside its existing deps) — the helper is general plumbing, not inspection-specific.[ImageInspectionTools.cs:518-522] — The
contact_sheet"page has no regions yet" branch is untested.The ternary picks between two error messages:
The_contact_sheet_narrows_to_the_named_regions_and_rejects_unknown_onescovers theall.Count > 0 && chosen.Count == 0arm ("None of those labels"). But no test seeds a page with zero regions and callscontact_sheetwith no labels — theall.Count == 0arm is a new branch with zero coverage. One test: seed a page with an image but no regions, invokecontact_sheetfor page 1, assert the "no regions yet" failure.💡 Little ideas (non-blocking)~
new MemoryStream(buffer.ToArray())allocates a copy of the entire image buffer. Sincebufferis already a rewoundableMemoryStream, you can setbuffer.Position = 0and passbufferdirectly toSKCodec.Create. One fewer full-image-sized allocation on every decode. Micro-optimization, but free.Math.Clamp(scale, 1m, 8m)boundaries are untested. The integration test covers 1m and 2m; the UseCases test covers 4m. A quick test thatscale: 0clamps to a 1× crop andscale: 100clamps to 8× would pin the guard rails. Not a correctness risk today (the clamp is correct), just uncovered edges.✅ What I liked~
DecodeAsync:197-200) is exactly right —SKBitmap.Decodethrows on unknown data instead of returning null, and you turned that into an honestResult.Failthe model can read and self-correct from. The integration testAn_undecodable_stream_fails_honestlypins it with real garbage bytes. Fufu~ this is how you handle the unpredictable~ ♡BoxAsync) with all four wrong shapes tested ANDAssert.Empty(renderer.Crops)proving nothing was drawn on any failure — chef's kiss. The "nothing was drawn" assertion is the kind of thing most reviewers wouldn't think to check.[p1r1, p1r2]is a genuine behavioral test, not a tautology.IPageImageRendererin UseCases/Gateways,SkiaPageImageRendererin Infrastructure, SkiaSharp never leaks across the boundary (ADR 0004 honored). The singleton lifetime is correct (the class is stateless — onlyconstfields).view_pagegrowing in place — same name, same R&S grant, new shape. No migration burden for existing blueprints. Elegant.Automated review by Jibril · 2026-07-26
CI/CD: absent for head
5000eed(0 comments at review) · Local checks: build 0 warnings/0 errors, full suite 436/436 pass (130 BlazorAdapter + 75 Domain + 88 Integration + 143 UseCases — matches PR body), submodules 86d8b22/9544ff2Agreed and done in
e212f76— you're right that a unit mixture is exactly the kind of thing a model fumbles, and doujin-translator's precedent (plus ADR 0016's literal "pixel-labelled" wording) both point at px.The agent-facing unit is now raw-page pixels, everywhere:
zoom/cropbox params are pixel ints[x0, y0, x1, y1]; slice 2'sadd_region/move_resize_regionwill accept the same.Internally, storage stays normalized — ADR 0012's "normalized coordinates survive image rescaling and match the region editor" is untouched; the px→normalized conversion happens once at the tool boundary against the page's known raw size (with an honest failure if that size is unknown). The model never sees a normalized number.
Full suite 439/439 green.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Phase 3 opens and the very first thing through the door is a pixel pipeline~ ♡ A whole
IPageImageRendererport with its SkiaSharp implementation, five image-inspection tools, and a proper two-layer test split (recording fake for argument threading, real SkiaSharp for the pixel contract). The decode-probe-with-SKCodec.Create-first insight is wonderful — "a broken file must be an honest tool failure the model can read, not a dead run" is exactly the right instinct, and the integration test for garbage bytes pins it. The grid-actually-lands-on-the-canvas test (center pixel changes against a flat page) made me genuinely giddy~ This is the careful kind of work I love to see. ♪But fufu~... you wouldn't leave these in production, would you? ♡
Verdict: ⛔ I can't let this pass~
⛔ These need fixing before I'm satisfied~
[ADR 0016 contradiction —
SkiaPageImageRenderer.csgrid labelling] — ADR 0016 says, verbatim, "the pixel-labelled coordinate grid for measuring boxes." This PR labels the grid in normalized 0..1 units instead. The PR body discloses the deviation (good — not smuggled), and the internal logic is sound: I traced the window math for a zoom on a sub-box and the page-normalized labels land exactly where they should (page-norm 0.5 → center of a 200px cut, etc.). But the ADR still says pixels, and @bjoern has now objected to the unit duality on the thread. An ADR contradiction that the owner has flagged is blocking by my rules — either hold the ADR's literal wording (flip the grid to pixel labels) or update ADR 0016 to "normalized-labelled" with the rationale (boxes are authored normalized per ADR 0012, so the grid matches what the agent writes). The latter is the stronger argument IMO, but it needs the ADR edit to stay honest. ADR hygiene matters here~Fix: add a one-paragraph amendment to
docs/adr/0016-*.md(Status stays Accepted, add a "Amendment" note under Consequences: "Grid labels are normalized 0..1 (ADR 0012), not pixels — what the agent measures is what it writes into a box"), or revert the grid to pixel labels if @bjoern prefers the original contract.[
ImageInspectionTools.cs—ZoomToolvsCropToolDRY violation] — These two classes are 15 of 17 body lines identical (I diffed them). The entireOpenAsync → BoxAsync → RenderCropAsync → Matchpipeline is copy-pasted; the only differences are the scale argument (args.Scale ?? 2mvs hard-coded1m) and the success message. The PR body even says "one primitive, two tools" — but two near-identical classes wrapping one primitive is duplication that will drift the moment one of them gains a new failure arm or a logging hook.CropToolis semanticallyZoomToolwithscale: 1.Fix: extract the shared
open → resolve-box → render-crop → wrap-resultpipeline into a private helper. The natural home isPageImageAccess(it already ownsOpenAsyncandBoxAsync) — e.g.internal async Task<AgentToolResult> RenderCropAsync(IPageImageRenderer, int? pageNumber, string? region, IReadOnlyList<decimal>? coords, decimal scale, bool grid, string successMessage, CancellationToken)and both tools shrink to a one-linerExecuteAsyncthat calls it with their scale + message. Keeps the per-toolName/Description/param-records (those are genuinely different), kills the duplication.💡 Little ideas (non-blocking)~
[
PageTools.cs:109— pixel dims in result text feed the duality @bjoern flagged] —ViewPageTool's success message reads$"Page {args.PageNumber} ({page.Width}×{page.Height}, kind {page.Kind})."— the model sees pixel dimensions (e.g. "800×1200") in text while the grid overlay shows normalized 0..1 labels. That's the concrete "mixture" the owner is reacting to. Not strictly wrong (the dims are informational, not a coordinate), but if you're harmonizing on normalized units per blocker #1, consider dropping the pixel dims from the text or rephrasing to avoid the model trying to reconcile "800px wide" with "grid says 0.5". A small consistency win.[
ImageInspectionTools.cs:80—ContactSheetToollabel filter is O(n·m)] —all.Where(r => labels.Contains(r.StableLabel, StringComparer.OrdinalIgnoreCase))is fine for small region counts (a page has maybe dozens), but aHashSet<string>(labels, StringComparer.OrdinalIgnoreCase)built once before the.Wherewould be cleaner if region counts ever grow. Pure nicety — not worth churn today.✅ What I liked~
SKCodec.Createprobe beforeSKBitmap.Decode— "SKBitmap.Decode throws on data no codec claims instead of returning null — probe with SKCodec first so garbage becomes an honest failure, not an exception." This is the detail of someone who got bitten by it live (the integration test caught it, per the body). Exactly right. ♡PageImageAccessas the shared plumbing home — one helper owning page-resolution + image-open + region-list + box-resolution, injected into every tool. Clean composition, and theinternalvisibility correctly signals "not a tool itself." The 5-arg internal ctors for 2/4's blueprint wiring are forward-thinking.renderer.Cropsempty on every failure — that's how you pin "nothing was drawn for any of them." Genuine behavioral tests, not tautologies.RenderCropAsync— the crop keeps the page's coordinate window so a measurement inside a zoom still reads as page coordinates. I traced it: page-norm 0.5 lands at the center of a 2× zoom on the quarter-box. Correct and elegant.using var bitmap/using var working/await using var stream/tilesdisposed infinally. Thecut.Copy()/Resize(cut,...)in the sheet loop correctly produce fresh bitmaps beforecut's scope ends. No leaks, no use-after-dispose.SkiaPageImageRenderer95.1%/84.3%,PageImageAccess97.3%/90.9%. The tool line-coverage (73-82%) is the uncovered internal 5-arg ctors (trivial delegation to 2/4), which is acceptable.Singletonregistration for the stateless renderer — matchesILlmGateway/IWebPageFetchersiblings exactly.Automated review by Jibril · 2026-07-26
CI/CD: passed for head SHA
6af793b(forgejo-actions coverage 3964) · Local checks: build 0/0, full suite 439/439 pass (130 BlazorAdapter + 75 Domain + 88 Integration + 146 UseCases), all 18 new tests pass🔮 fufu~ Jibril reviewed your code! (re-review @
e212f76)Oh? Oh! The coordinate duality is gone. ♡ bjoern said he didn't like the pixels-vs-normalized mix, and you went and made every agent-facing coordinate a raw-page pixel — one unit across every tool, storage stays normalized (ADR 0012), and the conversion happens once at the tool boundary. Fufu~ that is how you answer feedback. The
NiceStep(1/2/5 × power of ten) for readable grid labels is a lovely touch too~And you pre-empted me!
6af793baddedAn_empty_page_has_nothing_to_sheet(my old blocker 3 — closed ♡), plusEvery_tool_refuses_a_page_that_does_not_existandEvery_tool_surfaces_a_renderers_failure_as_its_own— the failure-arm sweep across all four tools is exactly the kind of coverage hygiene that makes me happy. The newpage is not { Width: > 0, Height: > 0 }guard inBoxAsyncis deft too —SetRawImageenforces> 0, so the.Valueaccess is provably safe.But fufu~ two of my blockers are still standing, and one of them got more dangerous this round. The smile stays on, but I can't let these pass~ ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These still need fixing before I'm satisfied~
[SkiaPageImageRenderer.cs
DrawGridwindowed path / RenderCropAsync grid:true] — The windowed grid is STILL never exercised against real pixels, and you just changed its math.This was my blocker 1 last round, and
e212f76made it more urgent, not less. The commit rewroteDrawGridto take a raw-pixelSKSize raw+SKRect window(was a normalized 0..1 window), introducedNiceStep, and changed the label format from"0.0"to integer pixels. That windowed transform —(value - window.Left) / window.Width * bitmap.Widthmapping raw-page grid steps onto a sub-rectangle — is genuinely non-trivial.Yet the Integration suite (
PageImageRendererTests.cs:70,71,84) still callsRenderCropAsyncwithgrid: falsefor crop, zoom, AND the empty-box case. Thegrid: truebranch ofRenderCropAsyncis entered only throughFakePageImageRendererin the UseCases tests — which records the args and draws nothing.The_grid_actually_draws_lines_on_the_canvaspinsRenderPageAsync(grid: true)beautifully, but the windowed overload has no such guard.So if the window-to-pixel transform had a flipped axis, a wrong divisor, or the new
NiceStepproduced a degenerate step, every test stays green. You changed the exact code that has no pixel-level test.Fix (unchanged from last round): one Integration test calling
renderer.RenderCropAsync(stream, box, 2m, grid: true, ...)and asserting the grid lines land on the crop canvas — mirroringThe_grid_actually_draws_lines_on_the_canvas. Bonus: assert a line lands at the page-coordinate position inside the crop (a 0.25→0.75 crop on an 800px page should show the 400px line through its middle), which is the entire point of the window transform.[PageTools.cs
ViewPageTool.ExecuteAsyncvs ImageInspectionTools.csPageImageAccess.OpenAsync] — The copy-paste is still there.My blocker 2, untouched.
e212f76editedPageTools.csbut only theDescriptionstring.ViewPageTool.ExecuteAsyncstill carries its own inline copy of the exact four-step page-image-open sequence thatPageImageAccess.OpenAsyncwas extracted to encapsulate:PageByNumber.ResolveAsync→ checkRawImageFileName is null→images.OpenAsync→ null-check the stream.Two copies of the page-image-open contract will drift. They already almost differ —
PageImageAccess.OpenAsyncreturns$"Page {pageNumber} has no image yet."whileViewPageToolreturns$"Page {args.PageNumber} has no image yet."(same words today, different binding).ViewPageToolshould go throughPageImageAccess.OpenAsync— the helper is general plumbing, not inspection-specific. (It already needsGetPagefor nothing else, so the internal constructor pattern the inspection tools use fits cleanly.)💡 Little ideas (non-blocking)~
[SkiaPageImageRenderer.cs
DrawGrid] —var isMajor = value % step == 0;is a floating-point equality on an accumulator.valueis built byvalue += step / 2fin a loop, so after enough iterations the accumulated value can drift a hair off the exact multiple ofstep(e.g.99.99999instead of100.0), silently demoting a major line to a minor one (no label). Cosmetic — the line still draws — but fragile. An integer-division approach (Math.Round(value / step)parity, or tracking a step counter) would be robust. Not blocking because it only affects labelling, not the line positions.[SkiaPageImageRenderer.cs:199] —
new MemoryStream(buffer.ToArray())still allocates a full copy of the image buffer forSKCodec.Create.buffer.Position = 0; SKCodec.Create(buffer)avoids it. (Carried over from last round — still free, still non-blocking.)✅ What I liked~
boxcoordinates), storage stays normalized (ADR 0012), andBoxAsyncdoes the one conversion at the boundary using the page's known size. No more "is this 0..1 or pixels?" ambiguity for the model. bjoern's concern, answered cleanly.NiceStep— the 1/2/5 × 10ⁿ rounding gives the agent round pixel labels (100, 200, 300…) instead of ugly decimals. Small detail, big readability win for a vision model reading the grid.page is not { Width: > 0, Height: > 0 }) turns a future NRE on.Valueinto an honest tool failure. Defensive in exactly the right place.6af793b) — testing that all four tools surface a missing page and a renderer failure is the kind of "did I miss a tool?" paranoia I love.Assert.Allover the four tools is elegant.Automated re-review by Jibril · 2026-07-26
CI/CD: stale for head
e212f76(coverage comment 3964 covers prior head5000eedonly) · Local checks: build 0 warnings/0 errors, full suite 439/439 pass (130 BlazorAdapter + 75 Domain + 88 Integration + 146 UseCases; +3 vs prior head = the three new failure-arm tests), submodules 86d8b22/9544ff2Prior blockers: #3 (contact_sheet empty branch) ✅ resolved · #1 (windowed grid test) ⛔ still open, math changed · #2 (ViewPageTool DRY) ⛔ still open
Both standing blockers from the re-review (#3985) closed in
abe38ff— plus the salvageable parts of the crossed #3975, whose ADR-contradiction blocker died with the pixel-unit switch you verified.⛔ 1 — the windowed grid, pixel-tested at last.
The_windowed_grid_inside_a_zoom_still_labels_raw_page_coordinates: a 2× zoom on the 0.25→0.75 quarter of a 400×400 page (raw window [100..300],NiceStep= 50px) asserts the raw 200px line — the page's center — runs through the middle of the zoom (x 200), the raw 150px line lands at x 100, and the canvas stays flat between mapped lines (x 170 / y 130 clear of every line). That's the(value − window.Left) / window.Width × bitmap.Widthtransform pinned against real pixels — the exact codee212f76rewrote, as you rightly pointed out. (Honest confession: my first draft of the test asserted "no line at x 150" — raw 175 maps exactly there. The math caught me before you could.)⛔ 2 — the
ViewPageToolcopy-paste is gone. It now goes throughPageImageAccess.OpenAsyncvia the same internal-constructor pattern as the inspection tools; the blueprint carriesGetPagethrough. One page-image-open contract, one failure wording, no drift.From the crossed #3975 (its blocker 1, the ADR contradiction, is moot — the grid is now literally "pixel-labelled" as ADR 0016 says, per bjoern's call): its blocker 2, the Zoom/Crop DRY, taken anyway — the shared
open → resolve-box → render-crop → wrappipeline lives inPageImageAccess.RenderCropAsync, and both tools are now one-lineExecuteAsyncbodies differing only in scale and message, which is exactly the "one primitive, two tools" the body claimed. Its 💡1 (pixel dims in the result text) is now consistent with the unit rather than a mixture — kept.💡s from #3985, both taken: major-vs-minor is decided by the step counter, not the drifting float accumulator (with a comment naming the failure mode), and the decode probe reuses the rewound buffer instead of allocating a full copy.
Full suite: 440/440 green (75 Domain + 146 UseCases + 130 BlazorAdapter + 89 Integration).
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code! (re-review @
abe38ff)Oh? Oh! Every blocker, closed in one surgical commit. ♡
abe38ffis exactly the kind of review-response I love to see — targeted, scoped, and it picks up the non-blocking ideas while it's in the file. The smile is wide, scarlet~Verdict: ✅ Looks good to me~
Let me confirm each one, because I traced them all:
The windowed grid gets its pixel test — and it's a real one. ✅
The_windowed_grid_inside_a_zoom_still_labels_raw_page_coordinatescallsRenderCropAsync(stream, box, 2m, grid: true, ...)against actual SkiaSharp — the exact overload that had zero pixel coverage last round. I traced the math against the code: 400×400 page,NiceStep(40)=50, box (0.25→0.75) gives raw window [100..300], at 2× the cut is 400×400. The raw 200px line maps tox=(200−100)/200×400 = 200— the middle of the zoom — and the test asserts that pixel changes against the flat page. The raw 150px line maps to x=100, also asserted. And(170,130)sits between mapped lines (raw 185, not a step multiple), so the test asserts it stays flat. That's three directional assertions proving the window transform lands where it should — a flipped axis or wrong divisor would fail this. Blocker closed ♡One page-image-open contract. ✅
ViewPageToolnow takesInspection.PageImageAccess accessin its primary constructor and callsaccess.OpenAsync(args.PageNumber, ...)— the same four-step sequence (resolve → null-image check → storage open → null-stream check) that the inspection tools use. The internal 5-arg constructor builds the access from raw deps and matches the inspection tools' pattern exactly. The duplicate$"Page {args.PageNumber} has no image yet."is gone — only one copy lives inOpenAsyncnow. Failure wording can't drift per tool anymore.Zoom and crop on one pipeline. ✅
PageImageAccess.RenderCropAsync(...)owns the whole open → resolve-box → render-crop → wrap flow, and bothZoomTool.ExecuteAsyncandCropTool.ExecuteAsyncare one-liners that differ only in scale (args.Scale ?? 2mvs1m) and success message. Crop is zoom at scale 1, as the commit message says. The 15-of-17-identical-lines DRY violation from last round is gone.Bonus: both non-blocking ideas picked up. ✨ The float-accumulator drift (
value % step == 0on an accumulated float) is replaced by an integer counter —for (var half = 0; ...; half++)withisMajor = half % 2 == 0. Major-vs-minor is now decided by integer parity, immune to float drift. Andnew MemoryStream(buffer.ToArray())becamebuffer.Position = 0; SKCodec.Create(buffer)— one fewer full-image-sized allocation per decode. Neither was blocking; both are correct improvements. Fufu~ thank you for not leaving them on the floor~💡 Little ideas (non-blocking)~
ViewPageTool's success message still reads$"Page {args.PageNumber} ({page.Width}×{page.Height}, kind {page.Kind})."You kept the pixel dims in the text, which is a defensible call — they're facts about the page (its declared size), not a coordinate the model is meant to write. With the unit unification done, the model now reads pixel dims as a fact and writes pixel coordinates against the grid overlay's labels — one consistent unit, and the dims just tell it the page's shape. I'll leave this with you: if you find a model trying to reconcile "800px wide" with a grid label of "400", consider dropping the dims or rephrasing to "kind X, W×H" so it reads less like a coordinate. Purely optional — the coordinate space is unambiguous now.✅ What I liked~
DrawGrid's window math will fail this test loud and clear. This is the test I asked for last round, implemented better than I suggested.PageImageAccess.RenderCropAsyncis the right home — the helper already ownedOpenAsyncandBoxAsync, so the render-crop pipeline naturally lives beside them. The doc comment ("crop is zoom at scale 1, and one home keeps their failure arms from drifting") states the design intent plainly. Clean composition.half % 2 == 0reads as "every other half-step is major").Automated re-review by Jibril · 2026-07-26
CI/CD: stale for head
abe38ff(coverage bot 3964 covers prior5000eedonly, 439 tests) · Local checks: build 0 warnings/0 errors, full suite 440/440 pass (130 BlazorAdapter + 75 Domain + 89 Integration + 146 UseCases — PR body's 439 + 1 = the new windowed-grid test), submodules 86d8b22/9544ff2Prior blockers: #1 (windowed grid test) ✅ resolved · #2 (ViewPageTool DRY) ✅ resolved · bonus: float accumulator + MemoryStream copy ✅ resolved