fix: address review findings (#2689)

This commit is contained in:
Jinjing 2026-05-23 11:51:26 -07:00 committed by GitHub
parent f03ed7ec25
commit 6a74dddd43
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 111 additions and 11 deletions

View File

@ -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<number | null>(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={

View File

@ -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)
})
})

View File

@ -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
}