Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 80 additions & 0 deletions app/src/components/RootLayout.test.tsx
Original file line number Diff line number Diff line change
@@ -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: () => <div data-testid="masthead" />,
}));

vi.mock('./NavBar', () => ({
NavBar: () => <div data-testid="navbar" />,
}));

vi.mock('./Footer', () => ({
Footer: () => <div data-testid="footer" />,
}));

const theme = createTheme();

function renderAt(initialPath: string) {
return render(
<ThemeProvider theme={theme}>
<MemoryRouter initialEntries={[initialPath]}>
<Routes>
<Route element={<RootLayout />}>
<Route
path="/"
element={
<div>
<Link to="/legal">go-legal</Link>
<Link to="/about#section">go-about-hash</Link>
</div>
}
/>
<Route path="/legal" element={<div>legal-page</div>} />
<Route path="/about" element={<div>about-page</div>} />
</Route>
</Routes>
</MemoryRouter>
</ThemeProvider>,
);
}

describe('RootLayout', () => {
let scrollSpy: ReturnType<typeof vi.spyOn>;

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);
});
});
18 changes: 16 additions & 2 deletions app/src/components/RootLayout.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Outlet, useLocation } from 'react-router-dom';
import { useEffect } from 'react';
import { Outlet, useLocation, useNavigationType } from 'react-router-dom';
import Box from '@mui/material/Box';
import Container from '@mui/material/Container';

Expand All @@ -22,9 +23,22 @@ const containerSx = {
*/
export function RootLayout() {
const { trackEvent } = useAnalytics();
const { pathname } = useLocation();
const { pathname, hash } = useLocation();
const navigationType = useNavigationType();
const mastheadSticks = pathname !== '/plots';

// 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, navigationType]);

return (
<Box sx={{
display: 'flex',
Expand Down
18 changes: 17 additions & 1 deletion app/src/pages/HomePage.test.tsx
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -116,4 +116,20 @@ describe('HomePage', () => {
render(<HomePage />);
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(<HomePage />);
expect(history.scrollRestoration).toBe('manual');
unmount();
expect(history.scrollRestoration).toBe('auto');
});
});
});
12 changes: 8 additions & 4 deletions app/src/pages/HomePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,15 @@ export function HomePage() {
const { specsData, librariesData, stats } = 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';
};
Comment on lines +35 to +39
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as PlotsPage: the effect cleanup hard-sets history.scrollRestoration to 'auto', which may override a pre-existing non-default value. Prefer saving the previous value on mount and restoring it on unmount (and keep the comment in sync with the behavior).

Copilot uses AI. Check for mistakes.
}, []);

// Custom hooks
Expand Down
18 changes: 17 additions & 1 deletion app/src/pages/PlotsPage.test.tsx
Original file line number Diff line number Diff line change
@@ -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', () => ({
Expand Down Expand Up @@ -68,4 +68,20 @@ describe('PlotsPage', () => {
render(<PlotsPage />);
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(<PlotsPage />);
expect(history.scrollRestoration).toBe('manual');
unmount();
expect(history.scrollRestoration).toBe('auto');
});
});
});
12 changes: 8 additions & 4 deletions app/src/pages/PlotsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
};
Comment on lines +28 to +32
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cleanup forces history.scrollRestoration back to 'auto', which can clobber a non-default prior value (e.g. if another part of the app set it to 'manual'). Consider capturing the previous history.scrollRestoration value before setting it to 'manual' and restoring that exact value in the cleanup (and adjust the comment above accordingly).

Copilot uses AI. Check for mistakes.
}, []);

const { trackPageview, trackEvent } = useAnalytics();
Expand Down
Loading