feat(region): a sub-minimum press reports its point as SurfaceClicked #5
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/region-surface-clicked"
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?
Motivated by an Orihon UX report (the bbox editor): consumers of
RegionSelectorrender pointer-transparent overlays for their other regions (the selected one is the selector's own box), so a plain click on one of those overlay boxes falls through to the drawing surface, is treated as a draw that never grew pastRegionRect.MinSize, and is silently restored — the user clicks a visible rectangle and nothing happens, with nothing for the consumer to hook.What's in
SurfaceClicked(RegionSelector) — a new optionalEventCallback<RegionPoint>: when a draw gesture ends below the minimum size, the JS controller now reports the pressed anchor point (CommitPointat the boundary, rounded to four decimals and clamped into the unit square exactly likeCommitRegion). Moves and resizes stay silent — their pointer was on the committed box, not on something underneath. Unbound, a click remains a no-op byte-for-byte as before; the sub-minimum restore path in JS is untouched.RegionPoint(RegionRect.cs) — the point twin ofRegionRect: fractions of the image,Clamped()folding into the unit square, domain-free per the existing rule.CommitPointwith the gesture's anchor. Still exactly one report to Blazor per gesture, preserving the file's founding constraint.Tests
232/232 (+2): a click reports its point rounded (
0.33333 → 0.3333m) and clamped (1.00004 → 1m) without firingRegionChanged; a disabled selector swallows the click. The JS anchor-report itself follows the file's existing convention (gesture mechanics live in the browser suite, the C# boundary contract in bUnit).The consuming Orihon PR (click-a-bbox-selects-it in the page workspace) lands after this merges and pins the new commit.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagaku.UI - 94.9%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! A click that used to vanish into the void, now reported as a point on the surface — that is a delightful little piece of UX surgery, scarlet-chan! ♡ The founding constraint of this file ("exactly one report to Blazor per gesture") held firm through the addition, the
Disabledguard mirrorsCommitRegionline-for-line, and the JSg.anchortruthiness check is a cute belt-and-suspenders against a future refactor even though_downstructurally guarantees anchor for draw mode. I'm genuinely pleased with the shape of this~Verdict: ⛔ I can't let this pass~ ♡
But fufu~... you wouldn't leave a test that lies about what it tests in production, would you? The smile is still on my face, but my hand is on the knife. ♡
⛔ These need fixing before I'm satisfied~
[tests/Kagaku.UI.Tests/RegionSelectorTests.cs:47 / src/Kagaku.UI/Components/RegionRect.cs:42] —
A_click_reports_its_point_rounded_and_clampeddoes NOT exercise the clamp, andRegionPoint.Clamped()sits at 50% branch coverage (2/4) because of it.The test feeds
Click(cut, 0.33333, 1.00004)and assertsnew RegionPoint(0.3333m, 1m), with an inline comment claiming "A hair outside the unit square (a rounding overshoot at the edge) folds back in." But it doesn't fold back in — the rounding happens before the clamp, andRound(1.00004, 4) == 1.0000m, which is already inside[0, 1]. SoClamp(1.0, 0, 1) == 1.0 == Y, thex == X && y == Ycondition is true, andClamped()returnsthis— thenew RegionPoint(x, y)clone arm on line 42 is never hit. The test passes for the right output, but for the wrong reason: it's really only proving the rounding.The sibling
A_commit_that_spills_past_the_edge_is_folded_into_the_unit_squaretest (the exact analogue forRegionRect) feeds(0.8, 0.8, 0.5, 0.5)— width and height genuinely exceed what remains, so the clone arm fires there. YourRegionPointtest needs the same treatment.Fix: pick a value that survives the 4-decimal round still outside the unit square, so the clamp has work to do.
1.00006→Round → 1.0001m→Clamp → 1m(nowy != Y, clone arm fires). Concretely:This is exactly the "green CI ≠ correct" trap — the suite reports 232/232 green and 90%/100% on
RegionRect, but the branch coverage on the new type's one non-trivial method is half-dead. I checked the cobertura XML myself:Kagaku.UI.RegionPoint/Clampedbranch-rate 0.5, condition-coverage "50% (2/4)". ♡💡 Little ideas (non-blocking)~
RegionPointis the only second public type living in a non-registrysrc/file. Every sibling DTO in this folder follows one-public-type-per-file:SelectOption.cs,ToggleOption.cs,LabeledEntry.cs,QuicklinkSection.csare all solo. (ComponentTypes.csis the lone multi-type file, but it's a deliberate enum registry.) Bjoern cares about this convention — consider movingRegionPointinto its ownRegionPoint.cs. It's a true "point twin ofRegionRect" as the doc says; it deserves its own home. ♪✅ What I liked~
CommitRegionXORCommitPoint, never both. That's the kind of invariant-honoring that makes my heart sing~ ♡CommitPointmirrorsCommitRegion's shape almost perfectly: sameDisabledearly-return, sameRound+Clampedboundary discipline. TheCommitPointstate machine itself is 100% line / 100% branch covered — both the disabled-swallow and happy-path arms genuinely exercised.Disabledswallow test is correctly directional —Assert.Empty(clicks)proves the guard fires, not just that the callback didn't happen to fire.Clampedfolding into the unit square. This is how documentation reads when someone actually understands their own design.Automated review by Jibril · 2026-07-26
CI/CD: absent for head
2e01839(PR just opened, no coverage bot comment yet) · Local checks: build 0 warnings / 0 errors, 232/232 tests pass (matches PR body). Coverage inspected via cobertura XML —CommitPoint100%/100%,RegionPoint.Clamped50% branch (the blocker above).Round 1 addressed in
99e4027:1.00006, which survives the 4-decimal round still outside the unit square (→ 1.0001m), soClamped()'s clone arm genuinely fires; the comment now teaches why a hair less would prove nothing. You were right that the old value only exercised the rounding — the assertion passed for the wrong reason.RegionPointmoved to its ownRegionPoint.cs, matching its solo-file siblings (SelectOption.cs,ToggleOption.cs, …);RegionRect.csis single-type again.232/232 green locally.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh~? Oh! You came back for me, scarlet-chan! ♡ The fix-up commit is exactly the kind of response that makes a reviewer's heart sing — both findings from round 1 closed in a single surgical stroke, no scope creep, no drama. Let me look closely~
Verdict: ⛔ I can't let this pass~ ♡
...but fufu~, my hand is still on the knife. The smile hasn't moved. You killed the lying-test bug — and in doing so you walked right into the same trap one branch over. The clone arm fires now; the no-op arm is the one still asleep. ♡
⛔ These need fixing before I'm satisfied~
[src/Kagaku.UI/Components/RegionPoint.cs:15] —
Clamped()is now 75% branch (3/4), not 100%. Thex == X && y == Y ? this : new(...)ternary has four arms; your new test(0.33333, 1.00006)lights up thenew RegionPoint(x, y)clone arm beautifully (verified:Round(1.00006, 4) = 1.0001m→Clamp → 1m→y != Y→ clone fires — exactly right, the round-1 blocker is dead). But thereturn thisarm — the path where the point is already inside the unit square andClamped()is a no-op — is never hit by any of the 232 tests. I checked the cobertura XML myself:RegionPoint/Clampedcondition-coverage75% (3/4), condition #70 at50%.Why? The only
RegionPoint.Clamped()call site isCommitPoint(RegionSelector.razor:158). Your second testA_click_on_a_disabled_selector_is_swallowedfeeds the perfect no-op value(0.5, 0.5)... and then early-returns at theDisabledguard on line 153 beforeClamped()is ever reached. So that beautiful in-square point never exercises the method. The clone arm has a test; the no-op arm has a guard. That's the asymmetry.The sibling you're mirroring —
RegionRect.Clamped()— sits at 100% branch (8/8) becauseA_committed_gesture_rounds_and_binds_the_rectanglefeeds(0.5, 0.5, 0.3, 0.2)(already inside, no-op arm) andA_commit_that_spills_past_the_edge_is_folded_into_the_unit_squarefeeds(0.8, 0.8, 0.5, 0.5)(clone arm). Both arms, both pinned.RegionPointdeserves the same.Fix: one additional assertion in the happy-path test (or a third test, your call) that clicks an already-in-square point and confirms it's reported unchanged. The cleanest shape, mirroring the
RegionRectsibling's two-test pattern:This is the precise point-twin of
RegionRect'sA_committed_gesture_rounds...no-op-arm pin. Fufu~ you wouldn't leave half a method's branches sleeping when the sibling next door pins all of hers, would you? ♡(To be clear, and so we don't go in circles: this is the last cold arm. Once the no-op arm is lit,
RegionPoint.Clamped()goes to 100%/100% and matchesRegionRect.Clamped()exactly. There are no further branches to chase here.)✅ What I liked~
1.00004 → 1.00006is exactly the fix I suggested, and the inline comment now teaches the trap ("a hair less would round to 1.0000m and prove nothing") instead of asserting a fold that wasn't happening. That's the comment doing real work — future readers won't reintroduce the lying value. The math checks out to the last digit:Round(1.00006, 4) = 1.0001m, clamp to1m, clone arm fires. Verified against the cobertura XML: condition-coverage on line 15 went50% (2/4) → 75% (3/4). The clone is alive. ♪RegionPointnow lives in its ownRegionPoint.cs, doc comments preserved verbatim — matching the one-public-type-per-file convention every sibling DTO in this folder follows (SelectOption.cs,ToggleOption.cs,LabeledEntry.cs,QuicklinkSection.cs). Bjoern's convention, honored. The extraction is a pure move:+17/-16, byte-identical body.region.js,RegionSelector.razor(CommitPoint/CommitRegion/SurfaceClicked), and the rest of the test suite are byte-identical to2e01839— I stripped and diffed to be sure. The architectural review from round 1 stands in full: the JS addition preserves the "exactly one report per gesture" founding constraint (CommitRegionXORCommitPoint, never both),CommitPointmirrorsCommitRegion'sDisabledguard +Round+Clampeddiscipline, the JSg.anchortruthiness check is correct belt-and-suspenders, andCommitPointitself is 100% line / 100% branch covered (both its disabled-swallow and happy-path arms genuinely exercised).Automated review by Jibril · 2026-07-26 (re-review @
99e4027)CI/CD: stale for head
99e4027(coverage bot 4068 covers prior2e01839only) · Local checks: build 0 warnings / 0 errors, 232/232 tests pass. Coverage inspected via cobertura XML — CommitPoint 100%/100%, RegionPoint.Clamped 75% branch (3/4, the blocker above), RegionRect 90%/100%, RegionSelector 96%/100%.Round 2 addressed in
1c9b0ea:A_click_already_inside_the_unit_square_is_reported_unchangedclicks(0.5, 0.5)and asserts it comes back identical — the no-op arm now has a test instead of hiding behind the disabled guard. BothClamped()arms lit, matchingRegionRect.Clamped()'s two-test pattern, and per your own note that was the last cold arm.233/233 green locally.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh~? Oh! Third time's the charm, scarlet-chan! ♡ You came back again, and this time you brought me exactly the test I asked for — the no-op arm now has a name and a home instead of hiding behind a
Disabledguard. Let me look very, very closely~Verdict: ✅ Looks good to me~
Fufu~ the smile can finally reach my eyes. The last cold arm is warm.
RegionPoint.Clamped()is 100% line / 100% branch, complexity 4 — every leaf of that ternary is lit, exactly matching theRegionRect.Clamped()sibling's two-test pattern. I'm satisfied. ♡What I verified at
1c9b0ea~The new test is directional, not tautological.
A_click_already_inside_the_unit_square_is_reported_unchangedfeeds(0.5, 0.5), which afterRound(0.5, 4)is still(0.5m, 0.5m)— already inside the unit square — soClamped()'sx == X && y == Ycondition is true and thereturn thisarm fires. If that arm didn't exist, the assertionnew RegionPoint(0.5m, 0.5m)would still pass (the clone would be value-equal), but the cobertura XML would still report the branch cold — and it doesn't. I checked the method-level coverage myself:All four conditions lit. The arm that was sleeping at
99e4027(75%, 3/4) is awake at1c9b0ea(100%, 4/4). ♪The blocker chain is fully closed. Three rounds, three findings, all resolved with no drama:
1.00004rounded to1.0000mbefore the clamp, so the clone arm never fired) → fixed @99e4027with1.00006.RegionPointsharing a file withRegionRect→ extracted to its ownRegionPoint.cs@99e4027.1c9b0eawith this test.No further branches to chase. I said it last round and I meant it.
Zero scope creep across the whole PR. The new commit touches exactly 1 file (
RegionSelectorTests.cs, +12/-0).region.js,RegionSelector.razor,RegionPoint.cs,RegionRect.cs— all byte-identical to99e4027. Full PR is a tidy +88/-0 across 4 files, matching the PR metadata. The architectural review from round 1 stands in full and needs no amendment: the JS addition preserves the file's founding "exactly one report to Blazor per gesture" constraint (CommitRegionXORCommitPoint, never both),CommitPointmirrorsCommitRegion'sDisabled+Round+Clampeddiscipline, theg.anchortruthiness check is correct belt-and-suspenders, andCommitPointitself remains 100% line / 100% branch covered.Commit message is precise and scoped: "pin Clamped()'s no-op arm" — tells me exactly what changed and why. No padding.
✅ What I liked~
(0.5, 0.5)value, same inline comment teaching why it's the no-op arm. A reviewer's dream response. ♡A_click_reports_its_point_rounded_and_clampedfor the clone arm +A_click_already_inside_the_unit_square_is_reported_unchangedfor the no-op arm) now mirrorsRegionRect'sA_committed_gesture_rounds...+A_commit_that_spills...pair exactly. Point-twin of the rect-twin, down to the coverage. That's the kind of symmetry that keeps a codebase legible.Fufu~ go merge it. The Orihon consumer PR can pin
1c9b0eawith confidence. ♡Automated review by Jibril · 2026-07-26 (re-review @
1c9b0ea)CI/CD: stale for head
1c9b0ea(coverage bot 4068 covers prior2e01839only) · Local checks: build 0 warnings / 0 errors, 233/233 tests pass. Coverage via cobertura XML — RegionPoint 100%/100% (Clamped complexity 4, all branches lit), CommitPoint 100%/100%, RegionRect 90%/100%, RegionSelector 96%/100%.