fix(web): option and tag field validation enhancement #1306
Conversation
WalkthroughThe pull request introduces enhancements to various components and test cases within the application, focusing on error handling and validation for user inputs in forms. Key changes include the addition of assertions in test cases to verify error messages for invalid inputs, modifications to components to visually indicate error states, and updates to translation files to support new validation messages. Overall, the changes improve the robustness and user experience of the application by ensuring proper feedback is provided during form interactions. Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@caichi-t has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (2)web/src/i18n/translations/ja.yml (2)
The translations for option field validation messages are clear, accurate, and follow consistent Japanese language conventions.
The translations for tag field validation messages maintain consistency with the option field messages and provide clear user feedback. 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: 2
🧹 Outside diff range and nitpick comments (15)
web/src/components/molecules/Common/MultiValueField/index.tsx (2)
96-96: Consider enhancing accessibility and error feedback.While the error status implementation is functional, consider these improvements for better accessibility and user experience:
- Add aria-invalid attribute for screen readers
- Implement error message display mechanism
<FieldInput style={{ flex: 1 }} {...props} onChange={(e: ChangeEvent<HTMLInputElement>) => handleInput(e, key)} onBlur={() => onBlur?.()} value={valueItem} - status={errorIndexes?.has(key) ? "error" : undefined} + status={errorIndexes?.has(key) ? "error" : undefined} + aria-invalid={errorIndexes?.has(key)} + aria-errormessage={errorIndexes?.has(key) ? `field-error-${key}` : undefined}
Line range hint
24-31: Consider memoizing the component for better performance.Since this component handles multiple inputs and array operations, it could benefit from memoization to prevent unnecessary re-renders, especially when used in a list of form fields.
-const MultiValueField: React.FC<Props> = ({ +const MultiValueField: React.FC<Props> = React.memo(({ value = [], onChange, onBlur, FieldInput, errorIndexes, ...props -}) => { +}) => { // ... component implementation -}; +});web/src/components/molecules/Common/MultiValueField/MultValueColoredTag/index.tsx (3)
149-149: Consider adding error tooltip for better UXWhile the error state is correctly implemented, consider adding a tooltip or helper text to explain why the field is in an error state.
<StyledInput {...props} onChange={(e: ChangeEvent<HTMLInputElement>) => handleInput(e, key)} value={valueItem.name} onBlur={() => handleInputBlur()} - status={errorIndexes?.has(key) ? "error" : undefined} + status={errorIndexes?.has(key) ? "error" : undefined} + title={errorIndexes?.has(key) ? t("Please enter a valid tag name") : undefined} />
154-155: Enhance accessibility for tag interactionWhile the click handling works, consider adding keyboard accessibility for better user experience.
<StyledTagContainer hidden={focusedTagIndex === key} - onClick={() => handleTagClick(key)} + onClick={() => handleTagClick(key)} + onKeyDown={(e) => e.key === 'Enter' && handleTagClick(key)} + role="button" + tabIndex={0} isError={errorIndexes?.has(key)}>
199-201: Consider using theme tokens for colorsThe hardcoded color values could be moved to theme tokens for better maintainability and consistency.
const StyledTagContainer = styled.div<{ isError?: boolean }>` cursor: pointer; - border: 1px solid ${({ isError }) => (isError ? "#ff4d4f" : "#d9d9d9")}; + border: 1px solid ${({ isError, theme }) => (isError ? theme.colors.error : theme.colors.border)}; padding: 4px 11px;web/e2e/project/item/metadata/tag.spec.ts (1)
33-35: LGTM: Good duplicate tag validation test with room for enhancementThe test effectively validates duplicate tag handling. Consider enhancing it by also verifying that the error state is cleared and the OK button becomes enabled when a valid, non-duplicate value is entered after the error.
Add these assertions after line 35:
await expect(page.getByText("Labels must be unique")).toBeVisible(); await expect(page.getByRole("button", { name: "OK" })).toBeDisabled(); + await page.locator("#tags").nth(1).fill("UniqueTag"); + await expect(page.getByText("Labels must be unique")).not.toBeVisible(); + await expect(page.getByRole("button", { name: "OK" })).toBeEnabled();web/e2e/project/item/fields/option.spec.ts (3)
28-29: LGTM! Consider enhancing test coverage.Good addition of assertions to verify empty value validation. The test properly checks both the error message visibility and the disabled state of the OK button.
Consider adding test cases for:
- Whitespace-only values
- Values with special characters
31-33: LGTM! Consider case sensitivity testing.Good addition of assertions to verify duplicate value validation. The test properly checks both the error message visibility and the disabled state of the OK button.
Consider adding test cases for:
- Case-sensitive duplicates (e.g., "First" vs "first")
- Duplicates with leading/trailing spaces
Line range hint
1-186: Well-structured test file with good coverage.The test file demonstrates good practices:
- Clear test organization with setup/teardown
- Comprehensive coverage of field operations
- Good mix of positive and negative test cases
Consider organizing the validation-related test cases into a separate test block for better maintainability, e.g.:
test("Option field validation shows appropriate error messages", async ({ page }) => { // Setup code... // Test empty values await page.getByRole("button", { name: "plus New" }).click(); await expect(page.getByText("Empty values are not allowed")).toBeVisible(); await expect(page.getByRole("button", { name: "OK" })).toBeDisabled(); // Test duplicates await page.locator("#values").nth(0).fill("first"); await page.locator("#values").nth(1).fill("first"); await expect(page.getByText("Option must be unique")).toBeVisible(); await expect(page.getByRole("button", { name: "OK" })).toBeDisabled(); // Additional validation cases... });web/src/components/molecules/Schema/FieldModal/hooks.ts (2)
238-242: Simplify validation conditionsThe nested validation conditions could be flattened for better readability and maintainability.
- if ( - form.getFieldValue("values")?.length === 0 || - form.getFieldValue("supportedTypes")?.length === 0 || - form.getFieldValue("tags")?.length === 0 - ) { - setButtonDisabled(true); - } else { - form - .validateFields() - .then(() => setButtonDisabled(changedKeys.current.size === 0)) - .catch(() => setButtonDisabled(true)); - } + const hasEmptyArrays = [ + form.getFieldValue("values"), + form.getFieldValue("supportedTypes"), + form.getFieldValue("tags") + ].some(array => array?.length === 0); + + if (hasEmptyArrays) { + setButtonDisabled(true); + return; + } + + form + .validateFields() + .then(() => setButtonDisabled(changedKeys.current.size === 0)) + .catch(() => setButtonDisabled(true));
383-416: LGTM! Comprehensive validation implementation with a minor type safety suggestionThe validation implementation effectively handles both empty and duplicate value checks. Consider adding type safety for the array indices.
- .map((value: string, index: number) => value.length === 0 && index) - .filter(value => typeof value === "number"); + .map((value: string, index: number): number | false => + value.length === 0 ? index : false) + .filter((value): value is number => value !== false);web/src/components/molecules/Schema/FieldModal/index.tsx (1)
251-262: Optimize tag validation logicThe current implementation extracts names multiple times. Consider simplifying the validation logic.
-validator: async (_, values?: Tag[]) => { - const names = values?.map(value => value.name); - return emptyValidator(names); -}, +validator: async (_, values?: Tag[]) => + emptyValidator(values?.map(value => value.name)), -validator: async (_, values?: Tag[]) => { - const names = values?.map(value => value.name); - return duplicatedValidator(names); -}, +validator: async (_, values?: Tag[]) => + duplicatedValidator(values?.map(value => value.name)),web/src/i18n/translations/en.yml (1)
347-348: Consider consistent terminology for tag-related messagesWhile the messages are clear, consider using consistent terminology. The error message uses "Labels" while the feature is referred to as "tags" elsewhere. Consider changing "Labels must be unique" to "Tags must be unique" for consistency.
-Labels must be unique: '' +Tags must be unique: ''web/src/components/organisms/Project/Schema/hooks.ts (1)
137-144: Consider adding type annotations for better type safetyThe function logic is correct and handles both metadata and regular schema fields appropriately. Consider adding TypeScript type annotations to improve type safety:
const keyUniqueCheck = useCallback( - (key: string, fieldId?: string, model?: Model) => { + (key: string, fieldId?: string, model?: Model): boolean => { const schema = isMeta ? model?.metadataSchema : model?.schema; const sameKeyField = schema?.fields?.find(field => field.key === key); return !sameKeyField || sameKeyField.id === fieldId; }, [isMeta], );web/src/components/molecules/Schema/FieldModal/FieldCreationModalWithSteps/index.tsx (1)
Line range hint
196-202: Consider refactoring the requiredMark function for better reusabilityThe
requiredMarkfunction is used across multiple Form components. Consider these improvements:
- Move it to a shared utility file to avoid duplication
- Make the
isShowparameter's default value consistent across all Form components- const requiredMark = ( - label: React.ReactNode, - { required }: { required: boolean }, - isShow = true, - ) => ( - <> - {required && <Required>*</Required>} - {label} - {isShow && !required && <Optional>{`(${t("optional")})`}</Optional>} - </> - );Create a new file
web/src/components/molecules/Common/Form/RequiredMark.tsx:import { useT } from "@reearth-cms/i18n"; interface RequiredMarkProps { label: React.ReactNode; required: boolean; showOptional?: boolean; } export const RequiredMark: React.FC<RequiredMarkProps> = ({ label, required, showOptional = true }) => { const t = useT(); return ( <> {required && <Required>*</Required>} {label} {showOptional && !required && <Optional>{`(${t("optional")})`}</Optional>} </> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
web/e2e/project/item/fields/option.spec.ts(1 hunks)web/e2e/project/item/metadata/tag.spec.ts(1 hunks)web/src/components/molecules/Common/MultiValueField/MultValueColoredTag/index.tsx(3 hunks)web/src/components/molecules/Common/MultiValueField/index.tsx(3 hunks)web/src/components/molecules/Schema/FieldModal/FieldCreationModalWithSteps/index.tsx(1 hunks)web/src/components/molecules/Schema/FieldModal/hooks.ts(6 hunks)web/src/components/molecules/Schema/FieldModal/index.tsx(4 hunks)web/src/components/molecules/Schema/types.ts(1 hunks)web/src/components/organisms/Project/Schema/hooks.ts(1 hunks)web/src/i18n/translations/en.yml(1 hunks)web/src/i18n/translations/ja.yml(1 hunks)
🔇 Additional comments (15)
web/src/components/molecules/Schema/types.ts (1)
Line range hint 46-50: LGTM! The Tag type export enhances type safety across components.
The export of the Tag type is a good improvement that enables proper type checking for tag-related validations across the application. The type structure with id, name, and color fields is well-defined.
Let's verify the usage of this exported type:
✅ Verification successful
Based on the search results, I can now provide a final response since I have gathered sufficient evidence about the Tag type usage across the codebase:
The Tag type export is being used correctly and serves as a crucial shared type definition.
The exported Tag type is actively used throughout the codebase:
- In form validation components (FieldModal, TagField)
- In schema field type definitions and GraphQL types
- In content rendering components (RenderField, MultiValueColoredTag)
- In form field components for tag input/display
The type export enables proper type checking and consistent tag handling across these components, making it a well-justified change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the Tag type is being imported and used across the codebase
# Expected: Find imports of the Tag type in form validation components
# Search for Tag type imports
rg -t typescript "import.*Tag.*from.*Schema/types"
# Search for Tag type usage in validation logic
ast-grep --pattern 'const $_ = ($x: Tag)'
Length of output: 131
Script:
#!/bin/bash
# Let's try different approaches to find Tag type usage
# Search for imports from Schema/types
rg "import.*from ['\"].*Schema/types['\"]" --type ts
# Search for Tag type usage in type annotations
rg ":\s*Tag[\[\]\s{},]" --type ts
# Search for files that might use Tag
rg "Tag" --type ts -C 2
# Find all TypeScript files in the web directory
fd "\.tsx?$" web/src
Length of output: 125664
web/src/components/molecules/Common/MultiValueField/index.tsx (2)
19-19: LGTM! Well-typed error tracking implementation.
The Set<number> type is an efficient choice for tracking error states with O(1) lookup performance.
28-28: LGTM! Clean prop destructuring.
The errorIndexes prop is properly destructured following the component's existing patterns.
web/src/components/molecules/Common/MultiValueField/MultValueColoredTag/index.tsx (3)
33-33: LGTM: Efficient error tracking with Set
The use of Set<number> for errorIndexes is an excellent choice, providing efficient O(1) lookups for error state checking.
37-42: LGTM: Clean component signature update
The component signature is well-structured with proper prop destructuring and defaults.
37-42: Verify error handling edge cases
Let's ensure the error handling works correctly in all scenarios.
✅ Verification successful
Let me gather more information about the error handling in the MultiValueColoredTag component by checking its test files and related components.
Based on the code analysis, I can now provide a final response about the error handling in the MultiValueColoredTag component.
Error handling implementation is appropriate
The error handling in MultiValueColoredTag is well-implemented:
- The component properly accepts an
errorIndexesprop of typeSet<number>to track which tags have errors - Error states are correctly applied in two places:
- Input field:
status={errorIndexes?.has(key) ? "error" : undefined} - Tag container:
isError={errorIndexes?.has(key)}
- Input field:
- The error handling is consistent with the parent MultiValueField component's pattern
- The implementation is used appropriately in the FieldModal component where errors are tracked for empty and duplicate values
The error handling approach using a Set of indexes is an efficient way to mark multiple erroneous fields, and the visual feedback is properly implemented through the status prop and isError styling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for comprehensive error handling test coverage
# Look for test files related to this component
fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts . | grep -i "MultiValueColoredTag"
# Check for error handling test cases
rg -A 5 "test|describe|it" $(fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts . | grep -i "MultiValueColoredTag") | grep -i "error"
Length of output: 459890
Script:
#!/bin/bash
# Search for test files related to MultiValueColoredTag
fd -e test.tsx -e spec.tsx . | grep -i "MultiValueColoredTag"
# Search for error handling tests in MultiValueColoredTag and related components
rg -A 5 "errorIndexes" web/src/components/molecules/Common/MultiValueField/
# Check usage of errorIndexes prop
rg -A 5 "errorIndexes" web/src/components/
Length of output: 11771
web/e2e/project/item/metadata/tag.spec.ts (1)
30-32: LGTM: Well-structured empty tag validation test
The test properly validates empty tag handling by checking both the error message visibility and button disabled state.
web/src/components/molecules/Schema/FieldModal/hooks.ts (4)
256-271: LGTM! Robust change detection implementation
The implementation correctly handles both array and primitive value comparisons, with proper sorting for arrays to ensure consistent comparison regardless of order.
326-334: LGTM! Well-structured validation chain
The key validation implementation effectively combines syntax validation (validateKey) with uniqueness check (handleFieldKeyUnique), while caching results for better performance.
371-378: LGTM! Comprehensive field initialization
The initialization logic properly handles all field types, ensuring arrays are initialized correctly when creating new fields.
30-30: Verify the removal of fieldId parameter's impact
The handleFieldKeyUnique function signature has been simplified to remove the optional fieldId parameter. While this makes the API cleaner, we should ensure this doesn't affect field key validation when editing existing fields.
✅ Verification successful
No impact from removing fieldId parameter
The implementation of handleFieldKeyUnique in Project/Schema/hooks.ts already handles the field ID context internally through the selectedField?.id dependency in its useCallback hook. The function is correctly implemented to consider the current field's ID during uniqueness validation, making the parameter removal in the interface safe and appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of handleFieldKeyUnique to verify if fieldId was used
rg "handleFieldKeyUnique.*fieldId" -A 2
Length of output: 41
Script:
#!/bin/bash
# Let's first find all usages of handleFieldKeyUnique
rg "handleFieldKeyUnique" -A 3
# Also search for the implementation
ast-grep --pattern 'const handleFieldKeyUnique = $_'
Length of output: 6582
web/src/components/molecules/Schema/FieldModal/index.tsx (1)
27-27: LGTM: Type-level improvements enhance type safety
The changes improve type safety by adding the Tag type and simplifying the handleFieldKeyUnique prop signature.
Also applies to: 41-41
web/src/i18n/translations/en.yml (1)
342-345: LGTM: Option field validation messages are clear and consistent
The new translation keys for option field validation follow the established pattern and provide clear error messages.
web/src/components/organisms/Project/Schema/hooks.ts (1)
137-144: Verify the usage in field validation contexts
Let's verify how this function is used in field validation contexts, particularly for option and tag fields mentioned in the PR objectives.
✅ Verification successful
Let me gather more specific information about how this function is used.
Let me gather one more piece of information to specifically check for Option and Tag field types.
Based on the gathered information, I can now provide the final response:
The keyUniqueCheck function is correctly implemented for field validation
The function is properly used in the schema management context:
- It's used in field key validation through
handleFieldKeyUniqueandhandleCorrespondingFieldKeyUnique - The validation system includes both Option and Tag fields, with proper type checking
- The function correctly handles both regular schema and metadata schema fields through the
isMetaflag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how keyUniqueCheck is used in field validation contexts
# Search for files that import and use keyUniqueCheck
rg -l "keyUniqueCheck|handleFieldKeyUnique|handleCorrespondingFieldKeyUnique" --type ts
# Search for option and tag field validation logic
rg -A 5 "type.*[\"'](?:Option|Tag)[\"'].*=.*{" --type ts
# Search for field validation components that might use this function
ast-grep --pattern 'const $_ = $$$; $_?.type === "Option" || $_?.type === "Tag"'
Length of output: 672
Script:
#!/bin/bash
# Check the actual usage of keyUniqueCheck in the identified files
rg -A 5 "keyUniqueCheck" web/src/components/organisms/Project/Schema/hooks.ts web/src/components/organisms/Project/Schema/index.tsx web/src/components/molecules/Schema/FieldModal/hooks.ts web/src/components/molecules/Schema/FieldModal/index.tsx web/src/components/molecules/Schema/FieldModal/FieldCreationModalWithSteps/index.tsx
# Look for field validation related code in these files
rg -A 5 "validateField|validation" web/src/components/molecules/Schema/FieldModal/hooks.ts web/src/components/molecules/Schema/FieldModal/index.tsx
# Check for type definitions and interfaces related to field validation
rg -A 3 "interface.*Field|type.*Field" web/src/components/molecules/Schema/FieldModal/
Length of output: 8975
Script:
#!/bin/bash
# Check for Option and Tag field type handling
rg -B 2 -A 5 "type === [\"'](?:Option|Tag)[\"']" web/src/components/organisms/Project/Schema/ web/src/components/molecules/Schema/
# Check the field validation component
rg -B 2 -A 5 "FieldValidationInputs" web/src/components/molecules/Schema/
Length of output: 4012
web/src/i18n/translations/ja.yml (1)
342-345: LGTM! The translations for option and tag field validation messages are well-structured.
The Japanese translations are clear, concise, and effectively convey the validation requirements:
- Option field validations (最低一つのオプション, 空の値, 一意である)
- Tag field validations (最低一つのタグ, 一意である)
Also applies to: 347-348
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
web/src/components/atoms/Input/index.tsx (1)
12-18: Consider enhancing status computation logicWhile the current implementation works, there are opportunities for improvement:
- The error status completely overrides external status. Consider preserving external error states even when length is valid.
- The computation could be more efficient by checking conditions earlier.
Consider this alternative implementation:
const _status = useMemo(() => { + if (!maxLength || !value) return status; + return runes(value).length > maxLength ? "error" : status; - if (maxLength && value && runes(value).length > maxLength) { - return "error"; - } else { - return status; - } }, [maxLength, status, value]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/src/components/atoms/Input/index.tsx(2 hunks)
🔇 Additional comments (3)
web/src/components/atoms/Input/index.tsx (3)
11-11: LGTM: Clean signature update
The addition of the status prop maintains type safety while enabling external status control.
Line range hint 1-34: Verify integration with form validation system
The component handles individual input validation well, but we should ensure it integrates properly with the form-level validation system.
#!/bin/bash
# Check for form validation integration patterns
ast-grep --pattern 'const $form = useForm($$$)'
rg -A 5 'validateFields|validateStatus|onFieldsChange'28-28: Verify error state styling
The status prop is correctly applied, but we should verify that error states are visually distinct and meet accessibility standards.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
web/src/components/atoms/Input/index.tsx (1)
12-17: Implementation looks good with room for enhancementThe error handling logic is correctly implemented, but consider enhancing it to distinguish between different types of errors.
Consider this enhancement to provide more granular error handling:
-const status = useMemo(() => { - if (isError || (maxLength && value && runes(value).length > maxLength)) { - return "error"; - } -}, [isError, maxLength, value]); +const status = useMemo(() => { + if (isError) return "error"; + if (maxLength && value && runes(value).length > maxLength) return "warning"; + return undefined; +}, [isError, maxLength, value]);This would allow:
- "error" status for validation errors (isError)
- "warning" status for length violations
- Better visual distinction between different types of issues
web/src/components/molecules/Schema/FieldModal/FieldDefaultInputs/URLField/index.tsx (4)
18-19: Consider emphasizing HTTPS usage in the help textWhile the help text correctly indicates URL format requirements, consider encouraging secure practices by recommending HTTPS over HTTP.
- extra={t("Default value must be a valid URL and start with 'http://' or 'https://'.")} + extra={t("Default value must be a valid URL and start with 'https://' (recommended) or 'http://'.")}
Line range hint
20-35: Enhance validation implementationSeveral improvements could make the validation more robust and user-friendly:
- The error message should be translated
- Empty string handling could be more explicit
- URL validation could be strengthened
rules={[ { - message: "URL is not valid", + message: t("URL is not valid"), validator: async (_, value) => { if (value) { + const validateValue = (url: string) => { + if (!url.trim()) return true; // Skip empty strings + return validateURL(url); + }; + if ( multiple && - value.some((valueItem: string) => !validateURL(valueItem) && valueItem.length > 0) + value.some((valueItem: string) => !validateValue(valueItem)) ) return Promise.reject(); - else if (!multiple && !validateURL(value) && value?.length > 0) + else if (!multiple && !validateValue(value)) return Promise.reject(); } return Promise.resolve(); }, }, ]}>
Line range hint
36-37: Improve accessibility and error handlingConsider enhancing the input components with better accessibility support and more specific error states.
- {multiple ? <MultiValueField FieldInput={Input} /> : <Input />} + {multiple ? ( + <MultiValueField + FieldInput={Input} + aria-label={t("Multiple URL input field")} + data-testid="url-input-multiple" + /> + ) : ( + <Input + aria-label={t("URL input field")} + data-testid="url-input-single" + /> + )}
Inconsistent use of internationalization in validation messages
- The
URLFieldcomponent uses a hard-coded validation message instead of thetfunction.🔗 Analysis chain
Line range hint
1-42: Verify integration with form validation enhancementsSince this PR focuses on validation enhancement, let's verify how this component integrates with other form components and their error handling patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent validation patterns across form components echo "Checking validation patterns in form components..." rg -l "Form\.Item.*rules=\[" "web/src/components" | while read -r file; do echo "=== $file ===" rg "Form\.Item.*rules=\[" "$file" -A 5 done # Check for consistent error message handling echo "Checking error message patterns..." rg "message: t\(" "web/src/components"Length of output: 27397
web/src/components/molecules/Schema/FieldModal/FieldCreationModalWithSteps/index.tsx (3)
Line range hint
532-550: Enhance field key validation feedbackThe validation implementation is good but could be improved:
- Consider making the error message more specific by indicating which validation rule failed (format, uniqueness, etc.)
- Consider memoizing the validation function to prevent unnecessary recreations
rules={[ { - message: t("Key is not valid"), + message: ({ pattern, required }) => + required ? t("Field key is required") : + !pattern ? t("Field key can only contain letters, numbers, underscores and dashes") : + t("Field key must be unique"), required: true, validator: async (_, value) => { await keyValidate( value, prevCorrespondingKey, handleCorrespondingFieldKeyUnique, ); }, }, ]}
Line range hint
532-550: Consider enhancing validation error visibilityThe current implementation could benefit from improved error state visibility:
- Consider adding a visual indicator on the "Validation" tab when there are validation errors
- Consider adding an error summary at the top of the form when there are multiple validation errors
Line range hint
1-650: Consider refactoring for better maintainabilityThe component is quite large and handles multiple responsibilities. Consider:
- Extracting the form validation logic into a custom hook (e.g.,
useFieldValidation)- Breaking down the component into smaller, focused components:
ReferenceSettingStepFieldSettingStepCorrespondingFieldStep- Moving form-related callbacks into a custom hook (e.g.,
useFieldForm)This would improve maintainability, testability, and reusability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
web/src/components/atoms/Input/index.tsx(1 hunks)web/src/components/molecules/Common/MultiValueField/index.tsx(3 hunks)web/src/components/molecules/Schema/FieldModal/FieldCreationModalWithSteps/index.tsx(2 hunks)web/src/components/molecules/Schema/FieldModal/FieldDefaultInputs/URLField/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/molecules/Common/MultiValueField/index.tsx
🔇 Additional comments (3)
web/src/components/atoms/Input/index.tsx (2)
9-9: LGTM: Type definition is well-structured
The isError prop is correctly typed as an optional boolean, following TypeScript best practices.
Line range hint 1-31: Verify consistent error handling patterns
Let's ensure this error handling pattern is consistently applied across related components.
✅ Verification successful
✅ Error handling is consistently applied across related components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent error handling patterns in related components
# Look for similar Input components that might need the same error handling
ast-grep --pattern 'const $_ = forwardRef<$_, $_>('
# Check for other uses of antd Input to ensure consistent error handling
rg -l "from 'antd'.*Input" | xargs rg "status.*=.*error"
Length of output: 270
web/src/components/molecules/Schema/FieldModal/FieldCreationModalWithSteps/index.tsx (1)
33-33:
Verify the restriction of selectedType to "Reference"
The prop type has been restricted to only accept "Reference", which seems to contradict:
- The PR objectives mentioning "option and tag field validation"
- The component name suggesting it handles multiple field types
- The presence of field type selection logic in the component
Consider keeping the generic FieldType type to maintain support for all field types.
✅ Verification successful
Restriction of selectedType to "Reference" Verified
No usages of selectedType with types other than "Reference" found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other field type usages
rg -A 2 "selectedType.*=.*['\"](?!Reference)[^'\"]+['\"]" --type ts
Length of output: 344
Script:
#!/bin/bash
# Check for other field type usages with PCRE2 enabled
rg --pcre2 -A 2 'selectedType\s*:\s*"(?!Reference")[^"]+"' --type ts
Length of output: 70
Overview
This PR enhances option and tag field validation.
What I've done
Memo
This PR includes small fixes regarding Reference field creation modal.
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
These updates enhance the overall usability and robustness of the application, particularly in form interactions.