Add option to toggle Scroll Sync On/Off #394
Conversation
WalkthroughThis pull request introduces several changes across multiple components in the frontend of the application. The Changes
Possibly related PRs
Suggested labels
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
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: 4
🧹 Outside diff range and nitpick comments (2)
frontend/src/store/configSlice.ts (1)
20-20: LGTM! Consider the property naming.The boolean property is appropriately typed for a toggle feature. While the negative state naming (
disableScrollSyncvsenableScrollSync) is acceptable, it might require careful attention when implementing the toggle logic to avoid double negatives.frontend/src/components/headers/DocumentHeader.tsx (1)
46-46: Consider adding type safety enhancement.The state and handlers are well-implemented following React and Material-UI patterns.
Consider using a more specific type for the anchor element state:
- const [moreButtonanchorEl, setMoreButtonAnchorEl] = useState<HTMLButtonElement | null>(null); + const [moreButtonanchorEl, setMoreButtonAnchorEl] = useState<HTMLElement | null>(null);This makes it more flexible if you decide to change the anchor element type in the future.
Also applies to: 88-94
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
frontend/src/components/editor/DocumentView.tsx(2 hunks)frontend/src/components/headers/DocumentHeader.tsx(5 hunks)frontend/src/components/popovers/DocumentPopover.tsx(1 hunks)frontend/src/store/configSlice.ts(2 hunks)
🔇 Additional comments (10)
frontend/src/store/configSlice.ts (4)
27-27: LGTM! Default value maintains backward compatibility.
Setting disableScrollSync to false by default ensures that existing users maintain their current scroll sync behavior, which is a good practice for introducing toggleable features.
43-45: LGTM! Clean and type-safe implementation.
The reducer follows Redux best practices with proper typing and a straightforward implementation appropriate for a toggle feature.
49-50: LGTM! Export statement properly includes the new action.
The new action is correctly exported alongside existing actions, maintaining consistency with the file's structure.
Line range hint 20-50: Verify the configuration usage in components.
The Redux configuration changes look good, but let's verify that components are properly consuming this configuration.
✅ Verification successful
Configuration is properly integrated and consumed by components
The verification shows proper implementation and usage of the disableScrollSync configuration:
DocumentPopover.tsxcorrectly toggles the setting via Redux dispatchDocumentView.tsxproperly consumes the configuration to enable/disable scroll sync functionality- The implementation follows Redux patterns and maintains consistency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify proper usage of the scroll sync configuration in components
# Search for usages of the configuration selector
echo "Checking for configuration selector usage:"
rg -l "selectConfig|useSelector.*config" frontend/src/
# Search for direct references to disableScrollSync
echo -e "\nChecking for disableScrollSync references:"
rg -l "disableScrollSync" frontend/src/
Length of output: 766
Script:
#!/bin/bash
# Let's examine how disableScrollSync is being used in the components
echo "Checking DocumentPopover.tsx implementation:"
rg "disableScrollSync" frontend/src/components/popovers/DocumentPopover.tsx -B 2 -A 2
echo -e "\nChecking DocumentView.tsx implementation:"
rg "disableScrollSync" frontend/src/components/editor/DocumentView.tsx -B 2 -A 2
# Let's also verify the selector pattern being used
echo -e "\nChecking selector usage pattern:"
rg "useSelector.*config" frontend/src/ -B 1 -A 1
Length of output: 962
frontend/src/components/popovers/DocumentPopover.tsx (2)
17-21: LGTM! Clean and proper hooks setup.
The component is well-structured with proper typing and hook usage.
1-64: Verify Redux store integration.
The component implements the scroll sync toggle UI as intended. Let's verify the Redux store integration:
✅ Verification successful
Redux store integration is properly implemented
The verification confirms:
- The Redux store correctly implements
disableScrollSyncstate inconfigSlice.tswith proper initialization and reducer - The state is correctly consumed in
DocumentView.tsxto control theScrollSynccomponent - The toggle in
DocumentPopover.tsxproperly dispatches state changes through the Redux store
The implementation aligns with the PR objectives and maintains proper state management integration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if configSlice properly implements the scroll sync state
rg -l "disableScrollSync" --type ts
Length of output: 170
Script:
#!/bin/bash
# Check configSlice implementation
rg "disableScrollSync" frontend/src/store/configSlice.ts -A 5 -B 5
# Check how DocumentView uses the scroll sync state
rg "disableScrollSync" frontend/src/components/editor/DocumentView.tsx -A 5 -B 5
Length of output: 1154
frontend/src/components/editor/DocumentView.tsx (2)
9-14: LGTM: Clean store integration
The new configuration store integration follows Redux best practices and maintains consistency with existing patterns.
28-28: Implementation looks good, verify edge cases
The ScrollSync toggle implementation correctly uses the configuration state. However, please verify:
- Scroll position behavior when toggling sync while content is scrolled
- Multi-user experience when different users have different sync settings
Let's check for any existing scroll position handling code:
✅ Verification successful
ScrollSync implementation is robust and safe
The implementation is well-structured and handles scroll syncing appropriately:
- ScrollSync state is managed through Redux store with proper initialization (default: enabled)
- Toggle functionality is properly implemented in DocumentPopover
- ScrollSyncPane components are correctly wrapped around both editor and preview content
- The library
react-scroll-synchandles the scroll position management internally
The concerns raised in the review comment can be addressed:
- Scroll position during toggle: The ScrollSync component from react-scroll-sync handles this internally by design
- Multi-user experience: The sync setting is client-side only and doesn't affect other users' views
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for scroll position management code
rg -l 'scroll(Top|Left|Position|Into)' --type tsx --type ts
Length of output: 89
Script:
#!/bin/bash
# Search for scroll-related code with proper extension patterns
rg -l 'scroll(Top|Left|Position|Into)' -g '*.tsx' -g '*.ts'
# Also search for ScrollSync component usage and implementation
rg -C 3 'ScrollSync' -g '*.tsx' -g '*.ts'
Length of output: 7858
frontend/src/components/headers/DocumentHeader.tsx (2)
17-17: LGTM: Import statements are properly organized.
The new imports are correctly placed and all are utilized within the component.
Also applies to: 30-31
164-168: Verify scroll sync toggle implementation in DocumentPopover.
Let's ensure the DocumentPopover component properly implements the scroll sync toggle functionality as per PR objectives.
✅ Verification successful
DocumentPopover correctly implements scroll sync toggle functionality
The component properly implements the scroll sync toggle feature with:
- A menu item for Panel Scroll Sync with toggle button UI
- Correct state management using Redux (setDisableScrollSync)
- Visual feedback with ToggleOn/ToggleOff icons based on current state
- Proper handler (handleScrollSyncChange) to toggle the functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if DocumentPopover implements scroll sync toggle
ast-grep --pattern 'function DocumentPopover' -A 20
Length of output: 53
Script:
#!/bin/bash
# Let's try to find the DocumentPopover component definition with a more flexible pattern
ast-grep --pattern 'const DocumentPopover = ' -A 20
# Also search for the file containing DocumentPopover
fd "DocumentPopover" --type f
# Search for scroll sync related code in the component
rg -l "scroll.*sync" --type ts --type tsx
Length of output: 208
Script:
#!/bin/bash
# Let's examine the DocumentPopover component content
cat frontend/src/components/popovers/DocumentPopover.tsx
# Search for scroll sync related code with correct file type
rg -l "scroll.*sync" -t ts -t typescript
Length of output: 1869
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
frontend/src/components/popovers/ProfilePopover.tsx (1)
35-35: Consider enhancing accessibility for theme toggle.While the logic is correct, consider adding an aria-label to improve accessibility.
- dispatch(setTheme(themeMode === ThemeType.LIGHT ? ThemeType.DARK : ThemeType.LIGHT)); + const newTheme = themeMode === ThemeType.LIGHT ? ThemeType.DARK : ThemeType.LIGHT; + dispatch(setTheme(newTheme)); + // Update aria-label on theme change + document.querySelector('button[aria-label*="theme"]')?.setAttribute('aria-label', `Switch to ${newTheme === ThemeType.LIGHT ? 'dark' : 'light'} theme`);frontend/src/components/editor/Editor.tsx (1)
Line range hint
124-136: Implement scroll sync toggle functionality.The PR aims to add scroll sync toggle functionality, but the
ScrollSyncPanecomponent is still unconditionally rendered. This component should respect the user's scroll sync preference from the Redux store.Consider implementing the toggle like this:
+ const { isScrollSyncDisabled } = useSelector(selectConfig); return ( <> <div style={{ height: `calc(100% - ${BOTTOM_BAR_HEIGHT}px)` }}> - <ScrollSyncPane> + {isScrollSyncDisabled ? ( <div style={{ height: "100%", overflow: "auto", }} > {/* ... content ... */} </div> + ) : ( + <ScrollSyncPane> + <div + style={{ + height: "100%", + overflow: "auto", + }} + > + {/* ... content ... */} + </div> + </ScrollSyncPane> + )} - </ScrollSyncPane> </div> <EditorBottomBar width={width} /> </> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
frontend/src/components/editor/Editor.tsx(1 hunks)frontend/src/components/popovers/DocumentPopover.tsx(1 hunks)frontend/src/components/popovers/ProfilePopover.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/popovers/DocumentPopover.tsx
🔇 Additional comments (2)
frontend/src/components/popovers/ProfilePopover.tsx (1)
35-35: LGTM! Improved type safety with strict equality.
The change from loose (==) to strict (===) equality is a good practice in TypeScript, ensuring both value and type checking.
frontend/src/components/editor/Editor.tsx (1)
80-80: LGTM! Good improvement in type safety.
The change from == to === for theme comparison is a positive improvement, enforcing strict equality checks which is a TypeScript best practice.
What this PR does / why we need it:
This PR introduces an option to toggle the Scroll Sync feature on or off. Previously, the Scroll Sync functionality was always enabled, which caused issues with improper scrolling for users, especially in cases where the application is used by a large number of users.
With this change, users now have the ability to disable Scroll Sync, allowing for a more controlled scrolling experience.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores