sadpandajoe commented on code in PR #43805:
URL: https://github.com/apache/superset/pull/43805#discussion_r3975202334
##########
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:
##########
@@ -167,6 +210,121 @@ export const useDownloadMenuItems = (
}
};
+ const triggerExportDownload = async (downloadUrl: string) => {
+ // Fetch the file and save it via an anchor instead of navigating: a link
+ // whose object is already gone answers a JSON error, and a navigation
+ // would replace the dashboard (fatal inside an embedded iframe) instead
+ // of surfacing a retryable toast.
+ const response = await SupersetClient.get({
+ endpoint: downloadUrl,
+ parseMethod: 'raw',
+ });
+ const disposition = response.headers.get('Content-Disposition');
+ let fileName = 'dashboard_export.xlsx';
+ if (disposition) {
+ try {
+ const parsed = parseContentDisposition(disposition);
+ if (parsed?.parameters?.filename) {
+ fileName = parsed.parameters.filename;
+ }
+ } catch (error) {
+ logging.warn('Failed to parse Content-Disposition header:', error);
+ }
+ }
+ const blob = await response.blob();
+ const url = window.URL.createObjectURL(blob);
+ try {
+ const a = document.createElement('a');
+ a.href = url;
+ a.download = fileName;
+ a.style.display = 'none';
+ document.body.appendChild(a);
+ a.click();
+ document.body.removeChild(a);
+ } finally {
+ window.URL.revokeObjectURL(url);
+ }
+ };
+
+ const pollExportStatus = (jobId: string, pollState: ExportPollState) => {
+ if (unmountedRef.current) {
+ return;
+ }
+ SupersetClient.get({
+ endpoint: `/api/v1/dashboard/export_xlsx/status/${jobId}/`,
+ })
+ .then(({ json }) => {
+ // A response in flight when the component unmounts must not navigate
+ // (redirect) or toast on whatever page the user moved to.
+ if (unmountedRef.current) {
+ return;
+ }
+ const {
+ status,
+ download_url: downloadUrl,
+ message,
+ } = json as ExportStatusResponse;
+ if (status === 'ready') {
+ if (downloadUrl) {
+ triggerExportDownload(downloadUrl)
+ .then(() =>
+ addSuccessToast(t('Your export is ready and downloading.')),
+ )
+ .catch(error => {
+ logging.error(error);
+ addDangerToast(
+ t(
+ 'Your export could not be downloaded. It may have expired;
please export again.',
+ ),
+ );
+ });
+ } else {
+ addDangerToast(t('Sorry, something went wrong. Try again later.'));
+ }
+ return;
+ }
+ if (status === 'error') {
+ addDangerToast(
+ message || t('Sorry, something went wrong. Try again later.'),
+ );
+ return;
+ }
+ if (status === 'running' && !pollState.sawRunning) {
+ // The task's execution budget only starts when a worker picks it
+ // up; restart the wait window then, so queue delay doesn't eat it.
+ pollState.sawRunning = true;
+ pollState.deadline = Date.now() + EXPORT_STATUS_POLL_TIMEOUT_MS;
+ }
+ if (Date.now() > pollState.deadline) {
+ addDangerToast(
+ t('Your export is taking longer than expected. Try again later.'),
+ );
+ return;
+ }
+ addExportPendingToast();
Review Comment:
This re-emits the pending toast on every poll. Info toasts expire after four
seconds and `noDuplicate` only checks live toasts, so a long export creates a
new `role="alert"` roughly every six seconds for up to twelve minutes. Can this
use the enqueue toast as the single announcement, or keep one non-repeating
status visible until the terminal result?
##########
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:
##########
@@ -167,6 +210,121 @@ export const useDownloadMenuItems = (
}
};
+ const triggerExportDownload = async (downloadUrl: string) => {
+ // Fetch the file and save it via an anchor instead of navigating: a link
+ // whose object is already gone answers a JSON error, and a navigation
+ // would replace the dashboard (fatal inside an embedded iframe) instead
+ // of surfacing a retryable toast.
+ const response = await SupersetClient.get({
+ endpoint: downloadUrl,
+ parseMethod: 'raw',
+ });
+ const disposition = response.headers.get('Content-Disposition');
+ let fileName = 'dashboard_export.xlsx';
+ if (disposition) {
+ try {
+ const parsed = parseContentDisposition(disposition);
+ if (parsed?.parameters?.filename) {
+ fileName = parsed.parameters.filename;
+ }
+ } catch (error) {
+ logging.warn('Failed to parse Content-Disposition header:', error);
+ }
+ }
+ const blob = await response.blob();
Review Comment:
`response.blob()` buffers the entire workbook in the dashboard tab before
the anchor runs, so a large multi-chart export can exhaust the tab’s memory
even though the backend now streams the response. Can the download remain
non-navigating without materializing the whole file in page memory?
##########
superset/tasks/export_dashboard_excel.py:
##########
@@ -486,28 +635,34 @@ def export_dashboard_excel(
dashboard_title=dashboard_title,
download_url=download_url,
requested_at=requested_at,
- expires_at=expires_at,
+ # Stored naive-local to match is_expired(); the
email
+ # labels its timestamps "UTC", so convert for
display.
+ expires_at=expires_at.astimezone(timezone.utc),
ttl_seconds=ttl,
errored=errored,
),
)
except Exception: # pylint: disable=broad-except
- # The file is already in S3; a send failure should not
trigger
+ # The file is already uploaded; a send failure should not
trigger
# a misleading failure email.
logger.exception("Failed to send export success email")
except SoftTimeLimitExceeded:
logger.warning("Dashboard excel export %s timed out", job_id)
- _send_failure_email(user, dashboard_title, requested_at)
+ _handle_export_failure(user, dashboard_title, requested_at, job_id,
ttl)
raise
except Exception:
logger.exception("Dashboard excel export %s failed", job_id)
- _send_failure_email(user, dashboard_title, requested_at)
+ _handle_export_failure(user, dashboard_title, requested_at, job_id,
ttl)
raise
finally:
try:
ReleaseDistributedLock(
Review Comment:
This release omits the acquisition token, so if the 720-second TTL expires
and another export acquires the same key, the first task’s `finally`
unconditionally deletes the new holder’s lock. With queue delay before a
near-limit export, that permits overlapping exports for the same user and
dashboard. Can the acquisition token be carried into the task and supplied to
`ReleaseDistributedLock`?
--
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]
