You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by di...@apache.org on 2026/04/30 15:04:31 UTC
(superset) branch master updated: fix(dashboard): prevent duplicate screenshot downloads (#39525)
This is an automated email from the ASF dual-hosted git repository.
geido pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 4fcb3144ff1 fix(dashboard): prevent duplicate screenshot downloads (#39525)
4fcb3144ff1 is described below
commit 4fcb3144ff1987c817eb8c58be98f8b149b8d253
Author: Geidō <60...@users.noreply.github.com>
AuthorDate: Thu Apr 30 18:04:24 2026 +0300
fix(dashboard): prevent duplicate screenshot downloads (#39525)
Co-authored-by: Claude Opus 4.7 (1M context) <no...@anthropic.com>
---
.../dashboard/hooks/useDownloadScreenshot.test.ts | 108 ++++++++++++++++++++-
.../src/dashboard/hooks/useDownloadScreenshot.ts | 22 ++++-
2 files changed, 122 insertions(+), 8 deletions(-)
diff --git a/superset-frontend/src/dashboard/hooks/useDownloadScreenshot.test.ts b/superset-frontend/src/dashboard/hooks/useDownloadScreenshot.test.ts
index a53f299bfb7..8af9ac17cb6 100644
--- a/superset-frontend/src/dashboard/hooks/useDownloadScreenshot.test.ts
+++ b/superset-frontend/src/dashboard/hooks/useDownloadScreenshot.test.ts
@@ -51,13 +51,52 @@ jest.mock('src/utils/urlUtils', () => ({
getDashboardUrlParams: jest.fn(() => []),
}));
-test('downloadScreenshot calls API with force=true to ensure fresh screenshots', async () => {
- const mockCacheKey = 'test-cache-key';
+const RETRY_INTERVAL = 3000;
+const DASHBOARD_ID = 123;
+const CACHE_KEY = 'test-cache-key';
+
+const mockPostSuccess = () =>
(SupersetClient.post as jest.Mock).mockResolvedValue({
- json: { cache_key: mockCacheKey },
+ json: { cache_key: CACHE_KEY },
});
- const { result } = renderHook(() => useDownloadScreenshot(123));
+const createResponse = (): Response =>
+ ({
+ headers: { get: () => null },
+ blob: () => Promise.resolve(new Blob(['image-data'])),
+ }) as unknown as Response;
+
+const notReadyError = () => ({ status: 404 });
+
+// Chain several Promise.resolves to drain nested microtasks (.then/.catch/.finally
+// in the hook). setImmediate-based flush would stall under fake timers.
+const flushPromises = async () => {
+ for (let i = 0; i < 10; i += 1) {
+ // eslint-disable-next-line no-await-in-loop
+ await Promise.resolve();
+ }
+};
+
+const triggerDownload = async () => {
+ const { result } = renderHook(() => useDownloadScreenshot(DASHBOARD_ID));
+ await act(async () => {
+ result.current(DownloadScreenshotFormat.PNG);
+ await flushPromises();
+ });
+ return result;
+};
+
+beforeEach(() => {
+ jest.clearAllMocks();
+ // Default: GET hangs so microtask chains don't throw on undefined in tests
+ // that only care about POST behavior.
+ (SupersetClient.get as jest.Mock).mockReturnValue(new Promise(() => {}));
+});
+
+test('downloadScreenshot calls API with force=true to ensure fresh screenshots', async () => {
+ mockPostSuccess();
+
+ const { result } = renderHook(() => useDownloadScreenshot(DASHBOARD_ID));
await act(async () => {
result.current(DownloadScreenshotFormat.PNG);
@@ -71,3 +110,64 @@ test('downloadScreenshot calls API with force=true to ensure fresh screenshots',
expect(callArgs.endpoint).toContain('force');
expect(callArgs.endpoint).toMatch(/force[:%]true|force[:%]!t/);
});
+
+test('does not issue overlapping GETs while a previous GET is in-flight', async () => {
+ jest.useFakeTimers();
+ mockPostSuccess();
+
+ // GET never resolves within the test — simulates a slow screenshot request.
+ (SupersetClient.get as jest.Mock).mockImplementation(
+ () => new Promise(() => {}),
+ );
+
+ await triggerDownload();
+
+ // First (immediate) GET fires right after POST resolves.
+ expect(SupersetClient.get).toHaveBeenCalledTimes(1);
+
+ // Advance past several retry intervals while the first GET is still pending.
+ await act(async () => {
+ jest.advanceTimersByTime(RETRY_INTERVAL * 5);
+ await flushPromises();
+ });
+
+ // isFetching guard must prevent the interval from stacking new requests.
+ expect(SupersetClient.get).toHaveBeenCalledTimes(1);
+
+ jest.clearAllTimers();
+ jest.useRealTimers();
+});
+
+test('triggers only one download when multiple successful responses race', async () => {
+ jest.useFakeTimers();
+ mockPostSuccess();
+
+ // First GET returns 404 (not ready), then resolves 200 for every subsequent call.
+ // Without the isDownloaded guard any late-arriving 200 would trigger a second click.
+ (SupersetClient.get as jest.Mock)
+ .mockRejectedValueOnce(notReadyError())
+ .mockResolvedValue(createResponse());
+
+ // jsdom does not implement URL.createObjectURL / revokeObjectURL — stub them.
+ Object.assign(window.URL, {
+ createObjectURL: jest.fn(() => 'blob:mock'),
+ revokeObjectURL: jest.fn(),
+ });
+ const clickSpy = jest
+ .spyOn(HTMLAnchorElement.prototype, 'click')
+ .mockImplementation(() => {});
+
+ await triggerDownload();
+
+ // Drive several interval ticks so multiple 200 responses could resolve.
+ await act(async () => {
+ jest.advanceTimersByTime(RETRY_INTERVAL * 5);
+ await flushPromises();
+ });
+
+ expect(clickSpy).toHaveBeenCalledTimes(1);
+
+ clickSpy.mockRestore();
+ jest.clearAllTimers();
+ jest.useRealTimers();
+});
diff --git a/superset-frontend/src/dashboard/hooks/useDownloadScreenshot.ts b/superset-frontend/src/dashboard/hooks/useDownloadScreenshot.ts
index 7610c18daa1..eae6ca79aad 100644
--- a/superset-frontend/src/dashboard/hooks/useDownloadScreenshot.ts
+++ b/superset-frontend/src/dashboard/hooks/useDownloadScreenshot.ts
@@ -74,6 +74,8 @@ export const useDownloadScreenshot = (
const downloadScreenshot = useCallback(
(format: DownloadScreenshotFormat) => {
let retries = 0;
+ let isFetching = false;
+ let isDownloaded = false;
const toastIntervalId = setInterval(
() =>
@@ -118,6 +120,11 @@ export const useDownloadScreenshot = (
return response.blob().then(blob => ({ blob, fileName }));
})
.then(({ blob, fileName }) => {
+ if (isDownloaded) {
+ return;
+ }
+ isDownloaded = true;
+ stopIntervals('success');
const url = window.URL.createObjectURL(blob);
const a = document.createElement('a');
a.href = url;
@@ -126,7 +133,6 @@ export const useDownloadScreenshot = (
a.click();
document.body.removeChild(a);
window.URL.revokeObjectURL(url);
- stopIntervals('success');
})
.catch(err => {
if ((err as SupersetApiError).status === 404) {
@@ -135,14 +141,22 @@ export const useDownloadScreenshot = (
});
const fetchImageWithRetry = (cacheKey: string) => {
+ if (isDownloaded || isFetching) {
+ return;
+ }
if (retries >= MAX_RETRIES) {
stopIntervals('failure');
logging.error('Max retries reached');
return;
}
- checkImageReady(cacheKey).catch(() => {
- retries += 1;
- });
+ isFetching = true;
+ checkImageReady(cacheKey)
+ .catch(() => {
+ retries += 1;
+ })
+ .finally(() => {
+ isFetching = false;
+ });
};
SupersetClient.post({