Uh oh!
There was an error while loading. Please reload this page.
explorer: B1 viewport-aware facet counts (#234 step 3) - #237
Merged
Conversation
The facet legend now scopes per-source / per-material / per-context / per-object_type counts to the current viewport bbox instead of the whole dataset. Pan or zoom → counts recompute. Resolves the silent disagreement between the legend and the "Samples in View" / table-count surfaces. What changed - New `isGlobalView()` helper: altitude shortcut (h > 10,000 km) plus a rect-saturation check on `paddedViewportBounds`. Returns true on the initial zoomed-out view so the legend doesn't jiggle on tiny rotations. - `updateCrossFilteredCounts` snapshots `bboxSQL` once at function entry. Cube fast-path is gated additionally on `bboxSQL === null` — the pre-aggregated cube has no spatial granularity, so it's invalid for any non-global view. The no-filter + non-global case now falls through to the slow path (B1's whole point: "what's in view" even with zero facet filters). - Slow path: when `bboxSQL` is set, JOIN `facets_url f` to `lite_url l` on `l.pid = f.pid` and apply the bbox predicate against `(l.latitude, l.longitude)`. `buildCrossFilterWhere` accepts an optional column prefix so the JOIN-shape WHERE qualifies columns with `f.` to avoid ambiguity with `lite_url.source`. - `viewer.camera.moveStart` → synchronous `markFacetCountsRecomputing()` so the legend shows italic-stale state the instant the user starts panning, instead of waiting for the 250 ms debounce. Cleared when `applyFacetCounts` writes the post-moveEnd query result. - `viewer.camera.moveEnd` → `refreshFacetCounts()`. Reuses the existing `facetCountsReqId` + 250 ms debounce, so bursts of moveEnd coalesce to one query and any superseded in-flight query discards its result on `await` resume via the stale guard. - On query error: existing `applyFacetCounts(key, null)` baseline fallback covers the Q3 decision (show globals, not error indicator). Q1 / Q2 / Q3 plan decisions (locked with rdhyee before implementation) - Q1: JOIN to `lite_url` for this PR; no existing parquet carries both the four facet columns and lat/lon. If real-world latency is bad, follow-up to bake a pre-joined facet+coords parquet. - Q2: Single-pass full query (no coarse-then-fine progressive scaffold yet — defer until latency is shown to need it). - Q3: On bbox-query throw, fall back to global baseline counts. Tests - New `tests/playwright/facet-viewport.spec.js`: 3 specs covering zoom-in-then-out (counts shrink, then restore to within 1 %), moveStart-sync `.recomputing` affordance, and the negative control that a cold global-view boot leaves no `.recomputing` stuck. - Regression: facetnote-url-load (2), search-real-count (3), url-roundtrip (5) all pass — 13 tests green locally. Known follow-ups - Latency was not formally benchmarked; the JOIN turns 4 parallel GROUP-BY queries into 4 parallel JOIN+GROUP-BY queries against the lite parquet (≈300 MB). If real-world UX feels slow, bake a pre-joined facets-with-coords parquet and route the bbox path through it (Q1 fallback). - A cancellation-race Playwright spec (two pans back-to-back inside the 250 ms window) is conceptually covered by the existing `facetCountsReqId` stale guard but not yet exercised by a test — add if Codex review flags it. Closes the B1 row in isamplesorg#234.
…#237 Codex caught three real issues + one documentation polish. 1. (BLOCKING) moveStart did not supersede in-flight or debounced facet count work. An older `updateCrossFilteredCounts` could land mid-pan, pass the stale guard at its `await` resume, call `applyFacetCounts`, and CLEAR `.recomputing` while writing pre-move counts — leaving the legend looking authoritative but stale until moveEnd's 250 ms debounce fired the new query. Fix: the moveStart listener now `clearTimeout(facetCountsDebounce); ++facetCountsReqId;` in addition to marking the italic-stale class. moveEnd's `refreshFacetCounts()` bumps the id AGAIN, harmlessly — supersession of already-superseded work, debounce coalesced. 2. (TEST GAP) The PR exercised the no-filter bbox path but NOT the filter-active JOIN path — exactly the new SQL shape this change introduces. Added a 4th Playwright spec `bbox-aware count under an active source filter (JOIN + filter)`: - activate a single source filter (cube fast-path at global view) - capture filtered material counts at global view (cube-derived) - fly to Cyprus → forces slow path with `f.source IN (…)` + JOIN - assert material counts shrink (a silent fallback to baseline on a column-ambiguity SQL error would write global counts, which would GROW back to the un-filtered globals — caught by the `cyprusTotal < filteredTotal` assertion) 3. (TOLERANCE TOO LOOSE) `zoom in → zoom out → restore` accepted a 1 % delta on the restored totals, but at `alt=15e6` the altitude shortcut in `isGlobalView()` forces the no-bbox baseline path → the counts should be value-by-value EXACTLY equal to the first global capture. Switched to `expect(restoredCounts).toEqual(globalCounts)`. A loose tolerance could hide a stale-read race writing a bbox-scoped count that just happens to look "close enough." 4. (POLISH) Sharpened the `moveStart` listener comment to explain why bumping the id is necessary (was missing the rationale). Test results: 4/4 new + 10/10 regression (facetnote-url-load, search-real-count, url-roundtrip) — 14 green locally. Codex's point about the altitude-shortcut UX (high-altitude pivot returns "global" even if camera angle doesn't show the full data extent) is an intentional product decision per the design rationale in `isGlobalView()`'s comment; nothing to change there.
rdhyee
commented
May 25, 2026
ContributorAuthor
Codex review iteration log (2 rounds → LGTM)
Round 2: LGTM (Codex)
Final test count: 14 passed locally (4 new B1 specs + 10 regression: 2 facetnote-url-load + 3 search-real-count + 5 url-roundtrip). Known follow-ups deferred per Q1/Q2 plan decisions, not raised by Codex:
Thanks codex for the round-1 catch on moveStart — the stale-and-not-italic interval was a real bug, not a theoretical one. |
rdhyee added a commit
to rdhyee/isamplesorg.github.io
that referenced
this pull request
May 27, 2026
Uh oh!
There was an error while loading. Please reload this page.
4 tasks
rdhyee added a commit
to rdhyee/isamplesorg.github.io
that referenced
this pull request
May 27, 2026
Adds `tests/playwright/helpers/url.js` exposing `siteUrl(path, suffix?)`
and `explorerUrl(suffix?)`. Migrates all 8 specs in tests/playwright/
to use them, replacing the hand-rolled BASE_URL/EXPLORER_PATH
constants and the two patterns that previously coexisted:
- 4 specs (cesium-queries, explorer-helper, explorer-layout-stability,
explorer-map-overlay) string-concatenated `${BASE_URL}${PATH}`.
Works on sub-path TEST_URLs only when TEST_URL has NO trailing
slash; a trailing slash produces `//explorer.html`.
- 4 specs (facet-viewport, facetnote-url-load, search-real-count,
url-roundtrip) called `page.goto('/explorer.html...')` and relied
on Playwright's `baseURL` config. Playwright resolves an absolute
path against the ORIGIN of baseURL, dropping any sub-path. So
`TEST_URL=https://rdhyee.github.io/isamplesorg.github.io/
npx playwright test` silently 404'd on those 4 specs.
The helper:
const BASE_URL = (process.env.TEST_URL || 'http://localhost:5860')
.replace(/\/$/, '');
function siteUrl(path, suffix = '') {
return `${BASE_URL}${path}${suffix}`;
}
function explorerUrl(suffix = '') {
return siteUrl('/explorer.html', suffix);
}
Trailing-slash strip means the helper produces the same URL whether
TEST_URL ends in `/` or not — eliminating a foot-gun any operator
running the suite against staging would otherwise hit.
Verified: 4/4 facet-viewport tests pass on localhost in isolation
(same as pre-PR baseline). The other migrated specs have pre-existing
failures unrelated to this change (proven by running an explorer-helper
test against the pre-migration spec via `git stash` — same 2/4 fail).
History: the URL bug was discovered 2026-05-27 verifying PR isamplesorg#237
against rdhyee fork staging. The original fix in this PR patched only
facet-viewport.spec.js; Codex round-1 review recommended extracting a
shared helper rather than duplicating the fix into every spec, since
3 other specs in the suite had the same latent bug. This commit
adopts that recommendation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>rdhyee added a commit
that referenced
this pull request
May 27, 2026
#238) Adds `tests/playwright/helpers/url.js` exposing `siteUrl(path, suffix?)` and `explorerUrl(suffix?)`. Migrates all 8 specs in tests/playwright/ to use them, replacing the hand-rolled BASE_URL/EXPLORER_PATH constants and the two patterns that previously coexisted: - 4 specs (cesium-queries, explorer-helper, explorer-layout-stability, explorer-map-overlay) string-concatenated `${BASE_URL}${PATH}`. Works on sub-path TEST_URLs only when TEST_URL has NO trailing slash; a trailing slash produces `//explorer.html`. - 4 specs (facet-viewport, facetnote-url-load, search-real-count, url-roundtrip) called `page.goto('/explorer.html...')` and relied on Playwright's `baseURL` config. Playwright resolves an absolute path against the ORIGIN of baseURL, dropping any sub-path. So `TEST_URL=https://rdhyee.github.io/isamplesorg.github.io/ npx playwright test` silently 404'd on those 4 specs. The helper: const BASE_URL = (process.env.TEST_URL || 'http://localhost:5860') .replace(/\/$/, ''); function siteUrl(path, suffix = '') { return `${BASE_URL}${path}${suffix}`; } function explorerUrl(suffix = '') { return siteUrl('/explorer.html', suffix); } Trailing-slash strip means the helper produces the same URL whether TEST_URL ends in `/` or not — eliminating a foot-gun any operator running the suite against staging would otherwise hit. Verified: 4/4 facet-viewport tests pass on localhost in isolation (same as pre-PR baseline). The other migrated specs have pre-existing failures unrelated to this change (proven by running an explorer-helper test against the pre-migration spec via `git stash` — same 2/4 fail). History: the URL bug was discovered 2026-05-27 verifying PR #237 against rdhyee fork staging. The original fix in this PR patched only facet-viewport.spec.js; Codex round-1 review recommended extracting a shared helper rather than duplicating the fix into every spec, since 3 other specs in the suite had the same latent bug. This commit adopts that recommendation. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 27, 2026
This was referenced Jun 15, 2026
Merged
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 freeto 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.
Summary
The facet legend now scopes per-source / per-material / per-context / per-object_type counts to the current map viewport bbox instead of the whole dataset. Pan or zoom → counts recompute. This resolves the silent disagreement between the legend ("global pivot tool") and the rest of the page ("Samples in View", table count, map dots) that the 2026-05-22 design session called out and that #234 picks as direction B1.
This is step 3 of the #234 roadmap, after #235 (facetNote URL-load, merged) and #236 (search real-count, merged). Subsequent steps (A1 search-as-global-filter, C3 auto-promote to point mode) are independent and build on this.
Decisions locked with @rdhyee before implementation
facets_urltolite_urlonpidfor this PR; follow-up to bake a pre-joined parquet if real-world latency proves bad.facetCountsReqIdinfrastructure. Defer coarse-then-fine scaffold until latency shows it's needed.applyFacetCounts(key, null)— matches current no-filter behavior; user can re-pan to retry.Architectural changes
isGlobalView()helper (altitude shortcut + rect-saturation check) decides when the cube fast-path stays valid. At alt > 10,000 km the camera is treated as global regardless ofcomputeViewRectangleper-angle quirks, so legend totals don't jiggle on tiny camera rotations near the default zoom-out.updateCrossFilteredCountssnapshotsbboxSQLonce at function entry. Cube fast-path gated additionally onbboxSQL === null. The no-filter + non-global case now falls through to the slow path (B1's whole point: counts reflect what's in view even with zero facet filters).bboxSQLset):JOIN facets_url f ↔ lite_url l ON l.pid = f.pidand apply the bbox predicate against(l.latitude, l.longitude).buildCrossFilterWheretakes an optional column-prefix arg so the JOIN-shape WHERE qualifies columns withf.to avoid ambiguity withlite_url.source.viewer.camera.moveStart→ synchronousmarkFacetCountsRecomputing(). Italic-stale legend appears the instant the user starts panning, cleared whenapplyFacetCountswrites the post-moveEnd result.viewer.camera.moveEnd→refreshFacetCounts(). Reuses the existing 250 ms debounce +facetCountsReqIdstale guard — bursts coalesce to one query, superseded in-flight queries discard their result onawaitresume.Test plan
quarto render explorer.qmd— passestests/playwright/facet-viewport.spec.js— 3 specs pass locally.recomputingsynchronously (verified via in-page listener that runs after the explorer's).recomputingstuckfacetnote-url-load.spec.js(2),search-real-count.spec.js(3),url-roundtrip.spec.js(5) — all pass (10/10)git diff --check upstream/main...HEAD— cleanKnown follow-ups
GROUP BYqueries into 4 parallelJOIN+GROUP BYqueries against the ≈300 MBlite_urlparquet. If UX feels slow in real use, follow-up to bake a pre-joined facets-with-coords parquet and route the bbox path through it (Q1 fallback).facetCountsReqIdstale guard is in place but not exercised by a Playwright test (two pans back-to-back inside the 250 ms window). Add if Codex flags it as a coverage gap.Cross-refs
Roadmap context: #234. Direction is A1 + B1 + (C3-when-feasible, C2-with-warning) with progressive refinement underlying the dynamic surfaces.