Revert "Remove temporary scroll-padding centering for boundary sidebar rows (#8036)"

This reverts commit 830024dc2e.
This commit is contained in:
Jinjing
2026-07-10 02:58:34 -07:00
parent c17d7cdae8
commit dd288dae27
4 changed files with 100 additions and 20 deletions
@@ -100,6 +100,7 @@ import {
type RenderRow
} from './worktree-list-virtual-rows'
import {
getElementCenteringScrollPadding,
revealElementInScrollContainer,
WORKTREE_SIDEBAR_REVEAL_TOP_INSET
} from './worktree-sidebar-reveal'
@@ -287,6 +288,7 @@ import {
import { getFolderWorkspaceCardPrDisplay } from './folder-workspace-card-pr-display'
export {
getCenteringScrollPadding,
getScrollTopToRevealBounds,
WORKTREE_SIDEBAR_REVEAL_TOP_INSET
} from './worktree-sidebar-reveal'
@@ -1320,6 +1322,11 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp
const [worktreeDragState, setWorktreeDragState] = useState<WorktreeRowDragState>(
WORKTREE_ROW_DRAG_INITIAL_STATE
)
const [centeringScrollPadding, setCenteringScrollPadding] = useState<{
targetKey: string
start: number
end: number
} | null>(null)
const [pendingRevealRetryTick, setPendingRevealRetryTick] = useState(0)
const [documentVisibilityRevision, setDocumentVisibilityRevision] = useState(0)
const [highlightedRevealRowKey, setHighlightedRevealRowKey] = useState<string | null>(null)
@@ -1969,6 +1976,8 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp
),
overscan: 10,
gap: 6,
paddingStart: centeringScrollPadding?.start ?? 0,
paddingEnd: centeringScrollPadding?.end ?? 0,
// Why: the active sticky group header is rendered inside the virtual list,
// so TanStack's scroll math needs the same top inset as the exact DOM reveal.
scrollPaddingStart: WORKTREE_SIDEBAR_REVEAL_TOP_INSET,
@@ -2120,10 +2129,30 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp
}
}
const optionId = getRenderRowOptionId(targetRow, pendingRevealWorktree.worktreeId)
const centeringTargetKey = `worktree:${optionId ?? pendingRevealWorktree.worktreeId}`
if (centeringScrollPadding && centeringScrollPadding.targetKey !== centeringTargetKey) {
setCenteringScrollPadding(null)
retryExactRevealOnNextFrame()
return
}
const option = optionId
? document.getElementById(optionId)
: getMountedWorktreeOptions(pendingRevealWorktree.worktreeId, container)[0]
const mountedOption = container && option && container.contains(option) ? option : null
if (container && mountedOption) {
const additionalPadding = getElementCenteringScrollPadding(container, mountedOption)
if (additionalPadding && (additionalPadding.start > 0 || additionalPadding.end > 0)) {
// Why: the first and last virtual rows need temporary boundary space
// or the browser clamps their centered position to the list edge.
setCenteringScrollPadding({
targetKey: centeringTargetKey,
start: (centeringScrollPadding?.start ?? 0) + additionalPadding.start,
end: (centeringScrollPadding?.end ?? 0) + additionalPadding.end
})
retryExactRevealOnNextFrame()
return
}
}
const revealedOption =
container && mountedOption && revealElementInScrollContainer(container, mountedOption)
? mountedOption
@@ -2199,6 +2228,7 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp
pendingRevealRetryTick,
flashRevealedRow,
setRenamingWorktreeId,
centeringScrollPadding,
schedulePendingRevealFrame,
cancelPendingRevealFrames
])
@@ -2282,6 +2312,14 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp
}
const container = scrollRef.current
// Why: only clear stale boundary padding here. While a worktree reveal is
// still pending, clearing would wipe the padding it just accumulated and
// its boundary target could exhaust retries without ever centering.
if (centeringScrollPadding && !pendingRevealWorktree) {
setCenteringScrollPadding(null)
retryExactRevealOnNextFrame()
return
}
const revealedElement = container
? revealMountedSidebarRowElement(container, pendingRevealSidebarRow.rowKey)
: null
@@ -2307,6 +2345,7 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp
}
}, [
pendingRevealSidebarRow,
pendingRevealWorktree,
repoMap,
projectGroups,
projectGrouping,
@@ -2318,6 +2357,7 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp
pendingRevealRetryTick,
flashRevealedRow,
clearPendingRevealSidebarRow,
centeringScrollPadding,
schedulePendingRevealFrame,
cancelPendingRevealFrames
])
@@ -1,12 +1,12 @@
import { describe, expect, it, vi } from 'vitest'
import {
countRecordKeysByReference,
getCenteringScrollPadding,
getScrollTopToRevealBounds,
resolvePendingSidebarReveal,
WORKTREE_SIDEBAR_REVEAL_TOP_INSET,
shouldAdjustWorktreeSidebarMeasuredRowScroll
} from './WorktreeList'
import { revealElementInScrollContainer } from './worktree-sidebar-reveal'
import {
extractWorktreeVirtualRowIndexes,
estimateRenderRowSize,
@@ -125,6 +125,21 @@ describe('shouldAdjustWorktreeSidebarMeasuredRowScroll', () => {
})
describe('getScrollTopToRevealBounds', () => {
it('requests leading space when the first target would be clamped above center', () => {
const container = makeScrollContainer(0, 400)
expect(
getCenteringScrollPadding(
container,
{
start: 34,
end: 74
},
WORKTREE_SIDEBAR_REVEAL_TOP_INSET
)
).toEqual({ start: 163, end: 0 })
})
it('centers a fully visible target', () => {
const container = makeScrollContainer(100, 400)
@@ -155,22 +170,19 @@ describe('getScrollTopToRevealBounds', () => {
).toBe(136)
})
it('clamps a boundary target to the list edge instead of padding the list', () => {
// Regression (#8019 follow-up): centering first/last rows via temporary
// paddingStart/paddingEnd left a permanent phantom gap in the sidebar once
// the reveal finished, since nothing cleared the padding afterwards.
const container = {
scrollTop: 100,
clientHeight: 200,
contains: () => true,
getBoundingClientRect: () => ({ top: 0, bottom: 200 })
} as unknown as HTMLElement
const firstRow = {
getBoundingClientRect: () => ({ top: -40, bottom: 20 })
} as unknown as Element
it('requests trailing space when the last target would be clamped below center', () => {
const container = makeScrollContainer(100, 400, 500)
expect(revealElementInScrollContainer(container, firstRow)).toBe(true)
expect(container.scrollTop).toBe(0)
expect(
getCenteringScrollPadding(
container,
{
start: 430,
end: 470
},
WORKTREE_SIDEBAR_REVEAL_TOP_INSET
)
).toEqual({ start: 0, end: 133 })
})
})
@@ -9,8 +9,8 @@ describe('getScrollTopToRevealBounds', () => {
}) as HTMLElement
it('centers a mounted current workspace card that starts above the viewport', () => {
// Raw result may be negative; revealElementInScrollContainer clamps it to
// the list edge so boundary reveals never pad the list.
// Raw result may be negative; boundary reveals convert the deficit into
// temporary paddingStart instead of clamping (see getCenteringScrollPadding).
expect(getScrollTopToRevealBounds(makeContainer(100, 200), { start: 60, end: 120 })).toBe(-10)
})
@@ -9,6 +9,11 @@ type SidebarRevealBounds = {
end: number
}
type SidebarCenteringScrollPadding = {
start: number
end: number
}
function getElementScrollBounds(container: HTMLElement, element: Element): SidebarRevealBounds {
const containerRect = container.getBoundingClientRect()
const elementRect = element.getBoundingClientRect()
@@ -31,6 +36,30 @@ export function getScrollTopToRevealBounds(
return targetCenter - viewportCenterOffset
}
export function getCenteringScrollPadding(
container: HTMLElement,
bounds: SidebarRevealBounds,
topInset = 0
): SidebarCenteringScrollPadding {
const desiredScrollTop = getScrollTopToRevealBounds(container, bounds, topInset)
const maxScrollTop = Math.max(0, container.scrollHeight - container.clientHeight)
return {
start: Math.ceil(Math.max(0, -desiredScrollTop)),
end: Math.ceil(Math.max(0, desiredScrollTop - maxScrollTop))
}
}
export function getElementCenteringScrollPadding(
container: HTMLElement,
element: Element,
topInset = WORKTREE_SIDEBAR_REVEAL_TOP_INSET
): SidebarCenteringScrollPadding | null {
if (!container.contains(element)) {
return null
}
return getCenteringScrollPadding(container, getElementScrollBounds(container, element), topInset)
}
export function revealElementInScrollContainer(container: HTMLElement, element: Element): boolean {
if (!container.contains(element)) {
return false
@@ -41,8 +70,7 @@ export function revealElementInScrollContainer(container: HTMLElement, element:
WORKTREE_SIDEBAR_REVEAL_TOP_INSET
)
// Why: sidebar reveal is a focus handoff, so reposition immediately instead
// of making the user track an animated list. Boundary rows clamp to the list
// edge — padding the list so they can center leaves a phantom gap behind.
// of making the user track an animated list.
container.scrollTop = Math.max(0, nextScrollTop)
return true
}