diff --git a/REFACTOR_PR4_PLAN.md b/REFACTOR_PR4_PLAN.md new file mode 100644 index 0000000..cf80239 --- /dev/null +++ b/REFACTOR_PR4_PLAN.md @@ -0,0 +1,209 @@ +# PR4 plan — URL/state single-writer boundary (#208) + +Draft for Codex review (2026-06-15). Continues the #249 strangler refactor. +PR1–PR3 (smoke gate, characterization tests, pure-module extraction) are +merged to upstream and live on isamples.org. This plan covers the next +documented stage: #208 "centralize URL writers, collapse dual mode." + +Method unchanged: extract along seams, behind the green smoke + +characterization gate, Codex review per step, behavior-neutral. + +--- + +## 1. Current state (verified against `explorer.qmd` @ be79a82) + +#208 has three sub-smells. **Smell 2 is already resolved** — do not redo it: + +| #208 sub-step | Status | Evidence | +|---|---|---| +| Smell 2 — collapse dual `mode` | ✅ DONE | `viewer._globeState.mode` is the sole store; `getMode()`/`setExplorerMode()` exist at L2368-2369; the only other `mode` token is a local `const mode = getMode()` (L2684). No closure-private `mode` variable remains. | +| Smell 1a — single URL-write boundary | ❌ TODO | **13** `history.{replace,push}State` sites; **11** write `buildHash(viewer)` (hash), 2 write `url` (query string, owned by `writeQueryState`). | +| Smell 1b — unify `camera.changed` + `moveEnd` via `reconcileCameraState` | ❌ TODO | Two listeners with overlapping duties + the sub-`percentageChanged` "URL updates but stats don't" gap; also the #262 heatmap→zoom-out cluster-restore bug. | + +### The 11 hash-write sites (Smell 1a migration surface) — Codex-corrected 2026-06-15 + +| Line | Verb | Gate today | Caller / intent | writeGlobeHash call | +|---|---|---|---|---| +| 1594 | push | **none** | map **sample-click** selection | `{replace:false, force:true}` | +| 1654 | push | **none** | map **cluster-click** selection | `{replace:false, force:true}` | +| 2059 | replace | **none** (bypasses suppress by design, L2055) | table row-click selection | `{force:true}` | +| 2698 | push | `pushHistory!==false` only (NOT suppress-gated) | `enterPointMode` | keep caller guard; `{replace:false, force:true}` | +| 2710 | push | `pushHistory!==false` only (NOT suppress-gated) | `exitPointMode` | keep caller guard; `{replace:false, force:true}` | +| 3525 | replace | **none** | source-filter invalidates selected H3 | `{force:true}` | +| 3548 | replace | **none** | source-filter invalidates selected pid | `{force:true}` | +| 3680 | replace | `if (!_suppressHashWrite)` | camera.changed EARLY write (#201 Bug A) | `{}` (gated default) | +| 3757 | replace | `if (!_suppressHashWrite)` | camera.changed LATE write | `{}` (gated default) | +| 3823 | replace | `if (!_suppressHashWrite)` | moveEnd backstop write (#204) | `{}` (gated default) | +| 4044 | replace | **none** | **Share button** pre-copy hash refresh | `{force:true}` | + +**Only 3 sites (3680/3757/3823) are actually `_suppressHashWrite`-gated.** The +other 8 are ungated today and MUST migrate with `force:true` to stay +behavior-neutral — default-gating them (my original error) would change behavior. + +Two non-hash writes stay out of scope (query string): `writeQueryState()` body +(L940-943) and `persistSearchScope()` (L4105-4111, preserves the existing hash). +**Correction:** L4044 is the Share button (a hash write, in scope); L4111 is +`persistSearchScope` (query string, out of scope) — the reverse of the original +draft. + +`_suppressHashWrite` lifecycle: set true at boot (L1506), cleared at L5176; +toggled around the hashchange deep-link apply (L3955/3973). Read by exactly 3 +hash writers (3679/3756/3822). + +### ⚠️ The one judgment call: 2698/2710 during boot (Q2) + +`enterPointMode` CAN fire in the suppressed boot window (camera.changed → +`tryEnterPointModeIfNeeded()` at L3705 → `enterPointMode(undefined)` → pushes at +L2698), even though boot reconciliation "should not add history" (L5197). +**PR4a decision: preserve current behavior exactly** (`force:true` when +`pushHistory!==false`) — keep the refactor pure. Whether boot should suppress +those pushes is a *separate, tested bugfix*, filed as follow-up, NOT folded into +PR4a (per "preserve working code patterns"). + +--- + +## 2. PR4a — `writeGlobeHash()` boundary (LOW risk, high clarity) + +### Design + +A single top-level function (sibling to `buildHash`), the only place that +writes the hash: + +```js +// replace: true (default) → history.replaceState; false → pushState +// force: true → write even when _suppressHashWrite is set (selection path) +function writeGlobeHash(viewer, { replace = true, force = false } = {}) { + if (viewer._suppressHashWrite && !force) return; + const hash = buildHash(viewer); + if (replace) history.replaceState(null, '', hash); + else history.pushState(null, '', hash); +} +``` + +This bakes the `_suppressHashWrite` gate into ONE place instead of repeating +`if (!viewer._suppressHashWrite)` at 4 sites and silently omitting it at 5. + +### Migration + +Drive entirely off the **Codex-corrected table in §1** (the `writeGlobeHash call` +column is the per-site spec). Summary: 3 gated-default sites (3680/3757/3823), +8 `force:true` sites (everything else), `replace:false` for the 4 push sites +(1594/1654/2698/2710), `replace` for the rest. enter/exitPointMode keep their +`pushHistory!==false` caller guard. + +### Behavior-neutrality argument + +Each migrated site produces the identical `history` call for identical state, +because the per-site `force`/`replace` flags reproduce today's exact gate (now +verified line-by-line by Codex). The only sites reading `_suppressHashWrite` +today are 3680/3757/3823 → they alone use the gated default; all others were +already ungated and use `force:true`. + +### Verification +- `git diff` shows only call-site swaps + one new function; `buildHash` + unchanged. +- Characterization (d1 `?search=`, d2 `&pid=`) + `url-roundtrip.spec.js` + (the existing deep deep-link spec) exercise push vs replace and the + suppress gate. Run both manually (live parquet) + smoke on the PR. +- Add a focused Playwright assertion: after a row-click, `history.length` + increases (push) and `#pid=` is in the URL; after a small pan, the URL + updates without a new history entry (replace). (Optional — the existing + url-roundtrip spec may already cover the round-trip.) + +--- + +## 3. PR4b — `reconcileCameraState(reason)` (HIGH risk — recommend caution) + +### Why it's risky + +`camera.changed` (L3666, debounced 600ms + `percentageChanged=0.1`) and +`moveEnd` (L3821, every settled move) overlap but are NOT redundant. Between +them they encode hard-won fixes for **#190, #193, #201, #204, #221, #234 +(A1/B1/C3), #237, #240, #262**. Specifically they differ in: + +- **URL write timing**: camera.changed writes EARLY (before awaits, #201 Bug A) + AND late; moveEnd writes once. A naive merge could drop the early write. +- **Mode transitions + resolution reload** live only in camera.changed (with + the #193 "chase" gates on `applied`). +- **Facet counts / heatmap** (#237/#240 stale-guards) live only in moveEnd. +- **Point-mode exit backstop** (#221 round 2) lives only in moveEnd. +- **Debounce**: camera.changed coalesces 600ms; moveEnd is immediate. + +`reason`-tagged unification (`'changed' | 'moveEnd'`) is feasible but every +branch above must be preserved exactly. This is the opposite of PR3's +mechanical safety. + +### Recommended shape (if we proceed) + +Do NOT merge the two handlers wholesale. Instead: +1. Extract the **shared settled-camera tail** both already run — the URL write + (now `writeGlobeHash`) + the cluster "Samples in View" stat update — into a + small `reconcileSettledCamera(viewer)` and call it from both. This closes + the #204 sub-threshold-URL gap minimally without touching mode/resolution + logic. +2. Leave mode-transition + resolution-reload in camera.changed and + facet/heatmap/point-exit in moveEnd as-is. +3. Treat #262 (heatmap→zoom-out cluster restore) as a SEPARATE targeted fix + with its own characterization test, NOT folded into a big merge. + +### Open recommendation +**Split confirmed: PR4a and PR4b are separate PRs.** PR4a is a clean win and +should land first and alone. PR4b's value (#208 smell 1b + #262) is real but +its risk profile argues for the minimal `reconcileSettledCamera` tail above +rather than the full handler merge Codex originally sketched. Flag for Codex: +is the minimal-tail extraction enough to claim #208 smell 1b, or does the +acceptance criterion ("shared settled-camera reconciliation entry point") +require the full merge? (Q3) + +--- + +## 4. Codec extraction question (Q4) + +The roadmap's PR4 title is "URL/state **codecs** + boundary." `readHash` is +already a module (PR3). `buildHash` is nearly pure — it reads +`viewer.camera`, `viewer._globeState`, and the `#heatmapToggle` DOM checkbox. +Options: +- (a) Leave `buildHash` in the qmd (it's the encode half; the DOM read makes + it not cleanly pure). Ship only `writeGlobeHash` this PR. +- (b) Extract `buildHash(viewer, { heatmapOn })` to `explorer-utils.js` with + the DOM read hoisted to the caller, unit-test the encode. More surface, more + value for #164's state-contract-in-code goal. + +Lean (a) for PR4a to keep it tiny; revisit (b) only if Codex thinks the codec +pairing (readHash already extracted) is worth completing now. + +--- + +## 5. Sequencing & gate + +1. **PR4a** — `writeGlobeHash` + migrate 11 sites. Behavior-neutral. + Gate: smoke + characterization (d1/d2) + url-roundtrip, Codex review. +2. **PR4b** — `reconcileSettledCamera` minimal tail (+ optional #262 fix as a + tracked sibling). Separate PR, separate Codex review, extra deep specs. +3. Defer the full camera-handler merge and the Vite/TS question (post-PR5, + per #249). + +Per-seam freeze only (not page-wide): while PR4a is open, avoid other edits to +the 11 listed write sites. + +--- + +## 6. Questions for Codex — ANSWERED (Codex gpt-5.5, 2026-06-15) +- **Q1**: 1594/1654 are map-click selections (not boot); they ignore suppress + today → preserve via `force:true`. ✅ +- **Q2**: Yes — enter/exitPointMode CAN fire during the suppressed boot window + (L3705→L2809→L2698). So preserve current behavior with `force:true`-when- + `pushHistory!==false`; treat boot-push suppression as a separate tested + bugfix, not part of PR4a. ✅ (see §1 judgment-call box) +- **Q3**: Full handler merge NOT required. Minimal shared-tail extraction + (hash write + cluster "Samples in View" stat) satisfies #208 smell 1b. ✅ +- **Q4**: Defer `buildHash` extraction (it reads `#heatmapToggle` DOM at L1186; + needs a signature change + encoder tests). ✅ +- **Q5**: Yes — corrected: 3525/3548 ungated (not gated); 4044 = Share button + (in scope); 4111 = persistSearchScope (query string, out of scope); no + hashchange-internal buildHash write exists; row-click = replace, map = push. + All folded into the §1 table. ✅ + +**Codex verdict:** agrees with the 2-PR split and with avoiding the full camera +merge; required §1 to be corrected before PR4a (done above). Plan is now +implementation-ready for PR4a. diff --git a/explorer.qmd b/explorer.qmd index d671972..73c8c0e 100644 --- a/explorer.qmd +++ b/explorer.qmd @@ -1187,6 +1187,24 @@ function buildHash(v) { return '#' + params.toString(); } +// === Single URL-hash writer (#208 smell 1a) === +// The ONLY place that writes the globe hash. Centralizes the +// `_suppressHashWrite` gate that was previously repeated at the camera/ +// moveEnd writers and silently omitted at the selection/share writers. +// replace: true (default) → history.replaceState; false → pushState. +// force: true → write even when _suppressHashWrite is set. Used by the +// explicit user-action writers (map/row selection, enter/exit +// point mode, source-filter selection-invalidation, Share button) +// that must always reflect the action; only the camera.changed / +// moveEnd settled-camera writers honor the suppress gate (force +// omitted). See REFACTOR_PR4_PLAN.md §1 for the per-site matrix. +function writeGlobeHash(v, { replace = true, force = false } = {}) { + if (v._suppressHashWrite && !force) return; + const hash = buildHash(v); + if (replace) history.replaceState(null, '', hash); + else history.pushState(null, '', hash); +} + // === Selection freshness primitive === // // Async work that updates `viewer._globeState`, the URL hash, or the side @@ -1591,7 +1609,7 @@ viewer = { showInMapCard(meta, { x: e.position.x, y: e.position.y }); v._globeState.selectedPid = meta.pid; v._globeState.selectedH3 = null; - history.pushState(null, '', buildHash(v)); + writeGlobeHash(v, { replace: false, force: true }); // Clear nearby list const sampEl = document.getElementById('samplesSection'); if (sampEl) sampEl.innerHTML = ''; @@ -1651,7 +1669,7 @@ viewer = { hideInMapCard(); v._globeState.selectedPid = null; v._globeState.selectedH3 = meta.h3_cell || null; - history.pushState(null, '', buildHash(v)); + writeGlobeHash(v, { replace: false, force: true }); const sampEl = document.getElementById('samplesSection'); if (sampEl) sampEl.innerHTML = '
Loading nearby samples...
'; @@ -2056,7 +2074,7 @@ tableView = { // zoomWatcher's listener. zoomWatcher may not be wired yet // at very-early-boot click time, and even when it is, the // _suppressHashWrite gate could swallow the write. - history.replaceState(null, '', buildHash(viewer)); + writeGlobeHash(viewer, { force: true }); // Repaint to apply .selected class without a full refresh. tableEl.querySelectorAll('tbody tr.selected').forEach(el => el.classList.remove('selected')); tr.classList.add('selected'); @@ -2695,7 +2713,7 @@ zoomWatcher = { async function enterPointMode(pushHistory) { setExplorerMode('point'); applyLayerVisibility(); - if (pushHistory !== false) history.pushState(null, '', buildHash(viewer)); + if (pushHistory !== false) writeGlobeHash(viewer, { replace: false, force: true }); // #facetNote is only meaningful in cluster mode (#234 step 1). syncFacetNote(); console.log('Entered point mode'); @@ -2707,7 +2725,7 @@ zoomWatcher = { setExplorerMode('cluster'); viewer.samplePoints.removeAll(); applyLayerVisibility(); - if (pushHistory !== false) history.pushState(null, '', buildHash(viewer)); + if (pushHistory !== false) writeGlobeHash(viewer, { replace: false, force: true }); // Returning to cluster mode: surface the honesty note if any // facet filter is active (#234 step 1). syncFacetNote(); @@ -3522,7 +3540,7 @@ zoomWatcher = { updateClusterCard(null); const sampEl = document.getElementById('samplesSection'); if (sampEl) sampEl.innerHTML = ''; - history.replaceState(null, '', buildHash(viewer)); + writeGlobeHash(viewer, { force: true }); } else { // Cluster's dominant_source still checked, but the // nearby-samples list inside hydrateClusterUI is @@ -3545,7 +3563,7 @@ zoomWatcher = { updateClusterCard(null); const sampEl = document.getElementById('samplesSection'); if (sampEl) sampEl.innerHTML = ''; - history.replaceState(null, '', buildHash(viewer)); + writeGlobeHash(viewer, { force: true }); } } } @@ -3676,9 +3694,7 @@ zoomWatcher = { // below settles — and a URL copy in the meantime captures the // PREVIOUS state. We still write again at the end of the // handler to capture any mode change that the awaits produced. - if (!viewer._suppressHashWrite) { - history.replaceState(null, '', buildHash(viewer)); - } + writeGlobeHash(viewer); // Determine target mode with hysteresis. // A1 (#234 Step 4) / C3: while a search is active, latch point @@ -3753,9 +3769,7 @@ zoomWatcher = { } // Update URL hash (replaceState for continuous movement) - if (!viewer._suppressHashWrite) { - history.replaceState(null, '', buildHash(viewer)); - } + writeGlobeHash(viewer); }, 600); }); viewer.camera.percentageChanged = 0.1; @@ -3819,9 +3833,7 @@ zoomWatcher = { }); viewer.camera.moveEnd.addEventListener(() => { - if (!viewer._suppressHashWrite) { - history.replaceState(null, '', buildHash(viewer)); - } + writeGlobeHash(viewer); // B1: viewport-aware facet counts. Bouncing through refreshFacetCounts // reuses the existing 250ms debounce + facetCountsReqId stale-guard, // so bursts of moveEnd (drag-pan, wheel-zoom) coalesce into one query @@ -4041,7 +4053,7 @@ zoomWatcher = { const shareBtn = document.getElementById('shareBtn'); if (shareBtn) { shareBtn.addEventListener('click', async () => { - history.replaceState(null, '', buildHash(viewer)); + writeGlobeHash(viewer, { force: true }); try { await navigator.clipboard.writeText(location.href); const toast = document.getElementById('shareToast');