From 3b3555b30999b59a809d802ddd3acc8f8e706329 Mon Sep 17 00:00:00 2001 From: Raymond Yee Date: Mon, 11 May 2026 22:13:13 -0700 Subject: [PATCH 1/3] tests: Playwright spec for explorer URL state round-trip (closes #209) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- tests/playwright/url-roundtrip.spec.js | 215 +++++++++++++++++++++++++ 1 file changed, 215 insertions(+) create mode 100644 tests/playwright/url-roundtrip.spec.js diff --git a/tests/playwright/url-roundtrip.spec.js b/tests/playwright/url-roundtrip.spec.js new file mode 100644 index 0000000..87446b9 --- /dev/null +++ b/tests/playwright/url-roundtrip.spec.js @@ -0,0 +1,215 @@ +/** + * Explorer URL state round-trip regressions (closes #209). + * + * CI-safe spec set extracted from `url_roundtrip_investigation.js` (which + * remains as a manual diagnostic targeting the live deploy). These tests + * run against `localhost:5860` (Quarto preview) and cover the URL state + * contract slices fixed in PRs #203, #205, #210, #212. They don't try + * to drive cold-cache deep-links — that's slow and network-sensitive and + * stays in the investigation harness. + * + * Coverage matrix: + * - Point-mode deep-link with `mode=point` (Bug B fix from #203) + * - Low-alt deep-link WITHOUT `mode=point` (#207 item 4) + * - Sub-threshold pan settles via `moveEnd` (#205) + * - Round-trip across contexts: copy URL, paste in fresh context, + * same camera/mode (#203/#205/#207 combined) + * - `h3` hashchange null-result clears `_globeState.selectedH3` + * (#207 item 6) + * + * Search-result row click selection (#207 item 8) needs working FTS + * data; deferred to a manual smoke or a separate spec once a fixture + * dataset is available. + */ + +const { test, expect } = require('@playwright/test'); + +const EXPLORER_PATH = '/explorer.html'; + +// Cyprus / Polis — confirmed dense region (~23k samples), used in #206/#210. +const LAT = 34.9957; +const LNG = 33.6798; +const ALT_POINT = 8000; // < ENTER_POINT_ALT = 120000 → point mode +const ALT_POINT_DEEP = 62054; +const ALT_CLUSTER = 500000; +const ENTER_POINT_ALT = 120000; + +/** Wait until `viewer._globeState.mode` equals `expected`. + * Cold-cache point-mode boot can take 60–90s per #190; allow 3 minutes. */ +async function waitForMode(page, expected, timeoutMs = 180000) { + await page.waitForFunction( + async (expectedMode) => { + try { + const v = await window._ojs?.ojsConnector?.mainModule?.value('viewer'); + return v?._globeState?.mode === expectedMode; + } catch { return false; } + }, + expected, + { timeout: timeoutMs } + ); +} + +/** Wait until `loadViewportSamples` has settled into the point-mode done message. */ +async function waitForPointModeSettled(page, timeoutMs = 120000) { + await page.waitForFunction( + () => { + const msg = document.getElementById('phaseMsg')?.textContent || ''; + return msg.includes('Click one for details'); + }, + null, + { timeout: timeoutMs } + ); +} + +/** Snapshot camera + mode + selection + URL hash from the live page. */ +async function snapshot(page) { + return await page.evaluate(async () => { + const v = await window._ojs?.ojsConnector?.mainModule?.value('viewer'); + const carto = v?.camera?.positionCartographic; + if (!carto) return null; + return { + url: location.href, + hash: location.hash, + lat: Cesium.Math.toDegrees(carto.latitude), + lng: Cesium.Math.toDegrees(carto.longitude), + alt: carto.height, + mode: v._globeState.mode, + selectedPid: v._globeState.selectedPid || null, + selectedH3: v._globeState.selectedH3 || null, + }; + }); +} + +// ---- Tests ---------------------------------------------------------------- + +test.describe('Explorer URL state round-trip (issue #209)', () => { + // Cold-cache point-mode deep-link can take 60–90s per #190; round-trip + // tests open multiple fresh contexts, each paying the cache cost. + test.setTimeout(360000); + + + test('deep-link with mode=point enters point mode (Bug B from #203)', async ({ page }) => { + const url = `${EXPLORER_PATH}#v=1&lat=${LAT}&lng=${LNG}&alt=${ALT_POINT_DEEP}&mode=point`; + await page.goto(url); + await waitForMode(page, 'point'); + await waitForPointModeSettled(page); + const s = await snapshot(page); + expect(s.mode).toBe('point'); + expect(Math.abs(s.alt - ALT_POINT_DEEP)).toBeLessThan(100); + }); + + test('deep-link with low altitude AND no mode=point still enters point mode (#207 item 4)', async ({ page }) => { + // No `mode=point` in URL. Boot should enter point based on altitude alone. + const url = `${EXPLORER_PATH}#v=1&lat=${LAT}&lng=${LNG}&alt=${ALT_POINT}`; + await page.goto(url); + // Wait for the settled point-mode done message — more reliable than + // waitForMode alone, which can match a transient mode flip during boot + // (the dual-mode-state anomaly being fixed in #208). + await waitForPointModeSettled(page); + const s = await snapshot(page); + expect(s.mode).toBe('point'); + expect(s.alt).toBeLessThan(ENTER_POINT_ALT); + }); + + test('sub-threshold pan updates URL hash via moveEnd (#205)', async ({ page }) => { + // Start at a settled point-mode view. + const url = `${EXPLORER_PATH}#v=1&lat=${LAT}&lng=${LNG}&alt=${ALT_POINT_DEEP}&mode=point`; + await page.goto(url); + await waitForMode(page, 'point'); + await waitForPointModeSettled(page); + const before = await snapshot(page); + + // Programmatically drive a SMALL pan (Δlat ≈ 0.02°). The pan is small + // enough that `camera.percentageChanged = 0.1` may not raise + // `camera.changed`, but `moveEnd` always fires once on flight complete. + const newLat = LAT + 0.02; + const newLng = LNG + 0.02; + await page.evaluate(async ({ lat, lng, alt }) => { + const v = await window._ojs.ojsConnector.mainModule.value('viewer'); + v.scene.requestRenderMode = false; // keep render loop alive in headless + v.camera.cancelFlight(); + v.camera.flyTo({ + destination: Cesium.Cartesian3.fromDegrees(lng, lat, alt), + duration: 1.0, + }); + }, { lat: newLat, lng: newLng, alt: ALT_POINT_DEEP }); + + // Wait flight duration + moveEnd debounce + await page.waitForTimeout(2500); + + const after = await snapshot(page); + // URL hash must reflect the new lat/lng — Bug A residual fixed by #205. + const params = new URLSearchParams(after.hash.slice(1)); + const urlLat = parseFloat(params.get('lat')); + const urlLng = parseFloat(params.get('lng')); + expect(Math.abs(urlLat - newLat)).toBeLessThan(0.001); + expect(Math.abs(urlLng - newLng)).toBeLessThan(0.001); + }); + + test('URL round-trips across browser contexts (#203 + #205 combined)', async ({ browser }) => { + // Context A: navigate, settle, programmatically pan + zoom, capture URL. + const ctxA = await browser.newContext(); + const pageA = await ctxA.newPage(); + await pageA.goto(`${EXPLORER_PATH}#v=1&lat=${LAT}&lng=${LNG}&alt=${ALT_POINT_DEEP}&mode=point`); + await waitForMode(pageA, 'point'); + await waitForPointModeSettled(pageA); + + const newLat = LAT + 0.01; + const newLng = LNG - 0.01; + const newAlt = 9500; // still below ENTER_POINT_ALT + await pageA.evaluate(async ({ lat, lng, alt }) => { + const v = await window._ojs.ojsConnector.mainModule.value('viewer'); + v.scene.requestRenderMode = false; + v.camera.cancelFlight(); + v.camera.flyTo({ + destination: Cesium.Cartesian3.fromDegrees(lng, lat, alt), + duration: 1.0, + }); + }, { lat: newLat, lng: newLng, alt: newAlt }); + await pageA.waitForTimeout(2500); + + const snapA = await snapshot(pageA); + expect(snapA.mode).toBe('point'); + + // Context B: open the captured URL, verify camera/mode round-trip. + const ctxB = await browser.newContext(); + const pageB = await ctxB.newPage(); + await pageB.goto(snapA.url); + await waitForPointModeSettled(pageB); + const snapB = await snapshot(pageB); + + expect(Math.abs(snapA.lat - snapB.lat)).toBeLessThan(0.001); + expect(Math.abs(snapA.lng - snapB.lng)).toBeLessThan(0.001); + expect(Math.abs(snapA.alt - snapB.alt)).toBeLessThan(50); + expect(snapB.mode).toBe('point'); + + await ctxA.close(); + await ctxB.close(); + }); + + test('h3 hashchange with invalid cell clears selectedH3 (#207 item 6)', async ({ page }) => { + // Boot at a cluster altitude with a deliberately invalid h3. + const invalidH3 = '0deadbeefffffff'; // not a real h3 cell + await page.goto(`${EXPLORER_PATH}#v=1&lat=${LAT}&lng=${LNG}&alt=${ALT_CLUSTER}&h3=${invalidH3}`); + // Wait for boot. Use mode-resolve as a proxy for "the cell init has run." + await waitForMode(page, 'cluster'); + await page.waitForTimeout(2000); + + // Drive a hashchange to a different invalid h3, then check that + // _globeState.selectedH3 was reset (the hashchange null-branch fix). + await page.evaluate((newH3) => { + const u = new URL(location.href); + const params = new URLSearchParams(u.hash.slice(1)); + params.set('h3', newH3); + // bump heading to force hashchange to fire (same hash wouldn't) + params.set('heading', '5.0'); + location.hash = '#' + params.toString(); + }, '0baadbeeffffffff'); + await page.waitForTimeout(3000); // hashchange flight (1.5s) + handler awaits + + const s = await snapshot(page); + // _globeState.selectedH3 should have been cleared by the null-result + // branch (no matching cluster found for the malformed h3). + expect(s.selectedH3).toBeNull(); + }); +}); From 7436c75d673b397d2226eb4d8001a98a1c6757e0 Mon Sep 17 00:00:00 2001 From: Raymond Yee Date: Tue, 12 May 2026 12:07:17 -0700 Subject: [PATCH 2/3] tests: address Codex round-1 review on URL roundtrip specs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- tests/playwright/url-roundtrip.spec.js | 210 ++++++++++++++++--------- 1 file changed, 137 insertions(+), 73 deletions(-) diff --git a/tests/playwright/url-roundtrip.spec.js b/tests/playwright/url-roundtrip.spec.js index 87446b9..44b2e16 100644 --- a/tests/playwright/url-roundtrip.spec.js +++ b/tests/playwright/url-roundtrip.spec.js @@ -1,12 +1,10 @@ /** - * Explorer URL state round-trip regressions (closes #209). + * Explorer URL state round-trip regressions (addresses #209). * * CI-safe spec set extracted from `url_roundtrip_investigation.js` (which * remains as a manual diagnostic targeting the live deploy). These tests * run against `localhost:5860` (Quarto preview) and cover the URL state - * contract slices fixed in PRs #203, #205, #210, #212. They don't try - * to drive cold-cache deep-links — that's slow and network-sensitive and - * stays in the investigation harness. + * contract slices fixed in PRs #203, #205, #210, #212. * * Coverage matrix: * - Point-mode deep-link with `mode=point` (Bug B fix from #203) @@ -17,9 +15,11 @@ * - `h3` hashchange null-result clears `_globeState.selectedH3` * (#207 item 6) * - * Search-result row click selection (#207 item 8) needs working FTS - * data; deferred to a manual smoke or a separate spec once a fixture - * dataset is available. + * These specs use the Quarto preview against warm-cache artifacts. + * Cold-cache live-site behavior and the rest of issue #209's checklist + * (`pid`+filters, `search`/`search_scope`, facet filters, `view=table`, + * search-result flight) stay in `url_roundtrip_investigation.js` and a + * follow-up issue. */ const { test, expect } = require('@playwright/test'); @@ -49,7 +49,14 @@ async function waitForMode(page, expected, timeoutMs = 180000) { ); } -/** Wait until `loadViewportSamples` has settled into the point-mode done message. */ +/** Wait until `loadViewportSamples` has settled into the point-mode done message. + * Matches on the trailing "Click one for details" phrase — it's the common + * denominator across both done-state phaseMsg branches at explorer.qmd:1610-1612 + * (normal: `" individual samples. Click one for details."` and cap-reached: + * `" samples in view (showing m — zoom in for more). Click one for details."`). + * Codex review (PR #214 round 1) suggested switching to the count phrase + * `\d[\d,]*\s+individual\s+samples`, but that pattern misses the cap-reached + * branch which has no "individual" word, so we stay with the trailing phrase. */ async function waitForPointModeSettled(page, timeoutMs = 120000) { await page.waitForFunction( () => { @@ -61,7 +68,46 @@ async function waitForPointModeSettled(page, timeoutMs = 120000) { ); } -/** Snapshot camera + mode + selection + URL hash from the live page. */ +/** Wait for boot to complete enough that the `hashchange` listener (registered + * late in the explorer cell at explorer.qmd:2210) is installed. `_globeState` + * is initialized very early (line 871), so `waitForMode` alone is not enough + * to guarantee the listener exists. `_suppressHashWrite` flips to false either + * via zoomWatcher init or at the end of the boot deep-link section (line 2734); + * by the time it's false, the hashchange listener is definitely registered. */ +async function waitForBootSettled(page, timeoutMs = 180000) { + await page.waitForFunction( + async () => { + try { + const v = await window._ojs?.ojsConnector?.mainModule?.value('viewer'); + return v?._suppressHashWrite === false; + } catch { return false; } + }, + null, + { timeout: timeoutMs } + ); +} + +/** Wait until the URL hash's lat/lng match the expected values within `eps`. + * Replaces fixed sleeps with a precise settle condition for moveEnd-driven + * hash writes (regression for #205). */ +async function waitForHashLatLng(page, expectedLat, expectedLng, eps = 0.001, timeoutMs = 10000) { + await page.waitForFunction( + ({ lat, lng, eps }) => { + const params = new URLSearchParams(location.hash.slice(1)); + const ul = parseFloat(params.get('lat')); + const un = parseFloat(params.get('lng')); + return Number.isFinite(ul) && Number.isFinite(un) + && Math.abs(ul - lat) < eps && Math.abs(un - lng) < eps; + }, + { lat: expectedLat, lng: expectedLng, eps }, + { timeout: timeoutMs } + ); +} + +/** Snapshot camera + mode + selection + URL hash from the live page. + * `selectedPid` and `selectedH3` are intentionally read from `viewer._globeState` + * — that's the canonical URL-selection backing state today. If a future refactor + * renames or relocates these fields, this spec is the place to update. */ async function snapshot(page) { return await page.evaluate(async () => { const v = await window._ojs?.ojsConnector?.mainModule?.value('viewer'); @@ -83,9 +129,9 @@ async function snapshot(page) { // ---- Tests ---------------------------------------------------------------- test.describe('Explorer URL state round-trip (issue #209)', () => { - // Cold-cache point-mode deep-link can take 60–90s per #190; round-trip - // tests open multiple fresh contexts, each paying the cache cost. - test.setTimeout(360000); + // Default per-test cap: point-mode boot in headless can take 60–90s per #190. + // The multi-context round-trip test overrides this to a longer cap below. + test.setTimeout(180000); test('deep-link with mode=point enters point mode (Bug B from #203)', async ({ page }) => { @@ -117,7 +163,6 @@ test.describe('Explorer URL state round-trip (issue #209)', () => { await page.goto(url); await waitForMode(page, 'point'); await waitForPointModeSettled(page); - const before = await snapshot(page); // Programmatically drive a SMALL pan (Δlat ≈ 0.02°). The pan is small // enough that `camera.percentageChanged = 0.1` may not raise @@ -134,82 +179,101 @@ test.describe('Explorer URL state round-trip (issue #209)', () => { }); }, { lat: newLat, lng: newLng, alt: ALT_POINT_DEEP }); - // Wait flight duration + moveEnd debounce - await page.waitForTimeout(2500); - - const after = await snapshot(page); - // URL hash must reflect the new lat/lng — Bug A residual fixed by #205. - const params = new URLSearchParams(after.hash.slice(1)); - const urlLat = parseFloat(params.get('lat')); - const urlLng = parseFloat(params.get('lng')); - expect(Math.abs(urlLat - newLat)).toBeLessThan(0.001); - expect(Math.abs(urlLng - newLng)).toBeLessThan(0.001); + // Poll the URL hash until lat/lng reflect the new pan — replaces a + // fixed 2.5s sleep that was tight against flight-complete + moveEnd + // debounce timing. URL hash settling is what we actually care about. + await waitForHashLatLng(page, newLat, newLng); }); test('URL round-trips across browser contexts (#203 + #205 combined)', async ({ browser }) => { + // Round-trip test opens two fresh contexts, each paying the cold-cache + // point-mode boot cost (60–90s per #190). Override describe-level 180s cap. + test.setTimeout(360000); + // Context A: navigate, settle, programmatically pan + zoom, capture URL. const ctxA = await browser.newContext(); - const pageA = await ctxA.newPage(); - await pageA.goto(`${EXPLORER_PATH}#v=1&lat=${LAT}&lng=${LNG}&alt=${ALT_POINT_DEEP}&mode=point`); - await waitForMode(pageA, 'point'); - await waitForPointModeSettled(pageA); - - const newLat = LAT + 0.01; - const newLng = LNG - 0.01; - const newAlt = 9500; // still below ENTER_POINT_ALT - await pageA.evaluate(async ({ lat, lng, alt }) => { - const v = await window._ojs.ojsConnector.mainModule.value('viewer'); - v.scene.requestRenderMode = false; - v.camera.cancelFlight(); - v.camera.flyTo({ - destination: Cesium.Cartesian3.fromDegrees(lng, lat, alt), - duration: 1.0, - }); - }, { lat: newLat, lng: newLng, alt: newAlt }); - await pageA.waitForTimeout(2500); - - const snapA = await snapshot(pageA); - expect(snapA.mode).toBe('point'); - - // Context B: open the captured URL, verify camera/mode round-trip. - const ctxB = await browser.newContext(); - const pageB = await ctxB.newPage(); - await pageB.goto(snapA.url); - await waitForPointModeSettled(pageB); - const snapB = await snapshot(pageB); - - expect(Math.abs(snapA.lat - snapB.lat)).toBeLessThan(0.001); - expect(Math.abs(snapA.lng - snapB.lng)).toBeLessThan(0.001); - expect(Math.abs(snapA.alt - snapB.alt)).toBeLessThan(50); - expect(snapB.mode).toBe('point'); - - await ctxA.close(); - await ctxB.close(); + let ctxB; + try { + const pageA = await ctxA.newPage(); + await pageA.goto(`${EXPLORER_PATH}#v=1&lat=${LAT}&lng=${LNG}&alt=${ALT_POINT_DEEP}&mode=point`); + await waitForMode(pageA, 'point'); + await waitForPointModeSettled(pageA); + + const newLat = LAT + 0.01; + const newLng = LNG - 0.01; + const newAlt = 9500; // still below ENTER_POINT_ALT + await pageA.evaluate(async ({ lat, lng, alt }) => { + const v = await window._ojs.ojsConnector.mainModule.value('viewer'); + v.scene.requestRenderMode = false; + v.camera.cancelFlight(); + v.camera.flyTo({ + destination: Cesium.Cartesian3.fromDegrees(lng, lat, alt), + duration: 1.0, + }); + }, { lat: newLat, lng: newLng, alt: newAlt }); + + // Wait for the URL hash to reflect the new camera, not a fixed sleep. + await waitForHashLatLng(pageA, newLat, newLng); + + const snapA = await snapshot(pageA); + expect(snapA.mode).toBe('point'); + + // Context B: open the captured URL, verify camera/mode round-trip. + ctxB = await browser.newContext(); + const pageB = await ctxB.newPage(); + await pageB.goto(snapA.url); + await waitForPointModeSettled(pageB); + const snapB = await snapshot(pageB); + + expect(Math.abs(snapA.lat - snapB.lat)).toBeLessThan(0.001); + expect(Math.abs(snapA.lng - snapB.lng)).toBeLessThan(0.001); + expect(Math.abs(snapA.alt - snapB.alt)).toBeLessThan(50); + expect(snapB.mode).toBe('point'); + } finally { + await ctxA.close(); + if (ctxB) await ctxB.close(); + } }); - test('h3 hashchange with invalid cell clears selectedH3 (#207 item 6)', async ({ page }) => { - // Boot at a cluster altitude with a deliberately invalid h3. - const invalidH3 = '0deadbeefffffff'; // not a real h3 cell + test('h3 hashchange with unknown cell clears selectedH3 (#207 item 6)', async ({ page }) => { + // Regression we're testing: the hashchange handler's null-result branch + // at explorer.qmd:2278-2289 must clear `_globeState.selectedH3` when + // `fetchClusterByH3` returns null. If line 2285 were removed, selectedH3 + // would retain the URL's h3 value (set at line 2272) instead of clearing. + // + // Codex review (PR #214 round 1) suggested seeding selectedH3 to a + // non-null sentinel before the hashchange to "prove prior state was + // cleared." We attempted that but the seed survived the hashchange + // unexpectedly (suspected OJS reactivity around `mainModule.value('viewer')` + // returning a different reference than the handler's closure-captured + // viewer). Falling back to the simpler form below, which still exercises + // the regression: line 2272 unconditionally writes 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 executing. + const invalidH3 = '0deadbeefffffff'; // 15 chars but not a real h3 cell await page.goto(`${EXPLORER_PATH}#v=1&lat=${LAT}&lng=${LNG}&alt=${ALT_CLUSTER}&h3=${invalidH3}`); - // Wait for boot. Use mode-resolve as a proxy for "the cell init has run." await waitForMode(page, 'cluster'); - await page.waitForTimeout(2000); + // `_globeState.mode` is initialized at explorer.qmd:871, well before the + // hashchange listener is registered at line 2210; wait for boot settle. + await waitForBootSettled(page); - // Drive a hashchange to a different invalid h3, then check that - // _globeState.selectedH3 was reset (the hashchange null-branch fix). + // Drive a hashchange to a different invalid h3. The handler runs: + // line 2272 sets selectedH3=, line 2273 awaits fetchClusterByH3, + // which returns null (line 2134 — fails 15-char regex), line 2285 clears. await page.evaluate((newH3) => { - const u = new URL(location.href); - const params = new URLSearchParams(u.hash.slice(1)); + const params = new URLSearchParams(location.hash.slice(1)); params.set('h3', newH3); - // bump heading to force hashchange to fire (same hash wouldn't) - params.set('heading', '5.0'); + params.set('heading', '5.0'); // bump heading to force hashchange to fire location.hash = '#' + params.toString(); }, '0baadbeeffffffff'); - await page.waitForTimeout(3000); // hashchange flight (1.5s) + handler awaits + + // Poll for `selectedH3 === null` — replaces a fixed 3s sleep. + await page.waitForFunction(async () => { + const v = await window._ojs.ojsConnector.mainModule.value('viewer'); + return v._globeState.selectedH3 === null; + }, null, { timeout: 10000 }); const s = await snapshot(page); - // _globeState.selectedH3 should have been cleared by the null-result - // branch (no matching cluster found for the malformed h3). expect(s.selectedH3).toBeNull(); }); }); From 0a87dee299c8effa819037242f603c13988aa350 Mon Sep 17 00:00:00 2001 From: Raymond Yee Date: Tue, 12 May 2026 12:24:09 -0700 Subject: [PATCH 3/3] tests: address Codex round-2 review on URL roundtrip specs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 2 sharpened the round-1 concern on Test 5 and was right: booting with `&h3=` 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) --- tests/playwright/url-roundtrip.spec.js | 78 +++++++++++++++++--------- 1 file changed, 50 insertions(+), 28 deletions(-) diff --git a/tests/playwright/url-roundtrip.spec.js b/tests/playwright/url-roundtrip.spec.js index 44b2e16..6718478 100644 --- a/tests/playwright/url-roundtrip.spec.js +++ b/tests/playwright/url-roundtrip.spec.js @@ -87,19 +87,28 @@ async function waitForBootSettled(page, timeoutMs = 180000) { ); } -/** Wait until the URL hash's lat/lng match the expected values within `eps`. - * Replaces fixed sleeps with a precise settle condition for moveEnd-driven - * hash writes (regression for #205). */ -async function waitForHashLatLng(page, expectedLat, expectedLng, eps = 0.001, timeoutMs = 10000) { +/** Wait until the URL hash's lat/lng (and optionally alt) match the expected + * values within `eps` / `altEps`. Replaces fixed sleeps with a precise settle + * condition for moveEnd-driven hash writes (regression for #205). */ +async function waitForHashLatLng(page, expectedLat, expectedLng, opts = {}) { + const eps = opts.eps ?? 0.001; + const expectedAlt = opts.alt ?? null; + const altEps = opts.altEps ?? 100; + const timeoutMs = opts.timeoutMs ?? 10000; await page.waitForFunction( - ({ lat, lng, eps }) => { + ({ lat, lng, eps, alt, altEps }) => { const params = new URLSearchParams(location.hash.slice(1)); const ul = parseFloat(params.get('lat')); const un = parseFloat(params.get('lng')); - return Number.isFinite(ul) && Number.isFinite(un) - && Math.abs(ul - lat) < eps && Math.abs(un - lng) < eps; + if (!Number.isFinite(ul) || !Number.isFinite(un)) return false; + if (Math.abs(ul - lat) >= eps || Math.abs(un - lng) >= eps) return false; + if (alt != null) { + const ua = parseFloat(params.get('alt')); + if (!Number.isFinite(ua) || Math.abs(ua - alt) >= altEps) return false; + } + return true; }, - { lat: expectedLat, lng: expectedLng, eps }, + { lat: expectedLat, lng: expectedLng, eps, alt: expectedAlt, altEps }, { timeout: timeoutMs } ); } @@ -212,8 +221,10 @@ test.describe('Explorer URL state round-trip (issue #209)', () => { }); }, { lat: newLat, lng: newLng, alt: newAlt }); - // Wait for the URL hash to reflect the new camera, not a fixed sleep. - await waitForHashLatLng(pageA, newLat, newLng); + // Wait for the URL hash to reflect the new camera (lat + lng + alt) — + // tighter assertion than lat/lng alone, since `buildHash` writes all + // three together. + await waitForHashLatLng(pageA, newLat, newLng, { alt: newAlt }); const snapA = await snapshot(pageA); expect(snapA.mode).toBe('point'); @@ -238,18 +249,21 @@ test.describe('Explorer URL state round-trip (issue #209)', () => { test('h3 hashchange with unknown cell clears selectedH3 (#207 item 6)', async ({ page }) => { // Regression we're testing: the hashchange handler's null-result branch // at explorer.qmd:2278-2289 must clear `_globeState.selectedH3` when - // `fetchClusterByH3` returns null. If line 2285 were removed, selectedH3 - // would retain the URL's h3 value (set at line 2272) instead of clearing. + // `fetchClusterByH3` returns null. // - // Codex review (PR #214 round 1) suggested seeding selectedH3 to a - // non-null sentinel before the hashchange to "prove prior state was - // cleared." We attempted that but the seed survived the hashchange - // unexpectedly (suspected OJS reactivity around `mainModule.value('viewer')` - // returning a different reference than the handler's closure-captured - // viewer). Falling back to the simpler form below, which still exercises - // the regression: line 2272 unconditionally writes 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 executing. + // Codex round-2 review (PR #214) caught a subtle weakness: booting with + // `&h3=` runs the BOOT deep-link path's own null-result branch + // (explorer.qmd:2728), which sets `selectedH3 = null` BEFORE the test + // even drives a hashchange. So a post-hashchange `selectedH3 === null` + // assertion is true regardless of whether the hashchange handler's own + // null-clear branch (line 2285) ran. + // + // Fix: gate the assertion on a handler-only side effect. The hashchange + // handler's `camera.flyTo` (lines 2220-2227) rotates `camera.heading` to + // the URL's `heading` value — a side effect that only the hashchange + // handler produces. Wait for that rotation BEFORE checking selectedH3; + // by then the handler has executed past line 2272 (which writes a new + // non-null selectedH3) AND reached line 2285 (the null-clear branch). const invalidH3 = '0deadbeefffffff'; // 15 chars but not a real h3 cell await page.goto(`${EXPLORER_PATH}#v=1&lat=${LAT}&lng=${LNG}&alt=${ALT_CLUSTER}&h3=${invalidH3}`); await waitForMode(page, 'cluster'); @@ -257,22 +271,30 @@ test.describe('Explorer URL state round-trip (issue #209)', () => { // hashchange listener is registered at line 2210; wait for boot settle. await waitForBootSettled(page); - // Drive a hashchange to a different invalid h3. The handler runs: - // line 2272 sets selectedH3=, line 2273 awaits fetchClusterByH3, - // which returns null (line 2134 — fails 15-char regex), line 2285 clears. + // Drive a hashchange to a DIFFERENT invalid h3, plus an explicit heading + // change to detect that the handler actually ran (boot's heading is 0). await page.evaluate((newH3) => { const params = new URLSearchParams(location.hash.slice(1)); params.set('h3', newH3); - params.set('heading', '5.0'); // bump heading to force hashchange to fire + params.set('heading', '5.0'); // distinctive value — only handler's flyTo writes this location.hash = '#' + params.toString(); }, '0baadbeeffffffff'); - // Poll for `selectedH3 === null` — replaces a fixed 3s sleep. + // Wait for the hashchange handler's `flyTo` to rotate camera heading + // toward 5°. 5° = ~0.0873 radians; wait until heading is within a few + // degrees of that target. This proves the handler executed past line + // 2225 (camera.flyTo with the new heading from the URL). await page.waitForFunction(async () => { const v = await window._ojs.ojsConnector.mainModule.value('viewer'); - return v._globeState.selectedH3 === null; - }, null, { timeout: 10000 }); + const headingDeg = Cesium.Math.toDegrees(v.camera.heading) % 360; + return Math.abs(headingDeg - 5.0) < 1.0; + }, null, { timeout: 15000 }); + // Now the handler has run. Line 2272 wrote `selectedH3` to a non-null + // value from the URL; line 2273 awaited `fetchClusterByH3` (which returns + // null at line 2134 for the malformed 16-char h3); line 2285 cleared it. + // If line 2285 is removed, selectedH3 stays as the URL's invalid h3, NOT + // null, and this assertion catches it. const s = await snapshot(page); expect(s.selectedH3).toBeNull(); });