korbit-ai[bot] commented on code in PR #35317:
URL: https://github.com/apache/superset/pull/35317#discussion_r2386264098
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -361,16 +361,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);
+ });
Review Comment:
### Unnecessary array copying in dependency map <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code creates unnecessary array copies with spread operator for every
filter when building the dependency map.
###### Why this matters
Creating new arrays with the spread operator for each filter adds memory
allocation overhead and garbage collection pressure, especially when dealing
with many filters.
###### Suggested change ā *Feature Preview*
Directly assign the arrays without copying since they're not being mutated:
```typescript
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);
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2bd32934-e5a2-46f6-bebf-25ed5c69f3c2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2bd32934-e5a2-46f6-bebf-25ed5c69f3c2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2bd32934-e5a2-46f6-bebf-25ed5c69f3c2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2bd32934-e5a2-46f6-bebf-25ed5c69f3c2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2bd32934-e5a2-46f6-bebf-25ed5c69f3c2)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:676f9b2e-a363-415b-8915-2a01e1b79fe3 -->
[](676f9b2e-a363-415b-8915-2a01e1b79fe3)
##########
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:
### Overly broad useMemo dependencies <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The useMemo dependency array includes `filters` but `getAvailableFilters`
function may not actually depend on the entire `filters` object, potentially
causing unnecessary re-computations.
###### Why this matters
This could lead to performance issues where the memoized value is
recalculated more frequently than needed, defeating the purpose of memoization
and potentially causing UI flickering or lag during filter configuration.
###### Suggested change ā *Feature Preview*
Review what specific parts of `filters` the `getAvailableFilters` function
actually uses and include only those specific dependencies, or remove `filters`
if it's not actually used by the function:
```typescript
const availableFilters = useMemo(
() => getAvailableFilters(filterId),
[getAvailableFilters, filterId], // Remove filters if not needed
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e5887af-1602-4399-86a1-14d56180852d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e5887af-1602-4399-86a1-14d56180852d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e5887af-1602-4399-86a1-14d56180852d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e5887af-1602-4399-86a1-14d56180852d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e5887af-1602-4399-86a1-14d56180852d)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a75464f1-abee-4ca2-9f22-d10faf87d1ef -->
[](a75464f1-abee-4ca2-9f22-d10faf87d1ef)
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -918,7 +921,8 @@ const FiltersConfigForm = (
children: (
<>
{canDependOnOtherFilters &&
- hasAvailableFilters && (
+ (hasAvailableFilters ||
+ dependencies.length > 0) && (
Review Comment:
### Inconsistent dependency UI state <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The condition allows rendering dependency UI when `dependencies.length > 0`
even if there are no available filters, which could lead to a confusing user
experience where dependencies are shown but cannot be modified.
###### Why this matters
Users might see existing dependencies but be unable to add, remove, or
modify them if no filters are available, creating an inconsistent and
potentially frustrating interface state.
###### Suggested change ā *Feature Preview*
Consider separating the logic to handle existing dependencies differently
from adding new ones, or provide clear messaging when dependencies exist but
cannot be modified:
```typescript
{canDependOnOtherFilters && (
hasAvailableFilters ? (
// Full dependency management UI
<DependencyList ... />
) : dependencies.length > 0 ? (
// Read-only view of existing dependencies with explanation
<ReadOnlyDependencyList dependencies={dependencies} />
) : null
)}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/eb17f215-9c67-4b9e-8041-4764b2288396/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/eb17f215-9c67-4b9e-8041-4764b2288396?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/eb17f215-9c67-4b9e-8041-4764b2288396?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/eb17f215-9c67-4b9e-8041-4764b2288396?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/eb17f215-9c67-4b9e-8041-4764b2288396)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ee752082-5e6d-49be-ae5e-565723eaacfb -->
[](ee752082-5e6d-49be-ae5e-565723eaacfb)
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -361,16 +361,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);
+ });
+ }
+
+ 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);
Review Comment:
### Inefficient circular dependency checking <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Creating a new Map and array for each filter being tested for circular
dependencies results in O(nĀ²) space complexity and unnecessary object creation.
###### Why this matters
This approach creates significant memory overhead when filtering many
potential dependencies, as it creates a full copy of the dependency map for
each candidate filter.
###### Suggested change ā *Feature Preview*
Optimize by temporarily modifying the original map and restoring it, or
implement a more efficient cycle detection algorithm that doesn't require full
map copying:
```typescript
.filter(id => {
// Temporarily add the dependency to check for cycles
const currentDependencies = dependencyMap.get(filterId) || [];
dependencyMap.set(filterId, [...currentDependencies, id]);
const hasCycle = hasCircularDependency(dependencyMap, filterId);
// Restore original state
dependencyMap.set(filterId, currentDependencies);
return !hasCycle;
})
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/10d7aac2-3de3-4a9e-ba6e-e6c4a56ba047/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/10d7aac2-3de3-4a9e-ba6e-e6c4a56ba047?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/10d7aac2-3de3-4a9e-ba6e-e6c4a56ba047?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/10d7aac2-3de3-4a9e-ba6e-e6c4a56ba047?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/10d7aac2-3de3-4a9e-ba6e-e6c4a56ba047)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0d645211-42e9-4cec-a860-f1dd2db6267f -->
[](0d645211-42e9-4cec-a860-f1dd2db6267f)
--
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]
