fix: getFieldsValue should return list root value when Form.List isn't wrapped #770
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修改了 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分钟
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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! This pull request addresses a specific bug in the form handling logic of 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
|
![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 addresses an issue where getFieldsValue would incorrectly return an empty array for a Form.List when its name was passed directly, instead of its actual value from the store. The new logic correctly retrieves the value from the main store, and only defaults to an empty array if the value is not found. This is a solid fix that improves the component's behavior. I have one minor suggestion to enhance code conciseness.
| mergedValues = setValue( | ||
| mergedValues, | ||
| namePath, | ||
| storeListValue === undefined ? [] : storeListValue, |
There was a problem hiding this comment.
For improved readability and conciseness, you can use the nullish coalescing operator (??) to provide a default value for storeListValue when it is null or undefined.
| storeListValue === undefined ? [] : storeListValue, | |
| storeListValue ?? [], |
There was a problem hiding this comment.
是 storeListValue === undefined 会漏 null, ??不会
There was a problem hiding this comment.
我的意思就是只想判断undefined;null的时候不处理成[]
|
请添加一个测试用例覆盖一下 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #770 +/- ##
=======================================
Coverage 99.53% 99.53%
=======================================
Files 19 19
Lines 1299 1302 +3
Branches 319 320 +1
=======================================
+ Hits 1293 1296 +3
Misses 6 6 ☔ 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: 0
🧹 Nitpick comments (1)
tests/list.test.tsx (1)
1143-1176: 建议增强测试以完全覆盖原始问题场景当前测试正确验证了
getFieldsValue(['list'])的返回值。但根据关联的 issue #56242，原始问题是form.getFieldsValue()和form.getFieldsValue(['list'])返回不同的值，而它们应该返回相等的值。建议添加断言来验证两种调用方式返回相同的结果，以更全面地覆盖修复场景。
在现有断言后添加以下代码：
expect(form.current?.getFieldsValue(['list'])).toEqual({ list: [{ name: 'n1', value: '1' }], }); + + // Verify both call patterns return the same result (issue #56242) + expect(form.current?.getFieldsValue()).toEqual( + form.current?.getFieldsValue(['list']) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/list.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/list.test.tsx (3)
src/List.tsx (1)
ListOperations(23-27)tests/common/InfoField.tsx (1)
Input(9-11)tests/common/index.ts (2)
changeValue(27-39)getInput(5-25)
|
@QDyanbing 你的这个 pr 我已经通过 patch node_modules 应用了，这个 getFieldsValue 的问题应该是修复了。但是又发现了 Form 的 onValuesChange 回调中，收到的 allValues 参数也不对的问题，对应字段也是 Form.List，调试了下发现 mergedAllValues 中数组从原来的 6 项变成了仅剩修改的那一项，所有别的字段也丢失了，可以帮忙看看嘛？应该是类似的修改方法？ |
|
https://codesandbox.io/p/devbox/jolly-platform-p68tsv 复现了 function Test() {
interface Fields {
list: { name: string; value: number }[];
}
let [alls, set_alls] = useState<Fields["list"]>([]);
return (
<>
<Form<Fields>
className=""
onValuesChange={(changeds, alls) => {
set_alls(alls.list);
}}
>
{/* 没有这个 Form.Item 的情况下，onValuesChange 的 alls 参数有问题 */}
{/* <Form.Item<Fields> name='list' label='list'> */}
<Form.List
name="list"
initialValue={[
{ name: "a", value: "1" },
{ name: "b", value: "2" },
]}
>
{(fields, { add }, { errors }) => (
<>
{fields.map(({ name, key }) => (
<div key={name} className="list-item">
<Form.Item name={[name, "name"]} label="name">
<Input />
</Form.Item>
<Form.Item name={[name, "value"]} label="value">
<Input />
</Form.Item>
</div>
))}
</>
)}
</Form.List>
{/* </Form.Item> */}
</Form>
<div>onValuesChange alls: {JSON.stringify(alls)}</div>
</>
);
}
|
|
好的，我看看 |
|
看起来应该和本次改动无关，明天我确认下这里的valuemerge逻辑 |
3 participants
fix: ant-design/ant-design#56242
Summary by CodeRabbit
发布说明
✏️ Tip: You can customize this high-level summary in your review settings.