LAB-1812: commit the pointer probe and make it a CI gate - #29
Conversation
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.
This comment has been minimized.
This comment has been minimized.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds a real-pointer Playwright probe for ChangesMap probe and CI integration
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant WranglerWorker
participant PlaywrightChromium
CI->>WranglerWorker: Start local Worker
CI->>WranglerWorker: Poll health endpoint
WranglerWorker-->>CI: Return health response
CI->>PlaywrightChromium: Run probe:map
PlaywrightChromium->>WranglerWorker: Open /map and perform pointer actions
WranglerWorker-->>PlaywrightChromium: Return map state and navigation responses
PlaywrightChromium-->>CI: Report assertion and runtime results
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
worker/test/pointer-probe.mjs (1)
182-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the "small marker" bound from the picked markers, not a literal.
small.r * 2 <= 24hard-codes 24 px. The marker radius scale depends on√capacityand on the current zoom. A future scale change makes this guard fail even though the pick logic is correct. Comparesmall.rwithclicked.rinstead, sincepickMarkeralready sorted by radius.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@worker/test/pointer-probe.mjs` at line 182, The assertion in the pointer-probe test should derive the small-marker bound from the picked markers rather than the hard-coded 24 px value. Update the check around small and clicked to compare small.r against clicked.r, preserving the existing pickMarker radius ordering and failure context..github/workflows/ci.yml (1)
100-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDump the Worker log if the probe fails.
Both readiness branches print
$log_filebefore they exit. The probe at Line 111 does not.set -euo pipefailaborts the step on a non-zero exit fromnpm run probe:map, then theEXITtrap deletes the log at Line 84. A probe failure caused by the server, for example a 500 from/api/v2/valuesor an asset-serving error, then leaves no server-side evidence. The probe output alone reports the symptom, not the cause.♻️ Proposed change
- npm run probe:map + if ! npm run probe:map; then + echo "--- wrangler dev log ---" >&2 + cat "$log_file" >&2 + exit 1 + fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 100 - 111, Update the workflow step around npm run probe:map to capture probe failure before the EXIT trap removes log_file: run the probe in an explicit conditional or otherwise handle its non-zero status, print a clear failure message and cat "$log_file" to stderr, then exit with the probe’s failure status. Preserve normal success behavior and the existing readiness checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@worker/README.md`:
- Around line 137-142: Update the bullet-count statement immediately above the
list in worker/README.md to reflect the three bugs described by the list, while
leaving the listed bug descriptions unchanged.
In `@worker/test/pointer-probe.mjs`:
- Around line 217-219: Assert the result of pickEmptyPoint() before using it in
both affected sites: worker/test/pointer-probe.mjs lines 217-219 and 292-293.
Add the existing assert(empty, 'no bare-basemap point found') immediately after
each assignment and before geometry() or drag() runs, so null results report the
correct cause.
- Around line 53-59: Update the geometry function’s page.evaluate return value
to expose rect.width and rect.height as plain object properties, rather than
returning the DOMRect instance in rect. Preserve the existing viewBox-derived
values and aspect-ratio calculation while ensuring callers can access the
serialized rectangle dimensions.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 100-111: Update the workflow step around npm run probe:map to
capture probe failure before the EXIT trap removes log_file: run the probe in an
explicit conditional or otherwise handle its non-zero status, print a clear
failure message and cat "$log_file" to stderr, then exit with the probe’s
failure status. Preserve normal success behavior and the existing readiness
checks.
In `@worker/test/pointer-probe.mjs`:
- Line 182: The assertion in the pointer-probe test should derive the
small-marker bound from the picked markers rather than the hard-coded 24 px
value. Update the check around small and clicked to compare small.r against
clicked.r, preserving the existing pickMarker radius ordering and failure
context.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a389090-84f5-47f6-ada7-d6493d7a7c28
⛔ Files ignored due to path filters (1)
worker/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
.github/workflows/ci.ymlworker/README.mdworker/package.jsonworker/public/map.jsworker/test/pointer-probe.mjs
…1812) 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 <svg>. 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.
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
|
@coderabbitai review |
|
@kody start-review |
|
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
worker/test/pointer-probe.mjs (1)
371-383: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWait for the drill-down request instead of a timer task.
setTimeout(resolve, 0)does not synchronise with the Playwrightrequestevent. The probe can readvaluesFetchesbefore all requests from the double-click are reported. This can produce a false result for the duplicate-fetch regression check.Create
page.waitForRequest()beforedblclick()and await it before the count assertion.Proposed fix
assert(target, 'no marker to double-click'); const fetchesBefore = valuesFetches; + const valuesRequest = page.waitForRequest((req) => req.url().includes('/api/v2/values')); await page.mouse.dblclick(target.x, target.y); await waitForStation(page, target, 'a double-click'); - await page.evaluate(() => new Promise((resolve) => setTimeout(resolve, 0))); + await valuesRequest; const after = await geometry(page);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@worker/test/pointer-probe.mjs` around lines 371 - 383, Replace the setTimeout-based task boundary in the double-click check with a request wait: create and retain a page.waitForRequest() promise before page.mouse.dblclick(), then await that promise after waitForStation and before asserting valuesFetches. Keep the existing fetch-count and map-size assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@worker/README.md`:
- Around line 175-177: Update the mouse.click() guidance in the README and
matching probe comments to clarify that it may emit a pointermove before
pressing but cannot test movement while held or drag thresholds. Replace those
probe examples with an explicit mouse.down() → mouse.move() → mouse.up()
sequence.
In `@worker/test/pointer-probe.mjs`:
- Around line 167-172: Update the map readiness waitForFunction call in the
pointer probe to use the configured 10-second timeout by removing the 30-second
override or setting it to 10_000, while preserving the existing readiness
conditions.
---
Outside diff comments:
In `@worker/test/pointer-probe.mjs`:
- Around line 371-383: Replace the setTimeout-based task boundary in the
double-click check with a request wait: create and retain a
page.waitForRequest() promise before page.mouse.dblclick(), then await that
promise after waitForStation and before asserting valuesFetches. Keep the
existing fetch-count and map-size assertions unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d699f3a-5f93-48a5-b03b-d0a68a10a42e
📒 Files selected for processing (3)
.github/workflows/ci.ymlworker/README.mdworker/test/pointer-probe.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
- 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.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
@coderabbitai review |
|
|
@kody start-review |
1 participant
Closes LAB-1812.
/maphas shipped four pointer-interaction bugs green. A syntheticelement.click()has no pointer plumbing to break — it never goes throughpointerdown→ capture →clickretargeting, never crosses a drag threshold, never hit-tests throughpreserveAspectRatioletterboxing, and never produces adetail > 1. The only thing that ever caught them was a browser moving a real mouse, and that probe lived in one agent's worktree. It was not in the repo (git ls-tree -r origin/masterhad no such file) and is gone from disk, so this is a rewrite from the behaviour LAB-1702 documented, not a copy.What lands
worker/test/pointer-probe.mjs— drives/mapwithpage.mouse.*, 13 assertions, each carrying the bug it exists for.playwright-corepinned to1.62.1as a devDependency. Production bundle and runtime dependencies untouched..github/workflows/ci.yml— cached browser, then the probe against thewrangler devthe/healthsmoke check already boots.worker/README.md— the "This is not wired into CI" paragraph is replaced by how the gate runs and how to run it locally.The probe found a fifth bug on its first headless run
boxAspect()measured the map box withclientWidth/clientHeight, which round to whole pixels, whileclientToUser()converts pointer coordinates with the fractionalgetBoundingClientRect(). At a landscape-phone height the box is 236.4 px andclientHeightcalls it 236, so the viewBox was fitted to an aspect the element does not have andpreserveAspectRatioletterboxed it — every click landing 0.17% off. Same family as the two bugs the README already warns about, at a viewport nobody had opened by hand. Both now read the same measurement, which is the only way they cannot disagree.The gate gates
Reintroducing the exact regression the README forbids —
svg.setPointerCapture(event.pointerId)in thepointerdownhandler — turns 4 of the 13 assertions red. Seeded on a throwaway branch and dispatched at CI, never merged, branch since deleted: run 31350888613 — failure.Clean tip is green: run 31350872098 — success.
That is one mutant. The full matrix, run locally against a map broken one way at a time — seven mutants, seven kills, each by the assertion that should catch it:
map.jssuppressClicknever set (a drag also selects)boxAspect()back on roundedclientWidth/clientHeightdetail > 1guard removed (one gesture selects twice)Three of those assertions could not fail when this PR was first pushed. The expert panel mutation-tested them and found all three green against a map with the bug still in it — a drag released where the marker no longer was, a
mouse.click()that emits nopointermoveand so passes at any drag threshold including a negative one, and no coverage at all of the label half ofrescaleToScreenPixels()(apointer-events: noneelement is invisible to every hit test). Fixed in32fea8b; the table above is the re-verification.Determinism
Every wait is on an observable condition the gesture causes — a URL parameter, a
viewBoxattribute, a scroll position, a rendered height. No sleeps, and no retries: the workflow runsnpm run probe:mapexactly once, because a probe allowed to pass on its second attempt reports "flaky" as "green". The server readiness poll (30 × 1 s against/health) is the one loop and it gates on the endpoint answering, not on elapsed time. Target markers are chosen by hit-testing withelementFromPointrather than by name or index, so afacilities.jsonrefresh cannot silently turn an assertion into a no-op — if nothing is hittable the probe fails rather than passing vacuously. Playwright's default 30 s timeout is lowered to 10 s so a red build says so quickly, and every failure reports what it saw rather than that it timed out.Cost
masterbaseline (3 recent runs)Added: ~13 s warm, ~30 s cold — against an expectation of ≤ 2 min. Browser install is 11 s on a cache hit (the apt deps are not cacheable and run either way), 24 s cold; the probe itself is ~2 s of the 5 s smoke step, and 1.8 s locally.
Expert panel
Run at high stakes. Bug-hunter's three CRITs are the mutation findings above, all applied. Also applied: failures now name what they saw instead of
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; assertion 8 no longer leaves the page scrolled under the ones after it; a probe failure dumps thewranglerlog the cleanup trap was deleting unread;restore-keysdropped from the browser cache. Security returned one LOW, accepted with reasoning:playwright-core installfetches the browser binary with no checksum verification, but the job holds zero secrets, runscontents: readonpull_request(notpull_request_target), andnpm ciin the same job already executes lifecycle scripts — so the realistic blast radius is a CI gate that lies, not credential theft, and the Cloudflare token indeploy.ymlis unreachable from here.Summary by CodeRabbit
Bug Fixes
Tests