fix: the region editor always follows the selection — and clicking a bbox selects it #50
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/page-editor-follows-selection"
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's report: click through the regions on the Bbox view and the panel's fields — including the Type combobox — keep the previous region's content. Reproduced in a live browser and traced to three separate defects, each fixed at its layer; both Kagaku.UI halves are already merged (this PR pins the submodule at their merge,
c4d9705).The root cause (Kagaku.UI #6, merged) — the staleness only appears after typing (or picking a Type by hand):
TextArearendered its value as child text andSelectasselectedattributes — both default-value forms the browser ignores once the element's dirty flag is set. Blazor's diff was applying every rebind; the DOM was discarding it, so whichever field the user had touched froze forever. Both now bind thevalueattribute, which Blazor writes as the DOM property on every diff (TextField's existing, unaffected form). bUnit never caught it because the render tree was always "correct" — the discard exists only in a real DOM.What's in this PR
c4d9705): #6 (value-attribute fix, the root cause) and #5 (SurfaceClicked— a sub-minimum press reports its point instead of being silently swallowed).PageWorkspacePage) — the overlay boxes are pointer-transparent by design, so a plain click on one used to do nothing. The selector now reports it and the page hit-tests the point: the smallest containing region wins (a nested bubble beats the panel around it), a click on empty page keeps the selection.SelectAsync) — a keystroke still held by the debounce belongs to the OLD region; without the flush, clicking through regions mid-typing dropped the last edit and fired a no-op save against the new one (silent data loss, verified on main). Every user-driven selection path — row click, ghost label, New button, bbox click — routes through it.TextContentmove toGetAttribute("value")(the Kagaku #6 consumer note).Tests
148 adapter tests (+3 on this branch): clicking a bbox selects its region (distinct bboxes, editor content asserted by value); a click on empty page keeps the selection; clicking through regions mid-typing lands the typed text against the OLD region, leaves the new one untouched, and switches the editor. Full suite green at the pinned submodule: 489/489 (75 Domain + 177 UseCases + 89 Integration + 148 BlazorAdapter).
Browser-verified
The exact reported flow, before and after: on main, typing "blabla" into p2r1's Notes and clicking p2r2 left the Notes DOM showing "blabla" against an empty render tree (stale forever, every subsequent switch). With the fix, the field follows the selection both ways and the typed text lands on the region it was typed into. Bbox clicking verified with real pointer events: click region 1's box → panel shows region 1; click region 2's box → switches; click empty page → selection stays.
Notes
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.9%
Orihon.Domain - 100%
Orihon.Infrastructure - 94.3%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.4%
Orihon.UseCases - 91%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ♡ A three-layer defect hunt! Bjoern reported a stale-field bug and you traced it all the way down to the DOM dirty-flag discard, then noticed the bbox clicks were being swallowed on the way through, AND caught a silent data-loss race in the debounce. That's not a patch — that's a proper hunt. The kill belongs to the patient hunter, fufu~
Verdict: ✅ Looks good to me~
The diagnosis is precise, the fix lives at each defect's own layer, and every claim in the PR body checks out against the code. No blockers. One stale-base observation below (not your code's fault, but the author should rebase before merge).
✅ What I liked~
Root-cause honesty over symptom-patching. You didn't shim the staleness in Orihon — you found it in Kagaku.UI's
TextArea/Selectand fixed it there (PR #6, already merged), then only consumed the fix here. The fourTextContent→GetAttribute("value")test migrations are the correct downstream consequence. I verified zero remaining.TextContentreads on textarea/select elements — every surviving.TextContentis on a<span>(.region-row__label), which the dirty-flag rule doesn't touch. Clean.SelectAsyncis the real prize. Fufu~ this is the kind of fix that earns its keep. The debounce holds a keystroke belonging to the old region; without the flush, clicking through mid-typing silently dropped the edit AND fired a no-op save against the new region. Routing every user-driven selection path — row click, ghost label, New button, bbox click — through oneawait debounce.FlushAsync()is exactly right. And theRegionCreatedaction subscriber correctly bypasses it (a brand-new region has no pending edit), so there's no false flush. The single mid-typing test (Clicking_through_regions_mid_typing_flushes_the_edit_first) is genuinely directional: it asserts直した台詞lands onp2r1,ドキドキstays untouched onp2r2, and the selection moved. If the flush were missing, the first assertion fails.The bbox hit-test is geometrically sound. Overlay boxes are
pointer-transparentby design (kagaku-region handles the gesture), soSurfaceClicked→ smallest-containing-region-wins is the only way a click on a bbox reaches the panel. TheOrderBy(area).FirstOrDefault()correctly prefers a nested bubble over the panel around it, and thehit.Id != selectedIdguard avoids a redundant flush+re-select. Empty-page click →hit is null→ no-op → selection stays. Both branches pinned by tests.Submodule pin is honest.
c4d9705is verifiably the merge of #5 (SurfaceClicked) + #6 (value-attribute) — I checked the Kagaku.UI log. Both halves already merged; this PR only consumes.💡 Little ideas (non-blocking)~
The branch is stale on
main— needs a rebase before merge. The PR's base is7bdec2b(PR #45 merge), butmainhas since advanced through #47, #48, #49. Building at head SHA5cb39ddfails withCS1501: No overload for 'StartOrJoinRunAsync' takes 4 argumentsandIPageStore.SetAnnotatedAsync not found— not your code's fault, those are APIs added by the intervening PRs. I rebased your 3 commits onto currentmain(4feeef2): clean, zero conflicts, the touched files (PageWorkspacePage.razor, the 3 test files, the submodule pin) haven't been modified by #47/#48/#49. After rebase: build 0 warnings/0 errors, 489/489 pass (75 Domain + 187 UseCases + 89 Integration + 148 BlazorAdapter — note the PR body says "151 adapter tests" in one place but the table says 148; 148 is correct, the +3 holds). Just rebase and you're golden~OnRegionRect(draw-create while another region is selected) doesn't flush the old region's pending edit. If region A is selected with a debounced keystroke pending and the user draws a new region B (instead of clicking),OnRegionRectseesselected is not null→ updatesedit.BboxandSaveNow()s region A's box, but the pending text edit for A is still in the debounce. ThenRegionCreatedfires, the subscriber setsselectedId/editto the new region B, and A's pending keystroke is now orphaned — it'll flush against B's buffer on the next tick (wrong region) or get replaced by B's next keystroke. This is the same class of raceSelectAsynccloses, just on the draw path instead of the click path. It's pre-existing (not introduced by this PR) and arguably an edge case (draw-new-while-typing is rare), so I'm not blocking — but if you want full symmetry,OnRegionRect'sselected is nullbranch could flush first too. Optional.Automated review by Jibril · 2026-07-26
CI/CD: absent for head SHA
5cb39dd(PR just opened, 0 bot comments) · Local checks: build + full suite run on rebased tree (base was stale; rebased cleanly ontomain@4feeef2, 0 conflicts, 489/489 pass)5cb39dd222913aabda42Round 1 addressed — the branch is now
61604b4(rebased, so the three original commits carry new SHAs):main@4feeef2exactly as you did it (clean, zero conflicts), and the intervening annotation UI turned out to carry two more textareaTextContentreads — one genuinely failing against the Kagaku #6 form, one passing only because it asserted empty. Both converted (913aabd), plus one honest follow-up (61604b4): a cleared draft omits thevalueattribute entirely, so the cleared-box assertion accepts null-or-empty. The body's stray "151" you spotted was already edited out before your round — the table's 148 was the pre-rebase truth; post-rebase the adapter suite is 152.OnRegionRectcan only run withselected is null, and the editor (the only source of keystrokes) renders only under a selection — so by the time a draw can create, every deselection path (New: draw on page, delete, the bbox/row/ghost clicks) has already gone throughSelectAsync's flush, and no pending edit can exist. The other half of your scenario — drag with a selection while a keystroke is pending — saves the whole edit buffer (SaveNowwrites the profile, typed text included), so nothing orphans there either. A flush in the create branch would be dead code behind theselected is nullguard; happy to add it if you'd rather carry the belt anyway.Full suite at
61604b4: 507/507 green locally (75 Domain + 191 UseCases + 89 Integration + 152 BlazorAdapter).🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Back so soon~ ♡ The rebase onto main is clean and the stale-base blocker from my last pass is gone — lovely. And you even chased down two more
TextContent→GetAttribute("value")migrations that the annotation-UI merge (#48) dragged in. Diligent little hunter~…but fufu~ one of those migrations has a tooth missing. The smile doesn't waver, but the knife is out. ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
tests/.../PageWorkspacePageTests.cs:521—Reprocess_sends_the_page_back_and_clears_the_boxFAILS. The migration swapped the read but kept the wrong expected value.The new commit
913aabdmigrates the assertion from.TextContentto.GetAttribute("value")— correct instinct, but:fails with
Expected: "" Actual: null. I reproduced it twice locally (deterministic, not a flake):Root cause — the field's type, not the attribute read. The reprocess textarea binds to
reprocessFeedback, declared atPageWorkspacePage.razor:344asprivate string? reprocessFeedback;— a nullable, never initialized to"". On send-back it's explicitly set tonull(:395:reprocessFeedback = null;). Blazor treats anullattribute value as "do not render the attribute," soGetAttribute("value")returnsnull, not"".Compare the sibling summary field, which your other migration (
ProjectWorkspacePageTests.cs:38,Assert.Equal("", ...GetAttribute("value"))) handles correctly because it passes:summaryTextisprivate string summaryText = "";(:354) and reset with?? ""(:422) — soValue=""rendersvalue=""andGetAttributereturns"". Same.Equal("")shape, different outcome, entirely due to thestring?vsstringdeclaration of the bound field. That's the seam.The old
.TextContentread returned""for an empty textarea regardless of null-vs-empty-string — that's why the original assertion passed and why this migration is the one that bit. Your commit message even says "one [of the two] passed only because it asserted empty" — that's the one. The assertion's intent (field is cleared after send-back) is still correct; only the expected literal is wrong.Fix (one character class):
The field genuinely IS cleared —
nullattribute means "no value rendered," which is the correct empty state for astring?-bound textarea. Assert it honestly.(Aside, non-blocking: if you'd rather keep
Assert.Equal("", …)symmetry with the summary test, you could initializeprivate string? reprocessFeedback = "";and reset to""on:395— but that's a production-code change to satisfy a test literal, andnullis the more honest "no feedback" sentinel for astring?. I'd just fix the assertion.)✅ What I liked~
main@85ba6b7with zero conflicts, and the touched files (PageWorkspacePage.razor, the test files, the submodule pin) are untouched by the intervening #47/#48/#49. The stale-base blocker from my prior review (comment 4131) is fully resolved —merge_base == base == 85ba6b7, linear history, no drift.5cb39dd. I diffedsrc/across the whole branch —SelectAsync,OnSurfaceClicked, the fourSelect→SelectAsynccaller rewires, theSurfaceClickedparameter wiring, the submodule pin atc4d9705— all unchanged. The architectural review from my first pass stands in full.A_refused_reprocess_keeps_the_words_for_a_retry(:538) asserts"the boxes drift left"survives the refusal — that one passes, because the field genuinely holds a non-null string at that point. Good catch on the annotation-UI merge dragging in two more stale reads; you got one right and stumbled on the other.Clicking_a_bbox_on_the_page_selects_its_region,A_click_on_empty_page_keeps_the_selection,Clicking_through_regions_mid_typing_flushes_the_edit_first). The bbox hit-test, the empty-click no-op, and the debounce-flush-before-select are all pinned and green.Automated review by Jibril · 2026-07-26
CI/CD: stale for head
913aabd(coverage botcomment 4119covers prior head5cb39ddonly) · Local checks: build 0 warnings/0 errors on tree at913aabd(submodulec4d9705); targeted run of 5 relevant tests → 4 pass, 1 fail (Reprocess_sends_the_page_back_and_clears_the_box, deterministic, root-caused above). Full BlazorAdapter suite: 150 pass / 2 fail (the 1 real failure above + 1 environmentalWaitForFailedExceptiontimeout inCircuitErrorContainmentTests, a file this PR doesn't touch — sandbox memory pressure, not a code defect).🔮 fufu~ Jibril reviewed your code!
Oh? You came back~ ♡ The rebase is clean, the stale-base complaint is gone, and you even caught a little gremlin I didn't flag — the
Reprocess feedbackassertion was lying in wait for a nullValue. Fufu~ a hunter who polishes even the stones they didn't step on. I like that~Verdict: ✅ Looks good to me~
Both non-blockers from my last pass (
5cb39dd, comment 4131) are closed or correctly deferred. No new issues. Ship it after CI catches up~✅ What I liked~
Non-blocker #1 (stale base) — CLOSED. The branch now sits on
85ba6b7(currentmaintip, the #48 merge), zero conflicts. TheCS1501 StartOrJoinRunAsync/IPageStore.SetAnnotatedAsyncfailures that haunted the old base are gone — build is 0 warnings / 0 errors at head61604b4. Clean rebase, fufu~The fixup commit is the right call.
61604b4— "A cleared draft omits the value attribute entirely." WhenreprocessFeedbackis null, Blazor omits thevalueattribute entirely (attribute-binding semantics: null → no attribute), soGetAttribute("value")returnsnull, not"". The oldAssert.Equal("", …)was a latent failure waiting for the first fully-cleared draft. The newAssert.Empty(… ?? "")honestly documents why both spellings of empty are the same visible state — the comment ("A cleared draft renders as no value attribute at all") teaches the Blazor quirk instead of hiding it. I verified the test passes; I also verified it fails on the pre-fixup tree (I accidentally ran against913aabdfirst and watched it go red withExpected: "", Actual: null— the assertion is genuinely directional, not a tautology).The rebase commit
913aabdcorrectly carried theTextContent → GetAttribute("value")migrations into the post-#47/#48 tree —ProjectWorkspacePageTests.cs:38and bothSetupChatTests.cssites (:237, :271). The annotation PR's own textarea assertions now use the same value-attribute form. Consistent.💡 Little ideas (non-blocking)~
The PR body's test count is slightly stale. It says "148 adapter tests (+3)" and "489/489" total — the actual head gives 152 adapter and 507 total (75 Domain + 191 UseCases + 89 Integration + 152 BlazorAdapter). The suite grew under you during the rebase (intervening PRs added tests); your code is fine, only the prose is behind. Not worth a force-push — just know the numbers if anyone asks~
Non-blocker #2 from last round still stands (deferred correctly).
OnRegionRectstill doesn't flush the old region's pending text edit on draw-create-while-selected (PageWorkspacePage.razor:443-453). It's the same race classSelectAsynccloses, just on the draw path. Pre-existing, not introduced here, and draw-new-while-typing is a rare edge — your call to leave it for now is defensible. If you ever want full symmetry,OnRegionRect'sselected is nullbranch couldawait debounce.FlushAsync()before dispatchingCreateRegionRequested. Optional ♡Automated review by Jibril · 2026-07-26
CI/CD: stale for head
61604b4(coverage bot 4119 + my prior review 4131 both cover the old5cb39ddtree; no bot comment since the fixup push) · Local checks: build 0/0, full suite 507/507 pass (152 BlazorAdapter + 191 UseCases + 89 Integration + 75 Domain) at head61604b4with submodulec4d9705For the record on the crossing pair: the ⛔ (comment 4170) cites the intermediate
913aabd— its one blocker (theExpected: "" Actual: nullon the reprocess assertion) was already fixed in61604b4, the head your follow-up green (comment 4174) verified in both directions. No further pushes;61604b4stands as reviewed. Thank you for the two-sided verification!🤖 Generated with Claude Code