Skip to content

feat(query-builder): migrate QueryBuilderWidgetV1 from antd to core-components - #29849

Open
chirag-madlani wants to merge 118 commits into
mainfrom
migrate-querybuilder-antd-to-core
Open

feat(query-builder): migrate QueryBuilderWidgetV1 from antd to core-components#29849
chirag-madlani wants to merge 118 commits into
mainfrom
migrate-querybuilder-antd-to-core

Conversation

@chirag-madlani

@chirag-madlani chirag-madlani commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Replaces all @react-awesome-query-builder/antd imports with @react-awesome-query-builder/ui (API-identical, same version) across 14 production files and 8 test files
  • Builds a custom OMConfig from BasicConfig with new widget factories backed by openmetadata-ui-core-components, eliminating Ant Design widget rendering inside query builder rules
  • Migrates QueryBuilderWidgetV1 outer shell (Card, Alert, Skeleton, Divider, Typography) from antd to core-components
  • Migrates button renderers in AdvancedSearchUtils and QueryBuilderUtils from antd Button + @ant-design/icons to core Button + @untitledui/icons
  • All query logic, config structure, elasticsearch format utilities, operator definitions, and async autocomplete behavior are preserved unchanged
Screen.Recording.2026-08-24.at.9.11.07.PM.mov
Screen.Recording.2026-08-24.at.9.29.51.PM.mov
Screen.Recording.2026-08-24.at.9.31.14.PM.mov

New widgets (src/utils/queryBuilderWidgets/)

Widget Core component
OMTextWidget Input
OMNumberWidget Input (numeric)
OMSelectWidget Select with async adapter
OMMultiSelectWidget MultiSelect + useListData from react-stately
OMBooleanWidget Toggle
OMDateWidget Native <input type="date/datetime-local/time"> (core DateInput is not publicly exported and requires @internationalized/date objects incompatible with RAQB string values)
OMFieldSelect Select (field/operator picker)
OMConjs ButtonGroup (AND/OR conjunction)

All assembled into src/utils/QueryBuilderOMConfig.tsx which exports OMConfig.

Test plan

  • All 27 QueryBuilderWidgetV1 tests pass
  • All 12 widget unit tests pass (src/utils/queryBuilderWidgets/)
  • Advanced Search modal opens and fields render correctly (no antd widget flash)
  • Adding/removing rules and groups works with the new button renderers
  • Async field autocomplete (owner, tags, tier) resolves correctly
  • JSON logic query builder (data contract) shows OM-styled widgets
  • TypeScript compiles with no new errors

🤖 Generated with Claude Code

Greptile Summary

This PR migrates the query builder UI from Ant Design to core components. The main changes are:

  • New core-component widgets for RAQB fields, values, conjunctions, and operators.
  • Core ComboBox and MultiSelect updates for live async option lists.
  • Query-builder shell and button rendering moved to core components.
  • Playwright selectors and sharding updated for the new UI.

Confidence Score: 4/5

This is close, but the remaining query-builder issues should be fixed before merging.

  • Async multiselect enum values beyond the first page still cannot be selected through browsing.
  • The number widget can still store Infinity or -Infinity in query state.
  • The core ComboBox async collection update looks aligned with the intended migration.

Files Needing Attention: OMMultiSelectWidget.tsx and OMNumberWidget.tsx

Important Files Changed

Filename Overview
openmetadata-ui/src/main/resources/ui/src/utils/queryBuilderWidgets/OMMultiSelectWidget.tsx Adds async result accumulation and saved-chip fallback handling, but still does not expose the async pagination path.
openmetadata-ui/src/main/resources/ui/src/utils/queryBuilderWidgets/OMNumberWidget.tsx Adds local input state and blocks NaN, but still accepts non-finite numeric values.
openmetadata-ui-core-components/src/main/resources/ui/src/components/base/select/combobox.tsx Moves ComboBox options to a controlled items collection for async callers.
openmetadata-ui/src/main/resources/ui/src/utils/QueryBuilderOMConfig.tsx Defines the core-components-backed RAQB configuration used by the query builder surfaces.

Reviews (48): Last reviewed commit: "fix flacky playwright" | Re-trigger Greptile

Greptile also left 2 inline comments on this PR.

Context used (3)

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

chirag-madlani and others added 13 commits July 8, 2026 14:55
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nput

Replace @react-awesome-query-builder/antd widgets with core-components. Two new widgets using Input component:
- OMTextWidget: string input values
- OMNumberWidget: numeric input with type="number"

All tests passing, TypeScript strict compilation verified.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…support

Implements Task 3 of the query builder migration from Ant Design to
openmetadata-ui-core-components. Provides async-capable single-select
widget wrapping core Select component. Includes handling for both static
list values and async fetch callbacks, with proper TypeScript typing.

- Converts listValues (array or object format) to SelectItemType[]
- Supports async data loading via asyncFetch callback
- Properly disables when readonly
- Fully tested with 2 core test cases (render + disabled state)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ith async support

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…and ButtonGroup

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…m core-component widgets

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ate button renderers to core-components

- AdvancedSearchClassBase: replace BasicConfig value with OMConfig from QueryBuilderOMConfig;
  BasicConfig is now type-only
- AdvancedSearchUtils: renderAdvanceSearchButtons uses Button (core), X and Trash01 icons
  from @untitledui/icons; removes @ant-design/icons and antd Button imports
- QueryBuilderUtils: renderQueryBuilderFilterButtons and renderJSONLogicQueryBuilderButtons
  use Button (core) and X/Plus from @untitledui/icons; removes antd and @ant-design/icons imports

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…and button renderers

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-components, clean up LESS

- Replace antd Card/Row/Col/Skeleton/Alert/Button/Divider/Typography with
  @openmetadata/ui-core-components equivalents
- Replace @ant-design/icons InfoCircleOutlined with @untitledui/icons InfoCircle
- Remove all .ant-* selectors from LESS; replace Less variable refs with
  CSS custom properties (--color-*)
- Update skeleton test selector from .ant-skeleton.ant-skeleton-active to
  [aria-hidden="true"] (core Skeleton uses aria-hidden)
- Update padding class test from .ant-col/.p-t-sm to .tw\\:pt-2

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Updated 7 test files to import from @react-awesome-query-builder/ui instead
of @react-awesome-query-builder/antd, and replaced AntdConfig with BasicConfig.
Applied UI checkstyle (organize-imports, lint:fix, prettier) on all modified
test files.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…invalid type cast, wire JSONLogicSearchClassBase to OMConfig

- OMDateWidget: replace getInputType(operator) with fieldType prop — operator values like "equal"/"less" never contain "time"/"datetime"; fieldType is the correct discriminant
- OMDateWidget: fix tw:bg-disabled_subtle → tw:bg-disabled-subtle (underscore → dash matches CSS token)
- OMNumberWidget: remove impossible `as number & null` intersection cast; Number(v) is already number
- JSONLogicSearchClassBase: import OMConfig and set baseConfig = OMConfig so JSON-logic query builder uses OM-styled widgets consistently with AdvancedSearchClassBase

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@chirag-madlani
chirag-madlani requested a review from a team as a code owner July 8, 2026 15:13
Copilot AI lite review requested due to automatic review settings July 8, 2026 15:13

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

github-actions Bot commented Jul 8, 2026

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 added safe to test Add this label to run secure Github workflows on PRs UI UI specific issues labels Jul 8, 2026
Comment on lines +78 to +91
async (search: string) => {
if (!asyncFetch) {
return;
}
const result = await asyncFetch(search);
setAllItems(
(result.values as ListItem[]).map((item) => ({
id: String(item.value),
label: String(item.title ?? item.value),
}))
);
},
[asyncFetch]
);

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 Async Pagination Is Dropped

The multi-select adapter always calls asyncFetch(search) and discards the returned hasMore state. Fetchers such as enum/custom-property autocomplete accept an offset and can return more pages, so values after the first page can never be loaded or selected in the query builder.

Comment on lines +58 to +75
useEffect(() => {
const currentIds = new Set(selectedItems.items.map((i) => i.id));
const targetIds = new Set(valueArray);

for (const id of targetIds) {
if (!currentIds.has(id)) {
const item = allItems.find((i) => i.id === id);
if (item) {
selectedItems.append(item);
}
}
}
for (const item of selectedItems.items) {
if (!targetIds.has(item.id)) {
selectedItems.remove(item.id);
}
}
}, [valueArray.join(',')]);

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 Saved Async Values Disappear

When a saved async multiselect filter is rendered, allItems is initially empty, so the sync effect cannot append chips for the current valueArray. After the async options arrive, the effect does not rerun for the same value, leaving persisted owner, tag, tier, or custom-property filters visually unselected even though the tree still contains their values.

Comment on lines +29 to +30
value={value !== null && value !== undefined ? String(value) : ''}
onChange={(v: string) => setValue(v === '' ? null : Number(v))}

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.

P2 Intermediate Numbers Store NaN

Number(v) is stored for every non-empty number-input string. Browser number inputs can emit intermediate values like 1e, -, or ., which convert to NaN; that value then enters the query tree and can produce an invalid or non-matching generated filter.

Suggested change
value={value !== null && value !== undefined ? String(value) : ''}
onChange={(v: string) => setValue(v === '' ? null : Number(v))}
value={value !== null && value !== undefined ? String(value) : ''}
onChange={(v: string) => {
if (v === '') {
setValue(null);
return;
}
const nextValue = Number(v);
if (Number.isFinite(nextValue)) {
setValue(nextValue);
}
}}

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 66%
67.04% (80586/120197) 51.45% (49339/95882) 52.44% (14745/28115)

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

return;
}
const result = await asyncFetch(search);
const fetched = (result.values as ListItem[]).map((item) => ({

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 Pagination Still Missing

loadAsync still only calls asyncFetch(search), so the enum custom-property fetcher always uses its first-page offset. That fetcher can return more results through hasMore, but this widget drops that metadata and never requests the next offset. When an enum custom property has more values than the first response, users cannot browse or select later values unless their search text narrows the value into page one.

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

if (!asyncFetch) {
return;
}
const result = await asyncFetch(search);

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 Pagination Still Missing

This still requests only the first async page. Enum custom-property fetchers accept an offset and return hasMore, but this call always uses the default offset and the result handling only stores the returned values. When a multiselect field has more values than the first response, users cannot browse or select later values unless their search text narrows the value into page one.

Comment on lines +54 to +56
if (!isNaN(num)) {
setValue(num);
}

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 Non-Finite Numbers Persist

This blocks NaN, but it still lets non-finite values into the query tree. A number input can accept exponent notation; typing 1e309 makes Number(v) become Infinity, and !isNaN(Infinity) is true, so setValue(Infinity) still runs. That leaves RAQB state with a value that cannot serialize as a valid query number.

Suggested change
if (!isNaN(num)) {
setValue(num);
}
if (Number.isFinite(num)) {
setValue(num);
}

@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), 109 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), 109 warning(s) across 27 changed file(s).

Count Rule
35 react-hooks/exhaustive-deps
22 @typescript-eslint/no-explicit-any
8 sonarjs/cyclomatic-complexity
7 openmetadata-imports/no-lower-layer-page-imports
6 sonarjs/no-duplicate-string
5 openmetadata-imports/no-impure-pure-utils
4 jsx-a11y/control-has-associated-label
4 openmetadata-imports/no-circular-imports
4 sonarjs/cognitive-complexity
4 @typescript-eslint/no-non-null-assertion
All findings
Location Rule Message
🟡 src/components/DataContract/ContractSemanticFormTab/ContractSemanticFormTab.test.tsx:40:6 @typescript-eslint/no-explicit-any Unexpected any. Specify a different type.
🟡 src/components/DataContract/ContractSemanticFormTab/ContractSemanticFormTab.test.tsx:110:8 @typescript-eslint/no-explicit-any Unexpected any. Specify a different type.
🟡 src/components/DataContract/ContractSemanticFormTab/ContractSemanticFormTab.tsx:99:5 react-hooks/exhaustive-deps React Hook useCallback has missing dependencies: 'form' and 'onChange'. Either include them or remove the dependency array. If 'onChange' changes too often, fin
🟡 src/components/DataContract/ContractSemanticFormTab/ContractSemanticFormTab.tsx:158:6 react-hooks/exhaustive-deps React Hook useEffect has a missing dependency: 'form'. Either include it or remove the dependency array.
🟡 src/components/DataContract/ContractSemanticFormTab/ContractSemanticFormTab.tsx:181:6 react-hooks/exhaustive-deps React Hook useEffect has a missing dependency: 'onChange'. Either include it or remove the dependency array. If 'onChange' changes too often, find the parent co
🟡 src/components/Explore/AdvanceSearchProvider/AdvanceSearchProvider.component.tsx:37:1 openmetadata-imports/no-lower-layer-page-imports Pages are route-level composition modules. Move the shared implementation/type to a lower layer instead of importing a page from here.
🟡 src/components/Explore/AdvanceSearchProvider/AdvanceSearchProvider.component.tsx:131:5 react-hooks/exhaustive-deps React Hook useMemo has a missing dependency: 'config'. Either include it or remove the dependency array.
🟡 src/components/Explore/AdvanceSearchProvider/AdvanceSearchProvider.component.tsx:160:6 react-hooks/exhaustive-deps React Hook useMemo has a missing dependency: 'config'. Either include it or remove the dependency array.
🟡 src/components/Explore/AdvanceSearchProvider/AdvanceSearchProvider.component.tsx:184:6 react-hooks/exhaustive-deps React Hook useEffect has a missing dependency: 'searchOutputType'. Either include it or remove the dependency array. If 'setConfig' needs the current value of '
🟡 src/components/Explore/AdvanceSearchProvider/AdvanceSearchProvider.component.tsx:204:9 react-hooks/exhaustive-deps The 'toggleModal' function makes the dependencies of useMemo Hook (at line 356) change on every render. Move it inside the useMemo callback. Alternatively, wrap
🟡 src/components/Explore/AdvanceSearchProvider/AdvanceSearchProvider.component.tsx:308:6 react-hooks/exhaustive-deps React Hook useEffect has a missing dependency: 'tabsInfo'. Either include it or remove the dependency array.
🟡 src/components/Explore/AdvanceSearchProvider/AdvanceSearchProvider.component.tsx:312:6 react-hooks/exhaustive-deps React Hook useEffect has a missing dependency: 'loadData'. Either include it or remove the dependency array.
🟡 src/components/Explore/AdvanceSearchProvider/AdvanceSearchProvider.component.tsx:325:6 react-hooks/exhaustive-deps React Hook useEffect has missing dependencies: 'handleReset' and 'loadTree'. Either include them or remove the dependency array.
🟡 src/components/MyData/Widgets/CuratedAssetsWidget/AdvancedAssetsFilterField/AdvancedAssetsFilterField.component.tsx:68:9 react-hooks/exhaustive-deps The 'selectedResource' logical expression could make the dependencies of useMemo Hook (at line 77) change on every render. To fix this, wrap the initialization
🟡 src/components/MyData/Widgets/CuratedAssetsWidget/AdvancedAssetsFilterField/AdvancedAssetsFilterField.component.tsx:68:9 react-hooks/exhaustive-deps The 'selectedResource' logical expression could make the dependencies of useMemo Hook (at line 135) change on every render. To fix this, wrap the initialization
🟡 src/components/MyData/Widgets/CuratedAssetsWidget/AdvancedAssetsFilterField/AdvancedAssetsFilterField.component.tsx:68:9 react-hooks/exhaustive-deps The 'selectedResource' logical expression could make the dependencies of useEffect Hook (at line 154) change on every render. To fix this, wrap the initializati
🟡 src/components/MyData/Widgets/CuratedAssetsWidget/AdvancedAssetsFilterField/AdvancedAssetsFilterField.component.tsx:121:5 react-hooks/exhaustive-deps React Hook useCallback has an unnecessary dependency: 'getExpandedResourceList'. Either exclude it or remove the dependency array. Outer scope values like 'getE
🟡 src/components/MyData/Widgets/CuratedAssetsWidget/AdvancedAssetsFilterField/AdvancedAssetsFilterField.component.tsx:124:37 react-hooks/exhaustive-deps React Hook useCallback received a function whose dependencies are unknown. Pass an inline function instead.
🟡 src/components/MyData/Widgets/CuratedAssetsWidget/AdvancedAssetsFilterField/AdvancedAssetsFilterField.component.tsx:172:6 react-hooks/exhaustive-deps React Hook useEffect has missing dependencies: 'config', 'fqn', 'onTreeUpdate', and 'queryFilter'. Either include them or remove the dependency array.
🟡 src/components/MyData/Widgets/CuratedAssetsWidget/AdvancedAssetsFilterField/AdvancedAssetsFilterField.component.tsx:187:13 jsx-a11y/label-has-for Form label must have ALL of the following types of associated control: nesting, id
🟡 src/components/common/Form/JSONSchema/JsonSchemaWidgets/QueryBuilderWidget/QueryBuilderWidget.tsx:41:1 openmetadata-imports/no-lower-layer-page-imports Pages are route-level composition modules. Move the shared implementation/type to a lower layer instead of importing a page from here.
🟡 src/components/common/Form/JSONSchema/JsonSchemaWidgets/QueryBuilderWidget/QueryBuilderWidget.tsx:68:77 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 14 which is greater than 10 authorized.","cost":4,"secondaryLocations":[{"line":68,"column":76,"endLine":68,"endColumn"
🟡 src/components/common/Form/JSONSchema/JsonSchemaWidgets/QueryBuilderWidget/QueryBuilderWidget.tsx:130:5 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'config'. Either include it or remove the dependency array.
🟡 src/components/common/Form/JSONSchema/JsonSchemaWidgets/QueryBuilderWidget/QueryBuilderWidget.tsx:144:5 react-hooks/exhaustive-deps React Hook useMemo has a missing dependency: 'searchResults'. Either include it or remove the dependency array.
🟡 src/components/common/Form/JSONSchema/JsonSchemaWidgets/QueryBuilderWidget/QueryBuilderWidget.tsx:219:6 react-hooks/exhaustive-deps React Hook useCallback has missing dependencies: 'debouncedFetchEntityCount', 'defaultField', 'onTreeUpdate', and 'subField'. Either include them or remove the
🟡 src/components/common/Form/JSONSchema/JsonSchemaWidgets/QueryBuilderWidget/QueryBuilderWidget.tsx:223:6 react-hooks/exhaustive-deps React Hook useEffect has missing dependencies: 'onChangeSearchIndex' and 'resolvedSearchIndex'. Either include them or remove the dependency array.
🟡 src/components/common/Form/JSONSchema/JsonSchemaWidgets/QueryBuilderWidget/QueryBuilderWidget.tsx:229:6 react-hooks/exhaustive-deps React Hook useEffect has a missing dependency: 'loadDefaultValueInTree'. Either include it or remove the dependency array.
🟡 src/components/common/Form/JSONSchema/JsonSchemaWidgets/QueryBuilderWidget/QueryBuilderWidget.tsx:235:6 react-hooks/exhaustive-deps React Hook useEffect has a missing dependency: 'props'. Either include it or remove the dependency array. However, 'props' will change when any prop changes,
🟡 src/components/common/QueryBuilderWidgetV1/QueryBuilderWidgetV1.test.tsx:109:39 @typescript-eslint/no-explicit-any Unexpected any. Specify a different type.
🟡 src/components/common/QueryBuilderWidgetV1/QueryBuilderWidgetV1.test.tsx:118:52 @typescript-eslint/no-explicit-any Unexpected any. Specify a different type.
🟡 src/components/common/QueryBuilderWidgetV1/QueryBuilderWidgetV1.test.tsx:126:30 @typescript-eslint/no-explicit-any Unexpected any. Specify a different type.
🟡 src/components/common/QueryBuilderWidgetV1/QueryBuilderWidgetV1.test.tsx:126:43 @typescript-eslint/no-explicit-any Unexpected any. Specify a different type.
🟡 src/components/common/QueryBuilderWidgetV1/QueryBuilderWidgetV1.test.tsx:144:18 @typescript-eslint/no-explicit-any Unexpected any. Specify a different type.
🟡 src/components/common/QueryBuilderWidgetV1/QueryBuilderWidgetV1.test.tsx:362:34 @typescript-eslint/no-explicit-any Unexpected any. Specify a different type.
🟡 src/components/common/QueryBuilderWidgetV1/QueryBuilderWidgetV1.tsx:49:1 openmetadata-imports/no-lower-layer-page-imports Pages are route-level composition modules. Move the shared implementation/type to a lower layer instead of importing a page from here.
🟡 src/components/common/QueryBuilderWidgetV1/QueryBuilderWidgetV1.tsx:200:5 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'config'. Either include it or remove the dependency array.
🟡 src/components/common/QueryBuilderWidgetV1/QueryBuilderWidgetV1.tsx:210:7 sonarjs/expression-complexity Reduce the number of conditional operators (4) used in the expression (maximum allowed 3).
🟡 src/components/common/QueryBuilderWidgetV1/QueryBuilderWidgetV1.tsx:215:5 react-hooks/exhaustive-deps React Hook useMemo has a missing dependency: 'searchResults'. Either include it or remove the dependency array.
🟡 src/components/common/QueryBuilderWidgetV1/QueryBuilderWidgetV1.tsx:261:6 react-hooks/exhaustive-deps React Hook useEffect has a missing dependency: 'props'. Either include it or remove the dependency array. However, 'props' will change when any prop changes,
🟡 src/components/common/QueryBuilderWidgetV1/QueryBuilderWidgetV1.tsx:337:13 jsx-a11y/control-has-associated-label A control must be associated with a text label.
🟡 src/components/common/TagsSection/TagsSection.test.tsx:143:15 jsx-a11y/control-has-associated-label A control must be associated with a text label.
🟡 src/utils/AdvancedSearchClassBase.ts:48:1 openmetadata-imports/no-circular-imports This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency.
🟡 src/utils/AdvancedSearchClassBase.ts:280:16 sonarjs/no-duplicate-string Define a constant instead of duplicating this literal 3 times.
🟡 src/utils/AdvancedSearchClassBase.ts:611:51 sonarjs/no-duplicate-string Define a constant instead of duplicating this literal 3 times.
🟡 src/utils/AdvancedSearchClassBase.ts:946:26 sonarjs/no-duplicate-string Define a constant instead of duplicating this literal 4 times.
🟡 src/utils/AdvancedSearchClassBase.ts:1360:11 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 21 which is greater than 10 authorized.","cost":11,"secondaryLocations":[{"line":1360,"column":10,"endLine":1360,"endCo
🟡 src/utils/AdvancedSearchPureUtils.ts:16:1 openmetadata-imports/no-impure-pure-utils Pure utilities must not depend on React, UI, state, hooks, pages, or REST clients. Move orchestration/rendering out or move shared types to a lower layer.
🟡 src/utils/AdvancedSearchPureUtils.ts:17:1 openmetadata-imports/no-impure-pure-utils Pure utilities must not depend on React, UI, state, hooks, pages, or REST clients. Move orchestration/rendering out or move shared types to a lower layer.
🟡 src/utils/AdvancedSearchPureUtils.ts:18:1 openmetadata-imports/no-impure-pure-utils Pure utilities must not depend on React, UI, state, hooks, pages, or REST clients. Move orchestration/rendering out or move shared types to a lower layer.
🟡 src/utils/AdvancedSearchPureUtils.ts:214:23 sonarjs/cyclomatic-complexity {"message":"Function has a complexity of 11 which is greater than 10 authorized.","cost":1,"secondaryLocations":[{"line":214,"column":22,"endLine":214,"endColum

… and 59 more. Run make ui-checkstyle-changed locally for the full list.


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

make ui-checkstyle-changed

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

renderField: (props) => <OMFieldSelect {...props} />,
// RAQB passes the same FieldProps shape (including setField) to both
// field and operator renderers, so the same select component works for both.
renderOperator: (props) => <OMFieldSelect {...props} />,

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 Operator callback miswired
renderOperator still renders OMFieldSelect, whose selection handler calls setField(id). This renderer is used for the rule's operator slot, so selecting ==, !=, or another operator needs to update the operator value, not the field value. When a user changes an operator, the rule can keep the old operator or update the wrong slot, and the generated query can differ from the visible rule.

if (!asyncFetch) {
return;
}
const result = await asyncFetch(search);

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 Pagination still missing
This still calls asyncFetch(search) without an offset and ignores the returned pagination state. Custom-property enum multiselect fetchers can return hasMore for later pages, but this widget never stores that state or requests the next offset. For enum fields with more than the first response page, users cannot browse or select later values unless their search narrows the value into page one.

Comment on lines +53 to +56
const num = Number(v);
if (!isNaN(num)) {
setValue(num);
}

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 Non-finite numbers persist
This rejects NaN, but it still allows non-finite numbers. A number input can accept exponent notation such as 1e309; Number('1e309') becomes Infinity, and !isNaN(Infinity) is true, so the widget writes Infinity into the query-builder state. Query serialization can then emit null or an invalid numeric filter instead of the value the user entered.

Suggested change
const num = Number(v);
if (!isNaN(num)) {
setValue(num);
}
const num = Number(v);
if (Number.isFinite(num)) {
setValue(num);
}

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

if (!asyncFetch) {
return;
}
const result = await asyncFetch(search);

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 Pagination Stays Unreachable

This still requests only the first async page. Enum custom-property fetchers accept an offset and return hasMore, but this call always uses the default offset and the result handling ignores the pagination state. When a multiselect field has values beyond the first response page, users cannot browse or select those values unless their search narrows the value into page one.

setValue(null);
} else {
const num = Number(v);
if (!isNaN(num)) {

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 Non-Finite Numbers Persist

This blocks NaN, but it still accepts non-finite numbers. A number input can accept exponent notation such as 1e309; Number('1e309') becomes Infinity, and !isNaN(Infinity) is true, so the widget writes Infinity into the query-builder state. Query serialization can then produce null or an invalid numeric filter instead of the value the user entered.

… groups

`Complex nested groups` set a Description Status / Is rule (an enum value) and
then did:

    await ruleLocator2.locator('.rule--value input').fill('production');

That line was always a no-op: the value is already `Incomplete` from the
preceding selectOption, and the typed text was discarded on blur. It only
resolved because antd's Select rendered an inner
`input.ant-select-selection-search-input`.

After the antd -> core migration the value widget is a react-aria Select, whose
`.rule--value` contains a trigger `<button>` and a visually hidden `<select>` —
no `<input>` at all. So `fill()` waits for a locator that can never resolve and
burns the full 180s test timeout (x2 with CI retries, each capturing trace and
video of a stuck page).

This is what pushed the chromium-02 shard past its 1500s budget: the shard was
SIGTERM'd (exit 124) with 129/131 tests passed, `Complex nested groups` never
reporting and `Placeholder validation` never starting behind it in the same
worker queue.

Verified locally against a build of this branch served from the container:
CuratedAssets.spec.ts now runs 26/26 green in 3.2m.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

if (!asyncFetch) {
return;
}
const result = await asyncFetch(search);

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 Pagination Still Missing

This still only fetches the first async page. Enum custom-property multiselects use an async fetcher that accepts an offset and returns hasMore, but this call always uses the default offset and the widget drops the pagination metadata. When an enum field has more values than the first response page, users cannot browse or select later values unless their search text narrows the value into page one. Preserve the pagination state and add a path that requests and appends later pages.

Comment on lines +54 to +56
if (!isNaN(num)) {
setValue(num);
}

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 Non-Finite Numbers Persist

This guard still allows non-finite numbers into query-builder state. A number input can accept exponent notation such as 1e309; Number('1e309') becomes Infinity, and !isNaN(Infinity) is true, so setValue(Infinity) runs. Downstream serialization cannot produce a valid numeric query value from that state, which can leave the generated filter broken or non-matching.

Suggested change
if (!isNaN(num)) {
setValue(num);
}
if (Number.isFinite(num)) {
setValue(num);
}

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

Comment on lines +54 to +56
if (!isNaN(num)) {
setValue(num);
}

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 Non-finite numbers persist

This guard rejects NaN, but it still accepts non-finite numbers. A number input can accept exponent notation such as 1e309; Number('1e309') becomes Infinity, and !isNaN(Infinity) is true, so setValue(Infinity) writes an invalid numeric value into the query-builder state. Downstream query serialization cannot produce a valid numeric filter from that value.

Suggested change
if (!isNaN(num)) {
setValue(num);
}
if (Number.isFinite(num)) {
setValue(num);
}

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

@gitar-bot

gitar-bot Bot commented Aug 24, 2026

Copy link
Copy Markdown
Code Review ⚠️ Changes requested 4 resolved / 6 findings

Migrates QueryBuilderWidgetV1 and its widgets from Ant Design to core-components with custom widget factories. Changes requested due to missing debounce on async select network fetches and dropped group labels in OMFieldSelect.

⚠️ Performance: Async select fires a network fetch on every keystroke (no debounce)

📄 openmetadata-ui/src/main/resources/ui/src/utils/queryBuilderWidgets/OMSelectWidget.tsx:74-88 📄 openmetadata-ui/src/main/resources/ui/src/utils/queryBuilderWidgets/OMSelectWidget.tsx:52-66

In OMSelectWidget.tsx the new async Select.ComboBox branch wires onInputChange={(v) => { loadAsync(v); }}, and loadAsync calls asyncFetch(search) which performs the /api/v1/search/aggregate request (owner/tag/tier autocomplete). There is no debounce/throttle, so every character typed issues a fresh backend request. The previous Ant Design RAQB widget debounced these lookups. This can produce a burst of aggregate queries per search, wasted work, and out-of-order responses where a slower earlier request overwrites items set by a later one (the last setItems to resolve wins, not the last request issued).

Suggested fix: debounce the input handler and/or guard against stale responses (e.g. track the latest request and ignore results from superseded searches).

Track the latest request and drop superseded responses; debounce onInputChange.
// debounce input + ignore stale responses
const latestReq = useRef(0);
const loadAsync = useCallback(async (search: string) => {
  if (!asyncFetch) return;
  const reqId = ++latestReq.current;
  const result = await asyncFetch(search);
  if (reqId !== latestReq.current) return; // stale
  setItems((result.values as ListItem[]).map((item) => ({
    id: String(item.value),
    label: String(item.title ?? item.value),
  })));
}, [asyncFetch]);
// then wrap the onInputChange call site in a debounce (e.g. lodash debounce, 300ms)
💡 Quality: OMFieldSelect drops group label (supportingText) from field picker

📄 openmetadata-ui/src/main/resources/ui/src/utils/queryBuilderWidgets/OMFieldSelect.tsx:24-27

In OMFieldSelect.tsx the mapping previously set supportingText: item.grouplabel, which surfaced the field's group/entity context in the field picker dropdown. This commit removes it, so the field/operator selector no longer shows the group label. If this is an intentional simplification it can be ignored, but it is a user-facing regression from the prior behavior where grouped fields displayed their group as supporting text. Confirm whether losing the group label is intended; if not, restore supportingText: item.grouplabel.

✅ 4 resolved
Edge Case: OMDateWidget can't render ISO/Date values in native inputs

📄 openmetadata-ui/src/main/resources/ui/src/utils/queryBuilderWidgets/OMDateWidget.tsx:32-45
OMDateWidget binds value={String(value ?? '')} directly to a native <input type="date|datetime-local|time">. These native inputs only accept strictly-formatted strings (yyyy-MM-dd, yyyy-MM-ddTHH:mm, HH:mm). If RAQB provides a full ISO timestamp (e.g. 2024-01-01T00:00:00.000Z) or a Date object for a preexisting value, String(value) will not match the required format and the input will silently render empty, dropping the previously-saved date when a rule is edited. Consider normalizing value to the format expected by each type before passing it to the input.

Bug: Async multiselect won't show preselected values as chips

📄 openmetadata-ui/src/main/resources/ui/src/utils/queryBuilderWidgets/OMMultiSelectWidget.tsx:58-72 📄 openmetadata-ui/src/main/resources/ui/src/utils/queryBuilderWidgets/OMMultiSelectWidget.tsx:77-91
In OMMultiSelectWidget, the effect that syncs selectedItems (from useListData) with the incoming valueArray only appends items it can find in allItems:

const item = allItems.find((i) => i.id === id);
if (item) {
  selectedItems.append(item);
}

and it is keyed only on [valueArray.join(',')].

For async fields (owner, tags, tier), listValues is empty at mount, so staticItems and the initial allItems are empty. When editing an existing filter that already has selected values, the sync effect runs once, fails to find the ids in the empty allItems, and appends nothing. loadAsync('') later populates allItems, but since allItems is not in the effect's dependency array the sync effect does not re-run — so the preselected values are never rendered as selected chips. This regresses the edit experience for async multiselect fields (a common case: editing a saved advanced-search / data-contract rule).

Suggested fix: include allItems in the effect dependencies, and/or fall back to a placeholder { id, label: id } when the item is not yet loaded so the selection is preserved.

Edge Case: Async single-select may not display label for preselected value

📄 openmetadata-ui/src/main/resources/ui/src/utils/queryBuilderWidgets/OMSelectWidget.tsx:49-63
In OMSelectWidget, selectedKey is set to String(value) and the visible label is looked up from items. For async fields, items starts empty (listValues is empty) and is only populated after loadAsync('') resolves. If the resolved page does not include the currently-selected value, or before it resolves, the Select has a selectedKey with no matching item and will render without the human-readable label. Consider seeding items with the current value (e.g. { id: String(value), label: String(value) }) when it is not present in the loaded list, mirroring the fix needed for the multiselect widget.

Edge Case: Selecting an async option re-triggers a fetch via onInputChange

📄 openmetadata-ui/src/main/resources/ui/src/utils/queryBuilderWidgets/OMSelectWidget.tsx:86-91
In the async Select.ComboBox branch of OMSelectWidget.tsx, onInputChange unconditionally calls loadAsync(v). When a user selects an option, react-aria ComboBox typically updates the input text to the selected item's label, which fires onInputChange again and triggers an extra asyncFetch call with the selected label as the search term. This is an unnecessary request and can also cause the items list to be replaced right after selection. Consider ignoring input changes that are not user-driven typing (react-aria provides trigger context via onInputChange's second argument in some versions) or comparing against the current selection before re-fetching.

🤖 Prompt for agents
Code Review: Migrates QueryBuilderWidgetV1 and its widgets from Ant Design to core-components with custom widget factories. Changes requested due to missing debounce on async select network fetches and dropped group labels in OMFieldSelect.

1. ⚠️ Performance: Async select fires a network fetch on every keystroke (no debounce)
   Files: openmetadata-ui/src/main/resources/ui/src/utils/queryBuilderWidgets/OMSelectWidget.tsx:74-88, openmetadata-ui/src/main/resources/ui/src/utils/queryBuilderWidgets/OMSelectWidget.tsx:52-66

   In `OMSelectWidget.tsx` the new async `Select.ComboBox` branch wires `onInputChange={(v) => { loadAsync(v); }}`, and `loadAsync` calls `asyncFetch(search)` which performs the `/api/v1/search/aggregate` request (owner/tag/tier autocomplete). There is no debounce/throttle, so every character typed issues a fresh backend request. The previous Ant Design RAQB widget debounced these lookups. This can produce a burst of aggregate queries per search, wasted work, and out-of-order responses where a slower earlier request overwrites `items` set by a later one (the last `setItems` to resolve wins, not the last request issued).
   
   Suggested fix: debounce the input handler and/or guard against stale responses (e.g. track the latest request and ignore results from superseded searches).

   Fix (Track the latest request and drop superseded responses; debounce onInputChange.):
   // debounce input + ignore stale responses
   const latestReq = useRef(0);
   const loadAsync = useCallback(async (search: string) => {
     if (!asyncFetch) return;
     const reqId = ++latestReq.current;
     const result = await asyncFetch(search);
     if (reqId !== latestReq.current) return; // stale
     setItems((result.values as ListItem[]).map((item) => ({
       id: String(item.value),
       label: String(item.title ?? item.value),
     })));
   }, [asyncFetch]);
   // then wrap the onInputChange call site in a debounce (e.g. lodash debounce, 300ms)

2. 💡 Quality: OMFieldSelect drops group label (supportingText) from field picker
   Files: openmetadata-ui/src/main/resources/ui/src/utils/queryBuilderWidgets/OMFieldSelect.tsx:24-27

   In `OMFieldSelect.tsx` the mapping previously set `supportingText: item.grouplabel`, which surfaced the field's group/entity context in the field picker dropdown. This commit removes it, so the field/operator selector no longer shows the group label. If this is an intentional simplification it can be ignored, but it is a user-facing regression from the prior behavior where grouped fields displayed their group as supporting text. Confirm whether losing the group label is intended; if not, restore `supportingText: item.grouplabel`.

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

if (!asyncFetch) {
return;
}
const result = await asyncFetch(search);

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 Pagination Still Missing
loadAsync still calls asyncFetch(search) without an offset and only stores result.values. Enum custom-property fetchers can return hasMore and use later offsets, but this widget drops that state and has no way to request the next page. When a multiselect field has more values than the first response, users cannot browse or select those later values unless their search text narrows the value into page one.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

setValue(null);
} else {
const num = Number(v);
if (!isNaN(num)) {

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 Non-Finite Numbers Persist
This guard still lets non-finite values into query-builder state. A number input can emit exponent notation such as 1e309; Number('1e309') becomes Infinity, and !isNaN(Infinity) is true, so setValue(Infinity) runs. The query formatter later places numeric values directly into term and range bodies, where Infinity cannot serialize as a valid JSON number and can produce a broken or non-matching filter.

Suggested change
if (!isNaN(num)) {
if (Number.isFinite(num)) {

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants