fix(web): enhance validator of required multiple values #1328
Conversation
WalkthroughThe changes in this pull request enhance the validation logic across multiple input components in the application. New properties such as Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
✅ Deploy Preview for reearth-cms ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (16)
web/src/components/molecules/Content/Form/fields/utils.ts (2)
3-4: Consider handling additional empty value casesThe function handles basic empty cases but could be enhanced to handle more edge cases:
- Empty arrays (
[])- Empty objects (
{})- Whitespace-only strings
Consider this enhanced implementation:
export const checkIfEmpty = (value: unknown) => - value === undefined || value === null || value === ""; + value === undefined || + value === null || + value === "" || + (typeof value === "string" && value.trim() === "") || + (Array.isArray(value) && value.length === 0) || + (value instanceof Object && + Object.keys(value).length === 0);
1-11: Consider evolving this into a robust validation frameworkThe current implementation provides a good foundation for form validation. For future scalability, consider:
- Creating a validation registry for custom validators
- Adding composition support for combining validators
- Implementing a validation context for sharing validation state
- Adding validation rule builders for complex validation scenarios
Example future structure:
// validation-registry.ts export class ValidationRegistry { static register(name: string, validator: ValidatorFn): void static get(name: string): ValidatorFn } // validation-builder.ts export class ValidationBuilder { required(): this custom(fn: ValidatorFn): this compose(...validators: ValidatorFn[]): ValidatorFn }Would you like me to create a GitHub issue to track these future improvements?
web/src/components/atoms/TextArea/index.tsx (2)
12-20: Simplify status logic and handle default caseThe status logic can be simplified and should explicitly handle the default case.
Consider this refactor:
const status = useMemo(() => { - if ( - isError || - (required && !value) || - (maxLength && value && runes(value).length > maxLength) - ) { - return "error"; - } + return ( + isError || + (required && !value) || + (maxLength && value && runes(value).length > maxLength) + ) ? "error" : undefined; }, [required, isError, maxLength, value]);
24-27: Document Unicode handling with runesThe use of
runesfor character counting is important for proper Unicode handling, but it's not documented.Consider adding a comment explaining the Unicode handling:
count={{ max: maxLength, + // Use runes for accurate Unicode character counting + // This ensures proper length validation for characters like emojis strategy: txt => runes(txt).length, }}web/src/components/atoms/Input/index.tsx (1)
Line range hint
6-9: Consider explicitly declaring therequiredprop in Props typeFor better code readability and type documentation, consider explicitly declaring the
requiredprop in the Props type interface.type Props = { value?: string; isError?: boolean; + required?: boolean; } & InputProps;web/src/components/molecules/Content/Form/fields/FieldComponents/DateField.tsx (2)
27-30: Consider improving error message specificityWhile the validation logic is correct, the error message could be more specific to date fields and handle both single and multiple value scenarios.
{ required: field.required, validator: requiredValidator, - message: t("Please input field!"), + message: field.multiple + ? t("Please input at least one date value!") + : t("Please input a date value!"), },
Line range hint
37-45: Enhance accessibility and error state visibilityConsider adding visual indicators for required fields and validation errors to improve user experience.
{field.multiple ? ( <MultiValueField onChange={onMetaUpdate} type="date" FieldInput={StyledDatePicker} disabled={disabled} + status={form.getFieldError([field.id, itemGroupId])?.length ? 'error' : undefined} + required={field.required} /> ) : ( <StyledDatePicker onChange={onMetaUpdate} disabled={disabled} + status={form.getFieldError(field.id)?.length ? 'error' : undefined} + required={field.required} /> )}Note: This suggestion assumes the DatePicker component supports
statusandrequiredprops. Please verify component API compatibility before implementing.web/src/components/molecules/Content/Form/fields/FieldComponents/SelectField.tsx (2)
27-31: Consider different validation messages for single/multiple selectThe validation looks good, but consider improving the user experience by providing distinct error messages for single and multiple select scenarios.
rules={[ { required: field.required, validator: requiredValidator, - message: t("Please select an option!"), + message: field.multiple + ? t("Please select at least one option!") + : t("Please select an option!"), }, ]}
Line range hint
10-14: Consider enhancing type safety for field propThe
Fieldtype could be more specific to ensure type safety for select field properties.type DefaultFieldProps = { - field: Field; + field: Field & { + typeProperty?: { + values?: string[]; + }; + }; itemGroupId?: string; disabled: boolean; };web/src/components/molecules/Content/Form/fields/FieldComponents/URLField.tsx (1)
Line range hint
33-46: Consider combining validation rules for better UXThe URL validation logic looks solid, but having separate error messages for "required" and "invalid URL" might lead to a confusing user experience when both conditions fail. Consider combining these validations to show the most relevant error first.
Here's a suggested approach:
rules={[ { required: field.required, validator: requiredValidator, message: t("Please input field!"), }, { message: t("URL is not valid"), - validator: async (_, value) => { + validator: async (_, value) => { + if (!value || (Array.isArray(value) && value.every(v => !v))) { + return Promise.resolve(); + } if (value) { if ( Array.isArray(value) && value.some((valueItem: string) => !validateURL(valueItem) && valueItem.length > 0) ) return Promise.reject(); else if (!Array.isArray(value) && !validateURL(value) && value?.length > 0) return Promise.reject(); } return Promise.resolve(); }, }, ]}web/src/components/molecules/Content/Form/fields/FieldComponents/NumberField.tsx (1)
46-48: Consider improving the error message specificityThe current error message "Please input field!" is generic. Consider making it more specific to number fields and multiple values context.
- message: t("Please input field!"), + message: t("Please input a valid number"),web/src/components/molecules/Content/Form/fields/FieldComponents/GeometryField.tsx (2)
42-45: Consider improving the error message specificityThe current error message "Please input field!" is generic. Consider making it more specific to geometry fields for better user experience.
Suggested improvement:
- message: t("Please input field!"), + message: t("Please input geometry data!"),
Line range hint
53-67: Consider splitting single and multiple value handlingThe component currently handles both single and multiple geometry values with the same validation rules. Consider splitting these into separate components or hooks for better separation of concerns and easier testing.
This would allow for:
- More specific validation logic for each case
- Clearer error handling
- Better testability
- Reduced complexity in the main component
web/src/components/atoms/Markdown/index.tsx (2)
17-25: Consider enhancing the validation logicWhile the current implementation works, there are opportunities for improvement:
- The empty check could be more robust to handle whitespace-only strings
- The logic could be more readable with early returns
Consider this refactoring:
const isError = useMemo(() => { - if (props.isError || (props.required && !value)) { - return true; - } else if (props.maxLength && value) { - return runes(value).length > props.maxLength; - } else { - return false; - } + if (props.isError) return true; + + const trimmedValue = value?.trim() ?? ''; + if (props.required && !trimmedValue) return true; + + if (props.maxLength && trimmedValue) { + return runes(trimmedValue).length > props.maxLength; + } + + return false; }, [props, value]);This refactoring:
- Handles whitespace-only strings properly
- Makes the logic more readable with early returns
- Maintains the same functionality while being more robust
Line range hint
77-79: Consider accessibility improvements for error statesThe error state styling uses color alone to indicate errors (
#ff4d4ffor text). Consider adding additional visual indicators (like icons) and ensuring the color contrast meets WCAG standards for accessibility.web/src/components/molecules/Content/Form/fields/ComplexFieldComponents/AssetField.tsx (1)
79-81: Consider making the error message more specificThe validation implementation looks good and properly handles both single and multiple asset cases. However, the error message "Please input field!" could be more specific to asset fields.
Consider making the error message more descriptive:
- message: t("Please input field!"), + message: t("Please select at least one asset!"),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
web/src/components/atoms/Input/index.tsx(1 hunks)web/src/components/atoms/Markdown/index.tsx(1 hunks)web/src/components/atoms/TextArea/index.tsx(1 hunks)web/src/components/molecules/Common/MultiValueField/index.tsx(3 hunks)web/src/components/molecules/Content/Form/fields/ComplexFieldComponents/AssetField.tsx(2 hunks)web/src/components/molecules/Content/Form/fields/FieldComponents/DateField.tsx(2 hunks)web/src/components/molecules/Content/Form/fields/FieldComponents/DefaultField.tsx(3 hunks)web/src/components/molecules/Content/Form/fields/FieldComponents/GeometryField.tsx(2 hunks)web/src/components/molecules/Content/Form/fields/FieldComponents/MarkdownField.tsx(3 hunks)web/src/components/molecules/Content/Form/fields/FieldComponents/NumberField.tsx(2 hunks)web/src/components/molecules/Content/Form/fields/FieldComponents/SelectField.tsx(2 hunks)web/src/components/molecules/Content/Form/fields/FieldComponents/TextareaField.tsx(3 hunks)web/src/components/molecules/Content/Form/fields/FieldComponents/URLField.tsx(2 hunks)web/src/components/molecules/Content/Form/fields/utils.ts(1 hunks)
🔇 Additional comments (30)
web/src/components/molecules/Content/Form/fields/utils.ts (1)
1-1: LGTM!
The import statement is clean and properly typed.
web/src/components/atoms/TextArea/index.tsx (2)
7-7: LGTM: Clean type definition for error state
The addition of the optional isError prop follows TypeScript best practices and integrates well with the existing type structure.
14-15: Verify required validation behavior in parent components
The required field validation has been added, but we should verify its usage in parent components, especially in multiple value scenarios mentioned in the PR objectives.
✅ Verification successful
Let me gather more information about the MultiValueField component and its interaction with TextArea.
Let me check the implementation of MultiValueField and TextareaField components to understand how they handle the required validation.
Let me check the TextareaField component implementation specifically.
Based on the implementation analysis, I can now provide the final response:
Required validation is properly implemented in parent components
The TextArea's required validation is correctly integrated:
- TextareaField component properly passes the required prop to both single and multi-value scenarios
- Form.Item includes proper validation rules with required validator
- MultiValueField component handles required validation for multiple values by checking if all values are empty
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TextArea usage in parent components
# particularly focusing on required prop and multiple value handling
ast-grep --pattern 'TextArea required={$_}'
# Look for MultiValueField components that might use TextArea
rg -A 5 "MultiValueField.*TextArea"
Length of output: 789
Script:
#!/bin/bash
# Check MultiValueField implementation
ast-grep --pattern 'const MultiValueField = $_'
# Look for TextArea imports and props interface
rg -A 5 "import.*TextArea"
# Check Form.Item validation related code around TextArea
ast-grep --pattern 'Form.Item $$$>{
$$$
<TextArea
$$$
/>'
Length of output: 13100
Script:
#!/bin/bash
# Check TextareaField implementation for required validation
ast-grep --pattern 'const TextareaField = $_'
# Check MultiValueField implementation for validation
cat web/src/components/molecules/Common/MultiValueField/index.tsx
# Look for Form.Item validation rules around TextArea
rg -A 5 "rules=\[.*required" web/src/components/molecules/Content/Form/fields/FieldComponents/TextareaField.tsx
Length of output: 4273
Script:
#!/bin/bash
# Check TextareaField implementation
cat web/src/components/molecules/Content/Form/fields/FieldComponents/TextareaField.tsx
# Look for validation related code in the parent Form component
rg -A 10 "Form.Item.*TextArea" web/src/components/molecules/Content/Form/fields/FieldComponents/TextareaField.tsx
Length of output: 2386
web/src/components/atoms/Input/index.tsx (2)
24-35: LGTM! Well-structured component rendering
The component rendering is well implemented with:
- Proper prop forwarding
- Correct ref handling
- Unicode-aware character counting
12-37: Verify integration with multiple value fields
Since this PR aims to enhance validation for required multiple values, let's verify how this Input component is used in multiple value contexts.
✅ Verification successful
Based on the search results, I can now provide a final response regarding the integration with multiple value fields:
Multiple value handling is already properly integrated
The Input component is correctly integrated with multiple value contexts through the MultiValueField wrapper component, as evidenced in several files:
web/src/components/molecules/Schema/FieldModal/FieldDefaultInputs/TextField/index.tsxshows proper integration using<MultiValueField FieldInput={Input} showCount maxLength={maxLength} />web/src/components/molecules/Schema/FieldModal/FieldDefaultInputs/URLField/index.tsxdemonstrates similar integration with{multiple ? <MultiValueField FieldInput={Input} /> : <Input />}
The validation logic in the Input component correctly handles both single and multiple value scenarios through its parent components, and the required field validation is properly implemented in the component's logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components that might use this Input component in a multiple value context
rg -l "MultipleValue.*Input" --type tsx
# Search for test files that verify multiple value validation
rg -l "test.*multiple.*value.*required" --type tsx --type test
Length of output: 162
Script:
#!/bin/bash
# Search for components that might use this Input component in a multiple value context
rg -l "MultipleValue.*Input"
# Search for test files that verify multiple value validation
rg -l "test.*multiple.*value.*required"
# Look for usages of Input component with required prop
rg "Input.*required" -A 2 -B 2
# Search for test files related to Input component
fd "Input.test" -e tsx -e ts -e js
Length of output: 692
Script:
#!/bin/bash
# Look for Input component usage patterns
rg "<Input" -A 3 -B 3
# Check for any Input component tests in the components directory
fd "Input" --type file web/src/components
# Look for form validation related tests
rg "describe.*Input" -A 5
Length of output: 30125
web/src/components/molecules/Content/Form/fields/FieldComponents/DateField.tsx (1)
10-10: Verify the implementation of requiredValidator
The imported validator is crucial for the enhanced validation logic. Let's verify its implementation to ensure it properly handles empty values in both single and multiple value scenarios.
✅ Verification successful
Implementation of requiredValidator is correct and robust
The validator implementation is well-designed for both single and multiple value scenarios:
- Properly handles undefined, null, and empty string values through
checkIfEmpty - For arrays, validates that not all values are empty using
value.every - Returns Promise.reject() for invalid cases and Promise.resolve() for valid ones
While there aren't specific tests for the validator, its implementation is straightforward and follows validation best practices for form fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of requiredValidator in utils.ts
# Expected: Function that properly validates both single and multiple empty values
# Find and display the utils.ts file content
fd -e ts utils.ts -x cat {} | grep -A 10 "requiredValidator"
Length of output: 600
Script:
#!/bin/bash
# Let's check the implementation of checkIfEmpty function as it's crucial for validation
fd -e ts utils.ts -x cat {} | grep -A 5 "checkIfEmpty"
# Also check if there are any tests for these validators
fd -e test.ts -e spec.ts utils -x cat {}
Length of output: 1324
web/src/components/molecules/Content/Form/fields/FieldComponents/SelectField.tsx (1)
8-8: Verify the requiredValidator implementation
The import statement looks correct, but let's ensure the validator implementation handles both single and multiple select cases appropriately.
✅ Verification successful
Let me verify the checkIfEmpty function as well to ensure it properly handles all cases.
Import and implementation of requiredValidator are correct
The validator implementation is well-designed:
- Properly handles both single values and arrays through
Array.isArraycheck - Uses
checkIfEmptyhelper that covers undefined, null, and empty string cases - Returns Promises as required by antd Form validation
- Correctly validates each value in an array using
every
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the requiredValidator implementation in utils.ts
# Expected: Find the validator implementation and confirm it handles array values
# Search for the requiredValidator implementation
ast-grep --pattern 'export const requiredValidator = $_' web/src/components/molecules/Content/Form/fields/utils.ts
# Check if the validator handles array values
rg -A 10 "requiredValidator" web/src/components/molecules/Content/Form/fields/utils.ts
Length of output: 1005
Script:
#!/bin/bash
# Search for the checkIfEmpty implementation
ast-grep --pattern 'export const checkIfEmpty = $_' web/src/components/molecules/Content/Form/fields/utils.ts
# Also check any other usage of checkIfEmpty in the codebase
rg "checkIfEmpty" web/src/components/molecules/Content/Form/fields/utils.ts -A 5
Length of output: 743
web/src/components/molecules/Content/Form/fields/FieldComponents/URLField.tsx (2)
9-9: LGTM: Clean import addition
The import of requiredValidator from the utils module is properly structured and follows the codebase's import conventions.
29-32: Verify the validation behavior for multiple empty values
The new required validator implementation looks good, but let's verify it handles the specific case mentioned in the PR objectives correctly (when all provided multiple values are empty).
✅ Verification successful
Let me check the implementation of checkIfEmpty function since it's crucial for handling empty values.
Based on the implementation I found, I can now provide the final response:
Required validator correctly handles multiple empty values
The implementation in utils.ts properly handles the case of multiple empty values through:
checkIfEmptyfunction that checks for undefined, null, and empty string valuesrequiredValidatorspecifically handles arrays usingvalue.every(v => checkIfEmpty(v))to validate that all values are non-empty
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of requiredValidator in utils.ts
# to ensure it properly handles multiple empty values
# Find and display the requiredValidator implementation
ast-grep --pattern 'export const requiredValidator = $_' -A 10
# Check for any related test files
fd "utils.test" --type f
Length of output: 776
Script:
#!/bin/bash
# Find the implementation of checkIfEmpty function
ast-grep --pattern 'export const checkIfEmpty = $_'
# Search for any tests specifically related to requiredValidator or checkIfEmpty
rg -A 5 "describe.*('(requiredValidator|checkIfEmpty)"
Length of output: 444
web/src/components/molecules/Content/Form/fields/FieldComponents/MarkdownField.tsx (4)
11-11: LGTM! Good practice to centralize validation logic.
The addition of the requiredValidator import helps centralize validation logic, making it more maintainable and consistent across components.
23-23: LGTM! Good use of useMemo for performance optimization.
Memoizing the required value prevents unnecessary recalculations when other props change.
31-32: Verify empty string handling in requiredValidator.
The validation looks good, but let's verify that the requiredValidator properly handles edge cases like empty strings and whitespace-only values.
✅ Verification successful
Empty string validation is properly handled
The requiredValidator implementation correctly handles empty strings and edge cases through the checkIfEmpty function, which validates against:
- undefined values
- null values
- empty strings ("")
- arrays containing only empty values
The validator is also being reused consistently across the codebase, particularly in MultiValueField component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check implementation of requiredValidator
ast-grep --pattern 'export const requiredValidator = $_'
# Look for test cases
rg -A 5 'test.*requiredValidator'
Length of output: 676
Script:
#!/bin/bash
# Check implementation of checkIfEmpty function
ast-grep --pattern 'export const checkIfEmpty = $_'
# Look for any tests related to empty value validation
rg -A 5 'test.*empty'
# Search for any other usage of checkIfEmpty
rg 'checkIfEmpty'
Length of output: 2318
54-61: Verify required prop handling in child components.
The required prop is consistently passed to both MultiValueField and MarkdownInput components. Let's verify their implementations handle this prop correctly.
✅ Verification successful
Required prop is properly handled in both components
Both MultiValueField and MarkdownInput components correctly implement the required prop validation:
- MultiValueField checks if the field is required and validates if all values are empty using
checkIfEmpty - MarkdownInput validates the required state by checking if the value is empty when required is true
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check MultiValueField implementation
ast-grep --pattern 'const MultiValueField = ({ required, $$$ }) => $$$'
# Check MarkdownInput implementation
ast-grep --pattern 'const MarkdownInput = ({ required, $$$ }) => $$$'
# Look for related tests
rg -A 5 'test.*(MultiValueField|MarkdownInput).*required'
Length of output: 205
Script:
#!/bin/bash
# Search for MultiValueField implementation with broader pattern
ast-grep --pattern 'MultiValueField = $$$'
# Search for MarkdownInput implementation with broader pattern
ast-grep --pattern 'MarkdownInput = $$$'
# Look for files containing these components
fd -e tsx -e ts "MultiValueField|MarkdownInput" --exec cat {}
# Search for any usage of required prop in test files
rg "required.*prop" -g "*.test.{ts,tsx}"
Length of output: 196
Script:
#!/bin/bash
# Search for files containing MultiValueField or MarkdownInput using ripgrep
rg -l "MultiValueField|MarkdownInput"
# Then search for required prop handling in those specific components
rg -A 5 "interface.*Props.*{" -g "*{MultiValueField,MarkdownInput}*.{tsx,ts}"
rg -A 5 "type.*Props.*{" -g "*{MultiValueField,MarkdownInput}*.{tsx,ts}"
# Look for validation logic related to required fields
rg "required.*validation" -g "*.{tsx,ts}"
Length of output: 2540
Script:
#!/bin/bash
# Check the implementation of MultiValueField
rg -A 10 "export.*MultiValueField" web/src/components/molecules/Common/MultiValueField/index.tsx
# Check the implementation of MarkdownInput in Markdown component
rg -A 10 "export.*Markdown" web/src/components/atoms/Markdown/index.tsx
# Look for Props interfaces/types
rg "Props.*{" web/src/components/molecules/Common/MultiValueField/index.tsx
rg "Props.*{" web/src/components/atoms/Markdown/index.tsx
Length of output: 953
Script:
#!/bin/bash
# Search for Props and required prop handling with fixed regex
rg "interface.*Props" web/src/components/molecules/Common/MultiValueField/index.tsx
rg "type.*Props" web/src/components/molecules/Common/MultiValueField/index.tsx
rg "required" web/src/components/molecules/Common/MultiValueField/index.tsx
# Check Markdown component
rg "interface.*Props" web/src/components/atoms/Markdown/index.tsx
rg "type.*Props" web/src/components/atoms/Markdown/index.tsx
rg "required" web/src/components/atoms/Markdown/index.tsx
# Check for validation handling
rg "validate|validation" web/src/components/molecules/Common/MultiValueField/index.tsx
rg "validate|validation" web/src/components/atoms/Markdown/index.tsx
Length of output: 786
web/src/components/molecules/Content/Form/fields/FieldComponents/TextareaField.tsx (4)
11-11: LGTM! Good practice using centralized validation.
The addition of a centralized validator improves code maintainability and consistency across form components.
23-23: LGTM! Good performance optimization.
Memoizing the required field prevents unnecessary re-renders when other props change.
31-32: Verify handling of multiple empty values.
While the validation setup looks correct, let's verify that the requiredValidator properly handles the case when all multiple values are empty, as mentioned in the PR objectives.
✅ Verification successful
Required validation correctly handles multiple empty values
The implementation properly validates multiple empty values:
requiredValidatorspecifically checks for array values usingArray.isArray(value)and validates that all items are non-empty usingeverycheckIfEmptycorrectly identifies empty values (undefined, null, or empty string)- The validator rejects when all values in the array are empty, which aligns with the PR objectives
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the requiredValidator implementation to verify empty values handling
ast-grep --pattern 'export const requiredValidator = $_' -A 10
Length of output: 646
Script:
#!/bin/bash
# Let's verify the checkIfEmpty implementation
ast-grep --pattern 'export const checkIfEmpty = $_'
Length of output: 278
60-60: Verify required prop handling in child components.
The required prop is consistently passed, but we should verify that both MultiValueField and TextArea components properly handle this prop for validation feedback.
Also applies to: 63-69
✅ Verification successful
Based on the search results, I can now generate the final response as we have found the validation implementation. The code shows that both TextareaField and MultiValueField components properly handle the required prop:
Both components properly handle required field validation
- TextareaField uses the
requiredValidatorfunction which validates both single and multiple values - The validation is consistently applied through Form.Item rules
- The required prop is properly passed from parent to child components
- The validator checks for empty values and array values appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for MultiValueField and TextArea components to verify required prop handling
ast-grep --pattern 'const MultiValueField = ({ required, $$$ }) => $_'
ast-grep --pattern 'const TextArea = ({ required, $$$ }) => $_'
Length of output: 138
Script:
#!/bin/bash
# Let's try a broader search for the components and their required prop handling
# First, let's find the actual component files
fd -e tsx -e ts "MultiValueField|TextArea" --type f
# Then search for required prop usage patterns in these components
rg -l "required.*prop" --type ts --type tsx
Length of output: 127
Script:
#!/bin/bash
# Let's also check the type definitions and interfaces for these components
ast-grep --pattern 'interface $_ {
$$$
required?: $_
$$$
}'
ast-grep --pattern 'type $_ = {
$$$
required?: $_
$$$
}'
Length of output: 1426
Script:
#!/bin/bash
# Search for validation logic related to required fields
rg "required.*validation|validation.*required" --type ts --type tsx -A 3
Length of output: 102
Script:
#!/bin/bash
# Let's try different search patterns without file type specifications
rg -l "MultiValueField|TextArea"
# Then let's examine the content of any matching files
rg "MultiValueField|TextArea" -A 10 -B 10
Length of output: 164702
Script:
#!/bin/bash
# Let's also search for required prop validation patterns without file type specifications
rg "required.*=.*{.*required.*}|required\s*\?:|required.*prop" -A 3
Length of output: 223
Script:
#!/bin/bash
# Let's search for the TextareaField component implementation
rg -A 10 -B 10 "const TextareaField"
# And also check for required prop handling in MultiValueField
rg -A 10 -B 10 "const MultiValueField"
Length of output: 4206
Script:
#!/bin/bash
# Let's check for the validation logic in TextareaField and MultiValueField
rg -A 10 "requiredValidator"
# Also check for any form validation rules
rg -A 5 "rules=\[\{"
Length of output: 23176
web/src/components/molecules/Content/Form/fields/FieldComponents/NumberField.tsx (2)
10-10: LGTM: Clean import addition
The import of the centralized validator function follows good practices for code organization.
Line range hint 45-57: Verify validation behavior for multiple values
Let's verify that the validation logic correctly handles these scenarios:
- Multiple values when field.multiple is true
- Empty array case for multiple values
- Interaction between required validation and number range validation
✅ Verification successful
Based on the gathered information, I can now provide a final response:
The number field validation implementation correctly handles all validation scenarios
The validation logic is properly implemented with:
- Required validation using
requiredValidatorwhich correctly handles both empty arrays and empty values throughcheckIfEmpty - Number range validation through the
validatefunction that properly checks min/max constraints - Array handling using
Array.isArray(value) ? value.some(v => validate(v)) : validate(value)which validates each value in multiple mode
The implementation properly covers:
- Multiple values through array handling
- Empty array cases via
checkIfEmptyin the required validator - Proper interaction between required validation and number range validation, as they are separate rules in the validation chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests and implementation of requiredValidator
echo "Searching for requiredValidator implementation and tests..."
rg -A 10 "requiredValidator" --type typescript
# Search for related test cases
echo "Searching for NumberField test cases..."
fd -e test.tsx -e spec.tsx | xargs rg -l "NumberField"
# Look for similar validation patterns in other field components
echo "Checking validation patterns in other field components..."
fd FieldComponents | xargs rg -l "required.*validator"
Length of output: 536
Script:
#!/bin/bash
# Search for requiredValidator implementation
echo "Searching for requiredValidator..."
rg "requiredValidator" -A 5
# Search for NumberField validation tests
echo "Looking for NumberField test files..."
fd "NumberField.*\.(test|spec)\.(ts|tsx|js|jsx)$"
# Search for validation logic in NumberField
echo "Checking NumberField validation implementation..."
rg "validator.*NumberField" -A 10
# Look for multiple value handling in NumberField
echo "Checking multiple value handling..."
rg "field\.multiple.*NumberField" -A 10
# Search for array validation patterns
echo "Checking array validation patterns..."
rg "Array\.isArray.*validate" -A 5
Length of output: 15462
Script:
#!/bin/bash
# Let's check the implementation of checkIfEmpty and the validate function
echo "Searching for checkIfEmpty implementation..."
rg "checkIfEmpty" -A 5
# Look for the validate function in NumberField
echo "Searching for validate function in NumberField..."
ast-grep --pattern 'const validate = $_'
# Check for field.multiple usage in NumberField component
echo "Checking field.multiple implementation in NumberField..."
ast-grep --pattern 'field.multiple ? $$ : $$' web/src/components/molecules/Content/Form/fields/FieldComponents/NumberField.tsx
Length of output: 6897
web/src/components/molecules/Content/Form/fields/FieldComponents/DefaultField.tsx (4)
11-11: LGTM! Good separation of concerns.
Moving the validation logic to a separate utility file improves code organization and reusability.
29-29: LGTM! Good performance optimization.
Using useMemo for field.required prevents unnecessary recalculations when other props change.
66-66: LGTM! Consistent handling of required state.
The required prop is correctly propagated to both single and multiple value input components, ensuring consistent validation behavior.
Also applies to: 69-75
11-11: Verify the requiredValidator implementation.
Let's ensure the requiredValidator correctly handles the case when all multiple values are empty, as mentioned in the PR objectives.
Also applies to: 37-38
✅ Verification successful
The implementation of requiredValidator shows that it correctly handles both single values and arrays, specifically checking if all values in an array are empty using value.every(v => checkIfEmpty(v)). Let's verify the checkIfEmpty function to ensure it handles all empty cases properly.
The requiredValidator implementation correctly handles empty multiple values
The implementation is properly designed to handle both single and multiple values:
checkIfEmptycorrectly identifies empty values (undefined, null, or empty string)- For arrays, it checks if all values are empty using
value.every(v => checkIfEmpty(v)) - The validator rejects when either a single value is empty or all values in an array are empty
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of requiredValidator
# Find the utils file containing the validator
fd utils.ts -x cat {} | grep -A 10 "requiredValidator"
# Find all usages of requiredValidator to understand the validation context
rg "requiredValidator" -A 5
Length of output: 13231
Script:
#!/bin/bash
# Find the implementation of checkIfEmpty function
rg "checkIfEmpty" -A 5 web/src/components/molecules/Content/Form/fields/utils.ts
Length of output: 406
web/src/components/molecules/Content/Form/fields/FieldComponents/GeometryField.tsx (2)
10-10: LGTM: Clean import of shared validator
The addition of the shared requiredValidator import follows good practices for code reuse and centralization of validation logic.
Line range hint 42-49: Verify validation behavior for multiple geometry values
The current implementation has two validators:
- requiredValidator for empty value validation
- errorSet validator for geometry-specific errors
Let's verify this handles the PR's objective of fixing validation when all multiple values are empty.
✅ Verification successful
Let me gather more information about the GeometryField component and its validation behavior.
Let me search for the actual GeometryField component implementation and its validation logic.
Let me try one final search to understand the complete validation setup.
Validation behavior for multiple geometry values is properly implemented
The implementation correctly handles validation for both single and multiple geometry values:
- The
requiredValidator(from utils) checks for empty values and specifically handles arrays by verifying if all elements are empty usingvalue.every(v => checkIfEmpty(v)). - The geometry-specific validation through
errorSettracks validation state for each geometry item independently. - The component properly switches between
MultiValueGeometryand singleGeometryItembased on themultipleflag, with both components having access to error handling viaerrorAddanderrorDelete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests or similar geometry field implementations to verify behavior
ast-grep --pattern 'test($$$GeometryField$$$)'
# Check for other uses of requiredValidator with multiple values
rg -A 5 'requiredValidator.*multiple.*true'
# Look for related validation logic in utils
fd utils.ts -x cat {} | grep -A 10 'requiredValidator'
Length of output: 371
Script:
#!/bin/bash
# Check GeometryField implementation
ast-grep --pattern 'const GeometryField = $_' -A 30
# Look for test files related to geometry
fd -e test.tsx -e spec.tsx -x rg -l "GeometryField"
# Check Form.Item usage in GeometryField
rg -A 5 "Form.Item.*GeometryField"
# Look for multiple prop usage
rg "multiple.*=.*true.*GeometryField"
Length of output: 3376
Script:
#!/bin/bash
# Search for the GeometryField component implementation
rg -A 30 "export.*GeometryField.*=.*" web/src/components/molecules/Content/Form/fields/FieldComponents/GeometryField.tsx
# Look for multiple value handling in the component
rg -A 10 "multiple.*\?.*\[.*\]" web/src/components/molecules/Content/Form/fields/FieldComponents/GeometryField.tsx
# Check for errorSet usage
rg -A 5 "errorSet.*=" web/src/components/molecules/Content/Form/fields/FieldComponents/GeometryField.tsx
# Look for validation related imports
rg "import.*validator" web/src/components/molecules/Content/Form/fields/FieldComponents/GeometryField.tsx
Length of output: 597
Script:
#!/bin/bash
# Get the full GeometryField component implementation
cat web/src/components/molecules/Content/Form/fields/FieldComponents/GeometryField.tsx
# Look for value handling in the component
rg -A 10 "const.*value.*=.*" web/src/components/molecules/Content/Form/fields/FieldComponents/GeometryField.tsx
# Check Form.Item rules setup
rg -A 10 "<Form.Item.*rules=\[" web/src/components/molecules/Content/Form/fields/FieldComponents/GeometryField.tsx
Length of output: 2655
web/src/components/atoms/Markdown/index.tsx (1)
11-11: LGTM: Props type extension is well-defined
The optional isError boolean prop is correctly typed and maintains backward compatibility.
web/src/components/molecules/Common/MultiValueField/index.tsx (3)
9-9: LGTM: Appropriate utility import
The import of checkIfEmpty utility is well-placed and aligns with the validation enhancement objective.
30-30: LGTM: Props interface enhancement
The addition of the required prop maintains type safety and follows React form field patterns.
98-98: Consider refactoring the validation logic for better readability
While the validation logic is functionally correct, it could be more maintainable if extracted into a separate function.
Consider refactoring like this:
+ const isFieldInError = (value: (string | number | Dayjs)[], index: number) => {
+ const isRequiredError = required && value.every(v => checkIfEmpty(v));
+ const hasIndexError = errorIndexes?.has(index);
+ return isRequiredError || hasIndexError;
+ };
<FieldInput
style={{ flex: 1 }}
{...props}
onChange={(e: ChangeEvent<HTMLInputElement>) => handleInput(e, key)}
onBlur={() => onBlur?.()}
value={valueItem}
- isError={(required && value.every(v => checkIfEmpty(v))) || errorIndexes?.has(key)}
+ isError={isFieldInError(value, key)}
/>Additionally, please verify the behavior with edge cases:
web/src/components/molecules/Content/Form/fields/ComplexFieldComponents/AssetField.tsx (1)
12-12: Verify the requiredValidator implementation
Let's ensure the imported validator is properly implemented in the utils file.
✅ Verification successful
Import of requiredValidator is correctly implemented and widely used
The requiredValidator is properly implemented in utils.ts with support for both single values and arrays. It uses a helper function checkIfEmpty to validate individual values and is consistently used across multiple form field components in the codebase (DefaultField, NumberField, URLField, etc.). The implementation correctly handles the validation requirements for the AssetField component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of requiredValidator in utils.ts
# Expected: Find the implementation and verify it handles both single and multiple values
# Search for the requiredValidator implementation
ast-grep --pattern 'export const requiredValidator = $_'
# Search for any tests related to the validator
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx | xargs rg "requiredValidator"
Length of output: 720
Script:
#!/bin/bash
# Let's check the implementation of checkIfEmpty function which is used in requiredValidator
ast-grep --pattern 'const checkIfEmpty = $_'
# Also search for any direct usage of requiredValidator in the codebase
rg "requiredValidator" -A 2
Length of output: 6660
web/src/components/molecules/Content/Form/fields/FieldComponents/DefaultField.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
web/e2e/project/item/fields/textarea.spec.ts (2)
105-105: Good addition to validation testing!The new assertion verifies that the Save button is properly disabled when required fields are empty, which aligns well with the PR's objective of enhancing validation for required multiple values.
Consider adding these additional validation scenarios to make the test more comprehensive:
- Test submitting with empty strings (
"") in multiple values- Test submitting with only whitespace values (
" ")- Test the validation message content for required fields
Line range hint
1-120: Consider breaking down the test into smaller, focused test cases.The current test file contains a single large test case that covers multiple aspects of functionality. This makes it harder to maintain and debug failures.
Consider restructuring the test into smaller, focused test cases:
describe('Textarea Field', () => { test('should create textarea field with basic settings', async () => { // Test field creation and basic settings }); test('should handle multiple values correctly', async () => { // Test multiple values functionality }); test('should validate required fields properly', async () => { // Test required field validation }); test('should enforce maximum length validation', async () => { // Test max length validation }); });This structure would:
- Make tests more maintainable
- Provide clearer failure messages
- Allow running specific scenarios independently
- Improve test readability
web/e2e/project/item/fields/markdown.spec.ts (1)
Line range hint
1-120: Consider adding more validation test cases.While the test covers basic validation scenarios, consider adding specific test cases for:
- Attempting to save with all multiple values being empty
- Edge cases around required field validation
- Validation behavior when toggling between single and multiple values
This would strengthen the test coverage for the enhanced validator functionality described in the PR objectives.
Would you like me to help generate additional test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
web/e2e/project/item/fields/markdown.spec.ts(1 hunks)web/e2e/project/item/fields/text.spec.ts(1 hunks)web/e2e/project/item/fields/textarea.spec.ts(1 hunks)
🔇 Additional comments (1)
web/e2e/project/item/fields/markdown.spec.ts (1)
105-105: LGTM! The added assertion enhances validation testing.
The new assertion verifies that the Save button is properly disabled when required fields are empty, which aligns with the PR objective of enhancing validation for required multiple values.
Overview
This PR fixes to enhance validator of required multiple values.
What I've done
Summary by CodeRabbit
Release Notes
New Features
Input,MarkdownInput,TextArea, and others, with the addition ofrequiredandisErrorproperties for improved error handling.Bug Fixes
Documentation