diff --git a/AGENTS.md b/AGENTS.md index 924bb05..fea8808 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -46,6 +46,12 @@ CLI input - Prefer one implementation path per feature instead of separate "old" and "new" codepaths that duplicate behavior. - When refactoring logic that spans helpers and UI components, add tests at the level where the user-visible behavior actually lives, not only at the lowest helper layer. +## code comments + +- Add short JSDoc-style comments to functions and helpers. +- Add inline comments for intent, invariants, or tricky behavior that would not be obvious to a fresh reader. +- Skip comments that only narrate what the code already says. + ## review behavior - Default behavior is a multi-file review stream in sidebar order. diff --git a/src/ui/App.tsx b/src/ui/App.tsx index d18dd13..998595a 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -107,6 +107,7 @@ function AppShell({ const terminal = useTerminalDimensions(); const filesScrollRef = useRef(null); const diffScrollRef = useRef(null); + const wrapToggleScrollTopRef = useRef(null); const [layoutMode, setLayoutMode] = useState(bootstrap.initialMode); const [themeId, setThemeId] = useState( () => resolveTheme(bootstrap.initialTheme, renderer.themeMode).id, @@ -232,9 +233,10 @@ function AppShell({ }, [maxFilesPaneWidth, showFilesPane]); useEffect(() => { - // Force an intermediate redraw when the shell geometry changes so pane relayout feels immediate. + // Force an intermediate redraw when shell geometry or row-wrapping changes so pane relayout + // feels immediate after toggling split/stack or line wrapping. renderer.intermediateRender(); - }, [renderer, resolvedLayout, showFilesPane, terminal.height, terminal.width]); + }, [renderer, resolvedLayout, showFilesPane, terminal.height, terminal.width, wrapLines]); useEffect(() => { if (!selectedFile && filteredFiles[0]) { @@ -335,6 +337,9 @@ function AppShell({ /** Toggle whether diff code rows wrap instead of truncating to one terminal row. */ const toggleLineWrap = () => { + // 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; setWrapLines((current) => !current); }; @@ -662,6 +667,11 @@ function AppShell({ return; } + if (key.name === "w" || key.sequence === "w") { + toggleLineWrap(); + return; + } + return; } @@ -949,6 +959,7 @@ function AppShell({ showLineNumbers={showLineNumbers} showHunkHeaders={showHunkHeaders} wrapLines={wrapLines} + wrapToggleScrollTop={wrapToggleScrollTopRef.current} theme={activeTheme} width={diffPaneWidth} onOpenAgentNotesAtHunk={openAgentNotesAtHunk} diff --git a/src/ui/components/panes/DiffPane.tsx b/src/ui/components/panes/DiffPane.tsx index 9026196..5cb2866 100644 --- a/src/ui/components/panes/DiffPane.tsx +++ b/src/ui/components/panes/DiffPane.tsx @@ -11,7 +11,11 @@ import { import type { DiffFile, LayoutMode } from "../../../core/types"; import type { VisibleAgentNote } from "../../lib/agentAnnotations"; import { computeHunkRevealScrollTop } from "../../lib/hunkScroll"; -import { measureDiffSectionMetrics } from "../../lib/sectionHeights"; +import { + measureDiffSectionMetrics, + type DiffSectionMetrics, + type DiffSectionRowMetric, +} from "../../lib/sectionHeights"; import { diffHunkId, diffSectionId } from "../../lib/ids"; import type { AppTheme } from "../../themes"; import { DiffSection } from "./DiffSection"; @@ -20,6 +24,101 @@ import { VerticalScrollbar, type VerticalScrollbarHandle } from "../scrollbar/Ve const EMPTY_VISIBLE_AGENT_NOTES: VisibleAgentNote[] = []; +/** Identify the rendered diff row that currently owns the top of the viewport. */ +interface ViewportRowAnchor { + fileId: string; + rowKey: string; + rowOffsetWithin: number; +} + +/** Find the rendered row metric covering a vertical offset within one file body. */ +function binarySearchRowMetric(rowMetrics: DiffSectionRowMetric[], relativeTop: number) { + let low = 0; + let high = rowMetrics.length - 1; + + while (low <= high) { + const mid = (low + high) >>> 1; + const rowMetric = rowMetrics[mid]!; + + if (relativeTop < rowMetric.offset) { + high = mid - 1; + } else if (relativeTop >= rowMetric.offset + rowMetric.height) { + low = mid + 1; + } else { + return rowMetric; + } + } + + return undefined; +} + +/** Capture a stable top-row anchor from the pre-toggle layout so it can be restored later. */ +function findViewportRowAnchor( + files: DiffFile[], + sectionMetrics: DiffSectionMetrics[], + scrollTop: number, +) { + let offsetY = 0; + + for (let index = 0; index < files.length; index += 1) { + if (index > 0) { + offsetY += 1; + } + + offsetY += 1; + const bodyTop = offsetY; + const metrics = sectionMetrics[index]; + const bodyHeight = metrics?.bodyHeight ?? 0; + const relativeTop = scrollTop - bodyTop; + + if (relativeTop >= 0 && relativeTop < bodyHeight && metrics) { + const rowMetric = binarySearchRowMetric(metrics.rowMetrics, relativeTop); + if (rowMetric) { + return { + fileId: files[index]!.id, + rowKey: rowMetric.key, + rowOffsetWithin: relativeTop - rowMetric.offset, + } satisfies ViewportRowAnchor; + } + } + + offsetY = bodyTop + bodyHeight; + } + + return null; +} + +/** Resolve a captured row anchor into its new scrollTop after wrapping/layout metrics change. */ +function resolveViewportRowAnchorTop( + files: DiffFile[], + sectionMetrics: DiffSectionMetrics[], + anchor: ViewportRowAnchor, +) { + let offsetY = 0; + + for (let index = 0; index < files.length; index += 1) { + if (index > 0) { + offsetY += 1; + } + + offsetY += 1; + const bodyTop = offsetY; + const file = files[index]; + const metrics = sectionMetrics[index]; + if (file?.id === anchor.fileId && metrics) { + const rowMetric = metrics.rowMetricsByKey.get(anchor.rowKey); + if (rowMetric) { + return bodyTop + rowMetric.offset + Math.min(anchor.rowOffsetWithin, rowMetric.height - 1); + } + return bodyTop; + } + + offsetY = bodyTop + (metrics?.bodyHeight ?? 0); + } + + return 0; +} + /** Render the main multi-file review stream. */ export function DiffPane({ diffContentWidth, @@ -36,6 +135,7 @@ export function DiffPane({ showLineNumbers, showHunkHeaders, wrapLines, + wrapToggleScrollTop, theme, width, onOpenAgentNotesAtHunk, @@ -55,6 +155,7 @@ export function DiffPane({ showLineNumbers: boolean; showHunkHeaders: boolean; wrapLines: boolean; + wrapToggleScrollTop: number | null; theme: AppTheme; width: number; onOpenAgentNotesAtHunk: (fileId: string, hunkIndex: number) => void; @@ -132,6 +233,10 @@ export function DiffPane({ const [scrollViewport, setScrollViewport] = useState({ top: 0, height: 0 }); const scrollbarRef = useRef(null); const prevScrollTopRef = useRef(0); + const previousSectionMetricsRef = useRef(null); + const previousFilesRef = useRef(files); + const previousWrapLinesRef = useRef(wrapLines); + const suppressNextSelectionAutoScrollRef = useRef(false); useEffect(() => { const updateViewport = () => { @@ -157,8 +262,20 @@ export function DiffPane({ }, [scrollRef]); const baseSectionMetrics = useMemo( - () => files.map((file) => measureDiffSectionMetrics(file, layout, showHunkHeaders, theme)), - [files, layout, showHunkHeaders, theme], + () => + files.map((file) => + measureDiffSectionMetrics( + file, + layout, + showHunkHeaders, + theme, + EMPTY_VISIBLE_AGENT_NOTES, + diffContentWidth, + showLineNumbers, + wrapLines, + ), + ), + [diffContentWidth, files, layout, showHunkHeaders, showLineNumbers, theme, wrapLines], ); const baseEstimatedBodyHeights = useMemo( () => baseSectionMetrics.map((metrics) => metrics.bodyHeight), @@ -226,6 +343,8 @@ export function DiffPane({ theme, visibleNotes, diffContentWidth, + showLineNumbers, + wrapLines, ); }), [ @@ -234,8 +353,10 @@ export function DiffPane({ files, layout, showHunkHeaders, + showLineNumbers, theme, visibleAgentNotesByFile, + wrapLines, ], ); const estimatedBodyHeights = useMemo( @@ -325,6 +446,57 @@ export function DiffPane({ const prevSelectedAnchorIdRef = useRef(null); useLayoutEffect(() => { + const wrapChanged = previousWrapLinesRef.current !== wrapLines; + const previousSectionMetrics = previousSectionMetricsRef.current; + const previousFiles = previousFilesRef.current; + + if (wrapChanged && previousSectionMetrics && previousFiles.length > 0) { + const previousScrollTop = + // Prefer the synchronously captured pre-toggle position so anchor restoration does not + // race the polling-based viewport snapshot. + wrapToggleScrollTop != null + ? wrapToggleScrollTop + : Math.max(prevScrollTopRef.current, scrollViewport.top); + const anchor = findViewportRowAnchor( + previousFiles, + previousSectionMetrics, + previousScrollTop, + ); + if (anchor) { + const nextTop = resolveViewportRowAnchorTop(files, sectionMetrics, anchor); + const restoreViewportAnchor = () => { + scrollRef.current?.scrollTo(nextTop); + }; + + restoreViewportAnchor(); + // The wrap-toggle anchor restore should win over the usual selection-following behavior. + suppressNextSelectionAutoScrollRef.current = true; + // Retry across a couple of repaint cycles so the restored top-row anchor sticks + // after wrapped row heights and viewport culling settle. + const retryDelays = [0, 16, 48]; + const timeouts = retryDelays.map((delay) => setTimeout(restoreViewportAnchor, delay)); + + previousWrapLinesRef.current = wrapLines; + previousSectionMetricsRef.current = sectionMetrics; + previousFilesRef.current = files; + + return () => { + timeouts.forEach((timeout) => clearTimeout(timeout)); + }; + } + } + + previousWrapLinesRef.current = wrapLines; + previousSectionMetricsRef.current = sectionMetrics; + previousFilesRef.current = files; + }, [files, scrollRef, scrollViewport.top, sectionMetrics, wrapLines, wrapToggleScrollTop]); + + useLayoutEffect(() => { + if (suppressNextSelectionAutoScrollRef.current) { + suppressNextSelectionAutoScrollRef.current = false; + return; + } + if (!selectedAnchorId && !selectedEstimatedHunkBounds) { prevSelectedAnchorIdRef.current = null; return; @@ -384,7 +556,8 @@ export function DiffPane({ } }; - // Run after this pane renders the selected section/hunk, then retry briefly while layout settles. + // Run after this pane renders the selected section/hunk, then retry briefly while layout + // settles across a couple of repaint cycles. scrollSelectionIntoView(); const retryDelays = [0, 16, 48]; const timeouts = retryDelays.map((delay) => setTimeout(scrollSelectionIntoView, delay)); @@ -429,7 +602,12 @@ export function DiffPane({ verticalScrollbarOptions={{ visible: false }} horizontalScrollbarOptions={{ visible: false }} > - + {files.map((file, index) => { const shouldRenderSection = visibleWindowedFileIds?.has(file.id) ?? true; const shouldPrefetchVisibleHighlight = diff --git a/src/ui/lib/sectionHeights.ts b/src/ui/lib/sectionHeights.ts index 1567b51..3eb96aa 100644 --- a/src/ui/lib/sectionHeights.ts +++ b/src/ui/lib/sectionHeights.ts @@ -1,15 +1,26 @@ import type { DiffFile, LayoutMode } from "../../core/types"; +import { measureAgentInlineNoteHeight } from "../components/panes/AgentInlineNote"; import { buildSplitRows, buildStackRows } from "../diff/pierre"; -import { measurePlannedHunkBounds, type PlannedHunkBounds } from "../diff/plannedReviewRows"; -import { buildReviewRenderPlan } from "../diff/reviewRenderPlan"; +import { measureRenderedRowHeight, findMaxLineNumber } from "../diff/renderRows"; +import type { PlannedHunkBounds } from "../diff/plannedReviewRows"; +import { buildReviewRenderPlan, type PlannedReviewRow } from "../diff/reviewRenderPlan"; +import { reviewRowId } from "./ids"; import type { VisibleAgentNote } from "./agentAnnotations"; import type { AppTheme } from "../themes"; +export interface DiffSectionRowMetric { + key: string; + offset: number; + height: number; +} + /** Cached placeholder sizing and hunk navigation metrics for one file section. */ export interface DiffSectionMetrics { bodyHeight: number; hunkAnchorRows: Map; hunkBounds: Map; + rowMetrics: DiffSectionRowMetric[]; + rowMetricsByKey: Map; } const NOTE_AWARE_SECTION_METRICS_CACHE = new WeakMap< @@ -17,7 +28,7 @@ const NOTE_AWARE_SECTION_METRICS_CACHE = new WeakMap< Map >(); -/** Build the same planned rows the renderer will consume, but without requiring mounted UI. */ +/** Build the exact review rows for one file before converting them into height metrics. */ function buildBasePlannedRows( file: DiffFile, layout: Exclude, @@ -37,9 +48,51 @@ function buildBasePlannedRows( }); } +/** Measure how many terminal rows one planned review row occupies for the current view settings. */ +function plannedRowHeight( + row: PlannedReviewRow, + showHunkHeaders: boolean, + layout: Exclude, + width: number, + lineNumberDigits: number, + showLineNumbers: boolean, + wrapLines: boolean, + theme: AppTheme, +) { + if (row.kind === "inline-note") { + return measureAgentInlineNoteHeight({ + annotation: row.annotation, + anchorSide: row.anchorSide, + layout, + width, + }); + } + + if (row.kind === "note-guide-cap") { + return 1; + } + + return measureRenderedRowHeight( + row.row, + width, + lineNumberDigits, + showLineNumbers, + showHunkHeaders, + wrapLines, + theme, + ); +} + +/** Return whether a measured review row should count toward the visible extent of its hunk. */ +function rowContributesToHunkBounds(row: PlannedReviewRow) { + // Collapsed gap rows sit between hunks, so they affect total section height but should not make a + // selected hunk look taller than the rows that actually belong to it. + return !(row.kind === "diff-row" && row.row.type === "collapsed"); +} + /** * Measure one file section from the same render plan used by PierreDiffView. - * This drives the windowed review stream and keeps scrolling and rendering aligned. + * This keeps placeholder sizing, viewport anchoring, and selected-hunk reveal math aligned. */ export function measureDiffSectionMetrics( file: DiffFile, @@ -48,16 +101,22 @@ export function measureDiffSectionMetrics( theme: AppTheme, visibleAgentNotes: VisibleAgentNote[] = [], width = 0, + showLineNumbers = true, + wrapLines = false, ): DiffSectionMetrics { if (file.metadata.hunks.length === 0) { return { bodyHeight: 1, hunkAnchorRows: new Map(), hunkBounds: new Map(), + rowMetrics: [], + rowMetricsByKey: new Map(), }; } - const cacheKey = `${file.id}:${layout}:${showHunkHeaders ? 1 : 0}:${theme.id}:${width}`; + // Width, wrapping, and line-number visibility all affect rendered row heights, so they must + // participate in the cache key alongside the structural file/layout inputs. + const cacheKey = `${file.id}:${layout}:${showHunkHeaders ? 1 : 0}:${theme.id}:${width}:${showLineNumbers ? 1 : 0}:${wrapLines ? 1 : 0}`; if (visibleAgentNotes.length > 0) { const cachedByNotes = NOTE_AWARE_SECTION_METRICS_CACHE.get(visibleAgentNotes); const cached = cachedByNotes?.get(cacheKey); @@ -67,13 +126,65 @@ export function measureDiffSectionMetrics( } const plannedRows = buildBasePlannedRows(file, layout, showHunkHeaders, theme, visibleAgentNotes); - // Reuse the same bounds pass as the live renderer so placeholder sizing and navigation math stay - // in lock-step with what the user actually sees. - const metrics = measurePlannedHunkBounds(plannedRows, { - showHunkHeaders, - layout, - width, - }); + const hunkAnchorRows = new Map(); + const hunkBounds = new Map(); + const rowMetrics: DiffSectionRowMetric[] = []; + const rowMetricsByKey = new Map(); + const lineNumberDigits = String(findMaxLineNumber(file)).length; + let bodyHeight = 0; + + for (const row of plannedRows) { + if (row.kind === "diff-row" && row.anchorId && !hunkAnchorRows.has(row.hunkIndex)) { + hunkAnchorRows.set(row.hunkIndex, bodyHeight); + } + + const height = plannedRowHeight( + row, + showHunkHeaders, + layout, + width, + lineNumberDigits, + showLineNumbers, + wrapLines, + theme, + ); + const rowMetric = { + key: row.key, + // Record both the starting offset and the measured height so callers can translate between + // scroll positions and stable review-row identities across wrap/layout changes. + offset: bodyHeight, + height, + }; + rowMetrics.push(rowMetric); + rowMetricsByKey.set(row.key, rowMetric); + + if (height > 0 && rowContributesToHunkBounds(row)) { + const rowId = reviewRowId(row.key); + const existingBounds = hunkBounds.get(row.hunkIndex); + + if (existingBounds) { + existingBounds.endRowId = rowId; + existingBounds.height += height; + } else { + hunkBounds.set(row.hunkIndex, { + top: bodyHeight, + height, + startRowId: rowId, + endRowId: rowId, + }); + } + } + + bodyHeight += height; + } + + const metrics = { + bodyHeight, + hunkAnchorRows, + hunkBounds, + rowMetrics, + rowMetricsByKey, + }; if (visibleAgentNotes.length > 0) { const cachedByNotes = NOTE_AWARE_SECTION_METRICS_CACHE.get(visibleAgentNotes) ?? new Map(); diff --git a/test/app-interactions.test.tsx b/test/app-interactions.test.tsx index 4595e14..0a56016 100644 --- a/test/app-interactions.test.tsx +++ b/test/app-interactions.test.tsx @@ -130,13 +130,15 @@ function createSingleFileBootstrap(): AppBootstrap { }; } -function createWrapBootstrap(): AppBootstrap { +/** Build a single-file fixture with one long changed line for wrap toggle interaction tests. */ +function createWrapBootstrap(pager = false): AppBootstrap { return { input: { kind: "git", staged: false, options: { mode: "split", + pager, }, }, changeset: { @@ -190,6 +192,39 @@ function createLineScrollBootstrap(pager = false): AppBootstrap { }; } +/** Build a long-line fixture that is tall enough to verify viewport-anchor restoration. */ +function createWrapScrollBootstrap(): AppBootstrap { + const before = + Array.from( + { length: 18 }, + (_, index) => `export const line${String(index + 1).padStart(2, "0")} = ${index + 1};`, + ).join("\n") + "\n"; + const after = + Array.from( + { length: 18 }, + (_, index) => + `export const line${String(index + 1).padStart(2, "0")} = ${index + 101}; // this is intentionally long wrap coverage for viewport anchoring`, + ).join("\n") + "\n"; + + return { + input: { + kind: "git", + staged: false, + options: { + mode: "split", + }, + }, + changeset: { + id: "changeset:app-wrap-scroll", + sourceLabel: "repo", + title: "repo working tree", + files: [createDiffFile("wrap-scroll", "wrap-scroll.ts", before, after, true)], + }, + initialMode: "split", + initialTheme: "midnight", + }; +} + async function flush(setup: Awaited>) { await act(async () => { await setup.renderOnce(); @@ -198,6 +233,42 @@ async function flush(setup: Awaited>) { }); } +/** Let wrap-toggle renders and follow-up layout retries settle before asserting on the frame. */ +async function settleWrapToggle(setup: Awaited>) { + await flush(setup); + await act(async () => { + await Bun.sleep(80); + await setup.renderOnce(); + }); +} + +/** Poll rendered frames until a predicate matches, which keeps interaction tests resilient to async repaints. */ +async function waitForFrame( + setup: Awaited>, + predicate: (frame: string) => boolean, + attempts = 8, +) { + let frame = setup.captureCharFrame(); + + for (let attempt = 0; attempt < attempts; attempt += 1) { + if (predicate(frame)) { + return frame; + } + + await act(async () => { + await Bun.sleep(30); + await setup.renderOnce(); + }); + frame = setup.captureCharFrame(); + } + + return frame; +} + +function firstVisibleAddedLine(frame: string) { + return frame.match(/line\d{2} = 1\d{2}/)?.[0] ?? null; +} + describe("App interactions", () => { test("keyboard shortcuts toggle notes, line numbers, and hunk metadata", async () => { const setup = await testRender(, { @@ -277,6 +348,80 @@ describe("App interactions", () => { } }); + test("pager mode keyboard shortcut can wrap long lines", async () => { + const setup = await testRender(, { + width: 140, + height: 20, + }); + + try { + await flush(setup); + + let frame = setup.captureCharFrame(); + expect(frame).not.toContain("interaction coverage"); + + await act(async () => { + await setup.mockInput.typeText("w"); + }); + await flush(setup); + + frame = setup.captureCharFrame(); + expect(frame).toContain("this is a very"); + expect(frame).toContain("long wrapped line"); + expect(frame).toContain("coverage"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("keyboard shortcut can toggle line wrapping on, off, and on again", async () => { + const setup = await testRender(, { + width: 102, + height: 24, + }); + + try { + await flush(setup); + + let frame = setup.captureCharFrame(); + expect(frame).not.toContain("coverage';"); + + await act(async () => { + await setup.mockInput.typeText("w"); + }); + await settleWrapToggle(setup); + + frame = await waitForFrame(setup, (nextFrame) => nextFrame.includes("coverage';")); + // Assert on a suffix fragment that only appears once the long line has actually wrapped; + // this is more stable than expecting the full sentence to remain on one terminal row. + expect(frame).toContain("wrapped line"); + expect(frame).toContain("coverage';"); + + await act(async () => { + await setup.mockInput.typeText("w"); + }); + await settleWrapToggle(setup); + + frame = await waitForFrame(setup, (nextFrame) => !nextFrame.includes("coverage';")); + expect(frame).not.toContain("coverage';"); + + await act(async () => { + await setup.mockInput.typeText("w"); + }); + await settleWrapToggle(setup); + + frame = await waitForFrame(setup, (nextFrame) => nextFrame.includes("coverage';")); + expect(frame).toContain("wrapped line"); + expect(frame).toContain("coverage';"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + test("bootstrap preferences initialize the visible view state", async () => { const setup = await testRender( { await flush(setup); const initialFrame = setup.captureCharFrame(); - expect(initialFrame).toContain("line01 = 101"); - expect(initialFrame).not.toContain("line08 = 108"); + expect(initialFrame).toContain("line01"); + expect(initialFrame).not.toContain("line08"); let frame = initialFrame; for (let index = 0; index < 24; index += 1) { @@ -543,13 +688,13 @@ describe("App interactions", () => { }); await flush(setup); frame = setup.captureCharFrame(); - if (frame.includes("line08 = 108") && !frame.includes("line01 = 101")) { + if (frame.includes("line08") && !frame.includes("line01")) { break; } } - expect(frame).toContain("line08 = 108"); - expect(frame).not.toContain("line01 = 101"); + expect(frame).toContain("line08"); + expect(frame).not.toContain("line01"); for (let index = 0; index < 12; index += 1) { await act(async () => { @@ -557,12 +702,12 @@ describe("App interactions", () => { }); await flush(setup); frame = setup.captureCharFrame(); - if (frame.includes("line01 = 101")) { + if (frame.includes("line01")) { break; } } - expect(frame).toContain("line01 = 101"); + expect(frame).toContain("line01"); } finally { await act(async () => { setup.renderer.destroy(); @@ -580,8 +725,8 @@ describe("App interactions", () => { await flush(setup); const initialFrame = setup.captureCharFrame(); - expect(initialFrame).toContain("line01 = 101"); - expect(initialFrame).not.toContain("line08 = 108"); + expect(initialFrame).toContain("line01"); + expect(initialFrame).not.toContain("line08"); let frame = initialFrame; for (let index = 0; index < 12; index += 1) { @@ -590,13 +735,13 @@ describe("App interactions", () => { }); await flush(setup); frame = setup.captureCharFrame(); - if (frame.includes("line08 = 108")) { + if (frame.includes("line08")) { break; } } - expect(frame).toContain("line08 = 108"); - expect(frame).not.toContain("line01 = 101"); + expect(frame).toContain("line08"); + expect(frame).not.toContain("line01"); for (let index = 0; index < 12; index += 1) { await act(async () => { @@ -604,12 +749,73 @@ describe("App interactions", () => { }); await flush(setup); frame = setup.captureCharFrame(); - if (frame.includes("line01 = 101")) { + if (frame.includes("line01")) { break; } } + expect(frame).toContain("line01"); + } finally { + await act(async () => { + setup.renderer.destroy(); + }); + } + }); + + test("toggling wrap preserves the current viewport anchor instead of snapping to the top", async () => { + const setup = await testRender(, { + width: 102, + height: 12, + }); + + try { + await flush(setup); + + let frame = setup.captureCharFrame(); expect(frame).toContain("line01 = 101"); + expect(frame).not.toContain("line08 = 108"); + + for (let index = 0; index < 24; index += 1) { + await act(async () => { + await setup.mockInput.pressArrow("down"); + }); + await flush(setup); + frame = setup.captureCharFrame(); + if (frame.includes("line08 = 108") && !frame.includes("line01 = 101")) { + break; + } + } + + expect(frame).toContain("line08 = 108"); + expect(frame).not.toContain("line01 = 101"); + const anchoredLine = firstVisibleAddedLine(frame); + expect(anchoredLine).not.toBeNull(); + + await act(async () => { + await setup.mockInput.typeText("w"); + }); + await flush(setup); + await act(async () => { + await Bun.sleep(80); + await setup.renderOnce(); + }); + + frame = setup.captureCharFrame(); + expect(frame).toContain(anchoredLine!); + expect(firstVisibleAddedLine(frame)).toBe(anchoredLine); + + await act(async () => { + await setup.mockInput.typeText("w"); + }); + await flush(setup); + await act(async () => { + await Bun.sleep(80); + await setup.renderOnce(); + }); + + frame = setup.captureCharFrame(); + expect(frame).toContain(anchoredLine!); + expect(firstVisibleAddedLine(frame)).toBe(anchoredLine); } finally { await act(async () => { setup.renderer.destroy(); @@ -655,15 +861,13 @@ describe("App interactions", () => { try { await flush(setup); - setup.captureCharFrame(); // Capture initial state + setup.captureCharFrame(); - // Press Space to scroll down - should not crash and should change view await act(async () => { await setup.mockInput.pressKey(" "); }); await flush(setup); - // App should still be rendering without errors const frame = setup.captureCharFrame(); expect(frame).toContain("export const line"); } finally { @@ -711,20 +915,17 @@ describe("App interactions", () => { try { await flush(setup); - // First scroll down to have room to scroll back up await act(async () => { await setup.mockInput.pressKey(" "); }); await flush(setup); - setup.captureCharFrame(); // Capture after Space + setup.captureCharFrame(); - // Now scroll back up with PageUp - should not crash await act(async () => { await setup.mockInput.pressKey("pageup"); }); await flush(setup); - // App should still be rendering const frame = setup.captureCharFrame(); expect(frame).toContain("export const line"); } finally { @@ -773,7 +974,6 @@ describe("App interactions", () => { await flush(setup); const initialFrame = setup.captureCharFrame(); - // Press 'd' (half page down) - should not crash await act(async () => { await setup.mockInput.pressKey("d"); }); @@ -781,7 +981,6 @@ describe("App interactions", () => { let frame = setup.captureCharFrame(); expect(frame).toContain("export const line"); - // Press 'u' (half page up) - should not crash await act(async () => { await setup.mockInput.pressKey("u"); }); @@ -789,14 +988,12 @@ describe("App interactions", () => { frame = setup.captureCharFrame(); expect(frame).toContain("export const line"); - // Press 'f' (page down, less-style) - should not crash await act(async () => { await setup.mockInput.pressKey("f"); }); await flush(setup); frame = setup.captureCharFrame(); expect(frame).toContain("export const line"); - // Content should have changed after scrolling expect(frame).not.toEqual(initialFrame); } finally { await act(async () => { diff --git a/test/tty-render-smoke.test.ts b/test/tty-render-smoke.test.ts index e021cd7..71faf6a 100644 --- a/test/tty-render-smoke.test.ts +++ b/test/tty-render-smoke.test.ts @@ -120,12 +120,30 @@ function createFixtureFiles(lines = 1) { return { dir, before, after, agent, patch, coloredPatch }; } +function createLongWrapFixtureFiles() { + const dir = mkdtempSync(join(tmpdir(), "hunk-tty-smoke-")); + tempDirs.push(dir); + + const before = join(dir, "before.ts"); + const after = join(dir, "after.ts"); + + writeFileSync(before, "export const message = 'short';\n"); + writeFileSync( + after, + "export const message = 'this is a very long wrapped line for tty smoke coverage';\n", + ); + + return { dir, before, after }; +} + async function runTtySmoke(options: { mode?: "split" | "stack"; pager?: boolean; agentContext?: boolean; + inputCommand?: string; + longWrapFixture?: boolean; }) { - const fixture = createFixtureFiles(); + const fixture = options.longWrapFixture ? createLongWrapFixtureFiles() : createFixtureFiles(); const transcript = join(fixture.dir, "transcript.txt"); const args = ["diff", fixture.before, fixture.after]; @@ -137,13 +155,13 @@ async function runTtySmoke(options: { args.push("--pager"); } - if (options.agentContext) { - args.push("--agent-context", fixture.agent); + if (options.agentContext && !options.longWrapFixture) { + args.push("--agent-context", (fixture as ReturnType).agent); } const hunkCommand = `bun run ${shellQuote(sourceEntrypoint)} ${args.map(shellQuote).join(" ")}`; const scriptCommand = `timeout 7 script -q -f -e -c ${shellQuote(hunkCommand)} ${shellQuote(transcript)}`; - const inputCommand = `(sleep 2; printf q)`; + const inputCommand = options.inputCommand ?? `(sleep 2; printf q)`; const proc = Bun.spawnSync(["bash", "-lc", `${inputCommand} | ${scriptCommand}`], { cwd: fixture.dir, stdin: "ignore", @@ -219,6 +237,36 @@ describe("TTY render smoke", () => { expect(output).toContain("▌1 + export const answer = 42;"); }); + ttyTest("regular mode can toggle wrapped lines from terminal input", async () => { + if (!ttyToolsAvailable) { + return; + } + + const output = await runTtySmoke({ + mode: "split", + longWrapFixture: true, + inputCommand: `(sleep 2; printf w; sleep 1; printf q)`, + }); + + expect(output).toContain("very long wrapped line for tty s"); + expect(output).toContain("moke coverage';"); + }); + + ttyTest("regular mode can toggle wrapped lines on, off, and on again", async () => { + if (!ttyToolsAvailable) { + return; + } + + const output = await runTtySmoke({ + mode: "split", + longWrapFixture: true, + inputCommand: `(sleep 2; printf www; sleep 1; printf q)`, + }); + + expect(output).toContain("very long wrapped line for tty s"); + expect(output).toContain("moke coverage';"); + }); + ttyTest( "stack mode keeps the terminal-native stacked rows without split separators", async () => { @@ -248,6 +296,22 @@ describe("TTY render smoke", () => { expect(output).toContain("export const answer = 42;"); }); + ttyTest("pager mode can toggle wrapped lines from terminal input", async () => { + if (!ttyToolsAvailable) { + return; + } + + const output = await runTtySmoke({ + mode: "split", + pager: true, + longWrapFixture: true, + inputCommand: `(sleep 2; printf w; sleep 1; printf q)`, + }); + + expect(output).toContain("very long wrapped line for tty smo"); + expect(output).toContain("ke coverage';"); + }); + ttyTest("stdin patch mode auto-enters pager mode and can quit from terminal input", async () => { if (!ttyToolsAvailable) { return; diff --git a/test/ui-components.test.tsx b/test/ui-components.test.tsx index 9331e33..588f93b 100644 --- a/test/ui-components.test.tsx +++ b/test/ui-components.test.tsx @@ -182,6 +182,7 @@ function createDiffPaneProps( showLineNumbers: true, showHunkHeaders: true, wrapLines: false, + wrapToggleScrollTop: null, theme, width: 76, onDismissAgentNote: () => {}, @@ -1041,6 +1042,46 @@ describe("UI components", () => { expect(addedLines.slice(1).some((line) => line.includes("age';"))).toBe(true); }); + 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); + const width = 64; + + const splitFrame = await captureFrame( + , + width + 4, + 18, + ); + const stackFrame = await captureFrame( + , + width + 4, + 18, + ); + + const splitContinuationRows = splitFrame.split("\n").filter((line) => /^▌\s+▌\s+\S/.test(line)); + const stackContinuationRows = stackFrame.split("\n").filter((line) => /^▌\s{6,}\S/.test(line)); + + expect(splitFrame).toContain("1 + export const message = 't"); + expect(stackFrame).toContain("1 + export const message = 'this is a very long wrapped line"); + expect(splitContinuationRows.length).toBeGreaterThan(stackContinuationRows.length); + }); + test("PierreDiffView anchors range-less notes to the first visible row when hunk headers are hidden", async () => { const file = createDiffFile( "note-fallback",