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

[eschutho]

Summary

WebDriverPlaywright.get_screenshot() waits for .chart-container elements to render before deciding whether to use tiled or standard screenshot capture for a dashboard. Two issues in that logic:

1. A timeout while waiting for chart containers to render was logged at ERROR (logger.exception), inconsistent with the WARNING level already used for the sibling spinner-wait timeouts a few lines below in the same method.
2. When the DOM query used to measure the dashboard's height returns 0 (the query found no matching element, or the layout hadn't settled yet), the code treated that the same as "the dashboard is small enough for a single tile" — which could veto tiled capture even when the chart count alone indicated a large dashboard. That could let a large, virtualized dashboard skip the scroll-driven tiling pass and ship with below-the-fold charts that never rendered.

Fix

• The chart-container timeout now logs at WARNING, matching the convention already used for the other Playwright locate-wait timeouts in this method (customer-side chart loading is usually just slow, not a Superset bug). The screenshot still fails — the exception is still re-raised — only the log level and message changed; the message now also reports how many chart containers rendered before the timeout, for easier debugging.
• An unmeasurable height (0) is now treated as "unknown" rather than "small". Whether tiling is attempted is decided by the chart-count signal alone when height can't be measured, instead of chart count being silently overridden. The corresponding log line is only emitted at WARNING when it's actually consequential (i.e. the dashboard looks large by chart count but tiling would otherwise have been skipped); it's DEBUG for the common, benign case of a small dashboard with an unmeasured height.

Not changed

• The tiled-capture path itself (per-tile readiness, wait budgeting, etc.) — this PR only touches the earlier decision of whether to tile and the log level of one existing timeout.
• Standard (non-tiled) capture behavior for dashboards that were already correctly identified as small.

Testing

Added to tests/unit_tests/utils/webdriver_test.py:

• A large dashboard (by chart count) whose height can't be measured still attempts tiled capture instead of being silently downgraded to standard capture.
• The chart-container timeout logs at WARNING with rendered/total progress and still raises (never logs via logger.exception).
• Updated the existing "unknown height falls back" test to assert DEBUG, not WARNING, for the small-dashboard case where that's the expected/benign path.

Full relevant test suite passing; ruff check, ruff format, and mypy clean on both changed files.

Additional information

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[bito-code-review]

Code Review Agent Run #6b7e95

Actionable Suggestions - 0

Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

• superset/utils/webdriver.py - 1

  • Format string argument mismatch · Line 406-414

Review Details

• Files reviewed - 2 · Commit Range: 0f752c9..0f752c9

  • superset/utils/webdriver.py
  • tests/unit_tests/utils/webdriver_test.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[codecov]

Codecov Report

❌ Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.44%. Comparing base (263d793) to head (a1cb930).
⚠️ Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/utils/webdriver.py	0.00%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #42153      +/-   ##
==========================================
- Coverage   65.44%   65.44%   -0.01%     
==========================================
  Files        2810     2810              
  Lines      159362   159371       +9     
  Branches    36372    36372              
==========================================
+ Hits       104292   104294       +2     
- Misses      53027    53034       +7     
  Partials     2043     2043              

Flag	Coverage Δ
hive	38.08% <0.00%> (-0.01%)	⬇️
mysql	57.82% <0.00%> (-0.01%)	⬇️
postgres	57.86% <0.00%> (-0.01%)	⬇️
presto	39.98% <0.00%> (-0.01%)	⬇️
python	59.25% <0.00%> (-0.01%)	⬇️
sqlite	57.49% <0.00%> (-0.01%)	⬇️
unit	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[rebenitez1802]

LGTM

[eschutho]

Rebased onto current master (clean cherry-pick, no conflicts) and ran an adversarial self-review against the newly merged readiness work before requesting human review. Verdict: no blockers. The scope checks requested for this PR, with evidence:

Production-safe selectors — confirmed. The diff introduces no data-test selectors; it only touches the pre-existing .chart-container locator, which is a literal className in Chart.tsx (the adjacent data-test="chart-container" attribute is what babel-plugin-jsx-remove-data-test-id strips — the class survives production builds).

Unknown-height/tiling behavior — still useful, and safer than it looks. By the time the tiling decision runs, element.wait_for() has already succeeded, so dashboard_height == 0 means a transient measurement anomaly on an element known to be visible — exactly the case where routing to take_tiled_screenshot (which re-waits and re-measures fresh) is right. Traced the worst cases end-to-end: late layout → tiling proceeds with correct late-measured dimensions; element never re-appears → tiled returns None → existing fallback (or, once #42273 lands, a loud failure — consistent with its intent); re-measured height also 0 → num_tiles = max(1, 0) = 1, the clip_height <= 0 guard skips the tile, combine_screenshot_tiles([]) returns b"", which the caller's falsy check handles — and that state requires visible-yet-zero-scrollHeight, practically contradictory.

Compatibility with #42624's tiling guard — preserved. For known heights, use_tiled = likely_large_dashboard and (height_unknown or dashboard_height > tile_height) still vetoes tiling when 0 < height <= tile_height, even at high chart counts. Chart count alone bypasses the short-dashboard veto only when height is unknown (0) — so #42624's "chart count alone does not force tiling when the dashboard is shorter than one tile" invariant holds; that PR will only see a textual rebase here, not a semantic conflict.

Two review nits applied in the amended commit:

• exc_info=True on the new chart-container timeout WARNING, for parity with the Selenium locate-wait warnings that all carry it.
• A comment noting locator.all() is a one-time snapshot, so the rendered/total progress counts describe the snapshot, not the final DOM.

One pre-existing inconsistency noted for a follow-up (not this PR): chart_count here counts .chart-container (mounted charts only), while the merged readiness machinery counts holders via CHART_HOLDER_SELECTOR — a strictly better large-dashboard signal since it sees unmounted/virtualized holders too.

tests/unit_tests/utils/ — 715 passed post-amend; ruff check/ruff format --check clean.

[codeant-ai-for-open-source]

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.

(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

👍 | 👎

[bito-code-review]

The flagged issue is correct. Locator.wait_for() only ensures the element is present in the DOM, not that the chart content inside it has finished rendering. Incrementing rendered_chart_count based on this check can lead to misleading logs if the chart is still loading (e.g., showing a spinner).

To resolve this, you should either rename the variable to reflect that it counts located containers, or implement a more robust readiness check (e.g., waiting for a specific class or state that indicates the chart has finished rendering). Given the current implementation, renaming the variable is the most straightforward fix.

Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well?

superset/utils/webdriver.py

slice_container_elems: list[Locator] = []
                located_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")
                    # 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()
                        located_chart_count += 1
                except PlaywrightTimeout:
                    # 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 located before the timeout)",
                        url,
                        located_chart_count,
                        len(slice_container_elems),
                        exc_info=True,
                    )

[bito-code-review]

Code Review Agent Run #a82407

Actionable Suggestions - 0

Review Details

• Files reviewed - 2 · Commit Range: a1cb930..a1cb930

  • superset/utils/webdriver.py
  • tests/unit_tests/utils/webdriver_test.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by