Revamped metric ontology and workflow header#29704
Conversation
anuj-kumary
added
UI
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| <button className="metric-actions-menu-item" type="button"> | ||
| <span className="metric-actions-icon"> | ||
| <File06 size={18} /> | ||
| </span> | ||
| <span> | ||
| <span className="metric-actions-title"> | ||
| {t('label.rename')} | ||
| </span> | ||
| <span className="metric-actions-description"> | ||
| {t('message.metrics-rename-collection-description')} | ||
| </span> | ||
| </span> | ||
| </button> | ||
| <button | ||
| className="metric-actions-menu-item metric-actions-menu-item-danger" |
There was a problem hiding this comment.
⚠️ Bug: Rename/Delete actions in metric menu are non-functional
In MetricListHeader.tsx the actions dropdown renders four items, but the "Rename" (lines 117-129) and "Delete" (lines 130-144) <button> elements have no onClick handler and no disabled state. They are always shown to users with permission.EditAll, so clicking them does nothing — a broken/dead UI action from the user's perspective. Either wire up the handlers (pass onRename/onDelete props from MetricListPage) or omit these items until they are implemented.
Was this helpful? React with 👍 / 👎
| <button | ||
| aria-busy={isExporting} | ||
| className="metric-actions-menu-item" | ||
| disabled={isExporting} | ||
| type="button" | ||
| onClick={onExport}> | ||
| <span className="metric-actions-icon"> | ||
| <Download01 size={18} /> | ||
| </span> | ||
| <span> | ||
| <span className="metric-actions-title"> | ||
| {t('label.export')} | ||
| </span> | ||
| <span className="metric-actions-description"> | ||
| {t('message.metrics-export-description')} |
There was a problem hiding this comment.
💡 Quality: Export/import menu items use raw instead of core components
The frontend guidelines call for prioritizing openmetadata-ui-core-components over ad-hoc elements. MetricListHeader.tsx builds the dropdown menu items as raw <button> elements with custom class names (metric-actions-menu-item) rather than using the core Dropdown/menu-item primitives already imported. This diverges from the design-system convention and relies on separate CSS. Consider using the core component menu items for consistency and accessibility (focus/keyboard handling).
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}
| {t('label.rename')} | ||
| </span> | ||
| <span className="metric-actions-description"> | ||
| {t('message.metrics-rename-collection-description')} | ||
| </span> | ||
| </span> | ||
| </button> | ||
| <button | ||
| className="metric-actions-menu-item metric-actions-menu-item-danger" | ||
| type="button"> | ||
| <span className="metric-actions-icon"> | ||
| <Trash01 size={18} /> | ||
| </span> | ||
| <span> | ||
| <span className="metric-actions-title"> | ||
| {t('label.delete')} | ||
| </span> | ||
| <span className="metric-actions-description"> | ||
| {t('message.metrics-delete-collection-description')} | ||
| </span> | ||
| </span> | ||
| </button> | ||
| </div> | ||
| </Dropdown.Popover> | ||
| </Dropdown.Root> | ||
| )} | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
The Rename (line 123) and Delete (line 136) buttons render with no onClick handler and no corresponding props in MetricListHeaderProps. Clicking them is a no-op. While these stubs existed in the original MetricListPage.tsx, extracting them into a formally typed component API makes the gap more visible — any Collate override that calls onRename/onDelete would need to add them to the interface. If these are intentional placeholders for future work, a // TODO comment with the tracking issue would clarify intent; if they're truly dead UI, they should be removed so the component matches its own contract.
| } from '../pages/OntologyExplorerPage/OntologyExplorerHeader'; | ||
|
|
||
|
|
||
| class OntologyExplorerClassBase { |
There was a problem hiding this comment.
| } from '../pages/OntologyExplorerPage/OntologyExplorerHeader'; | |
| class OntologyExplorerClassBase { | |
| } from '../pages/OntologyExplorerPage/OntologyExplorerHeader'; | |
| class OntologyExplorerClassBase { |
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!
1 participant
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.Greptile Summary
This PR refactors the Metric list page, Ontology Explorer page, and Workflows page headers by extracting inline JSX into dedicated
*Header.tsxcomponents and routing them through the class-base singleton pattern (MetricListPageClassBase,OntologyExplorerClassBase, updatedWorkflowClassBase). This enables Collate builds to swap in gradient-styled headers via the Vite class-replacement plugin without forking the shared page components.MetricListHeader,OntologyExplorerHeader,WorkflowsHeader) extract previously inline JSX with no logic changes; all props and state management remain in the parent pages.getHeader(): FC<Props>extension point so Collate overrides can inject a different header implementation without touching OSS code.MetricListHeadercarries over two non-functional stub buttons (Rename, Delete) with noonClickhandlers that were also stubs in the originalMetricListPage; now that they're part of a typed component API, they should either be wired up or removed.Confidence Score: 4/5
Safe to merge — the changes are a mechanical refactor that moves JSX into dedicated components with no logic alterations; the class-base wiring is straightforward and follows an established pattern in the codebase.
The Rename and Delete buttons in MetricListHeader are non-functional stubs (no onClick, not in the props interface), and getHeader() is called inside component render bodies rather than at module level, making all three pages mildly fragile against Collate overrides that return non-stable component references. Neither issue affects current behavior, but both deserve a follow-up before adding Collate overrides.
MetricListHeader.tsx — the stub Rename/Delete buttons and their absence from MetricListHeaderProps need resolution before adding Collate-side implementations.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD subgraph OSS ["OSS Build"] MLP["MetricListPage"] -->|"getHeader()"| MLCB["MetricListPageClassBase"] MLCB -->|returns| MLH["MetricListHeader"] OEP["OntologyExplorerPage"] -->|"getHeader()"| OECB["OntologyExplorerClassBase"] OECB -->|returns| OEH["OntologyExplorerHeader"] WP["WorkflowsPage"] -->|"getHeader()"| WCB["WorkflowClassBase"] WCB -->|returns| WH["WorkflowsHeader"] end subgraph Collate ["Collate Build - Vite plugin swaps ClassBase"] MLCB_C["MetricListPageClassCollate"] -->|"getHeader()"| MLH_C["GradientMetricHeader"] OECB_C["OntologyExplorerClassCollate"] -->|"getHeader()"| OEH_C["GradientOntologyHeader"] WCB_C["WorkflowClassCollate"] -->|"getHeader()"| WH_C["GradientWorkflowsHeader"] end MLP -.->|Collate override| MLCB_C OEP -.->|Collate override| OECB_C WP -.->|Collate override| WCB_C%%{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 subgraph OSS ["OSS Build"] MLP["MetricListPage"] -->|"getHeader()"| MLCB["MetricListPageClassBase"] MLCB -->|returns| MLH["MetricListHeader"] OEP["OntologyExplorerPage"] -->|"getHeader()"| OECB["OntologyExplorerClassBase"] OECB -->|returns| OEH["OntologyExplorerHeader"] WP["WorkflowsPage"] -->|"getHeader()"| WCB["WorkflowClassBase"] WCB -->|returns| WH["WorkflowsHeader"] end subgraph Collate ["Collate Build - Vite plugin swaps ClassBase"] MLCB_C["MetricListPageClassCollate"] -->|"getHeader()"| MLH_C["GradientMetricHeader"] OECB_C["OntologyExplorerClassCollate"] -->|"getHeader()"| OEH_C["GradientOntologyHeader"] WCB_C["WorkflowClassCollate"] -->|"getHeader()"| WH_C["GradientWorkflowsHeader"] end MLP -.->|Collate override| MLCB_C OEP -.->|Collate override| OECB_C WP -.->|Collate override| WCB_CComments Outside Diff (1)
openmetadata-ui/src/main/resources/ui/src/pages/MetricsPage/MetricListPage/MetricListPage.tsx, line 217 (link)getHeader()called inside the render body — same pattern in all three pagesmetricListPageClassBase.getHeader()(and the equivalent calls inOntologyExplorerPageandWorkflowsPage) runs on every render of the parent component. React uses reference equality to decide whether to reconcile or unmount+remount a child component: ifgetHeader()ever returns a different function reference between renders — e.g., an override that doesreturn (props) => <GradientHeader {...props}/>inline — React sees a new component type and unmounts the entire header subtree, discarding its local state. The current base-class implementations always return a stable module-level reference so it works today, but the pattern is fragile for Collate overrides. Moving these calls outside the component function (at module level) eliminates the risk entirely.Reviews (1): Last reviewed commit: "Revamped metric ontology and workflow he..." | Re-trigger Greptile