fix(tui): address Copilot review on editor handoff
- resolveEditor() now returns argv (string[]) so EDITOR='code --wait' and VISUAL='emacsclient -t' tokenize correctly into spawnSync's separate command + args. Previously the whole string was passed as argv[0] and would ENOENT. - Skip the POSIX X_OK PATH walk on Windows; return ['notepad.exe'] there since fs.constants.X_OK is not meaningful and PATHEXT-based resolution would need its own implementation. - Surface openEditor() rejections via actions.sys instead of letting them become unhandled promise rejections in the useInput callback. - Hotkey docs/comment now say Cmd/Ctrl+G to match isAction()'s platform-action-modifier behavior (Cmd on macOS, Ctrl elsewhere).
This commit is contained in:
@@ -257,13 +257,14 @@ export function useComposerState({
|
||||
const openEditor = useCallback(async () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), 'hermes-'))
|
||||
const file = join(dir, 'prompt.md')
|
||||
const [cmd, ...args] = resolveEditor()
|
||||
|
||||
writeFileSync(file, [...inputBuf, input].join('\n'))
|
||||
|
||||
let exitCode: null | number = null
|
||||
|
||||
await withInkSuspended(async () => {
|
||||
exitCode = spawnSync(resolveEditor(), [file], { stdio: 'inherit' }).status
|
||||
exitCode = spawnSync(cmd!, [...args, file], { stdio: 'inherit' }).status
|
||||
})
|
||||
|
||||
try {
|
||||
|
||||
@@ -366,10 +366,13 @@ export function useInputHandlers(ctx: InputHandlerContext): InputHandlerResult {
|
||||
return voiceRecordToggle()
|
||||
}
|
||||
|
||||
// Ctrl+G, plus Alt+G fallback for VSCode/Cursor (they bind Ctrl+G to
|
||||
// "Find Next" before the TUI sees it; Alt+G arrives as meta+g).
|
||||
// Cmd/Ctrl+G, plus Alt+G fallback for VSCode/Cursor (they bind the
|
||||
// primary keystroke to "Find Next" before the TUI sees it; Alt+G
|
||||
// arrives as meta+g across platforms).
|
||||
if (ch.toLowerCase() === 'g' && (isAction(key, ch, 'g') || key.meta)) {
|
||||
return cActions.openEditor()
|
||||
return void cActions.openEditor().catch((err: unknown) => {
|
||||
actions.sys(err instanceof Error ? `failed to open editor: ${err.message}` : 'failed to open editor')
|
||||
})
|
||||
}
|
||||
|
||||
// shift-tab flips yolo without spending a turn (claude-code parity)
|
||||
|
||||
@@ -18,7 +18,7 @@ const copyHotkeys: [string, string][] = isMac
|
||||
export const HOTKEYS: [string, string][] = [
|
||||
...copyHotkeys,
|
||||
[action + '+D', 'exit'],
|
||||
[action + '+G / Alt+G', 'open $EDITOR for prompt (Alt+G in VSCode/Cursor)'],
|
||||
[action + '+G / Alt+G', 'open $EDITOR (Alt+G fallback for VSCode/Cursor)'],
|
||||
[action + '+L', 'new session (clear)'],
|
||||
[paste + '+V / /paste', 'paste text; /paste attaches clipboard image'],
|
||||
['Tab', 'apply completion'],
|
||||
|
||||
@@ -23,11 +23,22 @@ describe('resolveEditor', () => {
|
||||
})
|
||||
|
||||
it('honors $VISUAL above all else', () => {
|
||||
expect(resolveEditor({ EDITOR: 'vim', PATH: dir, VISUAL: 'helix' })).toBe('helix')
|
||||
expect(resolveEditor({ EDITOR: 'vim', PATH: dir, VISUAL: 'helix' })).toEqual(['helix'])
|
||||
})
|
||||
|
||||
it('falls back to $EDITOR when $VISUAL is unset', () => {
|
||||
expect(resolveEditor({ EDITOR: 'nvim', PATH: dir })).toBe('nvim')
|
||||
expect(resolveEditor({ EDITOR: 'nvim', PATH: dir })).toEqual(['nvim'])
|
||||
})
|
||||
|
||||
it('shell-tokenizes editors with arguments', () => {
|
||||
expect(resolveEditor({ EDITOR: 'code --wait', PATH: dir })).toEqual(['code', '--wait'])
|
||||
expect(resolveEditor({ PATH: dir, VISUAL: 'emacsclient -t' })).toEqual(['emacsclient', '-t'])
|
||||
})
|
||||
|
||||
it('ignores whitespace-only env vars', () => {
|
||||
const expected = exe(dir, 'editor')
|
||||
|
||||
expect(resolveEditor({ EDITOR: ' ', PATH: dir, VISUAL: '' })).toEqual([expected])
|
||||
})
|
||||
|
||||
it('prefers `editor` over nano over vi on $PATH', () => {
|
||||
@@ -35,18 +46,18 @@ describe('resolveEditor', () => {
|
||||
exe(dir, 'vi')
|
||||
const expected = exe(dir, 'editor')
|
||||
|
||||
expect(resolveEditor({ PATH: dir })).toBe(expected)
|
||||
expect(resolveEditor({ PATH: dir })).toEqual([expected])
|
||||
})
|
||||
|
||||
it('falls back to nano before vi when both exist', () => {
|
||||
exe(dir, 'vi')
|
||||
const expected = exe(dir, 'nano')
|
||||
|
||||
expect(resolveEditor({ PATH: dir })).toBe(expected)
|
||||
expect(resolveEditor({ PATH: dir })).toEqual([expected])
|
||||
})
|
||||
|
||||
it('returns literal "vi" when $PATH is empty', () => {
|
||||
expect(resolveEditor({ PATH: '' })).toBe('vi')
|
||||
it('returns ["vi"] when $PATH is empty', () => {
|
||||
expect(resolveEditor({ PATH: '' })).toEqual(['vi'])
|
||||
})
|
||||
|
||||
it('walks multi-entry $PATH', () => {
|
||||
@@ -54,6 +65,10 @@ describe('resolveEditor', () => {
|
||||
const b = mkdtempSync(join(tmpdir(), 'editor-b-'))
|
||||
const expected = exe(b, 'editor')
|
||||
|
||||
expect(resolveEditor({ PATH: [a, b].join(delimiter) })).toBe(expected)
|
||||
expect(resolveEditor({ PATH: [a, b].join(delimiter) })).toEqual([expected])
|
||||
})
|
||||
|
||||
it('uses notepad.exe on Windows when no env override', () => {
|
||||
expect(resolveEditor({ PATH: dir }, 'win32')).toEqual(['notepad.exe'])
|
||||
})
|
||||
})
|
||||
|
||||
+17
-10
@@ -19,22 +19,29 @@ const isExecutable = (path: string): boolean => {
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve the editor to launch when the user hits Ctrl+G / Alt+G.
|
||||
* Resolve the editor invocation argv (without the file argument).
|
||||
*
|
||||
* 1. $VISUAL / $EDITOR (user's explicit choice)
|
||||
* 2. first FALLBACKS entry resolvable on $PATH
|
||||
* 3. literal `'vi'` so spawnSync still has something to try
|
||||
* 1. $VISUAL / $EDITOR, shell-tokenized so `EDITOR="code --wait"` works
|
||||
* 2. on POSIX: first FALLBACKS entry resolvable on $PATH
|
||||
* 3. on Windows: `notepad.exe`
|
||||
* 4. literal `['vi']` as the last-resort POSIX floor
|
||||
*/
|
||||
export const resolveEditor = (env: NodeJS.ProcessEnv = process.env): string => {
|
||||
if (env.VISUAL) {
|
||||
return env.VISUAL
|
||||
export const resolveEditor = (
|
||||
env: NodeJS.ProcessEnv = process.env,
|
||||
platform: NodeJS.Platform = process.platform
|
||||
): string[] => {
|
||||
const explicit = env.VISUAL ?? env.EDITOR
|
||||
|
||||
if (explicit?.trim()) {
|
||||
return explicit.trim().split(/\s+/)
|
||||
}
|
||||
|
||||
if (env.EDITOR) {
|
||||
return env.EDITOR
|
||||
if (platform === 'win32') {
|
||||
return ['notepad.exe']
|
||||
}
|
||||
|
||||
const dirs = (env.PATH ?? '').split(delimiter).filter(Boolean)
|
||||
const found = FALLBACKS.flatMap(name => dirs.map(d => join(d, name))).find(isExecutable)
|
||||
|
||||
return FALLBACKS.flatMap(name => dirs.map(d => join(d, name))).find(isExecutable) ?? 'vi'
|
||||
return [found ?? 'vi']
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user