fix(virtual-core): cancel the isScrolling debounce on scroll-observer cleanup - #1256
Conversation
… cleanup `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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review. 📝 WalkthroughWalkthroughThis change adds cancellation to the debounce utility. Offset observer teardown cancels pending scroll resets. Virtualizer cleanup resets scrolling state. Tests cover element replacement, disabling, unmounting, and delayed callbacks. ChangesScroll reset teardown
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change cancels a queued scrolling reset during observer cleanup, preventing late callbacks after teardown; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/virtual-core/tests/index.test.tsParsing error: "parserOptions.project" has been provided for Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
piecyk
left a comment
piecyk
left a comment
There was a problem hiding this comment.
Diagnosis is accurate and the fix is the right shape — cancel() in the shared observeOffset disposer covers both observers, needs no call-site changes, and is additive on a public util so patch is correct. Verified locally: virtual-core 129/129, react-virtual 7/7, tsc clean, and the new test does fail without the source change.
One thing to fix first: that debounce is the only writer of isScrolling = false — it's assigned in just one place, the scroll callback in _willUpdate().
That's harmless 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 now leaves isScrolling stuck true, where before the PR the zombie timer reset it to false. A stuck flag skips sync measurement in measureElement, strands iOS deferred adjustments in _flushIosDeferredIfReady, and leaves scrollDirection stale.
Could you add this to cleanup(), next to the existing scrollState = null, and cover the swap path in a test as well as unmount?
this.isScrolling = false
this.scrollDirection = nullAlso: packages/marko-virtual/e2e/app/e2e/option-gates.spec.ts has a waitForTimeout(250) with a note to remove it once a cancellable debounce lands. Worth dropping here — that's exactly the disable-within-150ms case above, so it doubles as verification.
Minor: the check rollup only shows CodeRabbit and Socket, so the test workflow doesn't look like it has run yet.
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.
|
Thanks — that's a real gap, and you're right about the mechanism. I went and checked rather than take it on trust: Added both resets in Three tests, one per way into I also dropped the Changeset now mentions the flag reset as well, since that's user-visible too. virtual-core 132/132, react-virtual 7/7, marko e2e 11/11, On the checks — you're right they haven't run. I can't trigger the workflow from a fork, so I think it needs an approval from your side. |
|
View your CI Pipeline Execution ↗ for commit 82eb0df
☁️ Nx Cloud last updated this comment at |
2 participants
Fixes #1255. The analysis in that issue is @Zinn-Digital-LTD's and it was accurate down to the line — they offered to send a patch, so if they'd rather own this, I'm glad to close in favour of theirs.
Problem
observeOffsetarms a debounce on everyscrollevent to resetisScrollingback tofalseafterisScrollingResetDelay(150 ms default). Its disposer removes the scroll/scrollend listeners but leaves that timer armed, and the handle is local to thedebounceclosure:so a consumer cannot clear it either — the fix has to live in the library.
Unmount a virtualizer inside that window and one late
(offset, false)callback still arrives, runningmaybeNotify()→ the consumer'sonChange→ in React auseReducerdispatch into a tree that no longer exists. A browser swallows that; under vitest/jsdom it surfaces as an update-after-unmount and can fail a suite that is otherwise correct.It's the default path rather than an edge case:
useScrollendEventdefaults tofalse, and jsdom has noonscrollend, so every jsdom-based suite goes through the debounce.Fix
debouncereturns a function carryingcancel(), and the disposer already returned byobserveOffsetcalls it alongside the listener removal. That disposer is pushed ontounsubs, whichcleanup()drains, so element and window observers are both covered and no call site changes.debounceis publicly exported, so this is additive — no existing signature changes.Testing
Added
observeElementOffset: cleanup drops the queued isScrolling reset, which scrolls, tears down inside the window, advances fake timers past the delay and asserts no further callback.Verified it fails for the right reason without the source change — the orphaned timer fires and the callback lands once (
Number of calls: 1) — and passes with it.vitest runinvirtual-core— 129/129 passtsc,eslint ./src,vite buildall cleanSummary by CodeRabbit
Bug Fixes
Tests