From 9f37704fb40e03d4d93af5a6e6bd8406e2f5c4fa Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Wed, 13 May 2026 22:24:25 +0000 Subject: [PATCH] fix(reports): scope global spinner wait to non-tiled path only The pre-branch viewport-only wait_for_function ran regardless of which screenshot path was taken. For tiled screenshots, per-tile checks in take_tiled_screenshot already handle every viewport as we scroll, so the pre-branch check is redundant there. For non-tiled screenshots, element.screenshot() captures the full element including below-the-fold content, so a global (all-spinners) check is correct. Move the global querySelector('.loading').length === 0 check into each non-tiled branch and remove it from the pre-branch position, so: - non-tiled path: global check (waits for all charts including below fold) - tiled path: no pre-check; per-tile viewport checks in screenshot_utils handle spinner detection tile by tile Co-Authored-By: Claude Sonnet 4.6 --- superset/utils/webdriver.py | 67 ++++++++++++++---------- tests/unit_tests/utils/webdriver_test.py | 11 +--- 2 files changed, 41 insertions(+), 37 deletions(-) diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py index ef6bb36aca8..a7f79e3b415 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -295,33 +295,6 @@ class WebDriverPlaywright(WebDriverProxy): url, ) raise - try: - # charts took too long to load - logger.debug( - "Wait for loading element of charts to be gone at url: %s", url - ) - page.wait_for_function( - """() => { - const els = document.querySelectorAll('.loading'); - for (const el of els) { - const r = el.getBoundingClientRect(); - if (r.top < window.innerHeight && r.bottom > 0) { - return false; - } - } - return true; - }""", - timeout=self._screenshot_load_wait * 1000, - ) - except PlaywrightTimeout: - logger.warning( - "Timed out waiting for charts to load at url %s " - "(SCREENSHOT_LOAD_WAIT=%ss)", - url, - self._screenshot_load_wait, - ) - raise - selenium_animation_wait = app.config[ "SCREENSHOT_SELENIUM_ANIMATION_WAIT" ] @@ -397,10 +370,50 @@ class WebDriverPlaywright(WebDriverProxy): page, element, element_name ) else: + # Standard screenshot captures the full element including + # below-the-fold content, so wait for all spinners globally. + try: + logger.debug( + "Wait for loading element of charts to be gone" + " at url: %s", + url, + ) + page.wait_for_function( + "() => document.querySelectorAll(" + "'.loading').length === 0", + timeout=self._screenshot_load_wait * 1000, + ) + except PlaywrightTimeout: + logger.warning( + "Timed out waiting for charts to load at url %s " + "(SCREENSHOT_LOAD_WAIT=%ss)", + url, + self._screenshot_load_wait, + ) + raise img = WebDriverPlaywright._get_screenshot( page, element, element_name ) else: + # Standard screenshot captures the full element including + # below-the-fold content, so wait for all spinners globally. + try: + logger.debug( + "Wait for loading element of charts to be gone at url: %s", + url, + ) + page.wait_for_function( + "() => document.querySelectorAll('.loading').length === 0", + timeout=self._screenshot_load_wait * 1000, + ) + except PlaywrightTimeout: + logger.warning( + "Timed out waiting for charts to load at url %s " + "(SCREENSHOT_LOAD_WAIT=%ss)", + url, + self._screenshot_load_wait, + ) + raise img = WebDriverPlaywright._get_screenshot( page, element, element_name ) diff --git a/tests/unit_tests/utils/webdriver_test.py b/tests/unit_tests/utils/webdriver_test.py index b756f9d06a3..b2e26269474 100644 --- a/tests/unit_tests/utils/webdriver_test.py +++ b/tests/unit_tests/utils/webdriver_test.py @@ -684,16 +684,7 @@ class TestWebDriverPlaywrightErrorHandling: driver.get_screenshot("http://example.com", "test-element", mock_user) mock_page.wait_for_function.assert_called_once_with( - """() => { - const els = document.querySelectorAll('.loading'); - for (const el of els) { - const r = el.getBoundingClientRect(); - if (r.top < window.innerHeight && r.bottom > 0) { - return false; - } - } - return true; - }""", + "() => document.querySelectorAll('.loading').length === 0", timeout=60 * 1000, ) # Guard against reintroducing the old snapshot-based approach