fix(reports): narrow spinner checks to viewport and tighten exception handling

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

superset/utils/webdriver.py

@@ -300,8 +300,19 @@ 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(
+                        """() => {
+                            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", url
@@ -366,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(
                                 (

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

tests/unit_tests/utils/webdriver_test.py

@@ -640,6 +640,123 @@ 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(
+            """() => {
+                            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
+        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")