ui: make the desktop app keyboard-complete (invariant K1, GH #78) - #89
Merged
Conversation
K1 promises B2 is fully operable from the keyboard, with the mouse an accelerator rather than a requirement. The GUI didn't keep it: the file tree had no arrow navigation (Tab-cycling 1500 rows was the only way in), graph nodes weren't focusable at all, Rename / Move… / Link… existed only behind a right-click, overlays neither took nor returned focus, and the chords that did exist lived only in button tooltips. The file tree now follows the ARIA `tree` pattern. Its shape *and its row order* move into `ui/src/treenav.ts`, pure and tested, because the paint and the arrow keys have to agree on that order down to the last tie-break — a tree you can arrow through in a different order than you can see is worse than no arrows at all. `render.ts` sorts through the same helpers. Rows get `role="treeitem"`, `aria-level`, and a roving `tabindex` (one Tab stop for the whole tree); ↑↓ walk visible rows, →← expand/enter and collapse/exit, Home/End jump, and a bare letter is first-letter typeahead. A delete hands focus to the neighbouring row rather than dropping it. Overlay focus is one hook — `syncOverlayFocus()`, called at the end of every `render()` and acting only on the open/close edge, so a toast timer's repaint can't steal focus mid-Tab. Opening takes focus, Tab is trapped while up, and closing restores it. Restoration is by identity that outlives an innerHTML swap (path → id → element), which is also how `paintTree` keeps a keyboard user in the tree across an unrelated repaint. New keyboard paths: ⇧F10 / Menu opens the context menu on the focused tree row or discovery card (the only route to Rename / Move… / Link…), F2 renames, ⌘⌫ now deletes the focused row (so folders get a chord, with the confirm), ⌘1/⌘2/⌘3 put the keyboard in a pane, ⏎/Space activate a graph node by dispatching the same click the mouse sends, ⏎ commits the link modal, and ⏎ resets a pane gutter (the double-click's missing sibling). Discoverability: `?` opens a shortcut sheet built from one table (`ui/src/shortcuts.ts`), also reachable from Settings; menu items carry their direct chord; the search box, gutters, graph toggle, and modals say what keys they answer to. Focus rings are a single `:focus-visible` rule. Verified in Chromium against the built bundle with a stubbed IPC: 32 keyboard paths, including focus restoration across repaints. Also fixes a pre-existing `clippy::type_complexity` failure in `link.rs`'s test table, tripped by Rust 1.96.1 and unrelated to this work — it was failing the gate on any branch. Refs: docs/design/invariants.md K1, crates/b2-desktop/CLAUDE.md Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BF2Fx3uMZjJ18oHCp5mebY
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe desktop UI gains keyboard-complete tree navigation, roving focus, overlay focus trapping, a shortcut reference modal, accessible graph controls, pane reset behavior, and related tests and documentation. ChangesKeyboard-complete desktop UI
Core link test maintenance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Keyboard
participant main.ts
participant treenav.ts
participant render.ts
Keyboard->>main.ts: Press tree navigation key
main.ts->>treenav.ts: Resolve movement or typeahead target
main.ts->>render.ts: Update focus and tree state
render.ts->>Keyboard: Expose focused ARIA tree row
Possibly related issues
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: 3
🧹 Nitpick comments (3)
ui/src/render.ts (1)
1086-1093: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
chordis interpolated unescaped.Every current caller passes a literal, so this is not exploitable today, but
label/chordsit next toescapeHtml-guarded interpolations everywhere else in this file; escaping both keeps the helper safe if a path- or title-derived string is ever passed.🤖 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 `@ui/src/render.ts` around lines 1086 - 1093, Update contextItemHtml so both label and chord are HTML-escaped before interpolation into the button markup, while preserving the existing conditional chord hint and danger styling behavior. Reuse the file’s existing escapeHtml helper.ui/src/main.ts (1)
3061-3096: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
treeRows()rebuilds the whole tree on every keystroke.
buildTree+visibleRowsrun per arrow press and per typeahead character (twice on a typeahead miss path is avoided, but the build is still O(notes + resources) with a sort per folder). Fine at today's vault sizes; if the tree grows, memoize the row list per(notes, resources, dirs, expandedDirs)identity rather than recomputing inside the handler.🤖 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 `@ui/src/main.ts` around lines 3061 - 3096, Memoize the result of treeRows() using the identities of notes, resources, dirs, and expandedDirs as the cache key, so unchanged tree state reuses the existing row list. Update the tree keydown handler to use that cached result for arrow navigation and typeahead without rebuilding the tree on each keystroke. Invalidate or replace the cache whenever any of those inputs changes.ui/style.css (1)
2328-2332: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the ignored
border-radiusfrom the SVG shape focus ring.
.gshapeis rendered as an SVG<circle>/<rect>, so this declaration cannot affect the shape curvature and only misleads future readers.🤖 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 `@ui/style.css` around lines 2328 - 2332, Remove the border-radius declaration from the .gnode:focus-visible .gshape rule, leaving the outline and outline-offset declarations unchanged.
🤖 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 600-622: Update syncOverlayFocus so each overlay-kind transition
captures the current trigger element, rather than only capturing on the
null-to-overlay transition. When the overlay closes or returns to a previous
kind, restore the focus target associated with the immediately underlying
overlay while preserving the existing inline-input exception and link-modal
preferred focus behavior.
- Around line 3190-3218: Update the Tab-handling branch in the overlay keyboard
handler to call preventDefault whenever currentOverlay() is non-null, including
when overlayFocusables() returns no items. Preserve the existing focus-wrapping
behavior when focusable controls exist, while swallowing Tab for empty overlays
before returning.
In `@ui/src/render.ts`:
- Around line 159-169: Update treeRenameRowHtml and treeCreateRowHtml so their
inline editor wrapper uses role="none" while preserving the existing labelled
input and row behavior. Ensure the role="tree" container exposes only valid
tree-pattern children during inline create or rename operations.
---
Nitpick comments:
In `@ui/src/main.ts`:
- Around line 3061-3096: Memoize the result of treeRows() using the identities
of notes, resources, dirs, and expandedDirs as the cache key, so unchanged tree
state reuses the existing row list. Update the tree keydown handler to use that
cached result for arrow navigation and typeahead without rebuilding the tree on
each keystroke. Invalidate or replace the cache whenever any of those inputs
changes.
In `@ui/src/render.ts`:
- Around line 1086-1093: Update contextItemHtml so both label and chord are
HTML-escaped before interpolation into the button markup, while preserving the
existing conditional chord hint and danger styling behavior. Reuse the file’s
existing escapeHtml helper.
In `@ui/style.css`:
- Around line 2328-2332: Remove the border-radius declaration from the
.gnode:focus-visible .gshape rule, leaving the outline and outline-offset
declarations unchanged.
🪄 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: bcef0339-dfec-4e7c-b4d2-1e4473915b73
📒 Files selected for processing (12)
CLAUDE.mdcrates/b2-core/src/link.rscrates/b2-desktop/CLAUDE.mdui/src/main.tsui/src/panes.tsui/src/render.tsui/src/shortcuts.test.tsui/src/shortcuts.tsui/src/state.tsui/src/treenav.test.tsui/src/treenav.tsui/style.css
… review) Three findings from review, all valid. The first turned out to be a deeper bug than reported. **Overlay focus return.** Closing the `?` sheet over Settings was supposed to land on the "Keyboard shortcuts" button that opened it, and didn't. The reported cause — `overlayReturn` captured only on the null→overlay edge — was real but not the whole story: `captureReturnFocus` read `document.activeElement` from inside `syncOverlayFocus`, which runs at the *end* of `render()`, by which point `render()` has already swapped `#modal-root`/`#menu-root` wholesale. The element that held focus is destroyed and `activeElement` has fallen back to `<body>`, so the capture returned null for *every* trigger living inside an overlay. Tree rows looked fine only because `paintTree` self-heals by path independently. So: track `lastFocused` continuously from a `focusin` listener (destroying a focused node fires no `focusin`, so the last value is still the trigger), and give the Settings button an id, since a thunk can only re-find a control across the repaint by id. Focus bookkeeping also splits into the two layers it always was — the sheet *stacks* over Settings while everything else is mutually exclusive, so the sheet gets its own return target while the base layer keeps capturing at the start of a chain (menu → Move… → close still lands on the tree row). **Tab trap.** With no focusable controls the handler returned without `preventDefault()`, letting Tab walk the page behind the backdrop — precisely the failure the block exists to prevent. Swallowed unconditionally now. **Tree ARIA.** The inline create/rename rows are plain `<div>`s directly inside the `role="tree"` container, so the tree owned a child of no known role — while a screen-reader user is naming a note. `role="none"` on the wrapper; the input keeps its own label. Also guards `?` behind an open context menu, an edge the layering made visible: a menu is transient and owns the keyboard like any other overlay. Verified in Chromium: both sheet-from-Settings paths (mouse and keyboard) now restore to the trigger, and the existing 32 keyboard paths still pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BF2Fx3uMZjJ18oHCp5mebY
This was referenced Jul 26, 2026
Desktop focus durability: the note pane (and the side pane's chrome) drop focus across a repaint
#91
Closed
This was referenced Jul 26, 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.
K1 promises B2 is fully operable from the keyboard, with the mouse an
accelerator rather than a requirement. The GUI didn't keep it: the file
tree had no arrow navigation (Tab-cycling 1500 rows was the only way in),
graph nodes weren't focusable at all, Rename / Move… / Link… existed only
behind a right-click, overlays neither took nor returned focus, and the
chords that did exist lived only in button tooltips.
The file tree now follows the ARIA
treepattern. Its shape and its roworder move into
ui/src/treenav.ts, pure and tested, because the paintand the arrow keys have to agree on that order down to the last tie-break
— a tree you can arrow through in a different order than you can see is
worse than no arrows at all.
render.tssorts through the same helpers.Rows get
role="treeitem",aria-level, and a rovingtabindex(one Tabstop for the whole tree); ↑↓ walk visible rows, →← expand/enter and
collapse/exit, Home/End jump, and a bare letter is first-letter typeahead.
A delete hands focus to the neighbouring row rather than dropping it.
Overlay focus is one hook —
syncOverlayFocus(), called at the end ofevery
render()and acting only on the open/close edge, so a toast timer'srepaint can't steal focus mid-Tab. Opening takes focus, Tab is trapped
while up, and closing restores it. Restoration is by identity that outlives
an innerHTML swap (path → id → element), which is also how
paintTreekeeps a keyboard user in the tree across an unrelated repaint.
New keyboard paths: ⇧F10 / Menu opens the context menu on the focused tree
row or discovery card (the only route to Rename / Move… / Link…), F2
renames, ⌘⌫ now deletes the focused row (so folders get a chord, with the
confirm), ⌘1/⌘2/⌘3 put the keyboard in a pane, ⏎/Space activate a graph
node by dispatching the same click the mouse sends, ⏎ commits the link
modal, and ⏎ resets a pane gutter (the double-click's missing sibling).
Discoverability:
?opens a shortcut sheet built from one table(
ui/src/shortcuts.ts), also reachable from Settings; menu items carrytheir direct chord; the search box, gutters, graph toggle, and modals say
what keys they answer to. Focus rings are a single
:focus-visiblerule.Verified in Chromium against the built bundle with a stubbed IPC: 32
keyboard paths, including focus restoration across repaints.
Also fixes a pre-existing
clippy::type_complexityfailure inlink.rs's test table, tripped by Rust 1.96.1 and unrelated to this work— it was failing the gate on any branch.
Refs: docs/design/invariants.md K1, crates/b2-desktop/CLAUDE.md
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01BF2Fx3uMZjJ18oHCp5mebY
Summary by CodeRabbit
?and a Settings button), plus keyboard chord hints in menus.