mirror of
https://github.com/stablyai/orca.git
synced 2026-10-17 00:03:08 +00:00
fix(terminal): linkify bare filenames nested in the worktree (#5024)
A file referenced in agent terminal output by BARE name (e.g. `TerminalContextMenu.test.tsx`) was not clickable. The link provider only resolved the parsed token relative to the terminal's cwd and then probed existence; a file nested elsewhere in the worktree failed that probe and was dropped, so it never became a clickable link. Add a workspace fallback: when a slash-free, non-remote token misses the cwd-relative existence check, resolve it against the worktree's own file listing (the same `fs:listFiles` Quick Open uses) and link it only on a UNIQUE basename match. Ambiguous names (e.g. `index.ts` in two dirs) stay unlinked rather than guessed. The basename index is cached per connection+worktree on a short TTL so the provider does not re-list on every hover, and the listing is best-effort (a failure never breaks link detection). The fallback is skipped for remote runtime hosts and for tokens that already contain a path separator. Refs #5024 (the created-after-miss / TTL case is tracked separately). Supersedes #6269. Co-authored-by: gatsby74 <166927047+gatsby74@users.noreply.github.com>
This commit is contained in:
@@ -13,6 +13,7 @@ import {
|
||||
openDetectedFilePath
|
||||
} from './terminal-link-handlers'
|
||||
import { TERMINAL_PATH_EXISTS_CACHE_MAX_ENTRIES } from './terminal-path-exists-cache'
|
||||
import { __resetWorkspaceFileIndexCacheForTest } from './terminal-workspace-file-resolution'
|
||||
import { handleOscLink } from './terminal-osc-link-routing'
|
||||
import { installHttpLinkClickFallback } from './terminal-url-link-hit-testing'
|
||||
import { registerHttpLinkStoreAccessor } from '@/lib/http-link-routing'
|
||||
@@ -31,6 +32,12 @@ const openFileMock = vi.fn()
|
||||
const authorizeExternalPathMock = vi.fn()
|
||||
const statMock = vi.fn().mockResolvedValue({ isDirectory: false })
|
||||
const fsPathExistsMock = vi.fn().mockResolvedValue(true)
|
||||
// Why: the workspace-fallback (issue #5024) calls fs.listFiles; default to an
|
||||
// empty listing so the fallback runs intentionally rather than being silently
|
||||
// swallowed by resolveWorkspaceFileByBasename's try/catch on a missing mock.
|
||||
const listFilesMock = vi
|
||||
.fn<(args: { rootPath: string }) => Promise<string[]>>()
|
||||
.mockResolvedValue([])
|
||||
const runtimeEnvironmentCallMock = vi.fn()
|
||||
const runtimeEnvironmentTransportCallMock = vi.fn()
|
||||
const setActiveWorktreeMock = vi.fn()
|
||||
@@ -105,6 +112,8 @@ beforeEach(() => {
|
||||
})
|
||||
vi.mocked(getConnectionId).mockReturnValue(null)
|
||||
openFilePathMock.mockResolvedValue(true)
|
||||
listFilesMock.mockResolvedValue([])
|
||||
__resetWorkspaceFileIndexCacheForTest()
|
||||
storeState.settings = undefined
|
||||
storeState.worktreesByRepo = {}
|
||||
registerHttpLinkStoreAccessor(() => storeState)
|
||||
@@ -120,7 +129,8 @@ beforeEach(() => {
|
||||
fs: {
|
||||
authorizeExternalPath: authorizeExternalPathMock,
|
||||
pathExists: fsPathExistsMock,
|
||||
stat: statMock
|
||||
stat: statMock,
|
||||
listFiles: listFilesMock
|
||||
},
|
||||
runtimeEnvironments: { call: runtimeEnvironmentTransportCallMock }
|
||||
}
|
||||
@@ -1233,6 +1243,45 @@ describe('createFilePathLinkProvider range bounds', () => {
|
||||
expect(window.api.shell.pathExists).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('linkifies a bare filename that misses cwd but is unique in the worktree (issue #5024)', async () => {
|
||||
setPlatform('Macintosh')
|
||||
listFilesMock.mockResolvedValue([
|
||||
'src/renderer/src/components/terminal-pane/TerminalContextMenu.test.tsx',
|
||||
'README.md'
|
||||
])
|
||||
const { provider, linkTooltip } = createProviderSetup(
|
||||
[makeBufferLine('TerminalContextMenu.test.tsx')],
|
||||
// The cwd-relative probe misses; the workspace fallback should resolve it.
|
||||
new Map([['active\0/repo/TerminalContextMenu.test.tsx', false]])
|
||||
)
|
||||
|
||||
const links = await new Promise<ILink[]>((resolve) => {
|
||||
provider.provideLinks(1, (provided) => resolve(provided ?? []))
|
||||
})
|
||||
|
||||
expect(links.map((link) => link.text)).toEqual(['TerminalContextMenu.test.tsx'])
|
||||
expect(listFilesMock).toHaveBeenCalledWith({ rootPath: '/repo' })
|
||||
links[0]!.hover?.({} as MouseEvent, links[0]!.text)
|
||||
expect(linkTooltip.textContent).toBe(
|
||||
'/repo/src/renderer/src/components/terminal-pane/TerminalContextMenu.test.tsx (⌘+click to open or ⇧⌘+click for default app)'
|
||||
)
|
||||
})
|
||||
|
||||
it('does not link a bare filename that is ambiguous in the worktree (issue #5024)', async () => {
|
||||
setPlatform('Macintosh')
|
||||
listFilesMock.mockResolvedValue(['src/a/index.ts', 'src/b/index.ts'])
|
||||
const { provider } = createProviderSetup(
|
||||
[makeBufferLine('index.ts')],
|
||||
new Map([['active\0/repo/index.ts', false]])
|
||||
)
|
||||
|
||||
const links = await new Promise<ILink[]>((resolve) => {
|
||||
provider.provideLinks(1, (provided) => resolve(provided ?? []))
|
||||
})
|
||||
|
||||
expect(links).toEqual([])
|
||||
})
|
||||
|
||||
it('linkifies a known worktree root printed with a trailing slash', async () => {
|
||||
setPlatform('Macintosh')
|
||||
storeState.worktreesByRepo = {
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import type { IDisposable, ILink, ILinkProvider, Terminal } from '@xterm/xterm'
|
||||
import type { IDisposable, ILinkProvider, Terminal } from '@xterm/xterm'
|
||||
import {
|
||||
extractTerminalFileLinkCandidates,
|
||||
extractTerminalFileLinks,
|
||||
@@ -20,8 +20,7 @@ import {
|
||||
import {
|
||||
buildHardWrappedPathLogicalLineCandidates,
|
||||
buildWrappedLogicalLine,
|
||||
rangeForParsedFileLink,
|
||||
type WrappedLogicalLine
|
||||
rangeForParsedFileLink
|
||||
} from './wrapped-terminal-link-ranges'
|
||||
import {
|
||||
getTerminalPathExistsCacheKey,
|
||||
@@ -37,6 +36,9 @@ import {
|
||||
isMacPlatform
|
||||
} from './terminal-link-open-hints'
|
||||
import { resolveKnownWorktreeRootPathLink } from './terminal-worktree-path-link'
|
||||
import { resolveWorkspaceFileByBasename } from './terminal-workspace-file-resolution'
|
||||
import { preferLongestNonOverlappingLinks } from './terminal-link-overlap-selection'
|
||||
import type { ProvidedFileLink } from './terminal-provided-file-link'
|
||||
|
||||
export { openDetectedFilePath } from './terminal-file-open-routing'
|
||||
export { openFilePathLinkAtBufferPosition } from './terminal-file-link-hit-testing'
|
||||
@@ -54,38 +56,6 @@ export type LinkHandlerDeps = {
|
||||
getRuntimeEnvironmentIdForPane?: (paneId: number) => string | null
|
||||
}
|
||||
|
||||
type ProvidedFileLink = {
|
||||
link: ILink
|
||||
logicalLine: WrappedLogicalLine
|
||||
}
|
||||
|
||||
function rangesOverlap(left: ILink['range'], right: ILink['range']): boolean {
|
||||
const leftStartsAfterRightEnds =
|
||||
left.start.y > right.end.y || (left.start.y === right.end.y && left.start.x > right.end.x)
|
||||
const rightStartsAfterLeftEnds =
|
||||
right.start.y > left.end.y || (right.start.y === left.end.y && right.start.x > left.end.x)
|
||||
return !leftStartsAfterRightEnds && !rightStartsAfterLeftEnds
|
||||
}
|
||||
|
||||
function preferLongestNonOverlappingLinks(links: ProvidedFileLink[]): ProvidedFileLink[] {
|
||||
const selected: ProvidedFileLink[] = []
|
||||
const byLengthDescending = [...links].sort(
|
||||
(a, b) =>
|
||||
b.link.text.length - a.link.text.length ||
|
||||
a.link.range.start.y - b.link.range.start.y ||
|
||||
a.link.range.start.x - b.link.range.start.x
|
||||
)
|
||||
for (const link of byLengthDescending) {
|
||||
if (!selected.some((existing) => rangesOverlap(existing.link.range, link.link.range))) {
|
||||
selected.push(link)
|
||||
}
|
||||
}
|
||||
return selected.sort(
|
||||
(a, b) =>
|
||||
a.link.range.start.y - b.link.range.start.y || a.link.range.start.x - b.link.range.start.x
|
||||
)
|
||||
}
|
||||
|
||||
export function createFilePathLinkProvider(
|
||||
paneId: number,
|
||||
deps: LinkHandlerDeps,
|
||||
@@ -155,6 +125,10 @@ export function createFilePathLinkProvider(
|
||||
if (/[\\/]$/.test(parsed.pathText) && !worktreeRootLink) {
|
||||
return null
|
||||
}
|
||||
// Why: the cwd-relative path is the default target, but a bare
|
||||
// filename may instead resolve to a file nested elsewhere in the
|
||||
// worktree (see the workspace fallback below).
|
||||
let linkAbsolutePath = resolved.absolutePath
|
||||
// Why: exact known workspace roots must stay clickable for SSH or
|
||||
// stale local paths even when filesystem probing says "missing".
|
||||
if (!worktreeRootLink) {
|
||||
@@ -166,7 +140,22 @@ export function createFilePathLinkProvider(
|
||||
: await window.api.shell.pathExists(resolved.absolutePath))
|
||||
writeTerminalPathExistsCache(pathExistsCache, cacheKey, exists)
|
||||
if (!exists) {
|
||||
return null
|
||||
// Why (issue #5024): a bare filename that doesn't exist at the
|
||||
// cwd often refers to a file nested elsewhere in the worktree.
|
||||
// Resolve it against the worktree's own file list (local/SSH;
|
||||
// runtime hosts are skipped). Only a unique match links.
|
||||
const workspaceMatch =
|
||||
!isRemoteRuntimePath && !/[\\/]/.test(parsed.pathText)
|
||||
? await resolveWorkspaceFileByBasename({
|
||||
basename: parsed.pathText,
|
||||
worktreePath,
|
||||
connectionId: fileContext.connectionId
|
||||
})
|
||||
: null
|
||||
if (!workspaceMatch) {
|
||||
return null
|
||||
}
|
||||
linkAbsolutePath = workspaceMatch
|
||||
}
|
||||
}
|
||||
|
||||
@@ -179,7 +168,7 @@ export function createFilePathLinkProvider(
|
||||
if (!isTerminalLinkActivation(event)) {
|
||||
return
|
||||
}
|
||||
openDetectedFilePath(resolved.absolutePath, resolved.line, resolved.column, {
|
||||
openDetectedFilePath(linkAbsolutePath, resolved.line, resolved.column, {
|
||||
worktreeId,
|
||||
worktreePath,
|
||||
runtimeEnvironmentId,
|
||||
@@ -191,16 +180,16 @@ export function createFilePathLinkProvider(
|
||||
// default escape hatch; remote paths may not exist locally.
|
||||
const canOpenWithSystemDefault = shouldOpenTerminalFileWithSystemDefault(
|
||||
fileContext,
|
||||
resolved.absolutePath
|
||||
linkAbsolutePath
|
||||
)
|
||||
const hint = worktreeRootLink
|
||||
? getTerminalWorktreePathOpenHint(canOpenWithSystemDefault)
|
||||
: canOpenWithSystemDefault
|
||||
? isHtmlFilePath(resolved.absolutePath)
|
||||
? isHtmlFilePath(linkAbsolutePath)
|
||||
? getTerminalHtmlFileOpenHint()
|
||||
: openLinkHint
|
||||
: getTerminalOrcaFileOpenHint()
|
||||
linkTooltip.textContent = `${resolved.absolutePath} (${hint})`
|
||||
linkTooltip.textContent = `${linkAbsolutePath} (${hint})`
|
||||
linkTooltip.style.display = ''
|
||||
},
|
||||
leave: () => {
|
||||
|
||||
@@ -0,0 +1,32 @@
|
||||
import type { ILink } from '@xterm/xterm'
|
||||
import type { ProvidedFileLink } from './terminal-provided-file-link'
|
||||
|
||||
function rangesOverlap(left: ILink['range'], right: ILink['range']): boolean {
|
||||
const leftStartsAfterRightEnds =
|
||||
left.start.y > right.end.y || (left.start.y === right.end.y && left.start.x > right.end.x)
|
||||
const rightStartsAfterLeftEnds =
|
||||
right.start.y > left.end.y || (right.start.y === left.end.y && right.start.x > left.end.x)
|
||||
return !leftStartsAfterRightEnds && !rightStartsAfterLeftEnds
|
||||
}
|
||||
|
||||
// Why: a wrapped path and a bare filename inside it can both match; keep the
|
||||
// longest link per overlapping span so the user gets the whole path, not a
|
||||
// fragment, then restore document order for stable rendering.
|
||||
export function preferLongestNonOverlappingLinks(links: ProvidedFileLink[]): ProvidedFileLink[] {
|
||||
const selected: ProvidedFileLink[] = []
|
||||
const byLengthDescending = [...links].sort(
|
||||
(a, b) =>
|
||||
b.link.text.length - a.link.text.length ||
|
||||
a.link.range.start.y - b.link.range.start.y ||
|
||||
a.link.range.start.x - b.link.range.start.x
|
||||
)
|
||||
for (const link of byLengthDescending) {
|
||||
if (!selected.some((existing) => rangesOverlap(existing.link.range, link.link.range))) {
|
||||
selected.push(link)
|
||||
}
|
||||
}
|
||||
return selected.sort(
|
||||
(a, b) =>
|
||||
a.link.range.start.y - b.link.range.start.y || a.link.range.start.x - b.link.range.start.x
|
||||
)
|
||||
}
|
||||
@@ -0,0 +1,10 @@
|
||||
import type { ILink } from '@xterm/xterm'
|
||||
import type { WrappedLogicalLine } from './wrapped-terminal-link-ranges'
|
||||
|
||||
// Why: a candidate link paired with the logical line it came from. Lives in its
|
||||
// own module so the link provider and the overlap selector can share the type
|
||||
// without an import cycle between handlers and overlap-selection.
|
||||
export type ProvidedFileLink = {
|
||||
link: ILink
|
||||
logicalLine: WrappedLogicalLine
|
||||
}
|
||||
@@ -0,0 +1,106 @@
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
import {
|
||||
__resetWorkspaceFileIndexCacheForTest,
|
||||
resolveWorkspaceFileByBasename
|
||||
} from './terminal-workspace-file-resolution'
|
||||
|
||||
const WT = '/Users/me/repo'
|
||||
|
||||
afterEach(() => {
|
||||
__resetWorkspaceFileIndexCacheForTest()
|
||||
})
|
||||
|
||||
describe('resolveWorkspaceFileByBasename', () => {
|
||||
it('resolves a bare filename to a unique nested workspace file (issue #5024)', async () => {
|
||||
const listFiles = vi.fn(async () => [
|
||||
'src/a/Foo.ts',
|
||||
'src/renderer/src/components/terminal-pane/TerminalContextMenu.test.tsx',
|
||||
'README.md'
|
||||
])
|
||||
const result = await resolveWorkspaceFileByBasename({
|
||||
basename: 'TerminalContextMenu.test.tsx',
|
||||
worktreePath: WT,
|
||||
listFiles,
|
||||
now: 1000
|
||||
})
|
||||
expect(result).toBe(
|
||||
'/Users/me/repo/src/renderer/src/components/terminal-pane/TerminalContextMenu.test.tsx'
|
||||
)
|
||||
expect(listFiles).toHaveBeenCalledWith({ rootPath: WT })
|
||||
})
|
||||
|
||||
it('returns null when multiple files share the basename (ambiguous, never guesses)', async () => {
|
||||
const listFiles = vi.fn(async () => ['a/index.ts', 'b/index.ts'])
|
||||
expect(
|
||||
await resolveWorkspaceFileByBasename({
|
||||
basename: 'index.ts',
|
||||
worktreePath: WT,
|
||||
listFiles,
|
||||
now: 1000
|
||||
})
|
||||
).toBeNull()
|
||||
})
|
||||
|
||||
it('returns null when no file matches', async () => {
|
||||
const listFiles = vi.fn(async () => ['a/x.ts'])
|
||||
expect(
|
||||
await resolveWorkspaceFileByBasename({
|
||||
basename: 'missing.ts',
|
||||
worktreePath: WT,
|
||||
listFiles,
|
||||
now: 1000
|
||||
})
|
||||
).toBeNull()
|
||||
})
|
||||
|
||||
it('caches the file list within the TTL and refetches once it expires', async () => {
|
||||
const listFiles = vi.fn(async () => ['a/Foo.ts'])
|
||||
await resolveWorkspaceFileByBasename({
|
||||
basename: 'Foo.ts',
|
||||
worktreePath: WT,
|
||||
listFiles,
|
||||
now: 1000
|
||||
})
|
||||
await resolveWorkspaceFileByBasename({
|
||||
basename: 'Foo.ts',
|
||||
worktreePath: WT,
|
||||
listFiles,
|
||||
now: 5000
|
||||
})
|
||||
expect(listFiles).toHaveBeenCalledTimes(1)
|
||||
await resolveWorkspaceFileByBasename({
|
||||
basename: 'Foo.ts',
|
||||
worktreePath: WT,
|
||||
listFiles,
|
||||
now: 1000 + 20_000
|
||||
})
|
||||
expect(listFiles).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
|
||||
it('keys the cache by connection and forwards connectionId for SSH worktrees', async () => {
|
||||
const remote = vi.fn(async () => ['y/remote.ts'])
|
||||
const result = await resolveWorkspaceFileByBasename({
|
||||
basename: 'remote.ts',
|
||||
worktreePath: WT,
|
||||
connectionId: 'ssh-1',
|
||||
listFiles: remote,
|
||||
now: 1
|
||||
})
|
||||
expect(result).toBe('/Users/me/repo/y/remote.ts')
|
||||
expect(remote).toHaveBeenCalledWith({ rootPath: WT, connectionId: 'ssh-1' })
|
||||
})
|
||||
|
||||
it('returns null without throwing when listing fails', async () => {
|
||||
const listFiles = vi.fn(async () => {
|
||||
throw new Error('listing unavailable')
|
||||
})
|
||||
expect(
|
||||
await resolveWorkspaceFileByBasename({
|
||||
basename: 'x.ts',
|
||||
worktreePath: WT,
|
||||
listFiles,
|
||||
now: 1
|
||||
})
|
||||
).toBeNull()
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,86 @@
|
||||
import { joinAbsolutePath } from '@/lib/terminal-path-normalization'
|
||||
|
||||
type WorkspaceFileIndexEntry = { fetchedAt: number; basenameToPaths: Map<string, string[]> }
|
||||
|
||||
type ListWorkspaceFiles = (args: { rootPath: string; connectionId?: string }) => Promise<string[]>
|
||||
|
||||
// Why: the link provider runs per hovered line, so cache the worktree's file
|
||||
// list (the same listing Quick Open uses) and refresh it on a short TTL rather
|
||||
// than re-listing on every lookup.
|
||||
const WORKSPACE_FILE_INDEX_TTL_MS = 15_000
|
||||
const workspaceFileIndexCache = new Map<string, WorkspaceFileIndexEntry>()
|
||||
|
||||
function basenameOf(relativePath: string): string {
|
||||
const normalized = relativePath.replace(/\\/g, '/')
|
||||
const slash = normalized.lastIndexOf('/')
|
||||
return slash === -1 ? normalized : normalized.slice(slash + 1)
|
||||
}
|
||||
|
||||
function buildBasenameIndex(files: readonly string[]): Map<string, string[]> {
|
||||
const index = new Map<string, string[]>()
|
||||
for (const relativePath of files) {
|
||||
const basename = basenameOf(relativePath)
|
||||
const existing = index.get(basename)
|
||||
if (existing) {
|
||||
existing.push(relativePath)
|
||||
} else {
|
||||
index.set(basename, [relativePath])
|
||||
}
|
||||
}
|
||||
return index
|
||||
}
|
||||
|
||||
export type ResolveWorkspaceFileArgs = {
|
||||
basename: string
|
||||
worktreePath: string
|
||||
connectionId?: string | null
|
||||
/** Test seams. */
|
||||
now?: number
|
||||
listFiles?: ListWorkspaceFiles
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve a bare filename to a file nested elsewhere in the worktree.
|
||||
*
|
||||
* Why (issue #5024): agent output frequently references a repo file by bare
|
||||
* name (e.g. `TerminalContextMenu.test.tsx`), which does not exist at the
|
||||
* terminal's cwd root, so the link provider's cwd-relative existence check
|
||||
* misses it. Falling back to the worktree's own file list makes those mentions
|
||||
* clickable. Only a UNIQUE basename match is returned — multiple files sharing
|
||||
* a name are ambiguous and left unlinked rather than guessed.
|
||||
*/
|
||||
export async function resolveWorkspaceFileByBasename(
|
||||
args: ResolveWorkspaceFileArgs
|
||||
): Promise<string | null> {
|
||||
const { basename, worktreePath } = args
|
||||
if (!basename || !worktreePath) {
|
||||
return null
|
||||
}
|
||||
const connectionId = args.connectionId ?? undefined
|
||||
const now = args.now ?? Date.now()
|
||||
const cacheKey = `${connectionId ?? 'local'}::${worktreePath}`
|
||||
|
||||
let entry = workspaceFileIndexCache.get(cacheKey)
|
||||
if (!entry || now - entry.fetchedAt > WORKSPACE_FILE_INDEX_TTL_MS) {
|
||||
const list = args.listFiles ?? ((listArgs) => window.api.fs.listFiles(listArgs))
|
||||
let files: string[]
|
||||
try {
|
||||
files = await list({ rootPath: worktreePath, ...(connectionId ? { connectionId } : {}) })
|
||||
} catch {
|
||||
// Best-effort: a failed listing must not break link detection.
|
||||
return null
|
||||
}
|
||||
entry = { fetchedAt: now, basenameToPaths: buildBasenameIndex(files) }
|
||||
workspaceFileIndexCache.set(cacheKey, entry)
|
||||
}
|
||||
|
||||
const matches = entry.basenameToPaths.get(basename)
|
||||
if (!matches || matches.length !== 1) {
|
||||
return null
|
||||
}
|
||||
return joinAbsolutePath(worktreePath, matches[0])
|
||||
}
|
||||
|
||||
export function __resetWorkspaceFileIndexCacheForTest(): void {
|
||||
workspaceFileIndexCache.clear()
|
||||
}
|
||||
Reference in New Issue
Block a user