Skip to content

Commit 12add69

Browse files
fix(app): preserve prior scrollRestoration value across mount/unmount (#5404)
Follow-up to #5403 addressing review feedback that arrived after the original PR was squash-merged. Because #5403 was squash-merged, this branch's individual commits show as new against `main`. The diff therefore covers both the feedback fix and the original commits — the description below reflects the full diff. ## Summary - **Preserve prior `history.scrollRestoration`** in `HomePage` and `PlotsPage` (the new bit). Capture the value on mount and restore it on unmount, instead of hard-setting `'auto'` — avoids clobbering a non-default value set elsewhere. - **Scroll-to-top on forward navigation** in `RootLayout` (carried over from the squash-merged #5403). Skips POP navigation so browser back/forward keeps native scroll restoration; skips when a hash anchor is present so in-page anchors still work. Comment in the source updated to accurately describe the SPA-PUSH cause (per review feedback). - **Scoped `scrollRestoration='manual'`** in `HomePage`/`PlotsPage` to those pages with cleanup on unmount, so other routes get native browser back/forward behavior. - **Tests:** new `RootLayout.test.tsx` (PUSH scrolls, hash skips); extended `HomePage.test.tsx` and `PlotsPage.test.tsx` covering mount/unmount restoration for both `'auto'` and `'manual'` prior values. ## User-facing behavior - Clicking a footer link like `/legal` from a long page now lands at the top. - Browser back/forward to a route without custom scroll handling restores the previous position. - Pages that manage their own saved scroll (`HomePage`, `PlotsPage`) still restore from their persisted state. ## Test plan - [ ] `cd app && yarn test` — all suites pass - [ ] Navigate `/plots` → footer `/legal` lands at top - [ ] Browser back from `/legal` to `/plots` restores prior scroll - [ ] In-page hash anchor (e.g. `/about#section`) still scrolls to anchor --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 06038fd commit 12add69

5 files changed

Lines changed: 34 additions & 14 deletions

File tree

app/src/components/RootLayout.tsx

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,12 +27,12 @@ export function RootLayout() {
2727
const navigationType = useNavigationType();
2828
const mastheadSticks = pathname !== '/plots';
2929

30-
// Reset scroll on forward navigation (PUSH/REPLACE). Without this, short
31-
// pages like /legal inherit the previous page's scroll position because
32-
// PlotsPage sets scrollRestoration='manual'. Skip on POP so browser
33-
// back/forward keeps native scroll restoration; skip when a hash anchor
34-
// is present so in-page anchors still work. PlotsPage runs its own
35-
// saved-scroll restore in a later effect, so it still overrides this.
30+
// Reset scroll on forward navigation (PUSH/REPLACE). In SPA route changes,
31+
// the next page can otherwise keep the previous page's scroll position,
32+
// which is especially noticeable on short pages like /legal. Skip on POP
33+
// so browser back/forward keeps native scroll restoration; skip when a
34+
// hash anchor is present so in-page anchors still work. Pages with their
35+
// own saved-scroll restore in a later effect can still override this.
3636
useEffect(() => {
3737
if (hash) return;
3838
if (navigationType === 'POP') return;

app/src/pages/HomePage.test.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,12 +124,20 @@ describe('HomePage', () => {
124124
history.scrollRestoration = original;
125125
});
126126

127-
it("sets scrollRestoration to 'manual' on mount and restores 'auto' on unmount", () => {
127+
it("sets scrollRestoration to 'manual' on mount and restores the previous value on unmount", () => {
128128
history.scrollRestoration = 'auto';
129129
const { unmount } = render(<HomePage />);
130130
expect(history.scrollRestoration).toBe('manual');
131131
unmount();
132132
expect(history.scrollRestoration).toBe('auto');
133133
});
134+
135+
it('restores a non-default previous value on unmount instead of forcing auto', () => {
136+
history.scrollRestoration = 'manual';
137+
const { unmount } = render(<HomePage />);
138+
expect(history.scrollRestoration).toBe('manual');
139+
unmount();
140+
expect(history.scrollRestoration).toBe('manual');
141+
});
134142
});
135143
});

app/src/pages/HomePage.tsx

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,15 @@ export function HomePage() {
2929
const { homeStateRef, saveScrollPosition } = useHomeState();
3030

3131
// Disable browser's automatic scroll restoration so we can restore from
32-
// our persisted state (homeStateRef.scrollY) instead. Restore on unmount
33-
// so other routes get native back/forward behavior.
32+
// our persisted state (homeStateRef.scrollY) instead. Capture the prior
33+
// mode and restore it on unmount, so we don't clobber any non-default
34+
// value set elsewhere and other routes get back native behavior.
3435
useEffect(() => {
3536
if (!('scrollRestoration' in history)) return;
37+
const previous = history.scrollRestoration;
3638
history.scrollRestoration = 'manual';
3739
return () => {
38-
history.scrollRestoration = 'auto';
40+
history.scrollRestoration = previous;
3941
};
4042
}, []);
4143

app/src/pages/PlotsPage.test.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,12 +76,20 @@ describe('PlotsPage', () => {
7676
history.scrollRestoration = original;
7777
});
7878

79-
it("sets scrollRestoration to 'manual' on mount and restores 'auto' on unmount", () => {
79+
it("sets scrollRestoration to 'manual' on mount and restores the previous value on unmount", () => {
8080
history.scrollRestoration = 'auto';
8181
const { unmount } = render(<PlotsPage />);
8282
expect(history.scrollRestoration).toBe('manual');
8383
unmount();
8484
expect(history.scrollRestoration).toBe('auto');
8585
});
86+
87+
it('restores a non-default previous value on unmount instead of forcing auto', () => {
88+
history.scrollRestoration = 'manual';
89+
const { unmount } = render(<PlotsPage />);
90+
expect(history.scrollRestoration).toBe('manual');
91+
unmount();
92+
expect(history.scrollRestoration).toBe('manual');
93+
});
8694
});
8795
});

app/src/pages/PlotsPage.tsx

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,15 @@ export function PlotsPage() {
2222
const { homeStateRef, saveScrollPosition } = useHomeState();
2323

2424
// Disable browser's automatic scroll restoration so we can restore from
25-
// our persisted state (homeStateRef.scrollY) instead. Restore on unmount
26-
// so other routes get native back/forward behavior.
25+
// our persisted state (homeStateRef.scrollY) instead. Capture the prior
26+
// mode and restore it on unmount, so we don't clobber any non-default
27+
// value set elsewhere and other routes get back native behavior.
2728
useEffect(() => {
2829
if (!('scrollRestoration' in history)) return;
30+
const previous = history.scrollRestoration;
2931
history.scrollRestoration = 'manual';
3032
return () => {
31-
history.scrollRestoration = 'auto';
33+
history.scrollRestoration = previous;
3234
};
3335
}, []);
3436

0 commit comments

Comments
 (0)