From 6c2dbc59ca336e1fb320516f5827ff3f78c7a2d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matheus=20Vila=C3=A7a=20de=20Jesus?= Date: Sat, 24 Oct 2026 14:58:33 -0300 Subject: [PATCH 1/2] chore: start #544 From 8ef27ecb15ac375e34d693c1cbd76b167ffc9e24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matheus=20Vila=C3=A7a=20de=20Jesus?= Date: Sat, 24 Oct 2026 15:51:00 -0300 Subject: [PATCH 2/2] fix(virtuallist): a user scroll beats a queued anchor restore (#544) Under load the restore frame ran after the user scrolled to the top and dragged the list back ~1000px to the pre-prepend anchor, so the next page was never requested. The restore now only applies if the scroll is still where the anchor was taken. ModalContract's 24-keystroke Tab test takes SLOW_FLOW; VirtualList's hand-rolled 3s waitFor is expect.poll. --- .../virtuallist-scroll-beats-restore.md | 5 ++ AGENTS.md | 2 +- .../Overlays/ModalContract.svelte.test.ts | 52 ++++++++++--------- .../components/VirtualList/VirtualList.svelte | 9 ++-- .../VirtualList/VirtualList.svelte.test.ts | 38 +++++++++++--- 5 files changed, 71 insertions(+), 35 deletions(-) create mode 100644 .changeset/virtuallist-scroll-beats-restore.md diff --git a/.changeset/virtuallist-scroll-beats-restore.md b/.changeset/virtuallist-scroll-beats-restore.md new file mode 100644 index 00000000..3bac814b --- /dev/null +++ b/.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. diff --git a/AGENTS.md b/AGENTS.md index 2e238764..d67bfe80 100644 --- a/AGENTS.md +++ b/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`. diff --git a/packages/ui/src/lib/components/Overlays/ModalContract.svelte.test.ts b/packages/ui/src/lib/components/Overlays/ModalContract.svelte.test.ts index 2d272488..30ae72e2 100644 --- a/packages/ui/src/lib/components/Overlays/ModalContract.svelte.test.ts +++ b/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(); - 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(); + 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(); diff --git a/packages/ui/src/lib/components/VirtualList/VirtualList.svelte b/packages/ui/src/lib/components/VirtualList/VirtualList.svelte index 37e8fb11..624f5108 100644 --- a/packages/ui/src/lib/components/VirtualList/VirtualList.svelte +++ b/packages/ui/src/lib/components/VirtualList/VirtualList.svelte @@ -179,7 +179,7 @@ // 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() { @@ -187,7 +187,7 @@ // 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; } diff --git a/packages/ui/src/lib/components/VirtualList/VirtualList.svelte.test.ts b/packages/ui/src/lib/components/VirtualList/VirtualList.svelte.test.ts index 470c33db..58a482ea 100644 --- a/packages/ui/src/lib/components/VirtualList/VirtualList.svelte.test.ts +++ b/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 } });