explorer: URL state contract micro-fixes (closes #207) by rdhyee · Pull Request #212 · isamplesorg/isamplesorg.github.io

rdhyee · 2026-05-12T04:15:47Z

Closes #207. Four small, independent fixes from the #203 retrospective Codex review (cluster A).

Items

#207 item 4 — low-altitude-without-`mode=point` round-trip

Problem: #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.

Fix: boot also enters point mode when ih.alt < ENTER_POINT_ALT, regardless of ih.mode. tryEnterPointModeIfNeeded() already short-circuits above the threshold, so cluster deep-links are unaffected.

#207 item 6 — `h3` hashchange null-result asymmetry

Problem: when fetchClusterByH3(state.h3) returned 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.

Fix: hashchange null branch now sets selectedH3 = null to match boot, preventing buildHash() from re-emitting invalid H3.

#207 item 8 — selection state with search-result flights

Problem: search-result row clicks flew the camera but didn't update selection. The URL captured at flight settle (via #205's moveEnd listener) carried whatever prior selectedPid / selectedH3 was set, misrepresenting the user's actual view.

Fix:

Row click now sets selectedPid to the clicked sample's pid (treats click as an explicit selection, matching on-globe sample clicks).
Auto-flight to first result on world-scope searches clears prior selection (treats auto-pan as a nudge, not an explicit selection — user clicks a specific row to establish a new selection).

#207 item 3 — boot/hash hydration shouldn't grow history

Problem: tryEnterPointModeIfNeeded() called enterPointMode() with default pushHistory, so boot would pushState a history entry every time.

Fix: threaded opts.pushHistory through the helper; boot call site passes { pushHistory: false }. Camera-handler call sites keep the default (pushState) since those are user-driven movements where adding history is intentional.

Verification

tests/playwright/url_roundtrip_investigation.js — all 6 iterations green on localhost
One-off test: URL #v=1&lat=35.0150&lng=33.7000&alt=8000 (low alt, NO mode param) now enters point mode in a fresh tab and shows the real 23k count (combination of explorer: 'Samples in View' counter is the fetch budget, not the real count (#201 Part 1) #206 + explorer: URL state contract micro-fixes (post-#203 follow-ups) #207 item 4).

Risks

Item 4 broadens boot's point-mode entry condition. Previously only ih.mode === 'point' triggered the explicit boot call; now altitude does too. Cluster deep-links above ENTER_POINT_ALT are unchanged (short-circuit). A bookmarked URL with stale mode=cluster and low altitude will now enter point mode at boot — but that's the correct behavior given the altitude is the more authoritative state.
Item 8 row-click selection. Setting selectedPid from a search-result click is a small UX change: the URL now reflects the searched-for sample as selected. The clicked sample's full sample card hydration is not part of this PR — that requires more data than the search-result row carries. Tracked as a possible follow-up if the lack of side-panel hydration is jarring.
Item 3 history avoidance. Boot no longer adds a history entry on point-mode entry. Users navigating away and using back will now go to the previous page rather than a "stuck on this page in cluster mode" intermediate state. Net win, no obvious regression.
Tests don't cover items 6 and 8 directly. They're tracked in explorer: expand Playwright URL round-trip coverage beyond camera+mode #209 (test coverage expansion). The fixes are small enough that code review covers them.

Test plan

url_roundtrip_investigation.js all 6 green
Low-alt no-mode URL enters point mode (verified manually)
Manual smoke on deploy preview:
- Cyprus deep-link → pan small → copy URL → paste → matches (regression check for explorer: fix URL state round-trip (closes #201 parts A+B) #203 + explorer: add camera.moveEnd URL-write backstop (closes #204) #205)
- URL with low alt but no mode=point → enters point mode (item 4)
- Hash to invalid h3 → URL cleans up (item 6)
- Search result click → URL reflects pid (item 8)
Codex review (recommended)

🤖 Generated with Claude Code

Four small, independent fixes addressing the URL state contract gaps surfaced by Codex's retrospective on PRs isamplesorg#203 + isamplesorg#205. Each is bounded to its own call site. **Item 4 — low-altitude-without-`mode=point` round-trip.** isamplesorg#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-isamplesorg#206 + isamplesorg#207 item 4). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ull search-row hydration) Two issues from the Codex review of PR isamplesorg#212: 1. **Hashchange handler now uses altitude-authoritative predicate** (matches boot's isamplesorg#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>

rdhyee · 2026-05-12T04:26:45Z

Codex review applied (commit `5176967`)

Codex's review surfaced two real issues I'd missed; both now addressed.

1. Hashchange handler parity with boot's altitude-authoritative predicate (item 4 follow-up). Boot now treats alt < ENTER_POINT_ALT as authoritative for entering point mode, but the hashchange post-flight setTimeout at explorer.qmd:2233 was still gated on s.mode === 'point' only. Result: back/forward through a #alt=8000 URL with no mode=point would re-enter cluster mode via the timeout, even though boot would have entered point mode for the same URL. Fixed by using the same wantsPoint = s.mode === 'point' || (s.alt != null && s.alt < ENTER_POINT_ALT) predicate in both places.

2. Search-row click now does the full sample-selection ceremony (item 8 follow-up). My first cut just set selectedPid + flew. Codex correctly pushed back: without freshness-token bump, sample-card hydration, and lazy detail load, a slow prior selection's repaint could land after the click, leaving UI inconsistent with _globeState and the URL. Now matches the on-globe sample click at line ~944:

freshSelectionToken(viewer) bump
updateSampleCard with full metadata from the search result row (built a Map<pid, result> for closure capture)
flyTo
Lazy detail query with stale guards in both success and catch paths

samplesSection is intentionally NOT cleared (unlike the globe click handler) — it holds the search results the user is browsing.

Codex confirmed items 3 and 6 are correctly handled (three-state opts?.pushHistory is right; null-h3 clear is race-safe via the isStale() check before mutation).

Verified: url_roundtrip_investigation.js all 6 iterations still green.

Codex review verbatim:

explorer.qmd:2233 still makes hashchange mode reconciliation depend only on s.mode === 'point'. So boot now treats alt < ENTER_POINT_ALT as authoritative, but back/forward or manual hash edits do not. [...] If altitude is authoritative, use the same predicate here as boot. [✅]

explorer.qmd:2515 and explorer.qmd:2538 mutate selection state without treating search as a real selection event. [...] A slow previous globe/hash selection can still repaint the panel after the search click [...] I would not leave this as a follow-up if this PR is about URL selection truth. [✅]
No other blocking issues from the diff.

Ready to merge from my side.

) (#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>

rdhyee and others added 2 commits May 11, 2026 21:15

rdhyee merged commit 9a20da5 into isamplesorg:main May 12, 2026
1 check passed

This was referenced May 12, 2026

tests: Playwright spec for explorer URL state round-trip (addresses #209) #214

Merged

explorer: round-trip specs for remaining URL state surfaces (follow-up to #209) #215

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

explorer: URL state contract micro-fixes (closes #207)#212

explorer: URL state contract micro-fixes (closes #207)#212
rdhyee merged 2 commits into
isamplesorg:mainfrom
rdhyee:fix/207-url-contract-microfixes

rdhyee commented May 12, 2026

Uh oh!

rdhyee commented May 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

rdhyee commented May 12, 2026

Items

#207 item 4 — low-altitude-without-mode=point round-trip

#207 item 6 — h3 hashchange null-result asymmetry

#207 item 8 — selection state with search-result flights

#207 item 3 — boot/hash hydration shouldn't grow history

Verification

Risks

Test plan

Uh oh!

rdhyee commented May 12, 2026

Codex review applied (commit 5176967)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

#207 item 4 — low-altitude-without-`mode=point` round-trip

#207 item 6 — `h3` hashchange null-result asymmetry

Codex review applied (commit `5176967`)