refactor(entity-rename): replace antd with core-components in EntityNameModal#29783
refactor(entity-rename): replace antd with core-components in EntityNameModal#29783anuj-kumary wants to merge 4 commits into
Conversation
… in EntityNameModal - Replace antd Form, Modal, Button, Typography with @openmetadata/ui-core-components equivalents (Dialog, ModalOverlay, Modal, Button, Typography) - Use react-hook-form (Controller/useForm) instead of antd Form context - Add SanitizedField helper wrapping InputBase in AriaTextField so onChange receives a string and id flows through to the DOM <input>, preserving Playwright locators (#name, #displayName, #name_help, [role="dialog"]) - Update EntityNameModal.interface.ts: remove antd Rule import, add EntityNameValidationRule object type - Update ManageButton.tsx: cast DISPLAY_NAME_FIELD_RULES as EntityNameValidationRule[] - Update SchemaTable.component.tsx: replace antd Form context for constraint with local editConstraint state; cast updateColumnDetails arg as Partial<Column> - Update entityPanel.ts: replace .ant-modal selector with [role="dialog"] Co-Authored-By: Claude Sonnet 4.6 <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 |
anuj-kumary
added
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}
| @@ -169,6 +165,7 @@ const SchemaTable = () => { | |||
| } = useFqn({ type: EntityType.TABLE }); | |||
There was a problem hiding this comment.
editConstraint loses Constraint enum type
The old implementation stored the constraint value inside the antd Form context typed as the Constraint enum; the new state is useState<string | undefined>(), which widens the type. The Select's onChange still receives a valid Constraint string at runtime, but TypeScript can no longer enforce it — a future change that accidentally passes an arbitrary string into setEditConstraint would not be caught at compile time. Consider typing this as useState<Constraint | undefined>() to preserve the original type safety.
| const modal = page.locator('[role="dialog"]'); | ||
| await modal.waitFor({ state: 'visible' }); |
There was a problem hiding this comment.
[role="dialog"] may match multiple elements in test context
Replacing .ant-modal (a CSS class unique to the Ant Design modal) with [role="dialog"] is semantically correct, but the new selector is far more generic — any ARIA dialog rendered on the page matches it. If another dialog (e.g., a confirmation toast or a nested modal from a different component) is visible when editDisplayNameFromPanel runs, page.locator('[role="dialog"]') will return multiple elements. Playwright's strict-mode will then throw a strict mode violation error when the subsequent .locator('#displayName') is called. Consider scoping the locator, e.g. page.locator('[role="dialog"][data-testid="…"]') or using page.getByRole('dialog', { name: … }) to match by dialog title.
Move the rules prop after render on the name/displayName Controllers to satisfy the sort-props lint rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- buildValidate: skip min/max/pattern for empty non-required values, mirroring antd async-validator behavior (clearing displayName no longer produces a spurious min-length error) - useEffect reset: depend on entity.name/entity.displayName primitives instead of the entity object reference, so inline object literals passed by callers (e.g. ManageButton) don't wipe in-progress edits on every parent re-render Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| for (const rule of rules) { | ||
| // Skip length/pattern checks for empty non-required values, | ||
| // mirroring antd async-validator behavior. | ||
| if (v.length === 0 && !rule.required) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
💡 Bug: buildValidate reads required but never enforces presence
The delta added if (v.length === 0 && !rule.required) { continue; } in buildValidate, which introduces awareness of the required field declared on EntityNameValidationRule (interface L25). However, required is only used to decide whether to skip checks for empty values — it never actually enforces presence. For a rule with { required: true } and no min/max/pattern, an empty value falls through: the continue is skipped, min/max are undefined (skipped), and there's no pattern to fail on, so buildValidate returns true. Any future caller passing a required rule via nameValidationRules/displayNameValidationRules would be silently misled into thinking empty values are rejected. (The name field's actual required check happens separately via react-hook-form's required rule at L186, so current behavior is unaffected.) Consider enforcing it: when v.length === 0 && rule.required, return the required message instead of just relying on it to skip.
Return the required message for empty values when the rule is required, otherwise skip length/pattern checks.:
for (const rule of rules) {
// Enforce required first for empty values.
if (v.length === 0) {
if (rule.required) {
return typeof rule.required === 'string'
? rule.required
: rule.message ?? '';
}
// Skip length/pattern checks for empty non-required values,
// mirroring antd async-validator behavior.
continue;
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
❌ 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 |
Previous commit accidentally renamed DeleteWidgetModal → DeleteEntityModal causing all test suites that import ManageButton to fail with "Cannot find module '../../DeleteWidget/DeleteEntityModal'". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review 👍 Approved with suggestions 2 resolved / 3 findingsReplaces Ant Design components in 💡 Bug: buildValidate reads
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
Jest test CoverageUI tests summary
|
|
🔴 Playwright Results — 3 failure(s), 23 flaky✅ 4462 passed · ❌ 3 failed · 🟡 23 flaky · ⏭️ 54 skipped
Genuine Failures (failed on all attempts)❌
|
1 participant
Why
EntityNameModaland its callers (SchemaTable,ManageButton,entityPanel) still used Ant Design components. This PR removes them and replaces them with@openmetadata/ui-core-componentsas part of the ongoing MUI/core-components migration.What changed
EntityNameModal.component.tsxForm,Modal,Button,Typographywith core-componentsDialog,ModalOverlay,Modal,Button,TypographyFormcontext toreact-hook-form(Controller/useForm)SanitizedFieldhelper that wrapsInputBaseinAriaTextField(react-aria-components) soonChangereceives astringandidflows throughInputContext→<input id="name">, preserving all Playwright locatorsEntityNameModal.interface.tsRuleimportEntityNameValidationRuleobject type (replacesRule[])ManageButton.tsxDISPLAY_NAME_FIELD_RULESasEntityNameValidationRule[]SchemaTable.component.tsxFormcontext for the column constraint field with localeditConstraintstateupdateColumnDetailsargument asPartial<Column>(handlesremoveConstraintfrom theUpdateColumnAPI type)playwright/utils/entityPanel.ts.ant-modalselector with[role="dialog"](AriaDialog renders with that role)Playwright compatibility
#nameidonAriaTextFieldflows viaInputContextto the<input>#displayName#name_help<span id="name_help">from react-hook-formfieldState.errordata-testid="save-button"Button[role="dialog"].ant-modalinentityPanel.tsTest plan
allowRenamedefaults tofalse)#name_help)editDisplayNameFromPanel,EntityRenameConsolidation.spec.ts🤖 Generated with Claude Code
Greptile Summary
This PR migrates
EntityNameModaland its callers from Ant Design components to@openmetadata/ui-core-components, swapping the antdForm/Modal/Buttonstack forreact-hook-form+Dialog/ModalOverlay/Buttonfrom the new component library.EntityNameModal.component.tsx: Rewrites form state management from antd Form context to react-hook-formController/useForm, adds aSanitizedFieldhelper to bridgereact-aria-componentsTextFieldwithInputBase, and preserves all Playwright-facing IDs (#name,#displayName,#name_help).SchemaTable.component.tsx: Extracts the column constraint value into localeditConstraintstate sinceadditionalFieldsare now plain JSX outside react-hook-form; cleans up theEntityNameWithAdditionFieldstype dependency.ManageButton.tsx/EntityNameModal.interface.ts: Removes the antdRuledependency; introduces a customEntityNameValidationRuletype and castsDISPLAY_NAME_FIELD_RULESto the new type.Confidence Score: 5/5
Safe to merge — the refactor correctly replaces antd form primitives with react-hook-form and core-components across all callers without introducing new functional regressions.
The component migration is mechanically straightforward: form state moves from antd Form context to react-hook-form, constraint state is lifted to local useState in SchemaTable (the only caller using additionalFields), and all Playwright-facing IDs are preserved via the SanitizedField/AriaTextField bridge. The onSave error-path loading-state issue and the [role="dialog"] selector broadening were both present before this PR or already flagged in prior review threads. No new data-loss or correctness bugs are introduced by the changed code paths.
No files require special attention beyond the concerns already raised in prior review threads.
Important Files Changed
requiredrule on EntityNameValidationRule (flagged in prior review).Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[User clicks Edit] --> B[handleEditDisplayNameClick] B --> C[setEditColumnDisplayName\nsetEditConstraint] C --> D[EntityNameModal renders\nModalOverlay isOpen=true] D --> E[useForm initialised\nreset on visible=true] E --> F{User action} F -->|Edits name/displayName| G[Controller onChange\nSanitizedField → getSanitizeContent] G --> F F -->|Changes constraint| H[antd Select onChange\nsetEditConstraint local state] H --> F F -->|Clicks Save / presses Enter| I[handleSubmit runs react-hook-form validation] I -->|Validation fails| J[fieldState.error shown\nspan#name_help] J --> F I -->|Validation passes| K[onSubmit called\nsetIsLoading true] K --> L[onSave data as T] L --> M[setEditColumnDisplayName undefined\nsetEditConstraint undefined] F -->|Clicks Cancel| N[onCancel\nsetEditColumnDisplayName undefined] N --> O[Modal unmounts] M --> O%%{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"}}}%% flowchart TD A[User clicks Edit] --> B[handleEditDisplayNameClick] B --> C[setEditColumnDisplayName\nsetEditConstraint] C --> D[EntityNameModal renders\nModalOverlay isOpen=true] D --> E[useForm initialised\nreset on visible=true] E --> F{User action} F -->|Edits name/displayName| G[Controller onChange\nSanitizedField → getSanitizeContent] G --> F F -->|Changes constraint| H[antd Select onChange\nsetEditConstraint local state] H --> F F -->|Clicks Save / presses Enter| I[handleSubmit runs react-hook-form validation] I -->|Validation fails| J[fieldState.error shown\nspan#name_help] J --> F I -->|Validation passes| K[onSubmit called\nsetIsLoading true] K --> L[onSave data as T] L --> M[setEditColumnDisplayName undefined\nsetEditConstraint undefined] F -->|Clicks Cancel| N[onCancel\nsetEditColumnDisplayName undefined] N --> O[Modal unmounts] M --> OReviews (3): Last reviewed commit: "fix(entity-rename): restore DeleteWidget..." | Re-trigger Greptile