diff --git a/superset-frontend/src/dashboard/hooks/useDownloadScreenshot.test.ts b/superset-frontend/src/dashboard/hooks/useDownloadScreenshot.test.ts index a53f299bfb77..8af9ac17cb61 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 7610c18daa12..eae6ca79aad2 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({