mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-20 09:09:35 +02:00
fix(shortcuts): address review on rebinding (gate holes, re-enable/Reset duplicates, labels, tests)
- isUserBoundShortcutEvent skips toggleSessionSidebar (it has its own gate) and an entry re-captured onto its own default, so ^L still reaches the CLI. - Re-enabling a shortcut and Reset run the duplicate check and refuse when the chord was taken meanwhile; Ctrl/Shift+Enter and Ctrl/Cmd+V are reserved with Alt+1-9, and the Alt-digit check no longer excludes Cmd. - Alt+[ / Alt+] keep their Alt/Option+[ labels via displayBindings. - Behavioural tests (vm harness) replace the source-string test. - Docs: wiki Rebinding / Why-chords-behave-oddly, architecture-invariants note. - Small: _shortcutActions indentation, stale Alt-block comment, typeof check instead of Object.hasOwn, full-width parentheses in zh-CN.
This commit is contained in:
1 parent
1aeabdfd55
commit
3776bbd431
8 files changed
+337
-42
No files matched your search
@@ -672,7 +672,7 @@ Matching semantics: a query containing `/` matches the relative path, otherwise
|
||||
|
||||
### Command palette and shortcut registry
|
||||
|
||||
**Command palette + shortcut registry** (COD-151/153/157/192, #146): `Ctrl/Cmd/Alt+K` opens the session palette (fuzzy search over live sessions; "Browse all sessions" → the Session Manager modal backed by `GET /api/sessions/unified`); the quick-start case `<select>` is fronted by a searchable picker (`buildCasePickerOptions`/`formatCasePickerLabel` — remote cases render `name @ hostId`). Shortcuts live in a rebindable registry (`DEFAULT_SHORTCUTS`/`getShortcutRegistry()`/`matchesShortcutEvent()` in app.js; overrides persist under `settings.shortcutOverrides` via `saveAppSettingsToStorage`); App Settings → Shortcuts renders capture/disable rows; `Ctrl+?` opens the registry-driven overlay (footer links to the full `#helpModal` reference). ⚠️ Palette-chord keys must ALSO be swallowed in `attachCustomKeyEventHandler` (terminal-ui.js) or xterm writes the control byte (0x0B) into the PTY. ⚠️ The global Escape handler (app.js) calls every overlay close method on every Escape, in the capture phase and so before the focused element's own Escape handler, which means a close method with side effects beyond hiding must return early when its overlay is not open: `closeCommandPalette()` and `closeSessionManager()` hand focus back on close (`_restoreOverlayFocus()`, panels-ui.js), and run unconditionally that restore stole focus from split Pane B and from any text field, and turned the inline tab rename's Escape into a commit. ⚠️ `saveAppSettings()` rebuilds settings from the DOM — keys edited elsewhere (`shortcutOverrides`, `showTokenCount`, `showCost`) need explicit `_prev` carry-over.
|
||||
**Command palette + shortcut registry** (COD-151/153/157/192, #146): `Ctrl/Cmd/Alt+K` opens the session palette (fuzzy search over live sessions; "Browse all sessions" → the Session Manager modal backed by `GET /api/sessions/unified`); the quick-start case `<select>` is fronted by a searchable picker (`buildCasePickerOptions`/`formatCasePickerLabel` — remote cases render `name @ hostId`). Shortcuts live in a rebindable registry (`DEFAULT_SHORTCUTS`/`getShortcutRegistry()`/`matchesShortcutEvent()` in app.js; overrides persist under `settings.shortcutOverrides` via `saveAppSettingsToStorage`); App Settings → Shortcuts renders capture/disable rows; `Ctrl+?` opens the registry-driven overlay (footer links to the full `#helpModal` reference). ⚠️ Palette-chord keys must ALSO be swallowed in `attachCustomKeyEventHandler` (terminal-ui.js) or xterm writes the control byte (0x0B) into the PTY. ⚠️ The global Escape handler (app.js) calls every overlay close method on every Escape, in the capture phase and so before the focused element's own Escape handler, which means a close method with side effects beyond hiding must return early when its overlay is not open: `closeCommandPalette()` and `closeSessionManager()` hand focus back on close (`_restoreOverlayFocus()`, panels-ui.js), and run unconditionally that restore stole focus from split Pane B and from any text field, and turned the inline tab rename's Escape into a commit. ⚠️ `saveAppSettings()` rebuilds settings from the DOM — keys edited elsewhere (`shortcutOverrides`, `showTokenCount`, `showCost`) need explicit `_prev` carry-over. ⚠️ Every xterm key handler (Pane A in terminal-ui.js, Pane B in terminal-tile.js) must call `isUserBoundShortcutEvent(ev)`, which is true only for a chord the user REBOUND a generically dispatched action to: the document capture handler's `preventDefault()` does not stop xterm, so a rebind to e.g. Alt+J would otherwise also type `ESC j` into the PTY. Defaults are deliberately not gated (Ctrl+L must reach the CLI for its redraw), so the gate also skips an entry re-captured onto its own default, `toggleSessionSidebar` (it has its own gate, `shouldToggleSessionSidebarFromShortcut`) and copy-selection. Capturing, re-enabling and Reset all go through `findShortcutConflict`/`findShortcutBindingConflict` (settings-ui.js); Alt+1-9, Ctrl/Shift+Enter and Ctrl/Cmd+V are reserved there.
|
||||
|
||||
**A key gate on a chord that can carry Shift alone must swallow the KEYPRESS too** (#520). xterm calls the custom handler from `attachCustomKeyEventHandler` for `keydown`, `keypress` AND `keyup`, and its own `_keyPress` drops a keypress carrying Ctrl, Alt or Meta but not one whose only modifier is Shift. So a gate that returns `false` for `ev.type === 'keydown'` alone is safe for a Ctrl or Alt chord (the palette's Ctrl+K, Alt+B, Ctrl+V, Ctrl+Z and the keydown-only smart copy below) and leaks for a Shift-only one. That is how Shift+Enter submitted the prompt from April to #520: its keypress fell through to `onData` as a bare `\r`, which goes out synchronously over the WebSocket ahead of the async `send-key` fetch, so the CR reached the PTY before the newline did. Ctrl+Enter worked all along only because Chrome fires no keypress for it. ⚠️ The Shift/Ctrl+Enter gate in terminal-ui.js and its twin in Pane B (terminal-split.js) return `false` for EVERY event type and key only the `send-key` POST on `keydown`; never narrow either back to keydown, and give any new Shift-only chord the same shape. Swallowing the keyup as well is harmless (xterm only skips a focus and cursor-style refresh for that one event). Tests: `test/shift-enter-keypress-swallowed.test.ts` (CI gate, a source guard on both files) and `test/shift-enter-keypress.browser.test.ts` (browser suite, which the gate does not run: real keys against both real handlers in the real app). Settings → Terminal & Input → **Key tester** shows what a device reports for each of the three events.
|
||||
|
||||
|
||||
@@ -67,6 +67,13 @@ default. See [Tile Grid](Tile-Grid).
|
||||
|
||||
**App Settings → Shortcuts.** Bindings live in a registry with per-user overrides, so a
|
||||
rebind is stored as an override on top of the default rather than replacing the table.
|
||||
Previous Session (`Alt+[`) and Next Session (alternate, `Alt+]`) are rows like any other,
|
||||
so they can be rebound too.
|
||||
|
||||
A chord another action already uses is refused, and the message names that action. The same
|
||||
check runs when you re-enable a disabled shortcut and when you press Reset: if its chord was
|
||||
given to something else in the meantime, it stays as it was. `Alt+1` to `Alt+9`,
|
||||
`Shift+Enter`/`Ctrl+Enter` and `Ctrl+V`/`Cmd+V` are reserved.
|
||||
|
||||
Two things are deliberately not rebindable:
|
||||
|
||||
@@ -79,8 +86,11 @@ Two things are deliberately not rebindable:
|
||||
|
||||
The terminal sees keystrokes before the app does. Any chord the app claims has to also be
|
||||
swallowed at the terminal layer, or xterm writes the control byte into the session as well
|
||||
as triggering the action. If you rebind something to a chord the terminal cares about
|
||||
(`Ctrl+D`, say), expect the CLI to see it too.
|
||||
as triggering the action. A chord **you** rebind an action to is kept out of the terminal
|
||||
too, so rebinding Previous Session to `Alt+J` switches the session and sends nothing to the
|
||||
CLI. The built-in defaults are deliberately not held back this way: `Ctrl+L` clears the
|
||||
screen and also reaches the CLI, which redraws on it. Binding an action back onto its own
|
||||
default behaves like the default.
|
||||
|
||||
`Alt+1` through `Alt+9` are matched on **physical key position** rather than the character
|
||||
produced, so macOS Option layouts that produce `¡™£` still switch tabs.
|
||||
|
||||
Reference in new issue
Block a user