codeant-ai-for-open-source[bot] commented on code in PR #37107:
URL: https://github.com/apache/superset/pull/37107#discussion_r2687773649
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -123,8 +234,8 @@ class ChartRenderer extends Component {
onFilterMenuOpen: this.props.onFilterMenuOpen,
onFilterMenuClose: this.props.onFilterMenuClose,
onLegendStateChanged: this.handleLegendStateChanged,
- setDataMask: dataMask => {
- this.props.actions?.updateDataMask(this.props.chartId, dataMask);
+ setDataMask: (dataMask: DataMask) => {
+ this.props.actions?.updateDataMask?.(this.props.chartId, dataMask);
Review Comment:
**Suggestion:** The `setDataMask` hook wired into `SuperChart` currently
ignores the optional `setDataMask` prop and only dispatches
`actions.updateDataMask`, which means any callers that pass a custom
`setDataMask` implementation (for non-dashboard or embedded contexts) will see
their handler silently ignored and cross-filter / server pagination hooks will
not work as intended when `updateDataMask` is absent. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
if (this.props.setDataMask) {
this.props.setDataMask(dataMask);
} else {
this.props.actions?.updateDataMask?.(this.props.chartId, dataMask);
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The PR's final code wires hooks.setDataMask to always call
actions.updateDataMask. The component also exposes a setDataMask prop on
ChartRendererProps, so callers that pass a custom setDataMask will be ignored.
This is a real functional bug: the hook should prefer an explicit prop callback
when present and fall back to the Redux action. The improved code fixes
behavior without changing other logic.
</details>
<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/ChartRenderer.tsx
**Line:** 238:238
**Comment:**
*Possible Bug: The `setDataMask` hook wired into `SuperChart` currently
ignores the optional `setDataMask` prop and only dispatches
`actions.updateDataMask`, which means any callers that pass a custom
`setDataMask` implementation (for non-dashboard or embedded contexts) will see
their handler silently ignored and cross-filter / server pagination hooks will
not work as intended when `updateDataMask` is absent.
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>
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -194,13 +312,18 @@ class ChartRenderer extends Component {
return false;
}
- handleAddFilter(col, vals, merge = true, refresh = true) {
- this.props.addFilter(col, vals, merge, refresh);
+ handleAddFilter(
+ col: string,
+ vals: FilterValue[],
Review Comment:
**Suggestion:** Drill-to-detail behavior is determined using
`formData.viz_type` instead of the `vizType` derived from `currentFormData`, so
when the user changes visualization type without re-running the query (i.e.
`chartIsStale` with `latestQueryFormData`), the metadata lookup can use the old
viz type and incorrectly enable or disable drill-to-detail props. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
.get(vizType)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The file already computes currentFormData and vizType (which takes
latestQueryFormData into account when chartIsStale). Using the original
formData.viz_type can therefore be stale. Switching the lookup to use vizType
(derived from currentFormData) aligns the drill-to-detail check with what is
actually being rendered. This fixes a logic hole that would cause incorrect
enabling/disabling of drill behavior.
</details>
<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/ChartRenderer.tsx
**Line:** 315:317
**Comment:**
*Logic Error: Drill-to-detail behavior is determined using
`formData.viz_type` instead of the `vizType` derived from `currentFormData`, so
when the user changes visualization type without re-running the query (i.e.
`chartIsStale` with `latestQueryFormData`), the metadata lookup can use the old
viz type and incorrectly enable or disable drill-to-detail props.
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>
##########
superset-frontend/src/components/Chart/chartAction.ts:
##########
@@ -0,0 +1,983 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/* eslint no-param-reassign: ["error", { "props": false }] */
+import {
+ FeatureFlag,
+ isDefined,
+ SupersetClient,
+ isFeatureEnabled,
+ getClientErrorObject,
+ QueryFormData,
+ JsonObject,
+ QueryData,
+ AnnotationLayer,
+ DataMask,
+ DatasourceType,
+ LatestQueryFormData,
+} from '@superset-ui/core';
+import { t } from '@apache-superset/core/ui';
+import type { ControlStateMapping } from '@superset-ui/chart-controls';
+import { getControlsState } from 'src/explore/store';
+import {
+ getAnnotationJsonUrl,
+ getExploreUrl,
+ getLegacyEndpointType,
+ buildV1ChartDataPayload,
+ getQuerySettings,
+ getChartDataUri,
+} from 'src/explore/exploreUtils';
+import { addDangerToast } from 'src/components/MessageToasts/actions';
+import { logEvent } from 'src/logger/actions';
+import { Logger, LOG_ACTIONS_LOAD_CHART } from 'src/logger/LogUtils';
+import { allowCrossDomain as domainShardingEnabled } from
'src/utils/hostNamesConfig';
+import { updateDataMask } from 'src/dataMask/actions';
+import { waitForAsyncData } from 'src/middleware/asyncEvent';
+import { ensureAppRoot } from 'src/utils/pathUtils';
+import { safeStringify } from 'src/utils/safeStringify';
+import { extendedDayjs } from '@superset-ui/core/utils/dates';
+import type { Dispatch, Action, AnyAction } from 'redux';
+import type { ThunkAction, ThunkDispatch } from 'redux-thunk';
+import type { History } from 'history';
+import type { ChartState } from 'src/explore/types';
+
+// Types for the Redux state
+export interface ChartsState {
+ [key: string]: ChartState;
+}
+
+export interface CommonState {
+ conf: {
+ SUPERSET_WEBSERVER_TIMEOUT?: number;
+ [key: string]: unknown;
+ };
+}
+
+export interface DashboardInfoState {
+ common: CommonState;
+}
+
+export interface DataMaskState {
+ [key: number]: {
+ ownState?: JsonObject;
+ };
+}
+
+export interface ExploreState {
+ form_data: QueryFormData;
+ [key: string]: unknown;
+}
+
+export interface RootState {
+ charts: ChartsState;
+ common: CommonState;
+ dashboardInfo: DashboardInfoState;
+ dataMask: DataMaskState;
+ explore: ExploreState;
+}
+
+// Action types
+export const CHART_UPDATE_STARTED = 'CHART_UPDATE_STARTED' as const;
+export const CHART_UPDATE_SUCCEEDED = 'CHART_UPDATE_SUCCEEDED' as const;
+export const CHART_UPDATE_STOPPED = 'CHART_UPDATE_STOPPED' as const;
+export const CHART_UPDATE_FAILED = 'CHART_UPDATE_FAILED' as const;
+export const CHART_RENDERING_FAILED = 'CHART_RENDERING_FAILED' as const;
+export const CHART_RENDERING_SUCCEEDED = 'CHART_RENDERING_SUCCEEDED' as const;
+export const REMOVE_CHART = 'REMOVE_CHART' as const;
+export const ANNOTATION_QUERY_SUCCESS = 'ANNOTATION_QUERY_SUCCESS' as const;
+export const ANNOTATION_QUERY_STARTED = 'ANNOTATION_QUERY_STARTED' as const;
+export const ANNOTATION_QUERY_FAILED = 'ANNOTATION_QUERY_FAILED' as const;
+export const DYNAMIC_PLUGIN_CONTROLS_READY =
+ 'DYNAMIC_PLUGIN_CONTROLS_READY' as const;
+export const TRIGGER_QUERY = 'TRIGGER_QUERY' as const;
+export const RENDER_TRIGGERED = 'RENDER_TRIGGERED' as const;
+export const UPDATE_QUERY_FORM_DATA = 'UPDATE_QUERY_FORM_DATA' as const;
+export const UPDATE_CHART_ID = 'UPDATE_CHART_ID' as const;
+export const ADD_CHART = 'ADD_CHART' as const;
+export const POST_CHART_FORM_DATA = 'POST_CHART_FORM_DATA' as const;
Review Comment:
**Suggestion:** The `POST_CHART_FORM_DATA` action type constant is defined
but never dispatched or handled, leaving dead code that can mislead readers
into thinking a corresponding action exists. [code quality]
**Severity Level:** Minor ⚠️
```suggestion
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
I searched the repository for POST_CHART_FORM_DATA and found only this
declaration in chartAction.ts. There are no dispatches or reducer cases that
reference this constant, so it is dead code that could mislead readers into
expecting a corresponding action. Removing it improves clarity.
</details>
<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/chartAction.ts
**Line:** 112:112
**Comment:**
*Code Quality: The `POST_CHART_FORM_DATA` action type constant is
defined but never dispatched or handled, leaving dead code that can mislead
readers into thinking a corresponding action exists.
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>
##########
superset-frontend/src/components/Chart/chartAction.ts:
##########
@@ -0,0 +1,983 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/* eslint no-param-reassign: ["error", { "props": false }] */
+import {
+ FeatureFlag,
+ isDefined,
+ SupersetClient,
+ isFeatureEnabled,
+ getClientErrorObject,
+ QueryFormData,
+ JsonObject,
+ QueryData,
+ AnnotationLayer,
+ DataMask,
+ DatasourceType,
+ LatestQueryFormData,
+} from '@superset-ui/core';
+import { t } from '@apache-superset/core/ui';
+import type { ControlStateMapping } from '@superset-ui/chart-controls';
+import { getControlsState } from 'src/explore/store';
+import {
+ getAnnotationJsonUrl,
+ getExploreUrl,
+ getLegacyEndpointType,
+ buildV1ChartDataPayload,
+ getQuerySettings,
+ getChartDataUri,
+} from 'src/explore/exploreUtils';
+import { addDangerToast } from 'src/components/MessageToasts/actions';
+import { logEvent } from 'src/logger/actions';
+import { Logger, LOG_ACTIONS_LOAD_CHART } from 'src/logger/LogUtils';
+import { allowCrossDomain as domainShardingEnabled } from
'src/utils/hostNamesConfig';
+import { updateDataMask } from 'src/dataMask/actions';
+import { waitForAsyncData } from 'src/middleware/asyncEvent';
+import { ensureAppRoot } from 'src/utils/pathUtils';
+import { safeStringify } from 'src/utils/safeStringify';
+import { extendedDayjs } from '@superset-ui/core/utils/dates';
+import type { Dispatch, Action, AnyAction } from 'redux';
+import type { ThunkAction, ThunkDispatch } from 'redux-thunk';
+import type { History } from 'history';
+import type { ChartState } from 'src/explore/types';
+
+// Types for the Redux state
+export interface ChartsState {
+ [key: string]: ChartState;
+}
+
+export interface CommonState {
+ conf: {
+ SUPERSET_WEBSERVER_TIMEOUT?: number;
+ [key: string]: unknown;
+ };
+}
+
+export interface DashboardInfoState {
+ common: CommonState;
+}
+
+export interface DataMaskState {
+ [key: number]: {
+ ownState?: JsonObject;
+ };
+}
+
+export interface ExploreState {
+ form_data: QueryFormData;
+ [key: string]: unknown;
+}
+
+export interface RootState {
+ charts: ChartsState;
+ common: CommonState;
+ dashboardInfo: DashboardInfoState;
+ dataMask: DataMaskState;
+ explore: ExploreState;
+}
+
+// Action types
+export const CHART_UPDATE_STARTED = 'CHART_UPDATE_STARTED' as const;
+export const CHART_UPDATE_SUCCEEDED = 'CHART_UPDATE_SUCCEEDED' as const;
+export const CHART_UPDATE_STOPPED = 'CHART_UPDATE_STOPPED' as const;
+export const CHART_UPDATE_FAILED = 'CHART_UPDATE_FAILED' as const;
+export const CHART_RENDERING_FAILED = 'CHART_RENDERING_FAILED' as const;
+export const CHART_RENDERING_SUCCEEDED = 'CHART_RENDERING_SUCCEEDED' as const;
+export const REMOVE_CHART = 'REMOVE_CHART' as const;
+export const ANNOTATION_QUERY_SUCCESS = 'ANNOTATION_QUERY_SUCCESS' as const;
+export const ANNOTATION_QUERY_STARTED = 'ANNOTATION_QUERY_STARTED' as const;
+export const ANNOTATION_QUERY_FAILED = 'ANNOTATION_QUERY_FAILED' as const;
+export const DYNAMIC_PLUGIN_CONTROLS_READY =
+ 'DYNAMIC_PLUGIN_CONTROLS_READY' as const;
+export const TRIGGER_QUERY = 'TRIGGER_QUERY' as const;
+export const RENDER_TRIGGERED = 'RENDER_TRIGGERED' as const;
+export const UPDATE_QUERY_FORM_DATA = 'UPDATE_QUERY_FORM_DATA' as const;
+export const UPDATE_CHART_ID = 'UPDATE_CHART_ID' as const;
+export const ADD_CHART = 'ADD_CHART' as const;
+export const POST_CHART_FORM_DATA = 'POST_CHART_FORM_DATA' as const;
+
+// Action interfaces
+export interface ChartUpdateStartedAction {
+ type: typeof CHART_UPDATE_STARTED;
+ queryController: AbortController;
+ latestQueryFormData: QueryFormData | LatestQueryFormData;
+ key: string | number;
+}
+
+export interface ChartUpdateSucceededAction {
+ type: typeof CHART_UPDATE_SUCCEEDED;
+ queriesResponse: QueryData[];
+ key: string | number;
+}
+
+export interface ChartUpdateStoppedAction {
+ type: typeof CHART_UPDATE_STOPPED;
+ key: string | number;
+}
+
+export interface ChartUpdateFailedAction {
+ type: typeof CHART_UPDATE_FAILED;
+ queriesResponse: QueryData[] | JsonObject[];
+ key: string | number;
+}
+
+export interface ChartRenderingFailedAction {
+ type: typeof CHART_RENDERING_FAILED;
+ error: string;
+ key: string | number;
+ stackTrace: string | null;
+}
+
+export interface ChartRenderingSucceededAction {
+ type: typeof CHART_RENDERING_SUCCEEDED;
+ key: string | number;
+}
+
+export interface RemoveChartAction {
+ type: typeof REMOVE_CHART;
+ key: string | number;
+}
+
+export interface AnnotationQuerySuccessAction {
+ type: typeof ANNOTATION_QUERY_SUCCESS;
+ annotation: AnnotationLayer;
+ queryResponse: { data: unknown } | JsonObject;
+ key: string | number;
+}
+
+export interface AnnotationQueryStartedAction {
+ type: typeof ANNOTATION_QUERY_STARTED;
+ annotation: AnnotationLayer;
+ queryController: AbortController;
+ key: string | number;
+}
+
+export interface AnnotationQueryFailedAction {
+ type: typeof ANNOTATION_QUERY_FAILED;
+ annotation: AnnotationLayer;
+ queryResponse: { error: string } | JsonObject;
+ key: string | number;
+}
+
+export interface DynamicPluginControlsReadyAction {
+ type: typeof DYNAMIC_PLUGIN_CONTROLS_READY;
+ key: string | number;
+ controlsState: ControlStateMapping;
+}
+
+export interface TriggerQueryAction {
+ type: typeof TRIGGER_QUERY;
+ value: boolean;
+ key: string | number;
+}
+
+export interface RenderTriggeredAction {
+ type: typeof RENDER_TRIGGERED;
+ value: number;
+ key: string | number;
+}
+
+export interface UpdateQueryFormDataAction {
+ type: typeof UPDATE_QUERY_FORM_DATA;
+ value: QueryFormData | LatestQueryFormData;
+ key: string | number;
+}
+
+export interface UpdateChartIdAction {
+ type: typeof UPDATE_CHART_ID;
+ newId: number;
+ key: string | number;
+}
+
+export interface AddChartAction {
+ type: typeof ADD_CHART;
+ chart: ChartState;
+ key: string | number;
+}
+
+export type ChartAction =
+ | ChartUpdateStartedAction
+ | ChartUpdateSucceededAction
+ | ChartUpdateStoppedAction
+ | ChartUpdateFailedAction
+ | ChartRenderingFailedAction
+ | ChartRenderingSucceededAction
+ | RemoveChartAction
+ | AnnotationQuerySuccessAction
+ | AnnotationQueryStartedAction
+ | AnnotationQueryFailedAction
+ | DynamicPluginControlsReadyAction
+ | TriggerQueryAction
+ | RenderTriggeredAction
+ | UpdateQueryFormDataAction
+ | UpdateChartIdAction
+ | AddChartAction;
+
+// Type for thunk actions
+export type ChartThunkDispatch = ThunkDispatch<RootState, undefined,
AnyAction>;
+export type ChartThunkAction<R = void> = ThunkAction<
+ R,
+ RootState,
+ undefined,
+ AnyAction
+>;
+
+// Request params interface
+export interface RequestParams {
+ signal?: AbortSignal;
+ timeout?: number;
+ dashboard_id?: number;
+ mode?: string;
+ credentials?: RequestCredentials;
+ [key: string]: unknown;
+}
+
+// Query settings type
+export interface QuerySettings extends RequestParams {
+ url?: string;
+ postPayload?: { form_data: QueryFormData | LatestQueryFormData };
+ parseMethod?: string;
+ headers?: Record<string, string>;
+ body?: string;
+}
+
+// Chart data response type
+export interface ChartDataResponse {
+ response: Response;
+ json: {
+ result: QueryData[];
+ };
+}
Review Comment:
**Suggestion:** The `ChartDataResponse` interface is never used, so keeping
it around adds dead code and can confuse future maintainers about the actual
response types in play. [code quality]
**Severity Level:** Minor ⚠️
```suggestion
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
I searched the repo for usages of ChartDataResponse and the only declaration
is this interface in chartAction.ts. It's not referenced elsewhere in the
codebase (no imports/usages found), so keeping it adds dead code and potential
confusion about the actual response shapes used. Removing it is a small, safe
cleanup that doesn't change runtime behavior.
</details>
<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/chartAction.ts
**Line:** 259:265
**Comment:**
*Code Quality: The `ChartDataResponse` interface is never used, so
keeping it around adds dead code and can confuse future maintainers about the
actual response types in play.
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
