From 8fee2de145d748d8d48bc0fd4a57297ff57c0564 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Tue, 12 May 2026 18:54:21 +0100 Subject: [PATCH] fix(web): preserve sidebar scroll position across task switches (#884) --- .../components/task/task-session-sidebar.tsx | 2 +- .../task/sidebar-scroll-preservation.spec.ts | 83 +++++++++++++ apps/web/lib/layout/panel-portal-host.tsx | 114 +++++++++++++++++- 3 files changed, 197 insertions(+), 2 deletions(-) create mode 100644 apps/web/e2e/tests/task/sidebar-scroll-preservation.spec.ts diff --git a/apps/web/components/task/task-session-sidebar.tsx b/apps/web/components/task/task-session-sidebar.tsx index 6d5864e2c1..457985e230 100644 --- a/apps/web/components/task/task-session-sidebar.tsx +++ b/apps/web/components/task/task-session-sidebar.tsx @@ -570,7 +570,7 @@ export const TaskSessionSidebar = memo(function TaskSessionSidebar({ return ( - + { + test("clicking another task does not reset the sidebar scroll position", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(60_000); + + // Seed enough tasks so the sidebar list overflows its viewport. Titles + // are zero-padded so sort order matches creation order. + const TASK_COUNT = 25; + const created: { id: string; title: string }[] = []; + for (let i = 0; i < TASK_COUNT; i++) { + const title = `Scroll Task ${String(i).padStart(2, "0")}`; + const task = await apiClient.createTask(seedData.workspaceId, title, { + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }); + created.push({ id: task.id, title }); + } + + // Navigate to the most recently created task (renders at the top of the + // sidebar since tasks sort by createdAt desc within a state bucket). + const navTask = created[created.length - 1]; + await testPage.goto(`/t/${navTask.id}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + await expect(session.sidebar).toBeVisible({ timeout: 10_000 }); + + // Wait for the full task list to render in the sidebar. + await expect(session.sidebar.getByTestId("sidebar-task-item")).toHaveCount(TASK_COUNT, { + timeout: 10_000, + }); + + const scrollContainer = testPage.getByTestId("task-sidebar-scroll"); + await expect(scrollContainer).toBeVisible(); + + // Sanity: list overflows the scroll container. + const dimensions = await scrollContainer.evaluate((el) => ({ + clientHeight: el.clientHeight, + scrollHeight: el.scrollHeight, + })); + expect(dimensions.scrollHeight).toBeGreaterThan(dimensions.clientHeight); + + // Scroll to the bottom of the sidebar. + await scrollContainer.evaluate((el) => { + el.scrollTop = el.scrollHeight - el.clientHeight; + }); + const scrollBefore = await scrollContainer.evaluate((el) => el.scrollTop); + expect(scrollBefore).toBeGreaterThan(0); + + // Pick a task that is currently visible near the bottom (the oldest one). + // The first-created task appears last in the list because of desc sort. + const bottomTask = created[0]; + const bottomRow = session.sidebarTaskItem(bottomTask.title).first(); + await expect(bottomRow).toBeVisible(); + await expect(bottomRow).toBeInViewport(); + + await bottomRow.click(); + + // URL switches to the new task once selection settles. + await expect.poll(() => testPage.url(), { timeout: 10_000 }).toContain(bottomTask.id); + + // Sidebar should still be scrolled near the previous position. Without + // the fix, scrollTop would snap back to 0 after the dockview slot is + // re-created. Allow a small tolerance for any minor adjustments. + await expect + .poll(() => scrollContainer.evaluate((el) => el.scrollTop), { timeout: 5_000 }) + .toBeGreaterThan(scrollBefore - 50); + }); +}); diff --git a/apps/web/lib/layout/panel-portal-host.tsx b/apps/web/lib/layout/panel-portal-host.tsx index 41ee0ed68c..11b6c1edb6 100644 --- a/apps/web/lib/layout/panel-portal-host.tsx +++ b/apps/web/lib/layout/panel-portal-host.tsx @@ -85,10 +85,29 @@ export function usePortalSlot( const entry = panelPortalManager.acquire(panelId, component, params, props.api, envId); - // Reparent the portal element into this panel's DOM slot. + // Reparent the portal element into this panel's DOM slot, then restore any + // scroll positions captured before the previous detach. Browsers reset + // scrollTop/scrollLeft when an element is removed from the DOM, so without + // this the sidebar (and any other scrollable descendant of a portal) jumps + // back to the top after every dockview layout switch. container.appendChild(entry.element); + const cancelRestore = restorePortalScroll(entry.element); + // Track every scroll inside this portal so we always have an up-to-date + // snapshot to restore after the next reattach. We do this via a capturing + // listener because dockview can rip the DOM apart before React runs the + // cleanup below (removeChild fires too late to read scrollTop reliably). + const onScroll = (e: Event) => { + const target = e.target; + if (target instanceof Element && entry.element.contains(target)) { + const html = target as HTMLElement; + scrollSnapshots.set(target, { top: html.scrollTop, left: html.scrollLeft }); + } + }; + entry.element.addEventListener("scroll", onScroll, true); return () => { + cancelRestore(); + entry.element.removeEventListener("scroll", onScroll, true); // Detach only — do NOT release. The element stays in the manager // and will be re-adopted when the panel remounts after fromJSON(). if (entry.element.parentNode === container) { @@ -113,3 +132,96 @@ export function usePortalSlot( return containerRef; } + +// --------------------------------------------------------------------------- +// Scroll preservation across portal detach/reattach +// --------------------------------------------------------------------------- + +/** + * Per-element scroll snapshots, kept up-to-date by a capturing scroll + * listener attached to each portal element. Stored in a WeakMap so detached + * subtrees can be garbage collected if the portal is ever released. + */ +const scrollSnapshots = new WeakMap(); + +/** Window during which we keep re-applying snapshots after attach. */ +const RESTORE_WINDOW_MS = 1500; + +/** + * Restore captured scroll positions on every scrollable descendant of `portal`, + * then keep re-applying for a short window so that a snapshot which gets + * clamped by a not-yet-laid-out container (`scrollTop` capped to + * `scrollHeight - clientHeight`) is corrected once layout settles. + * + * Returns a function that cancels any pending retries (call from cleanup so + * we never write to a detached subtree). + */ +function restorePortalScroll(portal: Element): () => void { + // Collect every descendant that has a snapshot up front so we don't re-walk + // the tree on each retry. + const targets: Array<{ el: HTMLElement; snap: { top: number; left: number } }> = []; + const walker = document.createTreeWalker(portal, NodeFilter.SHOW_ELEMENT); + // Skip the portal element itself — it is never user-scrolled, and the loop + // body assumes Element nodes (guaranteed by SHOW_ELEMENT for descendants). + let node = walker.nextNode() as HTMLElement | null; + while (node) { + const snap = scrollSnapshots.get(node); + if (snap && (snap.top > 0 || snap.left > 0)) { + targets.push({ el: node, snap }); + } + node = walker.nextNode() as HTMLElement | null; + } + if (targets.length === 0) return () => {}; + + // Track which elements still need retrying. An element is removed from + // `pending` once the layout has grown enough to accommodate the snapshot, + // so subsequent ResizeObserver/rAF callbacks won't fight a deliberate user + // scroll (including scrolling upward, away from the snapshot position). + const pending = new Set(targets.map((t) => t.el)); + let cancelled = false; + let stopId = 0; + let ro: ResizeObserver | null = null; + + const stop = () => { + cancelled = true; + ro?.disconnect(); + window.clearTimeout(stopId); + }; + + const apply = () => { + for (const { el, snap } of targets) { + if (!pending.has(el)) continue; + if (el.scrollTop < snap.top) el.scrollTop = snap.top; + if (el.scrollLeft < snap.left) el.scrollLeft = snap.left; + // Once layout can fully accommodate the snapshot, the clamping bug is + // resolved — drop the element from pending so future ticks stay out of + // the way of any user-initiated scroll. + const canReachTop = el.scrollHeight - el.clientHeight >= snap.top; + const canReachLeft = el.scrollWidth - el.clientWidth >= snap.left; + if (canReachTop && canReachLeft) pending.delete(el); + } + if (pending.size === 0) stop(); + }; + apply(); + + if (!cancelled) { + ro = new ResizeObserver(() => { + if (cancelled) return; + apply(); + }); + for (const { el } of targets) ro.observe(el); + // A short rAF chain catches the common case where layout settles within + // the next two frames without a measurable size change. + requestAnimationFrame(() => { + if (cancelled) return; + apply(); + requestAnimationFrame(() => { + if (cancelled) return; + apply(); + }); + }); + stopId = window.setTimeout(stop, RESTORE_WINDOW_MS); + } + + return stop; +}