refactor(ui): migrate TagsForm from antd Form to React Hook Form + Autocomplete#29776
refactor(ui): migrate TagsForm from antd Form to React Hook Form + Autocomplete#29776Rohit0301 wants to merge 4 commits into
Conversation
…tocomplete Replace the antd Form/FormInstance pattern with react-hook-form (UseFormReturn) and swap MUI owner/domain pickers for Autocomplete-based USER_TEAM_SELECT_INPUT and DOMAIN_SELECT fields, following the AddDomainForm pattern exactly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Rohit0301
added
the
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🔴 Playwright Results — 3 failure(s), 18 flaky✅ 4044 passed · ❌ 3 failed · 🟡 18 flaky · ⏭️ 34 skipped
Genuine Failures (failed on all attempts)❌
|
| const mutuallyExclusiveLabel = useMemo( | ||
| () => t('label.mutually-exclusive'), | ||
| [t] | ||
| ); |
There was a problem hiding this comment.
💡 Edge Case: Mutually-exclusive helper alert dropped in migration
The old antd getMutuallyExclusiveField config surfaced a contextual alert (t('message.mutually-exclusive-alert-info')) whenever the toggle was switched on, warning the user about the consequence of enabling mutual exclusivity. The new inline Toggle (lines 506-522) renders only the label and no longer shows that informational message, so the useWatch on mutuallyExclusive was removed. This is a user-facing information regression rather than a functional bug. If the alert is still desired, render a HintText/alert below the toggle conditioned on field.value.
Restore the info alert shown when mutual exclusivity is enabled.:
{({ field }) => (
<Box direction="col" gap={2}>
<Box align="center" direction="row" gap={2}>
<Toggle
aria-label={mutuallyExclusiveLabel}
data-testid="mutually-exclusive-button"
isDisabled={disableMutuallyExclusiveField}
isSelected={field.value ?? false}
onBlur={field.onBlur}
onChange={field.onChange}
/>
<FormItemLabel label={mutuallyExclusiveLabel} />
</Box>
{field.value && (
<HintText>{t('message.mutually-exclusive-alert-info')}</HintText>
)}
</Box>
)}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| @@ -172,33 +141,29 @@ export const getDisabledField = ({ | |||
| required: false, | |||
| label: 'label.disable-tag', | |||
| id: 'root/disabled', | |||
| type: FieldTypes.UT_SWITCH, | |||
| formItemLayout: FormItemLayout.HORIZONTAL, | |||
| type: FieldTypes.SWITCH, | |||
| props: { | |||
| 'data-testid': 'disabled', | |||
| initialValue, | |||
| isDisabled: disabled, | |||
| disabled, | |||
There was a problem hiding this comment.
💡 Quality: getDisabledField requires unused initialValue param
getDisabledField still declares a required initialValue parameter but immediately discards it (initialValue: _initialValue). The SWITCH field now derives its state purely from form.reset(...), so the parameter has no effect yet all callers (TagsForm.tsx:427) are still forced to compute and pass (initialValues as Tag)?.disabled ?? false. Keeping a dead parameter in the exported signature is misleading. Remove it from the signature and the caller.
Drop the unused initialValue parameter; update the caller in TagsForm to pass only { disabled }.:
export const getDisabledField = ({
disabled,
}: {
disabled: boolean;
}): FieldProp => ({
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 1 resolved / 3 findingsMigrates 💡 Edge Case: Mutually-exclusive helper alert dropped in migration📄 openmetadata-ui/src/main/resources/ui/src/pages/TagsPage/TagsForm.tsx:437-440 📄 openmetadata-ui/src/main/resources/ui/src/pages/TagsPage/TagsForm.tsx:506-520 The old antd Restore the info alert shown when mutual exclusivity is enabled.💡 Quality: getDisabledField requires unused initialValue param📄 openmetadata-ui/src/main/resources/ui/src/pages/TagsPage/tagFormFields.tsx:133-147
Drop the unused initialValue parameter; update the caller in TagsForm to pass only { disabled }.✅ 1 resolved✅ Bug: Create payload includes extraneous id:'' from form defaults
🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
| const nameField = useMemo(() => { | ||
| const field = getNameField(disableNameField || false); | ||
|
|
||
| return { | ||
| ...field, | ||
| muiLabel: t(field.muiLabel), | ||
| placeholder: t(field.placeholder), | ||
| rules: TAG_NAME_VALIDATION_RULES.map((rule) => ({ | ||
| ...rule, | ||
| message: rule.message | ||
| ? t(rule.message, { | ||
| ...rule.messageData, | ||
| field: rule.messageData?.field | ||
| ? t(rule.messageData.field) | ||
| : undefined, | ||
| entity: rule.messageData?.entity | ||
| ? t(rule.messageData.entity) | ||
| : undefined, | ||
| }) | ||
| : undefined, | ||
| })), | ||
| label: t(field.label as string), | ||
| placeholder: t(field.placeholder ?? ''), | ||
| }; | ||
| }, [t, disableNameField]); |
There was a problem hiding this comment.
The old code explicitly applied TAG_NAME_VALIDATION_RULES to the name field — which enforced NAME_LENGTH_REGEX (2–64 chars) and TAG_NAME_REGEX (no disallowed characters). The new nameField memo no longer attaches any pattern rules, so a user can now create a tag or classification with a one-character name, a 200-character name, or a name containing characters that fail backend validation. The backend will likely reject these, but the error surface shifts from a clear inline form message to a raw API error toast.
Jest test CoverageUI tests summary
|
|
1 participant
Describe your changes:
I worked on migrating the
TagsFormcomponent (used for creating/editing Tags and Classifications) from the legacy antdForm/FormInstancepattern to React Hook Form, following theAddDomainFormreference pattern exactly. The owner and domain pickers were switched fromMUIUserTeamSelect/DOMAIN_SELECT_MUIto Autocomplete-basedUSER_TEAM_SELECT_INPUT/DOMAIN_SELECTfields with async search.Fixes 4875
Type of change:
High-level design:
TagsForm.tsx— Replaced antdFormwithHookFormfrom@openmetadata/ui-core-components. Added async user/team and domain fetch logic (debounced, mirrorsAddDomainForm). Description now usesFormField+RichTextEditorinstead ofgenerateFormFields. AsubmitRefprop allows the external Save buttons inSlideoutMenu.Footerto programmatically trigger validated form submission (submitRef.current = () => form.handleSubmit(handleSave)()).tagFormFields.tsx— All_MUIfield type variants replaced with RHF core-components equivalents (TEXT,ICON_PICKER,COLOR_PICKER,SWITCH). Owner/domain fields now acceptoptions/onFocus/onSearchChangeprops.FieldPropandFieldTypesimported from@openmetadata/ui-core-componentsinstead of the legacy local interface.namepaths changed from antdNamePatharrays to RHF dot-notation strings.TagsPage.interface.ts— AddedTagFormValues,TagFormSelectItem,TAG_FORM_DEFAULTS.formRef: FormInstancereplaced withform: UseFormReturn<TagFormValues>+ optionalsubmitRefacross all three prop interfaces.ClassificationFormDrawer/TagFormDrawer— Each creates asubmitRefref, passes it toTagsForm, and wires the Save button tosubmitRef.current().TagsPage.tsx—useForm()fromantd/lib/form/Formreplaced withuseForm<TagFormValues>()fromreact-hook-form;resetFields()→reset().Tests:
Use cases covered
Unit tests
TagsForm.test.tsx,ClassificationFormDrawer.test.tsx,TagFormDrawer.test.tsxto use RHFuseFormharness instead of antdForm.useFormtagFormFields.test.tsxto assert against the new RHF field configsBackend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
TypeScript compilation passes with zero errors (
npx tsc --noEmit).UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Greptile Summary
This PR migrates the
TagsFormcomponent (used for creating/editing Tags and Classifications) from the legacy antdForm/FormInstancepattern to React Hook Form, following theAddDomainFormreference pattern. Owner and domain pickers are switched fromMUIUserTeamSelect/DOMAIN_SELECT_MUIto Autocomplete-basedUSER_TEAM_SELECT_INPUT/DOMAIN_SELECTfields with async search, andMUIUserTeamSelect.tsxis deleted entirely.TagsForm.tsx: Replaced antdFormwithHookForm; added debounced async fetch for user/team and domain options; introducedsubmitRefto let external Save buttons trigger validated submission.tagFormFields.tsx: All_MUIfield type variants replaced with RHF core-components equivalents (TEXT,ICON_PICKER,COLOR_PICKER,SWITCH);namepaths changed from antdNamePatharrays to RHF dot-notation strings.TagsPage.interface.ts/TagsPage.tsx: AddedTagFormValues,TagFormSelectItem,TAG_FORM_DEFAULTS;FormInstancereplaced withUseFormReturn;resetFields()→reset().Confidence Score: 4/5
Safe to merge with one fix: the name field's format and length validation rules must be restored before merge.
The migration is well-structured and the overall approach is sound. However,
TAG_NAME_VALIDATION_RULES— which enforced theTAG_NAME_REGEXcharacter-set check and the 2–64 character length constraint — were dropped from thenameFieldduring the antd → RHF refactor. Users can now submit tag/classification names that the backend will reject (single character, >64 chars, disallowed characters), silently turning a clear inline validation message into an API error toast.openmetadata-ui/src/main/resources/ui/src/pages/TagsPage/TagsForm.tsx — the
nameFieldmemo needs theTAG_NAME_VALIDATION_RULESpatterns re-attached via RHFrules.Important Files Changed
getDisabledFieldacceptsinitialValuebut silently ignores it.TagFormValues,TagFormSelectItem,TAG_FORM_DEFAULTS; replacedFormInstancewithUseFormReturnacross all prop interfaces.useFormfor RHFuseForm;resetFields()calls replaced withreset(); no logic changes.USER_TEAM_SELECT_MUIcase and itsMUIUserTeamSelectimport; other field type handlers unchanged.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant User participant SaveButton as Save Button (SlideoutMenu.Footer) participant submitRef as submitRef.current participant HookForm as HookForm / RHF participant TagsForm participant API User->>SaveButton: click SaveButton->>submitRef: submitRef.current() submitRef->>HookForm: form.handleSubmit(handleSave)() HookForm->>HookForm: validate fields (required only for name — pattern rules missing) HookForm->>TagsForm: handleSave(formData) TagsForm->>TagsForm: map owners/domains to EntityReference TagsForm->>API: "onSubmit(CreateClassification | CreateTag)" API-->>TagsForm: success / error TagsForm-->>HookForm: resolve HookForm-->>HookForm: "isSubmitSuccessful = true" HookForm-->>TagsForm: useEffect → descriptionEditorKey++ TagsForm-->>User: form reset / drawer closes%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant User participant SaveButton as Save Button (SlideoutMenu.Footer) participant submitRef as submitRef.current participant HookForm as HookForm / RHF participant TagsForm participant API User->>SaveButton: click SaveButton->>submitRef: submitRef.current() submitRef->>HookForm: form.handleSubmit(handleSave)() HookForm->>HookForm: validate fields (required only for name — pattern rules missing) HookForm->>TagsForm: handleSave(formData) TagsForm->>TagsForm: map owners/domains to EntityReference TagsForm->>API: "onSubmit(CreateClassification | CreateTag)" API-->>TagsForm: success / error TagsForm-->>HookForm: resolve HookForm-->>HookForm: "isSubmitSuccessful = true" HookForm-->>TagsForm: useEffect → descriptionEditorKey++ TagsForm-->>User: form reset / drawer closesComments Outside Diff (1)
openmetadata-ui/src/main/resources/ui/src/pages/TagsPage/tagFormFields.tsx, line 133-139 (link)initialValueparameter accepted but silently droppedgetDisabledFieldrenames theinitialValueparameter to_initialValueand never uses it, yet callers are still forced to pass a value. The disabled switch now reads its initial state purely fromform.reset(), so the parameter can be removed from the signature. Keeping the parameter in the public API is misleading and will confuse future callers.Reviews (2): Last reviewed commit: "lint fix" | Re-trigger Greptile