Skip to content

Fixes #27726: match domain colors on assigned assets#27727

Open
manavmax wants to merge 11 commits intoopen-metadata:mainfrom
manavmax:codex/fixes-27726-matching-domain-colors
Open

Fixes #27726: match domain colors on assigned assets#27727
manavmax wants to merge 11 commits intoopen-metadata:mainfrom
manavmax:codex/fixes-27726-matching-domain-colors

Conversation

@manavmax
Copy link
Copy Markdown

@manavmax manavmax commented Apr 24, 2026

Describe your changes:

Fixes #27726

I worked on matching domain colors on assigned assets because the issue reporter showed that colors configured in the Domains section were not carried over when the same domains appeared on tables and other assets.

I added a shared domain-style resolver/cache, updated the reusable domain label/display components to apply the saved color to the badge and icon, and replaced the plain domain rendering used in list/table views with the shared styled display. I also added focused tests covering both direct styled rendering and the fallback fetch path when the domain reference does not already include style.

How I tested:

  • yarn test src/components/common/DomainDisplay/DomainDisplay.test.tsx src/components/common/DomainLabel/DomainLabel.test.tsx --runInBand

Screenshots to attach:
27726-domain-colors-dropdown
27726-domain-colors-full
27726-domain-colors-list-card
27726-domain-colors-viewport

Type of change:

  • Bug fix

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes 27726: match domain colors on assigned assets
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

Summary by Gitar

  • Test stability enhancements:
    • Optimized Glossary.spec.ts to check asset counts via the dedicated /assets endpoint for faster verification.
    • Updated polling logic to rely on the total paging count rather than full glossary object retrieval.

This will update automatically on new commits.

@manavmax manavmax requested a review from a team as a code owner April 24, 2026 22:31
@github-actions
Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

Comment thread openmetadata-ui/src/main/resources/ui/src/utils/DomainStyleUtils.tsx Outdated
Comment thread openmetadata-ui/src/main/resources/ui/src/utils/DomainStyleUtils.tsx Outdated
@github-actions
Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@manavmax
Copy link
Copy Markdown
Author

I pushed follow-up fixes for the DomainStyleUtils review points in 5d44c04.

@harshach harshach added the safe to test Add this label to run secure Github workflows on PRs label Apr 25, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 25, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 61%
61.98% (61861/99807) 42.1% (33101/78608) 45.16% (9788/21674)

@rohan-jadhav-dev
Copy link
Copy Markdown

Hey @manavmax, nice work on this! 🙌 The domain color resolver approach makes total sense — it's clean and reusable across asset pages and table cells.

Just noticed the UI Checkstyle CI is failing — should be fixable by running make ui-checkstyle-changed locally to auto-fix the ESLint/Prettier/import issues before your next push.

Also the Playwright E2E tests are still running — fingers crossed those pass! 🤞

One small thing — did you also check if the color fallback works correctly when the domain reference doesn't include the style field yet? Since you mentioned handling that fetch path in your description, just curious if edge cases like that are covered in tests.

Overall solid fix though, looking forward to seeing this merged! 🚀

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 25, 2026

🔴 Playwright Results — 1 failure(s), 26 flaky

✅ 3946 passed · ❌ 1 failed · 🟡 26 flaky · ⏭️ 86 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 295 0 4 4
🟡 Shard 2 754 0 5 8
🟡 Shard 3 728 0 4 7
🟡 Shard 4 756 0 3 18
🟡 Shard 5 686 0 1 41
🔴 Shard 6 727 1 9 8

Genuine Failures (failed on all attempts)

Pages/Glossary.spec.ts › Add and Remove Assets (shard 6)
Error: �[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32m2�[39m
Received: �[31m-1�[39m

Call Log:
- Timeout 180000ms exceeded while waiting on the predicate
🟡 26 flaky test(s) (passed on retry)
  • Flow/Tour.spec.ts › Tour should work from help section (shard 1, 1 retry)
  • Flow/Tour.spec.ts › Tour should work from welcome screen (shard 1, 1 retry)
  • Flow/Tour.spec.ts › Tour should work from URL directly (shard 1, 1 retry)
  • Pages/UserCreationWithPersona.spec.ts › Create user with persona and verify on profile (shard 1, 1 retry)
  • Features/ActivityAPI.spec.ts › Activity event is created when description is updated (shard 2, 1 retry)
  • Features/ActivityAPI.spec.ts › Activity event shows the actor who made the change (shard 2, 1 retry)
  • Features/CuratedAssets.spec.ts › Entity type "ALL" with basic filter (shard 2, 1 retry)
  • Features/DomainFilterQueryFilter.spec.ts › Domain filter should persist across page navigation (shard 2, 1 retry)
  • Features/DomainFilterQueryFilter.spec.ts › Domain filter should use exact match and prefix with dot to prevent false positives (shard 2, 1 retry)
  • Features/RTL.spec.ts › Verify Following widget functionality (shard 3, 1 retry)
  • Flow/IngestionBot.spec.ts › Ingestion bot should be able to access domain specific domain (shard 3, 1 retry)
  • Flow/LineageSettings.spec.ts › Verify global lineage config (shard 3, 2 retries)
  • Flow/NestedChildrenUpdates.spec.ts › should add and remove tags to nested column immediately without refresh (shard 3, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for Table (shard 4, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for SearchIndex (shard 4, 1 retry)
  • Pages/DataContractsSemanticRules.spec.ts › Validate DataProduct Rule Any_In (shard 4, 1 retry)
  • Pages/ExplorePageRightPanel.spec.ts › Should verify deleted user not visible in owner selection for table (shard 5, 1 retry)
  • Pages/InputOutputPorts.spec.ts › Lineage section collapse/expand (shard 6, 1 retry)
  • Pages/Lineage/DataAssetLineage.spec.ts › verify create lineage for entity - Stored Procedure (shard 6, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify lineage schema filter selection (shard 6, 1 retry)
  • Pages/Lineage/LineageRightPanel.spec.ts › Verify custom properties tab IS visible for supported type: searchIndex (shard 6, 1 retry)
  • Pages/ODCSImportExport.spec.ts › Multi-object ODCS contract - object selector shows all schema objects (shard 6, 1 retry)
  • Pages/Tags.spec.ts › Verify Owner Add Delete (shard 6, 1 retry)
  • Pages/Teams.spec.ts › Teams Page Flow (shard 6, 1 retry)
  • Pages/UserDetails.spec.ts › Admin user can edit teams from the user profile (shard 6, 1 retry)
  • Pages/Users.spec.ts › Update token expiration for Data Steward (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@manavmax
Copy link
Copy Markdown
Author

Hi @rohan-jadhav-dev Thanks for the review. I pushed a follow-up commit
5f2421b to address the UI Checkstyle issues and the Playwright-related CI failures, so I’m waiting on the rerun now.

For the domain color fallback: yes, that path is handled in the shared resolver. If the domain reference comes in without style, we fetch the domain with fields=style and then apply the resolved color. That unresolved-domain path is covered in tests as well, along with the cache/error behavior, since the same resolver is used across the asset views and table cells.

@sonarqubecloud
Copy link
Copy Markdown

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Apr 26, 2026

Code Review 👍 Approved with suggestions 3 resolved / 4 findings

Aligns asset colors with domain styles by resolving caching issues and unstable effect dependencies. Relaxing the status badge assertion may mask state-machine bugs, so consider tightening this check.

💡 Quality: Relaxed status assertion may mask real state-machine bugs

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryWorkflow.spec.ts:186 📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryWorkflow.spec.ts:335

The status badge assertion was changed from an exact 'Draft' match to /Draft|In Review/. While this stabilizes flaky tests caused by timing (the workflow may advance before the assertion fires), it also means a test will pass even if the term incorrectly skips straight to 'In Review' when it should remain 'Draft'. Consider using toHaveText('Draft') with a short retry/poll, or asserting a deterministic precondition (e.g., wait for the row to appear, then immediately check status) so the test remains precise about the expected initial state.

✅ 3 resolved
Bug: Failed API fetches are never cached, causing retry storms

📄 openmetadata-ui/src/main/resources/ui/src/utils/DomainStyleUtils.tsx:68-82
When getDomainByName rejects (e.g., network error, 404, 500), the .then block that populates domainStyleCache is skipped, and .finally removes the in-flight request from domainStyleRequestCache. On the next render (or effect re-run), domainStyleCache.has(domainFqn) still returns false, so fetchDomainStyle fires again. This creates a retry loop — every re-render of any component using useDomainsWithStyle will re-issue failed API calls, potentially flooding the server if many assets reference a domain whose API consistently errors.

Edge Case: Module-level cache persists forever without eviction

📄 openmetadata-ui/src/main/resources/ui/src/utils/DomainStyleUtils.tsx:22-23
The domainStyleCache Map lives at module scope and is never evicted except by clearDomainStyleCache() (only used in tests). In a long-lived SPA session, if domain colors are updated by an admin, users will see stale colors until they do a full page refresh. This may be acceptable for now, but consider adding a TTL or clearing the cache on domain-related mutations (e.g., after domain update API calls).

Edge Case: useEffect depends on domains array reference identity

📄 openmetadata-ui/src/main/resources/ui/src/utils/DomainStyleUtils.tsx:132
The useEffect at line 92 depends on [domains]. Since arrays are compared by reference, if the parent component creates a new array on each render (e.g., domains={[activeDomain]} inline), the effect will re-run every render even if the domain content hasn't changed. This won't cause visible bugs (the cache check prevents re-fetching), but it will trigger unnecessary state updates. Consider using a stable serialization (e.g., JSON.stringify(domains?.map(d => d.fullyQualifiedName))) or a custom comparison hook.

🤖 Prompt for agents
Code Review: Aligns asset colors with domain styles by resolving caching issues and unstable effect dependencies. Relaxing the status badge assertion may mask state-machine bugs, so consider tightening this check.

1. 💡 Quality: Relaxed status assertion may mask real state-machine bugs
   Files: openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryWorkflow.spec.ts:186, openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryWorkflow.spec.ts:335

   The status badge assertion was changed from an exact `'Draft'` match to `/Draft|In Review/`. While this stabilizes flaky tests caused by timing (the workflow may advance before the assertion fires), it also means a test will pass even if the term incorrectly skips straight to 'In Review' when it should remain 'Draft'. Consider using `toHaveText('Draft')` with a short retry/poll, or asserting a deterministic precondition (e.g., wait for the row to appear, then immediately check status) so the test remains precise about the expected initial state.

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Matching Domain Colors

3 participants