From 32f6b79b8107fc0b014865c5c3bc903fcf97c5a7 Mon Sep 17 00:00:00 2001 From: Raymond Yee Date: Thu, 28 May 2026 18:00:31 -0700 Subject: [PATCH] explorer: facet counts use padded viewport, matching the table (#234) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 #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) --- tests/playwright/facet-viewport.spec.js | 46 ++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 5 deletions(-) diff --git a/tests/playwright/facet-viewport.spec.js b/tests/playwright/facet-viewport.spec.js index 5d330d97..a029c665 100644 --- a/tests/playwright/facet-viewport.spec.js +++ b/tests/playwright/facet-viewport.spec.js @@ -107,8 +107,24 @@ async function readFacetCounts(page, facet) { * populates at boot. */ async function readSourceCounts(page) { return readFacetCounts(page, 'source'); } -/** flyTo a destination, then wait for the post-moveEnd debounce + bbox query - * to settle. Uses a zero-duration flight to make the test deterministic. */ +/** flyTo a destination, then wait for the facet counts to actually reflect + * the new viewport — not merely for `.recomputing` to clear. + * + * Hardened 2026-05-28: the old wait (`waitForTimeout(50)` + one + * `.recomputing`-clear check) could observe a transient "no-recomputing" + * instant *before* the new viewport's counts applied, or capture a stale + * prior-viewport result. It was already intermittently flaky on baseline; + * the #234 padding fix (facet counts now scan the 30%-padded bbox, ~1.69× + * the rows → slower query) widened the race and made it fail reliably. + * + * Robust synchronization keys on the recompute CYCLE (not a value change, + * which would hang for moves that leave the counts unchanged): wait for + * `.recomputing` to (a) APPEAR — so we can't sample the pre-query + * transient — then (b) CLEAR (query applied), then (c) hold steady across + * two consecutive reads, so a late-landing query can't shift the values + * after we return. If a real product stale-overwrite race exists, the + * steady state will be the wrong value and the caller's assertion will + * (correctly) fail rather than flake. */ async function flyToAndSettle(page, lat, lng, alt) { await page.evaluate(async ({ lat, lng, alt }) => { const v = await window._ojs?.ojsConnector?.mainModule?.value('viewer'); @@ -118,10 +134,30 @@ async function flyToAndSettle(page, lat, lng, alt) { duration: 0, }); }, { lat, lng, alt }); - // Give the debounce + query a chance to land. waitForFacetCountsStable - // is the real synchronization point; this just kicks the event loop. - await page.waitForTimeout(50); + // Synchronize on the recompute CYCLE, not on a value change — so this + // helper is correct even for moves that leave the counts unchanged + // (Codex review 2026-05-28). `moveStart` sets `.recomputing` + // synchronously and the 250 ms debounce keeps it observable; wait for + // it to APPEAR so we can't sample the pre-query transient, then for it + // to CLEAR (query applied). The `.catch` tolerates the rare case where + // the cycle is missed/instant — we still fall through to the stability + // gate below rather than hang. + await page.waitForFunction( + () => document.querySelector('.facet-count.recomputing') !== null, + null, { timeout: 10000 } + ).catch(() => {}); await waitForFacetCountsStable(page); + // Stability gate: two consecutive equal source-count reads, so a + // late-landing query can't shift the values after we return. If a real + // product stale-overwrite race existed, the steady state would be the + // wrong value and the caller's assertion would (correctly) fail. + let prev = null; + await expect.poll(async () => { + const cur = JSON.stringify(await readSourceCounts(page)); + const settled = cur === prev; + prev = cur; + return settled; + }, { timeout: 30000, intervals: [400, 600, 800] }).toBe(true); } function totalOf(counts) {