You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/04/17 17:35:49 UTC
[PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
rusackas opened a new pull request, #39459:
URL: https://github.com/apache/superset/pull/39459
## Summary
- Converts `ChartRenderer`, `Chart`, and `DrillByChart` in `src/components/Chart/` from class components to function components
- Includes updated `ChartRenderer.test.tsx`
Part of the broader class→function component lint cleanup tracked in #37902.
## Test plan
- [ ] CI passes on the target feature branch
- [ ] Charts render correctly in explore view and on dashboards
- [ ] Drill-by feature works correctly
## Additional information
This is sub-PR 8 of ~10, each targeting `chore/lint-cleanup-function-components`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39459:
URL: https://github.com/apache/superset/pull/39459#discussion_r3102176704
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -186,252 +170,318 @@ 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,
+ labelColors: _labelColors,
+ sharedLabelColors: _sharedLabelColors,
+ vizType,
+ isFiltersInitialized: _isFiltersInitialized,
+ latestQueryFormData,
+ triggerQuery,
+ postTransformProps,
+ emitCrossFilters,
+ onChartStateChange,
+ suppressLoadingSpinner,
+ } = 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}
+ source={dashboardId ? ChartSource.Dashboard : ChartSource.Explore}
+ data-test={vizType}
Review Comment:
**🟠 Architect Review — HIGH**
Chart no longer forwards the filterState prop to ChartRenderer, so dashboard dataMask[chartId].filterState is dropped and chart customization plugins (such as DeckglLayerVisibility) no longer receive or hydrate their filter state.
**Suggestion:** Extend ChartProps to include filterState and explicitly pass filterState={filterState} to ChartRenderer in renderChartContainer (restoring the previous Chart→ChartRenderer passthrough), then validate with a dashboard where dataMask[chartId].filterState is set (e.g., using DeckglLayerVisibility) to confirm state is preserved.
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fcomponents%2FChart%2FChart.tsx%0A%2A%2ALine%3A%2A%2A%20353%3A383%0A%2A%2AComment%3A%2A%2A%0A%09%2AHIGH%3A%20Chart%20no%20longer%20forwards%20the%20filterState%20prop%20to%20ChartRenderer%2C%20so%20dashboard%20dataMask%5BchartId%5D.filterState%20is%20dropped%20and%20chart%20customization%20plugins%20%28such%20as%20DeckglLayerVisibility%29%20no%20longer%20receive%20or%20hydrate%20their%20filter%20state.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc
%2Fcomponents%2FChart%2FChart.tsx%0A%2A%2ALine%3A%2A%2A%20353%3A383%0A%2A%2AComment%3A%2A%2A%0A%09%2AHIGH%3A%20Chart%20no%20longer%20forwards%20the%20filterState%20prop%20to%20ChartRenderer%2C%20so%20dashboard%20dataMask%5BchartId%5D.filterState%20is%20dropped%20and%20chart%20customization%20plugins%20%28such%20as%20DeckglLayerVisibility%29%20no%20longer%20receive%20or%20hydrate%20their%20filter%20state.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Chart/Chart.tsx
**Line:** 353:383
**Comment:**
*HIGH: Chart no longer forwards the filterState prop to ChartRenderer, so dashboard dataMask[chartId].filterState is dropped and chart customization plugins (such as DeckglLayerVisibility) no longer receive or hydrate their filter state.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39459:
URL: https://github.com/apache/superset/pull/39459#issuecomment-4627603700
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #39459:
URL: https://github.com/apache/superset/pull/39459#discussion_r3126947250
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -37,6 +46,8 @@ import {
} from '@superset-ui/core';
import { logging } from '@apache-superset/core/utils';
import { t } from '@apache-superset/core/translation';
+import { SupersetTheme } from '@apache-superset/core/theme';
+import { useTheme } from '@emotion/react';
Review Comment:
Fixed in ca6e1206 — switched to the typed `useTheme` from `@apache-superset/core/theme` and dropped the `SupersetTheme` cast.
##########
superset-frontend/src/components/Chart/DrillBy/DrillByChart.tsx:
##########
@@ -45,6 +46,7 @@ export default function DrillByChart({
onContextMenu,
inContextMenu,
}: DrillByChartProps) {
+ const theme = useTheme() as SupersetTheme;
const hooks = useMemo(() => ({ onContextMenu }), [onContextMenu]);
Review Comment:
Fixed in c704cedd — moved the `useTheme` import over to `@apache-superset/core/theme` (combined with the existing `css` import) and dropped the cast.
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -175,111 +178,263 @@ 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);
- }
+interface ChartRendererState {
+ showContextMenu: boolean;
+ inContextMenu: boolean;
+ legendState: LegendState | undefined;
+ legendIndex: number;
+}
+
+interface PrevPropsRef {
+ queriesResponse: QueryData[] | null | undefined;
+ datasource: Datasource | undefined;
+ annotationData: AnnotationData | undefined;
+ ownState: OwnState | undefined;
+ filterState: FilterState | undefined;
+ height: number | undefined;
+ width: number | undefined;
+ triggerRender: boolean;
+ labelsColor: Record<string, string> | undefined;
+ labelsColorMap: Record<string, string> | undefined;
+ formData: QueryFormData;
+ cacheBusterProp: string | undefined;
+ emitCrossFilters: boolean | undefined;
+ postTransformProps: ((props: JsonObject) => JsonObject) | undefined;
+}
+
+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,
+ labelsColor,
+ labelsColorMap,
+ height,
+ width,
+ vizType: propVizType,
+ chartAlert,
+ chartStatus,
+ queriesResponse,
+ chartIsStale,
+ ownState,
+ filterState,
+ postTransformProps,
+ source,
+ emitCrossFilters,
+ cacheBusterProp,
+ onChartStateChange,
+ } = restProps;
+
+ const theme = useTheme() as SupersetTheme;
+
+ 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 mutableQueriesResponseRef = useRef<QueryData[] | null | undefined>(
+ cloneDeep(queriesResponse),
+ );
+ const contextMenuRef = createRef<ChartContextMenuRef>();
+
+ // Track previous props for shouldComponentUpdate logic
+ const prevPropsRef = useRef<PrevPropsRef>({
+ queriesResponse,
+ datasource,
+ annotationData,
+ ownState,
+ filterState,
+ height,
+ width,
+ triggerRender,
+ labelsColor,
+ labelsColorMap,
+ formData,
+ cacheBusterProp,
+ emitCrossFilters,
+ postTransformProps,
+ });
+
+ // Handler functions
+ const handleAddFilter = useCallback(
+ (col: string, vals: FilterValue[], merge = true, refresh = true): void => {
+ addFilter?.(col, vals, merge, refresh);
+ },
+ [addFilter],
+ );
+
+ 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
+ if (hasQueryResponseChangeRef.current) {
+ actions.logEvent(LOG_ACTIONS_RENDER_CHART, {
+ slice_id: chartId,
+ viz_type: propVizType,
+ start_offset: renderStartTimeRef.current,
+ ts: new Date().getTime(),
+ 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,
+ );
+
+ // 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],
+ );
+
+ const handleSetControlValue = useCallback(
+ (name: string, value: unknown): void => {
+ if (setControlValue) {
+ setControlValue(name, value);
+ }
+ },
+ [setControlValue],
+ );
+
+ const handleOnContextMenu = useCallback(
+ (offsetX: number, offsetY: number, filters?: ContextMenuFilters): void => {
+ contextMenuRef.current?.open(offsetX, offsetY, filters);
+ setState(prev => ({ ...prev, inContextMenu: true }));
+ },
+ [contextMenuRef],
+ );
Review Comment:
Fixed in 43c1ae22 — switched `contextMenuRef` to `useRef<ChartContextMenuRef>(null)` so the ref object is stable across renders.
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -186,252 +173,323 @@ 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,
+ labelColors: _labelColors,
+ sharedLabelColors: _sharedLabelColors,
+ vizType,
+ isFiltersInitialized: _isFiltersInitialized,
+ 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}
+ data-test={vizType}
Review Comment:
Fixed in 3f59d774 — dropped the `data-test` prop from the `ChartRenderer` call site. It wasn't being forwarded to any DOM node anyway, and the relevant Cypress selectors use `data-test-viz-type` on the dashboard chart container instead.
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -175,111 +178,263 @@ 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);
- }
+interface ChartRendererState {
+ showContextMenu: boolean;
+ inContextMenu: boolean;
+ legendState: LegendState | undefined;
+ legendIndex: number;
+}
+
+interface PrevPropsRef {
+ queriesResponse: QueryData[] | null | undefined;
+ datasource: Datasource | undefined;
+ annotationData: AnnotationData | undefined;
+ ownState: OwnState | undefined;
+ filterState: FilterState | undefined;
+ height: number | undefined;
+ width: number | undefined;
+ triggerRender: boolean;
+ labelsColor: Record<string, string> | undefined;
+ labelsColorMap: Record<string, string> | undefined;
+ formData: QueryFormData;
+ cacheBusterProp: string | undefined;
+ emitCrossFilters: boolean | undefined;
+ postTransformProps: ((props: JsonObject) => JsonObject) | undefined;
+}
+
+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,
+ labelsColor,
+ labelsColorMap,
+ height,
+ width,
+ vizType: propVizType,
+ chartAlert,
+ chartStatus,
+ queriesResponse,
+ chartIsStale,
+ ownState,
+ filterState,
+ postTransformProps,
+ source,
+ emitCrossFilters,
+ cacheBusterProp,
+ onChartStateChange,
+ } = restProps;
+
+ const theme = useTheme() as SupersetTheme;
+
+ 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 mutableQueriesResponseRef = useRef<QueryData[] | null | undefined>(
+ cloneDeep(queriesResponse),
+ );
+ const contextMenuRef = createRef<ChartContextMenuRef>();
+
+ // Track previous props for shouldComponentUpdate logic
+ const prevPropsRef = useRef<PrevPropsRef>({
+ queriesResponse,
+ datasource,
+ annotationData,
+ ownState,
+ filterState,
+ height,
+ width,
+ triggerRender,
+ labelsColor,
+ labelsColorMap,
+ formData,
+ cacheBusterProp,
+ emitCrossFilters,
+ postTransformProps,
+ });
+
+ // Handler functions
+ const handleAddFilter = useCallback(
+ (col: string, vals: FilterValue[], merge = true, refresh = true): void => {
+ addFilter?.(col, vals, merge, refresh);
+ },
+ [addFilter],
+ );
+
+ 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
+ if (hasQueryResponseChangeRef.current) {
+ actions.logEvent(LOG_ACTIONS_RENDER_CHART, {
+ slice_id: chartId,
+ viz_type: propVizType,
+ start_offset: renderStartTimeRef.current,
+ ts: new Date().getTime(),
+ 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,
+ );
+
+ // 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],
+ );
+
+ const handleSetControlValue = useCallback(
+ (name: string, value: unknown): void => {
+ if (setControlValue) {
+ setControlValue(name, value);
+ }
+ },
+ [setControlValue],
+ );
+
+ const handleOnContextMenu = useCallback(
+ (offsetX: number, offsetY: number, filters?: ContextMenuFilters): void => {
+ contextMenuRef.current?.open(offsetX, offsetY, filters);
+ setState(prev => ({ ...prev, inContextMenu: true }));
+ },
+ [contextMenuRef],
+ );
+
+ const handleContextMenuSelected = useCallback((): void => {
+ setState(prev => ({ ...prev, inContextMenu: false }));
+ }, []);
+
+ const handleContextMenuClosed = useCallback((): void => {
+ setState(prev => ({ ...prev, inContextMenu: false }));
+ }, []);
+
+ const handleLegendStateChanged = useCallback(
+ (legendState: LegendState): void => {
+ setState(prev => ({ ...prev, legendState }));
+ },
+ [],
+ );
- shouldComponentUpdate(
- nextProps: ChartRendererProps,
- nextState: ChartRendererState,
- ): boolean {
+ 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.
+ const onContextMenuFallback = useCallback(
+ (event: MouseEvent<HTMLDivElement>): void => {
+ if (!state.inContextMenu) {
+ event.preventDefault();
+ handleOnContextMenu(event.clientX, event.clientY);
+ }
+ },
+ [handleOnContextMenu, state.inContextMenu],
+ );
+
+ 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,
+ ],
+ );
+
+ // shouldComponentUpdate logic - implemented as a useMemo that tracks if we should render
+ // Note: The return value is not used directly, but the useMemo contains necessary
+ // side effects (updating refs).
+ useMemo(() => {
+ const prevProps = prevPropsRef.current;
const resultsReady =
- nextProps.queriesResponse &&
- ['success', 'rendered'].indexOf(nextProps.chartStatus as string) > -1 &&
- !nextProps.queriesResponse?.[0]?.error;
+ queriesResponse &&
+ ['success', 'rendered'].indexOf(chartStatus as string) > -1 &&
+ !queriesResponse?.[0]?.error;
if (resultsReady) {
- if (!isEqual(this.state, nextState)) {
- return true;
- }
- this.hasQueryResponseChange =
- nextProps.queriesResponse !== this.props.queriesResponse;
+ hasQueryResponseChangeRef.current =
+ queriesResponse !== prevProps.queriesResponse;
- if (this.hasQueryResponseChange) {
- this.mutableQueriesResponse = cloneDeep(nextProps.queriesResponse);
+ if (hasQueryResponseChangeRef.current) {
+ mutableQueriesResponseRef.current = cloneDeep(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 nextFormData = formData as JsonObject;
+ const currentFormData = prevProps.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'));
+ nextFormData.matrixify_enable_vertical_layout === true ||
+ nextFormData.matrixify_enable_horizontal_layout === true;
if (!isMatrixifyEnabled) return false;
Review Comment:
Fixed in 814cf2d9 — restored the canonical `matrixify_enable` + `matrixify_mode_rows`/`matrixify_mode_columns` checks (and updated the tests to match). A follow-up commit (08aac731) then removed the whole matrixify comparator as part of the broader cleanup below, since its return value was never consumed.
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -292,285 +447,210 @@ class ChartRenderer extends Component<ChartRendererProps, ChartRendererState> {
);
};
- 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 ||
+ const nextFormData = formData as JsonObject;
+ const currentFormData = prevProps.formData as JsonObject;
+
+ const shouldRender =
+ hasQueryResponseChangeRef.current ||
+ !isEqual(datasource, prevProps.datasource) ||
+ annotationData !== prevProps.annotationData ||
+ ownState !== prevProps.ownState ||
+ filterState !== prevProps.filterState ||
+ height !== prevProps.height ||
+ width !== prevProps.width ||
+ triggerRender === true ||
+ labelsColor !== prevProps.labelsColor ||
+ labelsColorMap !== prevProps.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()
- );
+ cacheBusterProp !== prevProps.cacheBusterProp ||
+ emitCrossFilters !== prevProps.emitCrossFilters ||
+ postTransformProps !== prevProps.postTransformProps ||
+ hasMatrixifyChanges();
+
+ // Update prev props ref
+ prevPropsRef.current = {
+ queriesResponse,
+ datasource,
+ annotationData,
+ ownState,
+ filterState,
+ height,
+ width,
+ triggerRender,
+ labelsColor,
+ labelsColorMap,
+ formData,
+ cacheBusterProp,
+ emitCrossFilters,
+ postTransformProps,
+ };
+
+ return shouldRender;
}
return false;
+ }, [
Review Comment:
Fixed in 08aac731. You're right that the `useMemo` was being misused for side effects with an ignored return value. Since the component is already wrapped in `memo()` (shallow prop compare handles re-render gating), the manual `shouldRender` computation was dead code. Replaced it with idiomatic hooks: a proper `useMemo` that clones `queriesResponse` (legitimate memoization) and a prev-ref + `useEffect` pair that tracks whether `queriesResponse` changed for the render-time logging callbacks. Dropped the now-unused `PrevPropsRef` interface and the dead matrixify comparator along the way.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #39459:
URL: https://github.com/apache/superset/pull/39459#issuecomment-4626855085
Thanks @aminghadersohi! Addressed all three in 0e0685232a:
- **MEDIUM (FilterState shadowing)** — Removed the local `FilterState` interface in `ChartRenderer.tsx` and imported the canonical one from `@superset-ui/core`, so it can't drift from the version `Chart.tsx` already uses.
- **NIT (unused `setDataMask`)** — Dropped the prop from `ChartRendererProps`. It was dead even in the pre-refactor class version (the hook always routed through `actions.updateDataMask`), so there's no call site to wire up.
- **NIT (tautological matrixify test)** — Rewrote `should detect changes in matrixify properties` to assert on the rendered `SuperChart` output (matching its sibling tests) instead of re-checking its own fixture values.
CI green locally (type-check + the full `ChartRenderer.test.tsx` suite pass).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #39459:
URL: https://github.com/apache/superset/pull/39459#issuecomment-4270542311
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/39459?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `76.05634%` with `51 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 64.46%. Comparing base ([`5106afb`](https://app.codecov.io/gh/apache/superset/commit/5106afb07f3e8b81b08d0bca16ed7dc47c8ac8b6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`575722f`](https://app.codecov.io/gh/apache/superset/commit/575722f5d3197fc3684dceaca9680023fc3f9d3f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/39459?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...et-frontend/src/components/Chart/ChartRenderer.tsx](https://app.codecov.io/gh/apache/superset/pull/39459?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fcomponents%2FChart%2FChartRenderer.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvQ2hhcnRSZW5kZXJlci50c3g=) | 72.72% | [39 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39459?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset-frontend/src/components/Chart/Chart.tsx](https://app.codecov.io/gh/apache/superset/pull/39459?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fcomponents%2FChart%2FChart.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvQ2hhcnQudHN4) | 82.60% | [12 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39459?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## chore/lint-cleanup-function-components #39459 +/- ##
==========================================================================
+ Coverage 64.45% 64.46% +0.01%
==========================================================================
Files 2541 2541
Lines 131662 131647 -15
Branches 30517 30521 +4
==========================================================================
+ Hits 84863 84871 +8
+ Misses 45333 45310 -23
Partials 1466 1466
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/39459/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/39459/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `66.08% <76.05%> (+0.02%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/39459?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39459:
URL: https://github.com/apache/superset/pull/39459#issuecomment-4299843412
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39459:
URL: https://github.com/apache/superset/pull/39459#issuecomment-4503775423
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39459:
URL: https://github.com/apache/superset/pull/39459#issuecomment-4270100619
<!-- Bito Reply -->
The flagged issue is correct: the conversion to a functional component removed filterState from ChartProps and the explicit prop passing to ChartRenderer, breaking the passthrough of dashboard dataMask[chartId].filterState to plugins like DeckglLayerVisibility. To resolve, restore filterState in the interface and pass it explicitly.
**superset-frontend/src/components/Chart/Chart.tsx**
```
import {
ensureIsArray,
FeatureFlag,
...
type FilterState,
type JsonObject,
...
} from '@superset-ui/core';
export interface ChartProps {
...
filterState?: FilterState;
}
function Chart({
...
filterState,
...restProps
}: ChartProps): JSX.Element {
...
const renderChartContainer = useCallback(
() => (
<div className="slice_container" data-test="slice-container">
{shouldRenderChart() ? (
<ChartRenderer
...
filterState={filterState}
...
/>
) : ...
</div>
),
[...],
);
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #39459:
URL: https://github.com/apache/superset/pull/39459#discussion_r3104078267
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -186,252 +170,318 @@ 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,
+ labelColors: _labelColors,
+ sharedLabelColors: _sharedLabelColors,
+ vizType,
+ isFiltersInitialized: _isFiltersInitialized,
+ latestQueryFormData,
+ triggerQuery,
+ postTransformProps,
+ emitCrossFilters,
+ onChartStateChange,
+ suppressLoadingSpinner,
+ } = 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}
+ source={dashboardId ? ChartSource.Dashboard : ChartSource.Explore}
+ data-test={vizType}
Review Comment:
Fixed in 21a089b4a0 — `filterState` and `suppressLoadingSpinner` are now destructured from props and passed explicitly to `<ChartRenderer>` (and added to the `renderChartContainer` useCallback dep array). These had been inadvertently dropped when converting from `{...this.props}` spread to explicit props.
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -186,252 +170,318 @@ 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,
+ labelColors: _labelColors,
+ sharedLabelColors: _sharedLabelColors,
+ vizType,
+ isFiltersInitialized: _isFiltersInitialized,
+ latestQueryFormData,
+ triggerQuery,
+ postTransformProps,
+ emitCrossFilters,
+ onChartStateChange,
+ suppressLoadingSpinner,
+ } = 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}
+ source={dashboardId ? ChartSource.Dashboard : ChartSource.Explore}
+ data-test={vizType}
/>
Review Comment:
Fixed in 21a089b4a0 — `filterState` and `suppressLoadingSpinner` are now destructured from props and passed explicitly to `<ChartRenderer>` (and added to the `renderChartContainer` useCallback dep array). These had been inadvertently dropped when converting from `{...this.props}` spread to explicit props.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #39459:
URL: https://github.com/apache/superset/pull/39459#discussion_r3364260456
##########
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:
Done in 9268647bca — typed the `requiredProps` fixture (and the derived `initialProps`/`updatedProps` objects) as the full `ChartRendererProps` and dropped the per-call-site `as ChartRendererProps` casts, so TypeScript now validates the test setup directly. `tsc --noEmit` passes.
##########
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:
Done in 9268647bca — typed the requiredProps fixture (and the derived initialProps/updatedProps objects) as the full ChartRendererProps and dropped the per-call-site `as ChartRendererProps` casts, so TypeScript now validates the test setup directly. tsc --noEmit passes.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #39459:
URL: https://github.com/apache/superset/pull/39459
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39459:
URL: https://github.com/apache/superset/pull/39459#discussion_r3102190053
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -186,252 +170,318 @@ 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,
+ labelColors: _labelColors,
+ sharedLabelColors: _sharedLabelColors,
+ vizType,
+ isFiltersInitialized: _isFiltersInitialized,
+ latestQueryFormData,
+ triggerQuery,
+ postTransformProps,
+ emitCrossFilters,
+ onChartStateChange,
+ suppressLoadingSpinner,
+ } = 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}
+ source={dashboardId ? ChartSource.Dashboard : ChartSource.Explore}
+ data-test={vizType}
/>
Review Comment:
**Suggestion:** `suppressLoadingSpinner` is not forwarded to `ChartRenderer`, so during auto-refresh the parent hides the spinner but the child still returns `null` in its loading branch, causing the chart area to go blank instead of showing the previous data. Pass this prop through to keep refresh behavior consistent. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard charts blank during auto-refresh, hiding previously loaded data.
- ⚠️ Auto-refresh spinner suppression inconsistent with ChartRenderer loading behavior.
```
</details>
```suggestion
suppressLoadingSpinner={suppressLoadingSpinner}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard with at least one chart so it fully loads (chart state populated in
`src/dashboard/components/gridComponents/Chart/Chart.tsx:179-183` with `chartStatus` and
`queriesResponse` coming from Redux `state.charts[props.id]`).
2. Enable or start an auto-refresh cycle on the dashboard; `useIsAutoRefreshing()` in
`src/dashboard/contexts/AutoRefreshContext.tsx:91-93` returns `true`, which is read into
`const suppressLoadingSpinner = useIsAutoRefreshing();` at
`src/dashboard/components/gridComponents/Chart/Chart.tsx:237` and passed down as
`suppressLoadingSpinner={suppressLoadingSpinner}` to `<ChartContainer>` at
`Chart.tsx:725-765`.
3. `ChartContainer` is a thin connector that renders `Chart`
(`src/components/Chart/ChartContainer.tsx:22-40`); inside
`src/components/Chart/Chart.tsx:173-213` the `Chart` function destructures
`suppressLoadingSpinner` from props and uses it only to compute `const showSpinner =
isLoading && !suppressLoadingSpinner;` at lines 422-424, so during auto-refresh
(`suppressLoadingSpinner === true` and `chartStatus === 'loading'`) it chooses
`renderChartContainer()` over the spinner in the return JSX at lines 420-425 and 459-523.
4. `renderChartContainer()` (Chart.tsx:353-384) renders `<ChartRenderer>` without passing
`suppressLoadingSpinner`, so in `src/components/Chart/ChartRenderer.tsx:510-521` the
loading guard `if (chartStatus === 'loading') { if (!restProps.suppressLoadingSpinner ||
!hasValidPreviousData) return null; }` sees `restProps.suppressLoadingSpinner` as
`undefined` and returns `null` even when `queriesResponse` still contains valid previous
data. Because the dashboard `Chart` has also hidden its own overlay when `isLoading &&
!suppressLoadingSpinner` at lines 625-717, the chart area goes completely blank during
auto-refresh instead of displaying the previous results, contrary to the intended behavior
validated by `renders chart during loading when suppressLoadingSpinner has valid data` in
`src/components/Chart/ChartRenderer.test.tsx:22-38`.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fcomponents%2FChart%2FChart.tsx%0A%2A%2ALine%3A%2A%2A%20384%3A384%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20%60suppressLoadingSpinner%60%20is%20not%20forwarded%20to%20%60ChartRenderer%60%2C%20so%20during%20auto-refresh%20the%20parent%20hides%20the%20spinner%20but%20the%20child%20still%20returns%20%60null%60%20in%20its%20loading%20branch%2C%20causing%20the%20chart%20area%20to%20go%20blank%20instead%20of%20showing%20the%20previous%20data.%20Pass%20this%20prop%20through%20to%20keep%20refresh%20behavior%20consistent.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-cl
aude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fcomponents%2FChart%2FChart.tsx%0A%2A%2ALine%3A%2A%2A%20384%3A384%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20%60suppressLoadingSpinner%60%20is%20not%20forwarded%20to%20%60ChartRenderer%60%2C%20so%20during%20auto-refresh%20the%20parent%20hides%20the%20spinner%20but%20the%20child%20still%20returns%20%60null%60%20in%20its%20loading%20branch%2C%20causing%20the%20chart%20area%20to%20go%20blank%20instead%20of%20showing%20the%20previous%20data.%20Pass%20this%20prop%20through%20to%20keep%20refresh%20behavior%20consistent.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Chart/Chart.tsx
**Line:** 384:384
**Comment:**
*Logic Error: `suppressLoadingSpinner` is not forwarded to `ChartRenderer`, so during auto-refresh the parent hides the spinner but the child still returns `null` in its loading branch, causing the chart area to go blank instead of showing the previous data. Pass this prop through to keep refresh behavior consistent.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39459&comment_hash=31b6e0de14630ec1b200c11803fbf9a2f07331a423c6945f67ffabe204b9abe2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39459&comment_hash=31b6e0de14630ec1b200c11803fbf9a2f07331a423c6945f67ffabe204b9abe2&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39459:
URL: https://github.com/apache/superset/pull/39459#issuecomment-4270057067
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #39459:
URL: https://github.com/apache/superset/pull/39459#issuecomment-4270229032
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 08d8ceec0c9bf11751a0ef37a9a1b734a6ebb15f |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69e27450a7b56b0008c24419 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-39459--superset-docs-preview.netlify.app](https://deploy-preview-39459--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-39459--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #39459:
URL: https://github.com/apache/superset/pull/39459#discussion_r3107382710
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -292,285 +447,210 @@ class ChartRenderer extends Component<ChartRendererProps, ChartRendererState> {
);
};
- 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 ||
+ const nextFormData = formData as JsonObject;
+ const currentFormData = prevProps.formData as JsonObject;
+
+ const shouldRender =
+ hasQueryResponseChangeRef.current ||
+ !isEqual(datasource, prevProps.datasource) ||
+ annotationData !== prevProps.annotationData ||
+ ownState !== prevProps.ownState ||
+ filterState !== prevProps.filterState ||
+ height !== prevProps.height ||
+ width !== prevProps.width ||
+ triggerRender === true ||
+ labelsColor !== prevProps.labelsColor ||
+ labelsColorMap !== prevProps.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()
- );
+ cacheBusterProp !== prevProps.cacheBusterProp ||
+ emitCrossFilters !== prevProps.emitCrossFilters ||
+ postTransformProps !== prevProps.postTransformProps ||
+ hasMatrixifyChanges();
+
+ // Update prev props ref
+ prevPropsRef.current = {
+ queriesResponse,
+ datasource,
+ annotationData,
+ ownState,
+ filterState,
+ height,
+ width,
+ triggerRender,
+ labelsColor,
+ labelsColorMap,
+ formData,
+ cacheBusterProp,
+ emitCrossFilters,
+ postTransformProps,
+ };
+
+ return shouldRender;
}
return false;
+ }, [
Review Comment:
The former `shouldComponentUpdate` logic is now implemented as a `useMemo`, but its return value is not used to actually skip rendering. As a result, the component will re-render whenever React decides to render it, and the `useMemo` is also being used for side effects (mutating refs), which is not what `useMemo` is for. If the goal is to preserve the old update gating, move this logic into a custom comparator passed to `memo(…, areEqual)` (or restructure so the expensive parts are memoized) and use `useEffect` for ref updates.
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -175,111 +178,263 @@ 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);
- }
+interface ChartRendererState {
+ showContextMenu: boolean;
+ inContextMenu: boolean;
+ legendState: LegendState | undefined;
+ legendIndex: number;
+}
+
+interface PrevPropsRef {
+ queriesResponse: QueryData[] | null | undefined;
+ datasource: Datasource | undefined;
+ annotationData: AnnotationData | undefined;
+ ownState: OwnState | undefined;
+ filterState: FilterState | undefined;
+ height: number | undefined;
+ width: number | undefined;
+ triggerRender: boolean;
+ labelsColor: Record<string, string> | undefined;
+ labelsColorMap: Record<string, string> | undefined;
+ formData: QueryFormData;
+ cacheBusterProp: string | undefined;
+ emitCrossFilters: boolean | undefined;
+ postTransformProps: ((props: JsonObject) => JsonObject) | undefined;
+}
+
+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,
+ labelsColor,
+ labelsColorMap,
+ height,
+ width,
+ vizType: propVizType,
+ chartAlert,
+ chartStatus,
+ queriesResponse,
+ chartIsStale,
+ ownState,
+ filterState,
+ postTransformProps,
+ source,
+ emitCrossFilters,
+ cacheBusterProp,
+ onChartStateChange,
+ } = restProps;
+
+ const theme = useTheme() as SupersetTheme;
+
+ 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 mutableQueriesResponseRef = useRef<QueryData[] | null | undefined>(
+ cloneDeep(queriesResponse),
+ );
+ const contextMenuRef = createRef<ChartContextMenuRef>();
+
+ // Track previous props for shouldComponentUpdate logic
+ const prevPropsRef = useRef<PrevPropsRef>({
+ queriesResponse,
+ datasource,
+ annotationData,
+ ownState,
+ filterState,
+ height,
+ width,
+ triggerRender,
+ labelsColor,
+ labelsColorMap,
+ formData,
+ cacheBusterProp,
+ emitCrossFilters,
+ postTransformProps,
+ });
+
+ // Handler functions
+ const handleAddFilter = useCallback(
+ (col: string, vals: FilterValue[], merge = true, refresh = true): void => {
+ addFilter?.(col, vals, merge, refresh);
+ },
+ [addFilter],
+ );
+
+ 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
+ if (hasQueryResponseChangeRef.current) {
+ actions.logEvent(LOG_ACTIONS_RENDER_CHART, {
+ slice_id: chartId,
+ viz_type: propVizType,
+ start_offset: renderStartTimeRef.current,
+ ts: new Date().getTime(),
+ 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,
+ );
+
+ // 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],
+ );
+
+ const handleSetControlValue = useCallback(
+ (name: string, value: unknown): void => {
+ if (setControlValue) {
+ setControlValue(name, value);
+ }
+ },
+ [setControlValue],
+ );
+
+ const handleOnContextMenu = useCallback(
+ (offsetX: number, offsetY: number, filters?: ContextMenuFilters): void => {
+ contextMenuRef.current?.open(offsetX, offsetY, filters);
+ setState(prev => ({ ...prev, inContextMenu: true }));
+ },
+ [contextMenuRef],
+ );
+
+ const handleContextMenuSelected = useCallback((): void => {
+ setState(prev => ({ ...prev, inContextMenu: false }));
+ }, []);
+
+ const handleContextMenuClosed = useCallback((): void => {
+ setState(prev => ({ ...prev, inContextMenu: false }));
+ }, []);
+
+ const handleLegendStateChanged = useCallback(
+ (legendState: LegendState): void => {
+ setState(prev => ({ ...prev, legendState }));
+ },
+ [],
+ );
- shouldComponentUpdate(
- nextProps: ChartRendererProps,
- nextState: ChartRendererState,
- ): boolean {
+ 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.
+ const onContextMenuFallback = useCallback(
+ (event: MouseEvent<HTMLDivElement>): void => {
+ if (!state.inContextMenu) {
+ event.preventDefault();
+ handleOnContextMenu(event.clientX, event.clientY);
+ }
+ },
+ [handleOnContextMenu, state.inContextMenu],
+ );
+
+ 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,
+ ],
+ );
+
+ // shouldComponentUpdate logic - implemented as a useMemo that tracks if we should render
+ // Note: The return value is not used directly, but the useMemo contains necessary
+ // side effects (updating refs).
+ useMemo(() => {
+ const prevProps = prevPropsRef.current;
const resultsReady =
- nextProps.queriesResponse &&
- ['success', 'rendered'].indexOf(nextProps.chartStatus as string) > -1 &&
- !nextProps.queriesResponse?.[0]?.error;
+ queriesResponse &&
+ ['success', 'rendered'].indexOf(chartStatus as string) > -1 &&
+ !queriesResponse?.[0]?.error;
if (resultsReady) {
- if (!isEqual(this.state, nextState)) {
- return true;
- }
- this.hasQueryResponseChange =
- nextProps.queriesResponse !== this.props.queriesResponse;
+ hasQueryResponseChangeRef.current =
+ queriesResponse !== prevProps.queriesResponse;
- if (this.hasQueryResponseChange) {
- this.mutableQueriesResponse = cloneDeep(nextProps.queriesResponse);
+ if (hasQueryResponseChangeRef.current) {
+ mutableQueriesResponseRef.current = cloneDeep(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 nextFormData = formData as JsonObject;
+ const currentFormData = prevProps.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'));
+ nextFormData.matrixify_enable_vertical_layout === true ||
+ nextFormData.matrixify_enable_horizontal_layout === true;
if (!isMatrixifyEnabled) return false;
Review Comment:
`hasMatrixifyChanges()` now treats matrixify as enabled based on `matrixify_enable_vertical_layout`/`matrixify_enable_horizontal_layout`, but the rest of the codebase (e.g. `ControlPanelsContainer.tsx`, `DrillBySubmenu.tsx`, `ChartContextMenu.tsx`) uses `matrixify_enable` together with `matrixify_mode_rows`/`matrixify_mode_columns`. Unless those new flags are actually part of the formData schema, this condition will never be true in practice and matrixify-related changes won’t be detected. Suggest reverting to the existing `matrixify_enable` + mode checks (or supporting both during migration).
##########
superset-frontend/src/components/Chart/DrillBy/DrillByChart.tsx:
##########
@@ -45,6 +46,7 @@ export default function DrillByChart({
onContextMenu,
inContextMenu,
}: DrillByChartProps) {
+ const theme = useTheme() as SupersetTheme;
const hooks = useMemo(() => ({ onContextMenu }), [onContextMenu]);
Review Comment:
Same as `ChartRenderer.tsx`: `useTheme` is imported from `@emotion/react` and cast to `SupersetTheme`, but the repo typically uses the typed hook from `@apache-superset/core/theme`. Switching to that avoids the cast and keeps theme usage consistent.
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -186,252 +173,323 @@ 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,
+ labelColors: _labelColors,
+ sharedLabelColors: _sharedLabelColors,
+ vizType,
+ isFiltersInitialized: _isFiltersInitialized,
+ 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}
+ data-test={vizType}
Review Comment:
`ChartRenderer` is being passed a `data-test` prop here, but `ChartRendererProps` no longer declares a `'data-test'` field. In TypeScript this will fail JSX prop type-checking (and the prop is currently not forwarded/used inside `ChartRenderer` anyway). Either add `'data-test'?: string` to `ChartRendererProps` (and optionally apply it to a DOM element), or remove this prop from the call site.
```suggestion
```
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -37,6 +46,8 @@ import {
} from '@superset-ui/core';
import { logging } from '@apache-superset/core/utils';
import { t } from '@apache-superset/core/translation';
+import { SupersetTheme } from '@apache-superset/core/theme';
+import { useTheme } from '@emotion/react';
Review Comment:
`useTheme` is imported from `@emotion/react` and then cast to `SupersetTheme`. The codebase already uses the typed `useTheme` from `@apache-superset/core/theme` (e.g. `ChartContextMenu.tsx`), which avoids the cast and keeps theme typing consistent. Consider switching to `import { useTheme } from '@apache-superset/core/theme'` and dropping the explicit `SupersetTheme` cast/import here.
```suggestion
import { SupersetTheme, useTheme } from '@apache-superset/core/theme';
```
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -175,111 +178,263 @@ 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);
- }
+interface ChartRendererState {
+ showContextMenu: boolean;
+ inContextMenu: boolean;
+ legendState: LegendState | undefined;
+ legendIndex: number;
+}
+
+interface PrevPropsRef {
+ queriesResponse: QueryData[] | null | undefined;
+ datasource: Datasource | undefined;
+ annotationData: AnnotationData | undefined;
+ ownState: OwnState | undefined;
+ filterState: FilterState | undefined;
+ height: number | undefined;
+ width: number | undefined;
+ triggerRender: boolean;
+ labelsColor: Record<string, string> | undefined;
+ labelsColorMap: Record<string, string> | undefined;
+ formData: QueryFormData;
+ cacheBusterProp: string | undefined;
+ emitCrossFilters: boolean | undefined;
+ postTransformProps: ((props: JsonObject) => JsonObject) | undefined;
+}
+
+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,
+ labelsColor,
+ labelsColorMap,
+ height,
+ width,
+ vizType: propVizType,
+ chartAlert,
+ chartStatus,
+ queriesResponse,
+ chartIsStale,
+ ownState,
+ filterState,
+ postTransformProps,
+ source,
+ emitCrossFilters,
+ cacheBusterProp,
+ onChartStateChange,
+ } = restProps;
+
+ const theme = useTheme() as SupersetTheme;
+
+ 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 mutableQueriesResponseRef = useRef<QueryData[] | null | undefined>(
+ cloneDeep(queriesResponse),
+ );
+ const contextMenuRef = createRef<ChartContextMenuRef>();
+
+ // Track previous props for shouldComponentUpdate logic
+ const prevPropsRef = useRef<PrevPropsRef>({
+ queriesResponse,
+ datasource,
+ annotationData,
+ ownState,
+ filterState,
+ height,
+ width,
+ triggerRender,
+ labelsColor,
+ labelsColorMap,
+ formData,
+ cacheBusterProp,
+ emitCrossFilters,
+ postTransformProps,
+ });
+
+ // Handler functions
+ const handleAddFilter = useCallback(
+ (col: string, vals: FilterValue[], merge = true, refresh = true): void => {
+ addFilter?.(col, vals, merge, refresh);
+ },
+ [addFilter],
+ );
+
+ 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
+ if (hasQueryResponseChangeRef.current) {
+ actions.logEvent(LOG_ACTIONS_RENDER_CHART, {
+ slice_id: chartId,
+ viz_type: propVizType,
+ start_offset: renderStartTimeRef.current,
+ ts: new Date().getTime(),
+ 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,
+ );
+
+ // 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],
+ );
+
+ const handleSetControlValue = useCallback(
+ (name: string, value: unknown): void => {
+ if (setControlValue) {
+ setControlValue(name, value);
+ }
+ },
+ [setControlValue],
+ );
+
+ const handleOnContextMenu = useCallback(
+ (offsetX: number, offsetY: number, filters?: ContextMenuFilters): void => {
+ contextMenuRef.current?.open(offsetX, offsetY, filters);
+ setState(prev => ({ ...prev, inContextMenu: true }));
+ },
+ [contextMenuRef],
+ );
Review Comment:
`contextMenuRef` is created with `createRef()` inside the function component, so a new ref object is allocated on every render. This defeats ref stability and forces `handleOnContextMenu`/`hooks` to churn each render. Use `useRef<ChartContextMenuRef>(null)` for a stable ref across renders.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #39459:
URL: https://github.com/apache/superset/pull/39459#discussion_r3133523562
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -186,252 +173,322 @@ 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,
+ labelColors: _labelColors,
+ sharedLabelColors: _sharedLabelColors,
+ vizType,
+ isFiltersInitialized: _isFiltersInitialized,
+ latestQueryFormData,
Review Comment:
Fixed in c01a4ffd — `restProps` isn't spread anywhere, so the `_`-prefixed destructurings of `labelColors`, `sharedLabelColors`, and `isFiltersInitialized` were pure dead code. Removed outright.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39459:
URL: https://github.com/apache/superset/pull/39459#issuecomment-4635273691
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39459:
URL: https://github.com/apache/superset/pull/39459#issuecomment-4443931685
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #39459:
URL: https://github.com/apache/superset/pull/39459#discussion_r3126896611
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -186,252 +173,322 @@ 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,
+ labelColors: _labelColors,
+ sharedLabelColors: _sharedLabelColors,
+ vizType,
+ isFiltersInitialized: _isFiltersInitialized,
+ latestQueryFormData,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>dead code removal</b></div>
<div id="fix">
The variables _labelColors, _sharedLabelColors, and _isFiltersInitialized are destructured from props but never used in the component, creating dead code that should be removed for cleaner code.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
annotationData,
vizType,
latestQueryFormData,
````
</div>
</details>
</div>
<small><i>Code Review Run #b72fdb</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39459:
URL: https://github.com/apache/superset/pull/39459#issuecomment-4307728831
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore(lint): convert ChartRenderer, Chart, DrillByChart to function components [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39459:
URL: https://github.com/apache/superset/pull/39459#issuecomment-4303370784
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org