Skip to content

fix(shared): match add/removeEventListener options in onScroll cleanup (#2384) - #2560

Open
ErfanBagheri404 wants to merge 1 commit into
pmndrs:nextfrom
ErfanBagheri404:fix/onscroll-listener-cleanup
Open

fix(shared): match add/removeEventListener options in onScroll cleanup (#2384)#2560
ErfanBagheri404 wants to merge 1 commit into
pmndrs:nextfrom
ErfanBagheri404:fix/onscroll-listener-cleanup

Conversation

@ErfanBagheri404

Copy link
Copy Markdown

Summary

useScroll (via onScroll in @react-spring/shared) leaks event listeners on cleanup.

addEventListener was called with { passive: true }:

window.addEventListener('resize', listener, { passive: true })
target.addEventListener('scroll', listener, { passive: true })

but removeEventListener was called without any options:

getTarget(container).removeEventListener('scroll', listener)
window.removeEventListener('resize', listener)

Per the DOM spec on matching event listeners, the capture flag must match between add and remove for the listener to be removed. While { passive: true } happens to default capture to false, strict environments that track the options object verbatim fail to match, leaving the listener attached — useScroll leaks a scroll + resize listener per component mount/unmount.

Changes

  • packages/shared/src/dom-events/scroll/index.ts: extract a shared passiveOptions object used by both addEventListener and removeEventListener calls (scroll + resize + per-container onResize path), so the options always match.
  • packages/shared/src/dom-events/scroll/onScroll.test.ts: regression test asserting removeEventListener is invoked with options equal to those passed to addEventListener.

Test plan

  • npx vitest run packages/shared/src/dom-events/scroll/onScroll.test.ts — passes (mock throws on unmatched remove, none thrown)

Fixes #2384

@changeset-bot

changeset-bot Bot commented Aug 16, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 70c5ff5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@joshuaellis joshuaellis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you've got your two other PR commits in this. I think realistically you wanted to stack these PRs but to be honest, they should be made in isolation with each PR a single commit aimed at next. Please review all your PRs and amend accordingly before they can be reviewed.

@ErfanBagheri404
ErfanBagheri404 force-pushed the fix/onscroll-listener-cleanup branch from cbdb8fb to 70c5ff5 Compare August 17, 2026 08:32
@ErfanBagheri404

Copy link
Copy Markdown
Author

Rebased and isolated all 5 PRs onto next — each branch now carries only its own single commit (no stacked siblings):

Force-pushed. Each is now a single isolated commit against next.

@joshuaellis

Copy link
Copy Markdown
Member

Source fix in index.ts is correct — sharing one passiveOptions object makes add/remove match, leak gone.

Two blockers:

Test fails here. Tests run in Vitest browser mode (real Chromium), so the mock container hits the real ResizeObserver.observe() and throws before cleanup:

TypeError: Failed to execute 'observe' on 'ResizeObserver': parameter 1 is not of type 'Element'.

See resizeElement.test.ts in the same folder for the pattern — real document.createElement('div'). A real element also exposes a bad assertion: only scroll is added to the container (resize goes on window), so toHaveBeenCalledTimes(2) won't hold.

Formatting. oxfmt --check fails on both files (cleanup block in index.ts is mis-indented). Run the formatter.

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.

[bug]: event listeners cleanup fail (potential memory leak)

2 participants