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.

@rdhyee
Copy link
Copy Markdown
Contributor Author

rdhyee commented May 11, 2026

Follow-up issues filed from the Codex retrospective above, clustered for tractability:

Working through them in that order.

rdhyee added a commit that referenced this pull request May 11, 2026
#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>
rdhyee added a commit that referenced this pull request May 12, 2026
* 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>
rdhyee added a commit that referenced this pull request May 12, 2026
) (#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>
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