diff --git a/ui/src/App.activity-routing.test.tsx b/ui/src/App.activity-routing.test.tsx index 534bf3ecb1..5a7e5ce7de 100644 --- a/ui/src/App.activity-routing.test.tsx +++ b/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)", () => { diff --git a/ui/src/components/DocumentAnnotationPopover.test.tsx b/ui/src/components/DocumentAnnotationPopover.test.tsx index fc8ecae2b0..f1eeddb3ec 100644 --- a/ui/src/components/DocumentAnnotationPopover.test.tsx +++ b/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(); + // `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(); + }); 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 () => { diff --git a/ui/src/components/task-chat/TaskMessageScroller.test.tsx b/ui/src/components/task-chat/TaskMessageScroller.test.tsx index 29a07575d7..fbe9e82704 100644 --- a/ui/src/components/task-chat/TaskMessageScroller.test.tsx +++ b/ui/src/components/task-chat/TaskMessageScroller.test.tsx @@ -56,6 +56,20 @@ describe("TaskMessageScroller", () => { return container.querySelector(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 { + 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 }); });