From e1ac7d2bda89e61407630ddb1d1b3c7d8f436e2f Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 15 Aug 2026 10:48:11 -0400 Subject: [PATCH] =?UTF-8?q?=E2=9A=93=20ci:=20Settle=20the=20E2E=20Reply=20?= =?UTF-8?q?Before=20the=20Double-Click=20Quote=20(#14840)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(e2e): grant MULTI_CONVO.USE in the mock e2e config `agent-skills-added.spec.ts` drives the composer's `+` command, which opens the added-model popover. That path is gated on MULTI_CONVO.USE: if (!hasMultiConvoAccess || !plusCommandEnabled || isAssistantsEndpoint(endpoint)) return; The mock config never sets `interface.multiConvo`, so the permission falls through to the seeded role default and `handlePlusCommand` returns before opening the popover. The spec then fails on a popover that is absent from the DOM entirely, which reads as a selector or timing problem rather than a missing permission. Set it explicitly, the same way `contextCost` is set just above for the usage gauge — the mock config's job is to make each exercised feature's gate explicit rather than inherit a default. * test(e2e): wait for the reply to settle before the double-click quote `quotes.spec.ts` › 'summons the popup from a native double-click word selection' double-clicks a word as soon as `mockReply` becomes visible. But `sendMessage` resolves on the stream *response*, not on the final render, so the reply can still be re-rendering. A streaming markdown re-render swaps out the text node the selection points at, which collapses the selection — the same mechanism the sibling `selectionchange` test documents deliberately. A double-click landing mid-stream therefore loses its selection before the popup can be clicked, and because the whole gesture is wrapped in `toPass`, every retry re-runs into the same still-streaming reply rather than recovering from a one-off. This is a different race from the one #14777 fixed. That one is the *selection* still settling (touch long-press, native handle drags, block-granularity gestures) and is handled inside QuoteButton. This one is the *reply* still streaming, which no amount of component-side settling can absorb. Observed on a downstream fork running this suite on slower hardware: the test fails all three attempts, deterministically on the in-memory stream store while the Redis lane passes the same shard — the in-memory store's final re-renders land late enough to outlive the gesture. Four separate runs, same split. Wait for the reply text to hold steady before selecting. * ci(e2e): install ffmpeg so first-retry video actually records (#14841) `playwright.config.mock.ts` sets `video: 'on-first-retry'`, but the runner only installs `install-deps chrome`, which does not include ffmpeg. Without it the first retry fails inside `browserContext.newPage` while setting up video recording — before the test body runs. The cost is the retry itself: a genuinely flaky test loses the attempt that would have recovered it, and the reported failure is a video-setup error rather than the original symptom. Bounded and non-fatal on purpose. The CLI has been observed hanging after the download completes on these runners, so the step is wrapped in `timeout` and its failure is swallowed — if ffmpeg cannot be installed the job proceeds exactly as it does today, and no lane is blocked on it. Applied to both jobs that run Playwright (`e2e_shards` and `mcp_tool_list_changed`), since both configure retries. --- .github/workflows/playwright-mock.yml | 16 +++++++++++++++ e2e/specs/mock/quotes.spec.ts | 28 +++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/.github/workflows/playwright-mock.yml b/.github/workflows/playwright-mock.yml index 3baae0e307..db3a275b25 100644 --- a/.github/workflows/playwright-mock.yml +++ b/.github/workflows/playwright-mock.yml @@ -144,6 +144,16 @@ jobs: - name: Verify Chrome is present run: google-chrome --version + # `video: 'on-first-retry'` needs ffmpeg; without it the first retry dies in + # browserContext.newPage before the test body runs, so a flaky test loses the + # retry that would have recovered it. The CLI can hang after the download + # finishes on these runners, so bound it and keep it non-fatal — worst case is + # today's behaviour of retrying without video. + - name: Install Playwright ffmpeg (best effort) + timeout-minutes: 3 + continue-on-error: true + run: timeout -k 10 90 npx playwright install ffmpeg + # The runner's Chrome is an apt package, so its real library dependencies are # already satisfied; all `install-deps` adds here are optional CJK/Thai/Cyrillic # font packages (~21MB from azure.archive.ubuntu.com). Nothing in CI asserts on @@ -235,6 +245,12 @@ jobs: - name: Verify Chrome is present run: google-chrome --version + # ffmpeg for retry video — see the note in the e2e_shards job. + - name: Install Playwright ffmpeg (best effort) + timeout-minutes: 3 + continue-on-error: true + run: timeout -k 10 90 npx playwright install ffmpeg + # Optional fonts only — see the note in the e2e_shards job. - name: Install optional Playwright font dependencies (best effort) timeout-minutes: 4 diff --git a/e2e/specs/mock/quotes.spec.ts b/e2e/specs/mock/quotes.spec.ts index 4e524b9551..37e2cba038 100644 --- a/e2e/specs/mock/quotes.spec.ts +++ b/e2e/specs/mock/quotes.spec.ts @@ -98,6 +98,33 @@ async function doubleClickWord(page: Page, needle: string) { await page.mouse.dblclick(point.x, point.y); } +/** + * Wait for the reply to stop re-rendering before selecting text inside it. + * + * Distinct from the settle window `QuoteButton` applies to a *selection*: this + * is the reply itself still streaming. A markdown re-render swaps out the text + * node the selection points at, which collapses it — so a double-click landing + * mid-stream loses its selection before the popup can be clicked, and every + * `toPass` retry loses the same race rather than recovering from it. + * + * `sendMessage` resolves on the stream *response*, not on the final render, so + * the wait has to be explicit. + */ +async function waitForReplyToSettle(page: Page, needle: string) { + const readReply = () => + page.evaluate((text) => { + const renders = Array.from(document.querySelectorAll('.message-render')); + const host = [...renders].reverse().find((el) => (el.textContent ?? '').includes(text)); + return host?.textContent ?? ''; + }, needle); + + await expect(async () => { + const before = await readReply(); + await page.waitForTimeout(250); + expect(await readReply()).toBe(before); + }).toPass({ timeout: 20000 }); +} + /** * Triple-click the block containing `needle` with native mouse events, which * makes Chromium select that whole block and park the selection's far boundary @@ -311,6 +338,7 @@ test.describe('quote references', () => { const response = await sendMessage(page, 'seed for dblclick'); expect(response.ok()).toBeTruthy(); await expect(mockReply(page)).toBeVisible({ timeout: 20000 }); + await waitForReplyToSettle(page, MOCK_REPLY_TEXT); // A real double-click selects the word under the cursor. Chromium commits // that selection on `dblclick`, AFTER `mouseup` fires, so only a `dblclick`