Skip to content

Commit 0fb8f9f

Browse files
geidoclaude
andcommitted
fix(dashboard): required filters reliably apply default + Apply enables on change
Three small fixes for native filter state desync: 1. utils.ts — remove the selectedCount !== appliedCount early-return in checkIsApplyDisabled (regression from #37681). It disabled Apply whenever Selected had an entry Applied lacked — i.e. precisely when a user typed a value into a filter whose default never made it to Applied. dataEqual with ignoreUndefined already handles the "no real change" case correctly. 2. reducer.ts updateDataMaskForFilterChanges — only preserve existing filterState/extraFormData when there's actually a non-empty value. Previously, modifying a required filter would preserve empty existing state and wipe the newly defined default. 3. reducer.ts fillNativeFilters — on HYDRATE_DASHBOARD the shallow spread of the permalink-loaded dataMask wiped filter.defaultDataMask.filterState when the loaded mask was captured mid-initialization. Now defaults survive when the loaded mask lacks a real value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent df8222f commit 0fb8f9f

3 files changed

Lines changed: 67 additions & 13 deletions

File tree

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.test.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,10 @@ test('checkIsApplyDisabled returns true when required filter is missing value in
299299
);
300300
});
301301

302-
test('checkIsApplyDisabled handles filter count mismatch', () => {
302+
test('checkIsApplyDisabled enables Apply when Selected has a filter value not yet in Applied', () => {
303+
// Regression: when a required filter's default isn't applied (Applied missing
304+
// the entry) and the user types a value, Selected gains an entry Applied
305+
// doesn't have. Apply must be enabled so the user can commit the value.
303306
const dataMaskSelected: DataMaskStateWithId = {
304307
'filter-1': {
305308
id: 'filter-1',
@@ -322,7 +325,7 @@ test('checkIsApplyDisabled handles filter count mismatch', () => {
322325
const filters = [createFilter('filter-1'), createFilter('filter-2')];
323326

324327
expect(checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters)).toBe(
325-
true,
328+
false,
326329
);
327330
});
328331

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.ts

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -74,13 +74,9 @@ export const checkIsApplyDisabled = (
7474
const selectedExtraFormData = getOnlyExtraFormData(dataMaskSelected);
7575
const appliedExtraFormData = getOnlyExtraFormData(dataMaskApplied);
7676

77-
// Check counts first
78-
const selectedCount = Object.keys(selectedExtraFormData).length;
79-
const appliedCount = Object.keys(appliedExtraFormData).length;
80-
81-
if (selectedCount !== appliedCount) return true;
82-
83-
// Check for changes
77+
// Check for changes. ignoreUndefined drops empty keys on both sides so that
78+
// a filter present in Selected with a real value but absent (or undefined)
79+
// in Applied is correctly detected as a change.
8480
const dataEqual = areObjectsEqual(
8581
selectedExtraFormData,
8682
appliedExtraFormData,

superset-frontend/src/dataMask/reducer.ts

Lines changed: 59 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -109,10 +109,53 @@ function fillNativeFilters(
109109
) {
110110
filterConfig.forEach((filter: Filter) => {
111111
const dataMask = initialDataMask || {};
112+
const loaded = dataMask[filter.id];
113+
114+
// The shallow spread of `loaded` would override `filter.defaultDataMask`
115+
// even when the loaded mask is incomplete (e.g. a permalink captured
116+
// mid-initialization), wiping out a valid default. For REQUIRED filters
117+
// with a default, fall back to the default when the loaded mask carries
118+
// no real value OR is missing the extraFormData needed to filter charts.
119+
// Non-required filters keep current behavior so a user's explicit clear
120+
// isn't undone.
121+
const isRequired = !!filter.controlValues?.enableEmptyFilter;
122+
const loadedValue = loaded?.filterState?.value;
123+
const loadedHasValue =
124+
loadedValue !== undefined &&
125+
loadedValue !== null &&
126+
!(
127+
Array.isArray(loadedValue) &&
128+
// Empty array OR an all-null array (range filters use [null, null] as
129+
// their canonical cleared value).
130+
(loadedValue.length === 0 || loadedValue.every(v => v === null))
131+
);
132+
const loadedHasExtraFormData =
133+
!!loaded?.extraFormData &&
134+
Object.keys(loaded.extraFormData).length > 0;
135+
const defaultHasExtraFormData =
136+
!!filter.defaultDataMask?.extraFormData &&
137+
Object.keys(filter.defaultDataMask.extraFormData).length > 0;
138+
// Restore when:
139+
// (1) loaded value is empty — classic "default wiped by stale permalink", OR
140+
// (2) loaded has a value but no extraFormData and the default does — the
141+
// "value present in UI but not applied to charts" gap-window case where
142+
// a permalink was captured before FilterValue produced extraFormData.
143+
const shouldRestoreDefault =
144+
isRequired &&
145+
!!filter.defaultDataMask &&
146+
(!loadedHasValue ||
147+
(!loadedHasExtraFormData && defaultHasExtraFormData));
148+
112149
mergedDataMask[filter.id] = {
113150
...getInitialDataMask(filter.id), // take initial data
114151
...filter.defaultDataMask, // if something new came from BE - take it
115-
...dataMask[filter.id],
152+
...loaded,
153+
...(shouldRestoreDefault
154+
? {
155+
filterState: filter.defaultDataMask?.filterState,
156+
extraFormData: filter.defaultDataMask?.extraFormData,
157+
}
158+
: {}),
116159
};
117160
if (
118161
currentFilters &&
@@ -155,12 +198,24 @@ function updateDataMaskForFilterChanges(
155198
// Check if targets are equal
156199
const areTargetsEqual = isEqual(prevFilterDef?.targets, filter?.targets);
157200

158-
// Preserve state only if filter exists, has enableEmptyFilter=true and targets match
201+
// For required filters, only preserve existing state when it actually has
202+
// a value — otherwise the empty existing state would wipe the (possibly
203+
// newly-defined) default. defaultToFirstItem filters keep the old behavior:
204+
// FilterValue re-resolves the first item at runtime, so preserving the
205+
// mid-init empty state is fine.
206+
const isRequired = !!filter.controlValues?.enableEmptyFilter;
207+
const isFirstItem = !!filter.controlValues?.defaultToFirstItem;
208+
const existingValue = existingFilter?.filterState?.value;
209+
const hasExistingValue =
210+
existingValue !== undefined &&
211+
existingValue !== null &&
212+
!(Array.isArray(existingValue) && existingValue.length === 0);
213+
159214
const shouldPreserveState =
160215
existingFilter &&
161216
areTargetsEqual &&
162-
(filter.controlValues?.enableEmptyFilter ||
163-
filter.controlValues?.defaultToFirstItem);
217+
(isRequired || isFirstItem) &&
218+
(isFirstItem || hasExistingValue);
164219

165220
mergedDataMask[filter.id] = {
166221
...getInitialDataMask(filter.id),

0 commit comments

Comments
 (0)