Skip to content

refactor(DimensionalityTab): migrate table to core-ui Table component#27969

Open
shah-harshit wants to merge 9 commits intomainfrom
feat/migrate-dimensionality-tab-core-ui-table
Open

refactor(DimensionalityTab): migrate table to core-ui Table component#27969
shah-harshit wants to merge 9 commits intomainfrom
feat/migrate-dimensionality-tab-core-ui-table

Conversation

@shah-harshit
Copy link
Copy Markdown
Contributor

@shah-harshit shah-harshit commented May 7, 2026

Summary

  • Replace Ant Design Table with core-ui Table (react-aria-components) in DimensionalityTab
  • Flatten cell renderer and memoize columns for performance
  • Minor UI fix for dimension count div min-width

Test plan

  • Verify DimensionalityTab table renders correctly with all columns visible
  • Run unit tests for affected components

🤖 Generated with Claude Code


Summary by Gitar

  • Refactored DimensionalityTab:
    • Migrated DimensionalityTab table to core-ui Table component, replacing the Ant Design implementation.
    • Optimized rendering by flattening the cell renderer and memoizing table columns.
  • UI enhancements:
    • Added tw:h-8 and fontSize="xs" to dimension-select for consistent styling.
    • Updated DataQualityTab to set tw:min-w-13 on the dimension count container.
    • Added border styling to DimensionalityTab by wrapping the table in a container with tw:rounded-xl and tw:ring-1.
  • Code cleanup:
    • Replaced Trans with TransComponent wrapper for type safety in DimensionalityTab.

This will update automatically on new commits.

Replace Ant Design Table with react-aria-based Table from
@openmetadata/ui-core-components. Removes antd ColumnsType,
HelpCircle icon, and Tooltip imports in favour of Table.Head
tooltip prop. Impact-score tooltip now rendered natively by
Table.Head. Trans type error fixed via ComponentType cast.
Select dimension dropdown styled to match date picker height
(h-8/32px) and font-size xs.

Refs: open-metadata/openmetadata-collate#3837
@shah-harshit shah-harshit requested a review from a team as a code owner May 7, 2026 14:22
@shah-harshit shah-harshit self-assigned this May 7, 2026
@shah-harshit shah-harshit added UI UI specific issues safe to test Add this label to run secure Github workflows on PRs labels May 7, 2026
@shah-harshit shah-harshit changed the title refactor(DimensionalityTab): migrate table to core-ui Table component Migrate dimenstionality table to core-ui Table component May 7, 2026
Extract nested ternary cell renderer into a switch-based renderCell
function. Wrap dimensionTableColumns in useMemo([t]) to stabilize
the array reference across renders.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 62%
62.46% (63114/101033) 42.87% (34123/79595) 45.84% (10067/21959)

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

🔴 Playwright Results — 2 failure(s), 10 flaky

✅ 4016 passed · ❌ 2 failed · 🟡 10 flaky · ⏭️ 86 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 298 0 1 4
🟡 Shard 2 752 0 3 8
🔴 Shard 3 756 2 1 7
🟡 Shard 4 788 0 2 18
🟡 Shard 5 686 0 1 41
🟡 Shard 6 736 0 2 8

Genuine Failures (failed on all attempts)

Features/Tasks/TaskNavigation.spec.ts › clicking task notification while on entity task tab refreshes the task list (shard 3)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed

Locator: locator('.notification-box').locator('li.ant-list-item.notification-dropdown-list-btn').first()
Expected: visible
Timeout: 15000ms
Error: element(s) not found

Call log:
�[2m  - Expect "toBeVisible" with timeout 15000ms�[22m
�[2m  - waiting for locator('.notification-box').locator('li.ant-list-item.notification-dropdown-list-btn').first()�[22m

Features/Tasks/TaskNavigation.spec.ts › two sessions: admin on Columns tab creates task, assignee sees refresh on notification click (shard 3)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed

Locator: locator('.notification-box').locator('li.ant-list-item.notification-dropdown-list-btn').first()
Expected: visible
Timeout: 15000ms
Error: element(s) not found

Call log:
�[2m  - Expect "toBeVisible" with timeout 15000ms�[22m
�[2m  - waiting for locator('.notification-box').locator('li.ant-list-item.notification-dropdown-list-btn').first()�[22m

🟡 10 flaky test(s) (passed on retry)
  • Pages/AuditLogs.spec.ts › should apply both User and EntityType filters simultaneously (shard 1, 1 retry)
  • Features/ActivityAPI.spec.ts › Activity event shows the actor who made the change (shard 2, 1 retry)
  • Features/BulkEditEntity.spec.ts › Glossary (shard 2, 1 retry)
  • Features/Glossary/GlossaryWorkflow.spec.ts › should start term as Draft when glossary has reviewers (shard 2, 1 retry)
  • Flow/PersonaFlow.spec.ts › Set default persona for team should work properly (shard 3, 1 retry)
  • Pages/CustomProperties.spec.ts › Set & Update all CP types on apiCollection (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Delete Api Endpoint (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Delete Topic (shard 5, 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 NOT visible for dashboardService in platform lineage (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

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented May 8, 2026

Code Review ✅ Approved 4 resolved / 4 findings

Migrates DimensionalityTab to the core-ui Table component and optimizes rendering through column memoization and flattened cell structures. These changes address previous findings regarding nested ternary chains, redundant re-renders, and missing row keys.

✅ 4 resolved
Quality: Deeply nested ternary chain in cell renderer hurts readability

📄 openmetadata-ui/src/main/resources/ui/src/components/DataQuality/IncidentManager/DimensionalityTab/DimensionalityTab.tsx:285-299
The cell rendering logic (lines 286–323) uses a 4-level nested ternary expression to switch on col.id. This is hard to read and maintain. Consider extracting a helper function or using a switch statement / lookup map pattern, which is more consistent with how other core-ui Table consumers typically render cells.

Performance: dimensionTableColumns recreated on every render

📄 openmetadata-ui/src/main/resources/ui/src/components/DataQuality/IncidentManager/DimensionalityTab/DimensionalityTab.tsx:181-190
dimensionTableColumns is declared as a plain array inside the component body (lines 181–190). Since it depends on t() (which is stable across renders unless the language changes), wrapping it in useMemo with [t] as a dependency would avoid creating a new array reference on every render, preventing unnecessary reconciliation of Table.Header.

Bug: Table.Row key is undefined when record.id is missing

📄 openmetadata-ui/src/main/resources/ui/src/components/IncidentManager/IncidentManager.component.tsx:628
At line 628, key={record.id} passes undefined as the React key when record.id is absent (it's an optional field in TestCaseResolutionStatus). If multiple records lack an id, React cannot distinguish them during reconciliation, leading to duplicate-key warnings and potentially corrupted component state. The id prop also falls back to '', giving multiple rows the same DOM id.

Performance: renderRow is not memoized, recreated every render

📄 openmetadata-ui/src/main/resources/ui/src/components/IncidentManager/IncidentManager.component.tsx:615-629
renderRow is a plain function that closes over multiple reactive values (testCasePermissions, isPermissionLoading, handleStatusSubmit, etc.). Because it's recreated on every render, the Table.Body's dependencies array alone controls re-rendering — but having a new function reference every time defeats React's ability to bail out of rendering children. Wrapping it in useCallback with the same deps used in dependencies would be more consistent.

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 8, 2026

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 UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants