eschutho opened a new pull request, #42253:
URL: https://github.com/apache/superset/pull/42253
### Decisions made that were not in the instructions
- **Viewport scoping (investigated, not assumed):** the new non-tiled
readiness check is scoped to viewport-intersecting chart holders only, same as
the tiled path. Reasoning: `WebDriverPlaywright.get_screenshot()`'s non-tiled
branches never call `page.set_viewport_size()` before capturing (that call only
exists in the tiled branch, to resize to `tile_height`) — the browser viewport
stays at the configured window size (e.g. `DEFAULT_DASHBOARD_WINDOW_SIZE =
1600, 1200`) for the whole capture. `element.screenshot()` /
`page.screenshot(full_page=True)` can capture below-the-fold content without
ever scrolling the page or resizing the viewport, so
`DashboardVirtualization`'s `IntersectionObserver`-based placeholders below the
fold never mount anything real before the screenshot is taken. Requiring *all*
chart holders (not just viewport-visible ones) to reach a terminal state would
therefore deadlock on those by-design placeholders. This mirrors the tiled
path's exact reasoning.
- **Shared JS predicate, not reimplemented:** `screenshot_utils.py`'s
`_TILE_READY_CHECK_JS` / `_FIND_UNREADY_CHART_HOLDERS_JS` /
`_UNREADY_CHART_HOLDERS_JS_BODY` (added in #42119) already implement exactly
the terminal-state check this fix needs. They're renamed to drop the leading
underscore (`CHART_HOLDERS_READY_JS`, `FIND_UNREADY_CHART_HOLDERS_JS`,
`UNREADY_CHART_HOLDERS_JS_BODY`) and imported into `webdriver.py` rather than
reimplemented, so the two capture paths can't drift apart.
- **Deduplicated the two copies of the wait:** `get_screenshot()` had two
near-identical copies of the old spinner wait (one for `tiled_enabled=True` +
small dashboard, one for `tiled_enabled=False`). Both are replaced by a single
new `WebDriverPlaywright._wait_for_charts_ready()` static method to avoid
maintaining the fix twice.
### SUMMARY
#42119 fixed a "vacuous pass" bug in the *tiled* screenshot path: the
per-tile spinner check only looked for the absence of currently-mounted
`.loading` elements, so a chart holder that hadn't started rendering yet
(nothing mounted, no spinner) passed the check immediately and got captured
blank.
The identical defect was still present, unfixed, in the *non-tiled/standard*
capture path. `WebDriverPlaywright.get_screenshot()` had its own global spinner
wait using the same old predicate:
```python
page.wait_for_function(
"() => document.querySelectorAll('.loading').length === 0",
timeout=self._screenshot_load_wait * 1000,
)
```
If no chart has mounted a `.loading` element yet at the exact moment this
check first runs (a timing gap between page-navigation-complete and React/query
bootstrap), the predicate is satisfied immediately and silently — no timeout,
no warning, no error. The result is a structurally valid PNG that's just
visually blank.
This PR replaces that predicate with the same positive terminal-state check
#42119 introduced for the tiled path: every viewport-intersecting
`[data-test="dashboard-component-chart-holder"]` must show either a rendered
chart (`[data-test="slice-container"]`, no nested `.loading`) or an error/empty
state (`[role="alert"]`, `.ant-empty`, `.missing-chart-container`). On timeout,
it now logs a WARNING (not ERROR — a customer chart-loading issue, not a system
fault, matching #42119's convention) with per-chart diagnostics (chart id + why
it's unready: `nothing_mounted` / `waiting_on_database` / `spinner_mounted`)
and re-raises, instead of the previous generic "timed out waiting for charts to
load" message with no diagnostics.
`log_context` (already threaded through `get_screenshot` for the tiled call
site) is threaded into the new non-tiled wait too, for log correlation.
This is the suspected root cause of a customer-reported blank-screenshot
issue on dashboard PDF downloads: a structurally valid but visually blank
capture, with no corresponding error anywhere, consistent with a silent
capture-time vacuous pass rather than a loud failure.
### NOT changed
- The tiled path (`take_tiled_screenshot` in
`superset/utils/screenshot_utils.py`) — already fixed by #42119, untouched here
except for the constant rename (no behavior change).
- The tiling-routing decision (chart-count / height-threshold logic that
decides tiled vs. non-tiled) — that's #42153's territory; not touched.
### TESTING INSTRUCTIONS
- `tests/unit_tests/utils/webdriver_test.py`:
- `test_uses_wait_for_function_to_detect_spinners` /
`test_spinner_timeout_logs_warning_and_raises` updated to assert the new
predicate and diagnostics instead of the old absence-of-`.loading` string.
- New `TestWebDriverPlaywrightChartReadiness` class:
- `test_chart_holder_with_nothing_mounted_does_not_satisfy_wait` —
regression test for the vacuous-pass fix; asserts no screenshot is captured.
- `test_all_chart_holders_ready_passes` — happy path, screenshot is
returned.
- `test_readiness_check_scoped_to_viewport_visible_holders` — asserts
the JS predicate is viewport-scoped
(`getBoundingClientRect`/`window.innerHeight`) and that `set_viewport_size` is
never called on this path, proving the viewport-scoping finding above.
- `test_log_context_threaded_into_readiness_wait` — asserts
`log_context` appears in the timeout warning.
- `test_spinner_timeout_logs_warning_and_raises` (updated) — asserts
WARNING (not ERROR) level with per-chart diagnostics on timeout.
- `tests/unit_tests/utils/test_screenshot_utils.py` updated for the constant
rename only (no behavior change to the tiled path).
- Ran `ruff check` / `ruff format` and `mypy` on both modified source files;
no new mypy errors versus master (confirmed by diffing mypy output before/after
the change — same 3 pre-existing errors, only line numbers shifted).
- `pytest tests/unit_tests/utils/webdriver_test.py
tests/unit_tests/utils/test_screenshot_utils.py` — 64 passed.
### 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
--
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]
