fix(web): ensure the argument of runes function are string #1340
Conversation
WalkthroughThe pull request introduces modifications across several components to enhance type safety and validation logic. Specifically, the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ 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 (6)
web/src/components/atoms/TextArea/index.tsx (1)
15-15: Optimize the type check conditionThe condition has a redundant check. Since an empty string is still a string, the
value &&check after the type check is unnecessary.-(maxLength && typeof value === "string" && value && runes(value).length > maxLength) +(maxLength && typeof value === "string" && runes(value).length > maxLength)web/src/components/molecules/Schema/FieldModal/FieldDefaultInputs/Markdown/index.tsx (1)
27-30: LGTM! Consider adding descriptive error messages.The type checks properly ensure string arguments for the
runesfunction. However, the empty error message (message: "") might not be user-friendly.Consider adding descriptive error messages for validation failures:
rules={[ { validator: (_, value) => { if (value && maxLength) { if (Array.isArray(value)) { if (value.some(v => typeof v === "string" && maxLength < runes(v).length)) { - return Promise.reject(); + return Promise.reject(new Error(t("Input exceeds maximum length"))); } } else if (typeof value === "string" && maxLength < runes(value).length) { - return Promise.reject(); + return Promise.reject(new Error(t("Input exceeds maximum length"))); } } return Promise.resolve(); }, - message: "", }, ]}web/src/components/molecules/Schema/FieldModal/FieldDefaultInputs/TextArea/index.tsx (1)
26-29: Consider extracting shared validation logic.The validation logic is identical to the MarkdownField component. Consider extracting this into a shared utility function to maintain DRY principles and ensure consistent validation across components.
Example implementation:
// utils/validation.ts export const validateStringLength = (value: unknown, maxLength: number) => { if (!value || !maxLength) return Promise.resolve(); if (Array.isArray(value)) { if (value.some(v => typeof v === "string" && maxLength < runes(v).length)) { return Promise.reject(new Error(t("Input exceeds maximum length"))); } } else if (typeof value === "string" && maxLength < runes(value).length) { return Promise.reject(new Error(t("Input exceeds maximum length"))); } return Promise.resolve(); };Then use it in both components:
rules={[ { validator: (_, value) => validateStringLength(value, maxLength), }, ]}web/src/components/molecules/Content/Form/fields/FieldComponents/TextareaField.tsx (1)
39-42: Consider extracting shared validation logicThe same validation logic appears in multiple components (TextareaField, DefaultField). Consider extracting it into a shared utility function.
Create a new utility function in
utils.ts:export const validateMaxLength = (value: unknown, maxLength?: number) => { if (!value || !maxLength) return true; if (Array.isArray(value)) { return !value.some(v => typeof v === "string" && maxLength < runes(v).length); } return !(typeof value === "string" && maxLength < runes(value).length); };Then use it in the components:
validator: (_, value) => { - if (value && maxLength) { - if (Array.isArray(value)) { - if (value.some(v => typeof v === "string" && maxLength < runes(v).length)) { - return Promise.reject(); - } - } else if (typeof value === "string" && maxLength < runes(value).length) { - return Promise.reject(); - } - } - return Promise.resolve(); + return validateMaxLength(value, maxLength) + ? Promise.resolve() + : Promise.reject(t("Text length cannot exceed {maxLength} characters", { maxLength })); },web/src/components/atoms/Markdown/index.tsx (2)
19-22: Simplify the type check conditionThe current condition can be simplified while maintaining type safety.
- } else if (props.maxLength && typeof value === "string" && value) { + } else if (props.maxLength && typeof value === "string" && value.length > 0) { return runes(value).length > props.maxLength;
54-54: Use empty string as fallback for non-string valuesUsing
undefinedas fallback might cause ReactMarkdown to use its default fallback. It's more explicit to use an empty string.- <ReactMarkdown>{typeof value === "string" ? value : undefined}</ReactMarkdown> + <ReactMarkdown>{typeof value === "string" ? value : ""}</ReactMarkdown>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
web/src/components/atoms/Input/index.tsx(1 hunks)web/src/components/atoms/Markdown/index.tsx(2 hunks)web/src/components/atoms/TextArea/index.tsx(1 hunks)web/src/components/molecules/Content/Form/fields/FieldComponents/DefaultField.tsx(1 hunks)web/src/components/molecules/Content/Form/fields/FieldComponents/MarkdownField.tsx(1 hunks)web/src/components/molecules/Content/Form/fields/FieldComponents/TextareaField.tsx(1 hunks)web/src/components/molecules/Schema/FieldModal/FieldDefaultInputs/Markdown/index.tsx(1 hunks)web/src/components/molecules/Schema/FieldModal/FieldDefaultInputs/TextArea/index.tsx(1 hunks)web/src/components/molecules/Schema/FieldModal/FieldDefaultInputs/TextField/index.tsx(1 hunks)
🔇 Additional comments (4)
web/src/components/molecules/Schema/FieldModal/FieldDefaultInputs/TextField/index.tsx (1)
27-31: Verify all usages of runes function across the codebase
Let's ensure all usages of the runes function have proper type checks.
✅ Verification successful
All usages of runes function have proper type checks
After reviewing all occurrences of the runes function across the codebase, every usage is properly guarded with type checks:
- Input/TextArea components check
typeof value === "string"before usingrunes - Form field components check both array elements and direct values with
typeof v === "string"ortypeof value === "string" - Markdown component includes type check
typeof value === "string" - All implementations consistently validate string type before applying
runes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of runes function
echo "Searching for runes function calls without type checks..."
rg "runes\(" --type typescript -C 2 | grep -v "typeof.*===.*string.*&&.*runes"
Length of output: 240
Script:
#!/bin/bash
# Search for all usages of runes function with proper context
echo "Searching for runes function calls..."
rg "runes\(" -t ts -t tsx -C 2
Length of output: 145
Script:
#!/bin/bash
# Search for all usages of runes function with proper context
echo "Searching for runes function calls..."
rg "runes\(" --type-add 'ts:*.{ts,tsx}' -t ts -C 2
Length of output: 8325
web/src/components/molecules/Content/Form/fields/FieldComponents/MarkdownField.tsx (1)
39-42: Verify validation behavior with itemGroupId.
The type checking changes look good but are duplicated across components. Additionally, since this component handles grouped items differently, let's verify the validation behavior.
Strongly recommend extracting the validation logic into a shared utility as it's now duplicated in three components. This will:
- Reduce code duplication
- Ensure consistent validation behavior
- Make future updates easier to maintain
- Centralize error message handling
See the previous review comment for the suggested implementation of validateStringLength.
✅ Verification successful
Let me gather more information about the validation behavior in grouped items.
Let me try a different approach to check the validation handling.
Let me try one final verification to check the validation implementation.
Based on the verification results, I can now provide the final response:
Validation behavior with itemGroupId is correctly implemented
The validation logic in MarkdownField works correctly with itemGroupId as it:
- Uses Form.Item's standard name composition
[field.id, itemGroupId] - Leverages MultiValueField component which properly handles array values and validation
- Maintains validation state independently for each field in the group
The suggestion to extract validation logic remains valid as the string length validation is still duplicated, but the itemGroupId handling itself is properly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any special handling of validation in grouped items
# Look for other uses of itemGroupId with validation rules
rg -A 5 "itemGroupId.*validation" .
rg -A 5 "\[field\.id,\s*itemGroupId\]" .
Length of output: 9988
Script:
#!/bin/bash
# Check MultiValueField implementation and validation handling
ast-grep --pattern 'const MultiValueField = $_'
# Check for any validation-related code in MultiValueField
rg -A 5 "validation|validator|rules" ./web/src/components/molecules/Common/MultiValueField/
Length of output: 143
Script:
#!/bin/bash
# Check MultiValueField component implementation
rg -A 10 "export.*MultiValueField" ./web/src/components/molecules/Common/MultiValueField/
# Check how validation is handled in the component
ast-grep --pattern 'function $_($$$) {
$$$
maxLength
$$$
}'
# Look for validation rules in form items
rg -A 5 "rules=\{" ./web/src/components/molecules/Common/MultiValueField/
Length of output: 1126
Script:
#!/bin/bash
# Check the implementation of MultiValueField component
cat ./web/src/components/molecules/Common/MultiValueField/index.tsx
# Look for validation handling in the parent Form component
rg -A 10 "Form.Item.*rules" ./web/src/components/molecules/Content/Form/fields/FieldComponents/MarkdownField.tsx
Length of output: 4228
web/src/components/molecules/Content/Form/fields/FieldComponents/DefaultField.tsx (1)
45-48: Same issues as in TextareaField component
The validation logic has the same issues as identified in TextareaField:
- Empty validation message
- Duplicated validation logic
Please refer to the suggestions made for TextareaField component regarding:
- Adding descriptive validation messages
- Extracting shared validation logic
web/src/components/atoms/Markdown/index.tsx (1)
Line range hint 8-11: Verify the removal of value from Props type
The removal of value from Props type needs verification to ensure it doesn't break existing usage.
✅ Verification successful
Removal of value prop is safe to proceed
Based on the search results, the value prop is not being used directly in any <Markdown> component instances across the codebase. The component is used in several places (FieldDefaultInputs, MarkdownField.tsx) but they don't pass the value prop explicitly. Instead, they use form context or other mechanisms to handle the value state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of value prop in Markdown components
rg -l "value.*props" --type typescript | xargs ast-grep --pattern 'value={$_}'
# Search for spread operator usage with Markdown
rg -l "\.\.\..*props" --type typescript | xargs ast-grep --pattern '<MarkdownInput {...$_}/>'
Length of output: 246
Script:
#!/bin/bash
# Search for direct usage of value prop in Markdown components
rg -l "value.*props" --type ts --type tsx | xargs ast-grep --pattern 'value={$_}'
# Search for spread operator usage with Markdown components
rg -l "\.\.\..*props" --type ts --type tsx | xargs ast-grep --pattern '<Markdown {...$_}/>'
# Search for direct value prop usage in Markdown components
rg -l "Markdown" --type ts --type tsx | xargs ast-grep --pattern '<Markdown value={$_}/>'
# Search for any Markdown component usage
rg -l "Markdown" --type ts --type tsx | xargs ast-grep --pattern '<Markdown$$$>'
Length of output: 460
Script:
#!/bin/bash
# Search for any usage of Markdown component
rg -l "Markdown"
# Then search for specific usages in those files
rg -A 5 "<Markdown"
# Search for any spread operator usage with components
rg -A 5 "<Markdown.*\.\.\."
# Look for value prop usage
rg -A 5 "<Markdown.*value="
Length of output: 4800
web/src/components/molecules/Schema/FieldModal/FieldDefaultInputs/TextField/index.tsx
Show resolved
Hide resolved
web/src/components/molecules/Content/Form/fields/FieldComponents/TextareaField.tsx
Show resolved
Hide resolved
Overview
This PR fixes to ensure the argument of runes function are string.
Memo
A fatal bug occurs when the argument of rune function is not string.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor