Fix code pointer line reveals

This commit is contained in:
Neil 2026-05-15 14:01:27 -07:00 committed by GitHub
parent f5cc8196f9
commit b10177b8e4
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 170 additions and 67 deletions

View File

@ -800,13 +800,18 @@ export default function MarkdownPreview({
return
}
const classified = resolveMarkdownLinkTarget(href, filePath, worktreeRoot)
if (classified?.kind === 'markdown') {
if (
classified?.kind === 'markdown' ||
(classified?.kind === 'file' && classified.line !== undefined)
) {
// Why: use the classifier's stripped absolutePath (no `:line:col`
// or `#L10` suffix) so the OS handler receives a clean file URI.
const cleanUri = absolutePathToFileUri(classified.absolutePath)
void window.api.shell.pathExists(classified.absolutePath).then((exists) => {
if (!exists) {
toast.error(`File not found: ${classified.relativePath}`)
toast.error(
`File not found: ${classified.relativePath ?? classified.absolutePath}`
)
return
}
void window.api.shell.openFileUri(cleanUri)
@ -832,12 +837,19 @@ export default function MarkdownPreview({
return
}
const absolutePath = fileUrlToAbsolutePath(target)
const classified = resolveMarkdownLinkTarget(href, filePath, worktreeRoot)
const classifiedFileTarget =
classified?.kind === 'markdown' || classified?.kind === 'file' ? classified : null
const absolutePath = classifiedFileTarget?.absolutePath ?? fileUrlToAbsolutePath(target)
if (!absolutePath) {
return
}
const lineTarget =
classifiedFileTarget?.line !== undefined
? { line: classifiedFileTarget.line, column: classifiedFileTarget.column }
: parseLineTarget(target.hash)
if (absolutePath === filePath && target.hash) {
if (absolutePath === filePath && target.hash && !lineTarget) {
void scrollToAnchor(target.hash.slice(1))
return
}
@ -874,13 +886,12 @@ export default function MarkdownPreview({
const relativePath = absolutePath.slice(targetWorktree.path.length + 1)
const language = detectLanguage(absolutePath)
// Why: line-target fragments like #L10 or #L10C5 should open the
// source editor and reveal the line, not open a preview tab that
// treats "L10" as a heading anchor.
const lineTarget = parseLineTarget(target.hash)
if (language === 'markdown' && lineTarget) {
const fileId = absolutePath
setMarkdownViewMode(fileId, 'source')
// Why: line targets like #L10 and path.ts:10 should reveal in Monaco,
// not open a preview tab or a literal path with the suffix included.
if (lineTarget) {
if (language === 'markdown') {
setMarkdownViewMode(absolutePath, 'source')
}
openFile({
filePath: absolutePath,
relativePath,

View File

@ -11,7 +11,7 @@ import '@/lib/monaco-setup'
import { computeEditorFontSize } from '@/lib/editor-font-zoom'
import { useContextualCopySetup } from './useContextualCopySetup'
import { performReveal } from './monaco-reveal'
import { MAX_REVEAL_CONTENT_WAIT_FRAMES, performReveal } from './monaco-reveal'
import { syncContentOnMount, syncContentUpdate } from './monaco-content-sync'
import {
beginProgrammaticContentSync,
@ -104,6 +104,21 @@ export default function MonacoEditor({
settings?.terminalFontSize ?? 13,
editorFontZoomLevel
)
// Why: `keepCurrentModel` retains Monaco models across unmounts, and
// @monaco-editor/react skips its value→model sync on the first render after
// a remount. Without explicit sync, external file changes that arrived
// while the tab was unmounted leave the retained model showing stale text.
// contentRef lets handleMount read the current content without re-binding;
// lastSyncedContentRef lets the update effect distinguish our own onChange
// emissions from real prop drift.
// Invariant: the mount path (handleMount's syncContentOnMount call) MUST
// read `contentRef.current`, never `lastSyncedContentRef.current`. The
// useLayoutEffect below can run before mount with `editorRef.current === null`
// and bails without updating lastSyncedContentRef, so that ref may be stale
// pre-mount; only contentRef is guaranteed to reflect the latest prop.
const contentRef = useRef(content)
contentRef.current = content
const lastSyncedContentRef = useRef<string>(content)
const markdownComments = useMemo(
() =>
(allDiffComments ?? []).filter((c) => c.filePath === relativePath && isMarkdownComment(c)),
@ -197,46 +212,46 @@ export default function MonacoEditor({
onApplied?: () => void
) => {
cancelScheduledReveal()
let waitFrames = 0
// Why: the search click path already waits two frames before publishing
// the reveal intent, but Monaco can still mount before its viewport math
// settles. Deferring the actual reveal by two editor-owned frames keeps
// scroll-to-match and inline highlight deterministic on fresh opens.
revealRafRef.current = requestAnimationFrame(() => {
revealInnerRafRef.current = requestAnimationFrame(() => {
performReveal(
editorInstance,
line,
column,
matchLength,
clearTransientRevealHighlight,
revealDecorationRef,
revealHighlightTimerRef
)
onApplied?.()
revealRafRef.current = null
revealInnerRafRef.current = null
const schedule = (): void => {
// Why: the search click path already waits two frames before publishing
// the reveal intent, but Monaco can still mount before its viewport math
// settles. Deferring the actual reveal by two editor-owned frames keeps
// scroll-to-match and inline highlight deterministic on fresh opens.
revealRafRef.current = requestAnimationFrame(() => {
revealInnerRafRef.current = requestAnimationFrame(() => {
revealRafRef.current = null
revealInnerRafRef.current = null
const modelLineCount = editorInstance.getModel()?.getLineCount() ?? 0
if (line > 1 && modelLineCount < line && waitFrames < MAX_REVEAL_CONTENT_WAIT_FRAMES) {
// Why: fresh file opens can mount Monaco against an empty one-line
// model before the async file read arrives. Waiting prevents the
// requested line from being clamped to 1 and then cleared.
waitFrames += 2
schedule()
return
}
performReveal(
editorInstance,
line,
column,
matchLength,
clearTransientRevealHighlight,
revealDecorationRef,
revealHighlightTimerRef
)
onApplied?.()
})
})
})
}
schedule()
},
[cancelScheduledReveal, clearTransientRevealHighlight]
)
// Why: `keepCurrentModel` retains Monaco models across unmounts, and
// @monaco-editor/react skips its value→model sync on the first render after
// a remount. Without explicit sync, external file changes that arrived
// while the tab was unmounted leave the retained model showing stale text.
// contentRef lets handleMount read the current content without re-binding;
// lastSyncedContentRef lets the update effect distinguish our own onChange
// emissions from real prop drift.
// Invariant: the mount path (handleMount's syncContentOnMount call) MUST
// read `contentRef.current`, never `lastSyncedContentRef.current`. The
// useLayoutEffect below can run before mount with `editorRef.current === null`
// and bails without updating lastSyncedContentRef, so that ref may be stale
// pre-mount; only contentRef is guaranteed to reflect the latest prop.
const contentRef = useRef(content)
contentRef.current = content
const lastSyncedContentRef = useRef<string>(content)
// Why: Monaco model reconciliation reuses real edit operations so retained
// models keep sane undo behavior. Those edits are programmatic, not user
// typing, so split panes must suppress the resulting onChange callback or a

View File

@ -10,6 +10,7 @@ import { useAppStore } from '@/store'
import { getConnectionId } from '@/lib/connection-context'
import { findWorktreeById } from '@/store/slices/worktree-helpers'
import { getRuntimeGitRemoteFileUrl } from '@/runtime/runtime-git-client'
import { formatPathLineReference } from './line-copy-path'
type MonacoGutterContextMenuProps = {
open: boolean
@ -39,12 +40,16 @@ export function MonacoGutterContextMenu({
/>
</DropdownMenuTrigger>
<DropdownMenuContent sideOffset={0} align="start">
<DropdownMenuItem onSelect={() => window.api.ui.writeClipboardText(`${filePath}#L${line}`)}>
<DropdownMenuItem
onSelect={() => window.api.ui.writeClipboardText(formatPathLineReference(filePath, line))}
>
<Copy className="w-3.5 h-3.5 mr-1.5" />
Copy Path to Line
</DropdownMenuItem>
<DropdownMenuItem
onSelect={() => window.api.ui.writeClipboardText(`${relativePath}#L${line}`)}
onSelect={() =>
window.api.ui.writeClipboardText(formatPathLineReference(relativePath, line))
}
>
<Copy className="w-3.5 h-3.5 mr-1.5" />
Copy Rel. Path to Line

View File

@ -0,0 +1,10 @@
import { describe, expect, it } from 'vitest'
import { formatPathLineReference } from './line-copy-path'
describe('formatPathLineReference', () => {
it('uses the standard path:line format', () => {
expect(formatPathLineReference('src/components/PdfViewer.tsx', 142)).toBe(
'src/components/PdfViewer.tsx:142'
)
})
})

View File

@ -0,0 +1,3 @@
export function formatPathLineReference(filePath: string, line: number): string {
return `${filePath}:${line}`
}

View File

@ -44,6 +44,17 @@ describe('resolveMarkdownLinkTarget', () => {
expect(r).toMatchObject({ kind: 'markdown', line: 10, column: 5 })
})
it('extracts line+col from non-markdown file links', () => {
const r = resolveMarkdownLinkTarget('../src/PdfViewer.tsx:142:7', SOURCE, ROOT)
expect(r).toMatchObject({
kind: 'file',
absolutePath: '/repo/src/PdfViewer.tsx',
relativePath: 'src/PdfViewer.tsx',
line: 142,
column: 7
})
})
it('ignores non-line-col hashes', () => {
const r = resolveMarkdownLinkTarget('./guide.md#intro', SOURCE, ROOT)
expect(r).toMatchObject({ kind: 'markdown', line: undefined, column: undefined })

View File

@ -15,7 +15,14 @@ export type MarkdownLinkTarget =
line?: number
column?: number
}
| { kind: 'file'; uri: string; absolutePath: string; relativePath?: string }
| {
kind: 'file'
uri: string
absolutePath: string
relativePath?: string
line?: number
column?: number
}
// Why: renderer runs with sandbox + contextIsolation, so process.platform is
// unavailable. navigator.userAgent is the portable fallback (AGENTS.md).
@ -210,8 +217,12 @@ export function resolveMarkdownLinkTarget(
// approximation; for trailing-colon paths there's no clean URL form,
// so we reconstruct from the stripped absolute path.
const cleanUri = line === undefined ? resolved.toString() : toFileUrl(pathForClassification)
if (line === undefined) {
return { kind: 'file', uri: cleanUri, absolutePath: pathForClassification, relativePath }
return {
kind: 'file',
uri: cleanUri,
absolutePath: pathForClassification,
relativePath,
line,
column
}
return { kind: 'file', uri: cleanUri, absolutePath: pathForClassification, relativePath }
}

View File

@ -2,6 +2,8 @@ import type React from 'react'
import type { editor } from 'monaco-editor'
import { computeMonacoRevealRange } from './monaco-reveal-range'
export const MAX_REVEAL_CONTENT_WAIT_FRAMES = 120
/**
* Shared reveal logic used by both onMount and useEffect paths in MonacoEditor.
* Positions the cursor, optionally selects the match range, scrolls into center,

View File

@ -1457,6 +1457,30 @@ describe('createEditorSlice activateMarkdownLink', () => {
expect(openFileUriMock).not.toHaveBeenCalled()
})
it('reveals line targets for non-markdown file links', async () => {
const store = createEditorStore()
await store.getState().activateMarkdownLink('../src/PdfViewer.tsx:142:7', {
sourceFilePath: '/repo/docs/note.md',
worktreeId: 'wt-1',
worktreeRoot: '/repo'
})
expect(store.getState().openFiles).toEqual([
expect.objectContaining({
filePath: '/repo/src/PdfViewer.tsx',
relativePath: 'src/PdfViewer.tsx',
mode: 'edit',
isPreview: true
})
])
expect(store.getState().pendingEditorReveal).toEqual({
filePath: '/repo/src/PdfViewer.tsx',
line: 142,
column: 7,
matchLength: 0
})
})
it('opens explicit file URLs inside the worktree in Orca', async () => {
const store = createEditorStore()
await store.getState().activateMarkdownLink('file:///repo/docs/image.png', {

View File

@ -157,6 +157,27 @@ export type ClosedEditorTabSnapshot = Omit<OpenFile, 'id' | 'isDirty'>
const MAX_RECENT_CLOSED_EDITOR_TABS = 10
function scheduleEditorLineReveal(
get: () => AppState,
filePath: string,
line: number,
column?: number
): void {
// Why: openFile can replace a preview and remount Monaco asynchronously; the
// reveal must land after that remount or the old editor can clear it.
get().setPendingEditorReveal(null)
requestAnimationFrame(() => {
requestAnimationFrame(() => {
get().setPendingEditorReveal({
filePath,
line,
column: column ?? 1,
matchLength: 0
})
})
})
}
export type EditorSlice = {
// Why: #300 originally kept EditorPanel mounted while hidden so unsaved
// drafts and autosave timers could survive tab switches. Drafts live in the
@ -2226,6 +2247,7 @@ export const createEditorSlice: StateCreator<AppState, [], [], EditorSlice> = (s
return
}
if (target.kind === 'file') {
const { line, column } = target
if (target.relativePath === undefined) {
if (sourceSettings?.activeRuntimeEnvironmentId?.trim()) {
// Why: a file:// link outside the worktree is a client-local escape
@ -2255,6 +2277,9 @@ export const createEditorSlice: StateCreator<AppState, [], [], EditorSlice> = (s
recordReplacedPreview: true
}
)
if (line !== undefined) {
scheduleEditorLineReveal(get, target.absolutePath, line, column)
}
return
}
@ -2311,21 +2336,7 @@ export const createEditorSlice: StateCreator<AppState, [], [], EditorSlice> = (s
}
if (line !== undefined) {
// Why: double-RAF matches search-match-open.ts. openFile may replace a
// preview, remounting Monaco asynchronously; the mount's own
// setPendingEditorReveal(null) would otherwise clobber a reveal scheduled
// in the same tick.
get().setPendingEditorReveal(null)
requestAnimationFrame(() => {
requestAnimationFrame(() => {
get().setPendingEditorReveal({
filePath: absolutePath,
line,
column: column ?? 1,
matchLength: 0
})
})
})
scheduleEditorLineReveal(get, absolutePath, line, column)
}
},