Conversation
…ificationTag and Icon into openmetadata-ui-core-components
Rohit0301
added
the
safe to test
❌ 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 |
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
Code Review ✅ Approved 3 closed / 3 findingsRefactor moving ✅ 3 closed✅ Bug: AutoClassificationTag no longer sets --tag-color for close icon
✅ Quality: AUTO_CLASSIFICATION_TAG_COLOR is now dead + comment is stale
✅ Quality: tag-chip-content emits literal 'undefined' class when labelClassName omitted
OptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // `vendor-untitled` chunk regardless of code-splitting, so the moved code | ||
| // still counts against this app's eager bootstrap bytes even though nothing | ||
| // new is rendered. ~5 KiB of headroom above the post-move measurement. | ||
| const MAX_HTML_BOOTSTRAP_JS_BROTLI_BYTES = 975 * 1024; |
There was a problem hiding this comment.
Raising the bootstrap limit from 970 KiB to 975 KiB admits an eager-payload increase caused by this migration instead of preserving the existing performance ratchet. The migrated tags are exported through the core package’s root barrel, and the app groups that package into the bootstrap vendor-untitled chunk. As a result, code that renders nothing new adds roughly 5 KiB to the initial download. Please keep the migration payload-neutral or isolate these exports rather than increasing the threshold.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| @@ -9127,14 +9127,6 @@ | |||
| "playwright/e2e/Pages/DataContracts.spec.ts" | |||
| ] | |||
| }, | |||
There was a problem hiding this comment.
Removing the dedicated GlossaryTag.tsx to Glossary.spec.ts edge leaves the relocated component covered only by the broad core-components mapping, whose three selected specs do not include the glossary flow. Because that broad entry marks the file as mapped, the full-suite fallback for unmapped code does not apply. A glossary-tag rendering or navigation regression can therefore pass targeted PR checks and surface later. Please preserve the Glossary spec mapping for the relocated source.
Knowledge Base Used:
|
| Count | Rule |
|---|---|
| 54 | react-hooks/exhaustive-deps |
| 42 | openmetadata-imports/no-circular-imports |
| 7 | no-restricted-imports |
| 6 | openmetadata-imports/no-internal-barrel-imports |
| 4 | openmetadata-ui-patterns/no-raw-title-attribute |
| 3 | openmetadata-imports/no-lower-layer-page-imports |
| 2 | openmetadata-imports/review-sequential-api-calls |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | src/components/Certification/Certification.component.tsx:136:9 |
react-hooks/exhaustive-deps |
The 'handleScroll' function makes the dependencies of useMemo Hook (at line 242) change on every render. Move it inside the useMemo callback. Alternatively, wra |
| 🟡 | src/components/Certification/Certification.component.tsx:278:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'currentCertificate' and 'getCertificationData'. Either include them or remove the dependency array. If 'setSelec |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:42:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:43:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:44:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:81:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:85:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:89:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:98:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:99:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:103:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:104:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:105:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:106:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:110:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:116:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:117:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:118:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:119:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:258:5 |
react-hooks/exhaustive-deps |
React Hook useMemo has a missing dependency: 'hasFollowers'. Either include it or remove the dependency array. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:382:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'entityType', 'fetchActiveAnnouncement', 'fetchContainerAncestors', and 'fetchDQFailureCount'. Either include the |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:560:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has a missing dependency: 'dataAsset'. Either include it or remove the dependency array. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:696:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has a missing dependency: 'dataAsset.fullyQualifiedName'. Either include it or remove the dependency array. |
| 🟡 | src/components/DataAssets/DataAssetsHeader/DataAssetsHeader.component.tsx:772:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'fetchDataContract'. Either include it or remove the dependency array. |
| 🟡 | src/components/Domain/AddDomainForm/AddDomainForm.component.tsx:90:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/components/ExploreV1/ExploreSearchCard/ExploreSearchCard.tsx:44:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/ExploreV1/ExploreSearchCard/ExploreSearchCard.tsx:48:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/ExploreV1/ExploreSearchCard/ExploreSearchCard.tsx:51:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/ExploreV1/ExploreSearchCard/ExploreSearchCard.tsx:53:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/ExploreV1/ExploreSearchCard/ExploreSearchCard.tsx:54:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/ExploreV1/ExploreSearchCard/ExploreSearchCard.tsx:56:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/ExploreV1/ExploreSearchCard/ExploreSearchCard.tsx:58:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/ExploreV1/ExploreSearchCard/ExploreSearchCard.tsx:59:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/ExploreV1/ExploreSearchCard/ExploreSearchCard.tsx:839:8 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 'renderOwnerContent', 't', and 'toOwnersWithHref'. Either include them or remove the dependency array. |
| 🟡 | src/components/Glossary/GlossaryHeader/GlossaryHeader.component.tsx:631:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'isGlossary', 'onAddGlossaryTerm', and 'selectedData'. Either include them or remove the dependency array. If ' |
| 🟡 | src/components/Glossary/GlossaryHeader/GlossaryHeader.component.tsx:711:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has a missing dependency: 'showModal'. Either include it or remove the dependency array. |
| 🟡 | src/components/Glossary/GlossaryHeader/GlossaryHeader.component.tsx:773:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 'handleAddGlossaryTermClick' and 't'. Either include them or remove the dependency array. |
| 🟡 | src/components/Glossary/GlossaryHeader/GlossaryHeader.component.tsx:815:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'handleBreadcrumb'. Either include it or remove the dependency array. |
| 🟡 | src/components/Glossary/GlossaryHeader/GlossaryHeader.component.tsx:821:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'fetchCurrentGlossaryInfo' and 'isVersionView'. Either include them or remove the dependency array. |
| 🟡 | src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:116:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:515:34 |
react-hooks/exhaustive-deps |
React Hook useCallback received a function whose dependencies are unknown. Pass an inline function instead. |
| 🟡 | src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:519:9 |
react-hooks/exhaustive-deps |
The 'fetchChildTerms' function makes the dependencies of useCallback Hook (at line 1022) change on every render. To fix this, wrap the definition of 'fetchChild |
| 🟡 | src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:603:22 |
openmetadata-imports/review-sequential-api-calls |
Review these sequential API requests. If they are independent, start them together with Promise.all/Promise.allSettled; keep sequencing only when data-dependent |
| 🟡 | src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:618:9 |
react-hooks/exhaustive-deps |
The 'fetchAllTerms' function makes the dependencies of useCallback Hook (at line 1339) change on every render. To fix this, wrap the definition of 'fetchAllTerm |
| 🟡 | src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:686:9 |
react-hooks/exhaustive-deps |
The 'fetchExpadedTree' function makes the dependencies of useCallback Hook (at line 1339) change on every render. To fix this, wrap the definition of 'fetchExpa |
| 🟡 | src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:820:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'fetchAllTerms', 'handlePagingChange', and 'setGlossaryChildTerms'. Either include them or remove the dependency |
| 🟡 | src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:832:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'setGlossaryChildTerms'. Either include it or remove the dependency array. |
| 🟡 | src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:994:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'applyGlossaryTermApprovalOutcome', 'notifyPendingApprovalTasks', and 't'. Either include them or remove the de |
| 🟡 | src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1258:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 'currentUser', 'onAddGlossaryTerm', 'onEditGlossaryTerm', 'renderOwnerContent', 't', and 'toOwnersWithHref'. Either |
| 🟡 | src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1398:5 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 'handleCheckboxChange' and 't'. Either include them or remove the dependency array. |
… and 68 more. Run make ui-checkstyle-changed locally for the full list.
Fix locally (fast - only checks files changed in this branch):
make ui-checkstyle-changed
|
2 participants
Describe your changes:
Fixes #
I worked on ... because ...
Type of change:
High-level design:
N/A — small change.
Tests:
Use cases covered
Unit tests
Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.Summary by Gitar
ClassificationTag,GlossaryTag,DomainTag,DataProductTag,AutoClassificationTag, andIconintoopenmetadata-ui-core-components@openmetadata/ui-core-componentsThis will update automatically on new commits.
Greptile Summary
This PR relocates the five entity-tag variants and the configurable Icon implementation into
@openmetadata/ui-core-components, migrates application imports, adds a dedicated Icon package subpath, and moves tag styling and tests into the shared library.iconsubpath.Confidence Score: 4/5
The PR appears safe to merge functionally, but the non-blocking eager-bundle regression and lost targeted Glossary test coverage should be addressed.
The component, icon, routing, styling, and package-export contracts remain compatible; the remaining findings concern additional initial-download cost and a concrete reduction in targeted Playwright coverage.
Files Needing Attention: openmetadata-ui/src/main/resources/ui/scripts/check-bundle-chunks.mjs; .github/playwright/impact-map.generated.json
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR App[OpenMetadata UI callers] --> Root[Core components root barrel] Root --> Tags[Shared entity-tag variants] Tags --> Badge[Badge and BadgeWithButton] Tags --> LazyIcon[Lazy configurable Icon] LazyIcon --> IconEntry[Dedicated icon subpath] Root --> Vendor[vendor-untitled bootstrap chunk] CoreCSS[Core globals.css] --> TagsReviews (1): Last reviewed commit: "Merge branch 'main' into move-tags-to-co..." | Re-trigger Greptile
Context used: