From 6a74dddd43eb33e73f2fd3d7ab3ce9617a0c33e0 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 23 May 2026 11:51:26 -0700 Subject: [PATCH] fix: address review findings (#2689) --- .../src/components/sidebar/WorktreeList.tsx | 38 ++++++++++---- .../worktree-list-scroll-adjustment.test.ts | 35 ++++++++++++- .../sidebar/worktree-list-virtual-rows.ts | 49 +++++++++++++++++++ 3 files changed, 111 insertions(+), 11 deletions(-) diff --git a/src/renderer/src/components/sidebar/WorktreeList.tsx b/src/renderer/src/components/sidebar/WorktreeList.tsx index 8878bd75d..ee2410e99 100644 --- a/src/renderer/src/components/sidebar/WorktreeList.tsx +++ b/src/renderer/src/components/sidebar/WorktreeList.tsx @@ -69,6 +69,8 @@ import { import { estimateRenderRowSize, getActiveStickyHeaderIndex, + getActiveStickyHeaderIndexForScroll, + getPreviousStickyHeaderIndex, getStickyHeaderIndexes, getVirtualRowTransform, shouldUseHeaderTopSpacing, @@ -671,6 +673,7 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp const stickyHeaderIndexesRef = useRef(stickyHeaderIndexes) stickyHeaderIndexesRef.current = stickyHeaderIndexes const activeStickyHeaderIndexRef = useRef(null) + const stickyRangeStartIndexRef = useRef(0) const activeWorktreeRowIndex = useMemo( () => renderRows.findIndex((row) => renderRowContainsWorktree(row, activeWorktreeId)), [renderRows, activeWorktreeId] @@ -799,20 +802,28 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp ), measureElement: measureCurrentVirtualRowElement, rangeExtractor: useCallback((range: Range) => { + stickyRangeStartIndexRef.current = range.startIndex const activeStickyHeaderIndex = getActiveStickyHeaderIndex( stickyHeaderIndexesRef.current, range.startIndex ) - activeStickyHeaderIndexRef.current = activeStickyHeaderIndex if (activeStickyHeaderIndex === null) { return defaultRangeExtractor(range) } // Why: this mirrors TanStack Virtual's sticky example — the active // section header remains a real virtual row even after it scrolls out. - return Array.from(new Set([activeStickyHeaderIndex, ...defaultRangeExtractor(range)])).sort( - (a, b) => a - b + const previousStickyHeaderIndex = getPreviousStickyHeaderIndex( + stickyHeaderIndexesRef.current, + activeStickyHeaderIndex ) + return Array.from( + new Set([ + activeStickyHeaderIndex, + ...(previousStickyHeaderIndex === null ? [] : [previousStickyHeaderIndex]), + ...defaultRangeExtractor(range) + ]) + ).sort((a, b) => a - b) }, []), overscan: 10, gap: 6, @@ -944,6 +955,15 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp ) const totalSize = virtualizer.getTotalSize() const virtualItems = virtualizer.getVirtualItems() + const activeStickyHeaderIndex = getActiveStickyHeaderIndexForScroll({ + firstHeaderIndex, + rangeStartIndex: stickyRangeStartIndexRef.current, + rows: renderRows, + scrollOffset: virtualizer.scrollOffset ?? scrollOffsetRef.current, + stickyHeaderIndexes, + virtualItems + }) + activeStickyHeaderIndexRef.current = activeStickyHeaderIndex const measureMountedRows = useCallback(() => { virtualizer.elementsCache.forEach((element) => { @@ -1812,8 +1832,11 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp className={cn( 'left-0 right-0', // Why: keep the secondary-header spacer on the measured - // virtual row so sticky swaps do not change row height. - hasHeaderTopSpacing && 'pt-2', + // virtual row only while it scrolls in normally. The + // active sticky row is measured from estimates, and moving + // the painted header with a transform makes the repo label + // visibly hop during sticky handoff. + hasHeaderTopSpacing && !isActiveStickyHeader && 'pt-2', isActiveStickyHeader ? 'sticky -top-px z-20 bg-sidebar' : 'absolute top-0' )} style={ @@ -1840,11 +1863,6 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp isPinnedHeader && pinDragOver && 'rounded-md bg-sidebar-accent ring-1 ring-sidebar-ring/40', - // First header sits directly under SidebarHeader, which - // already supplies its own spacing. Secondary sticky - // headers keep their spacer measured while the painted - // header stays flush to the scrollport top. - isActiveStickyHeader && hasHeaderTopSpacing && '-translate-y-2', row.repo && 'overflow-hidden' )} onDragOver={ diff --git a/src/renderer/src/components/sidebar/worktree-list-scroll-adjustment.test.ts b/src/renderer/src/components/sidebar/worktree-list-scroll-adjustment.test.ts index 0655b524b..55b2240e2 100644 --- a/src/renderer/src/components/sidebar/worktree-list-scroll-adjustment.test.ts +++ b/src/renderer/src/components/sidebar/worktree-list-scroll-adjustment.test.ts @@ -4,7 +4,10 @@ import { resolvePendingSidebarReveal, shouldAdjustWorktreeSidebarMeasuredRowScroll } from './WorktreeList' -import { estimateRenderRowSize } from './worktree-list-virtual-rows' +import { + estimateRenderRowSize, + getActiveStickyHeaderIndexForScroll +} from './worktree-list-virtual-rows' const makeHeaderRow = (key: string) => ({ @@ -105,4 +108,34 @@ describe('estimateRenderRowSize', () => { expect(inactiveSize).toBe(36) expect(activeSize).toBe(36) }) + + it('keeps the previous header active while a secondary header spacer crosses the top', () => { + const rows = [makeHeaderRow('first'), makeHeaderRow('second')] + + expect( + getActiveStickyHeaderIndexForScroll({ + firstHeaderIndex: 0, + rangeStartIndex: 1, + rows, + scrollOffset: 100, + stickyHeaderIndexes: [0, 1], + virtualItems: [{ key: 'hdr:second', index: 1, start: 100, end: 136, size: 36, lane: 0 }] + }) + ).toBe(0) + }) + + it('activates a secondary header once its painted header reaches the top', () => { + const rows = [makeHeaderRow('first'), makeHeaderRow('second')] + + expect( + getActiveStickyHeaderIndexForScroll({ + firstHeaderIndex: 0, + rangeStartIndex: 1, + rows, + scrollOffset: 108, + stickyHeaderIndexes: [0, 1], + virtualItems: [{ key: 'hdr:second', index: 1, start: 100, end: 136, size: 36, lane: 0 }] + }) + ).toBe(1) + }) }) diff --git a/src/renderer/src/components/sidebar/worktree-list-virtual-rows.ts b/src/renderer/src/components/sidebar/worktree-list-virtual-rows.ts index 173f31c2e..f334de178 100644 --- a/src/renderer/src/components/sidebar/worktree-list-virtual-rows.ts +++ b/src/renderer/src/components/sidebar/worktree-list-virtual-rows.ts @@ -1,3 +1,4 @@ +import type { VirtualItem } from '@tanstack/react-virtual' import type { Row } from './worktree-list-groups' import { PINNED_GROUP_KEY } from './worktree-list-groups' @@ -69,3 +70,51 @@ export function getActiveStickyHeaderIndex( } return null } + +export function getPreviousStickyHeaderIndex( + stickyHeaderIndexes: readonly number[], + headerIndex: number +): number | null { + const currentPosition = stickyHeaderIndexes.indexOf(headerIndex) + if (currentPosition <= 0) { + return null + } + return stickyHeaderIndexes[currentPosition - 1] ?? null +} + +export function getActiveStickyHeaderIndexForScroll(args: { + firstHeaderIndex: number + rangeStartIndex: number + rows: readonly RenderRow[] + scrollOffset: number + stickyHeaderIndexes: readonly number[] + virtualItems: readonly VirtualItem[] +}): number | null { + const candidateIndex = getActiveStickyHeaderIndex(args.stickyHeaderIndexes, args.rangeStartIndex) + if (candidateIndex === null) { + return null + } + + const candidate = args.virtualItems.find((item) => item.index === candidateIndex) + if (!candidate) { + return candidateIndex + } + + const activationOffset = + candidate.start + + (shouldUseHeaderTopSpacing({ + rows: args.rows, + index: candidateIndex, + firstHeaderIndex: args.firstHeaderIndex + }) + ? SECONDARY_GROUP_HEADER_TOP_MARGIN + : 0) + if (args.scrollOffset >= activationOffset) { + return candidateIndex + } + + // Why: secondary headers include their inter-group spacer in the measured + // row. Keeping the previous sticky header active until the painted header, + // not the spacer, reaches the top prevents an 8px snap on handoff. + return getPreviousStickyHeaderIndex(args.stickyHeaderIndexes, candidateIndex) ?? candidateIndex +}