fix(web): item could not be saved if the multiple value had an empty value #1321
Conversation
WalkthroughThe changes in this pull request primarily focus on the Changes
Possibly related PRs
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: 0
🧹 Outside diff range and nitpick comments (2)
web/src/components/molecules/Content/Form/index.tsx (2)
360-374: Consider enhancing error handling in metaFieldsGetWhile the implementation is clean and efficient, consider adding explicit error handling for validation failures and logging for debugging purposes.
const metaFieldsGet = useCallback(async () => { const result: ItemField[] = []; - const metaValues = await metaForm.validateFields(); + try { + const metaValues = await metaForm.validateFields(); + for (const [key, value] of Object.entries(metaValues)) { + const metaField = metaFieldsMap.get(key); + if (metaField) { + result.push({ + value: inputValueGet(value as ItemValue, metaField.multiple), + schemaFieldId: key, + type: metaField.type, + }); + } + } + return result; + } catch (error) { + console.error('Meta fields validation failed:', error); + throw error; + } - for (const [key, value] of Object.entries(metaValues)) { - const metaField = metaFieldsMap.get(key); - if (metaField) { - result.push({ - value: inputValueGet(value as ItemValue, metaField.multiple), - schemaFieldId: key, - type: metaField.type, - }); - } - } - return result; }, [inputValueGet, metaFieldsMap, metaForm]);
431-445: Improve error handling in handleSubmitThe error handling in
handleSubmitsilently catches all errors. Consider logging the error and potentially showing a user-friendly notification.- changedKeys.current.clear(); - } catch (_) { + changedKeys.current.clear(); + } catch (error) { + console.error('Failed to submit form:', error); + Notification.error({ + message: t('Failed to save'), + description: t('An error occurred while saving the item. Please try again.'), + }); setIsDisabled(false); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/src/components/molecules/Content/Form/index.tsx(4 hunks)
🔇 Additional comments (2)
web/src/components/molecules/Content/Form/index.tsx (2)
339-341: LGTM: Empty value handling implementation looks good!
The implementation correctly converts empty strings to undefined for multiple values while preserving the handling of dayjs objects. This change effectively addresses the issue where items couldn't be saved if multiple values had empty values.
350-358: LGTM: Efficient field mapping implementation!
The introduction of memoized Maps for both model and meta fields is a great optimization. Using Map provides O(1) lookup complexity, which is particularly beneficial when dealing with large schemas. The memoization dependencies are correctly set to rebuild only when the field arrays change.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
web/src/components/molecules/Content/Form/index.tsx (3)
350-358: Consider adding null checks for model fieldsWhile the memoization implementation is good for performance, consider adding null checks to handle cases where
modelmight be undefined:const modelFields = useMemo( - () => new Map((model?.schema.fields || []).map(field => [field.id, field])), + () => new Map(model?.schema?.fields?.map(field => [field.id, field]) ?? []), [model?.schema.fields], ); const metaFieldsMap = useMemo( - () => new Map((model?.metadataSchema.fields || []).map(field => [field.id, field])), + () => new Map(model?.metadataSchema?.fields?.map(field => [field.id, field]) ?? []), [model?.metadataSchema.fields], );
360-374: Consider adding error handling for validation failuresThe
metaFieldsGetfunction correctly handles metadata field transformation, but consider adding error handling for validation failures:const metaFieldsGet = useCallback(async () => { const result: ItemField[] = []; - const metaValues = await metaForm.validateFields(); + try { + const metaValues = await metaForm.validateFields(); + for (const [key, value] of Object.entries(metaValues)) { + const metaField = metaFieldsMap.get(key); + if (metaField) { + result.push({ + value: inputValueGet(value as ItemValue, metaField.multiple), + schemaFieldId: key, + type: metaField.type, + }); + } + } + return result; + } catch (error) { + console.error('Meta fields validation failed:', error); + throw error; + } - for (const [key, value] of Object.entries(metaValues)) { - const metaField = metaFieldsMap.get(key); - if (metaField) { - result.push({ - value: inputValueGet(value as ItemValue, metaField.multiple), - schemaFieldId: key, - type: metaField.type, - }); - } - } - return result; }, [inputValueGet, metaFieldsMap, metaForm]);
450-458: Consider adding user feedback for metadata update failuresWhile the error handling catches validation failures, consider adding user feedback:
const handleMetaUpdate = useCallback(async () => { if (!itemId) return; try { const metaFields = await metaFieldsGet(); await onMetaItemUpdate({ metaItemId: item?.metadata?.id, metaFields, }); } catch (info) { - console.log("Validate Failed:", info); + console.error("Metadata validation failed:", info); + Notification.error({ + message: t("Failed to update metadata"), + description: t("Please check your input and try again"), + }); } }, [itemId, metaFieldsGet, onMetaItemUpdate, item?.metadata?.id]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/src/components/molecules/Content/Form/index.tsx(4 hunks)
🔇 Additional comments (1)
web/src/components/molecules/Content/Form/index.tsx (1)
339-341: LGTM: Empty string handling for multiple values
The implementation correctly converts empty strings to undefined for multiple values while preserving date objects and other non-empty values. This change directly addresses the issue where items couldn't be saved with empty multiple values.
Overview
This PR fixes a bug that an item could not be saved if the multiple value had an empty value.
What I've done
Screenshot
Summary by CodeRabbit
New Features
Bug Fixes
undefinedfor empty string values in form processing.Refactor
Tests