feat(mergeProps): enhance mergeProps to handle className and style#745
feat(mergeProps): enhance mergeProps to handle className and style#745guoyunhe wants to merge 1 commit intoreact-component:masterfrom
Conversation
|
@guoyunhe 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, 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 refines the 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. Footnotes
|
概览该PR增强了 变更
预估代码审查工作量🎯 2 (简单) | ⏱️ ~10 分钟 可能相关的PR
建议的审核人
诗句
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 enhances the mergeProps utility function by adding an example to its JSDoc and introducing specific merging logic for className and style properties. For className, it concatenates values, and for style, it merges objects. New test cases for these specific merges have also been added. The review highlights that the current className merging logic could be more robust by filtering falsy values to avoid issues with null or empty strings. Additionally, the style merging logic is identified as potentially unsafe, as it assumes both values are objects, and suggests adding type checks before merging. It is also recommended to add more edge case tests for className and style to improve test coverage and prevent these issues.
| if (item[key] !== undefined) { | ||
| ret[key] = item[key]; | ||
| if (key === 'className') { | ||
| ret[key] = ret[key] ? `${ret[key]} ${item[key]}` : item[key]; |
There was a problem hiding this comment.
当前合并 className 的逻辑不够健壮，在某些情况下可能产生不正确的结果：
- 如果
className的值为null，它会被字符串化，例如'a null'。 - 如果
className的值是空字符串，可能会导致多余的空格，例如'a '。
更稳妥的方法是在连接 class 名称之前过滤掉所有假值（falsy values）。
| ret[key] = ret[key] ? `${ret[key]} ${item[key]}` : item[key]; | |
| ret[key] = [ret[key], item[key]].filter(Boolean).join(' '); |
| if (key === 'className') { | ||
| ret[key] = ret[key] ? `${ret[key]} ${item[key]}` : item[key]; | ||
| } else if (key === 'style') { | ||
| ret[key] = { ...ret[key], ...item[key] }; |
There was a problem hiding this comment.
当前合并 style 对象的逻辑是不安全的。它无条件地对 ret[key] 和 item[key] 使用了展开语法，假定它们都是对象。如果其中一个是其他类型（例如字符串），这可能导致意外行为。例如，展开一个字符串 'foo' 会得到 { '0': 'f', '1': 'o', '2': 'o' }，这将创建一个无效的样式对象。
合并操作只应在 style 的现有值和新值都是对象时执行。否则，它应该回退到默认的覆盖属性的行为。
if (ret[key] && typeof ret[key] === 'object' && item[key] && typeof item[key] === 'object') {
ret[key] = { ...ret[key], ...item[key] };
} else {
ret[key] = item[key];
}| style: { color: 'red', backgroundColor: 'blue' }, | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
为 className 和 style 合并添加的测试用例很棒！为了使它们更健壮，建议增加一些边缘情况的测试用例，例如 null 值、className 的空字符串，以及 style 的非对象值。这将有助于防止我在 src/mergeProps.ts 中指出的问题再次出现。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #745 +/- ##
==========================================
+ Coverage 86.31% 86.37% +0.06%
==========================================
Files 39 39
Lines 1052 1057 +5
Branches 372 391 +19
==========================================
+ Hits 908 913 +5
Misses 142 142
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/mergeProps.test.ts (1)
10-23: 建议补两组边界用例，锁定新语义建议再加：
style同名 key 冲突时右侧覆盖左侧；className为''/null时不引入多余空格或"null"字符串。
这样可以直接防止本次逻辑在后续重构中回退。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mergeProps.test.ts` around lines 10 - 23, Add two boundary tests to tests/mergeProps.test.ts to lock the intended semantics for mergeProps: one test where both inputs have the same style property (e.g., a = { style: { color: 'red' } }, b = { style: { color: 'blue' } }) asserting mergeProps(a, b).style.color === 'blue' to ensure right-side overrides left; and one or two tests for className where left or right is '' or null (e.g., a = { className: '' }, b = { className: 'b' } and a = { className: null }, b = { className: 'b' }) asserting merged className === 'b' (no extra spaces or "null" string) to prevent regressions in mergeProps behavior.src/mergeProps.ts (1)
5-8: 建议补充能体现新规则的示例当前示例没有展示
className拼接和style浅合并，建议补一段最小示例，避免调用方误以为仍是普通覆盖。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mergeProps.ts` around lines 5 - 8, 在 mergeProps 的文档示例中补充一个最小示例，展示 className 的拼接规则和 style 的浅合并行为：说明用 mergeProps(defaults, config, props) 时若 defaults/className 与 props.className 同时存在会被拼接（比如 "btn" + " primary"），并说明 style 会进行浅合并（例如 defaults.style = { color: 'red', padding: 4 } 与 props.style = { padding: 8, fontSize: 12 } 合并后为 { color: 'red', padding: 8, fontSize: 12 }）；在说明中引用 mergeProps、className、style 等符号以便定位代码并避免误解为普通覆盖。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mergeProps.ts`:
- Around line 19-21: The className concatenation in mergeProps may produce "a "
or "a null" because it doesn't skip empty/null/undefined values; update the
className branch in the mergeProps logic (the block referencing ret, item and
key === 'className') to first check that item[key] is a non-empty string (and
not null/undefined) before concatenating, and if item[key] is empty skip it
entirely (or trim and only join when both sides are non-empty) so ret[key] never
becomes "a " or "a null".
---
Nitpick comments:
In `@src/mergeProps.ts`:
- Around line 5-8: 在 mergeProps 的文档示例中补充一个最小示例，展示 className 的拼接规则和 style
的浅合并行为：说明用 mergeProps(defaults, config, props) 时若 defaults/className 与
props.className 同时存在会被拼接（比如 "btn" + " primary"），并说明 style 会进行浅合并（例如
defaults.style = { color: 'red', padding: 4 } 与 props.style = { padding: 8,
fontSize: 12 } 合并后为 { color: 'red', padding: 8, fontSize: 12 }）；在说明中引用
mergeProps、className、style 等符号以便定位代码并避免误解为普通覆盖。
In `@tests/mergeProps.test.ts`:
- Around line 10-23: Add two boundary tests to tests/mergeProps.test.ts to lock
the intended semantics for mergeProps: one test where both inputs have the same
style property (e.g., a = { style: { color: 'red' } }, b = { style: { color:
'blue' } }) asserting mergeProps(a, b).style.color === 'blue' to ensure
right-side overrides left; and one or two tests for className where left or
right is '' or null (e.g., a = { className: '' }, b = { className: 'b' } and a =
{ className: null }, b = { className: 'b' }) asserting merged className === 'b'
(no extra spaces or "null" string) to prevent regressions in mergeProps
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7068d635-bc73-4a28-ab1b-63aa5fc58e68
📒 Files selected for processing (2)
src/mergeProps.tstests/mergeProps.test.ts
| if (key === 'className') { | ||
| ret[key] = ret[key] ? `${ret[key]} ${item[key]}` : item[key]; | ||
| } else if (key === 'style') { |
There was a problem hiding this comment.
className 在空值场景会拼出脏结果
Line 20 对空字符串/null 没有过滤，可能得到 "a " 或 "a null"。建议在拼接前跳过空值。
🔧 建议修改
- if (key === 'className') {
- ret[key] = ret[key] ? `${ret[key]} ${item[key]}` : item[key];
+ if (key === 'className') {
+ const nextClassName = item[key];
+ if (nextClassName === '' || nextClassName == null) {
+ continue;
+ }
+ ret[key] = ret[key] ? `${ret[key]} ${nextClassName}` : nextClassName;📝 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.
| if (key === 'className') { | |
| ret[key] = ret[key] ? `${ret[key]} ${item[key]}` : item[key]; | |
| } else if (key === 'style') { | |
| if (key === 'className') { | |
| const nextClassName = item[key]; | |
| if (nextClassName === '' || nextClassName == null) { | |
| continue; | |
| } | |
| ret[key] = ret[key] ? `${ret[key]} ${nextClassName}` : nextClassName; | |
| } else if (key === 'style') { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mergeProps.ts` around lines 19 - 21, The className concatenation in
mergeProps may produce "a " or "a null" because it doesn't skip
empty/null/undefined values; update the className branch in the mergeProps logic
(the block referencing ret, item and key === 'className') to first check that
item[key] is a non-empty string (and not null/undefined) before concatenating,
and if item[key] is empty skip it entirely (or trim and only join when both
sides are non-empty) so ret[key] never becomes "a " or "a null".
1 participant
className 和 style 是所有 antd 组件都支持的的 config，他们的 merge 规则和普通属性不同，这里做特殊处理，以减少 antd 中的重复代码逻辑。
Summary by CodeRabbit
Bug Fixes
Tests