Repository navigation
Fixes #34976: Align dark-theme surfaces and controls across shared UI - #34977
Conversation
shah-harshit
added
UI
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Swap PR-added var(--om-color-*) to the core design-system var(--tw-color-*) equivalents, and drop caller-side overrides on core Card that duplicate its default border/surface.
…bservability-issues
|
| Count | Rule |
|---|---|
| 34 | react-hooks/exhaustive-deps |
| 1 | openmetadata-imports/review-sequential-api-calls |
| 1 | openmetadata-imports/no-lower-layer-page-imports |
| 1 | openmetadata-ui-patterns/no-non-adaptive-palette |
| 1 | openmetadata-imports/no-api-calls-in-iteration |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | src/components/DataProducts/DataProductsDetailsPage/DataProductsDetailsPage.component.tsx:902:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 'getEntityFeedCount', 'isVersionsView', and 'openAssetDrawer'. Either include them or remove the dependency array. |
| 🟡 | src/components/DataProducts/DataProductsDetailsPage/DataProductsDetailsPage.component.tsx:940:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'fetchActiveAnnouncement', 'fetchActivityCount', 'fetchDataProductAssets', 'fetchDataProductContract', and 'fetch |
| 🟡 | src/components/DataProducts/DataProductsDetailsPage/DataProductsDetailsPage.component.tsx:953:5 |
react-hooks/exhaustive-deps |
React Hook useMemo has a missing dependency: 'tabs'. Either include it or remove the dependency array. |
| 🟡 | src/components/DataProducts/DataProductsDetailsPage/DataProductsDetailsPage.component.tsx:953:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has a complex expression in the dependency array. Extract it to a separate variable so it can be statically checked. |
| 🟡 | src/components/DataProducts/DataProductsDetailsPage/DataProductsDetailsPage.component.tsx:995:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 'handleTabChange' and 't'. Either include them or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/DataObservability/DataObservabilityTab.tsx:125:44 |
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/Database/Profiler/DataObservability/DataObservabilityTab.tsx:163:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'fetchNewIncidentCount'. Either include it or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:138:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has a missing dependency: 't'. Either include it or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:155:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has a missing dependency: 't'. Either include it or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:403:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'onVisibilityChange', 't', and 'tableId'. Either include them or remove the dependency array. If 'onVisibilityC |
| 🟡 | src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:410:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'onVisibilityChange' and 'updateInitialConfig'. Either include them or remove the dependency array. If 'onVisib |
| 🟡 | src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:417:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has a missing dependency: 'handleStateChange'. Either include it or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:425:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has a missing dependency: 'handleStateChange'. Either include it or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:432:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has a missing dependency: 'handleStateChange'. Either include it or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:457:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'form' and 'handleStateChange'. Either include them or remove the dependency array. |
| 🟡 | src/components/Database/Profiler/TableProfiler/ProfilerSettingsModal/ProfilerSettingsModal.tsx:477:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'fetchProfileConfig'. Either include it or remove the dependency array. |
| 🟡 | src/components/Domain/DomainDetails/DomainDetails.component.tsx:359:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'domain.fullyQualifiedName' and 't'. Either include them or remove the dependency array. |
| 🟡 | src/components/Domain/DomainDetails/DomainDetails.component.tsx:361:9 |
react-hooks/exhaustive-deps |
The 'handleTabChange' function makes the dependencies of useCallback Hook (at line 470) change on every render. To fix this, wrap the definition of 'handleTabCh |
| 🟡 | src/components/Domain/DomainDetails/DomainDetails.component.tsx:361:9 |
react-hooks/exhaustive-deps |
The 'handleTabChange' function makes the dependencies of useCallback Hook (at line 639) change on every render. To fix this, wrap the definition of 'handleTabCh |
| 🟡 | src/components/Domain/DomainDetails/DomainDetails.component.tsx:749:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'closeSubDomainDrawer' and 't'. Either include them or remove the dependency array. |
| 🟡 | src/components/Domain/DomainDetails/DomainDetails.component.tsx:948:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 'activeTab', 'addSubDomain', 'getEntityFeedCount', 'isVersionsView', 'onAddDataProduct', 'onDeleteSubDomain', and ' |
| 🟡 | src/components/Domain/DomainDetails/DomainDetails.component.tsx:978:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'fetchActiveAnnouncement', 'fetchActivityCount', 'fetchDataProducts', 'fetchDomainAssets', and 'fetchTaskCounts'. |
| 🟡 | src/components/Domain/DomainDetails/DomainDetails.component.tsx:991:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has an unnecessary dependency: 'isSubDomain'. Either exclude it or remove the dependency array. |
| 🟡 | src/components/Domain/DomainDetails/DomainDetails.component.tsx:999:5 |
react-hooks/exhaustive-deps |
React Hook useMemo has a missing dependency: 'tabs'. Either include it or remove the dependency array. |
| 🟡 | src/components/Domain/DomainDetails/DomainDetails.component.tsx:999:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has a complex expression in the dependency array. Extract it to a separate variable so it can be statically checked. |
| 🟡 | src/components/DomainListing/DomainListPage.tsx:291:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 'domainColumns', 'domainListing.handleSelect', and 'domainListing.handleSelectAll'. Either include them or remove t |
| 🟡 | src/components/common/EntityDescription/Description.tsx:112:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has a missing dependency: 'navigate'. Either include it or remove the dependency array. |
| 🟡 | src/components/common/EntityDescription/Description.tsx:116:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has a missing dependency: 'navigate'. Either include it or remove the dependency array. |
| 🟡 | src/components/common/SelectableList/SelectableList.component.tsx:150:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has a missing dependency: 'checkActiveSelectedItem'. Either include it or remove the dependency array. |
| 🟡 | src/components/common/SelectableList/SelectableList.component.tsx:166:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'fetchOptionFailed' and 'fetchOptions'. Either include them or remove the dependency array. If 'fetchOptions' c |
| 🟡 | src/components/common/SelectableList/SelectableList.component.tsx:170:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'fetchListOptions'. Either include it or remove the dependency array. |
| 🟡 | src/components/common/SelectableList/SelectableList.component.tsx:176:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'selectedItemsInternal' and 'sortUniqueListFromSelectedList'. Either include them or remove the dependency array. |
| 🟡 | src/components/common/SelectableList/SelectableList.component.tsx:203:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'fetchOptions' and 'sortUniqueListFromSelectedList'. Either include them or remove the dependency array. If 'fe |
| 🟡 | src/components/common/UserTeamSelectableList/UserTeamSelectableList.component.tsx:335:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'init'. Either include it or remove the dependency array. |
| 🟡 | src/components/observability/Alerts/AlertAiFormFields.constants.ts:14:1 |
openmetadata-imports/no-lower-layer-page-imports |
Pages are route-level composition modules. Move the shared implementation/type to a lower layer instead of importing a page from here. |
| 🟡 | src/components/observability/Alerts/AlertAiFormFields.constants.ts:57:5 |
openmetadata-ui-patterns/no-non-adaptive-palette |
Raw palette class "tw:hover:text-error-600, tw:hover::data-icon:text-error-600, tw:focus:text-error-600, tw:focus::data-icon:text-error-600" is static — it do |
| 🟡 | src/pages/ContextCenterPage/ContextCenterAIContextPage/ContextCenterAIContextPage.tsx:62:40 |
openmetadata-imports/no-api-calls-in-iteration |
Avoid issuing one API request per item. Fetch at the data owner, use a bulk endpoint, or use useQueries with an intentional concurrency policy. |
| 🟡 | src/pages/ContextCenterPage/ContextCenterDashboardPage/ContextCenterDashboardPage.tsx:202:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has an unnecessary dependency: 't'. Either exclude it or remove the dependency array. |
Fix locally (fast - only checks files changed in this branch):
make ui-checkstyle-changed…bservability-issues # Conflicts: # openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Tasks/TaskCreation.spec.ts # openmetadata-ui/src/main/resources/ui/playwright/utils/glossary.ts # openmetadata-ui/src/main/resources/ui/playwright/utils/incidentManager.ts # openmetadata-ui/src/main/resources/ui/playwright/utils/task.ts # openmetadata-ui/src/main/resources/ui/playwright/utils/taskWorkflow.ts # openmetadata-ui/src/main/resources/ui/src/pages/TasksPage/RequestDescriptionPage/RequestDescriptionPage.tsx # openmetadata-ui/src/main/resources/ui/src/pages/TasksPage/shared/Assignees.tsx # openmetadata-ui/src/main/resources/ui/src/pages/TasksPage/shared/DescriptionTabs.tsx
…bservability-issues # Conflicts: # openmetadata-ui/src/main/resources/ui/src/components/UploadFile/UploadFile.tsx
|
✅ SSO Login Nightly — PASSED View workflow run · Commit
Totals: 94 passed · 0 failed · 0 flaky · 0 skipped · duration 19m 41s |
The profile-sample-type string has no {{type}} placeholder, so the value
and query fields rendered the same label as the sample-type select.
…bservability-issues
The popover dialog clipped its content at the viewport-capped height, cutting off Clear all / Cancel / Update. Let the dialog inherit that max height and scroll instead.
This reverts commit c99d05d.
Restore core FeaturedIcon (global light-mode change), the Classic left sidebar (AI mode is the only target), and drop the caller-side bg-raised overrides on core popovers together with the core popoverClassName type.
The shared user/team picker now uses the core ButtonUtility trigger (28px), which makes the Organization header 4px taller. Baseline taken from the CI visual-regression artifact (same Playwright image as the other baselines).
Code Review ✅ Approved 2 closed / 2 findings🟡 Medium risk · Shared dark-theme updates span 60 frontend files and migrate dialogs and selectors; styling or popup-containment regressions could obscure controls or disrupt workflows. Dark-theme alignment across Observability, Data Marketplace, Context Center, Entity Details, Glossary, and Classification shared UI, using semantic tokens and core components. Resolved this session: the assignee rewrite that broke Playwright selectors and dropped the Form.Item id, and the AntD dropdowns in SlideoutMenu that portaled outside the modal. No open issues found. ✅ 2 closed✅ Bug: Assignees rewrite breaks Playwright selectors and drops Form.Item id
✅ Bug: Antd dropdowns in react-aria SlideoutMenu still portal outside the modal
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 |
|
3 participants
Describe your changes
Fixes #34976
Part of open-metadata/openmetadata-collate#7154 (Governance items 4.04 and 4.14)
Apply 30 issues from the Notion dark-theme review, Observability onward, including the supplied sidebar recording and the four pending follow-ups. All four pending follow-ups belong to OSS; custom properties use the core elevated style, including its heavier shadow.
64 files: 54 source files (33 TypeScript, 15 LESS, 6 SVG), 3 Jest test files, 2
.spec.ts, 4 Playwright helpers, and 1 CI-generated impact-map JSON.Counterpart: https://github.com/open-metadata/openmetadata-collate/pull/7241. Consume the shared OSS changes before the dependent Collate changes; no submodule pointer changes are included.
getPopupContainer.Type of change
High-level design
Repeated patterns fixed at shared boundaries: hard-coded light fills/borders; child cards blending into parent cards; static SVG colors; selected/hover icon overrides; overlays sharing drawer backgrounds; and legacy selectors after core-component migrations. Entity details reuse the shared description, asset-card, custom-property, profiler, activity-panel, and editor fixes rather than route-specific copies.
Use existing core components and semantic tokens. Preserve form validation, permissions, async selection, uploaded icons, chart colors, and specialized profiler fields. Collate additionally normalizes nested message content and waits for nonempty artifacts before opening a panel; later updates respect dismissal. OpenMetadata remains at the Collate base's recorded submodule commit
9071b8b5ed3190d97b7ce8f0c9c0684f75510519.Tests
Use cases covered by the changes
The issue/fix table above describes the scope. Local visual checks cover both themes; focused browser cases cover the highest-impact forms, navigation, editors, ER view switching, and artifact recovery.
Unit tests
Local CI-failure reproduction: the three failing suites reproduced all 22 failing tests. After the fix, 9 suites / 83 tests pass, 0 failures on
6d11f7d1dafccff73773974b8847470d1ce37c08:DataAssetSummaryPanelV1.test.tsx: preserve real core exports in the partial mock so the migrated user/team picker renders.UploadFile.test.tsx: assert the real upload SVG uses semantic foreground andcurrentColor, replacing the obsolete asset mock.TableV2Utils.test.ts: assert semantic surface, hover, and selected classes while retaining the no-inline-body-background check.ProfilerSettingsModal.test.tsx,SelectableList.test.tsx,UserTeamSelectableList.test.tsx,UserTeamSelectableList.popover.test.tsx,StyleModal.test.tsx, andDomainDetails.component.test.tsx.Measured line coverage from this focused run: profiler settings 90.18%, upload 93.33%, asset summary 87.50%, and table utilities 45.31%; combined 82.85% lines / 59.27% branches. This is focused coverage, not whole-application coverage. The full Jest suite also passes: 1,866 suites / 21,525 tests, 0 failures in GitHub UI Coverage CI, including the SonarCloud scan and UI Sonar gate. GitHub UI Checkstyle passes on
6d11f7d1dafccff73773974b8847470d1ce37c08. Local Jest used the current workspace core build and an ignored module-resolution configuration for the shared dependency installation. No committed test configuration, coverage exclusions, or thresholds changed.After merging main
82656b57a6cinto4a0c9ea51c5309beb4acd470c4d3c3c0d5f36606, the same nine focused Jest suites reran: 83 passed, 0 failed. The profiler conflict preserves main's core Grid/Box layout alongside this PR's core drawer/buttons and all seven popup containers. Changed-file ESLint, Prettier, deprecation guard, andgit diff --checkpass. The full CI results linked above belong to the preceding6d11f7d1dafcommit; CI reruns on the merge commit.Backend integration tests
Not applicable — frontend-only changes; no API/schema/migration changes.
Ingestion integration tests
Not applicable — no ingestion changes.
Playwright (UI) tests
TaskCreation.spec.ts(full spec)TaskAssigneeManagement.spec.tsGlossary.spec.ts— Update Glossary and Glossary TermClassificationImportExport.spec.ts— import navigatesBlockEditorEmbedLink.spec.tsProfilerSettingsAfterReindex.spec.tsDomainUIInteractions.spec.ts— Edit domain styleAll seven selected OSS specs ran after the latest main merge on OSS
f828272429bwith Collate5fbfdcd. After the profiler popup review fix, the extended profiler/reindex case reran against the final committed source tree and passed, including accessible options and keyboard sample-type switching. Other selected cases are unaffected by that final two-file change. Task creation also verifies the accessible Assignees label and clicking the label focuses the combobox. Earlier task attempts failed during low disk/backend downtime and on the required-label selector; the latest full spec passes. The profiler regression was corrected during development to use the displayed Row Count option and the AntD selector surface rather than its covered input; its final run passes.Local execution: one spec at a time, installed Chrome, one worker, zero retries, no full visual-regression suite. Verified source tree committed as
5472f5a5a114daa7104706eae95d126c2e0b4959. Coordinated frontend: OSS5472f5a5a114daa7104706eae95d126c2e0b4959, Collate5fbfdcd7062346b51374516daf727fb8ec8e6c9f, Vitehttp://127.0.0.1:3191; backendhttp://localhost:8585, Collate2.0.0-SNAPSHOT, revision103aecd645195ed64513b76cd8905e2fea595622, MySQL/Elasticsearch. The later bot commit8e2e75ec9d44217a2011e20dc2aa5ebf991bce07only refreshes.github/playwright/impact-map.generated.json; its JSON parses successfully and application/test code matches the tested commit. CI follow-up6d11f7d1dafccff73773974b8847470d1ce37c08reranProfilerSettingsAfterReindex.spec.ts: 1 passed / 0 failed / 0 flaky in 13.7 seconds on the same coordinated frontend/backend. Popup containment now usesgetPopupContaineron all existing Select/TreeSelect controls, avoiding a new AntD import while preserving modal accessibility. Earlier attempts hit the stopped frontend and a timeout during development-server refresh; the stable rerun passes. The other previously selected specs above are unchanged. No real LLM or nightly infrastructure suites ran.Merge follow-up
4a0c9ea51c5309beb4acd470c4d3c3c0d5f36606:ProfilerSettingsAfterReindex.spec.tsreran locally on the same Vite3191/ backend8585stack: 1 passed, 0 failed/flaky, 9.9 seconds. This verifies popup accessibility, keyboard sample-type switching, save, and reindex persistence with the merged Grid/Box layout. Other selected specs were not rerun for this one-file conflict resolution.Manual testing performed
dim_addresscustom properties and ER Diagram; compare canvas/card separation and Diagram/Table selected icons against main.Exact CI import organizer (organize-imports-cli 0.10.0), ESLint, and Prettier pass across 40 application/Playwright TypeScript files; a second pass makes no changes. Core lint/formatting pass for both changed core files. AntD/SVG deprecation guard, license headers, i18n sync, app-doc generation, and
git diff --checkpass. I18n and app-doc generation produce no tracked changes. The earlier design-token comparison matched the baseline 88 errors / 47 warnings; the added Classic submenu styles also use semantic variables. All changed LESS files compiled and SVGs parsed. Focused TypeScript has three existing changed/global diagnostics (missing local lodash-es types, existing SelectableList ref type, existing translated field-label type), plus 861 other dependency diagnostics; a clean whole-project typecheck is not claimed. No new runtime code was added for those existing type issues.UI screen recording / screenshots
Main comparisons use OSS
fc6a14d76d6ea58c4a13d49991e44e9423dc805aand Collate5d0de83; an ignored comparison fixture selects the initial theme without changing component styles. Fixed images use the coordinated working branches. Light/dark before-and-after images are followed by additional fixed views.Additional verified views
Checklist