Skip to content

explorer: fix URL state round-trip (closes #201 parts A+B)#203

Merged
rdhyee merged 1 commit into
isamplesorg:mainfrom
rdhyee:fix/201-url-state-roundtrip
May 11, 2026
Merged

explorer: fix URL state round-trip (closes #201 parts A+B)#203
rdhyee merged 1 commit into
isamplesorg:mainfrom
rdhyee:fix/201-url-state-roundtrip

Conversation

@rdhyee
Copy link
Copy Markdown
Contributor

@rdhyee rdhyee commented May 11, 2026

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 awaits loadRes / tryEnterPointModeIfNeeded (cold-cache point-mode boot can be 60–90s per #190) before reaching the trailing history.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=point silently ignored when heading=0

explorer.qmd boot path positions the camera via camera.setView, which in some cases (notably when the URL omits heading= and it defaults to 0) does not raise camera.changed. The camera-handler that drives the cluster→point transition therefore never runs, and the URL's mode=point is ignored.

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 is 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.

Iter Pan/zoom step Live site (pre-fix) localhost (post-fix)
1 no-op baseline
2 pan NE only ❌ URL stale (Bug A)
3 zoom in, heading 0 ❌ B stuck cluster (Bug B)
4 heading 45° at zoom
5 pan+zoom+heading 90°
6 heading 360° (=0 mod) ❌ B stuck cluster (Bug B)

Harness logs land at /tmp/url_roundtrip_log.json. Run with TEST_URL=http://localhost:5860/explorer.html node tests/playwright/url_roundtrip_investigation.js.

Risks

  • Double URL write per camera-change: The handler now writes the hash at the start AND at the end of the debounced callback. Both are replaceState (no history-entry growth). If the awaits don't change mode, the second write is identical to the first. Acceptable cost.
  • Bug B fix relies on tryEnterPointModeIfNeeded() being safe to call from boot. Reviewed: it short-circuits on mode === 'point' and on alt >= ENTER_POINT_ALT, and uses the same loadRes path that the camera handler would. Functionally equivalent to "what would happen if camera.changed fired."
  • Cesium 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 ensure setView raises the event (likely by tweaking the call shape or following with a no-op camera move).
  • No new tests in the regression suite yet. The harness is shipped as an artifact under tests/playwright/ but is a Node script, not a Playwright spec. A follow-up should adapt it into a proper *.spec.js so it runs in CI. Tracked as a TODO in the harness file's header comment.

Test plan

  • Harness passes all 6 iterations against localhost (Quarto preview, this branch)
  • Manual smoke on deploy preview: Cyprus deep-link → pan → copy URL → paste in private window → view matches
  • Codex review (next round)

🤖 Generated with Claude Code

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>
@rdhyee rdhyee merged commit 64a5041 into isamplesorg:main May 11, 2026
1 check passed
rdhyee added a commit that referenced this pull request May 11, 2026
#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>
@rdhyee
Copy link
Copy Markdown
Contributor Author

rdhyee commented May 11, 2026

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 explorer.qmd directly. Verbatim review below.


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 critique

The early URL write in the camera.changed debounce is mostly safe because it writes current viewer state with replaceState, and the trailing write later re-emits the final mode/state after async work settles. I do not see a direct ordering bug where the two corrupt each other.

The caveat: the early write captures the current mode, not the intended target mode. If the user crosses below ENTER_POINT_ALT and copies the URL while the res8 / point-mode transition is still pending, the URL can be "low altitude, no mode=point". That is better than the old stale camera URL, but it may still fail the intended "this view round-trips into point mode" contract because boot only explicitly forces point mode when the URL says mode=point.

moveEnd as added in #205 is idempotent with the camera.changed URL writes. Duplicate replaceState calls are cheap, and if camera.changed later transitions mode, its final write wins with the more complete state. I do not see a serious competition bug there.

The boot-end tryEnterPointModeIfNeeded() call is directionally correct. Running it after _suppressHashWrite = false is better than keeping URL writes suppressed during a potentially long cold-cache point-mode load. But there is a history-stack smell: tryEnterPointModeIfNeeded() eventually calls enterPointMode() with default pushHistory, so boot hydration can pushState during initial load. That behavior probably existed through the camera-handler path too, but the explicit boot path makes it more deterministic. Boot and hash hydration should generally reconcile state without adding a new browser history entry.

_suppressHashWrite covers the major intended paths: initial setView and hashchange-driven flyTo. The weak spot is that hashchange suppression is cleared by a fixed 2s timer, not by actual flight completion. Given the hashchange flight duration is 1.5s, this is probably okay in normal conditions, but it is still timer-coupled state management.

Programmatic flyTo calls outside hashchange are now URL-writing via moveEnd. That is usually desirable for search-result flights. But it also means any stale selectedPid / selectedH3 already in _globeState will be preserved into the new camera URL unless that action explicitly clears or updates selection. That was already possible through camera.changed; #205 just makes it more reliable.

Design critique

moveEnd is the right backstop for URL freshness. Lowering percentageChanged would increase churn and still would not give a clean "settled final camera" signal unless set very low. Cesium's docs define changed as threshold-based and moveEnd as the stopped-moving event, so moveEnd matches this problem better.

However, #205 only uses moveEnd for URL writes. That means sub-threshold moves still bypass the rest of the camera-settled pipeline: sample reloads, cached viewport re-rendering, and cluster count updates are still tied to camera.changed. If a small pan is important enough to update the URL, it may also be important enough to update "Samples in View" / "Clusters in View". I'd follow up by routing both camera.changed and moveEnd through one settled-camera reconciliation path, with cheap URL writes separated from expensive data work.

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 moveEnd. I'd rather see a small explicit layer:

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 camera.changed from setView. The cleaner fix is what #203 did: after programmatic camera placement, call the app's own reconciliation logic explicitly. A tiny camera nudge would be hacky. flyTo({ duration: 0 }) may give you a completion callback, but I would still not make app correctness depend on Cesium deciding to emit movement/change events for a zero-duration operation.

What we missed

The harness tested camera + mode, but not the full URL contract.

Untested or suspicious cases:

  • pid + filters: boot/hashchange PID hydration does not appear to apply sourceFilterSQL() or facet filters, so ?sources=...#...&pid=... can hydrate a sample that the active filters exclude.
  • h3 hashchange: when fetchClusterByH3(state.h3) returns null, the hashchange path clears the UI but does not clear viewer._globeState.selectedH3. Boot does clear it. That asymmetry can cause invalid/filtered h3 to be re-emitted later by buildHash().
  • search, search_scope, sources, material, context, object_type, and view=table are outside the round-trip harness.
  • Low-altitude URLs without mode=point remain ambiguous. The early-write fix can create exactly that state while point-mode entry is pending.
  • Search-result flights do not set or clear selection, so the URL can become "new camera, old selectedPid/h3".
  • The "Samples in View = 5000" cap remains unresolved, as expected.

The mode-sync anomaly is worth filing. The code currently has two mode stores: closure-private mode and public viewer._globeState.mode. They are updated together today, but the duplication is fragile, and the harness/tests use _globeState.mode as truth. I would either make _globeState.mode the single source of truth or wrap mode transitions in one setter and add an invariant assertion/log.

The investigation script should be promoted, but not necessarily as-is. Keep the artifact, then extract CI-safe specs:

  • point deep-link with omitted/default heading boots into point mode;
  • sub-threshold pan updates hash after moveEnd;
  • copied URL from context A restores camera/mode in context B;
  • selection/filter URL cases for pid, h3, and sources.

The full cold-cache/live-site investigation can remain a manual diagnostic because it is slow and network-sensitive.

Suggested follow-up issues

  1. Fix explorer: 'Samples in View' shows the 5000 fetch budget, not real count + URL round-trip doesn't reproduce view #201 Part 1: make "Samples in View" honest when the point query hits DEFAULT_POINT_BUDGET; show capped state or compute a real count separately.
  2. Centralize camera URL/mode reconciliation; make moveEnd drive settled-camera data/UI updates, not only URL writes.
  3. Avoid pushState during boot hydration and hash hydration; mode reconciliation should support { pushHistory: false }.
  4. Define and test the low-altitude-without-mode=point contract.
  5. Add Playwright specs for URL round-trip coverage beyond camera/mode: pid, h3, search, search_scope, sources, facets, and view=table.
  6. Fix h3 hashchange null-result handling so runtime state and URL stop preserving invalid/filtered H3 cells.
  7. Investigate/remove the dual mode state (mode closure variable vs viewer._globeState.mode).
  8. Clear or intentionally update selection state when search-result flights move the camera away from a selected sample/cluster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

explorer: 'Samples in View' shows the 5000 fetch budget, not real count + URL round-trip doesn't reproduce view

1 participant