korbit-ai[bot] commented on code in PR #33769:
URL: https://github.com/apache/superset/pull/33769#discussion_r2145565141
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/ScopingTree.tsx:
##########
@@ -83,22 +102,57 @@ const ScopingTree: FC<ScopingTreeProps> = ({
const handleCheck = (checkedKeys: string[]) => {
forceUpdate();
- const scope = findFilterScope(checkedKeys, layout);
+
+ const layerKeys = checkedKeys.filter(key => key.includes('-layer-'));
+ const nonLayerKeys = checkedKeys.filter(key => !key.includes('-layer-'));
+
+ const scope = findFilterScope(nonLayerKeys, layout);
+
+ const parentChartIds = new Set<number>();
+ layerKeys.forEach(layerKey => {
+ const match = layerKey.match(/^chart-(\d+)-layer-\d+$/);
+ if (match) {
+ const chartId = parseInt(match[1], 10);
+ parentChartIds.add(chartId);
+ }
+ });
+
+ parentChartIds.forEach(chartId => {
+ const chartLayoutKey = Object.keys(layout).find(
+ key => layout[key]?.meta?.chartId === chartId,
+ );
+ if (chartLayoutKey && layout[chartLayoutKey]) {
+ const tempScope = findFilterScope(
+ [...nonLayerKeys, chartLayoutKey],
+ layout,
+ );
+ scope.rootPath = tempScope.rootPath;
+ scope.excluded = tempScope.excluded;
+ }
+ });
+
if (chartId !== undefined) {
scope.excluded = [...new Set([...scope.excluded, chartId])];
}
+
updateFormValues({
- scope,
+ scope:
+ layerKeys.length > 0 ? { ...scope, selectedLayers: layerKeys } : scope,
});
};
Review Comment:
### Complex function with multiple responsibilities <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The handleCheck function is too complex and handles multiple
responsibilities, violating the Single Responsibility Principle.
###### Why this matters
Complex functions are harder to maintain, test, and understand. This can
lead to bugs when modifying the code and makes it difficult to reuse parts of
the logic.
###### Suggested change ∙ *Feature Preview*
Split the function into smaller, focused functions:
```typescript
const handleCheck = (checkedKeys: string[]) => {
forceUpdate();
const { layerKeys, nonLayerKeys } = separateKeys(checkedKeys);
const scope = calculateScope(nonLayerKeys, layerKeys, layout, chartId);
updateFormValues(createFormValues(scope, layerKeys));
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6e627128-7052-4d4d-b68e-6ebd7f284c0e/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6e627128-7052-4d4d-b68e-6ebd7f284c0e?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6e627128-7052-4d4d-b68e-6ebd7f284c0e?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6e627128-7052-4d4d-b68e-6ebd7f284c0e?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6e627128-7052-4d4d-b68e-6ebd7f284c0e)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4da27cce-9661-47b8-af18-781fb6e84bbf -->
[](4da27cce-9661-47b8-af18-781fb6e84bbf)
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.ts:
##########
@@ -128,13 +204,37 @@ const checkTreeItem = (
export const getTreeCheckedItems = (
scope: NativeFilterScope,
layout: Layout,
+ selectedLayers?: string[],
) => {
const checkedItems: string[] = [];
checkTreeItem(checkedItems, layout, [...scope.rootPath],
[...scope.excluded]);
+
+ // If we have individual layer selections, exclude their parent charts from
checkedItems
+ // to prevent Tree component from auto-checking all children
+ if (selectedLayers && selectedLayers.length > 0) {
+ const parentChartIds = new Set<number>();
+ selectedLayers.forEach(layerKey => {
+ const match = layerKey.match(/^chart-(\d+)-layer-\d+$/);
Review Comment:
### Regex in Loop <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Regular expression compilation and execution inside a loop for each layer
key.
###### Why this matters
Creating and executing regular expressions in loops is computationally
expensive and can cause performance issues with many iterations.
###### Suggested change ∙ *Feature Preview*
Compile the regex once outside the loop:
```typescript
const LAYER_KEY_REGEX = /^chart-(\d+)-layer-\d+$/;
const parentChartIds = new Set<number>();
selectedLayers.forEach(layerKey => {
const match = layerKey.match(LAYER_KEY_REGEX);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9322d286-131c-46bd-85f8-5350dd1cffe9/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9322d286-131c-46bd-85f8-5350dd1cffe9?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9322d286-131c-46bd-85f8-5350dd1cffe9?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9322d286-131c-46bd-85f8-5350dd1cffe9?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9322d286-131c-46bd-85f8-5350dd1cffe9)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ab8ad736-75ce-4709-9af0-621316a59a62 -->
[](ab8ad736-75ce-4709-9af0-621316a59a62)
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/state.ts:
##########
@@ -43,6 +43,12 @@ export function useFilterScopeTree(
);
const charts = useSelector<RootState, Charts>(({ charts }) => charts);
+
+ // Get slice entities to access deck.gl layer information
+ const sliceEntities = useSelector<RootState, any>(
+ ({ sliceEntities }) => sliceEntities.slices || {},
+ );
Review Comment:
### Silent Error Masking in State Selector <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The selector silently handles potential undefined sliceEntities by using the
OR operator, which masks potential errors and makes debugging harder.
###### Why this matters
If sliceEntities is undefined due to an unexpected state shape, this will
silently return an empty object instead of alerting developers to the root
cause, making it harder to diagnose state management issues.
###### Suggested change ∙ *Feature Preview*
Add explicit type checking and handle the error case appropriately:
```typescript
const sliceEntities = useSelector<RootState, any>(state => {
if (!state.sliceEntities) {
console.warn('sliceEntities not found in state');
return {};
}
return state.sliceEntities.slices || {};
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1d053c4e-2f50-4be5-8b5a-3872ed3c17e6/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1d053c4e-2f50-4be5-8b5a-3872ed3c17e6?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1d053c4e-2f50-4be5-8b5a-3872ed3c17e6?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1d053c4e-2f50-4be5-8b5a-3872ed3c17e6?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1d053c4e-2f50-4be5-8b5a-3872ed3c17e6)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:94d0cf6c-a424-4a2e-acba-8b0b75b40fbf -->
[](94d0cf6c-a424-4a2e-acba-8b0b75b40fbf)
--
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-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
