diff --git a/.planning/NOTES.md b/.planning/NOTES.md index 06379cd..1cf0812 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -3891,3 +3891,40 @@ scan takes a minute and is worth running before demoting anybody's links — `explore-album-details`'s tracklist (number / title / artist / duration) is the one that plausibly *is* mostly link, and is the one #5 is about to add selection to. + +## A layout is still moving when a guard says it has arrived (measured 2026-08-20) + +`album-dropdown.spec.ts` failed with `Expected 80, Received 10` twice +over two sessions, and #133 already strengthened its guard from +"scrollable at all" to "has at least the range the assertion needs". +That was necessary and could not be sufficient, and the reason is +structural rather than a matter of thresholds: **a guard and the write +it guards are separate CDP round trips**, so the page is free to +re-lay-out between them. Polling harder cannot close a window between +two moments; only removing the window can. + +Measured directly, sampling `scrollHeight - clientHeight` on +`.grid-scroll-container` every frame across a 1440x900 → 900x600 resize, +three runs: + +| t (ms) | range | +|---|---| +| 0 | 0 | +| 1 | **88** | +| 8–14 | 330 (settled) | + +88 satisfies a guard asking for 80 and is not the settled value, so the +guard can pass while the grid is one layout pass from done. Under +full-suite load the transient is worse — the observed failure had 10 — +which is why it shows up on the second run of a suite and not in ten +consecutive runs of the file alone (0/10 both before and after the fix). + +The shape to write instead: **one page-side call that performs the +action and returns what it observes**, with `expect.poll` retrying +*that*. `scrollTo()` sets `scrollTop` and returns `scrollTop`, so the +assertion is about what the grid did rather than about what it was +ready to do. `layout-overflow.spec.ts`'s sidebar probe already had the +fused half and was missing the retry; it has both now. + +Worth generalising: a spec that resizes and then measures is asserting +about a moving target for the next dozen frames. Fuse, then poll. diff --git a/e2e/specs/album-dropdown.spec.ts b/e2e/specs/album-dropdown.spec.ts index c629b62..9ade4a1 100644 --- a/e2e/specs/album-dropdown.spec.ts +++ b/e2e/specs/album-dropdown.spec.ts @@ -110,27 +110,30 @@ test.describe('the album dropdown', () => { await app.setViewportSize({ width: 900, height: 600 }); try { - // Wait for the range the assertion below actually needs, not for - // "scrollable at all" (#133). The guard used to be - // `scrollHeight > clientHeight + 40` while the next line asks to - // reach 80, so any range in 41-79 satisfied it and could not - // satisfy the assertion — and the grid passes through exactly - // that while it settles, because it recomputes its columns after - // the resize rather than during it. The settled range here is - // 330, so this waits rather than weakening anything. + // The container has to be a scroller at all, which is the thing + // the defect behind this spec broke and is a property rather + // than a moment. await expect .poll(() => scrollRange(app)) - .toMatchObject({ room: true, overflowY: 'auto' }); + .toMatchObject({ overflowY: 'auto' }); - await app.evaluate((target) => { - const sc = document - .querySelector('cover-grid') - ?.shadowRoot?.querySelector('.grid-scroll-container'); - - if (sc) sc.scrollTop = target; - }, SCROLL_TARGET); - - expect(await scrollTop(app)).toBe(SCROLL_TARGET); + // **Scrolling it and reading it back are one round trip** (#151). + // + // #133 made the guard ask for the range this needs rather than + // for "scrollable at all", which was necessary and is not + // sufficient: a guard and the write it guards are separate + // `evaluate` calls, so the grid can satisfy the range and settle + // out of it before the write lands. It still does — observed as + // `Expected 80, Received 10` in the second of three consecutive + // full-suite runs, with the spec green alone on the same app + // straight afterwards. + // + // Polling harder cannot close a window between two moments; only + // removing the window can. So the probe sets `scrollTop` and + // returns what it reads back, in one page-side call, and the + // poll retries *that* — which also means the assertion is about + // what the grid did rather than about what it was ready to do. + await expect.poll(() => scrollTo(app, SCROLL_TARGET)).toBe(SCROLL_TARGET); // And the dropdown it opens is on screen, wherever the manager // decides that leaves the scroll. It is *not* "the position is @@ -261,7 +264,7 @@ async function closeDropdown(app: Page): Promise { }); } -/** Whether the grid can scroll at all, which decides if a probe can move. */ +/** Whether the grid is a scroller at all, which is what the bug broke. */ async function scrollRange(app: Page) { return app.evaluate((target) => { const sc = document @@ -269,22 +272,37 @@ async function scrollRange(app: Page) { ?.shadowRoot?.querySelector('.grid-scroll-container'); return { - // `room` is the precondition of the assertion that follows it: - // enough range to actually reach the target. A threshold below - // what the caller depends on is not a guard. + // Reported for the failure message rather than waited on: `room` + // was the guard #133 strengthened, and #151 is that a guard in + // its own round trip cannot speak for the write in the next one. + // `scrollTo` below is the assertion now; this says *why* it did + // not reach the target when it does not. room: !!sc && sc.scrollHeight - sc.clientHeight >= target, overflowY: sc ? getComputedStyle(sc).overflowY : '', }; }, SCROLL_TARGET); } -async function scrollTop(app: Page): Promise { - return app.evaluate( - () => - document - .querySelector('cover-grid') - ?.shadowRoot?.querySelector('.grid-scroll-container')?.scrollTop ?? -1, - ); +/** + * Scroll the grid and report where it actually landed, in one call. + * + * The whole point is that the set and the read share a moment: a + * `scrollTop` write is clamped to the range *at the instant it lands*, + * so reading it back in a second round trip asks a container that may + * have re-laid out in between. + */ +async function scrollTo(app: Page, target: number): Promise { + return app.evaluate((to) => { + const sc = document + .querySelector('cover-grid') + ?.shadowRoot?.querySelector('.grid-scroll-container'); + + if (!sc) return -1; + + sc.scrollTop = to; + + return sc.scrollTop; + }, target); } /** Whether the open dropdown is inside the scroll container's viewport. */ diff --git a/e2e/specs/layout-overflow.spec.ts b/e2e/specs/layout-overflow.spec.ts index eaa1953..9815b49 100644 --- a/e2e/specs/layout-overflow.spec.ts +++ b/e2e/specs/layout-overflow.spec.ts @@ -132,22 +132,34 @@ test.describe('the app fits in its own window', () => { // be dragged here, but a scaled display or a large system font can // still land the layout in it, and clipping the nav with no scroll // is the failure that made Settings unreachable. - const reachable = await app.locator('app-sidebar').evaluate((el) => { - const settings = el.shadowRoot?.querySelector( - '[data-testid="nav-settings"]', - ); + // + // The scroll and the measurement share one `evaluate` — #151's + // rule, which this already had — and the whole probe is polled, + // which it did not: a viewport change settles asynchronously, so a + // single attempt reads whatever the sidebar happened to be doing. + // The probe is safe to repeat because scrolling to the bottom twice + // is scrolling to the bottom. + await expect + .poll(() => + app.locator('app-sidebar').evaluate((el) => { + const settings = el.shadowRoot?.querySelector( + '[data-testid="nav-settings"]', + ); - if (!settings) return null; + if (!settings) return null; - el.scrollTop = el.scrollHeight; + el.scrollTop = el.scrollHeight; - const item = settings.getBoundingClientRect(); - const pane = el.getBoundingClientRect(); + const item = settings.getBoundingClientRect(); + const pane = el.getBoundingClientRect(); - return item.bottom <= Math.ceil(pane.bottom) && item.top >= Math.floor(pane.top); - }); - - expect(reachable).toBe(true); + return ( + item.bottom <= Math.ceil(pane.bottom) && + item.top >= Math.floor(pane.top) + ); + }), + ) + .toBe(true); }); });