Skip to content

explorer: centralize globe-hash writes behind writeGlobeHash() (#208 PR4a) #288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rdhyee wants to merge 1 commit into
base: main
Choose a base branch
from
Open

explorer: centralize globe-hash writes behind writeGlobeHash() (#208 PR4a) #288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
209 changes: 209 additions & 0 deletions REFACTOR_PR4_PLAN.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
46 changes: 29 additions & 17 deletions explorer.qmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 = '';
Expand Down Expand Up @@ -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 = '<div style="text-align: center; color: #999; padding: 12px;">Loading nearby samples...</div>';
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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');
Expand All @@ -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();
Expand Down Expand Up @@ -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
Expand All @@ -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 });
}
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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');
Expand Down
Loading