Copilot commented on code in PR #39459:
URL: https://github.com/apache/superset/pull/39459#discussion_r3190578822
##########
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:
`cloneDeep(queriesResponse)` will run on every `queriesResponse` identity
change even when the component returns `null` (e.g. `chartStatus === 'loading'`
without spinner suppression) because hooks run unconditionally. Previously the
clone only occurred when results were ready and the response actually changed.
Consider gating the deep clone on `resultsReady` (e.g., memoize `undefined`
when not ready, or keep a ref to the last cloned value and only update it under
the same conditions as before) to avoid unnecessary deep cloning during
loading/idle transitions.
##########
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:
`showContextMenu` is derived from props (`source`, viz type metadata, and
feature flags) but is only computed once in `useState` initializers. If
`source` or the chart’s viz type changes for the same mounted instance,
`state.showContextMenu` can become stale and the context menu enablement will
be incorrect. Prefer deriving `showContextMenu` from current props (via
`useMemo`) and keeping only truly user-interaction state in React state (e.g.
`inContextMenu`, legend state), or sync `showContextMenu` with a `useEffect`
that updates it when its inputs change.
##########
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:
The previous implementation forwarded all props to `ChartRenderer` via
`{...this.props}`, but the new function component forwards an explicit subset.
This is a concrete behavior change because any `ChartProps` fields not
explicitly passed here will no longer reach `ChartRenderer`/`SuperChart` (e.g.,
additional rendering/cache-buster/label-color props that may exist on
`ChartProps`). To avoid accidental regressions, pass through the remaining
props (e.g. spread the rest object) and only override the few values that need
to change (like `source` / `data-test`), or ensure the full
`ChartRendererProps` surface is explicitly forwarded.
##########
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:
Casting `props as ChartRendererProps` in tests can mask real type
regressions introduced by the refactor. Prefer updating the test `props`
fixture to be typed as `ChartRendererProps` (or constructing it with a helper
that returns `ChartRendererProps`) so TypeScript continues to validate the test
setup rather than bypassing 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],
+ );
+
+ // 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:
The old class implementation used a custom `shouldComponentUpdate` with
fairly specific conditions (including readiness gating and selected prop
comparisons). Replacing it with `memo`’s default shallow compare can
significantly change re-render behavior (often more renders), which also risks
making render-time logging less meaningful because `renderStartTimeRef.current`
is reset on each render. Consider providing `memo(ChartRendererComponent,
areEqual)` that mirrors the prior SCU intent (or otherwise reintroduce
equivalent gating) so render frequency and logged durations remain consistent
with the previous behavior.
--
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]
