Skip to content

Improve glossary terms loading and expand/collapse behaviour - #31886

Closed
anuj-kumary wants to merge 7 commits into
mainfrom
issue-29916
Closed

Improve glossary terms loading and expand/collapse behaviour#31886
anuj-kumary wants to merge 7 commits into
mainfrom
issue-29916

Conversation

@anuj-kumary

@anuj-kumary anuj-kumary commented Aug 21, 2026

Copy link
Copy Markdown
Member

What & Why

Fixes #29916

On the glossary page, the terms list previously requested the entire set of terms in one go. On large glossaries this was slow and wasteful. This PR loads terms in pages as the user scrolls, and cleans up a few related glitches in the terms table.

Changes

  • Terms now load in pages as you scroll down the list, instead of fetching everything up front. This keeps the page fast even for glossaries with a large number of terms.
  • Expand all and the approval tasks it depends on are also fetched in pages rather than in a single oversized request.
  • Fixed: scrolling stopped loading more terms after using Expand / Collapse all — it now keeps loading as expected.
  • Fixed: switching between glossaries now reliably resets the list back to the top-level, scrollable view.

Testing

  • Verified on a glossary with 120 terms: the list loads 50 at a time and continues to load the rest on scroll, with no duplicates.
  • Verified Expand all still expands the full term hierarchy, and scrolling keeps working afterwards.
  • Existing unit tests updated and passing.

Greptile Summary

The PR changes glossary terms, Expand All, and approval tasks to use paginated requests while adding request-context guards around shared UI state.

  • Adds paginated loading for glossary terms and approval tasks.
  • Resets and reloads the term view when switching glossaries, including during search.
  • Coordinates ordinary and Expand All requests through a shared sequence, but the winning Expand All cleanup can leave the superseded load-more flag set.

Confidence Score: 4/5

The PR is not yet safe to merge because Expand All can permanently disable scrolling and later glossary reloads when it supersedes an active load-more request.

The shared request sequence suppresses the load-more request's cleanup, while the winning Expand All cleanup does not clear isLoadingMore, leaving the component stuck in a loading state.

Files Needing Attention: openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx

Important Files Changed

Filename Overview
openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx Adds paginated fetching and stale-response coordination, but Expand All can strand isLoadingMore when it supersedes an active scroll request.
openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.test.tsx Updates task-fetch expectations for glossary-scoped pagination but does not cover the overlapping load-more and Expand All lifecycle.

Sequence Diagram

sequenceDiagram
  participant U as User
  participant L as Load More
  participant E as Expand All
  participant S as Component State
  U->>L: Scroll
  L->>S: "isLoadingMore = true, sequence = N"
  U->>E: Click Expand All
  E->>S: "sequence = N+1"
  L-->>S: Cleanup rejected as stale
  E-->>S: Clear table/expand loading
  Note over S: isLoadingMore remains true
Loading

Reviews (6): Last reviewed commit: "fix: reload terms on glossary switch eve..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

Load glossary terms in pages instead of requesting the full list at once,
and keep the collapsed-view infinite scroll working after using Expand/
Collapse all.

Fixes #29916

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the UI UI specific issues label Aug 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@anuj-kumary anuj-kumary self-assigned this Aug 21, 2026
@anuj-kumary anuj-kumary added the safe to test Add this label to run secure Github workflows on PRs label Aug 21, 2026
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

⚠️ UI Checkstyle passed — lint findings in changed files

🔍 ESLint findings in this PR's files — 0 error(s), 36 warning(s)

Errors block the build. Warnings do not yet — they are rules whose backlog is still
being worked down, listed so this PR does not add to it. See docs/ui-code-quality-gate.md.

0 error(s), 36 warning(s) across 2 changed file(s).

Count Rule
15 react-hooks/exhaustive-deps
4 sonarjs/expression-complexity
3 sonarjs/cyclomatic-complexity
3 sonarjs/no-duplicate-string
2 sonarjs/cognitive-complexity
2 openmetadata-imports/no-api-calls-in-iteration
2 jsx-a11y/click-events-have-key-events
2 jsx-a11y/no-static-element-interactions
1 openmetadata-imports/review-sequential-api-calls
1 react/no-unstable-nested-components
All findings
Location Rule Message
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:137:75 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 17 which is greater than 10 authorized.","cost":7,"secondaryLocations":[{"line":137,"column":74,"endLine":137,"endColum
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:174:6 react-hooks/exhaustive-deps React Hook useMemo has an unnecessary dependency: 'findExpandableKeysForArray'. Either exclude it or remove the dependency array. Outer scope values like 'findE
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:228:34 react-hooks/exhaustive-deps React Hook useCallback received a function whose dependencies are unknown. Pass an inline function instead.
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:232:9 react-hooks/exhaustive-deps The 'fetchChildTerms' function makes the dependencies of useCallback Hook (at line 880) change on every render. To fix this, wrap the definition of 'fetchChildT
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:303:9 react-hooks/exhaustive-deps The 'fetchAllTerms' function makes the dependencies of useEffect Hook (at line 657) change on every render. To fix this, wrap the definition of 'fetchAllTerms'
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:303:50 sonarjs/cognitive-complexity Refactor this function to reduce its Cognitive Complexity from 23 to the 15 allowed.
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:303:50 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 21 which is greater than 10 authorized.","cost":11,"secondaryLocations":[{"line":303,"column":49,"endLine":303,"endColu
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:371:31 openmetadata-imports/review-sequential-api-calls Review these sequential API requests. If they are independent, start them together with Promise.all/Promise.allSettled; keep sequencing only when data-dependent
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:448:40 openmetadata-imports/no-api-calls-in-iteration Avoid issuing one API request per item. Fetch at the data owner, use a bulk endpoint, or use useQueries with an intentional concurrency policy.
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:509:40 openmetadata-imports/no-api-calls-in-iteration Avoid issuing one API request per item. Fetch at the data owner, use a bulk endpoint, or use useQueries with an intentional concurrency policy.
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:577:6 react-hooks/exhaustive-deps React Hook useEffect has missing dependencies: 'fetchAllTerms', 'handlePagingChange', and 'setGlossaryChildTerms'. Either include them or remove the dependency
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:584:6 react-hooks/exhaustive-deps React Hook useEffect has a missing dependency: 'setGlossaryChildTerms'. Either include it or remove the dependency array.
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:631:9 sonarjs/expression-complexity Reduce the number of conditional operators (4) used in the expression (maximum allowed 3).
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:678:9 sonarjs/expression-complexity Reduce the number of conditional operators (4) used in the expression (maximum allowed 3).
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:706:6 react-hooks/exhaustive-deps React Hook useEffect has a missing dependency: 'toggleExpandBtn'. Either include it or remove the dependency array.
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:776:7 sonarjs/cognitive-complexity Refactor this function to reduce its Cognitive Complexity from 19 to the 15 allowed.
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:776:7 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 12 which is greater than 10 authorized.","cost":2,"secondaryLocations":[{"line":776,"column":6,"endLine":776,"endColumn
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:852:5 react-hooks/exhaustive-deps React Hook useCallback has missing dependencies: 'setGlossaryChildTerms', 't', and 'updateGlossaryTermStatus'. Either include them or remove the dependency arra
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1111:31 sonarjs/no-duplicate-string Define a constant instead of duplicating this literal 4 times.
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1148:6 react-hooks/exhaustive-deps React Hook useMemo has missing dependencies: 'currentUser', 'onAddGlossaryTerm', 'onEditGlossaryTerm', and 't'. Either include them or remove the dependency arr
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1217:6 react-hooks/exhaustive-deps React Hook useCallback has missing dependencies: 'fetchAllTerms' and 'fetchExpadedTree'. Either include them or remove the dependency array. Outer scope values
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1291:5 react-hooks/exhaustive-deps React Hook useMemo has missing dependencies: 'handleCheckboxChange' and 't'. Either include them or remove the dependency array.
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1385:6 react-hooks/exhaustive-deps React Hook useMemo has missing dependencies: 'handleEditGlossary', 'handleSearchChange', 'permissions.EditAll', and 't'. Either include them or remove the depen
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1422:19 react/no-unstable-nested-components Do not define components during render. React will see a new component type on every render and destroy the entire subtree’s DOM nodes and state (https://reactj
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1429:31 sonarjs/no-duplicate-string Define a constant instead of duplicating this literal 3 times.
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1430:29 sonarjs/no-duplicate-string Define a constant instead of duplicating this literal 3 times.
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1454:16 sonarjs/expression-complexity Reduce the number of conditional operators (4) used in the expression (maximum allowed 3).
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1509:11 sonarjs/expression-complexity Reduce the number of conditional operators (4) used in the expression (maximum allowed 3).
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1514:5 react-hooks/exhaustive-deps React Hook useMemo has unnecessary dependencies: 'glossaryChildTerms', 'glossaryTerms', and 'setGlossaryChildTerms'. Either exclude them or remove the dependenc
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1587:6 react-hooks/exhaustive-deps React Hook useMemo has an unnecessary dependency: 'movedGlossaryTerm'. Either exclude it or remove the dependency array.
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:1796:6 react-hooks/exhaustive-deps React Hook useEffect has missing dependencies: 'activeGlossary', 'fetchAllTerms', and 'previousGlossaryFQN'. Either include them or remove the dependency array.
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.test.tsx:141:7 jsx-a11y/click-events-have-key-events Visible, non-interactive elements with click handlers must have at least one keyboard listener.
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.test.tsx:141:7 jsx-a11y/no-static-element-interactions Avoid non-native interactive elements. If using native HTML is not possible, add an appropriate role and support for tabbing, mouse, keyboard, and touch inputs
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.test.tsx:151:9 jsx-a11y/click-events-have-key-events Visible, non-interactive elements with click handlers must have at least one keyboard listener.
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.test.tsx:151:9 jsx-a11y/no-static-element-interactions Avoid non-native interactive elements. If using native HTML is not possible, add an appropriate role and support for tabbing, mouse, keyboard, and touch inputs
🟡 src/components/Glossary/GlossaryTermTab/GlossaryTermTab.test.tsx:261:63 react/no-array-index-key Do not use Array index in keys

Fix locally (fast - only checks files changed in this branch):

make ui-checkstyle-changed

…nating pagination

- Discard in-flight term/task responses when the glossary has changed so a
  previous glossary's data can no longer repopulate the table.
- Stop the pagination loops on no forward progress (empty page or a cursor
  that does not advance) instead of relying on the server nulling the token.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… state

Move the stale-context guard in fetchAllTerms ahead of the search-offset and
continuation-cursor updates so a response for a now-inactive search term,
status filter, or glossary can no longer advance the pagination state that the
active glossary's scrolling depends on.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 66%
67.04% (80413/119946) 51.47% (49265/95702) 52.44% (14700/28028)

@sonarqubecloud

Copy link
Copy Markdown

fetchExpadedTree now shares the fetchRequestSeqRef sequence used by
fetchAllTerms, so a superseded expand-all applies no rows and clears no
loading flags — a stale expand-all can no longer release a newer operation's
loading state. fetchAllTerms also honours the sequence before applying rows so
a stale first-level load cannot clobber a newer expand-all, and a fresh
first-level fetch clears the expand indicator it supersedes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The status-filtered empty-page path issues a second request (the total count)
after the initial staleness guard. Extract the staleness check into a reusable
isSuperseded() helper and re-run it after that count request so a response that
went stale mid-flight cannot overwrite the active context's term list, count,
or expanded rows.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The glossary-switch effect skipped fetching when a search term was set, and the
search effect depends only on search term/status (not the glossary), so
switching glossaries mid-search left the new glossary empty or showing the
previous one's state. Drop the search-term guard so a glossary change always
reloads; fetchAllTerms re-runs the search scoped to the newly active glossary.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@gitar-bot

gitar-bot Bot commented Aug 21, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 1 resolved / 1 findings

Paginated glossary term loading and added stale-response guards to fix scroll behavior, addressing the non-terminating pagination finding.

✅ 1 resolved
Edge Case: Pagination loops rely solely on after token to terminate

📄 openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:421-434 📄 openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx:463-475
In fetchExpadedTree and fetchAllTasks the do...while (after) loops only stop when paging.after is falsy. If a backend page ever returns a non-null after while yielding empty/no new data, the loop keeps issuing identical requests indefinitely, hanging the tab with the loading spinner stuck. Add a defensive guard to break when data.length === 0 (or when the returned after equals the previous cursor) so termination doesn't depend entirely on the server nulling the token.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

Comment on lines +492 to +494
if (requestSeq === fetchRequestSeqRef.current) {
setIsTableLoading(false);
setIsExpandingAll(false);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Expand All strands loading state

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 isLoadingMore. The flag remains true, permanently disabling subsequent infinite scrolling and preventing glossary switches from reloading their terms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove high limit usage of API_RES_MAX_SIZE in GlossaryTermTab

1 participant