mirror of
https://github.com/stablyai/orca.git
synced 2026-10-12 16:03:03 +00:00
test(sidebar): pin the sticky offset at the call site, not just the helper
The five resolveStickyScrollOffset tests each build the offset by calling the helper directly and hand the result to getActiveStickyIndexesForScroll. None of them runs useVirtualRowMeasurementSync, which is where the fix actually lives, so reverting that call site to `virtualizer.scrollOffset ?? scrollOffsetRef.current` — the bug, restored in the shipping path — left all 2,511 sidebar tests green. Manual validation was negative, so the helper ablation was the only evidence there was, and it was evidence about the helper. This drives the hook with a scroll element pinned at 0 and a drifted virtualizer.scrollOffset, and asserts activeStickyHeaderIndexRef lands on the first project. A second case scrolls the element for real and asserts the second project is promoted, so the first cannot pass because the hook never ran. The element is modelled with a getter and a no-op setter, which is what an unscrollable list does: the browser clamps the write, the element never moves, and that is precisely why the remembered offset can drift and never resync. Also corrects the cost note on resolveStickyScrollOffset. The read is cheap because measureElement already forces layout on the same pass, not because nothing above it touches the DOM — the hook's own layout effect can re-render synchronously through measureElement. The stated invariant would have gone stale the day someone added a DOM write above the read; this one does not.
This commit is contained in:
+123
@@ -0,0 +1,123 @@
|
||||
// @vitest-environment happy-dom
|
||||
|
||||
import { cleanup, renderHook } from '@testing-library/react'
|
||||
import { afterEach, describe, expect, it } from 'vitest'
|
||||
import type { VirtualItem } from '@tanstack/react-virtual'
|
||||
import { useVirtualRowMeasurementSync } from './use-row-measurement'
|
||||
import { getStickyHeaderIndexes } from './virtual-rows'
|
||||
import type { RenderRow } from '../listing/render-row'
|
||||
import type { WorktreeListVirtualizer } from './use-virtualizer'
|
||||
import type { VirtualizedScrollAnchor } from '@/hooks/useVirtualizedScrollAnchor'
|
||||
|
||||
/**
|
||||
* STA-6697 at the CALL SITE.
|
||||
*
|
||||
* virtual-rows.test.ts pins `resolveStickyScrollOffset` itself, but every one of those tests
|
||||
* builds the offset by calling the helper directly and hands the result to the resolver. None of
|
||||
* them runs this hook, which is where the fix actually lives — so reverting this file to
|
||||
* `virtualizer.scrollOffset ?? scrollOffsetRef.current` left the whole sidebar suite green while
|
||||
* shipping the bug. This is the test that fails on that revert.
|
||||
*/
|
||||
|
||||
// Minimal but real: buildLineageRowRekeyMap reads `sectionKey` and the worktree's lineage fields.
|
||||
function itemRow(id: string, sectionKey: string): RenderRow {
|
||||
return {
|
||||
type: 'item',
|
||||
key: id,
|
||||
sectionKey,
|
||||
worktree: { id, path: `/tmp/${id}`, branch: id }
|
||||
} as unknown as RenderRow
|
||||
}
|
||||
|
||||
// Two projects, collapsed, in a list too short to scroll: header, row, header, row.
|
||||
const rows: RenderRow[] = [
|
||||
{ type: 'header', key: 'project-a', label: 'project-a', count: 1, tone: 'text-foreground' },
|
||||
itemRow('wt-a', 'project-a'),
|
||||
{ type: 'header', key: 'project-b', label: 'project-b', count: 1, tone: 'text-foreground' },
|
||||
itemRow('wt-b', 'project-b')
|
||||
]
|
||||
const virtualItems = [
|
||||
{ index: 0, start: 0 },
|
||||
{ index: 1, start: 36 },
|
||||
{ index: 2, start: 70 },
|
||||
{ index: 3, start: 106 }
|
||||
] as VirtualItem[]
|
||||
|
||||
/** The believed offset a post-mount size correction books while the element stays at 0. */
|
||||
const DRIFTED_OFFSET = 70
|
||||
|
||||
function createScrollElement(scrollTop: number): HTMLDivElement {
|
||||
const element = document.createElement('div')
|
||||
document.body.append(element)
|
||||
// Getter plus a no-op setter: this models the STA-6697 case faithfully. The list is too short
|
||||
// to scroll, so the browser clamps any write and the element never moves — which is exactly why
|
||||
// the virtualizer's remembered offset can drift away from it and never resync.
|
||||
Object.defineProperty(element, 'scrollTop', {
|
||||
configurable: true,
|
||||
get: () => scrollTop,
|
||||
set: () => undefined
|
||||
})
|
||||
return element
|
||||
}
|
||||
|
||||
function renderMeasurementSync(args: { scrollTop: number; virtualizerOffset: number | null }) {
|
||||
const activeStickyHeaderIndexRef = { current: null as number | null }
|
||||
const virtualization = {
|
||||
virtualizer: {
|
||||
scrollOffset: args.virtualizerOffset,
|
||||
elementsCache: new Map<string, Element>(),
|
||||
getTotalSize: () => 142,
|
||||
getVirtualItems: () => virtualItems,
|
||||
measureElement: () => undefined
|
||||
},
|
||||
isCurrentVirtualRowElement: () => true,
|
||||
stickyHeaderIndexes: getStickyHeaderIndexes(rows),
|
||||
activeStickyHeaderIndexRef,
|
||||
activeStickyHostIndexRef: { current: null as number | null },
|
||||
// Drift also advanced the virtualizer's range, so the candidate handed in is already project B.
|
||||
stickyRangeStartIndexRef: { current: 2 }
|
||||
} as unknown as WorktreeListVirtualizer
|
||||
|
||||
const scrollRef = { current: createScrollElement(args.scrollTop) }
|
||||
const view = renderHook(() =>
|
||||
useVirtualRowMeasurementSync({
|
||||
renderRows: rows,
|
||||
virtualization,
|
||||
scrollRef,
|
||||
scrollOffsetRef: { current: DRIFTED_OFFSET },
|
||||
scrollAnchorRef: { current: {} as VirtualizedScrollAnchor },
|
||||
hasDirectScrollInput: () => false,
|
||||
shouldSkipScrollAnchorRestore: () => true
|
||||
})
|
||||
)
|
||||
return { activeStickyHeaderIndexRef, view }
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
cleanup()
|
||||
document.body.replaceChildren()
|
||||
})
|
||||
|
||||
describe('useVirtualRowMeasurementSync sticky offset', () => {
|
||||
it('pins the header the element is actually showing, not the drifted one (STA-6697)', () => {
|
||||
const { activeStickyHeaderIndexRef } = renderMeasurementSync({
|
||||
scrollTop: 0,
|
||||
virtualizerOffset: DRIFTED_OFFSET
|
||||
})
|
||||
|
||||
// Project A fills the viewport, so Project A stays pinned. Pinning B paints it over row 0
|
||||
// and leaves B's own slot blank — the reported gap.
|
||||
expect(activeStickyHeaderIndexRef.current).toBe(0)
|
||||
})
|
||||
|
||||
it('promotes the second project once the element has really scrolled', () => {
|
||||
const { activeStickyHeaderIndexRef } = renderMeasurementSync({
|
||||
scrollTop: DRIFTED_OFFSET,
|
||||
virtualizerOffset: DRIFTED_OFFSET
|
||||
})
|
||||
|
||||
// Guards the test above against passing for a setup reason: the hook does run, does write the
|
||||
// ref, and does promote when the element genuinely moved.
|
||||
expect(activeStickyHeaderIndexRef.current).toBe(2)
|
||||
})
|
||||
})
|
||||
@@ -190,6 +190,15 @@ function getHostStickyIndexes(rows: readonly RenderRow[], sticky: readonly numbe
|
||||
* Reading the element keeps the pinned header tied to what is actually on
|
||||
* screen. The remembered offset stays as the fallback for the pre-mount pass,
|
||||
* where there is no element to ask.
|
||||
*
|
||||
* On cost: this is a render-phase DOM read, which is only cheap while layout is
|
||||
* clean at that point. What keeps it clean is not that nothing above it touches
|
||||
* the DOM — `useVirtualRowMeasurementSync`'s own layout effect can re-render
|
||||
* synchronously through `measureElement` — it is that `measureElement` already
|
||||
* calls `getBoundingClientRect()` on the same pass, so layout has been forced
|
||||
* before this read reaches it. There are no production `scrollTop` writes in the
|
||||
* viewport or the scroll-anchor hook, so nothing creates a write-read interleave.
|
||||
* Adding a DOM write above this read would invalidate that, not the ordering.
|
||||
*/
|
||||
export function resolveStickyScrollOffset(args: {
|
||||
element: Pick<HTMLElement, 'scrollTop'> | null
|
||||
|
||||
Reference in New Issue
Block a user