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: [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]
