codeant-ai-for-open-source[bot] commented on code in PR #40470:
URL: https://github.com/apache/superset/pull/40470#discussion_r3347825879
##########
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,
+ }
+ : {}),
};
Review Comment:
**Suggestion:** The restore condition now treats a required filter with a
valid loaded value but empty `extraFormData` as a signal to fully replace both
state fields with the default. This can overwrite a permalink/user-selected
value and force the default value instead. Keep the loaded `filterState` when
it has a real value, and only repair missing `extraFormData` without replacing
the selected value. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Required filter permalinks reload with defaults, ignoring selections.
- ⚠️ Users lose confidence in dashboard filter permalink reliability.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `src/dashboard/containers/DashboardPage.tsx:185-203`, the
`getDataMaskApplied`
effect loads a `dataMask` object from permalink or URL params and later
dispatches
`hydrateDashboard({ history, dashboard, charts, activeTabs, dataMask,
chartStates })` at
lines 24-31.
2. The HYDRATE_DASHBOARD action from `src/dashboard/actions/hydrate.ts:73`
is handled by
`dataMaskReducer` in `src/dataMask/reducer.ts:255-289`, which calls
`fillNativeFilters(metadata?.native_filter_configuration ?? [], cleanState,
draft,
loadedDataMask)` with the loaded `dataMask` as `loadedDataMask`.
3. Inside `fillNativeFilters` in `src/dataMask/reducer.ts:103-178`, for a
required filter
(`filter.controlValues?.enableEmptyFilter` truthy) whose
`filter.defaultDataMask` contains
non-empty `extraFormData`, and a corresponding `loaded` entry where
`filterState.value`
has a real user selection but `extraFormData` is empty or missing,
`loadedHasValue` is
true, `loadedHasExtraFormData` is false, and `defaultHasExtraFormData` is
true, so
`shouldRestoreDefault` at lines 141-144 evaluates to `true`.
4. Because `shouldRestoreDefault` is `true`, the assignment at lines 146-155
overwrites
both `filterState` and `extraFormData` with `filter.defaultDataMask` values,
replacing the
user-selected `filterState.value` from the permalink with the default value,
so when the
dashboard hydrates the required filter shows and applies the default instead
of the
persisted user selection.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=32dca2e05c9543938301742b2e314e3a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=32dca2e05c9543938301742b2e314e3a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dataMask/reducer.ts
**Line:** 141:156
**Comment:**
*Incorrect Condition Logic: The restore condition now treats a required
filter with a valid loaded value but empty `extraFormData` as a signal to fully
replace both state fields with the default. This can overwrite a
permalink/user-selected value and force the default value instead. Keep the
loaded `filterState` when it has a real value, and only repair missing
`extraFormData` without replacing the selected value.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40470&comment_hash=970315740599d017e232ba91646d726b7ca72a9bcf79246a67b91feeff636915&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40470&comment_hash=970315740599d017e232ba91646d726b7ca72a9bcf79246a67b91feeff636915&reaction=dislike'>👎</a>
##########
superset-frontend/src/dataMask/reducer.ts:
##########
@@ -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);
Review Comment:
**Suggestion:** The new preserve-state condition now excludes non-required,
non-`defaultToFirstItem` filters even when targets are unchanged. That causes
users to lose their current selection after editing unrelated filter settings.
Preserve existing state for unchanged-target filters generally, and apply the
new `hasExistingValue` guard only to the required-filter branch. [incorrect
condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Editing optional filter settings clears current selection unexpectedly.
- ⚠️ Dashboards show unfiltered data after innocuous filter edits.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. A dashboard editor opens the native filters configuration modal
(`FiltersConfigModal`
in
`src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx`)
and
edits an existing optional filter (with `controlValues.enableEmptyFilter`
falsy and
`controlValues.defaultToFirstItem` falsy) without changing its `targets`—for
example, only
updating the filter name or description.
2. Saving the modal creates a `SaveFilterChangesType` and dispatches
`setFilterConfiguration(filterChanges)` in
`src/dashboard/actions/nativeFilters.ts:71-88`,
which issues the `/api/v1/dashboard/{id}/filters` PUT request and on success
dispatches
`setDataMaskForFilterChangesComplete(filterChanges, oldFilters)` at line 97,
where
`oldFilters` is the previous filters config from Redux.
3. The `setDataMaskForFilterChangesComplete` action is handled by
`dataMaskReducer` in
`src/dataMask/reducer.ts:364-372`, which invokes
`updateDataMaskForFilterChanges(filterChanges, cleanState, draft,
action.filters,
action.isCustomizationChanges)`; here `draft` holds the current `dataMask`
including the
user's existing `filterState` for the edited filter.
4. Inside `updateDataMaskForFilterChanges` in
`src/dataMask/reducer.ts:180-253`, for the
modified optional filter `isRequired` and `isFirstItem` are both false; even
though
`areTargetsEqual` is true and `existingFilter.filterState.value` contains
the user's
selection, the `shouldPreserveState` condition at lines 215-219 evaluates
false, so the
merge at lines 221-230 omits `existingFilter.extraFormData` and
`filterState`, resetting
that filter's dataMask to its default state and discarding the current
selection after
saving unrelated configuration changes.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4676a5eb4a5f4428b4c272553fd5bfcd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4676a5eb4a5f4428b4c272553fd5bfcd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dataMask/reducer.ts
**Line:** 215:219
**Comment:**
*Incorrect Condition Logic: The new preserve-state condition now
excludes non-required, non-`defaultToFirstItem` filters even when targets are
unchanged. That causes users to lose their current selection after editing
unrelated filter settings. Preserve existing state for unchanged-target filters
generally, and apply the new `hasExistingValue` guard only to the
required-filter branch.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40470&comment_hash=02b594d517c4bdecd703f4ae8cef9a4ad66111a4e150e7ef0a53d87b5902efab&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40470&comment_hash=02b594d517c4bdecd703f4ae8cef9a4ad66111a4e150e7ef0a53d87b5902efab&reaction=dislike'>👎</a>
--
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]
