From b966e96e8e94a2f3e6b91f9f5a278b8bdf59956e Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Thu, 21 May 2026 21:36:15 -0400 Subject: [PATCH] fix(frontend): listener / interval leaks in weight-slider, curation, grid-view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../frontend/static/javascript/curation.js | 43 ++++++++++--- .../frontend/static/javascript/grid-view.js | 5 +- .../static/javascript/weight-slider.js | 64 ++++++++++++++----- tests/frontend/weight-slider.test.js | 14 ++-- 4 files changed, 94 insertions(+), 32 deletions(-) diff --git a/photomap/frontend/static/javascript/curation.js b/photomap/frontend/static/javascript/curation.js index 4ea21294..0de7e968 100644 --- a/photomap/frontend/static/javascript/curation.js +++ b/photomap/frontend/static/javascript/curation.js @@ -20,6 +20,34 @@ const lowFreqIndices = new Set(); // < 70% // Metadata Map for Persistent CSV Export (Index -> {filename, subfolder, frequency, count}) const globalMetadataMap = new Map(); +// Grid-highlight refresh interval. Runs only while the curation panel is +// visible — previously this was a perma-setInterval(1000) started at module +// init time that woke up every second forever, even with the panel closed. +let _gridHighlightInterval = null; + +function _startGridHighlightInterval() { + if (_gridHighlightInterval !== null) { + return; + } + _gridHighlightInterval = setInterval(() => { + const gridContainer = document.getElementById("gridViewContainer"); + if ( + (currentSelectionIndices.size > 0 || excludedIndices.size > 0) && + gridContainer && + gridContainer.style.display !== "none" + ) { + applyGridHighlights(); + } + }, 1000); +} + +function _stopGridHighlightInterval() { + if (_gridHighlightInterval !== null) { + clearInterval(_gridHighlightInterval); + _gridHighlightInterval = null; + } +} + window.toggleCurationPanel = function () { const panel = document.getElementById("curationPanel"); if (panel) { @@ -36,9 +64,11 @@ window.toggleCurationPanel = function () { backStack.markNextAsJump("curation"); slideState.navigateToIndex(index, false); }); + _startGridHighlightInterval(); } else { // When panel is closed, restore default cluster selection behavior setUmapClickCallback(null); + _stopGridHighlightInterval(); } // Force update of current image marker (yellow dot) to show it @@ -570,16 +600,9 @@ function setupEventListeners() { }; } - setInterval(() => { - const gridContainer = document.getElementById("gridViewContainer"); - if ( - (currentSelectionIndices.size > 0 || excludedIndices.size > 0) && - gridContainer && - gridContainer.style.display !== "none" - ) { - applyGridHighlights(); - } - }, 1000); + // (The grid-highlight refresh interval is now driven by ``toggleCurationPanel`` + // so it only runs while the panel is actually visible.) + // Listen for UMAP redraws (e.g. Cluster Strength change) to restore curation highlights window.addEventListener("umapRedrawn", () => { const panel = document.getElementById("curationPanel"); diff --git a/photomap/frontend/static/javascript/grid-view.js b/photomap/frontend/static/javascript/grid-view.js index 26b62b30..c2269b98 100644 --- a/photomap/frontend/static/javascript/grid-view.js +++ b/photomap/frontend/static/javascript/grid-view.js @@ -566,7 +566,10 @@ class GridViewManager { if (this.gridGeometryChanged(newGeometry)) { const currentGlobalIndex = slideState.getCurrentSlide().globalIndex; - this.resetAllSlides(); + // ``resetAllSlides`` is async — without awaiting, the next line's + // ``initializeGridSwiper`` would destroy the swiper while the reset + // was still mid-await, leading to flickers and stale slide DOM. + await this.resetAllSlides(); this.initializeGridSwiper(); this.setBatchLoading(true); await this.loadBatch(currentGlobalIndex); diff --git a/photomap/frontend/static/javascript/weight-slider.js b/photomap/frontend/static/javascript/weight-slider.js index b5b20da5..8081cfbe 100644 --- a/photomap/frontend/static/javascript/weight-slider.js +++ b/photomap/frontend/static/javascript/weight-slider.js @@ -46,25 +46,57 @@ export class WeightSlider { this.setValueFromEvent(e); }); - // Drag to set value - this.bar.addEventListener("mousedown", (e) => { - this.isDragging = true; - this.setValueFromEvent(e); - document.body.style.userSelect = "none"; - }); + // Drag to set value. Document-level move/up listeners are added on the + // mousedown (or touchstart) that begins a drag and removed on release, + // instead of being installed perma-listening at render time — multiple + // slider instances used to leak one mousemove + one mouseup listener + // each onto window, and the slider was unusable on touch devices. + this.bar.addEventListener("mousedown", (e) => this._beginDrag(e, false)); + this.bar.addEventListener( + "touchstart", + (e) => { + if (e.touches.length !== 1) { + return; + } + this._beginDrag(e.touches[0], true); + e.preventDefault(); + }, + { passive: false } + ); + } - window.addEventListener("mousemove", (e) => { - if (this.isDragging) { - this.setValueFromEvent(e); - } - }); + _beginDrag(event, isTouch) { + this.isDragging = true; + this.setValueFromEvent(event); + document.body.style.userSelect = "none"; - window.addEventListener("mouseup", () => { - if (this.isDragging) { - this.isDragging = false; - document.body.style.userSelect = ""; + const onMove = (e) => { + if (!this.isDragging) { + return; } - }); + const point = isTouch && e.touches ? e.touches[0] : e; + this.setValueFromEvent(point); + if (isTouch) { + e.preventDefault(); + } + }; + + const onEnd = () => { + this.isDragging = false; + document.body.style.userSelect = ""; + document.removeEventListener(isTouch ? "touchmove" : "mousemove", onMove); + document.removeEventListener(isTouch ? "touchend" : "mouseup", onEnd); + document.removeEventListener("touchcancel", onEnd); + }; + + if (isTouch) { + document.addEventListener("touchmove", onMove, { passive: false }); + document.addEventListener("touchend", onEnd); + document.addEventListener("touchcancel", onEnd); + } else { + document.addEventListener("mousemove", onMove); + document.addEventListener("mouseup", onEnd); + } } setValueFromEvent(e) { diff --git a/tests/frontend/weight-slider.test.js b/tests/frontend/weight-slider.test.js index 764bea38..4a572fc8 100644 --- a/tests/frontend/weight-slider.test.js +++ b/tests/frontend/weight-slider.test.js @@ -227,13 +227,14 @@ describe("weight-slider.js", () => { width: 100, })); - // Start drag + // Start drag — listeners are now attached on document (and only while + // dragging) instead of perma-bound to window. const mousedownEvent = new MouseEvent("mousedown", { clientX: 50 }); bar.dispatchEvent(mousedownEvent); // End drag const mouseupEvent = new MouseEvent("mouseup"); - window.dispatchEvent(mouseupEvent); + document.dispatchEvent(mouseupEvent); expect(slider.isDragging).toBe(false); }); @@ -256,7 +257,7 @@ describe("weight-slider.js", () => { // Move mouse const mousemoveEvent = new MouseEvent("mousemove", { clientX: 70 }); - window.dispatchEvent(mousemoveEvent); + document.dispatchEvent(mousemoveEvent); expect(slider.value).toBe(0.7); expect(onChangeMock).toHaveBeenCalledWith(0.7); @@ -270,9 +271,12 @@ describe("weight-slider.js", () => { width: 100, })); - // Move mouse without starting drag + // Move mouse without starting drag — the new code attaches the + // mousemove listener only on mousedown, so this dispatch is a no-op + // (preserves the test's intent that ``value`` stays at its initial + // setting). const mousemoveEvent = new MouseEvent("mousemove", { clientX: 70 }); - window.dispatchEvent(mousemoveEvent); + document.dispatchEvent(mousemoveEvent); expect(slider.value).toBe(0.5); expect(onChangeMock).not.toHaveBeenCalled();