Skip to content

Commit 7a872d1

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 7a872d1

3 files changed

Lines changed: 41 additions & 11 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: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,10 +109,33 @@ 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 `loaded.filterState` is empty (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. Non-required filters keep current behavior so a user's
119+
// explicit clear isn't undone.
120+
const isRequired = !!filter.controlValues?.enableEmptyFilter;
121+
const loadedValue = loaded?.filterState?.value;
122+
const loadedHasValue =
123+
loadedValue !== undefined &&
124+
loadedValue !== null &&
125+
!(Array.isArray(loadedValue) && loadedValue.length === 0);
126+
const shouldRestoreDefault =
127+
isRequired && !loadedHasValue && !!filter.defaultDataMask;
128+
112129
mergedDataMask[filter.id] = {
113130
...getInitialDataMask(filter.id), // take initial data
114131
...filter.defaultDataMask, // if something new came from BE - take it
115-
...dataMask[filter.id],
132+
...loaded,
133+
...(shouldRestoreDefault
134+
? {
135+
filterState: filter.defaultDataMask?.filterState,
136+
extraFormData: filter.defaultDataMask?.extraFormData,
137+
}
138+
: {}),
116139
};
117140
if (
118141
currentFilters &&
@@ -155,10 +178,18 @@ function updateDataMaskForFilterChanges(
155178
// Check if targets are equal
156179
const areTargetsEqual = isEqual(prevFilterDef?.targets, filter?.targets);
157180

158-
// Preserve state only if filter exists, has enableEmptyFilter=true and targets match
181+
// Preserve state only when there's actually a value to preserve. Otherwise
182+
// the empty existing state would wipe the (possibly newly-defined) default.
183+
const existingValue = existingFilter?.filterState?.value;
184+
const hasExistingValue =
185+
existingValue !== undefined &&
186+
existingValue !== null &&
187+
!(Array.isArray(existingValue) && existingValue.length === 0);
188+
159189
const shouldPreserveState =
160190
existingFilter &&
161191
areTargetsEqual &&
192+
hasExistingValue &&
162193
(filter.controlValues?.enableEmptyFilter ||
163194
filter.controlValues?.defaultToFirstItem);
164195

0 commit comments

Comments
 (0)