codeant-ai-for-open-source[bot] commented on code in PR #37625: URL: https://github.com/apache/superset/pull/37625#discussion_r2772904615
########## superset-frontend/src/features/reports/ReportModal/actions.ts: ########## @@ -0,0 +1,257 @@ +/** + * 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 camelcase: 0 */ +import { SupersetClient } from '@superset-ui/core'; +import { t } from '@apache-superset/core/ui'; +import rison from 'rison'; +import { + addDangerToast, + addSuccessToast, +} from 'src/components/MessageToasts/actions'; +import { isEmpty } from 'lodash'; +import { Dispatch, AnyAction } from 'redux'; +import { ThunkDispatch } from 'redux-thunk'; +import { ReportObject, ReportCreationMethod } from 'src/features/reports/types'; +import { DashboardInfo, ChartsState } from 'src/dashboard/types'; +import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; +import { ExplorePageState } from 'src/explore/types'; + +// Type definitions for report-related state +interface ReportApiResponse { + result?: ReportObject[]; +} + +interface ReportApiJsonResponse { + result: Partial<ReportObject>; + id: number; +} + +interface ReportRootState { + user: UserWithPermissionsAndRoles; + dashboardInfo: DashboardInfo; + charts: ChartsState; + explore: ExplorePageState['explore'] & { + user?: UserWithPermissionsAndRoles; + }; +} + +type ReportFilterField = 'dashboard_id' | 'chart_id'; + +export const SET_REPORT = 'SET_REPORT' as const; + +export interface SetReportAction { + type: typeof SET_REPORT; + report: ReportApiResponse; + resourceId: number; + creationMethod: ReportCreationMethod; + filterField: ReportFilterField; +} + +export function setReport( + report: ReportApiResponse, + resourceId: number, + creationMethod: ReportCreationMethod, + filterField: ReportFilterField, +): SetReportAction { + return { type: SET_REPORT, report, resourceId, creationMethod, filterField }; +} + +interface FetchUISpecificReportParams { + userId: number | undefined; + filterField: ReportFilterField; + creationMethod: ReportCreationMethod; + resourceId: number; +} + +export function fetchUISpecificReport({ + userId, + filterField, + creationMethod, + resourceId, +}: FetchUISpecificReportParams) { + const queryParams = rison.encode({ + filters: [ + { + col: filterField, + opr: 'eq', + value: resourceId, + }, + { + col: 'creation_method', + opr: 'eq', + value: creationMethod, + }, + { + col: 'created_by', + opr: 'rel_o_m', + value: userId, + }, + ], + }); + return function fetchUISpecificReportThunk(dispatch: Dispatch<AnyAction>) { + return SupersetClient.get({ + endpoint: `/api/v1/report/?q=${queryParams}`, + }) + .then(({ json }) => { + dispatch( + setReport( + json as ReportApiResponse, + resourceId, + creationMethod, + filterField, + ), + ); + }) + .catch(() => + dispatch(addDangerToast(t('There was an issue fetching reports.'))), + ); + }; +} + +const structureFetchAction = ( + dispatch: ThunkDispatch<ReportRootState, unknown, AnyAction>, + getState: () => ReportRootState, +) => { + const state = getState(); + const { user, dashboardInfo, charts, explore } = state; + if (!isEmpty(dashboardInfo)) { + dispatch( + fetchUISpecificReport({ + userId: user.userId, + filterField: 'dashboard_id', + creationMethod: 'dashboards', + resourceId: dashboardInfo.id, + }), + ); + } else if (!isEmpty(charts)) { + const [chartArr] = Object.keys(charts); + dispatch( + fetchUISpecificReport({ + userId: explore.user?.userId || user?.userId, + filterField: 'chart_id', + creationMethod: 'charts', + resourceId: charts[chartArr].id, + }), + ); + } +}; + +export const ADD_REPORT = 'ADD_REPORT' as const; + +export interface AddReportAction { + type: typeof ADD_REPORT; + json: ReportApiJsonResponse; +} + +export const addReport = + (report: Partial<ReportObject>) => (dispatch: Dispatch<AnyAction>) => + SupersetClient.post({ + endpoint: `/api/v1/report/`, + jsonPayload: report, + }) + .then(({ json }) => { + dispatch({ type: ADD_REPORT, json } as AddReportAction); + dispatch(addSuccessToast(t('The report has been created'))); + }) + .catch(() => { + dispatch(addDangerToast(t('Failed to create report'))); + }); + +export const EDIT_REPORT = 'EDIT_REPORT' as const; + +export interface EditReportAction { + type: typeof EDIT_REPORT; + json: ReportApiJsonResponse; +} + +export const editReport = + (id: number, report: Partial<ReportObject>) => + (dispatch: Dispatch<AnyAction>) => + SupersetClient.put({ + endpoint: `/api/v1/report/${id}`, + jsonPayload: report, + }) + .then(({ json }) => { + dispatch({ type: EDIT_REPORT, json } as EditReportAction); + dispatch(addSuccessToast(t('Report updated'))); + }) + .catch(() => { + dispatch(addDangerToast(t('Failed to update report'))); + }); + +export function toggleActive(report: ReportObject, isActive: boolean) { + return function toggleActiveThunk( + dispatch: ThunkDispatch<ReportRootState, unknown, AnyAction>, + ) { + return SupersetClient.put({ Review Comment: **Suggestion:** The `toggleActive` thunk assumes `report.id` is always defined, but the `ReportObject` type declares `id` as optional; if a caller ever passes a report without an `id` (which the type system currently allows), this will issue a PUT to `/api/v1/report/undefined`, silently fail to update the correct resource, and still show the generic error toast after the failed request. Adding an early guard for a missing `id` avoids sending an invalid request and provides a consistent error path. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Report activate/deactivate UI may send invalid requests. - ⚠️ Causes confusing error toasts on missing id. - ⚠️ Triggers unnecessary structureFetchAction calls. ``` </details> ```suggestion if (!report.id) { dispatch( addDangerToast( t('We were unable to activate or deactivate this report.'), ), ); return Promise.resolve(); } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Confirm type declaration: ReportObject.id is optional in src/features/reports/types.ts (line 38-39: "id?: number;"), so callers may legally pass a ReportObject without id. 2. Find toggleActive implementation in src/features/reports/ReportModal/actions.ts (lines 198-219). It unconditionally builds endpoint `/api/v1/report/${report.id}` (line 203) and calls SupersetClient.put. 3. Concrete reproduce: (a) From UI or unit test, construct a ReportObject missing id (e.g., { active: true, name: 'x', ... } with no id). (b) Call dispatch(toggleActive(reportWithoutId, true)). (c) The code will call SupersetClient.put to /api/v1/report/undefined (actions.ts:203) — an invalid request. The request will fail, leading to the generic danger toast in .catch (actions.ts:209-215). The code may also still call structureFetchAction in .finally (actions.ts:216-218), causing an extra fetch. 4. This is verifiable by adding a unit test dispatching toggleActive with a ReportObject without id or by instrumenting network calls when performing the action in a dev build. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/features/reports/ReportModal/actions.ts **Line:** 202:202 **Comment:** *Logic Error: The `toggleActive` thunk assumes `report.id` is always defined, but the `ReportObject` type declares `id` as optional; if a caller ever passes a report without an `id` (which the type system currently allows), this will issue a PUT to `/api/v1/report/undefined`, silently fail to update the correct resource, and still show the generic error toast after the failed request. Adding an early guard for a missing `id` avoids sending an invalid request and provides a consistent error path. 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/features/reports/ReportModal/actions.ts: ########## @@ -0,0 +1,257 @@ +/** + * 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 camelcase: 0 */ +import { SupersetClient } from '@superset-ui/core'; +import { t } from '@apache-superset/core/ui'; +import rison from 'rison'; +import { + addDangerToast, + addSuccessToast, +} from 'src/components/MessageToasts/actions'; +import { isEmpty } from 'lodash'; +import { Dispatch, AnyAction } from 'redux'; +import { ThunkDispatch } from 'redux-thunk'; +import { ReportObject, ReportCreationMethod } from 'src/features/reports/types'; +import { DashboardInfo, ChartsState } from 'src/dashboard/types'; +import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; +import { ExplorePageState } from 'src/explore/types'; + +// Type definitions for report-related state +interface ReportApiResponse { + result?: ReportObject[]; +} + +interface ReportApiJsonResponse { + result: Partial<ReportObject>; + id: number; +} + +interface ReportRootState { + user: UserWithPermissionsAndRoles; + dashboardInfo: DashboardInfo; + charts: ChartsState; + explore: ExplorePageState['explore'] & { + user?: UserWithPermissionsAndRoles; + }; +} + +type ReportFilterField = 'dashboard_id' | 'chart_id'; + +export const SET_REPORT = 'SET_REPORT' as const; + +export interface SetReportAction { + type: typeof SET_REPORT; + report: ReportApiResponse; + resourceId: number; + creationMethod: ReportCreationMethod; + filterField: ReportFilterField; +} + +export function setReport( + report: ReportApiResponse, + resourceId: number, + creationMethod: ReportCreationMethod, + filterField: ReportFilterField, +): SetReportAction { + return { type: SET_REPORT, report, resourceId, creationMethod, filterField }; +} + +interface FetchUISpecificReportParams { + userId: number | undefined; + filterField: ReportFilterField; + creationMethod: ReportCreationMethod; + resourceId: number; +} + +export function fetchUISpecificReport({ + userId, + filterField, + creationMethod, + resourceId, +}: FetchUISpecificReportParams) { + const queryParams = rison.encode({ + filters: [ + { + col: filterField, + opr: 'eq', + value: resourceId, + }, + { + col: 'creation_method', + opr: 'eq', + value: creationMethod, + }, + { + col: 'created_by', + opr: 'rel_o_m', + value: userId, + }, + ], + }); + return function fetchUISpecificReportThunk(dispatch: Dispatch<AnyAction>) { + return SupersetClient.get({ + endpoint: `/api/v1/report/?q=${queryParams}`, + }) + .then(({ json }) => { + dispatch( + setReport( + json as ReportApiResponse, + resourceId, + creationMethod, + filterField, + ), + ); + }) + .catch(() => + dispatch(addDangerToast(t('There was an issue fetching reports.'))), + ); + }; +} + +const structureFetchAction = ( + dispatch: ThunkDispatch<ReportRootState, unknown, AnyAction>, + getState: () => ReportRootState, +) => { + const state = getState(); + const { user, dashboardInfo, charts, explore } = state; + if (!isEmpty(dashboardInfo)) { + dispatch( + fetchUISpecificReport({ + userId: user.userId, + filterField: 'dashboard_id', + creationMethod: 'dashboards', + resourceId: dashboardInfo.id, + }), + ); + } else if (!isEmpty(charts)) { + const [chartArr] = Object.keys(charts); + dispatch( + fetchUISpecificReport({ + userId: explore.user?.userId || user?.userId, + filterField: 'chart_id', + creationMethod: 'charts', + resourceId: charts[chartArr].id, + }), + ); + } +}; + +export const ADD_REPORT = 'ADD_REPORT' as const; + +export interface AddReportAction { + type: typeof ADD_REPORT; + json: ReportApiJsonResponse; +} + +export const addReport = + (report: Partial<ReportObject>) => (dispatch: Dispatch<AnyAction>) => + SupersetClient.post({ + endpoint: `/api/v1/report/`, + jsonPayload: report, + }) + .then(({ json }) => { + dispatch({ type: ADD_REPORT, json } as AddReportAction); + dispatch(addSuccessToast(t('The report has been created'))); + }) + .catch(() => { + dispatch(addDangerToast(t('Failed to create report'))); + }); + +export const EDIT_REPORT = 'EDIT_REPORT' as const; + +export interface EditReportAction { + type: typeof EDIT_REPORT; + json: ReportApiJsonResponse; +} + +export const editReport = + (id: number, report: Partial<ReportObject>) => + (dispatch: Dispatch<AnyAction>) => + SupersetClient.put({ + endpoint: `/api/v1/report/${id}`, + jsonPayload: report, + }) + .then(({ json }) => { + dispatch({ type: EDIT_REPORT, json } as EditReportAction); + dispatch(addSuccessToast(t('Report updated'))); + }) + .catch(() => { + dispatch(addDangerToast(t('Failed to update report'))); + }); + +export function toggleActive(report: ReportObject, isActive: boolean) { + return function toggleActiveThunk( + dispatch: ThunkDispatch<ReportRootState, unknown, AnyAction>, + ) { + return SupersetClient.put({ + endpoint: encodeURI(`/api/v1/report/${report.id}`), + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + active: isActive, + }), + }) + .catch(() => { + dispatch( + addDangerToast( + t('We were unable to activate or deactivate this report.'), + ), + ); + }) + .finally(() => { + dispatch(structureFetchAction); + }); + }; +} + +export const DELETE_REPORT = 'DELETE_REPORT' as const; + +/** Minimal fields required to delete a report/alert */ +export interface DeletableReport { + id?: number; + name?: string; + creation_method?: string; + dashboard?: number; + chart?: number; +} + +export interface DeleteReportAction { + type: typeof DELETE_REPORT; + report: DeletableReport; +} + +export function deleteActiveReport(report: DeletableReport) { + return function deleteActiveReportThunk(dispatch: Dispatch<AnyAction>) { + return SupersetClient.delete({ Review Comment: **Suggestion:** Similarly, `deleteActiveReport` uses `report.id` directly even though `DeletableReport.id` is optional; if the function is ever called with an object lacking `id`, the client will attempt to DELETE `/api/v1/report/undefined`, misleadingly show a "Deleted" toast (possibly with an undefined name), and leave the actual report undeleted. Adding a guard to fail fast when `id` is missing prevents invalid API calls and keeps UI state consistent with the backend. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Report deletion UI can attempt invalid DELETE requests. - ⚠️ May show incorrect "Deleted" toast with undefined name. - ⚠️ Backend state may remain unchanged while UI reflects deletion. ``` </details> ```suggestion if (!report.id) { dispatch(addDangerToast(t('Your report could not be deleted'))); return Promise.resolve(); } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Verify DeletableReport typing: in actions.ts the DeletableReport interface defines "id?: number" (lines 225-227), so callers may pass a DeletableReport without id. 2. Inspect deleteActiveReport implementation at actions.ts:238-251. It unconditionally calls SupersetClient.delete with endpoint `/api/v1/report/${report.id}` (line 241). 3. Concrete reproduce: (a) Trigger delete flow in UI or call deleteActiveReport with a DeletableReport object missing id (e.g., constructed by some UI code or malformed state). (b) The client will issue DELETE /api/v1/report/undefined (actions.ts:241). (c) If the backend returns an error, the .catch will show the danger toast; if network or mock returns 200 for that path, the code will dispatch DELETE_REPORT and show "Deleted: %s" possibly with undefined name (actions.ts:244-245), causing UI to reflect deletion though none occurred. 4. This can be reproduced in unit tests or by simulating a malformed DeletableReport and observing the outgoing request and subsequent toasts. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/features/reports/ReportModal/actions.ts **Line:** 240:240 **Comment:** *Logic Error: Similarly, `deleteActiveReport` uses `report.id` directly even though `DeletableReport.id` is optional; if the function is ever called with an object lacking `id`, the client will attempt to DELETE `/api/v1/report/undefined`, misleadingly show a "Deleted" toast (possibly with an undefined name), and leave the actual report undeleted. Adding a guard to fail fast when `id` is missing prevents invalid API calls and keeps UI state consistent with the backend. 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/features/reports/ReportModal/HeaderReportDropdown/index.tsx: ########## @@ -79,7 +79,7 @@ export const useHeaderReportMenuItems = ({ : CreationMethod.Charts; // Select the reports state and specific report with proper reactivity - const report = useSelector<any, AlertObject | null>(state => { + const report = useSelector<any, ReportObject | null>(state => { if (!resourceId) return null; // Select directly from the reports state to ensure reactivity Review Comment: **Suggestion:** The selector currently returns the wrapper object stored in `state.reports[resourceType][resourceId]` instead of the actual report payload under its `result` key (as used in the existing Redux state and tests), so `report.active` and other `ReportObject` fields are `undefined`, which prevents the "Email reports active" checkbox from reflecting the real state and blocks the toggle handler from ever dispatching `toggleActive`. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Email report toggle UI shows incorrect active state. - ❌ Toggle action dispatch sends incorrect payload. - ⚠️ Report management submenu appears inconsistent. ``` </details> ########## superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.tsx: ########## @@ -153,9 +225,9 @@ export const useExploreAdditionalActionsMenu = ( const [isStreamingModalVisible, setIsStreamingModalVisible] = useState(false); const { progress, - isExporting, + isExporting: _isExporting, startExport, - cancelExport, + cancelExport: _cancelExport, Review Comment: **Suggestion:** The streaming export "Cancel" action only hides the modal and resets local progress state but never calls the underlying `cancelExport` from `useStreamingExport`, so the network request continues in the background, wasting resources and potentially confusing users who expect the export to be aborted; wire the close handler to `cancelExport` before resetting state so the request is actually aborted. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Streaming exports continue after modal closed. - ⚠️ Wastes backend and network resources. - ⚠️ Confusing user experience for large exports. ``` </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]
