From 186317fb0a804a5434e15b6529073930dbecccc3 Mon Sep 17 00:00:00 2001 From: solomon Date: Tue, 18 Aug 2026 08:36:34 +0530 Subject: [PATCH 1/2] fix(virtual-core): cancel the isScrolling debounce on scroll-observer cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `observeOffset` arms a debounce on every scroll event to reset `isScrolling` back to `false` after `isScrollingResetDelay`. Its disposer removed the listeners but left that timer running, and the handle was closure-local, so no consumer could clear it either. Unmounting a virtualizer within the delay window therefore still delivered one `(offset, false)` callback, which runs through `maybeNotify()` into the consumer's `onChange` — in React, a dispatch into a tree that no longer exists. A browser swallows it; under jsdom it surfaces as an update after unmount and can fail an otherwise correct suite. `debounce` now exposes `cancel()` and the disposer calls it. The path is the default one: `useScrollendEvent` is off by default, and jsdom has no `onscrollend`, so every jsdom suite takes it. --- .changeset/olive-pugs-repeat.md | 5 ++++ packages/virtual-core/src/index.ts | 4 ++++ packages/virtual-core/src/utils.ts | 18 ++++++++++---- packages/virtual-core/tests/index.test.ts | 29 +++++++++++++++++++++++ 4 files changed, 52 insertions(+), 4 deletions(-) create mode 100644 .changeset/olive-pugs-repeat.md diff --git a/.changeset/olive-pugs-repeat.md b/.changeset/olive-pugs-repeat.md new file mode 100644 index 000000000..fbc11be02 --- /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, so a virtualizer unmounted within `isScrollingResetDelay` of a scroll no longer emits one late `onChange`. diff --git a/packages/virtual-core/src/index.ts b/packages/virtual-core/src/index.ts index bc74b3ea7..420a4d9c0 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() } } diff --git a/packages/virtual-core/src/utils.ts b/packages/virtual-core/src/utils.ts index 5e16080cd..65793f779 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 1c7794c21..2b7020661 100644 --- a/packages/virtual-core/tests/index.test.ts +++ b/packages/virtual-core/tests/index.test.ts @@ -3361,6 +3361,35 @@ test('observeElementOffset: attaches scroll listener and fires callback with scr expect(listeners.has('scroll')).toBe(false) }) +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() From 82eb0df0b8d258ba0461ec1b7a375bcc944a991b Mon Sep 17 00:00:00 2001 From: solomon Date: Tue, 18 Aug 2026 14:25:53 +0530 Subject: [PATCH 2/2] fix(virtual-core): reset the scroll flags in cleanup Cancelling the debounce removed the only writer of `isScrolling = false`. That is fine on unmount, but `cleanup()` also runs when the scroll element changes and when `enabled` goes false, and there the instance stays alive: scroll, disable, wait past `isScrollingResetDelay`, re-enable, and the flag was left stuck on. A stuck flag skips sync measurement, strands the iOS deferred adjustment and leaves `scrollDirection` stale. Reset both flags in `cleanup()` next to the other per-element state. Also drop the `waitForTimeout(250)` in the marko option-gates e2e. It was there to sidestep this exact zombie timer and is annotated to be removed once a cancellable debounce lands, so it now doubles as coverage. --- .changeset/olive-pugs-repeat.md | 2 +- .../e2e/app/e2e/option-gates.spec.ts | 11 --- packages/virtual-core/src/index.ts | 7 ++ packages/virtual-core/tests/index.test.ts | 96 +++++++++++++++++++ 4 files changed, 104 insertions(+), 12 deletions(-) diff --git a/.changeset/olive-pugs-repeat.md b/.changeset/olive-pugs-repeat.md index fbc11be02..5ec01ab63 100644 --- a/.changeset/olive-pugs-repeat.md +++ b/.changeset/olive-pugs-repeat.md @@ -2,4 +2,4 @@ '@tanstack/virtual-core': patch --- -Cancel the pending `isScrolling` reset when a scroll observer is torn down, so a virtualizer unmounted within `isScrollingResetDelay` of a scroll no longer emits one late `onChange`. +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 be26112f3..f1e97e19b 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 420a4d9c0..dc6f1010c 100644 --- a/packages/virtual-core/src/index.ts +++ b/packages/virtual-core/src/index.ts @@ -767,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/tests/index.test.ts b/packages/virtual-core/tests/index.test.ts index 2b7020661..05a674663 100644 --- a/packages/virtual-core/tests/index.test.ts +++ b/packages/virtual-core/tests/index.test.ts @@ -3361,6 +3361,102 @@ 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 {