feat(query-builder): migrate QueryBuilderWidgetV1 from antd to core-components#29849
feat(query-builder): migrate QueryBuilderWidgetV1 from antd to core-components#29849chirag-madlani wants to merge 46 commits into
Conversation
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>
…ocument native input in OMDateWidget
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
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 |
github-actions
Bot
added
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| 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); |
There was a problem hiding this comment.
⚠️ Bug: Async multiselect won't show preselected values as chips
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.
Re-run the sync when allItems loads and fall back to a placeholder label so preselected async values are preserved.:
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) ?? {
id,
label: id,
};
selectedItems.append(item);
}
}
for (const item of selectedItems.items) {
if (!targetIds.has(item.id)) {
selectedItems.remove(item.id);
}
}
}, [valueArray.join(','), allItems]);
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| const staticItems = toSelectItems(listValues); | ||
| const [items, setItems] = useState<SelectItemType[]>(staticItems); | ||
|
|
||
| const loadAsync = useCallback( | ||
| async (search: string) => { | ||
| if (!asyncFetch) { | ||
| return; | ||
| } | ||
| const result = await asyncFetch(search); | ||
| setItems( | ||
| (result.values as ListItem[]).map((item) => ({ | ||
| id: String(item.value), | ||
| label: String(item.title ?? item.value), | ||
| })) | ||
| ); |
There was a problem hiding this comment.
💡 Edge Case: Async single-select may not display label for preselected value
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.
Was this helpful? React with 👍 / 👎
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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] | ||
| ); |
There was a problem hiding this comment.
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.
| 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(',')]); |
There was a problem hiding this comment.
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.
| value={value !== null && value !== undefined ? String(value) : ''} | ||
| onChange={(v: string) => setValue(v === '' ? null : Number(v))} |
There was a problem hiding this comment.
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.
| 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); | |
| } | |
| }} |
Jest test CoverageUI tests summary
|
…rch — verified against local server
Debugged live against a dev server + local backend instead of CI. Five
root-caused fixes, each verified by re-running the previously failing
Playwright specs (DataContractsSemanticRules DisplayName ×6 + UpdatedOn ×3
and CustomProperties "for table" ×26 now pass locally):
1. combobox.tsx: pass defaultItems to AriaComboBox instead of items on the
inner ListBox. With items only on the ListBox, react-aria 1.17 (the
app's resolved version — core-components pins 1.16 in its own tree)
loses the options' textValue, so any typed filter matches nothing, the
popup closes and the input reverts.
2. OMFieldSelect: use item.path (full dotted path) as the option id —
item.key is only the leaf segment for nested fields, so setField() was
silently ignored and custom-property cascades never advanced. Memoize
the mapped items (identity churn rebuilds the RAC collection mid-open)
and control inputValue explicitly so app re-renders can't clobber the
user's in-progress filter text.
3. OMSelectWidget (async): allowsEmptyCollection — while a fetch is in
flight the typed text transiently filters old results to zero and RAC
would close the popup for good.
4. multi-select.tsx: chain the consumer's onInputChange instead of
dropping it (internal handler is applied after {...props}), and derive
the visible options from the live items prop — useListData snapshots
initialItems on mount, so async search results never appeared.
5. selectOption helper: press ArrowDown after fill to deterministically
(re)open the popup RAC closes while processing the atomic fill; click
MultiSelect inputs before filling (their popup opens from a mousedown
handler and no chevron overlays them).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| export const OMConfig: BasicConfig = { | ||
| ...QbBasicConfig, | ||
| settings: { | ||
| ...QbBasicConfig.settings, |
There was a problem hiding this comment.
renderOperator still reuses OMFieldSelect, and that component only calls setField(String(key)) when a selection is made. When a user changes a rule operator, RAQB reaches this renderer with the operator-change callback contract, but the selected operator is still routed through the field-selection path. Changing an operator such as == to != can leave the operator unchanged or update the wrong part of the rule, so the generated query no longer matches the UI.
| const result = await asyncFetch(search); | ||
| if (requestId === requestIdRef.current) { | ||
| setAllItems( | ||
| (result.values as ListItem[]).map((item) => ({ | ||
| id: String(item.value), | ||
| label: String(item.title ?? item.value), | ||
| })) | ||
| ); | ||
| } |
There was a problem hiding this comment.
hasMore when more values exist, but this widget only calls asyncFetch(search) with the default offset and replaces allItems with result.values. For an enum custom property with more than the first page of values, users can only browse page one and cannot select later values unless a search narrows them into that first response.
| 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(',')]); |
There was a problem hiding this comment.
allItems, but it only reruns when valueArray.join(',') changes. When a saved async owner, tag, tier, or custom-property filter remounts, allItems starts from the static list and the effect cannot find the saved id. loadAsync('') later updates allItems, but the saved value is unchanged, so the chip is never appended even though the query still contains the filter.
| size="sm" | ||
| type="number" | ||
| value={value !== null && value !== undefined ? String(value) : ''} | ||
| onChange={(v: string) => setValue(v === '' ? null : Number(v))} |
There was a problem hiding this comment.
Number(v) for every non-empty number-input string. During normal editing, number inputs can emit intermediate values such as -, ., or 1e; these convert to NaN and get written into RAQB's query value. That leaves the rule with an invalid numeric value, which can produce broken serialization or a filter that never matches.
Down to 15 CI failures; the query-builder-related ones are stale antd selectors that no longer exist after the widget migration: - DataContractsSemanticRules Version tests (<, <=, >, >=) hung on .ant-input-number-input — OMNumberWidget now renders a core-components Input with data-testid="qb-number-input" (verified locally: 3/3 pass). - CuratedAssets AND/OR tests hung on .ant-switch — OMBooleanWidget renders a react-aria Switch (a label element) — and clicked conjunctions via .group--conjunctions button:has-text(...), which are radio-role toggles now. The other failures in the run (ContextCenter, DataQualityDashboard, TestCaseImportExport, ExploreQuickFilters, PersonaDeletion, Tour, the CustomProperties right-panel scroll) don't touch the query builder. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
||
| for (const id of targetIds) { | ||
| if (!currentIds.has(id)) { | ||
| const item = allItems.find((i) => i.id === id); |
There was a problem hiding this comment.
When an async multiselect remounts with a saved value, selectedItems starts from staticItems, and this effect reads allItems before loadAsync('') has filled it. The saved value string does not change after async options arrive, so this effect does not run again and the selected chip stays missing even though the query still contains the filter. Reconcile again when async options change, or keep selected option objects separately from the current option page.
| } | ||
| } | ||
| }, [valueArray.join(',')]); | ||
|
|
There was a problem hiding this comment.
Enum custom-property fetchers can return hasMore and an offset for the next page, but this path only calls asyncFetch(search) and keeps result.values. The widget drops the pagination state and replaces allItems, so enum fields with more than the first page still cannot be browsed or selected unless the user already knows text that narrows the search into the first response. Keep the pagination metadata and add a path that requests and appends later offsets.
| size="sm" | ||
| type="number" | ||
| value={value !== null && value !== undefined ? String(value) : ''} | ||
| onChange={(v: string) => setValue(v === '' ? null : Number(v))} |
There was a problem hiding this comment.
This still stores Number(v) for every non-empty input string. During normal editing, number inputs can emit transient strings like -, ., or 1e; those convert to NaN and get written into RAQB state. The generated filter can then serialize with an invalid numeric value or stop matching. Only call setValue for finite numbers, or keep the transient text local until it becomes a valid number.
…Option Verified locally against a live server (7 CI failures → the QB-related ones reproduced and fixed): - OMDateWidget rendered type="date" for datetime custom properties: derive date/time/datetime-local from the field's configured moment format, convert stored (valueFormat, e.g. "DD-MM-YYYY HH:mm:ss") ↔ native input values with moment, and set step=1 — native time inputs reject seconds at the default 60s step. Verified: Date/DateTime/Time CP "all operators" tests pass locally. - customPropertyAdvancedSearchUtils: convert CP display values to the native ISO shape before filling; datetime values carrying seconds are written through the native value setter because Playwright's fill() rejects seconds on datetime-local inputs regardless of step. - selectOption: scope the popup to its own control via aria-controls — popovers portal to <body> and MultiSelect popups stay open by design, so a stale popup from a previous step could hijack the global [role="listbox"]:visible locator (the CuratedAssets 'Owners' failures). Retry the resolve+click loop while the builder re-renders under it, close the popup after selection so it can't pollute the next step, and tolerate the control disappearing (field selection can morph the rule row). Same scoping for the non-existing-value helper, fixing its strict mode violation when two popups were visible. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| selectedItems.remove(item.id); | ||
| } | ||
| } | ||
| }, [valueArray.join(',')]); |
There was a problem hiding this comment.
This reconciliation reads allItems, but it only runs when the saved value string changes. For async multiselect fields, selectedItems starts from staticItems, which is empty for async-backed owner, tag, tier, or custom-property values. On mount, this effect can run before loadAsync('') fills allItems, so the saved ids are not appended. When the async options arrive, the value has not changed, so the chips stay missing even though the query still contains the filter.
| }, [valueArray.join(',')]); | |
| }, [valueArray.join(','), allItems]); |
| size="sm" | ||
| type="number" | ||
| value={value !== null && value !== undefined ? String(value) : ''} | ||
| onChange={(v: string) => setValue(v === '' ? null : Number(v))} |
There was a problem hiding this comment.
This still writes Number(v) for every non-empty edit. During normal number entry, browser inputs can emit transient strings such as -, ., or 1e; those convert to NaN and are passed into RAQB state. The widget then renders from that state with String(value), so the rule can keep an invalid numeric value and generate a broken or non-matching filter. Keep the transient text local until it parses to a finite number, or otherwise avoid calling setValue with NaN.
…data assertion - selectOption: close leftover popups by BLURRING the control instead of pressing Escape — a trace + screenshot proved Escape bubbles to the surrounding antd form/modal (capture-phase handler) and dismisses the whole Add-Contract form. Verified locally: the Contains semantic test passes with blur where Escape closed the form. - Tolerate the control disappearing in the ensure-closed tail (selecting a field morphs the rule row and unmounts the original control). - runRuleGroupTestsWithNonExistingValue: with allowsEmptyCollection the popup stays open and renders a "No data" empty-state row, so assert that instead of toHaveCount(0) which can never pass. - CP utils: react-aria MultiSelect commits the FOCUSED option on Enter, so press ArrowDown first (antd committed the typed text) — enum CP values were silently never selected, producing empty filters. - TEMP diagnostic in the Not_Contains semantic test: dumps the DOM inventory (buttons/rows) right before the Add New Rule click that can't resolve in CI — the same click passes in the Contains test; local runs show the edit form open but the button unattached. Will be removed once CI pinpoints the state. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| React Aria owns the collection and applies its contains-filter to | ||
| the typed input — with items only on the ListBox, react-aria ≥1.17 | ||
| loses the options' textValue and any typed filter matches nothing, | ||
| which closes the popup and reverts the input. */} |
There was a problem hiding this comment.
This forwards the option collection as defaultItems, so the ComboBox owns only the initial collection. Async query-builder single-selects update their items after the empty-search preload or typed search resolves, but those later results can be ignored by React Aria because they are still passed as uncontrolled defaults. When a user searches an async-backed single-select field, the fetch can return matching values while the popup continues filtering the stale initial options, leaving the value impossible to select. Use a controlled collection for the live items updates instead of defaultItems on this path.
…en leak Root cause of the "element exists but role queries never resolve" failure class (DataContracts Contains/Not_Contains 'Add New Rule', and every getByRole step after a query-builder popup interaction): react-aria modal popovers apply aria-hidden to the entire page outside the overlay (ariaHideOutside). When the popover unmounts abruptly — which the query builder does constantly, since selecting a field or operator morphs the rule row and its widgets — the restore cleanup never runs, leaving the whole app aria-hidden. The page looks normal and CSS queries work, but the accessibility tree is empty, so role-based locators (and assistive tech) see nothing. Diagnosed via an in-test DOM dump that reproduced identically in CI and locally: add-new-rule-btn present in DOM, btnHiddenAncestor=true, 86 aria-hidden elements. Combobox/select popups are semantically non-modal anyway (focus stays in the input), so pass isNonModal — react-aria then never hides the rest of the page. Also removes the temporary diagnostic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| the typed input — with items only on the ListBox, react-aria ≥1.17 | ||
| loses the options' textValue and any typed filter matches nothing, | ||
| which closes the popup and reverts the input. */} | ||
| <AriaComboBox defaultItems={items} menuTrigger="focus" {...otherProps}> |
There was a problem hiding this comment.
defaultItems makes this collection uncontrolled, so async callers that update items after mount can keep showing the initial option set. OMSelectWidget replaces its items state when asyncFetch(search) resolves; with this line, typed owner, tag, tier, or custom-property searches can fetch matching values while the ComboBox popup still filters the initial empty or stale collection. The user then cannot select the returned value. This path needs a controlled collection for live async results while preserving the text-value filtering fix.
| return; | ||
| } | ||
| // Guard against out-of-order responses: only the latest request may | ||
| // set items, otherwise a slow earlier fetch overwrites newer results. |
There was a problem hiding this comment.
This still requests only the first async page and ignores the pagination result. Enum and custom-property fetchers accept an offset and can return hasMore: true, but loadAsync always calls asyncFetch(search) and then replaces allItems with that response. For enum fields with more than the first page of values, the dropdown has no path to request or append later pages, so users cannot browse and select those values unless their typed search narrows the result into the first response.
| for (const item of selectedItems.items) { | ||
| if (!targetIds.has(item.id)) { | ||
| selectedItems.remove(item.id); | ||
| } |
There was a problem hiding this comment.
This reconciliation reads allItems, but it only reruns when the saved value string changes. When a saved async multiselect filter mounts, allItems starts from staticItems, so owner, tag, tier, or custom-property ids that arrive from loadAsync('') cannot be appended as chips. After the async options update allItems, the saved value is unchanged, so the chip remains missing while the query tree still contains the filter.
| 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(',')]); |
There was a problem hiding this comment.
This reconciliation reads allItems, but it only runs when the saved value string changes. When an async multiselect remounts with a saved owner, tag, tier, or custom-property value, loadAsync('') fills allItems after this effect has already run. The query tree still contains the saved value, but the chip is never appended, so the filter appears unselected. Reconcile again when async options arrive, or keep selected option objects separately from the current option page.
| const result = await asyncFetch(search); | ||
| if (requestId === requestIdRef.current) { | ||
| setAllItems( | ||
| (result.values as ListItem[]).map((item) => ({ | ||
| id: String(item.value), | ||
| label: String(item.title ?? item.value), | ||
| })) | ||
| ); |
There was a problem hiding this comment.
This still requests only the first async page. Enum and custom-property fetchers can return hasMore and use an offset for later pages, but this code calls asyncFetch(search) without an offset, discards the pagination metadata, and replaces allItems with the first response. For enum fields with more than one page of values, users cannot browse or select later values unless a search narrows them into the first page. Store the pagination state and add a path that requests and appends later pages.
| size="sm" | ||
| type="number" | ||
| value={value !== null && value !== undefined ? String(value) : ''} | ||
| onChange={(v: string) => setValue(v === '' ? null : Number(v))} |
There was a problem hiding this comment.
This still writes Number(v) for every non-empty edit. During normal number entry, browser inputs can emit transient strings such as -, ., or 1e; those convert to NaN and are passed into RAQB state. The rule can then serialize with an invalid numeric value or produce a filter that never matches. Keep the transient text local until it parses to a finite number, or avoid calling setValue with NaN.
| the typed input — with items only on the ListBox, react-aria ≥1.17 | ||
| loses the options' textValue and any typed filter matches nothing, | ||
| which closes the popup and reverts the input. */} | ||
| <AriaComboBox defaultItems={items} menuTrigger="focus" {...otherProps}> |
There was a problem hiding this comment.
ComboBox destructures items out of otherProps, then forwards it only as defaultItems. Async callers such as OMSelectWidget replace their items state after asyncFetch(search) resolves, but defaultItems is an uncontrolled initial collection. A typed owner, tag, tier, or custom-property search can fetch matching options while the popup keeps filtering the initial collection, leaving the returned value impossible to select. Use a controlled collection for live async updates while preserving the text-value filtering behavior.
The CuratedAssets "Complex nested groups" CI failure showed the
add-condition button present in the accessibility snapshot as an unnamed
button (its img + "Add Condition" text listed as children but the computed
accessible name empty — name-from-contents breaks inside RAQB's action
markup, consistent with the button-in-button DOM-nesting warnings), so
getByRole('button', { name: 'Add Condition' }) could never resolve.
Give both addRule renderers an explicit aria-label, which wins accessible
name computation regardless of nesting. The JSONLogic variant was icon-only
with no label at all — a genuine accessibility gap for screen readers too.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| the typed input — with items only on the ListBox, react-aria ≥1.17 | ||
| loses the options' textValue and any typed filter matches nothing, | ||
| which closes the popup and reverts the input. */} | ||
| <AriaComboBox defaultItems={items} menuTrigger="focus" {...otherProps}> |
There was a problem hiding this comment.
defaultItems makes this collection uncontrolled after mount, so async callers cannot reliably push later result sets into the popup. OMSelectWidget updates its local items state after asyncFetch(search) resolves and passes that state into Select.ComboBox, but this wrapper now forwards it as defaultItems instead of a live items collection. When a user searches owner, tag, tier, or custom-property values, the request can return matches while React Aria keeps filtering the initial collection, leaving the returned value impossible to select.
| 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(',')]); |
There was a problem hiding this comment.
This reconciliation still only runs when the saved value string changes, but it reads allItems. On an async multiselect remount, selectedItems starts from static options and this effect can run before loadAsync('') fills allItems. When the async options later arrive, the saved owner, tag, tier, or custom-property value has not changed, so the chip is never appended even though the query tree still contains the filter.
| const result = await asyncFetch(search); | ||
| if (requestId === requestIdRef.current) { | ||
| setAllItems( |
There was a problem hiding this comment.
This path still requests and stores only the first async result set. The async list fetcher can return pagination metadata and accept an offset for later pages, but loadAsync calls asyncFetch(search) without an offset, discards the returned pagination state, and replaces allItems with result.values. For enum or custom-property multiselects with more values than the first response, users cannot browse or select later values unless they already know text that narrows the search into that first response.
| size="sm" | ||
| type="number" | ||
| value={value !== null && value !== undefined ? String(value) : ''} | ||
| onChange={(v: string) => setValue(v === '' ? null : Number(v))} |
There was a problem hiding this comment.
This still writes Number(v) for every non-empty edit. During normal number entry, browser inputs can emit transient strings such as -, ., or 1e; those convert to NaN and are passed into RAQB state. The rule can then serialize with an invalid numeric value or produce a filter that never matches. Keep the transient text local until it parses to a finite number, or avoid calling setValue with NaN.
| the typed input — with items only on the ListBox, react-aria ≥1.17 | ||
| loses the options' textValue and any typed filter matches nothing, | ||
| which closes the popup and reverts the input. */} | ||
| <AriaComboBox defaultItems={items} menuTrigger="focus" {...otherProps}> |
There was a problem hiding this comment.
defaultItems is still an initial-only collection, so async callers cannot push later result sets into this ComboBox. OMSelectWidget updates its items state after asyncFetch(search) resolves and passes the new array to Select.ComboBox, but this wrapper forwards it only as defaultItems. When a user types into an async-backed owner, tag, tier, or custom-property single-select, the request can return matching values while the popup keeps filtering the collection from mount, so the returned value cannot be selected.
| const result = await asyncFetch(search); | ||
| if (requestId === requestIdRef.current) { | ||
| setAllItems( | ||
| (result.values as ListItem[]).map((item) => ({ | ||
| id: String(item.value), | ||
| label: String(item.title ?? item.value), | ||
| })) | ||
| ); |
There was a problem hiding this comment.
The multiselect still requests only the first async page. Enum and custom-property fetchers accept an offset and can return hasMore, but this code always calls asyncFetch(search) with the default offset and then replaces allItems with result.values. For fields with more than the first response page, users still cannot browse or select later values unless their search narrows the value into page one.
| 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(',')]); |
There was a problem hiding this comment.
This reconciliation reads allItems, but it only reruns when the saved value string changes. On mount, allItems starts from staticItems, so saved owner, tag, tier, or custom-property ids that arrive from loadAsync('') are not found during the first pass. When the async options later update allItems, the saved value is unchanged, so the query tree can still contain a filter that renders with no selected chip.
| size="sm" | ||
| type="number" | ||
| value={value !== null && value !== undefined ? String(value) : ''} | ||
| onChange={(v: string) => setValue(v === '' ? null : Number(v))} |
There was a problem hiding this comment.
This still writes Number(v) for every non-empty edit. During normal number entry, browser inputs can emit transient strings such as -, ., or 1e; those convert to NaN and are passed into RAQB state. The rule can then serialize with an invalid numeric value or produce a filter that never matches.
| settings: { | ||
| ...QbBasicConfig.settings, | ||
| renderField: (props) => <OMFieldSelect {...props} />, | ||
| renderOperator: (props) => <OMFieldSelect {...props} />, |
There was a problem hiding this comment.
renderOperator is still wired to OMFieldSelect, whose selection handler calls setField(...). The operator renderer is reached when RAQB needs to change a rule operator, so the selected operator must go through the operator-change callback instead of the field-change callback. Changing an operator such as == to != can call a missing field callback or update the wrong rule slot, leaving the generated query inconsistent with the UI.
…en leak
The DataContracts semantic tests regressed intermittently with the same
signature the select-popover isNonModal fix resolved: an app-level button
('Add New Rule') present in the DOM but unresolvable by role queries for
minutes — the accessibility tree hidden by a leaked ariaHideOutside.
The select-family popovers were fixed, but Dropdown (e.g. the Add Contract
menu), the general-purpose application Popover, and the nav account card
popover were still modal. Any of them unmounting abruptly (navigation
while open or mid-close) skips the aria-hidden restore and leaves the page
invisible to the accessibility tree — intermittent by timing, which
matches these tests passing one run and failing the next. Menus and
lightweight panels are non-modal per the ARIA patterns anyway.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
|
Summary
@react-awesome-query-builder/antdimports with@react-awesome-query-builder/ui(API-identical, same version) across 14 production files and 8 test filesOMConfigfromBasicConfigwith new widget factories backed byopenmetadata-ui-core-components, eliminating Ant Design widget rendering inside query builder rulesQueryBuilderWidgetV1outer shell (Card, Alert, Skeleton, Divider, Typography) from antd to core-componentsAdvancedSearchUtilsandQueryBuilderUtilsfrom antdButton+@ant-design/iconsto coreButton+@untitledui/iconsNew widgets (
src/utils/queryBuilderWidgets/)OMTextWidgetInputOMNumberWidgetInput(numeric)OMSelectWidgetSelectwith async adapterOMMultiSelectWidgetMultiSelect+useListDatafromreact-statelyOMBooleanWidgetToggleOMDateWidget<input type="date/datetime-local/time">(coreDateInputis not publicly exported and requires@internationalized/dateobjects incompatible with RAQB string values)OMFieldSelectSelect(field/operator picker)OMConjsButtonGroup(AND/OR conjunction)All assembled into
src/utils/QueryBuilderOMConfig.tsxwhich exportsOMConfig.Test plan
QueryBuilderWidgetV1tests passsrc/utils/queryBuilderWidgets/)🤖 Generated with Claude Code
Greptile Summary
This PR migrates the query builder stack from
@react-awesome-query-builder/antdto@react-awesome-query-builder/uiand replaces all Ant Design widget rendering with customOM*widgets backed byopenmetadata-ui-core-components. It also migrates the outer shell (QueryBuilderWidgetV1) and button renderers inAdvancedSearchUtils/QueryBuilderUtilsfrom antd to core components.OMTextWidget,OMNumberWidget,OMSelectWidget,OMMultiSelectWidget,OMBooleanWidget,OMDateWidget,OMFieldSelect,OMConjs) are assembled intoQueryBuilderOMConfig.tsxand registered as theOMConfigbase configuration used by bothAdvancedSearchClassBaseandJSONLogicSearchClassBase.combobox.tsxchange moves theitemscollection fromAriaListBoxtoAriaComboBoxasdefaultItemsto fix a React Aria ≥1.17 filter-matching regression; playwright tests and E2E specs are updated to match the new component selectors.Confidence Score: 4/5
The core widget migration is structurally sound, but several behavioral issues raised across many prior review rounds remain open in the current revision.
The new widget components are well-structured and the import migration from antd is clean. However, a cluster of behavioral issues flagged in previous review rounds — async option staleness in ComboBox, saved-chip reconciliation in OMMultiSelectWidget, NaN propagation in OMNumberWidget, and operator-callback routing in OMConfig — are still present. These affect real user workflows (loading saved filters, editing operators, entering numeric values). The newly found debounce gap in OMSelectWidget and OMMultiSelectWidget is a further regression on async-field performance.
The cluster of new widget files under
src/utils/queryBuilderWidgets/(particularlyOMSelectWidget,OMMultiSelectWidget,OMNumberWidget, andOMFieldSelect) andsrc/utils/QueryBuilderOMConfig.tsxandcombobox.tsxin core-components need the most attention before this is ready to land.Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[QueryBuilderWidgetV1 / ContractSemanticFormTab] --> B[OMConfig / QueryBuilderOMConfig] B --> C[settings.renderField → OMFieldSelect] B --> D[settings.renderOperator → OMFieldSelect ⚠️] B --> E[settings.renderConjs → OMConjs] B --> F[widgets.text/textarea → OMTextWidget] B --> G[widgets.number → OMNumberWidget] B --> H[widgets.select → OMSelectWidget] B --> I[widgets.multiselect → OMMultiSelectWidget] B --> J[widgets.boolean → OMBooleanWidget] B --> K[widgets.date/time/datetime → OMDateWidget] H --> L{useAsyncSearch?} L -- Yes --> M[Select.ComboBox] L -- No --> N[Select] M --> O[combobox.tsx → AriaComboBox defaultItems=items ⚠️] I --> P[MultiSelect + useListData] C --> Q[Select.ComboBox with setField] D --> Q Q --> R[onSelectionChange → setField ⚠️ should be setOperator for renderOperator] style D fill:#ffcccc style O fill:#ffcccc style R fill:#ffcccc%%{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"}}}%% flowchart TD A[QueryBuilderWidgetV1 / ContractSemanticFormTab] --> B[OMConfig / QueryBuilderOMConfig] B --> C[settings.renderField → OMFieldSelect] B --> D[settings.renderOperator → OMFieldSelect ⚠️] B --> E[settings.renderConjs → OMConjs] B --> F[widgets.text/textarea → OMTextWidget] B --> G[widgets.number → OMNumberWidget] B --> H[widgets.select → OMSelectWidget] B --> I[widgets.multiselect → OMMultiSelectWidget] B --> J[widgets.boolean → OMBooleanWidget] B --> K[widgets.date/time/datetime → OMDateWidget] H --> L{useAsyncSearch?} L -- Yes --> M[Select.ComboBox] L -- No --> N[Select] M --> O[combobox.tsx → AriaComboBox defaultItems=items ⚠️] I --> P[MultiSelect + useListData] C --> Q[Select.ComboBox with setField] D --> Q Q --> R[onSelectionChange → setField ⚠️ should be setOperator for renderOperator] style D fill:#ffcccc style O fill:#ffcccc style R fill:#ffccccReviews (22): Last reviewed commit: "fix(ui-core): make remaining popovers no..." | Re-trigger Greptile