From fdff9fe3020d41d95a9564b7a94ea0d80fe1a018 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Apr 2026 19:20:27 +0000 Subject: [PATCH 1/3] fix(app): scroll to top on route change PlotsPage and HomePage set history.scrollRestoration='manual', which persists across navigations. Clicking a footer link like /legal while scrolled down kept the previous scroll position, so users landed mid- or bottom-page on the new route. Reset scroll on pathname change in RootLayout; PlotsPage's saved-scroll restoration runs in a later effect, so back-navigation still restores the prior position. --- app/src/components/RootLayout.tsx | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/app/src/components/RootLayout.tsx b/app/src/components/RootLayout.tsx index e0b398a38f..35a697a13c 100644 --- a/app/src/components/RootLayout.tsx +++ b/app/src/components/RootLayout.tsx @@ -1,3 +1,4 @@ +import { useEffect } from 'react'; import { Outlet, useLocation } from 'react-router-dom'; import Box from '@mui/material/Box'; import Container from '@mui/material/Container'; @@ -22,9 +23,19 @@ const containerSx = { */ export function RootLayout() { const { trackEvent } = useAnalytics(); - const { pathname } = useLocation(); + const { pathname, hash } = useLocation(); const mastheadSticks = pathname !== '/plots'; + // Reset scroll on route change. PlotsPage sets scrollRestoration='manual', + // so without this the browser keeps the previous scroll position when + // navigating to short pages (e.g. /legal from the footer). Pages that + // restore a saved scroll position (PlotsPage) do so in a later effect, so + // they still override this on back-navigation. + useEffect(() => { + if (hash) return; + window.scrollTo(0, 0); + }, [pathname, hash]); + return ( Date: Sat, 25 Apr 2026 20:28:09 +0000 Subject: [PATCH 2/3] fix(app): scope scrollRestoration='manual' and skip top-scroll on POP Address review feedback on the scroll-to-top fix: - Scope the history.scrollRestoration='manual' side effect to PlotsPage and HomePage with cleanup that resets to 'auto' on unmount, so other routes get native browser back/forward scroll restoration. - Skip the RootLayout scroll-to-top when navigationType is 'POP' so back /forward navigation preserves the previous scroll position. Forward (PUSH/REPLACE) navigation still scrolls to top, fixing the original /legal-from-footer bug. - Add RootLayout.test.tsx covering the PUSH-scrolls and hash-skips cases. --- app/src/components/RootLayout.test.tsx | 80 ++++++++++++++++++++++++++ app/src/components/RootLayout.tsx | 17 +++--- app/src/pages/HomePage.tsx | 12 ++-- app/src/pages/PlotsPage.tsx | 12 ++-- 4 files changed, 106 insertions(+), 15 deletions(-) create mode 100644 app/src/components/RootLayout.test.tsx diff --git a/app/src/components/RootLayout.test.tsx b/app/src/components/RootLayout.test.tsx new file mode 100644 index 0000000000..4274635fea --- /dev/null +++ b/app/src/components/RootLayout.test.tsx @@ -0,0 +1,80 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render } from '@testing-library/react'; +import { ThemeProvider, createTheme } from '@mui/material/styles'; +import { MemoryRouter, Routes, Route, Link } from 'react-router-dom'; +import userEvent from '@testing-library/user-event'; +import { RootLayout } from './RootLayout'; + +vi.mock('../hooks', () => ({ + useAnalytics: () => ({ trackEvent: vi.fn(), trackPageview: vi.fn() }), +})); + +vi.mock('./MastheadRule', () => ({ + MastheadRule: () =>
, +})); + +vi.mock('./NavBar', () => ({ + NavBar: () =>
, +})); + +vi.mock('./Footer', () => ({ + Footer: () =>
, +})); + +const theme = createTheme(); + +function renderAt(initialPath: string) { + return render( + + + + }> + + go-legal + go-about-hash +
+ } + /> + legal-page
} /> + about-page
} /> + + + + , + ); +} + +describe('RootLayout', () => { + let scrollSpy: ReturnType; + + beforeEach(() => { + scrollSpy = vi.spyOn(window, 'scrollTo').mockImplementation(() => {}); + }); + + afterEach(() => { + scrollSpy.mockRestore(); + }); + + it('scrolls to top when navigating to a new path (PUSH)', async () => { + const user = userEvent.setup(); + renderAt('/'); + scrollSpy.mockClear(); + + await user.click(document.querySelector('a[href="/legal"]') as HTMLElement); + + expect(scrollSpy).toHaveBeenCalledWith(0, 0); + }); + + it('does not scroll to top when the destination has a hash anchor', async () => { + const user = userEvent.setup(); + renderAt('/'); + scrollSpy.mockClear(); + + await user.click(document.querySelector('a[href="/about#section"]') as HTMLElement); + + expect(scrollSpy).not.toHaveBeenCalledWith(0, 0); + }); +}); diff --git a/app/src/components/RootLayout.tsx b/app/src/components/RootLayout.tsx index 35a697a13c..94e80b5e2f 100644 --- a/app/src/components/RootLayout.tsx +++ b/app/src/components/RootLayout.tsx @@ -1,5 +1,5 @@ import { useEffect } from 'react'; -import { Outlet, useLocation } from 'react-router-dom'; +import { Outlet, useLocation, useNavigationType } from 'react-router-dom'; import Box from '@mui/material/Box'; import Container from '@mui/material/Container'; @@ -24,17 +24,20 @@ const containerSx = { export function RootLayout() { const { trackEvent } = useAnalytics(); const { pathname, hash } = useLocation(); + const navigationType = useNavigationType(); const mastheadSticks = pathname !== '/plots'; - // Reset scroll on route change. PlotsPage sets scrollRestoration='manual', - // so without this the browser keeps the previous scroll position when - // navigating to short pages (e.g. /legal from the footer). Pages that - // restore a saved scroll position (PlotsPage) do so in a later effect, so - // they still override this on back-navigation. + // Reset scroll on forward navigation (PUSH/REPLACE). Without this, short + // pages like /legal inherit the previous page's scroll position because + // PlotsPage sets scrollRestoration='manual'. Skip on POP so browser + // back/forward keeps native scroll restoration; skip when a hash anchor + // is present so in-page anchors still work. PlotsPage runs its own + // saved-scroll restore in a later effect, so it still overrides this. useEffect(() => { if (hash) return; + if (navigationType === 'POP') return; window.scrollTo(0, 0); - }, [pathname, hash]); + }, [pathname, hash, navigationType]); return ( { - if ('scrollRestoration' in history) { - history.scrollRestoration = 'manual'; - } + if (!('scrollRestoration' in history)) return; + history.scrollRestoration = 'manual'; + return () => { + history.scrollRestoration = 'auto'; + }; }, []); // Custom hooks diff --git a/app/src/pages/PlotsPage.tsx b/app/src/pages/PlotsPage.tsx index 19c22dfb23..ca579da667 100644 --- a/app/src/pages/PlotsPage.tsx +++ b/app/src/pages/PlotsPage.tsx @@ -21,11 +21,15 @@ export function PlotsPage() { const { specsData, librariesData } = useAppData(); const { homeStateRef, saveScrollPosition } = useHomeState(); - // Disable browser's automatic scroll restoration + // Disable browser's automatic scroll restoration so we can restore from + // our persisted state (homeStateRef.scrollY) instead. Restore on unmount + // so other routes get native back/forward behavior. useEffect(() => { - if ('scrollRestoration' in history) { - history.scrollRestoration = 'manual'; - } + if (!('scrollRestoration' in history)) return; + history.scrollRestoration = 'manual'; + return () => { + history.scrollRestoration = 'auto'; + }; }, []); const { trackPageview, trackEvent } = useAnalytics(); From 41e1e5a8e0daa65d8f7ff22bc11c563bc36318d9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Apr 2026 20:31:20 +0000 Subject: [PATCH 3/3] test(app): cover scrollRestoration mount/unmount in HomePage and PlotsPage The cleanup-on-unmount branch added in the previous commit wasn't exercised, so codecov/patch/frontend stayed below the 80% threshold. Add a mount/unmount test to each page that asserts scrollRestoration flips to 'manual' on mount and back to 'auto' on unmount. --- app/src/pages/HomePage.test.tsx | 18 +++++++++++++++++- app/src/pages/PlotsPage.test.tsx | 18 +++++++++++++++++- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/app/src/pages/HomePage.test.tsx b/app/src/pages/HomePage.test.tsx index 2fa6c508a6..9bb9ff6806 100644 --- a/app/src/pages/HomePage.test.tsx +++ b/app/src/pages/HomePage.test.tsx @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { render, screen } from '../test-utils'; // Mock hooks @@ -116,4 +116,20 @@ describe('HomePage', () => { render(); expect(screen.getByTestId('helmet')).toBeInTheDocument(); }); + + describe('scrollRestoration', () => { + const original = history.scrollRestoration; + + afterEach(() => { + history.scrollRestoration = original; + }); + + it("sets scrollRestoration to 'manual' on mount and restores 'auto' on unmount", () => { + history.scrollRestoration = 'auto'; + const { unmount } = render(); + expect(history.scrollRestoration).toBe('manual'); + unmount(); + expect(history.scrollRestoration).toBe('auto'); + }); + }); }); diff --git a/app/src/pages/PlotsPage.test.tsx b/app/src/pages/PlotsPage.test.tsx index cc27bfe507..b2229b44a8 100644 --- a/app/src/pages/PlotsPage.test.tsx +++ b/app/src/pages/PlotsPage.test.tsx @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { render, screen } from '../test-utils'; vi.mock('../hooks', () => ({ @@ -68,4 +68,20 @@ describe('PlotsPage', () => { render(); expect(screen.getByTestId('helmet')).toBeInTheDocument(); }); + + describe('scrollRestoration', () => { + const original = history.scrollRestoration; + + afterEach(() => { + history.scrollRestoration = original; + }); + + it("sets scrollRestoration to 'manual' on mount and restores 'auto' on unmount", () => { + history.scrollRestoration = 'auto'; + const { unmount } = render(); + expect(history.scrollRestoration).toBe('manual'); + unmount(); + expect(history.scrollRestoration).toBe('auto'); + }); + }); });