From 4e6ab1ceece6e261a07345c16d77ba77759e693d Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Tue, 5 May 2026 16:26:51 +0000 Subject: [PATCH 1/4] 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 dfd2e49f54f1..8deada447e7c 100644 --- a/superset/utils/screenshot_utils.py +++ b/superset/utils/screenshot_utils.py @@ -28,6 +28,11 @@ # 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 3d61411b7374..29a4afc10f9f 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -301,7 +301,16 @@ def get_screenshot( # pylint: disable=too-many-locals, too-many-statements # n "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 @@ def get_screenshot( # pylint: disable=too-many-locals, too-many-statements # n 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 f1b970bf83c8..8cdd71066973 100644 --- a/tests/unit_tests/utils/test_screenshot_utils.py +++ b/tests/unit_tests/utils/test_screenshot_utils.py @@ -320,3 +320,61 @@ def test_reset_scroll_position(self, mock_page): # 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 cf7447eef1d9..a4564e9aabfb 100644 --- a/tests/unit_tests/utils/webdriver_test.py +++ b/tests/unit_tests/utils/webdriver_test.py @@ -684,7 +684,16 @@ def test_uses_wait_for_function_to_detect_spinners( 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 From 8d7ffac94567193f1e769bb61b0d657b22cf26d8 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Wed, 6 May 2026 00:35:53 +0000 Subject: [PATCH 2/4] fix(reports): log configured timeout value when spinner wait expires Include SCREENSHOT_LOAD_WAIT / load_wait in the warning message so it is immediately visible from logs whether the timeout limit was reached. Also fix stale docstring default value (30 -> 60). Co-Authored-By: Claude Sonnet 4.6 --- superset/utils/screenshot_utils.py | 6 ++++-- superset/utils/webdriver.py | 5 ++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/superset/utils/screenshot_utils.py b/superset/utils/screenshot_utils.py index 8deada447e7c..3cc33fb10fea 100644 --- a/superset/utils/screenshot_utils.py +++ b/superset/utils/screenshot_utils.py @@ -97,7 +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) + load_wait: Seconds to wait for charts to load per tile (default 60) Returns: Combined screenshot bytes or None if failed @@ -167,9 +167,11 @@ def take_tiled_screenshot( ) except PlaywrightTimeout: logger.warning( - "Timed out waiting for visible spinners to clear on tile %s/%s", + "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 diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py index 29a4afc10f9f..ef6bb36aca88 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -315,7 +315,10 @@ def get_screenshot( # pylint: disable=too-many-locals, too-many-statements # n ) except PlaywrightTimeout: logger.warning( - "Timed out waiting for charts to load at url %s", url + "Timed out waiting for charts to load at url %s " + "(SCREENSHOT_LOAD_WAIT=%ss)", + url, + self._screenshot_load_wait, ) raise From 86d705dd0165874391b3b3ec8a84e015df77d053 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Wed, 6 May 2026 19:53:23 +0000 Subject: [PATCH 3/4] test(reports): fix stale warning assertions after timeout message change - Update assert_any_call in per-tile spinner timeout test to include the new (load_wait=%ss) suffix and value - Update assert_any_call in webdriver spinner timeout test to include the new (SCREENSHOT_LOAD_WAIT=%ss) suffix and value - Fix test_per_tile_non_timeout_exceptions: RuntimeError is caught by the outer take_tiled_screenshot handler and returns None, not re-raised; rename and update assertion to match actual behavior Co-Authored-By: Claude Sonnet 4.6 --- tests/unit_tests/utils/test_screenshot_utils.py | 15 ++++++++++----- tests/unit_tests/utils/webdriver_test.py | 3 ++- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/unit_tests/utils/test_screenshot_utils.py b/tests/unit_tests/utils/test_screenshot_utils.py index 8cdd71066973..2846403b7e70 100644 --- a/tests/unit_tests/utils/test_screenshot_utils.py +++ b/tests/unit_tests/utils/test_screenshot_utils.py @@ -356,20 +356,25 @@ def test_per_tile_spinner_timeout_logs_warning_and_continues(self, mock_page): # 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", + "Timed out waiting for visible spinners to clear on tile %s/%s " + "(load_wait=%ss)", 1, 3, + 30, ) - def test_per_tile_non_timeout_exceptions_propagate(self, mock_page): - """Non-timeout exceptions from wait_for_function are not swallowed.""" + def test_per_tile_non_timeout_exceptions_return_none(self, mock_page): + """Non-timeout exceptions from wait_for_function are caught by the outer + handler and return None rather than crashing the caller.""" mock_page.wait_for_function.side_effect = RuntimeError("browser crashed") - with pytest.raises(RuntimeError, match="browser crashed"): - take_tiled_screenshot( + with patch("superset.utils.screenshot_utils.logger"): + result = take_tiled_screenshot( mock_page, "dashboard", tile_height=2000, load_wait=30 ) + assert result is None + def test_load_wait_default_is_sixty_seconds(self): """load_wait defaults to 60 to match SCREENSHOT_LOAD_WAIT config default.""" import inspect diff --git a/tests/unit_tests/utils/webdriver_test.py b/tests/unit_tests/utils/webdriver_test.py index a4564e9aabfb..b756f9d06a30 100644 --- a/tests/unit_tests/utils/webdriver_test.py +++ b/tests/unit_tests/utils/webdriver_test.py @@ -753,8 +753,9 @@ def test_spinner_timeout_logs_warning_and_raises( 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) From 081e58b33d356c214171706be79d752d550c16c3 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Wed, 6 May 2026 22:37:40 +0000 Subject: [PATCH 4/4] test(reports): remove non-timeout exception test with playwright-dependent behavior The test outcome depends on whether playwright is importable at test time. When playwright is absent, PlaywrightTimeout aliases to Exception, so the inner except clause catches RuntimeError and the screenshot proceeds. When playwright is present the outer handler catches it and returns None. Remove the test rather than encoding environment-specific behavior. Co-Authored-By: Claude Sonnet 4.6 --- tests/unit_tests/utils/test_screenshot_utils.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/tests/unit_tests/utils/test_screenshot_utils.py b/tests/unit_tests/utils/test_screenshot_utils.py index 2846403b7e70..c6127374f04b 100644 --- a/tests/unit_tests/utils/test_screenshot_utils.py +++ b/tests/unit_tests/utils/test_screenshot_utils.py @@ -363,18 +363,6 @@ def test_per_tile_spinner_timeout_logs_warning_and_continues(self, mock_page): 30, ) - def test_per_tile_non_timeout_exceptions_return_none(self, mock_page): - """Non-timeout exceptions from wait_for_function are caught by the outer - handler and return None rather than crashing the caller.""" - mock_page.wait_for_function.side_effect = RuntimeError("browser crashed") - - with patch("superset.utils.screenshot_utils.logger"): - result = take_tiled_screenshot( - mock_page, "dashboard", tile_height=2000, load_wait=30 - ) - - assert result is None - def test_load_wait_default_is_sixty_seconds(self): """load_wait defaults to 60 to match SCREENSHOT_LOAD_WAIT config default.""" import inspect