From 2ba365810c6fe9c5df7064840435911ea5da86de Mon Sep 17 00:00:00 2001 From: Raymond Yee Date: Sat, 9 May 2026 11:35:02 -0700 Subject: [PATCH 1/3] explorer: extract freshSelectionToken() primitive (closes #187 step 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six rounds of Codex review on PR #186 surfaced the same class of bug eight times: async work mutating selection state without checking that a newer user event had already superseded it. The duct-tape was a `_selGen` counter inlined at three call sites with slightly different shapes (filter handler, hashchange handler, boot deep-link). Each site bumped the counter, captured the snapshot, and rolled its own stale-check arrow. Extract the pattern as `freshSelectionToken()` in zoomWatcher (~10 lines): bumps `viewer._selGen`, returns an `isStale()` closure. The four call sites now just do: const isStale = freshSelectionToken(); ... await someWork(); if (isStale()) return; — with `isStale` passed into hydrateClusterUI's existing predicate parameter for nested-await freshness. Behavior-preserving refactor: verified locally that the three round-trips from PR #186's verification (boot at #h3=, uncheck non-cluster source, uncheck cluster's own source) produce identical results. Also documents the async-selection invariant explicitly in EXPLORER_STATE.md §4 (a new row for `viewer._selGen` plus an "Async-selection invariant" subsection) so future selection-touching code finds the rule before tripping the wire. Per #187 conversation between Claude Code and Codex: Step 2 of that issue (consolidating selection mutations into a `selectSample` / `selectCluster` / `clearSelection` controller) is YAGNI-deferred until the next feature actually has to touch click + boot + hashchange + filter + URL paths. A wholesale state-machine rewrite is not justified. Refs #187, #186. Co-Authored-By: Claude Opus 4.7 (1M context) --- EXPLORER_STATE.md | 7 ++++++ explorer.qmd | 64 +++++++++++++++++++++++++++++++---------------- 2 files changed, 49 insertions(+), 22 deletions(-) diff --git a/EXPLORER_STATE.md b/EXPLORER_STATE.md index 923fd9c..adba611 100644 --- a/EXPLORER_STATE.md +++ b/EXPLORER_STATE.md @@ -111,8 +111,15 @@ across cells without becoming a separate OJS reactive value. | `viewer.h3Points` | Cesium `PointPrimitiveCollection` | `:815` | cluster mode rendering | | | `viewer.samplePoints` | Cesium `PointPrimitiveCollection` | `:818` | point mode rendering | | | `viewer.pointLabel` | Cesium label entity | `:823` | mouse-move handler (`:836-848`) | hover tooltip | +| `viewer._selGen` | int | bumped by every `freshSelectionToken()` call (in zoomWatcher cell) | snapshot captured by each handler that mutates selection | freshness counter; see invariant below | | `window.refreshSamplesTable` | `() => Promise` | `:1238` | external (debug / Playwright) | not used by other cells; safe to keep or remove | +### Async-selection invariant + +Any async work that updates `viewer._globeState`, the URL hash, or the side-panel DOM **must check freshness after every await**. The `freshSelectionToken()` helper (defined in the zoomWatcher cell) is the primitive: each user-input event handler that touches selection (cluster/sample click, hashchange, source-filter toggle, boot deep-link) calls it once at start to bump `_selGen` and capture an `isStale()` closure; every subsequent await is followed by `if (isStale()) return;` before any state/URL/DOM mutation. Pass `isStale` into nested helpers (`hydrateClusterUI`'s second param) so their internal awaits also bail before touching the DOM. + +This invariant exists because there's no central "selection store" — selection state lives in `_globeState`, the URL hash, and the side-panel DOM, and four different paths (click, hashchange, filter, boot) write to all three. Without the freshness check, a slow earlier handler can repaint the side panel for a selection the user has already moved off of. Issue #187 has the post-mortem on the 6-round Codex review that motivated extracting the primitive. + ### `_urlParamsHydrated` — confirmed gone Grep for `_urlParamsHydrated` in `explorer.qmd` returns no hits. The flag from diff --git a/explorer.qmd b/explorer.qmd index 686d23a..dadacf5 100644 --- a/explorer.qmd +++ b/explorer.qmd @@ -1305,6 +1305,29 @@ zoomWatcher = { let requestId = 0; // stale-request guard // clusterDataCache stored on viewer._clusterData (set by phase1 and loadRes) + // --- Selection freshness primitive --- + // + // Async work that updates `viewer._globeState`, the URL hash, or the side + // panel must check freshness after every await. A user input (click, + // hashchange, source-filter toggle, boot deep-link) bumps `_selGen`; any + // in-flight async work whose generation no longer matches must NOT mutate + // anything that an interactive newer event has already moved. + // + // Usage: + // const isStale = freshSelectionToken(); + // await someWork(); + // if (isStale()) return; + // updateDOM(); + // + // Pass `isStale` into helpers (see hydrateClusterUI) so their internal + // awaits also bail before touching DOM. See issue #187 for the post-mortem + // that motivated extracting this primitive. + function freshSelectionToken() { + viewer._selGen = (viewer._selGen || 0) + 1; + const gen = viewer._selGen; + return () => gen !== viewer._selGen; + } + // Hysteresis thresholds to avoid flicker const ENTER_POINT_ALT = 120000; // 120 km → enter point mode const EXIT_POINT_ALT = 180000; // 180 km → exit point mode @@ -1699,8 +1722,7 @@ zoomWatcher = { // any in-flight selection lookup AND re-validate the current selection // (cluster or sample) under the new filter — if it's filtered out, drop // it from runtime state and the URL so the side panel matches the globe. - viewer._selGen = (viewer._selGen || 0) + 1; - const filterSelGen = viewer._selGen; + const isStale = freshSelectionToken(); try { updateSourceLegendState(); writeQueryState(); @@ -1714,11 +1736,11 @@ zoomWatcher = { refreshFacetCounts(); // Re-validate selection (only if no newer filter change has fired). - if (filterSelGen === viewer._selGen) { + if (!isStale()) { const sel = viewer._globeState; if (sel.selectedH3) { const meta = await fetchClusterByH3(sel.selectedH3); - if (filterSelGen !== viewer._selGen) return; + if (isStale()) return; if (!meta) { sel.selectedH3 = null; updateClusterCard(null); @@ -1731,7 +1753,7 @@ zoomWatcher = { // source-filtered too — re-run it under the new filter // so the panel doesn't show stale rows from unchecked // sources (or miss newly-checked ones). - await hydrateClusterUI(meta, () => filterSelGen !== viewer._selGen); + await hydrateClusterUI(meta, isStale); } } else if (sel.selectedPid) { const safe = sel.selectedPid.replace(/'/g, "''"); @@ -1741,7 +1763,7 @@ zoomWatcher = { ${sourceFilterSQL('source')} LIMIT 1 `); - if (filterSelGen !== viewer._selGen) return; + if (isStale()) return; if (!stillVisible || stillVisible.length === 0) { sel.selectedPid = null; updateClusterCard(null); @@ -1930,7 +1952,7 @@ zoomWatcher = { window.addEventListener('hashchange', async () => { // Bump the selection generation BEFORE any early-return so even // hashchanges that lack lat/lng invalidate stale async work. - viewer._selGen = (viewer._selGen || 0) + 1; + const isStale = freshSelectionToken(); const state = readHash(); if (state.lat == null || state.lng == null) return; @@ -1958,9 +1980,8 @@ zoomWatcher = { // EXPLORER_CLUSTER_URL_PROPOSAL §4). Both branches do an `await` against // a remote parquet, so a fast back/forward could race: an older fetch // resolves AFTER a newer hash has applied, and would otherwise repaint - // the side panel with stale data. `_selGen` is bumped at the very top - // of this handler; we capture it here and check after each await. - const selGen = viewer._selGen; + // the side panel with stale data. `isStale` is the freshness token + // captured at the top of this handler; we check it after each await. if (state.pid) { viewer._globeState.selectedPid = state.pid; viewer._globeState.selectedH3 = null; @@ -1971,7 +1992,7 @@ zoomWatcher = { WHERE pid = '${state.pid.replace(/'/g, "''")}' LIMIT 1 `); - if (selGen !== viewer._selGen) return; + if (isStale()) return; if (sample && sample.length > 0) { const s = sample[0]; updateSampleCard({ @@ -1987,10 +2008,10 @@ zoomWatcher = { viewer._globeState.selectedPid = null; viewer._globeState.selectedH3 = state.h3.toLowerCase(); const meta = await fetchClusterByH3(state.h3); - if (selGen !== viewer._selGen) return; + if (isStale()) return; if (meta) { viewer._globeState.selectedH3 = meta.h3_cell; // canonical lowercase - await hydrateClusterUI(meta, () => selGen !== viewer._selGen); + await hydrateClusterUI(meta, isStale); } else { // Unknown / malformed h3 — clear the side panel rather than // leaving prior content stranded. @@ -2337,14 +2358,13 @@ zoomWatcher = { // Sample mode wins if both &pid= and &h3= are present (see EXPLORER_CLUSTER_URL_PROPOSAL §4). // The boot path runs once, but the hashchange listener is already registered // by this point — back/forward or a manual hash edit during the boot await - // could supersede this lookup. Use the same `_selGen` token the hashchange - // handler uses; bumping it here also invalidates any in-flight lookups. + // could supersede this lookup. Capture the same freshness token the + // hashchange handler uses; bumping it here also invalidates any in-flight + // lookups. // Wrap in try/finally so `_suppressHashWrite = false` always runs even if // a stale early-return aborts the deep-link work — otherwise a no-lat/lng // hashchange during boot could leave hash writes suppressed forever. - viewer._selGen = (viewer._selGen || 0) + 1; - const bootSelGen = viewer._selGen; - const isBootStale = () => bootSelGen !== viewer._selGen; + const isStale = freshSelectionToken(); const ih = viewer._initialHash; try { if (ih.pid) { @@ -2357,7 +2377,7 @@ zoomWatcher = { WHERE pid = '${ih.pid.replace(/'/g, "''")}' LIMIT 1 `); - if (isBootStale()) return "active"; + if (isStale()) return "active"; if (sample && sample.length > 0) { const s = sample[0]; updateSampleCard({ @@ -2370,7 +2390,7 @@ zoomWatcher = { WHERE pid = '${ih.pid.replace(/'/g, "''")}' LIMIT 1 `); - if (isBootStale()) return "active"; + if (isStale()) return "active"; if (detail && detail.length > 0) updateSampleDetail(detail[0]); else updateSampleDetail({ description: '' }); } @@ -2380,10 +2400,10 @@ zoomWatcher = { } else if (ih.h3) { viewer._globeState.selectedH3 = ih.h3.toLowerCase(); const meta = await fetchClusterByH3(ih.h3); - if (isBootStale()) return "active"; + if (isStale()) return "active"; if (meta) { viewer._globeState.selectedH3 = meta.h3_cell; // canonical lowercase - await hydrateClusterUI(meta, isBootStale); + await hydrateClusterUI(meta, isStale); } else { // Unknown / malformed h3, OR filtered out by ?sources=. Drop it // from runtime state so buildHash() doesn't keep emitting it. From 797605a0e1ec38f25d9b971d877e6aa49fda0761 Mon Sep 17 00:00:00 2001 From: Raymond Yee Date: Sat, 9 May 2026 12:22:29 -0700 Subject: [PATCH 2/3] explorer: extend freshness primitive to viewer-cell click handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex's review of PR #188 caught a P2 inconsistency: EXPLORER_STATE.md's async-selection invariant claimed coverage of cluster/sample click handlers, but freshSelectionToken() was defined inside the zoomWatcher cell — the click handler lives in the earlier viewer cell and couldn't see it. The click handler still had two unguarded awaits (sample detail load, cluster nearby-samples load) where a slow earlier click could repaint the side panel after the user clicked a different sample/cluster. Fix: move `freshSelectionToken(v)` to top-level OJS scope alongside `readHash` / `buildHash` so both cells can reach it. Take the viewer as a parameter (rather than closing over it) since the function is now defined before the viewer cell runs. Apply `isStale()` checks at the click handler's two await sites: sample-detail post-await and pre-DOM-write, cluster-nearby same. Doc adjusted to reflect the top-level definition. Behavior-preserving regression check: identical results to PR #186's verification suite (boot at #h3=, unchecked-non-cluster source, unchecked cluster source). Refs #187 (post-mortem), #188 (origin refactor PR — Codex's review of that diff surfaced this). Co-Authored-By: Claude Opus 4.7 (1M context) --- EXPLORER_STATE.md | 2 +- explorer.qmd | 64 ++++++++++++++++++++++++++++------------------- 2 files changed, 39 insertions(+), 27 deletions(-) diff --git a/EXPLORER_STATE.md b/EXPLORER_STATE.md index adba611..e9ff509 100644 --- a/EXPLORER_STATE.md +++ b/EXPLORER_STATE.md @@ -116,7 +116,7 @@ across cells without becoming a separate OJS reactive value. ### Async-selection invariant -Any async work that updates `viewer._globeState`, the URL hash, or the side-panel DOM **must check freshness after every await**. The `freshSelectionToken()` helper (defined in the zoomWatcher cell) is the primitive: each user-input event handler that touches selection (cluster/sample click, hashchange, source-filter toggle, boot deep-link) calls it once at start to bump `_selGen` and capture an `isStale()` closure; every subsequent await is followed by `if (isStale()) return;` before any state/URL/DOM mutation. Pass `isStale` into nested helpers (`hydrateClusterUI`'s second param) so their internal awaits also bail before touching the DOM. +Any async work that updates `viewer._globeState`, the URL hash, or the side-panel DOM **must check freshness after every await**. The `freshSelectionToken(viewer)` helper (defined at top level alongside `readHash` / `buildHash` so both the viewer-cell click handler and the zoomWatcher-cell handlers can reach it) is the primitive: each user-input event handler that touches selection (cluster/sample click, hashchange, source-filter toggle, boot deep-link) calls it once at start to bump `_selGen` and capture an `isStale()` closure; every subsequent await is followed by `if (isStale()) return;` before any state/URL/DOM mutation. Pass `isStale` into nested helpers (`hydrateClusterUI`'s second param) so their internal awaits also bail before touching the DOM. This invariant exists because there's no central "selection store" — selection state lives in `_globeState`, the URL hash, and the side-panel DOM, and four different paths (click, hashchange, filter, boot) write to all three. Without the freshness check, a slow earlier handler can repaint the side panel for a selection the user has already moved off of. Issue #187 has the post-mortem on the 6-round Codex review that motivated extracting the primitive. diff --git a/explorer.qmd b/explorer.qmd index dadacf5..0566aa0 100644 --- a/explorer.qmd +++ b/explorer.qmd @@ -649,6 +649,30 @@ function buildHash(v) { return '#' + params.toString(); } +// === Selection freshness primitive === +// +// Async work that updates `viewer._globeState`, the URL hash, or the side +// panel must check freshness after every await. A user input (click, +// hashchange, source-filter toggle, boot deep-link) bumps `_selGen`; any +// in-flight async work whose generation no longer matches must NOT mutate +// anything that an interactive newer event has already moved. +// +// Usage: +// const isStale = freshSelectionToken(viewer); +// await someWork(); +// if (isStale()) return; +// updateDOM(); +// +// Pass `isStale` into helpers (see hydrateClusterUI) so their internal +// awaits also bail before touching DOM. Top-level so both the viewer-cell +// click handler and the zoomWatcher-cell handlers can reach it. See issue +// #187 for the post-mortem that motivated extracting this primitive. +function freshSelectionToken(v) { + v._selGen = (v._selGen || 0) + 1; + const gen = v._selGen; + return () => gen !== v._selGen; +} + // === Helpers: update DOM imperatively (no OJS reactivity) === function updateStats(phase, points, samples, time, pointsLabel, samplesLabel) { const s = (id, v) => { const e = document.getElementById(id); if (e) e.textContent = v; }; @@ -885,11 +909,14 @@ viewer = { } }, Cesium.ScreenSpaceEventType.MOUSE_MOVE); - // Click handler — routes to cluster card or sample card + // Click handler — routes to cluster card or sample card. + // Uses freshSelectionToken so a slow detail/nearby fetch doesn't repaint + // the side panel after the user has clicked a different sample/cluster. new Cesium.ScreenSpaceEventHandler(v.scene.canvas).setInputAction(async (e) => { const picked = v.scene.pick(e.position); if (!Cesium.defined(picked) || !picked.primitive || !picked.id) return; const meta = picked.id; + const isStale = freshSelectionToken(v); if (typeof meta === 'object' && meta.type === 'sample') { // --- Individual sample click --- @@ -909,12 +936,14 @@ viewer = { WHERE pid = '${meta.pid.replace(/'/g, "''")}' LIMIT 1 `); + if (isStale()) return; if (detail && detail.length > 0) { updateSampleDetail(detail[0]); } else { updateSampleDetail({ description: '' }); } } catch(err) { + if (isStale()) return; console.error("Detail query failed:", err); updateSampleDetail(null); } @@ -943,8 +972,10 @@ viewer = { LIMIT 30 `; const samples = await db.query(nearbyQuery); + if (isStale()) return; updateSamples(samples); } catch(err) { + if (isStale()) return; console.error("Sample query failed:", err); if (sampEl) sampEl.innerHTML = '
Query failed — try again.
'; } @@ -1305,28 +1336,9 @@ zoomWatcher = { let requestId = 0; // stale-request guard // clusterDataCache stored on viewer._clusterData (set by phase1 and loadRes) - // --- Selection freshness primitive --- - // - // Async work that updates `viewer._globeState`, the URL hash, or the side - // panel must check freshness after every await. A user input (click, - // hashchange, source-filter toggle, boot deep-link) bumps `_selGen`; any - // in-flight async work whose generation no longer matches must NOT mutate - // anything that an interactive newer event has already moved. - // - // Usage: - // const isStale = freshSelectionToken(); - // await someWork(); - // if (isStale()) return; - // updateDOM(); - // - // Pass `isStale` into helpers (see hydrateClusterUI) so their internal - // awaits also bail before touching DOM. See issue #187 for the post-mortem - // that motivated extracting this primitive. - function freshSelectionToken() { - viewer._selGen = (viewer._selGen || 0) + 1; - const gen = viewer._selGen; - return () => gen !== viewer._selGen; - } + // freshSelectionToken(viewer) is defined at top level (alongside readHash / + // buildHash) so the viewer cell's click handler and this cell's handlers + // can both reach it. See issue #187. // Hysteresis thresholds to avoid flicker const ENTER_POINT_ALT = 120000; // 120 km → enter point mode @@ -1722,7 +1734,7 @@ zoomWatcher = { // any in-flight selection lookup AND re-validate the current selection // (cluster or sample) under the new filter — if it's filtered out, drop // it from runtime state and the URL so the side panel matches the globe. - const isStale = freshSelectionToken(); + const isStale = freshSelectionToken(viewer); try { updateSourceLegendState(); writeQueryState(); @@ -1952,7 +1964,7 @@ zoomWatcher = { window.addEventListener('hashchange', async () => { // Bump the selection generation BEFORE any early-return so even // hashchanges that lack lat/lng invalidate stale async work. - const isStale = freshSelectionToken(); + const isStale = freshSelectionToken(viewer); const state = readHash(); if (state.lat == null || state.lng == null) return; @@ -2364,7 +2376,7 @@ zoomWatcher = { // Wrap in try/finally so `_suppressHashWrite = false` always runs even if // a stale early-return aborts the deep-link work — otherwise a no-lat/lng // hashchange during boot could leave hash writes suppressed forever. - const isStale = freshSelectionToken(); + const isStale = freshSelectionToken(viewer); const ih = viewer._initialHash; try { if (ih.pid) { From 56d0064280823d9c62b8deb145d802b0bfcc999d Mon Sep 17 00:00:00 2001 From: Raymond Yee Date: Sat, 9 May 2026 12:24:43 -0700 Subject: [PATCH 3/3] docs: sync _selGen row in EXPLORER_STATE.md to top-level helper location Codex's P3 nit: the table row still said "in zoomWatcher cell" while the helper is now top-level. The invariant text below was already correct; this just aligns the row. --- EXPLORER_STATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/EXPLORER_STATE.md b/EXPLORER_STATE.md index e9ff509..ebd6b39 100644 --- a/EXPLORER_STATE.md +++ b/EXPLORER_STATE.md @@ -111,7 +111,7 @@ across cells without becoming a separate OJS reactive value. | `viewer.h3Points` | Cesium `PointPrimitiveCollection` | `:815` | cluster mode rendering | | | `viewer.samplePoints` | Cesium `PointPrimitiveCollection` | `:818` | point mode rendering | | | `viewer.pointLabel` | Cesium label entity | `:823` | mouse-move handler (`:836-848`) | hover tooltip | -| `viewer._selGen` | int | bumped by every `freshSelectionToken()` call (in zoomWatcher cell) | snapshot captured by each handler that mutates selection | freshness counter; see invariant below | +| `viewer._selGen` | int | bumped by every `freshSelectionToken(viewer)` call (top-level helper, see invariant below) | snapshot captured by each handler that mutates selection | freshness counter; see invariant below | | `window.refreshSamplesTable` | `() => Promise` | `:1238` | external (debug / Playwright) | not used by other cells; safe to keep or remove | ### Async-selection invariant