From 199435e5c55e7df227e0cada9340c8137ca44840 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Thu, 23 Apr 2026 02:01:59 +0000 Subject: [PATCH 1/2] fix(reports): poll for spinner absence instead of snapshotting loading elements The Playwright screenshot path waited for `.loading` elements to detach by calling `page.locator(".loading").all()` once and iterating the snapshot. Spinners that appeared after the snapshot was taken (common on long dashboards where charts load lazily) were never waited for, causing PDF reports to capture charts that were still fetching data. Replace the snapshot loop with `page.wait_for_function` which continuously polls `document.querySelectorAll('.loading').length === 0` until the page is genuinely clear of all spinners, then use `self._screenshot_load_wait` (already stored on the base class) for the timeout to stay consistent with the Selenium path. Co-Authored-By: Claude Sonnet 4.6 --- superset/utils/webdriver.py | 6 +- tests/unit_tests/utils/webdriver_test.py | 108 +++++++++++++++++++++++ 2 files changed, 112 insertions(+), 2 deletions(-) diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py index f38c67fd64c7..3d61411b7374 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -300,8 +300,10 @@ def get_screenshot( # pylint: disable=too-many-locals, too-many-statements # n logger.debug( "Wait for loading element of charts to be gone at url: %s", url ) - for loading_element in page.locator(".loading").all(): - loading_element.wait_for(state="detached") + 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 diff --git a/tests/unit_tests/utils/webdriver_test.py b/tests/unit_tests/utils/webdriver_test.py index ad55f4b68e60..cf7447eef1d9 100644 --- a/tests/unit_tests/utils/webdriver_test.py +++ b/tests/unit_tests/utils/webdriver_test.py @@ -640,6 +640,114 @@ def test_find_unexpected_errors_processes_alerts( mock_button.click.assert_called_once() mock_close_button.click.assert_called_once() + @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True) + @patch("superset.utils.webdriver.sync_playwright") + @patch("superset.utils.webdriver.app") + def test_uses_wait_for_function_to_detect_spinners( + self, mock_app, mock_sync_playwright + ): + """wait_for_function polls for spinner absence rather than snapshotting.""" + mock_user = MagicMock() + mock_user.username = "test_user" + mock_app.config = { + "WEBDRIVER_OPTION_ARGS": [], + "WEBDRIVER_WINDOW": {"pixel_density": 1}, + "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000, + "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle", + "SCREENSHOT_SELENIUM_HEADSTART": 0, + "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 0, + "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False, + "SCREENSHOT_TILED_ENABLED": False, + "SCREENSHOT_LOCATE_WAIT": 10, + "SCREENSHOT_LOAD_WAIT": 60, + "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10, + "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10, + } + + mock_playwright_instance = MagicMock() + mock_browser = MagicMock() + mock_context = MagicMock() + mock_page = MagicMock() + mock_element = MagicMock() + + mock_sync_playwright.return_value.__enter__.return_value = ( + mock_playwright_instance + ) + mock_playwright_instance.chromium.launch.return_value = mock_browser + mock_browser.new_context.return_value = mock_context + mock_context.new_page.return_value = mock_page + mock_page.locator.return_value = mock_element + mock_element.screenshot.return_value = b"screenshot" + + with patch.object(WebDriverPlaywright, "auth", return_value=mock_context): + driver = WebDriverPlaywright("chrome") + driver.get_screenshot("http://example.com", "test-element", mock_user) + + mock_page.wait_for_function.assert_called_once_with( + "() => document.querySelectorAll('.loading').length === 0", + timeout=60 * 1000, + ) + # Guard against reintroducing the old snapshot-based approach + loading_locator_calls = [ + c for c in mock_page.locator.call_args_list if c.args == (".loading",) + ] + assert loading_locator_calls == [] + + @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True) + @patch("superset.utils.webdriver.sync_playwright") + @patch("superset.utils.webdriver.logger") + @patch("superset.utils.webdriver.app") + def test_spinner_timeout_logs_warning_and_raises( + self, mock_app, mock_logger, mock_sync_playwright + ): + """Spinner timeout is logged as a warning and re-raised.""" + from superset.utils.webdriver import PlaywrightTimeout + + mock_user = MagicMock() + mock_user.username = "test_user" + mock_app.config = { + "WEBDRIVER_OPTION_ARGS": [], + "WEBDRIVER_WINDOW": {"pixel_density": 1}, + "SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000, + "SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle", + "SCREENSHOT_SELENIUM_HEADSTART": 0, + "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 0, + "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False, + "SCREENSHOT_TILED_ENABLED": False, + "SCREENSHOT_LOCATE_WAIT": 10, + "SCREENSHOT_LOAD_WAIT": 60, + "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10, + "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10, + } + + mock_playwright_instance = MagicMock() + mock_browser = MagicMock() + mock_context = MagicMock() + mock_page = MagicMock() + mock_element = MagicMock() + + mock_sync_playwright.return_value.__enter__.return_value = ( + mock_playwright_instance + ) + mock_playwright_instance.chromium.launch.return_value = mock_browser + mock_browser.new_context.return_value = mock_context + mock_context.new_page.return_value = mock_page + mock_page.locator.return_value = mock_element + + timeout = PlaywrightTimeout() + mock_page.wait_for_function.side_effect = timeout + + with patch.object(WebDriverPlaywright, "auth", return_value=mock_context): + driver = WebDriverPlaywright("chrome") + with pytest.raises(PlaywrightTimeout) as exc_info: + driver.get_screenshot("http://example.com", "test-element", mock_user) + + assert exc_info.value is timeout + mock_logger.warning.assert_any_call( + "Timed out waiting for charts to load at url %s", + "http://example.com", + ) + @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True) @patch("superset.utils.webdriver.sync_playwright") @patch("superset.utils.webdriver.logger") From a0b2b1811b38901b627f90826755b7c96c108e90 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Tue, 5 May 2026 16:26:51 +0000 Subject: [PATCH 2/2] 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