Fixes 30715: block saving a persona AI context rule with an unfinished condition - #30719
Conversation
The emitted queryFilter now drops a condition that has a field but no value,
because `{"term":{}}` is rejected outright by both search engines. Dropping it
silently on save is its own bug: a rule the user built as "tier is ___" persists
as no filter at all, so it matches everything and pulls the entity cap instead of
the handful of assets they selected.
The rule editor now refuses the save while a condition is unfinished and says so
inline, rather than quietly widening the rule.
QueryBuilderWidgetV1 reports tree validity through a new optional
onValidityChange prop, using the config react-awesome-query-builder has already
extended in its onChange — the same validity RAQB uses internally. Only the
persona rule editor opts in, so Explore and the other query-builder consumers are
untouched.
Playwright covers the three cases, including the regression guard: a rule with no
filter at all must stay saveable, since "empty selects every entity of the
configured type" is documented behaviour.
Relates to #30715.
❌ 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
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| useWatch({ control: form.control, name: 'fullyRendered' }) ?? false; | ||
| const maxAssets = useWatch({ control: form.control, name: 'maxAssets' }); | ||
| const queryFilter = useWatch({ control: form.control, name: 'queryFilter' }); | ||
| const [filterIncomplete, setFilterIncomplete] = useState(false); |
There was a problem hiding this comment.
⚠️ Bug: filterIncomplete state is never reset, leaving stale save-block
filterIncomplete is only ever cleared when the query builder fires onValidityChange(true) from a user interaction (handleChange in QueryBuilderWidgetV1), which does not run on mount. Neither handleDismiss nor the open/reset effect resets it. So if a user starts an unfinished condition (setting filterIncomplete=true), then closes the drawer and reopens it (for a new rule or a different rule), the inline error stays visible and handleSubmit keeps returning early — blocking the save of a rule that has no filter at all, which the PR explicitly guarantees must remain saveable. Reset the flag when the drawer resets/dismisses.
Also add setFilterIncomplete(false) alongside the form.reset() call inside the open/reset useEffect (line ~672) so reopening for a different rule starts clean.:
const handleDismiss = useCallback(() => {
previewRequestRef.current++;
setPreview(undefined);
setMaxAssetsDraft(undefined);
setFilterIncomplete(false);
form.reset(getDefaultRule(rule));
onClose();
}, [form, onClose, rule]);
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| shouldDirty: true, | ||
| }); | ||
| }} | ||
| onValidityChange={(isValid) => setFilterIncomplete(!isValid)} |
There was a problem hiding this comment.
When an unfinished condition marks the filter invalid, dismissing and reopening the editor or changing the entity type resets the filter without resetting filterIncomplete. The submit handler therefore continues rejecting the now-valid or empty rule.
| // nConfig is the config react-awesome-query-builder has already extended, which isValidTree | ||
| // needs. A condition the user started but has not finished reads as invalid, letting the caller | ||
| // block the save instead of silently dropping the condition from the emitted filter. | ||
| props.onValidityChange?.(QbUtils.isValidTree(nTree, nConfig)); |
There was a problem hiding this comment.
When the editor loads an existing rule containing an unfinished persisted condition and the user saves without changing the query tree, validity is never reported because this callback runs only from handleChange. filterIncomplete remains false, so the rule is submitted despite the new validation guard.
Jest test CoverageUI tests summary
|
|
🔴 Playwright Results — workflow failedValidated commit ✅ 575 passed · ❌ 1 failed · 🟡 0 flaky · ⏭️ 5 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 49m 7s ⏱️ Max setup 3m 1s · max shard execution 17m 3s · max shard-job elapsed before upload 20m 5s · reporting 6s 🌐 197.28 requests/attempt · 2.75 app boots/UI scenario · 28.54% common-shard skew Optimization targets still in progress:
Genuine Failures (failed on all attempts)❌
|
1 participant
Describe your changes:
Relates to #30715. Stacked on #30716 — base is that PR's branch, so this diff shows only the follow-up. Retarget to
mainonce #30716 merges.#30716 stopped a half-built condition from crashing the search:
{"term":{}}is rejected outright by both OpenSearch and Elasticsearch, so the emittedqueryFilternow drops it. That fixes the 500, but leaves a worse-shaped UX bug behind it — dropping the condition silently on save. A rule the user built as "tier is ___" persists as no filter at all, so it matches everything and pulls the entity cap instead of the handful of assets they picked. Nothing tells them.So the editor now refuses the save while a condition is unfinished, and says so inline, rather than quietly widening the rule. Either the condition is in, or it is out.
QueryBuilderWidgetV1reports tree validity through a new optionalonValidityChangeprop. It usesQbUtils.isValidTreewith the config react-awesome-query-builder has already extended in its ownonChange— the same validity RAQB uses internally, so operators with no value (is_nulland friends) are not false-flagged. Only the persona rule editor opts in; Explore and the other query-builder consumers are untouched.The backend prune from #30716 stays regardless. It is the safety net, not the UX: rules with
{"term":{}}are already persisted inpersona.contextDefinitiontoday and a UI fix does not heal them, andPOST /aiContext/rulesis a public API that must not 500 on bad input.Type of change:
High-level design:
N/A — small change.
Tests:
Use cases covered
POST /aiContext/rulesis fired.Unit tests
QueryBuilderElasticsearchFormatUtils,AdvancedSearchClassBase,RuleQueryBuilderField,AdvancedAssetsFilterField,QueryBuilderWidget,QueryBuilderWidgetV1,ContractSemanticFormTab, and everyPersonaAIContextsuite.Playwright (UI) tests
openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/PersonaAIContext.spec.ts:blocks saving a rule whose condition has no value enteredsaves a rule that has no filter conditions at allclears the filter error once the unfinished condition is removedWhy Playwright and not jest — please read before reviewing. Every jest suite in this repo mocks react-awesome-query-builder wholesale (
jest.mock('@react-awesome-query-builder/antd')inQueryBuilderWidgetV1.test.tsx), soQbUtilsis a stub there and a unit test would assert nothing about realisValidTreebehaviour. Building the real config standalone does not work either — OM'sgetTreeConfigoutput is not complete until RAQB's<Query>extends it, soConfigUtils.extendConfigthrows on it. Playwright is the only place the real query builder actually runs.The second test is the load-bearing one.
isValidTreeis RAQB's own validity check and I could not verify locally how it treats a tree with no rules. If it reports an empty tree as invalid, this change would block saving every "match everything" rule — which the schema documents as legal ("Empty selects every entity of the configured type") and which would be a worse regression than the bug being fixed. That test exists to fail loudly in CI if that is the case. I have deliberately not run it locally; please watch that job. If it goes red, the fix is to gate the check on the tree being non-empty, not to loosen the test.Backend integration tests
Ingestion integration tests
Manual testing performed
None — flagged deliberately. The local stack was unavailable, which is exactly why the verification is pushed into the Playwright job rather than asserted here.
Lint and type-check were run:
eslinton the changed spec — 0 errors, 0 warnings.yarn lint:playwright— 0 errors (280 pre-existing warnings elsewhere in the suite, none in the changed file).tsc --noEmitreports one error inQueryBuilderWidgetV1.test.tsx:297, confirmed pre-existing on the base commit and untouched by this change.UI screen recording / screenshots:
Not attached — no local stack available to record against. The three Playwright cases document the behaviour end-to-end instead.
i18n
message.persona-context-rule-filter-incompleteadded and synced withyarn i18n. All 19 non-English locales carry a real translation rather than the English placeholderyarn i18ninserts. Notepr-pris Persian, not Portuguese.Checklist:
Fixes <issue-number>: <short explanation>🤖 Generated with Claude Code
Greptile Summary
The PR prevents saving unfinished persona AI-context filters.
Confidence Score: 4/5
The validity lifecycle needs to be fixed before merging because valid rules can remain blocked after resets while invalid persisted rules can bypass the guard without an edit.
Validity is retained after the displayed filter is reset, but it is not initialized when an existing filter tree loads, causing both false save rejection and missed validation.
Files Needing Attention: openmetadata-ui/src/main/resources/ui/src/components/Settings/Persona/PersonaAIContext/ContextRuleEditor/ContextRuleEditor.component.tsx; openmetadata-ui/src/main/resources/ui/src/components/common/QueryBuilderWidgetV1/QueryBuilderWidgetV1.tsx
Important Files Changed
Reviews (1): Last reviewed commit: "fix(ui): block saving a persona rule wit..." | Re-trigger Greptile