From 8d2b655c22d8e678b25f8be1cea8602523bd6f90 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 15 May 2026 13:35:07 -0700 Subject: [PATCH] fix(reports): narrow spinner checks to viewport and tighten exception handling (#39895) Co-authored-by: Claude Sonnet 4.6 --- superset/utils/screenshot_utils.py | 36 ++++++++++- superset/utils/webdriver.py | 62 ++++++++++++++----- .../unit_tests/utils/test_screenshot_utils.py | 51 +++++++++++++++ tests/unit_tests/utils/webdriver_test.py | 3 +- 4 files changed, 134 insertions(+), 18 deletions(-) diff --git a/superset/utils/screenshot_utils.py b/superset/utils/screenshot_utils.py index dfd2e49f54f..3cc33fb10fe 100644 --- a/superset/utils/screenshot_utils.py +++ b/superset/utils/screenshot_utils.py @@ -28,6 +28,11 @@ logger = logging.getLogger(__name__) # Time to wait after scrolling for content to settle and load (in milliseconds) SCROLL_SETTLE_TIMEOUT_MS = 1000 +try: + from playwright.sync_api import TimeoutError as PlaywrightTimeout +except ImportError: + PlaywrightTimeout = Exception + if TYPE_CHECKING: try: from playwright.sync_api import Page @@ -80,7 +85,10 @@ def combine_screenshot_tiles(screenshot_tiles: list[bytes]) -> bytes: def take_tiled_screenshot( - page: "Page", element_name: str, tile_height: int + page: "Page", + element_name: str, + tile_height: int, + load_wait: int = 60, ) -> bytes | None: """ Take a tiled screenshot of a large dashboard by scrolling and capturing sections. @@ -89,6 +97,7 @@ def take_tiled_screenshot( page: Playwright page object element_name: CSS class name of the element to screenshot tile_height: Height of each tile in pixels + load_wait: Seconds to wait for charts to load per tile (default 60) Returns: Combined screenshot bytes or None if failed @@ -139,6 +148,31 @@ def take_tiled_screenshot( ) # Wait for scroll to settle and content to load page.wait_for_timeout(SCROLL_SETTLE_TIMEOUT_MS) + # Wait for any loading spinners visible in the current viewport to clear. + # Only check viewport-visible spinners to avoid blocking on + # virtualization placeholders rendered for off-screen charts. + try: + 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=load_wait * 1000, + ) + except PlaywrightTimeout: + logger.warning( + "Timed out waiting for visible spinners to clear on tile %s/%s " + "(load_wait=%ss)", + i + 1, + num_tiles, + load_wait, + ) # Calculate what portion of the element we want to capture for this tile tile_start_in_element = i * tile_height diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py index 3d61411b737..a7f79e3b415 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -295,21 +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( - "() => 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", url - ) - raise - selenium_animation_wait = app.config[ "SCREENSHOT_SELENIUM_ANIMATION_WAIT" ] @@ -368,7 +353,12 @@ class WebDriverPlaywright(WebDriverProxy): page.set_viewport_size( {"height": tile_height, "width": viewport_width} ) - img = take_tiled_screenshot(page, element_name, tile_height) + img = take_tiled_screenshot( + page, + element_name, + tile_height, + load_wait=self._screenshot_load_wait, + ) if img is None: logger.warning( ( @@ -380,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/test_screenshot_utils.py b/tests/unit_tests/utils/test_screenshot_utils.py index f1b970bf83c..c6127374f04 100644 --- a/tests/unit_tests/utils/test_screenshot_utils.py +++ b/tests/unit_tests/utils/test_screenshot_utils.py @@ -320,3 +320,54 @@ class TestTakeTiledScreenshot: # Each wait should use the scroll settle timeout constant for call in mock_page.wait_for_timeout.call_args_list: assert call[0][0] == SCROLL_SETTLE_TIMEOUT_MS + + def test_per_tile_spinner_wait_uses_viewport_check(self, mock_page): + """wait_for_function polls viewport-visible spinners after each scroll.""" + with patch("superset.utils.screenshot_utils.combine_screenshot_tiles"): + take_tiled_screenshot( + mock_page, "dashboard", tile_height=2000, load_wait=30 + ) + + # 3 tiles → 3 wait_for_function calls, one per tile + assert mock_page.wait_for_function.call_count == 3 + + # Each call uses viewport-scoped JS and the load_wait timeout + for call in mock_page.wait_for_function.call_args_list: + js = call[0][0] + assert "getBoundingClientRect" in js + assert "window.innerHeight" in js + assert call[1]["timeout"] == 30 * 1000 + + def test_per_tile_spinner_timeout_logs_warning_and_continues(self, mock_page): + """A per-tile spinner timeout logs a warning but still takes the screenshot.""" + from superset.utils.screenshot_utils import PlaywrightTimeout + + timeout = PlaywrightTimeout() + mock_page.wait_for_function.side_effect = timeout + + with patch("superset.utils.screenshot_utils.logger") as mock_logger: + with patch("superset.utils.screenshot_utils.combine_screenshot_tiles"): + result = take_tiled_screenshot( + mock_page, "dashboard", tile_height=2000, load_wait=30 + ) + + # Screenshot should still proceed (non-fatal) + assert result is not None + # Warning logged for each tile that timed out + assert mock_logger.warning.call_count == 3 + mock_logger.warning.assert_any_call( + "Timed out waiting for visible spinners to clear on tile %s/%s " + "(load_wait=%ss)", + 1, + 3, + 30, + ) + + def test_load_wait_default_is_sixty_seconds(self): + """load_wait defaults to 60 to match SCREENSHOT_LOAD_WAIT config default.""" + import inspect + + from superset.utils.screenshot_utils import take_tiled_screenshot + + sig = inspect.signature(take_tiled_screenshot) + assert sig.parameters["load_wait"].default == 60 diff --git a/tests/unit_tests/utils/webdriver_test.py b/tests/unit_tests/utils/webdriver_test.py index cf7447eef1d..b2e26269474 100644 --- a/tests/unit_tests/utils/webdriver_test.py +++ b/tests/unit_tests/utils/webdriver_test.py @@ -744,8 +744,9 @@ class TestWebDriverPlaywrightErrorHandling: assert exc_info.value is timeout mock_logger.warning.assert_any_call( - "Timed out waiting for charts to load at url %s", + "Timed out waiting for charts to load at url %s (SCREENSHOT_LOAD_WAIT=%ss)", "http://example.com", + 60, ) @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)