feat(ui): Glossary Terms table on TableV2 — adaptable search, column resize, drag reorder, sticky header#29728
feat(ui): Glossary Terms table on TableV2 — adaptable search, column resize, drag reorder, sticky header#29728siddhant1 wants to merge 25 commits into
Conversation
❌ 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! |
siddhant1
added
UI
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| const { dragAndDropHooks } = useDragAndDrop({ | ||
| getItems: (keys) => { | ||
| const key = Array.from(keys)[0]; | ||
| const record = key ? glossaryTermByFqn.get(String(key)) : undefined; | ||
|
|
||
| if (!record || record.isLoadMoreButton) { | ||
| return []; | ||
| } | ||
|
|
||
| return [{ [GLOSSARY_TERM_DRAG_TYPE]: record.fullyQualifiedName ?? '' }]; | ||
| }, | ||
| acceptedDragTypes: [GLOSSARY_TERM_DRAG_TYPE], | ||
| onDragStart: (event) => { | ||
| const key = Array.from(event.keys)[0]; | ||
| draggedGlossaryTermRef.current = key |
There was a problem hiding this comment.
💡 Edge Case: Row drag reorder is not gated by edit permission
The new useDragAndDrop wiring is always enabled and passed to both the populated and empty tables regardless of permissions.EditAll/permissions.Create. The toolbar Bulk Edit button is permission-gated (getBulkEditButton(permissions.EditAll, ...)), but any user who can view the terms can now drag a row to re-parent/reorder it. This triggers the confirmation modal and a patchGlossaryTerm PATCH which will fail server-side for unauthorized users, surfacing an error toast instead of preventing the action. Consider only providing dragAndDropHooks when the user has the required permission (e.g. permissions.EditAll/permissions.Create), or making getDropOperation/getItems return cancel/[] when unauthorized so the drag affordance is disabled entirely.
Disable drag source when the user lacks edit permission (also guard getDropOperation).:
getItems: (keys) => {
if (!permissions.EditAll) {
return [];
}
const key = Array.from(keys)[0];
const record = key ? glossaryTermByFqn.get(String(key)) : undefined;
if (!record || record.isLoadMoreButton) {
return [];
}
return [{ [GLOSSARY_TERM_DRAG_TYPE]: record.fullyQualifiedName ?? '' }];
},
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
|
|
||
| const GLOSSARY_TERM_DRAG_TYPE = 'application/x-om-glossary-term'; | ||
|
|
||
| const GLOSSARY_TABLE_SCROLL = { y: 'calc(100vh - 350px)' }; |
There was a problem hiding this comment.
💡 Quality: Sticky-header scroll offset is a hardcoded magic value
GLOSSARY_TABLE_SCROLL = { y: 'calc(100vh - 350px)' } hardcodes a viewport-relative offset that assumes a fixed amount of chrome (350px) above the table. In embedded/modal layouts or when the header height changes, the table's internal scroll region will be mis-sized (too tall, causing double scrollbars, or too short). The PR description itself flags this as needing per-layout tuning. Consider deriving the height from the actual container via a ref/ResizeObserver (a containerWidth observer already exists in this component) rather than a constant, or documenting the assumption clearly.
Document/derive the scroll height dynamically to avoid layout-specific breakage.:
// Prefer computing from the measured container instead of a fixed 350px:
// const scrollY = containerHeight ? containerHeight - toolbarHeight : 'calc(100vh - 350px)';
const GLOSSARY_TABLE_SCROLL = { y: 'calc(100vh - 350px)' };
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
There was a problem hiding this comment.
Pull request overview
Migrates the Glossary Terms table UI to the React Aria TableV2/TableCard stack and restores key UX behaviors (adaptive search layout, column resizing affordance, drag-and-drop re-parenting, sticky header + internal scroll for infinite loading).
Changes:
- Updates
GlossaryTermTabto renderTableV2insideTableCard, add internalscroll.y, and implement RAC drag-and-drop viauseDragAndDrop. - Adjusts glossary table styling for nested-row shading and RAC sticky-header scrolling behavior.
- Fixes
ColumnResizerhandle styling by replacing deprecated Tailwind arbitrary CSS var syntax with theme utility classes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx | Switch to TableV2/TableCard, implement RAC drag-and-drop hooks, and rework table scroll/infinite-load wiring. |
| openmetadata-ui/src/main/resources/ui/src/components/Glossary/glossaryV1.less | Update nested-row background styling for the new table DOM and adjust overflow behavior for sticky header scroll container. |
| openmetadata-ui/src/main/resources/ui/src/components/common/Table/TableV2.tsx | Fix resizer indicator styling using valid Tailwind theme tokens. |
| getItems: (keys) => { | ||
| const key = Array.from(keys)[0]; | ||
| const record = key ? glossaryTermByFqn.get(String(key)) : undefined; | ||
|
|
||
| if (!record || record.isLoadMoreButton) { | ||
| return []; | ||
| } | ||
|
|
||
| return [{ [GLOSSARY_TERM_DRAG_TYPE]: record.fullyQualifiedName ?? '' }]; | ||
| }, |
| // Monitor for DOM changes to detect when the table becomes scrollable | ||
| useEffect(() => { | ||
| const observer = new MutationObserver(() => { |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| const GLOSSARY_TERM_DRAG_TYPE = 'application/x-om-glossary-term'; | ||
|
|
||
| const GLOSSARY_TABLE_SCROLL = { y: 'calc(100vh - 350px)' }; |
There was a problem hiding this comment.
calc(100vh - 350px) assumes exactly 350 px of chrome above the table (nav bars, breadcrumbs, tab headers). The PR description acknowledges this "may need minor tuning per layout," but storing it as a module-level constant makes it invisible and easy to miss when layout changes. Consider deriving the offset from tableContainerRef at runtime (similar to the existing containerWidth measurement) so the table height stays correct regardless of the page's header height.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
siddhant1
force-pushed
the
glossary-table-v2
branch
from
559633f to
900c293
Compare
| const glossaryTermByFqn = useMemo(() => { | ||
| const termByFqn = new Map<string, ModifiedGlossaryTerm>(); | ||
| const walk = (terms: ModifiedGlossaryTerm[]) => { | ||
| terms.forEach((term) => { | ||
| if (term.fullyQualifiedName) { | ||
| termByFqn.set(term.fullyQualifiedName, term); | ||
| } | ||
| if (term.children?.length) { | ||
| walk(term.children as ModifiedGlossaryTerm[]); | ||
| } | ||
| }); | ||
| }; | ||
| walk(filteredGlossaryTerms); | ||
|
|
||
| return termByFqn; | ||
| }, [filteredGlossaryTerms]); |
There was a problem hiding this comment.
glossaryTermByFqn corrupted by load-more placeholders, breaking drag-and-drop on paginated parents
processTermsWithLoadMore appends a load-more placeholder to a parent term's children via spread ({ ...term, isLoadMoreButton: true, key: ... }). The placeholder inherits the parent's fullyQualifiedName. When walk descends into that parent's children, it hits the placeholder and overwrites the parent's entry in the map:
termByFqn.set("Glossary.TermA", loadMoreButton) ← clobbers the real term
From that point, for any parent with hasMoreChildren: true and childrenPagingAfter set:
getItemsreturns[](placeholder'sisLoadMoreButtoncheck), so the row cannot be draggedgetDropOperationreturns'cancel', so nothing can be dropped onto it
Fix: skip load-more placeholders in the walk by adding an !term.isLoadMoreButton guard before inserting into the map.
…resize, drag reorder, sticky header Finish migrating the Glossary Terms table to the React Aria TableV2/TableCard stack and restore the behaviors lost in the migration: - Adaptable search bar: the toolbar search grows/shrinks to fill space while Status/Bulk Edit/Expand-All stay fixed and visible. - Column resize indicator: fix the ColumnResizer handle, which was invisible under Tailwind v4 (deprecated bg-[--var] arbitrary syntax -> valid bg-border-* utilities). - Row drag-and-drop reorder/re-parent: replace the stripped native HTML5 onRow drag props with React Aria useDragAndDrop (dragAndDropHooks), reusing the existing move-confirmation flow; load-more rows excluded as sources/targets. - Sticky header: give the table its own vertical scroll (scroll.y) so the sticky <thead> engages, make the outer container non-scrolling, and re-point infinite scroll at the internal scroll region. - glossaryV1.less: RAC-compatible nested-row shading and the sticky-header enabling rule (neutralize the table's overflow-x). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
siddhant1
force-pushed
the
glossary-table-v2
branch
from
900c293 to
c86e6be
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1256
extraTableFiltersis memoized withuseMemo, but its dependency array is missing several values used inside the memo (e.g.,t,permissions.EditAll,handleEditGlossary,handleSearchChange). This can cause stale UI (e.g., bulk edit button visibility not updating with permissions, search input handler changes not reflected, and i18n text not updating on language change) and may violate the repo’s hook-deps lint rules.
}, [
isAllExpanded,
isExpandingAll,
isStatusDropdownVisible,
statusDropdownMenu,
The suite still mocked the old react-dnd stack and rendered the real react-aria resizable + drag-and-drop TableV2, which does not mount in jsdom (all 43 tests crashed on render). Mock common/Table/TableV2 with a lightweight table that invokes the component's column render functions, expand icon, and toolbar filters — matching the pattern used by other TableV2-based suites — and drop the dead react-dnd mocks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jest test CoverageUI tests summary
|
onDragStart stored whatever row key was dragged into draggedGlossaryTermRef without the isLoadMoreButton guard its sibling getItems has. Since react-aria spreads drag props on the whole row, a load-more placeholder could be dragged and reach the custom root-drop handler, opening the move modal with an invalid "<parentFqn>-load-more" id. Align onDragStart with getItems so placeholder drags are inert. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Glossary Terms table moved from AntD Table to react-aria TableV2, changing
the rendered markup. Update the shared glossary util and specs that still
queried AntD-specific selectors so the glossary E2E suite matches the new DOM.
- validateGlossaryTerm: getByRole('columnheader',{name}) -> th hasText — the
react-aria <th> isn't matched by the columnheader role query. This helper is
called by createGlossaryTerm, so its failure cascaded across nearly the whole
glossary suite.
- filterStatus / status-filter specs: tbody.ant-table-tbody > tr -> tbody > tr
- row counts: tbody .ant-table-row -> tbody tr[data-row-key]
- nested level rows: .ant-table-row-level-1 -> tr[data-level="1"]
- expand controls: .ant-table-row-expand-icon / .vertical-baseline
-> [data-testid="expand-icon"]
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to the glossary selector migration, validated against a real
TableV2 build (dev server) which surfaced interaction/role differences the
AntD DOM masked:
- selectActiveGlossaryTerm: use a plain click; a forced click on the term
link is swallowed by the draggable react-aria row and never navigates.
- confirmationDragAndDropGlossary: scope the "Move" button to the
confirmation modal — TableV2 drag handles expose aria-label "Move the
term", which also matched getByRole('button', { name: 'Move' }).
- openColumnDropdown: drop the AntD '.ant-dropdown [role=menu]' scope; the
customize dropdown is a react-aria popover.
- Term name cells are react-aria rowheaders, not cells:
getByRole('cell', { name }) -> getByRole('rowheader', { name }); the
exact-name variant (drag-handle label pollutes the accessible name) ->
getByTestId.
- GlossaryTermTab: emit a stable glossary-term-level-{n} row class
(react-aria clobbers data-level), replacing .ant-table-row-level-N.
- Fix a glossary-term-table testid typo (-> glossary-terms-table).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace brittle CSS-class selectors with data-testid in the glossary terms
table code (specs + the component/DOM queries they depend on):
- GlossaryTermTab: add data-testid to the scroll container, status-filter
options and Save/Cancel buttons; switch findScrollContainer + the drag
effect off class/Tailwind querySelectors (.p-x-md.p-y-md,
.glossary-terms-scroll-container .glossary-terms-table table) to testids;
drop the now-unused glossary-term-level row class.
- TableV2: add data-testid="table-toolbar" (backs the drag effect's toolbar
query). DraggableMenuItem: add data-testid="column-menu-item-{key}".
- Specs: .glossary-terms-scroll-container, .glossary-dropdown-label,
.glossary-term-level-1 (now counted via child term testids +
load-more-children-button), .draggable-menu-item*, .glossary-details,
status .ant-btn-primary/.ant-btn-default, .status-selection-dropdown, and
.ant-skeleton all switch to getByTestId. Column/status helpers now take
stable keys/values.
Validated against a real TableV2 build (dev server).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The column-customize helpers now select by data-testid keyed on the column
key (column-menu-item-{key}); this caller still passed the display label
('Reviewer'), so getByTestId('column-menu-item-Reviewer') never matched and
the test timed out. Pass the key ('reviewers') and keep the label for the
header visibility check.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Discard glossary term fetch responses whose search term or status filter no longer matches the active context, so an in-flight listing request can no longer repopulate the table after a search has cleared it. Deduplicate terms by fullyQualifiedName (memo + load-more merge) to avoid duplicate row keys, which made the table collection throw "Invalid array length" while rendering. Fix the large-glossary pagination E2E to scroll the table's inner scroll region (which owns the vertical overflow) instead of the outer container, so infinite scroll is actually triggered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review 👍 Approved with suggestions 1 resolved / 3 findingsMigrates the Glossary Terms table to the TableV2 stack with improved drag-and-drop, sticky headers, and responsive search layout. Please address the missing drag-and-drop permission gating and consider replacing the magic scroll offset with a more robust layout-relative value. 💡 Edge Case: Row drag reorder is not gated by edit permissionThe new Disable drag source when the user lacks edit permission (also guard getDropOperation).💡 Quality: Sticky-header scroll offset is a hardcoded magic value📄 openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:127 📄 openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1618 📄 openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1656
Document/derive the scroll height dynamically to avoid layout-specific breakage.✅ 1 resolved✅ Bug: Infinite-scroll relies on fragile nested DOM selector
🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
What
Completes the migration of the Glossary Terms table to the React Aria
TableV2/TableCardstack and restores the behaviors that were lost during the migration.Changes
ColumnResizerhandle was invisible under Tailwind v4 (deprecatedbg-[--var]arbitrary-value syntax silently dropped). Switched to the validbg-border-secondary/bg-border-brandutilities: a subtle divider at idle, brand-blue while dragging.Rowstrips native HTML5 drag props (filterDOMProps), so the oldonRowdrag handlers never reached the DOM. Reimplemented withuseDragAndDrop(dragAndDropHooks), reusing the existing move-confirmation modal + PATCH flow. Load-more placeholder rows are excluded as drag sources/targets.scroll.y) so the sticky<thead>engages; the outer container no longer scrolls; and infinite scroll is re-pointed at the internal scroll region.Files
GlossaryTermTab.component.tsx— search layout, RAC drag-and-drop, internal-scroll + sticky-header wiring, infinite-scroll rework.common/Table/TableV2.tsx— column resize indicator token fix.Testing
Manual verification in the Glossary → Terms tab:
🤖 Generated with Claude Code
Summary by Gitar
useDragAndDropfromreact-aria-componentsto handle row re-parenting and promotion, replacing deprecated HTML5 handlers.getDropOperationlogic to manage valid drop targets and prevent interaction with load-more rows.TableCard.Rootand updatedGlossaryTermTabto use internalscroll.yfor consistent sticky header behavior.useInViewinfinite scroll observer in favor of the table's internal scroll management.glossaryTermByFqnlookup memo for efficient drag-and-drop target identification.permissions.EditAlldependency inextraTableFiltersmemo to ensure accurate bulk-edit button visibility.This will update automatically on new commits.
Greptile Summary
This PR completes the Glossary Terms table migration to the React Aria
TableV2/TableCardstack, restoring four behaviors lost during the migration: an adaptable search bar, a visible column-resize indicator, row drag-and-drop reorder/re-parent viauseDragAndDrop, and a sticky header backed by an internal scroll region. Playwright tests are updated to match the new DOM structure —data-testidselectors replace Ant Design class-name selectors, and scroll helpers walk the DOM to find the real scrollable container instead of the now-static outer div.GlossaryTermTab.component.tsx— switches fromuseInView-based infinite scroll to aMutationObserver+ scroll-event hybrid, wires up React Aria drag-and-drop withuseDragAndDrop, and routes internal scroll through the TableV2scroll.yprop so the sticky header engages.TableV2.tsx— fixes theColumnResizercolor token from the invalidbg-[--var]arbitrary-value syntax to the validbg-border-secondary/bg-border-brandTailwind v4 utilities.LargeGlossaryPerformance.spec.tsandGlossaryStatusFilterLargeDataset.spec.tsnow use the DOM-walking helper to locate the real scroll container, resolving the silent-discard scroll tests flagged in earlier review rounds.Confidence Score: 5/5
Safe to merge; all changes are additive UI behavior or test selector updates with no data-model or API contract changes.
The core drag-and-drop and infinite-scroll rewrites are mechanically sound and the previously flagged scroll-container issues in the test suite have been addressed. The two newly found concerns are narrow edge cases that do not affect primary user flows and are mitigated by surrounding guards.
GlossaryTermTab.component.tsx carries the most complex logic (dual MutationObserver + scroll handler, drag-and-drop wiring, glossaryTermByFqn map) and warrants the closest reading; DraggableMenuItemV2.component.tsx should be verified for keyboard accessibility before the next accessibility audit.
Important Files Changed
Reviews (22): Last reviewed commit: "Merge branch 'main' into glossary-table-..." | Re-trigger Greptile