-
Notifications
You must be signed in to change notification settings - Fork 106
feat: add sidebar scroll position persistence #75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
70776cb
ced00da
305c7d0
93020dd
8e57f94
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| import SideBar from '@node-core/ui-components/Containers/Sidebar'; | ||
| import { sidebar } from '../../site.json' with { type: 'json' }; | ||
| import { useRef, useLayoutEffect } from 'react'; | ||
| import useScrollToElement from '../../hooks/useScrollToElement'; | ||
|
|
||
| /** @param {string} url */ | ||
| const redirect = url => (window.location.href = url); | ||
|
|
@@ -9,12 +11,26 @@ const PrefetchLink = props => <a {...props} rel="prefetch" />; | |
| /** | ||
| * Sidebar component for MDX documentation with page navigation | ||
| */ | ||
| export default ({ metadata }) => ( | ||
| <SideBar | ||
| pathname={`/learn${metadata.path.replace('/index', '')}`} | ||
| groups={sidebar} | ||
| onSelect={redirect} | ||
| as={PrefetchLink} | ||
| title="Navigation" | ||
| /> | ||
| ); | ||
| export default ({ metadata }) => { | ||
| const sidebarRef = useRef(null); | ||
|
|
||
| // SideBar from @node-core/ui-components does not support forwardRef, | ||
| // so ref={sidebarRef} is silently ignored. useLayoutEffect runs before | ||
| // useEffect, so by the time useScroll's effect attaches the scroll | ||
| // listener, sidebarRef.current already points to the real <aside> element. | ||
| useLayoutEffect(() => { | ||
| sidebarRef.current = document.querySelector('aside'); | ||
| }, []); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wrong aside targeted globallyMedium Severity Scroll persistence binds to Reviewed by Cursor Bugbot for commit 8e57f94. Configure here. |
||
|
|
||
| useScrollToElement('sidebar', sidebarRef); | ||
|
|
||
| return ( | ||
| <SideBar | ||
| pathname={`/learn${metadata.path.replace('/index', '')}`} | ||
| groups={sidebar} | ||
| onSelect={redirect} | ||
| as={PrefetchLink} | ||
| title="Navigation" | ||
| /> | ||
| ); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import { useEffect, useRef } from 'react'; | ||
|
|
||
| // Custom hook to handle scroll events with optional debouncing | ||
| const useScroll = (ref, { debounceTime = 300, onScroll }) => { | ||
| const timeoutRef = useRef(undefined); | ||
| const onScrollRef = useRef(onScroll); | ||
|
|
||
| // Keep onScrollRef updated with the latest callback | ||
| useEffect(() => { | ||
| onScrollRef.current = onScroll; | ||
| }, [onScroll]); | ||
| useEffect(() => { | ||
| // Get the current element | ||
| const element = ref.current; | ||
|
|
||
| // Return early if no element or onScroll callback is provided | ||
| if (!element || !onScrollRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| // Debounced scroll handler | ||
| const handleScroll = () => { | ||
| // Clear existing timeout | ||
| if (timeoutRef.current) { | ||
| clearTimeout(timeoutRef.current); | ||
| } | ||
|
|
||
| // Set new timeout to call onScroll after debounceTime | ||
| timeoutRef.current = setTimeout(() => { | ||
| if (element && onScrollRef.current) { | ||
| onScrollRef.current({ | ||
| x: element.scrollLeft, | ||
| y: element.scrollTop, | ||
| }); | ||
| } | ||
| }, debounceTime); | ||
| }; | ||
|
|
||
| element.addEventListener('scroll', handleScroll, { passive: true }); | ||
|
|
||
| return () => { | ||
| element.removeEventListener('scroll', handleScroll); | ||
| // Clear any pending debounced calls | ||
| if (timeoutRef.current) { | ||
| clearTimeout(timeoutRef.current); | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Debounced scroll not saved on exitMedium Severity When the scroll listener unmounts, cleanup clears the debounce timer without persisting the latest Reviewed by Cursor Bugbot for commit 8e57f94. Configure here. |
||
| }; | ||
| }, [debounceTime]); | ||
| }; | ||
|
|
||
| export default useScroll; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| import { useContext, useEffect } from 'react'; | ||
|
|
||
| import { NavigationStateContext } from '../providers/navigationStateProvider'; | ||
|
|
||
| import useScroll from './useScroll'; | ||
|
|
||
| const useScrollToElement = (id, ref, debounceTime = 300) => { | ||
| const navigationState = useContext(NavigationStateContext); | ||
|
|
||
| // Restore scroll position on mount | ||
| useEffect(() => { | ||
| const element = ref.current; | ||
| if (!element) { | ||
| return; | ||
| } | ||
|
|
||
| // Prefer in-memory context state (set during same session/SPA navigation). | ||
| // Fall back to localStorage so position is restored after a full page refresh. | ||
| let savedState = navigationState[id]; | ||
|
|
||
| if (!savedState) { | ||
| try { | ||
| const raw = localStorage.getItem(`navigationState:${id}`); | ||
| if (raw) { | ||
| savedState = JSON.parse(raw); | ||
| // Hydrate context so it's available for the rest of the session | ||
| navigationState[id] = savedState; | ||
| } | ||
| } catch { | ||
| localStorage.removeItem(`navigationState:${id}`); | ||
| } | ||
| } | ||
|
|
||
| // Scroll only if the saved position differs from current | ||
| if (savedState && savedState.y !== element.scrollTop) { | ||
| element.scroll({ top: savedState.y, behavior: 'auto' }); | ||
| } | ||
| }, [id]); | ||
|
|
||
| // Save scroll position on scroll | ||
| const handleScroll = position => { | ||
| try { | ||
| localStorage.setItem(`navigationState:${id}`, JSON.stringify(position)); | ||
| } catch { | ||
| // localStorage may be unavailable (e.g. Safari private browsing) | ||
| // or the quota may be exceeded — fall through so in-memory state | ||
| // is still updated below. | ||
| } | ||
| // Always update in-memory state regardless of localStorage availability | ||
| navigationState[id] = position; | ||
| }; | ||
|
cursor[bot] marked this conversation as resolved.
|
||
|
|
||
| // Use the useScroll hook to handle scroll events with debouncing | ||
| useScroll(ref, { debounceTime, onScroll: handleScroll }); | ||
| }; | ||
|
|
||
| export default useScrollToElement; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| 'use client'; | ||
|
|
||
| import { createContext, useRef } from 'react'; | ||
|
|
||
| export const NavigationStateContext = createContext({}); | ||
|
|
||
| export const NavigationStateProvider = ({children}) => { | ||
| const navigationStateRef = useRef({}); | ||
|
|
||
| return ( | ||
| <NavigationStateContext.Provider value={navigationStateRef.current}> | ||
| {children} | ||
| </NavigationStateContext.Provider> | ||
| ); | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| }; | ||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Announcement banner removed accidentally
Medium Severity
The layout change drops
RemoteLoadableBannerand its import while the PR only describes sidebar scroll persistence. Learn pages no longer render the remote announcement banner that previously appeared site-wide.Reviewed by Cursor Bugbot for commit 8e57f94. Configure here.