-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Improve glossary terms loading and expand/collapse behaviour #31886
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
732d914
fb24694
4637fbf
d984655
06d49d8
3a2dbf1
05cd6d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,7 +56,6 @@ | |
| import { OwnerLabel } from '../../../components/common/OwnerLabel/OwnerLabel.component'; | ||
| import StatusBadge from '../../../components/common/StatusBadge/StatusBadge.component'; | ||
| import { | ||
| API_RES_MAX_SIZE, | ||
| DE_ACTIVE_COLOR, | ||
| NO_DATA_PLACEHOLDER, | ||
| PAGE_SIZE_LARGE, | ||
|
|
@@ -135,7 +134,7 @@ | |
|
|
||
| const GLOSSARY_TABLE_SCROLL = { x: 'max-content', y: 'calc(100vh - 350px)' }; | ||
|
|
||
| const GlossaryTermTab = ({ isGlossary, className }: GlossaryTermTabProps) => { | ||
|
Check warning on line 137 in openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx
|
||
| const navigate = useNavigate(); | ||
| const { currentUser } = useApplicationStore(); | ||
| const tableContainerRef = useRef<HTMLDivElement>(null); | ||
|
|
@@ -172,7 +171,7 @@ | |
| expandableKeys: findExpandableKeysForArray(terms), | ||
| glossaryTerms: terms, | ||
| }; | ||
| }, [glossaryChildTerms, findExpandableKeysForArray]); | ||
|
Check warning on line 174 in openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx
|
||
|
|
||
| const [movedGlossaryTerm, setMovedGlossaryTerm] = | ||
| useState<MoveGlossaryTermType>(); | ||
|
|
@@ -204,6 +203,12 @@ | |
| const [searchTerm, setSearchTerm] = useState(''); | ||
| const searchTermRef = useRef(searchTerm); | ||
| searchTermRef.current = searchTerm; | ||
| // Live ref to the active glossary so an in-flight request can verify the | ||
| // glossary is still the current one before applying its response — the | ||
| // component stays mounted across glossary switches, so an uncancelled request | ||
| // for the previous glossary must not repopulate the table. | ||
| const activeGlossaryFqnRef = useRef(activeGlossary?.fullyQualifiedName); | ||
| activeGlossaryFqnRef.current = activeGlossary?.fullyQualifiedName; | ||
| const [searchInput, setSearchInput] = useState(''); | ||
| const [searchPaging, setSearchPaging] = useState<{ | ||
| offset: number; | ||
|
|
@@ -220,11 +225,11 @@ | |
| setSearchTerm(value); | ||
| }, []); | ||
|
|
||
| const debouncedSetSearchTerm = useCallback(debounce(handleSearch, 500), [ | ||
|
Check warning on line 228 in openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx
|
||
| handleSearch, | ||
| ]); | ||
|
|
||
| const fetchChildTerms = async (parentFQN: string, after?: string) => { | ||
|
Check warning on line 232 in openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx
|
||
| setLoadingChildren((prev) => ({ ...prev, [parentFQN]: true })); | ||
| try { | ||
| const response = await getGlossaryTermChildrenLazy(parentFQN, 50, after); | ||
|
|
@@ -283,6 +288,10 @@ | |
| } | ||
|
|
||
| setIsTableLoading(true); | ||
| // A fresh first-level fetch supersedes any in-flight expand-all, so clear | ||
| // its indicator; the superseded expand-all's own cleanup is sequence-gated | ||
| // and will not touch this loading state. | ||
| setIsExpandingAll(false); | ||
|
|
||
| if (searchTerm) { | ||
| setSearchPaging({ offset: 0, total: undefined, hasMore: true }); | ||
|
|
@@ -291,7 +300,7 @@ | |
| } | ||
| }; | ||
|
|
||
| const fetchAllTerms = async (loadMore = false) => { | ||
|
Check warning on line 303 in openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx
|
||
| // `fetchSearchTerm` / `fetchStatusKey` record the search and status filter | ||
| // this request was issued for so its response can be discarded if either has | ||
| // since changed. `requestSeq` tracks the most recent fetch so only the | ||
|
|
@@ -300,6 +309,7 @@ | |
| const requestSeq = ++fetchRequestSeqRef.current; | ||
| const fetchSearchTerm = searchTerm; | ||
| const fetchStatusKey = selectedStatus.join(','); | ||
| const fetchGlossaryFqn = activeGlossary?.fullyQualifiedName; | ||
| initializeLoadingStates(loadMore); | ||
|
|
||
| try { | ||
|
|
@@ -311,34 +321,23 @@ | |
| ? selectedStatus.filter((s) => s !== 'all').join(',') | ||
| : undefined; | ||
|
|
||
| // Use search API if search term is present | ||
| // Fetch the page first and defer every shared-state mutation until the | ||
| // response is confirmed current by the stale-context guard below. | ||
| let searchOffset = 0; | ||
| if (searchTerm) { | ||
| const currentOffset = loadMore ? searchPaging.offset : 0; | ||
| searchOffset = loadMore ? searchPaging.offset : 0; | ||
| const response = await searchGlossaryTermsPaginated({ | ||
| q: searchTerm, | ||
| glossaryFqn: activeGlossary?.fullyQualifiedName, | ||
| limit: PAGE_SIZE_LARGE, | ||
| offset: currentOffset, | ||
| offset: searchOffset, | ||
| fields: | ||
| 'children,relatedTerms,reviewers,owners,tags,usageCount,domains,extension,childrenCount', | ||
| entityStatus: entityStatusParam, | ||
| }); | ||
| data = response.data; | ||
| pagingResponse = response.paging; | ||
|
|
||
| // Update search pagination state | ||
| const newOffset = currentOffset + PAGE_SIZE_LARGE; | ||
| const hasMore = | ||
| data.length === PAGE_SIZE_LARGE && | ||
| (pagingResponse?.total === undefined || | ||
| newOffset < pagingResponse?.total); | ||
| setSearchPaging({ | ||
| offset: newOffset, | ||
| total: pagingResponse?.total, | ||
| hasMore, | ||
| }); | ||
| } else { | ||
| // Use regular listing API when no search term | ||
| const response = await getFirstLevelGlossaryTermsPaginated( | ||
| activeGlossary?.fullyQualifiedName || '', | ||
| PAGE_SIZE_LARGE, | ||
|
|
@@ -347,39 +346,60 @@ | |
| ); | ||
| data = response.data; | ||
| pagingResponse = response.paging; | ||
|
|
||
| // Update regular paging state for next page | ||
| handlePagingChange((prev) => ({ | ||
| ...prev, | ||
| after: pagingResponse?.after, | ||
| total: pagingResponse?.total || prev.total, | ||
| })); | ||
| } | ||
|
|
||
| // Apply the response only when it still matches the active search context. | ||
| // A response computed for a different (now-outdated) search term — e.g. a | ||
| // listing request that was in flight when the user typed a query, or a | ||
| // stale search-mode fetch after the query changed — is discarded so it | ||
| // cannot repopulate or clear the table against the user's current intent. | ||
| if ( | ||
| !data || | ||
| !Array.isArray(data) || | ||
| // A response is applied only while it still matches the context it was | ||
| // issued for (same request sequence, search term, status filter, and | ||
| // glossary). Re-checked at every await boundary — including the follow-up | ||
| // count request below — so a request that goes stale mid-flight touches | ||
| // no shared state (pagination cursor, term list, or count). | ||
| const isSuperseded = () => | ||
| requestSeq !== fetchRequestSeqRef.current || | ||
| fetchSearchTerm !== searchTermRef.current || | ||
| fetchStatusKey !== selectedStatusRef.current.join(',') | ||
| ) { | ||
| fetchStatusKey !== selectedStatusRef.current.join(',') || | ||
| fetchGlossaryFqn !== activeGlossaryFqnRef.current; | ||
|
|
||
| if (!data || !Array.isArray(data) || isSuperseded()) { | ||
| return; | ||
| } | ||
|
|
||
| // A status-filtered empty page needs a follow-up total for the empty | ||
| // state. Fetch it before applying anything and re-validate afterwards so | ||
| // the previous context's empty result cannot overwrite the active one. | ||
| let totalCount = data.length; | ||
| if (data.length === 0 && isStatusFilterActive) { | ||
| const countResponse = await getFirstLevelGlossaryTermsPaginated( | ||
|
Check warning on line 371 in openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx
|
||
| activeGlossary?.fullyQualifiedName || '', | ||
| fetchGlossaryFqn || '', | ||
| 0 | ||
| ); | ||
| setTotalTermsCount(countResponse.paging?.total ?? 0); | ||
| if (isSuperseded()) { | ||
| return; | ||
| } | ||
| totalCount = countResponse.paging?.total ?? 0; | ||
| } | ||
|
|
||
| // Advance pagination state now that the response is confirmed current. | ||
| if (searchTerm) { | ||
| const newOffset = searchOffset + PAGE_SIZE_LARGE; | ||
| const hasMore = | ||
| data.length === PAGE_SIZE_LARGE && | ||
| (pagingResponse?.total === undefined || | ||
| newOffset < pagingResponse?.total); | ||
| setSearchPaging({ | ||
| offset: newOffset, | ||
| total: pagingResponse?.total, | ||
| hasMore, | ||
| }); | ||
| } else { | ||
| setTotalTermsCount(data.length); | ||
| handlePagingChange((prev) => ({ | ||
| ...prev, | ||
| after: pagingResponse?.after, | ||
| total: pagingResponse?.total || prev.total, | ||
| })); | ||
| } | ||
|
|
||
| setTotalTermsCount(totalCount); | ||
|
|
||
| const newTerms = data as ModifiedGlossary[]; | ||
|
|
||
| if (loadMore) { | ||
|
|
@@ -411,53 +431,111 @@ | |
| }; | ||
|
|
||
| const fetchExpadedTree = async () => { | ||
| // Share the same monotonic sequence as fetchAllTerms so whichever term | ||
| // load starts last owns the shared table + loading state; a superseded | ||
| // expand-all then applies no rows and clears no loading flags. | ||
| const requestSeq = ++fetchRequestSeqRef.current; | ||
| setIsTableLoading(true); | ||
| setIsExpandingAll(true); | ||
| const key = isGlossary ? 'glossary' : 'parent'; | ||
| const { data } = await getGlossaryTerms({ | ||
| [key]: activeGlossary?.id || '', | ||
| limit: API_RES_MAX_SIZE, | ||
| fields: [ | ||
| TabSpecificField.OWNERS, | ||
| TabSpecificField.PARENT, | ||
| TabSpecificField.CHILDREN, | ||
| ], | ||
| }); | ||
| setGlossaryChildTerms(buildTree(data) as ModifiedGlossary[]); | ||
| const keys = data.reduce((prev, curr) => { | ||
| if (curr.children?.length) { | ||
| prev.push(curr.fullyQualifiedName ?? ''); | ||
| const requestedGlossaryFqn = activeGlossary?.fullyQualifiedName; | ||
|
|
||
| try { | ||
| const allTerms: GlossaryTerm[] = []; | ||
| let after: string | undefined; | ||
|
|
||
| do { | ||
| const { data, paging } = await getGlossaryTerms({ | ||
|
Check warning on line 448 in openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx
|
||
| [key]: activeGlossary?.id || '', | ||
| limit: PAGE_SIZE_LARGE, | ||
| after, | ||
| fields: [ | ||
| TabSpecificField.OWNERS, | ||
| TabSpecificField.PARENT, | ||
| TabSpecificField.CHILDREN, | ||
| ], | ||
| }); | ||
| allTerms.push(...data); | ||
| // Stop on no forward progress — an empty page or a cursor that did not | ||
| // advance — so termination never depends solely on the server nulling | ||
| // the token (a stuck cursor would otherwise loop forever). | ||
| const nextAfter = paging?.after; | ||
| const hasProgress = data.length > 0 && nextAfter !== after; | ||
| after = hasProgress ? nextAfter : undefined; | ||
| } while (after); | ||
|
|
||
| // Discard the result if the glossary changed under us, or if a newer term | ||
| // load superseded this one while it was paging. | ||
| if ( | ||
| requestedGlossaryFqn !== activeGlossaryFqnRef.current || | ||
| requestSeq !== fetchRequestSeqRef.current | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| return prev; | ||
| }, [] as string[]); | ||
| setGlossaryChildTerms(buildTree(allTerms) as ModifiedGlossary[]); | ||
| const keys = allTerms.reduce((prev, curr) => { | ||
| if (curr.children?.length) { | ||
| prev.push(curr.fullyQualifiedName ?? ''); | ||
| } | ||
|
|
||
| setExpandedRowKeys(keys); | ||
| setIsTableLoading(false); | ||
| setIsExpandingAll(false); | ||
| return prev; | ||
| }, [] as string[]); | ||
|
|
||
| setExpandedRowKeys(keys); | ||
| } catch (error) { | ||
| if (requestSeq === fetchRequestSeqRef.current) { | ||
| showErrorToast(error as AxiosError); | ||
| } | ||
| } finally { | ||
| // Only the latest term load owns the shared loading flags. | ||
| if (requestSeq === fetchRequestSeqRef.current) { | ||
| setIsTableLoading(false); | ||
| setIsExpandingAll(false); | ||
|
Comment on lines
+492
to
+494
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the user clicks Expand All while an infinite-scroll request is pending, the shared sequence suppresses that request's cleanup, but the winning Expand All cleanup does not clear |
||
| } | ||
| } | ||
|
greptile-apps[bot] marked this conversation as resolved.
|
||
| }; | ||
| const fetchAllTasks = useCallback(async () => { | ||
| if (!activeGlossary?.fullyQualifiedName) { | ||
| const glossaryFqn = activeGlossary?.fullyQualifiedName; | ||
| if (!glossaryFqn) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const { data } = await listTasks({ | ||
| status: TaskEntityStatus.Open, | ||
| category: TaskCategory.Approval, | ||
| type: TaskEntityType.RequestApproval, | ||
| limit: API_RES_MAX_SIZE, | ||
| fields: 'about,assignees', | ||
| }); | ||
| const tasks: Task[] = []; | ||
| let after: string | undefined; | ||
|
|
||
| do { | ||
| const { data, paging } = await listTasks({ | ||
|
Check warning on line 509 in openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx
|
||
| status: TaskEntityStatus.Open, | ||
| category: TaskCategory.Approval, | ||
| type: TaskEntityType.RequestApproval, | ||
| aboutEntity: glossaryFqn, | ||
| limit: PAGE_SIZE_LARGE, | ||
| after, | ||
| fields: 'about,assignees', | ||
| }); | ||
| tasks.push(...data); | ||
| // Stop on no forward progress (empty page or non-advancing cursor) so | ||
| // termination never depends solely on the server nulling the token. | ||
| const nextAfter = paging?.after; | ||
| const hasProgress = data.length > 0 && nextAfter !== after; | ||
| after = hasProgress ? nextAfter : undefined; | ||
| } while (after); | ||
|
|
||
| // Discard the result if the user switched glossaries while paging. | ||
| if (glossaryFqn !== activeGlossaryFqnRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| // Glossary approvals are now workflow-managed RequestApproval tasks created | ||
| // for each glossary term, not legacy glossary-root tasks. | ||
| const tasksByTerm = data.reduce( | ||
| const tasksByTerm = tasks.reduce( | ||
| (acc: Record<string, Task[]>, task: Task) => { | ||
| const termFQN = task.about?.fullyQualifiedName; | ||
| const isGlossaryTermTask = | ||
| task.about?.type === EntityType.GLOSSARY_TERM && | ||
| termFQN?.startsWith(`${activeGlossary.fullyQualifiedName}.`); | ||
| termFQN?.startsWith(`${glossaryFqn}.`); | ||
|
|
||
| if (isGlossaryTermTask && termFQN) { | ||
| const entityLink = `<#E::${EntityType.GLOSSARY_TERM}::${termFQN}>`; | ||
|
|
@@ -485,26 +563,18 @@ | |
| useEffect(() => { | ||
| const currentFQN = activeGlossary?.fullyQualifiedName; | ||
|
|
||
| if ( | ||
| currentFQN && | ||
| !isLoadingMore && | ||
| currentFQN !== previousGlossaryFQN && | ||
| !toggleExpandBtn && | ||
| !searchTerm // Don't fetch if there's an active search | ||
| ) { | ||
| // Clear existing terms when switching glossaries | ||
| if (currentFQN && !isLoadingMore && currentFQN !== previousGlossaryFQN) { | ||
| // Reload whenever the glossary changes — including while a search is | ||
| // active, in which case fetchAllTerms re-runs the search scoped to the | ||
| // new glossary. Reset to the collapsed, paginated view so the new | ||
| // glossary starts scrollable and no stuck expand-all flag carries over. | ||
| setToggleExpandBtn(false); | ||
| setGlossaryChildTerms([]); | ||
| handlePagingChange((prev) => ({ ...prev, after: undefined })); | ||
| setPreviousGlossaryFQN(currentFQN); | ||
| fetchAllTerms(); | ||
| } | ||
| }, [ | ||
| activeGlossary?.fullyQualifiedName, | ||
| isLoadingMore, | ||
| previousGlossaryFQN, | ||
| toggleExpandBtn, | ||
| searchTerm, | ||
| ]); | ||
| }, [activeGlossary?.fullyQualifiedName, isLoadingMore, previousGlossaryFQN]); | ||
|
|
||
| // Clear terms when component unmounts | ||
| useEffect(() => { | ||
|
|
@@ -1130,8 +1200,14 @@ | |
| }; | ||
|
|
||
| const toggleExpandAll = useCallback(async () => { | ||
| setToggleExpandBtn((prev) => !prev); | ||
| if (expandedRowKeys.length === expandableKeys.length) { | ||
| // `toggleExpandBtn` marks whether the fully-expanded tree is currently | ||
| // shown; the scroll handlers gate the collapsed-view infinite scroll on | ||
| // `!toggleExpandBtn`. Reflect the resulting mode rather than blindly | ||
| // flipping — a blind toggle leaves the flag stuck `true` after an odd | ||
| // number of clicks, which permanently disables scroll-to-load-more. | ||
| const isCollapsing = expandedRowKeys.length === expandableKeys.length; | ||
| setToggleExpandBtn(!isCollapsing); | ||
| if (isCollapsing) { | ||
| // Collapse all - immediate UI update | ||
| setExpandedRowKeys([]); | ||
| fetchAllTerms(); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.