fix(reports): positive per-tile chart readiness check for tiled screenshots by eschutho · Pull Request #42119 · apache/superset

eschutho · 2026-07-16T22:21:59Z

Summary

take_tiled_screenshot() (added in #34561) captures a large dashboard tile-by-tile, waiting for each tile to finish loading before capturing it. The per-tile wait (added in #39895) polls for viewport-visible .loading elements to disappear. Two defects in that predicate could ship a blank or spinner screenshot in a scheduled report:

Vacuous-pass race: the predicate only checks for existing .loading elements. With dashboard virtualization on (default), a chart holder that has just scrolled into view but hasn't started rendering yet (its IntersectionObserver callback hasn't fired) has mounted neither a spinner nor a chart. Zero .loading elements are found, so the wait passes immediately and the tile is captured before the chart starts loading.
Silent timeout: on a per-tile timeout, the code logged a warning and captured the tile anyway, so a spinner could end up in the delivered report.

This PR replaces the predicate with a positive readiness check and makes a timeout fail the report instead of silently capturing a bad tile.

Fix

Positive readiness check. Instead of checking for the absence of .loading, we now check that every chart holder ([data-test="dashboard-component-chart-holder"]) intersecting the viewport is in a terminal state: rendered (.slice_container present, no nested .loading) or errored/empty ([role="alert"], .ant-empty, .missing-chart-container). A holder with nothing mounted yet doesn't satisfy this, closing the vacuous-pass race. A tile with no chart holders (e.g. markdown-only) passes trivially, so empty-dashboard screenshots (fix(empty dashboards): Allow downloading a screenshot of an empty dashboard #30767, fix(playwright): allow screenshotting empty dashboards #33107) still work.
Fail on timeout instead of degrading silently. A per-tile readiness timeout now re-raises PlaywrightTimeout instead of warning-and-continuing, so the report fails cleanly (ReportScheduleScreenshotFailedError) instead of delivering a spinner image. The timeout is logged at WARNING, matching the precedent set by fix(screenshots): downgrade screenshot timeout logs from ERROR to WARNING #38130 and chore(playwright): Using warning for timeouts #38441 for the other screenshot-timeout log sites — a chart failing to load in time is a data/query issue, not a system fault, so it stays at ERROR only for genuine failures (browser crash, etc.).
Better timeout diagnostics. The timeout log now includes elapsed wait time, tile index/count, the configured load_wait, and — per still-unready chart holder — its data-test-chart-id and which state it was stuck in (waiting_on_database, spinner_mounted, or nothing_mounted), so a slow query can be told apart from the virtualization race directly from the logs. A per-tile DEBUG line also logs wait time for profiling. A log_context parameter (aligned with the mechanism added in fix(reports): time-budget tiled screenshot to fail cleanly instead of hitting Celery kill #42118) threads an optional caller-supplied string through these log lines; it defaults to None and doesn't affect the thumbnail/cache-warming paths.

Why this selector, not something else

.chart-container also mounts during the pure-loading state, so it can't be used as a "ready" signal without recreating the same vacuous-pass problem one layer down.
.slice_container is the correct positive "chart has data" signal since feat: show more information when loading chart #27255, which made it mount only once loading finishes. fix(reports): Update the element class to wait for when taking a screenshot #28745 moved a different wait (the initial page locate wait, not this one) off .slice_container because of a much shorter timeout budget there — this PR's readiness check runs under the full load_wait budget, so that concern doesn't apply here.
A pure absence-of-.loading check has been tried and has failed three times in this codebase (fix(reports): poll for spinner absence instead of snapshotting loading elements #39579, fix(reports): narrow spinner checks to viewport and tighten exception handling #39895, and now this vacuous-pass race) — this PR avoids a fourth attempt at that pattern by always requiring a positive terminal-state marker, not just the absence of a spinner.

Known gap: chart errors of type MARSHMALLOW_ERROR don't render through the shared [role="alert"] wrapper the way other chart error types do. This is a payload-validation error that's very unlikely to appear during a scheduled screenshot (it's normally surfaced on save operations), so it isn't handled here — if it does occur, the tile will hit the full timeout and the report will fail rather than pass. Flagging for visibility rather than adding a narrow selector for a low-probability case.

Not changed

The non-tiled (single full-page) screenshot wait in webdriver.py — it doesn't scroll, so there's no virtualization race, and it already fails loudly on timeout.
The equivalent Selenium wait pattern — Selenium isn't used for tiled screenshots.
Cumulative timeout budgeting across tiles — see fix(reports): time-budget tiled screenshot to fail cleanly instead of hitting Celery kill #42118.
Wait duration defaults (load_wait/animation_wait).

Testing

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

Vacuous-pass regression: a chart holder with nothing mounted blocks the wait.
A per-tile timeout raises, skips the capture, and logs at WARNING (not ERROR) with the full diagnostic payload.
Log context appears on both the timeout warning and the exception-handling log line.
Unready holders are classified correctly (waiting_on_database / spinner_mounted / nothing_mounted).
Per-tile timing is logged at DEBUG.
All-holders-ready passes; the readiness check uses the viewport-scoped predicate.
Removed the old test that asserted warn-and-continue behavior, since that behavior is gone.

Also updated one exact-kwargs assertion in tests/unit_tests/utils/webdriver_test.py for the new log_context parameter.

pytest tests/unit_tests/utils/test_screenshot_utils.py tests/unit_tests/utils/screenshot_test.py tests/unit_tests/utils/webdriver_test.py tests/unit_tests/commands/report/execute_test.py — all passing (one pre-existing, unrelated environment-specific failure reproduces identically on unmodified master). ruff check, ruff format, and mypy clean on 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

…nshots `take_tiled_screenshot()` waited for the *absence* of `.loading` elements visible in the viewport before capturing each tile. With DashboardVirtualization on (default), a chart holder that has just scrolled into view but hasn't fired its IntersectionObserver callback yet mounts neither a spinner nor a chart, so the predicate passed vacuously and the tile was captured blank. On top of that, a per-tile timeout was caught, logged as a warning, and the tile was captured anyway -- delivering a spinner screenshot to report recipients instead of failing the report. Replace the absence-of-`.loading` predicate with a positive readiness check: every chart holder (`data-test="dashboard-component-chart-holder"`) intersecting the viewport must show a terminal state (a rendered chart via `.slice_container`, or an error/empty state via `[role="alert"]` / `.ant-empty` / `.missing-chart-container`) before a tile is captured. A holder with nothing mounted no longer satisfies the wait. A per-tile timeout is now logged at ERROR with the tile index, the load_wait, and the identities of the still-unready chart holders, and re-raises instead of being swallowed -- the report now fails (ReportScheduleScreenshotFailedError) instead of silently shipping a degraded screenshot. Co-Authored-By: Claude <noreply@anthropic.com>

…ion id and diagnostics The per-tile readiness timeout added in the prior commit logged at ERROR. A chart failing to load in time is a customer chart-loading issue (slow query, error state), not a Superset system fault, so downgrade it to WARNING -- matching the precedent set in #38130 and #38441 for the other screenshot timeout paths. The report still fails loudly (raise is unchanged); only the log level changes. Genuine system faults (the catch-all Exception handler) stay at ERROR/exception level. Thread an optional execution_id through ChartScreenshot/DashboardScreenshot.get_screenshot -> WebDriverProxy subclasses -> take_tiled_screenshot, populated from BaseReportState._execution_id in the report pipeline (None elsewhere, e.g. thumbnails), so every log line this change touches can be correlated back to the report execution that produced it. The readiness-timeout diagnostics now identify not just which chart holders are unready but the state each is stuck in -- "waiting_on_database" (initial query in flight, whole container replaced by a spinner), "spinner_mounted" (query finished but the chart isn't in the virtualization viewport yet, spinner nested inside an otherwise-present slice_container), or "nothing_mounted" (the vacuous-pass race the previous commit closed) -- so a slow query can be told apart from the virtualization race during an incident. Also added a per-tile DEBUG line with the time spent waiting for readiness, to profile slow dashboards from logs. Co-Authored-By: Claude <noreply@anthropic.com>

…'s log_context Renamed the execution_id parameter/suffix mechanism added in the previous commit to log_context / " [context]", matching the mechanism used by the concurrent fix-tile-wait-budget PR (#42118) for the same call chain (BaseReportState._get_screenshots -> BaseScreenshot.get_screenshot -> WebDriverProxy subclasses -> take_tiled_screenshot). Both PRs touch take_tiled_screenshot()'s signature, so using the same parameter name and suffix format keeps the eventual rebase mechanical instead of producing a semantic conflict and two different correlation-id formats in the logs. The call site in execute.py now passes log_context=f"execution_id=..." (rather than a bare execution_id kwarg), matching #42118's convention of a self-describing context string (it uses cache_key=... for thumbnails). Only the correlation-id plumbing changed -- the per-chart unready-state diagnostics (chart id + waiting_on_database/spinner_mounted/nothing_mounted) added in the previous commit are unchanged. Co-Authored-By: Claude <noreply@anthropic.com>

rusackas · 2026-07-17T18:19:11Z

Thanks @eschutho, nice write-up. CI looks like it needs some love and might be fixed by a rebase to pick up a CI retry PER I just merged.

bito-code-review · 2026-07-20T18:38:18Z

The suggestion to update the user parameter annotation in superset/utils/screenshots.py is correct. The current type hint user: User implies it is mandatory, but the implementation allows it to be passed as None from other modules. To resolve this, update the type hint to user: User | None.

Would you like me to implement this fix for you? If so, I can also check the rest of the PR for similar issues and apply fixes if you approve.

superset/utils/screenshots.py

def get_screenshot(
        self,
        user: User | None,
        window_size: WindowSize | None = None,
        log_context: str | None = None,
    ) -> bytes | None:

bito-code-review · 2026-07-20T21:39:13Z

Code Review Agent Run #136e72

Actionable Suggestions - 0

Additional Suggestions - 1

tests/unit_tests/utils/test_screenshot_utils.py - 1
- Test assertion looseness · Line 401-401
  
  The substring check on line 401 is technically correct but imprecise. The message format has a deterministic `Timed out after %.2fs...` prefix (screenshot_utils.py:254) — anchoring the assertion to that prefix rather than the generic `unready` token reduces regression risk if the message structure changes.

Filtered by Review Rules

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

superset/utils/webdriver.py - 1
- Unused parameter in method signature · Line 803-803

Review Details

Files reviewed - 6 · Commit Range: d32c2b8..ff8189f
- superset/commands/report/execute.py
- superset/utils/screenshot_utils.py
- superset/utils/screenshots.py
- superset/utils/webdriver.py
- tests/unit_tests/utils/test_screenshot_utils.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

…unrelated errors CI failures on this branch traced to two issues: 1. take_tiled_screenshot()'s outer `except PlaywrightTimeout: raise` / `except Exception: return None` pair assumed PlaywrightTimeout is always distinguishable from a generic Exception. It isn't: when the playwright package isn't installed, `PlaywrightTimeout = Exception` (see the try/except ImportError above the function). In that case *any* exception -- not just our own deliberate per-tile readiness-timeout raise -- matched `except PlaywrightTimeout` first and incorrectly propagated instead of degrading to `None` like every other unexpected error (e.g. the initial dashboard element never appearing). This is exactly what unit-tests hit in CI (no playwright installed there), turning `test_element_not_found_returns_none` and `test_exception_handling_*` into uncaught-exception failures. Fixed by tracking the "this is our own readiness timeout" case with an explicit boolean flag set right before the inner `raise`, instead of relying on exception-type dispatch that can't tell the two cases apart when the import fallback is active. 2. `execute.py` now always passes `log_context=...` to `screenshot.get_screenshot(...)`. One integration test (`test_email_chart_report_schedule_alpha_owner`) mocks `ChartScreenshot.get_screenshot` with a side_effect function whose signature only accepted `user`, so the new kwarg raised a TypeError, caught by execute.py's own error handling and reported as a ReportScheduleScreenshotFailedError in test-mysql/postgres/sqlite. Fixed by accepting the new `log_context` keyword in that test's side_effect signature. Confirmed this was the only such call site in the test suite (all others use plain `.return_value`/exception-instance side effects, which don't care about the new kwarg). Verified locally with and without playwright installed, matching both conditions CI exercises. Co-Authored-By: Claude <noreply@anthropic.com>

…superset into fix-tile-readiness-check

codecov · 2026-07-21T00:06:49Z

Codecov Report

❌ Patch coverage is 35.00000% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.24%. Comparing base (5903577) to head (ecc2bf3).
⚠️ Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/utils/screenshot_utils.py	25.00%	12 Missing ⚠️
superset/utils/screenshots.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #42119      +/-   ##
==========================================
+ Coverage   65.22%   65.24%   +0.02%     
==========================================
  Files        2768     2768              
  Lines      156280   156318      +38     
  Branches    35774    35780       +6     
==========================================
+ Hits       101933   101990      +57     
+ Misses      52382    52361      -21     
- Partials     1965     1967       +2

Flag	Coverage Δ
hive	`38.67% <20.00%> (+0.06%)`	⬆️
mysql	`57.92% <35.00%> (+0.05%)`	⬆️
postgres	`57.98% <35.00%> (+0.05%)`	⬆️
presto	`40.60% <20.00%> (+0.06%)`	⬆️
python	`59.38% <35.00%> (+0.05%)`	⬆️
sqlite	`57.58% <35.00%> (+0.05%)`	⬆️
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.

rusackas

LGTM! Going positive on readiness is the right call after the absence-of-.loading pattern burned us three times, and failing the report loudly beats mailing someone a spinner. Ran the new unit tests locally, all green, and I checked that the re-raise lands well in both callers (reports fail cleanly, thumbnails just mark the cache entry as errored).

I resolved the two CodeAnt threads, both flagged pre-existing code the diff only rewrapped. Approving, and happy to merge once you've had a look at those, unless you'd rather land it yourself.

bito-code-review · 2026-07-21T02:55:05Z

Bito Automatic Review Skipped – PR Already Merged

Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.
No action is needed if you didn't intend to review it. To get a review, you can type /review in a comment and save it

…or tile readiness Companion to the previous commit's positive per-tile readiness check, carrying the rest of PR #42119's changes onto this base: - Per-tile readiness timeout logs at WARNING (customer chart-loading issue, not a system fault -- matching #38130/#38441) with elapsed wait time, tile index/total, load_wait, and the identity + stuck-state of each unready chart holder (waiting_on_database / spinner_mounted / nothing_mounted), then re-raises so the report fails rather than shipping a blank or spinner tile. A per-tile DEBUG line logs readiness wait time for profiling. - The deliberate readiness-timeout re-raise is tracked with an explicit flag rather than `except PlaywrightTimeout` at the outer level, since PlaywrightTimeout is aliased to bare Exception when playwright isn't installed and would otherwise swallow-or-propagate the wrong cases. - Threads an optional log_context (e.g. "execution_id=<uuid>") from BaseReportState._get_screenshots through BaseScreenshot.get_screenshot and both WebDriverProxy implementations into take_tiled_screenshot, so timeout logs correlate back to the report run. Defaults to None for callers outside the report pipeline (thumbnails). Backported from #42119 (commits 37da786, f6feb70, 97d1548 squashed) onto this branch's base; only the log lines this change itself adds or touches carry the context suffix -- pre-existing log lines on this base (f-string style) are left as-is. Co-Authored-By: Claude <noreply@anthropic.com>

…verride BaseScreenshot.get_screenshot gained an optional log_context kwarg in #42119; the pooled MCP subclass's override kept the old signature, which stricter mypy configs flag as an incompatible override (Liskov) -- caught by superset-private's pre-commit and fixed there in preset-io/superset-private#996. Land the same one-liner on master: accept (and ignore) the kwarg, since the pooled Selenium path doesn't emit the per-tile readiness logs that use it. Co-Authored-By: Claude <noreply@anthropic.com>

take_tiled_screenshot crossed ruff's C901 complexity threshold (11 > 10) once the budget logic merged with the positive readiness check and fail-loud flag from #42119 -- suppress like webdriver.py's get_screenshot does. Plus ruff-format on the rebased test file. Co-Authored-By: Claude <noreply@anthropic.com>

The non-tiled/standard screenshot path in WebDriverPlaywright.get_screenshot() had the same vacuous-pass defect that #42119 fixed for tiled screenshots: it waited for document.querySelectorAll('.loading').length === 0, which is satisfied immediately if no chart has mounted a spinner yet (e.g. in the gap between page-navigation-complete and React/query bootstrap), silently capturing a blank screenshot with no timeout or error. Replace it with the same positive terminal-state predicate #42119 introduced for the tiled path (CHART_HOLDERS_READY_JS / FIND_UNREADY_CHART_HOLDERS_JS in screenshot_utils.py, now exported and reused verbatim instead of reimplemented) via a new WebDriverPlaywright._wait_for_charts_ready() helper, deduplicating the two copies of this wait that existed in get_screenshot(). On timeout it logs per-chart diagnostics (chart id + state) at WARNING and re-raises instead of capturing. Co-Authored-By: Claude <noreply@anthropic.com>

pull-request-size Bot added the size/L label Jul 16, 2026

dosubot Bot added the alert-reports Namespace | Anything related to the Alert & Reports feature label Jul 16, 2026

eschutho mentioned this pull request Jul 16, 2026

fix(screenshots): validate cached screenshot image bytes on read and write #42120

Merged

9 tasks

github-actions Bot added the preset-io label Jul 17, 2026

Merge branch 'master' into fix-tile-readiness-check

ff8189f

codeant-ai-for-open-source Bot reviewed Jul 20, 2026

View reviewed changes

Comment thread superset/utils/screenshots.py

codeant-ai-for-open-source Bot reviewed Jul 20, 2026

View reviewed changes

Comment thread superset/commands/report/execute.py

eschutho and others added 2 commits July 20, 2026 23:45

Merge branch 'fix-tile-readiness-check' of https://github.com/apache/…

ecc2bf3

…superset into fix-tile-readiness-check

rusackas approved these changes Jul 21, 2026

View reviewed changes

eschutho merged commit 4b659da into master Jul 21, 2026
63 of 64 checks passed

eschutho deleted the fix-tile-readiness-check branch July 21, 2026 00:31

eschutho mentioned this pull request Jul 21, 2026

fix(reports): positive readiness check for non-tiled screenshots #42253

Merged

9 tasks

eschutho mentioned this pull request Jul 21, 2026

fix(reports): tile readiness check backport on 6.0-release-era base (report-fixes-42119) #42278

Closed

9 tasks

eschutho mentioned this pull request Jul 23, 2026

fix(reports): time-budget tiled screenshot to fail cleanly instead of hitting Celery kill #42118

Merged

6 tasks

sadpandajoe mentioned this pull request Jul 24, 2026

fix(mcp): align pooled screenshot override signature #42384

Merged

9 tasks

fitzee mentioned this pull request Jul 26, 2026

fix(reports): make #42253 screenshot readiness production-safe #42427

Merged

9 tasks

Navigation Menu

Uh oh!

fix(reports): positive per-tile chart readiness check for tiled screenshots - #42119

fix(reports): positive per-tile chart readiness check for tiled screenshots#42119
eschutho merged 6 commits into
masterfrom
fix-tile-readiness-check

eschutho commented Jul 16, 2026 •

edited

Loading

Uh oh!

rusackas commented Jul 17, 2026

Uh oh!

Uh oh!

bito-code-review Bot commented Jul 20, 2026

Uh oh!

Uh oh!

bito-code-review Bot commented Jul 20, 2026 •

edited

Loading

Code Review Agent Run #136e72

Uh oh!

codecov Bot commented Jul 21, 2026 •

edited

Loading

Uh oh!

rusackas left a comment

Uh oh!

Uh oh!

bito-code-review Bot commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

eschutho commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fix

Why this selector, not something else

Not changed

Testing

Additional information

Uh oh!

rusackas commented Jul 17, 2026

Uh oh!

Uh oh!

bito-code-review Bot commented Jul 20, 2026

Uh oh!

Uh oh!

bito-code-review Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #136e72

Uh oh!

codecov Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rusackas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bito-code-review Bot commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eschutho commented Jul 16, 2026 •

edited

Loading

bito-code-review Bot commented Jul 20, 2026 •

edited

Loading

codecov Bot commented Jul 21, 2026 •

edited

Loading