-
-
Notifications
You must be signed in to change notification settings - Fork 280
fix(Form.List): do not skip list root when no child field entities exist #773
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
base: master
Are you sure you want to change the base?
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. |
Walkthrough修改 getFieldsValue 对列表项的识别逻辑:新增对子字段存在性的检测,只有当列表项下确有子字段时才将其 namePath 计入 listNamePaths,避免在无子字段时错误返回空对象;并新增测试覆盖该场景。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
诗歌
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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! 此拉取请求旨在解决 Form.List 组件在特定场景下 getFieldsValue 方法行为不符合预期的问题。当 Form.List 实例虽然有数据但没有直接渲染子 Field 组件时,getFieldsValue 错误地返回空对象。通过调整 FormStore 内部处理逻辑,确保 Form.List 根节点仅在其拥有实际子字段时才被跳过,从而保证了 getFieldsValue 能够准确地获取到列表数据。 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. 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 修复了 Form.List 在没有子字段实体时 getFieldsValue 调用返回异常的问题。解决方案是仅当 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
🧹 Nitpick comments (2)
src/hooks/useForm.ts (2)
296-296: 建议使用英文注释以保持代码库一致性。代码中的中文注释与代码库中其他部分的英文注释风格不一致。建议将这些注释翻译为英文,以便于国际协作和维护。
🔎 建议的修改
- // 这里直接跳过跟节点是有bug的,getFieldsValue("list") 会返回空对象 + // Skipping the root node directly has a bug: getFieldsValue("list") returns an empty object const hasChild = fieldEntities.some(other => { if (other === entity) return false; const otherNamePath = other.INVALIDATE_NAME_PATH || other.getNamePath(); return ( otherNamePath.length > namePath.length && matchNamePath(otherNamePath as InternalNamePath, namePath as InternalNamePath, true) ); }); - // 只有当它确实有子节点时,才跳过根节点 + // Only skip the root node if it actually has children if (hasChild) {Also applies to: 306-306
297-304: 注意嵌套循环的性能影响。当前实现在
fieldEntities.forEach(line 290) 内部使用fieldEntities.some(line 297),产生 O(n²) 时间复杂度。在包含大量字段的表单中,这可能会影响性能。不过,考虑到这是修复 bug 的必要逻辑,且当前选择了"Chill"审查模式,这个性能影响是可接受的。如果后续收到性能反馈,可以考虑使用缓存或更高效的数据结构优化。
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/hooks/useForm.tstests/list.test.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
tests/list.test.tsx (1)
docs/examples/fieldTouched.tsx (1)
form(4-66)
src/hooks/useForm.ts (2)
src/utils/valueUtil.ts (1)
matchNamePath(53-67)src/interface.ts (1)
InternalNamePath(5-5)
🔇 Additional comments (2)
src/hooks/useForm.ts (1)
307-310: 修复逻辑正确!
hasChild检查正确地识别了列表根节点是否有实际的子 Field 实体:
- 通过长度比较确保子节点路径更长
- 使用
matchNamePath的partialMatch模式验证前缀匹配- 正确排除了自身比较
这个修改有效解决了
getFieldsValue(['list'])在列表没有子 Field 时返回空对象的问题。tests/list.test.tsx (1)
1143-1168: 测试用例设计优秀!这个测试用例完美验证了修复的 bug:
- 清晰地测试了 Form.List 包含值但没有子 Field 组件的边缘情况
- 使用原始 input 配合
setFieldValue模拟了没有 Field 实体的场景- 验证了初始状态和数据变更后
getFieldsValue(['list'])都能正确返回列表值而非空对象- 正确使用了
act和fireEvent处理异步操作测试覆盖了 PR 描述中提到的具体问题场景。
# Conflicts: # tests/list.test.tsx
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #773 +/- ##
==========================================
- Coverage 99.53% 99.46% -0.08%
==========================================
Files 19 19
Lines 1291 1298 +7
Branches 324 308 -16
==========================================
+ Hits 1285 1291 +6
- Misses 6 7 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| listNamePaths.push(namePath); | ||
| return; | ||
| // 这里直接跳过跟节点是有bug的,getFieldsValue("list") 会返回空对象 | ||
| const hasChild = fieldEntities.some(other => { |
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.
这个性能有点炸,如果一个大表单里的某个 List 没有 children 会导致全部遍历。其实最简单的应该是无论是否有 children 都初始化一个空的 array 就行了。剩下的让 children 自己去填。
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.
getFieldsValue(["list"])调用的时候fieldEntities不会有children;就会导致一致是[];
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 (1)
tests/list.test.tsx (1)
1207-1232: 测试覆盖了 PR 修复的核心场景,逻辑正确。该测试有效验证了当 Form.List 有值但没有子 Field 实体时,
getFieldsValue(['list'])能正确返回列表值。测试使用原生 input(而非 Field 组件)来模拟无子字段实体的场景,并通过setFieldValue更新值后再次验证,覆盖了修复的关键路径。💡 可选改进:添加注释说明测试意图
为提高可读性,可以在测试开头添加注释说明为何使用原生 input 而非 Field 组件:
it('getFieldsValue(["list"]) should keep list value when list has value but no child Field entities', async () => { + // Use raw input instead of Field to simulate a list with no child Field entities const [container] = generateForm( () => ( <input
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/list.test.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
tests/list.test.tsx (3)
src/Field.tsx (1)
render(660-681)docs/examples/renderProps.tsx (1)
render(12-57)docs/examples/fieldTouched.tsx (1)
form(4-66)
🔇 Additional comments (1)
tests/list.test.tsx (1)
1143-1173: 测试逻辑正确,验证了列表项更新时数据完整性。该测试有效验证了更新列表项字段时不会丢失其他列表项数据,并且 onValuesChange 能正确捕获包含所有列表项的完整值。
修复 Form.List 在无子字段时 getFieldsValue 异常的问题
问题
当前在
getFieldsValue处理中,Form.List的根节点(isList)会被直接跳过,默认由子 Field 补齐数据。
当
Form.List不存在任何子 Field 实体时,该逻辑会导致:getFieldsValue(['list'])返回空对象解决方案
仅当
Form.List根节点 确实存在子 Field 实体 时,才跳过该节点;若不存在子节点,则将其作为普通字段参与结果组装,保证 list 值正确返回。
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.