From 71fac5330f715a98f21bf69236f839111b2270f8 Mon Sep 17 00:00:00 2001 From: efiten Date: Wed, 2 Sep 2026 19:03:44 +0200 Subject: [PATCH 1/3] test(#1616): wait for swatch focus to move instead of reading it immediately MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 589fa987 (2026-08-31) — the last completed master run before today master push 859173f1 (2026-09-02) — the first completed master run after #1938 PR #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) Claude-Session: https://claude.ai/code/session_01Wzwr3eXseyNM7Xj598djjE --- test-channel-color-picker-e2e.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test-channel-color-picker-e2e.js b/test-channel-color-picker-e2e.js index c8cc335eb..5743610f4 100644 --- a/test-channel-color-picker-e2e.js +++ b/test-channel-color-picker-e2e.js @@ -145,6 +145,24 @@ function assert(c, m) { if (!c) throw new Error(m || 'assertion failed'); } const firstColor = await page.evaluate(() => document.activeElement.getAttribute('data-color')); await page.keyboard.press('ArrowRight'); + // Wait for focus to actually move rather than reading activeElement on the + // next tick. The keydown handler moves focus, but under CI load that can + // land after Playwright's evaluate has already run, and the assertion then + // compares the swatch against itself: "was #ef4444, now #ef4444". + // + // This is the same macrotask race the "outside click" step below documents + // at length for #1317; the fix there was to wait on the real condition + // instead of a proxy, and this step needs it too. Observed failing on the + // master pushes for 589fa987 (2026-08-31) and 859173f1 (2026-09-02), and + // on PR #1884, which passed unchanged on a re-run. + try { + await page.waitForFunction((prev) => { + const el = document.activeElement; + if (!el || !el.classList || !el.classList.contains('cc-swatch')) return false; + const c = el.getAttribute('data-color'); + return !!c && c !== prev; + }, firstColor, { timeout: 3000 }); + } catch (_) { /* fall through so the assertion reports the actual value */ } const nextColor = await page.evaluate(() => document.activeElement.getAttribute('data-color')); assert(nextColor && nextColor !== firstColor, From f1346bec4845894821a83c50144d14c219da01c5 Mon Sep 17 00:00:00 2001 From: efiten Date: Wed, 2 Sep 2026 21:46:29 +0200 Subject: [PATCH 2/3] test: wait for the multibyte toggle to be restored, not merely present MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second instance of the same race, found by re-running the master pipeline for 859173f1. 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 (#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 #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) Claude-Session: https://claude.ai/code/session_01Wzwr3eXseyNM7Xj598djjE --- test-live-multibyte-only-e2e.js | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/test-live-multibyte-only-e2e.js b/test-live-multibyte-only-e2e.js index d33fa7c8c..4a0423cdf 100644 --- a/test-live-multibyte-only-e2e.js +++ b/test-live-multibyte-only-e2e.js @@ -119,8 +119,24 @@ function makePkt(hash, rawHex) { localStorage.setItem('live-multibyte-only', 'true'); }); await persistPage.goto(BASE + '/#/live', { waitUntil: 'domcontentloaded' }); - await persistPage.waitForFunction(() => !!document.getElementById('liveMultibyteToggle'), { timeout: 15000 }); - const checked = await persistPage.evaluate(() => document.getElementById('liveMultibyteToggle').checked); + // Wait for the toggle to be RESTORED, not merely present. The element + // existing does not mean the code that reads localStorage and sets + // .checked has run yet, and under CI load it frequently has not. Waiting + // on existence is a proxy for the thing under test, which is the same + // mistake #1317 documented in test-channel-color-picker-e2e.js. + // + // Observed failing on the master push for 859173f1 (2026-09-02, re-run) + // with none of the PR code that first surfaced it (#1933) present. + try { + await persistPage.waitForFunction(() => { + const el = document.getElementById('liveMultibyteToggle'); + return !!el && el.checked === true; + }, { timeout: 15000 }); + } catch (_) { /* fall through so the assertion reports what it actually saw */ } + const checked = await persistPage.evaluate(() => { + const el = document.getElementById('liveMultibyteToggle'); + return el ? el.checked : '(toggle absent)'; + }); await persistCtx.close(); assert(checked === true, 'multibyte toggle should restore checked=true from localStorage'); }); From 21cb99b8d398a090447be562d866d412c5b049f2 Mon Sep 17 00:00:00 2001 From: efiten Date: Wed, 2 Sep 2026 21:59:52 +0200 Subject: [PATCH 3/3] Revert the multibyte half: it masked a product bug, not a test race @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 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) Claude-Session: https://claude.ai/code/session_01Wzwr3eXseyNM7Xj598djjE --- test-live-multibyte-only-e2e.js | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/test-live-multibyte-only-e2e.js b/test-live-multibyte-only-e2e.js index 4a0423cdf..d33fa7c8c 100644 --- a/test-live-multibyte-only-e2e.js +++ b/test-live-multibyte-only-e2e.js @@ -119,24 +119,8 @@ function makePkt(hash, rawHex) { localStorage.setItem('live-multibyte-only', 'true'); }); await persistPage.goto(BASE + '/#/live', { waitUntil: 'domcontentloaded' }); - // Wait for the toggle to be RESTORED, not merely present. The element - // existing does not mean the code that reads localStorage and sets - // .checked has run yet, and under CI load it frequently has not. Waiting - // on existence is a proxy for the thing under test, which is the same - // mistake #1317 documented in test-channel-color-picker-e2e.js. - // - // Observed failing on the master push for 859173f1 (2026-09-02, re-run) - // with none of the PR code that first surfaced it (#1933) present. - try { - await persistPage.waitForFunction(() => { - const el = document.getElementById('liveMultibyteToggle'); - return !!el && el.checked === true; - }, { timeout: 15000 }); - } catch (_) { /* fall through so the assertion reports what it actually saw */ } - const checked = await persistPage.evaluate(() => { - const el = document.getElementById('liveMultibyteToggle'); - return el ? el.checked : '(toggle absent)'; - }); + await persistPage.waitForFunction(() => !!document.getElementById('liveMultibyteToggle'), { timeout: 15000 }); + const checked = await persistPage.evaluate(() => document.getElementById('liveMultibyteToggle').checked); await persistCtx.close(); assert(checked === true, 'multibyte toggle should restore checked=true from localStorage'); });