Conversation
Migrate AddDomainForm from Ant Design to @openmetadata/ui-core-components using HookForm, getField, and FieldTypes. Add onFocus prop support to AutocompleteBase, add rules prop to FormField, and export form-field components from the core library. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- Switch FieldTypes.SELECT to use Select with Select.Item children for proper icon/avatar/supportingText support - Pass fontSize through SelectContext so items respect the fontSize prop - Show colored-initial avatars for users and team icons via Avatar - Show tag color dots using Dot component on tag options - Replace custom glossary terms autocomplete with MUIGlossaryTagSuggestion Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
siddhant1
requested review from
a team,
chirag-madlani and
karanh37
as code owners
|
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! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR refactors the Domain “Add” form UI to use @openmetadata/ui-core-components + react-hook-form (instead of Ant Design Form), while introducing reusable form-field building blocks in openmetadata-ui-core-components to support the new form patterns.
Changes:
- Refactor
AddDomainFormtoreact-hook-form+ ui-core-components fields (icon picker, color picker, tag/domain/user selectors, etc.). - Add a new shared form-field system to
openmetadata-ui-core-components(Field,getField,FormItemLabel,FieldTypes, etc.) and export it from the components entrypoint. - Extend core select/autocomplete plumbing (e.g.,
SelectContextnow includesfontSize; autocomplete can invoke anonFocuscallback).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/src/components/Domain/AddDomainForm/AddDomainForm.component.tsx | Rebuilds the Domain/Data Product form using react-hook-form + ui-core-components fields and a custom imperative ref bridge. |
| openmetadata-ui/src/main/resources/ui/src/components/Domain/AddDomainForm/AddDomainForm.interface.ts | Replaces AntD FormInstance with a custom DomainFormRef contract for submit/reset/validate. |
| openmetadata-ui/src/main/resources/ui/src/components/Domain/AddDomainForm/AddDomainForm.test.tsx | Updates tests/mocks to align with the refactored form and new ref behavior. |
| openmetadata-ui-core-components/src/main/resources/ui/src/components/index.ts | Exports the newly added form-field primitives/types. |
| openmetadata-ui-core-components/src/main/resources/ui/src/components/base/select/select.tsx | Adds fontSize to SelectContext and provides it via SelectContext.Provider. |
| openmetadata-ui-core-components/src/main/resources/ui/src/components/base/select/select-item.tsx | Uses fontSize from context to control item label/supporting text typography. |
| openmetadata-ui-core-components/src/main/resources/ui/src/components/base/form/hook-form.tsx | Adds rules support to FormField props for react-hook-form validation. |
| openmetadata-ui-core-components/src/main/resources/ui/src/components/base/autocomplete/autocomplete.tsx | Adds onFocus passthrough (used to resize popover on focus) during trigger focus. |
| openmetadata-ui-core-components/src/main/resources/ui/src/components/application/form-field/form-item-label.tsx | New shared label component with required marker, tooltip, and beta badge. |
| openmetadata-ui-core-components/src/main/resources/ui/src/components/application/form-field/form-field.types.ts | New shared types/enums for form fields (FieldTypes, FieldProp, etc.). |
| openmetadata-ui-core-components/src/main/resources/ui/src/components/application/form-field/form-field.tsx | New implementation of Field/getField rendering for multiple input types. |
| if ('current' in targetRef) { | ||
| targetRef.current = value; | ||
|
|
||
| return () => { | ||
| if (targetRef.current === value) { | ||
| targetRef.current = null; | ||
| } | ||
| : null; | ||
|
|
||
| const iconField: FieldProp = { | ||
| name: 'iconURL', | ||
| id: 'root/iconURL', | ||
| label: t('label.icon'), | ||
| muiLabel: t('label.icon'), | ||
| required: false, | ||
| type: FieldTypes.ICON_PICKER_MUI, | ||
| helperText: iconTooltipDataRender(), | ||
| props: { | ||
| 'data-testid': 'icon-url', | ||
| allowUrl: true, | ||
| placeholder: t('label.icon-url'), | ||
| backgroundColor: selectedColor, | ||
| defaultIcon: | ||
| }; |
There was a problem hiding this comment.
syncFormRef assigns to targetRef.current, but formRef is typed as Ref<DomainFormRef> where RefObject.current is readonly under @types/react v18. This will cause a TS error and/or require unsafe mutation of a readonly ref.
Consider changing DomainFormRefProp to accept MutableRefObject<DomainFormRef | null> (and/or RefObject separately) and casting inside this branch, or using useImperativeHandle with a forwarded ref instead of mutating .current directly.
| const fetchDomainOptions = useCallback(async (searchText = '') => { | ||
| try { | ||
| const domains = await searchDomains(searchText, 1); | ||
| const nextOptions = domains.map((domain: any) => | ||
| mapEntityReferenceToOption({ | ||
| displayName: domain.displayName, | ||
| fullyQualifiedName: domain.fullyQualifiedName, | ||
| id: domain.id, | ||
| name: domain.name, | ||
| type: EntityType.DOMAIN, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
domains.map((domain: any) => ...) introduces an any in UI code. This breaks the repo’s zero-tolerance type-safety requirement and also hides the shape expected from searchDomains().
Prefer typing the response from searchDomains() (e.g., Domain[] or a minimal interface with id/name/displayName/fullyQualifiedName) and removing the any cast.
| const expertsField: FieldProp = { | ||
| id: 'root/experts', | ||
| label: t('label.expert-plural'), | ||
| }, | ||
| formItemProps: { | ||
| valuePropName: 'value', | ||
| trigger: 'onChange', | ||
| initialValue: [], | ||
| }, | ||
| }; | ||
|
|
||
| const createPermission = useMemo( | ||
| () => | ||
| checkPermission(Operation.Create, ResourceEntity.GLOSSARY, permissions), | ||
| [permissions] | ||
| ); | ||
|
|
||
| const selectedOwners = | ||
| Form.useWatch<EntityReference | EntityReference[]>('owners', form) ?? []; | ||
|
|
||
| const ownersList = Array.isArray(selectedOwners) | ||
| ? selectedOwners | ||
| : [selectedOwners]; | ||
|
|
||
| const expertsList = Form.useWatch<EntityReference[]>('experts', form) ?? []; | ||
|
|
||
| const handleFormSubmit: FormProps['onFinish'] = (formData) => { | ||
| const updatedData = omit( | ||
| formData, | ||
| 'color', | ||
| 'iconURL', | ||
| 'glossaryTerms' | ||
| // Keep 'coverImage' - parent will extract and remove it before API call | ||
| // Don't exclude 'domains' - we need it for DataProducts | ||
| ); | ||
| const style = { | ||
| color: formData.color, | ||
| iconURL: formData.iconURL, | ||
| // Don't include coverImage here - it's not uploaded yet | ||
| // Parent will add it to style after upload | ||
| name: 'experts', | ||
| placeholder: t('label.select-field', { | ||
| field: t('label.expert-plural'), | ||
| }), | ||
| props: { | ||
| filterOption: () => true, | ||
| multiple: true, | ||
| onFocus: handleUserTeamFocus, | ||
| onSearchChange: (searchText: string) => | ||
| debouncedUserTeamSearch(searchText), | ||
| options: userTeamOptions, | ||
| }, | ||
| type: FieldTypes.USER_TEAM_SELECT, | ||
| }; |
There was a problem hiding this comment.
experts in CreateDomain/CreateDataProduct is defined as a list of user/login names (string[]), but the options backing expertsField (userTeamOptions) include teams. Selecting a team here will submit a team name as an expert, which is likely invalid and can break creation.
Consider fetching users-only options for the experts field (or filtering userTeamOptions by type === EntityType.USER) so only users can be selected as experts.
| export const SelectContext = createContext<{ | ||
| size: 'sm' | 'md'; | ||
| fontSize: 'xs' | 'sm' | 'md' | 'lg' | 'xl'; | ||
| }>({ | ||
| size: 'sm', | ||
| fontSize: 'md', | ||
| }); | ||
|
|
||
| const Select = ({ | ||
| placeholder = 'Select', | ||
| placeholderIcon, | ||
| size = 'sm', | ||
| fontSize = 'md', | ||
| children, | ||
| items, | ||
| label, | ||
| hint, | ||
| tooltip, | ||
| className, | ||
| ...rest | ||
| }: SelectProps) => { | ||
| return ( | ||
| <SelectContext.Provider value={{ size }}> | ||
| <SelectContext.Provider value={{ size, fontSize }}> | ||
| <AriaSelect |
There was a problem hiding this comment.
SelectContext now requires fontSize in its value shape. This is a breaking change for existing SelectContext.Provider usages that only provide { size } (e.g., Select ComboBox / Autocomplete providers), and will either fail type-checking or make fontSize undefined downstream.
To avoid regressions, update all SelectContext.Provider values to include fontSize (or change the context type to make fontSize optional with a default).
siddhant1
added
the
safe to test
…rm PR Fix jsx-sort-props errors in form-field.tsx, replace `any` with `Domain` type, use MutableRefObject to fix readonly ref assignment, add missing fontSize to SelectContext providers, and restrict experts field to user-only options matching original behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…y modules Split the monolithic form-field.tsx (~1290 lines) into focused files: - fields/color-picker-field.tsx and fields/icon-picker-field.tsx for standalone field components - render-field-element.tsx for the central field type dispatch - form-field.tsx reduced to thin Field/FormFields wrapper (~95 lines) Replaced Record<string, unknown> FieldPropsMap with a properly typed interface, eliminated all `as` casts and `unknown` types, and removed the unnecessary select normalization layer (consumers pass correctly shaped SelectItemType data). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
openmetadata-ui/src/main/resources/ui/src/components/Domain/AddDomainForm/AddDomainForm.test.tsx:253
- The refactor changes submission/validation behavior substantially, but the “should handle form submission” test doesn’t assert that
onSubmitis invoked or that the payload is correctly shaped (e.g., tags/owners/experts/domains mapping, coverImage file value). Strengthen this test to populate key fields, trigger submit, and verify the exactonSubmitarguments (andformRef.validateFields()behavior if that’s part of the external contract).
it('should handle form submission', () => {
render(<AddDomainForm {...defaultProps} />);
const saveButton = screen.getByTestId('save-domain');
fireEvent.click(saveButton);
| <button | ||
| aria-label={`Select color ${color}`} | ||
| aria-pressed={isSelected} |
There was a problem hiding this comment.
Can we use here core component button instead of native element ?
Addresses @anuj-kumary's review on #26951: replace native <button>, <span>, and flex-column <div> with core-library components wherever they fit. - color-picker-field: swatch <button> → Button; empty-state <span> → Typography; outer flex <div> → Box. - icon-picker-field: tile <button> + trigger <button> → Button; all user-visible <span> (fallback "?", first-letter initial, empty state, URL panel label) → Typography; URL-panel wrapper → Box. - form-field: field wrapper <div> → Box. - render-field-element: COVER_IMAGE_UPLOAD wrapper <div> → Box. - AddDomainForm (consumer): four flex-container <div> (icon+color row, name+displayName row, description wrapper, CTA row) → Box. Grid layouts, positional wrappers, and inline-flex label spans are left as native elements — Box/Typography would change layout or DOM semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Runs eslint/prettier autofix that CI's lint-core-components expected. - Bumps the icon-picker dropdown to 22rem and grid gap to gap-4 so the 6-column icon grid has visible breathing room. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Refactored AddDomainForm renders Box unconditionally and Autocomplete / Avatar / Dot in the tag-suggestion and user-team paths. The existing jest.mock of @openmetadata/ui-core-components omitted these, so every render through AddDomainFormHarness threw "Element type is invalid", failing all 11 describe('AddDomainForm') tests on SonarCloud.
Add passthrough stubs (Autocomplete with .Item, Avatar, Box, Dot) to the mock so the component tree resolves.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The first batch in this test added two siblings from a mutuallyExclusive glossary and relied on the server rejecting the save so the UI stayed empty and `add-tag` remained clickable for the next batch. TreeAsyncSelectList.handleChange now drops mutex siblings client-side once its glossaries-list fetch resolves, so only one term reaches the server, it saves successfully, tags become non-empty, and `add-tag` is replaced by `edit-button`. The test then times out waiting for `add-tag`. Whether the filter kicks in depends on a race between the dropdown's `/api/v1/glossaries?fields=mutuallyExclusive` call and the user clicks, which is why the test sometimes passed and sometimes timed out at 3 minutes under CI load. Remove the mutex batch (and the now-unused glossary1/term1/term2 fixtures). The mutex flow was never actually asserted here — it was only used as a side channel to keep tags empty. The remaining non-mutex batch + chart-level tagging + asset verification still cover the add/remove-assets behavior end-to-end. Local: 5/5 passes in ~7s each (was timing out at 180s). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
URL-detection for the custom-icon preview no longer lives inside the core picker. hasCustomImage (allowUrl + non-empty value + no selectedItem) is enough to decide whether to render the <img>; matches main's identity of keeping image-validity rules in consumer utils, not in the picker itself. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Strip changes that weren't required by the form-library migration so the diff against main is purely the MUI → Untitled UI swap: - utils/IconUtils.tsx + test: restore getEntityAvatarProps to main. The ICON_MAP[iconURL] lookup for stored icon names is new rendering behaviour, not needed by the form refactor. - AddDomainForm.component.tsx: drop the getImageDimensions helper and the coverImage rules.validate block. On main that size/dimension check lived inside MUICoverImageUpload via maxSizeMB/maxDimensions props — keep the same principle (backend rejects on upload), don't reimplement it in the form. - playwright DomainUIInteractions.spec.ts: restore the 'Invalid@Name#Test' input and .ant-form-item-explain-error selector. The branch had swapped both the test input and the assertion target; neither was required by the drawer testid migration. - playwright Glossary.spec.ts: restore to match main. The 67-line flaky-batch deletion is unrelated to the Add Domain form migration. - core form-item-label.tsx / form-field.types.ts / form-field.tsx: drop the unused isBeta / betaLabel props — no consumer sets them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hook-form defaults color / iconURL to '', which made the submit payload
include `style: { color: '', iconURL: '' }`. Because getEntityAvatarProps
uses ?? (not ||) for the brand-600 fallback, empty strings defeated the
default and avatars rendered with no background. Strip empty values at
the submit boundary so the POST payload matches what the Ant-based form
on main produced.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Untitled UI icon picker stores preset icon selections as the icon's name (e.g. 'Bank') in entity.style.iconURL. Main's getEntityAvatarProps only handled http/absolute-path URLs, so any stored icon name fell through to the entity-type default — the user's picked icon never showed up in list views, tree views, cards, or detail headers. Look up non-URL iconURL values in ICON_MAP and render the matching icon component as the avatar's placeholderIcon. Unknown names still fall back to the entity-type default. Restore the two tests that cover both paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Input didn't actually violate ENTITY_NAME_REGEX (`/^((?!::).)*$/`), and the selector targeted the old AntD error class which the HookForm refactor removed. Use a `::` input and match the message via text. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Dedupes the 14-color swatch palette between ColorPickerField and its icon-picker sibling into a single ENTITY_PALETTE_HEX export under @/colors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
…tyles Use ENTITY_PALETTE_HEX directly instead of re-exporting it as DEFAULT_COLOR_OPTIONS, and express icon display/stroke-width via Tailwind utilities so the picker fields no longer carry an inline style object. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
normalizeHexColor canonicalizes hex strings to uppercase, so both sides of every comparison were already in the same case. Replace the .toLowerCase() pairs with plain equality (and palette.some with palette.includes), and drop the redundant array copy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review ✅ Approved 10 resolved / 10 findingsRefactored the domain creation form to ensure type safety, correct payload transformation, and stable event handling. All identified validation, permission, and submission issues have been successfully resolved. ✅ 10 resolved✅ Quality: Untyped
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
Summary
Refactors the Add Domain / Sub-Domain / Data Product form onto the
@openmetadata/ui-core-componentsstack. The form now usesHookForm,FormField, typedFieldTypes, anduseFormDrawer— the ad-hoc Ant Design + RJSF pieces are gone. Shared form plumbing moves into the core components library so other forms can reuse it.What changed
AddDomainForm (consumer)
DomainFormValuesand aDomainFormSelectItemshape (tags / owners / experts / domainType / domains all flow through the same select contract).transformDomainFormData(formData, type, parentDomain)builds theCreateDomainorCreateDataProductAPI payload: merges glossary terms into tags, packagescolor + iconURLintostyle, resolves the data-product parent-domain fallback, and strips UI-only fields.submitAndClosehelper + stableadd-domain-formtest-id; scroll-to-error on validation failure.Boxprimitive instead of rawtw:flexdivs.Core components (
openmetadata-ui-core-components)form-fieldrestructured into typed, single-responsibility field renderers (color-picker-field,icon-picker-field,render-field-element,form-item-label, splitform-field.types).AutocompleteforwardsonBlur;FormFieldforwardsonFocus.Alertguards against empty-stringhelperText.render-field-elementno longer special-cases aMULTIPLE_SELECTION_FIELD_TYPESset — callers declare multiplicity explicitly.Button,Typography, andBoxprimitives instead of native<button>/<span>/<div>— addresses @anuj-kumary's review feedback on this PR.Callers migrated to the new form
DomainListPage,DomainDetails,AddSubDomainModal,DataProductListPage, and both Marketplace widgets (MarketplaceDomainsWidget,MarketplaceDataProductsWidget) now invoketransformDomainFormDatabefore POSTing.Tests
transformDomainFormDatacovering domain, sub-domain, and data-product payload shapes — tag + glossary merging, style packaging, owner / expert extraction, nulldomainType→undefined, missing expert name →'', empty-collection handling, and the data-product parent-domain fallback (9 cases).DataMarketplace,DataProducts,DataProductAndSubdomains,DomainUIInteractions,Domainsspecs updated to target the new drawer test-ids and tag-option selectors; sharedplaywright/utils/domain.tshelpers adjusted.AddDomainForm.test.tsxcomponent tests updated for the new renderer and hook-form harness.Type of change
Checklist
Summary by Gitar
DomainassetdryRuninteractions, verifying move confirmations and warning modal behavior.domain.tsfor managing domain assets viadryRunand commit workflows.getActiveAnnouncementtogetActiveAnnouncementsinDomainDetailsto support updated announcement entity fetching.This will update automatically on new commits.