fix(Ballpit): bind resize/visibility handlers once so removeEventListener works - #1020
Conversation
There was a problem hiding this comment.
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
resizeandvisibilitychange. - 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.
|
Pushed the rebuilt registry artifacts. |
… 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.
601f611 to
e47ba52
Compare
The problem
Ballpitregisters two listeners in#initObserversand removes them in the teardown — but binds a fresh function each time:.bind()returns a new function object on every call, soremoveEventListeneris handed something that was never registered and removes nothing. Both listeners stay attached for the lifetime of the page, and each one keepsthisreachable — the Three.js scene, renderer and canvas with it.Anything that mounts
Ballpitmore 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,
Ballpitmounted then unmounted/remounted four times. InstrumentedaddEventListener/removeEventListenerto mirror the browser's own bookkeeping — a listener is keyed by(type, function), and aremovewith a different function object is a no-op, exactly as it is here:main)window: resizedocument: visibilitychangedocument: selectionchange(React's own — control)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:
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
npx tsc --noEmit: no new errors in either TS variant.npx vite build: passes.prettier --checkclean on all four files.Note for reproducing
On current
mainthe TS variants ofBallpitcan't be loaded at all in a stock Vite app —WebGLRendererParametersis imported as a value fromthree, which fails at both bundle and runtime (see #1018). To measure this leak I applied that one-linetypemodifier locally so the component would load. This PR is independent of #1018 and touches different lines; they merge in either order.