fix(test): deflake channel-color-picker outside-click test (real fix) (#1462)

## Summary

Master CI has been failing on `test-channel-color-picker-e2e.js` — the
"outside click closes popover" step — most recently on run
[26574358472](https://github.com/Kpa-clawbot/CoreScope/actions/runs/26574358472)
(master push `d24246395`). The previous deflake attempt (#1317, commit
62a81776) only papered over part of the race.

## Root cause

`showPopover` in `public/channel-color-picker.js:148-152` installs the
document-level outside-click listener inside a `setTimeout(0)`:

```js
setTimeout(function() {
  document.addEventListener('click', onOutsideClick, true);
  document.addEventListener('keydown', onEscape, true);
}, 0);
```

The previous fix tried to wait for that listener with a `rect.width > 0`
"popover visible" proxy — but visibility ≠ listener install. Under CI
load, the macrotask can be deferred past Playwright's polling
resolution, so `page.mouse.click(700, 500)` fires before the listener
exists, the click is dropped, and the second `waitForFunction` runs out
the 8s default timeout.

## Fix (test-only)

1. **Drain pending macrotasks node-side** with `requestAnimationFrame` ×
2 + `setTimeout(0)` before clicking, so the same scheduler tier the
listener uses has definitely run.
2. **Retry the outside click in a small loop** (up to 10×, 1s each).
Even if the very first synthetic click still races install, subsequent
clicks land cleanly. Each retry is cheap (~ms), and `assert(closed,
...)` gives a clear failure message if the popover never hides.

## Verification

| Scenario | Old test | New test |
|---|---|---|
| Baseline (no artificial delay) | passes | 45/45 clean runs locally |
| Artificially delay listener install to **250ms** | **5/5 FAIL** | 5/5
PASS (popover closes on retry #2) |

Production code untouched. Comment block in-test captures the history so
the next person doesn't re-introduce the race.

## Linked

- Supersedes the partial fix in #1317
- CI run that exposed it:
https://github.com/Kpa-clawbot/CoreScope/actions/runs/26574358472

Co-authored-by: Kpa-clawbot <bot@kpa-clawbot.local>
This commit is contained in:
Kpa-clawbot
2026-05-28 08:10:49 -07:00
committed by GitHub
parent 1ca8497ca2
commit 6d5c731d2e
+46 -19
View File
@@ -162,27 +162,54 @@ function assert(c, m) { if (!c) throw new Error(m || 'assertion failed'); }
});
await step('outside click closes popover', async () => {
// De-flake history: #1317 (62a81776) tried `mouse.click(700,500)` + a
// `rect.width > 0` "listener installed" proxy. That proxy is FALSE — it
// only proves the popover is visible, not that showPopover's
// `setTimeout(0)` document-level click listener has actually run. Under
// CI load the macrotask can be deferred past Playwright's polling
// resolution, so the synthetic click fires BEFORE the listener exists,
// is dropped, and the popover never hides → 8s default-timeout failure
// (see run 26574358472 / d24246395 master push).
//
// Real fix: (1) install a one-shot probe of our own via
// `requestAnimationFrame + setTimeout(0)` and `await` it from
// node-side, guaranteeing showPopover's setTimeout(0) drained;
// (2) retry the click in a small loop, since even with the probe
// there's no synchronous handle on Playwright's internal event-loop
// ordering. Each click is cheap (~ms); the popover hides on the first
// one that reaches the installed listener.
await page.evaluate(() =>
window.ChannelColorPicker.show('#outsidechan', 100, 100));
await page.waitForSelector('.cc-picker-popover');
// Wait for the deferred (setTimeout 0) document-level click listener
// to be installed before dispatching the outside click. Otherwise the
// click races the listener registration and the popover stays open.
await page.waitForFunction(() => {
const el = document.querySelector('.cc-picker-popover');
const rect = el && el.getBoundingClientRect();
return rect && rect.width > 0 && rect.height > 0;
}, { timeout: 5000 });
// Real mouse click at a viewport coordinate that is clearly outside
// the popover (popover anchored at 100,100; click at 700,500).
// page.mouse.click dispatches PointerEvent + MouseEvent with real
// coords, more representative than HTMLElement.click() and reliably
// reaches the document-level capture-phase listener.
await page.mouse.click(700, 500);
await page.waitForFunction(() => {
const el = document.querySelector('.cc-picker-popover');
return el && el.style.display === 'none';
}, { timeout: 15000 });
await page.waitForSelector('.cc-picker-popover', { state: 'visible', timeout: 5000 });
// Drain pending macrotasks (showPopover's setTimeout(0) installs the
// outside-click listener). Wait two animation frames + a setTimeout(0)
// so the same scheduler tier the listener uses has definitely run.
await page.evaluate(() => new Promise((r) => {
requestAnimationFrame(() => requestAnimationFrame(() =>
setTimeout(r, 0)));
}));
// Click outside in a retry loop — if the very first synthetic click
// still races the listener install, subsequent clicks land cleanly.
// Popover anchored at (100,100); click at (700,500) is unambiguously
// outside its bounding rect (popover is ~300×80).
const closed = await (async () => {
for (let i = 0; i < 10; i++) {
await page.mouse.click(700, 500);
try {
await page.waitForFunction(() => {
const el = document.querySelector('.cc-picker-popover');
return el && el.style.display === 'none';
}, { timeout: 1000 });
return true;
} catch (_) {
// Re-check listener install by waiting another rAF and retrying.
await page.evaluate(() => new Promise((r) =>
requestAnimationFrame(() => setTimeout(r, 0))));
}
}
return false;
})();
assert(closed, 'popover did not close after 10 outside-click attempts');
});
// Cleanup