fix: prevent memory leaks in table and sticky scroll components #1288
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes potential memory leaks by ensuring that event listeners in the StickyScrollBar and Table components are properly cleaned up.
- In StickyScrollBar, new refs are added to track the previous container and scroll parents and their associated event listeners are cleaned up before adding new ones.
- In Table, an unmountedRef is introduced to prevent state updates after the component has unmounted.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/stickyScrollBar.tsx | Adds cleanup for previous event listeners by tracking the last container and scroll parents. |
| src/Table.tsx | Introduces an unmountedRef to prevent delayed operations on an unmounted component. |
Comments suppressed due to low confidence (1)
src/stickyScrollBar.tsx:159
- For consistency and clarity, consider explicitly passing the capture option (false) to removeEventListener, as is done with addEventListener.
lastScrollParentsRef.current.forEach(p => p.removeEventListener('scroll', checkScrollBarVisible),
src/stickyScrollBar.tsx
Outdated
| return () => { | ||
| scrollParents.forEach(p => p.removeEventListener('scroll', checkScrollBarVisible)); | ||
| window.removeEventListener('resize', checkScrollBarVisible); | ||
| window.removeEventListener('scroll', checkScrollBarVisible); | ||
| container.removeEventListener('scroll', checkScrollBarVisible); | ||
| }; | ||
| }, [container]); | ||
| }, [container, scrollBodyRef.current]); |
There was a problem hiding this comment.
Using a mutable ref value (scrollBodyRef.current) in the dependency array may not trigger updates as expected; consider verifying that the effect is rerun when intended or refactoring to use a stable state or ref object.
| }, [container, scrollBodyRef.current]); | |
| }, [container, scrollBodyNode]); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
@afc163 Copilot 的建议是合理的，但解决方案有问题。
问题分析：
- ✅ Copilot 正确指出了问题：
scrollBodyRef.current是可变的 ref 值，React 不会追踪其变化 - ✅ 当
scrollBodyRef.current变化但container不变时，useEffect 不会重新运行 - ❌ 但建议的
scrollBodyNode变量在代码中并不存在
更好的解决方案：
// 选项1：移除 scrollBodyRef.current 依赖
}, [container]);
// 选项2：使用 callback ref 替代
const [scrollBodyNode, setScrollBodyNode] = React.useState<HTMLDivElement | null>(null);
const scrollBodyCallback = React.useCallback((node: HTMLDivElement) => {
setScrollBodyNode(node);
}, []);
// 然后在依赖中使用 scrollBodyNode
}, [container, scrollBodyNode]);推荐选项1，因为：
- effect 内部已有
if (!scrollBodyRef.current) return;的检查 - 滚动容器变化时通常意味着整个组件重新渲染，scrollBodyRef 也会相应更新
- 避免了不必要的复杂性
Copilot 识别了真实问题，但提供的解决方案需要改进。
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## antd-5.x #1288 +/- ##
============================================
- Coverage 97.78% 97.77% -0.02%
============================================
Files 77 77
Lines 7594 7669 +75
Branches 1153 1161 +8
============================================
+ Hits 7426 7498 +72
- Misses 162 165 +3
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
和 #1266 要做的是同一件事？ |
|
@yoyo837 看上去是的。 |
|
看上去 我已详细检查了 1. 事件监听与清理
2. 其它副作用
3. ref 传递
结论FixedHolder 组件本身没有内存泄漏问题。
你可以放心使用。如果有特殊场景（如频繁 mount/unmount 或动态切换 ref），也不会有泄漏风险。如需进一步排查其它细节或组件，欢迎继续指定！ |
src/stickyScrollBar.tsx
Outdated
|
|
||
| scrollParents.forEach(p => p.addEventListener('scroll', checkScrollBarVisible, false)); | ||
| window.addEventListener('resize', checkScrollBarVisible, false); | ||
| window.addEventListener('scroll', checkScrollBarVisible, false); | ||
| container.addEventListener('scroll', checkScrollBarVisible, false); | ||
|
|
||
| // 清理上一次 container 的事件监听 | ||
| if (lastContainerRef.current && lastContainerRef.current !== container) { |
There was a problem hiding this comment.
看起来不需要 lastContainerRef，在 effect 里记录这个值就行了？
There was a problem hiding this comment.
@afc163 你好！zombieJ 的建议是正确的。由于 useEffect 的依赖包含了 [container]，当 container 改变时整个 effect 会重新运行，清理函数会自动移除之前 container 的事件监听器，所以确实不需要 lastContainerRef。
以下是修改建议：
const StickyScrollBar: React.ForwardRefRenderFunction<unknown, StickyScrollBarProps> = (
{ scrollBodyRef, onScroll, offsetScroll, container, direction },
ref,
) => {
const prefixCls = useContext(TableContext, 'prefixCls');
const bodyScrollWidth = scrollBodyRef.current?.scrollWidth || 0;
const bodyWidth = scrollBodyRef.current?.clientWidth || 0;
const scrollBarWidth = bodyScrollWidth && bodyWidth * (bodyWidth / bodyScrollWidth);
const scrollBarRef = React.useRef<HTMLDivElement>();
const [scrollState, setScrollState] = useLayoutState<{
scrollLeft: number;
isHiddenScrollBar: boolean;
}>({
scrollLeft: 0,
isHiddenScrollBar: true,
});
const refState = React.useRef<{
delta: number;
x: number;
}>({
delta: 0,
x: 0,
});
const [isActive, setActive] = React.useState(false);
const rafRef = React.useRef<number | null>(null);
// 记录上一次的 scrollParents
const lastScrollParentsRef = React.useRef<(HTMLElement | SVGElement)[]>([]);
// ... 其他代码保持不变 ...
// Loop for scroll event check
React.useEffect(() => {
if (!scrollBodyRef.current) {
return;
}
// 清理上一次 scrollParents 的事件监听
lastScrollParentsRef.current.forEach(p =>
p.removeEventListener('scroll', checkScrollBarVisible),
);
const scrollParents: (HTMLElement | SVGElement)[] = [];
let parent = getDOM(scrollBodyRef.current);
while (parent) {
scrollParents.push(parent);
parent = parent.parentElement;
}
lastScrollParentsRef.current = scrollParents;
scrollParents.forEach(p => p.addEventListener('scroll', checkScrollBarVisible, false));
window.addEventListener('resize', checkScrollBarVisible, false);
window.addEventListener('scroll', checkScrollBarVisible, false);
container.addEventListener('scroll', checkScrollBarVisible, false);
return () => {
scrollParents.forEach(p => p.removeEventListener('scroll', checkScrollBarVisible));
window.removeEventListener('resize', checkScrollBarVisible);
window.removeEventListener('scroll', checkScrollBarVisible);
container.removeEventListener('scroll', checkScrollBarVisible);
};
}, [container]);
// ... 其他代码保持不变 ...
};主要变更：
- 移除了
lastContainerRef的声明 - 移除了手动清理之前 container 事件监听器的代码
- 保留了
lastScrollParentsRef因为 scrollParents 的清理逻辑仍然需要
这样的修改更简洁，也避免了重复的事件监听器清理逻辑。
🐰💡
∩───∩
( ◕ ◕ )
│ ∩─∩ │
└─┘ └─┘
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -149,14 +151,22 @@ const StickyScrollBar: React.ForwardRefRenderFunction<unknown, StickyScrollBarPr | |||
|
|
|||
| // Loop for scroll event check | |||
| React.useEffect(() => { | |||
| if (!scrollBodyRef.current) return; | |||
| if (!scrollBodyRef.current) { | |||
让 cursor 帮忙修复了一下可能的内存泄漏问题。
close #1266
close ant-design/ant-design#53506