From 5d765d1127961a6fd6acd67f465c36663a222208 Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Fri, 8 May 2026 16:36:04 -0500 Subject: [PATCH 01/10] feat(keymap): add user-configurable keybindings Introduce a lazygit-style keybinding remap system so users can override any default key via `[keybindings.]` in their global or repo config. Scopes are `global`, `pager`, `menu`, and `filter`; each action in the new registry can be bound to a single key, an array of keys, or the `` sentinel to unbind a default. Token grammar mirrors lazygit: bare printable characters (`q`, `?`), named special keys (``, ``, ``), and modifier-prefixed forms (``, ``, stackable as ``). The matcher is modifier-permissive for sequence specs, so order of more-specific shift bindings before bare equivalents matters in the dispatch table (documented at the matcher and pinned by tests). The help dialog and `docs/keybindings.md` are derived from the same action registry, so adding an action surfaces it in both places. --- CHANGELOG.md | 2 + README.md | 4 + docs/keybindings.md | 141 +++++++ src/core/config.test.ts | 36 ++ src/core/config.ts | 23 +- src/core/keyboard.ts | 6 + src/core/keymap/actions.ts | 448 +++++++++++++++++++++++ src/core/keymap/format.test.ts | 41 +++ src/core/keymap/format.ts | 59 +++ src/core/keymap/load.test.ts | 103 ++++++ src/core/keymap/load.ts | 110 ++++++ src/core/keymap/match.test.ts | 144 ++++++++ src/core/keymap/match.ts | 92 +++++ src/core/keymap/parse.test.ts | 91 +++++ src/core/keymap/parse.ts | 183 +++++++++ src/core/types.ts | 3 + src/ui/App.tsx | 10 + src/ui/components/chrome/HelpDialog.tsx | 130 ++++--- src/ui/components/chrome/StatusBar.tsx | 2 +- src/ui/components/ui-components.test.tsx | 54 ++- src/ui/hooks/useAppKeyboardShortcuts.ts | 164 +++++---- src/ui/lib/keyboard.ts | 50 --- src/ui/lib/ui-lib.test.ts | 46 +-- 23 files changed, 1711 insertions(+), 231 deletions(-) create mode 100644 docs/keybindings.md create mode 100644 src/core/keyboard.ts create mode 100644 src/core/keymap/actions.ts create mode 100644 src/core/keymap/format.test.ts create mode 100644 src/core/keymap/format.ts create mode 100644 src/core/keymap/load.test.ts create mode 100644 src/core/keymap/load.ts create mode 100644 src/core/keymap/match.test.ts create mode 100644 src/core/keymap/match.ts create mode 100644 src/core/keymap/parse.test.ts create mode 100644 src/core/keymap/parse.ts delete mode 100644 src/ui/lib/keyboard.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index f245293d..c4354897 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ All notable user-visible changes to Hunk are documented in this file. ### Added +- Configurable keybindings via the new `[keybindings.]` config sections (global, pager, menu, filter). Use `` to unbind a default. See `docs/keybindings.md`. + ### Changed - Auto-detect Jujutsu checkouts for `hunk diff` and `hunk show`, while keeping explicit `vcs` config overrides. diff --git a/README.md b/README.md index 981858e7..3f9fb92b 100644 --- a/README.md +++ b/README.md @@ -122,6 +122,10 @@ agent_notes = false `exclude_untracked` affects Git working-tree `hunk diff` sessions only. +Keybindings are configurable through the same files; see +[`docs/keybindings.md`](docs/keybindings.md) for the action reference and +token syntax. + ### Git integration Set Hunk as your Git pager so `git diff` and `git show` open in Hunk automatically: diff --git a/docs/keybindings.md b/docs/keybindings.md new file mode 100644 index 00000000..bd3f905b --- /dev/null +++ b/docs/keybindings.md @@ -0,0 +1,141 @@ +# Keybindings + +Hunk reads keybinding overrides from the same config files as every other view +preference: the global `~/.config/hunk/config.toml` and the repo-local +`.hunk/config.toml`. Repo settings take precedence over global ones, and CLI +flags do not affect the keymap. + +Bindings are organized into four scopes that mirror the modes the app can be +in at any moment: + +| Scope | Active when | +| -------- | ------------------------------------------------- | +| `global` | The main review UI is focused. | +| `pager` | Hunk is invoked in pager mode (`hunk pager`). | +| `menu` | A menu (View, Theme, Agent, ...) is open. | +| `filter` | The file-filter input is focused. | + +Each scope is configured under its own TOML section, e.g. +`[keybindings.global]`. Missing keys keep their defaults; v1 has no "clear all" +switch. + +## Token syntax + +Bindings accept either a single string or an array of strings: + +```toml +[keybindings.global] +quit = "q" +"scroll.pageDown" = ["", "f", ""] +``` + +Tokens use angle brackets for special keys and modifiers, mirroring lazygit: + +| Token | Meaning | +| ---------------- | ---------------------------------------------------- | +| `q`, `?`, `[` | Bare printable characters (literal sequence match). | +| `` | Escape key. | +| `` | Tab key. | +| `` | Spacebar. | +| `` / `` | Enter (both spellings are equivalent). | +| `` | Backspace. | +| `` / `` / `` / `` | Arrow keys. | +| `` / `` | Home and End. | +| `` / `` | Page Up / Page Down. | +| ``–`` | Function keys. | +| `` | Ctrl+C. | +| `` | Shift+Up. | +| ``, `` | Alt+X / Meta+X. | +| `` | Modifiers stack in any order. | +| `` | Sentinel that unbinds the action. | + +Multi-key sequences like vim's `gg` are not supported in v1 — every binding is +a single chord. + +To unbind a default: + +```toml +[keybindings.global] +"sidebar.toggle" = "" +``` + +Unknown action ids and malformed tokens are logged once to stderr and +otherwise ignored. They never abort startup. + +## Action reference + +### `[keybindings.global]` + +| Action | Default keys | What it does | +| -------------------- | ------------------------- | ----------------------------- | +| `scroll.lineDown` | `j`, `` | Move line-by-line (down). | +| `scroll.lineUp` | `k`, `` | Move line-by-line (up). | +| `scroll.pageDown` | ``, `f`, `` | Page down. | +| `scroll.pageUp` | `b`, ``, `` | Page up. | +| `scroll.halfPageDown` | `d` | Half page down. | +| `scroll.halfPageUp` | `u` | Half page up. | +| `scroll.toTop` | `` | Jump to top of stream. | +| `scroll.toBottom` | `` | Jump to bottom of stream. | +| `scroll.codeLeft` | `` | Scroll code left one column. | +| `scroll.codeRight` | `` | Scroll code right one column. | +| `scroll.codeLeftFast` | `` | Scroll code left (fast). | +| `scroll.codeRightFast` | `` | Scroll code right (fast). | +| `hunk.prev` | `[` | Previous hunk. | +| `hunk.next` | `]` | Next hunk. | +| `annotatedHunk.prev` | `{` | Previous comment hunk. | +| `annotatedHunk.next` | `}` | Next comment hunk. | +| `layout.split` | `1` | Split layout. | +| `layout.stack` | `2` | Stack layout. | +| `layout.auto` | `0` | Auto layout. | +| `sidebar.toggle` | `s` | Toggle sidebar. | +| `theme.cycle` | `t` | Cycle theme. | +| `agentNotes.toggle` | `a` | Toggle agent notes. | +| `lineNumbers.toggle` | `l` | Toggle line numbers. | +| `wrap.toggle` | `w` | Toggle line wrap. | +| `hunkHeaders.toggle` | `m` | Toggle hunk metadata headers. | +| `quit` | `q`, `` | Quit. | +| `help.toggle` | `?` | Toggle help. | +| `filter.focus` | `/` | Focus the file filter. | +| `focus.toggle` | `` | Toggle files/filter focus. | +| `reload` | `r` | Reload current input. | +| `menu.open` | `` | Open the menus. | + +### `[keybindings.pager]` + +The pager scope mirrors the global scroll/wrap actions for `hunk pager`. +`quit` defaults to `q` and ``. Defaults are the same as the matching +global actions; rebind them under this section to override pager-only. + +### `[keybindings.menu]` + +| Action | Default keys | What it does | +| --------------- | ------------------------ | --------------------------- | +| `menu.close` | `` | Close the menu. | +| `menu.prev` | `` | Previous menu. | +| `menu.next` | ``, `` | Next menu. | +| `menu.itemUp` | `` | Previous item. | +| `menu.itemDown` | `` | Next item. | +| `menu.activate` | ``, `` | Activate current item. | + +### `[keybindings.filter]` + +| Action | Default keys | What it does | +| -------------- | ------------ | -------------------------- | +| `focus.toggle` | `` | Leave the filter input. | + +Esc and Enter inside the filter input are owned by the input element itself +(Esc clears, then closes; Enter submits) and are not configurable in v1. + +## Example + +```toml +# ~/.config/hunk/config.toml + +[keybindings.global] +quit = ["q", ""] +"sidebar.toggle" = "" +"theme.cycle" = "" + +[keybindings.pager] +"wrap.toggle" = "" +``` diff --git a/src/core/config.test.ts b/src/core/config.test.ts index f1e28080..b24d5394 100644 --- a/src/core/config.test.ts +++ b/src/core/config.test.ts @@ -305,4 +305,40 @@ describe("config resolution", () => { expect(bootstrap.initialTheme).toBe("graphite"); }); + + test("repo keybindings.global override surfaces on the resolved keymap", () => { + const home = createTempDir("hunk-config-home-"); + const repo = createTempDir("hunk-config-repo-"); + createRepo(repo); + + mkdirSync(join(repo, ".hunk"), { recursive: true }); + writeFileSync( + join(repo, ".hunk", "config.toml"), + [ + "[keybindings.global]", + 'quit = ["", "x"]', + '"sidebar.toggle" = ""', + "", + ].join("\n"), + ); + + const resolved = resolveConfiguredCliInput( + { + kind: "patch", + file: "-", + options: { pager: false }, + }, + { cwd: repo, env: { HOME: home } }, + ); + const keymap = resolved.input.options.keymap; + expect(keymap).toBeDefined(); + if (!keymap) return; + + const quit = keymap.global.quit ?? []; + expect(quit).toHaveLength(2); + // disabled action should be present but empty. + expect(keymap.global["sidebar.toggle"]).toEqual([]); + // unrelated defaults still present. + expect(keymap.global["help.toggle"]?.length ?? 0).toBeGreaterThan(0); + }); }); diff --git a/src/core/config.ts b/src/core/config.ts index 21f12fd3..60c0001a 100644 --- a/src/core/config.ts +++ b/src/core/config.ts @@ -1,5 +1,7 @@ import fs from "node:fs"; import { dirname, join, resolve } from "node:path"; +import { applyKeymapOverrides, loadKeymapDefaults } from "./keymap/load"; +import type { Keymap } from "./keymap/match"; import { resolveGlobalConfigPath } from "./paths"; import type { CliInput, @@ -28,7 +30,7 @@ interface HunkConfigResolution { repoConfigPath?: string; } -function isRecord(value: unknown): value is Record { +export function isRecord(value: unknown): value is Record { return typeof value === "object" && value !== null && !Array.isArray(value); } @@ -81,6 +83,7 @@ function mergeOptions(base: CommonOptions, overrides: CommonOptions): CommonOpti wrapLines: overrides.wrapLines ?? base.wrapLines, hunkHeaders: overrides.hunkHeaders ?? base.hunkHeaders, agentNotes: overrides.agentNotes ?? base.agentNotes, + keymap: overrides.keymap ?? base.keymap, }; } @@ -167,18 +170,27 @@ export function resolveConfiguredCliInput( agentNotes: DEFAULT_VIEW_PREFERENCES.showAgentNotes, }; - if (userConfigPath) { + // Keymap is layered separately from view options. It only honors the + // top-level `[keybindings.]` blocks — not command-section overrides — + // because a per-command keymap would be confusing and is unnecessary for v1. + let keymap: Keymap = loadKeymapDefaults(); + const userTomlRoot = userConfigPath ? readTomlRecord(userConfigPath) : undefined; + const repoTomlRoot = repoConfigPath ? readTomlRecord(repoConfigPath) : undefined; + + if (userConfigPath && userTomlRoot) { resolvedOptions = mergeOptions( resolvedOptions, - resolveConfigLayer(readTomlRecord(userConfigPath), input), + resolveConfigLayer(userTomlRoot, input), ); + keymap = applyKeymapOverrides(keymap, userTomlRoot); } - if (repoConfigPath) { + if (repoConfigPath && repoTomlRoot) { resolvedOptions = mergeOptions( resolvedOptions, - resolveConfigLayer(readTomlRecord(repoConfigPath), input), + resolveConfigLayer(repoTomlRoot, input), ); + keymap = applyKeymapOverrides(keymap, repoTomlRoot); } resolvedOptions = mergeOptions(resolvedOptions, input.options); @@ -194,6 +206,7 @@ export function resolveConfiguredCliInput( wrapLines: resolvedOptions.wrapLines ?? DEFAULT_VIEW_PREFERENCES.wrapLines, hunkHeaders: resolvedOptions.hunkHeaders ?? DEFAULT_VIEW_PREFERENCES.showHunkHeaders, agentNotes: resolvedOptions.agentNotes ?? DEFAULT_VIEW_PREFERENCES.showAgentNotes, + keymap, }; return { diff --git a/src/core/keyboard.ts b/src/core/keyboard.ts new file mode 100644 index 00000000..6fc1ed67 --- /dev/null +++ b/src/core/keyboard.ts @@ -0,0 +1,6 @@ +import type { KeyEvent } from "@opentui/core"; + +/** Normalize the escape key aliases emitted by different terminal input paths. */ +export function isEscapeKey(key: KeyEvent) { + return key.name === "escape" || key.name === "esc"; +} diff --git a/src/core/keymap/actions.ts b/src/core/keymap/actions.ts new file mode 100644 index 00000000..739df95a --- /dev/null +++ b/src/core/keymap/actions.ts @@ -0,0 +1,448 @@ +export type ActionScope = "global" | "pager" | "menu" | "filter"; + +export type ActionId = + // global + | "quit" + | "help.toggle" + | "filter.focus" + | "focus.toggle" + | "scroll.pageDown" + | "scroll.pageUp" + | "scroll.halfPageDown" + | "scroll.halfPageUp" + | "scroll.lineDown" + | "scroll.lineUp" + | "scroll.toTop" + | "scroll.toBottom" + | "scroll.codeLeft" + | "scroll.codeRight" + | "scroll.codeLeftFast" + | "scroll.codeRightFast" + | "layout.split" + | "layout.stack" + | "layout.auto" + | "sidebar.toggle" + | "reload" + | "theme.cycle" + | "agentNotes.toggle" + | "lineNumbers.toggle" + | "wrap.toggle" + | "hunkHeaders.toggle" + | "hunk.prev" + | "hunk.next" + | "annotatedHunk.prev" + | "annotatedHunk.next" + | "menu.open" + // menu + | "menu.close" + | "menu.prev" + | "menu.next" + | "menu.itemUp" + | "menu.itemDown" + | "menu.activate"; + +export interface ActionDef { + id: ActionId; + scope: ActionScope; + defaultKeys: string[]; + description: string; + group: string; +} + +/** + * Full action registry. Order within the array drives help-dialog grouping + * order; `group` drives section labels. + */ +export const ACTIONS: readonly ActionDef[] = [ + // ---------- global: navigation ---------- + { + id: "scroll.lineDown", + scope: "global", + defaultKeys: ["j", ""], + description: "move line-by-line (down)", + group: "Navigation", + }, + { + id: "scroll.lineUp", + scope: "global", + defaultKeys: ["k", ""], + description: "move line-by-line (up)", + group: "Navigation", + }, + { + id: "scroll.pageDown", + scope: "global", + defaultKeys: ["", "f", ""], + description: "page down", + group: "Navigation", + }, + { + id: "scroll.pageUp", + scope: "global", + defaultKeys: ["b", "", ""], + description: "page up", + group: "Navigation", + }, + { + id: "scroll.halfPageDown", + scope: "global", + defaultKeys: ["d"], + description: "half page down", + group: "Navigation", + }, + { + id: "scroll.halfPageUp", + scope: "global", + defaultKeys: ["u"], + description: "half page up", + group: "Navigation", + }, + { + id: "scroll.toTop", + scope: "global", + defaultKeys: [""], + description: "jump to top", + group: "Navigation", + }, + { + id: "scroll.toBottom", + scope: "global", + defaultKeys: [""], + description: "jump to bottom", + group: "Navigation", + }, + { + id: "scroll.codeLeft", + scope: "global", + defaultKeys: [""], + description: "scroll code left", + group: "Navigation", + }, + { + id: "scroll.codeRight", + scope: "global", + defaultKeys: [""], + description: "scroll code right", + group: "Navigation", + }, + { + id: "scroll.codeLeftFast", + scope: "global", + defaultKeys: [""], + description: "scroll code left (fast)", + group: "Navigation", + }, + { + id: "scroll.codeRightFast", + scope: "global", + defaultKeys: [""], + description: "scroll code right (fast)", + group: "Navigation", + }, + { + id: "hunk.prev", + scope: "global", + defaultKeys: ["["], + description: "previous hunk", + group: "Review", + }, + { + id: "hunk.next", + scope: "global", + defaultKeys: ["]"], + description: "next hunk", + group: "Review", + }, + { + id: "annotatedHunk.prev", + scope: "global", + defaultKeys: ["{"], + description: "previous comment", + group: "Review", + }, + { + id: "annotatedHunk.next", + scope: "global", + defaultKeys: ["}"], + description: "next comment", + group: "Review", + }, + // ---------- global: view ---------- + { + id: "layout.split", + scope: "global", + defaultKeys: ["1"], + description: "split layout", + group: "View", + }, + { + id: "layout.stack", + scope: "global", + defaultKeys: ["2"], + description: "stack layout", + group: "View", + }, + { + id: "layout.auto", + scope: "global", + defaultKeys: ["0"], + description: "auto layout", + group: "View", + }, + { + id: "sidebar.toggle", + scope: "global", + defaultKeys: ["s"], + description: "toggle sidebar", + group: "View", + }, + { + id: "theme.cycle", + scope: "global", + defaultKeys: ["t"], + description: "cycle theme", + group: "View", + }, + { + id: "agentNotes.toggle", + scope: "global", + defaultKeys: ["a"], + description: "toggle agent notes", + group: "View", + }, + { + id: "lineNumbers.toggle", + scope: "global", + defaultKeys: ["l"], + description: "toggle line numbers", + group: "View", + }, + { + id: "wrap.toggle", + scope: "global", + defaultKeys: ["w"], + description: "toggle line wrap", + group: "View", + }, + { + id: "hunkHeaders.toggle", + scope: "global", + defaultKeys: ["m"], + description: "toggle hunk metadata headers", + group: "View", + }, + // ---------- global: app ---------- + { + id: "quit", + scope: "global", + defaultKeys: ["q", ""], + description: "quit", + group: "App", + }, + { + id: "help.toggle", + scope: "global", + defaultKeys: ["?"], + description: "toggle help", + group: "App", + }, + { + id: "filter.focus", + scope: "global", + defaultKeys: ["/"], + description: "focus file filter", + group: "App", + }, + { + id: "focus.toggle", + scope: "global", + defaultKeys: [""], + description: "toggle files/filter focus", + group: "App", + }, + { + id: "reload", + scope: "global", + defaultKeys: ["r"], + description: "reload current input", + group: "App", + }, + { + id: "menu.open", + scope: "global", + defaultKeys: [""], + description: "open menus", + group: "App", + }, + + // ---------- pager scope ---------- + { + id: "quit", + scope: "pager", + defaultKeys: ["q", ""], + description: "quit", + group: "Pager", + }, + { + id: "scroll.lineDown", + scope: "pager", + defaultKeys: ["j", ""], + description: "scroll one line down", + group: "Pager", + }, + { + id: "scroll.lineUp", + scope: "pager", + defaultKeys: ["k", ""], + description: "scroll one line up", + group: "Pager", + }, + { + id: "scroll.pageDown", + scope: "pager", + defaultKeys: ["", "f", ""], + description: "page down", + group: "Pager", + }, + { + id: "scroll.pageUp", + scope: "pager", + defaultKeys: ["b", "", ""], + description: "page up", + group: "Pager", + }, + { + id: "scroll.halfPageDown", + scope: "pager", + defaultKeys: ["d"], + description: "half page down", + group: "Pager", + }, + { + id: "scroll.halfPageUp", + scope: "pager", + defaultKeys: ["u"], + description: "half page up", + group: "Pager", + }, + { + id: "scroll.toTop", + scope: "pager", + defaultKeys: [""], + description: "jump to top", + group: "Pager", + }, + { + id: "scroll.toBottom", + scope: "pager", + defaultKeys: [""], + description: "jump to bottom", + group: "Pager", + }, + { + id: "scroll.codeLeft", + scope: "pager", + defaultKeys: [""], + description: "scroll code left", + group: "Pager", + }, + { + id: "scroll.codeRight", + scope: "pager", + defaultKeys: [""], + description: "scroll code right", + group: "Pager", + }, + { + id: "scroll.codeLeftFast", + scope: "pager", + defaultKeys: [""], + description: "scroll code left (fast)", + group: "Pager", + }, + { + id: "scroll.codeRightFast", + scope: "pager", + defaultKeys: [""], + description: "scroll code right (fast)", + group: "Pager", + }, + { + id: "wrap.toggle", + scope: "pager", + defaultKeys: ["w"], + description: "toggle line wrap", + group: "Pager", + }, + + // ---------- menu scope ---------- + { + id: "menu.close", + scope: "menu", + defaultKeys: [""], + description: "close menu", + group: "Menu", + }, + { + id: "menu.prev", + scope: "menu", + defaultKeys: [""], + description: "previous menu", + group: "Menu", + }, + { + id: "menu.next", + scope: "menu", + defaultKeys: ["", ""], + description: "next menu", + group: "Menu", + }, + { + id: "menu.itemUp", + scope: "menu", + defaultKeys: [""], + description: "previous item", + group: "Menu", + }, + { + id: "menu.itemDown", + scope: "menu", + defaultKeys: [""], + description: "next item", + group: "Menu", + }, + { + id: "menu.activate", + scope: "menu", + defaultKeys: ["", ""], + description: "activate item", + group: "Menu", + }, + + // ---------- filter scope ---------- + { + id: "focus.toggle", + scope: "filter", + defaultKeys: [""], + description: "leave filter input", + group: "Filter", + }, +]; + +/** All action definitions, indexed by scope. */ +export const ACTIONS_BY_SCOPE: Record = { + global: ACTIONS.filter((action) => action.scope === "global"), + pager: ACTIONS.filter((action) => action.scope === "pager"), + menu: ACTIONS.filter((action) => action.scope === "menu"), + filter: ACTIONS.filter((action) => action.scope === "filter"), +}; + +/** Look up a single (scope, id) action definition. */ +export function getAction(scope: ActionScope, id: ActionId): ActionDef | undefined { + return ACTIONS.find((action) => action.scope === scope && action.id === id); +} + +/** Return every action defined in a scope, preserving registry order. */ +export function getActionsInScope(scope: ActionScope): readonly ActionDef[] { + return ACTIONS_BY_SCOPE[scope]; +} diff --git a/src/core/keymap/format.test.ts b/src/core/keymap/format.test.ts new file mode 100644 index 00000000..d56e5677 --- /dev/null +++ b/src/core/keymap/format.test.ts @@ -0,0 +1,41 @@ +import { describe, expect, test } from "bun:test"; +import { formatBinding, formatKeySpec } from "./format"; +import { parseKeyToken } from "./parse"; + +function spec(token: string) { + const parsed = parseKeyToken(token); + if (!parsed || parsed === "disabled") { + throw new Error(`bad fixture token: ${token}`); + } + return parsed; +} + +describe("formatKeySpec", () => { + test("renders bare characters verbatim", () => { + expect(formatKeySpec(spec("q"))).toBe("q"); + expect(formatKeySpec(spec("?"))).toBe("?"); + expect(formatKeySpec(spec("["))).toBe("["); + }); + + test("renders named keys with friendly labels", () => { + expect(formatKeySpec(spec(""))).toBe("Esc"); + expect(formatKeySpec(spec(""))).toBe("F10"); + expect(formatKeySpec(spec(""))).toBe("Space"); + }); + + test("renders modifiers with + separator", () => { + expect(formatKeySpec(spec(""))).toBe("Ctrl+C"); + expect(formatKeySpec(spec(""))).toBe("Shift+Space"); + expect(formatKeySpec(spec(""))).toBe("Ctrl+Shift+A"); + }); +}); + +describe("formatBinding", () => { + test("joins multiple specs with slashes", () => { + expect(formatBinding([spec("q"), spec("")])).toBe("q / Esc"); + }); + + test("renders empty list as disabled", () => { + expect(formatBinding([])).toBe("disabled"); + }); +}); diff --git a/src/core/keymap/format.ts b/src/core/keymap/format.ts new file mode 100644 index 00000000..ed27c9e5 --- /dev/null +++ b/src/core/keymap/format.ts @@ -0,0 +1,59 @@ +/** + * Render parsed key specs back to human-readable strings for the help dialog. + * The output is intentionally informal (`Ctrl+C`, `Esc`, `Shift+Space`) + * rather than the angle-bracket token form — users reading help expect + * familiar conventional names, not config syntax. + */ + +import type { KeySpec } from "./parse"; + +const NAME_LABELS: Record = { + escape: "Esc", + tab: "Tab", + space: "Space", + enter: "Enter", + backspace: "Backspace", + up: "Up", + down: "Down", + left: "Left", + right: "Right", + home: "Home", + end: "End", + pageup: "PgUp", + pagedown: "PgDn", +}; + +function labelForName(name: string): string { + const label = NAME_LABELS[name]; + if (label) return label; + if (/^f([1-9]|1[0-2])$/.test(name)) return name.toUpperCase(); + if (name.length === 1) return name.toUpperCase(); + return name; +} + +/** Render one KeySpec for the help dialog. */ +export function formatKeySpec(spec: KeySpec): string { + if (spec.sequence !== undefined) { + // Bare-character bindings render verbatim — they're already what the + // user types. + return spec.sequence; + } + + const parts: string[] = []; + if (spec.ctrl) parts.push("Ctrl"); + if (spec.alt) parts.push("Alt"); + if (spec.meta) parts.push("Meta"); + if (spec.shift) parts.push("Shift"); + + if (spec.name !== undefined) { + parts.push(labelForName(spec.name)); + } + + return parts.join("+"); +} + +/** Join multiple bindings for the same action with " / ". */ +export function formatBinding(specs: KeySpec[]): string { + if (specs.length === 0) return "disabled"; + return specs.map(formatKeySpec).join(" / "); +} diff --git a/src/core/keymap/load.test.ts b/src/core/keymap/load.test.ts new file mode 100644 index 00000000..c3a2fe36 --- /dev/null +++ b/src/core/keymap/load.test.ts @@ -0,0 +1,103 @@ +import { describe, expect, test } from "bun:test"; +import type { KeyEvent } from "@opentui/core"; +import { ACTIONS } from "./actions"; +import { applyKeymapOverrides, loadKeymapDefaults } from "./load"; +import { matchesAction } from "./match"; + +function makeKey(overrides: Partial): KeyEvent { + return { + name: "", + ctrl: false, + meta: false, + shift: false, + option: false, + sequence: "", + number: false, + raw: "", + eventType: "press" as KeyEvent["eventType"], + source: "raw", + preventDefault: () => {}, + stopPropagation: () => {}, + ...overrides, + } as unknown as KeyEvent; +} + +describe("loadKeymapDefaults", () => { + test("includes every action defined in the registry", () => { + const keymap = loadKeymapDefaults(); + for (const action of ACTIONS) { + const specs = keymap[action.scope][action.id]; + expect(specs, `missing ${action.scope}.${action.id}`).toBeDefined(); + expect(specs!.length).toBeGreaterThan(0); + } + }); +}); + +describe("applyKeymapOverrides", () => { + test("replaces the bindings for a single action", () => { + const base = loadKeymapDefaults(); + const next = applyKeymapOverrides(base, { + keybindings: { + global: { + quit: "x", + }, + }, + }); + const xKey = makeKey({ name: "x", sequence: "x" }); + const qKey = makeKey({ name: "q", sequence: "q" }); + expect(matchesAction(next, "global", "quit", xKey)).toBe(true); + expect(matchesAction(next, "global", "quit", qKey)).toBe(false); + }); + + test("disables an action via ", () => { + const base = loadKeymapDefaults(); + const next = applyKeymapOverrides(base, { + keybindings: { + global: { + "sidebar.toggle": "", + }, + }, + }); + expect(next.global["sidebar.toggle"]).toEqual([]); + const sKey = makeKey({ name: "s", sequence: "s" }); + expect(matchesAction(next, "global", "sidebar.toggle", sKey)).toBe(false); + }); + + test("supports array form for multiple keys", () => { + const base = loadKeymapDefaults(); + const next = applyKeymapOverrides(base, { + keybindings: { + global: { + quit: ["x", ""], + }, + }, + }); + expect(next.global.quit).toHaveLength(2); + expect(matchesAction(next, "global", "quit", makeKey({ name: "x", sequence: "x" }))).toBe(true); + expect( + matchesAction(next, "global", "quit", makeKey({ name: "c", ctrl: true })), + ).toBe(true); + }); + + test("ignores unknown action ids without throwing", () => { + const base = loadKeymapDefaults(); + const next = applyKeymapOverrides(base, { + keybindings: { + global: { + "this.does.not.exist": "x", + }, + }, + }); + // Defaults preserved. + expect(next.global.quit).toEqual(base.global.quit); + }); + + test("does not mutate the input keymap", () => { + const base = loadKeymapDefaults(); + const beforeQuit = base.global.quit; + applyKeymapOverrides(base, { + keybindings: { global: { quit: "x" } }, + }); + expect(base.global.quit).toBe(beforeQuit); + }); +}); diff --git a/src/core/keymap/load.ts b/src/core/keymap/load.ts new file mode 100644 index 00000000..cf013256 --- /dev/null +++ b/src/core/keymap/load.ts @@ -0,0 +1,110 @@ +/** + * Build a keymap from defaults and apply user/repo TOML overrides. + * + * Layering convention (caller-driven): + * defaults -> user config -> repo config + * + * Each layer can override individual `[keybindings.]` keys; missing + * keys keep the previous layer's value. To unbind a default, the user sets + * the action to `` (parsed elsewhere as an empty spec list). + */ + +import { isRecord } from "../config"; +import { + ACTIONS, + ACTIONS_BY_SCOPE, + type ActionId, + type ActionScope, +} from "./actions"; +import type { Keymap } from "./match"; +import { parseBinding, type KeySpec } from "./parse"; + +const ACTION_SCOPES = Object.keys(ACTIONS_BY_SCOPE) as ActionScope[]; + +const KNOWN_IDS_BY_SCOPE: Record> = ACTION_SCOPES.reduce( + (acc, scope) => { + acc[scope] = new Set(ACTIONS_BY_SCOPE[scope].map((action) => action.id)); + return acc; + }, + {} as Record>, +); + +/** Build a fresh keymap populated from the action registry's defaults. */ +export function loadKeymapDefaults(): Keymap { + const keymap: Keymap = { + global: {}, + pager: {}, + menu: {}, + filter: {}, + }; + + for (const action of ACTIONS) { + const parsed = parseBinding(action.defaultKeys); + keymap[action.scope][action.id] = parsed.disabled ? [] : parsed.specs; + } + + return keymap; +} + +function isStringOrStringArray(value: unknown): value is string | string[] { + if (typeof value === "string") return true; + return Array.isArray(value) && value.every((item) => typeof item === "string"); +} + +/** Clone a keymap so override application doesn't mutate the input. */ +function cloneKeymap(base: Keymap): Keymap { + return { + global: { ...base.global }, + pager: { ...base.pager }, + menu: { ...base.menu }, + filter: { ...base.filter }, + }; +} + +/** + * Apply `[keybindings.]` overrides from a parsed TOML object onto a + * base keymap. Unknown action ids and malformed bindings emit a one-line + * stderr warning and are otherwise ignored — config errors should never + * abort startup. + */ +export function applyKeymapOverrides( + base: Keymap, + source: Record, +): Keymap { + const keybindings = source.keybindings; + if (!isRecord(keybindings)) return base; + + const next = cloneKeymap(base); + + for (const scope of ACTION_SCOPES) { + const scopeOverrides = keybindings[scope]; + if (!isRecord(scopeOverrides)) continue; + + const knownIds = KNOWN_IDS_BY_SCOPE[scope]; + + for (const [actionId, value] of Object.entries(scopeOverrides)) { + if (!knownIds.has(actionId as ActionId)) { + process.stderr.write( + `[hunk] keybindings: unknown action "${scope}.${actionId}" — ignored.\n`, + ); + continue; + } + if (!isStringOrStringArray(value)) { + process.stderr.write( + `[hunk] keybindings: invalid binding for "${scope}.${actionId}" (expected string or string[]) — ignored.\n`, + ); + continue; + } + const parsed = parseBinding(value); + for (const rejected of parsed.rejectedTokens) { + process.stderr.write( + `[hunk] keybindings: unrecognized token "${rejected}" for "${scope}.${actionId}" — skipped.\n`, + ); + } + const specs: KeySpec[] = parsed.disabled ? [] : parsed.specs; + next[scope][actionId as ActionId] = specs; + } + } + + return next; +} diff --git a/src/core/keymap/match.test.ts b/src/core/keymap/match.test.ts new file mode 100644 index 00000000..e6b59a2c --- /dev/null +++ b/src/core/keymap/match.test.ts @@ -0,0 +1,144 @@ +import { describe, expect, test } from "bun:test"; +import type { KeyEvent } from "@opentui/core"; +import { findActionForKey, matchesAction, matchesKey } from "./match"; +import { loadKeymapDefaults } from "./load"; +import { parseKeyToken } from "./parse"; + +function makeKey(overrides: Partial): KeyEvent { + return { + name: "", + ctrl: false, + meta: false, + shift: false, + option: false, + sequence: "", + number: false, + raw: "", + eventType: "press" as KeyEvent["eventType"], + source: "raw", + preventDefault: () => {}, + stopPropagation: () => {}, + ...overrides, + } as unknown as KeyEvent; +} + +describe("matchesKey", () => { + test("matches bare-character sequence", () => { + const spec = parseKeyToken("q"); + if (!spec || spec === "disabled") throw new Error("bad fixture"); + expect(matchesKey(spec, makeKey({ sequence: "q", name: "q" }))).toBe(true); + expect(matchesKey(spec, makeKey({ sequence: "x", name: "x" }))).toBe(false); + }); + + test("matches escape via either name alias", () => { + const spec = parseKeyToken(""); + if (!spec || spec === "disabled") throw new Error("bad fixture"); + expect(matchesKey(spec, makeKey({ name: "escape" }))).toBe(true); + expect(matchesKey(spec, makeKey({ name: "esc" }))).toBe(true); + }); + + test("matches enter and return alias", () => { + const spec = parseKeyToken(""); + if (!spec || spec === "disabled") throw new Error("bad fixture"); + expect(matchesKey(spec, makeKey({ name: "return" }))).toBe(true); + expect(matchesKey(spec, makeKey({ name: "enter" }))).toBe(true); + }); + + test("differentiates shift-left from left", () => { + const left = parseKeyToken(""); + const shiftLeft = parseKeyToken(""); + if (!left || left === "disabled") throw new Error("bad fixture"); + if (!shiftLeft || shiftLeft === "disabled") throw new Error("bad fixture"); + + const plainLeft = makeKey({ name: "left" }); + const heldShiftLeft = makeKey({ name: "left", shift: true }); + + // Plain spec matches with or without shift (modifiers are + // "must-be-present" only when the spec asks for them). + expect(matchesKey(left, plainLeft)).toBe(true); + expect(matchesKey(left, heldShiftLeft)).toBe(true); + + // Shift-left spec only matches when shift is held. + expect(matchesKey(shiftLeft, plainLeft)).toBe(false); + expect(matchesKey(shiftLeft, heldShiftLeft)).toBe(true); + }); + + test("matches space with all OpenTUI variants", () => { + const spec = parseKeyToken(""); + if (!spec || spec === "disabled") throw new Error("bad fixture"); + expect(matchesKey(spec, makeKey({ name: "space" }))).toBe(true); + expect(matchesKey(spec, makeKey({ name: " " }))).toBe(true); + expect(matchesKey(spec, makeKey({ sequence: " " }))).toBe(true); + }); + + test("requires ctrl modifier when specified", () => { + const spec = parseKeyToken(""); + if (!spec || spec === "disabled") throw new Error("bad fixture"); + expect(matchesKey(spec, makeKey({ name: "c", ctrl: true }))).toBe(true); + expect(matchesKey(spec, makeKey({ name: "c" }))).toBe(false); + }); +}); + +describe("matchesAction", () => { + test("dispatches via the default keymap", () => { + const keymap = loadKeymapDefaults(); + const key = makeKey({ name: "q", sequence: "q" }); + expect(matchesAction(keymap, "global", "quit", key)).toBe(true); + expect(matchesAction(keymap, "global", "help.toggle", key)).toBe(false); + }); +}); + +describe("findActionForKey", () => { + test("returns the matching action id in scope", () => { + const keymap = loadKeymapDefaults(); + const f10 = makeKey({ name: "f10" }); + expect(findActionForKey(keymap, "global", f10)).toBe("menu.open"); + const noMatch = makeKey({ name: "x", sequence: "x" }); + expect(findActionForKey(keymap, "global", noMatch)).toBeNull(); + }); +}); + +/** + * Regression: Shift+Space must scroll up, not down. The matcher is + * modifier-permissive by design (a bare `` spec matches with or + * without shift), so the hook has to query the more-specific `scroll.pageUp` + * binding before the bare-space `scroll.pageDown` binding. These tests pin + * down both the spec-level overlap and the resolution order the hook relies + * on so a future reorder of the handler chain trips the test instead of + * silently inverting Shift+Space. + */ +describe("Shift+Space page-up precedence", () => { + const shiftSpace = () => makeKey({ name: "space", shift: true }); + + test("Shift+Space matches both pageUp and pageDown by default", () => { + const keymap = loadKeymapDefaults(); + // Both true is fine: the handler chain picks pageUp first. + expect(matchesAction(keymap, "global", "scroll.pageUp", shiftSpace())).toBe(true); + expect(matchesAction(keymap, "global", "scroll.pageDown", shiftSpace())).toBe(true); + expect(matchesAction(keymap, "pager", "scroll.pageUp", shiftSpace())).toBe(true); + expect(matchesAction(keymap, "pager", "scroll.pageDown", shiftSpace())).toBe(true); + }); + + // Mirror the hook's "first action that matches wins" sequence for the page + // family. If someone reorders the hook so pageDown is checked first, this + // resolver returns the wrong id and the test fails loudly. + function resolvePageAction(scope: "global" | "pager", key: KeyEvent) { + const keymap = loadKeymapDefaults(); + const ordered = ["scroll.pageUp", "scroll.pageDown"] as const; + for (const action of ordered) { + if (matchesAction(keymap, scope, action, key)) return action; + } + return null; + } + + test("Shift+Space resolves to pageUp under the hook's check order", () => { + expect(resolvePageAction("global", shiftSpace())).toBe("scroll.pageUp"); + expect(resolvePageAction("pager", shiftSpace())).toBe("scroll.pageUp"); + }); + + test("plain Space still resolves to pageDown", () => { + const space = makeKey({ name: "space" }); + expect(resolvePageAction("global", space)).toBe("scroll.pageDown"); + expect(resolvePageAction("pager", space)).toBe("scroll.pageDown"); + }); +}); diff --git a/src/core/keymap/match.ts b/src/core/keymap/match.ts new file mode 100644 index 00000000..fbfb8a9b --- /dev/null +++ b/src/core/keymap/match.ts @@ -0,0 +1,92 @@ +/** + * Match parsed key specs against live KeyEvent objects emitted by OpenTUI's + * keyboard layer. A spec matches a KeyEvent when: + * + * - sequence specs (bare printable like `q`, `?`, `[`): require + * `key.sequence === spec.sequence`. Modifiers on the live event are + * ignored — the literal-sequence form is "type this character", not + * "press this physical key". + * - name specs (``, ``, ``, etc.): require `key.name` match + * and require every modifier flagged on the spec to be present on the + * event. Spec modifiers are "must-be-present"; spec-without-modifier + * does NOT require the modifier to be absent. This preserves the + * pre-keymap behavior where bare-character handlers fired regardless of + * incidental shift state. + */ + +import type { KeyEvent } from "@opentui/core"; +import { isEscapeKey } from "../keyboard"; +import type { ActionId, ActionScope } from "./actions"; +import type { KeySpec } from "./parse"; + +export type Keymap = Record>>; + +/** Match a single KeySpec against a live KeyEvent. */ +export function matchesKey(spec: KeySpec, key: KeyEvent): boolean { + if (spec.sequence !== undefined) { + return key.sequence === spec.sequence; + } + + if (spec.name === undefined) return false; + + if (!nameMatches(spec.name, key)) return false; + + // Required modifiers must be present. OpenTUI exposes alt under `option`. + if (spec.ctrl && !key.ctrl) return false; + if (spec.shift && !key.shift) return false; + if (spec.meta && !key.meta) return false; + if (spec.alt && !key.option) return false; + + return true; +} + +/** + * Compare a spec's key name to the live event, accounting for the small set + * of OpenTUI/terminal aliases (`enter`/`return`, `escape`/`esc`, and the + * space variants). + */ +function nameMatches(specName: string, key: KeyEvent): boolean { + if (specName === "enter") { + return key.name === "return" || key.name === "enter"; + } + if (specName === "escape") { + return isEscapeKey(key); + } + if (specName === "space") { + return key.name === "space" || key.name === " " || key.sequence === " "; + } + return key.name === specName; +} + +/** True when any spec bound to `(scope, action)` matches the event. */ +export function matchesAction( + keymap: Keymap, + scope: ActionScope, + action: ActionId, + key: KeyEvent, +): boolean { + const specs = keymap[scope]?.[action]; + if (!specs || specs.length === 0) return false; + return specs.some((spec) => matchesKey(spec, key)); +} + +/** + * Resolve which action (if any) is bound to the live event in this scope. + * Used when the caller wants to dispatch by id rather than poll-by-action. + */ +export function findActionForKey( + keymap: Keymap, + scope: ActionScope, + key: KeyEvent, +): ActionId | null { + const scopeMap = keymap[scope]; + if (!scopeMap) return null; + + for (const [action, specs] of Object.entries(scopeMap)) { + if (!specs || specs.length === 0) continue; + if (specs.some((spec) => matchesKey(spec, key))) { + return action as ActionId; + } + } + return null; +} diff --git a/src/core/keymap/parse.test.ts b/src/core/keymap/parse.test.ts new file mode 100644 index 00000000..6ccf0efd --- /dev/null +++ b/src/core/keymap/parse.test.ts @@ -0,0 +1,91 @@ +import { describe, expect, test } from "bun:test"; +import { parseBinding, parseKeyToken } from "./parse"; + +describe("parseKeyToken", () => { + test("parses bare printable as sequence", () => { + const spec = parseKeyToken("q"); + expect(spec).toEqual({ + sequence: "q", + ctrl: false, + shift: false, + meta: false, + alt: false, + }); + }); + + test("parses bracketed special key", () => { + const spec = parseKeyToken(""); + expect(spec).toEqual({ + name: "escape", + ctrl: false, + shift: false, + meta: false, + alt: false, + }); + }); + + test("parses ctrl modifier", () => { + const spec = parseKeyToken(""); + expect(spec).toMatchObject({ name: "c", ctrl: true }); + }); + + test("parses shift+arrow", () => { + const spec = parseKeyToken(""); + expect(spec).toMatchObject({ name: "up", shift: true }); + }); + + test("parses stacked modifiers", () => { + const spec = parseKeyToken(""); + expect(spec).toMatchObject({ name: "a", ctrl: true, shift: true }); + }); + + test("parses function key", () => { + expect(parseKeyToken("")).toMatchObject({ name: "f10" }); + expect(parseKeyToken("")).toMatchObject({ name: "f1" }); + }); + + test("returns disabled sentinel", () => { + expect(parseKeyToken("")).toBe("disabled"); + }); + + test("rejects malformed tokens", () => { + expect(parseKeyToken("")).toBeNull(); + expect(parseKeyToken("<>")).toBeNull(); + expect(parseKeyToken("")).toBeNull(); + expect(parseKeyToken("")).toBeNull(); + }); + + test("treats alias as enter", () => { + expect(parseKeyToken("")).toMatchObject({ name: "enter" }); + expect(parseKeyToken("")).toMatchObject({ name: "enter" }); + }); +}); + +describe("parseBinding", () => { + test("accepts string form", () => { + const result = parseBinding("q"); + expect(result.disabled).toBe(false); + expect(result.specs).toHaveLength(1); + expect(result.specs[0]?.sequence).toBe("q"); + }); + + test("accepts array form", () => { + const result = parseBinding(["q", ""]); + expect(result.specs).toHaveLength(2); + }); + + test("disabled wins over other tokens", () => { + const result = parseBinding(["q", ""]); + expect(result.disabled).toBe(true); + expect(result.specs).toEqual([]); + }); + + test("reports unparseable tokens via rejectedTokens", () => { + const result = parseBinding(["q", ""]); + expect(result.specs).toHaveLength(1); + expect(result.specs[0]?.sequence).toBe("q"); + expect(result.rejectedTokens).toEqual([""]); + }); +}); diff --git a/src/core/keymap/parse.ts b/src/core/keymap/parse.ts new file mode 100644 index 00000000..f31d8064 --- /dev/null +++ b/src/core/keymap/parse.ts @@ -0,0 +1,183 @@ +/** + * Token parser for keybinding strings. The grammar mirrors lazygit's: + * + * - `` => sentinel that unbinds an action. + * - `` => named special key (`esc`, `tab`, `f10`, ...). + * - `` / `` / ... => named key with modifier prefixes (`c-`, `s-`, + * `a-`, `m-`). Modifiers may stack (``). + * - bare `q`, `?`, `[`, `{` => literal printable; matched by `key.sequence`. + * + * No multi-key sequences are supported in v1 (single chord per binding). + */ + +export interface KeySpec { + /** Named OpenTUI `key.name` to match (e.g. `escape`, `f10`, `tab`). */ + name?: string; + /** Literal `key.sequence` to match for bare-character bindings. */ + sequence?: string; + ctrl: boolean; + shift: boolean; + meta: boolean; + alt: boolean; +} + +const NAMED_KEY_TOKENS: Record = { + esc: "escape", + escape: "escape", + tab: "tab", + space: "space", + enter: "enter", + return: "enter", + backspace: "backspace", + up: "up", + down: "down", + left: "left", + right: "right", + home: "home", + end: "end", + pgup: "pageup", + pageup: "pageup", + pgdown: "pagedown", + pagedown: "pagedown", +}; + +function isFunctionKeyToken(token: string): boolean { + if (token.length < 2 || token.length > 3) return false; + if (token[0] !== "f") return false; + const num = Number(token.slice(1)); + return Number.isInteger(num) && num >= 1 && num <= 12; +} + +/** + * Parse a single binding token. Returns `"disabled"` for the unbind sentinel + * and `null` on parse error so the caller can log and ignore. + */ +export function parseKeyToken(rawToken: string): KeySpec | "disabled" | null { + const token = rawToken.trim(); + if (token.length === 0) return null; + + // Bare single character (and not enclosed in <>): literal sequence match. + if (token[0] !== "<") { + if (token.length !== 1) { + // Anything multi-character outside angle brackets is a parse error. + return null; + } + return { + sequence: token, + ctrl: false, + shift: false, + meta: false, + alt: false, + }; + } + + if (token[token.length - 1] !== ">") return null; + const inner = token.slice(1, -1).toLowerCase(); + if (inner.length === 0) return null; + + if (inner === "disabled") return "disabled"; + + const parts = inner.split("-"); + let ctrl = false; + let shift = false; + let meta = false; + let alt = false; + + // Modifier prefixes ("c", "s", "m", "a") in any order; the final segment is + // always the key name (or a single literal char). + while (parts.length > 1) { + const head = parts[0]; + if (head === "c") { + ctrl = true; + } else if (head === "s") { + shift = true; + } else if (head === "m") { + meta = true; + } else if (head === "a") { + alt = true; + } else { + break; + } + parts.shift(); + } + + if (parts.length !== 1) return null; + const keyToken = parts[0]; + if (keyToken === undefined) return null; + + const namedKey = NAMED_KEY_TOKENS[keyToken]; + if (namedKey) { + return { + name: namedKey, + ctrl, + shift, + meta, + alt, + }; + } + + if (isFunctionKeyToken(keyToken)) { + return { + name: keyToken, + ctrl, + shift, + meta, + alt, + }; + } + + // Single character with modifiers (e.g. ``). We match by `key.name` + // because OpenTUI emits `name === "c"` for `Ctrl+C`. + if (keyToken.length === 1) { + return { + name: keyToken, + ctrl, + shift, + meta, + alt, + }; + } + + return null; +} + +export interface ParsedBinding { + /** Parsed specs in source order. Empty if the binding is disabled. */ + specs: KeySpec[]; + /** True when any token was `` — caller should treat as unbound. */ + disabled: boolean; + /** Tokens that failed to parse, surfaced so callers can warn with context. */ + rejectedTokens: string[]; +} + +/** Parse a binding value (single string or array) into normalized specs. */ +export function parseBinding(value: string | string[]): ParsedBinding { + const tokens = Array.isArray(value) ? value : [value]; + const specs: KeySpec[] = []; + const rejectedTokens: string[] = []; + let disabled = false; + + for (const token of tokens) { + if (typeof token !== "string") { + rejectedTokens.push(String(token)); + continue; + } + const parsed = parseKeyToken(token); + if (parsed === "disabled") { + // Disabled wins; callers ignore specs when disabled is set. + disabled = true; + continue; + } + if (parsed === null) { + rejectedTokens.push(token); + continue; + } + specs.push(parsed); + } + + if (disabled) { + return { specs: [], disabled: true, rejectedTokens }; + } + + return { specs, disabled: false, rejectedTokens }; +} diff --git a/src/core/types.ts b/src/core/types.ts index b79fa395..8120fe41 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -1,4 +1,5 @@ import type { FileDiffMetadata } from "@pierre/diffs"; +import type { Keymap } from "./keymap/match"; export type LayoutMode = "auto" | "split" | "stack"; export type VcsMode = "git" | "jj"; @@ -67,6 +68,8 @@ export interface CommonOptions { wrapLines?: boolean; hunkHeaders?: boolean; agentNotes?: boolean; + /** Resolved keymap for this invocation; populated by config layering. */ + keymap?: Keymap; } export interface PersistedViewPreferences { diff --git a/src/ui/App.tsx b/src/ui/App.tsx index 56b71523..569ac8f6 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -5,6 +5,7 @@ import { } from "@opentui/core"; import { useRenderer, useTerminalDimensions } from "@opentui/react"; import { Suspense, lazy, useCallback, useEffect, useMemo, useState, useRef } from "react"; +import { loadKeymapDefaults } from "../core/keymap/load"; import type { AppBootstrap, CliInput, LayoutMode } from "../core/types"; import { canReloadInput, computeWatchSignature } from "../core/watch"; import type { HunkSessionBrokerClient, ReloadedSessionResult } from "../hunk-session/types"; @@ -538,6 +539,13 @@ export function App({ toggleMenu, } = useMenuController(menus); + // Bootstraps prepared by tests or older callers may not include a resolved + // keymap on `input.options`; fall back to defaults so behavior is identical. + const keymap = useMemo( + () => bootstrap.input.options.keymap ?? loadKeymapDefaults(), + [bootstrap.input.options.keymap], + ); + useAppKeyboardShortcuts({ activeMenuId, activateCurrentMenuItem, @@ -547,6 +555,7 @@ export function App({ cycleTheme, focusArea, focusFilter, + keymap, moveToAnnotatedHunk, moveToHunk: review.moveToHunk, moveMenuItem, @@ -768,6 +777,7 @@ export function App({ (); + + const trackedActions = ACTIONS.filter((action) => { + // Pager-scope entries duplicate global navigation. Only show them if the + // user has actually rebound them — otherwise they're noise. + if (action.scope !== "global") return false; + if (action.id === "reload" && !canRefresh) return false; + return true; + }); + + for (const action of trackedActions) { + const specs = keymap[action.scope][action.id] ?? []; + const row = buildRow(action, specs); + + let bucket = buckets.get(action.group); + if (!bucket) { + bucket = []; + buckets.set(action.group, bucket); + order.push(action.group); + } + bucket.push(row); + } + + const sections: HelpSection[] = order.map((title) => ({ + title, + items: buckets.get(title) ?? [], + })); + + // Preserve "Mouse" placement between Navigation and View — it sits with the + // primary movement controls. Insert just before the "View" group. + const viewIndex = sections.findIndex((section) => section.title === "View"); + const insertAt = viewIndex >= 0 ? viewIndex : sections.length; + sections.splice(insertAt, 0, MOUSE_SECTION); + + return sections; +} + +function buildRow(action: ActionDef, specs: KeySpec[]): HelpRow { + const keys = specs.length === 0 ? "disabled" : formatBinding(specs); + return { + keys, + description: action.description, + }; +} + /** Render the in-app controls help modal. */ export function HelpDialog({ canRefresh = false, + keymap, terminalHeight, terminalWidth, theme, onClose, }: { canRefresh?: boolean; + keymap: Keymap; terminalHeight: number; terminalWidth: number; theme: AppTheme; onClose: () => void; }) { - const sections = [ - { - title: "Navigation", - items: [ - ["↑ / ↓", "move line-by-line"], - ["Space / f", "page down (alt: f)"], - ["b", "page up"], - ["Shift+Space", "page up (alt)"], - ["d / u", "half page down / up"], - ["[ / ]", "previous / next hunk"], - ["{ / }", "previous / next comment"], - ["← / →", "scroll code left / right (Shift = faster)"], - ["Home / End", "jump to top / bottom"], - ], - }, - { - title: "Mouse", - items: [ - ["Wheel", "scroll vertically"], - ["Shift+Wheel", "scroll code horizontally"], - ], - }, - { - title: "View", - items: [ - ["1 / 2 / 0", "split / stack / auto"], - ["s / t", "sidebar / theme"], - ["a", "toggle AI notes"], - ["l / w / m", "lines / wrap / metadata"], - ], - }, - { - title: "Review", - items: [ - ["/", "focus file filter"], - ["Tab", "toggle files/filter focus"], - ["F10", "open menus"], - [canRefresh ? "r / q" : "q", canRefresh ? "reload / quit" : "quit"], - ], - }, - ] as const; + const sections = useMemo(() => buildHelpSections(keymap, canRefresh), [keymap, canRefresh]); const width = Math.min(74, Math.max(56, terminalWidth - 8)); const bodyWidth = Math.max(1, width - 4); - const keyWidth = Math.min(16, Math.max(12, Math.floor(bodyWidth * 0.28))); + const keyWidth = Math.min(20, Math.max(12, Math.floor(bodyWidth * 0.32))); const descriptionWidth = Math.max(1, bodyWidth - keyWidth); const sectionSpacerRowCount = Math.max(0, sections.length - 1); const contentRowCount = @@ -78,13 +116,13 @@ export function HelpDialog({ {section.title} - {section.items.map(([keys, description]) => ( + {section.items.map((row) => ( - {padText(fitText(keys, keyWidth), keyWidth)} - {fitText(description, descriptionWidth)} + {padText(fitText(row.keys, keyWidth), keyWidth)} + {fitText(row.description, descriptionWidth)} ))} {sectionIndex < sections.length - 1 ? : null} diff --git a/src/ui/components/chrome/StatusBar.tsx b/src/ui/components/chrome/StatusBar.tsx index a5bda04f..041af68a 100644 --- a/src/ui/components/chrome/StatusBar.tsx +++ b/src/ui/components/chrome/StatusBar.tsx @@ -1,4 +1,4 @@ -import { isEscapeKey } from "../../lib/keyboard"; +import { isEscapeKey } from "../../../core/keyboard"; import type { AppTheme } from "../../themes"; /** Render the active file filter input or current filter summary. */ diff --git a/src/ui/components/ui-components.test.tsx b/src/ui/components/ui-components.test.tsx index 668a3f15..17f92e83 100644 --- a/src/ui/components/ui-components.test.tsx +++ b/src/ui/components/ui-components.test.tsx @@ -13,6 +13,7 @@ import { buildFileSectionLayouts, buildInStreamFileHeaderHeights } from "../lib/ const { AppHost } = await import("../AppHost"); const { buildSidebarEntries } = await import("../lib/files"); const { HelpDialog } = await import("./chrome/HelpDialog"); +const { loadKeymapDefaults } = await import("../../core/keymap/load"); const { SidebarPane } = await import("./panes/SidebarPane"); const { AgentCard } = await import("./panes/AgentCard"); const { AgentInlineNote } = await import("./panes/AgentInlineNote"); @@ -1583,48 +1584,45 @@ describe("UI components", () => { test("HelpDialog renders every documented control row without overlap", async () => { const theme = resolveTheme("midnight", null); + const keymap = loadKeymapDefaults(); const frame = await captureFrame( {}} />, 76, - 36, + 56, ); - const expectedRows = [ + // The exact row layout is now driven by the action registry; assert that + // every section header and each action description renders in the modal, + // leaving the precise key-column formatting to the format-helper tests. + const expectedFragments = [ "Controls help", - "[Esc]", "Navigation", - "↑ / ↓ move line-by-line", - "Space / f page down (alt: f)", - "b page up", - "Shift+Space page up (alt)", - "d / u half page down / up", - "[ / ] previous / next hunk", - "{ / } previous / next comment", - "← / → scroll code left / right (Shift = faster)", - "Home / End jump to top / bottom", "Mouse", - "Wheel scroll vertically", - "Shift+Wheel scroll code horizontally", "View", - "1 / 2 / 0 split / stack / auto", - "s / t sidebar / theme", - "a toggle AI notes", - "l / w / m lines / wrap / metadata", + "App", "Review", - "/ focus file filter", - "Tab toggle files/filter focus", - "F10 open menus", - "r / q reload / quit", - ] as const; - - for (const expectedRow of expectedRows) { - expect(frame).toContain(expectedRow); + "move line-by-line (down)", + "page down", + "previous hunk", + "next comment", + "split layout", + "toggle agent notes", + "toggle line numbers", + "focus file filter", + "reload current input", + "open menus", + "scroll vertically", + ]; + + for (const fragment of expectedFragments) { + expect(frame).toContain(fragment); } const lines = frame.split("\n"); @@ -1636,8 +1634,6 @@ describe("UI components", () => { expect(lines[mouseHeaderIndex - 1]).toMatch(blankModalRow); expect(lines[viewHeaderIndex - 1]).toMatch(blankModalRow); expect(lines[reviewHeaderIndex - 1]).toMatch(blankModalRow); - expect(frame).not.toContain("linese/Awrapt/smetadata"); - expect(frame).not.toContain("reloade/uquit"); }); test("DiffSectionPlaceholder preserves offscreen section chrome without mounting rows", async () => { diff --git a/src/ui/hooks/useAppKeyboardShortcuts.ts b/src/ui/hooks/useAppKeyboardShortcuts.ts index f2349d9e..e2118495 100644 --- a/src/ui/hooks/useAppKeyboardShortcuts.ts +++ b/src/ui/hooks/useAppKeyboardShortcuts.ts @@ -1,18 +1,11 @@ import type { KeyEvent } from "@opentui/core"; import { useKeyboard } from "@opentui/react"; import { useRef } from "react"; +import type { ActionId, ActionScope } from "../../core/keymap/actions"; +import type { Keymap } from "../../core/keymap/match"; +import { matchesAction } from "../../core/keymap/match"; import type { LayoutMode } from "../../core/types"; import type { MenuId } from "../components/chrome/menu"; -import { - isEscapeKey, - isHalfPageDownKey, - isHalfPageUpKey, - isPageDownKey, - isPageUpKey, - isShiftSpacePageUpKey, - isStepDownKey, - isStepUpKey, -} from "../lib/keyboard"; type FocusArea = "files" | "filter"; type ScrollUnit = "step" | "viewport" | "content" | "half"; @@ -28,6 +21,7 @@ export interface UseAppKeyboardShortcutsOptions { cycleTheme: () => void; focusArea: FocusArea; focusFilter: () => void; + keymap: Keymap; moveToAnnotatedHunk: (delta: number) => void; moveToHunk: (delta: number) => void; moveMenuItem: (delta: number) => void; @@ -59,6 +53,7 @@ export function useAppKeyboardShortcuts({ cycleTheme, focusArea, focusFilter, + keymap, moveToAnnotatedHunk, moveToHunk, moveMenuItem, @@ -83,19 +78,24 @@ export function useAppKeyboardShortcuts({ const focusAreaRef = useRef(focusArea); const pagerModeRef = useRef(pagerMode); const showHelpRef = useRef(showHelp); + const keymapRef = useRef(keymap); activeMenuIdRef.current = activeMenuId; focusAreaRef.current = focusArea; pagerModeRef.current = pagerMode; showHelpRef.current = showHelp; + keymapRef.current = keymap; const runAndCloseMenu = (action: () => void) => { action(); closeMenu(); }; + const isAction = (scope: ActionScope, id: ActionId, key: KeyEvent) => + matchesAction(keymapRef.current, scope, id, key); + const handleMenuToggleShortcut = (key: KeyEvent) => { - if (key.name !== "f10") { + if (!isAction("global", "menu.open", key)) { return false; } @@ -113,62 +113,74 @@ export function useAppKeyboardShortcuts({ }; const handlePagerShortcut = (key: KeyEvent) => { - if (key.name === "q" || isEscapeKey(key)) { + if (isAction("pager", "quit", key)) { requestQuit(); return; } - if (isPageDownKey(key)) { - scrollDiff(1, "viewport"); + // pageUp must be queried before pageDown — see match.test.ts "Shift+Space + // page-up precedence". Same pattern repeats for codeLeftFast/codeRightFast. + if (isAction("pager", "scroll.pageUp", key)) { + scrollDiff(-1, "viewport"); return; } - if (isPageUpKey(key) || isShiftSpacePageUpKey(key)) { - scrollDiff(-1, "viewport"); + if (isAction("pager", "scroll.pageDown", key)) { + scrollDiff(1, "viewport"); return; } - if (isHalfPageDownKey(key)) { + if (isAction("pager", "scroll.halfPageDown", key)) { scrollDiff(1, "half"); return; } - if (isHalfPageUpKey(key)) { + if (isAction("pager", "scroll.halfPageUp", key)) { scrollDiff(-1, "half"); return; } - if (isStepDownKey(key)) { + if (isAction("pager", "scroll.lineDown", key)) { scrollDiff(1, "step"); return; } - if (isStepUpKey(key)) { + if (isAction("pager", "scroll.lineUp", key)) { scrollDiff(-1, "step"); return; } - if (key.name === "left") { - scrollCodeHorizontally(key.shift ? -FAST_CODE_HORIZONTAL_SCROLL_COLUMNS : -1); + if (isAction("pager", "scroll.codeLeftFast", key)) { + scrollCodeHorizontally(-FAST_CODE_HORIZONTAL_SCROLL_COLUMNS); + return; + } + + if (isAction("pager", "scroll.codeRightFast", key)) { + scrollCodeHorizontally(FAST_CODE_HORIZONTAL_SCROLL_COLUMNS); + return; + } + + if (isAction("pager", "scroll.codeLeft", key)) { + scrollCodeHorizontally(-1); return; } - if (key.name === "right") { - scrollCodeHorizontally(key.shift ? FAST_CODE_HORIZONTAL_SCROLL_COLUMNS : 1); + if (isAction("pager", "scroll.codeRight", key)) { + scrollCodeHorizontally(1); return; } - if (key.name === "home") { + if (isAction("pager", "scroll.toTop", key)) { scrollDiff(-1, "content"); return; } - if (key.name === "end") { + if (isAction("pager", "scroll.toBottom", key)) { scrollDiff(1, "content"); return; } - if (key.name === "w" || key.sequence === "w") { + if (isAction("pager", "wrap.toggle", key)) { toggleLineWrap(); return; } @@ -179,7 +191,10 @@ export function useAppKeyboardShortcuts({ }; const handleHelpShortcut = (key: KeyEvent) => { - if (!showHelpRef.current || !isEscapeKey(key)) { + if (!showHelpRef.current) { + return false; + } + if (!isAction("global", "quit", key) && !isAction("global", "help.toggle", key)) { return false; } @@ -192,32 +207,32 @@ export function useAppKeyboardShortcuts({ return false; } - if (isEscapeKey(key)) { + if (isAction("menu", "menu.close", key)) { closeMenu(); return true; } - if (key.name === "left") { + if (isAction("menu", "menu.prev", key)) { switchMenu(-1); return true; } - if (key.name === "right" || key.name === "tab") { + if (isAction("menu", "menu.next", key)) { switchMenu(1); return true; } - if (key.name === "up") { + if (isAction("menu", "menu.itemUp", key)) { moveMenuItem(-1); return true; } - if (key.name === "down") { + if (isAction("menu", "menu.itemDown", key)) { moveMenuItem(1); return true; } - if (key.name === "return" || key.name === "enter") { + if (isAction("menu", "menu.activate", key)) { activateCurrentMenuItem(); return true; } @@ -230,7 +245,7 @@ export function useAppKeyboardShortcuts({ return false; } - if (key.name === "tab") { + if (isAction("filter", "focus.toggle", key)) { toggleFocusArea(); return true; } @@ -240,148 +255,153 @@ export function useAppKeyboardShortcuts({ }; const handleAppShortcut = (key: KeyEvent) => { - if (key.name === "q") { + if (isAction("global", "quit", key)) { requestQuit(); return; } - if (key.name === "?" || key.sequence === "?") { + if (isAction("global", "help.toggle", key)) { toggleHelp(); closeMenu(); return; } - if (isEscapeKey(key)) { - requestQuit(); - return; - } - - if (key.name === "tab") { + if (isAction("global", "focus.toggle", key)) { toggleFocusArea(); return; } - if (key.name === "/") { + if (isAction("global", "filter.focus", key)) { focusFilter(); return; } - if (isPageDownKey(key)) { - scrollDiff(1, "viewport"); + if (isAction("global", "scroll.pageUp", key)) { + scrollDiff(-1, "viewport"); return; } - if (isPageUpKey(key) || isShiftSpacePageUpKey(key)) { - scrollDiff(-1, "viewport"); + if (isAction("global", "scroll.pageDown", key)) { + scrollDiff(1, "viewport"); return; } - if (isHalfPageDownKey(key)) { + if (isAction("global", "scroll.halfPageDown", key)) { scrollDiff(1, "half"); return; } - if (isHalfPageUpKey(key)) { + if (isAction("global", "scroll.halfPageUp", key)) { scrollDiff(-1, "half"); return; } - if (key.name === "home") { + if (isAction("global", "scroll.toTop", key)) { scrollDiff(-1, "content"); return; } - if (key.name === "end") { + if (isAction("global", "scroll.toBottom", key)) { scrollDiff(1, "content"); return; } - if (isStepUpKey(key)) { + if (isAction("global", "scroll.lineUp", key)) { scrollDiff(-1, "step"); return; } - if (isStepDownKey(key)) { + if (isAction("global", "scroll.lineDown", key)) { scrollDiff(1, "step"); return; } - if (key.name === "left") { - scrollCodeHorizontally(key.shift ? -FAST_CODE_HORIZONTAL_SCROLL_COLUMNS : -1); + if (isAction("global", "scroll.codeLeftFast", key)) { + scrollCodeHorizontally(-FAST_CODE_HORIZONTAL_SCROLL_COLUMNS); + return; + } + + if (isAction("global", "scroll.codeRightFast", key)) { + scrollCodeHorizontally(FAST_CODE_HORIZONTAL_SCROLL_COLUMNS); + return; + } + + if (isAction("global", "scroll.codeLeft", key)) { + scrollCodeHorizontally(-1); return; } - if (key.name === "right") { - scrollCodeHorizontally(key.shift ? FAST_CODE_HORIZONTAL_SCROLL_COLUMNS : 1); + if (isAction("global", "scroll.codeRight", key)) { + scrollCodeHorizontally(1); return; } - if (key.name === "1") { + if (isAction("global", "layout.split", key)) { runAndCloseMenu(() => selectLayoutMode("split")); return; } - if (key.name === "2") { + if (isAction("global", "layout.stack", key)) { runAndCloseMenu(() => selectLayoutMode("stack")); return; } - if (key.name === "0") { + if (isAction("global", "layout.auto", key)) { runAndCloseMenu(() => selectLayoutMode("auto")); return; } - if (key.name === "s") { + if (isAction("global", "sidebar.toggle", key)) { runAndCloseMenu(toggleSidebar); return; } - if ((key.name === "r" || key.sequence === "r") && canRefreshCurrentInput) { + if (isAction("global", "reload", key) && canRefreshCurrentInput) { runAndCloseMenu(triggerRefreshCurrentInput); return; } - if (key.name === "t") { + if (isAction("global", "theme.cycle", key)) { runAndCloseMenu(cycleTheme); return; } - if (key.name === "a") { + if (isAction("global", "agentNotes.toggle", key)) { runAndCloseMenu(toggleAgentNotes); return; } - if (key.name === "l" || key.sequence === "l") { + if (isAction("global", "lineNumbers.toggle", key)) { runAndCloseMenu(toggleLineNumbers); return; } - if (key.name === "w" || key.sequence === "w") { + if (isAction("global", "wrap.toggle", key)) { runAndCloseMenu(toggleLineWrap); return; } - if (key.name === "m" || key.sequence === "m") { + if (isAction("global", "hunkHeaders.toggle", key)) { runAndCloseMenu(toggleHunkHeaders); return; } - if (key.name === "[") { + if (isAction("global", "hunk.prev", key)) { runAndCloseMenu(() => moveToHunk(-1)); return; } - if (key.name === "]") { + if (isAction("global", "hunk.next", key)) { runAndCloseMenu(() => moveToHunk(1)); return; } - if (key.sequence === "{") { + if (isAction("global", "annotatedHunk.prev", key)) { runAndCloseMenu(() => moveToAnnotatedHunk(-1)); return; } - if (key.sequence === "}") { + if (isAction("global", "annotatedHunk.next", key)) { runAndCloseMenu(() => moveToAnnotatedHunk(1)); } }; diff --git a/src/ui/lib/keyboard.ts b/src/ui/lib/keyboard.ts deleted file mode 100644 index 8bb81e83..00000000 --- a/src/ui/lib/keyboard.ts +++ /dev/null @@ -1,50 +0,0 @@ -import type { KeyEvent } from "@opentui/core"; - -function isSpaceKey(key: KeyEvent) { - return key.name === "space" || key.name === " " || key.sequence === " "; -} - -/** Normalize the escape key aliases emitted by different terminal input paths. */ -export function isEscapeKey(key: KeyEvent) { - return key.name === "escape" || key.name === "esc"; -} - -/** Match any key alias that should scroll forward by a full viewport. */ -export function isPageDownKey(key: KeyEvent) { - return ( - key.name === "pagedown" || - (!key.shift && isSpaceKey(key)) || - key.name === "f" || - key.sequence === "f" - ); -} - -/** Match any key alias that should scroll backward by a full viewport. */ -export function isPageUpKey(key: KeyEvent) { - return key.name === "pageup" || key.name === "b" || key.sequence === "b"; -} - -/** Match any key alias that should scroll forward by a single diff row. */ -export function isStepDownKey(key: KeyEvent) { - return key.name === "down" || key.name === "j" || key.sequence === "j"; -} - -/** Match any key alias that should scroll backward by a single diff row. */ -export function isStepUpKey(key: KeyEvent) { - return key.name === "up" || key.name === "k" || key.sequence === "k"; -} - -/** Match any key alias that should scroll forward by half a viewport. */ -export function isHalfPageDownKey(key: KeyEvent) { - return key.name === "d" || key.sequence === "d"; -} - -/** Match any key alias that should scroll backward by half a viewport. */ -export function isHalfPageUpKey(key: KeyEvent) { - return key.name === "u" || key.sequence === "u"; -} - -/** Match the less-style Shift+Space reverse page key. */ -export function isShiftSpacePageUpKey(key: KeyEvent) { - return key.shift && isSpaceKey(key); -} diff --git a/src/ui/lib/ui-lib.test.ts b/src/ui/lib/ui-lib.test.ts index 98c77b8f..0d58dbd7 100644 --- a/src/ui/lib/ui-lib.test.ts +++ b/src/ui/lib/ui-lib.test.ts @@ -11,16 +11,9 @@ import { } from "../components/chrome/menu"; import { buildAgentPopoverContent, resolveAgentPopoverPlacement, wrapText } from "./agentPopover"; import { buildAppMenus } from "./appMenus"; -import { - isEscapeKey, - isHalfPageDownKey, - isHalfPageUpKey, - isPageDownKey, - isPageUpKey, - isShiftSpacePageUpKey, - isStepDownKey, - isStepUpKey, -} from "./keyboard"; +import { isEscapeKey } from "../../core/keyboard"; +import { loadKeymapDefaults } from "../../core/keymap/load"; +import { matchesAction } from "../../core/keymap/match"; import { fitText, padText } from "./text"; import { computeHunkRevealScrollTop } from "./hunkScroll"; import { estimateDiffSectionBodyRows, measureDiffSectionGeometry } from "./diffSectionGeometry"; @@ -177,27 +170,24 @@ describe("ui helpers", () => { ).toBe(true); }); - test("keyboard alias helpers normalize the shared scroll shortcut keys", () => { + test("default keymap covers the shared scroll shortcut keys", () => { + const keymap = loadKeymapDefaults(); expect(isEscapeKey(createKeyEvent({ name: "escape" }))).toBe(true); expect(isEscapeKey(createKeyEvent({ name: "esc" }))).toBe(true); - expect(isPageDownKey(createKeyEvent({ name: "pagedown" }))).toBe(true); - expect(isPageDownKey(createKeyEvent({ name: "space" }))).toBe(true); - expect(isPageDownKey(createKeyEvent({ name: "f" }))).toBe(true); - expect(isPageDownKey(createKeyEvent({ sequence: "f" }))).toBe(true); - expect(isPageUpKey(createKeyEvent({ name: "pageup" }))).toBe(true); - expect(isPageUpKey(createKeyEvent({ name: "b" }))).toBe(true); - expect(isPageUpKey(createKeyEvent({ sequence: "b" }))).toBe(true); - expect(isShiftSpacePageUpKey(createKeyEvent({ name: "space", shift: true }))).toBe(true); - expect(isHalfPageDownKey(createKeyEvent({ name: "d" }))).toBe(true); - expect(isHalfPageUpKey(createKeyEvent({ sequence: "u" }))).toBe(true); - expect(isStepDownKey(createKeyEvent({ name: "down" }))).toBe(true); - expect(isStepDownKey(createKeyEvent({ sequence: "j" }))).toBe(true); - expect(isStepUpKey(createKeyEvent({ name: "up" }))).toBe(true); - expect(isStepUpKey(createKeyEvent({ sequence: "k" }))).toBe(true); + expect(matchesAction(keymap, "global", "scroll.pageDown", createKeyEvent({ name: "pagedown" }))).toBe(true); + expect(matchesAction(keymap, "global", "scroll.pageDown", createKeyEvent({ name: "space" }))).toBe(true); + expect(matchesAction(keymap, "global", "scroll.pageDown", createKeyEvent({ sequence: "f" }))).toBe(true); + expect(matchesAction(keymap, "global", "scroll.pageUp", createKeyEvent({ name: "pageup" }))).toBe(true); + expect(matchesAction(keymap, "global", "scroll.pageUp", createKeyEvent({ sequence: "b" }))).toBe(true); + expect(matchesAction(keymap, "global", "scroll.pageUp", createKeyEvent({ name: "space", shift: true }))).toBe(true); + expect(matchesAction(keymap, "global", "scroll.halfPageDown", createKeyEvent({ sequence: "d" }))).toBe(true); + expect(matchesAction(keymap, "global", "scroll.halfPageUp", createKeyEvent({ sequence: "u" }))).toBe(true); + expect(matchesAction(keymap, "global", "scroll.lineDown", createKeyEvent({ name: "down" }))).toBe(true); + expect(matchesAction(keymap, "global", "scroll.lineDown", createKeyEvent({ sequence: "j" }))).toBe(true); + expect(matchesAction(keymap, "global", "scroll.lineUp", createKeyEvent({ name: "up" }))).toBe(true); + expect(matchesAction(keymap, "global", "scroll.lineUp", createKeyEvent({ sequence: "k" }))).toBe(true); expect(isEscapeKey(createKeyEvent({ name: "q" }))).toBe(false); - expect(isPageDownKey(createKeyEvent({ name: "space", shift: true }))).toBe(false); - expect(isPageDownKey(createKeyEvent({ name: "q" }))).toBe(false); - expect(isShiftSpacePageUpKey(createKeyEvent({ name: "space", shift: false }))).toBe(false); + expect(matchesAction(keymap, "global", "scroll.pageDown", createKeyEvent({ sequence: "q" }))).toBe(false); }); test("fitText and padText clamp using the terminal fallback marker", () => { From b18f3917dee2a1337b7b1958849ad80b06c085a4 Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Fri, 8 May 2026 18:46:54 -0500 Subject: [PATCH 02/10] fix(keymap): address review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - restore `q` quitting while help is open (regression in 89ad19d) - bare-char matcher accepts `key.name` parity with `key.sequence` - guard TOML parse errors so malformed config doesn't abort startup - warn on empty `[]` bindings, unknown scopes, and `` mixed with other tokens — previously silent - enumerate the pager scope in docs/keybindings.md and tighten the "logged once" wording - pin pager-scope independence, user/repo layering, overridden HelpDialog rendering, modifier-required matching, and the rejectedTokens warning path with new tests --- docs/keybindings.md | 27 +++++- src/core/config.test.ts | 71 +++++++++++++++ src/core/config.ts | 22 +++-- src/core/keymap/load.test.ts | 106 +++++++++++++++++++++++ src/core/keymap/load.ts | 24 +++++ src/core/keymap/match.test.ts | 26 ++++++ src/core/keymap/match.ts | 8 +- src/core/keymap/parse.test.ts | 19 ++++ src/core/keymap/parse.ts | 19 +++- src/ui/AppHost.interactions.test.tsx | 74 ++++++++++++++++ src/ui/components/ui-components.test.tsx | 32 ++++++- src/ui/hooks/useAppKeyboardShortcuts.ts | 12 ++- 12 files changed, 419 insertions(+), 21 deletions(-) diff --git a/docs/keybindings.md b/docs/keybindings.md index bd3f905b..6075134d 100644 --- a/docs/keybindings.md +++ b/docs/keybindings.md @@ -45,6 +45,8 @@ Tokens use angle brackets for special keys and modifiers, mirroring lazygit: | ``–`` | Function keys. | | `` | Ctrl+C. | | `` | Shift+Up. | +| `` | Shift+Space — modifiers stack on any named key, not just arrows. | +| `` | Ctrl+PageDown — same idea with a different modifier. | | ``, `` | Alt+X / Meta+X. | | `` | Modifiers stack in any order. | | `` | Sentinel that unbinds the action. | @@ -59,8 +61,8 @@ To unbind a default: "sidebar.toggle" = "" ``` -Unknown action ids and malformed tokens are logged once to stderr and -otherwise ignored. They never abort startup. +Unknown action ids and malformed tokens are logged to stderr and otherwise +ignored. They never abort startup. ## Action reference @@ -103,8 +105,25 @@ otherwise ignored. They never abort startup. ### `[keybindings.pager]` The pager scope mirrors the global scroll/wrap actions for `hunk pager`. -`quit` defaults to `q` and ``. Defaults are the same as the matching -global actions; rebind them under this section to override pager-only. +Defaults match the corresponding global actions; rebind them under this +section to override pager-only. + +| Action | Default keys | What it does | +| ---------------------- | --------------------------- | ----------------------------- | +| `quit` | `q`, `` | Quit. | +| `scroll.lineDown` | `j`, `` | Scroll one line down. | +| `scroll.lineUp` | `k`, `` | Scroll one line up. | +| `scroll.pageDown` | ``, `f`, `` | Page down. | +| `scroll.pageUp` | `b`, ``, `` | Page up. | +| `scroll.halfPageDown` | `d` | Half page down. | +| `scroll.halfPageUp` | `u` | Half page up. | +| `scroll.toTop` | `` | Jump to top. | +| `scroll.toBottom` | `` | Jump to bottom. | +| `scroll.codeLeft` | `` | Scroll code left one column. | +| `scroll.codeRight` | `` | Scroll code right one column. | +| `scroll.codeLeftFast` | `` | Scroll code left (fast). | +| `scroll.codeRightFast` | `` | Scroll code right (fast). | +| `wrap.toggle` | `w` | Toggle line wrap. | ### `[keybindings.menu]` diff --git a/src/core/config.test.ts b/src/core/config.test.ts index b24d5394..d7ec918c 100644 --- a/src/core/config.test.ts +++ b/src/core/config.test.ts @@ -341,4 +341,75 @@ describe("config resolution", () => { // unrelated defaults still present. expect(keymap.global["help.toggle"]?.length ?? 0).toBeGreaterThan(0); }); + + test("repo keybindings override user-level keybindings", () => { + const home = createTempDir("hunk-config-home-"); + const repo = createTempDir("hunk-config-repo-"); + createRepo(repo); + + mkdirSync(join(home, ".config", "hunk"), { recursive: true }); + writeFileSync( + join(home, ".config", "hunk", "config.toml"), + ["[keybindings.global]", 'quit = "x"', ""].join("\n"), + ); + + mkdirSync(join(repo, ".hunk"), { recursive: true }); + writeFileSync( + join(repo, ".hunk", "config.toml"), + ["[keybindings.global]", 'quit = "y"', ""].join("\n"), + ); + + const resolved = resolveConfiguredCliInput( + { + kind: "patch", + file: "-", + options: { pager: false }, + }, + { cwd: repo, env: { HOME: home } }, + ); + const keymap = resolved.input.options.keymap; + expect(keymap).toBeDefined(); + if (!keymap) return; + + const quit = keymap.global.quit ?? []; + expect(quit).toHaveLength(1); + expect(quit[0]?.sequence).toBe("y"); + }); + + test("malformed TOML config does not abort startup", async () => { + const home = createTempDir("hunk-config-home-"); + const cwd = createTempDir("hunk-config-cwd-"); + + mkdirSync(join(home, ".config", "hunk"), { recursive: true }); + writeFileSync( + join(home, ".config", "hunk", "config.toml"), + 'theme = "graphite\n[keybindings.global\nquit = "y"\n', + ); + + // Silence the expected stderr warning so the test runner stays clean. + const originalWrite = process.stderr.write.bind(process.stderr); + const captured: string[] = []; + process.stderr.write = ((chunk: unknown) => { + captured.push(typeof chunk === "string" ? chunk : String(chunk)); + return true; + }) as typeof process.stderr.write; + + try { + const resolved = resolveConfiguredCliInput( + { + kind: "patch", + file: "-", + options: { pager: false }, + }, + { cwd, env: { HOME: home } }, + ); + + // Defaults preserved despite the malformed file. + expect(resolved.input.options.theme).toBe("graphite"); + expect(resolved.input.options.keymap?.global.quit?.[0]?.sequence).toBe("q"); + expect(captured.some((line) => line.includes("parse error"))).toBe(true); + } finally { + process.stderr.write = originalWrite; + } + }); }); diff --git a/src/core/config.ts b/src/core/config.ts index 60c0001a..f97cd245 100644 --- a/src/core/config.ts +++ b/src/core/config.ts @@ -131,18 +131,28 @@ function detectRepoVcsMode(repoRoot?: string): VcsMode { return "git"; } -/** Parse one TOML config file into a plain object. */ +/** + * Parse one TOML config file into a plain object. Missing files yield `{}`; + * malformed TOML and non-object roots are reported to stderr and treated as + * absent so a bad config never aborts startup. + */ function readTomlRecord(path: string) { if (!fs.existsSync(path)) { return {}; } - const parsed = Bun.TOML.parse(fs.readFileSync(path, "utf8")); - if (!isRecord(parsed)) { - throw new Error(`Expected ${path} to contain a TOML object.`); + try { + const parsed = Bun.TOML.parse(fs.readFileSync(path, "utf8")); + if (!isRecord(parsed)) { + process.stderr.write(`[hunk] config: ${path} is not a TOML object — ignored.\n`); + return {}; + } + return parsed; + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + process.stderr.write(`[hunk] config: parse error in ${path}: ${message} — ignored.\n`); + return {}; } - - return parsed; } /** Resolve CLI input against global and repo-local config files. */ diff --git a/src/core/keymap/load.test.ts b/src/core/keymap/load.test.ts index c3a2fe36..ab8fdc3b 100644 --- a/src/core/keymap/load.test.ts +++ b/src/core/keymap/load.test.ts @@ -4,6 +4,26 @@ import { ACTIONS } from "./actions"; import { applyKeymapOverrides, loadKeymapDefaults } from "./load"; import { matchesAction } from "./match"; +/** + * Replace `process.stderr.write` with an in-memory collector for the duration + * of `fn`. Returns the captured chunks so tests can assert on warning content + * without leaking warnings into the test runner's output. + */ +async function captureStderr(fn: () => void | Promise): Promise { + const chunks: string[] = []; + const original = process.stderr.write.bind(process.stderr); + process.stderr.write = ((chunk: unknown) => { + chunks.push(typeof chunk === "string" ? chunk : String(chunk)); + return true; + }) as typeof process.stderr.write; + try { + await fn(); + } finally { + process.stderr.write = original; + } + return chunks; +} + function makeKey(overrides: Partial): KeyEvent { return { name: "", @@ -100,4 +120,90 @@ describe("applyKeymapOverrides", () => { }); expect(base.global.quit).toBe(beforeQuit); }); + + test("warns and skips empty array bindings", async () => { + const base = loadKeymapDefaults(); + let next!: ReturnType; + const stderr = await captureStderr(() => { + next = applyKeymapOverrides(base, { + keybindings: { global: { quit: [] } }, + }); + }); + + // Empty array should be ignored, defaults preserved. + expect(next.global.quit).toEqual(base.global.quit); + expect(stderr.some((line) => line.includes("empty binding") && line.includes("global.quit"))).toBe(true); + }); + + test("warns on unknown scope but keeps walking known scopes", async () => { + const base = loadKeymapDefaults(); + let next!: ReturnType; + const stderr = await captureStderr(() => { + next = applyKeymapOverrides(base, { + keybindings: { + gloabl: { quit: "x" }, + global: { quit: "z" }, + }, + }); + }); + + expect(stderr.some((line) => line.includes('unknown scope "gloabl"'))).toBe(true); + // Known scope still applied. + const zKey = makeKey({ name: "z", sequence: "z" }); + expect(matchesAction(next, "global", "quit", zKey)).toBe(true); + }); + + test("pager scope bindings are independent of global overrides", () => { + const base = loadKeymapDefaults(); + const next = applyKeymapOverrides(base, { + keybindings: { global: { quit: "x" } }, + }); + + // Overriding global quit must not touch the pager scope's quit binding. + expect(next.pager.quit).toEqual(base.pager.quit); + const qKey = makeKey({ name: "q", sequence: "q" }); + expect(matchesAction(next, "pager", "quit", qKey)).toBe(true); + }); + + test("rejected tokens warn but still bind the parseable ones", async () => { + const base = loadKeymapDefaults(); + let next!: ReturnType; + const stderr = await captureStderr(() => { + next = applyKeymapOverrides(base, { + keybindings: { global: { quit: ["q", ""] } }, + }); + }); + + expect( + stderr.some( + (line) => + line.includes("") && line.includes("global.quit") && line.includes("unrecognized"), + ), + ).toBe(true); + + // `q` still binds even though `` was dropped. + const qKey = makeKey({ name: "q", sequence: "q" }); + expect(matchesAction(next, "global", "quit", qKey)).toBe(true); + }); + + test("warns when mixes with other tokens", async () => { + const base = loadKeymapDefaults(); + let next!: ReturnType; + const stderr = await captureStderr(() => { + next = applyKeymapOverrides(base, { + keybindings: { global: { quit: ["q", ""] } }, + }); + }); + + expect( + stderr.some( + (line) => + line.includes("global.quit") && + line.includes("") && + line.includes("mixes"), + ), + ).toBe(true); + // Disabled still wins — caller is warned, but the binding ends up empty. + expect(next.global.quit).toEqual([]); + }); }); diff --git a/src/core/keymap/load.ts b/src/core/keymap/load.ts index cf013256..d2816d05 100644 --- a/src/core/keymap/load.ts +++ b/src/core/keymap/load.ts @@ -76,6 +76,16 @@ export function applyKeymapOverrides( const next = cloneKeymap(base); + // Walk the user's scopes once first so we can warn on typos like + // `[keybindings.gloabl]` instead of silently dropping the whole block. + for (const scope of Object.keys(keybindings)) { + if (!(ACTION_SCOPES as readonly string[]).includes(scope)) { + process.stderr.write( + `[hunk] keybindings: unknown scope "${scope}" — ignored. (valid: ${ACTION_SCOPES.join(", ")})\n`, + ); + } + } + for (const scope of ACTION_SCOPES) { const scopeOverrides = keybindings[scope]; if (!isRecord(scopeOverrides)) continue; @@ -89,6 +99,15 @@ export function applyKeymapOverrides( ); continue; } + // Detect `quit = []` before the array passes the type check below — an + // empty array would otherwise silently disable the action without the + // explicit `` sentinel. + if (Array.isArray(value) && value.length === 0) { + process.stderr.write( + `[hunk] keybindings: empty binding for "${scope}.${actionId}" — use "" to unbind. ignored.\n`, + ); + continue; + } if (!isStringOrStringArray(value)) { process.stderr.write( `[hunk] keybindings: invalid binding for "${scope}.${actionId}" (expected string or string[]) — ignored.\n`, @@ -101,6 +120,11 @@ export function applyKeymapOverrides( `[hunk] keybindings: unrecognized token "${rejected}" for "${scope}.${actionId}" — skipped.\n`, ); } + if (parsed.mixedWithDisabled) { + process.stderr.write( + `[hunk] keybindings: "${scope}.${actionId}" mixes "" with other tokens — entire binding disabled.\n`, + ); + } const specs: KeySpec[] = parsed.disabled ? [] : parsed.specs; next[scope][actionId as ActionId] = specs; } diff --git a/src/core/keymap/match.test.ts b/src/core/keymap/match.test.ts index e6b59a2c..6400d1d1 100644 --- a/src/core/keymap/match.test.ts +++ b/src/core/keymap/match.test.ts @@ -77,6 +77,32 @@ describe("matchesKey", () => { expect(matchesKey(spec, makeKey({ name: "c", ctrl: true }))).toBe(true); expect(matchesKey(spec, makeKey({ name: "c" }))).toBe(false); }); + + test("bare-character spec accepts events that only set name", () => { + // Some OpenTUI input paths emit `name` without `sequence` for printables. + // The matcher must accept either signal, otherwise those paths regress. + const spec = parseKeyToken("r"); + if (!spec || spec === "disabled") throw new Error("bad fixture"); + expect(matchesKey(spec, makeKey({ name: "r" }))).toBe(true); + expect(matchesKey(spec, makeKey({ sequence: "r" }))).toBe(true); + expect(matchesKey(spec, makeKey({ name: "x" }))).toBe(false); + }); + + test("modifier-required specs do not match plain events", () => { + // The reverse direction of the modifier rule: spec asks for shift/ctrl/alt, + // event is missing it, so the match must fail. These are cheap pins on the + // most common regression vector. + const shiftUp = parseKeyToken(""); + const ctrlC = parseKeyToken(""); + const metaX = parseKeyToken(""); + if (!shiftUp || shiftUp === "disabled") throw new Error("bad fixture"); + if (!ctrlC || ctrlC === "disabled") throw new Error("bad fixture"); + if (!metaX || metaX === "disabled") throw new Error("bad fixture"); + + expect(matchesKey(shiftUp, makeKey({ name: "up" }))).toBe(false); + expect(matchesKey(ctrlC, makeKey({ name: "c" }))).toBe(false); + expect(matchesKey(metaX, makeKey({ name: "x" }))).toBe(false); + }); }); describe("matchesAction", () => { diff --git a/src/core/keymap/match.ts b/src/core/keymap/match.ts index fbfb8a9b..db1714ac 100644 --- a/src/core/keymap/match.ts +++ b/src/core/keymap/match.ts @@ -24,7 +24,13 @@ export type Keymap = Record>>; /** Match a single KeySpec against a live KeyEvent. */ export function matchesKey(spec: KeySpec, key: KeyEvent): boolean { if (spec.sequence !== undefined) { - return key.sequence === spec.sequence; + if (key.sequence === spec.sequence) return true; + // Defensive parity with the pre-keymap hook, which accepted both + // `key.name === "X"` and `key.sequence === "X"` for single-character keys. + // Some OpenTUI input paths populate `name` without `sequence` for bare + // printables; matching either keeps those events binding correctly. + if (spec.sequence.length === 1 && key.name === spec.sequence) return true; + return false; } if (spec.name === undefined) return false; diff --git a/src/core/keymap/parse.test.ts b/src/core/keymap/parse.test.ts index 6ccf0efd..d49ccd34 100644 --- a/src/core/keymap/parse.test.ts +++ b/src/core/keymap/parse.test.ts @@ -80,6 +80,24 @@ describe("parseBinding", () => { const result = parseBinding(["q", ""]); expect(result.disabled).toBe(true); expect(result.specs).toEqual([]); + expect(result.mixedWithDisabled).toBe(true); + }); + + test("plain is not flagged as mixed", () => { + const single = parseBinding(""); + expect(single.disabled).toBe(true); + expect(single.mixedWithDisabled).toBe(false); + + const arr = parseBinding([""]); + expect(arr.disabled).toBe(true); + expect(arr.mixedWithDisabled).toBe(false); + }); + + test("disabled mixed with a rejected token is also flagged", () => { + const result = parseBinding(["", ""]); + expect(result.disabled).toBe(true); + expect(result.mixedWithDisabled).toBe(true); + expect(result.rejectedTokens).toEqual([""]); }); test("reports unparseable tokens via rejectedTokens", () => { @@ -87,5 +105,6 @@ describe("parseBinding", () => { expect(result.specs).toHaveLength(1); expect(result.specs[0]?.sequence).toBe("q"); expect(result.rejectedTokens).toEqual([""]); + expect(result.mixedWithDisabled).toBe(false); }); }); diff --git a/src/core/keymap/parse.ts b/src/core/keymap/parse.ts index f31d8064..f1a4207d 100644 --- a/src/core/keymap/parse.ts +++ b/src/core/keymap/parse.ts @@ -146,6 +146,13 @@ export interface ParsedBinding { specs: KeySpec[]; /** True when any token was `` — caller should treat as unbound. */ disabled: boolean; + /** + * True when `` appeared alongside any other token (parsed or + * rejected). Pure `""` is *not* mixed; only loud combinations + * like `["q", ""]` set this flag so callers can warn that the + * other tokens were silently dropped. + */ + mixedWithDisabled: boolean; /** Tokens that failed to parse, surfaced so callers can warn with context. */ rejectedTokens: string[]; } @@ -156,10 +163,14 @@ export function parseBinding(value: string | string[]): ParsedBinding { const specs: KeySpec[] = []; const rejectedTokens: string[] = []; let disabled = false; + // Track non-disabled tokens separately so the "disabled wins" zeroing + // doesn't erase the evidence we need for mixedWithDisabled. + let hadOtherTokens = false; for (const token of tokens) { if (typeof token !== "string") { rejectedTokens.push(String(token)); + hadOtherTokens = true; continue; } const parsed = parseKeyToken(token); @@ -170,14 +181,18 @@ export function parseBinding(value: string | string[]): ParsedBinding { } if (parsed === null) { rejectedTokens.push(token); + hadOtherTokens = true; continue; } specs.push(parsed); + hadOtherTokens = true; } + const mixedWithDisabled = disabled && hadOtherTokens; + if (disabled) { - return { specs: [], disabled: true, rejectedTokens }; + return { specs: [], disabled: true, mixedWithDisabled, rejectedTokens }; } - return { specs, disabled: false, rejectedTokens }; + return { specs, disabled: false, mixedWithDisabled, rejectedTokens }; } diff --git a/src/ui/AppHost.interactions.test.tsx b/src/ui/AppHost.interactions.test.tsx index 82dc9e45..3d6773f1 100644 --- a/src/ui/AppHost.interactions.test.tsx +++ b/src/ui/AppHost.interactions.test.tsx @@ -2307,6 +2307,80 @@ describe("App interactions", () => { } }); + test("pressing q while help is open still routes through onQuit (regression for help-fall-through)", async () => { + // Pre-keymap behavior on main: only Esc closed help, but `q` still quit the + // app even while help was open. The keymap rewrite regressed this by making + // `q` (matching `global.quit`) close help instead of falling through to the + // app handler. This test pins the fall-through: with help open, `q` must + // still reach the quit handler. The companion test below confirms that Esc + // (and the help toggle key) continue to close the dialog. + const onQuit = mock(() => undefined); + const setup = await testRender( + , + { width: 220, height: 32 }, + ); + + try { + await flush(setup); + + await act(async () => { + await setup.mockInput.typeText("?"); + }); + await flush(setup); + + const frame = setup.captureCharFrame(); + expect(frame).toContain("Controls help"); + + await act(async () => { + await setup.mockInput.typeText("q"); + }); + await flush(setup); + + expect(onQuit).toHaveBeenCalledTimes(1); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("Esc closes the help dialog without quitting the app", async () => { + const onQuit = mock(() => undefined); + const setup = await testRender( + , + { width: 220, height: 32 }, + ); + + try { + await flush(setup); + + await act(async () => { + await setup.mockInput.typeText("?"); + }); + await flush(setup); + + let frame = setup.captureCharFrame(); + expect(frame).toContain("Controls help"); + + await act(async () => { + await setup.mockInput.pressEscape(); + }); + await flush(setup); + await act(async () => { + await Bun.sleep(40); + await setup.renderOnce(); + }); + + frame = await waitForFrame(setup, (currentFrame) => !currentFrame.includes("Controls help")); + expect(frame).not.toContain("Controls help"); + expect(onQuit).not.toHaveBeenCalled(); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + test("quit shortcuts route through the provided onQuit handler in regular and pager modes", async () => { const regularQuit = mock(() => undefined); const regularSetup = await testRender( diff --git a/src/ui/components/ui-components.test.tsx b/src/ui/components/ui-components.test.tsx index 17f92e83..8db86283 100644 --- a/src/ui/components/ui-components.test.tsx +++ b/src/ui/components/ui-components.test.tsx @@ -13,7 +13,7 @@ import { buildFileSectionLayouts, buildInStreamFileHeaderHeights } from "../lib/ const { AppHost } = await import("../AppHost"); const { buildSidebarEntries } = await import("../lib/files"); const { HelpDialog } = await import("./chrome/HelpDialog"); -const { loadKeymapDefaults } = await import("../../core/keymap/load"); +const { applyKeymapOverrides, loadKeymapDefaults } = await import("../../core/keymap/load"); const { SidebarPane } = await import("./panes/SidebarPane"); const { AgentCard } = await import("./panes/AgentCard"); const { AgentInlineNote } = await import("./panes/AgentInlineNote"); @@ -1636,6 +1636,36 @@ describe("UI components", () => { expect(lines[reviewHeaderIndex - 1]).toMatch(blankModalRow); }); + test("HelpDialog reflects an overridden quit binding instead of the default `q`", async () => { + const theme = resolveTheme("midnight", null); + const keymap = applyKeymapOverrides(loadKeymapDefaults(), { + keybindings: { global: { quit: "" } }, + }); + + const frame = await captureFrame( + {}} + />, + 76, + 56, + ); + + // Find the row that holds the "quit" action description and check its + // key column shows the override label, not the original `q`. + const quitLine = frame.split("\n").find((line) => line.includes("quit")); + expect(quitLine).toBeDefined(); + expect(quitLine!).toContain("Ctrl+C"); + // The default `q` binding must not appear as the quit label anymore. Allow + // `q` elsewhere (e.g. in unrelated descriptions); the assertion targets + // the quit row specifically. + expect(quitLine!).not.toMatch(/^[^a-zA-Z0-9]*q\s/); + }); + test("DiffSectionPlaceholder preserves offscreen section chrome without mounting rows", async () => { const bootstrap = createBootstrap(); const theme = resolveTheme("midnight", null); diff --git a/src/ui/hooks/useAppKeyboardShortcuts.ts b/src/ui/hooks/useAppKeyboardShortcuts.ts index e2118495..d0837ca4 100644 --- a/src/ui/hooks/useAppKeyboardShortcuts.ts +++ b/src/ui/hooks/useAppKeyboardShortcuts.ts @@ -4,6 +4,7 @@ import { useRef } from "react"; import type { ActionId, ActionScope } from "../../core/keymap/actions"; import type { Keymap } from "../../core/keymap/match"; import { matchesAction } from "../../core/keymap/match"; +import { isEscapeKey } from "../../core/keyboard"; import type { LayoutMode } from "../../core/types"; import type { MenuId } from "../components/chrome/menu"; @@ -191,13 +192,10 @@ export function useAppKeyboardShortcuts({ }; const handleHelpShortcut = (key: KeyEvent) => { - if (!showHelpRef.current) { - return false; - } - if (!isAction("global", "quit", key) && !isAction("global", "help.toggle", key)) { - return false; - } - + if (!showHelpRef.current) return false; + // Only Esc and the help-toggle binding close help here. Other keys (notably + // `quit`) fall through so they continue to fire their app-level handlers. + if (!isEscapeKey(key) && !isAction("global", "help.toggle", key)) return false; closeHelp(); return true; }; From 0d1e5234696251d38d5ecf9a8062f7138b232369 Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Sun, 10 May 2026 00:25:27 -0500 Subject: [PATCH 03/10] test(pty): cover keymap regressions in live UI Three PTY tests that exercise the keymap end-to-end through OpenTUI: - pressing `q` while the help dialog is open quits the app (regression pinned: keymap rewrite briefly broke fall-through to the global quit binding when the help-close gate matched on `q`) - pressing Esc closes the help dialog without quitting - pressing `space` then `b` exercises bare-character pageDown / pageUp dispatch through the matcher The `` precedence over `` is pinned in match.test.ts; tuistory's PTY encoder drops the shift modifier on space (no CSI u keycode for space), so it can't be exercised end-to-end here. --- test/pty/ui-integration.test.ts | 107 ++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/test/pty/ui-integration.test.ts b/test/pty/ui-integration.test.ts index cd769913..cad58957 100644 --- a/test/pty/ui-integration.test.ts +++ b/test/pty/ui-integration.test.ts @@ -1258,4 +1258,111 @@ describe("live UI integration", () => { session.close(); } }); + + test("q quits the app while the help dialog is open", async () => { + const fixture = harness.createTwoFileRepoFixture(); + const session = await harness.launchHunk({ + args: ["diff", "--mode", "split"], + cwd: fixture.dir, + cols: 220, + rows: 24, + }); + + try { + await session.waitForText(/View\s+Navigate\s+Theme\s+Agent\s+Help/, { + timeout: 15_000, + }); + + await session.press("?"); + await session.waitForText(/Controls help|Keyboard help/, { timeout: 5_000 }); + + await session.press("q"); + // Old hook only matched Esc to dismiss help, so q fell through to + // requestQuit. The keymap rewrite briefly broke this — q closed help + // and stayed in the app. Quit should clear the chrome row entirely. + const after = await harness.waitForSnapshot( + session, + (text) => !text.includes("View Navigate Theme Agent Help"), + 5_000, + ); + expect(after).not.toContain("View Navigate Theme Agent Help"); + } finally { + session.close(); + } + }); + + test("Esc closes the help dialog without quitting the app", async () => { + const fixture = harness.createTwoFileRepoFixture(); + const session = await harness.launchHunk({ + args: ["diff", "--mode", "split"], + cwd: fixture.dir, + cols: 220, + rows: 24, + }); + + try { + await session.waitForText(/View\s+Navigate\s+Theme\s+Agent\s+Help/, { + timeout: 15_000, + }); + + await session.press("?"); + await session.waitForText(/Controls help|Keyboard help/, { timeout: 5_000 }); + + await session.press("escape"); + const after = await harness.waitForSnapshot( + session, + (text) => !text.includes("Controls help") && !text.includes("Keyboard help"), + 5_000, + ); + // App must still be alive — chrome row should be redrawn. + expect(after).toMatch(/View\s+Navigate\s+Theme\s+Agent\s+Help/); + expect(after).not.toContain("Controls help"); + expect(after).not.toContain("Keyboard help"); + } finally { + session.close(); + } + }); + + test("b pages back up after space pages down", async () => { + const fixture = harness.createScrollableFilePair(); + const session = await harness.launchHunk({ + args: ["diff", fixture.before, fixture.after, "--mode", "split"], + cols: 220, + rows: 12, + }); + + try { + const initial = await session.waitForText(/View\s+Navigate\s+Theme\s+Agent\s+Help/, { + timeout: 15_000, + }); + + expect(initial).toContain("line01 = 101"); + expect(initial).not.toContain("line08 = 108"); + + // Live coverage of the matcher's bare-character dispatch through OpenTUI + // for the pageDown / pageUp pair. The `` precedence over + // `` is pinned in match.test.ts; tuistory's PTY encoder drops the + // shift modifier on space, so it can't be exercised end-to-end here. + await session.waitIdle({ timeout: 200 }); + await session.press("space"); + const paged = await harness.waitForSnapshot( + session, + (text) => !text.includes("line01 = 101"), + 5_000, + ); + + expect(paged).not.toContain("line01 = 101"); + + await session.press("b"); + const restored = await harness.waitForSnapshot( + session, + (text) => text.includes("line01 = 101"), + 5_000, + ); + + expect(restored).toContain("line01 = 101"); + } finally { + session.close(); + } + }); }); From 5feede4db7f33ca5c937a28aa1b37d2bfd9c503f Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Sun, 10 May 2026 00:47:10 -0500 Subject: [PATCH 04/10] fix(keymap): route pager sidebar toggle through the keymap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upstream's pager `s` → toggleSidebar from #216 survived the rebase as a hardcoded `key.name === "s"` block at the tail of handlePagerShortcut. Add a `sidebar.toggle` action to the pager scope and replace the hardcoded check with `isAction("pager", "sidebar.toggle", key)` so the binding is discoverable, rebindable, and consistent with every other pager-scope shortcut. --- docs/keybindings.md | 1 + src/core/keymap/actions.ts | 7 +++++++ src/ui/hooks/useAppKeyboardShortcuts.ts | 2 +- 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/docs/keybindings.md b/docs/keybindings.md index 6075134d..9a1b1793 100644 --- a/docs/keybindings.md +++ b/docs/keybindings.md @@ -124,6 +124,7 @@ section to override pager-only. | `scroll.codeLeftFast` | `` | Scroll code left (fast). | | `scroll.codeRightFast` | `` | Scroll code right (fast). | | `wrap.toggle` | `w` | Toggle line wrap. | +| `sidebar.toggle` | `s` | Toggle sidebar. | ### `[keybindings.menu]` diff --git a/src/core/keymap/actions.ts b/src/core/keymap/actions.ts index 739df95a..37f1008a 100644 --- a/src/core/keymap/actions.ts +++ b/src/core/keymap/actions.ts @@ -374,6 +374,13 @@ export const ACTIONS: readonly ActionDef[] = [ description: "toggle line wrap", group: "Pager", }, + { + id: "sidebar.toggle", + scope: "pager", + defaultKeys: ["s"], + description: "toggle sidebar", + group: "Pager", + }, // ---------- menu scope ---------- { diff --git a/src/ui/hooks/useAppKeyboardShortcuts.ts b/src/ui/hooks/useAppKeyboardShortcuts.ts index d0837ca4..347349bc 100644 --- a/src/ui/hooks/useAppKeyboardShortcuts.ts +++ b/src/ui/hooks/useAppKeyboardShortcuts.ts @@ -186,7 +186,7 @@ export function useAppKeyboardShortcuts({ return; } - if (key.name === "s" || key.sequence === "s") { + if (isAction("pager", "sidebar.toggle", key)) { toggleSidebar(); } }; From 0527a36091453578149279900c9596534a7978ae Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Sun, 10 May 2026 01:22:47 -0500 Subject: [PATCH 05/10] test(keymap): close review-pass coverage gaps Seven follow-ups from a six-agent review pass: - assert all three loader stderr warning paths fire with the right context (`invalid binding shape`, `unknown action id`, non-object TOML root) - harden the PTY `q quits help` test by probing that `?` does not reopen help after quit, proving the app is dead - document `override replaces` semantics in docs/keybindings.md - hook coverage for theme.cycle (`t`), scroll.toTop/toBottom (Home/End), annotatedHunk.next (`}`) - pin findActionForKey's "first registered action wins" contract for cross-action collisions - pin that an overridden keymap actually flows through to the hook - pin that a `` action does not fire from a live press --- docs/keybindings.md | 5 + src/core/config.test.ts | 41 ++++ src/core/keymap/load.test.ts | 39 ++++ src/core/keymap/match.test.ts | 26 ++- src/ui/AppHost.interactions.test.tsx | 278 +++++++++++++++++++++++++++ test/pty/ui-integration.test.ts | 15 ++ 6 files changed, 403 insertions(+), 1 deletion(-) diff --git a/docs/keybindings.md b/docs/keybindings.md index 9a1b1793..70521a67 100644 --- a/docs/keybindings.md +++ b/docs/keybindings.md @@ -19,6 +19,11 @@ Each scope is configured under its own TOML section, e.g. `[keybindings.global]`. Missing keys keep their defaults; v1 has no "clear all" switch. +Overrides REPLACE the default key list for an action — they do not merge. +For example, if you set `[keybindings.global] quit = ""`, then `q` +no longer quits hunk; only Ctrl+Q does. To keep the default key alongside +your custom one, list both explicitly: `quit = ["q", ""]`. + ## Token syntax Bindings accept either a single string or an array of strings: diff --git a/src/core/config.test.ts b/src/core/config.test.ts index d7ec918c..28c63450 100644 --- a/src/core/config.test.ts +++ b/src/core/config.test.ts @@ -376,6 +376,47 @@ describe("config resolution", () => { expect(quit[0]?.sequence).toBe("y"); }); + test("warns when a config file's root is not a TOML object", () => { + const home = createTempDir("hunk-config-home-"); + const cwd = createTempDir("hunk-config-cwd-"); + + // The defensive branch at config.ts:147 catches a non-record root from + // Bun.TOML.parse. Real TOML files always parse to a table, so this is + // exercised by stubbing the parser to return an array — which is the + // shape `isRecord` rejects via its `Array.isArray` guard. + mkdirSync(join(home, ".config", "hunk"), { recursive: true }); + writeFileSync(join(home, ".config", "hunk", "config.toml"), "foo = 1\n"); + + const originalParse = Bun.TOML.parse; + Bun.TOML.parse = ((_input: string) => [1, 2, 3]) as typeof Bun.TOML.parse; + + const originalWrite = process.stderr.write.bind(process.stderr); + const captured: string[] = []; + process.stderr.write = ((chunk: unknown) => { + captured.push(typeof chunk === "string" ? chunk : String(chunk)); + return true; + }) as typeof process.stderr.write; + + try { + const resolved = resolveConfiguredCliInput( + { + kind: "patch", + file: "-", + options: { pager: false }, + }, + { cwd, env: { HOME: home } }, + ); + + expect(captured.some((line) => line.includes("not a TOML object"))).toBe(true); + // Defaults still apply despite the bad root shape. + expect(resolved.input.options.theme).toBe("graphite"); + expect(resolved.input.options.keymap?.global.quit?.[0]?.sequence).toBe("q"); + } finally { + process.stderr.write = originalWrite; + Bun.TOML.parse = originalParse; + } + }); + test("malformed TOML config does not abort startup", async () => { const home = createTempDir("hunk-config-home-"); const cwd = createTempDir("hunk-config-cwd-"); diff --git a/src/core/keymap/load.test.ts b/src/core/keymap/load.test.ts index ab8fdc3b..4e976018 100644 --- a/src/core/keymap/load.test.ts +++ b/src/core/keymap/load.test.ts @@ -112,6 +112,45 @@ describe("applyKeymapOverrides", () => { expect(next.global.quit).toEqual(base.global.quit); }); + test("warns when an unknown action id is overridden", async () => { + // Companion to the "ignores unknown action ids" test above. That one + // pins the silent fallback behavior; this one pins that the warning + // actually emits with the fully-qualified `.` so users can + // grep their stderr to find a typo. + const base = loadKeymapDefaults(); + const stderr = await captureStderr(() => { + applyKeymapOverrides(base, { + keybindings: { + global: { + "this.does.not.exist": "x", + }, + }, + }); + }); + expect( + stderr.some((line) => + line.includes('unknown action "global.this.does.not.exist"'), + ), + ).toBe(true); + }); + + test("warns when a binding is neither string nor array", async () => { + const base = loadKeymapDefaults(); + let next!: ReturnType; + const stderr = await captureStderr(() => { + next = applyKeymapOverrides(base, { + keybindings: { global: { quit: 5 as unknown as string } }, + }); + }); + expect( + stderr.some( + (line) => + line.includes("invalid binding") && line.includes('"global.quit"'), + ), + ).toBe(true); + expect(next.global.quit).toEqual(base.global.quit); + }); + test("does not mutate the input keymap", () => { const base = loadKeymapDefaults(); const beforeQuit = base.global.quit; diff --git a/src/core/keymap/match.test.ts b/src/core/keymap/match.test.ts index 6400d1d1..20f87bc4 100644 --- a/src/core/keymap/match.test.ts +++ b/src/core/keymap/match.test.ts @@ -1,7 +1,7 @@ import { describe, expect, test } from "bun:test"; import type { KeyEvent } from "@opentui/core"; import { findActionForKey, matchesAction, matchesKey } from "./match"; -import { loadKeymapDefaults } from "./load"; +import { applyKeymapOverrides, loadKeymapDefaults } from "./load"; import { parseKeyToken } from "./parse"; function makeKey(overrides: Partial): KeyEvent { @@ -122,6 +122,30 @@ describe("findActionForKey", () => { const noMatch = makeKey({ name: "x", sequence: "x" }); expect(findActionForKey(keymap, "global", noMatch)).toBeNull(); }); + + test("first registered action wins when two actions bind the same key", () => { + // Contract: when a key collision exists, `findActionForKey` returns + // whichever action appears first when iterating `Object.entries(scopeMap)`. + // That iteration order matches the insertion order in `loadKeymapDefaults`, + // which walks ACTIONS in registry order. `quit` is registered before + // `help.toggle` in `actions.ts`, so a colliding `x` resolves to `quit`. + // + // If a future reorder of `ACTIONS` changes the relative position of two + // colliding actions, this test will flip — that's the intended forcing + // function so the precedence change is a deliberate decision rather than + // a silent regression. + const base = loadKeymapDefaults(); + const next = applyKeymapOverrides(base, { + keybindings: { + global: { + quit: "x", + "help.toggle": "x", + }, + }, + }); + const xKey = makeKey({ name: "x", sequence: "x" }); + expect(findActionForKey(next, "global", xKey)).toBe("quit"); + }); }); /** diff --git a/src/ui/AppHost.interactions.test.tsx b/src/ui/AppHost.interactions.test.tsx index 3d6773f1..a0683d97 100644 --- a/src/ui/AppHost.interactions.test.tsx +++ b/src/ui/AppHost.interactions.test.tsx @@ -12,6 +12,7 @@ import type { HunkSessionSnapshot, } from "../hunk-session/types"; import type { AppBootstrap, LayoutMode } from "../core/types"; +import { applyKeymapOverrides, loadKeymapDefaults } from "../core/keymap/load"; import { createTestVcsAppBootstrap } from "../../test/helpers/app-bootstrap"; import { createTestDiffFile as buildTestDiffFile, lines } from "../../test/helpers/diff-helpers"; @@ -2422,4 +2423,281 @@ describe("App interactions", () => { }); } }); + + test("t cycles to the next theme", async () => { + // createBootstrap() goes through createTestVcsAppBootstrap, whose default + // initialTheme is "midnight". Cycling once should advance to the next + // theme in the THEMES registry, which is "paper". We observe this by + // walking through the menu bar to the Theme dropdown — the entry whose + // label starts with "[x] " is the active theme. + const setup = await testRender(, { + width: 220, + height: 24, + }); + + try { + await flush(setup); + + await act(async () => { + await setup.mockInput.typeText("t"); + }); + await flush(setup); + + // Open menu bar, then arrow-right three times: File -> View -> Navigate -> Theme. + await act(async () => { + await setup.mockInput.pressKey("F10"); + }); + await waitForFrame(setup, (currentFrame) => + currentFrame.includes("Toggle files/filter focus"), + ); + for (let index = 0; index < 3; index += 1) { + await act(async () => { + await setup.mockInput.pressArrow("right"); + }); + await flush(setup); + } + + const frame = await waitForFrame(setup, (currentFrame) => + currentFrame.includes("[x] Paper"), + ); + expect(frame).toContain("[x] Paper"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("Home jumps the review stream to the top", async () => { + const setup = await testRender(, { + width: 220, + height: 12, + }); + + try { + await flush(setup); + + // Walk down a few lines so the top-of-stream marker scrolls off frame. + let scrolled = setup.captureCharFrame(); + for (let index = 0; index < 18; index += 1) { + await act(async () => { + await setup.mockInput.pressArrow("down"); + }); + await flush(setup); + scrolled = setup.captureCharFrame(); + if (!scrolled.includes("line01 = 101")) break; + } + expect(scrolled).not.toContain("line01 = 101"); + + await act(async () => { + await setup.mockInput.pressKey("HOME"); + }); + await flush(setup); + + const frame = await waitForFrame(setup, (currentFrame) => + currentFrame.includes("line01 = 101"), + ); + expect(frame).toContain("line01 = 101"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("End jumps the review stream to the bottom", async () => { + const setup = await testRender(, { + width: 220, + height: 12, + }); + + try { + await flush(setup); + + const initial = setup.captureCharFrame(); + expect(initial).toContain("line01 = 101"); + // The fixture is short enough that the last numbered line might also be + // visible at startup if no scrolling were applied — guard against that + // by asserting it isn't yet, before pressing End. + expect(initial).not.toContain("line18 = 118"); + + await act(async () => { + await setup.mockInput.pressKey("END"); + }); + await flush(setup); + + // The createLineScrollBootstrap fixture renders 18 numbered lines; the + // final line of the diff must be visible after End fires. + const frame = await waitForFrame(setup, (currentFrame) => + currentFrame.includes("line18 = 118"), + ); + expect(frame).toContain("line18 = 118"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("} jumps to the next annotated hunk in the review stream", async () => { + const { hostClient, getLatestSnapshot } = createMockHostClient(); + const setup = await testRender( + , + { width: 104, height: 18 }, + ); + + try { + await flush(setup); + + // Initial render parks the selection on hunk 0 of the only file. The + // sidecar's only annotation lives on hunk 1, so a single `}` press + // should move selection there and reveal the inline note. + const initialSnapshot = getLatestSnapshot(); + expect(initialSnapshot?.selectedHunkIndex).toBe(0); + + await act(async () => { + await setup.mockInput.typeText("}"); + }); + await flush(setup); + + const advanced = await waitForSnapshot( + setup, + getLatestSnapshot, + (snapshot) => snapshot.selectedHunkIndex === 1, + ); + expect(advanced?.selectedHunkIndex).toBe(1); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("a TOML-overridden quit binding flows through to the keyboard hook", async () => { + // Pin the end-to-end path: a keymap built from defaults + override is + // delivered through bootstrap.input.options.keymap, picked up inside + // App.tsx's useMemo, and ends up inside useAppKeyboardShortcuts's + // matchesAction lookup. With `quit` rebound to `x`, pressing `x` must + // fire onQuit and pressing `q` must NOT. + const keymap = applyKeymapOverrides(loadKeymapDefaults(), { + keybindings: { global: { quit: "x" } }, + }); + const onQuit = mock(() => undefined); + const bootstrap = createTestVcsAppBootstrap({ + changesetId: "changeset:keymap-override", + files: [ + createTestDiffFile( + "alpha", + "alpha.ts", + "export const alpha = 1;\n", + "export const alpha = 2;\n", + ), + ], + vcsOptions: { keymap }, + }); + + const setup = await testRender( + , + { width: 220, height: 24 }, + ); + + try { + await flush(setup); + + await act(async () => { + await setup.mockInput.typeText("q"); + }); + await flush(setup); + expect(onQuit).not.toHaveBeenCalled(); + + await act(async () => { + await setup.mockInput.typeText("x"); + }); + await flush(setup); + expect(onQuit).toHaveBeenCalledTimes(1); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("a action does not fire from a live press", async () => { + // Pin the unbind path: `` clears the spec list for an action, + // and the hook treats an empty spec list as "no match". `s` should be + // inert with this keymap, but unrelated actions like `t` (theme.cycle) + // must still fire — proving only the disabled action is dead. + const keymap = applyKeymapOverrides(loadKeymapDefaults(), { + keybindings: { global: { "sidebar.toggle": "" } }, + }); + const bootstrap = createTestVcsAppBootstrap({ + changesetId: "changeset:keymap-disabled", + files: [ + createTestDiffFile( + "alpha", + "alpha.ts", + "export const alpha = 1;\n", + "export const alpha = 2;\n", + ), + createTestDiffFile( + "beta", + "beta.ts", + "export const beta = 1;\n", + "export const betaValue = 1;\n", + ), + ], + vcsOptions: { keymap }, + }); + + const setup = await testRender(, { + width: 240, + height: 24, + }); + + try { + await flush(setup); + + // Sidebar starts visible — both files appear twice (sidebar + main pane). + let frame = setup.captureCharFrame(); + expect((frame.match(/alpha\.ts/g) ?? []).length).toBe(2); + + await act(async () => { + await setup.mockInput.typeText("s"); + }); + await flush(setup); + + // With sidebar.toggle unbound, `s` must NOT collapse the sidebar. + frame = setup.captureCharFrame(); + expect((frame.match(/alpha\.ts/g) ?? []).length).toBe(2); + + // Sibling check: an unrelated key (t) must still cycle theme. We + // confirm that by walking to the Theme menu and seeing that the + // checked entry has advanced past the default "midnight". + await act(async () => { + await setup.mockInput.typeText("t"); + }); + await flush(setup); + + await act(async () => { + await setup.mockInput.pressKey("F10"); + }); + await waitForFrame(setup, (currentFrame) => + currentFrame.includes("Toggle files/filter focus"), + ); + for (let index = 0; index < 3; index += 1) { + await act(async () => { + await setup.mockInput.pressArrow("right"); + }); + await flush(setup); + } + const themeFrame = await waitForFrame(setup, (currentFrame) => + currentFrame.includes("[x] Paper"), + ); + expect(themeFrame).toContain("[x] Paper"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); }); diff --git a/test/pty/ui-integration.test.ts b/test/pty/ui-integration.test.ts index cad58957..883bc26c 100644 --- a/test/pty/ui-integration.test.ts +++ b/test/pty/ui-integration.test.ts @@ -1286,6 +1286,21 @@ describe("live UI integration", () => { 5_000, ); expect(after).not.toContain("View Navigate Theme Agent Help"); + expect(after).not.toMatch(/Controls help|Keyboard help/); + + // Probe: if hunk were still alive, `?` would reopen the help dialog. + // After a real quit the PTY child is gone, so the probe write either + // does nothing or fires after process exit; either way, no help + // dialog should be drawn. + try { + await session.press("?"); + await session.waitIdle({ timeout: 300 }); + } catch { + // PTY can be torn down by the time the probe lands; that's the + // exact "process is dead" signal we want, so swallow and continue. + } + const probe = await session.text({ immediate: true }); + expect(probe).not.toMatch(/Controls help|Keyboard help/); } finally { session.close(); } From 7f07630c524fb4d18bb1334a1e965f8360828f5c Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Sun, 10 May 2026 02:00:26 -0500 Subject: [PATCH 06/10] fix(keymap): don't let lowercase-letter spec shadow uppercase on Shift+letter OpenTUI lowercases `key.name` and sets `key.shift = true` for shifted letters (e.g. Shift+G arrives as `{name:"g", sequence:"G", shift:true}`). The bare-character parity check accepted `key.name === spec.sequence`, which let a lowercase `g` binding match Shift+G via name-parity, ahead of an explicit `G` binding. This made `["g", "G"]` for top/bottom indistinguishable: `G` always fired the action bound to lowercase `g`. Gate the parity fallback on `!key.shift` so single-character specs distinguish case for shifted letters. Plain unshifted letters keep the defensive name/sequence parity that fix(keymap): address review findings introduced. --- src/core/keymap/match.test.ts | 22 ++++++++++++++++++++++ src/core/keymap/match.ts | 15 ++++++++++----- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/core/keymap/match.test.ts b/src/core/keymap/match.test.ts index 20f87bc4..f43db683 100644 --- a/src/core/keymap/match.test.ts +++ b/src/core/keymap/match.test.ts @@ -88,6 +88,28 @@ describe("matchesKey", () => { expect(matchesKey(spec, makeKey({ name: "x" }))).toBe(false); }); + test("lowercase-letter spec does not shadow uppercase spec on Shift+letter", () => { + // OpenTUI lowercases `key.name` and sets `key.shift = true` for shifted + // letters; the literal `key.sequence` stays uppercase. The name-parity + // fallback must NOT fire for shifted-letter events, otherwise an + // uppercase binding (e.g. `G` for scroll.toBottom) is shadowed by the + // lowercase one (`g` for scroll.toTop) since both `g` and `G` would + // otherwise match the same Shift+G keypress. + const lowerG = parseKeyToken("g"); + const upperG = parseKeyToken("G"); + if (!lowerG || lowerG === "disabled") throw new Error("bad fixture"); + if (!upperG || upperG === "disabled") throw new Error("bad fixture"); + + const shiftG = makeKey({ name: "g", sequence: "G", shift: true }); + expect(matchesKey(upperG, shiftG)).toBe(true); + expect(matchesKey(lowerG, shiftG)).toBe(false); + + // Plain g still matches lowercase spec (sequence-direct path). + const plainG = makeKey({ name: "g", sequence: "g" }); + expect(matchesKey(lowerG, plainG)).toBe(true); + expect(matchesKey(upperG, plainG)).toBe(false); + }); + test("modifier-required specs do not match plain events", () => { // The reverse direction of the modifier rule: spec asks for shift/ctrl/alt, // event is missing it, so the match must fail. These are cheap pins on the diff --git a/src/core/keymap/match.ts b/src/core/keymap/match.ts index db1714ac..a18df977 100644 --- a/src/core/keymap/match.ts +++ b/src/core/keymap/match.ts @@ -25,11 +25,16 @@ export type Keymap = Record>>; export function matchesKey(spec: KeySpec, key: KeyEvent): boolean { if (spec.sequence !== undefined) { if (key.sequence === spec.sequence) return true; - // Defensive parity with the pre-keymap hook, which accepted both - // `key.name === "X"` and `key.sequence === "X"` for single-character keys. - // Some OpenTUI input paths populate `name` without `sequence` for bare - // printables; matching either keeps those events binding correctly. - if (spec.sequence.length === 1 && key.name === spec.sequence) return true; + // Defensive parity for single-character specs — some OpenTUI paths + // populate `key.name` without `key.sequence`. Gate on `!key.shift` so a + // lowercase-letter spec doesn't shadow an uppercase-letter spec on + // Shift+letter events: OpenTUI lowercases `key.name` and sets + // `key.shift = true` for `A-Z`, which would otherwise let `g` match + // Shift+G (the literal `key.sequence === "G"`) ahead of an explicit `G` + // binding. + if (spec.sequence.length === 1 && !key.shift && key.name === spec.sequence) { + return true; + } return false; } From 7e97658b9bac056a17d724339477b9225c97ba8b Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Sun, 10 May 2026 08:56:04 -0500 Subject: [PATCH 07/10] chore(keymap): apply oxfmt formatting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's format:check exited 1 because the keymap commits weren't run through oxfmt locally. No semantic changes — `bun run format` over the seven affected files produced this diff. --- docs/keybindings.md | 172 +++++++++++++-------------- src/core/config.test.ts | 9 +- src/core/config.ts | 10 +- src/core/keymap/load.test.ts | 21 ++-- src/core/keymap/load.ts | 12 +- src/ui/AppHost.interactions.test.tsx | 28 ++--- src/ui/lib/ui-lib.test.ts | 57 +++++++-- 7 files changed, 157 insertions(+), 152 deletions(-) diff --git a/docs/keybindings.md b/docs/keybindings.md index 70521a67..cacb0f61 100644 --- a/docs/keybindings.md +++ b/docs/keybindings.md @@ -8,12 +8,12 @@ flags do not affect the keymap. Bindings are organized into four scopes that mirror the modes the app can be in at any moment: -| Scope | Active when | -| -------- | ------------------------------------------------- | -| `global` | The main review UI is focused. | -| `pager` | Hunk is invoked in pager mode (`hunk pager`). | -| `menu` | A menu (View, Theme, Agent, ...) is open. | -| `filter` | The file-filter input is focused. | +| Scope | Active when | +| -------- | --------------------------------------------- | +| `global` | The main review UI is focused. | +| `pager` | Hunk is invoked in pager mode (`hunk pager`). | +| `menu` | A menu (View, Theme, Agent, ...) is open. | +| `filter` | The file-filter input is focused. | Each scope is configured under its own TOML section, e.g. `[keybindings.global]`. Missing keys keep their defaults; v1 has no "clear all" @@ -36,25 +36,25 @@ quit = "q" Tokens use angle brackets for special keys and modifiers, mirroring lazygit: -| Token | Meaning | -| ---------------- | ---------------------------------------------------- | -| `q`, `?`, `[` | Bare printable characters (literal sequence match). | -| `` | Escape key. | -| `` | Tab key. | -| `` | Spacebar. | -| `` / `` | Enter (both spellings are equivalent). | -| `` | Backspace. | -| `` / `` / `` / `` | Arrow keys. | -| `` / `` | Home and End. | -| `` / `` | Page Up / Page Down. | -| ``–`` | Function keys. | -| `` | Ctrl+C. | -| `` | Shift+Up. | -| `` | Shift+Space — modifiers stack on any named key, not just arrows. | -| `` | Ctrl+PageDown — same idea with a different modifier. | -| ``, `` | Alt+X / Meta+X. | -| `` | Modifiers stack in any order. | -| `` | Sentinel that unbinds the action. | +| Token | Meaning | +| ---------------------------------------- | ---------------------------------------------------------------- | +| `q`, `?`, `[` | Bare printable characters (literal sequence match). | +| `` | Escape key. | +| `` | Tab key. | +| `` | Spacebar. | +| `` / `` | Enter (both spellings are equivalent). | +| `` | Backspace. | +| `` / `` / `` / `` | Arrow keys. | +| `` / `` | Home and End. | +| `` / `` | Page Up / Page Down. | +| ``–`` | Function keys. | +| `` | Ctrl+C. | +| `` | Shift+Up. | +| `` | Shift+Space — modifiers stack on any named key, not just arrows. | +| `` | Ctrl+PageDown — same idea with a different modifier. | +| ``, `` | Alt+X / Meta+X. | +| `` | Modifiers stack in any order. | +| `` | Sentinel that unbinds the action. | Multi-key sequences like vim's `gg` are not supported in v1 — every binding is a single chord. @@ -73,39 +73,39 @@ ignored. They never abort startup. ### `[keybindings.global]` -| Action | Default keys | What it does | -| -------------------- | ------------------------- | ----------------------------- | -| `scroll.lineDown` | `j`, `` | Move line-by-line (down). | -| `scroll.lineUp` | `k`, `` | Move line-by-line (up). | -| `scroll.pageDown` | ``, `f`, `` | Page down. | -| `scroll.pageUp` | `b`, ``, `` | Page up. | -| `scroll.halfPageDown` | `d` | Half page down. | -| `scroll.halfPageUp` | `u` | Half page up. | -| `scroll.toTop` | `` | Jump to top of stream. | -| `scroll.toBottom` | `` | Jump to bottom of stream. | -| `scroll.codeLeft` | `` | Scroll code left one column. | -| `scroll.codeRight` | `` | Scroll code right one column. | -| `scroll.codeLeftFast` | `` | Scroll code left (fast). | -| `scroll.codeRightFast` | `` | Scroll code right (fast). | -| `hunk.prev` | `[` | Previous hunk. | -| `hunk.next` | `]` | Next hunk. | -| `annotatedHunk.prev` | `{` | Previous comment hunk. | -| `annotatedHunk.next` | `}` | Next comment hunk. | -| `layout.split` | `1` | Split layout. | -| `layout.stack` | `2` | Stack layout. | -| `layout.auto` | `0` | Auto layout. | -| `sidebar.toggle` | `s` | Toggle sidebar. | -| `theme.cycle` | `t` | Cycle theme. | -| `agentNotes.toggle` | `a` | Toggle agent notes. | -| `lineNumbers.toggle` | `l` | Toggle line numbers. | -| `wrap.toggle` | `w` | Toggle line wrap. | -| `hunkHeaders.toggle` | `m` | Toggle hunk metadata headers. | -| `quit` | `q`, `` | Quit. | -| `help.toggle` | `?` | Toggle help. | -| `filter.focus` | `/` | Focus the file filter. | -| `focus.toggle` | `` | Toggle files/filter focus. | -| `reload` | `r` | Reload current input. | -| `menu.open` | `` | Open the menus. | +| Action | Default keys | What it does | +| ---------------------- | -------------------------- | ----------------------------- | +| `scroll.lineDown` | `j`, `` | Move line-by-line (down). | +| `scroll.lineUp` | `k`, `` | Move line-by-line (up). | +| `scroll.pageDown` | ``, `f`, `` | Page down. | +| `scroll.pageUp` | `b`, ``, `` | Page up. | +| `scroll.halfPageDown` | `d` | Half page down. | +| `scroll.halfPageUp` | `u` | Half page up. | +| `scroll.toTop` | `` | Jump to top of stream. | +| `scroll.toBottom` | `` | Jump to bottom of stream. | +| `scroll.codeLeft` | `` | Scroll code left one column. | +| `scroll.codeRight` | `` | Scroll code right one column. | +| `scroll.codeLeftFast` | `` | Scroll code left (fast). | +| `scroll.codeRightFast` | `` | Scroll code right (fast). | +| `hunk.prev` | `[` | Previous hunk. | +| `hunk.next` | `]` | Next hunk. | +| `annotatedHunk.prev` | `{` | Previous comment hunk. | +| `annotatedHunk.next` | `}` | Next comment hunk. | +| `layout.split` | `1` | Split layout. | +| `layout.stack` | `2` | Stack layout. | +| `layout.auto` | `0` | Auto layout. | +| `sidebar.toggle` | `s` | Toggle sidebar. | +| `theme.cycle` | `t` | Cycle theme. | +| `agentNotes.toggle` | `a` | Toggle agent notes. | +| `lineNumbers.toggle` | `l` | Toggle line numbers. | +| `wrap.toggle` | `w` | Toggle line wrap. | +| `hunkHeaders.toggle` | `m` | Toggle hunk metadata headers. | +| `quit` | `q`, `` | Quit. | +| `help.toggle` | `?` | Toggle help. | +| `filter.focus` | `/` | Focus the file filter. | +| `focus.toggle` | `` | Toggle files/filter focus. | +| `reload` | `r` | Reload current input. | +| `menu.open` | `` | Open the menus. | ### `[keybindings.pager]` @@ -113,40 +113,40 @@ The pager scope mirrors the global scroll/wrap actions for `hunk pager`. Defaults match the corresponding global actions; rebind them under this section to override pager-only. -| Action | Default keys | What it does | -| ---------------------- | --------------------------- | ----------------------------- | -| `quit` | `q`, `` | Quit. | -| `scroll.lineDown` | `j`, `` | Scroll one line down. | -| `scroll.lineUp` | `k`, `` | Scroll one line up. | -| `scroll.pageDown` | ``, `f`, `` | Page down. | -| `scroll.pageUp` | `b`, ``, `` | Page up. | -| `scroll.halfPageDown` | `d` | Half page down. | -| `scroll.halfPageUp` | `u` | Half page up. | -| `scroll.toTop` | `` | Jump to top. | -| `scroll.toBottom` | `` | Jump to bottom. | -| `scroll.codeLeft` | `` | Scroll code left one column. | -| `scroll.codeRight` | `` | Scroll code right one column. | -| `scroll.codeLeftFast` | `` | Scroll code left (fast). | -| `scroll.codeRightFast` | `` | Scroll code right (fast). | -| `wrap.toggle` | `w` | Toggle line wrap. | -| `sidebar.toggle` | `s` | Toggle sidebar. | +| Action | Default keys | What it does | +| ---------------------- | -------------------------- | ----------------------------- | +| `quit` | `q`, `` | Quit. | +| `scroll.lineDown` | `j`, `` | Scroll one line down. | +| `scroll.lineUp` | `k`, `` | Scroll one line up. | +| `scroll.pageDown` | ``, `f`, `` | Page down. | +| `scroll.pageUp` | `b`, ``, `` | Page up. | +| `scroll.halfPageDown` | `d` | Half page down. | +| `scroll.halfPageUp` | `u` | Half page up. | +| `scroll.toTop` | `` | Jump to top. | +| `scroll.toBottom` | `` | Jump to bottom. | +| `scroll.codeLeft` | `` | Scroll code left one column. | +| `scroll.codeRight` | `` | Scroll code right one column. | +| `scroll.codeLeftFast` | `` | Scroll code left (fast). | +| `scroll.codeRightFast` | `` | Scroll code right (fast). | +| `wrap.toggle` | `w` | Toggle line wrap. | +| `sidebar.toggle` | `s` | Toggle sidebar. | ### `[keybindings.menu]` -| Action | Default keys | What it does | -| --------------- | ------------------------ | --------------------------- | -| `menu.close` | `` | Close the menu. | -| `menu.prev` | `` | Previous menu. | -| `menu.next` | ``, `` | Next menu. | -| `menu.itemUp` | `` | Previous item. | -| `menu.itemDown` | `` | Next item. | -| `menu.activate` | ``, `` | Activate current item. | +| Action | Default keys | What it does | +| --------------- | --------------------- | ---------------------- | +| `menu.close` | `` | Close the menu. | +| `menu.prev` | `` | Previous menu. | +| `menu.next` | ``, `` | Next menu. | +| `menu.itemUp` | `` | Previous item. | +| `menu.itemDown` | `` | Next item. | +| `menu.activate` | ``, `` | Activate current item. | ### `[keybindings.filter]` -| Action | Default keys | What it does | -| -------------- | ------------ | -------------------------- | -| `focus.toggle` | `` | Leave the filter input. | +| Action | Default keys | What it does | +| -------------- | ------------ | ----------------------- | +| `focus.toggle` | `` | Leave the filter input. | Esc and Enter inside the filter input are owned by the input element itself (Esc clears, then closes; Enter submits) and are not configurable in v1. diff --git a/src/core/config.test.ts b/src/core/config.test.ts index 28c63450..1cb05722 100644 --- a/src/core/config.test.ts +++ b/src/core/config.test.ts @@ -314,12 +314,9 @@ describe("config resolution", () => { mkdirSync(join(repo, ".hunk"), { recursive: true }); writeFileSync( join(repo, ".hunk", "config.toml"), - [ - "[keybindings.global]", - 'quit = ["", "x"]', - '"sidebar.toggle" = ""', - "", - ].join("\n"), + ["[keybindings.global]", 'quit = ["", "x"]', '"sidebar.toggle" = ""', ""].join( + "\n", + ), ); const resolved = resolveConfiguredCliInput( diff --git a/src/core/config.ts b/src/core/config.ts index f97cd245..4529c2b5 100644 --- a/src/core/config.ts +++ b/src/core/config.ts @@ -188,18 +188,12 @@ export function resolveConfiguredCliInput( const repoTomlRoot = repoConfigPath ? readTomlRecord(repoConfigPath) : undefined; if (userConfigPath && userTomlRoot) { - resolvedOptions = mergeOptions( - resolvedOptions, - resolveConfigLayer(userTomlRoot, input), - ); + resolvedOptions = mergeOptions(resolvedOptions, resolveConfigLayer(userTomlRoot, input)); keymap = applyKeymapOverrides(keymap, userTomlRoot); } if (repoConfigPath && repoTomlRoot) { - resolvedOptions = mergeOptions( - resolvedOptions, - resolveConfigLayer(repoTomlRoot, input), - ); + resolvedOptions = mergeOptions(resolvedOptions, resolveConfigLayer(repoTomlRoot, input)); keymap = applyKeymapOverrides(keymap, repoTomlRoot); } diff --git a/src/core/keymap/load.test.ts b/src/core/keymap/load.test.ts index 4e976018..8399ae6c 100644 --- a/src/core/keymap/load.test.ts +++ b/src/core/keymap/load.test.ts @@ -94,9 +94,7 @@ describe("applyKeymapOverrides", () => { }); expect(next.global.quit).toHaveLength(2); expect(matchesAction(next, "global", "quit", makeKey({ name: "x", sequence: "x" }))).toBe(true); - expect( - matchesAction(next, "global", "quit", makeKey({ name: "c", ctrl: true })), - ).toBe(true); + expect(matchesAction(next, "global", "quit", makeKey({ name: "c", ctrl: true }))).toBe(true); }); test("ignores unknown action ids without throwing", () => { @@ -128,9 +126,7 @@ describe("applyKeymapOverrides", () => { }); }); expect( - stderr.some((line) => - line.includes('unknown action "global.this.does.not.exist"'), - ), + stderr.some((line) => line.includes('unknown action "global.this.does.not.exist"')), ).toBe(true); }); @@ -143,10 +139,7 @@ describe("applyKeymapOverrides", () => { }); }); expect( - stderr.some( - (line) => - line.includes("invalid binding") && line.includes('"global.quit"'), - ), + stderr.some((line) => line.includes("invalid binding") && line.includes('"global.quit"')), ).toBe(true); expect(next.global.quit).toEqual(base.global.quit); }); @@ -171,7 +164,9 @@ describe("applyKeymapOverrides", () => { // Empty array should be ignored, defaults preserved. expect(next.global.quit).toEqual(base.global.quit); - expect(stderr.some((line) => line.includes("empty binding") && line.includes("global.quit"))).toBe(true); + expect( + stderr.some((line) => line.includes("empty binding") && line.includes("global.quit")), + ).toBe(true); }); test("warns on unknown scope but keeps walking known scopes", async () => { @@ -237,9 +232,7 @@ describe("applyKeymapOverrides", () => { expect( stderr.some( (line) => - line.includes("global.quit") && - line.includes("") && - line.includes("mixes"), + line.includes("global.quit") && line.includes("") && line.includes("mixes"), ), ).toBe(true); // Disabled still wins — caller is warned, but the binding ends up empty. diff --git a/src/core/keymap/load.ts b/src/core/keymap/load.ts index d2816d05..2d917e8d 100644 --- a/src/core/keymap/load.ts +++ b/src/core/keymap/load.ts @@ -10,12 +10,7 @@ */ import { isRecord } from "../config"; -import { - ACTIONS, - ACTIONS_BY_SCOPE, - type ActionId, - type ActionScope, -} from "./actions"; +import { ACTIONS, ACTIONS_BY_SCOPE, type ActionId, type ActionScope } from "./actions"; import type { Keymap } from "./match"; import { parseBinding, type KeySpec } from "./parse"; @@ -67,10 +62,7 @@ function cloneKeymap(base: Keymap): Keymap { * stderr warning and are otherwise ignored — config errors should never * abort startup. */ -export function applyKeymapOverrides( - base: Keymap, - source: Record, -): Keymap { +export function applyKeymapOverrides(base: Keymap, source: Record): Keymap { const keybindings = source.keybindings; if (!isRecord(keybindings)) return base; diff --git a/src/ui/AppHost.interactions.test.tsx b/src/ui/AppHost.interactions.test.tsx index a0683d97..7b1a2581 100644 --- a/src/ui/AppHost.interactions.test.tsx +++ b/src/ui/AppHost.interactions.test.tsx @@ -2316,10 +2316,10 @@ describe("App interactions", () => { // still reach the quit handler. The companion test below confirms that Esc // (and the help toggle key) continue to close the dialog. const onQuit = mock(() => undefined); - const setup = await testRender( - , - { width: 220, height: 32 }, - ); + const setup = await testRender(, { + width: 220, + height: 32, + }); try { await flush(setup); @@ -2347,10 +2347,10 @@ describe("App interactions", () => { test("Esc closes the help dialog without quitting the app", async () => { const onQuit = mock(() => undefined); - const setup = await testRender( - , - { width: 220, height: 32 }, - ); + const setup = await testRender(, { + width: 220, + height: 32, + }); try { await flush(setup); @@ -2457,9 +2457,7 @@ describe("App interactions", () => { await flush(setup); } - const frame = await waitForFrame(setup, (currentFrame) => - currentFrame.includes("[x] Paper"), - ); + const frame = await waitForFrame(setup, (currentFrame) => currentFrame.includes("[x] Paper")); expect(frame).toContain("[x] Paper"); } finally { await act(async () => { @@ -2596,10 +2594,10 @@ describe("App interactions", () => { vcsOptions: { keymap }, }); - const setup = await testRender( - , - { width: 220, height: 24 }, - ); + const setup = await testRender(, { + width: 220, + height: 24, + }); try { await flush(setup); diff --git a/src/ui/lib/ui-lib.test.ts b/src/ui/lib/ui-lib.test.ts index 0d58dbd7..3ab03164 100644 --- a/src/ui/lib/ui-lib.test.ts +++ b/src/ui/lib/ui-lib.test.ts @@ -174,20 +174,51 @@ describe("ui helpers", () => { const keymap = loadKeymapDefaults(); expect(isEscapeKey(createKeyEvent({ name: "escape" }))).toBe(true); expect(isEscapeKey(createKeyEvent({ name: "esc" }))).toBe(true); - expect(matchesAction(keymap, "global", "scroll.pageDown", createKeyEvent({ name: "pagedown" }))).toBe(true); - expect(matchesAction(keymap, "global", "scroll.pageDown", createKeyEvent({ name: "space" }))).toBe(true); - expect(matchesAction(keymap, "global", "scroll.pageDown", createKeyEvent({ sequence: "f" }))).toBe(true); - expect(matchesAction(keymap, "global", "scroll.pageUp", createKeyEvent({ name: "pageup" }))).toBe(true); - expect(matchesAction(keymap, "global", "scroll.pageUp", createKeyEvent({ sequence: "b" }))).toBe(true); - expect(matchesAction(keymap, "global", "scroll.pageUp", createKeyEvent({ name: "space", shift: true }))).toBe(true); - expect(matchesAction(keymap, "global", "scroll.halfPageDown", createKeyEvent({ sequence: "d" }))).toBe(true); - expect(matchesAction(keymap, "global", "scroll.halfPageUp", createKeyEvent({ sequence: "u" }))).toBe(true); - expect(matchesAction(keymap, "global", "scroll.lineDown", createKeyEvent({ name: "down" }))).toBe(true); - expect(matchesAction(keymap, "global", "scroll.lineDown", createKeyEvent({ sequence: "j" }))).toBe(true); - expect(matchesAction(keymap, "global", "scroll.lineUp", createKeyEvent({ name: "up" }))).toBe(true); - expect(matchesAction(keymap, "global", "scroll.lineUp", createKeyEvent({ sequence: "k" }))).toBe(true); + expect( + matchesAction(keymap, "global", "scroll.pageDown", createKeyEvent({ name: "pagedown" })), + ).toBe(true); + expect( + matchesAction(keymap, "global", "scroll.pageDown", createKeyEvent({ name: "space" })), + ).toBe(true); + expect( + matchesAction(keymap, "global", "scroll.pageDown", createKeyEvent({ sequence: "f" })), + ).toBe(true); + expect( + matchesAction(keymap, "global", "scroll.pageUp", createKeyEvent({ name: "pageup" })), + ).toBe(true); + expect( + matchesAction(keymap, "global", "scroll.pageUp", createKeyEvent({ sequence: "b" })), + ).toBe(true); + expect( + matchesAction( + keymap, + "global", + "scroll.pageUp", + createKeyEvent({ name: "space", shift: true }), + ), + ).toBe(true); + expect( + matchesAction(keymap, "global", "scroll.halfPageDown", createKeyEvent({ sequence: "d" })), + ).toBe(true); + expect( + matchesAction(keymap, "global", "scroll.halfPageUp", createKeyEvent({ sequence: "u" })), + ).toBe(true); + expect( + matchesAction(keymap, "global", "scroll.lineDown", createKeyEvent({ name: "down" })), + ).toBe(true); + expect( + matchesAction(keymap, "global", "scroll.lineDown", createKeyEvent({ sequence: "j" })), + ).toBe(true); + expect(matchesAction(keymap, "global", "scroll.lineUp", createKeyEvent({ name: "up" }))).toBe( + true, + ); + expect( + matchesAction(keymap, "global", "scroll.lineUp", createKeyEvent({ sequence: "k" })), + ).toBe(true); expect(isEscapeKey(createKeyEvent({ name: "q" }))).toBe(false); - expect(matchesAction(keymap, "global", "scroll.pageDown", createKeyEvent({ sequence: "q" }))).toBe(false); + expect( + matchesAction(keymap, "global", "scroll.pageDown", createKeyEvent({ sequence: "q" })), + ).toBe(false); }); test("fitText and padText clamp using the terminal fallback marker", () => { From 2b2939d10874f0813b978193f73db1fc0ed559b2 Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Sun, 10 May 2026 09:01:50 -0500 Subject: [PATCH 08/10] fix(test): poll for lazy HelpDialog mount in CI-flake-prone tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two help-dialog regression tests in AppHost.interactions.test.tsx asserted on the first frame after pressing `?`. Locally the lazy-loaded HelpDialog mounts in time; on slow CI the Suspense boundary can take an extra tick, so the assertion fired before "Controls help" landed in the frame. Switch to waitForFrame so the test polls until the dialog is actually visible — same pattern the surrounding F10/menu tests use. --- src/ui/AppHost.interactions.test.tsx | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/ui/AppHost.interactions.test.tsx b/src/ui/AppHost.interactions.test.tsx index 7b1a2581..6f292818 100644 --- a/src/ui/AppHost.interactions.test.tsx +++ b/src/ui/AppHost.interactions.test.tsx @@ -2327,10 +2327,12 @@ describe("App interactions", () => { await act(async () => { await setup.mockInput.typeText("?"); }); - await flush(setup); - - const frame = setup.captureCharFrame(); - expect(frame).toContain("Controls help"); + // HelpDialog is lazy-loaded — poll instead of asserting on the first + // frame so slow CI mounts don't trip the assertion. + const helpFrame = await waitForFrame(setup, (currentFrame) => + currentFrame.includes("Controls help"), + ); + expect(helpFrame).toContain("Controls help"); await act(async () => { await setup.mockInput.typeText("q"); @@ -2358,9 +2360,10 @@ describe("App interactions", () => { await act(async () => { await setup.mockInput.typeText("?"); }); - await flush(setup); - - let frame = setup.captureCharFrame(); + // Same lazy-mount handling as the regression test above. + let frame = await waitForFrame(setup, (currentFrame) => + currentFrame.includes("Controls help"), + ); expect(frame).toContain("Controls help"); await act(async () => { From 0ac21db71b4c47d8de830dde0ee6d6da62b57cbe Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Sun, 10 May 2026 09:06:44 -0500 Subject: [PATCH 09/10] test(pty): bump Esc-closes-help timeout for slow CI runners The Esc-closes-help PTY test waits for the help dialog to vanish after pressing escape. OpenTUI's input parser can buffer a bare `\x1b` while disambiguating it from a multi-byte CSI sequence; on the modem-dev/hunk GitHub Actions Linux runner that buffer settled in just under the 5s poll window after a `?`-then-Esc roundtrip and tripped the test. Locally on a real terminal the dialog disappears in well under 100ms. Bump the harness.waitForSnapshot timeout from 5s to 10s so the test isn't a CI flake. --- test/pty/ui-integration.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/pty/ui-integration.test.ts b/test/pty/ui-integration.test.ts index 883bc26c..c36286c9 100644 --- a/test/pty/ui-integration.test.ts +++ b/test/pty/ui-integration.test.ts @@ -1323,11 +1323,15 @@ describe("live UI integration", () => { await session.press("?"); await session.waitForText(/Controls help|Keyboard help/, { timeout: 5_000 }); + // OpenTUI's input parser can buffer a bare `\x1b` to disambiguate it + // from a multi-byte escape sequence; on slower CI runners that buffer + // can take several hundred ms to settle. Give it 10s before declaring + // the Esc didn't reach the app. await session.press("escape"); const after = await harness.waitForSnapshot( session, (text) => !text.includes("Controls help") && !text.includes("Keyboard help"), - 5_000, + 10_000, ); // App must still be alive — chrome row should be redrawn. expect(after).toMatch(/View\s+Navigate\s+Theme\s+Agent\s+Help/); From 5c75c614862647d45271d9f5d76cf5ca9daeed81 Mon Sep 17 00:00:00 2001 From: Alejandro Bernal Date: Sun, 10 May 2026 09:36:19 -0500 Subject: [PATCH 10/10] fix(keymap): scope-bind ActionId types and address Greptile review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes from the PR review pass: 1. HelpDialog filter comment said "show pager entries if rebound" but the code unconditionally hides every non-global action. Reword the comment to match the actual behavior — the pager/menu/filter scopes duplicate global navigation and are hidden to keep help concise. 2. Document `findActionForKey`'s collision contract: when two actions bind the same key, it returns whichever was inserted first into the scope map (registry order). The hook avoids this by polling actions in explicit priority order; document so future callers don't re-hit it. Already pinned by `match.test.ts` "first registered wins". 3. Split the global `ActionId` union into per-scope unions (GlobalActionId, PagerActionId, MenuActionId, FilterActionId), keep `ActionId` as the union of all four, and add `ActionIdForScope`. `Keymap` is now a mapped type that only stores the scope-specific ids. `matchesAction` and `findActionForKey` are generic over the scope, so `matchesAction(km, "global", "menu.close", k)` is a compile-time error instead of a silent `false`. Add a `getActionSpecs(keymap, action)` helper for callers iterating the `ACTIONS` registry that need to read by `ActionDef`. --- src/core/keymap/actions.ts | 52 ++++++++++++++++++++++--- src/core/keymap/load.test.ts | 6 +-- src/core/keymap/load.ts | 24 ++++++++---- src/core/keymap/match.ts | 49 +++++++++++++++++------ src/ui/components/chrome/HelpDialog.tsx | 8 ++-- 5 files changed, 108 insertions(+), 31 deletions(-) diff --git a/src/core/keymap/actions.ts b/src/core/keymap/actions.ts index 37f1008a..5a8246f2 100644 --- a/src/core/keymap/actions.ts +++ b/src/core/keymap/actions.ts @@ -1,7 +1,12 @@ export type ActionScope = "global" | "pager" | "menu" | "filter"; -export type ActionId = - // global +/** + * Per-scope action id unions. Splitting `ActionId` by scope lets the + * `Keymap` type and `matchesAction`/`findActionForKey` reject mismatched + * (scope, id) pairs at compile time — `keymap.global["menu.close"]` is now a + * type error instead of a silent `undefined`. + */ +export type GlobalActionId = | "quit" | "help.toggle" | "filter.focus" @@ -32,8 +37,26 @@ export type ActionId = | "hunk.next" | "annotatedHunk.prev" | "annotatedHunk.next" - | "menu.open" - // menu + | "menu.open"; + +export type PagerActionId = + | "quit" + | "scroll.pageDown" + | "scroll.pageUp" + | "scroll.halfPageDown" + | "scroll.halfPageUp" + | "scroll.lineDown" + | "scroll.lineUp" + | "scroll.toTop" + | "scroll.toBottom" + | "scroll.codeLeft" + | "scroll.codeRight" + | "scroll.codeLeftFast" + | "scroll.codeRightFast" + | "wrap.toggle" + | "sidebar.toggle"; + +export type MenuActionId = | "menu.close" | "menu.prev" | "menu.next" @@ -41,6 +64,22 @@ export type ActionId = | "menu.itemDown" | "menu.activate"; +export type FilterActionId = "focus.toggle"; + +/** Map a scope to its specific action id union. */ +export type ActionIdForScope = S extends "global" + ? GlobalActionId + : S extends "pager" + ? PagerActionId + : S extends "menu" + ? MenuActionId + : S extends "filter" + ? FilterActionId + : never; + +/** Union of every legal action id across all scopes. */ +export type ActionId = GlobalActionId | PagerActionId | MenuActionId | FilterActionId; + export interface ActionDef { id: ActionId; scope: ActionScope; @@ -445,7 +484,10 @@ export const ACTIONS_BY_SCOPE: Record = { }; /** Look up a single (scope, id) action definition. */ -export function getAction(scope: ActionScope, id: ActionId): ActionDef | undefined { +export function getAction( + scope: S, + id: ActionIdForScope, +): ActionDef | undefined { return ACTIONS.find((action) => action.scope === scope && action.id === id); } diff --git a/src/core/keymap/load.test.ts b/src/core/keymap/load.test.ts index 8399ae6c..d59c77ae 100644 --- a/src/core/keymap/load.test.ts +++ b/src/core/keymap/load.test.ts @@ -2,7 +2,7 @@ import { describe, expect, test } from "bun:test"; import type { KeyEvent } from "@opentui/core"; import { ACTIONS } from "./actions"; import { applyKeymapOverrides, loadKeymapDefaults } from "./load"; -import { matchesAction } from "./match"; +import { getActionSpecs, matchesAction } from "./match"; /** * Replace `process.stderr.write` with an in-memory collector for the duration @@ -46,9 +46,9 @@ describe("loadKeymapDefaults", () => { test("includes every action defined in the registry", () => { const keymap = loadKeymapDefaults(); for (const action of ACTIONS) { - const specs = keymap[action.scope][action.id]; + const specs = getActionSpecs(keymap, action); expect(specs, `missing ${action.scope}.${action.id}`).toBeDefined(); - expect(specs!.length).toBeGreaterThan(0); + expect(specs.length).toBeGreaterThan(0); } }); }); diff --git a/src/core/keymap/load.ts b/src/core/keymap/load.ts index 2d917e8d..be5e01b7 100644 --- a/src/core/keymap/load.ts +++ b/src/core/keymap/load.ts @@ -10,23 +10,31 @@ */ import { isRecord } from "../config"; -import { ACTIONS, ACTIONS_BY_SCOPE, type ActionId, type ActionScope } from "./actions"; +import { ACTIONS, ACTIONS_BY_SCOPE, type ActionScope } from "./actions"; import type { Keymap } from "./match"; import { parseBinding, type KeySpec } from "./parse"; const ACTION_SCOPES = Object.keys(ACTIONS_BY_SCOPE) as ActionScope[]; -const KNOWN_IDS_BY_SCOPE: Record> = ACTION_SCOPES.reduce( +const KNOWN_IDS_BY_SCOPE: Record> = ACTION_SCOPES.reduce( (acc, scope) => { - acc[scope] = new Set(ACTIONS_BY_SCOPE[scope].map((action) => action.id)); + acc[scope] = new Set(ACTIONS_BY_SCOPE[scope].map((action) => action.id)); return acc; }, - {} as Record>, + {} as Record>, ); +/** + * Helper type used inside this module to bypass the per-scope `Keymap` index + * checks when iterating the registry. The registry guarantees every + * `(scope, id)` pair is valid at runtime; we just need to satisfy the + * compiler that string-keyed assignment is allowed during construction. + */ +type LooseKeymap = Record>; + /** Build a fresh keymap populated from the action registry's defaults. */ export function loadKeymapDefaults(): Keymap { - const keymap: Keymap = { + const keymap: LooseKeymap = { global: {}, pager: {}, menu: {}, @@ -38,7 +46,7 @@ export function loadKeymapDefaults(): Keymap { keymap[action.scope][action.id] = parsed.disabled ? [] : parsed.specs; } - return keymap; + return keymap as Keymap; } function isStringOrStringArray(value: unknown): value is string | string[] { @@ -85,7 +93,7 @@ export function applyKeymapOverrides(base: Keymap, source: Record>>; +/** + * Per-scope keymap. Each scope only stores the action ids that legally belong + * to it, so `keymap.global["menu.close"]` is rejected at compile time. + */ +export type Keymap = { + [S in ActionScope]: Partial, KeySpec[]>>; +}; /** Match a single KeySpec against a live KeyEvent. */ export function matchesKey(spec: KeySpec, key: KeyEvent): boolean { @@ -69,14 +75,26 @@ function nameMatches(specName: string, key: KeyEvent): boolean { return key.name === specName; } +/** + * Look up the specs for an action by its registry definition. Use this when + * iterating `ACTIONS` (e.g. building the help dialog or asserting every + * default is populated) — it sidesteps the per-scope `Keymap` index narrowing + * that callers can't satisfy when `action` is a generic `ActionDef`. + */ +export function getActionSpecs(keymap: Keymap, action: ActionDef): KeySpec[] { + const scopeMap = keymap[action.scope] as Partial>; + return scopeMap[action.id] ?? []; +} + /** True when any spec bound to `(scope, action)` matches the event. */ -export function matchesAction( +export function matchesAction( keymap: Keymap, - scope: ActionScope, - action: ActionId, + scope: S, + action: ActionIdForScope, key: KeyEvent, ): boolean { - const specs = keymap[scope]?.[action]; + const scopeMap = keymap[scope] as Partial>; + const specs = scopeMap[action]; if (!specs || specs.length === 0) return false; return specs.some((spec) => matchesKey(spec, key)); } @@ -84,19 +102,28 @@ export function matchesAction( /** * Resolve which action (if any) is bound to the live event in this scope. * Used when the caller wants to dispatch by id rather than poll-by-action. + * + * **Collision contract:** when two actions share a key (e.g. `` matches + * both `scroll.pageDown` and `scroll.pageUp` under modifier-permissive + * matching), this function returns whichever action was inserted first into + * the scope map — i.e. whichever appears first in the `ACTIONS` registry for + * that scope. The hook does NOT use this for primary dispatch; it polls + * actions in explicit priority order (e.g. `scroll.pageUp` before + * `scroll.pageDown` for Shift+Space). Callers that need a specific precedence + * must replicate that ordering or use `matchesAction` per-action. */ -export function findActionForKey( +export function findActionForKey( keymap: Keymap, - scope: ActionScope, + scope: S, key: KeyEvent, -): ActionId | null { - const scopeMap = keymap[scope]; +): ActionIdForScope | null { + const scopeMap = keymap[scope] as Partial> | undefined; if (!scopeMap) return null; for (const [action, specs] of Object.entries(scopeMap)) { if (!specs || specs.length === 0) continue; if (specs.some((spec) => matchesKey(spec, key))) { - return action as ActionId; + return action as ActionIdForScope; } } return null; diff --git a/src/ui/components/chrome/HelpDialog.tsx b/src/ui/components/chrome/HelpDialog.tsx index 66ed14bb..9f7f3ccb 100644 --- a/src/ui/components/chrome/HelpDialog.tsx +++ b/src/ui/components/chrome/HelpDialog.tsx @@ -1,7 +1,7 @@ import { useMemo } from "react"; import { ACTIONS, type ActionDef } from "../../../core/keymap/actions"; import { formatBinding } from "../../../core/keymap/format"; -import type { Keymap } from "../../../core/keymap/match"; +import { getActionSpecs, type Keymap } from "../../../core/keymap/match"; import type { KeySpec } from "../../../core/keymap/parse"; import { fitText, padText } from "../../lib/text"; import type { AppTheme } from "../../themes"; @@ -36,15 +36,15 @@ function buildHelpSections(keymap: Keymap, canRefresh: boolean): HelpSection[] { const buckets = new Map(); const trackedActions = ACTIONS.filter((action) => { - // Pager-scope entries duplicate global navigation. Only show them if the - // user has actually rebound them — otherwise they're noise. + // Pager/menu/filter-scope entries duplicate global navigation; hide them + // to avoid noise in the help dialog (they share the same action ids). if (action.scope !== "global") return false; if (action.id === "reload" && !canRefresh) return false; return true; }); for (const action of trackedActions) { - const specs = keymap[action.scope][action.id] ?? []; + const specs = getActionSpecs(keymap, action); const row = buildRow(action, specs); let bucket = buckets.get(action.group);