Re: [PR] feat(deck-gl): Enable individual deck.gl layer selection in FilterScope tree [superset]
geido merged PR #33769: URL: https://github.com/apache/superset/pull/33769 -- 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]
Re: [PR] feat(deck-gl): Enable individual deck.gl layer selection in FilterScope tree [superset]
rebecagbenevides commented on PR #33769: URL: https://github.com/apache/superset/pull/33769#issuecomment-3048808596 Tested and I didn't find any issues related to this PR - QA passed -- 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]
Re: [PR] feat(deck-gl): Enable individual deck.gl layer selection in FilterScope tree [superset]
github-actions[bot] commented on PR #33769: URL: https://github.com/apache/superset/pull/33769#issuecomment-3036651167 @geido Ephemeral environment spinning up at http://35.94.140.242:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. -- 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]
Re: [PR] feat(deck-gl): Enable individual deck.gl layer selection in FilterScope tree [superset]
github-actions[bot] commented on PR #33769: URL: https://github.com/apache/superset/pull/33769#issuecomment-3036606519 @geido Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/16076531061). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments) -- 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]
Re: [PR] feat(deck-gl): Enable individual deck.gl layer selection in FilterScope tree [superset]
korbit-ai[bot] commented on code in PR #33769:
URL: https://github.com/apache/superset/pull/33769#discussion_r2185165798
##
superset-frontend/src/dashboard/util/activeAllDashboardFilters.ts:
##
@@ -51,23 +56,93 @@
}): ActiveFilters => {
const activeFilters: ActiveFilters = {};
- // Combine native filters with cross filters, because they have similar logic
+ const hasLayerSelectionsInAnyFilter = Object.values(dataMask).some(
+({ id: filterId }) => {
+ const selectedLayers = (nativeFilters?.[filterId]?.scope as any)
+?.selectedLayers;
+ return selectedLayers && selectedLayers.length > 0;
+},
+ );
+
+ let masterSelectedLayers: string[] = [];
+ let masterExcluded: number[] = [];
+ if (hasLayerSelectionsInAnyFilter) {
+Object.values(dataMask).forEach(({ id: filterId }) => {
+ const selectedLayers = (nativeFilters?.[filterId]?.scope as any)
+?.selectedLayers;
+ const excluded =
+(nativeFilters?.[filterId]?.scope as any)?.excluded || [];
+ if (selectedLayers && selectedLayers.length > 0) {
+masterSelectedLayers = selectedLayers;
+masterExcluded = excluded;
+ }
+});
+ }
+
Object.values(dataMask).forEach(({ id: filterId, extraFormData = {} }) => {
-const scope =
+let scope =
nativeFilters?.[filterId]?.chartsInScope ??
chartConfiguration?.[parseInt(filterId, 10)]?.crossFilters
?.chartsInScope ??
allSliceIds ??
[];
const filterType = nativeFilters?.[filterId]?.filterType;
-const targets = nativeFilters?.[filterId]?.targets ?? scope;
-// Iterate over all roots to find all affected charts
+const targets = nativeFilters?.[filterId]?.targets;
+
+let selectedLayers = (nativeFilters?.[filterId]?.scope as any)
+ ?.selectedLayers;
+let excludedCharts =
+ (nativeFilters?.[filterId]?.scope as any)?.excluded || [];
+
+if (
+ hasLayerSelectionsInAnyFilter &&
+ (!selectedLayers || selectedLayers.length === 0)
+) {
+ selectedLayers = masterSelectedLayers;
+ excludedCharts = masterExcluded;
+}
+
+let layerScope;
+if (selectedLayers && selectedLayers.length > 0) {
+ layerScope = extractLayerIndicesFromKeys(selectedLayers);
+
+ const explicitlyTargetedCharts = new Set();
+
+ selectedLayers.forEach((selectionKey: string) => {
+const layerMatch = selectionKey.match(/^chart-(\d+)-layer-(\d+)$/);
+if (layerMatch) {
+ explicitlyTargetedCharts.add(parseInt(layerMatch[1], 10));
+}
+ });
Review Comment:
### Duplicate Regex Pattern Matching 
Tell me more
## What is the issue?
Regular expression pattern matching is performed multiple times on the same
strings - once in extractLayerIndicesFromKeys and again in the forEach loop.
## Why this matters
Repeated regex operations are computationally expensive and impact
performance when processing large numbers of layers.
## Suggested change ∙ *Feature Preview*
Cache the regex matches from extractLayerIndicesFromKeys and pass the parsed
values along with the layer map:
```typescript
interface LayerInfo {
layerMap: { [chartId: number]: number[] };
chartIds: Set;
}
```
## Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b317ac6d-934b-463a-ad66-f2d4c20a1e1c/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b317ac6d-934b-463a-ad66-f2d4c20a1e1c?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b317ac6d-934b-463a-ad66-f2d4c20a1e1c?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b317ac6d-934b-463a-ad66-f2d4c20a1e1c?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b317ac6d-934b-463a-ad66-f2d4c20a1e1c)
💬 Looking for more details? Reply to this comment to chat with Korbit.
[](be3af707-cac2-4eda-8cc9-d5a14d333d0e)
##
superset-frontend/src/dashboard/util/activeAllDashboardFilters.ts:
##
@@ -51,23 +56,93 @@ export const getAllActiveFilters = ({
}): ActiveFilters => {
const activeFilters: ActiveFilters = {};
- // Combine native filters with cross filters, because they have similar logic
+ const hasLayerSelectionsInAnyFilter = Object.values(dataMask).some(
+({ id: filterId }) => {
+ const
Re: [PR] feat(deck-gl): Enable individual deck.gl layer selection in FilterScope tree [superset]
github-actions[bot] commented on PR #33769: URL: https://github.com/apache/superset/pull/33769#issuecomment-3035726558 @geido Ephemeral environment spinning up at http://44.243.248.92:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. -- 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]
Re: [PR] feat(deck-gl): Enable individual deck.gl layer selection in FilterScope tree [superset]
github-actions[bot] commented on PR #33769: URL: https://github.com/apache/superset/pull/33769#issuecomment-3035687774 @geido Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/16072545617). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments) -- 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]
Re: [PR] feat(deck-gl): Enable individual deck.gl layer selection in FilterScope tree [superset]
geido commented on PR #33769: URL: https://github.com/apache/superset/pull/33769#issuecomment-3004983757 Left a first-pass comment. Thanks for all the great work here! -- 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]
Re: [PR] feat(deck-gl): Enable individual deck.gl layer selection in FilterScope tree [superset]
geido commented on code in PR #33769:
URL: https://github.com/apache/superset/pull/33769#discussion_r2166727261
##
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/ScopingTree.tsx:
##
@@ -40,8 +48,19 @@ const buildTreeLeafTitle = (
label: string,
hasTooltip: boolean,
tooltipTitle?: string,
+ isDeckMultiChart?: boolean,
) => {
let title = {label};
+
+ if (isDeckMultiChart) {
+title = (
+
+
Review Comment:
Icons have standard sizes that you can leverage in the props. Also, let's
use the `css` prop with template literals over `style`
##
superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.tsx:
##
@@ -111,29 +111,49 @@ const DeckMulti = (props: DeckMultiProps) => {
(formData: QueryFormData, payload: JsonObject, viewport?: Viewport) => {
setViewport(getAdjustedViewport());
setSubSlicesLayers({});
+
payload.data.slices.forEach(
-(subslice: { slice_id: number } & JsonObject) => {
- // Filters applied to multi_deck are passed down to underlying charts
- // note that dashboard contextual information (filter_immune_slices
and such) aren't
- // taken into consideration here
- const extra_filters = [
+(subslice: { slice_id: number } & JsonObject, payloadIndex: number) =>
{
+ const correctLayerIndex = formData.deck_slices
+? formData.deck_slices.indexOf(subslice.slice_id)
+: payloadIndex;
+
+ const layerFilterScope = formData.layer_filter_scope;
+
+ const layerSpecificExtraFilters = [
...(subslice.form_data.extra_filters || []),
...(formData.extra_filters || []),
-...(formData.extra_form_data?.filters || []),
];
- const adhoc_filters = [
+ const layerSpecificAdhocFilters = [
...(formData.adhoc_filters || []),
...(subslice.formData?.adhoc_filters || []),
-...(formData.extra_form_data?.adhoc_filters || []),
];
+ if (layerFilterScope) {
+const filterDataMapping = formData.filter_data_mapping || {};
+
+Object.entries(layerFilterScope).forEach(
+ ([filterId, filterScope]: [string, any]) => {
+if (!filterScope || filterScope.includes(correctLayerIndex)) {
Review Comment:
`ensureIsArray` util in superset ui core could help drying up this a bit
##
superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts:
##
@@ -125,20 +141,49 @@ export default function getFormDataWithExtraFilters({
return cachedFormData;
}
- let extraData: { extra_form_data?: JsonObject } = {};
const activeFilters = getAllActiveFilters({
chartConfiguration,
-dataMask,
nativeFilters,
+dataMask,
allSliceIds,
});
+
+ let extraData: JsonObject = {};
const filterIdsAppliedOnChart = Object.entries(activeFilters)
.filter(([, { scope }]) => scope.includes(chart.id))
.map(([filterId]) => filterId);
+
if (filterIdsAppliedOnChart.length) {
+const aggregatedFormData = getExtraFormData(
+ dataMask,
+ filterIdsAppliedOnChart,
+);
extraData = {
- extra_form_data: getExtraFormData(dataMask, filterIdsAppliedOnChart),
+ extra_form_data: aggregatedFormData,
};
+
+if (chart.form_data?.viz_type === 'deck_multi') {
Review Comment:
I saw we are using this check in a few places and I am wondering if this is
the right way. I am worried that we are tying logics that are meant to be
generic to specific plugins now. Is there a better way to apply the required
logics so that do not feel so tied to the `deck_multi` viz type only?
##
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/ScopingTree.tsx:
##
@@ -18,14 +18,22 @@
*/
import { FC, useMemo, useState, memo } from 'react';
-import { NativeFilterScope } from '@superset-ui/core';
+import { NativeFilterScope, styled } from '@superset-ui/core';
import { Tree } from 'src/components';
import { DASHBOARD_ROOT_ID } from 'src/dashboard/util/constants';
import { Tooltip } from 'src/components/Tooltip';
import { Icons } from 'src/components/Icons';
+import { Layout } from 'src/dashboard/types';
import { useFilterScopeTree } from './state';
import { findFilterScope, getTreeCheckedItems } from './utils';
+const StyledTree = styled(Tree)`
+ .antd5-tree-title {
Review Comment:
When rebasing with master all class names should now use the standard `ant`
prefix. `antd5` prefix is gone as the migration has been finalized.
##
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.ts:
##
@@ -37,6 +37,52 @@ export const getNodeTitle = (node: LayoutItem) =>
node?.id?.toString?.() ??
'';
Re: [PR] feat(deck-gl): Enable individual deck.gl layer selection in FilterScope tree [superset]
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 = ({
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();
+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 
Tell me more
## 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)
💬 Looking for more details? Reply to this comment to chat with Korbit.
[](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();
+selectedLayers.forEach(layerKey => {
+ const match = layerKey.match(/^chart-(\d+)-layer-\d+$/);
Review Comment:
### Regex in Loop 
Tell me more
## 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
[PR] feat(deck-gl): Enable individual deck.gl layer selection in FilterScope tree [superset]
richardfogaca opened a new pull request, #33769: URL: https://github.com/apache/superset/pull/33769 ### SUMMARY 1. Added support for individual deck.gl layer selection in the FilterScope tree 2. Enhanced the tree UI to display deck.gl layers with appropriate icons and styling 3. Modified the filter scope logic to handle layer-specific selections 4. Updated the state management to track selected layers separately from chart selections 5. Added new types and utilities to support deck.gl layer handling ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF https://github.com/user-attachments/assets/06c20866-7cf4-4162-bd15-046786eb6d09"; /> ### TESTING INSTRUCTIONS 1. Create a dashboard with a deck.gl multi-layer chart 2. Add a native filter 3. In the filter's "Scoping" tab: - Expand the deck.gl chart to see its layers - Verify that layers are listed correctly and are independently selectable ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [X] 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 - [X] 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: [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]
