mirror of
https://github.com/stablyai/orca.git
synced 2026-10-17 00:03:08 +00:00
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.
This commit is contained in:
@@ -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<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)
|
||||
@@ -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
|
||||
])
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user