From b10177b8e4b6609c41b5cfc351f66796d90ebb84 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 15 May 2026 14:01:27 -0700 Subject: [PATCH] Fix code pointer line reveals --- .../src/components/editor/MarkdownPreview.tsx | 33 ++++--- .../src/components/editor/MonacoEditor.tsx | 85 +++++++++++-------- .../editor/MonacoGutterContextMenu.tsx | 9 +- .../components/editor/line-copy-path.test.ts | 10 +++ .../src/components/editor/line-copy-path.ts | 3 + .../editor/markdown-internal-links.test.ts | 11 +++ .../editor/markdown-internal-links.ts | 19 ++++- .../src/components/editor/monaco-reveal.ts | 2 + src/renderer/src/store/slices/editor.test.ts | 24 ++++++ src/renderer/src/store/slices/editor.ts | 41 +++++---- 10 files changed, 170 insertions(+), 67 deletions(-) create mode 100644 src/renderer/src/components/editor/line-copy-path.test.ts create mode 100644 src/renderer/src/components/editor/line-copy-path.ts diff --git a/src/renderer/src/components/editor/MarkdownPreview.tsx b/src/renderer/src/components/editor/MarkdownPreview.tsx index 90d755ce0..29cda7cd5 100644 --- a/src/renderer/src/components/editor/MarkdownPreview.tsx +++ b/src/renderer/src/components/editor/MarkdownPreview.tsx @@ -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, diff --git a/src/renderer/src/components/editor/MonacoEditor.tsx b/src/renderer/src/components/editor/MonacoEditor.tsx index fe966e773..82df9211a 100644 --- a/src/renderer/src/components/editor/MonacoEditor.tsx +++ b/src/renderer/src/components/editor/MonacoEditor.tsx @@ -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(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(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 diff --git a/src/renderer/src/components/editor/MonacoGutterContextMenu.tsx b/src/renderer/src/components/editor/MonacoGutterContextMenu.tsx index a702e9d6d..1b6db18d4 100644 --- a/src/renderer/src/components/editor/MonacoGutterContextMenu.tsx +++ b/src/renderer/src/components/editor/MonacoGutterContextMenu.tsx @@ -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({ /> - window.api.ui.writeClipboardText(`${filePath}#L${line}`)}> + window.api.ui.writeClipboardText(formatPathLineReference(filePath, line))} + > Copy Path to Line window.api.ui.writeClipboardText(`${relativePath}#L${line}`)} + onSelect={() => + window.api.ui.writeClipboardText(formatPathLineReference(relativePath, line)) + } > Copy Rel. Path to Line diff --git a/src/renderer/src/components/editor/line-copy-path.test.ts b/src/renderer/src/components/editor/line-copy-path.test.ts new file mode 100644 index 000000000..b4d90556f --- /dev/null +++ b/src/renderer/src/components/editor/line-copy-path.test.ts @@ -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' + ) + }) +}) diff --git a/src/renderer/src/components/editor/line-copy-path.ts b/src/renderer/src/components/editor/line-copy-path.ts new file mode 100644 index 000000000..47623c1ff --- /dev/null +++ b/src/renderer/src/components/editor/line-copy-path.ts @@ -0,0 +1,3 @@ +export function formatPathLineReference(filePath: string, line: number): string { + return `${filePath}:${line}` +} diff --git a/src/renderer/src/components/editor/markdown-internal-links.test.ts b/src/renderer/src/components/editor/markdown-internal-links.test.ts index 11a9824e6..95e8e6407 100644 --- a/src/renderer/src/components/editor/markdown-internal-links.test.ts +++ b/src/renderer/src/components/editor/markdown-internal-links.test.ts @@ -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 }) diff --git a/src/renderer/src/components/editor/markdown-internal-links.ts b/src/renderer/src/components/editor/markdown-internal-links.ts index c2512629a..deac13b4c 100644 --- a/src/renderer/src/components/editor/markdown-internal-links.ts +++ b/src/renderer/src/components/editor/markdown-internal-links.ts @@ -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 } } diff --git a/src/renderer/src/components/editor/monaco-reveal.ts b/src/renderer/src/components/editor/monaco-reveal.ts index 3ab623da3..1a6d1510f 100644 --- a/src/renderer/src/components/editor/monaco-reveal.ts +++ b/src/renderer/src/components/editor/monaco-reveal.ts @@ -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, diff --git a/src/renderer/src/store/slices/editor.test.ts b/src/renderer/src/store/slices/editor.test.ts index 875946f1d..9ec1e7a76 100644 --- a/src/renderer/src/store/slices/editor.test.ts +++ b/src/renderer/src/store/slices/editor.test.ts @@ -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', { diff --git a/src/renderer/src/store/slices/editor.ts b/src/renderer/src/store/slices/editor.ts index 74fe667f4..b93ceeee6 100644 --- a/src/renderer/src/store/slices/editor.ts +++ b/src/renderer/src/store/slices/editor.ts @@ -157,6 +157,27 @@ export type ClosedEditorTabSnapshot = Omit 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 = (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 = (s recordReplacedPreview: true } ) + if (line !== undefined) { + scheduleEditorLineReveal(get, target.absolutePath, line, column) + } return } @@ -2311,21 +2336,7 @@ export const createEditorSlice: StateCreator = (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) } },