fix(tui): interaction improvements to diff viewer (#28851)

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<Fil
   else nodes[input.parent]!.children.push(id)
   return id
 }
+
+function fileTreeParentDirectories(tree: FileTree, id: number) {
+  const result = new Set<number>()
+  for (let parent = tree.nodes[id]?.parent; parent !== undefined; parent = tree.nodes[parent]?.parent) {
+    result.add(parent)
+  }
+  return result
+}

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

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<number | undefined>()
   const [reviewedFileNames, setReviewedFileNames] = createSignal<ReadonlySet<string>>(new Set())
   const fileRows = createMemo(() => flattenFileTree(fileTree(), expandedFileNodes()))
+  const patchFileIndexes = createMemo(() => orderedPatchFileIndexes(flattenFileTree(fileTree())))
   const focusRunner = (input: Record<DiffViewerFocus, () => 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 scrollToFileIndex = (fileIndex: number | undefined) => {
-    if (fileIndex === undefined) return
+  const selectPatchFile = (fileIndex: number) => {
+    revealFileTreeFile(fileIndex)
     setActivePatchFileIndex(fileIndex)
     setSelectedFileIndex(fileIndex)
+  }
+
+  const scrollToFileIndex = (fileIndex: number | undefined) => {
+    if (fileIndex === undefined) return
+    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}

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")!

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<ReturnType<typeof testRender>>) {
   await app.renderOnce()
 }
 
+async function captureSettledFrame(app: Awaited<ReturnType<typeof testRender>>) {
+  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 (
     <TuiConfigProvider config={createTuiResolvedConfig()}>