fix(app): scroll to top on route change#5403
Conversation
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.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Adds centralized scroll management in the React app shell to prevent unintended scroll position carry-over between routes when some pages opt into history.scrollRestoration = 'manual'.
Changes:
- Add a
useEffectinRootLayoutto scroll to the top onpathnamechanges (skipping hash navigations). - Extend
useLocation()usage to includehashto avoid interfering with anchor navigation.
| // 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(() => { |
There was a problem hiding this comment.
This scroll-reset effect doesn’t address the underlying global side effect that history.scrollRestoration was set to 'manual' (and that setting persists across routes). As a result, back/forward scroll restoration can remain disabled for routes that don’t implement their own saved-scroll logic. Consider resetting history.scrollRestoration back to 'auto' on route change here (guarded by 'scrollRestoration' in history) or adding cleanup in the pages that set it to 'manual' so the behavior is scoped to those pages.
| // 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(() => { | |
| // navigating to short pages (e.g. /legal from the footer). Reset the | |
| // browser's global scroll restoration mode here as well so routes without | |
| // custom saved-scroll handling fall back to native back/forward behavior. | |
| // Pages that restore a saved scroll position (PlotsPage) do so in a later | |
| // effect, so they still override this on back-navigation. | |
| useEffect(() => { | |
| if ('scrollRestoration' in window.history) { | |
| window.history.scrollRestoration = 'auto'; | |
| } |
| useEffect(() => { | ||
| if (hash) return; | ||
| window.scrollTo(0, 0); | ||
| }, [pathname, hash]); |
There was a problem hiding this comment.
This change introduces new navigation/scroll behavior but there’s no unit test covering it. Consider adding a RootLayout.test.tsx that renders the layout with a memory router and asserts window.scrollTo is called on pathname changes and not called (or handled appropriately) when navigating to a URL with a hash.
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.
…sPage 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.
| if (!('scrollRestoration' in history)) return; | ||
| history.scrollRestoration = 'manual'; | ||
| return () => { | ||
| history.scrollRestoration = 'auto'; | ||
| }; |
There was a problem hiding this comment.
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).
| if (!('scrollRestoration' in history)) return; | ||
| history.scrollRestoration = 'manual'; | ||
| return () => { | ||
| history.scrollRestoration = 'auto'; | ||
| }; |
There was a problem hiding this comment.
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).
# Conflicts: # app/src/components/RootLayout.tsx # app/src/pages/HomePage.test.tsx # app/src/pages/HomePage.tsx # app/src/pages/PlotsPage.test.tsx # app/src/pages/PlotsPage.tsx
…#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>
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.