From 0e7cefbb72ba7c4a5eac3e6b42e767ad8b90d60f Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 19 Oct 2026 21:13:46 -0700 Subject: [PATCH] fix(diff): keep combined diff position stable across worktree switches (#30950) A hidden worktree is display:none, so ResizeObserver reported every mounted diff row as 0px and the virtualizer cached that. Rows above the viewport collapsed, and the unchanged scrollTop showed files two rows further down after each switch back. Keep the last real row size while the scroller has no layout box, and skip Monaco height reports from a hidden (0-width) editor. --- .../src/components/editor/DiffSectionItem.tsx | 5 + .../use-combined-diff-virtualizer.ts | 3 + ...virtualized-hidden-row-measurement.test.ts | 137 ++++++++++++++++++ .../virtualized-hidden-row-measurement.ts | 31 ++++ 4 files changed, 176 insertions(+) create mode 100644 src/renderer/src/hooks/virtualized-hidden-row-measurement.test.ts create mode 100644 src/renderer/src/hooks/virtualized-hidden-row-measurement.ts diff --git a/src/renderer/src/components/editor/DiffSectionItem.tsx b/src/renderer/src/components/editor/DiffSectionItem.tsx index 45ddd300f22..179673e5978 100644 --- a/src/renderer/src/components/editor/DiffSectionItem.tsx +++ b/src/renderer/src/components/editor/DiffSectionItem.tsx @@ -18,6 +18,7 @@ import { useDiffSectionFallbackCleanup } from './useDiffSectionFallbackCleanup' import { submitDiffSectionComment } from './diff-section-comment-submit' import type { DiffSectionItemProps } from './diff-section-item-props' import { useDiffSectionModelLifecycle } from './use-diff-section-model-lifecycle' +import { hasLayoutBox } from '@/hooks/virtualized-hidden-row-measurement' export function DiffSectionItem({ section, @@ -177,6 +178,10 @@ export function DiffSectionItem({ let diffLayoutReady = false let pendingHeightFrame: number | null = null const updateHeight = (): void => { + // Why: a hidden (0-width) side-by-side editor falls back to inline and reports that taller height. + if (!hasLayoutBox(editor.getContainerDomNode())) { + return + } const contentHeight = editor.getModifiedEditor().getContentHeight() setSectionHeights((prev) => { if (prev[index] === contentHeight) { diff --git a/src/renderer/src/components/editor/combined-diff/scroll-viewport/use-combined-diff-virtualizer.ts b/src/renderer/src/components/editor/combined-diff/scroll-viewport/use-combined-diff-virtualizer.ts index 4cca1137aff..925bc272214 100644 --- a/src/renderer/src/components/editor/combined-diff/scroll-viewport/use-combined-diff-virtualizer.ts +++ b/src/renderer/src/components/editor/combined-diff/scroll-viewport/use-combined-diff-virtualizer.ts @@ -2,6 +2,7 @@ import { useLayoutEffect } from 'react' import type React from 'react' import { elementScroll, useVirtualizer, type Virtualizer } from '@tanstack/react-virtual' import type { ProgrammaticScrollMarks } from '@/hooks/programmatic-scroll-marks' +import { measureVirtualRowRetainingHiddenSize } from '@/hooks/virtualized-hidden-row-measurement' import type { DiffSection } from '../../diff-section-types' import { getDiffSectionRowEstimatedHeight } from '../../diff-section-layout' import { getCombinedDiffRenderRange } from './combined-diff-render-range' @@ -37,6 +38,8 @@ export function useCombinedDiffVirtualizer({ count: sections.length, getScrollElement: () => scrollContainerRef.current, estimateSize, + // Why: a hidden worktree reports every mounted row as 0px; keep the visible sizes so the anchor survives re-show. + measureElement: measureVirtualRowRetainingHiddenSize, // Offscreen Monaco sections render their full height; bound that work by pixels, too. rangeExtractor: (range) => getCombinedDiffRenderRange( diff --git a/src/renderer/src/hooks/virtualized-hidden-row-measurement.test.ts b/src/renderer/src/hooks/virtualized-hidden-row-measurement.test.ts new file mode 100644 index 00000000000..003a3fa9252 --- /dev/null +++ b/src/renderer/src/hooks/virtualized-hidden-row-measurement.test.ts @@ -0,0 +1,137 @@ +// @vitest-environment happy-dom +import { + measureElement, + observeElementOffset, + observeElementRect, + Virtualizer +} from '@tanstack/react-virtual' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { measureVirtualRowRetainingHiddenSize } from './virtualized-hidden-row-measurement' + +const ROW_COUNT = 20 +const MEASURED_ROW_HEIGHT = 1162 +const UNMEASURED_ESTIMATE = 400 +const SCROLL_TOP = 5250 + +type RowMeasurer = ( + element: Element, + entry: ResizeObserverEntry | undefined, + instance: Virtualizer +) => number + +function sizeEntry(target: Element, size: number): ResizeObserverEntry { + const box = [{ blockSize: size, inlineSize: size }] + return { + target, + borderBoxSize: box, + contentBoxSize: box, + devicePixelContentBoxSize: box, + contentRect: new DOMRect(0, 0, size, size) + } +} + +function setHidden(element: HTMLElement): void { + // Why: happy-dom always reports one client rect; a display:none subtree reports none. + const empty = element.getClientRects() + Array.prototype.splice.call(empty, 0, empty.length) + vi.spyOn(element, 'getClientRects').mockReturnValue(empty) +} + +function createViewport(measure: RowMeasurer): { + rowElement: (index: number) => HTMLElement + scrollElement: HTMLDivElement + virtualizer: Virtualizer +} { + const scrollElement = document.createElement('div') + document.body.appendChild(scrollElement) + const virtualizer = new Virtualizer({ + count: ROW_COUNT, + estimateSize: () => UNMEASURED_ESTIMATE, + getScrollElement: () => scrollElement, + initialOffset: SCROLL_TOP, + measureElement: measure, + observeElementOffset, + observeElementRect, + // Why: the browser keeps a hidden scroller's scrollTop; adjustments must not move it here either. + scrollToFn: () => {} + }) + virtualizer._willUpdate() + const rowElement = (index: number): HTMLElement => { + const row = document.createElement('div') + row.setAttribute('data-index', String(index)) + scrollElement.appendChild(row) + return row + } + // The user scrolled through rows 0-6, so each has a real measured height. + for (let index = 0; index <= 6; index++) { + const row = rowElement(index) + virtualizer.resizeItem(index, measure(row, sizeEntry(row, MEASURED_ROW_HEIGHT), virtualizer)) + } + return { rowElement, scrollElement, virtualizer } +} + +/** Hides the scroller, delivers the 0px ResizeObserver reports for the mounted rows, and re-shows it. */ +function runHideCycle(measure: RowMeasurer): { topKey: unknown; totalSize: number } { + const { rowElement, scrollElement, virtualizer } = createViewport(measure) + setHidden(scrollElement) + for (const index of [4, 5]) { + const row = rowElement(index) + virtualizer.resizeItem(index, measure(row, sizeEntry(row, 0), virtualizer)) + } + vi.restoreAllMocks() + return { + topKey: virtualizer.getVirtualItemForOffset(SCROLL_TOP)?.key, + totalSize: virtualizer.getTotalSize() + } +} + +afterEach(() => { + vi.restoreAllMocks() + document.body.replaceChildren() +}) + +describe('measureVirtualRowRetainingHiddenSize', () => { + const visibleState = (): { topKey: unknown; totalSize: number } => { + const { virtualizer } = createViewport(measureVirtualRowRetainingHiddenSize) + return { + topKey: virtualizer.getVirtualItemForOffset(SCROLL_TOP)?.key, + totalSize: virtualizer.getTotalSize() + } + } + + it('reproduces the drift with the default TanStack measurement', () => { + const before = visibleState() + const after = runHideCycle(measureElement) + + expect(before.topKey).toBe(4) + // Rows above the fold collapsed to 0px, so the same scrollTop now lands two files later. + expect(after.topKey).toBe(6) + expect(after.totalSize).toBe(before.totalSize - 2 * MEASURED_ROW_HEIGHT) + }) + + it('keeps the same row under the same scrollTop across a hide/re-show', () => { + const before = visibleState() + const after = runHideCycle(measureVirtualRowRetainingHiddenSize) + + expect(after).toEqual(before) + }) + + it('measures normally while the scroller is laid out', () => { + const { rowElement, virtualizer } = createViewport(measureVirtualRowRetainingHiddenSize) + const row = rowElement(4) + + expect(measureVirtualRowRetainingHiddenSize(row, sizeEntry(row, 560), virtualizer)).toBe(560) + }) + + it('falls back to the estimate for a row first seen while hidden', () => { + const { rowElement, scrollElement, virtualizer } = createViewport( + measureVirtualRowRetainingHiddenSize + ) + setHidden(scrollElement) + const row = rowElement(12) + + expect(measureVirtualRowRetainingHiddenSize(row, sizeEntry(row, 0), virtualizer)).toBe( + UNMEASURED_ESTIMATE + ) + }) +}) diff --git a/src/renderer/src/hooks/virtualized-hidden-row-measurement.ts b/src/renderer/src/hooks/virtualized-hidden-row-measurement.ts new file mode 100644 index 00000000000..0f5ae72860c --- /dev/null +++ b/src/renderer/src/hooks/virtualized-hidden-row-measurement.ts @@ -0,0 +1,31 @@ +import { measureElement, type Virtualizer } from '@tanstack/react-virtual' + +/** False under a `display: none` ancestor, where every size reads as 0. */ +export function hasLayoutBox(element: Element): boolean { + return element.getClientRects().length > 0 +} + +/** + * TanStack `measureElement` that keeps each row's last size while the scroll + * element has no layout box (a `display: none` ancestor, e.g. a hidden worktree). + * + * Hidden rows report 0px through ResizeObserver. Caching that collapses rows + * above the viewport, so the unchanged scrollTop shows later content on re-show. + * Unlike `useCachedMeasurements`, this reads the DOM, so it needs no visibility + * prop and covers every hiding ancestor. + */ +export function measureVirtualRowRetainingHiddenSize( + element: Element, + entry: ResizeObserverEntry | undefined, + instance: Virtualizer +): number { + const scrollElement = instance.scrollElement + if (scrollElement && !hasLayoutBox(scrollElement)) { + const index = instance.indexFromElement(element) + return ( + instance.itemSizeCache.get(instance.options.getItemKey(index)) ?? + instance.options.estimateSize(index) + ) + } + return measureElement(element, entry, instance) +}