fix(web): toggle ok button on modal for updating field #1252
Conversation
✅ Deploy Preview for reearth-cms ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Co-authored-by: Nour Balaha <nourbalaha0@gmail.com>
WalkthroughThe pull request introduces several enhancements across multiple components, primarily focusing on error handling, validation, and state management. The 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
web/src/components/molecules/Common/MultiValueField/MultiValueGeometry/index.tsx (1)
Line range hint
28-33: Consider memoizing the handleInput functionWhile the current changes look good, there's an opportunity to improve the component's performance. The
handleInputfunction is recreated on every render because its dependency array includesvalue. Consider memoizing this function usinguseCallbackand including onlyonChangein its dependency array. You can then use the functional update form ofonChangeto ensure you're always working with the latestvalue.Here's a suggested refactor:
const handleInput = useCallback( (e: string, id: number) => { onChange?.(prevValue => prevValue?.map((valueItem, index) => (index === id ? e : valueItem))); }, [onChange] );This change would reduce unnecessary re-creations of the
handleInputfunction, potentially improving performance in scenarios with frequent re-renders.web/src/components/molecules/Schema/FieldModal/index.tsx (2)
131-131: LGTM! Consider adding a comment for clarity.The addition of
handleValuesChangeandonValuesChangeprop to theFormcomponent enhances the form's interactivity, allowing it to respond to user input in real-time. This change aligns well with the PR objective of toggling the "ok" button based on the value being updated.Consider adding a brief comment above the
Formcomponent to explain the purpose ofonValuesChange, which could help other developers understand its role in toggling the "ok" button:// Handle form value changes to toggle the "ok" button state <Form form={form} layout="vertical" initialValues={initialValues} requiredMark={requiredMark} onValuesChange={handleValuesChange}>Also applies to: 180-185
Line range hint
1-424: Overall, the changes look good. Consider adding unit tests.The changes to the
FieldModalcomponent are minimal and focused on enhancing the form's interactivity. They are well-integrated and don't introduce any obvious issues. The component's overall structure and logic remain intact.To ensure the new functionality works as expected, consider adding unit tests for the
handleValuesChangefunction and its integration with the form. This will help maintain the reliability of the component as it evolves. Would you like assistance in creating these unit tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- web/src/components/molecules/Common/Form/GeometryItem/index.tsx (1 hunks)
- web/src/components/molecules/Common/MultiValueField/MultiValueGeometry/index.tsx (1 hunks)
- web/src/components/molecules/Schema/FieldModal/hooks.ts (5 hunks)
- web/src/components/molecules/Schema/FieldModal/index.tsx (2 hunks)
- web/src/components/molecules/Schema/types.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (9)
web/src/components/molecules/Common/MultiValueField/MultiValueGeometry/index.tsx (2)
53-53: LGTM: Enhanced error handling for item deletionThe addition of
errorDelete?.(key);improves the component's error handling capabilities. It notifies about item deletions, which can be useful for parent components to react to these events.
55-55: LGTM: Correct update to useCallback dependenciesThe addition of
errorDeleteto the dependency array ofuseCallbackis correct and necessary. This ensures that thehandleInputDeletefunction always has the latest version of theerrorDeletecallback, preventing issues with stale closures.web/src/components/molecules/Schema/types.ts (3)
103-103: Approved: Standardized tags property typeThe change from an inline object type to
Tag[]for thetagsproperty inFieldTypePropertyInputis a good improvement. It enhances type consistency and leverages the existingTagtype definition, which will make the code more maintainable and less prone to errors.
165-165: Approved: Consistent use of Tag typeThe modification of the
tagsproperty inFormTypesto useTag[]is consistent with the earlier change and further improves type consistency across the codebase. This change will help maintain a uniform structure for tag-related data.
Line range hint
1-170: Summary: Improved type definitions with minor suggestionOverall, the changes in this file significantly improve type safety and consistency, particularly in the handling of tags and supported types. These improvements align well with the PR objectives of enhancing field updates and validations.
Key improvements:
- Standardized use of the
Tagtype for better consistency.- More specific typing for
supportedTypesto ensure only valid types are used.There's one minor suggestion to consider:
- Ensure
EditorSupportedTypeis used as an array type in thesupportedTypesproperty ofFormTypes.These changes will contribute to a more robust and maintainable codebase.
web/src/components/molecules/Schema/FieldModal/hooks.ts (2)
243-243: Optimize validation frequency inuseEffectThe
useEffecthook triggersform.validateFields()on every change tovalues, which may lead to performance issues, especially with larger forms. Consider debouncing the validation or specifying more granular dependencies to reduce unnecessary validations.[performance]
Suggested approach:
- Implement a debounce mechanism using
lodash.debounceor similar.import { useCallback, useEffect, useMemo, useState, useRef } from "react"; + import debounce from "lodash/debounce"; ... - useEffect(() => { + const validateForm = useCallback( + debounce(() => { if (form.getFieldValue("title") && form.getFieldValue("key")) { if (form.getFieldValue("supportedTypes")?.length === 0) { setButtonDisabled(true); } else { form .validateFields() .then(() => setButtonDisabled(changedKeys.current.size === 0)) .catch(() => setButtonDisabled(true)); } } else { setButtonDisabled(true); } + }, 300), + [form, changedKeys], + ); + + useEffect(() => { + validateForm(); }, [form, values]);
255-260: Ensure reliable sorting for arrays of non-primitive elementsThe current sorting of arrays in
handleValuesChangeassumes that the arrays contain primitive values. If the arrays contain objects or non-primitive elements, the defaultsort()method may not function as intended. Provide a compare function tosort()for accurate sorting of complex elements.Example:
if (Array.isArray(changedValue)) { - changedValue = [...changedValue].sort(); + changedValue = [...changedValue].sort((a, b) => /* custom compare function */); } if (Array.isArray(defaultValue)) { - defaultValue = [...defaultValue].sort(); + defaultValue = [...defaultValue].sort((a, b) => /* custom compare function */); }web/src/components/molecules/Common/Form/GeometryItem/index.tsx (2)
228-228: Ensure proper error state reset after successful validationAfter a successful validation and type check,
handleErrorDelete();is called followed by areturn;. Please verify that this correctly resets any existing error states and that no additional error handling is required beyond this point.
233-233: Potential redundant error handlingIn the
catchblock,handleErrorAdd();is invoked to handle JSON parsing errors. With the proposed change to callhandleErrorAdd();directly when validation fails, ensure that this does not result in duplicate error handling for the same error condition.
Overview
This PR fixes the ok button on modal for updating field to toggle depending on the value.
Summary by CodeRabbit
New Features
GeometryItemcomponent.MultiValueGeometrycomponent.FieldModalcomponent.Bug Fixes
FieldModalcomponent.Documentation