-
Notifications
You must be signed in to change notification settings - Fork 192
feat: support custom merge #711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded@zombieJ has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
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在 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分钟
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 @zombieJ, 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! 此拉取请求引入了一个新的 deepMerge 工具函数,旨在增强对象的合并能力,尤其解决了在 Form.List 等组件中数组直接替换可能导致数据丢失的问题。新的函数允许在合并过程中自定义数组的准备逻辑,从而提供了更精细的控制并防止了意外的数据丢失。 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
这个 PR 引入了 deepMerge 功能以支持更灵活的对象合并策略,特别是针对数组的合并,解决了 Form.List 中数据可能被意外覆盖的问题。整体实现思路清晰,通过重构现有的 merge 函数来保持向后兼容性,这是一个很好的实践。我注意到一些可以改进的地方,主要在于提高代码的清晰度和可读性,具体请看我的评论。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #711 +/- ##
==========================================
+ Coverage 90.47% 90.53% +0.06%
==========================================
Files 37 37
Lines 903 909 +6
Branches 311 299 -12
==========================================
+ Hits 817 823 +6
Misses 84 84
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/utils.test.ts (1)
256-287: 测试逻辑正确,验证了自定义数组合并行为。测试用例有效验证了
prepareArray配置项的功能。不过prepareArray回调只使用了第一个参数srcVal,忽略了第二个参数tgtVal。考虑补充一个使用两个参数的测试用例,以更完整地展示 API 能力。示例补充测试:
it('deepMerge with prepareArray using both params', () => { const src = { list: [1, 2] }; const tgt = { list: [3, 4] }; const merged = deepMerge<any>([src, tgt], { prepareArray: (originVal, newVal) => { return [...(originVal || []), ...newVal]; }, }); expect(merged).toEqual({ list: [1, 2, 3, 4] }); });src/utils/set.ts (1)
70-70: 参数命名可以更清晰。
srcVal和tgtVal的命名容易与deepMerge的sources参数混淆。结合 line 107 的实际调用finalPrepareArray(originValue, value),建议改为更具描述性的名称:-export type MergeFn = (srcVal: any, tgtVal: any) => any; +export type MergeFn = (originValue: any, newValue: any) => any;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/utils/set.ts(3 hunks)tests/utils.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/utils.test.ts (1)
src/utils/set.ts (1)
deepMerge(79-126)
⏰ 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 (2)
tests/utils.test.ts (1)
3-3: LGTM!导入
deepMerge与现有的set和merge一起导入,符合新增功能的测试需求。src/utils/set.ts (1)
79-126: LGTM!
deepMerge实现逻辑正确:
- 默认
prepareArray返回空数组,保持与原merge行为一致- 循环引用检测通过
loopSet正确处理- 支持自定义数组合并策略,满足 PR 目标中 Form.List 的需求
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/utils/set.ts (1)
128-133: 补全 merge 的 JSDoc 并显式声明返回类型
merge的注释目前被截断为 “Merge objects which will create”,语义不完整,也没有参数 / 返回值说明。这一点和之前的 review 反馈是一致的。可以顺便把返回类型显式标出来,便于 IDE 和文档生成:
-/** - * Merge objects which will create - */ -export function merge<T extends object>(...sources: T[]) { - return deepMerge(sources); -} +/** + * Wrapper around {@link deepMerge} using the default array strategy. + * 默认情况下,数组会被后面的源对象覆盖。 + * + * @param sources 要从左到右合并的对象列表。 + * @returns 新的合并结果对象,不修改任何源对象。 + */ +export function merge<T extends object>(...sources: T[]): T { + return deepMerge(sources); +}
🧹 Nitpick comments (2)
src/utils/set.ts (2)
69-87: 补充 deepMerge 的 JSDoc 说明 prepareArray 语义
MergeFn与config.prepareArray已经支持自定义数组策略,但 JSDoc 目前只说明了“默认返回空数组会覆盖后面的数组”,没有明确回调参数current/next的含义,也没有标注返回值信息;对使用者来说不够直观。建议在 JSDoc 中补充参数和返回值说明,大致如下(文案可按项目惯例微调):
-/** - * Merge multiple objects. Support custom merge logic. - * @param sources object sources - * @param config.prepareArray Customize array prepare function. - * It will return empty [] by default. - * So when match array, it will auto be override with next array in sources. - */ +/** + * Deep merge multiple objects. Support custom merge logic. + * + * @param sources 对象源数组,从左到右依次合并。 + * @param config 可选的合并配置。 + * @param config.prepareArray + * 自定义数组容器的准备逻辑。 + * 参数 `current` 为当前结果中的旧值(可能为 `undefined`), + * `next` 为当前源对象上的数组值。 + * 默认实现始终返回新的空数组 `[]`,因此数组会被后面的源对象覆盖。 + * @returns 一个新的已合并对象,不会修改输入对象。 + */ export function deepMerge<T extends object>( sources: T[], config: { prepareArray?: MergeFn; } = {}, ) { const { prepareArray } = config; const finalPrepareArray: MergeFn = prepareArray || (() => []);
105-107: 更新数组分支注释以反映可自定义行为这里已经通过
finalPrepareArray(originValue, value)支持自定义数组策略,// Array will always be override注释只描述了默认行为,容易误导阅读者。建议改成更准确的说明,例如:
- if (isArr) { - // Array will always be override - clone = set(clone, path, finalPrepareArray(originValue, value)); + if (isArr) { + // Array container is prepared via `prepareArray` (default: override). + clone = set(clone, path, finalPrepareArray(originValue, value));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/set.ts(3 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/utils/set.ts (1)
128-134: LGTM!JSDoc 已完善,清晰说明了函数行为和数组默认替换语义。保持了向后兼容性的同时委托给
deepMerge实现。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/set.ts(3 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). (2)
- GitHub Check: test / react component workflow
- GitHub Check: test / react component workflow
🔇 Additional comments (2)
src/utils/set.ts (2)
69-71: LGTM!
MergeFn类型的参数命名current和next清晰明了,与 line 107 的调用finalPrepareArray(originValue, value)语义一致。
105-107: LGTM!数组合并逻辑正确,
finalPrepareArray(originValue, value)的调用顺序与MergeFn类型签名一致,默认行为(返回空数组)确保数组被后续源覆盖。
ref ant-design/ant-design#56102
在 react-component/field-form#761 中更新了调用逻辑,以裁剪输出内容。但是对于 Form.List 比较特殊的一个点则是对于 List 项目的增减操作是覆盖操作,而对于 List.Item 操作则是更新操作。所以 merge 直接的替换更新会导致丢失内容。这里添加一个函数以支持合并更新的逻辑。
Summary by CodeRabbit
发布说明
新功能
测试
✏️ Tip: You can customize this high-level summary in your review settings.