Nc feat/form validation #8409
Nc feat/form validation #8409
Conversation
WalkthroughWalkthroughThe recent updates aim to enhance form functionalities and validation processes within the Changes
These changes reflect a concerted effort to improve form-related features and validation mechanisms, enhancing the overall user experience and application functionality. 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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 7
Out of diff range and nitpick comments (5)
packages/nc-gui/components/cell/PhoneNumber.vue (1)
[!TIP]
Codebase VerificationThe search results indicate that the identifier
isSurveyFormis still being used in multiple Vue components within thepackages/nc-gui/components/celldirectory. This contradicts the expectation that all occurrences should have been replaced byisForm. The presence ofisSurveyFormin these files suggests that the renaming might not have been fully implemented across all relevant components, or that both identifiers are being used for potentially different purposes.Files still using
isSurveyForm:
packages/nc-gui/components/cell/attachment/index.vuepackages/nc-gui/components/cell/SingleSelect.vuepackages/nc-gui/components/cell/RichText.vuepackages/nc-gui/components/cell/MultiSelect.vuepackages/nc-gui/components/cell/Checkbox.vueAnalysis chain
Line range hint
4-37: Renaming toisFormand updates to phone number validation logic.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of renaming `isSurveyForm` to `isForm` on other components. # Test: Search for occurrences of `isSurveyForm`. Expect: No occurrences, should be replaced by `isForm`. rg --type vue 'isSurveyForm' 'packages/nc-gui/components/cell'Length of output: 88
Script:
#!/bin/bash # Description: Verify the impact of renaming `isSurveyForm` to `isForm` on other components. # Test: Search for occurrences of `isSurveyForm`. Expect: No occurrences, should be replaced by `isForm`. rg 'isSurveyForm' 'packages/nc-gui/components/cell'Length of output: 1205
packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/survey.vue (2)
195-195: Consider adding documentation forhandleAddMissingRequiredFieldDefaultState.The function
handleAddMissingRequiredFieldDefaultStateis called without any comments or documentation. Adding a brief comment describing its purpose and usage can improve code readability and maintainability.
564-587: Ensure consistent styling rules in scoped and global styles.The component mixes scoped and global styles, which might lead to styling conflicts or unexpected behaviors. Consider reviewing the necessity of global styles or ensure that they are scoped properly to avoid affecting other components unintentionally.
packages/nc-gui/components/smartsheet/Form.vue (2)
250-250: Clarify the purpose ofclearValidatein comments for better maintainability.Consider adding a comment explaining the purpose of
clearValidateto improve code readability and maintainability.
1481-1498: Review the logic for disabling the switch based on field requirements.Consider reviewing the logic for disabling the switch based on field requirements to ensure it aligns with the intended functionality.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/nc-gui/assets/nc-icons/alert-triangle.svgis excluded by!**/*.svg,!**/*.svg
Files selected for processing (19)
- packages/nc-gui/components/cell/Currency.vue (3 hunks)
- packages/nc-gui/components/cell/Decimal.vue (2 hunks)
- packages/nc-gui/components/cell/Percent.vue (2 hunks)
- packages/nc-gui/components/cell/PhoneNumber.vue (4 hunks)
- packages/nc-gui/components/cell/attachment/index.vue (3 hunks)
- packages/nc-gui/components/smartsheet/Form.vue (22 hunks)
- packages/nc-gui/components/smartsheet/form/field-config-error.vue (1 hunks)
- packages/nc-gui/components/smartsheet/form/field-settings.vue (1 hunks)
- packages/nc-gui/composables/useFormViewStore.ts (1 hunks)
- packages/nc-gui/composables/useSharedFormViewStore.ts (14 hunks)
- packages/nc-gui/composables/useViewData.ts (1 hunks)
- packages/nc-gui/composables/useViewFilters.ts (1 hunks)
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vue (3 hunks)
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/survey.vue (9 hunks)
- packages/nc-gui/utils/formValidations.ts (1 hunks)
- packages/nc-gui/utils/iconUtils.ts (2 hunks)
- packages/nc-gui/utils/index.ts (1 hunks)
- packages/nocodb-sdk/src/lib/form.ts (1 hunks)
- packages/nocodb-sdk/src/lib/index.ts (1 hunks)
Files skipped from review due to trivial changes (5)
- packages/nc-gui/components/cell/Percent.vue
- packages/nc-gui/components/smartsheet/form/field-config-error.vue
- packages/nc-gui/composables/useViewFilters.ts
- packages/nc-gui/utils/iconUtils.ts
- packages/nocodb-sdk/src/lib/index.ts
Additional comments not posted (13)
packages/nc-gui/utils/index.ts (1)
28-28: Export offormValidationsmodule added.Verification successful
The verification process aimed to confirm the existence and correct implementation of the
formValidationsmodule in thepackages/nc-gui/utilsdirectory. The results from the executed scripts provide the following information:
- The
formValidations.tsfile exists within the specified directory, as confirmed by thefdcommand.- The
rgcommand, which was intended to detect basic syntax errors by searching for the phrase "syntax error" within theformValidations.tsfile, did not return any results. This suggests that there are no overt syntax errors labeled as such in the file.Given this information, it appears that the
formValidationsmodule exists and does not contain basic syntax errors as per the search criteria used. Therefore, the export addition in theindex.tsfile is correctly referencing an existing module without apparent syntax issues.Export of
formValidationsmodule verified.
- The module exists in the expected directory.
- No basic syntax errors were detected in the module.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and correct implementation of the `formValidations` module. # Test: Search for the `formValidations.ts` file. Expect: File should exist. fd --type file 'formValidations.ts' 'packages/nc-gui/utils' # Test: Check for basic syntax errors in the `formValidations.ts` file. rg --type ts -N 'syntax error' 'packages/nc-gui/utils/formValidations.ts'Length of output: 172
packages/nocodb-sdk/src/lib/form.ts (1)
1-83: Comprehensive addition of validation types and enums.packages/nc-gui/utils/formValidations.ts (1)
1-109: Addition of form validation functions for email, phone number, URL, and number inputs.packages/nc-gui/components/cell/Decimal.vue (1)
Line range hint
10-106: Addition ofplaceholderprop to enhance flexibility in decimal input handling.Verification successful
The verification of the
placeholderprop in theDecimal.vuefile has been successful. The prop is defined in the component's props and is used correctly in the template. The usage includes a conditional expression that checks ifplaceholderis defined; if not, it defaults to a localized string ifisEditColumnis true, otherwise, it defaults to an empty string. This ensures that the placeholder text is appropriately internationalized and dynamically set based on the component's state.Conclusion:
- The
placeholderprop is correctly defined and used within the component.- The prop's usage in the template includes proper internationalization.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the internationalization and usage of the `placeholder` prop in `Decimal.vue`. # Test: Search for usage of the `placeholder` prop in the template. Expect: Correct internationalization and usage. rg --type vue 'placeholder' 'packages/nc-gui/components/cell/Decimal.vue'Length of output: 99
Script:
#!/bin/bash # Description: Verify the internationalization and usage of the `placeholder` prop in `Decimal.vue`. # Test: Search for usage of the `placeholder` prop in the template. Expect: Correct internationalization and usage. rg 'placeholder' 'packages/nc-gui/components/cell/Decimal.vue'Length of output: 186
packages/nc-gui/components/cell/Currency.vue (1)
Line range hint
18-123: Addition ofplaceholderandhidePrefixprops to enhance flexibility in currency input handling.Verification successful
The verification of the
placeholderandhidePrefixprops in theCurrency.vuefile has been successful. The script output confirms that both properties are correctly utilized within the template:
- The
hidePrefixproperty is used in a conditional rendering statement to determine whether to display the currency prefix.- The
placeholderproperty is used to set the placeholder text of an input element, with a fallback to a localized string ifplaceholderis not defined.This confirms that the implementation aligns with the intended functionality as described in the review comment.
Correct Usage of
placeholderandhidePrefixProperties:
hidePrefixis used to conditionally hide the currency prefix in the component based on the property's value.placeholderprovides a dynamic placeholder text for the input field, which can default to a localized label if not specified.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct usage of `placeholder` and `hidePrefix` props in `Currency.vue`. # Test: Search for usage of the `placeholder` and `hidePrefix` props in the template. Expect: Correct usage and functioning of the prefix hiding logic. rg --type vue 'placeholder|hidePrefix' 'packages/nc-gui/components/cell/Currency.vue'Length of output: 113
Script:
#!/bin/bash # Description: Re-verify the correct usage of `placeholder` and `hidePrefix` props in `Currency.vue`. # Test: Search for usage of the `placeholder` and `hidePrefix` props in the template. Expect: Correct usage and functioning of the prefix hiding logic. rg 'placeholder|hidePrefix' 'packages/nc-gui/components/cell/Currency.vue'Length of output: 273
packages/nc-gui/components/smartsheet/form/field-settings.vue (1)
8-18: Addition ofupdateSelectFieldLayoutandcolumnSupportsScanningfunctions to enhance field configurability in forms.Verification successful
The verification of the
updateSelectFieldLayoutandcolumnSupportsScanningfunctions in thefield-settings.vuefile has been successful. The script output confirms that both functions are defined and used within the Vue template. Specifically:
- The
updateSelectFieldLayoutfunction is triggered by an event (@update:value), which is a common pattern in Vue.js for reacting to user inputs.- The
columnSupportsScanningfunction is used within a conditional rendering directive (v-if), which is appropriate for controlling the display of elements based on the type of the active field.This confirms that the functions are not only present but are also integrated into the component's logic as intended.
Conclusion:
The addition and usage ofupdateSelectFieldLayoutandcolumnSupportsScanningfunctions are correctly implemented in thefield-settings.vuefile. This enhances the configurability and functionality of the form fields as intended in the pull request.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct implementation and usage of `updateSelectFieldLayout` and `columnSupportsScanning` functions in `field-settings.vue`. # Test: Search for usage of these functions in the template. Expect: Correct implementation and usage. rg --type vue 'updateSelectFieldLayout|columnSupportsScanning' 'packages/nc-gui/components/smartsheet/form/field-settings.vue'Length of output: 154
Script:
#!/bin/bash # Description: Re-verify the correct implementation and usage of `updateSelectFieldLayout` and `columnSupportsScanning` functions in `field-settings.vue`. # Test: Search for usage of these functions in the template. Expect: Correct implementation and usage. rg 'updateSelectFieldLayout|columnSupportsScanning' packages/nc-gui/components/smartsheet/form/field-settings.vueLength of output: 393
packages/nc-gui/composables/useFormViewStore.ts (1)
1-138: Comprehensive management of form view state and utilities.Verification successful
The shell script output indicates that the hooks and utilities from
useFormViewStore.tsare being used in multiple composables within thepackages/nc-gui/composablesdirectory. Specifically,useFormanduseProvideFormViewStoreare utilized inuseFormViewStore.ts,useSharedFormViewStore.ts, anduseColumnCreateStore.ts. This suggests that the hooks are being reused across different modules, which is generally a good practice for maintaining consistency and reducing code duplication.However, the script was primarily focused on finding occurrences of
useFormanduseProvideFormViewStore. To fully verify the correct usage and performance of these hooks and utilities, we would need to inspect how they are integrated within each composable and ensure that there are no performance issues or incorrect implementations.Given the current output, there is no direct evidence of misuse or performance issues, but a deeper inspection into how these hooks are used in context would be necessary for a comprehensive verification.
Conclusion
The hooks and utilities from
useFormViewStore.tsare being reused in multiple composables, which is a positive sign of modular and reusable code. However, for a complete verification of correct usage and performance, further detailed code review or runtime profiling would be required.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct usage and performance of hooks and utilities in `useFormViewStore.ts`. # Test: Search for usage of hooks and utilities from `useFormViewStore.ts`. Expect: Correct usage and no performance issues. rg --type ts 'useForm|useProvideFormViewStore' 'packages/nc-gui/composables'Length of output: 1295
packages/nc-gui/components/cell/attachment/index.vue (1)
78-78: Ensure proper error handling forremoveFilefunction.packages/nc-gui/composables/useViewData.ts (1)
307-307: Ensure thatvalidatorsarray initialization is correctly implemented and used.Verification successful
The initialization and usage of the
validatorsarray in theuseViewDatacomposable appear to be correctly implemented. The array is initialized as empty and is designed to be overridden by metadata properties fromfieldByIdandc.meta, which is consistent with patterns observed in other parts of the codebase. This approach allows for dynamic handling of validation rules based on field-specific metadata, aligning with the design seen in related composables.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `validators` array is correctly initialized and used within the composable. # Test: Search for usage of `validators` array. Expect: Proper initialization and usage in validation logic. rg --type ts $'validators'Length of output: 1439
packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vue (1)
176-276: Review form validation logic to ensure it meets the requirements.Verification successful
The verification of the form validation logic in the file
packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vuehas been successful. The script output confirms the presence of validation logic within this specific file, as indicated by multiple instances of thevalidatefunction andvalidateInfosbeing used. This suggests that the form validation logic is indeed implemented and likely aligns with the requirements as it is actively used in the form component.
- The
validatefunction is called directly and also bound to UI events to ensure fields are validated on updates.- The
validateInfosobject is used to bind validation rules to form fields, indicating a structured approach to validation.This comprehensive use of validation mechanisms within the form component confirms that the form validation logic is implemented as expected.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the form validation logic is comprehensive and aligns with the requirements. # Test: Search for validation logic in the form component. Expect: Comprehensive validation rules that align with the requirements. rg --type vue $'validate'Length of output: 52
Script:
#!/bin/bash # Description: Verify that the form validation logic is comprehensive and aligns with the requirements. # Test: Search for validation logic in the form component. Expect: Comprehensive validation rules that align with the requirements. rg --type vue -g '*.vue' 'validate' .Length of output: 65
Script:
#!/bin/bash # Description: Verify that the form validation logic is comprehensive and aligns with the requirements. # Test: Search for validation logic in the form component. Expect: Comprehensive validation rules that align with the requirements. rg -g '*.vue' 'validate' .Length of output: 29555
packages/nc-gui/composables/useSharedFormViewStore.ts (1)
218-250: Ensure that the form validation rules are correctly implemented and cover all necessary cases.Verification successful
The form validation rules in the
useSharedFormViewStore.tsfile are implemented using a combination of computed properties and utility functions, which is consistent with other parts of the codebase. The use ofextractFieldValidatorto dynamically add validation rules based on column metadata suggests that the implementation is designed to be flexible and comprehensive.
- The
extractFieldValidatorfunction is utilized in multiple composables, indicating a shared approach to handling additional validation rules.- The use of computed properties for validators in various stores supports a reactive validation pattern, which is typical in Vue.js applications.
Overall, the implementation appears to be correctly set up to handle the necessary validation cases dynamically and reactively.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the form validation rules are correctly implemented and cover all necessary cases. # Test: Search for the implementation of form validation rules. Expect: Comprehensive and correct implementation of validation rules. rg --type ts $'validators'Length of output: 1439
packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/survey.vue (1)
377-495: Review the use of dynamic references in Vue templates.The template uses dynamic references such as
:key="field?.title"which can lead to re-rendering issues iffield.titlechanges unexpectedly. Verify that the field titles are stable or consider using a more stable property for keys.packages/nc-gui/components/smartsheet/Form.vue (1)
102-115: Ensure proper error handling inuseProvideFormViewStore.
|
Uffizzi Preview |
o1lab
force-pushed
the
nc-feat/form-validation
branch
from
6d943bf
to
0e30071
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Out of diff range and nitpick comments (1)
packages/nc-gui/composables/useSharedFormViewStore.ts (1)
43-43: Consider using a more specific import forForm.useFormto avoid potential confusion.It's generally a good practice to import specific functions directly from their modules to avoid confusion and potential errors in larger projects. Consider changing the import statement to something like
import { useForm } from 'ant-design-vue/es/form'.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/nc-gui/assets/nc-icons/alert-triangle.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/lang/en.jsonis excluded by!**/*.json
Files selected for processing (19)
- packages/nc-gui/components/cell/Currency.vue (3 hunks)
- packages/nc-gui/components/cell/Decimal.vue (2 hunks)
- packages/nc-gui/components/cell/Percent.vue (2 hunks)
- packages/nc-gui/components/cell/PhoneNumber.vue (4 hunks)
- packages/nc-gui/components/cell/attachment/index.vue (3 hunks)
- packages/nc-gui/components/smartsheet/Form.vue (22 hunks)
- packages/nc-gui/components/smartsheet/form/field-config-error.vue (1 hunks)
- packages/nc-gui/components/smartsheet/form/field-settings.vue (1 hunks)
- packages/nc-gui/composables/useFormViewStore.ts (1 hunks)
- packages/nc-gui/composables/useSharedFormViewStore.ts (14 hunks)
- packages/nc-gui/composables/useViewData.ts (1 hunks)
- packages/nc-gui/composables/useViewFilters.ts (1 hunks)
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vue (3 hunks)
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/survey.vue (9 hunks)
- packages/nc-gui/utils/formValidations.ts (1 hunks)
- packages/nc-gui/utils/iconUtils.ts (2 hunks)
- packages/nc-gui/utils/index.ts (1 hunks)
- packages/nocodb-sdk/src/lib/form.ts (1 hunks)
- packages/nocodb-sdk/src/lib/index.ts (1 hunks)
Files skipped from review as they are similar to previous changes (16)
- packages/nc-gui/components/cell/Currency.vue
- packages/nc-gui/components/cell/Decimal.vue
- packages/nc-gui/components/cell/Percent.vue
- packages/nc-gui/components/cell/PhoneNumber.vue
- packages/nc-gui/components/cell/attachment/index.vue
- packages/nc-gui/components/smartsheet/form/field-config-error.vue
- packages/nc-gui/components/smartsheet/form/field-settings.vue
- packages/nc-gui/composables/useFormViewStore.ts
- packages/nc-gui/composables/useViewData.ts
- packages/nc-gui/composables/useViewFilters.ts
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vue
- packages/nc-gui/utils/formValidations.ts
- packages/nc-gui/utils/iconUtils.ts
- packages/nc-gui/utils/index.ts
- packages/nocodb-sdk/src/lib/form.ts
- packages/nocodb-sdk/src/lib/index.ts
Additional comments not posted (7)
packages/nc-gui/composables/useSharedFormViewStore.ts (1)
553-553: Ensure thatclearValidateis called in all necessary scenarios.The
clearValidatefunction is called in several places, but it's important to ensure that it's being called in all scenarios where the form validation state needs to be reset. Consider adding additional calls toclearValidatewhere necessary.packages/nc-gui/components/smartsheet/Form.vue (6)
221-226: Ensure proper handling of undefined form fields.This comment is still valid as the proposed change ensures that undefined required fields are set to
nullonly if they fail validation.
228-228: Add error handling for thevalidatefunction to manage exceptions properly.This comment is still valid as it suggests adding error handling to manage exceptions during form validation.
616-616: Ensure thatvalidatefunction handles all potential exceptions.This comment is still valid as it suggests ensuring that all potential exceptions are handled in the
validatefunction.
221-228: The form submission logic appears correctly implemented.Ensure that the form validation and submission process is thoroughly tested, especially the new validation logic.
250-250: The form clearing logic is correctly implemented.Ensure that the form clearing process works as expected, especially in conjunction with the form reload functionality.
1481-1498: The field visibility toggle logic is correctly implemented.Ensure that the visibility toggle works as expected, especially in scenarios where fields are required or the form is locked.
| @@ -186,6 +196,7 @@ | |||
|
|
|||
| handlePreFillForm() | |||
| } catch (e: any) { | |||
| console.log('err', e) | |||
There was a problem hiding this comment.
Improve error handling by providing more specific error messages.
The error handling in the loadSharedView function logs a generic error message. It would be beneficial for debugging and user feedback to log more specific error messages based on the type of error encountered.
| const rulesObj: Record<string, RuleObject[]> = {} | ||
|
|
||
| if (!formColumns.value) return obj | ||
| if (!formColumns.value) return rulesObj | ||
|
|
||
| for (const column of formColumns.value) { | ||
| if ( | ||
| !isVirtualCol(column) && | ||
| ((column.rqd && !column.cdf) || (column.pk && !(column.ai || column.cdf)) || column.required) | ||
| ) { | ||
| obj.localState[column.title!] = { | ||
| required: fieldRequired(undefined, !!(column.uidt === UITypes.Checkbox && column.required)), | ||
| } | ||
| } else if ( | ||
| isLinksOrLTAR(column) && | ||
| column.colOptions && | ||
| (column.colOptions as LinkToAnotherRecordType).type === RelationTypes.BELONGS_TO | ||
| ) { | ||
| const col = columns.value?.find((c) => c.id === (column?.colOptions as LinkToAnotherRecordType)?.fk_child_column_id) | ||
| let rules: RuleObject[] = [] | ||
|
|
||
| rules.push({ | ||
| validator: (_rule: RuleObject, value: any) => { | ||
| return new Promise((resolve, reject) => { | ||
| if (isRequired(column)) { | ||
| if (column.uidt === UITypes.Checkbox && !value) { | ||
| return reject(t('msg.error.fieldRequired', { value: 'This field' })) | ||
| } else if (column.uidt !== UITypes.Checkbox) | ||
| if (value === null || !value?.length) { | ||
| return reject(t('msg.error.fieldRequired', { value: 'This field' })) | ||
| } | ||
| } | ||
| return resolve() | ||
| }) | ||
| }, | ||
| }) | ||
|
|
||
| if ((col && col.rqd && !col.cdf) || column.required) { | ||
| if (col) { | ||
| obj.virtual[column.title!] = { required: fieldRequired() } | ||
| } | ||
| } | ||
| } else if (isVirtualCol(column) && column.required) { | ||
| obj.virtual[column.title!] = { | ||
| minLength: minLength(1), | ||
| required: fieldRequired(), | ||
| } | ||
| const additionalRules = extractFieldValidator(parseProp(column.meta).validators ?? [], column) | ||
| rules = [...rules, ...additionalRules] | ||
|
|
||
| if (rules.length) { | ||
| rulesObj[column.title!] = rules | ||
| } | ||
| } | ||
|
|
||
| return rulesObj | ||
| }) |
There was a problem hiding this comment.
Optimize the validation rules setup for better performance and readability.
The setup for validation rules in the validators computed property is quite complex and could be optimized for better performance and readability. Consider breaking down the rule setup into smaller functions or using a more declarative approach to define the rules.
o1lab
force-pushed
the
nc-feat/form-validation
branch
from
0e30071
to
72b915e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/nc-gui/assets/nc-icons/alert-triangle.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/lang/en.jsonis excluded by!**/*.json
Files selected for processing (19)
- packages/nc-gui/components/cell/Currency.vue (3 hunks)
- packages/nc-gui/components/cell/Decimal.vue (2 hunks)
- packages/nc-gui/components/cell/Percent.vue (2 hunks)
- packages/nc-gui/components/cell/PhoneNumber.vue (4 hunks)
- packages/nc-gui/components/cell/attachment/index.vue (3 hunks)
- packages/nc-gui/components/smartsheet/Form.vue (22 hunks)
- packages/nc-gui/components/smartsheet/form/field-config-error.vue (1 hunks)
- packages/nc-gui/components/smartsheet/form/field-settings.vue (1 hunks)
- packages/nc-gui/composables/useFormViewStore.ts (1 hunks)
- packages/nc-gui/composables/useSharedFormViewStore.ts (14 hunks)
- packages/nc-gui/composables/useViewData.ts (1 hunks)
- packages/nc-gui/composables/useViewFilters.ts (1 hunks)
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vue (3 hunks)
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/survey.vue (9 hunks)
- packages/nc-gui/utils/formValidations.ts (1 hunks)
- packages/nc-gui/utils/iconUtils.ts (2 hunks)
- packages/nc-gui/utils/index.ts (1 hunks)
- packages/nocodb-sdk/src/lib/form.ts (1 hunks)
- packages/nocodb-sdk/src/lib/index.ts (1 hunks)
Files skipped from review as they are similar to previous changes (17)
- packages/nc-gui/components/cell/Currency.vue
- packages/nc-gui/components/cell/Decimal.vue
- packages/nc-gui/components/cell/Percent.vue
- packages/nc-gui/components/cell/PhoneNumber.vue
- packages/nc-gui/components/cell/attachment/index.vue
- packages/nc-gui/components/smartsheet/form/field-config-error.vue
- packages/nc-gui/components/smartsheet/form/field-settings.vue
- packages/nc-gui/composables/useFormViewStore.ts
- packages/nc-gui/composables/useViewData.ts
- packages/nc-gui/composables/useViewFilters.ts
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vue
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/survey.vue
- packages/nc-gui/utils/formValidations.ts
- packages/nc-gui/utils/iconUtils.ts
- packages/nc-gui/utils/index.ts
- packages/nocodb-sdk/src/lib/form.ts
- packages/nocodb-sdk/src/lib/index.ts
Additional comments not posted (4)
packages/nc-gui/composables/useSharedFormViewStore.ts (4)
149-163: Refactor default value setting to reduce complexity.This comment is still valid as the logic for setting default values based on the column type is quite verbose and repetitive. Consider creating a helper function that handles the setting of default values based on the column type to make this code cleaner and more maintainable.
199-199: Improve error handling by providing more specific error messages.This comment is still valid. The error handling in the
loadSharedViewfunction logs a generic error message. It would be beneficial for debugging and user feedback to log more specific error messages based on the type of error encountered.
218-250: Optimize the validation rules setup for better performance and readability.This comment is still valid as the setup for validation rules in the
validatorscomputed property is quite complex and could be optimized for better performance and readability. Consider breaking down the rule setup into smaller functions or using a more declarative approach to define the rules.
256-256: Ensure proper cleanup of form validation state.This comment is still valid. The
useFormhook is used here to manage form validation state, but there's no clear mechanism for cleaning up this state when the component is unmounted or when the form is reset. Consider adding a cleanup function to properly reset the validation state.
o1lab
force-pushed
the
nc-feat/form-validation
branch
5 times, most recently
from
b497075
to
8d4f141
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/nc-gui/assets/nc-icons/alert-triangle.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/lang/en.jsonis excluded by!**/*.json
Files selected for processing (17)
- packages/nc-gui/components/cell/Currency.vue (3 hunks)
- packages/nc-gui/components/cell/Decimal.vue (2 hunks)
- packages/nc-gui/components/cell/Percent.vue (2 hunks)
- packages/nc-gui/components/cell/PhoneNumber.vue (3 hunks)
- packages/nc-gui/components/cell/attachment/index.vue (2 hunks)
- packages/nc-gui/components/smartsheet/Form.vue (22 hunks)
- packages/nc-gui/components/smartsheet/form/field-config-error.vue (1 hunks)
- packages/nc-gui/components/smartsheet/form/field-settings.vue (1 hunks)
- packages/nc-gui/composables/useFormViewStore.ts (1 hunks)
- packages/nc-gui/composables/useSharedFormViewStore.ts (13 hunks)
- packages/nc-gui/composables/useViewData.ts (1 hunks)
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vue (3 hunks)
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/survey.vue (9 hunks)
- packages/nc-gui/utils/formValidations.ts (1 hunks)
- packages/nc-gui/utils/iconUtils.ts (2 hunks)
- packages/nocodb-sdk/src/lib/form.ts (1 hunks)
- packages/nocodb-sdk/src/lib/index.ts (1 hunks)
Files skipped from review as they are similar to previous changes (14)
- packages/nc-gui/components/cell/Currency.vue
- packages/nc-gui/components/cell/Decimal.vue
- packages/nc-gui/components/cell/Percent.vue
- packages/nc-gui/components/cell/PhoneNumber.vue
- packages/nc-gui/components/cell/attachment/index.vue
- packages/nc-gui/components/smartsheet/form/field-config-error.vue
- packages/nc-gui/components/smartsheet/form/field-settings.vue
- packages/nc-gui/composables/useFormViewStore.ts
- packages/nc-gui/composables/useViewData.ts
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vue
- packages/nc-gui/utils/formValidations.ts
- packages/nc-gui/utils/iconUtils.ts
- packages/nocodb-sdk/src/lib/form.ts
- packages/nocodb-sdk/src/lib/index.ts
Additional comments not posted (15)
packages/nc-gui/composables/useSharedFormViewStore.ts (6)
78-84: Ensure consistent filtering logic for form columns.
124-138: Refactor default value setting to reduce complexity.
174-174: Improve error handling by providing more specific error messages.
193-225: Optimize the validation rules setup for better performance and readability.
231-231: Ensure proper cleanup of form validation state.
528-528: Ensure thatclearValidateis called appropriately to reset validation states.packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/survey.vue (4)
20-34: Consider using TypeScript's utility types for destructuring with better type safety.
80-80: Add error handling for missingvalidateInfosentries.
112-118: RefactorvalidateFieldto handle exceptions more transparently.
196-196: Optimize the conditional checks inshowSubmitConfirmModal.packages/nc-gui/components/smartsheet/Form.vue (5)
194-199: Ensure proper handling of undefined form fields.
201-201: Add error handling for thevalidatefunction to manage exceptions properly.
589-589: Ensure thatvalidatefunction handles all potential exceptions.
223-223: Implementation ofclearFormlooks good.
223-223: Implementation ofonMovefunction looks good.
o1lab
force-pushed
the
nc-feat/form-validation
branch
from
8d4f141
to
aceeceb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Out of diff range and nitpick comments (1)
packages/nc-gui/components/smartsheet/Form.vue (1)
Line range hint
862-1136: Review the form layout and ensure accessibility and responsiveness.Consider reviewing the form layout to ensure it is accessible and responsive. This includes checking for proper use of ARIA attributes and ensuring that the form is usable on all devices and screen sizes.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/nc-gui/assets/nc-icons/alert-triangle.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/lang/en.jsonis excluded by!**/*.json
Files selected for processing (16)
- packages/nc-gui/components/cell/Currency.vue (3 hunks)
- packages/nc-gui/components/cell/Decimal.vue (2 hunks)
- packages/nc-gui/components/cell/Percent.vue (2 hunks)
- packages/nc-gui/components/cell/PhoneNumber.vue (3 hunks)
- packages/nc-gui/components/smartsheet/Form.vue (22 hunks)
- packages/nc-gui/components/smartsheet/form/field-config-error.vue (1 hunks)
- packages/nc-gui/components/smartsheet/form/field-settings.vue (1 hunks)
- packages/nc-gui/composables/useFormViewStore.ts (1 hunks)
- packages/nc-gui/composables/useSharedFormViewStore.ts (13 hunks)
- packages/nc-gui/composables/useViewData.ts (1 hunks)
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vue (3 hunks)
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/survey.vue (9 hunks)
- packages/nc-gui/utils/formValidations.ts (1 hunks)
- packages/nc-gui/utils/iconUtils.ts (2 hunks)
- packages/nocodb-sdk/src/lib/form.ts (1 hunks)
- packages/nocodb-sdk/src/lib/index.ts (1 hunks)
Files skipped from review as they are similar to previous changes (13)
- packages/nc-gui/components/cell/Currency.vue
- packages/nc-gui/components/cell/Decimal.vue
- packages/nc-gui/components/cell/Percent.vue
- packages/nc-gui/components/cell/PhoneNumber.vue
- packages/nc-gui/components/smartsheet/form/field-config-error.vue
- packages/nc-gui/components/smartsheet/form/field-settings.vue
- packages/nc-gui/composables/useFormViewStore.ts
- packages/nc-gui/composables/useViewData.ts
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vue
- packages/nc-gui/utils/formValidations.ts
- packages/nc-gui/utils/iconUtils.ts
- packages/nocodb-sdk/src/lib/form.ts
- packages/nocodb-sdk/src/lib/index.ts
Additional comments not posted (13)
packages/nc-gui/composables/useSharedFormViewStore.ts (7)
18-19: Consider removing the unused constantuseForm.
78-84: Ensure consistent filtering logic for form columns.
124-138: Refactor default value setting to reduce complexity.
174-174: Improve error handling by providing more specific error messages.
193-225: Optimize the validation rules setup for better performance and readability.
231-231: Ensure proper cleanup of form validation state.
528-528: Ensure thatclearValidateis called appropriately to reset validation states.packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/survey.vue (4)
20-34: Consider using TypeScript's utility types for destructuring with better type safety.
80-80: Add error handling for missingvalidateInfosentries.
112-118: RefactorvalidateFieldto handle exceptions more transparently.
196-196: Optimize the conditional checks inshowSubmitConfirmModal.packages/nc-gui/components/smartsheet/Form.vue (2)
201-201: Ensure robust error handling in thevalidatefunction.This is a duplicate of a previous comment made on an outdated commit, but it remains relevant. Proper error handling is crucial for maintaining the robustness of form validation processes.
589-589: Refactor error handling in thevalidatefunction to improve clarity and maintainability.This is a duplicate of a previous comment made on an outdated commit, but it remains relevant. Refactoring error handling can help in better understanding and maintaining the code, especially when dealing with form validations.
o1lab
force-pushed
the
nc-feat/form-validation
branch
from
aceeceb
to
bb51f2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/nc-gui/assets/nc-icons/alert-triangle.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/lang/en.jsonis excluded by!**/*.json
Files selected for processing (17)
- packages/nc-gui/components/cell/Currency.vue (3 hunks)
- packages/nc-gui/components/cell/Decimal.vue (2 hunks)
- packages/nc-gui/components/cell/Percent.vue (2 hunks)
- packages/nc-gui/components/cell/PhoneNumber.vue (3 hunks)
- packages/nc-gui/components/cell/attachment/index.vue (2 hunks)
- packages/nc-gui/components/smartsheet/Form.vue (22 hunks)
- packages/nc-gui/components/smartsheet/form/field-config-error.vue (1 hunks)
- packages/nc-gui/components/smartsheet/form/field-settings.vue (1 hunks)
- packages/nc-gui/composables/useFormViewStore.ts (1 hunks)
- packages/nc-gui/composables/useSharedFormViewStore.ts (13 hunks)
- packages/nc-gui/composables/useViewData.ts (1 hunks)
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vue (3 hunks)
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/survey.vue (9 hunks)
- packages/nc-gui/utils/formValidations.ts (1 hunks)
- packages/nc-gui/utils/iconUtils.ts (2 hunks)
- packages/nocodb-sdk/src/lib/form.ts (1 hunks)
- packages/nocodb-sdk/src/lib/index.ts (1 hunks)
Files skipped from review as they are similar to previous changes (14)
- packages/nc-gui/components/cell/Currency.vue
- packages/nc-gui/components/cell/Decimal.vue
- packages/nc-gui/components/cell/Percent.vue
- packages/nc-gui/components/cell/PhoneNumber.vue
- packages/nc-gui/components/cell/attachment/index.vue
- packages/nc-gui/components/smartsheet/form/field-config-error.vue
- packages/nc-gui/components/smartsheet/form/field-settings.vue
- packages/nc-gui/composables/useFormViewStore.ts
- packages/nc-gui/composables/useViewData.ts
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vue
- packages/nc-gui/utils/formValidations.ts
- packages/nc-gui/utils/iconUtils.ts
- packages/nocodb-sdk/src/lib/form.ts
- packages/nocodb-sdk/src/lib/index.ts
Additional comments not posted (17)
packages/nc-gui/composables/useSharedFormViewStore.ts (8)
15-15: Import ofRuleObjectfromant-design-vue/es/formadded.This import is necessary for the new validation logic introduced in the file.
78-84: Ensure consistent filtering logic for form columns.This comment is still valid as the complexity in the filtering logic remains. Consider simplifying this logic or breaking it down into smaller, more testable functions.
124-138: Refactor default value setting to reduce complexity.This comment is still valid as the logic for setting default values based on the column type is verbose and repetitive. Consider creating a helper function that handles the setting of default values based on the column type to make this code cleaner and more maintainable.
174-174: Improve error handling by providing more specific error messages.This comment is still valid as the error handling in the
loadSharedViewfunction logs a generic error message. It would be beneficial for debugging and user feedback to log more specific error messages based on the type of error encountered.
193-225: Optimize the validation rules setup for better performance and readability.This comment is still valid as the setup for validation rules in the
validatorscomputed property is quite complex and could be optimized for better performance and readability. Consider breaking down the rule setup into smaller functions or using a more declarative approach to define the rules.
231-231: Ensure proper cleanup of form validation state.This comment is still valid as there's no clear mechanism for cleaning up the validation state when the component is unmounted or when the form is reset. Consider adding a cleanup function to properly reset the validation state.
528-528: Ensure thatclearValidateis called appropriately to reset validation states.This comment is still valid. The call to
clearValidateshould be accompanied by a comment explaining its purpose, especially in a complex function likewatch. This helps maintain clarity in the code's intention, particularly for future maintainers or during debugging.
18-19: Consider removing the unused constantuseForm.This comment is still valid. This line defines
useFormbut it is not used anywhere in the provided code. Removing unused code helps in maintaining cleaner and more readable codebases.packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/survey.vue (4)
20-34: Consider using TypeScript's utility types for destructuring with better type safety.This comment is still valid as the destructuring of
useSharedFormStoreOrThrow()could benefit from TypeScript's utility types to ensure that all properties are correctly typed. This can prevent runtime errors due to incorrect property accesses and improve maintainability.
80-80: Add error handling for missingvalidateInfosentries.This comment is still valid as the code assumes that
validateInfos[field.value.title]will always be valid. Consider adding a check or a default value to handle cases wherevalidateInfosdoes not have an entry for a given title to prevent runtime errors.
112-118: RefactorvalidateFieldto handle exceptions more transparently.This comment is still valid as the catch block in
validateFieldfunction uses a catch-all typeanyfor the exception. It's a good practice to avoid usinganyand instead use a more specific error type or at least log the error for debugging purposes.- } catch (_e: any) { + } catch (_e: Error) { + console.error("Validation failed:", _e); return false }
196-196: Optimize the conditional checks inshowSubmitConfirmModal.This comment is still valid as the function
showSubmitConfirmModalperforms multiple checks in a single line, which can be refactored for better readability and efficiency. Consider separating these conditions or restructuring the function to early return on simpler conditions.packages/nc-gui/components/smartsheet/Form.vue (5)
194-199: Consider initializingformStateproperties tonullonly if they are required and currently undefined.
201-201: Add error handling for thevalidatefunction to manage exceptions properly.
223-223: EnsureclearValidateis called only when necessary.
589-589: Ensure thatvalidatefunction handles all potential exceptions.
1454-1471: Ensure that the logic for showing or hiding columns is clear and handles all edge cases.
o1lab
force-pushed
the
nc-feat/form-validation
branch
2 times, most recently
from
7c5eb55
to
e263a8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Out of diff range and nitpick comments (1)
packages/nc-gui/components/smartsheet/Form.vue (1)
Line range hint
860-1134: Review the form rendering logic for potential improvements in clarity and performance.Consider simplifying the form rendering logic to improve readability and potentially enhance performance. This could involve breaking down complex conditional structures into smaller, more manageable components or using more computed properties to handle visibility and styling.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/nc-gui/assets/nc-icons/alert-triangle.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/lang/en.jsonis excluded by!**/*.json
Files selected for processing (17)
- packages/nc-gui/components/cell/Currency.vue (3 hunks)
- packages/nc-gui/components/cell/Decimal.vue (2 hunks)
- packages/nc-gui/components/cell/Percent.vue (2 hunks)
- packages/nc-gui/components/cell/PhoneNumber.vue (3 hunks)
- packages/nc-gui/components/cell/attachment/index.vue (2 hunks)
- packages/nc-gui/components/smartsheet/Form.vue (22 hunks)
- packages/nc-gui/components/smartsheet/form/field-config-error.vue (1 hunks)
- packages/nc-gui/components/smartsheet/form/field-settings.vue (1 hunks)
- packages/nc-gui/composables/useFormViewStore.ts (1 hunks)
- packages/nc-gui/composables/useSharedFormViewStore.ts (12 hunks)
- packages/nc-gui/composables/useViewData.ts (1 hunks)
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vue (3 hunks)
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/survey.vue (9 hunks)
- packages/nc-gui/utils/formValidations.ts (1 hunks)
- packages/nc-gui/utils/iconUtils.ts (2 hunks)
- packages/nocodb-sdk/src/lib/form.ts (1 hunks)
- packages/nocodb-sdk/src/lib/index.ts (1 hunks)
Files skipped from review as they are similar to previous changes (14)
- packages/nc-gui/components/cell/Currency.vue
- packages/nc-gui/components/cell/Decimal.vue
- packages/nc-gui/components/cell/Percent.vue
- packages/nc-gui/components/cell/PhoneNumber.vue
- packages/nc-gui/components/cell/attachment/index.vue
- packages/nc-gui/components/smartsheet/form/field-config-error.vue
- packages/nc-gui/components/smartsheet/form/field-settings.vue
- packages/nc-gui/composables/useFormViewStore.ts
- packages/nc-gui/composables/useViewData.ts
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vue
- packages/nc-gui/utils/formValidations.ts
- packages/nc-gui/utils/iconUtils.ts
- packages/nocodb-sdk/src/lib/form.ts
- packages/nocodb-sdk/src/lib/index.ts
Additional comments not posted (12)
packages/nc-gui/composables/useSharedFormViewStore.ts (6)
15-15: Added import forRuleObjectfromant-design-vue/es/form.This import is necessary for the new validation logic introduced in this file.
18-19: Consider removing the unused constantuseForm.This line defines
useFormbut it is not used anywhere in the provided code. Removing unused code helps in maintaining cleaner and more readable codebases.
124-138: Ensure correct handling of default values based on column types.The logic to handle default values based on different column types (e.g., Number, Checkbox) is implemented correctly.
192-224: Refactor the validation rules setup for better performance and readability.The setup for validation rules in the
validatorscomputed property is quite complex and could be optimized for better performance and readability. Consider breaking down the rule setup into smaller functions or using a more declarative approach to define the rules.
226-229: Ensure thatvalidationFieldStatecorrectly aggregates state for validation.This computed property correctly combines
formStateandadditionalStatefor validation purposes.
566-576: Ensure that the watcher foradditionalStatecorrectly triggers validation.The watcher setup for
additionalStateis correctly configured to trigger validation when the state changes.packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/survey.vue (6)
20-34: Ensure that destructured imports fromuseSharedFormStoreOrThroware used correctly.The destructured imports are used throughout the component, ensuring that the component's functionality is maintained with the new store structure.
80-80: Validate thatvalidateInfosis correctly accessed for error status.The access pattern for
validateInfosto check the validation status of a field is implemented correctly.
112-118: Ensure thatvalidateFieldfunction handles validation correctly.The
validateFieldfunction correctly uses thevalidatemethod to check the validation status of a field and handles exceptions appropriately.
165-165: Ensure thatclearValidateis called appropriately inresetForm.The
clearValidatemethod is called to reset the validation state when the form is reset, which is a correct usage.
182-182: Ensure thathandleAddMissingRequiredFieldDefaultStateis called at the start of the form interaction.Calling
handleAddMissingRequiredFieldDefaultStateat the start ensures that any missing required fields are initialized properly.
364-482: Ensure that the form is structured correctly and all bindings are accurate.The form structure and bindings, including dynamic references to
formStateand validation information, are correctly implemented.
| for (const col of visibleColumns.value) { | ||
| if (isRequired(col, col.required) && formState.value[col.title] === undefined) { | ||
| formState.value[col.title] = null | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Initialization of form state fields to null should be conditional.
for (const col of visibleColumns.value) {
if (isRequired(col, col.required) && formState.value[col.title] === undefined) {
+ if (isRequired(col, col.required)) {
formState.value[col.title] = null
+ }
}
}This change ensures that only required fields that are not already defined get initialized to null, preventing unnecessary overwrites.
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.
| for (const col of visibleColumns.value) { | |
| if (isRequired(col, col.required) && formState.value[col.title] === undefined) { | |
| formState.value[col.title] = null | |
| } | |
| } | |
| for (const col of visibleColumns.value) { | |
| if (isRequired(col, col.required) && formState.value[col.title] === undefined) { | |
| if (isRequired(col, col.required)) { | |
| formState.value[col.title] = null | |
| } | |
| } | |
| } |
| try { | ||
| await formRef.value?.validateFields() | ||
| await validate([...Object.keys(formState.value)]) |
There was a problem hiding this comment.
Add error handling for the validate function to manage exceptions properly.
await validate([...Object.keys(formState.value)])
+ } catch (e) {
+ console.error('Validation failed:', e);
+ throw e;
} catch (e: any) {
if (e.errorFields.length) {
message.error(t('msg.error.someOfTheRequiredFieldsAreEmpty'))
return
}
}This change adds a more generic error handler for the validate function, ensuring that any type of exception is caught and logged, improving the robustness of the form submission process.
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.
| await validate([...Object.keys(formState.value)]) | |
| await validate([...Object.keys(formState.value)]) | |
| } catch (e) { | |
| console.error('Validation failed:', e); | |
| throw e; | |
| } catch (e: any) { | |
| if (e.errorFields.length) { | |
| message.error(t('msg.error.someOfTheRequiredFieldsAreEmpty')) | |
| return | |
| } | |
| } |
| @@ -234,7 +220,7 @@ | |||
|
|
|||
| formState.value = {} | |||
| state.value = {} | |||
| await formRef.value?.clearValidate() | |||
| clearValidate() | |||
There was a problem hiding this comment.
Ensure clearValidate is called only when necessary.
clearValidate()
+ if (shouldClearValidation) {
clearValidate()
+ }Consider adding a condition to check whether clearing the validation state is necessary based on the current form state or other conditions. This prevents unnecessary operations and potential side effects.
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.
| clearValidate() | |
| if (shouldClearValidation) { | |
| clearValidate() | |
| } |
| } catch (e: any) { | ||
| e.errorFields.map((f: Record<string, any>) => console.error(f.errors.join(','))) | ||
| } | ||
| await validate([...Object.keys(formState.value)]) |
There was a problem hiding this comment.
Ensure that validate function handles all potential exceptions.
await validate([...Object.keys(formState.value)])
+ } catch (e) {
+ console.error('Validation error:', e);
+ throw e;
} catch (e: any) {
e.errorFields.map((f: Record<string, any>) => console.error(f.errors.join(',')))
}This modification ensures that all exceptions from the validate function are properly handled, providing better error reporting and control flow management.
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.
| await validate([...Object.keys(formState.value)]) | |
| await validate([...Object.keys(formState.value)]) | |
| } catch (e) { | |
| console.error('Validation error:', e); | |
| throw e; | |
| } catch (e: any) { | |
| e.errorFields.map((f: Record<string, any>) => console.error(f.errors.join(','))) | |
| } |
| const formColumns = computed( | ||
| () => | ||
| columns.value | ||
| ?.filter((c) => c.show) | ||
| .filter( | ||
| (col) => !isSystemColumn(col) && col.uidt !== UITypes.SpecificDBType && (!isVirtualCol(col) || isLinksOrLTAR(col.uidt)), | ||
| ) || [], |
There was a problem hiding this comment.
Optimize the filtering logic in formColumns computed property.
- .filter((col) => !isSystemColumn(col) && col.uidt !== UITypes.SpecificDBType && (!isVirtualCol(col) || isLinksOrLTAR(col.uidt)),
+ .filter((col) => !isSystemColumn(col) && col.uidt !== UITypes.SpecificDBType && (!isVirtualCol(col) || isLinksOrLTAR(col.uidt))),Ensure that the filtering conditions are correctly grouped to avoid logical errors.
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.
| const formColumns = computed( | |
| () => | |
| columns.value | |
| ?.filter((c) => c.show) | |
| .filter( | |
| (col) => !isSystemColumn(col) && col.uidt !== UITypes.SpecificDBType && (!isVirtualCol(col) || isLinksOrLTAR(col.uidt)), | |
| ) || [], | |
| const formColumns = computed( | |
| () => | |
| columns.value | |
| ?.filter((c) => c.show) | |
| .filter( | |
| (col) => !isSystemColumn(col) && col.uidt !== UITypes.SpecificDBType && (!isVirtualCol(col) || isLinksOrLTAR(col.uidt))), | |
| ) || [], |
| <NcTooltip | ||
| :disabled="!field.required || isLocked || !isEditable" | ||
| class="flex" | ||
| placement="topRight" | ||
| > | ||
| <template #title> You can't hide a required field.</template> | ||
| <a-switch | ||
| :checked="!!field.show" | ||
| :disabled="field.required || isLocked || !isEditable" | ||
| class="flex-none nc-switch" | ||
| size="small" | ||
| @change=" | ||
| (value) => { | ||
| showOrHideColumn(field, value, true) | ||
| } | ||
| " | ||
| /> | ||
| </NcTooltip> |
There was a problem hiding this comment.
Ensure that the logic for showing or hiding columns is clear and handles all edge cases.
showOrHideColumn(field, value, true)
+ if (canModifyField(field)) {
showOrHideColumn(field, value, true)
+ }Adding a check to ensure that the field can be modified before attempting to show or hide it can prevent runtime errors and ensure that the UI behaves as expected.
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.
| <NcTooltip | |
| :disabled="!field.required || isLocked || !isEditable" | |
| class="flex" | |
| placement="topRight" | |
| > | |
| <template #title> You can't hide a required field.</template> | |
| <a-switch | |
| :checked="!!field.show" | |
| :disabled="field.required || isLocked || !isEditable" | |
| class="flex-none nc-switch" | |
| size="small" | |
| @change=" | |
| (value) => { | |
| showOrHideColumn(field, value, true) | |
| } | |
| " | |
| /> | |
| </NcTooltip> | |
| <NcTooltip | |
| :disabled="!field.required || isLocked || !isEditable" | |
| class="flex" | |
| placement="topRight" | |
| > | |
| <template #title> You can't hide a required field.</template> | |
| <a-switch | |
| :checked="!!field.show" | |
| :disabled="field.required || isLocked || !isEditable" | |
| class="flex-none nc-switch" | |
| size="small" | |
| @change=" | |
| (value) => { | |
| if (canModifyField(field)) { | |
| showOrHideColumn(field, value, true) | |
| } | |
| } | |
| " | |
| /> | |
| </NcTooltip> |
| function isRequired(column: Record<string, any>) { | ||
| if (!isVirtualCol(column) && ((column.rqd && !column.cdf) || (column.pk && !(column.ai || column.cdf)) || column.required)) { | ||
| return true | ||
| } else if ( | ||
| isLinksOrLTAR(column) && | ||
| column.colOptions && | ||
| (column.colOptions as LinkToAnotherRecordType).type === RelationTypes.BELONGS_TO | ||
| ) { | ||
| const col = columns.value?.find((c) => c.id === (column?.colOptions as LinkToAnotherRecordType)?.fk_child_column_id) | ||
|
|
||
| if ((col && col.rqd && !col.cdf) || column.required) { | ||
| if (col) { | ||
| return true | ||
| } | ||
| } | ||
| } else if (isVirtualCol(column) && column.required) { | ||
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Refactor the isRequired function to improve readability and maintainability.
- if (!isVirtualCol(column) && ((column.rqd && !column.cdf) || (column.pk && !(column.ai || column.cdf)) || column.required)) {
+ if (!isVirtualCol(column) && (column.required || (column.rqd && !column.cdf) || (column.pk && !(column.ai || column.cdf)))) {Reorder conditions for clarity and ensure that all necessary checks are performed in a logical sequence.
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.
| function isRequired(column: Record<string, any>) { | |
| if (!isVirtualCol(column) && ((column.rqd && !column.cdf) || (column.pk && !(column.ai || column.cdf)) || column.required)) { | |
| return true | |
| } else if ( | |
| isLinksOrLTAR(column) && | |
| column.colOptions && | |
| (column.colOptions as LinkToAnotherRecordType).type === RelationTypes.BELONGS_TO | |
| ) { | |
| const col = columns.value?.find((c) => c.id === (column?.colOptions as LinkToAnotherRecordType)?.fk_child_column_id) | |
| if ((col && col.rqd && !col.cdf) || column.required) { | |
| if (col) { | |
| return true | |
| } | |
| } | |
| } else if (isVirtualCol(column) && column.required) { | |
| return true | |
| } | |
| return false | |
| } | |
| function isRequired(column: Record<string, any>) { | |
| if (!isVirtualCol(column) && (column.required || (column.rqd && !column.cdf) || (column.pk && !(column.ai || column.cdf)))) { | |
| return true | |
| } else if ( | |
| isLinksOrLTAR(column) && | |
| column.colOptions && | |
| (column.colOptions as LinkToAnotherRecordType).type === RelationTypes.BELONGS_TO | |
| ) { | |
| const col = columns.value?.find((c) => c.id === (column?.colOptions as LinkToAnotherRecordType)?.fk_child_column_id) | |
| if ((col && col.rqd && !col.cdf) || column.required) { | |
| if (col) { | |
| return true | |
| } | |
| } | |
| } else if (isVirtualCol(column) && column.required) { | |
| return true | |
| } | |
| return false | |
| } |
o1lab
force-pushed
the
nc-feat/form-validation
branch
from
e263a8a
to
e809d0d
Compare
…er custom validation
o1lab
force-pushed
the
nc-feat/form-validation
branch
from
e809d0d
to
a33dade
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/nc-gui/assets/nc-icons/alert-triangle.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/lang/en.jsonis excluded by!**/*.json
Files selected for processing (17)
- packages/nc-gui/components/cell/Currency.vue (3 hunks)
- packages/nc-gui/components/cell/Decimal.vue (2 hunks)
- packages/nc-gui/components/cell/Percent.vue (2 hunks)
- packages/nc-gui/components/cell/PhoneNumber.vue (3 hunks)
- packages/nc-gui/components/cell/attachment/index.vue (2 hunks)
- packages/nc-gui/components/smartsheet/Form.vue (24 hunks)
- packages/nc-gui/components/smartsheet/form/field-config-error.vue (1 hunks)
- packages/nc-gui/components/smartsheet/form/field-settings.vue (1 hunks)
- packages/nc-gui/composables/useFormViewStore.ts (1 hunks)
- packages/nc-gui/composables/useSharedFormViewStore.ts (12 hunks)
- packages/nc-gui/composables/useViewData.ts (1 hunks)
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vue (3 hunks)
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/survey.vue (9 hunks)
- packages/nc-gui/utils/formValidations.ts (1 hunks)
- packages/nc-gui/utils/iconUtils.ts (2 hunks)
- packages/nocodb-sdk/src/lib/form.ts (1 hunks)
- packages/nocodb-sdk/src/lib/index.ts (1 hunks)
Files skipped from review as they are similar to previous changes (14)
- packages/nc-gui/components/cell/Currency.vue
- packages/nc-gui/components/cell/Decimal.vue
- packages/nc-gui/components/cell/Percent.vue
- packages/nc-gui/components/cell/PhoneNumber.vue
- packages/nc-gui/components/cell/attachment/index.vue
- packages/nc-gui/components/smartsheet/form/field-config-error.vue
- packages/nc-gui/components/smartsheet/form/field-settings.vue
- packages/nc-gui/composables/useFormViewStore.ts
- packages/nc-gui/composables/useViewData.ts
- packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/index.vue
- packages/nc-gui/utils/formValidations.ts
- packages/nc-gui/utils/iconUtils.ts
- packages/nocodb-sdk/src/lib/form.ts
- packages/nocodb-sdk/src/lib/index.ts
Additional comments not posted (18)
packages/nc-gui/composables/useSharedFormViewStore.ts (7)
15-15: Added import forRuleObjectfromant-design-vue/es/form.This import is necessary for the new validation logic introduced in this file.
78-84: Optimized filtering logic informColumnscomputed property.The filtering conditions are correctly grouped, ensuring logical correctness and improved readability.
124-138: Introduced default value handling for various field types in the form state initialization.This change ensures that default values are correctly parsed and set based on the field type, enhancing the robustness of form initialization.
192-224: Refactored validation rules setup invalidatorscomputed property.The new setup is more modular and easier to understand, improving maintainability and performance of the validation logic.
226-229: AddedvalidationFieldStatecomputed property to manage the state of fields during validation.This addition helps in maintaining a consistent state across validations, improving the reliability of form validations.
232-247: IntroducedhandleAddMissingRequiredFieldDefaultStatefunction to handle missing required fields.This function ensures that all required fields have a default state before validation, which is crucial for maintaining form integrity.
531-550: RefactoredisRequiredfunction to improve readability and maintainability.The conditions are reordered for clarity, and all necessary checks are performed in a logical sequence, enhancing the function's reliability.
packages/nc-gui/pages/index/[typeOrId]/form/[viewId]/index/survey.vue (6)
20-34: Imported and destructured various properties and functions fromuseSharedFormStoreOrThrow.These imports are necessary for the form functionality and are correctly destructured for use within the component.
80-80: Added error handling for form fields infieldHasErrorcomputed property.This addition improves user feedback by correctly identifying and displaying errors related to form fields.
112-118: RefactoredvalidateFieldfunction to handle field validation asynchronously.This change allows for asynchronous validation of fields, improving the responsiveness and accuracy of form validations.
165-165: AddedclearValidatecall inresetFormfunction to reset form validations.This ensures that validations are correctly reset when the form is reset, maintaining the integrity of form state.
182-182: CalledhandleAddMissingRequiredFieldDefaultStateinonStartfunction to ensure all required fields are initialized.This call is crucial for ensuring that all required fields are properly initialized before the form starts, enhancing form reliability.
364-482: Implemented a dynamic form layout with conditional rendering and validation.The dynamic layout allows for flexible form configurations and robust validation handling, improving the user experience and form functionality.
packages/nc-gui/components/smartsheet/Form.vue (5)
194-199: Initialization of form state fields tonullshould be conditional.
201-201: Add error handling for thevalidatefunction to manage exceptions properly.
223-223: EnsureclearValidateis called only when necessary.
589-589: Ensure thatvalidatefunction handles all potential exceptions.
1452-1469: Ensure that the logic for showing or hiding columns is clear and handles all edge cases.
Change Summary
Provide summary of changes with issue number if any.
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of