Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,57 @@ describe('Native filters', () => {
);
});

it('user cannot create bi-directional dependencies between filters', () => {
prepareDashboardFilters([
{ name: 'region', column: 'region', datasetId: 2 },
{ name: 'country_name', column: 'country_name', datasetId: 2 },
{ name: 'country_code', column: 'country_code', datasetId: 2 },
{ name: 'year', column: 'year', datasetId: 2 },
]);
enterNativeFilterEditModal();

// First, make country_name dependent on region
selectFilter(1);
cy.get(nativeFilters.filterConfigurationSections.displayedSection).within(
() => {
cy.contains('Values are dependent on other filters')
.should('be.visible')
.click();
},
);
addParentFilterWithValue(0, testItems.topTenChart.filterColumnRegion);

// Second, make country_code dependent on country_name
selectFilter(2);
cy.get(nativeFilters.filterConfigurationSections.displayedSection).within(
() => {
cy.contains('Values are dependent on other filters')
.should('be.visible')
.click();
},
);
addParentFilterWithValue(0, testItems.topTenChart.filterColumn);

// Now select region filter and try to add dependency
selectFilter(0);
cy.get(nativeFilters.filterConfigurationSections.displayedSection).within(
() => {
cy.contains('Values are dependent on other filters')
.should('be.visible')
.click();

// Verify that only 'year' is available as dependency for region
// 'country_name' and 'country_code' should not be available (would create circular dependency)
cy.get('input[aria-label^="Limit type"]').click({ force: true });
cy.get('[role="listbox"]').should('be.visible');
cy.get('[role="listbox"]').should('contain', 'year');
cy.get('[role="listbox"]').should('not.contain', 'country_name');
cy.get('[role="listbox"]').should('not.contain', 'country_code');
cy.get('[role="listbox"]').contains('year').click();
},
);
});

it('Dependent filter selects first item based on parent filter selection', () => {
prepareDashboardFilters([
{ name: 'region', column: 'region', datasetId: 2 },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import { useState } from 'react';
import { useState, useEffect } from 'react';
import { styled, t } from '@superset-ui/core';
import { Icons } from '@superset-ui/core/components/Icons';
import { Select } from '@superset-ui/core/components';
Expand Down Expand Up @@ -189,10 +189,28 @@ const DependencyList = ({
const hasAvailableFilters = availableFilters.length > 0;
const hasDependencies = dependencies.length > 0;

// Clean up invalid dependencies when available filters change
useEffect(() => {
if (dependencies.length > 0) {
const availableFilterIds = new Set(availableFilters.map(f => f.value));
const validDependencies = dependencies.filter(dep =>
availableFilterIds.has(dep),
);

// If some dependencies are no longer valid, update the list
if (validDependencies.length !== dependencies.length) {
onDependenciesChange(validDependencies);
}
}
}, [availableFilters, dependencies, onDependenciesChange]);

const onCheckChanged = (value: boolean) => {
const newDependencies: string[] = [];
if (value && !hasDependencies && hasAvailableFilters) {
newDependencies.push(getDependencySuggestion());
const suggestion = getDependencySuggestion();
if (suggestion) {
newDependencies.push(suggestion);
}
}
onDependenciesChange(newDependencies);
};
Expand All @@ -201,7 +219,7 @@ const DependencyList = ({
<MainPanel>
<CollapsibleControl
title={t('Values are dependent on other filters')}
initialValue={hasDependencies}
checked={hasDependencies}
onChange={onCheckChanged}
tooltip={t(
'Values selected in other filters will affect the filter options to only show relevant values',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,10 @@ const FiltersConfigForm = (
return Promise.reject(new Error(t('Pre-filter is required')));
};

const availableFilters = getAvailableFilters(filterId);
const availableFilters = useMemo(
() => getAvailableFilters(filterId),
[getAvailableFilters, filterId, filters],
);
Comment on lines +587 to +590

This comment was marked as resolved.

Comment on lines +587 to +590
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memoization dependency mismatch with function parameters category Functionality

Tell me more
What is the issue?

The useMemo dependency array includes filters but the memoized function getAvailableFilters(filterId) only takes filterId as a parameter, creating a potential mismatch between dependencies and actual function inputs.

Why this matters

This could lead to unnecessary re-computations when filters changes but getAvailableFilters doesn't actually use the updated filters data, or conversely, stale memoized results if getAvailableFilters internally depends on filters data that isn't reflected in its parameters.

Suggested change ∙ Feature Preview

Verify that getAvailableFilters function signature matches its actual dependencies. If it internally uses filters data, it should accept it as a parameter:

const availableFilters = useMemo(
  () => getAvailableFilters(filterId, filters),
  [getAvailableFilters, filterId, filters],
);

Or if filters is not needed, remove it from dependencies:

const availableFilters = useMemo(
  () => getAvailableFilters(filterId),
  [getAvailableFilters, filterId],
);
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

const hasAvailableFilters = availableFilters.length > 0;
const hasTimeDependency = availableFilters
.filter(filter => filter.type === 'filter_time')
Expand Down Expand Up @@ -918,7 +921,8 @@ const FiltersConfigForm = (
children: (
<>
{canDependOnOtherFilters &&
hasAvailableFilters && (
(hasAvailableFilters ||
dependencies.length > 0) && (
Comment on lines 920 to +925

This comment was marked as resolved.

<StyledRowFormItem
expanded={expanded}
name={[
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -319,16 +319,49 @@ function FiltersConfigModal({
);

const getAvailableFilters = useCallback(
(filterId: string) =>
filterIds
(filterId: string) => {
// Build current dependency map
const dependencyMap = new Map<string, string[]>();
const filters = form.getFieldValue('filters');
if (filters) {
Object.keys(filters).forEach(key => {
const formItem = filters[key];
const configItem = filterConfigMap[key];
let array: string[] = [];
if (formItem && 'dependencies' in formItem) {
array = [...formItem.dependencies];
} else if (configItem?.cascadeParentIds) {
array = [...configItem.cascadeParentIds];
}
dependencyMap.set(key, array);
});
Comment on lines +327 to +337

This comment was marked as resolved.

}

return filterIds
.filter(id => id !== filterId)
.filter(id => canBeUsedAsDependency(id))
.filter(id => {
// Check if adding this dependency would create a circular dependency
const currentDependencies = dependencyMap.get(filterId) || [];
const testDependencies = [...currentDependencies, id];
const testMap = new Map(dependencyMap);
testMap.set(filterId, testDependencies);
Comment on lines +346 to +348

This comment was marked as resolved.

return !hasCircularDependency(testMap, filterId);
})
.map(id => ({
label: getFilterTitle(id),
value: id,
type: filterConfigMap[id]?.filterType,
})),
[canBeUsedAsDependency, filterConfigMap, filterIds, getFilterTitle],
}));
},
[
canBeUsedAsDependency,
filterConfigMap,
filterIds,
getFilterTitle,
form,
form.getFieldValue('filters'),
],
);

/**
Expand Down
Loading