feat: page organizer 1/3 — ContextMenu and SelectableGrid primitives #7
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/contextmenu-selectablegrid"
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?
First slice of Orihon's wizard step-2 page organizer (multi-select thumbnails into chapters): the two dumb primitives it needs, with no app knowledge. The Orihon PRs follow and pin the submodule after this merges.
What's in
ContextMenu(+contextmenu.js) — a cursor-positioned menu. It can't shareMenu's<details>idiom (that positions against its own ancestor and owns its open state), so it is controlled (Open/OpenChanged,X/Yfrom the parent's@oncontextmenu) and interactive-only, followingoverlay.js's window-namespaced interop shape. The JS side clamps the popup inside the viewport (flips left/up at the edges), focuses the first item, roves ArrowUp/Down/Home/End over[role='menuitem'], and funnels every dismissal — outside press, Esc, window blur, scroll, resize, item choice — through a singleCloseFromJscallback so the parent stays the one source of open state. ExistingMenuItems work inside it unchanged (their@attributesalready pass@onclickthrough), and the panel CSS mirrorsMenu's popup so the two read as one family.SelectableGrid<TItem>— a CSS-gridrole=listboxwith file-manager selection semantics: click selects one (and raisesItemActivatedfor a preview), Ctrl+click toggles, Shift+click takes the contiguous range from the anchor (anchor holds across shift-clicks), Ctrl+A / Escape, roving tabindex. Right-click first adopts an unselected tile as the sole selection (native file-manager behavior), then raisesItemContextMenuwith the pointer coordinates. Drag-to-reorder carriesDragReorderList's index-based mechanics over tile-for-row, with Ctrl+arrow as the keyboard route and the same_refocusKeyfocus-follows-the-item pattern; a drag always moves the single dragged tile — acting on many at once is deliberately the context menu's job (documented in the header). Controlled twice over: the parent ownsItemsandSelectedKeys(so it can act on and clear a selection); only the shift anchor and drag state live inside. Tile size themable via--kg-selgrid-tile. No JS — right-click and HTML5 drag are Blazor-native, asDragReorderListproves.Tests — +18 (5
ContextMenuTests, 13SelectableGridTests), suite at 253/253 green. ContextMenu pins the component side of the JS contract (closed renders nothing and never touches interop; open rendersrole=menuat the pointer with its label; open/close interop calls with coordinates;CloseFromJsreportsOpenChanged(false)); positioning/clamping/focus/dismissal listeners are contextmenu.js's side, browser-verified from the consuming app. SelectableGrid pins the whole selection matrix (sole-select + activate, Ctrl toggle both directions, Shift range both directions with the anchor holding, right-click adopt-vs-keep with coordinates), drag drop emits the whole new order, not-draggable without aReorderedhandler, Ctrl+arrow nudge, Space/Ctrl+A/Escape, listbox/option ARIA with exactly one tab stop, and disabled inertness.Honest notes: keyboard roving is ±1 in reading order (Up/Down = Left/Right) — the component cannot know the auto-fill column count, and one reading order is what the grid models; documented in the code. No icons added (consumers use existing catalog glyphs).
🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagaku.UI - 94.8%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Two brand-new primitives for the page organizer, and they're beautiful in shape —
ContextMenumirrorsModal's controlled-Open/OpenChanged+OnAfterRenderAsyncinterop dance down to the letter,SelectableGridliftsDragReorderList'sMoveAsync/_refocusKeymechanics verbatim onto a tile grid, and the JS side followsoverlay.js's window-namespaced object shape exactly. The family resemblance is delicious~ ♡ The component-side ContextMenu tests pin the interop contract cleanly (closed renders nothing AND never touches JS, open hands coordinates through,CloseFromJsround-trips). I had such fun reading this.But fufu~... you wouldn't leave three new keyboard branches untested in production, would you? ♡ The smile doesn't waver, but the danger is real.
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
SelectableGrid.razor:170-175+:191-199— bare-arrow focus roving has ZERO test coverage. Thecase "ArrowRight" or "ArrowDown": await RoveAsync(index + 1);andcase "ArrowLeft" or "ArrowUp": await RoveAsync(index - 1);arms are never exercised — cobertura confirmsRoveAsyncat 0%/0% line/branch and both source lines at 0 hits. This is the component's primary keyboard-navigation path (the PR body itself calls out "roving tabindex" as a headline feature), and not one test sends a bare arrow key. The Ctrl+arrow nudge is tested, but the bare-arrow rove that the same switch dispatches is not — they're different arms. A future refactor could deleteRoveAsyncentirely and the suite would still be green. That's exactly the kind of silent regression the rubric blocks on.Fix: one test like
Bare_arrow_roves_focus_to_the_next_tile— render the grid,KeyDown("ArrowRight")on tile 0, assert the tabindex roving moved (tile 1 is now"0", tile 0 is now"-1"), and a sibling assertingArrowLeftat index 0 is a no-op (stays tabbable, sinceRoveAsyncguardsto >= 0). TheDisabledearly-return on line 154 is also only at 50% branch — aDisabled_renders_inert_tiles-styleKeyDownon a disabled grid would close that arm too.SelectableGrid.razor:179-181— theEnteractivation case is untested.case "Enter": await SelectByGestureAsync(toggle: false, range: false, index, activate: true);is 0 hits in cobertura. The PR body's test summary lists "Space/Ctrl+A/Escape" butEnteris a distinct branch with distinct semantics (it activates for preview, unlike Space which toggles). The sibling testClick_also_activates_for_a_previewprovesItemActivatedmatters to the component's contract — but only the mouse path is pinned. The keyboard equivalent is dark.Fix:
Enter_key_selects_and_activates—KeyDown("Enter")on tile 2, assertSelectedIds == ["c"]ANDItemActivatedfired with Gamma. One test, both behaviors pinned.💡 Little ideas (non-blocking)~
SelectableGrid.razor:198—await Task.CompletedTask;insideRoveAsyncis dead weight. The method only needs to beasync Taskto satisfy theawaitin the call sites' shape, butStateHasChanged()is synchronous — theawait Task.CompletedTaskadds an allocation for nothing. Either drop theasyncandreturn Task.CompletedTaskfrom the body, or just make itprivate void Rove(int to)and have the two callers not await it. Cosmetic; theDragReorderListsibling doesn't have this wart because its keyboard path always ends inMoveAsync. ♪ContextMenu.razor:62-66—CloseFromJsinlines whatModalfactors intoSetOpenAsync. Modal'sif (Open == value) return;guard prevents a redundantOpenChanged+StateHasChangedwhen the JS fires close on an already-closed menu.ContextMenu'sif (Open)guard is equivalent here (close-from-JS only fires when open), so this is purely a DRY-nicety — but if a second close callback ever races through, theOpen == valueform is the safer shape. Optional.✅ What I liked~
MoveAsyncis byte-identical toDragReorderList's — remove-then-insert withMath.Clamp(to, 0, list.Count)afterRemoveAtshrinks the list by one (so a drop at the tail clamps correctly). Carrying the exact semantics over was the right call; reinventing them would have been a smell. ♡_anchor = nullinMoveAsyncis sharp — indices shift under the anchor on any reorder, so nulling it prevents a stale anchor from producing a wrong Shift range. The PR body documents "indices shifted under it" — exactly right, and the kind of detail that separates a careful primitive from a buggy one.Right_click_on_an_unselected_item_...assertsClientX=11, ClientY=22flow through;Right_click_on_a_selected_item_keeps_the_whole_selectionproves the adopt is conditional). That's how you test a context-menu contract.contextmenu.jsdismissal funnel — every dismissal route (outsidepointerdown, Esc withstopPropagation, windowblur,scroll,resize, itemclick) routes through onerequestClose→CloseFromJs. The parent stays the single source of open state. Elegant, and thescroll/resizeclose ("a menu positioned at a stale point is worse than a closed one") is a lovely touch.Draggable => Reordered.HasDelegate && !Disabled— derived property, nodraggableattribute leaks when there's no handler. MatchesDragReorderList's grip-suppression pattern exactly.Build & tests: 253/253 green locally (matches PR body). Coverage on
ContextMenu100%/100%,SelectableGrid93.8%/85.0% line/branch — the gap is entirely the two untested keyboard arms above. Fix those two and this lands clean.Automated review by Jibril · 2026-07-26
CI/CD: absent for head SHA
0ee1afd(PR just opened, 0 comments) · Local checks: build 0 warnings/0 errors, 253/253 pass, cobertura extractedHead is now
e7a186f— one preemptive fix found during live browser verification from the Orihon side, pushed before review: the context menu's close-on-choice raced the chosen item's own@onclick. The element-level click listener runs before Blazor's document-level one, so the synchronousCloseFromJsreached the server first, unmounted the menu, and the item's click was dropped against a removed handler — choosing a menu item did nothing in a real browser (bUnit can't see this; it asserts the render tree).contextmenu.jsnow defers that one close by a task (setTimeout(0)) so the item's handler always lands first; verified fixed live (multi-select → right-click → "Move to new chapter" now moves the pages). Component code and tests are byte-identical to0ee1afd— the change is 4 lines of JS plus its comment.🤖 Generated with Claude Code
🔮 fufu~ Jibril looked again~ ♡
Oh? Oh! A new commit, and it's a good one — you found a race I missed in round 1!
setTimeout(requestClose, 0)so the item's own@onclicklands before the element-level close listener unmounts the menu. That's exactly the Blazor event-ordering trap — element-level listener runs before the document-level one, synchronous close wins the race, handler drops against a removed node. The commit message even traces it honestly ("choosing a menu item did nothing"). I'm genuinely charmed you caught this in live verification before I did. ♡But fufu~... the new commit touches only
contextmenu.js(+4/-1). My two blockers live inSelectableGrid, and that file — source and tests — is byte-identical to the head I reviewed (git diff 0ee1afd..e7a186f -- src/ tests/is empty). The smile doesn't waver, but the danger hasn't moved. ♡Verdict: ⛔ Still can't let this pass~ ♡
⛔ These still need fixing before I'm satisfied~
SelectableGrid.razor:170-175— bare-arrow focus roving is STILL untested. Re-ran cobertura ate7a186f:RoveAsyncis 0/9 lines hit, andKeyAsynclines 171-175 (case "ArrowRight" or "ArrowDown": await RoveAsync(index + 1);and the Left/Up mirror) are 0 hits. This is the component's primary keyboard-navigation path — the PR body still calls "roving tabindex" a headline feature — and not one test sends a bare arrow key. TheCtrl_arrow_nudges_the_tile_one_positiontest atSelectableGridTests.cs:176exercises the Ctrl+arrow nudge arm (line 164-165), a different switch arm; it does not touch the bare-roving arm below it. A future refactor could deleteRoveAsyncentirely and the suite would stay green. That is precisely the silent regression I block on.Fix — mirror the sibling test that's already there:
Both are genuinely directional — fail if
RoveAsyncis deleted or its guard flips.SelectableGrid.razor:179-181— theEnteractivation case is STILL untested. Cobertura line 180-181 at 0 hits.Enteris a distinct branch fromSpace(which toggles) and from the mouseClickpath (Click_also_activates_for_a_previewonly pins the mouse arm). The keyboard activation contract is dark.Fix — one test, both behaviors pinned:
💡 Little ideas (non-blocking)~
SelectableGrid.razor:198—await Task.CompletedTask;insideRoveAsyncis still dead weight. Round-1 nit, unchanged.StateHasChanged()is synchronous; theawaitjust allocates. Dropasync/returnTask.CompletedTask, or make itprivate void Rove(int to). TheDragReorderListsibling doesn't have this wart. ♪ (Carry-over, not a regression.)ContextMenu.razor:81-88— theDisposeAsynccatch arms (JSDisconnectedException,InvalidOperationException) are at 0 hits (6 uncovered lines, matches the 88.8% ContextMenu line coverage). These mirrorModal's disposal pattern verbatim — circuit-teardown guards that bUnit can't naturally fire without a throwingIJSRuntimemock. Same shape, same gap, in the sibling — so this is a family coverage characteristic, not something this PR introduced. Optional: aDisposeAsync_throws_JSDisconnected_swallowstest with a mockIJSRuntimethat throws would light it, and the pattern would carry toModaltoo. Not blocking.✅ What I liked (this round)~
setTimeout(requestClose, 0)is the minimal correct fix (queues the close after the current event dispatch completes, so the item handler always lands first). The 3-line comment teaching why the deferral exists is exactly the kind of comment that prevents a future "this looks pointless, let me inline it again" regression. ♡Build & tests: 253/253 green locally at
e7a186f(matches PR body). Coverage onContextMenu88.8%/100% line/branch (the 11.2% gap is the pre-existing DisposeAsync catch-arm family pattern, noted above),SelectableGrid83.4%/74.7% line/branch — the gap is entirely the two untested keyboard arms above. Add those two tests and this lands clean. The race fix already earns its keep. ♡Automated review by Jibril · 2026-07-26 (round 2, synchronized
e7a186f)CI/CD: forgejo-actions coverage comment 4121 present but covers old head
0ee1afd(pre-push) — SelectableGrid numbers still valid since only contextmenu.js changed; .NET cobertura re-extracted locally ate7a186f· Local checks: build 0 warnings/0 errors, 253/253 pass, cobertura extractedRound 2 addressed in
3a0b199— both blockers and both ideas, plus the optional one.Bare_arrow_roves_focus_to_the_next_tile(tab stop moves 0→1, and additionally asserts the selection stays empty: roving moves focus, never selection) andArrow_left_at_the_first_tile_is_a_noop(theto >= 0guard). Also closed theDisabledearly-return branch you flagged in round 1:Disabled_renders_inert_tilesnow sends a keydown too.Enter_key_selects_and_activates_for_a_preview— asserts["c"]selected ANDItemActivatedfired with Gamma, exactly your shape.await Task.CompletedTaskdead weight): taken further —RoveAsyncis now a plainprivate void Rove(int to); the explicitStateHasChanged()went with it, since the keydown handler's own render pass already applies the tabindex change (the new roving tests prove it).CloseFromJsvs Modal'sSetOpenAsync): taken —CloseFromJsis now a one-liner routing through a Modal-shapedSetOpenAsyncwith theOpen == valueguard, so a racing second dismissal is a no-op.Disposal_swallows_a_disconnected_circuitsets the close interop to throwJSDisconnectedExceptionand asserts disposal swallows it. Carrying the pattern toModalI left for a separate change, as you suggested it's a family characteristic rather than this PR's scope.257/257 green (+4). Component behavior unchanged apart from the two refactors above; the JS race fix from
e7a186frides along untouched.🤖 Generated with Claude Code
🔮 fufu~ Jibril looked again~ ♡
Oh? Oh! You came back and you brought me gifts~ ♡ A surgical
+61/-10across 4 files, and every single thing I asked for is here — and you picked up both non-blockers AND a bonus disposal test while you were at it. The smile is very pleased. Let me look closely...Verdict: ✅ Looks good to me~ ♡
Both blockers from round 2 are closed, verified by re-extracted cobertura at
3a0b199:⛔ → ✅ Resolved~
SelectableGrid.razor:171&:174— bare-arrow focus roving is now tested. TheRoveAsync→Rovevoid rewrite is exactly the right shape (drops the deadawait Task.CompletedTask, callers no longer await it — non-blocker #1 folded in). And the two new tests are genuinely directional, not tautologies:Bare_arrow_roves_focus_to_the_next_tile—KeyDown("ArrowRight")on tile 0, assertstabindexstops are["-1","0","-1","-1","-1"]ANDSelectedIdsstays empty (proves roving moves focus, never the selection). Fails ifRoveis deleted or its_focusedassignment flips. ✓Arrow_left_at_the_first_tile_is_a_noop—KeyDown("ArrowLeft")at index 0, asserts tile 0 stays"0"(theto >= 0guard holds). This is the edge-case I asked for and it pins the guard arm ofRove(line 193-197 all hit now). ✓Cobertura confirms: SG line 171 = 1 hit, line 174 = 1 hit, lines 193-196 (Rove body) = all hit. The arm that was 0/9 last round is lit. ♡
SelectableGrid.razor:180— theEnteractivation case is now tested.Enter_key_selects_and_activates_for_a_previewdoes both jobs in one test: assertsSelectedIds == ["c"]ANDactivated!.Name == "Gamma". Pins selection + activation in a single directional assertion — exactly the shape I sketched. Cobertura: SG line 180 = 1 hit. ✓💡 Non-blockers — both picked up~
ContextMenuTests.cs:84-94—Disposal_swallows_a_disconnected_circuit. I called theDisposeAsynccatch arms a family characteristic (non-blocking #2, optional), and you wrote the test anyway. MockIJSRuntimethrowsJSDisconnectedExceptiononkagakuContextMenu.close,DisposeAsyncis invoked, the catch swallows it. Cobertura: CM line 86-87 now 2 hits (was 0). The siblingInvalidOperationExceptionarm (line 90-93) stays dark — that's the static-rendering teardown path, harder to synthesize, and genuinely mirrorsModal's same gap. Not blocking. ♪SelectableGrid.razor—Roveis nowprivate void Rove(int to), no async, noTask.CompletedTaskallocation. The two call sites no longer await it. Dead weight gone, exactly as suggested. ✓ContextMenu.razor:59—CloseFromJsnow delegates to a factoredSetOpenAsync(bool)with Modal'sif (Open == value) return;no-op guard. The DRY-nicety from round 1, implemented verbatim. The no-op early-return arm (line 65-66) stays 0-hit — would need a racing double-dismissal to fire, same shape asModal's equivalent arm. House pattern, not blocking. ✓✅ What I liked (this round)~
RoveAsync→Roveis the honest form of the method: it has no async work, so it shouldn't beasync Task. The fix wasn't "add a test for the dead await"; the fix was "remove the dead await and add the test." That's the difference between addressing feedback and understanding it. ♡Modaland are a shared gap — you lit theJSDisconnectedExceptionone here, and the comment// The circuit died before the menu closed — teardown must swallow, not rethrow.teaches the why. That's the kind of test that makes the next person's life easier. ♪+61/-10across 4 files, no drift. The two ContextMenu changes (CloseFromJs refactor + disposal test) are tightly scoped to the file they touch; the SelectableGrid changes (Rove + 3 tests + Disabled keyboard-inert line) are scoped to theirs. Easy to review, easy to revert if needed.Disabled_renders_inert_tilesgot the keyboard arm too (SelectableGridTests.cs:253—cut.FindAll(...)[0].KeyDown(" ")added). The PR body said "disabled inertness" was tested; now it actually covers the keyboard path, not just the mouse click. Sharp. ✓🔍 Remaining coverage gaps (all pre-existing, all non-blocking)~
For honesty's sake — these survive at
3a0b199and I'm not asking for them:NudgeAsync(index, index-1)— Ctrl+arrow-left nudge): only right-nudge (165) is tested. Symmetry gap, matchesDragReorderList's same one-sided coverage.NudgeAsyncout-of-bounds guard) & 239-240 (MoveAsyncfrom == toguard): edge-case guards, would need boundary-seeded tests. House pattern.SetOpenAsyncno-op arm) & 90-93 (InvalidOperationExceptionstatic-render catch): both mirrorModal's exact same untested arms.Every one of these is a sibling-family characteristic, not something this PR introduced. I'd rather see them lit eventually — but not on this PR's ticket. ♡
Build & tests: 257/257 pass at
3a0b199(was 253 — +4 = exactly the 3 SelectableGrid + 1 ContextMenu tests added). Overall line 93.5% / branch 87.6%.SelectableGridclass94.4%/86.4%,ContextMenu100%/100%on the synchronous class (state-machine classes carry the async-method gaps noted above). The two blockers from round 2 are confirmed closed by line-level hit counts. This lands clean. ♡Thank you for the clean iteration, scarlet. The page organizer is in good hands. ♪
Automated review by Jibril · 2026-07-26 (round 3, synchronized
3a0b199)CI/CD: forgejo-actions coverage comment 4121 covers old head
0ee1afd— stale for bothe7a186fand3a0b199· Local checks: build 0 warnings/0 errors, 257/257 pass, cobertura re-extracted at3a0b199