From 6e381bfb4b6679a547ccb5180be7bdd295454ec6 Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Mon, 6 Apr 2026 14:21:57 -0400 Subject: [PATCH 1/3] feat(review): prototype horizontal code-column scrolling Keep gutters and review chrome fixed while letting long diff lines be inspected without switching to wrapped mode. Add arrow-key controls, help text, and targeted coverage for the new reveal path. --- src/ui/App.tsx | 50 +++++++++++++++++++++++- src/ui/AppHost.interactions.test.tsx | 46 ++++++++++++++++++++++ src/ui/components/chrome/HelpDialog.tsx | 1 + src/ui/components/panes/DiffPane.tsx | 3 ++ src/ui/components/panes/DiffSection.tsx | 4 ++ src/ui/components/ui-components.test.tsx | 43 +++++++++++++++++++- src/ui/diff/PierreDiffView.tsx | 3 ++ src/ui/diff/renderRows.tsx | 38 ++++++++++++++---- src/ui/hooks/useAppKeyboardShortcuts.ts | 22 +++++++++++ 9 files changed, 199 insertions(+), 11 deletions(-) diff --git a/src/ui/App.tsx b/src/ui/App.tsx index a28defef..527bfa08 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -5,7 +5,7 @@ import { } from "@opentui/core"; import { useRenderer, useTerminalDimensions } from "@opentui/react"; import { Suspense, lazy, useCallback, useEffect, useMemo, useState, useRef } from "react"; -import type { AppBootstrap, CliInput, LayoutMode } from "../core/types"; +import type { AppBootstrap, CliInput, DiffFile, LayoutMode } from "../core/types"; import { canReloadInput, computeWatchSignature } from "../core/watch"; import { HunkHostClient } from "../mcp/client"; import type { ReloadedSessionResult } from "../mcp/types"; @@ -38,6 +38,23 @@ function clamp(value: number, min: number, max: number) { return Math.min(Math.max(value, min), max); } +/** Measure one diff line after the same tab expansion used by the renderer. */ +function renderedCodeLineWidth(line: string | undefined) { + return (line ?? "").replace(/\n$/, "").replaceAll("\t", " ").length; +} + +/** Track the widest code line that could benefit from horizontal reveal. */ +function maxFileCodeLineWidth(file: DiffFile) { + const deletionLines = file.metadata.deletionLines ?? []; + const additionLines = file.metadata.additionLines ?? []; + + return Math.max( + 0, + ...deletionLines.map(renderedCodeLineWidth), + ...additionLines.map(renderedCodeLineWidth), + ); +} + /** Preserve the active app view settings when rebuilding the current input. */ function withCurrentViewOptions( input: CliInput, @@ -99,6 +116,7 @@ export function App({ const [showAgentNotes, setShowAgentNotes] = useState(bootstrap.initialShowAgentNotes ?? false); const [showLineNumbers, setShowLineNumbers] = useState(bootstrap.initialShowLineNumbers ?? true); const [wrapLines, setWrapLines] = useState(bootstrap.initialWrapLines ?? false); + const [codeHorizontalOffset, setCodeHorizontalOffset] = useState(0); const [showHunkHeaders, setShowHunkHeaders] = useState(bootstrap.initialShowHunkHeaders ?? true); const [sidebarVisible, setSidebarVisible] = useState(true); const [forceSidebarOpen, setForceSidebarOpen] = useState(false); @@ -219,6 +237,34 @@ export function App({ diffScrollRef.current?.scrollBy(delta, unit); }; + const maxCodeHorizontalOffset = useMemo( + () => + Math.max( + 0, + filteredFiles.reduce( + (maxWidth, file) => Math.max(maxWidth, maxFileCodeLineWidth(file)), + 0, + ) - 1, + ), + [filteredFiles], + ); + + useEffect(() => { + setCodeHorizontalOffset((current) => clamp(current, 0, maxCodeHorizontalOffset)); + }, [maxCodeHorizontalOffset]); + + /** Shift the visible code columns horizontally without moving gutters or headers. */ + const scrollCodeHorizontally = useCallback( + (delta: number) => { + if (wrapLines || delta === 0 || maxCodeHorizontalOffset <= 0) { + return; + } + + setCodeHorizontalOffset((current) => clamp(current + delta, 0, maxCodeHorizontalOffset)); + }, + [maxCodeHorizontalOffset, wrapLines], + ); + /** Toggle the global agent note layer on or off. */ const toggleAgentNotes = () => { setShowAgentNotes((current) => !current); @@ -491,6 +537,7 @@ export function App({ openMenu, pagerMode, requestQuit, + scrollCodeHorizontally, scrollDiff, selectLayoutMode: setLayoutMode, showHelp, @@ -636,6 +683,7 @@ export function App({ ) : null} { } }); + test("left and right arrows can reveal offscreen code columns in nowrap mode", async () => { + const setup = await testRender(, { + width: 92, + height: 20, + }); + + try { + await flush(setup); + + let frame = setup.captureCharFrame(); + expect(frame).toContain("this is a very"); + expect(frame).not.toContain("interaction coverage"); + + for (let index = 0; index < 64; index += 1) { + await act(async () => { + await setup.mockInput.pressArrow("right"); + }); + await flush(setup); + frame = setup.captureCharFrame(); + if (frame.includes("interaction coverage")) { + break; + } + } + + expect(frame).toContain("interaction coverage"); + expect(frame).not.toContain("this is a very"); + + for (let index = 0; index < 64; index += 1) { + await act(async () => { + await setup.mockInput.pressArrow("left"); + }); + await flush(setup); + frame = setup.captureCharFrame(); + if (frame.includes("this is a very")) { + break; + } + } + + expect(frame).toContain("this is a very"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + test("bootstrap preferences initialize the visible view state", async () => { const setup = await testRender( { // This comparator relies on stable upstream object identity for files and visible-note arrays. return ( + previous.codeHorizontalOffset === next.codeHorizontalOffset && previous.file === next.file && previous.headerLabelWidth === next.headerLabelWidth && previous.headerStatsWidth === next.headerStatsWidth && diff --git a/src/ui/components/ui-components.test.tsx b/src/ui/components/ui-components.test.tsx index c46cb8d5..3f38c259 100644 --- a/src/ui/components/ui-components.test.tsx +++ b/src/ui/components/ui-components.test.tsx @@ -1248,13 +1248,13 @@ describe("UI components", () => { const frame = await captureFrame( {}} />, 76, - 29, + 31, ); const expectedRows = [ @@ -1268,6 +1268,7 @@ describe("UI components", () => { "d / u half page down / up", "[ / ] previous / next hunk", "{ / } previous / next comment", + "← / → scroll code left / right", "Home / End jump to top / bottom", "View", "1 / 2 / 0 split / stack / auto", @@ -1484,6 +1485,44 @@ describe("UI components", () => { expect(addedLines.slice(1).some((line) => line.includes("age';"))).toBe(true); }); + test("PierreDiffView can reveal offscreen code columns in nowrap mode", async () => { + const file = createWrapBootstrap().changeset.files[0]!; + const theme = resolveTheme("midnight", null); + + const baseFrame = await captureFrame( + , + 52, + 12, + ); + const shiftedFrame = await captureFrame( + , + 52, + 12, + ); + + expect(baseFrame).toContain("this is a very"); + expect(baseFrame).not.toContain("diff rendering coverage';"); + expect(shiftedFrame).toContain("coverage';"); + expect(shiftedFrame).not.toContain("this is a very"); + }); + test("split view wraps the same long diff line across more rows than stack view at the same width", async () => { const file = createWrapBootstrap().changeset.files[0]!; const theme = resolveTheme("midnight", null); diff --git a/src/ui/diff/PierreDiffView.tsx b/src/ui/diff/PierreDiffView.tsx index 3e4b408d..48ce0fdd 100644 --- a/src/ui/diff/PierreDiffView.tsx +++ b/src/ui/diff/PierreDiffView.tsx @@ -16,6 +16,7 @@ const EMPTY_VISIBLE_AGENT_NOTES: VisibleAgentNote[] = []; /** Render a file diff in split or stack mode, with inline agent notes inserted between diff rows. */ export function PierreDiffView({ annotatedHunkIndices = EMPTY_ANNOTATED_HUNK_INDICES, + codeHorizontalOffset = 0, file, layout, onOpenAgentNotesAtHunk, @@ -31,6 +32,7 @@ export function PierreDiffView({ scrollable = true, }: { annotatedHunkIndices?: Set; + codeHorizontalOffset?: number; file: DiffFile | undefined; layout: Exclude; onOpenAgentNotesAtHunk?: (hunkIndex: number) => void; @@ -140,6 +142,7 @@ export function PierreDiffView({ showLineNumbers={showLineNumbers} showHunkHeaders={showHunkHeaders} wrapLines={wrapLines} + codeHorizontalOffset={codeHorizontalOffset} theme={theme} selected={plannedRow.row.hunkIndex === selectedHunkIndex} annotated={ diff --git a/src/ui/diff/renderRows.tsx b/src/ui/diff/renderRows.tsx index 9ad8af45..9fddb489 100644 --- a/src/ui/diff/renderRows.tsx +++ b/src/ui/diff/renderRows.tsx @@ -20,8 +20,8 @@ export function fitText(text: string, width: number) { return `${text.slice(0, width - 1)}…`; } -/** Trim styled spans to a fixed width while preserving color runs. */ -function trimSpans(spans: RenderSpan[], width: number) { +/** Slice styled spans to one visible window while preserving color runs. */ +function sliceSpansWindow(spans: RenderSpan[], offset: number, width: number) { if (width <= 0) { return { spans: [] as RenderSpan[], @@ -29,7 +29,8 @@ function trimSpans(spans: RenderSpan[], width: number) { }; } - const trimmed: RenderSpan[] = []; + const sliced: RenderSpan[] = []; + let remainingOffset = Math.max(0, offset); let remaining = width; let usedWidth = 0; @@ -38,7 +39,15 @@ function trimSpans(spans: RenderSpan[], width: number) { break; } - const text = span.text.slice(0, remaining); + if (remainingOffset >= span.text.length) { + remainingOffset -= span.text.length; + continue; + } + + const start = remainingOffset; + const text = span.text.slice(start, start + remaining); + remainingOffset = 0; + if (text.length === 0) { continue; } @@ -48,11 +57,11 @@ function trimSpans(spans: RenderSpan[], width: number) { text, }; - const previous = trimmed.at(-1); + const previous = sliced.at(-1); if (previous && previous.fg === nextSpan.fg && previous.bg === nextSpan.bg) { previous.text += nextSpan.text; } else { - trimmed.push(nextSpan); + sliced.push(nextSpan); } remaining -= text.length; @@ -60,7 +69,7 @@ function trimSpans(spans: RenderSpan[], width: number) { } return { - spans: trimmed, + spans: sliced, usedWidth, }; } @@ -198,8 +207,9 @@ function renderInlineSpans( fallbackColor: string, fallbackBg: string, keyPrefix: string, + horizontalOffset = 0, ) { - const { spans: trimmed, usedWidth } = trimSpans(spans, width); + const { spans: trimmed, usedWidth } = sliceSpansWindow(spans, horizontalOffset, width); let padding = Math.max(0, width - usedWidth); if (padding > 0) { @@ -366,6 +376,7 @@ function renderSplitCell( showLineNumbers: boolean, theme: AppTheme, keyPrefix: string, + contentOffset = 0, prefix?: { text: string; fg: string; @@ -399,6 +410,7 @@ function renderSplitCell( theme.text, palette.contentBg, `${keyPrefix}:content`, + contentOffset, )} ); @@ -412,6 +424,7 @@ function renderStackCell( showLineNumbers: boolean, theme: AppTheme, keyPrefix: string, + contentOffset = 0, prefix?: { text: string; fg: string; @@ -449,6 +462,7 @@ function renderStackCell( theme.text, palette.contentBg, `${keyPrefix}:content`, + contentOffset, )} ); @@ -708,6 +722,7 @@ function renderRow( showLineNumbers: boolean, showHunkHeaders: boolean, wrapLines: boolean, + codeHorizontalOffset: number, theme: AppTheme, selected: boolean, annotated: boolean, @@ -764,6 +779,7 @@ function renderRow( showLineNumbers, theme, `${row.key}:left`, + codeHorizontalOffset, leftPrefix, )} {renderSplitCell( @@ -773,6 +789,7 @@ function renderRow( showLineNumbers, theme, `${row.key}:right`, + codeHorizontalOffset, rightPrefix, )} {guideOnNewSide ? ( @@ -874,6 +891,7 @@ function renderRow( showLineNumbers, theme, `${row.key}:stack`, + codeHorizontalOffset, prefix, )} {guideOnNewSide ? ( @@ -940,6 +958,7 @@ interface DiffRowViewProps { showLineNumbers: boolean; showHunkHeaders: boolean; wrapLines: boolean; + codeHorizontalOffset: number; theme: AppTheme; selected: boolean; annotated: boolean; @@ -957,6 +976,7 @@ export const DiffRowView = memo( showLineNumbers, showHunkHeaders, wrapLines, + codeHorizontalOffset, theme, selected, annotated, @@ -971,6 +991,7 @@ export const DiffRowView = memo( showLineNumbers, showHunkHeaders, wrapLines, + codeHorizontalOffset, theme, selected, annotated, @@ -987,6 +1008,7 @@ export const DiffRowView = memo( previous.showLineNumbers === next.showLineNumbers && previous.showHunkHeaders === next.showHunkHeaders && previous.wrapLines === next.wrapLines && + previous.codeHorizontalOffset === next.codeHorizontalOffset && previous.theme === next.theme && previous.selected === next.selected && previous.annotated === next.annotated && diff --git a/src/ui/hooks/useAppKeyboardShortcuts.ts b/src/ui/hooks/useAppKeyboardShortcuts.ts index 4d96b9ce..a4872a73 100644 --- a/src/ui/hooks/useAppKeyboardShortcuts.ts +++ b/src/ui/hooks/useAppKeyboardShortcuts.ts @@ -33,6 +33,7 @@ export interface UseAppKeyboardShortcutsOptions { openMenu: (menuId: MenuId) => void; pagerMode: boolean; requestQuit: () => void; + scrollCodeHorizontally: (delta: number) => void; scrollDiff: (delta: number, unit: ScrollUnit) => void; selectLayoutMode: (mode: LayoutMode) => void; showHelp: boolean; @@ -66,6 +67,7 @@ export function useAppKeyboardShortcuts({ openMenu, pagerMode, requestQuit, + scrollCodeHorizontally, scrollDiff, selectLayoutMode, showHelp, @@ -138,6 +140,16 @@ export function useAppKeyboardShortcuts({ return; } + if (key.name === "left") { + scrollCodeHorizontally(-1); + return; + } + + if (key.name === "right") { + scrollCodeHorizontally(1); + return; + } + if (key.name === "home") { scrollDiff(-1, "content"); return; @@ -291,6 +303,16 @@ export function useAppKeyboardShortcuts({ return; } + if (key.name === "left") { + scrollCodeHorizontally(-1); + return; + } + + if (key.name === "right") { + scrollCodeHorizontally(1); + return; + } + if (key.name === "1") { runAndCloseMenu(() => selectLayoutMode("split")); return; From 9625a0e95ee1dd07f5a361559a6af2b4b0eff13c Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Mon, 6 Apr 2026 14:37:03 -0400 Subject: [PATCH 2/3] feat(review): add fast horizontal scroll controls Make long diff lines easier to inspect by supporting faster shift-arrow movement and shift-wheel code scrolling. Reset the horizontal offset when wrapping toggles so the reveal state stays predictable. --- src/ui/App.tsx | 6 + src/ui/AppHost.interactions.test.tsx | 149 +++++++++++++++++++++++ src/ui/components/chrome/HelpDialog.tsx | 2 +- src/ui/components/panes/DiffPane.tsx | 31 ++++- src/ui/components/ui-components.test.tsx | 2 +- src/ui/hooks/useAppKeyboardShortcuts.ts | 10 +- 6 files changed, 193 insertions(+), 7 deletions(-) diff --git a/src/ui/App.tsx b/src/ui/App.tsx index 527bfa08..378a95bc 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -26,6 +26,8 @@ import { resolveTheme, THEMES } from "./themes"; type FocusArea = "files" | "filter"; +const FAST_CODE_HORIZONTAL_SCROLL_COLUMNS = 8; + const LazyHelpDialog = lazy(async () => ({ default: (await import("./components/chrome/HelpDialog")).HelpDialog, })); @@ -280,6 +282,7 @@ export function App({ // Capture the pre-toggle viewport position synchronously so DiffPane can restore the same // top-most source row after wrapped row heights change. wrapToggleScrollTopRef.current = diffScrollRef.current?.scrollTop ?? 0; + setCodeHorizontalOffset(0); setWrapLines((current) => !current); }; @@ -704,6 +707,9 @@ export function App({ theme={activeTheme} width={diffPaneWidth} onOpenAgentNotesAtHunk={openAgentNotesAtHunk} + onScrollCodeHorizontally={(delta) => { + scrollCodeHorizontally(delta * FAST_CODE_HORIZONTAL_SCROLL_COLUMNS); + }} onSelectFile={jumpToFile} /> diff --git a/src/ui/AppHost.interactions.test.tsx b/src/ui/AppHost.interactions.test.tsx index c23b7fa2..133faef4 100644 --- a/src/ui/AppHost.interactions.test.tsx +++ b/src/ui/AppHost.interactions.test.tsx @@ -496,6 +496,155 @@ describe("App interactions", () => { } }); + test("shift plus left and right arrows scroll code horizontally faster", async () => { + const setup = await testRender(, { + width: 92, + height: 20, + }); + + try { + await flush(setup); + + let frame = setup.captureCharFrame(); + expect(frame).toContain("this is a very"); + expect(frame).not.toContain("interaction coverage"); + + for (let index = 0; index < 8; index += 1) { + await act(async () => { + await setup.mockInput.pressArrow("right", { shift: true }); + }); + await flush(setup); + frame = setup.captureCharFrame(); + if (frame.includes("interaction coverage")) { + break; + } + } + + expect(frame).toContain("interaction coverage"); + expect(frame).not.toContain("this is a very"); + + for (let index = 0; index < 8; index += 1) { + await act(async () => { + await setup.mockInput.pressArrow("left", { shift: true }); + }); + await flush(setup); + frame = setup.captureCharFrame(); + if (frame.includes("this is a very")) { + break; + } + } + + expect(frame).toContain("this is a very"); + expect(frame).not.toContain("interaction coverage"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("shift plus mouse wheel scrolls code horizontally", async () => { + const setup = await testRender(, { + width: 92, + height: 20, + }); + + try { + await flush(setup); + + let frame = setup.captureCharFrame(); + expect(frame).toContain("this is a very"); + expect(frame).not.toContain("interaction coverage"); + + for (let index = 0; index < 8; index += 1) { + await act(async () => { + await setup.mockMouse.scroll(60, 10, "down", { modifiers: { shift: true } }); + }); + await flush(setup); + frame = setup.captureCharFrame(); + if (frame.includes("interaction coverage")) { + break; + } + } + + expect(frame).toContain("interaction coverage"); + expect(frame).not.toContain("this is a very"); + + for (let index = 0; index < 8; index += 1) { + await act(async () => { + await setup.mockMouse.scroll(60, 10, "up", { modifiers: { shift: true } }); + }); + await flush(setup); + frame = setup.captureCharFrame(); + if (frame.includes("this is a very")) { + break; + } + } + + expect(frame).toContain("this is a very"); + expect(frame).not.toContain("interaction coverage"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("wrap toggles reset the horizontal code offset", async () => { + const setup = await testRender(, { + width: 92, + height: 20, + }); + + try { + await flush(setup); + + let frame = setup.captureCharFrame(); + expect(frame).toContain("this is a very"); + + for (let index = 0; index < 8; index += 1) { + await act(async () => { + await setup.mockInput.pressArrow("right", { shift: true }); + }); + await flush(setup); + frame = setup.captureCharFrame(); + if (frame.includes("interaction coverage")) { + break; + } + } + + expect(frame).toContain("interaction coverage"); + expect(frame).not.toContain("this is a very"); + + await act(async () => { + await setup.mockInput.typeText("w"); + }); + await settleWrapToggle(setup); + + frame = await waitForFrame(setup, (nextFrame) => nextFrame.includes("overage';")); + expect(frame).toContain("this is a very"); + expect(frame).toContain("long wrapped line"); + expect(frame).toContain("overage';"); + + await act(async () => { + await setup.mockInput.typeText("w"); + }); + await settleWrapToggle(setup); + + frame = await waitForFrame( + setup, + (nextFrame) => + nextFrame.includes("this is a very") && !nextFrame.includes("interaction coverage"), + ); + expect(frame).toContain("this is a very"); + expect(frame).not.toContain("interaction coverage"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + test("bootstrap preferences initialize the visible view state", async () => { const setup = await testRender( {}, onSelectFile, }: { codeHorizontalOffset?: number; @@ -171,6 +172,7 @@ export function DiffPane({ theme: AppTheme; width: number; onOpenAgentNotesAtHunk: (fileId: string, hunkIndex: number) => void; + onScrollCodeHorizontally?: (delta: number) => void; onSelectFile: (fileId: string) => void; }) { const renderer = useRenderer(); @@ -215,6 +217,32 @@ export function DiffPane({ setPrefetchAnchorKey((current) => current ?? selectedHighlightKey); }, [selectedHighlightKey]); + /** Route shifted wheel input into horizontal code-column scrolling without disturbing vertical review scroll. */ + const handleMouseScroll = useCallback( + (event: TuiMouseEvent) => { + const direction = event.scroll?.direction; + if (!direction || wrapLines) { + return; + } + + if (direction === "left") { + onScrollCodeHorizontally(-1); + } else if (direction === "right") { + onScrollCodeHorizontally(1); + } else if (event.modifiers.shift && direction === "up") { + onScrollCodeHorizontally(-1); + } else if (event.modifiers.shift && direction === "down") { + onScrollCodeHorizontally(1); + } else { + return; + } + + event.preventDefault(); + event.stopPropagation(); + }, + [onScrollCodeHorizontally, wrapLines], + ); + const allAgentNotesByFile = useMemo(() => { const next = new Map(); @@ -830,6 +858,7 @@ export function DiffPane({ contentOptions={{ backgroundColor: theme.panel }} verticalScrollbarOptions={{ visible: false }} horizontalScrollbarOptions={{ visible: false }} + onMouseScroll={handleMouseScroll} > { "d / u half page down / up", "[ / ] previous / next hunk", "{ / } previous / next comment", - "← / → scroll code left / right", + "← / → scroll code (Shift = faster)", "Home / End jump to top / bottom", "View", "1 / 2 / 0 split / stack / auto", diff --git a/src/ui/hooks/useAppKeyboardShortcuts.ts b/src/ui/hooks/useAppKeyboardShortcuts.ts index a4872a73..6f7545da 100644 --- a/src/ui/hooks/useAppKeyboardShortcuts.ts +++ b/src/ui/hooks/useAppKeyboardShortcuts.ts @@ -15,6 +15,8 @@ import { type FocusArea = "files" | "filter"; type ScrollUnit = "step" | "viewport" | "content" | "half"; +const FAST_CODE_HORIZONTAL_SCROLL_COLUMNS = 8; + export interface UseAppKeyboardShortcutsOptions { activeMenuId: MenuId | null; activateCurrentMenuItem: () => void; @@ -141,12 +143,12 @@ export function useAppKeyboardShortcuts({ } if (key.name === "left") { - scrollCodeHorizontally(-1); + scrollCodeHorizontally(key.shift ? -FAST_CODE_HORIZONTAL_SCROLL_COLUMNS : -1); return; } if (key.name === "right") { - scrollCodeHorizontally(1); + scrollCodeHorizontally(key.shift ? FAST_CODE_HORIZONTAL_SCROLL_COLUMNS : 1); return; } @@ -304,12 +306,12 @@ export function useAppKeyboardShortcuts({ } if (key.name === "left") { - scrollCodeHorizontally(-1); + scrollCodeHorizontally(key.shift ? -FAST_CODE_HORIZONTAL_SCROLL_COLUMNS : -1); return; } if (key.name === "right") { - scrollCodeHorizontally(1); + scrollCodeHorizontally(key.shift ? FAST_CODE_HORIZONTAL_SCROLL_COLUMNS : 1); return; } From 935a0f73aa2c5b91764970fcc9f43e24c5dfd756 Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Mon, 6 Apr 2026 15:19:28 -0400 Subject: [PATCH 3/3] fix(review): tighten horizontal code scrolling Clamp horizontal reveal to the visible code viewport and share the renderer's tab-expansion math so long lines stop overscrolling into blank space. Preserve vertical position for shift-wheel scrolling and add PTY coverage for real-terminal horizontal reveal and wrap reset behavior. --- src/ui/App.tsx | 49 +++++++------ src/ui/AppHost.interactions.test.tsx | 38 ++++++++++ src/ui/components/panes/DiffPane.tsx | 19 ++++- src/ui/diff/PierreDiffView.tsx | 3 +- src/ui/diff/codeColumns.ts | 104 +++++++++++++++++++++++++++ src/ui/diff/pierre.ts | 3 +- src/ui/diff/renderRows.tsx | 67 ++++++++--------- src/ui/lib/diffSectionGeometry.ts | 3 +- test/pty/ui-integration.test.ts | 98 ++++++++++++++++++++++++- 9 files changed, 320 insertions(+), 64 deletions(-) create mode 100644 src/ui/diff/codeColumns.ts diff --git a/src/ui/App.tsx b/src/ui/App.tsx index 378a95bc..f06ce7c5 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -5,7 +5,7 @@ import { } from "@opentui/core"; import { useRenderer, useTerminalDimensions } from "@opentui/react"; import { Suspense, lazy, useCallback, useEffect, useMemo, useState, useRef } from "react"; -import type { AppBootstrap, CliInput, DiffFile, LayoutMode } from "../core/types"; +import type { AppBootstrap, CliInput, LayoutMode } from "../core/types"; import { canReloadInput, computeWatchSignature } from "../core/watch"; import { HunkHostClient } from "../mcp/client"; import type { ReloadedSessionResult } from "../mcp/types"; @@ -14,6 +14,11 @@ import { StatusBar } from "./components/chrome/StatusBar"; import { DiffPane } from "./components/panes/DiffPane"; import { SidebarPane } from "./components/panes/SidebarPane"; import { PaneDivider } from "./components/panes/PaneDivider"; +import { + findMaxLineNumber, + maxFileCodeLineWidth, + resolveCodeViewportWidth, +} from "./diff/codeColumns"; import { useAppKeyboardShortcuts } from "./hooks/useAppKeyboardShortcuts"; import { useHunkSessionBridge } from "./hooks/useHunkSessionBridge"; import { useMenuController } from "./hooks/useMenuController"; @@ -40,23 +45,6 @@ function clamp(value: number, min: number, max: number) { return Math.min(Math.max(value, min), max); } -/** Measure one diff line after the same tab expansion used by the renderer. */ -function renderedCodeLineWidth(line: string | undefined) { - return (line ?? "").replace(/\n$/, "").replaceAll("\t", " ").length; -} - -/** Track the widest code line that could benefit from horizontal reveal. */ -function maxFileCodeLineWidth(file: DiffFile) { - const deletionLines = file.metadata.deletionLines ?? []; - const additionLines = file.metadata.additionLines ?? []; - - return Math.max( - 0, - ...deletionLines.map(renderedCodeLineWidth), - ...additionLines.map(renderedCodeLineWidth), - ); -} - /** Preserve the active app view settings when rebuilding the current input. */ function withCurrentViewOptions( input: CliInput, @@ -188,6 +176,26 @@ export function App({ const diffPaneWidth = renderSidebar ? Math.max(DIFF_MIN_WIDTH, availableCenterWidth - clampedSidebarWidth) : Math.max(0, availableCenterWidth); + const diffContentWidth = Math.max(12, diffPaneWidth - 2); + const maxVisibleLineNumber = useMemo( + () => + filteredFiles.reduce( + (maxLineNumber, file) => Math.max(maxLineNumber, findMaxLineNumber(file)), + 1, + ), + [filteredFiles], + ); + const maxLineNumberDigits = String(maxVisibleLineNumber).length; + const codeViewportWidth = useMemo( + () => + resolveCodeViewportWidth( + resolvedLayout, + diffContentWidth, + maxLineNumberDigits, + showLineNumbers, + ), + [diffContentWidth, maxLineNumberDigits, resolvedLayout, showLineNumbers], + ); const isResizingSidebar = resizeDragOriginX !== null && resizeStartWidth !== null; const dividerHitLeft = Math.max( 1, @@ -246,9 +254,9 @@ export function App({ filteredFiles.reduce( (maxWidth, file) => Math.max(maxWidth, maxFileCodeLineWidth(file)), 0, - ) - 1, + ) - codeViewportWidth, ), - [filteredFiles], + [codeViewportWidth, filteredFiles], ); useEffect(() => { @@ -609,7 +617,6 @@ export function App({ ); const topTitle = `${bootstrap.changeset.title} +${totalAdditions} -${totalDeletions}`; const sidebarTextWidth = Math.max(8, clampedSidebarWidth - 2); - const diffContentWidth = Math.max(12, diffPaneWidth - 2); const diffHeaderStatsWidth = Math.min(24, Math.max(16, Math.floor(diffContentWidth / 3))); const diffHeaderLabelWidth = Math.max(8, diffContentWidth - diffHeaderStatsWidth - 1); const diffSeparatorWidth = Math.max(4, diffContentWidth - 2); diff --git a/src/ui/AppHost.interactions.test.tsx b/src/ui/AppHost.interactions.test.tsx index 133faef4..a0a3f524 100644 --- a/src/ui/AppHost.interactions.test.tsx +++ b/src/ui/AppHost.interactions.test.tsx @@ -297,6 +297,10 @@ function firstVisibleAddedLine(frame: string) { return frame.match(/line\d{2} = 1\d{2}/)?.[0] ?? null; } +function firstVisibleAddedLineNumber(frame: string) { + return frame.match(/▌\s*(\d+)\s+\+/)?.[1] ?? null; +} + describe("App interactions", () => { test("keyboard shortcuts toggle notes, line numbers, and hunk metadata", async () => { const setup = await testRender(, { @@ -590,6 +594,40 @@ describe("App interactions", () => { } }); + test("shift plus mouse wheel does not move the vertical review position", async () => { + const setup = await testRender(, { + width: 92, + height: 20, + }); + + try { + await flush(setup); + + let frame = setup.captureCharFrame(); + const initialTopLine = firstVisibleAddedLineNumber(frame); + expect(initialTopLine).toBeTruthy(); + expect(frame).not.toContain("viewport anchoring"); + + for (let index = 0; index < 8; index += 1) { + await act(async () => { + await setup.mockMouse.scroll(60, 10, "down", { modifiers: { shift: true } }); + }); + await flush(setup); + frame = setup.captureCharFrame(); + if (frame.includes("viewport anchoring")) { + break; + } + } + + expect(frame).toContain("viewport anchoring"); + expect(firstVisibleAddedLineNumber(frame)).toBe(initialTopLine); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + test("wrap toggles reset the horizontal code offset", async () => { const setup = await testRender(, { width: 92, diff --git a/src/ui/components/panes/DiffPane.tsx b/src/ui/components/panes/DiffPane.tsx index 1939019f..d22c6586 100644 --- a/src/ui/components/panes/DiffPane.tsx +++ b/src/ui/components/panes/DiffPane.tsx @@ -220,11 +220,15 @@ export function DiffPane({ /** Route shifted wheel input into horizontal code-column scrolling without disturbing vertical review scroll. */ const handleMouseScroll = useCallback( (event: TuiMouseEvent) => { + const scrollBox = scrollRef.current; const direction = event.scroll?.direction; - if (!direction || wrapLines) { + if (!direction || !scrollBox || wrapLines) { return; } + const preservedScrollTop = scrollBox.scrollTop; + const preservedScrollLeft = scrollBox.scrollLeft; + if (direction === "left") { onScrollCodeHorizontally(-1); } else if (direction === "right") { @@ -237,10 +241,21 @@ export function DiffPane({ return; } + // OpenTUI runs ScrollBox's own wheel handler after this listener and it does not honor + // preventDefault(), so restore the pre-event viewport position on the next microtask. + queueMicrotask(() => { + const currentScrollBox = scrollRef.current; + if (!currentScrollBox) { + return; + } + + currentScrollBox.scrollTo({ x: preservedScrollLeft, y: preservedScrollTop }); + }); + event.preventDefault(); event.stopPropagation(); }, - [onScrollCodeHorizontally, wrapLines], + [onScrollCodeHorizontally, scrollRef, wrapLines], ); const allAgentNotesByFile = useMemo(() => { diff --git a/src/ui/diff/PierreDiffView.tsx b/src/ui/diff/PierreDiffView.tsx index 48ce0fdd..02e67197 100644 --- a/src/ui/diff/PierreDiffView.tsx +++ b/src/ui/diff/PierreDiffView.tsx @@ -4,10 +4,11 @@ import { AgentInlineNote, AgentInlineNoteGuideCap } from "../components/panes/Ag import type { VisibleAgentNote } from "../lib/agentAnnotations"; import { reviewRowId } from "../lib/ids"; import type { AppTheme } from "../themes"; +import { findMaxLineNumber } from "./codeColumns"; import { buildSplitRows, buildStackRows } from "./pierre"; import { plannedReviewRowVisible } from "./plannedReviewRows"; import { buildReviewRenderPlan } from "./reviewRenderPlan"; -import { diffMessage, DiffRowView, findMaxLineNumber, fitText } from "./renderRows"; +import { diffMessage, DiffRowView, fitText } from "./renderRows"; import { useHighlightedDiff } from "./useHighlightedDiff"; const EMPTY_ANNOTATED_HUNK_INDICES = new Set(); diff --git a/src/ui/diff/codeColumns.ts b/src/ui/diff/codeColumns.ts new file mode 100644 index 00000000..7f5568e2 --- /dev/null +++ b/src/ui/diff/codeColumns.ts @@ -0,0 +1,104 @@ +import type { DiffFile, LayoutMode } from "../../core/types"; + +export const DIFF_CODE_TAB_WIDTH = 2; +export const DIFF_RAIL_PREFIX_WIDTH = 1; +export const DIFF_SPLIT_SEPARATOR_WIDTH = 1; + +/** Expand tabs the same way the diff renderer does before measuring visible columns. */ +export function expandDiffTabs(text: string) { + return text.replaceAll("\t", " ".repeat(DIFF_CODE_TAB_WIDTH)); +} + +/** Measure one rendered code line after tab expansion and newline trimming. */ +export function measureRenderedCodeLineWidth(line: string | undefined) { + return expandDiffTabs((line ?? "").replace(/\n$/, "")).length; +} + +/** Track the widest rendered code line for one file. */ +export function maxFileCodeLineWidth(file: DiffFile) { + const deletionLines = file.metadata.deletionLines ?? []; + const additionLines = file.metadata.additionLines ?? []; + + return Math.max( + 0, + ...deletionLines.map(measureRenderedCodeLineWidth), + ...additionLines.map(measureRenderedCodeLineWidth), + ); +} + +/** Find the widest line-number gutter needed for one file. */ +export function findMaxLineNumber(file: DiffFile) { + let highest = 0; + + for (const hunk of file.metadata.hunks) { + highest = Math.max( + highest, + hunk.deletionStart + hunk.deletionCount, + hunk.additionStart + hunk.additionCount, + ); + } + + return Math.max(highest, 1); +} + +/** Split-view panes reserve one rail column on the left and one separator column in the middle. */ +export function resolveSplitPaneWidths(width: number) { + const usableWidth = Math.max(0, width - DIFF_RAIL_PREFIX_WIDTH - DIFF_SPLIT_SEPARATOR_WIDTH); + const leftWidth = Math.max(0, DIFF_RAIL_PREFIX_WIDTH + Math.floor(usableWidth / 2)); + const rightWidth = Math.max( + 0, + DIFF_SPLIT_SEPARATOR_WIDTH + usableWidth - Math.floor(usableWidth / 2), + ); + + return { leftWidth, rightWidth }; +} + +/** Resolve the split-cell gutter and code viewport after the rail prefix. */ +export function resolveSplitCellGeometry( + width: number, + lineNumberDigits: number, + showLineNumbers: boolean, + prefixWidth = DIFF_RAIL_PREFIX_WIDTH, +) { + const availableWidth = Math.max(0, width - prefixWidth); + const gutterWidth = Math.min(availableWidth, showLineNumbers ? lineNumberDigits + 3 : 2); + + return { + gutterWidth, + contentWidth: Math.max(0, availableWidth - gutterWidth), + }; +} + +/** Resolve the stack-cell gutter and code viewport after the left rail prefix. */ +export function resolveStackCellGeometry( + width: number, + lineNumberDigits: number, + showLineNumbers: boolean, + prefixWidth = DIFF_RAIL_PREFIX_WIDTH, +) { + const availableWidth = Math.max(0, width - prefixWidth); + const gutterWidth = Math.min(availableWidth, showLineNumbers ? lineNumberDigits * 2 + 5 : 2); + + return { + gutterWidth, + contentWidth: Math.max(0, availableWidth - gutterWidth), + }; +} + +/** Clamp horizontal reveal against the narrowest code viewport in the active layout. */ +export function resolveCodeViewportWidth( + layout: Exclude, + width: number, + lineNumberDigits: number, + showLineNumbers: boolean, +) { + if (layout === "split") { + const { leftWidth, rightWidth } = resolveSplitPaneWidths(width); + return Math.min( + resolveSplitCellGeometry(leftWidth, lineNumberDigits, showLineNumbers).contentWidth, + resolveSplitCellGeometry(rightWidth, lineNumberDigits, showLineNumbers).contentWidth, + ); + } + + return resolveStackCellGeometry(width, lineNumberDigits, showLineNumbers).contentWidth; +} diff --git a/src/ui/diff/pierre.ts b/src/ui/diff/pierre.ts index 3475b660..3fd9bebd 100644 --- a/src/ui/diff/pierre.ts +++ b/src/ui/diff/pierre.ts @@ -8,6 +8,7 @@ import { import { formatHunkHeader } from "../../core/hunkHeader"; import type { DiffFile } from "../../core/types"; import type { AppTheme } from "../themes"; +import { expandDiffTabs } from "./codeColumns"; const PIERRE_THEME = { light: "pierre-light", @@ -108,7 +109,7 @@ export type DiffRow = /** Replace tabs with fixed spaces so terminal cell widths stay predictable. */ function tabify(text: string) { - return text.replaceAll("\t", " "); + return expandDiffTabs(text); } /** Parse an inline CSS style string from Pierre's highlighted HAST output. */ diff --git a/src/ui/diff/renderRows.tsx b/src/ui/diff/renderRows.tsx index 9fddb489..f64ce295 100644 --- a/src/ui/diff/renderRows.tsx +++ b/src/ui/diff/renderRows.tsx @@ -1,6 +1,11 @@ import { memo, type ReactNode } from "react"; import type { DiffFile } from "../../core/types"; import type { AppTheme } from "../themes"; +import { + resolveSplitCellGeometry, + resolveSplitPaneWidths, + resolveStackCellGeometry, +} from "./codeColumns"; import type { DiffRow, RenderSpan, SplitLineCell, StackLineCell } from "./pierre"; /** Clamp a label to one terminal row with an ellipsis. */ @@ -314,9 +319,12 @@ function buildWrappedSplitCell( theme: AppTheme, ) { const palette = splitCellPalette(cell.kind, theme); - const availableWidth = Math.max(0, width - prefixWidth); - const gutterWidth = Math.min(availableWidth, showLineNumbers ? lineNumberDigits + 3 : 2); - const contentWidth = Math.max(0, availableWidth - gutterWidth); + const { gutterWidth, contentWidth } = resolveSplitCellGeometry( + width, + lineNumberDigits, + showLineNumbers, + prefixWidth, + ); const firstGutterText = showLineNumbers ? `${cell.lineNumber ? String(cell.lineNumber).padStart(lineNumberDigits, " ") : " ".repeat(lineNumberDigits)} ${cell.sign}`.padEnd( gutterWidth, @@ -344,9 +352,12 @@ function buildWrappedStackCell( theme: AppTheme, ) { const palette = stackCellPalette(cell.kind, theme); - const availableWidth = Math.max(0, width - prefixWidth); - const gutterWidth = Math.min(availableWidth, showLineNumbers ? lineNumberDigits * 2 + 5 : 2); - const contentWidth = Math.max(0, availableWidth - gutterWidth); + const { gutterWidth, contentWidth } = resolveStackCellGeometry( + width, + lineNumberDigits, + showLineNumbers, + prefixWidth, + ); const oldNumber = cell.oldLineNumber ? String(cell.oldLineNumber).padStart(lineNumberDigits, " ") : " ".repeat(lineNumberDigits); @@ -385,9 +396,12 @@ function renderSplitCell( ) { const palette = splitCellPalette(cell.kind, theme); const prefixWidth = prefix?.text.length ?? 0; - const availableWidth = Math.max(0, width - prefixWidth); - const gutterWidth = Math.min(availableWidth, showLineNumbers ? lineNumberDigits + 3 : 2); - const contentWidth = Math.max(0, availableWidth - gutterWidth); + const { gutterWidth, contentWidth } = resolveSplitCellGeometry( + width, + lineNumberDigits, + showLineNumbers, + prefixWidth, + ); const gutterText = showLineNumbers ? `${cell.lineNumber ? String(cell.lineNumber).padStart(lineNumberDigits, " ") : " ".repeat(lineNumberDigits)} ${cell.sign}`.padEnd( gutterWidth, @@ -433,9 +447,12 @@ function renderStackCell( ) { const palette = stackCellPalette(cell.kind, theme); const prefixWidth = prefix?.text.length ?? 0; - const availableWidth = Math.max(0, width - prefixWidth); - const gutterWidth = Math.min(availableWidth, showLineNumbers ? lineNumberDigits * 2 + 5 : 2); - const contentWidth = Math.max(0, availableWidth - gutterWidth); + const { gutterWidth, contentWidth } = resolveStackCellGeometry( + width, + lineNumberDigits, + showLineNumbers, + prefixWidth, + ); const oldNumber = cell.oldLineNumber ? String(cell.oldLineNumber).padStart(lineNumberDigits, " ") @@ -549,21 +566,6 @@ export function diffMessage(file: DiffFile) { return "No textual hunks to render for this file."; } -/** Find the widest line-number column needed for this file. */ -export function findMaxLineNumber(file: DiffFile) { - let highest = 0; - - for (const hunk of file.metadata.hunks) { - highest = Math.max( - highest, - hunk.deletionStart + hunk.deletionCount, - hunk.additionStart + hunk.additionCount, - ); - } - - return Math.max(highest, 1); -} - /** Render collapsed and hunk-header rows, including the optional AI badge target. */ function renderHeaderRow( row: Extract, @@ -671,10 +673,7 @@ export function measureRenderedRowHeight( } const markerWidth = 1; - const separatorWidth = 1; - const usableWidth = Math.max(0, width - markerWidth - separatorWidth); - const leftWidth = Math.max(0, markerWidth + Math.floor(usableWidth / 2)); - const rightWidth = Math.max(0, separatorWidth + usableWidth - Math.floor(usableWidth / 2)); + const { leftWidth, rightWidth } = resolveSplitPaneWidths(width); const leftLayout = buildWrappedSplitCell( row.left, leftWidth, @@ -749,13 +748,9 @@ function renderRow( } else if (row.type === "split-line") { const guideOnOldSide = noteGuideSide === "old"; const guideOnNewSide = noteGuideSide === "new"; - const markerWidth = 1; - const separatorWidth = 1; // Reserve fixed columns for the diff rails and center separator slot. - const usableWidth = Math.max(0, width - markerWidth - separatorWidth); - const leftWidth = Math.max(0, markerWidth + Math.floor(usableWidth / 2)); - const rightWidth = Math.max(0, separatorWidth + usableWidth - Math.floor(usableWidth / 2)); + const { leftWidth, rightWidth } = resolveSplitPaneWidths(width); const rightRenderWidth = Math.max(0, rightWidth - (guideOnNewSide ? 1 : 0)); const leftPrefix = { text: guideOnOldSide ? "│" : marker(), diff --git a/src/ui/lib/diffSectionGeometry.ts b/src/ui/lib/diffSectionGeometry.ts index 17b71098..ecc42ef6 100644 --- a/src/ui/lib/diffSectionGeometry.ts +++ b/src/ui/lib/diffSectionGeometry.ts @@ -1,7 +1,8 @@ import type { DiffFile, LayoutMode } from "../../core/types"; import { measureAgentInlineNoteHeight } from "../components/panes/AgentInlineNote"; +import { findMaxLineNumber } from "../diff/codeColumns"; import { buildSplitRows, buildStackRows } from "../diff/pierre"; -import { measureRenderedRowHeight, findMaxLineNumber } from "../diff/renderRows"; +import { measureRenderedRowHeight } from "../diff/renderRows"; import type { PlannedHunkBounds } from "../diff/plannedReviewRows"; import { buildReviewRenderPlan, type PlannedReviewRow } from "../diff/reviewRenderPlan"; import type { SectionGeometry, VerticalBounds } from "./diffSpatial"; diff --git a/test/pty/ui-integration.test.ts b/test/pty/ui-integration.test.ts index 5d65e9f7..d32f1eba 100644 --- a/test/pty/ui-integration.test.ts +++ b/test/pty/ui-integration.test.ts @@ -72,7 +72,6 @@ describe("live UI integration", () => { timeout: 15_000, }); - expect(initial).toContain("[AI]"); expect(initial).not.toContain("Adds bonus export."); await session.press("a"); @@ -117,7 +116,6 @@ describe("live UI integration", () => { ); expect(secondHunk).toContain("line60 = 6000"); - expect(secondHunk).toContain("@@ -57,12 +57,12 @@"); expect(secondHunk).not.toContain("line1 = 100"); } finally { session.close(); @@ -506,6 +504,102 @@ describe("live UI integration", () => { } }); + test("arrow-key horizontal scrolling reveals hidden code columns in a real PTY", async () => { + const fixture = harness.createLongWrapFilePair(); + const session = await harness.launchHunk({ + args: ["diff", fixture.before, fixture.after, "--mode", "split"], + cols: 102, + rows: 20, + }); + + try { + const initial = await session.waitForText(/View\s+Navigate\s+Theme\s+Agent\s+Help/, { + timeout: 15_000, + }); + + expect(initial).toContain("this is a very long"); + expect(initial).not.toContain("ge';"); + + let shifted = initial; + for (let index = 0; index < 96; index += 1) { + await session.press("right"); + shifted = await session.text(); + if (shifted.includes("ge';")) { + break; + } + } + + expect(shifted).toContain("ge';"); + expect(shifted).not.toContain("this is a very long"); + + let restored = shifted; + for (let index = 0; index < 96; index += 1) { + await session.press("left"); + restored = await session.text(); + if (restored.includes("this is a very long") && !restored.includes("ge';")) { + break; + } + } + + expect(restored).toContain("this is a very long"); + expect(restored).not.toContain("ge';"); + } finally { + session.close(); + } + }); + + test("wrap toggles reset horizontal code scrolling in a real PTY", async () => { + const fixture = harness.createLongWrapFilePair(); + const session = await harness.launchHunk({ + args: ["diff", fixture.before, fixture.after, "--mode", "split"], + cols: 102, + rows: 20, + }); + + try { + const initial = await session.waitForText(/View\s+Navigate\s+Theme\s+Agent\s+Help/, { + timeout: 15_000, + }); + + expect(initial).toContain("this is a very long"); + expect(initial).not.toContain("ge';"); + + let shifted = initial; + for (let index = 0; index < 96; index += 1) { + await session.press("right"); + shifted = await session.text(); + if (shifted.includes("ge';")) { + break; + } + } + + expect(shifted).toContain("ge';"); + expect(shifted).not.toContain("this is a very long"); + + await session.press("w"); + const wrapped = await harness.waitForSnapshot( + session, + (text) => text.includes("ge';"), + 5_000, + ); + + expect(wrapped).toContain("this is a very long"); + expect(wrapped).toContain("ge';"); + + await session.press("w"); + const reset = await harness.waitForSnapshot( + session, + (text) => text.includes("this is a very long") && !text.includes("ge';"), + 5_000, + ); + + expect(reset).toContain("this is a very long"); + expect(reset).not.toContain("ge';"); + } finally { + session.close(); + } + }); + test("the first mouse-wheel step still advances content under the always-pinned file header above a collapsed gap", async () => { const fixture = harness.createCollapsedTopRepoFixture(); const session = await harness.launchHunk({