From bd7c9bde0d04ef752db46a716bbb56133d18761a Mon Sep 17 00:00:00 2001 From: mtoohey Date: Wed, 4 Nov 2026 14:40:53 +0000 Subject: [PATCH 1/5] test(channels): deny the failed preference read so recovery can't race Retry ## Problem WebKit's `sidebar-section-creation.spec.mjs:9` "Channels offers empty-section creation with keyboard focus and confirmed persistence" failed at line 167 when clicking "Retry preferences". The click timed out after 10 s. Its call log says "element is not enabled", then "element was detached from the DOM". - clay/bestie-empty-state: run 41547288213, https://github.com/block/buzz-app/actions/runs/41547288213/job/125626193138 - kylez_NEXUS-4082_channel_unread_boundary: run 41553877476 attempt 1, https://github.com/block/buzz-app/actions/runs/41553877476/job/125648251816 (attempt 2 passed) ## Cause The journey's second creation fails the strong channel-sections writer read with an injected 502. `isTransientFailure` counts a 502 as transient, so the preference store publishes the error with `transient`, and relay recovery reads again after `RECOVERY_DELAYS_MS[0]` (1000 ms). That read disables Retry preferences (`disabled={refreshing}`), and its success removes the button. This is intended: recovery owns transient background reads. The trace from run 41547288213 shows the race. The 502 was fulfilled at 153289 ms and the click started at 153482. WebKit's first "visible, enabled and stable" check did not return until 154493. Recovery's read reached the route at 154393, so that check found the button disabled. The button detached at 155188, and the click timed out. Any click that takes more than about a second to land loses the Retry to recovery. ## Fix Inject a 403 instead. A denial is not transient, so `loadState` reports it as settled and recovery leaves it alone. The manual Retry is then the only way out, which is the path this journey exists to exercise. The fixture already uses this reasoning for activity reads ("Activity reads retry a 502 quietly, so their journeys inject a 403"). The failure is recorded under `app.report.relayFailures`, the fixture's allowance for injected relay 403s, rather than `sidebarPreferenceFailures`, which only matches 502s. The check that the alert doesn't show the status now looks for "403". A comment explains why a transient status would race recovery. The existing `writerReads` of 3 after the click now also confirms that no background read ran first. Only the test changes (+7/-4). No other assertions change, and no product code changes or browser cases are added or removed. Open PR #2150 ("Run native Pi agents through plugin execution targets") makes the same 403 and `relayFailures` change in this hunk as part of a feature change. It leaves the alert check on "502". When #2150 rebases, it can take this side of the hunk. ## Validation Local Linux workstation, Playwright 1.63.0, `--no-deps --workers=1 --retries=0`: - Adversarial ordering, using temporary, uncommitted changes: the Retry click was delayed 1.2 s, modelling the slow actionability check, and the first writer read after the injected failure was held for 600 ms. Without the fix: 10/10 failed (5 WebKit, 5 Chromium), all at the Retry click with "element is not enabled" followed by "element was detached from the DOM", the CI signature. Holding the read without delaying the click: 6/6 passed. With the fix: 10/10 passed, and 6/6 passed with a 3.5 s delay, past recovery's first two retries. - Natural contention: WebKit pinned to 4 CPUs shared with 4 busy loops, 2 workers, 24 repeats. Without the fix: 24/24 passed, so load alone did not open the window locally. With the fix: 24/24 passed. - `bin/pnpm test:browser tests/browser/sidebar-section-creation.spec.mjs --project webkit --project chromium --no-deps --workers=1 --grep "empty-section creation" --repeat-each=20`: 40/40 passed. - Full `sidebar-section-creation.spec.mjs`, both engines: 6/6 passed. - `biome check` and `git diff --check` are clean. A green run doesn't prove the flake is gone. Hosted CI covers broad validation. Co-Authored-By: Claude Opus 5.5 Signed-off-by: mtoohey (cherry picked from commit 69eae17b94b702a9c74c154e37ea147441c7ab11) --- tests/browser/sidebar-section-creation.spec.mjs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/browser/sidebar-section-creation.spec.mjs b/tests/browser/sidebar-section-creation.spec.mjs index 7d96f83226..658add4bac 100644 --- a/tests/browser/sidebar-section-creation.spec.mjs +++ b/tests/browser/sidebar-section-creation.spec.mjs @@ -57,7 +57,9 @@ test("Channels offers empty-section creation with keyboard focus and confirmed p writerReads++; if (failWriterRead) { failWriterRead = false; - // A denial keeps manual recovery available while transient reads retry. + // Relay recovery retries a transient failure such as a 502 about a + // second later, which can remove Retry preferences while a slow + // click is still landing. A denial leaves the retry to the user. app.report.relayFailures ??= []; app.report.relayFailures.push(route.request().url()); await route.fulfill({ @@ -163,7 +165,7 @@ test("Channels offers empty-section creation with keyboard focus and confirmed p await expect( dialog.getByRole("button", { name: "Create", exact: true }), ).toBeDisabled(); - await expect(dialog.getByRole("alert")).not.toContainText("502"); + await expect(dialog.getByRole("alert")).not.toContainText("403"); // Recover preferences without replaying creation, then Cancel/reopen. await dialog.getByRole("button", { name: "Retry preferences" }).click(); await expect(field).toBeFocused(); From c020a703effba710eddf875c405cb5668e843de7 Mon Sep 17 00:00:00 2001 From: mtoohey Date: Wed, 4 Nov 2026 21:23:05 +0000 Subject: [PATCH 2/5] test(inbox): wait for the entry reveal before filling an opened conversation ## Problem WebKit's `inbox-archive.spec.mjs:658` "reply and fresh mention reopen their filters without replacing composer draft" failed on attempt 1 on branch peon/people-relationships-cache, and the same SHA passed on rerun: - run 41752428902 (webkit 14/14), https://github.com/block/buzz-app/actions/runs/41752428902/job/126320899490 Error: expect(locator).toHaveText(expected) failed Expected: "Unsent mention retention draft" Received: "" ## Cause Opening a conversation reveals its entry reply: `useMessageReveal` scrolls to the row, sets `tabindex=-1` and focuses it once the thread target is ready, two frames after the commit. The journey filled the composer as soon as it appeared, so on a slow runner the reveal landed during the fill. Playwright fills a contenteditable in two steps: it focuses the editor and selects its contents, then inserts the text at the selection. In WebKit, focusing the row clears the editor's selection, so the insert had nowhere to go. A temporary probe that focused the row between the two steps reproduced it: WebKit inserted nothing, while Chromium kept the selection and inserted the text. The trace shows the same thing. Between the fill's before and after snapshots, the only DOM change is the entry row gaining `tabindex=-1`. The editor's paragraph and trailing break are the same unchanged nodes, so the text never landed. A reset that cleared it would have replaced them. A user typing as the thread finishes loading doesn't lose text to the seed reset. That reset only applies to an untouched draft, and `valueRef` updates synchronously on input. Probes logged no editor reset while text was typed. Characters typed before the reveal stay in the composer. The reveal does take focus from the composer, though. Its first focus for a request doesn't check where focus is now, so keys typed after it go to the row. That focus policy is shared by every reveal, so this change doesn't touch it. Three more journeys in this file open a conversation and fill it, and they share the race. The "unselected row Archive" journey already waited for the entry row to be in viewport plus two frames, but that isn't a barrier: the row is in view before the reveal runs. ## Fix Add a `revealed()` helper. It waits for the conversation list's aria-busy to be false, for the entry row to be focused, and for two frames via the shared `frames()` helper. A focused row is the reveal's own observable effect, so filling after it can't overlap the reveal. All four journeys that fill after Open now call it, replacing the inline in-view wait. Only the test changes (+22/-12). No assertions are loosened, and no product code changes or browser cases are added or removed. ## Validation Local Linux workstation, Playwright 1.63.0, `--no-deps --workers=1 --retries=0`: - Adversarial ordering, using a temporary, uncommitted model in `useMessageReveal`: the entry reveal was held for 800 ms. If a fill placed its selection in a textbox meanwhile, the model ran the reveal at that moment and cleared the selection when the row took focus, as WebKit did in CI. The four journeys, both engines, `--repeat-each=3`: without the fix 17/24 failed, with the empty editor (or a disabled Send) in every journey, the existing in-view wait included. With the fix: 24/24 passed. - The four journeys, both engines, `--repeat-each=10`: 80/80 passed. - The same four journeys with four busy loops pinned to the workstation's CPUs, `--repeat-each=4`: 32/32 passed. - Full `inbox-archive.spec.mjs`, both engines, `--repeat-each=2`: 40/40 passed. - `biome check` and `git diff --check` are clean. A green run doesn't prove the flake is gone. Hosted CI covers broad validation. Co-Authored-By: Claude Opus 5.5 Signed-off-by: mtoohey (cherry picked from commit c715b9eb7603aeca65fb85629fc210189df576c3) --- tests/browser/inbox-archive.spec.mjs | 34 ++++++++++++++++++---------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/tests/browser/inbox-archive.spec.mjs b/tests/browser/inbox-archive.spec.mjs index 9be0bc1158..af04bae933 100644 --- a/tests/browser/inbox-archive.spec.mjs +++ b/tests/browser/inbox-archive.spec.mjs @@ -1,6 +1,7 @@ import { test, expect } from "./fixture.mjs"; import { openPage } from "./navigation.mjs"; import { readJournal } from "./reading.mjs"; +import { frames } from "./timeline.mjs"; const channel = "f12918e7-88d0-4ddd-aa6b-d4888ff6d3bd"; // The Archive shortcut acts on the last opened conversation. @@ -25,6 +26,23 @@ test.use({ screenshot: "off", }); +// Opening a conversation reveals its entry reply, the window's first, by +// focusing that row. In WebKit the focus clears the editor selection, so a +// fill racing the reveal types nothing. The row can be in view before the +// reveal runs, and list aria-busy does not cover reader positioning: wait for +// the focus and the reveal's next-frame acknowledgement before filling. +async function revealed(page, inbox, app) { + await expect( + inbox.getByRole("list", { name: "Inbox conversations" }), + ).toHaveAttribute("aria-busy", "false"); + await expect( + conversations(inbox).locator( + `[data-message-id="${app.inboxWindow.replies[0].id}"]`, + ), + ).toBeFocused(); + await frames(page); +} + async function choose(page, inbox, control, option) { await inbox .getByRole("button", { name: "Inbox filters", exact: true }) @@ -449,6 +467,7 @@ test("Inbox sending keeps the conversation open until manually archived", async await expect( inbox.getByRole("checkbox", { name: "Archive on send" }), ).toHaveCount(0); + await revealed(page, inbox, app); const detail = conversations(inbox); const editor = detail.getByRole("textbox"); await editor.fill("Keep this conversation"); @@ -540,6 +559,7 @@ test("selected row button advances and menu closes without losing the original d const next = rows.filter({ hasText: "Unread neighbor" }); await expect(rows).toHaveCount(2); await thread.getByRole("button", { name: /^Open / }).click(); + await revealed(page, inbox, app); const editor = conversations(inbox).getByRole("textbox"); await editor.fill("Keep the selected archive draft"); await editor.evaluate((el) => { @@ -607,18 +627,7 @@ test("unselected row Archive preserves the open editor and unread neighbor", asy const next = rows.filter({ hasText: "Unread neighbor" }); await expect(rows).toHaveCount(2); await thread.getByRole("button", { name: /^Open / }).click(); - const target = conversations(inbox).locator( - `[data-message-id="${app.inboxWindow.replies[0].id}"]`, - ); - await expect(target).toBeInViewport(); - // Finish entry reveal and its next-frame acknowledgement before taking the - // editor/row-focus baseline; list aria-busy does not cover reader positioning. - await page.evaluate( - () => - new Promise((resolve) => - requestAnimationFrame(() => requestAnimationFrame(resolve)), - ), - ); + await revealed(page, inbox, app); const editor = conversations(inbox).getByRole("textbox"); await editor.fill("Keep the unselected archive draft"); await editor.evaluate((el) => { @@ -671,6 +680,7 @@ test("reply and fresh mention reopen their filters without replacing composer dr await choose(page, inbox, "Show", "Archived"); await expect(rows).toHaveCount(1); await rows.getByRole("button", { name: /^Open / }).click(); + await revealed(page, inbox, app); const detail = conversations(inbox); const editor = detail.getByRole("textbox"); await editor.fill("Unsent mention retention draft"); From 8c941afdfcd66b3a772fa54422115f6d9b82db67 Mon Sep 17 00:00:00 2001 From: mtoohey Date: Wed, 4 Nov 2026 23:13:00 +0000 Subject: [PATCH 3/5] test(voice-notes): serve the fixture as a build so WebKit's first load fits ## Problem The "WebKit recording (macOS)" lane failed in the first case of `voice-notes.spec.mjs`, "record, preview, send and play the chat card with speed and seeking" (:138). Both failures were in the shared beforeEach: `page.goto(.../tests/fixtures/voice-notes.html)` exceeded the 15 s navigation timeout waiting for load. The other three cases in the job passed. - renovate/all-minor-patch: run 41755043277, https://github.com/block/buzz-app/actions/runs/41755043277/job/126329063633 - peon/dnd-stable-transform: run 41757728323 attempt 1, https://github.com/block/buzz-app/actions/runs/41757728323/job/126337712196 (attempt 2 passed) ## Cause The fixture runs the real PluginRuntime, ConversationService, relay session and Voice Notes plugin, and a Vite dev server serves that graph module by module. Load waits until every module has run. Both traces show the client, not Vite, setting the pace: - Run 41755043277 made 818 requests: 314 .js, 263 .ts, 123 .tsx, 82 inline .png and 29 .css. Vite's median wait was 48 ms, but the median receive took 1.14 s and the slowest 2.6 s, for responses of 1-3 KB. The last response ended 13.3 s after the first request started. For over 3 s in the middle, no new request started. - Run 41757728323 made 816 requests: median wait 53 ms, median receive 1.0 s, and 14.5 s from first request to last response. Its `@vite/client` request also waited 6.1 s. Each module costs WebKit a round trip through its network and web processes. On a 3-core macOS runner with a cold browser, the 800 round trips by themselves nearly fill the budget. Later cases reuse the warm browser and pass. #1971 warmed Vite's transforms in beforeAll, which fixed only the server's side. ## Fix Build the fixture once in beforeAll and serve the bundle with `build` and `preview` from `vite-server.mjs`, as `agent-share-card`, `channel-members-focus`, `conversation-name` and `goose-model-row` already do. The page now loads in 4 requests instead of 838, and the warmup goes away because a build has nothing left to transform. The dev server served development React. The build defines `import.meta.env.DEV` and `process.env.NODE_ENV` as `build.mjs` does for `developmentReact`, so StrictMode's double effects and React's development warnings still apply. The first case asserts no unexplained page errors, and the microphone cases count releases. The bundle contains React's development warnings. The last case read the composer's draft by importing `/src/features/messages/attachment-draft.ts` from the dev server inside `page.evaluate`. A bundle has no such URL. The fixture now exposes `attachmentDraft` bound to its session instead. Drafts are cached per session in that module, so this reads the composer's own draft. The manual dev-server preview described in `docs/plugin-architecture.md` still works. No assertions, timeouts or retries change, and no browser cases are added or removed. ## Validation Local Linux workstation, Playwright 1.63.0, WebKit (WPE), `--no-deps --workers=1 --retries=0`. Each run started a new browser, so each run's first case was cold. Times are `page.goto` until load, with the run and busy loops pinned by `taskset`: | CPUs, busy loops | dev server (838 requests) | build (4 requests) | | ---------------- | ------------------------- | ------------------ | | 3, none | 1.15-1.23 s (5) | 0.53-0.65 s (5) | | 3, 3 | 2.0-2.85 s (20) | 0.44-1.21 s (20) | | 2, 8 | 3.7-6.8 s (10) | 1.1-2.55 s (10) | | 1, 3 | 5.1-6.1 s (10) | 2.0-2.3 s (10) | The first case alone, cold, with 3 CPUs and 3 loops: 2.3-2.8 s before, 0.7-1.1 s after. Linux WebKit spends much less per request than the macOS runner did (a median receive of about 1 s there), so these local gains are 2-3x. In CI, where the per-request cost dominated, the gain should be larger. The build itself takes 2.5 s on 3 CPUs and 3.9 s on 1 CPU, inside beforeAll's 120 s. - `bin/pnpm test:browser --config tests/browser/playwright.ci.config.mjs tests/browser/voice-notes.spec.mjs --project chromium --project webkit --no-deps --retries=0 --repeat-each=10` (Chromium runs all five cases, Linux WebKit the untagged one): 60/60 passed. The same command pinned to 3 CPUs shared with 3 busy loops, `--repeat-each=5`: 30/30 passed. - The macOS lane's selection (`--project webkit --grep @media-recorder`) can't pass on Linux, because Playwright's Linux WebKit has no MediaRecorder. Before and after the change, all four cases failed at the same step: Stop recording never appears. Hosted macOS CI runs them. - `biome check --error-on-warnings`, `tsc --noEmit` and `git diff --check` are clean. The flake-check listing test in `tests/integration/flake-check.test.mjs` that reads this spec passes. A green run doesn't prove the flake is gone. Hosted CI covers broad validation. Co-Authored-By: Claude Opus 5.5 Signed-off-by: mtoohey (cherry picked from commit eedec5a9b58fb97afef64f378f1cccdca928a573) --- tests/browser/voice-notes.spec.mjs | 65 ++++++++++++++++++------------ tests/fixtures/voice-notes.tsx | 4 +- 2 files changed, 43 insertions(+), 26 deletions(-) diff --git a/tests/browser/voice-notes.spec.mjs b/tests/browser/voice-notes.spec.mjs index 8c3904fb23..b796f231b1 100644 --- a/tests/browser/voice-notes.spec.mjs +++ b/tests/browser/voice-notes.spec.mjs @@ -1,27 +1,48 @@ import { watchPageErrors } from "./page-errors.mjs"; import { test, expect } from "./playwright-test.mjs"; -import { createServer } from "./vite-server.mjs"; +import { build, preview } from "./vite-server.mjs"; import react from "@vitejs/plugin-react"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { fileURLToPath } from "node:url"; -let server, origin; +let server, directory, url; +// A dev server serves this fixture as about 800 modules. On a busy runner, +// WebKit's first navigation took longer than the navigation timeout to load +// them, so build the page once and serve the bundle. test.beforeAll(async () => { - server = await createServer({ - root: fileURLToPath(new URL("../../", import.meta.url)), + const root = fileURLToPath(new URL("../../", import.meta.url)); + directory = await mkdtemp(join(tmpdir(), "buzz-voice-notes-")); + const config = { + root, configFile: false, envDir: false, + logLevel: "error", plugins: [react()], - server: { host: "127.0.0.1", port: 0 }, + // Development React, as the dev server served, keeps StrictMode's checks. + define: { + "import.meta.env.DEV": "true", + "process.env.NODE_ENV": '"development"', + }, + build: { + rollupOptions: { + input: join(root, "tests/fixtures/voice-notes.html"), + }, + outDir: join(directory, "dist"), + emptyOutDir: true, + target: "esnext", + }, + }; + await build(config); + server = await preview({ + ...config, + preview: { host: "127.0.0.1", port: 0, strictPort: true }, }); - await server.listen(); - origin = server.resolvedUrls.local[0]; - // A new server transforms the fixture's modules, Tailwind's globals.css - // among them, during its first navigation. On a slow runner that took longer - // than the navigation timeout, so transform them before any test navigates. - await server.warmupRequest("/tests/fixtures/voice-notes.tsx"); - await server.waitForRequestsIdle(); + url = `http://127.0.0.1:${server.httpServer.address().port}/tests/fixtures/voice-notes.html`; }); test.afterAll(async () => { - await server?.close(); + if (server) await new Promise((resolve) => server.httpServer.close(resolve)); + if (directory) await rm(directory, { recursive: true, force: true }); }); test.beforeEach(async ({ page }) => { await page.route("**/api/relay/media?*", async (route) => { @@ -38,7 +59,7 @@ test.beforeEach(async ({ page }) => { body: Buffer.from(bytes), }); }); - await page.goto(`${origin}tests/fixtures/voice-notes.html`); + await page.goto(url); await page.evaluate(() => { const mic = { mode: "allow", @@ -376,18 +397,12 @@ test("rejects a full attachment draft before opening the microphone", async ({ await expect( page.getByRole("status").filter({ hasText: /file-\d+\.txt:.*Ready/ }), ).toHaveCount(10); - const uploaded = await page.evaluate(async () => { - const { attachmentDraft } = await import( - "/src/features/messages/attachment-draft.ts" - ); - return attachmentDraft( - window.voiceFixture.session, - "voice-fixture:draft:general", - "general", - ) + const uploaded = await page.evaluate(() => + window.voiceFixture + .attachmentDraft("voice-fixture:draft:general", "general") .snapshot() - .map((item) => item.uploaded); - }); + .map((item) => item.uploaded), + ); expect(uploaded.map((item) => item.type)).toEqual( Array(10).fill("text/plain"), ); diff --git a/tests/fixtures/voice-notes.tsx b/tests/fixtures/voice-notes.tsx index b203337583..ba1d0db829 100644 --- a/tests/fixtures/voice-notes.tsx +++ b/tests/fixtures/voice-notes.tsx @@ -22,6 +22,7 @@ import { type UploadedAttachment, } from "../../src/features/relay/attachments"; import { PublishRejected } from "../../src/features/relay/outbox"; +import { attachmentDraft } from "../../src/features/messages/attachment-draft"; import "../../src/shared/styles/globals.css"; const viewer = keypair(), @@ -161,7 +162,8 @@ Object.assign(window, { voiceFixture: { report, media: (url: string) => urls.get(url), - session: owner.session, + attachmentDraft: (key: string, channelId: string) => + attachmentDraft(owner.session, key, channelId), toggle: (enabled: boolean) => runtime.reconcile(enabled ? [plugin] : []), }, }); From b10980033ea27201f68350f1dd870734268f47f7 Mon Sep 17 00:00:00 2001 From: mtoohey Date: Thu, 5 Nov 2026 00:15:19 +0000 Subject: [PATCH 4/5] test(github): seek the fullscreen PR video without pausing it in the same tick MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem WebKit's `github-body.spec.mjs:35` "PR media plays under the packaged media policy in a narrow GitHub panel" failed at :189, `expect(seek.actual).toBeCloseTo(seek.expected, 1)`. The fullscreen video's `seeked` fired, but `currentTime` was still near half a second instead of 15 (half the duration). - peon/federated-session-bundle: run 41567557720 attempt 1 (Received 0.533400327), https://github.com/block/buzz-app/actions/runs/41567557720/job/125692945945 (the rerun passed) - renovate/all-minor-patch: run 41683322444 (Received 0.48949321), https://github.com/block/buzz-app/actions/runs/41683322444/job/126083327417 ## Cause The fullscreen `VideoPlayer` autoplays, and its `onLoadedMetadata` handler seeks to the inline video's position (`initialTime`). The journey waited for `readyState` and `seekable`. Then, in one evaluate, it called `pause()` on the playing video and immediately set `currentTime` to half the duration. There were two possible explanations: (a) The restore lands after the journey's seek and overwrites it, so the `seeked` the journey awaits is the restore's. (b) WebKit completes a seek issued in the same tick as `pause()` on a playing video without updating `currentTime`. `seeked` fires, but `currentTime` stays where playback paused. This change fixes (b), the only one the evidence supports. Local probes (WebKit WPE, Playwright 1.63.0) repeated the journey's pause-then-seek many times per test. Some also logged every media event, every `currentTime` write and the journey's own seek: - Reloading the fullscreen video before each sample, so it autoplays and restores again, left about 0.2% of samples unmoved: `seeked` fired, and `currentTime` still equalled its value before the seek 300 ms later. That was 3 of 1,500 in the A/B below and 5 of 1,810 in earlier runs. - In all three unmoved samples whose event log was kept, `loadedmetadata` and the restore's write came before the journey's seek. Across 1,800 samples that reopened the viewer, `loadedmetadata` never landed after the journey's seek. Nothing showed (a) happening. - In the two unmoved samples that checked, `requestVideoFrameCallback` reported a presented frame at 15.2 s while `currentTime` still read 0.062 and 0.157. The pipeline had moved, but the position WebKit reported had not. The CI values sit near the inline video's paused position. By themselves they would fit either explanation. The local evidence decides between them. ## Fix Only the journey changes. Before it opens the viewer, a MutationObserver pauses the fullscreen video as soon as it mounts. Pausing clears the autoplay flag, so the video never plays. The observer also resolves `window.fullscreenRestored` on the video's first `seeked`, which is the restore's. The journey awaits that before its own seek and no longer calls `pause()` inside the seek evaluate. The seek now runs on a video that has never played, after the restore has finished. That avoids (b). It also rules out (a), because the restore has already landed when the journey seeks. The assertion stays exact (`toBeCloseTo(…, 1)`). The two inline videos still cover playback: each must reach 0.2 s and decode a frame. The fullscreen video's autoplay was never asserted. There is no product change and no `initialTime` guard in `VideoPlayer`, because nothing showed the restore landing late. No retries, sleeps or timeouts change, and no browser cases are added or removed. ## Validation Local Linux workstation, Playwright 1.63.0, `--no-deps --retries=0`: - Reload A/B, using a temporary, uncommitted spec: the journey up to the fullscreen viewer, then 50 samples per test. Each sample calls `load()` on the fullscreen video, waits for `readyState` and `seekable` as the journey does, and seeks to half the duration. WebKit, 30 repeats, 3 workers per mode, each mode pinned to 4 CPUs shared with 2 busy loops: - old (autoplay, then `pause()` and the seek in one tick): 3/1,500 unmoved (`currentTime` 0.091-0.112 against 15), all with the CI signature, and 1,500/1,500 playing before the seek. - new (`pause()` right after `load()`, then seek after the restore's `seeked`): 0/1,500 unmoved, 0/1,500 playing before the seek. Three against zero fits (b) but isn't statistically conclusive (one-sided p ≈ 0.12). - Reopen A/B, same setup: the viewer was closed and reopened before each of 30 samples per test. The new mode used this change's observer. Old: 0/900 unmoved. New: 0/900 unmoved, 0/900 playing before the seek. Neither mode reproduced the problem, so this run only shows the observer holds the video paused and the restore never landed late (0/1,800). - `bin/pnpm test:browser tests/browser/github-body.spec.mjs --project webkit --project chromium --no-deps --workers=1 --repeat-each=10 --grep "PR media plays"`: 20/20 passed. - The same journey, WebKit, pinned to 4 CPUs shared with 4 busy loops, `--workers=2 --repeat-each=16`: 16/16 passed. - Full `github-body.spec.mjs`, both engines: 2/2 passed. - `biome check` and `git diff --check` are clean. A green run doesn't prove the flake is gone. Hosted CI covers broad validation. Co-Authored-By: Claude Opus 5.5 Signed-off-by: mtoohey (cherry picked from commit c03a0a08deddd973dd29fe6728f9c56f93643f84) --- tests/browser/github-body.spec.mjs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/tests/browser/github-body.spec.mjs b/tests/browser/github-body.spec.mjs index a2e43df25c..b2ded6561f 100644 --- a/tests/browser/github-body.spec.mjs +++ b/tests/browser/github-body.spec.mjs @@ -151,6 +151,24 @@ test("PR media plays under the packaged media policy in a narrow GitHub panel", } for (const source of [before, after]) await expect(panel.locator(`a[href="${source}"]`)).toHaveCount(0); + // WebKit can finish a seek issued just after playback pauses without moving: + // seeked fires and currentTime stays where playback paused. Keep the + // fullscreen video from autoplaying, so the seek below follows no pause, and + // seek only after its initial-position restore has landed. + await page.evaluate(() => { + window.fullscreenRestored = new Promise((resolve) => { + const observer = new MutationObserver(() => { + const video = document.querySelector( + '[role="dialog"][aria-label="Video attachment"] video', + ); + if (!video) return; + observer.disconnect(); + video.pause(); + video.addEventListener("seeked", resolve, { once: true }); + }); + observer.observe(document.body, { childList: true, subtree: true }); + }); + }); await panel .getByRole("button", { name: "Open video fullscreen" }) .nth(1) @@ -175,9 +193,9 @@ test("PR media plays under the packaged media policy in a narrow GitHub panel", ), ) .toBe(true); + await page.evaluate(() => window.fullscreenRestored); // Native seek controls are browser-owned; observe the real media seek boundary. const seek = await fullscreen.evaluate(async (element) => { - element.pause(); const seconds = element.duration / 2; const completed = new Promise((resolve) => element.addEventListener("seeked", resolve, { once: true }), From 63c808ba0b5a018b58a2d01abd1c91fb690ae0a7 Mon Sep 17 00:00:00 2001 From: mtoohey Date: Thu, 5 Nov 2026 01:06:19 +0000 Subject: [PATCH 5/5] test(channels): deny the retried preference read so recovery can't race the final Retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Code review follow-up to "test(channels): deny the failed preference read so recovery can't race Retry" (suggestion, `tests/browser/navigation-groups.spec.mjs` "Create new retains its draft when preferences fail before submission and recovers in place"). The journey injects 502s on `sidebar-preferences`, which relay recovery treats as transient. It clicks "Retry preferences" twice, and the second click comes right after `page.unroute`. Recovery's next background read can then succeed and remove the alert before that click lands. That is the failure mode the earlier commit fixed in `sidebar-section-creation.spec.mjs`. ## Cause The timing, from `src/features/relay/recovery.ts` and the journey: - The first 502 starts recovery. It retries after 1 s and again about 2 s later (`RECOVERY_DELAYS_MS`), and the still-installed route fails both. - The inline "Reconnecting to relay…" notice the journey waits for renders only once `shown` is set, `RECOVERY_NOTICE_DELAY_MS` (3 s) after the first failure. So the first manual Retry comes after one or two background retries. - The dialog's Retry calls `preferences.reload`, not `recovery.retryNow`. Its read cancels recovery's pending timer but keeps `attempt`. When it fails with a 502, recovery schedules its next read at `RECOVERY_DELAYS_MS[attempt]`, 2 s or 4 s later. - The journey then asserts, unroutes and clicks Retry again. If that click hasn't landed when recovery's read runs, the read succeeds because the route is gone. The alert unmounts with its button, and the click times out waiting for it. The window is 2-4 s rather than the about 1 s of the earlier case, so this needs a slower click, but nothing in the journey orders it. ## Fix Keep the first failure a 502, so recovery still owns it and shows the notice the journey asserts. From the first manual Retry on, the route answers 403. A denial is not transient: `loadState` reports it as settled, recovery cancels its timer and stays idle, and the dialog's Retry is the only read left. After the unroute, the final click is the only thing that can recover preferences. The journey still exercises a failed retry that keeps the draft and Retry preferences. A 403 leaves the dialog unwritable the same way. The 403s are recorded under `app.report.relayFailures`, the fixture's allowance for injected relay 403s, and the 502 stays under `sidebarPreferenceFailures`. The manual retry's response wait now matches 403. Only the test changes (+14/-4). No assertions are loosened, and no product code changes or browser cases are added or removed. ## Validation Local Linux workstation, Playwright 1.63.0, `--no-deps --retries=0`: - Adversarial ordering, using a temporary, uncommitted copy of the spec that waited between `page.unroute` and the final Retry click to model a slow click. Both engines, `--workers=2`: - Without the fix, with 4.5 s: 4/4 failed (2 WebKit, 2 Chromium), all at the final click with "locator.click: Timeout 10000ms exceeded ... waiting for ... Retry preferences". - Without the fix, with 1.5 s: 6/6 passed, consistent with the 2-4 s window. - With the fix, with 4.5 s: 6/6 passed. - `bin/pnpm test:browser tests/browser/navigation-groups.spec.mjs --project webkit --project chromium --workers=4 --repeat-each=10 --grep "Create new retains"`: 20/20 passed. - Full `navigation-groups.spec.mjs`, both engines: 20/20 passed. - `biome check --error-on-warnings` and `git diff --check` are clean. No CI failure of this journey was looked up. The race was found by reading the code and confirmed with the forced delay. Co-Authored-By: Claude Opus 5.5 Signed-off-by: mtoohey (cherry picked from commit b1eb5a07ec427209e45cfbc51528ce643cd3adfb) --- tests/browser/navigation-groups.spec.mjs | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/tests/browser/navigation-groups.spec.mjs b/tests/browser/navigation-groups.spec.mjs index 6cbb4e180c..90312fe44b 100644 --- a/tests/browser/navigation-groups.spec.mjs +++ b/tests/browser/navigation-groups.spec.mjs @@ -884,13 +884,18 @@ test("Create new retains its draft when preferences fail before submission and r await page.getByText("Diagnostics", { exact: true }).click(); const held = gate(), started = gate(); + // Transient at first, so relay recovery owns the failure and shows its notice. + let status = 502; const failLegacy = async (route) => { started.resolve(); await held.promise; - app.report.sidebarPreferenceFailures ??= []; - app.report.sidebarPreferenceFailures.push(route.request().url()); + const failed = status; + const failures = + failed === 502 ? "sidebarPreferenceFailures" : "relayFailures"; + app.report[failures] ??= []; + app.report[failures].push(route.request().url()); await route.fulfill({ - status: 502, + status: failed, contentType: "application/json", body: JSON.stringify({ error: "Preferences unavailable" }), }); @@ -930,10 +935,19 @@ test("Create new retains its draft when preferences fail before submission and r await expect(dialog.getByRole("alert")).toContainText("Retry preferences"); expect(app.report.sidebarPublications ?? []).toHaveLength(0); // Another failed recovery must keep both the draft and recovery action. + // Recovery would retry a transient failure on its own seconds later, and + // after the unroute below that read could remove Retry preferences while + // the final click is still landing. A 4xx is not transient, so recovery + // leaves it alone and the dialog's Retry is the only way out. The test + // flips the status here, not the click, so a recovery read that lands + // first also gets a 403 and can satisfy the response wait. Reads + // serialize behind the disabled Retry, so toBeEnabled still waits for the + // manual read. + status = 403; const retryFailed = page.waitForResponse( (response) => new URL(response.url()).pathname.endsWith("/sidebar-preferences") && - response.status() === 502, + response.status() === 403, ); await dialog .getByRole("button", { name: "Retry preferences", exact: true })