From ec58ea1f5162a0ba4be3d645153053121600b8fc Mon Sep 17 00:00:00 2001 From: xarmian Date: Sun, 27 Sep 2026 09:57:51 +0000 Subject: [PATCH] fix(web): pane Back holds the restored scroll position until the pane settles, so a late-loading section no longer leaves the reader short (BUG-3250) #1604 (BUG-3240) made the body render as soon as the replay is applied, so PaneHost's restore now lands about 100ms after onReady, before other parts of the pane settle: the attachment strip's loading placeholder disappears and the timeline's loader becomes its content. Scroll anchoring and clamping then moved scrollTop, and a reader returning near the end of a long item landed 70-90px short. bug-2182's Back leg at 8 workers: 26/100 failed on main, 0/100 on the parent. After the first apply the restore now holds its target until RESTORE_CAP_MS: every resize re-applies it. The hold ends at the cap or at the first reader move: a wheel, touch or key gesture, or a scroll away from our last assignment while scrollHeight held still (a scrollbar drag). A clamp or an anchoring shift changes the height; our own assignment lands on the value we set. Legs pin each rule with a deterministic 200px shift: re-applied during the hold; a drag honoured; a gesture that scrolls nothing ends it; nothing after the cap. Main's PaneHost fails only the first; mutants of each rule fail only their own. Gate: the Back leg 300/300 at 8 workers. RESTORE_CAP_MS stays 2500 (TASK-3248's receipt): with a ~300-frame op-log the first apply reaches 1693ms and 9/100 never applied by 2500 (the parent: 6/100), so lowering it would cut into a tail that predates BUG-3240. Claude-Session: https://claude.ai/code/session_01NidgPePY7UvHcEJCMkD2Qz --- web/e2e/bug-2182-pane-back-scroll.spec.ts | 50 +++++++++++++++++ .../components/collections/PaneHost.svelte | 56 ++++++++++++++----- 2 files changed, 93 insertions(+), 13 deletions(-) diff --git a/web/e2e/bug-2182-pane-back-scroll.spec.ts b/web/e2e/bug-2182-pane-back-scroll.spec.ts index 0301e3bfd..f9271a3e7 100644 --- a/web/e2e/bug-2182-pane-back-scroll.spec.ts +++ b/web/e2e/bug-2182-pane-back-scroll.spec.ts @@ -103,3 +103,53 @@ test('BUG-2182: a reader gesture during the restore wait wins', async ({ page, f await deleteCollection(fixture, request, ctx.coll.slug); } }); + +// BUG-3250: once applied, the restore HOLDS its target until the cap, because +// parts of the pane settle after the body and move scrollTop (measured: the +// attachment strip's placeholder and the timeline's loader, 70-90px). A 200px +// spacer prepended inside the pane's content stands in for that late shift, +// deterministically. +async function backAndRestore(page: import('@playwright/test').Page, ctx: Awaited>) { + await page.locator('.pane-back-btn').click(); + await expect(ctx.pane.locator('a', { hasText: ctx.titleB }).last()).toBeAttached({ timeout: 10_000 }); + await expect.poll(ctx.scrollTop, { timeout: 3000, message: 'premise: the restore landed' }).toBeGreaterThan(ctx.before - 40); + return ctx.scrollTop(); +} +const shiftContent = (pane: import('@playwright/test').Locator) => + pane.evaluate((el) => { + const s = document.createElement('div'); + s.style.height = '200px'; + s.dataset.bug3250 = 'spacer'; + el.querySelector('.item-page')!.prepend(s); + }); + +for (const leg of [ + { name: 'a layout shift during the hold is re-applied', before: async () => {}, reapplied: true }, + { name: 'a scrollbar-style drag during the hold is honoured', before: async (ctx: any) => { await ctx.pane.evaluate((el: HTMLElement) => { el.scrollTop -= 600; }); }, reapplied: false }, + // A gesture that scrolls nothing: only the gesture listener can end the hold + // here, since a scrolling wheel is also caught by the drag rule above. + { name: 'a wheel gesture during the hold ends it, even one that scrolls nothing', before: async (ctx: any, page: any) => { const b = (await ctx.pane.boundingBox())!; await page.mouse.move(b.x + b.width / 2, b.y + b.height / 2); await page.mouse.wheel(0, 0); }, reapplied: false }, + { name: 'the hold ends at the cap, leaving no listener behind', before: async (_ctx: any, page: any) => { await page.waitForTimeout(3000); }, reapplied: false }, +]) { + test(`BUG-3250: ${leg.name}`, async ({ page, fixture, request }, testInfo) => { + test.skip(testInfo.project.name !== 'desktop-chromium', 'the split pane is a desktop layout'); + test.setTimeout(60_000); + const ctx = await setup(page, fixture, request); + try { + const restored = await backAndRestore(page, ctx); + await leg.before(ctx, page); + await page.waitForTimeout(150); + const beforeShift = await ctx.scrollTop(); + await shiftContent(ctx.pane); + await page.waitForTimeout(400); + const after = await ctx.scrollTop(); + if (leg.reapplied) { + expect(Math.abs(after - restored), `the shift was not re-applied (restored ${restored}, now ${after})`).toBeLessThanOrEqual(3); + } else { + expect(Math.abs(after - restored), `the restore overrode the reader or outlived its window (restored ${restored}, before the shift ${beforeShift}, now ${after})`).toBeGreaterThan(50); + } + } finally { + await deleteCollection(fixture, request, ctx.coll.slug); + } + }); +} diff --git a/web/src/lib/components/collections/PaneHost.svelte b/web/src/lib/components/collections/PaneHost.svelte index 183a991cc..e34f45d99 100644 --- a/web/src/lib/components/collections/PaneHost.svelte +++ b/web/src/lib/components/collections/PaneHost.svelte @@ -128,9 +128,18 @@ // applied once the pane can scroll that far — waited for with a // ResizeObserver on the pane's content, capped. The reader wins: any // scroll during the wait that is not our own, or a wheel/touch/key - // gesture, cancels it. Our own assignment happens only at the moment it - // succeeds, so the scroll event it raises is never mistaken for the - // reader's. + // gesture, cancels it. + // + // Once applied, the target is HELD until the cap (BUG-3250). Other parts + // of the pane settle after the body (the attachment strip's loading + // placeholder goes away, the timeline's loader becomes its content), the + // browser's scroll anchoring or clamping then moves scrollTop, and a + // one-shot restore left the reader 70-90px short of where they were. So + // every resize re-applies it. During the hold a scroll is the reader's only + // when it moves away from our last assignment while scrollHeight held + // still: a clamp or an anchoring shift comes with a height change, and our + // own assignment lands on the value we set. That rule is what honours a + // scrollbar drag, which raises no gesture event. // // THE CAP MUST CLEAR THE COLLAB SYNC GRACE (BUG-3228). The body renders // only once the collab provider reports `synced`. On a connection that gets @@ -145,15 +154,23 @@ // is a restore that lands late for a reader who has not moved, and any move // cancels it. If the grace comes down, this can come down with it. // - // The grace is gone (BUG-3240): the body now waits for the replay itself, - // which measured p50 8-241 ms (to 303 replay frames) and up to 740 ms for - // about 2,000 frames under 8 workers. Lowering this cap owes its own - // measurement of the pane Back spec on that build; it is unchanged here. + // The grace is gone (BUG-3240), and since BUG-3250 the cap is also the + // window in which the restore HOLDS its target. Re-measured for TASK-3248 + // on that build, the pane Back spec at 8 workers, ms from onReady: with a + // small op-log (x300), first apply p50 75 / p99 281 / max 435, and the + // last re-apply after a late layout shift max 320. With a ~300-frame + // op-log on the item (x100), first apply p99 and max 1693, and 9/100 runs + // had not applied by 2500. That tail predates BUG-3240: the parent build + // failed the same variant 6/100 (3 of them at the cap). So the cap stays at + // 2500: lowering it would cut into that tail, not into slack. const RESTORE_CAP_MS = 2500; let cancelRestore: (() => void) | null = null; function restorePaneScroll(el: HTMLElement, target: number): () => void { let done = false; + let applied = false; + let lastSet = -1; + let lastHeight = el.scrollHeight; const finish = () => { if (done) return; done = true; @@ -164,14 +181,27 @@ }; const apply = () => { if (done) return; - if (el.scrollHeight - el.clientHeight < target) return; - el.scrollTop = target; - finish(); + lastHeight = el.scrollHeight; + const max = el.scrollHeight - el.clientHeight; + // Before the first apply, wait until the pane can reach the target. + if (!applied && max < target) return; + applied = true; + const want = Math.min(target, max); + if (el.scrollTop !== want) el.scrollTop = want; + lastSet = el.scrollTop; }; - // Before the restore lands, the pane sits at 0 while content grows, which - // raises no scroll event; a scroll now can only be the reader's. const onScroll = () => { - if (!done && el.scrollTop > 2) finish(); + if (done) return; + // Before the restore lands, the pane sits at 0 while content grows, + // which raises no scroll event; a scroll now can only be the reader's. + if (!applied) { + if (el.scrollTop > 2) finish(); + return; + } + const height = el.scrollHeight; + const readerMoved = Math.abs(el.scrollTop - lastSet) > 2 && height === lastHeight; + lastHeight = height; + if (readerMoved) finish(); }; const ro = new ResizeObserver(apply); for (const child of Array.from(el.children)) ro.observe(child);