fix(reports): prevent blank/partial report PDFs from virtualized charts - #43348
fix(reports): prevent blank/partial report PDFs from virtualized charts#43348fitzee wants to merge 3 commits into
Conversation
Scheduled dashboard reports render the dashboard headlessly and screenshot it into a PDF. Tall dashboards with DashboardVirtualization active only ever mount a moving window of charts, so off-window charts are unrendered placeholders. Two capture paths handle this differently: - Tiled path: scrolls each region into view and waits per tile, so it renders correctly even with virtualization active (the good path). - Non-tiled path: takes a single full-page screenshot but its readiness gate only waits for viewport-visible holders, so below-the-fold holders are captured blank and delivered as state=Success. A large dashboard mis-routes to the non-tiled path when its scrollHeight is measured while charts are still virtualized/collapsed (0 < height <= one tile), which is why the same dashboard produces good PDFs on some runs and blank ones on others. Fix, all server-side (frontend virtualization can't be gated on standalone=Report because the Embedded SDK shares that mode): - Route large scheduled-report dashboards to the tiled path regardless of a stale short height measurement, so they can't fall into the single-shot non-tiled capture. - Harden the non-tiled report path: dispatch the client-side superset-force-all-in-view event to un-window virtualized charts, then require all mounted holders (not just viewport-visible) to reach a terminal state via a new REPORT_ALL_CHART_HOLDERS_READY_JS predicate. On timeout it raises, so a partial capture fails and retries instead of shipping a blank PDF. - Surface below-the-fold offenders in the readiness-timeout diagnostics (all_unready_holders), which the viewport-scoped scan hid as virtualized. Adds regression tests covering the tiled routing for large reports, the force-render + all-holders wait, and the fail-loud-on-partial behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
beda852 to
0958171
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43348 +/- ##
==========================================
- Coverage 66.73% 66.67% -0.06%
==========================================
Files 2876 2876
Lines 164218 164198 -20
Branches 37890 37876 -14
==========================================
- Hits 109590 109479 -111
- Misses 52469 52559 +90
- Partials 2159 2160 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dosubot
Bot
added
the
alert-reports
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| page.evaluate(FORCE_ALL_CHART_HOLDERS_IN_VIEW_JS) | ||
| readiness_predicate = REPORT_ALL_CHART_HOLDERS_READY_JS |
There was a problem hiding this comment.
Suggestion: The report readiness predicate only requires the currently mounted holders to be ready. Because dispatching the force-render event updates React state asynchronously, the existing mounted set can satisfy this predicate before additional virtualized rows mount, allowing the full-page screenshot to proceed while later charts are still absent. Require the mounted holder count to reach report_execution_context.expected_chart_count and then verify all holders are ready. [api mismatch]
Severity Level: Major ⚠️
- ❌ Non-tiled scheduled PDFs can omit later virtualized charts.
- ⚠️ Reports may falsely transition to successful delivery.
- ⚠️ The race affects forced off-screen chart mounting.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/webdriver.py
**Line:** 398:399
**Comment:**
*Api Mismatch: The report readiness predicate only requires the currently mounted holders to be ready. Because dispatching the force-render event updates React state asynchronously, the existing mounted set can satisfy this predicate before additional virtualized rows mount, allowing the full-page screenshot to proceed while later charts are still absent. Require the mounted holder count to reach `report_execution_context.expected_chart_count` and then verify all holders are ready.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct. The current implementation in the PR already addresses this by forcing all chart holders to render before the readiness check and using a non-viewport-scoped readiness predicate for non-tiled reports. Specifically, in superset/utils/webdriver.py |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Review — fix(reports): prevent blank/partial report PDFs from virtualized charts
Nice piece of work. This closes the exact blind spot the earlier three PRs (#43077 paint gate, #43031 fail-closed, #43253 auth wait) couldn't reach: the readiness scan skipped off-screen holders (if (!(r.top < window.innerHeight && r.bottom > 0)) continue;), but the non-tiled report path takes one full-page screenshot that includes below-the-fold content — so a virtualized holder that never rendered was never scanned, the predicate returned ready, and neither the paint gate nor the fail-closed check was ever consulted. The docstring names the outcome precisely ("captured blank and silently delivered as a Success"). I traced all four crux points and they hold up. Findings below, ranked; none are blocking.
1. Failure-mode: does the stricter predicate turn "blank PDF" into "no PDF"? — resolves in the PR's favor (non-blocking)
The stricter all-holders predicate (REPORT_ALL_CHART_HOLDERS_READY_JS) + FORCE_ALL_CHART_HOLDERS_IN_VIEW_JS only runs on the non-tiled path, and that path is now reached only by reports that are not likely_large_dashboard (chart_count < SCREENSHOT_TILED_CHART_THRESHOLD and dashboard_height ≤ height_threshold). Every likely-large report is routed to the tiled path by the new or report_execution_context is not None disjunct (webdriver.py:787-791), which keeps its own per-tile budget. So the added force-render work is bounded to small dashboards where nearly all holders are on-screen anyway — the "N off-screen charts × N extra renders on a huge dashboard" scenario doesn't hit this path.
The PR does not raise any timeout; it relies on existing effective_load_wait headroom. On the non-tiled path, wait_for_function polls until the predicate holds or the deadline expires; on timeout it raises and aborts before capture (webdriver.py:405-501). So yes, a small report with an off-screen holder that genuinely never renders now yields no report + a loud report_readiness_terminal log instead of a silently-blank Success. That is the correct trade — it is precisely the argument #43031 made, and a failure that pages someone beats a blank PDF delivered as success. I'd rather see this than a manufactured concern, so: no change requested here.
2. What FORCE_ALL_CHART_HOLDERS_IN_VIEW_JS mutates — safe; capture still reflects real layout (non-blocking)
The Python side mutates nothing: it dispatches a bare window event (superset-force-all-in-view). The actual work is the pre-existing frontend contract — Row.tsx:246-255 handleForceInView disconnects the two intersection observers and calls setIsInView(true), i.e. it materializes the row's real chart content; it does not touch position/transform/height/CSS. This is the same mechanism a user's own "Download as Image/PDF" triggers (downloadAsPdf.ts:66, forceLoadAllCharts), so the captured image is the same artifact that path produces — not a differently-laid-out one. The constant matches the frontend (constants.ts:65) and a listener exists (Row.tsx:264), so the dispatch is not a no-op. Good.
Minor robustness note (not blocking): the event is dispatched once. A row that mounts after the dispatch would miss it and stay virtualized — but since it runs after page-load/networkidle and the all-holders predicate would then time out (fail loud) rather than capture blank, the worst case degrades safely.
3. Tiling coupling — correct in both directions (non-blocking)
Dropping the old height_unknown or dashboard_height > tile_height gate for reports (replaced by the always-true report_execution_context is not None) is the right call: dashboard_height is measured via scrollHeight before force-in-view runs, so a large-by-count dashboard mid-layout (charts still collapsed) can measure ≤ tile_height and, under the old logic, drop to the single-shot non-tiled capture and ship a windowed partial. The new disjunct only adds tiling cases — any dashboard that tiled before still tiles — so there's no regression for the normal case, and the short-measured-height case is now covered. test_large_report_dashboard_tiles_even_when_measured_height_is_short exercises exactly this (52 charts, measured 1500px ≤ 2000px tile) and asserts take_tiled_screenshot runs and page.screenshot does not.
4. viewport_only split — the tiled path really does scroll every region into view (verified)
The claim that the tiled path can keep the viewport-scoped predicate holds: take_tiled_screenshot iterates tiles (screenshot_utils.py:609), window.scrollTo(0, scroll_y) each region into view (:620), then runs a per-tile readiness wait_for_function (:657) with the viewport-scoped body. Every region is scrolled in and waited on before capture, so the fix is complete — the blind spot was unique to the single-shot full-page path, which is the one that got the all-holders predicate.
Also confirmed the refactor of UNREADY_CHART_HOLDERS_JS_BODY into _unready_chart_holders_js_body(*, viewport_only) does not drop #43077's paint gate: hasUnpaintedEchart lives in the shared body (screenshot_utils.py:212-219), so both variants retain it; the only delta is the viewport_skip prefix.
5. Tests (Rule 26) — INSPECTED, each couples to a distinct production hunk
Could not RUN locally — the sandbox fails at collection with ModuleNotFoundError: No module named 'sqlglot.dialects.singlestore' (a dependency-version mismatch, unrelated to the PR). Leaving CI as the authority; note unit-tests (current) was still running at review time. By inspection, reverting each production hunk fails a specific test:
- Revert
... or report_execution_context is not None→test_large_report_dashboard_tiles_even_when_measured_height_is_shortfails (use_tiledgoes False,take_tilednot called /page.screenshotcalled). - Revert
page.evaluate(FORCE_ALL_CHART_HOLDERS_IN_VIEW_JS)+ predicate swap →test_report_readiness_forces_below_fold_render_and_waits_for_all_holdersfails (force call absent; predicate would carrygetBoundingClientRect/window.innerHeight). - Revert the
below_fold_unreadydiagnostic/log arg →test_report_readiness_below_fold_unrendered_fails_loudlyfails (all_unready_holders=absent).
No negative-assertion (wrong-reason) traps: the fail-loud test seeds 22 on-screen rendered holders alongside 30 off-screen virtualized/nothing_mounted ones, so a "no unready holders" pass on an empty fixture isn't possible; and the readiness test asserts the predicate is genuinely non-viewport-scoped, not merely that a constant exists.
Overall: well-reasoned, well-tested, and the trade-off it makes (loud failure over silent blank) is consistent with the rest of this family. Nothing blocking from me.
Code Review Agent Run #ce5756Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
Scheduled dashboard reports render the dashboard headlessly and screenshot it into
a PDF. On tall dashboards with
DashboardVirtualizationactive, only a movingwindow of charts is mounted, so off-window charts are unrendered placeholders.
correctly even with virtualization active (the good path).
only waits for viewport-visible holders, so below-the-fold holders are
captured blank and delivered as
state=Success.A large dashboard mis-routes to the non-tiled path when its
scrollHeightismeasured while charts are still virtualized/collapsed (
0 < height <= one tile),which is why the same dashboard yields good PDFs on some runs and blank on others.
Fix (all server-side — the frontend can't be gated on
standalone=Reportsincethe Embedded SDK shares that mode):
short height measurement, so they can't fall into the single-shot capture.
superset-force-all-in-viewevent to un-window virtualized charts, then requireall mounted holders (not just viewport-visible) to reach a terminal state via
a new
REPORT_ALL_CHART_HOLDERS_READY_JSpredicate. On timeout it raises, so apartial capture fails and retries instead of shipping a blank PDF.
(
all_unready_holders), which the viewport-scoped scan hid asvirtualized.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend screenshot pipeline change; covered by unit tests.
TESTING INSTRUCTIONS
pytest tests/unit_tests/utils/webdriver_test.py tests/unit_tests/utils/test_screenshot_utils.py.New regression tests: large report tiles despite a short measured height;
non-tiled report force-renders + waits for all holders; below-fold-unrendered
raises loudly and logs
all_unready_holders.Manual: schedule a report for a tall (> 1 viewport) dashboard with
DASHBOARD_VIRTUALIZATIONenabled and confirm the delivered PDF is complete.ADDITIONAL INFORMATION
DASHBOARD_VIRTUALIZATION(to reproduce)🤖 Generated with Claude Code