codeant-ai-for-open-source[bot] commented on code in PR #38535:
URL: https://github.com/apache/superset/pull/38535#discussion_r2908331889
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -247,29 +253,80 @@ const SliceHeaderControls = (
props.exportXLSX?.(props.slice.slice_id);
break;
case MenuKeys.DownloadAsImage: {
- // menu closes with a delay, we need to hide it manually,
- // so that we don't capture it on the screenshot
- const menu = document.querySelector(
- '.ant-dropdown:not(.ant-dropdown-hidden)',
- ) as HTMLElement;
- if (menu) {
- menu.style.visibility = 'hidden';
+ // Hide the dropdown overlay so it is not captured in the screenshot
+ const overlayContainer = dropdownOverlayContainerRef.current;
+ if (overlayContainer) {
+ overlayContainer.style.visibility = 'hidden';
}
downloadAsImage(
getScreenshotNodeSelector(props.slice.slice_id),
props.slice.slice_name,
true,
theme,
)(domEvent).then(() => {
- if (menu) {
- menu.style.visibility = 'visible';
+ if (overlayContainer) {
+ overlayContainer.style.visibility = 'visible';
}
});
+ // eslint-disable-next-line no-unused-expressions
props.logEvent?.(LOG_ACTIONS_CHART_DOWNLOAD_AS_IMAGE, {
chartId: props.slice.slice_id,
});
break;
}
+ case MenuKeys.DownloadAsPngTransparent:
+ case MenuKeys.DownloadAsPngSolid: {
+ const overlayContainer = dropdownOverlayContainerRef.current;
+ if (overlayContainer) {
+ overlayContainer.style.visibility = 'hidden';
+ }
+
+ const backgroundType =
+ key === MenuKeys.DownloadAsPngTransparent ? 'transparent' : 'solid';
+
+ downloadAsImage(
+ getScreenshotNodeSelector(props.slice.slice_id),
+ props.slice.slice_name,
+ true,
+ theme,
+ { format: 'png', backgroundType },
+ )(domEvent).then(() => {
+ if (overlayContainer) {
+ overlayContainer.style.visibility = 'visible';
+ }
+ });
+
+ // eslint-disable-next-line no-unused-expressions
+ props.logEvent?.(LOG_ACTIONS_CHART_DOWNLOAD_AS_PNG, {
+ chartId: props.slice.slice_id,
+ backgroundType,
+ });
+ break;
+ }
+ case MenuKeys.DownloadAsPdf: {
+ const overlayContainer = dropdownOverlayContainerRef.current;
+ if (overlayContainer) {
+ overlayContainer.style.visibility = 'hidden';
+ }
+
+ const pdfResult = downloadAsPdf(
+ getScreenshotNodeSelector(props.slice.slice_id),
+ props.slice.slice_name,
+ true,
+ )(domEvent);
+
+ Promise.resolve(pdfResult).then(() => {
+ if (overlayContainer) {
+ overlayContainer.style.visibility = 'visible';
+ }
+ });
+
+ // eslint-disable-next-line no-unused-expressions
+ props.logEvent?.(LOG_ACTIONS_CHART_DOWNLOAD_AS_PDF, {
+ chartId: props.slice.slice_id,
+ });
+ break;
+ }
Review Comment:
**Suggestion:** When the screenshot/PDF download promise rejects (for
example, if DOM-to-image fails), the dropdown overlay container's visibility is
set to hidden and never restored, leaving the download menu permanently
invisible; wrapping the handler call in a Promise and restoring visibility in a
finally block ensures the overlay is always made visible again on both success
and failure. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Chart screenshot (jpeg/png) downloads can leave menu invisible.
- ⚠️ PDF export failures can permanently hide that chart's dropdown.
- ⚠️ Affects dashboard slice menu `NoAnimationDropdown` behavior.
```
</details>
```suggestion
case MenuKeys.DownloadAsImage: {
// Hide the dropdown overlay so it is not captured in the screenshot
const overlayContainer = dropdownOverlayContainerRef.current;
if (overlayContainer) {
overlayContainer.style.visibility = 'hidden';
}
Promise.resolve(
downloadAsImage(
getScreenshotNodeSelector(props.slice.slice_id),
props.slice.slice_name,
true,
theme,
)(domEvent),
).finally(() => {
if (overlayContainer) {
overlayContainer.style.visibility = 'visible';
}
});
// eslint-disable-next-line no-unused-expressions
props.logEvent?.(LOG_ACTIONS_CHART_DOWNLOAD_AS_IMAGE, {
chartId: props.slice.slice_id,
});
break;
}
case MenuKeys.DownloadAsPngTransparent:
case MenuKeys.DownloadAsPngSolid: {
const overlayContainer = dropdownOverlayContainerRef.current;
if (overlayContainer) {
overlayContainer.style.visibility = 'hidden';
}
const backgroundType =
key === MenuKeys.DownloadAsPngTransparent ? 'transparent' :
'solid';
Promise.resolve(
downloadAsImage(
getScreenshotNodeSelector(props.slice.slice_id),
props.slice.slice_name,
true,
theme,
{ format: 'png', backgroundType },
)(domEvent),
).finally(() => {
if (overlayContainer) {
overlayContainer.style.visibility = 'visible';
}
});
// eslint-disable-next-line no-unused-expressions
props.logEvent?.(LOG_ACTIONS_CHART_DOWNLOAD_AS_PNG, {
chartId: props.slice.slice_id,
backgroundType,
});
break;
}
case MenuKeys.DownloadAsPdf: {
const overlayContainer = dropdownOverlayContainerRef.current;
if (overlayContainer) {
overlayContainer.style.visibility = 'hidden';
}
Promise.resolve(
downloadAsPdf(
getScreenshotNodeSelector(props.slice.slice_id),
props.slice.slice_name,
true,
)(domEvent),
).finally(() => {
if (overlayContainer) {
overlayContainer.style.visibility = 'visible';
}
});
// eslint-disable-next-line no-unused-expressions
props.logEvent?.(LOG_ACTIONS_CHART_DOWNLOAD_AS_PDF, {
chartId: props.slice.slice_id,
});
break;
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open any dashboard chart with download permissions so that
`SliceHeaderControls` menu
is rendered
(`superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx`,
component definition around lines 120–220).
2. Click the vertical dots button, then select one of the screenshot
options: "Export
screenshot (jpeg)", "Export screenshot (PNG) > Transparent background",
"Export screenshot
(PNG) > Solid background", or "Export as PDF"; all route to
`handleMenuClick` in the same
file (switch cases for `MenuKeys.DownloadAsImage`,
`MenuKeys.DownloadAsPngTransparent/Solid`, `MenuKeys.DownloadAsPdf` around
lines 180–260).
3. During this click handler,
`dropdownOverlayContainerRef.current.style.visibility` is
set to `'hidden'` before invoking `downloadAsImage(...)(domEvent)` or
`downloadAsPdf(...)(domEvent)`; if the underlying promise rejects (e.g.,
DOM-to-image
fails due to a canvas security error or unexpected DOM state), the
`.then(...)` callback
used to restore `visibility = 'visible'` is never invoked.
4. Because the only restoration logic lives inside `.then(...)` (and there
is no
`.catch`/`finally`), the overlay container remains with `visibility:
hidden`, and since
`NoAnimationDropdown` uses `getPopupContainer={() =>
dropdownOverlayContainerRef.current
?? document.body}` (same file, bottom of component), all future opens of the
dropdown
render into this hidden container, making the chart menu effectively
invisible until the
page is reloaded.
```
</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/SliceHeaderControls/index.tsx
**Line:** 255:329
**Comment:**
*Logic Error: When the screenshot/PDF download promise rejects (for
example, if DOM-to-image fails), the dropdown overlay container's visibility is
set to hidden and never restored, leaving the download menu permanently
invisible; wrapping the handler call in a Promise and restoring visibility in a
finally block ensures the overlay is always made visible again on both success
and failure.
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%2F38535&comment_hash=a35acba8c5a125f706467ab00a2178f58405c01d56c91adbe7b68cb6afe9db6e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38535&comment_hash=a35acba8c5a125f706467ab00a2178f58405c01d56c91adbe7b68cb6afe9db6e&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]
