From 19bad06126de2763207aacf15df264174c40f15a Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Mon, 10 Aug 2026 12:27:54 +1000 Subject: [PATCH 1/4] test(worker): commit the pointer probe and make it a CI gate (LAB-1812) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /map surface has shipped four pointer-interaction bugs green, because a synthetic `element.click()` has no pointer plumbing to break: it never goes through pointerdown -> capture -> click retargeting, never hit-tests through preserveAspectRatio letterboxing, and never produces a `detail > 1`. The only thing that caught them was a browser moving a real mouse, and that probe lived in one agent's worktree — one cleanup away from gone. It now lives in the repo and runs on every PR. `playwright-core` is a devDependency pinned to an exact version so the browser revision CI downloads is the one on your laptop; the production bundle and its runtime dependencies are untouched. The gate shares the `wrangler dev` the /health smoke check already boots rather than paying for a second one, and the browser is cached. Readiness is polled, never slept on, and there are no retries: a probe allowed to pass on its second attempt reports "flaky" as "green". Rewriting the probe found a fifth bug of the same family, at a viewport nobody had opened by hand. `boxAspect()` measured the map box with clientWidth / clientHeight, which round to whole pixels, while `clientToUser()` converts pointer coordinates with the fractional getBoundingClientRect(). At a landscape-phone height the box is 236.4 px and clientHeight calls it 236, so the viewBox was fitted to an aspect the element does not have and every click landed 0.17% off. Both now read the same measurement, which is the only way they cannot disagree. --- .github/workflows/ci.yml | 48 ++++- worker/README.md | 42 ++++- worker/package-lock.json | 80 ++++++++ worker/package.json | 2 + worker/public/map.js | 9 +- worker/test/pointer-probe.mjs | 343 ++++++++++++++++++++++++++++++++++ 6 files changed, 507 insertions(+), 17 deletions(-) create mode 100644 worker/test/pointer-probe.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5c619f4..7966c1a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -52,7 +52,26 @@ jobs: git diff --exit-code -- public/assets/styles.css - name: Apply local D1 migrations run: npm run migrate:local - - name: Smoke-check local /health + # The map's pointer handling cannot be checked by the suite above: a + # synthetic click never exercises pointerdown -> capture -> click + # retargeting, which is how four /map regressions shipped green (LAB-1702). + # The probe drives a real mouse in a real browser, so it needs one. + - name: Cache the probe's browser + uses: actions/cache@caa296126883cff596d87d8935842f9db880ef25 # v5.1.0 + with: + path: ~/.cache/ms-playwright + key: playwright-${{ runner.os }}-${{ hashFiles('worker/package-lock.json') }} + restore-keys: playwright-${{ runner.os }}- + - name: Install the probe's browser + # Pinned by playwright-core's version in package-lock.json — it downloads + # the one browser revision it was built against, never "latest Chrome". + # A cache hit skips the download; the apt deps are not cacheable and run + # either way. No system browser is assumed to exist. + run: npx playwright-core install --with-deps chromium + # One `wrangler dev` serves both gates: /health proves the Worker and its + # seeded D1 are up, and that same server is what the probe drives. Booting + # it twice would buy nothing but a second minute of CI. + - name: Smoke-check local /health, then drive /map with a real pointer shell: bash run: | set -euo pipefail @@ -66,18 +85,27 @@ jobs: } trap cleanup EXIT + # Readiness by polling the endpoint itself — never a fixed sleep, and + # never a retry of the assertions: a probe allowed to pass on the + # second attempt is a gate that reports "flaky" as "green". + response="" for attempt in {1..30}; do if response="$(curl --silent --show-error --fail http://127.0.0.1:8787/health 2>/dev/null)"; then - if [ "$response" = '{"status":"ok","generators":350}' ]; then - exit 0 - fi - echo "Unexpected /health response: $response" >&2 - cat "$log_file" >&2 - exit 1 + break fi + response="" sleep 1 done - echo "Local Worker did not become ready" >&2 - cat "$log_file" >&2 - exit 1 + if [ -z "$response" ]; then + echo "Local Worker did not become ready" >&2 + cat "$log_file" >&2 + exit 1 + fi + if [ "$response" != '{"status":"ok","generators":350}' ]; then + echo "Unexpected /health response: $response" >&2 + cat "$log_file" >&2 + exit 1 + fi + + npm run probe:map diff --git a/worker/README.md b/worker/README.md index 1abdac3..3eea4d6 100644 --- a/worker/README.md +++ b/worker/README.md @@ -134,13 +134,43 @@ and no console errors: the whole-NEM view — so the hand tremor in an ordinary click registered as a drag and the click was suppressed. It is now 4 CSS pixels. +- `boxAspect()` measured the box with `clientWidth`/`clientHeight`, which **round + to whole pixels**, while `clientToUser()` converts with the fractional + `getBoundingClientRect()`. A 236.4 px box read as 236 fitted the viewBox to an + aspect the element does not have — the same letterbox as above, at 0.17%, on + any viewport where the height is not a whole number. Both must be the same + measurement or they disagree by construction. + A synthetic `click` dispatched on a node (lightpanda, `element.click()`) does -**not** exercise `pointerdown` → capture → `click` retargeting, which is exactly -why both survived. Verify map interaction with playwright-core driving the system -Chrome (`executablePath: /usr/bin/google-chrome`) and `page.mouse.*`, and -screenshot it: an SVG geometry bug paints a blank or distorted map while every -DOM assertion passes. This is not wired into CI — it needs a browser dependency -the Worker does not otherwise carry. +**not** exercise `pointerdown` → capture → `click` retargeting, does not hit-test +through letterboxing, and never produces a `detail > 1` — which is exactly why +all of them survived a check that reported 213 markers with the right classes and +no console errors. + +**So a real browser moving a real mouse is a CI gate here**, not an optional +local ritual. `test/pointer-probe.mjs` drives `/map` with `page.mouse.*` through +playwright-core and asserts 12 things about the result: a click opens the station +under the cursor (including the smallest pin on the map), `+`/`−`/wheel zoom, +drag pans without stealing the click or changing the selection, a drag ending on +a pin does not open it, the page still scrolls beside the map, double-clicking a +pin opens it exactly once without zooming, the viewBox aspect matches the box, +and a resize to a landscape-phone height keeps the centre, the zoom and the +aspect. Every wait is on an observable condition — no sleeps, and **no retries**: +a probe allowed to pass on the second attempt reports "flaky" as "green". + +```sh +npx wrangler dev --local # in one shell; the probe needs a server +npm run probe:map # in another — PROBE_URL to point elsewhere +PROBE_HEADED=1 npm run probe:map # watch it drive +``` + +The browser is `playwright-core`'s own pinned chromium (`npx playwright-core +install chromium`), not a system Chrome, so CI and your laptop run the same +revision. In CI it is cached and the probe shares the `wrangler dev` the +`/health` smoke check already boots; the whole gate costs well under a minute. +Screenshots stay a manual diagnostic — an SVG geometry bug paints a blank or +distorted map while every DOM assertion passes, and eyes are still the cheapest +way to see that. Shared page chrome (`$`, `fetchJson`, `showError`, `REGIONS`, `TZ`, the theme toggle) lives in `public/chrome.js` and is imported by both pages; the theme diff --git a/worker/package-lock.json b/worker/package-lock.json index 9bc85a5..0c9030e 100644 --- a/worker/package-lock.json +++ b/worker/package-lock.json @@ -17,6 +17,7 @@ "@cloudflare/workers-types": "^5.20260721.1", "@tailwindcss/cli": "^4.3.3", "daisyui": "^5.7.0", + "playwright-core": "1.62.1", "tailwindcss": "^4.3.3", "typescript": "^5.8.3", "vitest": "^4.1.10", @@ -2371,6 +2372,72 @@ "node": ">=14.0.0" } }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/@emnapi/core": { + "version": "1.11.1", + "dev": true, + "inBundle": true, + "license": "MIT", + "optional": true, + "dependencies": { + "@emnapi/wasi-threads": "1.2.2", + "tslib": "^2.4.0" + } + }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/@emnapi/runtime": { + "version": "1.11.1", + "dev": true, + "inBundle": true, + "license": "MIT", + "optional": true, + "dependencies": { + "tslib": "^2.4.0" + } + }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/@emnapi/wasi-threads": { + "version": "1.2.2", + "dev": true, + "inBundle": true, + "license": "MIT", + "optional": true, + "dependencies": { + "tslib": "^2.4.0" + } + }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/@napi-rs/wasm-runtime": { + "version": "1.1.4", + "dev": true, + "inBundle": true, + "license": "MIT", + "optional": true, + "dependencies": { + "@tybys/wasm-util": "^0.10.1" + }, + "funding": { + "type": "github", + "url": "https://github.com/sponsors/Brooooooklyn" + }, + "peerDependencies": { + "@emnapi/core": "^1.7.1", + "@emnapi/runtime": "^1.7.1" + } + }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/@tybys/wasm-util": { + "version": "0.10.2", + "dev": true, + "inBundle": true, + "license": "MIT", + "optional": true, + "dependencies": { + "tslib": "^2.4.0" + } + }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/tslib": { + "version": "2.8.1", + "dev": true, + "inBundle": true, + "license": "0BSD", + "optional": true + }, "node_modules/@tailwindcss/oxide-win32-arm64-msvc": { "version": "4.3.3", "resolved": "https://registry.npmjs.org/@tailwindcss/oxide-win32-arm64-msvc/-/oxide-win32-arm64-msvc-4.3.3.tgz", @@ -3296,6 +3363,19 @@ "url": "https://github.com/sponsors/jonschlinkert" } }, + "node_modules/playwright-core": { + "version": "1.62.1", + "resolved": "https://registry.npmjs.org/playwright-core/-/playwright-core-1.62.1.tgz", + "integrity": "sha512-wPYSwEBJY9GHraISXqyqtx0na0LpO3XEX7jNDhntbex7tzUS7kLnZsOlFruFJB4Hi/rhDMjXGqHewDZ68nYZVw==", + "dev": true, + "license": "Apache-2.0", + "bin": { + "playwright-core": "cli.js" + }, + "engines": { + "node": ">=20" + } + }, "node_modules/postcss": { "version": "8.5.25", "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.5.25.tgz", diff --git a/worker/package.json b/worker/package.json index 4548d41..a58f245 100644 --- a/worker/package.json +++ b/worker/package.json @@ -10,6 +10,7 @@ "deploy": "wrangler deploy", "check": "tsc --noEmit && tsc --noEmit -p test", "test": "vitest run", + "probe:map": "node test/pointer-probe.mjs", "seed:generate": "node scripts/generate-seed.mjs", "refresh:generate": "node scripts/refresh-generators.mjs", "refresh:check": "node scripts/refresh-generators.mjs --self-check", @@ -28,6 +29,7 @@ "@cloudflare/workers-types": "^5.20260721.1", "@tailwindcss/cli": "^4.3.3", "daisyui": "^5.7.0", + "playwright-core": "1.62.1", "tailwindcss": "^4.3.3", "typescript": "^5.8.3", "vitest": "^4.1.10", diff --git a/worker/public/map.js b/worker/public/map.js index dd72a8d..942b537 100644 --- a/worker/public/map.js +++ b/worker/public/map.js @@ -135,7 +135,14 @@ function boxAspect() { // exactly the letterboxing this function exists to prevent. Only a zero // dimension (no layout yet) gets a fallback, and 4/3 is just something // finite to survive on until the resize listener supplies the truth. - const { clientWidth: w, clientHeight: h } = svg; + // + // Measured with getBoundingClientRect, NOT clientWidth/clientHeight, because + // those two round to whole pixels: at a 380 px viewport the box is 236.4 px + // tall and `clientHeight` calls it 236, so the viewBox gets fitted to an + // aspect the element does not have — a 0.17% letterbox, found by the pointer + // probe. It has to be the same measurement clientToUser() uses or the two + // disagree by construction, which is this bug's entire family. + const { width: w, height: h } = svg.getBoundingClientRect(); return w > 0 && h > 0 ? w / h : 4 / 3; } diff --git a/worker/test/pointer-probe.mjs b/worker/test/pointer-probe.mjs new file mode 100644 index 0000000..299315f --- /dev/null +++ b/worker/test/pointer-probe.mjs @@ -0,0 +1,343 @@ +#!/usr/bin/env node +/** + * Real-pointer probe for the station map (`/map`). + * + * WHY THIS EXISTS, and why the 217 vitest specs do not cover it: every bug this + * catches is a pointer-plumbing bug, and a synthetic `element.click()` does not + * have pointer plumbing. It dispatches one `click` straight at a node — it never + * goes through `pointerdown` -> pointer capture -> `pointerup` -> `click` + * retargeting, never hit-tests through `preserveAspectRatio` letterboxing, and + * never produces a `detail > 1`. Four shipped `/map` regressions (LAB-1702) were + * invisible to a DOM-assertion check that happily reported 213 markers with the + * right classes and no console errors on a page where clicking a station did + * nothing at all. Only a real browser moving a real mouse sees them. + * + * Runs headlessly against a live Worker (`wrangler dev --local` in CI). Set + * PROBE_URL to point it elsewhere; PROBE_HEADED=1 to watch it drive. + * + * Determinism rules for anything added here: never sleep, always wait on an + * observable condition, and never assume which marker is where — pick targets by + * hit-testing with `elementFromPoint` so a facilities-snapshot refresh cannot + * silently turn an assertion into a no-op. + */ +import { chromium } from 'playwright-core'; + +const BASE = process.env.PROBE_URL ?? 'http://127.0.0.1:8787'; +const HEADED = process.env.PROBE_HEADED === '1'; +/** Wide enough for the two-column layout, short enough that the page scrolls. */ +const VIEWPORT = { width: 1280, height: 800 }; +/** Landscape-phone height: the map box renders under the 240 px that boxAspect() + * used to floor at, which is the `boxAspect` boundary CodeRabbit found. */ +const SHORT_VIEWPORT = { width: 900, height: 380 }; + +const results = []; +let failed = 0; + +async function check(name, fn) { + try { + await fn(); + results.push(` ok ${name}`); + } catch (err) { + failed += 1; + results.push(` FAIL ${name}\n ${err instanceof Error ? err.message : String(err)}`); + } +} + +function assert(condition, message) { + if (!condition) throw new Error(message); +} + +/* ------------------------------------------------------------------- helpers */ + +/** The map's live geometry, read from the DOM the browser actually laid out. */ +const geometry = (page) => + page.evaluate(() => { + const svg = document.getElementById('map'); + const [x, y, w, h] = svg.getAttribute('viewBox').split(' ').map(Number); + const rect = svg.getBoundingClientRect(); + return { x, y, w, h, cx: x + w / 2, cy: y + h / 2, rect, box: rect.width / rect.height }; + }); + +/** + * Pick a marker we can actually hit: `elementFromPoint` at its centre must + * return the marker itself. Markers are painted biggest-first so small ones stay + * clickable, but a small station can still sit under a large one — asserting on a + * covered marker would test the wrong element and pass for the wrong reason. + * `order: 'smallest'` picks the tiniest hittable pin, which is the case that + * actually broke (a 6 px dot is not a pointer target). + */ +function pickMarker(page, { order = 'largest', exclude = null } = {}) { + return page.evaluate( + ({ order, exclude }) => { + const map = document.getElementById('map').getBoundingClientRect(); + const markers = [...document.querySelectorAll('circle.marker')] + .filter((m) => m.dataset.code !== exclude) + .map((m) => ({ node: m, r: m.getBoundingClientRect().width / 2 })) + .sort((a, b) => (order === 'smallest' ? a.r - b.r : b.r - a.r)); + for (const { node, r } of markers) { + const rect = node.getBoundingClientRect(); + const x = rect.x + rect.width / 2; + const y = rect.y + rect.height / 2; + // Inside the map box, or elementFromPoint answers about the page chrome + // (or nothing at all, for a marker scrolled out of the viewport). + if (x < map.x || x > map.right || y < map.y || y > map.bottom) continue; + if (document.elementFromPoint(x, y) !== node) continue; + return { code: node.dataset.code, name: node.querySelector('title').textContent, x, y, r }; + } + return null; + }, + { order, exclude }, + ); +} + +/** A point over bare basemap — no marker under it, so a drag or double-click + * there exercises the pan/zoom path and not the drill-down. */ +function pickEmptyPoint(page) { + return page.evaluate(() => { + const rect = document.getElementById('map').getBoundingClientRect(); + for (let fx = 0.2; fx <= 0.8; fx += 0.05) { + for (let fy = 0.2; fy <= 0.8; fy += 0.05) { + const x = rect.x + rect.width * fx; + const y = rect.y + rect.height * fy; + const hit = document.elementFromPoint(x, y); + if (hit && !hit.classList.contains('marker')) return { x, y }; + } + } + return null; + }); +} + +const selectedCode = (page) => + page.evaluate(() => document.querySelector('circle.marker.selected')?.dataset.code ?? null); + +const panelHeading = (page) => + page.evaluate(() => document.querySelector('#panel-body h2')?.textContent ?? null); + +/** Ready = the join ran, markers are in the DOM, and the view has been set. + * This is the only "wait" in the probe; everything after it waits on the + * specific thing it is about to assert on. */ +async function openMap(page) { + await page.goto(`${BASE}/map`, { waitUntil: 'domcontentloaded' }); + await page.waitForFunction( + () => + document.querySelectorAll('circle.marker').length > 0 && + document.getElementById('map').hasAttribute('viewBox'), + null, + { timeout: 30_000 }, + ); +} + +async function drag(page, from, to) { + await page.mouse.move(from.x, from.y); + await page.mouse.down(); + // Stepped so `pointermove` fires repeatedly and the 4 px drag slop is really + // crossed — a single jump can be delivered as one move and is not a drag. + await page.mouse.move(to.x, to.y, { steps: 12 }); + await page.mouse.up(); +} + +/* -------------------------------------------------------------------- probes */ + +async function main() { + const browser = await chromium.launch({ headless: !HEADED }); + const context = await browser.newContext({ viewport: VIEWPORT }); + // Every wait here is on a condition the gesture causes within a frame or two, + // so 10 s is generous — but it is what bounds a red build: at playwright's + // 30 s default, a broken click makes CI sit for minutes before saying so. + context.setDefaultTimeout(10_000); + const page = await context.newPage(); + + // Every /api/v2/values call is a station drill-down fetch. Counting them is how + // "one gesture selects once" is checked: the double-click bug fired the + // 24-hour fetch twice for a single gesture. + let valuesFetches = 0; + page.on('request', (req) => { + if (req.url().includes('/api/v2/values')) valuesFetches += 1; + }); + const crashes = []; + page.on('pageerror', (err) => crashes.push(err.message)); + + try { + await openMap(page); + + // 1. The headline regression: setPointerCapture() retargeted `pointerup` AND + // `click` to the , so the marker's own listener never ran and + // clicking a station did nothing. + let clicked; + await check('click on a marker opens that station', async () => { + clicked = await pickMarker(page, { order: 'largest' }); + assert(clicked, 'no hittable marker found'); + await page.mouse.click(clicked.x, clicked.y); + await page.waitForFunction((code) => new URLSearchParams(location.search).get('station') === code, clicked.code); + assert((await selectedCode(page)) === clicked.code, `selected ${await selectedCode(page)}, expected ${clicked.code}`); + assert(await panelHeading(page), 'drill-down panel stayed empty'); + }); + + // 2. Same path, smallest pin on the map: the drag threshold was ~1.6 px, so + // the hand tremor in a click on a small target read as a drag and was + // suppressed. Small stations are exactly the ones you click to identify. + await check('click on the smallest marker opens that station, not a neighbour', async () => { + const small = await pickMarker(page, { order: 'smallest', exclude: clicked.code }); + assert(small, 'no second hittable marker found'); + assert(small.r * 2 <= 24, `"smallest" marker is ${(small.r * 2).toFixed(1)} px wide — pick logic is wrong`); + await page.mouse.click(small.x, small.y); + await page.waitForFunction((code) => new URLSearchParams(location.search).get('station') === code, small.code); + assert((await selectedCode(page)) === small.code, 'a different station was selected'); + }); + + // 3-4. The visible, keyboard-reachable controls. They are the primary way in; + // wheel-only zoom was the revision Ray rejected. + await check('the + button zooms in', async () => { + const before = await geometry(page); + await page.click('#zoom-in'); + await page.waitForFunction((w) => Number(document.getElementById('map').getAttribute('viewBox').split(' ')[2]) < w, before.w); + }); + + await check('the − button zooms out', async () => { + const before = await geometry(page); + await page.click('#zoom-out'); + await page.waitForFunction((w) => Number(document.getElementById('map').getAttribute('viewBox').split(' ')[2]) > w, before.w); + }); + + // 5. Plain wheel, no modifier. The ctrl/⌘ requirement was a real fix applied + // in the wrong place; no map on the web works that way. + await check('an unmodified wheel zooms the map', async () => { + const empty = await pickEmptyPoint(page); + assert(empty, 'no bare-basemap point found'); + const before = await geometry(page); + await page.mouse.move(empty.x, empty.y); + await page.mouse.wheel(0, -240); + await page.waitForFunction((w) => Number(document.getElementById('map').getAttribute('viewBox').split(' ')[2]) < w, before.w); + }); + + // 6. Pan. Window-level pointermove/up replaced pointer capture, so this is + // the assertion that the replacement actually works. + await check('dragging pans the view', async () => { + await page.click('#reset-view'); + const empty = await pickEmptyPoint(page); + const before = await geometry(page); + await drag(page, empty, { x: empty.x - 160, y: empty.y }); + const after = await geometry(page); + const moved = after.cx - before.cx; + const expected = (160 / before.rect.width) * before.w; + assert(moved > 0, `drag left should move the view east; centre moved ${moved.toFixed(3)}`); + assert( + Math.abs(moved - expected) / expected < 0.25, + `pan moved ${moved.toFixed(3)} user units, expected ~${expected.toFixed(3)} — pointer coordinates are being mistranslated`, + ); + }); + + // 7. The other half of the drag contract: a drag that happens to finish over + // a pin must not open it. (`suppressClick`.) + await check('a drag ending on a marker does not select it', async () => { + const before = await selectedCode(page); + const target = await pickMarker(page, { order: 'largest', exclude: before }); + assert(target, 'no marker to drag onto'); + await drag(page, { x: target.x + 120, y: target.y + 60 }, { x: target.x, y: target.y }); + assert((await selectedCode(page)) === before, `a pan opened ${await selectedCode(page)}`); + }); + + // 8. The scroll-trap guard is CSS (the map is capped at min(62vh, 34rem)), so + // there is always page around it. Verified by scrolling with the cursor + // beside the map rather than by asserting on the cap. + await check('the page still scrolls with the cursor outside the map', async () => { + await page.evaluate(() => scrollTo(0, 0)); + const outside = await page.evaluate(() => { + const rect = document.getElementById('map').getBoundingClientRect(); + return { x: rect.x + rect.width / 2, y: Math.max(4, rect.y / 2) }; + }); + await page.mouse.move(outside.x, outside.y); + await page.mouse.wheel(0, 300); + await page.waitForFunction(() => scrollY > 0); + }); + + // 9. A viewBox whose aspect differs from the element's letterboxes under + // preserveAspectRatio and silently offsets EVERY pointer coordinate — the + // bug behind clicks landing on the wrong station. + await check('the viewBox aspect matches the box aspect', async () => { + const g = await geometry(page); + assert( + Math.abs(g.w / g.h - g.box) < 1e-4, + `viewBox aspect ${(g.w / g.h).toFixed(6)} vs box ${g.box.toFixed(6)} — the map is letterboxed`, + ); + }); + + // 10. A pin is a target; hitting it twice must not move the map out from + // under the panel that just opened, and one gesture must not fire the + // 24-hour fetch twice (`event.detail > 1`). + await check('double-clicking a pin opens it once and does not zoom', async () => { + await page.click('#reset-view'); + const before = await geometry(page); + const target = await pickMarker(page, { order: 'largest', exclude: await selectedCode(page) }); + assert(target, 'no marker to double-click'); + const fetchesBefore = valuesFetches; + await page.mouse.dblclick(target.x, target.y); + await page.waitForFunction((code) => new URLSearchParams(location.search).get('station') === code, target.code); + const after = await geometry(page); + assert(after.w === before.w, `double-click on a pin zoomed the map (${before.w} -> ${after.w})`); + assert( + valuesFetches - fetchesBefore === 1, + `one gesture fired ${valuesFetches - fetchesBefore} drill-down fetches`, + ); + }); + + // 11. reframe(): a resize must keep the visitor's centre and zoom, and the + // aspect must still match at a short viewport — the boxAspect() floor + // that reintroduced letterboxing on a landscape phone. + await check('a resize to a short viewport keeps the view and the aspect', async () => { + // Zoomed in before panning, so the view sits well inside clampView's + // bounds and "the centre survived" is testing reframe(), not the clamp. + await page.click('#zoom-in'); + await page.click('#zoom-in'); + const empty = await pickEmptyPoint(page); + await drag(page, empty, { x: empty.x - 100, y: empty.y - 60 }); + const before = await geometry(page); + await page.setViewportSize(SHORT_VIEWPORT); + // Wait on reframe having RUN (it rewrites the viewBox height), not on the + // element having been laid out — layout lands before the resize handler. + await page.waitForFunction( + (h) => Number(document.getElementById('map').getAttribute('viewBox').split(' ')[3]) !== h, + before.h, + ); + const after = await geometry(page); + assert( + after.rect.height < 240, + `short viewport gave a ${after.rect.height.toFixed(0)} px map — it no longer tests the floor boundary`, + ); + assert(Math.abs(after.cx - before.cx) < 1e-6 && Math.abs(after.cy - before.cy) < 1e-6, + `resize moved the centre (${before.cx.toFixed(3)}, ${before.cy.toFixed(3)}) -> (${after.cx.toFixed(3)}, ${after.cy.toFixed(3)})`); + assert(Math.abs(after.w - before.w) < 1e-6, `resize changed the zoom (${before.w} -> ${after.w})`); + assert( + Math.abs(after.w / after.h - after.box) < 1e-4, + `short viewport letterboxes: viewBox aspect ${(after.w / after.h).toFixed(6)} vs box ${after.box.toFixed(6)}`, + ); + // Back to the whole-NEM view so there are pins on screen at all, then + // prove a click still lands on the one under the cursor at this height — + // the letterbox offset would put it on a neighbour, or on nothing. + await page.click('#reset-view'); + const marker = await pickMarker(page, { order: 'largest', exclude: await selectedCode(page) }); + assert(marker, 'no hittable marker after the resize'); + await page.mouse.click(marker.x, marker.y); + await page.waitForFunction((code) => new URLSearchParams(location.search).get('station') === code, marker.code); + }); + + if (crashes.length) { + failed += 1; + results.push(` FAIL no uncaught page errors\n ${crashes.join('\n ')}`); + } else { + results.push(' ok no uncaught page errors'); + } + } finally { + await browser.close(); + } + + console.log(`pointer probe — ${BASE}/map\n${results.join('\n')}`); + const total = results.length; + console.log(failed ? `\n${failed} of ${total} FAILED` : `\n${total} assertions passed`); + process.exitCode = failed ? 1 : 0; +} + +main().catch((err) => { + console.error('pointer probe could not run:', err); + process.exitCode = 1; +}); From 32fea8b434f05329a36826676fafbf6e9a86229a Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Mon, 10 Aug 2026 12:50:04 +1000 Subject: [PATCH 2/4] test(worker): make three probe assertions actually able to fail (LAB-1812) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Expert panel, mutation-tested against a deliberately broken map: three of the assertions were green on code with the bug still in it. - "a drag ending on a marker does not select it" passed with suppressClick deleted outright. The drag panned the map, so the marker was no longer under the pointer at release and no marker click was ever in flight — it released on bare . It now drags out and back to the same pin, which is the only gesture that puts a real marker click in front of the suppression. - the drag-threshold assertion passed with DRAG_SLOP_PX = -1, because mouse.click() emits no pointermove at all, so dragging.moved is false at any threshold. It now moves the mouse 2 px between down and up, the way a hand does, and the out-and-back drag above brackets the threshold from the other side. - nothing covered the label-scaling half of rescaleToScreenPixels: a CSS pixel is a USER unit at a 45-unit viewBox, so an unconverted 13 px label renders about 150 px tall, and .region-label is pointer-events:none so no hit test can see it. One assertion on rendered height now does. Seven mutants, seven kills, each by the assertion that should catch it. Also from the panel: failures now say what they saw rather than "Timeout 10000ms exceeded", which could not distinguish "the click did nothing" from "the click opened the wrong station" — the two regressions this gate exists for; results print as they happen so a stall names the gesture it stalled on; assertion 8 no longer leaves the page scrolled under the ones after it; a probe failure dumps the wrangler log the cleanup trap was deleting unread; and the browser cache drops restore-keys, which could only ever restore the wrong revision and then save both. --- .github/workflows/ci.yml | 9 +- worker/README.md | 42 +++--- worker/test/pointer-probe.mjs | 255 +++++++++++++++++++++++----------- 3 files changed, 203 insertions(+), 103 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7966c1a..089fd29 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,8 +60,10 @@ jobs: uses: actions/cache@caa296126883cff596d87d8935842f9db880ef25 # v5.1.0 with: path: ~/.cache/ms-playwright + # Exact key only, no restore-keys: a partial hit would restore ~150 MB + # of the WRONG browser revision, download the right one anyway, then + # save a cache holding both. All cost, no benefit. key: playwright-${{ runner.os }}-${{ hashFiles('worker/package-lock.json') }} - restore-keys: playwright-${{ runner.os }}- - name: Install the probe's browser # Pinned by playwright-core's version in package-lock.json — it downloads # the one browser revision it was built against, never "latest Chrome". @@ -108,4 +110,7 @@ jobs: exit 1 fi - npm run probe:map + # The trap deletes the log on the way out, so a probe failure caused by + # the SERVER (a 500 from /api/v2/generators, migrations not applied) + # would otherwise present as a bare browser timeout with no evidence. + npm run probe:map || { cat "$log_file" >&2; exit 1; } diff --git a/worker/README.md b/worker/README.md index 3eea4d6..2e268f7 100644 --- a/worker/README.md +++ b/worker/README.md @@ -149,28 +149,34 @@ no console errors. **So a real browser moving a real mouse is a CI gate here**, not an optional local ritual. `test/pointer-probe.mjs` drives `/map` with `page.mouse.*` through -playwright-core and asserts 12 things about the result: a click opens the station -under the cursor (including the smallest pin on the map), `+`/`−`/wheel zoom, -drag pans without stealing the click or changing the selection, a drag ending on -a pin does not open it, the page still scrolls beside the map, double-clicking a -pin opens it exactly once without zooming, the viewBox aspect matches the box, -and a resize to a landscape-phone height keeps the centre, the zoom and the -aspect. Every wait is on an observable condition — no sleeps, and **no retries**: -a probe allowed to pass on the second attempt reports "flaky" as "green". +playwright-core and fails the build on a pointer regression; each assertion +carries the bug it exists for, so read the file rather than a list here that +would go stale the first time one moves. ```sh -npx wrangler dev --local # in one shell; the probe needs a server -npm run probe:map # in another — PROBE_URL to point elsewhere -PROBE_HEADED=1 npm run probe:map # watch it drive +npm run migrate:local # seed D1 — no data, no markers, no probe +npx playwright-core install --with-deps chromium +npx wrangler dev --local # in one shell; the probe needs a server +npm run probe:map # in another — PROBE_URL to point elsewhere +PROBE_HEADED=1 npm run probe:map # watch it drive ``` -The browser is `playwright-core`'s own pinned chromium (`npx playwright-core -install chromium`), not a system Chrome, so CI and your laptop run the same -revision. In CI it is cached and the probe shares the `wrangler dev` the -`/health` smoke check already boots; the whole gate costs well under a minute. -Screenshots stay a manual diagnostic — an SVG geometry bug paints a blank or -distorted map while every DOM assertion passes, and eyes are still the cheapest -way to see that. +The browser is `playwright-core`'s own pinned chromium, not a system Chrome, so +CI and your laptop run the same revision. In CI it is cached and the probe shares +the `wrangler dev` the `/health` smoke check already boots, which is why the gate +adds about half a minute rather than two. + +Two rules for anything added to it. **No sleeps and no retries** — wait on the +thing the gesture causes, and run the assertions exactly once, because a probe +allowed to pass on its second attempt reports "flaky" as "green". And **make the +gesture the code actually reacts to**: `mouse.click()` emits no `pointermove`, so +it passes at any drag threshold including a negative one, and a drag released +where the marker no longer is proves nothing about click suppression. Both of +those assertions were green against a deliberately broken map before they were +rewritten to move the mouse the way a hand does; `worker/README.md` is not the +place that will remind you, so the probe says it in its own header. Screenshots +stay a manual diagnostic — an SVG geometry bug can paint a distorted map while +every assertion passes, and eyes are still the cheapest way to see that. Shared page chrome (`$`, `fetchJson`, `showError`, `REGIONS`, `TZ`, the theme toggle) lives in `public/chrome.js` and is imported by both pages; the theme diff --git a/worker/test/pointer-probe.mjs b/worker/test/pointer-probe.mjs index 299315f..8a04da2 100644 --- a/worker/test/pointer-probe.mjs +++ b/worker/test/pointer-probe.mjs @@ -3,22 +3,29 @@ * Real-pointer probe for the station map (`/map`). * * WHY THIS EXISTS, and why the 217 vitest specs do not cover it: every bug this - * catches is a pointer-plumbing bug, and a synthetic `element.click()` does not - * have pointer plumbing. It dispatches one `click` straight at a node — it never - * goes through `pointerdown` -> pointer capture -> `pointerup` -> `click` - * retargeting, never hit-tests through `preserveAspectRatio` letterboxing, and - * never produces a `detail > 1`. Four shipped `/map` regressions (LAB-1702) were - * invisible to a DOM-assertion check that happily reported 213 markers with the - * right classes and no console errors on a page where clicking a station did - * nothing at all. Only a real browser moving a real mouse sees them. + * catches is a pointer-plumbing or screen-geometry bug, and a synthetic + * `element.click()` has neither. It dispatches one `click` straight at a node — + * it never goes through `pointerdown` -> pointer capture -> `pointerup` -> + * `click` retargeting, never crosses the drag threshold, never hit-tests through + * `preserveAspectRatio` letterboxing, and never produces a `detail > 1`. Four + * shipped `/map` regressions (LAB-1702) were invisible to a DOM-assertion check + * that happily reported 213 markers with the right classes and no console errors + * on a page where clicking a station did nothing at all. * * Runs headlessly against a live Worker (`wrangler dev --local` in CI). Set * PROBE_URL to point it elsewhere; PROBE_HEADED=1 to watch it drive. * - * Determinism rules for anything added here: never sleep, always wait on an - * observable condition, and never assume which marker is where — pick targets by - * hit-testing with `elementFromPoint` so a facilities-snapshot refresh cannot - * silently turn an assertion into a no-op. + * RULES for anything added here, learned by having each of them violated: + * - Never sleep. Wait on the observable thing the gesture causes. + * - Never assume which marker is where. Hit-test with `elementFromPoint`, so a + * facilities-snapshot refresh cannot silently turn an assertion into a no-op. + * - Make the gesture the code actually reacts to. `mouse.click()` emits no + * `pointermove`, so it cannot test a drag threshold from either side; a drag + * that ends somewhere the marker no longer is cannot test click suppression. + * Both of those passed against a deliberately broken map before they were + * rewritten to move the mouse the way a hand does. + * - Every failure must say what it saw, not just that it timed out. When this + * goes red it is 2am and the reader is not you. */ import { chromium } from 'playwright-core'; @@ -27,19 +34,30 @@ const HEADED = process.env.PROBE_HEADED === '1'; /** Wide enough for the two-column layout, short enough that the page scrolls. */ const VIEWPORT = { width: 1280, height: 800 }; /** Landscape-phone height: the map box renders under the 240 px that boxAspect() - * used to floor at, which is the `boxAspect` boundary CodeRabbit found. */ + * used to floor at, and its height stops being a whole number — which is the + * only viewport where the clientHeight-rounding letterbox shows up. */ const SHORT_VIEWPORT = { width: 900, height: 380 }; +/** Mirrors DRAG_SLOP_PX in public/map.js. The probe brackets it from both sides: + * a smaller movement must still count as a click, a larger one must not. */ +const DRAG_SLOP_PX = 4; +/** Mirrors LABEL_PX in public/map.js — jurisdiction labels are specified in + * SCREEN pixels and converted to user units on every view change. */ +const LABEL_PX = 13; -const results = []; let failed = 0; +let count = 0; async function check(name, fn) { + count += 1; try { await fn(); - results.push(` ok ${name}`); + // Printed as it happens, not buffered: a hang or a throw outside a check + // would otherwise discard every line collected so far, and the one thing you + // need from a stalled gate is which gesture it stalled on. + console.log(` ok ${name}`); } catch (err) { failed += 1; - results.push(` FAIL ${name}\n ${err instanceof Error ? err.message : String(err)}`); + console.log(` FAIL ${name}\n ${err instanceof Error ? err.message : String(err)}`); } } @@ -55,7 +73,7 @@ const geometry = (page) => const svg = document.getElementById('map'); const [x, y, w, h] = svg.getAttribute('viewBox').split(' ').map(Number); const rect = svg.getBoundingClientRect(); - return { x, y, w, h, cx: x + w / 2, cy: y + h / 2, rect, box: rect.width / rect.height }; + return { w, h, cx: x + w / 2, cy: y + h / 2, rect, box: rect.width / rect.height }; }); /** @@ -64,25 +82,24 @@ const geometry = (page) => * clickable, but a small station can still sit under a large one — asserting on a * covered marker would test the wrong element and pass for the wrong reason. * `order: 'smallest'` picks the tiniest hittable pin, which is the case that - * actually broke (a 6 px dot is not a pointer target). + * actually broke: a 6 px dot is not a pointer target. */ function pickMarker(page, { order = 'largest', exclude = null } = {}) { return page.evaluate( ({ order, exclude }) => { const map = document.getElementById('map').getBoundingClientRect(); - const markers = [...document.querySelectorAll('circle.marker')] + const candidates = [...document.querySelectorAll('circle.marker')] .filter((m) => m.dataset.code !== exclude) - .map((m) => ({ node: m, r: m.getBoundingClientRect().width / 2 })) - .sort((a, b) => (order === 'smallest' ? a.r - b.r : b.r - a.r)); - for (const { node, r } of markers) { - const rect = node.getBoundingClientRect(); + .map((m) => ({ node: m, rect: m.getBoundingClientRect() })) + .sort((a, b) => (order === 'smallest' ? a.rect.width - b.rect.width : b.rect.width - a.rect.width)); + for (const { node, rect } of candidates) { const x = rect.x + rect.width / 2; const y = rect.y + rect.height / 2; // Inside the map box, or elementFromPoint answers about the page chrome // (or nothing at all, for a marker scrolled out of the viewport). if (x < map.x || x > map.right || y < map.y || y > map.bottom) continue; if (document.elementFromPoint(x, y) !== node) continue; - return { code: node.dataset.code, name: node.querySelector('title').textContent, x, y, r }; + return { code: node.dataset.code, name: node.querySelector('title').textContent, x, y, r: rect.width / 2 }; } return null; }, @@ -92,8 +109,8 @@ function pickMarker(page, { order = 'largest', exclude = null } = {}) { /** A point over bare basemap — no marker under it, so a drag or double-click * there exercises the pan/zoom path and not the drill-down. */ -function pickEmptyPoint(page) { - return page.evaluate(() => { +async function pickEmptyPoint(page) { + const point = await page.evaluate(() => { const rect = document.getElementById('map').getBoundingClientRect(); for (let fx = 0.2; fx <= 0.8; fx += 0.05) { for (let fy = 0.2; fy <= 0.8; fy += 0.05) { @@ -105,34 +122,68 @@ function pickEmptyPoint(page) { } return null; }); + assert(point, 'no bare-basemap point found — is the map covered by pins, or off screen?'); + return point; } const selectedCode = (page) => page.evaluate(() => document.querySelector('circle.marker.selected')?.dataset.code ?? null); -const panelHeading = (page) => - page.evaluate(() => document.querySelector('#panel-body h2')?.textContent ?? null); +/** The drill-down opening is the observable, so this is the assertion for most + * of the click checks — and a bare "Timeout 10000ms exceeded" cannot tell + * "the click did nothing" (capture retargeting) from "the click opened the + * wrong station" (a coordinate offset). Those are two different bugs and the + * message has to name which one happened. */ +async function waitForStation(page, marker, what) { + try { + await page.waitForFunction((code) => new URLSearchParams(location.search).get('station') === code, marker.code); + } catch { + const [selected, station] = await Promise.all([ + selectedCode(page), + page.evaluate(() => new URLSearchParams(location.search).get('station')), + ]); + throw new Error( + selected == null + ? `${what} on ${marker.name} did nothing — no station selected` + : `${what} on ${marker.name} (${marker.code}) selected ${selected} instead (?station=${station})`, + ); + } +} -/** Ready = the join ran, markers are in the DOM, and the view has been set. - * This is the only "wait" in the probe; everything after it waits on the - * specific thing it is about to assert on. */ -async function openMap(page) { - await page.goto(`${BASE}/map`, { waitUntil: 'domcontentloaded' }); - await page.waitForFunction( - () => - document.querySelectorAll('circle.marker').length > 0 && - document.getElementById('map').hasAttribute('viewBox'), - null, - { timeout: 30_000 }, - ); +/** Ready = the join ran, markers are in the DOM, and the view has been set. */ +async function openMap(page, crashes) { + const response = await page.goto(`${BASE}/map`, { waitUntil: 'domcontentloaded' }); + try { + await page.waitForFunction( + () => + document.querySelectorAll('circle.marker').length > 0 && + document.getElementById('map').hasAttribute('viewBox'), + null, + { timeout: 30_000 }, + ); + } catch { + // The usual cause is a server with no seeded D1 (`npm run migrate:local`), + // which otherwise presents as an unexplained 30 s timeout. + const state = await page.evaluate(() => ({ + markers: document.querySelectorAll('circle.marker').length, + viewBox: document.getElementById('map')?.getAttribute('viewBox') ?? null, + error: document.getElementById('error-text')?.textContent || null, + })); + throw new Error( + `/map never became ready: HTTP ${response?.status()}, ${state.markers} markers, ` + + `viewBox=${state.viewBox}, page error=${state.error ?? 'none'}` + + (crashes.length ? `, uncaught: ${crashes.join('; ')}` : '') + + ' — is the Worker seeded (npm run migrate:local)?', + ); + } } -async function drag(page, from, to) { +/** Stepped, so `pointermove` fires repeatedly and the drag threshold is really + * crossed. A single jump can be delivered as one move and is not a drag. */ +async function drag(page, from, ...waypoints) { await page.mouse.move(from.x, from.y); await page.mouse.down(); - // Stepped so `pointermove` fires repeatedly and the 4 px drag slop is really - // crossed — a single jump can be delivered as one move and is not a drag. - await page.mouse.move(to.x, to.y, { steps: 12 }); + for (const point of waypoints) await page.mouse.move(point.x, point.y, { steps: 12 }); await page.mouse.up(); } @@ -157,32 +208,36 @@ async function main() { const crashes = []; page.on('pageerror', (err) => crashes.push(err.message)); + console.log(`pointer probe — ${BASE}/map`); try { - await openMap(page); + await openMap(page, crashes); // 1. The headline regression: setPointerCapture() retargeted `pointerup` AND // `click` to the , so the marker's own listener never ran and // clicking a station did nothing. - let clicked; + let clicked = null; await check('click on a marker opens that station', async () => { clicked = await pickMarker(page, { order: 'largest' }); assert(clicked, 'no hittable marker found'); await page.mouse.click(clicked.x, clicked.y); - await page.waitForFunction((code) => new URLSearchParams(location.search).get('station') === code, clicked.code); - assert((await selectedCode(page)) === clicked.code, `selected ${await selectedCode(page)}, expected ${clicked.code}`); - assert(await panelHeading(page), 'drill-down panel stayed empty'); + await waitForStation(page, clicked, 'a click'); + assert((await selectedCode(page)) === clicked.code, `marker class did not follow the selection`); }); - // 2. Same path, smallest pin on the map: the drag threshold was ~1.6 px, so - // the hand tremor in a click on a small target read as a drag and was - // suppressed. Small stations are exactly the ones you click to identify. - await check('click on the smallest marker opens that station, not a neighbour', async () => { - const small = await pickMarker(page, { order: 'smallest', exclude: clicked.code }); + // 2. Same path on the smallest pin on the map, with a hand tremor in it. The + // drag threshold was 0.2% of the viewBox width — under two screen pixels + // at the whole-NEM view — so the jitter in an ordinary click registered as + // a drag and the click was suppressed. `mouse.click()` cannot catch that: + // it emits no `pointermove` at all, so it passes at any threshold, + // including a negative one. The movement has to be real and sub-slop. + let small = null; + await check('a click with a hand tremor still opens the smallest pin', async () => { + assert(clicked, 'assertion 1 did not run, so there is no station to exclude'); + small = await pickMarker(page, { order: 'smallest', exclude: clicked.code }); assert(small, 'no second hittable marker found'); - assert(small.r * 2 <= 24, `"smallest" marker is ${(small.r * 2).toFixed(1)} px wide — pick logic is wrong`); - await page.mouse.click(small.x, small.y); - await page.waitForFunction((code) => new URLSearchParams(location.search).get('station') === code, small.code); - assert((await selectedCode(page)) === small.code, 'a different station was selected'); + assert(small.r * 2 <= 24, `"smallest" marker is ${(small.r * 2).toFixed(1)} px wide — the pick logic is wrong`); + await drag(page, small, { x: small.x + DRAG_SLOP_PX - 2, y: small.y + 1 }); + await waitForStation(page, small, 'a click with 2 px of tremor'); }); // 3-4. The visible, keyboard-reachable controls. They are the primary way in; @@ -203,7 +258,6 @@ async function main() { // in the wrong place; no map on the web works that way. await check('an unmodified wheel zooms the map', async () => { const empty = await pickEmptyPoint(page); - assert(empty, 'no bare-basemap point found'); const before = await geometry(page); await page.mouse.move(empty.x, empty.y); await page.mouse.wheel(0, -240); @@ -211,7 +265,8 @@ async function main() { }); // 6. Pan. Window-level pointermove/up replaced pointer capture, so this is - // the assertion that the replacement actually works. + // the assertion that the replacement actually works — and the distance + // check is what would catch a pointer coordinate being mistranslated. await check('dragging pans the view', async () => { await page.click('#reset-view'); const empty = await pickEmptyPoint(page); @@ -227,14 +282,23 @@ async function main() { ); }); - // 7. The other half of the drag contract: a drag that happens to finish over - // a pin must not open it. (`suppressClick`.) - await check('a drag ending on a marker does not select it', async () => { + // 7. The other half of the drag contract: a drag that finishes over a pin + // must not open it (`suppressClick`). It has to end where it STARTED — + // dragging away and releasing on empty space proves nothing, because the + // map pans with the pointer and the marker is no longer under it, so no + // click on a marker was ever in flight. Out and back also means the + // browser really does fire a click on the marker, which is the event that + // must be swallowed. + await check('a drag that returns to its marker does not select it', async () => { const before = await selectedCode(page); const target = await pickMarker(page, { order: 'largest', exclude: before }); assert(target, 'no marker to drag onto'); - await drag(page, { x: target.x + 120, y: target.y + 60 }, { x: target.x, y: target.y }); - assert((await selectedCode(page)) === before, `a pan opened ${await selectedCode(page)}`); + assert(DRAG_SLOP_PX < 40, 'the drag below must exceed the slop to be a drag at all'); + await drag(page, target, { x: target.x + 40, y: target.y + 24 }, target); + assert( + (await selectedCode(page)) === before, + `a pan opened ${await selectedCode(page)} — the drag was not distinguished from a click`, + ); }); // 8. The scroll-trap guard is CSS (the map is capped at min(62vh, 34rem)), so @@ -249,6 +313,10 @@ async function main() { await page.mouse.move(outside.x, outside.y); await page.mouse.wheel(0, 300); await page.waitForFunction(() => scrollY > 0); + // Left scrolled, the map sits partly above the viewport and every later + // hit-test would be measuring a different page than this one did. + await page.evaluate(() => scrollTo(0, 0)); + await page.waitForFunction(() => scrollY === 0); }); // 9. A viewBox whose aspect differs from the element's letterboxes under @@ -262,7 +330,27 @@ async function main() { ); }); - // 10. A pin is a target; hitting it twice must not move the map out from + // 10. Marker radii and jurisdiction label type are specified in SCREEN pixels + // but SVG attributes are in user units, so both are converted on every + // view change. Missing that renders a 13 px label about 150 px tall and + // swallows the continent — and it is invisible to hit-testing, because + // `.region-label` is `pointer-events: none`. Marker radii are covered by + // the clicks above (an unscaled pin is unhittable); labels need their own + // look, and this is the one assertion here that needs no pointer at all. + await check('jurisdiction labels stay screen-sized through a zoom', async () => { + await page.click('#zoom-in'); + const heights = await page.evaluate(() => + [...document.querySelectorAll('text.region-label')].map((t) => t.getBoundingClientRect().height), + ); + assert(heights.length > 0, 'no jurisdiction labels rendered'); + const worst = Math.max(...heights); + assert( + worst < LABEL_PX * 2, + `a jurisdiction label renders ${worst.toFixed(1)} px tall for a ${LABEL_PX} px spec — user units are being treated as screen pixels`, + ); + }); + + // 11. A pin is a target; hitting it twice must not move the map out from // under the panel that just opened, and one gesture must not fire the // 24-hour fetch twice (`event.detail > 1`). await check('double-clicking a pin opens it once and does not zoom', async () => { @@ -272,7 +360,12 @@ async function main() { assert(target, 'no marker to double-click'); const fetchesBefore = valuesFetches; await page.mouse.dblclick(target.x, target.y); - await page.waitForFunction((code) => new URLSearchParams(location.search).get('station') === code, target.code); + await waitForStation(page, target, 'a double-click'); + // Both clicks are delivered before `dblclick`, so a second drill-down fetch + // is issued during the same task as the first; this round trip yields to + // the page's task queue so that request is on the wire before we count. + // (A task boundary, not a sleep — nothing here waits on elapsed time.) + await page.evaluate(() => new Promise((resolve) => setTimeout(resolve, 0))); const after = await geometry(page); assert(after.w === before.w, `double-click on a pin zoomed the map (${before.w} -> ${after.w})`); assert( @@ -281,9 +374,10 @@ async function main() { ); }); - // 11. reframe(): a resize must keep the visitor's centre and zoom, and the + // 12. reframe(): a resize must keep the visitor's centre and zoom, and the // aspect must still match at a short viewport — the boxAspect() floor - // that reintroduced letterboxing on a landscape phone. + // that reintroduced letterboxing on a landscape phone, and the + // clientHeight rounding that survived it. await check('a resize to a short viewport keeps the view and the aspect', async () => { // Zoomed in before panning, so the view sits well inside clampView's // bounds and "the centre survived" is testing reframe(), not the clamp. @@ -304,8 +398,10 @@ async function main() { after.rect.height < 240, `short viewport gave a ${after.rect.height.toFixed(0)} px map — it no longer tests the floor boundary`, ); - assert(Math.abs(after.cx - before.cx) < 1e-6 && Math.abs(after.cy - before.cy) < 1e-6, - `resize moved the centre (${before.cx.toFixed(3)}, ${before.cy.toFixed(3)}) -> (${after.cx.toFixed(3)}, ${after.cy.toFixed(3)})`); + assert( + Math.abs(after.cx - before.cx) < 1e-6 && Math.abs(after.cy - before.cy) < 1e-6, + `resize moved the centre (${before.cx.toFixed(3)}, ${before.cy.toFixed(3)}) -> (${after.cx.toFixed(3)}, ${after.cy.toFixed(3)})`, + ); assert(Math.abs(after.w - before.w) < 1e-6, `resize changed the zoom (${before.w} -> ${after.w})`); assert( Math.abs(after.w / after.h - after.box) < 1e-4, @@ -313,31 +409,24 @@ async function main() { ); // Back to the whole-NEM view so there are pins on screen at all, then // prove a click still lands on the one under the cursor at this height — - // the letterbox offset would put it on a neighbour, or on nothing. + // a letterbox offset would put it on a neighbour, or on nothing. await page.click('#reset-view'); const marker = await pickMarker(page, { order: 'largest', exclude: await selectedCode(page) }); assert(marker, 'no hittable marker after the resize'); await page.mouse.click(marker.x, marker.y); - await page.waitForFunction((code) => new URLSearchParams(location.search).get('station') === code, marker.code); + await waitForStation(page, marker, 'a click at a short viewport'); }); - if (crashes.length) { - failed += 1; - results.push(` FAIL no uncaught page errors\n ${crashes.join('\n ')}`); - } else { - results.push(' ok no uncaught page errors'); - } + await check('no uncaught page errors', () => assert(!crashes.length, crashes.join('\n '))); } finally { await browser.close(); } - console.log(`pointer probe — ${BASE}/map\n${results.join('\n')}`); - const total = results.length; - console.log(failed ? `\n${failed} of ${total} FAILED` : `\n${total} assertions passed`); + console.log(failed ? `\n${failed} of ${count} FAILED` : `\n${count} assertions passed`); process.exitCode = failed ? 1 : 0; } main().catch((err) => { - console.error('pointer probe could not run:', err); + console.error(`\npointer probe could not run: ${err instanceof Error ? err.message : String(err)}`); process.exitCode = 1; }); From d879bee43f9f86683df4194ab4c4dcf421946aab Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Mon, 10 Aug 2026 14:09:30 +1000 Subject: [PATCH 3/4] test(worker): address CodeRabbit round 2 on the pointer probe (LAB-1812) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings, both applied: - geometry() returned the raw DOMRect across the evaluate() boundary. The pinned playwright-core 1.62.1 does carry its prototype-getter properties across (verified empirically: all rect fields arrive, and CI's 13 green assertions consume rect.width/rect.height), but Playwright only documents plain-serializable returns, so the probe now sends { width, height } as a plain object rather than leaning on an undocumented serializer behaviour. - the README said "Two bugs" over a list that had grown to three bullets, while the probe header counts four shipped regressions. The fourth — the double-click that zoomed the map out from under its own panel and fired the drill-down fetch twice — was already referenced in prose (detail > 1) but missing from the list. Added it; both counts now agree at four. Probe re-run against a seeded local wrangler dev: 13 assertions passed. tsc --noEmit clean on both configs. --- worker/README.md | 7 +++++-- worker/test/pointer-probe.mjs | 12 +++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/worker/README.md b/worker/README.md index 2e268f7..d464e50 100644 --- a/worker/README.md +++ b/worker/README.md @@ -120,7 +120,7 @@ aspect differs from the box letterboxes under `preserveAspectRatio` and silently offsets every pointer coordinate. The cap is also what makes an unmodified wheel zoom acceptable: there is always page above and below the map to scroll past it. -**Interaction here needs a real browser with real pointer events.** Two bugs +**Interaction here needs a real browser with real pointer events.** Four bugs shipped past a DOM-assertion check that reported 213 markers with correct classes and no console errors: @@ -133,7 +133,10 @@ and no console errors: - the drag threshold was 0.2% of the viewBox width — under two screen pixels at the whole-NEM view — so the hand tremor in an ordinary click registered as a drag and the click was suppressed. It is now 4 CSS pixels. - +- double-clicking a *pin* zoomed the map out from under the panel it had just + opened, and the two clicks in the gesture each fired the 24-hour drill-down + fetch. Double-click now zooms the basemap only, and the capture-phase click + handler drops any click with `detail > 1`. - `boxAspect()` measured the box with `clientWidth`/`clientHeight`, which **round to whole pixels**, while `clientToUser()` converts with the fractional `getBoundingClientRect()`. A 236.4 px box read as 236 fitted the viewBox to an diff --git a/worker/test/pointer-probe.mjs b/worker/test/pointer-probe.mjs index 8a04da2..88c122a 100644 --- a/worker/test/pointer-probe.mjs +++ b/worker/test/pointer-probe.mjs @@ -73,7 +73,17 @@ const geometry = (page) => const svg = document.getElementById('map'); const [x, y, w, h] = svg.getAttribute('viewBox').split(' ').map(Number); const rect = svg.getBoundingClientRect(); - return { w, h, cx: x + w / 2, cy: y + h / 2, rect, box: rect.width / rect.height }; + // Plain values only: a DOMRect's properties are prototype getters, and + // Playwright documents evaluate() returns as plain-serializable — 1.62.1 + // happens to carry them across, but that is not a contract to lean on. + return { + w, + h, + cx: x + w / 2, + cy: y + h / 2, + rect: { width: rect.width, height: rect.height }, + box: rect.width / rect.height, + }; }); /** From 8d010fdf9826157c303d3fbc5f0af736d4d125fb Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Mon, 10 Aug 2026 15:04:12 +1000 Subject: [PATCH 4/4] test(worker): address CodeRabbit round 3 on the pointer probe (LAB-1812) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - openMap: drop the 30 s waitForFunction override so the context's configured 10 s default bounds map readiness like every other wait. - Correct the mouse.click() guidance in the README and probe comments: click does emit a pointermove (before pointerdown) — what it cannot do is move while the button is down, which is why it cannot test a drag threshold. --- worker/README.md | 5 +++-- worker/test/pointer-probe.mjs | 14 +++++++------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/worker/README.md b/worker/README.md index d464e50..f242ce4 100644 --- a/worker/README.md +++ b/worker/README.md @@ -172,8 +172,9 @@ adds about half a minute rather than two. Two rules for anything added to it. **No sleeps and no retries** — wait on the thing the gesture causes, and run the assertions exactly once, because a probe allowed to pass on its second attempt reports "flaky" as "green". And **make the -gesture the code actually reacts to**: `mouse.click()` emits no `pointermove`, so -it passes at any drag threshold including a negative one, and a drag released +gesture the code actually reacts to**: `mouse.click()` never moves while the +button is down (its only `pointermove` lands before `pointerdown`), so it passes +at any drag threshold including a negative one, and a drag released where the marker no longer is proves nothing about click suppression. Both of those assertions were green against a deliberately broken map before they were rewritten to move the mouse the way a hand does; `worker/README.md` is not the diff --git a/worker/test/pointer-probe.mjs b/worker/test/pointer-probe.mjs index 88c122a..ffc0be8 100644 --- a/worker/test/pointer-probe.mjs +++ b/worker/test/pointer-probe.mjs @@ -19,8 +19,9 @@ * - Never sleep. Wait on the observable thing the gesture causes. * - Never assume which marker is where. Hit-test with `elementFromPoint`, so a * facilities-snapshot refresh cannot silently turn an assertion into a no-op. - * - Make the gesture the code actually reacts to. `mouse.click()` emits no - * `pointermove`, so it cannot test a drag threshold from either side; a drag + * - Make the gesture the code actually reacts to. `mouse.click()` never moves + * while the button is down (its only `pointermove` lands before + * `pointerdown`), so it cannot test a drag threshold from either side; a drag * that ends somewhere the marker no longer is cannot test click suppression. * Both of those passed against a deliberately broken map before they were * rewritten to move the mouse the way a hand does. @@ -168,12 +169,10 @@ async function openMap(page, crashes) { () => document.querySelectorAll('circle.marker').length > 0 && document.getElementById('map').hasAttribute('viewBox'), - null, - { timeout: 30_000 }, ); } catch { // The usual cause is a server with no seeded D1 (`npm run migrate:local`), - // which otherwise presents as an unexplained 30 s timeout. + // which otherwise presents as an unexplained timeout. const state = await page.evaluate(() => ({ markers: document.querySelectorAll('circle.marker').length, viewBox: document.getElementById('map')?.getAttribute('viewBox') ?? null, @@ -238,8 +237,9 @@ async function main() { // drag threshold was 0.2% of the viewBox width — under two screen pixels // at the whole-NEM view — so the jitter in an ordinary click registered as // a drag and the click was suppressed. `mouse.click()` cannot catch that: - // it emits no `pointermove` at all, so it passes at any threshold, - // including a negative one. The movement has to be real and sub-slop. + // it never moves between `pointerdown` and `pointerup`, so it passes at + // any threshold, including a negative one. The movement has to be real, + // button-down, and sub-slop. let small = null; await check('a click with a hand tremor still opens the smallest pin', async () => { assert(clicked, 'assertion 1 did not run, so there is no station to exclude');