msyavuz commented on code in PR #35317:
URL: https://github.com/apache/superset/pull/35317#discussion_r2442444019
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DependencyList.tsx:
##########
@@ -189,19 +189,43 @@ 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());
+ if (value) {
+ // When enabling dependencies, add a suggestion if no dependencies exist
+ if (!hasDependencies && hasAvailableFilters) {
+ const suggestion = getDependencySuggestion();
+ if (suggestion) {
+ onDependenciesChange([suggestion]);
+ }
+ }
+ // If no available filters or suggestion, keep the checkbox checked but
with empty dependencies
+ // This prevents the checkbox from immediately closing due to lack of
available filters
+ } else {
+ // When disabling dependencies, clear all dependencies
+ onDependenciesChange([]);
Review Comment:
This seems unneccessary. Can we revert this?
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DependencyList.tsx:
##########
@@ -189,19 +189,43 @@ 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());
+ if (value) {
+ // When enabling dependencies, add a suggestion if no dependencies exist
+ if (!hasDependencies && hasAvailableFilters) {
+ const suggestion = getDependencySuggestion();
+ if (suggestion) {
+ onDependenciesChange([suggestion]);
+ }
+ }
+ // If no available filters or suggestion, keep the checkbox checked but
with empty dependencies
+ // This prevents the checkbox from immediately closing due to lack of
available filters
+ } else {
+ // When disabling dependencies, clear all dependencies
+ onDependenciesChange([]);
}
- onDependenciesChange(newDependencies);
};
return (
<MainPanel>
<CollapsibleControl
title={t('Values are dependent on other filters')}
- initialValue={hasDependencies}
+ checked={hasDependencies}
Review Comment:
Are we making this component controlled now?
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -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],
+ );
Review Comment:
Do we need useMemo here? I don't think it improves performance that much.
--
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]
