feat(ui): close the TableV2 parity gaps, add a shared parity suite - #31954
feat(ui): close the TableV2 parity gaps, add a shared parity suite#31954harsh-vador wants to merge 5 commits into
Conversation
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>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
✅ Playwright Results — workflow succeededValidated commit ✅ 552 passed · ❌ 0 failed · 🟡 1 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 51m 25s ⏱️ Max setup 4m 16s · max shard execution 16m 47s · max shard-job elapsed before upload 20m 27s · reporting 4s 🌐 215.54 requests/attempt · 2.83 app boots/UI scenario · 20.06% common-shard skew Optimization targets still in progress:
🟡 1 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| const buildExpandedDetailRow = <T extends object>( | ||
| expandable: TableComponentProps<T>['expandable'], | ||
| flatRow: FlatRow<T>, | ||
| isExpanded: boolean, | ||
| columnCount: number | ||
| ) => { | ||
| const renderDetail = expandable?.expandedRowRender; | ||
|
|
||
| if (!renderDetail || !isExpanded || !flatRow.hasChildren) { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <UntitledTable.Row | ||
| id={`${flatRow.rowKey}-expanded`} |
There was a problem hiding this comment.
💡 Edge Case: expandedRowRender detail row colSpan ignores selection/expander cols
buildExpandedDetailRow sets the detail cell's colSpan to propsColumns.length only (TableV2.tsx:1086-1091, 229-233). When rowSelection is active (react-aria injects a selection column) or an expander column is present, the detail panel spans one column too few and will not stretch across the full table width — and a react-aria Row that omits the required selection cell may not align correctly. Compute the colSpan from the actual rendered column count (including selection/expander columns) rather than just propsColumns.length.
Was this helpful? React with 👍 / 👎
| const resolveClientPagination = <T,>( | ||
| pagination: TableComponentProps<T>['pagination'], | ||
| pageSizeOverride: number | null, | ||
| hasParentPagination: boolean | ||
| ) => { | ||
| if (pagination === false || hasParentPagination) { | ||
| return null; | ||
| } | ||
| const cfg = (pagination ?? {}) as TablePaginationConfig; | ||
|
|
||
| return { | ||
| pageSize: pageSizeOverride ?? (cfg.pageSize as number) ?? DEFAULT_PAGE_SIZE, | ||
| hideOnSinglePage: cfg.hideOnSinglePage ?? false, | ||
| showSizeChanger: cfg.showSizeChanger ?? false, | ||
| pageSizeOptions: (cfg.pageSizeOptions ?? []).map(Number), |
There was a problem hiding this comment.
💡 Bug: User pageSize override shadows a parent-controlled pagination.pageSize
Once the user picks a size, pageSizeOverride is set and resolveClientPagination returns pageSizeOverride ?? cfg.pageSize (TableV2.tsx:165,322,377). If the parent later re-renders with a different pagination.pageSize, that controlled value is permanently ignored because the override always wins and is never reset when the incoming pagination prop changes. Reset pageSizeOverride to null when rest.pagination (or its pageSize) changes, or prefer the prop value when it differs from the last override.
Was this helpful? React with 👍 / 👎
| /** | ||
| * React Aria always opens a fresh sort on 'ascending'. AntD lets a column say | ||
| * which way the first click should go via `sortDirections`, so honour the head | ||
| * of that list when the sort moves to a different column. | ||
| */ | ||
| const resolveSortDirection = <T,>( | ||
| column: ColumnType<T> | undefined, | ||
| isFirstClickOnColumn: boolean, | ||
| fallback: 'ascending' | 'descending' | null | ||
| ) => { | ||
| const preferred = column?.sortDirections?.[0]; | ||
|
|
||
| if (!isFirstClickOnColumn || !preferred) { | ||
| return fallback; | ||
| } |
There was a problem hiding this comment.
💡 Quality: sortDirections only honored when switching columns, not on re-click
resolveSortDirection applies the column's preferred first direction only when isFirstClickOnColumn is true, i.e. when moving to a different column (TableV2.tsx:195-207,653-657). AntD's sortDirections also governs the cycle on the same column (e.g. ['descend','ascend'] cycles descend→ascend→none); on repeat clicks TableV2 falls back to react-aria's ascending/descending toggle, so same-column cycling can diverge from AntD. This is a partial-parity gap worth documenting or handling if full parity is expected.
Was this helpful? React with 👍 / 👎
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| filteredDataSource | ||
| .map((record, index) => ({ key: getRowKey(record, index), record })) | ||
| .filter(({ record }) => getCheckboxProps(record).disabled) | ||
| .map(({ key }) => key) |
There was a problem hiding this comment.
When an expanded tree uses getCheckboxProps to disable nested records, disabledRowKeys evaluates only the top-level filteredDataSource, so child keys are omitted from disabledKeys. Those child rows remain selectable, allowing nested schema columns intended to be disabled to be added to a data contract.
harsh-vador
added
the
safe to test
Jest test CoverageUI tests summary
|
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.
84343ea to
6405dbd
Compare
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source
❌ UI Checkstyle Failed❌ ESLint + Prettier + Organise Imports (src)One or more source files have linting or formatting issues. Affected files
❌ Antd + Less Deprecation GuardA new Affected filesopenmetadata-ui/src/main/resources/ui/src/components/common/Table/tests/tableParity.shared.tsx: import { ColumnsType } from 'antd/lib/table' 🔍 ESLint findings in this PR's files — 0 error(s), 25 warning(s)Errors block the build. Warnings do not yet — they are rules whose backlog is still 0 error(s), 25 warning(s) across 2 changed file(s).
All findings
Fix locally (fast - only checks files changed in this branch): make ui-checkstyle-changed |
| const handleSortChange = useCallback( | ||
| (descriptor: AriaSortDescriptor) => { | ||
| const newKey = descriptor.column ? String(descriptor.column) : null; | ||
| const newDirection = descriptor.direction ?? null; | ||
| const clickedColumn = propsColumns.find((c, idx) => { | ||
| const key = String(c.key ?? (c as ColumnType<T>).dataIndex ?? idx); | ||
|
|
||
| return key === newKey; | ||
| }) as ColumnType<T> | undefined; | ||
|
|
||
| const newDirection = resolveSortDirection( | ||
| clickedColumn, | ||
| sortState.columnKey !== newKey, | ||
| descriptor.direction ?? null | ||
| ); | ||
| setSortState({ columnKey: newKey, direction: newDirection }); |
There was a problem hiding this comment.
💡 Bug: Sort lookup uses raw column key, not deduplicated columnIds
This commit makes each React Aria column id unique via getColumnIds (e.g. the second column sharing key name becomes name-1), but handleSortChange and the sort/filter matching still resolve columns with the raw String(c.key ?? c.dataIndex ?? idx). When a duplicate-keyed column is sortable, React Aria fires onSortChange with descriptor.column = 'name-1', which matches no column, so clickedColumn/matchedCol are undefined and the click sorts nothing (and onChange gets column: undefined). Derive the sort key the same way — build a columnId -> column map from columnIds (or compare against columnIds[idx]) in both handleSortChange and the filteredDataSource sort predicate so header ids and sort lookups agree.
Was this helpful? React with 👍 / 👎
| ); | ||
| }, [rest.rowSelection, filteredDataSource, getRowKey]); | ||
|
|
||
| const handleSelectionChange = useCallback( |
There was a problem hiding this comment.
⚠️ Bug: Select-all reports getCheckboxProps-disabled rows as selected
When the header select-all control is used, React Aria fires onSelectionChange with the sentinel 'all'. handleSelectionChange then maps every record in filteredDataSource to selected keys/rows without excluding disabledRowKeys, so rows the caller marked disabled via getCheckboxProps are reported as selected in onChange. This breaks parity with AntD (which excludes disabled rows from select-all) and the very contract getCheckboxProps is meant to enforce. The shared select-all spec doesn't catch it because that case uses no disabled rows. Filter out disabled keys in the 'all' branch.
Exclude disabled rows when select-all is used.:
const selectedKeys =
keys === 'all'
? dataSource
.map((r, i) => getRowKey(r, i))
.filter((k) => !disabledRowKeys?.has(k))
: [...keys].map(String);
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| const disabledRowKeys = useMemo((): Set<string> | undefined => { | ||
| const getCheckboxProps = rest.rowSelection?.getCheckboxProps; | ||
| if (!getCheckboxProps) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return new Set( | ||
| filteredDataSource | ||
| .map((record, index) => ({ key: getRowKey(record, index), record })) | ||
| .filter(({ record }) => getCheckboxProps(record).disabled) | ||
| .map(({ key }) => key) | ||
| ); | ||
| }, [rest.rowSelection, filteredDataSource, getRowKey]); |
There was a problem hiding this comment.
⚠️ Bug: disabledRowKeys omits nested/child rows, leaving them selectable
disabledRowKeys is derived only from the top-level filteredDataSource, but expanded tree rows are produced by flattenTreeRows which recurses into record.children. Child records that getCheckboxProps marks disabled never get their key into disabledRowKeys, so nested rows meant to be unselectable remain selectable — the opposite of the intended behavior. (The same top-level-only assumption in handleSelectionChange's selectedRows filter also drops selected child records.) Derive disabled keys by walking children recursively, mirroring flattenTreeRows.
Recurse into children so nested disabled rows are included.:
const collectDisabled = (rows: T[], acc: Set<string>, base = 0): number => {
let i = base;
for (const record of rows) {
const idx = i++;
if (getCheckboxProps(record).disabled) {
acc.add(getRowKey(record, idx));
}
const children = (record as Record<string, unknown>).children as T[] | undefined;
if (children?.length) {
i += collectDisabled(children, acc, i);
}
}
return i - base;
};
const keys = new Set<string>();
collectDisabled(filteredDataSource, keys);
return keys;
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
|
1 participant
TableV2is documented as a drop-in replacement for the legacyTable, but silently dropped a number of props. This adds a suite that drives both wrappers through the same specs, and fixes everything it found red.The suite is the point
Every spec runs against legacy
Tablefirst. A spec that cannot go green there is a wrong spec, not a legacy bug — three specs were corrected that way during the run, and one "bug" turned out not to exist at all (the customize dropdown does re-show a hidden column; a React Aria popover marks the pagearia-hiddenand the flag outlived the menu in jsdom).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
onRowonClickonActionmarks it interactive so the handler gets a realMouseEvent, with no second activation pathDialogTriggerreaches its child through aPressResponder, and the child was a coreButtonrather than a React Aria pressableclassNamesortOrderrowSelection.getCheckboxPropsdisabledKeys+disabledBehavior="selection"sortDirections,indentSize,footer,expandedRowRendershowSizeChanger/pageSizeOptions/onShowSizeChangenow reach the internal pager, and changing size resets to page oneDeliberately not implemented
summaryandcomponentsare removed fromTableV2Props, so passing them fails to compile rather than rendering a table that quietly lost a feature. React Aria's collection builder discards any table child that is not a Header or Body, so atfootnever reaches the DOM — and a summary drawn outside the table would not line up with the columns, which is worse than not drawing it.TeamHierarchyusescomponentsand will fail to compile when it migrates; that is intended.customPaginationPropsnow requirespagination={false}in the type, since it means the parent already fetched exactly this page. A runtime short-circuit backstops untyped call sites.Verification
src/components/common/Table(includes the pre-existingTable.test.tsxand DraggableMenu suites)tsc --noEmit: 573 errors, identical tomain— zero delta (note:mainis not clean; zero is unreachable)eslint: 0 errors🤖 Generated with Claude Code
Greptile Summary
TableV2 gains broader compatibility with the legacy Table API and a shared parity suite that runs equivalent behavior checks against both wrappers.
Confidence Score: 4/5
The PR is not yet safe to merge because expanded child rows marked disabled by
getCheckboxPropscan still be selected.The disabled-key set is built from top-level
filteredDataSourcerecords, while expanded descendants are introduced only inflatRows; consequently, disabled child keys never reach the table’sdisabledKeysselection guard.Files Needing Attention: openmetadata-ui/src/main/resources/ui/src/components/common/Table/TableV2.tsx
Important Files Changed
Reviews (2): Last reviewed commit: "fix(ui): keep TableV2 working when two c..." | Re-trigger Greptile