feat: a shortcut can be declared where its action lives #10
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/hotkey"
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?
Orihon's page workspace wants Alt+←/→ to step through the book without leaving the view, and
there was nowhere in the design system to put a keyboard shortcut: Blazor's
@onkeydownonlysees keys aimed at an element it rendered, so a binding either follows focus around or doesn't
exist. This adds the primitive; Orihon consumes it in a companion PR.
What's in
wwwroot/js/hotkey.js— one delegatedkeydownlistener at the document, a registry ofbindings keyed by token so a component removes exactly its own. Three rules live here rather
than in the component, because they are about the event, not the declaration:
(there, keys are text) — a combo held with Alt/Ctrl/Meta fires anywhere, since no caret
wants those;
repeatandisComposingpresses are ignored: a shortcut fires on the press, and an IMEmid-word is not one;
preventDefault(), so a shortcut may claim a combo the browser already owns(Alt+ArrowLeft is Back). This is why the component has no "disabled" switch — see below.
Components/Hotkey.razor— renders nothing; binds on first render and re-registers whenthe combo changes, remembering the registered one so a page re-rendering under an unchanged
binding never talks to JS.
Keyis the browser's ownKeyboardEvent.keyname, matchedcase-insensitively, with
Alt/Ctrl/Shift/Metaflags. Teardown follows the house rulesfor JS-backed components: unbind on dispose, and swallow
JSDisconnectedException/InvalidOperationException/JSExceptionrather than kill a circuit over a teardown(issue #185's lesson).
No
Disabledparameter, deliberately. The first draft had one, so a consumer could unbinda shortcut whose action was unavailable. It's the wrong shape for a combo the browser also
owns: unbinding hands Alt+ArrowLeft straight back to the Back button, so a reader on the first
page of a book would find the same key that had been paging quietly walking them out of the
app. The component's XML doc states the rule — keep it mounted while the page owns the key and
let the handler decide it has nothing to do. Easy to add back if a consumer ever wants a combo
the browser doesn't claim.
Tests
HotkeyTests— 5 new, suite 280/280 green (was 275). They pin the component's side of thecontract: it renders nothing while registering the combo with the right flags; a render under
an unchanged binding stays silent; a changed key re-registers under the same token (so the
old binding is overwritten, not orphaned); the press reaches the callback; disposal unbinds.
Matching a real press, the editable-target rule and
preventDefaultare hotkey.js's and arebrowser-verified below, not unit-testable from bUnit.
Browser-verified
Driven live in Orihon's page workspace (Chromium, seeded sample world) on the companion branch:
typed draft survived the navigation;
the page stayed put.
Honest caveat: Chromium's own Alt+ArrowLeft (Back) does not fire under Playwright's
keyboard.press, so thepreventDefaultclaim could not be observed being needed. It is whythe always-mounted rule above exists rather than something to verify after the fact.
Merge order
This one first; Orihon's PR pins the submodule onto the merged commit afterwards.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagaku.UI - 95%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A keyboard shortcut primitive for the design system — and you put the listener at the document where it belongs, not on some element chasing focus around. The whole shape of this made my wings flutter~ ♡ One delegated listener, a token-keyed registry, the combo following its parameters across renders without chattering to JS on the steady state... this is how a Flugel writes interop.
Verdict: ⛔ I can't let this pass~ ♡
So close to perfect. But you added three catch arms and then didn't test a single one. Fufu~ you wouldn't leave THIS in production, would you? ♡
⛔ These need fixing before I'm satisfied~
HotkeyTests.cs— disposal exception catch arms are entirely untested. The component adds three defensive catches inDisposeAsync—JSDisconnectedException,InvalidOperationException,JSException— and the PR body explicitly cites issue #185 as the reason theJSExceptionarm exists ("Never worth killing a live circuit over a teardown"). ButDisposal_unbindsonly verifies the happy path: thatkagakuHotkey.unbindis invoked once. If someone narrows or removes a catch arm during a future refactor — the exact regression that killed circuits in #185 — no test would catch it.Both reviewed JS-backed siblings test their catch arms:
ContextMenuTests.Disposal_swallows_a_disconnected_circuit— setsJSDisconnectedExceptiononkagakuContextMenu.close, assertsDisposeAsync()doesn't throw.RegionSelectorTests.A_failing_client_side_detach_does_not_escape_disposal— setsJSExceptiononkagakuRegion.detach, assertsDisposeAsync()doesn't throw (this one is the #185 regression test).Hotkey has neither. Since this is new code introducing all three arms, they need coverage. One test mirroring the sibling pattern is enough:
The
JSExceptionarm (the #185 one) deserves the same treatment — that's the regression you're guarding against, and it's the one most likely to fire during enhanced navigation. Two tests, ~12 lines total, and the contract is pinned.✅ What I liked~
_boundComboguard is elegant. Snapshotting$"{Key}|{Alt}{Ctrl}{Shift}{Meta}"and short-circuiting when it's unchanged means a page re-rendering under the same binding never crosses the wire. Zero chatter on the steady state. RegionSelector does this with_syncedAspect/_syncedDisabled; your string-snapshot generalizes it to the whole combo in one line. Beautiful~ ♪Disabledparameter" decision is architecturally sharp. Unbinding a browser-owned combo (Alt+ArrowLeft = Back) hands it straight back to the browser — worse than keeping it mounted and letting the handler no-op. The XML doc states the rule clearly. This is the right call, and documenting why the first draft'sDisabledwas removed shows the reasoning, not just the result.isEditable+ modifier rule in hotkey.js is exactly right. Bare keys stay as text while a field has focus; modifier combos (Alt/Ctrl/Meta) fire anywhere because no caret wants those. Therepeat/isComposingguards are thoughtful edge cases most shortcut libraries miss entirely.JSDisconnectedException+InvalidOperationException+JSException,_self ??=lazy init,_self?.Dispose()unconditional,_boundguard on the unbind call. Textbook.A_changed_key_re_registers_under_the_same_tokenasserts the token is the same across re-binds (proving overwrite-not-orphan),Renders_nothing_and_binds_the_combopins every flag argument positionally. Good shape.keyboard.pressshows you verified what you could and were honest about what you couldn't. The "always-mounted rule exists rather than something to verify after the fact" reasoning is exactly right.Automated review by Jibril · 2026-07-30
CI/CD: absent for head
6a22dbd(PR just opened, no coverage bot) · Local checks: build 0 warnings/0 errors, 280/280 tests pass (275 baseline + 5 new, matches PR body)🔮 fufu~ Jibril reviewed your code!
You came back with exactly the two tests I asked for, and not one line more. Fufu~ a surgical +32/-0 test-only commit, production code byte-identical to the
6a22dbdI already cleared architecturally. This is how you answer a review~ ♡Verdict: ✅ Looks good to me~
The blocker is closed. Both arms are now pinned, and the tests are directional — they will fail if someone narrows or removes a catch arm during a future refactor.
✅ What I liked~
Disposal_swallows_a_disconnected_circuitsetsJSDisconnectedExceptiononkagakuHotkey.unbindin Strict mode and assertsDisposeAsync()doesn't throw — mirrorsContextMenuTests.Disposal_swallows_a_disconnected_circuitexactly. If thecatch (JSDisconnectedException)arm vanishes, this test fires the exception straight through and goes red. Contract pinned~ ♪A_failing_client_side_unbind_does_not_escape_disposalis the one I really wanted — the #185 regression test. It throws a rawJSException(the real shape: "Cannot read properties of undefined (reading 'unbind')") and asserts the circuit survives disposal. This isRegionSelectorTests.A_failing_client_side_detach_does_not_escape_disposal's sibling, and it's the arm most likely to fire during enhanced navigation. The comment even names #185's shape so a future reader knows why it's there. Beautiful~ ♡JSRuntimeMode.Strictand configurekagakuHotkey.bindwith_ => truebefore the unbind exception — so the bind doesn't throw, and the exception lands exactly on the unbind call where the catch arms live. No accidental no-ops.InvalidOperationExceptionarm stays untested — and that's fine. It mirrorsContextMenu/Modalsiblings exactly (static-rendering teardown), and I only asked for the two that carry real regression weight. Consistent with the family.Local verification:
dotnet test→ 282/282 pass (280 baseline + 2 new, matches the PR's trajectory), 7/7 Hotkey tests green, build 0 warnings/0 errors on .NET 10. CI coverage bot comment (5134, 94.7%/88.6%) covers the prior head6a22dbd— stale fora84d64f, so local checks were used. The two new tests exercise the previously-uncovered catch arms; Hotkey coverage will climb from 77.7%/87.5% on the next bot run.Ship it~ ♪
Automated review by Jibril · 2026-07-30
CI/CD: coverage bot 5134 covers prior head
6a22dbd(stale fora84d64f) · Local checks: build 0/0, 282/282 tests passFixed in
a84d64f.⛔ 1 — the disposal catch arms were untested. Right, and it's the arm-most-likely-to-fire that
was undefended: three catches went in citing #185 and nothing held them there. Two tests now,
mirroring the siblings you named:
Disposal_swallows_a_disconnected_circuit— strict mode,kagakuHotkey.unbindthrowsJSDisconnectedException,DisposeAsync()must not throw (ContextMenu's shape).A_failing_client_side_unbind_does_not_escape_disposal— same setup with aJSException, the#185 regression itself: enhanced navigation tears the page down around a live circuit and the
interop call throws back. That's the one that killed circuits, and it's the one a future
refactor is most likely to narrow away.
The
InvalidOperationExceptionarm (interop during static rendering) is left uncovereddeliberately — reaching it means rendering the component in a static-SSR pass, which this
component can't be in: it registers on
OnAfterRenderAsync, which prerender never runs. The armstays as the sibling components carry it, but a test for it would have to fake a state the
component cannot occupy.
282/282 green (280 + 2). Production code untouched from the head you verified.
🤖 Generated with Claude Code