tests: Playwright spec for explorer URL state round-trip (addresses #209)

[rdhyee]

Addresses #209 (does NOT fully close — see #215 for the rest of the checklist). CI-safe Playwright specs covering the URL state contract slices fixed by PRs #203, #205, #210, #212.

What this adds

New file: tests/playwright/url-roundtrip.spec.js. Five tests:

#	Test	Regression for
1	Point-mode deep-link with mode=point enters point mode	Bug B fix in #203 (omitted heading)
2	Low-alt deep-link WITHOUT mode=point enters point mode	#207 item 4 (altitude-authoritative)
3	Sub-threshold pan updates URL hash via moveEnd	#205 (percentageChanged suppressed events)
4	URL round-trips across browser contexts	#203 + #205 combined
5	h3 hashchange with unknown cell clears _globeState.selectedH3	#207 item 6 (boot/hashchange asymmetry)

What's NOT covered

Tracked in follow-up #215:

• pid deep-link + active filters
• valid h3 deep-link round-trip with cluster-card hydration
• search / search_scope round-trip
• facet filters (?sources=, ?material=, etc.)
• view=table mode restore
• search-result flight (camera + selection)

Out of scope:

• Search-result row click selection (explorer: URL state contract micro-fixes (post-#203 follow-ups) #207 item 8) — needs FTS fixture data.
• Cold-cache live-site investigation — stays in tests/playwright/url_roundtrip_investigation.js (manual diagnostic, too slow/network-sensitive for CI).

Codex round-1 review applied

This PR went through one Codex review round; changes in 7436c75 address it:

• Replaced fixed waitForTimeout sleeps with waitForFunction polling URL hash (tests 3 + 4) — thin timing margin over flight + moveEnd debounce.
• Wrapped test 4 context creation in try/finally so ctxA/ctxB close on mid-test assertion failure.
• Moved 6-minute test.setTimeout(360000) from describe-default (180s) to per-test override on the round-trip test only — single broken test no longer waits 6 minutes to fail.
• Added waitForBootSettled helper (waits for viewer._suppressHashWrite === false) — _globeState.mode initializes at explorer.qmd:871, but the hashchange listener is registered at :2210, so waitForMode alone isn't enough.
• Title changed from closes #209 to addresses #209; opened explorer: round-trip specs for remaining URL state surfaces (follow-up to #209) #215 for the rest of the checklist.
• Header inconsistency cleaned up. Comment added in snapshot() noting _globeState.selectedH3/selectedPid field coupling.

Disagreement with Codex retained, documented in code:

• waitForPointModeSettled keeps the 'Click one for details' substring match rather than the count-phrase pattern Codex suggested — count phrase misses the cap-reached done-state branch at explorer.qmd:1611 ("<n> samples in view (showing <m> — zoom in for more). Click one for details." — no "individual" word). 'Click one for details' is the common denominator across both done-state branches.
• Test 5 retained the hashchange-driven form. 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 firing. Codex's "trivially true" framing missed that line 2272 fires first. (Tried seeding selectedH3 before the hashchange for an even stronger assertion; seed survived the handler unexpectedly — suspected OJS reactivity around mainModule.value('viewer') returning a different reference than the handler's closure-captured viewer. Documented in the test comment.)

Verification

All 5 tests pass against localhost (Quarto preview of current main) in ~1m 24s:

✓  1 deep-link with mode=point enters point mode (Bug B from #203) (13.0s)
✓  2 deep-link with low altitude AND no mode=point still enters point mode (#207 item 4) (10.5s)
✓  3 sub-threshold pan updates URL hash via moveEnd (#205) (17.0s)
✓  4 URL round-trips across browser contexts (#203 + #205 combined) (38.9s)
✓  5 h3 hashchange with unknown cell clears selectedH3 (#207 item 6) (1.4s)

🤖 Generated with Claude Code

[rdhyee]

Codex review — round 1

Self-contained Codex review (no shared context with our session). Pasting verbatim per the iterative-review pattern.

---

Summary

The specs are useful, but I would not approve this as-is if the PR title/body says closes #209. The biggest test-design issue is Test 5: it can pass without proving that stale selectedH3 state was cleared.

Per-ask response

1. h3 hashchange spec assertion strength

tests/playwright/url-roundtrip.spec.js:190-213 is too weak. It starts with an invalid h3, so selectedH3 can be null from boot and remain null; that does not prove the hashchange null-result branch clears prior state.

Stronger version: start with a known valid h3, wait until selectedH3 is non-null and preferably canonicalized, then mutate to invalid h3 and assert it clears:

await page.goto(`${EXPLORER_PATH}#v=1&lat=${LAT}&lng=${LNG}&alt=${ALT_CLUSTER}&h3=${VALID_H3}`);
await waitForMode(page, 'cluster');
await page.waitForFunction(async () => {
  const v = await window._ojs.ojsConnector.mainModule.value('viewer');
  return !!v._globeState.selectedH3;
});

await page.evaluate((badH3) => {
  const params = new URLSearchParams(location.hash.slice(1));
  params.set('h3', badH3);
  params.set('heading', '5.0');
  location.hash = '#' + params.toString();
}, '0baadbeeffffffff');

await page.waitForFunction(async () => {
  const v = await window._ojs.ojsConnector.mainModule.value('viewer');
  return v._globeState.selectedH3 === null;
});

That would actually catch a regression in the explorer.qmd:2278-2288 null-result branch.

2. requestRenderMode = false

This is probably OK because Playwright gives each test its own page/context, and Test 4 creates separate contexts explicitly. It should not leak across tests under the current config.

I would still extract a local helper like flyCameraToAndWaitForHash(...), mostly to centralize the pattern and avoid future drift. Cleanup is not necessary unless the same page is reused inside a test.

3. waitForPointModeSettled text match

Yes, tests/playwright/url-roundtrip.spec.js:53-61 makes user-facing copy load-bearing. A copy edit to explorer.qmd:1610-1612 would break these tests without changing behavior.

Better signals:

• expose a test-only/state flag such as viewer._globeState.pointModeSettled
• wait on data/state rather than prose, e.g. viewer._globeState.mode === 'point' plus viewport sample data present
• if staying DOM-based, match the more semantic count phrase already used elsewhere: \d[\d,]*\s+individual\s+samples

4. Timeout

test.setTimeout(360000) at url-roundtrip.spec.js:88 is too broad. The multi-context test may need a high cap, but the h3 invalid-cell test should not be allowed to hang for 6 minutes.

Recommendation:

• keep describe default closer to 120s or 180s
• use test.setTimeout(360000) only inside the round-trip/context-heavy test
• pass tighter helper timeouts in the tests that should settle quickly

5. Helper duplication

Reasonable for this PR, with one caveat. The existing helper file is itself a spec, not a clean support module, so importing from it would be awkward and could accidentally register tests.

I would endorse local duplication for now, but if a third Explorer spec needs waitForMode, phase-message history, or camera flight helpers, move them into tests/playwright/helpers/explorer.js.

6. "closes #209" correctness

I do not think closes #209 is correct.

From the eight listed #209 checklist items, this PR fully covers only:

• sub-threshold pan settles via moveEnd

It partially touches:

• cold-cache / low-alt point-mode deep-link, though the file comment says it intentionally does not try cold-cache deep-links
• h3 behavior, but not the requested valid h3 deep-link round-trip with cluster card hydration and URL persistence

Uncovered:

• pid deep-link + active filters
• valid h3 deep-link round-trip
• search + search_scope
• facet filters
• view=table
• search-result flight

So I would change this to addresses #209 or refs #209, and open/fill a follow-up issue for the remaining matrix.

7. Test 4 race

Yes, url-roundtrip.spec.js:169 should poll the URL hash rather than sleep. The implementation has immediate moveEnd hash writing at explorer.qmd:2117-2120, but timing still depends on Cesium flight completion and render scheduling.

Use waitForFunction:

await pageA.waitForFunction(
  ({ lat, lng }) => {
    const params = new URLSearchParams(location.hash.slice(1));
    return Math.abs(parseFloat(params.get('lat')) - lat) < 0.001 &&
           Math.abs(parseFloat(params.get('lng')) - lng) < 0.001;
  },
  { lat: newLat, lng: newLng },
  { timeout: 10000 }
);

Same improvement applies to Test 3 at url-roundtrip.spec.js:138.

8. selectedPid / selectedH3 field names

They are canonical today; explorer.qmd initializes and writes them directly in many places. But the test is coupled to internal state, not public UI.

Worth a short comment in snapshot() saying these fields are intentionally asserted as the URL-selection backing state. That makes future refactors update the tests consciously instead of treating breakage as mysterious.

9. Other findings

url-roundtrip.spec.js:120 stores before but never uses it. Drop it or assert that the URL changed from before.hash.

url-roundtrip.spec.js:151-187 should close contexts in finally; right now a mid-test assertion failure leaks ctxA/ctxB until Playwright cleanup.

The file header is internally inconsistent: it says the tests “don’t try to drive cold-cache deep-links” at url-roundtrip.spec.js:7-9, while the timeout comment at url-roundtrip.spec.js:86-87 cites cold-cache point-mode boot as the reason for 6 minutes. Tighten that wording.

Verdict

BLOCK

[rdhyee]

Codex review — round 2

Verbatim Codex round-2 response (no shared context). Round-2 prompt argued the original test-5 assertion was regression-tight via line 2272's unconditional write — Codex pushed back with a sharper, correct argument: the boot deep-link path's own null-result branch (line 2728) already cleared selectedH3 before the test's hashchange fires, so the post-mutation 'wait for null' is satisfied by boot, not by the handler under test. Applying their fix in a follow-up commit.

---

Summary

Still BLOCK on Test 5. I agree with the production-code trace, but not with the test synchronization claim.

Per-round-2-ask response

1. Test 5 re-verification

Your trace of explorer.qmd:2270-2285 is correct: line 2272 unconditionally writes selectedH3 = state.h3.toLowerCase(), so if the handler runs and line 2285 is removed, the final value should remain the new invalid H3.

The problem is the spec can still pass before the hashchange handler runs. After boot, the initial invalid h3 path has already cleared selectedH3 to null. Then the test mutates location.hash and immediately waits for selectedH3 === null, which is already true. So the assertion does not prove the post-hashchange null branch executed.

Suggested fix: after mutating the hash, wait for a handler-only side effect first, e.g. camera heading near 5.0, then assert selectedH3 === null. Setting the URL hash alone will not rotate the camera; the handler’s flyTo does.

2. Click one for details

Agree with the author. The /individual samples/ regex misses the cap-reached done branch at explorer.qmd:1610-1612.

A slightly stronger no-production-change version would be matching both done messages:

/(\d[\d,]* individual samples|\d[\d,]* samples in view .*zoom in for more\)).*Click one for details\./

But the trailing phrase is acceptable as the common done-state signal.

3. waitForBootSettled

For this test flow, yes: waiting for viewer._suppressHashWrite === false after boot is a reliable “listener registered + initial deep-link handling finished” signal. The true/false cycling on later hashchanges does not matter because this helper is only used before the test-driven hash mutation.

4. Per-test timeout

Yes, test.setTimeout(360000) inside the test body is valid Playwright idiom for a per-test override. test.use(...) is not the right mechanism.

5. waitForHashLatLng

The polling helper is a good replacement for fixed sleep. 10s is reasonable for a 1s flight plus URL-write debounce.

Minor nit: Test 4 changes altitude too, but the helper only waits on lat/lng. Since buildHash() writes lat/lng/alt together, this is probably fine, but adding optional altitude matching would make the round-trip assertion tighter.

6. Other

No other round-2 regressions found.

Verdict

BLOCK

[rdhyee]

Codex review — round 3

Verbatim Codex round-3 response. LGTM, ready to merge.

---

Summary

The round-3 fix addresses the round-2 blocker. The heading gate is a valid handler-only signal, and with the deliberately malformed 16-character H3, the null-clear continuation should complete well before the heading wait can pass.

Per-ask response

1. Test 5 regression-tightness verification

Yes, this is regression-tight against removing explorer.qmd:2285.

The key point is that boot starts with no heading, so readHash() defaults heading to 0, and boot uses camera.setView, not the hashchange flyTo. The later heading=5.0 mutation can only be applied by the hashchange handler at explorer.qmd:2220-2227.

If viewer._globeState.selectedH3 = null at explorer.qmd:2285 is removed, the handler still sets selectedH3 = '0baadbeeffffffff' at explorer.qmd:2272, then fetchClusterByH3 returns null, and nothing clears it. The final snapshot() would see the non-empty string, not null.

2. Race between heading and selectedH3 clear

Not a real concern for this exact test data.

0baadbeeffffffff is 16 chars, so fetchClusterByH3 fails the 15-char regex at explorer.qmd:2134 before any DB await. Because it is an async function, the handler continuation after await fetchClusterByH3(...) resumes on a microtask. The camera needs animation frames to approach 5 degrees, so the null-clear should happen long before the heading wait passes.

No extra microtask flush or second wait is needed.

3. Test 4 alt-in-URL assertion

Looks fine. buildHash rounds altitude to an integer, so altEps=100 is much wider than rounding error and still tight enough to reject the stale boot altitude (62054) versus 9500.

4. Anything else

No additional blocker or nit in the round-3 diff.

Verdict

LGTM, ready to merge