eschutho opened a new pull request, #42119:
URL: https://github.com/apache/superset/pull/42119
### Decisions made that were not in the instructions
- Selector set for "terminal state": in addition to a rendered chart
(`.slice_container` without a nested `.loading`) and a hard error
(`[role="alert"]`, from antd's `Alert` underlying
`ErrorAlert`/`ChartErrorMessage`), I also treat antd's `.ant-empty` (used by
`EmptyState`, e.g. "add required control values", "chart ready to go", "no
results") and `.missing-chart-container` (deleted-chart placeholder) as
terminal. These are legitimate non-loading states that Chart.tsx can render
instead of a chart or a `[role="alert"]` error; without including them,
dashboards containing such charts would time out on every screenshot. This
wasn't called out explicitly in the task but follows directly from "chart
rendered OR chart error state" needing to cover all of Chart.tsx's non-loading
render branches.
- For the diagnostic "identity of unready containers" in the timeout log, I
use the `data-test-chart-id` attribute (present on `.chart-slice` in
`superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx:701-704`)
rather than DOM position/index, since it's a stable, human-meaningful
identifier already used elsewhere (e.g. Cypress's `waitForChartLoad`).
- Item C (non-tiled global spinner wait in `webdriver.py`): evaluated and
left unchanged, see "Not changed" below.
### SUMMARY
**Background**: `take_tiled_screenshot()` (added in #39895,
`superset/utils/screenshot_utils.py`) scrolls a large dashboard tile-by-tile
and, before capturing each tile, waits for
`document.querySelectorAll('.loading')` to have no viewport-visible element.
Two defects in this predicate/timeout handling shipped blank or spinner
screenshots to several customers the week of 2026-07-13:
1. **Vacuous-pass race**: the predicate only inspects *existing* `.loading`
elements. With `DashboardVirtualization` on (default), a chart holder that has
just scrolled into view but whose `IntersectionObserver` callback
(`superset-frontend/src/dashboard/components/gridComponents/Row/Row.tsx:170-216`)
hasn't fired yet has mounted neither a spinner nor a chart. The predicate
finds zero `.loading` elements and passes immediately, so the tile is captured
before the chart even starts loading — a blank chart in the delivered report.
2. **Deliver-degraded on timeout**: on `PlaywrightTimeout`, the per-tile
wait logged a warning and fell through to capture the tile anyway, so a spinner
image could be delivered to report recipients instead of the report failing.
**Fix**:
- **(A) Positive readiness check.** Replaced the absence-of-`.loading`
predicate with a check over `[data-test="dashboard-component-chart-holder"]`
elements
(`superset-frontend/src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx:276`).
For every such holder intersecting the viewport, it must show a terminal
state: a rendered chart (`.slice_container` present, from
`superset-frontend/src/components/Chart/Chart.tsx:357`, with no nested
`.loading` spinner, from the shared `Loading` component at
`superset-frontend/packages/superset-ui-core/src/components/Loading/index.tsx:78`),
or an error/empty state (`[role="alert"]`, `.ant-empty`, or
`.missing-chart-container`). A holder with nothing mounted yet (no spinner, no
chart) does not satisfy this and correctly blocks the wait — closing the
vacuous-pass race. A tile with no chart holders (e.g. markdown-only) trivially
passes, since the loop finds nothing to check.
- **(B) Fail loudly on timeout.** Per direct product decision from @eschutho
("no silent errors, never snapshot spinners or blank charts"): a per-tile
readiness timeout now logs an ERROR (tile index, load_wait, and the
`data-test-chart-id`s of still-unready holders) and re-raises
`PlaywrightTimeout` instead of warning-and-continuing. This changes the
previously-shipped semantics from "warn and capture a degraded tile anyway" to
"abort the tiled screenshot." I traced the propagation path on current master
and no changes elsewhere were needed: `take_tiled_screenshot`'s own outer
handler already needed adjustment (see below) to stop swallowing the re-raised
timeout into a `None` return; from there it propagates through
`WebDriverPlaywright.get_screenshot`'s existing `except PlaywrightTimeout:
raise` (`superset/utils/webdriver.py:523-524`, unchanged), through
`BaseScreenshot.get_screenshot` (`superset/utils/screenshots.py:212-221`,
unchanged, no catch), to `ReportExecutor._get_screens
hots`'s existing `except Exception`
(`superset/commands/report/execute.py:586-595`, unchanged), which raises
`ReportScheduleScreenshotFailedError`. So the only code change required for (B)
was in `screenshot_utils.py`.
- **(C) Non-tiled global spinner wait**
(`superset/utils/webdriver.py:442-454` and `:490-501`) — evaluated, **not
changed**. It is not the same defect: this path sets the browser viewport to
the full window size configured for the screenshot and does not scroll at all,
so there's no scroll-driven `IntersectionObserver` transition for the
vacuous-pass race to occur on. It also already fails loudly (`except
PlaywrightTimeout: ... raise`, confirmed by existing tests in
`webdriver_test.py`), so defect 2 doesn't apply there either. Not touched, to
keep the diff minimal per instructions.
Note: the timeout behavior for per-tile readiness changes from
warn-and-capture to fail — this is a deliberate, directly-instructed behavior
change (not a "decision not in the instructions"), called out here per the
process notes.
### TESTING INSTRUCTIONS
- `tests/unit_tests/utils/test_screenshot_utils.py`:
- `test_chart_holder_with_nothing_mounted_blocks_wait` — regression test
for the vacuous-pass race: a chart holder in viewport with nothing mounted does
not satisfy the wait.
- `test_per_tile_readiness_timeout_raises_and_skips_capture` — a per-tile
timeout raises `PlaywrightTimeout` and `page.screenshot` is never called for
that tile.
- `test_all_chart_holders_ready_passes` — all chart holders in a terminal
state (rendered or errored) passes and the tile is captured.
- `test_per_tile_readiness_wait_uses_viewport_check` — updated to assert
the new viewport-scoped, chart-holder-based JS predicate.
- Removed the old
`test_per_tile_spinner_timeout_logs_warning_and_continues`, which ratified the
previous warn-and-continue semantics; replaced by the fail-loud tests above.
- Ran `pytest tests/unit_tests/utils/test_screenshot_utils.py` (23 passed),
`pytest tests/unit_tests/utils/webdriver_test.py` (32 passed, 1
pre-existing/unrelated environment-specific failure in
`test_get_screenshot_handles_playwright_timeout` caused by a
`playwright.sync_api.TimeoutError` constructor signature mismatch in this test
venv — reproduces identically on unmodified `master` for that untouched file,
not something this PR touches), and `pytest
tests/unit_tests/commands/report/execute_test.py` (81 passed) to confirm the
`ReportScheduleScreenshotFailedError` propagation path is unaffected.
- Ran `ruff check`, `ruff format --diff`, and `mypy --check-untyped-defs` on
both changed files — no new findings versus `master` (the pre-existing mypy
findings in this file, from the `PlaywrightTimeout = Exception` / `Page = None`
`ImportError` fallbacks and a `clip` dict typing, are unchanged before/after
this diff).
### ADDITIONAL INFORMATION
Not changed, intentionally:
- Cumulative-timeout budgeting across tiles (tracked separately in
`fix-tile-wait-budget`).
- Per-tile wait duration defaults (`load_wait`/`animation_wait` signatures
and defaults unchanged).
- The non-tiled global spinner wait in `webdriver.py` (see item C above).
- [ ] 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]
