feat(docs): preserve scrolling position when navigating to new page #4294
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
apps/docs/components/docs/sidebar.tsx (1)
244-244: Consider extracting scroll position logic to a custom hookThe scroll position preservation logic could be reused in other components. Consider extracting it to a custom hook for better maintainability and reusability.
function useScrollPositionPersistence(key: string) { const scrollViewPortRef = useRef<HTMLDivElement>(null); useLayoutEffect(() => { if (typeof window !== "undefined") { try { const savedPosition = sessionStorage.getItem(key); if (savedPosition && scrollViewPortRef.current) { scrollViewPortRef.current.scrollTop = Number(savedPosition); } } catch (error) { console.warn("Failed to restore scroll position:", error); } } }, [key]); const handleScroll = debounce(() => { if (typeof window !== "undefined" && scrollViewPortRef.current) { try { sessionStorage.setItem(key, scrollViewPortRef.current.scrollTop.toString()); } catch (error) { console.warn("Failed to save scroll position:", error); } } }, 300); return { scrollViewPortRef, handleScroll }; }Also applies to: 284-285
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/docs/components/docs/sidebar.tsx(4 hunks)apps/docs/components/scroll-area.tsx(1 hunks)
🔇 Additional comments (2)
apps/docs/components/scroll-area.tsx (1)
21-25: Verify ref forwarding chain
The component now handles two refs: one forwarded to Root and another for Viewport. Ensure this doesn't cause any ref-related issues in parent components.
✅ Verification successful
Ref forwarding implementation is correct and safe
The ScrollArea component's implementation shows proper ref handling:
- The main
refis correctly forwarded toScrollAreaPrimitive.Root - The
scrollViewPortRefis passed as a prop and properly applied toScrollAreaPrimitive.Viewport - Both refs have distinct purposes and don't interfere with each other
- The component's type definitions properly declare the ref types and props
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components using ScrollArea with refs
ast-grep --pattern 'const $_ = useRef<$_>();
$$$
<ScrollArea ref={$_}'
Length of output: 72
Script:
#!/bin/bash
# Let's try a different pattern to find ScrollArea usage with refs
ast-grep --pattern '<ScrollArea ref={$_}'
# Also search for the component definition to understand ref handling
ast-grep --pattern 'const ScrollArea = React.forwardRef<$_>'
# And check direct usage of ScrollAreaPrimitive
ast-grep --pattern '<ScrollAreaPrimitive.Root ref={$_}'
Length of output: 164
Script:
#!/bin/bash
# Let's examine the ScrollArea component implementation and its usage
rg -l "ScrollArea" --type tsx
# Then let's look at the content of the scroll-area.tsx file
cat apps/docs/components/scroll-area.tsx
Length of output: 2071
Script:
#!/bin/bash
# Let's search for files using ScrollArea with .ts and .jsx extensions as well
rg -l "ScrollArea" --type ts --type jsx
# And let's try to find any usage of scrollViewPortRef
rg "scrollViewPortRef" --type ts --type tsx --type jsx
Length of output: 150
apps/docs/components/docs/sidebar.tsx (1)
342-342: Verify dependency array completeness
The treeContent memo's dependency array now includes slug and tag. Ensure this covers all dependencies that could affect the rendered content.
✅ Verification successful
Let me search for the expandedKeys variable to ensure it's not a dependency that should be included in the array.
Dependency array is correctly specified
The treeContent memo's dependency array [routes, slug, tag] is complete and accurate. All dependencies used within the memoized content are properly included:
routesis used for mapping and also forexpandedKeyscalculationslugandtagare passed as props to theItemcomponentexpandedKeysis derived fromroutesand doesn't need separate inclusion- Other values used within the memo are either component props spread via
{...route}or derived from therouteiteration variable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all props used within the treeContent
ast-grep --pattern 'const treeContent = useMemo(() => {
$$$
$PROP
$$$
}, $_)'
Length of output: 1222
Script:
#!/bin/bash
# Search for expandedKeys definition and usage
ast-grep --pattern 'const expandedKeys = $_'
Length of output: 510
apps/docs/components/scroll-area.tsx
Outdated
| React.ComponentPropsWithoutRef<typeof ScrollAreaPrimitive.Root> & { | ||
| scrollViewPortRef: React.RefObject<HTMLDivElement>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making scrollViewPortRef optional
Making scrollViewPortRef a required prop might break existing usages of the ScrollArea component. Consider making it optional to maintain backward compatibility.
- scrollViewPortRef: React.RefObject<HTMLDivElement>;
+ scrollViewPortRef?: React.RefObject<HTMLDivElement>;📝 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.
| React.ComponentPropsWithoutRef<typeof ScrollAreaPrimitive.Root> & { | |
| scrollViewPortRef: React.RefObject<HTMLDivElement>; | |
| } | |
| React.ComponentPropsWithoutRef<typeof ScrollAreaPrimitive.Root> & { | |
| scrollViewPortRef?: React.RefObject<HTMLDivElement>; | |
| } |
| /* Handle scroll preservation */ | ||
| useLayoutEffect(() => { | ||
| if (typeof window !== "undefined") { | ||
| const savedPosition = sessionStorage.getItem("docsSidebarScrollPosition"); | ||
|
|
||
| if (savedPosition && scrollViewPortRef.current) { | ||
| scrollViewPortRef.current.scrollTop = Number(savedPosition); | ||
| } | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Add error handling for sessionStorage operations
The sessionStorage operations should be wrapped in try-catch blocks to handle potential errors (e.g., when storage is full or disabled).
useLayoutEffect(() => {
if (typeof window !== "undefined") {
- const savedPosition = sessionStorage.getItem("docsSidebarScrollPosition");
- if (savedPosition && scrollViewPortRef.current) {
- scrollViewPortRef.current.scrollTop = Number(savedPosition);
+ try {
+ const savedPosition = sessionStorage.getItem("docsSidebarScrollPosition");
+ if (savedPosition && scrollViewPortRef.current) {
+ scrollViewPortRef.current.scrollTop = Number(savedPosition);
+ }
+ } catch (error) {
+ console.warn("Failed to restore scroll position:", error);
}
}
}, []);📝 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.
| /* Handle scroll preservation */ | |
| useLayoutEffect(() => { | |
| if (typeof window !== "undefined") { | |
| const savedPosition = sessionStorage.getItem("docsSidebarScrollPosition"); | |
| if (savedPosition && scrollViewPortRef.current) { | |
| scrollViewPortRef.current.scrollTop = Number(savedPosition); | |
| } | |
| } | |
| }, []); | |
| /* Handle scroll preservation */ | |
| useLayoutEffect(() => { | |
| if (typeof window !== "undefined") { | |
| try { | |
| const savedPosition = sessionStorage.getItem("docsSidebarScrollPosition"); | |
| if (savedPosition && scrollViewPortRef.current) { | |
| scrollViewPortRef.current.scrollTop = Number(savedPosition); | |
| } | |
| } catch (error) { | |
| console.warn("Failed to restore scroll position:", error); | |
| } | |
| } | |
| }, []); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/docs/components/scroll-area.tsx (1)
13-13: Props destructuring looks goodClean separation of scroll-related props from other props. The rename to
restPropsimproves code clarity.Consider adding TypeScript type annotations for better code documentation:
- const {onScroll, scrollViewPortRef, ...restProps} = props; + const {onScroll, scrollViewPortRef, ...restProps}: typeof props = props;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/docs/components/docs/sidebar.tsx(4 hunks)apps/docs/components/scroll-area.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/docs/components/docs/sidebar.tsx
🔇 Additional comments (2)
apps/docs/components/scroll-area.tsx (2)
9-11: LGTM: Type definitions are well-structured
The scrollViewPortRef prop is correctly defined as optional, maintaining backward compatibility with existing usages of the ScrollArea component.
19-25: Verify scroll event listener cleanup
The implementation looks correct, but we should verify that any scroll listeners are properly cleaned up to prevent memory leaks.
Let's check if there are any cleanup mechanisms in place:
jrgarciadev
deleted the
eng-1643-preserve-scrolling-when-navigating-docs
branch
Preview (Tested on Safari, Chrome, Firefox)
BEFORE
nextui.sidebar.before.mp4
AFTER (Chrome)
preserverscrolling.mp4
AFTER (Safari)
safari.preserve.scrolling.mp4
📝 Description
This PR introduces functionality to preserve the scroll position in the documentation sidebar when navigating between pages. This feature enhances user experience by ensuring the sidebar retains its position, preventing the need to scroll back to the previous section manually.
⛳️ Current behavior (updates)
Currently, the sidebar scroll position is reset to the top whenever a user navigates to a new page. This behavior can be disruptive, particularly when exploring deeply nested sections or returning to a previous part of the documentation.
🚀 New behavior
sessionStorage.💣 Is this a breaking change (Yes/No):
No.
📝 Additional Information
useLayoutEffecthook to ensure that the scroll position is restored after the sidebar is rendered.ScrollAreaandTreecomponents to accommodate the scroll preservation logic.Summary by CodeRabbit
New Features
ScrollAreacomponent to manage viewport reference and handle scroll events effectively.Bug Fixes
Documentation