eschutho opened a new pull request, #42901:
URL: https://github.com/apache/superset/pull/42901
### 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:
```bash
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](https://github.com/apache/superset/issues/13351))
- [ ] 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
