sadpandajoe commented on code in PR #44069:
URL: https://github.com/apache/superset/pull/44069#discussion_r4018566742
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -311,6 +315,65 @@ 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;
+ // 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 && !isEqual(prevExtra, nextExtra);
+ if (parentValueChanged) {
+ const childIds = resolveTransitiveChildIds(filter.id, filters);
+ childIds.forEach(childId => {
+ // Only cascade-clear descendants that are in scope for the active
+ // tab, mirroring the handleClearAll scope guard. An out-of-scope
+ // child must keep its staged value (Apply would otherwise stage a
+ // null it never dispatches, leaving stale applied state) and its
+ // required-validateStatus (which would wrongly block Apply).
+ if (!inScopeFilterIds.has(childId)) return;
Review Comment:
Skipping an inactive-tab descendant leaves its old selection applied after
this parent change. When the user later opens that tab, no parent transition
reruns this block, so a globally scoped `Country=UK` can still be combined with
the preserved `City=New York` and blank the charts this change is meant to
protect; could skipped descendants be invalidated when they enter scope (or
cleared through an Apply-safe path)?
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -513,6 +515,27 @@ export default function PluginFilterSelect(props:
PluginFilterSelectProps) {
}
}, [clearAllTrigger, onClearAllComplete, updateDataMask]);
+ useEffect(() => {
+ // When a parent filter's value changes, a cascading clear signals this
+ // dependent filter to reset its visual selection. Same behavior as a
+ // global clear-all but scoped to one descendant.
+ if (cascadeClearTrigger) {
+ dispatchDataMask({
+ type: 'filterState',
+ extraFormData: {},
+ filterState: {
+ value: undefined,
+ label: undefined,
+ },
+ });
+
+ updateDataMask(null);
+ setSearch('');
Review Comment:
`setSearch('')` does not clear the Search-all query stored in
`dataMask.ownState`, and a pending `onSearch` debounce can write the old term
back after this effect. A child cleared after `Country` changes can therefore
refetch only the old `New York` matches while showing an empty search box;
should cascade clear cancel `onSearch` and reset `ownState.search` too?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -311,6 +315,65 @@ 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;
+ // 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 && !isEqual(prevExtra, nextExtra);
Review Comment:
This also treats the Select plugin's mount-time synchronization as a parent
change. A restored parent first emits its reducer's initial `extraFormData: {}`
before re-emitting its saved clauses, so opening a dashboard with `Country=USA`
and `City=New York` clears the child without any user action (and the current
integration assertion still passes because it only checks after the click).
Could this transition ignore plugin initialization emissions and only
invalidate descendants after a genuine user change?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -311,6 +315,65 @@ 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;
+ // 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 && !isEqual(prevExtra, nextExtra);
+ if (parentValueChanged) {
+ const childIds = resolveTransitiveChildIds(filter.id, filters);
+ childIds.forEach(childId => {
+ // Only cascade-clear descendants that are in scope for the active
+ // tab, mirroring the handleClearAll scope guard. An out-of-scope
+ // child must keep its staged value (Apply would otherwise stage a
+ // null it never dispatches, leaving stale applied state) and its
+ // required-validateStatus (which would wrongly block Apply).
+ if (!inScopeFilterIds.has(childId)) return;
+ const childMask = draft[childId];
+ if (!childMask) return;
+ childMask.extraFormData = {};
+ const { filterState } = childMask;
+ if (filterState) {
+ const childIsRequired =
+ !!filters[childId]?.controlValues?.enableEmptyFilter;
+ // Mirror handleClearAll: 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 =
+ filters[childId]?.filterType === 'filter_range'
+ ? [null, null]
+ : null;
+ filterState.validateStatus = childIsRequired
+ ? 'error'
+ : undefined;
+ }
+ // Signal the child's filter plugin to clear its visual selection
+ // and avoid re-applying defaults.
+ setCascadeClearTriggers(prev => ({
Review Comment:
Range and Time descendants never call `onCascadeClearComplete`, so this
trigger remains true after their value clears. If that filter is edited to a
Select without remounting the bar, the new Select consumes the stale trigger
and wipes its fresh default (or leaves a required filter invalid); should every
supported dependent type acknowledge the trigger, or should triggers only be
created for Select children?
--
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]
