feat(mergeProps): enhance mergeProps to handle className and style#745
feat(mergeProps): enhance mergeProps to handle className and style#745guoyunhe wants to merge 4 commits 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
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough更新 Changes
Sequence Diagram(s)（未生成：更改为单函数内部合并逻辑，交互方不足以构成序列图。） Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
诗句
🚥 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.
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 376 +4
==========================================
+ 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
…ding handling of undefined and null values
| for (const key of Object.keys(item)) { | ||
| if (item[key] !== undefined) { | ||
| ret[key] = item[key]; | ||
| if (key === 'className') { |
2 participants
className 和 style 是所有 antd 组件都支持的的 config，他们的 merge 规则和普通属性不同，这里做特殊处理，以减少 antd 中的重复代码逻辑。
Summary by CodeRabbit
Bug Fixes
Tests