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 1 commit 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}
| const submitData = { | ||
| ...formData, | ||
| owners: owners.length ? owners : undefined, | ||
| domains: domainsData, | ||
| } as CreateClassification | CreateTag; |
There was a problem hiding this comment.
⚠️ Bug: Create payload includes extraneous id:'' from form defaults
handleSave builds the submit payload with const submitData = { ...formData, owners, domains }. Because both forms are initialized with TAG_FORM_DEFAULTS, which sets id: '', formData.id is '' in create mode and is spread into submitData. This object is passed unchanged to createTag(...) / createClassification(...).
The generated CreateTag and CreateClassification interfaces have no id field, so id: '' is an extraneous property. The previous antd implementation only submitted registered Form.Item values, so no id was ever sent — this is a behavior regression. Sending id: '' risks a server-side validation error (empty string parsed as a UUID) or a silently created entity with an unexpected id, depending on backend leniency. Edit/patch flows are unaffected because convertToTagFormValues populates the real id and compare() produces no diff.
Strip id (and any other non-payload fields) before submitting in create mode.
Destructure id out of the form data so it is not included in the create payload.:
const handleSave = useCallback(
async (formData: TagFormValues) => {
const { id: _id, ...rest } = formData;
const owners = (rest.owners ?? []).map(
(item) => item.value as EntityReference
);
const domainItems = rest.domains ?? [];
// ...compute domainsData from domainItems as before...
const submitData = {
...rest,
owners: owners.length ? owners : undefined,
domains: domainsData,
} as CreateClassification | CreateTag;
// ...
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| multiple: | ||
| entityRules.canAddMultipleUserOwners || | ||
| entityRules.canAddMultipleTeamOwner, | ||
| options: userTeamOptions, | ||
| onFocus: handleUserTeamFocus, |
There was a problem hiding this comment.
The old owner field accepted separate multipleUser and multipleTeam flags, letting entity rules control them independently. The new implementation collapses those into a single multiple: canAddMultipleUserOwners || canAddMultipleTeamOwner. When canAddMultipleUserOwners is false but canAddMultipleTeamOwner is true, the OR produces true, so users can now pick multiple user-owners even though the entity rule forbids it. The inverse case (multiple users allowed, single team) is similarly broken. This silently over-permits owner selection for any entity whose rules have asymmetric user/team multiplicity.
| import { Suspense, useCallback, useEffect, useMemo, useState } from 'react'; | ||
| import { useTranslation } from 'react-i18next'; | ||
| import { useWatch } from 'react-hook-form'; |
There was a problem hiding this comment.
organize-imports will fail
react-hook-form is an external library import and should be grouped with the other external imports before react-i18next. The yarn organize-imports:cli check enforced by CI will reject this ordering.
| import { Suspense, useCallback, useEffect, useMemo, useState } from 'react'; | |
| import { useTranslation } from 'react-i18next'; | |
| import { useWatch } from 'react-hook-form'; | |
| import { Suspense, useCallback, useEffect, useMemo, useState } from 'react'; | |
| import { useWatch } from 'react-hook-form'; | |
| import { useTranslation } from 'react-i18next'; |
| try { | ||
| await onSubmit(submitData); | ||
| form.reset(TAG_FORM_DEFAULTS); | ||
| } catch { | ||
| // Parent will handle the error | ||
| } |
There was a problem hiding this comment.
form.reset() inside handleSave
onSubmit resolves only after the parent's handleTagFormSubmit (or handleClassificationFormSubmit) has already called handleTagDrawerClose() → tagForm.reset(). The form.reset(TAG_FORM_DEFAULTS) below is therefore a second reset of the same form object. This also risks interfering with the isSubmitSuccessful flag that the descriptionEditorKey effect depends on — handleSubmit sets isSubmitSuccessful = true after handleSave resolves, but a prior form.reset() inside handleSave clears that flag. Removing the explicit reset here leaves the parent as the single, clear owner of post-submit cleanup.
| try { | |
| await onSubmit(submitData); | |
| form.reset(TAG_FORM_DEFAULTS); | |
| } catch { | |
| // Parent will handle the error | |
| } | |
| try { | |
| await onSubmit(submitData); | |
| } catch { | |
| // Parent will handle the error | |
| } |
❌ UI Checkstyle Failed❌ ESLint + Prettier + Organise Imports (src)One or more source files have linting or formatting issues. Affected files
Fix locally (fast - only checks files changed in this branch): make ui-checkstyle-changed |
🔴 Playwright Results — 3 failure(s), 27 flaky✅ 4477 passed · ❌ 3 failed · 🟡 27 flaky · ⏭️ 50 skipped
Genuine Failures (failed on all attempts)❌
|
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.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
TagsFormand its related drawers from the legacy Ant DesignForm/FormInstancepattern to React Hook Form, following theAddDomainFormreference pattern. Owner and domain selectors are switched from MUI variants to autocomplete-basedUSER_TEAM_SELECT_INPUT/DOMAIN_SELECTfields with async search and debounce.TagsForm.tsx: Replacesantd FormwithHookForm; adds async user/team and domain fetch logic with debounce; introduces an externalsubmitRefpattern so the Save button outside the form can trigger validated submission; description field is now wired directly viaFormField+RichTextEditor.tagFormFields.tsx: All_MUIfield type variants replaced with RHF core-components equivalents;getDescriptionFieldremoved; owner and domain fields now acceptoptions/onFocus/onSearchChangeprops; field name paths changed from antdNamePatharrays to RHF dot-notation strings.TagsPage.interface.ts/TagsPage.tsx:FormInstancereplaced withUseFormReturn<TagFormValues>;resetFields()calls updated toreset(); newTagFormValues,TagFormSelectItem, andTAG_FORM_DEFAULTStypes added.Confidence Score: 3/5
The migration is generally sound but has a behaviour change in owner multi-select that could silently allow more owners than entity rules permit, and an import ordering issue that will break CI.
The owner field collapses two independent entity-rule flags into a single OR condition, which over-permits multi-select when only one flag is true. A redundant form.reset() inside handleSave also risks interfering with the isSubmitSuccessful flag that drives the description-editor remount. The import order in TagsForm.tsx will fail the CI organize-imports lint check. The external submitRef pattern and RHF wiring are otherwise well-structured.
openmetadata-ui/src/main/resources/ui/src/pages/TagsPage/TagsForm.tsx — owner multi-select logic, redundant reset, and import order; openmetadata-ui/src/main/resources/ui/src/pages/TagsPage/tagFormFields.tsx — dead initialValue parameter in getDisabledField.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant User participant Drawer as ClassificationFormDrawer / TagFormDrawer participant TagsForm participant RHF as React Hook Form participant API User->>Drawer: clicks Save button Drawer->>TagsForm: submitRef.current() TagsForm->>RHF: form.handleSubmit(handleSave)() RHF->>RHF: run validation alt validation fails RHF-->>TagsForm: show field errors else validation passes RHF->>TagsForm: handleSave(formData) TagsForm->>TagsForm: map owners/domains to EntityReference TagsForm->>API: onSubmit(submitData) API-->>TagsForm: success TagsForm->>RHF: form.reset(TAG_FORM_DEFAULTS) [redundant] TagsForm-->>RHF: handleSave resolves RHF->>RHF: "isSubmitSuccessful = true" RHF-->>TagsForm: effect increments descriptionEditorKey API-->>Drawer: onSubmit resolves Drawer->>RHF: tagForm.reset() via handleDrawerClose Drawer->>User: drawer closes end%%{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 Drawer as ClassificationFormDrawer / TagFormDrawer participant TagsForm participant RHF as React Hook Form participant API User->>Drawer: clicks Save button Drawer->>TagsForm: submitRef.current() TagsForm->>RHF: form.handleSubmit(handleSave)() RHF->>RHF: run validation alt validation fails RHF-->>TagsForm: show field errors else validation passes RHF->>TagsForm: handleSave(formData) TagsForm->>TagsForm: map owners/domains to EntityReference TagsForm->>API: onSubmit(submitData) API-->>TagsForm: success TagsForm->>RHF: form.reset(TAG_FORM_DEFAULTS) [redundant] TagsForm-->>RHF: handleSave resolves RHF->>RHF: "isSubmitSuccessful = true" RHF-->>TagsForm: effect increments descriptionEditorKey API-->>Drawer: onSubmit resolves Drawer->>RHF: tagForm.reset() via handleDrawerClose Drawer->>User: drawer closes endComments 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 (1): Last reviewed commit: "refactor(ui): migrate TagsForm from antd..." | Re-trigger Greptile