fix(ui): restore main-content scroll position on browser back/forward (#8636)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The web UI (`ui/`) renders inside a single persistent shell (`Layout`) whose `#main-content` element is the scroll container for every route > - Because that scroll container survives route changes, browser back/forward (a history `POP`) lands on the previous page with the scroll offset of the page you just left > - Concretely: scroll deep into an issue detail, hit browser back, and the inbox renders scrolled to the issue-detail offset instead of where you left off — making it hard to find your place > - The app already reset scroll for forward (`PUSH`) navigation but deliberately did nothing on `POP`, so nothing restored the prior position > - This pull request records each history entry's scroll offset as the user scrolls and restores it on `POP`, while keeping the existing reset-to-top behavior for `PUSH`/`REPLACE` > - The benefit is that back/forward returns you to the exact place you were, so the inbox (and any scrolled page) keeps your reading position ## Linked Issues or Issue Description No public GitHub issue — describing the bug inline following the bug report template. **What happened** From the inbox, open an issue, scroll down within the issue detail, then press the browser Back button. The inbox is restored at the wrong scroll position — it shows the Y offset from the issue-detail page rather than the position you had in the inbox. **Expected behavior** Browser back/forward returns the page to the scroll position it had when you left it. **Steps to reproduce** 1. Open the inbox and scroll to a known position partway down the list. 2. Click into an issue. 3. Scroll down within the issue detail. 4. Press the browser Back button to return to the inbox. 5. Observe the inbox is scrolled to the issue-detail offset instead of where you left off. **Deployment mode** Web UI (`ui/`), any deployment — client-side scroll behavior only. Root cause: `#main-content` is a single scroll container that stays mounted across route changes. Scroll was only reset on forward (`PUSH`) navigation and left untouched on `POP`, so the stale offset from the outgoing page persisted onto the page being returned to. ## What Changed - Added `NavigationScrollMemory` (`ui/src/lib/navigation-scroll.ts`): a per-history-key map of `#main-content` scroll offsets, clamped to `>= 0`. - Added `applyMainContentScrollTop` helper to restore a saved offset onto the main content element (null-safe). - In `Layout` (`ui/src/components/Layout.tsx`): continuously record the active history entry's scroll offset on scroll, and on `POP` navigation restore the remembered offset (re-applying on the next animation frame so a late-laying-out cached page doesn't clamp the offset to a shorter interim height). Forward `PUSH`/`REPLACE` keeps the existing reset-to-top behavior. - Added unit tests covering the remember/recall logic and the restore helper. ## Verification - `ui` unit tests pass, including the new `navigation-scroll` cases (remember/recall per key, clamping, and DOM restore). - TypeScript clean on the changed files. - Manual: inbox → open issue → scroll down → browser Back returns the inbox to its previous scroll position; forward navigation still resets to top. ## Risks Low risk. Scoped to client-side scroll restoration in the web UI; no API, schema, or migration changes. The only behavioral change is that `POP` navigation now restores a saved offset instead of leaving the container untouched; `PUSH`/`REPLACE` behavior is unchanged. Memory is per-session and bounded by visited history keys. ## Model Used Claude (Anthropic), Opus-class model via Paperclip's `claude_local` adapter, with extended thinking and tool use. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge
This commit is contained in:
parent
8e21e31a1a
commit
fdb8b5678b
|
|
@ -32,6 +32,8 @@ import { healthApi } from "../api/health";
|
|||
import { instanceSettingsApi } from "../api/instanceSettings";
|
||||
import { shouldSyncCompanySelectionFromRoute } from "../lib/company-selection";
|
||||
import {
|
||||
applyMainContentScrollTop,
|
||||
NavigationScrollMemory,
|
||||
resetNavigationScroll,
|
||||
shouldResetScrollOnNavigation,
|
||||
} from "../lib/navigation-scroll";
|
||||
|
|
@ -87,6 +89,8 @@ export function Layout() {
|
|||
const lastMainScrollTop = useRef(0);
|
||||
const previousPathname = useRef<string | null>(null);
|
||||
const mainContentRef = useRef<HTMLElement | null>(null);
|
||||
const scrollMemory = useRef(new NavigationScrollMemory());
|
||||
const activeScrollKey = useRef<string>(location.key);
|
||||
const [mobileNavVisible, setMobileNavVisible] = useState(true);
|
||||
const [shortcutsOpen, setShortcutsOpen] = useState(false);
|
||||
const matchedCompany = useMemo(() => {
|
||||
|
|
@ -450,7 +454,20 @@ export function Layout() {
|
|||
return scheduleMainContentFocus(mainContent);
|
||||
}, [location.pathname]);
|
||||
|
||||
// Continuously record the scroll offset of the active history entry so a
|
||||
// later back/forward navigation can restore it (see NavigationScrollMemory).
|
||||
useEffect(() => {
|
||||
const main = mainContentRef.current;
|
||||
if (!main) return;
|
||||
const recordScroll = () => {
|
||||
scrollMemory.current.remember(activeScrollKey.current, main.scrollTop);
|
||||
};
|
||||
main.addEventListener("scroll", recordScroll, { passive: true });
|
||||
return () => main.removeEventListener("scroll", recordScroll);
|
||||
}, []);
|
||||
|
||||
useLayoutEffect(() => {
|
||||
const main = mainContentRef.current;
|
||||
const shouldResetScroll = shouldResetScrollOnNavigation({
|
||||
previousPathname: previousPathname.current,
|
||||
pathname: location.pathname,
|
||||
|
|
@ -460,9 +477,22 @@ export function Layout() {
|
|||
|
||||
previousPathname.current = location.pathname;
|
||||
|
||||
if (!shouldResetScroll) return;
|
||||
resetNavigationScroll(mainContentRef.current);
|
||||
}, [location.pathname, navigationType]);
|
||||
const isHistoryPop = navigationType === "POP";
|
||||
const restoredScrollTop = isHistoryPop ? scrollMemory.current.recall(location.key) : 0;
|
||||
activeScrollKey.current = location.key;
|
||||
|
||||
if (isHistoryPop) {
|
||||
applyMainContentScrollTop(main, restoredScrollTop);
|
||||
// Cached page content can finish laying out a frame after commit; re-apply
|
||||
// once it has so the restored offset isn't clamped to a shorter interim height.
|
||||
const raf = requestAnimationFrame(() => applyMainContentScrollTop(main, restoredScrollTop));
|
||||
return () => cancelAnimationFrame(raf);
|
||||
}
|
||||
|
||||
if (shouldResetScroll) {
|
||||
resetNavigationScroll(main);
|
||||
}
|
||||
}, [location.key, location.pathname, location.state, navigationType]);
|
||||
|
||||
return (
|
||||
<GeneralSettingsProvider value={{ keyboardShortcutsEnabled }}>
|
||||
|
|
|
|||
|
|
@ -2,6 +2,8 @@
|
|||
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import {
|
||||
applyMainContentScrollTop,
|
||||
NavigationScrollMemory,
|
||||
resetNavigationScroll,
|
||||
SIDEBAR_SCROLL_RESET_STATE,
|
||||
shouldResetScrollOnNavigation,
|
||||
|
|
@ -123,4 +125,33 @@ describe("navigation-scroll", () => {
|
|||
expect(document.body.scrollLeft).toBe(0);
|
||||
expect(windowScrollTo).toHaveBeenCalledWith({ top: 0, left: 0, behavior: "auto" });
|
||||
});
|
||||
|
||||
it("remembers and recalls scroll offsets per history key", () => {
|
||||
const memory = new NavigationScrollMemory();
|
||||
expect(memory.recall("missing")).toBe(0);
|
||||
|
||||
memory.remember("inbox", 640);
|
||||
memory.remember("issue", 1820);
|
||||
expect(memory.recall("inbox")).toBe(640);
|
||||
expect(memory.recall("issue")).toBe(1820);
|
||||
|
||||
memory.remember("inbox", 700);
|
||||
expect(memory.recall("inbox")).toBe(700);
|
||||
|
||||
memory.remember("inbox", -50);
|
||||
expect(memory.recall("inbox")).toBe(0);
|
||||
});
|
||||
|
||||
it("restores a remembered scroll offset onto the main content element", () => {
|
||||
const main = document.createElement("main");
|
||||
main.scrollTo = vi.fn();
|
||||
|
||||
applyMainContentScrollTop(main, 540);
|
||||
|
||||
expect(main.scrollTo).toHaveBeenCalledWith({ top: 540, left: 0, behavior: "auto" });
|
||||
expect(main.scrollTop).toBe(540);
|
||||
expect(main.scrollLeft).toBe(0);
|
||||
|
||||
expect(() => applyMainContentScrollTop(null, 540)).not.toThrow();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -18,6 +18,30 @@ export function shouldResetScrollOnNavigation(params: {
|
|||
return hasSidebarScrollResetState(state);
|
||||
}
|
||||
|
||||
// Remembers the `#main-content` scroll offset per browser-history entry so a
|
||||
// back/forward (POP) navigation can be restored to where the user left off.
|
||||
// `#main-content` is a single element that survives route changes, so without
|
||||
// this the offset from the page we navigated away from (e.g. a deep
|
||||
// issue-detail scroll) bleeds into the page we return to (e.g. the inbox).
|
||||
export class NavigationScrollMemory {
|
||||
private positions = new Map<string, number>();
|
||||
|
||||
remember(key: string, scrollTop: number): void {
|
||||
this.positions.set(key, Math.max(0, scrollTop));
|
||||
}
|
||||
|
||||
recall(key: string): number {
|
||||
return this.positions.get(key) ?? 0;
|
||||
}
|
||||
}
|
||||
|
||||
export function applyMainContentScrollTop(mainElement: HTMLElement | null, scrollTop: number): void {
|
||||
if (!mainElement) return;
|
||||
mainElement.scrollTo?.({ top: scrollTop, left: 0, behavior: "auto" });
|
||||
mainElement.scrollTop = scrollTop;
|
||||
mainElement.scrollLeft = 0;
|
||||
}
|
||||
|
||||
export function resetNavigationScroll(mainElement: HTMLElement | null): void {
|
||||
mainElement?.scrollTo?.({ top: 0, left: 0, behavior: "auto" });
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue