feat(ui): add TableV2 as Untitled UI migration wrapper for Table component - #26214
feat(ui): add TableV2 as Untitled UI migration wrapper for Table component#26214harsh-vador wants to merge 45 commits into
Conversation
harsh-vador
added
UI
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
|
@gitar-bot all the comments are addressed, can you re-review? |
Re-review is in progress. Results will be posted shortly. |
| return [{ 'text/plain': key }]; | ||
| }, | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| getDropOperation(target: any) { |
There was a problem hiding this comment.
⚠️ Bug: Stale closure in getDropOperation defeats self-drop guard
getDropOperation closes over draggingFqn from the render cycle when useDragAndDrop was called. Although flushSync inside getItems forces a synchronous re-render with the new draggingFqn, React Aria holds onto the old getDropOperation closure from before the flush. As a result, draggingFqn is still null when getDropOperation runs on subsequent dragover events, so the self-drop and child-drop protection (targetFqn === draggingFqn || targetFqn.startsWith(draggingFqn + '.')) never fires — every target returns 'move'.
The fix is to mirror draggingFqn into a ref so getDropOperation always reads the latest value.
Suggested fix:
const draggingFqnRef = useRef<string | null>(null);
// Inside getItems:
flushSync(() => setDraggingFqn(key));
draggingFqnRef.current = key;
// Inside getDropOperation, read from ref:
const currentDraggingFqn = draggingFqnRef.current;
if (
currentDraggingFqn &&
(targetFqn === currentDraggingFqn ||
targetFqn.startsWith(currentDraggingFqn + '.'))
) {
return 'cancel';
}
// Inside onDragEnd:
draggingFqnRef.current = null;
setDraggingFqn(null);
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
|
|
||
| const getRowKey = useCallback( | ||
| (record: T, index: number): string => { | ||
| if (typeof rest.rowKey === 'function') { |
There was a problem hiding this comment.
💡 Edge Case: getRowKey function branch has no undefined/null guard
When rowKey is a function prop, getRowKey wraps the result with String(...) without checking for undefined/null. If the callback returns undefined for any record, the key becomes the literal string "undefined", causing key collisions across all such rows. The string-based rowKey path correctly falls back to the array index, but the function path does not.
In practice this is unlikely since callers provide well-behaved rowKey functions, but it's a subtle inconsistency with the string path's behavior.
Suggested fix:
if (typeof rest.rowKey === 'function') {
const val = (rest.rowKey as (record: T) => string | number)(record);
return val !== undefined && val !== null ? String(val) : String(index);
}
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
| </div> | ||
| </div> | ||
|
|
||
| <div |
There was a problem hiding this comment.
⚠️ Bug: Loading overlay uses absolute but parent lacks relative
The loading spinner overlay at line 621 uses tw:absolute tw:inset-0 to cover the table area. However, its parent <div> at line 616-617 does not have position: relative, so the overlay will be positioned relative to the nearest positioned ancestor (the outermost container div at line 538, which also lacks position: relative unless set via containerClassName). This means the overlay will not correctly cover just the table area — it may cover the search bar and pagination as well, or escape the intended bounds entirely if no ancestor is positioned.
Suggested fix:
Add `tw:relative` to the parent div:
```tsx
<div
className="tw:relative tw:flex tw:flex-col tw:w-full"
data-testid={dataTestId}
style={scrollStyle}>
```
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
| getItems(keys: any) { | ||
| const key = String([...keys][0]); | ||
|
|
||
| flushSync(() => setDraggingFqn(key)); |
There was a problem hiding this comment.
⚠️ Bug: flushSync inside React Aria's getItems may corrupt drag state
In GlossaryTermTab.component.tsx line 1447, flushSync(() => setDraggingFqn(key)) is called inside the getItems callback of useDragAndDrop. This forces a synchronous React re-render during React Aria's internal drag-start sequence. React Aria maintains internal drag/drop state that can be left inconsistent when an external synchronous re-render interrupts its initialization. This is also fragile because flushSync is the only mechanism preventing a stale closure bug in getDropOperation (which reads draggingFqn from state). A safer pattern is to use a useRef to store the dragging key synchronously and read it in getDropOperation, avoiding flushSync entirely.
Suggested fix:
Replace the state-based approach with a ref:
```tsx
const draggingFqnRef = useRef<string | null>(null);
const { dragAndDropHooks } = useDragAndDrop({
getItems(keys) {
const key = String([...keys][0]);
draggingFqnRef.current = key;
setDraggingFqn(key); // normal setState for UI updates
return [{ 'text/plain': key }];
},
getDropOperation(target) {
if (target.type === 'item') {
const targetFqn = String(target.key);
const curDrag = draggingFqnRef.current;
if (curDrag && (targetFqn === curDrag || targetFqn.startsWith(curDrag + '.')))
return 'cancel';
return 'move';
}
return 'cancel';
},
...
});
```
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
|
|
||
| // ─── Expand toggle ──────────────────────────────────────────────────────── | ||
|
|
||
| const handleExpandToggle = useCallback( |
There was a problem hiding this comment.
💡 Edge Case: Controlled expand: rapid toggles may send stale keys
In handleExpandToggle (TableV2.tsx ~line 321-335), when in controlled mode (expandedRowKeys provided), the next set passed to onExpandedRowsChange is computed from the current expandedKeys memo, which derives from the prop. If two rapid expand/collapse toggles happen before React re-renders with the parent's updated state, the second toggle computes next from a stale prop snapshot. This is a minor edge case since most expand interactions are single-click with synchronous parent state updates, but it could manifest in automated testing or with debounced parent state.
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
|
|
This PR has had no activity for 30 days and will be closed in 7 days if no further activity occurs. |
1 similar comment
|
This PR has had no activity for 30 days and will be closed in 7 days if no further activity occurs. |
❌ 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 |
1 participant
Fixes 3847
Describe your changes:
Summary
TableV2.tsxas a drop-in replacement forTable.tsx, rendering via@openmetadata/ui-core-components(React Aria + Untitled UI) instead of Ant DesignTableComponentProps<T>interface — zero changes required for existing consumersBotListV1to useTableV2What's migrated
locale.emptyText)What's implemented
UntitledTablerowKeywith AntD index-fallback parityNextPrevioussortedDataSourcebefore renderingcustomPaginationProps)scroll.x/scroll.yoverflowX/overflowY+maxHeightapplied to inner wrapperselectedRowKeysforwarded as controlledselectedKeys;onChangefired with correct typeresizableColumns)react-resizablewith stable per-column handlers (no closure-per-render)searchProps)Searchbarcomponentloadingspinner overlaysorter: (a,b) => numberapplied to full dataset before pagination;sorter: truedelegates to parentonChangerowClassName,cellClassNameonRowonClickandonDoubleClickforwarded toUntitledTable.Rowlocale.emptyTextrenderEmptyStateextraTableFiltersdata-row-keyattributeidas DOM attribute)classNameremoved from props typecontainerClassNameinsteadKnown limitations (v1)
Documented — accepted but ignored
expandable— React Aria has no built-in expandable row conceptcomponents— AntD custom cell/header renderersSilent gaps — will need fixes before migrating affected consumers
col.alignnot appliedalign: 'center'or'right'col.ellipsisnot appliedcol.fixed(sticky columns) not implementedcol.colSpan/rowSpanfrom render return discardedonRowother handlers (onMouseEnter,onContextMenuetc.) not wired<thead>not implementedscroll.yexpect sticky headersshowHeader={false}not implementedrowSelection.getCheckboxPropsignoredonChangeonly fired for sortonChangefor pagination/filter side effectsPlaywright fixes included
bot.ts:tr[id=…]→tr[data-row-key=…](React Aria doesn't set DOMidon rows)importUtils.ts:getByRole('cell')→getByRole('gridcell')(React Aria emits explicitrole="gridcell")importUtils.ts:getByRole('columnheader', { name })→.filter({ hasText })(innerrole="group"wrapper breaks ARIA name computation)Customproperties-part2.spec.ts: scroll test restructured — verifies 1 row = not scrollable, 3 rows = scrollable; uses distinct values per rowConsumers migrated in this PR
BotListV1— listing table with search, sort, row clickSchemaTable
Screen.Recording.2026-03-10.at.5.26.33.PM.mov
Screen.Recording.2026-03-04.at.1.07.58.PM.mov
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
getGlossaryTermGraphtoRdfRepositoryto enable graph-based visualization of glossary term relationships.UserSSOOAuthProviderto support unified SSO and Basic Auth for MCP servers.OAuthHttpStatelessServerTransportProviderwith authentication middleware and dynamic CORS configuration.SearchIndexRetryWorkerto manage and retry failed index events usingSearchIndexRetryQueue.OntologyExplorerfor visualizing glossary relationships and administrative settings viaGlossaryTermRelationSettingsPage.This will update automatically on new commits.