test(#1616): wait for swatch focus to move instead of reading it immediately - #1939
Conversation
…ing it immediately
The ArrowRight step reads document.activeElement on the tick after the key
press:
await page.keyboard.press('ArrowRight');
const nextColor = await page.evaluate(() =>
document.activeElement.getAttribute('data-color'));
The keydown handler moves focus, but under CI load that can land after the
evaluate has already run, so the assertion compares the swatch against itself
and reports "was #ef4444, now #ef4444".
This is the same macrotask race the "outside click" step in this file already
documents at length for Kpa-clawbot#1317. The fix there was to wait on the real condition
rather than on a proxy; this step needs the same treatment and did not get it.
Now waits for activeElement to be a .cc-swatch whose data-color differs from
the one focused before the key press, with a 3s budget. The wait is wrapped so
a timeout falls through to the original assertion, which then reports the value
actually observed rather than a bare Playwright timeout.
Observed failing on:
master push 589fa98 (2026-08-31) — the last completed master run before today
master push 859173f (2026-09-02) — the first completed master run after Kpa-clawbot#1938
PR Kpa-clawbot#1884, which passed unchanged on a re-run
Those are two out of two completed master runs, which is why master has had no
green badge either side of this work.
No product code changed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wzwr3eXseyNM7Xj598djjE
Second instance of the same race, found by re-running the master pipeline for 859173f. That re-run failed on: ✗ setting persists across reload: multibyte toggle should restore checked=true from localStorage with none of the PR code that first surfaced it (Kpa-clawbot#1933) present, which settles that it is a master-level flake and not that PR's doing. The step waited for the element to exist and then read .checked on the next tick: await persistPage.waitForFunction(() => !!document.getElementById('liveMultibyteToggle')); const checked = await persistPage.evaluate(() => ...checked); The element existing does not mean the code that reads localStorage and applies it has run. Waiting on existence is a proxy for the thing under test — the same mistake Kpa-clawbot#1317 documented in test-channel-color-picker-e2e.js. Now waits for the toggle to exist AND be checked, with the same fall-through-to-the-assertion shape as the swatch fix in this PR, and the read reports "(toggle absent)" rather than throwing if the element is genuinely missing. No product code changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wzwr3eXseyNM7Xj598djjE
efiten
changed the title
|
Extended to a second test, found by re-running the same master pipeline. The re-run of So master has at least two E2E steps racing the thing they test, and between them they have failed every completed master run since 2026-08-31. Same shape as the first: it waits for the toggle element to exist, then reads Both now wait on the real condition, both fall through to the original assertion on timeout so a genuine failure still reports what it saw. Two files, +36/-2, no product code. |
|
Thank you for re-running master to settle #1933 — that control was worth more than any argument I could have made. The colour-picker half of this PR looks right to me: On the multibyte half I think the wait hides a product bug rather than a test race, and I have a measurement rather than an opinion. The window is real, and the control is inert inside itIn
So between the panel being painted and the wiring running, the checkboxes exist, are clickable, and have no handler. Measured on
The click flips the DOM, writes nothing, and is then silently undone by Separately, the paint gap: the element appears at 370-405 ms and Why it matters for this PR
The fix I would proposeAttach listeners and restore I am not suggesting you drop anything here — your wait is still better than waiting on existence, and the colour-picker part stands on its own. If you would rather this PR stay test-only, say so and I will open the One correction I owe you from this morning, since it is the same file: my "312 vs 299" comment on #1912 measured the distance between the two declarations, which is not the pair that regex evaluates. Your 312 was a correct measurement of the thing you said you measured. |
@TeTeHacko measured what I assumed. The multibyte assertion was not racing the thing it tests; it was catching a real gap, and waiting on the condition would have hidden it. Verified independently in public/live.js on master: 1104 app.innerHTML = `...` panel written 1152 <input id="liveMultibyteToggle"> exists and is clickable 1256 await (await fetch('/api/config/map')).json() 1543 await loadNodes() 1612 multibyteToggle.checked = multibyteOnly 1614 multibyteToggle.addEventListener('change', ...) Two awaits separate the checkbox existing from it being restored and wired. In that window it is clickable with no handler: the click flips the DOM, writes nothing, and is then silently reverted at line 1613. TeTeHacko measured 3/3 on localhost, plus a 93-112ms stretch (3-5 rendered frames) in which a user whose preference is ON sees the toggle render OFF. So the test failing was the test being intermittently right. Making it wait would leave CI unable to notice if that window grew. The colour-picker fix in this PR stands: activeElement immediately after a keypress genuinely is a proxy, and there is no product bug behind it. The live.js fix belongs in its own PR by the person who found and measured it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wzwr3eXseyNM7Xj598djjE
efiten
changed the title
|
You are right, and I have reverted the multibyte half of this PR. I verified your line numbers rather than taking them on trust, the same way you did with mine this morning. In Two awaits between the checkbox existing and it being restored and wired. Your reading is exact. So the test was not flaky, it was intermittently right. That distinction is the whole point and I collapsed it. I saw "waits on existence, reads a property" and matched it to the pattern from the colour-picker file without asking whether there was a product gap behind it. The measurement that separates the two is the one you did and I did not: click in the window and see whether it persists. Making it wait would have been worse than leaving it red, because a red test at least keeps the window visible. After my change CI could not have told anyone if it grew. The colour-picker half stands and I have kept it. There the proxy really is only a proxy: Please do open the One consequence to name: with my change reverted, master will keep failing intermittently on that assertion until your fix lands. That is the correct trade. A test that is right 3 times in 10 is more useful than one that is quiet. And thank you for the correction on #1912 — you did not owe me that, and offering it while making a stronger point is the reason this exchange has been worth having. |
|
Correction to this PR: the fix here did not work, and the premise behind it was wrong. Filed as #1943 with the evidence. Short version. The swatch step failed again on #1941 (attempt 1), and the timestamps settle it: 16 ms for the whole step, while the wait this PR added has a 3-second budget. So Focus moves and is then taken back. Waiting cannot fix that, because the condition becomes true and then false again. This PR made the test observe the correct intermediate state and still assert against the reverted one. @TeTeHacko, this is the same distinction you made me see on the multibyte half of this PR, and I did not carry it across to the half I kept. You were right about the shape of the problem in this file, not just about that one test. I have not found what re-focuses the first swatch: 69 local runs across four configurations, including the instrumented build CI uses, all pass. #1943 lists what is ruled out and where I would look next. The wait itself is harmless and I am not proposing to revert it, but it should not be read as "this one is already fixed". |
…done (#1945) Closes #1943. The colour picker's keyboard navigation is broken, and the E2E flake that has been failing unrelated PRs (#1940, #1941, and master pushes `589fa987` and `859173f1`) was reporting it correctly. ## Cause `showPopover` deferred focusing the first swatch with an uncancellable `setTimeout(..., 0)` at `channel-color-picker.js:146`, and nothing cleared it on hide. The file contained **zero** `clearTimeout` calls. Reopen the popover while a swatch still holds focus and that timer lands after the user has already pressed an arrow key, pulling focus back to the first swatch. Proven, not argued. Instrumenting `HTMLElement.prototype.focus` with a stack trace, on one open: ``` focus(#f97316) @10205ms <- the keydown handler focus(#ef4444) @10208ms <- channel-color-picker.js:146:58 ``` Three milliseconds apart. ## The user-visible bug Worse than a flaky test. **Open the picker, arrow to a colour, press Enter, and the first colour is assigned instead of the one you chose.** Holding the timing still made the existing suite say so directly: ``` ✗ Enter should assign focused color (#f97316), got #ef4444 ``` ## Why the test looked flaky The revert happens on **every** open. Only whether the assertion reads before or after it varies, which is why an idle machine passes and a loaded runner does not. #1939 (mine) assumed the opposite: a race in which the handler had not yet moved focus, cured by waiting for it. #1943 has the measurement that disproves it. The failing step took **16 ms** while that wait has a **3 second** budget, so the wait was resolving successfully and then the value was reverted underneath it. It never helped. Its comment is corrected in this PR rather than left to mislead the next reader. ## Fix Keep a handle for the timer, cancel a pending one on both show and hide, and inside it do nothing when the popover has since been hidden or when focus already sits inside it. A fresh open still focuses the first swatch, which is what the accessibility behaviour is for. An open that inherits focus, or a user who has already navigated, is left alone. ## Verification - The **regression test added here fails on unmodified master** with `a late focus timer must not move focus after the user did` and passes with the fix. - It is **deterministic, not load-dependent**: it reproduces the exact sequence the stack trace identified (open, Escape, reopen, ArrowRight before the timer lands) rather than waiting for contention. It also asserts the Enter path, so the user-visible half is covered and not just focus position. - Full suite: 10 of 10, three consecutive runs. ## Note on the other flake This is one of two E2E failures blocking the queue. The other, #1925, is a different mechanism in a different file and is fixed separately in #1944. Together they should leave the E2E suite deterministic again. Same shape as @TeTeHacko's finding in #1940: something is operable before its setup has finished. That is now three instances in this codebase, so it may be worth a look as a pattern rather than three separate fixes. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t for ~100 ms (#1940) Follow-up to the #1939 discussion, where @efiten asked for this PR. The multibyte E2E assertion that has been failing intermittently on master is a symptom of this; with this change the unmodified test passes reliably (3/3 idle, 8/8 under a 24-core load run that previously failed it 2 in 6). ## The defect `init()` writes the whole controls panel with `app.innerHTML` and only restores toggle state and attaches the `change` listeners ~330 lines later, behind two awaits (line numbers on master, as verified in the #1939 thread): | line | | |---|---| | 1104 | `app.innerHTML = …` — the checkboxes are in the DOM, clickable | | 1256 | `await (await fetch('/api/config/map')).json()` | | 1543 | `await loadNodes()` | | 1612–1614 | `.checked = <pref>` and `addEventListener('change', …)` | **A click inside that window is silently lost.** Measured on master, localhost, clicking `#liveMultibyteToggle` on the first animation frame in which it exists: | run | click at | immediately after | after 2.5 s | |---|---|---|---| | 1 | 481 ms | `checked=true`, `localStorage=null` | `checked=false`, `localStorage=null` | | 2 | 505 ms | `checked=true`, `localStorage=null` | `checked=false`, `localStorage=null` | | 3 | 438 ms | `checked=true`, `localStorage=null` | `checked=false`, `localStorage=null` | No handler runs, nothing reaches localStorage, and the later `.checked = <pref>` reverts the click with no feedback. Separately, the restored state itself appears only **93–112 ms (3–5 rendered frames)** after the control is painted — `ghost` and `colorHash` default ON, so they visibly flick on for every visitor. ## The fix - **`wireLiveControls()`** — synchronous, right after `app.innerHTML`: restores `.checked` and attaches listeners for the eight persisted toggles, as one table instead of eight near-identical blocks. The matrix↔heat interlock applies from the first paint too. - **`applyLiveControlEffects()`** — after the awaits: applies the effects that need state built there (matrix theme, rain canvas). - **`syncHeatToggleToMatrix()`** — the interlock, extracted; it previously existed as two identical copies. - Heat gets a module-level mirror (`heatEnabled`) like the other seven toggles, so the layer is only built when wanted. Previously it was built unconditionally and torn down ~270 lines later — invisible (no await in between, so no frame composited; the cost is only ~9 ms at 1000 nodes), but any throw between the two calls left the layer visible against the stored preference. `showHeatMap()` now guards on the map existing instead of relying on `nodeData` being empty at that moment. ## Verification - Click on the first painted frame now persists, 3/3 (`localStorage` written, survives). - Restored state present on the first painted frame: 0 unchecked frames in 5 runs (was 3–5). - All four heat×matrix load combinations render identically to master (layer present/absent, checked, disabled). - `test-live-multibyte-only-e2e.js` unmodified: 3/3, plus 8/8 under CPU load. - With stored matrix ON, the heat toggle is `checked=false, disabled=true` from the first frame. ## The probe (as requested) <details><summary>~30-line Playwright harness that demonstrates the inert control</summary> ```js const { chromium } = require('playwright'); (async () => { const b = await chromium.launch(); for (let run = 0; run < 3; run++) { const ctx = await b.newContext({ viewport: { width: 1400, height: 900 } }); const p = await ctx.newPage(); await p.addInitScript(() => { window.__r = { clickedAt: null, afterClick: null, lsAfterClick: null, final: null, lsFinal: null }; const tick = () => { const el = document.getElementById('liveMultibyteToggle'); if (el && window.__r.clickedAt === null) { window.__r.clickedAt = performance.now(); el.click(); window.__r.afterClick = el.checked; window.__r.lsAfterClick = localStorage.getItem('live-multibyte-only'); return; } requestAnimationFrame(tick); }; requestAnimationFrame(tick); }); await p.goto('http://localhost:13581/#/live', { waitUntil: 'domcontentloaded' }); await p.waitForTimeout(2500); const r = await p.evaluate(() => { const el = document.getElementById('liveMultibyteToggle'); window.__r.final = el ? el.checked : null; window.__r.lsFinal = localStorage.getItem('live-multibyte-only'); return window.__r; }); console.log(`run ${run+1}: click at ${Math.round(r.clickedAt)}ms -> checked=${r.afterClick}, ls=${r.lsAfterClick}` + ` || after 2.5s: checked=${r.final}, ls=${r.lsFinal}`); await ctx.close(); } await b.close(); })(); ``` </details> ## Deliberately out of scope (each verified, none regressed here) - `#liveAudioToggle` has the same window (MeshAudio persists `live-audio-enabled`), but its restore runs through `MeshAudio.restore()` and a slider panel — its own change. - `#liveGeoFilterToggle` stays hidden until its own config fetch, so its window is not user-reachable; the fullscreen control is created by Leaflet after the map exists. - Pre-existing: `clearNodeMarkers()` (VCR resume path) drops the heat layer and nothing rebuilds it. `heatEnabled` is the right gate for fixing that, but it is a separate behaviour change. Happy to also submit the deterministic version of the multibyte test (it forces the window open by delaying `/api/config/map`, so it fails on this bug 3/3 instead of intermittently) as a follow-up if wanted.
2 participants
This is the test that has been keeping master red on both sides of today's queue run.
The failure
Observed on:
589fa987859173f1Two out of two completed master runs. Master has produced exactly two finished pipelines since 2026-08-31 and this test failed both, which is why the branch has had no green badge either side of a day of merges.
The cause
It reads
document.activeElementon the tick after the key press. The keydown handler moves focus, but under CI load that can land after the evaluate has already run, so the assertion compares the swatch against itself and reports the same colour twice.This file already knows about this. The "outside click" step below carries a long comment about exactly this macrotask race for #1317, and the conclusion there was to wait on the real condition instead of a proxy. That step got the treatment and this one did not.
The fix
Wait for
activeElementto be a.cc-swatchwhosedata-colordiffers from the one focused before the key press, with a 3s budget. The wait is wrapped so that a timeout falls through to the original assertion, which then reports the value actually observed rather than a bare Playwright timeout — a failing test should still say what it saw.No product code changed. One file, +18 lines, all of it the wait and the reasoning.
What this does not claim
It does not prove the focus handler is correct, only that the test stops racing it. If ArrowRight is ever genuinely broken, this still fails, and now with a useful message.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Wzwr3eXseyNM7Xj598djjE