fix: add bounds check to getColumnWidth function #1399
Conversation
|
Someone is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
概览
变更
代码审查工作量评估🎯 2 (简单) | ⏱️ ~8 分钟 需要关注的方面：
诗歌
Pre-merge checks and finishing touches✅ 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 |
Summary of ChangesHello @cactuser-Lu, 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! This pull request addresses and fixes a critical array out-of-bounds error in the Highlights
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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1399 +/- ##
=======================================
Coverage 96.09% 96.10%
=======================================
Files 57 57
Lines 3434 3438 +4
Branches 632 633 +1
=======================================
+ Hits 3300 3304 +4
Misses 129 129
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 aims to fix an array out-of-bounds error in the getColumnWidth function when colSpan is large. The proposed change correctly handles the out-of-bounds access for large colSpan values. However, it introduces a new bug in calculating the width for columns that start from the first column (colIndex 0). The current implementation incorrectly calculates the start offset in this scenario, leading to an incorrect width calculation. I've provided a corrected implementation that is more robust and handles all edge cases correctly.
| const startIndex = Math.max(colIndex, 0); | ||
| const endIndex = Math.min(startIndex + mergedColSpan, columnsOffset.length - 1); | ||
|
|
||
| const startOffset = columnsOffset[startIndex] || 0; | ||
| const endOffset = columnsOffset[endIndex] || startOffset; | ||
| return Math.max(endOffset - startOffset, 0); |
There was a problem hiding this comment.
While this change fixes the out-of-bounds access when colSpan is large, it introduces a new issue. When calculating the width for a span starting at the first column (i.e., when colIndex is -1), the startIndex becomes 0, and startOffset is incorrectly calculated as columnsOffset[0] instead of 0. This results in an incorrect width calculation.
I suggest a more robust implementation that correctly handles all edge cases, including negative colIndex, large colSpan, and out-of-bounds colIndex values, while being more readable.
const len = columnsOffset.length;
const lastOffset = len > 0 ? columnsOffset[len - 1] : 0;
const startIdx = colIndex;
const endIdx = colIndex + mergedColSpan;
const startOffset = startIdx < 0 ? 0 : (startIdx >= len ? lastOffset : columnsOffset[startIdx]);
const endOffset = endIdx < 0 ? 0 : (endIdx >= len ? lastOffset : columnsOffset[endIdx]);
return Math.max(endOffset - startOffset, 0);
![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 (1)
src/VirtualTable/VirtualCell.tsx (1)
34-34: 建议添加 JSDoc 文档说明参数约束。考虑为函数添加文档注释,说明参数的预期范围和边界情况的处理方式:
/** * Return the width of the column by `colSpan`. * When `colSpan` is `0` will be trade as `1`. + * + * @param colIndex - Starting column index (can be negative, will be clamped to 0) + * @param colSpan - Number of columns to span (0 will be treated as 1) + * @param columnsOffset - Array of cumulative column offsets + * @returns The calculated width (always non-negative) */ export function getColumnWidth(colIndex: number, colSpan: number, columnsOffset: number[]): number {这能帮助其他开发者理解函数如何处理边界情况,特别是负数
colIndex的处理(参见第 78 行的调用场景)。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/VirtualTable/VirtualCell.tsx(1 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). (1)
- GitHub Check: test / react component workflow
🔇 Additional comments (1)
src/VirtualTable/VirtualCell.tsx (1)
34-43: 边界检查实现完整正确,有效防止数组越界。验证结果确认:
- 负数索引处理: 当
colIndex = 0时,startColIndex = -1通过Math.max(colIndex, 0)安全处理,防止数组访问错误。- 超大 colSpan 处理:
Math.min(startIndex + mergedColSpan, columnsOffset.length - 1)正确限制 endIndex 在有效范围内,返回从起始位置到最后可用列的总宽度。- 零值处理:
colSpan || 1正确处理零值,视为跨度 1。- 防御性编程: 通过
|| 0和|| startOffset提供后备值,确保不会出现 undefined 计算。- 非负结果:
Math.max(endOffset - startOffset, 0)保证返回值非负。实现代码现已通过验证,所有边界情况均被正确处理。
1 participant
fix: prevent array bounds error in getColumnWidth calculation
当colSpan过大时，getColumnWidth 中数组越界，将产生样式错乱
Summary by CodeRabbit
发布说明