fix(web): field key unique check for fields in group #1308
Conversation
WalkthroughThe changes in this pull request enhance the functionality of 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: 1
🧹 Outside diff range and nitpick comments (3)
web/src/components/molecules/Schema/FieldModal/hooks.ts (1)
240-241: Consider making group validation conditionalThe button disable condition for empty group field should only apply when the selected type is "Group". Currently, it might unnecessarily disable the submit button for other field types.
Consider updating the condition to:
-form.getFieldValue("group")?.length === 0 || +((selectedType === "Group" && form.getFieldValue("group")?.length === 0) ||web/src/components/organisms/Project/Schema/hooks.ts (2)
54-57: Consider consolidating modal states.While the current implementation is functional, consider consolidating these modal states into a single object for better state management:
-const [modelModalShown, setModelModalShown] = useState(false); -const [modelDeletionModalShown, setModelDeletionModalShown] = useState(false); -const [groupModalShown, setGroupModalShown] = useState(false); -const [groupDeletionModalShown, setGroupDeletionModalShown] = useState(false); +const [modalStates, setModalStates] = useState({ + model: { shown: false, deletionShown: false }, + group: { shown: false, deletionShown: false } +});
123-128: Add type safety to data memoization.The logic for determining the schema type and data source is correct, but could benefit from explicit type safety:
const isGroup = useMemo( () => groupModalShown || selectedSchemaType === "group", [groupModalShown, selectedSchemaType], ); -const data = useMemo(() => (isGroup ? group : currentModel), [currentModel, group, isGroup]); +const data = useMemo<Schema | MetaDataSchema | undefined>( + () => (isGroup ? group : currentModel), + [currentModel, group, isGroup] +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/src/components/molecules/Schema/FieldModal/hooks.ts(2 hunks)web/src/components/organisms/Project/Schema/hooks.ts(4 hunks)
🔇 Additional comments (2)
web/src/components/organisms/Project/Schema/hooks.ts (2)
12-13: LGTM: Type imports align with the PR objective.
The addition of Schema and MetaDataSchema types provides proper typing support for the field key uniqueness check implementation.
151-171: 🛠️ Refactor suggestion
Add validation and error handling to key uniqueness checks.
The key uniqueness check implementation is correct but could benefit from additional validation:
const keyUniqueCheck = useCallback(
(key: string, fieldId?: string, schema?: Schema | MetaDataSchema) => {
+ if (!key || !schema) return false;
const sameKeyField = schema?.fields?.find(field => field.key === key);
return !sameKeyField || sameKeyField.id === fieldId;
},
[],
);
const handleFieldKeyUnique = useCallback(
(key: string) =>
- keyUniqueCheck(key, selectedField?.id, isMeta ? currentModel?.metadataSchema : data?.schema),
+ key && keyUniqueCheck(key, selectedField?.id, isMeta ? currentModel?.metadataSchema : data?.schema),
[keyUniqueCheck, selectedField?.id, isMeta, currentModel?.metadataSchema, data?.schema],
);
const handleCorrespondingFieldKeyUnique = useCallback(
(key: string) =>
- keyUniqueCheck(
- key,
- selectedField?.typeProperty?.correspondingField?.id,
- referencedModel?.schema,
- ),
+ key && keyUniqueCheck(
+ key,
+ selectedField?.typeProperty?.correspondingField?.id,
+ referencedModel?.schema,
+ ),
[keyUniqueCheck, referencedModel?.schema, selectedField?.typeProperty?.correspondingField?.id],
);Also, verify that these changes handle all field key uniqueness scenarios:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
web/e2e/project/item/fields/group.spec.ts (2)
44-44: Consider documenting the reason for using class selectorThe change from
getByLabelto class selector.ant-select-selectormakes the test more dependent on implementation details. If this change was necessary to fix flaky tests, please add a comment explaining why the class selector is preferred over the more semanticgetByLabel.
Line range hint
1-324: Review test coverage alignment with PR objectiveThe changes in this file focus on fixing the group selector in E2E tests, which appears to be a test maintenance task. However, given that the PR's objective is to fix "field key unique check for fields in group", I recommend:
- Adding explicit test cases to verify that duplicate field keys within a group are properly validated and rejected
- Including test coverage for both positive and negative scenarios of the field key uniqueness check
This will ensure that the PR's main objective is properly tested and prevent potential regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/e2e/project/item/fields/group.spec.ts(2 hunks)web/e2e/project/schema.spec.ts(1 hunks)
🔇 Additional comments (1)
web/e2e/project/item/fields/group.spec.ts (1)
174-174: Verify test coverage for field key uniqueness
While this change fixes the group selector locator, I notice that the PR's objective is to fix "field key unique check for fields in group". However, I don't see explicit test coverage for verifying that field keys within a group must be unique.
Could you help verify if there are other test files covering the field key uniqueness requirement? Let's check:
Would you like me to help create additional test cases to verify the field key uniqueness constraint within groups?
Overview
This PR fixes field key unique check for fields in group.
Summary by CodeRabbit
New Features
FieldModalcomponent, ensuring proper checks for thegroupfield and improved handling ofsupportedTypes.Project/Schemacomponent.Bug Fixes
Refactor