Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 79 additions & 32 deletions superset-frontend/src/dashboard/actions/chartCustomizationActions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { AnyAction } from 'redux';
import { ThunkAction, ThunkDispatch } from 'redux-thunk';
import { t } from '@apache-superset/core';
import { makeApi, getClientErrorObject, DataMask } from '@superset-ui/core';
import { isEqual } from 'lodash';
import { addDangerToast } from 'src/components/MessageToasts/actions';
import { DashboardInfo, RootState } from 'src/dashboard/types';
import {
Expand All @@ -30,6 +31,8 @@ import {
import { triggerQuery } from 'src/components/Chart/chartAction';
import { removeDataMask, updateDataMask } from 'src/dataMask/actions';
import { onSave } from './dashboardState';
import { getAffectedChartIdsFromCustomizations } from 'src/dashboard/util/getRelatedCharts';
import { hasValidColumn } from 'src/dashboard/components/nativeFilters/ChartCustomization/utils';

const createUpdateDashboardApi = (id: number) =>
makeApi<
Expand Down Expand Up @@ -100,7 +103,11 @@ export function saveChartCustomization(
dispatch: ThunkDispatch<RootState, null, AnyAction>,
getState: () => RootState,
) {
const { id, metadata, json_metadata } = getState().dashboardInfo;
const {
id,
metadata,
json_metadata: jsonMetadata,
} = getState().dashboardInfo;

const currentState = getState();
const currentChartCustomizationItems =
Expand Down Expand Up @@ -140,32 +147,51 @@ export function saveChartCustomization(
dispatch(removeDataMask(customizationFilterId));
});

simpleItems.forEach(item => {
const customizationFilterId = `chart_customization_${item.id}`;

if (item.customization?.column) {
const existingDataMask = getState().dataMask[customizationFilterId];

const existingFilterState = existingDataMask?.filterState;

dispatch(removeDataMask(customizationFilterId));

const dataMask = {
extraFormData: {},
filterState: {
value:
existingFilterState?.value ||
item.customization?.defaultDataMask?.filterState?.value ||
[],
},
ownState: {
column: item.customization.column,
},
};

dispatch(updateDataMask(customizationFilterId, dataMask));
chartCustomizationItems.forEach(newItem => {
const customizationFilterId = `chart_customization_${newItem.id}`;
const existingItem = existingItemsMap.get(newItem.id);

if (!existingItem) {
if (hasValidColumn(newItem.customization?.column)) {
const dataMask = {
extraFormData: {},
filterState: {
value:
newItem.customization?.defaultDataMask?.filterState?.value ||
[],
},
ownState: {
column: newItem.customization.column,
},
};
dispatch(updateDataMask(customizationFilterId, dataMask));
}
} else {
dispatch(removeDataMask(customizationFilterId));
const existingColumn = existingItem.customization?.column || null;
const newColumn = newItem.customization?.column || null;

if (!isEqual(existingColumn, newColumn)) {
if (hasValidColumn(newColumn)) {
const existingDataMask = getState().dataMask[customizationFilterId];
const existingFilterState = existingDataMask?.filterState;

const dataMask = {
extraFormData: {},
filterState: {
value:
existingFilterState?.value ||
newItem.customization?.defaultDataMask?.filterState?.value ||
[],
},
ownState: {
column: newColumn,
},
};
dispatch(updateDataMask(customizationFilterId, dataMask));
} else {
dispatch(removeDataMask(customizationFilterId));
}
}
}
});

Expand All @@ -174,9 +200,9 @@ export function saveChartCustomization(
try {
let parsedMetadata: any = {};
try {
parsedMetadata = json_metadata ? JSON.parse(json_metadata) : metadata;
parsedMetadata = jsonMetadata ? JSON.parse(jsonMetadata) : metadata;
} catch (e) {
console.error('Error parsing json_metadata:', e);
console.error('Error parsing jsonMetadata:', e);
parsedMetadata = metadata || {};
}

Expand Down Expand Up @@ -204,10 +230,31 @@ export function saveChartCustomization(
dispatch(onSave(lastModifiedTime));
}

const { dashboardState } = getState();
const chartIds = dashboardState.sliceIds || [];
if (chartIds.length > 0) {
chartIds.forEach(chartId => {
const sliceEntities = getState().sliceEntities || {};
const slices = sliceEntities.slices || {};

const changedItems: ChartCustomizationItem[] = [];

changedItems.push(...removedItems);

chartCustomizationItems.forEach(newItem => {
const existingItem = existingItemsMap.get(newItem.id);
if (existingItem && !newItem.removed) {
if (!isEqual(existingItem.customization, newItem.customization)) {
changedItems.push(existingItem);
}
}
});
Comment on lines +240 to +247
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing re-query for new customizations

In the loop over chartCustomizationItems, add an else if (!existingItem && hasValidColumn(newItem.customization?.column)) branch to push newItem so that newly added customizations trigger chart updates.

Code Review Run #9d6af9


Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them


const uniqueAffectedChartIds = new Set<number>();
const affectedCharts = getAffectedChartIdsFromCustomizations(
changedItems,
slices,
);
affectedCharts.forEach(chartId => uniqueAffectedChartIds.add(chartId));

if (uniqueAffectedChartIds.size > 0) {
Array.from(uniqueAffectedChartIds).forEach(chartId => {
dispatch(triggerQuery(true, chartId));
});
}
Expand Down
6 changes: 6 additions & 0 deletions superset-frontend/src/dashboard/components/Dashboard.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,12 @@ class Dashboard extends PureComponent {
);

[...allKeys].forEach(filterKey => {
// Skip chart customization filters - they are handled separately by saveChartCustomization
// which triggers queries only for affected charts
if (filterKey.startsWith('chart_customization_')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is pretty fragile. Any better check? Maybe centralizing it?

return;
}

if (
!currFilterKeys.includes(filterKey) &&
appliedFilterKeys.includes(filterKey)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -409,18 +409,35 @@ const ChartCustomizationForm: FC<Props> = ({
}
}, [form, item.id]);

const lastFetchedDatasetIdRef = useRef<number | null>(null);
const isFetchingRef = useRef<boolean>(false);

const fetchDatasetInfo = useCallback(async () => {
const formValues = getFormValues();
const dataset = formValues.dataset || customization.dataset;

if (!dataset) {
setMetrics([]);
lastFetchedDatasetIdRef.current = null;
return;
}

try {
const datasetId = getDatasetId(dataset);
if (datasetId === null) return;
if (datasetId === null) {
lastFetchedDatasetIdRef.current = null;
return;
}

if (
datasetId === lastFetchedDatasetIdRef.current ||
isFetchingRef.current
) {
return;
}

isFetchingRef.current = true;
lastFetchedDatasetIdRef.current = datasetId;

const cachedData = getCachedDataset(datasetId);
if (cachedData) {
Expand Down Expand Up @@ -465,6 +482,7 @@ const ChartCustomizationForm: FC<Props> = ({
} else {
setMetrics([]);
}
isFetchingRef.current = false;
return;
}

Expand Down Expand Up @@ -520,11 +538,16 @@ const ChartCustomizationForm: FC<Props> = ({
setMetrics([]);
}
}
isFetchingRef.current = false;
} catch (error) {
console.error('Error fetching dataset info:', error);
setMetrics([]);
isFetchingRef.current = false;
if (lastFetchedDatasetIdRef.current !== null) {
lastFetchedDatasetIdRef.current = null;
}
}
}, [form, item.id, customization.dataset, getDatasetId]);
}, [form, item.id, customization.dataset, getDatasetId, getFormValues]);

useEffect(() => {
const formValues = form.getFieldValue('filters')?.[item.id] || {};
Expand All @@ -533,11 +556,20 @@ const ChartCustomizationForm: FC<Props> = ({
if (dataset) {
const datasetId = getDatasetId(dataset);

if (datasetId !== null) {
if (datasetId !== null && datasetId !== lastFetchedDatasetIdRef.current) {
fetchDatasetInfo();
} else if (
datasetId === null &&
lastFetchedDatasetIdRef.current !== null
) {
lastFetchedDatasetIdRef.current = null;
setMetrics([]);
}
} else if (lastFetchedDatasetIdRef.current !== null) {
lastFetchedDatasetIdRef.current = null;
setMetrics([]);
}
}, [customization.dataset, fetchDatasetInfo, getDatasetId]);
}, [customization.dataset, fetchDatasetInfo, getDatasetId, form, item.id]);

const fetchDefaultValueData = useCallback(async () => {
const formValues = getFormValues();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import { FC, useCallback, useEffect, useMemo, useState } from 'react';
import { FC, useCallback, useEffect, useMemo, useState, useRef } from 'react';
import { t } from '@apache-superset/core';
import { DataMaskStateWithId, useTruncation } from '@superset-ui/core';
import { styled, css, useTheme } from '@apache-superset/core/ui';
Expand Down Expand Up @@ -416,9 +416,15 @@ const GroupByFilterCard: FC<GroupByFilterCardProps> = ({
setLoading(chartCustomizationLoading);
}, [chartCustomizationLoading]);

const lastFetchedDatasetIdRef = useRef<string | number | null>(null);
const isFetchingRef = useRef<boolean>(false);

useEffect(() => {
const fetchColumnOptions = async () => {
if (!dataset) return;
if (!dataset) {
lastFetchedDatasetIdRef.current = null;
return;
}

try {
const datasetId =
Expand All @@ -430,9 +436,20 @@ const GroupByFilterCard: FC<GroupByFilterCardProps> = ({
? (dataset as { value: string | number }).value
: null;

if (!datasetId) return;
if (!datasetId) {
lastFetchedDatasetIdRef.current = null;
return;
}

const response = await fetch(`/api/v1/dataset/${datasetId}`);
if (datasetId === lastFetchedDatasetIdRef.current) {
return;
}

isFetchingRef.current = true;
const currentDatasetId = datasetId;
lastFetchedDatasetIdRef.current = currentDatasetId;

const response = await fetch(`/api/v1/dataset/${currentDatasetId}`);
const data = await response.json();

if (data?.result?.columns) {
Expand All @@ -444,9 +461,15 @@ const GroupByFilterCard: FC<GroupByFilterCardProps> = ({
}));
setColumnOptions(options);
}
isFetchingRef.current = false;
} catch (error) {
console.warn('Failed to fetch column options:', error);
setColumnOptions([]);
isFetchingRef.current = false;
const currentDatasetId = lastFetchedDatasetIdRef.current;
if (currentDatasetId !== null) {
lastFetchedDatasetIdRef.current = null;
}
}
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,31 @@ export const ensureValidCustomization = (
column: customization.column || null,
...customization,
});

/**
* Checks if a column value is valid (non-null, non-empty).
* A column is considered valid if:
* - It's a non-empty string (after trimming)
* - It's a non-empty array
* - Other truthy values are considered valid
*
* @param column - The column value to check (string, string[], or null)
* @returns true if the column has a valid value, false otherwise
*/
export function hasValidColumn(
column: string | string[] | null | undefined,
): boolean {
if (column === null || column === undefined) {
return false;
}

if (typeof column === 'string') {
return column.trim() !== '';
}

if (Array.isArray(column)) {
return column.length > 0;
}

return true;
}
Loading
Loading