fix: test: browser tests fail intermittently under machine load (Menubar, VirtualList, NestedLayers)

.changeset/virtuallist-scroll-beats-restore.md

@@ -0,0 +1,5 @@
+---
+"@codeatlasdev/ui": patch
+---
+
+`VirtualList`: a scroll made while the anchor restore after a prepend or a row re-measure is still queued is no longer undone. On a slow machine the late restore dragged the list back to the old anchor, so a user scrolling to the top to load the next page was thrown ~1000px down.

AGENTS.md

@@ -68,7 +68,7 @@ Unit tests run on `bun test` (repo root, `bunfig.toml`); component and visual te
 - tears down the whole test process group on exit, Ctrl+C, SIGTERM or SIGHUP;
 - at the start of every run, kills browsers left by runs whose runner died (SIGKILL, closed terminal, killed agent job).
 
-Ownership is explicit: only processes carrying the tag of a dead run of this repo are touched. Other projects on the machine (`../ac`, …) share `~/Library/Caches/ms-playwright`; never kill Playwright browsers by name or path. To pass extra args: `bun run test:browser -- src/lib/components/Button`. Local runs use a quarter of the cores (`maxWorkers`, Playwright `workers: "25%"`, also when a shell exports `CI=true`: only `GITHUB_ACTIONS` gets every core, #539); behavior tests use the headless shell. VRT only runs on Linux (the `vrt` project excludes everything else): on a Mac use `test:vrt:check`/`test:vrt:update`, never a bare `test:vrt`.
+Ownership is explicit: only processes carrying the tag of a dead run of this repo are touched. Other projects on the machine (`../ac`, …) share `~/Library/Caches/ms-playwright`; never kill Playwright browsers by name or path. To pass extra args: `bun run test:browser -- src/lib/components/Button`. A browser test that must hold on a loaded machine gets its own `*.throttled.svelte.test.ts` (CDP `Emulation.setCPUThrottlingRate`, see `CommandPalette.throttled`); a full run under parallel CPU load (#544) must stay green. Multi-step keyboard flows take `SLOW_FLOW` from `tests/settled.ts` as their timeout; a condition is awaited with `expect.poll`/`expect.element`, never a fixed `setTimeout` deadline. Local runs use a quarter of the cores (`maxWorkers`, Playwright `workers: "25%"`, also when a shell exports `CI=true`: only `GITHUB_ACTIONS` gets every core, #539); behavior tests use the headless shell. VRT only runs on Linux (the `vrt` project excludes everything else): on a Mac use `test:vrt:check`/`test:vrt:update`, never a bare `test:vrt`.
 
 Naming conventions:
 - `*.test.ts` in `packages/ui/tests/`: unit test on `bun:test` (`mock`/`spyOn`/`jest.useFakeTimers`; DOM only through happy-dom's `GlobalRegistrator` inside that file; fresh module via `import("…?fresh=n")`). Types checked by `bun --check`.

packages/ui/src/lib/components/Overlays/ModalContract.svelte.test.ts

@@ -1,7 +1,7 @@
 import { describe, expect, it } from "vitest";
 import { page, userEvent } from "vitest/browser";
 import { render } from "vitest-browser-svelte";
-import { settled } from "../../../../tests/settled";
+import { SLOW_FLOW, settled } from "../../../../tests/settled";
 import Harness from "./ModalContract.test.svelte";
 
 /**
@@ -47,29 +47,33 @@ describe("Dialog modality", () => {
 		expect(document.querySelector("[data-testid='background-count']")?.textContent).toBe("0");
 	});
 
-	it("Tab never lands on a control behind the dialog (forward and backward)", async () => {
-		await openDialog();
-		const dialog = dialogEl();
-		const stops = new Set<Element | null>();
-		for (let i = 0; i < 12; i++) {
-			await userEvent.keyboard("{Tab}");
-			stops.add(document.activeElement);
-			const a = document.activeElement;
-			expect(a === document.body || dialog.contains(a), `Tab stop ${i} stays in the dialog`).toBe(
-				true
-			);
-		}
-		for (let i = 0; i < 12; i++) {
-			await userEvent.keyboard("{Shift>}{Tab}{/Shift}");
-			const a = document.activeElement;
-			expect(
-				a === document.body || dialog.contains(a),
-				`Shift+Tab stop ${i} stays in the dialog`
-			).toBe(true);
-		}
-		const bg = document.querySelector("#app-shell > button");
-		expect(stops.has(bg)).toBe(false);
-	});
+	it(
+		"Tab never lands on a control behind the dialog (forward and backward)",
+		async () => {
+			await openDialog();
+			const dialog = dialogEl();
+			const stops = new Set<Element | null>();
+			for (let i = 0; i < 12; i++) {
+				await userEvent.keyboard("{Tab}");
+				stops.add(document.activeElement);
+				const a = document.activeElement;
+				expect(a === document.body || dialog.contains(a), `Tab stop ${i} stays in the dialog`).toBe(
+					true
+				);
+			}
+			for (let i = 0; i < 12; i++) {
+				await userEvent.keyboard("{Shift>}{Tab}{/Shift}");
+				const a = document.activeElement;
+				expect(
+					a === document.body || dialog.contains(a),
+					`Shift+Tab stop ${i} stays in the dialog`
+				).toBe(true);
+			}
+			const bg = document.querySelector("#app-shell > button");
+			expect(stops.has(bg)).toBe(false);
+		},
+		SLOW_FLOW
+	);
 
 	it("the background subtree is not reachable by the accessibility tree focus order (inert)", async () => {
 		await openDialog();

packages/ui/src/lib/components/VirtualList/VirtualList.svelte

@@ -179,15 +179,15 @@
 
   // Anchor: the first row at/after the viewport top and its distance from it. Restoring it after
   // any layout change above (prepend, a row above re-measuring) keeps the content still.
-  let anchor: { key: string | number; delta: number } | null = null;
+  let anchor: { key: string | number; delta: number; at: number } | null = null;
   let restoring = false;
 
   function anchorBefore() {
     if (!scroller || restoring || keys.length === 0) return;
     // Read the DOM, not `scrollTop` state: a scroll event may not have been delivered yet.
     const top = scroller.scrollTop;
     const i = indexAt(top, offsets);
-    anchor = { key: keys[i], delta: offsets[i] - top };
+    anchor = { key: keys[i], delta: offsets[i] - top, at: top };
   }
 
   async function restoreAnchor() {
@@ -197,7 +197,10 @@
     await tick();
     if (scroller) {
       const i = keys.indexOf(wanted.key);
-      if (i >= 0 && !(follow && atBottom)) {
+      // `at` is where the scroll was when the anchor was taken (or where we last put it). Anywhere
+      // else, the user scrolled while the restore was pending: their position wins (#544).
+      const userMoved = Math.abs(scroller.scrollTop - wanted.at) > 1;
+      if (i >= 0 && !userMoved && !(follow && atBottom)) {
         const next = offsets[i] - wanted.delta;
         if (Math.abs(next - scroller.scrollTop) > 0.5) scroller.scrollTop = next;
       }

packages/ui/src/lib/components/VirtualList/VirtualList.svelte.test.ts

@@ -1,3 +1,4 @@
+import { tick } from "svelte";
 import { describe, expect, it, vi } from "vitest";
 import { render } from "vitest-browser-svelte";
 import { checkA11y } from "../../../../tests/a11y";
@@ -13,13 +14,9 @@ const rowOf = (c: Element, key: number) =>
 	c.querySelector(`[data-key="${key}"]`) as HTMLElement | null;
 const keys = (c: Element) =>
 	lis(c).map((li) => Number((li.querySelector("[data-key]") as HTMLElement).dataset.key));
-async function waitFor(fn: () => boolean, ms = 3000) {
-	const end = Date.now() + ms;
-	while (!fn()) {
-		if (Date.now() > end) throw new Error("waitFor timed out");
-		await new Promise((r) => setTimeout(r, 20));
-	}
-}
+// Awaits the condition with Vitest's poll (its timeout is the test's, not a fixed 3s that a starved
+// machine outruns), never a retry of the assertion that follows.
+const waitFor = (fn: () => boolean) => expect.poll(fn, { timeout: 30_000 }).toBe(true);
 const top = (el: HTMLElement) => el.getBoundingClientRect().top;
 
 describe("VirtualList", () => {
@@ -110,6 +107,33 @@ describe("VirtualList", () => {
 		}
 	});
 
+	it("a scroll while the anchor restore is queued wins over the late restore (#544)", async () => {
+		const screen = await render(Harness, { props: { initial: 300 } });
+		await settle();
+		const el = scroller(screen.container);
+		el.scrollTop = 4000;
+		await settle();
+
+		// A starved machine runs the restore frame after the user's next scroll. Park the frames the
+		// prepend queues, scroll like the user, then release them in order.
+		const real = window.requestAnimationFrame;
+		const parked: FrameRequestCallback[] = [];
+		window.requestAnimationFrame = (cb) => parked.push(cb);
+		try {
+			screen.component.prepend(20);
+			await tick();
+			el.scrollTop = 0;
+			await tick();
+		} finally {
+			window.requestAnimationFrame = real;
+		}
+		expect(parked.length).toBeGreaterThan(0);
+		for (const cb of parked) cb(performance.now());
+		await settle();
+		// The bug dragged the list back to the anchor ~1000px down; row re-measuring moves it a few px.
+		expect(el.scrollTop).toBeLessThan(100);
+	});
+
 	it("calls onloadmore once near the top, not again until items change", async () => {
 		const onloadmore = vi.fn();
 		const screen = await render(Harness, { props: { initial: 200, hasMore: true, onloadmore } });