fix(reports): positive readiness check for non-tiled screenshots

The non-tiled/standard screenshot path in WebDriverPlaywright.get_screenshot()
had the same vacuous-pass defect that #42119 fixed for tiled screenshots: it
waited for document.querySelectorAll('.loading').length === 0, which is
satisfied immediately if no chart has mounted a spinner yet (e.g. in the gap
between page-navigation-complete and React/query bootstrap), silently
capturing a blank screenshot with no timeout or error.

Replace it with the same positive terminal-state predicate #42119 introduced
for the tiled path (CHART_HOLDERS_READY_JS / FIND_UNREADY_CHART_HOLDERS_JS in
screenshot_utils.py, now exported and reused verbatim instead of
reimplemented) via a new WebDriverPlaywright._wait_for_charts_ready() helper,
deduplicating the two copies of this wait that existed in get_screenshot().
On timeout it logs per-chart diagnostics (chart id + state) at WARNING and
re-raises instead of capturing.

Co-Authored-By: Claude <noreply@anthropic.com>

superset/utils/screenshot_utils.py

@@ -40,9 +40,11 @@ if TYPE_CHECKING:
     except ImportError:
         Page = None
 
-# Selectors used to build a positive per-tile readiness check. A chart holder
-# is only "ready" once it shows a terminal state (a rendered chart or an
-# error/empty state) -- the mere absence of a `.loading` element is not
+# Selectors used to build a positive readiness check, shared by both the
+# tiled (take_tiled_screenshot, below) and standard/non-tiled
+# (WebDriverPlaywright.get_screenshot in webdriver.py) capture paths. A chart
+# holder is only "ready" once it shows a terminal state (a rendered chart or
+# an error/empty state) -- the mere absence of a `.loading` element is not
 # sufficient, since a chart holder that intersects the viewport but hasn't
 # mounted anything yet (e.g. its IntersectionObserver callback hasn't fired)
 # would otherwise pass vacuously.
@@ -54,6 +56,16 @@ if TYPE_CHECKING:
 # superset-frontend/packages/superset-ui-core/src/components/EmptyState for
 # `.ant-empty` (e.g. "no results"/"add required control values" states).
 #
+# Scoped to viewport-intersecting holders only (not just "in the tile", but
+# actually intersecting `window.innerHeight` at evaluation time) in both call
+# sites, since neither one resizes the browser viewport to the full dashboard
+# height before capturing: the tiled path scrolls tile-by-tile and the
+# standard path relies on Playwright's ability to capture below-the-fold
+# content without ever scrolling it into view. In both cases, chart holders
+# outside the current viewport are DashboardVirtualization placeholders that
+# haven't mounted anything real yet by design, so requiring them to reach a
+# terminal state would deadlock the wait.
+#
 # For diagnostics, each unready holder is additionally classified by *why*
 # it isn't ready, distinguishing a slow query from the virtualization race:
 #   - "waiting_on_database": `.loading` present with no `.slice_container`
@@ -65,7 +77,7 @@ if TYPE_CHECKING:
 #     of the chart.
 #   - "nothing_mounted": neither `.loading` nor any ready marker present --
 #     the vacuous-pass race this check exists to close.
-_UNREADY_CHART_HOLDERS_JS_BODY = """
+UNREADY_CHART_HOLDERS_JS_BODY = """
     const holders = document.querySelectorAll(
         '[data-test="dashboard-component-chart-holder"]'
     );
@@ -104,14 +116,14 @@ _UNREADY_CHART_HOLDERS_JS_BODY = """
 
 # Predicate for page.wait_for_function: true once every viewport-visible chart
 # holder has reached a terminal state.
-_TILE_READY_CHECK_JS = (
-    f"() => {{ {_UNREADY_CHART_HOLDERS_JS_BODY} return unready.length === 0; }}"
+CHART_HOLDERS_READY_JS = (
+    f"() => {{ {UNREADY_CHART_HOLDERS_JS_BODY} return unready.length === 0; }}"
 )
 
 # Diagnostic query for page.evaluate: chart id + state of holders still not
 # ready, used to build the timeout log message.
-_FIND_UNREADY_CHART_HOLDERS_JS = (
-    f"() => {{ {_UNREADY_CHART_HOLDERS_JS_BODY} return unready; }}"
+FIND_UNREADY_CHART_HOLDERS_JS = (
+    f"() => {{ {UNREADY_CHART_HOLDERS_JS_BODY} return unready; }}"
 )
 
 
@@ -249,12 +261,12 @@ def take_tiled_screenshot(
             tile_wait_start = time.monotonic()
             try:
                 page.wait_for_function(
-                    _TILE_READY_CHECK_JS,
+                    CHART_HOLDERS_READY_JS,
                     timeout=load_wait * 1000,
                 )
             except PlaywrightTimeout:
                 elapsed = time.monotonic() - tile_wait_start
-                unready_chart_holders = page.evaluate(_FIND_UNREADY_CHART_HOLDERS_JS)
+                unready_chart_holders = page.evaluate(FIND_UNREADY_CHART_HOLDERS_JS)
                 # A chart failing to load in time is a customer chart-loading
                 # issue (slow query, error state, etc.), not a Superset system
                 # fault, so this stays at WARNING -- the report still fails

superset/utils/webdriver.py

@@ -41,7 +41,11 @@ from selenium.webdriver.support.ui import WebDriverWait
 
 from superset.extensions import machine_auth_provider_factory
 from superset.utils.retries import retry_call
-from superset.utils.screenshot_utils import take_tiled_screenshot
+from superset.utils.screenshot_utils import (
+    CHART_HOLDERS_READY_JS,
+    FIND_UNREADY_CHART_HOLDERS_JS,
+    take_tiled_screenshot,
+)
 
 WindowSize = tuple[int, int]
 logger = logging.getLogger(__name__)
@@ -275,6 +279,62 @@ class WebDriverPlaywright(WebDriverProxy):
         else:
             return element.screenshot()
 
+    @staticmethod
+    def _wait_for_charts_ready(
+        page: Page,
+        url: str,
+        load_wait: int,
+        log_context: str | None = None,
+    ) -> None:
+        """
+        Wait for every viewport-visible chart holder to reach a terminal state
+        (rendered, or errored/empty) before taking a standard (non-tiled)
+        screenshot.
+
+        Uses the same positive readiness predicate as the tiled screenshot
+        path (see `take_tiled_screenshot` in screenshot_utils.py, #42119)
+        instead of checking for the mere absence of a `.loading` element: a
+        chart holder that hasn't mounted anything yet (no spinner, no
+        rendered content -- e.g. in the gap between page-load completing and
+        React/query bootstrap) would otherwise satisfy an absence-of-spinner
+        check immediately, producing a silently blank screenshot with no
+        timeout, warning, or error anywhere.
+
+        Scoped to viewport-intersecting chart holders only, same as the tiled
+        path: this method's caller never resizes the browser viewport to the
+        full dashboard height before capturing, so DashboardVirtualization
+        placeholders below the fold haven't mounted anything real yet by
+        design and must not block this wait.
+        """
+        context_suffix = f" [{log_context}]" if log_context else ""
+        logger.debug(
+            "Waiting for all chart holders to reach a terminal state at "
+            "url: %s (SCREENSHOT_LOAD_WAIT=%ss)%s",
+            url,
+            load_wait,
+            context_suffix,
+        )
+        try:
+            page.wait_for_function(
+                CHART_HOLDERS_READY_JS,
+                timeout=load_wait * 1000,
+            )
+        except PlaywrightTimeout:
+            unready_chart_holders = page.evaluate(FIND_UNREADY_CHART_HOLDERS_JS)
+            logger.warning(
+                "Timed out waiting for %s chart container(s) to become ready "
+                "at url %s (SCREENSHOT_LOAD_WAIT=%ss)%s; unready chart "
+                "holders (chart id, state): %s. Aborting screenshot rather "
+                "than capturing a blank or partially-loaded dashboard.",
+                len(unready_chart_holders),
+                url,
+                load_wait,
+                context_suffix,
+                unready_chart_holders,
+            )
+            raise
+        logger.debug("All chart holders ready at url: %s%s", url, context_suffix)
+
     def get_screenshot(  # pylint: disable=too-many-locals, too-many-statements  # noqa: C901
         self,
         url: str,
@@ -443,28 +503,14 @@ class WebDriverPlaywright(WebDriverProxy):
                             url,
                         )
                         # Standard screenshot captures the full element including
-                        # below-the-fold content, so wait for all spinners globally.
-                        try:
-                            logger.debug(
-                                "Waiting for all spinners to clear at url: %s "
-                                "(SCREENSHOT_LOAD_WAIT=%ss)",
-                                url,
-                                self._screenshot_load_wait,
-                            )
-                            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
-                        logger.debug("All spinners cleared for url: %s", url)
+                        # below-the-fold content, so wait for all viewport-visible
+                        # chart holders to reach a terminal state.
+                        WebDriverPlaywright._wait_for_charts_ready(
+                            page,
+                            url,
+                            self._screenshot_load_wait,
+                            log_context=log_context,
+                        )
                         if selenium_animation_wait > 0:
                             logger.debug(
                                 "Wait %i seconds for chart animation",
@@ -491,27 +537,14 @@ class WebDriverPlaywright(WebDriverProxy):
                         url,
                     )
                     # Standard screenshot captures the full element including
-                    # below-the-fold content, so wait for all spinners globally.
-                    try:
-                        logger.debug(
-                            "Waiting for all spinners to clear at url: %s "
-                            "(SCREENSHOT_LOAD_WAIT=%ss)",
-                            url,
-                            self._screenshot_load_wait,
-                        )
-                        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
-                    logger.debug("All spinners cleared for url: %s", url)
+                    # below-the-fold content, so wait for all viewport-visible
+                    # chart holders to reach a terminal state.
+                    WebDriverPlaywright._wait_for_charts_ready(
+                        page,
+                        url,
+                        self._screenshot_load_wait,
+                        log_context=log_context,
+                    )
                     if selenium_animation_wait > 0:
                         logger.debug(
                             "Wait %i seconds for chart animation",

tests/unit_tests/utils/test_screenshot_utils.py

@@ -481,11 +481,11 @@ class TestTakeTiledScreenshot:
         lets a slow query be told apart from the race during an incident.
         """
         from superset.utils.screenshot_utils import (
-            _FIND_UNREADY_CHART_HOLDERS_JS,
-            _TILE_READY_CHECK_JS,
+            CHART_HOLDERS_READY_JS,
+            FIND_UNREADY_CHART_HOLDERS_JS,
         )
 
-        for js in (_TILE_READY_CHECK_JS, _FIND_UNREADY_CHART_HOLDERS_JS):
+        for js in (CHART_HOLDERS_READY_JS, FIND_UNREADY_CHART_HOLDERS_JS):
             assert "spinner_mounted" in js
             assert "waiting_on_database" in js
             assert "nothing_mounted" in js

tests/unit_tests/utils/webdriver_test.py

@@ -668,7 +668,7 @@ class TestWebDriverPlaywrightErrorHandling:
     def test_uses_wait_for_function_to_detect_spinners(
         self, mock_app, mock_browser_manager
     ):
-        """wait_for_function polls for spinner absence rather than snapshotting."""
+        """wait_for_function polls for chart-holder readiness, not snapshotting."""
         mock_user = MagicMock()
         mock_user.username = "test_user"
         mock_app.config = {
@@ -701,10 +701,16 @@ class TestWebDriverPlaywrightErrorHandling:
             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,
-        )
+        mock_page.wait_for_function.assert_called_once()
+        call_args, call_kwargs = mock_page.wait_for_function.call_args
+        js = call_args[0]
+        # The old absence-of-`.loading` predicate passed vacuously when a
+        # chart holder hadn't mounted anything yet; the fix requires a
+        # positive terminal-state check instead (same predicate as the tiled
+        # path, #42119).
+        assert "dashboard-component-chart-holder" in js
+        assert "slice-container" in js
+        assert call_kwargs["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",)
@@ -718,7 +724,8 @@ class TestWebDriverPlaywrightErrorHandling:
     def test_spinner_timeout_logs_warning_and_raises(
         self, mock_app, mock_logger, mock_browser_manager
     ):
-        """Spinner timeout is logged as a warning and re-raised."""
+        """Readiness timeout is logged as a warning, with per-chart diagnostics,
+        and re-raised rather than silently capturing."""
         from superset.utils.webdriver import PlaywrightTimeout
 
         mock_user = MagicMock()
@@ -750,6 +757,9 @@ class TestWebDriverPlaywrightErrorHandling:
 
         timeout = PlaywrightTimeout()
         mock_page.wait_for_function.side_effect = timeout
+        mock_page.evaluate.return_value = [
+            {"chartId": "42", "state": "nothing_mounted"}
+        ]
 
         with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
             driver = WebDriverPlaywright("chrome")
@@ -757,11 +767,12 @@ class TestWebDriverPlaywrightErrorHandling:
                 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 (SCREENSHOT_LOAD_WAIT=%ss)",
-            "http://example.com",
-            60,
-        )
+        mock_logger.error.assert_not_called()
+        warning_call = mock_logger.warning.call_args
+        assert "Timed out waiting for" in warning_call[0][0]
+        assert "http://example.com" in warning_call[0]
+        assert 60 in warning_call[0]
+        assert [{"chartId": "42", "state": "nothing_mounted"}] in warning_call[0]
 
     @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
     @patch("superset.utils.webdriver._browser_manager")
@@ -974,6 +985,179 @@ class TestWebDriverPlaywrightErrorHandling:
         )
 
 
+class TestWebDriverPlaywrightChartReadiness:
+    """Regression tests for the non-tiled vacuous-pass fix.
+
+    The readiness predicate itself (`CHART_HOLDERS_READY_JS` /
+    `FIND_UNREADY_CHART_HOLDERS_JS` in screenshot_utils.py) is exercised
+    directly in test_screenshot_utils.py; these tests confirm the standard
+    (non-tiled) `get_screenshot` path wires up to that *same* shared
+    predicate instead of the old absence-of-`.loading` check, which passed
+    vacuously for a chart holder that hadn't mounted anything yet.
+    """
+
+    _base_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": 5,
+        "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
+        "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
+    }
+
+    def _make_pw_mocks(self, mock_browser_manager):
+        mock_browser = MagicMock()
+        mock_context = MagicMock()
+        mock_page = MagicMock()
+        mock_element = MagicMock()
+
+        mock_browser_manager.get_browser.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"
+        return mock_context, mock_page
+
+    @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+    @patch("superset.utils.webdriver._browser_manager")
+    @patch("superset.utils.webdriver.app")
+    def test_chart_holder_with_nothing_mounted_does_not_satisfy_wait(
+        self, mock_app, mock_browser_manager
+    ):
+        """A chart holder present in the DOM but with nothing mounted yet (no
+        spinner, no rendered content -- e.g. the gap between page-load
+        completing and React/query bootstrap) must not satisfy the readiness
+        wait. The old `.loading`-absence check passed immediately in this
+        case, producing a silently blank screenshot.
+        """
+        from superset.utils.webdriver import PlaywrightTimeout
+
+        mock_user = MagicMock()
+        mock_user.username = "test_user"
+        mock_app.config = {**self._base_config}
+
+        mock_context, mock_page = self._make_pw_mocks(mock_browser_manager)
+
+        def fake_wait_for_function(js, timeout=None):
+            # Confirm the predicate sent is the shared terminal-state check
+            # (not the old absence-of-`.loading` check), then simulate it
+            # correctly reporting "not ready" for a chart holder that hasn't
+            # mounted anything.
+            assert "dashboard-component-chart-holder" in js
+            raise PlaywrightTimeout("Timeout waiting for chart holders")
+
+        mock_page.wait_for_function.side_effect = fake_wait_for_function
+        mock_page.evaluate.return_value = [{"chartId": "7", "state": "nothing_mounted"}]
+
+        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+            driver = WebDriverPlaywright("chrome")
+            with pytest.raises(PlaywrightTimeout):
+                driver.get_screenshot("http://example.com", "test-element", mock_user)
+
+        # No screenshot should be captured -- fail loudly instead of
+        # silently returning a blank image.
+        mock_page.screenshot.assert_not_called()
+        mock_page.locator.return_value.screenshot.assert_not_called()
+
+    @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+    @patch("superset.utils.webdriver._browser_manager")
+    @patch("superset.utils.webdriver.app")
+    def test_all_chart_holders_ready_passes(self, mock_app, mock_browser_manager):
+        """All chart holders rendered or errored -> wait passes, screenshot taken."""
+        mock_user = MagicMock()
+        mock_user.username = "test_user"
+        mock_app.config = {**self._base_config}
+
+        mock_context, mock_page = self._make_pw_mocks(mock_browser_manager)
+        # mock_page.wait_for_function is a no-op MagicMock by default, i.e.
+        # the readiness predicate is satisfied immediately.
+
+        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+            driver = WebDriverPlaywright("chrome")
+            result = driver.get_screenshot(
+                "http://example.com", "test-element", mock_user
+            )
+
+        assert result == b"screenshot"
+        # No timeout, so the unready-holder diagnostics query never runs.
+        mock_page.evaluate.assert_not_called()
+
+    @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+    @patch("superset.utils.webdriver._browser_manager")
+    @patch("superset.utils.webdriver.app")
+    def test_readiness_check_scoped_to_viewport_visible_holders(
+        self, mock_app, mock_browser_manager
+    ):
+        """The non-tiled readiness check only requires viewport-intersecting
+        chart holders to be ready, mirroring the tiled path's reasoning
+        (#42119). `get_screenshot`'s standard (non-tiled) branch never
+        resizes the browser viewport to the full dashboard height before
+        capturing -- only the tiled branch's `set_viewport_size` call does
+        that -- so a below-the-fold chart holder is a
+        DashboardVirtualization placeholder that hasn't mounted anything
+        real yet by design and must not block this wait.
+        """
+        mock_user = MagicMock()
+        mock_user.username = "test_user"
+        mock_app.config = {**self._base_config}
+
+        mock_context, mock_page = self._make_pw_mocks(mock_browser_manager)
+
+        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+            driver = WebDriverPlaywright("chrome")
+            driver.get_screenshot("http://example.com", "test-element", mock_user)
+
+        js = mock_page.wait_for_function.call_args[0][0]
+        # The predicate skips any chart holder whose bounding rect doesn't
+        # intersect the current viewport -- an off-screen/below-fold holder
+        # is excluded from the readiness requirement rather than blocking it.
+        assert "getBoundingClientRect" in js
+        assert "window.innerHeight" in js
+        # set_viewport_size is only ever called on the tiled branch (to
+        # resize to tile_height); confirming it's untouched here is what
+        # makes the viewport-scoped predicate necessary for this branch.
+        mock_page.set_viewport_size.assert_not_called()
+
+    @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
+    @patch("superset.utils.webdriver._browser_manager")
+    @patch("superset.utils.webdriver.logger")
+    @patch("superset.utils.webdriver.app")
+    def test_log_context_threaded_into_readiness_wait(
+        self, mock_app, mock_logger, mock_browser_manager
+    ):
+        """log_context (e.g. report execution id) is threaded through the
+        non-tiled readiness wait for correlation, matching #42119's
+        convention for the tiled path."""
+        from superset.utils.webdriver import PlaywrightTimeout
+
+        mock_user = MagicMock()
+        mock_user.username = "test_user"
+        mock_app.config = {**self._base_config}
+
+        mock_context, mock_page = self._make_pw_mocks(mock_browser_manager)
+        mock_page.wait_for_function.side_effect = PlaywrightTimeout("timed out")
+        mock_page.evaluate.return_value = [{"chartId": "7", "state": "nothing_mounted"}]
+
+        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
+            driver = WebDriverPlaywright("chrome")
+            with pytest.raises(PlaywrightTimeout):
+                driver.get_screenshot(
+                    "http://example.com",
+                    "test-element",
+                    mock_user,
+                    log_context="execution_id=abc-123",
+                )
+
+        warning_call_args = mock_logger.warning.call_args[0]
+        assert " [execution_id=abc-123]" in warning_call_args
+
+
 class TestWebDriverPlaywrightAnimationWaitOrder:
     """Animation wait must run after the spinner wait, not before."""