bito-code-review[bot] commented on code in PR #44069:
URL: https://github.com/apache/superset/pull/44069#discussion_r4068355265
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -505,13 +513,63 @@ export default function PluginFilterSelect(props:
PluginFilterSelectProps) {
label: undefined,
},
});
-
- updateDataMask(null);
+ // A Search-all query lives in ownState.search and a debounced onSearch
+ // callback may still be pending. Cancel and reset both so the option
+ // list is not silently re-scoped to a stale search term while the
+ // filter shows an empty input. Only search-all filters carry a
+ // server-side search at all: a plain filter re-emitting ownState at
+ // staging (pre-Apply) would reload its options before Apply is clicked.
+ if (searchAllOptions) {
+ onSearch.cancel();
+ dispatchDataMask({
+ type: 'ownState',
+ ownState: { search: '' },
+ });
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>stale search after reset</b></div>
<div id="fix">
For a plain (non-search-all) filter, `onSearch.cancel()` is now skipped, so
a pending debounced `onSearch` still fires `setSearch(staleTerm)` after
`resetFilter` runs `setSearch('')`. The stale term then re-adds a creatable
option in `options`. Keep `onSearch.cancel()` unconditional and guard only the
`ownState` dispatch.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
onSearch.cancel();
if (searchAllOptions) {
dispatchDataMask({
type: 'ownState',
ownState: { search: '' },
});
}
````
</div>
</details>
</div>
<small><i>Code Review Run #828899</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -315,6 +319,90 @@ const FilterBar: FC<FiltersBarProps> = ({
const hasRequiredValue = isRequired && isEmptyValue;
+ // Cascade clearing: when a parent filter's value changes, every
+ // transitive descendant (child) dependent filter must have its
+ // selection reset. Otherwise the child keeps a stale value that no
+ // longer belongs to the parent's option set (e.g. Country=UK with a
+ // City value only valid under USA), producing impossible filter
+ // combinations that blank charts.
+ const prevMask = draft[filter.id];
+ const prevValue = prevMask?.filterState?.value;
+ const prevExtra = prevMask?.extraFormData;
+ const nextExtra = baseDataMask.extraFormData;
+ // Filters configured with defaultToFirstItem auto-select their first
+ // option on load. That seed is initialization, not a dependency
+ // change, and must not clear descendants. Persisted values reach the
+ // applied state through the sync effect rather than this callback, so
+ // any other first emission is a genuine user selection.
+ const isAutoSeedInit =
+ prevValue === undefined &&
!!filter.controlValues?.defaultToFirstItem;
+ // A filter being (re)initialized from persisted state re-emits its own
+ // saved mask on mount: first the reducer's empty extraFormData, then
+ // its saved clauses. Those synchronization emissions are not user
+ // changes and must not cascade-clear descendants, or opening a
+ // dashboard with a saved parent/child combination would wipe the child
+ // with no user action. The parent only counts as "live" once it has
+ // been initialized (received a value with non-empty extraFormData) or
+ // when it transitions from an empty/cleared state into a real
+ // selection.
+ const isInitializationEmission =
+ !initializedFilters.has(filter.id) &&
+ prevValue !== undefined &&
+ prevValue !== null;
+ // The effective dependency state is the parent's extraFormData (the
+ // clauses and time_range merged into descendants), not the raw
+ // selected value: inverse-selection toggles change the clause while
+ // the selected value stays identical.
+ const parentValueChanged =
+ !!prevMask &&
+ !isAutoSeedInit &&
+ !isInitializationEmission &&
+ !isEqual(prevExtra, nextExtra);
+ if (parentValueChanged) {
+ const childIds = resolveTransitiveChildIds(filter.id, filters);
+ childIds.forEach(childId => {
+ const childMask = draft[childId];
+ if (!childMask) return;
+ const childFilter = filters[childId];
+ const childInScope = inScopeFilterIds.has(childId);
+ childMask.extraFormData = {};
+ const { filterState } = childMask;
+ if (filterState) {
+ const childIsRequired =
+ !!childFilter?.controlValues?.enableEmptyFilter;
+ // A defaultToFirstItem child stages undefined (not null) so the
+ // Select plugin's init effect re-seeds the first option of the
+ // newly-scoped set: clearing it to null would leave it empty
even
+ // though its whole purpose is to resolve to the first value.
+ // Mirror handleClearAll otherwise: range filters use [null,
null]
+ // as the canonical cleared value. Bare null would be ignored by
+ // RangeFilterPlugin's sync effect, leaving stale UI.
+ filterState.value =
childFilter?.controlValues?.defaultToFirstItem
+ ? undefined
+ : childFilter?.filterType === 'filter_range'
+ ? [null, null]
+ : null;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated cleared-value mapping</b></div>
<div id="fix">
This nested ternary re-encodes the cleared-value mapping that
`handleClearAll` already owns at line 619 (`filter_range` -> `[null, null]`,
else `null`), adding a third `defaultToFirstItem` -> `undefined` branch. The
two copies have already diverged intentionally, so a future fourth filter-type
branch must be edited in both places. A shared helper with a
`keepDefaultToFirst` flag (same shape as `resetFilter`'s option in
`SelectFilterPlugin.tsx`) would keep the canonical mapping in one place.
</div>
</div>
<small><i>Code Review Run #828899</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
