From 3776bbd43139bb180a978f8ed2df91b8d06a24df Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Wed, 14 Oct 2026 02:47:58 +0800 Subject: [PATCH] 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. --- .changeset/keybinding-polish.md | 2 +- docs/architecture-invariants.md | 2 +- docs/wiki/Keyboard-Shortcuts.md | 14 +- src/web/public/app.js | 60 ++++---- src/web/public/i18n.js | 2 +- src/web/public/settings-ui.js | 54 +++++++- test/keyboard-shortcuts.test.ts | 10 -- test/shortcut-rebinding.test.ts | 235 ++++++++++++++++++++++++++++++++ 8 files changed, 337 insertions(+), 42 deletions(-) create mode 100644 test/shortcut-rebinding.test.ts diff --git a/.changeset/keybinding-polish.md b/.changeset/keybinding-polish.md index 2ca2e0b0..cdba12e3 100644 --- a/.changeset/keybinding-polish.md +++ b/.changeset/keybinding-polish.md @@ -2,4 +2,4 @@ 'aicodeman': minor --- -Shortcut rebinding (App Settings → Shortcuts) now covers Previous Session and the alternate Next Session, which were fixed at Alt/Option+[ and Alt/Option+]. Capturing a chord another action already owns is refused with a message naming that action (Alt+1-9 are reserved for tab switching). A chord you rebound to an app action no longer also types into the terminal: a rebind to Alt+J used to switch tabs and send ESC j to the session as well. +Shortcut rebinding (App Settings → Shortcuts) now covers Previous Session and the alternate Next Session, which were fixed at Alt/Option+[ and Alt/Option+]. Capturing a chord another action already owns is refused with a message naming that action (Alt+1-9 are reserved for tab switching, Ctrl/Shift+Enter for newline and Ctrl/Cmd+V for paste); re-enabling a disabled shortcut and Reset run the same check. A chord you rebound to an app action no longer also types into the terminal: a rebind to Alt+J used to switch tabs and send ESC j to the session as well. diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index ce51923b..ec53b87e 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -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 `` 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. diff --git a/docs/wiki/Keyboard-Shortcuts.md b/docs/wiki/Keyboard-Shortcuts.md index 3996685c..768085c1 100644 --- a/docs/wiki/Keyboard-Shortcuts.md +++ b/docs/wiki/Keyboard-Shortcuts.md @@ -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. diff --git a/src/web/public/app.js b/src/web/public/app.js index 0cd59388..5e4fb009 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -532,8 +532,8 @@ const DEFAULT_SHORTCUTS = [ group: 'Session', label: 'Toggle Session Sidebar', // Alt+B, not Ctrl+B: Ctrl+B must reach the terminal (tmux prefix, - // readline backward-char). The Alt block below claims only Digit1-9 and - // the brackets, and the registry claims Alt for KeyK and Slash only. + // readline backward-char). The Alt block below claims only Digit1-9; the + // brackets are registry entries, and the registry claims Alt for KeyK and Slash only. bindings: [{ modifiers: ['alt'], key: 'b', code: 'KeyB' }], action: 'toggleSessionSidebar', }, @@ -642,6 +642,7 @@ const DEFAULT_SHORTCUTS = [ id: 'previous-session', group: 'Session', label: 'Previous Session', + displayBindings: ['Alt/Option+['], bindings: [{ modifiers: ['alt'], code: 'BracketLeft' }], action: 'previousSession', }, @@ -649,6 +650,7 @@ const DEFAULT_SHORTCUTS = [ id: 'next-session-alt', group: 'Session', label: 'Next Session (alternate)', + displayBindings: ['Alt/Option+]'], bindings: [{ modifiers: ['alt'], code: 'BracketRight' }], action: 'nextSession', }, @@ -1450,7 +1452,8 @@ class CodemanApp { // Option/Alt session navigation uses physical key CODES, not e.key, so macOS // keyboard layouts that emit special characters under Option (Option+1 -> ¡, // Option+[ -> "“") still switch sessions. e.code is the physical key regardless - // of layout. Option+1-9 = switch by index. Option+[ / ] (prev / next) live in the shortcut registry, so they can be rebound. + // of layout. Option+1-9 = switch by index. Option+[ / ] (prev / next) live in the + // shortcut registry, so they can be rebound. if (e.altKey && !e.ctrlKey && !e.shiftKey) { const code = e.code || ''; const digitMatch = code.match(/^Digit([1-9])$/); @@ -10196,39 +10199,46 @@ class CodemanApp { * absent here (copy selection, the palette, the tile chords) have their own dispatch. */ _shortcutActions() { - return { - showShortcutOverlay: () => this.showShortcutOverlay(), - killActiveSession: () => this.killActiveSession(), - nextSession: () => this.nextSession(), - previousSession: () => this.prevSession(), - clearTerminal: () => this.clearTerminal(), - restoreTerminalSize: () => this.restoreTerminalSize(), - increaseFontSize: () => this.increaseFontSize(), - decreaseFontSize: () => this.decreaseFontSize(), - toggleVoiceInput: () => VoiceInput.toggle(), - moveActiveTabLeft: () => this.moveActiveTabLeft(), - moveActiveTabRight: () => this.moveActiveTabRight(), - toggleSessionSidebar: () => this.toggleSessionSidebar(), - }; + return { + showShortcutOverlay: () => this.showShortcutOverlay(), + killActiveSession: () => this.killActiveSession(), + nextSession: () => this.nextSession(), + previousSession: () => this.prevSession(), + clearTerminal: () => this.clearTerminal(), + restoreTerminalSize: () => this.restoreTerminalSize(), + increaseFontSize: () => this.increaseFontSize(), + decreaseFontSize: () => this.decreaseFontSize(), + toggleVoiceInput: () => VoiceInput.toggle(), + moveActiveTabLeft: () => this.moveActiveTabLeft(), + moveActiveTabRight: () => this.moveActiveTabRight(), + toggleSessionSidebar: () => this.toggleSessionSidebar(), + }; } /** * True when `e` is a chord the USER rebound to a generically dispatched action. * The document handler has already run the action, but its preventDefault() does not * stop xterm, so without this gate a rebind to e.g. Alt+J would also type ESC j into the - * session. Defaults are not covered on purpose: several (Ctrl+L) rely on reaching the PTY. + * session. Defaults are not covered on purpose: several (Ctrl+L) rely on reaching the PTY, + * and neither is a chord that matches the entry's own default (a re-capture of it). */ isUserBoundShortcutEvent(e) { if (!e.ctrlKey && !e.metaKey && !e.altKey) return false; const overrides = this.loadAppSettingsFromStorage().shortcutOverrides || {}; const actions = this._shortcutActions(); - return this.getShortcutRegistry().some( - (shortcut) => - !shortcut.disabled && - Array.isArray(overrides[shortcut.id]?.bindings) && - Object.hasOwn(actions, shortcut.action) && - this.matchesShortcutEvent(e, shortcut) - ); + return this.getShortcutRegistry().some((shortcut) => { + if (shortcut.disabled || !Array.isArray(overrides[shortcut.id]?.bindings)) return false; + // toggleSessionSidebar has its own gate (shouldToggleSessionSidebarFromShortcut) that + // lets the chord through when the sidebar layout is off; swallowing it here would make a + // rebound sidebar key dead in the header layout. + if (shortcut.action === 'toggleSessionSidebar') return false; + if (typeof actions[shortcut.action] !== 'function') return false; + if (!this.matchesShortcutEvent(e, shortcut)) return false; + // Re-capturing the entry's own default (Ctrl+L for Clear Terminal) must keep reaching the + // CLI: clearTerminal() relies on ^L for the redraw. + const fallback = DEFAULT_SHORTCUTS.find((d) => d.id === shortcut.id); + return !this.matchesShortcutEvent(e, fallback); + }); } // Returns the merged shortcut list: DEFAULT_SHORTCUTS with any per-shortcut diff --git a/src/web/public/i18n.js b/src/web/public/i18n.js index ff997009..37777730 100644 --- a/src/web/public/i18n.js +++ b/src/web/public/i18n.js @@ -1141,7 +1141,7 @@ 'Previous / Next Session': '上一个 / 下一个会话', 'Next Session': '下一个会话', 'Previous Session': '上一个会话', - 'Next Session (alternate)': '下一个会话(备用)', + 'Next Session (alternate)': '下一个会话（备用）', 'Switch to Tab N': '切换到第 N 个标签页', 'Move Active Tab Left': '向左移动当前标签页', 'Move Active Tab Right': '向右移动当前标签页', diff --git a/src/web/public/settings-ui.js b/src/web/public/settings-ui.js index d1d43503..7cd79406 100644 --- a/src/web/public/settings-ui.js +++ b/src/web/public/settings-ui.js @@ -4730,10 +4730,16 @@ Object.assign(CodemanApp.prototype, { /** Label of the action that already owns the chord in `e`, or '' when it is free. */ findShortcutConflict(e, shortcutId) { - // Alt+1-9 switch tabs by index; they are fixed, not in the registry. - if (e.altKey && !e.ctrlKey && !e.metaKey && !e.shiftKey && /^Digit[1-9]$/.test(e.code || '')) { + // Chords the keydown handlers claim before (or outside) the registry: they are fixed, so a + // rebind onto one of them would be dead or would hijack it. Alt+1-9 switch tabs by index + // (the document handler does not exclude Cmd); Ctrl/Shift+Enter insert a newline and + // Ctrl/Cmd+V pastes, and the user-bound gate sits above those branches in the xterm handler. + if (e.altKey && !e.ctrlKey && !e.shiftKey && /^Digit[1-9]$/.test(e.code || '')) { return 'Switch to Tab N'; } + // (Alt+Shift+Enter is the tile grid's Zoom chord, a registry entry, so Alt stays free.) + if (e.key === 'Enter' && !e.altKey && (e.ctrlKey || e.shiftKey)) return 'Insert Newline'; + if ((e.ctrlKey || e.metaKey) && !e.altKey && !e.shiftKey && String(e.key).toLowerCase() === 'v') return 'Paste'; if (typeof this.getShortcutRegistry !== 'function' || typeof this.matchesShortcutEvent !== 'function') return ''; const other = this.getShortcutRegistry().find( (shortcut) => @@ -4742,6 +4748,30 @@ Object.assign(CodemanApp.prototype, { return other ? other.label : ''; }, + /** + * Label of the action that already owns a chord in `shortcut`'s bindings, or ''. Used when a + * binding comes back without a capture (re-enable, Reset), where the chord may have been + * given to another action in the meantime. + */ + findShortcutBindingConflict(shortcut) { + for (const binding of shortcut?.bindings || []) { + const mods = binding.modifiers || []; + const taken = this.findShortcutConflict( + { + ctrlKey: mods.includes('ctrl'), + metaKey: mods.includes('meta'), + shiftKey: mods.includes('shift'), + altKey: mods.includes('alt'), + key: binding.key || '', + code: binding.code || '', + }, + shortcut.id + ); + if (taken) return taken; + } + return ''; + }, + describeShortcutEvent(e) { const parts = []; if (e.ctrlKey) parts.push('Ctrl'); @@ -4753,6 +4783,15 @@ Object.assign(CodemanApp.prototype, { }, resetShortcutOverride(shortcutId) { + // Reset puts the default chord back (and re-enables the row): refuse when another action + // has taken that chord since, instead of leaving two actions on one key. + const fallback = typeof DEFAULT_SHORTCUTS !== 'undefined' ? DEFAULT_SHORTCUTS.find((d) => d.id === shortcutId) : null; + const taken = fallback ? this.findShortcutBindingConflict(fallback) : ''; + if (taken) { + this.renderShortcutSettingsList(); + this.showToast?.(`The default chord is already used by ${taken}`, 'error'); + return; + } const settings = this.loadAppSettingsFromStorage(); const shortcutOverrides = { ...(settings.shortcutOverrides || {}) }; delete shortcutOverrides[shortcutId]; @@ -4762,6 +4801,17 @@ Object.assign(CodemanApp.prototype, { }, toggleShortcutEnabled(shortcutId, enabled) { + if (enabled) { + // Re-enabling brings the row's chord back into play: leave it disabled when another action + // has been bound to that chord while it was off. + const shortcut = this.getShortcutRegistry?.().find((s) => s.id === shortcutId); + const taken = this.findShortcutBindingConflict(shortcut); + if (taken) { + this.renderShortcutSettingsList(); + this.showToast?.(`Its chord is already used by ${taken}`, 'error'); + return; + } + } const settings = this.loadAppSettingsFromStorage(); const shortcutOverrides = { ...(settings.shortcutOverrides || {}) }; shortcutOverrides[shortcutId] = { ...(shortcutOverrides[shortcutId] || {}), disabled: !enabled }; diff --git a/test/keyboard-shortcuts.test.ts b/test/keyboard-shortcuts.test.ts index 288d9885..b46c69b6 100644 --- a/test/keyboard-shortcuts.test.ts +++ b/test/keyboard-shortcuts.test.ts @@ -85,14 +85,4 @@ describe('keyboard shortcuts', () => { expect(readme).toContain('`Ctrl/Cmd+C`'); expect(readme).toContain('`Ctrl+Shift+C`'); }); - - it('keeps a user-rebound chord out of the PTY and refuses duplicate bindings', () => { - const tile = readFileSync('src/web/public/terminal-tile.js', 'utf8'); - const settings = readFileSync('src/web/public/settings-ui.js', 'utf8'); - expect(appSource).toContain('isUserBoundShortcutEvent(e) {'); - expect(terminalUiSource).toContain('this.isUserBoundShortcutEvent?.(ev)) return false'); - expect(tile).toContain('global.app?.isUserBoundShortcutEvent?.(ev)) return false'); - expect(settings).toContain('this.findShortcutConflict(e, shortcutId)'); - expect(settings).toContain("return 'Switch to Tab N'"); - }); }); diff --git a/test/shortcut-rebinding.test.ts b/test/shortcut-rebinding.test.ts new file mode 100644 index 00000000..a9485c2e --- /dev/null +++ b/test/shortcut-rebinding.test.ts @@ -0,0 +1,235 @@ +/** + * @fileoverview Behaviour of shortcut rebinding: Alt+[ / Alt+] as registry entries, the + * user-bound gate that keeps a rebound chord out of the PTY, and refusal of duplicate chords. + * + * Real code under test: constants.js + app.js + settings-ui.js in one `vm` context (the + * makeApp() harness of ctrl-w-never-closes.test.ts, with settings-ui.js loaded after app.js). + */ +import { readFileSync } from 'node:fs'; +import { performance } from 'node:perf_hooks'; +import { resolve } from 'node:path'; +import vm from 'node:vm'; +import { describe, expect, it, vi } from 'vitest'; + +const read = (f: string) => readFileSync(resolve(import.meta.dirname, `../src/web/public/${f}`), 'utf8'); + +type Shortcut = { id: string; label: string; action?: string; bindings?: unknown[]; disabled?: boolean }; +type App = { + getShortcutRegistry(): Shortcut[]; + matchesShortcutEvent(e: Record, s: Shortcut): boolean; + isUserBoundShortcutEvent(e: Record): boolean; + findShortcutConflict(e: Record, id: string): string; + onShortcutCaptureKeydown(e: Record, id: string): void; + toggleShortcutEnabled(id: string, enabled: boolean): void; + resetShortcutOverride(id: string): void; + loadAppSettingsFromStorage: () => Record; + saveAppSettingsToStorage: (s: Record) => void; + renderShortcutSettingsList: () => void; + showToast: (msg: string, type?: string) => void; +}; + +function makeApp(initial: Record = {}) { + const settings: Record = structuredClone(initial); + const context = vm.createContext({ + console, + performance, + setInterval: vi.fn(), + clearInterval: vi.fn(), + setTimeout, + clearTimeout, + requestAnimationFrame: vi.fn(), + HTMLCanvasElement: class HTMLCanvasElement {}, + WebSocket: { OPEN: 1 }, + fetch: vi.fn(), + document: { addEventListener: vi.fn(), getElementById: () => null, querySelector: () => null }, + localStorage: { length: 0, key: vi.fn(), getItem: vi.fn(), setItem: vi.fn(), removeItem: vi.fn() }, + window: { addEventListener: vi.fn(), removeEventListener: vi.fn() }, + MobileDetection: { getDeviceType: () => 'desktop', isMobile: () => false, isTouchDevice: () => false }, + }); + vm.runInContext(`${read('constants.js')}\n${read('app.js')}\nglobalThis.__CodemanApp = CodemanApp;`, context); + vm.runInContext(read('settings-ui.js'), context, { filename: 'settings-ui.js' }); + const CodemanApp = (context as unknown as { __CodemanApp: { prototype: object } }).__CodemanApp; + const app = Object.create(CodemanApp.prototype) as App; + app.loadAppSettingsFromStorage = () => settings; + app.saveAppSettingsToStorage = (s) => { + const next = { ...s }; + Object.keys(settings).forEach((k) => delete settings[k]); + Object.assign(settings, next); + }; + app.renderShortcutSettingsList = vi.fn(); + app.showToast = vi.fn(); + return { app, settings }; +} + +const key = (o: Record) => ({ + type: 'keydown', + key: '', + code: '', + ctrlKey: false, + metaKey: false, + shiftKey: false, + altKey: false, + preventDefault: vi.fn(), + stopPropagation: vi.fn(), + ...o, +}); + +const ownerOf = (app: App, e: Record) => + app.getShortcutRegistry().find((s) => !s.disabled && s.action && app.matchesShortcutEvent(e, s))?.action; + +describe('Alt+[ and Alt+] are registry entries', () => { + it('resolve to previousSession and nextSession by physical key', () => { + const { app } = makeApp(); + expect(ownerOf(app, key({ altKey: true, key: '[', code: 'BracketLeft' }))).toBe('previousSession'); + expect(ownerOf(app, key({ altKey: true, key: ']', code: 'BracketRight' }))).toBe('nextSession'); + }); + + it('match the macOS Option layout that prints a curly quote for Option+[', () => { + const { app } = makeApp(); + expect(ownerOf(app, key({ altKey: true, key: '“', code: 'BracketLeft' }))).toBe('previousSession'); + }); + + it('keep their Alt/Option+[ labels until rebound', () => { + const { app } = makeApp({ + shortcutOverrides: { 'next-session-alt': { bindings: [{ modifiers: ['alt'], key: 'j' }] } }, + }); + const byId = (id: string) => + app.getShortcutRegistry().find((s) => s.id === id) as Shortcut & { displayBindings?: string[] }; + expect(byId('previous-session').displayBindings).toEqual(['Alt/Option+[']); + expect(byId('next-session-alt').displayBindings).toBeUndefined(); + }); +}); + +describe('isUserBoundShortcutEvent', () => { + const altJ = key({ altKey: true, key: 'j', code: 'KeyJ' }); + + it('is false with no overrides, even for a default chord', () => { + const { app } = makeApp(); + expect(app.isUserBoundShortcutEvent(key({ altKey: true, key: '[', code: 'BracketLeft' }))).toBe(false); + expect(app.isUserBoundShortcutEvent(key({ ctrlKey: true, key: 'l', code: 'KeyL' }))).toBe(false); + }); + + it('is true for a chord the user rebound an action to', () => { + const { app } = makeApp({ + shortcutOverrides: { 'previous-session': { bindings: [{ modifiers: ['alt'], key: 'j', code: 'KeyJ' }] } }, + }); + expect(app.isUserBoundShortcutEvent(altJ)).toBe(true); + }); + + it('is false for the copy-selection rebind, which has its own dispatch', () => { + const { app } = makeApp({ + shortcutOverrides: { 'copy-selection': { bindings: [{ modifiers: ['alt'], key: 'j', code: 'KeyJ' }] } }, + }); + expect(app.isUserBoundShortcutEvent(altJ)).toBe(false); + }); + + it('is false for a rebound sidebar toggle, whose own gate lets the chord through', () => { + const { app } = makeApp({ + shortcutOverrides: { 'toggle-session-sidebar': { bindings: [{ modifiers: ['alt'], key: 's', code: 'KeyS' }] } }, + }); + expect(app.isUserBoundShortcutEvent(key({ altKey: true, key: 's', code: 'KeyS' }))).toBe(false); + }); + + it('is false when the entry was re-captured onto its own default (Ctrl+L still reaches the CLI)', () => { + const { app } = makeApp({ + shortcutOverrides: { 'clear-terminal': { bindings: [{ modifiers: ['ctrl'], key: 'l', code: 'KeyL' }] } }, + }); + expect(app.isUserBoundShortcutEvent(key({ ctrlKey: true, key: 'l', code: 'KeyL' }))).toBe(false); + }); + + it('is false for a disabled rebound entry', () => { + const { app } = makeApp({ + shortcutOverrides: { + 'previous-session': { bindings: [{ modifiers: ['alt'], key: 'j', code: 'KeyJ' }], disabled: true }, + }, + }); + expect(app.isUserBoundShortcutEvent(altJ)).toBe(false); + }); +}); + +describe('duplicate chords are refused', () => { + const ctrlL = key({ ctrlKey: true, key: 'l', code: 'KeyL' }); + + it("a capture onto another action's chord names the owner and stores nothing", () => { + const { app, settings } = makeApp(); + app.onShortcutCaptureKeydown(ctrlL, 'next-session-alt'); + expect(app.showToast).toHaveBeenCalledWith(expect.stringContaining('already used by Clear Terminal'), 'error'); + expect(settings.shortcutOverrides).toBeUndefined(); + }); + + it("a shortcut's own id and a disabled owner are free", () => { + const { app } = makeApp({ shortcutOverrides: { 'previous-session': { disabled: true } } }); + expect(app.findShortcutConflict(ctrlL, 'clear-terminal')).toBe(''); + expect(app.findShortcutConflict(key({ altKey: true, key: '[', code: 'BracketLeft' }), 'next-session-alt')).toBe(''); + }); + + it.each([ + ['Alt+1', { altKey: true, key: '¡', code: 'Digit1' }], + ['Cmd+Alt+1', { altKey: true, metaKey: true, key: '¡', code: 'Digit1' }], + ['Ctrl+Enter', { ctrlKey: true, key: 'Enter', code: 'Enter' }], + ['Shift+Enter', { shiftKey: true, key: 'Enter', code: 'Enter' }], + ['Ctrl+V', { ctrlKey: true, key: 'v', code: 'KeyV' }], + ['Cmd+V', { metaKey: true, key: 'v', code: 'KeyV' }], + ])('%s is reserved', (_n, ev) => { + const { app } = makeApp(); + expect(app.findShortcutConflict(key(ev), 'previous-session')).not.toBe(''); + }); + + it('no two default shortcuts share a chord', () => { + const { app } = makeApp(); + for (const s of app.getShortcutRegistry()) { + for (const b of (s.bindings || []) as { modifiers?: string[]; key?: string; code?: string }[]) { + const m = b.modifiers || []; + const ev = key({ + ctrlKey: m.includes('ctrl'), + metaKey: m.includes('meta'), + shiftKey: m.includes('shift'), + altKey: m.includes('alt'), + key: b.key || '', + code: b.code || '', + }); + expect({ id: s.id, owner: app.findShortcutConflict(ev, s.id) }).toEqual({ id: s.id, owner: '' }); + } + } + }); + + it('re-enabling a disabled action whose chord was taken is refused and leaves it disabled', () => { + const { app, settings } = makeApp({ + shortcutOverrides: { + 'clear-terminal': { disabled: true }, + 'next-session-alt': { bindings: [{ modifiers: ['ctrl'], key: 'l', code: 'KeyL' }] }, + }, + }); + app.toggleShortcutEnabled('clear-terminal', true); + expect(app.showToast).toHaveBeenCalledWith( + expect.stringContaining('already used by Next Session (alternate)'), + 'error' + ); + expect(settings.shortcutOverrides['clear-terminal'].disabled).toBe(true); + }); + + it('re-enabling with no clash enables it', () => { + const { app, settings } = makeApp({ shortcutOverrides: { 'clear-terminal': { disabled: true } } }); + app.toggleShortcutEnabled('clear-terminal', true); + expect(settings.shortcutOverrides['clear-terminal'].disabled).toBe(false); + }); + + it('Reset is refused while another action owns the default chord, and works once it is free', () => { + const { app, settings } = makeApp({ + shortcutOverrides: { + 'clear-terminal': { bindings: [{ modifiers: ['ctrl'], key: 'j', code: 'KeyJ' }] }, + 'next-session-alt': { bindings: [{ modifiers: ['ctrl'], key: 'l', code: 'KeyL' }] }, + }, + }); + app.resetShortcutOverride('clear-terminal'); + expect(app.showToast).toHaveBeenCalledWith( + expect.stringContaining('already used by Next Session (alternate)'), + 'error' + ); + expect(settings.shortcutOverrides['clear-terminal']).toBeDefined(); + + app.resetShortcutOverride('next-session-alt'); + app.resetShortcutOverride('clear-terminal'); + expect(settings.shortcutOverrides['clear-terminal']).toBeUndefined(); + }); +});