fix: Sync-scrolling multiple panels jumps to bottom of shortest panel when scrolling up #3023
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. |
📝 WalkthroughWalkthroughModified DataBrowser.react.js to enhance multi-panel scroll synchronization. Added internal state fields Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as DataBrowser UI
participant ScrollLogic as Scroll Handler
participant State as Internal State
participant Panels as Panel Components
User->>UI: Wheel scroll on wrapper
UI->>ScrollLogic: handleWrapperWheel triggered
ScrollLogic->>State: Set isWheelScrolling = true
ScrollLogic->>ScrollLogic: Compute max scrollTop delta
ScrollLogic->>Panels: Apply delta to all panels
Note over ScrollLogic: Debounce clears flag
ScrollLogic->>State: Set isWheelScrolling = false
User->>UI: Mouse enter panel
UI->>Panels: Trigger mouse enter handler
Panels->>State: Update activePanelIndex
User->>Panels: Scroll within active panel
Panels->>ScrollLogic: handlePanelScroll triggered
ScrollLogic->>ScrollLogic: Check guards: isWheelScrolling? activePanelIndex match?
alt Guard passes
ScrollLogic->>Panels: Sync panel scroll
else Guard fails
ScrollLogic->>ScrollLogic: Exit early
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
mtrezza
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/dashboard/Data/Browser/DataBrowser.react.js (3)
917-927: Consider simplifying the guard condition.The check
this.activePanelIndex !== undefinedis redundant sinceactivePanelIndexis initialized to-1and only ever assigned numeric values. This could be simplified.- if ( - this.activePanelIndex !== -1 && - this.activePanelIndex !== undefined && - this.activePanelIndex !== index - ) { + if (this.activePanelIndex !== -1 && this.activePanelIndex !== index) { return; }
943-950: DeclarewheelTimeoutin the constructor for consistency.The
wheelTimeoutproperty is used here but not declared in the constructor, unlikesaveOrderTimeout(line 167). While JavaScript allows this, declaring it in the constructor improves code consistency and readability.Add to the constructor around line 167:
this.saveOrderTimeout = null; this.wheelTimeout = null;
1466-1469: Consider resettingactivePanelIndexwhen leaving panels.The active panel is tracked on enter/touch/focus but never reset when leaving. This could leave a stale value if the user moves the cursor outside all panels. Consider adding
onMouseLeaveto reset the index.onMouseEnter={() => (this.activePanelIndex = index)} + onMouseLeave={() => (this.activePanelIndex = -1)} onTouchStart={() => (this.activePanelIndex = index)} onFocus={() => (this.activePanelIndex = index)} + onBlur={() => (this.activePanelIndex = -1)} onScroll={(e) => this.handlePanelScroll(e, index)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/dashboard/Data/Browser/DataBrowser.react.js(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Node 20
- GitHub Check: Node 18
- GitHub Check: Docker linux/amd64
🔇 Additional comments (3)
src/dashboard/Data/Browser/DataBrowser.react.js (3)
170-171: LGTM - Instance variables for scroll tracking.Using instance variables instead of React state is appropriate here since these values are transient UI state that shouldn't trigger re-renders.
955-970: Scroll synchronization fix looks correct.The approach of computing
maxScrollTopacross all panels and applying the delta uniformly addresses the issue where scrolling up would jump to the shortest panel's position. This ensures all panels scroll from the furthest scroll position, preventing the jump-to-bottom behavior.
912-936: Verify scroll behavior with panels of different heights.The fix addresses the jump-to-bottom issue by using max scrollTop across panels. Confirm that when panels have significantly different content heights, the scroll synchronization behaves as expected (shorter panels will cap at their max scroll while longer panels continue scrolling).
# [8.1.0-alpha.3](8.1.0-alpha.2...8.1.0-alpha.3) (2025-11-26) ### Bug Fixes * Sync-scrolling multiple panels jumps to bottom of shortest panel when scrolling up ([#3023](#3023)) ([3f85f89](3f85f89))
|
🎉 This change has been released in version 8.1.0-alpha.3 |
2 participants
New Pull Request Checklist
Issue Description
Sync-scrolling multiple panels jumps to bottom of shortest panel when scrolling up.
Approach
Fix scroll position algorithm.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.