Skip to content

explorer: facet counts use padded viewport, matching the table (#234)#245

Merged
rdhyee merged 1 commit into
isamplesorg:mainfrom
rdhyee:fix/facet-count-padding
Jul 17, 2026
Merged

explorer: facet counts use padded viewport, matching the table (#234)#245
rdhyee merged 1 commit into
isamplesorg:mainfrom
rdhyee:fix/facet-count-padding

Conversation

@rdhyee

@rdhyee rdhyee commented May 29, 2026

Copy link
Copy Markdown
Contributor

What

Facet-legend counts were computed over the exact viewport (pad 0) in updateCrossFilteredCounts, while every other "in view" surface — the samples-table COUNT (loadCount), the point-mode sample loader, the "samples in view" stat, and the heatmap — pads the viewport by VIEWPORT_PAD_FACTOR (0.3). A sample sitting in that 30% margin was counted by the table but not the facet legend, so the legend read one (or more) low.

Reproduction: /explorer.html?material=…/mineral#v=1&lat=35.0900&lng=32.8900&alt=50000&mode=point → the table says "14 samples match the current filters" but the mineral facet shows 13.

Verified against live data (DuckDB queries at that exact view): the facet query returns 13 at pad 0 and 14 at pad 0.3 (== the table). Exactly one mineral sample lives in the pad margin.

Fix

One line in updateCrossFilteredCounts — align the facet-count bbox to the shared contract:

- const bboxSQL = isGlobal ? null : viewerBboxSQL('l.latitude', 'l.longitude', 0);
+ const bboxSQL = isGlobal ? null : viewerBboxSQL('l.latitude', 'l.longitude', VIEWPORT_PAD_FACTOR);

The heatmap hit the identical mismatch earlier and was moved to the padded contract in the #241 follow-up; this aligns the last stray surface.

The durable fix — one shared "in view" bbox source so these surfaces can't drift apart again — is noted as architectural follow-up on #234.

Tests (tests/playwright/facet-viewport.spec.js)

  • New coherence regression: active-material legend count == table "N match" count at the repro view (backreference assertion — 14/14 passes, 13/14 fails).
  • Hardened flyToAndSettle: the padded (slower) facet query reliably exposed a pre-existing race — the helper waited only for .recomputing to clear, which can be a transient before the new counts apply. It now keys on the full recompute cycle (.recomputing appear → clear) plus a two-consecutive-equal-reads stability gate, so it's correct even for moves that leave counts unchanged. This also confirms there is no real product stale-overwrite race — the restore settles correctly on global.
  • Full B1 suite green 2× each (10/10).

Provenance

Claude; empirically verified (DuckDB 13/14 + Playwright); Codex 2-round review (resolved an unchanged-counts hang in the test helper). Deployed to the rdhyee fork staging and verified live (mineral legend now reads 14, matching the table).

🤖 Generated with Claude Code

rdhyee added a commit that referenced this pull request Jun 18, 2026
…#292)

The Material tree shipped with STATIC global baseline counts; this makes them live
when the user zooms in. Map/table filtering was already live — this is the legend.

- describeCrossFilters: Material participates via materialSelection() (minimal nodes)
  ONLY when zoomed (!isGlobalView); at/near global it stays baseline.
- buildCrossFilterWhere: Material cross-filters OTHER dims via a membership
  pid-subquery (concept_uri IN selected) — a selected parent matches its subtree.
- updateCrossFilteredCounts: cube disabled in tree mode (no tree nodes in the cube);
  Material own-counts at global → baseline (instant); zoomed → membership query
  COUNT(DISTINCT pid) per concept_uri, scoped by bbox (lite JOIN) + other dims
  (facets_v3 pid-subquery) + search.

GLOBAL GATE (perf): the membership COUNT(DISTINCT) is a near-full scan at global/
large views and starved the single DuckDB-WASM connection (samples-table query
timed out). At true-global the baseline IS the correct global count, so we use it
(instant); live counts engage when zoomed, where the bbox prunes the scan.

Verified (202608): earthmaterial 4,091,133 global → 25,988 at Cyprus; legend(node)
== table(node filter) = 26,310 (coherence #245); parent>=child in-viewport; source⇄
material cross-filter both directions. 8 facet-tree specs + smoke green; render clean.
Codex: no blocking findings (verified cross-filter via direct DuckDB queries).

Accepted residual (documented): at global view WITH another filter/search, Material
counts show baseline (instant, slightly stale) — a precomputed facet_tree_cross_filter
cube is the follow-up if live global cross-filtered material counts are wanted.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lesorg#234)

Facet-legend counts were computed over the EXACT viewport (pad 0) in
updateCrossFilteredCounts, while every other "in view" surface — the
samples-table COUNT (loadCount), the point-mode sample loader, the
"samples in view" stat, and the heatmap — pads by VIEWPORT_PAD_FACTOR
(0.3). A matching sample sitting in the 30% pad margin was counted by the
table but not the legend, so the legend read one (or more) low. RY hit it
2026-05-28: material=mineral at a Cyprus deep-zoom showed 13 on the
legend but "14 samples match the current filters" in the table.

Verified against the live data: the facet query returns 13 at pad 0 and
14 at pad 0.3 (== the table). Fix: align the facet-count bbox to
VIEWPORT_PAD_FACTOR. The heatmap hit the identical mismatch earlier and
was moved to the padded contract in the isamplesorg#241 follow-up; this aligns the
last stray surface. (The durable fix — one shared "in view" bbox source so
these can't drift again — is tracked on isamplesorg#234.)

Tests (tests/playwright/facet-viewport.spec.js):
- New coherence regression: active-material legend count == table
  "N match" count at the exact repro view (backreference assertion).
- Hardened flyToAndSettle: the padded (slower) facet query reliably
  exposed a pre-existing race — the helper waited only for `.recomputing`
  to clear, which can be a transient before the new counts apply. It now
  keys on the full recompute cycle (`.recomputing` appear → clear) plus a
  two-consecutive-equal-reads stability gate, so it's correct even for
  moves that leave counts unchanged. Confirms there is no real product
  stale-overwrite race (the restore settles on global).
- Full B1 suite green 2x each (10/10).

Provenance: Claude; empirically verified (DuckDB 13/14 + Playwright);
Codex 2-round review (resolved the unchanged-counts hang in the helper).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rdhyee
rdhyee force-pushed the fix/facet-count-padding branch from c4aafcb to 32f6b79 Compare June 30, 2026 21:43
@rdhyee

rdhyee commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto upstream/main (was 41 commits behind). Finding: this PR's actual fix is already shipped. The conflict in explorer.qmd was at the exact viewerBboxSQL('l.latitude', 'l.longitude', ...) line this PR changes from pad 0 to VIEWPORT_PAD_FACTOR — upstream/main already reads VIEWPORT_PAD_FACTOR there, shipped independently via #251 ("explorer: search as a real global filter (#234 step 4)", May 31) a few days after this branch forked (May 28). After resolving in favor of upstream's version, explorer.qmd is now byte-identical to upstream/main — confirmed with git diff upstream/main -- explorer.qmd (empty).

What's left and still valuable: the new Playwright regression test in tests/playwright/facet-viewport.spec.js ("coherence: active-material legend count == table 'N match' count") plus the hardened flyToAndSettle helper (keys on the full .recomputing appear→clear cycle + a two-consecutive-equal-reads stability gate, replacing a flaky fixed-timeout wait). Those survived the rebase as the only remaining diff vs upstream/main (41 insertions / 5 deletions, test file only) and are worth keeping as a regression guard for the now-shipped fix.

Tests: npm run test:unit — 13/13 pass. The Playwright spec needs a live dev server with the real explorer page (Cesium globe + live R2 parquet data) — attempted it against a local quarto preview, but #cesiumContainer never became visible within the 30s timeout in this sandbox (no GPU/WebGL + likely network constraints), so I couldn't execute it end-to-end here. Recommend running it in CI or a normal dev machine before merge.

Pushed the rebased branch. Not auto-marking ready for review — flagging the already-shipped finding for you to decide whether to close this PR (superseded by #251) or merge it anyway for the regression test.

@rdhyee rdhyee added the CC+Codex+LGTM Claude and Codex both reached LGTM; ready for human review label Jul 17, 2026
@rdhyee

rdhyee commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

🤖 rbotyee (Claude, operated by @rdhyee) — Raymond's intent: closing-week PR sweep — merge if a fresh Codex review still says LGTM against today's main. What I did: ran that review; verdict below; merging.

Codex re-review vs current main (2026-07-17): LGTM, 0 findings

Notable: the runtime padding fix this PR was written for has since landed on main independentlyupdateCrossFilteredCounts() already uses VIEWPORT_PAD_FACTOR, and all count-bearing "in view" surfaces are consistently padded (verified against the post-#300 / post-#335 code). What this PR still contributes is the Playwright regression spec (5 facet-viewport tests), which Codex verified still matches today's moveStart/moveEnd event contract. Merging for the test coverage.

@rdhyee
rdhyee merged commit 097fe26 into isamplesorg:main Jul 17, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CC+Codex+LGTM Claude and Codex both reached LGTM; ready for human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant