codeant-ai-for-open-source[bot] commented on code in PR #42051: URL: https://github.com/apache/superset/pull/42051#discussion_r3584152823
########## superset-frontend/src/components/Chart/DrillDetail/DownloadDropdown.tsx: ########## @@ -0,0 +1,82 @@ +/** + * 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 { t } from '@apache-superset/core/translation'; +import { css, useTheme } from '@apache-superset/core/theme'; +import { Dropdown, Tooltip } from '@superset-ui/core/components'; +import { Icons } from '@superset-ui/core/components/Icons'; + +interface DownloadDropdownProps { + onDownloadCSV: () => void; + onDownloadXLSX: () => void; +} + +const DownloadDropdown = ({ + onDownloadCSV, + onDownloadXLSX, +}: DownloadDropdownProps) => { + const theme = useTheme(); + return ( + <Dropdown + trigger={['click']} + menu={{ + onClick: ({ key }) => { + if (key === 'csv') { + onDownloadCSV(); + } else if (key === 'xlsx') { + onDownloadXLSX(); + } + }, + items: [ + { + key: 'csv', + label: t('Export to CSV'), + icon: <Icons.FileOutlined />, + }, + { + key: 'xlsx', + label: t('Export to Excel'), + icon: <Icons.FileOutlined />, + }, + ], + }} + > + <Tooltip title={t('Download')}> + <span + tabIndex={0} + role="button" + aria-label={t('Download')} + data-test="drill-detail-download-btn" Review Comment: **Suggestion:** The trigger is implemented as a focusable `span` with `role="button"` but no keyboard activation handler, while the dropdown only listens to click events. Keyboard users can focus the control but cannot open the menu with Enter/Space, so download actions become unreachable. Use a native `button` element (preferred) or add explicit key handling that opens the dropdown on Enter/Space. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Drill-to-detail download menu unusable with keyboard navigation. - ⚠️ CSV/XLSX exports inaccessible to screen-reader-only users. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open any dashboard chart and use its overflow menu rendered by `SliceHeaderControls` in `superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:163-360`; selecting the “Drill to detail” menu item (`MenuKeys.DrillToDetail` handled in `handleMenuClick` at lines 245-347) sets `drillModalIsOpen` to true. 2. Observe that when `drillModalIsOpen` is true, `SliceHeaderControls` renders a `DrillDetailModal` at lines 673-682, which in turn embeds `DrillDetailPane` with `formData` and `initialFilters` in `superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx:30-35`. 3. In `DrillDetailPane` (`superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx:85-121`), permissions are read via `usePermissions()` (line 120) and, once results are available, the component renders `TableControls` (DrillDetailTableControls) at lines 405-416 with `canDownload={canDownload}` and wired `onDownloadCSV={handleDownloadCSV}` / `onDownloadXLSX={handleDownloadXLSX}`, so a download control appears in the drill-to-detail header. 4. `TableControls` in `superset-frontend/src/components/Chart/DrillDetail/DrillDetailTableControls.tsx:48-59` renders `DownloadDropdown` when `canDownload` is true at lines 138-143, and `DownloadDropdown` in `superset-frontend/src/components/Chart/DrillDetail/DownloadDropdown.tsx:36-79` configures `Dropdown` with `trigger={['click']}` (line 37) and a focusable `<span tabIndex={0} role="button">` trigger (lines 61-66) but no keyboard handlers; when a keyboard-only user tabs to this span and presses Enter or Space, browsers do not synthesize a `click` for non-native buttons, so the dropdown never opens and the CSV/XLSX menu actions wired in `menu.onClick` (lines 38-45) are unreachable via keyboard. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c4af391400fd464ea51670eba325c626&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c4af391400fd464ea51670eba325c626&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(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/DrillDetail/DownloadDropdown.tsx **Line:** 37:65 **Comment:** *Logic Error: The trigger is implemented as a focusable `span` with `role="button"` but no keyboard activation handler, while the dropdown only listens to click events. Keyboard users can focus the control but cannot open the menu with Enter/Space, so download actions become unreachable. Use a native `button` element (preferred) or add explicit key handling that opens the dropdown on Enter/Space. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42051&comment_hash=5ce766b8b57d0e233290d0f9b87029e5a0654cb0826c73e1297cca8c314b9538&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42051&comment_hash=5ce766b8b57d0e233290d0f9b87029e5a0654cb0826c73e1297cca8c314b9538&reaction=dislike'>👎</a> ########## superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx: ########## @@ -207,6 +219,64 @@ export default function DrillDetailPane({ setPageIndex(0); }, []); + const handleDownload = useCallback( + (exportType: 'csv' | 'xlsx') => { + const drillPayload = getDrillPayload(formData, filters); + if (!drillPayload) { + addDangerToast(t('Unable to generate download payload')); + return; + } + const payload: JsonObject = { + datasource: { + id: parseInt(datasourceId, 10), + type: datasourceType, + }, + queries: [ + { + ...drillPayload, + columns: [], + metrics: [], + orderby: [], + row_limit: ROW_LIMIT, + row_offset: 0, + }, + ], + result_type: 'drill_detail', + result_format: exportType, + force: false, + }; + if (dashboardId) { + payload.form_data = { dashboardId }; + } + SupersetClient.postForm(ensureAppRoot('/api/v1/chart/data'), { + form_data: safeStringify(payload), + }).catch(error => { + addDangerToast( + t('Failed to generate download: %s', error.message || error), + ); + }); Review Comment: **Suggestion:** The catch handler assumes every rejection has a `message` property; if a non-Error value is thrown (string/null/object without `message`), the toast formatting path can throw and suppress the original failure. Safely read the error message with optional access and fallback stringification. [null pointer] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Drill Detail download error toast can crash. - ⚠️ Download failures may surface without user-facing message. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In the Drill-to-Detail pane component `superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx`, inspect `handleDownload` defined at lines 222-258, which builds a `drill_detail` payload and calls `SupersetClient.postForm(ensureAppRoot('/api/v1/chart/data'), { form_data: safeStringify(payload) })`. 2. Note that the `catch` handler at lines 251-257 assumes `error` has a `message` property and formats the toast as `t('Failed to generate download: %s', error.message || error)`, without using optional chaining or guarding against `null`/`undefined`. 3. `handleDownload` is wrapped by `handleDownloadCSV`/`handleDownloadXLSX` at lines 270-278 and these callbacks are passed into `TableControls` via the Drill Detail table controls component (not shown in this diff but wired from `DrillDetailPane` at lines 398-416), which uses `DownloadDropdown` (`superset-frontend/src/components/Chart/DrillDetail/DownloadDropdown.tsx:30-60`) to trigger CSV/XLSX downloads. 4. If `SupersetClient.postForm` rejects with a non-standard error (for example `null`, `undefined`, or an object without a `message` field), accessing `error.message` in the catch block at `DrillDetailPane.tsx:253-255` throws a TypeError before `addDangerToast` runs, so the download failure both crashes this code path and fails to present a user-facing error toast. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0f9cfa96b046497f8bf4ec10956bb6ba&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0f9cfa96b046497f8bf4ec10956bb6ba&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(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/DrillDetail/DrillDetailPane.tsx **Line:** 251:257 **Comment:** *Null Pointer: The catch handler assumes every rejection has a `message` property; if a non-Error value is thrown (string/null/object without `message`), the toast formatting path can throw and suppress the original failure. Safely read the error message with optional access and fallback stringification. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42051&comment_hash=1fcfbc8a52af291f2bbb58bdef86c45141f36b4ba1d56bb41e6b9bfa26d15822&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42051&comment_hash=1fcfbc8a52af291f2bbb58bdef86c45141f36b4ba1d56bb41e6b9bfa26d15822&reaction=dislike'>👎</a> ########## superset-frontend/src/explore/components/DataTablesPane/components/DataTableControls.tsx: ########## @@ -106,9 +114,26 @@ export const TableControls = ({ {(!onRowLimitChange || rowcount < (rowLimit ?? Infinity)) && ( <RowCountLabel rowcount={rowcount} loading={isLoading} /> )} + {canDownload && onDownloadCSV && onDownloadXLSX && ( + <DownloadDropdown + onDownloadCSV={onDownloadCSV} + onDownloadXLSX={onDownloadXLSX} + /> + )} {canDownload && ( <CopyToClipboardButton data={formattedData} columns={columnNames} /> )} + {onReload && ( + <Tooltip title={t('Reload')}> + <Icons.ReloadOutlined + iconColor={theme.colorIcon} + iconSize="l" + aria-label={t('Reload')} + role="button" + onClick={onReload} + /> + </Tooltip> Review Comment: **Suggestion:** The reload icon is given `role="button"` but is not focusable, so keyboard users cannot reach or trigger it. Add keyboard focusability (`tabIndex`) on the interactive element (and keep it as an actual button-equivalent control). [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Reload control inaccessible to keyboard-only users. - ⚠️ Results reload harder in accessible workflows. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In `SingleQueryResultPane` (`superset-frontend/src/explore/components/DataTablesPane/components/SingleQueryResultPane.tsx:49-63,75-92`), observe that the `onReload` prop is forwarded to `TableControls` along with data and metadata, making the reload control part of the standard results table UI. 2. In `TableControls` (`superset-frontend/src/explore/components/DataTablesPane/components/DataTableControls.tsx:59-74,127-137`), the reload UI is rendered when `onReload` is provided: a `<Tooltip>` wraps `<Icons.ReloadOutlined>` configured with `aria-label={t('Reload')}` and `role="button"`, but no `tabIndex` or keyboard event handlers are added. 3. Load any results table that uses these controls (for example, Drill By results via `useResultsTableView` at `superset-frontend/src/components/Chart/DrillBy/useResultsTableView.tsx:34-41,47-59`), and navigate through the controls using the Tab key; focus moves through the filter input, row-limit select, and row count label, but the reload icon is skipped because it is not a focusable element. 4. As a result, keyboard-only users cannot focus or activate the reload control exposed as a `role="button"` element in `DataTableControls.tsx:129-135`, making the reload functionality effectively mouse-only despite being advertised as an interactive button. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2c3b8eeb573648df918fa092e74e229e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2c3b8eeb573648df918fa092e74e229e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(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/explore/components/DataTablesPane/components/DataTableControls.tsx **Line:** 129:135 **Comment:** *Possible Bug: The reload icon is given `role="button"` but is not focusable, so keyboard users cannot reach or trigger it. Add keyboard focusability (`tabIndex`) on the interactive element (and keep it as an actual button-equivalent control). 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42051&comment_hash=f7bb928592639295de31968bc6a8d609ff17b671bca31a306f275e5f50b24a05&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42051&comment_hash=f7bb928592639295de31968bc6a8d609ff17b671bca31a306f275e5f50b24a05&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]
