Skip to content

fix(reports): mount off-screen dashboard charts before standalone capture - #42901

Open
eschutho wants to merge 1 commit into
masterfrom
fix-report-blank-virtualized-holders
Open

fix(reports): mount off-screen dashboard charts before standalone capture#42901
eschutho wants to merge 1 commit into
masterfrom
fix-report-blank-virtualized-holders

Conversation

@eschutho

@eschutho eschutho commented Aug 7, 2026

Copy link
Copy Markdown
Member

SUMMARY

Scheduled reports (and dashboard thumbnails) could deliver a screenshot with
blank regions where charts should be. The blank charts are ones that were
virtualized (rendered off-screen and never mounted) at capture time.

WebDriverPlaywright.get_screenshot's standard, non-tiled path — the default,
since SCREENSHOT_TILED_ENABLED is off — waits for chart readiness via
_wait_for_charts_ready and then captures the dashboard with
page.screenshot(full_page=True).

Problem

The readiness gate (REPORT_CHART_HOLDERS_READY_JS /
UNREADY_CHART_HOLDERS_JS_BODY) only requires viewport-visible chart
holders to reach a terminal state; it deliberately skips holders whose bounding
rect is outside the window viewport so it won't deadlock on lazy-rendered
charts (readiness logic added in #42624). Those off-screen holders are
classified virtualized and counted as "ready".

But full_page=True captures the entire document height, including
below-the-fold content. DashboardVirtualization (an IntersectionObserver
keyed to the window viewport) only mounts a chart holder once it intersects the
viewport, so a holder that never scrolls into view is never mounted — and is
captured blank. Because the gate believes it is done, this happens with no
timeout and no error
(an affected report showed virtualized_holders=2 with
hundreds of seconds of budget still remaining).

Superset does try to disable virtualization for capture browsers via
isCurrentUserBot() (navigator.webdriver), but that heuristic is fragile:
any deployment where navigator.webdriver is falsy (custom browser args,
anti-automation flags) re-enables virtualization and reintroduces blank
reports. The capture path should not rely on it.

Fix

Before the readiness wait in the non-tiled standalone path, grow the browser
viewport to the full dashboard height (_mount_offscreen_chart_holders). Once
every holder intersects the (now tall) viewport, virtualization mounts it and
the existing readiness gate requires them all to reach a terminal state
(rendered, empty, or error) before capture. This is defense-in-depth,
independent of the navigator.webdriver heuristic, and does not reintroduce
the deadlock #42624 avoided: mounted charts either render or reach a terminal
error/empty state, and genuinely stuck charts still fail the report loudly via
the readiness timeout rather than shipping blank.

The tiled path (SCREENSHOT_TILED_ENABLED=True) was already immune — it scrolls
and captures tile-by-tile, mounting each tile's holders as it goes.

A memory guard (MAX_STANDALONE_CAPTURE_VIEWPORT_HEIGHT = 30000) caps the
expanded viewport for pathologically tall dashboards and logs a warning
recommending tiled screenshots; full_page already rasterizes the whole
document, so this cap is not a new capability limit.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Not applicable — server-side capture pipeline change; behavior is covered by
unit tests.

TESTING INSTRUCTIONS

Automated:

pytest tests/unit_tests/utils/webdriver_test.py \
       tests/unit_tests/utils/test_screenshot_utils.py

New/updated tests assert that a standalone dashboard taller than the viewport is
resized to its full height before the readiness wait, that the resize is
scoped to standalone dashboard captures (not chart captures), that it is skipped
when the dashboard already fits, that the height is capped with a warning, and
that mounting failures are swallowed so they never abort a capture.

Manual: schedule a report for a dashboard tall enough to have below-the-fold
charts, in an environment where navigator.webdriver is not detected as a bot,
and confirm the delivered image renders every chart rather than leaving blank
regions.

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

Builds on the report readiness logic introduced in #42624.

…ture

Scheduled reports could deliver a dashboard screenshot with blank regions
where off-screen ("virtualized") charts should be. The non-tiled standalone
capture path waits only for viewport-visible chart holders to reach a
terminal state (#42624), then takes a full_page screenshot that includes
below-the-fold content. DashboardVirtualization only mounts holders that
intersect the window viewport, so any holder that never scrolls into view is
declared ready yet captured blank -- with no timeout, since the gate believes
it is done.

Grow the viewport to the full dashboard height before the readiness wait so
every holder intersects the viewport and virtualization mounts it; the
existing readiness gate then requires them all to reach a terminal state
before capture.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dosubot dosubot Bot added the alert-reports Namespace | Anything related to the Alert & Reports feature label Aug 7, 2026
@bito-code-review

bito-code-review Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #171ff6

Actionable Suggestions - 0
Additional Suggestions - 1
  • superset/utils/webdriver.py - 1
    • Viewport not restored after expansion · Line 315-401
      `_mount_offscreen_chart_holders` expands the viewport to pull in off-screen chart holders, but `get_screenshot` never restores it afterward. On success the expanded viewport persists for the lifetime of the `page` object, and on exception the outer `except PlaywrightError` handler closes the `context` without restoring the viewport — affecting any reuse of this page (e.g., a second screenshot or any automation step that follows). The cap path (line 361) is handled correctly by `_wait_for_charts_ready` which treats tall dashboards as virtualization placeholders, so the fix is purely about viewport cleanup.
Review Details
  • Files reviewed - 3 · Commit Range: 4399427..4399427
    • superset/utils/screenshot_utils.py
    • 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

AI Code Review powered by Bito Logo

@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 17.39130% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.37%. Comparing base (eb8592d) to head (4399427).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
superset/utils/webdriver.py 13.63% 19 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #42901      +/-   ##
==========================================
- Coverage   66.38%   66.37%   -0.01%     
==========================================
  Files        2857     2857              
  Lines      161160   161183      +23     
  Branches    37074    37078       +4     
==========================================
+ Hits       106987   106988       +1     
- Misses      52151    52172      +21     
- Partials     2022     2023       +1     
Flag Coverage Δ
hive 38.24% <17.39%> (-0.01%) ⬇️
mysql 57.77% <17.39%> (-0.02%) ⬇️
postgres 57.82% <17.39%> (-0.02%) ⬇️
presto 40.19% <17.39%> (-0.01%) ⬇️
python 59.21% <17.39%> (-0.02%) ⬇️
sqlite 57.44% <17.39%> (-0.02%) ⬇️
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

alert-reports Namespace | Anything related to the Alert & Reports feature preset-io size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant