From 90455abd8dc5a7655633e70d44b079605beb9542 Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Mon, 25 May 2026 15:31:01 +0800 Subject: [PATCH] fix(desktop): preserve tab scroll position across Activity visibility cycles (MUL-2602) (#3196) Closes #3183. Tabs render under ``, which keeps React state but drops DOM scrollTop when the subtree leaves layout. Switching to another tab and back sent users to the top of long discussions. `useTabScrollRestore` records the scrollTop of every element marked with `data-tab-scroll-root` while the tab is visible (capture-phase scroll listener) and restores them in a useLayoutEffect on the next visible transition, before paint. Saved offsets are dropped when the tab's path changes so intra-tab navigation lands at scroll=0 instead of inheriting the previous route's position. Mark scroll containers in views with `data-tab-scroll-root` (issue detail + chat message list ship with the marker; other views can adopt the convention as needed). `useAutoScroll` previously called `scrollToBottom()` on every effect mount, which would have overwritten the restored offset every time a chat tab cycled back to visible. Guard it with a once-per-instance ref. Co-authored-by: multica-agent --- .../renderer/src/components/tab-content.tsx | 33 ++++- .../src/hooks/use-tab-scroll-restore.test.tsx | 116 ++++++++++++++++++ .../src/hooks/use-tab-scroll-restore.ts | 70 +++++++++++ packages/ui/hooks/use-auto-scroll.ts | 11 +- .../chat/components/chat-message-list.tsx | 7 +- .../views/issues/components/issue-detail.tsx | 1 + 6 files changed, 231 insertions(+), 7 deletions(-) create mode 100644 apps/desktop/src/renderer/src/hooks/use-tab-scroll-restore.test.tsx create mode 100644 apps/desktop/src/renderer/src/hooks/use-tab-scroll-restore.ts diff --git a/apps/desktop/src/renderer/src/components/tab-content.tsx b/apps/desktop/src/renderer/src/components/tab-content.tsx index 90e37b9e7..6113382f0 100644 --- a/apps/desktop/src/renderer/src/components/tab-content.tsx +++ b/apps/desktop/src/renderer/src/components/tab-content.tsx @@ -3,6 +3,7 @@ import { RouterProvider } from "react-router-dom"; import { useActiveGroup } from "@/stores/tab-store"; import { TabNavigationProvider } from "@/platform/navigation"; import { useTabRouterSync } from "@/hooks/use-tab-router-sync"; +import { useTabScrollRestore } from "@/hooks/use-tab-scroll-restore"; import type { Tab } from "@/stores/tab-store"; /** @@ -15,6 +16,28 @@ function TabRouterInner({ tab }: { tab: Tab }) { return null; } +/** + * Wraps a tab's subtree so its scroll position survives the round trip + * through ``. Lives inside Activity so the hook's + * effects cycle with the tab's visibility — see `useTabScrollRestore` for + * the mechanism. `display: contents` keeps the wrapper transparent to + * the surrounding flex layout. + */ +function TabScrollRestoreWrapper({ + tabPath, + children, +}: { + tabPath: string; + children: React.ReactNode; +}) { + const ref = useTabScrollRestore(tabPath); + return ( +
+ {children} +
+ ); +} + /** * Renders the active workspace's tabs using Activity for state preservation. * Only the active tab is visible; hidden tabs keep their DOM and React state. @@ -44,10 +67,12 @@ export function TabContent() { key={tab.id} mode={tab.id === group.activeTabId ? "visible" : "hidden"} > - - - - + + + + + + ))} diff --git a/apps/desktop/src/renderer/src/hooks/use-tab-scroll-restore.test.tsx b/apps/desktop/src/renderer/src/hooks/use-tab-scroll-restore.test.tsx new file mode 100644 index 000000000..3f0f25bf7 --- /dev/null +++ b/apps/desktop/src/renderer/src/hooks/use-tab-scroll-restore.test.tsx @@ -0,0 +1,116 @@ +import { Activity } from "react"; +import { describe, expect, it } from "vitest"; +import { fireEvent, render } from "@testing-library/react"; +import { useTabScrollRestore } from "./use-tab-scroll-restore"; + +function Harness({ path }: { path: string }) { + const ref = useTabScrollRestore(path); + return ( +
+
+
+
+
+
+
+
+
+
+
+ ); +} + +function App({ visible, path }: { visible: boolean; path: string }) { + return ( + + + + ); +} + +function setScroll(el: HTMLElement, top: number) { + el.scrollTop = top; + fireEvent.scroll(el); +} + +describe("useTabScrollRestore", () => { + it("restores scroll position when a tab cycles through hidden -> visible", () => { + const { rerender, getByTestId } = render( + , + ); + const scroller = getByTestId("scroller") as HTMLElement; + + setScroll(scroller, 500); + expect(scroller.scrollTop).toBe(500); + + // Simulate Activity hiding the subtree: layout would drop the offset. + rerender(); + scroller.scrollTop = 0; + + rerender(); + expect(scroller.scrollTop).toBe(500); + }); + + it("restores multiple named scroll roots independently", () => { + const { rerender, getByTestId } = render( + , + ); + const main = getByTestId("scroller") as HTMLElement; + const aside = getByTestId("aside") as HTMLElement; + + setScroll(main, 300); + setScroll(aside, 150); + + rerender(); + main.scrollTop = 0; + aside.scrollTop = 0; + + rerender(); + expect(main.scrollTop).toBe(300); + expect(aside.scrollTop).toBe(150); + }); + + it("ignores scroll on elements without the data-tab-scroll-root marker", () => { + const { rerender, getByTestId } = render( + , + ); + const unmarked = getByTestId("unmarked") as HTMLElement; + + setScroll(unmarked, 250); + + rerender(); + unmarked.scrollTop = 0; + + rerender(); + expect(unmarked.scrollTop).toBe(0); + }); + + it("drops saved offsets when the tab path changes (intra-tab navigation)", () => { + const { rerender, getByTestId } = render( + , + ); + const scroller = getByTestId("scroller") as HTMLElement; + + setScroll(scroller, 500); + + // Navigating within the tab swaps the active route — same marker key, + // different page. We should NOT restore the prior page's offset. + rerender(); + scroller.scrollTop = 0; + + rerender(); + rerender(); + expect(scroller.scrollTop).toBe(0); + }); +}); diff --git a/apps/desktop/src/renderer/src/hooks/use-tab-scroll-restore.ts b/apps/desktop/src/renderer/src/hooks/use-tab-scroll-restore.ts new file mode 100644 index 000000000..cfb3a1a37 --- /dev/null +++ b/apps/desktop/src/renderer/src/hooks/use-tab-scroll-restore.ts @@ -0,0 +1,70 @@ +import { useEffect, useLayoutEffect, useRef } from "react"; + +/** + * Persist a tab's scroll positions across visibility transitions. + * + * Tabs render under ``, which keeps React + * state but loses DOM scrollTop — the subtree is taken out of layout while + * hidden and rejoins with scrollTop=0. This hook records every marked + * container's `scrollTop` while the tab is visible (continuously, via a + * capture-phase scroll listener) and restores them in a `useLayoutEffect` + * the next time the tab becomes visible, before the browser paints. + * + * Mark scroll containers in views with `data-tab-scroll-root`. The + * attribute value is the cache key — defaults to `"main"` for unnamed + * roots. Most pages have a single scroll container, so a bare attribute + * is enough; named keys are only needed when a page has multiple + * independently scrollable regions whose positions must all be restored. + * + * When the tab's path changes (intra-tab navigation), the saved offsets + * are dropped — the new route's container shares the same marker key but + * is a different page, and restoring the old offset would land the user + * somewhere arbitrary on the new page. + */ +export function useTabScrollRestore(tabPath: string) { + const containerRef = useRef(null); + const savedRef = useRef>(new Map()); + const prevPathRef = useRef(tabPath); + + if (prevPathRef.current !== tabPath) { + savedRef.current.clear(); + prevPathRef.current = tabPath; + } + + // cleans up effects on hidden and re-mounts them on visible, + // so an empty-deps useLayoutEffect runs exactly on every hidden → visible + // transition. Restoring here (before the browser paints) avoids any + // flash at scrollTop=0. + useLayoutEffect(() => { + const root = containerRef.current; + if (!root) return; + const els = root.querySelectorAll("[data-tab-scroll-root]"); + els.forEach((el) => { + const key = scrollKey(el); + const saved = savedRef.current.get(key); + if (saved !== undefined && el.scrollTop !== saved) { + el.scrollTop = saved; + } + }); + }, []); + + useEffect(() => { + const root = containerRef.current; + if (!root) return; + const onScroll = (e: Event) => { + const target = e.target; + if (!(target instanceof HTMLElement)) return; + if (!target.hasAttribute("data-tab-scroll-root")) return; + savedRef.current.set(scrollKey(target), target.scrollTop); + }; + // Scroll events don't bubble, but capture catches them anyway. + root.addEventListener("scroll", onScroll, { capture: true, passive: true }); + return () => root.removeEventListener("scroll", onScroll, true); + }, []); + + return containerRef; +} + +function scrollKey(el: HTMLElement): string { + return el.getAttribute("data-tab-scroll-root") || "main"; +} diff --git a/packages/ui/hooks/use-auto-scroll.ts b/packages/ui/hooks/use-auto-scroll.ts index 8455722c4..74b23a96e 100644 --- a/packages/ui/hooks/use-auto-scroll.ts +++ b/packages/ui/hooks/use-auto-scroll.ts @@ -10,6 +10,11 @@ import { type RefObject, useEffect, useRef, useCallback } from "react" export function useAutoScroll(ref: RefObject) { const stickRef = useRef(true) const lockRef = useRef(false) + // Re-running the initial scroll-to-bottom on every effect mount would + // overwrite the scroll position any time React tears the effect down and + // brings it back — e.g. when the host tab cycles through `` and back. We want the jump only on a real first mount. + const didInitialScrollRef = useRef(false) useEffect(() => { const el = ref.current @@ -53,8 +58,10 @@ export function useAutoScroll(ref: RefObject) { el.addEventListener("scroll", onScroll, { passive: true }) - // Initial scroll to bottom - scrollToBottom() + if (!didInitialScrollRef.current) { + didInitialScrollRef.current = true + scrollToBottom() + } return () => { el.removeEventListener("scroll", onScroll) diff --git a/packages/views/chat/components/chat-message-list.tsx b/packages/views/chat/components/chat-message-list.tsx index 6eaf68edd..400a9c036 100644 --- a/packages/views/chat/components/chat-message-list.tsx +++ b/packages/views/chat/components/chat-message-list.tsx @@ -77,7 +77,12 @@ export function ChatMessageList({ const showStatusPill = !!pendingTaskId && !pendingAlreadyPersisted && !!pendingTask; return ( -
+
{/* Inner container matches issue / project detail width convention * (max-w-4xl + mx-auto) so switching between chat and content * views doesn't jolt the reading width. px-5 is a touch tighter diff --git a/packages/views/issues/components/issue-detail.tsx b/packages/views/issues/components/issue-detail.tsx index 384781a29..4bf589d5d 100644 --- a/packages/views/issues/components/issue-detail.tsx +++ b/packages/views/issues/components/issue-detail.tsx @@ -1674,6 +1674,7 @@ export function IssueDetail({ issueId, onDelete, onDone, defaultSidebarOpen = tr