explorer: fix URL state round-trip (closes #201 parts A+B)#203
Conversation
Bug A: camera-handler URL write was deferred behind the resolution-load and point-mode-transition awaits inside the debounced callback. When a user panned into a region that triggered cluster→res8 or cold-cache point-mode (60–90s on first load per isamplesorg#190), the URL didn't update until those awaits settled — and a URL copy in the meantime captured the PREVIOUS camera state. Reproduced empirically: iter 2 of the harness moved the camera but `location.href` stayed at the original deep-link. Fix: write the URL hash via `history.replaceState` at the START of the debounced callback, before any awaits. The camera position/heading is known synchronously. The existing trailing write is kept so any mode change produced by the awaits still gets captured. Bug B: deep-link `mode=point` was silently ignored when the URL omitted `heading` (i.e. heading at default 0). The boot path positions the camera via `camera.setView`, which in some cases does not raise `camera.changed`, so the camera-handler that drives the cluster→point transition never runs. Reproduced: iters 3 and 6 of the harness (URLs without `heading=`) failed to enter point mode in a fresh tab; iters 4 and 5 (with non-zero heading) succeeded. Fix: explicit `tryEnterPointModeIfNeeded()` call at the end of the deep-link boot path when `ih.mode === 'point'`. The helper short-circuits if alt >= ENTER_POINT_ALT or we're already in point mode, so this is a no-op for cluster deep-links. Verification: tests/playwright/url_roundtrip_investigation.js, run against localhost with these fixes — all 6 iterations now pass (cf. 4 failures against the live site pre-fix). Harness is shipped under tests/playwright/ as a regression artifact and starting point for a proper spec. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
#203 fixed the await-blocking case for Bug A but a residual remained: small camera moves below Cesium's `percentageChanged` threshold (set to 0.1 at line 2043) don't fire `camera.changed`, so the debounced callback containing the URL write never runs. A user who pans a small distance and immediately copies the URL gets stale state. Empirically confirmed via `url_roundtrip_investigation.js` against the post-#203 deploy: iter 2 (Δlat ≈ 0.02° pan-only, no zoom) had Context A at the new camera position but `location.href` still at the original deep-link, so Context B opened the stale URL. Fix: add `viewer.camera.moveEnd` as a parallel URL-write trigger. `moveEnd` fires once per discrete settled camera move regardless of magnitude (mouseup on drag-pan, wheel-stop on zoom, flight-complete on flyTo), so every position the user can reach gets captured. Keeps `camera.changed` for its existing role (mode transitions, resolution loads); just adds a lightweight URL-only backstop. The `_suppressHashWrite` gate already handles the hashchange-flight path correctly, so no fight with back/forward navigation. Verification: harness now passes all 6 iterations against localhost with this fix (post-#203 it failed on iter 2). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Retrospective Codex review (2026-05-11)Posted on behalf of OpenAI Codex (gpt-5.4). Run after merge + deploy of #203 + #205. Codex read both diffs and surrounding context in Overall: these PRs fixed the two observed round-trip failures in the right general direction, but the implementation is tactical and still leaves URL/state synchronization more fragile than I'd like. I would not revert it. I would file follow-ups. Correctness critiqueThe early URL write in the The caveat: the early write captures the current mode, not the intended target mode. If the user crosses below
The boot-end
Programmatic Design critique
However, #205 only uses The early-write pattern is understandable but not a healthy long-term shape. There are now many URL writers: selection clicks, mode transitions, source/facet query writers, share button, camera debounce early/trailing writes, and writeGlobeHash({ replace: true })
reconcileCameraState({ reason, writeEarlyHash: true })
setExplorerMode(nextMode, { pushHistory: false | true })Then boot, hashchange, user camera movement, and share/copy can call clear, intention-revealing paths. For Bug B, I would not try to force Cesium to emit What we missedThe harness tested camera + mode, but not the full URL contract. Untested or suspicious cases:
The mode-sync anomaly is worth filing. The code currently has two mode stores: closure-private The investigation script should be promoted, but not necessarily as-is. Keep the artifact, then extract CI-safe specs:
The full cold-cache/live-site investigation can remain a manual diagnostic because it is slow and network-sensitive. Suggested follow-up issues
|
|
Follow-up issues filed from the Codex retrospective above, clustered for tractability:
Working through them in that order. |
#206) (#210) * explorer: report real Samples-in-View count when cap is reached (closes #206) In point mode, the viewport sample query was capped at POINT_BUDGET = 5000 with no ORDER BY. The "Samples in View" stat box and phase message both displayed `cachedData.length`, which equals the budget in any region dense enough to exceed it. Cyprus deep-link (`#v=1&lat=34.9957&lng=33.6798&alt=15212&mode=point`) showed exactly 5,000 — real count from DuckDB-WASM against the lite parquet: 23,421 samples in the same viewport. Counter understated the density by ~5x. Two changes, both localized to `loadViewportSamples`: 1. **Real count**: after the LIMIT POINT_BUDGET data fetch, run a `count(*)` against the same WHERE clause when we hit the cap. Stat box `sSamples` now displays the true count; `sPoints` shows the rendered count under a "Samples Rendered" label so the user sees both numbers at a glance. Phase message becomes "23,422 samples in view (showing 5,000 — zoom in for more). Click one for details." when capped; unchanged when not. Stale-guard applied between the two queries. The count query only runs when `data.length >= POINT_BUDGET`. Below the cap the displayed and real counts are identical, so a second round-trip would be pure waste. 2. **`ORDER BY pid`** on the data query, so the POINT_BUDGET-worth of rows the LIMIT returns is deterministic across browsers and sessions. Codex's recommendation 4 from the #203 retrospective. Verification: - Smoke test against localhost confirms "Samples Rendered: 5,000 / Samples in View: 23,422" for the Cyprus URL (cf. matching ±0.13° DuckDB count from the OC investigation). - url_roundtrip_investigation.js still passes all 6 iterations against localhost (no regression in #203/#205 URL state work). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fixup: address Codex review on #206 (stale guard, cache stats, stable labels) Three changes from the Codex review of PR #210: 1. **Stale guard in count-query catch**: the `try/catch` around the real-count query could mutate state on a stale request if the count failed AFTER a newer request started. Added `if (myReqId !== requestId) return` at the top of the catch, before the warning log. 2. **Cache-hit stats**: when `isWithinCache(bounds)` short-circuits the fetch, the stat boxes were left whatever they were from the previous load (the original loose behavior, but now noticeable because the count info matters). Cached `totalCount` and `capReached` alongside bounds + data, and re-apply on cache hits. 3. **Always-on stable labels**: replaced the conditional label flip ("Samples in View" ⇄ "Samples Rendered") with stable labels — sPoints always says "Samples Rendered", sSamples always says "Samples in View". When the cap isn't reached, both boxes show the same number; Codex flagged the label flip as more confusing than the redundancy. All four `cachedBounds = null` reset sites also clear the new `cachedTotalCount` / `cachedCapReached` fields so they don't leak between mode transitions. Verified locally: - Cyprus smoke test still reads `Samples Rendered: 5,000 / Samples in View: 23,422`. - url_roundtrip_investigation.js all 6 iterations green. Follow-up issue #211 filed (Codex's other suggestion: tighten count to the visible viewport rather than the 30%-padded fetch box). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* explorer: URL state contract micro-fixes (closes #207) Four small, independent fixes addressing the URL state contract gaps surfaced by Codex's retrospective on PRs #203 + #205. Each is bounded to its own call site. **Item 4 — low-altitude-without-`mode=point` round-trip.** #203's early URL write can produce URLs like `alt=8000` *without* `mode=point` if the user copies during a pending point-mode transition. Boot only force-entered point mode when the URL had `mode=point`, so the round- trip landed in cluster mode at low altitude — not what the user saw. Boot now also enters point mode when `ih.alt < ENTER_POINT_ALT`, regardless of `ih.mode`. `tryEnterPointModeIfNeeded()` short-circuits above the altitude threshold so cluster deep-links are unaffected. **Item 6 — `h3` hashchange null-result asymmetry.** When `fetchClusterByH3(state.h3)` returns null in the hashchange handler, the UI was cleared but `viewer._globeState.selectedH3` was left set to the invalid value. Boot path already cleared it on the same condition; hashchange now matches. Prevents `buildHash()` from re-emitting the invalid h3 on subsequent camera moves. **Item 8 — selection state with search-result flights.** Search- result row clicks flew the camera but didn't update selection, so the URL captured at flight settle carried whatever prior pid/h3 was selected. Two changes: row click now sets `selectedPid` to the clicked sample's pid (treating the click as a selection, matching on-globe sample clicks); the auto-flight to first result on world- scope searches clears prior selection (treating the auto-pan as a nudge, not an explicit selection). **Item 3 — boot/hash hydration shouldn't grow history.** `tryEnter` `PointModeIfNeeded()` ultimately called `enterPointMode()` with default `pushHistory`, so boot would `pushState` a history entry. Threaded an `opts.pushHistory` option through the helper; boot now passes `{ pushHistory: false }`. Cluster deep-links unaffected (helper short-circuits before that path). Camera-handler call sites keep the default (pushState) since those are user-driven movements. Verified locally: - `url_roundtrip_investigation.js` all 6 iterations green - Low-alt no-`mode=point` URL (`#alt=8000` with no mode param) now enters point mode and shows the real 23k count (post-#206 + #207 item 4). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fixup: address Codex review on #207 (hashchange parity + full search-row hydration) Two issues from the Codex review of PR #212: 1. **Hashchange handler now uses altitude-authoritative predicate** (matches boot's #207 item 4 fix). Without this, navigating back/ forward through a `#alt=8000` URL with no `mode=point` would re-enter cluster mode via the 2s post-flight setTimeout — boot would enter point mode but hashchange would force exit it. Same predicate used in both places now. 2. **Search-result row click does the full sample-selection ceremony**, not just selectedPid + flyTo. Matches on-globe sample click at line ~944: freshness token bump, updateSampleCard with the sample's metadata, flyTo, lazy detail load with stale guards in both success and catch paths. Without this, a slow prior selection's detail query could repaint the panel after the click, leaving the UI inconsistent with `_globeState` and the URL. samplesSection is NOT cleared (unlike the globe click handler) because it holds the search results the user is browsing. Verified: url_roundtrip_investigation.js — all 6 iterations green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
) (#214) * tests: Playwright spec for explorer URL state round-trip (closes #209) CI-safe spec set extracted from `tests/playwright/url_roundtrip_investigation.js` (which remains as a manual diagnostic). Five tests covering the URL state contract slices fixed in PRs #203, #205, #210, #212: 1. **Point-mode deep-link with `mode=point` enters point mode** — regression for Bug B fix in #203 (when heading is omitted, the URL must still hydrate into point mode). 2. **Low-alt deep-link WITHOUT `mode=point` enters point mode** — regression for #207 item 4 (boot should enter point mode based on altitude alone, not just the URL's mode flag). 3. **Sub-threshold pan updates URL hash via `moveEnd`** — regression for #205 (Cesium's `camera.changed` suppresses small moves below `percentageChanged` threshold; `moveEnd` is the backstop). 4. **URL round-trips across browser contexts** — combined regression for #203 + #205. Context A pans + zooms after settle, captures URL; Context B opens the captured URL and arrives at the same camera + mode within tight tolerance. 5. **`h3` hashchange with invalid cell clears `_globeState.selectedH3`** — regression for #207 item 6 (asymmetry between boot and hashchange null-result branches). Search-result row click selection (#207 item 8) deferred — needs working FTS data and depends on substrate the spec shouldn't have to set up. Tests use `waitForPointModeSettled` (waits for the phase message's "Click one for details." text) instead of `waitForMode` for the no-`mode=point` cases — `waitForMode` can match a transient mode flip during the boot transition (the dual-mode-state anomaly being addressed structurally in #208). `test.setTimeout(360000)` per describe block — cold-cache point-mode boot can take 60–90s per #190, and the round-trip test opens multiple fresh contexts each paying the cost. Verified: all 5 tests pass against localhost in ~1m 24s. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * tests: address Codex round-1 review on URL roundtrip specs Per Codex review on PR #214: - Replace fixed `waitForTimeout` sleeps with `waitForFunction` polling URL hash in tests 3 (sub-threshold pan) and 4 (cross-context round-trip). Fixes thin timing margin (0.5s) over flight + moveEnd debounce. - Wrap context-creation in try/finally so ctxA/ctxB get closed on mid-test assertion failure (test 4). - Drop unused `before` snapshot in test 3. - Move 6-minute `test.setTimeout(360000)` from describe-level default (180s) to per-test override on the round-trip test only. Single broken test no longer waits 6 minutes to fail in CI. - Add `waitForBootSettled(page)` helper waiting for `viewer._suppressHashWrite === false` — `_globeState.mode` is initialized early (explorer.qmd:871) while the hashchange listener is registered late (explorer.qmd:2210); waitForMode alone isn't enough. - Strengthen test 5 with the boot-settle wait so the hashchange listener exists before driving the test hashchange. - Add comment in `snapshot()` noting `_globeState.selectedH3`/`selectedPid` field coupling. - Update header: change `closes #209` to `addresses #209` (PR explicitly defers several #209 checklist items to a follow-up). Remove the internally-inconsistent "don't try cold-cache deep-links" sentence that contradicted the timeout comment's cold-cache rationale. Kept (with rationale in code comments): - `waitForPointModeSettled` matches "Click one for details" rather than the count phrase Codex suggested — count phrase misses the cap-reached done-state branch (explorer.qmd:1611) which says "samples in view ..." with no "individual" word. "Click one for details" is the common denominator across both done-state branches. - Test 5 h3-hashchange remains a hashchange-driven test rather than the valid→invalid round-trip Codex proposed. The original assertion DOES catch the regression: line 2272 unconditionally overwrites selectedH3 from the URL on hashchange, so the only way the post-handler value is null is the explicit null-result branch at line 2285. Codex's "trivially true" framing missed that line 2272 always fires first. (Tried seeding selectedH3 before the hashchange to make the test even stronger; the seed survived the handler unexpectedly — suspected OJS reactivity around mainModule.value('viewer') returning a different reference than the handler's closure. Documented in test comment.) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * tests: address Codex round-2 review on URL roundtrip specs Codex round 2 sharpened the round-1 concern on Test 5 and was right: booting with `&h3=<invalid>` runs the boot deep-link's own null-result branch (explorer.qmd:2728) which sets `selectedH3=null` BEFORE the test even drives a hashchange. So the post-mutation `selectedH3 === null` wait is satisfied by boot, not by the hashchange handler's null-clear branch at line 2285 (the regression we want to guard). Fix: gate the assertion on a handler-only side effect. The hashchange handler's `camera.flyTo` (explorer.qmd:2220-2227) rotates `camera.heading` to the URL's heading value — only the handler produces this rotation. Wait for heading to reach ~5° before asserting `selectedH3 === null`. This proves the handler executed past line 2272 (which would write a non-null selectedH3 from the URL) AND reached line 2285 (the null-clear). Also addresses Codex's altitude nit on Test 4: `waitForHashLatLng` now takes an optional `alt` so the round-trip assertion checks lat + lng + alt together, matching what `buildHash` actually writes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes #201 (parts A + B). Part 1 — the misleading 'Samples in View' counter — is left for a separate PR.
What this changes
Two small, independent fixes in
explorer.qmd:Bug A — camera-handler URL write was deferred behind awaits
explorer.qmd:1965-2030— the camera-changed handler's debounced callback awaitsloadRes/tryEnterPointModeIfNeeded(cold-cache point-mode boot can be 60–90s per #190) before reaching the trailinghistory.replaceState. When a user pans into a region that triggers a resolution change or point-mode transition, the URL write is deferred until those awaits settle. A URL copy in the meantime captures the previous camera state.Fix: write the URL hash at the START of the debounced callback, before any awaits. The camera state is known synchronously. The trailing write is kept so any mode change produced by the awaits still gets captured.
Bug B — deep-link
mode=pointsilently ignored when heading=0explorer.qmdboot path positions the camera viacamera.setView, which in some cases (notably when the URL omitsheading=and it defaults to 0) does not raisecamera.changed. The camera-handler that drives the cluster→point transition therefore never runs, and the URL'smode=pointis ignored.Fix: explicit
tryEnterPointModeIfNeeded()call at the end of the deep-link boot path whenih.mode === 'point'. The helper short-circuits if alt >= ENTER_POINT_ALT or we're already in point mode, so this is a no-op for cluster deep-links.Verification
tests/playwright/url_roundtrip_investigation.jsis the harness that originally detected the bugs (see #201 comment with empirical evidence). It drives Context A through 6 deterministic pan/zoom steps from the Cyprus deep-link, then opens each resulting URL in a fresh Context B and probes at 5s/15s/30s.Harness logs land at
/tmp/url_roundtrip_log.json. Run withTEST_URL=http://localhost:5860/explorer.html node tests/playwright/url_roundtrip_investigation.js.Risks
replaceState(no history-entry growth). If the awaits don't change mode, the second write is identical to the first. Acceptable cost.tryEnterPointModeIfNeeded()being safe to call from boot. Reviewed: it short-circuits onmode === 'point'and on alt >= ENTER_POINT_ALT, and uses the sameloadRespath that the camera handler would. Functionally equivalent to "what would happen ifcamera.changedfired."setView-doesn't-fire-camera.changed is the underlying cause of Bug B's mechanism. Documented inline at the boot call site. This PR works around it; a longer-term fix would be to ensuresetViewraises the event (likely by tweaking the call shape or following with a no-op camera move).tests/playwright/but is a Node script, not a Playwright spec. A follow-up should adapt it into a proper*.spec.jsso it runs in CI. Tracked as a TODO in the harness file's header comment.Test plan
🤖 Generated with Claude Code