mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-04-28 01:21:43 +00:00
fix(tui): address PR review feedback
Fixes from OutThisLife review: 1. Restore Linux Alt+Enter newline: textInput.tsx now uses k.shift || (isMac ? isActionMod(k) : k.meta) so Alt+Enter inserts a newline on Linux (was broken by isMac guard). 2. Fix image.attach response type: useComposerState.ts now uses ImageAttachResponse (which already has remainder) instead of InputDetectDropResponse with intersection. 3. Expand looksLikeDroppedPath test coverage with edge cases for image extensions, file:// URIs, spaces, empty input, and non-file URLs. 4. Make terminalParity.test.ts hermetic: terminalParityHints() now accepts optional fileOps/homeDir and passes them through to shouldPromptForTerminalSetup(), so tests inject mock readFile instead of hitting the real filesystem. Fixes from Copilot inline review: 5. Remove unused options.now parameter from configureTerminalKeybindings. 6. Replace naive stripJsonComments (full-line // only) with a proper JSONC stripper that handles inline // comments, block comments, trailing commas, and preserves comment-like sequences in strings. 7. Move backupFile() call from immediately after read to right before write - backups are only created when changes will actually be written, not on every /terminal-setup invocation.
This commit is contained in:
parent
9556fef5a1
commit
bc9927dc50
7 changed files with 160 additions and 13 deletions
|
|
@ -1,4 +1,4 @@
|
|||
import { describe, expect, it } from 'vitest'
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import { terminalParityHints } from '../lib/terminalParity.js'
|
||||
|
||||
|
|
@ -15,7 +15,30 @@ describe('terminalParityHints', () => {
|
|||
})
|
||||
|
||||
it('suggests IDE setup only for VS Code-family terminals that still need bindings', async () => {
|
||||
const hints = await terminalParityHints({ TERM_PROGRAM: 'vscode' } as NodeJS.ProcessEnv)
|
||||
const readFile = vi.fn().mockRejectedValue(Object.assign(new Error('missing'), { code: 'ENOENT' }))
|
||||
|
||||
const hints = await terminalParityHints(
|
||||
{ TERM_PROGRAM: 'vscode' } as NodeJS.ProcessEnv,
|
||||
{ fileOps: { readFile }, homeDir: '/tmp/fake-home' }
|
||||
)
|
||||
expect(hints.some(h => h.key === 'ide-setup')).toBe(true)
|
||||
})
|
||||
|
||||
it('suppresses IDE setup hint when keybindings are already configured', async () => {
|
||||
const readFile = vi.fn().mockResolvedValue(
|
||||
JSON.stringify([
|
||||
{ key: 'shift+enter', command: 'workbench.action.terminal.sendSequence', when: 'terminalFocus', args: { text: '\\\r\n' } },
|
||||
{ key: 'ctrl+enter', command: 'workbench.action.terminal.sendSequence', when: 'terminalFocus', args: { text: '\\\r\n' } },
|
||||
{ key: 'cmd+enter', command: 'workbench.action.terminal.sendSequence', when: 'terminalFocus', args: { text: '\\\r\n' } },
|
||||
{ key: 'cmd+z', command: 'workbench.action.terminal.sendSequence', when: 'terminalFocus', args: { text: '\u001b[122;9u' } },
|
||||
{ key: 'shift+cmd+z', command: 'workbench.action.terminal.sendSequence', when: 'terminalFocus', args: { text: '\u001b[122;10u' } }
|
||||
])
|
||||
)
|
||||
|
||||
const hints = await terminalParityHints(
|
||||
{ TERM_PROGRAM: 'vscode' } as NodeJS.ProcessEnv,
|
||||
{ fileOps: { readFile }, homeDir: '/tmp/fake-home' }
|
||||
)
|
||||
expect(hints.some(h => h.key === 'ide-setup')).toBe(false)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -30,6 +30,21 @@ describe('terminalSetup helpers', () => {
|
|||
it('strips line comments from keybindings JSON', () => {
|
||||
expect(stripJsonComments('// comment\n[{"key":"shift+enter"}]')).toBe('\n[{"key":"shift+enter"}]')
|
||||
})
|
||||
|
||||
it('strips inline comments and block comments', () => {
|
||||
expect(stripJsonComments('[{"key":"a"} // inline\n]')).toBe('[{"key":"a"} \n]')
|
||||
expect(stripJsonComments('[/* block */{"key":"a"}]')).toBe('[{"key":"a"}]')
|
||||
})
|
||||
|
||||
it('removes trailing commas before ] or }', () => {
|
||||
expect(JSON.parse(stripJsonComments('[{"key":"a"},]'))).toEqual([{ key: 'a' }])
|
||||
expect(JSON.parse(stripJsonComments('[{"key":"a",}]'))).toEqual([{ key: 'a' }])
|
||||
})
|
||||
|
||||
it('preserves comment-like sequences inside strings', () => {
|
||||
const input = '[{"key":"a","args":{"text":"// not a comment"}}]'
|
||||
expect(JSON.parse(stripJsonComments(input))).toEqual([{ key: 'a', args: { text: '// not a comment' } }])
|
||||
})
|
||||
})
|
||||
|
||||
describe('configureTerminalKeybindings', () => {
|
||||
|
|
@ -48,6 +63,7 @@ describe('configureTerminalKeybindings', () => {
|
|||
expect(result.success).toBe(true)
|
||||
expect(result.requiresRestart).toBe(true)
|
||||
expect(writeFile).toHaveBeenCalledTimes(1)
|
||||
expect(copyFile).not.toHaveBeenCalled() // no existing file to back up
|
||||
const written = writeFile.mock.calls[0]?.[1] as string
|
||||
expect(written).toContain('shift+enter')
|
||||
expect(written).toContain('cmd+enter')
|
||||
|
|
@ -78,6 +94,24 @@ describe('configureTerminalKeybindings', () => {
|
|||
expect(result.success).toBe(false)
|
||||
expect(result.message).toContain('cmd+z')
|
||||
expect(writeFile).not.toHaveBeenCalled()
|
||||
expect(copyFile).not.toHaveBeenCalled() // no backup when not writing
|
||||
})
|
||||
|
||||
it('backs up existing keybindings.json only when writing changes', async () => {
|
||||
const mkdir = vi.fn().mockResolvedValue(undefined)
|
||||
const readFile = vi.fn().mockResolvedValue(JSON.stringify([]))
|
||||
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(true)
|
||||
expect(writeFile).toHaveBeenCalledTimes(1)
|
||||
expect(copyFile).toHaveBeenCalledTimes(1) // backup created before writing
|
||||
})
|
||||
|
||||
it('auto-detects the current IDE terminal', async () => {
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import { describe, expect, it } from 'vitest'
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import { looksLikeDroppedPath } from '../app/useComposerState.js'
|
||||
|
||||
|
|
@ -12,4 +12,38 @@ describe('looksLikeDroppedPath', () => {
|
|||
expect(looksLikeDroppedPath('hello world')).toBe(false)
|
||||
expect(looksLikeDroppedPath('line one\nline two')).toBe(false)
|
||||
})
|
||||
|
||||
it('recognizes common image file extensions', () => {
|
||||
expect(looksLikeDroppedPath('/Users/me/Desktop/photo.jpg')).toBe(true)
|
||||
expect(looksLikeDroppedPath('/Users/me/Desktop/diagram.png')).toBe(true)
|
||||
expect(looksLikeDroppedPath('/tmp/capture.webp')).toBe(true)
|
||||
expect(looksLikeDroppedPath('/tmp/image.gif')).toBe(true)
|
||||
})
|
||||
|
||||
it('recognizes file:// URIs with various extensions', () => {
|
||||
expect(looksLikeDroppedPath('file:///home/user/doc.pdf')).toBe(true)
|
||||
expect(looksLikeDroppedPath('file:///tmp/screenshot.png')).toBe(true)
|
||||
})
|
||||
|
||||
it('recognizes paths with spaces (not backslash-escaped)', () => {
|
||||
expect(looksLikeDroppedPath('/var/folders/x/T/TemporaryItems/Screenshot 2026-04-21 at 1.04.43 PM.png')).toBe(true)
|
||||
})
|
||||
|
||||
it('rejects empty/whitespace-only input', () => {
|
||||
expect(looksLikeDroppedPath('')).toBe(false)
|
||||
expect(looksLikeDroppedPath(' ')).toBe(false)
|
||||
expect(looksLikeDroppedPath('\n')).toBe(false)
|
||||
})
|
||||
|
||||
it('rejects URLs that are not file:// URIs', () => {
|
||||
expect(looksLikeDroppedPath('https://example.com/image.png')).toBe(false)
|
||||
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)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -15,7 +15,7 @@ import { useQueue } from '../hooks/useQueue.js'
|
|||
import { isUsableClipboardText, readClipboardText } from '../lib/clipboard.js'
|
||||
import { readOsc52Clipboard } from '../lib/osc52.js'
|
||||
import { pasteTokenLabel, stripTrailingPasteNewlines } from '../lib/text.js'
|
||||
import type { InputDetectDropResponse } from '../gatewayTypes.js'
|
||||
import type { ImageAttachResponse, InputDetectDropResponse } from '../gatewayTypes.js'
|
||||
|
||||
import type { MaybePromise, PasteSnippet, UseComposerStateOptions, UseComposerStateResult } from './interfaces.js'
|
||||
import { $isBlocked } from './overlayStore.js'
|
||||
|
|
@ -102,7 +102,7 @@ export function useComposerState({ gw, onClipboardPaste, onImageAttached, submit
|
|||
const sid = getUiState().sid
|
||||
if (sid && looksLikeDroppedPath(cleanedText)) {
|
||||
try {
|
||||
const attached = await gw.request<InputDetectDropResponse & { remainder?: string }>('image.attach', {
|
||||
const attached = await gw.request<ImageAttachResponse>('image.attach', {
|
||||
path: cleanedText,
|
||||
session_id: sid
|
||||
})
|
||||
|
|
|
|||
|
|
@ -573,7 +573,7 @@ export function TextInput({
|
|||
}
|
||||
|
||||
if (k.return) {
|
||||
k.shift || (isMac && isActionMod(k))
|
||||
k.shift || (isMac ? isActionMod(k) : k.meta)
|
||||
? commit(ins(vRef.current, curRef.current, '\n'), curRef.current + 1)
|
||||
: cbSubmit.current?.(vRef.current)
|
||||
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import { detectVSCodeLikeTerminal, shouldPromptForTerminalSetup } from './terminalSetup.js'
|
||||
import { detectVSCodeLikeTerminal, shouldPromptForTerminalSetup, type FileOps } from './terminalSetup.js'
|
||||
|
||||
export type MacTerminalHint = {
|
||||
key: string
|
||||
|
|
@ -24,11 +24,14 @@ export function detectMacTerminalContext(env: NodeJS.ProcessEnv = process.env):
|
|||
}
|
||||
}
|
||||
|
||||
export async function terminalParityHints(env: NodeJS.ProcessEnv = process.env): Promise<MacTerminalHint[]> {
|
||||
export async function terminalParityHints(
|
||||
env: NodeJS.ProcessEnv = process.env,
|
||||
options?: { fileOps?: Partial<FileOps>; homeDir?: string }
|
||||
): Promise<MacTerminalHint[]> {
|
||||
const ctx = detectMacTerminalContext(env)
|
||||
const hints: MacTerminalHint[] = []
|
||||
|
||||
if (ctx.vscodeLike && (await shouldPromptForTerminalSetup({ env }))) {
|
||||
if (ctx.vscodeLike && (await shouldPromptForTerminalSetup({ env, fileOps: options?.fileOps, homeDir: options?.homeDir }))) {
|
||||
hints.push({
|
||||
key: 'ide-setup',
|
||||
tone: 'info',
|
||||
|
|
|
|||
|
|
@ -4,7 +4,7 @@ import { join } from 'node:path'
|
|||
|
||||
export type SupportedTerminal = 'cursor' | 'vscode' | 'windsurf'
|
||||
|
||||
type FileOps = {
|
||||
export type FileOps = {
|
||||
copyFile: typeof copyFile
|
||||
mkdir: typeof mkdir
|
||||
readFile: typeof readFile
|
||||
|
|
@ -83,8 +83,57 @@ export function detectVSCodeLikeTerminal(env: NodeJS.ProcessEnv = process.env):
|
|||
return null
|
||||
}
|
||||
|
||||
/**
|
||||
* Strip JSONC features (// line comments, /* block comments *\/, trailing commas)
|
||||
* so the result is valid JSON parseable by JSON.parse().
|
||||
* Handles comments inside strings correctly (preserves them).
|
||||
*/
|
||||
export function stripJsonComments(content: string): string {
|
||||
return content.replace(/^\s*\/\/.*$/gm, '')
|
||||
let result = ''
|
||||
let i = 0
|
||||
const len = content.length
|
||||
|
||||
while (i < len) {
|
||||
const ch = content[i]!
|
||||
|
||||
// String literal — copy as-is, including any comment-like chars inside
|
||||
if (ch === '"') {
|
||||
let j = i + 1
|
||||
while (j < len) {
|
||||
if (content[j] === '\\') {
|
||||
j += 2 // skip escaped char
|
||||
} else if (content[j] === '"') {
|
||||
j++
|
||||
break
|
||||
} else {
|
||||
j++
|
||||
}
|
||||
}
|
||||
result += content.slice(i, j)
|
||||
i = j
|
||||
continue
|
||||
}
|
||||
|
||||
// Line comment
|
||||
if (ch === '/' && content[i + 1] === '/') {
|
||||
const eol = content.indexOf('\n', i)
|
||||
i = eol === -1 ? len : eol
|
||||
continue
|
||||
}
|
||||
|
||||
// Block comment
|
||||
if (ch === '/' && content[i + 1] === '*') {
|
||||
const end = content.indexOf('*/', i + 2)
|
||||
i = end === -1 ? len : end + 2
|
||||
continue
|
||||
}
|
||||
|
||||
result += ch
|
||||
i++
|
||||
}
|
||||
|
||||
// Remove trailing commas before ] or }
|
||||
return result.replace(/,(\s*[}\]])/g, '$1')
|
||||
}
|
||||
|
||||
function isRemoteShellSession(env: NodeJS.ProcessEnv): boolean {
|
||||
|
|
@ -127,7 +176,6 @@ export async function configureTerminalKeybindings(
|
|||
env?: NodeJS.ProcessEnv
|
||||
fileOps?: Partial<FileOps>
|
||||
homeDir?: string
|
||||
now?: () => Date
|
||||
platform?: NodeJS.Platform
|
||||
}
|
||||
): Promise<TerminalSetupResult> {
|
||||
|
|
@ -159,9 +207,10 @@ export async function configureTerminalKeybindings(
|
|||
await ops.mkdir(configDir, { recursive: true })
|
||||
|
||||
let keybindings: unknown[] = []
|
||||
let hasExistingFile = false
|
||||
try {
|
||||
const content = await ops.readFile(keybindingsFile, 'utf8')
|
||||
await backupFile(keybindingsFile, ops)
|
||||
hasExistingFile = true
|
||||
const parsed: unknown = JSON.parse(stripJsonComments(content))
|
||||
if (!Array.isArray(parsed)) {
|
||||
return {
|
||||
|
|
@ -208,6 +257,10 @@ export async function configureTerminalKeybindings(
|
|||
}
|
||||
}
|
||||
|
||||
if (hasExistingFile) {
|
||||
await backupFile(keybindingsFile, ops)
|
||||
}
|
||||
|
||||
await ops.writeFile(keybindingsFile, `${JSON.stringify(keybindings, null, 2)}\n`, 'utf8')
|
||||
|
||||
return {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue