diff --git a/photomap/frontend/static/css/umap-floating-window.css b/photomap/frontend/static/css/umap-floating-window.css index e715d333..8f03c9dd 100644 --- a/photomap/frontend/static/css/umap-floating-window.css +++ b/photomap/frontend/static/css/umap-floating-window.css @@ -268,6 +268,17 @@ display: none; } +/* The Cluster Strength field holds something the album cannot be set to — a + half-typed number, or one below the floor the server would silently raise. + umap.js answers those by doing nothing, so without a mark a refused + keystroke and a saved one look the same. Set from JS rather than `:invalid` + so it never fires on a derived strength that simply exceeds the spinner's + display `max`, which is a real number the map is clustering with. */ +#umapEpsSpinner.umap-eps-unusable { + border: 1px solid #ff8080; + outline: none; +} + #umapClickBehaviorContainer, #umapMediaFilterContainer { font-size: 0.85em; diff --git a/photomap/frontend/static/javascript/umap.js b/photomap/frontend/static/javascript/umap.js index 018375e4..b8df54d4 100644 --- a/photomap/frontend/static/javascript/umap.js +++ b/photomap/frontend/static/javascript/umap.js @@ -241,6 +241,21 @@ export function applyResolvedEps(data) { const epsSpinner = document.getElementById("umapEpsSpinner"); if (epsSpinner && typeof data?.eps === "number") { epsSpinner.value = data.eps; + // Replacing the contents ends whatever edit was in there, guard included. + // Half-typed text holds the guard even across a blur, on purpose — but it + // cannot go on holding it once the text it was protecting is gone, or one + // abandoned "0." disables re-resolving for the rest of the session. + epsEditPending = false; + // A *stored* strength the map cannot cluster with is still marked. The + // spinner refuses to save one now, but versions before it did, and the + // config file is hand-editable — and an unmarked field is this module + // asserting the number is in effect when the server has floored it. A + // derived value is never marked, however small: floored or not, it is + // exactly what the map was clustered with. + markEpsUnusable( + !data.auto && !epsIsUsable(data.eps), + `The album stores ${data.eps}, which is below the ${epsFloor()} the map can cluster with.` + ); } setEpsAutoBadge(Boolean(data?.auto)); } @@ -254,46 +269,210 @@ function setEpsAutoBadge(isAuto) { // --- EPS Spinner Debounce --- let epsUpdateTimer = null; +// Whether the field holds an edit the album has not been told about yet. +// The debounce handle cannot answer this on its own: the handler refuses to +// arm a save for text the browser cannot parse yet ("0.", "-"), so during +// exactly the keystrokes where the user has the most to lose there is no +// timer for anyone to see. Cleared when the save lands, and on blur — text +// that never becomes a number would otherwise read as mid-edit forever. +let epsEditPending = false; +// Bumped on every keystroke in the field. A snapshot taken before an await +// that no longer matches on the way back means the user has typed since, and +// whatever that await resolved to is about to be written over their edit. +let epsEditSeq = 0; +// Saves in the air. The debounce handle is nulled the moment the save starts, +// and a save that started after the field was blurred leaves nothing else +// behind — so without this the round trip is a window in which the field +// looks idle to everything that writes into it, while the server it would be +// re-read from is precisely the one still waiting for this POST. +let epsSavesInFlight = 0; + +// Whether the Cluster Strength field belongs to a user who is part-way +// through changing it. Anything that would write into the spinner from the +// outside — a post-re-index re-resolve, a late reply to an earlier fetch — +// has to ask first, or it moves the number under the cursor. +function epsEditInProgress() { + return epsUpdateTimer !== null || epsEditPending || epsSavesInFlight > 0; +} + +// Show that the field holds something the album cannot be set to, since the +// handler's answer to one is to do nothing at all: without a mark, a refused +// keystroke and a saved one look identical. Only the values this module +// actually refuses are marked — `:invalid` would also catch a derived +// strength above the spinner's `max`, which is a legitimate number to show. +function markEpsUnusable(unusable, reason = "") { + const epsSpinner = document.getElementById("umapEpsSpinner"); + if (!epsSpinner) { + return; + } + epsSpinner.classList.toggle("umap-eps-unusable", unusable); + if (unusable) { + epsSpinner.title = reason; + } else { + epsSpinner.removeAttribute("title"); + } +} + document.getElementById("umapEpsSpinner").oninput = async () => { + // Drop any pending save first, and before every early return below: what + // the field holds now supersedes it. Leaving it armed lets a save from an + // earlier keystroke fire a second later carrying a number the field no + // longer shows. + if (epsUpdateTimer) { + clearTimeout(epsUpdateTimer); + epsUpdateTimer = null; + } + // Marked before the early returns, not after: a refused keystroke leaves an + // edit sitting in the field just as much as an accepted one does, and it is + // the refused ones that have no timer to stand in for them. + epsEditPending = true; + epsEditSeq++; // An empty field means "go back to deriving it" — otherwise the only way // out of a value you typed once would be to edit the config file. null is // sent verbatim; a numeric fallback here is what used to pin every album // to 0.07 the moment the field was cleared. + // + // But `type="number"` reports an empty value for anything it cannot parse + // *yet* — "0.", "-", "1e" — so an empty field alone cannot be read as the + // user asking for a derived strength. `validity.badInput` is what separates + // the two: it is set only while the input holds text the browser could not + // turn into a number, so a pause mid-keystroke no longer throws away the + // value the album was tuned to. (`Number.isNaN` cannot do this job: the + // sanitized value is "", never "NaN".) + if (document.getElementById("umapEpsSpinner").validity?.badInput) { + markEpsUnusable(true, "Not a number yet — the Cluster Strength is unchanged."); + return; + } const eps = readSpinnerEps(); - if (eps !== null && Number.isNaN(eps)) { - return; // mid-typing garbage ("-", "0.") — wait for something parseable + // Below the spinner's own `min` the server floors the value + // (MIN_CLUSTER_EPS in cluster_eps.py), so storing one leaves the map + // clustering at something other than the number on screen — and DBSCAN + // refuses a non-positive epsilon outright. The ceiling is deliberately not + // enforced: `max` is a display bound, and a derived strength for a small + // album can legitimately exceed it (see the spinner's markup), so refusing + // to save above it would strand exactly those albums. + if (eps !== null && !epsIsUsable(eps)) { + markEpsUnusable(true, `The Cluster Strength must be at least ${epsFloor()}.`); + return; } + markEpsUnusable(false); // Typing a number is what turns a derived strength into a chosen one, so // the badge goes immediately rather than after the debounced save. Clearing // the field keeps it until the derived value comes back below. if (eps !== null) { setEpsAutoBadge(false); } - if (epsUpdateTimer) { - clearTimeout(epsUpdateTimer); - } + // Pinned when the save is armed, not read when it fires: the user typed this + // number while looking at this album, and they are free to switch to another + // one inside the debounce window. `state.album` at fire time is whichever + // album they are looking at *then* — which is how a number typed for one + // album ends up stored on a different one. + const albumAtEdit = state.album; epsUpdateTimer = setTimeout(async () => { // Cleared as the save starts, not left holding a fired timer's handle: // it is what tells the rest of the module an edit is still pending, and // a stale handle would read as "forever mid-edit" after the first edit. epsUpdateTimer = null; - await fetch("set_umap_eps/", { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ album: state.album, eps }), - }); - if (eps === null) { - // Put the derived number back in the field before redrawing, so the - // map is fetched with the value the user can actually see. - await refreshResolvedEps(); + const seq = epsEditSeq; + epsSavesInFlight++; + try { + // Both failure modes are handled the same way, because they mean the + // same thing to the user: the album is not set to what the field shows. + // A rejection is the server being unreachable; !ok is most often the + // 403 from require_no_lock, which is to say "not while this album is + // being indexed" — a state the user is quite likely to be tuning in. + let saved = false; + try { + const response = await fetch("set_umap_eps/", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ album: albumAtEdit, eps }), + }); + saved = response.ok; + } catch (err) { + console.warn("Could not save the cluster strength:", err); + } + if (epsEditSeq === seq) { + // Nothing typed while the save was in the air, so this edit is over + // either way — cleared here rather than after the success check, or a + // failed save would leave the field reading as mid-edit forever and + // silently disable every re-resolve. If something *was* typed, that + // keystroke armed its own save and owns the flag from here. + epsEditPending = false; + } + // Everything below writes to the screen, so it belongs to the album on + // screen. If the user has moved on, the save has done its job for the + // album it was typed in and there is nothing here to say about it. + const stillShowingThisAlbum = state.album === albumAtEdit; + if (!saved) { + // Nothing was stored and nothing about the map changed, so leave the + // map alone — but say so on the field, which is now showing a number + // the album does not have. Only if it is still that field's number: a + // keystroke since, or another album since, and the mark would be + // describing something else. + if (stillShowingThisAlbum && epsEditSeq === seq) { + markEpsUnusable(true, "Could not be saved — the album still has its previous Cluster Strength."); + } + return; + } + if (!stillShowingThisAlbum) { + return; + } + if (eps === null) { + // Put the derived number back in the field before redrawing, so the + // map is fetched with the value the user can actually see. + // + // The sequence pinned above is passed rather than re-read: a keystroke + // during the save itself already means the derive is answering a + // question the user has moved on from. + await refreshResolvedEps(albumAtEdit, seq); + } + state.dataChanged = true; + // Not while the window is closed: Plotly would lay the plot out at zero + // size and consume the flag that the next open depends on to redraw. + // Leaving the flag set is what makes toggleUmapWindow refetch instead. + if (umapWindowIsOpen()) { + await fetchUmapData(); + } + } finally { + epsSavesInFlight--; } - state.dataChanged = true; - await fetchUmapData(); }, 1000); }; +// Leaving the field ends the edit — otherwise a value the handler refused, +// which arms no save that could clear the flag, would read as mid-edit for +// the rest of the session and quietly disable every re-resolve. +// +// Except while the field still shows text the browser cannot parse. Blur is +// not the user saying they are done: it fires on a click anywhere else and on +// an alt-tab, and the half-typed text goes on sitting there in the field +// afterwards, so releasing the guard then is just the original bug with extra +// steps. That leaves the guard held until the text is dealt with, which costs +// only the display refresh after a re-index: the map itself is redrawn with +// no cluster_eps at all, so it clusters at the album's own strength. +document.getElementById("umapEpsSpinner").onblur = () => { + if (!document.getElementById("umapEpsSpinner").validity?.badInput) { + epsEditPending = false; + } +}; + +// The floor the spinner will accept, read from the element so it cannot drift +// from the markup — which in turn mirrors MIN_CLUSTER_EPS in cluster_eps.py. +function epsFloor() { + const min = parseFloat(document.getElementById("umapEpsSpinner").min); + return Number.isFinite(min) && min > 0 ? min : 0; +} + +// Whether a number is one the album can actually be set to. +function epsIsUsable(eps) { + return Number.isFinite(eps) && eps > 0 && eps >= epsFloor(); +} + // The spinner's value as a number, or null when the field is empty. -// NaN means the field holds something not yet parseable. +// A `type="number"` element never yields anything else: it sanitizes what it +// cannot parse to "", which is why `validity.badInput` — not a NaN check — +// is what tells a half-typed number from a cleared field. function readSpinnerEps() { const raw = document.getElementById("umapEpsSpinner").value.trim(); return raw === "" ? null : parseFloat(raw); @@ -314,7 +493,22 @@ function umapWindowIsOpen() { // minutes, and the user is free to switch albums while it runs. Writing a // late reply into the shared spinner would show one album's number — and its // auto badge — against another album's map. -async function refreshResolvedEps(albumKey = state.album) { +// +// The edit sequence is pinned for the same reason and against the same clock. +// Clearing the field asks for a derived strength, and the derive that answers +// it can still be running a minute later — long enough for the user to change +// their mind and type a number, which saves and redraws on its own. Applying +// the late reply then puts the derived value and the "auto" badge back over a +// strength the album is actually storing. Callers that started asking before +// the round-trip — the debounced save — pass the sequence they pinned then, +// since a keystroke during the save counts as having moved on too. +async function refreshResolvedEps(albumKey = state.album, seq = epsEditSeq) { + if (epsEditSeq !== seq) { + // Stale before it is even asked. Deriving is real server CPU — seconds to + // minutes — so don't spend it on an answer that would be discarded on + // arrival by the check below. + return false; + } try { const response = await fetch("get_umap_eps/", { method: "POST", @@ -322,7 +516,7 @@ async function refreshResolvedEps(albumKey = state.album) { body: JSON.stringify({ album: albumKey }), }); const data = await response.json(); - if (!data.success || state.album !== albumKey) { + if (!data.success || state.album !== albumKey || epsEditSeq !== seq) { return false; } applyResolvedEps(data); @@ -347,7 +541,12 @@ export async function fetchUmapData() { // derives the strength. Substituting a number here would quietly cluster // at something the user never chose and the spinner never showed. const eps = readSpinnerEps(); - const epsQuery = eps !== null && !Number.isNaN(eps) ? `?cluster_eps=${eps}` : ""; + // A value the spinner refuses to save must not be sent either. The server + // floors anything under MIN_CLUSTER_EPS, so sending one clusters the map + // at a number that is neither what the album stores nor what the field + // shows — the exact divergence refusing to save it is meant to prevent. + // Omitting it is the honest request: the album's own strength applies. + const epsQuery = eps !== null && epsIsUsable(eps) ? `?cluster_eps=${eps}` : ""; const album = encodeURIComponent(state.album); // Fetch UMAP data and cluster labels in parallel. Labels are best-effort: // a failure leaves clusterLabels empty and the hover popup falls back to @@ -2303,8 +2502,10 @@ window.addEventListener("albumIndexUpdated", async (e) => { // Skipped while an edit is pending: the spinner belongs to whoever is // typing in it, and the number they are mid-way through is about to become // a stored one that no derived value can override. The debounce redraws - // against the new coordinates a moment later anyway. - if (epsUpdateTimer === null && !(await refreshResolvedEps(albumKey))) { + // against the new coordinates a moment later anyway. "Pending" cannot be + // read off the debounce handle alone — half-typed text arms no save at all, + // and that is the state with the most to lose — so ask epsEditInProgress(). + if (!epsEditInProgress() && !(await refreshResolvedEps(albumKey))) { console.warn("Redrawing the semantic map with the previous cluster strength."); } // Re-check rather than trust the checks above: resolving is a round-trip diff --git a/tests/backend/test_cluster_eps.py b/tests/backend/test_cluster_eps.py index 4ae46a81..87953ea8 100644 --- a/tests/backend/test_cluster_eps.py +++ b/tests/backend/test_cluster_eps.py @@ -7,6 +7,7 @@ fails on small albums. """ +import re from pathlib import Path import numpy as np @@ -225,3 +226,26 @@ def test_unwritable_cache_dir_still_returns_a_value(tmp_path, monkeypatch): lambda *a, **k: (_ for _ in ()).throw(OSError("read-only")), ) assert cached_adaptive_cluster_eps(blobs(), Path(tmp_path)) > 0 + + +def test_spinner_min_matches_the_floor_the_server_enforces(): + """The Cluster Strength spinner refuses what the server would floor. + + The frontend reads its floor off the input's own ``min`` attribute rather + than hardcoding a number, so the two can only drift here — and drifting + means either the spinner accepts a value the map then clusters at + something else, or it refuses one the server would have honored. + """ + template = ( + Path(__file__).parent.parent.parent + / "photomap" + / "frontend" + / "templates" + / "modules" + / "umap-floating-window.html" + ).read_text(encoding="utf-8") + spinner = template[template.index('id="umapEpsSpinner"') :] + spinner = spinner[: spinner.index(">")] + match = re.search(r'min="([^"]+)"', spinner) + assert match, "the Cluster Strength spinner has lost its min attribute" + assert float(match.group(1)) == pytest.approx(MIN_CLUSTER_EPS) diff --git a/tests/frontend/umap-eps-debounce.test.js b/tests/frontend/umap-eps-debounce.test.js new file mode 100644 index 00000000..0e360b07 --- /dev/null +++ b/tests/frontend/umap-eps-debounce.test.js @@ -0,0 +1,491 @@ +// The Cluster Strength debounce: what reaches set_umap_eps, and when. +// +// An empty field is a deliberate signal here -- it means "go back to a +// derived strength". The trap is that `` also reports an +// empty value for anything it cannot parse *yet*: "0.", "-", "1e". So a pause +// of one second while retyping looked exactly like asking for a derived +// value, and threw the album's tuned number away. +// +// `validity.badInput` is what separates them, and it is the one thing here +// jsdom cannot produce: it sets value to "" for unparseable input but never +// sets badInput (verified -- see `typeUnparseable`). The tests below drive it +// with a stub, so they pin THIS MODULE's logic; that badInput is really set by +// a browser for a half-typed number is a platform guarantee, not something +// this suite proves. +// +// See umap-harness.js for why umap.js needs a harness to be importable. +// +// This file re-imports umap.js per test, and nothing unregisters the window +// listeners a previous import left behind. So do not assert on what a +// `window.dispatchEvent` did here — every stale module answers it too, and the +// one with no edit pending will happily satisfy an assertion the module under +// test failed. Those cases belong in umap-reindex-refresh.test.js, which +// imports once. + +import { jest, describe, it, expect, beforeEach, afterEach } from "@jest/globals"; + +import { installFetchMock, installPlotlyMock, loadUmapDom, removePlotlyMock } from "./umap-harness.js"; + +const JS = "../../photomap/frontend/static/javascript"; + +const mockState = { + album: "test-album", + dataChanged: true, + autotaggingEnabled: false, + umapMediaFilter: "both", + umapShowLandmarks: false, + umapShowHoverThumbnails: false, + umapExitFullscreenOnSelection: false, + umapClickSelectsCluster: true, + umapControlsVisible: true, + umapClickSelectsImage: false, + searchType: "clear", + searchResults: [], +}; + +const setUmapMediaFilter = jest.fn((v) => { + mockState.umapMediaFilter = v; +}); +const setSearchResults = jest.fn(); +// Mutable so a test can put the swiper on a specific image. +const currentSlideIndex = [-1, 0, null]; + +jest.unstable_mockModule(`${JS}/state.js`, () => ({ + state: mockState, + setUmapMediaFilter, + setUmapShowLandmarks: jest.fn((v) => { + mockState.umapShowLandmarks = v; + }), + setUmapClickSelectsCluster: jest.fn(), + setUmapControlsVisible: jest.fn(), + setUmapExitFullscreenOnSelection: jest.fn(), + setUmapShowHoverThumbnails: jest.fn(), + saveSettingsToLocalStorage: jest.fn(), +})); +jest.unstable_mockModule(`${JS}/album-manager.js`, () => ({ + albumManager: { fetchAvailableAlbums: jest.fn(() => Promise.resolve([])), setSwiperManager: jest.fn() }, + checkAlbumIndex: jest.fn(), +})); +jest.unstable_mockModule(`${JS}/back-stack.js`, () => ({ + backStack: { markNextAsJump: jest.fn(), popOne: jest.fn(), init: jest.fn(), setNavigator: jest.fn() }, +})); +jest.unstable_mockModule(`${JS}/cluster-utils.js`, () => ({ + CLUSTER_PALETTE: ["#ff0000", "#00ff00", "#0000ff"], + getClusterLabelInfo: jest.fn(() => null), + getImageLabelInfo: jest.fn(() => null), + setClusterLabels: jest.fn(), + trackVocabBuildRequest: jest.fn((p) => p), +})); +jest.unstable_mockModule(`${JS}/search-ui.js`, () => ({ exitSearchMode: jest.fn() })); +jest.unstable_mockModule(`${JS}/search.js`, () => ({ + getImagePath: jest.fn(() => Promise.resolve("/photos/example.jpg")), + setSearchResults, +})); +jest.unstable_mockModule(`${JS}/settings.js`, () => ({ switchAlbum: jest.fn() })); +jest.unstable_mockModule(`${JS}/slide-state.js`, () => ({ + slideState: { navigateToIndex: jest.fn(), getCurrentSlide: jest.fn(() => ({ globalIndex: 0 })) }, + getCurrentSlideIndex: jest.fn(() => currentSlideIndex), +})); +jest.unstable_mockModule(`${JS}/umap-reindex.js`, () => ({ + checkUmapReindexOngoing: jest.fn(), + initUmapReindexButton: jest.fn(), +})); +jest.unstable_mockModule(`${JS}/utils.js`, () => ({ + // Real debounce, capped so the 500ms landmark coalescing doesn't dominate + // the suite's runtime. + debounce: (fn, delay) => { + const wait = Math.min(delay, 10); + let timer = null; + return function (...args) { + if (timer) { + clearTimeout(timer); + } + timer = setTimeout(() => fn.apply(this, args), wait); + }; + }, + getPercentile: (arr, p) => { + const sorted = [...arr].sort((a, b) => a - b); + return sorted[Math.floor(((p / 100) * (sorted.length - 1)) | 0)] ?? 0; + }, + isColorLight: () => false, + makeDraggable: jest.fn(), + showToast: jest.fn(), +})); + +const SAVE_DEBOUNCE_MS = 1000; + +const spinner = () => document.getElementById("umapEpsSpinner"); +const badge = () => document.getElementById("umapEpsAutoBadge"); + +/** Everything POSTed to set_umap_eps, in order. */ +let savedEps; +/** The full body of each of those POSTs, so a test can check the album. */ +let savedBodies; + +function installEpsFetchMock() { + savedEps = []; + savedBodies = []; + fetched = []; + global.fetch = (url, options) => { + const href = String(url); + fetched.push(href); + if (href.startsWith("set_umap_eps")) { + savedBodies.push(JSON.parse(options.body)); + savedEps.push(JSON.parse(options.body).eps); + return Promise.resolve({ ok: true, json: () => Promise.resolve({ success: true }) }); + } + if (href.startsWith("umap_data/")) { + return Promise.resolve({ ok: true, json: () => Promise.resolve([]) }); + } + return Promise.resolve({ ok: true, json: () => Promise.resolve({ success: true, eps: 0.35 }) }); + }; +} + +/** Type into the spinner the way a user does: set the value, fire input. */ +function type(value) { + const el = spinner(); + if (el.dataset.realValidity) { + // Whatever the browser could not parse is gone, and so is badInput. + Object.defineProperty(el, "validity", { value: realValidity.get(el), configurable: true }); + delete el.dataset.realValidity; + } + el.value = value; + el.dispatchEvent(new Event("input")); +} + +/** + * A copy of a ValidityState with badInput forced on. + * + * Spreading it would produce `{}` — every flag is a getter on the prototype, + * so none of them is an own enumerable property. `for...in` walks the + * prototype chain and reads each getter against the real object, which is + * what keeps the stub honest as umap.js starts consulting other flags. + */ +function validityWithBadInput(real) { + const copy = {}; + for (const key in real) { + copy[key] = real[key]; + } + copy.badInput = true; + copy.valid = false; + return copy; +} + +/** + * Put the field into the state a real browser reports for text it cannot turn + * into a number yet ("0.", "-", "1e"): value "" and validity.badInput set. + * + * jsdom does the first but not the second, so badInput is stubbed. The stub + * stays installed — a browser does not stop reporting badInput just because + * the keystroke is over, and the field goes on showing the unparseable text + * until something replaces it. `type()` takes it back off, the way entering a + * parseable value does. + */ +function typeUnparseable() { + const el = spinner(); + if (!el.dataset.realValidity) { + realValidity.set(el, el.validity); + el.dataset.realValidity = "stubbed"; + } + Object.defineProperty(el, "validity", { + value: validityWithBadInput(realValidity.get(el)), + configurable: true, + }); + el.value = ""; + el.dispatchEvent(new Event("input")); +} + +const realValidity = new WeakMap(); + +/** + * Run out the debounce. + * + * Fake timers rather than a real 1.15s wait per assertion: the debounce is a + * second long by design, and five of those waits cost more wall clock than + * the rest of the frontend suite put together. `advanceTimersByTimeAsync` + * drains the microtask queue between firings, so the fetch chain the timer + * starts finishes before this returns. + */ +const pastTheDebounce = () => jest.advanceTimersByTimeAsync(SAVE_DEBOUNCE_MS + 150); + +/** Whether the field is marked as holding a value that cannot be saved. */ +const isMarkedUnusable = () => spinner().classList.contains("umap-eps-unusable"); + +/** URLs fetched, in order — so a test can see what the map was asked for. */ +let fetched; + +let umap; + +describe("Cluster Strength debounce", () => { + beforeEach(async () => { + jest.clearAllMocks(); + jest.useFakeTimers(); + loadUmapDom(); + installPlotlyMock(); + installFetchMock([]); + umap = await import(`${JS}/umap.js`); + installEpsFetchMock(); + // The map is on screen: a save redraws it, which is the path every one of + // these tests should be running unless it says otherwise. + document.getElementById("umapFloatingWindow").style.display = "block"; + mockState.dataChanged = true; + }); + + afterEach(() => { + jest.clearAllTimers(); + jest.useRealTimers(); + removePlotlyMock(); + delete global.fetch; + document.body.innerHTML = ""; + jest.resetModules(); + }); + + it("does not clear the album's strength when the user pauses mid-number", async () => { + // The bug: "0." sanitizes to "", which is the deliberate-clear signal, + // so hesitating for a second threw away the tuned value and put the + // album back on a derived one. + type("0.4"); + await pastTheDebounce(); + savedEps.length = 0; + + typeUnparseable(); + await pastTheDebounce(); + + expect(savedEps).toEqual([]); + }); + + it("still treats a genuinely emptied field as a request to derive one", async () => { + // The other half: this must keep working, or clearing the field stops + // being a way back to an automatic strength. + type(""); + await pastTheDebounce(); + + expect(savedEps).toEqual([null]); + }); + + it("saves the number once the user finishes typing it", async () => { + typeUnparseable(); + type("0.4"); + await pastTheDebounce(); + + expect(savedEps).toEqual([0.4]); + }); + + it("does not let a pending save land after the field goes unparseable", async () => { + // The save armed by "0.4" must not fire a second later: the field no + // longer shows that number. This is why the timer is cleared before the + // early return rather than after it. + type("0.4"); + typeUnparseable(); + await pastTheDebounce(); + + expect(savedEps).toEqual([]); + }); + + it("refuses to save a strength DBSCAN cannot use", async () => { + // A stored 0 or negative leaves the map clustering at the floor while + // the spinner and the info modal both report the number the user typed. + type("0"); + await pastTheDebounce(); + type("-2"); + await pastTheDebounce(); + + expect(savedEps).toEqual([]); + }); + + it("refuses a strength the server would silently raise", async () => { + // Positive, but under the spinner's own min and the server's + // MIN_CLUSTER_EPS: storing it means the map clusters at 0.01 while the + // spinner and the cluster-info modal both report 0.005. + type("0.005"); + await pastTheDebounce(); + + expect(savedEps).toEqual([]); + }); + + it("saves a strength above the spinner's display max", async () => { + // The ceiling is not enforced: a derived strength for a small album can + // legitimately exceed it, and refusing those would leave the albums that + // need tuning most unable to be tuned. + type("4.5"); + await pastTheDebounce(); + + expect(savedEps).toEqual([4.5]); + }); + + it("marks the field while it holds something that cannot be saved", async () => { + // The handler answers a value it will not store by doing nothing, so + // without a mark a refused keystroke looks exactly like a saved one. + typeUnparseable(); + expect(isMarkedUnusable()).toBe(true); + expect(spinner().title).not.toBe(""); + + type("0.005"); + expect(isMarkedUnusable()).toBe(true); + + type("0.4"); + expect(isMarkedUnusable()).toBe(false); + expect(spinner().hasAttribute("title")).toBe(false); + + await pastTheDebounce(); + expect(savedEps).toEqual([0.4]); + }); + + it("does not put a derived strength back over a number typed since", async () => { + // Clearing the field asks the server to derive one, and on a large album + // that answer can be a minute coming — long enough for the user to change + // their mind and type a number, which saves and redraws on its own. + // Applying the late reply then shows a derived value and an "auto" badge + // for an album that is storing the user's number. + let releaseDerive; + const realFetch = global.fetch; + global.fetch = (url, options) => { + if (String(url).startsWith("get_umap_eps")) { + return new Promise((resolve) => { + releaseDerive = () => + resolve({ ok: true, json: () => Promise.resolve({ success: true, eps: 0.07, auto: true }) }); + }); + } + return realFetch(url, options); + }; + + type(""); + await pastTheDebounce(); + expect(savedEps).toEqual([null]); + + // The derive is still running; the user types a strength instead. + type("0.5"); + await pastTheDebounce(); + expect(savedEps).toEqual([null, 0.5]); + + releaseDerive(); + await jest.advanceTimersByTimeAsync(0); + + expect(spinner().value).toBe("0.5"); + expect(badge().hidden).toBe(true); + }); + + it("does not redraw the map when the save fails", async () => { + // A rejection is the server being unreachable. Nothing was stored, so + // redrawing would show the map at a strength the album does not have. + const realFetch = global.fetch; + global.fetch = (url, options) => + String(url).startsWith("set_umap_eps") ? Promise.reject(new Error("offline")) : realFetch(url, options); + + type("0.5"); + await pastTheDebounce(); + + expect(fetched.some((u) => u.startsWith("umap_data/"))).toBe(false); + expect(isMarkedUnusable()).toBe(true); + }); + + it("does not redraw the map when the server refuses the save", async () => { + // The refusal in practice is the 403 from require_no_lock: the album is + // being indexed, which is exactly when someone is likely to be tuning it. + const realFetch = global.fetch; + global.fetch = (url, options) => + String(url).startsWith("set_umap_eps") + ? Promise.resolve({ ok: false, status: 403, json: () => Promise.resolve({ detail: "locked" }) }) + : realFetch(url, options); + + type("0.5"); + await pastTheDebounce(); + + expect(fetched.some((u) => u.startsWith("umap_data/"))).toBe(false); + expect(isMarkedUnusable()).toBe(true); + }); + + it("leaves the redraw to the next window open when the save lands on a closed map", async () => { + // Plotly lays a plot out at zero size in a hidden container, and the + // redraw would consume the flag the next open depends on to refetch. + type("0.5"); + document.getElementById("umapFloatingWindow").style.display = "none"; + await pastTheDebounce(); + + expect(savedEps).toEqual([0.5]); + expect(fetched.some((u) => u.startsWith("umap_data/"))).toBe(false); + expect(mockState.dataChanged).toBe(true); + }); + + it("does not send a strength it refused to save", async () => { + // Refusing to store 0.005 but still asking the map for it gets the map + // clustered at the server's floor — the very divergence the refusal is + // there to prevent. + type("0.005"); + await pastTheDebounce(); + fetched.length = 0; + + mockState.dataChanged = true; + await umap.fetchUmapData(); + + expect(savedEps).toEqual([]); + const mapUrl = fetched.find((u) => u.startsWith("umap_data/")); + expect(mapUrl).toBe("umap_data/test-album"); + }); + + it("saves the number to the album it was typed in, not the one in view a second later", async () => { + // Switching albums inside the debounce window used to hand the number to + // whichever album `state.album` pointed at when the timer fired: album B + // silently acquired a Cluster Strength typed for album A, and nothing on + // screen said so until the next time B's map was opened. + type("0.4"); + mockState.album = "other-album"; + await pastTheDebounce(); + + expect(savedBodies).toEqual([{ album: "test-album", eps: 0.4 }]); + // And the other album's map is not redrawn with it. + expect(fetched.some((u) => u.startsWith("umap_data/"))).toBe(false); + }); + + it("marks a stored strength the map cannot cluster with", async () => { + // Versions before the spinner refused these could store them, and the + // config file is hand-editable. Leaving it unmarked is this module saying + // the number is in effect when the server has floored it. + umap.applyResolvedEps({ success: true, eps: 0.005, auto: false }); + + expect(spinner().value).toBe("0.005"); + expect(isMarkedUnusable()).toBe(true); + }); + + it("does not mark a derived strength, however small", async () => { + // A degenerate album can derive its way below the floor. The spinner + // would refuse the number, but it is the one the map is clustered with. + umap.applyResolvedEps({ success: true, eps: 9.3e-7, auto: true }); + + expect(isMarkedUnusable()).toBe(false); + }); + + it("does not put a derived strength back over a number typed during the save", async () => { + // The same race one step earlier: the keystroke lands while the clear is + // still being POSTed, before the derive has even been asked for. Reading + // the edit sequence when the derive starts would miss it, which is why + // the save pins the sequence it began with and passes that down. + let releaseSave; + let held = true; + const realFetch = global.fetch; + global.fetch = (url, options) => { + if (held && String(url).startsWith("set_umap_eps")) { + held = false; + return new Promise((resolve) => { + releaseSave = () => { + realFetch(url, options); + resolve({ ok: true, json: () => Promise.resolve({ success: true }) }); + }; + }); + } + return realFetch(url, options); + }; + + type(""); + await pastTheDebounce(); + + // The clear is in the air; the user changes their mind before it lands. + type("0.5"); + releaseSave(); + await pastTheDebounce(); + + expect(savedEps).toEqual([null, 0.5]); + expect(spinner().value).toBe("0.5"); + expect(badge().hidden).toBe(true); + }); +}); diff --git a/tests/frontend/umap-reindex-refresh.test.js b/tests/frontend/umap-reindex-refresh.test.js index 89e09aef..423c5ff3 100644 --- a/tests/frontend/umap-reindex-refresh.test.js +++ b/tests/frontend/umap-reindex-refresh.test.js @@ -95,6 +95,44 @@ const flushAsync = async () => { const reindexed = (albumKey) => window.dispatchEvent(new CustomEvent("albumIndexUpdated", { detail: { albumKey } })); +/** + * Type something the browser cannot parse into a number yet ("0.", "-"). + * + * A real number input reports value "" and sets validity.badInput; jsdom does + * the first but not the second, so the flag is stubbed — and left in place, + * because the field goes on showing the unparseable text after the keystroke + * and a browser goes on reporting badInput for it. `restoreValidity` takes it + * back off. See umap-eps-debounce.test.js for what the stub does and does not + * prove. + */ +function typeUnparseable() { + const el = spinner(); + const copy = {}; + for (const key in el.validity) { + copy[key] = el.validity[key]; + } + copy.badInput = true; + copy.valid = false; + if (!realValidity) { + realValidity = el.validity; + } + Object.defineProperty(el, "validity", { value: copy, configurable: true }); + el.value = ""; + el.dispatchEvent(new Event("input")); +} + +let realValidity = null; + +function restoreValidity() { + if (realValidity) { + Object.defineProperty(spinner(), "validity", { value: realValidity, configurable: true }); + realValidity = null; + } +} + +/** Blur the field, the way clicking anywhere else on the page does. */ +const blurField = () => spinner().dispatchEvent(new Event("blur")); + // A fetch mock whose get_umap_eps reply is held open until released, so a // test can act inside the window the real resolve leaves open. function deferredEpsFetch(eps) { @@ -149,8 +187,13 @@ beforeEach(() => { mockState.album = "test-album"; mockState.dataChanged = true; mapWindow().style.display = "block"; - // The map is showing a derived strength for an album that had none. + // The module is imported once for the whole file, so its idea of whether an + // edit is in progress outlives any single test. Put the field back to a + // parseable value and blur it, which is what clears that flag — cleanup a + // test does at its own end is cleanup a failing test skips. + restoreValidity(); umap.applyResolvedEps({ success: true, eps: 0.2, auto: true }); + blurField(); }); describe("albumIndexUpdated on the album being shown", () => { @@ -226,6 +269,138 @@ describe("albumIndexUpdated on the album being shown", () => { expect(calls.some((u) => u.startsWith("get_umap_eps"))).toBe(true); }); + + it("leaves the spinner alone while the user is mid-number", async () => { + // Half-typed text arms no save at all — that is the whole point of the + // badInput guard — so the debounce handle cannot stand in for "an edit is + // pending" here. This is the state with the most to lose: overwriting it + // moves the number under the cursor while the user is still typing it. + const calls = immediateFetch({ eps: 0.49 }); + typeUnparseable(); + + reindexed("test-album"); + await flushAsync(); + + expect(calls.some((u) => u.startsWith("get_umap_eps"))).toBe(false); + expect(spinner().value).toBe(""); + }); + + it("keeps holding the field when the user clicks away mid-number", async () => { + // Blur is not the user saying they are done: it fires on a click anywhere + // else and on an alt-tab, and the browser goes on showing the half-typed + // text afterwards. Releasing the field then would put the number back + // under a user who is about to carry on typing it. + const calls = immediateFetch({ eps: 0.49 }); + typeUnparseable(); + blurField(); + + reindexed("test-album"); + await flushAsync(); + + expect(calls.some((u) => u.startsWith("get_umap_eps"))).toBe(false); + expect(spinner().value).toBe(""); + }); + + it("resumes re-resolving once a refused value is left behind", async () => { + // A value the handler refuses arms no save that could ever clear the + // flag, so blur has to — one 0.005 must not disable every re-resolve for + // the rest of the session. + spinner().value = "0.005"; + spinner().dispatchEvent(new Event("input")); + blurField(); + + const calls = immediateFetch({ eps: 0.49 }); + reindexed("test-album"); + await flushAsync(); + + expect(calls.some((u) => u.startsWith("get_umap_eps"))).toBe(true); + expect(spinner().value).toBe("0.49"); + }); + + it("stops holding the field once its contents have been replaced", async () => { + // Half-typed text holds the guard across a blur on purpose. But the hold + // has to end when the text does: re-opening the map refills the field + // from the server, and a guard still set for a value that is no longer + // there disables every re-resolve for the rest of the session. + typeUnparseable(); + blurField(); + restoreValidity(); + umap.applyResolvedEps({ success: true, eps: 0.4, auto: false }); + + const calls = immediateFetch({ eps: 0.49 }); + reindexed("test-album"); + await flushAsync(); + + expect(calls.some((u) => u.startsWith("get_umap_eps"))).toBe(true); + expect(spinner().value).toBe("0.49"); + }); + + it("does not let a failed save leave the field reading as mid-edit", async () => { + // The flag that says "someone is typing in here" suppresses this + // re-resolve. Stranded on an error path — the save rejects, and the user + // has not clicked away, so no blur clears it — it would suppress every + // re-resolve for the rest of the session. + const calls = []; + global.fetch = (url) => { + calls.push(String(url)); + if (String(url).startsWith("set_umap_eps")) { + return Promise.reject(new Error("connection lost")); + } + if (String(url).startsWith("get_umap_eps")) { + return Promise.resolve({ ok: true, json: () => Promise.resolve({ success: true, eps: 0.49, auto: true }) }); + } + return Promise.resolve({ ok: true, json: () => Promise.resolve([]) }); + }; + + spinner().value = "0.5"; + spinner().dispatchEvent(new Event("input")); + await new Promise((resolve) => setTimeout(resolve, 1100)); + calls.length = 0; + + reindexed("test-album"); + await flushAsync(); + + expect(calls.some((u) => u.startsWith("get_umap_eps"))).toBe(true); + }); + + it("holds the field while the save it armed is still in the air", async () => { + // The debounce handle is dropped the moment the save starts, and a save + // that started after a blur leaves nothing else behind — so this window + // used to look idle while the server it would be re-read from was the one + // still waiting for the POST. Re-resolving inside it puts the pre-save + // value and the "auto" badge back over the number just stored. + let releaseSave; + const calls = []; + global.fetch = async (url) => { + calls.push(String(url)); + if (String(url).startsWith("set_umap_eps")) { + await new Promise((resolve) => { + releaseSave = resolve; + }); + return { ok: true, json: () => Promise.resolve({ success: true }) }; + } + if (String(url).startsWith("get_umap_eps")) { + return { ok: true, json: () => Promise.resolve({ success: true, eps: 0.2, auto: true }) }; + } + return { ok: true, json: () => Promise.resolve([]) }; + }; + + spinner().value = "0.5"; + spinner().dispatchEvent(new Event("input")); + blurField(); + await new Promise((resolve) => setTimeout(resolve, 1100)); + + // The POST is in the air. A re-index completing now must not re-resolve. + reindexed("test-album"); + await flushAsync(); + + expect(calls.some((u) => u.startsWith("get_umap_eps"))).toBe(false); + expect(spinner().value).toBe("0.5"); + expect(badge().hidden).toBe(true); + + releaseSave(); + await flushAsync(); + }); }); describe("while the post-re-index resolve is in flight", () => {