fix(form): [form] modifying form validation sorting issues in asynchronous situations #2982
Conversation
WalkthroughA new function Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant V as Validate Function
participant S as sortFields
participant CB as Callback
C ->> V: call validate()
V ->> S: sort(invalidFields, invalidFieldArr)
S -->> V: return sorted fields
V ->> CB: invoke callback (with sorted data)
Suggested reviewers
Poem
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/renderless/src/form/index.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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 (
|
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| const sortFields = (fileds, val) => { |
There was a problem hiding this comment.
The function name fileds seems to be a typo. It should be fields to maintain consistency and avoid confusion.
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/renderless/src/form/index.ts(2 hunks)
🔇 Additional comments (3)
packages/renderless/src/form/index.ts (3)
182-185: Good addition for sorting invalid fields.The sorting logic ensures consistent order of validation errors based on field declaration order, which will improve the user experience in asynchronous validation scenarios.
197-212: Consider edge cases in the sorting logic.The current implementation might not handle all scenarios correctly:
- What if a field's prop doesn't exist in
arrField? It will get an index of -1 which could lead to inconsistent sorting.- The function doesn't handle null or undefined values for
valConsider adding additional checks:
const sortFields = (fileds, val) => { + if (val === null || val === undefined) { + return val; + } const arrField = fileds.map((item) => item.prop) if (Object.prototype.toString.call(val) === '[object Object]') { const keys = Object.keys(val) - const sortKeys = keys.sort((a, b) => arrField.indexOf(a) - arrField.indexOf(b)) + const sortKeys = keys.sort((a, b) => { + const aIndex = arrField.indexOf(a); + const bIndex = arrField.indexOf(b); + // If either field is not found, maintain original order + if (aIndex === -1 && bIndex === -1) return 0; + if (aIndex === -1) return 1; // Move not found fields to the end + if (bIndex === -1) return -1; + return aIndex - bIndex; + }) const sortedObject = sortKeys.reduce((acc, index) => { acc[index] = val[index] return acc }, {}) return sortedObject } if (Array.isArray(val)) { - val.sort((x, y) => arrField.indexOf(x) - arrField.indexOf(y)) + val.sort((x, y) => { + const xIndex = arrField.indexOf(x); + const yIndex = arrField.indexOf(y); + // If either field is not found, maintain original order + if (xIndex === -1 && yIndex === -1) return 0; + if (xIndex === -1) return 1; // Move not found fields to the end + if (yIndex === -1) return -1; + return xIndex - yIndex; + }) return val } + return val; }
187-187:❓ Verification inconclusive
Modification to callback signature.
The callback now receives an additional third parameter (
sortFieldArr). This could potentially break existing code if any consumers expect only two parameters. Consider adding documentation about this change.
🏁 Script executed:
#!/bin/bash # Check for direct uses of the validate function with custom callbacks rg "validate\(\s*\([^)]+\)\s*=>" --type tsLength of output: 44
Callback Signature Change – Please Verify for Compatibility
The callback in the
validatefunction now receives an additional parameter (sortFieldArr), which may impact consumers expecting only two parameters. Although our initial search using an arrow function pattern forvalidatecallbacks did not return any hits, it's unclear if alternate usage patterns exist elsewhere in the codebase.
- Action Required: Manually verify consumer usages of the
validatefunction (or its callbacks) to ensure that no external component assumes a two-parameter signature. If any such usages are found, update documentation or consider a compatibility fallback.- Documentation Update: Add clear documentation about this change to guide future users and developers.
| const sortFields = (fileds, val) => { | ||
| const arrField = fileds.map((item) => item.prop) | ||
| if (Object.prototype.toString.call(val) === '[object Object]') { | ||
| const keys = Object.keys(val) | ||
| const sortKeys = keys.sort((a, b) => arrField.indexOf(a) - arrField.indexOf(b)) | ||
| const sortedObject = sortKeys.reduce((acc, index) => { | ||
| acc[index] = val[index] | ||
| return acc | ||
| }, {}) | ||
| return sortedObject | ||
| } | ||
| if (Array.isArray(val)) { | ||
| val.sort((x, y) => arrField.indexOf(x) - arrField.indexOf(y)) | ||
| return val | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the new sortFields function with proper types and parameter naming.
The function has several issues that should be addressed:
- Typo in parameter name (
filedsshould befields) - Missing TypeScript return type
- Potential side effect in array sorting
- Missing return for non-object/non-array values
-const sortFields = (fileds, val) => {
- const arrField = fileds.map((item) => item.prop)
+const sortFields = (fields, val): any => {
+ const arrField = fields.map((item) => item.prop)
if (Object.prototype.toString.call(val) === '[object Object]') {
const keys = Object.keys(val)
const sortKeys = keys.sort((a, b) => arrField.indexOf(a) - arrField.indexOf(b))
const sortedObject = sortKeys.reduce((acc, index) => {
acc[index] = val[index]
return acc
}, {})
return sortedObject
}
if (Array.isArray(val)) {
- val.sort((x, y) => arrField.indexOf(x) - arrField.indexOf(y))
- return val
+ return [...val].sort((x, y) => arrField.indexOf(x) - arrField.indexOf(y))
}
+ return val
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sortFields = (fileds, val) => { | |
| const arrField = fileds.map((item) => item.prop) | |
| if (Object.prototype.toString.call(val) === '[object Object]') { | |
| const keys = Object.keys(val) | |
| const sortKeys = keys.sort((a, b) => arrField.indexOf(a) - arrField.indexOf(b)) | |
| const sortedObject = sortKeys.reduce((acc, index) => { | |
| acc[index] = val[index] | |
| return acc | |
| }, {}) | |
| return sortedObject | |
| } | |
| if (Array.isArray(val)) { | |
| val.sort((x, y) => arrField.indexOf(x) - arrField.indexOf(y)) | |
| return val | |
| } | |
| } | |
| const sortFields = (fields, val): any => { | |
| const arrField = fields.map((item) => item.prop) | |
| if (Object.prototype.toString.call(val) === '[object Object]') { | |
| const keys = Object.keys(val) | |
| const sortKeys = keys.sort((a, b) => arrField.indexOf(a) - arrField.indexOf(b)) | |
| const sortedObject = sortKeys.reduce((acc, index) => { | |
| acc[index] = val[index] | |
| return acc | |
| }, {}) | |
| return sortedObject | |
| } | |
| if (Array.isArray(val)) { | |
| return [...val].sort((x, y) => arrField.indexOf(x) - arrField.indexOf(y)) | |
| } | |
| return val | |
| } |
|
这个光看标题也不知道在解决什么问题，描述可以用中文描述清楚点 |
Just looking at the title, you don't know what the problem is being solved. The description can be described clearly in Chinese. |
修改问题： |
Modify the problem: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/renderless/src/form/index.ts (3)
183-186: Performance optimization opportunity for field sorting.The current implementation sorts fields during each validation callback, which is inefficient in an asynchronous loop. Instead of sorting during each iteration, consider only sorting once at the end of the validation process to improve performance from O(N²logN) to O(NlogN).
state.fields.forEach((field) => { field.validate('', (message, field) => { if (message) { valid = false } invalidFields = merge({}, invalidFields, field) if (field) { Object.keys(field).forEach((item) => invalidFieldArr.push(item)) } if (typeof callback === 'function' && ++count === state.fields.length) { - // 排序 - const sortField = sortFields(state.fields, invalidFields) - const sortFieldArr = sortFields(state.fields, invalidFieldArr) - callback(valid, sortField, sortFieldArr) + // Sort only once at the end + const sortedInvalidFields = sortFields(state.fields, invalidFields) + const sortedInvalidFieldArr = sortFields(state.fields, invalidFieldArr) + callback(valid, sortedInvalidFields, sortedInvalidFieldArr) } }) })
183-183: Translate Chinese comment for better maintainability.Consider translating the Chinese comment "排序" to English (e.g., "Sort fields") to maintain consistency with the rest of the codebase and improve accessibility for all contributors.
- // 排序 + // Sort fields based on their declaration order
196-211: Missing TypeScript interface for the sortFields function.The function lacks proper type definitions for its parameters and return value, which can lead to type errors and unexpected behavior. Consider defining explicit interfaces for the parameters and return type.
+interface FormField { + prop: string; + [key: string]: any; +} + +/** + * Sorts field entries based on their order in the fields array + * @param fields Form field definitions containing prop property + * @param val Object or array of field entries to be sorted + * @returns Sorted object, array, or the original value if not sortable + */ -const sortFields = (fileds, val) => { +const sortFields = (fields: FormField[], val: any): any => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/renderless/src/form/index.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (2)
packages/renderless/src/form/index.ts (2)
196-211: Function typo and missing implementation details.The function name
filedsis misspelled and contains several implementation issues:
- Missing TypeScript type definitions
- Mutating the original array with direct sorting
- No handling for non-object/non-array values
- No explicit TypeScript return type
Apply the following improvements:
-const sortFields = (fileds, val) => { - const arrField = fileds.map((item) => item.prop) +const sortFields = (fields, val): any => { + const arrField = fields.map((item) => item.prop) if (Object.prototype.toString.call(val) === '[object Object]') { const keys = Object.keys(val) const sortKeys = keys.sort((a, b) => arrField.indexOf(a) - arrField.indexOf(b)) const sortedObject = sortKeys.reduce((acc, index) => { acc[index] = val[index] return acc }, {}) return sortedObject } if (Array.isArray(val)) { - val.sort((x, y) => arrField.indexOf(x) - arrField.indexOf(y)) - return val + return [...val].sort((x, y) => arrField.indexOf(x) - arrField.indexOf(y)) } + return val }
152-156:❓ Verification inconclusive
Verify Promise handling with the new callback parameters.
The Promise handling in the validate function might not properly handle the new callback signature with three parameters. The promise resolution currently only considers the first parameter (
valid), but the sorting changes have modified the callback signature.
🏁 Script executed:
#!/bin/bash # Check if there are any usages of the validate method that rely on Promise resolution # to ensure they work correctly with the modified callback parameters rg -A 2 "validate\(\)" --type ts --no-filename echo "--------------------" rg -A 2 "validate\(" --type ts --no-filename | grep -v "validate\(\)"Length of output: 1518
Action: Verify that the Promise resolution in
validatecorrectly handles the new three-parameter callback signature.The current implementation in
packages/renderless/src/form/index.ts(lines 152–156) only inspects the first argument (valid). With the updated callback now receiving three parameters, please double-check that:
- Any additional arguments that influence validation outcomes (e.g., error details or supplementary data) are either correctly propagated or intentionally ignored.
- Consumers of the
validate()promise (e.g., the chain using.then(() => true).catch(() => false)) are updated if they rely on information from the extra parameters, or that a comment is added to clarify the design decision.
PR
修改问题：
当提交表单，校验字段跳转出错的情况（异步校验情况下，比如第一个校验及最后一个校验字段都必填时，点击提交表单，优先提示最后一个字段未填写，而不是第一个字段），是由于未按照props属性的顺序，进行校验。
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Summary by CodeRabbit