test(app): harden flaky e2e synchronization (#40556)

packages/app/e2e/AGENTS.md

@@ -0,0 +1,16 @@
+## Required Reading
+
+- Before writing, changing, or reviewing E2E tests, ALWAYS read and follow Playwright's official [Best Practices](https://playwright.dev/docs/best-practices), [Auto-waiting](https://playwright.dev/docs/actionability), and [Assertions](https://playwright.dev/docs/test-assertions) guides.
+- Use the official [Locators](https://playwright.dev/docs/locators), [Network](https://playwright.dev/docs/network), and [Test Isolation](https://playwright.dev/docs/browser-contexts) guides when those concerns apply.
+
+## Test Hygiene
+
+- Test user-visible behavior with isolated, deterministic data and scoped, unique locators.
+- Prefer role, label, text, and explicit test-contract locators. Do not use `.first()` or `.last()` merely to silence strictness errors.
+- Use locator actions, Playwright auto-waiting, and web-first assertions for observable readiness and outcomes.
+- NEVER use `waitForTimeout`, `setTimeout`, sleeps, animation-frame counts, or other wall-clock delays to synchronize a test. Wait for the specific UI state, request, response, event, or application outcome instead.
+- Do not treat navigation, a network response, DOM attachment, or visibility alone as proof that asynchronously rendered UI is ready. Assert the state the next action actually requires.
+- Register event and network waits before the action that triggers them.
+- Do not retry state-changing actions. Retry idempotent readiness checks, then perform the action once and assert its outcome.
+- Keep action and assertion timeouts adaptive. Do not use short timeouts as readiness probes or rely on retries to hide flakes.
+- Assert exact outcomes and identities so stale state, duplicate rendering, and interactions with the wrong element cannot pass.

packages/app/e2e/performance/timeline-stability/fixture.ts

@@ -197,7 +197,9 @@ export async function setupTimeline(
       )
     },
     async waitForPart(partID: string) {
-      await expect(page.locator(`[data-timeline-part-id="${partID}"]`).first()).toBeVisible()
+      const part = page.locator(`[data-timeline-part-id="${partID}"]`)
+      await expect(part).toHaveCount(1)
+      await expect(part).toBeVisible()
     },
   }
 }

packages/app/e2e/regression/review-line-comment.spec.ts

@@ -18,6 +18,7 @@ test("opens the comment editor when code is clicked", async ({ page }) => {
   await line.click()
 
   await expect(review.getByRole("textbox")).toBeVisible()
+  await expect(review.locator('[data-slot="line-comment-editor-label"]')).toHaveText("Commenting on line 2")
 })
 
 test("opens the comment editor when a line number is clicked", async ({ page }) => {
@@ -27,6 +28,7 @@ test("opens the comment editor when a line number is clicked", async ({ page })
   await lineNumber.click()
 
   await expect(review.getByRole("textbox")).toBeVisible()
+  await expect(review.locator('[data-slot="line-comment-editor-label"]')).toHaveText("Commenting on line 1")
 })
 
 test("opens the comment editor for a line number range", async ({ page }) => {
@@ -36,15 +38,10 @@ test("opens the comment editor for a line number range", async ({ page }) => {
   await expectAppVisible(start)
   await expectAppVisible(end)
 
-  const from = await start.boundingBox()
-  const to = await end.boundingBox()
-  if (!from || !to) throw new Error("Missing line number bounds")
-  await page.mouse.move(from.x + from.width / 2, from.y + from.height / 2)
-  await page.mouse.down()
-  await page.mouse.move(to.x + to.width / 2, to.y + to.height / 2)
-  await page.mouse.up()
+  await start.dragTo(end)
 
   await expect(review.getByRole("textbox")).toBeVisible()
+  await expect(review.locator('[data-slot="line-comment-editor-label"]')).toHaveText("Commenting on lines 1-3")
 })
 
 test("shows a comment button when a line number is hovered", async ({ page }) => {
@@ -54,31 +51,40 @@ test("shows a comment button when a line number is hovered", async ({ page }) =>
 
   const comment = review.getByRole("button", { name: "Comment", exact: true })
   await expect(async () => {
-    await page.mouse.move(0, 0)
     await lineNumber.hover()
-    await expect(comment).toBeVisible({ timeout: 500 })
-    await comment.click({ timeout: 500 })
-  }).toPass()
+    await expect(lineNumber).toHaveAttribute("data-hovered", "")
+    await expect(comment).toHaveCount(1)
+    await expect(comment).toHaveCSS("pointer-events", "auto")
+    await comment.focus()
+    await expect(comment).toBeFocused()
+  }).toPass({ timeout: 10_000 })
+  await comment.press("Enter")
   await expect(review.getByRole("textbox")).toBeVisible()
+  await expect(review.locator('[data-slot="line-comment-editor-label"]')).toHaveText("Commenting on line 1")
 })
 
 test("stages a submitted line comment in the prompt context", async ({ page }) => {
-  const requests: string[] = []
   page.on("request", (request) => {
-    if (request.method() !== "GET") requests.push(`${request.method()} ${new URL(request.url()).pathname}`)
+    expect
+      .soft(request.method(), `unexpected ${request.method()} ${new URL(request.url()).pathname}`)
+      .toBe("GET")
   })
 
   const review = page.locator('[data-component="session-review"]')
   await review.getByText("export const value = 'after'", { exact: true }).click()
-  await review.getByRole("textbox").fill("Use the existing value instead")
-  await review.locator('[data-slot="line-comment-action"][data-variant="primary"]').click()
+  const textbox = review.getByRole("textbox")
+  await expect(textbox).toBeVisible()
+  await expect(review.locator('[data-slot="line-comment-editor-label"]')).toHaveText("Commenting on line 2")
+  await textbox.fill("Use the existing value instead")
+  const submit = review.locator('[data-slot="line-comment-action"][data-variant="primary"]')
+  await expect(submit).toBeEnabled()
+  await submit.click()
 
   await expect(review.getByText("Use the existing value instead", { exact: true })).toBeVisible()
   await page.getByRole("tab", { name: "Session" }).click()
   const context = page.getByText("Use the existing value instead", { exact: true }).last()
   await expect(context).toBeVisible()
   await expect(context.locator("..")).toContainText("review.ts:2")
-  expect(requests).toEqual([])
 })
 
 async function openReview(page: Page) {
@@ -144,15 +150,22 @@ async function openReview(page: Page) {
 
   await page.goto(`/${base64Encode(directory)}/session/${sessionID}`)
   await expectSessionTitle(page, title)
-  const diffResponse = page.waitForResponse((response) => new URL(response.url()).pathname === "/api/vcs/diff")
-  await page.getByRole("tab", { name: "Changes" }).click()
+  const changes = page.getByRole("tab", { name: "Changes" })
+  const diffResponse = page.waitForResponse(
+    (response) =>
+      response.request().method() === "GET" && response.ok() && new URL(response.url()).pathname === "/api/vcs/diff",
+  )
+  await changes.click()
   expect((await (await diffResponse).json()).data).toHaveLength(1)
+  await expect(page.getByRole("tab", { selected: true })).toHaveAccessibleName(/Files Changed/)
 
   const review = page.locator('[data-component="session-review"]')
   await expectAppVisible(review)
-  await review
-    .getByRole("heading", { name: /review\.ts/ })
-    .getByRole("button")
-    .first()
-    .click()
+  const file = review.locator('[data-file="src/review.ts"]')
+  await expectAppVisible(file)
+  const trigger = file.getByRole("button", { expanded: false })
+  await expect(trigger).toHaveCount(1)
+  await trigger.click()
+  await expect(file.getByRole("button", { expanded: true })).toBeVisible()
+  await expect(file.getByText("export const value = 'after'", { exact: true })).toBeVisible()
 }

packages/app/e2e/regression/session-timeline-transport.spec.ts

@@ -1,9 +1,8 @@
-import { expect, test } from "@playwright/test"
+import { expect, test, type Page } from "@playwright/test"
 import {
   assistantMessage,
   partUpdated,
   setupTimeline,
-  status,
   textPart,
   userMessage,
 } from "../performance/timeline-stability/fixture"
@@ -17,7 +16,7 @@ test("keeps one connection open while delivering multiple events", async ({ page
   await timeline.waitForPart("prt_transport_first")
   await timeline.waitForPart("prt_transport_second")
   expect(first.connectionID).toBe(second.connectionID)
-  expect(await timeline.transport.connections()).toHaveLength(1)
+  await expect.poll(async () => (await timeline.transport.connections()).length).toBe(1)
   expect(await timeline.transport.acknowledgements()).toHaveLength(2)
 })
 
@@ -51,20 +50,28 @@ test("parses split JSON and a split multibyte code point", async ({ page }) => {
 })
 
 test("delivers server heartbeat without mutating the timeline", async ({ page }) => {
+  const sentinelID = "prt_transport_heartbeat_sentinel"
   const timeline = await setupTimeline(page, {
     messages: [userMessage(), assistantMessage([textPart("prt_transport_steady", "steady")])],
   })
-  const before = await page.locator("[data-timeline-row]").allTextContents()
+  await timeline.waitForPart("prt_transport_steady")
+  const before = await stableTimelineRows(page)
 
-  await timeline.transport.heartbeat()
-  await timeline.settle()
+  await timeline.transport.writeRaw(": heartbeat\n\n")
+  await timeline.transport.send(partUpdated(textPart(sentinelID, "heartbeat processed")))
+  await timeline.waitForPart(sentinelID)
 
-  expect(await page.locator("[data-timeline-row]").allTextContents()).toEqual(before)
-  expect(await timeline.transport.connections()).toHaveLength(1)
+  await expect
+    .poll(async () => {
+      const rows = await timelineRows(page)
+      return rows.filter((row) => before.some((item) => item.key === row.key))
+    })
+    .toEqual(before)
+  await expect.poll(async () => (await timeline.transport.connections()).length).toBe(1)
 })
 
 test("reconnects after a clean close", async ({ page }) => {
-  const timeline = await setupTimeline(page, { eventRetry: 10 })
+  const timeline = await setupTimeline(page)
   const first = await timeline.transport.waitForConnection()
 
   await timeline.transport.close()
@@ -77,20 +84,21 @@ test("reconnects after a clean close", async ({ page }) => {
 })
 
 test("reconnects after a stream error", async ({ page }) => {
-  const timeline = await setupTimeline(page, { eventRetry: 10 })
+  const timeline = await setupTimeline(page)
   const first = await timeline.transport.waitForConnection()
 
   await timeline.transport.error("contract failure")
   const second = await timeline.transport.waitForConnection({ after: first.id })
-  await timeline.transport.send(status("busy"))
+  await timeline.transport.send(partUpdated(textPart("prt_transport_error", "after error")))
 
+  await timeline.waitForPart("prt_transport_error")
   await expect.poll(async () => (await timeline.transport.connections()).length).toBe(2)
   expect(second.id).toBeGreaterThan(first.id)
   expect((await timeline.transport.connections())[0]?.endedBy).toBe("error")
 })
 
 test("does not request replay when reconnecting the volatile V2 event stream", async ({ page }) => {
-  const timeline = await setupTimeline(page, { eventRetry: 10, protocol: "v2" })
+  const timeline = await setupTimeline(page, { protocol: "v2" })
   const first = await timeline.transport.send(partUpdated(textPart("prt_transport_id", "event with id")), {
     id: "timeline-event-7",
   })
@@ -112,5 +120,35 @@ test("passes through non-event fetches", async ({ page }) => {
   })
 
   expect(health).toEqual({ healthy: true })
-  expect(await timeline.transport.connections()).toHaveLength(1)
+  await expect.poll(async () => (await timeline.transport.connections()).length).toBe(1)
 })
+
+async function stableTimelineRows(page: Page) {
+  let previous: Awaited<ReturnType<typeof timelineRows>> | undefined
+  let stable = 0
+  await expect
+    .poll(
+      async () => {
+        const next = await timelineRows(page)
+        stable = JSON.stringify(next) === JSON.stringify(previous) ? stable + 1 : 0
+        previous = next
+        return stable
+      },
+      { intervals: [50, 50, 100] },
+    )
+    .toBeGreaterThanOrEqual(2)
+  return previous!
+}
+
+function timelineRows(page: Page) {
+  return page.locator("[data-timeline-key]").evaluateAll((elements) =>
+    elements.map((element) => ({
+      key: element.getAttribute("data-timeline-key"),
+      row: element.querySelector("[data-timeline-row]")?.getAttribute("data-timeline-row"),
+      parts: Array.from(element.querySelectorAll("[data-timeline-part-id]"), (part) =>
+        part.getAttribute("data-timeline-part-id"),
+      ),
+      text: element.textContent,
+    })),
+  )
+}

packages/app/e2e/utils/sse-transport.ts

@@ -247,18 +247,23 @@ export async function installSseTransport<T>(
   return {
     server,
     async waitForConnection(input = {}) {
-      await page.waitForFunction(
+      const connection = await page.waitForFunction(
         (after) => {
           const transport = (window as BrowserTransport).__testSseTransport
           const connections = transport?.command({ type: "connections" }) as SseConnectionRecord[] | undefined
-          return connections?.some((connection) => connection.id > after)
+          return connections?.findLast((connection) => connection.id > after && connection.endedAt === undefined)
         },
         input.after ?? 0,
         { timeout: input.timeout },
       )
-      return (await command<SseConnectionRecord[]>({ type: "connections" })).findLast(
-        (connection) => connection.id > (input.after ?? 0),
-      )!
+      let result: SseConnectionRecord | undefined
+      try {
+        result = await connection.jsonValue()
+      } finally {
+        await connection.dispose()
+      }
+      if (!result) throw new Error("SSE transport connection disappeared while waiting")
+      return result
     },
     send(payload, eventOptions) {
       return command({ type: "send", deliveries: [{ payload, options: eventOptions }], burst: false })