codeant-ai-for-open-source[bot] commented on code in PR #37109:
URL: https://github.com/apache/superset/pull/37109#discussion_r2688247425
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx:
##########
@@ -130,7 +225,13 @@ export default function DrillDetailModal({
name={t('Drill to detail: %s', chartName)}
title={t('Drill to detail: %s', chartName)}
footer={
- <ModalFooter exploreChart={exploreChart} canExplore={canExplore} />
+ <ModalFooter
+ exploreChart={exploreChart}
+ canExplore={canExplore}
+ canDownload={canDownload}
+ onDownloadCSV={handleDownloadCSV}
Review Comment:
**Suggestion:** The footer's Close button is wired to an optional
`closeModal` prop, but `ModalFooter` is rendered without passing any
`closeModal` handler; clicking "Close" will do nothing. Pass the modal's hide
handler (`onHideModal`) into `ModalFooter` so the Close button closes the
modal. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
closeModal={onHideModal}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Accurate. ModalFooter defines closeModal?: () => void and the Close button
calls onClick={closeModal}, but the component is instantiated without passing
any handler. That means clicking "Close" in the footer is a no-op. Passing the
existing onHideModal handler (which is available in this scope) fixes the logic
bug without further changes.
</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/DrillDetail/DrillDetailModal.tsx
**Line:** 232:232
**Comment:**
*Logic Error: The footer's Close button is wired to an optional
`closeModal` prop, but `ModalFooter` is rendered without passing any
`closeModal` handler; clicking "Close" will do nothing. Pass the modal's hide
handler (`onHideModal`) into `ModalFooter` so the Close button closes the modal.
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/DrillDetail/DrillDetailModal.tsx:
##########
@@ -65,6 +76,38 @@ const ModalFooter = ({
{t('Edit chart')}
</Button>
)}
+ {canDownload && (
+ <Dropdown
+ trigger={['click']}
+ menu={{
+ items: [
+ {
+ key: 'csv',
+ label: t('Export to CSV'),
+ icon: <Icons.FileOutlined />,
+ onClick: onDownloadCSV,
+ },
+ {
+ key: 'xlsx',
+ label: t('Export to Excel'),
+ icon: <Icons.FileOutlined />,
+ onClick: onDownloadXLSX,
+ },
+ ],
Review Comment:
**Suggestion:** Ant Design's Dropdown with `menu.items` may not reliably run
per-item `onClick` handlers in all versions; put the click handling on the menu
(single onClick) and dispatch by `key` to ensure clicks trigger the intended
download handlers. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
},
{
key: 'xlsx',
label: t('Export to Excel'),
icon: <Icons.FileOutlined />,
},
],
onClick: ({ key }: { key: string }) => {
if (key === 'csv') {
onDownloadCSV();
} else if (key === 'xlsx') {
onDownloadXLSX();
}
},
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid compatibility concern. Ant Design's Dropdown/Menu
historically expects a single menu.onClick handler (and menu items only carry
keys/labels), and per-item onClick isn't always supported across versions.
Dispatching in menu.onClick by key is more robust and avoids flaky behavior if
the component library version in this project doesn't wire per-item onClick.
The suggested improved_code is a safe, minimal change that preserves 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/DrillDetail/DrillDetailModal.tsx
**Line:** 88:96
**Comment:**
*Possible Bug: Ant Design's Dropdown with `menu.items` may not reliably
run per-item `onClick` handlers in all versions; put the click handling on the
menu (single onClick) and dispatch by `key` to ensure clicks trigger the
intended download handlers.
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]
