fix: restore scroll position when navigating back to docs list #13325
Conversation
WalkthroughThe changes implement scroll position persistence for the document list in the DocsExplorer component. A new hook ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DocsListComponent
participant ScrollStateModule
participant useScrollRestoration
User->>DocsListComponent: Mounts Docs List
DocsListComponent->>useScrollRestoration: Initialize with ref and key
useScrollRestoration->>ScrollStateModule: Retrieve scroll position
ScrollStateModule-->>useScrollRestoration: Return last scroll position
useScrollRestoration->>DocsListComponent: Restore scroll position
User->>DocsListComponent: Navigates away
DocsListComponent->>useScrollRestoration: Cleanup
useScrollRestoration->>ScrollStateModule: Save current scroll position
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/frontend/core/src/modules/doc/stores/ui-state.ts (1)
3-7: Consider session persistence for better user experience.The current module-level variable approach doesn't persist scroll position across page reloads or browser sessions. Users would lose their scroll position if they refresh the page.
Consider using
sessionStorageorlocalStoragefor persistence, similar to theuseScrollRestorationhook implementation.-let lastScrollPosition = 0; +const STORAGE_KEY = 'docs-list-scroll-position'; -export const getDocsListScrollPosition = () => lastScrollPosition; +export const getDocsListScrollPosition = () => { + const stored = sessionStorage.getItem(STORAGE_KEY); + return stored ? parseInt(stored, 10) : 0; +}; -export const setDocsListScrollPosition = (pos: number) => { - lastScrollPosition = pos; -}; +export const setDocsListScrollPosition = (pos: number) => { + sessionStorage.setItem(STORAGE_KEY, pos.toString()); +};packages/frontend/core/src/hooks/useScrollRestoration.ts (1)
4-23: Consider using this hook instead of the current implementation.This
useScrollRestorationhook provides a more robust solution with session persistence compared to the current module-level approach used indocs-list.tsx. However, it's not being utilized in the codebase.Consider integrating this hook in the
DocsExplorercomponent to replace the current scroll restoration logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/frontend/core/src/components/explorer/docs-view/docs-list.tsx(10 hunks)packages/frontend/core/src/hooks/useScrollRestoration.ts(1 hunks)packages/frontend/core/src/modules/doc/stores/ui-state.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/frontend/core/src/components/explorer/docs-view/docs-list.tsx (1)
Learnt from: EYHN
PR: #12320
File: packages/frontend/core/src/desktop/dialogs/collection-editor/rules-mode.tsx:366-366
Timestamp: 2025-05-22T02:37:07.462Z
Learning: In React components, avoid redundant translation calls. When using services like docDisplayMetaService.title$ that already provide localized strings, don't wrap the result in another translation call like i18n.t() as it's unnecessary and may break interpolation if the string contains special markers.
🔇 Additional comments (10)
packages/frontend/core/src/hooks/useScrollRestoration.ts (1)
18-21: Remove redundant cleanup call in useEffect return.The
handleBeforeUnload()call in the cleanup function is redundant since the same function is already registered as abeforeunloadevent listener. This could cause the scroll position to be saved twice.window.addEventListener('beforeunload', handleBeforeUnload); return () => { - handleBeforeUnload(); window.removeEventListener('beforeunload', handleBeforeUnload); };Likely an incorrect or invalid review comment.
packages/frontend/core/src/components/explorer/docs-view/docs-list.tsx (9)
12-12: LGTM!The addition of
useRefimport is necessary for the scroll container reference implementation.
72-72: Good code simplification.Removing unnecessary braces improves readability while maintaining the same logic.
79-79: Good defensive programming practice.Adding the null check for
idprevents potential runtime errors whencharCodeAt(0)is called on undefined/null values.
130-130: Minor code style improvement noted.Simplifying the parameter name from
(group: any)improves readability, though theanytype could be more specific if the group type is available.
164-169: Code style improvements noted.The removal of unnecessary braces and early returns make the code more concise and readable.
177-179: Template string formatting improvement.The multiline template string formatting improves readability.
186-190: Variable extraction improves code clarity.Extracting
selectedvariable and simplifying the loop variable name enhances readability.
205-208: Consistent variable extraction pattern.The same variable extraction pattern is applied consistently, improving code maintainability.
251-254: Scroll container implementation is correct.The div wrapper with scroll properties correctly enables the scroll restoration functionality.
| const scrollContainerRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| // ✅ Scroll restore logic | ||
| useEffect(() => { | ||
| const container = scrollContainerRef.current; | ||
| if (container) { | ||
| container.scrollTop = getDocsListScrollPosition(); | ||
| } | ||
| return () => { | ||
| if (container) { | ||
| setDocsListScrollPosition(container.scrollTop); | ||
| } | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Scroll restoration implementation needs architectural review.
The scroll restoration logic is functionally correct but has some concerns:
- Code duplication: This duplicates the exact same functionality as
ui-state.ts - Memory persistence: Uses module-level state that doesn't persist across page reloads
- Unused hook: The
useScrollRestorationhook provides better functionality but isn't used
Consider using the useScrollRestoration hook instead:
- const scrollContainerRef = useRef<HTMLDivElement>(null);
-
- // ✅ Scroll restore logic
- useEffect(() => {
- const container = scrollContainerRef.current;
- if (container) {
- container.scrollTop = getDocsListScrollPosition();
- }
- return () => {
- if (container) {
- setDocsListScrollPosition(container.scrollTop);
- }
- };
- }, []);
+ const scrollContainerRef = useRef<HTMLDivElement>(null);
+ useScrollRestoration('docs-list', scrollContainerRef);📝 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 scrollContainerRef = useRef<HTMLDivElement>(null); | |
| // ✅ Scroll restore logic | |
| useEffect(() => { | |
| const container = scrollContainerRef.current; | |
| if (container) { | |
| container.scrollTop = getDocsListScrollPosition(); | |
| } | |
| return () => { | |
| if (container) { | |
| setDocsListScrollPosition(container.scrollTop); | |
| } | |
| }; | |
| }, []); | |
| const scrollContainerRef = useRef<HTMLDivElement>(null); | |
| useScrollRestoration('docs-list', scrollContainerRef); |
🤖 Prompt for AI Agents
In packages/frontend/core/src/components/explorer/docs-view/docs-list.tsx around
lines 231 to 244, the current scroll restoration logic duplicates functionality
found in ui-state.ts, uses non-persistent module-level state, and does not
leverage the existing useScrollRestoration hook. To fix this, remove the manual
scroll restoration code and replace it by importing and using the
useScrollRestoration hook, which provides improved and persistent scroll
restoration behavior across page reloads.
| let lastScrollPosition = 0; | ||
|
|
||
| export const getDocsListScrollPosition = () => lastScrollPosition; | ||
|
|
||
| export const setDocsListScrollPosition = (pos: number) => { | ||
| lastScrollPosition = pos; | ||
| }; |
There was a problem hiding this comment.
Remove duplicated scroll state management.
This code duplicates the exact same functionality as the ui-state.ts module, creating maintenance overhead and potential inconsistencies.
Since ui-state.ts already provides these functions, remove this duplication:
-// ✅ Scroll state management
-let lastScrollPosition = 0;
-
-export const getDocsListScrollPosition = () => lastScrollPosition;
-
-export const setDocsListScrollPosition = (pos: number) => {
- lastScrollPosition = pos;
-};And import from the centralized module instead:
+import { getDocsListScrollPosition, setDocsListScrollPosition } from '@affine/core/modules/doc/stores/ui-state';📝 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.
| let lastScrollPosition = 0; | |
| export const getDocsListScrollPosition = () => lastScrollPosition; | |
| export const setDocsListScrollPosition = (pos: number) => { | |
| lastScrollPosition = pos; | |
| }; | |
| import { getDocsListScrollPosition, setDocsListScrollPosition } from '@affine/core/modules/doc/stores/ui-state'; |
🤖 Prompt for AI Agents
In packages/frontend/core/src/components/explorer/docs-view/docs-list.tsx around
lines 295 to 301, remove the duplicated scroll state management functions
getDocsListScrollPosition and setDocsListScrollPosition. Instead, import these
functions from the existing centralized ui-state.ts module to avoid redundancy
and maintain consistency.
| const savedY = sessionStorage.getItem(`scroll-${key}`); | ||
| if (ref.current && savedY) { | ||
| ref.current.scrollTo(0, parseInt(savedY, 10)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and input validation for sessionStorage operations.
The current implementation doesn't handle potential sessionStorage errors or validate the parsed integer value, which could cause runtime issues.
useEffect(() => {
- const savedY = sessionStorage.getItem(`scroll-${key}`);
- if (ref.current && savedY) {
- ref.current.scrollTo(0, parseInt(savedY, 10));
- }
+ try {
+ const savedY = sessionStorage.getItem(`scroll-${key}`);
+ if (ref.current && savedY) {
+ const scrollPosition = parseInt(savedY, 10);
+ if (!isNaN(scrollPosition) && scrollPosition >= 0) {
+ ref.current.scrollTo(0, scrollPosition);
+ }
+ }
+ } catch (error) {
+ console.warn('Failed to restore scroll position:', error);
+ }🤖 Prompt for AI Agents
In packages/frontend/core/src/hooks/useScrollRestoration.ts around lines 6 to 9,
add try-catch blocks around the sessionStorage.getItem call to handle potential
errors accessing sessionStorage. Also, validate that the parsed integer from
savedY is a valid number before calling scrollTo, and only perform the scroll if
the value is a valid integer to prevent runtime issues.
| const handleBeforeUnload = () => { | ||
| if (ref.current) { | ||
| sessionStorage.setItem(`scroll-${key}`, ref.current.scrollTop.toString()); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for sessionStorage write operations.
Similar to the read operation, the write operation should handle potential sessionStorage errors.
const handleBeforeUnload = () => {
if (ref.current) {
- sessionStorage.setItem(`scroll-${key}`, ref.current.scrollTop.toString());
+ try {
+ sessionStorage.setItem(`scroll-${key}`, ref.current.scrollTop.toString());
+ } catch (error) {
+ console.warn('Failed to save 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.
| const handleBeforeUnload = () => { | |
| if (ref.current) { | |
| sessionStorage.setItem(`scroll-${key}`, ref.current.scrollTop.toString()); | |
| } | |
| }; | |
| const handleBeforeUnload = () => { | |
| if (ref.current) { | |
| try { | |
| sessionStorage.setItem(`scroll-${key}`, ref.current.scrollTop.toString()); | |
| } catch (error) { | |
| console.warn('Failed to save scroll position:', error); | |
| } | |
| } | |
| }; |
🤖 Prompt for AI Agents
In packages/frontend/core/src/hooks/useScrollRestoration.ts around lines 11 to
15, the sessionStorage.setItem call inside handleBeforeUnload lacks error
handling. Wrap the sessionStorage.setItem call in a try-catch block to catch and
handle any exceptions that may occur during the write operation, ensuring the
app does not crash and errors can be logged or handled gracefully.
| let lastScrollPosition = 0; | ||
|
|
||
| export const getDocsListScrollPosition = () => lastScrollPosition; | ||
|
|
||
| export const setDocsListScrollPosition = (pos: number) => { | ||
| lastScrollPosition = pos; | ||
| }; |
There was a problem hiding this comment.
Code duplication detected with docs-list.tsx implementation.
This module provides identical functionality to the scroll state management implemented directly in docs-list.tsx (lines 295-301). Having two separate implementations for the same purpose creates maintenance overhead and potential inconsistencies.
Consider consolidating to a single approach - either use this centralized module consistently or remove it in favor of the component-level implementation.
🤖 Prompt for AI Agents
In packages/frontend/core/src/modules/doc/stores/ui-state.ts lines 1 to 7, the
scroll position state management duplicates the implementation found in
docs-list.tsx lines 295 to 301. To fix this, choose one implementation to keep
and remove the other to avoid redundancy. If you keep this module, refactor
docs-list.tsx to use these exported functions for scroll position management;
otherwise, delete this module and rely solely on the component-level code.
| const scrollContainerRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| // ✅ Scroll restore logic | ||
| useEffect(() => { | ||
| const container = scrollContainerRef.current; | ||
| if (container) { | ||
| container.scrollTop = getDocsListScrollPosition(); | ||
| } | ||
| return () => { | ||
| if (container) { | ||
| setDocsListScrollPosition(container.scrollTop); | ||
| } | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
should use useScrollRestoration
2 participants
🛠️ What does this PR do?
This PR implements scroll position restoration in the document list. When a user opens a document and then returns to the list, the scroll position remains the same instead of jumping to the top.
📂 Files Changed
useScrollRestoration.ts: Created a custom hook to manage scroll position.ui-state.ts: Added state handling for scroll position.✅ How to Test
📌 Related Issue
Fixes #13314
Summary by CodeRabbit