refactor(web): central keyboard-shortcut registry (groundwork for #43) - #106
Merged
Conversation
Introduce web/src/shared/keybindings.js: a single source of truth for the remappable global/navigation/composer shortcuts, plus matcher helpers. Migrate the five inline handlers (keyboard-nav, session-globals, SessionsPage index, search-filters, textarea-controls) to ask the registry `matchesAction(id, e)` instead of hardcoding `e.key === …` / modifier checks. Pure refactor — defaults are unchanged and no override loading exists yet, so behavior is identical. All 722 frontend tests pass with no changes to the existing handler tests, confirming parity. Structural modal keys (Escape, arrows, Tab focus-traps), Enter-to-submit, and the multi-key `g g` sequence are intentionally left hardcoded; they are UI affordances, not preferences. Groundwork for #43 (customizable shortcuts). The settings UI, override persistence, conflict detection, and modal reflection land in a follow-up PR stacked on this one.
On layouts where '/' is a shifted key (German, French, ...), Cmd+/ arrives with shiftKey=true and the strict modifier check made the shortcuts-help binding unreachable. Shift is now only enforced where it changes meaning: letters and named keys like Tab. For punctuation, event.key is already the shifted result, so an unrequested Shift is layout noise.
daveyijzermans
pushed a commit
to daveyijzermans/pi-web
that referenced
this pull request
Aug 23, 2026
Failure A (frontend-lint):
- SessionsList.svelte: remove dead `{@const archOpen}` copy-paste
leftover in the archived-projects block (no `{#if archOpen}` or
toggle consumes it there; the active block's copy is untouched).
- SessionsPage.svelte: replace the mutable `Set` used for the
reactive `archivedProjects` with a `SvelteSet` (already imported),
dropping the redundant `$state` wrapper and mutating in place so
reassignment-based reactivity is no longer relied on. Archiving and
localStorage/server-settings persistence behave identically.
Failure B (E2E ENOENT on ubuntu):
- Makefile: make `BINARY` default OS-aware — `pi-web.exe` on Windows,
`pi-web` elsewhere — so `make build` on Linux emits the name the
Playwright harness (e2e/lib/paths.ts) spawns. Windows dev workflow
and `make install` are unchanged.
Closes ygncode#106
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
1 participant
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.
Summary
PR 1 of 2 toward #43 (customizable keyboard shortcuts). This is a pure refactor with no behavior change — it introduces a central registry for the remappable shortcuts and migrates the scattered inline handlers to consult it. No user-facing change yet.
Turns out the shortcuts were not all in
keyboard-nav.jsas the issue assumed — they were spread across five handler files. This PR unifies them behind one registry so PR 2 can add the settings UI and per-user overrides without touching each handler again.What changed
web/src/shared/keybindings.js— theKEY_ACTIONSregistry (id, category, default combo) plus matcher helpers (matchesAction,comboMatchesEvent,parseCombo,expectedEventKey). Combo grammar:mod= ⌘/Ctrl,ctrl= Ctrl-only, plusshift/alt.matchesAction(id, e):shared/keyboard-nav.js— settings,j/k,⇧G,⇧Isession/session-globals.js—⌘K ⌘B ⌘T ⌘⇧L ⌘⇧N ⌘/routes/SessionsPage.svelte— index⌘⇧L,⌘Ksession/ui/search-filters.js—t/o/pcomponents/session/chat/textarea-controls.js—⇧Tab,⌃I/⌃LScope (deliberate)
Remappable = global/navigation/composer only. Left hardcoded: structural modal keys (Escape-close, arrow-nav, Tab focus-trap), composer Enter-to-submit, and the multi-key
g gsequence — these are UI affordances, not preferences.Fidelity
The nav keys match the literal produced key char (
e.key === 'G') exactly as before, so Caps Lock and every prior path behave identically. The three plain toggles keep their key-only, modifier-agnostic matching. All 722 frontend tests pass with zero changes to the existing handler tests — the parity check for "no behavior change."Testing
New
keybindings.test.js(matcher semantics, aliases, overrides, registry integrity).make buildgreen (incl. export bundle).Next (PR 2)
Settings UI section, override persistence via the existing settings store + a new
settingDefaultskey, conflict detection, ShortcutsModal reading the registry (also fixes the current cheatsheet drift), Playwright E2E + screenshots, docs.Follow-up commit: layout-tolerant punctuation matching
A second commit (
9dc6a12) relaxes the matcher for punctuation combos: Shift absence is only enforced where Shift changes meaning (letters, named keys like Tab). On layouts where/is a shifted key (German, French, …),⌘/arrives withshiftKey=trueand the strict check made the shortcuts-help binding unreachable.event.keyis already the shifted result for punctuation, so an unrequested Shift there is layout noise — while⌘⇧K-style rejection still holds for letters. One existing keyboard-nav test (Cmd+Shift+,) was updated to encode the new semantics:'<'(US layout) still doesn't navigate;','with Shift (shifted-comma layouts) does.