refactor(ui): migrate the remaining fixed-feature tables to TableV2 - #32467
Merged
Conversation
…size prop
The AntD tables being migrated onto TableV2 are dense — entity pages show 20+
rows at a time, and the existing `sm`/`md` heights push a third of them below
the fold. Adds a `compact` size (h-10 rows, h-8 header, px-4/py-2 cells) for
them to map onto.
`selectionMode="single"` rendered a checkbox, which reads as "pick several".
It now renders RadioButtonBase through React Aria's selection slot, so the
behaviour stays React Aria's and only the visual changes.
Also fixes the `size` prop, which never had any effect: TableContext defaulted
to a filled-in `{ size: 'md' }`, so `context?.size ?? size` always resolved to
the context and discarded the prop. Every table has been rendering at `md`
regardless of what it asked for. The context now defaults to null, so an
explicit prop wins and an enclosing TableCard is still inherited.
Adds table.test.tsx covering all three sizes, the size default, and single vs
multiple selection.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TableV2 was documented as a drop-in replacement for the legacy Table but
silently dropped a number of props. This adds a parity suite that drives both
wrappers through the same specs, and fixes everything it found red.
The suite is the interesting part: every spec runs against legacy Table first,
so a spec that cannot go green there is a wrong spec rather than a legacy bug.
Three specs were corrected that way during the run. DOM differences that are
genuine design choices (which element the pager is, how a control is activated)
live in a per-wrapper adapter; the assertions stay shared.
Fixed:
- onRow onClick never fired. React Aria strips a row's click handler unless the
row is interactive; an empty onAction marks it interactive so the call site's
handler receives a real MouseEvent, with no second activation path.
- The column filter dropdown could not open at all — DialogTrigger reaches its
child through a PressResponder, and the child was a core Button rather than a
React Aria pressable.
- className was accepted by the props type and dropped at render.
- Controlled sortOrder was ignored; a column declaring it now drives the sort.
- rowSelection.getCheckboxProps was ignored, so rows meant to be unselectable
were selectable. Mapped to disabledKeys + disabledBehavior="selection".
- sortDirections, indentSize, footer and expandedRowRender were unimplemented.
- The page-size changer never appeared: showSizeChanger, pageSizeOptions and
onShowSizeChange now reach the internal pager, and changing size resets to
page one.
Two props stay unsupported and are now omitted from TableV2Props, so passing
them fails to compile instead of rendering a table that quietly lost a feature:
summary (React Aria discards any table child that is not a Header or Body, and
a summary drawn outside the table would not line up with the columns) and
components (no equivalent — use dragAndDropHooks or a column render).
customPaginationProps now requires pagination={false} in the type, since it
means the parent already fetched exactly this page; slicing again would drop
rows. A runtime short-circuit backstops untyped call sites.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tests that select on `.ant-table-row` break the moment a page moves to TableV2, which would put an unrelated red suite in the middle of every sweep PR. AntD's rc-table emits `data-row-key` natively and TableV2 emits it too, so a selector written against it passes on both wrappers and survives the migration. Rewrites the three suites that reached for AntD class names, and adds `filter-trigger` / `filter-dropdown` testids to TableV2 so the filter UI has a hook that does not depend on AntD's DOM. The stable hooks are listed in the TableV2 header comment. The parity suite keeps its `.ant-table` selectors: they live in the legacy adapter, which exists precisely to drive the AntD DOM. Adds docs/antd-migration/table.md — the review contract for the call-site sweeps: what is supported (with a spec behind each claim), what is blocked and why, the selector table, and the per-page checklist. Playwright specs are deliberately not touched. They need a running stack to verify, and rewriting e2e selectors blind is how a suite goes quietly broken — each one moves in the sweep PR for its page, where it can actually be run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rule reversed the filter icon and title by targeting .ant-table-filter-column, which TableV2 does not render — so the icon would silently move back to the right on every filtered table the sweep touches (5 call sites across both repos). TableV2's column header now carries data-testid=column-header-content, and the rule matches both, so the class keeps working either side of a migration.
AntD tolerates duplicate column keys and renders both. React Aria uses the id as a collection key, so the duplicate collapsed the column while the row still rendered a cell for it — 'Cell count must match column count. Found 5 cells and 4 columns'. VersionTable has two columns keyed 'tags', and it is unlikely to be the only one. Column ids are now derived once and de-duplicated by suffixing repeats, so the header and the body agree. Parity spec added: legacy already rendered both columns, and TableV2 now matches.
propsColumns was state initialised to [] and filled in an effect, so the first render had zero columns. React Aria registers the column collection on that render, and the body then rendered cells against an empty header — 'Cell count must match column count. Found 3 cells and 0 columns'. AntD tolerated the transient empty state; React Aria does not. The columns are now derived with useMemo, so header and body always agree. Drag reorder keeps its own state (the reordered key list) rather than writing the resolved columns back.
React Aria keys rows and columns in one namespace. ContractSchemaTable uses rowKey='name' and has a row named 'name', which collided with the 'name' column's id — React Aria dropped the column and the row then rendered three cells against zero columns. Schema tables render an entity's columns as rows, so values like 'name' or 'description' are routine; this is not a one-off. Column ids are now prefixed, and columnKeys carries the original key for anything reported back to the call site, so onChange still receives the caller's own key. Parity specs cover both collision shapes: two columns sharing a key, and a row key matching a column key. Legacy passes both.
ListViewTab keys executions by name-status-key and its rows repeat, so several share a key. AntD renders them all (with a console warning); React Aria treats the id as the collection key and kept only the first — seven rows became one. Row ids are now de-duplicated by suffixing repeats, with a map back to the call site's own key so selection and onRowAction still report what the caller keyed by. data-row-key keeps the original value, so selectors are unaffected. Parity spec added; legacy already rendered all three rows.
…port Three correctness bugs, all found in review: - A fabricated dedup suffix could equal a real key: for ['dup','dup','dup-1'] the counter produced ['dup','dup-1','dup-1'], so the synthesized id collided with the genuine third row and React Aria dropped it — reintroducing the silent row drop this was written to prevent. Ids are now checked against every id already emitted, not just against repeats of the same base. - getCheckboxProps was evaluated over filteredDataSource while rows were keyed by their position within the page. With no rowKey, keys fall back to the array index, so the disabled set used whole-dataset positions and disabled the wrong rows on every page after the first. - Selection filtered the data by key, so selecting one of two rows sharing a key handed the call site both records. All three came from row identity being derived three separate ways. There is now one `rowEntries` list — React Aria id, the call site's key, the record — and disabled rows and selection both read from it. Also removes the parity suite's own `antd/lib/table` import: the type re-exports that would replace it belong to a different PR, and tw-guard is right that this one should not add a new antd dependency. The fixtures are typed structurally instead. Specs added for the first two collisions. The page-offset one lives in the TableV2-only file: legacy AntD has the same weakness, so asserting it as parity would be asking legacy to be better than it is.
The selection slot only accepts a checkbox, so a single-selection row looks like a radio but announces as one. Swapping to a radiogroup would mean re-implementing selection, focus and keyboard handling outside the table's own state, so the limitation is recorded where the visual is chosen.
…ing it The previous comment claimed `aria-label` named the row. It did not — there was no aria-label, so the comment described behaviour that did not exist. The control now carries a real label. The semantic mismatch itself stays: ARIA does not permit role="radio" on an input[type=checkbox], and React Aria's selection slot only accepts a checkbox, so the alternatives are a checkbox that announces wrongly or re-implementing selection, focus and keyboard handling outside the table's own state. The label carries the "only one row" affordance that the role cannot, so the announcement is no longer misleading about what the control does. Test asserts the label so it cannot silently disappear.
`check-i18n-keys` failed: t('label.select-only-one-row') had no entry in
en-us.json. Added there and translated into all 19 other languages — the
`check-translations-not-english` gate rejects an English string copied into a
non-English file, and it is right to.
Locally `yarn i18n` alone reports clean, which is why this was not caught
before pushing: it syncs keys between locale files but does not extract keys
from source. `yarn check-i18n-all` is the one that does.
…t migrate ContractSchemaTable.test.tsx was swept into this branch during the PR consolidation. It asserts the shared NextPrevious testids, but the component still imports the legacy wrapper here, so AntD renders its own numbered pager and [data-testid="pagination"] does not exist — one failing test across the whole 14,610-test run. Reverted to main's version. The assertion belongs with the PR that migrates the component, alongside the import change. Same mistake as the ChildTermsTab selector caught in review on the Collate side: a test moved to the TableV2 contract ahead of its call site. Worth checking for by diffing test changes against whether the matching component moved in the same commit.
Every migrated table got `stickyHeader`, so its header carried `position: sticky; z-index: 10` and painted over anything the page drew on top of the table afterwards — the notification-templates list header sat above the Edit Template drawer. AntD sticks a header only when asked, via `sticky` or via `scroll.y` giving the body its own scroll container. TableV2 now matches: `sticky` was in the prop census and simply never got wired. Parity spec asserts the header is not stuck by default, which legacy satisfies too. The opt-in cases stay in the TableV2-only file — legacy fixes its header by rendering a separate header table, so there is no shared class to assert.
An empty table rendered the bare string "no data" in a div. AntD fell back to its own <Empty> illustration, so a table with no rows read as an empty state rather than a stray label. TableV2 now falls back to the core EmptyPlaceholder — icon, title, same shape the rest of the app uses (see IngestionUtils). Call sites that pass `locale.emptyText` still win, which is how most of them hand in their own ErrorPlaceHolder or FilterTablePlaceHolder. Parity spec asserts the placeholder renders an illustration rather than text alone; legacy satisfies it through AntD's Empty.
Every cell wrapped its content in a flex box with `max-w-full`. That wrapper exists only to place the tree expander beside the value, but it applied to all cells, so anything a cell rendered inline was sized by it — the Glossary Terms dropdown on a pipeline's task list came out a few pixels wide. AntD puts cell content straight into the <td>. The wrapper is now `display: contents` unless the cell actually carries an expander, so it stops laying anything out while keeping the expander case unchanged. Specs cover both shapes. They are TableV2-only: AntD has no cell wrapper, so there is nothing to assert parity against.
Raised in review: with no expander the cell wrapper is `display: contents`, so `flex-1 min-w-0` on the value div have no flex parent and do nothing. Truncation was never at risk — `truncate` is what clips, and without the wrapper's box the div is a block child of the cell and already fills it — but shipping classes that do nothing invites someone to assume they matter. They are now applied only in the expander case, where the wrapper really is a flex row. Specs cover both paths; neither was tested before.
A parent that fetches one page at a time hands TableV2 only that page and reports the real size through pagination.total, then refetches from the table-level onChange callback. TableV2 read neither: it re-sliced the rows it had already been given (so every page after the first came out empty) and never called onChange, so the parent had no way to learn the page had moved. Treat a total larger than the rows in hand as server-driven: render the rows as-is, size the pager from total, honour a controlled current, and report page changes through onChange with action 'paginate' — matching what AntD does. Four parity specs cover it, green against legacy Table first. Also fixes three compile errors already on this branch and touching the same surface: a rowKeyById reference left behind by the rowEntryById rename, two implicitly-any sorter params in the parity suite, and the DragAndDropHooks mismatch caused by the core package bundling react-aria-components 1.16 while the app resolves 1.17. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…more pages hideOnSinglePage compared the rows in hand against pageSize. Under server-driven pagination those rows are one page by definition, so a full first page (5 rows, pageSize 5, total 40) read as 'single page' and the pager vanished — pages 2-8 became unreachable. Compare against the reported total when there is one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit c01de42)
…ct-radio # Conflicts: # openmetadata-ui/src/main/resources/ui/src/components/common/Table/TableV2.tsx
… sites opt out The legacy wrapper hardcoded tableLayout="fixed" after its prop spread, so every table it served solved column widths that way and no call site could ask for anything else. TableV2 left the layout at the browser default, so migrated tables silently switched to content-sized columns. Default to fixed to match, and honour an explicit tableLayout: the handful of tables that were on raw AntD were laid out by content and need to stay that way. Resizable columns stay fixed regardless — an auto table re-solves its own widths and swallows the drag. Also dedupes react-aria-components for jest. ui-core-components keeps its own copy under a link: install; vite already dedupes for the build, but jest did not, so tests loaded two copies and context lookups across the boundary missed — rendering a resizable table threw "Wrap your <Table> in a <ResizableTableContainer>" with the container right there in the tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 3d479f3)
Two gaps found by auditing TableV2 against the AntD prop surface the legacy wrapper passed straight through. onRow's return was filtered through an allowlist of click and drag handlers, so a call site's test ids, aria attributes, styles and other mouse handlers were dropped without a word. Forward the whole object. The drag half of that allowlist was justified by a comment claiming native HTML5 handlers would fight React Aria's — they cannot, because React Aria's Row never attaches them either way. That limitation is now pinned by a test instead of described wrongly in a comment: rows are dragged through dragAndDropHooks. bordered was ignored outright, so the seven tables that ask for AntD's grid lost their cell borders on migration. Adds parity coverage for the rest of the surface an audit turned up untested: rowClassName, onRow's arguments, data-row-key, bordered, the column reorder handle, and column resizing — the last of which had never run, because until the react-aria dedupe it threw before reaching an assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit a3ce41e)
Reading the handler off the column twice needed a non-null assertion to get past the second read. One binding, no assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nto feat/core-table-compact-radio
A comma selector list only unions pure CSS selectors — mixing role= engines across a comma is a parse error at runtime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…row by role The core EmptyPlaceholder is absolute/inset-0 and fills its nearest positioned ancestor — TableV2's empty-state slot had no position and no height, so the placeholder escaped the table box. The slot is now relative with a real min-height. React Aria renders the empty-state cell as the row's rowheader, so the cell/gridcell union never matched; the row's accessible name is the placeholder text under both engines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The page opted into bordered long before the migration; the full grid reads as noise next to the other member pages, which are borderless. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
karanh37
previously approved these changes
Sep 7, 2026
Contributor
🚦 Removed from the merge queue —
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
The remaining-tables TableV2 sweep drops AntD's `.ant-*` table classes and the
`cell` role, so specs asserting against them broke. Route cell lookups through
getCellByName (cell/rowheader/gridcell), and swap AntD internals for the DOM
TableV2 keeps:
- getByRole('cell') -> getCellByName (bot, OnlineUsers, BulkEditEntity)
- .ant-table-cell -> td; .ant-table-thead > tr > .ant-table-cell -> thead > tr > th
- tbody tr.ant-table-row -> tbody tr; .ant-table -> [role="grid"]
- Data Contract schema selection: .ant-checkbox-input -> input[type="checkbox"],
.ant-checkbox-checked -> row [aria-selected="true"], and the select-all CSS
attribute selector (case-sensitive, missed react-aria's "Select All") ->
getByRole('checkbox', { name: 'Select all' })
getCellByName also gained a RegExp name overload. Verified with ui-checkstyle
(eslint + prettier) on the changed files.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n Data Contract schema The Data Contract schema form is a TableV2 grid with row selection, so its checkboxes are React Aria: a visually-hidden <input> under a visual <div> that intercepts pointer events, wrapped in a pressable <label slot="selection">. Clicking the <input> (or getByRole checkbox .check/.uncheck) hangs on that interception. Target the label instead: per-row via label[slot="selection"], select-all via the checkbox's ancestor label. Assertions (toBeChecked / toBeDisabled) stay on the input, whose state is readable while sr-only. The rows are also React Aria pressable, so a pointer click on the inner expand icon triggers the row press + selection plus the expansion's layout shift and never settles — fire the icon's own click via dispatchEvent instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ager
Two more TableV2 migration mismatches surfaced on CI:
- OnlineUsers header asserts used getByRole('columnheader', { name }), but
TableV2 wraps header text in <div role="group">, so Chrome's
name-from-content no longer bubbles the text to the columnheader and the
by-name query misses. Match by text content instead:
getByRole('columnheader').filter({ hasText }).
- DataContracts pagination used the old AntD pager (getByTitle('10') page chip
and getByRole('listitem', { name: 'Next Page' })). TableV2 renders the
NextPrevious pager with no numbered chips: assert the page count via
data-testid="page-indicator" and page via data-testid="next".
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The paginated schema selection table is part of the contract edit form; the
saved read view does not render it, so getByTestId('pagination') after save
found nothing. Assert the pager before clicking save, where it exists.
Selection persistence stays covered by the reopen + select-all assertions.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Restore the persistence check the review flagged: after re-selecting page-1 columns and saving, reopen the edit form and assert rows 1-5 stay selected (aria-selected), rather than only asserting the pager exists. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…Contracts:747 Step 2 reopened the contract edit form and immediately toggled the header select-all to deselect page 1. The saved-selection restore is async and not gated by the column loader, so the click raced it and the selection netted back to checked (trace shows all page-1 rows re-selected). Wait for select-all to reflect the restored full selection first -- which also proves persistence -- then toggle once from that stable state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
TableV2's onSelectionChange only ever reported the current page's rows, and consumers replace their selection with whatever it returns. With server-side pagination the previously selected keys on other pages were silently dropped, so a multi-page selection collapsed to the last page acted on -- e.g. building a data contract across paginated schema columns saved an empty/partial schema. Honor AntD's preserveSelectedRowKeys: merge the current page's selection with the previously selected keys that are not on this page before calling onChange. Verified against DataContracts:747 (contract schema now persists all selected columns across pages and restores on reopen). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
karanh37
previously approved these changes
Sep 8, 2026
Contributor
🚦 Removed from the merge queue —
|
# Conflicts: # openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/DataQuality/TestLibrary.spec.ts # openmetadata-ui/src/main/resources/ui/src/components/common/Table/TableV2.tsx
Contributor
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
…adow - effectiveFilterOf: while a controlled column's dropdown is open, prefer the in-progress draft over the parent filteredValue so checkboxes reflect each click (confirm already applied the draft; this is the missing visual echo). - Add an unmount-only cleanup that removes the ping scroll listener; the attach effect intentionally runs every render to catch a re-mounted scroller, so it can't own removal without stripping the listener between renders. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review ✅ Approved 4 resolved / 4 findingsMigrates 43 call sites from AntD tables to TableV2, including fixes for comma-union role selectors in Playwright tests, re-select validation, controlled filter dropdown selection, and scroll listener cleanup. Surfaces and fixes a real engine bug where rows with empty-string ✅ 4 resolved✅ Bug: Comma-union of role= selectors is invalid Playwright syntax
✅ Quality: Re-select step no longer validates persistence, only pager
✅ Bug: Controlled filter dropdown ignores in-progress selection
✅ Quality: Scroll listener effect lacks cleanup and runs every render
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 |
|
karanh37
approved these changes
Sep 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Stacked on #31953 (merge after it). Third tranche of the AntD→TableV2 sweep: the 43 call sites whose tables use nothing beyond the engine's supported surface — plain import swaps, local aliases kept.
Test fallout was mechanical: 12 stale legacy-wrapper jest mocks repointed, 5 wholesale
ui-core-componentsmocks given therequireActualspread,role="table"→grid, one empty-state assertion moved to the raw i18n key, and AntD pagination-title assertions replaced with the core pager's labelled buttons.One real engine bug surfaced: a row whose
rowKeyfield is an empty string was silently dropped (React Aria rejects''as an id) — a CSV import's failure rows, keyed on their blank name column, vanished from the result table; AntD rendered them. Empty string now falls back to the row index like undefined always did, pinned by the import-result suites.All 311 tests across the 60 touched suites pass.
Type of change
Checklist
🤖 Generated with Claude Code