From 69e4f5227232df49211baa6ace143a1b9b86416c Mon Sep 17 00:00:00 2001 From: James Long Date: Fri, 22 May 2026 12:30:10 -0400 Subject: [PATCH] fix(tui): interaction improvements to diff viewer (#28851) --- .../system/diff-viewer-file-tree-utils.ts | 51 ++++++ .../system/diff-viewer-file-tree.tsx | 8 +- .../feature-plugins/system/diff-viewer.tsx | 159 +++++++++++++----- .../tui/diff-viewer-file-tree-utils.test.ts | 99 +++++++++++ .../cli/tui/diff-viewer-file-tree.test.tsx | 12 +- 5 files changed, 285 insertions(+), 44 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-file-tree.tsx b/packages/opencode/src/cli/cmd/tui/feature-plugins/system/diff-viewer-file-tree.tsx index f95a06ec0a..b34e67be9b 100644 --- a/packages/opencode/src/cli/cmd/tui/feature-plugins/system/diff-viewer-file-tree.tsx +++ b/packages/opencode/src/cli/cmd/tui/feature-plugins/system/diff-viewer-file-tree.tsx @@ -95,10 +95,10 @@ export function DiffViewerFileTree(props: DiffViewerFileTreeProps) { fg={ highlighted() ? props.theme.background - : reviewed() - ? props.theme.textMuted - : selected() - ? props.theme.primary + : selected() + ? props.theme.primary + : reviewed() + ? props.theme.textMuted : row.kind === "directory" ? tint(props.theme.text, props.theme.background, 0.35) : props.theme.text 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..3cf9028a24 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") @@ -154,52 +160,93 @@ function DiffViewer(props: { api: TuiPluginApi }) { setActivePatchFileIndex(undefined) } - const scrollPatchNodeToTop = (patchNode: BoxRenderable, fileIndex: number) => { - if (!scroll) return - const offset = fileIndex === 0 ? 0 : 1 - scroll.scrollBy(patchNode.y - scroll.viewport.y + offset) + const scrollPatchNodeToTop = (patchNode: BoxRenderable) => { requestAnimationFrame(() => { - if (scroll) scroll.scrollBy(patchNode.y - scroll.viewport.y + offset) + if (!scroll) return + const scrollDelta = patchNode.y - scroll.viewport.y + const contentY = scroll.scrollTop + scrollDelta + const offset = contentY === 0 ? 0 : 1 + scroll.scrollBy(scrollDelta + offset) }) } 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) + if (patchNode) scrollPatchNodeToTop(patchNode) } 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) => { + if (singlePatch()) { + const next = movePatchFileIndex( + patchFileIndexes(), + visiblePatchFiles()[0]?.fileIndex ?? selectedFileIndex() ?? activePatchFileIndex() ?? firstPatchFileIndex(), + offset, + ) + if (next === undefined) return + selectPatchFile(next) + scrollSinglePatchToTop() + return + } + const current = focus() === "files" ? highlightedFileNode() : undefined const nextFromSelection = current === undefined ? undefined : moveFileTreeSelectionToFile(fileRows(), current, offset) @@ -207,41 +254,64 @@ 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) + requestAnimationFrame(() => { + const patchNode = patchNodeByFileIndex.get(fileIndex) + if (patchNode) scrollPatchNodeToTop(patchNode) + setPendingPatchScrollFileIndex(undefined) + }) + }) + } + + const scrollSinglePatchToTop = () => { + requestAnimationFrame(() => { + scroll?.scrollTo(0) + requestAnimationFrame(() => scroll?.scrollTo(0)) + }) } const registerPatchNode = (fileIndex: number, element: BoxRenderable) => { patchNodeByFileIndex.set(fileIndex, element) if (pendingPatchScrollFileIndex() !== fileIndex) return requestAnimationFrame(() => { - scrollPatchNodeToTop(element, fileIndex) + scrollPatchNodeToTop(element) requestAnimationFrame(() => { - scrollPatchNodeToTop(element, fileIndex) + scrollPatchNodeToTop(element) setPendingPatchScrollFileIndex(undefined) }) }) @@ -437,12 +507,23 @@ 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 +662,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")! diff --git a/packages/opencode/test/cli/tui/diff-viewer-file-tree.test.tsx b/packages/opencode/test/cli/tui/diff-viewer-file-tree.test.tsx index 4e86a74053..57a1aae416 100644 --- a/packages/opencode/test/cli/tui/diff-viewer-file-tree.test.tsx +++ b/packages/opencode/test/cli/tui/diff-viewer-file-tree.test.tsx @@ -155,7 +155,7 @@ async function renderFrame(component: () => JSX.Element) { const app = await testRender(() => withTheme(component), { width: 40, height: 10 }) try { await renderOnceSettled(app) - return app.captureCharFrame() + return await captureSettledFrame(app) } finally { app.renderer.destroy() } @@ -167,6 +167,16 @@ async function renderOnceSettled(app: Awaited>) { await app.renderOnce() } +async function captureSettledFrame(app: Awaited>) { + for (let attempt = 0; attempt < 5; attempt++) { + const frame = app.captureCharFrame() + if (frame.trim().length > 0) return frame + await new Promise((resolve) => setTimeout(resolve, 25)) + await app.renderOnce() + } + return app.captureCharFrame() +} + function withTheme(component: () => JSX.Element) { return (