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<number>();
+
+ 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 <sub></sub>
<details>
<summary>Tell me more</summary>
###### 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<number>;
}
```
###### 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)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:be3af707-cac2-4eda-8cc9-d5a14d333d0e -->
[](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 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<number>();
+
+ selectedLayers.forEach((selectionKey: string) => {
+ const layerMatch = selectionKey.match(/^chart-(\d+)-layer-(\d+)$/);
+ if (layerMatch) {
+ explicitlyTargetedCharts.add(parseInt(layerMatch[1], 10));
+ }
Review Comment:
### Missing Integer Parsing Validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code doesn't validate the parsed integers, which could lead to NaN
values being added to explicitlyTargetedCharts if the parsing fails.
###### Why this matters
Invalid chart IDs in the selection keys could cause filtering to malfunction
and potentially crash the application.
###### Suggested change ∙ *Feature Preview*
Add validation for parsed integers:
```typescript
if (layerMatch) {
const chartId = parseInt(layerMatch[1], 10);
if (!Number.isNaN(chartId)) {
explicitlyTargetedCharts.add(chartId);
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc43e7f6-df9d-4c84-99f7-0f4b645c8b1b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc43e7f6-df9d-4c84-99f7-0f4b645c8b1b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc43e7f6-df9d-4c84-99f7-0f4b645c8b1b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc43e7f6-df9d-4c84-99f7-0f4b645c8b1b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc43e7f6-df9d-4c84-99f7-0f4b645c8b1b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:fd0056f3-7ffd-4303-ad5f-f713982aac81 -->
[](fd0056f3-7ffd-4303-ad5f-f713982aac81)
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx:
##########
@@ -768,7 +768,25 @@ function mapStateToProps(state) {
const fieldsToOmit = hasQueryMode
? retainQueryModeRequirements(hiddenFormData)
: Object.keys(hiddenFormData ?? {});
- const form_data = omit(getFormDataFromControls(controls), fieldsToOmit);
+
+ const controlsBasedFormData = omit(
+ getFormDataFromControls(controls),
+ fieldsToOmit,
+ );
+ const isDeckGLChart = explore.form_data?.viz_type === 'deck_multi';
+
+ const form_data = isDeckGLChart
+ ? {
+ ...controlsBasedFormData,
+ ...(explore.form_data?.layer_filter_scope && {
+ layer_filter_scope: explore.form_data.layer_filter_scope,
+ }),
+ ...(explore.form_data?.filter_data_mapping && {
+ filter_data_mapping: explore.form_data.filter_data_mapping,
+ }),
+ }
+ : controlsBasedFormData;
Review Comment:
### Complex nested ternary with spreads <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The ternary operator with nested spreads and conditionals makes the
form_data assignment hard to read at a glance.
###### Why this matters
Complex nested operations reduce code scanability and make it harder to
understand the data transformation flow.
###### Suggested change ∙ *Feature Preview*
```javascript
const getDeckGLFormData = () => {
const formData = { ...controlsBasedFormData };
if (explore.form_data?.layer_filter_scope) {
formData.layer_filter_scope = explore.form_data.layer_filter_scope;
}
if (explore.form_data?.filter_data_mapping) {
formData.filter_data_mapping = explore.form_data.filter_data_mapping;
}
return formData;
};
const form_data = isDeckGLChart ? getDeckGLFormData() :
controlsBasedFormData;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e9a27010-3a08-4e95-a412-f8a2dd39c4bf/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e9a27010-3a08-4e95-a412-f8a2dd39c4bf?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e9a27010-3a08-4e95-a412-f8a2dd39c4bf?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e9a27010-3a08-4e95-a412-f8a2dd39c4bf?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e9a27010-3a08-4e95-a412-f8a2dd39c4bf)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f644a148-63ea-426d-82cf-51d6b12dfd99 -->
[](f644a148-63ea-426d-82cf-51d6b12dfd99)
##########
superset-frontend/src/dashboard/components/nativeFilters/utils.ts:
##########
@@ -192,3 +192,154 @@
}
return t('Unknown value');
};
+
+export interface FilterTarget {
+ type: 'CHART' | 'LAYER';
+ chartId: string;
+ layerId?: string;
+}
+
+export interface FilterScope {
+ filterId: string;
+ targets: FilterTarget[];
+}
+
+const LAYER_KEY_REGEX = /^chart-(\d+)-layer-(\d+)$/;
+const CHART_KEY_REGEX = /^chart-(\d+)$/;
+
+export function parseFilterTarget(scopeKey: string): FilterTarget | null {
+ const layerMatch = scopeKey.match(LAYER_KEY_REGEX);
+ if (layerMatch) {
+ return {
+ type: 'LAYER',
+ chartId: layerMatch[1],
+ layerId: layerMatch[2],
+ };
+ }
+
+ const chartMatch = scopeKey.match(CHART_KEY_REGEX);
+ if (chartMatch) {
+ return {
+ type: 'CHART',
+ chartId: chartMatch[1],
+ };
+ }
+
+ return null;
+}
+
+export function getFilterScope(
+ filterId: string,
+ filterScopes: Record<string, string[]>,
+): FilterScope {
+ const scopeKeys = filterScopes[filterId] || [];
+ const targets: FilterTarget[] = [];
+
+ scopeKeys.forEach(scopeKey => {
+ const target = parseFilterTarget(scopeKey);
+ if (target) {
+ targets.push(target);
+ }
Review Comment:
### Silent Failure in Filter Target Parsing <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Silent failure when parsing invalid filter target keys. The code silently
skips invalid scope keys without logging or alerting.
###### Why this matters
This can make debugging difficult when filter scopes are not being applied
as expected due to malformed keys.
###### Suggested change ∙ *Feature Preview*
```typescript
const target = parseFilterTarget(scopeKey);
if (target) {
targets.push(target);
} else {
console.warn(`Invalid filter scope key format: ${scopeKey}`);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6a8434e2-d2a3-4566-bdac-fe34dc6be72d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6a8434e2-d2a3-4566-bdac-fe34dc6be72d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6a8434e2-d2a3-4566-bdac-fe34dc6be72d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6a8434e2-d2a3-4566-bdac-fe34dc6be72d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6a8434e2-d2a3-4566-bdac-fe34dc6be72d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:95e68ea1-34d1-4e3a-9684-b01603846150 -->
[](95e68ea1-34d1-4e3a-9684-b01603846150)
##########
superset-frontend/src/explore/controlUtils/getFormDataWithDashboardContext.ts:
##########
@@ -204,25 +220,76 @@ export const getFormDataWithDashboardContext = (
.reduce(
(acc, key) => ({
...acc,
- [key]: removeExtraFieldForNewCharts(
- applyTimeRangeFilters(
- dashboardContextFormData,
- removeAdhocFilterDuplicates([
- ...ensureIsArray(exploreFormData[key]),
- ...ensureIsArray(filterBoxData[key]),
- ...ensureIsArray(nativeFiltersData[key]),
- ]),
- ),
- exploreFormData.slice_id === 0,
- ),
+ [key]: (() => {
+ const beforeDuplicates = [
+ ...ensureIsArray(exploreFormData[key]),
+ ...ensureIsArray(filterBoxData[key]),
+ ...ensureIsArray(nativeFiltersData[key]),
+ ];
+
+ const afterDuplicates =
removeAdhocFilterDuplicates(beforeDuplicates);
+
+ const final = removeExtraFieldForNewCharts(
+ applyTimeRangeFilters(dashboardContextFormData, afterDuplicates),
+ exploreFormData.slice_id === 0,
+ );
+
+ return final;
+ })(),
}),
{},
);
-
const ownColorScheme = exploreFormData.color_scheme;
const dashboardColorScheme = dashboardContextFormData.color_scheme;
const appliedColorScheme = dashboardColorScheme || ownColorScheme;
+ const deckGLProperties: JsonObject = {};
+
+ if (
+ isDeckGLChart &&
+ isDefined(deckSlices) &&
+ 'adhoc_filters' in adhocFilters &&
+ Array.isArray(adhocFilters?.adhoc_filters)
+ ) {
+ const adhocFiltersWithDeckSlices =
+ adhocFilters?.adhoc_filters?.map((filter: AdhocFilter) => ({
+ ...filter,
+ ...(Array.isArray(deckSlices) &&
+ deckSlices?.length > 0 &&
+ filter?.isExtra && {
+ deck_slices: deckSlices,
+ }),
+ })) || [];
+
+ adhocFilters.adhoc_filters = adhocFiltersWithDeckSlices;
+ }
+
+ if (isDeckGLChart) {
+ if (dashboardContextFormData.layer_filter_scope) {
+ deckGLProperties.layer_filter_scope =
+ dashboardContextFormData.layer_filter_scope;
+ }
+ if (dashboardContextFormData.filter_data_mapping) {
+ deckGLProperties.filter_data_mapping =
+ dashboardContextFormData.filter_data_mapping;
+ }
+ }
+
+ if (saveAction === 'overwrite') {
+ return {
+ ...dashboardContextFormData,
+ ...filterBoxData,
+ ...nativeFiltersData,
+ ...adhocFilters,
+ ...exploreFormData, // Explore form data comes last to override
+ own_color_scheme: ownColorScheme,
+ color_scheme: appliedColorScheme,
+ dashboard_color_scheme: dashboardColorScheme,
+ ...deckGLProperties,
+ };
+ }
+
+ // Default behavior: dashboard context takes precedence
Review Comment:
### Incomplete precedence explanation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The inline comment doesn't fully explain the return value's object spread
order implications.
###### Why this matters
The order of spread operations is crucial for understanding which values
will override others in the final merged object.
###### Suggested change ∙ *Feature Preview*
// Default behavior: Dashboard context overrides explore data, but adhoc
filters, color schemes
// and specific properties from filterBox and native filters take final
precedence
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9f9976aa-8e95-4e73-be90-7d2a6f474f07/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9f9976aa-8e95-4e73-be90-7d2a6f474f07?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9f9976aa-8e95-4e73-be90-7d2a6f474f07?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9f9976aa-8e95-4e73-be90-7d2a6f474f07?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9f9976aa-8e95-4e73-be90-7d2a6f474f07)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d9c9d4c7-dcd7-436c-bb54-9e6f4e1c9870 -->
[](d9c9d4c7-dcd7-436c-bb54-9e6f4e1c9870)
##########
superset-frontend/src/dashboard/components/nativeFilters/utils.ts:
##########
@@ -192,3 +192,154 @@
}
return t('Unknown value');
};
+
+export interface FilterTarget {
+ type: 'CHART' | 'LAYER';
+ chartId: string;
+ layerId?: string;
+}
+
+export interface FilterScope {
+ filterId: string;
+ targets: FilterTarget[];
+}
+
+const LAYER_KEY_REGEX = /^chart-(\d+)-layer-(\d+)$/;
+const CHART_KEY_REGEX = /^chart-(\d+)$/;
Review Comment:
### Unexplained Regex Patterns <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Complex regex patterns are not explained, making their purpose unclear to
readers.
###### Why this matters
Without understanding the pattern structure, developers will struggle to
maintain or modify these regex patterns if the key format needs to change.
###### Suggested change ∙ *Feature Preview*
Add comments explaining the expected format and examples:
```typescript
// Matches layer keys in format: 'chart-123-layer-456' where 123 is chartId
and 456 is layerId
const LAYER_KEY_REGEX = /^chart-(\d+)-layer-(\d+)$/;
// Matches chart keys in format: 'chart-123' where 123 is chartId
const CHART_KEY_REGEX = /^chart-(\d+)$/;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bad4b639-b6f8-46e6-86b7-880fc2ca7510/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bad4b639-b6f8-46e6-86b7-880fc2ca7510?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bad4b639-b6f8-46e6-86b7-880fc2ca7510?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bad4b639-b6f8-46e6-86b7-880fc2ca7510?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bad4b639-b6f8-46e6-86b7-880fc2ca7510)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2ecdfbf1-5ad3-49b7-b87b-7127366042c9 -->
[](2ecdfbf1-5ad3-49b7-b87b-7127366042c9)
##########
superset-frontend/src/dashboard/components/nativeFilters/utils.ts:
##########
@@ -192,3 +192,154 @@
}
return t('Unknown value');
};
+
+export interface FilterTarget {
+ type: 'CHART' | 'LAYER';
+ chartId: string;
+ layerId?: string;
+}
+
+export interface FilterScope {
+ filterId: string;
+ targets: FilterTarget[];
+}
+
+const LAYER_KEY_REGEX = /^chart-(\d+)-layer-(\d+)$/;
+const CHART_KEY_REGEX = /^chart-(\d+)$/;
+
+export function parseFilterTarget(scopeKey: string): FilterTarget | null {
+ const layerMatch = scopeKey.match(LAYER_KEY_REGEX);
+ if (layerMatch) {
+ return {
+ type: 'LAYER',
+ chartId: layerMatch[1],
+ layerId: layerMatch[2],
+ };
+ }
+
+ const chartMatch = scopeKey.match(CHART_KEY_REGEX);
+ if (chartMatch) {
+ return {
+ type: 'CHART',
+ chartId: chartMatch[1],
+ };
+ }
+
+ return null;
+}
+
+export function getFilterScope(
+ filterId: string,
+ filterScopes: Record<string, string[]>,
+): FilterScope {
+ const scopeKeys = filterScopes[filterId] || [];
+ const targets: FilterTarget[] = [];
+
+ scopeKeys.forEach(scopeKey => {
+ const target = parseFilterTarget(scopeKey);
+ if (target) {
+ targets.push(target);
+ }
+ });
+
+ return {
+ filterId,
+ targets,
+ };
+}
+
+export function aggregateFiltersForTarget(
+ dataMask: DataMaskStateWithId,
+ filterIds: string[],
+): ExtraFormData {
+ let aggregatedFormData: ExtraFormData = {};
+
+ filterIds.forEach(filterId => {
+ const filterData = dataMask[filterId];
+ if (filterData?.extraFormData) {
+ aggregatedFormData = mergeExtraFormData(
+ aggregatedFormData,
+ filterData.extraFormData,
+ );
+ }
+ });
+
+ return aggregatedFormData;
+}
+
+export function groupFiltersByTarget(
+ dataMask: DataMaskStateWithId,
+ filterScopes: Record<string, string[]>,
+): {
+ chartFilters: Map<string, ExtraFormData>;
+ layerFilters: Map<string, ExtraFormData>;
+} {
+ const chartFilters = new Map<string, ExtraFormData>();
+ const layerFilters = new Map<string, ExtraFormData>();
+
+ Object.keys(dataMask).forEach(filterId => {
+ const scope = getFilterScope(filterId, filterScopes);
+
+ scope.targets.forEach(target => {
+ const filterData = dataMask[filterId]?.extraFormData || {};
+ const targetKey =
+ target.type === 'LAYER'
+ ? `${target.chartId}-${target.layerId}`
+ : target.chartId;
Review Comment:
### Complex Key Construction <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The targetKey construction logic is embedded in a ternary operation, making
it hard to understand the key format at a glance.
###### Why this matters
Complex string formatting in ternaries can be difficult to read and
maintain, especially when the format is important for other parts of the code.
###### Suggested change ∙ *Feature Preview*
Extract to a clear, named function:
```typescript
function createTargetKey(target: FilterTarget): string {
if (target.type === 'LAYER') {
return `${target.chartId}-${target.layerId}`;
}
return target.chartId;
}
const targetKey = createTargetKey(target);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9be3ce0-6cdc-4677-8df5-9ec035e3be68/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9be3ce0-6cdc-4677-8df5-9ec035e3be68?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9be3ce0-6cdc-4677-8df5-9ec035e3be68?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9be3ce0-6cdc-4677-8df5-9ec035e3be68?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9be3ce0-6cdc-4677-8df5-9ec035e3be68)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:799b645a-de31-48e3-bcdc-8f32ac2514fa -->
[](799b645a-de31-48e3-bcdc-8f32ac2514fa)
##########
superset-frontend/src/dashboard/components/nativeFilters/utils.ts:
##########
@@ -192,3 +192,154 @@ export const getFilterValueForDisplay = (
}
return t('Unknown value');
};
+
+export interface FilterTarget {
+ type: 'CHART' | 'LAYER';
+ chartId: string;
+ layerId?: string;
+}
+
+export interface FilterScope {
+ filterId: string;
+ targets: FilterTarget[];
+}
+
+const LAYER_KEY_REGEX = /^chart-(\d+)-layer-(\d+)$/;
+const CHART_KEY_REGEX = /^chart-(\d+)$/;
+
+export function parseFilterTarget(scopeKey: string): FilterTarget | null {
+ const layerMatch = scopeKey.match(LAYER_KEY_REGEX);
+ if (layerMatch) {
+ return {
+ type: 'LAYER',
+ chartId: layerMatch[1],
+ layerId: layerMatch[2],
+ };
+ }
+
+ const chartMatch = scopeKey.match(CHART_KEY_REGEX);
+ if (chartMatch) {
+ return {
+ type: 'CHART',
+ chartId: chartMatch[1],
+ };
+ }
+
+ return null;
+}
+
+export function getFilterScope(
+ filterId: string,
+ filterScopes: Record<string, string[]>,
+): FilterScope {
+ const scopeKeys = filterScopes[filterId] || [];
+ const targets: FilterTarget[] = [];
+
+ scopeKeys.forEach(scopeKey => {
+ const target = parseFilterTarget(scopeKey);
+ if (target) {
+ targets.push(target);
+ }
+ });
+
+ return {
+ filterId,
+ targets,
+ };
+}
+
+export function aggregateFiltersForTarget(
+ dataMask: DataMaskStateWithId,
+ filterIds: string[],
+): ExtraFormData {
+ let aggregatedFormData: ExtraFormData = {};
+
+ filterIds.forEach(filterId => {
+ const filterData = dataMask[filterId];
+ if (filterData?.extraFormData) {
+ aggregatedFormData = mergeExtraFormData(
+ aggregatedFormData,
+ filterData.extraFormData,
+ );
+ }
+ });
+
+ return aggregatedFormData;
+}
+
+export function groupFiltersByTarget(
+ dataMask: DataMaskStateWithId,
+ filterScopes: Record<string, string[]>,
+): {
+ chartFilters: Map<string, ExtraFormData>;
+ layerFilters: Map<string, ExtraFormData>;
+} {
+ const chartFilters = new Map<string, ExtraFormData>();
+ const layerFilters = new Map<string, ExtraFormData>();
+
+ Object.keys(dataMask).forEach(filterId => {
+ const scope = getFilterScope(filterId, filterScopes);
+
+ scope.targets.forEach(target => {
+ const filterData = dataMask[filterId]?.extraFormData || {};
+ const targetKey =
+ target.type === 'LAYER'
+ ? `${target.chartId}-${target.layerId}`
+ : target.chartId;
+
+ if (target.type === 'CHART') {
+ const existing = chartFilters.get(targetKey) || {};
+ chartFilters.set(targetKey, mergeExtraFormData(existing, filterData));
+ } else if (target.type === 'LAYER') {
+ const existing = layerFilters.get(targetKey) || {};
+ layerFilters.set(targetKey, mergeExtraFormData(existing, filterData));
+ }
+ });
+ });
+
+ return { chartFilters, layerFilters };
+}
+
+export function buildFilterScopesFromFilters(
+ filters: any,
+): Record<string, string[]> {
+ const filterScopes: Record<string, string[]> = {};
+
+ Object.values(filters).forEach((filter: any) => {
+ if (filter.chartsInScope) {
+ filterScopes[filter.id] = filter.chartsInScope.map(
+ (chartId: number) => `chart-${chartId}`,
+ );
+ }
+ });
Review Comment:
### Unsafe property access without type guards <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The function contains nested type assertions and property access without
proper type guards.
###### Why this matters
Lack of proper type checking can lead to runtime errors if the filter object
doesn't have the expected structure.
###### Suggested change ∙ *Feature Preview*
```typescript
Object.values(filters).forEach((filter: Filter) => {
if (filter.chartsInScope?.length) {
filterScopes[filter.id] = filter.chartsInScope.map(
chartId => `chart-${chartId}`,
);
}
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/670687b9-5fd2-4b70-9f62-bdfd00f2d44b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/670687b9-5fd2-4b70-9f62-bdfd00f2d44b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/670687b9-5fd2-4b70-9f62-bdfd00f2d44b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/670687b9-5fd2-4b70-9f62-bdfd00f2d44b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/670687b9-5fd2-4b70-9f62-bdfd00f2d44b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:3eed7b74-f75d-4788-a56d-9eb1b4c065e1 -->
[](3eed7b74-f75d-4788-a56d-9eb1b4c065e1)
##########
superset-frontend/src/explore/controlUtils/getFormDataWithDashboardContext.ts:
##########
@@ -204,25 +220,76 @@
.reduce(
(acc, key) => ({
...acc,
- [key]: removeExtraFieldForNewCharts(
- applyTimeRangeFilters(
- dashboardContextFormData,
- removeAdhocFilterDuplicates([
- ...ensureIsArray(exploreFormData[key]),
- ...ensureIsArray(filterBoxData[key]),
- ...ensureIsArray(nativeFiltersData[key]),
- ]),
- ),
- exploreFormData.slice_id === 0,
- ),
+ [key]: (() => {
+ const beforeDuplicates = [
+ ...ensureIsArray(exploreFormData[key]),
+ ...ensureIsArray(filterBoxData[key]),
+ ...ensureIsArray(nativeFiltersData[key]),
+ ];
+
+ const afterDuplicates =
removeAdhocFilterDuplicates(beforeDuplicates);
+
+ const final = removeExtraFieldForNewCharts(
+ applyTimeRangeFilters(dashboardContextFormData, afterDuplicates),
+ exploreFormData.slice_id === 0,
+ );
+
+ return final;
+ })(),
}),
{},
);
-
const ownColorScheme = exploreFormData.color_scheme;
const dashboardColorScheme = dashboardContextFormData.color_scheme;
const appliedColorScheme = dashboardColorScheme || ownColorScheme;
+ const deckGLProperties: JsonObject = {};
+
+ if (
+ isDeckGLChart &&
+ isDefined(deckSlices) &&
+ 'adhoc_filters' in adhocFilters &&
+ Array.isArray(adhocFilters?.adhoc_filters)
+ ) {
+ const adhocFiltersWithDeckSlices =
+ adhocFilters?.adhoc_filters?.map((filter: AdhocFilter) => ({
+ ...filter,
+ ...(Array.isArray(deckSlices) &&
+ deckSlices?.length > 0 &&
+ filter?.isExtra && {
+ deck_slices: deckSlices,
+ }),
+ })) || [];
+
+ adhocFilters.adhoc_filters = adhocFiltersWithDeckSlices;
+ }
+
+ if (isDeckGLChart) {
+ if (dashboardContextFormData.layer_filter_scope) {
+ deckGLProperties.layer_filter_scope =
+ dashboardContextFormData.layer_filter_scope;
+ }
+ if (dashboardContextFormData.filter_data_mapping) {
+ deckGLProperties.filter_data_mapping =
+ dashboardContextFormData.filter_data_mapping;
+ }
+ }
+
+ if (saveAction === 'overwrite') {
+ return {
+ ...dashboardContextFormData,
+ ...filterBoxData,
+ ...nativeFiltersData,
+ ...adhocFilters,
+ ...exploreFormData, // Explore form data comes last to override
+ own_color_scheme: ownColorScheme,
+ color_scheme: appliedColorScheme,
+ dashboard_color_scheme: dashboardColorScheme,
+ ...deckGLProperties,
+ };
+ }
Review Comment:
### DeckGL properties override order incorrect <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The order of spreading properties in the overwrite case may override
deck.gl-specific properties needed for layer filtering, as deckGLProperties is
spread last after exploreFormData.
###### Why this matters
The deck.gl layer selection functionality could be broken when saving with
'overwrite' action since the layer_filter_scope and filter_data_mapping might
be overwritten by exploreFormData.
###### Suggested change ∙ *Feature Preview*
Move exploreFormData before deckGLProperties to ensure layer filtering
properties are preserved:
```typescript
if (saveAction === 'overwrite') {
return {
...dashboardContextFormData,
...filterBoxData,
...nativeFiltersData,
...adhocFilters,
...exploreFormData,
own_color_scheme: ownColorScheme,
color_scheme: appliedColorScheme,
dashboard_color_scheme: dashboardColorScheme,
...deckGLProperties, // Move deckGLProperties last to ensure it's not
overridden
};
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9fd3aa60-63e2-4847-b5be-fa4e5f1d78bc/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9fd3aa60-63e2-4847-b5be-fa4e5f1d78bc?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9fd3aa60-63e2-4847-b5be-fa4e5f1d78bc?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9fd3aa60-63e2-4847-b5be-fa4e5f1d78bc?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9fd3aa60-63e2-4847-b5be-fa4e5f1d78bc)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:69a11db6-9fda-4e54-8173-9d24bc0048af -->
[](69a11db6-9fda-4e54-8173-9d24bc0048af)
##########
superset-frontend/src/dashboard/components/nativeFilters/utils.ts:
##########
@@ -192,3 +192,154 @@
}
return t('Unknown value');
};
+
+export interface FilterTarget {
+ type: 'CHART' | 'LAYER';
+ chartId: string;
+ layerId?: string;
+}
+
+export interface FilterScope {
+ filterId: string;
+ targets: FilterTarget[];
+}
+
+const LAYER_KEY_REGEX = /^chart-(\d+)-layer-(\d+)$/;
+const CHART_KEY_REGEX = /^chart-(\d+)$/;
+
+export function parseFilterTarget(scopeKey: string): FilterTarget | null {
+ const layerMatch = scopeKey.match(LAYER_KEY_REGEX);
+ if (layerMatch) {
+ return {
+ type: 'LAYER',
+ chartId: layerMatch[1],
+ layerId: layerMatch[2],
+ };
+ }
+
+ const chartMatch = scopeKey.match(CHART_KEY_REGEX);
+ if (chartMatch) {
+ return {
+ type: 'CHART',
+ chartId: chartMatch[1],
+ };
+ }
+
+ return null;
+}
+
+export function getFilterScope(
+ filterId: string,
+ filterScopes: Record<string, string[]>,
+): FilterScope {
+ const scopeKeys = filterScopes[filterId] || [];
+ const targets: FilterTarget[] = [];
+
+ scopeKeys.forEach(scopeKey => {
+ const target = parseFilterTarget(scopeKey);
+ if (target) {
+ targets.push(target);
+ }
+ });
+
+ return {
+ filterId,
+ targets,
+ };
+}
+
+export function aggregateFiltersForTarget(
+ dataMask: DataMaskStateWithId,
+ filterIds: string[],
+): ExtraFormData {
+ let aggregatedFormData: ExtraFormData = {};
+
+ filterIds.forEach(filterId => {
+ const filterData = dataMask[filterId];
+ if (filterData?.extraFormData) {
+ aggregatedFormData = mergeExtraFormData(
+ aggregatedFormData,
+ filterData.extraFormData,
+ );
+ }
+ });
+
+ return aggregatedFormData;
+}
+
+export function groupFiltersByTarget(
+ dataMask: DataMaskStateWithId,
+ filterScopes: Record<string, string[]>,
+): {
+ chartFilters: Map<string, ExtraFormData>;
+ layerFilters: Map<string, ExtraFormData>;
+} {
+ const chartFilters = new Map<string, ExtraFormData>();
+ const layerFilters = new Map<string, ExtraFormData>();
+
+ Object.keys(dataMask).forEach(filterId => {
+ const scope = getFilterScope(filterId, filterScopes);
+
+ scope.targets.forEach(target => {
+ const filterData = dataMask[filterId]?.extraFormData || {};
+ const targetKey =
+ target.type === 'LAYER'
+ ? `${target.chartId}-${target.layerId}`
+ : target.chartId;
+
+ if (target.type === 'CHART') {
+ const existing = chartFilters.get(targetKey) || {};
+ chartFilters.set(targetKey, mergeExtraFormData(existing, filterData));
+ } else if (target.type === 'LAYER') {
+ const existing = layerFilters.get(targetKey) || {};
+ layerFilters.set(targetKey, mergeExtraFormData(existing, filterData));
+ }
+ });
+ });
+
+ return { chartFilters, layerFilters };
+}
+
+export function buildFilterScopesFromFilters(
+ filters: any,
+): Record<string, string[]> {
+ const filterScopes: Record<string, string[]> = {};
+
+ Object.values(filters).forEach((filter: any) => {
+ if (filter.chartsInScope) {
+ filterScopes[filter.id] = filter.chartsInScope.map(
+ (chartId: number) => `chart-${chartId}`,
+ );
+ }
+ });
+
+ return filterScopes;
+}
+
+export function getLayerSpecificExtraFormData(
+ dataMask: DataMaskStateWithId,
+ filterIds: string[],
+ chartId: number,
+ layerId?: string,
+): ExtraFormData {
+ let extraFormData: ExtraFormData = {};
+
+ if (layerId) {
+ const layerKey = `${chartId}-${layerId}`;
+ const layerFilterData = dataMask[layerKey];
+ if (layerFilterData?.extraFormData) {
+ return layerFilterData.extraFormData;
+ }
Review Comment:
### Layer filters override chart filters instead of merging <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The getLayerSpecificExtraFormData function returns layer-specific filters
without merging them with chart-level filters when layer filters exist.
###### Why this matters
This creates inconsistent filtering behavior where chart-level filters are
completely ignored when layer-specific filters are present, potentially leading
to incorrect data display.
###### Suggested change ∙ *Feature Preview*
Modify the code to merge layer-specific filters with chart-level filters:
```typescript
if (layerId) {
const layerKey = `${chartId}-${layerId}`;
const layerFilterData = dataMask[layerKey];
if (layerFilterData?.extraFormData) {
extraFormData = mergeExtraFormData(extraFormData,
layerFilterData.extraFormData);
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4ef70d58-da2c-4136-97c5-b9151ba6a72b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4ef70d58-da2c-4136-97c5-b9151ba6a72b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4ef70d58-da2c-4136-97c5-b9151ba6a72b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4ef70d58-da2c-4136-97c5-b9151ba6a72b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4ef70d58-da2c-4136-97c5-b9151ba6a72b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8eb5997f-c5fc-4fb4-964d-0a6ce3b89666 -->
[](8eb5997f-c5fc-4fb4-964d-0a6ce3b89666)
--
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]
