Skip to content

remove tree in mui with untitled ui#29911

Open
shrabantipaul-collate wants to merge 21 commits into
mainfrom
tree-select-untitled-ui
Open

remove tree in mui with untitled ui#29911
shrabantipaul-collate wants to merge 21 commits into
mainfrom
tree-select-untitled-ui

Conversation

@shrabantipaul-collate

@shrabantipaul-collate shrabantipaul-collate commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Describe your changes:

Fixes #
image

Uploading Screen Recording 2026-07-14 at 3.46.38 PM.mov…

I worked on ... because ...

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

High-level design:

N/A — small change.

Tests:

Use cases covered

Unit tests

Backend integration tests

Ingestion integration tests

Playwright (UI) tests

Manual testing performed

UI screen recording / screenshots:

Not applicable.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.
  • For UI changes: I attached a screen recording and/or screenshots above.
  • I have added tests (unit / integration / Playwright as applicable) and listed them above.

Summary by Gitar

  • Refactored Tree components:
    • Replaced MUIAsyncTreeSelect and associated legacy atoms/hooks with a new TreeSelect component in openmetadata-ui-core-components.
    • Renamed and migrated MUIGlossaryTagSuggestion to GlossaryTermTreeSelect and MUIDomainSelect to DomainTreeSelect.
  • New shared components:
    • Added TreeSelect implementation including data fetching, search, and selection logic via new hooks useTreeSelectData, useTreeSelectSearch, and useTreeSelectSelection.
  • UI/UX improvements:
    • Added TreeSelect.stories.tsx for documentation and testing of the new component.
    • Updated AddDomainForm to utilize the new GlossaryTermTreeSelect.

This will update automatically on new commits.

Greptile Summary

This PR replaces the legacy MUI tree select with the new shared TreeSelect. The main changes are:

  • Added a reusable TreeSelect component and hooks in ui-core-components.
  • Migrated domain and glossary term selectors to the new component.
  • Passed abort signals through domain and glossary REST calls.
  • Updated glossary term ids to prefer fully qualified names.
  • Updated related tests and Storybook coverage.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
openmetadata-ui-core-components/src/main/resources/ui/src/components/application/tree-select/tree-select.tsx Adds the shared TreeSelect component with controlled value syncing, search input, selection display, and lazy expansion support.
openmetadata-ui-core-components/src/main/resources/ui/src/components/application/tree-select/use-tree-select-data.ts Adds async tree loading with abort handling, loading state tracking, per-node loading, and child caching.
openmetadata-ui/src/main/resources/ui/src/components/common/GlossaryTermTreeSelect/GlossaryTermTreeSelect.tsx Adds the glossary term wrapper around TreeSelect with server-side search, glossary expansion, and TagLabel conversion.
openmetadata-ui/src/main/resources/ui/src/components/common/GlossaryTermTreeSelect/GlossaryTagSuggestionUtils.ts Converts glossary API results into TreeSelect nodes and keys term nodes by fully qualified name first.
openmetadata-ui/src/main/resources/ui/src/components/common/DomainTreeSelect/DomainTreeSelect.tsx Adds the domain wrapper around TreeSelect with hierarchy loading, search, and EntityReference conversion.

Comments Outside Diff (12)

  1. openmetadata-ui/src/main/resources/ui/src/components/common/GlossaryTermTreeSelect/GlossaryTagSuggestionUtils.ts, line 61-63 (link)

    P1 Glossary Term Ids Diverge

    Loaded term nodes use the bare term name as id, but preselected values in GlossaryTermTreeSelect use tag.tagFQN. A saved tag such as PII.Email is seeded under PII.Email while the visible node is keyed as Email, so it does not show as selected and clicking it can add a duplicate tag instead of toggling the saved one.

  2. openmetadata-ui/src/main/resources/ui/src/components/common/GlossaryTermTreeSelect/GlossaryTermTreeSelect.tsx, line 235 (link)

    P1 Rendered Labels Are Dropped

    The shared formUtils path passes a rendered muiLabel node, but this branch forwards undefined unless the label is a string. Dynamic glossary-tag fields can lose their visible label and required marker even though the form still treats the field as required.

  3. openmetadata-ui/src/main/resources/ui/src/components/common/GlossaryTermTreeSelect/GlossaryTagSuggestionUtils.ts, line 65 (link)

    P1 Term Ids Still Diverge Saved glossary selections are seeded with tag.tagFQN, but fetched term nodes are still keyed by the bare term name first. For a saved tag such as PII.Email, the selected value is stored under PII.Email while the visible expanded node is keyed as Email, so the checkbox does not show as selected and clicking the node can add a duplicate instead of toggling the saved tag.

    Context Used: CLAUDE.md (source)

  4. openmetadata-ui/src/main/resources/ui/src/components/common/GlossaryTermTreeSelect/GlossaryTagSuggestionUtils.ts, line 65 (link)

    P1 Term Ids Still Diverge

    Fetched glossary terms are still keyed by the short name before the fully qualified name, while controlled selections are seeded with id: tag.tagFQN. For a nested term like PII.Email, the saved value is tracked under PII.Email but the expanded/search result node is tracked under Email, so the checkbox or radio can appear unselected and clicking the visible node can add a duplicate instead of toggling the saved tag.

    Context Used: CLAUDE.md (source)

  5. openmetadata-ui/src/main/resources/ui/src/components/common/GlossaryTermTreeSelect/GlossaryTagSuggestionUtils.ts, line 65-66 (link)

    P1 Term Ids Diverge Fetched glossary terms are still keyed by the short name, while controlled selections are seeded with id: tag.tagFQN. For a nested term such as PII.Email, the saved value is tracked under PII.Email but the expanded node is tracked under Email, so the control does not show the saved term as selected and clicking it can add a duplicate instead of toggling it.

  6. openmetadata-ui/src/main/resources/ui/src/components/common/GlossaryTermTreeSelect/GlossaryTagSuggestionUtils.ts, line 65 (link)

    P1 Term ids still diverge

    Fetched term nodes still use the short option.name as their id whenever it exists, but controlled selections are seeded with id: tag.tagFQN. For a nested term such as PII.Email, the visible expanded/search result node is keyed as Email while the saved selected node is keyed as PII.Email. The id-based selection map will not mark the visible node as selected, and clicking it can add or toggle a separate entry instead of the saved tag.

    Context Used: CLAUDE.md (source)

  7. openmetadata-ui/src/main/resources/ui/src/components/common/GlossaryTermTreeSelect/GlossaryTagSuggestionUtils.ts, line 65 (link)

    P1 Term ids diverge

    Fetched glossary term nodes still use the short name as id, but controlled selections are seeded with id: tag.tagFQN. TreeSelect checks selection by node.id, so a saved nested term like PII.Email is stored under PII.Email while the rendered node is keyed as Email. The term can appear unselected, and clicking it can add or toggle a separate entry instead of the saved tag.

    Context Used: CLAUDE.md (source)

  8. openmetadata-ui/src/main/resources/ui/src/components/common/GlossaryTermTreeSelect/GlossaryTagSuggestionUtils.ts, line 65 (link)

    P1 Term ids diverge

    Loaded glossary term nodes are still keyed by the local name before the fully qualified name, while controlled selections are reconstructed with id: tag.tagFQN. When a saved nested term like PII.Email is loaded, the selected map is seeded under PII.Email, but the expanded node is keyed as Email. The checkbox or radio can appear unchecked, and clicking the visible node can add or toggle a separate entry instead of the saved tag.

    Context Used: CLAUDE.md (source)

  9. openmetadata-ui/src/main/resources/ui/src/components/common/GlossaryTermTreeSelect/GlossaryTagSuggestionUtils.ts, line 65 (link)

    P1 Term ids diverge

    Fetched glossary term nodes still prefer the short option.name as their id, while controlled selections are rebuilt with id: tag.tagFQN. When a saved nested term such as PII.Email is rendered after expanding or searching, the visible node is keyed as Email but the selected value is keyed as PII.Email. The checkbox can appear unchecked, and clicking the node can add or toggle a separate entry instead of the saved tag.

    Context Used: CLAUDE.md (source)

  10. openmetadata-ui/src/main/resources/ui/src/components/common/GlossaryTermTreeSelect/GlossaryTagSuggestionUtils.ts, line 65 (link)

    P1 Term ids diverge

    Fetched term nodes still use the short name as their id whenever it exists, while controlled selections are rebuilt with id: tag.tagFQN. TreeSelect checks selection by node id, so a saved nested term like PII.Email is stored under PII.Email but the expanded or searched node is keyed as Email. The checkbox can appear unchecked, and clicking the visible node can add or toggle a separate entry instead of the saved tag.

    Context Used: CLAUDE.md (source)

  11. openmetadata-ui/src/main/resources/ui/src/components/common/GlossaryTermTreeSelect/GlossaryTagSuggestionUtils.ts, line 65 (link)

    P1 Term Ids Diverge Fetched term nodes still prefer the short option.name as their id, while controlled selections are seeded with id: tag.tagFQN. For a nested term such as PII.Email, the rendered node is keyed as Email while the saved value is keyed as PII.Email. Since selection is checked by node id, the checkbox or radio can appear unselected, and clicking the visible term can add or toggle a separate entry instead of the saved tag.

    Context Used: CLAUDE.md (source)

  12. openmetadata-ui/src/main/resources/ui/src/components/common/GlossaryTermTreeSelect/GlossaryTagSuggestionUtils.ts, line 65 (link)

    P1 Term ids still diverge

    Fetched term nodes are still keyed by the short option.name, while controlled selections are rebuilt with id: tag.tagFQN. For a nested term like PII.Email, the expanded or searched node is keyed as Email but the saved selected value is keyed as PII.Email. The visible term can appear unselected, and clicking it can add or toggle a separate entry instead of the saved tag.

    Context Used: CLAUDE.md (source)

Reviews (19): Last reviewed commit: "fix gitar bot comment" | Re-trigger Greptile

Context used (3)

  • Context used - CLAUDE.md (source)
  • Context used - openmetadata-ui-core-components/CLAUDE.md (source)
  • Context used - AGENTS.md (source)

@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!

@Rohit0301 Rohit0301 added the safe to test Add this label to run secure Github workflows on PRs label Jul 10, 2026
@Rohit0301 Rohit0301 marked this pull request as ready for review July 10, 2026 12:08
@Rohit0301 Rohit0301 requested review from a team, chirag-madlani and karanh37 as code owners July 10, 2026 12:08
@github-actions

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

  • No GitHub issue is linked. Link an issue in the Development section of the PR (or add Fixes #12345 to the description). For a same-org cross-repo issue, add Fixes open-metadata/<repo>#123 to the description.

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 skip-pr-checks label.

@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 64%
64.82% (75461/116407) 48.66% (45025/92515) 49.37% (13596/27535)

@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

🔴 Playwright Results — 7 failure(s), 2 flaky

✅ 429 passed · ❌ 7 failed · 🟡 2 flaky · ⏭️ 16 skipped

Shard Passed Failed Flaky Skipped
🔴 Shard 1 418 7 2 16
✅ Shard 2 11 0 0 0

Genuine Failures (failed on all attempts)

Pages/AuditLogs.spec.ts › should create audit log entry when glossary is created (shard 1)
Error: Timed out waiting for entityCreated audit entry for glossary:AuditTest_Create_1784011111832

�[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32mtrue�[39m
Received: �[31mfalse�[39m

Call Log:
- Test timeout of 60000ms exceeded
Pages/AuditLogs.spec.ts › should create audit log entry when glossary is updated (shard 1)
Error: Timed out waiting for entityCreated audit entry for glossary:AuditTest_Update_1784011118380

�[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32mtrue�[39m
Received: �[31mfalse�[39m

Call Log:
- Test timeout of 60000ms exceeded
Pages/AuditLogs.spec.ts › should create audit log entry when glossary is soft deleted (shard 1)
Error: Timed out waiting for entityCreated audit entry for glossary:AuditTest_SoftDelete_1784011157763

�[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32mtrue�[39m
Received: �[31mfalse�[39m

Call Log:
- Test timeout of 60000ms exceeded
Pages/AuditLogs.spec.ts › should create audit log entry when glossary is restored (shard 1)
Error: Timed out waiting for entityCreated audit entry for glossary:AuditTest_Restore_1784011292940

�[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32mtrue�[39m
Received: �[31mfalse�[39m

Call Log:
- Test timeout of 60000ms exceeded
Pages/AuditLogs.spec.ts › should create audit log entry when glossary is hard deleted (shard 1)
Error: Timed out waiting for entityCreated audit entry for glossary:AuditTest_HardDelete_1784011301396

�[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32mtrue�[39m
Received: �[31mfalse�[39m

Call Log:
- Test timeout of 60000ms exceeded
Pages/AuditLogs.spec.ts › should verify complete audit trail for entity lifecycle (shard 1)
Error: Timed out waiting for entityCreated audit entry for glossary:AuditTest_FullLifecycle_1784011340471

�[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32mtrue�[39m
Received: �[31mfalse�[39m

Call Log:
- Test timeout of 60000ms exceeded
Pages/AuditLogs.spec.ts › should display audit log entry in UI after entity creation (shard 1)
Error: Timed out waiting for entityCreated audit entry for glossary:AuditTest_UI_1784011483386

�[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32mtrue�[39m
Received: �[31mfalse�[39m

Call Log:
- Test timeout of 60000ms exceeded
🟡 2 flaky test(s) (passed on retry)
  • Pages/AuditLogs.spec.ts › should handle audit logs access for non-admin users (shard 1, 1 retry)
  • Flow/SearchRBAC.spec.ts › a fully denied user sees neither asset type when browsing (shard 1, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@gitar-bot

gitar-bot Bot commented Jul 14, 2026

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

Replaced legacy MUI tree selectors with a unified Untitled UI TreeSelect component, resolving issues with async data fetching, selection state mismatches, and component re-renders. All previously identified bugs, including ID divergence in glossary terms and race conditions in data fetching, have been addressed.

✅ 6 resolved
Bug: AbortController never wired to fetchData; stale search results race

📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/application/tree-select/use-tree-select-data.ts:79-93
In useTreeSelectData.fetchTreeData, an AbortController is created and the previous one is aborted on each call (lines 81-82), but its signal is never passed to fetchData (line 98: fetchData({ ...params, pageSize })). TreeSelectDataFetcherParams also has no signal field. As a result, aborting has no effect on in-flight requests — the prior request is not cancelled and still resolves, calling setState and overwriting fresher data.

Concretely, typing a search 'a' then 'ab' fires two concurrent fetches. If the slower 'a' request resolves after the 'ab' request, the tree ends up showing results for 'a' while the input reads 'ab'. The catch-block guard if (abortControllerRef.current?.signal.aborted) (line 133) also never helps here because abortControllerRef.current always points to the newest controller, not the one belonging to the settled request, and the success path has no such guard at all.

Suggested fix: thread the controller's signal through to fetchData (add signal to TreeSelectDataFetcherParams and pass it), and/or capture the controller in a local variable and ignore the result if controller.signal.aborted in both the success and error branches.

Bug: TreeSelect ignores external value prop changes after first mount

📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/application/tree-select/tree-select.tsx:140-148
In tree-select.tsx, the effect that seeds selection from the value prop is guarded by seededRef so it runs only once (lines 140-148):

if (!seededRef.current) {
  const initial = toArray(value);
  if (initial.length > 0) setSelection(initial);
  seededRef.current = true;
}

After the first render the internal selectedNodes map becomes the sole source of truth. If the parent updates value without going through onChange — e.g. react-hook-form reset, asynchronously-loaded initial data when editing an existing Domain/Data Product, or clearing the field programmatically — the displayed selection will not update. TreeSelect is consumed as a controlled component in DomainTreeSelect/GlossaryTermTreeSelect (via formUtils / AddDomainForm), so edit/prefill flows can render stale selections.

Also note the initial seed is skipped entirely when initial.length === 0, so transitioning from a non-empty value to an empty one is never reflected. Consider syncing selection whenever value changes (comparing against current selection to avoid loops) rather than seeding once.

Performance: isNodeSelected is a new function each render, defeating renderNodes memo

📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/application/tree-select/use-tree-select-selection.ts:150-157 📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/application/tree-select/tree-select.tsx:202-216
useTreeSelectSelection returns isNodeSelected as an inline arrow function (isNodeSelected: (nodeId) => selectedNodes.has(nodeId)), so a fresh function identity is produced on every render. In tree-select.tsx, isNodeSelected is a dependency of the renderNodes useCallback (line 229), which therefore is re-created on every render, causing the entire tree to re-render even when nothing relevant changed. Wrap isNodeSelected in useCallback (depending on selectedNodes) so the memoization of renderNodes is effective.

Bug: Aborted root fetch can clear loading of a newer in-flight search

📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/application/tree-select/use-tree-select-data.ts:105-116 📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/application/tree-select/use-tree-select-data.ts:150-162
The new else branches (lines 111-113 and 157-159) set loading: false whenever an aborted fetch without a parentId (i.e. a root/search fetch) resolves. This is correct when the fetch was superseded by a lazy child load, but it introduces a regression when one root/search fetch supersedes another.

Scenario: the user types quickly so searchTerm changes twice. Fetch A (root, loading: true, controller A) is aborted by Fetch B (root), which re-sets loading: true for its own request. When the aborted Fetch A later resolves, params.parentId is undefined so it falls into the new else and calls setState({ loading: false }) — even though Fetch B is still in flight and the spinner should remain visible. The result is a transient loss of the loading indicator (spinner flicker) until Fetch B resolves and restores the correct state. Network reordering (A resolving after B) makes the window larger.

The state eventually converges, so this is a transient UI glitch rather than a stuck state, hence minor. The fix should only clear loading in the aborted branch when the superseding fetch is not another root/search fetch (e.g. track whether the currently active in-flight fetch is a root fetch and skip the reset if so).

Bug: Duplicate InlineAlertProps import breaks compilation

📄 openmetadata-ui/src/main/resources/ui/src/utils/formUtils.tsx:54 📄 openmetadata-ui/src/main/resources/ui/src/utils/formUtils.tsx:57
InlineAlertProps is imported twice from the identical path ../components/common/InlineAlert/InlineAlert.interface — once on line 54 and again on line 57 (added by this commit). This is a duplicate identifier, which fails TypeScript compilation (TS2300: Duplicate identifier 'InlineAlertProps') and violates the no-duplicate-imports ESLint rule, so the build/checkstyle will not pass. Remove the redundant line 57 import.

...and 1 more resolved from earlier reviews

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 👍 / 👎 | Gitar

1 similar comment
@gitar-bot

gitar-bot Bot commented Jul 14, 2026

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

Replaced legacy MUI tree selectors with a unified Untitled UI TreeSelect component, resolving issues with async data fetching, selection state mismatches, and component re-renders. All previously identified bugs, including ID divergence in glossary terms and race conditions in data fetching, have been addressed.

✅ 6 resolved
Bug: AbortController never wired to fetchData; stale search results race

📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/application/tree-select/use-tree-select-data.ts:79-93
In useTreeSelectData.fetchTreeData, an AbortController is created and the previous one is aborted on each call (lines 81-82), but its signal is never passed to fetchData (line 98: fetchData({ ...params, pageSize })). TreeSelectDataFetcherParams also has no signal field. As a result, aborting has no effect on in-flight requests — the prior request is not cancelled and still resolves, calling setState and overwriting fresher data.

Concretely, typing a search 'a' then 'ab' fires two concurrent fetches. If the slower 'a' request resolves after the 'ab' request, the tree ends up showing results for 'a' while the input reads 'ab'. The catch-block guard if (abortControllerRef.current?.signal.aborted) (line 133) also never helps here because abortControllerRef.current always points to the newest controller, not the one belonging to the settled request, and the success path has no such guard at all.

Suggested fix: thread the controller's signal through to fetchData (add signal to TreeSelectDataFetcherParams and pass it), and/or capture the controller in a local variable and ignore the result if controller.signal.aborted in both the success and error branches.

Bug: TreeSelect ignores external value prop changes after first mount

📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/application/tree-select/tree-select.tsx:140-148
In tree-select.tsx, the effect that seeds selection from the value prop is guarded by seededRef so it runs only once (lines 140-148):

if (!seededRef.current) {
  const initial = toArray(value);
  if (initial.length > 0) setSelection(initial);
  seededRef.current = true;
}

After the first render the internal selectedNodes map becomes the sole source of truth. If the parent updates value without going through onChange — e.g. react-hook-form reset, asynchronously-loaded initial data when editing an existing Domain/Data Product, or clearing the field programmatically — the displayed selection will not update. TreeSelect is consumed as a controlled component in DomainTreeSelect/GlossaryTermTreeSelect (via formUtils / AddDomainForm), so edit/prefill flows can render stale selections.

Also note the initial seed is skipped entirely when initial.length === 0, so transitioning from a non-empty value to an empty one is never reflected. Consider syncing selection whenever value changes (comparing against current selection to avoid loops) rather than seeding once.

Performance: isNodeSelected is a new function each render, defeating renderNodes memo

📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/application/tree-select/use-tree-select-selection.ts:150-157 📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/application/tree-select/tree-select.tsx:202-216
useTreeSelectSelection returns isNodeSelected as an inline arrow function (isNodeSelected: (nodeId) => selectedNodes.has(nodeId)), so a fresh function identity is produced on every render. In tree-select.tsx, isNodeSelected is a dependency of the renderNodes useCallback (line 229), which therefore is re-created on every render, causing the entire tree to re-render even when nothing relevant changed. Wrap isNodeSelected in useCallback (depending on selectedNodes) so the memoization of renderNodes is effective.

Bug: Aborted root fetch can clear loading of a newer in-flight search

📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/application/tree-select/use-tree-select-data.ts:105-116 📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/application/tree-select/use-tree-select-data.ts:150-162
The new else branches (lines 111-113 and 157-159) set loading: false whenever an aborted fetch without a parentId (i.e. a root/search fetch) resolves. This is correct when the fetch was superseded by a lazy child load, but it introduces a regression when one root/search fetch supersedes another.

Scenario: the user types quickly so searchTerm changes twice. Fetch A (root, loading: true, controller A) is aborted by Fetch B (root), which re-sets loading: true for its own request. When the aborted Fetch A later resolves, params.parentId is undefined so it falls into the new else and calls setState({ loading: false }) — even though Fetch B is still in flight and the spinner should remain visible. The result is a transient loss of the loading indicator (spinner flicker) until Fetch B resolves and restores the correct state. Network reordering (A resolving after B) makes the window larger.

The state eventually converges, so this is a transient UI glitch rather than a stuck state, hence minor. The fix should only clear loading in the aborted branch when the superseding fetch is not another root/search fetch (e.g. track whether the currently active in-flight fetch is a root fetch and skip the reset if so).

Bug: Duplicate InlineAlertProps import breaks compilation

📄 openmetadata-ui/src/main/resources/ui/src/utils/formUtils.tsx:54 📄 openmetadata-ui/src/main/resources/ui/src/utils/formUtils.tsx:57
InlineAlertProps is imported twice from the identical path ../components/common/InlineAlert/InlineAlert.interface — once on line 54 and again on line 57 (added by this commit). This is a duplicate identifier, which fails TypeScript compilation (TS2300: Duplicate identifier 'InlineAlertProps') and violates the no-duplicate-imports ESLint rule, so the build/checkstyle will not pass. Remove the redundant line 57 import.

...and 1 more resolved from earlier reviews

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 👍 / 👎 | Gitar

@sonarqubecloud

Copy link
Copy Markdown

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants