perf: formApi added validateAndSubmitForm & filedMapToTime renamed fieldMappingTime #4842
Conversation
|
|
Warning Rate limit exceeded@anncwb has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 48 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. WalkthroughThe changes in this pull request primarily focus on enhancing the documentation and functionality of the Vben Form component, along with updates to related utilities and components. Key modifications include the addition of new properties and methods, such as Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (11)
packages/@core/base/shared/src/utils/date.ts (3)
1-1: Consider optimizing the dayjs import.For better tree-shaking and reduced bundle size, consider importing only the required functionality from dayjs.
-import dayjs from 'dayjs'; +import dayjs from 'dayjs/esm';
7-9: Consider making the datetime format configurable.The function provides a good standard format, but consider making it configurable through an optional parameter for flexibility.
-export function formatDateTime(time: number | string) { - return formatDate(time, 'YYYY-MM-DD HH:mm:ss'); +export function formatDateTime( + time: number | string, + format = 'YYYY-MM-DD HH:mm:ss' +) { + return formatDate(time, format); }
1-9: Add JSDoc documentation and unit tests.While the implementation is clean, please consider adding:
- JSDoc documentation for both functions describing parameters, return values, and examples
- Unit tests covering various scenarios including edge cases
Would you like me to help generate the documentation and unit test templates?
packages/@core/base/shared/package.json (1)
87-87: Consider using catalog versioning for dayjs.While adding dayjs is appropriate for date formatting utilities, consider using
"catalog:"instead of"^1.11.13"to maintain consistent version management with other dependencies in the monorepo.packages/@core/ui-kit/form-ui/src/components/form-actions.vue (2)
86-92: Consider making the date format more configurable.The hardcoded default format 'YYYY-MM-DD' might be too restrictive. Consider:
- Moving it to a constant or configuration
- Making it injectable through props
105-108: LGTM! Consider memoizing formatted dates.The date formatting implementation is clean and consistent. However, for performance optimization, consider memoizing the formatted dates if they're used in multiple places or if the component re-renders frequently.
playground/src/views/examples/form/basic.vue (1)
19-22: Add documentation for the fieldMappingTime configuration.While the configuration is valid, adding a comment would improve maintainability by explaining:
- The purpose of the mapping
- The expected input/output format
- The relationship between rangePicker and the mapped fields
Consider applying this change:
+ // Maps the rangePicker field to separate startTime and endTime fields + // Input: rangePicker array with 2 dates + // Output: { startTime: 'YYYY-MM-DD', endTime: 'YYYY-MM-DD' } fieldMappingTime: [['rangePicker', ['startTime', 'endTime'], 'YYYY-MM-DD']],Additionally, consider extracting the date format string into a constant to maintain consistency across the codebase.
packages/@core/ui-kit/form-ui/src/types.ts (1)
Line range hint
209-213: Add JSDoc comments to improve type documentation.The
FieldMappingTimetype appears to be used for mapping time-related fields, but its purpose and structure could be clearer with proper documentation.Add JSDoc comments to explain the purpose and structure:
+/** + * Type for mapping a single field into two time fields (e.g., start/end times) + * @example + * [ + * // Maps 'timeRange' field into 'startTime' and 'endTime' with 'HH:mm' format + * ['timeRange', ['startTime', 'endTime'], ['HH:mm', 'HH:mm']] + * ] + */ export type FieldMappingTime = [ string, [string, string], ([string, string] | string)?, ][];packages/@core/ui-kit/form-ui/src/form-api.ts (1)
245-252: LGTM! Consider adding JSDoc documentation.The implementation of
submitAndValidateFormis clean and follows good practices by validating before submission. However, it would benefit from JSDoc documentation to describe its purpose, return type, and behavior.Add documentation like this:
+/** + * Validates the form and submits it if validation passes. + * @returns {Promise<Record<string, any> | undefined>} The form values if validation and submission succeed, undefined otherwise. + */ async submitAndValidateForm() {docs/src/components/common-ui/vben-form.md (2)
283-283: Enhance documentation for submitAndValidateForm method.The description for the new
submitAndValidateFormmethod could be more detailed. Consider:
- Explaining how it differs from the existing
submitFormmethod- Clarifying the validation process and when it occurs
- Providing an example usage snippet
- Documenting the expected behavior when validation fails
Consider expanding the description like this:
-| submitAndValidateForm | 提交并校验表单 | `(e:Event)=>Promise<Record<string,any>>` | +| submitAndValidateForm | 提交并校验表单。在提交前进行表单验证，只有验证通过才会提交表单。验证失败时会返回rejected promise。 | `(e:Event)=>Promise<Record<string,any>>` |
313-313: Improve documentation for fieldMappingTime property.The description and type information for the new
fieldMappingTimeproperty needs enhancement:
- The type
[string, [string, string], string?][]is complex and needs explanation- The current description doesn't fully explain the mapping behavior
- Missing example usage
Consider expanding the documentation like this:
-| fieldMappingTime | 用于将表单内时间区域的应设成 2 个字段 | `[string, [string, string], string?][]` | - | +| fieldMappingTime | 用于将表单内时间区域映射成 2 个独立字段。每个映射配置包含：源字段名、目标字段名数组[开始时间字段, 结束时间字段]、可选的日期格式。例如：`[['dateRange', ['startDate', 'endDate'], 'YYYY-MM-DD']]` 会将 dateRange 字段的值分别映射到 startDate 和 endDate 字段。 | `[string, [string, string], string?][]` | - |Also consider adding a code example in the documentation:
// Example usage const [Form, formApi] = useVbenForm({ fieldMappingTime: [ // Maps dateRange field to startDate and endDate ['dateRange', ['startDate', 'endDate'], 'YYYY-MM-DD'], // Maps timeRange field to from and to ['timeRange', ['from', 'to'], 'HH:mm:ss'] ] });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
docs/src/components/common-ui/vben-form.md(2 hunks)packages/@core/base/shared/package.json(1 hunks)packages/@core/base/shared/src/utils/date.ts(1 hunks)packages/@core/base/shared/src/utils/index.ts(1 hunks)packages/@core/ui-kit/form-ui/src/components/form-actions.vue(4 hunks)packages/@core/ui-kit/form-ui/src/form-api.ts(1 hunks)packages/@core/ui-kit/form-ui/src/types.ts(2 hunks)packages/effects/plugins/src/vxe-table/extends.ts(2 hunks)playground/src/views/examples/form/basic.vue(1 hunks)playground/src/views/examples/modal/form-modal-demo.vue(1 hunks)playground/src/views/examples/modal/index.vue(1 hunks)playground/src/views/examples/vxe-table/form.vue(0 hunks)
💤 Files with no reviewable changes (1)
- playground/src/views/examples/vxe-table/form.vue
✅ Files skipped from review due to trivial changes (1)
- playground/src/views/examples/modal/index.vue
🔇 Additional comments (12)
packages/@core/base/shared/src/utils/index.ts (2)
2-2: LGTM! Export statement follows proper conventions.
The new export statement is correctly placed in alphabetical order and follows the same pattern as other exports.
2-2: Verify the date module integration.
Let's verify the existence of the date module and its usage in the form API changes.
✅ Verification successful
Let me check the actual content of these files to verify the integration properly.
Date module integration is properly implemented and used
The date module exports two utility functions (formatDate and formatDateTime) and is correctly integrated in the codebase:
- The module exists at the correct location
- It provides date formatting functionality using dayjs
- It's properly imported and used in form-actions.vue for formatting start and end times
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the date module integration
# Test 1: Check if date.ts exists in the utils directory
echo "Checking for date.ts file..."
fd --type f "date.ts" "packages/@core/base/shared/src/utils"
# Test 2: Search for usage of date formatting functions in form-related files
echo "Checking usage of date formatting functions..."
rg -l "formatDate|formatDateTime" "packages" | grep -i "form"
Length of output: 395
Script:
#!/bin/bash
# Check the content of date.ts and its usage in form-actions.vue
echo "Content of date.ts:"
cat packages/@core/base/shared/src/utils/date.ts
echo -e "\nUsage in form-actions.vue:"
rg "formatDate|formatDateTime" packages/@core/ui-kit/form-ui/src/components/form-actions.vue -C 2
Length of output: 800
packages/effects/plugins/src/vxe-table/extends.ts (2)
54-56: Verify date format consistency with previous implementation.
The implementation is cleaner, but we should ensure that formatDate and formatDateTime maintain the same format as before (YYYY-MM-DD and YYYY-MM-DD HH:mm:ss respectively).
Let's verify the format strings used in these utility functions:
#!/bin/bash
# Description: Verify the date format strings used in the utility functions
# Test: Search for format string definitions in the utility functions
rg -B 5 -A 5 'YYYY-MM-DD' --type tsAlso applies to: 60-62
5-5: LGTM! Verify utility function implementations.
The change to use utility functions instead of direct dayjs usage is a good architectural decision that can help reduce bundle size and provide better abstraction.
Let's verify the implementation of these utility functions:
✅ Verification successful
Verified: Date formatting utilities are properly implemented
The verification shows that:
- The
formatDateandformatDateTimefunctions are properly implemented inpackages/@core/base/shared/src/utils/date.ts - They use
dayjsinternally with correct format patterns - The functions are correctly exported from
@vben/utilsand imported in the VXE table extension
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of formatDate and formatDateTime utilities
# Test: Check the implementation of date formatting utilities
rg -A 10 "export function format(Date|DateTime)" --type ts
Length of output: 2586
Script:
#!/bin/bash
# Let's verify the implementation of formatDate and formatDateTime utilities with a more precise search
rg -A 5 "^export function format(Date|DateTime)" --type ts
# Also check where these utilities are imported from @vben/utils
rg -A 2 "from '@vben/utils'" --type ts
Length of output: 6108
playground/src/views/examples/modal/form-modal-demo.vue (2)
60-61: Clarify the intention behind commented modal close.
The modalApi.close() is commented out, which might lead to a poor user experience as the modal will stay open even after successful submission. If this is intentional (e.g., for showing submission feedback), consider adding a comment explaining the rationale. Otherwise, it should be uncommented and wrapped in proper error handling as suggested above.
60-61: Verify the existence of submitAndValidateForm method.
Let's ensure this new method is properly exported from the form API.
✅ Verification successful
The submitAndValidateForm method exists and is properly implemented
The method is correctly implemented in the FormApi class and performs validation before submission:
- It first validates the form using
form.validate() - If validation passes, it calls
submitForm() - If validation fails, it returns early
The commented-out modalApi.close() should be handled based on the submission result:
- On success: Modal should close
- On validation failure: Modal should stay open
Example usage:
const result = await formApi.submitAndValidateForm();
if (result) {
modalApi.close();
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of submitAndValidateForm
# Expected: Find the method definition in the form API files
# Search for the method definition
ast-grep --pattern 'submitAndValidateForm'
Length of output: 42
Script:
#!/bin/bash
# Let's try a more comprehensive search using ripgrep to find any occurrences
# of submitAndValidateForm in the codebase
rg -l "submitAndValidateForm"
# Also search for similar method names that might be related
rg -l "submitForm|validateForm"
# Let's also check the actual implementation of the form API
fd -e ts -e js -e vue "form.*api"
Length of output: 551
Script:
#!/bin/bash
# Let's check the implementation of submitAndValidateForm in the form API
cat packages/@core/ui-kit/form-ui/src/form-api.ts
# Also check the test file to understand the expected behavior
cat packages/@core/ui-kit/form-ui/__tests__/form-api.test.ts
Length of output: 13096
packages/@core/base/shared/package.json (1)
87-87: Verify the impact on bundle size.
The addition of dayjs as a new dependency might affect the bundle size. Consider using tree-shaking to import only the required functionality.
✅ Verification successful
Let me analyze the import patterns and dependency versions to better understand the tree-shaking situation and provide a more informed response.
Tree-shaking is already properly implemented for dayjs
The codebase shows proper tree-shaking practices for dayjs:
- Dynamic imports are used for locale files (
import('dayjs/locale/en')) - Specific plugin imports when needed (
import timezone from 'dayjs/plugin/timezone') - Simple imports for basic functionality (
import dayjs from 'dayjs')
Additionally, dayjs is already a dependency across multiple packages in the monorepo, and this addition in @core/base/shared follows the established pattern. The library itself is relatively lightweight (~2KB gzipped), and the current usage demonstrates good practices for minimizing bundle size.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the size impact of dayjs
# Look for existing dayjs imports in the codebase to understand usage patterns
rg -l 'import.*dayjs'
# Check if dayjs is already a dependency in other packages
fd -t f 'package.json' -x grep -l 'dayjs'
Length of output: 593
Script:
#!/bin/bash
# Check how dayjs is being imported in the files to verify tree-shaking usage
rg 'import.*dayjs' -A 2
# Check the specific dayjs dependencies in package.json files
fd -t f 'package.json' -x grep -A 1 -B 1 '"dayjs"'
Length of output: 4585
packages/@core/ui-kit/form-ui/src/components/form-actions.vue (2)
6-11: LGTM! Good consolidation of date formatting utilities.
The change to use formatDate from shared utils promotes consistency across the codebase.
72-74: Verify consistent renaming across the codebase.
The renaming from fieldMapToTime to fieldMappingTime looks correct here.
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information. The search results show that:
- There are no remaining instances of the old name
fieldMapToTime - The new name
fieldMappingTimeis consistently used across documentation, types, and component files
Renaming from fieldMapToTime to fieldMappingTime is consistently applied
The variable has been renamed consistently throughout the codebase, including:
- Type definitions in
packages/@core/ui-kit/form-ui/src/types.ts - Component logic in
packages/@core/ui-kit/form-ui/src/components/form-actions.vue - Documentation in
docs/src/components/common-ui/vben-form.md - Example usage in
playground/src/views/examples/form/basic.vue
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent renaming across the codebase
echo "Checking for any remaining instances of old name 'fieldMapToTime':"
rg "fieldMapToTime"
echo -e "\nChecking usage of new name 'fieldMappingTime':"
rg "fieldMappingTime"
Length of output: 1209
playground/src/views/examples/form/basic.vue (1)
19-22: Verify the integration with form submission and value setting.
Please ensure that:
- The mapped fields (startTime, endTime) are correctly populated in the form submission
- The handleSetFormValue function correctly handles the mapping when setting rangePicker values
Let's verify the integration:
✅ Verification successful
Based on the gathered information, I can now provide the final response:
The rangePicker integration with fieldMappingTime is properly configured
The code shows correct integration between the rangePicker and form submission:
- The fieldMappingTime is properly configured to map rangePicker values to startTime and endTime fields with the correct date format
- The handleSetFormValue function correctly sets rangePicker values using dayjs objects
- The onSubmit handler receives and processes the mapped form values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of fieldMappingTime to ensure consistent format
rg "fieldMappingTime.*YYYY-MM-DD" --type ts --type vue
# Search for form submission handlers to verify they handle the mapped fields
rg "startTime.*endTime" --type ts --type vue
Length of output: 159
Script:
#!/bin/bash
# Search for fieldMappingTime usage with correct file type
rg "fieldMappingTime.*YYYY-MM-DD"
# Search for rangePicker and related field mappings
rg "rangePicker.*startTime.*endTime"
# Search for form submission handling
rg "handleSubmit|onSubmit" -A 5
# Look for handleSetFormValue implementation
ast-grep --pattern 'handleSetFormValue($$$) {
$$$
}'
Length of output: 38755
Script:
#!/bin/bash
# Search for the form implementation in the basic.vue file
rg -A 20 "fieldMappingTime.*YYYY-MM-DD" playground/src/views/examples/form/basic.vue
# Search for onSubmit implementation in the same file
rg -A 10 "function onSubmit" playground/src/views/examples/form/basic.vue
# Look for form schema implementation
ast-grep --pattern 'schema: [
$$$
]'
Length of output: 881
Script:
#!/bin/bash
# Get the complete implementation of handleSetFormValue
rg -A 15 "function handleSetFormValue" playground/src/views/examples/form/basic.vue
# Look for rangePicker component in the schema
rg -B 5 -A 10 "component: .*rangePicker" playground/src/views/examples/form/basic.vue
Length of output: 557
Script:
#!/bin/bash
# Search for the complete schema array to find rangePicker configuration
rg -A 50 "schema: \[" playground/src/views/examples/form/basic.vue
# Look for any imports or utility functions related to field mapping
rg "fieldMapping|useVbenForm" playground/src/views/examples/form/basic.vue
Length of output: 2387
packages/@core/ui-kit/form-ui/src/types.ts (1)
314-317: Enhance property documentation and verify rename impact.
The documentation for this property could be more descriptive to help developers understand its usage.
Let's verify the rename from fieldMapToTime to fieldMappingTime:
Add more detailed JSDoc comments:
/**
* 表单字段映射成时间格式
+ * Maps a single field into two time fields (e.g., for time range inputs)
+ * @example
+ * fieldMappingTime: [
+ * ['timeRange', ['startTime', 'endTime'], 'HH:mm']
+ * ]
*/
fieldMappingTime?: FieldMappingTime;✅ Verification successful
Let me gather more information to verify if this is truly a rename or a new property addition by checking the git history and searching for any usage patterns.
Let me search for any implementations or usages of both the old and new property to ensure there are no remaining references that need updating.
Property rename has been properly implemented
The verification shows that:
- The rename from
fieldMapToTimetofieldMappingTimewas done correctly in commitcaf2cc4 - All implementations in
form-actions.vueare using the new property name - No remaining references to the old property name exist in the codebase
- Type definitions are properly updated
The suggested documentation improvement is still valid but not critical since the implementation in form-actions.vue clearly shows the property's purpose for handling time range fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old name
echo "Checking for old property name usage..."
rg "fieldMapToTime" --type ts
# Search for new property name usage to ensure consistency
echo "Checking new property name usage..."
rg "fieldMappingTime" --type ts
Length of output: 313
Script:
#!/bin/bash
# Check git history for the file to verify the rename
git log -p packages/@core/ui-kit/form-ui/src/types.ts | grep -A 5 -B 5 "fieldMap.*Time"
# Search for any form-related components or utilities that might use this property
echo "Checking form-related files..."
fd "form" -e ts -e tsx --exec rg -l "fieldMap.*Time" {}
# Search for test files that might reference this property
echo "Checking test files..."
fd "test|spec" -e ts -e tsx --exec rg -l "fieldMap.*Time" {}
Length of output: 1407
Script:
#!/bin/bash
# Search for implementations and usages in the form-ui directory
echo "Checking form-ui directory for implementations..."
rg -A 5 -B 5 "fieldMapToTime|fieldMappingTime" packages/@core/ui-kit/form-ui/
# Search for any type references or imports
echo "Checking for type references..."
rg -l "FieldMapToTime|FieldMappingTime" --type ts
# Check if there are any schema or configuration files that might reference these properties
echo "Checking for config files..."
fd . -e json -e yaml -e yml --exec rg -l "fieldMapToTime|fieldMappingTime" {}
Length of output: 3697
packages/@core/ui-kit/form-ui/src/form-api.ts (1)
245-252: Verify integration with modal forms.
Let's verify that this new method is properly integrated with modal forms as mentioned in the AI summary.
anncwb
changed the title
Description
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
Release Notes
New Features
fieldMappingTimeconfiguration for enhanced date handling in forms.submitAndValidateFormmethod for improved form submission with validation.Improvements
formatDateandformatDateTimefunctions.Bug Fixes
Refactor