diff --git a/.changeset/steady-sidebar-follow.md b/.changeset/steady-sidebar-follow.md new file mode 100644 index 000000000..66cdcda88 --- /dev/null +++ b/.changeset/steady-sidebar-follow.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Keep the selected file visible in the sidebar while holding down next-file or previous-file navigation, instead of snapping the list back to the top. diff --git a/packages/hunk/src/extensions/default/ui/sidebar/FileSidebars.test.tsx b/packages/hunk/src/extensions/default/ui/sidebar/FileSidebars.test.tsx new file mode 100644 index 000000000..c83317406 --- /dev/null +++ b/packages/hunk/src/extensions/default/ui/sidebar/FileSidebars.test.tsx @@ -0,0 +1,183 @@ +import { describe, expect, test } from "bun:test"; +import { testRender } from "@opentui/react/test-utils"; +import { act, forwardRef, useImperativeHandle, useState } from "react"; +import { createTestDiffFile } from "../../../../../../../test/helpers/diff-helpers"; +import { resolveTheme } from "../../../../ui/themes"; +import { toReadOnlyFileViews } from "../../../events"; +import { FlexFileSidebar } from "./FileSidebars"; + +const FILE_COUNT = 40; +const FRAME_ROWS = 8; + +const fileId = (index: number) => `file-${String(index).padStart(2, "0")}`; + +const files = toReadOnlyFileViews( + Array.from({ length: FILE_COUNT }, (_, index) => + createTestDiffFile({ + id: fileId(index), + path: `src/${fileId(index)}.ts`, + before: "export const value = 1;\n", + after: "export const value = 2;\n", + }), + ), +); + +interface SidebarSelectionHandle { + select(id: string): void; +} + +/** Host the bundled sidebar behind a selection setter so tests drive it like the review does. */ +const SidebarSelectionHarness = forwardRef( + function SidebarSelectionHarness({ initialFileId }, ref) { + const [selectedFileId, setSelectedFileId] = useState(initialFileId); + useImperativeHandle(ref, () => ({ select: setSelectedFileId }), []); + return ( + false, getKeys: () => [] }} + actions={{ + copyText: () => false, + selectFile: () => {}, + selectHunk: () => {}, + revealLine: () => {}, + notify: () => {}, + }} + /> + ); + }, +); + +/** Return the sidebar row carrying the selected-file marker, if one is on screen. */ +function selectedRow(frame: string) { + return frame.split("\n").find((line) => line.includes("▌")); +} + +async function renderSidebar(initialFileId: string) { + const handle: { current: SidebarSelectionHandle | null } = { current: null }; + const setup = await testRender( + { + handle.current = value; + }} + initialFileId={initialFileId} + />, + { width: 36, height: FRAME_ROWS }, + ); + await act(async () => { + await setup.renderOnce(); + }); + + /** Change the selection without letting a frame lay the new row out first. */ + const selectWithoutFrame = async (id: string) => { + await act(async () => { + handle.current?.select(id); + }); + }; + + /** Change the selection and let one frame settle, the way an unhurried key press does. */ + const selectAndSettle = async (id: string) => { + await selectWithoutFrame(id); + await act(async () => { + await setup.renderOnce(); + }); + }; + + const frame = async () => { + await act(async () => { + await setup.renderOnce(); + }); + return setup.captureCharFrame(); + }; + + const destroy = async () => { + await act(async () => { + setup.renderer.destroy(); + }); + }; + + return { selectWithoutFrame, selectAndSettle, frame, destroy }; +} + +describe("FlexFileSidebar selected-row reveal", () => { + test("follows an unhurried walk down the list", async () => { + const sidebar = await renderSidebar(fileId(0)); + try { + for (let index = 1; index <= 12; index += 1) { + await sidebar.selectAndSettle(fileId(index)); + } + const frame = await sidebar.frame(); + expect(selectedRow(frame)).toContain(`${fileId(12)}.ts`); + expect(frame).not.toContain(`${fileId(0)}.ts`); + } finally { + await sidebar.destroy(); + } + }); + + test("keeps following when selection outruns the laid-out rows", async () => { + // Held-down file navigation lands several commits between two frames. Rows the render + // window mounts for those commits have no layout yet, so a reveal that reads their + // geometry either scrolls back to the top or does nothing. + const sidebar = await renderSidebar(fileId(0)); + try { + for (let index = 1; index <= 12; index += 1) { + await sidebar.selectWithoutFrame(fileId(index)); + } + const frame = await sidebar.frame(); + expect(selectedRow(frame)).toContain(`${fileId(12)}.ts`); + expect(frame).not.toContain(`${fileId(0)}.ts`); + } finally { + await sidebar.destroy(); + } + }); + + test("does not scroll back to the top when a far row mounts fresh", async () => { + const sidebar = await renderSidebar(fileId(0)); + try { + for (let index = 1; index <= 20; index += 1) { + await sidebar.selectAndSettle(fileId(index)); + } + expect(selectedRow(await sidebar.frame())).toContain(`${fileId(20)}.ts`); + + await sidebar.selectWithoutFrame(fileId(30)); + const frame = await sidebar.frame(); + expect(selectedRow(frame)).toContain(`${fileId(30)}.ts`); + expect(frame).not.toContain(`${fileId(0)}.ts`); + + // Stepping onward from the stuck state must keep revealing too. + await sidebar.selectWithoutFrame(fileId(31)); + expect(selectedRow(await sidebar.frame())).toContain(`${fileId(31)}.ts`); + } finally { + await sidebar.destroy(); + } + }); + + test("reveals a selection that starts outside the first viewport", async () => { + const sidebar = await renderSidebar(fileId(25)); + try { + const frame = await sidebar.frame(); + expect(selectedRow(frame)).toContain(`${fileId(25)}.ts`); + } finally { + await sidebar.destroy(); + } + }); + + test("walks back up after the list has scrolled", async () => { + const sidebar = await renderSidebar(fileId(0)); + try { + for (let index = 1; index <= 20; index += 1) { + await sidebar.selectAndSettle(fileId(index)); + } + for (let index = 19; index >= 5; index -= 1) { + await sidebar.selectWithoutFrame(fileId(index)); + } + const frame = await sidebar.frame(); + expect(selectedRow(frame)).toContain(`${fileId(5)}.ts`); + } finally { + await sidebar.destroy(); + } + }); +}); diff --git a/packages/hunk/src/extensions/default/ui/sidebar/FileSidebars.tsx b/packages/hunk/src/extensions/default/ui/sidebar/FileSidebars.tsx index 19ae651ea..57bbda9a7 100644 --- a/packages/hunk/src/extensions/default/ui/sidebar/FileSidebars.tsx +++ b/packages/hunk/src/extensions/default/ui/sidebar/FileSidebars.tsx @@ -1,6 +1,6 @@ import type { ScrollBoxRenderable } from "@opentui/core"; import { useTerminalDimensions } from "@opentui/react"; -import { useEffect, useMemo, useRef, useState, type ReactNode } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState, type ReactNode } from "react"; import type { ExtensionPaneProps } from "../../../../extension-api/types"; import { buildFlatSidebarEntries, @@ -13,8 +13,10 @@ import { toggleCollapsedDirectoryPath, type SidebarEntry, } from "../../../../ui/lib/files"; -import { fileRowId } from "../../../../ui/lib/ids"; -import { buildSidebarRenderWindow } from "../../../../ui/lib/sidebarRenderWindow"; +import { + buildSidebarRenderWindow, + planSidebarRowReveal, +} from "../../../../ui/lib/sidebarRenderWindow"; import { FileDirectoryRow, FileGroupHeader, @@ -27,24 +29,21 @@ export type BuiltInSidebarProps = Omit< > & Partial>; -type FileSidebarVariantProps = Pick< - BuiltInSidebarProps, - "actions" | "files" | "selectedFileId" | "theme" -> & { - estimatedViewportRows: number; - scrollTop: number; - textWidth: number; - viewportHeight: number; -}; - /** Ignore directory toggles for projections that cannot contain directory rows. */ function ignoreDirectoryToggle() {} -interface VirtualizedFileSidebarRowsProps extends Omit { +interface VirtualizedFileSidebarRowsProps extends Pick< + BuiltInSidebarProps, + "actions" | "selectedFileId" | "theme" +> { collapsedDirectoryPaths?: ReadonlySet; entries: SidebarEntry[]; + estimatedViewportRows: number; onToggleDirectory?: (path: string) => void; paddingLeft?: number; + scrollTop: number; + textWidth: number; + viewportHeight: number; } /** Render one windowed sidebar projection with shared file selection and stats lanes. */ @@ -132,43 +131,15 @@ export function VirtualizedFileSidebarRows({ ); } -/** Render the compact directory-group projection for a narrow file sidebar. */ -export function FlatFileSidebar({ files, ...props }: FileSidebarVariantProps): ReactNode { - const entries = useMemo(() => buildFlatSidebarEntries(files), [files]); - return ; -} - -/** Render the ordered hierarchy after applying the sidebar's collapsed directory paths. */ -export function TreeFileSidebar({ - collapsedDirectoryPaths, - files, - onToggleDirectory, - ...props -}: FileSidebarVariantProps & { - collapsedDirectoryPaths: ReadonlySet; - onToggleDirectory: (path: string) => void; -}): ReactNode { - const entries = useMemo( - () => collapseTreeSidebarEntries(buildTreeSidebarEntries(files), collapsedDirectoryPaths), - [collapsedDirectoryPaths, files], - ); - return ( - - ); -} - /** - * Adapt the built-in file sidebar between compact and hierarchical projections. + * Render the built-in file sidebar, switching between the compact directory-group + * projection and the collapsible tree as the pane width changes. * - * Resizing only replaces the rows inside one stable scrollbox. The sidebar keeps - * file navigation and selected-row reveal shared so both projections preserve - * the same review-stream behavior. + * Resizing only replaces the rows inside one stable scrollbox. File navigation, a + * projection change, and a reload all reveal the selected row through the same + * fixed-row geometry the render window uses, so the reveal never depends on a row + * having been laid out: rows mounted since the last frame carry no position yet, and + * held-down navigation lands several commits between frames. */ export function FlexFileSidebar({ files, @@ -180,6 +151,10 @@ export function FlexFileSidebar({ const scrollRef = useRef(null); const previousSelectedFileIdRef = useRef(selectedFileId); const skipSelectedFileRevealRef = useRef(false); + // The file whose row still has to be brought on screen. It stays set until a measured + // viewport confirms the row is visible, so a reveal that ran before the first layout or + // against a content height the next layout will grow retries once geometry settles. + const pendingRevealFileIdRef = useRef(null); const [collapsedDirectoryPaths, setCollapsedDirectoryPaths] = useState>( () => new Set(), ); @@ -188,16 +163,13 @@ export function FlexFileSidebar({ // Mirrors the host layout: one column of row highlight plus row padding. const textWidth = Math.max(8, width - 2); const mode = resolveFileSidebarMode(textWidth); - const variantProps: FileSidebarVariantProps = { - actions, - estimatedViewportRows: terminal.height, - files, - scrollTop: scrollViewport.top, - selectedFileId, - textWidth, - theme, - viewportHeight: scrollViewport.height, - }; + const entries = useMemo( + () => + mode === "tree" + ? collapseTreeSidebarEntries(buildTreeSidebarEntries(files), collapsedDirectoryPaths) + : buildFlatSidebarEntries(files), + [collapsedDirectoryPaths, files, mode], + ); /** Toggle one logical directory everywhere it appears in the ordered tree projection. */ const toggleDirectory = (path: string) => { @@ -205,6 +177,54 @@ export function FlexFileSidebar({ setCollapsedDirectoryPaths((current) => toggleCollapsedDirectoryPath(current, path)); }; + /** + * Scroll the pending file's row into the viewport and clear the request once it is visible. + * + * Works from the row's entry index rather than its rendered position, so it is correct + * for rows the render window mounted in this very commit. A viewport that has not been + * measured yet, or a scroll the scrollbox clamped against a stale content height, leaves + * the request pending for the next viewport event. + */ + const revealPendingRow = useCallback(() => { + const scrollBox = scrollRef.current; + const fileId = pendingRevealFileIdRef.current; + if (!scrollBox || !fileId) { + return; + } + + const entryIndex = entries.findIndex((entry) => entry.kind === "file" && entry.id === fileId); + if (entryIndex < 0) { + pendingRevealFileIdRef.current = null; + return; + } + + const viewportHeight = scrollBox.viewport.height ?? 0; + if (viewportHeight <= 0) { + return; + } + + const target = planSidebarRowReveal({ + entryIndex, + scrollTop: scrollBox.scrollTop ?? 0, + viewportHeight, + }); + if (target !== null) { + scrollBox.scrollTo(target); + } + + const stillHidden = + planSidebarRowReveal({ + entryIndex, + scrollTop: scrollBox.scrollTop ?? 0, + viewportHeight, + }) !== null; + if (!stillHidden) { + pendingRevealFileIdRef.current = null; + } + }, [entries]); + const revealPendingRowRef = useRef(revealPendingRow); + revealPendingRowRef.current = revealPendingRow; + useEffect(() => { const previousSelectedFileId = previousSelectedFileIdRef.current; previousSelectedFileIdRef.current = selectedFileId; @@ -241,6 +261,8 @@ export function FlexFileSidebar({ ); }; + // OpenTUI emits these from its own layout and slider work; one microtask per burst + // reads the settled geometry and gives a pending reveal its retry. const handleViewportChange = () => { if (scheduled) { return; @@ -254,6 +276,7 @@ export function FlexFileSidebar({ try { readViewport(); + revealPendingRowRef.current(); } finally { scheduled = false; } @@ -262,19 +285,19 @@ export function FlexFileSidebar({ readViewport(); scrollBox.verticalScrollBar.on("change", handleViewportChange); - scrollBox.viewport.on("layout-changed", handleViewportChange); - scrollBox.viewport.on("resized", handleViewportChange); + scrollBox.viewport.on("resize", handleViewportChange); + scrollBox.content.on("resize", handleViewportChange); return () => { cancelled = true; scrollBox.verticalScrollBar.off("change", handleViewportChange); - scrollBox.viewport.off("layout-changed", handleViewportChange); - scrollBox.viewport.off("resized", handleViewportChange); + scrollBox.viewport.off("resize", handleViewportChange); + scrollBox.content.off("resize", handleViewportChange); }; }, [files, mode]); - // Selection and projection changes can both move the target row, so follow - // the stable file id after either event instead of only after navigation. + // Selection and projection changes can both move the target row, so follow the stable + // file id after either event instead of only after navigation. useEffect(() => { if (skipSelectedFileRevealRef.current) { skipSelectedFileRevealRef.current = false; @@ -284,8 +307,9 @@ export function FlexFileSidebar({ return; } - scrollRef.current?.scrollChildIntoView(fileRowId(selectedFileId)); - }, [collapsedDirectoryPaths, files, mode, selectedFileId]); + pendingRevealFileIdRef.current = selectedFileId; + revealPendingRow(); + }, [revealPendingRow, selectedFileId]); return ( - {mode === "tree" ? ( - - ) : ( - - )} + ); } diff --git a/packages/hunk/src/extensions/default/ui/sidebar/index.tsx b/packages/hunk/src/extensions/default/ui/sidebar/index.tsx index 3366ade1c..69c564e43 100644 --- a/packages/hunk/src/extensions/default/ui/sidebar/index.tsx +++ b/packages/hunk/src/extensions/default/ui/sidebar/index.tsx @@ -22,7 +22,7 @@ import { FlexFileSidebar } from "./FileSidebars"; export const BUNDLED_SIDEBAR_EXTENSION_ID = HUNK_VENDOR_EXTENSION_ID; export const BUNDLED_SIDEBAR_VIEW_ID = "files"; -export { FlatFileSidebar, FlexFileSidebar, TreeFileSidebar } from "./FileSidebars"; +export { FlexFileSidebar } from "./FileSidebars"; /** Register the responsive built-in file navigation pane. */ const registerBundledSidebar: ExtensionFactory = (hunk) => { diff --git a/packages/hunk/src/ui/lib/sidebarRenderWindow.test.ts b/packages/hunk/src/ui/lib/sidebarRenderWindow.test.ts index 33c6a9732..6f36bdcfe 100644 --- a/packages/hunk/src/ui/lib/sidebarRenderWindow.test.ts +++ b/packages/hunk/src/ui/lib/sidebarRenderWindow.test.ts @@ -1,6 +1,10 @@ import { describe, expect, test } from "bun:test"; import type { SidebarEntry } from "./files"; -import { buildSidebarRenderWindow, type SidebarRenderWindowItem } from "./sidebarRenderWindow"; +import { + buildSidebarRenderWindow, + planSidebarRowReveal, + type SidebarRenderWindowItem, +} from "./sidebarRenderWindow"; /** Build fixed-height sidebar rows with occasional group headers. */ function createEntries(ids: string[]): SidebarEntry[] { @@ -202,3 +206,26 @@ describe("buildSidebarRenderWindow", () => { expect(renderedHeight(plan.items)).toBe(entries.length); }); }); + +describe("planSidebarRowReveal", () => { + test("leaves a row that is already inside the viewport alone", () => { + expect(planSidebarRowReveal({ entryIndex: 3, scrollTop: 0, viewportHeight: 8 })).toBeNull(); + expect(planSidebarRowReveal({ entryIndex: 7, scrollTop: 0, viewportHeight: 8 })).toBeNull(); + expect(planSidebarRowReveal({ entryIndex: 12, scrollTop: 12, viewportHeight: 8 })).toBeNull(); + }); + + test("scrolls a row below the viewport onto its bottom edge", () => { + expect(planSidebarRowReveal({ entryIndex: 8, scrollTop: 0, viewportHeight: 8 })).toBe(1); + expect(planSidebarRowReveal({ entryIndex: 30, scrollTop: 13, viewportHeight: 8 })).toBe(23); + }); + + test("scrolls a row above the viewport onto its top edge", () => { + expect(planSidebarRowReveal({ entryIndex: 4, scrollTop: 13, viewportHeight: 8 })).toBe(4); + expect(planSidebarRowReveal({ entryIndex: 0, scrollTop: 1, viewportHeight: 8 })).toBe(0); + }); + + test("refuses to plan without a measured viewport or a real row", () => { + expect(planSidebarRowReveal({ entryIndex: 5, scrollTop: 0, viewportHeight: 0 })).toBeNull(); + expect(planSidebarRowReveal({ entryIndex: -1, scrollTop: 0, viewportHeight: 8 })).toBeNull(); + }); +}); diff --git a/packages/hunk/src/ui/lib/sidebarRenderWindow.ts b/packages/hunk/src/ui/lib/sidebarRenderWindow.ts index 25ef570d4..96071c1a2 100644 --- a/packages/hunk/src/ui/lib/sidebarRenderWindow.ts +++ b/packages/hunk/src/ui/lib/sidebarRenderWindow.ts @@ -70,6 +70,39 @@ function entryRangeHeight(startIndex: number, endIndex: number) { return startIndex > endIndex ? 0 : (endIndex - startIndex + 1) * SIDEBAR_ROW_HEIGHT; } +/** + * Return the scroll offset that brings one fixed-height sidebar row inside the viewport, or + * `null` when the row is already fully visible. + * + * Works from the entry index and row height alone so callers never read a row's rendered + * position: rows the render window mounted since the last frame carry no layout yet. Scrolls + * to the nearest edge, so a row above the viewport lands on its top and a row below lands on + * its bottom. Callers pass a measured, positive viewport height. + */ +export function planSidebarRowReveal({ + entryIndex, + scrollTop, + viewportHeight, +}: { + entryIndex: number; + scrollTop: number; + viewportHeight: number; +}): number | null { + if (entryIndex < 0 || viewportHeight <= 0) { + return null; + } + + const rowTop = entryIndex * SIDEBAR_ROW_HEIGHT; + const rowBottom = rowTop + SIDEBAR_ROW_HEIGHT; + if (rowTop < scrollTop) { + return rowTop; + } + if (rowBottom > scrollTop + viewportHeight) { + return rowBottom - viewportHeight; + } + return null; +} + /** Build a sparse sidebar render plan that preserves exact scroll height with spacers. */ export function buildSidebarRenderWindow({ entries, diff --git a/test/pty/harness.ts b/test/pty/harness.ts index 71521b6ed..39e00fa6e 100644 --- a/test/pty/harness.ts +++ b/test/pty/harness.ts @@ -754,6 +754,20 @@ end ); } + /** Build a repo with enough one-line files that the sidebar list overflows a short terminal. */ + function createManyFileSidebarRepoFixture(count = 40) { + return createGitRepoFixture( + Array.from({ length: count }, (_, index) => { + const name = `file-${String(index).padStart(2, "0")}`; + return { + path: `src/${name}.ts`, + before: `export const ${name.replace("-", "")} = ${index};\n`, + after: `export const ${name.replace("-", "")} = ${index + 100};\n`, + }; + }), + ); + } + function createPinnedHeaderRepoFixture() { return createGitRepoFixture([ { @@ -1125,6 +1139,7 @@ end createNarrowHeaderTestRepoFixture, createPagerPatchFixture, createManyShortFileRepoFixture, + createManyFileSidebarRepoFixture, createPinnedHeaderRepoFixture, createRapidThemePreviewTestRepoFixture, createScrollableFilePair, diff --git a/test/pty/sidebar-follow-integration.test.ts b/test/pty/sidebar-follow-integration.test.ts new file mode 100644 index 000000000..390530fc2 --- /dev/null +++ b/test/pty/sidebar-follow-integration.test.ts @@ -0,0 +1,73 @@ +import { afterEach, describe, expect, setDefaultTimeout, test } from "bun:test"; +import { createPtyHarness } from "./harness"; + +const harness = createPtyHarness(); + +/** Give PTY-backed startup and redraws enough headroom for slower CI machines. */ +setDefaultTimeout(20_000); + +afterEach(() => { + harness.cleanup(); +}); + +/** Return the file named on the sidebar row that carries the selected-file marker. */ +function selectedSidebarFile(text: string) { + for (const line of text.split("\n")) { + const match = /^\s*▌\s+\S\s+(file-\d\d\.ts)/.exec(line); + if (match) { + return match[1]; + } + } + return null; +} + +/** Report whether the sidebar lists a file row, selected or not. */ +function sidebarLists(text: string, file: string) { + return new RegExp(`^\\s*▌?\\s+\\S\\s+${file.replace(".", "\\.")}\\s`, "m").test(text); +} + +describe("PTY sidebar selection follow", () => { + test("a burst of next-file presses keeps the selected file inside the sidebar viewport", async () => { + // Held-down `.` delivers key repeats faster than the renderer paints frames. The sidebar + // must keep the selected row on screen even when several selections land between two + // frames, on rows the render window has only just mounted. + const fixture = harness.createManyFileSidebarRepoFixture(60); + const session = await harness.launchHunk({ + args: ["diff", "--mode", "split"], + cwd: fixture.dir, + cols: 160, + rows: 24, + }); + + try { + const initial = await session.waitForText(/View\s+Navigate\s+Agent\s+Help/, { + timeout: 15_000, + }); + expect(selectedSidebarFile(initial)).toBe("file-00.ts"); + expect(sidebarLists(initial, "file-40.ts")).toBe(false); + + // One write carries the whole burst so every press is handled before the next frame. + session.writeRaw(".".repeat(40)); + await harness.waitForSnapshot(session, (text) => text.includes("file40 = 140"), 5_000); + const revealed = await harness.waitForSnapshot( + session, + (text) => selectedSidebarFile(text) === "file-40.ts", + 3_000, + ); + expect(sidebarLists(revealed, "file-00.ts")).toBe(false); + + // Stepping onward from a scrolled list must not snap the sidebar back to its top. + session.writeRaw(".".repeat(10)); + await harness.waitForSnapshot(session, (text) => text.includes("file50 = 150"), 5_000); + const revealedAgain = await harness.waitForSnapshot( + session, + (text) => selectedSidebarFile(text) === "file-50.ts", + 3_000, + ); + expect(sidebarLists(revealedAgain, "file-00.ts")).toBe(false); + expect(sidebarLists(revealedAgain, "file-40.ts")).toBe(true); + } finally { + session.close(); + } + }); +});