explorer: add camera.moveEnd URL-write backstop (closes #204)#205
Merged
Merged
Conversation
isamplesorg#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-isamplesorg#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-isamplesorg#203 it failed on iter 2). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 11, 2026
Closed
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>
4 tasks
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>
This was referenced May 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #204. Follow-up to #203.
Problem
#203 fixed the await-blocking case for Bug A but the harness against the deployed site revealed a residual: small camera moves below Cesium's
percentageChanged = 0.1threshold don't firecamera.changed, so the URL write is never scheduled. A user who pans a small distance and immediately copies the URL gets stale state. Empirically reproduced on iter 2 ofurl_roundtrip_investigation.js(Δlat ≈ 0.02° pan only, no zoom).Fix
Add
viewer.camera.moveEndas a parallel URL-write trigger. It fires once per discrete settled camera move regardless of magnitude — covers drag-pan mouseup, wheel-stop, flight-complete on flyTo. The existingcamera.changedhandler keeps its role (mode transitions, resolution loads); this just adds a lightweight URL-only backstop.The
_suppressHashWritegate already handles the hashchange-flight path, so no fight with back/forward navigation.Verification
tests/playwright/url_roundtrip_investigation.jsagainst localhost on this branch — all 6 iterations pass (cf. iter 2 still failing on the post-#203 deploy).Risks
camera.changedandmoveEndwill both fire for above-threshold moves. Both arereplaceState(no history-entry growth) and write the same value at settle, so the second write is idempotent. Acceptable cost for the small-move coverage.-triggered writes**:moveEndfires on programmatic flights too. The_suppressHashWriteflag is set during hashchange flights (explorer.qmd:2139`), so back/forward hydration paths are still respected. Other flights (click handlers, source-filter rebuild) intentionally want the URL updated — same as today's behavior.Test plan
🤖 Generated with Claude Code