Merge remote-tracking branch 'origin/master' into geido/fix-required-filters

Addresses review feedback (@msyavuz): `fillNativeFilters` already treats an
all-null array (a range filter's canonical cleared value) as "no value", but
`updateDataMaskForFilterChanges` only checked for an empty array. A required
range filter cleared to [null, null] would therefore be considered to "have a
value" and its empty state preserved, wiping a newly-defined default — the same
class of bug this PR fixes.

Mirror `loadedHasValue` so both paths agree. `[].every()` is true, so the empty
array case is still covered (and the oxlint no-useless-length-check stays
satisfied). Adds a regression test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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

@@ -299,7 +299,10 @@ test('checkIsApplyDisabled returns true when required filter is missing value in
   );
 });
 
-test('checkIsApplyDisabled handles filter count mismatch', () => {
+test('checkIsApplyDisabled enables Apply when Selected has a filter value not yet in Applied', () => {
+  // Regression: when a required filter's default isn't applied (Applied missing
+  // the entry) and the user types a value, Selected gains an entry Applied
+  // doesn't have. Apply must be enabled so the user can commit the value.
   const dataMaskSelected: DataMaskStateWithId = {
     'filter-1': {
       id: 'filter-1',
@@ -322,7 +325,7 @@ test('checkIsApplyDisabled handles filter count mismatch', () => {
   const filters = [createFilter('filter-1'), createFilter('filter-2')];
 
   expect(checkIsApplyDisabled(dataMaskSelected, dataMaskApplied, filters)).toBe(
-    true,
+    false,
   );
 });
 

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

@@ -74,13 +74,9 @@ export const checkIsApplyDisabled = (
   const selectedExtraFormData = getOnlyExtraFormData(dataMaskSelected);
   const appliedExtraFormData = getOnlyExtraFormData(dataMaskApplied);
 
-  // Check counts first
-  const selectedCount = Object.keys(selectedExtraFormData).length;
-  const appliedCount = Object.keys(appliedExtraFormData).length;
-
-  if (selectedCount !== appliedCount) return true;
-
-  // Check for changes
+  // Check for changes. ignoreUndefined drops empty keys on both sides so that
+  // a filter present in Selected with a real value but absent (or undefined)
+  // in Applied is correctly detected as a change.
   const dataEqual = areObjectsEqual(
     selectedExtraFormData,
     appliedExtraFormData,

superset-frontend/src/dataMask/reducer.test.ts

@@ -121,6 +121,38 @@ test('when user edits a filter without changing targets, their selection is pres
   );
 });
 
+test('when a required range filter was cleared to [null, null], modifying it applies the new default instead of the cleared state', () => {
+  // Regression for the PR #40470 review: [null, null] is a range filter's
+  // canonical "cleared" value. It must count as "no value" so the empty state
+  // does not wipe a newly-defined default — consistent with `loadedHasValue`
+  // in fillNativeFilters.
+  const initialState: DataMaskStateWithId = {
+    'NATIVE_FILTER-1': {
+      id: 'NATIVE_FILTER-1',
+      ...getInitialDataMask('NATIVE_FILTER-1'),
+      filterState: { value: [null, null] },
+    },
+  };
+
+  const oldFilters = {
+    'NATIVE_FILTER-1': createFilter('NATIVE_FILTER-1', 'col_a', {
+      enableEmptyFilter: true,
+    }),
+  };
+
+  const modifiedFilter: Filter = {
+    ...createFilter('NATIVE_FILTER-1', 'col_a', { enableEmptyFilter: true }),
+    defaultDataMask: { filterState: { value: [10, 20] } },
+  };
+
+  const action = createModifyAction(modifiedFilter, oldFilters);
+
+  const result = reducer(initialState, action);
+
+  // The cleared [null, null] state must not be preserved; the new default wins.
+  expect(result['NATIVE_FILTER-1']?.filterState?.value).toEqual([10, 20]);
+});
+
 // Runtime data from the server can contain null entries in
 // chart_customization_config even though the TS type does not include | null
 // yet. These helpers build HYDRATE_DASHBOARD actions that mirror that reality.

superset-frontend/src/dataMask/reducer.ts

@@ -109,10 +109,50 @@ function fillNativeFilters(
 ) {
   filterConfig.forEach((filter: Filter) => {
     const dataMask = initialDataMask || {};
+    const loaded = dataMask[filter.id];
+
+    // The shallow spread of `loaded` would override `filter.defaultDataMask`
+    // even when the loaded mask is incomplete (e.g. a permalink captured
+    // mid-initialization), wiping out a valid default. For REQUIRED filters
+    // with a default, fall back to the default when the loaded mask carries
+    // no real value OR is missing the extraFormData needed to filter charts.
+    // Non-required filters keep current behavior so a user's explicit clear
+    // isn't undone.
+    const isRequired = !!filter.controlValues?.enableEmptyFilter;
+    const loadedValue = loaded?.filterState?.value;
+    const loadedHasValue =
+      loadedValue !== undefined &&
+      loadedValue !== null &&
+      !(
+        // Treat all-null arrays (range filters use [null, null] as their
+        // canonical cleared value) and empty arrays as "no value".
+        (Array.isArray(loadedValue) && loadedValue.every(v => v === null))
+      );
+    const loadedHasExtraFormData =
+      !!loaded?.extraFormData && Object.keys(loaded.extraFormData).length > 0;
+    const defaultHasExtraFormData =
+      !!filter.defaultDataMask?.extraFormData &&
+      Object.keys(filter.defaultDataMask.extraFormData).length > 0;
+    // Restore when:
+    //  (1) loaded value is empty — classic "default wiped by stale permalink", OR
+    //  (2) loaded has a value but no extraFormData and the default does — the
+    //      "value present in UI but not applied to charts" gap-window case where
+    //      a permalink was captured before FilterValue produced extraFormData.
+    const shouldRestoreDefault =
+      isRequired &&
+      !!filter.defaultDataMask &&
+      (!loadedHasValue || (!loadedHasExtraFormData && defaultHasExtraFormData));
+
     mergedDataMask[filter.id] = {
       ...getInitialDataMask(filter.id), // take initial data
       ...filter.defaultDataMask, // if something new came from BE - take it
-      ...dataMask[filter.id],
+      ...loaded,
+      ...(shouldRestoreDefault
+        ? {
+            filterState: filter.defaultDataMask?.filterState,
+            extraFormData: filter.defaultDataMask?.extraFormData,
+          }
+        : {}),
     };
     if (
       currentFilters &&
@@ -155,12 +195,28 @@ function updateDataMaskForFilterChanges(
     // Check if targets are equal
     const areTargetsEqual = isEqual(prevFilterDef?.targets, filter?.targets);
 
-    // Preserve state only if filter exists, has enableEmptyFilter=true and targets match
+    // For required filters, only preserve existing state when it actually has
+    // a value — otherwise the empty existing state would wipe the (possibly
+    // newly-defined) default. defaultToFirstItem filters keep the old behavior:
+    // FilterValue re-resolves the first item at runtime, so preserving the
+    // mid-init empty state is fine.
+    const isRequired = !!filter.controlValues?.enableEmptyFilter;
+    const isFirstItem = !!filter.controlValues?.defaultToFirstItem;
+    const existingValue = existingFilter?.filterState?.value;
+    const hasExistingValue =
+      existingValue !== undefined &&
+      existingValue !== null &&
+      // Treat all-null arrays (range filters use [null, null] as their
+      // canonical cleared value) and empty arrays as "no value", consistent
+      // with `loadedHasValue` in fillNativeFilters above. `[].every()` is
+      // true, so this also covers the empty-array case.
+      !(Array.isArray(existingValue) && existingValue.every(v => v === null));
+
     const shouldPreserveState =
       existingFilter &&
       areTargetsEqual &&
-      (filter.controlValues?.enableEmptyFilter ||
-        filter.controlValues?.defaultToFirstItem);
+      (isRequired || isFirstItem) &&
+      (isFirstItem || hasExistingValue);
 
     mergedDataMask[filter.id] = {
       ...getInitialDataMask(filter.id),