codeant-ai-for-open-source[bot] commented on code in PR #37679: URL: https://github.com/apache/superset/pull/37679#discussion_r2785029958
########## superset-frontend/src/dashboard/components/ExportDashboardDataModal/ChartSelector.tsx: ########## @@ -0,0 +1,185 @@ +/** + * 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. + */ +import { styled, t } from '@apache-superset/core'; +import { Checkbox } from '@superset-ui/core/components'; + +const Container = styled.div` + ${({ theme }) => ` + padding: ${theme.sizeUnit * 4}px 0; + `} +`; + +const HeaderRow = styled.div` + ${({ theme }) => ` + display: flex; + justify-content: space-between; + align-items: center; + margin-bottom: ${theme.sizeUnit * 3}px; + padding: 0 ${theme.sizeUnit * 4}px; + `} +`; + +const SelectionInfo = styled.div` + ${({ theme }) => ` + color: ${theme.colorTextSecondary}; + font-size: ${theme.sizeUnit * 3.5}px; + `} +`; + +const Actions = styled.div` + ${({ theme }) => ` + display: flex; + gap: ${theme.sizeUnit * 2}px; + `} +`; + +const ActionLink = styled.a` + ${({ theme }) => ` + color: ${theme.colorPrimary}; + cursor: pointer; + font-size: ${theme.sizeUnit * 3.5}px; + + &:hover { + text-decoration: underline; + } + `} +`; + +const ChartList = styled.div` + ${({ theme }) => ` + max-height: 400px; + overflow-y: auto; + border: 1px solid ${theme.colorBorder}; + border-radius: ${theme.sizeUnit}px; + `} +`; + +const ChartItem = styled.div<{ disabled?: boolean }>` + ${({ theme, disabled }) => ` + display: flex; + align-items: center; + padding: ${theme.sizeUnit * 3}px ${theme.sizeUnit * 4}px; + border-bottom: 1px solid ${theme.colorBorderSecondary}; + cursor: ${disabled ? 'not-allowed' : 'pointer'}; + opacity: ${disabled ? 0.5 : 1}; + + &:last-child { + border-bottom: none; + } + + &:hover { + background-color: ${disabled ? 'transparent' : theme.colorBgTextHover}; + } + `} +`; + +const ChartName = styled.span` + ${({ theme }) => ` + margin-left: ${theme.sizeUnit * 2}px; + font-size: ${theme.sizeUnit * 3.5}px; + `} +`; + +const ChartMeta = styled.span` + ${({ theme }) => ` + margin-left: auto; + color: ${theme.colorTextSecondary}; + font-size: ${theme.sizeUnit * 3}px; + `} +`; + +export interface ChartInfo { + id: number; + name: string; + vizType?: string; +} + +export interface ChartSelectorProps { + charts: ChartInfo[]; + selectedChartIds: Set<number>; + onSelectionChange: (selectedIds: Set<number>) => void; + disabled?: boolean; +} + +export const ChartSelector = ({ + charts, + selectedChartIds, + onSelectionChange, + disabled = false, +}: ChartSelectorProps) => { + const handleSelectAll = () => { + const allIds = new Set(charts.map(c => c.id)); + onSelectionChange(allIds); + }; + + const handleDeselectAll = () => { + onSelectionChange(new Set()); + }; + + const handleToggleChart = (chartId: number) => { + if (disabled) return; + + const newSelection = new Set(selectedChartIds); + if (newSelection.has(chartId)) { + newSelection.delete(chartId); + } else { + newSelection.add(chartId); + } + onSelectionChange(newSelection); + }; + + return ( + <Container> + <HeaderRow> + <SelectionInfo> + {t('%s of %s charts selected', selectedChartIds.size, charts.length)} + </SelectionInfo> + <Actions> + <ActionLink onClick={handleSelectAll}> + {t('Select all')} + </ActionLink> + <span>|</span> + <ActionLink onClick={handleDeselectAll}> + {t('Deselect all')} + </ActionLink> + </Actions> + </HeaderRow> + + <ChartList> + {charts.map(chart => ( + <ChartItem + key={chart.id} + onClick={() => handleToggleChart(chart.id)} + disabled={disabled} + > + <Checkbox + checked={selectedChartIds.has(chart.id)} + onChange={() => handleToggleChart(chart.id)} + disabled={disabled} Review Comment: **Suggestion:** Clicking directly on the checkbox will trigger both the checkbox `onChange` and the parent row's `onClick`, calling the toggle handler twice and leaving the selection unchanged, so the checkbox appears unresponsive; stopping event propagation from the checkbox avoids the double toggle. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Checkbox clicks fail to toggle chart selection. - ⚠️ Users perceive chart selection UI as broken. ``` </details> ```suggestion onClick={event => event.stopPropagation()} ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Render the `ChartSelector` component from `superset-frontend/src/dashboard/components/ExportDashboardDataModal/ChartSelector.tsx:120-185` with `disabled={false}`, a non-empty `charts` array, and an `onSelectionChange` handler that logs received IDs. 2. In the rendered output, locate the first chart row generated by the `charts.map` loop at lines 165-181 where `<ChartItem>` (line 166-170) wraps the `<Checkbox>` (lines 171-175). 3. Click directly on the checkbox (not the row background) for that chart: this fires the checkbox `onChange={() => handleToggleChart(chart.id)}` at line 173 and also bubbles a click event to the parent `<ChartItem onClick={() => handleToggleChart(chart.id)}> ` at line 168, calling `handleToggleChart` twice. 4. Observe in the UI and/or console logs that `onSelectionChange` is invoked twice and the final selection state for that chart ID is unchanged (toggled on then off), so checkbox clicks appear to have no effect while clicking the row background toggles correctly. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/ExportDashboardDataModal/ChartSelector.tsx **Line:** 174:174 **Comment:** *Logic Error: Clicking directly on the checkbox will trigger both the checkbox `onChange` and the parent row's `onClick`, calling the toggle handler twice and leaving the selection unchanged, so the checkbox appears unresponsive; stopping event propagation from the checkbox avoids the double toggle. 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%2F37679&comment_hash=78ff6a4fd906b0308e326eaf4fd9309ae039cc21efda70ce96ef84da056899d3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37679&comment_hash=78ff6a4fd906b0308e326eaf4fd9309ae039cc21efda70ce96ef84da056899d3&reaction=dislike'>👎</a> ########## superset-frontend/src/dashboard/components/ExportDashboardDataModal/ChartSelector.tsx: ########## @@ -0,0 +1,185 @@ +/** + * 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. + */ +import { styled, t } from '@apache-superset/core'; +import { Checkbox } from '@superset-ui/core/components'; + +const Container = styled.div` + ${({ theme }) => ` + padding: ${theme.sizeUnit * 4}px 0; + `} +`; + +const HeaderRow = styled.div` + ${({ theme }) => ` + display: flex; + justify-content: space-between; + align-items: center; + margin-bottom: ${theme.sizeUnit * 3}px; + padding: 0 ${theme.sizeUnit * 4}px; + `} +`; + +const SelectionInfo = styled.div` + ${({ theme }) => ` + color: ${theme.colorTextSecondary}; + font-size: ${theme.sizeUnit * 3.5}px; + `} +`; + +const Actions = styled.div` + ${({ theme }) => ` + display: flex; + gap: ${theme.sizeUnit * 2}px; + `} +`; + +const ActionLink = styled.a` + ${({ theme }) => ` + color: ${theme.colorPrimary}; + cursor: pointer; + font-size: ${theme.sizeUnit * 3.5}px; + + &:hover { + text-decoration: underline; + } + `} +`; + +const ChartList = styled.div` + ${({ theme }) => ` + max-height: 400px; + overflow-y: auto; + border: 1px solid ${theme.colorBorder}; + border-radius: ${theme.sizeUnit}px; + `} +`; + +const ChartItem = styled.div<{ disabled?: boolean }>` + ${({ theme, disabled }) => ` + display: flex; + align-items: center; + padding: ${theme.sizeUnit * 3}px ${theme.sizeUnit * 4}px; + border-bottom: 1px solid ${theme.colorBorderSecondary}; + cursor: ${disabled ? 'not-allowed' : 'pointer'}; + opacity: ${disabled ? 0.5 : 1}; + + &:last-child { + border-bottom: none; + } + + &:hover { + background-color: ${disabled ? 'transparent' : theme.colorBgTextHover}; + } + `} +`; + +const ChartName = styled.span` + ${({ theme }) => ` + margin-left: ${theme.sizeUnit * 2}px; + font-size: ${theme.sizeUnit * 3.5}px; + `} +`; + +const ChartMeta = styled.span` + ${({ theme }) => ` + margin-left: auto; + color: ${theme.colorTextSecondary}; + font-size: ${theme.sizeUnit * 3}px; + `} +`; + +export interface ChartInfo { + id: number; + name: string; + vizType?: string; +} + +export interface ChartSelectorProps { + charts: ChartInfo[]; + selectedChartIds: Set<number>; + onSelectionChange: (selectedIds: Set<number>) => void; + disabled?: boolean; +} + +export const ChartSelector = ({ + charts, + selectedChartIds, + onSelectionChange, + disabled = false, +}: ChartSelectorProps) => { + const handleSelectAll = () => { + const allIds = new Set(charts.map(c => c.id)); + onSelectionChange(allIds); + }; + + const handleDeselectAll = () => { Review Comment: **Suggestion:** When the selector is disabled (e.g. during export), the "Select all" and "Deselect all" links still change the selection because their handlers ignore the `disabled` flag, allowing state changes that the UI is supposed to prevent. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Disabled selector still mutates chart selection via links. - ⚠️ Users can change export selection during supposed lockout. ``` </details> ```suggestion if (disabled) { return; } const allIds = new Set(charts.map(c => c.id)); onSelectionChange(allIds); }; const handleDeselectAll = () => { if (disabled) { return; } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Render the `ChartSelector` component from `superset-frontend/src/dashboard/components/ExportDashboardDataModal/ChartSelector.tsx:120-185` with `disabled={true}`, a non-empty `charts` array, an initial non-empty `selectedChartIds` set, and an `onSelectionChange` handler that logs updates. 2. Note that `disabled` is passed down to `ChartItem` and `Checkbox` (lines 166-170 and 171-175), so individual rows and checkboxes appear visually disabled and `handleToggleChart` early-returns when `disabled` is true (lines 135-138). 3. Despite the disabled state, click the "Select all" link rendered in the header actions at lines 154-156, which is wired directly to `handleSelectAll` at lines 126-129 and does not check `disabled`. 4. Observe that `onSelectionChange` is called and the selection is mutated (e.g., all chart IDs selected or cleared when clicking "Deselect all"), even though the control is visually disabled and row interactions are blocked, leading to inconsistent disabled 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/dashboard/components/ExportDashboardDataModal/ChartSelector.tsx **Line:** 127:131 **Comment:** *Logic Error: When the selector is disabled (e.g. during export), the "Select all" and "Deselect all" links still change the selection because their handlers ignore the `disabled` flag, allowing state changes that the UI is supposed to prevent. 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%2F37679&comment_hash=f22f5fe77e5890aa76b0c0768aaa488463ff38303328bbd6173901262c964954&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37679&comment_hash=f22f5fe77e5890aa76b0c0768aaa488463ff38303328bbd6173901262c964954&reaction=dislike'>👎</a> ########## superset-frontend/src/dashboard/components/ExportDashboardDataModal/useBulkExport.ts: ########## @@ -0,0 +1,250 @@ +/** + * 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. + */ +import { useState, useCallback } from 'react'; +import { SupersetClient } from '@superset-ui/core'; +import { buildV1ChartDataPayload } from 'src/explore/exploreUtils'; +import { ensureAppRoot } from 'src/utils/pathUtils'; + +export interface ChartExportData { + chartId: number; + chartName: string; + data: any[]; + columns: string[]; + status: 'pending' | 'exporting' | 'success' | 'error'; + error?: string; +} + +export interface BulkExportProgress { + current: number; + total: number; + charts: ChartExportData[]; +} + +export interface UseBulkExportProps { + onComplete?: (results: ChartExportData[]) => void; + onError?: (error: string) => void; +} + +export const useBulkExport = ({ + onComplete, + onError, +}: UseBulkExportProps = {}) => { + const [isExporting, setIsExporting] = useState(false); + const [progress, setProgress] = useState<BulkExportProgress>({ + current: 0, + total: 0, + charts: [], + }); + + const sanitizeSheetName = (name: string): string => { + // Excel sheet names: max 31 chars, no special characters + let sanitized = name + .replace(/[:\\/?*[\]]/g, '_') // Replace invalid chars + .substring(0, 31); // Truncate to 31 chars + + return sanitized || 'Sheet'; + }; + + const exportSingleChart = async ( + chartId: number, + chartName: string, + formData: any, + ): Promise<ChartExportData> => { + const chartData: ChartExportData = { + chartId, + chartName, + data: [], + columns: [], + status: 'exporting', + }; + + try { + // Build query payload + const payload = await buildV1ChartDataPayload({ + formData: { + ...formData, + slice_id: chartId, + }, + force: false, + resultFormat: 'json', + resultType: 'full', + setDataMask: () => {}, + ownState: {}, + }); + + // Call chart data API + const response = await SupersetClient.post({ + endpoint: ensureAppRoot('/api/v1/chart/data'), + jsonPayload: payload, + }); + + // Extract data from response + if (response.json?.result?.[0]) { + const [queryResult] = response.json.result; + chartData.data = queryResult.data || []; + chartData.columns = queryResult.colnames || []; + chartData.status = 'success'; + } else { + throw new Error('No data returned from API'); + } + } catch (error: any) { + chartData.status = 'error'; + chartData.error = error.message || 'Failed to export chart'; + } + + return chartData; + }; + + const exportCharts = useCallback( + async (charts: { id: number; name: string; formData: any }[]) => { + setIsExporting(true); Review Comment: **Suggestion:** When `exportCharts` is called with an empty charts array, it still reports "All chart exports failed" via `onError`, even though no export was attempted, which is misleading and can trigger error flows in the UI for a non-error state; adding an early return for the empty-case avoids this incorrect error signaling. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Empty bulk export selection triggers misleading error callback. - ⚠️ Dashboard bulk export UI may show false failure state. - ⚠️ Downstream error-handling flows activated without real error. ``` </details> ```suggestion if (!charts.length) { // Nothing to export; reset state and treat as a no-op instead of an error setIsExporting(false); setProgress({ current: 0, total: 0, charts: [], }); onComplete?.([]); return []; } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Use the `useBulkExport` hook from `superset-frontend/src/dashboard/components/ExportDashboardDataModal/useBulkExport.ts` (function `useBulkExport` starts at line 48 and defines `exportCharts` at line 114). 2. From any consumer of this hook, call the returned `exportCharts` function with an empty array `[]` as argument (this directly invokes the implementation at lines 114–175 in `useBulkExport.ts`). 3. Inside `exportCharts`, observe that `setIsExporting(true)` is called and `setProgress` is set with `total: charts.length` equal to `0`, and `results` is initialized as an empty array (lines 116–131). 4. The `for` loop over `charts` (lines 137–160) is skipped because `charts.length === 0`, so no export attempts are made; after the loop, `successCount` is computed as `0` (no successes), and `onError?.('All chart exports failed')` is invoked at lines 167–170, incorrectly signaling a failure state even though nothing was actually exported. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/ExportDashboardDataModal/useBulkExport.ts **Line:** 116:116 **Comment:** *Logic Error: When `exportCharts` is called with an empty charts array, it still reports "All chart exports failed" via `onError`, even though no export was attempted, which is misleading and can trigger error flows in the UI for a non-error state; adding an early return for the empty-case avoids this incorrect error signaling. 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%2F37679&comment_hash=2a4be7e3e4a4203adb8c286fa1f48ecebc1348bf48b5c05ef8c6b78bcf18ee4a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37679&comment_hash=2a4be7e3e4a4203adb8c286fa1f48ecebc1348bf48b5c05ef8c6b78bcf18ee4a&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: [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]
