diff --git a/.changeset/olive-pugs-repeat.md b/.changeset/olive-pugs-repeat.md new file mode 100644 index 00000000..5ec01ab6 --- /dev/null +++ b/.changeset/olive-pugs-repeat.md @@ -0,0 +1,5 @@ +--- +'@tanstack/virtual-core': patch +--- + +Cancel the pending `isScrolling` reset when a scroll observer is torn down, and reset `isScrolling` and `scrollDirection` in `cleanup()` so they don't stay stuck after the scroll element changes or is removed. diff --git a/packages/marko-virtual/e2e/app/e2e/option-gates.spec.ts b/packages/marko-virtual/e2e/app/e2e/option-gates.spec.ts index be26112f..f1e97e19 100644 --- a/packages/marko-virtual/e2e/app/e2e/option-gates.spec.ts +++ b/packages/marko-virtual/e2e/app/e2e/option-gates.spec.ts @@ -101,17 +101,6 @@ test('enabled=false disables the virtualizer (empty window); enabled=true re-win const deep = await renderedIndexes(page) expect(deep[0]!).toBeGreaterThan(50) - // Let the end-of-scroll debounce fire while STILL ENABLED before toggling. - // KNOWN UPSTREAM CORE BUG (found by this gate): core's debounce (utils.ts) has no - // cancel, and observeOffset's unsubscribe only removes the event listeners — a - // pending end-of-scroll timer survives cleanup() and later fires - // cb(staleOffset, false) into the live instance. Disable + re-enable within - // isScrollingResetDelay (150ms) and the stale offset overwrites the correct - // re-enable recompute, leaving a stale window until the next real scroll event. - // This wait sidesteps the zombie timer so the gate asserts the enabled contract - // itself; remove it if/when the core fix (cancellable debounce) lands. - await page.waitForTimeout(250) - // Disable: the deep window disappears (measurements cleared, empty/collapsed window). await page.locator('[data-testid="toggle"]').click() await page.waitForFunction( diff --git a/packages/virtual-core/src/index.ts b/packages/virtual-core/src/index.ts index bc74b3ea..dc6f1010 100644 --- a/packages/virtual-core/src/index.ts +++ b/packages/virtual-core/src/index.ts @@ -217,6 +217,10 @@ const observeOffset = ( if (registerScrollendEvent) { element.removeEventListener('scrollend', endHandler) } + // Removing the listener doesn't retract a reset already queued by the + // last scroll, and that call would land on a virtualizer that has been + // torn down — in React, a dispatch into an unmounted tree. + fallback?.cancel() } } @@ -763,6 +767,13 @@ export class Virtualizer< this.rafId = null } this.scrollState = null + // The debounce cancelled above is the only thing that writes `isScrolling` + // back to false, so a cleanup inside the reset window would strand it, and + // the direction derived from it, as true. That matters because `cleanup` + // also runs when the scroll element changes or `enabled` goes false, where + // the instance lives on. + this.isScrolling = false + this.scrollDirection = null // The iOS gesture/deferral state is scoped to the current scroll // element: the touch listeners that maintain it were just removed, and // an in-flight touch keeps targeting the old element (implicit touch diff --git a/packages/virtual-core/src/utils.ts b/packages/virtual-core/src/utils.ts index 5e16080c..65793f77 100644 --- a/packages/virtual-core/src/utils.ts +++ b/packages/virtual-core/src/utils.ts @@ -102,8 +102,18 @@ export const debounce = ( ms: number, ) => { let timeoutId: number - return function (this: any, ...args: Array) { - targetWindow.clearTimeout(timeoutId) - timeoutId = targetWindow.setTimeout(() => fn.apply(this, args), ms) - } + return Object.assign( + function (this: any, ...args: Array) { + targetWindow.clearTimeout(timeoutId) + timeoutId = targetWindow.setTimeout(() => fn.apply(this, args), ms) + }, + { + // The handle is closure-local, so a caller that has already + // unsubscribed has no way to stop a queued call. Teardown paths use + // this to drop the pending invocation instead of letting it land. + cancel: () => { + targetWindow.clearTimeout(timeoutId) + }, + }, + ) } diff --git a/packages/virtual-core/tests/index.test.ts b/packages/virtual-core/tests/index.test.ts index 1c7794c2..05a67466 100644 --- a/packages/virtual-core/tests/index.test.ts +++ b/packages/virtual-core/tests/index.test.ts @@ -3361,6 +3361,131 @@ test('observeElementOffset: attaches scroll listener and fires callback with scr expect(listeners.has('scroll')).toBe(false) }) +// ─── cleanup resets the scroll flags ───────────────────────────────────────── +// The cancelled debounce is the only writer of `isScrolling = false`, and +// `cleanup()` also runs while the instance stays alive (element swap, +// `enabled: false`), so it has to reset the flags itself. + +const makeScrollFlagsVirtualizer = () => { + const MockResizeObserver = vi.fn(function () { + return { observe: vi.fn(), unobserve: vi.fn(), disconnect: vi.fn() } + }) + const mockWindow = { + requestAnimationFrame: vi.fn(), + cancelAnimationFrame: vi.fn(), + ResizeObserver: MockResizeObserver, + } + const makeElement = () => + ({ + scrollTop: 0, + scrollLeft: 0, + scrollWidth: 1000, + scrollHeight: 5000, + offsetWidth: 400, + offsetHeight: 600, + ownerDocument: { defaultView: mockWindow }, + }) as unknown as HTMLDivElement + + const first = makeElement() + const second = makeElement() + let element: HTMLDivElement | null = first + let emit: ((offset: number, isScrolling: boolean) => void) | null = null + + const virtualizer = new Virtualizer({ + count: 100, + estimateSize: () => 50, + getScrollElement: () => element, + scrollToFn: vi.fn(), + observeElementRect: (_instance, cb) => { + cb({ width: 400, height: 600 }) + return () => {} + }, + observeElementOffset: (_instance, cb) => { + emit = cb + return () => {} + }, + }) + + virtualizer._willUpdate() + + // Mid-scroll: this is the state the debounce used to clear on its own. + emit!(500, true) + + return { + virtualizer, + swapElement: () => { + element = second + virtualizer._willUpdate() + }, + disable: () => { + element = null + virtualizer._willUpdate() + }, + } +} + +test('cleanup resets the scroll flags when the scroll element is swapped', () => { + const { virtualizer, swapElement } = makeScrollFlagsVirtualizer() + + expect(virtualizer.isScrolling).toBe(true) + + swapElement() + + expect(virtualizer.isScrolling).toBe(false) + expect(virtualizer.scrollDirection).toBe(null) +}) + +test('cleanup resets the scroll flags when the scroll element goes away', () => { + const { virtualizer, disable } = makeScrollFlagsVirtualizer() + + expect(virtualizer.isScrolling).toBe(true) + + disable() + + expect(virtualizer.isScrolling).toBe(false) + expect(virtualizer.scrollDirection).toBe(null) +}) + +test('cleanup resets the scroll flags on unmount', () => { + const { virtualizer } = makeScrollFlagsVirtualizer() + + expect(virtualizer.isScrolling).toBe(true) + + virtualizer._didMount()() + + expect(virtualizer.isScrolling).toBe(false) + expect(virtualizer.scrollDirection).toBe(null) +}) + +test('observeElementOffset: cleanup drops the queued isScrolling reset', () => { + vi.useFakeTimers() + try { + const cb = vi.fn() + const listeners = new Map() + const el: any = { + scrollTop: 50, + scrollLeft: 0, + addEventListener: (name: string, fn: any) => listeners.set(name, fn), + removeEventListener: (name: string) => listeners.delete(name), + } + const cleanup = observeElementOffset(makeObserveInstance(el) as any, cb) + + // Each scroll arms a debounce that resets isScrolling to false. + listeners.get('scroll')!({} as Event) + expect(cb).toHaveBeenCalledWith(50, true) + cb.mockClear() + + // Tearing down inside that window must not leave the reset queued — + // it would arrive after the consumer stopped listening. + cleanup?.() + vi.advanceTimersByTime(1000) + + expect(cb).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } +}) + test('observeElementOffset: reads scrollLeft + applies isRtl when horizontal', () => { const cb = vi.fn() const listeners = new Map()