From 89a6db63803232cbabad39c67a19322ee696ab11 Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Tue, 31 Mar 2026 10:50:38 -0400 Subject: [PATCH] refactor(ui): clarify layout, geometry, and bounds naming --- AGENTS.md | 7 + benchmarks/large-stream-profile.ts | 8 +- src/ui/components/panes/DiffPane.tsx | 159 +++++++++--------- src/ui/diff/plannedReviewRows.ts | 13 +- ...ctionHeights.ts => diffSectionGeometry.ts} | 73 ++++---- src/ui/lib/fileSectionLayout.ts | 31 ++-- ....test.ts => diff-section-geometry.test.ts} | 42 ++--- test/ui-components.test.tsx | 4 +- test/ui-lib.test.ts | 31 ++-- 9 files changed, 186 insertions(+), 182 deletions(-) rename src/ui/lib/{sectionHeights.ts => diffSectionGeometry.ts} (73%) rename test/{section-heights.test.ts => diff-section-geometry.test.ts} (63%) diff --git a/AGENTS.md b/AGENTS.md index 1558723..26836db 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -57,6 +57,13 @@ CLI input - 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. +## naming + +- Prefer names that match the role the code plays in the product and architecture. +- Use `layout` for structural placement or arrangement data. +- Use `geometry` for aggregate spatial data used by rendering, scrolling, or interaction. +- Use `bounds` for one concrete visible extent within a larger structure. + ## review behavior - Default behavior is a multi-file review stream in sidebar order. diff --git a/benchmarks/large-stream-profile.ts b/benchmarks/large-stream-profile.ts index d383856..37ac65b 100644 --- a/benchmarks/large-stream-profile.ts +++ b/benchmarks/large-stream-profile.ts @@ -3,7 +3,7 @@ import { performance } from "perf_hooks"; import { buildSplitRows } from "../src/ui/diff/pierre"; import { buildReviewRenderPlan } from "../src/ui/diff/reviewRenderPlan"; -import { measureDiffSectionMetrics } from "../src/ui/lib/sectionHeights"; +import { measureDiffSectionGeometry } from "../src/ui/lib/diffSectionGeometry"; import { resolveTheme } from "../src/ui/themes"; import { createLargeSplitStreamFiles, @@ -30,9 +30,9 @@ function measureMs(run: () => void) { return performance.now() - start; } -const sectionMetricsMs = measureMs(() => { +const sectionGeometryMs = measureMs(() => { windowedFiles.forEach((file) => { - measureDiffSectionMetrics(file, "split", true, theme); + measureDiffSectionGeometry(file, "split", true, theme); }); }); @@ -56,7 +56,7 @@ const noteReviewPlanMs = measureMs(() => { }); }); -console.log(`METRIC section_metrics_ms=${sectionMetricsMs.toFixed(2)}`); +console.log(`METRIC section_geometry_ms=${sectionGeometryMs.toFixed(2)}`); console.log(`METRIC split_rows_ms=${splitRowsMs.toFixed(2)}`); console.log(`METRIC note_review_plan_ms=${noteReviewPlanMs.toFixed(2)}`); console.log(`METRIC split_rows=${windowedRows}`); diff --git a/src/ui/components/panes/DiffPane.tsx b/src/ui/components/panes/DiffPane.tsx index 70ef4dc..8df246f 100644 --- a/src/ui/components/panes/DiffPane.tsx +++ b/src/ui/components/panes/DiffPane.tsx @@ -12,14 +12,14 @@ import type { DiffFile, LayoutMode } from "../../../core/types"; import type { VisibleAgentNote } from "../../lib/agentAnnotations"; import { computeHunkRevealScrollTop } from "../../lib/hunkScroll"; import { - measureDiffSectionMetrics, - type DiffSectionMetrics, - type DiffSectionRowMetric, -} from "../../lib/sectionHeights"; + measureDiffSectionGeometry, + type DiffSectionGeometry, + type DiffSectionRowBounds, +} from "../../lib/diffSectionGeometry"; import { - buildFileSectionLayoutMetrics, + buildFileSectionLayouts, findHeaderOwningFileSection, - resolveFileSectionHeaderTop, + getFileSectionHeaderTop, } from "../../lib/fileSectionLayout"; import { diffHunkId, diffSectionId } from "../../lib/ids"; import type { AppTheme } from "../../themes"; @@ -37,21 +37,21 @@ interface ViewportRowAnchor { rowOffsetWithin: number; } -/** Find the rendered row metric covering a vertical offset within one file body. */ -function binarySearchRowMetric(rowMetrics: DiffSectionRowMetric[], relativeTop: number) { +/** Find the rendered row bounds covering a vertical offset within one file body. */ +function binarySearchRowBounds(sectionRowBounds: DiffSectionRowBounds[], relativeTop: number) { let low = 0; - let high = rowMetrics.length - 1; + let high = sectionRowBounds.length - 1; while (low <= high) { const mid = (low + high) >>> 1; - const rowMetric = rowMetrics[mid]!; + const rowBounds = sectionRowBounds[mid]!; - if (relativeTop < rowMetric.offset) { + if (relativeTop < rowBounds.top) { high = mid - 1; - } else if (relativeTop >= rowMetric.offset + rowMetric.height) { + } else if (relativeTop >= rowBounds.top + rowBounds.height) { low = mid + 1; } else { - return rowMetric; + return rowBounds; } } @@ -61,28 +61,28 @@ function binarySearchRowMetric(rowMetrics: DiffSectionRowMetric[], relativeTop: /** Capture a stable top-row anchor from the pre-toggle layout so it can be restored later. */ function findViewportRowAnchor( files: DiffFile[], - sectionMetrics: DiffSectionMetrics[], + sectionGeometry: DiffSectionGeometry[], scrollTop: number, ) { - const fileSectionLayoutMetrics = buildFileSectionLayoutMetrics( + const fileSectionLayouts = buildFileSectionLayouts( files, - sectionMetrics.map((metrics) => metrics?.bodyHeight ?? 0), + sectionGeometry.map((metrics) => metrics?.bodyHeight ?? 0), ); for (let index = 0; index < files.length; index += 1) { - const layoutMetric = fileSectionLayoutMetrics[index]; - const bodyTop = layoutMetric?.bodyTop ?? 0; - const metrics = sectionMetrics[index]; - const bodyHeight = metrics?.bodyHeight ?? 0; + const sectionLayout = fileSectionLayouts[index]; + const bodyTop = sectionLayout?.bodyTop ?? 0; + const geometry = sectionGeometry[index]; + const bodyHeight = geometry?.bodyHeight ?? 0; const relativeTop = scrollTop - bodyTop; - if (relativeTop >= 0 && relativeTop < bodyHeight && metrics) { - const rowMetric = binarySearchRowMetric(metrics.rowMetrics, relativeTop); - if (rowMetric) { + if (relativeTop >= 0 && relativeTop < bodyHeight && geometry) { + const rowBounds = binarySearchRowBounds(geometry.rowBounds, relativeTop); + if (rowBounds) { return { fileId: files[index]!.id, - rowKey: rowMetric.key, - rowOffsetWithin: relativeTop - rowMetric.offset, + rowKey: rowBounds.key, + rowOffsetWithin: relativeTop - rowBounds.top, } satisfies ViewportRowAnchor; } } @@ -91,26 +91,26 @@ function findViewportRowAnchor( return null; } -/** Resolve a captured row anchor into its new scrollTop after wrapping/layout metrics change. */ +/** Resolve a captured row anchor into its new scrollTop after wrapping or layout changes. */ function resolveViewportRowAnchorTop( files: DiffFile[], - sectionMetrics: DiffSectionMetrics[], + sectionGeometry: DiffSectionGeometry[], anchor: ViewportRowAnchor, ) { - const fileSectionLayoutMetrics = buildFileSectionLayoutMetrics( + const fileSectionLayouts = buildFileSectionLayouts( files, - sectionMetrics.map((metrics) => metrics?.bodyHeight ?? 0), + sectionGeometry.map((metrics) => metrics?.bodyHeight ?? 0), ); for (let index = 0; index < files.length; index += 1) { - const layoutMetric = fileSectionLayoutMetrics[index]; - const bodyTop = layoutMetric?.bodyTop ?? 0; + const sectionLayout = fileSectionLayouts[index]; + const bodyTop = sectionLayout?.bodyTop ?? 0; 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); + const geometry = sectionGeometry[index]; + if (file?.id === anchor.fileId && geometry) { + const rowBounds = geometry.rowBoundsByKey.get(anchor.rowKey); + if (rowBounds) { + return bodyTop + rowBounds.top + Math.min(anchor.rowOffsetWithin, rowBounds.height - 1); } return bodyTop; } @@ -237,7 +237,7 @@ export function DiffPane({ const [scrollViewport, setScrollViewport] = useState({ top: 0, height: 0 }); const scrollbarRef = useRef(null); const prevScrollTopRef = useRef(0); - const previousSectionMetricsRef = useRef(null); + const previousSectionGeometryRef = useRef(null); const previousFilesRef = useRef(files); const previousWrapLinesRef = useRef(wrapLines); const previousSelectedFileTopAlignRequestIdRef = useRef(selectedFileTopAlignRequestId); @@ -267,10 +267,10 @@ export function DiffPane({ return () => clearInterval(interval); }, [scrollRef]); - const baseSectionMetrics = useMemo( + const baseSectionGeometry = useMemo( () => files.map((file) => - measureDiffSectionMetrics( + measureDiffSectionGeometry( file, layout, showHunkHeaders, @@ -284,11 +284,11 @@ export function DiffPane({ [diffContentWidth, files, layout, showHunkHeaders, showLineNumbers, theme, wrapLines], ); const baseEstimatedBodyHeights = useMemo( - () => baseSectionMetrics.map((metrics) => metrics.bodyHeight), - [baseSectionMetrics], + () => baseSectionGeometry.map((metrics) => metrics.bodyHeight), + [baseSectionGeometry], ); - const baseFileSectionLayoutMetrics = useMemo( - () => buildFileSectionLayoutMetrics(files, baseEstimatedBodyHeights), + const baseFileSectionLayouts = useMemo( + () => buildFileSectionLayouts(files, baseEstimatedBodyHeights), [baseEstimatedBodyHeights, files], ); @@ -297,11 +297,11 @@ export function DiffPane({ const minVisibleY = Math.max(0, scrollViewport.top - overscanRows); const maxVisibleY = scrollViewport.top + scrollViewport.height + overscanRows; return new Set( - baseFileSectionLayoutMetrics + baseFileSectionLayouts .filter((metric) => metric.sectionBottom >= minVisibleY && metric.sectionTop <= maxVisibleY) .map((metric) => metric.fileId), ); - }, [baseFileSectionLayoutMetrics, scrollViewport.height, scrollViewport.top]); + }, [baseFileSectionLayouts, scrollViewport.height, scrollViewport.top]); const visibleAgentNotesByFile = useMemo(() => { const next = new Map(); @@ -327,15 +327,15 @@ export function DiffPane({ return next; }, [allAgentNotesByFile, selectedFileId, showAgentNotes, visibleViewportFileIds]); - const sectionMetrics = useMemo( + const sectionGeometry = useMemo( () => files.map((file, index) => { const visibleNotes = visibleAgentNotesByFile.get(file.id) ?? EMPTY_VISIBLE_AGENT_NOTES; if (visibleNotes.length === 0) { - return baseSectionMetrics[index]!; + return baseSectionGeometry[index]!; } - return measureDiffSectionMetrics( + return measureDiffSectionGeometry( file, layout, showHunkHeaders, @@ -347,7 +347,7 @@ export function DiffPane({ ); }), [ - baseSectionMetrics, + baseSectionGeometry, diffContentWidth, files, layout, @@ -359,32 +359,31 @@ export function DiffPane({ ], ); const estimatedBodyHeights = useMemo( - () => sectionMetrics.map((metrics) => metrics.bodyHeight), - [sectionMetrics], + () => sectionGeometry.map((metrics) => metrics.bodyHeight), + [sectionGeometry], ); - const fileSectionLayoutMetrics = useMemo( - () => buildFileSectionLayoutMetrics(files, estimatedBodyHeights), + const fileSectionLayouts = useMemo( + () => buildFileSectionLayouts(files, estimatedBodyHeights), [estimatedBodyHeights, files], ); // Read the live scroll box position during render so sticky-header ownership flips // immediately after imperative scrolls instead of waiting for the polled viewport snapshot. const effectiveScrollTop = scrollRef.current?.scrollTop ?? scrollViewport.top; - const totalContentHeight = - fileSectionLayoutMetrics[fileSectionLayoutMetrics.length - 1]?.sectionBottom ?? 0; + const totalContentHeight = fileSectionLayouts[fileSectionLayouts.length - 1]?.sectionBottom ?? 0; const stickyHeaderFile = useMemo(() => { if (files.length < 2) { return null; } - const owner = findHeaderOwningFileSection(fileSectionLayoutMetrics, effectiveScrollTop); + const owner = findHeaderOwningFileSection(fileSectionLayouts, effectiveScrollTop); if (!owner || effectiveScrollTop <= owner.headerTop) { return null; } return files[owner.sectionIndex] ?? null; - }, [effectiveScrollTop, fileSectionLayoutMetrics, files]); + }, [effectiveScrollTop, fileSectionLayouts, files]); const visibleWindowedFileIds = useMemo(() => { if (!windowingEnabled) { @@ -418,7 +417,7 @@ export function DiffPane({ return null; } - const selectedFileSectionLayout = fileSectionLayoutMetrics[selectedFileIndex]; + const selectedFileSectionLayout = fileSectionLayouts[selectedFileIndex]; if (!selectedFileSectionLayout) { return null; } @@ -427,7 +426,7 @@ export function DiffPane({ 0, Math.min(selectedHunkIndex, selectedFile.metadata.hunks.length - 1), ); - const hunkBounds = sectionMetrics[selectedFileIndex]?.hunkBounds.get(clampedHunkIndex); + const hunkBounds = sectionGeometry[selectedFileIndex]?.hunkBounds.get(clampedHunkIndex); if (!hunkBounds) { return null; } @@ -439,13 +438,7 @@ export function DiffPane({ endRowId: hunkBounds.endRowId, sectionTop: selectedFileSectionLayout.sectionTop, }; - }, [ - fileSectionLayoutMetrics, - sectionMetrics, - selectedFile, - selectedFileIndex, - selectedHunkIndex, - ]); + }, [fileSectionLayouts, sectionGeometry, selectedFile, selectedFileIndex, selectedHunkIndex]); /** Absolute scroll offset and height of the first inline note in the selected hunk, if any. */ const selectedNoteBounds = useMemo(() => { @@ -453,19 +446,19 @@ export function DiffPane({ return null; } - const metrics = sectionMetrics[selectedFileIndex]; - if (!metrics) { + const geometry = sectionGeometry[selectedFileIndex]; + if (!geometry) { return null; } const sectionRelativeHunkTop = selectedEstimatedHunkBounds.top - selectedEstimatedHunkBounds.sectionTop; const sectionRelativeHunkBottom = sectionRelativeHunkTop + selectedEstimatedHunkBounds.height; - const noteRow = metrics.rowMetrics.find( + const noteRow = geometry.rowBounds.find( (row) => row.key.startsWith("inline-note:") && - row.offset >= sectionRelativeHunkTop && - row.offset < sectionRelativeHunkBottom, + row.top >= sectionRelativeHunkTop && + row.top < sectionRelativeHunkBottom, ); if (!noteRow) { @@ -473,10 +466,10 @@ export function DiffPane({ } return { - top: selectedEstimatedHunkBounds.sectionTop + noteRow.offset, + top: selectedEstimatedHunkBounds.sectionTop + noteRow.top, height: noteRow.height, }; - }, [scrollToNote, sectionMetrics, selectedEstimatedHunkBounds, selectedFileIndex]); + }, [scrollToNote, sectionGeometry, selectedEstimatedHunkBounds, selectedFileIndex]); // Track the previous selected anchor to detect actual selection changes. const prevSelectedAnchorIdRef = useRef(null); @@ -489,7 +482,7 @@ export function DiffPane({ /** Scroll one file header to the top using the latest planned section geometry. */ const scrollFileHeaderToTop = useCallback( (fileId: string) => { - const headerTop = resolveFileSectionHeaderTop(fileSectionLayoutMetrics, fileId); + const headerTop = getFileSectionHeaderTop(fileSectionLayouts, fileId); if (headerTop == null) { return false; } @@ -502,12 +495,12 @@ export function DiffPane({ scrollBox.scrollTo(headerTop); return true; }, - [fileSectionLayoutMetrics, scrollRef], + [fileSectionLayouts, scrollRef], ); useLayoutEffect(() => { const wrapChanged = previousWrapLinesRef.current !== wrapLines; - const previousSectionMetrics = previousSectionMetricsRef.current; + const previousSectionMetrics = previousSectionGeometryRef.current; const previousFiles = previousFilesRef.current; if (wrapChanged && previousSectionMetrics && previousFiles.length > 0) { @@ -523,7 +516,7 @@ export function DiffPane({ previousScrollTop, ); if (anchor) { - const nextTop = resolveViewportRowAnchorTop(files, sectionMetrics, anchor); + const nextTop = resolveViewportRowAnchorTop(files, sectionGeometry, anchor); const restoreViewportAnchor = () => { scrollRef.current?.scrollTo(nextTop); }; @@ -537,7 +530,7 @@ export function DiffPane({ const timeouts = retryDelays.map((delay) => setTimeout(restoreViewportAnchor, delay)); previousWrapLinesRef.current = wrapLines; - previousSectionMetricsRef.current = sectionMetrics; + previousSectionGeometryRef.current = sectionGeometry; previousFilesRef.current = files; return () => { @@ -547,9 +540,9 @@ export function DiffPane({ } previousWrapLinesRef.current = wrapLines; - previousSectionMetricsRef.current = sectionMetrics; + previousSectionGeometryRef.current = sectionGeometry; previousFilesRef.current = files; - }, [files, scrollRef, scrollViewport.top, sectionMetrics, wrapLines, wrapToggleScrollTop]); + }, [files, scrollRef, scrollViewport.top, sectionGeometry, wrapLines, wrapToggleScrollTop]); useLayoutEffect(() => { if (previousSelectedFileTopAlignRequestIdRef.current === selectedFileTopAlignRequestId) { @@ -588,7 +581,7 @@ export function DiffPane({ return; } - const desiredTop = resolveFileSectionHeaderTop(fileSectionLayoutMetrics, pendingFileId); + const desiredTop = getFileSectionHeaderTop(fileSectionLayouts, pendingFileId); if (desiredTop == null) { return; } @@ -602,7 +595,7 @@ export function DiffPane({ scrollFileHeaderToTop(pendingFileId); }, [ clearPendingFileTopAlign, - fileSectionLayoutMetrics, + fileSectionLayouts, files, scrollFileHeaderToTop, scrollRef, @@ -622,7 +615,7 @@ export function DiffPane({ return; } - // Only auto-scroll when the selection actually changes, not when metrics update during + // Only auto-scroll when the selection actually changes, not when geometry updates during // scrolling or when the selected section refines its measured bounds. const isSelectionChange = prevSelectedAnchorIdRef.current !== selectedAnchorId; prevSelectedAnchorIdRef.current = selectedAnchorId; diff --git a/src/ui/diff/plannedReviewRows.ts b/src/ui/diff/plannedReviewRows.ts index b82b275..9bc09f7 100644 --- a/src/ui/diff/plannedReviewRows.ts +++ b/src/ui/diff/plannedReviewRows.ts @@ -22,6 +22,13 @@ export interface PlannedHunkBounds { endRowId: string; } +/** Aggregate geometry for one file section measured from planned review rows. */ +export interface PlannedSectionGeometry { + bodyHeight: number; + hunkAnchorRows: Map; + hunkBounds: Map; +} + /** Return whether this planned row should count toward a hunk's own visible extent. */ function rowContributesToHunkBounds(row: PlannedReviewRow) { // Collapsed gap rows belong between hunks, so they affect total section height but not a hunk's @@ -63,14 +70,14 @@ export function plannedReviewRowVisible( } /** - * Walk one file's planned rows and derive both section metrics and hunk-local bounds. + * Walk one file's planned rows and derive section geometry plus hunk-local bounds. * * `top` is measured in section-body rows, so callers can add the file section offset later. */ -export function measurePlannedHunkBounds( +export function measurePlannedSectionGeometry( plannedRows: PlannedReviewRow[], options: PlannedReviewRowLayoutOptions, -) { +): PlannedSectionGeometry { const hunkAnchorRows = new Map(); const hunkBounds = new Map(); let bodyHeight = 0; diff --git a/src/ui/lib/sectionHeights.ts b/src/ui/lib/diffSectionGeometry.ts similarity index 73% rename from src/ui/lib/sectionHeights.ts rename to src/ui/lib/diffSectionGeometry.ts index 3eb96aa..e46a4bd 100644 --- a/src/ui/lib/sectionHeights.ts +++ b/src/ui/lib/diffSectionGeometry.ts @@ -8,27 +8,27 @@ import { reviewRowId } from "./ids"; import type { VisibleAgentNote } from "./agentAnnotations"; import type { AppTheme } from "../themes"; -export interface DiffSectionRowMetric { +export interface DiffSectionRowBounds { key: string; - offset: number; + top: number; height: number; } -/** Cached placeholder sizing and hunk navigation metrics for one file section. */ -export interface DiffSectionMetrics { +/** Cached placeholder sizing and hunk navigation geometry for one file section. */ +export interface DiffSectionGeometry { bodyHeight: number; hunkAnchorRows: Map; hunkBounds: Map; - rowMetrics: DiffSectionRowMetric[]; - rowMetricsByKey: Map; + rowBounds: DiffSectionRowBounds[]; + rowBoundsByKey: Map; } -const NOTE_AWARE_SECTION_METRICS_CACHE = new WeakMap< +const NOTE_AWARE_SECTION_GEOMETRY_CACHE = new WeakMap< VisibleAgentNote[], - Map + Map >(); -/** Build the exact review rows for one file before converting them into height metrics. */ +/** Build the exact review rows for one file before converting them into section geometry. */ function buildBasePlannedRows( file: DiffFile, layout: Exclude, @@ -90,11 +90,8 @@ function rowContributesToHunkBounds(row: PlannedReviewRow) { return !(row.kind === "diff-row" && row.row.type === "collapsed"); } -/** - * Measure one file section from the same render plan used by PierreDiffView. - * This keeps placeholder sizing, viewport anchoring, and selected-hunk reveal math aligned. - */ -export function measureDiffSectionMetrics( +/** Measure one file section from the same render plan used by PierreDiffView. */ +export function measureDiffSectionGeometry( file: DiffFile, layout: Exclude, showHunkHeaders: boolean, @@ -103,14 +100,14 @@ export function measureDiffSectionMetrics( width = 0, showLineNumbers = true, wrapLines = false, -): DiffSectionMetrics { +): DiffSectionGeometry { if (file.metadata.hunks.length === 0) { return { bodyHeight: 1, hunkAnchorRows: new Map(), hunkBounds: new Map(), - rowMetrics: [], - rowMetricsByKey: new Map(), + rowBounds: [], + rowBoundsByKey: new Map(), }; } @@ -118,7 +115,7 @@ export function measureDiffSectionMetrics( // 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 cachedByNotes = NOTE_AWARE_SECTION_GEOMETRY_CACHE.get(visibleAgentNotes); const cached = cachedByNotes?.get(cacheKey); if (cached) { return cached; @@ -128,8 +125,8 @@ export function measureDiffSectionMetrics( const plannedRows = buildBasePlannedRows(file, layout, showHunkHeaders, theme, visibleAgentNotes); const hunkAnchorRows = new Map(); const hunkBounds = new Map(); - const rowMetrics: DiffSectionRowMetric[] = []; - const rowMetricsByKey = new Map(); + const rowBounds: DiffSectionRowBounds[] = []; + const rowBoundsByKey = new Map(); const lineNumberDigits = String(findMaxLineNumber(file)).length; let bodyHeight = 0; @@ -148,15 +145,15 @@ export function measureDiffSectionMetrics( wrapLines, theme, ); - const rowMetric = { + const rowBoundsEntry = { key: row.key, - // Record both the starting offset and the measured height so callers can translate between + // Record both the starting top and the measured height so callers can translate between // scroll positions and stable review-row identities across wrap/layout changes. - offset: bodyHeight, + top: bodyHeight, height, }; - rowMetrics.push(rowMetric); - rowMetricsByKey.set(row.key, rowMetric); + rowBounds.push(rowBoundsEntry); + rowBoundsByKey.set(row.key, rowBoundsEntry); if (height > 0 && rowContributesToHunkBounds(row)) { const rowId = reviewRowId(row.key); @@ -178,35 +175,35 @@ export function measureDiffSectionMetrics( bodyHeight += height; } - const metrics = { + const geometry: DiffSectionGeometry = { bodyHeight, hunkAnchorRows, hunkBounds, - rowMetrics, - rowMetricsByKey, + rowBounds, + rowBoundsByKey, }; if (visibleAgentNotes.length > 0) { - const cachedByNotes = NOTE_AWARE_SECTION_METRICS_CACHE.get(visibleAgentNotes) ?? new Map(); - cachedByNotes.set(cacheKey, metrics); - NOTE_AWARE_SECTION_METRICS_CACHE.set(visibleAgentNotes, cachedByNotes); + const cachedByNotes = NOTE_AWARE_SECTION_GEOMETRY_CACHE.get(visibleAgentNotes) ?? new Map(); + cachedByNotes.set(cacheKey, geometry); + NOTE_AWARE_SECTION_GEOMETRY_CACHE.set(visibleAgentNotes, cachedByNotes); } - return metrics; + return geometry; } -/** Estimate the number of diff-body rows for one file in the windowed path. */ -export function estimateDiffBodyRows( +/** Estimate the number of diff-body rows for one file section in the windowed path. */ +export function estimateDiffSectionBodyRows( file: DiffFile, layout: Exclude, showHunkHeaders: boolean, theme: AppTheme, ) { - return measureDiffSectionMetrics(file, layout, showHunkHeaders, theme).bodyHeight; + return measureDiffSectionGeometry(file, layout, showHunkHeaders, theme).bodyHeight; } -/** Estimate the body-row offset for the anchor that should represent the selected hunk. */ -export function estimateHunkAnchorRow( +/** Estimate the body-row position for the anchor that should represent the selected hunk. */ +export function estimateHunkAnchorBodyRow( file: DiffFile, layout: Exclude, showHunkHeaders: boolean, @@ -219,7 +216,7 @@ export function estimateHunkAnchorRow( const clampedHunkIndex = Math.max(0, Math.min(hunkIndex, file.metadata.hunks.length - 1)); return ( - measureDiffSectionMetrics(file, layout, showHunkHeaders, theme).hunkAnchorRows.get( + measureDiffSectionGeometry(file, layout, showHunkHeaders, theme).hunkAnchorRows.get( clampedHunkIndex, ) ?? 0 ); diff --git a/src/ui/lib/fileSectionLayout.ts b/src/ui/lib/fileSectionLayout.ts index 60a732a..ed39f8a 100644 --- a/src/ui/lib/fileSectionLayout.ts +++ b/src/ui/lib/fileSectionLayout.ts @@ -1,7 +1,7 @@ import type { DiffFile } from "../../core/types"; /** Stream geometry for one file section in the main review pane. */ -export interface FileSectionLayoutMetric { +export interface FileSectionLayout { fileId: string; sectionIndex: number; sectionTop: number; @@ -12,8 +12,8 @@ export interface FileSectionLayoutMetric { } /** Build absolute section offsets from file order and measured body heights. */ -export function buildFileSectionLayoutMetrics(files: DiffFile[], bodyHeights: number[]) { - const metrics: FileSectionLayoutMetric[] = []; +export function buildFileSectionLayouts(files: DiffFile[], bodyHeights: number[]) { + const layouts: FileSectionLayout[] = []; let cursor = 0; files.forEach((file, index) => { @@ -24,7 +24,7 @@ export function buildFileSectionLayoutMetrics(files: DiffFile[], bodyHeights: nu const bodyTop = headerTop + 1; const sectionBottom = bodyTop + bodyHeight; - metrics.push({ + layouts.push({ fileId: file.id, sectionIndex: index, sectionTop, @@ -37,29 +37,29 @@ export function buildFileSectionLayoutMetrics(files: DiffFile[], bodyHeights: nu cursor = sectionBottom; }); - return metrics; + return layouts; } /** Return the file section that owns the viewport top, switching at each next header row. */ export function findHeaderOwningFileSection( - sectionMetrics: FileSectionLayoutMetric[], + fileSectionLayouts: FileSectionLayout[], scrollTop: number, ) { - if (sectionMetrics.length === 0) { + if (fileSectionLayouts.length === 0) { return null; } // Choose the last header whose top has reached the viewport, so separator rows still belong // to the previous section until the next header itself takes over. let low = 0; - let high = sectionMetrics.length - 1; + let high = fileSectionLayouts.length - 1; let winner = 0; while (low <= high) { const mid = (low + high) >>> 1; - const metric = sectionMetrics[mid]!; + const layout = fileSectionLayouts[mid]!; - if (metric.headerTop <= scrollTop) { + if (layout.headerTop <= scrollTop) { winner = mid; low = mid + 1; } else { @@ -67,15 +67,12 @@ export function findHeaderOwningFileSection( } } - return sectionMetrics[winner]!; + return fileSectionLayouts[winner]!; } -/** Resolve the scroll target needed to make one file header own the viewport top. */ -export function resolveFileSectionHeaderTop( - sectionMetrics: FileSectionLayoutMetric[], - fileId: string, -) { - const targetSection = sectionMetrics.find((metric) => metric.fileId === fileId); +/** Return the scroll top needed to make one file header own the viewport top. */ +export function getFileSectionHeaderTop(fileSectionLayouts: FileSectionLayout[], fileId: string) { + const targetSection = fileSectionLayouts.find((layout) => layout.fileId === fileId); if (!targetSection) { return null; } diff --git a/test/section-heights.test.ts b/test/diff-section-geometry.test.ts similarity index 63% rename from test/section-heights.test.ts rename to test/diff-section-geometry.test.ts index c87abfe..8f613bc 100644 --- a/test/section-heights.test.ts +++ b/test/diff-section-geometry.test.ts @@ -1,17 +1,17 @@ import { describe, expect, test } from "bun:test"; import type { VisibleAgentNote } from "../src/ui/lib/agentAnnotations"; -import { measureDiffSectionMetrics } from "../src/ui/lib/sectionHeights"; +import { measureDiffSectionGeometry } from "../src/ui/lib/diffSectionGeometry"; import { resolveTheme } from "../src/ui/themes"; import { createDiffFile, createHeaderOnlyDiffFile, lines } from "./fixtures/diff-helpers"; -describe("measureDiffSectionMetrics", () => { +describe("measureDiffSectionGeometry", () => { const theme = resolveTheme("midnight", null); test("measures split and stack layouts from the render plan", () => { const file = createDiffFile(); - const split = measureDiffSectionMetrics(file, "split", true, theme); - const stack = measureDiffSectionMetrics(file, "stack", true, theme); + const split = measureDiffSectionGeometry(file, "split", true, theme); + const stack = measureDiffSectionGeometry(file, "stack", true, theme); expect(split.bodyHeight).toBeGreaterThan(0); expect(stack.bodyHeight).toBeGreaterThan(split.bodyHeight); @@ -26,14 +26,14 @@ describe("measureDiffSectionMetrics", () => { id: "annotation:example:0", annotation: { newRange: [1, 1], - rationale: "Keep note height in section metrics.", + rationale: "Keep note height in section geometry.", summary: "Explain the change", }, }, ]; - const baseMetrics = measureDiffSectionMetrics(file, "split", true, theme, [], 120); - const noteMetrics = measureDiffSectionMetrics( + const baseGeometry = measureDiffSectionGeometry(file, "split", true, theme, [], 120); + const noteGeometry = measureDiffSectionGeometry( file, "split", true, @@ -42,12 +42,12 @@ describe("measureDiffSectionMetrics", () => { 120, ); - expect(noteMetrics.bodyHeight).toBeGreaterThan(baseMetrics.bodyHeight); - expect(noteMetrics.hunkAnchorRows.get(0)).toBe(baseMetrics.hunkAnchorRows.get(0)); - expect(noteMetrics.rowMetrics.some((row) => row.key.startsWith("inline-note:"))).toBe(true); + expect(noteGeometry.bodyHeight).toBeGreaterThan(baseGeometry.bodyHeight); + expect(noteGeometry.hunkAnchorRows.get(0)).toBe(baseGeometry.hunkAnchorRows.get(0)); + expect(noteGeometry.rowBounds.some((row) => row.key.startsWith("inline-note:"))).toBe(true); }); - test("wraps long rows into taller section metrics when wrapping is enabled", () => { + test("wraps long rows into taller section geometry when wrapping is enabled", () => { const file = createDiffFile({ before: lines("const alpha = 1;", "const beta = 2;"), after: lines( @@ -58,7 +58,7 @@ describe("measureDiffSectionMetrics", () => { path: "wrapped.ts", }); - const nowrapMetrics = measureDiffSectionMetrics( + const nowrapGeometry = measureDiffSectionGeometry( file, "stack", true, @@ -68,7 +68,7 @@ describe("measureDiffSectionMetrics", () => { true, false, ); - const wrappedMetrics = measureDiffSectionMetrics( + const wrappedGeometry = measureDiffSectionGeometry( file, "stack", true, @@ -79,9 +79,9 @@ describe("measureDiffSectionMetrics", () => { true, ); - expect(wrappedMetrics.bodyHeight).toBeGreaterThan(nowrapMetrics.bodyHeight); - expect(wrappedMetrics.hunkBounds.get(0)?.height).toBeGreaterThan( - nowrapMetrics.hunkBounds.get(0)?.height ?? 0, + expect(wrappedGeometry.bodyHeight).toBeGreaterThan(nowrapGeometry.bodyHeight); + expect(wrappedGeometry.hunkBounds.get(0)?.height).toBeGreaterThan( + nowrapGeometry.hunkBounds.get(0)?.height ?? 0, ); }); @@ -93,25 +93,25 @@ describe("measureDiffSectionMetrics", () => { path: "empty.ts", }); - const metrics = measureDiffSectionMetrics(file, "split", true, theme); + const metrics = measureDiffSectionGeometry(file, "split", true, theme); expect(file.metadata.hunks).toHaveLength(0); expect(metrics.bodyHeight).toBe(1); expect(metrics.hunkBounds.size).toBe(0); - expect(metrics.rowMetrics).toEqual([]); + expect(metrics.rowBounds).toEqual([]); }); test("can measure a header-only hunk stream without line rows", () => { const file = createHeaderOnlyDiffFile(); - const metrics = measureDiffSectionMetrics(file, "split", true, theme); + const metrics = measureDiffSectionGeometry(file, "split", true, theme); expect(file.metadata.hunks).toHaveLength(1); expect(metrics.bodyHeight).toBe(1); expect(metrics.hunkAnchorRows.size).toBe(1); expect(metrics.hunkAnchorRows.get(0)).toBe(0); expect(metrics.hunkBounds.get(0)).toMatchObject({ height: 1, top: 0 }); - expect(metrics.rowMetrics).toHaveLength(1); - expect(metrics.rowMetrics[0]?.key).toContain(":header:"); + expect(metrics.rowBounds).toHaveLength(1); + expect(metrics.rowBounds[0]?.key).toContain(":header:"); }); }); diff --git a/test/ui-components.test.tsx b/test/ui-components.test.tsx index ae2aae5..d6eb1cb 100644 --- a/test/ui-components.test.tsx +++ b/test/ui-components.test.tsx @@ -5,7 +5,7 @@ import { parseDiffFromFile } from "@pierre/diffs"; import { act, createRef, type ReactNode } from "react"; import type { AppBootstrap, DiffFile } from "../src/core/types"; import { resolveTheme } from "../src/ui/themes"; -import { measureDiffSectionMetrics } from "../src/ui/lib/sectionHeights"; +import { measureDiffSectionGeometry } from "../src/ui/lib/diffSectionGeometry"; const { AppHost } = await import("../src/ui/AppHost"); const { buildSidebarEntries } = await import("../src/ui/lib/files"); @@ -524,7 +524,7 @@ describe("UI components", () => { height: 10, }); - const firstBodyHeight = measureDiffSectionMetrics( + const firstBodyHeight = measureDiffSectionGeometry( firstFile, "split", true, diff --git a/test/ui-lib.test.ts b/test/ui-lib.test.ts index dc57e0b..b2adc68 100644 --- a/test/ui-lib.test.ts +++ b/test/ui-lib.test.ts @@ -26,7 +26,10 @@ import { } from "../src/ui/lib/keyboard"; import { fitText, padText } from "../src/ui/lib/text"; import { computeHunkRevealScrollTop } from "../src/ui/lib/hunkScroll"; -import { estimateDiffBodyRows, measureDiffSectionMetrics } from "../src/ui/lib/sectionHeights"; +import { + estimateDiffSectionBodyRows, + measureDiffSectionGeometry, +} from "../src/ui/lib/diffSectionGeometry"; import { resizeSidebarWidth } from "../src/ui/lib/sidebar"; import { resolveTheme } from "../src/ui/themes"; @@ -251,20 +254,20 @@ describe("ui helpers", () => { expect(resizeSidebarWidth(34, 33, 120, 22, 80)).toBe(80); }); - test("estimateDiffBodyRows matches split and stack row counts from the render plan", async () => { + test("estimateDiffSectionBodyRows matches split and stack row counts from the render plan", async () => { const file = createDiffFile(); const theme = resolveTheme("midnight", null); - expect(estimateDiffBodyRows(file, "split", true, theme)).toBeGreaterThan(0); - expect(estimateDiffBodyRows(file, "stack", true, theme)).toBeGreaterThan( - estimateDiffBodyRows(file, "split", true, theme), + expect(estimateDiffSectionBodyRows(file, "split", true, theme)).toBeGreaterThan(0); + expect(estimateDiffSectionBodyRows(file, "stack", true, theme)).toBeGreaterThan( + estimateDiffSectionBodyRows(file, "split", true, theme), ); - expect(estimateDiffBodyRows(file, "split", false, theme)).toBe( - estimateDiffBodyRows(file, "split", true, theme) - file.metadata.hunks.length, + expect(estimateDiffSectionBodyRows(file, "split", false, theme)).toBe( + estimateDiffSectionBodyRows(file, "split", true, theme) - file.metadata.hunks.length, ); }); - test("measureDiffSectionMetrics tracks hidden-header anchor rows across multiple hunks", () => { + test("measureDiffSectionGeometry tracks hidden-header anchor rows across multiple hunks", () => { const file = createDiffFile( lines( "const line1 = 1;", @@ -296,7 +299,7 @@ describe("ui helpers", () => { ), ); const theme = resolveTheme("midnight", null); - const metrics = measureDiffSectionMetrics(file, "split", false, theme); + const metrics = measureDiffSectionGeometry(file, "split", false, theme); expect(metrics.bodyHeight).toBeGreaterThan(0); expect(metrics.hunkAnchorRows.get(0)).toBe(1); @@ -308,11 +311,11 @@ describe("ui helpers", () => { expect(metrics.hunkBounds.get(1)?.height).toBe(1); }); - test("measureDiffSectionMetrics includes visible inline note rows in split mode", () => { + test("measureDiffSectionGeometry includes visible inline note rows in split mode", () => { const file = createDiffFile(); const theme = resolveTheme("midnight", null); - const baseMetrics = measureDiffSectionMetrics(file, "split", true, theme); - const noteMetrics = measureDiffSectionMetrics( + const baseGeometry = measureDiffSectionGeometry(file, "split", true, theme); + const noteGeometry = measureDiffSectionGeometry( file, "split", true, @@ -330,8 +333,8 @@ describe("ui helpers", () => { 120, ); - expect(noteMetrics.bodyHeight).toBeGreaterThan(baseMetrics.bodyHeight); - expect(noteMetrics.hunkAnchorRows.get(0)).toBe(baseMetrics.hunkAnchorRows.get(0)); + expect(noteGeometry.bodyHeight).toBeGreaterThan(baseGeometry.bodyHeight); + expect(noteGeometry.hunkAnchorRows.get(0)).toBe(baseGeometry.hunkAnchorRows.get(0)); }); test("computeHunkRevealScrollTop keeps a hunk fully visible when it fits", () => {