diff --git a/src/renderer/src/components/sidebar/worktree-list/viewport/use-row-measurement.test.tsx b/src/renderer/src/components/sidebar/worktree-list/viewport/use-row-measurement.test.tsx new file mode 100644 index 00000000000..a46610c64ef --- /dev/null +++ b/src/renderer/src/components/sidebar/worktree-list/viewport/use-row-measurement.test.tsx @@ -0,0 +1,123 @@ +// @vitest-environment happy-dom + +import { cleanup, renderHook } from '@testing-library/react' +import { afterEach, describe, expect, it } from 'vitest' +import type { VirtualItem } from '@tanstack/react-virtual' +import { useVirtualRowMeasurementSync } from './use-row-measurement' +import { getStickyHeaderIndexes } from './virtual-rows' +import type { RenderRow } from '../listing/render-row' +import type { WorktreeListVirtualizer } from './use-virtualizer' +import type { VirtualizedScrollAnchor } from '@/hooks/useVirtualizedScrollAnchor' + +/** + * STA-6697 at the CALL SITE. + * + * virtual-rows.test.ts pins `resolveStickyScrollOffset` itself, but every one of those tests + * builds the offset by calling the helper directly and hands the result to the resolver. None of + * them runs this hook, which is where the fix actually lives — so reverting this file to + * `virtualizer.scrollOffset ?? scrollOffsetRef.current` left the whole sidebar suite green while + * shipping the bug. This is the test that fails on that revert. + */ + +// Minimal but real: buildLineageRowRekeyMap reads `sectionKey` and the worktree's lineage fields. +function itemRow(id: string, sectionKey: string): RenderRow { + return { + type: 'item', + key: id, + sectionKey, + worktree: { id, path: `/tmp/${id}`, branch: id } + } as unknown as RenderRow +} + +// Two projects, collapsed, in a list too short to scroll: header, row, header, row. +const rows: RenderRow[] = [ + { type: 'header', key: 'project-a', label: 'project-a', count: 1, tone: 'text-foreground' }, + itemRow('wt-a', 'project-a'), + { type: 'header', key: 'project-b', label: 'project-b', count: 1, tone: 'text-foreground' }, + itemRow('wt-b', 'project-b') +] +const virtualItems = [ + { index: 0, start: 0 }, + { index: 1, start: 36 }, + { index: 2, start: 70 }, + { index: 3, start: 106 } +] as VirtualItem[] + +/** The believed offset a post-mount size correction books while the element stays at 0. */ +const DRIFTED_OFFSET = 70 + +function createScrollElement(scrollTop: number): HTMLDivElement { + const element = document.createElement('div') + document.body.append(element) + // Getter plus a no-op setter: this models the STA-6697 case faithfully. The list is too short + // to scroll, so the browser clamps any write and the element never moves — which is exactly why + // the virtualizer's remembered offset can drift away from it and never resync. + Object.defineProperty(element, 'scrollTop', { + configurable: true, + get: () => scrollTop, + set: () => undefined + }) + return element +} + +function renderMeasurementSync(args: { scrollTop: number; virtualizerOffset: number | null }) { + const activeStickyHeaderIndexRef = { current: null as number | null } + const virtualization = { + virtualizer: { + scrollOffset: args.virtualizerOffset, + elementsCache: new Map(), + getTotalSize: () => 142, + getVirtualItems: () => virtualItems, + measureElement: () => undefined + }, + isCurrentVirtualRowElement: () => true, + stickyHeaderIndexes: getStickyHeaderIndexes(rows), + activeStickyHeaderIndexRef, + activeStickyHostIndexRef: { current: null as number | null }, + // Drift also advanced the virtualizer's range, so the candidate handed in is already project B. + stickyRangeStartIndexRef: { current: 2 } + } as unknown as WorktreeListVirtualizer + + const scrollRef = { current: createScrollElement(args.scrollTop) } + const view = renderHook(() => + useVirtualRowMeasurementSync({ + renderRows: rows, + virtualization, + scrollRef, + scrollOffsetRef: { current: DRIFTED_OFFSET }, + scrollAnchorRef: { current: {} as VirtualizedScrollAnchor }, + hasDirectScrollInput: () => false, + shouldSkipScrollAnchorRestore: () => true + }) + ) + return { activeStickyHeaderIndexRef, view } +} + +afterEach(() => { + cleanup() + document.body.replaceChildren() +}) + +describe('useVirtualRowMeasurementSync sticky offset', () => { + it('pins the header the element is actually showing, not the drifted one (STA-6697)', () => { + const { activeStickyHeaderIndexRef } = renderMeasurementSync({ + scrollTop: 0, + virtualizerOffset: DRIFTED_OFFSET + }) + + // Project A fills the viewport, so Project A stays pinned. Pinning B paints it over row 0 + // and leaves B's own slot blank — the reported gap. + expect(activeStickyHeaderIndexRef.current).toBe(0) + }) + + it('promotes the second project once the element has really scrolled', () => { + const { activeStickyHeaderIndexRef } = renderMeasurementSync({ + scrollTop: DRIFTED_OFFSET, + virtualizerOffset: DRIFTED_OFFSET + }) + + // Guards the test above against passing for a setup reason: the hook does run, does write the + // ref, and does promote when the element genuinely moved. + expect(activeStickyHeaderIndexRef.current).toBe(2) + }) +}) diff --git a/src/renderer/src/components/sidebar/worktree-list/viewport/virtual-rows.ts b/src/renderer/src/components/sidebar/worktree-list/viewport/virtual-rows.ts index e5a1d9fb4f6..2a2e52a35bf 100644 --- a/src/renderer/src/components/sidebar/worktree-list/viewport/virtual-rows.ts +++ b/src/renderer/src/components/sidebar/worktree-list/viewport/virtual-rows.ts @@ -190,6 +190,15 @@ function getHostStickyIndexes(rows: readonly RenderRow[], sticky: readonly numbe * Reading the element keeps the pinned header tied to what is actually on * screen. The remembered offset stays as the fallback for the pre-mount pass, * where there is no element to ask. + * + * On cost: this is a render-phase DOM read, which is only cheap while layout is + * clean at that point. What keeps it clean is not that nothing above it touches + * the DOM — `useVirtualRowMeasurementSync`'s own layout effect can re-render + * synchronously through `measureElement` — it is that `measureElement` already + * calls `getBoundingClientRect()` on the same pass, so layout has been forced + * before this read reaches it. There are no production `scrollTop` writes in the + * viewport or the scroll-anchor hook, so nothing creates a write-read interleave. + * Adding a DOM write above this read would invalidate that, not the ordering. */ export function resolveStickyScrollOffset(args: { element: Pick | null