feat(ui): Glossary Terms table on TableV2 — adaptable search, column resize, drag reorder, sticky header#29728
Open
siddhant1 wants to merge 3 commits into
Open
feat(ui): Glossary Terms table on TableV2 — adaptable search, column resize, drag reorder, sticky header#29728siddhant1 wants to merge 3 commits into
siddhant1 wants to merge 3 commits into
Conversation
Contributor
❌ 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 |
Contributor
|
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}
Comment on lines
+1478
to
+1492
| 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 👍 / 👎
Contributor
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. |
Comment on lines
+1479
to
+1488
| 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 ?? '' }]; | ||
| }, |
Comment on lines
497
to
499
| // 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)' }; |
Contributor
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
Comment on lines
+1461
to
+1476
| 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]); |
Contributor
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
Contributor
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,
…and fix scroll-observer deps - getDropOperation: only accept item drops with dropPosition 'on' (whole-row re-parent). Previously before/after positions returned 'move', so React Aria offered between-row insertion targets that no handler serviced — the indicator showed and the drop was silently discarded. - MutationObserver infinite-scroll effect: add missing searchTerm and fetchAllTerms deps so its callback no longer reads a stale search mode / fetch closure (matches the sibling scroll-listener effect). 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 |
Contributor
🔴 Playwright Results — 77 failure(s), 18 flaky✅ 4377 passed · ❌ 77 failed · 🟡 18 flaky · ⏭️ 69 skipped
Genuine Failures (failed on all attempts)❌
|
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
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.
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
TableV2/TableCardstack, restoring three behaviors that were lost in the migration: column resize visibility (Tailwind v4 token fix), row drag-and-drop re-parenting (reimplemented with React Aria'suseDragAndDrop), and sticky header (viascroll.yon the table itself).flex-1and groups Status / Bulk Edit / Expand-All in a fixedflex-shrinkcontainer, preventing button clipping on narrow viewports.onRow/onHeaderRowHTML5 handler approach (which React Aria'sfilterDOMPropssilently stripped) withuseDragAndDrop; adraggedGlossaryTermRefcaptures the source term ononDragStartand is consumed inonItemDrop/onRootDrop, reusing the existing move-confirmation modal and PATCH flow.useInViewin favour of aMutationObserverthat triggers auto-fill when content doesn't fill the viewport, plus a scroll event listener on the table's internal scroll container for page-through loading.Confidence Score: 4/5
Safe to merge with the understanding that between-row drops are silently discarded (no reorder) and a parent term can be dragged onto its own descendant, showing a valid drop indicator before failing at the API layer.
The drag-and-drop reimplementation is largely correct, but getDropOperation returns 'move' for 'between'-type targets even though there is no onBetweenDrop handler — drops landing between rows are silently ignored, which is confusing to users expecting reorder behaviour. Additionally, there is no ancestor-descendant cycle guard: dragging a parent over one of its children shows a valid drop indicator, opens the confirmation modal, and only fails at the API level with an error toast. Neither issue corrupts data, but both produce misleading UI feedback on a core interaction path.
GlossaryTermTab.component.tsx — specifically the getDropOperation callback and the drag-and-drop wiring; the TableV2.tsx and glossaryV1.less changes are straightforward and low risk.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant User participant TableV2 as TableV2 (React Aria) participant useDragAndDrop participant GlossaryTermTab participant API User->>TableV2: starts dragging row (TermA) TableV2->>useDragAndDrop: onDragStart(keys) useDragAndDrop->>GlossaryTermTab: "sets draggedGlossaryTermRef.current = TermA" User->>TableV2: hovers over target row (TermB) TableV2->>useDragAndDrop: getDropOperation(target, types) useDragAndDrop->>GlossaryTermTab: looks up target in glossaryTermByFqn GlossaryTermTab-->>useDragAndDrop: returns move or cancel User->>TableV2: drops on TermB TableV2->>useDragAndDrop: onItemDrop(event) useDragAndDrop->>GlossaryTermTab: reads draggedGlossaryTermRef, calls handleMoveRow(TermA, TermB) GlossaryTermTab->>GlossaryTermTab: opens confirmation modal User->>GlossaryTermTab: confirms move GlossaryTermTab->>API: patchGlossaryTerm(TermA.id, jsonPatch) API-->>GlossaryTermTab: success or error GlossaryTermTab->>GlossaryTermTab: refreshGlossaryTerms() or showErrorToast() User->>TableV2: drops on empty area (root) TableV2->>useDragAndDrop: onRootDrop() useDragAndDrop->>GlossaryTermTab: reads draggedGlossaryTermRef, calls handleMoveRow(TermA, undefined) GlossaryTermTab->>API: patchGlossaryTerm(TermA.id, remove-parent patch)%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant User participant TableV2 as TableV2 (React Aria) participant useDragAndDrop participant GlossaryTermTab participant API User->>TableV2: starts dragging row (TermA) TableV2->>useDragAndDrop: onDragStart(keys) useDragAndDrop->>GlossaryTermTab: "sets draggedGlossaryTermRef.current = TermA" User->>TableV2: hovers over target row (TermB) TableV2->>useDragAndDrop: getDropOperation(target, types) useDragAndDrop->>GlossaryTermTab: looks up target in glossaryTermByFqn GlossaryTermTab-->>useDragAndDrop: returns move or cancel User->>TableV2: drops on TermB TableV2->>useDragAndDrop: onItemDrop(event) useDragAndDrop->>GlossaryTermTab: reads draggedGlossaryTermRef, calls handleMoveRow(TermA, TermB) GlossaryTermTab->>GlossaryTermTab: opens confirmation modal User->>GlossaryTermTab: confirms move GlossaryTermTab->>API: patchGlossaryTerm(TermA.id, jsonPatch) API-->>GlossaryTermTab: success or error GlossaryTermTab->>GlossaryTermTab: refreshGlossaryTerms() or showErrorToast() User->>TableV2: drops on empty area (root) TableV2->>useDragAndDrop: onRootDrop() useDragAndDrop->>GlossaryTermTab: reads draggedGlossaryTermRef, calls handleMoveRow(TermA, undefined) GlossaryTermTab->>API: patchGlossaryTerm(TermA.id, remove-parent patch)Reviews (4): Last reviewed commit: "fix(ui): address review — restrict term ..." | Re-trigger Greptile