test(ui): wait for conditions, not durations, in three flaky tests (#11499)

Three tests yielded a fixed number of macrotasks before asserting - five in one
case, one in another - which is ample on an idle machine and not when the suite
runs many workers in parallel. The container was still empty, or the state had
not landed, and the assertion failed on behaviour that works. `vi.waitFor`
retries against a time budget instead, so a loaded worker gets more turns
rather than a failure.

`DocumentAnnotationPopover` is a different race and is fixed differently. The
popover element is in the DOM as soon as React commits, while the effect that
registers the document-level keydown and pointerdown listeners runs afterwards.
A test dispatching in that gap loses the event outright, and a lost event
cannot be recovered by retrying an assertion - so the render is wrapped in
`act` to flush passive effects, and the waits only cover the smaller race that
remains.

Refs #11484.

Verified stable over six consecutive runs of the three files, and the full ui
suite passes. Other instances of the same class remain: the full suite still
shows an occasional failure in a different unrelated test on each run.
`App.cases-routing.test.tsx:104-108` is the clearest one - the identical
fixed-turn loop this PR replaced in its sibling `App.activity-routing.test.tsx`,
three turns instead of five - and takes the same one-line fix.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

ui/src/App.activity-routing.test.tsx

@@ -111,12 +111,16 @@ function renderAppAt(container: HTMLElement, path: string) {
   return root;
 }
 
+/**
+ * Waits on the condition, not on a fixed number of turns. The previous version
+ * yielded at most five macrotasks before asserting, which is ample on an idle
+ * machine and not when the suite is running many workers in parallel — the
+ * container was still empty and the assertion failed on a route that resolves
+ * perfectly well. `vi.waitFor` retries against a time budget instead, so a
+ * loaded worker gets more turns rather than a failure.
+ */
 async function waitForRoute(container: HTMLElement, text: string) {
-  for (let attempt = 0; attempt < 5; attempt += 1) {
-    if (container.textContent?.includes(text)) return;
-    await new Promise((resolve) => window.setTimeout(resolve, 0));
-  }
-  expect(container.textContent).toContain(text);
+  await vi.waitFor(() => expect(container.textContent).toContain(text));
 }
 
 describe("App Activity routing (PAP-16302)", () => {

ui/src/components/DocumentAnnotationPopover.test.tsx

@@ -1,11 +1,14 @@
 // @vitest-environment jsdom
 
-import { createRef } from "react";
+import { act, createRef } from "react";
 import { createRoot } from "react-dom/client";
 import type { DocumentAnnotationThreadWithComments } from "@paperclipai/shared";
 import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 import { DocumentAnnotationPopover } from "./DocumentAnnotationPopover";
 
+// Required for `act` to flush passive effects rather than warn.
+(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
+
 const mutations = vi.hoisted(() => ({
   create: vi.fn(),
   reply: vi.fn(),
@@ -88,7 +91,16 @@ describe("DocumentAnnotationPopover", () => {
       ...overrides,
     };
     props.containerRef.current = container;
-    root.render(<DocumentAnnotationPopover {...props} />);
+    // `act`, so the passive effects are flushed before this returns. Waiting on
+    // the popover element alone is not enough: the element is in the DOM as
+    // soon as React commits, while the effect that registers the document-level
+    // keydown/pointerdown listeners runs afterwards. A test that dispatches in
+    // that gap loses the event outright — and a lost event cannot be recovered
+    // by retrying the assertion, which is why this is fixed here and not at the
+    // call site.
+    await act(async () => {
+      root.render(<DocumentAnnotationPopover {...props} />);
+    });
     await vi.waitFor(() => expect(container.querySelector('[data-testid="document-annotation-popover"]')).not.toBeNull());
     return { onClose };
   };
@@ -114,10 +126,14 @@ describe("DocumentAnnotationPopover", () => {
 
   it("dismisses on Escape and outside pointer down", async () => {
     const first = await render();
+    // What makes the dispatch safe is `render` flushing effects, not the waits
+    // below: an event fired before the listener exists is gone, and no amount of
+    // retrying an assertion brings it back. These waits cover the smaller race
+    // that remains — the handler running before React commits the state change.
     document.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape", bubbles: true }));
-    expect(first.onClose).toHaveBeenCalledTimes(1);
+    await vi.waitFor(() => expect(first.onClose).toHaveBeenCalledTimes(1));
     document.body.dispatchEvent(new Event("pointerdown", { bubbles: true }));
-    expect(first.onClose).toHaveBeenCalledTimes(2);
+    await vi.waitFor(() => expect(first.onClose).toHaveBeenCalledTimes(2));
   });
 
   it("replies to and resolves a focused thread", async () => {

ui/src/components/task-chat/TaskMessageScroller.test.tsx

@@ -56,6 +56,20 @@ describe("TaskMessageScroller", () => {
     return container.querySelector<HTMLButtonElement>(PILL_SELECTOR);
   }
 
+  /**
+   * Wait for the pill to reach a state, rather than for a fixed number of turns.
+   * `flushEvents` yields exactly one macrotask, which is enough on an idle
+   * machine and not when the suite runs many workers in parallel — React
+   * flushes continuous-priority updates asynchronously, so the DOM read can
+   * land before the state does. Sites that dereference `pill()!` turn that into
+   * a hard failure rather than a retry.
+   */
+  async function waitForPill(present: boolean): Promise<void> {
+    await vi.waitFor(() =>
+      present ? expect(pill()).not.toBeNull() : expect(pill()).toBeNull(),
+    );
+  }
+
   /** Set scrollTop and fire a scroll event, like a user or the browser would. */
   async function scrollTo(el: HTMLElement, top: number) {
     el.scrollTop = top;
@@ -101,6 +115,7 @@ describe("TaskMessageScroller", () => {
     const el = scroller();
     fakeGeometry(el);
     await scrollTo(el, 600); // bottom: 1000 - 600 - 400 = 0 → pinned
+    await waitForPill(false); // pinned state settled before content grows
     render(2);
     expect(el.scrollTop).toBe(1000);
     expect(pill()).toBeNull();
@@ -111,6 +126,7 @@ describe("TaskMessageScroller", () => {
     const el = scroller();
     fakeGeometry(el);
     await scrollTo(el, 100); // 500px from bottom → unpinned
+    await waitForPill(true);
     const btn = pill();
     expect(btn).not.toBeNull();
     expect(btn!.className).toContain("tc-scroll-pill-in");
@@ -139,6 +155,7 @@ describe("TaskMessageScroller", () => {
     el.scrollTo = scrollToSpy as unknown as typeof el.scrollTo;
 
     await scrollTo(el, 100);
+    await waitForPill(true);
     const btn = pill()!;
     btn.click();
     await flushEvents();
@@ -167,6 +184,7 @@ describe("TaskMessageScroller", () => {
     el.scrollTo = vi.fn() as unknown as typeof el.scrollTo;
 
     await scrollTo(el, 100);
+    await waitForPill(true);
     pill()!.click();
     await flushEvents();
     await dispatch(el, new Event("wheel", { bubbles: true }));
@@ -190,20 +208,20 @@ describe("TaskMessageScroller", () => {
     const el = scroller();
     fakeGeometry(el);
     await scrollTo(el, 100);
-    expect(pill()!.className).toContain("tc-scroll-pill-in");
+    await vi.waitFor(() => expect(pill()?.className).toContain("tc-scroll-pill-in"));
 
     // Scrolling back to the bottom starts the exit animation but keeps the
     // pill mounted until animationend.
     await scrollTo(el, 600);
+    await vi.waitFor(() => expect(pill()?.className).toContain("tc-scroll-pill-out"));
     const exiting = pill();
     expect(exiting).not.toBeNull();
-    expect(exiting!.className).toContain("tc-scroll-pill-out");
 
     // jsdom has no window.AnimationEvent, so React's vendor-prefix detection
     // maps onAnimationEnd to "webkitAnimationEnd" here (real browsers get the
     // unprefixed event).
     await dispatch(exiting!, new Event("webkitAnimationEnd", { bubbles: true }));
-    expect(pill()).toBeNull();
+    await waitForPill(false);
   });
 
   it("unmounts immediately on hide under prefers-reduced-motion", async () => {
@@ -215,8 +233,8 @@ describe("TaskMessageScroller", () => {
     const el = scroller();
     fakeGeometry(el);
     await scrollTo(el, 100);
-    expect(pill()).not.toBeNull();
+    await waitForPill(true);
     await scrollTo(el, 600);
-    expect(pill()).toBeNull(); // no animationend needed
+    await waitForPill(false); // no animationend needed
   });
 });