From 83129e72de7baf202437f31e7158c8c794cdbdb3 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Sat, 25 Apr 2026 20:24:06 -0500 Subject: [PATCH] refactor(tui): tighten editor handoff helpers - editor.ts: collapse two private helpers into one flatMap-driven lookup, keep `isExecutable` as the only named primitive, document the fallback chain with prompt_toolkit parity - editor.test.ts: hoist the `exe` helper out of `describe`, drop the empty afterEach + dead mkdir branch, materialize expected paths before the resolveEditor call so argument evaluation order doesn't bite - useComposerState.openEditor: rmSync the mkdtemp dir (was leaking), early-return on bad exit / empty buffer, run cleanup in finally - useInputHandlers: cheap `ch.toLowerCase() === 'g'` guard before the modifier check - hermes-ink/screen.ts: pick up `npm run fix` import-sort cleanup so lint passes --- ui-tui/packages/hermes-ink/src/ink/screen.ts | 4 +- ui-tui/src/app/useComposerState.ts | 29 +++++---- ui-tui/src/app/useInputHandlers.ts | 7 +-- ui-tui/src/lib/editor.test.ts | 63 ++++++++------------ ui-tui/src/lib/editor.ts | 62 ++++++++----------- 5 files changed, 74 insertions(+), 91 deletions(-) diff --git a/ui-tui/packages/hermes-ink/src/ink/screen.ts b/ui-tui/packages/hermes-ink/src/ink/screen.ts index 32c7e7d7e6..6916b8598e 100644 --- a/ui-tui/packages/hermes-ink/src/ink/screen.ts +++ b/ui-tui/packages/hermes-ink/src/ink/screen.ts @@ -1,6 +1,6 @@ -import { ansiCodesToString, diffAnsiCodes, type AnsiCode } from '@alcalzone/ansi-tokenize' +import { type AnsiCode, ansiCodesToString, diffAnsiCodes } from '@alcalzone/ansi-tokenize' -import { unionRect, type Point, type Rectangle, type Size } from './layout/geometry.js' +import { type Point, type Rectangle, type Size, unionRect } from './layout/geometry.js' import { BEL, ESC, SEP } from './termio/ansi.js' import * as warn from './warn.js' diff --git a/ui-tui/src/app/useComposerState.ts b/ui-tui/src/app/useComposerState.ts index 79d5497304..9bc12b61b8 100644 --- a/ui-tui/src/app/useComposerState.ts +++ b/ui-tui/src/app/useComposerState.ts @@ -255,27 +255,34 @@ export function useComposerState({ ) const openEditor = useCallback(async () => { - const editor = resolveEditor() - const file = join(mkdtempSync(join(tmpdir(), 'hermes-')), 'prompt.md') - let code: null | number = null + const dir = mkdtempSync(join(tmpdir(), 'hermes-')) + const file = join(dir, 'prompt.md') writeFileSync(file, [...inputBuf, input].join('\n')) + let exitCode: null | number = null + await withInkSuspended(async () => { - code = spawnSync(editor, [file], { stdio: 'inherit' }).status + exitCode = spawnSync(resolveEditor(), [file], { stdio: 'inherit' }).status }) - if (code === 0) { + try { + if (exitCode !== 0) { + return + } + const text = readFileSync(file, 'utf8').trimEnd() - if (text) { - setInput('') - setInputBuf([]) - submitRef.current(text) + if (!text) { + return } - } - rmSync(file, { force: true }) + setInput('') + setInputBuf([]) + submitRef.current(text) + } finally { + rmSync(dir, { force: true, recursive: true }) + } }, [input, inputBuf, submitRef]) const actions = useMemo( diff --git a/ui-tui/src/app/useInputHandlers.ts b/ui-tui/src/app/useInputHandlers.ts index 88d065feeb..51a65a8d41 100644 --- a/ui-tui/src/app/useInputHandlers.ts +++ b/ui-tui/src/app/useInputHandlers.ts @@ -366,10 +366,9 @@ export function useInputHandlers(ctx: InputHandlerContext): InputHandlerResult { return voiceRecordToggle() } - // Alt+G is the escape hatch for terminals that swallow Ctrl+G — VSCode and - // Cursor bind it to "Find Next" by default, so the keystroke never reaches - // the embedded TUI. Alt+G arrives as `\x1bg` → meta+g across platforms. - if (isAction(key, ch, 'g') || (key.meta && ch.toLowerCase() === 'g')) { + // 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). + if (ch.toLowerCase() === 'g' && (isAction(key, ch, 'g') || key.meta)) { return cActions.openEditor() } diff --git a/ui-tui/src/lib/editor.test.ts b/ui-tui/src/lib/editor.test.ts index 07a3c4d130..0aba6cd2f9 100644 --- a/ui-tui/src/lib/editor.test.ts +++ b/ui-tui/src/lib/editor.test.ts @@ -1,30 +1,27 @@ -import { chmodSync, mkdirSync, mkdtempSync, writeFileSync } from 'node:fs' +import { chmodSync, mkdtempSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { delimiter, join } from 'node:path' -import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { beforeEach, describe, expect, it } from 'vitest' import { resolveEditor } from './editor.js' +const exe = (dir: string, name: string): string => { + const path = join(dir, name) + + writeFileSync(path, '#!/bin/sh\nexit 0\n') + chmodSync(path, 0o755) + + return path +} + describe('resolveEditor', () => { let dir: string - const exe = (name: string) => { - const path = join(dir, name) - writeFileSync(path, '#!/bin/sh\nexit 0\n') - chmodSync(path, 0o755) - - return path - } - beforeEach(() => { dir = mkdtempSync(join(tmpdir(), 'editor-test-')) }) - afterEach(() => { - // tmp dir is small; let the OS reap it - }) - it('honors $VISUAL above all else', () => { expect(resolveEditor({ EDITOR: 'vim', PATH: dir, VISUAL: 'helix' })).toBe('helix') }) @@ -33,40 +30,30 @@ describe('resolveEditor', () => { expect(resolveEditor({ EDITOR: 'nvim', PATH: dir })).toBe('nvim') }) - it('prefers system editor over nano over vi on $PATH', () => { - exe('nano') - exe('vi') - const editor = exe('editor') + it('prefers `editor` over nano over vi on $PATH', () => { + exe(dir, 'nano') + exe(dir, 'vi') + const expected = exe(dir, 'editor') - expect(resolveEditor({ PATH: dir })).toBe(editor) + expect(resolveEditor({ PATH: dir })).toBe(expected) }) - it('falls back to nano when only nano and vi exist', () => { - const nano = exe('nano') - exe('vi') + it('falls back to nano before vi when both exist', () => { + exe(dir, 'vi') + const expected = exe(dir, 'nano') - expect(resolveEditor({ PATH: dir })).toBe(nano) + expect(resolveEditor({ PATH: dir })).toBe(expected) }) - it('falls back to vi when only vi exists', () => { - const vi = exe('vi') - - expect(resolveEditor({ PATH: dir })).toBe(vi) + it('returns literal "vi" when $PATH is empty', () => { + expect(resolveEditor({ PATH: '' })).toBe('vi') }) - it('returns literal "vi" when nothing on PATH and no env', () => { - mkdirSync(join(dir, 'empty'), { recursive: true }) - - expect(resolveEditor({ PATH: join(dir, 'empty') })).toBe('vi') - }) - - it('walks multi-entry PATH', () => { + it('walks multi-entry $PATH', () => { const a = mkdtempSync(join(tmpdir(), 'editor-a-')) const b = mkdtempSync(join(tmpdir(), 'editor-b-')) + const expected = exe(b, 'editor') - writeFileSync(join(b, 'editor'), '#!/bin/sh\n') - chmodSync(join(b, 'editor'), 0o755) - - expect(resolveEditor({ PATH: [a, b].join(delimiter) })).toBe(join(b, 'editor')) + expect(resolveEditor({ PATH: [a, b].join(delimiter) })).toBe(expected) }) }) diff --git a/ui-tui/src/lib/editor.ts b/ui-tui/src/lib/editor.ts index a871eb7e6d..018fe2c88e 100644 --- a/ui-tui/src/lib/editor.ts +++ b/ui-tui/src/lib/editor.ts @@ -2,44 +2,13 @@ import { accessSync, constants } from 'node:fs' import { delimiter, join } from 'node:path' /** - * Resolve which editor to launch when the user hits Ctrl+G / Alt+G. - * - * Order of preference: - * 1. $VISUAL / $EDITOR (user's explicit choice) - * 2. prompt_toolkit-compatible system fallback: - * editor → nano → pico → vi → emacs - * 3. literal `'vi'` so spawnSync still has something to try - * - * This intentionally mirrors prompt_toolkit's Buffer.open_in_editor() picker - * used by the classic CLI. In Cursor/VSCode terminals, nano is a better prompt - * editing default than dropping casual users into vi's modal interface. + * Editor fallback chain when neither $VISUAL nor $EDITOR is set. Mirrors + * prompt_toolkit's `Buffer.open_in_editor()` picker so the classic CLI and + * the TUI launch the same editor on a given box. */ -export function resolveEditor(env: NodeJS.ProcessEnv = process.env): string { - return ( - env.VISUAL || - env.EDITOR || - findEditor(env.PATH ?? '', 'editor', 'nano', 'pico', 'vi', 'emacs') || - 'vi' - ) -} +const FALLBACKS = ['editor', 'nano', 'pico', 'vi', 'emacs'] -function findEditor(path: string, ...names: string[]): null | string { - const dirs = path.split(delimiter).filter(Boolean) - - for (const name of names) { - for (const dir of dirs) { - const candidate = join(dir, name) - - if (isExecutable(candidate)) { - return candidate - } - } - } - - return null -} - -function isExecutable(path: string): boolean { +const isExecutable = (path: string): boolean => { try { accessSync(path, constants.X_OK) @@ -48,3 +17,24 @@ function isExecutable(path: string): boolean { return false } } + +/** + * Resolve the editor to launch when the user hits Ctrl+G / Alt+G. + * + * 1. $VISUAL / $EDITOR (user's explicit choice) + * 2. first FALLBACKS entry resolvable on $PATH + * 3. literal `'vi'` so spawnSync still has something to try + */ +export const resolveEditor = (env: NodeJS.ProcessEnv = process.env): string => { + if (env.VISUAL) { + return env.VISUAL + } + + if (env.EDITOR) { + return env.EDITOR + } + + const dirs = (env.PATH ?? '').split(delimiter).filter(Boolean) + + return FALLBACKS.flatMap(name => dirs.map(d => join(d, name))).find(isExecutable) ?? 'vi' +}