Skip to content

use same state accross app for nlq search#29165

Open
shrabantipaul-collate wants to merge 16 commits into
mainfrom
marketplace-nlq-search
Open

use same state accross app for nlq search#29165
shrabantipaul-collate wants to merge 16 commits into
mainfrom
marketplace-nlq-search

Conversation

@shrabantipaul-collate

@shrabantipaul-collate shrabantipaul-collate commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Describe your changes:

Problem:

The Data Marketplace search bar maintained its own local isNLPEnabled/isNLQActive state and made a redundant GET /api/v1/system/search/nlq call on every marketplace page load. NLQ active state was also siloed — toggling it on the explore or home page had no effect in the marketplace and vice versa.

Fix:

Replace local state and the useEffect API call in MarketplaceSearchBar with useSearchStore (the existing Zustand store already used by GlobalSearchBar and ExplorePageV1). isNLPEnabled is now populated once (when the global search bar mounts) and shared app-wide. isNLPActive toggled anywhere persists everywhere.

Tests:

Updated MarketplaceSearchBar.test.tsx to remove the getNLPEnabledStatus mock and seed NLQ state via useSearchStore.setState(...) instead, matching how the component actually reads it.
Fixes #

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.

Greptile Summary

This PR consolidates NLQ/NLP state into the existing useSearchStore Zustand store so that isNLPEnabled and isNLPActive are shared across the GlobalSearchBar, CustomiseSearchBar, and MarketplaceSearchBar — eliminating a redundant per-component API call and the siloed local state in the marketplace.

  • useSearchStore gains an isNLPInitialized guard and an initNLP() async action that replaces the previous setNLPEnabled action; each consumer now calls initNLP() on mount, and the guard short-circuits if the store is already hydrated.
  • MarketplaceSearchBar drops its local isNLPEnabled/isNLQActive state and useEffect API call, reading from the shared store instead; toggling NLQ in the marketplace now propagates app-wide.
  • Tests are updated to seed state via useSearchStore.setState and a new test verifies the cold-store bootstrap path (direct marketplace navigation before the global nav mounts).

Confidence Score: 5/5

Safe to merge — the change is a straightforward state consolidation with no new API surface and well-updated tests.

The core logic is a clean refactor: local component state and a redundant API call replaced by a shared Zustand store with a one-time-init guard. The guard has a theoretical concurrent-call edge case but the consequence is a single duplicate API request at startup — the same outcome as the code being replaced. Tests cover the cold-store bootstrap and skip-when-initialized paths. No data correctness or security concerns.

GlobalSearchBar.tsx and CustomiseSearchBar.tsx — initNLP is omitted from their useEffect dependency arrays, which may trigger a lint warning.

Important Files Changed

Filename Overview
openmetadata-ui/src/main/resources/ui/src/hooks/useSearchStore.ts Introduces isNLPInitialized flag and an initNLP async action; replaces setNLPEnabled. Guard is not atomic so concurrent callers can race past it, but the outcome is benign.
openmetadata-ui/src/main/resources/ui/src/components/DataMarketplace/MarketplaceSearchBar/MarketplaceSearchBar.component.tsx Replaces local NLP state and redundant API call with shared useSearchStore; correctly calls initNLP() on mount with [initNLP] in deps.
openmetadata-ui/src/main/resources/ui/src/components/GlobalSearchBar/GlobalSearchBar.tsx Simplifies NLP init to call initNLP() directly; initNLP is missing from the useEffect dependency array (potential lint violation).
openmetadata-ui/src/main/resources/ui/src/components/MyData/CustomizableComponents/CustomiseLandingPageHeader/CustomiseSearchBar.tsx Same simplification as GlobalSearchBar; same missing initNLP dep in the useEffect dependency array.
openmetadata-ui/src/main/resources/ui/src/components/DataMarketplace/MarketplaceSearchBar/MarketplaceSearchBar.test.tsx Tests updated to seed NLQ state via useSearchStore.setState; new test for cold-store bootstrap path added; getNLPEnabledStatus mock retained for the bootstrap test.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant GSB as GlobalSearchBar
    participant MSB as MarketplaceSearchBar
    participant Store as useSearchStore
    participant API as GET /api/v1/system/search/nlq

    Note over GSB,MSB: Normal flow (nav includes GlobalSearchBar)
    GSB->>Store: "initNLP() isNLPInitialized=false fetch"
    Store->>API: getNLPEnabledStatus()
    API-->>Store: true
    Store->>Store: "set isNLPEnabled=true, isNLPInitialized=true"
    MSB->>Store: "initNLP() isNLPInitialized=true skip"

    Note over MSB,Store: Direct deep-link to marketplace (no GSB)
    MSB->>Store: "initNLP() isNLPInitialized=false fetch"
    Store->>API: getNLPEnabledStatus()
    API-->>Store: true
    Store->>Store: "set isNLPEnabled=true, isNLPInitialized=true"

    Note over GSB,MSB: NLQ toggle shared state
    GSB->>Store: setNLPActive(true)
    Store-->>MSB: "isNLPActive=true re-render"
    MSB->>Store: setNLPActive(false)
    Store-->>GSB: "isNLPActive=false re-render"
Loading
%%{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 GSB as GlobalSearchBar
    participant MSB as MarketplaceSearchBar
    participant Store as useSearchStore
    participant API as GET /api/v1/system/search/nlq

    Note over GSB,MSB: Normal flow (nav includes GlobalSearchBar)
    GSB->>Store: "initNLP() isNLPInitialized=false fetch"
    Store->>API: getNLPEnabledStatus()
    API-->>Store: true
    Store->>Store: "set isNLPEnabled=true, isNLPInitialized=true"
    MSB->>Store: "initNLP() isNLPInitialized=true skip"

    Note over MSB,Store: Direct deep-link to marketplace (no GSB)
    MSB->>Store: "initNLP() isNLPInitialized=false fetch"
    Store->>API: getNLPEnabledStatus()
    API-->>Store: true
    Store->>Store: "set isNLPEnabled=true, isNLPInitialized=true"

    Note over GSB,MSB: NLQ toggle shared state
    GSB->>Store: setNLPActive(true)
    Store-->>MSB: "isNLPActive=true re-render"
    MSB->>Store: setNLPActive(false)
    Store-->>GSB: "isNLPActive=false re-render"
Loading

Reviews (9): Last reviewed commit: "fix lint issues" | Re-trigger Greptile

@shrabantipaul-collate shrabantipaul-collate requested a review from a team as a code owner June 18, 2026 07:33
@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

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!

@shrabantipaul-collate shrabantipaul-collate added the skip-pr-checks Bypass PR metadata validation check label Jun 18, 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!

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

@github-actions

github-actions Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 62%
62.28% (66610/106944) 44.04% (37262/84608) 45.39% (11190/24649)

Comment on lines +62 to +66
if (!isNLPEnabled) {
getNLPEnabledStatus()
.then(setNLPEnabled)
.catch(() => setNLPEnabled(false));
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a primitive in store that calls this directly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No. The fetch and set logic lives in the components themselves.

Comment thread openmetadata-ui/src/main/resources/ui/src/hooks/useSearchStore.ts
@gitar-bot

gitar-bot Bot commented Jun 18, 2026

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

Centralizes NLQ search state into a shared Zustand store to resolve sync issues, though the initNLP guard needs refinement to fully eliminate redundant API calls when NLP is disabled.

✅ 2 resolved
Bug: NLQ toggle won't render on marketplace pages (isNLPEnabled never populated)

📄 openmetadata-ui/src/main/resources/ui/src/components/DataMarketplace/MarketplaceSearchBar/MarketplaceSearchBar.component.tsx:46 📄 openmetadata-ui/src/main/resources/ui/src/components/DataMarketplace/MarketplaceSearchBar/MarketplaceSearchBar.component.tsx:52-54 📄 openmetadata-ui/src/main/resources/ui/src/components/DataMarketplace/MarketplaceSearchBar/MarketplaceSearchBar.component.tsx:283
MarketplaceSearchBar previously called getNLPEnabledStatus() in a useEffect on mount to populate its local isNLPEnabled state, which gates rendering of the NLQ toggle button ({isNLPEnabled ? (<button .../>) : ...} around line 283). This PR removes that effect and instead reads isNLPEnabled from useSearchStore.

However, useSearchStore.isNLPEnabled defaults to false and is only populated by GlobalSearchBar / CustomiseSearchBar when they mount and call getNLPEnabledStatus() (useSearchStore.ts:25-28; GlobalSearchBar.tsx:189-199). On Data Marketplace pages the GlobalSearchBar is explicitly NOT rendered — NavBar.tsx:497 renders it only when !isDataMarketplacePage. As a result, when a user navigates directly to (or refreshes on) a marketplace page without having previously visited the explore/home page in the same session, isNLPEnabled stays false and the NLQ toggle button never appears. This is a functional regression versus the previous local-fetch behavior, where the toggle always appeared when NLP was enabled.

Fix: have MarketplaceSearchBar populate the store itself when the value is not yet set, e.g. fetch getNLPEnabledStatus() on mount (guarded to avoid redundant calls) and call setNLPEnabled. Note the updated test seeds the store via useSearchStore.setState(...), so it no longer exercises this population path and masks the regression.

Bug: initNLP race: concurrent callers still fire duplicate API calls

📄 openmetadata-ui/src/main/resources/ui/src/hooks/useSearchStore.ts:35-43
In initNLP, the dedup guard checks get().isNLPInitialized, but the flag is only set to true after the async getNLPEnabledStatus() call resolves (line 42). If two components mount and call initNLP concurrently (e.g. GlobalSearchBar and MarketplaceSearchBar mounting in the same tick), both pass the isNLPInitialized guard while it is still false, both await the network request, and the API is hit twice — the exact redundant-call scenario this PR aims to eliminate. Switching the guard from isNLPEnabled to isNLPInitialized fixes the disabled-NLP case but does not close the concurrency window.

Fix: set the guard synchronously before awaiting, or cache the in-flight promise so concurrent callers share it.

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown
Contributor

🟡 Playwright Results — all passed (15 flaky)

✅ 4299 passed · ❌ 0 failed · 🟡 15 flaky · ⏭️ 88 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 301 0 1 4
🟡 Shard 2 811 0 1 9
🟡 Shard 3 813 0 3 8
🟡 Shard 4 854 0 3 12
🟡 Shard 5 731 0 2 47
🟡 Shard 6 789 0 5 8
🟡 15 flaky test(s) (passed on retry)
  • Features/TagsSuggestion.spec.ts › should decline requested tags for an api endpoint request schema field (shard 1, 1 retry)
  • Features/DataQuality/DataQuality.spec.ts › TestCase filters (shard 2, 1 retry)
  • Features/IncidentManager.spec.ts › Complete Incident lifecycle with table owner (shard 3, 1 retry)
  • Features/KnowledgeCenterTextEditor.spec.ts › Rich Text Editor - Text Formatting (shard 3, 1 retry)
  • Flow/ExploreDiscovery.spec.ts › Should not display soft deleted assets in search suggestions (shard 3, 1 retry)
  • Pages/CustomProperties.spec.ts › Sql Query (shard 4, 1 retry)
  • Pages/CustomProperties.spec.ts › Set & Update all CP types on apiCollection (shard 4, 1 retry)
  • Pages/DataContractsSemanticRules.spec.ts › Validate Description Rule Is_Not_Set (shard 4, 1 retry)
  • Pages/EntityDataConsumer.spec.ts › Tier Add, Update and Remove (shard 5, 1 retry)
  • Pages/ExplorePageRightPanel_KnowledgeCenter.spec.ts › Should remove user owner for knowledgeCenter (shard 5, 1 retry)
  • Pages/Glossary.spec.ts › Drag and Drop Glossary Term (shard 6, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify lineage schema filter selection (shard 6, 1 retry)
  • Pages/Lineage/LineageRightPanel.spec.ts › Verify custom properties tab IS visible for supported type: searchIndex (shard 6, 1 retry)
  • Pages/Lineage/PlatformLineage.spec.ts › Verify domain platform view (shard 6, 1 retry)
  • Pages/UserDetails.spec.ts › Create team with domain and verify visibility of inherited domain in user profile after team removal (shard 6, 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

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 skip-pr-checks Bypass PR metadata validation check

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants