geido commented on code in PR #40470:
URL: https://github.com/apache/superset/pull/40470#discussion_r3347761996
##########
superset-frontend/src/dataMask/reducer.ts:
##########
@@ -155,12 +197,24 @@ 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 &&
+ !(Array.isArray(existingValue) && existingValue.length === 0);
Review Comment:
Good catch — yes, that was an inconsistency. `fillNativeFilters` treated an
all-null array (`[null, null]`, a range filter's canonical cleared value) as
"no value", but `hasExistingValue` here only checked for an *empty* array. So a
required range filter cleared to `[null, null]` would be considered to "have a
value", its empty state would be preserved, and a newly-defined default would
get wiped — exactly the bug this PR is fixing.
Fixed in de4b75cbc8 by mirroring `loadedHasValue`:
```ts
!(Array.isArray(existingValue) && existingValue.every(v => v === null));
```
`[].every()` is `true`, so the empty-array case is still covered (and
oxlint's `no-useless-length-check` stays satisfied). Added a regression test
that fails on the old logic (`Received [null, null]`, expected the new default)
and passes now.
##########
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))
Review Comment:
This is a false positive. The `length === 0` check was removed because it's
redundant: `[].every(v => v === null)` returns `true` (vacuous truth), so
`Array.isArray(loadedValue) && loadedValue.every(v => v === null)` already
evaluates to `true` for an empty array — empty arrays are still treated as "no
value" and `shouldRestoreDefault` still fires for required filters.
Re-adding `loadedValue.length === 0 ||` would re-introduce the `oxlint
unicorn/no-useless-length-check` error that's currently failing CI on this
file, so I'm not applying the suggestion. (Bito's own top-level reply above
already concluded "the logic is correct.")
--
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]
