ui: a keyboard registry, a collision gate, and CodeMirror's keymap folded in - #118
Merged
Merged
Conversation
…lded in
A chord used to be spelled twice — a modifier test in main.ts's keydown
handler and a row of display text in shortcuts.ts — with nothing but
discipline keeping them equal. K1 promises every action has a keyboard
path *and* that the path is findable, so a sheet free to fall behind the
wiring is a promise waiting to break. And because main.ts can't be
imported by node, none of it was testable: the app's whole keyboard
contract sat in the one file the suite can't see.
The chord is now declared once, in `ui/src/bindings.ts` — id, chord,
scope — and the rest derives. The handler matches with `isBound(e, id)`,
the editor's keymap takes `chordFor(id)` (the chord syntax is
CodeMirror's, so one spelling serves both), and the `?` sheet names ids
rather than spelling chords. `shortcuts.test.ts` fails on a binding no
row documents, which turns "add the row in the same change" from a
convention into something the suite keeps.
What deliberately stays as code: the order the surfaces get their turn,
and the guard beside each branch. A table rich enough to express "⌘G,
but only while the find bar is open" is a `when`-clause language, which
is a worse trade at this size than a guard you can read.
`conflicts()` is the gate — two commands answering to one keystroke in
one scope, run by `npm test` inside `just check` and `just ci`. It knows
that a `Mod-` chord answers to ⌃X as well as ⌘X, so `Ctrl-Tab` and
`Mod-Tab` are seen to collide. Scopes nest (`overlay:link` under
`overlay`), which is what keeps two ⏎ bindings from reading as a clash
while still reporting the shadows that are real: the overlay's Tab trap
takes ⌃Tab from the Settings rail, and only branch order saves it.
`editorkeys.ts` folds in the other keyboard. CodeMirror ships ~100 stock
bindings B2 never wrote, and three of B2's chords work while editing
only because it happens to leave them alone — an assumption that lived
in a parenthesis ("CodeMirror leaves Mod-e unbound, so the event bubbles
here"). It's a pinned list now. main.ts installs the stock keymaps *from*
that module, so the set checked is the set that runs.
Two findings from the fold-in, both pinned rather than changed:
- CodeMirror binds Mod-i (selectParentSyntax). ⌘I stays italic only
because the format chords are installed ahead of the defaults — load-
bearing ordering that nothing noticed before.
- The ⌃-as-⌘ alias puts ten B2 chords on macOS's emacs text bindings
inside the editor: ⌃F moves the caret *and* opens the find bar, ⌃E
goes to end-of-line *and* leaves edit mode. Pre-existing, and it all
retires at once if `Mod` stops accepting ⌃ — worth its own decision.
Behavior deltas, all narrow: chord matching is now strict about ⌥/⇧
where the hand-rolled tests weren't, so ⇧⌘S no longer force-saves and
⌘⏎ no longer commits the link dialog. Escape and Tab keep their
any-modifier leniency explicitly, via an `Any-` chord — the escape hatch
must not depend on a modifier the user hasn't let go of. In the sheet,
⇧⌃Tab now renders ⌃⇧Tab (Apple's HIG modifier order, applied in one
place rather than by hand), and a new row documents ⏎ committing an open
dialog — a chord that was bound and undocumented, which is exactly what
the coverage check is for.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A5LV3uA69fYYJBPes6ubF2
…ndings Every handler read `(e.metaKey || e.ctrlKey)`, so each ⌘ chord answered to ⌃ as well. That's the right reflex on Windows and Linux, where ⌃ is the platform modifier, and the wrong one on the only platform B2 ships on. macOS gives ⌃F/⌃B/⌃N/⌃P/⌃A/⌃E emacs meanings in every text field, and CodeMirror implements the same set so its editor feels native. A CodeMirror binding calls `preventDefault` but never `stopPropagation` (0 of the 73 stock bindings opt in), so the keystroke still reached the document handler and ran a second command. ⌃E moved the caret to end-of-line *and* left edit mode. ⌃F moved it forward a character *and* opened the find bar. Also ⌃⇧F, ⌃N, ⌃⇧N and ⌃⇧A; ⌃←/⌃→ were spared only by nav's unrelated `!state.editing` guard. `Mod` now means ⌘ and nothing else, and every modifier is compared literally — which also converges B2's reading of `Mod` with CodeMirror's (`normalizeKeyName` resolves it to meta on mac). That matters more than it sounds: the two share this chord syntax because `chordFor` hands specs straight to `keymap.of`, so the same word meaning two things was a latent trap, and it was already making editorkeys.ts over-expand CodeMirror's own chords when normalizing them. Falling out of the strictness: a chord now has exactly one keystroke form (`Any-` excepted, which is why forms are still a list), so the collision checker no longer reasons about aliasing; ⌘⌃X becomes representable, so parseChord stops rejecting it; nothing binds it. Two guards, both verified to fail when the alias is put back. In bindings.test.ts, a property over the whole table — a binding may claim a ⌃ keystroke only by asking for ⌃ — so it holds for chords not yet written. In editorkeys.test.ts, the other end: whatever B2 binds, it never meets CodeMirror on a ⌃ keystroke. The eight real overlaps are unchanged; the alias set that check used to pin is now empty, and `isAliasOnly` went with it. Unaffected and left alone: `livepreview.ts`'s ⌘-click-to-follow, which takes ⌃-click too. A mouse gesture rather than a chord, and ⌃-click is macOS's secondary click — the same shape of problem, worth its own look. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A5LV3uA69fYYJBPes6ubF2
…a synonym The live-preview editor's follow-a-link gesture read `(e.metaKey || e.ctrlKey)`, the mouse's version of the keyboard alias dropped in the previous commit. It's worse on this side: on macOS ⌃-click *is* the secondary click, so the OS synthesizes a context-menu gesture from it — ⌃-clicking a wikilink navigated away and right-clicked at once. ⌘ only now. The rule is a named exported function rather than an inline test, because the handler it lives in is buried in a ViewPlugin's `eventHandlers` where the suite can't reach it, and "restore the Ctrl branch for cross-platform symmetry" is a plausible-looking edit somebody will eventually make. bindings.test.ts and editorkeys.test.ts hold this line for the keyboard; neither can see a mouse handler. Two changes were needed to get livepreview.ts into the suite at all, and both are worth having on their own: - The two widget classes used constructor parameter properties, the one TypeScript construct node's `--experimental-strip-types` refuses. That made the entire module unimportable by the test runner. Now plain field declarations. - `./render` is now `./render.ts`. Node resolves specifiers literally; the extensionless form is what the app-only modules use, and this file isn't app-only any more. Verified by putting the Ctrl branch back and watching the new check fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A5LV3uA69fYYJBPes6ubF2
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR centralizes keyboard bindings, derives runtime handlers and shortcut documentation from the registry, validates B2 chords against CodeMirror keymaps, and restricts wikilink following to command-click. ChangesCentralized keyboard bindings
Wikilink follow-click behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant KeyboardEvent
participant main.ts
participant bindings.ts
participant UIAction
KeyboardEvent->>main.ts: keydown event
main.ts->>bindings.ts: isBound(event, bindingId)
bindings.ts-->>main.ts: binding match result
main.ts->>UIAction: dispatch matched action
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/b2-desktop/CLAUDE.md (1)
134-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShell titles still spell chords by hand.
The new rule ("never typed out") is only honoured by the find-close button so far;
buildShell()inui/src/main.tsstill hardcodesBack (⌘[),Forward (⌘]),⇧⌘Fin the search placeholder/title,Settings (⌘,), and the editor bar's⌘E (⌘S flushes anytime). Worth converting those todisplayKeys([...])in this PR (or noting them as follow-up) so the doc isn't ahead of the code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/b2-desktop/CLAUDE.md` around lines 134 - 137, Update buildShell() to remove all hardcoded keyboard chords from the Back, Forward, search, Settings, and editor-bar titles/placeholders, generating each shortcut with displayKeys([id]) using the corresponding action identifiers. Preserve the existing labels and menu/control placement while ensuring every displayed chord is derived from the shared key-display helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui/src/main.ts`:
- Around line 3092-3094: Wrap the displayKeys(["dismiss"]) result in escapeHtml
before interpolating it into the find-close button’s title attribute. Update
only this projected chord interpolation in the surrounding markup and preserve
the existing title text and displayKeys behavior.
---
Nitpick comments:
In `@crates/b2-desktop/CLAUDE.md`:
- Around line 134-137: Update buildShell() to remove all hardcoded keyboard
chords from the Back, Forward, search, Settings, and editor-bar
titles/placeholders, generating each shortcut with displayKeys([id]) using the
corresponding action identifiers. Preserve the existing labels and menu/control
placement while ensuring every displayed chord is derived from the shared
key-display helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ecfd2484-c142-4162-a944-ad17cb564429
📒 Files selected for processing (14)
CLAUDE.mdcrates/b2-desktop/CLAUDE.mdui/src/anomalies.tsui/src/bindings.test.tsui/src/bindings.tsui/src/editorkeys.test.tsui/src/editorkeys.tsui/src/format.test.tsui/src/format.tsui/src/livepreview.test.tsui/src/livepreview.tsui/src/main.tsui/src/shortcuts.test.tsui/src/shortcuts.ts
Review catch (PR #118). `displayKeys(["dismiss"])` was interpolated straight into the shell's `innerHTML`. The value is registry-derived and reads "Esc" today, so nothing is wrong on the screen — but `displayChord` renders whatever key the chord names, and `Mod-<` parses perfectly well, so a future binding over an HTML metacharacter would land in markup raw. The repo's rule is that every value B2 itself interpolates goes through `escapeHtml` (invariant E5) precisely so nobody has to re-derive a value's provenance at each call site. This was the one site in the change that skipped it: the anomaly badge sets `.title` as a DOM property, the toast writes `textContent`, and the `?` sheet already escapes both columns in `shortcutsGridHtml`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A5LV3uA69fYYJBPes6ubF2
This was referenced Jul 28, 2026
This was referenced Aug 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A chord used to be spelled twice — a modifier test in main.ts's keydown
handler and a row of display text in shortcuts.ts — with nothing but
discipline keeping them equal. K1 promises every action has a keyboard
path and that the path is findable, so a sheet free to fall behind the
wiring is a promise waiting to break. And because main.ts can't be
imported by node, none of it was testable: the app's whole keyboard
contract sat in the one file the suite can't see.
The chord is now declared once, in
ui/src/bindings.ts— id, chord,scope — and the rest derives. The handler matches with
isBound(e, id),the editor's keymap takes
chordFor(id)(the chord syntax isCodeMirror's, so one spelling serves both), and the
?sheet names idsrather than spelling chords.
shortcuts.test.tsfails on a binding norow documents, which turns "add the row in the same change" from a
convention into something the suite keeps.
What deliberately stays as code: the order the surfaces get their turn,
and the guard beside each branch. A table rich enough to express "⌘G,
but only while the find bar is open" is a
when-clause language, whichis a worse trade at this size than a guard you can read.
conflicts()is the gate — two commands answering to one keystroke inone scope, run by
npm testinsidejust checkandjust ci. It knowsthat a
Mod-chord answers to ⌃X as well as ⌘X, soCtrl-TabandMod-Tabare seen to collide. Scopes nest (overlay:linkunderoverlay), which is what keeps two ⏎ bindings from reading as a clashwhile still reporting the shadows that are real: the overlay's Tab trap
takes ⌃Tab from the Settings rail, and only branch order saves it.
editorkeys.tsfolds in the other keyboard. CodeMirror ships ~100 stockbindings B2 never wrote, and three of B2's chords work while editing
only because it happens to leave them alone — an assumption that lived
in a parenthesis ("CodeMirror leaves Mod-e unbound, so the event bubbles
here"). It's a pinned list now. main.ts installs the stock keymaps from
that module, so the set checked is the set that runs.
Two findings from the fold-in, both pinned rather than changed:
because the format chords are installed ahead of the defaults — load-
bearing ordering that nothing noticed before.
inside the editor: ⌃F moves the caret and opens the find bar, ⌃E
goes to end-of-line and leaves edit mode. Pre-existing, and it all
retires at once if
Modstops accepting ⌃ — worth its own decision.Behavior deltas, all narrow: chord matching is now strict about ⌥/⇧
where the hand-rolled tests weren't, so ⇧⌘S no longer force-saves and
⌘⏎ no longer commits the link dialog. Escape and Tab keep their
any-modifier leniency explicitly, via an
Any-chord — the escape hatchmust not depend on a modifier the user hasn't let go of. In the sheet,
⇧⌃Tab now renders ⌃⇧Tab (Apple's HIG modifier order, applied in one
place rather than by hand), and a new row documents ⏎ committing an open
dialog — a chord that was bound and undocumented, which is exactly what
the coverage check is for.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01A5LV3uA69fYYJBPes6ubF2
Summary by CodeRabbit