Skip to content

fix(useScrolling): clear pending timeout during cleanup - #2740

Open
wanxiankai wants to merge 1 commit into
streamich:masterfrom
wanxiankai:fix/2675-scrolling-timeout-cleanup
Open

wanxiankai wants to merge 1 commit into
streamich:masterfrom
wanxiankai:fix/2675-scrolling-timeout-cleanup

Conversation

@wanxiankai

Copy link
Copy Markdown

Description

useScrolling leaves its 150ms debounce timeout pending when the component unmounts or the ref object changes. The timeout can then update an unmounted component.

Clear the timeout in the effect cleanup and capture the subscribed element so the scroll listener is removed even when React has cleared the ref or it points to a different element. Keep the existing 150ms debounce behavior.

Fixes #2675.

Add seven tests covering the debounce interval, repeated scroll events, a missing element, unmount with and without a pending timeout, React clearing a real DOM ref, and ref replacement. Four of these tests fail on the original implementation. The updated hook has 100% statement, branch, function, and line coverage.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as before)

Checklist

  • Read the Contributing Guide
  • Perform a code self-review
  • Comment the code, particularly in hard-to-understand areas β€” the cleanup is straightforward and needs no additional inline comments.
  • Add documentation β€” describe the debounce interval and cleanup behavior in the existing hook documentation.
  • Add hook's story at Storybook β€” not applicable to this lifecycle fix; the existing Sensors/useScrolling story continues to cover the public API.
  • Cover changes with tests
  • Ensure the test suite passes (yarn test) β€” 77 suites, 499 tests.
  • Provide 100% tests coverage β€” all four coverage metrics for src/useScrolling.ts.
  • Make sure code lints (yarn lint). Fix it with yarn lint:fix in case of failure. β€” 0 errors; 96 warnings elsewhere in the existing codebase. The changed source and test files pass ESLint without warnings.
  • Make sure types are fine (yarn lint:types).

Additional validation: yarn build (CommonJS and ES modules), changed-file Prettier check using the repository's ESLint formatting options, and git diff --check all pass.

Prepared with AI assistance.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

useScrolling doesn't clean up its timer

1 participant