EnxDev commented on code in PR #43805:
URL: https://github.com/apache/superset/pull/43805#discussion_r4062522942
##########
superset/tasks/export_dashboard_excel.py:
##########
@@ -299,6 +346,13 @@ def _write_chart_sheets(
json_body["result_type"] = ChartDataResultType.FULL
json_body.pop("force", None)
+ # Guest authorization links a chart to its dashboard through
+ # ``form_data.dashboardId`` (raise_for_access); saved contexts don't carry
+ # it, so stamp it the way the browser does on interactive requests.
+ form_data = dict(json_body.get("form_data") or {})
+ form_data["dashboardId"] = dashboard_id
+ json_body["form_data"] = form_data
Review Comment:
Could we also set `form_data["slice_id"] = chart.id` here? A chart copied
with **Save as** can retain the source chart's ID in its saved params/query
context. Interactive requests use `Slice.form_data`, which replaces that ID
with the current one, but this export replays the stored value.
If the embedded dashboard contains the copy but not the source chart, the
guest access check rejects the export query and the copy ends up in the
skipped-charts summary even though it renders normally. I reproduced this with
the real schema and access check for both saved and rebuilt contexts; stamping
the current chart ID makes the same payload pass. Could we add a copied-chart
guest case alongside the `dashboardId` regression test?
##########
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:
##########
@@ -167,6 +210,101 @@ export const useDownloadMenuItems = (
}
};
+ const triggerExportDownload = (downloadUrl: string) => {
+ // Stream the file straight to disk via a hidden iframe. The endpoint sends
+ // Content-Disposition: attachment, so the browser saves it without
+ // navigating the dashboard away (fatal inside an embedded iframe) and
+ // without buffering the whole workbook in tab memory the way
fetch().blob()
+ // would. The status endpoint already confirmed the link is ready and
+ // backend-matched, so the only failure left is the narrow race where the
+ // object is removed between that check and this click; such an error
+ // response loads invisibly in the iframe and leaves the page untouched.
+ const iframe = document.createElement('iframe');
+ iframe.style.display = 'none';
+ iframe.src = downloadUrl;
+ document.body.appendChild(iframe);
+ // Remove the iframe after the browser has taken over the download; removal
+ // does not cancel a download already handed off to the browser.
+ setTimeout(() => {
+ if (iframe.parentNode) {
+ iframe.parentNode.removeChild(iframe);
+ }
+ }, EXPORT_STATUS_POLL_INTERVAL_MS);
Review Comment:
Could we avoid tying the download iframe's lifetime to the three-second
polling interval? I reproduced this in Chromium using this function and an
attachment endpoint that waits 4.5 seconds before sending headers: removing the
iframe cancels the request and no download starts, while keeping it alive lets
the same file download successfully.
A slow storage response or busy web server can therefore lose the export
after the success toast, and guests have no email fallback. Keeping the iframe
until the component unmounts, or using cleanup that doesn't race the response,
would avoid this. A browser test with delayed attachment headers would catch it.
--
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]
