fix: use only the attributes that are already defined #1176
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. |
Summary of ChangesHello @codewizardTM, 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 a bug 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
|
Walkthrough对 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分钟
Suggested reviewers
诗
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![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 fixes a bug where irrelevant attributes from Option components were being passed to a div, causing React warnings. The change achieves this by only using className and style from the option data and ignoring everything else.
My review suggests an improvement to this fix. While the change is correct, it is a bit too restrictive and removes the ability to pass other valid HTML attributes like data-* or aria-*. I've recommended using the pickAttrs utility to filter for valid attributes, which would solve the warning issue while maintaining flexibility. This would require adding an import for pickAttrs.
|
如果需要用 pickAttrs 过滤，我也可以加上 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1176 +/- ##
=======================================
Coverage 99.41% 99.41%
=======================================
Files 31 31
Lines 1200 1201 +1
Branches 428 428
=======================================
+ Hits 1193 1194 +1
Misses 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const option = selectContext.flattenOptions.find((opt) => opt.value === displayValue.value); | ||
| if (option?.data) { | ||
| const { label, value, className, style, key, ...rest } = option.data; | ||
| const { className, style } = option.data; |
There was a problem hiding this comment.
是不是得考虑 fieldNames 配置？ 还是说这里的 label value 和 key 都是 merge 过的。另外看看能不能加一个测试用例。
There was a problem hiding this comment.
我重新修改了一下，这个 PR 我是想将不合法的属性不注入到 div 上，按我的理解和 fieldNames 的配置是不是没有关系
![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/SelectInput/Content/SingleContent.tsx (1)
33-67: 建议添加测试用例以验证属性过滤行为。如之前评审者所建议的，应该为此修复添加测试用例。测试应覆盖以下场景：
- 有效的 HTML 属性能够通过：验证
data-*、aria-*等有效属性能正确传递到 DOM- 无效属性被过滤：验证自定义的非 HTML 属性不会传递到 DOM，从而避免 React 警告
- 特殊属性的合并：验证
className、style和title仍能正确合并- 核心属性被排除：验证
label、value、key不会被传播到 DOM这将确保修复按预期工作，并防止未来的回归。
您是否需要我协助生成测试用例代码？
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/SelectInput/Content/SingleContent.tsx(2 hunks)
🔇 Additional comments (2)
src/SelectInput/Content/SingleContent.tsx (2)
45-50: Let me search for information about how fieldNames configuration works in the react-component/select library.
Search Query: react-component/select fieldNames configuration option.data structure
3-3: Confirm thatpickAttrsis a documented and available function in@rc-component/util.The web search confirms that
pickAttrsis a legitimate, documented utility function in the@rc-component/utilpackage. It's specifically designed to filter props and keep only valid HTML attributes and event handlers—exactly the use case for this fix. The import path@rc-component/util/lib/pickAttrsmatches the documented API.The original concern about the function being unavailable or causing runtime errors appears to be unfounded based on the library's public documentation. The function is properly exported and used as shown in the API examples.
However, I cannot verify:
- The exact version of
@rc-component/utilspecified in this repository'spackage.json- The complete implementation details in
SingleContent.tsx(lines 45-50) wherepickAttrsis used- Whether test coverage was added (as requested in previous review comments)
2 participants
🤔 This is a ...
🔗 Related Issues
ant-design/ant-design#55877
💡 Background and Solution
Irrelevant attributes are also added to the div, causing React to show warnings in development mode，
This change only uses already defined attributes; all other attributes are ignored
📝 Change Log
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.