You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "ObservabilityTeam (via GitHub)" <gi...@apache.org> on 2025/07/11 14:19:27 UTC
[PR] fix(dashboard): adds dependent filters select first value fixes [superset]
ObservabilityTeam opened a new pull request, #34137:
URL: https://github.com/apache/superset/pull/34137
* adds a fix for correctly selecting first value of a dependent filter
* adds a fix for automatically selecting the correct dependent filter value on changing parent filter value
* adds dependent filters fix support for time range filter
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
These fixes address issues with dependent filters using the "Select first filter value by default" option:
- FilterValue.tsx: Prevents child filters from requesting data before their parent values are set. The request is now delayed until all parent filter data is available, ensuring correct first value selection.
- SelectFilterPlugin: Ensures the first value is correctly updated when parent filter values change. A post-check hook verifies external data changes and selects the appropriate default.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
1. Create a dashboard with two filters and some charts these filters are applied to.
2. Make both filters required and "Select first filter value as default".
3. Make one of the filters dependent (child) on the other.
4. Make sure the first value of the dependent filter is not available with the first value of the other filter.
5. Load dashboard in UI.
6. First value of the dependent (child) filter is NOT chosen based on it's parent filter(s)
7. If you change the value of the parent filter(s), the first value of the child does not change which should not be the case because it should reflect changes based on it's parent(s) value.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: Fixes #[22979](https://github.com/apache/superset/issues/22979)
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): adds dependent filters select first value fixes [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): adds dependent filters select first value fixes [superset]
Posted by "ObservabilityTeam (via GitHub)" <gi...@apache.org>.
ObservabilityTeam commented on PR #34137:
URL: https://github.com/apache/superset/pull/34137#issuecomment-3078262308
Hi @betodealmeida @eschutho β the `PR Lint / lint-check` job was cancelled and I donβt have permissions to re-run it.
Could one of you re-trigger the check? Thanks!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): adds dependent filters select first value fixes [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #34137:
URL: https://github.com/apache/superset/pull/34137#discussion_r2204110745
##########
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:
Thank you for clarifying. You're right - with only one possible time range filter, the current counting approach is correct.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): adds dependent filters select first value fixes [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #34137:
URL: https://github.com/apache/superset/pull/34137#discussion_r2204059679
##########
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:
Understood, you're right - this optimization is outside the PR's focus on dependent filters fixes. Let's keep the review centered on the core functionality changes.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): adds dependent filter select first value fixes [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #34137:
URL: https://github.com/apache/superset/pull/34137#discussion_r2215882487
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -368,6 +372,57 @@
inverseSelection,
]);
+ useEffect(() => {
+ const prev = prevDataRef.current;
+ const curr = data;
+
+ const stringifySafe = (val: unknown) =>
+ typeof val === 'bigint' ? val.toString() : val;
+
+ const hasDataChanged =
+ prev?.length !== curr?.length ||
+ JSON.stringify(prev?.map(row => stringifySafe(row[col]))) !==
+ JSON.stringify(curr?.map(row => stringifySafe(row[col])));
Review Comment:
### Inefficient Data Change Detection <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Unnecessary JSON string comparisons and repeated mapping operations to check for data changes.
###### Why this matters
The current approach creates intermediate arrays and JSON strings on every check, consuming memory and CPU cycles unnecessarily.
###### Suggested change β *Feature Preview*
Replace with direct value comparison in a single loop:
```typescript
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;
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/09fee2e6-e944-4e83-b02a-98fce013d13d/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/09fee2e6-e944-4e83-b02a-98fce013d13d?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/09fee2e6-e944-4e83-b02a-98fce013d13d?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/09fee2e6-e944-4e83-b02a-98fce013d13d?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/09fee2e6-e944-4e83-b02a-98fce013d13d)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:26394264-e8fa-4fdb-83b5-c7bfb7552bf5 -->
[](26394264-e8fa-4fdb-83b5-c7bfb7552bf5)
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -368,6 +372,57 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
inverseSelection,
]);
+ useEffect(() => {
+ const prev = prevDataRef.current;
+ const curr = data;
+
+ const stringifySafe = (val: unknown) =>
+ typeof val === 'bigint' ? val.toString() : val;
Review Comment:
### Incomplete JSON stringification error handling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The stringifySafe function handles bigint values but doesn't handle other potential JSON.stringify issues like circular references or undefined values.
###### Why this matters
If the data contains circular references or undefined values, JSON.stringify will throw errors that are not caught, potentially causing the component to crash.
###### Suggested change β *Feature Preview*
Add a try-catch block and handle other edge cases:
```typescript
const stringifySafe = (val: unknown) => {
try {
if (val === undefined) return null;
if (typeof val === 'bigint') return val.toString();
return val;
} catch (error) {
console.warn('Error stringifying value:', error);
return null;
}
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6cb7627e-ecfa-4b14-814c-dea577478534/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6cb7627e-ecfa-4b14-814c-dea577478534?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6cb7627e-ecfa-4b14-814c-dea577478534?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6cb7627e-ecfa-4b14-814c-dea577478534?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6cb7627e-ecfa-4b14-814c-dea577478534)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ab2b9121-8257-432c-aa6f-16e6b6ada074 -->
[](ab2b9121-8257-432c-aa6f-16e6b6ada074)
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -368,6 +372,57 @@
inverseSelection,
]);
+ useEffect(() => {
+ const prev = prevDataRef.current;
+ const curr = data;
+
+ const stringifySafe = (val: unknown) =>
+ typeof val === 'bigint' ? val.toString() : val;
+
+ const hasDataChanged =
+ prev?.length !== curr?.length ||
+ JSON.stringify(prev?.map(row => stringifySafe(row[col]))) !==
+ JSON.stringify(curr?.map(row => stringifySafe(row[col])));
+
+ // If data actually changed (e.g., due to parent filter), reset flag
+ if (hasDataChanged) {
+ isChangedByUser.current = false;
+ prevDataRef.current = data;
+ }
+ }, [data, col]);
+
+ useEffect(() => {
+ if (
+ isChangedByUser.current &&
+ filterState.value &&
+ data.some(row => row[col] === filterState.value[0])
+ )
+ return;
Review Comment:
### Incomplete Multi-select Value Validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The early return condition checks only the first value (filterState.value[0]) against the data rows, which is incorrect for multi-select filters where filterState.value could contain multiple values.
###### Why this matters
If a user has selected multiple values, the filter might unnecessarily reset to the first item even when some of the selected values are still valid in the updated data.
###### Suggested change β *Feature Preview*
Modify the condition to check all selected values against the data:
```typescript
if (
isChangedByUser.current &&
filterState.value &&
filterState.value.every(value => data.some(row => row[col] === value))
)
return;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/efe5fa10-2bdb-4351-a339-2821312c7776/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/efe5fa10-2bdb-4351-a339-2821312c7776?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/efe5fa10-2bdb-4351-a339-2821312c7776?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/efe5fa10-2bdb-4351-a339-2821312c7776?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/efe5fa10-2bdb-4351-a339-2821312c7776)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:506cb3bf-39ab-47f5-b935-ca21bb0e68ef -->
[](506cb3bf-39ab-47f5-b935-ca21bb0e68ef)
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx:
##########
@@ -155,6 +155,37 @@ const FilterValue: FC<FilterControlProps> = ({
dashboardId,
});
const filterOwnState = filter.dataMask?.ownState || {};
+ // if (Object.keys(dataMaskSelected))
+ if (filter?.cascadeParentIds?.length) {
+ // Prevent unnecessary backend requests by validating parent filter selections first
+
+ let selectedParentFilterValueCounts = 0;
+
+ filter?.cascadeParentIds?.forEach(pId => {
+ if (
+ dataMaskSelected?.[pId]?.extraFormData?.filters ||
+ dataMaskSelected?.[pId]?.extraFormData?.time_range
+ ) {
+ selectedParentFilterValueCounts +=
+ dataMaskSelected?.[pId]?.extraFormData?.filters?.length ?? 1;
+ }
+ });
Review Comment:
### Incorrect Parent Filter Count Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The parent filter count logic incorrectly handles time_range filters. When a time_range filter exists, it's counted as 1 in depsCount but might not be counted in selectedParentFilterValueCounts if filters array exists.
###### Why this matters
This discrepancy can lead to the filter never receiving updates because selectedParentFilterValueCounts will never match depsCount when both time_range and filters exist for a parent filter.
###### Suggested change β *Feature Preview*
Modify the counter logic to properly handle time_range filters:
```typescript
let selectedParentFilterValueCounts = 0;
filter?.cascadeParentIds?.forEach(pId => {
const extraFormData = dataMaskSelected?.[pId]?.extraFormData;
if (extraFormData?.filters?.length) {
selectedParentFilterValueCounts += extraFormData.filters.length;
} else if (extraFormData?.time_range) {
selectedParentFilterValueCounts += 1;
}
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8adc6c92-1b01-476b-8ab6-f86adf2d251c/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8adc6c92-1b01-476b-8ab6-f86adf2d251c?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8adc6c92-1b01-476b-8ab6-f86adf2d251c?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8adc6c92-1b01-476b-8ab6-f86adf2d251c?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8adc6c92-1b01-476b-8ab6-f86adf2d251c)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b9c10a72-03a9-4e55-8ca5-749fbece6911 -->
[](b9c10a72-03a9-4e55-8ca5-749fbece6911)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): adds dependent filter select first value fixes [superset]
Posted by "ObservabilityTeam (via GitHub)" <gi...@apache.org>.
ObservabilityTeam commented on PR #34137:
URL: https://github.com/apache/superset/pull/34137#issuecomment-3135410246
@rusackas, thanks for the approval.
I have added the eslint fix. Please let me know if there is anything else that needs to be done for the merge.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): adds dependent filters select first value fixes [superset]
Posted by "ObservabilityTeam (via GitHub)" <gi...@apache.org>.
ObservabilityTeam commented on code in PR #34137:
URL: https://github.com/apache/superset/pull/34137#discussion_r2204058210
##########
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:
Out of PR's scope
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): adds dependent filters select first value fixes [superset]
Posted by "ObservabilityTeam (via GitHub)" <gi...@apache.org>.
ObservabilityTeam commented on code in PR #34137:
URL: https://github.com/apache/superset/pull/34137#discussion_r2204059663
##########
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:
Incorrect. The "defaultDataMask" is being used to load the filters on first dashboard render.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): adds dependent filter select first value fixes [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #34137:
URL: https://github.com/apache/superset/pull/34137
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): adds dependent filters select first value fixes [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #34137:
URL: https://github.com/apache/superset/pull/34137#discussion_r2204060722
##########
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:
I see - defaultDataMask is indeed the correct choice here since this handles the initial filter loading state. Thanks for the clarification.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): adds dependent filters select first value fixes [superset]
Posted by "ObservabilityTeam (via GitHub)" <gi...@apache.org>.
ObservabilityTeam commented on code in PR #34137:
URL: https://github.com/apache/superset/pull/34137#discussion_r2204109365
##########
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:
There can only one time range filter as far as I know at a time on a dashboard.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org