You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by el...@apache.org on 2026/05/15 20:35:14 UTC
(superset) branch master updated: fix(reports): narrow spinner checks to viewport and tighten exception handling (#39895)
This is an automated email from the ASF dual-hosted git repository.
eschutho pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 8d2b655c22d fix(reports): narrow spinner checks to viewport and tighten exception handling (#39895)
8d2b655c22d is described below
commit 8d2b655c22d8e678b25f8be1cea8602523bd6f90
Author: Elizabeth Thompson <es...@gmail.com>
AuthorDate: Fri May 15 13:35:07 2026 -0700
fix(reports): narrow spinner checks to viewport and tighten exception handling (#39895)
Co-authored-by: Claude Sonnet 4.6 <no...@anthropic.com>
---
superset/utils/screenshot_utils.py | 36 +++++++++++++-
superset/utils/webdriver.py | 62 ++++++++++++++++++-------
tests/unit_tests/utils/test_screenshot_utils.py | 51 ++++++++++++++++++++
tests/unit_tests/utils/webdriver_test.py | 3 +-
4 files changed, 134 insertions(+), 18 deletions(-)
diff --git a/superset/utils/screenshot_utils.py b/superset/utils/screenshot_utils.py
index dfd2e49f54f..3cc33fb10fe 100644
--- a/superset/utils/screenshot_utils.py
+++ b/superset/utils/screenshot_utils.py
@@ -28,6 +28,11 @@ logger = logging.getLogger(__name__)
# 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 60)
Returns:
Combined screenshot bytes or None if failed
@@ -139,6 +148,31 @@ 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 "
+ "(load_wait=%ss)",
+ i + 1,
+ num_tiles,
+ load_wait,
+ )
# 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 3d61411b737..a7f79e3b415 100644
--- a/superset/utils/webdriver.py
+++ b/superset/utils/webdriver.py
@@ -295,21 +295,6 @@ class WebDriverPlaywright(WebDriverProxy):
url,
)
raise
- try:
- # charts took too long to load
- logger.debug(
- "Wait for loading element of charts to be gone at url: %s", url
- )
- 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
- )
- raise
-
selenium_animation_wait = app.config[
"SCREENSHOT_SELENIUM_ANIMATION_WAIT"
]
@@ -368,7 +353,12 @@ class WebDriverPlaywright(WebDriverProxy):
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(
(
@@ -380,10 +370,50 @@ class WebDriverPlaywright(WebDriverProxy):
page, element, element_name
)
else:
+ # Standard screenshot captures the full element including
+ # below-the-fold content, so wait for all spinners globally.
+ try:
+ logger.debug(
+ "Wait for loading element of charts to be gone"
+ " at url: %s",
+ url,
+ )
+ 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
img = WebDriverPlaywright._get_screenshot(
page, element, element_name
)
else:
+ # Standard screenshot captures the full element including
+ # below-the-fold content, so wait for all spinners globally.
+ try:
+ logger.debug(
+ "Wait for loading element of charts to be gone at url: %s",
+ url,
+ )
+ 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
img = WebDriverPlaywright._get_screenshot(
page, element, element_name
)
diff --git a/tests/unit_tests/utils/test_screenshot_utils.py b/tests/unit_tests/utils/test_screenshot_utils.py
index f1b970bf83c..c6127374f04 100644
--- a/tests/unit_tests/utils/test_screenshot_utils.py
+++ b/tests/unit_tests/utils/test_screenshot_utils.py
@@ -320,3 +320,54 @@ class TestTakeTiledScreenshot:
# 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 "
+ "(load_wait=%ss)",
+ 1,
+ 3,
+ 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 cf7447eef1d..b2e26269474 100644
--- a/tests/unit_tests/utils/webdriver_test.py
+++ b/tests/unit_tests/utils/webdriver_test.py
@@ -744,8 +744,9 @@ class TestWebDriverPlaywrightErrorHandling:
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)