fix(frontend): listener / interval leaks in weight-slider, curation, grid-view#266
Open
lstein wants to merge 2 commits into
Open
fix(frontend): listener / interval leaks in weight-slider, curation, grid-view#266lstein wants to merge 2 commits into
lstein wants to merge 2 commits into
Conversation
…grid-view Three fragility items from the code review, all about long-lived listeners or intervals that outlive their useful scope. ## weight-slider: perma-listeners + no touch support ``WeightSlider.render()`` was installing two listeners on ``window`` (``mousemove`` + ``mouseup``) at construction time and never removing them. Each ``new WeightSlider(...)`` leaked one of each. The slider was also unusable on touch devices — no ``touchstart``/``touchmove``/ ``touchend`` path. Rewrites the drag plumbing so document-level move/end listeners are attached on ``mousedown`` (or ``touchstart``) that begins a drag and removed on the matching release/cancel — same per-drag pattern the ``makeDraggable`` helper introduced in PR #255 uses. Mirror the mouse pipeline for touch so the slider works on phones. ## curation: perma-setInterval(1000) at module init ``setupCurationCallbacks`` was kicking off ``setInterval(() => { … }, 1000)`` at module load time, then guarded the work inside with a "is the panel visible" check — so the timer fired forever (one wakeup per second for the life of the tab) even when the curation panel was closed and the work was a no-op. Lifts the interval into ``toggleCurationPanel`` — started when the panel opens, ``clearInterval``ed when it closes. The grid-highlight behavior is unchanged when the panel is open; when it's closed, no timer runs at all. ## grid-view: unawaited resetAllSlides → swiper race ``setupGridResizeHandler``'s debounced resize callback was calling ``this.resetAllSlides()`` (async, returns a Promise) without ``await`` and then immediately calling ``this.initializeGridSwiper()`` — which destroys + re-creates the swiper the in-flight reset was still using. Visible as occasional flickers + stale slide DOM after a window resize while the grid view was active. Just adds the ``await``. ## Not changed: umap.js plotDiv The code-review item flagged a stale-ref leak — ``plotDiv`` grabbed at module load with ``Plotly.newPlot`` "replacing the DOM" on each album switch. ``Plotly.newPlot(plotDiv, ...)`` actually replaces *children* of ``plotDiv``, not the element itself, so the module-level reference and the ``mouseleave`` listener on it both stay valid for the lifetime of the page. Confirmed against the templates: ``#umapPlot`` is rendered once and never removed. No fix needed. Two weight-slider tests dispatched ``mouseup``/``mousemove`` on ``window``; updated to ``document`` since the new code listens there instead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Three fragility items, all long-lived listeners or intervals that outlive their useful scope.
1. `weight-slider.js` — perma-listeners + no touch support
`WeightSlider.render()` was installing two listeners on `window` (`mousemove` + `mouseup`) at construction time and never removing them. Each `new WeightSlider(...)` leaked one of each. The slider was also unusable on touch devices — no `touchstart`/`touchmove`/`touchend` path.
The drag plumbing is now per-drag: document-level move/end listeners attach on `mousedown` (or `touchstart`) and remove on the matching release / cancel — same pattern the `makeDraggable` helper from PR #255 uses. Touch pipeline mirrors mouse so the slider works on phones.
2. `curation.js` — `setInterval(1000)` at module init, forever
`setupCurationCallbacks` kicked off a 1-second interval at module load time and then guarded the work inside with an "is the panel visible" check. So the timer fired forever for the life of the tab, even when the curation panel was closed and the work was a no-op every wakeup.
Lifts the interval into `toggleCurationPanel`: started on open, `clearInterval`ed on close. Behavior while the panel is open is unchanged; closed-panel CPU usage drops to zero.
3. `grid-view.js` — unawaited `resetAllSlides` → swiper race
`setupGridResizeHandler`'s debounced resize callback called `this.resetAllSlides()` (async) without `await`, then immediately called `this.initializeGridSwiper()` — which destroys + recreates the swiper the in-flight reset was still using. Visible as occasional flickers + stale slide DOM after resizing the window while the grid view is active.
Just adds the `await`.
Not changed: `umap.js` plotDiv
The code-review item flagged a stale-ref leak — `plotDiv` grabbed at module load, with `Plotly.newPlot` "replacing the DOM" on each album switch. `Plotly.newPlot(plotDiv, ...)` actually replaces children of `plotDiv`, not the element itself, so the module-level reference and the `mouseleave` listener on it stay valid for the page lifetime. Confirmed against the templates — `#umapPlot` is rendered once and never removed. No fix needed.
Test plan
Two weight-slider tests were dispatching `mouseup`/`mousemove` on `window`; updated to `document` since the new code listens there instead.
Net +62 lines across 4 files.
🤖 Generated with Claude Code