Fixes #29916: replace API_RES_MAX_SIZE with cursor-based pagination in GlossaryTermTab - #31949
Fixes #29916: replace API_RES_MAX_SIZE with cursor-based pagination in GlossaryTermTab#31949anuj-kumary wants to merge 6 commits into
Conversation
…n GlossaryTermTab Replace two usages of API_RES_MAX_SIZE (100,000) with do…while cursor pagination using PAGE_SIZE_LARGE (50), so the expand-all and task-loading paths no longer issue a single massive request. Also wrap fetchExpadedTree in try/catch/finally so a mid-loop network error cannot leave the loading spinner stuck, and suppress the false-positive no-api-calls-in-iteration lint warnings (pagination is O(pages), not N+1). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…switch Snapshot activeGlossary.fullyQualifiedName before each paginated loop and compare against the store's current value after all pages are fetched. If the user navigated to a different glossary while the request was in flight, the results are silently dropped instead of overwriting the new glossary's state. Addresses the race condition flagged by Greptile on PR #31946. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sk results Replace FQN-only staleness guards with monotonic sequence counters (expandTreeSeqRef, fetchTasksSeqRef) — the same pattern fetchAllTerms already uses with fetchRequestSeqRef. This fixes two races the FQN guard missed: - A→B→A: user switches back to the original glossary before an old in-flight request completes; FQN matches but results are stale. - finally-block spinner leak: a stale expand-all's finally block ran unconditionally and cleared the loading state owned by a newer request, making the new glossary appear loaded before its data arrived. Both setIsTableLoading/setIsExpandingAll in fetchExpadedTree's finally are now guarded by the same seq check so only the latest invocation can reset the loading indicators. Addresses Greptile and gitar-bot review comments on PR #31946. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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! |
anuj-kumary
added
skip-pr-checks
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
❌ UI Checkstyle Failed❌ ESLint + Prettier + Organise Imports (src)One or more source files have linting or formatting issues. Affected files
🔍 ESLint findings in this PR's files — 0 error(s), 33 warning(s)Errors block the build. Warnings do not yet — they are rules whose backlog is still 0 error(s), 33 warning(s) across 2 changed file(s).
All findings
Fix locally (fast - only checks files changed in this branch): make ui-checkstyle-changed |
Bump expandTreeSeqRef when switching glossaries so a still-paginating expand-all for the old glossary cannot pass its seq check and overwrite the newly selected glossary's child terms. Closes #29916 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the stale limit: 100000 (API_RES_MAX_SIZE) assertion with limit: 50 (PAGE_SIZE_LARGE) to match the listTasks cursor-pagination call introduced in the parent fix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Jest test CoverageUI tests summary
|
…ll expansion Remove fetchExpadedTree's all-pages-at-once loop (getGlossaryTerms with a do-while) and replace it with the same infinite-scroll pattern the normal term table already uses: - "Expand All" now calls fetchAllTerms() instead of fetchExpadedTree() - After each page of first-level terms loads, rows with children are auto-expanded and their children lazy-loaded via fetchChildTerms — exactly the same code path as a manual row expand - Infinite scroll continues to work in expand-all mode (removed the !toggleExpandBtn guard from both the MutationObserver and scroll listener) so pages load and auto-expand as the user scrolls - Switching glossaries while in expand-all mode now resets the mode and clears expanded keys, preventing stale state bleed-through - Removes fetchExpadedTree, expandTreeSeqRef, and the unused getGlossaryTerms / buildTree / TabSpecificField imports Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| termsToExpand.forEach((term) => { | ||
| if ( | ||
| (!term.children || term.children.length === 0) && | ||
| (term.childrenCount ?? 0) > 0 | ||
| ) { | ||
| // eslint-disable-next-line openmetadata-imports/no-api-calls-in-iteration | ||
| fetchChildTerms(term.fullyQualifiedName ?? ''); | ||
| } | ||
| }); |
There was a problem hiding this comment.
⚠️ Bug: Concurrent expand-all child fetches clobber each other
The new expand-all path fires fetchChildTerms for every expandable top-level term in a single forEach (lines 421-429), so multiple calls run concurrently. fetchChildTerms captures glossaryChildTerms from its render closure and commits with the non-functional setGlossaryChildTerms(updatedTerms) (line 269-270), while the store setter only accepts a plain array (no functional updater). Each concurrent resolution therefore overwrites the store based on a stale snapshot that lacks the siblings' just-added children, so only the last-resolving parent keeps its children — expand-all visibly fails to expand most branches on any glossary with several expandable top-level terms. The same stale-closure path is unguarded on glossary switch: a lingering child fetch from glossary A can resolve after navigation and re-commit A's term tree over B. Read the freshest store state before computing the update (as the loadMore branch already does via useGlossaryStore.getState()).
Read the latest committed store state before applying the child-term update, replacing the stale closure read of glossaryChildTerms.:
// Recursive function to update nested terms
const updateNestedTerms = (
terms: ModifiedGlossary[]
): ModifiedGlossary[] => { /* unchanged */ };
// Read the freshest committed terms so concurrent child fetches (e.g.
// expand-all) build on each other instead of clobbering, and a fetch
// that resolves after a glossary switch becomes a no-op.
const currentTerms = useGlossaryStore.getState().glossaryChildTerms;
if (!Array.isArray(currentTerms)) {
return;
}
const updatedTerms = updateNestedTerms(currentTerms);
setGlossaryChildTerms(updatedTerms);
Was this helpful? React with 👍 / 👎
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source
| (term.childrenCount ?? 0) > 0 | ||
| ) { | ||
| // eslint-disable-next-line openmetadata-imports/no-api-calls-in-iteration | ||
| fetchChildTerms(term.fullyQualifiedName ?? ''); |
There was a problem hiding this comment.
When a user starts Expand All for glossary A and switches to glossary B while one of A's lazy child requests is pending, the unguarded child response updates the shared term tree after B loads, replacing B's displayed terms with A's hierarchy.
Knowledge Base Used: Governance and collaboration experience
Pull request was closed
|
1 participant
Fixes #29916
Summary
API_RES_MAX_SIZE(100,000) usages inGlossaryTermTab.component.tsxwithdo…whilecursor-based pagination usingPAGE_SIZE_LARGE(50), so the expand-all and task-loading paths no longer issue a single massive request.API_RES_MAX_SIZEimport.fetchExpadedTreeintry/catch/finallyso a mid-loop network error cannot leave the loading spinner permanently stuck.expandTreeSeqRef,fetchTasksSeqRef) — the same patternfetchAllTermsalready uses withfetchRequestSeqRef— to guard against two concurrency races:finallyblock no longer clears loading state owned by a newer request.eslint-disable-next-line openmetadata-imports/no-api-calls-in-iteration— the rule targets N+1-per-item patterns; cursor pagination is O(pages), not O(items).Changed files
openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsxTest plan
yarn ui-checkstyle:changed— 0 errors ✅🤖 Generated with Claude Code
Greptile Summary
The PR replaces oversized glossary-term and task requests with incremental pagination and adds stale-request sequencing. It also changes Expand All to use the normal paginated table and lazy child loading.
Confidence Score: 3/5
The PR is not yet safe to merge because pending expansion work can still update the term table after the user switches glossaries.
Expand All now launches unguarded child requests whose late responses can overwrite the newly selected glossary, and the previously reported isLoadingMore-gated switch invalidation remains outstanding.
Files Needing Attention: openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx
Important Files Changed
Sequence Diagram
sequenceDiagram participant U as User participant T as GlossaryTermTab participant API as Glossary API U->>T: Expand All on glossary A T->>API: Fetch first-level page API-->>T: Expandable terms loop Each unloaded expandable term T->>API: Fetch child terms end U->>T: Switch to glossary B T->>API: Fetch B first-level terms API-->>T: B terms API-->>T: Late A child terms T->>T: Unguarded child-state update Note over T: B table can be replaced by A hierarchyReviews (4): Last reviewed commit: "fix(glossary): replace eager expand-all ..." | Re-trigger Greptile
Context used: