fix(reports): commit permalink before Playwright navigates to tab URL

CreateDashboardPermalinkCommand only flushes the INSERT when invoked inside an outer @transaction() (the nesting guard skips the commit). The row is therefore not visible to Playwright's separate DB connection, causing dashboard_permalink to return a 404, the <body class="standalone"> element to never appear, and the screenshot to time out after 600s. Commit immediately after CreateDashboardPermalinkCommand.run() returns, matching the pattern already used by create_log() in the same class. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-06-16 13:09:20 +00:00 · 2026-06-15 23:50:30 +00:00
5 changed files with 24 additions and 312 deletions
--- a/superset/commands/report/execute.py
+++ b/superset/commands/report/execute.py
@@ -358,6 +358,11 @@ class BaseReportState:
            dashboard_id=str(self._report_schedule.dashboard.uuid),
            state=dashboard_state,
        ).run()
+        # Commit the permalink immediately so Playwright's separate DB connection
+        # can resolve the URL. CreateDashboardPermalinkCommand only flushes when
+        # called inside an outer @transaction(), leaving the row invisible to
+        # other connections until we explicitly commit here.
+        db.session.commit()  # pylint: disable=consider-using-transaction

        return get_url_path(
            "Superset.dashboard_permalink",
--- a/superset/utils/screenshots.py
+++ b/superset/utils/screenshots.py
@@ -312,8 +312,6 @@ class BaseScreenshot:
        if image:
            with event_logger.log_context(f"screenshot.cache.{self.thumbnail_type}"):
                cache_payload.update(image)
-        else:
-            cache_payload.error()

        logger.info("Caching thumbnail: %s", cache_key)
        self.cache.set(cache_key, cache_payload.to_dict())
--- a/superset/utils/webdriver.py
+++ b/superset/utils/webdriver.py
@@ -301,6 +301,15 @@ class WebDriverPlaywright(WebDriverProxy):
                selenium_animation_wait = app.config[
                    "SCREENSHOT_SELENIUM_ANIMATION_WAIT"
                ]
+                logger.debug(
+                    "Wait %i seconds for chart animation", selenium_animation_wait
+                )
+                page.wait_for_timeout(selenium_animation_wait * 1000)
+                logger.debug(
+                    "Taking a PNG screenshot of url %s as user %s",
+                    url,
+                    user.username if user else "None",
+                )
                if app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"]:
                    unexpected_errors = WebDriverPlaywright.find_unexpected_errors(page)
                    if unexpected_errors:
@@ -365,7 +374,7 @@ class WebDriverPlaywright(WebDriverProxy):
                            load_wait=self._screenshot_load_wait,
                            animation_wait=selenium_animation_wait,
                        )
-                        if not img:
+                        if img is None:
                            logger.warning(
                                (
                                    "Tiled screenshot failed, "
@@ -375,28 +384,14 @@ class WebDriverPlaywright(WebDriverProxy):
                            img = WebDriverPlaywright._get_screenshot(
                                page, element, element_name
                            )
-                        logger.debug(
-                            "Tiled screenshot result: %d bytes for url: %s",
-                            len(img) if img else 0,
-                            url,
-                        )
                    else:
-                        logger.debug(
-                            "Dashboard below tiling threshold "
-                            "(%s charts, %spx height); using standard screenshot "
-                            "for url: %s",
-                            chart_count,
-                            dashboard_height,
-                            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)",
+                                "Wait for loading element of charts to be gone"
+                                " at url: %s",
                                url,
-                                self._screenshot_load_wait,
                            )
                            page.wait_for_function(
                                "() => document.querySelectorAll("
@@ -411,40 +406,16 @@ class WebDriverPlaywright(WebDriverProxy):
                                self._screenshot_load_wait,
                            )
                            raise
-                        logger.debug("All spinners cleared for url: %s", url)
-                        if selenium_animation_wait > 0:
-                            logger.debug(
-                                "Wait %i seconds for chart animation",
-                                selenium_animation_wait,
-                            )
-                            page.wait_for_timeout(selenium_animation_wait * 1000)
-                        logger.debug(
-                            "Taking screenshot of url %s as user %s",
-                            url,
-                            user.username if user else "None",
-                        )
                        img = WebDriverPlaywright._get_screenshot(
                            page, element, element_name
                        )
-                        logger.debug(
-                            "Screenshot result: %d bytes for url: %s",
-                            len(img) if img else 0,
-                            url,
-                        )
                else:
-                    logger.debug(
-                        "Tiled screenshots disabled; using standard screenshot "
-                        "for url: %s",
-                        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)",
+                            "Wait for loading element of charts to be gone at url: %s",
                            url,
-                            self._screenshot_load_wait,
                        )
                        page.wait_for_function(
                            "() => document.querySelectorAll('.loading').length === 0",
@@ -458,26 +429,9 @@ class WebDriverPlaywright(WebDriverProxy):
                            self._screenshot_load_wait,
                        )
                        raise
-                    logger.debug("All spinners cleared for url: %s", url)
-                    if selenium_animation_wait > 0:
-                        logger.debug(
-                            "Wait %i seconds for chart animation",
-                            selenium_animation_wait,
-                        )
-                        page.wait_for_timeout(selenium_animation_wait * 1000)
-                    logger.debug(
-                        "Taking screenshot of url %s as user %s",
-                        url,
-                        user.username if user else "None",
-                    )
                    img = WebDriverPlaywright._get_screenshot(
                        page, element, element_name
                    )
-                    logger.debug(
-                        "Screenshot result: %d bytes for url: %s",
-                        len(img) if img else 0,
-                        url,
-                    )

            except PlaywrightTimeout:
                raise
--- a/tests/unit_tests/utils/test_screenshot_cache_fix.py
+++ b/tests/unit_tests/utils/test_screenshot_cache_fix.py
@@ -55,7 +55,7 @@ class MockCache:
        self._cache.clear()


-@pytest.fixture()
+@pytest.fixture
 def mock_user():
    """Fixture to create a mock user."""
    user = MagicMock()
@@ -63,7 +63,7 @@ def mock_user():
    return user


-@pytest.fixture()
+@pytest.fixture
 def screenshot_obj():
    """Fixture to create a BaseScreenshot object."""
    url = "http://example.com"
@@ -149,25 +149,6 @@ class TestCacheOnlyOnSuccess:
        assert cached_value["status"] == "Updated"
        assert cached_value["image"] is not None

-    def test_cache_error_status_when_screenshot_returns_empty_bytes(
-        self, mocker: MockerFixture, screenshot_obj, mock_user
-    ):
-        """Empty bytes from get_screenshot must set ERROR, not leave COMPUTING."""
-        mocker.patch(BASE_SCREENSHOT_PATH + ".get_from_cache_key", return_value=None)
-        mocker.patch(
-            BASE_SCREENSHOT_PATH + ".get_screenshot",
-            return_value=b"",
-        )
-        BaseScreenshot.cache = MockCache()
-
-        screenshot_obj.compute_and_cache(user=mock_user, force=True)
-
-        cache_key = screenshot_obj.get_cache_key()
-        cached_value = BaseScreenshot.cache.get(cache_key)
-        assert cached_value is not None
-        assert cached_value["status"] == "Error"
-        assert cached_value.get("image") is None
-
    def test_no_intermediate_cache_during_computing(
        self, mocker: MockerFixture, screenshot_obj, mock_user
    ):
@@ -182,9 +163,9 @@ class TestCacheOnlyOnSuccess:
            cache_key = screenshot_obj.get_cache_key()
            cached_value = BaseScreenshot.cache.get(cache_key)
            # Cache should be empty during screenshot generation
-            assert (
-                cached_value is None
-            ), "Cache should not be saved during COMPUTING state"
+            assert cached_value is None, (
+                "Cache should not be saved during COMPUTING state"
+            )
            return b"image_data"

        mocker.patch(
--- a/tests/unit_tests/utils/webdriver_test.py
+++ b/tests/unit_tests/utils/webdriver_test.py
@@ -29,7 +29,7 @@ from superset.utils.webdriver import (
 )


-@pytest.fixture()
+@pytest.fixture
 def mock_app():
    """Mock Flask app with webdriver configuration."""
    app = MagicMock()
@@ -898,229 +898,3 @@ class TestWebDriverPlaywrightErrorHandling:
            "dashboard",
            "http://example.com",
        )
-
-
-class TestWebDriverPlaywrightAnimationWaitOrder:
-    """Animation wait must run after the spinner wait, not before."""
-
-    _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": 2,
-        "SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
-        "SCREENSHOT_LOCATE_WAIT": 10,
-        "SCREENSHOT_LOAD_WAIT": 30,
-        "SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
-        "SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
-    }
-
-    def _make_pw_mocks(self, mock_sync_playwright):
-        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"
-        return mock_context, mock_page
-
-    @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
-    @patch("superset.utils.webdriver.sync_playwright")
-    @patch("superset.utils.webdriver.app")
-    def test_animation_wait_after_spinner_wait_tiled_disabled(
-        self, mock_app, mock_sync_playwright
-    ):
-        """Non-tiled path: animation wait runs after spinner wait_for_function."""
-        mock_user = MagicMock()
-        mock_user.username = "test_user"
-        mock_app.config = {**self._base_config, "SCREENSHOT_TILED_ENABLED": False}
-
-        mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
-
-        call_order: list[str] = []
-
-        def record_wait_for_function(*args, **kwargs):
-            call_order.append("spinner_wait")
-
-        def record_wait_for_timeout(ms):
-            if ms == 2 * 1000:
-                call_order.append("animation_wait")
-
-        mock_page.wait_for_function.side_effect = record_wait_for_function
-        mock_page.wait_for_timeout.side_effect = record_wait_for_timeout
-
-        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
-            WebDriverPlaywright("chrome").get_screenshot(
-                "http://example.com", "test-element", mock_user
-            )
-
-        assert "spinner_wait" in call_order
-        assert "animation_wait" in call_order
-        spinner_idx = call_order.index("spinner_wait")
-        anim_idx = call_order.index("animation_wait")
-        assert (
-            spinner_idx < anim_idx
-        ), "spinner wait must precede animation wait in non-tiled path"
-
-    @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
-    @patch("superset.utils.webdriver.sync_playwright")
-    @patch("superset.utils.webdriver.app")
-    def test_animation_wait_after_spinner_wait_tiled_enabled_small_dashboard(
-        self, mock_app, mock_sync_playwright
-    ):
-        """Non-tiled path (tiled on, small dashboard): animation after spinner."""
-        mock_user = MagicMock()
-        mock_user.username = "test_user"
-        mock_app.config = {
-            **self._base_config,
-            "SCREENSHOT_TILED_ENABLED": True,
-            "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
-            "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
-            "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
-        }
-
-        mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
-
-        # Small dashboard: 3 charts, 1000px height — below both thresholds
-        mock_page.evaluate.side_effect = [3, 1000]
-
-        call_order: list[str] = []
-
-        def record_wait_for_function(*args, **kwargs):
-            call_order.append("spinner_wait")
-
-        def record_wait_for_timeout(ms):
-            if ms == 2 * 1000:
-                call_order.append("animation_wait")
-
-        mock_page.wait_for_function.side_effect = record_wait_for_function
-        mock_page.wait_for_timeout.side_effect = record_wait_for_timeout
-
-        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
-            WebDriverPlaywright("chrome").get_screenshot(
-                "http://example.com", "test-element", mock_user
-            )
-
-        assert "spinner_wait" in call_order
-        assert "animation_wait" in call_order
-        assert call_order.index("spinner_wait") < call_order.index("animation_wait")
-
-    @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
-    @patch("superset.utils.webdriver.sync_playwright")
-    @patch("superset.utils.webdriver.take_tiled_screenshot")
-    @patch("superset.utils.webdriver.app")
-    def test_tiled_path_passes_animation_wait_per_tile_no_global_wait(
-        self, mock_app, mock_take_tiled, mock_sync_playwright
-    ):
-        """Tiled path delegates animation_wait to take_tiled_screenshot; no global."""
-        mock_user = MagicMock()
-        mock_user.username = "test_user"
-        mock_app.config = {
-            **self._base_config,
-            "SCREENSHOT_TILED_ENABLED": True,
-            "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
-            "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
-            "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
-        }
-
-        mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
-
-        # Large dashboard: 25 charts, 6000px height
-        mock_page.evaluate.side_effect = [25, 6000]
-        mock_take_tiled.return_value = b"tiled_screenshot"
-
-        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
-            result = WebDriverPlaywright("chrome").get_screenshot(
-                "http://example.com", "standalone", mock_user
-            )
-
-        assert result == b"tiled_screenshot"
-        mock_take_tiled.assert_called_once_with(
-            mock_page,
-            "standalone",
-            600,
-            load_wait=30,
-            animation_wait=2,
-        )
-        # The only wait_for_timeout call should be the 0ms headstart; no global
-        # animation wait should be issued (handled per-tile by take_tiled_screenshot)
-        animation_waits = [
-            call[0][0]
-            for call in mock_page.wait_for_timeout.call_args_list
-            if call[0][0] == 2 * 1000
-        ]
-        assert (
-            animation_waits == []
-        ), "No global 2s animation wait_for_timeout should fire on the tiled path"
-
-    @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
-    @patch("superset.utils.webdriver.sync_playwright")
-    @patch("superset.utils.webdriver.take_tiled_screenshot")
-    @patch("superset.utils.webdriver.app")
-    def test_tiled_fallback_triggered_on_empty_bytes(
-        self, mock_app, mock_take_tiled, mock_sync_playwright
-    ):
-        """Tiled fallback fires when take_tiled_screenshot returns b"" (not None)."""
-        mock_user = MagicMock()
-        mock_user.username = "test_user"
-        mock_app.config = {
-            **self._base_config,
-            "SCREENSHOT_TILED_ENABLED": True,
-            "SCREENSHOT_TILED_CHART_THRESHOLD": 20,
-            "SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
-            "SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
-        }
-
-        mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
-        mock_page.evaluate.side_effect = [25, 6000]
-        # Empty bytes — falsy but not None; was silently passed through before the fix
-        mock_take_tiled.return_value = b""
-
-        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
-            with patch.object(
-                WebDriverPlaywright, "_get_screenshot", return_value=b"fallback"
-            ) as mock_fallback:
-                result = WebDriverPlaywright("chrome").get_screenshot(
-                    "http://example.com", "standalone", mock_user
-                )
-
-        assert result == b"fallback"
-        mock_fallback.assert_called_once()
-
-    @patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
-    @patch("superset.utils.webdriver.sync_playwright")
-    @patch("superset.utils.webdriver.app")
-    def test_animation_wait_skipped_when_zero(self, mock_app, mock_sync_playwright):
-        """No extra wait_for_timeout call when SCREENSHOT_SELENIUM_ANIMATION_WAIT=0."""
-        mock_user = MagicMock()
-        mock_user.username = "test_user"
-        mock_app.config = {
-            **self._base_config,
-            "SCREENSHOT_SELENIUM_ANIMATION_WAIT": 0,
-            "SCREENSHOT_TILED_ENABLED": False,
-        }
-
-        mock_context, mock_page = self._make_pw_mocks(mock_sync_playwright)
-
-        with patch.object(WebDriverPlaywright, "auth", return_value=mock_context):
-            WebDriverPlaywright("chrome").get_screenshot(
-                "http://example.com", "test-element", mock_user
-            )
-
-        # Only headstart (0ms) should be called; no animation wait call
-        timeout_values = [
-            call[0][0] for call in mock_page.wait_for_timeout.call_args_list
-        ]
-        assert timeout_values == [
-            0
-        ], f"Expected only [0] (headstart), got {timeout_values}"