Skip to content

fix(reports): downgrade chart-container timeout log level and fix tiling veto on unknown height #42153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
eschutho merged 1 commit into from
Jul 31, 2026
+219 −12
Merged

fix(reports): downgrade chart-container timeout log level and fix tiling veto on unknown height #42153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 44 additions & 10 deletions superset/utils/webdriver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,16 +482,31 @@ def get_screenshot( # pylint: disable=too-many-locals, too-many-statements # n
logger.exception("Timed out requesting url %s", url)
raise

slice_container_elems: list[Locator] = []
rendered_chart_count = 0
try:
# chart containers didn't render
logger.debug("Wait for chart containers to draw at url: %s", url)
slice_container_locator = page.locator(".chart-container")
for slice_container_elem in slice_container_locator.all():
# One-time snapshot: containers mounting after this point
# are neither waited on nor counted, so the progress
# numbers below describe the snapshot, not the final DOM.
slice_container_elems = slice_container_locator.all()
for slice_container_elem in slice_container_elems:
slice_container_elem.wait_for()
rendered_chart_count += 1
Comment on lines 496 to +497

@codeant-ai-for-open-source codeant-ai-for-open-source Bot Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Locator.wait_for() only waits for the locator's default presence/visibility condition; it does not establish that a chart has rendered or reached a terminal state. The counter is therefore incremented for containers that may still contain a spinner or no chart content, and the timeout log can falsely report them as rendered. Use the actual chart readiness predicate for the progress count, or describe the count as containers located rather than rendered. [comment mismatch]

Severity Level: Minor 🧹 
- ⚠️ Timeout logs can overstate rendered chart progress.
- ⚠️ Report diagnostics may mislead operators during slow loads.
- ✅ Standard and tiled readiness gates remain unaffected.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset/utils/webdriver.py
**Line:** 496:497
**Comment:**
	*Comment Mismatch: `Locator.wait_for()` only waits for the locator's default presence/visibility condition; it does not establish that a chart has rendered or reached a terminal state. The counter is therefore incremented for containers that may still contain a spinner or no chart content, and the timeout log can falsely report them as rendered. Use the actual chart readiness predicate for the progress count, or describe the count as containers located rather than rendered.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

except PlaywrightTimeout:
logger.exception(
"Timed out waiting for chart containers to draw at url %s",
# Customer-side chart loading is often just slow, not a
# Superset bug, so this is a WARNING (matching the other
# locate-wait timeouts below) rather than an ERROR -- but
# it still fails the screenshot; see the `raise` below.
logger.warning(
"Timed out waiting for chart containers to draw at url %s "
"(%s of %s chart containers rendered before the timeout)",
url,
rendered_chart_count,
len(slice_container_elems),
exc_info=True,
)
raise
selenium_animation_wait = app.config[
Expand Down Expand Up @@ -529,19 +544,38 @@ def get_screenshot( # pylint: disable=too-many-locals, too-many-statements # n
"SCREENSHOT_TILED_VIEWPORT_HEIGHT", viewport_height
)

if dashboard_height == 0:
logger.warning(
# A height of 0 means the DOM query above found no matching
# element (or it hadn't laid out yet), not that the
# dashboard is actually empty. Treat it as "unknown" rather
# than "fits in a single tile": chart_count alone already
# tells us whether this looks like a large dashboard, and
# that signal must not be silently vetoed just because we
# couldn't measure height, or a large dashboard could skip
# tiling and ship with unrendered below-the-fold charts.
height_unknown = dashboard_height == 0
likely_large_dashboard = (
chart_count >= chart_threshold
or dashboard_height > height_threshold
)
if height_unknown:
log_fn = (
logger.warning if likely_large_dashboard else logger.debug
)
log_fn(
"Could not determine dashboard height for element %s "
"at url %s; falling back to standard screenshot behavior",
"at url %s (%s chart containers found); %s",
element_name,
url,
chart_count,
"attempting tiled screenshot anyway"
if likely_large_dashboard
else "falling back to standard screenshot behavior",
)

# Use tiled screenshots for large dashboards
use_tiled = (
chart_count >= chart_threshold
or dashboard_height > height_threshold
) and dashboard_height > tile_height
use_tiled = likely_large_dashboard and (
height_unknown or dashboard_height > tile_height
)

if use_tiled:
logger.info(
Expand Down
177 changes: 175 additions & 2 deletions tests/unit_tests/utils/webdriver_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -919,12 +919,185 @@ def evaluate_side_effect(script):
)

assert result == b"fake_screenshot"
mock_logger.warning.assert_any_call(
"Could not determine dashboard height for element %s at url %s; "
# chart_count (1) is well below the tiling threshold (20), so this is
# the benign/expected case and must not be logged as a WARNING.
mock_logger.debug.assert_any_call(
"Could not determine dashboard height for element %s "
"at url %s (%s chart containers found); %s",
"dashboard",
"http://example.com",
1,
"falling back to standard screenshot behavior",
)
assert not any(
call.args and "Could not determine dashboard height" in call.args[0]
for call in mock_logger.warning.call_args_list
)

@patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
@patch("superset.utils.webdriver._browser_manager")
@patch("superset.utils.webdriver.logger")
@patch("superset.utils.webdriver.take_tiled_screenshot")
def test_unknown_height_does_not_veto_tiling_for_large_dashboard(
self, mock_take_tiled, mock_logger, mock_browser_manager
):
"""
A large dashboard (by chart_count) whose height can't be measured
must still attempt tiling instead of being silently downgraded to a
standard screenshot, since below-the-fold charts may not have
rendered without the scroll-driven tiling pass.
"""
mock_user = MagicMock()
mock_user.username = "test_user"

mock_browser = MagicMock()
mock_context = MagicMock()
mock_page = MagicMock()
mock_element = MagicMock()
mock_chart_container = 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

def locator_side_effect(selector):
if selector == ".chart-container":
locator = MagicMock()
locator.all.return_value = [mock_chart_container]
return locator
return mock_element

mock_page.locator.side_effect = locator_side_effect
mock_element.wait_for.return_value = None
mock_chart_container.wait_for.return_value = None
mock_page.wait_for_timeout.return_value = None
mock_take_tiled.return_value = b"tiled_screenshot"

def evaluate_side_effect(script):
if script == 'document.querySelectorAll(".chart-container").length':
return 25 # chart_count >= threshold
if "const target = document.querySelector" in script:
return 0 # height could not be determined
return None

mock_page.evaluate.side_effect = evaluate_side_effect

with patch("superset.utils.webdriver.app") as mock_app:
mock_app.config = {
"WEBDRIVER_OPTION_ARGS": [],
"WEBDRIVER_WINDOW": {"pixel_density": 1},
"SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
"SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
"SCREENSHOT_SELENIUM_HEADSTART": 5,
"SCREENSHOT_SELENIUM_ANIMATION_WAIT": 1,
"SCREENSHOT_LOCATE_WAIT": 10,
"SCREENSHOT_LOAD_WAIT": 10,
"SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
"SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
"SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
"SCREENSHOT_TILED_ENABLED": True,
"SCREENSHOT_TILED_CHART_THRESHOLD": 20,
"SCREENSHOT_TILED_HEIGHT_THRESHOLD": 5000,
"SCREENSHOT_TILED_VIEWPORT_HEIGHT": 600,
}

with patch.object(WebDriverPlaywright, "auth") as mock_auth:
mock_auth.return_value = mock_context

driver = WebDriverPlaywright("chrome")
result = driver.get_screenshot(
"http://example.com", "dashboard", mock_user
)

assert result == b"tiled_screenshot"
mock_take_tiled.assert_called_once()
mock_logger.warning.assert_any_call(
"Could not determine dashboard height for element %s "
"at url %s (%s chart containers found); %s",
"dashboard",
"http://example.com",
25,
"attempting tiled screenshot anyway",
)

@patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
@patch("superset.utils.webdriver._browser_manager")
@patch("superset.utils.webdriver.logger")
def test_chart_container_timeout_logs_warning_with_progress_and_raises(
self, mock_logger, mock_browser_manager
):
"""
Timing out while waiting for `.chart-container` elements to draw must
be logged as a WARNING (matching the other locate-wait timeouts in
this method, and the customer-side-slowness convention established
for these Playwright timeouts) with rendered/total progress, and must
still fail the screenshot by re-raising.
"""
from superset.utils.webdriver import PlaywrightTimeout

mock_user = MagicMock()
mock_user.username = "test_user"

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

timeout = PlaywrightTimeout()
rendered_ok = MagicMock()
rendered_ok.wait_for.return_value = None
never_renders = MagicMock()
never_renders.wait_for.side_effect = timeout

def locator_side_effect(selector):
if selector == ".chart-container":
locator = MagicMock()
locator.all.return_value = [rendered_ok, never_renders]
return locator
return mock_element

mock_page.locator.side_effect = locator_side_effect
mock_element.wait_for.return_value = None

with patch("superset.utils.webdriver.app") as mock_app:
mock_app.config = {
"WEBDRIVER_OPTION_ARGS": [],
"WEBDRIVER_WINDOW": {"pixel_density": 1},
"SCREENSHOT_PLAYWRIGHT_DEFAULT_TIMEOUT": 30000,
"SCREENSHOT_PLAYWRIGHT_WAIT_EVENT": "networkidle",
"SCREENSHOT_SELENIUM_HEADSTART": 5,
"SCREENSHOT_SELENIUM_ANIMATION_WAIT": 1,
"SCREENSHOT_LOCATE_WAIT": 10,
"SCREENSHOT_LOAD_WAIT": 10,
"SCREENSHOT_WAIT_FOR_ERROR_MODAL_VISIBLE": 10,
"SCREENSHOT_WAIT_FOR_ERROR_MODAL_INVISIBLE": 10,
"SCREENSHOT_REPLACE_UNEXPECTED_ERRORS": False,
"SCREENSHOT_TILED_ENABLED": False,
}

with patch.object(WebDriverPlaywright, "auth") as mock_auth:
mock_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 chart containers to draw at url %s "
"(%s of %s chart containers rendered before the timeout)",
"http://example.com",
1,
2,
exc_info=True,
)
mock_logger.exception.assert_not_called()

@patch("superset.utils.webdriver.PLAYWRIGHT_AVAILABLE", True)
@patch("superset.utils.webdriver._browser_manager")
Expand Down
Loading