🔧 router scrollBehavior #11572
🔧 router scrollBehavior #11572
Conversation
roiLeo
requested review from
vikiival and
hassnian
and removed request for
a team
✅ Deploy Preview for koda-canary ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughA new router options configuration file has been added to define custom scroll behavior for a Nuxt.js application, replacing the previous standalone scroll behavior script. The logic for handling scroll positions, including hash navigation and route-based scroll disabling, is now encapsulated within an exported configuration object. Changes
Sequence Diagram(s)sequenceDiagram
participant Router
participant ScrollBehavior
participant DOM
Router->>ScrollBehavior: On route navigation (to, from)
alt Route name in disable list
ScrollBehavior-->>Router: Return false (no scroll)
else Route has hash
ScrollBehavior->>DOM: findEl(hash)
alt Element found
ScrollBehavior->>DOM: Scroll to element (smooth or instant)
else Element not found
ScrollBehavior->>DOM: Scroll to #app
end
else No hash
ScrollBehavior->>DOM: Scroll to (0,0)
end
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the router scroll behavior to fix scroll position issues during route changes and streamlines the configuration by removing the old scroll behavior file.
- Removed the outdated app/router.scrollBehavior.js file.
- Introduced a new TypeScript file (app/router.options.ts) for scroll behavior with expanded route handling logic.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/router.scrollBehavior.js | Removed the old scroll behavior implementation. |
| app/router.options.ts | Added a new scroll behavior configuration with improved type integration. |
| @@ -0,0 +1,56 @@ | |||
| import type { RouterConfig } from '@nuxt/schema' | |||
|
|
|||
| const findEl = async (hash, x = 0) => { | |||
There was a problem hiding this comment.
Consider adding explicit type annotations for the 'hash' parameter and for the function's return type to enhance clarity and maintainability.
| const findEl = async (hash, x = 0) => { | |
| const findEl = async (hash: string, x = 0): Promise<Element | null> => { |
Copilot uses AI. Check for mistakes.
| ) | ||
| } | ||
|
|
||
| const defaultScrollBehavior = async (to): Promise<false | any | { left: number, top: number }> => { |
There was a problem hiding this comment.
Consider defining a specific type for the 'to' parameter rather than using an implicit type to improve type safety and code clarity.
| const defaultScrollBehavior = async (to): Promise<false | any | { left: number, top: number }> => { | |
| const defaultScrollBehavior = async (to: RouteLocationNormalized): Promise<false | any | { left: number, top: number }> => { |
Copilot uses AI. Check for mistakes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/router.options.ts(1 hunks)app/router.scrollBehavior.js(0 hunks)
💤 Files with no reviewable changes (1)
- app/router.scrollBehavior.js
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Redirect rules
- GitHub Check: shard (8, 10)
- GitHub Check: shard (10, 10)
- GitHub Check: shard (2, 10)
- GitHub Check: runner / eslint
- GitHub Check: shard (9, 10)
- GitHub Check: shard (7, 10)
- GitHub Check: shard (5, 10)
- GitHub Check: shard (6, 10)
- GitHub Check: shard (3, 10)
- GitHub Check: shard (4, 10)
- GitHub Check: shard (1, 10)
- GitHub Check: build
- GitHub Check: build-cf
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (3)
app/router.options.ts (3)
1-1: LGTM!Correct import for Nuxt.js router configuration.
30-32: Clarify the intended behavior for disabled scroll routes.When a route is in the
disableScrollToToplist, the function returnsundefined. If the intention is to preserve the current scroll position, consider returningfalseinstead.if (disableScrollToTop.includes(toPath)) { - return + return false // Preserve current scroll position }Please confirm if this change aligns with the intended behavior for maintaining scroll position on these routes.
53-56: LGTM!The router configuration export follows Nuxt.js conventions correctly. The use of
satisfies RouterConfigensures type safety while maintaining type inference.
| ) | ||
| } | ||
|
|
||
| const defaultScrollBehavior = async (to): Promise<false | any | { left: number, top: number }> => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and clarify return behavior.
The return type uses any which reduces type safety. Also, when a route is in the disableScrollToTop list, the function returns undefined, which may not be the intended behavior for maintaining current scroll position.
-const defaultScrollBehavior = async (to): Promise<false | any | { left: number, top: number }> => {
+const defaultScrollBehavior = async (to: any): Promise<false | { left: number, top: number }> => {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/router.options.ts at line 17, the function defaultScrollBehavior uses
'any' in its return type, reducing type safety, and returns undefined for routes
in disableScrollToTop, which may cause unintended scroll behavior. Update the
return type to explicitly include 'false' or the scroll position object,
removing 'any', and ensure the function returns 'false' instead of undefined
when the route is in disableScrollToTop to maintain current scroll position.
| const findEl = async (hash, x = 0) => { | ||
| return ( | ||
| document.querySelector(hash) | ||
| || new Promise((resolve) => { | ||
| if (x > 50) { | ||
| return resolve(document.querySelector('#app')) | ||
| } | ||
| setTimeout(() => { | ||
| resolve(findEl(hash, ++x || 1)) | ||
| }, 100) | ||
| }) | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the retry logic and improve error handling.
The function has several areas for improvement:
- The
++x || 1logic is confusing and redundant sincexdefaults to 0 - No error handling if
document.querySelector('#app')returns null - The recursive Promise pattern could be simplified
-const findEl = async (hash, x = 0) => {
+const findEl = async (hash: string, attempts = 0): Promise<Element | null> => {
return (
document.querySelector(hash)
|| new Promise((resolve) => {
- if (x > 50) {
- return resolve(document.querySelector('#app'))
+ if (attempts >= 50) {
+ return resolve(document.querySelector('#app') || null)
}
setTimeout(() => {
- resolve(findEl(hash, ++x || 1))
+ resolve(findEl(hash, attempts + 1))
}, 100)
})
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const findEl = async (hash, x = 0) => { | |
| return ( | |
| document.querySelector(hash) | |
| || new Promise((resolve) => { | |
| if (x > 50) { | |
| return resolve(document.querySelector('#app')) | |
| } | |
| setTimeout(() => { | |
| resolve(findEl(hash, ++x || 1)) | |
| }, 100) | |
| }) | |
| ) | |
| } | |
| const findEl = async (hash: string, attempts = 0): Promise<Element | null> => { | |
| return ( | |
| document.querySelector(hash) | |
| || new Promise((resolve) => { | |
| if (attempts >= 50) { | |
| return resolve(document.querySelector('#app') || null) | |
| } | |
| setTimeout(() => { | |
| resolve(findEl(hash, attempts + 1)) | |
| }, 100) | |
| }) | |
| ) | |
| } |
🤖 Prompt for AI Agents
In app/router.options.ts lines 3 to 15, simplify the retry logic by removing the
confusing '++x || 1' and just increment x directly. Add error handling to check
if 'document.querySelector('#app')' returns null and handle that case
appropriately, such as rejecting the promise or returning a fallback element.
Refactor the recursive Promise pattern to a clearer loop or a simpler async
retry mechanism to improve readability and maintainability.
|
Summary by CodeRabbit