Skip to content

fix(Ballpit): bind resize/visibility handlers once so removeEventListener works - #1020

Open
noron12234 wants to merge 2 commits into
DavidHDev:mainfrom
noron12234:feat/fix-ballpit-listener-leak
Open

fix(Ballpit): bind resize/visibility handlers once so removeEventListener works#1020
noron12234 wants to merge 2 commits into
DavidHDev:mainfrom
noron12234:feat/fix-ballpit-listener-leak

Conversation

@noron12234

Copy link
Copy Markdown

The problem

Ballpit registers two listeners in #initObservers and removes them in the teardown — but binds a fresh function each time:

// setup
window.addEventListener('resize', this.#onResize.bind(this));
document.addEventListener('visibilitychange', this.#onVisibilityChange.bind(this));

// teardown
window.removeEventListener('resize', this.#onResize.bind(this));
document.removeEventListener('visibilitychange', this.#onVisibilityChange.bind(this));

.bind() returns a new function object on every call, so removeEventListener is handed something that was never registered and removes nothing. Both listeners stay attached for the lifetime of the page, and each one keeps this reachable — the Three.js scene, renderer and canvas with it.

Anything that mounts Ballpit more than once hits this: route changes in an SPA, a page that toggles backgrounds, or React StrictMode's double-mount in development.

Measurement

Stock Vite React-TS app, Ballpit mounted then unmounted/remounted four times. Instrumented addEventListener / removeEventListener to mirror the browser's own bookkeeping — a listener is keyed by (type, function), and a remove with a different function object is a no-op, exactly as it is here:

listener after 1 mount after 5 mounts (main) after 5 mounts (this PR)
window: resize 1 5 1
document: visibilitychange 1 5 1
document: selectionchange (React's own — control) 1 1 1

The control row is the point: an unrelated listener registered by React stays at 1 across the same cycles, so the growth is Ballpit's and not an artefact of the harness.

The change

Binds each handler once as a class field and uses that reference for both add and remove:

#boundResize = this.#onResize.bind(this);
#boundVisibilityChange = this.#onVisibilityChange.bind(this);

Private methods are installed on the instance before field initialisers run, so this is safe in the field position and needs no constructor changes.

Left alone deliberately: new ResizeObserver(this.#onResize.bind(this)) also binds inline, but observers are torn down with .disconnect(), which doesn't need the original reference — so there is no leak there and no reason to touch it.

Scope

All four variants, per the contributing guide — content, tailwind, ts-default, ts-tailwind. Only the two listener pairs and the new fields changed.

Verification

  • The measurement above, before and after.
  • npx tsc --noEmit: no new errors in either TS variant.
  • npx vite build: passes.
  • prettier --check clean on all four files.

Note for reproducing

On current main the TS variants of Ballpit can't be loaded at all in a stock Vite app — WebGLRendererParameters is imported as a value from three, which fails at both bundle and runtime (see #1018). To measure this leak I applied that one-line type modifier locally so the component would load. This PR is independent of #1018 and touches different lines; they merge in either order.

… removed

`#initObservers` registered the listeners with `this.#onResize.bind(this)` and
the teardown removed them with `this.#onResize.bind(this)` again. `.bind()`
returns a new function object every call, so `removeEventListener` was handed a
function that had never been registered and removed nothing.

Every mount therefore leaves a `resize` listener on `window` and a
`visibilitychange` listener on `document` attached for the lifetime of the page,
and each one keeps `this` alive — the Three.js scene, renderer and canvas with
it.

Binds both handlers once as class fields and uses those for add and remove.
Applied to all four variants.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes an event-listener teardown bug in the Ballpit background implementation where removeEventListener previously received a different function reference than the one registered (due to re-calling .bind()), preventing proper cleanup and causing listener accumulation across mounts.

Changes:

  • Introduces per-instance, pre-bound handler fields for resize and visibilitychange.
  • Updates listener registration and teardown to use the stable bound references across all four Ballpit variants.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/ts-tailwind/Backgrounds/Ballpit/Ballpit.tsx Store bound handler references once and reuse them for add/remove to ensure listeners are actually removed.
src/ts-default/Backgrounds/Ballpit/Ballpit.tsx Same fix as TS Tailwind variant to prevent leaked listeners across mounts.
src/tailwind/Backgrounds/Ballpit/Ballpit.jsx Same fix applied to the Tailwind JS variant using private bound fields.
src/content/Backgrounds/Ballpit/Ballpit.jsx Same fix applied to the Content JS variant using private bound fields.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

public/r/*.json embeds the component source verbatim and is what the jsrepo
CLI writes into a user's project, so a source-only fix still ships the
mismatched .bind() listeners. Regenerated with `npm run registry:build`.
@noron12234

Copy link
Copy Markdown
Author

Pushed the rebuilt registry artifacts. public/r/*.json embeds the component source verbatim and is what the jsrepo CLI writes into a user's project, so the source-only fix still shipped the broken version. Regenerated with npm run registry:build.

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.

2 participants