From db37dc2bcd4591ab88a569393d8d764291ebcfb1 Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 23 Sep 2026 17:15:46 -0400 Subject: [PATCH] fix(web): the split pane's Back returns to the previous item at the position it was read at (BUG-2182) (#1473) * test(BUG-2182): reproduce pane Back returning to the top of the previous item * fix(BUG-2182): the split pane's Back returns to the previous item at the position it was read at * test(BUG-2182): the drill-after-Back race with a scrolled A, the case the saved position turns into a shallow entry * test(BUG-2182): drop the scrolled race variant; the race it copies is flaky on main (filed separately) --- web/e2e/bug-2182-pane-back-scroll.spec.ts | 97 +++++++++++++++++ web/src/app.d.ts | 2 + web/src/lib/collections/paneController.ts | 13 +++ web/src/lib/collections/paneHostController.ts | 20 +++- .../components/collections/PaneHost.svelte | 103 ++++++++++++++++++ .../[workspace]/[collection]/+page.svelte | 1 + .../[collection]/[slug]/+page.svelte | 1 + 7 files changed, 236 insertions(+), 1 deletion(-) create mode 100644 web/e2e/bug-2182-pane-back-scroll.spec.ts diff --git a/web/e2e/bug-2182-pane-back-scroll.spec.ts b/web/e2e/bug-2182-pane-back-scroll.spec.ts new file mode 100644 index 000000000..5aada3874 --- /dev/null +++ b/web/e2e/bug-2182-pane-back-scroll.spec.ts @@ -0,0 +1,97 @@ +import { test, expect } from './fixtures'; +import { browserLogin } from './lib/collab-helpers'; +import { deleteCollection } from './lib/attachment-viewer'; + +/** + * BUG-2182 — the split pane's Back button (shown after following a link from + * item A to item B inside the pane) returned to A at the TOP, not at the scroll + * position A was read at. + */ +async function setup(page: import('@playwright/test').Page, fixture: import('./fixtures').SuiteFixture, request: import('@playwright/test').APIRequestContext) { + await page.setViewportSize({ width: 1400, height: 800 }); + await browserLogin(page); + const headers = { Authorization: `Bearer ${fixture.apiToken}`, 'Content-Type': 'application/json' }; + const stamp = Date.now(); + const res = await request.post(`/api/v1/workspaces/${fixture.workspaceSlug}/collections`, { + headers, + data: { name: `B2182 ${stamp}`, prefix: `BH${String(stamp).slice(-4)}` }, + }); + expect(res.ok(), await res.text()).toBeTruthy(); + const coll = await res.json(); + const titleB = `B2182 target ${stamp}`; + const b = await request.post(`/api/v1/workspaces/${fixture.workspaceSlug}/collections/${coll.slug}/items`, { + headers, data: { title: titleB, content: Array.from({ length: 40 }, (_, i) => `Target paragraph ${i}.`).join('\n\n') }, + }); + expect(b.ok(), await b.text()).toBeTruthy(); + const paras = Array.from({ length: 60 }, (_, i) => `Paragraph ${i} of a long item, long enough to scroll the pane.`).join('\n\n'); + const a = await request.post(`/api/v1/workspaces/${fixture.workspaceSlug}/collections/${coll.slug}/items`, { + headers, data: { title: `B2182 source ${stamp}`, content: `${paras}\n\nSee [[${titleB}]].` }, + }); + expect(a.ok(), await a.text()).toBeTruthy(); + const itemA = await a.json(); + + await page.goto(`/${fixture.adminUsername}/${fixture.workspaceSlug}/${coll.slug}?item=${itemA.slug}`); + // Scoped to the pane: an unscoped match finds B's CARD on the board behind it. + const pane = page.locator('.item-pane'); + const link = pane.locator('a', { hasText: titleB }).last(); + await expect(link).toBeAttached({ timeout: 15_000 }); + await link.scrollIntoViewIfNeeded(); + await page.waitForTimeout(300); + const scrollTop = () => pane.evaluate((el) => el.scrollTop); + const before = await scrollTop(); + expect(before, 'precondition: the pane scrolled').toBeGreaterThan(300); + + // A click on an editor link opens its popover; the popover's href drills. + await link.click(); + await page.locator('.link-href').first().click(); + await expect(page.locator('.pane-back-btn')).toBeVisible({ timeout: 10_000 }); + await expect(pane.getByText('Target paragraph 0.')).toBeVisible(); + await page.waitForTimeout(300); + return { coll, itemA, titleB, pane, before, scrollTop }; +} + +test('BUG-2182: pane Back returns to the previous item at its scroll position; the new item starts at the top', 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 { + // A cold entry (B, just drilled to, saved nothing) opens at the top. + expect(await ctx.scrollTop(), 'the drilled-to item did not open at the top').toBeLessThan(40); + + 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: 'pane Back did not restore the scroll position' }) + .toBeGreaterThan(ctx.before - 40); + expect(await ctx.scrollTop()).toBeLessThan(ctx.before + 40); + } finally { + await deleteCollection(fixture, request, ctx.coll.slug); + } +}); + +test('BUG-2182: a reader gesture during the restore wait wins', 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 { + // Hold A's item read on the way back, so the restore is still WAITING + // for content when the reader acts. + let release!: () => void; + const gate = new Promise((r) => (release = r)); + await page.route(`**/items/${ctx.itemA.slug}**`, async (route) => { + await gate; + await route.continue(); + }); + await page.locator('.pane-back-btn').click(); + await page.waitForTimeout(200); + const box = (await ctx.pane.boundingBox())!; + await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); + await page.mouse.wheel(0, 100); // the reader's gesture, during the wait + release(); + await expect(ctx.pane.locator('a', { hasText: ctx.titleB }).last()).toBeAttached({ timeout: 10_000 }); + await page.waitForTimeout(1500); // past the restore cap + expect(await ctx.scrollTop(), 'the restore overrode the reader').toBeLessThan(ctx.before / 2); + } finally { + await page.unrouteAll({ behavior: 'ignoreErrors' }); + await deleteCollection(fixture, request, ctx.coll.slug); + } +}); diff --git a/web/src/app.d.ts b/web/src/app.d.ts index 768ed731f..dfcb4bd7b 100644 --- a/web/src/app.d.ts +++ b/web/src/app.d.ts @@ -13,6 +13,8 @@ declare global { interface PageState { paneDepth?: number; paneOwned?: boolean; + /** Pane scroll offset saved when drilling forward from this entry (BUG-2182). */ + paneScrollTop?: number; } // interface Platform {} } diff --git a/web/src/lib/collections/paneController.ts b/web/src/lib/collections/paneController.ts index e5c566a17..ec118ffb0 100644 --- a/web/src/lib/collections/paneController.ts +++ b/web/src/lib/collections/paneController.ts @@ -37,6 +37,19 @@ export interface PaneHistoryState { * shared `?item=` URL. Drills copy (inherit) this from the current entry. */ paneOwned?: boolean; + /** + * The pane's scroll position when the user drilled FORWARD from this entry + * (BUG-2182). Written into the entry being left, never into the new one, so + * a Back/Forward traversal can put the reader where they were; an entry + * without it (a cold load, a first open) stays at the top. + */ + paneScrollTop?: number; +} + +/** The saved pane scroll position of an entry, or null when it has none. */ +export function readPaneScrollTop(state: PaneHistoryState | null | undefined): number | null { + const v = state?.paneScrollTop; + return typeof v === 'number' && Number.isFinite(v) && v > 0 ? Math.floor(v) : null; } /** The normalized, always-present form after {@link readPaneState}. */ diff --git a/web/src/lib/collections/paneHostController.ts b/web/src/lib/collections/paneHostController.ts index d5d26879e..910386300 100644 --- a/web/src/lib/collections/paneHostController.ts +++ b/web/src/lib/collections/paneHostController.ts @@ -22,7 +22,7 @@ // pane-follow, the list-row focus-return target, the unsaved-draft leave guard) // stays in the host and is handed in as a small set of injected callbacks. -import { goto, afterNavigate } from '$app/navigation'; +import { goto, afterNavigate, replaceState } from '$app/navigation'; import { page } from '$app/state'; import { browser } from '$app/environment'; import { itemUrlId, type Item, type PaneTarget } from '$lib/types'; @@ -32,6 +32,7 @@ import { planLateralOpen, planPaneClose, type ResolvedPaneState, + type PaneHistoryState, } from '$lib/collections/paneController'; import { resolvePaneTarget } from '$lib/collections/paneTarget'; @@ -57,6 +58,12 @@ export interface PaneControllerDeps { * `{#key itemSlug}` remount destroys the just-activated control (R1). */ focusPaneRegion: () => void; + /** + * The pane's current scroll offset, or null with no pane (BUG-2182). Read + * just before a forward drill and saved into the entry being left, so Back + * can restore it. Optional: a host without a pane has nothing to report. + */ + getPaneScrollTop?: () => number | null; /** * Capture the element that opened the pane, so an eventual close can return * focus to it (list-row fallback; collection page only). Called ONLY on a @@ -295,6 +302,17 @@ export function createPaneController(deps: PaneControllerDeps): PaneController { deps.cancelFollow(); const plan = planPaneDrill(deps.getOpenItemRef(), target, currentPaneState()); if (plan.kind === 'noop') return; + // BUG-2182: remember where the reader was in THIS item before leaving it, + // merged into the current entry's state through SvelteKit's own shallow + // routing (never a raw history.replaceState, which would clobber Kit's + // sveltekit:* keys). Only for a push: a capped drill REPLACES the entry, + // and the entry it would be saved on is the one being overwritten. + if (plan.kind === 'push') { + const top = deps.getPaneScrollTop?.() ?? null; + if (top !== null && top > 0) { + replaceState('', { ...(page.state as PaneHistoryState), paneScrollTop: top }); + } + } const url = new URL(page.url); url.searchParams.set('item', target); goto(`${url.pathname}${url.search}`, { diff --git a/web/src/lib/components/collections/PaneHost.svelte b/web/src/lib/components/collections/PaneHost.svelte index 9a79e7091..e7649b93a 100644 --- a/web/src/lib/components/collections/PaneHost.svelte +++ b/web/src/lib/components/collections/PaneHost.svelte @@ -24,6 +24,8 @@ // openItemRef}` gate, which mounts/unmounts THIS whole component. import { browser } from '$app/environment'; import { onDestroy, untrack } from 'svelte'; + import { page } from '$app/state'; + import { readPaneScrollTop } from '$lib/collections/paneController'; import ItemDetail from '$lib/components/items/ItemDetail.svelte'; import { viewport } from '$lib/stores/breakpoint.svelte'; import { paneOverlay } from '$lib/stores/paneOverlay.svelte'; @@ -111,6 +113,103 @@ // Exposed as a Svelte 5 instance export (PLAN-2154 Architecture E / TASK-2170) // so the host can wire it as the controller's `focusPaneRegion` dep AND its // list→pane Tab bridge, both via `paneHostEl?.focusPaneRegion()`. + /** The pane's scroll offset, for the controller's forward-drill save (BUG-2182). */ + export function getPaneScrollTop(): number | null { + return paneEl ? paneEl.scrollTop : null; + } + + // BUG-2182: a Back/Forward traversal onto an entry that saved a scroll + // position (see PaneHistoryState.paneScrollTop) puts the reader back there. + // Only on a POPSTATE (a traversal): page.state also changes on unrelated replaceState + // writes to the same entry, and restoring on those would yank the reader + // back to a position they have since scrolled away from. + // + // The item's content renders after the navigation, so the position is + // applied once the pane can scroll that far — waited for with a + // ResizeObserver on the pane's content, capped at ~1s (lead ruling). 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. + const RESTORE_CAP_MS = 1000; + let cancelRestore: (() => void) | null = null; + + function restorePaneScroll(el: HTMLElement, target: number): () => void { + let done = false; + const finish = () => { + if (done) return; + done = true; + ro.disconnect(); + clearTimeout(timer); + el.removeEventListener('scroll', onScroll); + for (const ev of GESTURES) el.removeEventListener(ev, finish); + }; + const apply = () => { + if (done) return; + if (el.scrollHeight - el.clientHeight < target) return; + el.scrollTop = target; + finish(); + }; + // 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(); + }; + const ro = new ResizeObserver(apply); + for (const child of Array.from(el.children)) ro.observe(child); + const timer = setTimeout(finish, RESTORE_CAP_MS); + el.addEventListener('scroll', onScroll, { passive: true }); + for (const ev of GESTURES) el.addEventListener(ev, finish, { passive: true }); + apply(); + return finish; + } + + // The window's popstate, not afterNavigate: saving the position makes the + // entry being left a SvelteKit SHALLOW entry (replaceState), and traversing + // back to a shallow entry updates page.state without running a navigation, + // so afterNavigate never fires for it (measured). SvelteKit applies the + // entry's state in its own popstate handler; the read waits a task for it. + // + // The restore then waits for ItemDetail's onReady(true) — the LOADED item + // matching the requested ref — before it starts. Starting at the popstate + // measured against the PREVIOUS item still on screen (it read B's height, + // then the swap collapsed it) and let the 1s cap run out on the network + // fetch. onReady is the same switch boundary the full-page scroll restore + // uses for this A→B→A case (BUG-1425). + let readyGen = 0; + let pendingRestore: { target: number; gen: number } | null = null; + const GESTURES = ['wheel', 'touchstart', 'keydown'] as const; + + // A reader gesture while the item is still loading cancels the pending + // restore too, not only one during the height wait: the reader has already + // started moving, and jumping them afterwards is the same override. + function dropPending() { + pendingRestore = null; + for (const ev of GESTURES) paneEl?.removeEventListener(ev, dropPending); + } + + function onPopState() { + cancelRestore?.(); + cancelRestore = null; + dropPending(); + setTimeout(() => { + const target = readPaneScrollTop(page.state); + if (target === null || !paneEl) return; + pendingRestore = { target, gen: readyGen }; + for (const ev of GESTURES) paneEl.addEventListener(ev, dropPending, { passive: true }); + }, 0); + } + + function handleItemReady(ready: boolean) { + if (!ready) return; + readyGen++; + const p = pendingRestore; + if (!p || readyGen <= p.gen || !paneEl) return; + dropPending(); + cancelRestore = restorePaneScroll(paneEl, p.target); + } + if (browser) window.addEventListener('popstate', onPopState); + export function focusPaneRegion() { if (!browser) return; if (!openItemRef || !paneEl) return; @@ -481,6 +580,9 @@ // this from an `openItemRef`→null effect — the shell's unmount IS that // transition now). onDestroy(() => { + cancelRestore?.(); + dropPending(); + if (browser) window.removeEventListener('popstate', onPopState); if (resizingPane && browser) { document.body.style.userSelect = ''; document.body.style.cursor = ''; @@ -656,6 +758,7 @@ openItemRef, cancelFollow: () => cancelPaneFollow(), focusPaneRegion: () => paneHostEl?.focusPaneRegion(), + getPaneScrollTop: () => paneHostEl?.getPaneScrollTop() ?? null, captureReturnFocus, setBypassNavGuard: (bypass) => { bypassNavGuard = bypass; diff --git a/web/src/routes/[username]/[workspace]/[collection]/[slug]/+page.svelte b/web/src/routes/[username]/[workspace]/[collection]/[slug]/+page.svelte index bb17be3c3..2df9498a5 100644 --- a/web/src/routes/[username]/[workspace]/[collection]/[slug]/+page.svelte +++ b/web/src/routes/[username]/[workspace]/[collection]/[slug]/+page.svelte @@ -168,6 +168,7 @@ }, captureReturnFocus: () => {}, setBypassNavGuard: () => {}, + getPaneScrollTop: () => paneHostEl?.getPaneScrollTop() ?? null, }); const { openItemPaneByRef,