From 8734a57dde1106c480b74fac077e81d855d9309b Mon Sep 17 00:00:00 2001 From: James Long Date: Thu, 21 May 2026 21:47:51 -0400 Subject: [PATCH] fix(tui): interaction improvements to diff viewer --- .../system/diff-viewer-file-tree-utils.ts | 51 +++++++ .../feature-plugins/system/diff-viewer.tsx | 125 +++++++++++++----- .../tui/diff-viewer-file-tree-utils.test.ts | 99 ++++++++++++++ 3 files changed, 244 insertions(+), 31 deletions(-) diff --git a/packages/opencode/src/cli/cmd/tui/feature-plugins/system/diff-viewer-file-tree-utils.ts b/packages/opencode/src/cli/cmd/tui/feature-plugins/system/diff-viewer-file-tree-utils.ts index 28aeb0fb65..e618047f29 100644 --- a/packages/opencode/src/cli/cmd/tui/feature-plugins/system/diff-viewer-file-tree-utils.ts +++ b/packages/opencode/src/cli/cmd/tui/feature-plugins/system/diff-viewer-file-tree-utils.ts @@ -157,6 +157,49 @@ export function moveFileTreeSelectionToFile( return next?.id ?? (offset < 0 ? fileRows[0]!.id : fileRows[fileRows.length - 1]!.id) } +export function fileTreeFileSelection(tree: FileTree, fileIndex: number) { + const node = tree.nodes.find((item) => item.kind === "file" && item.fileIndex === fileIndex) + if (!node) return undefined + return { + highlightedNode: node.id, + expandedNodes: fileTreeParentDirectories(tree, node.id), + } +} + +export function singlePatchFileIndex( + selected: number | undefined, + active: number | undefined, + current: number | undefined, + first: number | undefined, +) { + return selected ?? active ?? current ?? first +} + +export function orderedPatchFileIndexes(rows: readonly FileTreeRow[]) { + return rows.flatMap((row) => (row.fileIndex === undefined ? [] : [row.fileIndex])) +} + +export function movePatchFileIndex( + fileIndexes: readonly number[], + current: number | undefined, + offset: number, +) { + if (fileIndexes.length === 0) return undefined + const index = current === undefined ? -1 : fileIndexes.indexOf(current) + if (index === -1) return offset < 0 ? fileIndexes[fileIndexes.length - 1] : fileIndexes[0] + return fileIndexes[Math.max(0, Math.min(fileIndexes.length - 1, index + offset))] +} + +export function relativePatchFileIndexFromViewport( + entries: readonly { readonly fileIndex: number; readonly titleContentY: number }[], + scrollTop: number, + offset: number, +) { + const ordered = [...entries].sort((left, right) => left.titleContentY - right.titleContentY) + if (offset > 0) return ordered.find((entry) => entry.titleContentY > scrollTop)?.fileIndex + return ordered.findLast((entry) => entry.titleContentY < scrollTop)?.fileIndex +} + export function allExpandedFileTreeDirectories(tree: FileTree) { return new Set(tree.nodes.filter((node) => node.kind === "directory").map((node) => node.id)) } @@ -189,3 +232,11 @@ function addFileTreeNode(nodes: FileTreeNode[], roots: number[], input: Omit() + for (let parent = tree.nodes[id]?.parent; parent !== undefined; parent = tree.nodes[parent]?.parent) { + result.add(parent) + } + return result +} diff --git a/packages/opencode/src/cli/cmd/tui/feature-plugins/system/diff-viewer.tsx b/packages/opencode/src/cli/cmd/tui/feature-plugins/system/diff-viewer.tsx index 1ea8836f01..f0f9c0e899 100644 --- a/packages/opencode/src/cli/cmd/tui/feature-plugins/system/diff-viewer.tsx +++ b/packages/opencode/src/cli/cmd/tui/feature-plugins/system/diff-viewer.tsx @@ -14,12 +14,17 @@ import { DialogSelect } from "@tui/ui/dialog-select" import { allExpandedFileTreeDirectories, buildFileTree, + fileTreeFileSelection, flattenFileTree, moveFileTreeSelection, moveFileTreeSelectionToFirstChild, moveFileTreeSelectionToFile, moveFileTreeSelectionToParent, + movePatchFileIndex, + orderedPatchFileIndexes, + relativePatchFileIndexFromViewport, setFileTreeDirectoryExpanded, + singlePatchFileIndex, toggleFileTreeDirectory, } from "./diff-viewer-file-tree-utils" @@ -108,6 +113,7 @@ function DiffViewer(props: { api: TuiPluginApi }) { const [selectedFileIndex, setSelectedFileIndex] = createSignal() const [reviewedFileNames, setReviewedFileNames] = createSignal>(new Set()) const fileRows = createMemo(() => flattenFileTree(fileTree(), expandedFileNodes())) + const patchFileIndexes = createMemo(() => orderedPatchFileIndexes(flattenFileTree(fileTree()))) const focusRunner = (input: Record void>) => () => input[focus()]() const switchFocusShortcut = useCommandShortcut("diff.switch_focus") const nextFileShortcut = useCommandShortcut("diff.next_file") @@ -164,39 +170,67 @@ function DiffViewer(props: { api: TuiPluginApi }) { } const revealFileTreeFile = (fileIndex: number) => { - const node = fileTree().nodes.find((item) => item.kind === "file" && item.fileIndex === fileIndex) - if (!node) return + const selection = fileTreeFileSelection(fileTree(), fileIndex) + if (!selection) return setExpandedFileNodes((expanded) => { const next = new Set(expanded) - for (let parent = node.parent; parent !== undefined; parent = fileTree().nodes[parent]?.parent) { - next.add(parent) - } + selection.expandedNodes.forEach((node) => next.add(node)) return next }) - setHighlighted(node.id) + setHighlighted(selection.highlightedNode) + } + + const selectPatchFile = (fileIndex: number) => { + revealFileTreeFile(fileIndex) + setActivePatchFileIndex(fileIndex) + setSelectedFileIndex(fileIndex) } const scrollToFileIndex = (fileIndex: number | undefined) => { if (fileIndex === undefined) return - setActivePatchFileIndex(fileIndex) - setSelectedFileIndex(fileIndex) + selectPatchFile(fileIndex) const patchNode = patchNodeByFileIndex.get(fileIndex) if (patchNode) scrollPatchNodeToTop(patchNode, fileIndex) } const jumpToFileIndex = (fileIndex: number | undefined) => { if (fileIndex === undefined) return - revealFileTreeFile(fileIndex) scrollToFileIndex(fileIndex) } const currentPatchFileIndex = () => { if (!scroll) return undefined - const entries = files() - .map((_, fileIndex) => ({ fileIndex, node: patchNodeByFileIndex.get(fileIndex) })) + const viewportContentY = scroll.scrollTop + 1 + const entries = patchFileIndexes() + .map((fileIndex) => ({ + fileIndex, + node: patchNodeByFileIndex.get(fileIndex), + })) .filter((entry): entry is { fileIndex: number; node: BoxRenderable } => Boolean(entry.node)) - .sort((left, right) => left.node.y - right.node.y) - return entries.findLast((entry) => entry.node.y <= scroll!.viewport.y + 1)?.fileIndex ?? entries[0]?.fileIndex + .map((entry) => ({ + ...entry, + contentY: scroll!.scrollTop + entry.node.y - scroll!.viewport.y, + })) + .sort((left, right) => left.contentY - right.contentY) + return entries.findLast((entry) => entry.contentY <= viewportContentY)?.fileIndex ?? entries[0]?.fileIndex + } + + const nextPatchFileIndexFromViewport = (offset: number) => { + if (!scroll) return undefined + return relativePatchFileIndexFromViewport( + patchFileIndexes() + .map((fileIndex) => ({ fileIndex, node: patchNodeByFileIndex.get(fileIndex) })) + .filter((entry): entry is { fileIndex: number; node: BoxRenderable } => Boolean(entry.node)) + .map((entry) => { + const contentY = scroll!.scrollTop + entry.node.y - scroll!.viewport.y + return { + fileIndex: entry.fileIndex, + titleContentY: contentY + (contentY === 0 ? 0 : 1), + } + }), + scroll.scrollTop, + offset, + ) } const jumpRelativePatchFile = (offset: number) => { @@ -207,32 +241,55 @@ function DiffViewer(props: { api: TuiPluginApi }) { jumpToFileIndex(fileRows().find((row) => row.id === nextFromSelection)?.fileIndex) return } - const currentFileIndex = activePatchFileIndex() ?? currentPatchFileIndex() - const currentRow = fileRows().find((row) => row.fileIndex === currentFileIndex) scrollToFileIndex( - fileRows().find((row) => row.id === moveFileTreeSelectionToFile(fileRows(), currentRow?.id, offset))?.fileIndex, + nextPatchFileIndexFromViewport(offset) ?? + movePatchFileIndex(patchFileIndexes(), currentPatchFileIndex() ?? activePatchFileIndex(), offset), ) } const highlightedPatchFileIndex = () => fileRows().find((row) => row.id === highlightedFileNode())?.fileIndex const firstPatchFileIndex = () => fileRows().find((row) => row.fileIndex !== undefined)?.fileIndex const visiblePatchFiles = createMemo(() => { - if (!singlePatch()) return files().map((file, fileIndex) => ({ file, fileIndex })) - const fileIndex = activePatchFileIndex() ?? currentPatchFileIndex() ?? firstPatchFileIndex() + if (!singlePatch()) { + return patchFileIndexes().flatMap((fileIndex) => { + const file = files()[fileIndex] + return file ? [{ file, fileIndex }] : [] + }) + } + const fileIndex = singlePatchFileIndex( + selectedFileIndex(), + activePatchFileIndex(), + currentPatchFileIndex(), + firstPatchFileIndex(), + ) const file = fileIndex === undefined ? undefined : files()[fileIndex] return file && fileIndex !== undefined ? [{ file, fileIndex }] : [] }) const ensureHighlightedPatchFile = () => { - if (activePatchFileIndex() !== undefined) return - const fileIndex = currentPatchFileIndex() ?? firstPatchFileIndex() - if (fileIndex !== undefined) setActivePatchFileIndex(fileIndex) + const fileIndex = currentPatchFileIndex() ?? activePatchFileIndex() ?? firstPatchFileIndex() + if (fileIndex === undefined) return + selectPatchFile(fileIndex) } - const scrollToHighlightedPatchFile = () => { - const fileIndex = activePatchFileIndex() - if (fileIndex === undefined) return + const scrollToPatchFileIndexAfterRender = (fileIndex: number) => { setPendingPatchScrollFileIndex(fileIndex) + requestAnimationFrame(() => { + const patchNode = patchNodeByFileIndex.get(fileIndex) + if (patchNode) scrollPatchNodeToTop(patchNode, fileIndex) + requestAnimationFrame(() => { + const patchNode = patchNodeByFileIndex.get(fileIndex) + if (patchNode) scrollPatchNodeToTop(patchNode, fileIndex) + setPendingPatchScrollFileIndex(undefined) + }) + }) + } + + const scrollSinglePatchToTop = () => { + requestAnimationFrame(() => { + scroll?.scrollTo(0) + requestAnimationFrame(() => scroll?.scrollTo(0)) + }) } const registerPatchNode = (fileIndex: number, element: BoxRenderable) => { @@ -437,12 +494,18 @@ function DiffViewer(props: { api: TuiPluginApi }) { title: "Toggle single patch view", category: "VCS", run() { - setSinglePatch((value) => { - const next = !value - if (next) ensureHighlightedPatchFile() - else scrollToHighlightedPatchFile() - return next - }) + if (!singlePatch()) { + ensureHighlightedPatchFile() + setSinglePatch(true) + scrollSinglePatchToTop() + return + } + const fileIndex = + visiblePatchFiles()[0]?.fileIndex ?? + singlePatchFileIndex(selectedFileIndex(), activePatchFileIndex(), currentPatchFileIndex(), firstPatchFileIndex()) + if (fileIndex !== undefined) selectPatchFile(fileIndex) + setSinglePatch(false) + if (fileIndex !== undefined) scrollToPatchFileIndexAfterRender(fileIndex) }, }, { @@ -581,7 +644,7 @@ function DiffViewer(props: { api: TuiPluginApi }) { flexDirection="row" gap={1} flexShrink={0} - paddingLeft={2} + paddingLeft={1} paddingRight={1} border={["left"]} borderColor={theme().border} diff --git a/packages/opencode/test/cli/tui/diff-viewer-file-tree-utils.test.ts b/packages/opencode/test/cli/tui/diff-viewer-file-tree-utils.test.ts index 61d656a43a..d42e4b3bdd 100644 --- a/packages/opencode/test/cli/tui/diff-viewer-file-tree-utils.test.ts +++ b/packages/opencode/test/cli/tui/diff-viewer-file-tree-utils.test.ts @@ -2,12 +2,17 @@ import { describe, expect, test } from "bun:test" import { allExpandedFileTreeDirectories, buildFileTree, + fileTreeFileSelection, flattenFileTree, moveFileTreeSelection, moveFileTreeSelectionToFirstChild, moveFileTreeSelectionToFile, moveFileTreeSelectionToParent, + movePatchFileIndex, + orderedPatchFileIndexes, + relativePatchFileIndexFromViewport, setFileTreeDirectoryExpanded, + singlePatchFileIndex, toggleFileTreeDirectory, } from "../../../src/cli/cmd/tui/feature-plugins/system/diff-viewer-file-tree-utils" @@ -233,6 +238,100 @@ describe("diff viewer file tree utilities", () => { expect(moveFileTreeSelectionToFile(rows, readme.id, 1)).toBe(readme.id) }) + test("selects a file tree node and expands its parents for a patch file", () => { + const tree = buildFileTree([ + { file: "src/config/tui.ts" }, + { file: "src/session/index.ts" }, + { file: "README.md" }, + ]) + const selection = fileTreeFileSelection(tree, 1) + + expect(selection?.highlightedNode).toBe(tree.nodes.find((node) => node.kind === "file" && node.name === "index.ts")?.id) + expect([...selection!.expandedNodes].map((id) => tree.nodes[id]!.name)).toEqual(["session", "src"]) + expect(fileTreeFileSelection(tree, 99)).toBeUndefined() + }) + + test("prefers the selected file when choosing the single patch file", () => { + expect(singlePatchFileIndex(2, 1, 0, 3)).toBe(2) + expect(singlePatchFileIndex(undefined, 1, 0, 3)).toBe(1) + expect(singlePatchFileIndex(undefined, undefined, 0, 3)).toBe(0) + expect(singlePatchFileIndex(undefined, undefined, undefined, 3)).toBe(3) + }) + + test("orders patches by the flattened file tree order", () => { + const rows = flattenFileTree( + buildFileTree([ + { file: "src/dir-8/juniper-4.ts" }, + { file: "src/dir-8/harbor-94.ts" }, + { file: "src/dir-8/cedar-16.ts" }, + ]), + ) + + expect(orderedPatchFileIndexes(rows)).toEqual([2, 1, 0]) + }) + + test("moves patch selection through the ordered patch file indexes", () => { + const fileIndexes = [2, 1, 0] + + expect(movePatchFileIndex(fileIndexes, undefined, 1)).toBe(2) + expect(movePatchFileIndex(fileIndexes, undefined, -1)).toBe(0) + expect(movePatchFileIndex(fileIndexes, 2, 1)).toBe(1) + expect(movePatchFileIndex(fileIndexes, 1, -1)).toBe(2) + expect(movePatchFileIndex(fileIndexes, 0, 1)).toBe(0) + expect(movePatchFileIndex(fileIndexes, 99, 1)).toBe(2) + expect(movePatchFileIndex([], undefined, 1)).toBeUndefined() + }) + + test("moves to the next visible patch title below the viewport", () => { + expect( + relativePatchFileIndexFromViewport( + [ + { fileIndex: 0, titleContentY: 0 }, + { fileIndex: 1, titleContentY: 30 }, + { fileIndex: 2, titleContentY: 60 }, + ], + 10, + 1, + ), + ).toBe(1) + expect( + relativePatchFileIndexFromViewport( + [ + { fileIndex: 0, titleContentY: 0 }, + { fileIndex: 1, titleContentY: 30 }, + { fileIndex: 2, titleContentY: 60 }, + ], + 30, + 1, + ), + ).toBe(2) + }) + + test("moves to the previous visible patch title above the viewport", () => { + expect( + relativePatchFileIndexFromViewport( + [ + { fileIndex: 0, titleContentY: 0 }, + { fileIndex: 1, titleContentY: 30 }, + { fileIndex: 2, titleContentY: 60 }, + ], + 50, + -1, + ), + ).toBe(1) + expect( + relativePatchFileIndexFromViewport( + [ + { fileIndex: 0, titleContentY: 0 }, + { fileIndex: 1, titleContentY: 30 }, + { fileIndex: 2, titleContentY: 60 }, + ], + 30, + -1, + ), + ).toBe(0) + }) + test("toggles only selected directory expansion", () => { const tree = buildFileTree([{ file: "src/config/tui.ts" }, { file: "README.md" }]) const src = tree.nodes.find((node) => node.kind === "directory" && node.name === "src")!