fix: cover agent file link review fixes (#1415)
This commit is contained in:
@@ -40,6 +40,9 @@ func Split(raw string) (path string, startLine, lineCount int) {
|
||||
return raw, 0, 0
|
||||
}
|
||||
colon := lastSep + 1 + rel
|
||||
if lastSep < 0 && isWindowsDrivePrefix(raw, colon) {
|
||||
return raw, 0, 0
|
||||
}
|
||||
base, suffix := raw[:colon], raw[colon+1:]
|
||||
if base == "" || suffix == "" {
|
||||
return raw, 0, 0
|
||||
@@ -51,6 +54,14 @@ func Split(raw string) (path string, startLine, lineCount int) {
|
||||
return base, start, count
|
||||
}
|
||||
|
||||
func isWindowsDrivePrefix(raw string, colon int) bool {
|
||||
return colon == 1 && len(raw) >= 2 && isASCIIAlpha(raw[0])
|
||||
}
|
||||
|
||||
func isASCIIAlpha(b byte) bool {
|
||||
return (b >= 'A' && b <= 'Z') || (b >= 'a' && b <= 'z')
|
||||
}
|
||||
|
||||
// parseReadSelector validates the full selector tail (everything after the
|
||||
// first ':' in the final segment). Selector parts are joined by ':' so combos
|
||||
// such as "2-4:raw" and "raw:2-4" are accepted; each part must be a line-spec
|
||||
|
||||
@@ -62,6 +62,7 @@ func TestSplit_NoFalsePositives(t *testing.T) {
|
||||
"foo.go:bar", // non-numeric, non-keyword suffix
|
||||
"foo.go:1.2", // float-like, not a line spec
|
||||
`C:\Users\me\a.go`, // windows drive letter: suffix is not a line spec
|
||||
`C:43`, // windows drive-relative path, not a selector
|
||||
"dir:1/file.go", // colon lives in a directory component, not the file
|
||||
":43-94", // empty base
|
||||
"main.go:", // empty selector
|
||||
|
||||
@@ -208,7 +208,7 @@ describe("markdownComponents omp read selectors", () => {
|
||||
};
|
||||
});
|
||||
|
||||
it("opens file links with omp range and multi-range selectors in the editor", () => {
|
||||
it("opens file links with omp range selectors in the editor", () => {
|
||||
render(<Markdown>{"[readme](docs/README.md:16-20)"}</Markdown>);
|
||||
fireEvent.click(screen.getByRole("link", { name: "readme" }));
|
||||
expect(openFile).toHaveBeenCalledWith("docs/README.md");
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
"use client";
|
||||
|
||||
import { useState, memo, useCallback } from "react";
|
||||
import { useState, memo } from "react";
|
||||
import {
|
||||
IconCheck,
|
||||
IconX,
|
||||
@@ -16,7 +16,7 @@ import { DiffViewBlock } from "./diff-view-block";
|
||||
import { ExpandableRow } from "./expandable-row";
|
||||
import { transformFileMutation, type FileMutation } from "@/lib/diff";
|
||||
import { useExpandState } from "./use-expand-state";
|
||||
import { setPendingCursorPosition, scrollEditorIfMounted } from "@/hooks/use-file-editors";
|
||||
import { useOpenFileAtLine } from "@/hooks/use-file-editors";
|
||||
|
||||
type ModifyFilePayload = {
|
||||
file_path?: string;
|
||||
@@ -187,21 +187,7 @@ export const ToolEditMessage = memo(function ToolEditMessage({
|
||||
}
|
||||
};
|
||||
|
||||
// Navigate the editor to the first changed line. For a newly opened file the
|
||||
// pending position is consumed on editor mount; for an already-open file no
|
||||
// mount happens, so scroll the live editor directly.
|
||||
const handleOpenFile = useCallback(
|
||||
(path: string) => {
|
||||
if (startLine && startLine > 0) {
|
||||
setPendingCursorPosition(path, startLine, 1);
|
||||
onOpenFile?.(path);
|
||||
scrollEditorIfMounted(path, worktreePath ?? null, startLine, 1);
|
||||
return;
|
||||
}
|
||||
onOpenFile?.(path);
|
||||
},
|
||||
[onOpenFile, startLine, worktreePath],
|
||||
);
|
||||
const handleOpenFile = useOpenFileAtLine(onOpenFile, startLine, worktreePath);
|
||||
|
||||
return (
|
||||
<ExpandableRow
|
||||
|
||||
@@ -1,13 +1,13 @@
|
||||
"use client";
|
||||
|
||||
import { memo, useCallback } from "react";
|
||||
import { memo } from "react";
|
||||
import { IconCheck, IconX, IconFileCode2 } from "@tabler/icons-react";
|
||||
import { GridSpinner } from "@/components/grid-spinner";
|
||||
import { FilePathButton } from "./file-path-button";
|
||||
import type { Message } from "@/lib/types/http";
|
||||
import { ExpandableRow } from "./expandable-row";
|
||||
import { useExpandState } from "./use-expand-state";
|
||||
import { setPendingCursorPosition, scrollEditorIfMounted } from "@/hooks/use-file-editors";
|
||||
import { useOpenFileAtLine } from "@/hooks/use-file-editors";
|
||||
|
||||
type ReadFileOutput = {
|
||||
content?: string;
|
||||
@@ -86,21 +86,7 @@ export const ToolReadMessage = memo(function ToolReadMessage({
|
||||
parseReadMetadata(comment);
|
||||
const autoExpanded = status === "running";
|
||||
const { isExpanded, handleToggle } = useExpandState(status, autoExpanded);
|
||||
// Navigate the editor to the line the agent read (offset). For a newly opened
|
||||
// file the pending position is consumed on editor mount; for an already-open
|
||||
// file no mount happens, so scroll the live editor directly.
|
||||
const handleOpenFile = useCallback(
|
||||
(path: string) => {
|
||||
if (startLine && startLine > 0) {
|
||||
setPendingCursorPosition(path, startLine, 1);
|
||||
onOpenFile?.(path);
|
||||
scrollEditorIfMounted(path, worktreePath ?? null, startLine, 1);
|
||||
return;
|
||||
}
|
||||
onOpenFile?.(path);
|
||||
},
|
||||
[onOpenFile, startLine, worktreePath],
|
||||
);
|
||||
const handleOpenFile = useOpenFileAtLine(onOpenFile, startLine, worktreePath);
|
||||
|
||||
return (
|
||||
<ExpandableRow
|
||||
|
||||
@@ -0,0 +1,91 @@
|
||||
import { act, renderHook } from "@testing-library/react";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
const getMonacoInstance = vi.hoisted(() => vi.fn());
|
||||
const APP_PATH = "src/app.ts";
|
||||
const MISSING_PATH = "src/missing.ts";
|
||||
const WORKTREE_PATH = "/worktree";
|
||||
const WORKTREE_APP_PATH = "/worktree/src/app.ts";
|
||||
|
||||
vi.mock("@/components/editors/monaco/monaco-init", () => ({
|
||||
getMonacoInstance,
|
||||
}));
|
||||
|
||||
import {
|
||||
consumePendingCursorPosition,
|
||||
scrollEditorIfMounted,
|
||||
setPendingCursorPosition,
|
||||
useOpenFileAtLine,
|
||||
} from "./use-file-editors";
|
||||
|
||||
function createEditor(modelPath: string | null) {
|
||||
return {
|
||||
getModel: vi.fn(() => (modelPath ? { uri: { path: modelPath } } : null)),
|
||||
setPosition: vi.fn(),
|
||||
revealLineInCenter: vi.fn(),
|
||||
focus: vi.fn(),
|
||||
};
|
||||
}
|
||||
|
||||
describe("scrollEditorIfMounted", () => {
|
||||
afterEach(() => {
|
||||
getMonacoInstance.mockReset();
|
||||
consumePendingCursorPosition(APP_PATH);
|
||||
consumePendingCursorPosition(MISSING_PATH);
|
||||
});
|
||||
|
||||
it("scrolls the mounted Monaco editor that matches the worktree path", () => {
|
||||
const editor = createEditor(WORKTREE_APP_PATH);
|
||||
getMonacoInstance.mockReturnValue({ editor: { getEditors: () => [editor] } });
|
||||
setPendingCursorPosition(APP_PATH, 12, 1);
|
||||
|
||||
expect(scrollEditorIfMounted(APP_PATH, WORKTREE_PATH, 42, 3)).toBe(true);
|
||||
|
||||
expect(editor.setPosition).toHaveBeenCalledWith({ lineNumber: 42, column: 3 });
|
||||
expect(editor.revealLineInCenter).toHaveBeenCalledWith(42);
|
||||
expect(editor.focus).toHaveBeenCalledTimes(1);
|
||||
expect(consumePendingCursorPosition(APP_PATH)).toBeUndefined();
|
||||
});
|
||||
|
||||
it("returns false when no mounted Monaco editor matches the path", () => {
|
||||
const editor = createEditor("/worktree/src/other.ts");
|
||||
getMonacoInstance.mockReturnValue({ editor: { getEditors: () => [editor] } });
|
||||
setPendingCursorPosition(MISSING_PATH, 5, 1);
|
||||
|
||||
expect(scrollEditorIfMounted(MISSING_PATH, WORKTREE_PATH, 5, 1)).toBe(false);
|
||||
|
||||
expect(editor.setPosition).not.toHaveBeenCalled();
|
||||
expect(consumePendingCursorPosition(MISSING_PATH)).toEqual({ line: 5, column: 1 });
|
||||
});
|
||||
});
|
||||
|
||||
describe("useOpenFileAtLine", () => {
|
||||
afterEach(() => {
|
||||
getMonacoInstance.mockReset();
|
||||
consumePendingCursorPosition(APP_PATH);
|
||||
});
|
||||
|
||||
it("opens and scrolls an already-mounted file when a target line is present", () => {
|
||||
const openFile = vi.fn();
|
||||
const editor = createEditor(WORKTREE_APP_PATH);
|
||||
getMonacoInstance.mockReturnValue({ editor: { getEditors: () => [editor] } });
|
||||
const { result } = renderHook(() => useOpenFileAtLine(openFile, 88, WORKTREE_PATH));
|
||||
|
||||
act(() => result.current(APP_PATH));
|
||||
|
||||
expect(openFile).toHaveBeenCalledWith(APP_PATH);
|
||||
expect(editor.setPosition).toHaveBeenCalledWith({ lineNumber: 88, column: 1 });
|
||||
expect(consumePendingCursorPosition(APP_PATH)).toBeUndefined();
|
||||
});
|
||||
|
||||
it("opens without setting a pending position when the target line is missing", () => {
|
||||
const openFile = vi.fn();
|
||||
const { result } = renderHook(() => useOpenFileAtLine(openFile, undefined, WORKTREE_PATH));
|
||||
|
||||
act(() => result.current(APP_PATH));
|
||||
|
||||
expect(openFile).toHaveBeenCalledWith(APP_PATH);
|
||||
expect(getMonacoInstance).not.toHaveBeenCalled();
|
||||
expect(consumePendingCursorPosition(APP_PATH)).toBeUndefined();
|
||||
});
|
||||
});
|
||||
@@ -76,6 +76,25 @@ export function scrollEditorIfMounted(
|
||||
return false;
|
||||
}
|
||||
|
||||
export function useOpenFileAtLine(
|
||||
onOpenFile: ((path: string) => void) | undefined,
|
||||
startLine: number | undefined,
|
||||
worktreePath: string | null | undefined,
|
||||
) {
|
||||
return useCallback(
|
||||
(path: string) => {
|
||||
if (startLine && startLine > 0) {
|
||||
setPendingCursorPosition(path, startLine, 1);
|
||||
onOpenFile?.(path);
|
||||
scrollEditorIfMounted(path, worktreePath ?? null, startLine, 1);
|
||||
return;
|
||||
}
|
||||
onOpenFile?.(path);
|
||||
},
|
||||
[onOpenFile, startLine, worktreePath],
|
||||
);
|
||||
}
|
||||
|
||||
/** Read openFiles from the store without subscribing to changes. */
|
||||
function getOpenFiles() {
|
||||
return useDockviewStore.getState().openFiles;
|
||||
|
||||
Reference in New Issue
Block a user