Repository navigation
fix(core): clear pending native-scroll velocity timeout on destroy() - #542
Open
cipherprofessor wants to merge 1 commit into
Open
cipherprofessor wants to merge 1 commit into
cipherprofessor wants to merge 1 commit into
Conversation
onNativeScroll schedules _resetVelocityTimeout (a 400ms setTimeout) whenever native-scroll velocity is nonzero, to reset velocity/isScrolling once the browser's native scroll settles. destroy() removed listeners and classes but never cleared that pending timeout. If destroy() runs while that timeout is still pending, its callback fires anyway ~400ms later and sets isScrolling = false. That setter's private updateClassName() runs unconditionally on any change, and className's getter always starts from 'lenis' -- so the base class gets re-added to rootElement well after the instance was destroyed and cleanUpClassName() already ran, including for the common case of destroying an instance to honor prefers-reduced-motion. Fix: clear _resetVelocityTimeout in destroy(), the same guard onNativeScroll already applies to itself at the top of its own body (clearing any previous pending timeout before scheduling a new one). Testing: this repo has no test framework (no vitest/jest, no CI test step -- confirmed by checking package.json scripts and searching for a test config). Verified with a standalone jsdom-based reproduction script exercising the actual, unmodified source end to end: minimal window/document polyfills (matchMedia, requestAnimationFrame, scrollTo/scrollY), autoResize disabled to avoid needing ResizeObserver, then the reporter's exact repro sequence (dispatch a native scroll, wait for the 'native' scrolling state to fire, destroy, check the class immediately and again after 600ms). Confirmed red against unmodified source (class returns after ~400ms) and green after the fix (class stays cleared). `bun run build` and `bunx biome check` are both clean. Fixes darkroomengineering#541
|
@cipherprofessor is attempting to deploy a commit to the Darkroom Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
The diff adds a null-guarded clearTimeout for _resetVelocityTimeout in destroy(). This prevents a pending native-scroll velocity reset from firing after teardown and re-adding the root class. The change is minimal, confined to packages/core/src/lenis.ts, and matches the existing guard pattern; no correctness, scope, or standards issues are visible in the supplied hunk.
Review coverage: 16/16 diff lines supplied. Inline comments are limited to fully visible, valid right-side hunks. Reviewed commit: 2e14ce85aa482d869799df4a25a872bb74400676.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #541.
Problem
onNativeScrollschedules_resetVelocityTimeout(a 400mssetTimeout) whenever native-scroll velocity is nonzero, to reset velocity/isScrollingonce the browser's native scroll settles.destroy()removed listeners and classes but never cleared that pending timeout.If
destroy()runs while that timeout is still pending, its callback fires anyway ~400ms later and setsisScrolling = false. That setter's privateupdateClassName()runs unconditionally on any change, andclassName's getter always starts from'lenis'— so the base class gets re-added torootElementwell after the instance was destroyed andcleanUpClassName()already ran. Includes the common case of destroying an instance to honorprefers-reduced-motion, as described in the issue.Fix
Clear
_resetVelocityTimeoutindestroy(), the same guardonNativeScrollalready applies to itself at the top of its own body (clearing any previous pending timeout before scheduling a new one).Testing
This repo has no test framework (no vitest/jest, no CI test step — confirmed by checking
package.jsonscripts and searching for a test config). Verified with a standalone jsdom-based reproduction script exercising the actual, unmodified source end to end: minimalwindow/documentpolyfills (matchMedia,requestAnimationFrame,scrollTo/scrollY),autoResizedisabled to avoid needingResizeObserver, then the reporter's exact repro sequence — dispatch a native scroll, wait for the'native'scrolling state to fire,destroy(), check the class immediately and again after 600ms.Confirmed red against unmodified source (class returns after ~400ms, matching the reporter's exact numbers) and green after the fix (class stays cleared).
bun run buildandbunx biome checkare both clean.