From 4e6ab1ceece6e261a07345c16d77ba77759e693d Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Tue, 5 May 2026 16:26:51 +0000 Subject: [PATCH] fix(reports): narrow spinner checks to viewport and tighten exception handling - Replace global '.loading' count check in webdriver.py with a getBoundingClientRect viewport-visibility check to avoid deadlock when DashboardVirtualization renders off-screen placeholder spinners - Narrow except clause in screenshot_utils.py from bare Exception to PlaywrightTimeout so non-timeout errors (e.g. browser crash) propagate - Fix load_wait default from 30 s to 60 s to match SCREENSHOT_LOAD_WAIT config default - Add tests covering per-tile spinner wait, timeout warning, non-timeout propagation, and load_wait default value Co-Authored-By: Claude Sonnet 4.6 --- superset/utils/screenshot_utils.py | 34 ++++++++++- superset/utils/webdriver.py | 18 +++++- .../unit_tests/utils/test_screenshot_utils.py | 58 +++++++++++++++++++ tests/unit_tests/utils/webdriver_test.py | 11 +++- 4 files changed, 117 insertions(+), 4 deletions(-) diff --git a/superset/utils/screenshot_utils.py b/superset/utils/screenshot_utils.py index dfd2e49f54f..8deada447e7 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 30) Returns: Combined screenshot bytes or None if failed @@ -139,6 +148,29 @@ 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", + i + 1, + num_tiles, + ) # 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..29a4afc10f9 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -301,7 +301,16 @@ class WebDriverPlaywright(WebDriverProxy): "Wait for loading element of charts to be gone at url: %s", url ) page.wait_for_function( - "() => document.querySelectorAll('.loading').length === 0", + """() => { + 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: @@ -368,7 +377,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( ( diff --git a/tests/unit_tests/utils/test_screenshot_utils.py b/tests/unit_tests/utils/test_screenshot_utils.py index f1b970bf83c..8cdd7106697 100644 --- a/tests/unit_tests/utils/test_screenshot_utils.py +++ b/tests/unit_tests/utils/test_screenshot_utils.py @@ -320,3 +320,61 @@ 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", + 1, + 3, + ) + + def test_per_tile_non_timeout_exceptions_propagate(self, mock_page): + """Non-timeout exceptions from wait_for_function are not swallowed.""" + mock_page.wait_for_function.side_effect = RuntimeError("browser crashed") + + with pytest.raises(RuntimeError, match="browser crashed"): + take_tiled_screenshot( + mock_page, "dashboard", tile_height=2000, load_wait=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..a4564e9aabf 100644 --- a/tests/unit_tests/utils/webdriver_test.py +++ b/tests/unit_tests/utils/webdriver_test.py @@ -684,7 +684,16 @@ class TestWebDriverPlaywrightErrorHandling: driver.get_screenshot("http://example.com", "test-element", mock_user) mock_page.wait_for_function.assert_called_once_with( - "() => document.querySelectorAll('.loading').length === 0", + """() => { + 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=60 * 1000, ) # Guard against reintroducing the old snapshot-based approach