diff --git a/ui-tui/src/__tests__/createSlashHandler.test.ts b/ui-tui/src/__tests__/createSlashHandler.test.ts index e8c50c05d2e..18d4d2f16d6 100644 --- a/ui-tui/src/__tests__/createSlashHandler.test.ts +++ b/ui-tui/src/__tests__/createSlashHandler.test.ts @@ -173,6 +173,48 @@ describe('createSlashHandler', () => { expect(ctx.transcript.sys).toHaveBeenCalledWith(expect.stringContaining('usage: /skills')) }) + // Regressions from Copilot review on #19835: /voice output + frontend + // binding state must both track the gateway's fresh ``record_key`` on + // every response, or a config edit shows the new shortcut in text + // while push-to-talk still fires the old one until the next mtime + // poll (~5s). + it('/voice status renders the gateway record_key and pushes it into frontend state', async () => { + const rpc = vi.fn(() => Promise.resolve({ enabled: true, record_key: 'ctrl+space', tts: false })) + const ctx = buildCtx({ gateway: { ...buildGateway(), rpc } }) + + expect(createSlashHandler(ctx)('/voice status')).toBe(true) + await vi.waitFor(() => { + expect(ctx.transcript.sys).toHaveBeenCalledWith(' Record key: Ctrl+Space') + }) + expect(ctx.voice.setVoiceRecordKey).toHaveBeenCalledWith( + expect.objectContaining({ ch: 'space', mod: 'ctrl', named: 'space' }) + ) + }) + + it('/voice on renders the configured binding for the start/stop hint', async () => { + const rpc = vi.fn(() => Promise.resolve({ enabled: true, record_key: 'alt+r', tts: false })) + const ctx = buildCtx({ gateway: { ...buildGateway(), rpc } }) + + expect(createSlashHandler(ctx)('/voice on')).toBe(true) + await vi.waitFor(() => { + expect(ctx.transcript.sys).toHaveBeenCalledWith('Voice mode enabled') + expect(ctx.transcript.sys).toHaveBeenCalledWith(' Alt+R to start/stop recording') + }) + expect(ctx.voice.setVoiceRecordKey).toHaveBeenCalledWith( + expect.objectContaining({ ch: 'r', mod: 'alt' }) + ) + }) + + it('/voice falls back to Ctrl+B when the gateway response omits record_key', async () => { + const rpc = vi.fn(() => Promise.resolve({ enabled: false, tts: false })) + const ctx = buildCtx({ gateway: { ...buildGateway(), rpc } }) + + expect(createSlashHandler(ctx)('/voice status')).toBe(true) + await vi.waitFor(() => { + expect(ctx.transcript.sys).toHaveBeenCalledWith(' Record key: Ctrl+B') + }) + }) + it('cycles details mode and persists it', async () => { const ctx = buildCtx() @@ -648,7 +690,8 @@ const buildTranscript = () => ({ }) const buildVoice = () => ({ - setVoiceEnabled: vi.fn() + setVoiceEnabled: vi.fn(), + setVoiceRecordKey: vi.fn() }) interface Ctx { diff --git a/ui-tui/src/__tests__/platform.test.ts b/ui-tui/src/__tests__/platform.test.ts index a99f5c69756..872710405d1 100644 --- a/ui-tui/src/__tests__/platform.test.ts +++ b/ui-tui/src/__tests__/platform.test.ts @@ -108,12 +108,23 @@ describe('parseVoiceRecordKey (#18994)', () => { expect(parseVoiceRecordKey('alt+b').mod).toBe('alt') expect(parseVoiceRecordKey('option+b').mod).toBe('alt') - expect(parseVoiceRecordKey('cmd+b').mod).toBe('meta') - expect(parseVoiceRecordKey('command+b').mod).toBe('meta') + // ``cmd``/``command`` collapse onto ``super`` — hermes-ink's ``key.meta`` + // means Alt on most terminals, so mapping ``cmd`` to a distinct ``meta`` + // mod would make the display lie about the actual match target. + expect(parseVoiceRecordKey('cmd+b').mod).toBe('super') + expect(parseVoiceRecordKey('command+b').mod).toBe('super') expect(parseVoiceRecordKey('super+b').mod).toBe('super') expect(parseVoiceRecordKey('win+b').mod).toBe('super') }) + it('treats a bare "meta+b" as unrecognised (falls back to Ctrl+B)', async () => { + const { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } = await importPlatform('linux') + + // ``meta`` is ambiguous on the wire (Alt on xterm, Cmd on legacy + // macOS). Rather than pick the wrong one, refuse it. + expect(parseVoiceRecordKey('meta+b')).toEqual(DEFAULT_VOICE_RECORD_KEY) + }) + it('parses named keys (space, enter, tab, escape, backspace, delete)', async () => { const { parseVoiceRecordKey } = await importPlatform('linux') @@ -220,6 +231,64 @@ describe('isVoiceToggleKey honours configured record key (#18994)', () => { expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'b')).toBe(true) expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'o')).toBe(false) }) + + // Regressions from Copilot review on #19835: the previous implementation + // accepted ``isActionMod(key)`` in the ``ctrl`` branch for every + // configured key, so bare Esc (which hermes-ink reports with + // ``key.meta`` on some macOS terminals) fired ``ctrl+escape``, and + // Alt+Space / Alt+Tab fired ``ctrl+space`` / ``ctrl+tab``. The fallback + // is now gated to the documented default (``ctrl+b``) only. + it('ctrl+escape does NOT fire on bare Esc via key.meta on macOS', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin') + const ctrlEscape = parseVoiceRecordKey('ctrl+escape') + + // Bare Esc on a legacy macOS terminal: ``key.meta: true``, ``key.escape: true``, no ctrl. + expect(isVoiceToggleKey({ ctrl: false, escape: true, meta: true, super: false }, '', ctrlEscape)).toBe(false) + // Real Ctrl+Esc still fires. + expect(isVoiceToggleKey({ ctrl: true, escape: true, meta: false, super: false }, '', ctrlEscape)).toBe(true) + }) + + it('ctrl+space does NOT fire on Alt+Space on macOS', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin') + const ctrlSpace = parseVoiceRecordKey('ctrl+space') + + // Alt+Space surfaces as ``key.meta: true`` with space char. + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, ' ', ctrlSpace)).toBe(false) + // Real Ctrl+Space still fires. + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, ' ', ctrlSpace)).toBe(true) + }) + + it('default ctrl+b still accepts Cmd+B on macOS (muscle-memory preserved)', async () => { + const { DEFAULT_VOICE_RECORD_KEY, isVoiceToggleKey } = await importPlatform('darwin') + + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(true) + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(true) + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(true) + }) + + it('custom ctrl+ does NOT accept Cmd fallback on macOS', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin') + const ctrlO = parseVoiceRecordKey('ctrl+o') + + // Only ``ctrl+b`` gets the action-modifier fallback; ``ctrl+o`` must + // be a literal Ctrl bit — otherwise Cmd+O would steal the shortcut. + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'o', ctrlO)).toBe(false) + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'o', ctrlO)).toBe(false) + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'o', ctrlO)).toBe(true) + }) + + it('cmd+b renders "Cmd+B" and matches key.super OR (macOS) key.meta', async () => { + const { formatVoiceRecordKey, isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin') + const cmdB = parseVoiceRecordKey('cmd+b') + + expect(formatVoiceRecordKey(cmdB)).toBe('Cmd+B') + // Kitty-style: key.super + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', cmdB)).toBe(true) + // Legacy macOS terminal: key.meta + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', cmdB)).toBe(true) + // Ctrl held at the same time → reject (different chord). + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: true }, 'b', cmdB)).toBe(false) + }) }) describe('isMacActionFallback', () => { diff --git a/ui-tui/src/__tests__/useConfigSync.test.ts b/ui-tui/src/__tests__/useConfigSync.test.ts index fc2dad19f11..cc8076c1bfb 100644 --- a/ui-tui/src/__tests__/useConfigSync.test.ts +++ b/ui-tui/src/__tests__/useConfigSync.test.ts @@ -292,3 +292,48 @@ describe('applyDisplay → tui_status_indicator', () => { expect($uiState.get().indicatorStyle).toBe('kaomoji') }) }) + +// Regressions from Copilot review on #19835: the config-hydration path +// for voice.record_key was untested, so a future regression in the +// hydration or mtime-reapply wiring would slip past the suite. +describe('applyDisplay → voice.record_key (#18994)', () => { + beforeEach(() => { + resetUiState() + }) + + it('parses voice.record_key and pushes it through the setter', () => { + const setBell = vi.fn() + const setVoiceRecordKey = vi.fn() + + applyDisplay( + { config: { display: {}, voice: { record_key: 'ctrl+space' } } }, + setBell, + setVoiceRecordKey + ) + + expect(setVoiceRecordKey).toHaveBeenCalledWith( + expect.objectContaining({ ch: 'space', mod: 'ctrl', named: 'space', raw: 'ctrl+space' }) + ) + }) + + it('falls back to the documented default when voice.record_key is missing', () => { + const setBell = vi.fn() + const setVoiceRecordKey = vi.fn() + + applyDisplay({ config: { display: {} } }, setBell, setVoiceRecordKey) + + expect(setVoiceRecordKey).toHaveBeenCalledWith( + expect.objectContaining({ ch: 'b', mod: 'ctrl', raw: 'ctrl+b' }) + ) + }) + + it('is a no-op when the voice setter is not passed (back-compat)', () => { + const setBell = vi.fn() + + // applyDisplay is used in the setVoiceEnabled-less init path too; + // omitting the third arg must not throw. + expect(() => + applyDisplay({ config: { display: {}, voice: { record_key: 'alt+r' } } }, setBell) + ).not.toThrow() + }) +}) diff --git a/ui-tui/src/app/interfaces.ts b/ui-tui/src/app/interfaces.ts index 9be5acf5c93..2ef0d849513 100644 --- a/ui-tui/src/app/interfaces.ts +++ b/ui-tui/src/app/interfaces.ts @@ -293,6 +293,7 @@ export interface SlashHandlerContext { } voice: { setVoiceEnabled: StateSetter + setVoiceRecordKey: (v: ParsedVoiceRecordKey) => void } } diff --git a/ui-tui/src/app/slash/commands/session.ts b/ui-tui/src/app/slash/commands/session.ts index 65baea55df6..c371f383444 100644 --- a/ui-tui/src/app/slash/commands/session.ts +++ b/ui-tui/src/app/slash/commands/session.ts @@ -226,7 +226,15 @@ export const sessionCommands: SlashCommand[] = [ // instead of hardcoded "Ctrl+B" — the gateway response carries the // current value so /voice status and /voice on stay in sync with // both the CLI and the TUI's actual binding (#18994). - const recordKeyLabel = formatVoiceRecordKey(parseVoiceRecordKey(r.record_key ?? 'ctrl+b')) + // + // Copilot review on #19835 caught that rendering from the fresh + // backend response WITHOUT updating the frontend ``voice.recordKey`` + // state would skew display and binding between config-edit and + // the next ``mtime`` poll (~5s). Parse once, push into state so + // ``useInputHandlers()`` picks up the new binding immediately. + const parsed = parseVoiceRecordKey(r.record_key ?? 'ctrl+b') + ctx.voice.setVoiceRecordKey(parsed) + const recordKeyLabel = formatVoiceRecordKey(parsed) // Match CLI's _show_voice_status / _enable_voice_mode / // _toggle_voice_tts output shape so users don't have to learn diff --git a/ui-tui/src/app/useMainApp.ts b/ui-tui/src/app/useMainApp.ts index ad62fb33144..59b7bb1a3ad 100644 --- a/ui-tui/src/app/useMainApp.ts +++ b/ui-tui/src/app/useMainApp.ts @@ -644,7 +644,7 @@ export function useMainApp(gw: GatewayClient) { }, slashFlightRef, transcript: { page, panel, send, setHistoryItems, sys, trimLastExchange: session.trimLastExchange }, - voice: { setVoiceEnabled } + voice: { setVoiceEnabled, setVoiceRecordKey } }), [ catalog, diff --git a/ui-tui/src/lib/platform.ts b/ui-tui/src/lib/platform.ts index 09c5be034b1..6da69ea10ff 100644 --- a/ui-tui/src/lib/platform.ts +++ b/ui-tui/src/lib/platform.ts @@ -62,7 +62,7 @@ export const isCopyShortcut = ( * the configured letter so existing macOS muscle memory keeps working * alongside the documented Ctrl+ shortcut. */ -export type VoiceRecordKeyMod = 'alt' | 'ctrl' | 'meta' | 'super' +export type VoiceRecordKeyMod = 'alt' | 'ctrl' | 'super' /** Named (multi-character) keys we support, matching the CLI's * prompt_toolkit binding shape (``c-space``, ``c-enter``, etc.) so a @@ -85,13 +85,18 @@ export const DEFAULT_VOICE_RECORD_KEY: ParsedVoiceRecordKey = { raw: 'ctrl+b' } +/** Modifier aliases. ``meta`` is intentionally absent: hermes-ink sets + * ``key.meta`` for plain Alt/Option (and on some legacy terminals for Cmd + * too), so accepting a literal ``meta+b`` config would display as ``Cmd+B`` + * but match Alt+B on the wire — the kind of lie this fix was meant to + * remove. Users who want the platform action modifier spell it ``cmd`` / + * ``command`` / ``super`` / ``win``. */ const _MOD_ALIASES: Record = { alt: 'alt', - cmd: 'meta', - command: 'meta', + cmd: 'super', + command: 'super', control: 'ctrl', ctrl: 'ctrl', - meta: 'meta', option: 'alt', opt: 'alt', super: 'super', @@ -185,13 +190,17 @@ export const parseVoiceRecordKey = (raw: string): ParsedVoiceRecordKey => { let mod: VoiceRecordKeyMod = 'ctrl' - for (const cand of modCandidates) { - const norm = _MOD_ALIASES[cand] + if (modCandidates.length) { + const norm = _MOD_ALIASES[modCandidates[0]] - if (norm) { - mod = norm - break + // Unknown modifier token (e.g. bare ``meta+b`` which is ambiguous on + // the wire) falls back to the documented default rather than + // silently coercing to Ctrl and producing a misleading bind. + if (!norm) { + return DEFAULT_VOICE_RECORD_KEY } + + mod = norm } if (last.length === 1) { @@ -211,7 +220,7 @@ export const parseVoiceRecordKey = (raw: string): ParsedVoiceRecordKey => { /** Render a parsed key back as ``Ctrl+B`` / ``Ctrl+Space`` for status text. */ export const formatVoiceRecordKey = (parsed: ParsedVoiceRecordKey): string => { - const modLabel = parsed.mod === 'meta' ? 'Cmd' : parsed.mod[0].toUpperCase() + parsed.mod.slice(1) + const modLabel = parsed.mod === 'super' ? 'Cmd' : parsed.mod[0].toUpperCase() + parsed.mod.slice(1) // Named tokens render in title case (Ctrl+Space, Ctrl+Enter); single // chars render upper-case to match the existing Ctrl+B convention. const keyLabel = parsed.named @@ -221,6 +230,9 @@ export const formatVoiceRecordKey = (parsed: ParsedVoiceRecordKey): string => { return `${modLabel}+${keyLabel}` } +const _isDefaultVoiceKey = (parsed: ParsedVoiceRecordKey): boolean => + parsed.raw === DEFAULT_VOICE_RECORD_KEY.raw + export const isVoiceToggleKey = ( key: RuntimeKeyEvent, ch: string, @@ -241,15 +253,21 @@ export const isVoiceToggleKey = ( case 'alt': // Most terminals surface Alt as either ``alt`` or ``meta``; accept // both so the binding works across xterm-style and kitty-style - // protocols. - return key.alt === true || key.meta + // protocols. Guard against ctrl/super bits so a chord like + // Ctrl+Alt+ or Cmd+Alt+ doesn't spuriously fire the + // alt binding. + return (key.alt === true || key.meta) && !key.ctrl && key.super !== true case 'ctrl': - // Doc default — also accept the platform action modifier so macOS - // Cmd+ muscle memory keeps working alongside Ctrl+. - return key.ctrl || isActionMod(key) - case 'meta': - return key.meta || key.super === true + // Only the documented default (``ctrl+b``) gets the macOS + // Cmd-as-action-modifier fallback. For any user-configured binding + // we require the literal Ctrl bit — otherwise ``ctrl+escape`` would + // fire on bare Esc (hermes-ink sets ``key.meta`` for bare Esc on + // some macOS terminals) and ``ctrl+space`` would fire on Alt+Space. + return key.ctrl || (_isDefaultVoiceKey(configured) && isActionMod(key)) case 'super': - return key.super === true + // Kitty-style protocol surfaces Cmd as ``key.super``; legacy macOS + // terminals still surface it as ``key.meta``. Accept both but + // require the ctrl bit to be clear so Ctrl+Cmd+ doesn't match. + return (key.super === true || (isMac && key.meta)) && !key.ctrl } }