korbit-ai[bot] commented on code in PR #34137:
URL: https://github.com/apache/superset/pull/34137#discussion_r2200871312
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -368,6 +372,47 @@
inverseSelection,
]);
+ useEffect(() => {
+ const prev = prevDataRef.current;
+ const curr = data;
+
+ const prevFirst = prev?.[0]?.[col];
+ const currFirst = curr?.[0]?.[col];
+
+ // If data actually changed (e.g., due to parent filter), reset flag
+ if (prevFirst !== currFirst) {
+ isChangedByUser.current = false;
+ prevDataRef.current = data;
+ }
+ }, [data, col]);
+
+ useEffect(() => {
+ if (isChangedByUser.current) return;
+
+ const firstItem: SelectValue = data[0]
+ ? (groupby.map(col => data[0][col]) as string[])
+ : null;
+
+ if (
+ defaultToFirstItem &&
+ Object.keys(formData?.extraFormData || {}).length &&
+ filterState.value !== undefined &&
+ firstItem !== null &&
+ filterState.value !== firstItem
+ ) {
+ if (firstItem?.[0] !== undefined) {
+ updateDataMask(firstItem);
+ }
+ }
+ }, [
+ defaultToFirstItem,
+ updateDataMask,
+ formData,
+ data,
+ JSON.stringify(filterState.value),
+ isChangedByUser.current,
+ ]);
+
useEffect(() => {
setDataMask(dataMask);
}, [JSON.stringify(dataMask)]);
Review Comment:
### Inefficient JSON.stringify in Dependency Array <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using JSON.stringify in a useEffect dependency array causes unnecessary
re-renders as it creates a new string on every render.
###### Why this matters
This can lead to performance degradation, especially with large data
structures, as the effect will run on every render even when the actual data
hasn't changed.
###### Suggested change ∙ *Feature Preview*
Replace JSON.stringify with a proper dependency tracking or use a deep
comparison utility:
```typescript
import { isEqual } from 'lodash';
const prevDataMaskRef = useRef(dataMask);
useEffect(() => {
if (!isEqual(prevDataMaskRef.current, dataMask)) {
setDataMask(dataMask);
prevDataMaskRef.current = dataMask;
}
}, [dataMask]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f2d0b19a-10c8-48c3-b706-17646589cd43/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f2d0b19a-10c8-48c3-b706-17646589cd43?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f2d0b19a-10c8-48c3-b706-17646589cd43?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f2d0b19a-10c8-48c3-b706-17646589cd43?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f2d0b19a-10c8-48c3-b706-17646589cd43)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f9a872eb-77c3-4d7f-bd31-0bca7820df88 -->
[](f9a872eb-77c3-4d7f-bd31-0bca7820df88)
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -368,6 +372,47 @@
inverseSelection,
]);
+ useEffect(() => {
+ const prev = prevDataRef.current;
+ const curr = data;
+
+ const prevFirst = prev?.[0]?.[col];
+ const currFirst = curr?.[0]?.[col];
Review Comment:
### Incomplete data change detection <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The data change detection is only comparing the first item of the datasets,
which might miss changes in the rest of the data.
###### Why this matters
If the first item remains the same but other items change, the filter won't
reset the user change flag, potentially leading to stale filter states.
###### Suggested change ∙ *Feature Preview*
Compare the full datasets or use a more comprehensive comparison:
```typescript
useEffect(() => {
const prev = prevDataRef.current;
const curr = data;
// Compare dataset lengths and content
const hasDataChanged =
prev?.length !== curr?.length ||
JSON.stringify(prev?.map(row => row[col])) !==
JSON.stringify(curr?.map(row => row[col]));
if (hasDataChanged) {
isChangedByUser.current = false;
prevDataRef.current = data;
}
}, [data, col]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2e12257c-d246-4cb1-a7dc-4e64f52f171b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2e12257c-d246-4cb1-a7dc-4e64f52f171b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2e12257c-d246-4cb1-a7dc-4e64f52f171b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2e12257c-d246-4cb1-a7dc-4e64f52f171b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2e12257c-d246-4cb1-a7dc-4e64f52f171b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9544bca0-d696-40ae-8a2b-bf81ede2ea41 -->
[](9544bca0-d696-40ae-8a2b-bf81ede2ea41)
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -368,6 +372,47 @@ export default function PluginFilterSelect(props:
PluginFilterSelectProps) {
inverseSelection,
]);
+ useEffect(() => {
+ const prev = prevDataRef.current;
+ const curr = data;
+
+ const prevFirst = prev?.[0]?.[col];
+ const currFirst = curr?.[0]?.[col];
+
+ // If data actually changed (e.g., due to parent filter), reset flag
+ if (prevFirst !== currFirst) {
+ isChangedByUser.current = false;
+ prevDataRef.current = data;
+ }
+ }, [data, col]);
+
+ useEffect(() => {
+ if (isChangedByUser.current) return;
Review Comment:
### Filter state not updating after parent filter changes <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The early return in the useEffect hook prevents the firstItem selection
logic from running when isChangedByUser is true, which could lead to
inconsistent filter states.
###### Why this matters
If a parent filter changes and invalidates the current selection, the filter
should update even if the user had previously made a selection.
###### Suggested change ∙ *Feature Preview*
Add a condition to check if the current value is still valid in the data set:
```typescript
useEffect(() => {
if (isChangedByUser.current && filterState.value && data.some(row =>
row[col] === filterState.value[0])) return;
const firstItem: SelectValue = data[0]
? (groupby.map(col => data[0][col]) as string[])
: null;
if (
defaultToFirstItem &&
Object.keys(formData?.extraFormData || {}).length &&
filterState.value !== undefined &&
firstItem !== null &&
filterState.value !== firstItem
) {
if (firstItem?.[0] !== undefined) {
updateDataMask(firstItem);
}
}
}, [defaultToFirstItem, updateDataMask, formData, data,
JSON.stringify(filterState.value), isChangedByUser.current]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0dc3499-68e6-4163-af9a-3511218adc31/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0dc3499-68e6-4163-af9a-3511218adc31?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0dc3499-68e6-4163-af9a-3511218adc31?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0dc3499-68e6-4163-af9a-3511218adc31?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0dc3499-68e6-4163-af9a-3511218adc31)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f62ee924-05c4-4ca2-9fe3-7ec1d8b0a892 -->
[](f62ee924-05c4-4ca2-9fe3-7ec1d8b0a892)
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx:
##########
@@ -155,6 +157,37 @@
dashboardId,
});
const filterOwnState = filter.dataMask?.ownState || {};
+ if (filter?.cascadeParentIds?.length) {
+ // check if it is a child filter
+
+ let selectedParentFilterValueCounts = 0;
+
+ filter?.cascadeParentIds?.forEach(pId => {
+ if (
+ allFilters[pId]?.controlValues?.defaultToFirstItem ||
+ allFilters[pId]?.defaultDataMask?.extraFormData?.filters ||
+ allFilters[pId]?.defaultDataMask?.extraFormData?.time_range
+ ) {
+ selectedParentFilterValueCounts +=
+ allFilters[pId]?.defaultDataMask?.extraFormData?.filters?.length
?? 1;
+ }
+ });
Review Comment:
### Parent Filter Value Count Uses Default Instead of Current State
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code incorrectly counts parent filter values by using the filters length
from defaultDataMask instead of the current dataMask, which could lead to
incorrect dependency calculations.
###### Why this matters
Using defaultDataMask instead of the current dataMask means the code is
checking the default state rather than the current state of the filters,
potentially causing dependent filters to update based on incorrect parent
filter states.
###### Suggested change ∙ *Feature Preview*
Replace the code with a check against the current dataMask:
```typescript
filter?.cascadeParentIds?.forEach(pId => {
if (
allFilters[pId]?.controlValues?.defaultToFirstItem ||
allFilters[pId]?.dataMask?.extraFormData?.filters ||
allFilters[pId]?.dataMask?.extraFormData?.time_range
) {
selectedParentFilterValueCounts +=
allFilters[pId]?.dataMask?.extraFormData?.filters?.length ?? 1;
}
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/250e64b7-a2a1-4f1d-be4b-a3b08d0bcba4/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/250e64b7-a2a1-4f1d-be4b-a3b08d0bcba4?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/250e64b7-a2a1-4f1d-be4b-a3b08d0bcba4?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/250e64b7-a2a1-4f1d-be4b-a3b08d0bcba4?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/250e64b7-a2a1-4f1d-be4b-a3b08d0bcba4)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f6e897fe-4e15-4c5e-ab93-c1a925ce670d -->
[](f6e897fe-4e15-4c5e-ab93-c1a925ce670d)
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx:
##########
@@ -155,6 +157,37 @@
dashboardId,
});
const filterOwnState = filter.dataMask?.ownState || {};
+ if (filter?.cascadeParentIds?.length) {
+ // check if it is a child filter
+
+ let selectedParentFilterValueCounts = 0;
+
+ filter?.cascadeParentIds?.forEach(pId => {
+ if (
+ allFilters[pId]?.controlValues?.defaultToFirstItem ||
+ allFilters[pId]?.defaultDataMask?.extraFormData?.filters ||
+ allFilters[pId]?.defaultDataMask?.extraFormData?.time_range
+ ) {
+ selectedParentFilterValueCounts +=
+ allFilters[pId]?.defaultDataMask?.extraFormData?.filters?.length
?? 1;
+ }
+ });
+
+ // check if all parent filters with defaults have a value selected
+
+ let depsCount = dependencies.filters?.length ?? 0;
+
+ if (dependencies?.time_range) {
+ depsCount += 1;
+ }
Review Comment:
### Inaccurate Time Range Dependency Counting <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The dependency counting logic assumes a time_range always counts as 1
dependency, which may not accurately reflect multiple time range filters or
complex time range selections.
###### Why this matters
This oversimplified counting could cause filters to update prematurely or
incorrectly when dealing with complex time range dependencies.
###### Suggested change ∙ *Feature Preview*
Implement a more precise time range dependency counting:
```typescript
let depsCount = dependencies.filters?.length ?? 0;
if (dependencies?.time_range) {
const timeRangeValues = Object.keys(dependencies.time_range).length;
depsCount += timeRangeValues > 0 ? timeRangeValues : 1;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8f91f6be-bf09-47df-9422-2246033f2d48/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8f91f6be-bf09-47df-9422-2246033f2d48?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8f91f6be-bf09-47df-9422-2246033f2d48?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8f91f6be-bf09-47df-9422-2246033f2d48?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8f91f6be-bf09-47df-9422-2246033f2d48)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:16984a50-2303-4e15-ac52-009b13cb0fd5 -->
[](16984a50-2303-4e15-ac52-009b13cb0fd5)
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx:
##########
@@ -155,6 +157,37 @@ const FilterValue: FC<FilterControlProps> = ({
dashboardId,
});
const filterOwnState = filter.dataMask?.ownState || {};
+ if (filter?.cascadeParentIds?.length) {
+ // check if it is a child filter
+
+ let selectedParentFilterValueCounts = 0;
Review Comment:
### Improve cascade filter check comment <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The comment 'check if it is a child filter' is redundant as it simply
restates what the code does without explaining why the check is necessary.
###### Why this matters
Without understanding the purpose of this check, future developers may
struggle to maintain or modify this cascade filtering logic correctly.
###### Suggested change ∙ *Feature Preview*
// Prevent unnecessary backend requests by validating parent filter
selections first
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5d97da5-54dc-4154-9248-c4a148e67b23/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5d97da5-54dc-4154-9248-c4a148e67b23?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5d97da5-54dc-4154-9248-c4a148e67b23?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5d97da5-54dc-4154-9248-c4a148e67b23?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5d97da5-54dc-4154-9248-c4a148e67b23)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:c9c81d5d-5633-4803-9a8d-8f7031ad1ecd -->
[](c9c81d5d-5633-4803-9a8d-8f7031ad1ecd)
--
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]
