From d9ae6f0b721bc91dfaea8bd3b2671b2022d8c351 Mon Sep 17 00:00:00 2001 From: mtoohey Date: Thu, 12 Nov 2026 06:28:05 +0000 Subject: [PATCH 1/3] test(browser): wait for thread rows to leave positioning before pressing or focusing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While a thread positions its first page, ThreadPanel renders its rows with inert={positioning} and sets aria-busy={positioning} on the "Thread messages" region. Playwright's toBeVisible() ignores inert, and locator.press()/focus() make no actionability check, so the two steps #3140 added could run before positioning finished. In the empty-thread test, activity.press("Enter") went to the still-focused reply composer, which is prefilled with "@Local tester": WebKit sent the draft and raised the "Mention people outside this channel?" dialog, Chromium just kept focus in the composer, and the Activity tab never appeared. The toBeFocused() on the composer passes early because the composer sits outside the inert rows. The "finishing activity … stop" variant failed the same way: focus() on Stop was a no-op, so the handoff to "More message actions" never happened. DOM snapshots from all four failing traces show aria-busy="true" and inert="" on the rows at the moment of the press or focus. Not a product bug: users cannot reach inert rows either. Wait for the region's aria-busy="false" — the readiness signal channel-lifecycle, nested-replies and navigation-thread-history already use — after the composer toBeFocused() and before activity.press("Enter"), and in the finishing loop after the bubble is visible and before any focus(), so the bubble variant is covered as well. Verified with `bin/pnpm test:browser:ci --no-deps --workers=1 --repeat-each=8 --reporter=list tests/browser/local-agent-activity.spec.mjs -g "opens an active message|without stealing focus"` (4 tests × 8): - --project webkit: 32/32 passed (2.5 m; 4.1–4.2 s and 3.3–3.4 s). - --project chromium: 32/32 passed (2.1 m; 3.1 s and 2.5–2.7 s). The triage only reproduced the inert-row press by delaying the relay query, not on an idle host, so these runs guard against regression rather than reproduce CI timing. Co-Authored-By: Claude Fable 5.1 Signed-off-by: mtoohey Signed-off-by: Matt Toohey --- tests/browser/local-agent-activity.spec.mjs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/browser/local-agent-activity.spec.mjs b/tests/browser/local-agent-activity.spec.mjs index 61819dd7e3..781c2b39d1 100644 --- a/tests/browser/local-agent-activity.spec.mjs +++ b/tests/browser/local-agent-activity.spec.mjs @@ -554,6 +554,11 @@ test("opens an active message's empty thread without losing its activity preview await expect( thread.getByRole("textbox", { name: "Reply to thread", exact: true }), ).toBeFocused(); + // The composer takes focus before the rows finish positioning, and the + // rows stay inert until then, so a press on one would land in the composer. + await expect( + thread.getByRole("region", { name: "Thread messages", exact: true }), + ).toHaveAttribute("aria-busy", "false"); await activity.press("Enter"); await expect( page.getByRole("tab", { name: "Activity", selected: true }), @@ -607,6 +612,11 @@ for (const focusOn of ["bubble", "stop", "composer"]) { name: /^View agent activity:/, }); await expect(bubble).toBeVisible(); + // Rows stay inert while the thread positions its first page, so focus() + // on a control inside one is a no-op until the region is no longer busy. + await expect( + thread.getByRole("region", { name: "Thread messages", exact: true }), + ).toHaveAttribute("aria-busy", "false"); // Finish entrance and hold only the exit so the intermediate state is testable. await animationsSettled(message.locator("section"), { subtree: true }); // Hold the actual exit animation before its first frame, rather than racing From eb5165cff879bbe9c245c646bf10042594130650 Mon Sep 17 00:00:00 2001 From: mtoohey Date: Thu, 12 Nov 2026 07:14:50 +0000 Subject: [PATCH 2/3] test(browser): wait for the local read mark, not the publication, after an exact reveal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The exact-reveal dwell test ended by polling app.report.readPublications with the default 10 s expect timeout. A publication needs the 300 ms reading dwell (use-reading.ts), then read-state's 5,000 ms publish debounce (read-state.ts schedule()), then flush()'s own-slot read, sign and publish, leaving about 4.5 s of slack. In the failing CI trace the flush's read reached the fixture on time and the relay answered in ~4 ms, but the browser took 5.6 s to receive the response body, and read-state-sign arrived 0.9 s after the poll deadline. evidence.json at teardown held the publication, so the behaviour was correct; the test was waiting on the wrong signal for what it asserts (dwell marks read, open does not). Assert the local read instead, with readJournal from reading.mjs as the sibling test in this file already does: right after the held query is seen, the journal's frontiers are still {} (nothing is marked read at open), and at the end poll for a non-empty frontier set. save() awaits storage.update() before schedule() arms the debounce, so the journal write is strictly earlier than the publication and does not depend on the relay round-trip. The publication path itself stays covered by unread.spec.mjs, which polls readPublications with timeout: 12000. The existing readPublications-unchanged check at open is kept. Verified with `bin/pnpm test:browser:ci --no-deps --reporter=list tests/browser/message-navigation.spec.mjs`: - --project chromium --workers=1 --repeat-each=8 -g "exact reveal uses ordinary dwell": 8/8 passed (3.4–3.5 s each, 1.1 m). - --project webkit --workers=1 --repeat-each=4 -g "exact reveal uses ordinary dwell": 4/4 passed (4.3 s each, 36.8 s). - whole file, --workers=2: Chromium 22/22 (1.5 m), WebKit 22/22 (2.4 m). Biome is clean on the file. The slow body read did not reproduce on an idle host; the triage pinned it to CI load, so the local runs guard against regression rather than reproduce CI timing. Not addressed here: the same default-timeout readPublications poll after page.clock.resume() in "settings blocks reading and focus in a retained thread" (this file) and in notifications.spec.mjs. Co-Authored-By: Claude Fable 5.1 Signed-off-by: mtoohey Signed-off-by: Matt Toohey --- tests/browser/message-navigation.spec.mjs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/browser/message-navigation.spec.mjs b/tests/browser/message-navigation.spec.mjs index 05b7db377a..4fea0d320e 100644 --- a/tests/browser/message-navigation.spec.mjs +++ b/tests/browser/message-navigation.spec.mjs @@ -875,6 +875,8 @@ readTest( try { expect(await openTarget(page, target(app))).toEqual({ status: "opened" }); await seen; + // Opening marks nothing read locally, so nothing can have been published. + expect((await readJournal(page))?.state.frontiers ?? {}).toEqual({}); expect(app.report.readPublications.length).toBe(before); await expect( thread(page).locator(`[data-message-id="${app.exact.target.id}"]`), @@ -886,9 +888,14 @@ readTest( await expect( thread(page).locator(`[data-message-id="${app.exact.target.id}"]`), ).toBeFocused(); + // Dwell marks read locally before the publish debounce even starts; the + // debounced, signed publication itself is covered by unread.spec.mjs. await expect - .poll(() => app.report.readPublications.length) - .toBeGreaterThan(before); + .poll( + async () => + Object.keys((await readJournal(page))?.state.frontiers ?? {}).length, + ) + .toBeGreaterThan(0); }, ); From 0ab64f7d313453b608fda90ac7d59a807d6dae5b Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 13 Nov 2026 17:30:35 +1100 Subject: [PATCH 3/3] test(browser): pause the clock so the exact-reveal journal check cannot race dwell Review of PR 3444 found that the empty-journal assertion added in eb5165cff races the behaviour it sits next to. ThreadPanel's completeTarget sets readingSettled and dispatches reading-positioned before it completes navigation, so use-reading.ts starts the target's 300 ms dwell before openTarget() even resolves. The held depth query does not stop it: the exact target is already shown and focused. If the test needs more than 300 ms of real time to get from there to its IndexedDB read, correct behaviour has already written the target's frontier and `toEqual({})` fails. The check only passed because an idle host gets there in time. Confirmed before changing anything: with a temporary 400 ms wait after `await seen`, the eb5165cff test failed 2/2 on Chromium and 2/2 on WebKit at that assertion, each time with exactly one frontier, `msg:: 1700000182`. Unmodified, it passed 2/2 on WebKit. Control the page clock instead, the way "settings blocks reading and focus in a retained thread" below already does: - Install the clock before the page loads, and pause it at a fixed offset once startup, unread.ensure() and the route are in place. - Start openTarget() without awaiting it and step 16 ms frames until navigation reports "opened". Exact reveal needs two animation frames, and Playwright's clock fakes requestAnimationFrame, so a paused clock would otherwise stall it. Dwell can have earned at most one frame. - Assert the journal is empty and readPublications is unchanged (both kept), and that the target is focused. Fake time is frozen here, so no amount of real time lets dwell elapse. - Advance 300 ms and poll the journal for exactly `{ "msg:": target.created_at }`. This replaces the old "some frontier exists" poll and is exact because the target is the only thread row while its context query is held. The poll only waits for the asynchronous IndexedDB write; it advances no time. - Release the held query, resume the clock, and keep the 81-row and focus checks. The held query, the route handler and navigation completion run on network and promise callbacks, which the clock does not fake. The sibling test's holdReadingFocus() is not needed: startup leaves focus on
, outside every reading surface, and the journal was `{}` at the pause in every run. No timeouts or sleeps were added, and nothing outside this test changed. Verified on a loaded local macOS host (load average 34 to 42) with `bin/pnpm test:browser:ci --no-deps --workers=1 --reporter=list tests/browser/message-navigation.spec.mjs -g "exact reveal uses ordinary dwell"`: - --project=chromium --repeat-each=8: 8/8 passed (1.2 m, 3.4 to 4.1 s). - --project=webkit --repeat-each=8: 8/8 passed (1.5 m, 3.8 to 6.1 s). - With the temporary 400 ms wait after `await seen` in the fixed test, --repeat-each=2: Chromium 2/2 and WebKit 2/2 passed. The wait was removed before committing. - A temporary dump of the fake timers just before the 300 ms advance (5 Chromium runs, 3 WebKit) showed the same state every time: clock at 20,064 ms, one thread row, dwell timer due at 20,364 ms, and after the advance the 5 s publish debounce armed at 20,364 ms. The dwell was never restarted after "opened". - Biome is clean on the file; `git diff --check` is clean. Not run: the rest of message-navigation.spec.mjs, any other spec, and Linux engines. Hosted CI covers those. Signed-off-by: Matt Toohey Co-Authored-By: Claude Fable 5.2 --- tests/browser/message-navigation.spec.mjs | 37 +++++++++++++++++------ 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/tests/browser/message-navigation.spec.mjs b/tests/browser/message-navigation.spec.mjs index 4fea0d320e..92b35f1119 100644 --- a/tests/browser/message-navigation.spec.mjs +++ b/tests/browser/message-navigation.spec.mjs @@ -847,6 +847,11 @@ test.describe("fractional row geometry", () => { readTest( "exact reveal uses ordinary dwell rather than marking read at open", async ({ page, app }) => { + // The revealed target starts its 300 ms dwell as navigation completes, so + // only a paused clock keeps "nothing read at open" from racing that dwell. + // Pause at a fixed offset, not the page's already-ticking clock. + const base = Date.now(); + await page.clock.install({ time: base }); await open(page, app); await page.evaluate(() => window.fixtureRelay.snapshot().session.unread.ensure(), @@ -872,30 +877,42 @@ readTest( await route.continue().catch(() => {}); }); const before = app.report.readPublications.length; + const frontiers = async () => + (await readJournal(page))?.state.frontiers ?? {}; + await page.clock.pauseAt(base + 20_000); try { - expect(await openTarget(page, target(app))).toEqual({ status: "opened" }); + const opening = openTarget(page, target(app)); + // Exact reveal requires real focus for two animation frames. Advance one + // frame at a time until its owner completes, without earning reading dwell. + await expect + .poll(async () => { + await page.clock.runFor(16); + return status(page); + }) + .toBe("opened"); + expect(await opening).toEqual({ status: "opened" }); await seen; // Opening marks nothing read locally, so nothing can have been published. - expect((await readJournal(page))?.state.frontiers ?? {}).toEqual({}); + expect(await frontiers()).toEqual({}); expect(app.report.readPublications.length).toBe(before); await expect( thread(page).locator(`[data-message-id="${app.exact.target.id}"]`), ).toBeFocused(); + // Dwell then marks the target, the only row shown while its context is + // held, read locally before the publish debounce even starts; the + // debounced, signed publication itself is covered by unread.spec.mjs. + await page.clock.runFor(300); + await expect.poll(frontiers).toEqual({ + [`msg:${app.exact.target.id}`]: app.exact.target.created_at, + }); } finally { release(); } + await page.clock.resume(); await expect(thread(page).locator("[data-message-id]")).toHaveCount(81); await expect( thread(page).locator(`[data-message-id="${app.exact.target.id}"]`), ).toBeFocused(); - // Dwell marks read locally before the publish debounce even starts; the - // debounced, signed publication itself is covered by unread.spec.mjs. - await expect - .poll( - async () => - Object.keys((await readJournal(page))?.state.frontiers ?? {}).length, - ) - .toBeGreaterThan(0); }, );