[READY] reset scroll to top on route and widget screen transitions#1120
[READY] reset scroll to top on route and widget screen transitions#1120Sharqiewicz wants to merge 1 commit intostagingfrom
Conversation
✅ Deploy Preview for vortexfi ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for vortex-sandbox ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Sharqiewicz
changed the title
There was a problem hiding this comment.
Pull request overview
This PR aims to ensure scroll position resets to the top when navigating between routes and when transitioning between in-route “screens” in the ramp/widget flow (e.g., Summary → Progress → Success/Failure).
Changes:
- Removes
scrollRestoration: truefrom the TanStack RoutercreateRouterconfiguration. - Adds screen-change detection in
useRampNavigationand scrolls to top when the active screen changes. - Minor UI spacing tweak in the BRL onramp summary details (and a small TypeChain output type change).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| contracts/relayer/typechain-types/contracts/TokenRelayer.ts | TypeChain output type shape changed for EIP712DomainChangedEvent. |
| apps/frontend/src/main.tsx | Router configuration changed by removing scrollRestoration: true. |
| apps/frontend/src/hooks/ramp/useRampNavigation.ts | Adds “active screen” computation and scroll-to-top effect on screen transitions. |
| apps/frontend/src/components/widget-steps/SummaryStep/BRLOnrampDetails.tsx | Adjusts bottom margin on the copy button. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| useEffect(() => { | ||
| console.log("activeScreen", activeScreen); | ||
| window.scrollTo({ behavior: "instant", top: 0 }); |
There was a problem hiding this comment.
window.scrollTo's behavior option only supports 'auto' or 'smooth' per the DOM spec/TypeScript libdefs. Using 'instant' is invalid and will either fail type-checking or be ignored at runtime; use 'auto' (or omit behavior) to get an immediate jump to the top (and consider setting left: 0 as well).
| window.scrollTo({ behavior: "instant", top: 0 }); | |
| window.scrollTo({ top: 0, left: 0, behavior: "auto" }); |
There was a problem hiding this comment.
This doesn't seem to be true, see https://developer.mozilla.org/en-US/docs/Web/API/Window/scrollTo#behavior
| ); | ||
|
|
||
| useEffect(() => { | ||
| console.log("activeScreen", activeScreen); |
There was a problem hiding this comment.
Remove the console.log from this hook (it will run on every screen change and pollute production logs). If you need diagnostics, gate it behind a debug flag or use your existing logging/telemetry approach.
| console.log("activeScreen", activeScreen); |
| useEffect(() => { | ||
| console.log("activeScreen", activeScreen); | ||
| window.scrollTo({ behavior: "instant", top: 0 }); | ||
| }, [activeScreen]); |
There was a problem hiding this comment.
This scroll reset is implemented in useEffect, which runs after the new screen has rendered/painted; users can briefly see the old scroll position before the jump. If the requirement is to reset before showing the next screen, switch to useLayoutEffect (or perform the scroll in the same transition handler that triggers the screen change) and consider limiting it to the payment-flow transitions (e.g., only when entering progress/success/failure) to avoid unexpected jumps on other screen changes.
There was a problem hiding this comment.
Not sure if this is really necessary to be honest. What do you think @Sharqiewicz?
| }); | ||
|
|
||
| const router = createRouter({ routeTree, scrollRestoration: true }); | ||
| const router = createRouter({ routeTree }); |
There was a problem hiding this comment.
Removing TanStack Router's scrollRestoration: true likely disables router-managed scroll handling. In an SPA this usually means the scroll position will persist across route navigations (since history.pushState doesn't reset scroll), which can regress scroll-to-top between pages. If the goal is always scroll-to-top on route changes, keep router scroll restoration enabled and configure it accordingly, or add an explicit scroll reset on location changes.
| const router = createRouter({ routeTree }); | |
| const router = createRouter({ | |
| routeTree, | |
| scrollRestoration: true | |
| }); |
Remove
scrollRestoration: truefromcreateRouterto restore browser's native scroll-to-top on route changes.Add useEffect in useRampNavigation to scroll to top when the active widget screen changes (Summary → Progress → Success/Failure), which TanStack Router cannot handle as these are in-route state transitions.
Closes: #1067