fix: avoid array length truncation in mergeWith and add sparse merge test #714
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新增针对 mergeWith 的单元测试，并修改 Changes
Sequence Diagram(s)（无） Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 @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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
![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/test/mergeWith.test.ts (1)
7-7: 可选：考虑注释语言的一致性代码中包含中文注释 "仅更新第 2 项，长度为 2"。如果项目有注释语言规范，建议保持一致性。
🔎 可选的英文注释
- changedValues.list[1] = 'BB'; // 仅更新第 2 项，长度为 2 + changedValues.list[1] = 'BB'; // Only update index 1, length is 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/mergeWith.test.tssrc/utils/set.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/mergeWith.test.ts (1)
src/utils/set.ts (1)
mergeWith(79-129)
🔇 Additional comments (2)
src/utils/set.ts (1)
113-116: 修复正确，防止了数组长度被意外截断！这个改动准确地解决了问题：当合并数组时，
Reflect.ownKeys会返回包括'length'在内的所有属性键。如果不过滤掉'length'，后续的稀疏数组或较短数组的length值会通过递归合并覆盖之前已合并的数组长度，导致元素丢失。通过显式过滤
'length'属性，确保只遍历真实的数组索引键，保留了数组的整体覆盖语义。src/test/mergeWith.test.ts (1)
1-16: 测试用例有效验证了修复！这个测试准确地覆盖了修复的场景：
- 创建了一个稀疏数组（长度为 2，但只有索引 1 有值）
- 使用
prepareArray保留现有数组内容- 验证合并后的数组保持了原始长度 4，并且只更新了指定的索引
测试逻辑清晰，断言全面（同时检查了数组内容和长度）。
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils/set.ts (1)
113-117: 考虑添加可选链以提高防御性，并评估性能优化的可能性。这个解决方案相比硬编码检查
'length'更加优雅和通用，能够自然地过滤掉所有非可枚举属性（包括数组的length）。不过有两点可以考虑改进：
- 防御性编程：虽然在当前上下文中
Object.getOwnPropertyDescriptor(value, key)理论上不会返回undefined（因为key来自keys(value)），但添加可选链会更安全：- if (Object.getOwnPropertyDescriptor(value, key).enumerable) { + if (Object.getOwnPropertyDescriptor(value, key)?.enumerable) {
- 性能优化：在不支持
Reflect的环境中，keys函数使用的是Object.keys（第 67 行），它本身就只返回可枚举属性，此时这个检查是冗余的。可以考虑优化：// 在文件顶部判断一次 const needsEnumerableCheck = typeof Reflect !== 'undefined'; // 在循环中 keys(value).forEach(key => { if (!needsEnumerableCheck || Object.getOwnPropertyDescriptor(value, key)?.enumerable) { internalMerge([...path, key], loopSet); } });不过考虑到现代环境基本都支持
Reflect，性能优化的收益可能有限。建议至少添加可选链以提高代码的健壮性。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #714 +/- ##
=======================================
Coverage 91.57% 91.58%
=======================================
Files 37 37
Lines 938 939 +1
Branches 306 322 +16
=======================================
+ Hits 859 860 +1
Misses 77 77
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
问题点：util 的 mergeWith 内部用 Reflect.ownKeys 遍历键，数组场景下会得到 length。合并多个 source 时，后一个稀疏/更短的数组会把前面已合并好的长度覆盖掉，导致数组被截断、元素丢失。
现处理：在 mergeWith 的数组分支里遍历键时跳过 length，只递归真实索引键，保持数组整体覆盖语义但不改写已有长度。这样不会再被后续数组的 length 破坏结果。
fix ant-design/ant-design#56245
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.