fix(tui): address code review findings
Medium fixes: - textInput.tsx: prevent silent data loss when async paste resolves after user types — fall back to raw text insert at current cursor instead of dropping the content entirely - useComposerState.ts: tighten looksLikeDroppedPath to require a second '/' or '.' for bare absolute paths, avoiding unnecessary RPC round-trips for pasted text like /api or /help - useComposerState.ts: add cross-reference comment linking to the canonical _detect_file_drop() in cli.py - osc52.ts: add 500ms timeout via Promise.race so terminals that do not support OSC52 clipboard queries cannot hang paste Low fixes: - terminalSetup.ts: export isRemoteShellSession and reuse in terminalParity.ts and useComposerState.ts (was inlined 3 times) - useComposerState.ts: extract insertAtCursor helper, replacing 3 copies of the lead/tail spacing logic - useComposerState.ts: remove redundant gw from handleTextPaste useCallback dependency array - terminalSetup.test.ts: add EACCES (read-only keybindings.json) and unterminated block comment test coverage
This commit is contained in:
parent
bc9927dc50
commit
c9e8d82ef4
@ -45,6 +45,13 @@ describe('terminalSetup helpers', () => {
|
||||
const input = '[{"key":"a","args":{"text":"// not a comment"}}]'
|
||||
expect(JSON.parse(stripJsonComments(input))).toEqual([{ key: 'a', args: { text: '// not a comment' } }])
|
||||
})
|
||||
|
||||
it('handles unterminated block comments gracefully', () => {
|
||||
const input = '[{"key":"a"} /* never closed'
|
||||
const stripped = stripJsonComments(input)
|
||||
// The unterminated comment is consumed to end-of-file; the remainder is parseable
|
||||
expect(stripped).toBe('[{"key":"a"} ')
|
||||
})
|
||||
})
|
||||
|
||||
describe('configureTerminalKeybindings', () => {
|
||||
@ -114,6 +121,23 @@ describe('configureTerminalKeybindings', () => {
|
||||
expect(copyFile).toHaveBeenCalledTimes(1) // backup created before writing
|
||||
})
|
||||
|
||||
it('reports error when keybindings.json is not readable (EACCES)', async () => {
|
||||
const mkdir = vi.fn().mockResolvedValue(undefined)
|
||||
const readFile = vi.fn().mockRejectedValue(Object.assign(new Error('permission denied'), { code: 'EACCES' }))
|
||||
const writeFile = vi.fn().mockResolvedValue(undefined)
|
||||
const copyFile = vi.fn().mockResolvedValue(undefined)
|
||||
|
||||
const result = await configureTerminalKeybindings('vscode', {
|
||||
fileOps: { copyFile, mkdir, readFile, writeFile },
|
||||
homeDir: '/Users/me',
|
||||
platform: 'darwin'
|
||||
})
|
||||
|
||||
expect(result.success).toBe(false)
|
||||
expect(result.message).toContain('Failed to read')
|
||||
expect(writeFile).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('auto-detects the current IDE terminal', async () => {
|
||||
const mkdir = vi.fn().mockResolvedValue(undefined)
|
||||
const readFile = vi.fn().mockRejectedValue(Object.assign(new Error('missing'), { code: 'ENOENT' }))
|
||||
|
||||
@ -40,10 +40,16 @@ describe('looksLikeDroppedPath', () => {
|
||||
expect(looksLikeDroppedPath('http://localhost/file.pdf')).toBe(false)
|
||||
})
|
||||
|
||||
it('treats leading-slash strings as potential paths (server-side validates)', () => {
|
||||
// The heuristic is intentionally broad — starts with / could be a path.
|
||||
// Server-side image.attach / input.detect_drop does real validation.
|
||||
expect(looksLikeDroppedPath('/help')).toBe(true)
|
||||
expect(looksLikeDroppedPath('/model sonnet')).toBe(true)
|
||||
it('rejects short slash-like strings without path structure', () => {
|
||||
// No second '/' or '.' → not a plausible file path
|
||||
expect(looksLikeDroppedPath('/help')).toBe(false)
|
||||
expect(looksLikeDroppedPath('/model sonnet')).toBe(false)
|
||||
expect(looksLikeDroppedPath('/api')).toBe(false)
|
||||
})
|
||||
|
||||
it('accepts absolute paths with directory separators or extensions', () => {
|
||||
expect(looksLikeDroppedPath('/usr/bin/test')).toBe(true)
|
||||
expect(looksLikeDroppedPath('/tmp/file.txt')).toBe(true)
|
||||
expect(looksLikeDroppedPath('/etc/hosts')).toBe(true) // has second /
|
||||
})
|
||||
})
|
||||
|
||||
@ -14,6 +14,7 @@ import { useInputHistory } from '../hooks/useInputHistory.js'
|
||||
import { useQueue } from '../hooks/useQueue.js'
|
||||
import { isUsableClipboardText, readClipboardText } from '../lib/clipboard.js'
|
||||
import { readOsc52Clipboard } from '../lib/osc52.js'
|
||||
import { isRemoteShellSession } from '../lib/terminalSetup.js'
|
||||
import { pasteTokenLabel, stripTrailingPasteNewlines } from '../lib/text.js'
|
||||
import type { ImageAttachResponse, InputDetectDropResponse } from '../gatewayTypes.js'
|
||||
|
||||
@ -43,6 +44,24 @@ const trimSnips = (snips: PasteSnippet[]): PasteSnippet[] => {
|
||||
return out.length === snips.length ? snips : out
|
||||
}
|
||||
|
||||
/** Insert text at the cursor position, adding spacing to separate from adjacent non-whitespace. */
|
||||
function insertAtCursor(value: string, cursor: number, text: string): { cursor: number; value: string } {
|
||||
const lead = cursor > 0 && !/\s/.test(value[cursor - 1] ?? '') ? ' ' : ''
|
||||
const tail = cursor < value.length && !/\s/.test(value[cursor] ?? '') ? ' ' : ''
|
||||
const insert = `${lead}${text}${tail}`
|
||||
|
||||
return {
|
||||
cursor: cursor + insert.length,
|
||||
value: value.slice(0, cursor) + insert + value.slice(cursor)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Quick client-side heuristic to detect text that looks like a dropped file path.
|
||||
* When this returns true the composer sends RPC calls to the server for actual
|
||||
* validation. Keep in sync with _detect_file_drop() in cli.py — see that
|
||||
* function for the canonical prefix list.
|
||||
*/
|
||||
export function looksLikeDroppedPath(text: string): boolean {
|
||||
const trimmed = text.trim()
|
||||
|
||||
@ -50,19 +69,31 @@ export function looksLikeDroppedPath(text: string): boolean {
|
||||
return false
|
||||
}
|
||||
|
||||
return (
|
||||
trimmed.startsWith('/') ||
|
||||
trimmed.startsWith('~') ||
|
||||
// file:// URIs, relative, home-relative, quoted, and Windows drive paths
|
||||
if (
|
||||
trimmed.startsWith('file://') ||
|
||||
trimmed.startsWith('~/') ||
|
||||
trimmed.startsWith('./') ||
|
||||
trimmed.startsWith('../') ||
|
||||
trimmed.startsWith('file://') ||
|
||||
trimmed.startsWith('"/') ||
|
||||
trimmed.startsWith("'/") ||
|
||||
trimmed.startsWith('"~') ||
|
||||
trimmed.startsWith("'~") ||
|
||||
(/^[A-Za-z]:[\\/]/.test(trimmed)) ||
|
||||
(/^["'][A-Za-z]:[\\/]/.test(trimmed))
|
||||
)
|
||||
(/^[A-Za-z]:[/\\]/.test(trimmed)) ||
|
||||
(/^["'][A-Za-z]:[/\\]/.test(trimmed))
|
||||
) {
|
||||
return true
|
||||
}
|
||||
|
||||
// Bare absolute paths (start with /) — require a second '/' or a '.' to avoid
|
||||
// false positives on short strings like "/api" or "/help" which would trigger
|
||||
// unnecessary RPC round-trips.
|
||||
if (trimmed.startsWith('/')) {
|
||||
const rest = trimmed.slice(1)
|
||||
return rest.includes('/') || rest.includes('.')
|
||||
}
|
||||
|
||||
return false
|
||||
}
|
||||
|
||||
export function useComposerState({ gw, onClipboardPaste, onImageAttached, submitRef }: UseComposerStateOptions): UseComposerStateResult {
|
||||
@ -114,14 +145,7 @@ export function useComposerState({ gw, onClipboardPaste, onImageAttached, submit
|
||||
return { cursor, value }
|
||||
}
|
||||
|
||||
const lead = cursor > 0 && !/\s/.test(value[cursor - 1] ?? '') ? ' ' : ''
|
||||
const tail = cursor < value.length && !/\s/.test(value[cursor] ?? '') ? ' ' : ''
|
||||
const insert = `${lead}${remainder}${tail}`
|
||||
|
||||
return {
|
||||
cursor: cursor + insert.length,
|
||||
value: value.slice(0, cursor) + insert + value.slice(cursor)
|
||||
}
|
||||
return insertAtCursor(value, cursor, remainder)
|
||||
}
|
||||
} catch {
|
||||
// Fall back to generic file-drop detection below.
|
||||
@ -134,14 +158,7 @@ export function useComposerState({ gw, onClipboardPaste, onImageAttached, submit
|
||||
})
|
||||
|
||||
if (dropped?.matched && dropped.text) {
|
||||
const lead = cursor > 0 && !/\s/.test(value[cursor - 1] ?? '') ? ' ' : ''
|
||||
const tail = cursor < value.length && !/\s/.test(value[cursor] ?? '') ? ' ' : ''
|
||||
const insert = `${lead}${dropped.text}${tail}`
|
||||
|
||||
return {
|
||||
cursor: cursor + insert.length,
|
||||
value: value.slice(0, cursor) + insert + value.slice(cursor)
|
||||
}
|
||||
return insertAtCursor(value, cursor, dropped.text)
|
||||
}
|
||||
} catch {
|
||||
// Fall through to normal text paste behavior.
|
||||
@ -158,9 +175,7 @@ export function useComposerState({ gw, onClipboardPaste, onImageAttached, submit
|
||||
}
|
||||
|
||||
const label = pasteTokenLabel(cleanedText, lineCount)
|
||||
const lead = cursor > 0 && !/\s/.test(value[cursor - 1] ?? '') ? ' ' : ''
|
||||
const tail = cursor < value.length && !/\s/.test(value[cursor] ?? '') ? ' ' : ''
|
||||
const insert = `${lead}${label}${tail}`
|
||||
const inserted = insertAtCursor(value, cursor, label)
|
||||
|
||||
setPasteSnips(prev => trimSnips([...prev, { label, text: cleanedText }]))
|
||||
|
||||
@ -177,10 +192,7 @@ export function useComposerState({ gw, onClipboardPaste, onImageAttached, submit
|
||||
})
|
||||
.catch(() => {})
|
||||
|
||||
return {
|
||||
cursor: cursor + insert.length,
|
||||
value: value.slice(0, cursor) + insert + value.slice(cursor)
|
||||
}
|
||||
return inserted
|
||||
},
|
||||
[gw, onClipboardPaste, onImageAttached]
|
||||
)
|
||||
@ -188,7 +200,7 @@ export function useComposerState({ gw, onClipboardPaste, onImageAttached, submit
|
||||
const handleTextPaste = useCallback(
|
||||
({ bracketed, cursor, hotkey, text, value }: PasteEvent): MaybePromise<null | { cursor: number; value: string }> => {
|
||||
if (hotkey) {
|
||||
const preferOsc52 = Boolean(process.env.SSH_CONNECTION || process.env.SSH_TTY || process.env.SSH_CLIENT)
|
||||
const preferOsc52 = isRemoteShellSession(process.env)
|
||||
const readPreferredText = preferOsc52
|
||||
? readOsc52Clipboard(querier).then(async osc52Text => {
|
||||
if (isUsableClipboardText(osc52Text)) {
|
||||
@ -215,7 +227,7 @@ export function useComposerState({ gw, onClipboardPaste, onImageAttached, submit
|
||||
|
||||
return handleResolvedPaste({ bracketed: !!bracketed, cursor, text, value })
|
||||
},
|
||||
[gw, handleResolvedPaste, onClipboardPaste, querier]
|
||||
[handleResolvedPaste, onClipboardPaste, querier]
|
||||
)
|
||||
|
||||
const openEditor = useCallback(() => {
|
||||
|
||||
@ -438,10 +438,18 @@ export function TextInput({
|
||||
const h = cbPaste.current?.(e)
|
||||
|
||||
if (isPasteResultPromise(h)) {
|
||||
const fallbackText = e.text
|
||||
|
||||
void h
|
||||
.then(result => {
|
||||
if (result && editVersionRef.current === startVersion) {
|
||||
commit(result.value, result.cursor)
|
||||
} else if (result && fallbackText && PRINTABLE.test(fallbackText)) {
|
||||
// User typed while async paste was in-flight — fall back to raw text insert
|
||||
// so the pasted content is not silently lost.
|
||||
const cur = curRef.current
|
||||
const v = vRef.current
|
||||
commit(v.slice(0, cur) + fallbackText + v.slice(cur), cur + fallbackText.length)
|
||||
}
|
||||
})
|
||||
.catch(() => {})
|
||||
|
||||
@ -48,18 +48,21 @@ export function parseOsc52ClipboardData(data: string): null | string {
|
||||
}
|
||||
}
|
||||
|
||||
export async function readOsc52Clipboard(querier: null | OscQuerier): Promise<null | string> {
|
||||
export async function readOsc52Clipboard(querier: null | OscQuerier, timeoutMs = 500): Promise<null | string> {
|
||||
if (!querier) {
|
||||
return null
|
||||
}
|
||||
|
||||
const response = await querier.send<OscResponse>({
|
||||
const timeout = new Promise<undefined>(resolve => setTimeout(resolve, timeoutMs))
|
||||
const query = querier.send<OscResponse>({
|
||||
request: buildOsc52ClipboardQuery(),
|
||||
match: (r: unknown): r is OscResponse => {
|
||||
return !!r && typeof r === 'object' && (r as OscResponse).type === 'osc' && (r as OscResponse).code === 52
|
||||
}
|
||||
})
|
||||
|
||||
const response = await Promise.race([query, timeout])
|
||||
|
||||
await querier.flush()
|
||||
|
||||
return response ? parseOsc52ClipboardData(response.data) : null
|
||||
|
||||
@ -1,4 +1,4 @@
|
||||
import { detectVSCodeLikeTerminal, shouldPromptForTerminalSetup, type FileOps } from './terminalSetup.js'
|
||||
import { detectVSCodeLikeTerminal, isRemoteShellSession, shouldPromptForTerminalSetup, type FileOps } from './terminalSetup.js'
|
||||
|
||||
export type MacTerminalHint = {
|
||||
key: string
|
||||
@ -18,7 +18,7 @@ export function detectMacTerminalContext(env: NodeJS.ProcessEnv = process.env):
|
||||
|
||||
return {
|
||||
isAppleTerminal: termProgram === 'Apple_Terminal' || !!env['TERM_SESSION_ID'],
|
||||
isRemote: !!(env['SSH_CONNECTION'] || env['SSH_TTY'] || env['SSH_CLIENT']),
|
||||
isRemote: isRemoteShellSession(env),
|
||||
isTmux: !!env['TMUX'],
|
||||
vscodeLike: detectVSCodeLikeTerminal(env)
|
||||
}
|
||||
|
||||
@ -136,7 +136,7 @@ export function stripJsonComments(content: string): string {
|
||||
return result.replace(/,(\s*[}\]])/g, '$1')
|
||||
}
|
||||
|
||||
function isRemoteShellSession(env: NodeJS.ProcessEnv): boolean {
|
||||
export function isRemoteShellSession(env: NodeJS.ProcessEnv): boolean {
|
||||
return Boolean(env['SSH_CONNECTION'] || env['SSH_TTY'] || env['SSH_CLIENT'])
|
||||
}
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user