rusackas commented on code in PR #39459:
URL: https://github.com/apache/superset/pull/39459#discussion_r3263904667
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -186,252 +173,319 @@ const MessageSpan = styled.span`
color: ${({ theme }) => theme.colorText};
`;
-class Chart extends PureComponent<ChartProps, {}> {
- static defaultProps = defaultProps;
-
- renderStartTime: any;
-
- constructor(props: ChartProps) {
- super(props);
- this.handleRenderContainerFailure =
- this.handleRenderContainerFailure.bind(this);
- }
-
- componentDidMount() {
- if (this.props.triggerQuery) {
- this.runQuery();
- }
- }
-
- componentDidUpdate() {
- if (this.props.triggerQuery) {
- this.runQuery();
- }
- }
-
- shouldRenderChart() {
- return (
- this.props.isInView ||
+function Chart({
+ addFilter = () => BLANK,
+ onFilterMenuOpen = () => BLANK,
+ onFilterMenuClose = () => BLANK,
+ initialValues = BLANK,
+ setControlValue = () => BLANK,
+ triggerRender = false,
+ dashboardId,
+ chartStackTrace,
+ force = false,
+ isInView = true,
+ ...restProps
+}: ChartProps): JSX.Element {
+ const {
+ actions,
+ chartId,
+ datasource,
+ formData,
+ timeout,
+ ownState,
+ chartAlert,
+ chartStatus,
+ queriesResponse = [],
+ errorMessage,
+ chartIsStale,
+ width,
+ height,
+ datasetsStatus,
+ onQuery,
+ annotationData,
+ vizType,
+ latestQueryFormData,
+ triggerQuery,
+ postTransformProps,
+ emitCrossFilters,
+ onChartStateChange,
+ suppressLoadingSpinner,
+ filterState,
+ } = restProps;
+
+ const renderStartTimeRef = useRef<number>(Logger.getTimestamp());
+ // Update on each render to accurately track render duration
+ renderStartTimeRef.current = Logger.getTimestamp();
+
+ const shouldRenderChart = useCallback(
+ () =>
+ isInView ||
!isFeatureEnabled(FeatureFlag.DashboardVirtualization) ||
- isCurrentUserBot()
- );
- }
+ isCurrentUserBot(),
+ [isInView],
+ );
- runQuery() {
+ const runQuery = useCallback(() => {
if (
isFeatureEnabled(FeatureFlag.DashboardVirtualizationDeferData) &&
- !this.shouldRenderChart()
+ !shouldRenderChart()
) {
return;
}
// Create chart with POST request
- this.props.actions.postChartFormData(
- this.props.formData,
- Boolean(this.props.force || getUrlParam(URL_PARAMS.force)), // allow
override via url params force=true
- this.props.timeout,
- this.props.chartId,
- this.props.dashboardId,
- this.props.ownState,
- );
- }
-
- handleRenderContainerFailure(
- error: Error,
- info: { componentStack: string } | null,
- ) {
- const { actions, chartId } = this.props;
- logging.warn(error);
- actions.chartRenderingFailed(
- error.toString(),
+ actions.postChartFormData(
+ formData,
+ Boolean(force || getUrlParam(URL_PARAMS.force)), // allow override via
url params force=true
+ timeout,
chartId,
- info ? info.componentStack : null,
+ dashboardId,
+ ownState,
);
+ }, [
+ actions,
+ chartId,
+ dashboardId,
+ formData,
+ force,
+ ownState,
+ shouldRenderChart,
+ timeout,
+ ]);
+
+ const handleRenderContainerFailure = useCallback(
+ (error: Error, info: { componentStack: string } | null) => {
+ logging.warn(error);
+ actions.chartRenderingFailed(
+ error.toString(),
+ chartId,
+ info ? info.componentStack : null,
+ );
- actions.logEvent(LOG_ACTIONS_RENDER_CHART, {
- slice_id: chartId,
- has_err: true,
- error_details: error.toString(),
- start_offset: this.renderStartTime,
- ts: new Date().getTime(),
- duration: Logger.getTimestamp() - this.renderStartTime,
- });
- }
+ actions.logEvent(LOG_ACTIONS_RENDER_CHART, {
+ slice_id: chartId,
+ has_err: true,
+ error_details: error.toString(),
+ start_offset: renderStartTimeRef.current,
+ ts: new Date().getTime(),
+ duration: Logger.getTimestamp() - renderStartTimeRef.current,
+ });
+ },
+ [actions, chartId],
+ );
- renderErrorMessage(queryResponse: ChartErrorType) {
- const {
- chartId,
+ // componentDidMount and componentDidUpdate combined
+ useEffect(() => {
+ if (triggerQuery) {
+ runQuery();
+ }
+ }, [triggerQuery, runQuery]);
+
+ const renderErrorMessage = useCallback(
+ (queryResponse: ChartErrorType) => {
+ const error = queryResponse?.errors?.[0];
+ const message = chartAlert || queryResponse?.message;
+
+ // if datasource is still loading, don't render JS errors
+ // but always show backend API errors (which have an errors array)
+ // so users can see real issues like auth failures
+ if (
+ !error &&
+ chartAlert !== undefined &&
+ chartAlert !== NONEXISTENT_DATASET &&
+ datasource === PLACEHOLDER_DATASOURCE &&
+ datasetsStatus !== ResourceStatus.Error
+ ) {
+ return (
+ <Styles
+ key={chartId}
+ data-ui-anchor="chart"
+ className="chart-container"
+ data-test="chart-container"
+ height={height}
+ >
+ <Loading size={dashboardId ? 's' : 'm'} muted={!!dashboardId} />
+ </Styles>
+ );
+ }
+
+ return (
+ <ChartErrorMessage
+ key={chartId}
+ chartId={chartId}
+ error={error}
+ subtitle={message}
+ link={queryResponse ? queryResponse.link : undefined}
+ source={dashboardId ? ChartSource.Dashboard : ChartSource.Explore}
+ stackTrace={chartStackTrace}
+ />
+ );
+ },
+ [
chartAlert,
+ chartId,
chartStackTrace,
- datasource,
dashboardId,
- height,
datasetsStatus,
- } = this.props;
- const error = queryResponse?.errors?.[0];
- const message = chartAlert || queryResponse?.message;
+ datasource,
+ height,
+ ],
+ );
+
+ const renderSpinner = useCallback(
+ (databaseName: string | undefined) => {
+ const message = databaseName
+ ? t('Waiting on %s', databaseName)
+ : t('Waiting on database...');
- // if datasource is still loading, don't render JS errors
- // but always show backend API errors (which have an errors array)
- // so users can see real issues like auth failures
- if (
- !error &&
- chartAlert !== undefined &&
- chartAlert !== NONEXISTENT_DATASET &&
- datasource === PLACEHOLDER_DATASOURCE &&
- datasetsStatus !== ResourceStatus.Error
- ) {
return (
- <Styles
- key={chartId}
- data-ui-anchor="chart"
- className="chart-container"
- data-test="chart-container"
- height={height}
- >
+ <LoadingDiv>
<Loading
- size={this.props.dashboardId ? 's' : 'm'}
- muted={!!this.props.dashboardId}
+ position="inline-centered"
+ size={dashboardId ? 's' : 'm'}
+ muted={!!dashboardId}
/>
- </Styles>
+ <MessageSpan>{message}</MessageSpan>
+ </LoadingDiv>
);
- }
-
- return (
- <ChartErrorMessage
- key={chartId}
- chartId={chartId}
- error={error}
- subtitle={message}
- link={queryResponse ? queryResponse.link : undefined}
- source={dashboardId ? ChartSource.Dashboard : ChartSource.Explore}
- stackTrace={chartStackTrace}
- />
- );
- }
-
- renderSpinner(databaseName: string | undefined) {
- const message = databaseName
- ? t('Waiting on %s', databaseName)
- : t('Waiting on database...');
-
- return (
- <LoadingDiv>
- <Loading
- position="inline-centered"
- size={this.props.dashboardId ? 's' : 'm'}
- muted={!!this.props.dashboardId}
- />
- <MessageSpan>{message}</MessageSpan>
- </LoadingDiv>
- );
- }
+ },
+ [dashboardId],
+ );
- renderChartContainer() {
- return (
+ const renderChartContainer = useCallback(
+ () => (
<div className="slice_container" data-test="slice-container">
- {this.shouldRenderChart() ? (
+ {shouldRenderChart() ? (
<ChartRenderer
- {...this.props}
- source={
- this.props.dashboardId
- ? ChartSource.Dashboard
- : ChartSource.Explore
- }
- data-test={this.props.vizType}
+ annotationData={annotationData}
+ actions={actions}
+ chartId={chartId}
+ datasource={datasource}
+ initialValues={initialValues}
+ formData={formData}
+ height={height}
+ width={width}
+ setControlValue={setControlValue}
+ vizType={vizType}
+ triggerRender={triggerRender}
+ chartAlert={chartAlert}
+ chartStatus={chartStatus}
+ queriesResponse={queriesResponse}
+ triggerQuery={triggerQuery}
+ chartIsStale={chartIsStale}
+ addFilter={addFilter}
+ onFilterMenuOpen={onFilterMenuOpen}
+ onFilterMenuClose={onFilterMenuClose}
+ ownState={ownState}
+ postTransformProps={postTransformProps}
+ emitCrossFilters={emitCrossFilters}
+ onChartStateChange={onChartStateChange}
+ latestQueryFormData={latestQueryFormData}
+ filterState={filterState}
+ suppressLoadingSpinner={suppressLoadingSpinner}
+ source={dashboardId ? ChartSource.Dashboard : ChartSource.Explore}
/>
Review Comment:
Leaving the explicit prop list — `ChartRenderer` only consumes a known
subset of `ChartProps` (see `ChartRendererProps` interface at the top of the
file), so listing them is more correct than spreading. If there's a specific
prop you've identified as silently dropped (cache-buster / label-color etc.),
call it out and I'll add it.
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -175,402 +177,370 @@ const BIG_NO_RESULT_MIN_HEIGHT = 220;
const behaviors = [Behavior.InteractiveChart];
-const defaultProps: Partial<ChartRendererProps> = {
- addFilter: () => BLANK,
- onFilterMenuOpen: () => BLANK,
- onFilterMenuClose: () => BLANK,
- initialValues: BLANK,
- setControlValue: () => {},
- triggerRender: false,
-};
-
-class ChartRenderer extends Component<ChartRendererProps, ChartRendererState> {
- static defaultProps = defaultProps;
-
- private hasQueryResponseChange: boolean;
-
- private contextMenuRef: RefObject<ChartContextMenuRef>;
-
- private hooks: ChartHooks;
-
- private mutableQueriesResponse: QueryData[] | null | undefined;
-
- private renderStartTime: number;
-
- constructor(props: ChartRendererProps) {
- super(props);
- const suppressContextMenu = getChartMetadataRegistry().get(
- props.formData.viz_type ?? props.vizType,
- )?.suppressContextMenu;
- this.state = {
- showContextMenu:
- props.source === ChartSource.Dashboard &&
- !suppressContextMenu &&
- isFeatureEnabled(FeatureFlag.DrillToDetail),
- inContextMenu: false,
- legendState: undefined,
- legendIndex: 0,
- };
- this.hasQueryResponseChange = false;
- this.renderStartTime = 0;
-
- this.contextMenuRef = createRef<ChartContextMenuRef>();
-
- this.handleAddFilter = this.handleAddFilter.bind(this);
- this.handleRenderSuccess = this.handleRenderSuccess.bind(this);
- this.handleRenderFailure = this.handleRenderFailure.bind(this);
- this.handleSetControlValue = this.handleSetControlValue.bind(this);
- this.handleOnContextMenu = this.handleOnContextMenu.bind(this);
- this.handleContextMenuSelected = this.handleContextMenuSelected.bind(this);
- this.handleContextMenuClosed = this.handleContextMenuClosed.bind(this);
- this.handleLegendStateChanged = this.handleLegendStateChanged.bind(this);
- this.onContextMenuFallback = this.onContextMenuFallback.bind(this);
- this.handleLegendScroll = this.handleLegendScroll.bind(this);
-
- this.hooks = {
- onAddFilter: this.handleAddFilter,
- onContextMenu: this.state.showContextMenu
- ? this.handleOnContextMenu
- : undefined,
- onError: this.handleRenderFailure,
- setControlValue: this.handleSetControlValue,
- onFilterMenuOpen: this.props.onFilterMenuOpen,
- onFilterMenuClose: this.props.onFilterMenuClose,
- onLegendStateChanged: this.handleLegendStateChanged,
- setDataMask: (dataMask: DataMask) => {
- this.props.actions?.updateDataMask?.(this.props.chartId, dataMask);
- },
- onLegendScroll: this.handleLegendScroll,
- onChartStateChange: this.props.onChartStateChange,
- };
-
- // TODO: queriesResponse comes from Redux store but it's being edited by
- // the plugins, hence we need to clone it to avoid state mutation
- // until we change the reducers to use Redux Toolkit with Immer
- this.mutableQueriesResponse = cloneDeep(this.props.queriesResponse);
- }
-
- shouldComponentUpdate(
- nextProps: ChartRendererProps,
- nextState: ChartRendererState,
- ): boolean {
- const resultsReady =
- nextProps.queriesResponse &&
- ['success', 'rendered'].indexOf(nextProps.chartStatus as string) > -1 &&
- !nextProps.queriesResponse?.[0]?.error;
-
- if (resultsReady) {
- if (!isEqual(this.state, nextState)) {
- return true;
- }
- this.hasQueryResponseChange =
- nextProps.queriesResponse !== this.props.queriesResponse;
-
- if (this.hasQueryResponseChange) {
- this.mutableQueriesResponse = cloneDeep(nextProps.queriesResponse);
- }
-
- // Check if any matrixify-related properties have changed
- const hasMatrixifyChanges = (): boolean => {
- const nextFormData = nextProps.formData as JsonObject;
- const currentFormData = this.props.formData as JsonObject;
- const isMatrixifyEnabled =
- nextFormData.matrixify_enable === true &&
- ((nextFormData.matrixify_mode_rows !== undefined &&
- nextFormData.matrixify_mode_rows !== 'disabled') ||
- (nextFormData.matrixify_mode_columns !== undefined &&
- nextFormData.matrixify_mode_columns !== 'disabled'));
- if (!isMatrixifyEnabled) return false;
-
- // Check all matrixify-related properties
- const matrixifyKeys = Object.keys(nextFormData).filter(key =>
- key.startsWith('matrixify_'),
- );
-
- return matrixifyKeys.some(
- key => !isEqual(nextFormData[key], currentFormData[key]),
- );
- };
-
- const nextFormData = nextProps.formData as JsonObject;
- const currentFormData = this.props.formData as JsonObject;
-
- return (
- this.hasQueryResponseChange ||
- !isEqual(nextProps.datasource, this.props.datasource) ||
- nextProps.annotationData !== this.props.annotationData ||
- nextProps.ownState !== this.props.ownState ||
- nextProps.filterState !== this.props.filterState ||
- nextProps.height !== this.props.height ||
- nextProps.width !== this.props.width ||
- nextProps.triggerRender === true ||
- nextProps.labelsColor !== this.props.labelsColor ||
- nextProps.labelsColorMap !== this.props.labelsColorMap ||
- nextFormData.color_scheme !== currentFormData.color_scheme ||
- nextFormData.stack !== currentFormData.stack ||
- nextFormData.subcategories !== currentFormData.subcategories ||
- nextProps.cacheBusterProp !== this.props.cacheBusterProp ||
- nextProps.emitCrossFilters !== this.props.emitCrossFilters ||
- nextProps.postTransformProps !== this.props.postTransformProps ||
- hasMatrixifyChanges()
- );
- }
- return false;
- }
+interface ChartRendererState {
+ showContextMenu: boolean;
+ inContextMenu: boolean;
+ legendState: LegendState | undefined;
+ legendIndex: number;
+}
- handleAddFilter(
- col: string,
- vals: FilterValue[],
- merge = true,
- refresh = true,
- ): void {
- this.props.addFilter?.(col, vals, merge, refresh);
+function ChartRendererComponent({
+ addFilter = () => BLANK,
+ onFilterMenuOpen = () => BLANK,
+ onFilterMenuClose = () => BLANK,
+ initialValues = BLANK,
+ setControlValue = () => {},
+ triggerRender = false,
+ ...restProps
+}: ChartRendererProps): JSX.Element | null {
+ const {
+ annotationData,
+ actions,
+ chartId,
+ datasource,
+ formData,
+ latestQueryFormData,
+ height,
+ width,
+ vizType: propVizType,
+ chartAlert,
+ chartStatus,
+ queriesResponse,
+ chartIsStale,
+ ownState,
+ filterState,
+ postTransformProps,
+ source,
+ emitCrossFilters,
+ onChartStateChange,
+ } = restProps;
+
+ const theme = useTheme();
+
+ const suppressContextMenu = getChartMetadataRegistry().get(
+ formData.viz_type ?? propVizType,
+ )?.suppressContextMenu;
+
+ const [state, setState] = useState<ChartRendererState>({
+ showContextMenu:
+ source === ChartSource.Dashboard &&
+ !suppressContextMenu &&
+ isFeatureEnabled(FeatureFlag.DrillToDetail),
+ inContextMenu: false,
+ legendState: undefined,
+ legendIndex: 0,
+ });
+
+ const hasQueryResponseChangeRef = useRef(false);
+ const renderStartTimeRef = useRef(0);
+ const contextMenuRef = useRef<ChartContextMenuRef>(null);
+
+ // Results are "ready" when we have a non-error queriesResponse and the
+ // chartStatus reflects it. This mirrors the gating logic from the former
+ // shouldComponentUpdate implementation.
+ const resultsReady =
+ queriesResponse &&
+ ['success', 'rendered'].indexOf(chartStatus as string) > -1 &&
+ !queriesResponse?.[0]?.error;
+
+ // Track whether queriesResponse changed since the previous render so that
+ // handleRenderSuccess / handleRenderFailure know whether to log render time.
+ // Updating a ref during render is safe when the value doesn't affect the
+ // render output (here it's read asynchronously from SuperChart callbacks).
+ const prevQueriesResponseRef = useRef<QueryData[] | null | undefined>(
+ queriesResponse,
+ );
+ if (resultsReady) {
+ hasQueryResponseChangeRef.current =
+ queriesResponse !== prevQueriesResponseRef.current;
}
+ useEffect(() => {
+ prevQueriesResponseRef.current = queriesResponse;
+ }, [queriesResponse]);
+
+ // Clone queriesResponse to protect against plugin mutation of Redux state.
+ // TODO: remove once reducers use Redux Toolkit with Immer.
+ const mutableQueriesResponse = useMemo(
+ () => cloneDeep(queriesResponse),
+ [queriesResponse],
+ );
Review Comment:
Fixed in 664a891f. Gated the `cloneDeep` on `resultsReady` so it only runs
when results are about to render, not on every `queriesResponse` identity
change during loading/idle.
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -175,402 +177,370 @@ const BIG_NO_RESULT_MIN_HEIGHT = 220;
const behaviors = [Behavior.InteractiveChart];
-const defaultProps: Partial<ChartRendererProps> = {
- addFilter: () => BLANK,
- onFilterMenuOpen: () => BLANK,
- onFilterMenuClose: () => BLANK,
- initialValues: BLANK,
- setControlValue: () => {},
- triggerRender: false,
-};
-
-class ChartRenderer extends Component<ChartRendererProps, ChartRendererState> {
- static defaultProps = defaultProps;
-
- private hasQueryResponseChange: boolean;
-
- private contextMenuRef: RefObject<ChartContextMenuRef>;
-
- private hooks: ChartHooks;
-
- private mutableQueriesResponse: QueryData[] | null | undefined;
-
- private renderStartTime: number;
-
- constructor(props: ChartRendererProps) {
- super(props);
- const suppressContextMenu = getChartMetadataRegistry().get(
- props.formData.viz_type ?? props.vizType,
- )?.suppressContextMenu;
- this.state = {
- showContextMenu:
- props.source === ChartSource.Dashboard &&
- !suppressContextMenu &&
- isFeatureEnabled(FeatureFlag.DrillToDetail),
- inContextMenu: false,
- legendState: undefined,
- legendIndex: 0,
- };
- this.hasQueryResponseChange = false;
- this.renderStartTime = 0;
-
- this.contextMenuRef = createRef<ChartContextMenuRef>();
-
- this.handleAddFilter = this.handleAddFilter.bind(this);
- this.handleRenderSuccess = this.handleRenderSuccess.bind(this);
- this.handleRenderFailure = this.handleRenderFailure.bind(this);
- this.handleSetControlValue = this.handleSetControlValue.bind(this);
- this.handleOnContextMenu = this.handleOnContextMenu.bind(this);
- this.handleContextMenuSelected = this.handleContextMenuSelected.bind(this);
- this.handleContextMenuClosed = this.handleContextMenuClosed.bind(this);
- this.handleLegendStateChanged = this.handleLegendStateChanged.bind(this);
- this.onContextMenuFallback = this.onContextMenuFallback.bind(this);
- this.handleLegendScroll = this.handleLegendScroll.bind(this);
-
- this.hooks = {
- onAddFilter: this.handleAddFilter,
- onContextMenu: this.state.showContextMenu
- ? this.handleOnContextMenu
- : undefined,
- onError: this.handleRenderFailure,
- setControlValue: this.handleSetControlValue,
- onFilterMenuOpen: this.props.onFilterMenuOpen,
- onFilterMenuClose: this.props.onFilterMenuClose,
- onLegendStateChanged: this.handleLegendStateChanged,
- setDataMask: (dataMask: DataMask) => {
- this.props.actions?.updateDataMask?.(this.props.chartId, dataMask);
- },
- onLegendScroll: this.handleLegendScroll,
- onChartStateChange: this.props.onChartStateChange,
- };
-
- // TODO: queriesResponse comes from Redux store but it's being edited by
- // the plugins, hence we need to clone it to avoid state mutation
- // until we change the reducers to use Redux Toolkit with Immer
- this.mutableQueriesResponse = cloneDeep(this.props.queriesResponse);
- }
-
- shouldComponentUpdate(
- nextProps: ChartRendererProps,
- nextState: ChartRendererState,
- ): boolean {
- const resultsReady =
- nextProps.queriesResponse &&
- ['success', 'rendered'].indexOf(nextProps.chartStatus as string) > -1 &&
- !nextProps.queriesResponse?.[0]?.error;
-
- if (resultsReady) {
- if (!isEqual(this.state, nextState)) {
- return true;
- }
- this.hasQueryResponseChange =
- nextProps.queriesResponse !== this.props.queriesResponse;
-
- if (this.hasQueryResponseChange) {
- this.mutableQueriesResponse = cloneDeep(nextProps.queriesResponse);
- }
-
- // Check if any matrixify-related properties have changed
- const hasMatrixifyChanges = (): boolean => {
- const nextFormData = nextProps.formData as JsonObject;
- const currentFormData = this.props.formData as JsonObject;
- const isMatrixifyEnabled =
- nextFormData.matrixify_enable === true &&
- ((nextFormData.matrixify_mode_rows !== undefined &&
- nextFormData.matrixify_mode_rows !== 'disabled') ||
- (nextFormData.matrixify_mode_columns !== undefined &&
- nextFormData.matrixify_mode_columns !== 'disabled'));
- if (!isMatrixifyEnabled) return false;
-
- // Check all matrixify-related properties
- const matrixifyKeys = Object.keys(nextFormData).filter(key =>
- key.startsWith('matrixify_'),
- );
-
- return matrixifyKeys.some(
- key => !isEqual(nextFormData[key], currentFormData[key]),
- );
- };
-
- const nextFormData = nextProps.formData as JsonObject;
- const currentFormData = this.props.formData as JsonObject;
-
- return (
- this.hasQueryResponseChange ||
- !isEqual(nextProps.datasource, this.props.datasource) ||
- nextProps.annotationData !== this.props.annotationData ||
- nextProps.ownState !== this.props.ownState ||
- nextProps.filterState !== this.props.filterState ||
- nextProps.height !== this.props.height ||
- nextProps.width !== this.props.width ||
- nextProps.triggerRender === true ||
- nextProps.labelsColor !== this.props.labelsColor ||
- nextProps.labelsColorMap !== this.props.labelsColorMap ||
- nextFormData.color_scheme !== currentFormData.color_scheme ||
- nextFormData.stack !== currentFormData.stack ||
- nextFormData.subcategories !== currentFormData.subcategories ||
- nextProps.cacheBusterProp !== this.props.cacheBusterProp ||
- nextProps.emitCrossFilters !== this.props.emitCrossFilters ||
- nextProps.postTransformProps !== this.props.postTransformProps ||
- hasMatrixifyChanges()
- );
- }
- return false;
- }
+interface ChartRendererState {
+ showContextMenu: boolean;
+ inContextMenu: boolean;
+ legendState: LegendState | undefined;
+ legendIndex: number;
+}
- handleAddFilter(
- col: string,
- vals: FilterValue[],
- merge = true,
- refresh = true,
- ): void {
- this.props.addFilter?.(col, vals, merge, refresh);
+function ChartRendererComponent({
+ addFilter = () => BLANK,
+ onFilterMenuOpen = () => BLANK,
+ onFilterMenuClose = () => BLANK,
+ initialValues = BLANK,
+ setControlValue = () => {},
+ triggerRender = false,
+ ...restProps
+}: ChartRendererProps): JSX.Element | null {
+ const {
+ annotationData,
+ actions,
+ chartId,
+ datasource,
+ formData,
+ latestQueryFormData,
+ height,
+ width,
+ vizType: propVizType,
+ chartAlert,
+ chartStatus,
+ queriesResponse,
+ chartIsStale,
+ ownState,
+ filterState,
+ postTransformProps,
+ source,
+ emitCrossFilters,
+ onChartStateChange,
+ } = restProps;
+
+ const theme = useTheme();
+
+ const suppressContextMenu = getChartMetadataRegistry().get(
+ formData.viz_type ?? propVizType,
+ )?.suppressContextMenu;
+
+ const [state, setState] = useState<ChartRendererState>({
+ showContextMenu:
+ source === ChartSource.Dashboard &&
+ !suppressContextMenu &&
+ isFeatureEnabled(FeatureFlag.DrillToDetail),
+ inContextMenu: false,
+ legendState: undefined,
+ legendIndex: 0,
+ });
+
Review Comment:
Fixed in 664a891f. Moved `showContextMenu` from `useState` initializer to a
`useMemo` derived from `source` + `suppressContextMenu` + the `DrillToDetail`
feature flag, so it recomputes whenever its inputs change (matching the
pre-refactor semantic). Renamed `state.showContextMenu` → `showContextMenu` at
all four call sites.
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -175,402 +177,370 @@ const BIG_NO_RESULT_MIN_HEIGHT = 220;
const behaviors = [Behavior.InteractiveChart];
-const defaultProps: Partial<ChartRendererProps> = {
- addFilter: () => BLANK,
- onFilterMenuOpen: () => BLANK,
- onFilterMenuClose: () => BLANK,
- initialValues: BLANK,
- setControlValue: () => {},
- triggerRender: false,
-};
-
-class ChartRenderer extends Component<ChartRendererProps, ChartRendererState> {
- static defaultProps = defaultProps;
-
- private hasQueryResponseChange: boolean;
-
- private contextMenuRef: RefObject<ChartContextMenuRef>;
-
- private hooks: ChartHooks;
-
- private mutableQueriesResponse: QueryData[] | null | undefined;
-
- private renderStartTime: number;
-
- constructor(props: ChartRendererProps) {
- super(props);
- const suppressContextMenu = getChartMetadataRegistry().get(
- props.formData.viz_type ?? props.vizType,
- )?.suppressContextMenu;
- this.state = {
- showContextMenu:
- props.source === ChartSource.Dashboard &&
- !suppressContextMenu &&
- isFeatureEnabled(FeatureFlag.DrillToDetail),
- inContextMenu: false,
- legendState: undefined,
- legendIndex: 0,
- };
- this.hasQueryResponseChange = false;
- this.renderStartTime = 0;
-
- this.contextMenuRef = createRef<ChartContextMenuRef>();
-
- this.handleAddFilter = this.handleAddFilter.bind(this);
- this.handleRenderSuccess = this.handleRenderSuccess.bind(this);
- this.handleRenderFailure = this.handleRenderFailure.bind(this);
- this.handleSetControlValue = this.handleSetControlValue.bind(this);
- this.handleOnContextMenu = this.handleOnContextMenu.bind(this);
- this.handleContextMenuSelected = this.handleContextMenuSelected.bind(this);
- this.handleContextMenuClosed = this.handleContextMenuClosed.bind(this);
- this.handleLegendStateChanged = this.handleLegendStateChanged.bind(this);
- this.onContextMenuFallback = this.onContextMenuFallback.bind(this);
- this.handleLegendScroll = this.handleLegendScroll.bind(this);
-
- this.hooks = {
- onAddFilter: this.handleAddFilter,
- onContextMenu: this.state.showContextMenu
- ? this.handleOnContextMenu
- : undefined,
- onError: this.handleRenderFailure,
- setControlValue: this.handleSetControlValue,
- onFilterMenuOpen: this.props.onFilterMenuOpen,
- onFilterMenuClose: this.props.onFilterMenuClose,
- onLegendStateChanged: this.handleLegendStateChanged,
- setDataMask: (dataMask: DataMask) => {
- this.props.actions?.updateDataMask?.(this.props.chartId, dataMask);
- },
- onLegendScroll: this.handleLegendScroll,
- onChartStateChange: this.props.onChartStateChange,
- };
-
- // TODO: queriesResponse comes from Redux store but it's being edited by
- // the plugins, hence we need to clone it to avoid state mutation
- // until we change the reducers to use Redux Toolkit with Immer
- this.mutableQueriesResponse = cloneDeep(this.props.queriesResponse);
- }
-
- shouldComponentUpdate(
- nextProps: ChartRendererProps,
- nextState: ChartRendererState,
- ): boolean {
- const resultsReady =
- nextProps.queriesResponse &&
- ['success', 'rendered'].indexOf(nextProps.chartStatus as string) > -1 &&
- !nextProps.queriesResponse?.[0]?.error;
-
- if (resultsReady) {
- if (!isEqual(this.state, nextState)) {
- return true;
- }
- this.hasQueryResponseChange =
- nextProps.queriesResponse !== this.props.queriesResponse;
-
- if (this.hasQueryResponseChange) {
- this.mutableQueriesResponse = cloneDeep(nextProps.queriesResponse);
- }
-
- // Check if any matrixify-related properties have changed
- const hasMatrixifyChanges = (): boolean => {
- const nextFormData = nextProps.formData as JsonObject;
- const currentFormData = this.props.formData as JsonObject;
- const isMatrixifyEnabled =
- nextFormData.matrixify_enable === true &&
- ((nextFormData.matrixify_mode_rows !== undefined &&
- nextFormData.matrixify_mode_rows !== 'disabled') ||
- (nextFormData.matrixify_mode_columns !== undefined &&
- nextFormData.matrixify_mode_columns !== 'disabled'));
- if (!isMatrixifyEnabled) return false;
-
- // Check all matrixify-related properties
- const matrixifyKeys = Object.keys(nextFormData).filter(key =>
- key.startsWith('matrixify_'),
- );
-
- return matrixifyKeys.some(
- key => !isEqual(nextFormData[key], currentFormData[key]),
- );
- };
-
- const nextFormData = nextProps.formData as JsonObject;
- const currentFormData = this.props.formData as JsonObject;
-
- return (
- this.hasQueryResponseChange ||
- !isEqual(nextProps.datasource, this.props.datasource) ||
- nextProps.annotationData !== this.props.annotationData ||
- nextProps.ownState !== this.props.ownState ||
- nextProps.filterState !== this.props.filterState ||
- nextProps.height !== this.props.height ||
- nextProps.width !== this.props.width ||
- nextProps.triggerRender === true ||
- nextProps.labelsColor !== this.props.labelsColor ||
- nextProps.labelsColorMap !== this.props.labelsColorMap ||
- nextFormData.color_scheme !== currentFormData.color_scheme ||
- nextFormData.stack !== currentFormData.stack ||
- nextFormData.subcategories !== currentFormData.subcategories ||
- nextProps.cacheBusterProp !== this.props.cacheBusterProp ||
- nextProps.emitCrossFilters !== this.props.emitCrossFilters ||
- nextProps.postTransformProps !== this.props.postTransformProps ||
- hasMatrixifyChanges()
- );
- }
- return false;
- }
+interface ChartRendererState {
+ showContextMenu: boolean;
+ inContextMenu: boolean;
+ legendState: LegendState | undefined;
+ legendIndex: number;
+}
- handleAddFilter(
- col: string,
- vals: FilterValue[],
- merge = true,
- refresh = true,
- ): void {
- this.props.addFilter?.(col, vals, merge, refresh);
+function ChartRendererComponent({
+ addFilter = () => BLANK,
+ onFilterMenuOpen = () => BLANK,
+ onFilterMenuClose = () => BLANK,
+ initialValues = BLANK,
+ setControlValue = () => {},
+ triggerRender = false,
+ ...restProps
+}: ChartRendererProps): JSX.Element | null {
+ const {
+ annotationData,
+ actions,
+ chartId,
+ datasource,
+ formData,
+ latestQueryFormData,
+ height,
+ width,
+ vizType: propVizType,
+ chartAlert,
+ chartStatus,
+ queriesResponse,
+ chartIsStale,
+ ownState,
+ filterState,
+ postTransformProps,
+ source,
+ emitCrossFilters,
+ onChartStateChange,
+ } = restProps;
+
+ const theme = useTheme();
+
+ const suppressContextMenu = getChartMetadataRegistry().get(
+ formData.viz_type ?? propVizType,
+ )?.suppressContextMenu;
+
+ const [state, setState] = useState<ChartRendererState>({
+ showContextMenu:
+ source === ChartSource.Dashboard &&
+ !suppressContextMenu &&
+ isFeatureEnabled(FeatureFlag.DrillToDetail),
+ inContextMenu: false,
+ legendState: undefined,
+ legendIndex: 0,
+ });
+
+ const hasQueryResponseChangeRef = useRef(false);
+ const renderStartTimeRef = useRef(0);
+ const contextMenuRef = useRef<ChartContextMenuRef>(null);
+
+ // Results are "ready" when we have a non-error queriesResponse and the
+ // chartStatus reflects it. This mirrors the gating logic from the former
+ // shouldComponentUpdate implementation.
+ const resultsReady =
+ queriesResponse &&
+ ['success', 'rendered'].indexOf(chartStatus as string) > -1 &&
+ !queriesResponse?.[0]?.error;
+
+ // Track whether queriesResponse changed since the previous render so that
+ // handleRenderSuccess / handleRenderFailure know whether to log render time.
+ // Updating a ref during render is safe when the value doesn't affect the
+ // render output (here it's read asynchronously from SuperChart callbacks).
+ const prevQueriesResponseRef = useRef<QueryData[] | null | undefined>(
+ queriesResponse,
+ );
+ if (resultsReady) {
+ hasQueryResponseChangeRef.current =
+ queriesResponse !== prevQueriesResponseRef.current;
}
+ useEffect(() => {
+ prevQueriesResponseRef.current = queriesResponse;
+ }, [queriesResponse]);
+
+ // Clone queriesResponse to protect against plugin mutation of Redux state.
+ // TODO: remove once reducers use Redux Toolkit with Immer.
+ const mutableQueriesResponse = useMemo(
+ () => cloneDeep(queriesResponse),
+ [queriesResponse],
+ );
+
+ // Handler functions
+ const handleAddFilter = useCallback(
+ (col: string, vals: FilterValue[], merge = true, refresh = true): void => {
+ addFilter?.(col, vals, merge, refresh);
+ },
+ [addFilter],
+ );
- handleRenderSuccess(): void {
- const { actions, chartStatus, chartId, vizType } = this.props;
+ const handleRenderSuccess = useCallback((): void => {
if (['loading', 'rendered'].indexOf(chartStatus as string) < 0) {
actions.chartRenderingSucceeded(chartId);
}
// only log chart render time which is triggered by query results change
- // currently we don't log chart re-render time, like window resize etc
- if (this.hasQueryResponseChange) {
+ if (hasQueryResponseChangeRef.current) {
actions.logEvent(LOG_ACTIONS_RENDER_CHART, {
slice_id: chartId,
- viz_type: vizType,
- start_offset: this.renderStartTime,
+ viz_type: propVizType,
+ start_offset: renderStartTimeRef.current,
ts: new Date().getTime(),
- duration: Logger.getTimestamp() - this.renderStartTime,
+ duration: Logger.getTimestamp() - renderStartTimeRef.current,
});
}
- }
+ }, [actions, chartId, chartStatus, propVizType]);
+
+ const handleRenderFailure = useCallback(
+ (error: Error, info: { componentStack: string } | null): void => {
+ logging.warn(error);
+ actions.chartRenderingFailed(
+ error.toString(),
+ chartId,
+ info ? info.componentStack : null,
+ );
- handleRenderFailure(
- error: Error,
- info: { componentStack: string } | null,
- ): void {
- const { actions, chartId } = this.props;
- logging.warn(error);
- actions.chartRenderingFailed(
- error.toString(),
- chartId,
- info ? info.componentStack : null,
- );
+ // only trigger render log when query is changed
+ if (hasQueryResponseChangeRef.current) {
+ actions.logEvent(LOG_ACTIONS_RENDER_CHART, {
+ slice_id: chartId,
+ has_err: true,
+ error_details: error.toString(),
+ start_offset: renderStartTimeRef.current,
+ ts: new Date().getTime(),
+ duration: Logger.getTimestamp() - renderStartTimeRef.current,
+ });
+ }
+ },
+ [actions, chartId],
+ );
- // only trigger render log when query is changed
- if (this.hasQueryResponseChange) {
- actions.logEvent(LOG_ACTIONS_RENDER_CHART, {
- slice_id: chartId,
- has_err: true,
- error_details: error.toString(),
- start_offset: this.renderStartTime,
- ts: new Date().getTime(),
- duration: Logger.getTimestamp() - this.renderStartTime,
- });
- }
- }
+ const handleSetControlValue = useCallback(
+ (name: string, value: unknown): void => {
+ if (setControlValue) {
+ setControlValue(name, value);
+ }
+ },
+ [setControlValue],
+ );
- handleSetControlValue(name: string, value: unknown): void {
- const { setControlValue } = this.props;
- if (setControlValue) {
- setControlValue(name, value);
- }
- }
+ const handleOnContextMenu = useCallback(
+ (offsetX: number, offsetY: number, filters?: ContextMenuFilters): void => {
+ contextMenuRef.current?.open(offsetX, offsetY, filters);
+ setState(prev => ({ ...prev, inContextMenu: true }));
+ },
+ [contextMenuRef],
+ );
- handleOnContextMenu(
- offsetX: number,
- offsetY: number,
- filters?: ContextMenuFilters,
- ): void {
- this.contextMenuRef.current?.open(offsetX, offsetY, filters);
- this.setState({ inContextMenu: true });
- }
+ const handleContextMenuSelected = useCallback((): void => {
+ setState(prev => ({ ...prev, inContextMenu: false }));
+ }, []);
- handleContextMenuSelected(): void {
- this.setState({ inContextMenu: false });
- }
+ const handleContextMenuClosed = useCallback((): void => {
+ setState(prev => ({ ...prev, inContextMenu: false }));
+ }, []);
- handleContextMenuClosed(): void {
- this.setState({ inContextMenu: false });
- }
+ const handleLegendStateChanged = useCallback(
+ (legendState: LegendState): void => {
+ setState(prev => ({ ...prev, legendState }));
+ },
+ [],
+ );
- handleLegendStateChanged(legendState: LegendState): void {
- this.setState({ legendState });
- }
+ const handleLegendScroll = useCallback((legendIndex: number): void => {
+ setState(prev => ({ ...prev, legendIndex }));
+ }, []);
// When viz plugins don't handle `contextmenu` event, fallback handler
// calls `handleOnContextMenu` with no `filters` param.
- onContextMenuFallback(event: MouseEvent<HTMLDivElement>): void {
- if (!this.state.inContextMenu) {
- event.preventDefault();
- this.handleOnContextMenu(event.clientX, event.clientY);
- }
- }
+ const onContextMenuFallback = useCallback(
+ (event: MouseEvent<HTMLDivElement>): void => {
+ if (!state.inContextMenu) {
+ event.preventDefault();
+ handleOnContextMenu(event.clientX, event.clientY);
+ }
+ },
+ [handleOnContextMenu, state.inContextMenu],
+ );
- handleLegendScroll(legendIndex: number): void {
- this.setState({ legendIndex });
+ const setDataMaskCallback = useCallback(
+ (dataMask: DataMask) => {
+ actions?.updateDataMask?.(chartId, dataMask);
+ },
+ [actions, chartId],
+ );
+
+ // Hooks object - memoized
+ const hooks = useMemo<ChartHooks>(
+ () => ({
+ onAddFilter: handleAddFilter,
+ onContextMenu: state.showContextMenu ? handleOnContextMenu : undefined,
+ onError: handleRenderFailure,
+ setControlValue: handleSetControlValue,
+ onFilterMenuOpen,
+ onFilterMenuClose,
+ onLegendStateChanged: handleLegendStateChanged,
+ setDataMask: setDataMaskCallback,
+ onLegendScroll: handleLegendScroll,
+ onChartStateChange,
+ }),
+ [
+ handleAddFilter,
+ handleLegendScroll,
+ handleLegendStateChanged,
+ handleOnContextMenu,
+ handleRenderFailure,
+ handleSetControlValue,
+ onChartStateChange,
+ onFilterMenuClose,
+ onFilterMenuOpen,
+ setDataMaskCallback,
+ state.showContextMenu,
+ ],
+ );
+
+ const hasAnyErrors = queriesResponse?.some(item => item?.error);
+ const hasValidPreviousData =
+ (queriesResponse?.length ?? 0) > 0 && !hasAnyErrors;
+
+ if (!!chartAlert || chartStatus === null) {
+ return null;
}
- render(): ReactNode {
- const { chartAlert, chartStatus, chartId, emitCrossFilters } = this.props;
-
- const hasAnyErrors = this.props.queriesResponse?.some(item => item?.error);
- const hasValidPreviousData =
- (this.props.queriesResponse?.length ?? 0) > 0 && !hasAnyErrors;
-
- if (!!chartAlert || chartStatus === null) {
+ if (chartStatus === 'loading') {
+ if (!restProps.suppressLoadingSpinner || !hasValidPreviousData) {
return null;
}
+ }
- if (chartStatus === 'loading') {
- if (!this.props.suppressLoadingSpinner || !hasValidPreviousData) {
- return null;
- }
- }
-
- this.renderStartTime = Logger.getTimestamp();
-
- const {
- width,
- height,
- datasource,
- annotationData,
- initialValues,
- ownState,
- filterState,
- chartIsStale,
- formData,
- latestQueryFormData,
- postTransformProps,
- } = this.props;
-
- const currentFormData =
- chartIsStale && latestQueryFormData ? latestQueryFormData : formData;
- const vizType = currentFormData.viz_type || this.props.vizType;
-
- // It's bad practice to use unprefixed `vizType` as classnames for chart
- // container. It may cause css conflicts as in the case of legacy table
chart.
- // When migrating charts, we should gradually add a `superset-chart-`
prefix
- // to each one of them.
- const snakeCaseVizType = snakeCase(vizType);
- const chartClassName =
- vizType === VizType.Table
- ? `superset-chart-${snakeCaseVizType}`
- : snakeCaseVizType;
-
- const webpackHash =
- process.env.WEBPACK_MODE === 'development'
- ? `-${
- // eslint-disable-next-line camelcase
- typeof __webpack_require__ !== 'undefined' &&
- // eslint-disable-next-line camelcase, no-undef
- typeof __webpack_require__.h === 'function' &&
- // eslint-disable-next-line no-undef, camelcase
- __webpack_require__.h()
- }`
- : '';
-
- let noResultsComponent: ReactNode;
- const noResultTitle = t('No results were returned for this query');
- const noResultDescription =
- this.props.source === ChartSource.Explore
- ? t(
- 'Make sure that the controls are configured properly and the
datasource contains data for the selected time range',
- )
- : undefined;
- const noResultImage = 'chart.svg';
- if (
- (width ?? 0) > BIG_NO_RESULT_MIN_WIDTH &&
- (height ?? 0) > BIG_NO_RESULT_MIN_HEIGHT
- ) {
- noResultsComponent = (
- <EmptyState
- size="large"
- title={noResultTitle}
- description={noResultDescription}
- image={noResultImage}
- />
- );
- } else {
- noResultsComponent = (
- <EmptyState title={noResultTitle} image={noResultImage} size="small" />
- );
- }
-
- // Check for Behavior.DRILL_TO_DETAIL to tell if chart can receive Drill to
- // Detail props or if it'll cause side-effects (e.g. excessive re-renders).
- const drillToDetailProps = getChartMetadataRegistry()
- .get(vizType)
- ?.behaviors.find(behavior => behavior === Behavior.DrillToDetail)
- ? { inContextMenu: this.state.inContextMenu }
- : {};
- // By pass no result component when server pagination is enabled & the
table has:
- // - a backend search query, OR
- // - non-empty AG Grid filter model
- const hasSearchText = (ownState?.searchText?.length || 0) > 0;
- const hasAgGridFilters =
- ownState?.agGridFilterModel &&
- Object.keys(ownState.agGridFilterModel).length > 0;
-
- const currentFormDataExtended = currentFormData as JsonObject;
- const bypassNoResult = !(
- currentFormDataExtended?.server_pagination &&
- (hasSearchText || hasAgGridFilters)
+ renderStartTimeRef.current = Logger.getTimestamp();
+
+ const currentFormData =
+ chartIsStale && latestQueryFormData ? latestQueryFormData : formData;
+ const vizType = currentFormData.viz_type || propVizType;
+
+ // It's bad practice to use unprefixed `vizType` as classnames for chart
+ // container. It may cause css conflicts as in the case of legacy table
chart.
+ // When migrating charts, we should gradually add a `superset-chart-` prefix
+ // to each one of them.
+ const snakeCaseVizType = snakeCase(vizType);
+ const chartClassName =
+ vizType === VizType.Table
+ ? `superset-chart-${snakeCaseVizType}`
+ : snakeCaseVizType;
+
+ const webpackHash =
+ process.env.WEBPACK_MODE === 'development'
+ ? `-${
+ // eslint-disable-next-line camelcase
+ typeof __webpack_require__ !== 'undefined' &&
+ // eslint-disable-next-line camelcase, no-undef
+ typeof __webpack_require__.h === 'function' &&
+ // eslint-disable-next-line no-undef, camelcase
+ __webpack_require__.h()
+ }`
+ : '';
+
+ let noResultsComponent: ReactNode;
+ const noResultTitle = t('No results were returned for this query');
+ const noResultDescription =
+ source === ChartSource.Explore
+ ? t(
+ 'Make sure that the controls are configured properly and the
datasource contains data for the selected time range',
+ )
+ : undefined;
+ const noResultImage = 'chart.svg';
+ if (
+ (width ?? 0) > BIG_NO_RESULT_MIN_WIDTH &&
+ (height ?? 0) > BIG_NO_RESULT_MIN_HEIGHT
+ ) {
+ noResultsComponent = (
+ <EmptyState
+ size="large"
+ title={noResultTitle}
+ description={noResultDescription}
+ image={noResultImage}
+ />
);
-
- return (
- <>
- {this.state.showContextMenu && (
- <ChartContextMenu
- ref={this.contextMenuRef}
- id={chartId}
- formData={currentFormData as QueryFormData}
- onSelection={this.handleContextMenuSelected}
- onClose={this.handleContextMenuClosed}
- />
- )}
- <div
- onContextMenu={
- this.state.showContextMenu ? this.onContextMenuFallback : undefined
- }
- >
- <SuperChart
- disableErrorBoundary
- key={`${chartId}${webpackHash}`}
- id={`chart-id-${chartId}`}
- className={chartClassName}
- chartType={vizType}
- width={width}
- height={height}
- annotationData={annotationData}
- datasource={datasource}
- initialValues={initialValues}
- formData={currentFormData}
- ownState={ownState}
- filterState={filterState}
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- hooks={this.hooks as any}
- behaviors={behaviors}
- queriesData={this.mutableQueriesResponse ?? undefined}
- onRenderSuccess={this.handleRenderSuccess}
- onRenderFailure={this.handleRenderFailure}
- noResults={noResultsComponent}
- postTransformProps={postTransformProps}
- emitCrossFilters={emitCrossFilters}
- legendState={this.state.legendState}
- enableNoResults={bypassNoResult}
- legendIndex={this.state.legendIndex}
- isRefreshing={
- Boolean(this.props.suppressLoadingSpinner) &&
- chartStatus === 'loading'
- }
- {...drillToDetailProps}
- />
- </div>
- </>
+ } else {
+ noResultsComponent = (
+ <EmptyState title={noResultTitle} image={noResultImage} size="small" />
);
}
+
+ // Check for Behavior.DRILL_TO_DETAIL to tell if chart can receive Drill to
+ // Detail props or if it'll cause side-effects (e.g. excessive re-renders).
+ const drillToDetailProps = getChartMetadataRegistry()
+ .get(vizType)
+ ?.behaviors.find(behavior => behavior === Behavior.DrillToDetail)
+ ? { inContextMenu: state.inContextMenu }
+ : {};
+ // By pass no result component when server pagination is enabled & the table
has:
+ // - a backend search query, OR
+ // - non-empty AG Grid filter model
+ const hasSearchText = (ownState?.searchText?.length || 0) > 0;
+ const hasAgGridFilters =
+ ownState?.agGridFilterModel &&
+ Object.keys(ownState.agGridFilterModel).length > 0;
+
+ const currentFormDataExtended = currentFormData as JsonObject;
+ const bypassNoResult = !(
+ currentFormDataExtended?.server_pagination &&
+ (hasSearchText || hasAgGridFilters)
+ );
+
+ return (
+ <>
+ {state.showContextMenu && (
+ <ChartContextMenu
+ ref={contextMenuRef}
+ id={chartId}
+ formData={currentFormData as QueryFormData}
+ onSelection={handleContextMenuSelected}
+ onClose={handleContextMenuClosed}
+ />
+ )}
+ <div
+ onContextMenu={
+ state.showContextMenu ? onContextMenuFallback : undefined
+ }
+ >
+ <SuperChart
+ disableErrorBoundary
+ key={`${chartId}${webpackHash}`}
+ id={`chart-id-${chartId}`}
+ className={chartClassName}
+ chartType={vizType}
+ width={width}
+ height={height}
+ theme={theme}
+ annotationData={annotationData}
+ datasource={datasource}
+ initialValues={initialValues}
+ formData={currentFormData}
+ ownState={ownState}
+ filterState={filterState}
+ hooks={hooks as unknown as Parameters<typeof SuperChart>[0]['hooks']}
+ behaviors={behaviors}
+ queriesData={mutableQueriesResponse ?? undefined}
+ onRenderSuccess={handleRenderSuccess}
+ onRenderFailure={handleRenderFailure}
+ noResults={noResultsComponent}
+ postTransformProps={postTransformProps}
+ emitCrossFilters={emitCrossFilters}
+ legendState={state.legendState}
+ enableNoResults={bypassNoResult}
+ legendIndex={state.legendIndex}
+ isRefreshing={
+ Boolean(restProps.suppressLoadingSpinner) &&
+ chartStatus === 'loading'
+ }
+ {...drillToDetailProps}
+ />
+ </div>
+ </>
+ );
}
+const ChartRenderer = memo(ChartRendererComponent);
+
Review Comment:
Acknowledged. The class's `shouldComponentUpdate` had viz-specific bail
conditions that are non-trivial to port faithfully into an `areEqual` for
`memo`. Default-memo + memoized hook outputs is the more idiomatic React-18
shape, and the per-render `renderStartTimeRef` reset behaves correctly because
the success/failure callbacks read it asynchronously. Leaving as-is unless we
see a perf regression in practice.
##########
superset-frontend/src/components/Chart/ChartRenderer.test.tsx:
##########
@@ -394,7 +394,9 @@ test('renders chart during loading when
suppressLoadingSpinner has valid data',
queriesResponse: [{ data: [{ value: 1 }] }],
};
- const { getByTestId } = render(<ChartRenderer {...props} />);
+ const { getByTestId } = render(
+ <ChartRenderer {...(props as ChartRendererProps)} />,
+ );
Review Comment:
Fair point — the cast does mask type drift. Holding it as a test-cleanup
follow-up since re-typing the fixture means updating the partial-props shape
across the file. Will track separately.
--
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]
