rob/PlaceMark
Watch
1
0
Fork
You've already forked PlaceMark
 0
Code Issues Pull requests Projects Releases Packages 3 Wiki Activity Actions

Make the deferred-click window overridable to fix the click-vs-panel race (task 233) #173

Merged
rob merged 6 commits from fix/task-233-distant-clicks-flake into main 2026-08-17 07:27:49 +00:00
Conversation 8 Commits 6 Files changed 4 +296 -37
rob commented 2026-08-14 20:56:53 +00:00
Owner
Copy link

Fixes task 233. Picks the parked branch back up after the coordinate-change attempt below was disproved.

Why a coordinate change cannot work (preserved from this PR's earlier analysis)

All three logged failures (be2a075, fd0bd0e, 00aa81d) showed SecondClickX (1050) landing inside .modal-overlay-panel's own footprint once the add-place panel is open, naming the longitude input as the interceptor. Moving SecondClickX to 450, clear of the panel's own bounds, did not fix it: OverlayBackdrop (position: fixed; inset: 0; z-index: 1299; pointer-events: auto once visible) covers the entire viewport the instant the panel opens, above the map at every point, not only behind the panel's own footprint. Proven directly on a throwaway branch (PR #174, closed): a click at x=450, delivered 600ms after the first click — double the 300ms deferral window, panel unambiguously open — was still intercepted, now by div.overlay-backdrop instead of the longitude input. The coordinate change had no effect on the actual failure rate (measured 3 of 11 completed CI runs, ~27%); it only changed which element got named in the failure.

The actual fix: widen the delivery margin instead of racing it

The test's premise is that both clicks land inside deferSingleClick's deferral window while the first click's own callback is still pending. With clicks staggered 250ms apart against a 300ms production window, that left only ~50ms of real margin for the second click's delivery (Chromium's round trip through Playwright, not merely dispatch) — margin a busy CI runner can and does consume.

DOUBLE_TAP_WINDOW_MS (map.js) is now overridable, for tests only:

const DOUBLE_TAP_WINDOW_MS = globalThis.__placeMarkTestDoubleTapWindowMs ?? 300;

Nothing this application ships ever sets, reads, or exposes that global — no component, no query string, no localStorage key — so production start-up always falls through to the unchanged 300ms literal. The only writer anywhere in this repository is DistantMapClicksBothSurviveJourneyTests, via Playwright's page.AddInitScriptAsync, injected before either click and before any of the page's own scripts run — a surface no real reader's browser has a route to reach.

The test raises the window to 3000ms (ten times production) while leaving the 250ms stagger unchanged, turning the ~50ms delivery margin into ~2.75s — not a smaller race won more often, but a margin no observed CI load comes close to consuming. SecondClickX's move to 450 and the AriaLabel-scoped map locator both stay (still correct on their own merits), but neither is load-bearing now: with the window this wide, the second click is always delivered while the panel is nowhere close to open, so no coordinate could collide with it regardless.

See ADR-0160 for the alternatives considered — including the "prove it without a second real click" option task 233 itself named — and why the query-string/localStorage route was rejected specifically.

Checked against #184 (merged onto main while this branch was in flight, which changes what a map click does while the detail panel is open): that change is gated on PlacesState.SelectedPlace is not null; this test never selects a place, so HandleMapClicked still takes the same add-place branch it always did here. Confirmed by rerunning after rebasing onto it, and again after rebasing onto #184/#186 a second time.

Verification

  • Discrimination proved by mutation, not assumed. With ADR-0145's positional check reverted (deferSingleClick restored to a single pending slot, cancelling on any second click regardless of distance) and the override otherwise unchanged, the test reddened — neither click opened the panel, exactly the pre-fix shape. Restored before this shipped.
  • What actually supports the fix is the structural margin, not a local run count. The flake was CI-load-induced click-delivery delay specifically; an unloaded local machine has no reason to reproduce that condition, so local runs — however many, and however green — are weak evidence either way for a CI-load-specific race, and are not cited here as if they were the proof. The load-bearing evidence is the arithmetic (~50ms margin, which only had to survive incidental CI load, becoming ~2.75s, which would need CI load two orders of magnitude worse than anything observed) together with a green run of the fixed test on CI itself, under the load the flake actually depends on.
  • CI green at head, under real load, on the actual shared runner: run #770 (ea88df8), 12m14s, and reconfirmed after two further rebases.
  • The three sibling tests that also depend on DOUBLE_TAP_WINDOW_MS (MapClickDeferralJourneyTests, DoubleTapDragZoomJourneyTests, MarkerDragCancelsPendingClickJourneyTests) pass unaffected — they never set the override, so they continue to exercise the real 300ms production window.
  • Full PlaceMark.E2E.Tests suite green (28/28); PlaceMark.Architecture.Tests green (129/129, including the new ADR's own index/supersession checks); PlaceMark.WebUI.Tests green (814/818, 4 pre-existing skips).

Scoped to test code plus one production constant's own override hook — no other ADR needed beyond ADR-0160, which records that hook as its own decision.

Fixes task 233. Picks the parked branch back up after the coordinate-change attempt below was disproved. ## Why a coordinate change cannot work (preserved from this PR's earlier analysis) All three logged failures (`be2a075`, `fd0bd0e`, `00aa81d`) showed `SecondClickX` (1050) landing inside `.modal-overlay-panel`'s own footprint once the add-place panel is open, naming the longitude input as the interceptor. Moving `SecondClickX` to 450, clear of the panel's own bounds, did not fix it: **`OverlayBackdrop`** (`position: fixed; inset: 0; z-index: 1299; pointer-events: auto` once visible) covers the *entire viewport* the instant the panel opens, above the map at every point, not only behind the panel's own footprint. Proven directly on a throwaway branch (PR #174, closed): a click at x=450, delivered 600ms after the first click — double the 300ms deferral window, panel unambiguously open — was still intercepted, now by `div.overlay-backdrop` instead of the longitude input. The coordinate change had no effect on the actual failure rate (measured 3 of 11 completed CI runs, ~27%); it only changed which element got named in the failure. ## The actual fix: widen the delivery margin instead of racing it The test's premise is that both clicks land inside `deferSingleClick`'s deferral window while the first click's own callback is still pending. With clicks staggered 250ms apart against a 300ms production window, that left only ~50ms of real margin for the second click's *delivery* (Chromium's round trip through Playwright, not merely dispatch) — margin a busy CI runner can and does consume. `DOUBLE_TAP_WINDOW_MS` (`map.js`) is now overridable, for tests only: ```js const DOUBLE_TAP_WINDOW_MS = globalThis.__placeMarkTestDoubleTapWindowMs ?? 300; ``` Nothing this application ships ever sets, reads, or exposes that global — no component, no query string, no `localStorage` key — so production start-up always falls through to the unchanged 300ms literal. The only writer anywhere in this repository is `DistantMapClicksBothSurviveJourneyTests`, via Playwright's `page.AddInitScriptAsync`, injected before either click and before any of the page's own scripts run — a surface no real reader's browser has a route to reach. The test raises the window to 3000ms (ten times production) while leaving the 250ms stagger unchanged, turning the ~50ms delivery margin into ~2.75s — not a smaller race won more often, but a margin no observed CI load comes close to consuming. `SecondClickX`'s move to 450 and the `AriaLabel`-scoped map locator both stay (still correct on their own merits), but neither is load-bearing now: with the window this wide, the second click is always delivered while the panel is nowhere close to open, so no coordinate could collide with it regardless. See [ADR-0160](docs/adr/0160-a-test-only-override-for-the-double-tap-window.md) for the alternatives considered — including the "prove it without a second real click" option task 233 itself named — and why the query-string/`localStorage` route was rejected specifically. **Checked against #184 (merged onto `main` while this branch was in flight, which changes what a map click does while the detail panel is open):** that change is gated on `PlacesState.SelectedPlace is not null`; this test never selects a place, so `HandleMapClicked` still takes the same add-place branch it always did here. Confirmed by rerunning after rebasing onto it, and again after rebasing onto #184/#186 a second time. ## Verification - **Discrimination proved by mutation, not assumed.** With ADR-0145's positional check reverted (`deferSingleClick` restored to a single pending slot, cancelling on any second click regardless of distance) and the override otherwise unchanged, the test reddened — neither click opened the panel, exactly the pre-fix shape. Restored before this shipped. - **What actually supports the fix is the structural margin, not a local run count.** The flake was CI-load-induced click-delivery delay specifically; an unloaded local machine has no reason to reproduce that condition, so local runs — however many, and however green — are weak evidence either way for a CI-load-specific race, and are not cited here as if they were the proof. The load-bearing evidence is the arithmetic (~50ms margin, which only had to survive incidental CI load, becoming ~2.75s, which would need CI load two orders of magnitude worse than anything observed) together with a green run of the fixed test on CI itself, under the load the flake actually depends on. - **CI green at head, under real load, on the actual shared runner**: run #770 (`ea88df8`), 12m14s, and reconfirmed after two further rebases. - The three sibling tests that also depend on `DOUBLE_TAP_WINDOW_MS` (`MapClickDeferralJourneyTests`, `DoubleTapDragZoomJourneyTests`, `MarkerDragCancelsPendingClickJourneyTests`) pass unaffected — they never set the override, so they continue to exercise the real 300ms production window. - Full `PlaceMark.E2E.Tests` suite green (28/28); `PlaceMark.Architecture.Tests` green (129/129, including the new ADR's own index/supersession checks); `PlaceMark.WebUI.Tests` green (814/818, 4 pre-existing skips). Scoped to test code plus one production constant's own override hook — no other ADR needed beyond ADR-0160, which records that hook as its own decision.
Move the second click clear of the add-place panel's own footprint
Some checks failed
CI / build (pull_request) Failing after 4m45s
Details
CI / e2e (pull_request) Has been skipped
Details
CI / container-images (pull_request) Has been skipped
Details
15d14497e5
Retrigger CI: ConfirmationModal closing-delay test flaked under load, unrelated to this change
All checks were successful
CI / build (pull_request) Successful in 3m13s
Details
CI / container-images (pull_request) Has been skipped
Details
CI / e2e (pull_request) Successful in 2m43s
Details
a7d7de9dec
Retrigger CI: gather a second after-fix E2E data point for task 233
All checks were successful
CI / build (pull_request) Successful in 4m42s
Details
CI / container-images (pull_request) Has been skipped
Details
CI / e2e (pull_request) Successful in 3m24s
Details
e3aae38545
rob reviewed 2026-08-14 21:34:05 +00:00
rob left a comment
Copy link

Verdict: changes needed

Reviewed at head e3aae385.

1. The geometry only clears the panel's own content — it doesn't clear OverlayBackdrop, which is what should actually be intercepting once the panel is open. ModalOverlay.razor renders <OverlayBackdrop Visible="!_closing" .../> alongside the panel, and that component is position: fixed; inset: 0 (full viewport, not just the panel's footprint), z-index: var(--z-overlay) - 1 = 1299, pointer-events: auto once visible, with an @onclick wired to RequestDismissAsync. It doesn't pass HideAboveWideViewport, so it's live at 1280×720. Home.razor's own <LeafletMap ... Inert="OverlayFocus.IsOpen" .../> also goes inert the moment the panel opens. .leaflet-map-host carries no z-index of its own (confirmed in LeafletMap.razor.css), so once the panel has genuinely opened, the backdrop sits above the map everywhere in the viewport, not just behind the panel — x=450 is exempt from the panel's own content but not from this.

Given the click actually registers through Leaflet's native map.on("click", …) listener at real DOM-dispatch time, and the deferred callback that opens the panel doesn't fire until ~300ms after the first click (DOUBLE_TAP_WINDOW_MS), the real question for "does this land or not" is whether the second click's actual delivery beats that 300ms mark — same margin as before, just a different consequence on the losing side. Before the fix, losing it at x=1050 hit the panel's own longitude field (confirmed by all three logged failures). After the fix, losing it at x=450 should, by this reasoning, hit the full-viewport backdrop instead (interception timeout, or a click-away dismiss that never updates the coordinate) — a different failure signature, not a removed race.

All three confirmed production failures used the old x=1050, which sits inside the panel, so none of them says anything about what a late click does at x=450. The two post-fix green runs are equally consistent with "still beating 300ms" as with "genuinely immune once open" — they don't distinguish the two. Given the value the discrimination proof already put on actually watching a guarded thing fail (PR body itself, and ADR-0053/0043/0066 in CLAUDE.md), the same rigour should apply here: force the second click's delivery past 300ms on a throwaway branch (e.g. bump StaggerMs past the deferral window once) and confirm it still lands — if it does, the "outright, timing-independent" claim is proven and this note is moot; if it doesn't, the fix needs to also account for the backdrop, not just the panel.

2. "3/3 green after" overstates what's in the job logs. The commit that actually contains the fix, 15d1449, had exactly one real CI run against it (run #627/id 719): build failed on ConfirmationModalTests.ConfirmationModal_BackdropClicked_RaisesOnCancelAfterTheClosingDelay (independently confirmed from the log — a genuine, unrelated bUnit closing-delay flake, matching the retrigger's own stated reason), and e2e was skipped, contributing no data point either way. Only the two subsequent content-free retrigger commits (a7d7de9, e3aae38) actually ran e2e, and both passed 25/25. So the honest tally is 2 green e2e runs post-fix, not 3 — worth correcting in the PR body and the Vikunja comment. At the stated ~27% pre-fix rate, 2 straight passes has roughly a 53% chance of happening by luck alone, which matters more, not less, given point 1 is still open.

What holds up:

  • Discrimination proof (independently verified against the job log, not just trusted): run #626/id 718, PR #172 (guard reverted), e2e failed with DistantMapClicksBothSurviveJourneyTests.TwoClicksFarApart_WithinTheDeferralWindow_BothOpenAndThenReseedTheAddPlacePanel as the named failing test. The updated test still catches task 229's guard being removed.
  • The drawer (320px) and panel (x∈[816,1264] at 1280px) footprints are exactly as described — checked against the actual CSS (--drawer-width: 20rem, .modal-overlay-panel's right: var(--space-medium); width: min(28rem, …)), not asserted.
  • AriaLabel scoping is sound: Home.razor sets "Your places", the picker (PlaceForm.razor) sets a distinct "Pick this place's location", both surfaced via role="region" aria-label="@AriaLabel" — GetByRole(Region, Name: "Your places", Exact: true) can't silently match the picker's, and a real regression would still redden the assertion.
  • Expect(...).Not.ToHaveValueAsync(...) still discriminates a silently-discarded second click (it polls to timeout rather than reading once, but a value that never changes still fails it).
  • No other E2E test hardcodes a click inside the drawer's or this panel's footprint; the sibling journeys' claim of no shared exposure checks out.
  • Suite total independently confirmed from the run #631/id 723 job log: 25/25.
  • British spelling and no AI attribution are both fine.
Verdict: changes needed Reviewed at head `e3aae385`. **1. The geometry only clears the panel's own content — it doesn't clear `OverlayBackdrop`, which is what should actually be intercepting once the panel is open.** `ModalOverlay.razor` renders `<OverlayBackdrop Visible="!_closing" .../>` alongside the panel, and that component is `position: fixed; inset: 0` (full viewport, not just the panel's footprint), `z-index: var(--z-overlay) - 1` = 1299, `pointer-events: auto` once visible, with an `@onclick` wired to `RequestDismissAsync`. It doesn't pass `HideAboveWideViewport`, so it's live at 1280×720. `Home.razor`'s own `<LeafletMap ... Inert="OverlayFocus.IsOpen" .../>` also goes `inert` the moment the panel opens. `.leaflet-map-host` carries no z-index of its own (confirmed in `LeafletMap.razor.css`), so once the panel has genuinely opened, the backdrop sits above the map everywhere in the viewport, not just behind the panel — x=450 is exempt from the panel's own content but not from this. Given the click actually registers through Leaflet's native `map.on("click", …)` listener at real DOM-dispatch time, and the deferred callback that opens the panel doesn't fire until ~300ms after the *first* click (`DOUBLE_TAP_WINDOW_MS`), the real question for "does this land or not" is whether the *second* click's actual delivery beats that 300ms mark — same margin as before, just a different consequence on the losing side. Before the fix, losing it at x=1050 hit the panel's own longitude field (confirmed by all three logged failures). After the fix, losing it at x=450 should, by this reasoning, hit the full-viewport backdrop instead (interception timeout, or a click-away dismiss that never updates the coordinate) — a different failure signature, not a removed race. All three confirmed production failures used the old x=1050, which sits inside the panel, so none of them says anything about what a late click does at x=450. The two post-fix green runs are equally consistent with "still beating 300ms" as with "genuinely immune once open" — they don't distinguish the two. Given the value the discrimination proof already put on actually watching a guarded thing fail (PR body itself, and ADR-0053/0043/0066 in CLAUDE.md), the same rigour should apply here: force the second click's delivery past 300ms on a throwaway branch (e.g. bump `StaggerMs` past the deferral window once) and confirm it still lands — if it does, the "outright, timing-independent" claim is proven and this note is moot; if it doesn't, the fix needs to also account for the backdrop, not just the panel. **2. "3/3 green after" overstates what's in the job logs.** The commit that actually contains the fix, `15d1449`, had exactly one real CI run against it (run #627/id 719): `build` failed on `ConfirmationModalTests.ConfirmationModal_BackdropClicked_RaisesOnCancelAfterTheClosingDelay` (independently confirmed from the log — a genuine, unrelated bUnit closing-delay flake, matching the retrigger's own stated reason), and `e2e` was **skipped**, contributing no data point either way. Only the two subsequent content-free retrigger commits (`a7d7de9`, `e3aae38`) actually ran `e2e`, and both passed 25/25. So the honest tally is **2 green e2e runs post-fix, not 3** — worth correcting in the PR body and the Vikunja comment. At the stated ~27% pre-fix rate, 2 straight passes has roughly a 53% chance of happening by luck alone, which matters more, not less, given point 1 is still open. **What holds up:** - Discrimination proof (independently verified against the job log, not just trusted): run #626/id 718, PR #172 (guard reverted), `e2e` failed with `DistantMapClicksBothSurviveJourneyTests.TwoClicksFarApart_WithinTheDeferralWindow_BothOpenAndThenReseedTheAddPlacePanel` as the named failing test. The updated test still catches task 229's guard being removed. - The drawer (320px) and panel (x∈[816,1264] at 1280px) footprints are exactly as described — checked against the actual CSS (`--drawer-width: 20rem`, `.modal-overlay-panel`'s `right: var(--space-medium); width: min(28rem, …)`), not asserted. - `AriaLabel` scoping is sound: `Home.razor` sets `"Your places"`, the picker (`PlaceForm.razor`) sets a distinct `"Pick this place's location"`, both surfaced via `role="region" aria-label="@AriaLabel"` — `GetByRole(Region, Name: "Your places", Exact: true)` can't silently match the picker's, and a real regression would still redden the assertion. - `Expect(...).Not.ToHaveValueAsync(...)` still discriminates a silently-discarded second click (it polls to timeout rather than reading once, but a value that never changes still fails it). - No other E2E test hardcodes a click inside the drawer's or this panel's footprint; the sibling journeys' claim of no shared exposure checks out. - Suite total independently confirmed from the run #631/id 723 job log: 25/25. - British spelling and no AI attribution are both fine.
rob changed title from Fix DistantMapClicksBothSurviveJourneyTests' click-vs-panel race to PARKED: coordinate change alone does not fix the click-vs-panel race 2026-08-14 21:46:43 +00:00
rob force-pushed fix/task-233-distant-clicks-flake from e3aae38545
All checks were successful
CI / build (pull_request) Successful in 4m42s
Details
CI / container-images (pull_request) Has been skipped
Details
CI / e2e (pull_request) Successful in 3m24s
Details
to 19adf468d7
All checks were successful
CI / build (pull_request) Successful in 6m17s
Details
CI / container-images (pull_request) Successful in 4s
Details
CI / e2e (pull_request) Successful in 4m20s
Details
2026-08-17 06:26:54 +00:00 Compare
rob changed title from PARKED: coordinate change alone does not fix the click-vs-panel race to Make the deferred-click window overridable to fix the click-vs-panel race (task 233) 2026-08-17 06:27:19 +00:00
rob force-pushed fix/task-233-distant-clicks-flake from 19adf468d7
All checks were successful
CI / build (pull_request) Successful in 6m17s
Details
CI / container-images (pull_request) Successful in 4s
Details
CI / e2e (pull_request) Successful in 4m20s
Details
to ea88df84c6
All checks were successful
CI / build (pull_request) Successful in 7m37s
Details
CI / container-images (pull_request) Successful in 3s
Details
CI / e2e (pull_request) Successful in 4m34s
Details
2026-08-17 06:38:58 +00:00 Compare
rob reviewed 2026-08-17 06:57:54 +00:00
rob left a comment
Copy link

Verdict: mergeable

Reviewed at head ea88df84 (matches the branch tip). Checked the five points independently rather than accepting the report:

  • Discrimination holds. Read deferSingleClick directly: matchIndex is purely point.distanceTo(entry.point) < CLICK_CANCEL_SLOP_PIXELS — never touches DOUBLE_TAP_WINDOW_MS. Reverting to the pre-ADR-0145 shape (findIndex(() => true), matching the leftover mutation on the local chore/task-233-confirm-guard branch) cancels the second click regardless of window width, since the second click (t≈250ms) always lands well inside a still-pending 3000ms entry. The widened window cannot make this pass independent of the positional guard — confirmed structurally, not just from the author's report.
  • No other writer. git grep __placeMarkTestDoubleTapWindowMs across the tree at this commit: one writer (the test, via AddInitScriptAsync), one reader (map.js's ??). Production default is unreachable through any surface the app itself exposes. (It's still a plain global a console or extension could poke — true of any client-side JS constant, not a new attack surface, so no concern beyond what already exists.)
  • #184 interaction claim checks out. Diffed Home.razor.cs between #184's merge parents — the dismiss branch is gated on _editingPlaceId is null && _addingPlaceAtCoordinates is null && PlacesState.SelectedPlace is not null. This test never selects a place, so HandleMapClicked never reaches that branch; #184 is a no-op for this journey exactly as claimed.
  • Answered by correctly unused. Neither ADR-0144 nor ADR-0145 names an explicit open question about making the window configurable (grepped both for it) — ADR-0107's condition 3 isn't met, so a plain new ADR is the right shape.
  • Disproof preserved. The coordinate-change analysis and the backdrop finding survive in the PR body, ADR-0160's Context, and the test's own doc comment — not lost in the retitle.

Two non-blocking points:

  1. ADR-0160's Alternatives don't address task 233's own second option — proving the guarantee without a second real map click (e.g. below the E2E layer, against deferSingleClick or the component directly). Worth a line on why that wasn't taken, even if the answer is "pointer-input guards need real-Chromium proof" (ADR-0145's own justification for the same call).
  2. "14 consecutive local runs, all green" reads stronger than it is: the flake was specifically CI-load-induced delivery delay, which an unloaded local run is unlikely to reproduce regardless of whether the fix works — local runs were probably already reliably green before this change too. The load-bearing evidence is the ~2.75s structural margin plus the green CI run at this head, not the local run count against the 27% CI-measured rate.
Verdict: mergeable Reviewed at head `ea88df84` (matches the branch tip). Checked the five points independently rather than accepting the report: - **Discrimination holds.** Read `deferSingleClick` directly: `matchIndex` is purely `point.distanceTo(entry.point) < CLICK_CANCEL_SLOP_PIXELS` — never touches `DOUBLE_TAP_WINDOW_MS`. Reverting to the pre-ADR-0145 shape (`findIndex(() => true)`, matching the leftover mutation on the local `chore/task-233-confirm-guard` branch) cancels the second click regardless of window width, since the second click (t≈250ms) always lands well inside a still-pending 3000ms entry. The widened window cannot make this pass independent of the positional guard — confirmed structurally, not just from the author's report. - **No other writer.** `git grep __placeMarkTestDoubleTapWindowMs` across the tree at this commit: one writer (the test, via `AddInitScriptAsync`), one reader (`map.js`'s `??`). Production default is unreachable through any surface the app itself exposes. (It's still a plain global a console or extension could poke — true of any client-side JS constant, not a new attack surface, so no concern beyond what already exists.) - **#184 interaction claim checks out.** Diffed `Home.razor.cs` between #184's merge parents — the dismiss branch is gated on `_editingPlaceId is null && _addingPlaceAtCoordinates is null && PlacesState.SelectedPlace is not null`. This test never selects a place, so `HandleMapClicked` never reaches that branch; #184 is a no-op for this journey exactly as claimed. - **`Answered by` correctly unused.** Neither ADR-0144 nor ADR-0145 names an explicit open question about making the window configurable (grepped both for it) — ADR-0107's condition 3 isn't met, so a plain new ADR is the right shape. - **Disproof preserved.** The coordinate-change analysis and the backdrop finding survive in the PR body, ADR-0160's Context, and the test's own doc comment — not lost in the retitle. Two non-blocking points: 1. ADR-0160's Alternatives don't address task 233's own second option — proving the guarantee without a second real map click (e.g. below the E2E layer, against `deferSingleClick` or the component directly). Worth a line on why that wasn't taken, even if the answer is "pointer-input guards need real-Chromium proof" (ADR-0145's own justification for the same call). 2. "14 consecutive local runs, all green" reads stronger than it is: the flake was specifically CI-load-induced delivery delay, which an unloaded local run is unlikely to reproduce regardless of whether the fix works — local runs were probably already reliably green before this change too. The load-bearing evidence is the ~2.75s structural margin plus the green CI run at this head, not the local run count against the 27% CI-measured rate.
docs/adr/0160-a-test-only-override-for-the-double-tap-window.md Outdated
@ -0,0 +103,4 @@
unconditional full-viewport block, ADR-0102, ADR-0116), not a test fix, and belongs in its own ticket
if ever wanted.
**Delete the test, on the grounds that a wall-clock race can never be made trustworthy.** Considered
rob commented 2026-08-17 06:57:54 +00:00
Author
Owner
Copy link

Missing task 233's own second named option: proving the two-independent-deferred-clicks guarantee without a second real map click (unit-level against deferSingleClick, or a bUnit-level component test), which would sidestep the E2E click-delivery round trip entirely. Worth a line here on why that wasn't pursued — non-blocking, but the Alternatives section is meant to be the record of what was weighed.

Missing task 233's own second named option: proving the two-independent-deferred-clicks guarantee without a second real map click (unit-level against `deferSingleClick`, or a bUnit-level component test), which would sidestep the E2E click-delivery round trip entirely. Worth a line here on why that wasn't pursued — non-blocking, but the Alternatives section is meant to be the record of what was weighed.
rob force-pushed fix/task-233-distant-clicks-flake from ea88df84c6
All checks were successful
CI / build (pull_request) Successful in 7m37s
Details
CI / container-images (pull_request) Successful in 3s
Details
CI / e2e (pull_request) Successful in 4m34s
Details
to 0994846890
All checks were successful
CI / build (pull_request) Successful in 3m9s
Details
CI / container-images (pull_request) Successful in 2s
Details
CI / e2e (pull_request) Successful in 3m31s
Details
2026-08-17 07:08:18 +00:00 Compare
rob commented 2026-08-17 07:08:54 +00:00
Author
Owner
Copy link

Both points actioned in 0994846 (on top of a rebase onto #184/#186):

  1. ADR-0160's Consequences now says plainly that the structural margin, not the local run count, is what carries the weight — a local run cannot reproduce CI-load-induced delivery delay regardless of how many times it's repeated, so it's weak evidence either way. PR body corrected the same way.
  2. Added the missing Alternatives entry: proving the guarantee without a second real click, rejected for the same reason ADR-0145 gives for its own click-survival test — the defect is in real click delivery through real CSS layering (OverlayBackdrop), which nothing below the E2E layer can observe.

Rebased onto current main (merge_base caa287c), mergeable.

Both points actioned in `0994846` (on top of a rebase onto #184/#186): 1. ADR-0160's Consequences now says plainly that the structural margin, not the local run count, is what carries the weight — a local run cannot reproduce CI-load-induced delivery delay regardless of how many times it's repeated, so it's weak evidence either way. PR body corrected the same way. 2. Added the missing Alternatives entry: proving the guarantee without a second real click, rejected for the same reason ADR-0145 gives for its own click-survival test — the defect is in real click delivery through real CSS layering (`OverlayBackdrop`), which nothing below the E2E layer can observe. Rebased onto current `main` (merge_base `caa287c`), mergeable.
rob commented 2026-08-17 07:15:23 +00:00
Author
Owner
Copy link

CI green: run #775, head 0994846, 6m42s. Mergeable against current main.

CI green: run #775, head `0994846`, 6m42s. Mergeable against current `main`.
rob reviewed 2026-08-17 07:17:26 +00:00
rob left a comment
Copy link

Verdict: changes needed

Re-reviewed at head 09948468. Confirmed clean: map.js and the test file are byte-identical to ea88df84 (rebase onto #184/#186 touched neither), so the deferSingleClick positional-guard reasoning, the single-writer/single-reader property of __placeMarkTestDoubleTapWindowMs, and the preserved coordinate-change analysis all still hold exactly as before — re-grepped and re-diffed rather than assumed.

The two requested points are handled well:

  • The new Alternatives entry is a real weighing, not a formality — it correctly identifies that the flake was never about deferSingleClick's cancellation logic (which a unit/bUnit test could observe fine) but about whether a real click reaches the map through OverlayBackdrop's CSS layering, which only a real-DOM E2E test can see. That's specific enough to actually stop a re-proposal.
  • The run-count language lands on the right side of the line — better than requested, in fact: rather than reframing "14 local runs" as weak-but-real evidence, both the ADR and PR body drop the local figure entirely and point to the CI-under-load run instead, which is the right evidence class for a CI-load-specific race.

One new problem, introduced by this round, blocking: ADR-0160's Consequences (line 154) cites "(PR #179, PR #183)" as prior instances of this project sending work back for overstating what a local run count establishes. Checked both — #179 was a connection-string-escaping/injection fix, #183 was Leaflet dark-mode theming and a stale ADR-0091 sentence; neither PR's review history says anything about local-run-count evidence. The citation doesn't support the claim it's attached to, in a record that becomes immutable once accepted. Fix the citation (or drop it and let the reasoning stand on its own — it doesn't need the precedent) before merging.

Verdict: changes needed Re-reviewed at head `09948468`. Confirmed clean: `map.js` and the test file are byte-identical to `ea88df84` (rebase onto #184/#186 touched neither), so the `deferSingleClick` positional-guard reasoning, the single-writer/single-reader property of `__placeMarkTestDoubleTapWindowMs`, and the preserved coordinate-change analysis all still hold exactly as before — re-grepped and re-diffed rather than assumed. The two requested points are handled well: - The new Alternatives entry is a real weighing, not a formality — it correctly identifies that the flake was never about `deferSingleClick`'s cancellation logic (which a unit/bUnit test could observe fine) but about whether a real click reaches the map through `OverlayBackdrop`'s CSS layering, which only a real-DOM E2E test can see. That's specific enough to actually stop a re-proposal. - The run-count language lands on the right side of the line — better than requested, in fact: rather than reframing "14 local runs" as weak-but-real evidence, both the ADR and PR body drop the local figure entirely and point to the CI-under-load run instead, which is the right evidence class for a CI-load-specific race. **One new problem, introduced by this round, blocking:** ADR-0160's Consequences (line 154) cites "(PR #179, PR #183)" as prior instances of this project sending work back for overstating what a local run count establishes. Checked both — #179 was a connection-string-escaping/injection fix, #183 was Leaflet dark-mode theming and a stale ADR-0091 sentence; neither PR's review history says anything about local-run-count evidence. The citation doesn't support the claim it's attached to, in a record that becomes immutable once accepted. Fix the citation (or drop it and let the reasoning stand on its own — it doesn't need the precedent) before merging.
docs/adr/0160-a-test-only-override-for-the-double-tap-window.md Outdated
@ -0,0 +151,4 @@
that caused the failure. A run count gathered locally is therefore weak evidence for "this fixes the
CI flake" specifically, however many times it is repeated, and citing one as if it were strong
evidence is exactly the overstatement this project has already sent work back for more than once
(PR #179, PR #183). What actually supports the fix is the arithmetic in this record's own Decision —
rob commented 2026-08-17 07:17:26 +00:00
Author
Owner
Copy link

"(PR #179, PR #183)" doesn't check out — read both PRs' full review history: #179 is about connection-string quoting/injection escaping, #183 is about Leaflet dark-mode theming and a stale ADR-0091 sentence. Neither involves overstating a local run count as evidence. This citation is unsupported and needs fixing (or dropping) before this record is accepted and its body becomes frozen.

"(PR #179, PR #183)" doesn't check out — read both PRs' full review history: #179 is about connection-string quoting/injection escaping, #183 is about Leaflet dark-mode theming and a stale ADR-0091 sentence. Neither involves overstating a local run count as evidence. This citation is unsupported and needs fixing (or dropping) before this record is accepted and its body becomes frozen.
Correct ADR-0160's PR #179/#183 citation to what those PRs actually said
All checks were successful
CI / build (pull_request) Successful in 3m11s
Details
CI / container-images (pull_request) Successful in 3s
Details
CI / e2e (pull_request) Successful in 3m34s
Details
8f003bffe7 
They were about a durable comment asserting more than the code
delivered, not about local-run evidence — the pattern this record's
own claim now names accurately rather than by a mismatched detail.
rob commented 2026-08-17 07:19:04 +00:00
Author
Owner
Copy link

Fixed in 8f003bf. The citation was wrong as written — broadened it to the pattern that genuinely connects #179 and #183 (a durable record asserting more than the evidence in front of it supports), described each accurately (#179's pg_quote round-trip claim, #183's non-existent CI schedule claim), and said plainly this record's own overstated run-count risk is the same kind of failure, not the same detail.

Fixed in `8f003bf`. The citation was wrong as written — broadened it to the pattern that genuinely connects #179 and #183 (a durable record asserting more than the evidence in front of it supports), described each accurately (#179's `pg_quote` round-trip claim, #183's non-existent CI schedule claim), and said plainly this record's own overstated run-count risk is the same kind of failure, not the same detail.
rob commented 2026-08-17 07:26:26 +00:00
Author
Owner
Copy link

CI green: run #776, head 8f003bf, 6m46s. Mergeable against current main.

CI green: run #776, head `8f003bf`, 6m46s. Mergeable against current `main`.
rob reviewed 2026-08-17 07:27:29 +00:00
rob left a comment
Copy link

Verdict: mergeable

Diff from 09948468 to 8f003bff touches only ADR-0160's Consequences paragraph — map.js, the test, and the Alternatives entry are untouched. The corrected citation checks out against what's actually in #179 and #183: pg_quote's comment did claim "every case exercised round-trips" when the trailing-newline case didn't, and the fixture's remarks did assert a schedule-only trigger that ci.yml no longer has — both genuinely "asserted more than the evidence supports," and the generalisation is specific enough to those two facts rather than a vaguer claim that's merely harder to falsify.

Verdict: mergeable Diff from `09948468` to `8f003bff` touches only ADR-0160's Consequences paragraph — `map.js`, the test, and the Alternatives entry are untouched. The corrected citation checks out against what's actually in #179 and #183: `pg_quote`'s comment did claim "every case exercised round-trips" when the trailing-newline case didn't, and the fixture's remarks did assert a schedule-only trigger that `ci.yml` no longer has — both genuinely "asserted more than the evidence supports," and the generalisation is specific enough to those two facts rather than a vaguer claim that's merely harder to falsify.
rob merged commit 57b03cf213 into main 2026-08-17 07:27:49 +00:00
rob deleted branch fix/task-233-distant-clicks-flake 2026-08-17 07:27:49 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
rob/PlaceMark!173
No description provided.