rusackas commented on code in PR #44069:
URL: https://github.com/apache/superset/pull/44069#discussion_r4161997991
##########
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:
Looks handled by the latest push to me. The cascade now skips the first
emissions from a filter that is still being initialized from saved state (the
empty extraFormData one and the re-emit of its saved clauses), so a restored
Country=USA / City=New York should survive load. Worth making sure the
integration test asserts that before any click.
##########
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:
I think this is covered now. The reset cancels the pending onSearch debounce
and, for search-all filters, resets ownState.search too, and the Select
remounts so it drops any stale search text.
##########
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:
This one looks addressed too. Cascade clear triggers are now only created
for in-scope Select children, so Range/Time descendants never leave an
unconsumed trigger behind.
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -311,6 +315,46 @@ 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 prevValue = draft[filter.id]?.filterState?.value;
+ const nextValue = baseDataMask.filterState?.value;
+ const parentValueChanged =
+ prevValue !== undefined && !isEqual(prevValue, nextValue);
+ if (parentValueChanged) {
+ const childIds = resolveTransitiveChildIds(filter.id, filters);
Review Comment:
Partly handled. Out-of-scope descendants are now staged Apply-safe (no error
status), but getFiltersToApply still only applies an out-of-scope filter if it
has a non-null staged value. So the cleared child never gets dispatched and its
old selection stays applied. I think we need either to apply those cleared
out-of-scope masks on Apply, or to restrict the cascade to in-scope descendants
and invalidate the rest when they enter scope.
##########
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:
Same gap as the in-scope guard thread, I think. The clear gets staged for
the inactive-tab child, but getFiltersToApply skips it because the staged value
is null and it is out of scope, so the old City=New York is still applied when
the tab opens. Needs an Apply-safe path for those cleared masks.
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -315,6 +333,86 @@ 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.
+ filterState.value = getClearedValue(
Review Comment:
This looks real to me. A defaultToFirstItem child is staged as undefined on
cascade clear, so an Apply before its options load commits an undefined value.
When the first option shows up, needsAutoApply bails because the applied value
is undefined, and the control shows a selection the charts are not using.
Probably Apply should wait for those children to resolve, or the auto-apply
check should accept an undefined applied value for 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]
