fix: avoid table jitter casued by changes in the first row #301
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
src/List.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /.eslintrc.js
概述演练此次更改主要优化了 变更
序列图sequenceDiagram
participant User
participant List
participant ScrollHandler
User->>List: 触发滚动
List->>ScrollHandler: 开始滚动
ScrollHandler-->>List: 更新滚动状态
ScrollHandler->>List: 防抖重置状态
可能相关的 PR
兔子诗角
Finishing Touches
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: 2
🧹 Nitpick comments (2)
src/List.tsx (2)
386-388: 建议优化防抖函数的性能当前的防抖延迟时间可能需要根据实际滚动性能进行调整。建议将延迟时间作为组件的配置项。
-const toggleScrollStatus = React.useCallback(debounce(() => { +const SCROLL_DEBOUNCE_DELAY = 100; +const toggleScrollStatus = React.useCallback(debounce(() => { isScrollingRef.current = false; -}, 100), []); +}, SCROLL_DEBOUNCE_DELAY), []);
280-282: 建议添加注释说明防抖动逻辑为了便于其他开发者理解这个修复方案，建议添加更详细的注释。
// When the `scrollHeight` change is not caused by scrolling, // end the function execution avoiding table jitter caused by changes in the first row +// 说明： +// 1. 只在用户主动滚动时才进行高度调整 +// 2. 避免首行高度变化导致的表格抖动 +// 3. 通过 isScrollingRef 跟踪真实的滚动状态 if (!isScrollingRef.current) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/List.tsx(4 hunks)src/utils/debounce.ts(1 hunks)
🔇 Additional comments (1)
src/List.tsx (1)
275-275: 优化滚动状态判断逻辑通过
isScrollingRef跟踪滚动状态，避免非滚动导致的高度变化引起抖动，这是一个很好的改进。Also applies to: 280-282
| export function debounce(fn: (...args) => void, delay: number = 300) { | ||
| let timer; | ||
| return function (...args) { | ||
| if (timer) clearTimeout(timer); | ||
| timer = setTimeout(() => { | ||
| fn.apply(this, args); | ||
| timer = null; | ||
| }, delay); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
需要处理定时器清理以防止内存泄漏
当前的防抖函数实现可能会导致内存泄漏，因为定时器没有在适当的时候被清理。
建议按照以下方式重构代码：
export function debounce(fn: (...args) => void, delay: number = 300) {
let timer;
- return function (...args) {
+ const debounced = function (...args) {
if (timer) clearTimeout(timer);
timer = setTimeout(() => {
fn.apply(this, args);
timer = null;
}, delay);
+ };
+
+ debounced.cancel = () => {
+ if (timer) {
+ clearTimeout(timer);
+ timer = null;
+ }
};
+
+ return debounced;
}📝 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.
| export function debounce(fn: (...args) => void, delay: number = 300) { | |
| let timer; | |
| return function (...args) { | |
| if (timer) clearTimeout(timer); | |
| timer = setTimeout(() => { | |
| fn.apply(this, args); | |
| timer = null; | |
| }, delay); | |
| } | |
| } | |
| export function debounce(fn: (...args) => void, delay: number = 300) { | |
| let timer; | |
| const debounced = function (...args) { | |
| if (timer) clearTimeout(timer); | |
| timer = setTimeout(() => { | |
| fn.apply(this, args); | |
| timer = null; | |
| }, delay); | |
| }; | |
| debounced.cancel = () => { | |
| if (timer) { | |
| clearTimeout(timer); | |
| timer = null; | |
| } | |
| }; | |
| return debounced; | |
| } |
| // Set the scroll status to `true` | ||
| isScrollingRef.current = true; | ||
| // Set the scroll status to `false` after scrolling ends | ||
| toggleScrollStatus(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
建议添加滚动状态重置的清理逻辑
在组件卸载时需要确保滚动状态被正确重置。
+useEffect(() => {
+ return () => {
+ isScrollingRef.current = false;
+ toggleScrollStatus.cancel();
+ };
+}, []);Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #301 +/- ##
==========================================
+ Coverage 97.50% 97.54% +0.03%
==========================================
Files 19 20 +1
Lines 802 814 +12
Branches 187 190 +3
==========================================
+ Hits 782 794 +12
Misses 20 20 ☔ View full report in Codecov by Sentry. |
|
我发现这个会导致Table的分页空白 |
|
@jinxiangqiang Can you reproduce it please? |
|
rc-virtual-list-3.17.2.tgz |
You need to clone rc-table, and link rc-virtual-list to rc-table, then link rc-table to ant-design |
It doesn't work. I exectued |
I'm sorry. I forgot run |
You can add attribute |
Yes, my problem, I copied the whole page and pasted it directly causing the difference, I pulled the latest code and changed yours more into it and now it's OK |
ref ant-design/ant-design#52418
Bug fix
Summary by CodeRabbit
新功能
性能改进