fix(source-control): avoid render-time layout reads (#8193)

* fix(source-control): stop measuring layout on every virtual list render

Shared-scroller scrollMargin used a deps-less useLayoutEffect that called
getBoundingClientRect after every React render (including git-status polls).
Measure on mount/attach only, then refresh via ResizeObserver and childList
MutationObserver with cleanup, and add focused tests for no render-time
layout reads plus multi-section margin updates.

* fix(source-control): prune stale layout observers
This commit is contained in:
Jinjing 2026-07-10 18:12:57 -07:00 committed by GitHub
parent efa6b7b945
commit 73e082b577
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 553 additions and 13 deletions

View File

@ -0,0 +1,465 @@
// @vitest-environment happy-dom
import { act, useState, type ReactElement } from 'react'
import { createRoot, type Root } from 'react-dom/client'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import {
measureSourceControlScrollMargin,
observeSourceControlScrollMargin,
SOURCE_CONTROL_FILE_ROW_HEIGHT_PX,
SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS,
SourceControlVirtualFileList
} from './source-control-virtual-file-list'
const VIEWPORT_HEIGHT_PX = 600
type ResizeObserverBoxSize = {
blockSize: number
inlineSize: number
}
type TrackedResizeObserver = {
callback: ResizeObserverCallback
elements: Set<Element>
}
const activeResizeObservers = new Set<TrackedResizeObserver>()
class MockResizeObserver implements ResizeObserver {
readonly elements = new Set<Element>()
readonly callback: ResizeObserverCallback
constructor(callback: ResizeObserverCallback) {
this.callback = callback
activeResizeObservers.add(this)
}
observe(element: Element): void {
this.elements.add(element)
}
unobserve(element: Element): void {
this.elements.delete(element)
}
disconnect(): void {
this.elements.clear()
activeResizeObservers.delete(this)
}
}
function fireResizeObservers(target?: Element): void {
for (const observer of activeResizeObservers) {
const targets = target
? observer.elements.has(target)
? [target]
: []
: Array.from(observer.elements)
if (targets.length === 0) {
continue
}
const entries = targets.map((element) => {
const rect = element.getBoundingClientRect()
const size: ResizeObserverBoxSize = {
blockSize: rect.height,
inlineSize: rect.width
}
return {
target: element,
contentRect: rect,
borderBoxSize: [size],
contentBoxSize: [size],
devicePixelContentBoxSize: [size]
} satisfies ResizeObserverEntry
})
observer.callback(entries, observer as unknown as ResizeObserver)
}
}
function manyRows(count: number): string[] {
return Array.from({ length: count }, (_, index) => `row-${String(index).padStart(3, '0')}`)
}
let host: HTMLDivElement
let root: Root
/** Synthetic layout tops for getBoundingClientRect (happy-dom has no layout). */
let topsByElement: WeakMap<Element, number>
beforeEach(() => {
globalThis.IS_REACT_ACT_ENVIRONMENT = true
activeResizeObservers.clear()
topsByElement = new WeakMap()
host = document.createElement('div')
document.body.appendChild(host)
root = createRoot(host)
vi.stubGlobal('ResizeObserver', MockResizeObserver)
vi.spyOn(HTMLElement.prototype, 'offsetHeight', 'get').mockImplementation(
function (this: HTMLElement) {
return this.classList.contains('overflow-auto')
? VIEWPORT_HEIGHT_PX
: SOURCE_CONTROL_FILE_ROW_HEIGHT_PX
}
)
vi.spyOn(Element.prototype, 'getBoundingClientRect').mockImplementation(function (this: Element) {
const top = topsByElement.get(this) ?? 0
const height = this.classList.contains('overflow-auto')
? VIEWPORT_HEIGHT_PX
: SOURCE_CONTROL_FILE_ROW_HEIGHT_PX
return {
top,
bottom: top + height,
height,
left: 0,
right: 240,
width: 240,
x: 0,
y: top,
toJSON: () => ({})
} as DOMRect
})
})
afterEach(() => {
act(() => root.unmount())
host.remove()
activeResizeObservers.clear()
vi.unstubAllGlobals()
vi.restoreAllMocks()
})
function setTop(element: Element, top: number): void {
topsByElement.set(element, top)
}
function SharedScrollerHarness({
aboveHeight,
rows,
rowTestId = 'virtual-row'
}: {
aboveHeight: number
rows: readonly string[]
rowTestId?: string
}): ReactElement {
const [scroller, setScroller] = useState<HTMLDivElement | null>(null)
return (
<div
className="overflow-auto"
ref={(node) => {
if (node) {
setTop(node, 0)
Object.defineProperty(node, 'scrollTop', {
configurable: true,
writable: true,
value: node.scrollTop
})
setScroller(node)
} else {
setScroller(null)
}
}}
>
<div
data-testid="content-above"
ref={(node) => {
if (node) {
setTop(node, 0)
}
}}
style={{ height: aboveHeight }}
/>
<div
data-testid="section-host"
ref={(node) => {
if (node) {
setTop(node, aboveHeight)
}
}}
>
<SourceControlVirtualFileList
rows={rows}
scrollElement={scroller}
getRowKey={(row) => row}
renderRow={(row) => (
<div key={row} data-testid={rowTestId}>
{row}
</div>
)}
/>
</div>
</div>
)
}
function MultiSectionHarness({
firstRows,
secondRows
}: {
firstRows: readonly string[]
secondRows: readonly string[]
}): ReactElement {
const [scroller, setScroller] = useState<HTMLDivElement | null>(null)
return (
<div
className="overflow-auto"
ref={(node) => {
if (node) {
setTop(node, 0)
Object.defineProperty(node, 'scrollTop', {
configurable: true,
writable: true,
value: node.scrollTop
})
}
setScroller(node)
}}
>
<div data-testid="first-section">
<SourceControlVirtualFileList
rows={firstRows}
scrollElement={scroller}
getRowKey={(row) => row}
renderRow={(row) => <div data-testid="first-row">{row}</div>}
/>
</div>
<div data-testid="second-section">
<SourceControlVirtualFileList
rows={secondRows}
scrollElement={scroller}
getRowKey={(row) => row}
renderRow={(row) => <div data-testid="second-row">{row}</div>}
/>
</div>
</div>
)
}
function syncListTop(aboveHeight: number): HTMLDivElement | null {
const list = host.querySelector<HTMLDivElement>('[data-testid="source-control-virtual-list"]')
if (list) {
setTop(list, aboveHeight)
}
const scroller = host.querySelector<HTMLDivElement>('.overflow-auto')
if (scroller) {
setTop(scroller, 0)
}
return list
}
function syncMultiSectionListTops(firstRowCount: number): void {
const lists = host.querySelectorAll<HTMLElement>('[data-testid="source-control-virtual-list"]')
if (lists[0]) {
setTop(lists[0], 0)
}
if (lists[1]) {
setTop(lists[1], firstRowCount * SOURCE_CONTROL_FILE_ROW_HEIGHT_PX)
}
}
describe('measureSourceControlScrollMargin', () => {
it('returns the list offset inside the scroller independent of scrollTop', () => {
const scroller = document.createElement('div')
const list = document.createElement('div')
setTop(scroller, 100)
setTop(list, 250)
Object.defineProperty(scroller, 'scrollTop', { configurable: true, value: 40 })
expect(measureSourceControlScrollMargin(list, scroller)).toBe(190)
Object.defineProperty(scroller, 'scrollTop', { configurable: true, value: 120 })
setTop(list, 170)
// list.top dropped by the same amount scrollTop rose → margin unchanged.
expect(measureSourceControlScrollMargin(list, scroller)).toBe(190)
})
})
describe('observeSourceControlScrollMargin', () => {
it('notifies on resize and disconnects cleanly', () => {
const scroller = document.createElement('div')
const child = document.createElement('div')
const list = document.createElement('div')
scroller.append(child, list)
const onLayout = vi.fn()
const cleanup = observeSourceControlScrollMargin(list, scroller, onLayout)
expect(activeResizeObservers.size).toBe(1)
fireResizeObservers()
expect(onLayout).toHaveBeenCalled()
onLayout.mockClear()
cleanup()
expect(activeResizeObservers.size).toBe(0)
fireResizeObservers()
expect(onLayout).not.toHaveBeenCalled()
})
it('tracks current direct scroller children without retaining removed siblings', async () => {
const scroller = document.createElement('div')
const list = document.createElement('div')
const removedSibling = document.createElement('div')
scroller.append(removedSibling, list)
const onLayout = vi.fn()
const cleanup = observeSourceControlScrollMargin(list, scroller, onLayout)
const observer = Array.from(activeResizeObservers)[0]
expect(observer?.elements.has(removedSibling)).toBe(true)
onLayout.mockClear()
const sibling = document.createElement('div')
scroller.insertBefore(sibling, list)
removedSibling.remove()
// happy-dom delivers MutationObserver callbacks asynchronously.
await vi.waitFor(() => {
expect(onLayout).toHaveBeenCalled()
expect(observer?.elements.has(sibling)).toBe(true)
expect(observer?.elements.has(removedSibling)).toBe(false)
})
cleanup()
})
})
describe('SourceControlVirtualFileList scroll-margin lifecycle', () => {
it('does not read layout during ordinary re-renders after the initial measure', () => {
const aboveHeight = 160
const baseRows = manyRows(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS)
act(() => {
root.render(
<SharedScrollerHarness aboveHeight={aboveHeight} rows={baseRows.map((row) => `${row}@0`)} />
)
})
syncListTop(aboveHeight)
act(() => {
fireResizeObservers()
})
const rectSpy = vi.mocked(Element.prototype.getBoundingClientRect)
rectSpy.mockClear()
// Status-poll style re-render: new row identities, unchanged layout.
act(() => {
root.render(
<SharedScrollerHarness aboveHeight={aboveHeight} rows={baseRows.map((row) => `${row}@1`)} />
)
})
expect(rectSpy).not.toHaveBeenCalled()
expect(host.textContent).toContain('row-000@1')
})
it('keeps multi-section windowing correct when content above resizes', () => {
const rows = manyRows(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS + 20)
let aboveHeight = 200
act(() => {
root.render(
<SharedScrollerHarness aboveHeight={aboveHeight} rows={rows} rowTestId="section-row" />
)
})
syncListTop(aboveHeight)
act(() => {
fireResizeObservers()
})
expect(host.querySelector<HTMLElement>('[data-index="0"]')?.style.transform).toBe(
'translateY(0px)'
)
// Content above grows (sibling section / commit area).
aboveHeight = 420
act(() => {
root.render(
<SharedScrollerHarness aboveHeight={aboveHeight} rows={rows} rowTestId="section-row" />
)
})
syncListTop(aboveHeight)
act(() => {
fireResizeObservers()
})
// Local row positioning stays container-relative after margin update.
expect(host.querySelector<HTMLElement>('[data-index="0"]')?.style.transform).toBe(
'translateY(0px)'
)
const scroller = host.querySelector<HTMLDivElement>('.overflow-auto')
expect(scroller).toBeTruthy()
if (!scroller) {
return
}
// Scroll to the section origin; window should still show this section's head.
Object.defineProperty(scroller, 'scrollTop', {
configurable: true,
writable: true,
value: aboveHeight
})
act(() => {
scroller.dispatchEvent(new Event('scroll'))
})
expect(host.querySelector('[data-testid="section-row"]')?.textContent).toBe('row-000')
expect(host.querySelectorAll('[data-testid="section-row"]').length).toBeLessThan(rows.length)
})
it('updates a later virtual section when an earlier virtual section resizes', () => {
let firstRows = manyRows(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS + 10).map((row) => `first-${row}`)
const secondRows = manyRows(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS + 20).map(
(row) => `second-${row}`
)
act(() => {
root.render(<MultiSectionHarness firstRows={firstRows} secondRows={secondRows} />)
})
syncMultiSectionListTops(firstRows.length)
act(() => fireResizeObservers())
firstRows = manyRows(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS + 80).map((row) => `first-${row}`)
act(() => {
root.render(<MultiSectionHarness firstRows={firstRows} secondRows={secondRows} />)
})
syncMultiSectionListTops(firstRows.length)
const firstSection = host.querySelector('[data-testid="first-section"]')
expect(firstSection).toBeTruthy()
act(() => {
if (firstSection) {
fireResizeObservers(firstSection)
}
})
const scroller = host.querySelector<HTMLDivElement>('.overflow-auto')
expect(scroller).toBeTruthy()
if (!scroller) {
return
}
Object.defineProperty(scroller, 'scrollTop', {
configurable: true,
writable: true,
value: firstRows.length * SOURCE_CONTROL_FILE_ROW_HEIGHT_PX
})
act(() => scroller.dispatchEvent(new Event('scroll')))
expect(host.querySelector('[data-testid="second-row"]')?.textContent).toBe('second-row-000')
expect(host.querySelectorAll('[data-testid="second-row"]').length).toBeLessThan(
secondRows.length
)
})
it('renders small lists without the virtualization shell or observers', () => {
const rows = ['a', 'b', 'c']
expect(rows.length).toBeLessThan(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS)
act(() => {
root.render(<SharedScrollerHarness aboveHeight={80} rows={rows} rowTestId="plain" />)
})
expect(host.querySelector('[data-testid="source-control-virtual-list"]')).toBeNull()
expect(host.querySelectorAll('[data-testid="plain"]').length).toBe(3)
expect(activeResizeObservers.size).toBe(0)
})
})

View File

@ -13,6 +13,67 @@ export const SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS = 50
export const SOURCE_CONTROL_FILE_ROW_HEIGHT_PX = 24
export const SOURCE_CONTROL_FILE_ROW_OVERSCAN = 10
/**
* Offset of `container` from the start of `scrollElement`'s scrollable content.
* Independent of current scrollTop (relative tops + scrollTop cancel out).
*/
export function measureSourceControlScrollMargin(
container: HTMLElement,
scrollElement: HTMLElement
): number {
return Math.round(
container.getBoundingClientRect().top -
scrollElement.getBoundingClientRect().top +
scrollElement.scrollTop
)
}
/**
* Re-measure when the list or anything that can shift its offset inside the
* shared scroller changes size or structure (commit area, headers, siblings).
* Does not observe subtree mutations virtualized row mount/unmount would
* thrash and never change scroll margin.
*/
export function observeSourceControlScrollMargin(
container: HTMLElement,
scrollElement: HTMLElement,
onLayout: () => void
): () => void {
const resizeObserver = new ResizeObserver(onLayout)
resizeObserver.observe(container)
resizeObserver.observe(scrollElement)
let observedChildren = new Set<Element>()
const observeScrollerChildren = (): void => {
const currentChildren = new Set(scrollElement.children)
// Why: child-list churn can detach previously observed sections; pruning
// targets prevents the long-lived virtual list from retaining stale DOM.
for (const child of observedChildren) {
if (!currentChildren.has(child) && child !== container) {
resizeObserver.unobserve(child)
}
}
for (const child of currentChildren) {
resizeObserver.observe(child)
}
observedChildren = currentChildren
}
observeScrollerChildren()
// Why: sections mount/unmount as direct scroller children; re-observe so a
// newly inserted sibling can still shift this list's margin when it resizes.
const mutationObserver = new MutationObserver(() => {
observeScrollerChildren()
onLayout()
})
mutationObserver.observe(scrollElement, { childList: true })
return () => {
resizeObserver.disconnect()
mutationObserver.disconnect()
}
}
/**
* Windows one source-control section's rows inside the panel's shared
* scroller. Sections below SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS render plainly;
@ -38,23 +99,26 @@ export function SourceControlVirtualFileList<TRow>({
// Why: the section shares the panel scroller with the commit area and
// sibling sections, so the virtualizer needs this list's offset inside that
// scroller. Everything above changes height only through React renders
// (commit drafts, banners, section collapse), so a per-render layout read
// keeps the margin current without observers. The guarded setState makes the
// measure/render loop converge in one extra pass.
// eslint-disable-next-line react-hooks/exhaustive-deps -- no dependency list on purpose: any sibling render can move this list inside the shared scroller.
// scroller. Measure on mount / scrollElement attach and when observers
// report layout shifts — never during ordinary React renders (git-status
// polls re-render often and must not force synchronous layout).
useLayoutEffect(() => {
if (!virtualize) {
return
}
const container = containerRef.current
if (!container || !scrollElement) {
return
}
const nextMargin = Math.round(
container.getBoundingClientRect().top -
scrollElement.getBoundingClientRect().top +
scrollElement.scrollTop
)
setScrollMargin((current) => (current === nextMargin ? current : nextMargin))
})
const updateMargin = (): void => {
const nextMargin = measureSourceControlScrollMargin(container, scrollElement)
setScrollMargin((current) => (current === nextMargin ? current : nextMargin))
}
updateMargin()
return observeSourceControlScrollMargin(container, scrollElement, updateMargin)
}, [scrollElement, virtualize])
const virtualizer = useVirtualizer({
count: rows.length,

View File

@ -80,7 +80,7 @@ async function addAndActivateRepo(orcaPage: Page, repoPath: string): Promise<str
)
.toBeGreaterThan(0)
return await orcaPage.evaluate(
const worktreeId = await orcaPage.evaluate(
({ targetRepoId, pathToRepo }) => {
const store = window.__store
if (!store) {
@ -100,6 +100,17 @@ async function addAndActivateRepo(orcaPage: Page, repoPath: string): Promise<str
},
{ targetRepoId: repoId, pathToRepo: repoPath }
)
// Why: repo activation can finish sidebar routing after the store mutation;
// enter Source Control through the visible control before timing its render.
const sourceControlButton = orcaPage.getByRole('button', { name: /^Source Control/ })
await expect(sourceControlButton).toBeVisible()
await sourceControlButton.click()
await expect
.poll(() => orcaPage.evaluate(() => window.__store?.getState().rightSidebarTab))
.toBe('source-control')
return worktreeId
}
/**