fix: sync table scroll by hover source#1495
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
React Doctor found 27 issues in 4 files · 27 warnings · score 68 / 100 (Needs work) · vs 27 warnings
Reviewed by React Doctor for commit |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -747,6 +793,8 @@ const Table = <RecordType extends DefaultRecordType>( | |||
| className={`${prefixCls}-header`} | |||
| ref={scrollHeaderRef} | |||
| colGroup={bodyColGroup} | |||
There was a problem hiding this comment.
React Doctor · react-doctor/jsx-no-jsx-as-prop (warning)
This child redraws every render because the prop gets brand new JSX each time.
Fix → Move the JSX outside the component or wrap it in useMemo so memoized children do not redraw every render.
| @@ -763,6 +811,8 @@ const Table = <RecordType extends DefaultRecordType>( | |||
| className={`${prefixCls}-summary`} | |||
| ref={scrollSummaryRef} | |||
| colGroup={bodyColGroup} | |||
There was a problem hiding this comment.
React Doctor · react-doctor/jsx-no-jsx-as-prop (warning)
This child redraws every render because the prop gets brand new JSX each time.
Fix → Move the JSX outside the component or wrap it in useMemo so memoized children do not redraw every render.
| className={`${prefixCls}-sticky-scroll`} | ||
| onMouseEnter={onMouseEnter} | ||
| > | ||
| <div |
There was a problem hiding this comment.
React Doctor · react-doctor/no-static-element-interactions (warning)
Screen reader users can't tell this click handler is interactive because it has no role, so add a role or use a button or link.
Fix → Give clickable static elements a role, or use a button or link.
✅ Preview is ready!
↩️ Previous: ⚡️ 🤖 Powered by surge-preview |
|||||||||||||||
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to track the active scroll source (header, body, summary, or sticky scrollbar) using hover states (onMouseEnter) to improve scroll synchronization across different table sections. It also includes corresponding unit tests. The review feedback highlights two critical issues: first, a potential bug in getScrollSource where undefined or null values could incorrectly match and resolve the scroll source; second, the lack of a reset mechanism for hoverEle when the mouse leaves the table, which could break scroll synchronization during keyboard navigation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const getScrollSource = ( | ||
| source: ScrollSource | undefined, | ||
| currentTarget: HTMLElement | undefined, | ||
| refs: Record<ScrollSource, HTMLElement | null | undefined>, | ||
| ): ScrollSource | undefined => { | ||
| if (source) { | ||
| return source; | ||
| } | ||
|
|
||
| return (Object.keys(refs) as ScrollSource[]).find(key => refs[key] === currentTarget); | ||
| }; |
There was a problem hiding this comment.
In getScrollSource, if currentTarget is undefined or null, and any reference in refs is also undefined or null (e.g., refs.sticky is explicitly undefined, or refs.summary is null when no summary is rendered), the comparison refs[key] === currentTarget will evaluate to true.
This will incorrectly resolve the scroll source to that key (e.g., 'sticky' or 'summary'). To prevent this, we should ensure currentTarget is truthy before performing the comparison.
| const getScrollSource = ( | |
| source: ScrollSource | undefined, | |
| currentTarget: HTMLElement | undefined, | |
| refs: Record<ScrollSource, HTMLElement | null | undefined>, | |
| ): ScrollSource | undefined => { | |
| if (source) { | |
| return source; | |
| } | |
| return (Object.keys(refs) as ScrollSource[]).find(key => refs[key] === currentTarget); | |
| }; | |
| const getScrollSource = ( | |
| source: ScrollSource | undefined, | |
| currentTarget: HTMLElement | undefined, | |
| refs: Record<ScrollSource, HTMLElement | null | undefined>, | |
| ): ScrollSource | undefined => { | |
| if (source) { | |
| return source; | |
| } | |
| return (Object.keys(refs) as ScrollSource[]).find( | |
| key => currentTarget && refs[key] === currentTarget, | |
| ); | |
| }; |
| const [hoverEle, setHoverEle] = React.useState<ScrollSource | null>(null); | ||
| const onHoverScrollSource = React.useCallback( | ||
| (source: ScrollSource) => () => { | ||
| setHoverEle(source); | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
hoverEle is set to the hovered scroll source on onMouseEnter, but it is never reset to null when the mouse leaves the table container.
If a user hovers the header (setting hoverEle to 'header'), moves the mouse out of the table, and then uses keyboard navigation (e.g., Arrow keys) to scroll the table body, the scroll event will be triggered with source: 'body'. Since hoverEle is still locked to 'header', the condition hoverEle !== scrollSource will be true, causing the scroll sync to be ignored and breaking synchronization.
To fix this, we should reset hoverEle to null when the mouse leaves the table container by listening to the mouseleave event on fullTableRef.current.
const [hoverEle, setHoverEle] = React.useState<ScrollSource | null>(null);
const onHoverScrollSource = React.useCallback(
(source: ScrollSource) => () => {
setHoverEle(source);
},
[],
);
React.useEffect(() => {
const tableEl = fullTableRef.current;
const handleMouseLeave = () => {
setHoverEle(null);
};
tableEl?.addEventListener('mouseleave', handleMouseLeave);
return () => {
tableEl?.removeEventListener('mouseleave', handleMouseLeave);
};
}, []);
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1495 +/- ##
==========================================
+ Coverage 99.04% 99.20% +0.15%
==========================================
Files 45 45
Lines 1362 1377 +15
Branches 409 412 +3
==========================================
+ Hits 1349 1366 +17
+ Misses 13 11 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Fixes ant-design/ant-design#57850
Summary
Test Plan