-
-
Notifications
You must be signed in to change notification settings - Fork 621
fix(table): guard virtual rowSpan height calc when data shrinks #1433
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@QDyanbing is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough为虚拟表格在处理跨行合并(rowSpan)时的高度计算加入防护:当跨行结束项缺失或其 record 为 undefined 时,改为使用最后可用项或退回单行高度,避免读取不存在的 record 导致异常;并新增回归测试覆盖 pageSize 缩小场景。 Changes
Sequence Diagram(s)(本次改动为防御性修复,未引入跨多个组件的新控制流,故省略序列图。) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分钟 Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
Summary of ChangesHello @QDyanbing, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求旨在解决虚拟表格在数据量减少时, Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
本次 PR 修复了虚拟表格在数据缩小时,rowSpan 高度计算可能使用过期索引导致越界访问的问题。通过在 getHeight 方法中增加防御性处理,有效避免了运行时错误。代码逻辑正确,解决了所述问题。我提出了一点关于代码结构优化的建议,以提高代码的可读性和可维护性。
| const endItem = flattenData[endItemIndex]; | ||
|
|
||
| if (!endItem || !endItem.record) { | ||
| // clamp 到当前可用的最后一行,或退化为默认高度 | ||
| const safeEndIndex = Math.min(endItemIndex, flattenData.length - 1); | ||
| const safeEndItem = flattenData[safeEndIndex]; | ||
|
|
||
| if (!safeEndItem || !safeEndItem.record) { | ||
| // 兜底:没有任何安全 endItem,就返回单行高度 | ||
| const single = getSize(rowKey); | ||
| return single.bottom - single.top; | ||
| } | ||
|
|
||
| const endItemKey = getRowKey(safeEndItem.record, safeEndIndex); | ||
| const sizeInfo = getSize(rowKey, endItemKey); | ||
| return sizeInfo.bottom - sizeInfo.top; | ||
| } | ||
|
|
||
| const endItemKey = getRowKey(endItem.record, endItemIndex); | ||
|
|
||
| const sizeInfo = getSize(rowKey, endItemKey); | ||
| return sizeInfo.bottom - sizeInfo.top; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里的逻辑是正确的,但存在一些代码重复,并且可以组织得更清晰。我们可以通过重构来简化代码,将正常路径和回退路径的逻辑合并,以避免重复并提高可读性。
let targetItem = flattenData[endItemIndex];
let targetIndex = endItemIndex;
if (!targetItem || !targetItem.record) {
// clamp 到当前可用的最后一行
targetIndex = Math.min(endItemIndex, flattenData.length - 1);
targetItem = flattenData[targetIndex];
// 兜底:没有任何安全 endItem,就返回单行高度
if (!targetItem || !targetItem.record) {
const single = getSize(rowKey);
return single.bottom - single.top;
}
}
const endItemKey = getRowKey(targetItem.record, targetIndex);
const sizeInfo = getSize(rowKey, endItemKey);
return sizeInfo.bottom - sizeInfo.top;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/VirtualTable/BodyGrid.tsx (1)
198-203: 代码注释应使用英文以保持项目一致性。代码中的中文注释(第 198 行和第 203 行)应改为英文,以匹配项目的编码规范和国际化需求。
🔎 建议的英文注释
- // clamp 到当前可用的最后一行,或退化为默认高度 + // Clamp to the last available row, or fall back to default height const safeEndIndex = Math.min(endItemIndex, flattenData.length - 1); const safeEndItem = flattenData[safeEndIndex]; if (!safeEndItem || !safeEndItem.record) { - // 兜底:没有任何安全 endItem,就返回单行高度 + // Fallback: if no safe endItem exists, return single row height const single = getSize(rowKey); return single.bottom - single.top; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/VirtualTable/BodyGrid.tsx
🔇 Additional comments (2)
src/VirtualTable/BodyGrid.tsx (2)
195-217: 修复有效:防御性处理避免了运行时错误。这段代码正确地解决了 PR 目标中描述的问题——当数据缩减时(例如分页从 200 条变为 10 条),
endItemIndex可能超出flattenData的范围,导致endItem.record访问未定义对象而抛出 TypeError。新增的防御逻辑:
- 检查
endItem是否存在- 将索引钳制到数组边界内
- 提供单行高度作为最终降级方案
这确保了虚拟表格在数据转换期间不会崩溃。
基于学习记录,请在实际使用场景中验证此修复是否完全解决了 ant-design/ant-design#56459 中报告的问题,特别是在快速切换分页大小时的表现。
197-211: 当前的防御性检查已能正确处理边界情况。代码在
end < 0时已有守卫(第135行),防止在无数据时执行。同时,getHeight仅在spanLines非空时才会被调用,而spanLines的构造依赖于flattenData中存在 rowSpan 的项目。这意味着当flattenData为空时,spanLines也为空,getHeight不会被执行,因此不会到达第199行的Math.min(endItemIndex, -1)路径。即便代码路径被到达,第197-202行的防御性检查(
!safeEndItem || !safeEndItem.record)也能正确处理-1索引导致的undefined情况。关于降级到单行高度的视觉行为,当前处理是合理的防御性设计。建议通过添加注释说明这种降级行为是预期的,以增加代码可维护性。
|
是否好写用例? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1433 +/- ##
=======================================
Coverage 96.09% 96.10%
=======================================
Files 57 57
Lines 3434 3442 +8
Branches 632 637 +5
=======================================
+ Hits 3300 3308 +8
Misses 129 129
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
补充了一个 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Virtual.spec.tsx (1)
640-720: 测试用例设计合理,有效覆盖了数据缩减场景下的崩溃问题。该测试准确复现了 issue #56459 中描述的问题场景:
- 创建大数据集并使用 rowSpan
- 滚动到底部
- 缩减数据量(从 200 条到 10 条)
- 验证不会抛出异常且无 undefined 访问错误
测试结构清晰,断言充分。
可选的增强建议
可以考虑在数据缩减后添加正向断言,验证表格仍正确渲染:
// 不应出现 record undefined 相关错误 const errorText = errSpy.mock.calls.map(args => String(args[0] ?? '')).join('\n'); expect(errorText).not.toContain('Cannot read properties of undefined'); expect(errorText).not.toContain("reading 'record'"); + + // 验证表格正确渲染了新数据 + const rows = container.querySelectorAll('.rc-table-row'); + expect(rows.length).toBeGreaterThan(0);这能确保表格在数据缩减后不仅没有崩溃,而且能正常显示内容。不过对于回归测试来说,当前的断言已经足够。
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Virtual.spec.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-08T12:53:09.293Z
Learnt from: bbb169
Repo: react-component/table PR: 1202
File: src/Table.tsx:903-904
Timestamp: 2024-11-08T12:53:09.293Z
Learning: 在 `src/Table.tsx` 文件的 React 组件 `Table` 中,即使 `bodyScrollLeft` 频繁更新,也需要在 `TableContextValue` 的 `useMemo` 依赖数组中包含 `bodyScrollLeft` 和 `headerCellRefs`,因为每次滚动时重新计算 `TableContextValue` 是解决该问题所必须的。
Applied to files:
tests/Virtual.spec.tsx
🧬 Code graph analysis (1)
tests/Virtual.spec.tsx (2)
src/interface.ts (1)
Reference(48-51)src/index.ts (2)
Reference(24-24)VirtualTable(21-21)
修复说明
虚拟表格在数据缩小时,
rowSpan高度计算可能使用过期索引,导致访问越界的
flattenData并触发运行时错误。本次在
getHeight中增加防御性处理,避免过渡状态下崩溃,不影响正常渲染行为。
fix ant-design/ant-design#56459
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.