fix(app): preserve prior scrollRestoration value across mount/unmount#5404
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.
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.
Address review feedback on the scoped scrollRestoration cleanup:
hard-setting 'auto' on unmount could clobber a non-default prior value
set elsewhere. Capture history.scrollRestoration on mount and restore
that exact value on unmount in both HomePage and PlotsPage.
Tests now cover both prior-state cases ('auto' and 'manual').
There was a problem hiding this comment.
Pull request overview
Adjusts scroll restoration handling to avoid overwriting any pre-existing history.scrollRestoration mode, while expanding test coverage around the mount/unmount behavior.
Changes:
- Capture
history.scrollRestorationon mount inHomePage/PlotsPageand restore the captured value on unmount (instead of forcing'auto'). - Extend
HomePageandPlotsPagetests to validate restoration for both prior values ('auto'and'manual'). - Add route-navigation scroll-to-top behavior in
RootLayout, plus a new unit test file validating scroll reset and hash-anchor exemptions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/pages/PlotsPage.tsx | Save previous scrollRestoration, set to manual while mounted, restore on unmount. |
| app/src/pages/PlotsPage.test.tsx | Add tests verifying mount/unmount restoration for 'auto' and 'manual'. |
| app/src/pages/HomePage.tsx | Same scrollRestoration preservation pattern as PlotsPage. |
| app/src/pages/HomePage.test.tsx | Add tests verifying mount/unmount restoration for 'auto' and 'manual'. |
| app/src/components/RootLayout.tsx | Scroll-to-top on PUSH/REPLACE navigation (skip POP and hash). |
| app/src/components/RootLayout.test.tsx | New tests for RootLayout scroll-to-top behavior and hash-anchor skip. |
| // 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. |
There was a problem hiding this comment.
The inline comment attributes the scroll reset behavior to "PlotsPage sets scrollRestoration='manual'", but history.scrollRestoration only affects browser back/forward restoration (POP), not whether SPA route transitions (PUSH/REPLACE) keep the current scroll position. Consider updating this comment to reflect the actual reason for window.scrollTo(0, 0) and (since HomePage also sets scrollRestoration) avoid calling out only PlotsPage.
| // 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. | |
| // Reset scroll on forward navigation (PUSH/REPLACE). In SPA route changes, | |
| // the next page can otherwise keep the previous page's scroll position, | |
| // which is especially noticeable on short pages like /legal. Skip on POP | |
| // so browser back/forward keeps native scroll restoration; skip when a | |
| // hash anchor is present so in-page anchors still work. Pages with their | |
| // own saved-scroll restore in a later effect can still override this. |
| // 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]); |
There was a problem hiding this comment.
The PR description focuses on preserving/restoring history.scrollRestoration in HomePage/PlotsPage, but this PR also introduces new global scroll-to-top behavior in RootLayout (and adds a new RootLayout test file). Please update the PR description (or split the change) so reviewers understand the additional user-facing behavior being introduced.
scrollRestoration only affects POP navigation, not PUSH/REPLACE. The real reason for window.scrollTo(0, 0) is that SPA route transitions inherently keep the previous scroll position. Update the comment to state that, and stop singling out PlotsPage since HomePage also sets scrollRestoration.
|
@copilot resolve the merge conflicts in this pull request |
# 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
Merge conflicts resolved in 0490658. All 5 conflicting files ( |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
history.scrollRestorationinHomePageandPlotsPage(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.RootLayout(carried over from the squash-merged fix(app): scroll to top on route change #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).scrollRestoration='manual'inHomePage/PlotsPageto those pages with cleanup on unmount, so other routes get native browser back/forward behavior.RootLayout.test.tsx(PUSH scrolls, hash skips); extendedHomePage.test.tsxandPlotsPage.test.tsxcovering mount/unmount restoration for both'auto'and'manual'prior values.User-facing behavior
/legalfrom a long page now lands at the top.HomePage,PlotsPage) still restore from their persisted state.Test plan
cd app && yarn test— all suites pass/plots→ footer/legallands at top/legalto/plotsrestores prior scroll/about#section) still scrolls to anchor