From 69d9b553ebcdb37cebe783cd7fa99c012ab02c2b Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 9 Jul 2026 23:49:33 -0700 Subject: [PATCH] Remove temporary scroll-padding centering for boundary sidebar rows Boundary reveals (first/last row) previously widened the virtual list with paddingStart/paddingEnd to force centering, but nothing cleared that padding once the reveal finished, leaving a phantom gap in the sidebar (#8019 follow-up). Clamp boundary targets to the list edge via revealElementInScrollContainer instead, and drop the now-unused centering-padding helpers and state. --- .../src/components/sidebar/WorktreeList.tsx | 40 ----------------- .../worktree-list-scroll-adjustment.test.ts | 44 +++++++------------ .../worktree-scroll-to-current-button.test.ts | 4 +- .../sidebar/worktree-sidebar-reveal.ts | 32 +------------- 4 files changed, 20 insertions(+), 100 deletions(-) diff --git a/src/renderer/src/components/sidebar/WorktreeList.tsx b/src/renderer/src/components/sidebar/WorktreeList.tsx index a1542323e62..84f5ab77838 100644 --- a/src/renderer/src/components/sidebar/WorktreeList.tsx +++ b/src/renderer/src/components/sidebar/WorktreeList.tsx @@ -100,7 +100,6 @@ import { type RenderRow } from './worktree-list-virtual-rows' import { - getElementCenteringScrollPadding, revealElementInScrollContainer, WORKTREE_SIDEBAR_REVEAL_TOP_INSET } from './worktree-sidebar-reveal' @@ -288,7 +287,6 @@ import { import { getFolderWorkspaceCardPrDisplay } from './folder-workspace-card-pr-display' export { - getCenteringScrollPadding, getScrollTopToRevealBounds, WORKTREE_SIDEBAR_REVEAL_TOP_INSET } from './worktree-sidebar-reveal' @@ -1322,11 +1320,6 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp const [worktreeDragState, setWorktreeDragState] = useState( 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(null) @@ -1976,8 +1969,6 @@ 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, @@ -2129,30 +2120,10 @@ 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 @@ -2228,7 +2199,6 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp pendingRevealRetryTick, flashRevealedRow, setRenamingWorktreeId, - centeringScrollPadding, schedulePendingRevealFrame, cancelPendingRevealFrames ]) @@ -2312,14 +2282,6 @@ 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 @@ -2345,7 +2307,6 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp } }, [ pendingRevealSidebarRow, - pendingRevealWorktree, repoMap, projectGroups, projectGrouping, @@ -2357,7 +2318,6 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp pendingRevealRetryTick, flashRevealedRow, clearPendingRevealSidebarRow, - centeringScrollPadding, schedulePendingRevealFrame, cancelPendingRevealFrames ]) 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 f9a5a5f62ce..aa71d479445 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 @@ -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,21 +125,6 @@ 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) @@ -170,19 +155,22 @@ describe('getScrollTopToRevealBounds', () => { ).toBe(136) }) - it('requests trailing space when the last target would be clamped below center', () => { - const container = makeScrollContainer(100, 400, 500) + 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 - expect( - getCenteringScrollPadding( - container, - { - start: 430, - end: 470 - }, - WORKTREE_SIDEBAR_REVEAL_TOP_INSET - ) - ).toEqual({ start: 0, end: 133 }) + expect(revealElementInScrollContainer(container, firstRow)).toBe(true) + expect(container.scrollTop).toBe(0) }) }) diff --git a/src/renderer/src/components/sidebar/worktree-scroll-to-current-button.test.ts b/src/renderer/src/components/sidebar/worktree-scroll-to-current-button.test.ts index 850fc45704d..e2ec2cfaffd 100644 --- a/src/renderer/src/components/sidebar/worktree-scroll-to-current-button.test.ts +++ b/src/renderer/src/components/sidebar/worktree-scroll-to-current-button.test.ts @@ -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; boundary reveals convert the deficit into - // temporary paddingStart instead of clamping (see getCenteringScrollPadding). + // Raw result may be negative; revealElementInScrollContainer clamps it to + // the list edge so boundary reveals never pad the list. expect(getScrollTopToRevealBounds(makeContainer(100, 200), { start: 60, end: 120 })).toBe(-10) }) diff --git a/src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts b/src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts index 765efc2eabc..56586682a50 100644 --- a/src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts +++ b/src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts @@ -9,11 +9,6 @@ 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() @@ -36,30 +31,6 @@ 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 @@ -70,7 +41,8 @@ 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. + // 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. container.scrollTop = Math.max(0, nextScrollTop) return true }