gabotorresruiz commented on code in PR #43405:
URL: https://github.com/apache/superset/pull/43405#discussion_r3875113529
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.ts:
##########
@@ -48,9 +48,10 @@ export const checkIsMissingRequiredValue = (
filter: FilterElement,
filterState?: FilterState,
) => {
- const isRequired =
- !!filter.controlValues?.enableEmptyFilter ||
- !!filter.controlValues?.defaultToFirstItem;
+ // Only `enableEmptyFilter` ("Filter value is required") makes a value
+ // mandatory. `defaultToFirstItem` merely seeds an initial selection, so a
+ // filter that has been cleared by the user must stay clearable and
appliable.
Review Comment:
Just a small NIT: "appliable" reads oddly. Copilot suggested "applicable",
but that would change the meaning; what you mean is that the filter must remain
possible to apply. Maybe something like "...must stay clearable, with Apply
enabled." Not a blocker.
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -430,26 +429,6 @@ export default function PluginFilterSelect(props:
PluginFilterSelectProps) {
clearAllTrigger,
]);
- useEffect(() => {
- const prev = prevDataRef.current;
- const curr = data;
-
- const hasDataChanged =
- prev?.length !== curr?.length ||
- prev?.some((row, i) => {
- const prevVal = row[col];
- const currVal = curr[i][col];
- return typeof prevVal === 'bigint' || typeof currVal === 'bigint'
- ? prevVal?.toString() !== currVal?.toString()
- : prevVal !== currVal;
- });
-
- // If data actually changed (e.g., due to parent filter), reset flag
- if (hasDataChanged) {
- prevDataRef.current = data;
- }
- }, [data, col]);
-
useEffect(() => {
Review Comment:
Not a blocker, pre-existing behavior, but worth recording since it touches
this PR's headline scenario: `userClearedRef` is component-local, so it does
not survive a dashboard reload. I mounted the plugin fresh with a persisted
cleared value (`filterState.value = null`): an independent `defaultToFirstItem`
filter stays cleared, but a dependent one (non-empty `extraFormData`)
re-applies the first item through this effect, because
`!userClearedRef.current` is true again after the remount. I get the same
result on master, so nothing regressed here. Do you think a follow-up should
also skip this effect when `filterState.value === null` (an explicitly cleared
state), or is re-defaulting on reload the intended semantics for dependent
filters?
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -1393,6 +1395,94 @@ test('preserves dependent filter value restored from URL
when it exists in data'
});
});
+test('keeps a dependent filter empty after the user clears it', async () => {
Review Comment:
Not a blocker, just for the record: I ran this test against master's
`SelectFilterPlugin.tsx` and it passes there too, so it pins the existing
`userClearedRef` round-trip behavior rather than the `utils.ts` change (the
three new utils and FilterControl tests are the ones that fail pre-fix). Still
worth keeping as a guard for the dead-effect removal.
--
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]
