fix(web): the logic to update metadata from edit page #1335
Conversation
WalkthroughThe changes in this pull request involve modifications to several components within the web application, primarily focusing on the removal of 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: 1
🧹 Outside diff range and nitpick comments (6)
web/src/components/molecules/Content/Form/fields/FieldComponents/TagField.tsx (1)
Line range hint
43-51: Consider adding type safety for tag mappingThe implementation looks good, but we could improve type safety for the tag mapping.
Consider defining an interface for the tag object and using it in the mapping:
interface Tag { id: string; name: string; color: string; } // Then in the mapping: {field.typeProperty?.tags?.map((tag: Tag) => (web/src/components/molecules/Content/Form/fields/FieldComponents/CheckboxField.tsx (1)
13-13: Consider adding ARIA labels for better accessibilityWhile the removal of
onMetaUpdateprop simplifies the component, consider enhancing accessibility by adding ARIA labels to both single and multiple checkbox inputs.- <MultiValueBooleanField FieldInput={Checkbox} disabled={disabled} /> + <MultiValueBooleanField + FieldInput={Checkbox} + disabled={disabled} + aria-label={`${field.title} multiple values`} + />- <Checkbox disabled={disabled} /> + <Checkbox + disabled={disabled} + aria-label={field.title} + />Also applies to: 21-21, 23-23
web/src/components/molecules/Content/Form/fields/FieldComponents/DateField.tsx (2)
18-18: Consider adding date format validation and performance optimizationWhile the removal of
onMetaUpdatesimplifies the component, there are opportunities for enhancement:
- The current validation only checks for required fields. Consider adding date format validation:
rules={[ { required: field.required, validator: requiredValidator, message: t("Please input field!"), }, + { + validator: async (_, value) => { + if (value && !Date.parse(value)) { + throw new Error(t("Invalid date format")); + } + }, + }, ]}
- For multiple value mode, consider implementing memoization to optimize re-renders:
+const MemoizedMultiValueField = React.memo(MultiValueField); + const DateField: React.FC<DateFieldProps> = ({ field, itemGroupId, disabled }) => { // ... {field.multiple ? ( - <MultiValueField type="date" FieldInput={StyledDatePicker} disabled={disabled} /> + <MemoizedMultiValueField type="date" FieldInput={StyledDatePicker} disabled={disabled} /> ) : (Also applies to: 34-34, 36-36
Line range hint
13-24: Review form state management architectureThe removal of
onMetaUpdateacross multiple field components suggests a significant architectural change in form state management. This change likely centralizes state updates in a parent component, which is a good practice for maintaining consistency and reducing prop drilling.However, please ensure that:
- The parent form component efficiently handles state updates
- Form validation remains consistent across all field types
- Performance is not impacted by frequent re-renders
Consider implementing a form context to manage state and validation:
// FormContext.tsx type FormContextType = { updateField: (fieldId: string, value: any) => void; validateField: (fieldId: string, value: any) => Promise<void>; }; const FormContext = React.createContext<FormContextType | undefined>(undefined); export const FormProvider: React.FC = ({ children }) => { const updateField = useCallback((fieldId: string, value: any) => { // Centralized update logic }, []); const validateField = useCallback(async (fieldId: string, value: any) => { // Centralized validation logic }, []); return ( <FormContext.Provider value={{ updateField, validateField }}> {children} </FormContext.Provider> ); };Also applies to: 14-24, 18-36
web/src/components/molecules/Content/Form/index.tsx (2)
467-504: Consider adding documentation for the array handling logic.The array validation logic is complex and would benefit from better documentation explaining the conditions for triggering updates, especially the empty value handling.
Add JSDoc comments to explain the logic:
const handleMetaValuesChange = useCallback( async (changedValues: Record<string, unknown>) => { + /** + * Handles metadata value changes with debouncing: + * - For arrays: Updates when all values are non-empty or when array length changes + * - For single values: Updates when value differs from initial value + * - Debounces updates with 800ms timeout + */
486-486: Consider making the debounce timeout configurable.The 800ms debounce timeout is hardcoded. Consider making it configurable through props or constants to allow adjustment based on different use cases or performance requirements.
+const META_UPDATE_DEBOUNCE_MS = 800; const ContentForm: React.FC<Props> = ({ title, + metaUpdateDebounceMs = META_UPDATE_DEBOUNCE_MS, // ... other props }) => { // ... in handleMetaValuesChange - timeout.current = setTimeout(handleMetaUpdate, 800); + timeout.current = setTimeout(handleMetaUpdate, metaUpdateDebounceMs);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
web/src/components/molecules/Common/MultiValueField/index.tsx(1 hunks)web/src/components/molecules/Content/Form/fields/FieldComponents/BoolField.tsx(1 hunks)web/src/components/molecules/Content/Form/fields/FieldComponents/CheckboxField.tsx(1 hunks)web/src/components/molecules/Content/Form/fields/FieldComponents/DateField.tsx(2 hunks)web/src/components/molecules/Content/Form/fields/FieldComponents/TagField.tsx(2 hunks)web/src/components/molecules/Content/Form/index.tsx(2 hunks)
🔇 Additional comments (7)
web/src/components/molecules/Content/Form/fields/FieldComponents/TagField.tsx (1)
16-16: LGTM! Clean component signature
The simplified props interface improves maintainability by removing the onMetaUpdate callback, which aligns with centralizing metadata update handling at a higher level.
web/src/components/molecules/Common/MultiValueField/index.tsx (3)
Line range hint 41-52: LGTM: Robust type handling in handleInput
The implementation correctly handles different input types (number, Dayjs, and standard input events) while maintaining type safety.
Line range hint 54-58: LGTM: Proper value normalization in useEffect
The effect hook correctly handles edge cases:
- Initializes empty array for undefined values
- Wraps single Dayjs values in an array
Line range hint 106-106: LGTM: Comprehensive error handling
The error validation logic effectively combines:
- Required field validation using checkIfEmpty
- Index-specific errors through errorIndexes
web/src/components/molecules/Content/Form/fields/FieldComponents/BoolField.tsx (1)
14-14: Verify form state updates in parent component
The removal of onMetaUpdate suggests that form state updates are now handled at a higher level. Please verify that the parent form component correctly captures changes to these boolean fields.
Additionally, consider adding ARIA labels for better accessibility:
- <MultiValueBooleanField FieldInput={Switch} disabled={disabled} />
+ <MultiValueBooleanField
+ FieldInput={Switch}
+ disabled={disabled}
+ aria-label={`${field.title} multiple values`}
+ />Also applies to: 22-22, 24-24
✅ Verification successful
Based on the search results, I can now generate the final response:
Form state updates are handled correctly through Ant Design Form
The BoolField component is properly integrated with Ant Design's Form system:
- Form state is managed through Form.Item with proper name binding:
name={itemGroupId ? [field.id, itemGroupId] : field.id} - Value updates are handled via the standard
valuePropName="checked"prop - MultiValueBooleanField has built-in onChange handler that maintains array state for multiple values
The accessibility suggestion is still valid though:
- <MultiValueBooleanField FieldInput={Switch} disabled={disabled} />
+ <MultiValueBooleanField
+ FieldInput={Switch}
+ disabled={disabled}
+ aria-label={`${field.title} multiple values`}
+ />🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for form state management in parent components
ast-grep --pattern 'const $name = ($props: any) => {
$$$
const [formState, setFormState] = useState($$$);
$$$
}'
Length of output: 120
Script:
#!/bin/bash
# Let's try a broader search for form state management patterns
ast-grep --pattern 'const $name: React.FC = ($props) => {
$$$
const [formState, setFormState] = useState($$$);
$$$
}'
# Search for form state management with specific props type
ast-grep --pattern 'const $name: React.FC<$props> = ($params) => {
$$$
const [formState, setFormState] = useState($$$);
$$$
}'
# Look for BoolField usage in parent components
rg "BoolField" -A 5 -B 5
# Search for form state management using onChange handlers
ast-grep --pattern 'onChange={$handler}'
Length of output: 55588
Script:
#!/bin/bash
# Let's search for form state management in parent components and Form.Item usage
rg -A 5 -B 5 "Form.Item.*name=\[.*\]"
# Look for MultiValueBooleanField implementation
rg -A 10 -B 5 "MultiValueBooleanField"
# Search for form state updates in parent components
ast-grep --pattern 'const $form = Form.useForm();'
Length of output: 15033
web/src/components/molecules/Content/Form/index.tsx (2)
465-466: LGTM: Proper timeout cleanup.
The timeout cleanup is properly implemented to prevent memory leaks and race conditions.
Also applies to: 470-473
732-732: LGTM: Simplified field component props.
The removal of the onMetaUpdate prop aligns with the new metadata update handling mechanism using debounced updates.
Overview
This PR fixes the logic to update metadata from edit page.
Summary by CodeRabbit
New Features
MultiValueFieldcomponent for improved user experience.ContentFormcomponent, improving responsiveness.Bug Fixes
onMetaUpdatefunctionality from multiple field components, simplifying their interaction logic.Refactor