Skip to content

Fix(UI): Add conditional rendering for ClassificationDetails GenericProvider#26120

Open
siddhant1 wants to merge 4 commits intomainfrom
fix/classification-details-conditional-rendering
Open

Fix(UI): Add conditional rendering for ClassificationDetails GenericProvider#26120
siddhant1 wants to merge 4 commits intomainfrom
fix/classification-details-conditional-rendering

Conversation

@siddhant1
Copy link
Member

@siddhant1 siddhant1 commented Feb 26, 2026

Summary

  • Conditionally render GenericProvider in ClassificationDetails only when currentClassification is defined, preventing child components (DomainLabelV2, OwnerLabelV2) from receiving an empty {} object and accessing undefined properties
  • Show a <Loader /> while classification data is being fetched
  • Remove unsafe as Classification type assertion and ?? {} fallback

Test plan

  • Navigate between classifications — verify Loader appears during transitions
  • Verify classification details (description, tags table, domain, owner) render correctly after loading
  • Simulate API error — verify ErrorPlaceHolder renders correctly
  • Verify version history button and manage button still work as expected

…rovider

Prevent GenericProvider from receiving an empty object when
currentClassification is undefined by conditionally rendering
it only when data is available. Show a Loader during fetch.
This fixes potential runtime errors in child components
(DomainLabelV2, OwnerLabelV2) that destructure properties
from the provider context data.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@github-actions
Copy link
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!

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@github-actions
Copy link
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!

Add 5 test cases covering the new Loader/GenericProvider conditional
rendering: loading state, gap state, loaded state, GenericProvider
data prop, and the classification-switching scenario.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
…al rendering

Add 4 E2E test cases covering the loader/content transition:
- Initial page load: loader disappears then content renders
- All detail sections render after loading
- Switching between classifications renders correct content
- Page reload renders classification without blank state

Co-Authored-By: Claude Opus 4.6 <[email protected]>
import Table from '../../common/Table/Table';
import { GenericProvider } from '../../Customization/GenericProvider/GenericProvider';
import { DomainLabelV2 } from '../../DataAssets/DomainLabelV2/DomainLabelV2';
import Loader from '../../common/Loader/Loader';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Quality: Loader import breaks grouping order of imports

The Loader import on line 68 is placed between the ../../Customization/ and ../../DataAssets/ import groups, breaking the established import ordering convention. All ../../common/ imports should be grouped together (lines 60-65) before the ../../Customization/ imports.

This is purely cosmetic and has no functional impact, but consistent import ordering helps with readability and reduces merge conflicts.

Suggested fix:

import Table from '../../common/Table/Table';
import Loader from '../../common/Loader/Loader';
import { GenericProvider } from '../../Customization/GenericProvider/GenericProvider';
import { DomainLabelV2 } from '../../DataAssets/DomainLabelV2/DomainLabelV2';
import { OwnerLabelV2 } from '../../DataAssets/OwnerLabelV2/OwnerLabelV2';

Was this helpful? React with 👍 / 👎

@gitar-bot
Copy link

gitar-bot bot commented Feb 26, 2026

🔍 CI failure analysis for c721209: Playwright shard 4/6 failed again on retry with 3 different test failures in Entity, Domains, and Pipeline tests - all unrelated to this PR's ClassificationDetails changes.

Issue

Playwright CI shard 4/6 failed on retry (job 64997267789) with 3 test failures, all in components unrelated to the PR changes.

Root Cause

All failing tests are in Entity/Domain/Pipeline-related test files, completely separate from the ClassificationDetails component modified in this PR:

Shard 4/6 Retry (job 64997267789):

  1. Test: Entity.spec.ts:2140:7 › Table › Set & update column-level custom property

    • Error: Test timeout (180000ms exceeded) + loader stuck visible (expected 0, received 1)
    • Location: entity.ts:53 - waitForAllLoadersToDisappear
    • Retry: Failed on both initial attempt and retry Create teams page similar to tags #1
    • Type: Infrastructure/timeout issue with loader not disappearing
  2. Test: Domains.spec.ts:1400:3 › Domains › Verify duplicate domain creation

    • Error: Strict mode violation - getByTestId('add-domain') resolved to 2 elements instead of 1
    • Location: Domains.spec.ts:1415
    • Type: DOM element duplication issue
  3. Test: Entity.spec.ts:1379:7 › Pipeline › Glossary Term Add, Update and Remove for child entities

    • Error: Element not found - add-tag button not visible
    • Location: entity.ts:1079
    • Type: Missing UI element

Details

Why these failures are unrelated to this PR:

This PR only modifies:

  • ClassificationDetails.tsx - conditional rendering for GenericProvider
  • ClassificationDetails.test.tsx - unit tests for the component
  • ClassificationConditionalRendering.spec.ts - E2E tests for classification page

The failing tests are for:

  • Table entity custom property management
  • Domains page duplicate domain creation validation
  • Pipeline entity glossary term management

These are entirely different UI flows, components, and data models. The shard has failed consistently across multiple retries with different flaky tests, indicating infrastructure instability rather than PR-related issues.

Previous failures in this shard:

  • Initial run (job 64981423405): Same custom property timeout
  • Current retry (job 64997267789): Same custom property timeout + 2 new failures
Code Review 👍 Approved with suggestions 0 resolved / 1 findings

Clean fix that properly guards GenericProvider with conditional rendering, eliminating unsafe type assertion and empty object fallback. Good test coverage for the new loading states. Only a minor import ordering nit.

💡 Quality: Loader import breaks grouping order of imports

📄 openmetadata-ui/src/main/resources/ui/src/components/Classifications/ClassificationDetails/ClassificationDetails.tsx:68

The Loader import on line 68 is placed between the ../../Customization/ and ../../DataAssets/ import groups, breaking the established import ordering convention. All ../../common/ imports should be grouped together (lines 60-65) before the ../../Customization/ imports.

This is purely cosmetic and has no functional impact, but consistent import ordering helps with readability and reduces merge conflicts.

Suggested fix
import Table from '../../common/Table/Table';
import Loader from '../../common/Loader/Loader';
import { GenericProvider } from '../../Customization/GenericProvider/GenericProvider';
import { DomainLabelV2 } from '../../DataAssets/DomainLabelV2/DomainLabelV2';
import { OwnerLabelV2 } from '../../DataAssets/OwnerLabelV2/OwnerLabelV2';

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@github-actions
Copy link
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.75% (56554/86018) 45.25% (29643/65512) 47.96% (8925/18608)

@sonarqubecloud
Copy link

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 To release Will cherry-pick this PR into the release branch UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant