diff --git a/.changeset/current-line-editor.md b/.changeset/current-line-editor.md new file mode 100644 index 00000000..5e185f1f --- /dev/null +++ b/.changeset/current-line-editor.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": minor +--- + +Open `$EDITOR` at the current line instead of the start of the selected hunk. diff --git a/src/ui/App.tsx b/src/ui/App.tsx index 88d9714f..14178680 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -417,6 +417,11 @@ export function App({ const currentLinePaintPending = currentLinePaintState.status === "pending" || (currentLinePaintState.status === "ready" && !currentLinePaintMatchesCursor); + /** The review stream's current line, or null when line-level navigation is off. */ + const activeLineCursor = useMemo( + () => (cursorLine === "off" ? null : review.lineCursor), + [cursorLine, review.lineCursor], + ); const sessionFileViews = useMemo( () => (extensions ? resolveExtensionFileViews(extensions.registry).views : []), [extensions], @@ -1222,7 +1227,7 @@ export function App({ /** Step one line: move the current line, or scroll the viewport when there is no marker. */ const stepDiffLine = (delta: number) => { - if (cursorLine === "off" || !review.lineCursor) { + if (!activeLineCursor) { scrollDiff(delta, "step"); return; } @@ -1569,6 +1574,7 @@ export function App({ const message = openSelectedFileInEditor({ basePath, file: selectedFile, + lineCursor: activeLineCursor, renderer, selectedHunk: review.selectedHunk, }); @@ -1582,6 +1588,7 @@ export function App({ triggerRefreshCurrentInput(); } }, [ + activeLineCursor, bootstrap.changeset.sourceLabel, bootstrap.input.kind, canRefreshCurrentInput, @@ -1729,8 +1736,7 @@ export function App({ const startUserNote = useCallback( (fileId?: string, hunkIndex?: number, target?: UserNoteLineTarget) => { const hoverTarget = fileId === undefined ? activeAddNoteTarget : null; - const keyboardTarget = - hoverTarget ?? (fileId === undefined && cursorLine !== "off" ? review.lineCursor : null); + const keyboardTarget = hoverTarget ?? (fileId === undefined ? activeLineCursor : null); const draft = review.startUserNote( fileId ?? keyboardTarget?.fileId, hunkIndex ?? keyboardTarget?.hunkIndex, @@ -1742,7 +1748,7 @@ export function App({ setFocusArea("note"); } }, - [activeAddNoteTarget, cursorLine, review.lineCursor, review.startUserNote], + [activeAddNoteTarget, activeLineCursor, review.startUserNote], ); /** Mark the inline draft note textarea as the active keyboard input. */ diff --git a/src/ui/AppHost.edit-in-editor.test.tsx b/src/ui/AppHost.edit-in-editor.test.tsx new file mode 100644 index 00000000..b36cadbe --- /dev/null +++ b/src/ui/AppHost.edit-in-editor.test.tsx @@ -0,0 +1,148 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { testRender } from "@opentui/react/test-utils"; +import { mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { act } from "react"; +import type { AppBootstrap } from "../core/types"; +import { createTestVcsAppBootstrap } from "../../test/helpers/app-bootstrap"; +import { createTestDiffFile, lines } from "../../test/helpers/diff-helpers"; + +const { AppHost } = await import("./AppHost"); + +const WIDE = { width: 200, height: 24 }; + +const BEFORE = lines( + "const alpha = 1;", + "const beta = 2;", + "const gamma = 3;", + "const delta = 4;", + "const epsilon = 5;", +); +const AFTER = lines( + "const alpha = 1;", + "const beta = 22222;", + "const gamma = 3;", + "const delta = 4;", + "const epsilon = 5;", +); + +const originalEditor = process.env.EDITOR; +const originalSpawnSync = Bun.spawnSync; +const tempDirs: string[] = []; + +let setup: Awaited> | undefined; + +function createTempWorkspace() { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "hunk-apphost-editor-"))); + tempDirs.push(dir); + writeFileSync(join(dir, "sample.ts"), AFTER); + return dir; +} + +function mockSpawnSync(implementation: typeof Bun.spawnSync) { + const mutableBun = Bun as unknown as { spawnSync: typeof Bun.spawnSync }; + mutableBun.spawnSync = implementation; +} + +/** Bootstrap one working-tree review whose file really exists under `sourceLabel`. */ +function createEditorBootstrap(sourceLabel: string): AppBootstrap { + return createTestVcsAppBootstrap({ + changesetId: "changeset:edit-in-editor", + initialMode: "stack", + sourceLabel, + files: [ + createTestDiffFile({ + after: AFTER, + agent: false, + before: BEFORE, + context: 3, + id: "sample", + path: "sample.ts", + }), + ], + }); +} + +async function flush(target: Awaited>) { + await act(async () => { + await target.renderOnce(); + await Bun.sleep(0); + await target.renderOnce(); + }); +} + +async function pressKeys(target: Awaited>, keys: string) { + for (const key of keys) { + await act(async () => { + await target.mockInput.typeText(key); + }); + await flush(target); + } +} + +beforeEach(() => { + delete process.env.EDITOR; +}); + +afterEach(async () => { + if (setup) { + const current = setup; + setup = undefined; + await act(async () => { + current.renderer.destroy(); + }); + } + + if (originalEditor === undefined) { + delete process.env.EDITOR; + } else { + process.env.EDITOR = originalEditor; + } + mockSpawnSync(originalSpawnSync); + + while (tempDirs.length > 0) { + const dir = tempDirs.pop(); + if (dir) { + rmSync(dir, { recursive: true, force: true }); + } + } +}); + +describe("AppHost edit-selected-file shortcut", () => { + test("pressing e with no $EDITOR surfaces a notice instead of crashing", async () => { + setup = await testRender( + , + WIDE, + ); + await flush(setup); + + await pressKeys(setup, "e"); + + // openSelectedFileInEditor returns "$EDITOR is not set." which shows as a session notice. + expect(setup.captureCharFrame()).toContain("EDITOR is not set"); + }); + + test("pressing e opens the editor at the current line, not the hunk start", async () => { + const workspace = createTempWorkspace(); + process.env.EDITOR = "vim"; + + const spawnCalls: string[][] = []; + mockSpawnSync(((cmds: string[]) => { + spawnCalls.push(cmds); + return { exitCode: 1 }; + }) as unknown as typeof Bun.spawnSync); + + setup = await testRender(, WIDE); + await flush(setup); + + // The hunk starts at line 1; step down onto the changed line, then one line past it. + await pressKeys(setup, "jje"); + await pressKeys(setup, "je"); + + expect(spawnCalls).toEqual([ + ["vim", "+2", join(workspace, "sample.ts")], + ["vim", "+3", join(workspace, "sample.ts")], + ]); + }); +}); diff --git a/src/ui/AppHost.sidebar-resize.test.tsx b/src/ui/AppHost.sidebar-resize.test.tsx index d9df3532..856a7a36 100644 --- a/src/ui/AppHost.sidebar-resize.test.tsx +++ b/src/ui/AppHost.sidebar-resize.test.tsx @@ -196,32 +196,3 @@ describe("AppHost sidebar resize", () => { expect(dividerColumn(setup)).toBe(INITIAL_DIVIDER_COLUMN); }); }); - -describe("AppHost edit-selected-file shortcut", () => { - const originalEditor = process.env.EDITOR; - - beforeEach(() => { - delete process.env.EDITOR; - }); - - afterEach(() => { - if (originalEditor === undefined) { - delete process.env.EDITOR; - } else { - process.env.EDITOR = originalEditor; - } - }); - - test("pressing e with no $EDITOR surfaces a notice instead of crashing", async () => { - setup = await testRender(, WIDE); - await flush(setup); - - await act(async () => { - await setup!.mockInput.typeText("e"); - }); - await flush(setup); - - // openSelectedFileInEditor returns "$EDITOR is not set." which shows as a session notice. - expect(setup.captureCharFrame()).toContain("EDITOR is not set"); - }); -}); diff --git a/src/ui/lib/openInEditor.test.ts b/src/ui/lib/openInEditor.test.ts index f508362d..16d099fb 100644 --- a/src/ui/lib/openInEditor.test.ts +++ b/src/ui/lib/openInEditor.test.ts @@ -222,6 +222,174 @@ describe("open in editor helpers", () => { expect(renderer.resume).toHaveBeenCalledTimes(1); }); + test("opens the current line instead of the selected hunk start", () => { + const basePath = createTempDir(); + writeFileSync(join(basePath, "example.ts"), "const value = 1;\n"); + process.env.EDITOR = "vim"; + + const spawnCalls: string[][] = []; + mockSpawnSync((cmds) => { + spawnCalls.push(cmds); + return { exitCode: 0 }; + }); + + const file = createTestDiffFile({ path: "example.ts" }); + + expect( + openSelectedFileInEditor({ + basePath, + file, + lineCursor: { + fileId: file.id, + hunkIndex: 1, + target: { side: "new", line: 3 }, + }, + renderer: createRenderer(), + selectedHunk: file.metadata.hunks[0], + }), + ).toBeNull(); + + expect(spawnCalls).toEqual([["vim", "+3", join(basePath, "example.ts")]]); + }); + + test("maps an old-side current line onto the line on disk", () => { + const basePath = createTempDir(); + writeFileSync(join(basePath, "example.ts"), "one\nfour\n"); + process.env.EDITOR = "vim"; + + const spawnCalls: string[][] = []; + mockSpawnSync((cmds) => { + spawnCalls.push(cmds); + return { exitCode: 0 }; + }); + + const file = createTestDiffFile({ + path: "example.ts", + before: "one\ntwo\nthree\nfour\n", + after: "one\nfour\n", + }); + + expect( + openSelectedFileInEditor({ + basePath, + file, + lineCursor: { + fileId: file.id, + hunkIndex: 0, + target: { side: "old", line: 3 }, + }, + renderer: createRenderer(), + selectedHunk: file.metadata.hunks[0], + }), + ).toBeNull(); + + expect(spawnCalls).toEqual([["vim", "+2", join(basePath, "example.ts")]]); + }); + + test("walks leading context when mapping an old-side current line", () => { + const basePath = createTempDir(); + writeFileSync(join(basePath, "example.ts"), "one\nfour\n"); + process.env.EDITOR = "vim"; + + const spawnCalls: string[][] = []; + mockSpawnSync((cmds) => { + spawnCalls.push(cmds); + return { exitCode: 0 }; + }); + + const file = createTestDiffFile({ + path: "example.ts", + before: "one\ntwo\nthree\nfour\n", + after: "one\nfour\n", + context: 1, + }); + + expect( + openSelectedFileInEditor({ + basePath, + file, + lineCursor: { + fileId: file.id, + hunkIndex: 0, + target: { side: "old", line: 3 }, + }, + renderer: createRenderer(), + selectedHunk: file.metadata.hunks[0], + }), + ).toBeNull(); + + // Old line 3 ("three") was removed, so the editor lands on the line that now follows "one". + expect(spawnCalls).toEqual([["vim", "+2", join(basePath, "example.ts")]]); + }); + + test("preserves the deleted line's offset within a multi-line replacement", () => { + const basePath = createTempDir(); + writeFileSync(join(basePath, "example.ts"), "one\nTWO\nTHREE\nfour\n"); + process.env.EDITOR = "vim"; + + const spawnCalls: string[][] = []; + mockSpawnSync((cmds) => { + spawnCalls.push(cmds); + return { exitCode: 0 }; + }); + + const file = createTestDiffFile({ + path: "example.ts", + before: "one\ntwo\nthree\nfour\n", + after: "one\nTWO\nTHREE\nfour\n", + }); + + expect( + openSelectedFileInEditor({ + basePath, + file, + lineCursor: { + fileId: file.id, + hunkIndex: 0, + target: { side: "old", line: 3 }, + }, + renderer: createRenderer(), + selectedHunk: file.metadata.hunks[0], + }), + ).toBeNull(); + + // Old line 3 ("three") is the second of two replaced lines, so the editor + // lands on the second replacement line ("THREE") rather than the first. + expect(spawnCalls).toEqual([["vim", "+3", join(basePath, "example.ts")]]); + }); + + test("falls back to the selected hunk when the cursor is in another file", () => { + const basePath = createTempDir(); + writeFileSync(join(basePath, "example.ts"), "const value = 1;\n"); + process.env.EDITOR = "vim"; + + const spawnCalls: string[][] = []; + mockSpawnSync((cmds) => { + spawnCalls.push(cmds); + return { exitCode: 0 }; + }); + + const file = createTestDiffFile({ path: "example.ts" }); + + expect( + openSelectedFileInEditor({ + basePath, + file, + lineCursor: { + fileId: "other-file", + hunkIndex: 0, + target: { side: "new", line: 42 }, + }, + renderer: createRenderer(), + selectedHunk: file.metadata.hunks[1], + }), + ).toBeNull(); + + expect(spawnCalls).toEqual([ + ["vim", `+${file.metadata.hunks[1]!.additionStart}`, join(basePath, "example.ts")], + ]); + }); + test("uses deletion line numbers for deleted files", () => { const basePath = createTempDir(); writeFileSync(join(basePath, "deleted.ts"), "const old = true;\n"); diff --git a/src/ui/lib/openInEditor.ts b/src/ui/lib/openInEditor.ts index a45da464..4b9f1ef2 100644 --- a/src/ui/lib/openInEditor.ts +++ b/src/ui/lib/openInEditor.ts @@ -2,17 +2,76 @@ import { existsSync } from "node:fs"; import { basename, resolve, win32 } from "node:path"; import type { CliRenderer } from "@opentui/core"; import type { DiffFile } from "../../core/types"; +import type { LineCursor } from "./lineCursors"; export interface EditorCommand { command: string; args: string[]; } +type DiffHunk = DiffFile["metadata"]["hunks"][number]; + +/** The review stream's current line, minus the geometry fields this module never reads. */ +export type EditorLineCursor = Pick; + +/** + * Translate an old-side line to the line it maps to in the file on disk. + * + * Deleted lines have no on-disk counterpart, so they resolve to the position their + * replacement occupies, which is where the reader expects the editor to land. + */ +function deletionLineToFileLine(hunk: DiffHunk, deletionLine: number) { + let deletionCursor = hunk.deletionStart; + // A zero-count side names the line before the change, so step past it to land inside the file. + let additionCursor = hunk.additionCount === 0 ? hunk.additionStart + 1 : hunk.additionStart; + + for (const content of hunk.hunkContent) { + if (content.type === "context") { + if (deletionLine < deletionCursor + content.lines) { + return additionCursor + (deletionLine - deletionCursor); + } + + deletionCursor += content.lines; + additionCursor += content.lines; + continue; + } + + if (deletionLine < deletionCursor + content.deletions) { + // Land on the corresponding replacement line, clamped to the last one this block adds. + const offset = Math.min(deletionLine - deletionCursor, Math.max(content.additions - 1, 0)); + return additionCursor + offset; + } + + deletionCursor += content.deletions; + additionCursor += content.additions; + } + + return additionCursor; +} + +/** Prefer the current line over the selected hunk's first line. */ function selectedLine( file: DiffFile, - selectedHunk: DiffFile["metadata"]["hunks"][number] | undefined, + selectedHunk: DiffHunk | undefined, + lineCursor: EditorLineCursor | null | undefined, ) { - if (file.metadata.type === "deleted") { + // Deleted files are opened against their pre-change content, every other file against its new one. + const isDeleted = file.metadata.type === "deleted"; + const diskSide = isDeleted ? "old" : "new"; + const cursor = lineCursor?.fileId === file.id ? lineCursor : undefined; + + if (cursor) { + if (cursor.target.side === diskSide) { + return cursor.target.line; + } + + const cursorHunk = file.metadata.hunks[cursor.hunkIndex]; + if (!isDeleted && cursorHunk) { + return deletionLineToFileLine(cursorHunk, cursor.target.line); + } + } + + if (isDeleted) { return selectedHunk?.deletionStart ?? 1; } @@ -84,13 +143,15 @@ export function resolveEditableFilePath(filePath: string, basePath = process.cwd export function openSelectedFileInEditor({ basePath, file, + lineCursor, renderer, selectedHunk, }: { basePath?: string; file: DiffFile | undefined; + lineCursor?: EditorLineCursor | null; renderer: Pick; - selectedHunk: DiffFile["metadata"]["hunks"][number] | undefined; + selectedHunk: DiffHunk | undefined; }) { if (!file) { return "No file selected."; @@ -106,7 +167,7 @@ export function openSelectedFileInEditor({ return `Cannot edit ${file.path}: file does not exist on disk.`; } - const line = Math.max(1, selectedLine(file, selectedHunk)); + const line = Math.max(1, selectedLine(file, selectedHunk, lineCursor)); const command = buildEditorCommand({ editor, filePath: absolutePath,