feat(landing): improve navigation between pages with hash routing and…#29
Conversation
… scroll handling - Fix AppNavbar anchor navigation to detect current route before scrolling — when on a non-landing page (e.g. /contributing), navigate to the landing page with the hash first instead of trying to find non-existent DOM elements - Add scroll-to-hash handler in LandingPage onMount so that navigating from another page with a hash (e.g. /#hero) smoothly scrolls to the target section after the page renders - Add scrollBehavior to vue-router to automatically scroll to top on every page navigation, handle hash-based scrolling, and restore scroll position on browser back/forward
📝 WalkthroughWalkthroughHash-based smooth scrolling is added across three files: ChangesHash-based smooth scrolling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
landing/src/views/LandingPage.vue (1)
15-23: ⚡ Quick winReplace magic timeout with DOM-ready mechanism.
The 100ms timeout is arbitrary and may be unreliable—too short on slower devices where rendering might take longer, or unnecessarily long on fast devices. Consider using
nextTickorrequestAnimationFrameto ensure the DOM is ready, or implement a retry mechanism that checks for element existence.♻️ Alternative approach using nextTick
+import { onMounted, nextTick } from 'vue' -import { onMounted } from 'vue' import { useRoute } from 'vue-router' const route = useRoute() onMounted(() => { if (route.hash) { const id = route.hash.slice(1) - setTimeout(() => { + nextTick(() => { const el = document.getElementById(id) if (el) el.scrollIntoView({ behavior: 'smooth', block: 'start' }) - }, 100) + }) } })Note: If
nextTickproves insufficient, consider a polling approach that checks for element existence with a maximum retry limit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@landing/src/views/LandingPage.vue` around lines 15 - 23, The arbitrary 100ms setTimeout in the onMounted hook is unreliable across different device speeds and may cause the element scroll to fail if rendering takes longer. Replace the setTimeout approach with Vue's nextTick function, which guarantees the DOM is ready before executing the scroll logic. Import nextTick from Vue at the top of the file, then wrap the document.getElementById and scrollIntoView logic inside a nextTick callback instead of using the hardcoded timeout. This ensures the element lookup and smooth scroll behavior execute once the DOM updates are complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@landing/src/router/index.ts`:
- Around line 19-27: The scrollBehavior function in the router and the onMounted
hook in LandingPage.vue are both attempting to scroll to hash targets, causing
duplicate scroll operations. To fix this, remove the hash handling logic from
the scrollBehavior function (the condition checking to.hash and returning { el:
to.hash, behavior: 'smooth' }) and let the onMounted hook in LandingPage.vue
handle all scrolling to hash elements. This ensures a single, consistent scroll
mechanism at the component level with proper DOM-ready timing, eliminating race
conditions and jarring double-scroll behavior.
---
Nitpick comments:
In `@landing/src/views/LandingPage.vue`:
- Around line 15-23: The arbitrary 100ms setTimeout in the onMounted hook is
unreliable across different device speeds and may cause the element scroll to
fail if rendering takes longer. Replace the setTimeout approach with Vue's
nextTick function, which guarantees the DOM is ready before executing the scroll
logic. Import nextTick from Vue at the top of the file, then wrap the
document.getElementById and scrollIntoView logic inside a nextTick callback
instead of using the hardcoded timeout. This ensures the element lookup and
smooth scroll behavior execute once the DOM updates are complete.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6a054fd-ae7f-4bf5-8056-ebea93d54c21
📒 Files selected for processing (3)
landing/src/components/AppNavbar.vuelanding/src/router/index.tslanding/src/views/LandingPage.vue
| scrollBehavior(to, _from, savedPosition) { | ||
| if (to.hash) { | ||
| return { el: to.hash, behavior: 'smooth' } | ||
| } | ||
| if (savedPosition) { | ||
| return savedPosition | ||
| } | ||
| return { top: 0 } | ||
| }, |
There was a problem hiding this comment.
Potential duplicate scrolling with LandingPage onMounted hook.
When navigating to the landing page with a hash (e.g., from /contributing to /#hero), both this scrollBehavior function and the onMounted hook in LandingPage.vue (lines 15-23) will attempt to scroll to the same target element. The router's scrollBehavior executes during navigation, then the component's onMounted waits 100ms and scrolls again.
Consider one of these approaches:
- Remove hash handling from
scrollBehaviorand rely solely on the component-levelonMountedhook for consistency - Remove the
onMountedhook fromLandingPage.vueand handle DOM-ready timing inscrollBehavior(though Vue Router provides limited delay options) - Add a flag to coordinate between the two layers to prevent double scrolling
The current implementation may cause jarring double-scroll behavior or race conditions depending on render timing.
Related files: landing/src/views/LandingPage.vue:15-23, landing/src/components/AppNavbar.vue:27-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@landing/src/router/index.ts` around lines 19 - 27, The scrollBehavior
function in the router and the onMounted hook in LandingPage.vue are both
attempting to scroll to hash targets, causing duplicate scroll operations. To
fix this, remove the hash handling logic from the scrollBehavior function (the
condition checking to.hash and returning { el: to.hash, behavior: 'smooth' })
and let the onMounted hook in LandingPage.vue handle all scrolling to hash
elements. This ensures a single, consistent scroll mechanism at the component
level with proper DOM-ready timing, eliminating race conditions and jarring
double-scroll behavior.
2 participants
… scroll handling
Summary by CodeRabbit