Skip to content
This repository was archived by the owner on Dec 5, 2025. It is now read-only.

🔧 router scrollBehavior #11572

Open
roiLeo wants to merge 6 commits into
base: main
Choose a base branch
from
Open

🔧 router scrollBehavior #11572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
daddb46
🔧 router scrollBehavior
roiLeo May 28, 2025
052b519
Merge branch 'main' into fix/router/scrollBehavior
roiLeo May 30, 2025
db24a93
Merge branch 'main' into fix/router/scrollBehavior
roiLeo Jun 9, 2025
0c321ee
Merge branch 'main' into fix/router/scrollBehavior
roiLeo Jun 12, 2025
eeedbc3
Merge branch 'main' into fix/router/scrollBehavior
roiLeo Jun 23, 2025
d3539e7
Merge branch 'main' into fix/router/scrollBehavior
roiLeo Jun 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions app/router.options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import type { RouterConfig } from '@nuxt/schema'

const findEl = async (hash, x = 0) => {

Copilot AI May 28, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding explicit type annotations for the 'hash' parameter and for the function's return type to enhance clarity and maintainability.

Suggested change
const findEl = async (hash, x = 0) => {
const findEl = async (hash: string, x = 0): Promise<Element | null> => {

Copilot uses AI. Check for mistakes.
return (
document.querySelector(hash)
|| new Promise((resolve) => {
if (x > 50) {
return resolve(document.querySelector('#app'))
}
setTimeout(() => {
resolve(findEl(hash, ++x || 1))
}, 100)
})
)
}
Comment on lines +3 to +15

@coderabbitai coderabbitai Bot May 28, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify the retry logic and improve error handling.

The function has several areas for improvement:

  1. The ++x || 1 logic is confusing and redundant since x defaults to 0
  2. No error handling if document.querySelector('#app') returns null
  3. 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.

Suggested change
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.


const defaultScrollBehavior = async (to): Promise<false | any | { left: number, top: number }> => {

Copilot AI May 28, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider defining a specific type for the 'to' parameter rather than using an implicit type to improve type safety and code clarity.

Suggested change
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 coderabbitai Bot May 28, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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 toPath = to.name
const disableScrollToTop = [
'prefix-collection-id',
'prefix-collection-id-activity',
'prefix-collection-id-offers',
'prefix-collection-id-swaps',
'prefix-explore-items',
'prefix-explore-collectibles',
'prefix-u-id',
'prefix-transfer',
]

if (disableScrollToTop.includes(toPath)) {
return
}

if (to.hash) {
const el = await findEl(to.hash)
if (el) {
const top = el.offsetTop
// Perform scroll as a side effect
if ('scrollBehavior' in document.documentElement.style) {
window.scrollTo({ top, behavior: 'smooth' })
}
else {
window.scrollTo(0, top)
}
// Always return the correct type, not the result of window.scrollTo
return false
}
}

return { left: 0, top: 0 }
}

export default {
scrollBehavior: defaultScrollBehavior,
scrollBehaviorType: 'smooth',
} satisfies RouterConfig
43 changes: 0 additions & 43 deletions app/router.scrollBehavior.js
View file
Open in desktop

This file was deleted.

Loading